Skip to content

Fix Finish now history catch-up - #2864

Closed
thomaschow19 wants to merge 1 commit into
steipete:mainfrom
thomaschow19:codex/fix-finish-now-history-window
Closed

Fix Finish now history catch-up#2864
thomaschow19 wants to merge 1 commit into
steipete:mainfrom
thomaschow19:codex/fix-finish-now-history-window

Conversation

@thomaschow19

Copy link
Copy Markdown
Contributor

Summary

  • Make Usage & Spend Finish now accelerate the effective history window: max(30, configured history days).
  • Restart the active dashboard catch-up when that effective history scope changes.
  • Expand the opaque progress fingerprint to cover durable file, buffered-retry, discovery, and active-lookback cursor metadata.

Root cause

The dashboard catch-up context always passed 30 days, even when the shared cache represented a wider configured history window. Its no-progress check also fingerprinted only file-parsing fields, so persisted discovery, lookback, or buffered-retry cursor advancement could be mistaken for a stall.

Implementation

  • Forward arbitrary configured windows such as 123, 248, and 365 days unchanged while preserving the dashboard's 30-day floor.
  • Include the effective history window in catch-up task identity and current-context validation.
  • Fingerprint every persisted cursor that can keep catch-up pending.
  • Return only an opaque in-process digest; raw path and session identifier values are not returned or logged.
  • Leave automatic duty cycles, partial-result presentation, pricing, and scanner architecture unchanged.

User impact

Finish now now accelerates the full effective configured-history scope. Catch-up continues while durable discovery/lookback/retry cursors advance, while a genuinely unchanged pass still stops safely.

Validation

  • Focused dashboard catch-up and cost-fetcher tests: 9 tests passed.
  • Provider architecture gate: 38 tests passed.
  • git diff --check
  • make check
  • make test: 840 selections across 70 groups, 0 failures and 0 retries.
  • All validation used synthetic caches and test stores; no live provider or Keychain probes were run.

Fixes #2861

Refs #2815, #2848

@clawsweeper

clawsweeper Bot commented Aug 11, 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 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 11, 2026
@clawsweeper

clawsweeper Bot commented Aug 11, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed August 11, 2026, 11:21 PM ET / August 12, 2026, 03:21 UTC.

ClawSweeper review

What this changes

The PR makes Usage & Spend’s Finish now use the larger of 30 days and the configured history window, and expands Codex catch-up progress tracking to persisted cursor state.

Merge readiness

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

Keep this draft open. The effective-window fix addresses a real current-main mismatch, but the owner has explicitly asked not to merge it independently of the persisted-inventory decision in the companion work, and the added digest restores unbounded per-pass traversal.

Priority: P2
Reviewed head: 185c516a2b19e31790d1c77ab4d0fee30091b3f9

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The PR identifies a real defect, but it has a P1 bounded-work regression and lacks real behavior proof.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: The reported validation uses synthetic caches and test stores; it does not show an after-fix configured-history dashboard run in a real setup. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🦪 silver shellfish (2/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: The reported validation uses synthetic caches and test stores; it does not show an after-fix configured-history dashboard run in a real setup. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 5 items Current-main behavior remains affected: The dashboard catch-up context still passes the fixed 30-day dashboard window, while the primary Codex catch-up uses the configured history setting.
Unbounded digest work: The proposed progress key sorts and hashes complete session mappings and pending identifier collections; the sibling lookback helper also sorts every retained pending path.
Digest runs after each bounded pass: The dashboard calls the catch-up advance operation in its loop and compares the resulting progress key before scheduling the next pass.
Findings 1 actionable finding [P1] Avoid corpus-wide progress hashing after each bounded pass
Security None None.

How this fits together

Usage & Spend starts an accelerated local Codex-history scanner and reads its shared cache for progress and costs. Persisted scanner cursors determine whether bounded catch-up continues or is treated as stalled.

flowchart LR
A[History window setting] --> B[Finish now action]
B --> C[Catch-up context]
C --> D[Bounded Codex scanner]
D --> E[Persisted scan cache]
E --> F[Progress decision]
F --> G[Dashboard status and costs]
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: The reported validation uses synthetic caches and test stores; it does not show an after-fix configured-history dashboard run in a real setup. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Avoid corpus-wide progress hashing after each bounded pass (P1) - This was present at the prior reviewed head, but the owner’s bounded-pass note makes the impact clear: sorting every session map and pending-ID queue here runs after each catch-up pass, so a bounded scanner can still do corpus-sized synchronous work before it can report progress. Replace this with scanner-owned durable progress metadata that advances without traversing the collections.
  • Resolve merge risk (P1) - Hashing every persisted discovery mapping and lookback queue after a bounded scanner pass can reintroduce corpus-sized synchronous work on large histories.
  • Resolve merge risk (P1) - The draft has no after-fix real dashboard run, and its setting-change test bypasses the visible dashboard lifecycle.
  • Resolve merge risk (P1) - The owner has identified an unresolved integration dependency on Fix Codex usage history indexing and catch-up completion #2849.
  • Complete next step (P2) - The owner has already requested that this draft remain open pending the companion scanner ownership decision; the remaining work is an integration choice, not a safe autonomous repair.
  • Improve patch quality - Wait for the persisted-inventory ownership decision in the companion PR.
  • Improve patch quality - Use scanner-owned durable progress rather than hashing all persisted cursor collections.
  • Improve patch quality - After integration, add redacted proof that a visible dashboard safely replaces work after a history-window change.

Findings

  • [P1] Avoid corpus-wide progress hashing after each bounded pass — Sources/CodexBarCore/CostUsageFetcher.swift:393-401
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production versus test delta production +109/-11, tests +202/-13 Most of the patch is regression coverage, but the production digest change runs in the catch-up loop.

Merge-risk options

Maintainer options:

  1. Integrate after scanner ownership settles (recommended)
    Retain the effective-window behavior but rebase it onto the approved persisted-inventory and durable-progress contract.
  2. Pause this draft
    Keep the PR open without additional cursor-contract churn until the companion scanner decision is complete.

Technical review

Best possible solution:

After the companion scanner contract is decided, preserve the effective history-window behavior but obtain progress from a scanner-owned durable-progress signal and validate the actual settings-to-dashboard lifecycle.

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

Yes—current source proves that dashboard catch-up uses 30 days while the primary catch-up uses the configured window. The reported real setup was not rerun during this read-only review.

Is this the best way to solve the issue?

No—the effective-window idea is correct, but the current digest is not the safest implementation because it performs whole-collection traversal and does not align with the approved companion scanner contract.

Full review comments:

  • [P1] Avoid corpus-wide progress hashing after each bounded pass — Sources/CodexBarCore/CostUsageFetcher.swift:393-401
    This was present at the prior reviewed head, but the owner’s bounded-pass note makes the impact clear: sorting every session map and pending-ID queue here runs after each catch-up pass, so a bounded scanner can still do corpus-sized synchronous work before it can report progress. Replace this with scanner-owned durable progress metadata that advances without traversing the collections.
    Confidence: 0.98
    Late finding: first raised on code an earlier review cycle already covered.

Overall correctness: patch is incorrect
Overall confidence: 0.96

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • add merge-risk: 🚨 compatibility: The patch changes how existing configured history windows and persisted scan state control dashboard catch-up.
  • add merge-risk: 🚨 availability: The new per-pass progress digest can make bounded catch-up perform corpus-sized work.

Label justifications:

  • P2: This fixes an established accelerated history path with limited but meaningful impact on large local histories.
  • merge-risk: 🚨 availability: The new per-pass progress digest can make bounded catch-up perform corpus-sized work.
  • merge-risk: 🚨 compatibility: The patch changes how existing configured history windows and persisted scan state control dashboard catch-up.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🦪 silver shellfish.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The reported validation uses synthetic caches and test stores; it does not show an after-fix configured-history dashboard run in a real setup. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

Likely related people:

  • steipete: Current-line blame identifies Peter Steinberger, and the owner both authored the recent merged catch-up performance work and set the integration constraint on this PR. (role: current implementation owner and recent area contributor; confidence: high; commits: 330ae4384b18, d7934a5f6121; files: Sources/CodexBar/UsageStore+SpendDashboardCodexCostCatchUp.swift, Sources/CodexBarCore/CostUsageFetcher.swift)

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 (1 earlier review cycle)
  • reviewed 2026-08-11T13:43:33.757Z sha 185c516 :: needs real behavior proof before merge. :: none

@steipete

Copy link
Copy Markdown
Owner

Thanks for isolating the shared-cache scope bug and for preserving arbitrary configured history values. The core max(30, configuredHistoryDays) behavior is correct.

This draft cannot safely land independently of #2849, though. The proposed progress digest covers current main's older lookback state but omits the new current-window, directory, flat-root, migration, and inventory cursors on #2849; a cursor-only pass could still be classified as stalled. Sorting/hashing every cached file, session mapping, and pending queue also conflicts with #2849's bounded-pass invariant. Finally, the history-change test invokes synchronization directly rather than proving that a visible dashboard observes the setting change and replaces work at a safe pass boundary.

Please keep this draft open. After the persisted-inventory ownership decision on #2849, the right integration is to retain the effective-window concept but use scanner-owned madeDurableProgress reporting (or the approved equivalent), include the real settings/dashboard lifecycle, and prove no corpus-wide status traversal. I won't ask you to chase a moving cursor contract in this branch before that decision.

@clawsweeper clawsweeper Bot added merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. labels Aug 12, 2026
@steipete

Copy link
Copy Markdown
Owner

Fixed on main via #2918: "Finish now" now honors the configured cost-history window (max(30, configured)) with the window carried in the catch-up scope signature. Your fix and tests were folded in with Co-authored-by credit — thank you @thomaschow19! Closing as superseded; #2861 is closed by the merge.

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

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. 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.

[Codex cost] “Finish now” accelerates only 30 days instead of the configured history window

2 participants