PR Review Summary

(1) Total Issues | Risk: Low

🟠⚠️ Major (1) 🟠⚠️

🟠 1) manage-schema.ts:293-299 Missing regression test for cross-tenant skill isolation

Issue: This PR fixes a multi-tenancy bug where the unique constraint on sub_agent_skills was scoped only to (sub_agent_id, skill_id) instead of the full tenant hierarchy. However, no regression test is included to prevent this bug from recurring.

Why: A similar fix in PR #2151 included comprehensive scoping tests (subAgentSkills.scoping.test.ts), but those tests only cover same-tenant/same-project/different-agent isolation — not the cross-tenant scenario this PR fixes. Without a specific test for cross-tenant isolation, this bug could reoccur if someone modifies the unique constraint. The production scenario: Tenant A creates sub-agent research-agent with skill documentation-lookup. When Tenant B tries to create the same combination, they get a unique constraint violation that blocks legitimate operations.

Fix: Add a regression test that explicitly verifies different tenants can create the same (sub_agent_id, skill_id) combination. Following the pattern in existing scoping tests:

// packages/agents-core/src/__tests__/data-access/scoping/subAgentSkills.cross-tenant.test.ts
it('should allow different tenants to use same sub_agent_id + skill_id combination', async () => {
  const tenant1 = 'tenant-1';
  const tenant2 = 'tenant-2';
  const sharedSubAgentId = 'shared-subagent';
  const sharedSkillId = 'shared-skill';
  
  // Create projects, agents, sub-agents, and skills for both tenants
  // ...
  
  // First tenant's insert should succeed
  await db.insert(subAgentSkills).values([{
    tenantId: tenant1, projectId: project1Id, agentId: agent1Id,
    subAgentId: sharedSubAgentId, skillId: sharedSkillId, id: generateId(), index: 0
  }]);
  
  // Second tenant's insert should also succeed (not throw unique constraint error)
  await expect(
    db.insert(subAgentSkills).values([{
      tenantId: tenant2, projectId: project2Id, agentId: agent2Id,
      subAgentId: sharedSubAgentId, skillId: sharedSkillId, id: generateId(), index: 0
    }])
  ).resolves.not.toThrow();
});

Refs:

• subAgentSkills.scoping.test.ts — existing scoping tests (same-tenant only)
• agentFull.components-scoping.test.ts — reference pattern from similar fix in PR #2139

💭 Consider (1) 💭

Inline Comments:

• 💭 Consider: 0010_oval_angel.sql:2 Missing trailing newline (POSIX file convention)

Summary: The schema fix is correct and properly scopes the unique constraint to the full tenant/project/agent hierarchy. The migration safely relaxes the constraint (more columns = less restrictive). The data access layer already enforces this scoping, so this brings database-level enforcement in line with application-level behavior.

The one actionable suggestion is adding a regression test for cross-tenant isolation — similar fixes have included such tests, and it would prevent this class of bug from recurring. This is a minor gap that doesn't block the fix, but would strengthen confidence in the change.

Nice catch on this multi-tenancy isolation bug! 🎉

PR Review Summary

(0) Total Issues | Risk: Low

✅ Prior Feedback Addressed

Both issues from the prior review have been resolved:

Delta Review: New Test File

The new test file (packages/agents-core/src/__tests__/data-access/subAgentSkills.cross-tenant.test.ts) is well-constructed:

Test 1 — Cross-tenant isolation (lines 15-111):

• Creates two separate tenants with distinct projects and agents
• Uses identical sub_agent_id and skill_id values across tenants (the exact scenario that caused the production bug)
• Verifies the second tenant's insert succeeds (.resolves.not.toThrow())
• This would have failed with the old constraint

Test 2 — Same-tenant duplicate prevention (lines 113-173):

• Ensures the fix did not regress the intended uniqueness behavior
• Verifies duplicates within the same tenant/project/agent/subagent are still rejected

Quality observations:

• Follows established patterns from existing scoping tests
• Uses createTestProject helper for proper tenant/project setup
• Clear test names describing the behavior being verified
• Tests at the database level without mocking (appropriate for constraint testing)

Summary: All prior review feedback has been addressed. The regression test comprehensively covers the cross-tenant isolation fix while verifying same-tenant uniqueness is preserved. Nice work! 🎉