From 6cddbdab4823e0f778809dacb6f1bbf7411ecd79 Mon Sep 17 00:00:00 2001 From: Terri Chan Date: Wed, 19 Aug 2026 07:51:55 +0700 Subject: [PATCH] fix(kanban): drop unknown per-task skills instead of crashing workers Pinning a skill the assignee profile does not have used to spawn `hermes --skills ` and raise ValueError on boot when every pin was missing. Filter the pin list against the assignee's skills tree at create time and again at spawn. Record dropped names on the created event. No runtime install. Leave pins alone when the assignee profile directory does not exist, so review-dispatch tests and first-time assignees still work. --- hermes_cli/kanban_db.py | 123 +++++++++++++++++- .../test_kanban_core_functionality.py | 123 ++++++++++++++++++ website/docs/user-guide/features/kanban.md | 2 +- 3 files changed, 241 insertions(+), 7 deletions(-) diff --git a/hermes_cli/kanban_db.py b/hermes_cli/kanban_db.py index 79398b7d6881a..78c2f30578980 100644 --- a/hermes_cli/kanban_db.py +++ b/hermes_cli/kanban_db.py @@ -3155,6 +3155,96 @@ def _canonical_assignee(assignee: Optional[str]) -> Optional[str]: return normalize_profile_name(assignee) +_SKILL_FRONTMATTER_NAME = re.compile( + r"^name:\s*[\"']?([A-Za-z0-9][A-Za-z0-9._-]*)", + re.MULTILINE, +) + + +def _add_skill_md_names(path: Path, names: set[str]) -> None: + names.add(path.parent.name) + try: + text = path.read_text(encoding="utf-8", errors="replace")[:2000] + except OSError: + return + match = _SKILL_FRONTMATTER_NAME.search(text) + if match: + names.add(match.group(1)) + + +def _collect_skill_names( + root: Path, names: set[str], seen: Optional[set[Path]] = None +) -> None: + """Walk a skills tree, following directory symlinks, collecting names.""" + visited = seen if seen is not None else set() + try: + resolved = root.resolve() + except OSError: + return + if resolved in visited: + return + visited.add(resolved) + if resolved.is_file() and resolved.name == "SKILL.md": + _add_skill_md_names(resolved, names) + return + if not resolved.is_dir(): + return + skill_md = resolved / "SKILL.md" + if skill_md.is_file(): + _add_skill_md_names(skill_md, names) + return + try: + children = list(resolved.iterdir()) + except OSError: + return + for child in children: + _collect_skill_names(child, names, visited) + + +def _assignee_skill_catalog(assignee: Optional[str]) -> Optional[set[str]]: + """Skill names the assignee profile can ``--skills`` load, or None if unknown. + + ``None`` means we cannot see a profile directory, so the pin list is left + alone (review-dispatch tests and not-yet-created assignees). An existing + profile with no ``skills/`` tree is an empty catalog — every pin is dropped. + """ + if not assignee: + return None + try: + from hermes_cli.profiles import get_profile_dir + + home = get_profile_dir(assignee) + except Exception: + return None + if not home.is_dir(): + return None + names: set[str] = set() + skills_root = home / "skills" + if skills_root.exists(): + _collect_skill_names(skills_root, names) + return names + + +def _filter_skills_for_assignee( + assignee: Optional[str], skills: list[str] +) -> tuple[list[str], list[str]]: + """Split *skills* into (kept, dropped) against the assignee catalog. + + When the catalog cannot be resolved, every name is kept. + """ + catalog = _assignee_skill_catalog(assignee) + if catalog is None: + return list(skills), [] + kept: list[str] = [] + dropped: list[str] = [] + for name in skills: + if name in catalog: + kept.append(name) + else: + dropped.append(name) + return kept, dropped + + def create_task( conn: sqlite3.Connection, *, @@ -3203,8 +3293,11 @@ def create_task( ``skills`` is an optional list of skill names to force-load into the worker when dispatched. Stored as JSON; the dispatcher passes - each name to ``hermes --skills ...``. Use this to pin a task to a - specialist skill (e.g. ``skills=["translation"]`` so the worker loads the + each name to ``hermes --skills ...``. Names the assignee profile + cannot load are dropped (recorded on the ``created`` event as + ``dropped_skills``) so the worker does not crash on boot. Use this + to pin a task to a specialist skill (e.g. ``skills=["translation"]`` + so the worker loads the translation skill regardless of the profile's default config). ``model_override`` / ``provider_override`` pin the worker to a specific @@ -3393,6 +3486,14 @@ def create_task( ) skills_list = cleaned + dropped_skills: list[str] = [] + if skills_list: + skills_list, dropped_skills = _filter_skills_for_assignee( + assignee, skills_list + ) + if not skills_list: + skills_list = None + # Idempotency check — return the existing task instead of creating a # duplicate. Done BEFORE entering write_txn to keep the fast path fast # and to avoid holding a write lock during the lookup. Race is @@ -3549,6 +3650,7 @@ def create_task( "branch_name": branch_name, "project_id": project_id, "skills": list(skills_list) if skills_list else None, + "dropped_skills": list(dropped_skills) or None, "goal_mode": bool(goal_mode) or None, "model_override": model_override, "provider_override": provider_override, @@ -10853,10 +10955,19 @@ def _default_spawn( # accepts both forms (action='append' + comma-split), but # per-name pairs are easier to read in `ps` output and avoid any # quoting ambiguity if a skill name ever contains unusual chars. - if task.skills: - for sk in task.skills: - if sk: - cmd.extend(["--skills", sk]) + spawn_skills, dropped_skills = _filter_skills_for_assignee( + task.assignee, list(task.skills or []) + ) + if dropped_skills: + _log.warning( + "Dropping unknown skills for assignee %s on task %s: %s", + task.assignee, + task.id, + ", ".join(dropped_skills), + ) + for sk in spawn_skills: + if sk: + cmd.extend(["--skills", sk]) if task.model_override: cmd.extend(["-m", task.model_override]) # Pin the provider too when the override names one, so the worker diff --git a/tests/hermes_cli/test_kanban_core_functionality.py b/tests/hermes_cli/test_kanban_core_functionality.py index 0a47445ec1108..c8f46ece82938 100644 --- a/tests/hermes_cli/test_kanban_core_functionality.py +++ b/tests/hermes_cli/test_kanban_core_functionality.py @@ -748,8 +748,131 @@ def fake_popen(cmd, **kwargs): # --------------------------------------------------------------------------- +def _plant_profile_skill(home: Path, profile: str, skill_name: str, category: str = "general") -> Path: + """Write a minimal SKILL.md the assignee catalog scanner can see.""" + if profile == "default": + root = home / "skills" / category / skill_name + else: + root = home / "profiles" / profile / "skills" / category / skill_name + root.mkdir(parents=True, exist_ok=True) + (root / "SKILL.md").write_text( + f"---\nname: {skill_name}\ndescription: test fixture\n---\n\n# {skill_name}\n", + encoding="utf-8", + ) + return root + + +def test_create_task_drops_skills_missing_from_assignee_catalog(kanban_home): + """Pinning a skill the assignee cannot load must not be stored. + + That used to spawn ``hermes --skills `` and crash the worker + on boot when every pinned name was missing from the profile. + """ + _plant_profile_skill(kanban_home, "orch", "human-in-loop-agent-graphs") + conn = kb.connect() + try: + tid = kb.create_task( + conn, + title="drop unknown pins", + assignee="orch", + skills=["human-in-loop-agent-graphs", "revenueos"], + ) + task = kb.get_task(conn, tid) + payload = json.loads( + conn.execute( + "SELECT payload FROM task_events WHERE task_id = ? AND kind = 'created'", + (tid,), + ).fetchone()[0] + ) + finally: + conn.close() + + assert task.skills == ["human-in-loop-agent-graphs"] + assert payload.get("dropped_skills") == ["revenueos"] + +def test_create_task_keeps_skills_when_assignee_has_no_catalog(kanban_home): + """No profile dir means we cannot tell — leave the pin list alone. + Review-dispatch tests (and first-time assignees) rely on this: they + pin skills without planting a profile tree. + """ + conn = kb.connect() + try: + tid = kb.create_task( + conn, + title="unknown assignee", + assignee="ghost", + skills=["domain-specific-review"], + ) + task = kb.get_task(conn, tid) + finally: + conn.close() + + assert task.skills == ["domain-specific-review"] + + +def test_create_task_sees_symlinked_skill_on_assignee(kanban_home): + """A skill installed as a symlink into the profile still counts as present.""" + real = _plant_profile_skill(kanban_home, "default", "hermes-profiles", "autonomous-ai-agents") + dest_parent = kanban_home / "profiles" / "orch" / "skills" / "autonomous-ai-agents" + dest_parent.mkdir(parents=True) + os.symlink(real, dest_parent / "hermes-profiles") + + conn = kb.connect() + try: + tid = kb.create_task( + conn, + title="symlink pin", + assignee="orch", + skills=["hermes-profiles", "revenueos"], + ) + task = kb.get_task(conn, tid) + finally: + conn.close() + + assert task.skills == ["hermes-profiles"] + + +def test_default_spawn_omits_skills_missing_from_assignee(kanban_home, monkeypatch): + """Dispatcher must not pass ``--skills`` for names the profile lacks. + + Belt-and-suspenders for rows written before the create-time filter, + or updated by hand. + """ + _plant_profile_skill(kanban_home, "orch", "plan") + captured = {} + + class FakeProc: + def __init__(self): + self.pid = 4242 + + def fake_popen(cmd, **kwargs): + captured["cmd"] = cmd + return FakeProc() + + monkeypatch.setattr("subprocess.Popen", fake_popen) + + conn = kb.connect() + try: + tid = kb.create_task(conn, title="stale pin", assignee="orch", skills=["plan"]) + conn.execute( + "UPDATE tasks SET skills = ? WHERE id = ?", + (json.dumps(["plan", "revenueos"]), tid), + ) + conn.commit() + task = kb.get_task(conn, tid) + workspace = kb.resolve_workspace(task) + pid = kb._default_spawn(task, str(workspace)) + assert pid == 4242 + finally: + conn.close() + + cmd = captured["cmd"] + skills_flags = [ + cmd[i + 1] for i, tok in enumerate(cmd) if tok == "--skills" + ] + assert skills_flags == ["plan"], cmd diff --git a/website/docs/user-guide/features/kanban.md b/website/docs/user-guide/features/kanban.md index ebb1c26d6590e..8e97789a432b9 100644 --- a/website/docs/user-guide/features/kanban.md +++ b/website/docs/user-guide/features/kanban.md @@ -461,7 +461,7 @@ hermes kanban create "audit auth flow" \ **From the dashboard**, type the skills comma-separated into the **skills** field of the create-task dialog. -The dispatcher emits one `--skills ` flag per skill listed, so the worker spawns with all of them loaded on top of the auto-injected kanban guidance. The skill names must match skills that are actually installed on the assignee's profile (run `hermes skills list` to see what's available); there's no runtime install. +The dispatcher emits one `--skills ` flag per skill listed, so the worker spawns with all of them loaded on top of the auto-injected kanban guidance. Names the assignee profile cannot load are **dropped** at create time (and again at spawn) instead of crashing the worker — there is still no runtime install. Run `hermes -p skills list` to see what that profile actually has before pinning. ### Per-task model override