Add invocation-scoped context and availability for plugin slash commands - #108645
shannonsands wants to merge 4 commits into
Conversation
alt-glitch
added
type/feature
Related: open community PRs for the same plugin slash-command context need -- #106467 (bind slash commands to session context), #93901 (inject event context into handlers), #51596 (pass gateway context), #104590 (centralized authenticated dispatch + PluginCommandContext) -- and feature requests #51555 / #69882. Linking so reviewers can decide which of those this supersedes. |
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Review — invocation authority is strong; route ownership still has one fail-open edge
Reviewed exact head 2ad6e3190d1558760d495f0b72e391aa2404a73f against the current plugin command paths, the two surviving commits, hosted CI, existing review state, current main, and overlapping plugin-context work.
The core authority model is substantially better than ambient/session-global context: the host creates the invocation, chat/thread identity stays distinct, availability must return literal True, async/throwing/expired checks fail closed, the context is revoked after discovery/dispatch, and supervised plugin background tasks explicitly clear inherited invocation authority. Those are the right invariants.
I found one landing blocker in command routing, though. An availability denial currently relinquishes ownership of the slash-command name. register_command() only prevents collisions with built-in registry commands; a plugin command can still have the same name as a skill or bundle. On CLI, _get_plugin_cmd_handler_names(invocation) removes a registered-but-unavailable plugin, then _process_unregistered_slash() immediately tries the same name as a bundle/skill. TUI command.dispatch has the same load-bearing quick > plugin > bundle > skill fallthrough: _dispatch_plugin() returns None when availability denies, so the lower-precedence resolvers receive the request. The messaging Gateway has the same shape: _hm_dispatch_quick_and_plugin_commands() returns unhandled after a denied plugin, then _hm_skill_slash_rewrite() gets a chance to turn that same slash name into model-facing skill input.
That contradicts the PR's explicit contract that an unavailable registered command does not fall through into model dispatch. The gate proves "this caller may not invoke this registered route"; it cannot safely mean "try another authority owner with the same spelling." The useful distinction already exists in is_plugin_command_registered(): keep registered-name ownership separate from availability. If the name is registered but get_plugin_command_handler(name, invocation) denies it, terminate as unavailable/unknown rather than continuing to bundle/skill/model resolution. Please pin this with adversarial regressions on all three surfaces: registered /secure + availability False + same-named skill/bundle, asserting neither fallback nor model execution occurs.
Interlocks / ownership
- #106467 (etelyatn) is direct overlapping prior work in the same public
hermes_cli/plugin_invocation.pycontract. It carries a session-bounddispatch_toolcapability andon_session_startlifecycle context that this PR explicitly leaves out of scope. I would treat #108645 as the stronger/newer carrier for invocation identity + availability, but not silently discard #106467: if its tool-dispatch/lifecycle pieces are still wanted, preserve that authorship and reconcile them into one invocation API rather than landing two incompatible public context objects. - #96179 (jc214-fullstack) is adjacent, not duplicate: its primary ownership is gateway injection settlement, but its task-local
ctx.call_contextexposes overlapping host-bound profile/session/thread/user identity. These should converge on one identity vocabulary rather than creating two plugin-facing provenance surfaces. - #94266 was merged and then explicitly reverted from
mainas the in-tree Collective Wisdom implementation. This PR is therefore best understood as a generic host-contract extraction for an external consumer, not as restoring the reverted product logic. That separation is good and should stay explicit.
Exact-object evidence
The branch is currently 2 commits ahead / 49 behind live main@bf51fee548ceaa28d2c299a41c0f65066c04c061 from merge base 31d0a2428e9db346d6781da66f5b37ff3e12def2. The first surviving commit 4813581799e3306a4b258accf146b99b14915254 has no associated hosted PR workflow runs. Exact head has Docker 34655514722 and Nix 34655514721 green, but CI 34655515301 is red: Python tests / Run tests failed and the required-check aggregate failed. I could verify the failing job state but not retrieve its log payload through the connected API, so I am not guessing at the specific test failure.
That leaves 0/2 surviving commits with complete hosted-green exact-object proof. After the routing fix, the reconciled current-main object still needs exact CI + Docker + Nix evidence, with every surviving commit green before this is done.
This is a good authority primitive and a much cleaner boundary than letting plugins infer identity from ambient state. The remaining defect is narrow but important: denial must be terminal for the route identity that was denied, not permission to reinterpret the same spelling under a different owner.
| try: | ||
| if bare in _get_plugin_cmd_handler_names(plugin_invocation): | ||
| self._run_plugin_slash_command(base_cmd, user_args, plugin_invocation) | ||
| elif base_cmd in skill_bundles: |
There was a problem hiding this comment.
P1 — a denied registered plugin route can be reinterpreted as a lower-precedence command owner. _get_plugin_cmd_handler_names(plugin_invocation) contains only available plugins, so when /secure is registered but its availability predicate returns False, this branch is reached if a same-named bundle exists (and the next branch does the same for a skill). The TUI and messaging Gateway have the equivalent plugin→bundle/skill fallthrough. That breaks the PR's fail-closed contract: availability should deny the registered route, not release the spelling for reinterpretation. Please distinguish is_plugin_command_registered(bare) from get_plugin_command_handler(..., invocation): registered + unavailable should terminate as unavailable/unknown, with regressions proving same-named skill/bundle/model paths never run on CLI, TUI RPC, or Gateway.
|
Verified current head c4032ae: CI run https://github.com/NousResearch/hermes-agent/actions/runs/34656724622 completed successfully, including the required-check aggregator, Linux tests (47,564 passed, zero failed, 455 skipped), and Windows/macOS lanes. The old command-rewrite mock failure is resolved. This is CI evidence for the generic command slice, not live Wisdom acceptance or completion of tool/account/idle/post-delivery interfaces. PR remains draft. |
|
Not required |
3 participants
Scope
Draft generic host command API slice for external plugins. No Wisdom business logic is added to Agent. This does not yet provide the full tool/lifecycle/account interface needed by the external plugin.
Changes
Verification
At c4032ae, the integrator reran 102 focused command/context tests successfully. Pinned Ruff and diff checks passed. Local tests use Python 3.12 with isolated test tooling and read-only runtime dependencies from a reference environment; this is not a clean locked host installation.
Earlier full Linux CI at 2ad6e31 passed 47,549 tests with one failure caused by an old-signature mock in the command rewrite test. That fixture now exercises real plugin registration with allowed and denied rewrites; its full file passes. New-head full CI is tracked by this PR and must be checked independently.
Still pending
Plugin-facing account/entitlement/request facade; tool and subagent invocation bindings; actual delivery facts, idle lifecycle, fencing and typed continuation events; live native acceptance. Static platform command publication has no user context and therefore hides gated commands. This PR is deliberately draft while the host milestone is incomplete.