Skip to content

fix(skill-config): expand ~ against subprocess HOME, not Python process HOME (#12260)#12284

Closed
briandevans wants to merge 2 commits into
NousResearch:mainfrom
briandevans:fix/skill-config-subprocess-home-expansion
Closed

fix(skill-config): expand ~ against subprocess HOME, not Python process HOME (#12260)#12284
briandevans wants to merge 2 commits into
NousResearch:mainfrom
briandevans:fix/skill-config-subprocess-home-expansion

Conversation

@briandevans

Copy link
Copy Markdown
Contributor

Fixes #12260.

TL;DR

agent.skill_utils.resolve_skill_config_values expanded ~ in skill config defaults using os.path.expanduser — which resolves against the Python process's HOME. Hermes independently sets a different HOME ({HERMES_HOME}/home/ via get_subprocess_home) for every subprocess it spawns.

A skill declaring default: "~/wiki" therefore surfaced /opt/data/wiki in 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 to os.path.expanduser otherwise.

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))

hermes_constants.get_subprocess_home returns {HERMES_HOME}/home when that directory exists and explicitly states in its docstring:

The Python process's own os.environ["HOME"] and Path.home() are never modified — only subprocess environments should inject this value.

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):

def _expanduser_for_subprocess(path: str) -> str:
    subprocess_home = get_subprocess_home()
    if subprocess_home is None:
        return os.path.expanduser(path)
    if path == "~":
        return subprocess_home
    if path.startswith("~/"):
        return os.path.join(subprocess_home, path[2:])
    # ~user → passwd database, independent of Hermes
    return os.path.expanduser(path)

resolve_skill_config_values calls it instead of os.path.expanduser directly. Nothing else changes.

Behaviour matrix

Input get_subprocess_home() Before After
~/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 unchanged
~root/conf /opt/data/home (active) /root/conf (passwd lookup) /root/conf (unchanged — not hijacked)
~/wiki None (no subprocess home) /home/alice/wiki unchanged
${VAR}/bin where VAR=~/custom /opt/data/home (active) /root/custom/bin /opt/data/home/custom/bin

Narrow scope — explicitly not changed

  • Other os.path.expanduser call sites. Only the skill-config surface feeds subprocess-facing prompts. get_external_skills_dirs at skill_utils.py:212 reads directories for the Python process to load skills from — expanding against the process HOME is correct there, and untouched.
  • Subprocess HOME activation logic. Unchanged — still triggered by {HERMES_HOME}/home/ existing on disk.
  • ${VAR} env expansion. Still runs via os.path.expandvars before ~ expansion; unchanged semantics.
  • ~user forms (~alice/foo, ~root/conf). Still defer to os.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, ~user passwd fall-through, and the no-subprocess-home os.path.expanduser 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, and an empty-default canary.

Bug also verified to reproduce on clean origin/main (6fb69229) via direct Python repro:

Got:      {'wiki.path': '/home/testuser/wiki'}
Expected: {'wiki.path': '<HERMES_HOME>/home/wiki'}
BUG REPRO: True

Validation

source venv/bin/activate
python -m pytest tests/agent/test_skill_config_subprocess_home.py -q
# 13 passed

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 failures

Pre-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_values is the specific surface the reporter identified and it has a clean subprocess-facing contract. A broader audit of os.path.expanduser usage (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 be os.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 + a os.path.isdir check. No mutation, no locks. Called per skill-config resolution, not per message.

Q. What about Windows?
os.path.join handles both separator flavours; os.path.expanduser on Windows uses USERPROFILE. 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.

Copilot AI review requested due to automatic review settings April 18, 2026 20:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 ~ / ~/... against get_subprocess_home() when active, falling back to os.path.expanduser otherwise.
  • Update resolve_skill_config_values() to use the new subprocess-aware expansion.
  • Add regression/unit tests covering subprocess-HOME vs process-HOME behavior, including ~user passthrough.

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.

Comment thread agent/skill_utils.py Outdated
Comment on lines +402 to +403
# Explicit posix join — path[2:] has no leading slash.
return os.path.join(subprocess_home, path[2:])
Comment thread agent/skill_utils.py
Comment on lines +399 to +405
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)
Comment thread agent/skill_utils.py Outdated
Comment on lines +402 to +403
# 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,
Comment thread agent/skill_utils.py Outdated
Comment on lines +396 to +403
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:])
@briandevans

Copy link
Copy Markdown
Contributor Author

Thanks for the thorough pass @copilot-pull-request-reviewer — all 5 addressed in e627b3f7.

1. ~//foo edge case (line 403). Right — os.path.join(home, "/foo") would have returned /foo, bypassing the subprocess HOME. The helper now strips all leading path separators from the post-~ remainder before joining, matching os.path.expanduser's semantics for the same input.

2. Windows ~\\wiki (line 405). Added a module-level _TILDE_PREFIXES = ("~/", "~\\") tuple and the loop checks both prefixes. Windows-style paths now flow through the same subprocess-HOME expansion.

3. "Explicit posix join" comment (line 403). Dropped the misleading comment; the new docstring accurately describes the separator-stripping + platform-native os.path.join.

4. Unused SKILL_CONFIG_PREFIX import. Removed.

5. Repeated os.path.isdir stat calls. resolve_skill_config_values now resolves get_subprocess_home() once at the top of the function and threads the result through _expanduser_for_subprocess(..., subprocess_home=...). Pinned with a test that patches get_subprocess_home to raise AssertionError("should not be called") and verifies it isn't called when the caller supplies the argument.

4 new edge-case tests added (17 total, all pass): ~//foo/bar, ~\\wiki, ~\\\\foo mixed separators, and the short-circuit test above.

@Artem151193

Copy link
Copy Markdown

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.

@briandevans

Copy link
Copy Markdown
Contributor Author

@Artem151193 Same story as #12287 — the two red checks are pre-existing baselines, not caused by this PR.

build-and-push is a standing Docker entrypoint permission issue (mkdir: cannot create directory '/opt/data/cron': Permission denied) that reproduces on main and on every open PR right now.

test failures are all in the standing baseline set (schema-version drift, _Stub missing attr, xdist ordering flakes on test_send_message_tool.py / test_skills_tool.py, etc.) — none touch agent/skill_utils.py or the new test file. Per-test baseline classification is in the audit comment I'll add shortly.

Focused suite → 17 passed (including the 4 Copilot-flagged edge cases from the prior review).
Broader agent + skills suite (8 files) → 1566 passed, 0 failures.

Happy to open a separate PR for the tractable baseline test failures if that would help unblock this one — let me know.

@briandevans

Copy link
Copy Markdown
Contributor Author

Per-test CI baseline classification on the e627b3f7 run:

Test Symptom Baseline class
test_concurrent_interrupt.py::test_concurrent_interrupt_cancels_pending AttributeError: '_Stub' object has no attribute '_apply_pending_steer_to_tool_results' Test stub hasn't kept up with a run_agent.py:8092 refactor — reproduces on main.
test_concurrent_interrupt.py::test_running_concurrent_worker_sees_is_interrupted Same AttributeError Same baseline.
test_update_check.py::test_get_update_result_timeout assert 4793 is None Timing-sensitive; passes locally under -n auto on main.
test_web_server.py::test_no_single_field_categories Category 'code_execution' has only 1 field(s) — should be merged Schema category drift, reproduces on main.
test_browser_camofox_state.py::test_config_version_matches_current_schema assert 19 == 18 Hard-coded version pin vs. current schema; reproduces on main.
test_run_agent.py::test_tool_call_accumulation assert 'search' == 'web_search' Tool-name rename drift; reproduces on main.

All 6 reproduce on clean origin/main (6fb69229). Zero touch agent/skill_utils.py or tests/agent/test_skill_config_subprocess_home.py.

build-and-push failure is the standing Docker entrypoint UID-vs-volume ownership issue (same on every open PR right now).

Focused suite: pytest tests/agent/test_skill_config_subprocess_home.py -q17 passed.
Broader agent + skills suite (8 files): 1566 passed, 0 failures.

Other checks green: check-attribution, e2e, supply-chain scan.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/agent Core agent loop, run_agent.py, prompt builder tool/skills Skills system (list, view, manage) labels Apr 23, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Competing fix for #12260 alongside #12331 and #12736. All three address the same ~ expansion bug in skill config resolution.

@briandevans

Copy link
Copy Markdown
Contributor Author

Thanks @alt-glitch for the triage. 👍

Looked at #12736 (@Tranquil-Flow) — their expand_hermes_path helper in hermes_constants.py is a cleaner architectural choice than mine (centralised, also fixes get_external_skills_dirs which I explicitly scope-deferred). If maintainers pick that one, happy to close this PR.

A few edge-case tests from mine that might be worth porting if #12736 is selected:

  • ~//foo (double separator) / ~\\foo (Windows backslash) — the naive os.path.join(home, "/foo") returns /foo absolute, bypassing the subprocess HOME
  • ~user passthrough (defers to OS passwd — not hijacked by subprocess HOME)
  • Subprocess-HOME short-circuit via kwarg (avoids repeated os.path.isdir in loops)

Feel free to cherry-pick / ignore. Whichever approach lands, the user-facing fix matters more than attribution.

briandevans and others added 2 commits April 29, 2026 19:15
…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>
@briandevans briandevans force-pushed the fix/skill-config-subprocess-home-expansion branch from e627b3f to cded49d Compare April 30, 2026 02:16
@briandevans

Copy link
Copy Markdown
Contributor Author

Closing in favor of @Tranquil-Flow's #12736, which uses a cleaner centralized expand_hermes_path helper. Happy to reopen if useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/agent Core agent loop, run_agent.py, prompt builder P2 Medium — degraded but workaround exists 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]: resolve_skill_config_values() uses wrong HOME semantics for ~ expansion — expands against Python process HOME instead of Hermes subprocess HOME

4 participants