P2 [CRF-4] Construction validates the raw bedrockCfg before region resolution, so it judges a different config snapshot than the two request-time gates. (Hisoka P2, Ryosuke P3)

Two Validate() calls that look identical but judge different config.

Traced and confirmed: Validate() runs here at line 72 on bedrockCfg (Region == ""), but buildBedrockCredentials (line 75) resolves the region from the AWS environment and it lands in runtimeCfg.Region only afterward (lines 82-84). The request-time gate (base.go:377) validates that resolved runtimeCfg. For mantle, Validate() requires Region != "" unconditionally (config.go:97-99), so a mantle provider that relied on AWS_REGION/IMDS for its region constructed and served before this PR and now fails construction with bedrock config: region required. Invoke-model escapes only because its Region=="" && BaseURL=="" guard is duplicated in buildBedrockCredentials; mantle has no such escape.

Fix: move the Validate() call to run on runtimeCfg after line 84. Both gates then read the same config, and the per-request Validate() calls at base.go:336/:377 become redundant and removable.

🤖

Fixed

P3 [CRF-1] The construction-time Validate() gate, the PR's stated purpose, ships with no test exercising its error path. (Netero, Mafu-san; Bisky Note)

Per-block coverage reports anthropic.go:72.47,74.4 at count 0. Every existing caller passes a valid config and asserts require.NoError; nothing asserts NewAnthropic rejects an invalid bedrock config. The white-box harness in anthropic_internal_test.go already builds config.AWSBedrock cases, so a table row passing an invalid config and asserting bedrock config: ... is a few lines. Consequence is bounded (request-time Validate() still catches it on first request), hence P3, but the fix's stated value, catching it at construction, is exactly what nothing guards.

🤖

Fixed

P3 [CRF-2] The new aws_bedrock_protocol trace branch, the PR's only observable behavior, has 0% coverage, and ResolvedProtocol()'s empty->invoke-model mapping never runs under test. (Bisky P3; Netero Note)

go tool cover reports both baseTraceAttributes (base.go:166) and ResolvedProtocol (config.go:76) at 0.0%

If the mapping regresses (emits "", or the branch stops firing), spans silently carry a missing or wrong protocol and nobody notices until someone debugs a Bedrock incident with bad telemetry. Blast radius is observability only, hence P3, but the gap is permanent absent a test. Bisky wrote and ran a green sketch: build an interceptionBase with a bedrock config, assert baseTraceAttributes emits invoke-model for the empty protocol and omits the attribute when bedrock == nil. One test covers the resolver, the append branch, and the nil-guard.

🤖

P3 [CRF-5] ResolvedProtocol() centralizes the empty->InvokeModel mapping but is wired to only the trace attribute, leaving the same invariant hand-rolled in two other places. (Mafuuu P3, Ryosuke P3, Chopper Nit, Razor Nit)

The empty-value default now lives in three spots: here, isBedrockInvokeModel() (base.go:143, Protocol == "" || Protocol == BedrockProtocolInvokeModel), and Validate() (config.go:86, case "", BedrockProtocolInvokeModel). Behavior is identical today, so nothing breaks now. The consequence is latent divergence: if the zero-value default ever changes, a maintainer updating ResolvedProtocol() leaves the routing predicates disagreeing, so Model()/dispatch route down one protocol while the trace attribute reports the other. Route isBedrockInvokeModel()/isBedrockMantle() through ResolvedProtocol() so the invariant lives in one place.

🤖

Fixed

Nit [CRF-6] The ResolvedProtocol doc says "legacy" twice in one sentence. (Gon)

"mapping the empty value to the legacy InvokeModel protocol so existing providers keep the legacy behavior." Drop the first: "returns the configured protocol, or BedrockProtocolInvokeModel when unset, so existing providers keep legacy behavior."

🤖

Nit [CRF-7] ResolvedProtocol collapses to return cmp.Or(c.Protocol, BedrockProtocolInvokeModel). (Ging-Go)

BedrockProtocol is a string type, so cmp.Or (Go 1.22+) returns c.Protocol when non-empty else the default, identical behavior with no side effects. go.mod is on 1.26, and cmp.Or is already used in the tree (e.g. aibridge/internal/testutil/mockupstream.go:214).

🤖

needs to be a nolint

It passed CI

Potentially a larger refactor, so take it or leave it: unexport c.Protocol and rename this method Protocol() so that callers are forced to use this method that handles the zero value.

Alternatively, make BedrockProtocolInvokeModel the empty string?

Potentially a larger refactor, so take it or leave it: unexport c.Protocol and rename this method Protocol() so that callers are forced to use this method that handles the zero value.

I think this requires too many changes. We'd need to add a constructor and update dozens of call sites.
It also goes against the pattern we use in config/config.go, where all fields are exported.

Alternatively, make BedrockProtocolInvokeModel the empty string?

That's an interesting idea—I hadn't considered it when I first implemented this.
At this point, though, it would require additional work, since we'd also need to update the codersdk layer.
I'm also not fully convinced it's an improvement. An empty string can be confusing at the API level.

I wouldn't consider this confusing:

type BedrockProtocol string
const (
  BedrockProtocolDefault     = ""
  BedrockProtocolInvokeModel = "invoke-model"
  BedrockProtocolMantle      = "mantle"
)

...

  switch (proto) {
    case BedrockProtocolDefault, BedrockProtocolInvokeModel:
      // handle invoke-model
    case BedrockProtocolMantle:
      // handle mantle
    default:
      // unknown, error
  }

But it's fair enough to leave it as a follow-up. We do still have a window to change the bedrock protocol stuff around before the next release though!

I thought you mean:

type BedrockProtocol string
const (
  BedrockProtocolInvokeModel = ""
  BedrockProtocolMantle      = "mantle"
)

...

  switch (proto) {
    case BedrockProtocolInvokeModel:
      // handle invoke-model
    case BedrockProtocolMantle:
      // handle mantle
    default:
      // unknown, error
  }