Test Token Activity coverage through the spend dashboard - #2797
Conversation
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Codex review: needs maintainer review before merge. Reviewed August 10, 2026, 1:36 PM ET / 17:36 UTC. ClawSweeper reviewWhat this changesAdds cached Codex token-activity regression coverage through the spend dashboard and a narrow cache-root resolver seam so the test uses the production activity loader. Merge readiness✅ Ready for maintainer review Keep open for normal maintainer review. The refreshed branch contains a focused testability seam and regression coverage, prior actionable findings are addressed, and no new correctness defect was found. Priority: P3 Review scores
Verification
How this fits togetherCodexBar reads cached provider usage data and converts it into dashboard inputs and a dashboard model for the app’s spend UI. The changed path loads Codex token activity from the local cache, preserves coverage information, and supplies it to the model. flowchart LR
A[Codex activity cache] --> B[Usage fetcher]
B --> C[Spend dashboard source]
C --> D[Provider input]
D --> E[Dashboard model]
E --> F[Spend UI]
Before mergeNone. Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Merge the focused regression coverage after ordinary review of the exact refreshed head and its running checks. Do we have a high-confidence way to reproduce the issue? Not applicable: this PR adds coverage rather than reporting a separate user bug. The body includes a terminal transcript of the two new integration tests passing through the default activity-loader path. Is this the best way to solve the issue? Yes. Injecting only the cache-root resolver retains the production activity loader while isolating test storage, which is narrower than replacing the loader in the test. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 4f0ac0680cd2. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (14 earlier review cycles; latest 8 shown)
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
2007d2e to
17c1235
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 17c1235260
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return await CostUsageFetcher.loadCachedCodexTokenActivity( | ||
| now: context.now, | ||
| codexHomePath: context.account.homePath, | ||
| maximumDays: context.historyDays, | ||
| scannerOptions: scannerOptions) |
There was a problem hiding this comment.
If SpendDashboardSource.loadCodexActivity regresses—for example by resolving the wrong cache root or no longer forwarding the account home—these tests still pass because the injected closure bypasses that production method and reads the captured test store directly through scannerOptions. Seed the cache where the source resolves it and invoke the default loader, or expose a cache-root seam, so this actually protects the advertised store-to-source path.
Useful? React with 👍 / 👎.
| #expect(unavailableResult.inputs.first?.tokenActivityCache == nil) | ||
| #expect(unavailableModel.tokenActivity.allSatisfy { $0.totalTokens == nil }) |
There was a problem hiding this comment.
When the source unexpectedly drops or fails the Codex input, both assertions pass vacuously: inputs.first?.tokenActivityCache is nil, and a model built from no inputs has an empty activity array for which allSatisfy is true. Require the input, verify the source did not fail, and assert the expected 365-point series before checking that every point is unavailable.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
proof: sufficient
|
@clawsweeper re-review — the PR body now includes a |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
d82ccbd to
3a159a7
Compare
clawsweeper
Bot
added
proof: sufficient
clawsweeper
Bot
added
the
status: 👀 ready for maintainer look
|
@clawsweeper re-review — the test now exercises the production default SpendDashboardSource activity-loader path (no injected loader), the provider-architecture gatekeeper entries were re-anchored after the seam, and CI is green on 52b9b68. |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
… stored slugs (#1100) Every direct publication for a mixed-case repository name failed deterministically: one side of the pipeline preserved the repository's case in the expected slug while the other lowercased it in the record path, and the Worker's containment checks compared with ===. GitHub repository names are case-insensitive identifiers, so a case mismatch must never reject a publication. ~170 of the ~191 tuple_protocol_invalid dead letters were one mixed-case repository, and items like steipete/CodexBar#2797 were re-reviewed to batch revision 14+ because their publication never persisted -- a full Codex review burned per cycle, failing identically each time. Validation at all seven comparison sites (Worker and client) is now case-insensitive, and -- per review finding -- accepted slugs are canonicalized to lowercase before durable writes, so storage keeps exactly one namespace and an accepted uppercase path cannot fork the canonical or export tables. Error messages preserve the original input case. After deploy, the existing dead-letter reconciler re-runs the affected items (all fresh_recovery.eligible) and they publish; the re-review loop stops because publications finally persist. Proof: docs/proof/mixed-case-publication-slugs drives the real Worker and Durable Object over HTTP: a mixed-case plan with lowercase paths is accepted, served from the lowercase namespace with no uppercase namespace rows, a truly-foreign path is still rejected with the detailed 400, and lowercase repos behave identically. Verified on the reviewed head in a Docker-backed Crabbox local-container.
|
@clawsweeper re-review The branch was updated onto current |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Merged as Proof:
No live app proof was needed: this is regression-test coverage plus a narrow internal cache-root injection seam, with runtime defaults unchanged. |
2 participants
Summary
CostUsageStore-> cachedCodextoken activity ->SpendDashboardSource->ProviderInput->SpendDashboardModelThe synthetic shared store covers 2026-04-06 through 2026-04-08 with token totals 18, 0, and 24. The dashboard model must keep only those three established days in its 365-day activity series, leave the day before coverage unavailable, and retain the established zero gap. A second case verifies that an established empty range produces zeros while an unavailable range remains entirely unavailable, and its unavailable-case assertions are non-vacuous: they require a present source input, no failed sources, and the full 365-point activity series before asserting every point is unavailable.
The test now drives the production default activity-loader path: it calls
SpendDashboardSource.load(_:cacheRootResolver:codexSnapshotLoader:), which uses the productionloadCodexActivityimplementation, soSpendDashboardSourceitself passescontext.cacheRootandcontext.account.homePathtoCostUsageFetcher(cacheRoot:)->loadCachedCodexTokenActivity(now:codexHomePath:maximumDays:). The test injects only the narrowest testability seam (a cache-root resolver for filesystem isolation); it no longer injects a fullcodexActivityLoaderor reimplements the production fetcher call.Production changes are limited to dependency-injection plumbing: the existing
loadoverloads now forward through a private implementation that accepts aCodexCacheRootResolver, and the public entry points keep the same default resolver (codexCacheRoot(for:)), so runtime default behavior is unchanged (+32/-2 inSpendDashboardController.swift).Duplicate check
steipete/CodexBarmainatc94fe8d8(no conflicts; the 7 new upstream commits do not touch this PR's files)CostUsageFetcherCacheSnapshotTestsexists, but nothing exercises the fullCostUsageStore-> cached Codex activity ->SpendDashboardSource->ProviderInput->SpendDashboardModelpath)Validation
swift test --filter SpendDashboardTokenActivityIntegrationTests— 2 tests passed (real output below)swift test --filter SpendDashboard— 128 tests in 12 suites passed, covering the refactoredloadoverload family (SpendDashboardControllerTests,SpendDashboardSourceConcurrencyTests,SpendDashboardForceStateMachineTests, ...)swift test --filter ProviderArchitectureGatekeeperTests— 38 tests passed, including the line-anchoredSpendDashboardControllerprovider-construct entries re-anchored after the loader seam--stricton changed files — 0 violationsgit diff --check— cleanmake check— repo-wide JS/lint tool steps (oxfmt/oxlint/typescript) could not be downloaded inside the local sandbox because its network is unavailable; all portable steps that ran passed (locales, parser hash, provider manifests), and the same checks are green in CI (lint job)make test(Scripts/ci_swift_test_by_suite.py, 835 selections / 70 groups) — all groups pass except the Alibaba token-plan cookie-cache tests, which fail only inside the local Codex sandbox withEPERM("Operation not permitted") when locking the real user cookie-cache path; reproduced standalone on the pre-change base commit, so it is environmental and unrelated to this changeReal behavior proof
On PR head
52b9b68a(rebased onto latestmainc94fe8d8), the focused integration tests actually execute and pass through the production default Codex activity loader:PR diff vs
main: 3 files changed, 267 insertions(+), 11 deletions(-) — new test file (226 insertions), the minimal DI seam inSpendDashboardController.swift(+32/-2, runtime default behavior unchanged), and the re-anchored provider-architecture gatekeeper entries (9 lines updated) that keep the line-anchored suite valid after the seam moved the anchors.