Skip to content

Replacement: validate forced skills at card creation - #107868

Open
gustavohsouza wants to merge 1 commit into
NousResearch:mainfrom
gustavohsouza:wt/t_283563a3
Open

gustavohsouza wants to merge 1 commit into
NousResearch:mainfrom
gustavohsouza:wt/t_283563a3

Conversation

@gustavohsouza

Copy link
Copy Markdown

What changed

  • Validate forced task skills against the effective assignee profile before any task write.
  • Return an actionable error naming the missing skill, assignee, profile skill directory, and list command.
  • Add regression coverage for a skill present in default but absent in foreman, plus valid skill preservation.
  • Seed affected lifecycle/swarm fixtures with their declared profile skills.

How it was verified

  • uv run --extra dev pytest tests/tools/test_kanban_tools.py tests/hermes_cli/test_kanban_review_lifecycle.py tests/hermes_cli/test_kanban_swarm.py -q -> 58 passed in 6.03s
  • Sabotage run without the validation call: regression failed because the invalid task persisted.
  • uv run --extra dev ruff check hermes_cli/kanban_db.py tests/tools/test_kanban_tools.py tests/hermes_cli/test_kanban_review_lifecycle.py tests/hermes_cli/test_kanban_swarm.py -> All checks passed.
  • git diff --check -> clean.

What was not tested

  • Full repository test suite was not run.
  • Independent subagent review was attempted but the reviewer hit the Azure model rate limit.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cron Cron scheduler and job management labels Sep 11, 2026
@yoyodine-industries

Copy link
Copy Markdown

Ran into this while writing a card that carries --skill, and it lands on the same ground as #44072, #93807 and #104257, so I'm not opening a competing PR. Two cases in the current diff that I measured locally, in case they're useful.

An unassigned card plus a skill name fails outright. create_task calls _validate_task_skills_for_assignee(assignee, skills_list), and _canonical_assignee(None) returns None above it, so normalize_profile_name(assignee or "") receives "" and raises before any name is looked at:

# this PR's logic, verbatim, against a throwaway HERMES_HOME
A unassigned + valid name   : ValueError -> profile name cannot be empty
B disabled name for assignee: NO RAISE -> None
C unknown name (control)    : ValueError -> refused: ['nope-skill']
D unassigned + no skills    : NO RAISE -> None
# a loader-delegating variant, same fixtures
E unassigned + valid name   : no raise
F disabled name for assignee: refused -> "Skill 'demo-skill' is disabled. ..."

So kanban create "..." --skill translation with no --assignee errors even when the name is good. An early return when the assignee is falsy would cover it: there is no registry to judge against until the card is assigned, and assign_task is where the profile actually gets attached.

A disabled skill still passes the check. available is built by walking the profile's skills directory, so a name the profile disables is in the set and the validator returns without complaint (B above). The worker disagrees, because it resolves through the skill loader, which drops disabled names (F above). The card gets created, then the worker exits on Unknown skill(s) before the agent starts, which is the failure mode in #74125.

The control (C) matters here: an unknown name is refused either way, so the check itself works. The two cases above are just gaps in what it asks.

Asking the loader rather than walking the directory covers both, and it would inherit whatever else the loader resolves today. Worth checking whether plugin or bundled skill directories are inside that walk: if they aren't, a name that only resolves through a plugin would be refused even though the worker loads it fine.

I have the loader-delegating version on a local branch, with tests for the disabled case and the unassigned case. Happy to fold it in here if you would rather keep one PR on this concern than two.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management P3 Low — cosmetic, nice to have type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants