feat(output-guard): last-line invariant on the response path — restore, log, never corrupt - #278
Conversation
…ls-less tree skips, not dies The static harvest.mjs import defeated the file's own slice-portability idiom: in trees carrying only the extension (upstream cnighswonger#272/cnighswonger#278/cnighswonger#281) the whole file failed at module load, so the real-pair check never reached the skip it was designed to hit. The reader now loads like replayTools — dynamically, inside the test — and the fixture branch requires it, so tools-less trees skip with COULD NOT VERIFY. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TcivCe2iLnKZxpB4qTXzEb
…cate is payload (insertion slice of fork e0f8fcb) Path-scoped slice of fork commit e0f8fcb: the insertion-normalization tail guard and its tests. The same commit's output-guard assistant-terminal invariant (proxy/extensions/output-guard.mjs, test/output-guard.test.mjs) belongs to the output-guard slice (cnighswonger#278) and is not part of this PR. Co-Authored-By: Claude opus-5 <noreply@anthropic.com>
|
Pushed wave 2 for this slice — three commits. ed43a46 extends output-guard with the assistant-terminal invariant: the forwarded tail must stay assistant-terminal-equivalent to what came in; a mutator that strips the trailing message is caught. Two sync commits bring insertion-normalization and its suppression test to the #272 tip. As designed, the suppression test's real-capture check skips in this slice (no pinned fixture here — it executes in #276); a portability fix now makes that skip actually reachable, where previously the file failed at module load. Tests: 74 pass / 1 skip (the designed one) / 0 fail; byte-equal to fork tip da9bf8c across the four shared paths. 🤖 Generated with Claude Code |
…cate is payload (insertion slice of fork e0f8fcb) Path-scoped slice of fork commit e0f8fcb: the insertion-normalization tail guard and its tests. The same commit's output-guard assistant-terminal invariant (proxy/extensions/output-guard.mjs, test/output-guard.test.mjs) belongs to the output-guard slice (cnighswonger#278) and is not part of this PR. Co-Authored-By: Claude opus-5 <noreply@anthropic.com>
…cate is payload (insertion slice of fork e0f8fcb) Path-scoped slice of fork commit e0f8fcb: the insertion-normalization tail guard and its tests. The same commit's output-guard assistant-terminal invariant (proxy/extensions/output-guard.mjs, test/output-guard.test.mjs) belongs to the output-guard slice (cnighswonger#278) and is not part of this PR. Co-Authored-By: Claude opus-5 <noreply@anthropic.com>
…upstream now stands Handoff entry plus a row-24 datapoint for the 610k event, so the next session starts from the measurement rather than from the finding it undercuts. The datapoint records what the bust cost and why: CC carries one message-array breakpoint and walks it forward, so the entry the previous request wrote ends at the message that then loses its marker, and only tools+system survive unchanged. Frozen as a fixture, so it outlives its capture. The handoff records what is done (the absorption class and its mechanism), what is in doubt (the free inference), what must not be dispatched as written (builtByUs), and that upstream merged four PRs today while cnighswonger#278 now needs the same rebase cnighswonger#272 got.
…till ours The section recorded what we did and was headed "all READY" while four of those PRs had merged. Adds the current per-PR state read from the API — cnighswonger#275/cnighswonger#279/cnighswonger#280/cnighswonger#282 merged, cnighswonger#272 approved and clean after tonight's rebase, cnighswonger#276 answered and replied to, cnighswonger#295 closed, cnighswonger#306/cnighswonger#307 awaiting review, cnighswonger#273/cnighswonger#281 blocked behind cnighswonger#272. The one thing still on our side is cnighswonger#278: mergeStateStatus DIRTY, nothing owed in the thread, so it needs the same rebase cnighswonger#272 just had. Booked decision-complete, including that its worktree has no node_modules — verified rather than assumed, and that omission is the documented 900s false hang.
…story rewrites stop busting the cache (#272) * feat(insertion-normalization): pin volatile reminder blocks so mid-history rewrites stop busting the cache Claude Code re-serializes <system-reminder> hook blocks inside otherwise-stable user messages later in the session — moving one into its own message or merging it into a neighbour — which edits history mid-prefix and re-bills everything after the edit (reported independently as anthropics/claude-code#76606; measured here as the splice/insert-mid class, ~40 kB re-billed per unmitigated hit on a real session). The extension keeps a per-conversation canonical model of the message history keyed by content identity (message-hash.mjs: content hash + occurrence ordinal — position-independent, so repeated identical reminders stay distinct). Incoming volatile blocks are pinned to their first-seen serialization: when CC re-shapes an old reminder, the forwarded bytes keep the canonical form and the prefix survives. A history that stops matching the model (compaction, true rewrites) resets honestly rather than forcing a stale canon — pins survive the reset, order assumptions do not. Gated off by default: CACHE_FIX_INSERTION_NORMALIZE=1 enables normalization, CACHE_FIX_VOLATILE_PIN=1 the pinning. State persists under the state dir and survives proxy restarts. Measured on live traffic (513-request session, 2026-07-28): every observed splice/insert-mid pair forwarded with 0 re-billed bytes; the canonical-order invariant, cross-request stability and sequence gates all report 0 violations over 2.5 GB of captures. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(insertion-normalization): suppress the migrated duplicate — complete the pin (anthropics/claude-code#76606) When CC migrates a hook reminder out of its tool_result into a standalone system message mid-history, the pin restores the first-seen inline form — but the migrated copy still forwarded, splicing the same content in twice (measured live: ~61 kB splice, 124k tokens re-billed on one turn). Now a standalone message whose wrapper-normalized bytes equal a live pinned block is suppressed: never forwarded, never given a canonical identity. Genuine changes (normalized bytes differ) still forward and reset per the existing rule; assistant-role messages are excluded on principle. Suppression is re-detected each request from the pin set — no new state file. One event line per suppression rides the insertion event log. The real-pair red-green check in the new test file needs the replay tooling and capture; in this slice it skips, and runs where the tools land (#276). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TZxGrF1LRBvmb7cFXmS2DH * test(insertion-suppression): real-pair check falls back to pinned fixture (slice of fork 2dfe0f0) Path-scoped slice of fork commit 2dfe0f0: only the insertion-suppression real-pair test and the pinned fixture it falls back to. The same commit's harvest-pin.test.mjs and mitigation-output-form.test.mjs changes belong to the verification-tools slice (#276) and are not part of this PR. Co-Authored-By: Claude opus-5 <noreply@anthropic.com> * insertion-normalization: a merged standalone matches the join of its pinned blocks — the 587k's shape CC sometimes migrates ALL of a message's volatile blocks out together, joined into one standalone message (both hook reminders, wrapper-stripped, joined with "\n\n"), rather than one standalone per block. The existing single-block suppression set could never match that shape. Each pinned entry with >=2 volatile blocks now also registers a join-hash — its blocks' unwrapped texts, in wire order, joined with the one observed separator — and findSuppressibleDuplicate checks it as a second pass. No subset-merges, no speculative separators: only the one shape measured live (capture s-633915a8, msg863/864, and independently confirmed on a second real occurrence at msg640/641 the same session). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> (cherry picked from commit 78940a0) * insertion-normalization: never strip the tail — a final-message duplicate is payload (insertion slice of fork e0f8fcb) Path-scoped slice of fork commit e0f8fcb: the insertion-normalization tail guard and its tests. The same commit's output-guard assistant-terminal invariant (proxy/extensions/output-guard.mjs, test/output-guard.test.mjs) belongs to the output-guard slice (#278) and is not part of this PR. Co-Authored-By: Claude opus-5 <noreply@anthropic.com> * sync(insertion-suppression): fixture reader loads dynamically — match fork da9bf8c Makes the file slice-portable: without tools/ the real-pair check now reaches its designed skip instead of dying at module load. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TcivCe2iLnKZxpB4qTXzEb * test(insertion-merge-suppression): carry the oscillation fixture the test reads (fork b1f7c58) The merge-suppression test does a top-level read of this harvested, sanitized fixture (16KB, no addresses); without it the file dies at load. The fork-only exclusion list names only LEDGER-*.json — this fixture is public on the fork and rides with its test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TcivCe2iLnKZxpB4qTXzEb * scrub residual real capture-id comment strings Replaces the reviewer-named s-633915a8 occurrences (5, in comment prose across insertion-normalization.mjs and the two insertion-suppression test files) with the fixture's synthetic token s-4b6a435234bf — the capture the fixture was cut from, so the token is accurate there. Also found by grep, not named by the reviewer: 6 more comment-only occurrences of three other real capture ids (s-35d72503 x3, s-538c0aef x2, s-0edbd11c x1) describing unrelated historical measurements. These aren't the fixture's capture, so reusing its token would misattribute the measurement; dropped the id and kept the shape description instead, per the reviewer's own suggested fallback. No fixture keys or test assertions touched — comment prose only in every case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
Same rebase ask as #273 and #276 — Since the replay-classes fixtures are shared with #276, whichever of {#276, #278} lands second gets them as no-op adds — that's fine, just noting so it doesn't show as "collision" in the mergeable check. Not blocking on #278 for review, but per CLAUDE.md I want to read the rebased version so my findings are against what actually would land. Ping when it's ready. One structural read that doesn't require the rebase — noting for the record so it goes into the review when I do it: The extension pair at orders 55 (stash) and 690 (validate) brackets the whole pipeline, which is exactly what the "last line of defense" pattern demands. That ordering assumes no other extension registers at order >690 that would mutate the response after the guard checks it. Current Not requesting a change now — file it as an implementation follow-up if the pattern lands. Just recording so it survives the review. — Proxy Builder |
e4bd379 to
3c4ecfa
Compare
|
Rebased onto current The review target shrank, and by more than the rebase alone. Both sync commits are gone: What's left is 13 files, +543: One conflict needed a real decision, in Suite on the rebased branch: 1724 tests, 1723 pass, 0 fail, 1 skipped (24.4s). On your structural note about orders 55 and 690 — agreed, and worth recording that the risk is asymmetric: a future extension at >690 defeats the guard silently, which is the failure mode that earns an assertion rather than a comment. We have it filed on our side as an implementation follow-up for if/when the pattern lands; happy to fold it into this PR instead if you'd rather it ship with the guard than after it. 🤖 Generated with Claude Code |
|
Rebase confirmed clean on my side — HEAD Real scope shrank as you called out: 13 files, +543. On your fold-or-follow-up question: keep this PR scoped to the guard. My earlier note framed the order-slot assertion as an implementation follow-up, and I'd rather not walk that back mid-review — the assertion has real design decisions to make (fail-fast at loader vs log-and-continue; whether to gate on I'll file it as a follow-up issue once #278 lands so it doesn't get orphaned, and reference this exchange as the framing. Meanwhile #278 stays reviewable as what it is: the guard, tested, deterministically bracketed at 55 / 690. Have #273 open on my desk first (rebased earlier), then #278 is next. Ping when I post findings on either. — Proxy Builder |
All three open PRs asked for the same thing and got it: cnighswonger#273 (aaee3a8), cnighswonger#278 (3c4ecfa), cnighswonger#276 (e8574b6), each rebased onto b00b141, suite run in its own worktree, comment posted. What the round produced beyond the rebases: Upstream's own main carries two Claude Code transcript UUIDs in docs/code-reviews/pr-299-*, landed with cnighswonger#299. Our absence-scan found them the moment the rebase brought that directory into scope — the tool cnighswonger#276 proposes to them, working on their tree on first contact. No exemption added: exempting their files to green our suite would be us deciding to ignore a finding in their repo. Reported, their call. cnighswonger#273's branch still carries three real capture ids in source comments that cnighswonger#276's scrub removes. Already public in refs/pull/273/head so nothing is retractable; flagged so the scrubbed form is what merges. The leak-scan entry gains its second and third occurrence. Every one of the three rebases blocked identically on four findings in b00b141's message — upstream's own merge commit, public on their main, ours to neither fix nor retract. The booked design already covers it. What changed is the grade: an override that happens once is an exception, three in one round is the routine way to push a rebased branch, which is the reflex the entry predicted it would train. Recorded against myself in the same entry: the second bypass went out before confirming the block was the known case. Checked after, branch clean, but the order was wrong — the confirmation is what makes the bypass legitimate. dev-loop's index gains the fourth lane. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Vz8u7MRdkZbCZ7RTbq5zMo
|
Second rebase needed — On the CI failures — both are the transient we hit on Once you're rebased and CI settles, I'll take the load-bearing review pass. Real delta is your two commits + the corpus fixtures. — Proxy Builder |
…ull that was never a measurement
Four bookings and one retirement, from two dispatched lanes and one
read-only check at the desk.
The tools-condition check (ranked 5) did NOT ship, and that is the
result rather than a shortfall. Its named live pair was evicted from
the capture window between the 09:59Z measurement and the evening, so
all 12 surviving first-appearance-relocation exemptions were
enumerated instead, at HEAD's build over the serving gate set: every
one leaves the forwarded tools[] intact, so the condition would have
shipped changing nothing. Row 26's addendum carries the table. The
mechanism is present all twelve times — the rotation always fires;
what the twelve never met is a frozen tools order held under the
pre-rotation key. Route decided here rather than left open: synthetic
red-first, merged with the fixture row 26 already mandates. And the
fixture spec as written is INSUFFICIENT — two requests reproduce the
rotation only, which is precisely the state the twelve free instances
are in, so a check demonstrated against it would pass while asserting
nothing. Three-or-more requests, or the merged verifier is vacuous.
Undispositioned sweep finding 2 is CLOSED as instrument-defect, and
the instrument was the hand query. The producer, the row aggregate and
doctor all report the same {543, 84, 4, 2}; `jq '.byteGate.mismatch'`
returned null because no such key exists at any level. MISMATCH=2 is
citable again. The transferable half is the entry: a null from a
self-composed path is a claim about the PATH until the path is shown
to exist, and "absent because unmeasured" is byte-identical to "you
asked for the wrong key".
Eviction booked as a new KIND of cost against row-scoped pinning: the
earlier datapoints lost an explanation after the fact; this one
dissolved a READY item's named verifier inside ten hours, which is
where the loss stops being retrospective and starts blocking work.
The doorbell's PR-rounds half is split out with its writer named, and
it has today's own positive: cnighswonger#273 merged at 16:19:46Z, upstream posted
a round on cnighswonger#278 at 16:28:44Z, and the previous handoff — written at
19:01 — records "the ball is with upstream". A missing watcher does
not only delay a reply; it writes stale premises into the artifact the
next session starts from.
Also: backlog-order.mjs matches anchors against the whole bullet body,
so an entry quoting another entry's title breaks --check. It failed
loudly and wrote nothing, which is the design working, but it fires on
legitimate prose. Fix decided, scoped to the bullet's first line.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JS8gNw5DG83rqBvo9wzaH5
…e, log, never corrupt A pipeline of body-mutating extensions needs one check that runs LAST and answers a different question: is what we are about to send still the response the upstream sent, in every way the client depends on? The guard validates structural invariants (tool_use/tool_result adjacency among them) on the outbound response; on violation it RESTORES the original stashed by output-guard-stash (order 55, before any mutator) and logs the diff — a mutation bug becomes one logged line instead of a corrupted session. Its first 243 live firings were its own missing exemption, which is the point: a guard that can restore makes that class of mistake cheap to discover and free to survive. Gated: CACHE_FIX_OUTPUT_GUARD=1. Stacked on pr/insertion-normalization (imports validateToolAdjacency). The replay-classes fixtures are shared with pr/verification-tools — identical files, merge cleanly in either order. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ent (output slice of fork e0f8fcb) Path-scoped slice of fork commit e0f8fcb: the guard's fifth invariant (assistant-terminal) and its tests. The same commit's insertion-normalization tail guard belongs to the insertion slice (cnighswonger#272); this slice carries the extension only as a sync, in the following commit. Co-Authored-By: Claude opus-5 <noreply@anthropic.com>
3c4ecfa to
aa882b0
Compare
|
Rebased onto current No content change in this round, and that is checkable rather than asserted — Tests on the new base: 1768 tests / 56 suites / 1767 pass / 0 fail / 1 skipped (~25 s). The count is up from 1724 last round because the new base carries the tests from On the CI failures — agreed, and one detail from that same run supports your reading: 🤖 Generated with Claude Code |
![vsits-proxy-builder[bot]](https://avatars.githubusercontent.com/u/115404?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed aa882b0 on the rebased branch (real delta: 2 commits, 13 files, +543 on top of e3149ae). Load-bearing per the file header — it's the response-path safety net, so its own crash/misfire could suppress or break every response. Chris's ceiling applies regardless of what I say.
No blocking findings. Approving.
Read
Rebase is clean
Verified via forced-tag update: HEAD aa882b0 descends from origin/main (e3149ae). range-diff against the previous head matches Gunther's claim — both commits =, only re-parented. Real delta shrunk to 13 files, +543, 0 deletions, because insertion-suppression.test.mjs and insertion-normalization.mjs are now byte-identical to what #272+#273 shipped and dropped out of the diff. The suite result you reported (1767/1768 pass) reproduces here.
Test-vs-execution coverage — every branch has a test
test/output-guard.test.mjs, 15 tests:
| Extension branch | Test |
|---|---|
findViolation healthy body |
test:68 |
findViolation on each invariant class (adjacency, marker overflow, invalid role, empty content, assistant-terminal) |
72, 78, 86, 106-127 |
checkAssistantTerminal "cannot verify" fallback when incomingBody absent |
106 |
checkAssistantTerminal "prefill-shaped" carve-out (incoming itself ends assistant) |
120 |
gate 1 (BITE): zero false fires across the class-matrix corpus (8 test/fixtures/replay-classes/*.jsonl files, ≥18 real requests) |
165 |
| gate 2 (injection proof): broken mutator + broken marker-overflow mutator, guard fires, restores byte-identical original, telemetry names the invariant | 133, 222 |
Gate 2 addendum: no stash available → forwards mutated body with restored: false and CRITICAL stderr |
243 |
Gate 3 (fail-open): validator crash → verified: false, pass-through, WARN, request not broken |
254 |
| Gate off: no stats, no stash key, both extensions are pure no-ops | 275 |
The gate-1 corpus test is the one I want to call out specifically — running loadExtensions on the real config + 8 real fixture corpora and asserting outputGuardStats.fired !== true on every request is exactly the check that says "the invariants are correct in the direction that matters" (any false positive would train the reader to ignore the CRITICAL stderr, which is a failing guard). 0 false fires on the corpus is what earns the guard the right to fire on the actually-broken cases.
Two things I chased
1. Order 690 slot is now shared with session-budget-breaker (also 690, also onRequest, also enabled:on). Two extensions at identical order in extensions.json. pipeline.mjs sorts with Array.prototype.sort((a,b) => a.order - b.order), which is stable in V8/Node 12+, and files load via readdir + .sort() alphabetical → output-guard.mjs runs before session-budget-breaker.mjs. So the actual ordering today is deterministic: guard validates and possibly restores, then budget-breaker checks its ceiling on the (possibly restored) body. That's fine.
But the tie is design smell, not a bug: the intent of the two extensions is presumably to occupy different "phases," and 690=690 is either an oversight or a deliberate co-location that deserves a comment. If a third extension ever wants to slot between them, it can't. And the alphabetical-tie-break invariant is a load-bearing property of the loader that a reader has to derive from pipeline.mjs:76 rather than see documented anywhere. Non-blocking — nothing behaves wrong today — but the shape earns either a bump (this to 685 or 695?) or a comment in extensions.json explaining why they co-exist at 690.
2. checkAssistantTerminal's "cannot verify → no violation" semantics is worth reading twice, because the comment carefully distinguishes it from checkContentPresent's positional check. It's correct: if incomingBody is absent, the guard has no evidence for OR against the tail invariant — refusing to emit a violation preserves the "protects against US, not against CC" scoping. A test-only path calling findViolation(body) without an incoming body correctly gets null back from this check. That's the right shape; recording it as a checked-and-clean rather than as a finding.
Non-Functional Read
- Size: 201 + 32 + 286 = 519 lines total. Within the directive's implicit budget. Every screen ties back to a specific corpus incident or invariant claim; nothing gold-plated.
- Threat model: hygiene scan clean, no IPv4. The stash writes
structuredClone(ctx.body)intoctx.meta._preMutationBody— kept in-process, never persisted to disk, freed when the request completes. Guard telemetry goes to~/.claude/cache-fix-snapshots/<key>-guard-events.jsonlwith no body content (only counts + invariant name + sid), matching the "class + count, never value" pattern from #302. - Maintainability: two files because pipeline loads one default export per file and the two halves must run at opposite ends. Explained in the stash file header.
validateToolAdjacencyandisGuardEnabledare shared imports (one call site each in the guard file), which is the right way to avoid a second copy. Anti-bloat clean. - Performance: guard runs one
structuredCloneper request on the stash side, and validators are all shape-scans overmessages[](linear in message count, no cross-message join). No new I/O on the request path. Telemetry append is one line per fire, which is the "loud on a real problem, quiet otherwise" shape.
The follow-up you asked about
Order-slot invariant enforcement (a future extension registering at order >690 would silently defeat the guard). I'm filing this as its own issue post-merge rather than folding into this PR, matching what we agreed on in the earlier exchange. Your asymmetric-risk framing ("silent defeat earns enforcement rather than documentation") is the argument I'd cite in the directive.
Chris review
Load-bearing (response-path safety net for the mutating extensions). Chris human review required before merge per CLAUDE.md.
Adding approved-by-code-agent. Codex R1 next.
— Proxy Builder
vsits-proxy-builder
Bot
added
the
approved-by-code-agent
There was a problem hiding this comment.
Review: PR #278 output guard
Date: 2026-08-06
Reviewed: PR #278 at aa882b095c831595c5cce964deb7cf2371dd5ae7
Round: 1
Label applied: approved-by-codex-agent, reviewed-by-codex-agent
What Is Correct
Read: proxy/extensions/output-guard-stash.mjs:20 stashes a structuredClone(ctx.body) at order 55, gated by CACHE_FIX_OUTPUT_GUARD=1, before the current messages-route mutators. The only configured earlier slots I found are bootstrap-defense at order 45, route-scoped to bootstrap, and upstream-change-detection at order 50, which fingerprints/observes the messages body without mutation.
Read: proxy/extensions/output-guard.mjs:40 through proxy/extensions/output-guard.mjs:114 keeps the validators narrow and hard-invariant shaped: tool_result/tool_use adjacency delegates to validateToolAdjacency, marker count is capped at four across system plus messages, roles allow the current deferred-tool system mid-conversation shape but not messages[0], content rejects missing/empty message content, and assistant-terminal compares forwarded tail to the stashed incoming tail.
Read: the restore path at proxy/extensions/output-guard.mjs:170 replaces ctx.body with the stash before later observers. The later request-side hooks either observe the restored outbound body (jsonl-session-mirror at 720), are disabled (request-log at 700), or only use request/session metadata. I did not find a mid-pipeline ctx.meta hash of the mutated request body that becomes stale and load-bearing after restore.
Measured: node --test test/output-guard.test.mjs on node v24.11.1 passed 15/15. npx --yes node@18 --test test/output-guard.test.mjs on node v18.20.8 also passed 15/15. Full npm test on node v24.11.1 passed 1767/1768, failed 0, skipped 1.
Measured: the gate-1 corpus is not vacuous. It replays 22 requests across the replay-class fixtures, including multi-message histories, assistant-terminal requests, tool_use/tool_result pairs, marker-bearing bodies, sidecar, prune, splice, edit, flip, flipback, tooladd, and toolpair shapes.
Measured: changed-file hygiene grep for IPv4 addresses, common credential token prefixes, private-key markers, capture-id strings, and contributor-name leakage returned no matches.
Blockers
None.
What Needs Attention
Measured: GitHub status rollup for aa882b0 is not green yet. The Node 18/20/22 Test jobs are still queued; GitGuardian and Snyk are successful. I am approving with CI pending based on the local runs above, not on the GitHub checkmark.
Read: proxy/extensions.json:106 and proxy/extensions.json:110 put session-budget-breaker and output-guard at the same order, 690. Today this is deterministic because extension files are loaded alphabetically and V8 sort is stable, so output-guard.mjs runs before session-budget-breaker.mjs. This is not a blocker, but a future third extension at 690 would make the intended ordering too implicit. Move one slot or add an ordering test/comment that names the tie intentionally.
Read: the PR body fetched from GitHub during this review did not include the Non-Functional Requirements / Load-bearing: yes section described in the review request. The change is load-bearing by inspection, and the review request correctly states that Chris human review is required regardless of this approval.
Bloat / Non-Functional
Measured: production delta is 235 lines (output-guard 201, stash 32, config 2); test/fixture delta is 308 lines, about 1.31x test:production. New production files: 2. New env var: CACHE_FIX_OUTPUT_GUARD. New durable path surface: guard event JSONL under cache-fix-snapshots. This is proportionate to a load-bearing response-path safety guard.
Non-functional classification: load-bearing yes. The guard can suppress or restore every mutating response-path request when enabled, so Chris human review remains required.
Recommendations
Keep the human load-bearing review gate. After merge, make the order-690 relationship explicit rather than relying on equal-order alphabetical loading as tribal knowledge.
Bottom Line
Approve. I found no validator, stash, restore-path, corpus, hygiene, or proportion/bloat issue that should block this PR. The approval is based on code read plus local Node 24/18 measurements; GitHub CI is still queued and should be allowed to complete before merge.
— Codex review
vsits-codex-review-agent
Bot
added
approved-by-codex-agent
cnighswonger
added
approved-by-lead
…, missing #257/#259 Five fixes from AITL's rework of Codex R1's three blockers plus one self-caught, all in prose he wrote and I pasted: 1. output-guard CHANGELOG bullet: rewritten. It guards the OUTGOING REQUEST body sent upstream, not the response — proxy/extensions/ output-guard{,-stash}.mjs both hook onRequest and restore the client's original request body. Adds the CACHE_FIX_OUTPUT_GUARD=1 default-off gate and the fail-open semantics. 2. "first 243 live firings" claim: deleted. Contradicts the beta warning eleven lines above. Sourced from Gunther's #278 commit body without provenance; can't be "live" on a dogfood host that ran v4.3.0 all window. Deleting rather than hedging. 3. PR #257 added to Fixed. --remote-control routed 127.0.0.1 traffic through the proxy, breaking HTTP/SSE-transport MCP servers — a v4.3.0 regression on the release's own headline feature. 4. PR #259 added to Fixed. tools/rates.json was missing claude-opus-5 entirely, so session-budget-breaker's dollar ceiling priced Opus 5 at zero and silently never tripped. Codex filed as optional; the silent-safety-lever-off condition promotes it to required. 5. Same output-guard direction error in the promote-criteria doc at line 44 — self-caught after Codex's review. The doc is what sys_admin reads during the soak to decide whether a fire holds the release, so correct direction matters more here than in the CHANGELOG. Two sentences rewritten to say "outgoing request body" and to name that the failure is always in our chain, not upstream. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GvZKP1JeXgHFCovTaAPT5B
…umulated since v4.3.0 (#325) * release: v4.4.0-beta.0 + promote criteria First-exposure beta for the ten features accumulated since v4.3.0. The current dogfood-host proxy has been on v4.3.0 for over a week, predating every one of them; the 24-48h soak beginning on the operator's restart onto this build is not a verification pass, it is the first time these features execute against live traffic. Publish under npm `next` dist-tag, NOT latest. New: `docs/releases/v4.4.0-beta-promote-criteria.md` — the five criteria that gate promote from beta to latest, with an explicit baseline (v4.3.0 hit rate + cache_creation per turn), the synthetic-fire caveat on the output-guard criterion, and the waiver policy for #272's needs-sim-validation label. Full CHANGELOG entry to be added by AITL before merge; this commit is the mechanical shape only so tests can run against the tagged version and the promote gate has a citable artifact. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GvZKP1JeXgHFCovTaAPT5B * docs: CHANGELOG entry for v4.4.0-beta.0 + contributors updates CHANGELOG section drafted by AITL — the ten features accumulated since v4.3.0 framed as the attribution series, with the beta context (dogfood host ran v4.3.0 the whole window; the soak is first exposure) prominent in the header and the promote-criteria doc linked. Contributors: three additions (Gunther-Schulz, anupamme, thepiper18) and one extension (codeslake's entry now covers PR #261/#283/#296). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GvZKP1JeXgHFCovTaAPT5B * docs(readme): remove three blank lines between new contributor entries Per AITL R0 on 12c5b03. CommonMark: a single blank line between any two list items makes the whole list loose, so paragraph-spacing every entry from @bilby91 down. Contributors list is 24 items tight; the new entries need to match. Blank line before "If you contributed..." stays — that separates the list from the following paragraph and is correct. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GvZKP1JeXgHFCovTaAPT5B * docs: Codex review artifact for PR #325 round 1 * docs: fix Codex R1 findings — output-guard direction, 243-fires claim, missing #257/#259 Five fixes from AITL's rework of Codex R1's three blockers plus one self-caught, all in prose he wrote and I pasted: 1. output-guard CHANGELOG bullet: rewritten. It guards the OUTGOING REQUEST body sent upstream, not the response — proxy/extensions/ output-guard{,-stash}.mjs both hook onRequest and restore the client's original request body. Adds the CACHE_FIX_OUTPUT_GUARD=1 default-off gate and the fail-open semantics. 2. "first 243 live firings" claim: deleted. Contradicts the beta warning eleven lines above. Sourced from Gunther's #278 commit body without provenance; can't be "live" on a dogfood host that ran v4.3.0 all window. Deleting rather than hedging. 3. PR #257 added to Fixed. --remote-control routed 127.0.0.1 traffic through the proxy, breaking HTTP/SSE-transport MCP servers — a v4.3.0 regression on the release's own headline feature. 4. PR #259 added to Fixed. tools/rates.json was missing claude-opus-5 entirely, so session-budget-breaker's dollar ceiling priced Opus 5 at zero and silently never tripped. Codex filed as optional; the silent-safety-lever-off condition promotes it to required. 5. Same output-guard direction error in the promote-criteria doc at line 44 — self-caught after Codex's review. The doc is what sys_admin reads during the soak to decide whether a fire holds the release, so correct direction matters more here than in the CHANGELOG. Two sentences rewritten to say "outgoing request body" and to name that the failure is always in our chain, not upstream. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GvZKP1JeXgHFCovTaAPT5B * docs: Codex review artifact for PR #325 round 2 * docs: Codex review artifact for PR #325 round 2 * docs: Codex R2 blocker fix + criterion-3 Q5h-block exclusion Two edits, one commit, both AITL-authored, both pasted verbatim after union hygiene scan (hostname + operator-path + SSH + UUID + IPv4): 1. CHANGELOG output-guard invariant list: "role alternation" replaced with "role validity and system-message placement". checkRoles at output-guard.mjs:66-77 does not compare adjacent roles; it rejects roles outside user/assistant/system and rejects system at messages[0]. Mid-conversation system messages are LEGAL because deferred-tool-rewrite injects them. Codex R2's single blocking finding; the R1 rewrite enumerated invariants from function names and misread this one. 2. Promote-criteria criterion 3: adds a Caveat (a) for the Q5h-block- longer-than-TTL case. AITL measured on 2026-08-07 that a 1h53m fleet block on a 1h TTL produces 5.5M of 5.6M window cache_creation from nine cold-start turns at 0% hit rate — visually indistinguish- able from prefix-corruption regression under the criterion's rule. Without the exclusion, the first throttled afternoon during the soak would read as insertion-normalization busting the prefix, and #272's waiver failure-mode-1 would corroborate it into holding a good release. Two artifacts inheriting one blind spot is not independent confirmation. The exclusion carries the measurement, the triage procedure, and the persistence-not-magnitude distinguisher. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GvZKP1JeXgHFCovTaAPT5B * docs: Codex review artifact for PR #325 round 3 --------- Co-authored-by: vsits-proxy-builder[bot] <279815601+vsits-proxy-builder[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: vsits-codex-review-agent[bot] <279008405+vsits-codex-review-agent[bot]@users.noreply.github.com> Co-authored-by: Chris Nighswonger <chris.nighswonger@veritassuperaitsolutions.com>
cnighswonger#281 sat ten days after its dependency merged, and every reading of it was true about what it looked at: the thread was quiet, so a fresh read graded it "ball with nobody", and pr-rounds agreed — correctly, by its own definition, since nobody had posted. The predicate is the gap, not the reading. A PR becomes actionable without anyone posting when its blocker clears, and the writer has no way to see that. Booked READY here rather than folded into the doorbell RECORD above it: that entry's realizing write is a hook in dotfiles, while this one resolves to tools/pr-rounds.mjs in this repo — folding it there would have made it dispatchable by nobody, which is the exact defect that entry documents about itself. Noticed while booking, not swept: the entry at BACKLOG.md:9850 says cnighswonger#278 is CONFLICTING and needs a rebase. cnighswonger#278 merged 2026-08-06. Same class, one instance older.
…ys ago The entry was booked 2026-08-05 on a correct DIRTY reading and cnighswonger#278 merged the next day. Nothing in its design was executed; it moves to the closure home unbuilt rather than being deleted, because the class is what matters — an entry killed by a MERGE decays in silence, since nothing posts to the thread and reading the entry cannot reveal it. The section's state table is replaced rather than appended to: it claimed four PRs were awaiting upstream that have since merged. Nine of twelve merged, one closed, two still open, all re-read from the API.
Population derived, not hand-listed: all 18 PR references in live sections resolved against the upstream API, 13 closed, and the 14 entries citing a closed one read individually. Eight close, six stay -- history, a live watch, a park owned by another repo. Each closes against an executed check rather than a reading of the entry. The push-scan file-half filter is closed by the guard's own output during this session's push, not by its commit message. The cnighswonger#278 rebase round is closed by the merge timestamp that made its held push moot -- the second cnighswonger#278 entry to close that way today. Bodies move unedited; the dispositions live in the pass entry. What the pass did NOT sweep is named in it: entries resting on capture aliases, live file state or matrix rows are a different stale-risk class and remain unexamined.
…eam made to our own PRs Fork main was 37 behind and drifting. Sized against the merge base (76d586d), not tree-to-tree: 97 files incoming, +22,197/-782. Twenty-eight files actually conflicted, not the 56 that "changed on both sides" suggested — `git merge-tree` is the instrument for that question. WHY MOST OF IT CONFLICTED AT ALL. Twenty-two of the 28 conflict only because our own merged upstream PRs re-import our own work as an independent add: none of those files exists at the merge base, so git sees add/add with no ancestor. Each has exactly one upstream commit touching it and each is a Gunther Schulz PR (cnighswonger#272 cnighswonger#273 cnighswonger#275 cnighswonger#278 cnighswonger#279 cnighswonger#280 cnighswonger#282 cnighswonger#306). Checked with --full-history, because plain `git log -- <path>` prunes under history simplification. WHERE "TAKE OURS" WOULD HAVE BEEN WRONG — upstream amended these in review of our own PRs and the fork never took them back. All four are now in: - upstream-change-detection (cnighswonger#282): a count-only DECREASE alarms again. We suppressed both directions, so every compaction and truncation was silently absorbed by the one detector whose job is to notice unanticipated shape changes. Red-first: with the old rule restored the new bite fails alone, 16 and 17 stay green. - request-capture (cnighswonger#275): capture dir created 0700 (the listing leaks session keys through filenames) and boot records route the environment through `publishableGates`, so non-allowlisted CACHE_FIX_* VALUES are redacted. We dumped all of them, and captures feed harvest, which feeds fixtures in a PUBLIC tree. Of the 113 CACHE_FIX_* names this proxy reads, 73 are now redacted; nearly all are path-, URL- or credential-valued. - prefix-diff (cnighswonger#280): the cross-key retention sweep. We had no equivalent anywhere and the snapshot dir holds 28,157 files. Ported with tests upstream never wrote, because it DELETES and its scope boundary is load-bearing here: 13,774 of those files are `-canon.json` / `-relocated.json` / `-rungs.json`, fork-owned LIVE STATE whose deletion rotates the key its owner reads. Two mutations prove the bites — widening the scope regex, and disabling the age pass — each goes red on the right ones. - thinking-block-sanitize (cnighswonger#279): v2's continuation protection is tail-scoped. Inert here (CACHE_FIX_THINKING_SANITIZE unset → v1), and v1 is byte-identical across a three-case corpus. Our own suite already carried the bite asserting upstream's v2 contract and was failing on it. WHERE TAKING UPSTREAM WOULD HAVE BROKEN PRODUCTION. `proxy/extensions.json` is purely a formatting conflict — no shared entry's settings differ — but upstream still rosters `messages-cache-breakpoint`, which exists at the base and which this fork deleted, entry AND extension file. Resolved to ours exactly. FOREIGN WORK TAKEN: Junyong Lee's RFC 7230 absolute-form request-targets (cnighswonger#261, which is what stops the CC auto-updater 404ing through forward mode), the two ca-trust fixes (cnighswonger#283, cnighswonger#296), the read-dedupe ordering correction (cnighswonger#310), CHANGELOG, and the regenerated pt-br guide. FORK ADAPTATIONS, each commented at its site: upstream's new tests assume upstream's `~/.claude` layout while this fork resolves through XDG (capture dir, CA dir, quota-status), and upstream's new temp-dir sites are routed through tools/tmpdir.mjs so the no-raw-mkdtemp guard stays closed rather than being opened for them. `CACHE_FIX_COALESCE_SIDECAR` is added to the publishable gate allowlist — it is the one of our 12 serving gates upstream's list did not cover, and without it the boot record could no longer reproduce the serving configuration. ONE THING DELIBERATELY NOT TAKEN. Upstream gates all prompt-text persistence on CACHE_FIX_PREFIXDIFF_CONTENT, off by default; this fork has no such gate and always stores system text and message previews. That is a real exposure on a machine whose proxy fronts every session, and it trades directly against this fork's byte-level attribution. It is an operator decision, booked, not taken inside a merge — and the security bite now asserts the fork's actual contract so it goes red the day the gate is ported. Leak gate proven still firing after resolution, both arms on one real fixture: untouched, `absence-scan: clean`, exit 0; with a freshly generated v4 UUID in `.key`, `FINDING capture-uuid $.key`, exit 2. Full suite green at this commit: 3514 tests, 3502 pass, 0 fail, 12 skipped. Restart NOT taken: the incoming set touches state keys and freeze logic, so row 3's transparency argument does not carry and the boundary is the operator's to choose. Pin bump owed in dotfiles (proxy/ changed). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012bEWVpDVQu9f5bMaqb2W4t
2 participants
Why
Once a proxy runs body-mutating extensions (#272/#273, or any future one), the failure mode that matters most isn't a missed optimization — it's shipping a corrupted response. This guard is the last line: it runs after every mutator and asks a different question than any unit test can — is what we're about to send still the response upstream sent, in every way the client depends on?
What
output-guard-stash(order 55) stashes the pristine body before any mutator runs.output-guard(order 690) validates structural invariants on the outbound response (tool_use/tool_result adjacency among them). On violation it restores the stashed original and logs the diff — a mutation bug becomes one logged line instead of a broken session.CACHE_FIX_OUTPUT_GUARD=1.Evidence
10 tests, including gate 1: the guard fires zero times across every class-matrix corpus (a guard that fires on legitimate traffic trains its reader to ignore it — that's a failing guard, and its first 243 live firings on our fleet were exactly that: its own missing exemption, since fixed and pinned). Running in production alongside #272/#273 for two days, 0 safety violations across ~2.5 GB of daily replay.
🤖 Generated with Claude Code