Keep shared Codex auth read-only during usage refresh - #2970
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. |
clawsweeper
Bot
added
merge-risk: 🚨 auth-provider
|
Codex review: needs changes before merge. Reviewed August 16, 2026, 3:00 AM ET / 07:00 UTC. ClawSweeper reviewWhat this changesThe PR stops Codex usage refresh from modifying shared OAuth files, adds source-aware recovery and workspace scoping, and updates related tests and documentation. Merge readinessKeep this owner-authored PR open: its read-only credential boundary is valuable, but an explicit CLI refresh under a selected managed workspace can still combine CLI usage from one workspace with reset credits requested for another. Priority: P1 Review scores
Verification
How this fits togetherCodexBar’s Codex provider reads a selected account’s credentials, fetches usage and optional reset-credit inventory, and supplies that snapshot to the menu-bar UI. The fetch plan chooses OAuth, CLI, or web sources while managed-account metadata supplies workspace scope. flowchart LR
A[Selected Codex account] --> B[Fetch-plan source selection]
C[CLI auth file] --> B
B --> D[OAuth or CLI usage request]
A --> E[Managed workspace scope]
D --> F[Reset-credit enrichment]
E --> F
F --> G[Menu-bar usage snapshot]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Copy recommended automerge instructionTechnical reviewBest possible solution: Preserve the read-only OAuth design, but skip supplemental managed-workspace reset credits for unscoped CLI outcomes unless the CLI result can prove it used the same workspace; add a direct-CLI regression test. Do we have a high-confidence way to reproduce the issue? Yes, from source: select a managed workspace B, choose explicit CLI while auth.json is scoped to A, and enable credit enrichment; the CLI usage path is unscoped while the supplemental request is forced to B. Is this the best way to solve the issue? No: the read-only ownership approach is sound, but generic reset-credit enrichment must not apply managed scope after an unscoped CLI result. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against e0f07f9c65f8. LabelsLabel 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 (1 earlier review cycle)
|
![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: a29973fe99
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let priorTokenAccountSnapshot = self.tokenAccountSnapshot(provider: provider, account: tokenAccount) | ||
| let descriptor = spec.descriptor | ||
| let codexResetCreditsFetcher = self.codexResetCreditsFetcher() | ||
| let codexResetCreditsFetcher = self.codexResetCreditsFetcher(workspaceAccountID: fetchContext.codexWorkspaceID) |
There was a problem hiding this comment.
When a managed account selects workspace B while its now-read-only auth.json remains scoped to workspace A, choosing the explicit CLI source still runs CodexCLIUsageStrategy, which cannot carry the selected workspace header, but this fetcher forces workspace B onto the supplemental reset-credit request. The resulting snapshot can therefore combine workspace A's CLI usage with workspace B's reset credits; either suppress the unscoped CLI route for managed workspaces or only apply this override when the winning usage result was fetched with the same workspace scope.
Useful? React with 👍 / 👎.
clawsweeper
Bot
removed
the
status: 👀 ready for maintainer look
clawsweeper
Bot
added
rating: 🦐 gold shrimp
2 participants
Summary
main, preserving all contributor commits and authorship.codex login.Maintainer review findings
The core ownership thesis is sound, but the stale PR still had two blocking routing gaps. Automatic mode could reach an unscoped CLI fallback while a managed workspace was selected, and stale external credentials could also fall through to the CLI instead of failing closed. This repair suppresses both paths and adds regressions for each.
The production usage path has no call to
CodexOAuthCredentialsStore.saveorCodexTokenRefresher.refresh; only fixture/test code uses those helpers.CODEX_HOMEkeeps documented precedence, explicit homes never borrow external credentials, and external discovery remains opt-in and off by default. Touched logging paths carry only classifications/metadata, not access, refresh, or ID tokens. The OAuth guide no longer recommends printingauth.jsonor placing bearer tokens in shell history.Conflict resolution
ProviderArchitectureGatekeeperTestsconflict against the currentSettingsStore.swiftanchor (line 1054).0.50.1 — Unreleasedchangelog bullet and added the credited Keep shared Codex auth read-only during usage refresh #2944 Codex fix entry.CI failure investigation
libdispatch; it was not a Linux build error or a Codex auth assertion. Touched networking code already uses conditionalFoundationNetworking, and macOS-only browser code remains guarded.idleTimeoutvsdeadlineExceeded). That group and the complete sharded suite pass on this merged result.lint-build-testfailure was downstream of the failed macOS shard.Validation
swiftformat Sources Tests(then retained only task-scoped formatter changes; project-pinned lint confirms the whole tree is formatted)swiftlint --strictmake checkswift test --skip-build --filter 'CodexOAuth|CodexAuth|CodexResetCredit'— 114 macOS tests and 1 Linux compatibility test passedswift test --skip-build --filter 'ProviderArchitectureGatekeeperTests|CodexBaselineCharacterizationTests'— 48 passedmake test— 870/870 selections across 73/73 groups; zero failures, retries, or timeoutsCredit to @Yuxin-Qiao for the original implementation and ownership analysis.
Closes #2944 supersedes the original conflicting pull request.