fix project instruction review findings
This commit is contained in:
@@ -53,8 +53,8 @@ dsh-subagent-acp ← dsh-subagent, dsh-agent, dsh-llm, @agentclientprotocol/sdk
|
||||
dsh-tool-subagent ← dsh-subagent, dsh-tools, dsh-agent, dsh-llm (model-facing delegation tool)
|
||||
dsh-tool-todo ← dsh-tools, dsh-agent, dsh-session (model-facing todo_write tool; whole list on the session log)
|
||||
dsh-agent-core ← timer, dsh-llm, dsh-session, dsh-system-prompt, dsh-tools, dsh-agent, dsh-invariants, dsh-tool-bash, dsh-project-instructions, dsh-agent-loop (the providerless spine, as one bundle plugin)
|
||||
dsh-stdio-agent ← dsh-agent-core, dsh-ui-stdio, dsh-session-persistence-jsonl, dsh-agent, dsh-session (stdio chat APP + bin)
|
||||
dsh-acp-agent ← dsh-agent-core, dsh-acp, dsh-session-persistence-jsonl (ACP server APP + bin)
|
||||
dsh-stdio-agent ← dsh-agent-core, dsh-ui-stdio, dsh-session-persistence-jsonl, dsh-agent, dsh-session, dsh-project-instructions (stdio chat APP + bin)
|
||||
dsh-acp-agent ← dsh-agent-core, dsh-acp, dsh-session-persistence-jsonl, dsh-project-instructions (ACP server APP + bin)
|
||||
```
|
||||
|
||||
The rule: **extension** plugins depend on interfaces, never on the concrete loop. `dsh-agent-loop` is swappable — UI/hook/tool plugins keep working against the `dsh-agent` vocabulary if the loop is replaced. The sanctioned exception is a **composition/bundle** package like `dsh-agent-core`, whose whole job is to assemble the concrete spine: it depends on `dsh-agent-loop` (and the other concrete spine plugins) on purpose. The rule constrains plugins that EXTEND the system, not the bundle that COMPOSES it — swapping the loop means shipping a different bundle, not rewiring every extension. A swappable capability splits into interface / implementation / consumer packages (the bash trio is the template — see [capability seams](../docs/rfc/implemented/architecture/2026-06-13-capability-seams.md)).
|
||||
|
||||
@@ -31,6 +31,7 @@
|
||||
"schemastery": "^3.18.0"
|
||||
},
|
||||
"devDependencies": {
|
||||
"@cordisjs/plugin-loader": "workspace:^",
|
||||
"@deepseek-ai/dsh-agent": "workspace:^",
|
||||
"@deepseek-ai/dsh-agent-loop": "workspace:^",
|
||||
"@deepseek-ai/dsh-llm": "workspace:^",
|
||||
|
||||
@@ -6,13 +6,13 @@
|
||||
* @module @deepseek-ai/dsh-project-instructions
|
||||
*/
|
||||
|
||||
import { readFile, stat } from 'node:fs/promises'
|
||||
import { lstat, readFile, stat } from 'node:fs/promises'
|
||||
import { dirname, join, relative, resolve } from 'node:path'
|
||||
import type { Context } from 'cordis'
|
||||
import z from 'schemastery'
|
||||
import type { GenerateOptions, Message } from '@deepseek-ai/dsh-llm'
|
||||
import type { Agent } from '@deepseek-ai/dsh-agent'
|
||||
import { DEFAULT_DSH_HOME_DISPLAY, defaultDshHome, expandHomePath } from '@deepseek-ai/dsh-paths'
|
||||
import { DEFAULT_DSH_HOME_DISPLAY, defaultDshHome, resolveDshHome } from '@deepseek-ai/dsh-paths'
|
||||
|
||||
export const name = 'project-instructions'
|
||||
|
||||
@@ -35,7 +35,7 @@ export interface Config {
|
||||
}
|
||||
|
||||
export const Config: z<Config> = z.object({
|
||||
dshHome: z.string().default(defaultDshHome()),
|
||||
dshHome: z.string(),
|
||||
projectRootMarkers: z.array(z.string()).default([...DEFAULT_PROJECT_ROOT_MARKERS]),
|
||||
baselineMaxBytes: z.number().default(DEFAULT_BASELINE_MAX_BYTES),
|
||||
enableClaudeFallback: z.boolean().default(true),
|
||||
@@ -98,7 +98,7 @@ interface LoadOptions extends DiscoverOptions {
|
||||
|
||||
function resolveConfig(config: Config): ResolvedConfig {
|
||||
return {
|
||||
dshHome: resolve(expandHomePath(config.dshHome ?? defaultDshHome())),
|
||||
dshHome: resolveDshHome(config.dshHome),
|
||||
projectRootMarkers: config.projectRootMarkers ?? [...DEFAULT_PROJECT_ROOT_MARKERS],
|
||||
baselineMaxBytes: config.baselineMaxBytes ?? DEFAULT_BASELINE_MAX_BYTES,
|
||||
enableClaudeFallback: config.enableClaudeFallback ?? true,
|
||||
@@ -110,12 +110,16 @@ function byteLength(value: string): number {
|
||||
}
|
||||
|
||||
function truncateUtf8(value: string, maxBytes: number): string {
|
||||
return Buffer.from(value, 'utf8').subarray(0, Math.max(0, maxBytes)).toString('utf8')
|
||||
let truncated = Buffer.from(value, 'utf8').subarray(0, Math.max(0, maxBytes)).toString('utf8')
|
||||
while (byteLength(truncated) > maxBytes) {
|
||||
truncated = truncated.slice(0, -1)
|
||||
}
|
||||
return truncated
|
||||
}
|
||||
|
||||
async function statFile(path: string): Promise<FileSignature | undefined> {
|
||||
try {
|
||||
const info = await stat(path)
|
||||
const info = await lstat(path)
|
||||
if (!info.isFile()) return undefined
|
||||
return { mtimeMs: info.mtimeMs, size: info.size }
|
||||
} catch {
|
||||
@@ -234,7 +238,7 @@ async function readCached(path: string, signature: FileSignature, cache: Instruc
|
||||
|
||||
export async function loadBaselineInstructions(options: LoadOptions): Promise<RenderedProjectInstructions | undefined> {
|
||||
const config = resolveConfig(options)
|
||||
if (config.baselineMaxBytes === 0) return undefined
|
||||
if (config.baselineMaxBytes <= 0 || !Number.isFinite(config.baselineMaxBytes)) return undefined
|
||||
const cache = options.cache ?? new Map<string, CachedContent>()
|
||||
const discovered = await discoverInstructionFiles(options)
|
||||
const loaded: LoadedInstructionFile[] = []
|
||||
@@ -311,21 +315,26 @@ function truncateToFit(
|
||||
}
|
||||
|
||||
export function renderProjectInstructions(files: LoadedInstructionFile[], options: { maxBytes: number }): RenderedProjectInstructions {
|
||||
if (options.maxBytes <= 0) return { text: '', omitted: files, truncated: [] }
|
||||
if (options.maxBytes <= 0 || !Number.isFinite(options.maxBytes)) return { text: '', omitted: files, truncated: [] }
|
||||
|
||||
const fullText = buildInstructionText(files, options.maxBytes, [], [])
|
||||
if (byteLength(fullText) <= options.maxBytes) {
|
||||
return { text: fullText, omitted: [], truncated: [] }
|
||||
}
|
||||
|
||||
for (let start = 1; start < files.length; start += 1) {
|
||||
const included = files.slice(start)
|
||||
const omitted = files.slice(0, start).map(file => ({ absolutePath: file.absolutePath, displayPath: file.displayPath }))
|
||||
const suffixText = buildInstructionText(included, options.maxBytes, omitted, [])
|
||||
if (byteLength(suffixText) <= options.maxBytes) {
|
||||
return { text: suffixText, omitted, truncated: [] }
|
||||
}
|
||||
}
|
||||
|
||||
const mostSpecific = files.at(-1)
|
||||
/* v8 ignore next -- callers only reach this after a non-empty fullText was built. */
|
||||
if (mostSpecific === undefined) return { text: '', omitted: [], truncated: [] }
|
||||
const omitted = files.slice(0, -1).map(file => ({ absolutePath: file.absolutePath, displayPath: file.displayPath }))
|
||||
const mostSpecificOnly = buildInstructionText([mostSpecific], options.maxBytes, omitted, [])
|
||||
if (byteLength(mostSpecificOnly) <= options.maxBytes) {
|
||||
return { text: mostSpecificOnly, omitted, truncated: [] }
|
||||
}
|
||||
|
||||
for (const intro of [WORKSPACE_CONTEXT_INTRO, COMPACT_WORKSPACE_CONTEXT_INTRO]) {
|
||||
const truncatedFile = truncateToFit(mostSpecific, [], options.maxBytes, omitted, intro)
|
||||
@@ -360,7 +369,7 @@ export function apply(ctx: Context, config: Config): void {
|
||||
const resolved = resolveConfig(config)
|
||||
const cache: InstructionContentCache = new Map()
|
||||
ctx.on('agent/request', async (agent: Agent, _turn: number, _step: number, request: GenerateOptions, next) => {
|
||||
if (resolved.baselineMaxBytes === 0) return next()
|
||||
if (resolved.baselineMaxBytes <= 0 || !Number.isFinite(resolved.baselineMaxBytes)) return next()
|
||||
/* v8 ignore next -- stdio compatibility fallback; tests avoid process.chdir() because cwd is process-global. */
|
||||
const cwd = agent.session.header.cwd ?? process.cwd()
|
||||
const instructions = await loadBaselineInstructions({
|
||||
|
||||
@@ -1,8 +1,10 @@
|
||||
import { chmod, mkdtemp, mkdir, rm, writeFile } from 'node:fs/promises'
|
||||
import { chmod, mkdtemp, mkdir, rm, symlink, writeFile } from 'node:fs/promises'
|
||||
import { join } from 'node:path'
|
||||
import { tmpdir } from 'node:os'
|
||||
import { describe, expect, it, vi } from 'vitest'
|
||||
import { Context } from 'cordis'
|
||||
import Loader from '@cordisjs/plugin-loader'
|
||||
import * as projectInstructions from '@deepseek-ai/dsh-project-instructions'
|
||||
import type { GenerateOptions } from '@deepseek-ai/dsh-llm'
|
||||
import { Session, SessionId, SESSION_FORMAT_VERSION } from '@deepseek-ai/dsh-session'
|
||||
import type { Agent } from '@deepseek-ai/dsh-agent'
|
||||
@@ -148,6 +150,27 @@ describe('project instruction discovery', () => {
|
||||
}
|
||||
})
|
||||
|
||||
it('rejects symlinked instruction files instead of following repository-controlled links', async () => {
|
||||
const root = await tempRepo()
|
||||
const home = await tempRepo()
|
||||
const outside = await tempRepo()
|
||||
try {
|
||||
await mkdir(join(root, '.git'), { recursive: true })
|
||||
await write(join(outside, 'secret.txt'), 'outside secret')
|
||||
await symlink(join(outside, 'secret.txt'), join(root, 'AGENTS.md'))
|
||||
|
||||
const files = await discoverBaselineInstructionFiles({ cwd: root, dshHome: home })
|
||||
const loaded = await loadBaselineInstructions({ cwd: root, dshHome: home })
|
||||
|
||||
expect(files).toEqual([])
|
||||
expect(loaded).toBeUndefined()
|
||||
} finally {
|
||||
await rm(root, { recursive: true, force: true })
|
||||
await rm(home, { recursive: true, force: true })
|
||||
await rm(outside, { recursive: true, force: true })
|
||||
}
|
||||
})
|
||||
|
||||
it('disables baseline loading when the byte budget is zero', async () => {
|
||||
const root = await tempRepo()
|
||||
const home = await tempRepo()
|
||||
@@ -195,6 +218,23 @@ describe('project instruction discovery', () => {
|
||||
}
|
||||
})
|
||||
|
||||
it('honors DSH_HOME when dshHome is not configured explicitly', async () => {
|
||||
const root = await tempRepo()
|
||||
const envHome = await tempRepo()
|
||||
try {
|
||||
await write(join(envHome, 'AGENTS.md'), 'env global rule')
|
||||
vi.stubEnv('DSH_HOME', envHome)
|
||||
|
||||
const files = await discoverBaselineInstructionFiles({ cwd: root })
|
||||
|
||||
expect(files).toEqual([{ absolutePath: join(envHome, 'AGENTS.md'), displayPath: '$DSH_HOME/AGENTS.md' }])
|
||||
} finally {
|
||||
vi.unstubAllEnvs()
|
||||
await rm(root, { recursive: true, force: true })
|
||||
await rm(envHome, { recursive: true, force: true })
|
||||
}
|
||||
})
|
||||
|
||||
it('labels the default DSH home as ~/.dsh when HOME points at the configured default', async () => {
|
||||
const root = await tempRepo()
|
||||
const home = await tempRepo()
|
||||
@@ -322,6 +362,21 @@ describe('project instruction rendering', () => {
|
||||
expect(rendered.truncated).toEqual([])
|
||||
})
|
||||
|
||||
it('keeps the longest most-specific suffix that fits under the byte budget', () => {
|
||||
const rendered = renderProjectInstructions([
|
||||
{ absolutePath: '/repo/AGENTS.md', displayPath: 'AGENTS.md', content: 'root '.repeat(200) },
|
||||
{ absolutePath: '/repo/pkg/AGENTS.md', displayPath: 'pkg/AGENTS.md', content: 'package rule' },
|
||||
{ absolutePath: '/repo/pkg/app/AGENTS.md', displayPath: 'pkg/app/AGENTS.md', content: 'app rule' },
|
||||
], { maxBytes: 760 })
|
||||
|
||||
expect(rendered.text).toContain('omitted AGENTS.md')
|
||||
expect(rendered.text).toContain('## pkg/AGENTS.md\n\npackage rule')
|
||||
expect(rendered.text).toContain('## pkg/app/AGENTS.md\n\napp rule')
|
||||
expect(rendered.text).not.toContain('root root')
|
||||
expect(rendered.omitted.map(item => item.displayPath)).toEqual(['AGENTS.md'])
|
||||
expect(rendered.truncated).toEqual([])
|
||||
})
|
||||
|
||||
it('truncates a single oversized file to the largest content slice that fits', () => {
|
||||
const rendered = renderProjectInstructions([
|
||||
{ absolutePath: '/repo/AGENTS.md', displayPath: 'AGENTS.md', content: 'x'.repeat(1000) },
|
||||
@@ -366,6 +421,14 @@ describe('project instruction rendering', () => {
|
||||
expect(rendered.truncated).toEqual([{ displayPath: 'pkg/AGENTS.md', originalBytes: 1000, includedBytes: 0 }])
|
||||
expect(Buffer.byteLength(rendered.text, 'utf8')).toBe(20)
|
||||
})
|
||||
|
||||
it('keeps compact truncation notices within budget when a multibyte display path is cut', () => {
|
||||
const rendered = renderProjectInstructions([
|
||||
{ absolutePath: '/repo/路径/AGENTS.md', displayPath: '路径/AGENTS.md', content: 'x'.repeat(1000) },
|
||||
], { maxBytes: 53 })
|
||||
|
||||
expect(Buffer.byteLength(rendered.text, 'utf8')).toBeLessThanOrEqual(53)
|
||||
})
|
||||
})
|
||||
|
||||
describe('project instruction request injection', () => {
|
||||
@@ -492,6 +555,28 @@ describe('project instruction request injection', () => {
|
||||
}
|
||||
})
|
||||
|
||||
it('does not inject an empty workspace-context message when baselineMaxBytes is negative', async () => {
|
||||
const root = await tempRepo()
|
||||
const home = await tempRepo()
|
||||
try {
|
||||
await mkdir(join(root, '.git'), { recursive: true })
|
||||
await write(join(root, 'AGENTS.md'), 'repo rule')
|
||||
const ctx = new Context()
|
||||
await ctx.plugin({ name: 'project-instructions', apply }, { dshHome: home, baselineMaxBytes: -1 })
|
||||
|
||||
const request: GenerateOptions = {
|
||||
model: 'mock',
|
||||
messages: [{ role: 'user', content: [{ type: 'text', text: 'actual prompt' }] }],
|
||||
}
|
||||
const result = await ctx.waterfall('agent/request', stubAgent(root), 1, 1, request, async () => request)
|
||||
|
||||
expect(result.messages).toEqual([{ role: 'user', content: [{ type: 'text', text: 'actual prompt' }] }])
|
||||
} finally {
|
||||
await rm(root, { recursive: true, force: true })
|
||||
await rm(home, { recursive: true, force: true })
|
||||
}
|
||||
})
|
||||
|
||||
it('leaves the request unchanged when no instruction files are present', async () => {
|
||||
const root = await tempRepo()
|
||||
const home = await tempRepo()
|
||||
@@ -527,7 +612,7 @@ describe('project instruction request injection', () => {
|
||||
}
|
||||
})
|
||||
|
||||
it('reuses the discovery stat signature when reading cached content', async () => {
|
||||
it('reuses the discovery lstat signature when reading cached content', async () => {
|
||||
const root = await tempRepo()
|
||||
const home = await tempRepo()
|
||||
try {
|
||||
@@ -540,9 +625,9 @@ describe('project instruction request injection', () => {
|
||||
const actual = await importOriginal<typeof import('node:fs/promises')>()
|
||||
return {
|
||||
...actual,
|
||||
stat: async (path: string) => {
|
||||
lstat: async (path: string) => {
|
||||
observedStats.set(path, (observedStats.get(path) ?? 0) + 1)
|
||||
return actual.stat(path)
|
||||
return actual.lstat(path)
|
||||
},
|
||||
}
|
||||
})
|
||||
@@ -562,3 +647,17 @@ describe('project instruction request injection', () => {
|
||||
}
|
||||
})
|
||||
})
|
||||
|
||||
describe('project instruction plugin export shape', () => {
|
||||
it('has the namespace-plugin export shape (no stray default) so the Loader keeps name/Config/apply', () => {
|
||||
expect('default' in projectInstructions).toBe(false)
|
||||
expect(typeof projectInstructions.apply).toBe('function')
|
||||
|
||||
const loader = Object.create(Loader.prototype) as Loader
|
||||
const unwrapped = loader.unwrapExports(projectInstructions) as Record<string, unknown>
|
||||
expect(unwrapped).toBe(projectInstructions)
|
||||
expect(unwrapped.name).toBe('project-instructions')
|
||||
expect(unwrapped.Config).toBeDefined()
|
||||
expect(typeof unwrapped.apply).toBe('function')
|
||||
})
|
||||
})
|
||||
|
||||
@@ -5,7 +5,7 @@
|
||||
*/
|
||||
|
||||
import { homedir } from 'node:os'
|
||||
import { join } from 'node:path'
|
||||
import { join, resolve } from 'node:path'
|
||||
|
||||
/** Directory name for the default DeepSeek Harness home under the OS home. */
|
||||
export const DSH_HOME_DIR_NAME = '.dsh'
|
||||
@@ -13,6 +13,9 @@ export const DSH_HOME_DIR_NAME = '.dsh'
|
||||
/** Stable user-facing display form for the default DeepSeek Harness home. */
|
||||
export const DEFAULT_DSH_HOME_DISPLAY = `~/${DSH_HOME_DIR_NAME}`
|
||||
|
||||
/** Environment variable that overrides the default DeepSeek Harness home. */
|
||||
export const DSH_HOME_ENV = 'DSH_HOME'
|
||||
|
||||
/** Resolve the default DeepSeek Harness home using Node's platform path rules. */
|
||||
export function defaultDshHome(): string {
|
||||
return join(homedir(), DSH_HOME_DIR_NAME)
|
||||
@@ -24,3 +27,9 @@ export function expandHomePath(path: string): string {
|
||||
if (path.startsWith('~/') || path.startsWith('~\\')) return join(homedir(), path.slice(2))
|
||||
return path
|
||||
}
|
||||
|
||||
/** Resolve an explicitly configured, env-selected, or default DSH home path. */
|
||||
export function resolveDshHome(configured?: string, env: Record<string, string | undefined> = process.env): string {
|
||||
const selected = configured ?? env[DSH_HOME_ENV] ?? defaultDshHome()
|
||||
return resolve(expandHomePath(selected))
|
||||
}
|
||||
|
||||
@@ -6,6 +6,7 @@ import {
|
||||
DSH_HOME_DIR_NAME,
|
||||
defaultDshHome,
|
||||
expandHomePath,
|
||||
resolveDshHome,
|
||||
} from '@deepseek-ai/dsh-paths'
|
||||
|
||||
describe('dsh path helpers', () => {
|
||||
@@ -22,4 +23,12 @@ describe('dsh path helpers', () => {
|
||||
expect(expandHomePath('/tmp/.dsh')).toBe('/tmp/.dsh')
|
||||
expect(expandHomePath('~other/.dsh')).toBe('~other/.dsh')
|
||||
})
|
||||
|
||||
it('resolves explicit DSH home before environment and default locations', () => {
|
||||
const envHome = join(homedir(), 'env-dsh')
|
||||
|
||||
expect(resolveDshHome(undefined, { DSH_HOME: '~/env-dsh' })).toBe(envHome)
|
||||
expect(resolveDshHome('/tmp/explicit-dsh', { DSH_HOME: '~/env-dsh' })).toBe('/tmp/explicit-dsh')
|
||||
expect(resolveDshHome(undefined, {})).toBe(defaultDshHome())
|
||||
})
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user