Preserve Codex cost usage window under byte pressure - #2825
Open
Whiteknight07 wants to merge 1 commit into
Open
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
Whiteknight07
marked this pull request as ready for review
August 10, 2026 00:30
clawsweeper
Bot
added
merge-risk: 🚨 compatibility
|
Codex review: needs real behavior proof before merge. Reviewed August 12, 2026, 11:59 AM ET / 15:59 UTC. ClawSweeper reviewWhat this changesThis PR makes Codex cost-cache byte-budget cleanup retain files needed by the requested Spend Dashboard window, updates SQLite-store regression tests, and adds a release-note entry. Regression provenancePossible regression — probable (reproduction; reviewed change). No predecessor PR is attributed. Merge readiness⛔ Blocked until real behavior proof from a real setup is added - 7 items remain Keep open. The retention fix addresses a current-main data-loss path, but the branch has a concrete changelog merge conflict, lacks current-head real-cache proof, and changes a storage policy that VISION.md requires an explicit sign-off for. Priority: P1 Review scores
Verification
How this fits togetherCodexBar scans local Codex rollout files into a SQLite cost cache and reads that cache for Spend Dashboard reports. Post-save retention trims the cache by row and byte budgets before those reports are rendered. flowchart LR
A[Codex rollout files] --> B[Cost cache scan]
B --> C[SQLite cost cache]
C --> D[Retention budgets]
D --> E{Outside report window?}
E -->|Yes| F[Prune safe file]
E -->|No| G[Retain report data]
C --> H[Spend Dashboard]
Decision needed
Why: VISION.md requires sign-off for data-storage behavior changes, and the two outcomes intentionally trade disk use against report fidelity. Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Land a rebased retention fix after explicit approval of the fidelity-first storage policy, with one accurate entry under the current unreleased version and redacted real-cache evidence. Do we have a high-confidence way to reproduce the issue? Yes. Current main passes Is this the best way to solve the issue? Conditionally yes: protecting the requested window is a narrow repair for the demonstrated loss, but the cache-growth tradeoff requires the storage-policy sign-off before it is the accepted solution. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against c4ed34d0e44a. LabelsLabel justifications:
EvidenceWhat 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
HistoryReview history (5 earlier review cycles)
|
Author
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
proof: sufficient
Co-authored-by: Whiteknight07 <fiestyboxer@gmail.com>
steipete
force-pushed
the
agent/preserve-cost-usage-window
branch
from
August 10, 2026 08:20
502d123 to
166d5e0
Compare
clawsweeper
Bot
added
rating: 🦪 silver shellfish
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Root cause
The row-budget path protected the requested reporting window, but the byte-budget path explicitly disabled that protection. Deleting a file cascaded into
file_day_aggregates, so rebuilding global aggregates permanently removed older requested days from Spend Dashboard.The initial PR patch correctly enabled requested-window protection, but then fell back to
stripOldestRebuildableDetail. That reset protected files to incomplete, deleted their token/row/accumulator state, and marked catch-up pending. The next refresh would reconstruct the same detail, exceed the same cap, strip it again, and potentially repeat indefinitely.The maintainer revision uses a stable policy: delete only safe out-of-window completed files. Once every remaining file is protected, stop enforcement without mutating scan state. The current report and fork baselines remain authoritative, catch-up does not restart, and the database may remain above its best-effort byte cap.
Storage-policy decision
VISION.mdrequires explicit sign-off for data-storage behavior changes. This PR deliberately prioritizes requested-window report fidelity and stable refresh behavior over an absolute 256 MiB SQLite cap.Recommendation: approve the fidelity-first policy. A hard cap cannot be maintained for arbitrarily large requested windows without either deleting displayed history or introducing a larger durable aggregate-only storage design.
Validation
CODEXBAR_SUPPRESS_TEST_KEYCHAIN_ACCESS=1 swift test --filter CostUsageStoreTests— 60 tests passedCODEXBAR_SUPPRESS_TEST_KEYCHAIN_ACCESS=1 swift test --filter CostUsagePerformanceGateTests— 25 tests passedmake check— passedCODEXBAR_SUPPRESS_TEST_KEYCHAIN_ACCESS=1 make test— 834 selections in 70 groups passed; zero failures or retriesCostUsageStore/SQLite path and proves two consecutive over-cap enforcement passes retain in-window rows and report aggregates without setting catch-up pendingFixes #2823