feat: add DB queries for aibridge_coderd_keys#25564
Conversation
|
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. |
884d73a to
07965bc
Compare
07965bc to
4809647
Compare
f842713 to
3248cc3
Compare
4809647 to
68caa22
Compare
056ea4d to
ea066e6
Compare
09af228 to
9851434
Compare
3c2624b to
ca08383
Compare
ae89f52 to
56911cf
Compare
Adds Insert / List / Delete queries on aibridge_coderd_keys plus the
ResourceAIGatewayCoderdKey RBAC object (Create / Read / Delete actions).
The hashed_secret column is intentionally excluded from every query's
RETURNING / SELECT list, so generated row types never carry it.
Also extends the migration with token_prefix (varchar(11)), the
'aibridge_coderd_key' resource_type enum value for audit support, and
ai_gateway_coderd_key:{*,create,delete,read} api_key_scope values.
56911cf to
4a8ae2b
Compare
|
/coder-agents-review |
There was a problem hiding this comment.
Solid query-layer PR. The three queries are well-structured: explicit column subsets that exclude hashed_secret from return types (structural security boundary enforced at the SQL level, not caller discipline), correct RBAC resource/action pairing, and thorough constraint tests covering all six DB constraints. Pariston tried to build a case against the design and couldn't. Kite praised the secret-exclusion pattern specifically. Mafuuu traced the full contract chain and confirmed correctness.
3 P3, 2 Nit. No blockers.
Process note: the PR title says aibridge_coderd_keys but the actual table is ai_gateway_coderd_keys. When this squash-merges, git log --grep ai_gateway_coderd_keys won't find it. Worth fixing the title before merge.
"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 of the stack." (Pariston)
🤖 This review was automatically generated with Coder Agents.
| @@ -0,0 +1,12 @@ | |||
| -- name: InsertAIGatewayCoderdKey :one | |||
| INSERT INTO ai_gateway_coderd_keys (id, created_at, name, secret_prefix, hashed_secret) | |||
| VALUES ($1, $2, lower(@name), $3, $4) | |||
There was a problem hiding this comment.
P3 [DEREM-1] The lower(@name) here silently normalizes user input before the constraint check. A caller passing "My-Key" gets back "my-key" with no indication the transformation happened. The sibling in roles.sql documents this:
-- Always force lowercase names
lower(@name),A one-line comment like -- Names are stored lowercase for case-insensitive uniqueness. pins the design decision so the next reader (or handler author comparing row.Name == inputName) doesn't have to deduce it from the constraint. (Gon P3, Leorio P3)
🤖
| name: "duplicate secret prefix", | ||
| params: aiGatewayCoderdKeyParams("different-key", "cgw_test001", []byte("hashed-secret-002")), | ||
| expectUniqueErr: database.UniqueAiGatewayCoderdKeysSecretPrefixIndex, | ||
| }, |
There was a problem hiding this comment.
P3 [DEREM-2] The name CHECK constraint is name ~ '^[a-z0-9]+(-[a-z0-9]+)*$', but the only test exercising it uses the empty string (line 14563). The test would still pass if the regex were relaxed to '^.+$' (anything non-empty). The structural boundaries of the regex are unverified: trailing dash ("test-key-"), consecutive dashes ("test--key"), underscores ("test_key"), spaces.
Adding one or two boundary rows to the table (e.g. "test-key-" and "test_key" both expecting CheckAiGatewayCoderdKeysNameCheck) pins the regex tightly and prevents silent relaxation. (Bisky)
🤖
| requireAIGatewayCoderdKeyRow(t, keys[1], second.ID, "second-key", second.SecretPrefix) | ||
| require.False(t, keys[1].LastUsedAt.Valid) | ||
|
|
||
| err = db.DeleteAIGatewayCoderdKey(ctx, first.ID) |
There was a problem hiding this comment.
P3 [DEREM-3] Delete of a nonexistent key is untested. DeleteAIGatewayCoderdKey is :exec, so DELETE ... WHERE id = $1 on a missing row returns nil (not sql.ErrNoRows). When the HTTP handler wraps this, it needs to decide whether nil means "deleted" or "not found." A one-line assertion after the existing delete block documents the contract:
err = db.DeleteAIGatewayCoderdKey(ctx, uuid.New())
require.NoError(t, err) // :exec DELETE is silent on miss(Bisky)
🤖
| expectCheckErr database.CheckConstraint | ||
| }{ | ||
| { | ||
| name: "duplicate name", |
There was a problem hiding this comment.
Nit [DEREM-4] This test case inserts base "test-key" then tries "TEST-KEY", which tests case-insensitive name uniqueness via lower() + the unique index. But the name "duplicate name" reads as an exact-match duplicate. Every other duplicate test in this table uses literally identical values. Something like "duplicate name (case insensitive)" makes the lower() contract visible to the next reader. (Leorio, Meruem)
🤖
| } | ||
| } | ||
|
|
||
| func requireAIGatewayCoderdKeyRow(t *testing.T, row database.ListAIGatewayCoderdKeysRow, id uuid.UUID, name string, secretPrefix string) { |
There was a problem hiding this comment.
Nit [DEREM-5] requireAIGatewayCoderdKeyRow asserts ID, Name, and SecretPrefix but skips CreatedAt and LastUsedAt. LastUsedAt is checked separately, but CreatedAt from list results is never verified (the insert test checks it via WithinDuration, but if the list query returned wrong timestamps, nothing catches it). Either add CreatedAt to the helper or rename to requireAIGatewayCoderdKeyIdentity to signal the partial scope. (Bisky, Gon)
🤖
Adds Insert, List and Delete queries for AI Gateway keys.