refactor: sweep remaining bare expanduser() to safe_expanduser() (depends on #41870) - #41881
refactor: sweep remaining bare expanduser() to safe_expanduser() (depends on #41870)#41881rickychen-xm wants to merge 2 commits into
Conversation
Path.expanduser() raises RuntimeError('Could not determine home
directory') when HOME is unset and pwd.getpwuid() fails. This
condition surfaces in launchd-managed daemons (common macOS gateway
deployment), containerized runs, and sudo -E invocations.
safe_expanduser() wraps the call in a (RuntimeError, OSError) catch
and returns the original path (or an explicit default) instead of
crashing — matching what virtually every caller in this codebase
wants when HOME can't be resolved.
Changes:
- utils.py: new safe_expanduser() function with full docstring
- agent/subdirectory_hints.py: switch from Path().expanduser()
to safe_expanduser() as the first production adoption
- tests/test_utils_expanduser.py: 7 tests covering normal,
edge-case, and failure-path behavior
Adopts safe_expanduser() (introduced in NousResearch#41870) across all remaining production call sites — 70 conversions across 36 files. The same 'Could not determine home directory' RuntimeError that crashed agent/subdirectory_hints.py affects every caller that uses Path().expanduser() or X.expanduser() in a context where HOME may be absent — launchd-managed daemons (the macOS gateway path), Docker/k8s images with stripped passwd entries, sudo -E invocations, etc. This is a mechanical conversion. Semantics are preserved: - Path(X).expanduser() → safe_expanduser(X) - X.expanduser() (X is Path) → safe_expanduser(X) - Path(X).expanduser().resolve() → safe_expanduser(X).resolve() - Same with .parts / chained ops / division operator Coverage by area: - acp_adapter/ (1 file, 4 sites) - agent/ (7 files, 11 sites) - cron/ (2 files, 3 sites) - gateway/ (5 files, 10 sites) - hermes_cli/ (10 files, 28 sites) - plugins/ (6 files, 7 sites) - tools/ (6 files, 15 sites) - cli.py (1 site) Intentionally NOT changed in this PR: - tests/ (test code is allowed to use bare expanduser) - skills/ (per-skill scripts, independent lifecycle) - optional-skills/ (same as above) - utils.py (where safe_expanduser is implemented) Depends on NousResearch#41870 (safe_expanduser() definition).
teknium1
left a comment
There was a problem hiding this comment.
Thanks for documenting the HOME-resolution failure mode. The failure class remains relevant, but this sweep needs a current-code audit before it is salvageable.
Problems
- The current checkout still has unhandled direct expansion in
cron/lifecycle_guard.py:89;_read_script_for_scanning()catches onlyOSErrorat:108. This path is outside the PR despite matching the stated crash class. Other direct calls remain in PR-touched surfaces such asagent/context_references.py:145,tools/file_tools.py:1212, andcron/scheduler.py:2049. - The
agent/subdirectory_hints.pypilot conversion overlaps a focused implementation already on main:c126a99fc1e2f82a1e23ebe27fb52e26687fdafacatchesRuntimeErroratagent/subdirectory_hints.py:147, with regressions attests/agent/test_subdirectory_hints_tilde.py:19and:38.
Suggested changes
- Re-scope from the old static list to a fresh audit of current call sites, preserving each caller's existing error contract; include the cron lifecycle guard path or coordinate with #56517.
- Drop or re-evaluate the already-landed subdirectory-hints hunk.
Automated hermes-sweeper review.
| @@ -127,7 +129,7 @@ def _add_path_candidate(self, raw_path: str, candidates: Set[Path]): | |||
| ``project/src/`` has no hint files of its own. | |||
| """ | |||
There was a problem hiding this comment.
Current main already resolves this walker failure without changing expansion behavior: c126a99fc catches RuntimeError around this call, with regressions in tests/agent/test_subdirectory_hints_tilde.py:19 and :38. Re-evaluate this overlapping hunk during salvage.
| assert result == Path("/some/path") | ||
|
|
||
| def test_respects_default(self): | ||
| """If default is provided, it is returned on expansion failure.""" |
There was a problem hiding this comment.
test_respects_default does not trigger an expansion failure, so it only proves the return type; test_default_on_expand_failure already exercises the fallback. Please either force the failure condition here or remove this redundant assertion.
What does this PR do?
Completes the adoption of
safe_expanduser()across the entire core codebase, sweeping remaining barePath().expanduser()andPathVar.expanduser()invocations that would crash withRuntimeError("Could not determine home directory")in environments where HOME cannot be resolved.This is the follow-up sweep PR. The helper was introduced in #41870 and adopted by
agent/subdirectory_hints.pyas the pilot site.Type of Change
Changes Made
36 files / +106 / −70 | 70 call sites converted across 8 areas:
agent/acp_adapter/cron/gateway/hermes_cli/plugins/tools/cli.pyConversion pattern (mechanical):
Only the
expanduser()call is wrapped — all chained operations (.resolve(),.parts,/,.is_absolute()etc.) are preserved unchanged.Not changed (out of scope)
tests/— test code intentionally uses bareexpanduser()(valid HOME assumed)skills/,optional-skills/— per-skill scripts with independent lifecycleutils.py— definessafe_expanduseritselfHow to Test
Merge order
Merge #41870 first — this branch is built on top of it (both commits are included in this PR). After #41870 merges upstream, this PR's diff shrinks to just the sweep commit.
Checklist
Code
refactor: sweep ...)python -c "import <module>"— 36/36 import clean, no syntax errors introducedDocumentation & Housekeeping