From 1e731520bb3a18ee72049374ba5d13206c60b478 Mon Sep 17 00:00:00 2001 From: Dotta <34892728+cryppadotta@users.noreply.github.com> Date: Wed, 14 Oct 2026 07:39:54 -0500 Subject: [PATCH] fix(mcp): deduplicate discovered tool descriptors (#16281) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Agents receive governed app tools through the MCP gateway. > - GitHub review tasks can contain a PR discussion and inline finding threads. > - Discovery repeated each tool for every conversation linked to the task. > - Native tool registration treated these repeated descriptors as a fatal collision. > - This pull request lists GitHub tools once and deduplicates identical descriptors across providers. > - Agents can continue work while conflicting definitions and normal tool permissions remain enforced. ## Linked Issues or Issue Description **What happened?** A follow-up run on a GitHub review task failed with `assigned_mcp_tool_name_collision` before the provider started. The task had a main conversation and an inline finding thread. Discovery returned the same seven tools twice. Repeated descriptors from other providers could cause the same failure. **Expected behavior** Discovery exposes each identical tool once. Separate connection names remain separate tools. Conflicting definitions remain an error. Every call still passes the gateway's policy, approval, revocation, and audit checks. **Steps to reproduce** 1. Run a GitHub review and publish an inline finding. 2. Send a follow-up request on the same PR task. 3. Inspect the task-bound bot catalog and the next native startup. 4. Separately, return two identical descriptors from an assigned app or plugin catalog. Related: #13717 added the GitHub review workflow. ## What Changed - Use a distinct GitHub discovery query across conversations that share an endpoint and task. Keep both thread mappings. - Deduplicate identical gateway descriptors by their full gateway name across providers. Use this rule for MCP listings, plugin listings, assigned-tool startup, and fresh assigned-tool discovery. - Reject conflicting descriptors with `tool_name_collision`. Keep the projected-name collision guard. - Cover GitHub inline-thread discovery, repeated plugin/app tools, direct and on-demand catalogs, revocation, conflicting bindings, and separate connection names. - Update the execution contract, GitHub documentation, and agent access map. ## Verification - Regression baseline: six new assertions failed on the original implementation. GitHub returned fourteen entries, plugin discovery repeated a tool, and native startup failed on repeated app tools. - Focused verification passed: 272 GitHub cases, all 29 assigned-tool cases, and all 88 gateway acceptance cases. - `pnpm -r typecheck` passed. - `pnpm build` passed. - `node scripts/check-maps.mjs` passed. - Current-head CI passed: 55 checks passed, two inapplicable checks skipped. Greptile scored the current head 5/5 with no unresolved threads. - Full local `pnpm test:run` encountered a 15-second timeout in the existing bearer-auth throttling test, before tool discovery. That test passed in the separate 88-case gateway run and on one isolated rerun. The duplicate broad local run was then stopped; the complete current-head CI matrix passed. ## Risks - Deduplication keeps the first identical descriptor and preserves listing order. Same-name descriptors with different definitions fail with HTTP 409 rather than selecting an ambiguous binding. - The GitHub query changes result cardinality. It retains the existing task/company/agent filters and leaves conversation rows intact. - No schema migration or credential change. Live staging recovery requires deployment and a new authorized run. ## Model Used OpenAI Codex, based on GPT-6, with code inspection, editing, browser investigation, and test execution. The exact serving model ID and context window are not exposed in this session. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip --- doc/connections/GITHUB.md | 6 +++ doc/execution-semantics.md | 7 ++++ maps/agent-access-map/gateways-profiles.md | 8 ++++ .../chat-channels.integration.test.ts | 14 +++++++ server/src/__tests__/tool-gateway.test.ts | 12 +++--- server/src/services/chat-github-tools.ts | 4 +- .../native-runtime/assigned-mcp-tools.test.ts | 38 +++++++++++++++++++ .../native-runtime/assigned-mcp-tools.ts | 6 +-- server/src/services/tool-gateway.ts | 25 ++++++++++-- 9 files changed, 108 insertions(+), 12 deletions(-) diff --git a/doc/connections/GITHUB.md b/doc/connections/GITHUB.md index 1d2abe95ec5..a374f8616c8 100644 --- a/doc/connections/GITHUB.md +++ b/doc/connections/GITHUB.md @@ -243,6 +243,12 @@ an agent runtime can execute a review. ### One editable response per request +A pull request task can retain both its main discussion and inline finding +threads. Bot tool discovery lists each endpoint/catalog tool once across those +threads. Publishing an inline finding therefore does not duplicate the tool +catalog on the task's next run. Both conversation mappings remain available for +authorized follow-up replies. + After accepting authorized GitHub work, Paperclip posts **Working on this…** before starting the agent. Issue and PR discussion comments use the same GitHub comment API; inline-review conversations receive an inline reply. Description diff --git a/doc/execution-semantics.md b/doc/execution-semantics.md index daca73b1ba6..cc05e150bfc 100644 --- a/doc/execution-semantics.md +++ b/doc/execution-semantics.md @@ -1628,6 +1628,13 @@ URL, and token together through the launch allowlist; unrelated environment secrets remain excluded. This does not restrict arbitrary network access to a public service outside the gateway. +Tool discovery collapses identical descriptors with the same gateway name for +every provider. Native assigned-tool projection applies the same rule at startup +and during fresh discovery. A name with conflicting schemas, risk, or source +bindings remains an error; distinct names retain separate tools. Discovery does +not grant execution authority: calls still use the existing gateway policy, +approval, revocation, and audit paths. + ### Use real connection requests (2026-09-14) diff --git a/maps/agent-access-map/gateways-profiles.md b/maps/agent-access-map/gateways-profiles.md index 7ab04395a01..034ddeced1f 100644 --- a/maps/agent-access-map/gateways-profiles.md +++ b/maps/agent-access-map/gateways-profiles.md @@ -84,6 +84,14 @@ remaining operations before qualifying the whole capability. Gaps: Scoped sessions remain agent/run-bound. MCP requires its existing protocol client; named gateways and gateway-token management retain board permissions. +Discovery collapses identical descriptors with the same gateway name across +providers, including native assigned-tool startup and fresh searches. Conflicting +definitions remain an error. Distinct names and the normal invocation grants, +approvals and revocation checks remain independent. Regression coverage lives in +[gateway tests](../../server/src/__tests__/tool-gateway.test.ts) and +[assigned-tool tests](../../server/src/services/native-runtime/assigned-mcp-tools.test.ts); +these fixtures do not certify a live external-client journey. + ## `tokens` Review: `baseline`. diff --git a/server/src/__tests__/chat-channels.integration.test.ts b/server/src/__tests__/chat-channels.integration.test.ts index 901ef5c54ac..fea8b7f8f6a 100644 --- a/server/src/__tests__/chat-channels.integration.test.ts +++ b/server/src/__tests__/chat-channels.integration.test.ts @@ -2555,6 +2555,20 @@ describeEmbeddedPostgres("chat channel control-plane integration", () => { const session = { companyId: f.companyId, agentId: f.assignedAgentId, issueId: conversation.issueId, runId: run.id }; return { ...f, conversation, run, session }; } + it("lists each GitHub bot tool once when an inline thread shares the task", async () => { + const f = await toolReplyFixture("github:paperclipai/paperclip:418"); + const { id: _id, ...conversation } = f.conversation; + await db.insert(chatConversations).values({ + ...conversation, + externalThreadId: "github:paperclipai/paperclip:418:rc:41901", + providerUrl: "https://github.com/paperclipai/paperclip/pull/418#discussion_r41901", + }); + const tools = await githubBotToolsForSession(db, f.session); + expect(tools).toHaveLength(7); + expect(new Set(tools.map(tool => tool.name)).size).toBe(7); + expect(await githubBotToolsForSession(db, { ...f.session, issueId: randomUUID() })).toEqual([]); + expect(await readChatConversationsByEndpointId(f.endpoint.id)).toHaveLength(2); + }); it.each([ { label: "returns the confirmed GitHub assessment when publication takes longer than ten seconds", delayMs: 11_000, timeoutMs: undefined }, { label: "preserves an explicit shorter GitHub tool deadline and reconciles without duplicate publication", delayMs: 200, timeoutMs: 50 }, diff --git a/server/src/__tests__/tool-gateway.test.ts b/server/src/__tests__/tool-gateway.test.ts index bc5604c0423..af1f05b45f9 100644 --- a/server/src/__tests__/tool-gateway.test.ts +++ b/server/src/__tests__/tool-gateway.test.ts @@ -5856,7 +5856,7 @@ rl.on("line", (line) => { }); }); - it("wraps plugin tool discovery and execution behind the same gateway policy", async () => { + it.each([1, 2])("wraps plugin tool discovery and execution behind the same gateway policy (%i copies)", async copies => { const company = await createCompany(db); const agent = await createAgent(db, company.id); const { run, project } = await createIssueAndRun(db, company.id, agent.id); @@ -5864,15 +5864,13 @@ rl.on("line", (line) => { const dispatcher: PluginToolDispatcher = { initialize: async () => {}, teardown: () => {}, - listToolsForAgent: () => [ - { + listToolsForAgent: () => Array.from({ length: copies }, () => ({ name: "demo-plugin:read_status", displayName: "Read status", description: "Read status through a plugin tool.", parametersSchema: { type: "object" }, pluginId: "demo-plugin", - }, - ], + })), getTool: () => null, executeTool: async (tool, parameters, runContext) => { calls.push({ tool, parameters, runContext }); @@ -5906,6 +5904,10 @@ rl.on("line", (line) => { await allowToolsForAgent(db, company.id, agent.id, ["demo-plugin:read_status"]); + const session = await gateway.createSession({ companyId: company.id, agentId: agent.id, runId: run.id, projectId: project.id }); + expect((await gateway.listToolsForSession(session.token)).filter(tool => tool.pluginId === "demo-plugin")) + .toEqual([expect.objectContaining({ name: "demo-plugin:read_status" })]); + await expect(gateway.listPluginToolsForAgent({ companyId: company.id, agentId: agent.id })).resolves.toEqual([ expect.objectContaining({ name: "demo-plugin:read_status" }), ]); diff --git a/server/src/services/chat-github-tools.ts b/server/src/services/chat-github-tools.ts index 4dad738fc3f..e958c9ed83f 100644 --- a/server/src/services/chat-github-tools.ts +++ b/server/src/services/chat-github-tools.ts @@ -241,7 +241,9 @@ export async function githubBotToolsForSession( ): Promise { if (!session.agentId || !session.issueId || !session.runId) return []; const rows = await db - .select({ + // A PR task can also own inline finding threads. They share one endpoint + // and catalog, so discovery must not emit a tool once per conversation. + .selectDistinct({ endpoint: chatEndpoints, connection: toolConnections, entry: toolCatalogEntries, diff --git a/server/src/services/native-runtime/assigned-mcp-tools.test.ts b/server/src/services/native-runtime/assigned-mcp-tools.test.ts index 4ea146181a0..cd57e37c5dd 100644 --- a/server/src/services/native-runtime/assigned-mcp-tools.test.ts +++ b/server/src/services/native-runtime/assigned-mcp-tools.test.ts @@ -18,6 +18,44 @@ describe("assigned MCP runner tools", () => { const searchName = "paperclip_search_assigned_tools"; const callName = "paperclip_call_assigned_tool"; + it("deduplicates repeated app tools in direct and on-demand catalogs while retaining revocation", async () => { + const read = descriptor("memory.search"); + const write = descriptor("memory.store", "write"); + const f = fixture([read, structuredClone(read), write, structuredClone(write)]); + const assigned = await createAssignedMcpTools(f); + const direct = assigned.definitions(); + expect(direct).toHaveLength(2); + expect(assigned.definitions(tools => tools.length <= 2)).toEqual(direct); + expect(assigned.definitions(tools => tools[0]?.name === searchName).map(tool => tool.name)).toEqual([searchName, callName]); + await expect(assigned.execute({ tool: searchName, arguments: { query: "" } }, "planning")) + .resolves.toEqual({ tools: [direct[0]], nextOffset: null }); + await assigned.execute({ tool: callName, arguments: { name: direct[0]!.name, arguments: { query: "fixture" } } }); + expect(f.executeTool).toHaveBeenCalledExactlyOnceWith(expect.objectContaining({ tool: read.name })); + f.listToolsForNamedGateway.mockResolvedValue([write, structuredClone(write)]); + await expect(assigned.execute({ tool: searchName, arguments: { query: "" } })) + .resolves.toEqual({ tools: [direct[1]], nextOffset: null }); + }); + + it.each([ + { risk: "write" as const }, + { connectionId: "another-connection" }, + { parametersSchema: { type: "object", properties: { secret: { type: "string" } } } }, + ])("rejects conflicting definitions with the same gateway name: %j", async change => { + const tool = descriptor("memory.search"); + const f = fixture([tool, { ...tool, ...change }]); + await expect(createAssignedMcpTools(f)).rejects.toMatchObject({ reasonCode: "tool_name_collision" }); + expect(f.executeTool).not.toHaveBeenCalled(); + }); + + it("keeps same-action tools from distinct connections separately callable", async () => { + const f = fixture([descriptor("mcp.memory-first:search"), descriptor("mcp.memory-second:search")]); + const assigned = await createAssignedMcpTools(f); + const direct = assigned.definitions(); + expect(new Set(direct.map(tool => tool.name)).size).toBe(2); + for (const tool of direct) await assigned.execute({ tool: tool.name as string, arguments: {} }); + expect(f.executeTool.mock.calls.map(([call]) => call.tool)).toEqual(["mcp.memory-first:search", "mcp.memory-second:search"]); + }); + it("pages through an oversized catalog and calls every tool through the original gateway", async () => { const f = fixture(Array.from({ length: 224 }, (_, i) => descriptor(`app.action_${i}`))); const assigned = await createAssignedMcpTools(f); diff --git a/server/src/services/native-runtime/assigned-mcp-tools.ts b/server/src/services/native-runtime/assigned-mcp-tools.ts index d8817f0675f..81577464734 100644 --- a/server/src/services/native-runtime/assigned-mcp-tools.ts +++ b/server/src/services/native-runtime/assigned-mcp-tools.ts @@ -1,6 +1,6 @@ import { createHash } from "node:crypto"; import type { Db } from "@paperclipai/db"; -import { ToolGatewayHttpError, type ToolGatewayDescriptor, type ToolGatewayService } from "../tool-gateway.js"; +import { deduplicateToolDescriptors, ToolGatewayHttpError, type ToolGatewayDescriptor, type ToolGatewayService } from "../tool-gateway.js"; type WorkMode = "standard" | "planning" | "ask"; type ToolDefinition = Record; @@ -92,7 +92,7 @@ export async function createAssignedMcpTools(input: { bearerToken: input.bearerToken, }); const tools = new Map(); - for (const descriptor of listed) { + for (const descriptor of deduplicateToolDescriptors(listed)) { const name = assignedToolName(descriptor.name); if (tools.has(name)) throw new Error("assigned_mcp_tool_name_collision"); tools.set(name, descriptor); @@ -114,7 +114,7 @@ export async function createAssignedMcpTools(input: { } // Intersect fresh grants with this session's pinned catalog. Revocations // take effect immediately; newly assigned tools require a new session. - const current = new Map((await input.gateway.listToolsForNamedGateway({ + const current = new Map(deduplicateToolDescriptors(await input.gateway.listToolsForNamedGateway({ gatewayPublicId: input.gatewayPublicId, bearerToken: input.bearerToken, })).map(tool => [tool.name, tool])); const terms = args.query.toLowerCase().trim().split(/\s+/).filter(Boolean); diff --git a/server/src/services/tool-gateway.ts b/server/src/services/tool-gateway.ts index b6382677fe0..902d5e0d294 100644 --- a/server/src/services/tool-gateway.ts +++ b/server/src/services/tool-gateway.ts @@ -22,6 +22,7 @@ import { extractRemoteMcpPending } from "./remote-mcp-pending.js"; import { logger } from "../middleware/logger.js"; import { spawn } from "node:child_process"; import { createHash, randomBytes, randomUUID } from "node:crypto"; +import { isDeepStrictEqual } from "node:util"; import { and, asc, @@ -354,6 +355,22 @@ export class ToolGatewayHttpError extends Error { } } +/** Repeated discovery rows are one tool; conflicting bindings stay ambiguous. */ +export function deduplicateToolDescriptors(tools: readonly ToolGatewayDescriptor[]): ToolGatewayDescriptor[] { + const unique = new Map(); + for (const tool of tools) { + const previous = unique.get(tool.name); + if (previous) { + if (!isDeepStrictEqual(previous, tool)) { + throw new ToolGatewayHttpError(409, "Conflicting tool definitions share the same name.", "tool_name_collision"); + } + continue; + } + unique.set(tool.name, tool); + } + return [...unique.values()]; +} + interface ExecuteGatewayToolInput { sessionToken: string; gatewayId?: string | null; @@ -3084,7 +3101,7 @@ export function createToolGatewayService( visibleTools.push(...VIRTUAL_TOOLS); } } - return visibleTools; + return deduplicateToolDescriptors(visibleTools); } async function executeBuiltinTool( @@ -9036,12 +9053,14 @@ export function createToolGatewayService( tool, }), ); - return decisions + const visibleTools = decisions .filter( ({ decision }) => decision.allowed || decision.decision === "require_approval", ) - .map(({ tool }) => { + .map(({ tool }) => tool); + return deduplicateToolDescriptors(visibleTools) + .map((tool) => { const { providerType: _providerType, risk: _risk,