P2 [CRF-4] Unknown protocol values pass API validation and silently degrade to unsigned requests at runtime.

Validate() checks that mantle requires a region but never rejects an unknown Protocol string. An operator who sets protocol: "mnatle" (typo) passes validation. At runtime, isBedrockInvokeModel() matches only "" and "invoke-model", isBedrockMantle() matches only "mantle". The unknown value matches neither, so neither signing branch in newMessagesService fires. The request proceeds to the upstream unsigned.

config.AWSBedrock.Validate() has a default reject clause, but it lives inside withBedrockInvokeModelOptions and withBedrockMantleOptions, which are never entered because the dispatch predicates already excluded the unknown value. The validation is unreachable for this failure mode.

"An operator who sets protocol: "tyop" passes validation. The value is stored. At request time, the request falls through to the generic Anthropic path with no SigV4 signing. AWS returns 403, and the operator gets a confusing error with no mention of the invalid protocol." (Pariston P3)

The same gap applies to UpdateAIProviderRequest.Validate() (line 335), which does not inspect the protocol field at all.

Fix in both Create and Update validators:

switch req.Settings.Bedrock.Protocol {
case "", AIProviderBedrockProtocolInvokeModel, AIProviderBedrockProtocolMantle:
default:
    validations = append(validations, ValidationError{
        Field:  "settings.protocol",
        Detail: fmt.Sprintf("unsupported bedrock protocol %q", req.Settings.Bedrock.Protocol),
    })
}

(Hisoka P2, Mafuuu P2, Knov P2, Meruem P2, Ryosuke P2, Pariston P3, Kurapika P3, Melody P3, Chopper P3)

🤖

Fixed: 6cd6d58

P3 [CRF-2] UpdateAIProviderRequest.Validate() (line 335) does not check the protocol field at all. An update that sets protocol=mantle without providing a region, or sets an unknown protocol value, passes validation. The same protocol-enum check from CRF-4 and the mantle-region check from line 287 should be replicated in the Update validator.

(Netero, Hisoka, Knov, Meruem)

🤖

Fixed:

• 6cd6d58
• 00e8b76

P3 [CRF-1] Missing nil guard on i.bedrock. The sibling function withBedrockInvokeModelOptions (line 323) guards with if i.bedrock == nil { return nil, xerrors.New("nil bedrock runtime") }. This function dereferences i.bedrock.Cfg directly. The caller guards via isBedrockMantle() which requires i.bedrock != nil, so this won't panic in production. The risk is a nil-dereference panic if called directly without the caller guard.

(Netero)

🤖

Fixed: 7419630

P3 [CRF-5] Test claims passthrough but only asserts Model().

The payload includes thinking, metadata, and context_management fields that augmentRequestForBedrockInvokeModel would strip or rewrite for InvokeModel. The point of mantle is that these fields survive untouched, but no assertion verifies it. If someone accidentally added augmentation to the mantle path, Model() would still return the client's model (it reads from reqPayload for mantle, from config for InvokeModel), and this test would still pass.

Add an assertion that i.reqPayload is unchanged after the mantle code path runs, or remove the fields that aren't being tested.

(Bisky)

🤖

Fixed: 4db66ee

P3 [CRF-6] This test is titled "InvokeModelDoesNotRequireRegionField" but Settings: codersdk.AIProviderSettings{} leaves Bedrock nil, so Validate() never enters the if req.Settings.Bedrock != nil block. The protocol check is never reached.

Set Bedrock: &codersdk.AIProviderBedrockSettings{} (zero value resolves to InvokeModel via ResolvedProtocol()) so the test enters the protocol-gated branch and proves the region check is scoped to mantle.

(Bisky)

🤖

Fixed: 0db20ff

Nit [CRF-19] hasFieldError is a manual loop that slices.ContainsFunc (available since Go 1.21) replaces. The project already uses it in tests.

hasFieldError := func(vs []codersdk.ValidationError, field string) bool {
    return slices.ContainsFunc(vs, func(v codersdk.ValidationError) bool {
        return v.Field == field
    })
}

(Ging-Go)

🤖

Re-raising [CRF-19]: this round added two more hand-rolled []ValidationError loops (hasProtocolError and another hasFieldError) rather than reducing the pattern. slices.ContainsFunc (Go 1.21, go.mod is 1.26) collapses each to one line. Still a nit, not a bug; flagging only because the sibling count grew.

🤖

Nit [CRF-20] context.Background() used where t.Context() is available since Go 1.24. The same PR's integration test already uses t.Context() for the mantle path.

(Ging-Go)

🤖

Nit [CRF-3] Grammar: "Empty fallbacks to invoke-model" should be "Empty falls back to invoke-model." ("fallbacks" is a noun; the verb form is "falls back.")

(Netero)

🤖

Nit [CRF-23] time.Second*30 is a bare literal in both the mantle block (here) and the InvokeModel block (line 287). A named constant like bedrockCredentialTimeout would make the intent clear and reduce the duplication.

(Gon)

🤖

Nit [CRF-21] BedrockPRMUserAgent doc comment doesn't expand PRM. PRM is the ISV Partner Registration Marker (or Partner Registration Manager, depending on the AWS doc vintage). The value itself is a percent-encoded APN token, adding another unexplained acronym. A one-line expansion in the doc comment saves someone a search:

// BedrockPRMUserAgent is Coder's AWS Partner Registration Marker (PRM),
// appended to Bedrock User-Agent headers for AWS ISV attribution.

(Leorio)

🤖

Do we ever set any upper limit on request body before this? If not, is that something to consider?

We set it here: https://github.com/coder/coder/blob/main/aibridge/bridge.go#L53 - 32MiB.

In what cases would callers be passing a nil request body here?

The body shouldn't be nil here; this is just a defensive check to avoid a panic in io.ReadAll function.

Just checking: do we need any validation around Settings.Bedrock.Region, or is just ensuring non-empty sufficient? My gut says it is.

I think ensuring it's non-empty is sufficient. That's what we've done throughout the codebase.

P3 [CRF-25] Protocol dispatch fails open: an unknown Protocol on a non-nil bedrock runtime matches neither branch, so newMessagesService builds a client with no AWS signing and forwards the request unsigned.

The CRF-4 fix (marked Author fixed) closed the codersdk API entry point via validateAIProviderBedrockProtocol. The runtime sink is still open. isBedrockInvokeModel() matches ""/"invoke-model", isBedrockMantle() matches "mantle"; the two if blocks here have no else/default. For any other value, neither withBedrockInvokeModelOptions nor withBedrockMantleOptions runs, so AWSBedrock.Validate() (whose default case rejects unknown protocols) is never reached, and the request goes out with only the generic WithBaseURL from line 269, unsigned.

"a newer coderd validates and stores a future protocol value (e.g. \"mantle-v2\"); the binary is rolled back to this version. ResolvedProtocol() passes the value through verbatim, bedrockConfig casts it into the runtime config unchanged, and every request for that provider is sent unsigned. The operator sees opaque upstream 403s, not a config error." (Mafuuu P3)

Rated P3 because API validation blocks the value at write time today; this is the defense-in-depth gap that makes drift, version skew, or a direct settings write fail open silently instead of loud. Fix: make the dispatch exhaustive, a switch on the protocol guarded by i.bedrock != nil with a default that returns an error, or call cfg.Validate() once before the branches so an unrecognized protocol fails closed at signing.

(Meruem P3, Kurapika P3, Mafuuu P3)

🤖

Fixed here: #27234

Nit [CRF-28] BedrockPRMUserAgent names a User-Agent suffix, not a full User-Agent. The value is appended (ua+" "+BedrockPRMUserAgent), and the only assembler (appendBedrockPRMUserAgent) is unexported, so someone reaching for the exported const could reasonably write req.Header.Set("User-Agent", BedrockPRMUserAgent) and ship a malformed UA. Rename to BedrockPRMUserAgentSuffix (or ...Marker) so the name states what it is; the doc comment already calls it an appended marker.

(Gon)

🤖

Nit [CRF-27] The same Validate() function names one field two ways: the InvokeModel branch returns "region or base url required" (space, prose) and the new mantle branch returns "base_url required" (underscore, field form). Only base_url matches what the operator types in settings. The base url string is pre-existing, but the mantle branch is new and sits a few lines away, so aligning them is this PR's to make. Suggest "region or base_url required" so both point at the field the operator edits.

(Leorio)

🤖

Nit [CRF-26] The two "mantle valid" rows (official api prefix at L67, proxy api prefix at L74) both land in case BedrockProtocolMantle, which only checks Region != "" and BaseURL != "". The BaseURL value never changes the branch, so the second row adds no coverage, and the official/proxy names imply the validator distinguishes an official bedrock-mantle.*.api.aws prefix from an arbitrary proxy URL. It does not. A reader who later adds prefix validation will believe it is already tested. Collapse to one row, or rename both to describe what is actually asserted (any non-empty base URL passes).

(Bisky)

🤖