Revert "Revert "fix: scope sub_agent_skills unique constraint to tenant/project/agent""#2207
Conversation
…nt/proje…" This reverts commit 1c5138d.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 3bbcfa2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
amikofalvy
deleted the
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
(0) Total Issues | Risk: Low
This PR re-applies the multi-tenant scoping fix for the sub_agent_skills unique constraint (originally from PR #2190, reverted in #2205). The change expands the constraint from (subAgentId, skillId) to (tenantId, projectId, agentId, subAgentId, skillId), properly aligning uniqueness enforcement with the existing FK scoping hierarchy.
🔴❗ Critical (0) ❗🔴
None
🟠⚠️ Major (0) 🟠⚠️
None
🟡 Minor (0) 🟡
None
💭 Consider (1) 💭
Inline Comments:
- 💭 Consider:
subAgentSkills.cross-tenant.test.ts:174Additional constraint boundary tests for cross-project/cross-agent isolation
✅ APPROVE
Summary: Clean implementation of a multi-tenant isolation fix. The migration safely relaxes the constraint (allows more combinations rather than restricting), the schema change aligns with established patterns in similar join tables, and the tests verify both the fix (cross-tenant isolation) and preserved behavior (same-scope deduplication). The one "Consider" suggestion is for expanded test coverage depth but is not blocking — current tests adequately validate the fix. 🎉
Discarded (0)
No findings were discarded.
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 | 0 | 0 | 0 | 0 | 0 | 4 |
pr-review-tests |
3 | 0 | 0 | 0 | 1 | 0 | 2 |
pr-review-types |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 7 | 0 | 0 | 0 | 1 | 0 | 6 |
Note: pr-review-breaking-changes returned 4 INFO-level confirmations that the migration is safe (all assessed as not issues). pr-review-tests returned 2 additional MINOR suggestions that were consolidated into the single inline comment.
| }) | ||
| ).rejects.toThrow(); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
💭 Consider Additional constraint boundary tests
Issue: The tests validate cross-tenant isolation and same-scope deduplication, but don't explicitly test cross-project (same tenant) or cross-agent (same tenant/project) isolation at the constraint level.
Why: The unique constraint includes 5 columns: (tenantId, projectId, agentId, subAgentId, skillId). While the cross-tenant test implicitly covers different projects, explicit tests for same-tenant-different-project and same-project-different-agent scenarios would catch subtle constraint regressions (e.g., if projectId or agentId were accidentally removed from the constraint).
Fix: Consider adding tests like:
it('should allow same tenant with different projects to use identical sub_agent_id + skill_id')it('should allow different agents in same project to use identical sub_agent_id + skill_id')
This is optional since the existing subAgentSkills.scoping.test.ts tests query-level isolation, but explicit constraint-level tests would provide defense-in-depth.
Refs:
1 participant
Reverts #2205