feat: add ai_providers table, queries, dbauthz, audit, RBAC#24892
Conversation
Docs preview📖 View docs preview for |
|
/coder-agents-review |
There was a problem hiding this comment.
First-pass review (Netero). 1 P2, 1 P3.
The database foundation is well-structured: naming conventions are correct, dbauthz wrappers follow the standard pattern, dbcrypt overrides are consistent, and tests are non-vacuous with proper encrypted-row verification. The decryptAIProvider shared helper keeps the dbcrypt layer clean.
One defect needs fixing before the full review panel runs: SoftDeleteAIProviderByID returns an AiProvider struct but has no dbcrypt override, so callers get encrypted ciphertext in api_key and settings. The missing test coverage (P3) is straightforward to add alongside.
This is a first-pass review only. These are mechanical findings from Netero; the full review panel has not yet reviewed this PR. The panel will review after these findings are addressed.
🤖 This review was automatically generated with Coder Agents.
80186d4 to
74a413d
Compare
| TRUE, | ||
| FALSE, | ||
| 'https://bedrock-runtime.us-west-2.amazonaws.com/', | ||
| '{"bedrock_region":"us-west-2","bedrock_model":"global.anthropic.claude-sonnet-4-5-20250929-v1:0","bedrock_access_key":"fixture-bedrock-access-key","bedrock_access_key_secret":"fixture-bedrock-access-key-secret"}' |
There was a problem hiding this comment.
This format is not final, but irrelevant for now.
There was a problem hiding this comment.
Nit [DEREM-5] Fixture filename uses old aibridge_providers naming. Migration is 000494_ai_providers.up.sql. Project convention: fixture filename matches the migration name prefix.
(Netero)
🤖
73ce74c to
1a61871
Compare
Adds the database foundation for runtime AI Bridge provider management described in the 'Common AI Provider Configs' RFC. - New ai_providers table with type/name/display_name/enabled/deleted, base_url, api_key, settings (encrypted blobs via dbcrypt). - New ai_provider_type enum (openai, anthropic). - Queries: GetAIProviderByID, GetAIProviderByName, GetAIProviderByNameIncludeDeleted, GetAIProviders, GetEnabledAIProviders, InsertAIProvider, UpdateAIProvider, SoftDeleteAIProviderByID. - dbauthz: gates all queries on ResourceAibridgeProvider. - dbcrypt: encrypts api_key and the settings JSON blob at rest with separate dbcrypt key references. - audit: tracks Create/Write/Delete on AiProvider with secret fields marked appropriately. - rbac: introduces aibridge_provider resource (owner-only via allPermsExcept default). - dbgen: AIProvider factory for tests. Refs RFC: https://www.notion.so/coderhq/RFC-Common-AI-Provider-Configs-34bd579be59280ed958feffb82024797
Adds GetAIProvidersForRotation and UpdateAIProviderEncryptedColumns
queries (and their dbauthz/dbcrypt wrappers) so the Rotate, Decrypt,
and Delete utilities walk the ai_providers table alongside every other
encrypted table.
Previously, key revocation failed with a foreign-key violation when
ai_providers rows still referenced the old key's digest:
error: rotate ciphers: revoke key: pq: update or delete on table
"dbcrypt_keys" violates foreign key constraint
"ai_providers_api_key_key_id_fkey" on table "ai_providers"
Soft-deleted rows are also walked because their FK references persist
until the row itself is hard-deleted.
…erated artifacts The TestRolePermissions/aibridge_provider-AllActions check failed because no test case covered the new aibridge_provider resource. Add an owner-only CRUD case alongside the existing AIBridgeInterception entries. Also refreshes the generated artifacts that the previous "make gen" run missed because of stale build timestamps: - coderd/apidoc/docs.go and swagger.json - codersdk/apikey_scopes_gen.go - docs/admin/security/audit-logs.md - docs/reference/api/schemas.md - site/src/api/typesGenerated.ts These all needed to pick up the aibridge_provider resource and the AiProvider audit table entry that PR 24892 introduced.
Per the updated RFC, ai_providers no longer carries the api_key column. Instead a new ai_provider_keys table holds 0..N keys per provider: - Bedrock providers have 0 keys; their AWS credentials live in ai_providers.settings (bedrock_access_key + bedrock_access_key_secret). - OpenAI/Anthropic providers have 1..N keys to support future failover and rotation. Changes: - Migration 000483: drop api_key/api_key_key_id from ai_providers; add ai_provider_keys(id, provider_id FK ON DELETE CASCADE, api_key, api_key_key_id, timestamps). - Queries split: aibridgeproviders.sql for the provider row, aibridgeproviderkeys.sql for the keys. - dbauthz: new key queries gated on ResourceAibridgeProvider. - dbcrypt: provider rotation walks only settings now; new key rotation walks ai_provider_keys including those whose provider is soft-deleted. - audit: new AiProviderKey row tracks Create/Delete with api_key marked ActionSecret. - dbgen: new AIProviderKey factory. - Tests: TestAIProviderKeys (round-trip), TestRotateAIProviderKeys (rotation including soft-deleted parent provider), and the existing AIProvider tests rewritten without the api_key field. - make gen refreshes all derived artifacts.
Renames SoftDeleteAIProviderByID -> DeleteAIProviderByID, UpdateAIProviderEncryptedColumns -> UpdateAIProviderSettings, GetAIProviderKeysForRotation -> GetAIProviderKeys, and UpdateAIProviderKeyEncryptedColumns -> UpdateEncryptedAIProviderKey. Drops GetAIProviderByNameIncludeDeleted, GetEnabledAIProviders, and GetAIProvidersForRotation; GetAIProviders now accepts optional enabled/deleted filters that the dbcrypt rotation walks with both flags NULL. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Renames the new RBAC resource so it can be used by all AI-related features, not just AI Bridge. Sets Name: "AIProvider" on the policy entry so the generated Resource* constants use AI; adds sqlc.yaml renames so the database struct types follow suit. Generator-derived ScopeAi*/APIKeyScopeAi* constants are left as-is. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ring Re-running make gen revealed three issues left by earlier commits on this branch. The apidoc, swagger, audit-logs, RBAC, and codersdk type artifacts were stale relative to the @Router/@basepath, RBAC, and codersdk changes that landed in 74a413d and the commits leading up to it. The AIProvider rename missed the AuditActionMap keys, which are looked up case-sensitively by the audit doc generator and so rendered the AI rows with empty actions. dbgen.AIProviderKey did not set the CreatedAt sql.NullTime field that InsertAIProviderKeyParams gained, tripping exhaustruct. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Renames query files, migration files, and Go symbols to drop the "aibridge" prefix for the generic AI provider concept. Adds a missing dbcrypt override on DeleteAIProviderByID so soft-deleted rows return decrypted settings, matching sibling methods. Env vars (CODER_AIBRIDGE_PROVIDER_<N>_<KEY>) and aibridgeproxyd symbols are intentionally left unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
sqlc orders queries.sql.go by source filename; renaming aibridgeproviders.sql / aibridgeproviderkeys.sql to ai_providers.sql / ai_provider_keys.sql shifts those query blocks ahead of aibridge.sql in the generated output. Pure reorder: 1673 lines moved, no content changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1a61871 to
692e9c3
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
Second-pass review (Netero). R1 findings addressed. 1 P1, 1 P3, 1 Nit, 1 Note new.
DEREM-1 (P2) and DEREM-2 (P3) are resolved: SoftDeleteAIProviderByID was converted to :exec (no return value), and the untested methods were removed entirely. Clean fixes.
The restructuring between rounds exposed a significant gap: the PR title and description claim dbcrypt wiring, but zero lines in enterprise/dbcrypt/ were changed. The schema has FK constraints from ai_providers.settings_key_id and ai_provider_keys.api_key_key_id to dbcrypt_keys(active_key_digest), so key rotation will fail on revocation once encryption is enabled.
This is a first-pass review only. These are mechanical findings from Netero; the full review panel has not yet reviewed this PR. The panel will review after these findings are addressed.
🤖 This review was automatically generated with Coder Agents.
johnstcn
left a comment
There was a problem hiding this comment.
Missing the dbcrypt methods. I'd also recommend some additional dbcrypt tests specifically for those queries.
@johnstcn I mentioned in the description; the dbcrypt changes have been split out into a separate PR upstack |
- Remove GetAIProviderByNameIncludeDeleted; soft-deleted lookups can be added back where actually needed. - Replace the 3-way sqlc.narg-based deleted/enabled filters on GetAIProviders with plain bool include_deleted/include_disabled flags that default to false, so the common path of "live + enabled rows only" needs no params. - Convert DeleteAIProviderByID and DeleteAIProviderKey to :exec since callers do not need the affected row back. - Require explicit created_at/updated_at on InsertAIProviderKey instead of falling back to NOW() inside the query.
The column is now nullable text with no default; callers fall back to name when display_name is NULL. The dbgen test factory mirrors this by seeding display_name from the provider name when none is supplied.
The previous 8-character prefix was lossy. Surface the full provider UUID so audit consumers can correlate the key back to its provider without guessing.
…ncryptedAIProviderSettings Mirrors UpdateEncryptedAIProviderKey on ai_provider_keys; the name now makes clear that the query targets the encrypted settings columns and is intended for the dbcrypt rotation utility.
Signed-off-by: Danny Kopping <danny@coder.com>
22dabfa to
62b664c
Compare
- Allow soft-deleted provider names to be reused by replacing the global UNIQUE constraint on ai_providers.name with a partial unique index that only applies to non-deleted rows. - Fix FriendlyString to return "ai provider" / "ai provider key" instead of "ai bridge provider". - Rename fixture file 000495_aibridge_providers.up.sql to 000495_ai_providers.up.sql to match its migration. - Update roles_test.go comment to say AI providers are deployment-wide, not workspace-wide. - Update GetAIProviderKeysByProviderID comment to say the oldest key (not the first key) is used. - Trim verbose GetAIProviders comment. - Update dbauthz test for GetAIProviders to use the new GetAIProvidersParams struct. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Disclaimer: implemented by a Coder Agent using Claude Opus 4.7 and heavily augmented by me.
Part of the implementation of RFC: Common AI Provider Configs.
This PR adds the database foundation for runtime AI Bridge provider management. Provider API keys live in a sibling
ai_provider_keystable so a provider can carry zero or many keys (Bedrock providers have none; OpenAI and Anthropic have one or more for failover).What's in this PR
ai_providerstable (type/name/display_name/enabled/deleted/base_url/settings/settings_key_id) andai_provider_keystable (provider_idFK withON DELETE CASCADE,api_key/api_key_key_id). Newai_provider_typeenum (openai,anthropic).display_nameis nullable; callers fall back toname.GetAIProviderByID,GetAIProviderByName,GetAIProviders(optionalenabledfilter, soft-deleted rows always excluded),InsertAIProvider,UpdateAIProvider,UpdateAIProviderSettings,DeleteAIProviderByID; plusGetAIProviderKeyByID,GetAIProviderKeysByProviderID,GetAIProviderKeys,InsertAIProviderKey,UpdateEncryptedAIProviderKey,DeleteAIProviderKey.ResourceAIProvider.Create/Write/DeleteonAIProviderandCreate/DeleteonAIProviderKey.settingsandapi_keyare markedActionSecretso they are never leaked in audit diffs. Addsai_provider/ai_provider_keytoresource_typeandai_provider:*scopes toapi_key_scope.ai_providerresource. Owner role gets full access viaallPermsExceptdefault; no other built-in role gets any action on this resource.AIProviderandAIProviderKeyfactories for tests.dbcrypt-at-rest support for
settingsandapi_keylives in #25326 upstack of this PR.Design notes
settingsis stored as a single JSON blob (not field-by-field). For Bedrock providers it carriesbedrock_region,bedrock_model,bedrock_small_fast_model, andbedrock_access_key_secret. The Bedrock access key itself reuses the existingapi_keycolumn onai_provider_keys.providers,proxy) will be enforced in the handler layer.