Conversation
AI review — feat(plugins): session-bound slash command contextSummary: Introduces immutable Strengths
Non-blocking
Verdict: Non-blocking. Nice isolation design. |
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head 905f9e88391053387d5ea95d55f1e6a31813d4e7 against current main@ead7e91dabf1e963796ec834b196984a2fa44ff4 and the live repository graph. I walked all 10 changed paths, the real CLI/Gateway/TUI/Desktop dispatch sites, PluginInvocation's fail-closed checks, the invocation→invoke_tool execution path, slash-policy canonicalization, lifecycle/observability forwarding, the focused tests, the existing PR discussion, and the earlier context-command proposals.
The core direction is good: an immutable, capability-minimized invocation object is a materially better narrow waist than handing plugins live gateway/runtime objects, legacy one-argument handlers remain compatible, and the session-effective tool ceiling is checked before dispatch. The Gateway also correctly tries to close the same separate-dispatch authorization class that #44727 exposed. I found two landing blockers on the other side of those boundaries:
-
P1 — Gateway tool execution can block the async inbound loop.
PluginInvocation.dispatch_tool()is synchronous, a synchronous plugin handler is invoked inline inside the async command dispatcher, and the Gateway_dispatchclosure calls synchronousinvoke_tool()directly. That means the new first-class capability can run terminal/browser/network/approval-wait work on the Gateway event-loop thread and stall unrelated chats/platform handling. The host boundary needs an async-aware/offloaded execution contract that preserves the session/approval/runtime context across the handoff, plus a real responsiveness regression. -
P1 — plugin command identity is normalized for lookup but not for authorization. The handler lookup converts Telegram-style underscores to hyphens, while
_check_slash_accessreceives the raw typed command.SlashAccessPolicycompares exact command names and its config normalizer only strips/and lowercases. So a non-admin explicitly allowedmy-pluginis still denied when Telegram delivers/my_plugin. One canonical plugin-command identity needs to feed lookup, policy, reporting, and tests.
There is also important provenance/merge-order context. #56782 is an earlier adjacent proposal for context-aware Gateway plugin commands (including live event/gateway/source objects and busy-session bypass). #72019 is a later complementary, more bounded proposal carrying authenticated actor metadata plus a control-plane/human-intervention bridge. This PR is not a wholesale duplicate: its session-bound tool capability, TUI/Desktop binding, and lifecycle projection are distinct and stronger in several respects. But all three occupy the same hermes_cli.plugins/Gateway command seam, so if #106467 becomes the carrier, please preserve the earlier contributor provenance and deliberately restack/consolidate the residual actor/control-plane work rather than leaving three incompatible command-context APIs alive.
Current-state note: this one-commit branch is now 1 ahead / 38 behind live main. The intervening main changes touch conversation_loop.py and cli.py, so the final carrier should be rebased and re-verified after these two fixes. The PR reports 126 focused local tests passing, but exact-head GitHub CI, Docker, and Nix are all currently action_required; therefore the hosted commit train is 0/1 established green and I would not call it land-ready yet.
This is substantial, careful extension work on a sensitive plugin boundary. Close the async execution boundary and the canonical command-identity boundary, reconcile the overlapping lineage, and this can be a strong common carrier. 🚀
| try: | ||
| # Task id mirrors the identity the agent loop uses for terminal/browser scoping. | ||
| task_id = getattr(agent, "session_id", "") or quick_key or "" | ||
| return invoke_tool(agent, name, args, task_id) |
There was a problem hiding this comment.
P1 — keep plugin tool execution off the Gateway event loop. This closure is reached from the async inbound command path, but PluginInvocation.dispatch_tool() is synchronous and sync plugin handlers are called inline before only coroutine results are awaited. A plugin calling dispatch_tool() can therefore run a blocking invoke_tool() (terminal/browser/network work or an approval wait) on the event-loop thread and stall unrelated Gateway sessions. Please make this capability async-aware or offload it at the host boundary while preserving the session cwd/approval context, and add a regression where a deliberately blocked tool is in flight while an independent event-loop heartbeat still advances.
| # them; apply the same admin/user policy to the raw typed name here (mirrors the | ||
| # quick-command gate above; #44727). The policy stays disabled until an operator | ||
| # lists an admin for the scope, so existing installs see no behavior change. | ||
| _denied = self._check_slash_access(source, command) |
There was a problem hiding this comment.
P1 — use the same canonical command identity for authorization as for lookup. The line above resolves command.replace("_", "-"), but this check receives the raw typed command. SlashAccessPolicy.can_run() performs exact membership and the configured allowlist normalizer only strips / + lowercases, so a Telegram autocomplete invocation like /my_plugin finds registered my-plugin but is denied for a non-admin explicitly allowed my-plugin. Compute plugin_name once and use it for handler lookup, _check_slash_access, and reporting; please pin this with a hyphenated registration/allowlist + underscored inbound-command regression.
|
Closing as superseded by #118845 (#118845), commit Thanks for the contribution — the underlying problem this PR addresses has been resolved on current main. If you believe this was closed in error, please comment and we'll reopen. |
Scope
Adds a generic, backward-compatible plugin command invocation context. It contains immutable session provenance and only a session-bound tool dispatcher that uses the normal agent tool path. Existing
handler(raw_args)commands remain unchanged; opt-in handlers accepthandler(raw_args, *, invocation=None).The change also binds CLI, Gateway, and TUI/Desktop command dispatch, applies the Gateway slash-access gate to plugin commands, and supplies additive session-start lifecycle context for plugins that need a pre-turn baseline.
Validation
scripts/run_tests.sh tests/hermes_cli/test_plugins.py tests/hermes_cli/test_plugin_invocation.py tests/gateway/test_plugin_command_slash_access.py tests/gateway/test_unknown_command.py tests/gateway/test_slash_access_dispatch.py tests/gateway/test_discord_slash_commands.py tests/tui_gateway/test_plugin_invocation.py tests/tui_gateway/test_compress_lock_skip.pyNo
/commitor Git-specific behavior is added to core.