Tianyi Cui
483e0e5edf
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).
3.2 KiB
RFC: Dev-mode invariants over compile-time deep-readonly
Status: implemented (accepted 2026-06-13)
Context
The session log is append-only by contract, but the types don't enforce it: session.events returns readonly SessionEvent[] whose elements are mutable, and deriveMessages() handed the logged content arrays/blocks out by reference. The loop then passes those derived messages into the agent/request waterfall and on to adapters, where mutating the request is sanctioned — so a request middleware could reach back and rewrite history, silently breaking replay equivalence and the derived-history guarantee. Separately, the event taxonomy (turn/step nesting, seq monotonicity, tool-call/result pairing, legal status transitions) was asserted only where individual tests happened to look.
Two ways to defend the log: make immutability part of the type (DeepReadonly<SessionEvent> on the way out), or catch corruption at runtime in dev. The runtime-validation proposal took the runtime route; the deep-readonly proposal took the type route.
Decision
Reject the pervasive DeepReadonly<T> type flip. Instead:
- Always-on:
deriveMessages()deep-clones the content it emits (onestructuredCloneper 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. - Dev-mode: a new
dsh-invariantsplugin (pure listeners, off in production, on in tests and demos) asserts the event contract andObject.freezes logged event data so any other code that mutates a logged event throws instead of corrupting silently. Seeded sessions are frozen and checked onsession/created(the constructor copies the seed without emittingsession/event).
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.
Consequences
- History corruption is caught loudly in tests and demos, at zero production cost and zero type noise. The trade-off is that the guarantee is dynamic (a dev-mode tripwire) rather than static.
- The invariants plugin doubles as executable documentation of the event taxonomy — the assertions are the contract.
Session.eventskeeps itsreadonly SessionEvent[]type; no consumer churn.- This folds in the deep-readonly proposal — there is no separate deep-readonly record; this records the decision to not pursue that approach.
InvariantErroris a plainErrorwith acodefor now; a later taxonomy change can promote it.