fix(favourite-index): route favourite/ calls even before tools/list (lazy resolver)
Some checks failed
CI/CD / typecheck (pull_request) Successful in 1m4s
CI/CD / lint (pull_request) Successful in 2m12s
CI/CD / test (pull_request) Successful in 1m20s
CI/CD / smoke (pull_request) Failing after 1m54s
CI/CD / build (pull_request) Successful in 4m31s
CI/CD / publish (pull_request) Has been skipped
Some checks failed
CI/CD / typecheck (pull_request) Successful in 1m4s
CI/CD / lint (pull_request) Successful in 2m12s
CI/CD / test (pull_request) Successful in 1m20s
CI/CD / smoke (pull_request) Failing after 1m54s
CI/CD / build (pull_request) Successful in 4m31s
CI/CD / publish (pull_request) Has been skipped
The presented→canonical resolver was only built during tools/list, so a favourite/<tool> call arriving before any list this session fell through to the namespaced router and 404'd (all/ was fine — it prefix-strips). Real MCP clients list first, but the smoke test's initialize→call flow exposed it. Extract buildPresentation() and lazily rebuild the resolver in onToolCallBefore when a favourite//all/ name isn't resolved yet. + unit test (favourite/ call with no prior list) and realistic listTools() in the smoke routing case. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -52,6 +52,64 @@ function splitServer(canonical: string): { server: string; short: string } {
|
|||||||
: { server: canonical.slice(0, i), short: canonical.slice(i + 1) };
|
: { server: canonical.slice(0, i), short: canonical.slice(i + 1) };
|
||||||
}
|
}
|
||||||
|
|
||||||
|
interface Presentation {
|
||||||
|
tools: ToolDefinition[];
|
||||||
|
/** presented name → canonical `server/tool` */
|
||||||
|
resolver: Map<string, string>;
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Reshape an upstream catalog into favourites-first + full all/ catalog, and
|
||||||
|
* the presented→canonical routing map. Pure — shared by onToolsList and the
|
||||||
|
* lazy rebuild in onToolCallBefore.
|
||||||
|
*/
|
||||||
|
function buildPresentation(
|
||||||
|
upstreamTools: ToolDefinition[],
|
||||||
|
favourites: string[],
|
||||||
|
cap?: number,
|
||||||
|
): Presentation {
|
||||||
|
const byName = new Map(upstreamTools.map((t) => [t.name, t]));
|
||||||
|
const resolver = new Map<string, string>();
|
||||||
|
const usedPresented = new Set<string>();
|
||||||
|
const seenCanonical = new Set<string>();
|
||||||
|
|
||||||
|
// Favourites first: configured order, only those present, capped, with
|
||||||
|
// short-name collision disambiguation.
|
||||||
|
const limit = cap ?? favourites.length;
|
||||||
|
const favTools: ToolDefinition[] = [];
|
||||||
|
for (const canonical of favourites) {
|
||||||
|
if (favTools.length >= limit) break;
|
||||||
|
if (seenCanonical.has(canonical)) continue;
|
||||||
|
const t = byName.get(canonical);
|
||||||
|
if (!t) continue; // stale/missing pin — skip (all/ still exposes it)
|
||||||
|
const { server, short } = splitServer(canonical);
|
||||||
|
let presented = `favourite/${short}`;
|
||||||
|
if (usedPresented.has(presented)) presented = `favourite/${server}-${short}`;
|
||||||
|
if (usedPresented.has(presented)) continue; // still collides (rare) — skip
|
||||||
|
seenCanonical.add(canonical);
|
||||||
|
usedPresented.add(presented);
|
||||||
|
resolver.set(presented, canonical);
|
||||||
|
favTools.push({
|
||||||
|
name: presented,
|
||||||
|
description: `${humanize(short)} — common (${server})`,
|
||||||
|
...(t.inputSchema !== undefined ? { inputSchema: t.inputSchema } : {}),
|
||||||
|
});
|
||||||
|
}
|
||||||
|
|
||||||
|
// Then the full catalog under all/<server>/<tool>.
|
||||||
|
const allTools: ToolDefinition[] = upstreamTools.map((t) => {
|
||||||
|
const presented = `all/${t.name}`;
|
||||||
|
resolver.set(presented, t.name);
|
||||||
|
return {
|
||||||
|
name: presented,
|
||||||
|
...(t.description !== undefined ? { description: t.description } : {}),
|
||||||
|
...(t.inputSchema !== undefined ? { inputSchema: t.inputSchema } : {}),
|
||||||
|
};
|
||||||
|
});
|
||||||
|
|
||||||
|
return { tools: [...favTools, ...allTools], resolver };
|
||||||
|
}
|
||||||
|
|
||||||
export function createFavouriteIndexPlugin(config: FavouriteIndexConfig): ProxyModelPlugin {
|
export function createFavouriteIndexPlugin(config: FavouriteIndexConfig): ProxyModelPlugin {
|
||||||
const favourites = config.favourites ?? [];
|
const favourites = config.favourites ?? [];
|
||||||
const cap = config.maxFavourites && config.maxFavourites > 0 ? config.maxFavourites : undefined;
|
const cap = config.maxFavourites && config.maxFavourites > 0 ? config.maxFavourites : undefined;
|
||||||
@@ -79,54 +137,33 @@ export function createFavouriteIndexPlugin(config: FavouriteIndexConfig): ProxyM
|
|||||||
return tools;
|
return tools;
|
||||||
}
|
}
|
||||||
|
|
||||||
const byName = new Map(upstreamTools.map((t) => [t.name, t]));
|
const { tools: presented, resolver } = buildPresentation(upstreamTools, favourites, cap);
|
||||||
const resolver = new Map<string, string>(); // presented → canonical
|
|
||||||
const usedPresented = new Set<string>();
|
|
||||||
const seenCanonical = new Set<string>();
|
|
||||||
|
|
||||||
// Favourites first: configured order, only those present, capped, with
|
|
||||||
// short-name collision disambiguation.
|
|
||||||
const limit = cap ?? favourites.length;
|
|
||||||
const favTools: ToolDefinition[] = [];
|
|
||||||
for (const canonical of favourites) {
|
|
||||||
if (favTools.length >= limit) break;
|
|
||||||
if (seenCanonical.has(canonical)) continue;
|
|
||||||
const t = byName.get(canonical);
|
|
||||||
if (!t) continue; // stale/missing pin — skip (all/ still exposes it)
|
|
||||||
const { server, short } = splitServer(canonical);
|
|
||||||
let presented = `favourite/${short}`;
|
|
||||||
if (usedPresented.has(presented)) presented = `favourite/${server}-${short}`;
|
|
||||||
if (usedPresented.has(presented)) continue; // still collides (rare) — skip
|
|
||||||
seenCanonical.add(canonical);
|
|
||||||
usedPresented.add(presented);
|
|
||||||
resolver.set(presented, canonical);
|
|
||||||
favTools.push({
|
|
||||||
name: presented,
|
|
||||||
description: `${humanize(short)} — common (${server})`,
|
|
||||||
...(t.inputSchema !== undefined ? { inputSchema: t.inputSchema } : {}),
|
|
||||||
});
|
|
||||||
}
|
|
||||||
|
|
||||||
// Then the full catalog under all/<server>/<tool>.
|
|
||||||
const allTools: ToolDefinition[] = upstreamTools.map((t) => {
|
|
||||||
const presented = `all/${t.name}`;
|
|
||||||
resolver.set(presented, t.name);
|
|
||||||
return {
|
|
||||||
name: presented,
|
|
||||||
...(t.description !== undefined ? { description: t.description } : {}),
|
|
||||||
...(t.inputSchema !== undefined ? { inputSchema: t.inputSchema } : {}),
|
|
||||||
};
|
|
||||||
});
|
|
||||||
|
|
||||||
ctx.state.set(RESOLVER_KEY, resolver);
|
ctx.state.set(RESOLVER_KEY, resolver);
|
||||||
return [...favTools, ...allTools, ...passthrough];
|
return [...presented, ...passthrough];
|
||||||
},
|
},
|
||||||
|
|
||||||
async onToolCallBefore(toolName, _args, request, ctx) {
|
async onToolCallBefore(toolName, _args, request, ctx) {
|
||||||
const resolver = ctx.state.get(RESOLVER_KEY) as Map<string, string> | undefined;
|
if (!toolName.startsWith('favourite/') && !toolName.startsWith('all/')) return null;
|
||||||
const canonical =
|
|
||||||
resolver?.get(toolName) ??
|
let resolver = ctx.state.get(RESOLVER_KEY) as Map<string, string> | undefined;
|
||||||
(toolName.startsWith('all/') ? toolName.slice('all/'.length) : undefined);
|
let canonical = resolver?.get(toolName);
|
||||||
|
|
||||||
|
// Resolver missing (a call arrived before tools/list this session): rebuild
|
||||||
|
// it so favourite/ names — which, unlike all/, can't be prefix-stripped —
|
||||||
|
// still route.
|
||||||
|
if (canonical === undefined) {
|
||||||
|
const upstream = await ctx.discoverTools();
|
||||||
|
if (upstream.length > 0) {
|
||||||
|
resolver = buildPresentation(upstream, favourites, cap).resolver;
|
||||||
|
ctx.state.set(RESOLVER_KEY, resolver);
|
||||||
|
canonical = resolver.get(toolName);
|
||||||
|
}
|
||||||
|
// all/ is always derivable by stripping the prefix.
|
||||||
|
if (canonical === undefined && toolName.startsWith('all/')) {
|
||||||
|
canonical = toolName.slice('all/'.length);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
if (canonical && canonical !== toolName) {
|
if (canonical && canonical !== toolName) {
|
||||||
// Rewrite presented name → canonical `server/tool`, then continue the
|
// Rewrite presented name → canonical `server/tool`, then continue the
|
||||||
// chain (return null) so normal routing + content-pipeline still run.
|
// chain (return null) so normal routing + content-pipeline still run.
|
||||||
|
|||||||
@@ -134,6 +134,19 @@ describe('favourite-index plugin', () => {
|
|||||||
expect(text).toBe('Called scale_deployment');
|
expect(text).toBe('Called scale_deployment');
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it('routes a favourite/ call even before tools/list (lazy resolver rebuild)', async () => {
|
||||||
|
const router = setup({ favourites: ['vault/read_secret'] });
|
||||||
|
router.addUpstream(mockUpstream('vault', CATALOG.vault));
|
||||||
|
// initialize only — deliberately NO tools/list, so the resolver is unset.
|
||||||
|
await router.route({ jsonrpc: '2.0', id: 1, method: 'initialize' }, { sessionId: 's1' });
|
||||||
|
|
||||||
|
const res = await router.route(
|
||||||
|
{ jsonrpc: '2.0', id: 2, method: 'tools/call', params: { name: 'favourite/read_secret', arguments: {} } },
|
||||||
|
{ sessionId: 's1' },
|
||||||
|
);
|
||||||
|
expect((res.result as { content: Array<{ text: string }> }).content[0]!.text).toBe('Called read_secret');
|
||||||
|
});
|
||||||
|
|
||||||
it('no-ops while the session is gated (only begin_session visible)', async () => {
|
it('no-ops while the session is gated (only begin_session visible)', async () => {
|
||||||
const router = setup({ gated: true, favourites: ['k8s/get_pods'] });
|
const router = setup({ gated: true, favourites: ['k8s/get_pods'] });
|
||||||
router.addUpstream(mockUpstream('k8s', CATALOG.k8s));
|
router.addUpstream(mockUpstream('k8s', CATALOG.k8s));
|
||||||
|
|||||||
@@ -104,6 +104,7 @@ describe('Smoke: favourite-index presentation', () => {
|
|||||||
chat.section('favourite-index routing');
|
chat.section('favourite-index routing');
|
||||||
try {
|
try {
|
||||||
await chat.initialize();
|
await chat.initialize();
|
||||||
|
await chat.listTools(); // real clients list before calling — populates the resolver
|
||||||
|
|
||||||
// Calling with no args → the UPSTREAM tool's arg validation fires, proving
|
// Calling with no args → the UPSTREAM tool's arg validation fires, proving
|
||||||
// the presented name routed to the real server (not a -32601 unknown tool).
|
// the presented name routed to the real server (not a -32601 unknown tool).
|
||||||
|
|||||||
Reference in New Issue
Block a user