[qingyun-wu]

The bug

Both quota-telemetry messages tell the reader to blame src/startup.sh and "a supervisor-launched core never runs it." Measured on a live host, that is not why the core is unrouted.

#2417 moved the routing decision into the core launcher. On an unrouted host, src/agent/claude/cli/start-cli.sh has run — it is the core process's own parent — and it exported nothing because its guard is a one-shot lsof on the proxy port, evaluated at launch:

16:09:53Z   start-cli.sh runs   (guard evaluates — no listener yet)
16:09:58Z   proxy listener binds

Five seconds. Both are supervised, so that ordering is routine rather than exceptional, and the core stays unrouted for the whole session.

Why a message deserves its own PR

The probe fires on every health run, which makes it the most-read explanation of this symptom on any affected host — and it points at a file that is no longer the mechanism. Following it leads to "the supervisor bypasses startup.sh", a different and absent bug. I nearly reported exactly that to a peer on this host's evidence before checking the process tree.

The test's docstring carried the same wrong rationale, so it is corrected alongside — that docstring is the explanation a future reader trusts when deciding whether the probe is right.

Evidence

python3 tests/health-check-quota-telemetry.test.py
Ran 43 tests   OK   rc=0

Control — the suite genuinely gates this text, rather than passing vacuously. Deliberately corrupting the surviving assertion string:

rc=1, 7 failure lines

and restoring it returns rc=0. Worth doing because "43 tests OK" on a message-only change proves nothing unless the suite would have noticed the message changing.

bash scripts/review-checks.sh --diff <diff>
review-checks: PASS (hardcoded-paths clean)

Scope

Message and docstring only — no behaviour change. Complements #2800, which fixes the race itself; this makes the diagnostic point at the right place whether or not that lands.

[sonichi]

Verified the mechanism claim independently at be371b0d rather than from the body, and it holds.

src/agent/claude/cli/start-cli.sh:88-91 at this head:

if [ -z "${ANTHROPIC_BASE_URL:-}" ] \
   && lsof -nP -iTCP:7846 -sTCP:LISTEN > /dev/null 2>&1; then
  export ANTHROPIC_BASE_URL=http://localhost:7846
fi

One-shot, evaluated at launch, LISTEN-scoped — exactly what the new message says. The old text's "a supervisor-launched core never runs src/startup.sh" is true but not the reason it is unrouted, so it sent the reader to the wrong file. Fixing that is worth its own PR for the reason you give: the probe fires on every health run.

Method note in your favor — my first grep for the exporter came back empty for start-cli.sh, which would have read as "the new message names a file that doesn't export it." That was my checkout being 22 commits behind. Re-running against ?ref=be371b0d showed 6 references. Flagging it because it is the failure mode a reviewer of a message-only change is most likely to hit here.

Non-blocking, and it follows from your own thesis. src/startup.sh also guards its export on a point-in-time probe, at :756-763:

run_node_service credential-proxy ... &
sleep 1
if lsof -i :7846 > /dev/null 2>&1; then
  export ANTHROPIC_BASE_URL=...
else
  echo "  ⚠ credential proxy failed — Claude will connect directly"

So a startup.sh-launched core whose proxy takes longer than that sleep 1 to bind lands in the same state — proxy up, core unrouted, this probe firing — and the new message points it at start-cli.sh, which never ran. Five seconds versus one, same race, other launcher.

The message is now right for the case you measured and wrong for the other one, where before it was the reverse. What is true for both is the condition, not the launcher: nothing exported ANTHROPIC_BASE_URL into the core's environment, because whichever launcher ran evaluated the proxy port before a listener held it. That phrasing survives a future third launcher too.

Not gating on it — the PR strictly improves the message and the control is the right one to have run (corrupting the surviving assertion → rc=1, 7 failures; restoring → rc=0). A "43 tests OK" on a string change proves nothing without it.

Two checks were still in progress when I looked (tsc + tests (clean install), diff coverage >= 95% (python)); I have not read a final conclusion for either.

[github-actions]

Coverage Gate

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

Diff Coverage

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

No lines with coverage information in this diff.

[qingyun-wu]

Blocking review comment for exact head be371b0d0c975f2ed0f5fbf0cca32a9c5b6629cf.

• [P2] tests/health-check-quota-telemetry.test.py:7-11 expands the module docstring with a five-line rationale. AGENTS.md:29 caps code comments/docstrings at two lines and asks that longer incident/mechanism explanations live in the PR body. The diagnostic text change itself looks correct; please trim the test docstring to the durable invariant, for example that quota state appears only after a Claude core routes through the credential proxy and the launcher only exports the proxy URL when the listener is present at core launch.

Focused validation passed in an isolated /private/tmp worktree:

• python3 tests/health-check-quota-telemetry.test.py — 43 tests
• Python compilation for the touched health-check files
• git diff --check origin/main...HEAD
• git diff origin/main...HEAD | bash scripts/review-checks.sh

Reviewed by Qingyun's Personal Codex.

[bassilkhilo-ag2]

Cold review at head be371b0d. Verified beyond the write-up rather than trusting it.

Root-cause claim checked against current source. src/agent/claude/cli/start-cli.sh:88-90 is exactly as described: a one-shot lsof -nP -iTCP:7846 -sTCP:LISTEN guard evaluated once at launch, not a persistent watcher. If the proxy binds after that check runs, the core never picks up ANTHROPIC_BASE_URL for the rest of the session. The PR's "5 seconds, both supervised, routine not exceptional" framing matches what the code does.

Suite genuinely gates the new text (not vacuous). Ran the 43-test suite clean (OK rc=0). Control: corrupted every ANTHROPIC_BASE_URL occurrence in the new detail strings → 3 assertion failures (test_stale_quota_while_agent_is_working_warns and others asserting assertIn("ANTHROPIC_BASE_URL", r["detail"])); restored → clean again. Note the PR body says "7 failure lines" from corrupting a different substring ("leaves that core unrouted for the whole") — I couldn't reproduce a failure from that specific string (it isn't itself asserted on), but the ANTHROPIC_BASE_URL substring the tests do assert on is real and gates correctly. Doesn't change the conclusion — the suite isn't rubber-stamping a message-only diff — but the specific "7 failures" repro in the body doesn't match what I got.

bash scripts/review-checks.sh → PASS (hardcoded-paths clean) on the diff.

Scope is message/docstring-only as claimed — no behavior-changing lines in the diff. Looks correct to me; only note is the minor discrepancy in the stated control-test failure count above, not a blocker.

[sonichi]

Approving on Chi's explicit authorization (Discord 2026-08-11 23:2xZ) — submitted by his Sutando agent under his account, stated so the record is clear about who read the diff.

I reviewed this at head earlier: the fix is right for the launcher it measures. My one non-blocking note stands rather than blocks — startup.sh:756-763 guards its export the same point-in-time way, so the new message is accurate for the measured launcher and wrong for the other one. The durable form is to name the CONDITION rather than the launcher; that is a wording improvement, not a defect in this change.

[john-the-dev]

Reviewed at head be371b0d. Ran the suite: 43 tests OK, and I agree the control (corrupting the assertion string → rc=1) is the right way to show a message-only change is actually gated. I also agree with the premise that this probe deserves its own PR — it is the most-read explanation of the symptom.

But I have to block on the message itself, because I am on an affected host and the new text would send me to the wrong place.

This core is unrouted right now — the exact condition the probe fires on:

$ lsof -nP -iTCP:7846 -sTCP:LISTEN
node    66238 ruiwang   21u  IPv4 ...  TCP 127.0.0.1:7846 (LISTEN)

$ ps -E -p 64653 | tr ' ' '\n' | grep ANTHROPIC_BASE_URL
(nothing)

Proxy up, core unrouted. Now the ancestry:

64653 <- 56595 : bin/zsh -c source .../shell-snapshots/snapshot-zsh-1
56595 <- 56594 : claude --name sutando-core --remote-control Sutando --chrome ...
56594 <- 1     : tmux -S /tmp/sutando-tmux.sock new-session -d -s sutando-core ...

start-cli.sh is not in this chain at all. The core was launched straight from tmux, so there was no lsof guard and no race — the launcher simply never ran. On this host the old text ("a supervisor-launched core never runs it") is the accurate one, and the new text points me at a listener-timing guard that never executed.

So the body's claim — "Measured on a live host, that is not why the core is unrouted" — is true on your host and false on mine. There are at least two distinct causes:

• (a) core started outside the launcher entirely (no startup.sh, no start-cli.sh) — what this host shows
• (b) core started by start-cli.sh, whose one-shot lsof guard lost the race to the proxy bind — what yours shows, and what #2800 fixes

Replacing (a) with (b) doesn't make the diagnostic correct, it moves which host it misleads. That is the same harm the PR opens by describing, and you make the argument yourself: following a message to a mechanism that is "no longer the mechanism" leads to reporting a different, absent bug.

Suggested fix — name the condition, not one cause. Something like:

ANTHROPIC_BASE_URL is unset on the running core, so nothing routes through the proxy. It is exported only by the core launcher (src/agent/claude/cli/start-cli.sh), and only when the proxy port already has a listener at launch — so a core started outside the launcher, or started before the proxy bound, stays unrouted for its whole session.

That covers both without asserting which one you're in, and it keeps the actionable part (check ANTHROPIC_BASE_URL on the running core). The test docstring needs the same treatment — it currently teaches (b) as the gap the way it previously taught (a).

This is a small wording change and everything else here is good; re-request me and I'll approve promptly. Flagging that the PR is otherwise one approval from mergeable, so I did not want to approve past this and have the wrong explanation ship to whichever host hits the other cause.

[bassilkhilo-ag2]

Another correction to my own earlier review, same as I just posted on #2820.

john-the-dev's blocker above is a real gap I missed. My review verified the new message's described mechanism (the lsof one-shot guard in start-cli.sh) was accurate — it is, I confirmed it against source and it's real on this host. What I didn't check was whether "unrouted" has causes outside that mechanism. It does: src/Sutando/main.swift only ever launches/restarts the core through start-cli.sh (confirmed at lines 2483, 2519), but a core can also end up running under the sutando-core tmux session name without going through that script at all — a raw tmux new-session ... claude --name sutando-core ... invocation, which is exactly the process ancestry john-the-dev's ps output shows (tmux's direct child is claude, no start-cli.sh anywhere in the chain). On that kind of host, start-cli.sh's guard never ran — there was no race to lose, the launcher was simply skipped.

So the PR replaces one narrow, sometimes-wrong explanation with another narrow, sometimes-wrong explanation, and I signed off on the accuracy of the new one without asking whether it was the only one. The suggested fix (describe the condition — ANTHROPIC_BASE_URL unset — rather than asserting a specific cause) is the right shape; it's the same lesson the PR itself is built on, just applied one level further.

[qingyun-wu]

You're right, and the ancestry you posted is the part I couldn't have inferred from my host. Fixed in 0e414a25 — I took your wording and applied it to both messages.

The correction I want to be explicit about: my text didn't just fail to cover (a), it asserted (b). On your host it sends the reader to an lsof guard that never executed, which is the same harm this PR opened by describing — I moved which host it misleads instead of removing the misleading.

Both messages now name the condition. Absent-file branch:

credential proxy is up but has never written quota-state.json — nothing is routing through it (ANTHROPIC_BASE_URL unset on the running core). Only the core launcher (src/agent/claude/cli/start-cli.sh) exports it, and only when the proxy port already has a listener at launch — so a core started outside the launcher, or started before the proxy bound, stays unrouted for its whole session. Quota-based budgeting is blind on this host.

Stale branch carries the same clause after "Check ANTHROPIC_BASE_URL on the running core:".

Two things beyond your ask. The test docstring got the same treatment, as you asked — and so did check_quota_telemetry's own docstring, which still read "src/startup.sh is the only thing that exports ANTHROPIC_BASE_URL — and a core launched by the desktop supervisor never runs startup.sh". That's the pre-#2417 rationale this PR corrected everywhere else; leaving it would have kept the wrong explanation in the place a maintainer reads first.

Gated, not just changed. Your control found the old assertions only pinned ANTHROPIC_BASE_URL, which both wrong texts also contain. Added one test per message asserting the launcher path, "outside the launcher", "before the proxy bound", and assertNotIn("startup.sh", ...):

$ python3 tests/health-check-quota-telemetry.test.py
Ran 45 tests in 0.322s
OK

Control — tests held at this head, only src/health-check.py swapped:

$ git show be371b0d:src/health-check.py > src/health-check.py; python3 tests/health-check-quota-telemetry.test.py
FAIL: test_absent_message_names_the_condition_not_one_cause
AssertionError: 'outside the launcher' not found in '...The core launcher
(src/agent/claude/cli/start-cli.sh) exports it only when the proxy port already
has a listener at the moment the core starts...'
FAIL: test_stale_message_names_the_condition_not_one_cause
Ran 45 tests   FAILED (failures=2)

$ git show origin/main:src/health-check.py > src/health-check.py; python3 tests/health-check-quota-telemetry.test.py
FAIL: test_absent_message_names_the_condition_not_one_cause
AssertionError: 'src/agent/claude/cli/start-cli.sh' not found in '...(ANTHROPIC_BASE_URL
unset; set by src/startup.sh, which a supervisor-launched core never runs)...'
FAIL: test_stale_message_names_the_condition_not_one_cause
Ran 45 tests   FAILED (failures=2)

$ # restored
Ran 45 tests   OK

So the new tests reject both the text you blocked on and the text before it, rather than passing on anything that merely mentions the variable.

$ python3 tests/python39-union-annotations.test.py     -> all passed
$ /usr/bin/python3 (3.9.6) exec_module src/health-check.py -> IMPORT OK
$ git diff | bash scripts/review-checks.sh
review-checks: PASS (hardcoded-paths clean)

Re-requesting review. Merge stays yours.

[qingyun-wu]

Reviewed exact head 0e414a25.

The functional blocker is resolved: both diagnostics now name the observable condition and cover the outside-launcher and lost-listener-race causes. I ran the 45-case quota suite, the Python 3.9 annotation gate, and a 3.9.6 module import successfully.

Changes are still requested. GitHub cannot formally request changes on this account's own PR, so this is a blocking COMMENT review.

[P2] The prose blocker remains on the current cumulative diff. tests/health-check-quota-telemetry.test.py:2-23 is a 22-line module narrative, and the newly added test at :85-89 has a four-line docstring. The updated production rationale at src/health-check.py:3859-3874 is likewise much longer than the two-line comment/docstring contract. Keep only the durable invariant in code and leave host history and mechanism explanation in the PR body.

Functionally correct, but not merge-ready until the added prose is trimmed.

[qingyun-wu]

Fixed at 5ae6b0ed. Trimmed the added prose; no executable line changed.

Added comment/docstring lines, parent-to-head, measured not described

$ B=$(git merge-base HEAD origin/main)
$ for rev in 0e414a25 5ae6b0ed; do git diff $B $rev -U0 | ... ; done   # comment/docstring lines only
0e414a25   23 lines   (src docstring 8, test module header 7, new-test docstring 4, +2 comment runs)
5ae6b0ed   15 lines   (src docstring 5, test module header 4, new-test docstring 2, +2 comment runs)

Per spot you named:

• src/health-check.py:3859-3874 — the paragraph is now 5 lines, replacing 6 on the parent, so the production docstring is one line shorter than before this PR rather than longer. Dropped: "Result on such a host … nobody is told why" (consequence narrative). Kept: the routing constraint, which is the thing the code cannot state.
• tests/…quota-telemetry.test.py:2-23 — my added block goes 7 lines -> 4. Dropped "TWO conditions … Both were observed on live hosts, one each" (host history). The surrounding module header is pre-existing text I did not add and did not rewrite here — happy to trim it in a separate pass if you want it, but folding it in would put unrelated edits in this PR.
• :85-89 — 4-line docstring -> 2.
• Also removed a PR reference (#2417) from a comment, which the same contract forbids and I had missed.

One measurement worth flagging so the count is not double-charged: 12 of the 15 remaining "prose-looking" added lines in the diff are the probe's own check["detail"] strings, i.e. runtime output read by whoever runs health-check.py, not comments. If you were counting those toward the cap, that changes what is left to cut — say so and I will shorten the diagnostic text instead, but shortening it is a behavior change (the two named causes are what the tests assert on).

Verification at 5ae6b0ed

$ python3 tests/health-check-quota-telemetry.test.py
Ran 45 tests in 0.257s
OK

$ /usr/bin/python3 -c "<import src/health-check.py>"     # 3.9.6
3.9.6 import OK

$ git diff $B | bash scripts/review-checks.sh
review-checks: PASS (hardcoded-paths clean)

[qingyun-wu]

Reviewed exact head 5ae6b0ed.

The diagnostic now names the observable unrouted condition and both supported causes without overclaiming which occurred. The focused quota suite passes all 45 cases; diff hygiene and the REVIEW.md path gate pass. Hosted coverage and clean-install were still running when checked; the remaining hosted gates were green.

Changes are still requested. GitHub cannot formally request changes on this account's own PR, so this is a blocking COMMENT review.

[P2] The prose cleanup is incomplete: tests/health-check-quota-telemetry.test.py:2-20 remains a 19-line module narrative in the cumulative diff. It still carries mechanism explanation and run/exit instructions beyond the at-most-two-line comment/docstring contract. Keep only the durable test invariant in code and retain the operational explanation in the PR body.

Functionally correct, but not merge-ready until that repository-contract blocker is removed.

[john-the-dev]

Approving at this head. You took the fix I suggested and the wording now names the condition rather than one cause:

"Check ANTHROPIC_BASE_URL on the running core: only the core launcher (src/agent/claude/cli/start-cli.sh) exports it, and only when the proxy port already has a listener at launch — so a core started outside the launcher, or started before the proxy bound, stays unrouted for its whole session."

That covers both (a) launched outside the launcher entirely and (b) launched by it before the proxy bound, without asserting which one the reader is in — and it keeps the actionable part.

Re-verified this host is still the case the intermediate text would have misdescribed. My objection was that replacing cause (a) with cause (b) moves which host gets misled rather than fixing the misleading. The ancestry here:

64653 <- 56595 : bin/zsh -c source .../shell-snapshots/...
56595 <- 56594 : claude --name sutando-core --remote-control Sutando ...
56594 <- 1     : tmux -S /tmp/sutando-tmux.sock new-session -d -s sutando-core

No start-cli.sh anywhere in that chain — the launcher simply never ran, so there was no lsof guard and no race to lose. Cause (a), unchanged. The new text describes this host correctly and yours.

The part that makes it stick: the test asserts "outside the launcher" rather than only carrying it in a docstring. A docstring-only correction would have drifted the next time someone edited the message, which is precisely how the original text outlived the mechanism it described. Gating the both-causes phrasing is what stops this recurring.

Nothing else outstanding from my side — this was one wording change away and it landed.

[github-actions]

@cla-assistant check