Revert "fix: scope sub_agent_skills unique constraint to tenant/project/agent"#2205
Conversation
…ct/agent…" This reverts commit 6cf779d.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
amikofalvy
deleted the
revert-2190-fix/sub-agent-skills-unique-constraint
branch
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(3) Total Issues | Risk: Medium
⚠️ Post-merge review — This PR was already merged. The findings below are for documentation and potential follow-up actions.
🟠⚠️ Major (2) 🟠⚠️
🟠 1) manage-schema.ts:293 Multi-tenant isolation regression risk
Issue: The unique constraint on sub_agent_skills reverts from (tenantId, projectId, agentId, subAgentId, skillId) to (subAgentId, skillId). Since subAgentId is scoped to tenant/project/agent (per the sub_agents table's PK at line 240), this narrower constraint could cause cross-tenant collisions.
Why: The original fix (PR #2190) addressed a multi-tenancy isolation bug where different tenants using the same sub-agent ID + skill ID combination would conflict. With this revert:
- Tenant A creates
(subAgentId: "helper", skillId: "coding")✓ - Tenant B attempts same combination → unique constraint violation ✗
This violates multi-tenant isolation where each tenant's data should be independent.
Fix: If there were operational issues with the original fix (migration failures, data conflicts), consider:
- Identifying and resolving the root cause
- Re-applying the fix with a new migration
- Or documenting why the narrower constraint is acceptable (e.g., if subAgentIds are UUID-generated and effectively unique)
Refs:
- sub_agents PK —
subAgentIdis(tenantId, projectId, agentId)scoped - Original fix: PR #2190
🟠 2) 0010_oval_angel.sql (deleted) Migration deletion may cause schema drift
Issue: Deleting migration file 0010 rather than creating a rollback migration (0011) violates the data-model-changes rule: "NEVER manually delete migration files — use pnpm db:drop" and "create new migrations instead of editing/deleting existing ones".
Why: If migration 0010 was applied to any environment (staging, production) before this revert merged:
- Those environments have the broader constraint
(tenantId, projectId, agentId, subAgentId, skillId) - The codebase now expects the narrower constraint
(subAgentId, skillId) - Running
pnpm db:migratewill not detect or fix this drift - Future migrations may fail due to constraint name conflicts
Fix: Verify environment state:
- Check if migration 0010 was deployed to production
- If yes, create migration 0011 to explicitly revert the constraint:
ALTER TABLE "sub_agent_skills" DROP CONSTRAINT "sub_agent_skills_sub_agent_skill_unique"; ALTER TABLE "sub_agent_skills" ADD CONSTRAINT "sub_agent_skills_sub_agent_skill_unique" UNIQUE("sub_agent_id","skill_id");
- Run
pnpm db:checkto verify schema consistency
Refs:
- AGENTS.md migration rules — "NEVER manually delete migration files"
Inline Comments:
- 🟠 Major:
manage-schema.ts:293Multi-tenant isolation regression
🟡 Minor (1) 🟡
🟡 1) subAgentSkills.cross-tenant.test.ts (deleted) Cross-tenant test coverage removed
Issue: The deleted test (174 lines) validated two critical behaviors:
- Different tenants CAN use the same
(subAgentId, skillId)combination - Same tenant/project/agent CANNOT duplicate skill assignments
Why: Without this test, regressions in multi-tenant isolation won't be caught. The remaining subAgentSkills.scoping.test.ts only tests within-project scoping, not cross-tenant scenarios.
Fix: If the revert is intentional, consider:
- Retaining the test with
.skipand a comment explaining the known limitation - Or creating an issue to track the multi-tenant constraint problem
Refs:
- Remaining scoping test — tests within-tenant only
💡 APPROVE WITH SUGGESTIONS
Summary: This revert PR was already merged, so this review serves as documentation. The revert potentially reintroduces a multi-tenant isolation issue where different tenants could conflict on (subAgentId, skillId) combinations. If the revert was necessary due to operational issues with the original fix, consider tracking this as a known limitation and planning a proper resolution. The migration deletion approach also risks schema drift if the migration was deployed before the revert.
Discarded (1)
| Location | Issue | Reason Discarded |
|---|---|---|
meta/_journal.json |
Manual editing of Drizzle meta files | Covered by the migration deletion finding — same root cause |
Reviewers (4)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-breaking-changes |
4 | 2 | 0 | 0 | 1 | 0 | 1 |
pr-review-architecture |
3 | 1 | 0 | 0 | 0 | 0 | 2 |
pr-review-tests |
1 | 1 | 0 | 0 | 0 | 0 | 0 |
| Total | 8 | 3 | 0 | 0 | 1 | 0 | 3 |
Note: Architecture and tests findings largely overlapped with breaking-changes findings and were consolidated.
| table.subAgentId, | ||
| table.skillId | ||
| ), | ||
| unique('sub_agent_skills_sub_agent_skill_unique').on(table.subAgentId, table.skillId), |
There was a problem hiding this comment.
🟠 MAJOR Multi-tenant isolation regression risk
Issue: This revert changes the unique constraint from (tenantId, projectId, agentId, subAgentId, skillId) back to (subAgentId, skillId). Since subAgentId is scoped to tenant/project/agent (not globally unique), this constraint could cause cross-tenant collisions if different tenants create sub-agents with the same ID.
Why: The original fix (PR #2190) addressed a multi-tenancy isolation bug. With this narrower constraint, if Tenant A creates a skill assignment for (subAgentId: "helper", skillId: "coding"), Tenant B cannot create the same combination — even though they're separate tenants with independent data.
Refs:
- sub_agents PK definition — shows
subAgentIdis scoped to(tenantId, projectId, agentId), not globally unique - Original fix: PR fix: scope sub_agent_skills unique constraint to tenant/project/agent #2190
1 participant
Reverts #2190