From cb0ca31cf356f6bc5433e23a54e206ab9076e58a Mon Sep 17 00:00:00 2001 From: mintaka Date: Sun, 8 Nov 2026 19:26:46 -0500 Subject: [PATCH 1/4] fix(approval): a reviewer DENY needs its own human answer (RIG-8544) Co-authored-by: Matt Wilkinson --- hermes_cli/cli_modal_mixin.py | 3 +- hermes_cli/cli_tui_mixin.py | 55 ++++++---- .../fixtures/plugin_origin_approval_peer.py | 4 +- tests/hermes_cli/test_cli_approval_ui.py | 22 ++++ ...est_approval_gateway_wait_coalesce_race.py | 100 ++++++++++++++++++ tests/tools/test_plugin_approval_review.py | 89 +++++++++++++++- tools/approval.py | 28 +++-- tools/approval_gateway_wait.py | 8 +- tools/delegate_tool_config.py | 13 ++- 9 files changed, 280 insertions(+), 42 deletions(-) diff --git a/hermes_cli/cli_modal_mixin.py b/hermes_cli/cli_modal_mixin.py index 684d29fc793c..074e82fff64f 100644 --- a/hermes_cli/cli_modal_mixin.py +++ b/hermes_cli/cli_modal_mixin.py @@ -1091,8 +1091,8 @@ def _approval_callback(self, command: str, description: str, from cli import CLI_CONFIG, _DIM, _RST, _cprint with self._approval_lock: - timeout = int(CLI_CONFIG.get("approvals", {}).get("timeout", 300)) response_queue = queue.Queue() + timeout = int(CLI_CONFIG.get("approvals", {}).get("timeout", 300)) self._approval_state = { "command": command, "description": description, @@ -1102,6 +1102,7 @@ def _approval_callback(self, command: str, description: str, allow_session=allow_session, smart_denied=smart_denied), "selected": 0, + "smart_denied": smart_denied, "response_queue": response_queue} self._approval_deadline = _time.monotonic() + timeout self._ring_bell(prompt=True, context=t("cli.approval.bell_context"), detail=command) diff --git a/hermes_cli/cli_tui_mixin.py b/hermes_cli/cli_tui_mixin.py index a456e92ac6b1..e17c9b95e96d 100644 --- a/hermes_cli/cli_tui_mixin.py +++ b/hermes_cli/cli_tui_mixin.py @@ -199,6 +199,16 @@ def _get_approval_display_fragments(self): wrap = _wrap_panel_text_keep_ws command = state["command"] description = state["description"] + reviewer_warning = None + if state.get("smart_denied"): + description_lines = description.splitlines() + warning_index = next(( + i for i, line in enumerate(description_lines) + if line.startswith("**Reviewer:** flagged as dangerous") + ), None) + if warning_index is not None: + reviewer_warning = description_lines.pop(warning_index) + description = "\n".join(description_lines) choices = state["choices"] selected = state.get("selected", 0) show_full = state.get("show_full", False) @@ -213,7 +223,7 @@ def _get_approval_display_fragments(self): box_width = _panel_box_width(title, preview_lines) inner_text_width = max(8, box_width - 2) - # Pre-wrap the mandatory content — command + choices must always render. + # Pre-wrap mandatory content so the reviewer warning, command and choices stay visible. cmd_wrapped = wrap(command, inner_text_width) if not show_full and "view" in choices and len(cmd_wrapped) > 4: cmd_wrapped = cmd_wrapped[:3] + wrap( @@ -222,37 +232,42 @@ def _get_approval_display_fragments(self): f"{'❯' if i == selected else ' '} {_num_prefix(i)}. {_approval_choice_label(choice)}" for i, choice in enumerate(choices)] choice_wrapped = _wrap_rows(wrap, choice_labels, inner_text_width, " ") + warning_wrapped = wrap(reviewer_warning, inner_text_width) if reviewer_warning else [] - # Row budget so HSplit never clips the command or choices. Full chrome = top border + - # title + blank + blank-between-cmd/choices + bottom border (5); when that doesn't fit, - # drop the separator blanks (3) so every choice stays on-screen in compact terminals. + # Inline the title on compact reviewer-DENY panels to reserve a visible warning row. available = max(0, _term_rows() - _PANEL_RESERVED_BELOW) - use_compact_chrome = 5 + len(cmd_wrapped) + len(choice_wrapped) > available - chrome_rows = 3 if use_compact_chrome else 5 - - # A command too long to leave room for the choices (e.g. "view" on a multi-hundred-char - # command) is truncated so approve/deny still render; keep at least 1 command row. - max_cmd_rows = max(1, available - chrome_rows - len(choice_wrapped)) - if len(cmd_wrapped) > max_cmd_rows: + use_compact_chrome = 5 + len(cmd_wrapped) + len(choice_wrapped) + bool(warning_wrapped) > available + chrome_rows = (2 if warning_wrapped else 3) if use_compact_chrome else (4 if warning_wrapped else 5) + + # Keep the warning first, then truncate the command before dropping its row. + warning_budget = max(1, available - chrome_rows - len(choice_wrapped) - 1) + warning_wrapped = warning_wrapped[:warning_budget] + max_cmd_rows = available - chrome_rows - len(choice_wrapped) - len(warning_wrapped) + if max_cmd_rows < 1: + cmd_wrapped = [] + elif len(cmd_wrapped) > max_cmd_rows: keep = max(1, max_cmd_rows - 1) if max_cmd_rows > 1 else 1 cmd_wrapped = cmd_wrapped[:keep] + wrap( t("cli.tui.approval_command_truncated"), inner_text_width) - # Remaining rows go to the description (minus the blank separator in full mode), capped - # at 10 so the panel stays compact even on huge terminals. - mandatory_no_desc = chrome_rows + len(cmd_wrapped) + len(choice_wrapped) + # Remaining rows go to the description (minus its blank separator), capped at 10. + mandatory_no_desc = chrome_rows + len(warning_wrapped) + len(cmd_wrapped) + len(choice_wrapped) available_for_desc = available - mandatory_no_desc - (0 if use_compact_chrome else 1) available_for_desc = max(0, min(available_for_desc, 10)) desc_wrapped = wrap(description, inner_text_width) if description else [] if available_for_desc < 1 or not desc_wrapped: desc_wrapped = [] elif len(desc_wrapped) > available_for_desc: - desc_wrapped = desc_wrapped[:max(1, available_for_desc - 1)] + [t("cli.tui.approval_description_truncated")] - - # Render title → command → choices → description; description last so any overflow - # clips the least-critical content, never the command or choices. - panel = _Panel('class:approval-border', box_width) - panel.row('class:approval-title', title) + desc_wrapped = desc_wrapped[:max(1, available_for_desc - 1)] + [ + t("cli.tui.approval_description_truncated")] + # Render the warning before the command; compact chrome integrates the title into its border. + panel = _Panel('class:approval-border', box_width, + title if use_compact_chrome and warning_wrapped else "", + 'class:approval-title') + if not (use_compact_chrome and warning_wrapped): + panel.row('class:approval-title', title) + for wrapped in warning_wrapped: + panel.row('class:approval-desc', wrapped) if not use_compact_chrome: panel.blank() for wrapped in cmd_wrapped: diff --git a/tests/gateway/fixtures/plugin_origin_approval_peer.py b/tests/gateway/fixtures/plugin_origin_approval_peer.py index fca9d138eafb..a00336cdb4b6 100644 --- a/tests/gateway/fixtures/plugin_origin_approval_peer.py +++ b/tests/gateway/fixtures/plugin_origin_approval_peer.py @@ -14,7 +14,7 @@ CASES = { "review-approve": {"hook": True, "review": True, "answer": "once"}, "review-escalate": {"hook": True, "review": True, "verdict": "escalate", "answer": "once"}, - "review-deny": {"hook": True, "review": True, "verdict": "deny", "answer": None}, + "review-deny": {"hook": True, "review": True, "verdict": "deny", "answer": "once"}, "human-deny": {"hook": True, "review": True, "answer": "deny"}, "timeout": {"hook": True, "review": True, "answer": "timeout"}, "stale-always": {"hook": True, "review": True, "answer": "always"}, @@ -203,6 +203,8 @@ async def answer_cards(*, answer, expected, before, settled): assert set(prompt.choices) == {"once", "deny"}, prompt.choices assert "wants to run" in prompt.text.lower(), prompt.text assert adapter._approval_mention_content() == "<@12345>" + if case_name == "review-deny": + assert "flagged as dangerous" in prompt.text.lower(), prompt.text if answer not in {None, "timeout"}: assert answer in {"once", "deny", "always"} assert resolve_gateway_approval(session_key, answer) == 1 diff --git a/tests/hermes_cli/test_cli_approval_ui.py b/tests/hermes_cli/test_cli_approval_ui.py index feec4091d651..46594988dbe2 100644 --- a/tests/hermes_cli/test_cli_approval_ui.py +++ b/tests/hermes_cli/test_cli_approval_ui.py @@ -214,6 +214,28 @@ def test_approval_display_truncates_giant_command_in_view_mode(self): # Command got truncated with a marker. assert t("cli.tui.approval_command_truncated") in rendered + + def test_reviewer_deny_warning_is_visible_with_long_reason_and_short_terminal(self): + cli = _make_cli_stub() + cli._approval_state = { + "command": "guarded action", + "description": "plugin reason " + ("x" * 1000) + + "\n**Reviewer:** flagged as dangerous: reviewer reason " + ("y" * 1000), + "choices": ["once", "deny"], + "selected": 0, + "smart_denied": True, + "response_queue": queue.Queue(), + } + + import shutil as _shutil + + with patch("cli.shutil.get_terminal_size", + return_value=_shutil.os.terminal_size((60, 12))): + fragments = cli._get_approval_display_fragments() + + rendered = "".join(text for _style, text in fragments) + assert "flagged as dangerous" in rendered + def test_background_task_registers_thread_local_approval_callbacks(self): """Background /btw tasks must use the prompt_toolkit approval UI. diff --git a/tests/tools/test_approval_gateway_wait_coalesce_race.py b/tests/tools/test_approval_gateway_wait_coalesce_race.py index a49a76ec5efe..7dca611edae5 100644 --- a/tests/tools/test_approval_gateway_wait_coalesce_race.py +++ b/tests/tools/test_approval_gateway_wait_coalesce_race.py @@ -203,3 +203,103 @@ def run(): thread.join(0.05) assert (prompts, queued) == (2, 1) assert [r["choice"] for r in results] == ["once"] * 3 + + +def test_smart_denied_follower_gets_its_own_prompt(monkeypatch): + _clear() + monkeypatch.setattr(wait_mod._ctx, "_fire_approval_hook", lambda name, **kwargs: None) + monkeypatch.setattr(wait_mod._ctx, "_get_approval_timeout", lambda: APPROVAL_TIMEOUT) + leader_notified = threading.Event() + follower_notified = threading.Event() + results = {} + + def notify_leader(data): + results["leader_id"] = data["request_id"] + leader_notified.set() + + def notify_follower(data): + results["follower_id"] = data["request_id"] + follower_notified.set() + + def run(name, notify, data): + results[name] = wait_mod._await_gateway_decision(SESSION_KEY, notify, data) + + leader = threading.Thread(target=run, args=("leader", notify_leader, dict(APPROVAL)), daemon=True) + denied_data = {**APPROVAL, "description": "reviewer flagged the request", "smart_denied": True} + follower = threading.Thread(target=run, args=("follower", notify_follower, denied_data), daemon=True) + leader.start() + assert leader_notified.wait(10), "ordinary leader prompt was not sent" + follower.start() + try: + assert follower_notified.wait(10), "DENY follower did not send its own prompt" + with mod._lock: + pending = list(mod._gateway_queues[SESSION_KEY]) + assert len(pending) == 2 + assert pending[1].data["smart_denied"] is True + + assert mod.resolve_gateway_approval(SESSION_KEY, "session", request_id=results["leader_id"]) == 1 + leader.join(10) + assert not leader.is_alive() + assert results["leader"]["choice"] == "session" + assert follower.is_alive(), "DENY follower adopted the leader's broad approval" + with mod._lock: + (waiting_deny,) = mod._gateway_queues[SESSION_KEY] + assert waiting_deny.data["request_id"] == results["follower_id"] + + assert mod.resolve_gateway_approval(SESSION_KEY, "once", request_id=results["follower_id"]) == 1 + follower.join(10) + assert not follower.is_alive() + assert results["follower"]["choice"] == "once" + finally: + mod.resolve_gateway_approval(SESSION_KEY, "deny", resolve_all=True) + leader.join(10) + follower.join(10) + + +def test_smart_denied_broad_gateway_reply_is_normalized_to_once(monkeypatch): + _clear() + monkeypatch.setattr(wait_mod._ctx, "_fire_approval_hook", lambda name, **kwargs: None) + result = {} + + def notify(data): + assert data["smart_denied"] is True + assert mod.resolve_gateway_approval(SESSION_KEY, "always") == 1 + + thread = threading.Thread( + target=lambda: result.update(wait_mod._await_gateway_decision( + SESSION_KEY, notify, {**APPROVAL, "smart_denied": True})), + daemon=True, + ) + thread.start() + thread.join(10) + + assert not thread.is_alive() + assert result["resolved"] is True + assert result["choice"] == "once" + + +def test_smart_denied_bulk_approval_waits_for_bulk_deny(): + _clear() + entry = wait_mod._ApprovalEntry({**APPROVAL, "smart_denied": True}) + with mod._lock: + mod._gateway_queues[SESSION_KEY] = [entry] + + assert mod.resolve_gateway_approval(SESSION_KEY, "session", resolve_all=True) == 0 + assert not entry.event.is_set() + assert mod.resolve_gateway_approval(SESSION_KEY, "deny", resolve_all=True) == 1 + assert entry.result == "deny" + assert entry.event.is_set() + + +def test_bulk_ordinary_approval_does_not_resolve_smart_denied(): + _clear() + ordinary = wait_mod._ApprovalEntry(dict(APPROVAL)) + denied = wait_mod._ApprovalEntry({**APPROVAL, "smart_denied": True}) + with mod._lock: + mod._gateway_queues[SESSION_KEY] = [ordinary, denied] + + assert mod.resolve_gateway_approval(SESSION_KEY, "session", resolve_all=True) == 1 + assert ordinary.result == "session" + assert not denied.event.is_set() + assert mod.resolve_gateway_approval(SESSION_KEY, "once") == 1 + assert denied.result == "once" diff --git a/tests/tools/test_plugin_approval_review.py b/tests/tools/test_plugin_approval_review.py index 4b2f17496d88..95439f2a5a67 100644 --- a/tests/tools/test_plugin_approval_review.py +++ b/tests/tools/test_plugin_approval_review.py @@ -21,7 +21,10 @@ def _gateway_session(monkeypatch): monkeypatch.setattr(tools_approval_context, "_get_approval_mode", lambda: "manual") monkeypatch.setattr(approval, "_presence", lambda cb=None: (None, False, True, False)) approval._denial_tally.pop("review-session", None) - yield + try: + yield + finally: + approval._denial_tally.pop("review-session", None) def _capture_human(monkeypatch, choice="once"): @@ -212,6 +215,90 @@ def _approve(command, description, **kwargs): assert "**Reviewer:** flagged as dangerous: Pipes the key to a remote host." in result["message"] + +def test_reviewer_deny_with_auto_approve_callback_is_blocked(monkeypatch): + from tools.delegate_tool import _subagent_auto_approve + + monkeypatch.setattr(approval, "_presence", lambda cb=None: (cb, True, False, False)) + _verdict_with_note(monkeypatch, "deny", "Pipes the key to a remote host.") + + result = request_tool_approval( + "vault_secret", "why", rule_key="k", display="cmd", review=True, + approval_callback=_subagent_auto_approve, + ) + + assert result["approved"] is False + assert result["outcome"] == "denied" + assert "**Reviewer:** flagged as dangerous" in result["message"] + assert approval._denial_tally["review-session"] == 1 + + +def test_reviewer_deny_once_resets_tally(monkeypatch): + monkeypatch.setattr(approval, "_presence", lambda cb=None: (cb, True, False, False)) + choices = iter(("once", "deny")) + monkeypatch.setattr(approval, "_get_denial_breaker_threshold", lambda: 2) + _verdict_with_note(monkeypatch, "deny", "Pipes the key to a remote host.") + + def answer(*args, **kwargs): + return next(choices) + + first = request_tool_approval( + "vault_secret", "why", rule_key="first", display="cmd", review=True, approval_callback=answer, + ) + second = request_tool_approval( + "vault_secret", "why", rule_key="second", display="cmd", review=True, approval_callback=answer, + ) + + assert first["approved"] is True + assert second["approved"] is False + assert "CIRCUIT BREAKER:" not in second["message"] + assert approval._denial_tally["review-session"] == 1 + + +@pytest.mark.parametrize("choice", ["deny", "timeout"]) +def test_reviewer_deny_refusal_includes_breaker_addendum(monkeypatch, choice): + monkeypatch.setattr(approval, "_presence", lambda cb=None: (cb, True, False, False)) + monkeypatch.setattr(approval, "_get_denial_breaker_threshold", lambda: 2) + approval._denial_tally["review-session"] = 1 + _verdict_with_note(monkeypatch, "deny", "Pipes the key to a remote host.") + result = request_tool_approval( + "vault_secret", "why", rule_key="k", display="cmd", review=True, + approval_callback=lambda *args, **kwargs: choice, + ) + + assert "CIRCUIT BREAKER:" in result["message"] + + +def test_reviewer_deny_broad_gateway_reply_is_once_and_not_saved(monkeypatch): + from tools import approval_context + + monkeypatch.setattr(approval, "_presence", lambda cb=None: (cb, False, True, False)) + _verdict_with_note(monkeypatch, "deny", "Pipes the key to a remote host.") + committed_choices = [] + persisted = [] + monkeypatch.setattr(approval_context, "_fire_approval_hook", + lambda name, **kwargs: committed_choices.append(kwargs["choice"]) + if name == "post_approval_response" else None) + monkeypatch.setattr(approval, "_persist_choice", lambda *args: persisted.append(args)) + + def reply(data): + assert data["smart_denied"] is True + assert data["allow_session"] is False + assert data["allow_permanent"] is False + assert approval.resolve_gateway_approval("review-session", "session") == 1 + + approval.register_gateway_notify("review-session", reply) + try: + result = request_tool_approval("vault_secret", "why", rule_key="k", display="cmd", review=True) + finally: + approval.unregister_gateway_notify("review-session") + + assert result["approved"] is True + assert committed_choices == ["once"] + assert persisted == [] + + + def test_a_stale_reason_does_not_leak_into_the_next_prompt(monkeypatch): from tools import approval_smart diff --git a/tools/approval.py b/tools/approval.py index 2d38c8c42e0f..4a1d01e1b388 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -161,8 +161,13 @@ def resolve_gateway_approval(session_key: str, choice: str, return 0 queue[:] = [entry for entry in queue if entry not in targets] elif resolve_all: - targets = list(queue) - queue.clear() + if choice == "deny": + targets = list(queue) + queue.clear() + else: + # Bulk approval cannot establish consent for an individual warning. + targets = [entry for entry in queue if not entry.data.get("smart_denied")] + queue[:] = [entry for entry in queue if entry.data.get("smart_denied")] else: targets = [queue.pop(0)] if not queue: @@ -171,7 +176,10 @@ def resolve_gateway_approval(session_key: str, choice: str, # ``_drop_entry`` reads ``entry.result`` under this same lock after its deadline check, so a # choice acked to the client here can never be popped-and-lost as a timeout (#112548). for entry in targets: - entry.result = choice + effective_choice = choice + if entry.data.get("smart_denied") and choice in {"session", "always"}: + effective_choice = "once" + entry.result = effective_choice if reason: entry.reason = reason entry.event.set() @@ -836,13 +844,14 @@ def _human_decision(spec: _GateSpec, *, command: str, description: str, allow_permanent = not one_shot def deny(template: str, outcome: str, **fmt) -> dict: - breaker = "" - if "{breaker}" in template: - breaker = _denial_breaker_addendum(session_key) + has_breaker = "{breaker}" in template + breaker = _denial_breaker_addendum(session_key) if has_breaker or smart_denied else "" deny_reason = fmt.pop("deny_reason", None) extra = {"deny_reason": deny_reason} if "reason" in fmt else {} - return _denied(template.format(description=description, breaker=breaker, **fmt), - pattern_key=pattern_key, description=description, + message = template.format(description=description, breaker=breaker, **fmt) + if smart_denied and not has_breaker: + message += breaker + return _denied(message, pattern_key=pattern_key, description=description, outcome=outcome, noun=spec.noun, **extra) def grant(choice: str) -> dict: @@ -850,8 +859,9 @@ def grant(choice: str) -> dict: _persist_choice(session_key, choice, pattern_keys) if spec.user_approved: return _user_approved(session_key, description) + if smart_denied: + _reset_denials(session_key) return _approved() - if spec.transport: attempt = _present_with_selected_transport( command=command, description=description, pattern_key=pattern_key, pattern_keys=pattern_keys, diff --git a/tools/approval_gateway_wait.py b/tools/approval_gateway_wait.py index 1265a445cb69..734c5e44792e 100644 --- a/tools/approval_gateway_wait.py +++ b/tools/approval_gateway_wait.py @@ -172,13 +172,15 @@ def _enqueue() -> None: # The leader lookup and our enqueue are ONE critical section: split, two identical requests both # miss each other, both enqueue and the user gets duplicate prompts. A ``once`` releases the # followers to prompt for themselves; they re-coalesce behind whichever re-prompts first, so the - # user still sees one prompt at a time (the answered leader is already off the queue). + # user still sees one prompt at a time (the answered leader is already off the queue). Reviewer + # DENY requests never share consent: every one must show its own warning to the human. while True: with _approval._lock: leader = None - if not preparing_terminal_approval(): + if not preparing_terminal_approval() and not approval_data.get("smart_denied"): leader = next((e for e in _approval._gateway_queues.get(session_key, []) - if e.data.get("command") == approval_data.get("command") + if not e.data.get("smart_denied") + and e.data.get("command") == approval_data.get("command") and list(e.data.get("pattern_keys") or []) == keys), None) if leader is None: _enqueue() diff --git a/tools/delegate_tool_config.py b/tools/delegate_tool_config.py index 357d1ba43a8f..09a4040049f8 100644 --- a/tools/delegate_tool_config.py +++ b/tools/delegate_tool_config.py @@ -33,12 +33,7 @@ def _cfg() -> dict: # ── Subagent approval callbacks ───────────────────────────────────────────── -# Subagent worker threads don't inherit the CLI's threading.local approval -# callback, so prompt_dangerous_approval() would fall back to input() and -# deadlock the parent's prompt_toolkit TUI. Every worker gets a non-interactive -# callback via ThreadPoolExecutor(initializer=...): deny by default, approve when -# delegation.subagent_auto_approve is true. Both warn for audit. Gateway sessions -# are unaffected (they resolve approvals via tools/approval.py's per-session queue). +# Subagent callbacks are non-interactive: reviewer DENY must block rather than auto-approve. def _subagent_auto_deny(command: str, description: str, **kwargs) -> str: """Auto-deny (safe default): returns 'deny' so the child sees a recoverable refusal.""" logger.warning( @@ -48,7 +43,11 @@ def _subagent_auto_deny(command: str, description: str, **kwargs) -> str: return "deny" def _subagent_auto_approve(command: str, description: str, **kwargs) -> str: - """Auto-approve (opt-in YOLO via delegation.subagent_auto_approve): returns 'once'.""" + """Auto-approve ordinary prompts; reviewer DENY still needs a human answer.""" + if kwargs.get("smart_denied") or "**Reviewer:** flagged as dangerous" in description: + logger.warning("Subagent blocked reviewer-denied command without human approval: %s (%s)", + command, description) + return "deny" logger.warning("Subagent auto-approved dangerous command: %s (%s)", command, description) return "once" From 6d66927efe8472a5c48864d277c37d80bd1aea55 Mon Sep 17 00:00:00 2001 From: mintaka Date: Sun, 8 Nov 2026 20:13:38 -0500 Subject: [PATCH 2/4] fix(approval): bind every approval reply to its own request so a reviewer DENY cannot be released by another (RIG-8544) --- gateway/relay/adapter.py | 6 ++- gateway/session_pending_controls.py | 2 +- gateway/slash_commands.py | 4 +- hermes_cli/cli_modal_mixin.py | 2 +- hermes_cli/cli_tui_mixin.py | 42 ++++++++++--------- hermes_cli/gateway_agent.py | 2 +- plugins/platforms/telegram/adapter.py | 10 +++-- .../fixtures/plugin_origin_approval_peer.py | 13 +++++- tests/gateway/relay/test_relay_interactive.py | 20 ++++++++- tests/gateway/test_approve_deny_commands.py | 16 +++++++ tests/gateway/test_plugin_origin_approval.py | 2 +- .../gateway/test_telegram_approval_buttons.py | 24 ++++++++++- tests/hermes_cli/test_cli_approval_ui.py | 9 ++-- .../test_reviewer_gateway_propagation.py | 19 +++++++++ ...est_approval_gateway_wait_coalesce_race.py | 6 +-- tests/tools/test_delegate_reviewer_signal.py | 6 +++ tests/tools/test_plugin_approval_review.py | 3 +- tests/tools/test_reviewer_fifo_guard.py | 27 ++++++++++++ tools/approval.py | 12 ++++-- tools/approval_gateway_wait.py | 7 +--- tools/delegate_tool_config.py | 6 +-- 21 files changed, 183 insertions(+), 55 deletions(-) create mode 100644 tests/hermes_cli/test_reviewer_gateway_propagation.py create mode 100644 tests/tools/test_delegate_reviewer_signal.py create mode 100644 tests/tools/test_reviewer_fifo_guard.py diff --git a/gateway/relay/adapter.py b/gateway/relay/adapter.py index 0b7711963d5d..7f933aef7e07 100644 --- a/gateway/relay/adapter.py +++ b/gateway/relay/adapter.py @@ -1940,7 +1940,7 @@ async def _send_exec_approval_prompt(self, prompt: ExecApprovalPrompt) -> SendRe metadata = dict(prompt.metadata or {}) metadata.pop("_approval_request_id", None) result = await self._mint_and_send_prompt( - "exec_approval", {"session_key": prompt.session_key}, prompt.chat_id, prompt_kind="approval", + "exec_approval", {"session_key": prompt.session_key, "request_id": prompt.request_id}, prompt.chat_id, prompt_kind="approval", text=prompt.text, options=options, metadata=metadata, ) return result if result is not None else self._PROMPT_UNAVAILABLE @@ -2055,7 +2055,9 @@ async def _resolve_exec_approval(self, state, option_id, chat_id, ack_meta) -> N from tools.approval import resolve_gateway_approval choice = option_id if option_id in _EXEC_APPROVAL_LABELS else "deny" - count = resolve_gateway_approval(str(state.get("session_key") or ""), choice) + request_id = state.get("request_id") + count = resolve_gateway_approval(str(state.get("session_key") or ""), choice, + request_id=request_id) if request_id else 0 label = _EXEC_APPROVAL_LABELS[choice] if count else "⌛ Approval expired — no command was waiting." # In-channel ack preserves the audit trail the native edit gives (the # connector's prompt message can't be edited cross-platform yet). diff --git a/gateway/session_pending_controls.py b/gateway/session_pending_controls.py index 4ff987908058..6dea006149c7 100644 --- a/gateway/session_pending_controls.py +++ b/gateway/session_pending_controls.py @@ -28,7 +28,7 @@ def register(self, session_id, route, generation, data, *, timeout_s=None): 'execution_generation': generation, 'command': _redact_approval_command(data.get('command', '')), 'description': _redact_approval_command(data.get('description', '')), - 'choices': choices} + 'choices': choices, 'smart_denied': bool(data.get('smart_denied'))} if 'edit' in data: prompt['edit'] = deepcopy(data['edit']) prompt['choices'] = ['once', 'deny'] diff --git a/gateway/slash_commands.py b/gateway/slash_commands.py index 6e2a2a620998..892fe0fafa56 100644 --- a/gateway/slash_commands.py +++ b/gateway/slash_commands.py @@ -1194,7 +1194,7 @@ def _blocking_approval_or_stale(self, event: MessageEvent, stale_key: str, none_ async def _handle_approve_command(self, event: MessageEvent) -> Optional[str]: """Handle /approve — unblock waiting agent thread(s). They block inside tools/approval.py; signalling the event resumes them so the command executes inline (same flow as the CLI).""" - from tools.approval import resolve_gateway_approval + from tools.approval import list_gateway_approvals, resolve_gateway_approval session_key, stale = self._blocking_approval_or_stale(event, "gateway.approval_expired", "gateway.approve.no_pending") if stale: @@ -1205,6 +1205,8 @@ async def _handle_approve_command(self, event: MessageEvent) -> Optional[str]: choice = "always" if "always" in choices else "session" if "session" in choices else "once" count = resolve_gateway_approval(session_key, choice, resolve_all="all" in args) if not count: + if any(entry.get("smart_denied") for entry in list_gateway_approvals(session_key)): + return "Reviewer DENY requests must be answered on their own card." return t("gateway.approve.no_pending") confirmation_text = t(f"gateway.approve.{choice}_{'plural' if count > 1 else 'singular'}", count=count) logger.info("User approved %d dangerous command(s) via /approve (%s)", count, choice) diff --git a/hermes_cli/cli_modal_mixin.py b/hermes_cli/cli_modal_mixin.py index 074e82fff64f..9c69ce906cad 100644 --- a/hermes_cli/cli_modal_mixin.py +++ b/hermes_cli/cli_modal_mixin.py @@ -1091,8 +1091,8 @@ def _approval_callback(self, command: str, description: str, from cli import CLI_CONFIG, _DIM, _RST, _cprint with self._approval_lock: - response_queue = queue.Queue() timeout = int(CLI_CONFIG.get("approvals", {}).get("timeout", 300)) + response_queue = queue.Queue() self._approval_state = { "command": command, "description": description, diff --git a/hermes_cli/cli_tui_mixin.py b/hermes_cli/cli_tui_mixin.py index e17c9b95e96d..e90090f3f907 100644 --- a/hermes_cli/cli_tui_mixin.py +++ b/hermes_cli/cli_tui_mixin.py @@ -193,6 +193,7 @@ def _get_approval_display_fragments(self): and is truncated to the remaining row budget, so HSplit never clips approve/deny off-screen. """ from cli import _panel_box_width, _wrap_panel_text_keep_ws + from tools.approval import REVIEWER_DENY_NOTE state = self._approval_state if not state: return [] @@ -204,7 +205,7 @@ def _get_approval_display_fragments(self): description_lines = description.splitlines() warning_index = next(( i for i, line in enumerate(description_lines) - if line.startswith("**Reviewer:** flagged as dangerous") + if line.startswith(REVIEWER_DENY_NOTE) ), None) if warning_index is not None: reviewer_warning = description_lines.pop(warning_index) @@ -223,7 +224,7 @@ def _get_approval_display_fragments(self): box_width = _panel_box_width(title, preview_lines) inner_text_width = max(8, box_width - 2) - # Pre-wrap mandatory content so the reviewer warning, command and choices stay visible. + # Wrap content before allocating the available rows. cmd_wrapped = wrap(command, inner_text_width) if not show_full and "view" in choices and len(cmd_wrapped) > 4: cmd_wrapped = cmd_wrapped[:3] + wrap( @@ -234,21 +235,24 @@ def _get_approval_display_fragments(self): choice_wrapped = _wrap_rows(wrap, choice_labels, inner_text_width, " ") warning_wrapped = wrap(reviewer_warning, inner_text_width) if reviewer_warning else [] - # Inline the title on compact reviewer-DENY panels to reserve a visible warning row. available = max(0, _term_rows() - _PANEL_RESERVED_BELOW) - use_compact_chrome = 5 + len(cmd_wrapped) + len(choice_wrapped) + bool(warning_wrapped) > available - chrome_rows = (2 if warning_wrapped else 3) if use_compact_chrome else (4 if warning_wrapped else 5) - - # Keep the warning first, then truncate the command before dropping its row. - warning_budget = max(1, available - chrome_rows - len(choice_wrapped) - 1) - warning_wrapped = warning_wrapped[:warning_budget] - max_cmd_rows = available - chrome_rows - len(choice_wrapped) - len(warning_wrapped) - if max_cmd_rows < 1: - cmd_wrapped = [] - elif len(cmd_wrapped) > max_cmd_rows: - keep = max(1, max_cmd_rows - 1) if max_cmd_rows > 1 else 1 - cmd_wrapped = cmd_wrapped[:keep] + wrap( - t("cli.tui.approval_command_truncated"), inner_text_width) + # Borders take two rows; full chrome adds a title and two separators. + # Compact warning panels place the title in the border. + use_compact_chrome = 5 + len(cmd_wrapped) + len(choice_wrapped) + len(warning_wrapped) > available + inline_title = use_compact_chrome and bool(warning_wrapped) + chrome_rows = 2 + (0 if inline_title else 1) + (0 if use_compact_chrome else 2) + warning_budget = max(0, available - chrome_rows - len(choice_wrapped) - 1) + if len(warning_wrapped) > warning_budget: + warning_wrapped = warning_wrapped[:warning_budget] + if warning_wrapped: + warning_wrapped[-1] = warning_wrapped[-1][:max(0, inner_text_width - 2)] + " …" + max_cmd_rows = max(1, available - chrome_rows - len(choice_wrapped) - len(warning_wrapped)) + if len(cmd_wrapped) > max_cmd_rows: + if max_cmd_rows == 1: + cmd_wrapped = [cmd_wrapped[0]] + else: + cmd_wrapped = cmd_wrapped[:max_cmd_rows - 1] + [ + t("cli.tui.approval_command_truncated")] # Remaining rows go to the description (minus its blank separator), capped at 10. mandatory_no_desc = chrome_rows + len(warning_wrapped) + len(cmd_wrapped) + len(choice_wrapped) @@ -258,13 +262,13 @@ def _get_approval_display_fragments(self): if available_for_desc < 1 or not desc_wrapped: desc_wrapped = [] elif len(desc_wrapped) > available_for_desc: - desc_wrapped = desc_wrapped[:max(1, available_for_desc - 1)] + [ + desc_wrapped = desc_wrapped[:available_for_desc - 1] + [ t("cli.tui.approval_description_truncated")] # Render the warning before the command; compact chrome integrates the title into its border. panel = _Panel('class:approval-border', box_width, - title if use_compact_chrome and warning_wrapped else "", + title if inline_title else "", 'class:approval-title') - if not (use_compact_chrome and warning_wrapped): + if not inline_title: panel.row('class:approval-title', title) for wrapped in warning_wrapped: panel.row('class:approval-desc', wrapped) diff --git a/hermes_cli/gateway_agent.py b/hermes_cli/gateway_agent.py index 39dc0190e333..251fd2ff3c2a 100644 --- a/hermes_cli/gateway_agent.py +++ b/hermes_cli/gateway_agent.py @@ -996,7 +996,7 @@ def _on_approval(self, payload, stream_callback, starts): try: choice = callback(payload.get("command") or "", payload.get("description") or "", allow_permanent="always" in choices, allow_session="session" in choices, - smart_denied=False) + smart_denied=bool(payload.get("smart_denied"))) except TypeError: choice = callback(payload.get("command") or "", payload.get("description") or "") if choice not in choices: diff --git a/plugins/platforms/telegram/adapter.py b/plugins/platforms/telegram/adapter.py index 7df8153d75c0..c65a37d6a0c4 100644 --- a/plugins/platforms/telegram/adapter.py +++ b/plugins/platforms/telegram/adapter.py @@ -4240,7 +4240,8 @@ def build(): buttons = [InlineKeyboardButton(label, callback_data=f"ea:{choice}:{approval_id}") for label, choice, _ in prompt.actions] return prompt.text, InlineKeyboardMarkup(self._rows_of_two(buttons)), ( - lambda msg: self._approval_state.__setitem__(approval_id, prompt.session_key)) + lambda msg: self._approval_state.__setitem__( + approval_id, (prompt.session_key, prompt.request_id))) return await self._send_prompt( "send_exec_approval", prompt.chat_id, prompt.metadata, build, parse_mode=ParseMode.HTML, thread_id=self._metadata_thread_id(prompt.metadata), reply_to_mode=self._reply_to_mode) @@ -4726,11 +4727,12 @@ async def _handle_exec_approval_callback(self, query, data: str, cb: dict[str, A except (ValueError, IndexError): await query.answer(text=_toast("platform.telegram.approval.toast_invalid_data")) return - session_key = await self._claim_callback_state( + state = await self._claim_callback_state( query, cb, self._approval_state, approval_id, _unauthorized(), _toast("platform.telegram.approval.toast_already_resolved")) - if not session_key: + if not state: return + session_key, request_id = state if isinstance(state, tuple) else (state, None) user_display = getattr(query.from_user, "first_name", None) or t("platform.telegram.user_fallback") # Resolve FIRST (unblocks the agent thread), render after: a tap landing after the wait timed out # (count == 0) must NOT claim "Approved" — the command was already denied. @@ -4739,7 +4741,7 @@ async def _handle_exec_approval_callback(self, query, data: str, cb: dict[str, A # the approval wait timed out (count == 0) must NOT claim "Approved" — the command was already # denied and will not run (#63501 regression follow-up: 60s waits made stale taps common). from tools.approval import resolve_gateway_approval - count = resolve_gateway_approval(session_key, choice) + count = resolve_gateway_approval(session_key, choice, request_id=request_id) if request_id else 0 logger.info( "Telegram button resolved %d approval(s) for session %s (choice=%s, user=%s)", count, session_key, choice, user_display) except Exception as exc: diff --git a/tests/gateway/fixtures/plugin_origin_approval_peer.py b/tests/gateway/fixtures/plugin_origin_approval_peer.py index a00336cdb4b6..42cc062588fe 100644 --- a/tests/gateway/fixtures/plugin_origin_approval_peer.py +++ b/tests/gateway/fixtures/plugin_origin_approval_peer.py @@ -13,6 +13,7 @@ PLUGIN_KEYS = ("allow_gateway_injection", "allow_origin_sessions") CASES = { "review-approve": {"hook": True, "review": True, "answer": "once"}, + "review-mixed-queue": {"hook": True, "review": True, "answer": "once"}, "review-escalate": {"hook": True, "review": True, "verdict": "escalate", "answer": "once"}, "review-deny": {"hook": True, "review": True, "verdict": "deny", "answer": "once"}, "human-deny": {"hook": True, "review": True, "answer": "deny"}, @@ -203,11 +204,19 @@ async def answer_cards(*, answer, expected, before, settled): assert set(prompt.choices) == {"once", "deny"}, prompt.choices assert "wants to run" in prompt.text.lower(), prompt.text assert adapter._approval_mention_content() == "<@12345>" - if case_name == "review-deny": + if case_name == "review-mixed-queue": + from tools.approval_gateway_wait import _ApprovalEntry + denied = _ApprovalEntry({"request_id": "earlier-denial", "smart_denied": True}) + with approval._lock: + approval._gateway_queues[session_key].insert(0, denied) + if case_name == "review-deny" and seen == before + 1: assert "flagged as dangerous" in prompt.text.lower(), prompt.text if answer not in {None, "timeout"}: assert answer in {"once", "deny", "always"} - assert resolve_gateway_approval(session_key, answer) == 1 + assert resolve_gateway_approval( + session_key, answer, request_id=prompt.request_id) == 1 + if case_name == "review-mixed-queue": + assert denied.result is None and not denied.event.is_set() continue try: await asyncio.wait_for(adapter.card_ready.wait(), timeout=0.2) diff --git a/tests/gateway/relay/test_relay_interactive.py b/tests/gateway/relay/test_relay_interactive.py index 9749877558ca..43053ae49a44 100644 --- a/tests/gateway/relay/test_relay_interactive.py +++ b/tests/gateway/relay/test_relay_interactive.py @@ -111,10 +111,28 @@ async def test_exec_approval_renders_full_option_set(): async def test_exec_approval_does_not_send_internal_request_id_in_metadata(): adapter, stub = _adapter() await adapter.send_exec_approval( - "c1", "rm -rf /tmp/x", "sess:1", metadata={"_approval_request_id": "internal"}, + "c1", "rm -rf /tmp/x", "sess:1", metadata={"_approval_request_id": "internal"}, request_id="internal", ) assert "_approval_request_id" not in stub.sent[-1].get("metadata", {}) + state = adapter._pending_prompts[stub.sent[-1]["prompt_id"]] + assert state["request_id"] == "internal" + + +@pytest.mark.asyncio +async def test_relay_card_targets_ordinary_request_ahead_of_denial(monkeypatch): + from tools import approval + from tools.approval_gateway_wait import _ApprovalEntry + + adapter, stub = _adapter() + denied = _ApprovalEntry({"request_id": "deny", "smart_denied": True}) + ordinary = _ApprovalEntry({"request_id": "ordinary"}) + monkeypatch.setitem(approval._gateway_queues, "s", [denied, ordinary]) + await adapter.send_exec_approval("c1", "cmd", "s", request_id="ordinary") + state = adapter._pending_prompts[stub.sent[-1]["prompt_id"]] + await adapter._resolve_exec_approval(state, "once", "c1", {}) + assert ordinary.result == "once" + assert denied.result is None @pytest.mark.asyncio diff --git a/tests/gateway/test_approve_deny_commands.py b/tests/gateway/test_approve_deny_commands.py index 178e66d21050..5328f9a0761e 100644 --- a/tests/gateway/test_approve_deny_commands.py +++ b/tests/gateway/test_approve_deny_commands.py @@ -162,6 +162,22 @@ class TestApproveCommand: def setup_method(self): _clear_approval_state() + @pytest.mark.asyncio + async def test_approve_skips_denial_and_reports_card_when_only_denial_remains(self): + from tools.approval import _gateway_queues + from tools.approval_gateway_wait import _ApprovalEntry + + runner = _make_runner() + session_key = runner._session_key_for_source(_make_source()) + denied = _ApprovalEntry({"request_id": "denied", "smart_denied": True}) + ordinary = _ApprovalEntry({"request_id": "ordinary"}) + _gateway_queues[session_key] = [denied, ordinary] + await runner._handle_approve_command(_make_event("/approve")) + assert ordinary.result == "once" and denied.result is None + response = await runner._handle_approve_command(_make_event("/approve")) + assert "own card" in response + assert denied.result is None + @pytest.mark.asyncio async def test_approve_all_resolves_multiple(self): diff --git a/tests/gateway/test_plugin_origin_approval.py b/tests/gateway/test_plugin_origin_approval.py index 4a3570cefc6d..0fe0d9bd6d2b 100644 --- a/tests/gateway/test_plugin_origin_approval.py +++ b/tests/gateway/test_plugin_origin_approval.py @@ -11,7 +11,7 @@ @pytest.mark.platforms("linux") @pytest.mark.parametrize("case", [ - "review-approve", "review-escalate", "review-deny", "human-deny", "timeout", "stale-always", + "review-approve", "review-escalate", "review-deny", "review-mixed-queue", "human-deny", "timeout", "stale-always", "terminal", "execute-code", "yolo", "session-yolo", "mode-off", "grants", "allowlist", "smart-approve", "smart-deny", "cotal-hook-deny", "cotal-command-deny", "cotal-code-deny", "operator-bypasses", diff --git a/tests/gateway/test_telegram_approval_buttons.py b/tests/gateway/test_telegram_approval_buttons.py index 197d71c7f11b..0072e8044268 100644 --- a/tests/gateway/test_telegram_approval_buttons.py +++ b/tests/gateway/test_telegram_approval_buttons.py @@ -202,6 +202,26 @@ async def mock_send_message(**kwargs): class TestTelegramApprovalCallback: """Test the approval callback handling in _handle_callback_query.""" + @pytest.mark.asyncio + async def test_tap_targets_card_and_stale_tap_cannot_release_denial(self, monkeypatch): + from tools import approval + from tools.approval_gateway_wait import _ApprovalEntry + + adapter = _make_adapter() + denied = _ApprovalEntry({"request_id": "a", "smart_denied": True}) + ordinary = _ApprovalEntry({"request_id": "b"}) + monkeypatch.setitem(approval._gateway_queues, "session", [denied, ordinary]) + adapter._approval_state[5] = ("session", "b") + adapter._approval_state[6] = ("session", "old") + query = AsyncMock() + query.from_user.first_name = "owner" + adapter._claim_callback_state = AsyncMock(side_effect=[("session", "b"), ("session", "old")]) + adapter._edit_md_quiet = AsyncMock() + await adapter._handle_exec_approval_callback(query, "ea:once:5", {"chat_id": "1"}) + await adapter._handle_exec_approval_callback(query, "ea:once:6", {"chat_id": "1"}) + assert ordinary.result == "once" + assert denied.result is None + @pytest.mark.asyncio async def test_resume_typing_after_inline_approval(self): @@ -212,7 +232,7 @@ async def test_resume_typing_after_inline_approval(self): rest of a long-running turn after a button click. """ adapter = _make_adapter() - adapter._approval_state[5] = "agent:main:telegram:group:12345:99" + adapter._approval_state[5] = ("agent:main:telegram:group:12345:99", "request-5") adapter.pause_typing_for_chat("12345") assert "12345" in adapter._typing_paused @@ -240,7 +260,7 @@ async def test_resume_typing_after_inline_approval(self): @pytest.mark.asyncio async def test_approval_callback_escapes_dynamic_user_name(self): adapter = _make_adapter() - adapter._approval_state[3] = "agent:main:telegram:group:12345:99" + adapter._approval_state[3] = ("agent:main:telegram:group:12345:99", "request-3") query = AsyncMock() query.data = "ea:once:3" diff --git a/tests/hermes_cli/test_cli_approval_ui.py b/tests/hermes_cli/test_cli_approval_ui.py index 46594988dbe2..2b08e7f06063 100644 --- a/tests/hermes_cli/test_cli_approval_ui.py +++ b/tests/hermes_cli/test_cli_approval_ui.py @@ -176,10 +176,6 @@ def test_handle_approval_selection_view_expands_in_place(self): assert cli._approval_state["response_queue"].empty() - - - - def test_approval_display_truncates_giant_command_in_view_mode(self): """If the user hits /view on a massive command, choices still render. @@ -235,6 +231,11 @@ def test_reviewer_deny_warning_is_visible_with_long_reason_and_short_terminal(se rendered = "".join(text for _style, text in fragments) assert "flagged as dangerous" in rendered + assert "guarded action" in rendered + assert rendered.index("flagged as dangerous") < rendered.index("guarded action") + assert "…" in rendered + from hermes_cli.cli_tui_mixin import _PANEL_RESERVED_BELOW + assert rendered.count("\n") <= 12 - _PANEL_RESERVED_BELOW def test_background_task_registers_thread_local_approval_callbacks(self): """Background /btw tasks must use the prompt_toolkit approval UI. diff --git a/tests/hermes_cli/test_reviewer_gateway_propagation.py b/tests/hermes_cli/test_reviewer_gateway_propagation.py new file mode 100644 index 000000000000..83098fbfee2d --- /dev/null +++ b/tests/hermes_cli/test_reviewer_gateway_propagation.py @@ -0,0 +1,19 @@ +from types import SimpleNamespace +from unittest.mock import Mock +import threading + +from gateway.session_pending_controls import PendingControls +from hermes_cli.gateway_agent import GatewayAgent +from tools import terminal_tool + + +def test_reviewer_denial_reaches_cli_callback(monkeypatch): + events = SimpleNamespace(lock=threading.RLock(), publish=Mock()) + controls = PendingControls(events) + controls.register("session", "route", 1, {"request_id": "r", "smart_denied": True}) + payload = events.publish.call_args.args[1] + callback = Mock(return_value="deny") + monkeypatch.setattr(terminal_tool, "_get_approval_callback", lambda: callback) + agent = SimpleNamespace(connection=SimpleNamespace(call=Mock()), session_id="session", _generation=1) + GatewayAgent._on_approval(agent, payload, None, None) + assert callback.call_args.kwargs["smart_denied"] is True diff --git a/tests/tools/test_approval_gateway_wait_coalesce_race.py b/tests/tools/test_approval_gateway_wait_coalesce_race.py index 7dca611edae5..ab8091da8e55 100644 --- a/tests/tools/test_approval_gateway_wait_coalesce_race.py +++ b/tests/tools/test_approval_gateway_wait_coalesce_race.py @@ -263,7 +263,7 @@ def test_smart_denied_broad_gateway_reply_is_normalized_to_once(monkeypatch): def notify(data): assert data["smart_denied"] is True - assert mod.resolve_gateway_approval(SESSION_KEY, "always") == 1 + assert mod.resolve_gateway_approval(SESSION_KEY, "always", request_id=data["request_id"]) == 1 thread = threading.Thread( target=lambda: result.update(wait_mod._await_gateway_decision( @@ -294,12 +294,12 @@ def test_smart_denied_bulk_approval_waits_for_bulk_deny(): def test_bulk_ordinary_approval_does_not_resolve_smart_denied(): _clear() ordinary = wait_mod._ApprovalEntry(dict(APPROVAL)) - denied = wait_mod._ApprovalEntry({**APPROVAL, "smart_denied": True}) + denied = wait_mod._ApprovalEntry({**APPROVAL, "smart_denied": True, "request_id": "deny"}) with mod._lock: mod._gateway_queues[SESSION_KEY] = [ordinary, denied] assert mod.resolve_gateway_approval(SESSION_KEY, "session", resolve_all=True) == 1 assert ordinary.result == "session" assert not denied.event.is_set() - assert mod.resolve_gateway_approval(SESSION_KEY, "once") == 1 + assert mod.resolve_gateway_approval(SESSION_KEY, "once", request_id="deny") == 1 assert denied.result == "once" diff --git a/tests/tools/test_delegate_reviewer_signal.py b/tests/tools/test_delegate_reviewer_signal.py new file mode 100644 index 000000000000..5134df35ed23 --- /dev/null +++ b/tests/tools/test_delegate_reviewer_signal.py @@ -0,0 +1,6 @@ +from tools.delegate_tool_config import _subagent_auto_approve + + +def test_reviewer_text_alone_does_not_override_ordinary_prompt(): + assert _subagent_auto_approve("command", "**Reviewer:** flagged as dangerous") == "once" + assert _subagent_auto_approve("command", "other wording", smart_denied=True) == "deny" diff --git a/tests/tools/test_plugin_approval_review.py b/tests/tools/test_plugin_approval_review.py index 95439f2a5a67..299449cb93bb 100644 --- a/tests/tools/test_plugin_approval_review.py +++ b/tests/tools/test_plugin_approval_review.py @@ -215,7 +215,6 @@ def _approve(command, description, **kwargs): assert "**Reviewer:** flagged as dangerous: Pipes the key to a remote host." in result["message"] - def test_reviewer_deny_with_auto_approve_callback_is_blocked(monkeypatch): from tools.delegate_tool import _subagent_auto_approve @@ -285,7 +284,7 @@ def reply(data): assert data["smart_denied"] is True assert data["allow_session"] is False assert data["allow_permanent"] is False - assert approval.resolve_gateway_approval("review-session", "session") == 1 + assert approval.resolve_gateway_approval("review-session", "session", request_id=data["request_id"]) == 1 approval.register_gateway_notify("review-session", reply) try: diff --git a/tests/tools/test_reviewer_fifo_guard.py b/tests/tools/test_reviewer_fifo_guard.py new file mode 100644 index 000000000000..cf6ba6d0162e --- /dev/null +++ b/tests/tools/test_reviewer_fifo_guard.py @@ -0,0 +1,27 @@ +from tools import approval +from tools.approval_gateway_wait import _ApprovalEntry + + +def test_unnamed_approval_skips_reviewer_denial(monkeypatch): + denied = _ApprovalEntry({"request_id": "a", "smart_denied": True}) + ordinary = _ApprovalEntry({"request_id": "b"}) + monkeypatch.setitem(approval._gateway_queues, "fifo-test", [denied, ordinary]) + assert approval.resolve_gateway_approval("fifo-test", "once") == 1 + assert ordinary.result == "once" + assert denied.result is None + assert approval.resolve_gateway_approval("fifo-test", "once") == 0 + + +def test_review_first_ignores_cached_approval(monkeypatch): + monkeypatch.setattr(approval.approval_context, "is_plugin_origin_turn", lambda: False) + monkeypatch.setattr(approval, "_yolo_active", lambda: False) + monkeypatch.setattr(approval.approval_context, "_get_approval_mode", lambda: "ask") + monkeypatch.setattr(approval, "is_approved", lambda *args: True) + monkeypatch.setattr(approval, "_presence", lambda cb: (cb, True, False, False)) + monkeypatch.setattr(approval, "_smart_verdict", lambda *args: "deny") + monkeypatch.setattr(approval, "_record_denial", lambda *args: None) + seen = {} + monkeypatch.setattr(approval, "_human_decision", lambda *args, **kwargs: seen.update(kwargs) or {}) + approval._run_approval_gate(pattern_key="key", description="reason", display_target="action", + autoapprove_log_prefix="test", review_first=True) + assert seen["smart_denied"] is True diff --git a/tools/approval.py b/tools/approval.py index 4a1d01e1b388..224ba07124c8 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -169,7 +169,11 @@ def resolve_gateway_approval(session_key: str, choice: str, targets = [entry for entry in queue if not entry.data.get("smart_denied")] queue[:] = [entry for entry in queue if entry.data.get("smart_denied")] else: - targets = [queue.pop(0)] + target = next((entry for entry in queue + if choice == "deny" or not entry.data.get("smart_denied")), None) + targets = [target] if target else [] + if target: + queue.remove(target) if not queue: _gateway_queues.pop(session_key, None) # Popping the entry and committing its outcome are ONE critical section: the waiter's @@ -862,6 +866,7 @@ def grant(choice: str) -> dict: if smart_denied: _reset_denials(session_key) return _approved() + if spec.transport: attempt = _present_with_selected_transport( command=command, description=description, pattern_key=pattern_key, pattern_keys=pattern_keys, @@ -994,10 +999,11 @@ def _presence(approval_callback=None) -> tuple: # Reviewer verdict appended to a plugin approval's reason when the plugin asked for review first. +REVIEWER_DENY_NOTE = "**Reviewer:** flagged as dangerous" _REVIEW_NOTES = { "approve": "**Reviewer:** passed", "escalate": "**Reviewer:** unsure, check it carefully", - "deny": "**Reviewer:** flagged as dangerous", + "deny": REVIEWER_DENY_NOTE, } @@ -1036,7 +1042,7 @@ def _run_approval_gate( session_key = get_current_session_key() if not plugin_origin and (_yolo_active() or approval_context._get_approval_mode() == "off"): return _approved() - if not plugin_origin and is_approved(session_key, pattern_key): + if not plugin_origin and not review_first and is_approved(session_key, pattern_key): return _approved() approval_callback, is_cli, is_gateway, is_ask = _presence(approval_callback) if not is_cli and not is_gateway and not is_ask: diff --git a/tools/approval_gateway_wait.py b/tools/approval_gateway_wait.py index 734c5e44792e..11a11cbf157b 100644 --- a/tools/approval_gateway_wait.py +++ b/tools/approval_gateway_wait.py @@ -169,11 +169,8 @@ def _enqueue() -> None: register_prepared_approval(session_key, entry) _approval._gateway_queues.setdefault(session_key, []).append(entry) - # The leader lookup and our enqueue are ONE critical section: split, two identical requests both - # miss each other, both enqueue and the user gets duplicate prompts. A ``once`` releases the - # followers to prompt for themselves; they re-coalesce behind whichever re-prompts first, so the - # user still sees one prompt at a time (the answered leader is already off the queue). Reviewer - # DENY requests never share consent: every one must show its own warning to the human. + # Lookup and enqueue must be atomic to coalesce concurrent identical requests. + # Once releases followers to reprompt; reviewer DENY never shares consent. while True: with _approval._lock: leader = None diff --git a/tools/delegate_tool_config.py b/tools/delegate_tool_config.py index 09a4040049f8..29b3bb377bc9 100644 --- a/tools/delegate_tool_config.py +++ b/tools/delegate_tool_config.py @@ -33,7 +33,7 @@ def _cfg() -> dict: # ── Subagent approval callbacks ───────────────────────────────────────────── -# Subagent callbacks are non-interactive: reviewer DENY must block rather than auto-approve. +# Callbacks are non-interactive because input() would deadlock the parent TUI. def _subagent_auto_deny(command: str, description: str, **kwargs) -> str: """Auto-deny (safe default): returns 'deny' so the child sees a recoverable refusal.""" logger.warning( @@ -43,8 +43,8 @@ def _subagent_auto_deny(command: str, description: str, **kwargs) -> str: return "deny" def _subagent_auto_approve(command: str, description: str, **kwargs) -> str: - """Auto-approve ordinary prompts; reviewer DENY still needs a human answer.""" - if kwargs.get("smart_denied") or "**Reviewer:** flagged as dangerous" in description: + """Auto-approve ordinary prompts; reviewer DENY is refused, never auto-approved.""" + if kwargs.get("smart_denied"): logger.warning("Subagent blocked reviewer-denied command without human approval: %s (%s)", command, description) return "deny" From 99e9aa7d93d6b4436a70d189d2ab8db297371e29 Mon Sep 17 00:00:00 2001 From: mintaka Date: Sun, 8 Nov 2026 20:48:04 -0500 Subject: [PATCH 3/4] fix(approval): fail closed on unbound replies to a DENY (RIG-8544) A reply with no request ID resolves nothing while a DENY heads the queue. review_first honours cached grants for non-DENY verdicts. The CLI panel keeps the warning and a command row at 11 rows. Refs RIG-8544 Co-authored-by: Matt Wilkinson --- gateway/slash_commands.py | 4 +++- hermes_cli/cli_tui_mixin.py | 7 ++++--- hermes_cli/gateway_agent.py | 5 +++-- locales/af.yaml | 1 + locales/ar.yaml | 1 + locales/de.yaml | 1 + locales/en.yaml | 1 + locales/es.yaml | 1 + locales/fr.yaml | 1 + locales/ga.yaml | 1 + locales/hu.yaml | 1 + locales/it.yaml | 1 + locales/ja.yaml | 1 + locales/ko.yaml | 1 + locales/pt.yaml | 1 + locales/ru.yaml | 1 + locales/tr.yaml | 1 + locales/uk.yaml | 1 + locales/zh-hant.yaml | 1 + locales/zh.yaml | 1 + plugins/platforms/telegram/adapter.py | 2 +- tests/gateway/test_approve_deny_commands.py | 20 +++++++++++++++---- tests/hermes_cli/test_cli_approval_ui.py | 9 ++++++--- .../test_reviewer_gateway_propagation.py | 16 +++++++++++++++ ...est_approval_gateway_wait_coalesce_race.py | 13 ++++++++++++ tests/tools/test_plugin_approval_review.py | 12 +++++++++++ tests/tools/test_reviewer_fifo_guard.py | 11 ---------- tools/approval.py | 16 ++++++++------- tools/approval_gateway_wait.py | 2 +- 29 files changed, 101 insertions(+), 33 deletions(-) diff --git a/gateway/slash_commands.py b/gateway/slash_commands.py index 892fe0fafa56..4e3e3d7d6a19 100644 --- a/gateway/slash_commands.py +++ b/gateway/slash_commands.py @@ -1206,9 +1206,11 @@ async def _handle_approve_command(self, event: MessageEvent) -> Optional[str]: count = resolve_gateway_approval(session_key, choice, resolve_all="all" in args) if not count: if any(entry.get("smart_denied") for entry in list_gateway_approvals(session_key)): - return "Reviewer DENY requests must be answered on their own card." + return t("gateway.approve.own_card") return t("gateway.approve.no_pending") confirmation_text = t(f"gateway.approve.{choice}_{'plural' if count > 1 else 'singular'}", count=count) + if "all" in args and any(entry.get("smart_denied") for entry in list_gateway_approvals(session_key)): + confirmation_text += "\n" + t("gateway.approve.own_card") logger.info("User approved %d dangerous command(s) via /approve (%s)", count, choice) return await self._deliver_approval_confirmation(event, confirmation_text, "approve") diff --git a/hermes_cli/cli_tui_mixin.py b/hermes_cli/cli_tui_mixin.py index e90090f3f907..0d5896a0f7a9 100644 --- a/hermes_cli/cli_tui_mixin.py +++ b/hermes_cli/cli_tui_mixin.py @@ -240,8 +240,9 @@ def _get_approval_display_fragments(self): # Compact warning panels place the title in the border. use_compact_chrome = 5 + len(cmd_wrapped) + len(choice_wrapped) + len(warning_wrapped) > available inline_title = use_compact_chrome and bool(warning_wrapped) - chrome_rows = 2 + (0 if inline_title else 1) + (0 if use_compact_chrome else 2) - warning_budget = max(0, available - chrome_rows - len(choice_wrapped) - 1) + omit_bottom_border = bool(warning_wrapped) and 2 + len(choice_wrapped) + 2 > available + chrome_rows = 2 - int(omit_bottom_border) + (0 if inline_title else 1) + (0 if use_compact_chrome else 2) + warning_budget = max(1, available - chrome_rows - len(choice_wrapped) - 1) if len(warning_wrapped) > warning_budget: warning_wrapped = warning_wrapped[:warning_budget] if warning_wrapped: @@ -286,7 +287,7 @@ def _get_approval_display_fragments(self): panel.blank() for wrapped in desc_wrapped: panel.row('class:approval-desc', wrapped) - return panel.close() + return panel.lines if omit_bottom_border else panel.close() def _get_tui_prompt_symbols(self) -> tuple[str, str]: """Return ``(normal_prompt, state_suffix)`` for the active skin. diff --git a/hermes_cli/gateway_agent.py b/hermes_cli/gateway_agent.py index 251fd2ff3c2a..55c3168e8825 100644 --- a/hermes_cli/gateway_agent.py +++ b/hermes_cli/gateway_agent.py @@ -988,16 +988,17 @@ def _emit_print(self, text): (self._print_fn or print)(text) def _on_approval(self, payload, stream_callback, starts): + from tools.approval_prompt import callback_accepts from tools.terminal_tool import _get_approval_callback callback = _get_approval_callback() choices = list(payload.get("choices") or []) choice = "deny" if callback is not None: - try: + if callback_accepts(callback, "smart_denied"): choice = callback(payload.get("command") or "", payload.get("description") or "", allow_permanent="always" in choices, allow_session="session" in choices, smart_denied=bool(payload.get("smart_denied"))) - except TypeError: + else: choice = callback(payload.get("command") or "", payload.get("description") or "") if choice not in choices: choice = "deny" diff --git a/locales/af.yaml b/locales/af.yaml index c83eee703f8c..29b8dcdd08d8 100644 --- a/locales/af.yaml +++ b/locales/af.yaml @@ -92,6 +92,7 @@ gateway: child_row: " - kind {index}: {calls} API-oproepe · {doing}" active_ago: " · {seconds}s gelede aktief" approve: + own_card: "Versoeke wat die beoordelaar afgekeur het, moet op hul eie kaart beantwoord word." no_pending: "Geen hangende opdrag om goed te keur nie." once_singular: "✅ Opdrag goedgekeur. Die agent gaan voort..." once_plural: "✅ Opdragte goedgekeur ({count} opdragte). Die agent gaan voort..." diff --git a/locales/ar.yaml b/locales/ar.yaml index 0ba1fc9f1985..05dbd5300cec 100644 --- a/locales/ar.yaml +++ b/locales/ar.yaml @@ -92,6 +92,7 @@ gateway: child_row: " - الوكيل الفرعي {index}: {calls} استدعاء API · {doing}" active_ago: " · كان نشطًا قبل {seconds} ث" approve: + own_card: "يجب الرد على الطلبات التي رفضها المراجع في بطاقتها الخاصة." no_pending: "لا يوجد أمر معلّق للموافقة عليه." once_singular: "✅ تمت الموافقة على الأمر. الوكيل يستأنف..." once_plural: "✅ تمت الموافقة على الأوامر ({count} أوامر). الوكيل يستأنف..." diff --git a/locales/de.yaml b/locales/de.yaml index 96a2f9bf1057..a2b1cf5ccc69 100644 --- a/locales/de.yaml +++ b/locales/de.yaml @@ -92,6 +92,7 @@ gateway: child_row: " - Kind {index}: {calls} API-Aufrufe · {doing}" active_ago: " · vor {seconds}s aktiv" approve: + own_card: "Vom Prüfer abgelehnte Anfragen müssen auf ihrer eigenen Karte beantwortet werden." no_pending: "Kein ausstehender Befehl zum Genehmigen." once_singular: "✅ Befehl genehmigt. Der Agent wird fortgesetzt..." once_plural: "✅ Befehle genehmigt ({count} Befehle). Der Agent wird fortgesetzt..." diff --git a/locales/en.yaml b/locales/en.yaml index 1a773037e498..da49806b9c47 100644 --- a/locales/en.yaml +++ b/locales/en.yaml @@ -95,6 +95,7 @@ gateway: child_row: " - child {index}: {calls} api calls · {doing}" active_ago: " · active {seconds}s ago" approve: + own_card: "Reviewer DENY requests must be answered on their own card." no_pending: "No pending command to approve." once_singular: "✅ Command approved. The agent is resuming..." once_plural: "✅ Commands approved ({count} commands). The agent is resuming..." diff --git a/locales/es.yaml b/locales/es.yaml index 25835352ec18..dd7f1d794ba9 100644 --- a/locales/es.yaml +++ b/locales/es.yaml @@ -92,6 +92,7 @@ gateway: child_row: " - hijo {index}: {calls} llamadas a la API · {doing}" active_ago: " · activo hace {seconds}s" approve: + own_card: "Las solicitudes rechazadas por el revisor deben responderse en su propia tarjeta." no_pending: "No hay ningún comando pendiente que aprobar." once_singular: "✅ Comando aprobado. El agente se está reanudando..." once_plural: "✅ Comandos aprobados ({count} comandos). El agente se está reanudando..." diff --git a/locales/fr.yaml b/locales/fr.yaml index 13b40dd94556..336224cbe586 100644 --- a/locales/fr.yaml +++ b/locales/fr.yaml @@ -97,6 +97,7 @@ gateway: child_row: " - enfant {index} : {calls} appels API · {doing}" active_ago: " · actif il y a {seconds}s" approve: + own_card: "Les demandes refusées par le réviseur doivent être traitées sur leur propre carte." no_pending: "Aucune commande en attente d'approbation." once_singular: "✅ Commande approuvée. L'agent reprend..." once_plural: "✅ Commandes approuvées ({count} commandes). L'agent reprend..." diff --git a/locales/ga.yaml b/locales/ga.yaml index 85f5164f5c2f..eedbaa5c3d0e 100644 --- a/locales/ga.yaml +++ b/locales/ga.yaml @@ -92,6 +92,7 @@ gateway: child_row: " - fo-ghníomhaire {index}: {calls} glao API · {doing}" active_ago: " · gníomhach {seconds}s ó shin" approve: + own_card: "Ní mór iarratais a dhiúltaigh an t-athbhreithneoir a fhreagairt ar a gcárta féin." no_pending: "Níl aon ordú ag fanacht le ceadú." once_singular: "✅ Ordú ceadaithe. Tá an gníomhaire ag atosú..." once_plural: "✅ Orduithe ceadaithe ({count} ordú). Tá an gníomhaire ag atosú..." diff --git a/locales/hu.yaml b/locales/hu.yaml index d73ed002d515..35a707b88363 100644 --- a/locales/hu.yaml +++ b/locales/hu.yaml @@ -92,6 +92,7 @@ gateway: child_row: " - gyerek {index}: {calls} API-hívás · {doing}" active_ago: " · {seconds} mp-e volt aktív" approve: + own_card: "Az ellenőrző által elutasított kéréseket a saját kártyájukon kell megválaszolni." no_pending: "Nincs jóváhagyásra váró parancs." once_singular: "✅ Parancs jóváhagyva. Az ügynök folytatja..." once_plural: "✅ Parancsok jóváhagyva ({count} parancs). Az ügynök folytatja..." diff --git a/locales/it.yaml b/locales/it.yaml index 23ed9d57f78d..c2e2568306e8 100644 --- a/locales/it.yaml +++ b/locales/it.yaml @@ -92,6 +92,7 @@ gateway: child_row: " - figlio {index}: {calls} chiamate API · {doing}" active_ago: " · attivo {seconds}s fa" approve: + own_card: "Le richieste rifiutate dal revisore devono essere gestite sulla propria scheda." no_pending: "Nessun comando in attesa di approvazione." once_singular: "✅ Comando approvato. L'agente sta riprendendo..." once_plural: "✅ Comandi approvati ({count} comandi). L'agente sta riprendendo..." diff --git a/locales/ja.yaml b/locales/ja.yaml index 992246f9d74f..78105e53d666 100644 --- a/locales/ja.yaml +++ b/locales/ja.yaml @@ -92,6 +92,7 @@ gateway: child_row: " - 子 {index}: API 呼び出し {calls} 回 · {doing}" active_ago: " · {seconds} 秒前に活動" approve: + own_card: "レビュー担当が拒否したリクエストには、それぞれのカードで回答してください。" no_pending: "承認待ちのコマンドはありません。" once_singular: "✅ コマンドを承認しました。エージェントを再開しています..." once_plural: "✅ コマンドを承認しました ({count} 件)。エージェントを再開しています..." diff --git a/locales/ko.yaml b/locales/ko.yaml index a76da7f4ff28..ea27c36b6388 100644 --- a/locales/ko.yaml +++ b/locales/ko.yaml @@ -92,6 +92,7 @@ gateway: child_row: " - 자식 {index}: API 호출 {calls}회 · {doing}" active_ago: " · {seconds}초 전 활동" approve: + own_card: "검토자가 거부한 요청은 해당 카드에서 응답해야 합니다." no_pending: "승인 대기 중인 명령이 없습니다." once_singular: "✅ 명령이 승인되었습니다. 에이전트가 재개됩니다..." once_plural: "✅ 명령이 승인되었습니다 ({count}개). 에이전트가 재개됩니다..." diff --git a/locales/pt.yaml b/locales/pt.yaml index 0656f7b0b418..e68b311b4c1e 100644 --- a/locales/pt.yaml +++ b/locales/pt.yaml @@ -92,6 +92,7 @@ gateway: child_row: " - filho {index}: {calls} chamadas de API · {doing}" active_ago: " · ativo há {seconds}s" approve: + own_card: "Os pedidos rejeitados pelo revisor devem ser respondidos no respetivo cartão." no_pending: "Não há nenhum comando pendente para aprovar." once_singular: "✅ Comando aprovado. O agente está a retomar..." once_plural: "✅ Comandos aprovados ({count} comandos). O agente está a retomar..." diff --git a/locales/ru.yaml b/locales/ru.yaml index 8002a2140f5a..3b0dc67b11fa 100644 --- a/locales/ru.yaml +++ b/locales/ru.yaml @@ -92,6 +92,7 @@ gateway: child_row: " - потомок {index}: {calls} вызовов API · {doing}" active_ago: " · активен {seconds}с назад" approve: + own_card: "На запросы, отклонённые проверяющим, нужно отвечать в их собственной карточке." no_pending: "Нет команды, ожидающей одобрения." once_singular: "✅ Команда одобрена. Агент возобновляет работу..." once_plural: "✅ Команды одобрены ({count} команд). Агент возобновляет работу..." diff --git a/locales/tr.yaml b/locales/tr.yaml index 6a6473c74298..54826ade6eea 100644 --- a/locales/tr.yaml +++ b/locales/tr.yaml @@ -92,6 +92,7 @@ gateway: child_row: " - alt ajan {index}: {calls} API çağrısı · {doing}" active_ago: " · {seconds} sn önce aktifti" approve: + own_card: "İnceleyicinin reddettiği istekler kendi kartlarında yanıtlanmalıdır." no_pending: "Onaylanacak bekleyen komut yok." once_singular: "✅ Komut onaylandı. Ajan devam ediyor..." once_plural: "✅ Komutlar onaylandı ({count} komut). Ajan devam ediyor..." diff --git a/locales/uk.yaml b/locales/uk.yaml index da3207f340b0..34839f782b7e 100644 --- a/locales/uk.yaml +++ b/locales/uk.yaml @@ -92,6 +92,7 @@ gateway: child_row: " - дочірній {index}: {calls} api-викликів · {doing}" active_ago: " · активний {seconds}с тому" approve: + own_card: "На запити, відхилені перевіряльником, потрібно відповідати на їхній власній картці." no_pending: "Немає команди на схвалення." once_singular: "✅ Команду схвалено. Агент відновлює роботу…" once_plural: "✅ Команди схвалено ({count} команд). Агент відновлює роботу…" diff --git a/locales/zh-hant.yaml b/locales/zh-hant.yaml index d24097161399..66ba8cc42db9 100644 --- a/locales/zh-hant.yaml +++ b/locales/zh-hant.yaml @@ -92,6 +92,7 @@ gateway: child_row: " - 子代理 {index}：{calls} 次 API 呼叫 · {doing}" active_ago: " · {seconds} 秒前活動" approve: + own_card: "審查者拒絕的請求必須在各自的卡片上回覆。" no_pending: "沒有待批准的指令。" once_singular: "✅ 指令已批准。代理正在恢復…" once_plural: "✅ 指令已批准（{count} 條指令）。代理正在恢復…" diff --git a/locales/zh.yaml b/locales/zh.yaml index ecb2348e2cfc..02101549d190 100644 --- a/locales/zh.yaml +++ b/locales/zh.yaml @@ -92,6 +92,7 @@ gateway: child_row: " - 子代理 {index}：{calls} 次 API 调用 · {doing}" active_ago: " · {seconds} 秒前活跃" approve: + own_card: "审核者拒绝的请求必须在各自的卡片上回复。" no_pending: "没有待批准的命令。" once_singular: "✅ 命令已批准。代理正在恢复…" once_plural: "✅ 命令已批准（{count} 条命令）。代理正在恢复…" diff --git a/plugins/platforms/telegram/adapter.py b/plugins/platforms/telegram/adapter.py index c65a37d6a0c4..097c578bfe09 100644 --- a/plugins/platforms/telegram/adapter.py +++ b/plugins/platforms/telegram/adapter.py @@ -701,7 +701,7 @@ def __init__(self, config: PlatformConfig): self._max_doc_bytes: int = 2 * 1024 * 1024 * 1024 if extra.get("base_url") else 20 * 1024 * 1024 self._model_picker_state: dict[str, dict] = {} # per-chat interactive picker state self._choice_picker_state: dict[str, dict] = {} - self._approval_state: dict[int, str] = {} # message_id → session_key + self._approval_state: dict[int, tuple[str, str]] = {} # message_id → (session_key, request_id) self._slash_confirm_state: dict[str, str] = {} # confirm_id → session_key self._clarify_state: dict[str, str] = {} # clarify_id → session_key # "important" (default): only final responses, approvals and slash confirmations notify; diff --git a/tests/gateway/test_approve_deny_commands.py b/tests/gateway/test_approve_deny_commands.py index 5328f9a0761e..69354c1493f4 100644 --- a/tests/gateway/test_approve_deny_commands.py +++ b/tests/gateway/test_approve_deny_commands.py @@ -163,7 +163,7 @@ def setup_method(self): _clear_approval_state() @pytest.mark.asyncio - async def test_approve_skips_denial_and_reports_card_when_only_denial_remains(self): + async def test_approve_stops_at_denial_and_reports_card(self): from tools.approval import _gateway_queues from tools.approval_gateway_wait import _ApprovalEntry @@ -172,11 +172,23 @@ async def test_approve_skips_denial_and_reports_card_when_only_denial_remains(se denied = _ApprovalEntry({"request_id": "denied", "smart_denied": True}) ordinary = _ApprovalEntry({"request_id": "ordinary"}) _gateway_queues[session_key] = [denied, ordinary] - await runner._handle_approve_command(_make_event("/approve")) - assert ordinary.result == "once" and denied.result is None response = await runner._handle_approve_command(_make_event("/approve")) assert "own card" in response - assert denied.result is None + assert ordinary.result is None and denied.result is None + + @pytest.mark.asyncio + async def test_approve_all_reports_remaining_denial(self): + from tools.approval import _gateway_queues + from tools.approval_gateway_wait import _ApprovalEntry + + runner = _make_runner() + session_key = runner._session_key_for_source(_make_source()) + denied = _ApprovalEntry({"request_id": "denied", "smart_denied": True}) + ordinary = _ApprovalEntry({"request_id": "ordinary"}) + _gateway_queues[session_key] = [denied, ordinary] + response = await runner._handle_approve_command(_make_event("/approve all")) + assert ordinary.result == "once" and denied.result is None + assert "own card" in response @pytest.mark.asyncio diff --git a/tests/hermes_cli/test_cli_approval_ui.py b/tests/hermes_cli/test_cli_approval_ui.py index 2b08e7f06063..cc70156b60bd 100644 --- a/tests/hermes_cli/test_cli_approval_ui.py +++ b/tests/hermes_cli/test_cli_approval_ui.py @@ -4,6 +4,8 @@ from types import SimpleNamespace from unittest.mock import MagicMock, patch +import pytest + import cli as cli_module from agent.i18n import t from cli import HermesCLI @@ -211,7 +213,8 @@ def test_approval_display_truncates_giant_command_in_view_mode(self): assert t("cli.tui.approval_command_truncated") in rendered - def test_reviewer_deny_warning_is_visible_with_long_reason_and_short_terminal(self): + @pytest.mark.parametrize("height", [11, 12, 14]) + def test_reviewer_deny_warning_is_visible_with_long_reason_and_short_terminal(self, height): cli = _make_cli_stub() cli._approval_state = { "command": "guarded action", @@ -226,7 +229,7 @@ def test_reviewer_deny_warning_is_visible_with_long_reason_and_short_terminal(se import shutil as _shutil with patch("cli.shutil.get_terminal_size", - return_value=_shutil.os.terminal_size((60, 12))): + return_value=_shutil.os.terminal_size((60, height))): fragments = cli._get_approval_display_fragments() rendered = "".join(text for _style, text in fragments) @@ -235,7 +238,7 @@ def test_reviewer_deny_warning_is_visible_with_long_reason_and_short_terminal(se assert rendered.index("flagged as dangerous") < rendered.index("guarded action") assert "…" in rendered from hermes_cli.cli_tui_mixin import _PANEL_RESERVED_BELOW - assert rendered.count("\n") <= 12 - _PANEL_RESERVED_BELOW + assert rendered.count("\n") <= height - _PANEL_RESERVED_BELOW def test_background_task_registers_thread_local_approval_callbacks(self): """Background /btw tasks must use the prompt_toolkit approval UI. diff --git a/tests/hermes_cli/test_reviewer_gateway_propagation.py b/tests/hermes_cli/test_reviewer_gateway_propagation.py index 83098fbfee2d..6b40b13b7ad2 100644 --- a/tests/hermes_cli/test_reviewer_gateway_propagation.py +++ b/tests/hermes_cli/test_reviewer_gateway_propagation.py @@ -2,6 +2,7 @@ from unittest.mock import Mock import threading +import pytest from gateway.session_pending_controls import PendingControls from hermes_cli.gateway_agent import GatewayAgent from tools import terminal_tool @@ -17,3 +18,18 @@ def test_reviewer_denial_reaches_cli_callback(monkeypatch): agent = SimpleNamespace(connection=SimpleNamespace(call=Mock()), session_id="session", _generation=1) GatewayAgent._on_approval(agent, payload, None, None) assert callback.call_args.kwargs["smart_denied"] is True + + +def test_callback_type_error_does_not_retry_without_deny_warning(monkeypatch): + calls = [] + + def callback(command, description, **kwargs): + calls.append(kwargs) + raise TypeError("callback bug") + + monkeypatch.setattr(terminal_tool, "_get_approval_callback", lambda: callback) + agent = SimpleNamespace(connection=SimpleNamespace(call=Mock()), session_id="session", _generation=1) + with pytest.raises(TypeError, match="callback bug"): + GatewayAgent._on_approval(agent, {"choices": ["once", "deny"], "smart_denied": True}, None, None) + assert len(calls) == 1 + assert calls[0]["smart_denied"] is True diff --git a/tests/tools/test_approval_gateway_wait_coalesce_race.py b/tests/tools/test_approval_gateway_wait_coalesce_race.py index ab8091da8e55..4490296a1ac4 100644 --- a/tests/tools/test_approval_gateway_wait_coalesce_race.py +++ b/tests/tools/test_approval_gateway_wait_coalesce_race.py @@ -303,3 +303,16 @@ def test_bulk_ordinary_approval_does_not_resolve_smart_denied(): assert not denied.event.is_set() assert mod.resolve_gateway_approval(SESSION_KEY, "once", request_id="deny") == 1 assert denied.result == "once" + + +def test_unbound_once_does_not_skip_smart_denied_queue_head(): + _clear() + denied = wait_mod._ApprovalEntry({**APPROVAL, "smart_denied": True, "request_id": "deny"}) + ordinary = wait_mod._ApprovalEntry({**APPROVAL, "request_id": "ordinary"}) + with mod._lock: + mod._gateway_queues[SESSION_KEY] = [denied, ordinary] + + assert mod.resolve_gateway_approval(SESSION_KEY, "once") == 0 + assert not denied.event.is_set() + assert not ordinary.event.is_set() + assert mod.resolve_gateway_approval(SESSION_KEY, "deny", resolve_all=True) == 2 diff --git a/tests/tools/test_plugin_approval_review.py b/tests/tools/test_plugin_approval_review.py index 299449cb93bb..3ef2f5eb88a3 100644 --- a/tests/tools/test_plugin_approval_review.py +++ b/tests/tools/test_plugin_approval_review.py @@ -88,6 +88,18 @@ def test_reviewer_approve_still_asks_the_human(monkeypatch): assert "passed" in seen["description"] +@pytest.mark.parametrize("verdict, prompts", [("approve", False), ("escalate", False), ("deny", True)]) +def test_review_first_honours_cached_grant_except_deny(monkeypatch, verdict, prompts): + monkeypatch.setattr(approval, "is_approved", lambda *_: True) + seen = _capture_human(monkeypatch) + calls = _verdict(monkeypatch, verdict) + result = request_tool_approval("terminal", "why", rule_key="k", display="cmd", review=True) + assert calls == ["cmd"] + assert result["approved"] is True + assert bool(seen) is prompts + if prompts: + assert seen["smart_denied"] is True + def test_vault_metadata_is_in_reviewer_display_and_human_prompt(monkeypatch): monkeypatch.setattr("agent.vault_backends.describe_ref", lambda item, field=None: { diff --git a/tests/tools/test_reviewer_fifo_guard.py b/tests/tools/test_reviewer_fifo_guard.py index cf6ba6d0162e..462a92d38ea5 100644 --- a/tests/tools/test_reviewer_fifo_guard.py +++ b/tests/tools/test_reviewer_fifo_guard.py @@ -1,15 +1,4 @@ from tools import approval -from tools.approval_gateway_wait import _ApprovalEntry - - -def test_unnamed_approval_skips_reviewer_denial(monkeypatch): - denied = _ApprovalEntry({"request_id": "a", "smart_denied": True}) - ordinary = _ApprovalEntry({"request_id": "b"}) - monkeypatch.setitem(approval._gateway_queues, "fifo-test", [denied, ordinary]) - assert approval.resolve_gateway_approval("fifo-test", "once") == 1 - assert ordinary.result == "once" - assert denied.result is None - assert approval.resolve_gateway_approval("fifo-test", "once") == 0 def test_review_first_ignores_cached_approval(monkeypatch): diff --git a/tools/approval.py b/tools/approval.py index 224ba07124c8..2703f87c042d 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -169,11 +169,11 @@ def resolve_gateway_approval(session_key: str, choice: str, targets = [entry for entry in queue if not entry.data.get("smart_denied")] queue[:] = [entry for entry in queue if entry.data.get("smart_denied")] else: - target = next((entry for entry in queue - if choice == "deny" or not entry.data.get("smart_denied")), None) - targets = [target] if target else [] - if target: - queue.remove(target) + target = queue[0] + if choice != "deny" and target.data.get("smart_denied"): + return 0 + targets = [target] + queue.pop(0) if not queue: _gateway_queues.pop(session_key, None) # Popping the entry and committing its outcome are ONE critical section: the waiter's @@ -1035,8 +1035,8 @@ def _run_approval_gate( Unattended deny text is ``ctx.block_message(subject, noun, advice)`` unless the caller passes an explicit ``*_deny_message`` (the file-tool write gates word their own). ``review_first``: run the smart reviewer before the human, whatever ``approvals.mode`` is. - Every verdict asks the human; a reviewer DENY is a one-operation owner override, so the - reviewer can never approve or block the request on its own. + A reviewer DENY is a one-operation owner override and always asks the human; other verdicts + honour cached human grants. """ plugin_origin = approval_context.is_plugin_origin_turn() session_key = get_current_session_key() @@ -1091,6 +1091,8 @@ def _run_approval_gate( reviewer_denied = True else: reviewer_denied = False + if not reviewer_denied and not plugin_origin and is_approved(session_key, pattern_key): + return _approved() description = f"{description}\n{_review_line(verdict, note)}" else: reviewer_denied = False diff --git a/tools/approval_gateway_wait.py b/tools/approval_gateway_wait.py index 11a11cbf157b..621d21233a68 100644 --- a/tools/approval_gateway_wait.py +++ b/tools/approval_gateway_wait.py @@ -170,7 +170,7 @@ def _enqueue() -> None: _approval._gateway_queues.setdefault(session_key, []).append(entry) # Lookup and enqueue must be atomic to coalesce concurrent identical requests. - # Once releases followers to reprompt; reviewer DENY never shares consent. + # When the leader finishes, followers re-enter and prompt again. Reviewer DENY never shares consent. while True: with _approval._lock: leader = None From d578b9d05be1ef243e8269c208611f33c5cd32e0 Mon Sep 17 00:00:00 2001 From: mintaka Date: Sun, 8 Nov 2026 21:02:12 -0500 Subject: [PATCH 4/4] fix(approval): reject empty approval request IDs (RIG-8544) An empty request ID no longer resolves the head of the queue, and the managed worker rejects empty prompt IDs. Refs RIG-8544 Co-authored-by: Matt Wilkinson --- agent/managed_worker.py | 3 ++- plugins/platforms/telegram/adapter.py | 2 +- .../test_managed_worker_clarify_timeout.py | 19 +++++++++++++++++++ ...est_approval_gateway_wait_coalesce_race.py | 12 ++++++++++++ tools/approval.py | 2 +- 5 files changed, 35 insertions(+), 3 deletions(-) diff --git a/agent/managed_worker.py b/agent/managed_worker.py index 5939edb57624..3ddfc32d36e8 100644 --- a/agent/managed_worker.py +++ b/agent/managed_worker.py @@ -204,7 +204,8 @@ def read(self): self.finish.set() return if (set(frame) != {'type', 'prompt_id', 'value'} or frame['type'] not in {'approval', 'clarify'} - or not isinstance(frame['prompt_id'], str) or not isinstance(frame['value'], str) + or not isinstance(frame['prompt_id'], str) or not frame['prompt_id'] + or not isinstance(frame['value'], str) or len(frame['value']) > 16384): raise ValueError('invalid_managed_control') if frame['type'] == 'approval': diff --git a/plugins/platforms/telegram/adapter.py b/plugins/platforms/telegram/adapter.py index 097c578bfe09..1176b2b21dc7 100644 --- a/plugins/platforms/telegram/adapter.py +++ b/plugins/platforms/telegram/adapter.py @@ -4732,7 +4732,7 @@ async def _handle_exec_approval_callback(self, query, data: str, cb: dict[str, A _toast("platform.telegram.approval.toast_already_resolved")) if not state: return - session_key, request_id = state if isinstance(state, tuple) else (state, None) + session_key, request_id = state user_display = getattr(query.from_user, "first_name", None) or t("platform.telegram.user_fallback") # Resolve FIRST (unblocks the agent thread), render after: a tap landing after the wait timed out # (count == 0) must NOT claim "Approved" — the command was already denied. diff --git a/tests/gateway/test_managed_worker_clarify_timeout.py b/tests/gateway/test_managed_worker_clarify_timeout.py index 90e8fd6f721c..14e452eeb8a0 100644 --- a/tests/gateway/test_managed_worker_clarify_timeout.py +++ b/tests/gateway/test_managed_worker_clarify_timeout.py @@ -52,3 +52,22 @@ def wait(self, timeout=None): monkeypatch.setattr(worker.threading, 'Event', _Event) assert _controls()._ask('q?', [], False) is None assert waits == [None] + + +@pytest.mark.parametrize('kind', ['approval', 'clarify']) +def test_control_reader_rejects_empty_prompt_id(monkeypatch, kind): + from agent import managed_worker as worker + from tools import approval + + controls = _controls() + controls.finish = threading.Event() + frames = iter([{'type': kind, 'prompt_id': '', 'value': 'once'}, {'type': 'finish'}]) + resolved = [] + monkeypatch.setattr(worker, 'read_frame', lambda _: next(frames)) + monkeypatch.setattr(approval, 'resolve_gateway_approval', lambda *args, **kwargs: resolved.append(args)) + + controls.read() + + assert controls.stopped.is_set() + assert controls.finish.is_set() + assert resolved == [] diff --git a/tests/tools/test_approval_gateway_wait_coalesce_race.py b/tests/tools/test_approval_gateway_wait_coalesce_race.py index 4490296a1ac4..526565a89cee 100644 --- a/tests/tools/test_approval_gateway_wait_coalesce_race.py +++ b/tests/tools/test_approval_gateway_wait_coalesce_race.py @@ -305,6 +305,18 @@ def test_bulk_ordinary_approval_does_not_resolve_smart_denied(): assert denied.result == "once" +def test_empty_request_id_does_not_resolve_queue_head(): + _clear() + ordinary = wait_mod._ApprovalEntry({**APPROVAL, "request_id": "ordinary"}) + with mod._lock: + mod._gateway_queues[SESSION_KEY] = [ordinary] + + assert mod.resolve_gateway_approval(SESSION_KEY, "once", request_id="") == 0 + assert mod._gateway_queues[SESSION_KEY] == [ordinary] + assert not ordinary.event.is_set() + mod._gateway_queues.pop(SESSION_KEY) + + def test_unbound_once_does_not_skip_smart_denied_queue_head(): _clear() denied = wait_mod._ApprovalEntry({**APPROVAL, "smart_denied": True, "request_id": "deny"}) diff --git a/tools/approval.py b/tools/approval.py index 2703f87c042d..e170f27cd36a 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -155,7 +155,7 @@ def resolve_gateway_approval(session_key: str, choice: str, queue = _gateway_queues.get(session_key) if not queue: return 0 - if request_id: + if request_id is not None: targets = [entry for entry in queue if entry.data.get("request_id") == request_id] if not targets: return 0