Skip to content

fix(skills): allow absolute skill paths that resolve within trusted directories (#59824) - #60195

Closed
webtecnica wants to merge 1 commit into
NousResearch:mainfrom
webtecnica:fix/59824-cron-skill-paths
Closed

webtecnica wants to merge 1 commit into
NousResearch:mainfrom
webtecnica:fix/59824-cron-skill-paths

Conversation

@webtecnica

Copy link
Copy Markdown
Contributor

Summary

P1 — Silent failure in production. Cron jobs with absolute skill paths (e.g. symlinked trading-skills repos at /home/hermes/agent-trading-skills/...) were silently rejected by _skill_lookup_path_error(), which blocked ALL absolute paths.

The cron appeared healthy (Last run: ok) but delivered degraded/empty responses because every tick's skill loads failed. The user saw "audit channel went silent."

Fix

Changed _skill_lookup_path_error() from a hard rejection of ALL absolute paths to a trusted-root validation:

  • Absolute paths resolving within HERMES_HOME/skills/ or HERMES_HOME/profiles/*/ are allowed
  • All other absolute paths (/etc/passwd, /tmp/...) are still rejected
  • .. traversal and Windows drive paths still blocked
  • Symlinks are resolved before checking the prefix

Files Changed

File Δ
tools/skills_tool.py +14/-1 lines

Verification

  • 11 behavioral assertions for edge cases (relative, traversal, trusted absolute, untrusted absolute, Windows drives)
  • 89 existing tests: ✅ all passed

Closes #59824

…irectories

_skill_lookup_path_error() rejected ALL absolute paths, but cron jobs
legitimately store absolute paths for skills outside ~/.hermes/skills/
(e.g. symlinked skill repos). Now allows absolute paths that resolve
within HERMES_HOME/skills/ or profiles/*/skills/.

Closes NousResearch#59824
@webtecnica

Copy link
Copy Markdown
Contributor Author

@teknium1 P1 fix — cron jobs with absolute skill paths were silently failing (no Discord delivery). Now allows absolute paths that resolve within Hermes trusted directories. 14 lines, 89 existing tests pass. 🙏

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/tools Tool registry, model_tools, toolsets tool/skills Skills system (list, view, manage) area/config Config system, migrations, profiles labels Jul 7, 2026
@webtecnica

Copy link
Copy Markdown
Contributor Author

@teknium1 Consolidated ping on this one — all 50 PRs from webtecnica are rebased on latest main, zero conflicts. This P2 fix (12 lines, vision fallback for text-only models) is among them. 🙏

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for the focused report and security-preserving intent. This is an automated hermes-sweeper review; the cron-path behavior is already implemented on current main.

  • 62972060caaa9f7f3fc5a58688757fdd59b9a117 (fix(cron): normalize absolute skill paths before skill_view (#59824)) landed the fix for this issue.
  • cron/scheduler.py:2392-2393 normalizes each cron skill identifier before calling skill_view.
  • agent/skill_utils.py:543-568 accepts the active skill root and configured skills.external_dirs, preserves lexical symlink paths, and leaves untrusted absolute paths for skill_view to reject.
  • tests/cron/test_scheduler.py:2968-2987 covers the absolute-path cron regression.

Closing as implemented on main.

@teknium1 teknium1 closed this Jul 15, 2026
@teknium1 teknium1 added sweeper:implemented-on-main Sweeper: behavior already present on current main sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:implemented-on-main Sweeper: behavior already present on current main sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades 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.

[Bug] Cron scheduler silently fails (no Discord delivery) — absolute skill paths rejected by skill_view path-security check

3 participants