Fix misleading Codex credential recovery guidance - #3534
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. |
clawsweeper
Bot
added
P2
|
Codex review: needs maintainer review before merge. Reviewed September 10, 2026, 4:17 AM ET / 08:17 UTC (Revision 2). ClawSweeper reviewWhat this changesThe PR replaces misleading Codex credential-refresh guidance with account-specific recovery instructions and adds matching regression assertions, documentation, and a changelog entry. Merge readiness✅ Ready for maintainer review This remains a useful, narrowly scoped correction: current main and the latest release still promise recovery that managed workspaces cannot perform. No blocking defects were found, and the previous review's empty finding list remains appropriate. Priority: P2 Review scores
Verification
How this fits togetherCodexBar reads credentials from the selected Codex home to retrieve account usage. Its provider pipeline prevents unscoped CLI fallback for managed workspaces, so expired credentials can surface a recovery message instead. flowchart TD
A[Selected Codex home] --> B[Read native credentials]
B --> C{Credentials need renewal?}
C -->|No| D[Retrieve account usage]
C -->|Yes| E{Managed workspace selected?}
E -->|No| F[Eligible CLI recovery]
E -->|Yes| G[Account-specific recovery guidance]
Before mergeNone. Agent review detailsSecurityNone. Review metricsNone. Technical reviewBest possible solution: Keep the recovery message truthful and account-specific while preserving workspace guards and handling automatic renewal separately. Do we have a high-confidence way to reproduce the issue? Yes, from source: expired managed credentials raise nativeRefreshRequired while both applicable CLI recovery paths are suppressed, leaving main's retry promise false. No live reproduction was executed. Is this the best way to solve the issue? Yes. Correcting the shared diagnostic and documenting existing targeted recovery is a bounded solution that preserves credential ownership and workspace isolation. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 3b6864fa9248. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (1 earlier review cycle)
|
|
Landed in f5c94a1. The shared diagnostic now gives account-specific recovery guidance instead of promising a CLI retry that managed workspace scopes deliberately suppress. No credential, fallback, or auth-file behavior changed. The regression failed eight added wording assertions before the fix. Afterward, 80 focused tests and the full 1,058-selection suite passed, with all 89 groups successful on the first pass and no retries or timeouts. Formatting/lint and independent P0–P2 review were clean. All checks passed in CI run 34451275400. #3523 remains open: this fixes its misleading guidance, while automatic renewal remains separate in #3379. Provider documentation and the Unreleased changelog are updated, with credit to @zhulijin1991 for the report. |
1 participant
The shared native-credential refresh error no longer promises that CodexBar will retry through the CLI. Managed workspace selection deliberately disables that unscoped fallback, so the previous promise was false in both Auto and OAuth modes. The error now points to reauthenticating the affected account or running
codex loginin the same Codex home.This corrects the diagnostic portion of #3523. It preserves
nativeRefreshRequired, all strategy selection, credential ownership, and the existing reauthentication flow. The broader automatic-renewal work remains separate in #3379; #3523 should stay open.Proof:
Documentation identifies the existing account-specific Reauthenticate action and same-home CLI recovery. The 0.58.1 Unreleased entry credits @zhulijin1991 for the report. No live credentials were used.