fix(matrix): batch long commands and bypass active session for skill commands - #58565
fix(matrix): batch long commands and bypass active session for skill commands#58565liuhao1024 wants to merge 1 commit into
Conversation
…commands Matrix bang commands that normalize to /command are MessageType.COMMAND, which skipped the text-batching buffer. Long commands near _SPLIT_THRESHOLD now go through batching so continuation chunks are aggregated. Skill commands (e.g. /arxiv) are resolved through a separate registry and were not recognized by should_bypass_active_session(), causing them to be queued as user text instead of dispatched directly. Fixes NousResearch#58559
Related: fixes #58559 (Matrix bang-command batching + skill-command active-session bypass). Same Matrix bang-command family as merged salvage #38175 and docs PR #31533, but a distinct mechanism (split-threshold batching + active-session bypass, not |
|
Hi, thanks for jumping on #58559. I reported that issue and reviewed this PR against the repro plus a few adjacent Matrix/active-session cases. It fixes the long-command happy path, but I found a couple of gaps that seem worth covering before this is considered fully closed:
I opened draft follow-up/alternative #58591 with runner-level handling and extra regression tests. Happy for maintainers to use whichever PR shape is cleaner; I mainly wanted the extra edge cases captured so #58559 is fixed end-to-end. |
|
Hi bbopen, thanks for the thorough review and for testing against the real repro plus adjacent cases. You're right that the4 gaps you identified are valid:
Your #58591 looks like a more complete fix that addresses all four gaps at the runner level. Happy for the maintainers to choose whichever PR shape works best — if #58591 is preferred, this one can be closed. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing the verified Matrix split-command and active-session issues. The current implementation still has the reported premises (plugins/platforms/matrix/adapter.py:2770-2788; hermes_cli/commands.py:389-409), but this patch does not cover the full dispatch path.
Problems
gateway/run.py:9345-9351resolves only built-ins during an active run, and its busy catch-all is gated on_cmd_def_inneratgateway/run.py:9589-9602. This PR bypasses the adapter guard but does not make a skill command recognized by the runner.- The new skill lookup bypasses
resolve_skill_command_key()(agent/skill_commands.py:496-512), so underscore/hyphen equivalents such as/claude_codeand/claude-codediverge. - A slash-prefixed continuation is still classified as
MessageType.COMMAND(plugins/platforms/matrix/adapter.py:2772-2774) and dispatched separately by the new condition. Also,_enqueue_text_event()appends into the existing event without changing its type (plugins/platforms/matrix/adapter.py:3463-3473), so a long command can be merged into a pending text batch and lose command semantics.
Suggested changes
- Handle normalized skill/plugin commands in both active-session layers, then return the normal busy response.
- Keep command continuations with a pending long command batch and isolate a new long command from a pending text batch; add regressions for both.
Automated hermes-sweeper review.
| # Skill commands (e.g. /arxiv) are resolved through a separate registry | ||
| # and must also bypass the active-session queue, matching how | ||
| # _resolve_matrix_bang_command already checks both registries. | ||
| try: |
There was a problem hiding this comment.
Please resolve through resolve_skill_command_key() rather than an exact slash-prefixed lookup. Skill dispatch deliberately normalizes underscores to hyphens (agent/skill_commands.py:496-512), so /claude_code otherwise still fails the active-session bypass for /claude-code.
| if msg_type == MessageType.TEXT and self._text_batch_delay_seconds > 0: | ||
| should_batch = self._text_batch_delay_seconds > 0 and ( | ||
| msg_type == MessageType.TEXT | ||
| or (msg_type == MessageType.COMMAND and len(body) >= self._SPLIT_THRESHOLD) |
There was a problem hiding this comment.
This only batches the initial long command. A short split continuation beginning with / is classified as MessageType.COMMAND and will dispatch immediately instead of joining the pending long command; a pending text batch can also absorb this command and retain MessageType.TEXT in _enqueue_text_event().
|
Thank you, @liuhao1024, for the original fix and for the helpful coordination here. I rebuilt draft #58591 on current |
What does this PR do?
Fixes two Matrix-specific command dispatch bugs:
Long
!commandmessages bypass text batching: Matrix clients split long messages at ~3900 chars. When a!commandnormalizes to/command, the adapter setsMessageType.COMMANDand dispatches immediately, so the continuation chunk arrives as a separate plain-text message instead of being aggregated with the command body. Fix: batchCOMMANDmessages when the body length is at or above_SPLIT_THRESHOLD, while keeping short commands (e.g./stop,/status) immediate.Skill commands don't bypass active-session queue:
should_bypass_active_session()only checksresolve_command()(built-in commands). Registered skill commands like/arxivare resolved through a separate registry (get_skill_commands()), so they get queued as user text instead of being dispatched. This matches how_resolve_matrix_bang_command()already checks both registries.Related Issue
Fixes #58559
Type of Change
Changes Made
plugins/platforms/matrix/adapter.py: Change_handle_text_message()batching condition to also enqueueMessageType.COMMANDwhenlen(body) >= _SPLIT_THRESHOLD— short commands still dispatch immediately.hermes_cli/commands.py: Extendshould_bypass_active_session()to checkget_skill_commands()after the built-in command lookup, so skill commands bypass the active-session guard.tests/gateway/test_command_bypass_active_session.py: AddTestSkillCommandBypassclass with two tests verifying skill commands bypass and unknown names don't.tests/gateway/test_matrix_command_batching.py: New test file withtest_long_matrix_command_is_batchedandtest_short_matrix_command_dispatches_immediately.How to Test
Run the regression tests:
All tests should pass (42 existing + 2 new bypass tests + 2 new batching tests).
Reproduce the original issue with the script from Matrix bang commands bypass split batching and skill-command active-session dispatch #58559 on the patched tree — both checks should now return
True:Run the full related test suite:
Should pass: 58 tests.
Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/A