Add scope-aware query helpers and junction table scoping isolation tests#2151
Conversation
Audit of the manage data-access layer found 9 queries on junction/scoped tables that did not filter on all scope columns, potentially leaking data across project or agent boundaries when child IDs overlap. Fixes: - deleteAgentRelationsByAgent: add missing projectId to DELETE - getAgentToolRelationByAgent: add missing agentId to SELECT (data + count) - deleteAgentDataComponentRelationByAgent: add missing projectId to DELETE - deleteAgentArtifactComponentRelationByAgent: add missing projectId to DELETE - stopWhen inheritance update: add missing agentId to UPDATE - fetchComponentRelationships: change ProjectScopeConfig to AgentScopeConfig, add agentId to WHERE, update both call sites - getDataComponentsForAgent: add tenantId + projectId to JOIN condition - getArtifactComponentsForAgent: add tenantId + projectId to JOIN condition - agentHasArtifactComponents: add projectId + agentId to JOIN condition Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Create tenantScopedWhere, projectScopedWhere, agentScopedWhere, subAgentScopedWhere helpers that enforce all scope columns for a given scope level - Convert subAgentExternalAgentRelations.ts to use helpers as proof of concept - Add scoping isolation tests for 9 junction tables: subAgentToolRelations, subAgentFunctionToolRelations, subAgentSkills, subAgentExternalAgentRelations, subAgentTeamAgentRelations, evaluationSuiteConfigEvaluatorRelations, evaluationJobConfigEvaluatorRelations, evaluationRunConfigSuiteConfigRelations, datasetRunConfigAgentRelations - Add unit tests for scope helper functions Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 704e878 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![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 is a well-designed Phase 2 of the scope filtering hardening work. The new scope-helpers.ts introduces compile-time type-safe query helpers that enforce all required scope columns for each hierarchy level (tenant → project → agent → subAgent). The proof-of-concept refactoring of subAgentExternalAgentRelations.ts demonstrates the pattern cleanly, and the 10 new test files provide comprehensive regression coverage for the 9 junction tables that were vulnerable in Phase 1.
No blocking issues identified. The code is well-structured, follows existing patterns appropriately, and the security-critical scoping logic is sound.
💭 Consider (2) 💭
💭 1) scope Add mutation operation scoping tests
Issue: The scoping isolation tests verify read operations (getSubAgentExternalAgentRelations, etc.) but don't test mutation operations (delete, update). If a future refactor incorrectly changes scoping in mutation functions, the current tests wouldn't catch it.
Why: While the scope helpers are used consistently for both reads and mutations, explicit testing of mutations would provide stronger regression protection for the security-critical scoping behavior.
Fix: Consider adding one representative mutation test per scope level in a follow-up. Example pattern:
// Setup: two agents with shared subAgentId, each with relations
// Action: deleteSubAgentExternalAgentRelationsBySubAgent for agent1
// Assert: agent1's relations deleted AND agent2's relations still existRefs: subAgentExternalAgentRelations.scoping.test.ts
💭 2) scope Add cross-tenant isolation tests
Issue: All scoping tests use a single tenantId = 'test-tenant' constant. Cross-project and cross-agent isolation is tested, but cross-tenant isolation (where different tenants have overlapping project/entity IDs) is not explicitly verified.
Why: Multi-tenant isolation is security-critical. While the scope helpers explicitly include tenantId in all WHERE clauses (so the implementation is correct), explicit cross-tenant tests would catch any regression where tenant scoping is accidentally removed.
Fix: Consider adding one cross-tenant test in a follow-up:
// Setup: tenant-a and tenant-b, each with projectId='shared-project'
// Insert records for each tenant
// Action: query with tenant-a's scope
// Assert: only tenant-a's records returnedRefs: datasetRunConfigAgentRelations.scoping.test.ts:22
✅ APPROVE
Summary: Excellent hardening work for multi-tenant data isolation. The scope helpers establish a clean pattern that provides compile-time safety against the class of bugs fixed in Phase 1. The comprehensive test coverage for 9 junction tables creates strong regression protection. The two suggestions above are minor enhancements for an already solid implementation. Ship it! 🚀
Discarded (6)
| Location | Issue | Reason Discarded |
|---|---|---|
scope-helpers.ts:10-13 |
Column type constraints use any |
Pragmatic design choice - type safety comes from enforcing column existence, not column types. Drizzle's eq() provides runtime type checking. |
scope-helpers.ts |
Only 1 of 20+ DAL files converted to use helpers | Intentional per PR description - this is Phase 2 / proof-of-concept. Split-world is acknowledged and planned. |
scope-helpers.ts:8 |
TenantScopeConfig defined locally instead of in types/utility.ts |
No existing TenantScopeConfig in utility.ts - only ProjectScopeConfig and above. Local definition is consistent. |
data-access/index.ts |
scope-helpers not exported from index | Intentional - these are internal utilities for DAL layer, not for external consumption. |
scoping/ directory |
New test directory structure | Reasonable organizational choice. Moving existing agentFull.components-scoping.test.ts is out of scope. |
scope-helpers.test.ts |
Unit tests don't verify actual DB behavior | Integration tests (9 scoping.test.ts files) provide DB-level verification. Unit tests appropriately verify clause construction. |
Reviewers (6)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-tests |
4 | 0 | 2 | 0 | 0 | 0 | 2 |
pr-review-consistency |
5 | 0 | 0 | 0 | 0 | 0 | 5 |
pr-review-architecture |
3 | 0 | 0 | 0 | 0 | 0 | 3 |
pr-review-types |
1 | 0 | 0 | 0 | 0 | 0 | 1 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-security-iam |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 13 | 0 | 2 | 0 | 0 | 0 | 11 |
Note: High discard rate reflects de-duplication across reviewers (3 reviewers flagged any types, 3 flagged split-world) and filtering of intentional design choices acknowledged in PR description.
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
Delta Review
This is a re-review scoped to changes since the last automated review (commit 6cc486d7afcf).
The delta consists entirely of merge artifacts from main:
.github/workflows/ci.yml— CI workflow optimizations from PR #2147 (already reviewed/merged)package.json— AddedsharpandtsxdevDependencies from PR #2152 (already reviewed/merged)
No changes to the actual PR content (scope-aware query helpers, scoping isolation tests, or DAL refactoring). The merge simply brought the branch up to date with main.
Prior Review Summary
The previous review approved this PR with 0 blocking issues:
- ✅
scope-helpers.tsprovides compile-time type-safe query helpers enforcing all scope columns - ✅ Proof-of-concept refactoring in
subAgentExternalAgentRelations.tsdemonstrates clean pattern - ✅ 10 new test files provide comprehensive scoping isolation coverage for 9 junction tables
- ✅ Security-critical multi-tenant scoping logic is sound
Two Consider items were noted (mutation operation tests, cross-tenant isolation tests) as potential follow-up enhancements — not blocking.
✅ APPROVE
Summary: No new issues in the delta. The merge from main brought in already-reviewed CI/devDependency changes. The core PR content remains solid — excellent Phase 2 hardening work for multi-tenant data isolation. Ship it! 🚀
1 participant
Summary
scope-helpers.ts):tenantScopedWhere,projectScopedWhere,agentScopedWhere,subAgentScopedWherefunctions that enforce all scope columns for a given scope level, making it a compile-time error to forget a columnsubAgentExternalAgentRelations.tsto use the new helpers, reducing boilerplate and eliminating the possibility of missing scope columnssubAgentToolRelationssubAgentFunctionToolRelationssubAgentSkillssubAgentExternalAgentRelationssubAgentTeamAgentRelationsevaluationSuiteConfigEvaluatorRelationsevaluationJobConfigEvaluatorRelationsevaluationRunConfigEvaluationSuiteConfigRelationsdatasetRunConfigAgentRelationsThis is Phase 2 of the scope filtering hardening work. Phase 1 (PR #2150) fixed the 9 existing bugs.
Test plan
🤖 Generated with Claude Code