diff --git a/CHANGELOG.md b/CHANGELOG.md index 710b179c..f5a8ff82 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,10 @@ **Token-gated warning.** `thinking_desync_risk` is computed from `context_tokens` against `CACHE_FIX_THINKING_RISK_HIGH_TOKENS` (default `340000`, just under the observed ~382K trip) and `CACHE_FIX_THINKING_RISK_WARN_TOKENS` (default `250000`). On first crossing into `high`, a one-time content-free stderr line is emitted. Block-count is recorded but does not yet gate the warning (calibrated fast-follow). `CACHE_FIX_THINKING_RISK=off` suppresses the warning signal (stderr line + risk field) while raw count telemetry keeps recording. +- **thinking-block-sanitize mitigation extension (#162), opt-in.** A new request-path extension (`proxy/extensions/thinking-block-sanitize.mjs`, order 550) that drops the *omitted* (`thinking:""` + signature) extended-thinking blocks CC re-sends on history-replay paths, before the request is forwarded — heading off the permanent `400 ... thinking blocks ... cannot be modified` wedge (upstream `anthropics/claude-code#63147`). This is the *mitigate* half (the *warn-before* half is session-health above). **Opt-in:** only runs when `CACHE_FIX_THINKING_SANITIZE=on` (default off) — it mutates request bodies and full live-coverage validation is pending. + + **Turn-selection rule (empirically resolved).** Drops omitted thinking from all prior assistant turns **and** the latest assistant turn — *unless* the latest turn is an active tool-continuation (its last block is a `tool_use` with a following `tool_result`), where the API requires the signed thinking intact and the proxy must not strip it (that case is uncoverable here — no env var both preserves thinking and avoids the wedge; `CLAUDE_CODE_DISABLE_THINKING=1`/`MAX_THINKING_TOKENS=0` stop it only by disabling thinking entirely, `DISABLE_INTERLEAVED_THINKING=1` does not stop the 400, so the answer there is heal/retire). Never touches non-empty thinking; `redacted_thinking` is out of scope for v1 (a full scan of the worst-case wedged transcript found zero). Deterministic and cache-prefix-stable. Emits a per-request `thinking_blocks_dropped` count into the per-session JSON (counts only — never content), via the existing `cache-telemetry` writer. + ### Fixed - **`ttl-management`: never inject a TTL into `thinking` / `redacted_thinking` blocks (#157).** `injectTtl` iterated every block in the request; if a `cache_control: {type: "ephemeral"}` breakpoint landed on a thinking block (possible on Opus 4.7 interleaved-thinking turns), it rewrote the block to add `ttl`, which mutates a signed thinking block — the API rejects that with `400 ... thinking blocks ... cannot be modified`. The injector now skips `thinking`/`redacted_thinking` blocks entirely (the chokepoint covers both the system-block and message-block paths). Defensive hardening: this was not the cause of the 2026-05-28 interleaved-thinking incident (that was CC-side, `anthropics/claude-code#63172`), but it's a real latent mutation path with zero upside to keeping. Regression tests pin the skip and the still-inject-on-non-thinking happy path. diff --git a/README.md b/README.md index d19d256d..0a588729 100644 --- a/README.md +++ b/README.md @@ -29,7 +29,7 @@ That's it. The proxy applies all 7 cache-fix extensions automatically. No wrappe ### What the proxy does -On every `/v1/messages` request, 8 extensions run in order: +On every `/v1/messages` request, 9 extensions run in order (one opt-in): | Extension | What it fixes | |-----------|--------------| @@ -41,6 +41,7 @@ On every `/v1/messages` request, 8 extensions run in order: | `cache-control-normalize` | Normalizes cache_control markers across messages | | `cache-telemetry` | Extracts cache stats from response headers → `~/.claude/quota-status/{account.json,sessions/.json}` | | `session-health` | Observes per-session thinking-desync risk (context size + thinking-block count) and warns before a session reaches the danger zone. Read-only | +| `thinking-block-sanitize` | Drops omitted (empty-text) thinking blocks to head off the CC thinking-desync `400` (#63147). **Opt-in** (`CACHE_FIX_THINKING_SANITIZE=on`) | Extensions are hot-reloadable — add, remove, or modify `.mjs` files in `proxy/extensions/` and changes apply to the next request without restarting. Configuration in `proxy/extensions.json`. @@ -746,6 +747,18 @@ Token thresholds are anchored to the observed ~382K-token trip with margin; the | `CACHE_FIX_THINKING_RISK_HIGH_TOKENS` | `340000` | Context-token level at which risk becomes `high` and the one-time stderr warn fires. | | `CACHE_FIX_THINKING_RISK` | unset (on) | Set to `off` to suppress the warning signal (stderr line + `thinking_desync_risk` field). Raw count telemetry keeps recording. | +## Thinking-block sanitize (proxy mode, opt-in, thinking-desync mitigation) + +The *mitigate* half of the thinking-desync response (the *warn-before* half is session-health above). On history-replay paths (resume / `--continue` / auto-compaction / parallel-tool-cancel), Claude Code re-sends prior assistant turns' extended thinking in the **omitted** shape `{ "type":"thinking", "thinking":"", "signature":"" }`. The API rejects modified thinking in the **latest** assistant message with a permanent `400 … thinking … blocks cannot be modified`, which wedges the session on every subsequent turn (upstream root cause: [anthropics/claude-code#63147](https://github.com/anthropics/claude-code/issues/63147)). + +The `thinking-block-sanitize` extension drops those omitted blocks — which the API treats as optional history — from the request before it is forwarded. Empirically-resolved turn-selection rule: drop omitted thinking from **all prior assistant turns and the latest assistant turn, unless the latest turn is an active tool-continuation** (its last block is a `tool_use` answered by a following `tool_result`). In that one case the API requires the signed thinking intact and the proxy cannot restore the emptied text, so it leaves the turn untouched. **No env var both preserves thinking and avoids the wedge for that case:** `CLAUDE_CODE_DISABLE_THINKING=1` / `MAX_THINKING_TOKENS=0` stop the wedge only by disabling thinking entirely (lossy — no reasoning), and `DISABLE_INTERLEAVED_THINKING=1` does *not* stop the `400` — so there the answer is don't-resume + heal/retire the session. That is exactly why the proxy mitigation matters: **it is the only path that preserves reasoning while avoiding the wedge** for the history-replay paths it covers. Non-empty thinking is never touched; `redacted_thinking` is out of scope for v1. + +**Opt-in.** v1 ships behind `CACHE_FIX_THINKING_SANITIZE=on` (default off): it mutates request bodies and full live-coverage validation is pending. The transform is deterministic and cache-prefix-stable, and emits a per-request `thinking_blocks_dropped` count into the per-session JSON (counts only — never content) that complements the session-health signal. + +| Env var | Default | Purpose | +|---------|---------|---------| +| `CACHE_FIX_THINKING_SANITIZE` | unset (off) | Set to `on` to enable the request-path drop of omitted thinking blocks. Off = no-op (no mutation, no telemetry). | + ## System prompt rewrite (preload mode, optional) The interceptor can rewrite Claude Code's `# Output efficiency` system-prompt section. Disabled by default. Enable with `CACHE_FIX_OUTPUT_EFFICIENCY_REPLACEMENT`. See [docs/output-efficiency-prompts.md](docs/output-efficiency-prompts.md) for the three known prompt variants and usage instructions. diff --git a/docs/code-reviews/pr162-thinking-sanitize-directive-codex-confirmation-2026-05-28.md b/docs/code-reviews/pr162-thinking-sanitize-directive-codex-confirmation-2026-05-28.md new file mode 100644 index 00000000..f90c12f7 --- /dev/null +++ b/docs/code-reviews/pr162-thinking-sanitize-directive-codex-confirmation-2026-05-28.md @@ -0,0 +1,41 @@ +# Review: proxy-thinking-block-sanitize directive + +Date: 2026-05-28 +Reviewed: PR #162 directive (`docs/directives/proxy-thinking-block-sanitize.md`) +Label applied: reviewed-by-codex-agent + +## What Is Correct + +- The remaining schema blocker is cleared. `redacted_thinking` is no longer part of the active v1 strip rule in either the threat model or behavior section, and the directive now explicitly defers it to Out of scope with the correct opaque `{ "type":"redacted_thinking", "data":"..." }` schema rationale ([directive lines 21, 28, 43](../directives/proxy-thinking-block-sanitize.md)). +- The v1 behavior is now internally consistent: the transform is scoped to prior-turn omitted `thinking` blocks only, non-empty `thinking` stays untouched, the latest assistant message remains protected pending empirical coverage validation, and empty-content assistant messages are dropped rather than rewritten ([directive lines 28-32](../directives/proxy-thinking-block-sanitize.md)). +- The non-functional framing is still sound. `Load-bearing? yes` remains correct for a request-path body mutator, the Chris-review gate is present, the determinism requirement is explicit, and the v1 default-off posture is still the right safety call until Open Question 1 is answered with a real captured repro ([directive lines 18-24, 32, 47-49](../directives/proxy-thinking-block-sanitize.md)). +- Open Question 1 remains correctly framed as the pre-implementation empirical gate: prove whether prior-turn dropping alone clears the latest-message-named 400, and widen only if a captured repro shows the latest completed non-continuation turn also needs stripping. The no-touch boundary for an active tool-continuation latest turn is preserved ([directive lines 30, 47-49](../directives/proxy-thinking-block-sanitize.md)). + +## Blockers + +None. + +## What Needs Attention + +- Resolve Open Question 1 against a captured wedged request before implementation locks. Approval here is for directive precision and scope, not for skipping the live coverage check. +- Resolve Open Question 2 toward dropping the now-empty assistant message, which the current behavior section already states. Keep implementation and tests aligned with that choice. +- When implementation starts, keep telemetry counts-only as specified and do not let the request walker expand past the directive's stated small-extension budget. + +## Bloat / Non-Functional + +None. The directive is now tighter than the previous pass and removes the only remaining schema overreach instead of adding a special-case abstraction. + +## Size Baseline + +- `docs/directives/proxy-thinking-block-sanitize.md` — 54 LOC — compact directive with a clear v1 boundary; the remaining work is empirical coverage validation, not spec expansion. +- `preload.mjs` — 2881 LOC — existing implementation surface; the directive still sets the right expectation that this should land as a small extension reusing current body-walk patterns. + +## Recommendations + +- Start implementation only after the captured-request validation in Open Question 1 settles the exact turn-selection rule. +- Keep `redacted_thinking` out of v1 unless a real repro demonstrates it participates in the rejection and a separate schema-accurate rule is added. +- Preserve the opt-in release posture for the first implementation cut. + +## Bottom Line + +Approve the directive for implementation. The one remaining blocker from the last pass is resolved: `redacted_thinking` is cleanly out of the v1 empty-text predicate, and the spec now hands implementation a precise, internally consistent v1 scope while keeping the load-bearing coverage question as an explicit pre-implementation gate. diff --git a/docs/code-reviews/pr162-thinking-sanitize-directive-codex-rereview-2026-05-28.md b/docs/code-reviews/pr162-thinking-sanitize-directive-codex-rereview-2026-05-28.md new file mode 100644 index 00000000..d6076441 --- /dev/null +++ b/docs/code-reviews/pr162-thinking-sanitize-directive-codex-rereview-2026-05-28.md @@ -0,0 +1,41 @@ +# Review: proxy-thinking-block-sanitize directive + +Date: 2026-05-28 +Reviewed: PR #162 directive (`docs/directives/proxy-thinking-block-sanitize.md`) +Label applied: changes-requested + +## What Is Correct + +- Blocker 1 is cleared. The directive now explicitly states that the omitted `{"type":"thinking","thinking":"","signature":"..."}` shape is normal, not corruption, and it reframes the transform as dropping prior-turn optional history rather than claiming a uniquely broken wire shape ([directive lines 8-16](../directives/proxy-thinking-block-sanitize.md)). +- The NFR section is still valid: it is present and non-empty, `Load-bearing? yes` is the correct classification for a shared request-path body mutator, and the Chris-review gate remains appropriate for this risk class ([directive lines 18-24](../directives/proxy-thinking-block-sanitize.md)). +- The v1 opt-in posture is the right release-safety call. A request-body mutator with unresolved live-coverage validation should not ship default-on, and the directive now reflects that clearly with `CACHE_FIX_THINKING_SANITIZE=on` defaulting to off ([directive lines 30-32, 46-48](../directives/proxy-thinking-block-sanitize.md)). +- Open Question 1 is the right place to hold the remaining coverage uncertainty. The spec no longer pretends that a prior-turn-only drop is already proven to clear every latest-message-named 400, and the "never touch an active tool-continuation turn" boundary is sound ([directive lines 30, 46-48](../directives/proxy-thinking-block-sanitize.md)). + +## Blockers + +- `redacted_thinking` is still specified with the wrong predicate. The directive's threat-model and behavior text still groups `redacted_thinking` under the same omitted/empty-text rule as regular `thinking` blocks ([directive lines 21, 28](../directives/proxy-thinking-block-sanitize.md)), but Anthropic's current extended-thinking docs define `redacted_thinking` as a distinct opaque block, `{ "type":"redacted_thinking", "data":"..." }`, and explicitly distinguish it from omitted `thinking` blocks with empty `thinking` text. If `redacted_thinking` is meant to be in scope as optional prior-turn history, it needs its own schema-aware rule and justification; otherwise it should be removed from v1 scope. As written, blocker 2 is not resolved and the directive is still not precise enough to hand to implementation. Source: https://platform.claude.com/docs/en/build-with-claude/extended-thinking + +## What Needs Attention + +- Resolve Open Question 1 into a concrete turn-selection rule before implementation starts. I agree with the directive's gating posture: validate against a captured wedged request whether dropping prior completed turns is sufficient, and only widen to the latest completed non-continuation turn if the capture proves that is the failing case. Do not generalize this to an active tool-continuation turn. +- Open Question 2 should resolve to dropping the now-empty assistant message, not synthesizing placeholder text. A placeholder mutates conversation bytes and semantics for no benefit, while the proxy is already operating on a wire-format message list rather than transcript node IDs. +- The testing section should regain an explicit `redacted_thinking` case once the rule is corrected, because the current unit list only names omitted `thinking` coverage ([directive lines 52-53](../directives/proxy-thinking-block-sanitize.md)). + +## Bloat / Non-Functional + +None. The directive is tighter than the prior version, and the remaining problem is rule precision, not size or abstraction. + +## Size Baseline + +- `docs/directives/proxy-thinking-block-sanitize.md` — 53 LOC — compact directive; the main remaining risk is schema precision around `redacted_thinking` and final turn coverage, not sprawl. +- `preload.mjs` — 2881 LOC — existing implementation surface; the directive still sets the right expectation that this should stay a small extension rather than grow a new subsystem. + +## Recommendations + +- Remove `redacted_thinking` from the v1 behavior unless you can define a separate, documented predicate for it that matches the actual `{type:"redacted_thinking", data:"..."}` schema. +- Keep v1 opt-in until Open Question 1 is answered with a real captured replay request and the exact turn-selection rule is written into the behavior and testing sections. +- Once coverage is validated, encode the rule explicitly in behavior/tests rather than leaving "latest completed non-continuation" as an implementation-time inference. + +## Bottom Line + +Changes requested again for directive stage, but the scope is much narrower now. The prior "normal omitted shape vs corruption" blocker is cleared, the opt-in posture is correct, and the coverage question is being handled in the right place. The remaining blocker is that `redacted_thinking` is still written as if it participated in an empty-text omitted-shape matcher, even though Anthropic documents it as a separate opaque `data` block. Fix that schema mismatch and then this is ready for another pass. diff --git a/docs/code-reviews/pr162-thinking-sanitize-directive-codex-review-2026-05-28.md b/docs/code-reviews/pr162-thinking-sanitize-directive-codex-review-2026-05-28.md new file mode 100644 index 00000000..2ad2c96d --- /dev/null +++ b/docs/code-reviews/pr162-thinking-sanitize-directive-codex-review-2026-05-28.md @@ -0,0 +1,46 @@ +# Review: proxy-thinking-block-sanitize directive + +Date: 2026-05-28 +Reviewed: PR #162 directive (`docs/directives/proxy-thinking-block-sanitize.md`) +Label applied: changes-requested + +## What Is Correct + +- The directive passes the new NFR gate: `## Non-Functional Requirements` is present, non-empty, and `Load-bearing? yes` is the correct classification for a shared request-path body mutator. The explicit Chris-review requirement is also correct. +- The overall cut is appropriately narrow for directive stage: one request-path transform, deterministic/stable output, counts-only telemetry, and explicit out-of-scope boundaries around disk repair and the in-flight/latest assistant message. +- The latest-assistant safety boundary is directionally right. Anthropic's current extended-thinking docs require preserving thinking blocks for the last assistant message during tool-use continuation, so "never touch the latest assistant message" is the correct hard stop for this mitigation. +- The testing checklist aims at the right execution paths: prior-turn strip, latest-turn no-touch, deterministic replay, and live validation against a real wedged request. +- Multiple confirmations on `anthropics/claude-code#63147` report that manually stripping historical thinking blocks from prior assistant messages can un-wedge replay paths, so a request-path mitigation in cache-fix is a reasonable direction to explore. + +## Blockers + +- `docs/directives/proxy-thinking-block-sanitize.md:8-12`, `:17`, `:24-28`, `:42-43` treat `{ "type":"thinking", "thinking":"", "signature":"..." }` as a uniquely corrupted shape. Anthropic's current extended-thinking docs say the opposite: on Opus 4.7/4.8, omitted thinking is the default response mode and returns regular `thinking` blocks with an empty `thinking` field plus a `signature`; they also state that any text placed in the `thinking` field of a round-tripped omitted block is ignored (see `https://platform.claude.com/docs/en/build-with-claude/extended-thinking`). As written, this directive would therefore strip the normal documented omitted-thinking shape from every prior assistant turn, not a narrowly identified corruption. That breaks the directive's own NFR claim that it removes only the "precisely-matched corrupted shape," and it makes the default-on posture in `:28` unsafe. +- `docs/directives/proxy-thinking-block-sanitize.md:24`, `:42` include `redacted_thinking` in the same "empty/whitespace text + non-empty signature" matcher, but Anthropic's current docs describe `redacted_thinking` as an opaque `{ "type":"redacted_thinking", "data":"..." }` block, not a text-plus-signature shape (same source). Without captured failing requests that show a distinct corrupted `redacted_thinking` wire format, this part of the rule is not safely implementable and risks removing blocks that the protocol expects to be preserved unchanged. + +## What Needs Attention + +- Resolve open question #1 in the directive itself instead of leaving it to implementation. If a prior assistant message becomes empty after strip, dropping the message is the better contract than synthesizing a placeholder text block: a placeholder mutates semantics and cache prefix bytes, while the Messages API already tolerates consecutive same-role turns by combining them. +- Make the "latest assistant message" definition explicit in the behavior section: "the highest index `i` where `body.messages[i].role === \"assistant\"`." That stays well-defined even when the request ends with a user `tool_result` message. +- Keep the rationale grounded in observed replay behavior rather than the current signature-desync explanation. The upstream issue thread now contains competing hypotheses, including reports that empty `thinking` + intact `signature` is also present in healthy transcripts. + +## Bloat / Non-Functional + +None. The directive is small, scoped, and has the right non-functional checklist; the problem is the precision of the core wire-shape matcher, not over-engineering. + +## Size Baseline + +- `docs/directives/proxy-thinking-block-sanitize.md` — 49 LOC — compact directive with a narrow intended scope, but the current matcher is too broad for a default-on shared-path mutator. +- `preload.mjs` — 2881 LOC — existing source-of-truth surface; implementation should stay near the directive's stated ~100-200 LOC and reuse existing message/content walk patterns rather than adding a subsystem. + +## Recommendations + +- Revise the strip predicate so it distinguishes an actually broken replay shape from the normal documented omitted-thinking format. The current "empty thinking + signature" test is not sufficient on Opus 4.7/4.8. +- Either remove `redacted_thinking` from this directive or replace it with a documented, captured failing shape. Do not ask implementation to infer a corruption pattern the spec cannot currently define. +- Answer the open questions as follows once the predicate is fixed: + - Drop the assistant message if stripping leaves `content[]` empty; do not inject placeholder text. + - Ship opt-in first, not default-on. A body-mutating thinking-block transform on the shared proxy path needs live validation before it becomes the all-users default. + - The non-empty-signature guard does avoid directly fighting 2.1.152-style signature stripping, but that is not enough by itself because the remaining matched shape is currently also the normal documented omitted-thinking format. + +## Bottom Line + +Request changes for directive stage. The NFR section and load-bearing gate are correct, and the basic mitigation direction is plausible, but the core strip rule currently targets a shape that Anthropic documents as normal omitted thinking on the very model family this PR targets. On top of that, the `redacted_thinking` half of the matcher does not match the documented schema. Tighten the predicate to a genuinely broken replay shape, resolve the empty-message behavior in the spec, and keep first release opt-in; then this is ready for rereview. diff --git a/docs/code-reviews/pr162-thinking-sanitize-directive-rereview-openq1-resolved-2026-05-29.md b/docs/code-reviews/pr162-thinking-sanitize-directive-rereview-openq1-resolved-2026-05-29.md new file mode 100644 index 00000000..9ab85ee0 --- /dev/null +++ b/docs/code-reviews/pr162-thinking-sanitize-directive-rereview-openq1-resolved-2026-05-29.md @@ -0,0 +1,37 @@ +# Review: proxy-thinking-block-sanitize directive + +Date: 2026-05-29 +Reviewed: PR #162 directive (`docs/directives/proxy-thinking-block-sanitize.md`) +Label applied: reviewed-by-codex-agent + +## What Is Correct +- The revised directive is internally coherent again after the Open Question 1 resolution. Goal, Behavior #1, Behavior #3, Out of scope, and the resolved Open Question now all describe the same v1 rule: drop omitted `thinking` blocks from prior assistant turns and also from the latest assistant turn unless that latest turn is still an active tool-continuation ([directive lines 6-16, 28-34, 42-47, 49-58](../directives/proxy-thinking-block-sanitize.md)). +- The turn-selection rule is now defensible against both the PR’s empirical evidence and Anthropic’s current docs. The PR thread’s 24 captured `400 ... cannot be modified` errors show the API naming the latest assistant message every time, so the old prior-turn-only rule would not have covered the documented failure. Anthropic’s current extended-thinking docs also still distinguish between prior assistant turns, whose thinking can be omitted, and active tool-use continuations, where the complete unmodified thinking block must be round-tripped. That makes the new exclusion boundary the right one for v1: latest completed turn is droppable; latest active tool-continuation is not. +- The `redacted_thinking` deferral still holds technically and empirically. The directive keeps it fully out of the active v1 predicate, explains the correct opaque `{ "type":"redacted_thinking", "data":"..." }` schema, and the PR evidence says the motivating worst-case transcript contained zero such blocks ([directive lines 28-29, 47, 51](../directives/proxy-thinking-block-sanitize.md)). +- The NFR section remains sound. `Load-bearing? yes` is the correct classification for a shared request-path body mutator, the Chris-review requirement is explicit, determinism/cache-stability are called out, and the size/maintainability budget still points implementation toward one small extension reusing the existing pipeline/body-walk patterns rather than a new subsystem ([directive lines 18-24](../directives/proxy-thinking-block-sanitize.md), [proxy/pipeline.mjs](../../proxy/pipeline.mjs), [preload.mjs](../../preload.mjs)). +- The opt-in posture is still the right release boundary. The directive now correctly treats the live A/B as the gate for future default-on reconsideration, not as a blocker to an opt-in v1 that is already narrowed to the completed-turn-resume class ([directive lines 34-36, 51, 53, 57-58](../directives/proxy-thinking-block-sanitize.md)). + +## Blockers +None + +## What Needs Attention +- Chris human review remains required before implementation/merge because this is still a load-bearing request-body mutator. That is a process gate, not a directive flaw ([directive line 24](../directives/proxy-thinking-block-sanitize.md), [CLAUDE.md](../../CLAUDE.md)). +- The remaining live A/B belongs to rollout posture, not directive correctness: keep v1 opt-in until a captured completed-turn repro proves the transform clears the 400 without surfacing a different rejection ([directive lines 36, 51, 58](../directives/proxy-thinking-block-sanitize.md)). +- Open Question 2 is still worth covering in implementation tests even though it is not blocking the directive anymore: if stripping leaves an assistant message empty, the chosen behavior is to drop the message, so that path should be exercised in live integration as well as unit tests ([directive lines 35, 52, 57-58](../directives/proxy-thinking-block-sanitize.md)). + +## Bloat / Non-Functional +None + +## Size Baseline +- `docs/directives/proxy-thinking-block-sanitize.md` — 58 LOC — compact directive; the behavioral change is substantive but still contained. +- `proxy/pipeline.mjs` — 120 LOC — existing extension execution surface; no new pipeline abstraction is warranted. +- `preload.mjs` — 2881 LOC — large incumbent helper surface; the directive still correctly biases toward reusing existing body-walk patterns instead of inventing new machinery. + +## Recommendations +- Approve the directive for implementation at the current scope. +- Keep `redacted_thinking` out of v1 unless a real repro demonstrates that it participates in this failure mode and justifies a separate schema-accurate rule. +- Treat the latest-turn exclusion exactly as written: only active tool-continuation turns are protected; latest completed assistant turns belong in the drop set. +- Keep the opt-in/default-off posture until the live proxy A/B is complete, even though the directive itself is now ready. + +## Bottom Line +Approve. The revised directive fixes the one load-bearing ambiguity that mattered: it no longer claims a prior-turn-only transform can solve an error the API consistently attributes to the latest assistant message, and it draws the correct no-touch boundary around active tool continuations. The remaining gates are operational, not architectural: Chris still needs to sign off on this load-bearing mutator, and default-on still waits for live validation. diff --git a/docs/code-reviews/pr162-thinking-sanitize-implementation-codex-rereview-2026-05-29.md b/docs/code-reviews/pr162-thinking-sanitize-implementation-codex-rereview-2026-05-29.md new file mode 100644 index 00000000..58aaa963 --- /dev/null +++ b/docs/code-reviews/pr162-thinking-sanitize-implementation-codex-rereview-2026-05-29.md @@ -0,0 +1,41 @@ +# Review: thinking-block-sanitize implementation + +Date: 2026-05-29 +Reviewed: PR #162 @ ac4b110 (`proxy/extensions/thinking-block-sanitize.mjs`, `test/proxy-thinking-block-sanitize.test.mjs`) +Label applied: approved-by-codex-agent + +## What Is Correct + +- The continuation guard now matches the approved rule exactly: it inspects the latest assistant message's terminal block, requires a `tool_use` with an `id`, and protects that turn only when a later `tool_result.tool_use_id` answers that exact call. +- The previous over-broad case is now closed: an unanswered terminal `tool_use`, or a later `tool_result` for a different call, no longer suppresses latest-turn stripping. +- The new tests pin the missing negative cases at both levels that matter: helper-level pairing logic and `planSanitize` behavior when a mismatched later `tool_result` exists. +- Local verification passed: `node --test` → 906 passing, 0 failing. + +## Blockers + +- None. + +## What Needs Attention + +- Chris's human review remains the merge gate because this is still a load-bearing request-body mutator. + +## Bloat / Non-Functional + +- None. The fix is narrowly scoped, behavior-preserving outside the blocked edge case, and adds only the regression coverage the prior review asked for. + +## Size Baseline + +- `proxy/extensions/thinking-block-sanitize.mjs` — 127 LOC — focused request-path transform with a narrow continuation matcher. +- `proxy/extensions/cache-telemetry.mjs` — 262 LOC — unchanged single-writer merge path that still carries the sanitize count. +- `test/proxy-thinking-block-sanitize.test.mjs` — 185 LOC — now covers matched, unmatched, and absent tool-result continuation cases. +- `test/proxy-quota-status-pipeline.test.mjs` — 241 LOC — existing end-to-end merge/order pin remains relevant. + +## Recommendations + +- Approve as implemented. +- Keep the new mismatched-`tool_use_id` regression tests; they pin the exact rule boundary that previously drifted broad. +- Preserve the current default-off posture until the already-noted live validation gate is complete. + +## Bottom Line + +Ship this implementation review as approved. The blocker from the prior head is fixed with the right pairing rule, the missing negative regression is now covered, and the full suite is green at `ac4b110`. diff --git a/docs/code-reviews/pr162-thinking-sanitize-implementation-codex-review-2026-05-29.md b/docs/code-reviews/pr162-thinking-sanitize-implementation-codex-review-2026-05-29.md new file mode 100644 index 00000000..afb538b6 --- /dev/null +++ b/docs/code-reviews/pr162-thinking-sanitize-implementation-codex-review-2026-05-29.md @@ -0,0 +1,46 @@ +# Review: thinking-block-sanitize implementation + +Date: 2026-05-29 +Reviewed: PR #162 @ b6ccd64 (`proxy/extensions/thinking-block-sanitize.mjs`, `proxy/extensions/cache-telemetry.mjs`, tests) +Label applied: changes-requested + +## What Is Correct + +- Opt-in gating is implemented at the request boundary: default off is a true no-op, and opt-in on emits counts only. +- `planSanitize` is pure and deterministic: it preserves message/block order, performs no nondeterministic rewrites, and returns the original `messages` array when nothing changes. +- Empty assistant messages are dropped rather than replaced with synthetic placeholder text, and the drop count is merged through the existing single writer in `cache-telemetry`. +- Ordering is correct for the sibling telemetry flow: sanitize runs before `session-health`, and the pipeline test pins that `thinking_block_count` reflects the forwarded body. +- Local verification passed: `node --test` → 904 passing, 0 failing. + +## Blockers + +- `isActiveToolContinuation` protects the latest assistant turn whenever **any** later message contains **any** `tool_result`, but the approved rule is narrower: protect only when the latest turn's terminal `tool_use` is the one answered by the following `tool_result`. The current implementation never checks `tool_use_id`; it just scans for the presence of a later `tool_result` anywhere. [`proxy/extensions/thinking-block-sanitize.mjs:35`](../../proxy/extensions/thinking-block-sanitize.mjs), [`proxy/extensions/thinking-block-sanitize.mjs:46`](../../proxy/extensions/thinking-block-sanitize.mjs) + + Concrete repro: latest assistant ends with `tool_use id="t1"`, later user message contains `tool_result tool_use_id="other"`. `isActiveToolContinuation(...)` currently returns `true`, so sanitize leaves the latest omitted thinking intact even though the approved rule says that turn is not the protected continuation case. That over-protection can leave the exact latest-turn omitted thinking in place that this mitigation is supposed to strip, so it is a correctness issue, not just a missing edge test. + + The tests cover only "some later tool_result exists" and "no later tool_result exists"; they do not pin the required negative case where a later `tool_result` exists but does **not** answer the terminal `tool_use`. [`test/proxy-thinking-block-sanitize.test.mjs:28`](../../test/proxy-thinking-block-sanitize.test.mjs), [`test/proxy-thinking-block-sanitize.test.mjs:64`](../../test/proxy-thinking-block-sanitize.test.mjs) + +## What Needs Attention + +- None beyond the blocker above. + +## Bloat / Non-Functional + +- None. The implementation stays within the directive's size budget, keeps the transform deterministic, and records counts only. + +## Size Baseline + +- `proxy/extensions/thinking-block-sanitize.mjs` — 121 LOC — focused request-path transform with three small helpers. +- `proxy/extensions/cache-telemetry.mjs` — +3 LOC — additive single-writer merge only. +- `test/proxy-thinking-block-sanitize.test.mjs` — 166 LOC — good happy-path coverage, missing the mismatched-`tool_result` guard. +- `test/proxy-quota-status-pipeline.test.mjs` — +29 LOC — end-to-end merge/order pin. + +## Recommendations + +- Match the latest message's terminal `tool_use.id` against later `tool_result.tool_use_id` blocks instead of treating any later `tool_result` as proof of continuation. +- Add a regression test where a later `tool_result` exists but targets a different `tool_use_id`; expected result is `false`, and the latest completed/non-matching turn should be stripped. +- Re-run `node --test` after tightening the matcher and keep the pipeline merge pin in place. + +## Bottom Line + +Revise before approval. The opt-in gating, deterministic rewrite, empty-message handling, telemetry merge, and content-free logging are otherwise solid, but the continuation guard is broader than the approved rule and can suppress the very latest-turn stripping this mitigation is supposed to perform. diff --git a/docs/code-reviews/pr162-thinking-sanitize-implementation-docs-reconfirm-2026-05-29.md b/docs/code-reviews/pr162-thinking-sanitize-implementation-docs-reconfirm-2026-05-29.md new file mode 100644 index 00000000..e93bfdef --- /dev/null +++ b/docs/code-reviews/pr162-thinking-sanitize-implementation-docs-reconfirm-2026-05-29.md @@ -0,0 +1,42 @@ +# Review: thinking-block-sanitize implementation + +Date: 2026-05-29 +Reviewed: PR #162 @ d915953 (docs-correction re-confirm against prior implementation approval at `84dbb0c`) +Label applied: approved-by-codex-agent + +## What Is Correct + +- The executable implementation remains the one already approved at `84dbb0c`. Between `84dbb0c` and `d915953`, `git diff --name-only -- proxy/extensions/thinking-block-sanitize.mjs proxy/extensions/cache-telemetry.mjs test` shows only `proxy/extensions/thinking-block-sanitize.mjs`, and that diff is header-comment-only. +- `proxy/extensions/cache-telemetry.mjs` is unchanged, and no test file changed at all on the re-confirm range. +- The docs correction is substantively right: the directive, README, CHANGELOG, and extension header now consistently state that no env var both preserves thinking and avoids the wedge; `CLAUDE_CODE_DISABLE_THINKING=1` / `MAX_THINKING_TOKENS=0` are lossy thinking-disable levers, and `DISABLE_INTERLEAVED_THINKING=1` is correctly described as not stopping the `400`. +- The public anchor swap in `d915953` is also right: the env/trigger discussion now cites the public `anthropics/claude-code#63147` comment instead of restating private binary-analysis details. +- Local verification passed on `d915953`: `node --test` → 906 passing, 0 failing. + +## Blockers + +- None. + +## What Needs Attention + +- Chris's human review still remains the merge gate because this is a load-bearing request-body mutator. + +## Bloat / Non-Functional + +- None. The post-approval changes are limited to correcting operator guidance and source attribution without widening scope or touching logic/tests. + +## Size Baseline + +- `proxy/extensions/thinking-block-sanitize.mjs` — 130 LOC — implementation unchanged; only the top-of-file behavior note was corrected. +- `proxy/extensions/cache-telemetry.mjs` — 262 LOC — unchanged single-writer telemetry merge path. +- `test/proxy-thinking-block-sanitize.test.mjs` — 185 LOC — unchanged from the approved implementation head. +- `test/proxy-quota-status-pipeline.test.mjs` — 241 LOC — unchanged end-to-end merge coverage for `thinking_blocks_dropped`. + +## Recommendations + +- Re-approve the implementation at `d915953` as a docs-only correction on top of the already-approved executable code. +- Keep the corrected value-prop framing: the proxy is the only non-lossy mitigation for the history-replay paths it covers. +- Leave `implementation-stage` in place and keep Chris's human review as the final merge gate. + +## Bottom Line + +Approve the current head. The executable code and tests are unchanged from the previously approved implementation, the env-lever guidance is now correct and no longer prescribes `DISABLE_INTERLEAVED_THINKING=1` as a fix, and the full suite is still green at 906/906 on `d915953`. diff --git a/docs/directives/proxy-thinking-block-sanitize.md b/docs/directives/proxy-thinking-block-sanitize.md new file mode 100644 index 00000000..19fa0253 --- /dev/null +++ b/docs/directives/proxy-thinking-block-sanitize.md @@ -0,0 +1,60 @@ +# Directive: proxy-thinking-block-sanitize (drop omitted thinking on replay) + +**Status:** DRAFT — AI Team Lead, 2026-05-28 (reframed same day per Codex + community convergence on the root cause). Directive-stage; pending Codex review + Proxy Builder implementation. **Open Question 1 + Behavior #3 resolved by Proxy Builder 2026-05-29** via empirical coverage capture against the worst-case wedged transcript (24 ground-truth `400` captures) — the latest-turn handling is now specified, not open; full findings in the PR #162 Open-Q1 comment. +**References:** anthropics/claude-code#63147 (canonical upstream bug, our #63172 consolidated into it), cache-fix #157 (defensive thinking-block guards — this realizes it), cache-fix #158 (session-health warning — complementary). + +## Goal + +On the request path, drop the **omitted** extended-thinking blocks (which CC persists in the shape `{ "type":"thinking", "thinking":"", "signature":"" }`) before the request leaves the machine — from **all prior assistant turns, and from the latest assistant turn unless it is an active tool-continuation** (Behavior #3, resolved from the empirical coverage capture). Omitted thinking carries no reasoning content (only a signature), so dropping it is safe and API-permitted for those turns. This heads off the permanent-session-death `400 messages..content.: thinking ... blocks cannot be modified` on the history-replay trigger paths, for the affected CC line (2.1.145–2.1.154+, incl. Opus 4.8), while Anthropic fixes the root cause upstream. + +**Why this matters (value prop).** No env var both preserves thinking and avoids the wedge (env-var gate logic + reload-trigger taxonomy: [anthropics/claude-code#63147](https://github.com/anthropics/claude-code/issues/63147#issuecomment-4574730571)): `CLAUDE_CODE_DISABLE_THINKING=1` / `MAX_THINKING_TOKENS=0` stop it only by disabling thinking *entirely* (lossy — no reasoning at all), and `DISABLE_INTERLEAVED_THINKING=1` keeps thinking but is confirmed NOT to stop the 400. So this proxy mitigation is the **only path that keeps thinking AND avoids the wedge** for the history-replay paths it covers — that is why it ships rather than just telling people to turn thinking off. + +## Why + +**The omitted shape is normal, not corruption.** CC persists *every* prior thinking block with the `thinking` text emptied to `""` and the `signature` retained — verified 6866/6866 on our wedged transcript, and confirmed by Codex and by community analysis on #63147 (healthy, working sessions show the identical shape). So "empty-text + signature" does not distinguish a broken session; it is simply how prior thinking is stored. + +**What actually 400s** is the API's rule that thinking blocks in the **latest assistant message** must not be modified. When CC rebuilds a request from the transcript (resume, `--continue`, auto-compaction on away/wake, mid-turn background-completion injection, parallel-tool-cancel) and replays interleaved-thinking-with-tools turns, the latest assistant turn's omitted thinking is rejected, and because the transcript is fixed, every retry re-sends it → permanent wedge. Anthropic's 2.1.152 signature-stripping safety-net does not cover these paths, and no staff have engaged on #63147 as of this writing. + +**Why dropping prior-turn thinking helps:** cache-fix proxies every request and already rewrites bodies. Removing the optional prior-turn thinking history is the API-permitted fix the issue itself lists ("drop thinking blocks entirely from reconstructed prior turns"). An omitted thinking block carries no reasoning content — only a signature — so dropping it loses nothing semantic. (See Open Questions on coverage vs. the latest-turn case.) + +## Non-Functional Requirements + +- **Size/complexity budget:** small — one focused request-transform extension plus tests (~100–200 LOC). A bounded `messages[].content[]` walk, not a new subsystem. Flag at review if it grows materially past that. +- **Threat model:** operates on request bodies that contain conversation content. MUST NOT log, persist, or emit thinking text or signature values (telemetry is counts only). MUST NOT remove content other than prior-turn omitted (`thinking:""`) `thinking` blocks. No new inbound surface. +- **Maintainability constraints:** reuse the existing extension pipeline and any existing message/content-walk helper in `preload.mjs`; do not introduce a new abstraction for a single transform. No dead code; no back-compat shims. +- **Performance/reliability:** O(content-blocks) per request, cheap. The transform MUST be deterministic and stable — identical input → identical output — so it does not itself churn the prompt-cache prefix across turns (a non-deterministic transform would defeat cache-fix's own purpose). +- **Load-bearing? yes** — modifies request bodies in a shared proxy on the request path; correctness-, security-, and cache-relevant. Requires human (Chris) review before merge, not just Lead + Codex. + +## Behavior + +1. In `onRequest`, walk `body.messages`. Remove `thinking` blocks whose text is empty/whitespace-only (the omitted shape) from **every prior assistant message, and from the latest assistant message unless it is an active tool-continuation** (see #3). Dropping omitted thinking is safe — it is optional history (prior turns) or a completed-turn block the API treats as optional once that turn is closed. (`redacted_thinking` is **out of scope for v1** — see Out of scope.) +2. **Do not touch non-empty thinking blocks** — a block with real thinking text + signature is intact, valid, and load-bearing; leave it exactly as-is. (In practice CC stores prior thinking empty, but guard against it anyway.) +3. **Latest-assistant-message handling — RESOLVED (Open Question 1, Proxy Builder, 2026-05-29).** Ground truth from the worst-case wedged transcript — 24 real `400 … cannot be modified` captures — shows the API **always** names the *latest* assistant message (`messages.137/.149/.157`, `content.5/.8/.14/.16/.23`), never a prior-turn index. So a prior-turn-only drop clears **nothing** for the documented failure; the earlier conservative default ("never modify the latest message") would not fix the bug. The transform therefore **must** also drop the latest assistant message's omitted thinking — with one exception: + - **Active tool-continuation** (the latest assistant message's last block is a `tool_use` paired with a following `tool_result`): the signature binds the reasoning to the pending tool call, the proxy cannot restore the emptied text, and stripping it would break the thinking↔tool_use binding. **Do not touch it.** This case is uncoverable by the proxy, and no env var both preserves thinking and avoids the wedge for it — the only env levers that stop it (`CLAUDE_CODE_DISABLE_THINKING=1` / `MAX_THINKING_TOKENS=0`) disable thinking entirely; otherwise heal/retire the session (see Out of scope). + - **Otherwise** (latest turn is completed — e.g. ends in `text`, or is the array tail with no pending tool chain): **drop its omitted thinking.** This is the common resume-after-a-finished-response class. + + The rule is deterministic and cache-prefix-stable (it depends only on message position + block types). **Honest scope note:** in the motivating extreme session the dominant boundary was the *continuation* case (uncoverable here) — so #162 is a real *partial* mitigation of the completed-turn-resume class, not a save-everything fix; the WARN half (#160, shipped) and the offline HEAL half (RCB#5) remain necessary. +4. If removing blocks would leave an assistant message with empty `content[]`, drop that message (prior-turn thinking-only messages are optional history). The proxy operates on the wire request, not the on-disk transcript, so there is no `parentUuid` to relink. +5. **Opt-in for v1** via `CACHE_FIX_THINKING_SANITIZE=on` (default **off**). Per Chris's release-safety call (2026-05-28): the transform mutates request bodies for every session and its coverage is not yet live-validated (Open Question 1), so v1 ships opt-in — we do not ship a body-mutating, not-yet-validated transform default-on. Revisit default-on once a captured wedged request confirms the predicate clears a real 400 without touching healthy sessions. + +## Telemetry + +Emit a per-request count of blocks dropped (counts only — never content). A non-zero count is a signal the session is in the danger zone; expose it to the per-session state so it can feed the #158 session-health warning. **#158 warns** before a session grows large enough to trip the bug; **this directive mitigates** the request when it would. + +## Out of scope + +- **The latest interleaved-thinking continuation.** When the latest assistant turn ended mid-tool-use and tool_results follow, the API requires that turn's thinking intact — the proxy cannot supply it (the text is gone) and must not strip it. **There is no env var that both preserves thinking and avoids the wedge for this case** (env-var gate logic: [anthropics/claude-code#63147](https://github.com/anthropics/claude-code/issues/63147#issuecomment-4574730571)): `CLAUDE_CODE_DISABLE_THINKING=1` / `MAX_THINKING_TOKENS=0` stop the wedge only by disabling thinking *entirely* (a lossy last resort — no reasoning at all); `DISABLE_INTERLEAVED_THINKING=1` and `CLAUDE_CODE_DISABLE_ADAPTIVE_THINKING=1` keep thinking but are confirmed NOT to stop the 400. The wedge fires on a **disk reload** that replaces the live in-memory transcript (the trigger taxonomy — `--resume`, remote re-drive, etc. — is on the same #63147 comment), so a never-reloaded session does not hit it. The reliable user-side answer for this uncoverable case is **don't-resume + heal/retire** the session (restore-claude-history-linux, RCB#5). It is the part the proxy cannot cover. +- **Repairing the on-disk `.jsonl` transcript.** The proxy acts on requests, not disk. Transcript repair is a recovery-tool concern (restore-claude-history-linux `heal-thinking-wedge`, RCB#5), tracked separately. +- **Persisting the real thinking text.** That is CC's job; the upstream fix lives in #63147. +- **`redacted_thinking` blocks (deferred from v1).** `redacted_thinking` is a distinct opaque `{ "type":"redacted_thinking", "data":"..." }` block — it carries no emptied text field, so it does not exhibit the empty-text-vs-signature mismatch that drives this 400 and is therefore unlikely to be part of the failure mode. Rather than special-case a schema-aware rule for a rare block with no evidence it wedges, v1 scopes to `thinking` only. Revisit if a captured repro ever shows prior-turn `redacted_thinking` contributing to the rejection (Codex re-review, 2026-05-28). + +## Open questions (for Codex / Proxy Builder) + +1. **RESOLVED (Proxy Builder, 2026-05-29) — coverage capture.** Ground truth from the worst-case wedged transcript (24 real `400` captures + a full thinking-block scan): (a) the 400 **always** names the *latest* assistant message — so prior-turn-only is insufficient; (b) the omitted shape is universal (6866/6866 `thinking:""` + intact signature — confirms the reframe); (c) **zero** `redacted_thinking` blocks present (empirically validates the v1 deferral); (d) the failing arrays were short (~140–158 msgs) vs. the natural 613-msg chain → resume/compaction rebuilds, the documented trigger. The "which turns" rule is now folded into Behavior #1/#3: drop omitted thinking from all prior assistant messages **and** the latest, *unless* the latest is an active tool-continuation. **Remaining empirical step (gates default-on, NOT opt-in v1):** a live A/B confirming the drop actually clears the 400 rather than shifting the error elsewhere — the Proxy Test Agent integration step in Testing. Full findings: PR #162 Open-Q1 coverage comment. +2. Drop-the-message vs. placeholder-text-block when an assistant message becomes empty-content — which does the API accept more cleanly in the messages array? +3. **RESOLVED (Chris, 2026-05-28): opt-in for v1** (`CACHE_FIX_THINKING_SANITIZE=on`, default off). Revisit default-on only after live coverage validation (Open Question 1). Rationale: don't ship a body-mutating, not-yet-validated transform default-on — no train wreck. + +## Testing + +- **Unit:** prior-turn omitted thinking → dropped; non-empty thinking → kept; latest-message handling per the resolved Open Question 1 rule; message that becomes empty-content → handled; determinism (same input twice → identical output). +- **Integration (Proxy Test Agent, live CC traffic):** replay a captured wedged extended-thinking request; confirm the transform makes the history-replay request succeed where it previously 400'd; confirm a healthy session is unchanged except for the targeted drops; confirm prompt-cache prefix stability across consecutive turns. diff --git a/proxy/extensions/cache-telemetry.mjs b/proxy/extensions/cache-telemetry.mjs index ad101388..03eefc01 100644 --- a/proxy/extensions/cache-telemetry.mjs +++ b/proxy/extensions/cache-telemetry.mjs @@ -233,6 +233,9 @@ export default { // 590, stashes these before this writer runs). Optional — absent if // that extension is disabled or produced nothing this request. ...(ctx.meta._sessionHealth || {}), + // Additive thinking-block-sanitize drop count (order 550, opt-in). + // Optional — absent unless CACHE_FIX_THINKING_SANITIZE=on. + ...(ctx.meta._thinkingSanitize || {}), timestamp, session_id: rawSid, }, diff --git a/proxy/extensions/thinking-block-sanitize.mjs b/proxy/extensions/thinking-block-sanitize.mjs new file mode 100644 index 00000000..4741ebfb --- /dev/null +++ b/proxy/extensions/thinking-block-sanitize.mjs @@ -0,0 +1,130 @@ +// thinking-block-sanitize — request-path mitigation for the CC thinking-desync +// wedge (anthropics/claude-code#63147). On replay paths (resume / --continue / +// auto-compaction / parallel-tool-cancel), CC re-sends prior assistant turns' +// thinking in the OMITTED shape `{ type:"thinking", thinking:"", signature }`. +// The API rejects modified thinking in the *latest* assistant message with a +// permanent 400, which wedges the session. This extension drops the omitted +// thinking blocks the API treats as optional, before the request is forwarded. +// +// Resolved turn-selection rule (directive Open Question 1, empirical capture): +// - drop omitted thinking from ALL prior assistant turns, AND +// - from the LATEST assistant turn UNLESS it is an active tool-continuation +// (last block is a tool_use with a following tool_result) — that case is +// uncoverable by the proxy (the API needs the signed thinking for the +// pending tool call; we can't restore the emptied text). No env var both +// preserves thinking and avoids the wedge there — CLAUDE_CODE_DISABLE_THINKING=1 +// / MAX_THINKING_TOKENS=0 stop it only by disabling thinking entirely +// (lossy); DISABLE_INTERLEAVED_THINKING=1 does NOT stop the 400 — so the +// answer for that case is don't-resume + heal/retire. +// Never touches non-empty thinking, and never touches redacted_thinking (v1). +// +// OPT-IN for v1: only runs when CACHE_FIX_THINKING_SANITIZE=on (default off) — +// it mutates request bodies and its coverage is not yet live-validated. +// +// Order 550: after the request-body mutators (ttl-management 500) and before +// session-health (590), so #160's thinking_block_count reflects the forwarded +// body. The per-request drop count is exposed via ctx.meta._thinkingSanitize +// for cache-telemetry (600) to merge into the per-session JSON. + +export function isOmittedThinking(block) { + return ( + !!block && + block.type === "thinking" && + typeof block.thinking === "string" && + block.thinking.trim() === "" + ); +} + +function answersToolUse(msg, toolUseId) { + return ( + !!msg && + Array.isArray(msg.content) && + msg.content.some( + (b) => b && b.type === "tool_result" && b.tool_use_id === toolUseId, + ) + ); +} + +// The latest assistant message is an active tool-continuation when its terminal +// block is a `tool_use` that is *paired with* — i.e. answered by — a following +// `tool_result` carrying the same `tool_use_id`. Only then does the API require +// that turn's thinking intact, so only then must we leave it untouched. Matching +// the id (not merely the presence of any later tool_result) keeps the guard as +// narrow as the approved rule: an unanswered terminal tool_use, or a later +// tool_result that answers a *different* call, is not the protected case. +export function isActiveToolContinuation(messages, idx) { + const msg = messages[idx]; + if (!msg || !Array.isArray(msg.content) || msg.content.length === 0) return false; + const last = msg.content[msg.content.length - 1]; + if (!last || last.type !== "tool_use" || !last.id) return false; + for (let j = idx + 1; j < messages.length; j++) { + if (answersToolUse(messages[j], last.id)) return true; + } + return false; +} + +function latestAssistantIndex(messages) { + for (let i = messages.length - 1; i >= 0; i--) { + if (messages[i] && messages[i].role === "assistant") return i; + } + return -1; +} + +// Pure planner: returns { messages, dropped }. Does not mutate the input. +// `messages` is the new array (a message that loses all content is dropped). +export function planSanitize(messages) { + if (!Array.isArray(messages)) return { messages, dropped: 0 }; + const latestAsst = latestAssistantIndex(messages); + const protectLatest = latestAsst >= 0 && isActiveToolContinuation(messages, latestAsst); + + let dropped = 0; + let changed = false; + const out = []; + for (let i = 0; i < messages.length; i++) { + const msg = messages[i]; + if (!msg || msg.role !== "assistant" || !Array.isArray(msg.content)) { + out.push(msg); + continue; + } + if (i === latestAsst && protectLatest) { + out.push(msg); // active continuation — leave its thinking intact + continue; + } + const kept = msg.content.filter((b) => { + if (isOmittedThinking(b)) { + dropped++; + return false; + } + return true; + }); + if (kept.length === msg.content.length) { + out.push(msg); // unchanged + } else if (kept.length === 0) { + changed = true; // message became empty → drop it entirely + } else { + out.push({ ...msg, content: kept }); + changed = true; + } + } + return { messages: changed ? out : messages, dropped }; +} + +export default { + name: "thinking-block-sanitize", + description: + "Drop omitted (empty-text) thinking blocks from prior assistant turns and the latest non-continuation turn, to head off the CC thinking-desync 400 (#63147). Opt-in via CACHE_FIX_THINKING_SANITIZE=on.", + order: 550, + + async onRequest(ctx) { + if (process.env.CACHE_FIX_THINKING_SANITIZE !== "on") return; + const body = ctx.body; + if (!body || !Array.isArray(body.messages)) return; + + const { messages, dropped } = planSanitize(body.messages); + if (dropped > 0) body.messages = messages; + + // Counts only — never content. Exposed for cache-telemetry to persist and + // for the #160 session-health signal. + ctx.meta._thinkingSanitize = { thinking_blocks_dropped: dropped }; + }, +}; diff --git a/test/proxy-quota-status-pipeline.test.mjs b/test/proxy-quota-status-pipeline.test.mjs index cd0ba8c1..a3c6bc4f 100644 --- a/test/proxy-quota-status-pipeline.test.mjs +++ b/test/proxy-quota-status-pipeline.test.mjs @@ -179,6 +179,35 @@ test("[pipeline #160] degraded path: no quota headers → no per-session write, } }); +test("[pipeline #162] thinking-block-sanitize drop count merges into the per-session JSON (opt-in)", async () => { + const env = setupHome(); + const old = process.env.CACHE_FIX_THINKING_SANITIZE; + process.env.CACHE_FIX_THINKING_SANITIZE = "on"; + try { + const exts = await loadExtensions(EXT_DIR, EXT_CONFIG); + const sid = "sess-sanitize-merge"; + const body = { + system: [], + messages: [ + { role: "assistant", content: [{ type: "thinking", thinking: "", signature: "S" }, { type: "text", text: "a1" }] }, + { role: "user", content: [{ type: "text", text: "q" }] }, + { role: "assistant", content: [{ type: "thinking", thinking: "", signature: "S" }, { type: "text", text: "a2" }] }, + ], + }; + await driveFullResponse(exts, { ...QUOTA_HEADERS, "x-claude-code-session-id": sid }, { body }); + + const sessionPath = join(env.home, ".claude", "quota-status", "sessions", `${sid}.json`); + const sess = JSON.parse(readFileSync(sessionPath, "utf8")); + assert.equal(sess.thinking_blocks_dropped, 2, "drop count merged into per-session JSON"); + // session-health (590) runs after sanitize (550), so it counts the post-sanitize forwarded body + assert.equal(sess.thinking_block_count, 0, "session-health counts the post-sanitize forwarded body"); + } finally { + if (old === undefined) delete process.env.CACHE_FIX_THINKING_SANITIZE; + else process.env.CACHE_FIX_THINKING_SANITIZE = old; + env.cleanup(); + } +}); + test("[pipeline #11j] malformed session-id ends up in a hashed file, no path-traversal escape", async () => { const env = setupHome(); try { diff --git a/test/proxy-thinking-block-sanitize.test.mjs b/test/proxy-thinking-block-sanitize.test.mjs new file mode 100644 index 00000000..934ca1f3 --- /dev/null +++ b/test/proxy-thinking-block-sanitize.test.mjs @@ -0,0 +1,185 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import ext, { + isOmittedThinking, + isActiveToolContinuation, + planSanitize, +} from "../proxy/extensions/thinking-block-sanitize.mjs"; + +const omitted = () => ({ type: "thinking", thinking: "", signature: "SIG==" }); +const realThinking = () => ({ type: "thinking", thinking: "real reasoning", signature: "SIG==" }); +const text = (t = "a") => ({ type: "text", text: t }); +const toolUse = (id = "t1") => ({ type: "tool_use", id, name: "x", input: {} }); +const toolResult = (id = "t1") => ({ type: "tool_result", tool_use_id: id, content: "r" }); + +// --- isOmittedThinking --- + +test("isOmittedThinking: empty thinking text → true; non-empty / other types → false", () => { + assert.equal(isOmittedThinking(omitted()), true); + assert.equal(isOmittedThinking({ type: "thinking", thinking: " ", signature: "S" }), true); + assert.equal(isOmittedThinking(realThinking()), false); + assert.equal(isOmittedThinking({ type: "redacted_thinking", data: "X" }), false); + assert.equal(isOmittedThinking(text()), false); + assert.equal(isOmittedThinking(null), false); +}); + +// --- isActiveToolContinuation --- + +test("isActiveToolContinuation: latest ends tool_use with a following tool_result → true", () => { + const messages = [ + { role: "assistant", content: [omitted(), toolUse("t1")] }, + { role: "user", content: [toolResult("t1")] }, + ]; + assert.equal(isActiveToolContinuation(messages, 0), true); +}); + +test("isActiveToolContinuation: latest ends in text (completed turn) → false", () => { + const messages = [ + { role: "assistant", content: [omitted(), text("done")] }, + { role: "user", content: [text("next")] }, + ]; + assert.equal(isActiveToolContinuation(messages, 0), false); +}); + +test("isActiveToolContinuation: ends tool_use but no following tool_result → false", () => { + const messages = [{ role: "assistant", content: [omitted(), toolUse("t1")] }]; + assert.equal(isActiveToolContinuation(messages, 0), false); +}); + +test("isActiveToolContinuation: later tool_result answers a DIFFERENT tool_use_id → false (must match the terminal call)", () => { + const messages = [ + { role: "assistant", content: [omitted(), toolUse("t1")] }, + { role: "user", content: [toolResult("other")] }, // answers a different call, not t1 + ]; + assert.equal(isActiveToolContinuation(messages, 0), false); +}); + +// --- planSanitize --- + +test("planSanitize: drops omitted thinking from a prior turn AND the latest completed turn", () => { + const messages = [ + { role: "user", content: [text("q1")] }, + { role: "assistant", content: [omitted(), text("a1")] }, // prior + { role: "user", content: [text("q2")] }, + { role: "assistant", content: [omitted(), text("a2")] }, // latest, completed (ends text) + ]; + const r = planSanitize(messages); + assert.equal(r.dropped, 2); + assert.deepEqual(r.messages[1].content, [text("a1")], "prior turn keeps text, loses omitted thinking"); + assert.deepEqual(r.messages[3].content, [text("a2")], "latest completed turn also stripped"); +}); + +test("planSanitize: protects the latest assistant turn when it is an active tool-continuation", () => { + const messages = [ + { role: "user", content: [text("q1")] }, + { role: "assistant", content: [omitted(), text("a1")] }, // prior → stripped + { role: "user", content: [text("q2")] }, + { role: "assistant", content: [omitted(), toolUse("t1")] }, // latest → protected (continuation) + { role: "user", content: [toolResult("t1")] }, + ]; + const r = planSanitize(messages); + assert.equal(r.dropped, 1, "only the prior turn's thinking is dropped"); + assert.deepEqual(r.messages[1].content, [text("a1")]); + assert.deepEqual(r.messages[3].content, [omitted(), toolUse("t1")], "continuation turn left byte-identical"); +}); + +test("planSanitize: latest turn whose terminal tool_use is NOT answered (mismatched tool_result) is stripped, not protected", () => { + const messages = [ + { role: "user", content: [text("q")] }, + { role: "assistant", content: [omitted(), toolUse("t1")] }, // latest assistant, terminal tool_use t1 + { role: "user", content: [toolResult("other")] }, // answers a different call → NOT the protected continuation + ]; + const r = planSanitize(messages); + assert.equal(r.dropped, 1, "latest-turn omitted thinking is stripped when its tool_use is unanswered"); + assert.deepEqual(r.messages[1].content, [toolUse("t1")], "thinking removed; tool_use kept"); +}); + +test("planSanitize: keeps non-empty thinking and redacted_thinking (v1 scope = thinking-empty only)", () => { + const messages = [ + { role: "assistant", content: [realThinking(), text("a1")] }, + { role: "user", content: [text("q")] }, + { role: "assistant", content: [{ type: "redacted_thinking", data: "X" }, text("a2")] }, + ]; + const r = planSanitize(messages); + assert.equal(r.dropped, 0, "neither non-empty thinking nor redacted_thinking is dropped"); + assert.equal(r.messages, messages, "unchanged → same array reference"); +}); + +test("planSanitize: drops an assistant message that becomes empty-content", () => { + const messages = [ + { role: "user", content: [text("q1")] }, + { role: "assistant", content: [omitted()] }, // thinking-only prior turn → message dropped + { role: "user", content: [text("q2")] }, + { role: "assistant", content: [text("a2")] }, // latest, no thinking + ]; + const r = planSanitize(messages); + assert.equal(r.dropped, 1); + assert.equal(r.messages.length, 3, "the now-empty assistant message is removed"); + assert.equal(r.messages.some((m) => m.role === "assistant" && m.content.length === 0), false); +}); + +test("planSanitize: deterministic — same input twice yields identical output", () => { + const mk = () => [ + { role: "assistant", content: [omitted(), text("a1")] }, + { role: "user", content: [text("q")] }, + { role: "assistant", content: [omitted(), text("a2")] }, + ]; + assert.deepEqual(planSanitize(mk()), planSanitize(mk())); +}); + +// --- onRequest (opt-in gating) --- + +function withSanitize(value, fn) { + const old = process.env.CACHE_FIX_THINKING_SANITIZE; + if (value === undefined) delete process.env.CACHE_FIX_THINKING_SANITIZE; + else process.env.CACHE_FIX_THINKING_SANITIZE = value; + try { + return fn(); + } finally { + if (old === undefined) delete process.env.CACHE_FIX_THINKING_SANITIZE; + else process.env.CACHE_FIX_THINKING_SANITIZE = old; + } +} + +test("onRequest: default (opt-in off) is a no-op — body unchanged, no telemetry", async () => { + await withSanitize(undefined, async () => { + const ctx = { + body: { messages: [{ role: "assistant", content: [omitted(), text("a")] }] }, + meta: {}, + }; + await ext.onRequest(ctx); + assert.deepEqual(ctx.body.messages[0].content, [omitted(), text("a")], "body untouched when off"); + assert.equal(ctx.meta._thinkingSanitize, undefined, "no telemetry when off"); + }); +}); + +test("onRequest: opt-in on mutates the body and emits the drop count", async () => { + await withSanitize("on", async () => { + const ctx = { + body: { + messages: [ + { role: "assistant", content: [omitted(), text("a1")] }, + { role: "user", content: [text("q")] }, + { role: "assistant", content: [omitted(), text("a2")] }, + ], + }, + meta: {}, + }; + await ext.onRequest(ctx); + assert.deepEqual(ctx.body.messages[0].content, [text("a1")]); + assert.deepEqual(ctx.body.messages[2].content, [text("a2")]); + assert.deepEqual(ctx.meta._thinkingSanitize, { thinking_blocks_dropped: 2 }); + }); +}); + +test("onRequest: opt-in on with nothing to drop emits a zero count and leaves the body intact", async () => { + await withSanitize("on", async () => { + const ctx = { + body: { messages: [{ role: "assistant", content: [realThinking(), text("a")] }] }, + meta: {}, + }; + await ext.onRequest(ctx); + assert.deepEqual(ctx.meta._thinkingSanitize, { thinking_blocks_dropped: 0 }); + assert.deepEqual(ctx.body.messages[0].content, [realThinking(), text("a")]); + }); +});