fix: render Bedrock agents models with Bedrock identity#26038
fix: render Bedrock agents models with Bedrock identity#26038ibetitsmike wants to merge 4 commits into
Conversation
|
/coder-agents-review |
|
Chat: Review in progress | View chat deep-review v0.7.1 | Round 6 | Last posted: Round 6, 21 findings (1 P1, 4 P2, 9 P3, 7 Nit), APPROVE. Review Finding inventoryFinding InventoryFindings
Round logRound 1Panel. Netero clean. 1 P1, 1 P2, 8 P3, 6 Nit. Reviewed against 167ac7b..d61e76c. Round 2Churn guard: PROCEED. 16/16 addressed. Netero clean. Panel: 2 P2, 1 P3 new. 1 dropped. Reviewed against 167ac7b..033b690. Round 3Churn guard: PROCEED. 3/3 addressed. Netero clean. Panel: 1 P2 new. Reviewed against 167ac7b..55c854d. Round 4Churn guard: PROCEED. 1/1 addressed. Netero: 1 Nit. Panel: 0 new findings. APPROVE. Reviewed against 167ac7b..899d3d0. Round 5BLOCKED. CRF-22 (Nit) silent. No review. Head SHA unchanged (899d3d0). Round 6Churn guard: PROCEED. 1/1 addressed. All 22 findings resolved. Netero clean. Panel clean. APPROVE. Reviewed against 167ac7b..9b7bae6. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
The resolution approach is well-designed: resolveAIProviderType in db2sdk as the single source of truth, threaded through every serialization and routing path. The seed migration writing type=bedrock for new rows is correct and ensures progressive data normalization. Test coverage for the list endpoint, seed paths, and Storybook is proportional. The frontend displayProvider fallback is a good defensive layer.
1 P1, 1 P2, 8 P3, 6 Nit.
The P1 is a regression: computer use breaks for Bedrock-only deployments because aiProviderForProviderType now matches on effective type ("bedrock") but computer use hardcodes "anthropic" as the lookup key.
The P2 is a blast-radius concern: effectiveAIProviderTypesByID loading all providers (including deleted) and failing hard on any corrupt settings blob takes down listChatModelConfigs for every user, a path that was previously infallible.
"I tried to build a case against this approach and couldn't find a P0 or P1 issue." (Pariston, before Mafuuu found the computer use regression. The panel works.)
coderd/x/chatd/model_routing_aibridge.go:269
P3 [CRF-9] resolveAIGatewayModelRouteForProviderType passes the caller's raw type as modelProviderHint, not the resolved effective type.
The function passes chatprovider.NormalizeProvider(providerType) (the caller's input) to resolveAIGatewayRoute. The sibling resolveAIGatewayModelRouteForConfig passes effectiveAIProviderName(provider). If a caller asks for "anthropic" and the matched provider's effective type is "bedrock", the hint says "anthropic" but the provider is Bedrock-backed. Impact is limited to logging and telemetry (routing itself uses fantasyConfigForAIBridge(effectiveProviderType) separately). (Mafuuu)
🤖
🤖 This review was automatically generated with Coder Agents.
d61e76c to
033b690
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
R1 findings addressed comprehensively. The two-pass matching pattern for CRF-1, graceful degradation for CRF-2, write-time normalization for CRF-3, and the constant extraction for CRF-14 all went beyond what was asked. Test density jumped from 23.5% to 42.7%.
2 P2, 1 P3 new this round.
CRF-17 (P2): The new LinkedBedrockProviderUsesBedrockIdentity test fails at line 3528, so the update and list verification paths never execute. One-line fix.
CRF-18 (P2): convertAIProviderSummary still hard-fails on corrupt settings, the same class of bug as CRF-2 on a sibling endpoint (BYOK key listing).
"The fix CRF-2 applied to the model config listing should be mirrored here." (Hisoka)
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
R2 findings addressed cleanly. convertAIProviderSummary refactored to use graceful fallback, test dbauthz context fixed, bestEffortAIProviderType now logs. All verified.
1 P2 new this round.
CRF-21 (P2): The CRF-1 fix (two-pass matching) bridges legacy type=anthropic rows for computer use. But the CRF-3 fix (write-time normalization) now stores type=bedrock in the DB for new providers. Neither pass matches when computer use looks for "anthropic" on these rows. Computer use for Bedrock-only was already broken before this PR (no API keys, ProviderAllowsAmbientCredentials("anthropic") is false), so this is an incomplete fix, not a regression. The test TestResolveDirectModelRouteForProviderTypeFallsBackToRawTypeForBedrock only covers the legacy type=anthropic case.
"The root cause is that computer use's 'anthropic' lookup string has no relationship to the 'bedrock' type the system now writes." (Mafuuu)
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
|
/coder-agents-review set-spend-limit:250 |
There was a problem hiding this comment.
All 21 findings resolved across 4 rounds. CRF-21 fix verified: aiProviderTypeCanSatisfyRequest correctly models the asymmetric relationship (bedrock satisfies anthropic, not vice versa), and the effective type is now passed to credential resolution.
1 Nit remaining (CRF-22, stale test name).
22 findings total: 1 P1, 4 P2, 9 P3, 7 Nit, 1 dropped. 21 addressed, 1 open Nit. Test density rose from 23.5% (R1) to 47.4% (R4). The centralized type resolution design is clean, the graceful degradation handles corrupt data, and the write-time normalization prevents new dual-representation rows.
"I tried to build a case against this change and couldn't. The problem is correctly understood, the solution is proportional, and the fix is at the right level." (Pariston)
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review set-spend-limit:250 |
|
/coder-agents-review set-spend-limit:250 |
There was a problem hiding this comment.
CRF-22 (Nit, stale test name TestResolveDirectModelRouteForProviderTypeFallsBackToRawTypeForBedrock) is unresolved with no author response. The PR was approved in R4 with this Nit open. Rename to reflect that the test now exercises the effective-type match path, not the raw-type fallback, or acknowledge it as intentional.
🤖 This review was automatically generated with Coder Agents.
There was a problem hiding this comment.
All 22 findings resolved across 6 rounds. CRF-22 (test rename) addressed in 9b7bae6. No new findings from Netero or the panel.
22 findings total: 1 P1, 4 P2, 9 P3, 7 Nit, 1 dropped. All addressed. Test density: 47.4%. The centralized type resolution, graceful degradation on corrupt settings, write-time normalization, and asymmetric provider-type matching are all clean.
🤖 This review was automatically generated with Coder Agents.
Bedrock-backed Agents model configs were still serialized and rendered with Anthropic provider identity when the linked AI provider stored Bedrock settings.
This PR resolves effective Bedrock provider identity on the backend for API summaries, chat model configs, availability, and chatd routing while keeping Bedrock runtime calls on the Anthropic-compatible schema. It also lets Anthropic computer use requests use first-class Bedrock providers, updates the Agents model admin list to render linked provider presentation, adds graceful fallback logging for unreadable provider settings, and adds regression coverage.
Refs CODAGT-549