Skip to content

Preserve wider Codex history across dashboard projections - #2915

Closed
thomaschow19 wants to merge 1 commit into
steipete:mainfrom
thomaschow19:codex/preserve-wider-cost-cache
Closed

Preserve wider Codex history across dashboard projections#2915
thomaschow19 wants to merge 1 commit into
steipete:mainfrom
thomaschow19:codex/preserve-wider-cost-cache

Conversation

@thomaschow19

Copy link
Copy Markdown
Contributor

Summary

  • keep retained Codex cache coverage independent of 7- and 30-day Usage & Spend projections
  • protect the union of retained and newly requested scan bounds during byte-budget enforcement
  • cover both identical-content and full-save persistence paths with a synthetic regression

Reproduction

The regression was reproduced against the v0.49.4 tag using a temporary synthetic store:

  • a wider retained window was seeded with one older and one recent complete row
  • 30-day and 7-day dashboard requests each evicted the older row under byte pressure
  • persisted scan metadata narrowed to the dashboard boundary

No real cache, account data, credentials, or logs were used.

Validation

  • CODEXBAR_SUPPRESS_TEST_KEYCHAIN_ACCESS=1 swift test --filter 'narrow dashboard windows preserve wider retained cache under byte pressure'
  • focused 118-test scanner/store group
  • make check
  • make test passed groups 1–61, including all cache/scanner/dashboard groups; group 62 hit an unrelated status-menu shared-state failure. Each half of that group passes independently.

This is distinct from #2864, which changes Finish now scope and progress rather than retention ownership.

Closes #2914
Refs #2823, #2825, #2861

@clawsweeper

clawsweeper Bot commented Aug 13, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Aug 13, 2026
@clawsweeper

clawsweeper Bot commented Aug 13, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed August 13, 2026, 4:34 PM ET / 20:34 UTC.

ClawSweeper review

What this changes

The PR changes Codex cost-cache saving so 7- and 30-day dashboard scans protect the union of the retained cache window and the newly requested scan window during budget enforcement, with regression coverage.

Regression provenance

Possible regression — probable (reproduction; reviewed change). No predecessor PR is attributed.

Merge readiness

Blocked until real behavior proof from a real setup is added - 4 items remain

Keep open for normal review: the branch targets the linked cache-retention bug, but the available evidence is synthetic-test-only and local repository inspection failed before current-main and history verification. Likely related people: Whiteknight07 (medium confidence), based on the merged adjacent retention-policy work.

Priority: P2
Reviewed head: 81b208002789f500290f6fa734c973e1a2adb44b

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The patch is focused and has targeted synthetic coverage, but external real-behavior proof and independent repository verification are still absent.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: The PR body reports synthetic XCTest and check results, but does not attach after-fix evidence from a real persisted-cache setup; add a redacted terminal transcript or equivalent live output, then update the PR body for re-review.
Patch quality 🦐 gold shrimp (3/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The PR body reports synthetic XCTest and check results, but does not attach after-fix evidence from a real persisted-cache setup; add a redacted terminal transcript or equivalent live output, then update the PR body for re-review.
Evidence reviewed 4 items Proposed retention change: The supplied PR diff computes a protection window from the cache and requested scan window, then passes it to both identical-content and full-save budget-enforcement paths.
Regression coverage in the branch: The supplied diff adds a synthetic store test covering both a 30-day identical-content save and a 7-day full save after seeding a wider retained window under byte pressure.
Adjacent merged retention policy: Merged #2825 established a fidelity-first, best-effort storage-cap policy for requested report windows; this PR addresses the distinct retained-window versus projection boundary.
Findings None None.
Security None None.

How this fits together

CodexBar scans local Codex usage into a persisted cost cache, then Usage & Spend requests short dashboard projections from that cache. The save path applies row and byte budgets before the dashboard reads retained daily history.

flowchart LR
A[Codex rollout history] --> B[Cost usage scanner]
B --> C[Persisted cost cache]
D[Dashboard scan request] --> E[Retention-window selection]
C --> E
E --> F[Row and byte budget enforcement]
F --> C
C --> G[Usage and Spend dashboard]
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The PR body reports synthetic XCTest and check results, but does not attach after-fix evidence from a real persisted-cache setup; add a redacted terminal transcript or equivalent live output, then update the PR body for re-review.
  • Resolve merge risk (P1) - Existing installations that previously trimmed older retained history during narrow dashboard saves may now retain more cache data or remain above the best-effort byte cap; upgrade behavior needs explicit confirmation.
  • Resolve merge risk (P1) - The repository sandbox prevented an independent current-main, history, and complete-discussion review.
  • Complete next step (P2) - Hold for ordinary review and contributor-supplied real behavior proof; the remaining need is evidence, not a safe automated repair.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Patch surface production +22/-4, tests +68, release notes +1; 3 files affected The implementation is narrowly concentrated in the cache save path with substantially more regression coverage than production change.

Root-cause cluster

Relationship: fixed_by_candidate
Canonical: #2914
Summary: This PR is a candidate fix for the canonical retained-window projection bug; the older retention and Finish-now reports overlap only partially or are distinct.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge-risk options

Maintainer options:

  1. Prove upgraded-cache behavior before merge (recommended)
    Add redacted after-fix terminal evidence using a pre-existing wider cache and narrow dashboard projections, including retained coverage and resulting cache-budget behavior.
  2. Accept larger best-effort caches
    Explicitly accept that preserving the wider retained window can leave some existing caches above the former effective trimming boundary.

Technical review

Best possible solution:

Preserve the persisted history window as the retention authority, evict only data outside the union window when possible, and document/prove the resulting best-effort cache-size behavior on an upgraded cache.

Do we have a high-confidence way to reproduce the issue?

Unclear for an independent current-main run: the PR describes a concrete synthetic-store path against v0.49.4, but the sandbox prevented verification of the current code and test path.

Is this the best way to solve the issue?

Unclear: protecting the retained/requested union matches the reported persistence boundary and adjacent fidelity-first policy, but upgrade cache-size behavior still needs direct evidence.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against daf02ae7a7a4.

Labels

Label changes:

  • add P2: This is a bounded persisted-history correctness issue in the Usage & Spend cache path, without evidence of an emergency runtime outage.
  • add merge-risk: 🚨 compatibility: The retention-window change can alter how much historical cache existing installations retain after an upgrade.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body reports synthetic XCTest and check results, but does not attach after-fix evidence from a real persisted-cache setup; add a redacted terminal transcript or equivalent live output, then update the PR body for re-review.

Label justifications:

  • P2: This is a bounded persisted-history correctness issue in the Usage & Spend cache path, without evidence of an emergency runtime outage.
  • merge-risk: 🚨 compatibility: The retention-window change can alter how much historical cache existing installations retain after an upgrade.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body reports synthetic XCTest and check results, but does not attach after-fix evidence from a real persisted-cache setup; add a redacted terminal transcript or equivalent live output, then update the PR body for re-review.

Evidence

What I checked:

Likely related people:

  • Whiteknight07: Authored the merged adjacent cache-retention policy change that modified the same Codex cost-usage retention area. (role: recent area contributor; confidence: medium; commits: 597b0f17d1e1; files: Sources/CodexBarCore/Vendored/CostUsage/CostUsageStore+CodexCache.swift)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Attach redacted terminal or live-output evidence that a pre-existing wider cache survives both narrow projection save paths.
  • Show the resulting cache-budget state so reviewers can evaluate the upgrade compatibility tradeoff.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@steipete

Copy link
Copy Markdown
Owner

Merged via #2920 — your fix was used essentially verbatim (rebased onto current main; only the CHANGELOG conflicted), with Co-authored-by credit on the landing commit and changelog thanks. The regression test proved the defect was still live post-#2918: the narrow dashboard save path fed the requested window into budget enforcement, deleting retained rows and rewriting scan metadata. budgetProtectionWindow now takes the union of retained and requested coverage on both save paths.

Closing this draft as superseded — thank you @thomaschow19 for the precise diagnosis and clean fix, the second one of yours to land this week!

@steipete steipete closed this Aug 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Usage & Spend projections can prune wider retained Codex cost history

2 participants