fix(usage): render checked stored-account limits - #4635
Conversation
647f818 to
9a0407c
Compare
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Maintainer review — reconstructed head 9a0407c
Verdict: APPROVE (Yeachan-Heo, maintainer; independent of PR author).
What this head is
The original head 647f818 became unmergeable after dev advanced to 6696988 (CHANGELOG [Unreleased] conflict only). Reconstructed by cherry-pick onto current dev: identical 3-file delta (+129/−21) — src/session/account-inventory.ts, new test/account-inventory-usage.test.ts, CHANGELOG entry — with authorship preserved (asdfqwerzxcc, original author date). Pushed with force-with-lease from 647f818. Source diff digest (canonical git diff --binary --full-index --no-ext-diff 6696988b...9a0407c0b7): 191aac363891f6616f0a01fc0ca07d500d4d9b7cccc3be70c41fe6237099da07.
Review findings (consensus run pr4635-usage-limits)
- Root cause confirmed in source: probe writes usage cache under a key embedding the resolved provider base URL; pre-fix inventory re-read used a baseUrl-less key (
"default"bucket) → limits dropped after a successful check, and cache-only/usagemissed the same bucket. - Fix is structurally sound: direct render of the (already
raw-stripped) probe report through theredactUsageReportallowlist removes the readback-key assumption; cache-only reads forwardmodelRegistry.getProviderBaseUrl(provider)(optional-chained; absent registry keeps legacy"default"key behavior byte-identical). - Redaction/privacy holds: redacted report ⊆ {provider, fetchedAt, limits[], metadata allowlist (email/accountId/account/user/projectId/orgId)}; no
raw; no credential material inAccountInventoryRow(type-level contract). - Non-blocking follow-ups recorded (not blocking this fix):
freshUsageCache15-min presentation freshness vs the cache tier's 5-min TTL (cosmetic, self-healing); thereport ⇒ okupstream invariant is correct but unencoded locally.
Bounded validation on this head
bun test packages/coding-agent/test/account-inventory-usage.test.ts— 2 pass (hermetic env; see note)bun test packages/ai/test/auth-storage-usage-cache.test.ts packages/ai/test/auth-storage-check-credentials.test.ts— 13 passbun test packages/coding-agent/test/usage-report-columns.test.ts packages/coding-agent/test/status-line-usage.test.ts— 9 passbun --cwd=packages/coding-agent run check— clean (biome 2846 files + tsc --noEmit)gjc accounts list --jsonfrom source — ok- Canonical binaries: dev-tip 6696988 →
66f481b8…, head 9a0407c →2db53013…; binary diff SHA-2565756dc5778db9eee67898053affe743c7aa87f031a6eadcebd72faa2608825c4, size delta +0.
Environment note (for the record)
On the maintainer workstation the new test fails only because ~/.bashrc exports OPENAI_API_KEY: providerSet then (correctly) adds the env-backed openai synthetic row, whose sourceHealth path isn't stubbed by the test's minimal AuthStorage. $credentialEnv intentionally reads home shell rc files (dev #4613 made dotenv parsing stricter; this is operator-env contamination, not a code defect). Not reproducible in CI (clean runner) — confirmed by env -i rerun: 2 pass.
9a0407c to
b760154
Compare
Fresh exact-head review requested — head
|
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Fresh exact-head review — APPROVE on b76015491b2546fb33e953df2f7e665e295d77d9
Reviewer: Yeachan-Heo (maintainer; independent of PR author asdfqwerzxcc). This review supersedes the dismissed approval on 9a0407c0b7 — nothing from that head was reused except verification method; every check below was re-run on this exact head.
Verification on b76015491b (base 44d7b6ee07)
- Code delta byte-identical to original commit
647f8188(git diff 647f8188 b76015491b -- packages/coding-agent/src/session/account-inventory.ts packages/coding-agent/test/account-inventory-usage.test.ts→ empty); only CHANGELOG[Unreleased]placement resolved. - Hermetic
bun test: 24 pass / 0 fail acrossaccount-inventory-usage,auth-storage-usage-cache,auth-storage-check-credentials,usage-report-columns,status-line-usage. bun --cwd=packages/coding-agent run checkclean (biome 2846 files +tsc --noEmit);git diff --checkclean.- Canonical binary diff SHA-256
ec817eb851fd5a1ae91370e9fd2b44eefe4e18c28f9e139cdff4d7f93c5bdceb(dev2c4dd412…vs head012bbb94…).
Review basis (consensus run pr4635-usage-limits, re-checked on this head)
- Root cause confirmed: probe writes usage cache keyed by resolved provider base URL; pre-fix read used the
"default"bucket → limits dropped. Fix renders theraw-stripped probe report directly through theredactUsageReportallowlist and forwardsgetProviderBaseUrlon cache-only reads. - Renderer compatibility re-verified (
usage-report.ts,accounts-cli.ts):row.usageshape unchanged; optional-chained consumers tolerate the now-populated usage. - Non-blocking follow-ups recorded elsewhere:
freshUsageCache15-min freshness label vs 5-min cache TTL (cosmetic); localreport ⇒ okguard.
APPROVE for merge to dev on CI green.
Successful credential probes returned usage limits, but the account inventory re-read them with a cache key that omitted the resolved provider base URL. Render explicit probe results directly and align cache-only reads with the provider URL so both usage command modes retain their limits. Lore-id: usage-limit-render-4634 Constraint: usage reports must remain redacted before entering presentation rows Rejected: force a second provider request | duplicates a successful probe Confidence: high Scope-risk: narrow Reversibility: clean-revert Tested: account inventory cache-key and direct probe report regression tests; coding-agent typecheck
b760154 to
39c89f5
Compare
Fresh exact-head review requested — head
|
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Fresh exact-head review — APPROVE on 39c89f5f4ff612eac538fcb46c78de8b5767d832
Reviewer: Yeachan-Heo (maintainer; independent of PR author asdfqwerzxcc). Supersedes the two dismissed approvals (9a0407c0b7, b76015491b) — both targeted heads whose bases were superseded by dev merges (#4610/#4624, then #4616); nothing from them is reused. All checks below were re-run on this exact head.
Verification on 39c89f5f4f (base 7265a61c8e)
- PR code/test files byte-identical to the original author commit
647f8188(diff over those paths is empty); only CHANGELOG[Unreleased]sibling ordering resolved (no entries dropped). - No overlap with #4616 (disjoint files:
session-manager.tsvsaccount-inventory.ts). - Hermetic
bun test: 24 pass / 0 fail (account-inventory-usage,auth-storage-usage-cache,auth-storage-check-credentials,usage-report-columns,status-line-usage). changelog-history-guard12 pass;verify-gjc-state-writers --failclean;bun --cwd=packages/coding-agent run checkclean;git diff --checkclean.- Source three-dot digest
da3c71200f1de3bacf53ca795f4fcc699717466b65aaf915386a09c1ae4a959a; three-dot binary diff SHA-2560f4844426360430d18a325d67ad64d2132e8135ddc160203c4707d0e9f0865e1(dev8921e02b…vs head260fd5c1…).
Review basis (re-verified on this head)
- Root cause confirmed: the probe writes the usage cache keyed by the resolved provider base URL; the pre-fix inventory re-read used the
"default"bucket, dropping limits after a successful check and on cache-only reads. - Fix contract intact and scoped:
/usage checkrenders the successful probe report directly (through theredactUsageReportallowlist,rawalready stripped upstream); cache-only reads forwardgetProviderBaseUrl. No auth/provider routing semantics changed. - Non-blocking follow-ups recorded:
freshUsageCache15-min presentation freshness label vs the cache tier's 5-min TTL (cosmetic, self-healing); optional localreport ⇒ okguard.
APPROVE for squash-merge to dev once contract + product CI are green on this head.
What
/usage checkrenders the successful stored-credential probe report directly, so provider limit windows (e.g.7 days: 24.00% used (76.0% left)) no longer vanish for healthy stored OAuth accounts./usagereads now pass each provider's resolved base URL into the stored-usage lookup, aligning the read key with the key the probe wrote (baseUrlis part of the usage-report cache key).packages/coding-agent/test/account-inventory-usage.test.ts).Closes #4634
Why
AuthStorage.checkCredentialswrites the usage cache under a key embedding the resolved provider base URL, but the account inventory re-read it with a baseUrl-less key that always hit the"default"bucket — so a successful probe's limits were dropped at render time. Rendering the probe report directly removes the readback-key assumption entirely, and forwarding the registry-resolved base URL fixes the cache-only mode the same way. No auth/provider routing semantics change beyond this bug fix.Rebase note
dev advanced to
7265a61c8ed489b6a9461ba7e991198e46c25208(#4616) after the prior reconstruction on44d7b6ee07; headb76015491b, its approval, contract success, and CI were stale and were discarded (approvals formally dismissed; nothing merged). Reconstructed by cherry-pick of the original commit647f8188onto7265a61c: PR code and test files are byte-identical to the original commit (git diff 647f8188 39c89f5f -- <PR files>is empty); the only resolution is CHANGELOG[Unreleased]sibling ordering (all entries kept, none dropped). Overlap with #4616 is none — it touchessession-manager.ts, this PR touchesaccount-inventory.ts. Authorship preserved:asdfqwerzxcc <wowls7990@gmail.com>, author date 2026-08-17.Testing
On reconstructed head
39c89f5f4ff612eac538fcb46c78de8b5767d832(base7265a61c8e):bun test:account-inventory-usage(2),auth-storage-usage-cache(7),auth-storage-check-credentials(6),usage-report-columns(3),status-line-usage(6) — 24 pass / 0 failbun test ./scripts/changelog-history-guard.test.ts— 12 passbun --cwd=packages/coding-agent run check— clean (biome 2847 files +tsc --noEmit)bun scripts/verify-gjc-state-writers.ts --fail— clean (0 write sites outside sanctioned writer)git diff --check 7265a61c 39c89f5f— clean7265a61c→8921e02b…, head39c89f5f→260fd5c1…; three-dot binary diff SHA-2560f4844426360430d18a325d67ad64d2132e8135ddc160203c4707d0e9f0865e1(size delta +0)git diff --binary --full-index --no-ext-diff 7265a61c...39c89f5f):da3c71200f1de3bacf53ca795f4fcc699717466b65aaf915386a09c1ae4a959aOPENAI_API_KEY(providerSetthen exercises the env-backed synthetic-row path against the test's minimalAuthStoragestub, which lackspeekCachedCredentialHealthForSource). This is operator-env contamination of the test environment, not a product hermeticity defect — clean shells and CI are unaffected (confirmed byenv -irerun); no product change made for it.GJC verdict
devbun checkpasses (coding-agent package check on reconstructed head)—
[repo owner's gaebal-gajae (clawdbot) 🦞]