feat: implement managed Bedrock logout#31325
Conversation
4e95e46 to
fc7cbe0
Compare
c0be3ad to
c9d1ab6
Compare
fc7cbe0 to
fa240d8
Compare
ff533e5 to
99691ca
Compare
677ef6f to
6702939
Compare
4b27932 to
3df78d4
Compare
6702939 to
01f9bf1
Compare
3df78d4 to
cb27132
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb27132d0f
ℹ️ 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".
f63bc0c to
634ab59
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15994f2071
ℹ️ 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".
| } | ||
|
|
||
| if managed_bedrock_auth { | ||
| clear_user_model_provider_if_bedrock(&self.config_manager).await?; |
There was a problem hiding this comment.
Make Bedrock config cleanup best-effort
When managed Bedrock auth is cached and config.toml becomes invalid after startup, logout_with_revoke() has already deleted auth.json before this cleanup reloads config via clear_user_value_if_matches; that error makes account/logout return a JSON-RPC error and skip account/updated even though the credential was removed. The fresh evidence is this post-removal cleanup call, so make provider cleanup best-effort (as with version conflicts) or run it before destructive auth removal.
AGENTS.md reference: AGENTS.md:L102-L110
Useful? React with 👍 / 👎.
| if config.model_provider.is_amazon_bedrock() && !managed_bedrock_auth { | ||
| return Err(invalid_request( | ||
| "cannot log out while Amazon Bedrock is using AWS-managed credentials; manage those credentials through AWS or switch model providers before logging out Codex authentication", | ||
| )); | ||
| } |
There was a problem hiding this comment.
Cancel pending logins before rejecting Bedrock logout
When a ChatGPT browser/device-code login is pending and the current config resolves to Amazon Bedrock with AWS-managed credentials, this early return runs before the existing active_login cancellation block. That leaves the pending login alive after the user asked to log out, so its callback/poll can still complete later and persist ChatGPT auth plus success notifications; cancel the active login before this provider guard or explicitly cancel on this error path.
AGENTS.md reference: AGENTS.md:L102-L110
Useful? React with 👍 / 👎.
| .load_thread_agnostic_config() | ||
| .await | ||
| .map_err(|err| ConfigManagerError::io("failed to load configuration", err))?; | ||
| let Some(user_layer) = layers.get_active_user_layer() else { |
There was a problem hiding this comment.
Clear Bedrock provider from the layer that set it
When logout runs with profile-v2 selected after managed Bedrock login wrote model_provider = "amazon-bedrock" to the base user config, this only inspects the active profile overlay. If that profile does not set model_provider, cleanup no-ops after the Bedrock API key has been deleted, so the effective config still selects Amazon Bedrock and subsequent account/read/model requests see AWS-managed Bedrock instead of returning to the default provider; clear the matching user layer (or effective user value) rather than only the active layer.
AGENTS.md reference: AGENTS.md:L102-L110
Useful? React with 👍 / 👎.
## Why App-server clients can report whether Amazon Bedrock is using AWS-managed credentials or a Codex-managed API key, but they do not have a matching API for creating the managed login. This PR defines that experimental wire contract independently from its implementation. Managed Bedrock API keys are already a primary `CodexAuth` mode. The API therefore describes a normal Codex login that replaces the current stored auth rather than introducing provider-scoped credential storage. ## What changed - Add the experimental `amazonBedrock` variant to `account/login/start`. - Accept an API key and AWS region and return a matching discriminated response. - Gate the request behind the app-server `experimentalApi` capability. - Regenerate the JSON and TypeScript protocol schemas. - Document the login contract, notifications, primary-auth replacement semantics, restart boundary, and non-transactional durable writes. ## Impact This PR defines the API shape but does not implement login behavior. The next PR adds validation, persistence through the existing Codex auth lifecycle, provider selection, and notifications. ## Validation - `just test -p codex-app-server-protocol` ## Stack 1. **#31327 Managed Bedrock experimental API** — base: `main` 2. #31326 Managed Bedrock login — base: `codex/managed-bedrock-api` 3. #31325 Managed Bedrock logout — base: `codex/managed-bedrock-login-v2`
f554c58 to
c31ae26
Compare
d2029a1 to
b8bf56b
Compare
Why
account/logoutnormally removes Codex's primary stored authentication. That behavior needs an ownership check when Amazon Bedrock is active:CodexAuthstate, so logout should remove it through the normal auth lifecycle.Managed Bedrock login also persists
model_provider = "amazon-bedrock". Logout should undo that selection only if it is still the user-level value, without overwriting a provider change made before or concurrently with logout.What changed
CodexAuth::BedrockApiKeyvalue.AuthManager::logout_with_revokepath.ConfigManager::clear_user_value_if_matchesoperation and use it to remove the user-levelmodel_provideronly while it remains"amazon-bedrock".Validation
just test -p codex-app-server -E 'test(/suite::v2::account/)'— 40 passedcargo run -p codex-app-server-test-client -- --codex-bin ./target/debug/codex test-logout:account/logoutreturned successfully,account/updatedreportedauthMode: nullandplanType: null,~/.codex/auth.jsonwas cleared, and the persistedmodel_provider = "amazon-bedrock"setting was removed from~/.codex/config.toml.Stack
maincodex/managed-bedrock-apicodex/managed-bedrock-login-v2