Revert "Revert "Revert "fix: scope sub_agent_skills unique constraint to tenant/project/agent"""#2215
Conversation
… to tena…" This reverts commit 56fd821.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
amikofalvy
deleted the
revert-2207-revert-2205-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: High
⚠️ Note: This PR has already been merged. This review documents identified concerns for awareness and potential follow-up.
🔴❗ Critical (1) ❗🔴
Inline Comments:
- 🔴 Critical:
manage-schema.ts:293Cross-tenant data isolation gap — unique constraint(subAgentId, skillId)lacks tenant scoping, allowing one tenant's configuration to block another tenant from using the same ID combination
🟠⚠️ Major (1) 🟠⚠️
🟠 1) subAgentSkills.cross-tenant.test.ts Deleted regression test removes multi-tenant isolation coverage
Issue: The deleted test file contained explicit validation that different tenants CAN use the same (subAgentId, skillId) combination without constraint violations, and that the same tenant/agent CANNOT create duplicates. This was a regression test for the original fix in PR #2190.
Why: Without this test, future changes could reintroduce the same isolation bug without any automated detection. The remaining subAgentSkills.scoping.test.ts tests query-level scoping, not constraint-level uniqueness behavior.
Fix: If the narrower constraint is intentional and permanent, document why cross-tenant collision is acceptable for this table. If not intentional, restore the test along with the tenant-scoped constraint in a follow-up PR.
Refs:
- Existing scoping test — tests query behavior, not constraint behavior
🟡 Minor (1) 🟡
🟡 1) 0010_oval_angel.sql Migration file deletion may cause schema drift
Issue: The migration that widened the unique constraint was deleted rather than rolled back via a new migration. If any database environment (staging, production) applied migration 0010 before this revert, those databases now have the tenant-scoped constraint while the schema file claims the narrower constraint.
Why: Schema drift between Drizzle-managed schema and actual database state can cause unexpected db:check or db:generate results, and potentially silent data integrity issues.
Fix: Verify whether migration 0010 was ever applied to any persistent environment. If so, create a rollback migration (0011) to explicitly revert the constraint change. Document the verification in the PR or follow-up issue.
Refs:
- data-model-changes skill — "NEVER edit existing migration SQL files after they've been applied - create new migrations instead"
💡 APPROVE WITH SUGGESTIONS
Summary: This PR has already been merged, so this review serves as documentation. The primary concern is a multi-tenant data isolation gap: the narrowed unique constraint (subAgentId, skillId) allows one tenant's configuration choices to block another tenant from using the same ID combination. This appears to be an operational compromise (the revert chain suggests migration difficulties) rather than an intentional architectural decision.
Recommended follow-up:
- File an issue to track re-implementing the tenant-scoped constraint with a reliable migration strategy
- Verify no cross-tenant collisions exist in production data
- Consider whether sub_agent_id generation should use globally unique IDs (e.g., nanoid) to mitigate the risk
Discarded (2)
| Location | Issue | Reason Discarded |
|---|---|---|
manage-schema.ts |
Inconsistent uniqueness pattern across join tables | Other join tables (subAgentToolRelations, etc.) intentionally lack unique constraints to allow different configurations via different id values. The pattern inconsistency is acceptable given different use cases. |
_journal.json |
Migration journal may be inconsistent | Journal correctly shows no entry for migration 0010, indicating proper handling via pnpm db:drop or equivalent. |
Reviewers (5)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-security-iam |
1 | 0 | 0 | 0 | 1 | 0 | 0 |
pr-review-architecture |
4 | 1 | 0 | 0 | 0 | 0 | 1 |
pr-review-breaking-changes |
3 | 1 | 0 | 0 | 0 | 0 | 1 |
pr-review-tests |
2 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-standards |
1 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 11 | 2 | 0 | 0 | 1 | 0 | 2 |
Note: Findings from multiple reviewers merged due to overlapping identification of the same cross-tenant isolation issue.
| table.subAgentId, | ||
| table.skillId | ||
| ), | ||
| unique('sub_agent_skills_sub_agent_skill_unique').on(table.subAgentId, table.skillId), |
There was a problem hiding this comment.
🔴 CRITICAL: Cross-tenant data isolation gap in unique constraint
Issue: The unique constraint (subAgentId, skillId) lacks tenant scoping. Since subAgentId is scoped per-agent (not globally unique via the PK at line 282) and skillId is scoped per-project (FK at line 288-291), different tenants using the same subAgentId and skillId combination will collide at the database level.
Why: This is a multi-tenant isolation bug. If Tenant A creates a sub_agent_skill with (subAgentId='qa-agent', skillId='summarize'), Tenant B cannot create the same logical combination in their own project — the database will reject it with a unique constraint violation. This violates tenant isolation and could cause one tenant's configuration choices to affect another tenant's ability to configure their agents.
Fix: The constraint should include tenant scoping columns to match the FK patterns used elsewhere in the table:
unique('sub_agent_skills_sub_agent_skill_unique').on(
table.tenantId,
table.projectId,
table.agentId,
table.subAgentId,
table.skillId
)Refs:
- subAgents PK includes tenant/project/agent scope
- skills PK includes tenant/project scope
- The deleted test
subAgentSkills.cross-tenant.test.tsexplicitly validated this isolation property
1 participant
Reverts #2207