fix(skills): cache-aware /skills install and uninstall in TUI (#3586)
Two fixes for /skills install and /skills uninstall slash commands: 1. input() hangs indefinitely inside prompt_toolkit's TUI event loop, soft-locking the CLI. The user typing the slash command is already implicit consent, so confirmation is now always skipped. 2. Cache invalidation was unconditional — installing or uninstalling a skill mid-session silently broke the prompt cache, increasing costs. The slash handler now defers cache invalidation by default (skill takes effect next session). Pass --now to invalidate immediately, with a message explaining the cost tradeoff. The CLI argparse path (hermes skills install) is unaffected and still invalidates. Fixes #3474 Salvaged from PR #3496 by dlkakbs.
This commit is contained in:
@@ -304,7 +304,8 @@ def do_browse(page: int = 1, page_size: int = 20, source: str = "all",
|
|||||||
|
|
||||||
|
|
||||||
def do_install(identifier: str, category: str = "", force: bool = False,
|
def do_install(identifier: str, category: str = "", force: bool = False,
|
||||||
console: Optional[Console] = None, skip_confirm: bool = False) -> None:
|
console: Optional[Console] = None, skip_confirm: bool = False,
|
||||||
|
invalidate_cache: bool = True) -> None:
|
||||||
"""Fetch, quarantine, scan, confirm, and install a skill."""
|
"""Fetch, quarantine, scan, confirm, and install a skill."""
|
||||||
from tools.skills_hub import (
|
from tools.skills_hub import (
|
||||||
GitHubAuth, create_source_router, ensure_hub_dirs,
|
GitHubAuth, create_source_router, ensure_hub_dirs,
|
||||||
@@ -417,12 +418,16 @@ def do_install(identifier: str, category: str = "", force: bool = False,
|
|||||||
c.print(f"[bold green]Installed:[/] {install_dir.relative_to(SKILLS_DIR)}")
|
c.print(f"[bold green]Installed:[/] {install_dir.relative_to(SKILLS_DIR)}")
|
||||||
c.print(f"[dim]Files: {', '.join(bundle.files.keys())}[/]\n")
|
c.print(f"[dim]Files: {', '.join(bundle.files.keys())}[/]\n")
|
||||||
|
|
||||||
# Invalidate the skills prompt cache so the new skill appears immediately
|
if invalidate_cache:
|
||||||
try:
|
# Invalidate the skills prompt cache so the new skill appears immediately
|
||||||
from agent.prompt_builder import clear_skills_system_prompt_cache
|
try:
|
||||||
clear_skills_system_prompt_cache(clear_snapshot=True)
|
from agent.prompt_builder import clear_skills_system_prompt_cache
|
||||||
except Exception:
|
clear_skills_system_prompt_cache(clear_snapshot=True)
|
||||||
pass
|
except Exception:
|
||||||
|
pass
|
||||||
|
else:
|
||||||
|
c.print("[dim]Skill will be available in your next session.[/]")
|
||||||
|
c.print("[dim]Use /reset to start a new session now, or --now to activate immediately (invalidates prompt cache).[/]\n")
|
||||||
|
|
||||||
|
|
||||||
def do_inspect(identifier: str, console: Optional[Console] = None) -> None:
|
def do_inspect(identifier: str, console: Optional[Console] = None) -> None:
|
||||||
@@ -610,7 +615,8 @@ def do_audit(name: Optional[str] = None, console: Optional[Console] = None) -> N
|
|||||||
|
|
||||||
|
|
||||||
def do_uninstall(name: str, console: Optional[Console] = None,
|
def do_uninstall(name: str, console: Optional[Console] = None,
|
||||||
skip_confirm: bool = False) -> None:
|
skip_confirm: bool = False,
|
||||||
|
invalidate_cache: bool = True) -> None:
|
||||||
"""Remove a hub-installed skill with confirmation."""
|
"""Remove a hub-installed skill with confirmation."""
|
||||||
from tools.skills_hub import uninstall_skill
|
from tools.skills_hub import uninstall_skill
|
||||||
|
|
||||||
@@ -630,11 +636,15 @@ def do_uninstall(name: str, console: Optional[Console] = None,
|
|||||||
success, msg = uninstall_skill(name)
|
success, msg = uninstall_skill(name)
|
||||||
if success:
|
if success:
|
||||||
c.print(f"[bold green]{msg}[/]\n")
|
c.print(f"[bold green]{msg}[/]\n")
|
||||||
try:
|
if invalidate_cache:
|
||||||
from agent.prompt_builder import clear_skills_system_prompt_cache
|
try:
|
||||||
clear_skills_system_prompt_cache(clear_snapshot=True)
|
from agent.prompt_builder import clear_skills_system_prompt_cache
|
||||||
except Exception:
|
clear_skills_system_prompt_cache(clear_snapshot=True)
|
||||||
pass
|
except Exception:
|
||||||
|
pass
|
||||||
|
else:
|
||||||
|
c.print("[dim]Change will take effect in your next session.[/]")
|
||||||
|
c.print("[dim]Use /reset to start a new session now, or --now to apply immediately (invalidates prompt cache).[/]\n")
|
||||||
else:
|
else:
|
||||||
c.print(f"[bold red]Error:[/] {msg}\n")
|
c.print(f"[bold red]Error:[/] {msg}\n")
|
||||||
|
|
||||||
@@ -1071,19 +1081,23 @@ def handle_skills_slash(cmd: str, console: Optional[Console] = None) -> None:
|
|||||||
|
|
||||||
elif action == "install":
|
elif action == "install":
|
||||||
if not args:
|
if not args:
|
||||||
c.print("[bold red]Usage:[/] /skills install <identifier> [--category <cat>] [--force|--yes]\n")
|
c.print("[bold red]Usage:[/] /skills install <identifier> [--category <cat>] [--force] [--now]\n")
|
||||||
return
|
return
|
||||||
identifier = args[0]
|
identifier = args[0]
|
||||||
category = ""
|
category = ""
|
||||||
# --yes / -y bypasses confirmation prompt (needed in TUI mode)
|
# Slash commands run inside prompt_toolkit where input() hangs.
|
||||||
# --force handles reinstall override
|
# Always skip confirmation — the user typing the command is implicit consent.
|
||||||
skip_confirm = any(flag in args for flag in ("--yes", "-y"))
|
skip_confirm = True
|
||||||
force = "--force" in args
|
force = "--force" in args
|
||||||
|
# --now invalidates prompt cache immediately (costs more money).
|
||||||
|
# Default: defer to next session to preserve cache.
|
||||||
|
invalidate_cache = "--now" in args
|
||||||
for i, a in enumerate(args):
|
for i, a in enumerate(args):
|
||||||
if a == "--category" and i + 1 < len(args):
|
if a == "--category" and i + 1 < len(args):
|
||||||
category = args[i + 1]
|
category = args[i + 1]
|
||||||
do_install(identifier, category=category, force=force,
|
do_install(identifier, category=category, force=force,
|
||||||
skip_confirm=skip_confirm, console=c)
|
skip_confirm=skip_confirm, invalidate_cache=invalidate_cache,
|
||||||
|
console=c)
|
||||||
|
|
||||||
elif action == "inspect":
|
elif action == "inspect":
|
||||||
if not args:
|
if not args:
|
||||||
@@ -1113,10 +1127,13 @@ def handle_skills_slash(cmd: str, console: Optional[Console] = None) -> None:
|
|||||||
|
|
||||||
elif action == "uninstall":
|
elif action == "uninstall":
|
||||||
if not args:
|
if not args:
|
||||||
c.print("[bold red]Usage:[/] /skills uninstall <name> [--yes]\n")
|
c.print("[bold red]Usage:[/] /skills uninstall <name> [--now]\n")
|
||||||
return
|
return
|
||||||
skip_confirm = any(flag in args for flag in ("--yes", "-y"))
|
# Slash commands run inside prompt_toolkit where input() hangs.
|
||||||
do_uninstall(args[0], console=c, skip_confirm=skip_confirm)
|
skip_confirm = True
|
||||||
|
invalidate_cache = "--now" in args
|
||||||
|
do_uninstall(args[0], console=c, skip_confirm=skip_confirm,
|
||||||
|
invalidate_cache=invalidate_cache)
|
||||||
|
|
||||||
elif action == "publish":
|
elif action == "publish":
|
||||||
if not args:
|
if not args:
|
||||||
|
|||||||
@@ -1,10 +1,13 @@
|
|||||||
"""
|
"""
|
||||||
Tests for skip_confirm behavior in /skills install and /skills uninstall.
|
Tests for skip_confirm and invalidate_cache behavior in /skills install
|
||||||
|
and /skills uninstall slash commands.
|
||||||
|
|
||||||
Verifies that --yes / -y bypasses the interactive confirmation prompt
|
Slash commands always skip confirmation (input() hangs in TUI).
|
||||||
that hangs inside prompt_toolkit's TUI.
|
Cache invalidation is deferred by default; --now opts into immediate
|
||||||
|
invalidation (at the cost of breaking prompt cache mid-session).
|
||||||
|
|
||||||
Based on PR #1595 by 333Alden333 (salvaged).
|
Based on PR #1595 by 333Alden333 (salvaged).
|
||||||
|
Updated for PR #3586 (cache-aware install/uninstall).
|
||||||
"""
|
"""
|
||||||
|
|
||||||
from unittest.mock import patch, MagicMock
|
from unittest.mock import patch, MagicMock
|
||||||
@@ -32,23 +35,43 @@ class TestHandleSkillsSlashInstallFlags:
|
|||||||
_, kwargs = mock_install.call_args
|
_, kwargs = mock_install.call_args
|
||||||
assert kwargs.get("skip_confirm") is True
|
assert kwargs.get("skip_confirm") is True
|
||||||
|
|
||||||
def test_force_flag_sets_force_not_skip(self):
|
def test_force_flag_sets_force(self):
|
||||||
from hermes_cli.skills_hub import handle_skills_slash
|
from hermes_cli.skills_hub import handle_skills_slash
|
||||||
with patch("hermes_cli.skills_hub.do_install") as mock_install:
|
with patch("hermes_cli.skills_hub.do_install") as mock_install:
|
||||||
handle_skills_slash("/skills install test/skill --force")
|
handle_skills_slash("/skills install test/skill --force")
|
||||||
mock_install.assert_called_once()
|
mock_install.assert_called_once()
|
||||||
_, kwargs = mock_install.call_args
|
_, kwargs = mock_install.call_args
|
||||||
assert kwargs.get("force") is True
|
assert kwargs.get("force") is True
|
||||||
assert kwargs.get("skip_confirm") is False
|
# Slash commands always skip confirmation (input() hangs in TUI)
|
||||||
|
assert kwargs.get("skip_confirm") is True
|
||||||
|
|
||||||
def test_no_flags(self):
|
def test_no_flags_still_skips_confirm(self):
|
||||||
|
"""Slash commands always skip confirmation — input() hangs in TUI."""
|
||||||
from hermes_cli.skills_hub import handle_skills_slash
|
from hermes_cli.skills_hub import handle_skills_slash
|
||||||
with patch("hermes_cli.skills_hub.do_install") as mock_install:
|
with patch("hermes_cli.skills_hub.do_install") as mock_install:
|
||||||
handle_skills_slash("/skills install test/skill")
|
handle_skills_slash("/skills install test/skill")
|
||||||
mock_install.assert_called_once()
|
mock_install.assert_called_once()
|
||||||
_, kwargs = mock_install.call_args
|
_, kwargs = mock_install.call_args
|
||||||
assert kwargs.get("force") is False
|
assert kwargs.get("force") is False
|
||||||
assert kwargs.get("skip_confirm") is False
|
assert kwargs.get("skip_confirm") is True
|
||||||
|
|
||||||
|
def test_default_defers_cache_invalidation(self):
|
||||||
|
"""Without --now, cache invalidation is deferred to next session."""
|
||||||
|
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("invalidate_cache") is False
|
||||||
|
|
||||||
|
def test_now_flag_invalidates_cache(self):
|
||||||
|
"""--now opts into immediate cache invalidation."""
|
||||||
|
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 --now")
|
||||||
|
mock_install.assert_called_once()
|
||||||
|
_, kwargs = mock_install.call_args
|
||||||
|
assert kwargs.get("invalidate_cache") is True
|
||||||
|
|
||||||
|
|
||||||
class TestHandleSkillsSlashUninstallFlags:
|
class TestHandleSkillsSlashUninstallFlags:
|
||||||
@@ -70,13 +93,32 @@ class TestHandleSkillsSlashUninstallFlags:
|
|||||||
_, kwargs = mock_uninstall.call_args
|
_, kwargs = mock_uninstall.call_args
|
||||||
assert kwargs.get("skip_confirm") is True
|
assert kwargs.get("skip_confirm") is True
|
||||||
|
|
||||||
def test_no_flags(self):
|
def test_no_flags_still_skips_confirm(self):
|
||||||
|
"""Slash commands always skip confirmation — input() hangs in TUI."""
|
||||||
from hermes_cli.skills_hub import handle_skills_slash
|
from hermes_cli.skills_hub import handle_skills_slash
|
||||||
with patch("hermes_cli.skills_hub.do_uninstall") as mock_uninstall:
|
with patch("hermes_cli.skills_hub.do_uninstall") as mock_uninstall:
|
||||||
handle_skills_slash("/skills uninstall test-skill")
|
handle_skills_slash("/skills uninstall test-skill")
|
||||||
mock_uninstall.assert_called_once()
|
mock_uninstall.assert_called_once()
|
||||||
_, kwargs = mock_uninstall.call_args
|
_, kwargs = mock_uninstall.call_args
|
||||||
assert kwargs.get("skip_confirm", False) is False
|
assert kwargs.get("skip_confirm") is True
|
||||||
|
|
||||||
|
def test_default_defers_cache_invalidation(self):
|
||||||
|
"""Without --now, cache invalidation is deferred to next session."""
|
||||||
|
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("invalidate_cache") is False
|
||||||
|
|
||||||
|
def test_now_flag_invalidates_cache(self):
|
||||||
|
"""--now opts into immediate cache invalidation."""
|
||||||
|
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 --now")
|
||||||
|
mock_uninstall.assert_called_once()
|
||||||
|
_, kwargs = mock_uninstall.call_args
|
||||||
|
assert kwargs.get("invalidate_cache") is True
|
||||||
|
|
||||||
|
|
||||||
class TestDoInstallSkipConfirm:
|
class TestDoInstallSkipConfirm:
|
||||||
|
|||||||
Reference in New Issue
Block a user