Skip to content

fix(cli): register send subcommand + add encoding on read_text (#27188 follow-up) - #27258

Closed
briandevans wants to merge 1 commit into
NousResearch:mainfrom
briandevans:fix/hermes-send-baseline-followup-27188
Closed

fix(cli): register send subcommand + add encoding on read_text (#27188 follow-up)#27258
briandevans wants to merge 1 commit into
NousResearch:mainfrom
briandevans:fix/hermes-send-baseline-followup-27188

Conversation

@briandevans

Copy link
Copy Markdown
Contributor

Summary

The recently-merged feat(cli): add hermes send (#27188, commit 29b1bd0e2) landed two small regressions that are blocking CI on origin/main:

  1. hermes_cli/main.py::_BUILTIN_SUBCOMMANDS doesn't include "send", so tests/hermes_cli/test_startup_plugin_gating.py::test_builtin_set_covers_every_registered_subcommand fails on every PR.
  2. hermes_cli/send_cmd.py:61 calls Path(file_path).read_text() without an explicit encoding= argument, tripping ruff PLW1514 (which is the blocking ruff job).

Both are mechanical follow-ups directly directed by the failing checks.

The bug

1. Missing "send" in _BUILTIN_SUBCOMMANDS

The parity test in test_startup_plugin_gating.py fails with the exact message:

AssertionError: _BUILTIN_SUBCOMMANDS is missing these live subcommands: ['send'].
Add them to hermes_cli/main.py::_BUILTIN_SUBCOMMANDS so plugin discovery
can be skipped when the user targets them.

Correctness is unaffected (missing entries only force unnecessary plugin discovery for users running hermes send …), but the parity test guards the perf invariant and has to stay green for CI.

2. Missing encoding= on read_text()

The blocking ruff enforcement job fails with:

error[PLW1514]: `pathlib.Path(...).read_text` without explicit `encoding` argument
  --> hermes_cli/send_cmd.py:61:20
   |
61 |             return Path(file_path).read_text()
   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^

The lint-diff-summary bot already flagged this as +1 new on the merging PR for hermes/hermes-db18949e.

The fix

  • Insert "send" into the alphabetically-sorted _BUILTIN_SUBCOMMANDS frozenset between "proxy" and "sessions".
  • Pass encoding="utf-8" to Path(file_path).read_text() in send_cmd.py:61.

Same pattern as #24738 (lsp _BUILTIN_SUBCOMMANDS), which was salvage-merged via #25011 (71c6dd0dc).

Test plan

  • Focused regression test: tests/hermes_cli/test_startup_plugin_gating.py37 passed (was 1 failed on clean origin/main). test_builtin_set_covers_every_registered_subcommand and test_builtin_set_has_no_phantom_entries both pass.
  • Ruff: ruff check hermes_cli/send_cmd.py hermes_cli/main.pyAll checks passed! (was Found 1 error on clean origin/main).
  • Regression guard: stashed the fix, re-ran the parity test → FAILED ... _BUILTIN_SUBCOMMANDS is missing these live subcommands: ['send']. Restored the fix → PASSED.

Sibling code paths that may need the same encoding= widening: other Path.read_text() callers across hermes_cli/ (e.g. auth.py, web_server.py, uninstall.py, profiles.py, banner.py, doctor.py). Intentionally left out of this PR's scope — ruff only flagged the new send_cmd.py site, and the rest are presumably grandfathered by current lint config. Happy to widen if preferred.

Related

…sResearch#27188 follow-up)

The recently-merged `feat(cli): add hermes send` (NousResearch#27188, commit 29b1bd0)
landed two small regressions that are now blocking CI on origin/main:

1. `hermes_cli/main.py::_BUILTIN_SUBCOMMANDS` doesn't include `"send"`, so
   `tests/hermes_cli/test_startup_plugin_gating.py::
   test_builtin_set_covers_every_registered_subcommand` fails on every PR
   with `_BUILTIN_SUBCOMMANDS is missing these live subcommands: ['send']`.
   The failing test explicitly directs the fix: "Add them to
   hermes_cli/main.py::_BUILTIN_SUBCOMMANDS so plugin discovery can be
   skipped when the user targets them." Correctness is unaffected (missing
   entries just force unnecessary plugin discovery), but the ruff blocker
   and the parity test both have to pass for CI to go green again.

2. `hermes_cli/send_cmd.py:61` calls `Path(file_path).read_text()` without
   an explicit `encoding=` argument, which trips `ruff PLW1514`
   (`pathlib.Path(...).read_text without explicit encoding argument`). The
   `lint-diff-summary` bot already flagged this as `+1 new` on the
   merging PR.

Both are mechanical fixes:
* Insert `"send"` into the alphabetically-sorted `_BUILTIN_SUBCOMMANDS`
  frozenset between `"proxy"` and `"sessions"`.
* Pass `encoding="utf-8"` to `read_text()` — matches the file's docstring
  intent (the value is fed straight into `send_message_tool`, which
  doesn't care about source encoding, but the codebase has standardised
  on explicit UTF-8 elsewhere).

Same pattern as NousResearch#24738 (lsp _BUILTIN_SUBCOMMANDS), which was
salvage-merged via NousResearch#25011 (71c6dd0).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 17, 2026 04:13

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Adds the send command to the CLI's reserved command list and explicitly specifies UTF-8 encoding when reading message body files.

Changes:

  • Register send as a known top-level CLI command.
  • Read message body files with explicit UTF-8 encoding rather than platform default.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
hermes_cli/send_cmd.py Use explicit utf-8 encoding for Path.read_text to ensure consistent decoding across platforms.
hermes_cli/main.py Insert "send" into the alphabetized list of reserved provider/command choices.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@cardtest15-coder

This comment was marked as spam.

@daimon-nous daimon-nous Bot added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/cli CLI entry point, hermes_cli/, setup wizard labels May 17, 2026
@briandevans

Copy link
Copy Markdown
Contributor Author

Closing — both fixes have since landed on main directly (_BUILTIN_SUBCOMMANDS now includes "send", and hermes_cli/send_cmd.py:61 already passes encoding="utf-8"). No follow-up needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants