feat: remove legacy chat provider tables#25416
Conversation
|
/coder-agents-review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54272617fd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for _, provider := range providers { | ||
| if provider.ID == selectedAIProviderID { | ||
| selectedIncluded = true | ||
| } | ||
| configuredProvider, err := p.aiProviderConfig(ctx, provider) |
There was a problem hiding this comment.
Restrict selected-provider key resolution to one provider
When selectedAIProviderID is set, this branch now iterates over all enabled AI providers and passes them to ResolveUserProviderKeys. That resolver collapses providers by normalized provider name, so if two enabled providers share a type (for example, two openai rows), the later one in iteration can overwrite the selected provider’s API key and base URL. In that case, a chat model config pinned to provider A can be executed with provider B’s credentials/endpoint, which can misroute traffic and leak prompts across provider configs.
Useful? React with 👍 / 👎.
|
/coder-agents-review |
1 similar comment
|
/coder-agents-review |
There was a problem hiding this comment.
The migration structure is solid: CHECK constraint before table drops, 410 stubs for removed endpoints, cache invalidation wired up, and the coderdtest helpers properly updated. The active tests (TestListChatModels, TestCreateChatModelConfig, etc.) were rewritten against AI provider APIs with a shared createAIProviderForTest helper.
Severity count: 2 P2, 8 P3, posted below.
The two P2s are both latent correctness/performance issues introduced by the key-resolution refactor. The type-keyed ProviderAPIKeys map silently overwrites credentials when two AI providers share a type (newly allowed since ai_providers has no UNIQUE on type). The N+1 query pattern per chat turn is a measurable regression from the old inline-key model.
Pattern worth noting: the removal is incomplete in three parallel ways. Five test functions are skipped but retain ~1100 lines of dead code (DEREM-3). The SDK client methods for removed APIs still compile (same root cause). A new compat shim (legacy_chat_provider_compat.go) was introduced to avoid migrating ~15 test files (DEREM-9). These are all mechanical conversions that could be completed in this PR or tracked in a ticket.
"Shall I show you what happens to audit logs when the API key doesn't match the provider the user selected? ♥" -- Hisoka
coderd/x/chatd/chatd.go:8077
P2 [DEREM-6] aiProviderConfig calls GetAIProviderKeysByProviderID per provider; every chat turn and model-list call now does N extra DB round-trips where the old code did zero. (Killua P2, Meruem P2, Mafuuu P2, Hisoka P3, Zoro P3)
Before this PR,
ChatProviderrows carried the API key inline.EnabledProvidersreturned fully-populated rows. Conversion was zero extra queries. NowAIProviderrows have no key column; the cache returns provider rows but every consumer hits the DB N times to get keys.
The same pattern exists in configuredProviderFromAIProvider at exp_chats.go:6806. Both branches of resolveUserProviderAPIKeys (selected and non-selected) pay this cost, as does getUserChatProviderAvailability.
Fix: either cache keys alongside providers in chatConfigCache (they change on the same events), or batch-fetch all keys in one query with GetAIProviderKeys() and group by provider_id in Go. Both reduce N queries to 1.
🤖
coderd/database/queries/chatmodelconfigs.sql:136
P3 [DEREM-2] Dead query DeleteChatModelConfigsByProvider has zero runtime callers. Its only caller (deleteChatProvider) was deleted in this PR. The query definition, dbauthz wrapper, and all generated plumbing remain. (Netero)
Fix: remove the query from this file, remove the dbauthz test at dbauthz_test.go:453, and run make gen.
🤖
🤖 This review was automatically generated with Coder Agents.
There was a problem hiding this comment.
Round 2 is blocked. All 11 findings from Round 1 are unresolved with no author response or code changes addressing them.
Unaddressed findings (2 P2, 8 P3, 1 Nit):
- DEREM-5 (P2): Type-keyed provider map silently overwrites credentials when two AI providers share a type
- DEREM-6 (P2): N+1 DB queries per chat turn for provider key lookups
- DEREM-1 (P3): Dead sentinel
errChatProviderNotConfiguredand two unreachable switch cases - DEREM-2 (P3): Dead query
DeleteChatModelConfigsByProviderhas zero runtime callers - DEREM-3 (P3): Five skipped test functions retain ~1100 lines of dead code + SDK methods not cleaned
- DEREM-4 (P3): Duplicate conversion logic with asymmetric enabled check
- DEREM-7 (P3): CHECK constraint validates existing rows without guard for orphaned configs
- DEREM-8 (P3): Fake
error(nil)pattern in 16 test call sites - DEREM-9 (P3): Legacy compat shim with no removal timeline or ticket
- DEREM-10 (P3):
configuredProviderFromAIProviderreturns bare errors without wrapping - DEREM-11 (Nit):
insertInternalChatProvideraccepts 4 params it ignores
Further review is blocked until the author responds to or pushes fixes for the open findings. The two P2 findings (type-keyed credential collision and N+1 query regression) are the priority items.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
1 similar comment
|
/coder-agents-review |
There was a problem hiding this comment.
Round 3 is blocked (2nd consecutive). All 11 findings from Round 1 remain unresolved with no author response or code changes addressing them. New commits between rounds did not target any flagged items.
Priority items still open:
- DEREM-5 (P2): Type-keyed provider map silently overwrites credentials when two AI providers share a type
- DEREM-6 (P2): N+1 DB queries per chat turn for provider key lookups
Plus 8 P3s and 1 Nit (DEREM-1, 2, 3, 4, 7, 8, 9, 10, 11).
Further review remains blocked until the author responds to or pushes fixes for the open findings.
🤖 This review was automatically generated with Coder Agents.
There was a problem hiding this comment.
Round 4 is blocked (3rd consecutive). All 11 findings from Round 1 remain unresolved with no author response or code changes addressing them. Commits between rounds are merge commits and docs changes; none target any finding.
Priority items (3 rounds without response):
- DEREM-5 (P2): Type-keyed provider map silently overwrites credentials when two AI providers share a type
- DEREM-6 (P2): N+1 DB queries per chat turn for provider key lookups
Plus 8 P3s and 1 Nit (DEREM-1, 2, 3, 4, 7, 8, 9, 10, 11).
To unblock: respond to the findings (fix, contest, or acknowledge) or push changes addressing them. Re-triggering /coder-agents-review without addressing findings will continue to produce blocked rounds.
🤖 This review was automatically generated with Coder Agents.
Acknowledging the cleanup PR findings. Most of these are inherited from earlier stack slices and have now been fixed or answered there. For the cleanup-specific priority items:
For the remaining P3 and nit findings, I am treating them as follow-up cleanup or already covered by downstack fixes unless the next review identifies a cleanup-specific correctness issue. |
|
/coder-agents-review |
There was a problem hiding this comment.
Round 5 is blocked. Thank you for the response on DEREM-5 and DEREM-6; those are now classified as contested and will be evaluated by the panel in the next unblocked round.
However, 9 findings (DEREM-1, 2, 3, 4, 7, 8, 9, 10, 11) remain silent. The blanket statement "treating as follow-up cleanup or already covered by downstack fixes" does not constitute per-finding engagement. For each finding, the review needs one of:
- Fix: push a code change addressing it
- Contest: explain why the current code is correct or acceptable (as done for DEREM-5 and DEREM-6)
- Acknowledge: state "accepted, will not fix" with a reason
- Defer: link a tracking ticket for follow-up
The 9 silent findings:
- DEREM-1 (P3): Dead sentinel
errChatProviderNotConfiguredand two unreachable switch cases - DEREM-2 (P3): Dead query
DeleteChatModelConfigsByProvider - DEREM-3 (P3): Five skipped tests retain ~1100 lines of dead code + dead SDK methods
- DEREM-4 (P3): Duplicate conversion logic with asymmetric enabled check
- DEREM-7 (P3): CHECK constraint validates without guard for orphaned configs
- DEREM-8 (P3): Fake
error(nil)pattern in 16 test call sites - DEREM-9 (P3): Legacy compat shim with no removal timeline
- DEREM-10 (P3): Bare error return without wrapping
- DEREM-11 (Nit): Unused parameters in test helper
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
Round 6 is blocked (5th consecutive). No new author engagement since Round 5. The 9 P3/Nit findings (DEREM-1, 2, 3, 4, 7, 8, 9, 10, 11) still need per-finding responses: fix, contest, acknowledge, or defer with a ticket.
DEREM-5 and DEREM-6 remain contested and will be evaluated by the panel once unblocked.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
1 similar comment
|
/coder-agents-review |
There was a problem hiding this comment.
Round 7 is blocked. Progress on the P2s: DEREM-5 is now addressed (selected provider appended last to prevent same-type overwrite, commit ab0dfc2b0). DEREM-6 remains contested.
The 9 P3/Nit findings (DEREM-1, 2, 3, 4, 7, 8, 9, 10, 11) still have no per-finding engagement. To unblock, each needs one of: fix, contest, acknowledge, or defer with ticket.
🤖 This review was automatically generated with Coder Agents.
There was a problem hiding this comment.
Round 8 is blocked. No new author engagement since Round 7. DEREM-5 was fixed; DEREM-6 remains contested (pending panel). The 9 P3/Nit findings (DEREM-1, 2, 3, 4, 7, 8, 9, 10, 11) still need per-finding responses.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
Round 9 is blocked (8th consecutive). No new author engagement. DEREM-5 fixed, DEREM-6 contested (pending panel). The 9 P3/Nit findings (DEREM-1, 2, 3, 4, 7, 8, 9, 10, 11) still need per-finding responses. This is the final automated reminder; the findings and required actions have been listed in rounds 5, 6, 7, and 8.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
Responses to cleanup PR findings:
|
|
/coder-agents-review |
|
/coder-agents-review |
There was a problem hiding this comment.
Round 11 is blocked, but nearly unblocked. Thank you for the per-finding responses.
Classified: DEREM-1, 3, 8, 9, 10, 11 acknowledged. DEREM-4, 7 contested (joining DEREM-6). DEREM-5 fixed.
One remaining silent finding blocks the review:
- DEREM-2 (P3): Dead query
DeleteChatModelConfigsByProviderhas zero runtime callers. It was omitted from your itemized response. A one-line acknowledgment (e.g. "cleanup debt, same as DEREM-1") will unblock and allow the panel to evaluate the three contested findings (DEREM-4, 6, 7).
🤖 This review was automatically generated with Coder Agents.
There was a problem hiding this comment.
Round 12 is blocked on a single omission: DEREM-2 (P3, dead query DeleteChatModelConfigsByProvider). It was the only finding not listed in the per-finding response. A one-line acknowledgment (same class as DEREM-1: harmless cleanup debt) will unblock the panel review of the three contested findings (DEREM-4, 6, 7).
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
ada0062 to
5252616
Compare
|
/coder-agents-review |
1 similar comment
|
/coder-agents-review |
There was a problem hiding this comment.
Round 16 panel verification of the DEREM-6 batch-query fix found a P0 regression: the new GetAIProviderKeysByProviderIDs method has no dbcrypt override, so encrypted API keys are returned as ciphertext on any deployment with dbcrypt enabled. This breaks all chat functionality for those deployments.
Severity count: 1 P0 (new), 1 P1 (new), 1 P3 (new).
The prior R14 findings (DEREM-6, 12, 13, 14, 15) are all verified fixed. The batch query approach is correct; it just needs the dbcrypt wrapper.
🤖 This review was automatically generated with Coder Agents.
There was a problem hiding this comment.
Round 17 is blocked. All 3 findings from R16 are silent:
- DEREM-16 (P0):
GetAIProviderKeysByProviderIDshas no dbcrypt override. Encrypted keys returned as-is on dbcrypt deployments. This breaks all chat. - DEREM-17 (P1): Test mocks reference singular method; two tests fail.
- DEREM-18 (P3):
providerConfiguredretains per-provider N+1 pattern.
Note: CI is also failing (test-go-pg, test-go-pg-17, test-go-race-pg), likely related to DEREM-17.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
e427bdd to
f737b21
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
All 18 findings resolved across 18 rounds. No new findings from the R18 panel or Netero scan.
Fixed (8): DEREM-5 (P2, type-keyed collision), DEREM-6 (P2, N+1 queries, batch query), DEREM-12 (P3, duplicate DELETE), DEREM-13/14 (Nit, stale names), DEREM-15 (P3, redundant fetch), DEREM-16 (P0, dbcrypt override), DEREM-17 (P1, test mocks), DEREM-18 (P3, subagent N+1).
Panel-closed (2): DEREM-4 (5/5), DEREM-7 (3/3).
Acknowledged (7): DEREM-1, 2, 3, 8, 9, 10, 11.
The final code correctly batch-fetches provider keys with dbcrypt decryption, preserves selected-provider precedence for same-type collisions, and eliminates redundant provider fetches. The migration, 410 stubs, and test helpers are structurally sound.
🤖 This review was automatically generated with Coder Agents.
Fixed the computer-use regression found by CI in commit 7f7505a. Computer-use chats now refresh provider keys with the all-provider path before resolving the computer-use model, so a root chat selected provider does not hide the configured Anthropic computer-use key. |
|
/coder-agents-review |
990266b to
ce5541c
Compare
7f7505a to
65f3eb5
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
Round 19 re-approval after new commits. No open findings. Netero and panel (Mafuuu, Pariston, Meruem) found no new issues in the delta.
All 18 findings remain resolved. CI failures (test-go-pg, test-go-race-pg) appear unrelated to this PR's changes; the new commits are merge-ups from the base branch.
🤖 This review was automatically generated with Coder Agents.
There was a problem hiding this comment.
Round 20 verification of the computer-use regression fix (commit 7f7505ae41) found a P1 data race.
The new providerKeys = computerUseProviderKeys reassignment at line 7367 races with the title-generation goroutine launched ~560 lines earlier (line 6806), which captures providerKeys by closure. The existing code already snapshots model, logger, and ctx for exactly this reason (lines 6799-6804), but the new commit does not snapshot providerKeys.
Consequence: race detector will flag this in CI. In production, title generation on computer-use chats can use wrong credentials (all-provider keys instead of chat-model keys), potentially sending provider A's key to provider B's endpoint.
🤖 This review was automatically generated with Coder Agents.
|
/coder-agents-review |
There was a problem hiding this comment.
Round 21 is blocked on a single Nit: DEREM-20 (two consecutive if isComputerUse blocks, 4 reviewers converged). DEREM-19 (P1 data race) is verified fixed in 2689be829 (providerKeys snapshot added).
One acknowledgment for DEREM-20 unblocks the final approval.
🤖 This review was automatically generated with Coder Agents.
ce5541c to
7995274
Compare
2689be8 to
d43f232
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
All 20 findings resolved across 22 rounds. No new issues from Netero or panel (Mafuuu, Pariston, Meruem).
Fixed (12): DEREM-5 (P2, type-keyed collision), DEREM-6 (P2, N+1 batch query), DEREM-12 (P3, duplicate DELETE), DEREM-13/14 (Nit, stale names), DEREM-15 (P3, redundant fetch), DEREM-16 (P0, dbcrypt override), DEREM-17 (P1, test mocks), DEREM-18 (P3, subagent N+1), DEREM-19 (P1, data race), DEREM-20 (Nit, merged if blocks).
Panel-closed (2): DEREM-4 (5/5), DEREM-7 (3/3).
Acknowledged (6): DEREM-1, 2, 3, 8, 9, 10, 11.
🤖 This review was automatically generated with Coder Agents.
Stack Context
This stack migrates Agents chat provider configuration from legacy chat provider tables to AI provider tables.
What?
Drops legacy chat provider SQL paths, removes the legacy tables, updates dbcrypt and tests, and keeps model configs tied to AI provider IDs. It also batches AI provider key lookups in Agents runtime and availability paths so removing legacy tables does not introduce per-provider key queries.
Why?
Once schema, API, backend, and frontend code use AI providers, the old tables and generated query surface should be removed to prevent split-brain provider configuration.