diff --git a/README.md b/README.md index 5026386c2..74b533c67 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/src/framework.ts b/src/framework.ts index 06b698273..6f38e3820 100644 --- a/src/framework.ts +++ b/src/framework.ts @@ -78,6 +78,7 @@ import { Agent, assertAgentSwitches } from './agent.js'; import { assertBooleanSetting, holdsSettings } from './config-checks.js'; import { ModuleRegistry, checkedToolResult, ensureStateRegistered, storableToolResult, thrownResult } from './module-registry.js'; import { McplServerRegistry } from './mcpl/server-registry.js'; +import { prefixesMeet, prefixesMeetError } from './mcpl/prefix-rule.js'; import { logDetail, logValue } from './mcpl/log-value.js'; import { FeatureSetManager } from './mcpl/feature-set-manager.js'; import { computeGrant, CapabilityGrant, expandAdvertisementShorthand } from './mcpl/capability-grant.js'; @@ -2291,8 +2292,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(); @@ -2844,6 +2845,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); } @@ -5090,8 +5095,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]}`); @@ -18530,8 +18535,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); } @@ -18597,13 +18601,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); @@ -20590,8 +20595,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); @@ -21157,8 +21167,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( @@ -21166,23 +21207,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); + } } /** @@ -21214,8 +21246,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(); @@ -21247,8 +21281,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 @@ -21256,6 +21289,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); } @@ -21325,8 +21362,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) }] : []; @@ -21936,6 +21973,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; @@ -21945,6 +21988,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); @@ -21952,9 +21996,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; @@ -21980,6 +22031,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 @@ -21995,6 +22047,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 1d2973cb6..d84c655ed 100644 --- a/src/mcpl/types.ts +++ b/src/mcpl/types.ts @@ -419,7 +419,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'], ];