feat(guard): add persistent tool result guard with Chronicle audit - #159
antra-tess wants to merge 1 commit into
Conversation
Preserve full original output in Chronicle while keeping pending results out of speculative compression. On a provider refusal, withhold the latest batch and retry inference once without repeating tool execution. Co-Authored-By: Codex (GPT-6) <noreply@openai.com>
Anarchid
left a comment
There was a problem hiding this comment.
🟠 NEEDS ATTENTION
Reviewer: Codex (GPT-5.6 Sol)
Reviewed head: 0169a2049696a986ee0f1f1c119ece1c6c900032
Finding
[P1] Do not claim a refusal for a batch that never reached the provider — src/framework.ts:8844
const guardRefusal = response.stopReason === 'refusal'
&& (agent.toolResultGuard.hasPending || agent.toolResultGuard.recovering);prepareRequest() explicitly supports a strategy folding the entire staged exchange away: it records pending.submitted = false and sends no original tool output. In that state, withhold() returns null and deliberately leaves pending intact. The completion path above nevertheless keeps guardRefusal === true, clears the refusal content, and later skips normal autoRewind handling because of !guardRefusal at line 9146. A deterministic refusal caused by older context can therefore repeat indefinitely with the never-submitted batch still pending; the guard keeps suppressing the existing recovery path even though the guarded bytes were never in any provider request.
A direct state reproduction on this head produced:
{"before":true,"outcome":null,"after":true,"auditTypes":["staged","linked"]}Please distinguish a submitted pending batch from a merely staged one. For example, expose hasSubmittedPending (or return a tagged outcome from withhold) and enter the guard-retry path only for a submitted batch or an active guard recovery. When a staged batch was omitted by compilation, record it as withheld, clear it, and let the refusal continue through the ordinary rewind/reaction path. Add a regression with a strategy that omits the staged exchange and autoRewind: true.
Tooling results
git diff --check origin/main...HEAD— passed.- Internal-name scan of the changed files — clean.
- Targeted TypeScript checks for
src/tool-result-guard.tsandsrc/agent.ts— passed. node --import tsx test/tool-result-guard.test.ts— 13 passed, 0 failed.node --import tsx test/framework.test.ts— 22 passed, 0 failed.- Direct unsubmitted-batch reproduction —
withhold()returnednullwhilehasPendingremainedtrue, as shown above. - Full
npx tsc --noEmitcould not be made representative locally: this repository has no lockfile,npm citherefore refuses to run, normal-sandbox package resolution has no network, and the available installed context-manager is older than this head's declared^0.10.0range. Its failures were missing history-query exports in that stale dependency, not in this diff. - GitHub checks — 5/5 successful on the reviewed head across Ubuntu/macOS and Node 20/24, including changelog validation.
Verdict
The guard's normal live, budget-restart, persistence, image, settings, and one-retry paths have strong focused coverage. The omitted-exchange state is already modeled in prepareRequest, but its refusal branch can strand the guard and disable the framework's existing recovery. Fix that state distinction before merge; no other material finding surfaced in the changed code or its immediate dependencies.
— Reviewed by GPT-5.6 Sol via OpenAI Codex.
Anarchid
left a comment
There was a problem hiding this comment.
Reviewers: Claude (Fable 5.1) + Codex (gpt-6-astra), every finding re-verified. Reviewed head: 0169a2049696a986ee0f1f1c119ece1c6c900032.
Complements the earlier Sol review on this head: its P1 (a staged-but-unsubmitted batch suppressing autoRewind) is the same family as #1 below and one fix — scoping guard effects to a submitted batch — covers both.
Verdict: CHANGES REQUESTED. The core mechanism is sound — ID/error-flag pairing survives withholding, tools are never re-run, the one-retry latch cannot re-arm, and the default-off path is byte-identical to before. But the guard keeps a second copy of truth (placeholder in the context manager, original on the wire, reconciled by a later edit), and every finding below is a place where some other subsystem — turn termination, budgeting, compression, streaming, durability, accounting — only sees one of the two copies. Most are reachable in ordinary operation with the guard on, not only on a refusal.
Line numbers are at the PR head. Dependency evidence is at the versions the PR pins: context-manager v0.10.0, membrane v0.5.85, chronicle v0.4.0, connectome-host anima/main@558095e.
Major
-
A turn ended by
end_turn/skip_replyleaves its batch pending indefinitely.storeResultsruns atsrc/framework.ts:6347, before theshouldEndTurnbranch (:6528), which resets the agent and settles the turn withoutaccept()or any settlement of the guard. For agents that close every turn with a tool (explicit prose routing), this is every turn. Consequences:- A restart or redeploy while idle turns the last batch into a permanent "withheld by the guard" notice although no refusal occurred.
docs/tool-result-guard.md:63describes this only as an interrupted submission; here nothing was interrupted. - The next turn opens with
hasPending === true, sorefusalRetriesis forced to 0 (src/agent.ts:886) andautoRewindis skipped (src/framework.ts:9146). A refusal caused by the new human message withholds the innocent previous batch, retries once, and then stops with no rewind —refusalHandlingis effectively disabled on turn-opening rounds. - Until then, history tools, operator views and compression see the placeholder.
Fix: settle the batch on the end-turn path (accept it — nothing was refused — or never stage a batch that will not be submitted), and scopehasPendingeffects to batches actually submitted in the current turn.
- A restart or redeploy while idle turns the last batch into a permanent "withheld by the guard" notice although no refusal occurred.
-
Restoring originals after compilation bypasses the context budget.
prepareRequest(src/tool-result-guard.ts:103, called fromsrc/agent.ts:791and:848) swaps the placeholders for the full wire results after the strategy has selected against the ~20-token notice (CMpassthrough.ts:38,77). A context-budget or physical-window restart — whose whole purpose is to shrink the request — recompiles small and then re-inflates; it can exceed the same window again. Astra reproduced ~30 budgeted vs ~10,014 submitted tokens.test/tool-result-guard.test.ts:238uses a tiny payload andmaxStreamTokens: 1and never checks the rebuilt request. Fix: reserve the pending batch's real wire cost during selection; assert the final request after substitution. -
Acceptance cannot repair compression that already read the placeholder. Staging is an ordinary
cm.addMessage(tool-result-guard.ts:80) →onNewMessagefires at once (CMcontext-manager.ts:1159); acceptance is onlyeditMessage(:114), and edits do not touchderivedentries or notify the strategy (CMcontext-manager.ts:1166-1190). The framework flushes deferred messages right behind the staged batch, which can push it out of the protected tail; Autobiographical then summarizes the notice and dedupes by message ID (autobiographical.ts:5108), so the accepted output is absent from compressed memory for good. The PR's claim is "speculative compression cannot incorporate output that is subsequently withheld" — true, but the converse failure is the common case. No test runs real compression. Fix: an explicit compression exclusion for pending exchanges until settled, or revision-aware invalidation. -
Streamed output: discarded text still leaks, accepted text is dropped. (found independently by both reviewers)
src/framework.ts:8633suppressesproseStream.feedwhilehasPending || recovering— buthasPendingis true on every post-tool round with the guard on, and the suppressed chunks are never replayed, sosendOutgoingComplete(:9782-9786) finalizes with pre-tool text only. Meanwhile theinference:tokenstrace directly above (:8622) is unconditional, and connectome-host'stts-relay-module.ts:380voices exactly that event. Net: TTS speaks the refused partial; outgoing-stream consumers never get the successful answer. The test at:333only asserts refusal text is absent from an array that may stay empty. Fix: buffer at the shared publication boundary; release on a clean round, discard on refusal; add a positive-delivery assertion. -
The audit is not crash-durable before the originals go to the provider.
staged/linkedare appended with nostore.sync()and submission follows atsrc/framework.ts:6585. Chronicle flushesstate.binchain heads only onsync()/Drop(chronicle src/state/manager.rs:268,src/store.rs:1727). A kill between staging and the periodic sync can reopen on the previous heads. Every reopen test goes throughframework.stop()(which syncs,:1855). The durability model is inherited; the unconditional promise indocs/tool-result-guard.md:63is new. Fix: a sync barrier before submission with explicit failure handling, plus an ungraceful-restart test — or soften the claim. -
Direct API: a compile failure wedges the agent.
src/agent.ts:596stages beforeawait compileWithInjections(:600), both outside thetryat:636. After a transient compile error the agent staysreadywith a pending batch, and every retry throwsTool result guard already has a pending batch(tool-result-guard.ts:71). Astra reproduced it. Fix: make staging idempotent for the current ready batch and include compile in the recovery boundary.
Minor
- Recipe option is not wired in the main host. (both reviewers) Changelog and docs advertise recipe
toolResultGuard; connectome-host forwards agent keys explicitly (src/framework-agent-config.ts:107-119) and has no such field, and unknown top-level agent keys are not rejected — so the recipe line is a silent no-op. "No companion PR required" holds only foragent_settingsand programmaticAgentConfig. Ship the host companion or qualify the docs. - Guard recovery loses same-turn explicit-send suppression. The restart re-enters
driveStreamwithhadToolCalls=false(:8483) andturnSilenced=false(:8517); a text-only recovery after a successfulsendpublishes a postscript the policy would have silenced (:9281). Budget restarts share this inherited limitation; the PR adds a new entry into it. - Usage of the abandoned stream is dropped from session totals. The guarded branch logs
tokenUsageand returns at:8888, beforeusageTracker.onInferenceCompleted(:9051). Membrane's usage there is cumulative for the whole physical tool loop, so successful earlier rounds vanish too.doInferencelikewise overwrites the first response (src/agent.ts:1076). - Audit growth is unbounded and triple-copied. Every guarded batch — accepted ones included — archives
originals,contentandwireResultsforever (tool-result-guard.ts:75); no retention, no reader or restore tool. Chronicle's size-aware snapshots keep this from going quadratic, but full reads materialize the whole log. - Stats drift on edit. CM's
tokenStatsCache(message-store.ts:1378) is documented as never write-through on edit; a stats read while pending prices the result as the notice permanently. Inherited; newly exercised on every tool round.
Verified fine
- Membrane reads
options.refusalRetriesper physical round on the native path (membrane.ts:3761), so the getter works; XML mode ignores retries anyway (:3020). withholdclearspendingbefore its audit append — a failed log write cannot re-arm rejected output.- Default-off:
storeResultsadds original content,prepareRequestis a no-op; refusal behaviour unchanged. framework/stateread-modify-write matches siblings (persistToolResultInlineCap,persistAgentRuntimeSettings); override restored on all four agent-creation paths.prepareRequestis idempotent across its two call sites; kv-unifiedlayoutHashis taken after substitution, so it is self-consistent.- Refusal normalization covers Anthropic, Bedrock, OpenAI-family
content_filter, Gemini SAFETY/RECITATION, Responses API. - Chronicle v0.4.0 API use is correct; blobs are content-addressed and individually synced.
- Changelog fragment follows policy; CI green on all four matrix legs; mergeable (main is one test-only commit ahead).
Method
Astra (gpt-6-astra, xhigh, read-only, codex 0.154): ~11 min, 90 commands, 3.08M input tokens (2.88M cached), 18.7k output; two in-memory probes. Claude: forest pass, network fetches, re-verification of every Astra citation (all eight held).
Both: #4 (suppressed-prose half), #7. Astra only: #2, #3 (Claude had the edit-notification gap but not the deferred-flush path), #4 (trace-leak half), #5, #6, #8, #9. Claude only: #1, #11.
Not reachable: live provider behaviour; full test suite (no node_modules in the worktree).
Problem
A provider refusal after tool output can stop an agent or trigger a broad rewind that removes complete exchanges from context. Agents need an opt-in way to continue without the latest tool output while preserving the original records.
Changes
agent_settings.tool_result_guardand the recipe defaulttoolResultGuard, both off by default. Disabling affects future output; previously withheld results remain withheld.Tool result withheld by the guard. The tool has already executed.Refusal details stay in operational logs.framework/tool-result-guard; large payloads use Chronicle blobs. Tests verify historical records and originals after reopening the store.Interrupted pending results stay withheld after restart, with their originals retained in the audit. No companion PR or dependency change is required.
Tests
npm run build: passed on the currentmainbase.npm testafter rebuilding on the current base: 912 passed / 3 failed / 4 skipped (919 total).mcpl-awareness-barrier.test.tsstartup matrix:host/commandwith heartbeat-first, andpush/event/channels/incomingwith discord-first. Each was accompanied by the fixture's 1,000 ms Discord-marker deadline expiring. Immediately rerunning the unchanged file in isolation (node --test --test-force-exit dist/test/mcpl-awareness-barrier.test.js) gave 25 passed / 0 failed. This suggests timing sensitivity under full-suite load; no claim is made that the failure count matches a separately runmainbaseline.Not verified
Live provider APIs and external Discord/MCPL delivery were not exercised. Provider and outgoing-delivery behavior were tested with deterministic adapters and in-memory channel stubs.
changelog.d/tool-result-guard.added.md.🤖 Generated with Codex.