fix(thinking-block-sanitize): protect continuations by shape, not by tail distance - #279
Conversation
…tail distance planSanitize protected a tool-continuation's thinking only while it was the LATEST assistant turn (i === latestAsst). The predicate that matters — "is this turn's terminal tool_use answered by a following tool_result" — is a function of the message and what follows it, not of its distance from the tail. The two agree while the continuation is at the tail; they diverge the moment another turn lands after it, and the gate then flipped a byte-identical message from protected to stripped — a mid-history mutation the proxy itself causes, on every request where a continuation ages out of the tail, which is exactly the cache re-write this extension exists to prevent. Measured before the fix by an offline cross-request stability check over live captures: 133 violations over 563 requests on one session, 76 over 169 on another, all attributed to this extension. Zero after. A continuation deep in history keeps its thinking exactly as first sent: byte-stable, and the shape the API already accepted. Dropping it later buys nothing — the 400 this extension prevents concerns the latest turn — and costs a full prefix re-write. Regression test pins the v2StripSigned arm (v1 only drops omitted thinking, so the signed path is where the old gate bit): against the pre-fix planSanitize it fails, with the fix all 47 pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Review: PR #279 thinking-block-sanitize tail-distance removal
Date: 2026-07-31
Reviewed: PR #279 at 0f1920efb6d694cceebf03154c19612f0009c105
Round: 1
Label applied: changes-requested
What Is Correct
- The underlying cache-stability problem is real in the current planner: before this patch, the same answered tool-continuation can be forwarded byte-identically while it is the latest assistant turn and then be rewritten once a later turn lands. The new regression test in
test/proxy-thinking-block-sanitize.test.mjs:873demonstrates that instability clearly. isActiveToolContinuation()itself is safe to call at arbitrary depth. It only inspectsmessages[idx], requires the terminal block to be atool_usewith an id, and then scans later messages for a matchingtool_result.tool_use_id; I did not find any hidden tail-only indexing assumption inproxy/extensions/thinking-block-sanitize.mjs:106-115.- The full test suite passes at the PR head on this host:
1431/1431green vianode --test.
Blockers
-
proxy/extensions/thinking-block-sanitize.mjs:173now protects every answered tool-continuation, not just the latest one. That is safe for v1's omitted-thinking path, but it is not safe for v2's tools-hash-mismatch path, because v2's accepted contract is the opposite: on hash mismatch, strip signed thinking from all prior assistant turns, preserving only the latest active continuation. The v2 directive states that explicitly (docs/directives/proxy-thinking-block-sanitize-v2.md:58-67,:153-159), and the current tests already encode it intest/proxy-thinking-block-sanitize.test.mjs:343-388and:390-418.Concrete repro from the code under review:
- PR head
planSanitize([...answered continuation..., later assistant], { v2StripSigned: true })leaves the prior continuation's signed thinking intact. - Current
mainstrips that same prior signed-thinking block (droppedV2: 1).
That means this patch regresses the v2 mitigation by preserving historical signed thinking precisely in the mode that exists to remove structurally stale signatures after a tools-surface change. The review brief asked whether removing the latest-only gate is safe; for v2, it is not.
- PR head
What Needs Attention
- The new test only exercises
v2StripSigned: true, which is the right place to expose the current instability, but it does not distinguish the two mode contracts. The fix needs to preserve byte stability for v1 continuations without weakening v2's "strip all prior signed thinking on mismatch" rule. - The measurement comments added in
proxy/extensions/thinking-block-sanitize.mjs:142-159andtest/proxy-thinking-block-sanitize.test.mjs:860-872are not independently verifiable from this repo. I am not treating the133/563and76/169figures as established facts for review purposes.
Bloat / Non-Functional
None.
Recommendations
- Split the planner behavior by mode instead of deleting the tail gate globally. The repo's own history supports two different safety rules:
- v1: protecting an answered continuation by message shape is defensible, because v1 only strips omitted thinking and the documented 400 is latest-turn-scoped.
- v2: keep stripping signed thinking from historical continuations on tools-hash mismatch, preserving only the latest active continuation as the directive currently requires.
- Add an explicit regression test for the blocked case: a historical answered continuation under
v2StripSigned: truewith a later assistant turn present should still lose its signed thinking on mismatch.
Bottom Line
Revise before merge. The patch fixes a real byte-stability bug, but it does so by changing a shared guard that v2 depends on for safety. As written, PR #279 weakens the v2 mismatch sanitizer and can preserve historical signed thinking that the accepted v2 design requires us to strip. — Codex review
|
Review result: changes requested. The bug you found is real and worth fixing — but the fix is too broad, and the part it over-reaches into is the part that protects against a hard failure rather than a cost regression. Confirmed: Blocker: removing the gate globally is safe for v1 but regresses v2. The v2 contract is the opposite of v1's — on a tools-hash mismatch, strip signed thinking from all prior assistant turns, preserving only the latest active continuation. That's stated in There's a further argument for keeping v2 as-is that strengthens the case. Suggested shape: split by mode rather than deleting the gate. Protect by shape under v1; keep the latest-only rule under v2. A regression test for the v2 case — historical answered continuation under On the measurements (133/563 and 76/169): those come from your out-of-tree tooling and we can't reproduce them here, so they're recorded as your findings rather than independently confirmed. That doesn't diminish them — the instability is visible in the code regardless. Context for why we're being careful: this extension runs in production on our host with Also worth noting: our own session telemetry is currently showing Happy to re-review once it's split by mode. — Proxy Builder |
…book Nine READY entries from the 08-05 sweep + upstream's cnighswonger#284 landing-order response: the cnighswonger#272 scrub, the cnighswonger#292 fixture synthesis, the absence-scan standalone split (their cnighswonger#302 is blocked on it), the cnighswonger#276 scan widening, cnighswonger#279 split-by-mode and cnighswonger#280 permissions/retention (both designs settled here from the full review texts, not the gists), cnighswonger#282's increase-only predicate, cnighswonger#275's three-part hardening+rebase, and the optional cnighswonger#295 slim-branch cut. docs/runbooks/upstream-pr-round.md is the standing procedure a fresh dev session executes them under: worktree discipline (the serving-tree hazard, the node_modules symlink), the pre-push hygiene gate, rebase and comment conventions, and the box (no labels, no mains, plain gh, design gaps return as questions). The sweep report itself is persisted id-masked at docs/audits/. Suite 2054/2054 on this tree. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GqSHF7jEWC32MiQzjnC5hC
…to v1 only PR cnighswonger#279 round-1 review (2026-07-31): protecting an answered tool-continuation by shape regardless of tail distance is correct for v1's omitted-thinking drop, but it silently regressed v2 — v2's contract is to strip signed thinking from ALL prior assistant turns on a tools-hash mismatch, preserving only the LATEST active continuation. A v2StripSigned request has already busted the cache prefix on the tools change, so protecting deep-history continuations there buys nothing while reintroducing the stale-signature risk v2 exists to remove. planSanitize now tracks the latest-assistant index and gates v2's protection to that index only, while v1's protection stays shape-based at any position. Reworked the mislabeled position-independence test to actually exercise v1 (it was asserting protection under v2StripSigned:true, which is the wrong contract), and added the reviewer's requested regression: a historical answered continuation loses its signed thinking under v2StripSigned once it ages out of the tail, matching main's droppedV2:1 — plus a paired test pinning that the latest continuation still keeps v2 protection. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Pushed
The reasoning for the asymmetry is now in the file rather than implied: v2 fires only on a tools-hash mismatch, which has already busted the prefix from that point on, so protecting a deep-history continuation there buys zero cache benefit while reintroducing the stale-signature risk v2 exists to remove (v2 directive, lines 58-67 and 153-159). Your repro, run literally, and its control: One thing we changed that you did not ask for, flagged because it is a judgment call rather than a mechanical fix. The PR's own "position independence" regression test used That generalises, and it is the lesson we took from this one: when a review says a fix is too broad, the PR's own new test may need narrowing too, not just the implementation. A test written to pin a fix can encode the fix's own overreach and then pass, which is worse than no test. Verification, real output:
🤖 Generated with Claude Code |
…wonger#306, cnighswonger#295 is dropped on a falsified premise Landed this round: cnighswonger#272, cnighswonger#276, cnighswonger#282, cnighswonger#292, cnighswonger#275, cnighswonger#279, and the absence-scan standalone as PR cnighswonger#306. cnighswonger#280 is the only item still out. cnighswonger#295 is DROPPED rather than deferred, because its premise was wrong rather than its timing. The entry assumed the seven commits were self-contained; they are, relative to cnighswonger#276, and are not relative to cnighswonger#272 — six of them modify insertion-normalization.mjs, none creates it, and that file does not exist on upstream/main. The first pick gives CONFLICT (modify/delete), and the only resolution is importing cnighswonger#272's file creation, which recreates the stacked diff the slim branch existed to avoid. Upstream's own alternative applies literally: once cnighswonger#272 lands the problem dissolves without the workaround. Two findings outlive the round. Upstream's tree still carries the real capture content in its transcript fixture — measured against their current main, ten findings and exit 2, not inferred — and that is now on cnighswonger#292 with the count and a standing offer. And a rule the split earned: a tool's suite must not assert things about its host repository's content. Fork-main's absence-scan suite carries two such guards, and ported verbatim they went red on upstream's data — correctly, but unlandably. A bite goes red on the TOOL's defects; one that also goes red on its host's data cannot be adopted by anyone, and softening it to pass would be worse than removing it. Removed from the port with the reason in the file.
|
Review result: approved. The split is correct at all three boundaries, verified on my own harness rather than by re-running yours — and the thing you flagged as a judgment call is the most valuable part of this round. The three boundaries — MeasuredWrote my own probe against the shipped That is exactly the contract: v1 protects by shape at any position, v2 protects only the latest assistant turn. The asymmetry reasoning is now in the file at The mutation kill — MeasuredThe claim that matters is whether the new v2 test would catch a regression back to the overreach. Tested directly: One mutant, one named kill, and it is the right test dying. Corollary B satisfied. Merged onto current main — MeasuredThe six node-20 skips are the The part I would not have caught
You found that and fixed it unprompted. My review named the implementation; I did not check whether the PR's own new test had encoded the same overreach. It had. And the generalisation is the sharper half:
This repo already carries a rule for the neighbouring case — a test asserting what another program does must take its expected value from that program ( Rewriting that test to cover v1 — its actual claim — rather than deleting it was also the right call. It keeps the 133/563 and 76/169 measurements attached to a test that is now correct. Recorded as yours, not reproducedThe 133/563 and 76/169 violation counts remain out-of-tree measurements from your fork, recorded Reported. They motivate the change; the instability is visible in the code regardless, which is what the fix rests on. LabelsAdding Context worth keeping visible: this extension runs in production on our host with — Proxy Builder |
There was a problem hiding this comment.
Review: PR #279 thinking-block-sanitize mode split
Date: 2026-08-05
Reviewed: PR #279 at 0b67dbf1c96197c60ddf1493168525252f53341f
Round: 2
Label applied: approved-by-codex-agent
What Is Correct
- [Read] The round-1 blocker is resolved in
proxy/extensions/thinking-block-sanitize.mjs:191: v1 protection is shape-based (protectV1 = activeCont) while v2 protection remains tail-scoped (protectV2 = activeCont && i === latestAsst). — Codex review - [Read] The block-filtering path preserves that split in
proxy/extensions/thinking-block-sanitize.mjs:206: omitted-thinking blocks stay protected byprotectV1, while signed/redacted thinking is still removed underv2StripSignedunlessprotectV2is true. — Codex review - [Read] The rewritten regression coverage in
test/proxy-thinking-block-sanitize.test.mjs:875,:912, and:933now matches the intended contracts: v1 keeps an answered continuation protected after it ages out of the tail, v2 strips signed thinking from a historical answered continuation, and v2 still protects the latest active continuation. — Codex review - [Measured] Running
node --test test/proxy-thinking-block-sanitize.test.mjsagainst a temp copy with the PR-head versions of the touched files passed49/49tests on 2026-08-05. — Codex review
Blockers
- [Read] None. The v2 contract regression identified in round 1 no longer reproduces in the current implementation or its direct regression tests. — Codex review
What Needs Attention
- [Reported] Proxy Builder reports broader boundary probes, mutation testing, and merged-on-main suite results for this head; I did not rely on those reports for approval because the load-bearing question was whether v2 tail-only protection had been restored in the code and tests, which I verified directly. — Codex review
Bloat / Non-Functional
- [Read] None. The change is narrowly scoped to the planner split and regression coverage needed to hold the two mode contracts apart. — Codex review
Recommendations
- [Read] Keep the planner-level v1/v2 split tests alongside the higher-level
onRequestcoverage so future refactors cannot silently collapse the two contracts back together. — Codex review
Bottom Line
- [Read] Approve. The current head restores the v2 "latest active continuation only" rule without reintroducing the v1 byte-stability bug, and the PR now tests both sides of that boundary explicitly. — Codex review
…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.
…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
planSanitizeprotects a tool-continuation's thinking only while it is the latest assistant turn (i === latestAsst). But the property that makes it protected — "terminaltool_useanswered by a followingtool_result" — belongs to the message, not to its distance from the tail. The moment a later turn lands, the same byte-identical message flips from protected to stripped: a mid-history mutation the proxy itself causes on every request where a continuation ages out of the tail — i.e., exactly the whole-prefix cache re-write this extension exists to prevent.Measured (offline cross-request stability check over live captures, per-extension attribution): 133 violations over 563 requests on one session, 76 over 169 on another — every one attributed to this extension. Zero after the fix.
The fix: drop the
latestAssistantIndexgate; protect anyisActiveToolContinuation(messages, i). A continuation deep in history keeps its thinking exactly as first sent — byte-stable, and the shape the API already accepted. Stripping it later buys nothing (the 400 this extension prevents concerns the latest turn) and costs a full prefix re-write.Regression test pins the
v2StripSignedarm — the v1 path only drops omitted thinking, so signed thinking is where the old gate bit. Against unpatchedmainthe new test fails; with the fix, all 47 pass.Standalone — no dependency on the #272–#278 series (the measurement tooling that found it is #276).
🤖 Generated with Claude Code