fix(plugins): register deferred platform client tools at discovery (#78050) - #78842
Conversation
|
Flagging that #78538 targets the same issue — agreeing on the loading contract seems more useful than two competing shapes. On cost, in fairness to #78538: I measured the marginal adapter import at ~17 ms in a realistic startup (612 ms discovery → 629 ms). That's small, and not a reason to prefer this PR. The distinction is the contract for future plugins. #78538's rule is "declares tools → eager-load the whole plugin." a2a's adapter is stdlib-only so that's nearly free today, but a platform plugin shipping client tools alongside a heavy SDK would import that SDK on every Suggested hybrid: use #78538's |
|
Hey — I'm the author of #79432, which was flagged as overlapping this one. Fully agree the loading contract question should settle on one shape, so I've split my PR to be a2a-only and I'm standing down on the mechanism itself. What I'd offer into whichever PR wins (yours or #78538):
Also: my PR adds No hard feelings either way — if you'd rather I close #79432 so the field is clear, say the word. Goal is the fix landing. |
|
Appreciated — standing down on a mechanism you'd already built is the least fun thing to do in this situation, and it's the right call for getting #78050 closed. Taking you up on the offer, with one correction and one genuine addition. On pieces 1–3: this PR already has them. I think the comparison was against #78538 — that's accurate there, but not here. Concretely, in
Not a turf point — two people independently landing on module reuse and attribution as the two things that need handling is decent evidence the shape is right. The So the trigger is now the manifest field and Credit for the field goes to @Tranquil-Flow in #78538 — this is their half of the hybrid, and it's the better signal. On closing #79432 — yes please, if you're still happy to. With the trigger adopted here, all three PRs now point at one shape, and that's the most useful thing a maintainer can walk into. Same offer in reverse: if they prefer #78538's eager-load contract instead, I'll close this one and the module-reuse/attribution pieces should be folded there, because #78538 genuinely does lack them. Thanks for splitting out #79479 too — that one stands on its own. |
|
Validated this PR on Linux (Fedora 44, Python 3.11.15) in a clean worktree with its own venv and editable install — your live install was not touched. Results:
This corroborates the macOS / Windows 11 / NixOS reproductions from the thread. Confirms the fix is platform-independent in practice as claimed. Two minor notes:
Happy to test the final merged version against a live cross-host A2A link once it lands. |
fb97ec5 to
76c9602
Compare
|
Re-validated the latest head ( Results:
The diagnosable-failure change is a good catch — a silent failure here would have reproduced the exact #78050 symptom with no thread to pull on. Confirmed clean on Linux alongside the earlier macOS / Windows 11 / NixOS reproductions. Still no CI on the branch (fork workflows unapproved). With positive validation on four OSes across two commits, worth asking a maintainer to approve the checks. |
monerostar
left a comment
There was a problem hiding this comment.
Native Win11 check (monerostar) — still needed on this box
Host: Windows 11 build 26200 · tech profile · install %LOCALAPPDATA%\hermes\hermes-agent
PR tip: 76c960296
Gap still on main
plugins/platforms/a2a/plugin.yamlhas noprovides_toolsPluginManagerhas no_register_deferred_platform_toolshermes -p tech tools list→ a2a count 0 (peers are configured undera2a_agents; inbound gateway works; CLI/TUI outbound client tools do not show up)
So issue #78050 is still live here: deferred platform plugin never registers the five client tools outside a gateway process.
PR
Opt-in is explicit (provides_tools on the a2a manifest) + discovery imports only tools.py for those names. Inbound adapter stays deferred.
pytest tests/hermes_cli/test_deferred_platform_client_tools.py -o addopts=
# 11 passed
Includes checklist coverage that a2a appears in the hermes tools list path without materializing the heavy platform adapter.
Looks good. This is the PR that matches how we actually use A2A from desktop/CLI on a multi-host fleet.
|
Thanks @monerostar — that's a genuinely useful review, and not only because it's positive. Your run is currently the only execution of this code by anyone but me. No CI has fired on this branch: it's from an outside-contributor fork, so the workflows need a maintainer to approve them before any checks execute. The PR shows zero checks for that reason, not because something failed. So a native Win11 run on a different install, against The two things you confirmed that matter most:
Since your review I've pushed Also worth recording here: #79432 was closed by its author in favour of this PR, and #78538's |
|
Re-validated the latest head ( Results:
The |
Changed since @monerostar's reviewThat review was against
|
…ousResearch#78050) Rebased onto current main. `hermes_cli/plugins.py` grew 103KB -> 265KB across 49 commits since the original branch point, and the attribution mechanism this change hooks into was replaced along the way: the `_tools_before` / `_plugin_tool_names` snapshot diff is now a registration ledger sliced from `registration_start`, and `_plugin_id` is `plugin_key`. Re-anchored accordingly: - Discovery-time pre-registration, module reuse, and the `provides_tools` opt-in are unchanged. - Attribution credits `_predeclared_tools` ahead of the ledger slice, since those tools registered before `registration_start` and the slice cannot see them. - A failed materialization no longer carries attribution across. The failure path now sweeps the whole ownership ledger for the plugin key, not just the `registration_start:` slice, so the pre-registered tools are disposed along with the adapter. Attribution and the registry now agree at zero instead of reporting tools the process is not serving. tests/hermes_cli/test_deferred_platform_client_tools.py 13/13. test_plugins.py, test_plugins_cmd_list.py, test_plugin_cli_registration.py 65/65. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
6543f91 to
22b4da5
Compare
Rebased onto current main — force-pushedHeads up @cadamec @monerostar: the branch has been rebuilt from current main, so Why. The PR went What did not change. The shape is intact: What had to be re-anchored.
One deliberate behaviour change. I took that route rather than exempting pre-registrations from the sweep, on the grounds that arguing against a fresh upstream invariant is a bigger ask than this PR needs. The counter-argument is real though, and I would rather a maintainer make the call: those client tools work without the adapter — that is the entire point of #78050 — so wiping them when the adapter fails makes that path worse than the adapter-never-materializes path, which is the common CLI case. Happy to switch if that is preferred. Tests. Apologies that this invalidates the SHAs you both tested against. Re-validation would be welcome, but genuinely no obligation. |
…t_platform_tools (#81163) Layer 2 of the #81163 / #78050 fix: _get_platform_tools computed plugin_ts_keys = _get_plugin_toolset_keys() but only used CONFIGURABLE_TOOLSETS in the explicit-config filter, so a user-listed plugin key like `a2a` in `platform_toolsets.cli: [hermes-cli, a2a]` was silently dropped. The filter now unions configurable and plugin toolset keys when evaluating has_explicit_config and when admitting per-key entries. Cherry-picked from PR #81190 (Layer 2 hunks only; Layer 1 is covered by the provides_tools mechanism from PR #78842).
|
Merged via PR #86660 (#86660). Your commit — discovery-time client-tool registration behind the |
What does this PR do?
Fixes #78050 — the five A2A client tools (
a2a_call,a2a_discover,a2a_list,a2a_history,a2a_orchestrate) are invisible to CLI/TUI sessions. They never appear inhermes tools, cannot be opted into, and are absent from the session's toolset, while the same tools work in web/dashboard sessions on the same install.Root cause
A bundled
kind: platformplugin is registered as a deferred loader (_register_deferred_platform,hermes_cli/plugins.py) sohermes chatdoesn't pay ~20 platform-SDK imports. That deferral is correct and worth keeping.The problem is that the a2a plugin ships two independent things behind that one deferral, and its own
register()says so:Deferring the plugin defers both. In a CLI/TUI process the module never imports, so
register_tools()never runs:resolve_toolset("a2a")→[](the toolset is registry-derived and absent from staticTOOLSETS)a2ais missing from_get_effective_configurable_toolsets(), so thehermes toolschecklist has nothing to tickresolve_toolset("hermes-a2a")returns core tools but drops a2a's own tools — the bundle path reads the tool registry behind a deliberately cheapis_registered()check that doesn't materialize the platformGateway and web-server processes call
platform_registry.plugin_entries()/all_entries()at startup, which fires_resolve_all()and imports the module. That is the entire reason the tools exist there and not in the TUI.a2asits in_DEFAULT_OFF_TOOLSETSnext tohomeassistant,spotify,video_genandx_search. The comment above that set describes the intended contract — "Users who want it opt in viahermes tools". Every other member honours it;a2awas the sole outlier:The fix
Client tools that live in a dedicated
toolssubmodule are registered at discovery time. Importing<plugin>/tools.pydoes not import the adapter, so the platform SDK stays unloaded and the deferral keeps doing its job.This is the second shape the issue suggested ("registering client tools at discovery time independent of the platform adapter"), and it fixes every facet in one place rather than patching
resolve_toolset, the checklist, and the config-expansion path separately.Two supporting details keep the pre-import invisible:
_load_pluginreuses the already-imported package instead of executing its body a second time when the adapter is later materialized.hermes plugins listattribution survives materialization (_load_pluginattributes tools by diffing the registry aroundregister(), and pre-registered tools are already in the "before" snapshot).a2ais currently the only platform plugin with atools.py. Plugins without one are untouched and stay fully deferred; an import error intools.pyis caught and degrades to today's behaviour.Cost
The deferral invariant is intact — measured on this branch:
discover_plugins()resolve_toolset("a2a")For reference, the blanket alternative (
_resolve_all()in the toolset/checklist path) costs 862 ms — that's what this avoids.How to test
Reproduce on
mainin a plain CLI process (no gateway):mainprintsa2a toolset: []/in hermes tools: False. This branch prints all five tools andTrue, while the plugin staysdeferred=True.New regression tests:
They cover the reported symptom against the real a2a plugin (toolset resolves, checklist entry appears,
hermes-a2abundle includes the tools, adapter stays deferred) and the general mechanism with a synthetic platform plugin (adapter not imported, plugins withouttools.pyunchanged, package body executes exactly once across discovery + materialization, attribution preserved, brokentools.pydoesn't break discovery). 6 of the 8 fail onmain; the 2 that pass either way are the negative controls.Test results
tests/hermes_cli/test_plugins.py,test_plugins_cmd_list.py,test_plugin_cli_registration.py,test_startup_plugin_gating.py,test_plugin_auxiliary_tasks.py,test_plugins_tts_registration.py,test_plugins_transcription_registration.py,test_plugins_hub_perf_guard.py,tests/test_toolsets.py— 89 passed.tests/hermes_cli/sweep shows no new failures: the 6 that fail here (test_plugins_cmd.py::TestResolveSubdirWithin::test_rejects_symlink_escape,TestNoAutoActivation::test_compressor_default_ignores_plugin,test_codex_runtime_plugin_migration.py, 3×test_plugin_runtime_disable_gate.py) fail identically on unmodifiedmain— Windows symlink-privilege and cp1252-decode environment issues, unrelated to this change.Platform tested: Windows 11, Python 3.11.15. The mechanism is platform-independent (plugin discovery + import), and the change touches no file I/O, process management, or path handling beyond an
is_file()existence check.Relationship to #57063
Open PR #57063 describes the same class of problem for eight other platforms, but it is about
hermes-<platform>inbound bundles —resolve_toolset("hermes-a2a")already returns core tools today. This issue is the separate outbounda2aclient toolset, which merged in #77109 a month after that PR was last touched (2026-07-15) and is not covered by it. That review thread also pushed back on requiring a statictoolsets.pybundle for every platform plugin; this PR deliberately takes the registry-aware route instead.🤖 Generated with Claude Code