From 483e0e5edf5fcd965afeba0cb61d2819fb6dcad2 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Tue, 30 Jun 2026 20:25:30 +0800 Subject: [PATCH] =?UTF-8?q?fix(events):=20address=20Codex=20review=20?= =?UTF-8?q?=E2=80=94=20protect=20post-execute=20result,=20purge=20stale=20?= =?UTF-8?q?tools/execute=20refs?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex's PR-C review found two (A) blockers: - tools/post-execute could corrupt the protected outcome. postExecute passed the mutable `result` to listeners and then read result.callId / spread result on the return paths, so a listener mutating the reference (flipping isError, rewriting callId, injecting an error) escaped the decision channel. Now the authoritative callId/isError/error are SNAPSHOT before the waterfall and the return value is rebuilt from the snapshot + the typed PostToolDecision — the decision is the only sanctioned way to change the outcome, and callId is always exec.callId. Added a regression test that mutates the result reference and asserts it has no effect; proven to fail red on the unfixed code. - Public docs/JSDoc still advertised the removed `tools/execute` waterfall after the split. Swept every current-state reference to tools/pre-execute + tools/post-execute: the ToolRegistry class JSDoc (and the regenerated catalog), loop.ts's ASCII flow (also added the prompt-submit/session-start steps it was missing), the package-map READMEs (packages, core, agent-core), core-data-structures core.md/tools.md, the bash + acp + invariants src/READMEs (the deferred permission gate is the tools/pre-execute deny/ask seam now), the cookbook, and the implemented RFCs whose factual seam catalog drifted. codec.ts's totality prose now lists `rejected`. Proposed-RFC references are left as-is (frozen proposals, validated when built). --- docs/cookbook/adding-a-tool.md | 2 +- docs/cordis-catalog/events-and-services.md | 4 ++-- docs/core-data-structures/core.md | 2 +- docs/core-data-structures/tools.md | 2 +- ...06-11-dev-invariants-over-deep-readonly.md | 2 +- .../2026-06-11-microkernel-event-taxonomy.md | 2 +- .../2026-06-13-capability-seams.md | 2 +- .../2026-06-21-subagent-capability-seam.md | 2 +- packages/README.md | 2 +- packages/bash/bash-local/README.md | 2 +- packages/bash/bash-local/src/index.ts | 4 ++-- packages/bash/tool-bash/README.md | 2 +- packages/bash/tool-bash/src/index.ts | 2 +- packages/core/README.md | 2 +- packages/core/agent-core/README.md | 2 +- packages/core/agent-loop/src/loop.ts | 17 +++++++++---- packages/core/tools/src/index.ts | 22 +++++++++++++---- packages/core/tools/tests/tools.spec.ts | 24 +++++++++++++++++++ packages/support/invariants/README.md | 2 +- packages/support/invariants/src/index.ts | 4 ++-- packages/ui/acp/README.md | 2 +- packages/ui/acp/src/codec.ts | 3 ++- packages/ui/acp/src/index.ts | 2 +- 23 files changed, 78 insertions(+), 32 deletions(-) diff --git a/docs/cookbook/adding-a-tool.md b/docs/cookbook/adding-a-tool.md index 73706c84f1..53906e3a49 100644 --- a/docs/cookbook/adding-a-tool.md +++ b/docs/cookbook/adding-a-tool.md @@ -46,7 +46,7 @@ Follow tool-bash's background pattern: a `run_in_background` flag returns a task ## Permissions / sandboxing -Prefer not to build policy into the tool. The seam is the `tools/execute` waterfall (veto or wrap — see the permission-gate example in [extension-cookbook.md](./extension-cookbook.md)), or a sandboxing implementation behind the tool's executor seam. +Prefer not to build policy into the tool. The seam is the `tools/pre-execute` gate (deny/ask — see the permission-gate example in [extension-cookbook.md](./extension-cookbook.md)) and the `tools/post-execute` inspect/transform seam, or a sandboxing implementation behind the tool's executor seam. ## Tests every tool needs diff --git a/docs/cordis-catalog/events-and-services.md b/docs/cordis-catalog/events-and-services.md index d37d2d4070..a8afdbd5f1 100644 --- a/docs/cordis-catalog/events-and-services.md +++ b/docs/cordis-catalog/events-and-services.md @@ -471,7 +471,7 @@ Source: [`packages/core/system-prompt/src/index.ts:71`](../../packages/core/syst ### `ctx.tools` — `ToolRegistry` -Tool registry (`ctx.tools`): tool plugins register definitions; the agent loop executes calls through the `tools/execute` waterfall. The registry contributes its schemas into the system-prompt assembly. +Tool registry (`ctx.tools`): tool plugins register definitions; the agent loop executes calls through the `tools/pre-execute` → dispatch → `tools/post-execute` pipeline. The registry contributes its schemas into the system-prompt assembly. ```ts cordis-catalog register(definition: ToolDefinition): () => void @@ -482,7 +482,7 @@ async execute(exec: ToolExecution): Promise Types: [ToolDefinition](../core-data-structures/tools.md) · [ToolExecution](../core-data-structures/tools.md) · [ToolExecutionResult](../core-data-structures/tools.md) -Source: [`packages/core/tools/src/index.ts:341`](../../packages/core/tools/src/index.ts) +Source: [`packages/core/tools/src/index.ts:342`](../../packages/core/tools/src/index.ts) ## Inherited tier (cordis core + loader/hmr/timer) diff --git a/docs/core-data-structures/core.md b/docs/core-data-structures/core.md index 78fc36edd9..2a430cebab 100644 --- a/docs/core-data-structures/core.md +++ b/docs/core-data-structures/core.md @@ -18,7 +18,7 @@ Everything else is documented on a **sub-page**, not here. The rule that draws t | [llm-streaming.md](llm-streaming.md) | the `StreamChunk` wire protocol + adapter contract, `BlockAssembler`, the `LlmAdapter` seam | | [session.md](session.md) | the full `SessionEventMap` variant catalog, `TurnTrigger`/`TurnEndReason`, `deriveMessages()`, the turn-enclosure invariant | | [persistence.md](persistence.md) | the durability seam: `SessionPersistence`, JSONL + SQLite backends, `session/flush`, crash recovery, `SessionHeader` | -| [tools.md](tools.md) | `ToolDefinition` full fields, the schema DSL, `ToolExecution`/`ToolResult`, tool-presentation UI types, the `tools/execute` waterfall | +| [tools.md](tools.md) | `ToolDefinition` full fields, the schema DSL, `ToolExecution`/`ToolResult`, tool-presentation UI types, the `tools/pre-execute`/`tools/post-execute` pipeline | | [bash.md](bash.md) | the bash executor seam: `BashExecRequest`/`Spec`, `BashRunResult`, background `BashTask`s | | [compaction.md](compaction.md) | the compaction seam: the `compact/*` session events, `CompactionResult`, the `CompactService` interface | | [subagent.md](subagent.md) | the subagent seam: the named-provider registry, `SubagentStartRequest`/`Result`/`Run`, the start-time-vs-runtime capability split | diff --git a/docs/core-data-structures/tools.md b/docs/core-data-structures/tools.md index 4d6306de14..e5a404803b 100644 --- a/docs/core-data-structures/tools.md +++ b/docs/core-data-structures/tools.md @@ -71,7 +71,7 @@ type InferArgs = Simplify< `defineTool({ name, description, parameters, execute, … })` ties it together: `parameters` is a `SchemaSpec`, `execute(args, exec)` gets `args: InferArgs`, and the helper converts the spec to JSON Schema (`schemaSpecToJsonSchema`) for the wire and validates model-generated args (`validateArgs`) before the typed body runs. A mismatch throws `ToolArgsError` (`code: 'INVALID_ARGS'`), which the registry turns into an `isError` result so the model can self-correct. Why a custom DSL and not schemastery: tool parameters need JSON Schema (the LLM wire format), not validation/transformation — the lightweight DSL gives the best authoring DX with the smallest surface. -## Execution: the `tools/execute` waterfall shapes +## Execution: the `tools/pre-execute` / `tools/post-execute` pipeline shapes `ctx.tools.execute()` runs each call through a two-waterfall pipeline — `tools/pre-execute` (the allow/deny/ask gate) → core dispatch → `tools/post-execute` (inspect/replace the result, attach context) — the seams where sandbox, permission, hook, and plan-mode plugins gate or transform a call. The pending call is a `ToolExecution`; the outcome is a `ToolExecutionResult`. diff --git a/docs/rfc/implemented/architecture/2026-06-11-dev-invariants-over-deep-readonly.md b/docs/rfc/implemented/architecture/2026-06-11-dev-invariants-over-deep-readonly.md index b9a182a4e9..212d2e82b7 100644 --- a/docs/rfc/implemented/architecture/2026-06-11-dev-invariants-over-deep-readonly.md +++ b/docs/rfc/implemented/architecture/2026-06-11-dev-invariants-over-deep-readonly.md @@ -17,7 +17,7 @@ Reject the pervasive `DeepReadonly` type flip. Instead: 1. **Always-on:** `deriveMessages()` deep-clones the content it emits (one `structuredClone` per derived message). In-flight mutation of a request can no longer reach the log — this is the real fix, and it costs nothing meaningful next to a model call. 2. **Dev-mode:** a new `dsh-invariants` plugin (pure listeners, off in production, on in tests and demos) asserts the event contract and `Object.freeze`s logged event data so any *other* code that mutates a logged event throws instead of corrupting silently. Seeded sessions are frozen and checked on `session/created` (the constructor copies the seed without emitting `session/event`). -The invariants encode the *real* contract, not an idealized one: a `tool/call` may have no `tool/result` (a thrown `tools/execute` waterfall ends the step), and both `idle→disposed` and `running→disposed` are legal. +The invariants encode the *real* contract, not an idealized one: a `tool/call` may have no `tool/result` (a thrown tool-execution pipeline step ends the turn), and both `idle→disposed` and `running→disposed` are legal. `DeepReadonly` was rejected because it is compile-time only (a plugin casts straight through it), high type-noise across every log/message consumer and adapter, and would force readonly types through code where mutation is the sanctioned API. The clone draws the mutable/immutable boundary exactly at "logged vs in-flight" without any of that noise. diff --git a/docs/rfc/implemented/architecture/2026-06-11-microkernel-event-taxonomy.md b/docs/rfc/implemented/architecture/2026-06-11-microkernel-event-taxonomy.md index c8869c0616..c1a7aba68f 100644 --- a/docs/rfc/implemented/architecture/2026-06-11-microkernel-event-taxonomy.md +++ b/docs/rfc/implemented/architecture/2026-06-11-microkernel-event-taxonomy.md @@ -12,7 +12,7 @@ The product principle (see the 微内核Harness实现思路 design doc) is "ever Pure Cordis event taxonomy. The loop's extension seams are typed events with deliberate dispatch modes: -- **waterfall** (around-middleware) where plugins mutate or veto: `agent/request`, `agent/step-result`, `agent/turn-continuation`, `tools/execute`, `llm/stream`, `system-prompt/assemble`. +- **waterfall** (around-middleware) where plugins mutate or veto: `agent/prompt-submit`, `agent/request`, `agent/step-result`, `agent/turn-continuation`, `tools/pre-execute`, `tools/post-execute`, `llm/stream`, `system-prompt/assemble`. - **emit** (sync fire-and-forget) for notifications: turn/step boundaries, stream chunks, lifecycle, errors. - **parallel** (awaited) for the one durability checkpoint: `session/flush`. diff --git a/docs/rfc/implemented/architecture/2026-06-13-capability-seams.md b/docs/rfc/implemented/architecture/2026-06-13-capability-seams.md index e66440c3ee..46bfe88d50 100644 --- a/docs/rfc/implemented/architecture/2026-06-13-capability-seams.md +++ b/docs/rfc/implemented/architecture/2026-06-13-capability-seams.md @@ -20,7 +20,7 @@ A swappable capability is **three packages**: Implementation and consumer then evolve independently: a sandboxed executor replaces `dsh-bash-local` without touching a tool schema. -Alternatives considered: **one combined package** — rejected because it recouples the three rates of change the split exists to separate (the whole point). **`@cordisjs/plugin-capability`** — a different axis entirely: it is a permission/capability-*security* service (named permissions with inheritance, tested against a session via `ctx.capability.test`), a candidate for the deferred permissions/sandbox work on the `tools/execute` veto seam, NOT a mechanism for swapping implementations. Confusing the two ("capability") is the trap this RFC names. +Alternatives considered: **one combined package** — rejected because it recouples the three rates of change the split exists to separate (the whole point). **`@cordisjs/plugin-capability`** — a different axis entirely: it is a permission/capability-*security* service (named permissions with inheritance, tested against a session via `ctx.capability.test`), a candidate for the deferred permissions/sandbox work on the `tools/pre-execute` deny/ask seam, NOT a mechanism for swapping implementations. Confusing the two ("capability") is the trap this RFC names. The split is not mandatory when the parts are genuinely one concern: the LLM seam folds interface + consumer into `dsh-llm` (the consumer is the loop itself, not a swappable schema surface) with adapters as the implementation packages. Don't split preemptively — a capability with one conceivable implementation and one consumer stays one package until a second appears. diff --git a/docs/rfc/implemented/feature/2026-06-21-subagent-capability-seam.md b/docs/rfc/implemented/feature/2026-06-21-subagent-capability-seam.md index f733daca7b..77a838b299 100644 --- a/docs/rfc/implemented/feature/2026-06-21-subagent-capability-seam.md +++ b/docs/rfc/implemented/feature/2026-06-21-subagent-capability-seam.md @@ -66,7 +66,7 @@ The `dsh-tool-subagent` consumer awaits `run.result` and returns the child's fin ## Risks and deferrals -- **Recursion.** Without a guard, an in-process child inherits the spawn tool and can spawn unboundedly. Depth-limit is an optional capability (the in-process backends enforce it; ACP advertises it off and rejects a `maxDepth` request); tool-filtering is likewise optional. Tool-filtering, when implemented, needs a `tools/execute` veto in the child context — schema filtering alone is insufficient because a model can hallucinate a denied tool name. +- **Recursion.** Without a guard, an in-process child inherits the spawn tool and can spawn unboundedly. Depth-limit is an optional capability (the in-process backends enforce it; ACP advertises it off and rejects a `maxDepth` request); tool-filtering is likewise optional. Tool-filtering, when implemented, needs a `tools/pre-execute` deny in the child context — schema filtering alone is insufficient because a model can hallucinate a denied tool name. - **Blocking the parent turn.** Synchronous collect holds the parent's `runStep` open for the child's full duration. This is acceptable for the first cut; **background / poll / spill semantics are deferred to a future redesign that unifies long-running-tool handling across subagents AND bash** (a sub-agent and a long `bash` background task pose the same "the model started something slow, how does it collect later" problem, and should share one mechanism rather than each inventing its own). - **Live progress.** This cut surfaces only lifecycle + final result; a per-chunk child→parent update stream is deferred with the background redesign. - **ACP client surface.** Proxying `fs`/`terminal` from the ACP child back to the parent (a shared-workspace mode) is future work; the first cut advertises neither, so the child self-serves in its own process. diff --git a/packages/README.md b/packages/README.md index 3997fd5190..7f63e305dc 100644 --- a/packages/README.md +++ b/packages/README.md @@ -62,7 +62,7 @@ The rule: **extension** plugins depend on interfaces, never on the concrete loop | `llm/` | `llm` | Abstract LLM service + content-block vocabulary + chunk assembler | `ctx.llm` | | `session/` | `core` | Event-sourced session log + in-memory store | `ctx.sessions` | | `system-prompt/` | `core` | Prompt-section + tool-schema assembly registry | `ctx.systemPrompt` | -| `tools/` | `core` | Tool registry + `tools/execute` waterfall | `ctx.tools` | +| `tools/` | `core` | Tool registry + `tools/pre-execute`/`tools/post-execute` pipeline | `ctx.tools` | | `agent/` | `core` | Agent interface, registry, `agent/*` event vocabulary | `ctx.agents` | | `agent-loop/` | `core` | THE concrete loop plugin: `ReactLoopAgent` + the loop driver | `ctx.agentLoop` | | `agent-core/` | `core` | Bundle plugin: the providerless/executor-less/UI-less spine as code (forwards `agent-loop`'s `agents`) | (loads the spine) | diff --git a/packages/bash/bash-local/README.md b/packages/bash/bash-local/README.md index 2ae905b628..9625442f68 100644 --- a/packages/bash/bash-local/README.md +++ b/packages/bash/bash-local/README.md @@ -26,4 +26,4 @@ Design surveyed against the bash tools of Claude Code, OpenCode, Codex, and pi; ## Sandboxing -`TODO(permissions/sandbox)`: execution policy does NOT belong in this package. Wrap the `tools/execute` waterfall (veto/ask) or implement a sandboxing `BashExecutor` — see docs/architecture.md § plugin checklist. Reference points: Claude Code wraps commands in sandbox-exec/bubblewrap; Codex applies seatbelt/landlock plus an execpolicy prefix-rule engine. +`TODO(permissions/sandbox)`: execution policy does NOT belong in this package. Use the `tools/pre-execute` deny/ask gate or implement a sandboxing `BashExecutor` — see docs/architecture.md § plugin checklist. Reference points: Claude Code wraps commands in sandbox-exec/bubblewrap; Codex applies seatbelt/landlock plus an execpolicy prefix-rule engine. diff --git a/packages/bash/bash-local/src/index.ts b/packages/bash/bash-local/src/index.ts index 53c369a24c..8b9cf04e7d 100644 --- a/packages/bash/bash-local/src/index.ts +++ b/packages/bash/bash-local/src/index.ts @@ -4,8 +4,8 @@ * own process group (see `./run.ts` for the plumbing and the agent-tool * survey notes), tracks background tasks, and kills everything on dispose. * - * TODO(permissions/sandbox): execution policy does NOT belong here — wrap - * the `tools/execute` waterfall (see docs/architecture.md § plugin + * TODO(permissions/sandbox): execution policy does NOT belong here — use + * the `tools/pre-execute` deny/ask gate (see docs/architecture.md § plugin * checklist) or implement a sandboxing `BashExecutor`. Reference points: * Claude Code wraps commands in sandbox-exec/bubblewrap; Codex applies * seatbelt/landlock plus an execpolicy prefix-rule engine. diff --git a/packages/bash/tool-bash/README.md b/packages/bash/tool-bash/README.md index f7a15894f9..893a989e55 100644 --- a/packages/bash/tool-bash/README.md +++ b/packages/bash/tool-bash/README.md @@ -46,4 +46,4 @@ The `BashExecRequest` seam carries optional `stdin` and `env` (a **trusted-plugi ## Permissions -`TODO(permissions)`: commands run with the executor's full authority. The permission/sandbox seam is the `tools/execute` waterfall (veto or ask) plus sandboxing `BashExecutor` implementations — see docs/architecture.md. `@cordisjs/plugin-capability` (a named-permission service with a session `test()`) is a candidate building block for that work. +`TODO(permissions)`: commands run with the executor's full authority. The permission/sandbox seam is the `tools/pre-execute` waterfall (deny or ask) plus sandboxing `BashExecutor` implementations — see docs/architecture.md. `@cordisjs/plugin-capability` (a named-permission service with a session `test()`) is a candidate building block for that work. diff --git a/packages/bash/tool-bash/src/index.ts b/packages/bash/tool-bash/src/index.ts index 9ad1f9a17c..492bd28d3c 100644 --- a/packages/bash/tool-bash/src/index.ts +++ b/packages/bash/tool-bash/src/index.ts @@ -31,7 +31,7 @@ * pre-existing reload-gap drop — but the ownership fence itself is HMR-proof.) * * TODO(permissions): commands run with the executor's full authority. The - * permission/sandbox seam is the `tools/execute` waterfall (veto/ask) plus + * permission/sandbox seam is the `tools/pre-execute` waterfall (deny/ask) plus * sandboxing `BashExecutor` implementations — see docs/architecture.md * § plugin checklist. * diff --git a/packages/core/README.md b/packages/core/README.md index 8d8805471a..7030dcc0eb 100644 --- a/packages/core/README.md +++ b/packages/core/README.md @@ -6,7 +6,7 @@ The packages every harness build is assembled from: the session log, the system- |---|---|---| | `session/` | Event-sourced session log + in-memory store | `ctx.sessions` | | `system-prompt/` | Prompt-section + tool-schema assembly registry | `ctx.systemPrompt` | -| `tools/` | Tool registry + `tools/execute` waterfall | `ctx.tools` | +| `tools/` | Tool registry + `tools/pre-execute`/`tools/post-execute` pipeline | `ctx.tools` | | `agent/` | Agent interface, registry, `agent/*` event vocabulary | `ctx.agents` | | `agent-loop/` | The concrete loop plugin: `ReactLoopAgent` + the loop driver | `ctx.agentLoop` | | `agent-core/` | Bundle plugin: the providerless/executor-less/UI-less spine as code | (loads the spine) | diff --git a/packages/core/agent-core/README.md b/packages/core/agent-core/README.md index 28a3592ac6..022ccba4f4 100644 --- a/packages/core/agent-core/README.md +++ b/packages/core/agent-core/README.md @@ -13,7 +13,7 @@ This is the package to read to see **the whole plugin tree at once** — the tea @deepseek-ai/dsh-llm abstract LLM service + content-block vocabulary @deepseek-ai/dsh-session event-sourced session log + store @deepseek-ai/dsh-system-prompt prompt-section + tool-schema assembly -@deepseek-ai/dsh-tools tool registry + tools/execute waterfall +@deepseek-ai/dsh-tools tool registry + tools/pre-execute/post-execute @deepseek-ai/dsh-agent agent registry + agent/* event vocabulary @deepseek-ai/dsh-invariants dev-mode event-contract assertions @deepseek-ai/dsh-tool-bash the model-facing bash/bash_output/bash_kill schemas diff --git a/packages/core/agent-loop/src/loop.ts b/packages/core/agent-loop/src/loop.ts index 6ac98743a2..735db7c20a 100644 --- a/packages/core/agent-loop/src/loop.ts +++ b/packages/core/agent-loop/src/loop.ts @@ -142,10 +142,13 @@ export interface LoopHandle { * The agent loop. One invocation drives one agent for its whole lifetime: * * ``` + * create agent → emit agent/session-start(source) ⟵ once, before turn 1 * forever: * wait for queued messages (idle) * TURN (error-contained — a throwing plugin ends the turn, never the loop): - * drain queued → 'turn/start' → session('user/message'…) → emit agent/turn-start + * 'turn/start'; each queued msg: waterfall agent/prompt-submit + * allow → session('user/message'…) (+ inject additionalContext) | block → drop + * every prompt blocked → 'turn/end'(rejected), 0 steps; emit agent/turn-start * STEP loop: * drain steering → session('steering/message') ⟵ catches late steering * session('step/start') ⟵ durable step boundary (no agent/* mirror) @@ -157,13 +160,17 @@ export interface LoopHandle { * msg = waterfall agent/step-result ⟵ BEFORE the log append, so the * session('assistant/message' {content, usage?}) session records what actually ran * each tool-call in msg (sequential, abort-checked): - * session('tool/call'); ctx.tools.execute() ⟵ waterfall tools/execute + * session('tool/call'); ctx.tools.execute() ⟵ tools/pre-execute (allow/deny/ask) + * → dispatch → tools/post-execute * session('tool/result') + * append buffered post-execute additionalContext → session('context/message')(s) * drain steering → session('steering/message'); emit agent/steering * session('step/end') ⟵ durable step boundary (no agent/* mirror) - * cont = waterfall agent/turn-continuation(default = hadToolCalls || steered) - * if !cont && steering arrived from step/end session-event/continuation listeners: cont = true - * if !cont: break + * cont = waterfall agent/turn-continuation ⟵ ContinuationDecision; default + * {action: hadToolCalls||steered ? 'continue':'stop'}; a continue.reason is + * recorded as next-step steering + * if action==stop && steering arrived (step/end/continuation listeners): continue anyway + * if action==stop: break * session('turn/end'); emit agent/turn-end * await ctx.parallel('session/flush', session) ⟵ durability checkpoint * re-enqueue leftover steering as queued ⟵ steering is never stranded diff --git a/packages/core/tools/src/index.ts b/packages/core/tools/src/index.ts index 25f57ed400..a8f881c214 100644 --- a/packages/core/tools/src/index.ts +++ b/packages/core/tools/src/index.ts @@ -335,8 +335,9 @@ function errorInfo(error: unknown): ToolErrorInfo | undefined { /** * Tool registry (`ctx.tools`): tool plugins register definitions; the agent - * loop executes calls through the `tools/execute` waterfall. The registry - * contributes its schemas into the system-prompt assembly. + * loop executes calls through the `tools/pre-execute` → dispatch → + * `tools/post-execute` pipeline. The registry contributes its schemas into the + * system-prompt assembly. */ export class ToolRegistry extends Service { static inject = ['systemPrompt'] @@ -464,6 +465,19 @@ export class ToolRegistry extends Service { * Runs inside `execute`'s outer try/catch (a throwing listener → isError). */ private async postExecute(exec: ToolExecution, result: ToolExecutionResult): Promise { + // Snapshot the protected outcome BEFORE the waterfall. A listener receives + // the same `result` reference, so a post-waterfall read of `result.callId`/ + // `.isError`/`.error` could carry a listener's mutation — violating the + // authoritative-call-id requirement and the "preserve the dispatched + // isError/error" contract. The decision is the ONLY sanctioned channel for a + // listener to change the outcome (block, or accept-with-replacement); the + // call id is always the authoritative `exec.callId`. + const dispatched = { + callId: exec.callId, + content: result.content, + isError: result.isError, + ...result.error ? { error: result.error } : {}, + } const decision = await this.ctx.waterfall( this, 'tools/post-execute', exec, result, () => Promise.resolve({ kind: 'accept' }), @@ -471,7 +485,7 @@ export class ToolRegistry extends Service { const additionalContext = decision.additionalContext if (decision.kind === 'block') { return { - callId: result.callId, + callId: dispatched.callId, content: decision.feedback, isError: true, ...additionalContext ? { additionalContext } : {}, @@ -479,7 +493,7 @@ export class ToolRegistry extends Service { } // accept: replace content if supplied, preserve the dispatched isError/error. return { - ...result, + ...dispatched, ...decision.content ? { content: decision.content } : {}, ...additionalContext ? { additionalContext } : {}, } diff --git a/packages/core/tools/tests/tools.spec.ts b/packages/core/tools/tests/tools.spec.ts index e67650ec95..ef67eddb95 100644 --- a/packages/core/tools/tests/tools.spec.ts +++ b/packages/core/tools/tests/tools.spec.ts @@ -201,6 +201,30 @@ describe('ToolRegistry', () => { expect(result.additionalContext).toMatchObject({ content: [{ text: 'fyi' }], source: { kind: 'plugin', plugin: 'test' } }) }) + it('a post-execute listener mutating the result object cannot corrupt callId/isError/error', async () => { + // The decision is the ONLY sanctioned channel to change the outcome. A + // listener that reaches in and mutates the passed result reference (flipping + // isError, rewriting callId, attaching a bogus error) must NOT affect what + // execute() returns — the registry snapshots the authoritative fields before + // the waterfall and rebuilds from the snapshot + decision. + const ctx = await setup() + ctx.tools.register(echoTool) + + ctx.on('tools/post-execute', async (_exec, result, next) => { + const mutable = result as { callId: string; isError: boolean; error?: unknown } + mutable.callId = 'hijacked' + mutable.isError = true + mutable.error = { name: 'Evil', code: 'EVIL' } + return next() // delegate to the default accept — no decision-level override + }) + + const result = await ctx.tools.execute({ callId: CallId('c1'), name: 'echo', arguments: { text: 'hi' } }) + expect(result.callId).toBe(CallId('c1')) // authoritative exec.callId, not 'hijacked' + expect(result.isError).toBe(false) // the real (successful) dispatch outcome + expect(result.error).toBeUndefined() // no listener-injected error + expect(result.content[0]).toMatchObject({ text: 'hi' }) + }) + it('composes pre + post waterfalls around dispatch (sandbox-wrap pattern)', async () => { const ctx = await setup() ctx.tools.register(echoTool) diff --git a/packages/support/invariants/README.md b/packages/support/invariants/README.md index a08ccf01a7..b26170d48e 100644 --- a/packages/support/invariants/README.md +++ b/packages/support/invariants/README.md @@ -34,7 +34,7 @@ Session log (per session): - **turns pair and nest** — `turn/start` opens a turn, `turn/end` closes the matching one; no overlapping turns. - **steps nest in turns** — `step/start` opens a step in the open turn; `step/end` closes the matching step. - **chunks belong to an open step** — `step/start` precedes its `assistant/chunk`s. -- **a `tool/result` needs a prior `tool/call`** — but NOT the converse: a `tool/call` may have no result (a thrown `tools/execute` waterfall ends the step with no `tool/result`, which is legal). +- **a `tool/result` needs a prior `tool/call`** — but NOT the converse: a `tool/call` may have no result (a thrown tool-execution pipeline step ends the turn with no `tool/result`, which is legal). Agent status (per agent): diff --git a/packages/support/invariants/src/index.ts b/packages/support/invariants/src/index.ts index b2372e28db..5e3fd26b82 100644 --- a/packages/support/invariants/src/index.ts +++ b/packages/support/invariants/src/index.ts @@ -259,8 +259,8 @@ function checkEvent(trace: SessionTrace, event: SessionEvent): void { case 'tool/result': { requireOpenStep(trace, 'tool/result', event.data.turn, event.data.step) // A result needs a prior matching call in the same step. (The converse - // does NOT hold: a call may have no result — a throwing tools/execute - // waterfall ends the step with no tool/result, which is legal.) + // does NOT hold: a call may have no result — a throwing tool-execution + // pipeline step ends the turn with no tool/result, which is legal.) const syntheticInterrupted = event.data.isError && event.data.error?.code === 'interrupted' if (!trace.pendingCalls.delete(event.data.callId) && !syntheticInterrupted) { throw new InvariantError(`tool/result for ${event.data.callId} with no prior tool/call in this step`) diff --git a/packages/ui/acp/README.md b/packages/ui/acp/README.md index ad50383542..524a873467 100644 --- a/packages/ui/acp/README.md +++ b/packages/ui/acp/README.md @@ -65,7 +65,7 @@ Teardown reaches quiescence: for EVERY live session settle any pending prompt as ## Known limitations (tracked TODOs) -- **`TODO(rfc010-permission-gate)`** — the `tools/execute` permission gate (`session/request_permission`) is NOT implemented; tools run with the executor's full authority. The `agent→sessionId` reverse map is in place so the gate can route a permission request (which receives only `exec.agent`) back to its originating session. [ACP support](../../../docs/rfc/proposed/feature/2026-06-14-acp-agent-client-protocol.md) and [ACP multi-session](../../../docs/rfc/proposed/feature/2026-06-14-acp-multi-session.md) stay `proposed` until the gate (and per-session permission ownership) land. +- **`TODO(rfc010-permission-gate)`** — the `tools/pre-execute` permission gate (`session/request_permission`) is NOT implemented; tools run with the executor's full authority. The `agent→sessionId` reverse map is in place so the gate can route a permission request (which receives only `exec.agent`) back to its originating session. [ACP support](../../../docs/rfc/proposed/feature/2026-06-14-acp-agent-client-protocol.md) and [ACP multi-session](../../../docs/rfc/proposed/feature/2026-06-14-acp-multi-session.md) stay `proposed` until the gate (and per-session permission ownership) land. - **`additionalDirectories`** — rejected. A session operates in its single `cwd` (see Per-session cwd); widening the tool/filesystem scope to extra roots is a separate sandbox concern, not yet implemented. ## stdout is the protocol diff --git a/packages/ui/acp/src/codec.ts b/packages/ui/acp/src/codec.ts index 57622fbbfe..3b03a81c83 100644 --- a/packages/ui/acp/src/codec.ts +++ b/packages/ui/acp/src/codec.ts @@ -17,7 +17,8 @@ import type { ContentBlock as AcpContentBlock, StopReason } from '@agentclientpr * Map a harness {@link TurnEndReason} to the ACP `StopReason` wire enum. * * The mapping is total over the kinds the loop actually produces today - * (`completed`/`aborted`/`error`/`disposed`/`max-tokens`). `TurnEndReason` is + * (`completed`/`aborted`/`error`/`disposed`/`max-tokens`/`rejected`). + * `TurnEndReason` is * merge-extensible, so an unknown future kind falls through to `end_turn` — * the safest default (the turn DID end; we just lack a more specific wire * reason) — rather than throwing into the SDK, which would reject an unknown diff --git a/packages/ui/acp/src/index.ts b/packages/ui/acp/src/index.ts index 754b1750be..b3ca2d3bf1 100644 --- a/packages/ui/acp/src/index.ts +++ b/packages/ui/acp/src/index.ts @@ -22,7 +22,7 @@ * `agent→sessionId` reverse map for O(1) demux of `agent/*` events; every * `session/event` and `agent/*` event is routed strictly to its owning session * record, so two sessions streaming at once never interleave their - * `session/update` notifications. The `tools/execute` permission gate is + * `session/update` notifications. The `tools/pre-execute` permission gate is * deferred — see the TODO(rfc010-permission-gate) note below. * * stdout is the protocol: this plugin must run in an example that loads NO