Skip to content

fix(doctor): use the bounded wrapper scanner for orphan profile aliases - #84464

Open
AlexxRussell wants to merge 2 commits into
NousResearch:mainfrom
AlexxRussell:fix/doctor-bounded-orphan-alias-scan
Open

fix(doctor): use the bounded wrapper scanner for orphan profile aliases#84464
AlexxRussell wants to merge 2 commits into
NousResearch:mainfrom
AlexxRussell:fix/doctor-bounded-orphan-alias-scan

Conversation

@AlexxRussell

Copy link
Copy Markdown
Contributor

What does this PR do?

Doctor's orphan profile alias check read every entry in the profile wrapper directory in full:

for wrapper in wrapper_dir.iterdir():
    if not wrapper.is_file():
        continue
    try:
        content = wrapper.read_text(encoding="utf-8")

That directory is normally ~/.local/bin, which on a real machine also holds large unrelated binaries (uv, Python, node, ffmpeg). Path.read_text() pulls each of those entirely into memory. On a 1 GB VPS running Hermes this is fatal: Doctor was OOM killed with SIGKILL while running that loop.

hermes_cli/profiles.py already ships a scanner built for exactly this directory. build_alias_map() reads at most _WRAPPER_READ_LIMIT (8192) bytes per candidate, opens with errors="strict" so binaries raise UnicodeDecodeError and are skipped, and is already the path used for profile listing. Doctor should reuse it rather than keep a second, unbounded scan of the same directory.

Orphans then fall out of the existing map: the profiles it resolved that no longer exist.

Related Issue

No existing issue. Found in production on a memory constrained host.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

  • hermes_cli/doctor.py: added _find_orphan_profile_aliases(), which reads build_alias_map() and keeps the entries whose profile is missing.
  • hermes_cli/doctor.py: run_doctor() now iterates that helper instead of walking the wrapper directory itself. Dropped the now unused profile_exists and re imports from that block.
  • tests/hermes_cli/test_doctor_orphan_alias_scan.py: new file, 5 tests.

Relationship to #77058

#77058 also touches this block, and the two do not overlap. That PR replaces the regex with _profile_from_wrapper() to support quoted aliases, but keeps wrapper.read_text(encoding="utf-8"), so the unbounded read remains (and shlex.split() then runs over the whole file). This PR is the memory fix and does not change parsing.

If #77058 lands first I am happy to rebase. My suggestion in that case is that quoted alias support belongs inside build_alias_map() rather than in a second parser in Doctor, so both Doctor and profile listing get it from one place. Happy to do that follow up if a maintainer prefers it.

How to Test

  1. Put a file larger than 8192 bytes in the profile wrapper directory whose hermes -p <profile> marker sits past the first 8192 bytes.
  2. Run hermes doctor.
  3. Before this change the marker is found (the whole file was read) and a bogus orphan alias is reported. After it, the entry is correctly ignored.

Behavioural differences I am aware of

Disclosing these rather than leaving them to be found in review. All three follow from reusing the shared scanner:

  1. Suffixed entries are no longer inspected. build_alias_map() skips entries with a suffix on POSIX and requires .bat on Windows. That matches how Hermes creates wrappers, but a hand renamed wrapper such as myalias.sh would no longer be reported as an orphan. The old loop checked every file.
  2. One warning per orphaned profile, not per wrapper. build_alias_map() keeps a single alias per profile (a custom alias wins over the profile named one). If two wrappers point at the same missing profile, the old code printed two warnings and this prints one.
  3. On Windows the reported alias is now the stem (foo) rather than the file name (foo.bat).

If any of these matter, the alternative is a new bounded helper in profiles.py that returns every matching wrapper rather than a deduplicated map. I went with reuse because a second scanner of the same directory is what caused this bug.

Verification

Run against current main with pytest:

  • New file: 5 passed.
  • Proof of coverage: reverting only hermes_cli/doctor.py gives 2 failed, 3 passed. The two failures are behavioural, not import errors:
    • test_profile_marker_after_read_limit_is_ignored (a marker past the read limit is visible again)
    • test_wrapper_dir_entries_are_never_read_whole (Path.read_text() is called on a wrapper dir entry again)
      The other three pass either way by design, since they assert that existing behaviour is preserved.
  • Neighbouring suites tests/hermes_cli/test_profiles.py and tests/hermes_cli/test_doctor.py: 97 passed, 2 skipped, 0 failed.

All five tests drive the real run_doctor code path and assert on its output rather than calling the new helper directly.

Checklist

Code

  • I've read the Contributing Guide

  • My commit messages follow Conventional Commits

  • I searched for existing PRs to make sure this isn't a duplicate (see the fix(doctor): support local Mem0 and quoted profile aliases #77058 section above)

  • My PR contains only changes related to this fix

  • I've run pytest tests/ -q and all tests pass

    Not ticked deliberately. I ran the new file plus the two neighbouring suites listed above. I did not run the whole tree, so I am not claiming it. CI is the authority here.

  • I've added tests for my changes

  • I've tested on my platform: Debian 12 (the affected host) and macOS 15

Documentation & Housekeeping

  • I've updated relevant documentation, or N/A. N/A, no user facing behaviour or config changes.
  • I've updated cli-config.yaml.example if I added/changed config keys, or N/A. N/A, no new keys.
  • I've updated CONTRIBUTING.md or AGENTS.md, or N/A. N/A.
  • I've considered cross-platform impact, or N/A. Covered in the behavioural differences section above; the Windows .bat and stem handling both come from the shared scanner.
  • I've updated tool descriptions/schemas, or N/A. N/A.

Doctor read every entry in the profile wrapper directory in full via
Path.read_text() while looking for orphan profile aliases. That directory
is normally ~/.local/bin, which also holds large unrelated binaries, so on
a small host the scan retained hundreds of MiB and Doctor was OOM killed.

Reuse hermes_cli.profiles.build_alias_map(), the scanner already used for
profile listing. It reads at most _WRAPPER_READ_LIMIT bytes per candidate
and skips binaries, then orphans are the mapped profiles that no longer
exist.
@alt-glitch alt-glitch added type/bug Something isn't working comp/cli CLI entry point, hermes_cli/, setup wizard area/profiles Multi-profile isolation, HERMES_HOME scoping P2 Medium — degraded but workaround exists labels Aug 12, 2026
@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

fix(doctor): use the bounded wrapper scanner for orphan profile aliases

Reviewed hermes_cli/doctor.py, the build_alias_map path in hermes_cli/profiles.py, and the new tests. Reusing the bounded scanner is a clear improvement over whole-file reads, and the regression tests for the read limit are good.

A few observations:

  1. Behavior parity with the old scanner. The old code matched any wrapper whose content contained hermes -p (\S+) verbatim; the new path goes through build_alias_map, which normalizes the profile name (normalize_profile_name) and only scans the first 8192 bytes. An alias whose target normalizes to a different string now reports under the normalized name — likely more correct (fewer false "missing profile" reports) but a behavior change. If the narrowing is intentional, a parity test pinning old-vs-new output would prevent silent coverage loss.

  2. Multiple wrappers for one missing profile collapse to a single report. build_alias_map stores one {profile: alias} pair (custom alias wins, profile-named wrapper only via setdefault), so two dangling wrappers pointing at the same missing profile yield one Orphan alias line, whereas the old loop reported every orphan wrapper file. If per-wrapper reporting matters, iterate the wrapper dir for all matches instead of the reverse map.

  3. Minor: _find_orphan_profile_aliases returns a deterministic sorted list — nice for stable doctor output.

build_alias_map() is keyed by profile, so it holds exactly one alias per
profile. Reading orphan wrappers back out of it collapsed several stranded
wrappers that name the same missing profile into a single warning. That is
not a rare shape: adding a custom alias leaves both the profile-named
wrapper and the alias wrapper on disk, so removing the profile strands two
files and Doctor named only one of them.

Extract the bounded per-wrapper scan into iter_wrapper_aliases() and build
build_alias_map() on top of it. The alias map keeps its exact previous
semantics and its two callers are untouched, while Doctor can now walk
every wrapper. The 8 KB read cap and the binary skip that this branch
introduced are unchanged, so the unbounded read stays gone.

Tests: two stranded wrappers now produce two warnings (fails without this
change, reporting only one). Also pins the case-folding behaviour raised in
review: a mixed-case target is reported under the canonical lowercase id,
and case never decides whether a profile counts as missing, since
profile_exists() already normalises its argument.
@AlexxRussell

Copy link
Copy Markdown
Contributor Author

Point 2 is correct and is now fixed in f782ae1.

build_alias_map() is keyed by profile, so it holds exactly one alias per profile, and reading orphan wrappers back out of it collapsed several stranded wrappers naming the same missing profile into a single warning. It is the common shape rather than an edge case: adding a custom alias leaves both the profile-named wrapper and the alias wrapper on disk, so removing that profile strands two files and only one was named.

Rather than iterate the wrapper dir separately, which would stand up a second scanner beside the first, I extracted the bounded per-wrapper scan into iter_wrapper_aliases() and rebuilt build_alias_map() on top of it. The alias map keeps its exact previous semantics, its two existing callers are untouched, and the 8192 byte cap plus the binary skip are unchanged, so the unbounded read stays gone. The new test puts two stranded wrappers in the dir and expects two warnings; it fails on the previous commit, reporting only one.

On point 1, the normalization is not narrowing which profiles get reported. profile_exists() already normalizes its own argument:

# hermes_cli/profiles.py:382
def profile_exists(name: str) -> bool:
    canon = normalize_profile_name(name)

The old path called profile_exists("Ghost"), which normalized to ghost internally and reached the same verdict the new path reaches. The set of profiles considered missing is identical before and after. What did change is the string in the warning: it now prints the canonical lowercase id instead of the literal token found in the wrapper. Both halves are pinned with tests, one asserting the canonical id is what gets reported, one asserting that case never decides whether a profile counts as missing.

I left out the old versus new parity test, since it would pin the scanner this PR exists to remove, and the remaining differences are the deliberate ones listed in the description. On the largest of those: alias names are validated against _PROFILE_ID_RE ([a-z0-9][a-z0-9_-]{0,63}), which forbids ., so skipping suffixed entries on POSIX cannot drop a wrapper that hermes itself created. It only skips foreign files in the wrapper dir that happen to contain the marker.

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

Labels

area/profiles Multi-profile isolation, HERMES_HOME scoping comp/cli CLI entry point, hermes_cli/, setup wizard P2 Medium — degraded but workaround exists type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants