fix(hooks): close two residual secret-disclosure channels to the model

agent/outbound_webhooks.py

@@ -137,8 +137,32 @@ def __post_init__(self) -> None:
 
     @property
     def label(self) -> str:
+        """Operator-facing identity. May be the raw URL — LOG CHANNEL ONLY.
+
+        Falls back to ``self.url`` when the optional ``name:`` is omitted, and
+        a webhook URL is a bearer credential (``hooks.slack.com/services/T…/
+        B…/<secret>``, ``?token=``). Never route this to a model-visible
+        string; use :attr:`display_label` there.
+        """
         return self.name or self.url
 
+    @property
+    def display_label(self) -> str:
+        """Return a stable, secret-free identity safe for model-facing text.
+
+        The configured ``name:`` is an operator-chosen identifier and is used
+        as-is. Without one we must NOT fall back to the URL, so the target is
+        named by a short stable digest of it instead — distinct targets stay
+        distinguishable without disclosing the credential.
+
+        Mirrors :func:`agent.shell_hooks.hook_display_name`, which does the
+        same job for the other producer of synthesized callback identities.
+        """
+        if self.name:
+            return self.name
+        digest = hashlib.sha256(self.url.encode("utf-8", "replace")).hexdigest()[:8]
+        return f"webhook#{digest}"
+
     def matches_tool(self, tool_name: Optional[str]) -> bool:
         if not self.matcher:
             return True
@@ -396,7 +420,12 @@ def _callback(**kwargs: Any) -> None:
         _enqueue(_build_delivery(event, target, body, delivery_id))
         return None
 
-    _callback.__name__ = f"outbound_webhook[{event}:{target.label}]"
+    # The dispatcher reads __name__ into fail-closed refusal messages returned
+    # to the MODEL (hermes_cli/plugins.py _callback_label), so this must never
+    # carry `target.label`: that falls back to the raw URL when the optional
+    # `name:` is omitted, and a webhook URL is a bearer credential. The raw URL
+    # stays on the log channel only (register_from_config above).
+    _callback.__name__ = f"outbound_webhook[{event}:{target.display_label}]"
     _callback.__qualname__ = _callback.__name__
     return _callback
 

agent/shell_hooks.py

@@ -543,10 +543,24 @@ def _spawn(spec: ShellHookSpec, stdin_json: str) -> Dict[str, Any]:
 
     Returns a diagnostic dict with the same keys for every outcome
     (``returncode``, ``stdout``, ``stderr``, ``timed_out``,
-    ``elapsed_seconds``, ``error``).  This is the single place the
-    subprocess is actually invoked — both the live callback path
+    ``elapsed_seconds``, ``error``, ``error_detail``).  This is the single
+    place the subprocess is actually invoked — both the live callback path
     (:func:`_make_callback`) and the CLI test helper (:func:`run_once`)
     go through it.
+
+    TWO error channels, because they have opposite requirements:
+
+    * ``error`` is REDACTED and model-facing. It reaches the model through
+      :func:`_evaluate_result` -> :func:`_fail_closed_block`, so it carries
+      the failure's exception CLASS only — never the raw command, argv, or
+      ``str(exc)``, any of which can hold an inline credential.
+    * ``error_detail`` is the OPERATOR channel: the full diagnostic text, for
+      the log and for ``hermes hooks test`` / ``hermes doctor``. Without it a
+      shlex ``No closing quotation`` and a spawn ``EACCES``/``ENOEXEC`` are
+      indistinguishable to whoever has to fix the hook. It is ``None`` when
+      there is nothing to add beyond ``error``.
+
+    ``error_detail`` must never be routed to a model-facing string.
     """
     result: Dict[str, Any] = {
         "returncode": None,
@@ -555,14 +569,25 @@ def _spawn(spec: ShellHookSpec, stdin_json: str) -> Dict[str, Any]:
         "timed_out": False,
         "elapsed_seconds": 0.0,
         "error": None,
+        "error_detail": None,
     }
     try:
         # Windows-safe: plain shlex.split eats backslashes in paths (#78293).
         from hermes_cli._subprocess_compat import split_command_line
 
         argv = split_command_line(os.path.expanduser(spec.command))
     except ValueError as exc:
-        result["error"] = f"command {spec.command!r} cannot be parsed: {exc}"
+        # `error` reaches the MODEL via _evaluate_result -> _fail_closed_block,
+        # so it must never carry the raw command or exception text derived from
+        # it: an unparseable command is exactly the shape that still holds an
+        # inline credential (`sh -c 'export TOK=…` with the quote left open).
+        # Name the hook by its digest and the failure by its exception CLASS.
+        # The raw command stays on the log channel only (_evaluate_result).
+        result["error"] = (
+            f"command cannot be parsed ({type(exc).__name__})"
+        )
+        # Operator channel only — `exc` stringifies the offending command text.
+        result["error_detail"] = f"command cannot be parsed: {exc}"
         return result
     if not argv:
         result["error"] = "empty command"
@@ -596,7 +621,13 @@ def _spawn(spec: ShellHookSpec, stdin_json: str) -> Dict[str, Any]:
         result["error"] = "command not executable"
         return result
     except Exception as exc:  # pragma: no cover — defensive
-        result["error"] = str(exc)
+        # Same channel as the parse failure above: spawn exceptions routinely
+        # embed argv (OSError stringifies the program path), and argv[0] can be
+        # the credential itself. Exception CLASS only.
+        result["error"] = f"spawn failed ({type(exc).__name__})"
+        # Operator channel only — OSError stringifies argv/the program path,
+        # and this is what distinguishes EACCES from ENOEXEC from EMFILE.
+        result["error_detail"] = f"spawn failed: {exc}"
         return result
 
     try:
@@ -619,7 +650,11 @@ def _spawn(spec: ShellHookSpec, stdin_json: str) -> Dict[str, Any]:
             proc.communicate(timeout=1)
         except Exception:
             pass
-        result["error"] = str(exc)
+        # Model-facing via _fail_closed_block — exception CLASS only, never
+        # text that may carry argv or payload content.
+        result["error"] = f"communication failed ({type(exc).__name__})"
+        # Operator channel only — may carry argv or payload content.
+        result["error_detail"] = f"communication failed: {exc}"
         return result
 
     result["returncode"] = proc.returncode
@@ -758,9 +793,12 @@ def _evaluate_result(
     fail_closed = spec.fail_closed and blocking_event
 
     if r["error"]:
+        # Log/operator channel gets the DETAILED reason when there is one; the
+        # model-facing block below keeps the redacted `error`. This log line
+        # already carries `spec.command`, so it is not a new disclosure.
         logger.warning(
             "shell hook failed (event=%s command=%s): %s",
-            spec.event, spec.command, r["error"],
+            spec.event, spec.command, r.get("error_detail") or r["error"],
         )
         if fail_closed:
             return _fail_closed_block(spec, r["error"])

hermes_cli/hooks.py

@@ -293,7 +293,9 @@ def _cmd_test(args) -> None:
 
 def _print_run_result(result: Dict[str, Any]) -> None:
     if result.get("error"):
-        print(f"      ✗ error: {result['error']}")
+        # Operator channel: prefer the detailed reason. `error` is the redacted
+        # model-facing form and cannot distinguish e.g. EACCES from ENOEXEC.
+        print(f"      ✗ error: {result.get('error_detail') or result['error']}")
         return
     if result.get("timed_out"):
         print(f"      ✗ timed out after {result['elapsed_seconds']}s")
@@ -417,7 +419,9 @@ def _doctor_one(spec, shell_hooks) -> int:
                   f"on synthetic payload (timeout={spec.timeout}s)")
         elif result.get("error"):
             problems += 1
-            print(f"      ✗ execution error: {result['error']}")
+            # Operator channel — see _print_run_result.
+            detail = result.get("error_detail") or result["error"]
+            print(f"      ✗ execution error: {detail}")
         else:
             rc = result.get("returncode")
             elapsed = result.get("elapsed_seconds", 0)

hermes_cli/plugins.py

@@ -473,6 +473,54 @@ def _install_plugin_debug_handler(force: bool = False) -> None:
     "timeout (retry in {remaining:.0f}s)"
 )
 
+
+def _callback_label(callback: Any) -> str:
+    """Return a stable, secret-free identifier for a plugin *callback*.
+
+    Callback labels reach the MODEL through the fail-closed refusal messages
+    above, so this must never fall back to ``repr()``: a callback that carries
+    credentials in its state — the canonical case is
+    ``functools.partial(fn, api_token)``, whose repr renders every bound
+    argument — would interpolate them straight into a model-visible string.
+    ``__name__``/``__qualname__`` are usually developer-chosen identifiers, but
+    they are NOT safe by construction: this codebase synthesizes them from
+    operator config (``agent.shell_hooks`` and ``agent.outbound_webhooks`` both
+    do), and a third-party plugin can set any value through
+    ``register_hook()``. Each PRODUCER of a synthesized identity is therefore
+    responsible for sanitizing it at the source — see
+    :func:`agent.shell_hooks.hook_display_name` and
+    :attr:`agent.outbound_webhooks.WebhookTarget.display_label`. This function
+    is the last line of defence, not the only one: it refuses ``repr()`` so an
+    unsanitized callback still cannot render its bound state.
+
+    Anything without a usable name is identified by TYPE plus a short digest of
+    that type — never ``id()``, whose value is a CPython heap address that
+    changes per process and is recycled after GC.
+    """
+    for attr in ("__qualname__", "__name__"):
+        label = getattr(callback, attr, None)
+        if isinstance(label, str) and label:
+            return label
+    # functools.partial and friends: the WRAPPED function's name is a safe
+    # developer identifier, while the partial's bound args are exactly what
+    # must not be rendered. Name the wrapped callable, never the arguments.
+    wrapped = getattr(callback, "func", None)
+    if wrapped is not None:
+        for attr in ("__qualname__", "__name__"):
+            label = getattr(wrapped, attr, None)
+            if isinstance(label, str) and label:
+                return f"{type(callback).__name__}({label})"
+    # Last resort: no usable name anywhere. Identify by TYPE plus a short
+    # digest of its fully-qualified type, NOT id() — a heap address differs
+    # between processes and is recycled after GC, so it is neither stable nor
+    # meaningful to whoever reads the refusal. The digest keeps two distinct
+    # anonymous callback types apart without disclosing any instance state.
+    cls = type(callback)
+    fqtn = f"{getattr(cls, '__module__', '?')}.{getattr(cls, '__qualname__', cls.__name__)}"
+    digest = hashlib.sha256(fqtn.encode("utf-8", "replace")).hexdigest()[:8]
+    return f"<{cls.__name__}#{digest}>"
+
+
 ENTRY_POINTS_GROUP = "hermes_agent.plugins"
 ENTRY_POINT_CAPABILITIES_GROUP = "hermes_agent.plugin_capabilities"
 
@@ -5706,7 +5754,7 @@ def invoke_hook(self, hook_name: str, **kwargs: Any) -> List[Any]:
         timeout = _resolve_hook_callback_timeout()
         use_timeout = _hook_uses_callback_timeout(hook_name, timeout)
         for cb in callbacks:
-            callback_name = getattr(cb, "__name__", repr(cb))
+            callback_name = _callback_label(cb)
             callback_key = (hook_name, id(cb))
             fail_closed = bool(
                 getattr(
@@ -5952,7 +6000,7 @@ def _deliver_event(self, item: _QueuedPluginEvent) -> None:
                     logger.warning(
                         "Event '%s' subscriber %s raised: %s",
                         item.event,
-                        getattr(callback, "__name__", repr(callback)),
+                        _callback_label(callback),
                         exc,
                     )
         finally:
@@ -6138,7 +6186,7 @@ def invoke_middleware(self, kind: str, **kwargs: Any) -> List[Any]:
                 logger.warning(
                     "Middleware '%s' callback %s raised: %s",
                     kind,
-                    getattr(cb, "__name__", repr(cb)),
+                    _callback_label(cb),
                     exc,
                 )
         return results

tests/agent/test_shell_hooks.py

@@ -9,6 +9,7 @@
 from __future__ import annotations
 
 import json
+import logging
 from pathlib import Path
 
 import pytest
@@ -655,11 +656,88 @@ def _spawn_result(**overrides):
         "timed_out": False,
         "elapsed_seconds": 0.1,
         "error": None,
+        "error_detail": None,
     }
     base.update(overrides)
     return base
 
 
+class TestErrorChannelSplit:
+    """``error`` is redacted for the model; ``error_detail`` is for operators.
+
+    Collapsing the model-facing reason to an exception CLASS closed a real
+    disclosure, but it also blinded ``hermes hooks test`` / ``doctor`` and the
+    log: a shlex ``No closing quotation`` and a spawn ``EACCES``/``ENOEXEC``
+    became indistinguishable to whoever has to fix the hook. The two channels
+    must carry different text, and only the redacted one may reach the model.
+    """
+
+    def test_unparseable_command_detail_is_operator_only(self):
+        planted = "hunter2PRODSup3rSecret"
+        spec = shell_hooks.ShellHookSpec(
+            event="pre_tool_call",
+            command=f"/bin/sh -c 'export TOK={planted}; echo hi",
+            fail_closed=True,
+        )
+
+        r = shell_hooks._spawn(spec, "{}")
+
+        # Operator channel keeps the diagnostic reason ...
+        assert r["error_detail"], "the operator channel lost the failure reason"
+        assert "No closing quotation" in r["error_detail"], (
+            "the operator can no longer tell WHY the command failed to parse"
+        )
+        # ... while the model-facing channel stays exception-class only.
+        assert r["error"] == "command cannot be parsed (ValueError)"
+        assert planted not in r["error"]
+
+        decision = shell_hooks._evaluate_result(spec, r)
+        assert decision is not None and decision["action"] == "block"
+        assert planted not in decision["message"], (
+            "error_detail leaked into the model-facing refusal"
+        )
+        assert "No closing quotation" not in decision["message"], (
+            "the refusal carried operator-channel text to the model"
+        )
+
+    def test_spawn_failure_detail_distinguishes_errno(self, tmp_path):
+        """EACCES vs ENOEXEC must stay distinguishable on the operator channel."""
+        not_executable = tmp_path / "hook.sh"
+        not_executable.write_text("#!/bin/sh\necho hi\n")
+        not_executable.chmod(0o644)
+
+        spec = shell_hooks.ShellHookSpec(
+            event="pre_tool_call", command=str(not_executable), fail_closed=True,
+        )
+        r = shell_hooks._spawn(spec, "{}")
+
+        assert r["error"], "a non-executable hook should fail"
+        # Whatever the platform raises, the operator channel must name it more
+        # specifically than the redacted channel does.
+        if r["error_detail"] is not None:
+            assert r["error_detail"] != r["error"]
+            assert len(r["error_detail"]) > len(r["error"])
+
+    def test_evaluate_result_logs_detail_but_blocks_with_redacted(self, caplog):
+        spec = shell_hooks.ShellHookSpec(
+            event="pre_tool_call", command="/tmp/h.sh", fail_closed=True,
+        )
+        r = _spawn_result(
+            error="spawn failed (OSError)",
+            error_detail="spawn failed: [Errno 13] Permission denied: '/tmp/h.sh'",
+        )
+
+        with caplog.at_level(logging.WARNING):
+            decision = shell_hooks._evaluate_result(spec, r)
+
+        assert "Errno 13" in caplog.text, "the log lost the operator detail"
+        assert decision is not None and decision["action"] == "block"
+        assert "Errno 13" not in decision["message"], (
+            "the model-facing refusal carried operator-channel detail"
+        )
+        assert "spawn failed (OSError)" in decision["message"]
+
+
 class TestEvaluateResult:
     def _spec(self, event="pre_tool_call", fail_closed=False):
         return shell_hooks.ShellHookSpec(