refactor(codex): forward catch-up mode budgets - #3509
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs real behavior proof before merge. Reviewed September 8, 2026, 8:48 AM ET / 12:48 UTC. ClawSweeper reviewWhat this changesForward 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 Review scores
Verification
How this fits togetherCodexBar 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
Decision needed
Why: The owner's latest review deliberately supersedes approval; passing tests and the corrected description cannot release that hold. Before merge
Agent review detailsSecurityNone. Review metrics
Technical reviewBest 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. 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)
|
There was a problem hiding this comment.
💡 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".
steipete
left a comment
There was a problem hiding this comment.
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.
0d8be67 to
c874f32
Compare
|
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 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>
|
Thanks for narrowing the change and preserving the automatic policy. I checked the production call graph at Could you point to the production UI/action that selects I agree that |
steipete
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
|
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. |
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 testpassed 1,041 selections in 87 groups on the first attempt without retries/timeouts;make checkand 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.