mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-21 08:22:20 +02:00
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 <noreply@paperclip.ing>
This commit is contained in:
1 parent
e9b6c4d44c
commit
1e731520bb
9 files changed
+108
-12
No files matched your search
@@ -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
|
||||
|
||||
@@ -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)
|
||||
|
||||
|
||||
@@ -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`.
|
||||
|
||||
@@ -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 },
|
||||
|
||||
@@ -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" }),
|
||||
]);
|
||||
|
||||
@@ -241,7 +241,9 @@ export async function githubBotToolsForSession(
|
||||
): Promise<ToolGatewayDescriptor[]> {
|
||||
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,
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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<string, unknown>;
|
||||
@@ -92,7 +92,7 @@ export async function createAssignedMcpTools(input: {
|
||||
bearerToken: input.bearerToken,
|
||||
});
|
||||
const tools = new Map<string, ToolGatewayDescriptor>();
|
||||
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);
|
||||
|
||||
@@ -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<string, ToolGatewayDescriptor>();
|
||||
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,
|
||||
|
||||
Reference in new issue
Block a user