Conversation
…e allowlist Fixes NousResearch#74373. DistributionManifest.from_dict() parses distribution_owned, and owned_paths() resolves it (falling back to DEFAULT_DIST_OWNED when unset), but the install/update mutation path (_copy_dist_payload()) never called owned_paths() at all. It iterated every top-level staged entry and copied it unless the name was in USER_OWNED_EXCLUDE or hit one of the .env.template/config.yaml special cases -- so an explicit manifest like: distribution_owned: - SOUL.md still installed unrelated top-level entries (unlisted.txt, mcp.json, cron/, etc.) that were never declared. On update, a staged directory also replaced the corresponding destination directory wholesale (shutil.rmtree then a fresh shutil.copytree), silently deleting any target-only children a user had added inside an owned directory (e.g. skills/local-only/). Fix: 1. Resolve owned = set(manifest.owned_paths()) once before the copy loop; skip any top-level staged entry whose name isn't in that set (after the existing USER_OWNED_EXCLUDE and .env.template handling, which are unconditional regardless of the allowlist). 2. Directory updates now use shutil.copytree(..., dirs_exist_ok=True) instead of rmtree + fresh copytree -- this overwrites/adds the staged files but preserves a target-only child the staged directory doesn't mention, so a user file inside an owned directory survives an update instead of being silently deleted. Per the issue's own "Suggested direction," added regression tests for all 5 scenarios it named: listed entry installed, unlisted entry excluded, top-level hard exclusions preserved regardless of the manifest, target-only children surviving an update via merge-copy semantics, and config.yaml preservation / --force-config retained. Also fixed 4 pre-existing tests in TestNestedUserOwnedExcludeNotFiltered that had encoded the pre-fix behavior as expected (installing an undeclared top-level directory like tools/ or scripts/ just because it wasn't in USER_OWNED_EXCLUDE) -- updated them to explicitly opt the new top-level directory into distribution_owned, preserving their actual intent (a nested dir sharing a name with a USER_OWNED_EXCLUDE entry, like tools/bin/, must not be filtered by name collision) under the corrected allowlist semantics. 75/75 tests pass in the full tests/hermes_cli/test_profile_distribution.py file (6 new, 4 fixed, 65 unaffected); 11/11 in tests/hermes_cli/test_noninteractive_git.py (no regression).
teknium1
left a comment
There was a problem hiding this comment.
Thanks for addressing a real current-main defect: _copy_dist_payload() currently ignores DistributionManifest.owned_paths() (hermes_cli/profile_distribution.py:563-590).
Problems
- The new root-name allowlist check at
hermes_cli/profile_distribution.py:584cannot support documented nested entries such asskills/research/andcron/digest.json(website/docs/user-guide/profile-distributions.md:251-258;website/docs/reference/profile-commands.md:420-433). Those values do not equal root entriesskillsorcron, so their source content is skipped. dirs_exist_ok=Trueathermes_cli/profile_distribution.py:596retains files removed from a later distribution revision. That changes the documented replacement contract (website/docs/user-guide/profile-distributions.md:247,385) and does not distinguish stale distributed files from target-only user files.
Suggested changes
- Copy validated manifest-relative paths and add install/update coverage for nested file and directory allowlist entries.
- Settle and document directory ownership semantics; preserve user files only with a mechanism that still handles distributor removals correctly.
This is an automated hermes-sweeper review.
| # variables; it is not itself a content path subject to the | ||
| # owned_paths() allowlist. | ||
| shutil.copy2(entry, target / ENV_EXAMPLE_FILENAME) | ||
| continue |
There was a problem hiding this comment.
This compares a root entry.name with the complete manifest path. The documented distribution_owned syntax permits skills/research/ and cron/digest.json, neither of which matches root entries skills or cron; both payloads would be skipped. Please copy validated relative manifest paths and add nested-path install/update coverage.
| staged_resolved = staged.resolve() | ||
| shutil.copytree( | ||
| entry, | ||
| dest, |
There was a problem hiding this comment.
dirs_exist_ok=True also retains files that were distributed in an older revision but removed from the new source. The current docs promise distribution-owned paths are replaced. Please establish an ownership mechanism that preserves genuine user additions without retaining stale distributed files, and document the resulting update contract.
|
Both real gaps addressed in #75351:
Three more bugs surfaced while implementing and testing this (all documented in code comments): an upfront rmtree for a whole-directory owned entry would wipe user additions before the diff could run; distribution.yaml is itself a default owned path so the copy loop was clobbering tracking info before it could be read; walking the destination instead of the source to determine what this payload installed incorrectly counted pre-existing stale files; and skipping config.yaml for preserve_config made it look stale and get deleted. 6 new tests plus 2 pre-existing tests updated (they predated any allowlist enforcement and assumed unrestricted copying). 49/49 pass in the full file. Closing this in favor of #75351. |
Context
Fixes #74373.
Problem
_copy_dist_payload()never calledmanifest.owned_paths()at all -- it iterated every top-level staged entry and copied it unless the name was inUSER_OWNED_EXCLUDEor hit a special case. An explicitdistribution_owned: [SOUL.md]manifest still installed unrelated entries (unlisted.txt,mcp.json,cron/, etc.) never declared. On update, a staged directory also replaced the destination directory wholesale, silently deleting any target-only children a user had added inside an owned directory.Fix
owned = set(manifest.owned_paths())once before the copy loop; skip any top-level staged entry whose name isn't in that set.shutil.copytree(..., dirs_exist_ok=True)instead ofrmtree+ freshcopytree-- overwrites/adds staged files but preserves a target-only child, so a user file inside an owned directory survives an update.Verification
Per the issue's own suggested direction, added regression tests for all 5 named scenarios: listed entry installed, unlisted entry excluded, top-level hard exclusions preserved regardless of manifest, target-only children surviving an update, and config.yaml preservation /
--force-configretained. Also fixed 4 pre-existing tests that had encoded the pre-fix behavior as expected, updating them to explicitly opt the relevant directory intodistribution_ownedwhile preserving their actual intent (nested name-collision withUSER_OWNED_EXCLUDEmust not filter).75/75 tests pass in the full
test_profile_distribution.pyfile; 11/11 in a dependent test file (no regression).