fix(tools): don't drop a toolset from platform inference when a tool is registered into it - #56480
designnotdrum wants to merge 2 commits into
Conversation
…orm inference Adds include_registry=True kwarg to resolve_toolset/get_toolset. When False, returns only the static TOOLSETS view with no registry-merged tools — the composite-authored membership platform reverse-mapping must compare against. Default True preserves all existing behavior; this is the enabling half of the api_server toolset-drop fix (NousResearch#49622). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…is registered into it _get_platform_tools reverse-maps a platform composite to configurable toolsets with an all-tools subset test. Because get_toolset() merges registry-registered tools into a toolset, a tool added to a toolset (delegate_cli -> delegation; desktop-only read_terminal -> terminal) that the static composite never listed made the subset test fail, silently dropping the entire toolset on api_server and other inference-based platforms. Compare the toolset's static membership at all three reverse-map sites. Fixes NousResearch#49622. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…stry-derived aliases too Follow-up on the #56480 salvage: the include_registry=False docstring said None is returned only for registry/MCP-only toolsets; it also applies to registry-derived aliases, which have no static TOOLSETS counterpart.
…vage Attribution audit gate: the salvaged contributor commits carry kiljadn@gmail.com (Nick Mason / @designnotdrum). Add the mapping so contributor_audit.py resolves the author on this PR.
|
Merged via #56887 — your commits were cherry-picked with authorship preserved (both Thanks for the clean root-cause fix — solving this at the reverse-map layer ( Closing this in favor of the salvage PR. Appreciate the contribution! |
…is registered into it (salvage NousResearch#56480) (#722) Co-authored-by: qbit-mirror-bot <qbit-mirror-bot@users.noreply.github.com>
…stry-derived aliases too Follow-up on the NousResearch#56480 salvage: the include_registry=False docstring said None is returned only for registry/MCP-only toolsets; it also applies to registry-derived aliases, which have no static TOOLSETS counterpart.
…ch#56480 salvage Attribution audit gate: the salvaged contributor commits carry kiljadn@gmail.com (Nick Mason / @designnotdrum). Add the mapping so contributor_audit.py resolves the author on this PR.
…stry-derived aliases too Follow-up on the NousResearch#56480 salvage: the include_registry=False docstring said None is returned only for registry/MCP-only toolsets; it also applies to registry-derived aliases, which have no static TOOLSETS counterpart.
…ch#56480 salvage Attribution audit gate: the salvaged contributor commits carry kiljadn@gmail.com (Nick Mason / @designnotdrum). Add the mapping so contributor_audit.py resolves the author on this PR.
…stry-derived aliases too Follow-up on the NousResearch#56480 salvage: the include_registry=False docstring said None is returned only for registry/MCP-only toolsets; it also applies to registry-derived aliases, which have no static TOOLSETS counterpart.
…ch#56480 salvage Attribution audit gate: the salvaged contributor commits carry kiljadn@gmail.com (Nick Mason / @designnotdrum). Add the mapping so contributor_audit.py resolves the author on this PR.
…stry-derived aliases too Follow-up on the NousResearch#56480 salvage: the include_registry=False docstring said None is returned only for registry/MCP-only toolsets; it also applies to registry-derived aliases, which have no static TOOLSETS counterpart.
…ch#56480 salvage Attribution audit gate: the salvaged contributor commits carry kiljadn@gmail.com (Nick Mason / @designnotdrum). Add the mapping so contributor_audit.py resolves the author on this PR.
…stry-derived aliases too Follow-up on the NousResearch#56480 salvage: the include_registry=False docstring said None is returned only for registry/MCP-only toolsets; it also applies to registry-derived aliases, which have no static TOOLSETS counterpart.
…ch#56480 salvage Attribution audit gate: the salvaged contributor commits carry kiljadn@gmail.com (Nick Mason / @designnotdrum). Add the mapping so contributor_audit.py resolves the author on this PR.
…stry-derived aliases too Follow-up on the NousResearch#56480 salvage: the include_registry=False docstring said None is returned only for registry/MCP-only toolsets; it also applies to registry-derived aliases, which have no static TOOLSETS counterpart.
…ch#56480 salvage Attribution audit gate: the salvaged contributor commits carry kiljadn@gmail.com (Nick Mason / @designnotdrum). Add the mapping so contributor_audit.py resolves the author on this PR.
What does this PR do?
On inference-based platforms —
api_server(the OpenAI-compatible HTTP surface used by Open WebUI) and plugin platforms with no explicitplatform_toolsetsentry —_get_platform_tools()reverse-maps the platform's static composite (e.g.hermes-api-server) back to configurable toolsets using an all-toolsset.issubset()test.get_toolset()merges registry-registered tools into a toolset's resolution, soresolve_toolset("terminal")grows to include the desktop-onlyread_terminalafter discovery, andresolve_toolset("delegation")grows once a plugin registers a tool intodelegation. Platform composites enumerate static tool names and gain nothing from the registry. So the moment a tool is registered into a toolset that the static composite never listed, the subset test fails and the entire toolset is silently dropped — no log, no error. On a defaultapi_serverthis dropsterminal(issue #49622).The fix compares each candidate toolset's static (pre-registry-merge) membership during reverse-mapping, via a new
include_registryflag onresolve_toolset()/get_toolset(). Registry-added tools stop gating toolset inference but remain exposed (subject to their owncheck_fn) once the toolset is enabled — matching how explicitly-configured platforms (cli/slack/gateway) already behave. This fixes the class at the reverse-map layer rather than data-patching one composite at a time.Related Issue
Fixes #49622
Supersedes #49626 (which adds
read_terminalto thehermes-api-servercomposite — one symptom). This PR fixes the underlying reverse-map, so no composite needs per-tool maintenance; the composite is left unchanged.Out of scope (distinct root causes, not this class): #35527 (discord platform-name collision with
_TOOLSET_PLATFORM_RESTRICTIONS+_DEFAULT_OFF_TOOLSETS) and #55793 (pluginknown_plugin_toolsetsdiscoverability, framed by its reporter as docs + a log breadcrumb, not a behavior change). Left for separate follow-ups.Type of Change
Changes Made
toolsets.py— addedinclude_registry: bool = Truekwarg toget_toolset()andresolve_toolset(); threaded through recursion, theall/*alias, includes, and thehermes-<plugin>fallback.include_registry=Falsereturns the staticTOOLSETSview (no registry-merged tools).hermes_cli/tools_config.py— at the three reverse-map candidate comparisons in_get_platform_tools()(no-explicit-config branch,has_explicit_configmixed-composite expansion, and the non-configurable recovery loop), resolve the candidate toolset withinclude_registry=False. Universes stay registry-merged so a platform never under-counts what it actually provides.tests/test_toolsets.py— behavior-contract tests for theinclude_registryflag (static view excludes registry extras, threads through includes,allalias, registry-only toolset → empty).tests/gateway/test_api_server_toolset.py— regression tests: defaultapi_serverkeepsterminal([Bug]: API server default toolset drops terminal/process after terminal toolset includes desktop-only read_terminal #49622); registering a tool into an existing toolset does not drop that toolset from inference; default-off / platform-restricted toolsets stay off.How to Test
_get_platform_tools({}, "api_server")now containsterminal(previously dropped because desktop-onlyread_terminalis registered into theterminaltoolset but absent from thehermes-api-servercomposite).api_serverinference (covered bytest_registering_tool_into_toolset_does_not_drop_toolset_from_inference).homeassistantwithoutHASS_TOKEN,x_searchwithout creds) and platform-restricted (discord) toolsets remain off onapi_server.scripts/run_tests.sh tests/test_toolsets.py tests/gateway/test_api_server_toolset.py tests/hermes_cli/test_tools_config.py -qVerified: the two new regression tests fail without the
tools_configchange and pass with it; the negative-contract test passes both ways; the toolset/platform/model_tools/coding/cli/cron/web_server test surface stays green.Checklist
Code
fix(tools): ...)Documentation & Housekeeping
get_toolset/resolve_toolset) — or N/Acli-config.yaml.exampleif I added/changed config keys — N/A (no config keys)CONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/A