Repository navigation
Conversation
…cal skills _copy_dist_payload previously used shutil.rmtree + copytree for directories, destroying all locally-installed skills on every profile install or update. The distribution_owned manifest field existed but was never enforced during the copy phase. New _additive_copytree helper walks the source tree and overwrites only files present in the distribution, leaving local-only files intact. This prevents data loss while still correctly updating distribution shipped content. Closes NousResearch#25120
zccyman
force-pushed
the
fix/profile-install-additive-merge
branch
from
May 18, 2026 00:28
342c2ff to
e220f85
Compare
14 of 19 tasks
teknium1
reviewed
Jul 13, 2026
teknium1
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for addressing the destructive profile-update path; current main still has the rmtree(dest) behavior at hermes_cli/profile_distribution.py:573-577, so the underlying report is valid.
Problems
- The new helper applies its
ignorecallback at every recursive level (hermes_cli/profile_distribution.py:541-545in this PR). The supplied callback excludes every nested basename inUSER_OWNED_EXCLUDE, which regresses the nested-path fix ine53b74c39450d85d210ba06e69be5022278eb974; current regression coverage starts attests/hermes_cli/test_profile_distribution.py:505. - A fully additive traversal retains removed distribution files indefinitely. The documented contract says distribution-owned paths are replaced (
website/docs/user-guide/profile-distributions.md:247), but the added tests do not cover a removed or renamed shipped file.
Suggested changes
- Preserve the current root-only exclusion behavior when salvaging; the outer staged-root guard already filters protected root paths.
- Add an update test for removed/renamed shipped content and implement the selected replacement policy. Linked PR #44386 contains a related selective-merge approach worth comparing.
Automated hermes-sweeper review.
| continue | ||
| dest_entry = dst / name | ||
| if entry.is_dir(): | ||
| _additive_copytree(entry, dest_entry, ignore=ignore) |
Collaborator
There was a problem hiding this comment.
Forwarding this callback recursively reintroduces the nested-basename exclusion fixed on main by e53b74c: USER_OWNED_EXCLUDE entries such as tools/bin or skills/<category>/hermes-agent are skipped at every depth. Keep the current root-only exclusion behavior; the outer staged-root loop already filters protected root entries.
This was referenced Sep 14, 2026
Closed
13 of 19 tasks
This branch has not been deployed
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.
Summary
_copy_dist_payloadusedshutil.rmtree+copytreefor all directories, destroying locally-installed skills on everyhermes profile installorhermes profile update. Thedistribution_ownedmanifest field was parsed but never enforced during the copy phase.Before
Walking a profile with
skills/devops/my-custom-skill/→ gone after update.After
New
_additive_copytreehelper walks the source tree and copies/overwrites only files present in the distribution, preserving everything else:Behavior
Testing
_additive_copytree(new files, preserve local, overwrite dist, nested merge, ignore)_copy_dist_payload(install preserves locals, update overwrites dist only, fresh install)Files changed
hermes_cli/profile_distribution.py_additive_copytreehelper; replacermtree+copytreewith additive mergetests/hermes_cli/test_profile_distribution_additive.pyCloses #25120