Skip to content

fix(tools): honor canonical MCP names in platform allowlists - #128141

Open
Wenfengcheng wants to merge 1 commit into
NousResearch:mainfrom
Wenfengcheng:fix/128017-mcp-platform-alias
Open

Wenfengcheng wants to merge 1 commit into
NousResearch:mainfrom
Wenfengcheng:fix/128017-mcp-platform-alias

Conversation

@Wenfengcheng

@Wenfengcheng Wenfengcheng commented Sep 29, 2026 •

Copy link
Copy Markdown

Problem

Fixes #128017.

A saved platform_toolsets.cli: [hermes-cli, mcp-linear] adds every enabled MCP server, whereas the bare alias linear correctly selects only that server.

Root cause

_merge_mcp_servers recognizes only enabled bare server names. Canonical mcp-<server> names fall through as custom entries and leave the explicit MCP allowlist empty, activating the default-all branch.

Change

Recognize both spellings in the existing merge function, preserving the caller's spelling. no_mcp also overrides canonical selections. No new configuration, discovery, or tools.

Verification

Frozen base: 5000e29936df69d5209f7cf2eea8e5776cb4cbb1.

  • Before production changes: scripts/run_tests.sh tests/hermes_cli/test_platform_mcp_allowlist.py -q --tb=short -j 1: 2 expected assertion failures, 6 passed (canonical selection broadens; canonical selection survives no_mcp).
  • After: same command: 12 passed. Includes bare/canonical spellings, default inclusion enabled/disabled, custom passthrough, portable-server selection and discovery failure fallback.
  • Native Windows, existing interpreter, isolated HOME/USERPROFILE/HERMES_HOME/APPDATA/LOCALAPPDATA before imports. No external MCP server or production state accessed.
  • Broader regression: scripts/run_tests.sh tests/hermes_cli/test_tools_config.py tests/hermes_cli/test_platform_mcp_allowlist.py -q --tb=short -j 1: 56 passed, 6 skipped. The first broader attempt lacked a source package in the sparse checkout; materializing the unchanged hermes_platform package resolved those setup errors.
  • Local integration harness: real temporary YAML -> load_config() -> _get_platform_tools() -> registered MCP resolve_toolset() output: PASS. Selected tool present, unrelated server absent, bare/canonical tool sets equal, and no_mcp excludes both; no MCP process spawned.
  • Independent exact-head Codex static review: PASS. Native Claude Code 2.1.280 / Opus 5.5 review remains unresolved: repeated service HTTP 503 followed by timeout, not a pass. No full-suite or live-provider claim.

Scope / non-goals

The issue's enabled-server platform allowlist contract is covered. Existing bare-alias behavior and default inclusion policy remain unchanged. Per-job cron allowlists (#108073), server-side per-platform scope (#125393), config validation (#127978), and explicit empty lists (#107452) have separate implementations and are not changed.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard tool/mcp MCP client and OAuth area/config Config system, migrations, profiles labels Sep 29, 2026
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference; please use your judgment.

The fix is right and I'd approve it. Verified against base 5000e299 (pre-image confirmed by resolve_base.py --recover).

It works. _merge_mcp_servers treats listed server names as an allowlist but tested membership against bare names only. A server's canonical toolset is mcp-<server> with the bare name as an alias — both user-facing per website/docs/reference/toolsets-reference.md:127,138, and tools/mcp_tool_registration.py:130 registers tools under mcp-<name> then aliases the bare name. So mcp-linear matched nothing, explicit_mcp_servers came back empty, the default-inclusion branch fired, and every enabled server was unioned in: the allowlist inverted into "all servers".Red on base, green on head: base source + your tests gives 4 failed / 8 passed, every failure an mcp-linear row while every bare-linear row passes on both sides. Head: 12 passed; with test_tools_config.py, 56 passed / 6 skipped.

One gap worth a follow-up — cron/scheduler.py:471-485 has the same bug. _merge_mcp_into_per_job_toolsets re-implements this allowlist bare-only, so a per-job enabled_toolsets of ["mcp-linear"] grants tools from every enabled server. With mcp_servers: {linear, github, notion}:

job=['linear']     -> ['linear']                                    github_leaked=False
job=['mcp-linear'] -> ['github', 'linear', 'mcp-linear', 'notion']  github_leaked=True

Same intent, opposite outcome — and user-reachable: tools/cronjob_tools.py:1123 exposes enabled_toolsets as a free-form array of toolset-name strings, so the agent writes mcp-<server> names there routinely. tests/cron/test_scheduler.py:100-111 covers only the bare spelling. Its docstring says it "Mirrors _get_platform_tools" — after this PR the mirror is broken.

Widen the membership test the same way:

mcp_names = enabled_mcp | {f"mcp-{name}" for name in enabled_mcp}
if set(result) & mcp_names:
    return result

Keep the union loop on bare enabled_mcp (matching _merge_mcp_servers). Canonical entries already in result pass through, so no_mcp and current tests keep their behavior. Better: share the membership test via a helper both sites call.

Not blocking: cron/scheduler_preflight.py:348-352 filters bare-only too, so its "resolves to zero tools" warning stays silent for the canonical spelling.

@Wenfengcheng

Copy link
Copy Markdown
Author

@Enough1122 Thanks for verifying the platform allowlist fix. I checked the proposed cron follow-up against the actual diff of the still-open #108073: it already implements canonical-name recognition in _merge_mcp_into_per_job_toolsets with per-job allowlist tests. That is a pre-existing implementation of the same trigger, failure and owner boundary, so I am deliberately not duplicating it in this PR. #128141 remains scoped to the platform allowlist; the per-job contract was already listed among its non-goals.

@JoaoMarcos44 JoaoMarcos44 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Wenfengcheng — I independently checked the reported selection mechanism and am documenting the bounded result here. This is a COMMENT review, not an approval or a request-changes verdict.

Scope and pins

Reviewed head: dc1d9229aa707c97cf6544bb8ad39f35aadc17a6; PR base: 5000e29936df69d5209f7cf2eea8e5776cb4cbb1. The executed production baseline was 20ae6087ffa5de9faf7e3b454b677a53a9e14856, not this PR's base. Native Windows, Python 3.14.7, pytest 9.1.1, using scripts/run_tests.sh.

Independently observed behavior

With three enabled synthetic MCP servers and platform_toolsets.cli: [hermes-cli, mcp-linear], the baseline platform resolver activates unrelated server names. I followed that through the real registry/alias machinery and model_tools.get_tool_definitions, rather than stopping at the config list: unrelated inert fixture schemas are model-visible. The bare linear selection exposes only the intended fixture schema.

Two contract assertions fail on the baseline. Replaying exactly the _merge_mcp_servers symbol from this head with the same baseline dependencies and the same schema-visibility contracts gives 2 passed, 0 failed. No MCP server was started, no handler was executed, and no credentials were used.

The controlling boundary is the bare-name-only intersection in the baseline _merge_mcp_servers, before the schema builder. Your change recognizes both spellings there while preserving the caller's spelling. This is preferable to a registry-only correction, which would arrive after the resolver had already widened the selection, or a save-time-only correction, which would miss existing/hand-written configuration.

Evidence limits and merge gates

This was an exact-symbol source probe, not your complete candidate-branch suite and not a full same-command branch RED/GREEN run. I inspected, but did not independently execute as a full branch suite, your portable-discovery, no_mcp, default-inclusion and disabled-server edge cases. The inspected MCP selection/schema functions have matching ASTs in subsequently observed main 79af3f6cea8067284a7ea5725078578b3f790adb; that is static identity evidence, not a test run on that later main.

Your existing PR owns the inspected platform fix; I am not proposing a duplicate or expanding it into the separate cron ownership boundary in #108073. The local result supports this mechanism, but full branch/current-base verification and the required CI/review gates remain separate conditions. An empty check rollup means no checks reported, not that CI never ran or passed.

This is a confirmed functional capability-selection defect in the baseline with security relevance. In-process tool selection is not OS containment under SECURITY.md, and schema visibility is not proof of credential use, exfiltration, or an official vulnerability.

This branch has not been deployed

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

Labels

area/config Config system, migrations, profiles comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists tool/mcp MCP client and OAuth type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

platform_toolsets: an mcp-<server> entry enables every MCP server instead of narrowing to that one

4 participants