diff --git a/agent/turn_context.py b/agent/turn_context.py index e080d6a5d9692..e46b39a659be1 100644 --- a/agent/turn_context.py +++ b/agent/turn_context.py @@ -1126,6 +1126,7 @@ def build_turn_context( # Per-turn file-mutation verifier state. agent._turn_failed_file_mutations = {} + agent._turn_superseded_file_mutations = {} agent._turn_file_mutation_paths = set() agent._verification_stop_nudges = 0 agent._pre_verify_nudges = 0 diff --git a/agent/turn_finalizer.py b/agent/turn_finalizer.py index d4d6a23e78650..2ac57dfb9d46b 100644 --- a/agent/turn_finalizer.py +++ b/agent/turn_finalizer.py @@ -474,8 +474,9 @@ def finalize_turn( if final_response and not interrupted: try: _failed = getattr(agent, "_turn_failed_file_mutations", None) or {} - if _failed and agent._file_mutation_verifier_enabled(): - footer = agent._format_file_mutation_failure_footer(_failed) + _superseded = getattr(agent, "_turn_superseded_file_mutations", None) or {} + if (_failed or _superseded) and agent._file_mutation_verifier_enabled(): + footer = agent._format_file_mutation_failure_footer(_failed, _superseded) if footer: final_response = final_response.rstrip() + "\n\n" + footer except Exception as _ver_err: diff --git a/run_agent.py b/run_agent.py index 9a65429259897..0522ac3a7e659 100644 --- a/run_agent.py +++ b/run_agent.py @@ -3311,11 +3311,21 @@ def _record_file_mutation_result( state dict hasn't been initialised yet (e.g. a tool dispatched outside ``run_conversation``). """ - if tool_name not in _FILE_MUTATING_TOOLS: - return state = getattr(self, "_turn_failed_file_mutations", None) if state is None: return + + # A protected config.yaml patch/write_file failure can be recovered in the same + # turn by the sanctioned CLI path (`hermes config set ...`). That is + # not a file-tool mutation, so the verifier should not keep implying + # the turn outcome is unknown when it has direct evidence that the + # approved config command succeeded. + if tool_name == "terminal": + self._record_config_set_superseded_mutation(args, result, is_error) + return + + if tool_name not in _FILE_MUTATING_TOOLS: + return targets = _extract_file_mutation_targets(tool_name, args) if not targets: return @@ -3339,6 +3349,69 @@ def _record_file_mutation_result( for path in targets: state.pop(path, None) + @staticmethod + def _terminal_config_set_path(args: Dict[str, Any], result: Any) -> Optional[str]: + """Return config.yaml path from a successful sanctioned config write. + + This intentionally recognizes only the narrow, approved path that + resolves protected config.yaml patch failures: `hermes config set ...` + with a zero exit code and the CLI's success output naming config.yaml. + It does not try to infer arbitrary file changes from terminal output. + """ + command = str(args.get("command") or "") if isinstance(args, dict) else "" + if not re.search(r"(?:^|[;&|]\s*)hermes\s+config\s+set\b", command): + return None + if not isinstance(result, str): + return None + try: + data = json.loads(result.strip()) + except Exception: + return None + if not isinstance(data, dict) or data.get("exit_code") != 0 or data.get("error"): + return None + output = str(data.get("output") or "") + match = re.search(r"(?m)\bSet\b.*?\bin\s+(.+?config\.yaml)\s*$", output) + if not match: + return None + return match.group(1).strip() + + @staticmethod + def _same_path(left: str, right: str) -> bool: + def _norm(p: str) -> str: + return os.path.expanduser(str(p)).replace("\\", "/") + return _norm(left) == _norm(right) + + def _record_config_set_superseded_mutation( + self, + args: Dict[str, Any], + result: Any, + is_error: bool, + ) -> None: + """Move protected config patch failures to a recovered bucket. + + The regular verifier state is keyed by file-tool targets. A later + `hermes config set ...` terminal call can legitimately modify the same + config file through the sanctioned interface, so preserve both facts: + the patch failed, and the approved config command succeeded. + """ + if is_error: + return + config_path = self._terminal_config_set_path(args, result) + if not config_path: + return + failed = getattr(self, "_turn_failed_file_mutations", None) + if not failed: + return + superseded = getattr(self, "_turn_superseded_file_mutations", None) + if superseded is None: + superseded = {} + self._turn_superseded_file_mutations = superseded + for path in list(failed): + if self._same_path(path, config_path): + info = dict(failed.pop(path) or {}) + info["superseded_by"] = "terminal: hermes config set" + superseded[path] = info + def _file_mutation_verifier_enabled(self) -> bool: """Check whether the per-turn file-mutation verifier footer is on. @@ -3396,12 +3469,17 @@ def _neutralize_footer_paths(cls, text: str) -> str: return cls._FOOTER_PATH_RE.sub(lambda m: f"`{m.group(0)}`", text) @classmethod - def _format_file_mutation_failure_footer(cls, failed: Dict[str, Dict[str, Any]]) -> str: - """Render the per-turn failed-mutation dict as a user-facing footer. + def _format_file_mutation_failure_footer( + cls, + failed: Dict[str, Dict[str, Any]], + superseded: Optional[Dict[str, Dict[str, Any]]] = None, + ) -> str: + """Render per-turn file-mutation verifier state as a footer. - Displays up to 10 paths with their first error preview, then a - count of any additional failures. Returns an empty string when - the dict is empty so callers can concatenate unconditionally. + Displays failed file-tool mutations and, when applicable, protected + config edit attempts that failed via patch/write_file but were later + recovered by an approved command such as `hermes config set`. + Returns an empty string when both dicts are empty. Every file path that reaches the user-facing text — both the bullet path and any path echoed inside the tool's error preview — is @@ -3409,14 +3487,33 @@ def _format_file_mutation_failure_footer(cls, failed: Dict[str, Dict[str, Any]]) bare-path media extractor can never auto-attach a protected file (e.g. ``~/.hermes/config.yaml``) to a messaging channel (#35584). """ - if not failed: + failed = failed or {} + superseded = superseded or {} + if not failed and not superseded: return "" - lines = [ - "⚠️ File-mutation verifier: " - f"{len(failed)} file(s) were NOT modified this turn despite any " - "wording above that may suggest otherwise. Run `git status` or " - "`read_file` to confirm." - ] + lines: List[str] = [] + if failed and superseded: + lines.append( + "⚠️ File-mutation verifier: " + f"{len(failed)} file mutation attempt(s) still failed; " + f"{len(superseded)} protected config edit attempt(s) failed via " + "direct file mutation but were later applied through an approved command. " + "Run `git status` or `read_file` to confirm." + ) + elif failed: + lines.append( + "⚠️ File-mutation verifier: " + f"{len(failed)} file(s) were NOT modified this turn despite any " + "wording above that may suggest otherwise. Run `git status` or " + "`read_file` to confirm." + ) + else: + lines.append( + "⚠️ File-mutation verifier: direct file mutation was blocked, " + "but the requested protected config change was later applied " + "through the approved `hermes config set` command." + ) + shown = 0 for path, info in failed.items(): if shown >= 10: @@ -3428,7 +3525,21 @@ def _format_file_mutation_failure_footer(cls, failed: Dict[str, Dict[str, Any]]) else: lines.append(f" • `{path}` — [{tool}] failed") shown += 1 - remaining = len(failed) - shown + + for path, info in superseded.items(): + if shown >= 10: + break + preview = (info.get("error_preview") or "").strip() + tool = info.get("tool") or "patch" + superseded_by = info.get("superseded_by") or "terminal: hermes config set" + if preview: + lines.append(f" • `{path}` — [{tool}] blocked: {preview}") + else: + lines.append(f" • `{path}` — [{tool}] blocked") + lines.append(f" ↳ recovered by [{superseded_by}]") + shown += 1 + + remaining = len(failed) + len(superseded) - shown if remaining > 0: lines.append(f" • … and {remaining} more") # Neutralize any path the preview text echoed (the bullet path is diff --git a/tests/run_agent/test_file_mutation_verifier.py b/tests/run_agent/test_file_mutation_verifier.py index bfb5ff49c04d6..33d2bcdaada3a 100644 --- a/tests/run_agent/test_file_mutation_verifier.py +++ b/tests/run_agent/test_file_mutation_verifier.py @@ -110,6 +110,7 @@ def _bare_agent() -> AIAgent: """ agent = object.__new__(AIAgent) agent._turn_failed_file_mutations = {} + agent._turn_superseded_file_mutations = {} agent._turn_file_mutation_paths = set() return agent @@ -227,6 +228,202 @@ def test_repeated_failure_keeps_first_error(self): # the initial root cause. assert "first error" in agent._turn_failed_file_mutations["/tmp/a.md"]["error_preview"] + def test_successful_hermes_config_set_supersedes_config_patch_failure(self): + agent = _bare_agent() + cfg = "/home/u/.hermes/config.yaml" + agent._record_file_mutation_result( + "patch", + {"mode": "replace", "path": cfg, "old_string": "", "new_string": "max_concurrent_sessions: 10"}, + json.dumps({"error": f"Refusing to write to Hermes config file: {cfg}"}), + is_error=True, + ) + assert cfg in agent._turn_failed_file_mutations + + agent._record_file_mutation_result( + "terminal", + {"command": "hermes config set max_concurrent_sessions 10"}, + json.dumps({ + "output": f"✓ Set max_concurrent_sessions = 10 in {cfg}", + "exit_code": 0, + "error": None, + }), + is_error=False, + ) + + assert agent._turn_failed_file_mutations == {} + assert cfg in agent._turn_superseded_file_mutations + info = agent._turn_superseded_file_mutations[cfg] + assert info["tool"] == "patch" + assert info["superseded_by"] == "terminal: hermes config set" + + def test_failed_hermes_config_set_does_not_supersede_config_patch_failure(self): + agent = _bare_agent() + cfg = "/home/u/.hermes/config.yaml" + agent._record_file_mutation_result( + "patch", + {"mode": "replace", "path": cfg, "old_string": "", "new_string": "x"}, + json.dumps({"error": "Refusing to write to Hermes config file"}), + is_error=True, + ) + + agent._record_file_mutation_result( + "terminal", + {"command": "hermes config set max_concurrent_sessions 10"}, + json.dumps({"output": "boom", "exit_code": 1, "error": None}), + is_error=True, + ) + + assert cfg in agent._turn_failed_file_mutations + assert agent._turn_superseded_file_mutations == {} + + def test_hermes_config_set_only_supersedes_matching_config_yaml_leaves_unrelated(self): + """Successful `hermes config set` moves only the matching config.yaml + failure to superseded; an unrelated failed file mutation stays put.""" + agent = _bare_agent() + cfg = "/home/u/.hermes/config.yaml" + other = "/tmp/other.md" + + # Unrelated file mutation fails. + agent._record_file_mutation_result( + "patch", + {"mode": "replace", "path": other, "old_string": "x", "new_string": "y"}, + json.dumps({"error": "Could not find old_string"}), + is_error=True, + ) + # Config patch fails. + agent._record_file_mutation_result( + "patch", + {"mode": "replace", "path": cfg, "old_string": "", "new_string": "z"}, + json.dumps({"error": f"Refusing to write to Hermes config file: {cfg}"}), + is_error=True, + ) + assert other in agent._turn_failed_file_mutations + assert cfg in agent._turn_failed_file_mutations + + # Sanctioned config set succeeds for the same config.yaml. + agent._record_file_mutation_result( + "terminal", + {"command": "hermes config set max_concurrent_sessions 10"}, + json.dumps({ + "output": f"✓ Set max_concurrent_sessions = 10 in {cfg}", + "exit_code": 0, + "error": None, + }), + is_error=False, + ) + + # Only the matching config.yaml was moved to superseded. + assert other in agent._turn_failed_file_mutations + assert cfg not in agent._turn_failed_file_mutations + assert cfg in agent._turn_superseded_file_mutations + assert other not in agent._turn_superseded_file_mutations + + def test_hermes_config_set_different_path_does_not_supersede_original(self): + """A `hermes config set` targeting a *different* config.yaml path + leaves the original failed config mutation untouched.""" + agent = _bare_agent() + cfg_orig = "/home/u/.hermes/config.yaml" + cfg_other = "/home/u/.hermes/profiles/foo/config.yaml" + + # Config patch fails for the original path. + agent._record_file_mutation_result( + "patch", + {"mode": "replace", "path": cfg_orig, "old_string": "", "new_string": "x"}, + json.dumps({"error": "Refusing to write to Hermes config file"}), + is_error=True, + ) + assert cfg_orig in agent._turn_failed_file_mutations + + # Sanctioned config set succeeds for a *different* config file. + agent._record_file_mutation_result( + "terminal", + {"command": "hermes config set max_concurrent_sessions 10"}, + json.dumps({ + "output": f"✓ Set max_concurrent_sessions = 10 in {cfg_other}", + "exit_code": 0, + "error": None, + }), + is_error=False, + ) + + # Original failure still stands; nothing was superseded. + assert cfg_orig in agent._turn_failed_file_mutations + assert agent._turn_superseded_file_mutations == {} + + def test_hermes_config_set_tilde_path_matches_absolute_path(self): + """``hermes config set`` output with ``~/.hermes/config.yaml`` + matches the absolute ``/home/...`` path stored in failed mutations.""" + import os as _os + agent = _bare_agent() + home = _os.path.expanduser("~") + cfg_abs = f"{home}/.hermes/config.yaml" + cfg_tilde = "~/.hermes/config.yaml" + + # Config patch fails for absolute path. + agent._record_file_mutation_result( + "patch", + {"mode": "replace", "path": cfg_abs, "old_string": "", "new_string": "x"}, + json.dumps({"error": "Refusing to write to Hermes config file"}), + is_error=True, + ) + assert cfg_abs in agent._turn_failed_file_mutations + + # Sanctioned config set succeeds; output uses tilde path. + agent._record_file_mutation_result( + "terminal", + {"command": "hermes config set max_concurrent_sessions 10"}, + json.dumps({ + "output": f"\u2713 Set max_concurrent_sessions = 10 in {cfg_tilde}", + "exit_code": 0, + "error": None, + }), + is_error=False, + ) + + # Tilde path matches absolute path via expanduser, superseding failure. + assert agent._turn_failed_file_mutations == {} + assert cfg_abs in agent._turn_superseded_file_mutations + + def test_hermes_config_set_success_line_can_have_trailing_output(self): + """Recovery should not depend on the success line being final output.""" + agent = _bare_agent() + cfg = "/home/u/.hermes/config.yaml" + + agent._record_file_mutation_result( + "patch", + {"mode": "replace", "path": cfg, "old_string": "", "new_string": "x"}, + json.dumps({"error": "Refusing to write to Hermes config file"}), + is_error=True, + ) + assert cfg in agent._turn_failed_file_mutations + + agent._record_file_mutation_result( + "terminal", + {"command": "hermes config set max_concurrent_sessions 10"}, + json.dumps({ + "output": f"✓ Set max_concurrent_sessions = 10 in {cfg}\nReloaded config", + "exit_code": 0, + "error": None, + }), + is_error=False, + ) + + assert agent._turn_failed_file_mutations == {} + assert cfg in agent._turn_superseded_file_mutations + + def test_v4a_multi_file_all_tracked(self): + agent = _bare_agent() + body = ( + "*** Begin Patch\n" + "*** Update File: /tmp/a.md\n@@ @@\n-a\n+b\n" + "*** Update File: /tmp/b.md\n@@ @@\n-a\n+b\n" + "*** End Patch\n" + ) + agent._record_file_mutation_result( + "patch", {"mode": "patch", "patch": body}, + json.dumps({"error": "parse failure"}), is_error=True, + ) + assert set(agent._turn_failed_file_mutations) == {"/tmp/a.md", "/tmp/b.md"} @@ -263,6 +460,23 @@ def test_truncation_at_10_entries(self): assert len(bullet_lines) == 11 # 10 shown + 1 summary + def test_superseded_config_patch_footer_shows_both_attempts(self): + cfg = "/home/u/.hermes/config.yaml" + out = AIAgent._format_file_mutation_failure_footer( + {}, + {cfg: { + "tool": "patch", + "error_preview": f"Refusing to write to Hermes config file: {cfg}", + "superseded_by": "terminal: hermes config set", + }}, + ) + + assert "direct file mutation was blocked" in out + assert "approved `hermes config set` command" in out + assert "[patch] blocked" in out + assert "recovered by [terminal: hermes config set]" in out + assert f"`{cfg}`" in out + def test_footer_path_not_extracted_by_gateway(self): """End-to-end: the gateway's extract_local_files must NOT pull a config.yaml path out of the rendered footer (#35584).""" @@ -273,7 +487,7 @@ def test_footer_path_not_extracted_by_gateway(self): tmp = tempfile.mkdtemp(prefix="hermes_footer_") try: cfg = os.path.join(tmp, "config.yaml") - with open(cfg, "w") as fh: + with open(cfg, "w", encoding="utf-8") as fh: fh.write("openrouter_api_key: sk-LEAK\n") footer = AIAgent._format_file_mutation_failure_footer( {cfg: {