Skip to content
82 changes: 76 additions & 6 deletions components/mcp/tools/application.ts
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,34 @@ interface ToolAnnotationsLike {
openWorldHint?: boolean;
}

type ResourcesRegistry = Map<string, ResourceRegistryEntry>;
/**
* 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;
}
Comment thread
kriszyp marked this conversation as resolved.

/** A compiled parameterised route (e.g. `/widget/:id`), stored outside the base Map. */
interface ParamRouteEntry {
pattern: string;
entry: ResourceRegistryEntry;
}
type ResourcesRegistry = Map<string, ResourceRegistryEntry> & { paramRoutes?: ParamRouteEntry[] };
type RequestTargetCtor = new () => Record<string, unknown> & {
conditions?: unknown[];
operator?: string;
Expand Down Expand Up @@ -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;
}

Expand Down Expand Up @@ -933,21 +966,44 @@ function buildApplicationTools(resources: ResourcesRegistry): void {
const claimedSuffixes = new Set<string>();
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);
Expand All @@ -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)`
);
Expand Down
172 changes: 172 additions & 0 deletions unitTests/components/mcp/tools/application-paramroutes.test.js
Original file line number Diff line number Diff line change
@@ -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())}`
);
});
}
});
Loading