Repository navigation
fix(profiles): reserve main and preserve approved distribution content - #109581
guicybercode wants to merge 8 commits into
Conversation
…file Fixes NousResearch#17879. `hermes profile create main` succeeded today and produced session keys with the `agent:main:*` prefix — the exact prefix the default profile uses — re-introducing the cross-profile collision NousResearch#12099 (and the in-flight NousResearch#12266 fix) is meant to eliminate. The same hole let `root`, `hermes`, `test`, `tmp`, and `sudo` through too: `_RESERVED_NAMES` was defined but only consumed by `check_alias_collision()` (a wrapper-script-creation check), not by `validate_profile_name()` or `create_profile()`. Fix: - Add "main" to `_RESERVED_NAMES`. - Replace the one-name `if name == "default"` hardcode in `create_profile()` with a uniform `if name in _RESERVED_NAMES` check (the new error message still names the offending value and lists all reserved names). `validate_profile_name()` is intentionally left permissive so existing callers (`rename_profile`, `delete_profile`, `set_active_profile`, `export_profile`, `import_profile`, `resolve_profile_env`) keep working for any legacy `"main"` profile that may already exist on disk — only *creation* of a new reserved-name profile is restricted. Orthogonal to NousResearch#12266: even after that PR lands, a user who runs `hermes profile create main` still hits the original collision because `_resolve_session_key_prefix("main")` evaluates to `"agent:main"`. This PR closes that gap permanently. Tests: `tests/hermes_cli/test_profiles.py::TestReservedNames` parametrises all seven reserved names and verifies (a) each is rejected, (b) the error message lists the offending name plus the full reserved set, (c) no orphan directory is left behind. All 102 existing profile tests still pass. (cherry picked from commit 7aebb2a)
gaoanze888
left a comment
There was a problem hiding this comment.
Canonicalization and legacy-main handling are correct across create/import/rename and CLI/REST/TUI core paths, but distribution install has a target-identity TOCTOU at exact head ce433902c32bb984ff8ed76ff3070f49f27e3582.
plan_install() permits main only because a legacy profile exists at planning time. install_distribution() later trusts that stale plan and writes without revalidating under a lifecycle lock. I reproduced: plan an update to legacy main, delete the directory, then install with name="MAIN"; it recreates a brand-new reserved profiles/main. A destination replacement between plan/apply can likewise receive unplanned content.
Acquire the profile lifecycle lock and revalidate source/destination identity immediately before publication: a main update is allowed only if the same planned legacy instance still exists; otherwise refuse. Add barrier tests for plan→rename/delete, tombstone/recreate, and destination replacement. Frontends may also provide an early reserved-name message, although server-side rejection already works.
gaoanze888
left a comment
There was a problem hiding this comment.
The destination lifecycle fence at 442b6565f941d14863042844f2a0329fa6cf0a10 fixes the delete/rename/recreate race I reported, and the focused profile suites pass 141 tests (2 platform skips).
One part of the new source fence is still weaker than its contract: source_identity identifies only the source directory inode. For a local-directory distribution, an editor/process can replace or mutate distribution.yaml, SOUL.md, skills/, etc. in place after plan_install() returns without changing that directory identity. _copy_dist_payload() then publishes content different from the reviewed manifest/plan; a file can also become a symlink after _reject_distribution_symlinks() and before copying. The new source_replacement test renames/recreates the whole root, so it does not exercise these same-directory mutations.
Please stage local sources into the operation-owned temporary directory (as Git sources already are), reject symlinks in that immutable staged copy, and plan/publish only from it; alternatively pin and revalidate the complete owned-entry tree, including type/content identity. Add a barrier test that replaces an owned file and one that swaps an owned file/directory to a symlink while preserving the source root inode. Destination locking/identity otherwise looks good.
gaoanze888
left a comment
There was a problem hiding this comment.
Rechecked exact head 80191ada0e52537813c5556421ce462b8f70d00a; the focused profile suite passes 49/49. Copying local inputs to operation-owned staging does fix the same-root post-plan mutations I reported, but two boundary issues remain in the staging operation itself:
-
The “staging outside target” check runs only after
_stage_source()has completedcopytree(). A caller-provided workdir undertarget_diris therefore modified before the operation is rejected. Forworkdir=target/skills/tmp, the rejected plan leavestarget/skills/tmp/local/{distribution.yaml,SOUL.md,...}. The new test preserves existing files but does not assert that this unplanned payload/debris is absent. Resolve/canonicalize the prospective local staging path and target before copying, reject overlap first, and clean a partially-created staged tree on every failure. -
shutil.copytree(local_source, staged, symlinks=True)is not an immutable snapshot against a concurrent source mutation. There is a check/use interval between directory enumeration / symlink classification andcopy2: a regular source entry can be replaced by a symlink during the copy and be dereferenced into a regular staged file, after which_reject_distribution_symlinks(staged)cannot detect its provenance. Please use a copy primitive that opens entries without following links and verifies identity/type around each read (or otherwise treat concurrent-copy inconsistency as an error), with a barrier test that performs the file→symlink swap during staging rather than afterplan_install().
Also wrap staging filesystem failures (PermissionError, disappearing entries, etc.) as DistributionError; _profile_install() catches only DistributionError/ValueError, so a new copytree() failure currently escapes as a traceback instead of the command's controlled error path.
gaoanze888
left a comment
There was a problem hiding this comment.
Rechecked exact head 09b60a3ec3eea20721af508229a5f0ec5a98204d. The new no-follow source handles, pre-copy overlap check, owned-stage cleanup, and OSError normalization resolve the three staging findings from my previous review; focused profile suites pass 119/119, compilation and diff check are clean.
Two lifecycle gaps remain:
-
The staged artifact is mutable between validation and publication.
_revalidate_plan()checks only the staging root directory identity;_copy_dist_payload()then follows descendant paths withis_dir()/copytree()/copy2(). I replaced stagedSOUL.mdwith a symlink after revalidation and before publication; install copied the externalSECRETbytes into a regular target file. Keep verified handles through publication, seal/revalidate the complete staged tree, or publish with the same no-follow identity-verifying traversal used for source capture. Add a barrier at the revalidate→copy boundary. -
Interactive CLI installs a different snapshot from the one the user approved.
_profile_install()stages and renders a preview, destroys that temporary tree after confirmation, theninstall_distribution()stages the mutable source again. A local source or moving git ref can therefore change between prompt and apply. Carry the approved staged plan/artifact into publication instead of re-fetching/re-copying it.
There is also a mode regression in the new staging copier: every source directory is created as 0700 and its verified mode is never restored. A source skills/ at 0755 becomes staged (and then installed) as 0700; files do preserve mode. Apply directory mode after recursively copying children and add file/directory/empty-directory mode tests.
I could not execute the native Windows branch on this host; its handle design is materially stronger, but dedicated Windows coverage for reparse points, UNC/extended paths, EOF and sharing failures would make that guarantee auditable.
gaoanze888
left a comment
There was a problem hiding this comment.
The approved-snapshot reuse, verified no-follow publication traversal, and directory-mode restoration at exact head e8f2a8ebcef4b77a1fa80dab6869fc7400e31139 resolve the three findings from my previous review. Focused profile suites pass 131/131 (10 platform skips), Ruff, py_compile, and diff check are clean.
One publication atomicity issue remains. For an existing profile with multiple owned top-level entries, _copy_dist_payload() commits each entry independently while validating it. If a later entry fails validation, earlier entries are already replaced. I reproduced an allowlist ordered as SOUL.md, then later.md; mutating staged later.md after planning makes install_plan(..., force=True) raise Distribution source content changed after planning, but the target is left mixed: SOUL.md is the new value while later.md remains old. The new tests currently permit this by catching DistributionError and checking only selected paths.
Please fully validate/materialize all owned entries into a separate operation-owned publication tree before changing any destination-owned path, then commit the complete set transactionally (or provide rollback). Add a regression where a late second/third owned entry fails and assert every pre-existing target entry and manifest remain unchanged. This also prevents a failed update from presenting partially updated code under the previous on-disk manifest.
|
The prepare-before-commit transaction at A destination-parent TOCTOU remains inside Please bind destination ancestry through directory handles (POSIX |
gaoanze888
left a comment
There was a problem hiding this comment.
The retained destination handles fix the parent-directory swap I reported: the new barrier path fails with Distribution destination parent changed, creates nothing externally, and the focused profile suites pass 150/150 (10 platform skips) at 19c57adecd5fe8cba0733b6a004d0dd79f5dfa3a. Ruff, py_compile, and diff check are clean.
The new commit path, however, reintroduces a source-leaf check/use gap at the final publication boundary. commit_owned_payload() records each prepared leaf's stat() while building sources, but _replace(source, name, destination) later verifies only the parent directory anchors before renaming the leaf. It does not bind or revalidate the leaf identity/type/content immediately around that rename.
I reproduced with a prepared SOUL.md containing APPROVED: after the initial source scan, immediately before _replace renamed it, I atomically replaced that leaf in the same prepared directory with a regular file containing UNAPPROVED. The directory handle and inode remained unchanged, commit_owned_payload() returned success, and the target contained UNAPPROVED. This violates the approved-snapshot guarantee even though the earlier staged-tree traversal is sound—the operation-owned publication tree is still writable until commit completes.
Please bind each prepared leaf to the identity validated during the source scan and revalidate it immediately before/after the handle-relative rename (or retain/open source leaf handles and publish from those). For regular files, identity alone also does not detect in-place content mutation; the prepared publication artifact needs to be sealed from writers or compared against the verified content identity used to materialize it. Add barriers for both same-parent atomic replacement and in-place overwrite between the initial scan and final rename, asserting the operation fails and the old target/manifest remain intact. The destination-parent finding itself is resolved.
Creating a profile named
mainreuses the default profile's session namespace. This change rejects that destination during creation, archive import, rename and distribution installation while preserving access to existingmainprofiles.Install plans retain source and destination identity. Publication revalidates those identities under the same lifecycle lock used by creation, import, rename and deletion. A removed, renamed, tombstoned or replaced destination cannot receive a stale plan. Downloads and interactive confirmation stay outside the lock, and a partially failed deletion can be retried.
Local sources are captured before planning with verified handles that reject links and concurrent changes. The manifest and destination are checked before copying payload, including refusal of staging inside the source or target profile. Each staged plan records the tree's names, modes and file hashes, and reads its manifest against that snapshot. The interactive CLI retains the same plan from preview through confirmation and application.
Publication validates and materializes the complete owned payload, including the resulting manifest, before changing installed files. It then replaces the disjoint entries with recoverable backups while keeping the profile root in place. A late validation failure leaves the destination unchanged. A publication failure rolls back prior replacements, the manifest and newly created directories. If rollback itself fails, the error identifies retained backups with their original relative paths. Destination parents stay bound to retained directory handles through publication, rollback and backup cleanup. POSIX operations use directory descriptors and verify ancestry around each rename; Windows holds native ancestry handles that reject reparse points and prevent parent replacement. If another process moves a directory on POSIX, rollback restores our changes through the original descriptor without undoing that process's rename. This coordinates Hermes lifecycle operations and provides recovery from exceptions; it does not promise isolation from external readers or recovery after power loss.
Distribution ownership, preserved configuration, credential templates and original source provenance retain their existing behavior. POSIX file and directory permission bits are preserved, including empty directories and an existing manifest omitted from the owned allowlist. Existing links to personal bootstrap directories remain usable without writing through them. Filesystem failures become controlled CLI errors, and cleanup removes only entries created by the operation. Documentation explains reserved names, the approved snapshot and publication recovery.
Fixes #17879.
Continues #17880 by @cola-runner, preserving the original commit and authorship.
Validation: two test functions exercise 108 scenarios with real files, synchronization barriers and native platform markers. The four focused profile suites passed 207 tests on macOS with Python 3.13.12 and native Linux with Python 3.11.15, with 12 platform skips on each. Ten partial publication regressions failed before the transaction correction on both macOS and native Windows. Four destination parent replacement regressions, including a readonly prior payload, fail on the previous publication commit and pass with retained directory handles. Two additional regressions reproduce a directory pin failure after successful creation and verify removal of the same empty directory, including backup setup. Tests cover late changes to the second and third owned entries, failures after replacements including the manifest, incomplete rollback recovery, existing personal directory links and permission preservation. Earlier regressions cover lifecycle races, source changes during staging, overlap cleanup and confirmation snapshot reuse. Ruff, compatibility checks, whitespace checks and independent review passed.
Native Windows with Python 3.11.14 passed all 102 applicable scenarios in the new test file, with six platform skips. The four suites totaled 207 passed, four preexisting failures and eight skipped. The existing failures concern two POSIX permission expectations and two shell alias expectations in
test_profiles.py; they were reproduced before this change and remain visible. Both additional tests against a temporary SMB share passed using native UNC and extended UNC paths, and share cleanup succeeded. A separate native handle probe confirmed that retained read handles with read/write sharing allow child publication while preventing parent replacement. Windows validation run.