feat(db): add project_credentials table and migration 0040#844
Conversation
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
Clean, well-structured PR. The migration logic is correct, the role-to-env_var_key mapping matches the PROVIDER_CREDENTIAL_ROLES source of truth, the reEncryptCredential helper is properly tested, and the Drizzle schema follows existing conventions. One schema/migration consistency issue to address.
Should Fix
Timestamp type mismatch — src/db/schema/projectCredentials.ts:15-18 vs src/db/migrations/0040_project_scoped_credentials.sql:19-20
The migration uses TIMESTAMPTZ for created_at and updated_at, but the Drizzle schema uses timestamp() without { withTimezone: true }. Drizzle's timestamp() maps to PostgreSQL timestamp without time zone, not timestamptz. This means the Drizzle schema doesn't accurately reflect the actual DB column types, which could cause issues if db:generate or db:push is ever run (it would detect a diff and try to alter the columns).
Two options to fix:
- Match existing
credentialstable pattern: Change the SQL migration fromTIMESTAMPTZtoTIMESTAMP(lines 19-20 of the migration). This is consistent with how other credential-adjacent tables (credentials,integrations,organizations) are defined. - Match
prWorkItemspattern: Change Drizzle schema totimestamp('created_at', { withTimezone: true }).defaultNow()and same forupdated_at. Then keepTIMESTAMPTZin the migration.
Either approach works — the key is that the Drizzle schema and SQL migration should agree on the type.
Everything else looks good
- ✅ The
CASErole-to-env_var_key mapping in migration Step 4 exactly matchesPROVIDER_CREDENTIAL_ROLESinsrc/config/integrationRoles.ts - ✅ The
reEncryptCredentialhelper correctly handles all three cases (encrypted→re-encrypt, plaintext→passthrough, no key→passthrough) with good test coverage - ✅ The backfill logic correctly uses
ON CONFLICT DO NOTHINGfor org defaults (Step 3) andON CONFLICT DO UPDATEfor integration overrides (Step 4), so integration-specific credentials take precedence - ✅ The Drizzle schema structure follows existing patterns (
serialPK,textFK toprojects.id,uniqueIndexnaming convention) - ✅ Migration journal entry is correctly sequenced
- ✅ All CI checks pass
🕵️ claude-code · claude-opus-4-6 · run details
Change TIMESTAMPTZ to TIMESTAMP in 0040 migration so the SQL column type matches the Drizzle schema's timestamp() (without timezone), consistent with the credentials and integrations table patterns. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
nhopeatall
left a comment
There was a problem hiding this comment.
Summary
LGTM — Clean, well-structured migration that adds the project_credentials table with correct backfill logic.
Verification
- Drizzle schema ↔ SQL migration: Table definition, column types, nullability, FK constraint, and unique index are consistent between
projectCredentials.tsand0040_project_scoped_credentials.sql. - CASE mapping: The
role → env_var_keymapping in Step 4 exactly matchesPROVIDER_CREDENTIAL_ROLESinsrc/config/integrationRoles.ts(all 8 roles across trello/jira/github). TheELSE ic.rolefallback is unreachable due to theWHERE ic.role IN (...)filter — defensive but harmless. - Backfill ordering: Step 3 inserts org defaults with
DO NOTHING, Step 4 inserts integration-specific withDO UPDATE— correct precedence (integration credentials override org defaults). - Encryption strategy: Values are copied as-is (AAD stays
orgId), with re-encryption deferred to a post-migration tool. This is the right call — avoids needing the master key during migration and keeps the migration purely SQL. reEncryptCredentialhelper: Correctly delegates to existingisEncryptedValue→decryptCredential→encryptCredentialpipeline. All edge cases tested (encrypted, plaintext, encryption disabled).- Schema export: Correctly added to
src/db/schema/index.ts. - Journal entry:
idx: 40,when: 1775000000000follows the established monotonic sequence. - CI: All checks passing.
🕵️ claude-code · claude-opus-4-6 · run details
Summary
project_credentialstable with unique(project_id, env_var_key)constraint and AES-GCM encryption support0040_project_scoped_credentials.sqlthat creates the new table and backfills existing org-scoped + integration credentials into per-project rowsreEncryptCredential(value, oldAad, newAad)helper tosrc/db/crypto.tsfor decrypt-then-re-encrypt with a different AADsrc/db/schema/projectCredentials.tsand exports fromsrc/db/schema/index.tsTrello card: https://trello.com/c/Kf8oZsLx/365-as-a-developer-i-want-a-projectcredentials-table-and-migration-so-that-secrets-can-be-stored-per-project
Changes
New files
src/db/schema/projectCredentials.ts— Drizzle schema for the new tablesrc/db/migrations/0040_project_scoped_credentials.sql— SQL migration with CREATE TABLE, backfill from org defaults, and backfill from integration credentials (overriding defaults)Modified files
src/db/crypto.ts— addsreEncryptCredential(stored, oldAad, newAad)helpersrc/db/schema/index.ts— exportsprojectCredentialssrc/db/migrations/meta/_journal.json— adds journal entry for migration 0040tests/unit/db/crypto.test.ts— 3 new tests forreEncryptCredentialMigration behaviour
project_credentials(id, project_id FK→projects, env_var_key, value, name, created_at, updated_at)withUNIQUE(project_id, env_var_key)is_default=truecredentials from thecredentialstable into every project in the same orgPROVIDER_CREDENTIAL_ROLES), overriding org defaults on conflictorgId); re-encryption withprojectIdas AAD can be done post-migration via thereEncryptCredentialhelperNo runtime changes
This PR only adds the table and backfills data. Existing credential resolution continues to use the old
credentialsandintegration_credentialstables unchanged.Test plan
reEncryptCredentialunit tests pass (3 new test cases)crypto.test.tstests pass🕵️ claude-code · claude-sonnet-4-6 · run details