From 7163fb1ac4a99e3675fd9752afc740c93bcf2887 Mon Sep 17 00:00:00 2001 From: briandevans <252620095+briandevans@users.noreply.github.com> Date: Tue, 12 May 2026 19:13:57 -0700 Subject: [PATCH 1/3] fix(cli): add 'lsp' to _BUILTIN_SUBCOMMANDS so plugin discovery is skipped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 #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 #24168. Co-Authored-By: Claude Opus 4.7 (1M context) --- hermes_cli/main.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 64310dc6af1ce..e8aa0d761c46c 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -9188,10 +9188,10 @@ def _build_provider_choices() -> list[str]: "computer-use", "config", "cron", "curator", "dashboard", "debug", "doctor", "dump", "fallback", "gateway", "hooks", "import", "insights", - "kanban", "login", "logout", "logs", "mcp", "memory", "model", - "pairing", "plugins", "profile", "sessions", "setup", "skills", - "slack", "status", "tools", "uninstall", "update", "version", - "webhook", "whatsapp", "chat", + "kanban", "login", "logout", "logs", "lsp", "mcp", "memory", + "model", "pairing", "plugins", "profile", "sessions", "setup", + "skills", "slack", "status", "tools", "uninstall", "update", + "version", "webhook", "whatsapp", "chat", # Help-ish invocations — plugin commands not being listed in # top-level --help is an acceptable trade-off for skipping an # expensive eager import of every bundled plugin module. From 5bf19273bad325d66b771a37dc1d03e18c5ee5c4 Mon Sep 17 00:00:00 2001 From: GodsBoy Date: Wed, 13 May 2026 11:36:07 +0200 Subject: [PATCH 2/3] fix: classify landed file mutations with diagnostics --- agent/display.py | 3 ++ agent/tool_guardrails.py | 3 ++ agent/tool_result_classification.py | 26 ++++++++++ run_agent.py | 4 +- tests/agent/test_display.py | 22 ++++++++ tests/agent/test_tool_guardrails.py | 16 ++++++ .../agent/test_tool_result_classification.py | 30 +++++++++++ .../run_agent/test_file_mutation_verifier.py | 50 +++++++++++++++++++ 8 files changed, 153 insertions(+), 1 deletion(-) create mode 100644 agent/tool_result_classification.py create mode 100644 tests/agent/test_tool_result_classification.py diff --git a/agent/display.py b/agent/display.py index e9a19ff6192b8..6c5c970aeff21 100644 --- a/agent/display.py +++ b/agent/display.py @@ -14,6 +14,7 @@ from pathlib import Path from utils import safe_json_loads +from agent.tool_result_classification import file_mutation_result_landed # ANSI escape codes for coloring tool failure indicators _RED = "\033[31m" @@ -810,6 +811,8 @@ def _detect_tool_failure(tool_name: str, result: str | None) -> tuple[bool, str] """ if result is None: return False, "" + if file_mutation_result_landed(tool_name, result): + return False, "" if tool_name == "terminal": data = safe_json_loads(result) diff --git a/agent/tool_guardrails.py b/agent/tool_guardrails.py index 3c85d78209023..5a9ddd507ba54 100644 --- a/agent/tool_guardrails.py +++ b/agent/tool_guardrails.py @@ -14,6 +14,7 @@ from typing import Any, Mapping from utils import safe_json_loads +from agent.tool_result_classification import file_mutation_result_landed IDEMPOTENT_TOOL_NAMES = frozenset( @@ -196,6 +197,8 @@ def classify_tool_failure(tool_name: str, result: str | None) -> tuple[bool, str """ if result is None: return False, "" + if file_mutation_result_landed(tool_name, result): + return False, "" if tool_name == "terminal": data = safe_json_loads(result) diff --git a/agent/tool_result_classification.py b/agent/tool_result_classification.py new file mode 100644 index 0000000000000..e136e2964da8e --- /dev/null +++ b/agent/tool_result_classification.py @@ -0,0 +1,26 @@ +"""Shared helpers for classifying tool result payloads.""" + +from __future__ import annotations + +import json +from typing import Any + + +FILE_MUTATING_TOOL_NAMES = frozenset({"write_file", "patch"}) + + +def file_mutation_result_landed(tool_name: str, result: Any) -> bool: + """Return True when a file mutation result proves the write landed.""" + if tool_name not in FILE_MUTATING_TOOL_NAMES or not isinstance(result, str): + return False + try: + data = json.loads(result.strip()) + except Exception: + return False + if not isinstance(data, dict) or data.get("error"): + return False + if tool_name == "write_file": + return "bytes_written" in data + if tool_name == "patch": + return data.get("success") is True + return False diff --git a/run_agent.py b/run_agent.py index f0597c90880d4..a218530093195 100644 --- a/run_agent.py +++ b/run_agent.py @@ -181,6 +181,7 @@ def __repr__(self): append_toolguard_guidance, toolguard_synthetic_result, ) +from agent.tool_result_classification import file_mutation_result_landed from agent.trajectory import ( convert_scratchpad_to_think, has_incomplete_scratchpad, save_trajectory as _save_trajectory_to_file, @@ -5347,7 +5348,8 @@ def _record_file_mutation_result( targets = _extract_file_mutation_targets(tool_name, args) if not targets: return - if is_error: + landed = file_mutation_result_landed(tool_name, result) + if is_error and not landed: preview = _extract_error_preview(result) for path in targets: # Keep the FIRST error we saw for a given path unless we diff --git a/tests/agent/test_display.py b/tests/agent/test_display.py index c6ad837af9736..5e18fa17e0c29 100644 --- a/tests/agent/test_display.py +++ b/tests/agent/test_display.py @@ -1,6 +1,7 @@ """Tests for agent/display.py — build_tool_preview() and inline diff previews.""" import os +import json import pytest from unittest.mock import MagicMock, patch @@ -149,6 +150,27 @@ def test_path_preview_uses_positive_configured_limit_not_default(self): assert path in line assert "..." not in line + def test_write_file_lint_error_result_is_not_marked_failed(self): + result = json.dumps({ + "bytes_written": 12, + "lint": {"status": "error", "output": "SyntaxError: invalid syntax"}, + }) + + line = get_cute_tool_message("write_file", {"path": "/tmp/a.py"}, 0.1, result=result) + + assert "[error]" not in line + + def test_patch_lsp_diagnostics_result_is_not_marked_failed(self): + result = json.dumps({ + "success": True, + "diff": "--- a/tmp.py\n+++ b/tmp.py\n", + "lsp_diagnostics": "ERROR [1:1] type mismatch", + }) + + line = get_cute_tool_message("patch", {"path": "/tmp/a.py"}, 0.1, result=result) + + assert "[error]" not in line + class TestEditDiffPreview: def test_extract_edit_diff_for_patch(self): diff --git a/tests/agent/test_tool_guardrails.py b/tests/agent/test_tool_guardrails.py index c50be56f43e0c..26593b7ef620d 100644 --- a/tests/agent/test_tool_guardrails.py +++ b/tests/agent/test_tool_guardrails.py @@ -7,6 +7,7 @@ ToolCallGuardrailController, ToolCallSignature, canonical_tool_args, + classify_tool_failure, ) @@ -131,6 +132,21 @@ def test_success_resets_exact_signature_failure_streak(): assert controller.before_call("web_search", args).action == "allow" +def test_file_mutation_lint_error_result_is_not_a_tool_failure(): + write_result = json.dumps({ + "bytes_written": 12, + "lint": {"status": "error", "output": "SyntaxError: invalid syntax"}, + }) + patch_result = json.dumps({ + "success": True, + "diff": "--- a/tmp.py\n+++ b/tmp.py\n", + "lsp_diagnostics": "ERROR [1:1] type mismatch", + }) + + assert classify_tool_failure("write_file", write_result) == (False, "") + assert classify_tool_failure("patch", patch_result) == (False, "") + + def test_same_tool_varying_args_warns_by_default_without_halting(): controller = ToolCallGuardrailController( ToolCallGuardrailConfig(same_tool_failure_warn_after=2, same_tool_failure_halt_after=3) diff --git a/tests/agent/test_tool_result_classification.py b/tests/agent/test_tool_result_classification.py new file mode 100644 index 0000000000000..2b4b5b150cf70 --- /dev/null +++ b/tests/agent/test_tool_result_classification.py @@ -0,0 +1,30 @@ +"""Tests for shared tool result classification helpers.""" + +import json + +from agent.tool_result_classification import file_mutation_result_landed + + +def test_write_file_with_nested_lint_error_counts_as_landed(): + result = json.dumps({ + "bytes_written": 12, + "lint": {"status": "error", "output": "SyntaxError: invalid syntax"}, + }) + + assert file_mutation_result_landed("write_file", result) is True + + +def test_patch_with_nested_lsp_diagnostics_counts_as_landed(): + result = json.dumps({ + "success": True, + "diff": "--- a/tmp.py\n+++ b/tmp.py\n", + "lsp_diagnostics": "ERROR [1:1] type mismatch", + }) + + assert file_mutation_result_landed("patch", result) is True + + +def test_top_level_file_mutation_error_does_not_count_as_landed(): + result = json.dumps({"success": True, "error": "post-write verification failed"}) + + assert file_mutation_result_landed("patch", result) is False diff --git a/tests/run_agent/test_file_mutation_verifier.py b/tests/run_agent/test_file_mutation_verifier.py index fca002d2314cb..73684ad1c2e75 100644 --- a/tests/run_agent/test_file_mutation_verifier.py +++ b/tests/run_agent/test_file_mutation_verifier.py @@ -166,6 +166,56 @@ def test_success_removes_prior_failure(self): ) assert agent._turn_failed_file_mutations == {} + def test_write_file_with_lint_error_counts_as_landed(self): + agent = _bare_agent() + agent._record_file_mutation_result( + "write_file", + {"path": "/tmp/a.py", "content": "bad"}, + json.dumps({"error": "write failed"}), + is_error=True, + ) + assert "/tmp/a.py" in agent._turn_failed_file_mutations + + result = json.dumps({ + "bytes_written": 24, + "lint": {"status": "error", "output": "SyntaxError: invalid syntax"}, + }) + + agent._record_file_mutation_result( + "write_file", + {"path": "/tmp/a.py", "content": "def nope(:\n"}, + result, + is_error=True, + ) + + assert agent._turn_failed_file_mutations == {} + + def test_patch_with_lsp_diagnostics_counts_as_landed(self): + agent = _bare_agent() + agent._record_file_mutation_result( + "patch", + {"mode": "replace", "path": "/tmp/a.py", "old_string": "x", "new_string": "y"}, + json.dumps({"error": "Could not find old_string"}), + is_error=True, + ) + assert "/tmp/a.py" in agent._turn_failed_file_mutations + + result = json.dumps({ + "success": True, + "diff": "--- a/tmp.py\n+++ b/tmp.py\n", + "files_modified": ["/tmp/a.py"], + "lsp_diagnostics": "ERROR [1:1] type mismatch", + }) + + agent._record_file_mutation_result( + "patch", + {"mode": "replace", "path": "/tmp/a.py", "old_string": "x", "new_string": "y"}, + result, + is_error=True, + ) + + assert agent._turn_failed_file_mutations == {} + def test_repeated_failure_keeps_first_error(self): agent = _bare_agent() agent._record_file_mutation_result( From 39faa1aec398b20f22ae4ee6c0617925faa99a03 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Wed, 13 May 2026 18:59:03 +0530 Subject: [PATCH 3/3] refactor: import FILE_MUTATING_TOOL_NAMES from shared module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- run_agent.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/run_agent.py b/run_agent.py index a218530093195..f2f3379e0d78e 100644 --- a/run_agent.py +++ b/run_agent.py @@ -181,7 +181,10 @@ def __repr__(self): append_toolguard_guidance, toolguard_synthetic_result, ) -from agent.tool_result_classification import file_mutation_result_landed +from agent.tool_result_classification import ( + FILE_MUTATING_TOOL_NAMES as _FILE_MUTATING_TOOLS, + file_mutation_result_landed, +) from agent.trajectory import ( convert_scratchpad_to_think, has_incomplete_scratchpad, save_trajectory as _save_trajectory_to_file, @@ -350,7 +353,7 @@ def remaining(self) -> int: # Tools that mutate files on disk. Used by the per-turn verifier that # surfaces silently-failed file edits so the model can't over-claim success. -_FILE_MUTATING_TOOLS = frozenset({"write_file", "patch"}) +# Imported above as `_FILE_MUTATING_TOOLS` from `agent.tool_result_classification`. # Maximum number of concurrent worker threads for parallel tool execution. _MAX_TOOL_WORKERS = 8