feat(skills): declare and enforce skill-to-skill dependencies (depends_on)

[mehmetkr-31]

Closes #71853.

Skills carry two kinds of prerequisite metadata today — prerequisites (env vars, commands) and related_skills (advisory cross-references) — but neither says "this skill does not work without that one", and nothing was enforced at install time. A skill that drives another skill's commands installs cleanly on its own; the failure surfaces only when the agent reaches for the missing piece, or the skill silently runs degraded.

The field

depends_on: [obsidian, kanban]          # shorthand — all required

depends_on:
  - name: obsidian
    required: true
    reason: "reads from Obsidian vaults"
  - name: kanban
    required: false

The gate

hermes skill install resolves it after the security verdict and before the confirmation prompt, so a blocked install never reaches "Install 'x'?" and the user is told what to install first rather than discovering it at runtime:

Installation blocked: 'chronicle' requires 1 skill(s) that are not installed:
  • obsidian — reads from Obsidian vaults

Install them first:  hermes skill install obsidian
Or bypass this check:  hermes skill install chronicle --force

Required blocks, optional warns and proceeds, --force downgrades the block — matching how --force already overrides the security verdict.

Two details worth review

Installed-ness is read from the lockfile and from disk. The hub lockfile only knows hub installs, but official skills shipped with Hermes and hand-placed directories are equally installed from the agent's point of view. A dependency the user genuinely has must never be reported missing, so installed_skill_names() unions both (skipping .hub / .trash).

Malformed entries are skipped, never raised. A bad depends_on must not make an otherwise installable skill uninstallable, so junk list entries and unparseable front-matter fall through to today's behaviour.

Verification — each piece verified to fail when removed

reverted	result
the do_install call site	2 failed — "do_install ran to completion with a missing required dependency — the gate is defined but never called"
required deps no longer block	3 failed
on-disk skills not counted as installed	1 failed — "its dependents would be blocked for a dependency the user has"
malformed entries raise instead of skip	1 failed
nothing	21 passed

TestDoInstallActuallyCallsTheGate exists specifically because every other test calls _check_skill_dependencies directly and would pass with the call site deleted — the feature would ship declared and unenforced, which is the exact thing the issue is about. It drives the real do_install over a loopback-served skill, reusing the scaffolding from tests/tools/test_skill_bundle_provenance.py.

tests/tools -k skill: 14 failures on this branch, 14 on a pristine origin/main worktree at the same base — identical set. tests/hermes_cli -k skill: 104 passed.

Deliberately out of scope

Auto-installing missing dependencies would mean running the security scan and confirmation flow recursively, which deserves its own review. The error names the exact command instead. Happy to follow up with --with-optional and prompted auto-install if you want that shape.

[mehmetkr-31]

CI note: the single red job is not this PR. The only failing test is tests/hermes_cli/test_update_eol_churn.py::test_churn_across_more_files_than_fit_in_one_argv — the known _normalize_managed_eol CRLF-churn cluster tracked as #75175, with a fix in flight as #75213. It touches no file this PR touches (skills hub / skills tool / docs) and reproduces on unmodified main. Happy to rebase once #75213 lands.

[teknium1]

Thanks for adding a focused install-time dependency declaration and an end-to-end install-path test. The premise is real: current do_install moves from the security-policy check at hermes_cli/skills_hub.py:691-701 directly to confirmation at :708-710, without dependency enforcement.

Problems

• installed_skill_names() in the PR (tools/skills_hub.py:3373) only scans _skills_dir() and records skill_md.parent.name. Hermes treats configured skills.external_dirs as active skill roots (agent/skill_utils.py:566-574; tools/skills_tool.py:696-721), so an externally supplied required dependency would be falsely reported missing.
• Runtime discovery uses frontmatter.name before falling back to the directory name (tools/skills_tool.py:737). A hand-placed skill in a differently named directory is therefore usable at runtime but absent from this gate. The existing tools/skills_sync.py:84-110 indexes external skills by both forms.

Suggested changes

• Enumerate the same local and external roots as runtime discovery, and index parsed frontmatter names with a directory-name fallback.
• Add tests for an external-root dependency and a frontmatter-name/directory-name mismatch.

Automated hermes-sweeper review.

[teknium1]

This only scans the active profile root. Runtime skill discovery also scans configured skills.external_dirs (agent/skill_utils.py:566-574, tools/skills_tool.py:696-721), so a usable dependency installed there is falsely blocked. Please enumerate the same roots and parse each SKILL.md's declared name as well; runtime discovery treats that name as canonical (tools/skills_tool.py:737).

[mehmetkr-31]

Both correct — the gate was checking a narrower notion of "installed" than the agent's own, which is the wrong direction for a check that blocks.

External roots

installed_skill_names() now enumerates agent.skill_utils.get_all_skills_dirs() — runtime's own helper, which returns the local skills dir followed by every configured skills.external_dirs — instead of only _skills_dir(). A dependency supplied from a team/shared root is loadable at runtime, so blocking on it was a false negative.

Declared name vs directory name

Also right. tools/skills_tool.py:743 resolves a skill as frontmatter.get("name", skill_dir.name), so a hand-placed skill in vendor-obsidian-v2/ declaring name: obsidian is usable as obsidian. Each SKILL.md now contributes both spellings, matching what tools/skills_sync.py already does ("Index by directory name … Also index by frontmatter name"). Unreadable or absent front-matter falls back to the directory name rather than dropping the skill.

Verification

reverted	result
external roots dropped (profile-only scan)	5 failed — "a skill provided by an external root was reported missing — its dependents would be blocked despite being loadable at runtime"
declared front-matter name not indexed	1 failed — "depends_on: [obsidian] would be blocked even though the agent can load that skill by its declared name"
nothing	25 passed

Both cases you asked for have their own tests, plus one for a root that doesn't exist (a stale external_dirs entry must not break discovery of the others) and one for a SKILL.md with no front-matter.

tests/tools -k skill: 14 failures on this branch, 14 on pristine origin/main at the same base — identical set.

Separately: the one red CI job is test_update_eol_churn.py::test_churn_across_more_files_than_fit_in_one_argv, the known #75175 cluster with a fix in flight as #75213. It touches no file this PR touches and reproduces on unmodified main.

[chancelu]

I built on top of this PR and implemented the remaining features requested in #71853:

• Recursive dependency graph resolution — _resolve_transitive() walks the full tree, detects cycles, and caps depth at 10
• --with-optional flag — treats optional deps as required (both CLI and /skills slash command)
• Auto-install support — do_install() can recursively install missing deps in topological order (deepest first), each going through quarantine + scan

32 tests all green, including new coverage for transitive resolution, cycle detection, max-depth capping, --with-optional, and auto-install behavior.

Would you prefer I open a separate PR that supersedes this one with the full implementation, or should I wait for this to land first and then follow up?

Branch: https://github.com/chancelu/hermes-agent/tree/pr75782

[mehmetkr-31]

@chancelu Thanks for picking this up — and for carrying my commit unsquashed rather than rewriting it. I pulled your branch and reviewed it properly. Supersede this PR, for a reason specific to this repo, but three things to fix first.

Answer to your question: supersede, don't sequence

I measured the merge dynamics here recently: of the last 120 merged PRs, 120 merged within 72 hours, median 30 minutes, and none took more than a week. There are ~17.9k open PRs, ~88% of them older than a week. So "land mine, then follow up" means your work waits on a PR that is already outside the window that actually merges — both age out. One complete PR answering all of #71853 is the better target, and the salvage-with-authorship-preserved shape you've used is exactly what landed for #75345.

1. auto_install is unreachable from the CLI (blocking)

This is the one worth fixing before you open it. auto_install defaults to False and is only ever set True in the recursive call at hermes_cli/skills_hub.py:957 — i.e. once auto-install is already running. Neither dispatch site passes it:

# :2020 — argparse path
do_install(args.identifier, category=args.category, force=args.force,
           with_optional=getattr(args, "with_optional", False),   # passed
           skip_confirm=getattr(args, "yes", False), ...)         # auto_install: not passed

# :2196 — /skills slash path — same

and there's no --auto-install in build_skills_parser. So for every real invocation it's False and the recursion never starts. --with-optional is wired on both paths; auto-install just needs the same treatment.

Worth a test that drives do_install and asserts a dependency actually gets installed — the unit tests pass with the flag unreachable, which is how this stays invisible. (The sweeper caught exactly this class on two of my other PRs: a handler nothing routes to.)

2. skip_confirm=True skips the disclaimer for skills the user never named

do_install(dep_name, ..., skip_confirm=True) bypasses the "You are installing a third-party skill at your own risk" panel. The scan still runs and still blocks, so the security floor holds — but a user typing hermes skill install chronicle can end up with N third-party skills without seeing that panel for any of them.

I deliberately left auto-install out of this PR for that reason ("it would mean running the security scan and confirmation flow recursively, which deserves its own review"). Not saying your choice is wrong — one prompt listing the whole resolved set up front would be reasonable — but it's a decision worth surfacing in the PR body so @teknium1 rules on it rather than finding it.

3. You'll get two front-matter parsers on rebase

Your branch is based on fb4ae34c; I've since pushed 531e0c3c, which adds parse_skill_frontmatter() to tools/skills_hub.py (it consolidated three copies of that parsing). Your read_skill_frontmatter() lands in the same file. Rebase onto my current head and keep one — yours takes str | bytes | Path, which is the more useful signature, so folding mine into it is fine by me.

What I checked and liked

• Transitive results are deduped (seen_req / seen_opt at :562-573), so a diamond doesn't report or install a skill twice — that was my first worry.
• Path-scoped _visited is the right choice for cycle detection, and the depth cap reports the path rather than failing silently.
• auto_install defaulting to False is the right default.
• All 32 tests green here, and my tests survive unmodified.

Once yours is open I'll close this one and point at it.

[mehmetkr-31]

Superseded by #76021, which carries this commit unsquashed and completes the rest of #71853 (transitive resolution, --with-optional, --auto-install).

I reviewed that PR and confirmed it fixes the two blocking issues I raised — --auto-install is now wired on both dispatch paths, and the duplicate front-matter parser is merged into one. Review notes are on #76021; the one item still open there is that auto-install has no test exercising it.

Closing here so there's a single target for the issue.