From 8bd80e5e9bf21acc3edc149541fdcf696618beee Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Mon, 6 Jul 2026 04:13:51 +0800 Subject: [PATCH] fix review findings: one frozen seed through the waterfall; stale agent/request docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex diff review, round 1, two (A) findings: - The agent/request fallback resolved the RAW seed object — on later steps the session's cached header fold — so a delegating listener (await next(), mutate, return) could rewrite the fold in place and the change would compare as already-baseline: no delta logged, the persisted log unable to reconstruct the request (the dev invariant would fire on the divergence, but the log would still lie). One structuredClone'd, deep-frozen seed now serves both the listener chain and the fallback — in-place shaping after delegation throws — and Session.requestHeader() freezes its fold on update, so the leak class is unrepresentable from either side. Pinned by a loop-level delegating-mutator test. - Doc sweep for the old contract: agent README's event row (mutate GenerateOptions / tool filtering → frozen config seed, replacement out, logged header), compact-basic's module JSDoc (summarize routed through agent/request → direct one-shot at llm/stream), and architecture.md's event-domain line (request mutation → call-config shaping). --- docs/architecture.md | 2 +- docs/cordis-catalog/services.md | 2 +- packages/compact/compact-basic/src/index.ts | 7 +++-- packages/core/agent-loop/src/loop.ts | 12 ++++++-- .../tests/request-reconstruction.spec.ts | 29 +++++++++++++++++++ packages/core/agent/README.md | 2 +- packages/core/session/src/index.ts | 6 +++- 7 files changed, 50 insertions(+), 10 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 9eec6199c7..ff51688513 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -41,7 +41,7 @@ Events are the harness extension API. Each service owns the vocabulary for the b Use the event domain to decide where new behavior belongs: - **Session events** are durable, replayable facts. Turn and step boundaries, user input, assistant output, tool calls, tool results, steering, compaction records, and tool-owned durable facts append to the session log and flow through `session/event`. -- **Agent events** are live runtime surfaces. They carry the live `Agent` handle for status, diagnostics, prompt admission, request mutation, result validation, and continuation policy. +- **Agent events** are live runtime surfaces. They carry the live `Agent` handle for status, diagnostics, prompt admission, call-config shaping, result validation, and continuation policy. - **Capability events** belong to the seam that owns the action. `tools/*`, `llm/*`, `system-prompt/*`, `fs/*`, and `subagent/*` let policy and adapters attach without importing the loop. ### Interception Semantics diff --git a/docs/cordis-catalog/services.md b/docs/cordis-catalog/services.md index 60ee35c29c..0221946118 100644 --- a/docs/cordis-catalog/services.md +++ b/docs/cordis-catalog/services.md @@ -163,7 +163,7 @@ get(id: SessionId): Session | undefined list(): Session[] ``` -Source: [`packages/core/session/src/index.ts:367`](../../packages/core/session/src/index.ts) +Source: [`packages/core/session/src/index.ts:371`](../../packages/core/session/src/index.ts) ## `ctx.subagents` — `SubagentService` diff --git a/packages/compact/compact-basic/src/index.ts b/packages/compact/compact-basic/src/index.ts index a38bf809e3..01fcadbaea 100644 --- a/packages/compact/compact-basic/src/index.ts +++ b/packages/compact/compact-basic/src/index.ts @@ -9,9 +9,10 @@ * to the next balanced tool-pairing boundary so a compacted region never * splits a step's tool-call/result pair (an open tail step is never crossed — * compaction declines and retries once it closes). - * - **Summarization** — `ctx.llm.stream()` assembled via `BlockAssembler` - * (the single model-call surface; same path the loop uses) with a fixed - * condense-the-history system prompt routed through `agent/request`. + * - **Summarization** — a direct one-shot `ctx.llm.stream()` call assembled + * via `BlockAssembler` with a fixed condense-the-history system prompt; + * NOT a loop step, so `agent/request` never fires — interception happens + * at `llm/stream` like any other direct call. * - **Surface mutation** — a single `user/message` replace node carries the * summary; `compact/*` events are log-only lock + provenance records. * - **Auto-compaction** — an `agent/pre-step` listener delegates to diff --git a/packages/core/agent-loop/src/loop.ts b/packages/core/agent-loop/src/loop.ts index 40dd0fc380..9484f0ad5f 100644 --- a/packages/core/agent-loop/src/loop.ts +++ b/packages/core/agent-loop/src/loop.ts @@ -694,17 +694,23 @@ async function runStep( // baseline, which is what keeps fork model-overrides and resume-time // reconfiguration correct. Later steps seed from the log's folded header, // which by then is exactly what this instance last logged. - const seedConfig: LlmCallConfig = transmission.loggedHeader + // One deep-cloned, frozen seed serves BOTH the listener chain and the + // no-listener fallback: structuredClone decouples it from the session's + // cached header fold (a raw reference would let a delegating listener + // mutate the fold in place and silently skip the delta log), and the freeze + // makes in-place shaping unrepresentable — a switch is a RETURNED + // replacement, which the header event below records. + const seedConfig: LlmCallConfig = deepFreeze(structuredClone(transmission.loggedHeader // eslint-disable-next-line @typescript-eslint/no-non-null-assertion -- loggedHeader ⟹ a snapshot is in the log ? session.requestHeader()!.config - : { model: options.model ?? '' } + : { model: options.model ?? '' })) // Shape the call config: listeners return a replacement to switch model or // sampling (the seed is frozen — content shaping is not expressible here; // model-visible content flows through the log channels). The header event // below records whatever the request ACTUALLY uses, so a listener's switch // is a logged, reconstructable fact, never silent drift. - const config = await ctx.waterfall('agent/request', agent, turn, step, deepFreeze({ ...seedConfig }), () => Promise.resolve(seedConfig)) + const config = await ctx.waterfall('agent/request', agent, turn, step, seedConfig, () => Promise.resolve(seedConfig)) if (!config.model) { throw new Error(`agent "${agent.id}" has no model: set AgentOptions.model or supply one via the agent/request waterfall`) } diff --git a/packages/core/agent-loop/tests/request-reconstruction.spec.ts b/packages/core/agent-loop/tests/request-reconstruction.spec.ts index 0afaadb958..f1d6b02ac4 100644 --- a/packages/core/agent-loop/tests/request-reconstruction.spec.ts +++ b/packages/core/agent-loop/tests/request-reconstruction.spec.ts @@ -240,6 +240,35 @@ describe('request stability across the loop', () => { expectPrefixExtension(adapter.requests[0]!, adapter2.requests[0]!) }) + it('a delegating listener cannot mutate the seed through next() — the fold stays log-true', async () => { + const adapter = new MockAdapter([textResponse('one'), textResponse('two')]) + const ctx = await harness(adapter) + const agent = ctx.agentLoop.create(AgentId('a1'), { model: 'mock' }) + + ctx.on('agent/request', async (_agent, _turn, _step, _config, next) => { + const config = await next() + // next() resolves the SAME frozen seed — in-place shaping after + // delegation is unrepresentable, so a "mutate what next() returned" + // listener cannot desync the log from the request (nor reach the + // session's cached header fold, which is deep-cloned away and itself + // frozen). + expect(Object.isFrozen(config)).toBe(true) + expect(() => { (config as { temperature?: number }).temperature = 0.9 }).toThrow(TypeError) + return config + }) + + send(agent, 'first') + await waitForIdle(ctx, agent) + send(agent, 'second') + await waitForIdle(ctx, agent) + + // No delta was logged (nothing really changed), and the session's own + // fold is immutable state. + expect(agent.session.events.filter(e => e.type === 'request/header-delta')).toHaveLength(0) + expect(Object.isFrozen(agent.session.requestHeader())).toBe(true) + expect(adapter.requests[1]!.temperature).toBeUndefined() + }) + it('THEOREM: every request rebuilds byte-equal from the session log alone', async () => { const adapter = new MockAdapter([ toolCallResponse('c1', 'echo', { text: 'one' }, 'calling'), diff --git a/packages/core/agent/README.md b/packages/core/agent/README.md index a6446bf7d1..9e15356ed0 100644 --- a/packages/core/agent/README.md +++ b/packages/core/agent/README.md @@ -44,7 +44,7 @@ Turn and step boundaries are NOT mirrored as `agent/*` emits: a consumer that ne - `agent/session-start` (emit) — fired once before the first turn; a listener seeds context via `agent.inject()` (it cannot veto startup). - `agent/prompt-submit` — decide what happens to one drained queued message before it becomes a `user/message`: `PromptDecision` = `allow` (optionally rewriting the prompt `content` or attaching `additionalContext`) or `block` (drop it; a batch whose every prompt is blocked opens a zero-step turn that ends `rejected`). Maps onto Claude Code's `UserPromptSubmit`. - `agent/pre-step` (serial) — mutate the session surface before the step opens and history is derived (compaction). Fires after `turn/start` and before `step/start`, so a listener's appended events land outside the step. -- `agent/request` — mutate `GenerateOptions` before the model call (hooks, model switching, tool filtering) +- `agent/request` — shape the call config before the model call: a frozen `LlmCallConfig` seed in, a replacement out (model switching, sampling overrides). Content is not shapeable here — every request is a pure function of the session log ([reconstructability RFC](../../../docs/rfc/implemented/architecture/2026-07-05-reconstructable-requests.md)); the loop logs whatever config the request actually uses as a `request/header*` event - `agent/step-result` — post-process the assembled assistant message before tool dispatch (validates what the log records) - `agent/turn-continuation` — override the continue/stop decision via `ContinuationDecision` = `{action:'stop'}` or `{action:'continue', reason?}` (a `continue` `reason` is recorded as next-step steering in the same turn — the typed `/goal` pattern). Force-continue `/loop`, force-stop budget guard. diff --git a/packages/core/session/src/index.ts b/packages/core/session/src/index.ts index 7fa85aa796..18c7b4a8a2 100644 --- a/packages/core/session/src/index.ts +++ b/packages/core/session/src/index.ts @@ -252,7 +252,11 @@ export class Session { */ requestHeader(): EpochHeader | undefined { if (this.headerFoldSeq < this.log.length) { - this.headerFold = foldRequestHeader(this.log.slice(this.headerFoldSeq), this.headerFold) + // Frozen on update: the fold is session state exposed by reference — a + // consumer mutating it in place (instead of building a replacement) + // would desync every later comparison against the log, so mutation + // throws instead. + this.headerFold = deepFreeze(foldRequestHeader(this.log.slice(this.headerFoldSeq), this.headerFold)) this.headerFoldSeq = this.log.length } return this.headerFold