Repository navigation
fix(cli): scope USER_OWNED_EXCLUDE to root in profile distribution copy - #31123
Closed
briandevans wants to merge 1 commit into
Closed
briandevans wants to merge 1 commit into
briandevans wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes an install/update bug where USER_OWNED_EXCLUDE was incorrectly applied at all descendant depths (by basename), causing distribution-owned nested paths to be silently skipped during payload copy.
Changes:
- Remove
shutil.copytree(..., ignore=...)filtering soUSER_OWNED_EXCLUDEis effectively root-scoped. - Add regression tests ensuring nested distribution paths with protected basenames are preserved while root-level protected entries remain excluded.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/hermes_cli/test_profile_distribution.py | Adds regression tests covering root-scoped USER_OWNED_EXCLUDE behavior. |
| hermes_cli/profile_distribution.py | Updates directory copy logic to stop ignoring protected basenames in nested directories. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
558
to
+567
| if entry.is_dir(): | ||
| if dest.exists(): | ||
| shutil.rmtree(dest) | ||
| shutil.copytree( | ||
| entry, | ||
| dest, | ||
| ignore=lambda d, names: [n for n in names if n in USER_OWNED_EXCLUDE], | ||
| ) | ||
| # ``USER_OWNED_EXCLUDE`` is root-scoped: the loop above already | ||
| # filters protected root entries. Filtering by basename at every | ||
| # descendant depth (the prior ``ignore=`` lambda) silently dropped | ||
| # distribution-owned nested paths that happened to share a name | ||
| # with a root-level protected entry, e.g. | ||
| # ``skills/<category>/hermes-agent/SKILL.md``. | ||
| shutil.copytree(entry, dest) |
briandevans
force-pushed
the
fix/profile-distribution-nested-basename-31033
branch
3 times, most recently
from
May 29, 2026 21:12
11e6f37 to
d5bb4f9
Compare
briandevans
force-pushed
the
fix/profile-distribution-nested-basename-31033
branch
from
June 2, 2026 22:13
d5bb4f9 to
b12d11c
Compare
`_copy_dist_payload` filtered protected basenames at every recursive depth via `shutil.copytree(..., ignore=lambda d, names: ...)`. The outer `for entry in staged.iterdir()` already drops root-level user-owned entries, so the inner lambda was redundant for the protection use case but actively wrong for distribution-owned nested paths that share a basename with a root-level protected entry — e.g. `skills/<category>/hermes-agent/SKILL.md`. The nested skill silently disappeared during `hermes profile install --force` / `hermes profile update`. Drop the per-depth `ignore=` argument. Root-scoped exclusion stays intact via the existing outer-loop check at the top of `_copy_dist_payload`. The sibling export path (`profiles.py::_default_export_ignore`) already implements the correct "root-only vs all-depth" split — this aligns the install/update path with that intent. Fixes NousResearch#31033
briandevans
force-pushed
the
fix/profile-distribution-nested-basename-31033
branch
from
June 5, 2026 23:15
b12d11c to
aceb400
Compare
Collaborator
|
This appears to be implemented on current main by a separate fix. Automated hermes-sweeper review evidence:
Thanks for the detailed root-scope analysis here; the behavior described by this PR is now present on main. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
hermes_cli/profile_distribution.py::_copy_dist_payloadfiltered protectedbasenames at every recursive depth via
shutil.copytree(..., ignore=lambda d, names: [n for n in names if n in USER_OWNED_EXCLUDE]).The outer
for entry in staged.iterdir()already drops root-leveluser-owned entries (lines 545-549), so the inner lambda was redundant for
the protection use case but actively wrong for distribution-owned nested
paths that share a basename with a root-level protected entry — for
example
skills/<category>/hermes-agent/SKILL.md. The nested skillsilently disappeared during
hermes profile install --force/hermes profile update.Drop the per-depth
ignore=argument. Root-scoped exclusion stays intactvia the existing outer-loop check at the top of
_copy_dist_payload. Thesibling export path (
hermes_cli/profiles.py::_default_export_ignore)already implements the correct "root-only vs all-depth" split, so this
aligns the install/update path with the same intent.
Related Issue
Fixes #31033
Type of Change
Changes Made
hermes_cli/profile_distribution.py— drop theignore=lambda from therecursive
shutil.copytreecall inside_copy_dist_payload; commentdocuments the root-scoped intent.
tests/hermes_cli/test_profile_distribution.py— addTestUserOwnedExcludeRootScopewith two regression cases:test_nested_distribution_path_with_protected_basename_preserved—proves
skills/<category>/hermes-agent/SKILL.mdlands in the targetprofile.
test_root_protected_basename_still_excluded— proves a root-levelhermes-agent/in the staging payload is still skipped, so the fixdoes not weaken the root-scoped protection.
How to Test
skills/autonomous-ai-agents/hermes-agent/SKILL.mdand a root-levelhermes-agent/directory.uv run --with pytest --with pytest-xdist --with pytest-asyncio --with pytest-timeout python3 -m pytest tests/hermes_cli/test_profile_distribution.py -vnew tests fail with
Nested distribution-owned hermes-agent/ was dropped.Checklist
Code
fix(scope):,feat(scope):, etc.)Documentation & Housekeeping
docs/, docstrings) — N/A; behavior matches the docstring's stated intentcli-config.yaml.exampleif I added/changed config keys — N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — N/Apathlib/shutilchange, platform-neutralRelated / Positioning
@zccyman's open PR fix(cli): use additive merge in profile install/update to preserve local skills #25150 (additive-merge fix for destructive
rmtree+copytreedeletion of local skills). The two fixes areorthogonal: fix(cli): use additive merge in profile install/update to preserve local skills #25150 swaps the destructive copy for an additive walk but
keeps the same
ignore=lambda d, names: ...pattern, so the nestedbasename bug would survive that PR untouched. Whichever lands first,
the other should rebase trivially — only the same
ignore=argumentneeds to be removed from the new helper.