feat: route aibridge through ai_providers IDs#25606
Conversation
Docs preview📖 View docs preview for |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
ab0d49f to
0e7f01a
Compare
7b99bcd to
b546f4a
Compare
b546f4a to
a090eae
Compare
0e7f01a to
ef6ee2a
Compare
c85bf0b to
be0b8d1
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
First-pass review (Netero only). This is a mechanical scan; the full review panel has not yet reviewed this PR. The panel will review after these findings are addressed.
The migration strategy (two-pass backfill, fallback filter in SQL, ON DELETE SET NULL) is well-structured. The provider interface extension is clean, and the CLI flag mutual-exclusivity check is a good UX detail.
1 P2, 2 P3, 1 Nit.
"Zero test coverage." — Netero, staring at BuildProvidersFromDB
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
dannykopping
left a comment
There was a problem hiding this comment.
Addressed all review comments in 63fcc71.
There was a problem hiding this comment.
Round 2 (Netero + panel of 17). R1 findings DEREM-1 through DEREM-4 all addressed cleanly in 63fcc71, with generalized sibling fixes and appropriate test coverage.
The migration strategy, provider interface extension, and SQL fallback design are solid. The release note warning for large tables is appreciated.
1 P1, 3 P2, 3 P3, 3 Nit.
The P1 is a migration blocker: the backfill casts free text to an enum, which aborts on any value outside the enum (copilot is the known case). The P2s are an unsupported-type server startup failure, an RBAC permission gap for auditors, and zero test coverage on the SQL legacy-row fallback.
"Enum-to-text never fails. Text-to-enum is the one that breaks." — Hisoka
cli/aibridged.go:188
P3 [DEREM-5] BuildProviders (env-driven constructor) has zero production callers after this PR. Both cli/server.go and enterprise/cli/server.go now call BuildProvidersFromDB. The function, its ~174 lines, and its dedicated tests remain as dead code. (Netero, Zoro P4)
🤖
enterprise/coderd/aibridge.go:107
Nit [DEREM-14] Swagger @Param annotation still lists provider as an available filter key; the actual key is now provider_id. Same at line 222 for sessions. (Chopper, Knov P3)
🤖
🤖 This review was automatically generated with Coder Agents.
63fcc71 to
8b2c09c
Compare
8b2c09c to
73a836a
Compare
73a836a to
98d68fd
Compare
56bf0c1 to
0d9718e
Compare
98d68fd to
bcaf3ed
Compare
bcaf3ed to
30e7ff0
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
Round 3 blocked. Two findings from round 2 have no author response:
DEREM-5 (P3): BuildProviders (env-driven constructor at cli/aibridged.go:188) has zero production callers after this PR. Both server paths now call BuildProvidersFromDB. The function (~174 lines) and its dedicated tests remain as dead code. Response needed: remove, or acknowledge with a ticket for later cleanup.
DEREM-14 (Nit): Swagger @Param annotation at enterprise/coderd/aibridge.go:107 (and line 222 for sessions) still lists provider as an available filter key; the actual key is now provider_id. API consumers following the generated docs will use the wrong key.
Both findings were folded into the review body (not inline threads) because their lines were outside the diff. Further review is blocked until the author responds or pushes fixes for these items.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
|
Review in progress | Chat deep-review v0.5.0 | Round 4 | Last posted: Round 4, 21 findings (1 P1, 4 P2, 9 P3, 7 Nit), APPROVE. Review Finding inventoryFindings
Contested and acknowledged(none) Round logRound 1Netero-only. 1 P2, 2 P3, 1 Nit. Reviewed against ef6ee2a..be0b8d1. Round 2Panel (17 reviewers). DEREM-1 through DEREM-4 addressed. 1 P1, 3 P2, 3 P3, 3 Nit new (Netero: 1 P3, 2 Nit; Panel: 1 P1, 3 P2, 2 P3, 1 Nit). Reviewed against ef6ee2a..63fcc71. Round 3BLOCKED. DEREM-5 (P3) and DEREM-14 (Nit) silent. No review. Round 4Panel (4 reviewers). DEREM-5 through DEREM-16 all addressed. 4 P3, 1 Nit new. No findings from Netero or Chopper. Reviewed against 0d9718e..01702e8. About deep-reviewCRF = Coder Review Finding (P0-P4, Nit, Note)
|
Adds provider_id (FK to ai_providers, ON DELETE SET NULL) on aibridge_interceptions, populated via two-pass backfill (by name, then unambiguous type). Provider IDs are plumbed through aibridge.Provider, the dRPC proto, and aibridgedserver. aibridged is now seeded from the database via BuildProvidersFromDB so each in-memory Provider carries its row UUID, removing the per-request name->ID lookup. The interception/session list and count filters take a provider_id UUID; the SQL coalesces back to the legacy provider_name/provider snapshot columns for rows that pre-date the column or could not be unambiguously resolved at backfill time. Telemetry summaries continue to aggregate by provider type. Response payloads are unchanged; the legacy provider and provider_name text columns are kept until a follow-up PR drops them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… key Address review findings: DEREM-5: Remove BuildProviders (env-driven constructor) and its helpers (bedrockConfigFromProvider, getBedrockConfig) which have zero production callers after the migration to BuildProvidersFromDB. Remove the dedicated TestBuildProviders test suite. Update enterprise TestDomainsFromProviders to construct providers directly instead of via BuildProviders. DEREM-14: Fix Swagger @Param annotations in aiBridgeListInterceptions and aiBridgeListSessions to document 'provider_id' instead of 'provider' as the search query key, matching the actual parseAIProvider implementation.
01702e8 to
3eea892
Compare
There was a problem hiding this comment.
Round 4 (Netero + panel of 4). All 12 prior findings (DEREM-5 through DEREM-16) verified as fixed. The P1 migration cast, P2 server startup blocker, P2 RBAC gap, and P2 legacy fallback coverage gap are all resolved. Dead code removed. Swagger annotations corrected. N+1 query eliminated.
The author's approach for DEREM-10 (granting auditors read access to providers instead of using AsSystemRestricted) is cleaner than the suggestion; it keeps the permission model explicit.
4 P3, 1 Nit remaining. These are test coverage gaps and design-level observations, none blocking.
"Shall I show you? Deploy two openai providers." — Hisoka, narrating the type fallback edge case
🤖 This review was automatically generated with Coder Agents.
| "encoding/json" | ||
| "testing" | ||
|
|
||
| "github.com/google/uuid" |
There was a problem hiding this comment.
P3 [CRF-17] TestBuildProvidersFromDB has no subtest for the Copilot provider type. The buildProviderFromRow switch has three live branches (OpenAI, Anthropic, Copilot), but only OpenAI and Anthropic are tested. The Copilot path has unique behavior (ignores API keys, no Bedrock, no key pool). Deleting the case database.AiProviderTypeCopilot block would leave all tests green. (Bisky)
🤖
| t.Helper() | ||
| return slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) | ||
| } | ||
|
|
There was a problem hiding this comment.
P3 [CRF-18] AnthropicProviderWithBedrock asserts p.Name() and p.Type() but nothing about the Bedrock wiring. If the if settings.Bedrock != nil guard were inverted, the test would still pass. Adding assert.Equal(t, "https://api.anthropic.com/", p.BaseURL()) would distinguish "Bedrock config was parsed" from "regular Anthropic pointing at a Bedrock URL." (Bisky)
🤖
| if id == uuid.Nil { | ||
| return uuid.Nil, "", "" | ||
| } | ||
| p, err := db.GetAIProviderByID(ctx, id) |
There was a problem hiding this comment.
P3 [CRF-19] Filtering by a soft-deleted provider's UUID returns a 400 validation error instead of filtered results. parseAIProvider calls GetAIProviderByID which filters deleted = FALSE. Once an admin soft-deletes a provider, its historical interceptions become unfilterable by provider_id. The old text-based filter worked regardless. This is a design trade-off worth documenting; if unintentional, the lookup could include soft-deleted rows (read-only resolution is safe). (Hisoka)
🤖
| AND ( | ||
| (@provider_name::text != '' AND aibridge_interceptions.provider_name = @provider_name::text) | ||
| OR (aibridge_interceptions.provider_name = '' AND @provider_type::text != '' AND aibridge_interceptions.provider = @provider_type::text) | ||
| ) |
There was a problem hiding this comment.
P3 [CRF-20] The runtime type fallback for legacy rows (provider_name = '', provider_id IS NULL) matches on provider type string without the uniqueness guard the migration backfill has (HAVING COUNT(*) = 1). With two providers of the same type, legacy rows match whichever provider UUID is used in the filter, potentially misattributing traffic. Impact is bounded (only pre-000458 rows), but the mismatch between migration and runtime semantics is worth a SQL comment. (Mafuuu P3, Hisoka Note)
🤖
| t.Helper() | ||
| return slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) | ||
| } | ||
|
|
There was a problem hiding this comment.
Nit [CRF-21] Two consecutive blank lines between testLogger and providerNames. Single blank line suffices. (Bisky)
🤖
Release note: the migration may take some time to run if
aibridge_interceptionsis large.