Skip to content

Commit cf5bff5

Browse files
richardsolomourichardsolomou
authored
fix(local-mcp): test and dedupe relay permission gating
getMcpToolApprovalState's always-ask branch and the permission handler's relay-or-deny branch for relayed-server tools had no coverage — a regression letting a relayed tool auto-approve without a reachable client would have passed the whole suite. Added tests for both: desktop-connected relays, durable-stream-only counts as reachable, background mode denies, no reachable client denies rather than auto-approves, non-relayed servers are unaffected. Extracted the reachability check (session.hasDesktopConnected or an active event stream) into one hasReachableClient() method — it was duplicated three times with the same explanatory comment.
1 parent 24cdc6d commit cf5bff5

3 files changed

Lines changed: 192 additions & 20 deletions

File tree

‎packages/agent/src/adapters/claude/mcp/tool-metadata.test.ts‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,12 +5,14 @@ import {
55
getMcpToolMetadata,
66
isMcpToolReadOnly,
77
sanitizeMcpServerName,
8+
setAlwaysAskMcpServers,
89
setMcpToolApprovalStates,
910
} from "./tool-metadata";
1011

1112
describe("tool-metadata approval states", () => {
1213
beforeEach(() => {
1314
clearMcpToolMetadataCache();
15+
setAlwaysAskMcpServers([]);
1416
});
1517

1618
describe("setMcpToolApprovalStates", () => {
@@ -64,6 +66,45 @@ describe("tool-metadata approval states", () => {
6466
});
6567
});
6668

69+
describe("setAlwaysAskMcpServers", () => {
70+
it("defaults tools on a relayed server to needs_approval", () => {
71+
setAlwaysAskMcpServers(["slack"]);
72+
73+
expect(getMcpToolApprovalState("mcp__slack__send_message")).toBe(
74+
"needs_approval",
75+
);
76+
});
77+
78+
it("leaves tools on other servers unaffected", () => {
79+
setAlwaysAskMcpServers(["slack"]);
80+
81+
expect(getMcpToolApprovalState("mcp__posthog__query")).toBeUndefined();
82+
});
83+
84+
it("lets a cached explicit approval state win over the always-ask default", () => {
85+
setAlwaysAskMcpServers(["slack"]);
86+
setMcpToolApprovalStates({
87+
mcp__slack__send_message: "approved",
88+
});
89+
90+
expect(getMcpToolApprovalState("mcp__slack__send_message")).toBe(
91+
"approved",
92+
);
93+
});
94+
95+
it("clears previously always-ask servers when called again", () => {
96+
setAlwaysAskMcpServers(["slack"]);
97+
setAlwaysAskMcpServers(["grafana"]);
98+
99+
expect(
100+
getMcpToolApprovalState("mcp__slack__send_message"),
101+
).toBeUndefined();
102+
expect(getMcpToolApprovalState("mcp__grafana__query")).toBe(
103+
"needs_approval",
104+
);
105+
});
106+
});
107+
67108
describe("isMcpToolReadOnly with approval states", () => {
68109
it("returns false for tools that only have approval state", () => {
69110
setMcpToolApprovalStates({

‎packages/agent/src/server/agent-server.test.ts‎

Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1290,6 +1290,143 @@ describe("AgentServer HTTP Mode", () => {
12901290
});
12911291
});
12921292

1293+
describe("relayed MCP server tool permissions", () => {
1294+
function exposeCloudClient(testServer: AgentServer) {
1295+
return testServer as unknown as {
1296+
config: { relayMcpServers?: string[]; mode?: string };
1297+
session: { hasDesktopConnected?: boolean } | null;
1298+
eventStreamSender: unknown;
1299+
relayPermissionToClient: (params: unknown) => Promise<unknown>;
1300+
createCloudClient(payload: {
1301+
run_id: string;
1302+
task_id: string;
1303+
team_id: number;
1304+
user_id: number;
1305+
distinct_id: string;
1306+
mode?: "interactive" | "background";
1307+
}): {
1308+
requestPermission(params: unknown): Promise<{
1309+
outcome: { outcome: string; optionId?: string };
1310+
_meta?: Record<string, unknown>;
1311+
}>;
1312+
};
1313+
};
1314+
}
1315+
1316+
function permissionRequestFor(mcpToolName: string) {
1317+
return {
1318+
options: [{ optionId: "allow_once", kind: "allow_once" }],
1319+
toolCall: {
1320+
kind: "other",
1321+
_meta: { claudeCode: { toolName: mcpToolName } },
1322+
rawInput: {},
1323+
},
1324+
};
1325+
}
1326+
1327+
const basePayload = {
1328+
run_id: "run-1",
1329+
task_id: "task-1",
1330+
team_id: 1,
1331+
user_id: 1,
1332+
distinct_id: "user-1",
1333+
};
1334+
1335+
it("relays a relayed-server tool call when the desktop is connected", async () => {
1336+
const testServer = exposeCloudClient(createServer());
1337+
testServer.config.relayMcpServers = ["slack"];
1338+
testServer.session = { hasDesktopConnected: true };
1339+
const relaySpy = vi
1340+
.spyOn(testServer, "relayPermissionToClient")
1341+
.mockResolvedValue({
1342+
outcome: { outcome: "selected", optionId: "allow_once" },
1343+
});
1344+
1345+
const { requestPermission } = testServer.createCloudClient(basePayload);
1346+
const result = await requestPermission(
1347+
permissionRequestFor("mcp__slack__send_message"),
1348+
);
1349+
1350+
expect(relaySpy).toHaveBeenCalledOnce();
1351+
expect(result).toEqual({
1352+
outcome: { outcome: "selected", optionId: "allow_once" },
1353+
});
1354+
});
1355+
1356+
it("relays when only the durable event stream is reachable (no desktop session)", async () => {
1357+
const testServer = exposeCloudClient(createServer());
1358+
testServer.config.relayMcpServers = ["slack"];
1359+
testServer.session = null;
1360+
testServer.eventStreamSender = {
1361+
enqueue: vi.fn(),
1362+
stop: vi.fn(async () => {}),
1363+
};
1364+
const relaySpy = vi
1365+
.spyOn(testServer, "relayPermissionToClient")
1366+
.mockResolvedValue({
1367+
outcome: { outcome: "selected", optionId: "allow_once" },
1368+
});
1369+
1370+
const { requestPermission } = testServer.createCloudClient(basePayload);
1371+
await requestPermission(permissionRequestFor("mcp__slack__send_message"));
1372+
1373+
expect(relaySpy).toHaveBeenCalledOnce();
1374+
});
1375+
1376+
it("denies a relayed-server tool call instead of auto-approving when no client is reachable", async () => {
1377+
const testServer = exposeCloudClient(createServer());
1378+
testServer.config.relayMcpServers = ["slack"];
1379+
testServer.session = null;
1380+
testServer.eventStreamSender = null;
1381+
const relaySpy = vi.spyOn(testServer, "relayPermissionToClient");
1382+
1383+
const { requestPermission } = testServer.createCloudClient(basePayload);
1384+
const result = await requestPermission(
1385+
permissionRequestFor("mcp__slack__send_message"),
1386+
);
1387+
1388+
expect(relaySpy).not.toHaveBeenCalled();
1389+
expect(result.outcome).toEqual({ outcome: "cancelled" });
1390+
});
1391+
1392+
it("denies a relayed-server tool call in background mode even when a client is reachable", async () => {
1393+
const testServer = exposeCloudClient(createServer());
1394+
testServer.config.relayMcpServers = ["slack"];
1395+
testServer.session = { hasDesktopConnected: true };
1396+
const relaySpy = vi.spyOn(testServer, "relayPermissionToClient");
1397+
1398+
const { requestPermission } = testServer.createCloudClient({
1399+
...basePayload,
1400+
mode: "background",
1401+
});
1402+
const result = await requestPermission(
1403+
permissionRequestFor("mcp__slack__send_message"),
1404+
);
1405+
1406+
expect(relaySpy).not.toHaveBeenCalled();
1407+
expect(result.outcome).toEqual({ outcome: "cancelled" });
1408+
});
1409+
1410+
it("does not treat a tool on a non-relayed server as always-ask", async () => {
1411+
const testServer = exposeCloudClient(createServer());
1412+
testServer.config.relayMcpServers = ["slack"];
1413+
testServer.session = null;
1414+
testServer.eventStreamSender = null;
1415+
const relaySpy = vi.spyOn(testServer, "relayPermissionToClient");
1416+
1417+
const { requestPermission } = testServer.createCloudClient(basePayload);
1418+
const result = await requestPermission(
1419+
permissionRequestFor("mcp__posthog__query"),
1420+
);
1421+
1422+
expect(relaySpy).not.toHaveBeenCalled();
1423+
expect(result.outcome).toEqual({
1424+
outcome: "selected",
1425+
optionId: "allow_once",
1426+
});
1427+
});
1428+
});
1429+
12931430
describe("GET /events", () => {
12941431
it("returns 401 without authorization header", async () => {
12951432
await createServer().start();

‎packages/agent/src/server/agent-server.ts‎

Lines changed: 14 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -399,13 +399,7 @@ export class AgentServer {
399399
this.mcpRelayServer = new McpRelayServer({
400400
servers: names,
401401
emitEvent: (event) => this.broadcastEvent(event),
402-
// Same reachability signal as the permission relay: a direct SSE
403-
// viewer or an active durable event stream. The desktop reads the
404-
// durable stream through the agent-proxy without connecting to the
405-
// sandbox, so anything stricter would 503 every request.
406-
hasReachableClient: () =>
407-
Boolean(this.session?.hasDesktopConnected) ||
408-
this.eventStreamSender !== null,
402+
hasReachableClient: () => this.hasReachableClient(),
409403
logger: this.logger,
410404
});
411405
await this.mcpRelayServer.start();
@@ -491,6 +485,17 @@ export class AgentServer {
491485
return this.getRuntimeAdapter() === "codex" ? "auto" : "default";
492486
}
493487

488+
// A direct SSE viewer or an active durable event stream both count: the
489+
// desktop reads the durable stream through the agent-proxy without ever
490+
// connecting to the sandbox, so requiring hasDesktopConnected alone would
491+
// 503/auto-deny every relayed request and permission prompt.
492+
private hasReachableClient(): boolean {
493+
return (
494+
Boolean(this.session?.hasDesktopConnected) ||
495+
this.eventStreamSender !== null
496+
);
497+
}
498+
494499
private shouldRelayPermissionToClient(mode: PermissionMode): boolean {
495500
// "plan" relays like "read-only" (look-don't-touch): escalations need a human
496501
// veto, not silent auto-approval.
@@ -3650,10 +3655,7 @@ ${signedCommitInstructions}${prLinkInstructions}${shellEfficiencyInstructions}
36503655
mcpServerName &&
36513656
(this.config.relayMcpServers ?? []).includes(mcpServerName)
36523657
) {
3653-
const hasReachableClient =
3654-
Boolean(this.session?.hasDesktopConnected) ||
3655-
this.eventStreamSender !== null;
3656-
if (mode !== "background" && hasReachableClient) {
3658+
if (mode !== "background" && this.hasReachableClient()) {
36573659
return this.relayPermissionToClient(params);
36583660
}
36593661
return {
@@ -3683,15 +3685,7 @@ ${signedCommitInstructions}${prLinkInstructions}${shellEfficiencyInstructions}
36833685
sessionPermissionMode,
36843686
);
36853687

3686-
// With durable event ingest nothing connects to GET /events, so
3687-
// hasDesktopConnected stays false even while the web/desktop task
3688-
// views follow the run through the agent-proxy stream. Those views
3689-
// render permission_request frames and answer via
3690-
// permission_response, so an active event stream counts as a
3691-
// reachable client for questions.
3692-
const hasReachableClient =
3693-
Boolean(this.session?.hasDesktopConnected) ||
3694-
this.eventStreamSender !== null;
3688+
const hasReachableClient = this.hasReachableClient();
36953689

36963690
// A background run has no human to answer a relayed approval
36973691
// (hasDesktopConnected is true from the event-relay reader), so

0 commit comments

Comments
 (0)