From d78111115fbaee9a2c3df90e059570f6f0f0d4eb Mon Sep 17 00:00:00 2001 From: "vsits-proxy-builder[bot]" <206502658+vsits-proxy-builder[bot]@users.noreply.github.com> Date: Thu, 28 May 2026 15:32:22 +0000 Subject: [PATCH 01/13] directive(draft): session-health early-warning for thinking-desync risk Scope-review draft for AI Team Lead. cache-fix surfaces an early warning (per-session context tokens + interleaved thinking-block count) before a session reaches the scale that triggers CC's thinking-signature desync (anthropics/claude-code#63172). Warn-only; not a fix. Four open scope questions for AI Team Lead before the Codex directive loop. Not yet a directive-stage PR. --- .../proxy-session-health-warning.md | 69 +++++++++++++++++++ 1 file changed, 69 insertions(+) create mode 100644 docs/directives/proxy-session-health-warning.md diff --git a/docs/directives/proxy-session-health-warning.md b/docs/directives/proxy-session-health-warning.md new file mode 100644 index 00000000..39ae2cb9 --- /dev/null +++ b/docs/directives/proxy-session-health-warning.md @@ -0,0 +1,69 @@ +# Directive: session-health early-warning (thinking-desync risk) + +**Status:** DRAFT — authored by Proxy Builder 2026-05-28, pending AI Team Lead scope approval. New feature → minor release; per `docs/release-workflow.md` the maintenance-mode gate is at this directive stage. +**Author:** Proxy Builder +**References:** +- `anthropics/claude-code#63172` — upstream root-cause bug (interleaved-thinking signature desync on extreme-scale sessions) +- cache-fix `#157` — defensive thinking-block guards (related but separate) +- `playbook_manual_compact_procedure.md` (shared memory) — the manual retirement procedure this warning feeds into + +## Problem statement + +Long-running Opus 4.7 (`claude-opus-4-7[1m]`) sessions accumulate interleaved thinking blocks and grow their context window until Claude Code's own history management (compaction / context-editing / parallel-tool-cancellation reconstruction) desyncs a thinking-block signature. The result is a hard `400 messages..content.: thinking blocks ... cannot be modified` on essentially every subsequent turn — the session becomes unusable and the only recovery is retiring it (see #63172 for the full mechanism). + +Observed failure scale on the incident that motivated this (2026-05-28): ~382K-token live context, ~6,850 accumulated thinking blocks, ~7 weeks of continuous session age, 99% cache-read right up until the trip. **There was no proactive signal.** The session ran healthy for weeks and then died abruptly. The existing `manual-compact.sh` retirement procedure relies entirely on a human noticing context-% creep — which nobody did until it was too late. + +cache-fix is uniquely positioned to provide the missing early warning: it proxies every request, already reads response `usage` for telemetry, already maintains per-session state files, and already feeds the statusline. It can measure the exact conditions that correlate with the desync risk and warn before the session reaches the danger zone. + +**Scope boundary:** cache-fix CANNOT fix the desync — that's CC-side (#63172). This feature only *warns* so the operator can retire the session deliberately (write SESSION_STATE, `/clear`) instead of being surprised by a dead session. It is an early-warning, not a mitigation of the bug itself. + +## What the proxy can measure (all already in hand) + +- **Live context size** — from response `usage`: `input_tokens + cache_read_input_tokens + cache_creation_input_tokens`. The `cache-telemetry` extension already reads these. +- **Interleaved thinking-block count in the request** — count `thinking` / `redacted_thinking` blocks across `body.messages[*].content[*]` in `onRequest`. The proxy has the full request body. +- **Session age / first-seen** — the per-session quota-status file already exists (cache-fix v3.5.0+); first-seen timestamp gives age. +- **Per-session keying** — session id is already resolved in `onRequest` (the v3.5.4 fix moved session-id resolution to request headers). + +## Proposed design + +### Phase 1 — measure (no thresholds yet) + +Add per-session telemetry so we can calibrate thresholds against real data rather than guessing: + +- Extend the per-session quota-status JSON with: `context_tokens` (latest), `thinking_block_count` (latest request), `thinking_block_max` (session high-water), `first_seen`, `request_count`. +- Emit these on each request via the existing per-session writer. +- No warning behavior yet — this phase exists to gather the distribution of `thinking_block_count` and `context_tokens` at which real sessions start failing, since the incident data only gives session-*total* thinking blocks (~6,850), not the in-context count at the trip. + +### Phase 2 — warn (thresholds calibrated from Phase 1 data) + +- Compute a `thinking_desync_risk` field per session: `"ok" | "warn" | "high"`, derived from `context_tokens` and `thinking_block_count` crossing configurable thresholds. +- Surface in three places: + 1. **Per-session JSON** — `thinking_desync_risk` + the raw counts (for any consumer). + 2. **Statusline** — a segment that appears only at `warn`/`high` (e.g. `⚠ ctx 310K / 220 think-blocks — consider retiring`), consumed by `tools/quota-statusline.sh`. + 3. **One-time stderr log** — when a session first crosses into `high`, so headless/non-statusline surfaces still get the signal once. +- Config (env vars, with defaults anchored to the observed failure scale): + - `CACHE_FIX_THINKING_RISK_WARN_TOKENS` (default ~250000) + - `CACHE_FIX_THINKING_RISK_HIGH_TOKENS` (default ~340000) + - `CACHE_FIX_THINKING_RISK_WARN_BLOCKS` / `_HIGH_BLOCKS` (defaults TBD from Phase 1) + - `CACHE_FIX_THINKING_RISK=off` to disable entirely + +### Risk model (starting point, to refine in Phase 2) + +`high` when EITHER context_tokens ≥ high-tokens OR thinking_block_count ≥ high-blocks; `warn` at the lower thresholds; `ok` otherwise. Token-OR-block (not AND) because either dimension alone can carry the risk, and the cheap conservative bias is to warn early — a false "retire soon" is far cheaper than a dead session. + +## Open questions (for AI Team Lead) + +1. **Phasing** — ship measure + warn together (one minor), or land Phase 1 telemetry first to calibrate, then Phase 2 in a follow-up? Lean: separate, so thresholds are evidence-based. But that's two releases. +2. **Statusline ownership** — `quota-statusline.sh` is community-contributed (@schuay). A new risk segment touches it; coordinate or keep the signal in the per-session JSON only and let the statusline opt in later? +3. **Default thresholds** — anchor on the single observed incident (~382K / ~6,850 total), or hold defaults conservative and let Phase 1 data set them? The in-context block count at failure is the number we actually lack. +4. **Scope of "thinking_block_count"** — count only the latest request's blocks, or track session high-water? (Directive proposes both fields.) + +## Out of scope + +- **Fixing or working around the desync** — that's #63172, CC-side. cache-fix must not attempt to mutate/strip thinking blocks to "repair" a session (that path is exactly what #157 guards against). Warning only. +- **Auto-retiring / auto-clearing a session** — too aggressive; the operator decides. We warn, they act. +- **Cross-host aggregation** — single-host per-session state, consistent with the existing model. + +## Version target + +Minor — **v3.8.0** (new extension + new env vars + new statusline behavior). Per `docs/release-workflow.md`, AI Team Lead approves this directive's scope before implementation begins. If split into measure/warn phases, Phase 1 telemetry could ship as a smaller minor and Phase 2 as the next. From 8fa3a7ac055885848f9f537090a42ebeeb228bf2 Mon Sep 17 00:00:00 2001 From: "vsits-proxy-builder[bot]" <206502658+vsits-proxy-builder[bot]@users.noreply.github.com> Date: Thu, 28 May 2026 16:12:12 +0000 Subject: [PATCH 02/13] directive: finalize session-health early-warning per AI Team Lead scope approval (#158) Scope approved with refinements (#158): - One release (v3.8.0), split by dimension: active token-gated warn now (trip anchored at ~382K), block-count telemetry-only with a calibrated fast-follow. - No statusline change this release; signal via per-session JSON + one-time stderr log. Separate coordination issue for the @schuay statusline opt-in. - Token defaults: high ~340K, warn ~250K. No blind block defaults. - Track both latest thinking_block_count and thinking_block_max. - Warn-only + the three out-of-scope items kept explicit. Ref #158 --- .../proxy-session-health-warning.md | 61 +++++++++++-------- 1 file changed, 34 insertions(+), 27 deletions(-) diff --git a/docs/directives/proxy-session-health-warning.md b/docs/directives/proxy-session-health-warning.md index 39ae2cb9..3f39e9da 100644 --- a/docs/directives/proxy-session-health-warning.md +++ b/docs/directives/proxy-session-health-warning.md @@ -1,7 +1,7 @@ # Directive: session-health early-warning (thinking-desync risk) -**Status:** DRAFT — authored by Proxy Builder 2026-05-28, pending AI Team Lead scope approval. New feature → minor release; per `docs/release-workflow.md` the maintenance-mode gate is at this directive stage. -**Author:** Proxy Builder +**Status:** Scope APPROVED by AI Team Lead 2026-05-28 (issue #158). Ready for directive-stage PR + Codex review. New feature → minor release (v3.8.0); per `docs/release-workflow.md` the maintenance-mode gate is at this directive stage and has been cleared. +**Author:** Proxy Builder (directive), AI Team Lead (scope approval + refinements) **References:** - `anthropics/claude-code#63172` — upstream root-cause bug (interleaved-thinking signature desync on extreme-scale sessions) - cache-fix `#157` — defensive thinking-block guards (related but separate) @@ -24,39 +24,46 @@ cache-fix is uniquely positioned to provide the missing early warning: it proxie - **Session age / first-seen** — the per-session quota-status file already exists (cache-fix v3.5.0+); first-seen timestamp gives age. - **Per-session keying** — session id is already resolved in `onRequest` (the v3.5.4 fix moved session-id resolution to request headers). -## Proposed design +## Design (v3.8.0 — single release, split by dimension) -### Phase 1 — measure (no thresholds yet) +Per AI Team Lead's scope decision: ship one useful release now rather than a warn-nothing telemetry release followed by a warn release. The **token dimension is already anchored** (we directly observed the trip at ~382K live context), so it gates an active warning immediately. The **block dimension is recorded in telemetry but does not yet gate a warning** — we lack the in-context block distribution at failure (the incident only gives session-*total* ~6,850), so its threshold stays evidence-driven and activates in a calibrated fast-follow. -Add per-session telemetry so we can calibrate thresholds against real data rather than guessing: +### Telemetry (all fields, this release) -- Extend the per-session quota-status JSON with: `context_tokens` (latest), `thinking_block_count` (latest request), `thinking_block_max` (session high-water), `first_seen`, `request_count`. -- Emit these on each request via the existing per-session writer. -- No warning behavior yet — this phase exists to gather the distribution of `thinking_block_count` and `context_tokens` at which real sessions start failing, since the incident data only gives session-*total* thinking blocks (~6,850), not the in-context count at the trip. +Extend the per-session quota-status JSON, written on each request via the existing per-session writer: -### Phase 2 — warn (thresholds calibrated from Phase 1 data) +- `context_tokens` — latest request's live context (`input_tokens + cache_read_input_tokens + cache_creation_input_tokens`) +- `thinking_block_count` — count of `thinking`/`redacted_thinking` blocks in the latest request (the live risk driver) +- `thinking_block_max` — session high-water mark of the above (this is exactly the calibration data we're missing for the block threshold; free to record) +- `first_seen`, `request_count` +- `thinking_desync_risk` — `"ok" | "warn" | "high"` (computed; see below) -- Compute a `thinking_desync_risk` field per session: `"ok" | "warn" | "high"`, derived from `context_tokens` and `thinking_block_count` crossing configurable thresholds. -- Surface in three places: - 1. **Per-session JSON** — `thinking_desync_risk` + the raw counts (for any consumer). - 2. **Statusline** — a segment that appears only at `warn`/`high` (e.g. `⚠ ctx 310K / 220 think-blocks — consider retiring`), consumed by `tools/quota-statusline.sh`. - 3. **One-time stderr log** — when a session first crosses into `high`, so headless/non-statusline surfaces still get the signal once. -- Config (env vars, with defaults anchored to the observed failure scale): - - `CACHE_FIX_THINKING_RISK_WARN_TOKENS` (default ~250000) - - `CACHE_FIX_THINKING_RISK_HIGH_TOKENS` (default ~340000) - - `CACHE_FIX_THINKING_RISK_WARN_BLOCKS` / `_HIGH_BLOCKS` (defaults TBD from Phase 1) - - `CACHE_FIX_THINKING_RISK=off` to disable entirely +### Active warning (token-gated, this release) -### Risk model (starting point, to refine in Phase 2) +- Compute `thinking_desync_risk` from `context_tokens` only, in this release: `high` when `context_tokens ≥ high-tokens`, `warn` at `≥ warn-tokens`, else `ok`. (Block-count is recorded but does NOT contribute to the risk level yet.) +- Surface in **two** places (NOT the statusline this release — see Resolved decisions #2): + 1. **Per-session JSON** — `thinking_desync_risk` + the raw counts, for any consumer. + 2. **One-time stderr log** — when a session first crosses into `high`, so headless/non-statusline surfaces get the signal once (not on every request). -`high` when EITHER context_tokens ≥ high-tokens OR thinking_block_count ≥ high-blocks; `warn` at the lower thresholds; `ok` otherwise. Token-OR-block (not AND) because either dimension alone can carry the risk, and the cheap conservative bias is to warn early — a false "retire soon" is far cheaper than a dead session. +### Config (env vars) -## Open questions (for AI Team Lead) +- `CACHE_FIX_THINKING_RISK_WARN_TOKENS` (default **250000**) +- `CACHE_FIX_THINKING_RISK_HIGH_TOKENS` (default **340000** — just under the observed ~382K trip, with margin) +- `CACHE_FIX_THINKING_RISK=off` to disable the warning (telemetry still recorded) +- Block-threshold env vars (`..._WARN_BLOCKS` / `..._HIGH_BLOCKS`) are **deferred to the fast-follow**, set once `thinking_block_max` telemetry gives the failure distribution. Not introduced this release. -1. **Phasing** — ship measure + warn together (one minor), or land Phase 1 telemetry first to calibrate, then Phase 2 in a follow-up? Lean: separate, so thresholds are evidence-based. But that's two releases. -2. **Statusline ownership** — `quota-statusline.sh` is community-contributed (@schuay). A new risk segment touches it; coordinate or keep the signal in the per-session JSON only and let the statusline opt in later? -3. **Default thresholds** — anchor on the single observed incident (~382K / ~6,850 total), or hold defaults conservative and let Phase 1 data set them? The in-context block count at failure is the number we actually lack. -4. **Scope of "thinking_block_count"** — count only the latest request's blocks, or track session high-water? (Directive proposes both fields.) +Conservative early-warn bias is intentional: a premature "retire soon" is far cheaper than a dead session. + +### Fast-follow (separate, after data) + +Once production `thinking_block_max` telemetry shows the in-context block count at/near failure, add the block dimension to the risk computation (`high`/`warn` on EITHER tokens OR blocks) with calibrated `..._BLOCKS` defaults. Tracked as a follow-up, not part of v3.8.0. + +## Resolved scope decisions (AI Team Lead, 2026-05-28, #158) + +1. **Phasing → split by dimension, one release.** v3.8.0 ships full telemetry + the active token-gated warn now (token trip is anchored); the block dimension is telemetry-only and activates in a calibrated fast-follow. Avoids shipping a warn-nothing release while the failure keeps recurring. +2. **Statusline → leave `quota-statusline.sh` untouched this release.** Signal via per-session JSON + one-time stderr log only. A separate coordination issue/PR will propose the optional risk segment for @schuay to opt into or own — keeping community-code edits out of this release and the contributor boundary clean. +3. **Defaults → anchor tokens, hold blocks.** Token `high` ~340K / `warn` ~250K; no blind block defaults (telemetry-only until data sets them). Conservative early-warn bias retained. +4. **`thinking_block_count` → track both.** Latest-request count (live driver) and `thinking_block_max` high-water (the missing calibration data). ## Out of scope @@ -66,4 +73,4 @@ Add per-session telemetry so we can calibrate thresholds against real data rathe ## Version target -Minor — **v3.8.0** (new extension + new env vars + new statusline behavior). Per `docs/release-workflow.md`, AI Team Lead approves this directive's scope before implementation begins. If split into measure/warn phases, Phase 1 telemetry could ship as a smaller minor and Phase 2 as the next. +Minor — **v3.8.0** (new extension + new env vars). No statusline change this release (decision #2), so no community-code edit. Scope approved by AI Team Lead (#158); ready for the directive-stage PR + Codex review loop. The block-dimension fast-follow is a later patch/minor once telemetry calibrates it. From f5f6f434a18b4259716c96c70dd07206ec98044d Mon Sep 17 00:00:00 2001 From: "vsits-codex-review-agent[bot]" <279859562+vsits-codex-review-agent[bot]@users.noreply.github.com> Date: Thu, 28 May 2026 16:47:38 +0000 Subject: [PATCH 03/13] docs(review): directive review for session-health warning PR #160 --- ...ealth-directive-codex-review-2026-05-28.md | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 docs/code-reviews/pr160-session-health-directive-codex-review-2026-05-28.md diff --git a/docs/code-reviews/pr160-session-health-directive-codex-review-2026-05-28.md b/docs/code-reviews/pr160-session-health-directive-codex-review-2026-05-28.md new file mode 100644 index 00000000..eae9e626 --- /dev/null +++ b/docs/code-reviews/pr160-session-health-directive-codex-review-2026-05-28.md @@ -0,0 +1,33 @@ +# Review: session-health early-warning directive + +Date: 2026-05-28 +Reviewed: `docs/directives/proxy-session-health-warning.md` +Label applied: `reviewed-by-codex-agent` + +## What Is Correct + +- The warn-only boundary is explicit and appropriately narrow. The directive states twice that cache-fix must not try to repair or mutate thinking blocks and keeps auto-retire, auto-clear, and cross-host aggregation out of scope, which is the right safety cut for an upstream Claude Code failure mode rather than a proxy-owned one (`docs/directives/proxy-session-health-warning.md:16-18,68-72`). +- The split-by-dimension release shape is sound. Shipping token-gated warning now while holding block-gated warning for a calibrated fast-follow matches the evidence quality we actually have: one live-context trip point at ~382K tokens, but no in-context block-count distribution yet (`docs/directives/proxy-session-health-warning.md:27-30,43,50-53,57-59`). +- The measurability claims are real against the current proxy surfaces. Request bodies are fully parsed and passed through `runOnRequest()` before forwarding (`proxy/server.mjs:26-61`), SSE `usage` is already captured from `message_start` / `message_delta` (`proxy/stream.mjs:15-29`), and `cache-telemetry` already resolves session id on the request side and writes the per-session file on stream completion (`proxy/extensions/cache-telemetry.mjs:158-239`). +- The telemetry extension point is backward-safe as described, provided the existing cache fields stay intact. Current shipped consumers read `cache.ttl_tier`, `cache.hit_rate`, and the top-level `timestamp`; additive top-level risk fields will not break that contract (`proxy/extensions/cache-telemetry.mjs:213-229`, `tools/quota-statusline.sh:85-99,185-194`). +- Keeping statusline changes out of v3.8.0 is the right coordination boundary. Per-session JSON plus a stderr signal is enough to ship the warning without pulling community-owned UI code into the same change set (`docs/directives/proxy-session-health-warning.md:44-46,61-65`). + +## Blockers + +None. + +## What Needs Attention + +- `docs/directives/proxy-session-health-warning.md:52` should be tightened before implementation/docs fan-out so `CACHE_FIX_THINKING_RISK=off` has one unambiguous meaning. As written, "telemetry still recorded" leaves room for two materially different behaviors: raw counts only, or raw counts plus a still-populated `thinking_desync_risk` field. The implementation should not have to guess whether "off" suppresses only stderr or every built-in warning surface. +- `docs/directives/proxy-session-health-warning.md:24` slightly overstates today's surface. The current per-session file does not already carry `first_seen`; it only writes `cache`, `timestamp`, and `session_id` today (`proxy/extensions/cache-telemetry.mjs:214-227`). That does not block the design, but the wording should reflect that `first_seen` is part of the new persisted state, not an already-available field. +- The eventual implementation should count `thinking_block_count` from the post-pipeline request body that is actually forwarded upstream, not a raw pre-pipeline snapshot. The server runs the full request pipeline before serializing `reqCtx.body` back to `forwardBody` (`proxy/server.mjs:43-58`), and using that final shape gives the most accurate "live risk driver" measurement if any earlier extension has normalized the body. + +## Recommendations + +- Approve the directive and move to implementation, but lock the `CACHE_FIX_THINKING_RISK=off` behavior in the implementation PR and README so JSON consumers and stderr behavior stay consistent. +- In the implementation review checklist, require the one-time stderr signal to be keyed on a per-session transition into `high` rather than "every request while high." The directive already implies that outcome; making it explicit will keep tests and implementation aligned. +- Preserve the current per-session JSON keys exactly and append the new risk fields additively at the top level. That keeps `tools/quota-statusline.sh` and other current readers compatible while still exposing the new telemetry. + +## Bottom Line + +Ship this directive. The scope boundary is disciplined, the token/block split matches the current evidence, and the underlying proxy already has the measurement surfaces the design relies on. I do not see a directive-level blocker to implementation; only a few wording clarifications should be tightened so the env toggle and persisted-state contract are interpreted the same way by every implementer and consumer. From 8e3eaee2d6f752b6905f9aacfe2fe63e7992cb3c Mon Sep 17 00:00:00 2001 From: "vsits-proxy-builder[bot]" <206502658+vsits-proxy-builder[bot]@users.noreply.github.com> Date: Thu, 28 May 2026 17:26:25 +0000 Subject: [PATCH 04/13] directive: add warning-half/recovery-half framing + correct canonical upstream ref - Point References at canonical anthropics/claude-code#63147 (our #63172 was consolidated into it) + note the #63143 trigger variant. - Add warning-half (this directive) vs recovery-half (in-place .jsonl heal, tracked in restore-claude-history-linux) framing. Docs-only related-links addition; no scope change. --- docs/directives/proxy-session-health-warning.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/directives/proxy-session-health-warning.md b/docs/directives/proxy-session-health-warning.md index 3f39e9da..370e56d8 100644 --- a/docs/directives/proxy-session-health-warning.md +++ b/docs/directives/proxy-session-health-warning.md @@ -3,10 +3,12 @@ **Status:** Scope APPROVED by AI Team Lead 2026-05-28 (issue #158). Ready for directive-stage PR + Codex review. New feature → minor release (v3.8.0); per `docs/release-workflow.md` the maintenance-mode gate is at this directive stage and has been cleared. **Author:** Proxy Builder (directive), AI Team Lead (scope approval + refinements) **References:** -- `anthropics/claude-code#63172` — upstream root-cause bug (interleaved-thinking signature desync on extreme-scale sessions) +- `anthropics/claude-code#63147` — canonical upstream root-cause bug (interleaved-thinking signature desync; our report #63172 was consolidated into it). `anthropics/claude-code#63143` is the `AskUserQuestion`-cancel trigger variant. - cache-fix `#157` — defensive thinking-block guards (related but separate) - `playbook_manual_compact_procedure.md` (shared memory) — the manual retirement procedure this warning feeds into +**Warning half vs recovery half:** this directive is the **pre-wedge early-warning** — it flags a session approaching the desync-trip scale so the operator can retire it deliberately. The complementary **post-wedge recovery** — healing a wedged session in place by stripping standalone thinking rows from the `.jsonl` and re-linking `parentUuid` (see `playbook_heal_thinking_wedged_session.md`) — is being tracked as a repair-tool in the `restore-claude-history-linux` project. Warn before; heal after. + ## Problem statement Long-running Opus 4.7 (`claude-opus-4-7[1m]`) sessions accumulate interleaved thinking blocks and grow their context window until Claude Code's own history management (compaction / context-editing / parallel-tool-cancellation reconstruction) desyncs a thinking-block signature. The result is a hard `400 messages..content.: thinking blocks ... cannot be modified` on essentially every subsequent turn — the session becomes unusable and the only recovery is retiring it (see #63172 for the full mechanism). From ee26c48e2b222eebcaf602fbc346d1adffcde732 Mon Sep 17 00:00:00 2001 From: "vsits-proxy-builder[bot]" <206502658+vsits-proxy-builder[bot]@users.noreply.github.com> Date: Thu, 28 May 2026 21:00:57 +0000 Subject: [PATCH 05/13] directive: add Non-Functional Requirements section to session-health (NFR standard #161) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Brings the directive into compliance with the revised review standard (PR #161 / AGENTS.md): size budget, threat model (counts-only, read-only on bodies), maintainability, performance, and a Load-bearing? declaration (yes on the schema-contract dimension — extends the per-session JSON; NOT request-body-mutating like #162). Ref #158 #160 --- docs/directives/proxy-session-health-warning.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/docs/directives/proxy-session-health-warning.md b/docs/directives/proxy-session-health-warning.md index 370e56d8..e7302f1f 100644 --- a/docs/directives/proxy-session-health-warning.md +++ b/docs/directives/proxy-session-health-warning.md @@ -67,6 +67,14 @@ Once production `thinking_block_max` telemetry shows the in-context block count 3. **Defaults → anchor tokens, hold blocks.** Token `high` ~340K / `warn` ~250K; no blind block defaults (telemetry-only until data sets them). Conservative early-warn bias retained. 4. **`thinking_block_count` → track both.** Latest-request count (live driver) and `thinking_block_max` high-water (the missing calibration data). +## Non-Functional Requirements + +- **Size/complexity budget:** small–moderate — telemetry fields on the existing per-session writer + a token-threshold risk computation + a one-time stderr warn. Reuses `cache-telemetry`'s `usage` read, the per-session quota-status writer, and `onRequest` body access. No new subsystem. ~100–200 LOC + tests; flag at review if it grows materially past that. +- **Threat model:** counts/tokens only. MUST NOT log, persist, or emit thinking text, signatures, or any request/response content — telemetry is numeric (`context_tokens`, `thinking_block_count`, `thinking_block_max`, risk level) plus a content-free warn line. **Read-only on request/response bodies** — this extension observes and records; it never mutates the body. No new inbound surface. +- **Maintainability constraints:** reuse the existing per-session quota-status writer and `cache-telemetry`'s usage extraction; do not introduce a new abstraction for the count/threshold logic. New JSON fields are additive. No dead code, no back-compat shims. +- **Performance/reliability:** O(content-blocks) per request to count thinking blocks; cheap. Because the transform is read-only on the body, it does not churn the prompt-cache prefix. +- **Load-bearing? yes — schema-contract dimension only.** It does **not** modify request/response bodies (unlike the sibling #162 sanitize), so it carries none of the request-path correctness/cache-mutation risk. BUT it extends the per-session quota-status JSON — a wire/schema contract that downstream consumers (statusline, dashboards) read — so it qualifies as load-bearing on the schema dimension. The additions are backward-compatible (new optional fields; existing consumers unaffected). Recommend the `schema-change` label and a brief human (Chris) confirmation that the schema additions don't break existing per-session consumers; it does not carry the request-mutation review burden #162 does. + ## Out of scope - **Fixing or working around the desync** — that's #63172, CC-side. cache-fix must not attempt to mutate/strip thinking blocks to "repair" a session (that path is exactly what #157 guards against). Warning only. From 9f0b90fe8ec4662d25151ff9d63b157c45a424e7 Mon Sep 17 00:00:00 2001 From: "vsits-codex-review-agent[bot]" <279859562+vsits-codex-review-agent[bot]@users.noreply.github.com> Date: Thu, 28 May 2026 21:13:08 +0000 Subject: [PATCH 06/13] docs(review): re-review session-health directive PR #160 --- ...lth-directive-codex-rereview-2026-05-28.md | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 docs/code-reviews/pr160-session-health-directive-codex-rereview-2026-05-28.md diff --git a/docs/code-reviews/pr160-session-health-directive-codex-rereview-2026-05-28.md b/docs/code-reviews/pr160-session-health-directive-codex-rereview-2026-05-28.md new file mode 100644 index 00000000..15363a9b --- /dev/null +++ b/docs/code-reviews/pr160-session-health-directive-codex-rereview-2026-05-28.md @@ -0,0 +1,46 @@ +# Review: session-health early-warning directive + +Date: 2026-05-28 +Reviewed: `docs/directives/proxy-session-health-warning.md` +Label applied: `changes-requested` + +## What Is Correct + +- The directive now includes a real `## Non-Functional Requirements` section, and the size, threat-model, maintainability, and performance lines are concrete and non-empty rather than boilerplate (`docs/directives/proxy-session-health-warning.md:70-76`). +- The design remains disciplined. It keeps the feature on the warn-only side of the boundary, explicitly forbids any thinking-block mutation or repair behavior, and leaves auto-retire/auto-clear and cross-host aggregation out of scope (`docs/directives/proxy-session-health-warning.md:20,78-82`). +- The split-by-dimension release plan is still the right shape. Token-gated warning now and block-gated warning only after calibration matches the evidence quality we actually have: a live-context failure point around `~382K`, but no in-context block-count failure distribution yet (`docs/directives/proxy-session-health-warning.md:31,45,55,61,65-68`). +- The implementation surfaces the directive relies on are present in the current codebase. Request bodies are available and rewritten from the post-pipeline `reqCtx.body` before forwarding (`proxy/server.mjs:43-58`), SSE usage is already extracted from `message_start` / `message_delta` (`proxy/stream.mjs:15-29`), and `cache-telemetry` already persists per-session JSON keyed from request-side session headers (`proxy/extensions/cache-telemetry.mjs:158-167,182-238`). +- The load-bearing classification itself is directionally correct: this feature is read-only on request/response bodies, so it does not carry the request-path mutation risk of the sibling sanitize directive, but it does extend a consumed JSON contract and therefore qualifies as load-bearing on the schema-contract dimension (`docs/directives/proxy-session-health-warning.md:73-76`, `CLAUDE.md:86-94`). + +## Blockers + +- `docs/directives/proxy-session-health-warning.md:76` understates the required review burden for a load-bearing change. The repo standard is explicit that anything touching a wire/schema contract is load-bearing and therefore requires Chris review before merge (`CLAUDE.md:92-94`). The current text says "Recommend ... a brief human (Chris) confirmation," which is weaker than the rule it is trying to satisfy. This should be tightened from a recommendation to an explicit requirement before the directive is treated as approved under the revised workflow. + +## What Needs Attention + +- `docs/directives/proxy-session-health-warning.md:54` should be made unambiguous before implementation: `CACHE_FIX_THINKING_RISK=off` currently says "disable the warning (telemetry still recorded)," but that still leaves room for disagreement about whether `thinking_desync_risk` continues to be written in JSON or whether only the raw numeric telemetry remains. +- `docs/directives/proxy-session-health-warning.md:26` slightly overstates the current state of the session file. The per-session file already exists, but `first_seen` is new persisted state, not an already-present field; current writes are still limited to `cache`, `timestamp`, and `session_id` (`proxy/extensions/cache-telemetry.mjs:213-227`). +- Implementation should count `thinking_block_count` from the post-pipeline request body that is actually forwarded upstream, not from a raw pre-pipeline snapshot. The current server architecture makes that straightforward and keeps the metric aligned with the true live request shape (`proxy/server.mjs:44-58`). + +## Bloat / Non-Functional + +- No bloat finding. The stated `~100-200 LOC + tests` budget is reasonable for additive fields on the existing per-session writer, a small threshold computation, and one transition-gated stderr warning. The directive explicitly avoids a new subsystem and stays consistent with the repo's existing telemetry-first patterns (`docs/directives/proxy-session-health-warning.md:72-75`, `preload.mjs:1752-1760,2711-2789`). +- The schema-contract-only load-bearing call is the right strength. It should not be weakened to "non-load-bearing" just because the fields are additive, because the repo rule explicitly treats wire/schema-contract changes as load-bearing. It also does not need to be strengthened to request-path/security-risk parity with `#162`, because this directive is explicitly read-only on request/response bodies (`docs/directives/proxy-session-health-warning.md:73-76`, `CLAUDE.md:92-94`). + +## Size Baseline + +- `docs/directives/proxy-session-health-warning.md` — 86 LOC — directive is focused, with clear scope boundaries and one new NFR gate to tighten. +- `proxy/extensions/cache-telemetry.mjs` — 248 LOC — natural implementation home for additive session JSON fields and usage-derived risk data. +- `proxy/server.mjs` — 355 LOC — already exposes the post-pipeline request body needed for accurate block counting without new plumbing. +- `proxy/stream.mjs` — 110 LOC — already extracts the token telemetry the warning logic depends on. +- `tools/quota-statusline.sh` — 204 LOC — existing consumer of the per-session JSON; relevant to schema compatibility, intentionally untouched by this release. + +## Recommendations + +- Update the `Load-bearing?` line so it explicitly says Chris review is required before merge for this directive's schema-contract change, then keep the `schema-change` label on the PR. +- Keep the directive approved on design once that wording is fixed; the core plan is still sound and scoped appropriately for a minor release. +- When implementation begins, lock the `CACHE_FIX_THINKING_RISK=off` behavior in code and README at the same time so stderr behavior and JSON contract stay aligned. + +## Bottom Line + +The design still looks good, and the new NFR section is materially better than the earlier version. The remaining problem is process correctness, not architecture: the directive correctly identifies itself as load-bearing on the schema-contract dimension, but it phrases the Chris review requirement as optional. Under the revised repo standard, that needs to be made explicit before this directive should carry an approval state again. From a30d21c3f2e8a26ecf6350c30a20347322bdc5f3 Mon Sep 17 00:00:00 2001 From: "vsits-proxy-builder[bot]" <206502658+vsits-proxy-builder[bot]@users.noreply.github.com> Date: Thu, 28 May 2026 21:16:58 +0000 Subject: [PATCH 07/13] =?UTF-8?q?directive:=20address=20Codex=20#160=20re-?= =?UTF-8?q?review=20=E2=80=94=20require=20(not=20recommend)=20Chris=20revi?= =?UTF-8?q?ew=20for=20schema=20change;=20clarify=20kill-switch=20scope?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Load-bearing schema-contract change now states Chris review is REQUIRED per CLAUDE.md (was framed as a recommendation — Codex blocker). - Clarify CACHE_FIX_THINKING_RISK=off: suppresses the warn (stderr + thinking_desync_risk field); raw count telemetry keeps recording. Ref #160 --- docs/directives/proxy-session-health-warning.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/directives/proxy-session-health-warning.md b/docs/directives/proxy-session-health-warning.md index e7302f1f..aefe90cd 100644 --- a/docs/directives/proxy-session-health-warning.md +++ b/docs/directives/proxy-session-health-warning.md @@ -51,7 +51,7 @@ Extend the per-session quota-status JSON, written on each request via the existi - `CACHE_FIX_THINKING_RISK_WARN_TOKENS` (default **250000**) - `CACHE_FIX_THINKING_RISK_HIGH_TOKENS` (default **340000** — just under the observed ~382K trip, with margin) -- `CACHE_FIX_THINKING_RISK=off` to disable the warning (telemetry still recorded) +- `CACHE_FIX_THINKING_RISK=off` disables the warning signal — both the stderr warn line AND the computed `thinking_desync_risk` field — while raw count telemetry (`context_tokens`, `thinking_block_count`, `thinking_block_max`) keeps recording (always useful, and it feeds the block-threshold calibration) - Block-threshold env vars (`..._WARN_BLOCKS` / `..._HIGH_BLOCKS`) are **deferred to the fast-follow**, set once `thinking_block_max` telemetry gives the failure distribution. Not introduced this release. Conservative early-warn bias is intentional: a premature "retire soon" is far cheaper than a dead session. @@ -73,7 +73,7 @@ Once production `thinking_block_max` telemetry shows the in-context block count - **Threat model:** counts/tokens only. MUST NOT log, persist, or emit thinking text, signatures, or any request/response content — telemetry is numeric (`context_tokens`, `thinking_block_count`, `thinking_block_max`, risk level) plus a content-free warn line. **Read-only on request/response bodies** — this extension observes and records; it never mutates the body. No new inbound surface. - **Maintainability constraints:** reuse the existing per-session quota-status writer and `cache-telemetry`'s usage extraction; do not introduce a new abstraction for the count/threshold logic. New JSON fields are additive. No dead code, no back-compat shims. - **Performance/reliability:** O(content-blocks) per request to count thinking blocks; cheap. Because the transform is read-only on the body, it does not churn the prompt-cache prefix. -- **Load-bearing? yes — schema-contract dimension only.** It does **not** modify request/response bodies (unlike the sibling #162 sanitize), so it carries none of the request-path correctness/cache-mutation risk. BUT it extends the per-session quota-status JSON — a wire/schema contract that downstream consumers (statusline, dashboards) read — so it qualifies as load-bearing on the schema dimension. The additions are backward-compatible (new optional fields; existing consumers unaffected). Recommend the `schema-change` label and a brief human (Chris) confirmation that the schema additions don't break existing per-session consumers; it does not carry the request-mutation review burden #162 does. +- **Load-bearing? yes — schema-contract dimension only.** It does **not** modify request/response bodies (unlike the sibling #162 sanitize), so it carries none of the request-path correctness/cache-mutation risk. BUT it extends the per-session quota-status JSON — a wire/schema contract that downstream consumers (statusline, dashboards) read — so it qualifies as load-bearing on the schema dimension. The additions are backward-compatible (new optional fields; existing consumers unaffected). Per CLAUDE.md's load-bearing rule, wire/schema-contract changes **require** human (Chris) review before merge — here specifically to confirm the additive fields don't break existing per-session consumers — and the `schema-change` label applies. (It does not carry #162's request-mutation review burden, but the schema-contract change is a Chris-review gate, not a recommendation.) ## Out of scope From 24ed623d1dcd95d7117fe1ac3ac9637c0523d45e Mon Sep 17 00:00:00 2001 From: "vsits-codex-review-agent[bot]" <279859562+vsits-codex-review-agent[bot]@users.noreply.github.com> Date: Thu, 28 May 2026 21:59:50 +0000 Subject: [PATCH 08/13] docs(review): confirm session-health directive PR #160 --- ...th-directive-codex-rereview2-2026-05-28.md | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 docs/code-reviews/pr160-session-health-directive-codex-rereview2-2026-05-28.md diff --git a/docs/code-reviews/pr160-session-health-directive-codex-rereview2-2026-05-28.md b/docs/code-reviews/pr160-session-health-directive-codex-rereview2-2026-05-28.md new file mode 100644 index 00000000..7e352716 --- /dev/null +++ b/docs/code-reviews/pr160-session-health-directive-codex-rereview2-2026-05-28.md @@ -0,0 +1,29 @@ +# Review: session-health early-warning directive + +Date: 2026-05-28 +Reviewed: `docs/directives/proxy-session-health-warning.md` +Label applied: `reviewed-by-codex-agent` + +## What Is Correct + +- The prior blocker is resolved at `92f8192`: the `Load-bearing?` line now states that this schema-contract change requires Chris review before merge, which is the right gate for additive per-session JSON fields consumed by downstream status surfaces (`docs/directives/proxy-session-health-warning.md:76`). +- The `CACHE_FIX_THINKING_RISK=off` contract is now explicit. It disables both built-in warning surfaces that this directive introduces: the one-time stderr warn line and the computed `thinking_desync_risk` field, while keeping raw numeric telemetry recording in place for calibration (`docs/directives/proxy-session-health-warning.md:54`). +- The broader directive remains disciplined and implementation-ready: warn-only scope, token-gated warning in this release, block telemetry recorded now but held out of the risk computation until the fast-follow calibration, and no statusline/community-code coupling in v3.8.0 (`docs/directives/proxy-session-health-warning.md:45-68,78-82`). + +## Blockers + +None. + +## What Needs Attention + +None at the directive level. Implementation review should still verify that `thinking_block_count` is derived from the forwarded post-pipeline body and that the additive per-session JSON fields remain backward-safe for existing consumers. + +## Recommendations + +- Keep the `schema-change` label on the PR. +- Treat Chris review as a required merge gate for this directive's additive per-session JSON fields, as the directive now says explicitly. +- Carry the clarified `CACHE_FIX_THINKING_RISK=off` behavior through implementation and documentation without drifting from the approved contract. + +## Bottom Line + +Approve. The only blocking process mismatch from the prior re-review is fixed at `92f8192`, the kill-switch scope is now unambiguous, and I do not see a new directive-level issue that should keep this PR in `changes-requested`. From b0b84d417f8b238f3a573154336196e147ae1205 Mon Sep 17 00:00:00 2001 From: "vsits-team-lead-agent[bot]" <279795570+vsits-team-lead-agent[bot]@users.noreply.github.com> Date: Thu, 28 May 2026 23:33:26 +0000 Subject: [PATCH 09/13] feat(session-health): thinking-desync early-warning extension (#158, #160) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New read-only observation extension (order 590) that flags long-running Opus 4.7 [1m] sessions approaching the thinking-desync wedge (anthropics/claude-code#63147) before they die, so the operator can retire them deliberately. It never mutates the body and never tries to repair the desync — warn-before only. Records additive per-session JSON telemetry (context_tokens, thinking_block_count, thinking_block_max, first_seen, request_count, thinking_desync_risk) via the existing single writer (cache-telemetry merges ctx.meta._sessionHealth), plus a token-gated one-time stderr warn. Cross-request fields seed from the prior file so they survive proxy restarts. Counts only — no thinking text or signatures recorded. CACHE_FIX_THINKING_RISK=off suppresses the warning signal while raw counts keep recording. Implements the session-health directive; full suite 886 green. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 8 + README.md | 25 ++- proxy/extensions/cache-telemetry.mjs | 11 + proxy/extensions/session-health.mjs | 152 +++++++++++++ test/proxy-quota-status-pipeline.test.mjs | 41 +++- test/proxy-session-health.test.mjs | 249 ++++++++++++++++++++++ 6 files changed, 482 insertions(+), 4 deletions(-) create mode 100644 proxy/extensions/session-health.mjs create mode 100644 test/proxy-session-health.test.mjs diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f00b63f..710b179c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,14 @@ ## [Unreleased] +### Added + +- **session-health early-warning extension (#158, #160).** A new read-only observation extension (`proxy/extensions/session-health.mjs`, order 590) that flags long-running Opus 4.7 `[1m]` sessions approaching the thinking-desync wedge (upstream `anthropics/claude-code#63147`) before they die. It never mutates the request/response body and never attempts to repair the desync — it only warns so the operator can retire the session deliberately. This is the *warn-before* half of the thinking-desync response; mitigation (#162) and offline heal are tracked separately. + + **New per-session JSON fields (additive).** `~/.claude/quota-status/sessions/.json` now also carries `context_tokens` (latest live context = `input + cache_read + cache_creation`), `thinking_block_count` (`thinking`/`redacted_thinking` blocks in the latest request), `thinking_block_max` (session high-water, carried across proxy restarts), `first_seen`, `request_count`, and `thinking_desync_risk` (`ok`/`warn`/`high`). Fields are written by the existing single per-session writer (`cache-telemetry`); existing consumers are unaffected (all use optional reads). Counts only — no thinking text or signatures are ever recorded. + + **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. + ### 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 07e80f0b..d19d256d 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, 7 extensions run in order: +On every `/v1/messages` request, 8 extensions run in order: | Extension | What it fixes | |-----------|--------------| @@ -40,6 +40,7 @@ On every `/v1/messages` request, 7 extensions run in order: | `fresh-session-sort` | Fixes non-deterministic ordering on first turn | | `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 | 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`. @@ -723,6 +724,28 @@ Scoping rules baked into the extension: |---------|---------|---------| | `CACHE_FIX_THINKING_DISPLAY` | `summarized` (built-in) | One of `summarized` / `omitted` / `disabled`. `summarized` restores thinking summaries (default). `omitted` force-suppresses thinking blocks. `disabled` opts the extension out entirely. | +## Session-health early-warning (proxy mode, thinking-desync risk) + +Long-running Opus 4.7 `[1m]` sessions accumulate interleaved thinking blocks and grow their live context until Claude Code's own history reconstruction desyncs a thinking-block signature, producing a permanent `400 … thinking blocks … cannot be modified` on every subsequent turn (upstream root cause: [anthropics/claude-code#63147](https://github.com/anthropics/claude-code/issues/63147)). The session dies abruptly with no prior signal. + +The `session-health` extension watches the conditions that correlate with the trip and warns **before** a session reaches the danger zone, so the operator can retire it deliberately (write a session-state handoff, `/clear`) instead of being surprised by a dead session. It is **read-only** — it never mutates the request/response body and never attempts to repair the desync (that is CC-side, #63147). It records numeric telemetry into the per-session file (`~/.claude/quota-status/sessions/.json`) on each request and, when a session first crosses into `high` risk, emits a one-time stderr line. Counts only — no thinking text or signatures are ever logged. + +Fields added to the per-session JSON: + +- `context_tokens` — latest request's live context (`input + cache_read + cache_creation`) +- `thinking_block_count` — `thinking`/`redacted_thinking` blocks in the latest request +- `thinking_block_max` — session high-water mark (carried across proxy restarts) +- `first_seen`, `request_count` — session age + request tally +- `thinking_desync_risk` — `ok` / `warn` / `high` (omitted when the signal is disabled) + +Token thresholds are anchored to the observed ~382K-token trip with margin; the warning is conservative by design — a premature "retire soon" is far cheaper than a dead session. Block-count is recorded but does not yet gate the warning (it activates in a calibrated fast-follow once the failure distribution is known). + +| Env var | Default | Purpose | +|---------|---------|---------| +| `CACHE_FIX_THINKING_RISK_WARN_TOKENS` | `250000` | Context-token level at which `thinking_desync_risk` becomes `warn`. | +| `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. | + ## 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/proxy/extensions/cache-telemetry.mjs b/proxy/extensions/cache-telemetry.mjs index 274406ab..ad101388 100644 --- a/proxy/extensions/cache-telemetry.mjs +++ b/proxy/extensions/cache-telemetry.mjs @@ -49,6 +49,13 @@ export function sessionFilename(rawId) { return "inv-" + createHash("sha256").update(s).digest("hex").slice(0, 16); } +// Full path to the per-session file for a raw session id. Exported so sibling +// extensions (e.g. session-health) can READ the prior state this writer wrote, +// using the identical filename rule — reuse, not duplicate. +export function sessionFilePath(rawId) { + return join(paths().sessionsDir, `${sessionFilename(rawId)}.json`); +} + function resolveSessionId(headers) { if (!headers) return null; const sid = @@ -222,6 +229,10 @@ export default { hit_rate: hitRate, timestamp, }, + // Additive session-health fields (session-health extension, order + // 590, stashes these before this writer runs). Optional — absent if + // that extension is disabled or produced nothing this request. + ...(ctx.meta._sessionHealth || {}), timestamp, session_id: rawSid, }, diff --git a/proxy/extensions/session-health.mjs b/proxy/extensions/session-health.mjs new file mode 100644 index 00000000..c3b2fd5b --- /dev/null +++ b/proxy/extensions/session-health.mjs @@ -0,0 +1,152 @@ +import { readFileSync } from "node:fs"; +import { sessionFilename, sessionFilePath } from "./cache-telemetry.mjs"; + +// session-health — read-only early-warning for the CC thinking-desync wedge +// (anthropics/claude-code#63147). Long-running Opus 4.7 [1m] sessions grow +// their live context until CC's own history reconstruction desyncs a +// thinking-block signature, producing a permanent 400 on every subsequent +// turn. This extension OBSERVES (never mutates the body) and records the +// conditions that correlate with the trip, plus emits a one-time stderr warn +// so the operator can retire the session deliberately before it dies. +// +// It hands its computed fields to the existing per-session writer +// (cache-telemetry, order 600) via ctx.meta._sessionHealth; cache-telemetry +// merges them into the single per-session JSON write. This extension never +// writes that file itself (single-writer invariant). + +const THINKING_TYPES = new Set(["thinking", "redacted_thinking"]); + +const DEFAULT_WARN_TOKENS = 250_000; +const DEFAULT_HIGH_TOKENS = 340_000; // just under the observed ~382K trip + +// --- Module-scope state --- +// Cross-request accumulators, seeded once-per-process from the prior persisted +// file so first_seen / max / count stay accurate across the proxy restarts +// that multi-week sessions inevitably span. +const sessionState = new Map(); // key -> { firstSeen, max, count } +// Sessions already given the one-time "high" stderr warn this process. +const warnedSessions = new Set(); + +function parseTokenEnv(raw, def) { + if (raw === undefined || raw === "") return def; + const n = Number(raw); + return Number.isFinite(n) && n >= 0 ? n : def; +} + +// Exported for unit testing. +export function loadConfig(env = process.env) { + return { + warnTokens: parseTokenEnv(env.CACHE_FIX_THINKING_RISK_WARN_TOKENS, DEFAULT_WARN_TOKENS), + highTokens: parseTokenEnv(env.CACHE_FIX_THINKING_RISK_HIGH_TOKENS, DEFAULT_HIGH_TOKENS), + enabled: env.CACHE_FIX_THINKING_RISK !== "off", + }; +} + +export function countThinkingBlocks(body) { + if (!body || !Array.isArray(body.messages)) return 0; + let n = 0; + for (const msg of body.messages) { + if (!Array.isArray(msg.content)) continue; + for (const block of msg.content) { + if (block && THINKING_TYPES.has(block.type)) n++; + } + } + return n; +} + +export function computeContextTokens(cacheStats) { + if (!cacheStats) return 0; + return ( + (cacheStats.inputTokens || 0) + + (cacheStats.cacheRead || 0) + + (cacheStats.cacheCreation || 0) + ); +} + +export function computeRisk(contextTokens, { warnTokens, highTokens }) { + if (contextTokens >= highTokens) return "high"; + if (contextTokens >= warnTokens) return "warn"; + return "ok"; +} + +function seedFromFile(rawSid, now) { + let prev = null; + try { + prev = JSON.parse(readFileSync(sessionFilePath(rawSid), "utf8")); + } catch {} + return { + firstSeen: typeof prev?.first_seen === "string" ? prev.first_seen : now, + max: Number.isFinite(prev?.thinking_block_max) ? prev.thinking_block_max : 0, + count: Number.isFinite(prev?.request_count) ? prev.request_count : 0, + }; +} + +export default { + name: "session-health", + description: + "Observe per-session thinking-desync risk (context size + thinking-block count) and warn before the session reaches the danger zone. Read-only; never mutates the body.", + order: 590, // after request-body mutators (so the count is the forwarded body), before the writer (cache-telemetry, 600) + + async onRequest(ctx) { + // Count thinking blocks in the (near-final) forwarded body. Session id is + // resolved by cache-telemetry's onRequest (order 600), which runs AFTER + // this hook — so we don't read the session id here; we read it in + // onStreamEvent, by which time it is set. + ctx.meta._thinkingBlockCount = countThinkingBlocks(ctx.body); + }, + + async onStreamEvent(ctx) { + const { event } = ctx; + if (!event || event.type !== "message_delta") return; + // Once per response, regardless of how many message_delta events arrive. + if (ctx.meta._sessionHealthDone) return; + ctx.meta._sessionHealthDone = true; + + const now = new Date().toISOString(); + const rawSid = ctx.meta._sessionId ?? null; + const key = sessionFilename(rawSid); + const thinkingBlockCount = ctx.meta._thinkingBlockCount || 0; + const contextTokens = computeContextTokens(ctx.meta.cacheStats); + + let st = sessionState.get(key); + if (!st) { + st = seedFromFile(rawSid, now); + sessionState.set(key, st); + } + st.count += 1; + st.max = Math.max(st.max, thinkingBlockCount); + + const health = { + context_tokens: contextTokens, + thinking_block_count: thinkingBlockCount, + thinking_block_max: st.max, + first_seen: st.firstSeen, + request_count: st.count, + }; + + const cfg = loadConfig(); + if (cfg.enabled) { + const risk = computeRisk(contextTokens, cfg); + health.thinking_desync_risk = risk; + if (risk === "high" && !warnedSessions.has(key)) { + warnedSessions.add(key); + const sidLabel = rawSid || "unknown"; + process.stderr.write( + `[session-health] session ${sidLabel} high thinking-desync risk: ` + + `context_tokens=${contextTokens} (>= ${cfg.highTokens}), ` + + `thinking_block_count=${thinkingBlockCount}. ` + + `Consider retiring this session (write SESSION_STATE + /clear).\n`, + ); + } + } + + // Hand off to cache-telemetry (order 600) to persist in its single write. + ctx.meta._sessionHealth = health; + }, + + // Test-only: reset module state between tests. + __resetForTests() { + sessionState.clear(); + warnedSessions.clear(); + }, +}; diff --git a/test/proxy-quota-status-pipeline.test.mjs b/test/proxy-quota-status-pipeline.test.mjs index 2abeaa82..9130cb6a 100644 --- a/test/proxy-quota-status-pipeline.test.mjs +++ b/test/proxy-quota-status-pipeline.test.mjs @@ -41,7 +41,7 @@ function setupHome() { }; } -async function driveFullResponse(extSnapshot, headers, { cacheRead = 0, cacheCreation = 100 } = {}) { +async function driveFullResponse(extSnapshot, headers, { cacheRead = 0, cacheCreation = 100, body } = {}) { // Real proxy puts request and response headers on different ctx objects; // session-id headers come from the request, quota fields from the response. // For these synthetic tests we drive the same `headers` map through both @@ -51,8 +51,8 @@ async function driveFullResponse(extSnapshot, headers, { cacheRead = 0, cacheCre const telemetry = {}; // body required by some upstream-of-cache-telemetry extensions (e.g. // ttl-tier-detect at order 75 walks body.system / body.messages). - const minimalBody = { system: [], messages: [] }; - await runOnRequest({ body: minimalBody, headers, meta }, extSnapshot); + const reqBody = body || { system: [], messages: [] }; + await runOnRequest({ body: reqBody, headers, meta }, extSnapshot); await runOnResponseStart({ headers, meta }, extSnapshot); await runOnStreamEvent( { @@ -119,6 +119,41 @@ 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 () => { + const env = setupHome(); + try { + const exts = await loadExtensions(EXT_DIR, EXT_CONFIG); + const sid = "sess-health-merge"; + const body = { + system: [], + messages: [ + { role: "assistant", content: [ + { type: "thinking", thinking: "", signature: "S" }, + { type: "redacted_thinking", data: "OPAQUE" }, + { type: "text", text: "ok" }, + ] }, + ], + }; + // input_tokens 5 + cacheRead 0 + cacheCreation 100 = 105 context tokens → risk "ok" + 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")); + // existing cache fields still present + assert.equal(sess.session_id, sid); + assert.equal(sess.cache.cache_creation, 100); + // merged session-health fields + assert.equal(sess.context_tokens, 105); + assert.equal(sess.thinking_block_count, 2, "counts thinking + redacted_thinking"); + assert.equal(sess.thinking_block_max, 2); + assert.equal(sess.request_count, 1); + assert.equal(sess.thinking_desync_risk, "ok"); + assert.match(sess.first_seen, /^\d{4}-\d{2}-\d{2}T/); + } finally { + 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-session-health.test.mjs b/test/proxy-session-health.test.mjs new file mode 100644 index 00000000..000f1a19 --- /dev/null +++ b/test/proxy-session-health.test.mjs @@ -0,0 +1,249 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { mkdtempSync, rmSync, mkdirSync, writeFileSync } from "node:fs"; +import { join, dirname } from "node:path"; +import { tmpdir } from "node:os"; +import ext, { + countThinkingBlocks, + computeContextTokens, + computeRisk, + loadConfig, +} from "../proxy/extensions/session-health.mjs"; +import { sessionFilePath } from "../proxy/extensions/cache-telemetry.mjs"; + +// --- Pure-function unit tests --- + +test("countThinkingBlocks: counts thinking + redacted_thinking across messages", () => { + const body = { + messages: [ + { role: "assistant", content: [{ type: "thinking", thinking: "", signature: "S" }, { type: "text", text: "hi" }] }, + { role: "user", content: [{ type: "tool_result", content: "x" }] }, + { role: "assistant", content: [{ type: "redacted_thinking", data: "OPAQUE" }, { type: "tool_use", id: "t" }] }, + ], + }; + assert.equal(countThinkingBlocks(body), 2); +}); + +test("countThinkingBlocks: 0 for no thinking / no messages / string content", () => { + assert.equal(countThinkingBlocks({ messages: [{ role: "user", content: "plain" }] }), 0); + assert.equal(countThinkingBlocks({ messages: [] }), 0); + assert.equal(countThinkingBlocks({}), 0); + assert.equal(countThinkingBlocks(null), 0); +}); + +test("computeContextTokens: sums input + cache_read + cache_creation", () => { + assert.equal(computeContextTokens({ inputTokens: 5, cacheRead: 300_000, cacheCreation: 2_000 }), 302_005); + assert.equal(computeContextTokens({ cacheRead: 100 }), 100); + assert.equal(computeContextTokens(null), 0); + assert.equal(computeContextTokens({}), 0); +}); + +test("computeRisk: boundaries (ok / warn / high)", () => { + const cfg = { warnTokens: 250_000, highTokens: 340_000 }; + assert.equal(computeRisk(249_999, cfg), "ok"); + assert.equal(computeRisk(250_000, cfg), "warn"); // == warn → warn + assert.equal(computeRisk(339_999, cfg), "warn"); + assert.equal(computeRisk(340_000, cfg), "high"); // == high → high + assert.equal(computeRisk(400_000, cfg), "high"); +}); + +test("loadConfig: defaults, overrides, and off", () => { + assert.deepEqual(loadConfig({}), { warnTokens: 250_000, highTokens: 340_000, enabled: true }); + assert.deepEqual( + loadConfig({ CACHE_FIX_THINKING_RISK_WARN_TOKENS: "100000", CACHE_FIX_THINKING_RISK_HIGH_TOKENS: "200000" }), + { warnTokens: 100_000, highTokens: 200_000, enabled: true }, + ); + assert.equal(loadConfig({ CACHE_FIX_THINKING_RISK: "off" }).enabled, false); + // bad values fall back to defaults + assert.equal(loadConfig({ CACHE_FIX_THINKING_RISK_WARN_TOKENS: "nope" }).warnTokens, 250_000); +}); + +// --- onRequest --- + +test("onRequest: stashes thinking-block count on ctx.meta", async () => { + ext.__resetForTests(); + const meta = {}; + const body = { messages: [{ role: "assistant", content: [{ type: "thinking", thinking: "", signature: "S" }] }] }; + await ext.onRequest({ body, meta }); + assert.equal(meta._thinkingBlockCount, 1); +}); + +// --- onStreamEvent integration --- + +function setupTmpHome() { + const dir = mkdtempSync(join(tmpdir(), "sh-")); + const oldHome = process.env.HOME; + process.env.HOME = dir; + ext.__resetForTests(); + return { + home: dir, + cleanup: () => { + process.env.HOME = oldHome; + rmSync(dir, { recursive: true, force: true }); + }, + }; +} + +// Drive one request through session-health, simulating cache-telemetry having +// already resolved the session id (onRequest, order 600) and captured usage +// (message_start). Returns the meta object so the caller can inspect +// _sessionHealth. +async function driveHealth({ sessionId = "sess-1", thinkingBlocks = 0, cacheStats = { inputTokens: 5, cacheRead: 0, cacheCreation: 0 }, env = {} } = {}) { + const oldEnv = {}; + for (const k of Object.keys(env)) { + oldEnv[k] = process.env[k]; + process.env[k] = env[k]; + } + const meta = {}; + try { + const content = Array.from({ length: thinkingBlocks }, () => ({ type: "thinking", thinking: "", signature: "S" })); + await ext.onRequest({ body: { messages: [{ role: "assistant", content }] }, meta }); + // Simulate cache-telemetry's onRequest + message_start: + meta._sessionId = sessionId; + meta.cacheStats = cacheStats; + await ext.onStreamEvent({ event: { type: "message_delta", usage: { output_tokens: 10 } }, meta }); + } finally { + for (const k of Object.keys(oldEnv)) { + if (oldEnv[k] === undefined) delete process.env[k]; + else process.env[k] = oldEnv[k]; + } + } + return meta; +} + +test("onStreamEvent: stashes health fields with risk ok below thresholds", async () => { + const env = setupTmpHome(); + try { + const meta = await driveHealth({ thinkingBlocks: 3, cacheStats: { inputTokens: 5, cacheRead: 1000, cacheCreation: 0 } }); + assert.deepEqual(meta._sessionHealth, { + context_tokens: 1005, + thinking_block_count: 3, + thinking_block_max: 3, + first_seen: meta._sessionHealth.first_seen, // ISO string, set to now + request_count: 1, + thinking_desync_risk: "ok", + }); + assert.match(meta._sessionHealth.first_seen, /^\d{4}-\d{2}-\d{2}T/); + } finally { + env.cleanup(); + } +}); + +test("onStreamEvent: risk 'high' at high threshold", async () => { + const env = setupTmpHome(); + try { + const meta = await driveHealth({ cacheStats: { inputTokens: 0, cacheRead: 345_000, cacheCreation: 0 } }); + assert.equal(meta._sessionHealth.context_tokens, 345_000); + assert.equal(meta._sessionHealth.thinking_desync_risk, "high"); + } finally { + env.cleanup(); + } +}); + +test("onStreamEvent: CACHE_FIX_THINKING_RISK=off omits risk field but keeps raw counts", async () => { + const env = setupTmpHome(); + try { + const meta = await driveHealth({ + thinkingBlocks: 2, + cacheStats: { inputTokens: 0, cacheRead: 345_000, cacheCreation: 0 }, + env: { CACHE_FIX_THINKING_RISK: "off" }, + }); + assert.equal("thinking_desync_risk" in meta._sessionHealth, false, "risk field must be omitted when off"); + assert.equal(meta._sessionHealth.context_tokens, 345_000); + assert.equal(meta._sessionHealth.thinking_block_count, 2); + assert.equal(meta._sessionHealth.request_count, 1); + } finally { + env.cleanup(); + } +}); + +test("onStreamEvent: thinking_block_max is a high-water mark; request_count increments (within a process)", async () => { + const env = setupTmpHome(); + try { + await driveHealth({ sessionId: "sess-hw", thinkingBlocks: 10 }); + const meta2 = await driveHealth({ sessionId: "sess-hw", thinkingBlocks: 4 }); // fewer blocks this turn + assert.equal(meta2._sessionHealth.thinking_block_count, 4); + assert.equal(meta2._sessionHealth.thinking_block_max, 10, "max holds the earlier high"); + assert.equal(meta2._sessionHealth.request_count, 2, "count increments across requests"); + } finally { + env.cleanup(); + } +}); + +test("onStreamEvent: seeds first_seen / max / count from the prior persisted file (survives restart)", async () => { + const env = setupTmpHome(); + try { + // Simulate a prior process having written this session's file. + const sid = "sess-seed"; + const p = sessionFilePath(sid); + mkdirSync(dirname(p), { recursive: true }); + writeFileSync(p, JSON.stringify({ + cache: { ttl_tier: "1h" }, + context_tokens: 300_000, + thinking_block_count: 50, + thinking_block_max: 800, + first_seen: "2026-04-01T00:00:00.000Z", + request_count: 1234, + session_id: sid, + })); + // Fresh process (state cleared) sees this session for the first time. + ext.__resetForTests(); + const meta = await driveHealth({ sessionId: sid, thinkingBlocks: 60 }); + assert.equal(meta._sessionHealth.first_seen, "2026-04-01T00:00:00.000Z", "first_seen carried from file"); + assert.equal(meta._sessionHealth.thinking_block_max, 800, "max carried (current 60 < 800)"); + assert.equal(meta._sessionHealth.request_count, 1235, "count seeded from file + 1"); + } finally { + env.cleanup(); + } +}); + +test("onStreamEvent: one-time 'high' stderr warn fires exactly once per session per process", async () => { + const env = setupTmpHome(); + const origWrite = process.stderr.write.bind(process.stderr); + const lines = []; + process.stderr.write = (s) => { lines.push(String(s)); return true; }; + try { + const high = { inputTokens: 0, cacheRead: 345_000, cacheCreation: 0 }; + await driveHealth({ sessionId: "sess-warn", cacheStats: high }); + await driveHealth({ sessionId: "sess-warn", cacheStats: high }); // still high — must NOT warn again + const warns = lines.filter((l) => l.includes("[session-health]") && l.includes("high thinking-desync risk")); + assert.equal(warns.length, 1, "warn must fire exactly once per session per process"); + assert.match(warns[0], /context_tokens=345000/); + assert.equal(warns[0].includes("thinking"), true); + } finally { + process.stderr.write = origWrite; + env.cleanup(); + } +}); + +test("onStreamEvent: no warn when CACHE_FIX_THINKING_RISK=off even at high context", async () => { + const env = setupTmpHome(); + const origWrite = process.stderr.write.bind(process.stderr); + const lines = []; + process.stderr.write = (s) => { lines.push(String(s)); return true; }; + try { + await driveHealth({ sessionId: "sess-off", cacheStats: { inputTokens: 0, cacheRead: 345_000, cacheCreation: 0 }, env: { CACHE_FIX_THINKING_RISK: "off" } }); + assert.equal(lines.filter((l) => l.includes("[session-health]")).length, 0); + } finally { + process.stderr.write = origWrite; + env.cleanup(); + } +}); + +test("onStreamEvent: ignores non-message_delta events and is idempotent per response", async () => { + const env = setupTmpHome(); + try { + ext.__resetForTests(); + const meta = {}; + meta._sessionId = "sess-idem"; + meta.cacheStats = { inputTokens: 0, cacheRead: 1000, cacheCreation: 0 }; + meta._thinkingBlockCount = 1; + await ext.onStreamEvent({ event: { type: "message_start" }, meta }); + assert.equal(meta._sessionHealth, undefined, "no stash on message_start"); + await ext.onStreamEvent({ event: { type: "message_delta", usage: {} }, meta }); + await ext.onStreamEvent({ event: { type: "message_delta", usage: {} }, meta }); // second delta — must not double-count + assert.equal(meta._sessionHealth.request_count, 1, "counted once per response"); + } finally { + env.cleanup(); + } +}); From feb62f838255942df9c39629ad2d8e706ee50c15 Mon Sep 17 00:00:00 2001 From: "vsits-codex-review-agent[bot]" <279859562+vsits-codex-review-agent[bot]@users.noreply.github.com> Date: Thu, 28 May 2026 23:39:46 +0000 Subject: [PATCH 10/13] docs(review): review session-health implementation PR #160 --- ...-implementation-codex-review-2026-05-28.md | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 docs/code-reviews/pr160-session-health-implementation-codex-review-2026-05-28.md diff --git a/docs/code-reviews/pr160-session-health-implementation-codex-review-2026-05-28.md b/docs/code-reviews/pr160-session-health-implementation-codex-review-2026-05-28.md new file mode 100644 index 00000000..eb10f382 --- /dev/null +++ b/docs/code-reviews/pr160-session-health-implementation-codex-review-2026-05-28.md @@ -0,0 +1,33 @@ +# Review: session-health implementation + +Date: 2026-05-28 +Reviewed: PR #160 implementation at `5dd3873` (`proxy/extensions/session-health.mjs`, `proxy/extensions/cache-telemetry.mjs`, tests, and docs) +Label applied: approved-by-codex-agent + +## What Is Correct + +- The single-writer handoff is correct. `session-health` runs at order `590`, computes/stashes `ctx.meta._sessionHealth` during `onStreamEvent`, and `cache-telemetry` remains the only writer at order `600`. `cache-telemetry`'s request-side `_sessionId` stash is available by the time `session-health.onStreamEvent()` runs, so the read/write threading is coherent. +- The once-per-response guard is correct. `_sessionHealthDone` prevents double-counting when multiple `message_delta` events arrive for one response, and the writer still runs on that same delta event. +- The no-quota path behaves correctly. `session-health` does not depend on `_quotaData`, so it can still compute risk and emit the one-time `high` warning even when `cache-telemetry` skips the per-session write because no quota headers were present. I verified this with a targeted runtime probe in addition to the automated suite. +- Seed-from-file carry-forward is correct for the intended single-process model. On first sight of a session, the extension reads the prior per-session JSON once, seeds `first_seen` / `thinking_block_max` / `request_count`, then continues in memory for the rest of the process lifetime. Because this hook has no awaits, there is no intra-process race window around that seed/update step. +- The additive schema change is backward-safe for current in-repo consumers. The existing readers either use optional field access (`tools/quota-statusline.sh`) or do not parse the per-session JSON payload at all (`rate-limit-log` counts files by mtime). This is still a load-bearing schema-contract change, so Chris human review remains required before merge. +- The threat model is preserved. The extension only records numeric counts/tokens/risk and emits a content-free warning line. No thinking text, signatures, or request/response content are logged or persisted. +- Size and complexity stay within the directive budget. The new extension is small and direct, the writer change is additive, and the test coverage is focused. `node --test` passes cleanly: `886` passing, `0` failing. + +## Blockers + +None. + +## What Needs Attention + +- The automated suite does not yet pin the exact combined behavior the directive cares about most on the degraded telemetry path: no quota headers means no per-session write, but the `high` warning should still fire. The implementation does the right thing today, and I verified it manually, but this would be a useful regression test because that split responsibility crosses two extensions. +- One unit test (`onStreamEvent: risk 'high' at high threshold`) currently leaks a real `[session-health]` warning line into `node --test` output. This is minor, but it is easy to quiet by stubbing `stderr` the same way the dedicated one-time-warning test already does. + +## Recommendations + +- Add one end-to-end regression that drives the real pipeline with a request session id, no quota headers, and high context usage, then asserts both: `~/.claude/quota-status/` is not written and the warning line fires once. +- Silence the standalone high-threshold unit test's `stderr` output so the suite stays clean under repeated CI runs. + +## Bottom Line + +Ship it. The implementation matches the approved directive, preserves the single-writer/file-contract design, keeps the telemetry content-free, and behaves correctly in the restart-seeding and no-quota paths I checked. Formal implementation approval is appropriate at `5dd3873`, with the standing caveat that Chris still needs to sign off on the additive per-session JSON schema change before merge. From 83271f0f835fc52e71f1549bed97192a99f79249 Mon Sep 17 00:00:00 2001 From: "vsits-team-lead-agent[bot]" <279795570+vsits-team-lead-agent[bot]@users.noreply.github.com> Date: Thu, 28 May 2026 23:43:14 +0000 Subject: [PATCH 11/13] =?UTF-8?q?test(session-health):=20address=20Codex?= =?UTF-8?q?=20review=20nits=20=E2=80=94=20degraded-path=20regression=20+?= =?UTF-8?q?=20quiet=20stderr?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add end-to-end pipeline regression: no quota headers → no per-session write, but the high thinking-desync warn still fires once (pins the cross-extension split responsibility Codex flagged). - Stub stderr in the high-threshold unit test so the suite stays quiet under CI. Both non-blocking items from Codex's implementation review. Suite 887 green. Co-Authored-By: Claude Opus 4.7 (1M context) --- test/proxy-quota-status-pipeline.test.mjs | 25 +++++++++++++++++++++++ test/proxy-session-health.test.mjs | 5 +++++ 2 files changed, 30 insertions(+) diff --git a/test/proxy-quota-status-pipeline.test.mjs b/test/proxy-quota-status-pipeline.test.mjs index 9130cb6a..cd0ba8c1 100644 --- a/test/proxy-quota-status-pipeline.test.mjs +++ b/test/proxy-quota-status-pipeline.test.mjs @@ -154,6 +154,31 @@ test("[pipeline #160] session-health fields are merged into the per-session JSON } }); +test("[pipeline #160] degraded path: no quota headers → no per-session write, but the high warn still fires once", async () => { + const env = setupHome(); + const origWrite = process.stderr.write.bind(process.stderr); + const lines = []; + process.stderr.write = (s) => { lines.push(String(s)); return true; }; + try { + const exts = await loadExtensions(EXT_DIR, EXT_CONFIG); + const sid = "sess-noquota"; + // No QUOTA_HEADERS → cache-telemetry skips the per-session write. High + // context (cacheRead) → session-health still computes "high" and warns. + // The warn lives in session-health's own hook, independent of quota. + await driveFullResponse(exts, { "x-claude-code-session-id": sid }, { cacheRead: 345_000, cacheCreation: 0 }); + + const sessionsDir = join(env.home, ".claude", "quota-status", "sessions"); + assert.ok(!existsSync(join(sessionsDir, `${sid}.json`)), "no per-session file when quota headers absent"); + assert.ok(!existsSync(join(env.home, ".claude", "quota-status", "account.json")), "no account.json either"); + + const warns = lines.filter((l) => l.includes("[session-health]") && l.includes("high thinking-desync risk")); + assert.equal(warns.length, 1, "the high warn fires once even on the degraded (no-write) path"); + } finally { + process.stderr.write = origWrite; + 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-session-health.test.mjs b/test/proxy-session-health.test.mjs index 000f1a19..1896aad6 100644 --- a/test/proxy-session-health.test.mjs +++ b/test/proxy-session-health.test.mjs @@ -131,11 +131,16 @@ test("onStreamEvent: stashes health fields with risk ok below thresholds", async test("onStreamEvent: risk 'high' at high threshold", async () => { const env = setupTmpHome(); + // High context fires the one-time stderr warn; stub it so the suite stays + // quiet (the dedicated one-time-warn test asserts the line content). + const origWrite = process.stderr.write.bind(process.stderr); + process.stderr.write = () => true; try { const meta = await driveHealth({ cacheStats: { inputTokens: 0, cacheRead: 345_000, cacheCreation: 0 } }); assert.equal(meta._sessionHealth.context_tokens, 345_000); assert.equal(meta._sessionHealth.thinking_desync_risk, "high"); } finally { + process.stderr.write = origWrite; env.cleanup(); } }); From 0db81ad7f5413ff7db4049ef654b073c1dde5617 Mon Sep 17 00:00:00 2001 From: "vsits-codex-review-agent[bot]" <279859562+vsits-codex-review-agent[bot]@users.noreply.github.com> Date: Thu, 28 May 2026 23:46:15 +0000 Subject: [PATCH 12/13] docs(review): re-approve session-health implementation PR #160 --- ...mplementation-codex-rereview-2026-05-28.md | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) create mode 100644 docs/code-reviews/pr160-session-health-implementation-codex-rereview-2026-05-28.md diff --git a/docs/code-reviews/pr160-session-health-implementation-codex-rereview-2026-05-28.md b/docs/code-reviews/pr160-session-health-implementation-codex-rereview-2026-05-28.md new file mode 100644 index 00000000..6e67fb38 --- /dev/null +++ b/docs/code-reviews/pr160-session-health-implementation-codex-rereview-2026-05-28.md @@ -0,0 +1,29 @@ +# Review: session-health implementation re-review + +Date: 2026-05-28 +Reviewed: PR #160 implementation at `d54e75e` (re-confirm against previously approved `5dd3873`) +Label applied: approved-by-codex-agent + +## What Is Correct + +- The production implementation remains unchanged from the previously approved head. I diffed `5dd3873..d54e75e` for `proxy/extensions/session-health.mjs` and `proxy/extensions/cache-telemetry.mjs`; there is no production-code delta in that range. +- The new degraded-path regression in `test/proxy-quota-status-pipeline.test.mjs` correctly pins the cross-extension contract: with a real session id, no quota headers, and high context usage, the pipeline emits exactly one `high` warning while writing neither `sessions/.json` nor `account.json`. +- The new `process.stderr.write` stub in `test/proxy-session-health.test.mjs` is appropriate. It keeps the standalone high-threshold unit test quiet without weakening the dedicated one-time-warning assertion elsewhere in the suite. +- The broader implementation approval still holds at this head: single-writer file ownership is preserved, `session-health` remains read-only with respect to per-session persistence, and only numeric/count telemetry is recorded. +- Full verification passed at the current head: `node --test` reports `887` passing, `0` failing. + +## Blockers + +None. + +## What Needs Attention + +- Chris human review is still required before merge because this PR adds fields to the per-session JSON schema contract, even though the additions remain backward-compatible for current in-repo consumers. + +## Recommendations + +- None beyond the standing schema-review merge gate. + +## Bottom Line + +Re-approve. The only implementation delta after the previously approved code is the expected review-doc commit plus the two test improvements requested in the prior review, and both test additions strengthen the coverage in the right places without altering runtime behavior. Formal approval is appropriate again at `d54e75e`, with Chris's schema review still serving as the merge gate. From fa90b569f022f2ab9116748e135f411c8b67e4f4 Mon Sep 17 00:00:00 2001 From: "vsits-codex-review-agent[bot]" <279859562+vsits-codex-review-agent[bot]@users.noreply.github.com> Date: Fri, 29 May 2026 00:02:02 +0000 Subject: [PATCH 13/13] docs(review): post-rebase re-confirm session-health implementation PR #160 --- ...ntation-post-rebase-rereview-2026-05-28.md | 40 +++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 docs/code-reviews/pr160-session-health-implementation-post-rebase-rereview-2026-05-28.md diff --git a/docs/code-reviews/pr160-session-health-implementation-post-rebase-rereview-2026-05-28.md b/docs/code-reviews/pr160-session-health-implementation-post-rebase-rereview-2026-05-28.md new file mode 100644 index 00000000..020f78cd --- /dev/null +++ b/docs/code-reviews/pr160-session-health-implementation-post-rebase-rereview-2026-05-28.md @@ -0,0 +1,40 @@ +# Review: session-health implementation post-rebase re-review + +Date: 2026-05-28 +Reviewed: PR #160 implementation at `0db81ad` (post-rebase re-confirm against previously approved pre-rebase content) +Label applied: approved-by-codex-agent + +## What Is Correct + +- The production implementation is unchanged from the previously approved pre-rebase branch state. I compared `0db81ad` against `1d9f0e8` for `proxy/extensions/session-health.mjs`, `proxy/extensions/cache-telemetry.mjs`, `test/proxy-session-health.test.mjs`, and `test/proxy-quota-status-pipeline.test.mjs`; all 4 paths are byte-identical at both revisions. +- The rebase did not clobber the repo instructions. `AGENTS.md` and `CLAUDE.md` at `0db81ad` are byte-identical to `origin/main`. +- The remaining rebased delta is consistent with the PR discussion: the branch picks up mainline changes plus the `CHANGELOG.md` merge, without altering the already-approved session-health runtime behavior. +- Full verification passed at the rebased head: `node --test` reports `891` passing, `0` failing. +- The prior implementation approval still stands on substance: single-writer ownership remains intact, `session-health` stays read-only with respect to session persistence, and only numeric/count telemetry is persisted. + +## Blockers + +None. + +## What Needs Attention + +- Chris human review is still required before merge because this PR adds fields to the per-session JSON schema contract, even though the additions remain backward-compatible for current in-repo consumers. + +## Bloat / Non-Functional + +None. The rebase did not introduce any new runtime complexity or widen the implementation beyond the previously approved scope. + +## Size Baseline + +- `proxy/extensions/session-health.mjs` — 152 LOC — focused read-only extension with request/stream hooks plus small pure helpers. +- `proxy/extensions/cache-telemetry.mjs` — 259 LOC — existing single-writer persistence module; only parity-checked here, not functionally changed by the rebase. +- `test/proxy-session-health.test.mjs` — 254 LOC — targeted unit coverage for risk thresholds, persistence seeding, and warn-once behavior. +- `test/proxy-quota-status-pipeline.test.mjs` — 212 LOC — end-to-end pipeline coverage including the degraded no-quota/high-context path. + +## Recommendations + +- None beyond the standing schema-review merge gate. + +## Bottom Line + +Re-approve. At `0db81ad`, the session-health implementation and its tests are unchanged from the previously approved pre-rebase content, `AGENTS.md` / `CLAUDE.md` match `origin/main`, and the full suite is green at `891/0`. A fresh formal GitHub approval is appropriate for the new head, with Chris's schema review still serving as the merge gate.