fix: sync_skills skips bundled skills already in external_dirs

[zccyman]

Problem

When a profile configures skills.external_dirs to delegate skill resolution to another profile (e.g. the default one), sync_skills() still copies bundled skills into the profile-local skills/ directory. These shadow copies then collide with the external sources during skill loading, causing crashes like the kanban worker failure described in #28126.

Closes #28126

Root Cause

sync_skills() in tools/skills_sync.py uses module-level SKILLS_DIR (derived from HERMES_HOME) without consulting the skills.external_dirs config. seed_profile_skills() in hermes_cli/profiles.py sets HERMES_HOME to the profile directory and calls sync_skills(), but the function never checks whether the profile has external skill dirs that already provide those bundled skills.

Fix

1. _get_external_skill_names() — reads skills.external_dirs from the current profile's config.yaml and returns the set of skill names available via those external directories.
2. sync_skills() — skips any bundled skill whose name is already present in an external_dir, preventing the collision.
3. Return dict gains a skipped_external counter for observability.

Backward-compatible: profiles without external_dirs see no behavior change (empty set → skipped_external stays 0).

Testing

• 50 existing tests continue to pass
• 5 new tests in TestExternalDirsSkip class:
  • test_no_external_dirs_copies_all — baseline, all skills synced
  • test_external_dirs_skips_matching_skills — partial skip
  • test_all_skills_in_external_dirs_skips_everything — full skip
  • test_skipped_external_not_in_manifest — no manifest poisoning
  • test_return_dict_includes_skipped_external_key — backward compat

All 222 tests across test_skills_sync.py, test_skill_manager_tool.py, and test_skills_tool.py pass.

Files Changed

• tools/skills_sync.py — +88/-2 (new _get_external_skill_names(), _find_config_yaml(), skip logic)
• tests/tools/test_skills_sync.py — +107/-1 (5 new test methods)

[outsourc-e]

Validated locally on clean upstream/main worktree. skills sync/tool test suite passed (222 passed). Scope is focused and fixes the external_dirs collision cleanly.

[magnus919]

Code Review Summary

Verdict: Comment — clean fix with thorough test coverage. One cosmetic inconsistency noticed.

What it does

When a profile delegates skill resolution via skills.external_dirs, sync_skills() would previously copy bundled skills into the profile-local skills/ directory anyway. Those shadow copies then collided with the external sources, causing the kanban worker crash in #28126.

This PR fixes it by:

1. Adding _get_external_skill_names() — scans configured external_dirs for existing SKILL.md names
2. sync_skills() now skips any bundled skill whose name is already in an external dir
3. Return dict gains skipped_external counter for observability

Docker Validation

Isolated Docker container validates the core logic across 5 scenarios:

• External skill names correctly identified from SKILL.md frontmatter
• Matching bundled skills are skipped (2 skipped, 1 unique copied)
• No external_dirs = all skills synced (baseline backward compat)
• Self-referencing external_dir correctly ignored (avoids infinite skip)
• Missing frontmatter falls back to directory name safely

Minor: filter inconsistency

_get_external_skill_names() filters /.git/ and /.hub/ from rglob results, but the existing _discover_bundled_skills() at line 219 also filters /.github/. Consider adding /.github/ to the new filter for consistency, though this is unlikely to hit in practice.

Summary

Well-structured fix. The _get_external_skill_names → _find_config_yaml → skip chain is clean, the edge cases are handled (missing config, empty dirs, self-referencing dirs), and the 5 new tests provide good coverage. Backward compatible by design.

Docker validation confirms the logic works correctly in an isolated environment.

[zccyman]

Thanks @magnus919 — good catch on the /.github/ filter inconsistency. Pushed a fix in 4853ff2 that adds it to _get_external_skill_names() to match the existing _discover_bundled_skills() filter. Pure cosmetic, 50/50 tests still passing.

[teknium1]

Thanks for the focused fix. I verified the premise still holds on current main: sync_skills() still copies bundled skills into the profile-local SKILLS_DIR when the destination is missing, without consulting skills.external_dirs first (tools/skills_sync.py:484-553), and seed_profile_skills() runs that sync with HERMES_HOME set to the profile directory (hermes_cli/profiles.py:996-1001).

Problems

• The new tests mock _get_external_skill_names() (tests/tools/test_skills_sync.py:763), so they exercise the skip branch but not the real failure path: config.yaml -> skills.external_dirs resolution -> external SKILL.md scan -> sync skip.
• The PR adds a hand-rolled hidden-dir filter in tools/skills_sync.py:94, but current main centralizes this as is_excluded_skill_path() with a broader exclusion set in agent/skill_utils.py:27-62.

Suggested changes

• Reuse agent.skill_utils.get_external_skills_dirs() / is_excluded_skill_path() instead of duplicating config and scan semantics.
• Add one E2E-style regression test with a real profile config.yaml and real external skill directory, so the profile delegation case from #28126 is covered.

This is an automated hermes-sweeper review.

[teknium1]

This mocks the new helper, so the regression tests never exercise the actual bug path through config.yaml, skills.external_dirs resolution, and external SKILL.md scanning. Please add at least one test that writes a real profile config and lets sync_skills() discover the external skill itself.

[teknium1]

Current main has a shared is_excluded_skill_path() helper with a broader exclusion set; using it here avoids this scan drifting from the rest of the skills system as new ignored directories are added.

[teknium1]

Fixed via #53927 (merged to main), which salvages your refined fix from #33616 — same approach, reusing get_external_skills_dirs — with your authorship preserved in git history (commit db11849).

This earlier PR skipped writing shadowing skills but didn't clean up shadows a prior sync had already written; your #33616 added that self-heal, which is what makes the fix durable across hermes update. Closing in favor of the merged version. Thanks for both!

#53927