Skip to content

Preserve spend totals while local history catches up - #3051

Open
mauriciopolvora wants to merge 3 commits into
steipete:mainfrom
mauriciopolvora:fix-retained-spend-catch-up
Open

Preserve spend totals while local history catches up#3051
mauriciopolvora wants to merge 3 commits into
steipete:mainfrom
mauriciopolvora:fix-retained-spend-catch-up

Conversation

@mauriciopolvora

@mauriciopolvora mauriciopolvora commented Aug 18, 2026

Copy link
Copy Markdown

Summary

  • snapshot the last established local-cost report before a routine bounded refresh can enter catch-up
  • keep same-account, same-window established totals visible in the menu and Spend Dashboard while the new scan is incomplete
  • bind each retained dashboard input to the effective bucket time zone and snapshot history window
  • persist the retained payload's requested report window instead of relabeling it with wider cache bounds
  • replace the retained value after convergence, while continuing to drop it for account changes, scope changes, invalidation, or confirmed-empty history

Root cause

An incomplete bounded refresh is a successful scan result, not a hard failure. It correctly carries historyCoverageIsEstablished == false, and the Spend Dashboard correctly excludes such rows from totals. However, that successful incomplete result replaced the previous established snapshot, so Codex disappeared from Usage & Spend while Cursor remained visible.

The existing retained-history path covered scanner failures, but not successful exact-to-pending transitions during routine refreshes.

Safety invariants

  • The live scanner and CLI still report coverage as incomplete while catch-up is pending.
  • A retained report must match the requested date window, time zone, and session-root fingerprint.
  • Dashboard retention additionally requires the same effective bucket time zone and snapshot historyDays as the incomplete replacement.
  • A retained report records the bounds used to build its payload, not the potentially wider shared-cache bounds.
  • In-memory retention is limited to the current provider-config revision and cost-history scope.
  • Invalidated and confirmed-empty sources are never retained.
  • No external helper, network access, Keychain access, or real account data is added.

After-fix behavior proof

The runtime trace was captured on base fix commit d3cf7c8ab19c5ffe66658525bdfdf36493d8b3a3 against a copy-on-write, isolated mirror of the real local Codex session corpus. No synthetic session rows were used. A temporary uncommitted CLI harness only selected the isolated cache root and invoked the app's existing two-second bounded-refresh mode; it was removed before validation.

Amounts, token counts, account identity, paths, session IDs, and prompt contents are redacted. The trace records only state and equality checks:

baseline  coverage_established=true  spend_present=true  tokens_present=true  error_present=false
pending   coverage_established=false spend_present=true  retained_tokens_unchanged=true
          catch_up_pending=true       retained_report_present=true              error_present=false
converged coverage_established=true  spend_present=true  spend_replaced=true  tokens_replaced=true
          catch_up_pending=false      retained_report_present=false             error_present=false

This demonstrates that established spend remains present while the new scan is explicitly pending/stale, and that the newly converged report replaces it and clears the retained marker. The subsequent review commits only tighten request-scope matching and retained-report metadata; their current-head regressions are listed below.

Review follow-up

  • retained dashboard inputs now carry per-source request provenance
  • successful incomplete Codex results can reuse an established input only when source ID, effective bucket time zone, and history window all match
  • dashboard regressions cover both a UTC-to-Pacific request change and a 30-day-to-full-scan window change
  • the retained-report initializer now requires explicit payload bounds, preventing a narrow report from inheriting wider cache bounds
  • the scanner regression establishes a 365-day cache, enters pending catch-up through a 30-day refresh, verifies the retained payload contains only the requested window, and rejects it for a later 365-day request

Validation

  • rebased onto current main; regenerated parser hash a8843ee5c69a90fc
  • XCODE_DEFAULT_TOOLCHAIN_OVERRIDE=/Library/Developer/CommandLineTools make check — passed, 0 violations
  • swift build --product CodexBarCLI — passed on current head 4a60d7f4d9674e2e02e17002ea6387160f118af0
  • focused Swift Testing run for CostUsageFetcherCacheSnapshotTests|SpendDashboardCachedPresentationTests|UsageStoreCodexCostCatchUpTests — 37 tests passed in 3 suites
  • isolated rerun of the store writer-lock test after parallel-suite contention — passed
  • make test — attempted, but this host has Command Line Tools only and the unmodified runner cannot discover tests without Xcode's Testing module; the focused suites were run with local framework paths and pull-request CI uses the repository's Xcode matrix

The regression fixture proves an exact scan, an appended active tail, a bounded pending refresh, an unchanged retained total, and an eventual complete replacement. Additional tests cover dashboard presentation, cross-scope rejection, and retained-payload bounds.

Refs #2815, #2628, #2520, #2918.

@clawsweeper

clawsweeper Bot commented Aug 18, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

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

@mauriciopolvora
mauriciopolvora force-pushed the fix-retained-spend-catch-up branch from a597df3 to fa7c088 Compare August 18, 2026 15:44
@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: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Aug 18, 2026
@clawsweeper

clawsweeper Bot commented Aug 18, 2026

Copy link
Copy Markdown

Codex review: needs changes before merge. Reviewed August 19, 2026, 6:10 AM ET / 10:10 UTC.

ClawSweeper review

What this changes

This PR retains established Codex spend totals in the menu and Spend Dashboard while a bounded local-history refresh is incomplete, with account, window, and time-zone scope checks.

Merge readiness

⚠️ Needs maintainer review before merge - 3 items remain

Needs one compatibility repair before merge: the regenerated parser hash rebuilds existing local cost caches, temporarily removing the totals this PR aims to preserve. Likely related people: steipete (recent catch-up work) and NickGuAI (scan-completeness contract).

Priority: P1
Reviewed head: 4a60d7f4d9674e2e02e17002ea6387160f118af0

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) Strong real behavior proof and focused tests are offset by an upgrade-time cache-loss compatibility defect.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (live_output): The PR supplies a redacted isolated real-history trace for the core retained-total behavior; its later scope and metadata changes are backed by focused current-head regressions.
Patch quality 🦐 gold shrimp (3/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Verified Sufficient (live_output): The PR supplies a redacted isolated real-history trace for the core retained-total behavior; its later scope and metadata changes are backed by focused current-head regressions.
Evidence reviewed 6 items Scope guards address the prior findings: Dashboard retention now requires a matching source ID, bucket time zone, and snapshot history window, and excludes invalidated or confirmed-empty sources.
Focused regression coverage: Current-head tests cover retaining a successful incomplete refresh and rejecting retained totals across both bucket-time-zone and history-window changes.
Cache transition deletes existing data: The changed parser hash is a8843ee5c69a90fc, while the compatible predecessor list does not include the prior 8050a4faf4fddb96 hash shown by the PR diff. An incompatible store triggers rebuildDatabase, which deletes the SQLite database, WAL, and SHM files.
Findings 1 actionable finding [P1] Preserve the existing cost cache across the parser-hash transition
Security None None.

Live Verification

Command: swift run CodexBarCLI cost --help

Result: FAIL (failed) — execution before step 1 run: sh -lc pnpm install --ignore-scripts --frozen-lockfile failed: ! Corepack is about to download https://registry.npmjs.org/pnpm/-/pnpm-11.22.0.tgz

sh -lc pnpm install --ignore-scripts --frozen-lockfile failed: ! Corepack is about to download https://registry.npmjs.org/pnpm/-/pnpm-11.22.0.tgz

Assertions:

  • FAIL expect_output: codexbar cost [--format text|json]

How this fits together

CodexBar scans local Codex session history into cached cost snapshots, then publishes them to the menu and Spend Dashboard. The changed paths decide whether an incomplete refresh replaces a prior established snapshot or keeps it visible until convergence.

flowchart LR
A[Local Codex session history] --> B[Bounded cost scanner]
B --> C{Coverage established?}
C -->|Yes| D[Store and publish new totals]
C -->|No| E[Validate retained snapshot scope]
E --> F[Menu and Spend Dashboard]
D --> F
Loading

Before merge

  • Preserve the existing cost cache across the parser-hash transition (P1) - The new hash is not an allowed predecessor, so users with the prior 8050a4faf4fddb96 cache take the incompatible-schema path and rebuildDatabase deletes their cached rows. That makes totals disappear again until a potentially long rescan completes; retain the parsed cache while clearing or migrating the old retained-report payload, and cover that upgrade path.
  • Resolve merge risk (P1) - Without a narrow upgrade migration, users upgrading from the prior parser hash lose their derived local cost cache and can again see no totals until the full history scan catches up.
  • Complete next step (P2) - A narrow, testable compatibility repair can preserve existing parsed cache rows without changing the retained-total product direction.

Findings

  • [P1] Preserve the existing cost cache across the parser-hash transition — Sources/CodexBarCore/Generated/CodexParserHash.generated.swift:4
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production versus test delta production +76, tests +276 The patch has substantial targeted regression coverage across scanner, publication, and dashboard layers.
Affected surface 12 files changed The change crosses persistence, scanner, in-memory publication, dashboard presentation, and tests.

Merge-risk options

Maintainer options:

  1. Preserve cached history on upgrade (recommended)
    Add a narrow parser-hash transition that retains valid parsed cost rows but drops the old retained-report metadata, with an upgrade regression test.
Copy recommended automerge instruction
@clawsweeper automerge

Special instructions:
Preserve existing cost-cache rows across the 8050a4faf4fddb96 to a8843ee5c69a90fc transition, clear persisted previous-report metadata, and add an upgrade regression test.

Technical review

Best possible solution:

Adopt the prior parsed cache rows while clearing only the stale retained-report payload, then preserve scope-matched established totals during catch-up.

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

Yes—the supplied isolated real-history trace exercises established, pending, and converged states, and current-head tests cover the scope boundaries; this review did not execute tests because the checkout is read-only.

Is this the best way to solve the issue?

No—the retained-total implementation is sound, but it must preserve prior parsed cache data while invalidating only stale retained-report metadata on upgrade.

Full review comments:

  • [P1] Preserve the existing cost cache across the parser-hash transition — Sources/CodexBarCore/Generated/CodexParserHash.generated.swift:4
    The new hash is not an allowed predecessor, so users with the prior 8050a4faf4fddb96 cache take the incompatible-schema path and rebuildDatabase deletes their cached rows. That makes totals disappear again until a potentially long rescan completes; retain the parsed cache while clearing or migrating the old retained-report payload, and cover that upgrade path.
    Confidence: 0.96

Overall correctness: patch is incorrect
Overall confidence: 0.94

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • add P1: The parser-hash transition deletes existing local cost history, causing a user-visible total-loss regression during rebuild.
  • remove P2: Current review triage priority is P1, so this older priority label is no longer current.

Label justifications:

  • P1: The parser-hash transition deletes existing local cost history, causing a user-visible total-loss regression during rebuild.
  • merge-risk: 🚨 compatibility: The PR changes persisted cache compatibility and currently forces an upgrade-time cache rebuild.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦞 diamond lobster and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (live_output): The PR supplies a redacted isolated real-history trace for the core retained-total behavior; its later scope and metadata changes are backed by focused current-head regressions.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR supplies a redacted isolated real-history trace for the core retained-total behavior; its later scope and metadata changes are backed by focused current-head regressions.

Evidence

Acceptance criteria:

  • [P1] swift test --filter CostUsageStoreTests.
  • [P1] make check.
  • [P1] make test.

What I checked:

  • Scope guards address the prior findings: Dashboard retention now requires a matching source ID, bucket time zone, and snapshot history window, and excludes invalidated or confirmed-empty sources. (Sources/CodexBar/SpendDashboardController.swift:1285, 4a60d7f4d967)
  • Focused regression coverage: Current-head tests cover retaining a successful incomplete refresh and rejecting retained totals across both bucket-time-zone and history-window changes. (Tests/CodexBarTests/SpendDashboardCachedPresentationTests.swift:111, 4a60d7f4d967)
  • Cache transition deletes existing data: The changed parser hash is a8843ee5c69a90fc, while the compatible predecessor list does not include the prior 8050a4faf4fddb96 hash shown by the PR diff. An incompatible store triggers rebuildDatabase, which deletes the SQLite database, WAL, and SHM files. (Sources/CodexBarCore/Generated/CodexParserHash.generated.swift:4, 4a60d7f4d967)
  • Persisted retained reports are separately recoverable metadata: The store serializes previousReportPayload independently from the parsed cache rows, so an upgrade path can clear that stale retained report while preserving valid session aggregates. (Sources/CodexBarCore/Vendored/CostUsage/CostUsageStore+CodexCache.swift:293, 4a60d7f4d967)
  • Real behavior proof: The PR body includes a redacted isolated real-history trace showing established totals remain visible during pending coverage and are replaced after convergence; supplied screenshots also show the related retained-total menu state. (d3cf7c8ab19c)
  • History inspection limitation: The checkout contains the current PR-head snapshot but no parent, current-main, or earlier-review commit objects, so local ancestry and release provenance could not be inspected. (4a60d7f4d967)

Likely related people:

  • steipete: Merged the related cached-spend and catch-up cluster work that established this dashboard and scanner behavior. (role: recent adjacent implementation owner; confidence: high; commits: 698c332a250f, 7ef8993aa03f; files: Sources/CodexBar/SpendDashboardController.swift, Sources/CodexBarCore/Vendored/CostUsage/CostUsageScanner.swift)
  • NickGuAI: The merged completeness work established historyCoverageIsEstablished, which this PR uses to distinguish pending from established totals. (role: introduced scan-completeness contract; confidence: high; commits: 8c37c20a43d6; files: Sources/CodexBarCore/CostUsageFetcher.swift, Sources/CodexBarCore/Vendored/CostUsage/CostUsageScanner.swift)

Rank-up moves

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

  • Preserve valid prior parsed cache rows while clearing the stale retained-report payload.
  • Add an upgrade regression from the prior parser hash and run the focused store suite plus repository checks.

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.

History

Review history (2 earlier review cycles)
  • reviewed 2026-08-18T15:48:48.508Z sha fa7c088 :: needs real behavior proof before merge. :: none
  • reviewed 2026-08-19T09:39:21.955Z sha d3cf7c8 :: needs changes before merge. :: [P1] Guard retained totals by dashboard request scope

@mauriciopolvora
mauriciopolvora force-pushed the fix-retained-spend-catch-up branch from fa7c088 to d3cf7c8 Compare August 19, 2026 09:34
@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Aug 19, 2026
@mauriciopolvora
mauriciopolvora marked this pull request as ready for review August 19, 2026 09:45

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d3cf7c8ab1

ℹ️ 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".

Comment on lines +5182 to 5184
let sourceCache: CostUsageCache? = if plan.shouldRefresh,
!currentScanIsPending,
!cache.days.isEmpty

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Record the requested window in retained reports

When a shared Codex cache already spans 365 days but a routine 30-day refresh becomes bounded and pending, buildCodexReportFromCache creates a 30-day report while CostUsageCodexPreviousReport(report:cache:) labels it with the cache's wider scanSinceKey/scanUntilKey. A later 365-day dashboard or hydration request therefore passes matches, receives only the 30-day payload, and can treat it as established, hiding older spend until catch-up completes. Persist the actual requested report window rather than the cache's retained scan window.

Useful? React with 👍 / 👎.

Comment on lines +1258 to +1263
if !incompleteCodexSourceIDs.isEmpty {
let retainedInputs = self.loadedInputs.filter {
incompleteCodexSourceIDs.contains($0.id) &&
!unsafeSourceIDs.contains($0.id) &&
$0.provider == .codex &&
$0.snapshot.historyCoverageIsEstablished

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid retaining snapshots across bucket-time-zone changes

When the user changes bucketTimeZoneIdentifier while the replacement Codex scan reports incomplete coverage, this filter retains the prior established input solely by source ID. Bucket time zone is not part of sameSourceOwnership or unsafeSourceIDs, so the old snapshot's day keys are then rebuilt using the new calendar, misassigning boundary usage and selected-day totals until catch-up finishes. Retention should also require the previous and requested bucket-calendar scopes to match.

Useful? React with 👍 / 👎.

@clawsweeper clawsweeper Bot added P1 Urgent regression or broken agent/channel workflow affecting real users now. and removed P2 Normal priority bug or improvement with limited blast radius. labels Aug 19, 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. P1 Urgent regression or broken agent/channel workflow affecting real users now. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant