Make data components and artifact components unique across agents#2139
Conversation
🦋 Changeset detectedLatest commit: b19b087 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
(2) Total Issues | Risk: High
🔴❗ Critical (1) ❗🔴
Inline Comments:
- 🔴 Critical:
agents.ts:569-570Copy-paste bug usingsubAgentDataComponentsinstead ofsubAgentArtifactComponentsfor projectId/agentId filters
🟠⚠️ Major (1) 🟠⚠️
🟠 1) agents.ts:556-576 Missing test coverage for component isolation
Issue: The data component and artifact component isolation fix has no test coverage. The existing tests in agentFull.test.ts mock the findMany calls to return empty arrays but do not test the scenario where multiple agents exist and the query parameters must correctly isolate components to the specific agent being queried.
Why: Without a test that verifies agent-level isolation of data/artifact components, a regression could cause components from one agent to leak into another agent's definition. This could result in: (1) agents receiving incorrect UI components, (2) data components appearing for the wrong sub-agent, (3) potential data isolation issues if components contain sensitive configuration.
Fix: Add a test case that:
- Creates two agents (Agent A and Agent B) in the same project
- Creates data components and artifact components assigned to each agent's sub-agents
- Calls
getFullAgentDefinitionfor Agent A - Verifies that only Agent A's data/artifact components are returned, not Agent B's
Example test structure:
it('should isolate data and artifact components to the specific agent', async () => {
// Setup: Create Agent A with dataComponentA and artifactComponentA
// Setup: Create Agent B with dataComponentB and artifactComponentB
// Act: Get full definition for Agent A
const result = await getFullAgentDefinition(db)({
scopes: { tenantId, projectId, agentId: agentA.id }
});
// Assert: Only Agent A's components are returned
expect(result.subAgents[0].dataComponents).toContain(dataComponentA.id);
expect(result.subAgents[0].dataComponents).not.toContain(dataComponentB.id);
expect(result.subAgents[0].artifactComponents).toContain(artifactComponentA.id);
expect(result.subAgents[0].artifactComponents).not.toContain(artifactComponentB.id);
});Refs:
- Existing tests — current test file for
getFullAgentDefinition
🚫 REQUEST CHANGES
Summary: The fix correctly identifies the need to add projectId and agentId filters for uniqueness, but contains a critical copy-paste bug on lines 569-570 that must be fixed before merging. The artifact component query incorrectly references subAgentDataComponents instead of subAgentArtifactComponents, which will cause the query to produce incorrect results. Additionally, this bug fix would benefit from test coverage to prevent future regressions.
Discarded (0)
No discarded findings.
Reviewers (2)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-standards |
1 | 0 | 0 | 0 | 1 | 0 | 0 |
pr-review-tests |
2 | 1 | 0 | 0 | 0 | 0 | 1 |
| Total | 3 | 1 | 0 | 0 | 1 | 0 | 1 |
Note: One finding from pr-review-tests was deduplicated with the inline comment (both identified the same copy-paste bug).
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: Medium
🔴❗ Critical (0) ❗🔴
None — the prior Critical issue has been resolved.
🟠⚠️ Major (0) 🟠⚠️
No new Major issues in the delta.
🟡 Minor (0) 🟡
No new Minor issues in the delta.
💭 Consider (0) 💭
No new Consider items.
🕐 Pending Recommendations (1)
- 🟠
agents.ts:556-576Missing test coverage for component isolation — the data/artifact component isolation fix has no test coverage to prevent regression
💡 APPROVE WITH SUGGESTIONS
Summary: The Critical copy-paste bug from the prior review has been correctly fixed. The artifact component query now properly references subAgentArtifactComponents instead of subAgentDataComponents for the projectId and agentId filters, bringing it in line with the established pattern used elsewhere in the function. The fix is correct and complete.
The only remaining concern is the test coverage gap raised in the prior review — without a test that verifies cross-agent component isolation, a future regression could reintroduce this bug silently. Consider adding a test case before merging, but this is not a blocking issue for the fix itself.
Discarded (0)
No discarded findings.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Orchestrator only | 0 | 0 | 0 | 0 | 0 | 1 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 1 | 0 |
Note: Subagent dispatch was skipped for this re-review. The delta consists of a 2-line typo fix that directly addresses the Critical issue raised in the prior automated review. No additional review was warranted for this trivial correction.
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
🔴❗ Critical (0) ❗🔴
None.
🟠⚠️ Major (0) 🟠⚠️
None — the pending "Missing test coverage" recommendation has been addressed.
🟡 Minor (0) 🟡
None.
💭 Consider (0) 💭
None.
✅ APPROVE
Summary: The test coverage gap has been addressed. The new agentFull.components-scoping.test.ts file adds a well-structured regression test that:
- Tests the exact bug scenario — two agents with identically-named subagents (
shared-subagent) - Verifies proper isolation — each agent's data/artifact components are scoped correctly
- Follows established patterns — uses
testManageDbClient,createTestProject,generateId, and standard Vitest patterns consistent with the ~30 other test files in the same directory
This test would fail if the projectId/agentId filters were removed from the queries, providing strong regression protection. Ship it! 🚀
Discarded (0)
No discarded findings.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Orchestrator only | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Subagent dispatch was skipped for this delta re-review. The delta consists of a single test file (171 lines) that directly addresses the "Missing test coverage" pending recommendation from the prior review. The test file follows established patterns and no additional specialized review was warranted.
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
🔴❗ Critical (0) ❗🔴
None.
🟠⚠️ Major (0) 🟠⚠️
None.
🟡 Minor (0) 🟡
None.
💭 Consider (0) 💭
None.
✅ APPROVE
Summary: The delta since the last review is a single changeset file (curly-knives-watch.md) that correctly documents this bug fix for version management. The changeset:
- Targets the correct package (
@inkeep/agents-core) - Uses appropriate severity (
patch) for a bug fix - Has a clear, accurate message: "Fix data and artifact components being shared across agents when subagents share the same ID"
All substantive issues from prior reviews have been addressed — the copy-paste bug was fixed and regression tests were added. This PR is ready to ship! 🚀
Discarded (0)
No discarded findings.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Orchestrator only | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: Subagent dispatch was skipped for this delta re-review. The delta consists of a single changeset file (5 lines) that documents the bug fix for version management. No code changes occurred since the last review which already approved the PR.
2 participants
Problem: When multiple agents had subagents with the same ID, the component queries only filtered by tenantId and subAgentId. That caused components to be shared across those subagents, and the component arrays to grow on each save.
Fix: Add projectId and agentId to the queries so components are scoped to the correct parent agent. Each subagent instance now gets only its own components.