perf: reduce idle cost scan work - #2848
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e90838497
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if !options.forceRescan, | ||
| let priorMemo, | ||
| priorMemo.sourceInventory == sourceInventory, | ||
| priorMemo.reportKey == reportKey | ||
| { |
There was a problem hiding this comment.
Recheck debounce before returning memoized Claude reports
When the first call in a process happens inside the scanner debounce window after a Claude transcript changed, priorMemo is nil so the code below can build a report from the old disk cache and then store it against the current sourceInventory. Subsequent calls in long-lived CLI/serve processes then match this early-return guard and never reach the nowMs - cache.lastScanUnixMs > refreshMs check, so cost output can stay stale until a force rescan, cache rewrite, or another source change occurs; avoid installing/returning a memo for a debounced, unrefreshed cache whose source inventory has not been proven current.
Useful? React with 👍 / 👎.
|
Codex review: needs changes before merge. Reviewed August 10, 2026, 2:15 PM ET / 18:15 UTC. ClawSweeper reviewWhat this changesThis PR memoizes unchanged Claude cost reports, reuses persisted per-file cache rows, reduces automatic Codex catch-up duty cycles, and limits automatic local-history scans to provider refreshes. Merge readinessKeep open: the performance approach is useful and profiling is substantial, but the new Claude memo can permanently return stale local-cost data after a debounced cold-process scan. Priority: P2 Review scores
Verification
How this fits togetherCodexBar refreshes provider status and local token-cost history, then publishes the resulting usage to its menu and widget snapshots. The changed path inventories Claude transcript files, decides whether cached cost data is current, and returns a report to those consumers. flowchart LR
A[Scheduled provider refresh] --> B[Cost-history eligibility]
B --> C[Claude source inventory]
C --> D{Memo matches?}
D -->|yes| E[Return cached cost report]
D -->|no| F[Read or update persisted cache]
F --> G[Menu and widget snapshots]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep the inventory-based optimization, but store or return a memo only after the cache is proven to represent that inventory; add a regression covering a cold process inside the debounce interval. Do we have a high-confidence way to reproduce the issue? Yes—source-reproducible: modify a Claude transcript, start a fresh process inside a positive refresh debounce interval, then call again without another file change; the first call can store old cache output under the new inventory and later calls return it early. Is this the best way to solve the issue? No—the optimization is appropriate, but memoization must not certify freshness when the underlying disk cache was deliberately left unrefreshed. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 38c5a516f579. LabelsLabel changes:
Label justifications:
EvidenceAcceptance criteria:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
Summary
Profiling proof
Tests
make checkpassed.make test: 836/836 selections across 70 groups passed.No screenshots: this change has no UI impact.