Refresh managed Codex credentials without losing workspace scope - #3379
Refresh managed Codex credentials without losing workspace scope#3379Yuxin-Qiao wants to merge 2 commits into
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 real behavior proof before merge. Reviewed September 8, 2026, 11:28 AM ET / 15:28 UTC. ClawSweeper reviewWhat this changesRefresh native Codex credentials through the selected account’s CLI store, then retry workspace-scoped usage requests while keeping permission denials terminal. Merge readiness⛔ Blocked before merge - 7 items remain The managed credential recovery remains useful and absent from main. One cancellation defect blocks merge; the earlier timeout and permission-error findings are fixed. Provider-auth approval and denied-workspace proof remain outstanding. Priority: P2 Review scores
Verification
How this fits togetherCodexBar reads account credentials to retrieve provider usage for its menu bar and CLI. This recovery path asks the Codex app-server to renew credentials, then resumes the normal workspace-scoped HTTP request. flowchart TD
A[Selected account and workspace] --> B[OAuth usage request]
B --> C{Authentication result}
C -->|Expired or rejected token| D[Selected home credential renewal]
D --> E[Reload native credentials]
E --> B
C -->|Permission denied| F[Terminal error]
C -->|Success| G[Scoped usage publication]
Decision needed
Why: VISION.md reserves provider-auth behavior changes for sign-off, and this patch permits persistent token renewal where managed recovery was previously disabled. Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Keep renewal owned by the selected Codex home, preserve workspace authorization on retry, and make shared renewal cancellation-aware under an explicitly approved provider-auth contract. Do we have a high-confidence way to reproduce the issue? Yes, by source inspection: cancel a provider fetch while shared native renewal is pending, then request a replacement; it waits for the uncanceled renewal task. This path was not executed during the read-only review. Is this the best way to solve the issue? The owner-CLI renewal followed by scoped HTTP usage is a sound boundary, but the implementation needs waiter-aware cancellation before it is ready. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 928166f89947. 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 (6 earlier review cycles)
|
![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: 39367d446b
ℹ️ 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".
| } | ||
|
|
||
| func refreshAccount() async throws { | ||
| _ = try await self.request(method: "account/read", params: ["refreshToken": true]) |
There was a problem hiding this comment.
When token renewal takes more than three seconds, as can occur on slow or proxied networks, this request inherits requestTimeoutSeconds (3 seconds by default), terminates the app-server, and leaves stale or rejected managed credentials unrecovered. I confirmed via codex app-server generate-json-schema --experimental that refreshToken: true “requests a proactive token refresh before returning,” so unlike the existing local account/read operation this can include a network round trip; use a refresh-specific timeout comparable to the existing 30-second OAuth refresh timeout and cover a delayed response.
Useful? React with 👍 / 👎.
39367d4 to
2b26282
Compare
|
Addressed the actionable 401/403 finding in the updated head:
Live-account proof remains intentionally absent because AGENTS.md prohibits live provider/auth probes without an explicit request; the PR body states that boundary. @clawsweeper re-review |
|
🦞👀 Re-review progress:
|
2b26282 to
4b09066
Compare
|
Addressed the latest P1 on head
All auth validation remains fixture/stub based; no real auth files or Keychain entries were read. @clawsweeper re-review |
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
clawsweeper
Bot
added
the
merge-risk: 🚨 security-boundary
|
@steipete Provider-auth sign-off requested per |
4b09066 to
6ba56a8
Compare
|
Post-rebase verification on head |
6ba56a8 to
796d070
Compare
|
Current-head verification after rebase:
Evidence boundary remains explicit: the previously authorized live check proved the selected-home 401 renewal and scoped retry on pre-rebase head @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
…3466) HTTP 403 from Codex OAuth and PAT endpoints was treated as expired authentication. In Auto mode this could trigger CLI recovery for a permission denial. Preserve the status and response body as a terminal server error; HTTP 401 retains authentication recovery. Share request status, network-error, and cancellation handling across usage, reset-credit, spend-control, and PAT requests. Requests and decoding contracts remain unchanged. Production code decreases by 64 lines. The actual fetcher-to-pipeline regression failed before the change, including Auto selecting the CLI fallback sentinel. All 93 focused tests across five suites pass, covering OAuth/PAT endpoints, 401 controls, and terminal 403 behavior. `make check` and the full `make test` suite pass (1,028 selections across 86 groups; no retries). Independent review found no actionable P0–P2 findings. Tests use synthetic responses; no live account access is claimed. This extracts the status distinction discussed in #3379; the broader managed credential renewal work remains open. Thanks @Yuxin-Qiao for that work. Changelog and OAuth documentation are updated. Co-authored-by: Yuxin Qiao <104957188+Yuxin-Qiao@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 06baadaaff
ℹ️ 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".
| let task = Task { | ||
| try await operation() | ||
| } |
There was a problem hiding this comment.
When an in-flight provider refresh is superseded—for example after switching accounts or manually refreshing again—ProviderRefreshCoordinator cancels the old fetch and waits for it before starting the replacement. This unstructured Task does not inherit that cancellation, and awaiting task.value is not cancellation-aware, so the old app-server request can remain alive for the full initialization plus 30-second renewal timeout and block the replacement refresh. Make the coordinator waiter-aware so cancellation releases the canceled caller and cancels the subprocess when no other waiter still needs it.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
merge-risk: 🚨 availability
1 participant
Summary
Why
Managed Codex profiles remain signed in in separate native credential stores, but CodexBar previously disabled CLI recovery whenever a managed workspace was selected. That avoided showing usage for the wrong workspace, but it also left inactive accounts stuck once their auth.json aged or the backend rejected the access token.
This separates credential renewal from usage retrieval: the credential-owning app-server handles account/read with refreshToken enabled, then CodexBar reloads the exact native store and performs its existing account-scoped OAuth request. Only HTTP 401 is evidence for rejected bearer credentials; HTTP 403 stays fail-closed as a selected-workspace authorization error. The refresh request gets 30 seconds because it can perform network token renewal, while ordinary local RPC reads keep their 3-second budget.
Verification
Current head
796d070fd74cee91cb0bd45a34df55b5bf9029b0, rebased onto30f881aee0e8dce08690d8eb971a2a44ef1285be:make check: passed; SwiftFormat clean and SwiftLint reported 0 violationsmake test: 1009/1009 selections, 85/85 groups successful on the first pass, 0 failures, 0 retries, 0 timeoutsgit diff --check: passedCurrent-head regression validation used isolated temporary CODEX_HOME fixtures and stub transports; no additional real auth files or Keychain entries were read or changed.
Real behavior evidence and remaining boundary
An explicitly authorized live selected-home check on pre-rebase head
6ba56a88eobserved a real HTTP 401. The credential-owning app-server renewed the selected home's native credentials, the auth file rotated with mode 0600 preserved, and the workspace-scoped retry succeeded. No credentials, identities, or private endpoints are included.The authorized cross-account negative attempt did send the selected workspace header, but the service returned 2xx. It therefore does not prove the HTTP 403 terminal path. This PR does not claim real 403 evidence; a genuinely forbidden workspace or maintainer-approved negative target is still needed to demonstrate that no refresh or usage publication occurs after a real 403.
Provider-auth sign-off remains requested per
VISION.md.