Skip to content

refactor(codex): forward catch-up mode budgets - #3509

Open
kernnel wants to merge 4 commits into
steipete:mainfrom
kernnel:fix/codex-pending-token-checkpoints
Open

refactor(codex): forward catch-up mode budgets#3509
kernnel wants to merge 4 commits into
steipete:mainfrom
kernnel:fix/codex-pending-token-checkpoints

Conversation

@kernnel

@kernnel kernnel commented Sep 8, 2026

Copy link
Copy Markdown

Held: no reachable production bug fix is established.

The main usage catch-up helper accepts an accelerated mode, but its Accelerate/Background entry points have no production callers. Menu opening, explicit provider refresh, and stale-result retries retain automatic mode. The Spend Dashboard uses a separate worker that already forwards its selected budget. This patch therefore does not currently repair the user-visible report in #3508.

The proposed refactor forwards the selected duration through one shared worker/fetch boundary and removes the single-use provider wrapper while retaining its Codex guard. Production code is net −3 lines. It preserves automatic scheduling and exact-inventory publication; the earlier partial-row checkpoint approach has been removed completely. No new controls are added.

Validation of the code at 6a8bb78: the budget regression detects the old two-second behavior under test-invoked acceleration; 105 focused tests across six suites passed; full make test passed 1,041 selections in 87 groups on the first attempt without retries/timeouts; make check and independent P0–P2 review passed. The original oversized-active-day regression is retained alongside fresh/0.56.8-cache dated-discovery checks. Subsequent documentation-only correction removes unsupported release-note claims.

Missing proof: a reproduced automatic cursor/scheduling defect or an explicitly designed, reachable user-only acceleration route with signed runtime verification. Source and test consistency alone do not establish that production behavior. The PR remains open with changes requested, and #3508 remains open. No user-facing changelog entry is included.

Thanks @kernnel for the report, iteration, and reachability clarification.

@clawsweeper

clawsweeper Bot commented Sep 8, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

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

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@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 Sep 8, 2026
@clawsweeper

clawsweeper Bot commented Sep 8, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed September 8, 2026, 8:48 AM ET / 12:48 UTC.

ClawSweeper review

What this changes

Forward the usage catch-up worker’s selected scan duration, remove a redundant provider wrapper, and extend budget and discovery regression coverage.

Merge readiness

Blocked before merge - 3 items remain

Keep open under the owner's explicit readiness hold. The narrowed refactor has no identified correctness defect, but its accelerated path is unreachable from production actions and does not establish a fix for the reported stale totals.

Likely related people: steipete (high-confidence routing), Yuxin-Qiao (medium-confidence routing).

Priority: P3
Reviewed head: 5185e5c912da9feda810a37e83dd25a08a6d3b0f
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The narrow implementation and reported validation are sound, but test-only evidence leaves the real-behavior gate unresolved.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: Needs real behavior proof before merge or an explicit exception: duration coverage intercepts the usage worker's fetch boundary, and scanner tests run separately. No after-fix runtime evidence traverses the changed worker through the real fetcher; the contributor correctly identifies the missing production caller. 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 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: Needs real behavior proof before merge or an explicit exception: duration coverage intercepts the usage worker's fetch boundary, and scanner tests run separately. No after-fix runtime evidence traverses the changed worker through the real fetcher; the contributor correctly identifies the missing production caller. 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 9 items Applicable repository policy: Read the complete root AGENTS.md. No nested AGENTS.md or maintainer-note files were found under the reviewed source and test paths. Applied guidance on focused changes, provider isolation, test isolation, and preserving intentional behavior; no builds or tests were executed during this read-only review.
Complete introduced change: Inspected all seven files in the merge-base-to-head diff. Production changes only forward duration and move the existing Codex guard to its caller; no partial-row publication, schema, dependency, permission, or scheduling-policy change remains.
Reachability and production boundary: The accelerated and background entry points have no production callers. Usage refresh and hydration select automatic mode; the separate Spend Dashboard worker already supplies its selected duration at line 354. The shared scanner and fetcher are owned by this repository.
Findings None None.
Security None None.

How this fits together

CodexBar scans local Codex session history to calculate token and cost totals for its menu and widgets. Its usage catch-up worker advances bounded scans and publishes completed history; the Spend Dashboard has a separate worker.

flowchart TD
  A[Usage refresh or cached startup] --> B[Automatic catch-up worker]
  C[Local Codex session history] --> D[Bounded history scanner]
  B -->|Selected scan duration| D
  D --> E{History fully validated?}
  E -->|No| B
  E -->|Yes| F[Menu and widget totals]
  G[Spend Dashboard worker] --> D
Loading

Decision needed

Question Recommendation
Should this dormant-path refactor remain held for a reachable production defect, or be accepted separately with an explicit proof exception? Retain the current hold: Preserve the branch until a reachable automatic-progress defect and relevant runtime evidence justify further work.

Why: The owner's latest review deliberately supersedes approval; passing tests and the corrected description cannot release that hold.

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: Needs real behavior proof before merge or an explicit exception: duration coverage intercepts the usage worker's fetch boundary, and scanner tests run separately. No after-fix runtime evidence traverses the changed worker through the real fetcher; the contributor correctly identifies the missing production caller. 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.
  • Complete next step (P2) - Obtain an explicit owner disposition of the readiness hold and satisfy the runtime-proof requirement or receive an explicit proof exception.
  • Resolve maintainer decision - Resolve the maintainer decision shown above before merge.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta Production +11/−14 (net −3); tests +161/−42 (net +119) The refactor reduces production code while retaining prior coverage and adding duration and dated-discovery checks.

Technical review

Best possible solution:

Keep scan budgets consistent with existing mode policy while preserving automatic scheduling and exact-inventory publication; address observable scheduling starvation separately.

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

Not applicable to the narrowed refactor. Source confirms the dormant duration mismatch, but no high-confidence reproduction of the linked automatic-scheduling failure is established.

Is this the best way to solve the issue?

Yes for internal budget consistency: forwarding the existing mode value is narrow and preserves behavior. It is not an established solution to the reported stale totals.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against b76508292e68.

Labels

Label justifications:

  • P3: The current branch is a bounded internal refactor with no established production-visible correction.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: Needs real behavior proof before merge or an explicit exception: duration coverage intercepts the usage worker's fetch boundary, and scanner tests run separately. No after-fix runtime evidence traverses the changed worker through the real fetcher; the contributor correctly identifies the missing production caller. 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:

  • Applicable repository policy: Read the complete root AGENTS.md. No nested AGENTS.md or maintainer-note files were found under the reviewed source and test paths. Applied guidance on focused changes, provider isolation, test isolation, and preserving intentional behavior; no builds or tests were executed during this read-only review. (AGENTS.md:1, 5185e5c912da)
  • Complete introduced change: Inspected all seven files in the merge-base-to-head diff. Production changes only forward duration and move the existing Codex guard to its caller; no partial-row publication, schema, dependency, permission, or scheduling-policy change remains. (Sources/CodexBar/UsageStore+CodexCostCatchUp.swift:343, 5185e5c912da)
  • Reachability and production boundary: The accelerated and background entry points have no production callers. Usage refresh and hydration select automatic mode; the separate Spend Dashboard worker already supplies its selected duration at line 354. The shared scanner and fetcher are owned by this repository. (Sources/CodexBar/UsageStore+CodexCostCatchUp.swift:86, 5185e5c912da)
  • Current-main and release comparison: Fetched main still omits the selected duration at the usage-worker boundary. The v0.56.8 source retrieved through GitHub also omits it. Neither establishes a shipped or main-branch resolution, and no merged fixing PR for this request was established. (Sources/CodexBar/UsageStore+CodexCostCatchUp.swift:345, b76508292e68)
  • Latest owner review supersedes approval: The owner's September 8, 12:42 UTC CHANGES_REQUESTED review explicitly supersedes the earlier approval, confirms the dormant accelerated path, requires a reachable production defect and proof, and rejects adding controls merely to make the helper reachable. (6a8bb78324fb)
  • Re-review continuity: Source and tests are unchanged from the previously reviewed head. GitHub's exact-head commit patch confirms that the latest commit only withdraws the changelog and documentation claims. Earlier checkpoint-publication and forced-acceleration findings are no longer present. (5185e5c912da)

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)

Rank-up moves

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

  • Resolve the owner's dormant-path hold explicitly; do not add a UI action solely to satisfy proof.
  • If runtime evidence becomes available, add terminal output or logs showing the changed boundary and observed result, redacting private paths, identifiers, endpoints, and credentials. Update the PR body to trigger review; if needed, ask a maintainer to comment @clawsweeper re-review.

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 (4 earlier review cycles)
  • reviewed 2026-09-08T10:49:56.669Z sha 0d8be67 :: needs real behavior proof before merge. :: [P2] Preserve established costs when accepting token-growth checkpoints
  • reviewed 2026-09-08T11:27:55.829Z sha c874f32 :: needs real behavior proof before merge. :: [P2] Keep automatic retries out of accelerated catch-up
  • reviewed 2026-09-08T12:05:03.770Z sha 6a8bb78 :: needs real behavior proof before merge. :: none
  • reviewed 2026-09-08T12:25:00.952Z sha 6a8bb78 :: needs real behavior proof before merge. :: none

@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: 0d8be670ba

ℹ️ 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 thread Sources/CodexBar/UsageStore+TokenCost.swift Outdated

@steipete steipete left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The stale-publication report is worth investigating, but this checkpoint rule is not ready to land.

[P2] A larger total token count does not validate replacing the whole daily row. The replacement at lines 201–204 can replace an established 1,100-token row with 100 output tokens and $2 cost with an incomplete 1,200-token row containing zero output tokens and $0.10 cost. The condition accepts the larger total while discarding previously established cost, token-class and model detail. Setting history coverage to established then presents the partial replacement as complete. Please add coverage for partial growth with missing/decreasing accounting fields and unresolved deduplication/fork accounting; token monotonicity alone is not enough to establish safe publication.

For the root-cause reproduction, #3490 is already included in the checked main revision. Current discovery starts at the newest date and moves backward, so a cursor four weeks before today can indicate successful progress. The progress key also includes discovery dates and directory offsets. A useful regression should use fixed roots/window and enough dated directories to exceed a page, record successive cursors and visits, and show whether discovery repeats/resets or is simply progressing slowly under the catch-up schedule. Include an upgraded 0.56.8 cache and a fresh cache. Seeding a partial report and observing that the established report remains visible exercises the intentional exact-inventory guard, but does not identify the remaining discovery/scheduling defect.

This is a source review; I have not reproduced the reported multi-hour signed-app run. Keeping the issue and PR open while that proof and accounting-safe correction are developed.

@kernnel
kernnel force-pushed the fix/codex-pending-token-checkpoints branch from 0d8be67 to c874f32 Compare September 8, 2026 11:23
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. label Sep 8, 2026
@steipete

steipete commented Sep 8, 2026

Copy link
Copy Markdown
Owner

Taking a maintainer pass on the narrower rewrite. I am keeping forced refreshes on the existing automatic policy: menu opening and stale-result retries also use force: true, so that flag must not promote them to acceleration. The independent defect is that the usage catch-up worker does not forward its already-selected 2s/10s duration to the core scanner, while the Spend Dashboard does.

I am adding worker-boundary duration coverage, restoring the original oversized-active-day regression alongside the new dated-discovery checks, and removing the single-use forwarding wrapper. The preservation/completeness rules remain intact. This will be scoped as an acceleration-budget fix related to #3508; the multi-hour automatic scheduling report remains open unless separately reproduced.

Co-authored-by: kernnel <kernnel@users.noreply.github.com>
@steipete steipete changed the title fix(codex): publish safe token checkpoints during catch-up fix(codex): honor the accelerated catch-up scan budget Sep 8, 2026
@clawsweeper clawsweeper Bot removed the merge-risk: 🚨 compatibility 🚨 Merging this PR could break existing users, config, migrations, defaults, or upgrades. label Sep 8, 2026
@kernnel

kernnel commented Sep 8, 2026

Copy link
Copy Markdown
Author

Thanks for narrowing the change and preserving the automatic policy. I checked the production call graph at 6a8bb78: startAcceleratedCodexCostCatchUp() and returnCodexCostCatchUpToBackground() have no callers outside their definitions. The explicit provider refresh still calls refreshTokenUsageNow(for:force: true), and menu opening calls scheduleForcedTokenRefresh(); both remain automatic. The Spend Dashboard's Finish Now action controls its separate catch-up worker.

Could you point to the production UI/action that selects .accelerated for the main usage worker? Without one, the corrected 10-second path cannot currently be exercised in a signed app, which also makes the requested real-behavior proof impossible for this worker path.

I agree that force is too broad because passive menu opening and stale retries share it. The smallest follow-up seems to be wiring only an explicit user-triggered Finish Now/accelerated action for the main usage worker, while leaving menu opening and stale retries automatic. I have updated #3508 to keep the multi-hour automatic-scheduling report separate and open.

@steipete steipete left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Reviewed revised head 6a8bb78. The unsafe partial-row/established-history path is gone, and forced refresh retains its original automatic policy. The remaining change forwards the already-selected scan budget, with the redundant wrapper removed.

The budget regression detects the old two-second behavior in accelerated mode. Focused tests, all 87 full-suite groups, formatting/lint, and independent P0–P2 review passed. Original oversized-directory coverage remains alongside the new discovery checks. Code review approved; final-head hosted CI remains the landing gate. #3508 stays open for any remaining automatic-discovery/scheduling reproduction.

@steipete steipete left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Holding this PR pending a reachable production defect and proof. The reporter's follow-up and a complete Sources search confirm that the main worker's Accelerate/Background entry points have no production callers. Current menu-open, explicit-refresh, and stale-retry paths remain automatic, while the Spend Dashboard uses its separate correctly-budgeted worker.

The narrowed diff is internally consistent and its tests/checks pass, but it does not currently change the reported production behavior. My earlier code approval did not establish that reachability; I am superseding it with this readiness hold and removing the user-facing changelog/documentation claim. The partial-checkpoint approach must remain removed.

Keep #3508 open. A production fix needs either a reproduced defect in automatic cursor/scheduling progress or an explicitly designed user-only acceleration route with signed runtime proof. This bugs-only pass will not add new controls merely to make the helper reachable. The existing branch and proof remain available for that follow-up.

@steipete steipete changed the title fix(codex): honor the accelerated catch-up scan budget refactor(codex): forward catch-up mode budgets Sep 8, 2026
@clawsweeper clawsweeper Bot added P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. and removed P2 Normal priority bug or improvement with limited blast radius. labels Sep 8, 2026
@kernnel

kernnel commented Sep 8, 2026

Copy link
Copy Markdown
Author

New signed-runtime evidence for the reachable automatic path is now posted on #3508: production reproduction.

The signed 0.56.8 run shows the startup 365-day shared Spend Dashboard worker and regular token usage sharing the same cache. A cache-wide migration queued 1,827 paths in 512-path pages, while the preserved 30-day report remained visible and all observed cursor fields stayed unchanged for more than 34 minutes. A retained cache from seven days earlier shows the same migration signature.

This confirms that the current dormant-main-worker budget refactor does not fix the reported behavior. I recommend keeping this PR held as-is and targeting the reachable shared worker's automatic migration scheduling next, while preserving exact-inventory publication and leaving the partial-checkpoint approach removed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. 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.

2 participants