From 3dbc0246b97d4a1fa1c6e02ddbcf5ce2a7bcf790 Mon Sep 17 00:00:00 2001 From: brooklyn! Date: Thu, 24 Sep 2026 05:15:24 -0500 Subject: [PATCH] fix(tui): do not ok-reply a skill banner from the slash worker Skill slashes that miss the 4018 gate print the loading banner and park the prompt on unread _pending_input. Refuse before process_command, and return command.dispatch's skill payload or a hard error when the skill scan raises so fail-open cannot drop the turn. --- scripts/ci/profile_scope_patterns.json | 2 +- tests/tui_gateway/test_protocol.py | 196 ++++++++++++++++++ .../test_slash_worker_skill_refuse.py | 39 ++++ tui_gateway/methods_tools.py | 53 ++++- tui_gateway/slash_worker.py | 34 +++ 5 files changed, 319 insertions(+), 5 deletions(-) create mode 100644 tests/tui_gateway/test_slash_worker_skill_refuse.py diff --git a/scripts/ci/profile_scope_patterns.json b/scripts/ci/profile_scope_patterns.json index 5594147196..67eb82ec1d 100644 --- a/scripts/ci/profile_scope_patterns.json +++ b/scripts/ci/profile_scope_patterns.json @@ -108,7 +108,7 @@ { "id": "P22", "class": "C3", - "pattern_regex": "scan_skill_commands\\(|get_skill_bundles\\(|resolve_bundle_command_key\\(|_is_profile_skill_command\\(|def _dispatch_(skill|bundle)\\(", + "pattern_regex": "scan_skill_commands\\(|get_skill_bundles\\(|resolve_bundle_command_key\\(|_profile_skill_command\\(|_is_profile_skill_command\\(|def _dispatch_(skill|bundle)\\(", "scope_hint": "command.dispatch's whole stage loop (quick -> plugin -> bundle -> skill) and slash.exec bundle routing must run under the session's profile home; use the home-keyed get_skill_commands().", "why": "The router bound the profile and said 'skill exists', the dispatcher scanned the launch home and answered 4018 'not a skill command' for every secondary-only skill." }, diff --git a/tests/tui_gateway/test_protocol.py b/tests/tui_gateway/test_protocol.py index 65dc032865..3df44602d0 100644 --- a/tests/tui_gateway/test_protocol.py +++ b/tests/tui_gateway/test_protocol.py @@ -1263,6 +1263,202 @@ def test_slash_exec_scopes_skill_lookup_to_session_profile(server, tmp_path): assert resp["error"]["code"] == 4018 +class _BannerWorker: + """Stand-in for the slash worker's current skill path: ok-reply the banner.""" + + def __init__(self): + self.calls = [] + self.closed = False + + def run(self, command): + self.calls.append(command) + return "⚡ Loading skill: grilling" + + def close(self): + self.closed = True + + +def _grilling_profile(tmp_path): + """A session whose profile has only the grilling skill, plus a banner worker.""" + empty_local_dir = tmp_path / "no-local-skills" + empty_local_dir.mkdir() + profile = tmp_path / "profile" + profile.mkdir() + external = tmp_path / "external" + skill_dir = external / "grilling" + skill_dir.mkdir(parents=True) + (skill_dir / "SKILL.md").write_text( + "---\nname: grilling\ndescription: Grill the plan.\n---\n\n# grilling\n\nAsk hard questions.\n" + ) + (profile / "config.yaml").write_text(f"skills:\n external_dirs:\n - {external}\n") + sid = "skill-failopen-session" + worker = _BannerWorker() + return empty_local_dir, { + "session_key": sid, + "agent": None, + "profile_home": str(profile), + "slash_worker": worker, + }, worker + + +def _assert_not_ok_banner(resp, worker): + blob = json.dumps(resp) + assert "Loading skill" not in blob + assert worker.calls == [] + result = resp.get("result") or {} + if result.get("type") == "skill": + assert result.get("message") + assert result.get("name") == "grilling" + return + assert "error" in resp + assert resp["error"]["code"] != 0 + + +def test_slash_exec_skill_scan_raise_returns_dispatch_payload_not_banner(server, tmp_path): + """A skill-scan exception must not fail open into an ok loading banner. + + The client gets command.dispatch's skill payload (the expanded prompt), never + a silent success that drops it. + """ + import agent.skill_commands as sc_mod + + empty_local_dir, session, worker = _grilling_profile(tmp_path) + sid = session["session_key"] + server._sessions[sid] = session + real_get = sc_mod.get_skill_commands + calls = {"n": 0} + + def flaky(): + calls["n"] += 1 + if calls["n"] == 1: + raise OSError("external_dirs hiccup") + return real_get() + + with ( + patch("tools.skills_tool.SKILLS_DIR", empty_local_dir), + patch.object(sc_mod, "get_skill_commands", flaky), + patch.object(sc_mod, "_skill_commands", {}), + patch.object(sc_mod, "_skill_commands_platform", None), + patch.object(sc_mod, "_skill_commands_home", None), + patch.object(sc_mod, "_skill_commands_project", None), + ): + resp = server.handle_request({ + "id": "r1", + "method": "slash.exec", + "params": {"command": "grilling tighten this", "session_id": sid}, + }) + + _assert_not_ok_banner(resp, worker) + assert resp["result"]["type"] == "skill" + assert "tighten this" in resp["result"]["message"] + + +def test_slash_exec_skill_scan_raise_is_hard_error_not_banner_when_dispatch_misses(server, tmp_path): + """If the scan keeps failing, slash.exec still must not ok-reply the banner.""" + import agent.skill_commands as sc_mod + + empty_local_dir, session, worker = _grilling_profile(tmp_path) + sid = session["session_key"] + server._sessions[sid] = session + + def always_raise(): + raise OSError("external_dirs hiccup") + + with ( + patch("tools.skills_tool.SKILLS_DIR", empty_local_dir), + patch.object(sc_mod, "get_skill_commands", always_raise), + patch.object(sc_mod, "_skill_commands", {}), + patch.object(sc_mod, "_skill_commands_home", None), + ): + resp = server.handle_request({ + "id": "r1", + "method": "slash.exec", + "params": {"command": "/grilling", "session_id": sid}, + }) + + _assert_not_ok_banner(resp, worker) + assert "error" in resp + + +def test_slash_exec_skill_scan_raise_still_runs_registry_commands(server): + """A skill-scan exception must not block built-ins the worker owns.""" + import agent.skill_commands as sc_mod + + class _StatusWorker: + def __init__(self): + self.calls = [] + + def run(self, command): + self.calls.append(command) + return "verbose ok" + + def close(self): + pass + + sid = "registry-during-skill-scan-failure" + worker = _StatusWorker() + server._sessions[sid] = {"session_key": sid, "agent": None, "slash_worker": worker} + + with patch.object(sc_mod, "get_skill_commands", side_effect=OSError("external_dirs hiccup")): + resp = server.handle_request({ + "id": "r1", + "method": "slash.exec", + "params": {"command": "/verbose", "session_id": sid}, + }) + + assert worker.calls == ["/verbose"] + assert resp.get("result", {}).get("output") == "verbose ok" + assert "error" not in resp + + +def test_slash_exec_worker_skill_refuse_returns_dispatch_payload(server, tmp_path): + """A worker that refuses a skill before process_command must not become a 5030 drop. + + The gate can miss (stale empty scan) while dispatch still resolves the skill. + The client gets that payload, and the worker stays up. + """ + import agent.skill_commands as sc_mod + + empty_local_dir, session, worker = _grilling_profile(tmp_path) + sid = session["session_key"] + + class _RefuseWorker(_BannerWorker): + def run(self, command): + self.calls.append(command) + raise RuntimeError("skill command refused before process: /grilling") + + worker = _RefuseWorker() + session["slash_worker"] = worker + server._sessions[sid] = session + real_get = sc_mod.get_skill_commands + calls = {"n": 0} + + def stale_then_real(): + calls["n"] += 1 + if calls["n"] == 1: + return {} + return real_get() + + with ( + patch("tools.skills_tool.SKILLS_DIR", empty_local_dir), + patch.object(sc_mod, "get_skill_commands", stale_then_real), + patch.object(sc_mod, "_skill_commands", {}), + patch.object(sc_mod, "_skill_commands_platform", None), + patch.object(sc_mod, "_skill_commands_home", None), + patch.object(sc_mod, "_skill_commands_project", None), + ): + resp = server.handle_request({ + "id": "r1", + "method": "slash.exec", + "params": {"command": "/grilling", "session_id": sid}, + }) + + assert worker.closed is False + assert resp.get("result", {}).get("type") == "skill" + assert resp["result"].get("message") + assert "Loading skill" not in json.dumps(resp) + + def test_command_dispatch_scopes_skill_lookup_to_session_profile(server, tmp_path): """command.dispatch must load a skill that exists only in the session profile.""" import agent.skill_commands as sc_mod diff --git a/tests/tui_gateway/test_slash_worker_skill_refuse.py b/tests/tui_gateway/test_slash_worker_skill_refuse.py new file mode 100644 index 0000000000..8fd578156d --- /dev/null +++ b/tests/tui_gateway/test_slash_worker_skill_refuse.py @@ -0,0 +1,39 @@ +"""Skill slashes must not ok-reply a loading banner from the slash worker.""" + +import pytest + + +def test_slash_worker_refuses_skill_before_process_command(monkeypatch): + """A skill command is refused before process_command, so the banner is never the reply.""" + from tui_gateway import slash_worker + + monkeypatch.setattr("cli.get_skill_commands", lambda: {"/grilling": {"name": "grilling"}}) + + class _CLI: + def __init__(self): + self.console = None + self.called = False + + def process_command(self, cmd): + self.called = True + print("\n⚡ Loading skill: grilling") + + cli = _CLI() + with pytest.raises(Exception, match="skill command refused before process: /grilling"): + slash_worker._run(cli, "/grilling tighten this") + assert cli.called is False + + +def test_slash_worker_still_runs_non_skill_commands(monkeypatch): + from tui_gateway import slash_worker + + monkeypatch.setattr("cli.get_skill_commands", lambda: {"/grilling": {"name": "grilling"}}) + + class _CLI: + def __init__(self): + self.console = None + + def process_command(self, cmd): + print("status ok") + + assert slash_worker._run(_CLI(), "/status") == "status ok" diff --git a/tui_gateway/methods_tools.py b/tui_gateway/methods_tools.py index 6995494d30..0fa22c3042 100644 --- a/tui_gateway/methods_tools.py +++ b/tui_gateway/methods_tools.py @@ -614,11 +614,43 @@ def _session_home_scope(session, cwd: str | None = None): hc.reset_hermes_home_override(token) -def _is_profile_skill_command(session: dict, base: str) -> bool: - """True when ``/base`` is a skill command of the session's profile. False on failure.""" +def _profile_skill_command(session: dict, base: str) -> bool | None: + """True when ``/base`` is a skill of the session profile. + + False when the scan succeeded and it is not. None when the scan raised — + callers must not treat that as "not a skill". Fail-open sends the command to + the slash worker, which ok-replies the loading banner and drops the prompt. + """ try: with _session_home_scope(session): return f"/{base}" in _tools_mod("agent.skill_commands").get_skill_commands() + except Exception: + return None + + +_SKILL_WORKER_REFUSED = "skill command refused before process: /" + + +def _skill_dispatch_or_refuse(rid, sid, base, arg): + """Return command.dispatch's directive, or a hard error. Never an ok banner.""" + dispatched = _methods["command.dispatch"](rid, {"name": base, "arg": arg, "session_id": sid}) + if "error" in dispatched or (dispatched.get("result") or {}).get("type"): + return dispatched + return _err(rid, 4018, f"skill command: use command.dispatch for /{base}") + + +def _worker_refused_skill(exc: BaseException) -> bool: + return _SKILL_WORKER_REFUSED in str(exc) + + +def _is_registry_command(base: str) -> bool: + """True when ``base`` is a built-in the slash worker may still run. + + Skill auto-registration skips names that collide with the registry, so a + built-in cannot be the skill whose prompt the worker would drop. + """ + try: + return _tools_mod("hermes_cli.commands").resolve_command(base) is not None except Exception: return False @@ -923,7 +955,7 @@ def _(rid, params: dict) -> dict: session = _sessions.get(params.get("session_id", "")) # Stage order is load-bearing: quick > plugin > bundle > skill > built-in. One home binding - # around the whole loop: the routing guard (``_is_profile_skill_command``) and the stages + # around the whole loop: the routing guard (``_profile_skill_command``) and the stages # must resolve against the SAME profile or a secondary-only skill is routed here and then # not found (#110695). stages = (_dispatch_quick, _dispatch_plugin, _dispatch_bundle, _dispatch_skill, _SLASH_BUILTINS.get(name)) @@ -962,8 +994,18 @@ def _(rid, params: dict) -> dict: target = base if base in _PENDING_INPUT_COMMANDS else _bundle_key_for(base) if target is not None: return _methods["command.dispatch"](rid, {"name": target.lstrip("/"), "arg": arg, "session_id": sid}) - if _is_profile_skill_command(session, base): + # Recognized skills keep the 4018 gate so clients command.dispatch. A scan + # exception must not fail open into the worker: return the dispatch payload + # (or a hard error) here, or the loading banner swallows the prompt. + skill_hit = _profile_skill_command(session, base) + if skill_hit is True: return _err(rid, 4018, f"skill command: use command.dispatch for /{base}") + if skill_hit is None: + dispatched = _skill_dispatch_or_refuse(rid, sid, base, arg) + # A built-in cannot collide with a skill slug, so the worker may still + # run it. Anything else might be the skill the scan failed to see. + if (dispatched.get("result") or {}).get("type") or not _is_registry_command(base): + return dispatched if plugin_handler := _plugin_command_handler(base) if base else None: try: return _ok(rid, {"output": _run_plugin_command(plugin_handler, arg, session) or "(no output)"}) @@ -994,6 +1036,9 @@ def _(rid, params: dict) -> dict: _publish_session_control_snapshot(sid, session) return _ok(rid, payload) except Exception as e: + if _worker_refused_skill(e): + # Refused before process_command; the worker is still healthy. + return _skill_dispatch_or_refuse(rid, sid, base, arg) with contextlib.suppress(Exception): worker.close() session["slash_worker"] = None diff --git a/tui_gateway/slash_worker.py b/tui_gateway/slash_worker.py index 03790abbbd..f768701ff7 100644 --- a/tui_gateway/slash_worker.py +++ b/tui_gateway/slash_worker.py @@ -64,10 +64,44 @@ def _start_parent_death_watchdog(original_ppid) -> None: threading.Thread(target=_loop, daemon=True).start() +def _slash_base(command: str) -> str: + cmd = (command or "").strip() + if cmd.startswith("/"): + cmd = cmd[1:] + return (cmd.split(maxsplit=1)[0] if cmd else "").lower() + + +class SkillSlashRefused(RuntimeError): + """Skill slash parks the prompt on ``_pending_input``; this worker has no reader.""" + + def __init__(self, base: str): + self.base = base + super().__init__(f"skill command refused before process: /{base}") + + +def _refuse_skill_slash(command: str) -> None: + """Refuse a skill command before ``process_command`` prints the loading banner. + + A scan failure here is not a miss the parent already handled: only a positive + hit is refused, so a broken skill index does not block ``/status``. + """ + base = _slash_base(command) + if not base: + return + try: + from cli import get_skill_commands + commands = get_skill_commands() + except Exception: + return + if f"/{base}" in commands: + raise SkillSlashRefused(base) + + def _run(cli: HermesCLI, command: str) -> str: cmd = (command or "").strip() if not cmd: return "" + _refuse_skill_slash(cmd) buf = io.StringIO() # Rich Console captures its file handle at construction, so redirect_stdout won't affect it; swap # the console's file so self.console.print() is captured. cli._cprint is likewise redirected.