Align cost cache save overshoot with documented load cap - #2703
Conversation
![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: 8563c2b313
ℹ️ 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".
|
|
||
| enum CodexParserHash { | ||
| static let value = "37aedd661c4272a8" | ||
| static let value = "c7188afe30460be4" |
There was a problem hiding this comment.
When upgrading from the immediately preceding revision, existing Codex caches carry codex:cu:p37aedd661c4272a8, but this generated change makes the expected key codex:cu:pc7188afe30460be4 while compatibleCodexProducerKeys still contains only p843ca061c36bbea1. Consequently both load and loadCodexForMigration reject every cache from the prior version and initiate a potentially lengthy bounded rebuild, even though this commit only changes save-budget documentation and uses the existing Gregorian day-key parser when constructing a catch-up report rather than changing persisted parsing totals or layout. Add the prior producer key to the compatibility set when rotating this hash.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Codex review: needs real behavior proof before merge. Reviewed August 6, 2026, 5:10 AM ET / 09:10 UTC. ClawSweeper reviewWhat this changesThis PR corrects Codex cost-cache compatibility and calendar handling, documents allowed save overshoot, and adds focused cache regression tests. Regression provenancePossible regression — probable (reviewed change; reproduction). No predecessor PR is attributed. Merge readiness⛔ Blocked until real behavior proof from a real setup is added - 3 items remain The prior producer-key blocker is resolved and the patch appears correct, but external-PR policy still requires after-fix proof from an existing cache upgrade before merge. Priority: P2 Review scores
Verification
How this fits togetherCodexBar persists parsed Codex session usage in a local cost cache. On refresh, cache size and producer keys determine whether existing usage data is reused or rebuilt. flowchart LR
A[Codex session files] --> B[Cost usage scanner]
B --> C[Local cost cache]
C --> D[Size and producer checks]
D --> E[Usage refresh]
E --> F[Menu bar usage]
Before merge
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Preserve the narrow parser reuse and predecessor allowlist, supported by one redacted real upgrade run that shows an existing Codex cache is accepted after installing this revision. Do we have a high-confidence way to reproduce the issue? Yes, at source level: a persisted predecessor-key cache exercises clear normal and migration load paths, though this read-only review did not run the test. Is this the best way to solve the issue? Yes: reusing the existing normalized day-key parser and retaining compatible producer keys is narrower and safer than a cache schema migration. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 005a71f550cd. 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 (4 earlier review cycles)
|
|
@clawsweeper re-review Addressed the P2: |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
8a24f9f to
fdc02fc
Compare
|
@clawsweeper re-review Rebased onto current main ( |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper re-review Added the real upgrade proof: took the existing local Codex cost cache (11.7 MB, 304 session files, 23 days), stamped it with the current-main producer key, and loaded it with this head via default |
|
🦞👀 Command router queued. I will update this comment with the next step. |
|
Landed. Verified before merge: confirmed the (256 MiB, 320 MiB] overshoot band is reachable only via protected entries and that this PR’s alignment is documentary — the never-persist-refusable invariant from #2646 is untouched and now pinned by a real save/load round trip. The calendar fix is a genuine latent bug (day keys misresolved under non-Gregorian system calendars). Maintainer fix pushed before merge ( |
…r (refs #2760) (#2767) * fix: restore JSON-cache retention semantics lost in the SQLite cutover (refs #2760) Semantics-parity audit of the CostUsageCache -> CostUsageStore migration found four behaviors the cutover dropped: - Retention pruned only the typed discovery columns while the scanner round-trips discovery through the opaque payload, so deleted files resurfaced on the next load; prune now updates both in lockstep and resets cursors/isComplete like the old cache did. - The row budget deleted the oldest files regardless of window; the old entry budget never sacrificed in-window or recently active files (the byte budget remains the only authority that may). - Fork-parent protection ignored the lineage-only dependency key and a stale parent survived when its only referencing child was pruned in the same pass; candidates now honor the dependency key and iterate to a fixpoint. - Out-of-window files with an in-window mtime (active sessions with unscanned rows) were deletable; they are protected again. saveCodexCache budgets are injectable for tests, restoring pins for previous-report preservation across trims and the non-Gregorian catch-up report (#2703), plus strip/compaction coverage. * docs: changelog for the SQLite retention semantics parity fixes (refs #2760)
2 participants
Summary
Follow-up to the review on #2637's merged fix (#2646, commit
90c6faed0). Addresses all points raised in the review comment and in the ClawSweeper re-review:dayDateduplicatedparseDayKeyand skipped calendar normalization — removed the privatedayDatehelper;previousReportForCatchUpnow calls the existingCostUsageScanner.parseDayKey, which normalizes vialocalGregorianCalendar(matching:)and anchors at noon. A day key like"2026-06-01"now parses to 2026 even whenCalendar.currentis non-Gregorian (e.g. Buddhist calendar would previously resolve to 1483).saveovershoot invariant now matches the documented load cap — updated themaxCacheLoadBytesdoc comment and the guard-fail comment so the behavior matches the contract:savebounds artifacts tomaxCacheFileBytes, and when protected entries (resuming sessions, fork parents) cannot be trimmed further it may overshoot only up to the load cap. Anything above the cap is dropped as legacy/foreign.codex load cap keeps headroom over the save budget with a real save load round tripnow exercisessave/loadand asserts the overshoot artifact is written and readable within the load cap. Also addedcatch up report honors a non-gregorian system calendaras a regression guard for theparseDayKeyfix.d3df003a9bff32af, so both the current-main key (codex:cu:p1cd29792d9ca2b11) and the prior bounded-cache key (codex:cu:p37aedd661c4272a8) were added tocompatibleCodexProducerKeys. Upgrading from either preceding build no longer rejects valid local caches or forces a rebuild.current codex cache accepts calendar normalization predecessorscovers both keys through defaultloadandloadCodexForMigrationwith a real save/load round trip.Also regenerated
CodexParserHash.generated.swift(repo requires it after touchingVendored/CostUsage) and updated the hardcoded line anchors inProviderArchitectureGatekeeperTestsfor the shifted source.Real behavior proof (existing local cache upgrade)
Ran an after-fix upgrade check against the existing local Codex cost cache (11.7 MB artifact, 304 session files, 23 days of usage data). The artifact was stamped with the current-main producer key (
codex:cu:p1cd29792d9ca2b11) to model the upgrade source, then loaded with this head's defaultloadandloadCodexForMigration:loadreuses the full artifact: all 304 files and 23 days survive,lastScanUnixMsis preserved — no rebuild.loadalso reuses it (incompatibleCache == nil).foreignKeyRefused=true), confirming the allowlist is what grants reuse rather than unconditional acceptance.Commands run
swift test --filter CostUsageCacheTests— 39/39 passswift test --filter ProviderArchitectureGatekeeperTests— 38/38 passmake test— full suite passesmake check— SwiftFormat/SwiftLint clean (0 violations)Scripts/regenerate-codex-parser-hash.shNotes
Branch rebased onto current
origin/main(005a71f55); PR is mergeable. The calendar reachability fordayDatein the original review was upstream-normalized in the app path (saveCodexCachepassesrange.calendar, already Gregorian); this change removes the latent bug at the API level anyway, matching the review's recommendation. No behavior change for Gregorian-calendar users.