[sonichi]

Summary

• check_credential_proxy() only ever proved the port is TCP-listening (probe=False). That left the exact 2026-09-27 incident's failure mode invisible: the proxy stays up while every request 401s/502s because its stored credential can't be refreshed — 55min-2h of every seat losing the API before a human noticed, because health-check kept reporting "ok".
• credential-proxy.ts already tracks this per request via recordCredentialState(), writing credential_state/_detail/_at to quota-state.json. That signal is self-healing (next successful request flips it back to ok) — nothing needed to change in the proxy itself.
• check_credential_proxy() now reads it and escalates to warn when the last-recorded state is exhausted, only while the port is already ok/stale, silent on any read failure.

Known gap this does NOT close: recordCredentialState is only called from the request path, so a background refresh failing before any request arrives stays invisible until #4842's give-up-after-N-failures exits the process — now bounded (~15.5 min) rather than unbounded, not eliminated. Fix #4 (per-seat runtime fallback) is separate follow-on work.

Fixes proposal #1 from the "Credential proxy (:7846) wedges silently" post-mortem (ww room), alongside already-merged #4842 (fix #2) and #4845 (fix #3, mine). Requested by Chi directly.

Test plan

• New suite tests/health-check-credential-proxy-drivability.test.py (9 cases): escalates ok/stale to warn on exhausted, leaves ok state untouched, self-heals, silent on missing/unreadable file, never fires when the port itself is down, age reporting, unparseable-timestamp fallback.
• Existing tests/health-check-credential-proxy-staleness.test.py (9 cases) still passes — also fixed a latent test-isolation gap it had (didn't patch WORKSPACE_DIR, so my new read path would have read this host's real quota-state.json through it).
• bash scripts/review-checks.sh clean (hardcoded-paths + root-artifacts + prose-cap).
• Live python3 src/health-check.py run: credential-proxy ok port 7846 — no crash, no false alarm (this host's real credential_state is currently ok).

$ python3 tests/health-check-credential-proxy-drivability.test.py
Ran 9 tests in 0.007s
OK

$ python3 tests/health-check-credential-proxy-staleness.test.py
Ran 9 tests in 0.014s
OK

$ git diff --cached | bash scripts/review-checks.sh
prose-cap: PASS (comment blocks within cap 2, exts .py)
review-checks: PASS (hardcoded-paths + root-artifacts + prose-cap clean)

🤖 Generated with Claude Code

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 90%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

• src/health-check.py (100%)

Summary

• Total: 30 lines
• Missing: 0 lines
• Coverage: 100%

[qingyun-wu]

Reviewed at e93598f303, including skills/quota-tracker/scripts/credential-proxy.ts at the same head, since the check reads what that file writes. The approach is right: it reuses the proxy's own recorded credential state rather than adding a probe. The field names (credential_state, credential_state_detail, credential_state_at) and the state/quota-state.json path match recordCredentialState() (credential-proxy.ts:722-731). Two things to fix before I'd approve.

1. False warn after a proxy restart (should fix).

• recordCredentialState is change-only (if (prev.credential_state === state) return;, line 725), and nothing resets the field when the proxy starts (lines 787-795).
• So once the proxy is restarted after an exhausted wedge (the usual repair), the file keeps exhausted until the first request succeeds. Every health check in between reports the new, healthy proxy as wedged.
• test_down_proxy_never_consults_the_file covers "down", but not "restarted and listening".
• Suggested fix: ignore an exhausted whose credential_state_at is older than the current proxy process's start time. check_credential_proxy already reads process start times via _proc_lstarts("credential-proxy"). Add a test: exhausted at T, proxy started at T+1, status stays ok.

2. The docstring breaks the repo's comment rule (should fix).

• CLAUDE.md: "Code comments: at most 2 lines, and only what the code cannot state itself ... no incident history". The new function carries a 20-line docstring that includes the 2026-09-27 incident. The test module's docstring repeats it.
• The incident narrative belongs in the PR body (it's already there). In the code, keep the one constraint a reader needs: "advisory; never marks a non-listening proxy down; silent on read failure".

Minor (non-blocking): the local from datetime import datetime as _dt with a comment explaining why it isn't at module scope. If datetime really can't be imported at module scope here, fine; otherwise hoist it and drop the comment.

The tests I read pin the right cases (escalate ok/stale, self-heal, missing or unreadable file silent, down never consults). I did not run them; CI is green at this head.

[qingyun-wu]

Re-reviewed at f16c27fc.

Fix 1 (restart false-warn): correct. _proc_lstarts returns (start timestamps, …), so at_ts < max(starts) means "the record predates the newest running proxy", and that case now returns early. A probe failure gives [], which falls through to warn, matching the new no-process-info test.

Fix 2 (comments): the incident history is gone, thanks.

One wording fix, then I approve. The docstring now says the opposite of the code: "Escalate … when … 'exhausted' and predates its current process incarnation". The code escalates only when the record does not predate the process, and ignores it when it does. Suggested wording: "…is 'exhausted' and was recorded by the current process (a record older than the process start is ignored)." The docstring is also still 5 lines against the 2-line rule; that one is a nit, but the inversion should be fixed, because a reader will trust it over the code.

CI was still running at this head when I looked (11 passing, 14 pending).

[github-actions]

@cla-assistant check

[qingyun-wu]

Approve at fb753b21. The only change since my last review (f16c27fc) is the docstring, which now matches the code: a record older than the running proxy's start is ignored. The logic was already verified at f16c27fc: restart false-warn fixed, probe failure still warns, 3 tests pin both. 3 checks were still pending when I looked; merge once they are green.

[john-the-dev]

Approve at fb753b21.

What I checked

• Field names and path match the writer. recordCredentialState() (credential-proxy.ts:722-731) writes credential_state, credential_state_detail and credential_state_at into statusPath('quota-state.json'). updateQuotaState() (:749-753) copies all three fields forward when it replaces the file, so a header write cannot erase an exhausted record before this check sees it.
• Tests pass at this head. I ran them in a clean worktree after clearing __pycache__. The drivability suite passes 12/12; the PR body says 9, but the restart-guard cases were added later. The staleness suite passes 9/9.
• The restart guard works against a real process, not just mocks. _proc_lstarts("credential-proxy") found this host's running proxy, started 2026-09-28T13:36:49Z. An exhausted record stamped an hour before that start left the status ok. The same record stamped 60s ago raised warn. The two timestamps compare correctly: lstart is parsed as naive local time and credential_state_at as aware UTC, and both become epoch seconds through .timestamp().
• The hardcoded-path, root-artifact and comment-length checks all pass on the full diff (gh pr diff 4874 | bash scripts/review-checks.sh).

Non-blocking

• The restart guard only recognises a proxy process that belongs to this checkout. _proc_lstarts keeps a process only if its command line or working directory is inside REPO_DIR (_filter_pids_this_checkout). A proxy launched from outside the checkout, such as a packaged runtime/services build, would give starts == []. The guard would then be skipped, and a proxy restarted after an exhausted wedge would read as warn until its first successful request. That's the same false warn qingyun's first review flagged, just on a different install layout. I didn't hit it on this host, where the proxy runs from <checkout>/dist/credential-proxy.js. The failure is a false warn, which is the safer direction, so it's fine to follow up separately.
• The PR body's test count is out of date (9 vs 12).

3 CI lanes were still running when I submitted: shell serial tail, and python standalone 2/5 and 3/5. The other 23 had passed.