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.
This commit is contained in:
brooklyn!
2026-09-24 05:15:24 -05:00
parent 890db53396
commit 3dbc0246b9
5 changed files with 319 additions and 5 deletions

View File

@@ -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."
},

View File

@@ -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

View File

@@ -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"

View File

@@ -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

View File

@@ -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.