diff --git a/components/mcp/tools/application.ts b/components/mcp/tools/application.ts index 03e19f17ce..388ccd309e 100644 --- a/components/mcp/tools/application.ts +++ b/components/mcp/tools/application.ts @@ -133,7 +133,34 @@ interface ToolAnnotationsLike { openWorldHint?: boolean; } -type ResourcesRegistry = Map; +/** + * True when `pattern`'s only dynamic segment is a single trailing `:id` param (e.g. `widget/:id`). + * That is the ONLY shape the verb handlers below bind — `target.id = args.id`, treating `:id` as + * the record itself — so a differently-named param (`:widgetId`), more than one param, a `*wildcard` + * segment, or an `:id` that is NOT the final segment (`widget/:id/action`, where the route names a + * sub-resource, not the widget) would advertise an `id` input the handler never actually threads + * onto the real segment(s). + */ +function isSimpleIdRoute(pattern: string): boolean { + const segments = pattern.split('/'); + if (segments[segments.length - 1] !== ':id') return false; + let paramCount = 0; + for (const segment of segments) { + if (segment.startsWith('*')) return false; + if (segment.startsWith(':')) { + if (segment !== ':id') return false; + paramCount++; + } + } + return paramCount === 1; +} + +/** A compiled parameterised route (e.g. `/widget/:id`), stored outside the base Map. */ +interface ParamRouteEntry { + pattern: string; + entry: ResourceRegistryEntry; +} +type ResourcesRegistry = Map & { paramRoutes?: ParamRouteEntry[] }; type RequestTargetCtor = new () => Record & { conditions?: unknown[]; operator?: string; @@ -198,9 +225,15 @@ function loadRequestTarget(): RequestTargetCtor | undefined { * * Falls back to the registration-time class only if the registry lookup fails * (e.g. the entry was removed), so a stale tool still dispatches *something*. + * + * Parameterised routes (e.g. `/widget/:id`) live outside the base Map in + * `paramRoutes`, so a plain `Map.get(path)` always misses for them — checked + * there too, or the live-dispatch guarantee above silently doesn't apply to + * param-route resources. */ function liveResource(path: string, fallback: ResourceClassLike): ResourceClassLike { - const entry = loadResources()?.get(path); + const registry = loadResources(); + const entry = registry?.get(path) ?? registry?.paramRoutes?.find((route) => route.pattern === path)?.entry; return (entry?.Resource as ResourceClassLike | undefined) ?? fallback; } @@ -933,21 +966,44 @@ function buildApplicationTools(resources: ResourcesRegistry): void { const claimedSuffixes = new Set(); let toolsRegistered = 0; let resourcesConsidered = 0; - for (const [path, entry] of resources) { + + // How much of a param route the GENERATED verb handlers can bind: + // 'id' — single-`:id` routes: get/update/patch/delete bind `target.id = args.id`, + // but makeCreateHandler forces `target.isCollection = true` (never binds id) + // and makeSearchHandler is collection-scoped, so create/search are dropped. + // 'none' — multi-segment/named-wildcard routes: no generated handler can bind the + // segments yet, so ALL generated verbs are dropped. Author-defined + // mcpTools/mcpPrompts carry their own schemas and handler methods, so they + // register regardless of binding mode. + const considerEntry = ( + path: string, + entry: ResourceRegistryEntry | undefined, + paramBinding?: 'id' | 'none' + ): void => { + if (!entry) return; resourcesConsidered++; - if (!shouldEnumerate(entry)) continue; + if (!shouldEnumerate(entry)) return; const ResourceClass = entry.Resource; // @hidden type-level: suppress the Resource from MCP tool listing entirely. // Data remains accessible via direct query/RBAC; only descriptive surfaces drop it. if (ResourceClass?.hidden === true) { harperLogger.trace(`MCP application: '/${path}' suppressed from tool listing (@hidden)`); - continue; + return; } const verbs = detectVerbs(ResourceClass); + if (paramBinding === 'id') { + verbs.search = false; + verbs.create = false; + } else if (paramBinding === 'none') { + harperLogger.trace( + `MCP application: '/${path}' generated verb tools skipped — multi-segment/named-wildcard binding not yet supported` + ); + verbs.get = verbs.search = verbs.create = verbs.updatePut = verbs.updatePatch = verbs.delete = false; + } const hasVerbs = verbs.get || verbs.search || verbs.create || verbs.updatePut || verbs.updatePatch || verbs.delete; const hasCustomTools = Array.isArray(ResourceClass?.mcpTools) && ResourceClass.mcpTools.length > 0; const hasCustomPrompts = Array.isArray(ResourceClass?.mcpPrompts) && ResourceClass.mcpPrompts.length > 0; - if (!hasVerbs && !hasCustomTools && !hasCustomPrompts) continue; + if (!hasVerbs && !hasCustomTools && !hasCustomPrompts) return; const databaseName = ResourceClass?.databaseName; const tableName = ResourceClass?.tableName; const suffix = uniqueSuffix(path, databaseName, claimedSuffixes); @@ -966,7 +1022,21 @@ function buildApplicationTools(resources: ResourcesRegistry): void { } toolsRegistered += registerCustomMcpTools(ResourceClass, path); registerCustomMcpPrompts(ResourceClass, path); + }; + + for (const [path, entry] of resources) considerEntry(path, entry); + + // Parameterised routes (e.g. `/widget/:id`) live OUTSIDE the base Map: Resources.setParamRoute + // stores them in `paramRoutes` and returns before the Map insert, so the loop above never sees + // them. Without this, a custom resource declaring `static path = '/widget/:id'` produces ZERO MCP + // tools — even though it appears in the OpenAPI document, which already iterates `paramRoutes`. + // Enumerate them so the tool surface matches the REST surface; the binding mode restricts the + // GENERATED verb tools to what their handlers actually bind (see considerEntry), while custom + // mcpTools/mcpPrompts register on every route shape. + for (const route of resources.paramRoutes ?? []) { + considerEntry(route.pattern, route.entry, isSimpleIdRoute(route.pattern) ? 'id' : 'none'); } + harperLogger.info( `MCP application profile: considered ${resourcesConsidered} resource(s), registered ${toolsRegistered} tool(s)` ); diff --git a/unitTests/components/mcp/tools/application-paramroutes.test.js b/unitTests/components/mcp/tools/application-paramroutes.test.js new file mode 100644 index 0000000000..f9638e9565 --- /dev/null +++ b/unitTests/components/mcp/tools/application-paramroutes.test.js @@ -0,0 +1,172 @@ +// Regression test for param-route enumeration: a custom resource declaring a +// parameterised path (e.g. `static path = '/widget/:id'`) is stored by +// Resources.setParamRoute in the `paramRoutes` side-array and never inserted +// into the base Map. MCP's buildApplicationTools previously iterated only the +// Map, so such resources produced ZERO tools — while OpenAPI (which iterates +// paramRoutes) did expose them. buildApplicationTools now enumerates paramRoutes +// too, so the tool surface matches the REST surface. + +const assert = require('node:assert'); +const { + registerApplicationTools, + _setResourcesForTest, + _setRequestTargetForTest, + _resetApplicationToolsRegisteredForTest, +} = require('#src/components/mcp/tools/application'); +const { listTools, getTool, _resetRegistryForTest } = require('#src/components/mcp/toolRegistry'); +const { _resetPromptRegistryForTest } = require('#src/components/mcp/promptRegistry'); + +const SUPER = { username: 'admin', role: { permission: { super_user: true } } }; + +class FakeRequestTarget {} + +// A custom (non-table) Resource registered at a parameterised path. +function makeParamResource() { + class Widget {} + Widget.prototype.get = function () {}; // read verb present + Widget.prototype.post = function () {}; // create verb present + Widget.get = async (target) => ({ id: target.id, name: 'sample' }); + Widget.post = async (_target, data) => ({ created: true, ...data }); + return Widget; +} + +// A registry (Map) with a `paramRoutes` side-array, mirroring resources/Resources.ts. +function makeRegistryWithParamRoute(pattern, Resource, exportTypes) { + const m = new Map(); // intentionally no static Map entries + m.paramRoutes = [{ pattern, entry: { path: pattern, Resource, exportTypes, hasSubPaths: false, relativeURL: '' } }]; + return m; +} + +function listSuperToolNames() { + const { tools } = listTools({ user: SUPER, profile: 'application', sessionId: 's', limit: 200 }); + return tools.map((t) => t.name); +} + +describe('mcp/tools/application — parameterised routes', () => { + beforeEach(() => { + _resetRegistryForTest(); + _resetPromptRegistryForTest(); + _setRequestTargetForTest(FakeRequestTarget); + }); + + afterEach(() => { + _resetRegistryForTest(); + _resetPromptRegistryForTest(); + _setResourcesForTest(undefined); + _setRequestTargetForTest(undefined); + _resetApplicationToolsRegisteredForTest(); + }); + + it('registers tools for a custom resource on a parameterised path (/widget/:id)', () => { + _setResourcesForTest(makeRegistryWithParamRoute('widget/:id', makeParamResource())); + registerApplicationTools(); + const names = listSuperToolNames(); + assert.ok(names.length > 0, `expected tools for the parameterised resource, got none`); + assert.ok( + names.some((n) => n.startsWith('get_')), + `expected a get_* tool, got ${JSON.stringify(names)}` + ); + }); + + it('keeps custom mcpTools on multi-segment/wildcard routes while suppressing generated verbs', () => { + // Generated verb handlers cannot bind multi-segment/wildcard params yet, but + // author-defined mcpTools carry their own schemas and handler methods — a + // `files/*rest` resource with explicit tools must not become invisible. + class Files {} + Files.prototype.get = function () {}; + Files.get = async () => ({}); + Files.prototype.listVersions = function () {}; + Files.mcpTools = [ + { + name: 'list_file_versions', + method: 'listVersions', + description: 'List versions of a file', + inputSchema: { type: 'object', properties: { path: { type: 'string' } }, required: ['path'] }, + }, + ]; + _setResourcesForTest(makeRegistryWithParamRoute('files/*rest', Files)); + registerApplicationTools(); + const names = listSuperToolNames(); + assert.ok(names.includes('list_file_versions'), `expected the custom tool, got ${JSON.stringify(names)}`); + assert.ok( + !names.some((n) => n.startsWith('get_')), + `expected no generated verb tools on a wildcard route, got ${JSON.stringify(names)}` + ); + }); + + it('does not register create_*/search_* for a param route (their handlers cannot bind the record id)', () => { + // makeCreateHandler forces `target.isCollection = true` and never sets `target.id`; + // makeSearchHandler is collection-scoped. Neither matches "the record named by :id", + // so a param-route resource with `post`/`search` should still get its `get_*` tool but + // not `create_*`/`search_*`. + _setResourcesForTest(makeRegistryWithParamRoute('widget/:id', makeParamResource())); + registerApplicationTools(); + const names = listSuperToolNames(); + assert.ok( + names.some((n) => n.startsWith('get_')), + `expected a get_* tool, got ${JSON.stringify(names)}` + ); + assert.ok( + !names.some((n) => n.startsWith('create_') || n.startsWith('search_')), + `expected no create_*/search_* tool for a param route, got ${JSON.stringify(names)}` + ); + }); + + it('respects mcp:false on a parameterised route', () => { + _setResourcesForTest(makeRegistryWithParamRoute('secret/:id', makeParamResource(), { mcp: false })); + registerApplicationTools(); + assert.equal(listSuperToolNames().length, 0, 'mcp:false should suppress the param-route resource'); + }); + + it('registers nothing when there are no resources at all', () => { + const empty = new Map(); + empty.paramRoutes = []; + _setResourcesForTest(empty); + registerApplicationTools(); + assert.equal(listSuperToolNames().length, 0); + }); + + it('dispatches on the live paramRoutes entry, not the class captured at registration', async () => { + // Regression guard: buildApplicationTools binds verb handlers to `liveResource(path, ...)`, + // which re-reads the registry at call time. For a param route, that lookup must consult + // `paramRoutes` (not just the base Map) or it silently falls back to the stale captured + // class — the same live-dispatch guarantee static routes rely on for per-record RBAC. + const registry = makeRegistryWithParamRoute('widget/:id', makeParamResource()); + _setResourcesForTest(registry); + registerApplicationTools(); + + class Replacement {} + Replacement.get = async (target) => ({ id: target.id, name: 'from-replacement' }); + registry.paramRoutes[0].entry.Resource = Replacement; + + const result = await getTool('get_widget_id').handler({ id: '1' }, { user: SUPER }); + assert.equal( + result.structuredContent?.name, + 'from-replacement', + 'expected the tool to dispatch on the live registry entry after it was swapped' + ); + }); + + // The verb handlers only bind `target.id = args.id`, treating `:id` as the record itself. + // A route with a differently-named param, more than one param, a `*wildcard` segment, or an + // `:id` that is not the final segment (`widget/:id/action` names a sub-resource, not the + // widget) would advertise an `id`-only tool that silently drops the other segment(s) — so + // those shapes must be skipped, not exposed. + for (const pattern of [ + 'widget/:id/action/:action', + 'widget/:id/action', + 'widget/:widgetId', + 'files/*rest', + 'files/*', + ]) { + it(`skips a parameterised route the verb handlers cannot fully bind (${pattern})`, () => { + _setResourcesForTest(makeRegistryWithParamRoute(pattern, makeParamResource())); + registerApplicationTools(); + assert.equal( + listSuperToolNames().length, + 0, + `expected no tools for an unsupported param shape, got ${JSON.stringify(listSuperToolNames())}` + ); + }); + } +});