fix(kanban): validate --skill against the worker profile's skill registry at create time - #44101
fix(kanban): validate --skill against the worker profile's skill registry at create time#44101AIalliAI wants to merge 2 commits into
Conversation
|
Duplicate of #44092 — the earlier open PR fixing the same issue (#44072) with the same approach: validate |
|
Status update on the duplicate flag above: #44092 has since been closed by its author in favor of this PR (it didn't cover relative-path/alias/ |
|
Requesting maintainer review — this is ready to land from my side. Standalone fork CI is pending first-run approval here; the rollup branch in #44061 carrying this session's batch is fully green on upstream CI (all test shards, typecheck, e2e). |
cfcb469 to
6dd228e
Compare
39d92a2 to
948345e
Compare
…stry at create time `hermes kanban create --skill <name>` accepted any string; unknown names only surfaced when the spawned worker crashed at CLI startup (`ValueError: Unknown skill(s): <name>`), burning the task's retry budget and completing with an empty result. Validate at create time instead: - kanban_db.unresolvable_task_skills() best-effort mirrors skill_view() lookup (dir-with-SKILL.md by name or relative path, frontmatter name: alias, legacy flat <name>.md) against the skills roots the worker will actually see: <home>/skills plus skills.external_dirs from that home's config.yaml. Names it can't reliably check cross-profile get the benefit of the doubt: plugin-qualified ns:skill names and the built-in kanban-worker (the dispatcher already gates that one on resolvability). - _cmd_create resolves the assignee profile's HERMES_HOME the same way the dispatcher's _default_spawn does, and rejects with exit 2 and a descriptive error listing the unknown skill(s) before enqueueing. Fixes NousResearch#44072
948345e to
d5af8ef
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing the worker retry-budget failure. The all-unresolvable case still exists on current main, but this implementation needs revision before salvage.
Problems
hermes_cli/kanban.py:1351rejects any unknown entry. Current main intentionally skips unknown skills when another requested skill loads (cli.py:15933-15947, commit018009bc382de86f864e3fa8af3982c2f0aa7bfb), so mixed valid/invalid task skills must remain enqueueable.hermes_cli/kanban_db.py:7677checks relativeskills.external_dirsagainst the creator cwd. Worker resolution anchors relative paths at its profile HERMES_HOME (agent/skill_utils.py:483-487), causing false rejection.hermes_cli/kanban_db.py:7744bypassesskill_viewexclusion and ambiguity behavior (agent/skill_utils.py:785-807;tools/skills_tool.py:1182-1204).
Suggested changes
- Reject only when every requested skill is unresolvable, and cover the mixed-list case.
- Reuse worker-equivalent external-directory, exclusion, and collision semantics.
GitHub currently reports the PR mergeable, but its base predates the mixed-skill behavior and requires substantive behavioral rework. This is an automated hermes-sweeper review.
| unknown = kb.unresolvable_task_skills( | ||
| skills, _assignee_hermes_home(args.assignee) | ||
| ) | ||
| if unknown: |
There was a problem hiding this comment.
This rejects a mixed valid/invalid list, but current main intentionally skips unknown entries when at least one requested skill loads (cli.py:15933-15947, 018009bc). Please preserve that behavior and reject only the all-unresolvable case.
| for raw in external: | ||
| if not isinstance(raw, str) or not raw.strip(): | ||
| continue | ||
| ext = _Path(os.path.expandvars(raw)).expanduser() |
There was a problem hiding this comment.
Relative skills.external_dirs must be resolved against the worker profile’s HERMES_HOME, not the creator cwd. See agent/skill_utils.py:483-487; otherwise this validator can reject a skill the worker would load.
| if "/" in name or "\\" in name: | ||
| continue | ||
| try: | ||
| for skill_md in root.rglob("SKILL.md"): |
There was a problem hiding this comment.
Use the shared indexed-skill iteration semantics here: raw rglob() includes paths that skill_view excludes and does not model its ambiguity rejection, so it can falsely accept a worker-invalid name.
…l_dirs, indexed lookup Three substantive changes to the kanban create-time skill validator: 1. Mixed valid/invalid skills (review #1): Reject only when ALL requested skills are unresolvable. When some load and some don't, accept the task — matching main's behavior (commit 018009b) of skipping unknown entries and continuing with whatever loaded. 2. Relative external_dirs (review #2): Resolve relative paths in skills.external_dirs against the worker's HERMES_HOME, not the creator cwd. Mirrors get_external_skills_dirs() in agent/skill_utils.py. 3. Shared indexed skill lookup (review #3): Replace raw rglob() with iter_skill_index_files() so excluded paths (VCS, venv, node_modules, cache dirs, skill support dirs) are not counted. Add ambiguity rejection: when a name matches across multiple search roots, treat it as unresolvable — matching skill_view()'s refusal to guess. +6 new tests covering mixed valid/invalid, relative external_dirs, ambiguity rejection, excluded paths, and skill support dir exclusion.
Summary
hermes kanban create --skill <name>accepted any string. Unknown names only surfaced when the spawned worker crashed at CLI startup (ValueError: Unknown skill(s): <name>) — burning the task's retry budget and completing with an empty result.This validates at create time and rejects the task with exit 2 and a descriptive error before it is enqueued.
Fixes #44072
How it works
kanban_db.unresolvable_task_skills(skills, hermes_home)— best-effort mirror ofskill_view()'s lookup strategies (directory named<name>containingSKILL.mdat the root or nested, relative-path formcat/name, frontmattername:alias, legacy flat<name>.md) against the skills roots the worker will actually see:<home>/skillsplusskills.external_dirsfrom that home'sconfig.yaml(read directly, since the validating CLI may run under a differentHERMES_HOMEthan the worker)._cmd_createresolves the assignee profile'sHERMES_HOMEthe same way the dispatcher's_default_spawndoes (resolve_profile_env), so validation runs against the worker's registry, not the creator's — the exact failure mode in the issue ("a profile dispatching a skill not installed in the target assignee's profile").Deliberately conservative
A name is only rejected when every cheap strategy misses. Names that can't be reliably checked cross-profile get the benefit of the doubt:
namespace:skillnames (plugin registries are per-home state), andkanban-worker(the dispatcher already gates it on resolvability and silently drops it when absent — never fatal).So a false accept degrades to today's behavior, while a reject is always a real startup crash avoided.
Error output
Tests
tests/hermes_cli/test_kanban_create_skill_validation.py(11 tests): unit coverage for every resolution strategy (direct dir, relative path, frontmatter alias, legacy flat .md, external_dirs, missing skills root) and end-to-endrun_slashcoverage including the assignee-profile-home case in both directions (skill only in profile home → accepted; skill only in root home → rejected).test_cli_create_skill_flag_repeatable/test_cli_show_renders_skillsto install the skills they reference (they test persistence/rendering, not resolution).tests/hermes_cli -k kanbanselection produces a failure set identical toorigin/mainbaseline (the handful of pre-existing ordering/environment-dependent failures are untouched).