Dangerous command approval prompts need context and clarity

Fixes on record

Merged to main. A stamp counts the issues a fix is linked to.

• #1649 fix(approval): show full command in dangerous command approval (#1553)

  merged to main

  files: gateway/platforms/discord.py, tests/conftest.py, tests/tools/test_approval.py, tools/approval.py

  Show the diff

  • gateway/platforms/discord.py

    @@ -1740,9 +1740,12 @@ async def send_exec_approval(
                 if not channel:
                     channel = await self._client.fetch_channel(int(chat_id))
     
    +            # Discord embed description limit is 4096; show full command up to that
    +            max_desc = 4088
    +            cmd_display = command if len(command) <= max_desc else command[: max_desc - 3] + "..."
                 embed = discord.Embed(
                     title="Command Approval Required",
    -                description=f"```\n{command[:500]}\n```",
    +                description=f"```\n{cmd_display}\n```",
                     color=discord.Color.orange(),
                 )
                 embed.set_footer(text=f"Approval ID: {approval_id}")

  • tests/conftest.py

    @@ -107,7 +107,11 @@ def _ensure_current_event_loop(request):
     
     @pytest.fixture(autouse=True)
     def _enforce_test_timeout():
    -    """Kill any individual test that takes longer than 30 seconds."""
    +    """Kill any individual test that takes longer than 30 seconds.
    +    SIGALRM is Unix-only; skip on Windows."""
    +    if sys.platform == "win32":
    +        yield
    +        return
         old = signal.signal(signal.SIGALRM, _timeout_handler)
         signal.alarm(30)
         yield

  • tests/tools/test_approval.py

    @@ -385,75 +385,47 @@ def test_legacy_find_key_still_approves_find_delete(self):
                 assert is_approved("legacy-find", key_delete) is True
     
     
    -class TestViewFullCommand:
    -    """Tests for the 'view full command' option in prompt_dangerous_approval."""
    +class TestFullCommandAlwaysShown:
    +    """The full command is always shown in the approval prompt (no truncation).
     
    -    def test_view_then_once_fallback(self):
    -        """Pressing 'v' shows the full command, then 'o' approves once."""
    +    Previously there was a [v]iew full option for long commands. Now the full
    +    command is always displayed. These tests verify the basic approval flow
    +    still works with long commands. (#1553)
    +    """
    +
    +    def test_once_with_long_command(self):
    +        """Pressing 'o' approves once even for very long commands."""
             long_cmd = "rm -rf " + "a" * 200
    -        inputs = iter(["v", "o"])
    -        with mock_patch("builtins.input", side_effect=inputs):
    +        with mock_patch("builtins.input", return_value="o"):
                 result = prompt_dangerous_approval(long_cmd, "recursive delete")
             assert result == "once"
     
    -    def test_view_then_deny_fallback(self):
    -        """Pressing 'v' shows the full command, then 'd' denies."""
    -        long_cmd = "rm -rf " + "b" * 200
    -        inputs = iter(["v", "d"])
    -        with mock_patch("builtins.input", side_effect=inputs):
    -            result = prompt_dangerous_approval(long_cmd, "recursive delete")
    -        assert result == "deny"
    -
    -    def test_view_then_session_fallback(self):
    -        """Pressing 'v' shows the full command, then 's' approves for session."""
    +    def test_session_with_long_command(self):
    +        """Pressing 's' approves for session with long commands."""
             long_cmd = "rm -rf " + "c" * 200
    -        inputs = iter(["v", "s"])
    -        with mock_patch("builtins.input", side_effect=inputs):
    +        with mock_patch("builtins.input", return_value="s"):
                 result = prompt_dangerous_approval(long_cmd, "recursive delete")
             assert result == "session"
     
    -    def test_view_then_always_fallback(self):
    -        """Pressing 'v' shows the full command, then 'a' approves always."""
    +    def test_always_with_long_command(self):
    +        """Pressing 'a' approves always with long commands."""
             long_cmd = "rm -rf " + "d" * 200
    -        inputs = iter(["v", "a"])
    -        with mock_patch("builtins.input", side_effect=inputs):
    +        with mock_patch("builtins.input", return_value="a"):
                 result = prompt_dangerous_approval(long_cmd, "recursive delete")
             assert result == "always"
     
    -    def test_view_then_session_when_permanent_hidden(self):
    -        """The view-full flow still works when allow_permanent=False."""
    -        long_cmd = "rm -rf " + "d" * 200
    -        inputs = iter(["v", "s"])
    -        with mock_patch("builtins.input", side_effect=inputs):
    -            result = prompt_dangerous_approval(
    -                long_cmd,
    -                "recursive delete",
    -                allow_permanent=False,
    -            )
    -        assert result == "session"
    +    def test_deny_with_long_command(self):
    +        """Pressing 'd' denies with long commands."""
    +        long_cmd = "rm -rf " + "b" * 200
    +        with mock_patch("builtins.input", return_value="d"):
    +            result = prompt_dangerous_approval(long_cmd, "recursive delete")
    +        assert result == "deny"
     
    -    def test_view_not_shown_for_short_command(self):
    -        """Short commands don't offer the view option; 'v' falls through to deny."""
    +    def test_invalid_input_denies(self):
    +        """Invalid input (like 'v' which no longer exists) falls through to deny."""
             short_cmd = "rm -rf /tmp"
             with mock_patch("builtins.input", return_value="v"):
                 result = prompt_dangerous_approval(short_cmd, "recursive delete")
    -        # 'v' is not a valid choice for short commands, should deny
    -        assert result == "deny"
    -
    -    def test_once_without_view(self):
    -        """Directly pressing 'o' without viewing still works."""
    -        long_cmd = "rm -rf " + "e" * 200
    -        with mock_patch("builtins.input", return_value="o"):
    -            result = prompt_dangerous_approval(long_cmd, "recursive delete")
    -        assert result == "once"
    -
    -    def test_view_ignored_after_already_shown(self):
    -        """After viewing once, 'v' on a now-untruncated display falls through to deny."""
    -        long_cmd = "rm -rf " + "f" * 200
    -        inputs = iter(["v", "v"])  # second 'v' should not match since is_truncated is False
    -        with mock_patch("builtins.input", side_effect=inputs):
    -            result = prompt_dangerous_approval(long_cmd, "recursive delete")
    -        # After first 'v', is_truncated becomes False, so second 'v' -> deny
             assert result == "deny"
     
     

  • tools/approval.py

    @@ -220,17 +220,15 @@ def prompt_dangerous_approval(command: str, description: str,
     
         os.environ["HERMES_SPINNER_PAUSE"] = "1"
         try:
    -        is_truncated = len(command) > 80
             while True:
                 print()
                 print(f"  ⚠️  DANGEROUS COMMAND: {description}")
    -            print(f"      {command[:80]}{'...' if is_truncated else ''}")
    +            print(f"      {command}")
                 print()
    -            view_hint = "  |  [v]iew full" if is_truncated else ""
                 if allow_permanent:
    -                print(f"      [o]nce  |  [s]ession  |  [a]lways  |  [d]eny{view_hint}")
    +                print("      [o]nce  |  [s]ession  |  [a]lways  |  [d]eny")
                 else:
    -                print(f"      [o]nce  |  [s]ession  |  [d]eny{view_hint}")
    +                print("      [o]nce  |  [s]ession  |  [d]eny")
                 print()
                 sys.stdout.flush()
     
    @@ -252,12 +250,6 @@ def get_input():
                     return "deny"
     
                 choice = result["choice"]
    -            if choice in ('v', 'view') and is_truncated:
    -                print()
    -                print("      Full command:")
    -                print(f"      {command}")
    -                is_truncated = False
    -                continue
                 if choice in ('o', 'once'):
                     print("      ✓ Allowed once")
                     return "once"
    @@ -394,7 +386,10 @@ def check_dangerous_command(command: str, env_type: str,
                 "status": "approval_required",
                 "command": command,
                 "description": description,
    -            "message": f"⚠️ This command is potentially dangerous ({description}). Asking the user for approval...",
    +            "message": (
    +                f"⚠️ This command is potentially dangerous ({description}). "
    +                f"Asking the user for approval.\n\n**Command:**\n```\n{command}\n```"
    +            ),
             }
     
         choice = prompt_dangerous_approval(command, description,
    @@ -542,7 +537,9 @@ def check_all_command_guards(command: str, env_type: str,
                 "status": "approval_required",
                 "command": command,
                 "description": combined_desc,
    -            "message": f"⚠️ {combined_desc}. Asking the user for approval...",
    +            "message": (
    +                f"⚠️ {combined_desc}. Asking the user for approval.\n\n**Command:**\n```\n{command}\n```"
    +            ),
             }
     
         # CLI interactive: single combined prompt