From db64a525747162c22d7b129b0492159722ab565e Mon Sep 17 00:00:00 2001 From: Humphrey-2407 Date: Sat, 17 Oct 2026 02:42:02 -0400 Subject: [PATCH 1/2] fix(mcpl): refuse toolPrefixes that meet, so one tool name has one server An MCPL tool reaches the model as `--`, and a prefix may itself contain `--`. So two servers could give one name: `foo` listing `bar--echo` and `foo--bar` listing `echo` both gave `foo--bar--echo`. The model's tool list then carried the name twice. Membrane's native formatter throws on such a list ("Native tool names collide after colon escaping"), and Anthropic's API answers 400 ("tools: Tool names must be unique."), so every inference of an agent offered the name failed. In XML tool mode the prompt carried both definitions, and calls reached whichever server listed the name last. Two prefixes give one name only when they meet: they're the same, one is the other's followed by `-`, or one begins with the other's followed by `--`. prefixesMeet (src/mcpl/prefix-rule.ts) says exactly that, and its test checks it against a search over every pair of short prefixes. create() already refused two servers with one prefix; it now refuses two whose prefixes meet, and so do a runtime connect and a restart, the restart before it disconnects anything. The error names both servers, both prefixes and the rule. With no two prefixes meeting, a name fits at most one server's prefix, so the listing, the longest prefix and the first configured server always agree. The runtime refusal (refuseMcplConnect) reads the prefix map, so three gaps that let the map disagree with the configured servers are closed with it. A connect of an id already registered wrote its config and prefix over the live server's before the registry refused it; it's now refused first, and so is a restart into another live server's id. A connect under a new prefix, for an id whose earlier connect had failed, wrote its entry beside the one that failed connect left, which then outlived the id; a connect now drops its id's other entries first. And a disconnect of an id with no config deleted the entry under that id's default prefix, which another server may hold; it now deletes only its own. A restart doesn't count the prefix of the server it replaces, since it disconnects that one first, whatever id the new config carries. A server can still list one tool twice. Its first listing is offered, with its class and feature set, and the rest are left out. Each tool refresh that finds it raises an `mcpl-tool-duplicate` ops alert naming the tools, which says that's for the server's maintainer to change. Pairs that meet but whose servers never listed meeting names ran before and are refused now, so this is a breaking change for those configurations, and the changelog says what to change. Module names are still compared with server prefixes by equality: a module's tool could meet a server's only through a module tool name containing `--` or starting with `-`. The comments, the resolver's doc and the earlier resolver fragment no longer describe prefixes that meet as a live configuration, and the tests that build such a pair directly say why. The resolution test that configured `mcpl--plain` beside `mcpl--plain--inner` at create() now uses `inner`, keeping a default prefix and a listed name with `--` in it. A connect reads the server's restored host-managed trees before it adds the server, and on main it skipped that read for a server already registered. refuseMcplConnect has refused such a server by then, so the read needs no check of its own. A restart's comment said a colliding toolPrefix was refused only by the connect after the disconnect; the restart's refuseMcplConnect refuses it before. See #265. Co-Authored-By: Claude Opus 5.5 --- README.md | 11 + .../265-prefixes-that-meet.breaking.md | 12 + changelog.d/265-repeated-tool-name.fixed.md | 6 + changelog.d/one-mcpl-resolver.fixed.md | 6 +- src/framework.ts | 130 +++++-- src/mcpl/prefix-rule.ts | 32 ++ src/mcpl/types.ts | 5 +- test/mcpl-prefix-rule.test.ts | 345 ++++++++++++++++++ test/mcpl-prefixes-meet.test.ts | 32 ++ test/mcpl-tool-class-listing.test.ts | 2 + test/mcpl-tool-resolution.test.ts | 12 +- 11 files changed, 551 insertions(+), 42 deletions(-) create mode 100644 changelog.d/265-prefixes-that-meet.breaking.md create mode 100644 changelog.d/265-repeated-tool-name.fixed.md create mode 100644 src/mcpl/prefix-rule.ts create mode 100644 test/mcpl-prefix-rule.test.ts create mode 100644 test/mcpl-prefixes-meet.test.ts diff --git a/README.md b/README.md index 9775fda2d..25cf60b79 100644 --- a/README.md +++ b/README.md @@ -255,6 +255,17 @@ defaults to `mcpl--`: server `search` with no `toolPrefix` offers `mcpl--search--query`, and with `toolPrefix: 'search'` it offers `search--query`. +No two servers' prefixes may meet, since their servers could then give one +tool name. Two prefixes meet when one is the other's followed by `-`, or +begins with the other's followed by `--`: server `foo` listing `bar--echo` and +server `foo--bar` listing `echo` would both give `foo--bar--echo`. `create()` +refuses such a pair, and so do a runtime connect and a restart (before it +disconnects anything), naming both servers and the rule. `foo` beside +`foo-bar` or `foobar` is fine. A server that lists one tool more than once has +its first listing offered to the model and the rest left out, and each tool +refresh that finds it raises an `mcpl-tool-duplicate` ops alert naming the +tools. Its webhook post goes out at most once per server in 15 minutes. + These settings take RFC-007 §6.2 patterns (`*` matches any run of characters) over that full model-facing name, so they need the prefix: diff --git a/changelog.d/265-prefixes-that-meet.breaking.md b/changelog.d/265-prefixes-that-meet.breaking.md new file mode 100644 index 000000000..5afe9f1d1 --- /dev/null +++ b/changelog.d/265-prefixes-that-meet.breaking.md @@ -0,0 +1,12 @@ +- **Operators:** two MCPL servers whose `toolPrefix`es meet are refused, at + `create()`, at a runtime connect and at a restart (before it disconnects + anything), with an error naming both servers, both prefixes and the rule + (#265). Two prefixes meet when one is the other's followed by `-`, or + begins with the other's followed by `--`, as `foo` and `foo--bar` do. Their + servers could give one tool name: `foo` listing `bar--echo` and `foo--bar` + listing `echo` both gave `foo--bar--echo`. Such a name reached the model + twice, which membrane's native formatter and Anthropic's API refuse, so + every inference of an agent offered it failed, and in XML tool mode its + calls reached whichever server listed it last. A configuration with such a + pair needs one of the two prefixes changed, such as `foo-bar` for + `foo--bar`. `foo` beside `foo-bar` or `foobar` is fine. diff --git a/changelog.d/265-repeated-tool-name.fixed.md b/changelog.d/265-repeated-tool-name.fixed.md new file mode 100644 index 000000000..c56d5525e --- /dev/null +++ b/changelog.d/265-repeated-tool-name.fixed.md @@ -0,0 +1,6 @@ +- A tool name an MCPL server lists more than once reaches the model once: + the server's first listing is offered and the rest are left out (#265). + The model's tool list carried the name twice, which membrane's native + formatter and Anthropic's API refuse. Each tool refresh that finds such a + listing raises an `mcpl-tool-duplicate` ops alert naming the tools; its + webhook post goes out at most once per server in 15 minutes. diff --git a/changelog.d/one-mcpl-resolver.fixed.md b/changelog.d/one-mcpl-resolver.fixed.md index 521f2581f..6e5ee4f66 100644 --- a/changelog.d/one-mcpl-resolver.fixed.md +++ b/changelog.d/one-mcpl-resolver.fixed.md @@ -4,9 +4,7 @@ offers the name, else the longest matching prefix. It took the first configured server whose prefix the name began with, so where one prefix begins with another's, such as `foo` beside `foo--bar` or beside `foo-`, it - reached whichever of the two was configured first (#227, #230). Where two - servers list the same full name, the listing holds the one listed last - (#265), and both paths reach that one. + reached whichever of the two was configured first (#227, #230). - An MCPL tool's source in the tool presentation now names the server a call of it reaches, by the same resolver. **Operators:** a tool's source was the first configured server whose prefix its name began with, so with `foo` @@ -17,3 +15,5 @@ - A server's close forgets only its own tools' RFC-008 class declarations. Closing `foo` also forgot `foo--bar`'s, leaving its tools unclassed until the next tool refresh. +- Since #265, prefixes that meet, as `foo` does `foo--bar` and `foo-`, are + refused at configuration, so the cases above no longer start. diff --git a/src/framework.ts b/src/framework.ts index 7fe1bdda8..e59a6dc0d 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -77,6 +77,7 @@ import { REFUSAL_REACTIONS, REFUSAL_REACTION_FALLBACK, REFUSAL_REACTION_BASELINE import { Agent } from './agent.js'; import { ModuleRegistry, checkedToolResult, isStateExistsError, storableToolResult, thrownResult } from './module-registry.js'; import { McplServerRegistry } from './mcpl/server-registry.js'; +import { prefixesMeet, prefixesMeetError } from './mcpl/prefix-rule.js'; import { logValue } from './mcpl/log-value.js'; import { FeatureSetManager } from './mcpl/feature-set-manager.js'; import { computeGrant, CapabilityGrant, expandAdvertisementShorthand } from './mcpl/capability-grant.js'; @@ -2172,8 +2173,8 @@ export class AgentFramework { private mcplToolRefreshPending = false; /** Maps tool prefix → serverId for dispatch routing. */ private mcplPrefixMap: Map = new Map(); - /** Namespaced tool name → the server whose tools/list produced it. Prefixes - * can nest (`foo`, `foo--bar`), so a prefix match alone can name two. */ + /** Namespaced tool name → the server whose tools/list produced it, from + * its first listing of the name. resolveMcplTool reads it first. */ private mcplToolServers: Map = new Map(); /** Servers whose tools/list answered in the latest refresh. */ private mcplListedServers: Set = new Set(); @@ -2735,6 +2736,10 @@ export class AgentFramework { `MCPL server "${serverConfig.id}" toolPrefix "${prefix}" collides with server "${existing}"` ); } + // Nor may two prefixes meet, giving two servers one tool name (#265). + for (const [other, otherId] of prefixesSeen) { + if (prefixesMeet(prefix, other)) throw prefixesMeetError(serverConfig.id, prefix, otherId, other); + } prefixesSeen.set(prefix, serverConfig.id); } @@ -4710,8 +4715,8 @@ export class AgentFramework { for (const tool of this.moduleRegistry.getAllTools()) { sources.set(tool.name, `Module: ${tool.name.split('--')[0]}`); } - // An MCPL tool's source is the server a call of it reaches (resolveMcplTool), - // not the first configured prefix it begins with: prefixes can nest. + // An MCPL tool's source is the server a call of it reaches, by the one + // rule that picks a tool's server (resolveMcplTool). for (const tool of tools) { const mcpl = this.resolveMcplTool(tool.name); if (mcpl) sources.set(tool.name, `MCPL server: ${mcpl[0]}`); @@ -16135,8 +16140,7 @@ export class AgentFramework { // Tolerate partially-constructed frameworks (tests build them with // Object.create): nothing cached means nothing to forget. if (!this.mcplToolClasses?.size) return; - // The names the listing gives this server: its prefix alone would also - // take a nested prefix's (`foo` beside `foo--bar`). + // The names the listing gives this server, as resolveMcplTool reads them. for (const [name, owner] of this.mcplToolServers ?? []) { if (owner === serverId) this.mcplToolClasses.delete(name); } @@ -16202,13 +16206,14 @@ export class AgentFramework { /** * Find the MCPL server for a tool: [serverId, prefix], or null. - * Prefixes can nest (`foo` and `foo--bar`), so `foo--bar--x` can match - * both. The tool listing decides: the server whose tools/list offered the + * The tool listing decides: the server whose tools/list offered the * name, after its tool policy, as of the last tool refresh (a server that - * closed since keeps its names until the next one). Where two servers list - * the same name, the listing holds the one listed last (#265). A name the - * listing doesn't hold (invented by the model, or no longer listed) goes to - * the longest matching prefix. Never prefix-map insertion order. + * closed since keeps its names until the next one). A name the listing + * doesn't hold (invented by the model, or no longer listed) goes to the + * longest matching prefix. Prefixes that meet, such as `foo` and + * `foo--bar`, are refused at configuration (prefixesMeet, #265), so a name + * fits at most one configured server's prefix; this rule doesn't rely on + * that. Never prefix-map insertion order. */ private resolveMcplTool(toolName: string): [string, string] | null { const listedBy = this.mcplToolServers?.get(toolName); @@ -18119,8 +18124,13 @@ export class AgentFramework { this.getPlacedReactionBaseline(); // Register prefix + config for tool dispatch routing (idempotent with - // the pre-registration pass in initializeMcpl). + // the pre-registration pass in initializeMcpl). One config gives an id + // one prefix: an entry left from a failed connect under another prefix + // goes, or the runtime refusal would keep reading it. const prefix = config.toolPrefix ?? `mcpl--${config.id}`; + for (const [other, owner] of this.mcplPrefixMap) { + if (owner === config.id && other !== prefix) this.mcplPrefixMap.delete(other); + } this.mcplPrefixMap.set(prefix, config.id); this.mcplServerConfigs.set(config.id, config); @@ -18673,8 +18683,39 @@ export class AgentFramework { await this.initializeMcpl([], this.mcplInferenceRoutingConfig ?? undefined); } - // Same collision rules create() enforces at startup: the tool prefix must - // not shadow a module name or another server's prefix. + this.refuseMcplConnect(config); + + // The server's restored host-managed trees are read before the server is + // added, so a read that fails leaves it unconnected; MCPL itself stays + // up, even when this connect brought it up. A feature set with no + // restored tree, as after a disconnect removed it, is read when it + // registers. A server already registered was refused just above, so + // the read never reaches a connected server's trees. + this.checkpointManager?.loadHostStates(config.id); + const oldToolNames = new Set(this.mcplTools.map(t => t.name)); + await this.connectMcplServerInternal(config); + await this.refreshMcplTools(); + this.emitMcplToolDiff(oldToolNames, config.id); + } + + /** + * Refuse a runtime connect, or a restart, before it writes anything: an id + * already registered, or a tool prefix that equals a module name or another + * server's prefix, or meets another server's (prefixesMeet, #265). create() + * applies the same prefix rules at startup. The connecting id's own entries + * don't count, nor, for a restart, those of the server it replaces + * (`replacing`), which it disconnects first. + */ + private refuseMcplConnect( + config: import('./mcpl/types.js').McplServerConfig, + replacing?: string, + ): void { + // The registry refuses a second server under one id too, but only once + // the connect has written its config and prefix over the live server's. + if (config.id !== replacing && this.mcplServerRegistry?.getServer(config.id)) { + throw new Error(`MCPL server "${config.id}" is already registered`); + } + const own = (id: string) => id === config.id || id === replacing; const prefix = config.toolPrefix ?? `mcpl--${config.id}`; if (this.moduleRegistry.getAllModules().some(m => m.name === prefix)) { throw new Error( @@ -18682,23 +18723,14 @@ export class AgentFramework { ); } const prefixOwner = this.mcplPrefixMap.get(prefix); - if (prefixOwner && prefixOwner !== config.id) { + if (prefixOwner && !own(prefixOwner)) { throw new Error( `MCPL server "${config.id}" toolPrefix "${prefix}" collides with server "${prefixOwner}"`, ); } - - // The server's restored host-managed trees are read before the server is - // added, so a read that fails leaves it unconnected; MCPL itself stays - // up, even when this connect brought it up. A feature set with no - // restored tree, as after a disconnect removed it, is read when it - // registers. A server already connected is refused by addServer below, - // so its trees are left as they are. - if (!this.mcplServerRegistry?.getServer(config.id)) this.checkpointManager?.loadHostStates(config.id); - const oldToolNames = new Set(this.mcplTools.map(t => t.name)); - await this.connectMcplServerInternal(config); - await this.refreshMcplTools(); - this.emitMcplToolDiff(oldToolNames, config.id); + for (const [other, otherId] of this.mcplPrefixMap) { + if (!own(otherId) && prefixesMeet(prefix, other)) throw prefixesMeetError(config.id, prefix, otherId, other); + } } /** @@ -18730,8 +18762,10 @@ export class AgentFramework { this.featureSetManager?.removeServer(id); this.checkpointManager?.removeServer(id); + // Only `id`'s own entry: an id with no config falls back to the default + // prefix, which another server may have set as its own. const prefix = config?.toolPrefix ?? `mcpl--${id}`; - this.mcplPrefixMap.delete(prefix); + if (this.mcplPrefixMap.get(prefix) === id) this.mcplPrefixMap.delete(prefix); this.mcplServerConfigs.delete(id); await this.refreshMcplTools(); @@ -18763,8 +18797,7 @@ export class AgentFramework { throw new Error(`MCPL server "${id}" is not configured`); } // Checked before the running server is disconnected, so a config this - // refuses leaves it up. (A colliding toolPrefix is refused only by the - // connect below, once the server is down.) + // refuses leaves it up, as refuseMcplConnect below does for its prefix. assertServerConfigBooleans(config); // Every connect reads the awareness ledger: its awareness barrier // reconciles it, and a stdio child taking the protective default derives @@ -18772,6 +18805,10 @@ export class AgentFramework { // refuses the restart and leaves the running child up, whatever the // server's baseline or transport. (No outbox configured: nothing to read.) this.getPlacedReactionBaseline(); + // What the connect would refuse refuses the restart the same way, before + // anything is disconnected. `id`'s own entries don't count, since the + // disconnect removes them whatever id the new config carries. + this.refuseMcplConnect(config, id); await this.runDisconnectMcplServer(id); await this.runConnectMcplServer(config); } @@ -18841,8 +18878,8 @@ export class AgentFramework { const prefix = config.toolPrefix ?? `mcpl--${id}`; const connection = this.mcplServerRegistry?.getServer(id) ?? null; const connected = connection?.isConnected ?? false; - // Attribute through the dispatch resolver: a bare prefix match would - // also count a nested prefix's tools (`foo` vs `foo--bar`) as ours. + // Attribute through the dispatch resolver, the one rule that picks a + // tool's server. const ownTools = (this.mcplTools ?? []).flatMap((t) => { const hit = this.resolveMcplTool(t.name); return hit && hit[0] === id ? [{ tool: t.name, serverTool: t.name.slice(hit[1].length + 2) }] : []; @@ -19256,6 +19293,12 @@ export class AgentFramework { /** * Discover tools from all connected MCPL servers and cache them. * Tools are namespaced as `{toolPrefix}--{toolName}` per server config. + * No two servers' prefixes meet (prefixesMeet, refused at create(), a + * runtime connect and a restart), so no two servers give one name. A + * server that lists one name more than once keeps its first listing, and + * an ops alert at each refresh that finds it says so (#265): a tool list + * holding one name twice is refused by membrane's native formatter and by + * Anthropic's API. */ private async refreshMcplTools(): Promise { if (!this.mcplServerRegistry) return; @@ -19265,6 +19308,7 @@ export class AgentFramework { const toolClasses = new Map(); const toolServers = new Map(); const listedServers = new Set(); + const repeats: Array<{ serverId: string; tools: string[] }> = []; for (const server of this.mcplServerRegistry.getAllServers()) { const config = this.mcplServerConfigs.get(server.id); @@ -19272,9 +19316,16 @@ export class AgentFramework { try { const result = await server.sendToolsList(); listedServers.add(server.id); + const listed = new Set(); + const repeated: string[] = []; for (const tool of result.tools) { if (!isToolAllowed(tool.name, config)) continue; const namespacedName = `${prefix}--${tool.name}`; + if (listed.has(namespacedName)) { + if (!repeated.includes(tool.name)) repeated.push(tool.name); + continue; + } + listed.add(namespacedName); toolServers.set(namespacedName, server.id); const attributedTool = tool as typeof tool & { featureSet?: unknown; @@ -19300,6 +19351,7 @@ export class AgentFramework { toolFeatureSets.set(namespacedName, featureSet); } } + if (repeated.length > 0) repeats.push({ serverId: server.id, tools: repeated }); } catch { // Server may not support tools/list — skip silently. A live server // whose handshake advertised no MCP `tools` has, in effect, listed @@ -19315,6 +19367,18 @@ export class AgentFramework { this.mcplToolClasses = toolClasses; this.mcplToolServers = toolServers; this.mcplListedServers = listedServers; + for (const { serverId, tools: repeatedTools } of repeats) { + const one = repeatedTools.length === 1; + const listed = repeatedTools.map((name) => logValue(name)).join(', '); + this.opsAlert( + 'mcpl-tool-duplicate', + serverId, + `MCPL server "${serverId}" lists ${one ? 'the tool' : 'the tools'} ${listed} more than once. ` + + `The model is offered the first listing of ${one ? 'it' : 'each'}, and the rest are left out. ` + + 'A server should list each tool once, so this is for its maintainer to change.', + { data: { tools: repeatedTools } }, + ); + } if (this.toolPatternChecksArmed) this.checkToolPatterns(); } diff --git a/src/mcpl/prefix-rule.ts b/src/mcpl/prefix-rule.ts new file mode 100644 index 000000000..eb07669d0 --- /dev/null +++ b/src/mcpl/prefix-rule.ts @@ -0,0 +1,32 @@ +/** + * When two MCPL servers' tool names can be the same name (#265). + * + * A server's tool reaches the model as `--`. Two prefixes + * give one name only when they meet: they're the same, one is the other's + * followed by `-`, or one begins with the other's followed by `--`. Server + * `foo` listing `bar--echo` and server `foo--bar` listing `echo` both give + * `foo--bar--echo`, and `foo` listing `-echo` and `foo-` listing `echo` both + * give `foo---echo`. Prefixes that don't meet never give one name, whatever + * their servers list, so `foo` beside `foo-bar` or `foobar` is fine. + */ +export function prefixesMeet(a: string, b: string): boolean { + const [shorter, longer] = a.length <= b.length ? [a, b] : [b, a]; + return longer === shorter || longer === `${shorter}-` || longer.startsWith(`${shorter}--`); +} + +/** + * The refusal for a server whose prefix meets another's: both servers, both + * prefixes, and the rule. + */ +export function prefixesMeetError( + serverId: string, + prefix: string, + otherId: string, + otherPrefix: string, +): Error { + return new Error( + `MCPL server "${serverId}" toolPrefix "${prefix}" meets server "${otherId}"'s toolPrefix "${otherPrefix}": ` + + `two prefixes can give one tool name when one is the other's followed by "-", or begins with the other's ` + + `followed by "--". Give one of the two servers a toolPrefix that doesn't meet the other's.`, + ); +} diff --git a/src/mcpl/types.ts b/src/mcpl/types.ts index 78d876e08..d22456da9 100644 --- a/src/mcpl/types.ts +++ b/src/mcpl/types.ts @@ -416,7 +416,10 @@ export interface McplServerConfig { * `toolLifecycle.*.tools`) match these full names; `enabledTools` / * `disabledTools` use the bare server-side names. * - * Must not collide with any module name or another server's prefix. + * Must not equal any module name or another server's prefix, nor meet + * another server's prefix: two prefixes meet when one is the other + * followed by `-`, or begins with the other followed by `--`, as `foo-` + * and `foo--bar` each meet `foo` (prefixesMeet, #265). */ toolPrefix?: string; diff --git a/test/mcpl-prefix-rule.test.ts b/test/mcpl-prefix-rule.test.ts new file mode 100644 index 000000000..fa9344e20 --- /dev/null +++ b/test/mcpl-prefix-rule.test.ts @@ -0,0 +1,345 @@ +/** + * Two MCPL servers can't give one tool name (#265). Their toolPrefixes may not + * meet, which create(), a runtime connect and a restart refuse, naming both + * servers and the rule. The prefix map those refusals read holds each + * configured server's own claim: a connect refuses an id already registered + * before it writes anything and drops any other entry of its id, and a + * disconnect removes only its own server's prefix. A server that lists one name + * more than once has its first listing offered to the model, and each refresh + * that finds it raises an `mcpl-tool-duplicate` ops alert. The rule itself is + * mcpl-prefixes-meet.test.ts's. + */ + +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { tmpdir } from 'node:os'; +import { mkdtempSync, rmSync } from 'node:fs'; +import { join } from 'node:path'; +import type { NormalizedRequest, NormalizedResponse } from '@animalabs/membrane'; +import type { ToolCall, ToolResult, TraceEvent } from '../src/index.js'; +import { AgentFramework } from '../src/index.js'; + +const FIXTURE = join(import.meta.dirname, 'fixtures/tool-listing-mcpl-server.mjs'); + +/** A framework, with the MCPL servers given configured at create(). */ +async function create(mcplServers: Array> = []) { + const dir = mkdtempSync(join(tmpdir(), 'af-mcpl-prefix-rule-')); + try { + const fw = await AgentFramework.create({ + storePath: join(dir, 'test.chronicle'), + membrane: { complete: async (_req: NormalizedRequest): Promise => { throw new Error('not used'); } } as never, + agents: [{ name: 'assistant', model: 'test-model', systemPrompt: 'test' }], + modules: [], + syncIntervalMs: 0, + ...(mcplServers.length > 0 ? { mcplServers } : {}), + } as never); + const cleanup = async () => { + await fw.stop(); + rmSync(dir, { recursive: true, force: true }); + }; + return { fw, cleanup }; + } catch (err) { + rmSync(dir, { recursive: true, force: true }); + throw err; + } +} + +/** A fixture server answering `:`, listing `echo`. */ +const fixture = (id: string, toolPrefix?: string) => ({ + id, + ...(toolPrefix === undefined ? {} : { toolPrefix }), + command: process.execPath, + args: [FIXTURE], + env: { NAME: id, TOOLS: 'echo' }, +}); + +/** A config whose command fails if it's ever run: a refusal must come first. */ +const unrun = (id: string, toolPrefix?: string) => ({ + id, + ...(toolPrefix === undefined ? {} : { toolPrefix }), + command: process.execPath, + args: ['-e', 'process.exit(3)'], +}); + +async function waitFor(cond: () => boolean, what: string, timeoutMs = 10_000): Promise { + const deadline = performance.now() + timeoutMs; + while (!cond()) { + if (performance.now() > deadline) throw new Error(`timed out waiting for ${what}`); + await new Promise((r) => setTimeout(r, 20)); + } +} + +/** The MCPL tools offered to the model, by name. */ +const offered = (fw: AgentFramework) => (fw as unknown as { mcplTools: Array<{ name: string }> }).mcplTools.map((t) => t.name).sort(); + +test('create() refuses two servers whose prefixes meet, naming both servers, both prefixes and the rule', async () => { + const cases: Array<[ReturnType[], string[]]> = [ + [[unrun('short', 'foo'), unrun('nested', 'foo--bar')], ['"nested"', '"foo--bar"', '"short"', '"foo"']], + [[unrun('nested', 'foo--bar'), unrun('short', 'foo')], ['"short"', '"foo"', '"nested"', '"foo--bar"']], + [[unrun('short', 'foo'), unrun('dash', 'foo-')], ['"dash"', '"foo-"', '"short"', '"foo"']], + // Default prefixes meet too: `mcpl--plain` and `mcpl--plain--inner`. + [[unrun('plain'), unrun('plain--inner')], ['"plain--inner"', '"mcpl--plain--inner"', '"plain"', '"mcpl--plain"']], + ]; + for (const [servers, parts] of cases) { + await assert.rejects(create(servers), (err: Error) => { + for (const part of [...parts, 'followed by "-"', 'followed by "--"', "a toolPrefix that doesn't meet the other's."]) { + assert.ok(err.message.includes(part), `the refusal says ${part}: ${err.message}`); + } + return true; + }); + } +}); + +test("create() takes prefixes that don't meet, and their servers' tools reach the model under each", async () => { + const h = await create([fixture('short', 'foo'), fixture('dashed', 'foo-bar'), fixture('joined', 'foobar')]); + try { + await waitFor(() => offered(h.fw).length === 3, 'the listings'); + assert.deepEqual(offered(h.fw), ['foo--echo', 'foo-bar--echo', 'foobar--echo']); + } finally { + await h.cleanup(); + } +}); + +test('a runtime connect whose prefix meets a server\'s is refused before anything is registered under it', async () => { + const h = await create([fixture('short', 'foo')]); + try { + await waitFor(() => offered(h.fw).length === 1, 'the listing'); + await assert.rejects(h.fw.connectMcplServer(unrun('nested', 'foo--bar')), /server "nested" toolPrefix "foo--bar" meets server "short"'s toolPrefix "foo"/); + assert.deepEqual(h.fw.listMcplServers().map((s) => s.id), ['short'], 'nothing is registered under the refused id'); + const internals = h.fw as unknown as { mcplServerConfigs: Map; mcplPrefixMap: Map }; + assert.equal(internals.mcplServerConfigs.has('nested'), false, 'and no config is kept for it'); + assert.equal(internals.mcplPrefixMap.has('foo--bar'), false, 'nor its prefix'); + assert.deepEqual(offered(h.fw), ['foo--echo']); + } finally { + await h.cleanup(); + } +}); + +test('a restart under a prefix that meets another server\'s is refused, and the running server stays up', async () => { + const h = await create([fixture('short', 'foo'), fixture('other', 'bar')]); + try { + await waitFor(() => offered(h.fw).length === 2, 'the listings'); + await assert.rejects(h.fw.restartMcplServer('other', fixture('other', 'foo-')), /server "other" toolPrefix "foo-" meets server "short"'s toolPrefix "foo"/); + assert.equal(h.fw.listMcplServers().find((s) => s.id === 'other')?.connected, true, 'the server the restart was for is still connected'); + assert.deepEqual(offered(h.fw), ['bar--echo', 'foo--echo']); + } finally { + await h.cleanup(); + } +}); + +test('a runtime connect of an id already registered is refused before it writes anything', async () => { + const h = await create([fixture('short', 'foo')]); + try { + await waitFor(() => offered(h.fw).length === 1, 'the listing'); + await assert.rejects(h.fw.connectMcplServer(unrun('short', 'elsewhere')), /MCPL server "short" is already registered/); + const internals = h.fw as unknown as { mcplServerConfigs: Map; mcplPrefixMap: Map }; + assert.equal(internals.mcplServerConfigs.get('short')?.toolPrefix, 'foo', "the live server's config is kept"); + assert.deepEqual([...internals.mcplPrefixMap], [['foo', 'short']], 'and no prefix is claimed for the refused connect'); + assert.equal(h.fw.listMcplServers().find((s) => s.id === 'short')?.connected, true); + } finally { + await h.cleanup(); + } +}); + +test('a restart into the id of another live server is refused before it disconnects anything', async () => { + const h = await create([fixture('first', 'one'), fixture('second', 'two')]); + try { + await waitFor(() => offered(h.fw).length === 2, 'the listings'); + await assert.rejects(h.fw.restartMcplServer('first', fixture('second', 'three')), /MCPL server "second" is already registered/); + const internals = h.fw as unknown as { mcplServerConfigs: Map }; + assert.equal(h.fw.listMcplServers().find((s) => s.id === 'first')?.connected, true, 'the server the restart was for is still connected'); + assert.equal(internals.mcplServerConfigs.get('second')?.toolPrefix, 'two', "and the other server's config is kept"); + assert.deepEqual(offered(h.fw), ['one--echo', 'two--echo']); + } finally { + await h.cleanup(); + } +}); + +test('a runtime connect retrying a server whose startup connect failed takes back its own prefix', async () => { + // The failed startup connect leaves `flaky`'s config and prefix configured. + const h = await create([unrun('flaky', 'flaky')]); + try { + assert.deepEqual(h.fw.listMcplServers().map((s) => [s.id, s.connected]), [['flaky', false]]); + await h.fw.connectMcplServer(fixture('flaky', 'flaky')); + await waitFor(() => offered(h.fw).length === 1, 'the listing'); + assert.deepEqual(offered(h.fw), ['flaky--echo']); + } finally { + await h.cleanup(); + } +}); + +test("a connect under a new prefix drops the prefix its id's failed connect left", async () => { + // The failed startup connect leaves `flaky` configured under `p`. + const h = await create([unrun('flaky', 'p')]); + try { + const internals = h.fw as unknown as { mcplPrefixMap: Map }; + assert.deepEqual([...internals.mcplPrefixMap], [['p', 'flaky']]); + await h.fw.connectMcplServer(fixture('flaky', 'q')); + await waitFor(() => offered(h.fw).length === 1, 'the listing'); + assert.deepEqual([...internals.mcplPrefixMap], [['q', 'flaky']], 'one entry for the id'); + await h.fw.disconnectMcplServer('flaky'); + assert.deepEqual([...internals.mcplPrefixMap], [], 'and none once it has gone'); + await h.fw.connectMcplServer(fixture('other', 'p-')); + await waitFor(() => offered(h.fw).length === 1, 'the new listing'); + assert.deepEqual(offered(h.fw), ['p---echo'], '`p-` meets nothing left behind'); + } finally { + await h.cleanup(); + } +}); + +test("a restart that changes the server's id isn't refused by the prefix it replaces", async () => { + // `mcpl--plain--x` meets `mcpl--plain`, but the restart disconnects `plain` first. + const h = await create([fixture('plain')]); + try { + await waitFor(() => offered(h.fw).length === 1, 'the listing'); + await h.fw.restartMcplServer('plain', fixture('plain--x')); + assert.deepEqual(h.fw.listMcplServers().map((s) => [s.id, s.connected]), [['plain--x', true]]); + await waitFor(() => offered(h.fw).length === 1, 'the new listing'); + assert.deepEqual(offered(h.fw), ['mcpl--plain--x--echo']); + } finally { + await h.cleanup(); + } +}); + +test("a disconnect of an id with no config leaves the prefix another server set as its own", async () => { + // `mcpl--ghost` is the default prefix of the unconfigured id `ghost`. + const h = await create([fixture('owner', 'mcpl--ghost')]); + try { + await waitFor(() => offered(h.fw).length === 1, 'the listing'); + await h.fw.disconnectMcplServer('ghost'); + const internals = h.fw as unknown as { mcplPrefixMap: Map }; + assert.equal(internals.mcplPrefixMap.get('mcpl--ghost'), 'owner', "the owner's prefix is still claimed"); + await assert.rejects(h.fw.connectMcplServer(unrun('dash', 'mcpl--ghost-')), /server "dash" toolPrefix "mcpl--ghost-" meets server "owner"'s toolPrefix "mcpl--ghost"/); + assert.deepEqual(offered(h.fw), ['mcpl--ghost--echo']); + } finally { + await h.cleanup(); + } +}); + +/** + * A framework with one stand-in server, `dup` on prefix `dup`, whose listing + * is `tools`: each [name, description, _meta?]. The stand-in records each + * call it reaches as ``. + */ +async function standIn(tools: Array<[string, string, Record?]>) { + const h = await create(); + const reached: string[] = []; + const connection = { + id: 'dup', + isConnected: true, + mcpToolsAdvertised: true, + sendToolsList: async () => ({ + tools: tools.map(([name, description, _meta]) => ({ name, description, inputSchema: { type: 'object', properties: {} }, ...(_meta ? { _meta } : {}) })), + }), + sendToolsCall: async (tool: string) => { + reached.push(tool); + return { content: [{ type: 'text', text: `dup:${tool}` }] }; + }, + }; + const internals = h.fw as unknown as { + mcplServerRegistry: unknown; + mcplServerConfigs: Map; + mcplPrefixMap: Map; + refreshMcplTools(): Promise; + dispatchToolCall(agentName: string, call: ToolCall): void; + }; + const before = internals.mcplServerRegistry; + internals.mcplServerRegistry = { getAllServers: () => [connection], getServer: (id: string) => (id === 'dup' ? connection : null) }; + internals.mcplServerConfigs = new Map([['dup', { id: 'dup', command: 'node', toolPrefix: 'dup' }]]); + internals.mcplPrefixMap = new Map([['dup', 'dup']]); + const alerts: Array> = []; + h.fw.onTrace((event) => { + if (event.type === 'ops:alert' && event.kind === 'mcpl-tool-duplicate') alerts.push(event as Extract); + }); + const answers = new Map(); + const push = h.fw.pushEvent.bind(h.fw); + h.fw.pushEvent = (event) => { + const e = event as { type: string; callId?: string; result?: ToolResult }; + if (e.type === 'tool-result' && e.callId && e.result) answers.set(e.callId, e.result); + push(event); + }; + /** The model's call of `name`: the tool name the stand-in was sent, or the refusal. */ + const callModel = async (name: string): Promise => { + const count = reached.length; + internals.dispatchToolCall('assistant', { id: `model-${name}-${count}`, name, input: {} }); + if (reached.length > count) return reached.at(-1)!; + await waitFor(() => answers.has(`model-${name}-${count}`), `the model's answer to ${name}`); + return `refused: ${answers.get(`model-${name}-${count}`)!.error}`; + }; + const cleanup = async () => { + // The stand-in registry has no closeAll for stop(): put the real one back. + internals.mcplServerRegistry = before; + await h.cleanup(); + }; + return { ...h, cleanup, alerts, refresh: () => internals.refreshMcplTools(), callModel }; +} + +test('a server listing one name twice is offered its first listing once, and a call reaches it', async () => { + const h = await standIn([['echo', 'the first echo'], ['ping', 'ping'], ['echo', 'the second echo']]); + try { + await h.refresh(); + const echoes = h.fw.getAllTools().filter((t) => t.name === 'dup--echo'); + assert.deepEqual(echoes.map((t) => t.description), ['the first echo'], 'offered once, as first listed'); + assert.deepEqual(offered(h.fw), ['dup--echo', 'dup--ping']); + assert.equal(await h.callModel('dup--echo'), 'echo', "the model's call reaches the server"); + } finally { + await h.cleanup(); + } +}); + +test("a repeated name keeps its first listing's class and feature set", async () => { + const h = await standIn([ + ['echo', 'the first echo', { 'mcpl/class': ['comms'], featureSet: 'first' }], + ['echo', 'the second echo', { 'mcpl/class': ['files'], featureSet: 'second' }], + ]); + try { + await h.refresh(); + const internals = h.fw as unknown as { mcplToolClasses: Map; mcplToolFeatureSets: Map }; + assert.deepEqual(internals.mcplToolClasses.get('dup--echo'), ['comms']); + assert.equal(internals.mcplToolFeatureSets.get('dup--echo'), 'first'); + } finally { + await h.cleanup(); + } +}); + +test('each refresh that finds a repeated name raises one alert naming it, and a listing without one raises none', async () => { + const h = await standIn([['echo', 'one'], ['echo', 'two'], ['echo', 'three']]); + try { + await h.refresh(); + assert.equal(h.alerts.length, 1, 'one alert for the refresh'); + const [alert] = h.alerts; + assert.equal(alert!.agentName, 'dup', 'raised for the server'); + assert.equal( + alert!.message, + 'MCPL server "dup" lists the tool echo more than once. The model is offered the first listing of it, and the rest are left out. ' + + 'A server should list each tool once, so this is for its maintainer to change.', + ); + assert.deepEqual(alert!.data?.tools, ['echo'], 'named once, though listed three times'); + await h.refresh(); + assert.equal(h.alerts.length, 2, 'and again at the next refresh that finds it'); + } finally { + await h.cleanup(); + } + const clean = await standIn([['echo', 'one'], ['ping', 'ping']]); + try { + await clean.refresh(); + assert.deepEqual(offered(clean.fw), ['dup--echo', 'dup--ping'], 'the listing was read'); + assert.equal(clean.alerts.length, 0); + } finally { + await clean.cleanup(); + } +}); + +test('two names repeated in one listing give one alert naming both', async () => { + const h = await standIn([['echo', 'a'], ['ping', 'b'], ['echo', 'c'], ['ping', 'd']]); + try { + await h.refresh(); + assert.equal(h.alerts.length, 1); + const [alert] = h.alerts; + assert.ok(alert!.message.includes('lists the tools echo, ping more than once. The model is offered the first listing of each,'), alert!.message); + assert.deepEqual(alert!.data?.tools, ['echo', 'ping']); + assert.deepEqual(offered(h.fw), ['dup--echo', 'dup--ping']); + } finally { + await h.cleanup(); + } +}); diff --git a/test/mcpl-prefixes-meet.test.ts b/test/mcpl-prefixes-meet.test.ts new file mode 100644 index 000000000..9ae425e51 --- /dev/null +++ b/test/mcpl-prefixes-meet.test.ts @@ -0,0 +1,32 @@ +/** + * prefixesMeet (#265): two MCPL toolPrefixes can give one tool name, + * `--`, exactly when they meet. Checked against a search + * over every pair of short prefixes; mcpl-prefix-rule.test.ts checks where + * the framework applies it. + */ + +import { test } from 'node:test'; +import assert from 'node:assert/strict'; +import { prefixesMeet } from '../src/mcpl/prefix-rule.js'; + +test('prefixesMeet says exactly when two prefixes can give one tool name', () => { + // Every pair of prefixes up to four characters of `a` and `-`, against a + // search for two tool names that join to one name. Four characters of tool + // name are enough for any such pair to show a shared name. + const words: string[] = []; + for (let n = 1; n <= 4; n++) { + for (let i = 0; i < 2 ** n; i++) words.push([...Array(n).keys()].map((k) => ((i >> k) & 1 ? '-' : 'a')).join('')); + } + for (const a of words) { + for (const b of words) { + const meet = words.some((t1) => words.some((t2) => `${a}--${t1}` === `${b}--${t2}`)); + assert.equal(prefixesMeet(a, b), meet, `"${a}" beside "${b}"`); + } + } + for (const [a, b] of [['foo', 'foo--bar'], ['foo--bar', 'foo'], ['foo', 'foo-'], ['mcpl--plain', 'mcpl--plain--inner'], ['foo', 'foo']]) { + assert.equal(prefixesMeet(a!, b!), true, `"${a}" meets "${b}"`); + } + for (const [a, b] of [['foo', 'foo-bar'], ['foo', 'foobar'], ['foo', 'bar'], ['foo-', 'foo-bar'], ['foo-bar', 'foo']]) { + assert.equal(prefixesMeet(a!, b!), false, `"${a}" doesn't meet "${b}"`); + } +}); diff --git a/test/mcpl-tool-class-listing.test.ts b/test/mcpl-tool-class-listing.test.ts index 888a47a9c..bd36ea573 100644 --- a/test/mcpl-tool-class-listing.test.ts +++ b/test/mcpl-tool-class-listing.test.ts @@ -51,6 +51,8 @@ test('listMcplServers: nested prefixes attribute each tool to one server, the on ['nested', { id: 'nested', command: 'node', toolPrefix: 'foo--bar' }], ]); framework.mcplServerRegistry = { getServer: () => connection }; + // Configuration refuses this pair since #265 (prefixesMeet). It's built + // here directly, to pin that attribution follows the listing, not a prefix. // The shorter prefix is registered first: insertion order must not decide. framework.mcplPrefixMap = new Map([['foo', 'foo'], ['foo--bar', 'nested']]); framework.mcplTools = [{ name: 'foo--ping' }, { name: 'foo--bar--baz' }, { name: 'foo--bar--qux' }]; diff --git a/test/mcpl-tool-resolution.test.ts b/test/mcpl-tool-resolution.test.ts index 765cc11bf..26f5fbfb8 100644 --- a/test/mcpl-tool-resolution.test.ts +++ b/test/mcpl-tool-resolution.test.ts @@ -7,8 +7,9 @@ * for a programmatic call, through the public executeToolCall or a module's * ctx.callTool, which also sends the server the tool name the model's call * sends; for an MCPL tool's source in the tool presentation; and for the - * RFC-008 class declarations a server's close forgets. Where two servers list - * the same full name, the listing holds the one listed last (#265). + * RFC-008 class declarations a server's close forgets. Prefixes that meet + * are refused at configuration since #265, so the stand-ins below build such + * a pair directly, to pin the resolver's own rule. */ import { test } from 'node:test'; @@ -385,13 +386,14 @@ async function live(servers: ReturnType[], names: string[]) { } test('with fixture servers configured at create(), one on the default prefix, every path reaches the server the model\'s call reaches', async () => { - // `plain` takes the default prefix, `mcpl--plain`; `inner`'s begins with it. + // `plain` takes the default prefix, `mcpl--plain`, and lists a name with + // `--` in it; `inner`'s prefix doesn't meet it (#265). const servers = [ fixture('plain', ['echo', 'inner--qux']), - fixture('inner', ['echo'], 'mcpl--plain--inner'), + fixture('inner', ['echo'], 'inner'), ]; const calls: Array<[string, string]> = [ - ['mcpl--plain--inner--echo', 'inner:echo'], + ['inner--echo', 'inner:echo'], ['mcpl--plain--echo', 'plain:echo'], ['mcpl--plain--inner--qux', 'plain:inner--qux'], ]; From 044f5268b98fe46ea815796de1a8920ceabe680a Mon Sep 17 00:00:00 2001 From: Humphrey-2407 Date: Sun, 18 Oct 2026 08:12:10 -0400 Subject: [PATCH 2/2] docs(changelog): leave the single resolver's fragment as main has it The pending fragment for the single MCPL resolver describes main as it is, where a tool name two servers list reaches the one listed last. This change's own fragments say what it changes: the breaking one for prefixes that meet, and the fixed one for a repeated tool name. Editing that fragment here also conflicts with a release's fold, which deletes every pending fragment. Co-Authored-By: Claude Opus 5.5 --- changelog.d/one-mcpl-resolver.fixed.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/changelog.d/one-mcpl-resolver.fixed.md b/changelog.d/one-mcpl-resolver.fixed.md index 6e5ee4f66..521f2581f 100644 --- a/changelog.d/one-mcpl-resolver.fixed.md +++ b/changelog.d/one-mcpl-resolver.fixed.md @@ -4,7 +4,9 @@ offers the name, else the longest matching prefix. It took the first configured server whose prefix the name began with, so where one prefix begins with another's, such as `foo` beside `foo--bar` or beside `foo-`, it - reached whichever of the two was configured first (#227, #230). + reached whichever of the two was configured first (#227, #230). Where two + servers list the same full name, the listing holds the one listed last + (#265), and both paths reach that one. - An MCPL tool's source in the tool presentation now names the server a call of it reaches, by the same resolver. **Operators:** a tool's source was the first configured server whose prefix its name began with, so with `foo` @@ -15,5 +17,3 @@ - A server's close forgets only its own tools' RFC-008 class declarations. Closing `foo` also forgot `foo--bar`'s, leaving its tools unclassed until the next tool refresh. -- Since #265, prefixes that meet, as `foo` does `foo--bar` and `foo-`, are - refused at configuration, so the cases above no longer start.