fix(skill-config): expand ~ against subprocess HOME, not Python process HOME (#12260)#12284
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes skill-config ~ expansion to match Hermes’ per-subprocess HOME ({HERMES_HOME}/home) so paths surfaced into prompts align with where spawned tools actually read/write files (addresses #12260).
Changes:
- Add
_expanduser_for_subprocess()to expand~/~/...againstget_subprocess_home()when active, falling back toos.path.expanduserotherwise. - Update
resolve_skill_config_values()to use the new subprocess-aware expansion. - Add regression/unit tests covering subprocess-HOME vs process-HOME behavior, including
~userpassthrough.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
agent/skill_utils.py |
Introduces subprocess-aware ~ expansion and applies it to skill config resolution. |
tests/agent/test_skill_config_subprocess_home.py |
Adds regression tests for correct ~ expansion behavior under subprocess HOME activation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Explicit posix join — path[2:] has no leading slash. | ||
| return os.path.join(subprocess_home, path[2:]) |
| if path == "~": | ||
| return subprocess_home | ||
| if path.startswith("~/"): | ||
| # Explicit posix join — path[2:] has no leading slash. | ||
| return os.path.join(subprocess_home, path[2:]) | ||
| # ``~user`` or embedded ``~`` — defer to the OS user database. | ||
| return os.path.expanduser(path) |
| # Explicit posix join — path[2:] has no leading slash. | ||
| return os.path.join(subprocess_home, path[2:]) |
| import pytest | ||
|
|
||
| from agent.skill_utils import ( | ||
| SKILL_CONFIG_PREFIX, |
| subprocess_home = get_subprocess_home() | ||
| if subprocess_home is None: | ||
| return os.path.expanduser(path) | ||
| if path == "~": | ||
| return subprocess_home | ||
| if path.startswith("~/"): | ||
| # Explicit posix join — path[2:] has no leading slash. | ||
| return os.path.join(subprocess_home, path[2:]) |
|
Thanks for the thorough pass @copilot-pull-request-reviewer — all 5 addressed in 1. 2. Windows 3. "Explicit posix join" comment (line 403). Dropped the misleading comment; the new docstring accurately describes the separator-stripping + platform-native 4. Unused 5. Repeated 4 new edge-case tests added (17 total, all pass): |
|
Automated review check: BLOCKED. Required CI is failing: build-and-push and test both failed. The branch is not merge-ready until those jobs pass. |
|
@Artem151193 Same story as #12287 — the two red checks are pre-existing baselines, not caused by this PR.
Focused suite → 17 passed (including the 4 Copilot-flagged edge cases from the prior review). Happy to open a separate PR for the tractable baseline test failures if that would help unblock this one — let me know. |
|
Per-test CI baseline classification on the
All 6 reproduce on clean
Focused suite: Other checks green: |
|
Thanks @alt-glitch for the triage. 👍 Looked at #12736 (@Tranquil-Flow) — their A few edge-case tests from mine that might be worth porting if #12736 is selected:
Feel free to cherry-pick / ignore. Whichever approach lands, the user-facing fix matters more than attribution. |
…ss HOME (NousResearch#12260) ``agent.skill_utils.resolve_skill_config_values`` expanded ``~`` in skill config defaults using ``os.path.expanduser``, which resolves against the Python process's own ``HOME`` (e.g. ``/opt/data`` or ``/root`` in Docker). Hermes independently sets a *different* ``HOME`` — ``{HERMES_HOME}/home/`` via ``hermes_constants.get_subprocess_home`` — for every subprocess it spawns (Bash, background processes, terminal launchers). The result: a skill declaring ``default: "~/wiki"`` surfaced ``/opt/data/wiki`` in the prompt, but subprocess tools spawned by the agent (``wiki init``, ``bash -c 'ls ~/wiki'``, …) resolved ``~`` to ``/opt/data/home/wiki``. Files written by one tool weren't findable by the next. Reporter: NousResearch#12260. Root cause ---------- ``agent/skill_utils.py:407-408`` (pre-fix):: if isinstance(value, str) and ("~" in value or "${" in value): value = os.path.expanduser(os.path.expandvars(value)) ``os.path.expanduser`` uses ``os.environ['HOME']`` (the Python process HOME) — which ``hermes_constants.get_subprocess_home``'s docstring explicitly calls out as "**never** modified — only subprocess environments should inject this value." Fix --- Add a small ``_expanduser_for_subprocess(path)`` helper that: * Uses ``get_subprocess_home()`` when active (``{HERMES_HOME}/home`` exists on disk). ``~`` → subprocess HOME; ``~/…`` → ``{subprocess_home}/…``. * Falls through to ``os.path.expanduser`` when the subprocess HOME isn't active — preserves the original behaviour for setups that never activated per-profile HOMEs. * Leaves ``~user`` forms to ``os.path.expanduser`` — those name a specific OS user via the passwd database and are independent of Hermes. ``resolve_skill_config_values`` uses the new helper instead of ``os.path.expanduser`` directly. No other callers of ``os.path.expanduser`` are touched — e.g. ``get_external_skills_dirs`` at ``skill_utils.py:212`` reads directories for the **Python process** to load skills from, so expanding against the process HOME is correct there. Narrow scope — explicitly not changed ------------------------------------- * **Other ``expanduser`` call sites.** Only the skill config surface (``resolve_skill_config_values``) feeds into subprocess-facing prompts. The rest of the codebase (external skills dirs, auth file paths, etc.) correctly targets the Python process. * **Subprocess HOME activation logic.** Unchanged — still controlled by the presence of ``{HERMES_HOME}/home/`` on disk. * **``${VAR}`` env expansion.** Still runs via ``os.path.expandvars`` before ``~`` expansion; unchanged semantics. Regression coverage ------------------- ``tests/agent/test_skill_config_subprocess_home.py`` is new, 13 cases across two classes: * ``TestExpanduserForSubprocess`` — 6 unit tests for the helper: ``~/x``, ``~``, ``~/a/b/c``, absolute paths, ``~user`` fall-through, no-subprocess-home fallback. * ``TestResolveSkillConfigValuesHOME`` — 7 end-to-end tests through the public API: reporter scenario, no-subprocess-home fallback, explicit config value expansion, absolute default unchanged, ``${VAR}`` interaction, non-string value pass-through, empty-default canary. Bug also verified to reproduce on clean ``origin/main`` (``6fb69229``) via a direct Python repro showing ``{'wiki.path': '/home/testuser/wiki'}`` when the expected value is ``{'wiki.path': '{HERMES_HOME}/home/wiki'}``. Validation ---------- ``source venv/bin/activate && python -m pytest tests/agent/test_skill_config_subprocess_home.py -q`` → **13 passed**. Broader agent + skills suite (``tests/agent/`` + ``tests/hermes_cli/ test_skills_config.py`` + ``test_chat_skills_flag.py`` + ``test_skills_hub.py`` + ``test_banner_skills.py`` + ``tests/cli/test_cli_preloaded_skills.py`` + ``tests/tools/ test_skill_manager_tool.py`` + ``test_skills_tool.py``) under ``-n auto`` → **1566 passed, 1 skipped, 0 failures.** Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…ew (follow-up to NousResearch#12260) Addresses all 5 Copilot review comments on NousResearch#12284: 1. **``~//foo`` edge case** (inline #1) — ``os.path.join(home, "/foo")`` used to return ``/foo`` (absolute), bypassing the subprocess HOME. ``_expanduser_for_subprocess`` now strips *all* leading path separators from the post-``~`` remainder before joining, mirroring :func:`os.path.expanduser`'s own behaviour for ``~//foo``. 2. **Windows ``~\\wiki``** (inline NousResearch#2) — the previous ``~/`` prefix check only matched forward slashes, so Windows-style paths fell through to ``os.path.expanduser`` and expanded against the Python process HOME. Added a ``_TILDE_PREFIXES`` module constant that matches both ``~/`` and ``~\\`` and runs the same subprocess-HOME expansion. 3. **Comment wording** (inline NousResearch#3) — removed the misleading "Explicit posix join" comment; the docstring now accurately describes that leading separators get stripped and the join uses the platform-native ``os.path.join``. 4. **Unused import** (inline NousResearch#4) — dropped the stray ``SKILL_CONFIG_PREFIX`` import from the test module. 5. **Repeated stat calls** (inline NousResearch#5) — ``resolve_skill_config_values`` now resolves ``get_subprocess_home()`` **once** at the top of the function and threads the result through the helper via a new ``subprocess_home=`` kwarg. Multiple config vars no longer trigger repeated ``os.path.isdir`` calls. New tests pinning the hardened behaviour: * ``test_double_slash_after_tilde_stays_under_subprocess_home`` (``~//foo/bar`` → ``/opt/data/home/foo/bar``) * ``test_tilde_backslash_on_windows_style_paths`` (``~\\wiki`` expands under subprocess HOME) * ``test_mixed_separators_after_tilde`` (``~\\\\foo`` stays under subprocess HOME) * ``test_subprocess_home_argument_short_circuits_lookup`` (passing ``subprocess_home=`` skips ``get_subprocess_home`` — mocked with ``AssertionError`` side effect to prove it isn't called) Validation ---------- ``source venv/bin/activate && python -m pytest tests/agent/test_skill_config_subprocess_home.py -q`` → **17 passed** (13 original + 4 new edge-case). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
e627b3f to
cded49d
Compare
|
Closing in favor of @Tranquil-Flow's #12736, which uses a cleaner centralized |
Fixes #12260.
TL;DR
agent.skill_utils.resolve_skill_config_valuesexpanded~in skill config defaults usingos.path.expanduser— which resolves against the Python process'sHOME. Hermes independently sets a differentHOME({HERMES_HOME}/home/viaget_subprocess_home) for every subprocess it spawns.A skill declaring
default: "~/wiki"therefore surfaced/opt/data/wikiin the prompt, but subprocess tools (Bash,wiki init, …) resolved~to/opt/data/home/wiki. Files written by one tool weren't findable by the next.Fix: expand
~against the subprocess HOME when active, fall through toos.path.expanduserotherwise.Root cause
agent/skill_utils.py:407-408(pre-fix):hermes_constants.get_subprocess_homereturns{HERMES_HOME}/homewhen that directory exists and explicitly states in its docstring:So the skill-config surface, which is consumed by subprocess-facing tools, was silently pointing at the wrong HOME.
Fix
New helper
_expanduser_for_subprocess(path):resolve_skill_config_valuescalls it instead ofos.path.expanduserdirectly. Nothing else changes.Behaviour matrix
get_subprocess_home()~/wiki/opt/data/home(active)/opt/data/wiki(wrong)/opt/data/home/wiki~/opt/data/home(active)/opt/data(wrong)/opt/data/home~/a/b/c/opt/data/home(active)/opt/data/a/b/c(wrong)/opt/data/home/a/b/c/abs/path/opt/data/home(active)/abs/path~root/conf/opt/data/home(active)/root/conf(passwd lookup)/root/conf(unchanged — not hijacked)~/wikiNone(no subprocess home)/home/alice/wiki${VAR}/binwhere VAR=~/custom/opt/data/home(active)/root/custom/bin/opt/data/home/custom/binNarrow scope — explicitly not changed
os.path.expandusercall sites. Only the skill-config surface feeds subprocess-facing prompts.get_external_skills_dirsatskill_utils.py:212reads directories for the Python process to load skills from — expanding against the process HOME is correct there, and untouched.{HERMES_HOME}/home/existing on disk.${VAR}env expansion. Still runs viaos.path.expandvarsbefore~expansion; unchanged semantics.~userforms (~alice/foo,~root/conf). Still defer toos.path.expanduser's passwd-database lookup — those name a specific OS user independent of Hermes, so the subprocess HOME should not hijack them. Pinned by a dedicated test.Regression coverage
tests/agent/test_skill_config_subprocess_home.py(new, 13 cases):TestExpanduserForSubprocess(6 unit tests) —~/x, bare~, nested~/a/b/c, absolute paths,~userpasswd fall-through, and the no-subprocess-homeos.path.expanduserfallback.TestResolveSkillConfigValuesHOME(7 end-to-end tests) through the public API — reporter scenario, no-subprocess-home fallback, explicit config value expansion, absolute default unchanged,${VAR}interaction, non-string value pass-through, and an empty-default canary.Bug also verified to reproduce on clean
origin/main(6fb69229) via direct Python repro:Validation
Broader agent + skills suites under
-n auto:python -m pytest tests/agent/ tests/hermes_cli/test_skills_config.py \ tests/hermes_cli/test_chat_skills_flag.py tests/hermes_cli/test_skills_hub.py \ tests/hermes_cli/test_banner_skills.py tests/cli/test_cli_preloaded_skills.py \ tests/tools/test_skill_manager_tool.py tests/tools/test_skills_tool.py -q -n auto # 1566 passed, 1 skipped, 0 failuresPre-empted review questions
Q. Should this behaviour apply everywhere the agent expands
~, not just skill config?Out of scope for this PR.
resolve_skill_config_valuesis the specific surface the reporter identified and it has a clean subprocess-facing contract. A broader audit ofos.path.expanduserusage (auth file paths, external skills dirs, etc.) is a separate, larger change with different trade-offs per call site.Q. What if a skill author genuinely wants the Python process HOME (e.g. for a Python-only tool)?
They can either pass an absolute path, or use an explicit
${HOME}that will beos.path.expandvars'd (expanding against the Python process env). Most skill defaults want "where the subprocess tools work" — this PR makes that the default.Q. Concurrency?
get_subprocess_home()is a read-only lookup of an env var + aos.path.isdircheck. No mutation, no locks. Called per skill-config resolution, not per message.Q. What about Windows?
os.path.joinhandles both separator flavours;os.path.expanduseron Windows usesUSERPROFILE. Both fall-through branches are preserved. No Windows-specific change.Co-authored via LLM assistance; I've reviewed every line and am responsible for correctness.