Skip to content

fix(codex): retry auth reads and reset quota baselines on plan changes - #4088

Closed
steipete wants to merge 4 commits into
mainfrom
triage/20260921-codex-auth-3
Closed

steipete wants to merge 4 commits into
mainfrom
triage/20260921-codex-auth-3

Conversation

@steipete

@steipete steipete commented Sep 28, 2026 •

Copy link
Copy Markdown
Owner

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.
  • Debug 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.
  • Actual OAuth Swift Testing suite linked against baseline and rebuilt core: 14 assertion issues before; all 10 tests passed after. Expanded core verification: 114 tests in 8 suites passed, covering credential reads, expiry, permissions, requests, credits and managed-workspace restrictions.
  • Identical exact-confidence plan fixtures compiled against the original versus patched publication-policy source: 8 assertion issues before; all 6 tests passed after. Including the existing weekly-reset suite: 35 tests in 2 suites passed. This isolated policy harness uses an empty store host and localization stub; full app-state integration is covered by PR CI.
  • Signed installed release 0.67.0: timeout 90 /Applications/CodexBar.app/Contents/Helpers/CodexBarCLI usage --provider codex --source oauth --no-credits --json returned OAuth usage without an error. No login, configuration change, browser import, or app relaunch was performed.
  • Independent Codex review: no actionable P0–P2 findings in the final delta. Review findings about backfill-only state and confirmation-plan consistency were addressed.

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/36388623395

Fixes #3389
Refs #3635
Refs #3523

Add intentionally red coverage for credential publication races and stale quota baselines after plan changes (#3635, #3389). All credentials, HTTP responses, homes, and account state are synthetic.
@clawsweeper

clawsweeper Bot commented Sep 28, 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: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. labels Sep 28, 2026
@clawsweeper

clawsweeper Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Codex review: needs changes before merge. Reviewed September 28, 2026, 4:20 AM ET / 08:20 UTC (Revision 5).

ClawSweeper review

What this changes

Retries 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
Reviewed head: f0ca85204a7aeb5087e6fbdff4a3b5fa0aab17b0

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) Focused validation and a compact production change provide useful signal, but two unchanged publication-review blockers limit readiness.
Proof confidence 🦐 gold shrimp (3/6) Not applicable: The owner-authored PR is exempt from the external-contributor proof gate. Its synthetic before-and-after suites exercise the changed OAuth fetch and quota policy, while the signed installed v0.67.0 CLI smoke does not exercise this patch; no stored-data format changes require migration proof.
Patch quality 🦐 gold shrimp (3/6) 2 actionable review findings remain.

Verification

Check Result Evidence
Real behavior Not applicable Not applicable: The owner-authored PR is exempt from the external-contributor proof gate. Its synthetic before-and-after suites exercise the changed OAuth fetch and quota policy, while the signed installed v0.67.0 CLI smoke does not exercise this patch; no stored-data format changes require migration proof.
Evidence reviewed 9 items Introduced publication check: The new plan-change decision compares the fresh result with a prior snapshot or a merged reset snapshot, then discards old baselines only when it detects a change.
Plan identity lost in merged reset evidence: The merged reset snapshot reconstructs windows and a timestamp without carrying the source snapshot's plan; a visible account can supply this snapshot when it has no saved account row.
Visible-account input path: Visible-account refresh passes a merged reset snapshot and a separately optional prior account snapshot into publication admission.
Findings 2 actionable findings [P2] Preserve plan identity before merging reset evidence
[P2] Make negative plan fixtures reach their intended guards
Security None None.

How this fits together

CodexBar 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]
Loading

Before merge

  • Preserve plan identity before merging reset evidence (P2) - When a visible account has no saved account row but does have last-known reset evidence, the merge produces windows and a timestamp without a plan. At this new comparison, planBaseline cannot detect a Plus-to-Pro transition; a fresh near-zero Pro reading can then be rejected against the old reset boundary, leaving the old quota visible. Compare against a plan-bearing source snapshot before merging, and cover this account path.
  • Make negative plan fixtures reach their intended guards (P2) - These new negative fixtures set the old reset to 86,400 seconds and the new reset to 3,600 seconds. The existing earlier-boundary guard can reject them before the new same-plan or exact-confidence guard runs, so they still pass if those guards regress. Use compatible reset boundaries and assert the intended admission path.
  • Complete next step (P2) - Fix the plan-baseline comparison for the last-known-only visible-account path and make the negative fixtures exercise their intended guards before merge.

Findings

  • [P2] Preserve plan identity before merging reset evidence — Sources/CodexBar/Providers/Codex/UsageStore+CodexWeeklyResetConfirmation.swift:55-58
  • [P2] Make negative plan fixtures reach their intended guards — Tests/CodexBarTests/CodexPlanTransitionPublicationTests.swift:47-48
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Code and test delta production +106/−112 (net −6); tests +299/−6 (net +293) The branch keeps production size roughly flat while adding targeted regression coverage that still needs two fixture and path corrections.

Technical review

Best 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:

  • [P2] Preserve plan identity before merging reset evidence — Sources/CodexBar/Providers/Codex/UsageStore+CodexWeeklyResetConfirmation.swift:55-58
    When a visible account has no saved account row but does have last-known reset evidence, the merge produces windows and a timestamp without a plan. At this new comparison, planBaseline cannot detect a Plus-to-Pro transition; a fresh near-zero Pro reading can then be rejected against the old reset boundary, leaving the old quota visible. Compare against a plan-bearing source snapshot before merging, and cover this account path.
    Confidence: 0.94
  • [P2] Make negative plan fixtures reach their intended guards — Tests/CodexBarTests/CodexPlanTransitionPublicationTests.swift:47-48
    These new negative fixtures set the old reset to 86,400 seconds and the new reset to 3,600 seconds. The existing earlier-boundary guard can reject them before the new same-plan or exact-confidence guard runs, so they still pass if those guards regress. Use compatible reset boundaries and assert the intended admission path.
    Confidence: 0.93

Overall correctness: patch is incorrect
Overall confidence: 0.91

AGENTS.md: found and applied where relevant.

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

Labels

Label changes:

  • remove merge-risk: 🚨 auth-provider: Current PR review selected no merge-risk labels.

Label justifications:

  • P2: The PR targets limited Codex usage and credential-refresh failures, with two bounded publication-review blockers.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🦐 gold shrimp.
  • status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Not applicable: The owner-authored PR is exempt from the external-contributor proof gate. Its synthetic before-and-after suites exercise the changed OAuth fetch and quota policy, while the signed installed v0.67.0 CLI smoke does not exercise this patch; no stored-data format changes require migration proof.

Evidence

Acceptance criteria:

  • [P1] swift test --filter CodexPlanTransitionPublicationTests.
  • [P1] make test.
  • [P1] make check.

What I checked:

Likely related people:

  • Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)
  • Yuxin Qiao: Raw commit b841e34 adds Sources/CodexBar/Providers/Codex/UsageStore+CodexResetBackfill.swift:45 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: b841e34e5f91; files: Sources/CodexBar/Providers/Codex/UsageStore+CodexResetBackfill.swift)
  • Zihao Qi: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

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

  • Cover a plan change through visible-account refresh when only last-known reset evidence exists.
  • Repair the negative fixtures so plan and confidence guards determine their results, then run focused Codex tests, make test, and make check.

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-28T03:27:25.003Z sha 2a2fb62 :: blocked before merge. :: [P2] Make negative plan cases exercise the intended admission guard
  • reviewed 2026-09-28T05:18:03.723Z sha 41536a1 :: blocked before merge. :: [P2] Make negative plan fixtures reach the intended guards
  • reviewed 2026-09-28T06:57:06.081Z sha f0ca852 :: blocked before merge. :: [P2] Preserve plan identity for merged reset evidence | [P2] Make negative plan fixtures reach their intended guards
  • reviewed 2026-09-28T07:36:16.799Z sha f0ca852 :: blocked before merge. :: [P2] Preserve plan identity before merging reset evidence | [P2] Make negative plan fixtures reach their intended guards

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.
@clawsweeper clawsweeper Bot added the merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. label Sep 28, 2026
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.
@clawsweeper clawsweeper Bot added merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. and removed merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. merge-risk: 🚨 auth-provider 🚨 Merging this PR could break OAuth, tokens, provider routing, model choice, or credentials. labels Sep 28, 2026
steipete added a commit that referenced this pull request Sep 28, 2026
…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!
@steipete

Copy link
Copy Markdown
Owner Author

Landed on main as 9013fd8 via merge train #4099 (one green CI run for the whole train).

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

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Usage not updating after ChatGPT/OpenAI subscription upgrade

1 participant