Repository navigation
fix(distribution): preserve profile skills on update - #110933
MohamadKanso wants to merge 2 commits into
Conversation
| # Default distribution-owned paths (relative to profile root). Authors may override via | ||
| # ``distribution_owned:``. config.yaml is dist-owned but preserved on update by default. | ||
| DEFAULT_DIST_OWNED: Tuple[str, ...] = ("SOUL.md", "config.yaml", "mcp.json", "skills", "cron", MANIFEST_FILENAME) | ||
| DEFAULT_DIST_OWNED: Tuple[str, ...] = ("SOUL.md", "config.yaml", "mcp.json", "cron", MANIFEST_FILENAME) |
There was a problem hiding this comment.
I not sure it is that simple. There are factory skills that come along for the ride (and skills in subdirectories) but there are other skills in sub-directories that are part of Hermes locally generating self learned patterns.
alt-glitch
added
type/bug
Competes with open #25150 (additive copytree in |
ehz0ah
left a comment
ehz0ah
left a comment
There was a problem hiding this comment.
I reproduced three destructive edge cases that the current merge does not handle. The existing profile-distribution suite passes, but focused regressions fail at this exact head. The merge needs to distinguish user-owned skills from distribution-owned skill roots before this is safe to land.
| target: Path, | ||
| manifest: DistributionManifest, | ||
| preserve_config: bool, | ||
| preserve_skills: bool = False, |
There was a problem hiding this comment.
[P1] The new preservation switch defaults to false, and the force-install path does not enable it. Install a distribution, add skills/user-created/SKILL.md, then run install_distribution(..., force=True). The custom skill is deleted even though --force says user data is preserved. This leaves the same destructive copy behavior active in the other overwrite path. Please preserve unshipped skills during force reinstall too and add a regression test for this flow.
| if dest.exists() and not merge_skills: | ||
| shutil.rmtree(dest) | ||
| shutil.copytree(src, dest, ignore=_ignore_user_owned) | ||
| shutil.copytree(src, dest, ignore=_ignore_user_owned, dirs_exist_ok=merge_skills) |
There was a problem hiding this comment.
[P1] dirs_exist_ok=True overlays files inside every staged skill root. If v1 ships skills/demo/retired.py and v2 removes it, an update leaves retired.py active. File-to-directory and directory-to-file transitions also produce the wrong result or raise shutil.Error. I reproduced these cases on this head, while the recorded base removes the stale entries. Please preserve skill roots absent from the new payload, but replace each skill root that the new distribution still ships.
| if dest.exists() and not merge_skills: | ||
| shutil.rmtree(dest) | ||
| shutil.copytree(src, dest, ignore=_ignore_user_owned) | ||
| shutil.copytree(src, dest, ignore=_ignore_user_owned, dirs_exist_ok=merge_skills) |
There was a problem hiding this comment.
[P1] This merge follows existing destination symlinks. I replaced skills/demo/helper.py with a symlink to a file outside the profile, then updated a distribution that ships that path. The update overwrote the external file. The recorded base does not do this because it removes the skills tree before copying. Please inspect destination entries without following symlinks and replace or reject symlinks before copying.
|
Independent validation of exact head The reported mechanism is real. On fetched current The same reproduction on this PR produced Local verification of this exact head:
The effective runtime fix is the update-only The PR is open under For the exact issue scope ( |
|
thanks for the reproduction. i pushed c60656d. the update path now treats each top-level entry under skills as its own ownership boundary: shipped roots are replaced cleanly so stale files disappear, roots missing from the new payload stay, and file/directory swaps are safe. destination symlinks are removed before copying, while source symlinks are still rejected. forced reinstalls keep the same user-skill protection for existing profiles. added regressions for custom and stale roots, file/directory changes, symlink safety, and force reinstall. profile distribution tests: 56 passed, plus ruff, diff, and compat checks. |
1 similar comment
…t skills The skills-only special case (a `preserve_skills` kwarg plus nested closures keyed on the literal "skills") left cron/ and any future owned directory on the old rmtree-then-copytree path, which deletes user-added files on update and on --force reinstall. The rule is the same for all of them: a top-level owned directory is a container of roots; replace only the roots the payload ships, never the whole destination. Apply that rule to every owned directory, drop the kwarg and the string check, and lift the helpers to module level. `_remove_existing` keeps its symlink-no-follow semantics and `_real_dir` replaces any symlink or file on the destination path so the payload is never written outside the profile. The fresh-install path had nothing to preserve, so it needs no flag. Restore "skills" in DEFAULT_DIST_OWNED; removing it was never needed for the fix. Add one invariant test: a user-added cron/ file survives an update. Refs NousResearch#110933
Keep the tests that pin behaviour a regression would break: user-added and stale skill roots survive an update while shipped roots are replaced, a forced reinstall keeps a user skill, and a user-added cron file survives an update. The allowlist and symlink-transition cases exercise the same code path (`_real_dir` + `_replace_entry`) and were duplicates of those. Refs NousResearch#110933
…t skills The skills-only special case (a `preserve_skills` kwarg plus nested closures keyed on the literal "skills") left cron/ and any future owned directory on the old rmtree-then-copytree path, which deletes user-added files on update and on --force reinstall. The rule is the same for all of them: a top-level owned directory is a container of roots; replace only the roots the payload ships, never the whole destination. Apply that rule to every owned directory, drop the kwarg and the string check, and lift the helpers to module level. `_remove_existing` keeps its symlink-no-follow semantics and `_real_dir` replaces any symlink or file on the destination path so the payload is never written outside the profile. The fresh-install path had nothing to preserve, so it needs no flag. Restore "skills" in DEFAULT_DIST_OWNED; removing it was never needed for the fix. Add one invariant test: a user-added cron/ file survives an update. Refs #110933
Keep the tests that pin behaviour a regression would break: user-added and stale skill roots survive an update while shipped roots are replaced, a forced reinstall keeps a user skill, and a user-added cron file survives an update. The allowlist and symlink-transition cases exercise the same code path (`_real_dir` + `_replace_entry`) and were duplicates of those. Refs #110933
…t skills The skills-only special case (a `preserve_skills` kwarg plus nested closures keyed on the literal "skills") left cron/ and any future owned directory on the old rmtree-then-copytree path, which deletes user-added files on update and on --force reinstall. The rule is the same for all of them: a top-level owned directory is a container of roots; replace only the roots the payload ships, never the whole destination. Apply that rule to every owned directory, drop the kwarg and the string check, and lift the helpers to module level. `_remove_existing` keeps its symlink-no-follow semantics and `_real_dir` replaces any symlink or file on the destination path so the payload is never written outside the profile. The fresh-install path had nothing to preserve, so it needs no flag. Restore "skills" in DEFAULT_DIST_OWNED; removing it was never needed for the fix. Add one invariant test: a user-added cron/ file survives an update. Refs #110933
Keep the tests that pin behaviour a regression would break: user-added and stale skill roots survive an update while shipped roots are replaced, a forced reinstall keeps a user skill, and a user-added cron file survives an update. The allowlist and symlink-transition cases exercise the same code path (`_real_dir` + `_replace_entry`) and were duplicates of those. Refs #110933
…t skills The skills-only special case (a `preserve_skills` kwarg plus nested closures keyed on the literal "skills") left cron/ and any future owned directory on the old rmtree-then-copytree path, which deletes user-added files on update and on --force reinstall. The rule is the same for all of them: a top-level owned directory is a container of roots; replace only the roots the payload ships, never the whole destination. Apply that rule to every owned directory, drop the kwarg and the string check, and lift the helpers to module level. `_remove_existing` keeps its symlink-no-follow semantics and `_real_dir` replaces any symlink or file on the destination path so the payload is never written outside the profile. The fresh-install path had nothing to preserve, so it needs no flag. Restore "skills" in DEFAULT_DIST_OWNED; removing it was never needed for the fix. Add one invariant test: a user-added cron/ file survives an update. Refs NousResearch#110933
Keep the tests that pin behaviour a regression would break: user-added and stale skill roots survive an update while shipped roots are replaced, a forced reinstall keeps a user skill, and a user-added cron file survives an update. The allowlist and symlink-transition cases exercise the same code path (`_real_dir` + `_replace_entry`) and were duplicates of those. Refs NousResearch#110933
|
closing this original PR in favor of merged #111344, which carried the implementation commits forward with the extra distribution safety follow-ups. |
6 participants
summary
profile updates used to delete the whole skills directory before copying the staged one. that could remove custom skills and older bundled skills that were not in the newest distribution.
this change:
fixes #110920
testing
scripts/run_tests.sh tests/hermes_cli/test_profile_distribution.py -q— 54 passedruff check hermes_cli/profile_distribution.py tests/hermes_cli/test_profile_distribution.pypython3 scripts/check_compat_pointers.pygit diff --check