From 3025fbaeb3312fd182806193a3c25a110181f270 Mon Sep 17 00:00:00 2001 From: kingwl Date: Fri, 10 Jul 2026 18:35:44 +0800 Subject: [PATCH] fix(mode): prepend the assemble filter; structured_output joins the plan allowlist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review finding with a real in-repo instance: the structured runtime's per-spawn final-assembly wrapper (prepend, post-next) re-injects structured_output OUTSIDE the mode filter, so a structured child in plan mode would see a tool the gate then denies — the soft policy and the hard gate telling different stories. The suggested fix (make the mode filter outermost) cannot beat that instance: prepend unshifts, so the per-spawn listener always registers later and wraps outer. Two-part resolution instead. Semantically, structured_output enters the shipped plan allowlist — it is a child's pure result channel, the same ask/report class as ask_user_question and exit_plan_mode, so the filter, the re-injection, and the gate now agree wherever a structured child runs in plan mode. Mechanically, the filter registers with prepend anyway: it now wraps outside every append-registered listener regardless of load order (regression test pins a pre-registered post-next mutator being filtered), narrowing the documented cosmetic residual to prepend-after-load listeners only, where the gate still covers execution. Severity note: no execution breach existed — the gate held throughout; this closes the prompt-honesty gap. --- docs/cordis-catalog/services.md | 2 +- .../feature/2026-07-07-plan-mode.md | 8 +++---- packages/mode/mode/README.md | 4 ++-- packages/mode/mode/src/index.ts | 15 ++++++++++-- packages/mode/mode/tests/mode.spec.ts | 24 ++++++++++++++++++- 5 files changed, 43 insertions(+), 10 deletions(-) diff --git a/docs/cordis-catalog/services.md b/docs/cordis-catalog/services.md index 8b2b430d01..18d19edc81 100644 --- a/docs/cordis-catalog/services.md +++ b/docs/cordis-catalog/services.md @@ -159,7 +159,7 @@ set(agent: Agent, mode: string): void Types: [Agent](../core-data-structures/core.md) -Source: [`packages/mode/mode/src/index.ts:205`](../../packages/mode/mode/src/index.ts) +Source: [`packages/mode/mode/src/index.ts:210`](../../packages/mode/mode/src/index.ts) ## `ctx.sessionPersistence` — `SessionPersistence` (abstract seam) diff --git a/docs/rfc/implemented/feature/2026-07-07-plan-mode.md b/docs/rfc/implemented/feature/2026-07-07-plan-mode.md index 948058ff76..c906366969 100644 --- a/docs/rfc/implemented/feature/2026-07-07-plan-mode.md +++ b/docs/rfc/implemented/feature/2026-07-07-plan-mode.md @@ -43,10 +43,10 @@ Mode definitions are validated plugin Config — per repo convention, changeable section: | You are in plan mode: explore and design, then present the plan for approval through exit_plan_mode. - tools: [read, todo_write, web_search, web_fetch, ask_user_question, exit_plan_mode] + tools: [read, todo_write, web_search, web_fetch, ask_user_question, structured_output, exit_plan_mode] ``` -`plan`'s shipped default allowlist is the read-only surface (`read`, `todo_write`, `web_search`/`web_fetch`, `ask_user_question`, `exit_plan_mode`) with `bash` and `subagent` excluded until the sandbox family can actually confine them — a deployment that accepts the risk widens its own config today. `default` is reserved (the absence of policy) and rejected as a key; an unknown mode name fails validation loudly at `set()` time. +`plan`'s shipped default allowlist is the read-only surface (`read`, `todo_write`, `web_search`/`web_fetch`, `ask_user_question`, `structured_output`, `exit_plan_mode` — the last three are the pure ask/report class) with `bash` and `subagent` excluded until the sandbox family can actually confine them — a deployment that accepts the risk widens its own config today. `default` is reserved (the absence of policy) and rejected as a key; an unknown mode name fails validation loudly at `set()` time. ### In the terminal @@ -94,7 +94,7 @@ A contained `session/event` listener ([defensive patterns](../../../defensive-pa A `system-prompt/assemble` waterfall listener reads the calling agent's mode (the `AssembleContext` carries `agent`) and, in a non-default mode, filters `assembly.tools` down to the mode's allowlist and appends the mode's guidance section. The loop already renders per step and logs the result: entering or leaving a mode surfaces on the next step as a `request/header-delta` — or as the full `request/header` fallback snapshot when the change is inexpressible in the delta encoding (adding `exit_plan_mode` resorts the canonical tool list, and a pure reordering has no delta form) — so every mode transition is an attributable log fact. The section is static per mode and the plan itself stays in the conversation (messages and tool args, already in context), so a mode does not add per-step prompt churn — re-injecting plan state into every request ([Prior art](#prior-art)'s compaction-survival hack) is unnecessary and would only burn prefix cache. -The guidance section is an ordinary registered section, `{ name: 'mode:policy', order: 50, text: context => … }` — order 50 sits after the persona (0) and before tool guidance (100–199); it resolves to the folded mode's configured text and to `''` (dropped at render) for the default mode or an agent-less assembly. The tool filter wraps: it awaits `next()` and filters the RETURNED assembly's `tools`, so additions made anywhere inside its wrap are covered. The filter enforces one rule in every mode: `exit_plan_mode` is visible IFF the agent's folded mode is `plan` — which is also what keeps a default-mode assembly byte-identical to a no-`dsh-mode` deployment even though the tool is always registered. In a non-default mode it additionally intersects with the mode's allowlist. +The guidance section is an ordinary registered section, `{ name: 'mode:policy', order: 50, text: context => … }` — order 50 sits after the persona (0) and before tool guidance (100–199); it resolves to the folded mode's configured text and to `''` (dropped at render) for the default mode or an agent-less assembly. The tool filter wraps with `prepend: true`: it awaits `next()` and filters the RETURNED assembly's `tools`, so additions made anywhere inside its wrap — including every append-registered listener's post-`next()` mutation, regardless of load order — are covered. The filter enforces one rule in every mode: `exit_plan_mode` is visible IFF the agent's folded mode is `plan` — which is also what keeps a default-mode assembly byte-identical to a no-`dsh-mode` deployment even though the tool is always registered. In a non-default mode it additionally intersects with the mode's allowlist. ### The hard layer: the gate @@ -193,4 +193,4 @@ What holds now, pinned by the unit, protocol, and snapshot tiers: - `exit_plan_mode`'s approve path flips the mode and restores the full toolset on the next step; the keep-planning path returns the corrective `isError` carrying the user's feedback and stays in plan mode; the ACP `session/set_mode` round-trip updates `current_mode_update`, and the exit review prompts through each surface's user-interaction provider. - The docs tail shipped with the landing: READMEs, regenerated catalogs (persistence log, config, cordis services, tools), the packages map and architecture rows, and the cookbook row. -The accepted costs: a pending user flip set while idle is lost if the process dies before the next turn (the UI re-applies; the idle-record primitive is the escape hatch if this bites in practice). Every mode transition is a logged header change and therefore a prefix-cache reset at the provider — inherent, visible in per-step usage, and an argument against mode-flapping UIs, not against the design. Sibling-listener order is not deterministic, so a foreign assemble listener wrapping OUTSIDE the mode listener could re-widen filtered schemas — the filter runs on the assembly `next()` returns, and the hard gate keeps anything re-widened non-executable; the residual cost is cosmetic (the model sees a tool it cannot use), accepted rather than mechanized. Plan mode's shipped allowlist excludes `bash` and `subagent`, which costs real exploration power until the sandbox family and mode inheritance land — a deployment that accepts the risk can widen its own config today. Two in-flight stacks touch the ACP mode surface (this one and the sandbox branch's config options, whose feature-matrix stance records session modes as deliberately unmodeled): the picker-to-modes / knobs-to-config-options division pinned in the [FAQ](#faq) is the contract, and the sandbox branch owes its matrix rows an amendment on merge-down. The ACP spec's draft v2 direction reportedly slates session modes for removal in favor of config options; if that lands, the picker migrates to a config-option select mechanically — the mode state and both enforcement layers are wire-agnostic — accepted. +The accepted costs: a pending user flip set while idle is lost if the process dies before the next turn (the UI re-applies; the idle-record primitive is the escape hatch if this bites in practice). Every mode transition is a logged header change and therefore a prefix-cache reset at the provider — inherent, visible in per-step usage, and an argument against mode-flapping UIs, not against the design. The mode filter prepends, so only a listener that ALSO prepends after `dsh-mode` loads can wrap outside it and re-widen filtered schemas — the one shipped instance is the structured runtime's per-spawn final-assembly wrapper, whose `structured_output` is on the plan allowlist precisely so the filter, that wrapper, and the gate agree; for any future such listener the hard gate keeps a re-widened tool non-executable, and the residual cost is cosmetic (the model sees a tool it cannot use), accepted rather than mechanized. Plan mode's shipped allowlist excludes `bash` and `subagent`, which costs real exploration power until the sandbox family and mode inheritance land — a deployment that accepts the risk can widen its own config today. Two in-flight stacks touch the ACP mode surface (this one and the sandbox branch's config options, whose feature-matrix stance records session modes as deliberately unmodeled): the picker-to-modes / knobs-to-config-options division pinned in the [FAQ](#faq) is the contract, and the sandbox branch owes its matrix rows an amendment on merge-down. The ACP spec's draft v2 direction reportedly slates session modes for removal in favor of config options; if that lands, the picker migrates to a config-option select mechanically — the mode state and both enforcement layers are wire-agnostic — accepted. diff --git a/packages/mode/mode/README.md b/packages/mode/mode/README.md index d9d0108d88..ec0c263583 100644 --- a/packages/mode/mode/README.md +++ b/packages/mode/mode/README.md @@ -34,9 +34,9 @@ The model-facing exit tool. Its single required argument is the plan text — a plan: section: | You are in plan mode: ... - tools: [read, todo_write, web_search, web_fetch, ask_user_question, exit_plan_mode] + tools: [read, todo_write, web_search, web_fetch, ask_user_question, structured_output, exit_plan_mode] ``` -Definitions are validated at load (`resolveConfig`): the built-in `plan` (read-only allowlist plus `ask_user_question`, `bash`/`subagent` excluded) merges unless overridden, `default` is rejected as a key, and allowlists may name not-yet-registered tools (registration is dynamic). An unknown name fails loudly at `set()` time. +Definitions are validated at load (`resolveConfig`): the built-in `plan` (read-only allowlist plus the ask/report channels `ask_user_question`/`structured_output`, `bash`/`subagent` excluded) merges unless overridden, `default` is rejected as a key, and allowlists may name not-yet-registered tools (registration is dynamic). An unknown name fails loudly at `set()` time. RFC: [plan mode](../../../docs/rfc/implemented/feature/2026-07-07-plan-mode.md). diff --git a/packages/mode/mode/src/index.ts b/packages/mode/mode/src/index.ts index 8f83e23811..f4671ed9a1 100644 --- a/packages/mode/mode/src/index.ts +++ b/packages/mode/mode/src/index.ts @@ -115,7 +115,12 @@ const PLAN_SECTION + 'its review fails, ask the user to switch the session out of plan mode instead ' + 'of retrying denied tools.' -const PLAN_TOOLS = ['read', 'todo_write', 'web_search', 'web_fetch', 'ask_user_question', EXIT_PLAN_MODE] +// 'structured_output' is a structured subagent child's result channel (pure +// reporting, the ask/exit class of read-only-safe): its runtime re-injects the +// schema into the FINAL assembly from an outermost per-spawn listener, so +// allowlisting is what keeps the soft filter, that re-injection, and the hard +// gate telling one consistent story when such a child runs in plan mode. +const PLAN_TOOLS = ['read', 'todo_write', 'web_search', 'web_fetch', 'ask_user_question', 'structured_output', EXIT_PLAN_MODE] /** The review question's approve option label — the answer item is matched by it. */ const APPROVE_LABEL = 'Approve' @@ -246,6 +251,12 @@ export class ModesService extends Service { text: context => (context.agent === undefined ? '' : this.activeDefinition(context.agent.session)?.definition.section ?? ''), }) + // prepend: the filter wraps OUTSIDE every append-registered listener + // regardless of load order, so their post-next() additions are filtered + // too. A listener that ALSO prepends after this plugin loads (the + // structured runtime's per-spawn wrapper) still wins the wrap — for that + // one the allowlist carries the contract, and the hard gate covers + // execution either way. ctx.on('system-prompt/assemble', async (_assembly, context, next) => { const result = await next() const agent = context.agent @@ -259,7 +270,7 @@ export class ModesService extends Service { result.tools = result.tools.filter(tool => allowed.has(tool.name) && (tool.name !== EXIT_PLAN_MODE || active.name === PLAN_MODE)) return result - }) + }, { prepend: true }) ctx.on('tools/pre-execute', (exec, next): Promise => { if (exec.agent === undefined) return next() diff --git a/packages/mode/mode/tests/mode.spec.ts b/packages/mode/mode/tests/mode.spec.ts index 7ca37354b5..b9429de265 100644 --- a/packages/mode/mode/tests/mode.spec.ts +++ b/packages/mode/mode/tests/mode.spec.ts @@ -75,7 +75,7 @@ describe('resolveConfig', () => { it('merges the built-in plan definition with the read-only allowlist', () => { const resolved = resolveConfig({}) const plan = resolved.definitions.get(PLAN_MODE) - expect(plan?.tools).toEqual(['read', 'todo_write', 'web_search', 'web_fetch', 'ask_user_question', EXIT_PLAN_MODE]) + expect(plan?.tools).toEqual(['read', 'todo_write', 'web_search', 'web_fetch', 'ask_user_question', 'structured_output', EXIT_PLAN_MODE]) expect(plan?.section).toContain('plan mode') }) @@ -327,6 +327,28 @@ describe('the soft layer', () => { expect(assembly.sections.find(section => section.name === 'mode:policy')?.text).toBe('reviewing') }) + it('filters additions from an append-registered final-assembly mutator, regardless of load order', async () => { + // A foreign listener that post-processes await next() and was registered + // BEFORE dsh-mode loaded: under append ordering it would wrap OUTSIDE the + // filter and its re-added tool would leak into the plan-mode header. The + // filter registers with prepend, so it wraps outside every + // append-registered listener and filters their additions too. + const ctx = new Context() + await ctx.plugin(SystemPrompt) + await ctx.plugin(ToolRegistry) + ctx.on('system-prompt/assemble', async (_assembly, _context, next) => { + const final = await next() + final.tools = [...final.tools, { name: 'smuggled', description: 'added after next()', parameters: {} }] + return final + }) + await ctx.plugin(ModesService) + registerNamedTools(ctx, ['read']) + const agent = agentWithSession() + agent.session.append('mode/set', { mode: PLAN_MODE }) + const assembly = await ctx.systemPrompt.assemble({ agent }) + expect(assembly.tools.map(tool => tool.name)).toEqual(['exit_plan_mode', 'read']) + }) + it('treats a dropped folded definition as the default mode', async () => { const ctx = await setup() registerNamedTools(ctx, ['read', 'write'])