review fixes + refactoring for 26035#26121
Conversation
…e unnecessary casts
|
/coder-agents-review |
|
Chat: Review in progress | View chat deep-review v0.7.1 | Round 3 | Last posted: Round 3, 9 findings (1 P2, 3 P3, 5 Nit), APPROVE. Review Finding inventoryFindings
Contested and acknowledgedCRF-2 (P3, chatd.go:8661) - Best-effort fallback pattern duplicated across 4 call sites
Round logRound 1Panel. 1 P2, 2 P3, 4 Nit. 1 dropped. Reviewed against 3b7ca8b..a3bb9f5. Round 2Churn guard: PROCEED. 7/7 addressed. CRF-2 contested, panel closed 7/7. 1 new P3 (CRF-9). Reviewed against 3b7ca8b..97c75d5. Round 3Churn guard: PROCEED. CRF-9 addressed with canonicalizedProvider struct. 1 new Nit (CRF-10). Reviewed against 3b7ca8b..06fdfc0. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
There was a problem hiding this comment.
Clean refactoring that removes six wrapper functions across two packages, changes db2sdk.CanonicalAIProviderType to return codersdk.AIProviderType (the right layer for a runtime concept), eliminates double-canonicalization in resolveAIGatewayRoute, and fixes a genuine bug in error reporting at ai_providers_migrate.go:144.
The test coverage is strong: 74.5% test density, every new error path is covered, and the TestEnabledProviderContainsName table-driven suite exercises 8 distinct code paths through the canonical/raw-type fallback logic.
1 P2, 2 P3, 4 Nit.
The P2 is a missed sibling of the bug fix the PR itself introduced: dp.Name at line 150 should be existing.Name, same as the fix at line 144. The P3s cover a test assertion that could be sharper and a question about whether the inlined best-effort fallback pattern (4 identical copies) should be re-consolidated.
"Boring code. Well-structured refactoring. The wrappers added nothing but indirection, and removing them reveals the actual data flow." (Hisoka)
coderd/ai_providers_migrate.go:150
P2 [CRF-1] The PR fixed the same class of bug at line 144 (dp.Name → existing.Name), but this adjacent error was missed. GetAIProviderKeysByProviderID loads keys for existing.ID, but the error reports dp.Name. When the Bedrock candidate loop (lines 119-131) reassigns existing to a row found via byName["anthropic"], existing.Name differs from dp.Name, misdirecting an operator debugging a key-loading failure.
The PR correctly fixed the sibling at line 144, but the adjacent error on line 150 was missed. In the Bedrock migration path,
existingcan be set to a candidate found viabyName[aibridge.ProviderAnthropic], soexisting.Namemay differ fromdp.Name. (Leorio P2, Razor P3)
Fix: dp.Name → existing.Name.
🤖
🤖 This review was automatically generated with Coder Agents.
|
CRF-1 (P2): Fixed — |
|
/coder-agents-review |
There was a problem hiding this comment.
Round 2. All R1 findings addressed.
CRF-1 (P2, dp.Name bug): fixed. CRF-3 (P3, require.Error): fixed with ErrorContains. CRF-4, CRF-5, CRF-6, CRF-7 (Nits): all fixed.
CRF-2 (P3, best-effort fallback duplication): panel unanimously accepts the author's defense (7/7). The differentiated warn messages ("for provider config", "for direct route", "for AI gateway route", etc.) address the diagnostic concern. The call sites serve three distinct error-handling modes (best-effort, skip, hard-error) that a unified helper would blur. Closed.
1 new P3.
"I tried to build a case against this change and could not. The problem is correctly understood across all four framings." (Pariston)
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
Round 3. All prior findings resolved.
CRF-9 (P3, re-canonicalization in newAIGatewayModel): fixed. The author went further than suggested, introducing a canonicalizedProvider struct that pairs the database row with its pre-computed canonical type. This makes re-canonicalization structurally impossible. Clean design.
1 new Nit (comment wording).
"I tried to build a case against this change. The premises hold. The problem is correctly understood, the solution is proportional, and the fix is at the right causal level." (Pariston)
🤖 This review was automatically generated with Coder Agents.
There was a problem hiding this comment.
Pull request overview
This PR refactors AI provider “canonical type” handling across chatd routing and related API conversion paths, standardizing on db2sdk.CanonicalAIProviderType (now returning codersdk.AIProviderType) and adding tests around Bedrock/Anthropic legacy settings and malformed provider settings.
Changes:
- Switch canonical provider type derivation to
db2sdk.CanonicalAIProviderTypeacross chatd routing/config codepaths, with consistent warn-and-fallback behavior for malformed settings in most cases. - Refactor AI Gateway routing to carry a pre-computed canonical provider type (
canonicalizedProvider) rather than re-deriving it at each use site. - Add/adjust internal tests to cover canonical Bedrock behavior, malformed settings fallbacks, and provider-name/type matching.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| coderd/x/chatd/subagent.go | Uses db2sdk.CanonicalAIProviderType when resolving model configs/providers; improves warning message specificity. |
| coderd/x/chatd/subagent_internal_test.go | Adds table-driven coverage for enabledProviderContainsName including malformed settings and Bedrock/Anthropic legacy behavior. |
| coderd/x/chatd/model_routing.go | Adjusts AI Gateway provider hint storage to use canonicalizedProvider.CanonicalType. |
| coderd/x/chatd/model_routing_internal_test.go | Updates route construction and provider-type mapping tests to use codersdk.AIProviderType. |
| coderd/x/chatd/model_routing_direct.go | Canonicalizes provider type for direct routes via db2sdk, with fallback logging; makes config-by-ID canonicalization strict. |
| coderd/x/chatd/model_routing_aibridge.go | Introduces canonicalizedProvider and switches AI Gateway routing/model construction to use precomputed canonical provider type. |
| coderd/x/chatd/chatd.go | Standardizes provider canonicalization in provider config + key resolution; adjusts fallback behavior for malformed settings. |
| coderd/x/chatd/chatd_internal_test.go | Adds tests for canonical Bedrock routing and malformed settings behavior in direct/AIGateway/config paths. |
| coderd/x/chatd/ai_provider_canonical.go | Simplifies canonical/raw match helpers to rely on db2sdk.CanonicalAIProviderType and codersdk constants. |
| coderd/exp_chats.go | Switches AI provider summary/config conversion to use db2sdk.CanonicalAIProviderType returning codersdk.AIProviderType. |
| coderd/database/db2sdk/db2sdk.go | Changes CanonicalAIProviderType to return codersdk.AIProviderType directly. |
| coderd/database/db2sdk/db2sdk_test.go | Updates expectations for new return type from CanonicalAIProviderType. |
| coderd/ai_providers.go | Inlines canonicalization conversion where helper was removed. |
| coderd/ai_providers_migrate.go | Updates canonicalization usage and fixes error messages to reference the correct provider name. |
| coderd/ai_provider_canonical.go | Removes now-redundant canonicalization helpers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (r resolvedModelRoute) withProviderHint(providerHint string) resolvedModelRoute { | ||
| switch r.kind { | ||
| case modelRouteKindDirect: | ||
| r.direct.ProviderHint = providerHint | ||
| case modelRouteKindAIGateway: | ||
| r.aiGateway.ModelProviderHint = providerHint | ||
| r.aiGateway.Provider.CanonicalType = codersdk.AIProviderType(providerHint) | ||
| } |
No description provided.