Skip to content

fix: use docker-visible skill paths for skills - #28569

Open
burjorjee wants to merge 7 commits into
NousResearch:mainfrom
burjorjee:fix/docker-visible-skill-paths-pr
Open

burjorjee wants to merge 7 commits into
NousResearch:mainfrom
burjorjee:fix/docker-visible-skill-paths-pr

Conversation

@burjorjee

Copy link
Copy Markdown
Contributor

Summary

  • translate bundled skill paths under $HERMES_HOME/skills to Docker-visible paths when exposing them to the agent
  • keep internal host-side skill loading on real host paths while showing /root/.hermes/skills/... in model-visible payloads
  • add regressions covering Docker skill directory rendering and skill_view() path behavior

Why

When Hermes runs inside Docker, model-visible skill metadata could include host-absolute skill paths. Those paths are invalid inside the container and leak host filesystem layout. This change makes model-visible skill paths match the Docker-mounted location while preserving host paths for internal operations.

How to test

  1. Run Hermes with the Docker terminal backend enabled.
  2. Load a bundled skill and confirm model-visible skill directory references use /root/.hermes/skills/... rather than host $HERMES_HOME paths.
  3. Confirm internal skill loading still works normally from the host checkout.
  4. Run the targeted regression suite below.

Validation

  • scripts/run_tests.sh tests/agent/test_skill_commands.py tests/tools/test_skills_tool.py -q

Platforms tested

  • macOS (with Docker-path translation logic validated by tests)

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists backend/docker Docker container execution tool/skills Skills system (list, view, manage) labels May 19, 2026
@burjorjee
burjorjee force-pushed the fix/docker-visible-skill-paths-pr branch from 98387ca to 0ea5d9b Compare May 19, 2026 18:58
@MKI13

MKI13 commented May 20, 2026

Copy link
Copy Markdown

Autofix follow-up for the blocked CI:

  • I cannot push directly to burjorjee/hermes-agent:fix/docker-visible-skill-paths-pr from MKI13; GitHub returns 403 Permission denied.
  • I created a contributor-fork PR with the fix here: fix: merge main to clear PR 28569 CI burjorjee/hermes-agent#2
  • That PR merges current NousResearch/hermes-agent:main into the branch to clear the stale BASE..HEAD Supply Chain Audit finding and also updates the web provider registry test for the new bundled xai provider.

Local verification:

  • supply scan for install-hook / .pth hits against merge-base: no findings
  • python -m pytest tests/plugins/web/test_web_search_provider_plugins.py tests/hermes_cli/test_update_hangup_protection.py::TestInstallHangupProtection::test_wraps_stdout_and_stderr_with_mirror -q -o 'addopts=' → 49 passed

@MKI13

MKI13 commented May 20, 2026

Copy link
Copy Markdown

Autofix follow-up for the latest failures on 1fe686a9ba2ff3d94114761ab06a6ec7661bb529:

  • Supply Chain Audit root cause is still the stale-branch BASE..HEAD supply scan: compared against current NousResearch/hermes-agent:main, the PR diff includes hermes_cli/setup.py, so the install-hook rule fails.
  • The Tests workflow also fails on stale test expectations: the web provider registry test expects seven providers while xai is registered, and the update hangup protection test needs the current main behavior.
  • I cannot push directly to burjorjee/hermes-agent:fix/docker-visible-skill-paths-pr; GitHub returns 403 Permission to burjorjee/hermes-agent.git denied to MKI13.
  • I updated the contributor-fork fix PR with the latest PR head and current main: fix: merge main to clear PR 28569 CI burjorjee/hermes-agent#2
  • Updated fix branch head: 3bb9521dc0f33f903c0dfd7ecb66e66e72b6a72a (MKI13/hermes-agent:autofix/pr-28569-merge-main).

Local verification:

  • Reproduced Supply Chain Audit scanner against origin/main..3bb9521d: no .pth, install-hook, base64+exec/eval, or obfuscated subprocess findings.
  • PYTHONPATH=$PWD python -m pytest tests/agent/test_skill_commands.py tests/plugins/web/test_web_search_provider_plugins.py tests/tools/test_skills_tool.py -q -o 'addopts=' → 178 passed.
  • PYTHONPATH=$PWD python -m pytest tests/plugins/web/test_web_search_provider_plugins.py::TestBundledPluginsRegister::test_all_eight_plugins_present_in_registry tests/hermes_cli/test_update_hangup_protection.py::TestInstallHangupProtection::test_wraps_stdout_and_stderr_with_mirror -q -o 'addopts=' → 2 passed.
  • python -m ruff check agent/skill_commands.py agent/skill_preprocessing.py tests/agent/test_skill_commands.py tests/plugins/web/test_web_search_provider_plugins.py tests/tools/test_skills_tool.py tools/credential_files.py tools/skills_tool.py → All checks passed.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for tackling this. The premise still holds on current main: skill prompts and skill_view() can expose host skill paths, while Docker mounts local skills at /root/.hermes/skills.

Problems

  • tools/credential_files.py:275 in this PR builds the returned Docker path with pathlib.Path. On native Windows hosts that can render backslashes, but the returned string is meant for a Linux container. Current main's cache-path translator uses posixpath.join and rel.as_posix() for the same reason at tools/credential_files.py:379-400.

Suggested changes

  • Build the translated skill path with a POSIX-only join, such as posixpath.join(container_base.rstrip('/'), 'skills', rel.as_posix()).
  • Preserve the internal agent_visible_paths=False path in agent/skill_commands.py so slash/bundle loaders keep host paths for supporting-file discovery.

Automated hermes-sweeper review.

Comment thread tools/credential_files.py Outdated
except Exception:
return host_path

return str(Path(container_base.rstrip("/")) / "skills" / rel)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use a POSIX-only join. pathlib.Path renders with host-platform separators, so native Windows can return \root\.hermes\skills\... here even though the string is for a Linux Docker container. posixpath.join(container_base.rstrip('/'), 'skills', rel.as_posix()) matches the existing cache-path mapper pattern.

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for addressing the canonical Docker skill path and for preserving host paths for internal slash-loader operations. The current main defect is real: agent/skill_commands.py:235-246 expands and emits host skill paths, while Docker mounts local skills under /root/.hermes/skills (tools/credential_files.py:203-230).

Problems

  • Configured external skills remain unaddressed. skill_view() searches get_external_skills_dirs() alongside the local root (tools/skills_tool.py:1082-1088) and returns their host skill_dir (tools/skills_tool.py:1554-1573). Docker mounts those roots at /root/.hermes/external_skills/<index> (tools/credential_files.py:232-241), but this PR intentionally leaves them untranslated.

Suggested changes

  • Map paths under each configured external root to its corresponding mounted external_skills/<index> destination, using POSIX paths.
  • Cover both skill_view() and slash-command rendering for an external Docker skill.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
@burjorjee

Copy link
Copy Markdown
Contributor Author

Follow-up pushed in cc5fb01b3 to address the external-skills Docker path gap flagged by the sweeper.

What changed:

  • Extended to_agent_visible_skill_path() so Docker mode now maps both:
    • local skills under $HERMES_HOME/skills/.../root/.hermes/skills/...
    • configured external skill dirs under skills.external_dirs/root/.hermes/external_skills/<index>/...
  • Kept the joins POSIX-only via posixpath.join(..., rel.as_posix()), so container-visible paths do not pick up host-native separators on Windows.
  • Preserved internal host-path behavior: agent_visible_paths=False still returns host paths for internal skill loading/support-file discovery.

Tests added:

  • direct external skill path mapping in tests/tools/test_credential_files.py
  • skill_view() rendering for an external Docker skill in tests/tools/test_skills_tool.py
  • slash-command rendering/supporting-file hints for an external Docker skill in tests/agent/test_skill_commands.py

Validation:

  • Local targeted suite: scripts/run_tests.sh tests/tools/test_credential_files.py tests/agent/test_skill_commands.py tests/tools/test_skills_tool.py -- -q170 passed
  • Ruff on touched files → All checks passed
  • GitHub CI → All required checks pass

@alt-glitch alt-glitch added the comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint label Jul 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend/docker Docker container execution comp/agent Core agent runtime: loop, agent_init, prompt builder, context-compression, responses endpoint P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data tool/skills Skills system (list, view, manage) type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants