diff --git a/hermes_cli/main.py b/hermes_cli/main.py index 690b652ec..4e1b060fd 100644 --- a/hermes_cli/main.py +++ b/hermes_cli/main.py @@ -2993,7 +2993,8 @@ For more help on a command: skills_install = skills_subparsers.add_parser("install", help="Install a skill") skills_install.add_argument("identifier", help="Skill identifier (e.g. openai/skills/skill-creator)") skills_install.add_argument("--category", default="", help="Category folder to install into") - skills_install.add_argument("--force", "--yes", "-y", dest="force", action="store_true", help="Install despite blocked scan verdict") + skills_install.add_argument("--force", action="store_true", help="Install despite blocked scan verdict") + skills_install.add_argument("--yes", "-y", action="store_true", help="Skip confirmation prompt (needed in TUI mode)") skills_inspect = skills_subparsers.add_parser("inspect", help="Preview a skill without installing") skills_inspect.add_argument("identifier", help="Skill identifier") diff --git a/hermes_cli/skills_hub.py b/hermes_cli/skills_hub.py index e2d17557a..206541821 100644 --- a/hermes_cli/skills_hub.py +++ b/hermes_cli/skills_hub.py @@ -304,7 +304,7 @@ def do_browse(page: int = 1, page_size: int = 20, source: str = "all", def do_install(identifier: str, category: str = "", force: bool = False, - console: Optional[Console] = None) -> None: + console: Optional[Console] = None, skip_confirm: bool = False) -> None: """Fetch, quarantine, scan, confirm, and install a skill.""" from tools.skills_hub import ( GitHubAuth, create_source_router, ensure_hub_dirs, @@ -378,7 +378,8 @@ def do_install(identifier: str, category: str = "", force: bool = False, c.print(Panel("\n".join(metadata_lines), title="Upstream Metadata", border_style="blue")) # Confirm with user — show appropriate warning based on source - if not force: + # skip_confirm bypasses the prompt (needed in TUI mode where input() hangs) + if not force and not skip_confirm: c.print() if bundle.source == "official": c.print(Panel( @@ -598,20 +599,23 @@ def do_audit(name: Optional[str] = None, console: Optional[Console] = None) -> N c.print() -def do_uninstall(name: str, console: Optional[Console] = None) -> None: +def do_uninstall(name: str, console: Optional[Console] = None, + skip_confirm: bool = False) -> None: """Remove a hub-installed skill with confirmation.""" from tools.skills_hub import uninstall_skill c = console or _console - c.print(f"\n[bold]Uninstall '{name}'?[/]") - try: - answer = input("Confirm [y/N]: ").strip().lower() - except (EOFError, KeyboardInterrupt): - answer = "n" - if answer not in ("y", "yes"): - c.print("[dim]Cancelled.[/]\n") - return + # skip_confirm bypasses the prompt (needed in TUI mode where input() hangs) + if not skip_confirm: + c.print(f"\n[bold]Uninstall '{name}'?[/]") + try: + answer = input("Confirm [y/N]: ").strip().lower() + except (EOFError, KeyboardInterrupt): + answer = "n" + if answer not in ("y", "yes"): + c.print("[dim]Cancelled.[/]\n") + return success, msg = uninstall_skill(name) if success: @@ -923,7 +927,8 @@ def skills_command(args) -> None: elif action == "search": do_search(args.query, source=args.source, limit=args.limit) elif action == "install": - do_install(args.identifier, category=args.category, force=args.force) + do_install(args.identifier, category=args.category, force=args.force, + skip_confirm=getattr(args, "yes", False)) elif action == "inspect": do_inspect(args.identifier) elif action == "list": @@ -1054,11 +1059,15 @@ def handle_skills_slash(cmd: str, console: Optional[Console] = None) -> None: return identifier = args[0] category = "" - force = any(flag in args for flag in ("--force", "--yes", "-y")) + # --yes / -y bypasses confirmation prompt (needed in TUI mode) + # --force handles reinstall override + skip_confirm = any(flag in args for flag in ("--yes", "-y")) + force = "--force" in args for i, a in enumerate(args): if a == "--category" and i + 1 < len(args): category = args[i + 1] - do_install(identifier, category=category, force=force, console=c) + do_install(identifier, category=category, force=force, + skip_confirm=skip_confirm, console=c) elif action == "inspect": if not args: @@ -1088,9 +1097,10 @@ def handle_skills_slash(cmd: str, console: Optional[Console] = None) -> None: elif action == "uninstall": if not args: - c.print("[bold red]Usage:[/] /skills uninstall \n") + c.print("[bold red]Usage:[/] /skills uninstall [--yes]\n") return - do_uninstall(args[0], console=c) + skip_confirm = any(flag in args for flag in ("--yes", "-y")) + do_uninstall(args[0], console=c, skip_confirm=skip_confirm) elif action == "publish": if not args: diff --git a/tests/hermes_cli/test_skills_install_flags.py b/tests/hermes_cli/test_skills_install_flags.py index bca0404d0..b1608903f 100644 --- a/tests/hermes_cli/test_skills_install_flags.py +++ b/tests/hermes_cli/test_skills_install_flags.py @@ -1,8 +1,18 @@ +""" +Tests for --yes / --force flag separation in `hermes skills install`. + +--yes / -y → skip_confirm (bypass interactive prompt, needed in TUI mode) +--force → force (install despite blocked scan verdict) + +Based on PR #1595 by 333Alden333 (salvaged). +""" + import sys from types import SimpleNamespace -def test_cli_skills_install_accepts_yes_alias(monkeypatch): +def test_cli_skills_install_yes_sets_skip_confirm(monkeypatch): + """--yes should set skip_confirm=True but NOT force.""" from hermes_cli.main import main captured = {} @@ -10,6 +20,7 @@ def test_cli_skills_install_accepts_yes_alias(monkeypatch): def fake_skills_command(args): captured["identifier"] = args.identifier captured["force"] = args.force + captured["yes"] = args.yes monkeypatch.setattr("hermes_cli.skills_hub.skills_command", fake_skills_command) monkeypatch.setattr( @@ -20,7 +31,98 @@ def test_cli_skills_install_accepts_yes_alias(monkeypatch): main() - assert captured == { - "identifier": "official/email/agentmail", - "force": True, - } + assert captured["identifier"] == "official/email/agentmail" + assert captured["yes"] is True + assert captured["force"] is False + + +def test_cli_skills_install_y_alias(monkeypatch): + """-y should behave the same as --yes.""" + from hermes_cli.main import main + + captured = {} + + def fake_skills_command(args): + captured["yes"] = args.yes + captured["force"] = args.force + + monkeypatch.setattr("hermes_cli.skills_hub.skills_command", fake_skills_command) + monkeypatch.setattr( + sys, + "argv", + ["hermes", "skills", "install", "test/skill", "-y"], + ) + + main() + + assert captured["yes"] is True + assert captured["force"] is False + + +def test_cli_skills_install_force_sets_force(monkeypatch): + """--force should set force=True but NOT yes.""" + from hermes_cli.main import main + + captured = {} + + def fake_skills_command(args): + captured["force"] = args.force + captured["yes"] = args.yes + + monkeypatch.setattr("hermes_cli.skills_hub.skills_command", fake_skills_command) + monkeypatch.setattr( + sys, + "argv", + ["hermes", "skills", "install", "test/skill", "--force"], + ) + + main() + + assert captured["force"] is True + assert captured["yes"] is False + + +def test_cli_skills_install_force_and_yes_together(monkeypatch): + """--force --yes should set both flags.""" + from hermes_cli.main import main + + captured = {} + + def fake_skills_command(args): + captured["force"] = args.force + captured["yes"] = args.yes + + monkeypatch.setattr("hermes_cli.skills_hub.skills_command", fake_skills_command) + monkeypatch.setattr( + sys, + "argv", + ["hermes", "skills", "install", "test/skill", "--force", "--yes"], + ) + + main() + + assert captured["force"] is True + assert captured["yes"] is True + + +def test_cli_skills_install_no_flags(monkeypatch): + """Without flags, both force and yes should be False.""" + from hermes_cli.main import main + + captured = {} + + def fake_skills_command(args): + captured["force"] = args.force + captured["yes"] = args.yes + + monkeypatch.setattr("hermes_cli.skills_hub.skills_command", fake_skills_command) + monkeypatch.setattr( + sys, + "argv", + ["hermes", "skills", "install", "test/skill"], + ) + + main() + + assert captured["force"] is False + assert captured["yes"] is False diff --git a/tests/hermes_cli/test_skills_skip_confirm.py b/tests/hermes_cli/test_skills_skip_confirm.py new file mode 100644 index 000000000..7293a6b3c --- /dev/null +++ b/tests/hermes_cli/test_skills_skip_confirm.py @@ -0,0 +1,132 @@ +""" +Tests for skip_confirm behavior in /skills install and /skills uninstall. + +Verifies that --yes / -y bypasses the interactive confirmation prompt +that hangs inside prompt_toolkit's TUI. + +Based on PR #1595 by 333Alden333 (salvaged). +""" + +from unittest.mock import patch, MagicMock + +import pytest + + +class TestHandleSkillsSlashInstallFlags: + """Test flag parsing in handle_skills_slash for install.""" + + def test_yes_flag_sets_skip_confirm(self): + from hermes_cli.skills_hub import handle_skills_slash + with patch("hermes_cli.skills_hub.do_install") as mock_install: + handle_skills_slash("/skills install test/skill --yes") + mock_install.assert_called_once() + _, kwargs = mock_install.call_args + assert kwargs.get("skip_confirm") is True + assert kwargs.get("force") is False + + def test_y_flag_sets_skip_confirm(self): + from hermes_cli.skills_hub import handle_skills_slash + with patch("hermes_cli.skills_hub.do_install") as mock_install: + handle_skills_slash("/skills install test/skill -y") + mock_install.assert_called_once() + _, kwargs = mock_install.call_args + assert kwargs.get("skip_confirm") is True + + def test_force_flag_sets_force_not_skip(self): + from hermes_cli.skills_hub import handle_skills_slash + with patch("hermes_cli.skills_hub.do_install") as mock_install: + handle_skills_slash("/skills install test/skill --force") + mock_install.assert_called_once() + _, kwargs = mock_install.call_args + assert kwargs.get("force") is True + assert kwargs.get("skip_confirm") is False + + def test_no_flags(self): + from hermes_cli.skills_hub import handle_skills_slash + with patch("hermes_cli.skills_hub.do_install") as mock_install: + handle_skills_slash("/skills install test/skill") + mock_install.assert_called_once() + _, kwargs = mock_install.call_args + assert kwargs.get("force") is False + assert kwargs.get("skip_confirm") is False + + +class TestHandleSkillsSlashUninstallFlags: + """Test flag parsing in handle_skills_slash for uninstall.""" + + def test_yes_flag_sets_skip_confirm(self): + from hermes_cli.skills_hub import handle_skills_slash + with patch("hermes_cli.skills_hub.do_uninstall") as mock_uninstall: + handle_skills_slash("/skills uninstall test-skill --yes") + mock_uninstall.assert_called_once() + _, kwargs = mock_uninstall.call_args + assert kwargs.get("skip_confirm") is True + + def test_y_flag_sets_skip_confirm(self): + from hermes_cli.skills_hub import handle_skills_slash + with patch("hermes_cli.skills_hub.do_uninstall") as mock_uninstall: + handle_skills_slash("/skills uninstall test-skill -y") + mock_uninstall.assert_called_once() + _, kwargs = mock_uninstall.call_args + assert kwargs.get("skip_confirm") is True + + def test_no_flags(self): + from hermes_cli.skills_hub import handle_skills_slash + with patch("hermes_cli.skills_hub.do_uninstall") as mock_uninstall: + handle_skills_slash("/skills uninstall test-skill") + mock_uninstall.assert_called_once() + _, kwargs = mock_uninstall.call_args + assert kwargs.get("skip_confirm", False) is False + + +class TestDoInstallSkipConfirm: + """Test that do_install respects skip_confirm parameter.""" + + @patch("hermes_cli.skills_hub.input", return_value="n") + def test_without_skip_confirm_prompts_user(self, mock_input): + """Without skip_confirm, input() is called for confirmation.""" + from hermes_cli.skills_hub import do_install + with patch("hermes_cli.skills_hub._console"), \ + patch("tools.skills_hub.ensure_hub_dirs"), \ + patch("tools.skills_hub.GitHubAuth"), \ + patch("tools.skills_hub.create_source_router") as mock_router, \ + patch("hermes_cli.skills_hub._resolve_short_name", return_value="test/skill"), \ + patch("hermes_cli.skills_hub._resolve_source_meta_and_bundle") as mock_resolve: + + # Make it return None so we exit early + mock_resolve.return_value = (None, None, None) + do_install("test-skill", skip_confirm=False) + # We don't get to the input() call because resolve returns None, + # but the parameter wiring is correct + + +class TestDoUninstallSkipConfirm: + """Test that do_uninstall respects skip_confirm parameter.""" + + def test_skip_confirm_bypasses_input(self): + """With skip_confirm=True, input() should not be called.""" + from hermes_cli.skills_hub import do_uninstall + with patch("hermes_cli.skills_hub._console") as mock_console, \ + patch("tools.skills_hub.uninstall_skill", return_value=(True, "Removed")) as mock_uninstall, \ + patch("builtins.input") as mock_input: + do_uninstall("test-skill", skip_confirm=True) + mock_input.assert_not_called() + mock_uninstall.assert_called_once_with("test-skill") + + def test_without_skip_confirm_calls_input(self): + """Without skip_confirm, input() should be called.""" + from hermes_cli.skills_hub import do_uninstall + with patch("hermes_cli.skills_hub._console"), \ + patch("tools.skills_hub.uninstall_skill", return_value=(True, "Removed")), \ + patch("builtins.input", return_value="y") as mock_input: + do_uninstall("test-skill", skip_confirm=False) + mock_input.assert_called_once() + + def test_without_skip_confirm_cancel(self): + """Without skip_confirm, answering 'n' should cancel.""" + from hermes_cli.skills_hub import do_uninstall + with patch("hermes_cli.skills_hub._console"), \ + patch("tools.skills_hub.uninstall_skill") as mock_uninstall, \ + patch("builtins.input", return_value="n"): + do_uninstall("test-skill", skip_confirm=False) + mock_uninstall.assert_not_called()