Resolve and pin MCP OAuth credential stores - #30293
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c432fcdffc
ℹ️ 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".
c432fcd to
38b9e10
Compare
c99bdc7 to
e4dcca6
Compare
c003752 to
c93f236
Compare
e4dcca6 to
f1e1c17
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review — Codex |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
[Codex Thread 019edd6d-6f14-74e2-853c-345d1803d4a6](https://codex-thread-link.openai.chatgpt-team.site/thread/019edd6d-6f14-74e2-853c-345d1803d4a6) ## Stack Review and merge in order. Every layer is independently correct and documents its safe stopping point. 1. [#30292](#30292) — aggregate File/Secrets store locking 2. [#30293](#30293) — resolve and lifecycle-pin the exact OAuth store 3. [#30416](#30416) — serialized authoritative refresh transaction 4. [#30294](#30294) — Codex-owned transport refresh and one-shot 401 recovery 5. [#30295](#30295) — login/logout transaction serialization 6. [#30296](#30296) — diagnostic-only Auto store drift reporting **This PR is layer 1.** ## Why MCP OAuth credentials stored in File or Secrets share one aggregate map. Concurrent read-modify-write operations for different MCP servers can both read the same snapshot and let the later write discard the earlier update. That is a correctness problem independent of refresh-token rotation. ## What this PR does - Adds a bounded cross-process lock around aggregate File and Secrets loads, saves, and deletes. - Distinguishes aggregate-lock failures from Secrets backend unavailability, so Auto can fall back only for the latter and cannot bypass serialization by reading or writing File. - Keeps Direct keyring operations outside this lock because they are already per credential. - Releases the Secrets aggregate lock before legacy File cleanup so cross-store cleanup cannot create nested aggregate-lock ordering. - Tests actual contention by waiting for an observed `WouldBlock`, rather than assuming a sleeping worker reached the lock. - Tests load and save with only the Secrets lock path broken while fallback File remains readable and writable. ## Decisions and non-goals - This lock protects aggregate-store read-modify-write integrity only. It does not choose a credential authority or serialize an OAuth refresh transaction. - The lock is scoped to the active `CODEX_HOME`, matching the aggregate files it protects. - Lock waits are bounded, and coordination failures are surfaced rather than treated as evidence that Secrets is unavailable. ## Safe stopping point This PR can merge alone. It prevents lost updates and partial aggregate reads. Auto can still resolve again during a client lifecycle until layer 2, and concurrent refreshes remain possible until layer 3. ## Validation - `just test -p codex-rmcp-client` (96 passed; expected environment skips) - Focused aggregate File/Secrets lock contention and Auto fallback tests
fb2c35f to
e5dc9f3
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5dc9f3632
ℹ️ 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".
|
@codex review — Codex |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review — Codex |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Codex Thread 019edd6d-6f14-74e2-853c-345d1803d4a6
Stack
Review and merge in order. Every layer is independently correct and documents its safe stopping point.
This PR is layer 2.
Why
Autois keyring-first with a File fallback, but re-evaluating that policy during transport reconstruction or persistence can make one MCP client read from one store and later write to another. With rotating refresh tokens, the second store may contain an older token. This layer makes the source selected at client startup explicit and keeps that authority stable for the client lifecycle.What this PR does
resolve_oauth_tokens_from_store_policyas the single configured-policy entry point and returns both credentials and the concrete File or Keyring source that supplied them.load,save, anddeleteoperations onResolvedOAuthCredentialStore, making “resolve configured policy” and “use the selected authority” distinct at call sites.pinned_credential_storein the transport recipe, so initialization retries and session reconstruction cannot re-evaluateAutoand adopt another store.OAuthPersistorthe resolved store and keeps subsequent persistence and removal on that authority.Autobehavior unchanged: prefer Keyring, fall back to File when unavailable, and clean up legacy File state after a successful keyring save.Explicit decisions and non-goals
CODEX_HOME.Automay choose File at the start of a later process if keyring availability changes. Once this client resolves, a selected-store failure is returned instead of hot-switching.CODEX_HOMEinstances remain independent even when they can access the same Direct keyring credential.Safe stopping point
This PR can merge alone. A single MCP client no longer hot-switches credential stores across transport rebuilds or persistence. Two processes can still refresh the same selected credential concurrently until layer 3.
Review size
The net layer is 9 files, +668/−144. The production change remains focused on store resolution and lifecycle pinning; the largest follow-up is integration coverage that drives real session recovery.
Validation
just test -p codex-rmcp-client(99 passed; 5 expected skips)