fix(codex): correct historical pace run-out forecasts - #1581
Conversation
Yuxin-Qiao
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d7c30ce2b5
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| APP="$TMP_APP" | ||
| SPARKLE="$APP/Contents/Frameworks/Sparkle.framework" | ||
| chmod -R u+w "$APP" | ||
| xattr -cr "$APP"; xattr -dr com.apple.FinderInfo "$APP" 2>/dev/null |
There was a problem hiding this comment.
On a normal package where com.apple.FinderInfo is not present, this aborts the script under set -e: xattr -cr "$APP" clears all attributes first, then xattr -dr com.apple.FinderInfo "$APP" tries to delete an attribute that is now absent. The macOS xattr man page documents -c as clearing all attributes, -d as deleting the named attribute, and non-zero exit on errors, while removexattr(2) reports ENOATTR when the attribute does not exist, so direct ./Scripts/package_app.sh can fail before signing unless this delete is made best-effort.
Useful? React with 👍 / 👎.
- Evaluator immediately returns 0 ETA when usage >= 100% - Bypass codesign detritus error by staging Sparkle framework signing through /tmp - Add test coverage for 100% exhaustion scenarios
ac73ecc to
7ffb96c
Compare
steipete
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ffb96cffb
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if actual >= 100 { | ||
| willLastToReset = false | ||
| etaSeconds = 0 | ||
| runOutProbability = 1 |
There was a problem hiding this comment.
When usedPercent is 100% or more and the UI gets pace via UsageStore.weeklyPace, this branch is skipped: Sources/CodexBar/UsageStore+HistoricalPace.swift returns nil for window.remainingPercent <= 0 before invoking CodexHistoricalPaceEvaluator. I checked the MenuDescriptor/menu-bar pace paths that call store.weeklyPace directly, so exhausted Codex weekly windows still do not receive the new Runs out now/etaSeconds = 0 pace there; allow exhausted windows through the store or handle the zero-ETA case at that layer.
Useful? React with 👍 / 👎.
|
Codex review: needs real behavior proof before merge. Reviewed June 18, 2026, 10:53 AM ET / 14:53 UTC. Summary Reproducibility: yes. at source level. The PR adds exhausted-quota handling in the evaluator, but the current store wrapper still returns nil for exhausted windows before the evaluator is called by menu/card paths. Review metrics: 2 noteworthy metrics.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Mantis proof suggestion Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Finish the store/menu exhausted-window path, keep the focused historical pace regressions, and add redacted real-app proof showing the corrected Codex weekly pace text. Do we have a high-confidence way to reproduce the issue? Yes, at source level. The PR adds exhausted-quota handling in the evaluator, but the current store wrapper still returns nil for exhausted windows before the evaluator is called by menu/card paths. Is this the best way to solve the issue? No. The evaluator changes are mostly the right direction, but the complete fix needs exhausted-window behavior to reach UsageStore.weeklyPace callers and needs real app proof. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 016f3b9e1314. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
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. How this review workflow works
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Landing proof for exact head
Public model identifier gate: N/A; no model-bearing content changed. |
2 participants
Fixes #1544
Summary
Scripts/package_app.shis byte-identical tomain.Lasts until resetwith a material rounded run-out risk.Proof
Exact reviewed head:
7ffb96cffb1ed737833db09288ff7eb8bf18269928% in deficitandRuns out in 13h 47m.5% in deficitwith a run-out forecast, never a reserve.Runs out now · ≈ 100% run-out risk.make checkpasses with zero format or lint findings.CodexGitCommit=7ffb96cf; app launched and remained running.