Skip to content

feat: auto-resolve service-prefixed tool names#16

Open
rodaddy wants to merge 1 commit intomainfrom
feat/auto-prefix-tool-names
Open

feat: auto-resolve service-prefixed tool names#16
rodaddy wants to merge 1 commit intomainfrom
feat/auto-prefix-tool-names

Conversation

@rodaddy
Copy link
Copy Markdown
Owner

@rodaddy rodaddy commented Apr 26, 2026

Summary

  • mcp2cli n8n list_workflows now auto-resolves to n8n_list_workflows when the unprefixed name doesn't match
  • Adds resolveToolName() in src/schema/introspect.ts -- tries exact match first, then {service}_{tool} prefix
  • Applied to all three call paths: getToolSchema(), direct connection, and daemon /call endpoint

Test plan

  • 788/788 tests pass
  • Verified mcp2cli n8n list_workflows --params '{}' resolves correctly
  • Deployed to Air, CT 216, Happy Agent, CC LXCs 120-123

🤖 Generated with Claude Code

`mcp2cli n8n list_workflows` now resolves to `n8n_list_workflows` automatically
since the service is already specified. Eliminates wasted calls from agents that
don't know the prefix convention.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 26, 2026

Claude finished @rodaddy's task —— View job


PR Review: feat/auto-prefix-tool-names

  • Read changed files and gather context
  • Analyze for bugs, security issues, resource leaks, error handling, TypeScript strictness
  • Post review feedback

Overall the implementation is clean and well-scoped. A few issues worth addressing, roughly ordered by severity:


Security: Access control checked against unresolved name

src/cli/commands/tool-call.ts:81 and src/daemon/server.ts:121

The access policy check happens before tool name resolution in both paths. This means a user can bypass a blocklist on n8n_dangerous_tool by calling mcp2cli n8n dangerous_tool — the policy check sees dangerous_tool (not in blocklist), passes, then the call resolves to n8n_dangerous_tool and executes.

In tool-call.ts (direct path):

checkToolAccess(parsed.value.toolName, policy)   // line 81 -- checks "dangerous_tool"
...
resolveToolName(...)  // line 200 -- resolves to "n8n_dangerous_tool"
conn.client.callTool({ name: resolvedName })      // executes blocked tool

In server.ts (daemon /call path), there's no access control check at all on the daemon side (it relies on the CLI layer), but the dry-run schema path in the CLI (tool-call.ts:98-128) also passes the unresolved name to the daemon without any pre-check against the resolved name.

Fix: resolve the tool name first (or re-check access against the resolved name after resolution).


Bug: getToolSchema doesn't paginate

src/schema/introspect.ts:103

const response = await client.listTools();  // no pagination

listToolsForService() correctly paginates via nextCursor, but getToolSchema() only fetches the first page. On services with many tools, resolveToolName() will operate on an incomplete list and may return null for tools that exist on later pages.

Fix: use listToolsForService() internally, or extract the paginated fetch into a shared helper.


Bug: Timeout timer not cleared on listTools error path

src/daemon/server.ts:116-122

const timer = setTimeout(() => controller.abort(), timeout);

let resolvedTool = body.tool;
try {
  const allTools = await conn.client.listTools();
  resolvedTool = resolveToolName(...) ?? body.tool;
} catch { /* listTools unavailable, use original name */ }

The timer is only cleared inside the finally block at line 139, which is inside the inner try for callTool. If listTools throws and we fall through to callTool, the timer is still running — that's fine. But if callTool itself throws synchronously before the Promise.race, the finally at line 139 does clear the timer, so this is actually OK. However, if the outer catch at line 154 fires before reaching clearTimeout, the timer leaks until it fires and calls abort() on an already-abandoned controller. Low severity since AbortController abort on a resolved race is a no-op, but it's worth noting.


TypeScript: resolveToolName not exported from barrel

src/schema/index.ts

resolveToolName is imported directly from ../../schema/introspect.ts in both call sites rather than from the barrel ../../schema/index.ts. The barrel doesn't re-export it:

// src/schema/index.ts (existing)
export { parseDotNotation, listToolsForService, getToolSchema, generateUsageExample } from "./introspect.ts";
// resolveToolName missing

Minor consistency issue — either add it to the barrel or document that it's intentionally internal.


Minor: Silent fallback hides resolution failures in daemon path

src/daemon/server.ts:121-122

resolvedTool = resolveToolName(allTools.tools, body.tool, body.service) ?? body.tool;

If resolveToolName returns null (tool genuinely doesn't exist), the fallback is the original unresolved name, which will then fail at callTool with a less-descriptive MCP protocol error rather than a clear "tool not found" message. The CLI direct path (tool-call.ts:200) has the same pattern. This was pre-existing behavior but is now subtly different — previously the unresolved name was always sent; now it's only sent when resolution fails. The error message from the MCP server may be confusing.


Minor: dry-run in daemon path uses unresolved tool name in preview

src/cli/commands/tool-call.ts:116-126

The dry-run preview in the daemon path shows parsed.value.toolName (the user-supplied name), not the resolved name. The direct path has the same issue. This means the preview shows list_workflows but the actual call would use n8n_list_workflows. Not a bug per se, but could confuse users.


Summary: The security issue (access control bypass via unresolved name) is the most important thing to address. The pagination gap in getToolSchema is also worth fixing for correctness. Everything else is minor polish.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant