Skip to content

fix(skills): scan slash-command skills from the live profile, not import time - #326

Merged
OmarB97 merged 1 commit into
mainfrom
fix/skill-commands-live-skills-dir-fork-20260802
Aug 2, 2026
Merged

fix(skills): scan slash-command skills from the live profile, not import time#326
OmarB97 merged 1 commit into
mainfrom
fix/skill-commands-live-skills-dir-fork-20260802

Conversation

@OmarB97

@OmarB97 OmarB97 commented Aug 2, 2026

Copy link
Copy Markdown
Owner

What does this PR do?

agent/skill_commands.py resolved the skills directory through
tools.skills_tool.SKILLS_DIR — a module-level constant evaluated at import
(SKILLS_DIR = HERMES_HOME / "skills", skills_tool.py:143). In a long-lived
backend serving more than one profile that constant is whatever home the process
started under, so it goes stale the moment a request is scoped elsewhere.

skills_tool already solved this for its own scan loop. _skills_dir() resolves
the live profile HERMES_HOME on every call while still honoring an explicit
SKILLS_DIR monkeypatch, and skills_list was migrated to it with the comment
"the module-level SKILLS_DIR can be stale in long-lived runtimes"
(skills_tool.py:693-696). skill_commands was simply left behind.

This is an unfinished migration, not a deliberate launch-home scope. Where this
codebase genuinely wants launch-home scoping it says so explicitly — a
get_process_hermes_home() call plus a docstring explaining why (dashboard themes,
dashboard plugin manifests). Neither is present here.

The visible effect. scan_skill_commands() is the /<skill-name> vocabulary
the gateway's command.dispatch resolves against. A session under a non-launch
profile saw its own skills mid-turn — the agent resolves via _skills_dir()
but the launch profile's /<skill-name> list. So a skill installed in that
profile was invisible as a slash command, and a skill name present in both homes
dispatched the wrong copy. Same session, two different answers to "which skills
exist", depending on which code path asked.

Related Issue

No filed issue — found while auditing command.dispatch for profile scoping, the
same audit that produced #323 and #324.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

All three SKILLS_DIR sites in agent/skill_commands.py move to _skills_dir():

  • scan_skill_commands (the scan root) — now active_skills_dir = _skills_dir(),
    mirroring the loop in tools/skills_tool.py. This is the one that made profile
    skills invisible as slash commands.
  • _load_skill_payload — the fallback reconstruction of skill_dir from a
    relative skill_path, which otherwise built a path into the wrong home.
  • _build_skill_messageskill_dir.relative_to(...) for the skill_view
    hint. Against a profile skill this raised ValueError and silently fell into the
    "external dir" branch, producing a wrong skill_view target.
  • Added a comment at the scan root recording why the live resolver is required, and
    corrected a now-inaccurate comment that still described the old reconstruction.
  • tests/tools/test_skills_tool_profile_scope.py — 3 new tests (see below). Added
    to the existing profile-scope regression file so the shared _write_skill /
    _reload_skills_tool helpers and the concern stay in one place.

The SKILLS_DIR monkeypatch escape hatch is preserved — 33 test files depend on it,
and _skills_dir() checks the patched value before resolving live. A new test pins
that contract for this code path, alongside the existing
test_explicit_skills_dir_monkeypatch_still_wins.

How to Test

  1. Confirm the new tests fail without the fix. Revert only
    agent/skill_commands.py and run
    pytest tests/tools/test_skills_tool_profile_scope.py -k scan_skill_commands.
    Two of the three fail, resolving to the import-time home:
    E  AssertionError: assert PosixPath('.../default-home/skills/software-development/shared-skill')
                           == PosixPath('.../profiles/orchestrator/skills/software-development/shared-skill')
    
    The third (..._honors_an_explicit_skills_dir_monkeypatch) passes either way by
    design — it guards the escape hatch rather than reproducing the bug.
  2. Confirm they pass with it.
    pytest tests/tools/test_skills_tool_profile_scope.py -q → 6 passed.
  3. Check for regressions across the skill-touching suites — every file that
    patches SKILLS_DIR or exercises profile isolation:
    pytest tests/agent/test_skill_commands.py tests/tools/test_skills_tool.py \
      tests/agent/test_skill_bundles.py tests/tools/test_skills_tool_profile_scope.py \
      tests/tools/test_cross_profile_guard.py tests/test_profile_isolation_runtime.py \
      tests/test_plugin_skills.py tests/tools/test_skill_manager_tool.py \
      tests/tools/test_skills_hub.py tests/hermes_cli/test_skills_config.py \
      tests/tools/test_skill_view_traversal.py \
      tests/hermes_cli/test_web_server_skills_profiles.py -q
    
    8 failed, 561 passed. The same 8 fail on an unpatched tree (7 in
    test_cross_profile_guard.py, 1 in test_skills_config.py) — I diffed the two
    failure sets and they are identical, so none are attributable to this change.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass — not claimed. The full suite is not green on this machine independent of this change. I ran the 12 skill-touching suites listed above and diffed the failure sets before and after: identical, +3 passing (the new tests).
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15 (Darwin 25.6.0), arm64

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A (the reasoning lives in a code comment beside the change; a stale comment was corrected)
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A (N/A — no config keys)
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A (N/A — completes an existing migration)
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A (_skills_dir() returns a pathlib.Path exactly as SKILLS_DIR did; no new path handling)
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A (N/A — no tool surface change)

Screenshots / Logs

Probe showing why the constant cannot follow a scoped request, and that the live
resolver can — the process imports under one home, then binds an override to another:

get_hermes_home()                        -> PROFILE ✅ follows
skills_tool.SKILLS_DIR (frozen const)    -> LAUNCH  ❌ pinned
skills_tool._skills_dir() (call-time)    -> PROFILE ✅ follows

scan_skill_commands() keys  (before)     -> ['/only-launch']    ❌
scan_skill_commands() keys  (after)      -> ['/only-profile']   ✅

…ort time

`agent/skill_commands.py` resolved the skills directory through
`tools.skills_tool.SKILLS_DIR`, a module-level constant evaluated at import
(`SKILLS_DIR = HERMES_HOME / "skills"`). In a long-lived backend serving more
than one profile that constant is whatever home the process started under, so
it goes stale the moment a request is scoped elsewhere.

`skills_tool` already solved this for its own scan loop: `_skills_dir()`
resolves the live profile `HERMES_HOME` on every call while still honoring an
explicit `SKILLS_DIR` monkeypatch, and `skills_tool.skills_list` was migrated
to it with the comment "the module-level SKILLS_DIR can be stale in long-lived
runtimes". `skill_commands` was left behind — an unfinished migration, not a
deliberate launch-home scope. There is no `get_process_hermes_home()` here and
no docstring claiming process scope, which is how the codebase marks the
genuinely launch-scoped assets (dashboard themes, dashboard plugin manifests).

The visible effect: `scan_skill_commands()` is the `/<skill-name>` vocabulary
the gateway's `command.dispatch` resolves against, so a session under a
non-launch profile saw its OWN skills mid-turn (the agent resolves via
`_skills_dir()`) but the LAUNCH profile's `/<skill-name>` list. A skill
installed in that profile was invisible as a slash command, and a name present
in both homes dispatched the wrong copy.

Move all three sites to `_skills_dir()`:

- `scan_skill_commands` — the scan root, mirroring `skills_tool`'s loop.
- `_load_skill_payload` — the fallback reconstruction of `skill_dir` from a
  relative path, which otherwise built a path into the wrong home.
- `_build_skill_message` — `skill_dir.relative_to(...)` for the `skill_view`
  hint, which otherwise raised `ValueError` against a profile skill and silently
  degraded to the external-dir branch.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@OmarB97
OmarB97 merged commit 1ea0741 into main Aug 2, 2026
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant