From b92795a73126e2ff163cd1e78bd4d98b54d8c21d Mon Sep 17 00:00:00 2001 From: Yichen Jiang Date: Sun, 5 Jul 2026 21:01:42 +0800 Subject: [PATCH] Fix nested project instruction review findings --- .../2026-06-24-project-instruction-files.md | 6 +- .../prompt/project-instructions/README.md | 8 +- .../prompt/project-instructions/src/index.ts | 80 ++++++++++++-- .../tests/project-instructions.spec.ts | 103 +++++++++++++++++- 4 files changed, 179 insertions(+), 18 deletions(-) diff --git a/docs/rfc/implemented/feature/2026-06-24-project-instruction-files.md b/docs/rfc/implemented/feature/2026-06-24-project-instruction-files.md index cc00aa5c60..411a870a55 100644 --- a/docs/rfc/implemented/feature/2026-06-24-project-instruction-files.md +++ b/docs/rfc/implemented/feature/2026-06-24-project-instruction-files.md @@ -42,7 +42,7 @@ If the user launches from the repository root, only the root directory is in the ### Nested discovery after file tools -The plugin observes successful `read`, `write`, and `edit` calls through `tools/post-execute`. For a touched file under the session cwd, it checks the directory chain from just below the session cwd through the touched file's parent directory, using the same file-name precedence as baseline discovery. Newly discovered nested files are attached as `additionalContext` so the loop records them after the tool result as durable `context/message` events for the next request. A per-session loaded-path set suppresses duplicate nested injections even if file content is evicted from the content cache. +The plugin observes successful `read`, `write`, and `edit` calls through `tools/post-execute`. For a touched file under the session cwd, it checks the directory chain from just below the session cwd through the touched file's parent directory, using the same file-name precedence as baseline discovery. Newly discovered nested files are attached as `additionalContext` so the loop records them after the tool result as durable `context/message` events for the next request. Visible session context suppresses duplicate nested injections even if file content is evicted from the content cache; compaction that replaces a nested context message out of the visible surface allows a later structured file touch to re-load the applicable instruction file. Shell commands are not a trigger. `dsh-bash-local` runs each command in a fresh shell and does not persist shell cwd, and parsing `cd subdir && cat file` reliably would require shell semantics the harness does not own. If bash-driven path discovery becomes necessary, it should be a separate design over an explicit path-reporting contract rather than a heuristic bolted onto this plugin. @@ -52,6 +52,8 @@ Baseline instructions are rendered as full text, not summarized. These files are The plugin injects baseline instructions through the `agent/request` waterfall by prepending a synthetic workspace-context message to `GenerateOptions.messages`. It deliberately does not register a global `ctx.systemPrompt.section()` because that service has no per-agent/cwd dimension. It also deliberately does not append to `GenerateOptions.system`: repository files are workspace-provided context, and in cloned or third-party repositories they may be attacker-controlled. They should guide the model, but they must not be represented as top-authority system instructions. +Because `agent/request` currently has no request-kind marker, baseline injection also applies to maintenance model calls such as compaction summarization. The implementation should not sniff the summarization prompt text to special-case this; a future request marker should let prompt-context plugins opt out of non-user-facing calls explicitly. + The rendered block uses an explicit envelope that says the content came from local instruction files, is lower authority than system/developer/direct user instructions, and must not override safety, permission, or secret-handling rules. The direct user prompt remains later in the message list, so normal conversational precedence still lets the user override repo guidance. The rendered shape is: @@ -92,7 +94,7 @@ The implementation should not cache a rendered block for the lifetime of the pro ### Source and role -Project instruction files enter the model as synthetic workspace context, not as provider system text. Baseline files are recomputed from disk for each request and are not durable session events, so changing a baseline instruction file affects future requests without rewriting the event log. Nested files discovered after file-tool touches are durable `context/message` events because they describe path-specific context the agent learned during the session; replay and resume should preserve that fact. Tests therefore need both request-shape coverage for baseline injection and tool-execution coverage for nested `additionalContext`. +Project instruction files enter the model as synthetic workspace context, not as provider system text. Baseline files are recomputed from disk for each request and are not durable session events, so changing a baseline instruction file affects future requests without rewriting the event log. Nested files discovered after file-tool touches are durable `context/message` events because they describe path-specific context the agent learned during the session; replay and resume should preserve that fact. Duplicate suppression should derive from the visible session surface, not only from live in-memory state: resumed sessions must not re-inject still-visible nested context, while compaction that replaces a nested context message out of the surface should allow a later structured file touch to re-load the applicable nested instructions. Tests therefore need both request-shape coverage for baseline injection and tool-execution coverage for nested `additionalContext`. ## Alternatives considered diff --git a/packages/prompt/project-instructions/README.md b/packages/prompt/project-instructions/README.md index 3d316e7b27..e79d9755ae 100644 --- a/packages/prompt/project-instructions/README.md +++ b/packages/prompt/project-instructions/README.md @@ -6,11 +6,13 @@ Project instruction file loader for the harness. It discovers `AGENTS.md` with ` The plugin listens on the `agent/request` waterfall and depends on the `ctx.fs` provider seam to read instruction file content. For each request it derives the workspace from `agent.session.header.cwd`; if the session has no cwd, it falls back to `process.cwd()` for single-session local/stdio runs. It then finds the project root by walking upward until it sees `.git` as either a directory or a file, considers the ancestor chain from project root to cwd, and loads at most one instruction file per directory: `AGENTS.md` wins, `CLAUDE.md` is a compatibility fallback. -The plugin also listens on `tools/post-execute` for successful structured filesystem touches from the first-party `read`, `write`, and `edit` tools. When one of those tools touches a descendant of the session cwd, the plugin checks the directories between the session cwd and the touched file for instruction files that were not already loaded in that session, then attaches them as `additionalContext` so the loop records a durable `context/message` for the next model request. This intentionally follows file-tool touches, not shell `cd`: `dsh-bash-local` uses fresh shells per call, and parsing arbitrary shell commands for reached paths would be brittle. +The plugin also listens on `tools/post-execute` for successful structured filesystem touches from the first-party `read`, `write`, and `edit` tools. When one of those tools touches a descendant of the session cwd, the plugin checks the directories between the session cwd and the touched file for instruction files that are not already visible in session context, then attaches them as `additionalContext` so the loop records a durable `context/message` for the next model request. This intentionally follows file-tool touches, not shell `cd`: `dsh-bash-local` uses fresh shells per call, and parsing arbitrary shell commands for reached paths would be brittle. User-global instructions live at `$DSH_HOME/AGENTS.md`; `$DSH_HOME` defaults to `~/.dsh`. A configured `~`, `~/...`, or Windows-style `~\...` prefix is expanded against the operating-system home directory before resolution. The user-global file renders before project files, so deeper project files appear later in the context and can override broader guidance. -Baseline files are inserted as a synthetic user-role workspace-context message, not as provider system text and not as persisted session events. Nested files discovered after structured file tools run are inserted through the existing `context/message` path, so they persist with the session and resume like other plugin-provided context. The rendered envelope states that these files are workspace-provided guidance, lower authority than system/developer/direct user instructions, and must not override safety, permission, or secret-handling rules. +Baseline files are inserted as a synthetic user-role workspace-context message, not as provider system text and not as persisted session events. Nested files discovered after structured file tools run are inserted through the existing `context/message` path, so they persist with the session and resume like other plugin-provided context. Nested duplicate suppression is derived from the visible session surface plus a short pending window before the loop records `additionalContext`; if compaction removes a nested context message from the surface, a later structured file touch may re-load it so the next model request still sees the applicable guidance. The rendered envelope states that these files are workspace-provided guidance, lower authority than system/developer/direct user instructions, and must not override safety, permission, or secret-handling rules. + +The baseline hook currently runs for every `agent/request`, including maintenance model calls such as compaction summarization. `GenerateOptions` does not yet carry a request-kind marker, so the plugin cannot distinguish user-facing turns from summarization without brittle prompt sniffing. A future request marker should let prompt-context plugins opt out of maintenance calls deliberately. ## Config @@ -29,7 +31,7 @@ export interface Config { The renderer keeps full text until the configured byte budget is exceeded. When it must trim, it preserves more-specific files first, drops whole less-specific files before truncating a more-specific file, and emits an HTML comment naming omitted and truncated files with byte counts. -Discovery re-walks the applicable ancestor chain on every request so newly created baseline files are noticed. File content is cached by normalized absolute path plus the provider's opaque file version and size; a changed signature causes a re-read. The discovery pass carries the file signature forward to the read pass, so a cache hit does not stat the same instruction file twice in one request. Nested instruction paths are tracked separately per live session so cache eviction or repeated reads do not duplicate the same durable context. +Discovery re-walks the applicable ancestor chain on every request so newly created baseline files are noticed. File content is cached by normalized absolute path plus the provider's opaque file version and size; a changed signature causes a re-read. The discovery pass carries the file signature forward to the read pass, so a cache hit does not stat the same instruction file twice in one request. Nested instruction paths are de-duplicated from recorded session context rather than from the content cache, so cache eviction or repeated reads do not duplicate still-visible durable context. ## Non-goals diff --git a/packages/prompt/project-instructions/src/index.ts b/packages/prompt/project-instructions/src/index.ts index 9063387037..b841ce6a6b 100644 --- a/packages/prompt/project-instructions/src/index.ts +++ b/packages/prompt/project-instructions/src/index.ts @@ -104,7 +104,8 @@ interface LoadOptions extends DiscoverOptions { interface NestedLoadOptions extends LoadOptions { touchedPath: string - loadedPaths: Set + loadedDisplayPaths: Set + pendingDisplayPaths: Set } function resolveConfig(config: Config): ResolvedConfig { @@ -290,7 +291,7 @@ async function discoverNestedInstructionFiles(options: NestedLoadOptions, fileSy const files: DiscoveredInstructionFile[] = [] for (const dir of descendantDirsBetween(cwd, options.touchedPath)) { const file = await firstExistingInstructionFile(dir, projectRoot, config.enableClaudeFallback, fileSystem) - if (file !== undefined && !options.loadedPaths.has(file.absolutePath)) files.push(file) + if (file !== undefined && !options.loadedDisplayPaths.has(file.displayPath)) files.push(file) } return files } @@ -354,12 +355,16 @@ async function loadNestedInstructions( if (content !== undefined) loaded.push({ absolutePath: file.absolutePath, displayPath: file.displayPath, content }) } if (loaded.length === 0) return undefined - for (const file of loaded) options.loadedPaths.add(file.absolutePath) + for (const file of loaded) options.pendingDisplayPaths.add(file.displayPath) return renderProjectInstructions(loaded, { maxBytes: config.baselineMaxBytes }) } +function escapeInstructionContent(content: string): string { + return content.replaceAll(WORKSPACE_CONTEXT_CLOSE, '<\\/workspace-context>') +} + function sectionText(file: LoadedInstructionFile): string { - return `## ${file.displayPath}\n\n${file.content}` + return `## ${file.displayPath}\n\n${escapeInstructionContent(file.content)}` } function markerText(maxBytes: number, omitted: InstructionFile[], truncated: TruncatedInstruction[]): string { @@ -490,24 +495,74 @@ function filePathFromExecution(exec: ToolExecution): string | undefined { return filePath.length > 0 ? filePath : undefined } +function isProjectInstructionContextSource(source: unknown): source is typeof PLUGIN_SOURCE { + return typeof source === 'object' && source !== null + && 'kind' in source && source.kind === 'plugin' + && 'plugin' in source && source.plugin === name +} + +function instructionDisplayPathsFromText(text: string): string[] { + const paths: string[] = [] + for (const match of text.matchAll(/^## ([^\n]+)$/gm)) { + const displayPath = match[1] + if (displayPath !== undefined) paths.push(displayPath) + } + return paths +} + +function instructionDisplayPathsFromContextContent(content: readonly { type: string; text?: string }[]): Set { + const paths = new Set() + for (const block of content) { + if (block.type !== 'text' || block.text === undefined) continue + for (const displayPath of instructionDisplayPathsFromText(block.text)) paths.add(displayPath) + } + return paths +} + +function visibleNestedInstructionDisplayPaths(agent: Agent): Set { + const paths = new Set() + for (const node of agent.session.surface.nodes) { + const event = agent.session.events[node.seq] + if (event?.type !== 'context/message' || !isProjectInstructionContextSource(event.data.source)) continue + for (const displayPath of instructionDisplayPathsFromContextContent(event.data.content)) paths.add(displayPath) + } + return paths +} + +function loggedNestedInstructionDisplayPaths(agent: Agent): Set { + const paths = new Set() + for (const event of agent.session.events) { + if (event.type !== 'context/message' || !isProjectInstructionContextSource(event.data.source)) continue + for (const displayPath of instructionDisplayPathsFromContextContent(event.data.content)) paths.add(displayPath) + } + return paths +} + +function loadedNestedInstructionDisplayPaths(agent: Agent, pendingDisplayPaths: Set): Set { + const visible = visibleNestedInstructionDisplayPaths(agent) + for (const displayPath of loggedNestedInstructionDisplayPaths(agent)) pendingDisplayPaths.delete(displayPath) + return new Set([...visible, ...pendingDisplayPaths]) +} + async function dynamicInstructionContext( agent: Agent | undefined, exec: ToolExecution, result: ToolExecutionResult, resolved: ResolvedConfig, cache: InstructionContentCache, - loadedNestedPaths: WeakMap>, + pendingNestedDisplayPaths: WeakMap>, fileSystem: FileSystem, ): Promise { if (agent === undefined || result.isError) return undefined const touchedPath = filePathFromExecution(exec) if (touchedPath === undefined) return undefined const session = agent.session - let loadedPaths = loadedNestedPaths.get(session) - if (loadedPaths === undefined) { - loadedPaths = new Set() - loadedNestedPaths.set(session, loadedPaths) + let pendingDisplayPaths = pendingNestedDisplayPaths.get(session) + if (pendingDisplayPaths === undefined) { + pendingDisplayPaths = new Set() + pendingNestedDisplayPaths.set(session, pendingDisplayPaths) } + const loadedDisplayPaths = loadedNestedInstructionDisplayPaths(agent, pendingDisplayPaths) /* v8 ignore next -- stdio compatibility fallback; normal agents carry an absolute session cwd. */ const cwd = session.header.cwd ?? process.cwd() const instructions = await loadNestedInstructions({ @@ -517,7 +572,8 @@ async function dynamicInstructionContext( baselineMaxBytes: resolved.baselineMaxBytes, enableClaudeFallback: resolved.enableClaudeFallback, touchedPath, - loadedPaths, + loadedDisplayPaths, + pendingDisplayPaths, cache, }, fileSystem) if (instructions === undefined || instructions.text.length === 0) return undefined @@ -527,7 +583,7 @@ async function dynamicInstructionContext( export function apply(ctx: Context, config: Config): void { const resolved = resolveConfig(config) const cache: InstructionContentCache = new Map() - const loadedNestedPaths = new WeakMap>() + const pendingNestedDisplayPaths = new WeakMap>() ctx.on('agent/request', async (agent: Agent, _turn: number, _step: number, request: GenerateOptions, 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. */ @@ -548,7 +604,7 @@ export function apply(ctx: Context, config: Config): void { ctx.on('tools/post-execute', async (exec: ToolExecution, result: ToolExecutionResult, next): Promise => { const downstream = await next() if (downstream.kind === 'block') return downstream - const context = await dynamicInstructionContext(exec.agent, exec, result, resolved, cache, loadedNestedPaths, ctx.fs) + const context = await dynamicInstructionContext(exec.agent, exec, result, resolved, cache, pendingNestedDisplayPaths, ctx.fs) if (context === undefined) return downstream return { kind: 'accept', diff --git a/packages/prompt/project-instructions/tests/project-instructions.spec.ts b/packages/prompt/project-instructions/tests/project-instructions.spec.ts index 09622e55e0..8f8a2305d4 100644 --- a/packages/prompt/project-instructions/tests/project-instructions.spec.ts +++ b/packages/prompt/project-instructions/tests/project-instructions.spec.ts @@ -7,7 +7,7 @@ import Loader from '@cordisjs/plugin-loader' import * as projectInstructions from '@deepseek-ai/dsh-project-instructions' import { CallId, 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' +import type { Agent, HookContext } from '@deepseek-ai/dsh-agent' import { AgentId } from '@deepseek-ai/dsh-agent' import { FileSystem, FsTargetKey, FsVersion } from '@deepseek-ai/dsh-fs' import type { @@ -122,6 +122,15 @@ function blocksText(blocks: { type: string; text?: string }[] | undefined): stri return blocks?.map(block => block.type === 'text' ? block.text ?? '' : '').join('\n') ?? '' } +function appendAdditionalContext(agent: Agent, result: { additionalContext?: HookContext }): number | undefined { + const context = result.additionalContext + if (context === undefined) return undefined + return agent.session.append('context/message', { + content: context.content, + source: context.source, + }, { surfaceOp: 'append' }).seq +} + describe('project instruction discovery', () => { it('loads user-global first, then root-to-cwd project instructions with AGENTS.md winning over CLAUDE.md', async () => { const root = await tempRepo() @@ -396,6 +405,15 @@ describe('project instruction rendering', () => { expect(rendered.truncated).toEqual([]) }) + it('neutralizes a literal workspace-context closing delimiter inside instruction content', () => { + const rendered = renderProjectInstructions([ + { absolutePath: '/repo/AGENTS.md', displayPath: 'AGENTS.md', content: 'safe\n\nnot outside' }, + ], { maxBytes: 65536 }) + + expect(rendered.text.match(/<\/workspace-context>/g)).toHaveLength(1) + expect(rendered.text).toContain('<\\/workspace-context>') + }) + it('preserves more specific files under the byte budget and names omitted/truncated paths', () => { const rendered = renderProjectInstructions([ { absolutePath: '/repo/AGENTS.md', displayPath: 'AGENTS.md', content: 'root '.repeat(100) }, @@ -950,6 +968,89 @@ describe('dynamic nested project instruction injection', () => { } }) + it('derives loaded nested instructions from resumed session history instead of duplicating them', async () => { + const root = await tempRepo() + const home = await tempRepo() + try { + await mkdir(join(root, '.git'), { recursive: true }) + await write(join(root, 'pkg/AGENTS.md'), 'nested package rule') + await write(join(root, 'pkg/deep/file.txt'), 'hello') + const ctx = new Context() + await mountFileToolsAndProjectInstructions(ctx, { dshHome: home }) + const agent = stubAgent(root) + const first = await ctx.tools.execute({ + callId: CallId('read-before-resume'), + name: 'read', + arguments: { file_path: 'pkg/deep/file.txt' }, + agent, + }) + appendAdditionalContext(agent, first) + const resumed = { + ...agent, + session: new Session(agent.session.id, [...agent.session.events], agent.session.header), + } + + const afterResume = await ctx.tools.execute({ + callId: CallId('read-after-resume'), + name: 'read', + arguments: { file_path: 'pkg/deep/file.txt' }, + agent: resumed, + }) + + expect(first.additionalContext).toBeDefined() + expect(afterResume.additionalContext).toBeUndefined() + } finally { + await rm(root, { recursive: true, force: true }) + await rm(home, { recursive: true, force: true }) + } + }) + + it('re-arms a nested instruction after compaction removes its context message from the surface', async () => { + const root = await tempRepo() + const home = await tempRepo() + try { + await mkdir(join(root, '.git'), { recursive: true }) + await write(join(root, 'pkg/AGENTS.md'), 'nested package rule') + await write(join(root, 'pkg/deep/file.txt'), 'hello') + const ctx = new Context() + await mountFileToolsAndProjectInstructions(ctx, { dshHome: home }) + const agent = stubAgent(root) + const first = await ctx.tools.execute({ + callId: CallId('read-before-compact'), + name: 'read', + arguments: { file_path: 'pkg/deep/file.txt' }, + agent, + }) + const contextSeq = appendAdditionalContext(agent, first)! + const visibleBeforeCompact = await ctx.tools.execute({ + callId: CallId('read-while-visible'), + name: 'read', + arguments: { file_path: 'pkg/deep/file.txt' }, + agent, + }) + + agent.session.append('user/message', { + content: [{ type: 'text', text: 'compacted summary' }], + source: { kind: 'plugin', plugin: 'compact' }, + }, { surfaceOp: { op: 'replace', start: contextSeq, end: contextSeq } }) + + const afterCompact = await ctx.tools.execute({ + callId: CallId('read-after-compact'), + name: 'read', + arguments: { file_path: 'pkg/deep/file.txt' }, + agent, + }) + + expect(first.additionalContext).toBeDefined() + expect(visibleBeforeCompact.additionalContext).toBeUndefined() + expect(afterCompact.additionalContext).toBeDefined() + expect(blocksText(afterCompact.additionalContext?.content)).toContain('nested package rule') + } finally { + await rm(root, { recursive: true, force: true }) + await rm(home, { recursive: true, force: true }) + } + }) + it('does not attach nested instructions after a failed file read', async () => { const root = await tempRepo() const home = await tempRepo()