diff --git a/CHANGELOG.md b/CHANGELOG.md index 09c5aba1..b4ed7849 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Behavior changes +- **`thinking-block-sanitize` v1 is now on by default (#162, #63147).** Was opt-in via `CACHE_FIX_THINKING_SANITIZE=on` in v3.8.0–v3.9.x. Seven days of prod dogfood (2026-05-29 → 2026-06-05) across 37 sessions: zero `cannot be modified` 400s, cache hit-rate aggregate 94.66% vs. 92.44% baseline (no prefix degradation), sanitize fired on ~35% of sessions with ~800 blocks dropped per day, max 938K context healthy. Set `CACHE_FIX_THINKING_SANITIZE=off` to explicitly disable. v2 (additional tools-hash-mismatch drop) stays opt-in via `=v2` pending its own prod-dogfood window after #196 closes the silent-load failure mode. - **In-process extension hot-reload is now off by default (#196, #198).** Was on in v3.x. Set `CACHE_FIX_HOT_RELOAD=on` in the proxy's runtime environment (or in the install-service environment if using `cache-fix-proxy install-service`) to restore the prior behavior. Off-by-default eliminates the Node ESM stale-import race that silently broke `thinking-block-sanitize v2` for 17 hours after PR #192's merge — the watcher re-imports an extension whose transitive dependencies are already cached by Node's loader, and Node cannot evict cached transitive modules in-process. Cold starts are unaffected. - **A supervisor-level proxy restart is now required after `npm install -g cache-fix-proxy@4`** to pick up extension changes. See [Upgrading from v3.x](README.md#upgrading-from-v3x) for per-platform restart commands. diff --git a/README.md b/README.md index caec5495..55f4282b 100644 --- a/README.md +++ b/README.md @@ -41,7 +41,7 @@ On every `/v1/messages` request, 9 extensions run in order (one opt-in): | `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`) | +| `thinking-block-sanitize` | Drops omitted (empty-text) thinking blocks to head off the CC thinking-desync `400` (#63147). **On by default as of v4.0.0** (v1 mode). Set `CACHE_FIX_THINKING_SANITIZE=off` to disable, `=v2` for additional tools-hash-mismatch drop (opt-in). | Extensions live as `.mjs` files in `proxy/extensions/` with configuration in `proxy/extensions.json`. As of v4.0.0 the proxy loads them once at startup; adding, removing, or modifying an extension requires a supervisor-level proxy restart (see [Upgrading from v3.x](#upgrading-from-v3x)). Hot-reload is available as opt-in via `CACHE_FIX_HOT_RELOAD=on` for users who want the v3.x behavior back; that path is subject to the Node ESM stale-import race documented in [#196](https://github.com/cnighswonger/claude-code-cache-fix/issues/196). @@ -213,9 +213,10 @@ Options (all optional; all fall back to the same env vars used by the CLI): ## Upgrading from v3.x -**Behavior change in v4.0.0:** +**Behavior changes in v4.0.0:** -**In-process extension hot-reload is now off by default.** Was on in v3.x. Set `CACHE_FIX_HOT_RELOAD=on` to restore the prior behavior. Off-by-default eliminates the Node ESM stale-import race documented in [#196](https://github.com/cnighswonger/claude-code-cache-fix/issues/196), where the watcher silently failed to load a newly-merged extension for 17 hours after a hot-reload trigger. The race fires when the file watcher re-imports an extension whose transitive dependencies are already cached by Node's loader; cold starts are unaffected. +- **`thinking-block-sanitize` v1 is now on by default.** Was opt-in via `CACHE_FIX_THINKING_SANITIZE=on` in v3.8.0–v3.9.x. After seven days of prod dogfood across 37 sessions (zero `cannot be modified` 400s, cache hit-rate aggregate 94.66% vs. 92.44% baseline, sanitize firing on ~35% of sessions with ~800 blocks dropped per day) the v1 mitigation is the new default. Set `CACHE_FIX_THINKING_SANITIZE=off` to explicitly disable. v2 (additional tools-hash-mismatch drop) stays opt-in via `=v2`. See [#63147](https://github.com/anthropics/claude-code/issues/63147) and [#162](https://github.com/cnighswonger/claude-code-cache-fix/issues/162). +- **In-process extension hot-reload is now off by default.** Was on in v3.x. Set `CACHE_FIX_HOT_RELOAD=on` to restore the prior behavior. Off-by-default eliminates the Node ESM stale-import race documented in [#196](https://github.com/cnighswonger/claude-code-cache-fix/issues/196), where the watcher silently failed to load a newly-merged extension for 17 hours after a hot-reload trigger. The race fires when the file watcher re-imports an extension whose transitive dependencies are already cached by Node's loader; cold starts are unaffected. Picking up a new extension or a code change to an existing one in v4.0.0 requires a supervisor-level proxy restart. There are two upgrade flows depending on whether you also want to opt back into hot-reload. @@ -823,17 +824,17 @@ 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) +## Thinking-block sanitize (proxy mode, on by default, 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. +**On by default as of v4.0.0.** v1 was opt-in via `CACHE_FIX_THINKING_SANITIZE=on` in v3.8.0–v3.9.x. After seven days of prod dogfood across 37 sessions (zero `cannot be modified` 400s, cache hit-rate aggregate 94.66% vs. 92.44% baseline, sanitize firing on ~35% of sessions with ~800 blocks dropped per day, max 938K context healthy) the v1 mitigation is the new default. 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. v2 stays opt-in pending its own prod-dogfood window after [#196](https://github.com/cnighswonger/claude-code-cache-fix/issues/196) closes the silent-load failure mode that prevented v2 from running in prior testing. | 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). | +| `CACHE_FIX_THINKING_SANITIZE` | unset (= v1) | v4.0.0+: v1 omitted-block drop is the default. Set to `off` to explicitly disable (returns to v3.x default-off behavior). Set to `v2` to additionally enable the v2 tools-hash-mismatch drop. Set to `on` for v1 (back-compat — same as unset). | ## System prompt rewrite (preload mode, optional) diff --git a/docs/code-reviews/pr-201-round-1-codex.md b/docs/code-reviews/pr-201-round-1-codex.md new file mode 100644 index 00000000..069f3fe4 --- /dev/null +++ b/docs/code-reviews/pr-201-round-1-codex.md @@ -0,0 +1,39 @@ +# Review: PR #201 thinking-block-sanitize v1 default-on + +Date: 2026-06-06 +Reviewed: PR #201 at 6f8d988 +Round: 1 +Label applied: reviewed-by-codex-agent, approved-by-codex-agent + +## What Is Correct + +- `proxy/extensions/thinking-block-sanitize.mjs:202` implements the intended gate exactly: the literal `off` disables, `v2` stays its own opt-in path, and unset / `on` / unknown values resolve to v1. Given the new default-on contract, permissive unknown -> on is the safer choice because a typo cannot silently turn the mitigation off. +- The v2 non-flip is clearly intentional rather than forgotten scope. The PR body states both reasons explicitly, and the in-file rationale around `proxy/extensions/thinking-block-sanitize.mjs:58` and `proxy/extensions/thinking-block-sanitize.mjs:195` makes the same distinction in code context. +- The test updates cover the behavior change in the right places. `test/proxy-thinking-block-sanitize.test.mjs:144` now proves that the default path mutates and emits telemetry, while `test/proxy-thinking-block-sanitize.test.mjs:163` preserves an explicit no-op check for `=off`. +- The `[pipeline #160]` adjustment in `test/proxy-quota-status-pipeline.test.mjs:122` is the right isolation mechanism for a full-pipeline merge test. It keeps session-health + writer behavior under real extension ordering while avoiding accidental sanitize coupling; the separate `[pipeline #162]` coverage continues to exercise the post-sanitize merge path. +- The committed docs are materially aligned with the v4.0.0 upgrade framing from PR #200. `README.md:218`, `README.md:833`, and `CHANGELOG.md:7` all explain the default flip, the explicit disable path, and the continued `=v2` opt-in. +- Verification matched the claims: `node --test test/proxy-thinking-block-sanitize.test.mjs test/proxy-quota-status-pipeline.test.mjs` passed, and `npm test` passed `999/999`. + +## Blockers + +None. + +## What Needs Attention + +- A few repo-local wording leftovers will be inaccurate after merge even though behavior is correct: `README.md:32` still says the request pipeline has "one opt-in" extension, `proxy/server.mjs:295` still cites sanitize as a strict `=== "on"` precedent, `proxy/extensions/cache-telemetry.mjs:241` still describes the sanitize metadata as opt-in-only, and `test/proxy-quota-status-pipeline.test.mjs:191` still labels the merge test "(opt-in)". These are non-blocking, but they are now the main source of future-reader confusion. + +## Bloat / Non-Functional + +- No speculative code or unnecessary surface area showed up in the diff. The change is appropriately narrow: gate semantics, tests, and operator-facing docs. +- The committed diff does not add new operator-local paths or environment leakage. The only operator-local source reference I saw is in the PR narrative, not in repo content. + +## Recommendations + +- Land this as approved, then do a short follow-up wording sweep for the remaining opt-in / `=on`-only references outside the diff. +- For future public PR descriptions, prefer repo-hosted artifacts or issue links over operator-local memory-note paths as evidence sources. + +## Bottom Line + +Ship it. The default-on flip is implemented consistently, the permissive on-path is well-documented and well-tested, v2's non-default status is clearly intentional, and the pipeline test isolation change is the right tradeoff for preserving end-to-end coverage. Only minor wording cleanup remains outside the touched files. + +— Codex review diff --git a/proxy/extensions/thinking-block-sanitize.mjs b/proxy/extensions/thinking-block-sanitize.mjs index a876865a..0157368f 100644 --- a/proxy/extensions/thinking-block-sanitize.mjs +++ b/proxy/extensions/thinking-block-sanitize.mjs @@ -45,12 +45,21 @@ // v2 no-ops entirely. The shared sessions/unknown.json would cross- // contaminate baselines across unrelated agents otherwise. // -// Modes via CACHE_FIX_THINKING_SANITIZE: -// "off" (or unset, or any other value) — extension no-ops (default) -// "on" — v1 only (omitted-text drop) +// Modes via CACHE_FIX_THINKING_SANITIZE (as of v4.0.0 — v1 default-on flip): +// unset (or "on") — v1 only (omitted-text drop). DEFAULT. +// "off" — extension no-ops (explicit disable) // "v2" — v1 + v2 (omitted-text drop AND // tools-hash-mismatch drop). v2 is -// strict superset of "on". +// strict superset of v1. +// any other value — treated as v1 (the default), not off. +// Matches the precedent of being +// permissive about the on-path. +// +// v1 default-on rationale: 7-day prod dogfood across 37 sessions (2026-05-29 +// → 2026-06-05) on `=on`: zero `cannot be modified` 400s, cache hit-rate +// aggregate 94.66% vs 92.44% baseline (no prefix degradation), sanitize fired +// on ~35% of sessions, ~800 blocks dropped per day, max 938K context healthy. +// v2 stays opt-in via `=v2` because the dogfood only ran v1. // // Order 550: after the request-body mutators (ttl-management 500) and before // session-health (590), so #160's thinking_block_count reflects the forwarded @@ -183,11 +192,18 @@ export function planSanitize(messages, { v2StripSigned = false } = {}) { // --- v2 mode + state --- -// "off" | "on" | "v2". Unknown values → "off" (fail-open, no extension activity). +// "off" | "on" | "v2". As of v4.0.0 the default flipped from "off" to "on" — +// v1 (omitted-text drop) is the new default behavior. Set +// CACHE_FIX_THINKING_SANITIZE=off to explicitly disable; =v2 to additionally +// enable the v2 tools-hash-mismatch drop (still opt-in pending its own +// prod-dogfood window after #200 closes the silent-load failure mode). +// Unknown values fall through to "on" — we are permissive about the on-path +// and only treat the literal "off" as a disable. export function modeFromEnv(env = process.env) { const v = env.CACHE_FIX_THINKING_SANITIZE; - if (v === "on" || v === "v2") return v; - return "off"; + if (v === "off") return "off"; + if (v === "v2") return "v2"; + return "on"; } // Per-session state, in memory. Keyed by canonical session filename @@ -218,7 +234,7 @@ export function _resetV2State() { 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). v1 mode: omitted-text drop only. v2 mode: also drop signed thinking + redacted_thinking on cross-request tools-hash mismatch (ToolSearch surface). Opt-in via CACHE_FIX_THINKING_SANITIZE=on (v1) or =v2 (v1+v2).", + "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). v1 mode: omitted-text drop only. v2 mode: also drop signed thinking + redacted_thinking on cross-request tools-hash mismatch (ToolSearch surface). v1 is now ON by default as of v4.0.0; set CACHE_FIX_THINKING_SANITIZE=off to disable, =v2 to additionally opt into v2.", order: 550, async onRequest(ctx) { diff --git a/test/proxy-quota-status-pipeline.test.mjs b/test/proxy-quota-status-pipeline.test.mjs index a3c6bc4f..053e55cc 100644 --- a/test/proxy-quota-status-pipeline.test.mjs +++ b/test/proxy-quota-status-pipeline.test.mjs @@ -120,6 +120,13 @@ test("[pipeline #17] two-session interleaving: per-session files distinct; accou }); test("[pipeline #160] session-health fields are merged into the per-session JSON by the writer", async () => { + // This test exercises session-health field merging, not sanitize behavior. + // v4.0.0 flipped sanitize to default-on (v1), which strips the empty + // thinking block from the body before session-health counts it. Set =off + // explicitly so the count this test asserts (2) reflects pre-sanitize + // body shape, isolating the assertion to the merge surface under test. + const oldSanit = process.env.CACHE_FIX_THINKING_SANITIZE; + process.env.CACHE_FIX_THINKING_SANITIZE = "off"; const env = setupHome(); try { const exts = await loadExtensions(EXT_DIR, EXT_CONFIG); @@ -151,6 +158,8 @@ test("[pipeline #160] session-health fields are merged into the per-session JSON assert.match(sess.first_seen, /^\d{4}-\d{2}-\d{2}T/); } finally { env.cleanup(); + if (oldSanit === undefined) delete process.env.CACHE_FIX_THINKING_SANITIZE; + else process.env.CACHE_FIX_THINKING_SANITIZE = oldSanit; } }); diff --git a/test/proxy-thinking-block-sanitize.test.mjs b/test/proxy-thinking-block-sanitize.test.mjs index 181fc08a..e8d142a6 100644 --- a/test/proxy-thinking-block-sanitize.test.mjs +++ b/test/proxy-thinking-block-sanitize.test.mjs @@ -127,7 +127,7 @@ test("planSanitize: deterministic — same input twice yields identical output", assert.deepEqual(planSanitize(mk()), planSanitize(mk())); }); -// --- onRequest (opt-in gating) --- +// --- onRequest gating (v4.0.0: v1 default-on) --- function withSanitize(value, fn) { const old = process.env.CACHE_FIX_THINKING_SANITIZE; @@ -141,19 +141,38 @@ function withSanitize(value, fn) { } } -test("onRequest: default (opt-in off) is a no-op — body unchanged, no telemetry", async () => { +test("onRequest (v4.0.0): default (envvar unset) runs v1 — body mutated, telemetry emitted", async () => { await withSanitize(undefined, 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")], "v1 strips omitted on default-on"); + assert.deepEqual(ctx.body.messages[2].content, [text("a2")]); + assert.deepEqual(ctx.meta._thinkingSanitize, { thinking_blocks_dropped: 2 }); + }); +}); + +test("onRequest (v4.0.0): explicit =off is a no-op — body unchanged, no telemetry", async () => { + await withSanitize("off", 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.deepEqual(ctx.body.messages[0].content, [omitted(), text("a")], "body untouched when explicitly 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 () => { +test("onRequest: explicit =on matches the default (back-compat)", async () => { await withSanitize("on", async () => { const ctx = { body: { @@ -208,14 +227,19 @@ const sid = (id) => ({ "x-claude-code-session-id": id }); // --- modeFromEnv --- -test("modeFromEnv: off / on / v2 / unknown", () => { - assert.equal(modeFromEnv({ CACHE_FIX_THINKING_SANITIZE: undefined }), "off"); - assert.equal(modeFromEnv({}), "off"); +// v4.0.0: v1 default-on flip. Unset/unknown → "on" (was "off"). Only the +// literal "off" is an explicit disable; "v2" stays as before. +test("modeFromEnv (v4.0.0 default-on): defaults to on; off is explicit; v2 unchanged", () => { + assert.equal(modeFromEnv({ CACHE_FIX_THINKING_SANITIZE: undefined }), "on"); + assert.equal(modeFromEnv({}), "on"); assert.equal(modeFromEnv({ CACHE_FIX_THINKING_SANITIZE: "off" }), "off"); assert.equal(modeFromEnv({ CACHE_FIX_THINKING_SANITIZE: "on" }), "on"); assert.equal(modeFromEnv({ CACHE_FIX_THINKING_SANITIZE: "v2" }), "v2"); - assert.equal(modeFromEnv({ CACHE_FIX_THINKING_SANITIZE: "v3" }), "off"); - assert.equal(modeFromEnv({ CACHE_FIX_THINKING_SANITIZE: "true" }), "off"); + // Unknown values fall through to "on" (the default), not to "off". + // We treat "off" as the sole explicit disable; everything else is on-path. + assert.equal(modeFromEnv({ CACHE_FIX_THINKING_SANITIZE: "v3" }), "on"); + assert.equal(modeFromEnv({ CACHE_FIX_THINKING_SANITIZE: "true" }), "on"); + assert.equal(modeFromEnv({ CACHE_FIX_THINKING_SANITIZE: "" }), "on"); }); // --- isSignedThinkingForV2 ---