Skip to content

salvage: batch 2 LSP-related fixes (#24738 lsp gating + #24929 mutation diagnostics false positive) - #25011

Merged
kshitijk4poor merged 3 commits into
NousResearch:mainfrom
kshitijk4poor:salvage/lsp-batch-24738-24929
May 13, 2026
Merged

salvage: batch 2 LSP-related fixes (#24738 lsp gating + #24929 mutation diagnostics false positive)#25011
kshitijk4poor merged 3 commits into
NousResearch:mainfrom
kshitijk4poor:salvage/lsp-batch-24738-24929

Conversation

@kshitijk4poor

Copy link
Copy Markdown
Collaborator

Salvage of two independent LSP-related fixes

Cherry-picks two small, unrelated bug fixes from open PRs onto current main. Both authors are preserved as commit authors and are already in AUTHOR_MAP.

Original PR Author Subsystem Fix
#24738 @briandevans CLI startup gating Adds lsp to _BUILTIN_SUBCOMMANDS so hermes lsp … skips the ~500-650ms plugin-discovery pass.
#24929 @GodsBoy Tool result classification Stops misclassifying successful write_file/patch results as failures when their JSON payload contains nested lint or LSP diagnostics with "error" keys (issue #24927).

Two independent fixes are batched here because each is small, the diffs touch disjoint files, and neither has interactions with the other. Each retains its original contributor as commit author.

#24738lsp missing from _BUILTIN_SUBCOMMANDS

The parity guard added in #22120 (tests/hermes_cli/test_startup_plugin_gating.py::test_builtin_set_covers_every_registered_subcommand) has been failing on main since lsp shipped via #24168. Verified empirically:

$ bash scripts/run_tests.sh tests/hermes_cli/test_startup_plugin_gating.py
FAILED test_builtin_set_covers_every_registered_subcommand
  AssertionError: _BUILTIN_SUBCOMMANDS is missing these live subcommands: ['lsp'].

After the fix: 37 passed.

#24929 — File mutation false positive on diagnostic-bearing results (#24927)

_detect_tool_failure() and classify_tool_failure() substring-match '"error"' and '"failed"' over the full result string. After PR #24168 wired post-write LSP diagnostics into write_file and patch, any successful mutation whose result includes nested lint/LSP errors gets a [error] failure tag in the CLI display, plus a turn-end verifier footer claiming the write didn't land — even though it did.

This PR introduces a shared agent/tool_result_classification.py::file_mutation_result_landed() helper and patches three callsites:

  • agent/display.py::_detect_tool_failure() — CLI failure tag
  • agent/tool_guardrails.py::classify_tool_failure() — guardrail mirror
  • run_agent.py::_record_file_mutation_result() — turn-end verifier upstream

A duplicate PR (#24960) was filed against the same issue. It has a real bug (data.get("bytes_written") truthiness check, which misclassifies an empty 0-byte write with lint diagnostics as a failure). #24929 uses "bytes_written" in data (presence check) which is correct. We are closing #24960 in favor of this salvage.

Test plan

bash scripts/run_tests.sh \
  tests/hermes_cli/test_startup_plugin_gating.py \
  tests/agent/test_display.py \
  tests/agent/test_tool_guardrails.py \
  tests/agent/test_tool_result_classification.py \
  tests/run_agent/test_file_mutation_verifier.py

Result: 117 passed.

E2E sanity:

  • 'lsp' in _BUILTIN_SUBCOMMANDS → True
  • _detect_tool_failure("write_file", '{"bytes_written": 0, "lint": {"errors": [{"severity": "error"}]}}')(False, "") (would be (True, " [error]") without the fix; this is the case PR fix(agent): prevent mutation verifier false positives from lint diagnostics #24960 mishandles)
  • _detect_tool_failure("write_file", '{"error": "Permission denied"}')(True, " [error]") (genuine failures still detected)
  • _detect_tool_failure("patch", '{"success": true, "lsp_diagnostics": "<diagnostics>ERROR foo</diagnostics>"}')(False, "")

Closes #24927 (via #24929).
Supersedes #24738, #24929, #24960.

cc @briandevans @GodsBoy — your commits are preserved as-is in this salvage; original authorship intact in git log.

briandevans and others added 3 commits May 13, 2026 18:43
…ipped

`lsp` is registered as a top-level subparser in `main()` (lines 9539-9545)
via `agent.lsp.cli.register_subparser`, so it shows up in `hermes --help`
output alongside the other built-ins. The `_BUILTIN_SUBCOMMANDS` set used
by `_plugin_cli_discovery_needed` to short-circuit the ~500-650ms plugin
import pass did not list it, so every `hermes lsp ...` invocation paid
the full discovery cost despite being a fully-built-in command.

This is also caught by the parity guard added in NousResearch#22120:
`tests/hermes_cli/test_startup_plugin_gating.py::test_builtin_set_covers_every_registered_subcommand`
has been failing on clean origin/main with:

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

Fix: add `"lsp"` to the frozenset (alphabetical position between `logs`
and `mcp`). The accompanying `test_builtin_set_has_no_phantom_entries`
guard still passes because `lsp` is genuinely live — registered via the
guarded `try/except Exception` in main() since NousResearch#24168.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drops the duplicate _FILE_MUTATING_TOOLS frozenset in run_agent.py and
imports the canonical FILE_MUTATING_TOOL_NAMES from
agent/tool_result_classification.py (aliased as _FILE_MUTATING_TOOLS to
avoid renaming the existing call sites). Prevents future drift if
another file-mutating tool is added — only one set needs updating.

No behavior change: same frozenset({'write_file', 'patch'}), and the
117 PR-scoped tests still pass.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint comp/cli CLI entry point, hermes_cli/, setup wizard labels May 13, 2026
@kshitijk4poor
kshitijk4poor merged commit c3094b4 into NousResearch:main May 13, 2026
12 of 13 checks passed
@kshitijk4poor
kshitijk4poor deleted the salvage/lsp-batch-24738-24929 branch May 13, 2026 13:53
briandevans added a commit to briandevans/hermes-agent that referenced this pull request May 28, 2026
…h#32477)

Typing /agents (or its /tasks alias) during an active agent turn does
nothing in classic CLI: the slash command goes through _pending_input,
which process_loop only drains after self.chat() returns. By the time
the queued command is pulled, the user has been staring at a quiet
screen for the entire delegation chain — defeating the introspection
command's only purpose.

Mirror the inline-dispatch pattern PR NousResearch#25011 introduced for /steer:
when the detector sees /agents while the agent is running, call
process_command directly on the UI thread. _handle_agents_command
is read-only (reads process_registry + a couple of CLI attrs) and
_cprint already routes thread-unsafe output through
prompt_toolkit's run_in_terminal, so UI-thread dispatch is safe.

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

Labels

comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint 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.

Fix file mutation verifier false positive for diagnostic-bearing writes

4 participants