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 changes before merge. Reviewed September 28, 2026, 4:20 AM ET / 08:20 UTC (Revision 5). ClawSweeper reviewWhat this changesRetries Codex OAuth credential reads during file publication and adjusts how quota snapshots are published after a subscription plan change, with tests and documentation. Merge readiness⛔ Needs changes before merge - 3 items remain Keep this PR open. Current main and v0.68.0 do not include its central change, and both findings from the prior review remain on the unchanged head. The targeted work is worth repairing. Priority: P2 Review scores
Verification
How this fits togetherCodexBar reads Codex credentials and usage responses, then combines fresh quota data with saved reset evidence. An account-scoped publication check decides what usage appears in the menu and account views. flowchart LR
A[Codex credential file] --> B[OAuth usage fetch]
B --> C[Fresh quota snapshot]
D[Saved reset evidence] --> E[Plan and reset check]
C --> E
E --> F[Menu and account usage]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Technical reviewBest possible solution: Carry the plan from a trusted source snapshot into visible-account admission, then use fixtures with otherwise valid reset boundaries to prove the plan and confidence guards decide publication. Do we have a high-confidence way to reproduce the issue? Yes for the core paths: the PR supplies synthetic credential-publication and quota-transition fixtures against the current code. The visible-account case with only last-known reset evidence is source-traceable but is not covered by those fixtures. Is this the best way to solve the issue? No, not yet: bounded credential rereads fit the existing CLI-owned token contract, but the quota repair needs a plan-bearing baseline and tests that isolate its new admission guards. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against a5252e24c844. LabelsLabel changes:
Label justifications:
EvidenceAcceptance criteria:
What 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)
|
Use the existing macOS 14 lock helper and typed TaskLocal readers. Cover mismatched near-zero confirmation plans, preserve higher-usage confirmations, and keep fixture timestamps relative to the test.
Retry transient credential publication failures without redeeming owner tokens. Discard old-plan quota evidence for fresh exact OAuth transitions and require matching plans for near-zero confirmation. Preserve selected workspace scope and existing higher-usage confirmation behavior. Refs #3635. Fixes #3389.
…ce (#4088) Codex credential reads retry with a bounded reread when auth.json is missing, partial, incomplete, or expiring while the Codex CLI publishes fresh credentials mid-fetch, and a fresh plan change invalidates the old quota/reset evidence so usage from the new plan replaces the previous one. Fixes #3389; refs #3635 #3523. Thanks @theDanielJLewis and @coygeek!
Codex could surface a credential error after one read overlapping an owner publication, and a subscription upgrade could leave the previous plan's quota snapshot on screen. OAuth availability and fetches now retry transient credential reads up to three times, 50 ms apart. A fresh exact plan change discards the old quota/reset baseline; near-zero confirmations must agree on the plan, while higher-usage confirmations retain their existing publication path.
Native and managed credentials remain CLI-owned. Reads retain the selected workspace and final error category; the five-minute renewal margin and managed CLI fallback guard are preserved. Automatic managed renewal remains a separate decision in #3523. Shared result construction and backfill helpers offset the changes: production delta is 106 additions / 112 deletions, net −6 against merged main.
Validation:
env TMPDIR="$PWD/.build/tmp" make check: passed; 0/2670 files require formatting and 0 lint violations in 2669 files.swift build --target CodexBarCore --jobs 2 --disable-index-store -debug-info-format none -Xswiftc -Xfrontend -Xswiftc -emit-macro-expansion-files -Xswiftc -Xfrontend -Xswiftc none: passed.timeout 90 /Applications/CodexBar.app/Contents/Helpers/CodexBarCLI usage --provider codex --source oauth --no-credits --jsonreturned OAuth usage without an error. No login, configuration change, browser import, or app relaunch was performed.The first CI baseline failed to compile the test fixtures; the follow-up replaced macOS-15-only synchronization and ambiguous TaskLocal closure syntax. The next baseline reached the intended OAuth failures. Local full test-bundle builds were blocked by prolonged filesystem rename waits, so focused suites were linked separately against real built core and policy source. No full sharded suite was run on the shared Mac.
Final CI passed for head
f0ca85204a7aeb5087e6fbdff4a3b5fa0aab17b0(lint, Linux builds, macOS compatibility and full macOS tests): https://github.com/steipete/CodexBar/actions/runs/36388623395Fixes #3389
Refs #3635
Refs #3523