From f7d5d5f787f2e59238ef5dbc421162eb7047c144 Mon Sep 17 00:00:00 2001 From: Brad Harris Date: Mon, 6 Jul 2026 18:19:20 -0600 Subject: [PATCH] Address security review: add null-repoRoot parent/child tests, trust comment - Add tests for senderRepoRoot=null + parent/child (the primary path the null-guard removal unlocked, previously untested) - Add comment documenting the trust assumption on parentAgentId Co-Authored-By: Claude Opus 4.6 --- apps/server/src/server/mcp-handlers.ts | 31 +-- apps/server/test/mcp-handlers.test.ts | 264 ++++++++++++++++++++++++- 2 files changed, 275 insertions(+), 20 deletions(-) diff --git a/apps/server/src/server/mcp-handlers.ts b/apps/server/src/server/mcp-handlers.ts index 916e7a60..9e990378 100644 --- a/apps/server/src/server/mcp-handlers.ts +++ b/apps/server/src/server/mcp-handlers.ts @@ -126,16 +126,23 @@ export function mcpMethodNotAllowed(): { /** * The set of agents a sender may address via dispatch_send_message and * list_agents: every other agent (self excluded), scoped to the sender's git - * repo root unless cross-repo messaging is enabled. This is the single - * definition of that visibility boundary — both message delivery and agent - * listing consult it, so they can never disagree about who is reachable. + * repo root unless cross-repo messaging is enabled. Direct parent ↔ child + * relationships always bypass repo-root scoping so spawned agents can + * coordinate with their parent regardless of working directory. This is the + * single definition of that visibility boundary — both message delivery and + * agent listing consult it, so they can never disagree about who is reachable. */ -async function addressableAgents( +async function addressableAgents< + T extends { id: string; cwd: string; parentAgentId?: string | null }, +>( all: T[], agentId: string, senderRepoRoot: string | null, crossRepo: boolean ): Promise { + const sender = all.find((a) => a.id === agentId); + const senderParentId = sender?.parentAgentId ?? null; + const result: T[] = []; for (const a of all) { if (a.id === agentId) continue; @@ -143,6 +150,12 @@ async function addressableAgents( result.push(a); continue; } + // Direct parent ↔ child always visible. parentAgentId is trusted because + // MCP-originated creation sets it server-side; the HTTP path is localhost-only. + if (a.id === senderParentId || a.parentAgentId === agentId) { + result.push(a); + continue; + } try { const aRoot = await resolveRepoRoot(a.cwd); if (aRoot === senderRepoRoot) result.push(a); @@ -900,11 +913,6 @@ export function createMcpHandlers(deps: CreateMcpHandlersDeps) { const senderRepoRoot = input.senderRepoRoot; const crossRepo = await isCrossRepoMessagingEnabled(pool); - if (!crossRepo && !senderRepoRoot) { - throw new Error( - "Cannot send messages: unable to determine your project's repository root." - ); - } const allAgents = await addressableAgents( await agentManager.listAgents(), @@ -991,11 +999,6 @@ export function createMcpHandlers(deps: CreateMcpHandlersDeps) { }> > { const crossRepo = await isCrossRepoMessagingEnabled(pool); - if (!crossRepo && !senderRepoRoot) { - throw new Error( - "Cannot list agents: unable to determine your project's repository root." - ); - } const agents = await addressableAgents( await agentManager.listAgents(), diff --git a/apps/server/test/mcp-handlers.test.ts b/apps/server/test/mcp-handlers.test.ts index ece19671..27d6cdce 100644 --- a/apps/server/test/mcp-handlers.test.ts +++ b/apps/server/test/mcp-handlers.test.ts @@ -1263,14 +1263,33 @@ describe("createMcpHandlers", () => { ).rejects.toThrow("Sender agent not found."); }); - it("throws when senderRepoRoot is null", async () => { + it("returns no match when senderRepoRoot is null and no parent/child", async () => { + deps.agentManager.listAgents.mockResolvedValue([ + { + id: "agt_test1", + name: "sender", + cwd: "/repo-a", + status: "running", + parentAgentId: null, + }, + { + id: "agt_other", + name: "other", + cwd: "/repo-b", + status: "running", + parentAgentId: null, + }, + ]); + vi.mocked(resolveRepoRoot).mockImplementation( + async (cwd) => cwd as string + ); await expect( handlers.sendMessage("agt_test1", { target: "agt_other", message: "hello", senderRepoRoot: null, }) - ).rejects.toThrow("Cannot send messages"); + ).rejects.toThrow('No agent found matching "agt_other"'); }); it("throws when target agent is not running", async () => { @@ -1364,6 +1383,93 @@ describe("createMcpHandlers", () => { ).rejects.toThrow('No agent found matching "other"'); }); + it("delivers to child agent in a different repo", async () => { + deps.agentManager.listAgents.mockResolvedValue([ + { + id: "agt_parent", + name: "parent", + cwd: "/repo-a", + status: "running", + parentAgentId: null, + }, + { + id: "agt_child", + name: "child", + cwd: "/repo-b", + status: "running", + parentAgentId: "agt_parent", + }, + ]); + vi.mocked(resolveRepoRoot).mockImplementation( + async (cwd) => cwd as string + ); + const result = await handlers.sendMessage("agt_parent", { + target: "agt_child", + message: "hi child", + senderRepoRoot: "/repo-a", + }); + expect(result.delivered).toBe(true); + expect(result.targetAgentId).toBe("agt_child"); + }); + + it("delivers to parent agent in a different repo", async () => { + deps.agentManager.listAgents.mockResolvedValue([ + { + id: "agt_parent", + name: "parent", + cwd: "/repo-a", + status: "running", + parentAgentId: null, + }, + { + id: "agt_child", + name: "child", + cwd: "/repo-b", + status: "running", + parentAgentId: "agt_parent", + }, + ]); + vi.mocked(resolveRepoRoot).mockImplementation( + async (cwd) => cwd as string + ); + const result = await handlers.sendMessage("agt_child", { + target: "agt_parent", + message: "hi parent", + senderRepoRoot: "/repo-b", + }); + expect(result.delivered).toBe(true); + expect(result.targetAgentId).toBe("agt_parent"); + }); + + it("delivers to child agent even when senderRepoRoot is null", async () => { + deps.agentManager.listAgents.mockResolvedValue([ + { + id: "agt_parent", + name: "parent", + cwd: "/not-a-repo", + status: "running", + parentAgentId: null, + }, + { + id: "agt_child", + name: "child", + cwd: "/repo-b", + status: "running", + parentAgentId: "agt_parent", + }, + ]); + vi.mocked(resolveRepoRoot).mockImplementation( + async (cwd) => cwd as string + ); + const result = await handlers.sendMessage("agt_parent", { + target: "agt_child", + message: "hi", + senderRepoRoot: null, + }); + expect(result.delivered).toBe(true); + expect(result.targetAgentId).toBe("agt_child"); + }); + it("delivers cross-repo when the cross-repo messaging setting is enabled", async () => { // getSetting(cross_repo_messaging_enabled) -> "true" deps.pool.query.mockResolvedValue({ rows: [{ value: "true" }] }); @@ -1429,10 +1535,156 @@ describe("createMcpHandlers", () => { }); }); - it("throws when senderRepoRoot is null", async () => { - await expect( - handlers.listAgentsForAgent("agt_self", null) - ).rejects.toThrow("Cannot list agents"); + it("returns empty list when senderRepoRoot is null and no parent/child", async () => { + deps.agentManager.listAgents.mockResolvedValue([ + { + id: "agt_self", + name: "self", + cwd: "/repo", + status: "running", + latestEvent: null, + }, + { + id: "agt_other", + name: "other", + cwd: "/other-repo", + status: "running", + latestEvent: null, + }, + ]); + vi.mocked(resolveRepoRoot).mockImplementation( + async (cwd) => cwd as string + ); + const result = await handlers.listAgentsForAgent("agt_self", null); + expect(result).toHaveLength(0); + }); + + it("includes direct child agent even in a different repo", async () => { + deps.agentManager.listAgents.mockResolvedValue([ + { + id: "agt_parent", + name: "parent", + cwd: "/repo-a", + status: "running", + latestEvent: null, + parentAgentId: null, + }, + { + id: "agt_child", + name: "child", + cwd: "/repo-b", + status: "running", + latestEvent: null, + parentAgentId: "agt_parent", + }, + { + id: "agt_unrelated", + name: "unrelated", + cwd: "/repo-b", + status: "running", + latestEvent: null, + parentAgentId: null, + }, + ]); + vi.mocked(resolveRepoRoot).mockImplementation( + async (cwd) => cwd as string + ); + + const result = await handlers.listAgentsForAgent("agt_parent", "/repo-a"); + expect(result.map((a) => a.id)).toEqual(["agt_child"]); + }); + + it("includes direct parent agent even in a different repo", async () => { + deps.agentManager.listAgents.mockResolvedValue([ + { + id: "agt_parent", + name: "parent", + cwd: "/repo-a", + status: "running", + latestEvent: null, + parentAgentId: null, + }, + { + id: "agt_child", + name: "child", + cwd: "/repo-b", + status: "running", + latestEvent: null, + parentAgentId: "agt_parent", + }, + ]); + vi.mocked(resolveRepoRoot).mockImplementation( + async (cwd) => cwd as string + ); + + const result = await handlers.listAgentsForAgent("agt_child", "/repo-b"); + expect(result.map((a) => a.id)).toEqual(["agt_parent"]); + }); + + it("does not include grandchild agents", async () => { + deps.agentManager.listAgents.mockResolvedValue([ + { + id: "agt_grandparent", + name: "grandparent", + cwd: "/repo-a", + status: "running", + latestEvent: null, + parentAgentId: null, + }, + { + id: "agt_parent", + name: "parent", + cwd: "/repo-a", + status: "running", + latestEvent: null, + parentAgentId: "agt_grandparent", + }, + { + id: "agt_grandchild", + name: "grandchild", + cwd: "/repo-b", + status: "running", + latestEvent: null, + parentAgentId: "agt_parent", + }, + ]); + vi.mocked(resolveRepoRoot).mockImplementation( + async (cwd) => cwd as string + ); + + const result = await handlers.listAgentsForAgent( + "agt_grandparent", + "/repo-a" + ); + // Only direct child visible, not grandchild in different repo + expect(result.map((a) => a.id)).toEqual(["agt_parent"]); + }); + + it("includes child agent even when senderRepoRoot is null", async () => { + deps.agentManager.listAgents.mockResolvedValue([ + { + id: "agt_parent", + name: "parent", + cwd: "/not-a-repo", + status: "running", + latestEvent: null, + parentAgentId: null, + }, + { + id: "agt_child", + name: "child", + cwd: "/repo-b", + status: "running", + latestEvent: null, + parentAgentId: "agt_parent", + }, + ]); + vi.mocked(resolveRepoRoot).mockImplementation( + async (cwd) => cwd as string + ); + + const result = await handlers.listAgentsForAgent("agt_parent", null); + expect(result.map((a) => a.id)).toEqual(["agt_child"]); }); it("lists agents across repos when the cross-repo messaging setting is enabled", async () => {