From 2dfb795cb78f2cbb880215c30c666512b46b250b Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Tue, 15 Sep 2026 20:05:00 -0700 Subject: [PATCH] fix(approval): undelivered or unanswered CLI approval prompts are not user denials MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the CLI approval callback raises, when no callback is registered on the thread while prompt_toolkit owns the terminal, or when the input() read is interrupted, prompt_dangerous_approval returned "deny" and the command gate rendered "BLOCKED: User denied this command" — attributing a refusal to a user who was never asked (#22992). #112308 fixed the gateway half of the class (withdrawn prompts -> outcome "cancelled" with a cause); this closes the CLI residual on the same shape. - tools/approval_prompt.py: those three paths return an Unanswered("cancelled") sentinel carrying the cause; MCP elicitation consent maps it to "cancel". - tools/approval.py: the CLI gate renders "BLOCKED: was not approved: the approval prompt could not be delivered or was not answered ()" with outcome "cancelled" — still fail-closed, "Silence is not consent". - tools/file_tools_write_guards.py: the protected-instruction write gate reports the undelivered prompt instead of "was denied by the user". - Shared metrics: "cancelled" is a counted approval outcome (contract + v2 schema) instead of falling into "unknown". - Docs: hook `choice="cancelled"` now covers the CLI causes. Fixes #22992 --- .../hermes.shared_metrics.v2.schema.json | 2 ++ .../observability/shared_metrics_contract.py | 3 +- tests/hermes_cli/test_relay_shared_metrics.py | 1 + tests/tools/test_approval.py | 4 +-- .../test_approval_cancelled_attribution.py | 26 ++++++++++++++ tools/approval.py | 9 ++++- tools/approval_prompt.py | 35 +++++++++++++------ tools/file_tools_write_guards.py | 3 ++ .../docs/developer-guide/observer-hooks.md | 4 +-- website/docs/user-guide/features/hooks.md | 2 +- 10 files changed, 72 insertions(+), 17 deletions(-) diff --git a/hermes_cli/observability/schemas/hermes.shared_metrics.v2.schema.json b/hermes_cli/observability/schemas/hermes.shared_metrics.v2.schema.json index 08c01d8fa7..114acc92d3 100644 --- a/hermes_cli/observability/schemas/hermes.shared_metrics.v2.schema.json +++ b/hermes_cli/observability/schemas/hermes.shared_metrics.v2.schema.json @@ -493,6 +493,7 @@ "approval_outcome": { "enum": [ "approved", + "cancelled", "denied", "not_required", "timed_out", @@ -578,6 +579,7 @@ "outcome": { "enum": [ "approved", + "cancelled", "denied", "timed_out", "unknown" diff --git a/hermes_cli/observability/shared_metrics_contract.py b/hermes_cli/observability/shared_metrics_contract.py index d864d4a342..b28b8bd752 100644 --- a/hermes_cli/observability/shared_metrics_contract.py +++ b/hermes_cli/observability/shared_metrics_contract.py @@ -61,7 +61,7 @@ TOOL_CATEGORIES = frozenset({ "skill", "terminal", "unknown", "web", }) TOOL_OUTCOMES = frozenset({"blocked", "cancelled", "failed", "success", "timed_out", "unknown"}) -TOOL_APPROVAL_OUTCOMES = frozenset({"approved", "denied", "not_required", "timed_out", "unknown"}) +TOOL_APPROVAL_OUTCOMES = frozenset({"approved", "cancelled", "denied", "not_required", "timed_out", "unknown"}) TOOL_APPROVAL_ATTRIBUTIONS = frozenset({"tool_call", "unattributed"}) TOOL_LATENCY_BUCKETS = frozenset({ "100ms_to_250ms", "10s_to_30s", "1s_to_2s", "250ms_to_500ms", "2s_to_5s", "500ms_to_1s", @@ -548,6 +548,7 @@ _APPROVAL_CHOICES = { ), **dict.fromkeys(("deny", "denied", "smart_deny"), "denied"), **dict.fromkeys(("timed_out", "timeout"), "timed_out"), + "cancelled": "cancelled", # prompt withdrawn / undeliverable / unanswered — not a user decision } diff --git a/tests/hermes_cli/test_relay_shared_metrics.py b/tests/hermes_cli/test_relay_shared_metrics.py index ec643f3db4..1586429982 100644 --- a/tests/hermes_cli/test_relay_shared_metrics.py +++ b/tests/hermes_cli/test_relay_shared_metrics.py @@ -699,6 +699,7 @@ def test_tool_outcome_is_bounded(status, expected): ("deny", "denied"), ("smart_deny", "denied"), ("timeout", "timed_out"), + ("cancelled", "cancelled"), (None, "unknown"), ], ) diff --git a/tests/tools/test_approval.py b/tests/tools/test_approval.py index bc14129cf9..7a0524273d 100644 --- a/tests/tools/test_approval.py +++ b/tests/tools/test_approval.py @@ -1217,7 +1217,7 @@ class TestFailClosedUnderPromptToolkit: When prompt_toolkit owns the terminal and no approval callback is registered on the calling thread, prompt_dangerous_approval() must - deny fast instead of falling through to the input() fallback -- which + fail closed fast instead of falling through to the input() fallback -- which deadlocks because the user's keystrokes go to prompt_toolkit's raw-mode stdin capture, not to input(). """ @@ -1246,7 +1246,7 @@ class TestFailClosedUnderPromptToolkit: "prompt_dangerous_approval deadlocked under prompt_toolkit " "with no callback -- fail-closed guard is broken" ) - assert result == ["deny"] + assert result == ["cancelled"] # unanswered, not a user denial (#22992) finally: ptc.get_app_or_none = orig diff --git a/tests/tools/test_approval_cancelled_attribution.py b/tests/tools/test_approval_cancelled_attribution.py index 92d8415504..b62fc16f14 100644 --- a/tests/tools/test_approval_cancelled_attribution.py +++ b/tests/tools/test_approval_cancelled_attribution.py @@ -127,3 +127,29 @@ def test_coalesced_follower_inherits_the_leaders_cancellation(gateway_session): assert not leader_thread.is_alive() and not follower_thread.is_alive() _assert_withdrawn(leader["result"], "parent delegation ended") _assert_withdrawn(follower["result"], "parent delegation ended") + + +@pytest.fixture +def cli_session(monkeypatch): + mod._session_approved.clear() + for k in ("HERMES_CRON_SESSION", "HERMES_YOLO_MODE", "HERMES_GATEWAY_SESSION", "HERMES_EXEC_ASK"): + monkeypatch.delenv(k, raising=False) + monkeypatch.setenv("HERMES_INTERACTIVE", "1") + monkeypatch.setattr(mod, "_YOLO_MODE_FROZEN", False) + monkeypatch.setattr(approval_context, "_get_approval_config", lambda: {"mode": "manual", "timeout": 60}) + hooks = [] + monkeypatch.setattr(approval_context, "_fire_approval_hook", lambda name, **kw: hooks.append((name, kw))) + return hooks + + +def test_cli_callback_failure_reports_undelivered_prompt_not_user_deny(cli_session): + """The CLI residual of #22992: a prompt that never reached a human (the approval callback + raised) is 'cancelled' with its cause, not 'User denied this command'.""" + def broken_callback(command, description, **kwargs): + raise TypeError("callback signature mismatch") + + result = mod.check_all_command_guards("rm -rf .git", "local", approval_callback=broken_callback) + _assert_withdrawn(result, "the approval callback failed: TypeError") + assert "user denied" not in result["message"].lower() + posts = [kw for name, kw in cli_session if name == "post_approval_response"] + assert posts[-1]["choice"] == "cancelled" diff --git a/tools/approval.py b/tools/approval.py index 899386b3e3..1f95f65685 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -483,7 +483,7 @@ _USER_SUMMARIES = { "denied": "You denied this {noun} — it did not run.", "timeout": "No answer within {minutes} — the {noun} did not run.", "notify_failed": "The approval request could not be delivered — the {noun} did not run.", - "cancelled": "The approval prompt was withdrawn before you answered — the {noun} did not run.", + "cancelled": "The approval prompt was withdrawn or never reached you — the {noun} did not run.", "blocked": "This {noun} is not allowed in an unattended session — it did not run.", } @@ -894,6 +894,13 @@ def _human_decision(spec: _GateSpec, *, command: str, description: str, approval_context._fire_approval_hook("post_approval_response", **hook_kwargs, choice=choice) if choice == "timeout": return deny(spec.cli_timeout, "timeout") + if choice == "cancelled": + # The prompt never reached a human (callback raised, no callback under prompt_toolkit, interrupted + # read): fail closed, but do not attribute a refusal to the user (#22992). + return deny(spec.gateway_refused, "cancelled", + reason="was not approved: the approval prompt could not be delivered or was not answered " + f"({getattr(choice, 'cause', 'no answer')})", + reason_addendum="", timeout_addendum=" Silence is not consent.", deny_reason=None) if choice == "deny": # No _record_denial(): the breaker counts consecutive guardian LLM # DENY verdicts, not deliberate human denials. diff --git a/tools/approval_prompt.py b/tools/approval_prompt.py index a681e3de68..85129a16a9 100644 --- a/tools/approval_prompt.py +++ b/tools/approval_prompt.py @@ -35,9 +35,11 @@ def prompt_dangerous_approval(command: str, description: str, timeout_seconds: i allow_permanent=True, allow_session=True, smart_denied=False) -> str``; legacy signatures keep working while both keywords hold their defaults. - Returns 'once', 'session', 'always', 'deny', or 'timeout'. 'timeout' means no - user response — still blocked (fail-closed), but callers report "no response" - rather than an explicit denial. + Returns 'once', 'session', 'always', 'deny', 'timeout', or 'cancelled'. 'timeout' + means no user response — still blocked (fail-closed), but callers report "no + response" rather than an explicit denial. 'cancelled' is an :class:`Unanswered` + sentinel: the prompt never reached a human (callback raised, no callback under + prompt_toolkit, interrupted read) and ``.cause`` says why (#22992). See #81887. """ @@ -51,6 +53,18 @@ def prompt_dangerous_approval(command: str, description: str, timeout_seconds: i approval_callback, allow_session, smart_denied, title=title) +class Unanswered(str): + """Choice ``"cancelled"`` carrying the reason nobody answered. Compares equal to the gateway's + withdrawn-prompt choice so every consumer already handling ``cancelled`` fails closed without + attributing a denial to the user.""" + __slots__ = ("cause",) + + def __new__(cls, cause: str): + self = super().__new__(cls, "cancelled") + self.cause = cause + return self + + _CLI_CHOICE_ALIASES = { "o": "once", "once": "once", "s": "session", "session": "session", @@ -112,14 +126,14 @@ def _ask_human(command: str, description: str, timeout_seconds: int, allow_perma return approval_callback(display_command, display_description, **callback_kwargs) except Exception as e: logger.error("Approval callback failed: %s", e, exc_info=True) - return "deny" + return Unanswered(f"the approval callback failed: {type(e).__name__}") # Fail-closed guard: when prompt_toolkit owns the terminal and no callback is registered on this thread, the # input() fallback would spawn a daemon thread whose read never sees Enter (keystrokes go to prompt_toolkit) — an - # invisible deadlock. Deny loudly instead; threads needing interactive approval must install a callback via + # invisible deadlock. Fail closed loudly instead; threads needing interactive approval must install a callback via # tools.terminal_tool.set_approval_callback() first. try: - # Deny fast and log loudly instead so the caller can surface a real error to the agent. Any thread + # Fail fast and log loudly so the caller can surface a real error to the agent. Any thread # that needs interactive approval must install a callback via # tools.terminal_tool.set_approval_callback() before reaching this point (see delegate_tool.py, # run_agent.py _execute_tool_calls_concurrent / _spawn_background_review for the established @@ -127,9 +141,10 @@ def _ask_human(command: str, description: str, timeout_seconds: int, allow_perma from prompt_toolkit.application.current import get_app_or_none if get_app_or_none() is not None: logger.warning("Dangerous-command approval requested on a thread with no " - "approval callback while prompt_toolkit is active; denying " + "approval callback while prompt_toolkit is active; failing closed " "to avoid stdin deadlock. command=%r description=%r", command, description) - return "deny" + return Unanswered("no approval callback is registered on this thread while prompt_toolkit owns " + "the terminal, so the prompt could not be shown") except Exception: pass # prompt_toolkit absent or detection failed: legacy input() path is safe @@ -159,7 +174,7 @@ def _ask_human(command: str, description: str, timeout_seconds: int, allow_perma return decision except (EOFError, KeyboardInterrupt): print("\n" + t("approval.cancelled")) - return "deny" + return Unanswered("the prompt was interrupted before an answer was given") finally: os.environ.pop("HERMES_SPINNER_PAUSE", None) print() @@ -263,7 +278,7 @@ def _consent(choice, unresolved: str) -> str: """Map an approval choice to an elicitation verdict; *unresolved* is the no-answer outcome.""" if choice in ("once", "session", "always"): return "accept" - return unresolved if choice == "timeout" else "decline" + return unresolved if choice in ("timeout", "cancelled") else "decline" def request_elicitation_consent(message: str, description: str, *, diff --git a/tools/file_tools_write_guards.py b/tools/file_tools_write_guards.py index 9288e64d3d..20acc57487 100644 --- a/tools/file_tools_write_guards.py +++ b/tools/file_tools_write_guards.py @@ -311,6 +311,9 @@ def _request_protected_instruction_approval(reasons: list[str], task_id: str = " return blocked.format(why=_NO_HUMAN) choice = prompt_dangerous_approval( display, description, allow_permanent=False, allow_session=False, approval_callback=callback) + if choice == "cancelled": + return blocked.format(why="approval prompt could not be delivered or was not answered " + f"({getattr(choice, 'cause', 'no answer')}).") timed = choice == "timeout" # Any tapped scope is a one-operation grant; nothing is persisted. if not timed and choice in {"once", "session", "always"}: diff --git a/website/docs/developer-guide/observer-hooks.md b/website/docs/developer-guide/observer-hooks.md index 5c223a8886..6a7d1389fb 100644 --- a/website/docs/developer-guide/observer-hooks.md +++ b/website/docs/developer-guide/observer-hooks.md @@ -206,8 +206,8 @@ Common fields include `command`, `description`, `pattern_key`, `pattern_keys`, `session_key`, and `surface`. `post_approval_response` also includes `choice`, with values such as `once`, -`session`, `always`, `deny`, `timeout`, and `cancelled` (prompt withdrawn before an -answer — turn interrupted or ended). +`session`, `always`, `deny`, `timeout`, and `cancelled` (nobody answered: the prompt +was withdrawn — turn interrupted or ended — or never reached the user on the CLI). Approval hooks are observer-only. Plugins cannot pre-answer or veto approvals from these hooks. To prevent a tool from reaching approval, use diff --git a/website/docs/user-guide/features/hooks.md b/website/docs/user-guide/features/hooks.md index e635add680..6d9ac48a47 100644 --- a/website/docs/user-guide/features/hooks.md +++ b/website/docs/user-guide/features/hooks.md @@ -1372,7 +1372,7 @@ Same kwargs as `pre_approval_request`, plus: | Parameter | Type | Description | |-----------|------|-------------| -| `choice` | `str` | Prompted surfaces use `"once"`, `"session"`, `"always"`, `"deny"`, `"timeout"`, `"cancelled"` (the prompt was withdrawn before anyone answered — turn interrupted or ended; the command did not run), or `"notify_failed"`; smart decisions use `"smart_approve"` or `"smart_deny"` | +| `choice` | `str` | Prompted surfaces use `"once"`, `"session"`, `"always"`, `"deny"`, `"timeout"`, `"cancelled"` (nobody answered — the prompt was withdrawn because the turn was interrupted or ended, or on the CLI it never reached the user because the approval callback failed, no callback was registered under prompt_toolkit, or the read was interrupted; the command did not run), or `"notify_failed"`; smart decisions use `"smart_approve"` or `"smart_deny"` | | `decided_by` | `str` | `"aux_llm"` for smart decisions; absent on prompted surfaces | **Return value:** ignored.