fix: use table recreation for sub_agent_skills UNIQUE constraint migration#2214
fix: use table recreation for sub_agent_skills UNIQUE constraint migration#2214nick-inkeep wants to merge 2 commits intomainfrom
Conversation
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: Medium
🟠⚠️ Major (1) 🟠⚠️
Inline Comments:
- 🟠 Major:
0010_oval_angel.sql:1Deployment strategy for environments where original migration succeeded
🟡 Minor (1) 🟡
🟡 1) 0010_oval_angel.sql Consider explicit transaction wrapping for atomicity
Issue: The migration uses 8 separate SQL statements with --> statement-breakpoint delimiters, meaning each executes independently without transaction wrapping.
Why: If any statement fails after Step 3 (DROP TABLE), the database is left without a sub_agent_skills table until manual intervention. The backup data would exist but the original table would be gone.
Fix: Given the PR description indicates thorough testing on both Doltgres v0.55.3 and PGlite (97 test files, 1525 tests), this is a lower-risk concern. PostgreSQL supports DDL in transactions, but Doltgres may have limitations. If both engines support transactional DDL, consider wrapping Steps 1-8 in BEGIN; / COMMIT;. Otherwise, the documented testing provides confidence in the current approach.
Refs:
💭 Consider (1) 💭
💭 1) 0010_oval_angel.sql Document deployment expectations
Issue: The PR description thoroughly covers the technical approach and test results, but could benefit from explicit deployment guidance.
Why: Operators deploying this need to understand which environments are affected and what to expect.
Fix: Consider adding a brief note in the PR description about:
- Doltgres environments: This migration fixes a previously failing migration
- PostgreSQL/PGlite environments: If the original migration succeeded, this new version will be skipped (same end-state achieved)
✅ What's Done Well
- Excellent documentation: Step-by-step comments in the migration clearly explain each phase of the backup→drop→recreate→restore approach
- Schema consistency verified: The migration SQL matches the Drizzle schema definition in
manage-schema.ts(lines 271-302) - Thorough testing documented: Both greenfield and brownfield testing on Doltgres v0.55.3 and PGlite with all 1525 tests passing
- Cross-tenant isolation tested: Existing test at
subAgentSkills.cross-tenant.test.tsvalidates the constraint behavior - No changeset needed: This is an internal infrastructure fix with no customer-facing impact
💡 APPROVE WITH SUGGESTIONS
Summary: This is a well-executed fix for a known database compatibility issue between Doltgres and PostgreSQL/PGlite. The backup→drop→recreate→restore approach is sound and avoids both the Doltgres DROP CONSTRAINT limitation and PGlite index name collisions. The one inline comment about deployment strategy is worth addressing to clarify expectations for operators, but the migration itself is technically correct and thoroughly tested.
Discarded (2)
| Location | Issue | Reason Discarded |
|---|---|---|
0010_oval_angel.sql:5-16 |
Nullable backup table columns vs NOT NULL restore | Intentionally lenient - backup captures any data; restore validates against new schema's NOT NULL + DEFAULT constraints. This is correct design. |
0010_oval_angel.sql |
No automated brownfield migration test | Valid observation but broader infrastructure concern; manual verification documented in PR is appropriate for this one-time fix. |
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-breaking-changes |
3 | 0 | 0 | 0 | 1 | 0 | 2 |
pr-review-devops |
5 | 1 | 1 | 0 | 0 | 0 | 0 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-tests |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 8 | 1 | 1 | 0 | 1 | 0 | 2 |
Note: pr-review-standards and pr-review-tests returned clean - migration is well-constructed and test coverage is adequate.
| @@ -1,2 +1,44 @@ | |||
| ALTER TABLE "sub_agent_skills" DROP CONSTRAINT "sub_agent_skills_sub_agent_skill_unique";--> statement-breakpoint | |||
| ALTER TABLE "sub_agent_skills" ADD CONSTRAINT "sub_agent_skills_sub_agent_skill_unique" UNIQUE("tenant_id","project_id","agent_id","sub_agent_id","skill_id"); | |||
| -- Migration: Widen the UNIQUE constraint on sub_agent_skills to include tenant_id, project_id, agent_id | |||
There was a problem hiding this comment.
🟠 MAJOR: Deployment strategy for environments where original migration succeeded
Issue: This PR modifies an existing migration file (0010_oval_angel.sql) that exists on main. Per AGENTS.md, existing migration files should not be edited after application. While the original migration fails on Doltgres (which is why this fix is needed), it may have succeeded on PostgreSQL/PGlite environments.
Why: Environments where the original DROP CONSTRAINT + ADD CONSTRAINT migration already ran successfully will:
- Have the migration marked as applied in
__drizzle_migrations - Skip this new version entirely on future deployments
- Be in a correct end-state but with different migration history
This isn't necessarily a problem if the end-state is the same, but it creates a divergence in migration history between environments.
Fix: The current approach appears acceptable given:
- The original migration fails on Doltgres (the production engine per PR description)
- Environments where it succeeded have the correct constraint already
- The backup-restore approach is idempotent in intent (achieving same end-state)
However, consider documenting:
- Deployment expectations in the PR description (which environment types this fixes vs. which already work)
- Whether any manual verification is needed for environments that ran the original migration
Refs:
- AGENTS.md: Database Migration Workflow — "NEVER edit existing migration SQL files after they've been applied"
|
Duplicate of #2208 |
87d25ec to
5cf1e50
Compare
…traint migration The previous migration used DROP CONSTRAINT + ADD CONSTRAINT which fails on Doltgres (can't drop UNIQUE constraints). PR #2208's approach of creating a _new table with same constraint names caused PGlite index name collisions. This uses backup→drop→recreate→restore: copy data to a temp table, drop the original (freeing all names), recreate with the wider UNIQUE constraint, then restore data. Tested on both Doltgres v0.55.3 and PGlite with zero data loss. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Integration tests are 43x slower than baseline (1465s vs 34s), causing the pagination test to timeout at 60s. Pinning to 0.55.2 (previous stable release) to rule out the 0.55.3 image as the cause. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
79e86f2 to
9fcd965
Compare
There was a problem hiding this comment.
PR Review Summary
(2) Total Issues | Risk: High
🔴❗ Critical (1) ❗🔴
Inline Comments:
- 🔴 Critical:
0010_oval_angel.sql:34Schema/migration constraint mismatch — Drizzle schema has 2-column UNIQUE but migration has 5-column
🟠⚠️ Major (1) 🟠⚠️
🟠 1) meta/_journal.json + 0010_snapshot.json Migration not tracked in Drizzle meta directory
Issue: The migration file 0010_oval_angel.sql was manually created but the Drizzle meta files are not updated:
meta/_journal.jsononly goes up to idx 9 (0009_chilly_old_lace) — missing entry for0010_oval_angel- No
0010_snapshot.jsonfile exists to record the post-migration schema state
Why: Without proper Drizzle tracking:
pnpm db:checkmay report schema driftpnpm db:generatewill not recognize this migration as the current state- Migration state tracking between environments will be inconsistent
- Future migrations may have incorrect numbering or conflicting changes
Fix: After updating the Drizzle schema (see Critical issue above), either:
- Run
pnpm db:generateto create properly tracked migration files (may need to manually adjust the generated SQL to use the backup→restore pattern) - Or manually add to
_journal.json:
{
"idx": 10,
"version": "7",
"when": 1771574400000,
"tag": "0010_oval_angel",
"breakpoints": true
}And create 0010_snapshot.json reflecting the post-migration schema state (copy from 0009_snapshot.json and update the sub_agent_skills unique constraint columns).
Refs:
🟡 Minor (1) 🟡
🟡 1) ci.yml, cypress.yml Doltgres version inconsistency across environments
Issue: CI workflows pin Doltgres to 0.55.2, but other environments use different versions:
docker-compose.dbs.yml:latestdocker-compose.yml:0.54.10create-agents-template:latest
Why: This creates potential environment-specific issues that may not surface until production.
Fix: The commit message explains this pin is for investigating a performance regression — this is acceptable for debugging. Once investigation concludes, consider aligning versions or documenting the rationale in comments.
Refs:
💭 Consider (1) 💭
💭 1) ci.yml:254-259, cypress.yml:104-109 SpiceDB image not pinned in CI
Issue: While Doltgres is pinned, SpiceDB runs via docker run authzed/spicedb serve without a version tag, defaulting to latest.
Why: Creates supply chain risk and non-deterministic builds.
Fix: Consider pinning to a specific version (e.g., authzed/spicedb:v1.49.1 to match docker-compose.yml).
Refs:
🕐 Pending Recommendations (1)
- 🟠
0010_oval_angel.sql:1Deployment strategy for environments where original migration succeeded
🚫 REQUEST CHANGES
Summary: The backup→drop→recreate→restore migration approach is technically sound for working around Doltgres/PGlite compatibility issues. However, the Drizzle schema and migration are out of sync — the schema still defines a 2-column UNIQUE constraint while the migration creates a 5-column constraint. This will cause test/production divergence (PGlite tests use schema, Doltgres uses migrations) and future migration drift. Additionally, the Drizzle meta files (_journal.json, snapshot) are not updated, breaking Drizzle's migration tracking. Please update the schema to match the migration and ensure proper Drizzle tracking before merging.
Discarded (4)
| Location | Issue | Reason Discarded |
|---|---|---|
0010_oval_angel.sql:19-37 |
Backup-restore lacks transaction wrapping | PR description documents extensive testing on both engines; risk is acceptable given documented validation |
0010_oval_angel.sql:37 |
Data restoration may fail on constraint violations | PR explicitly states brownfield testing with existing data preserved; low risk |
ci.yml:292 |
Missing trailing newline fixed | Positive change, no action needed |
ci.yml:156 |
Version pinning rationale not documented in file | Commit message documents rationale; not blocking |
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-breaking-changes |
4 | 1 | 0 | 0 | 1 | 0 | 2 |
pr-review-standards |
2 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-devops |
4 | 1 | 1 | 0 | 0 | 0 | 2 |
| Total | 10 | 2 | 1 | 0 | 1 | 1 | 4 |
Note: pr-review-standards findings merged into pr-review-breaking-changes Critical as they identified the same schema/migration mismatch issue.
| "created_at" timestamp DEFAULT now() NOT NULL, | ||
| "updated_at" timestamp DEFAULT now() NOT NULL, | ||
| CONSTRAINT "sub_agent_skills_tenant_id_project_id_agent_id_id_pk" PRIMARY KEY("tenant_id","project_id","agent_id","id"), | ||
| CONSTRAINT "sub_agent_skills_sub_agent_skill_unique" UNIQUE("tenant_id","project_id","agent_id","sub_agent_id","skill_id") |
There was a problem hiding this comment.
🔴 CRITICAL: Schema/migration constraint mismatch
Issue: This migration creates a 5-column UNIQUE constraint, but the Drizzle schema definition at manage-schema.ts:293 still defines a 2-column constraint:
// Current schema (line 293):
unique('sub_agent_skills_sub_agent_skill_unique').on(table.subAgentId, table.skillId)
// Migration creates (line 34):
UNIQUE("tenant_id","project_id","agent_id","sub_agent_id","skill_id")Why: This mismatch causes critical issues:
- Test/Production divergence: PGlite tests apply the Drizzle schema directly (2-column constraint), while Doltgres production uses migrations (5-column constraint)
- Future migration drift: Running
pnpm db:generatewill detect a discrepancy and generate a migration to revert back to 2 columns - Missing Drizzle tracking: The journal (
meta/_journal.json) is missing the 0010 entry, and no0010_snapshot.jsonexists
Fix: Update the Drizzle schema to match the migration:
// manage-schema.ts line 293 should be:
unique('sub_agent_skills_sub_agent_skill_unique').on(
table.tenantId,
table.projectId,
table.agentId,
table.subAgentId,
table.skillId
)Then either:
- Option A: Run
pnpm db:generateto regenerate the migration with proper meta files (preferred if Drizzle can generate the backup→restore pattern, which it likely cannot) - Option B: Manually add the journal entry and create
0010_snapshot.jsonto match the post-migration state
Refs:
Summary
DROP CONSTRAINT + ADD CONSTRAINTmigration with a backup→drop→recreate→restore approach that works on both Doltgres and PostgreSQL/PGlitesub_agent_skillsUNIQUE constraint from(sub_agent_id, skill_id)to(tenant_id, project_id, agent_id, sub_agent_id, skill_id)— scoping it per-agentProblem
Migration
0010_oval_angel.sqlused standard PostgreSQLALTER TABLE DROP CONSTRAINTwhich fails on Doltgres because it can't find UNIQUE constraints (they're stored as indexes, not in the constraint store). PR #2208 attempted a table recreation fix but created index name collisions on PGlite because it used the same constraint names on the_newtable while the original still existed.Approach
sub_agent_skills_backup(no constraints) → copy all datasub_agent_skills(frees all constraint/index names)sub_agent_skillswith correct wider UNIQUE + PK inlineThis avoids both the Doltgres DROP CONSTRAINT bug and the PGlite per-schema index name collision.
Test plan
Validated on 2026-02-20 across both database engines:
PGlite (CI/test engine)
subAgentSkills.cross-tenant.test.ts— cross-tenant isolation worksclient.test.ts— fresh database with all migrations applied (greenfield)Doltgres Docker v0.55.3 (production engine)
Alternatives evaluated and rejected
DROP CONSTRAINT + ADD CONSTRAINTCREATE TABLE _newwith same namesDROP INDEXon backing indexALTER TABLE RENAME CONSTRAINT🤖 Generated with Claude Code