Skip to content

feat(guard): add persistent tool result guard with Chronicle audit - #159

Open
antra-tess wants to merge 1 commit into
mainfrom
feat/tool-result-guard
Open

antra-tess wants to merge 1 commit into
mainfrom
feat/tool-result-guard

Conversation

@antra-tess

@antra-tess antra-tess commented Sep 18, 2026

Copy link
Copy Markdown
Collaborator

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

  • Add durable agent_settings.tool_result_guard and the recipe default toolResultGuard, both off by default. Disabling affects future output; previously withheld results remain withheld.
  • On the first structured refusal after a tool round, withhold that result batch and retry inference once on the same model. Preserve tool-call/result IDs and error flags, discard partial output, and never repeat tool execution. A second refusal does not automatically rewind older context.
  • Use the neutral notice: Tool result withheld by the guard. The tool has already executed. Refusal details stay in operational logs.
  • Keep pending results out of speculative compression until the following inference succeeds. Preserve full originals, including images and pre-truncation output, in the append-only Chronicle audit state framework/tool-result-guard; large payloads use Chronicle blobs. Tests verify historical records and originals after reopening the store.
  • Support streaming and direct Agent inference, ephemeral agents, context-budget restarts, and recovery while the host is quiescing. Include documentation and a changelog fragment.

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 current main base.
  • Guard tests: 13 passed / 0 failed, including real Membrane with a deterministic provider adapter, enabling mid-stream, parallel text/image/error results, retry bounds, persistence, oversized-original recovery, budget restart, ephemeral settlement, and quiesced continuation.
  • Focused guard/settings/quiesce/operator-surgery regressions after rebasing: 68 passed / 0 failed.
  • npm test after rebuilding on the current base: 912 passed / 3 failed / 4 skipped (919 total).
  • The three full-run failures were in the existing mcpl-awareness-barrier.test.ts startup matrix: host/command with heartbeat-first, and push/event / channels/incoming with 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 run main baseline.

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 fragment added: changelog.d/tool-result-guard.added.md.

🤖 Generated with Codex.

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>
@antra-tess
antra-tess marked this pull request as ready for review September 18, 2026 08:02

@Anarchid Anarchid left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟠 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 providersrc/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.ts and src/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() returned null while hasPending remained true, as shown above.
  • Full npx tsc --noEmit could not be made representative locally: this repository has no lockfile, npm ci therefore 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.0 range. 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 Anarchid left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. A turn ended by end_turn/skip_reply leaves its batch pending indefinitely. storeResults runs at src/framework.ts:6347, before the shouldEndTurn branch (:6528), which resets the agent and settles the turn without accept() 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:63 describes this only as an interrupted submission; here nothing was interrupted.
    • The next turn opens with hasPending === true, so refusalRetries is forced to 0 (src/agent.ts:886) and autoRewind is 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 — refusalHandling is 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 scope hasPending effects to batches actually submitted in the current turn.
  2. Restoring originals after compilation bypasses the context budget. prepareRequest (src/tool-result-guard.ts:103, called from src/agent.ts:791 and :848) swaps the placeholders for the full wire results after the strategy has selected against the ~20-token notice (CM passthrough.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:238 uses a tiny payload and maxStreamTokens: 1 and never checks the rebuilt request. Fix: reserve the pending batch's real wire cost during selection; assert the final request after substitution.

  3. Acceptance cannot repair compression that already read the placeholder. Staging is an ordinary cm.addMessage (tool-result-guard.ts:80) → onNewMessage fires at once (CM context-manager.ts:1159); acceptance is only editMessage (:114), and edits do not touch derived entries or notify the strategy (CM context-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.

  4. Streamed output: discarded text still leaks, accepted text is dropped. (found independently by both reviewers) src/framework.ts:8633 suppresses proseStream.feed while hasPending || recovering — but hasPending is true on every post-tool round with the guard on, and the suppressed chunks are never replayed, so sendOutgoingComplete (:9782-9786) finalizes with pre-tool text only. Meanwhile the inference:tokens trace directly above (:8622) is unconditional, and connectome-host's tts-relay-module.ts:380 voices exactly that event. Net: TTS speaks the refused partial; outgoing-stream consumers never get the successful answer. The test at :333 only 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.

  5. The audit is not crash-durable before the originals go to the provider. staged/linked are appended with no store.sync() and submission follows at src/framework.ts:6585. Chronicle flushes state.bin chain heads only on sync()/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 through framework.stop() (which syncs, :1855). The durability model is inherited; the unconditional promise in docs/tool-result-guard.md:63 is new. Fix: a sync barrier before submission with explicit failure handling, plus an ungraceful-restart test — or soften the claim.

  6. Direct API: a compile failure wedges the agent. src/agent.ts:596 stages before await compileWithInjections (:600), both outside the try at :636. After a transient compile error the agent stays ready with a pending batch, and every retry throws Tool 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

  1. 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 for agent_settings and programmatic AgentConfig. Ship the host companion or qualify the docs.
  2. Guard recovery loses same-turn explicit-send suppression. The restart re-enters driveStream with hadToolCalls=false (:8483) and turnSilenced=false (:8517); a text-only recovery after a successful send publishes a postscript the policy would have silenced (:9281). Budget restarts share this inherited limitation; the PR adds a new entry into it.
  3. Usage of the abandoned stream is dropped from session totals. The guarded branch logs tokenUsage and returns at :8888, before usageTracker.onInferenceCompleted (:9051). Membrane's usage there is cumulative for the whole physical tool loop, so successful earlier rounds vanish too. doInference likewise overwrites the first response (src/agent.ts:1076).
  4. Audit growth is unbounded and triple-copied. Every guarded batch — accepted ones included — archives originals, content and wireResults forever (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.
  5. 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.refusalRetries per physical round on the native path (membrane.ts:3761), so the getter works; XML mode ignores retries anyway (:3020).
  • withhold clears pending before its audit append — a failed log write cannot re-arm rejected output.
  • Default-off: storeResults adds original content, prepareRequest is a no-op; refusal behaviour unchanged.
  • framework/state read-modify-write matches siblings (persistToolResultInlineCap, persistAgentRuntimeSettings); override restored on all four agent-creation paths.
  • prepareRequest is idempotent across its two call sites; kv-unified layoutHash is 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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants