fix(db): dedup duplicate API keys per provider on connection create (#3023) - #3100
Conversation
…3023) createProviderConnection deduped apikey connections only by (provider, name). Adding the same key under a different/blank name created a duplicate row. It now also matches by the decrypted key value (AES-GCM ciphertext is non-deterministic, so we decrypt+compare plaintext, trimmed) and updates the existing connection instead. Tests cover same-key dedup, whitespace-variant dedup, and distinct-key separation.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Code Review
This pull request implements API key deduplication in createProviderConnection to prevent duplicate connection rows for the same provider when the same API key is added under different names, and includes corresponding unit tests. The review feedback correctly points out a runtime error in the new test file where beforeEach and after hooks are incorrectly accessed as properties of the default test export from node:test instead of being imported as named exports.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| @@ -0,0 +1,87 @@ | |||
| import test from "node:test"; | |||
| test.beforeEach(resetStorage); | ||
|
|
||
| test.after(() => { | ||
| core.resetDbInstance(); | ||
| fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); | ||
| }); |
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Other Observations (not in diff)Issues found in unchanged code that cannot receive inline comments:
Positive observations
Files Reviewed (3 files)
Reviewed by laguna-m.1-20260312:free · 3,620,309 tokens |
#3023 dedup) After #3100 (#3023) dedups provider connections by decrypted key value, the seedConnection helper's shared 'sk-test' default collapsed multiple seeded connections into one, breaking round-robin / least-used / fallback selection tests (they saw 1 account instead of 2+). Default to a unique key per connection (matching the existing unique-name default). Found via full test:unit — #3100 was merged via gh, bypassing the pre-push test gate, so these never ran post-merge.
…drift + #3100 dedup) Full CI surfaced real failures that local subsets missed (gh-merged PRs bypass the hooks that run these gates): - typecheck:core (Lint job): 3 now-unused @ts-expect-error in mcp-server/server.ts (#3077 dynamic tool loops) → @ts-ignore (lenient, no TS2578). - pack-artifact-policy.test.ts: build-reorg (#3124) renamed app/->dist/; the test still asserted app/ paths + REQUIRED order (it sorts alphabetically). - electron-packaging.test.ts: extraResources from .next/electron-standalone -> .build/electron-standalone (#3124). - glm-provider-model-import-route.test.ts: two GLM connections shared one apiKey, so #3100 (#3023) dedup collapsed them → only one discovery fetch. Distinct keys. Remaining CI flakes (batch expiration, ModelSync self-fetch) pass in isolation — concurrency/port flakiness under --test-concurrency=4, not real failures.
…iegosouzapw#3023) (diegosouzapw#3100) createProviderConnection deduped apikey connections only by (provider, name). Adding the same key under a different/blank name created a duplicate row. It now also matches by the decrypted key value (AES-GCM ciphertext is non-deterministic, so we decrypt+compare plaintext, trimmed) and updates the existing connection instead. Tests cover same-key dedup, whitespace-variant dedup, and distinct-key separation.
diegosouzapw#3023 dedup) After diegosouzapw#3100 (diegosouzapw#3023) dedups provider connections by decrypted key value, the seedConnection helper's shared 'sk-test' default collapsed multiple seeded connections into one, breaking round-robin / least-used / fallback selection tests (they saw 1 account instead of 2+). Default to a unique key per connection (matching the existing unique-name default). Found via full test:unit — diegosouzapw#3100 was merged via gh, bypassing the pre-push test gate, so these never ran post-merge.
…drift + diegosouzapw#3100 dedup) Full CI surfaced real failures that local subsets missed (gh-merged PRs bypass the hooks that run these gates): - typecheck:core (Lint job): 3 now-unused @ts-expect-error in mcp-server/server.ts (diegosouzapw#3077 dynamic tool loops) → @ts-ignore (lenient, no TS2578). - pack-artifact-policy.test.ts: build-reorg (diegosouzapw#3124) renamed app/->dist/; the test still asserted app/ paths + REQUIRED order (it sorts alphabetically). - electron-packaging.test.ts: extraResources from .next/electron-standalone -> .build/electron-standalone (diegosouzapw#3124). - glm-provider-model-import-route.test.ts: two GLM connections shared one apiKey, so diegosouzapw#3100 (diegosouzapw#3023) dedup collapsed them → only one discovery fetch. Distinct keys. Remaining CI flakes (batch expiration, ModelSync self-fetch) pass in isolation — concurrency/port flakiness under --test-concurrency=4, not real failures.
…iegosouzapw#3023) (diegosouzapw#3100) createProviderConnection deduped apikey connections only by (provider, name). Adding the same key under a different/blank name created a duplicate row. It now also matches by the decrypted key value (AES-GCM ciphertext is non-deterministic, so we decrypt+compare plaintext, trimmed) and updates the existing connection instead. Tests cover same-key dedup, whitespace-variant dedup, and distinct-key separation.
diegosouzapw#3023 dedup) After diegosouzapw#3100 (diegosouzapw#3023) dedups provider connections by decrypted key value, the seedConnection helper's shared 'sk-test' default collapsed multiple seeded connections into one, breaking round-robin / least-used / fallback selection tests (they saw 1 account instead of 2+). Default to a unique key per connection (matching the existing unique-name default). Found via full test:unit — diegosouzapw#3100 was merged via gh, bypassing the pre-push test gate, so these never ran post-merge.
…drift + diegosouzapw#3100 dedup) Full CI surfaced real failures that local subsets missed (gh-merged PRs bypass the hooks that run these gates): - typecheck:core (Lint job): 3 now-unused @ts-expect-error in mcp-server/server.ts (diegosouzapw#3077 dynamic tool loops) → @ts-ignore (lenient, no TS2578). - pack-artifact-policy.test.ts: build-reorg (diegosouzapw#3124) renamed app/->dist/; the test still asserted app/ paths + REQUIRED order (it sorts alphabetically). - electron-packaging.test.ts: extraResources from .next/electron-standalone -> .build/electron-standalone (diegosouzapw#3124). - glm-provider-model-import-route.test.ts: two GLM connections shared one apiKey, so diegosouzapw#3100 (diegosouzapw#3023) dedup collapsed them → only one discovery fetch. Distinct keys. Remaining CI flakes (batch expiration, ModelSync self-fetch) pass in isolation — concurrency/port flakiness under --test-concurrency=4, not real failures.
…iegosouzapw#3023) (diegosouzapw#3100) createProviderConnection deduped apikey connections only by (provider, name). Adding the same key under a different/blank name created a duplicate row. It now also matches by the decrypted key value (AES-GCM ciphertext is non-deterministic, so we decrypt+compare plaintext, trimmed) and updates the existing connection instead. Tests cover same-key dedup, whitespace-variant dedup, and distinct-key separation.
diegosouzapw#3023 dedup) After diegosouzapw#3100 (diegosouzapw#3023) dedups provider connections by decrypted key value, the seedConnection helper's shared 'sk-test' default collapsed multiple seeded connections into one, breaking round-robin / least-used / fallback selection tests (they saw 1 account instead of 2+). Default to a unique key per connection (matching the existing unique-name default). Found via full test:unit — diegosouzapw#3100 was merged via gh, bypassing the pre-push test gate, so these never ran post-merge.
…drift + diegosouzapw#3100 dedup) Full CI surfaced real failures that local subsets missed (gh-merged PRs bypass the hooks that run these gates): - typecheck:core (Lint job): 3 now-unused @ts-expect-error in mcp-server/server.ts (diegosouzapw#3077 dynamic tool loops) → @ts-ignore (lenient, no TS2578). - pack-artifact-policy.test.ts: build-reorg (diegosouzapw#3124) renamed app/->dist/; the test still asserted app/ paths + REQUIRED order (it sorts alphabetically). - electron-packaging.test.ts: extraResources from .next/electron-standalone -> .build/electron-standalone (diegosouzapw#3124). - glm-provider-model-import-route.test.ts: two GLM connections shared one apiKey, so diegosouzapw#3100 (diegosouzapw#3023) dedup collapsed them → only one discovery fetch. Distinct keys. Remaining CI flakes (batch expiration, ModelSync self-fetch) pass in isolation — concurrency/port flakiness under --test-concurrency=4, not real failures.
Closes #3023
Problem
Adding the same API key twice for one provider (e.g. with a different or blank name) created a second connection row.
Root cause
createProviderConnectiononly deduped apikey connections by(provider, auth_type, name). There was no comparison of the key value, so a duplicate key under a new name inserted a fresh row.Fix
In the apikey branch, after the name-based upsert check, also look up existing apikey connections for the provider and compare the decrypted key (stored keys use non-deterministic AES-GCM, so ciphertext can't be compared — decrypt + compare trimmed plaintext). A match reuses/updates the existing connection.
Tests —
tests/unit/provider-connection-apikey-dedup.test.ts3 pass / 0 fail (RED→GREEN verified). ESLint clean (0 errors).