fix: derive Bedrock regions on provider writes#26564
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b581ae94de
ℹ️ 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".
Bedrock create validation accepted any base_url as sufficient, so a native type=bedrock provider with a non-canonical endpoint and no region or credentials could be persisted, then silently skipped at runtime where bedrockConfigFromRow requires settings.Bedrock.IsConfigured(). Validate Bedrock readiness on the write path after region derivation: canonical AWS endpoints still derive a region for free, while custom endpoints must supply a region or credentials. Updates that switch a Bedrock provider to a non-derivable endpoint without a region are rejected too.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a1c1c0042
ℹ️ 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".
The previous guard only fired for type=bedrock, but the UI creates Bedrock providers as type=anthropic with a settings.bedrock discriminator. After the frontend began accepting non-canonical endpoints, a form submit with a custom base_url and blank credentials persisted a Bedrock settings blob with no region. At runtime bedrockConfigFromRow returns nil for such a row, so the enabled provider is silently dropped or treated as a plain Anthropic provider. Gate create and update validation on settings.Bedrock != nil rather than the provider type, so any write carrying Bedrock settings must resolve to usable config after region derivation. This matches the single invariant aibridge already enforces. The two near-duplicate reject tests are merged into one table-driven subtest.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fb3842c34
ℹ️ 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 req.BaseURL != nil && existing.Bedrock != nil { | ||
| if req.Settings == nil || req.Settings.Bedrock == nil || req.Settings.Bedrock.Region == "" { | ||
| existing.Bedrock.Region = "" | ||
| } |
There was a problem hiding this comment.
Preserve explicit regions on Bedrock UI edits
For an existing Bedrock provider that uses a custom/non-canonical endpoint with an explicit settings.bedrock.region (for example, created through API/Terraform with ambient AWS credentials), a normal UI edit sends base_url on every PATCH but cannot send a region because the form has no region field. This block erases the saved region just because base_url is present; since custom URLs cannot be re-derived, the PATCH is rejected with Bedrock settings require a region or credentials for ambient-credential providers, or silently drops the region when stored credentials keep IsConfigured() true. Preserve the existing region unless the request explicitly changes it or a canonical endpoint is re-derived.
Useful? React with 👍 / 👎.
Bedrock provider region derivation now happens in the API write path instead of the React provider form. Create and update requests normalize canonical AWS Bedrock runtime endpoints into
settings.bedrock.region, while preserving explicitly supplied regions and leaving custom endpoints to provide region separately.This came up while I was working on the coderd provider Terraform resource for creating AI providers on a Coder deployment. Doing this derivation in the frontend means every official client that creates providers has to repeat the same endpoint parsing, which felt backwards. It belongs on the backend, where every client gets the same behavior for free.
This also lets native
type=bedrockproviders validate with a base URL only, and relaxes the UI endpoint validation so clients are not limited to canonical AWS endpoints.Generated by mux, reviewed by a human.