Skip to content

[codex] fix(mcp): resolve toolsets from live registry - #9849

Closed
g-guthrie wants to merge 3 commits into
NousResearch:mainfrom
g-guthrie:codex/mcp-toolset-registry-pr2
Closed

g-guthrie wants to merge 3 commits into
NousResearch:mainfrom
g-guthrie:codex/mcp-toolset-registry-pr2

Conversation

@g-guthrie

@g-guthrie g-guthrie commented Apr 14, 2026 •

Copy link
Copy Markdown

What changed

This PR removes the MCP-side _sync_mcp_toolsets() path and stops mutating toolsets.TOOLSETS at runtime.

Instead:

  • MCP tools continue to register under their canonical mcp-<server> toolset in the live registry
  • raw server names such as github or fs are now explicit registry aliases for those canonical MCP toolsets
  • MCP shutdown and dynamic refresh now cleanly remove stale tool registrations and aliases by deregistering the canonical toolset entries

Why

The old flow had two sources of truth for MCP toolsets:

  • the live tool registry
  • runtime mutations to TOOLSETS

That created a real stale-state problem. When an MCP server refreshed its tool list via notifications/tools/list_changed, the registry was updated, but raw server-name toolset aliases depended on a separate _sync_mcp_toolsets() mutation path. It also meant MCP cleanup relied on manually injecting and scrubbing tools from unrelated hermes-* toolsets.

This PR makes the ownership boundary explicit:

  • the registry owns MCP tool membership and aliases
  • toolsets.py resolves static toolsets plus live registry-backed aliases
  • deregistering the last tool in an MCP toolset automatically removes its alias

Impact

  • raw MCP server names resolve from explicit live registry metadata instead of string-prefix inference
  • built-in toolset names still win on collisions, while mcp-<server> remains available
  • MCP refresh no longer needs to scrub or re-inject tools into hermes-* toolsets
  • MCP shutdown now removes stale tool registrations and aliases instead of leaving dead registry state behind
  • the change stays scoped to MCP/toolset resolution without touching CLI or gateway runtime flow

Validation

Scoped validation that covers the changed subsystem:

  • python -m pytest tests/test_toolsets.py tests/tools/test_mcp_dynamic_discovery.py tests/tools/test_mcp_tool.py tests/test_model_tools.py tests/hermes_cli/test_tools_config.py -q

Result: 238 passed

Representative attribution check for broader repo reds:

I re-ran a subset of the failing full-suite tests on a clean origin/main worktree in the same local environment. These failures reproduce there too, so they are not introduced by this PR:

  • tests/agent/test_auxiliary_client.py::TestReadCodexAccessToken::test_missing_returns_none
  • tests/hermes_cli/test_gateway_wsl.py::TestSupportsSystemdServicesWSL::test_native_linux
  • tests/tools/test_file_staleness.py::TestStalenessCheck::test_warning_when_file_modified_externally

I did not widen this PR to chase those unrelated failures.

@g-guthrie
g-guthrie marked this pull request as ready for review April 14, 2026 20:46
@nidhishgajjar

Copy link
Copy Markdown

Orb Code Review (powered by GLM 5.1 on Orb Cloud)

PR #9849 — fix(mcp): resolve toolsets from live registry

What it does

Replaces the pattern of mutating the static TOOLSETS dict at runtime with live registry-based toolset resolution. Adds a register_toolset_alias() mechanism to ToolRegistry and refactors get_toolset(), resolve_toolset(), validate_toolset() to query the live registry instead of a mutated dict.

Observations

✅ Architectural improvement
The previous approach of mutating TOOLSETS at runtime was fragile — it created race conditions with concurrent MCP server registration and made cleanup unreliable. The new alias-based approach is cleaner and properly scoped to the registry.

✅ Clean deregistration
shutdown() now deregisters tools and their aliases, and deregister() automatically cleans up aliases when the last tool in a toolset is removed. This prevents stale state after MCP server disconnections.

✅ Good test refactoring
Tests now assert against the live registry resolution (validate_toolset, resolve_toolset) rather than checking dict mutation, which better matches the new architecture.

⚠️ Minor: sorted() return values
resolve_toolset() and resolve_multiple_toolsets() now return sorted(all_tools) instead of list(all_tools). This changes the ordering from insertion-order to alphabetical. While generally beneficial for determinism, if any downstream code relies on insertion order (e.g., tool priority), this could subtly change behavior.

⚠️ Minor: for...else in get_toolset_names()

for alias, canonical in aliases.items():
    if canonical == ts_name and alias not in TOOLSETS:
        names.add(alias)
        break
else:
    names.add(ts_name)

The for...else pattern is valid Python but often confuses readers. A helper function or a simple any() check might be clearer.

Summary: Solid architectural improvement that eliminates runtime TOOLSETS mutation in favor of live registry lookups. Clean implementation with proper cleanup semantics. Minor notes about sorted ordering change and readability.

Assessment: approve

@teknium1

Copy link
Copy Markdown
Collaborator

Merged via PR #9947. Your commits were cherry-picked onto current main with your authorship preserved in git log. Thanks for the clean refactoring — single source of truth for MCP toolset membership is the right call.

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.

3 participants