Repository navigation
fix(dashboard): absent quota must not render as 0% behind a green check - #2788
Conversation
get_quota_status() already degrades honestly — a missing quota-state.json
returns {"available": True} with no numbers, specifically so the panel cannot
imply freshness it can't vouch for. The tiles in render_dashboard() undid
that: they formatted utilization with .get("utilization_5h", 0) and chose the
glyph off `available`, so an absent reading rendered as
✓ QUOTA (no data) 0% 5H USED 0% 7D USED
"quota healthy, nothing consumed" — the opposite of the truth, and the same
confidently-wrong class the reader's own docstring warns about for stale data.
Observed on a live host where nothing had written the file all session,
because the core runs without ANTHROPIC_BASE_URL (#2417). The owner read the
tile as both "quota is fine" and "the quota is not showing the right info".
Adds _quota_has_data() — same discriminator the age label already uses — and
renders an em-dash for the glyph and both utilization tiles when no reading
exists. A real reading is untouched.
Regression test asserts the absent case shows no "0%" and, as a control, that
a genuine reading still renders 42% / 13% / ✓.
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 78%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
|
Blocking finding:
Local validation:
Reviewed by Qingyun's Personal Codex. |
bassilkhilo-ag2
left a comment
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
The dashboard fix itself is correct — read the diff, confirmed _quota_has_data() matches the existing _quota_age_label() discriminator, and independently ran the new test: it genuinely fails against the pre-fix dashboard.py (AssertionError: absent quota rendered as "0%") and passes with the fix. The control (test_a_real_reading_still_shows_numbers) also checks out — a genuine reading isn't blanked.
But CI is currently red on this PR — both tsc + tests and diff coverage >= 95% fail — and it's a real bug in the new test, not a fluke: it passed on my machine (macOS) but fails on the Linux CI runner with
FileNotFoundError: [Errno 2] No such file or directory: '/usr/bin/pmset'
_render_with()'s fake() wraps the REAL get_system_stats() and only overrides the returned "quota" key — but get_system_stats() still runs subprocess.run(["/usr/bin/pmset", ...]) as a side effect before returning, and that binary doesn't exist on the Ubuntu CI image. That's also almost certainly why diff coverage is short — the crash happens before any of the new _quota_has_data()-gated lines in render_dashboard() execute under coverage measurement.
The sibling test tests/dashboard-battery-fallback.test.py already solved exactly this: it stubs subprocess.run directly (captured pmset output shapes) rather than letting the real subprocess call through. dashboard-quota-no-data.test.py's _render_with() should do the same — stub subprocess.run (or monkeypatch get_system_stats itself instead of wrapping it) so the test doesn't depend on a macOS-only binary being present.
Requesting changes: make the new test hermetic on Linux CI. No concerns with the actual dashboard.py fix.
Review finding (P1): _render_with() called the real get_system_stats() before replacing only the quota field, and that path shells out to /usr/bin/pmset. On the Ubuntu runner that binary does not exist, so both "tsc + tests (clean install)" and the coverage gate died with FileNotFoundError before any quota assertion ran. It passed locally only because the author's host is macOS. render_dashboard() reads exactly four keys off stats (disk_free, battery, charging, quota), so the harness now builds a complete synthetic dict and never touches the real collector.
|
Fixed at I reproduced your finding before touching anything, by stubbing Fix: And I re-ran the before/after control, because changing a harness is exactly how a test quietly goes vacuous. Reverting So it still fails without the production fix. Worth noting my first attempt at that control was wrong — I used Remote head verified unmoved ( Thanks for the catch — a platform-dependent test that only runs on the author's OS is worse than no test, since it reads as coverage. |
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Changes requested on exact head ed150655132cf8e86daabb707b998413d5e269ff (formal REQUEST_CHANGES is unavailable on this account's own PR).
- [P1]
src/dashboard.py:562no longer preserves a real unavailable quota state. Before this PR the glyph was✓only whenquota["available"]was true and✗otherwise. The new expression uses_quota_has_data()in place ofavailable, so any fresh reading with headers/age renders✓even whenavailable: false(the quota writer setsavailablefalse wheneveranthropic-ratelimit-unified-status != "allowed"). Exact-head synthetic render withavailable=False, nonempty headers,age_h=0.2, andstale=Falseproduced<div class="stat-val">✓</div>for Quota. Please keep the new—only for no-data while preserving✗for a real unavailable reading, and add that state as a regression control.
The Linux-hermetic test fix in ed150655 is verified: the new quota test, quota-staleness suite, battery-fallback suite, compilation, diff check, and hardcoded-path scan all pass. The blocker is the production glyph regression above.
Not merge-ready.
Reviewed by Qingyun's Personal Codex.
bassilkhilo-ag2
left a comment
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Fix commit ed150655 addresses the hermeticity gap exactly — replaced get_system_stats() with a fully synthetic 4-key stats dict, so the test never touches the real macOS-only pmset call at all. Cleaner than the sibling-test pattern I pointed at (that one still calls the real subprocess with a stubbed binary; this removes the OS dependency entirely).
Cloned the new head and ran the test myself: passes. All CI green now, including the two that were failing (tsc + tests, diff coverage >= 95%). The underlying dashboard.py fix was already verified correct in my prior pass. Approving.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Approving at head e4dde733. The fix is exactly right for the P1: _quota_has_data() (headers present OR age_h not None) gates the glyph and both utilization tiles, so an absent reading renders — instead of "✓ 0% used" — absence now looks like absence, not "healthy, nothing consumed." Stale still shows ⚠ + the age label with last-known numbers, which is the right call (you still see the last reading, flagged old).
Test tests/dashboard-quota-no-data.test.py is a real regression, and I note you fixed the vacuous-harness trap yourself — the original shelled to pmset (absent on the Ubuntu runner) and died before asserting, and the git stash "control" ran fixed code silently; git checkout main -- src/dashboard.py is the correct instrument and it does still fail on parent. Good catch on your own harness.
On your adjacent question (should the em-dash also cover the stale case differently): I'd leave it — ⚠ + age + last-known numbers is more useful for stale than blanking to —; — is right only for genuinely-absent. The fs-permission/silent-catch scope is a separate concern, correctly out of this PR.
Merges stay Qingyun's; this is a review, not a merge.
Review P1: the first cut of this fix swapped `available` for `_quota_has_data()` in the glyph, so a fresh reading with `available: False` rendered ✓. The writer sets `available` false whenever `anthropic-ratelimit-unified-status != "allowed"`, so a real rate-limit displayed as healthy — the same confidently-wrong class this PR exists to remove, just pointed at refusal instead of absence. The glyph is a three-way split, now written as one: no reading → —, a refused reading → ✗, a good reading → ✓. Stale still wins with ⚠. Adds the regression control the review asked for. Also anchors the test's tile extractor: it used `(.*?)` with re.S, which starts at the first stat-val on the page and swallows every tile until the label, so each tile appeared to contain every value. `[^<]*` pins it to its own tile.
|
@cla-assistant check |
|
Blocking finding on current head
Focused validation:
Not approving until the unavailable-state glyph is restored. Reviewed by Qingyun's Personal Codex. |
|
You're right, and thank you — that was a real regression, not a style point. The writer sets Pushed {"⚠" if stats["quota"].get("stale")
else ("—" if not _quota_has_data(stats["quota"])
else ("✓" if stats["quota"].get("available", True) else "✗"))}
Regression control added, plus the before/after you'd want: Three fixtures now: no-data ( Neighbours still green: One thing worth flagging, since it's the reason the first control didn't catch this. My tile assertion used r'<div class="stat-val">(.*?)</div><div class="stat-label">' + re.escape(label)Non-greedy doesn't help here — the match starts at the first Ready for re-review. |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Re-approving at 0d977898 — thanks for flagging that my e4dde73 approval had gone stale over a real fix (exactly the "APPROVED masking an open P1" shape worth surfacing; dismiss_stale=false means my old approve rode over the new commit).
Verified the second P1 is fixed: the glyph is now a correct four-way — ⚠ stale → — no-data → ✗ available:false (a real reading the API refused) → ✓ good. Collapsing refused into ✓ was the regression; the split is right, and the utilization tiles stay — only for genuinely-absent (a refused-but-present reading still shows its numbers, which is correct). The UNAVAILABLE test case pins the ✗ path so it can't collapse back. My approval now sits at the current head.
Everything else from my prior review stands. Merge stays Qingyun's.
|
Approval signal for current head The prior blocker is fixed: the Quota tile now keeps the intended three-way split ( Focused validation passed:
Hosted checks are green on this head. Reviewed by Qingyun's Personal Codex. |
What / why
get_quota_status()already degrades honestly: a missingquota-state.jsonreturns{"available": True}with no numbers, specifically so the panel cannot imply freshness it can't vouch for. Its docstring is explicit that a stale reading is "confidently wrong, which is the worse failure".The tiles in
render_dashboard()undid that. They formatted utilization with.get("utilization_5h", 0)and picked the glyph offavailable— which isTruefor a missing file — so an absent reading rendered as:"Quota healthy, nothing consumed" — the opposite of the truth, from the same class of failure the reader was written to avoid.
Found on a live host, where nothing had written the file for an entire session because the core runs without
ANTHROPIC_BASE_URL(#2417). The owner read that tile two ways in the same minute — as "quota is fine", and as "the quota is not showing the right info". Both are the tile's fault.Fix
_quota_has_data()— the same discriminator_quota_age_label()already uses — gates the three tiles. Absent reading → em-dash for the glyph and both utilization values. A real reading is untouched, and⚠/stale behaviour is unchanged.Evidence
The regression test fails on the parent commit and passes on the fix:
The test carries its own control so it can't pass vacuously — a genuine reading must still render numbers:
Existing dashboard tests at
b6b0416:Scope
Presentation-only, single concern. Does not attempt to fix why the file is missing — that's #2417, separate and not mine. This only stops the dashboard lying while it is.