From 3712f67bc647083fe45c66230a05f0fc86b00d90 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Wed, 1 Jul 2026 15:39:08 +0800 Subject: [PATCH] =?UTF-8?q?fix(events):=20address=20review=20=E2=80=94=20c?= =?UTF-8?q?ore.md=20turn-only=20taxonomy,=20RFC=20mechanism=20names,=20pos?= =?UTF-8?q?t-execute=20content=20snapshot?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - core-data-structures/core.md: the `agent/*` taxonomy said "turn/step boundaries", but the step-boundary mirror emits were dropped — `agent/*` mirrors only turn boundaries; step boundaries are durable `step/start`/ `step/end` session events. Narrow the catalog so plugin authors aren't pointed at nonexistent `agent/*` step events. - interception-seams RFC: replace stack-position phrasing ("a later stack PR", "the stack's first change", "the PR that makes...") with durable mechanism/RFC names (the hook bridge packages, the event-domain-semantics RFC). - tools/post-execute snapshot: `dispatched.content` was the same array reference as `result.content`, so a listener's in-place `push`/`splice` leaked into the returned content while a reassignment was masked — the "protect from tampering" comment over-claimed. Copy content into a fresh array so the snapshot guards the array structure; comment now states it is not deep immutability. Regression extended to push a block in-place and assert it does not leak (proven red without the copy). --- docs/core-data-structures/core.md | 2 +- .../implemented/feature/2026-06-30-interception-seams.md | 8 ++++---- packages/core/tools/src/index.ts | 7 +++++-- packages/core/tools/tests/tools.spec.ts | 5 ++++- 4 files changed, 14 insertions(+), 8 deletions(-) diff --git a/docs/core-data-structures/core.md b/docs/core-data-structures/core.md index 2a430cebab..8141df44da 100644 --- a/docs/core-data-structures/core.md +++ b/docs/core-data-structures/core.md @@ -306,7 +306,7 @@ interface Agent { } ``` -`AgentStatus` is `'idle' | 'running' | 'disposed'`. `AgentId` is a branded string. `AgentOptions` (`model?`, `systemPrompt?`) is merge-extensible — plugins add creation options by declaration merging. The `agent/*` event taxonomy (lifecycle, turn/step boundaries, the `agent/prompt-submit`/`agent/request`/`agent/step-result`/`agent/turn-continuation` waterfalls) is in [architecture.md § Event taxonomy](../architecture.md#event-taxonomy). +`AgentStatus` is `'idle' | 'running' | 'disposed'`. `AgentId` is a branded string. `AgentOptions` (`model?`, `systemPrompt?`) is merge-extensible — plugins add creation options by declaration merging. The `agent/*` event taxonomy (lifecycle, live turn boundaries, the `agent/prompt-submit`/`agent/request`/`agent/step-result`/`agent/turn-continuation` waterfalls) is in [architecture.md § Event taxonomy](../architecture.md#event-taxonomy); step boundaries are durable `step/start`/`step/end` session events only — `agent/*` mirrors turn boundaries, not steps. ## Interception decisions diff --git a/docs/rfc/implemented/feature/2026-06-30-interception-seams.md b/docs/rfc/implemented/feature/2026-06-30-interception-seams.md index 5c00760c46..c79f237343 100644 --- a/docs/rfc/implemented/feature/2026-06-30-interception-seams.md +++ b/docs/rfc/implemented/feature/2026-06-30-interception-seams.md @@ -6,9 +6,9 @@ Status: implemented (accepted 2026-06-30) ## Context -The harness needs a hooks subsystem: users extend or gate the agent at lifecycle points the way Claude Code (CC) and Codex do. The key reframe driving this design is that **"native hooks" are not a package** — a native hook is just an ordinary Cordis plugin subscribing to the canonical lifecycle events. So the real product is a *powerful, well-typed canonical event surface*; the CC/Codex bridges (a later stack PR) are merely translators that map an external shell-hook protocol onto that same surface. Anything a bridge can do, a plain plugin can do directly — more powerfully (no serialization boundary, full `ctx`, typed returns). +The harness needs a hooks subsystem: users extend or gate the agent at lifecycle points the way Claude Code (CC) and Codex do. The key reframe driving this design is that **"native hooks" are not a package** — a native hook is just an ordinary Cordis plugin subscribing to the canonical lifecycle events. So the real product is a *powerful, well-typed canonical event surface*; the CC/Codex bridges (the `dsh-hooks-claude` / `dsh-hooks-codex` packages) are merely translators that map an external shell-hook protocol onto that same surface. Anything a bridge can do, a plain plugin can do directly — more powerfully (no serialization boundary, full `ctx`, typed returns). -Before this change the interception surface was incomplete and inconsistent for that goal: there was no per-prompt seam (CC's `UserPromptSubmit`), no session-start signal (CC's `SessionStart`), the single `tools/execute` waterfall conflated the pre-gate and post-inspect phases (CC splits `PreToolUse`/`PostToolUse`), and `agent/turn-continuation` returned a bare `boolean` with no room for a force-continue *reason*. The [event-domain-semantics RFC](../architecture/2026-06-30-event-domain-semantics.md) (the stack's first change) pinned down the three-domain rule and the typed-Decision idiom as the interception convention; this RFC builds the actual seams on top of it. +Before this change the interception surface was incomplete and inconsistent for that goal: there was no per-prompt seam (CC's `UserPromptSubmit`), no session-start signal (CC's `SessionStart`), the single `tools/execute` waterfall conflated the pre-gate and post-inspect phases (CC splits `PreToolUse`/`PostToolUse`), and `agent/turn-continuation` returned a bare `boolean` with no room for a force-continue *reason*. The [event-domain-semantics RFC](../architecture/2026-06-30-event-domain-semantics.md) pinned down the three-domain rule and the typed-Decision idiom as the interception convention; this RFC builds the actual seams on top of it. ## Decision @@ -38,8 +38,8 @@ Add/​reshape the interception seams so every one returns a small, seam-specifi ### What this PR does NOT do -It does **not** declare `hook/*` SessionEvents (the durable hook-invocation log) — those belong to the `dsh-hook-protocol` library (a later stack PR), because a native plugin can already use the typed Decisions without a durable hook log. A worked native-plugin example/test in this PR (`packages/core/agent-loop/tests/interception.spec.ts`) proves all the seams compose end-to-end through the REAL loop with NO `hook/*` involved — the concrete proof that "native hooks are just a plugin". Compaction (`PreCompact`/`PostCompact`), the Notification hook, Codex `PermissionRequest`, the permission/`ask` system, and the Stop loop-guard remain deferred (`FIXME(permissions)` marks the `ask`→deny degrade). +It does **not** declare `hook/*` SessionEvents (the durable hook-invocation log) — those belong to the `dsh-hook-protocol` library, because a native plugin can already use the typed Decisions without a durable hook log. A worked native-plugin example/test in this PR (`packages/core/agent-loop/tests/interception.spec.ts`) proves all the seams compose end-to-end through the REAL loop with NO `hook/*` involved — the concrete proof that "native hooks are just a plugin". Compaction (`PreCompact`/`PostCompact`), the Notification hook, Codex `PermissionRequest`, the permission/`ask` system, and the Stop loop-guard remain deferred (`FIXME(permissions)` marks the `ask`→deny degrade). ## Consequences -The canonical interception surface is now complete and uniformly typed: a native plugin returns typed decisions directly, and a CC/Codex bridge maps its protocol fields onto the same unions. The loop gained four firing points (session-start emit, prompt-submit waterfall, the post-tool context buffer, the continuation reshape) and the `dsh-tools` registry runs a two-waterfall pipeline; both are documented in [architecture.md](../../../architecture.md) and the package READMEs, and the decision types in [core-data-structures](../../../core-data-structures/core.md#interception-decisions) + [tools.md](../../../core-data-structures/tools.md). All existing `tools/execute` and `turn-continuation` listeners (tests, docs) migrated to the new seams. The ACP bridge maps the new `rejected` reason to `cancelled` (its codec). A pure internal change with no editor-visible transcript shift for the existing scenarios — the new behavior only fires when a hook is registered — so the snapshot goldens are unchanged; a hook-driven snapshot scenario lands with the bridges (the PR that makes a hook observable end-to-end through ACP). +The canonical interception surface is now complete and uniformly typed: a native plugin returns typed decisions directly, and a CC/Codex bridge maps its protocol fields onto the same unions. The loop gained four firing points (session-start emit, prompt-submit waterfall, the post-tool context buffer, the continuation reshape) and the `dsh-tools` registry runs a two-waterfall pipeline; both are documented in [architecture.md](../../../architecture.md) and the package READMEs, and the decision types in [core-data-structures](../../../core-data-structures/core.md#interception-decisions) + [tools.md](../../../core-data-structures/tools.md). All existing `tools/execute` and `turn-continuation` listeners (tests, docs) migrated to the new seams. The ACP bridge maps the new `rejected` reason to `cancelled` (its codec). A pure internal change with no editor-visible transcript shift for the existing scenarios — the new behavior only fires when a hook is registered — so the snapshot goldens are unchanged; a hook-driven snapshot scenario lands with the `dsh-hooks-claude` bridge, which is what makes a hook observable end-to-end through ACP. diff --git a/packages/core/tools/src/index.ts b/packages/core/tools/src/index.ts index b08403503e..73500d9d13 100644 --- a/packages/core/tools/src/index.ts +++ b/packages/core/tools/src/index.ts @@ -471,10 +471,13 @@ export class ToolRegistry extends Service { // 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`. + // call id is always the authoritative `exec.callId`. `content` is copied into + // a fresh array so a listener's in-place `push`/`splice` on `result.content` + // cannot leak into the returned content either (the elements are the same + // references — the snapshot guards the array structure, not deep immutability). const dispatched = { callId: exec.callId, - content: result.content, + content: [...result.content], isError: result.isError, ...result.error ? { error: result.error } : {}, } diff --git a/packages/core/tools/tests/tools.spec.ts b/packages/core/tools/tests/tools.spec.ts index ef67eddb95..a924ab234b 100644 --- a/packages/core/tools/tests/tools.spec.ts +++ b/packages/core/tools/tests/tools.spec.ts @@ -211,10 +211,11 @@ describe('ToolRegistry', () => { ctx.tools.register(echoTool) ctx.on('tools/post-execute', async (_exec, result, next) => { - const mutable = result as { callId: string; isError: boolean; error?: unknown } + const mutable = result as { callId: string; isError: boolean; error?: unknown; content: unknown[] } mutable.callId = 'hijacked' mutable.isError = true mutable.error = { name: 'Evil', code: 'EVIL' } + mutable.content.push({ type: 'text', text: 'INJECTED' }) // in-place array mutation return next() // delegate to the default accept — no decision-level override }) @@ -222,7 +223,9 @@ describe('ToolRegistry', () => { 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).toHaveLength(1) // the in-place push did not leak in expect(result.content[0]).toMatchObject({ text: 'hi' }) + expect(result.content.some(b => (b as { text?: string }).text === 'INJECTED')).toBe(false) }) it('composes pre + post waterfalls around dispatch (sandbox-wrap pattern)', async () => {