diff --git a/README.md b/README.md index 55f4282b..74449019 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, 9 extensions run in order (one opt-in): +On every `/v1/messages` request, 9 extensions run in order: | Extension | What it fixes | |-----------|--------------| diff --git a/docs/code-reviews/pr-202-round-1-codex.md b/docs/code-reviews/pr-202-round-1-codex.md new file mode 100644 index 00000000..cacc7a8d --- /dev/null +++ b/docs/code-reviews/pr-202-round-1-codex.md @@ -0,0 +1,37 @@ +# Review: PR #202 sanitize default-on wording sweep + +Date: 2026-06-06 +Reviewed: PR #202 implementation at `35c74d009df473cb0d35ba6715211532ce8853b0` (`README.md`, `proxy/server.mjs`, `proxy/extensions/cache-telemetry.mjs`, `test/proxy-quota-status-pipeline.test.mjs`) +Round: 1 +Label applied: `changes-requested` + +## What Is Correct + +- The four targeted touch points from the PR #201 follow-up note were found and updated in the intended places: `README.md:32`, `proxy/server.mjs:308`, `proxy/extensions/cache-telemetry.mjs:241`, and `test/proxy-quota-status-pipeline.test.mjs:191`. +- The `proxy/server.mjs` rewrite is directionally correct. Hot-reload is default-off and only literal `CACHE_FIX_HOT_RELOAD=on` enables it, while sanitize is default-on and only literal `CACHE_FIX_THINKING_SANITIZE=off` disables it (`proxy/server.mjs:308`, `proxy/server.mjs:317`, `proxy/extensions/thinking-block-sanitize.mjs:195`, `proxy/extensions/thinking-block-sanitize.mjs:203`). +- The English README did not overshoot into legitimate historical or v2-only references. The remaining `CACHE_FIX_THINKING_SANITIZE=on` mentions there are either explicit history (`README.md:218`, `README.md:833`) or back-compat / v2 mode documentation (`README.md:44`, `README.md:837`). + +## Blockers + +- The new `cache-telemetry` comment is still factually wrong on the current code path. `proxy/extensions/cache-telemetry.mjs:241` now says the v1 spread is absent "when the request had nothing to drop", but `thinking-block-sanitize` unconditionally writes `ctx.meta._thinkingSanitize = { thinking_blocks_dropped: dropped }` whenever sanitize is on and `body.messages` is an array, including zero-drop requests (`proxy/extensions/thinking-block-sanitize.mjs:240`, `proxy/extensions/thinking-block-sanitize.mjs:245`, `proxy/extensions/thinking-block-sanitize.mjs:287`, `proxy/extensions/thinking-block-sanitize.mjs:295`). The existing test suite pins that behavior at zero-drop count, not absence (`test/proxy-thinking-block-sanitize.test.mjs:194`, `test/proxy-thinking-block-sanitize.test.mjs:201`). This PR therefore introduces a new inaccurate comment in one of the four touched files. +- The sweep is still incomplete. There are active, non-historical sanitize-as-opt-in references outside the diff that are neither v2-only nor prior-state prose: `README.zh.md:46`, `README.zh.md:808`, `README.zh.md:814`, `README.zh.md:818`, `proxy/extensions/thinking-block-sanitize.mjs:4`, and `test/proxy-thinking-block-sanitize.test.mjs:194`. Because the PR body explicitly says only v2 and historical references were intentionally left alone, these misses matter for review scope, not just as follow-up nits. + +## What Needs Attention + +None + +## Bloat / Non-Functional + +None + +## Recommendations + +- Fix `proxy/extensions/cache-telemetry.mjs:241` to describe the real contract: `_thinkingSanitize` is absent when sanitize is off or the extension returns early before a messages-array request, but zero-drop requests still emit `thinking_blocks_dropped: 0`. +- Extend the sweep to the remaining current-state wording leftovers, at minimum the active Chinese README sanitize section (`README.zh.md:46`, `README.zh.md:808`, `README.zh.md:814`, `README.zh.md:818`) and the still-current code/test wording at `proxy/extensions/thinking-block-sanitize.mjs:4` and `test/proxy-thinking-block-sanitize.test.mjs:194`. +- Keep the `proxy/server.mjs` rewrite as-is once the factual comment issue above is corrected; that divergence explanation is the strongest part of the PR. + +## Bottom Line + +Request changes. Three of the four intended cleanup edits landed as expected, and the hot-reload comment rewrite is accurate, but the new `cache-telemetry` wording is itself incorrect and the repo still contains active non-historical sanitize-as-opt-in leftovers the PR body claims were intentionally exhausted. This sweep is close, but it is not complete or fully accurate yet. + +— Codex review diff --git a/docs/code-reviews/pr-202-round-2-codex.md b/docs/code-reviews/pr-202-round-2-codex.md new file mode 100644 index 00000000..671bd84b --- /dev/null +++ b/docs/code-reviews/pr-202-round-2-codex.md @@ -0,0 +1,35 @@ +# Review: PR #202 sanitize default-on wording sweep + +Date: 2026-06-06 +Reviewed: PR #202 implementation at `5cc739ef9a1b531cf42ae7ef6e2b85f1e092d50d` (`README.md`, `proxy/server.mjs`, `proxy/extensions/cache-telemetry.mjs`, `proxy/extensions/thinking-block-sanitize.mjs`, `test/proxy-quota-status-pipeline.test.mjs`, `test/proxy-thinking-block-sanitize.test.mjs`) +Round: 2 +Label applied: `reviewed-by-codex-agent`, `approved-by-codex-agent` + +## What Is Correct + +- The round-1 factual blocker is closed. `thinking-block-sanitize` still writes `ctx.meta._thinkingSanitize = { thinking_blocks_dropped: dropped }` whenever sanitize runs on a messages-array request, including zero-drop cases (`proxy/extensions/thinking-block-sanitize.mjs:245`, `proxy/extensions/thinking-block-sanitize.mjs:295`, `test/proxy-thinking-block-sanitize.test.mjs:194`, `test/proxy-thinking-block-sanitize.test.mjs:201`). The rewritten spread comment now describes that contract accurately: present when sanitize ran, possibly with `thinking_blocks_dropped: 0`, and absent only when the extension is off or returned early (`proxy/extensions/cache-telemetry.mjs:241`). +- The two in-scope wording misses from round 1 are fixed. The header now states that v1 is default-on since v4.0.0 and `=on` is back-compat rather than the only enable path (`proxy/extensions/thinking-block-sanitize.mjs:4`). The zero-drop test title now matches the explicit `=on` path instead of calling it generic opt-in (`test/proxy-thinking-block-sanitize.test.mjs:194`). +- `README.zh.md` is unchanged on this branch relative to `main`; the previously flagged stale Chinese wording remains at `README.zh.md:46`, `README.zh.md:808`, `README.zh.md:814`, and `README.zh.md:818`, but this PR now explicitly defers that translation refresh to issue #199 instead of implying it was exhausted here. For a wording sweep otherwise limited to English/code/test updates, that is an acceptable scope boundary. + +## Blockers + +None + +## What Needs Attention + +- `README.zh.md` still reflects pre-v4 sanitize behavior on `main`; issue #199 remains the load-bearing place to finish that translation refresh. I am not treating that as a blocker on this PR because the branch leaves the file untouched and the PR body states that ownership boundary explicitly. + +## Bloat / Non-Functional + +None + +## Recommendations + +- Land the pending `README.zh.md` translation refresh under issue #199 so the Chinese docs catch up with the v4.0.0 default-on behavior and the related extension-count wording. +- Keep the rewritten `proxy/server.mjs` and `proxy/extensions/cache-telemetry.mjs` comments as the canonical explanation of the opposite-direction env-var gates; they are now aligned with the implementation. + +## Bottom Line + +Approve. The round-1 blockers are closed: the telemetry comment now matches the actual `_thinkingSanitize` contract, the remaining in-scope code/test wording was cleaned up, and the untouched Chinese README is explicitly deferred to the existing i18n-owner workflow instead of being silently missed. No new factual errors were introduced in the reviewed diff. + +— Codex review diff --git a/proxy/extensions/cache-telemetry.mjs b/proxy/extensions/cache-telemetry.mjs index 8e639fc4..f2e1e63b 100644 --- a/proxy/extensions/cache-telemetry.mjs +++ b/proxy/extensions/cache-telemetry.mjs @@ -238,8 +238,11 @@ 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. + // Additive thinking-block-sanitize drop count (order 550). On by + // default since v4.0.0; present (possibly with thinking_blocks_dropped:0) + // whenever sanitize ran. Absent when CACHE_FIX_THINKING_SANITIZE=off + // or when the extension returned early before reaching the planner + // (e.g., body.messages not an array). ...(ctx.meta._thinkingSanitize || {}), // Additive thinking-block-sanitize v2 fields (order 550, opt-in via // CACHE_FIX_THINKING_SANITIZE=v2). Optional — absent unless v2 is diff --git a/proxy/extensions/thinking-block-sanitize.mjs b/proxy/extensions/thinking-block-sanitize.mjs index 0157368f..7a97bdf2 100644 --- a/proxy/extensions/thinking-block-sanitize.mjs +++ b/proxy/extensions/thinking-block-sanitize.mjs @@ -1,7 +1,7 @@ // thinking-block-sanitize — request-path mitigation for the CC thinking-desync // wedge (anthropics/claude-code#63147). // -// v1 (CACHE_FIX_THINKING_SANITIZE=on): On replay paths (resume / --continue / +// v1 (default since v4.0.0; CACHE_FIX_THINKING_SANITIZE unset or =on): 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 diff --git a/proxy/server.mjs b/proxy/server.mjs index 0b1c3964..bd6db890 100644 --- a/proxy/server.mjs +++ b/proxy/server.mjs @@ -308,8 +308,12 @@ export async function startProxy(options = {}) { // Hot-reload is opt-in as of v4.0.0 (#196). The in-process watcher is the // only code path that triggers the Node ESM stale-import race; cold starts // have an empty module cache and load extensions cleanly. Strict `=== "on"` - // matches the existing CACHE_FIX_THINKING_SANITIZE precedent — any other - // value (including "true"/"1"/"yes") is treated as off. + // means any other value (including "true"/"1"/"yes") is treated as off — + // the safe default. Note this is the opposite stance from + // CACHE_FIX_THINKING_SANITIZE (default-on; only literal "off" disables): + // a hot-reload enable is a footgun, so we require the operator to type the + // exact opt-in token; a sanitize disable is also a footgun (loses the + // wedge mitigation), so we require the exact disable token there. const hotReloadOptIn = process.env.CACHE_FIX_HOT_RELOAD === "on"; const watch = options.watch !== false && hotReloadOptIn; diff --git a/test/proxy-quota-status-pipeline.test.mjs b/test/proxy-quota-status-pipeline.test.mjs index 053e55cc..d841d43f 100644 --- a/test/proxy-quota-status-pipeline.test.mjs +++ b/test/proxy-quota-status-pipeline.test.mjs @@ -188,7 +188,7 @@ 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 () => { +test("[pipeline #162] thinking-block-sanitize drop count merges into the per-session JSON", async () => { const env = setupHome(); const old = process.env.CACHE_FIX_THINKING_SANITIZE; process.env.CACHE_FIX_THINKING_SANITIZE = "on"; diff --git a/test/proxy-thinking-block-sanitize.test.mjs b/test/proxy-thinking-block-sanitize.test.mjs index e8d142a6..f98a0fa4 100644 --- a/test/proxy-thinking-block-sanitize.test.mjs +++ b/test/proxy-thinking-block-sanitize.test.mjs @@ -191,7 +191,7 @@ test("onRequest: explicit =on matches the default (back-compat)", async () => { }); }); -test("onRequest: opt-in on with nothing to drop emits a zero count and leaves the body intact", async () => { +test("onRequest: =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")] }] },