Revert "Revert "Revert "Revert "fix: scope sub_agent_skills unique constraint to tenant/project/agent""""#2241
Conversation
…nstraint…" This reverts commit 7847ae1.
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
🟡 Minor (0) 🟡
No minor issues found.
💭 Consider (1) 💭
Inline Comments:
- 💭 Consider:
subAgentSkills.cross-tenant.test.ts:164Assert specific constraint error type instead of generic.toThrow()
✅ APPROVE
Summary: This PR correctly fixes a multi-tenant isolation bug in the sub_agent_skills table. The unique constraint now properly includes tenant_id, project_id, and agent_id, aligning with the table's hierarchical scoping model and preventing cross-tenant collisions. The migration is safe (the new constraint is a superset of the original), comprehensive tests validate the fix, and all reviewers confirmed the implementation is correct. Ship it! 🚀
Discarded (4)
| Location | Issue | Reason Discarded |
|---|---|---|
0010_oval_angel.sql:1-2 |
Migration transaction safety concern | Drizzle migrations run in transactions by default; new constraint is a superset making violations impossible |
subAgentSkills.cross-tenant.test.ts |
Missing cross-project isolation test | Cross-tenant test adequately validates multi-column constraint behavior |
subAgentSkills.cross-tenant.test.ts |
Missing cross-agent isolation test | Already implicitly covered by existing subAgentSkills.scoping.test.ts |
0010_oval_angel.sql:1-2 |
Constraint is relaxation, not tightening | Informational confirmation, not an issue |
Reviewers (5)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-breaking-changes |
3 | 0 | 0 | 0 | 0 | 0 | 3 |
pr-review-tests |
3 | 0 | 0 | 0 | 1 | 0 | 2 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-types |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-security-iam |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 6 | 0 | 0 | 0 | 1 | 0 | 5 |
Note: Types and Security-IAM reviewers explicitly confirmed the fix is correct for multi-tenant isolation.
| await expect( | ||
| db.insert(subAgentSkills).values({ | ||
| tenantId, | ||
| projectId, |
There was a problem hiding this comment.
💭 Consider: Assert specific constraint error
Issue: The test uses .rejects.toThrow() without specifying the error type or message.
Why: This test would pass even if the insert failed for an unrelated reason (e.g., foreign key violation, connection error). Asserting the specific constraint name makes the test more robust against false positives.
Fix: Consider specifying the constraint name in the assertion:
).rejects.toThrow(/sub_agent_skills_sub_agent_skill_unique|unique constraint|duplicate key/i);Refs:
- PostgreSQL error codes — code
23505is unique_violation
Reverts #2215
Going for some kind of reversion record, I suppose.