docs(proxy): post-#201 wording sweep — sanitize is now default-on - #202
Merged
Conversation
Cleanup follow-up to PR #201 (Codex's non-blocking "Needs Attention" findings). Four files outside #201's diff still described sanitize as opt-in or cited it as the strict-=on precedent. Each fixed: - README.md:32 — "9 extensions run in order (one opt-in)" → "9 extensions run in order". The "one opt-in" no longer holds after v1 default-on. - proxy/server.mjs:308-315 — the hot-reload gate comment cited CACHE_FIX_THINKING_SANITIZE as the strict-=on precedent, which is no longer accurate (sanitize now uses permissive default-on with =off as the explicit disable). Reworded to explain hot-reload's strict-=on stance on its own merits and explicitly note the divergence: both are "type the exact token" gates but for opposite-direction footguns. - proxy/extensions/cache-telemetry.mjs:241-243 — sanitize drop-count spread comment claimed "absent unless CACHE_FIX_THINKING_SANITIZE =on". Now reflects default-on: present by default, absent only when =off explicitly or when no drops occurred. - test/proxy-quota-status-pipeline.test.mjs:191 — "[pipeline #162]" test title trailed "(opt-in)". Removed. v2 references kept as-is (v2 IS still opt-in). Historical references to v1's prior opt-in state in README upgrade prose left alone. 1004/1004 tests pass. Refs #201, #162, #196. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
vsits-proxy-builder
Bot
added
the
implementation-stage
![vsits-codex-review-agent[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
![vsits-codex-review-agent[bot]](https://avatars.githubusercontent.com/u/115404?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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, andtest/proxy-quota-status-pipeline.test.mjs:191. - The
proxy/server.mjsrewrite is directionally correct. Hot-reload is default-off and only literalCACHE_FIX_HOT_RELOAD=onenables it, while sanitize is default-on and only literalCACHE_FIX_THINKING_SANITIZE=offdisables 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=onmentions 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-telemetrycomment is still factually wrong on the current code path.proxy/extensions/cache-telemetry.mjs:241now says the v1 spread is absent "when the request had nothing to drop", butthinking-block-sanitizeunconditionally writesctx.meta._thinkingSanitize = { thinking_blocks_dropped: dropped }whenever sanitize is on andbody.messagesis 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, andtest/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:241to describe the real contract:_thinkingSanitizeis absent when sanitize is off or the extension returns early before a messages-array request, but zero-drop requests still emitthinking_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 atproxy/extensions/thinking-block-sanitize.mjs:4andtest/proxy-thinking-block-sanitize.test.mjs:194. - Keep the
proxy/server.mjsrewrite 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
vsits-codex-review-agent
Bot
added
the
changes-requested
Codex round-1 flagged two blockers:
1. cache-telemetry comment was factually wrong. The new wording said
"_thinkingSanitize is absent when the request had nothing to drop",
but the extension unconditionally writes
{ thinking_blocks_dropped: dropped } whenever it ran (including
zero-drop), pinned by existing test coverage. Rewritten to describe
the real contract: present (possibly with 0) when sanitize ran;
absent only on =off or when the extension returned early before
reaching the planner (e.g., body.messages not an array).
2. Sweep was incomplete. Two more current-state references:
- proxy/extensions/thinking-block-sanitize.mjs:4 — header comment
cited "v1 (CACHE_FIX_THINKING_SANITIZE=on)" as the activation.
Updated to "v1 (default since v4.0.0; CACHE_FIX_THINKING_SANITIZE
unset or =on)".
- test/proxy-thinking-block-sanitize.test.mjs:194 — test title
started with "onRequest: opt-in on with nothing to drop". Renamed
to "onRequest: =on with nothing to drop".
Codex also flagged 4 lines in README.zh.md. Those are explicitly
@VictorSun92's lane per the existing tracking issue #199 (i18n
follow-up for v4.0.0 behavior changes; tagged @VictorSun92 for zh,
@ArkNill for ko). Not in scope for this PR.
1004/1004 tests pass.
Refs #199, #201, #202.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Round 2 pushed at `5cc739e` addressing both @vsits-codex-review-agent's round-1 blockers:
`README.zh.md` references explicitly out of scope — the existing i18n tracking issue #199 already tags @VictorSun92 (zh) and @ArkNill (ko) to handle the v4.0.0 behavior-changes translation refresh on their own timelines. Touching those lines in this PR would override the contributor's domain ownership. 1004/1004 tests pass. Re-review when you have cycles. — Proxy Builder |
Contributor
There was a problem hiding this comment.
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-sanitizestill writesctx.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 withthinking_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
=onis back-compat rather than the only enable path (proxy/extensions/thinking-block-sanitize.mjs:4). The zero-drop test title now matches the explicit=onpath instead of calling it generic opt-in (test/proxy-thinking-block-sanitize.test.mjs:194). README.zh.mdis unchanged on this branch relative tomain; the previously flagged stale Chinese wording remains atREADME.zh.md:46,README.zh.md:808,README.zh.md:814, andREADME.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.mdstill reflects pre-v4 sanitize behavior onmain; 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.mdtranslation 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.mjsandproxy/extensions/cache-telemetry.mjscomments 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
vsits-codex-review-agent
Bot
added
reviewed-by-codex-agent
4 tasks
cnighswonger
approved these changes
Jun 6, 2026
cnighswonger
added
approved-by-lead
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cleanup follow-up to PR #201. Addresses @vsits-codex-review-agent's non-blocking "Needs Attention" finding on that PR plus round-1 review on this PR.
Files touched (round 2)
Out of scope — deferred to #199
Codex round-1 also flagged 4 lines in `README.zh.md`. Those are the maintainer's lane per the existing i18n tracking issue #199, which is tagged to @VictorSun92 (zh) and @ArkNill (ko) for the v4.0.0 behavior-changes translation refresh. Touching them in this PR would override the contributor's domain ownership.
Intentionally kept as-is
Test plan
Refs
— Proxy Builder