fix(skills): park a batch rollback outside the skills root

[yoyodine-industries]

What does this PR do?

A failed multi-op skill_manage batch parks the half-applied skill inside the skills
root while it copies the snapshot back (_restore_snapshot, tools/skill_manager_batch.py:
post_dir.with_name(post_dir.name + ".rollback-broken"), removed with
shutil.rmtree(aside, ignore_errors=True)).

That park is itself a skill:

• agent.skill_utils.iter_skill_index_files walks a root with os.walk(followlinks=True)
  and prunes only EXCLUDED_SKILL_DIRS, so a parked directory holding SKILL.md is
  offered as a skill — with the live skill's frontmatter name, so the name resolves
  twice in one root. A process killed between the park and the copy-back (SIGKILL, host
  restart, power cut) leaves it there for good, because nothing sweeps it.
• shutil.rmtree refuses a symlink, so when the entry is a directory symlink the park
  cannot be removed at all — ignore_errors=True swallowed the refusal and the link
  survived even the successful path.

This PR moves the park outside every skills root, removes it with a symlink-aware helper,
and sweeps parks older than a day.

Related Issue

Fixes #118691

Complementary to #112321 and #67748, not a duplicate of either: those fix which path
discovery returns and make snapshot/restore operate on the resolved link target. This
PR changes where the park lives — the two are independent, and they compose (with
either applied, the park becomes a real directory, but it is still parked inside the
skills root, so an interrupted restore still leaves a loadable shadow entry).
No files that #112321 touches are modified here.

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)
• ✨ New feature (non-breaking change that adds functionality)
• 🔒 Security fix
• 📝 Documentation update
• ✅ Tests (adding or improving test coverage)
• ♻️ Refactor (no behavior change)
• 🎯 New skill (bundled or hub)

Changes Made

• tools/skill_manager_batch.py
  • _restore_snapshot parks via the new _park_entry_aside and cleans up with
    _remove_path (symlink-aware) instead of shutil.rmtree, which cannot delete a
    symlink.
  • _park_entry_aside renames the entry into _asides_root() — a hidden
    .skill-rollback-asides directory beside the skills root (same filesystem, so the
    rename stays atomic; tempfile.gettempdir() when HERMES_HOME is itself configured
    as a skills root, so the park is never inside a walked tree) — and stamps the park's
    own mtime so the stale check is about the park, not the entry.
  • _sweep_stale_asides drops parks older than _ASIDE_STALE_SECONDS (24h): a park
    exists only between the rename and the copy-back, so anything that old was abandoned
    by a process that died mid-restore.
• tests/tools/test_skill_batch_rollback_park.py (new, 5 tests) — assertions use the
  loader's own walker (iter_skill_index_files), so "nothing loadable was left behind"
  is checked the way the reader side sees it.

How to Test

1. Base tree (change reverted): python -m pytest -q tests/tools/test_skill_batch_rollback_park.py
   → 4 failed, 1 passed, and on the same base tree
   python -m pytest -q tests/tools/test_skill_manage_batch.py → 9 passed (the delta
   is the new file). The one test that passes at base is the plain successful-rollback
   path; it is a regression guard.
2. This branch: python -m pytest -q tests/tools/test_skill_batch_rollback_park.py tests/tools/test_skill_manage_batch.py
   → 14 passed.
3. The interruption test SIGKILLs a child process that is between the park and the
   copy-back, then asserts the loader walk yields nothing under the root and exactly one
   park exists outside it.
4. scripts/run_tests_parallel.py -j 8 tests/*/test_skill*.py tests/*/test_skills*.py
   → 55 files, 883 passed, 0 failed, 9 skipped (macOS 26, Python 3.11). I ran this
   skill-related surface rather than the full suite.
5. python -m ruff check tools/skill_manager_batch.py tests/tools/test_skill_batch_rollback_park.py
   → clean.

Checklist

Code

• My commit messages follow Conventional Commits (fix(scope):)
• I searched for existing PRs — fix(skills): skill_manage follows directory symlinks; rollback and ownership stay on the link #112321 and fix(skills): follow symlinks in skill management #67748 cover the discovery/resolve halves; this is the park-location half (see Related Issue)
• My PR contains only changes related to this fix (no unrelated commits)
• I've run the related suite (see How to Test 2–4): 883 passed / 0 failed on the skill-related surface
• I've added tests for my changes (required for bug fixes)
• I've tested on my platform: macOS 26 (Darwin 26.5), Python 3.11

Documentation & Housekeeping

• I've updated relevant documentation (docstrings) — docstrings on the new/changed
  helpers explain why the park must not live inside a root; no user-facing docs
  describe this path
• I've updated cli-config.yaml.example if I added/changed config keys — N/A
• I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A
• I've considered cross-platform impact (Windows, macOS) per the
  compatibility guide —
  the symlink test skips where directory symlinks are unavailable, and the kill test
  asserts the platform-independent outcomes (−SIGKILL only on POSIX)
• I've updated tool descriptions/schemas if I changed tool behavior — N/A (the tool's
  reported behaviour is unchanged: a failed batch still rolls every touched skill back)

🤖 Generated with Hermes Agent (AI-authored, test-first;
every claim in "How to Test" was run on this branch)

[arkheioncorp]

APPROVE

Security scan: Clean.

Logic review: Two real bugs fixed: (1) shutil.rmtree refuses symlinks, so a symlinked park could never be removed; _remove_path now dispatches on is_symlink(). (2) Parks were created inside the skills root as siblings, making them loadable skills; _park_entry_aside now places them under .skill-rollback-asides off-root. Stale park sweep runs on every rollback. Failures in _remove_path are logged but not retried — acceptable for a rollback cleanup path.

Suggestions: _asides_root() catches all exceptions and falls back to tempfile.gettempdir() silently. If the skills-root check itself is what is failing (e.g. a resolver bug), the park still lands safely, but the root cause is masked. Consider logging the fallback at WARNING level.

[JoaoMarcos44]

Exact-head adversarial review at 2fbc6a3a13f509b1bd2fc2a0f3e0d4935cbbef85. The same-filesystem rename path closes the reported loadable-shadow failure, and symlink-aware cleanup is correct. I found one blocking portability/integrity gap in the explicitly supported off-tree fallback: when _asides_root() resolves onto another filesystem, shutil.move is copy-then-delete, so the operation loses the atomic evacuation property the crash-safety argument depends on. Details inline.

[JoaoMarcos44]

P1 BLOCKER — the cross-filesystem fallback reintroduces a loadable in-root entry during the crash window.

REPRODUCTION: configure the layout that makes _asides_root() return tempfile.gettempdir() (the code explicitly does this when the normal aside base would itself be under a walked skills root), with that temp directory mounted on a different filesystem from the skill. path.rename(aside) raises EXDEV, so this branch calls shutil.move. For a directory across filesystems, shutil.move copies the tree to aside and removes the source only after the copy succeeds. Kill/power-fail during that copy: the original skill still exists inside the walked root and is loadable, while a partial park exists off-tree. The exact invariant this PR advertises — interruption after parking leaves nothing loadable inside the root — is therefore false on the fallback path.

DIAGNOSIS: crash safety here depends on an atomic namespace move. Cross-device copy+delete cannot provide that invariant. The current interruption regression uses one temp hierarchy, so source and aside normally share a filesystem and never exercises EXDEV.

FIX: choose an aside directory guaranteed to be on the source filesystem but outside every loader root (derive a safe sibling from the source mount/root topology), or fail closed before moving when no same-filesystem non-loadable location can be established. Do not describe shutil.move as a safe equivalent for the crash window.

REGRESSION: force Path.rename to raise EXDEV (or use two test mounts), pause/kill during the fallback copy, and assert the loader cannot observe a loadable source entry. If that guarantee is impossible cross-device, assert rollback aborts without beginning the park and reports the limitation rather than entering a non-atomic half-move.

[yoyodine-industries]

Confirmed. The full disposition, the fix and the regression shape are in my comment on this PR. Short version: the gap is real because rename is atomic and shutil.move across filesystems is not, the fix is a same-filesystem aside outside every loader root or a fail-closed abort before any move, and the regression forces EXDEV and kills mid-copy.

[yoyodine-industries]

Accepted — the finding is real and the description cannot stay as it is.

path.rename() is what makes the evacuation atomic; shutil.move across filesystems is copy-then-delete, so on the explicitly supported off-tree fallback the interruption window you describe puts the original skill back inside the walked root and loadable while a partial park sits off-tree. That is exactly the invariant the PR advertises, so the fallback contradicts the change's own claim.

Fix as you specify: derive the aside from the source's own mount/root topology so it is on the source filesystem and outside every loader root, and where no such location can be established, fail closed before any move rather than entering a non-atomic half-move — and stop describing shutil.move as a safe equivalent for the crash window.

Regression: force Path.rename to raise EXDEV (or use two mounts), kill during the fallback copy, and assert the loader cannot observe a loadable in-root source entry; where the cross-device guarantee is impossible, assert the rollback aborts before beginning the park and reports the limitation. The current interruption test only ever exercises one temp hierarchy, so it can never reach this path.

arkheioncorp's APPROVE holds for the same-filesystem path, which is the path you also verified; this blocker is the other one. A change follows on this branch and I will post the head sha with it.

[yoyodine-industries]

Landed on this branch as a7f571b060 — the change you blocked on, per the disposition above.

The cross-device fallback is gone rather than softened. _park_dir_for() now derives the park from the entry's own root topology: the .skill-rollback-asides sibling of each ancestor, climbing outward from the root that holds the entry, and takes the first candidate that is (a) outside every loader root and (b) on the entry's own filesystem (st_dev, compared against the nearest existing ancestor of the candidate and re-checked after the mkdir). The tempdir fallback is removed — it was selected exactly when the local base is itself walked, and it is on the entry's filesystem only by luck, which is where the non-atomic path lived.

Where no candidate qualifies, the park raises BEFORE the first move. The rollback for that skill then fails closed: ROLLBACK FAILED ...; snapshot preserved at ..., naming each candidate it rejected and why, and the batch keeps that snapshot for hand recovery. No copy is attempted at any point — shutil.move is out of this path entirely, and the module comment no longer describes it as a safe equivalent for the crash window.

Containment is compared on real paths, so a root reached through a symlink (/var vs /private/var, a linked profile home) is recognised as the same directory the loader walks instead of being missed on a spelling mismatch.

Regression, in the shape you asked for (tests/tools/test_skill_batch_rollback_park.py):

• test_a_cross_device_park_is_refused_before_anything_moves — rename forced to raise EXDEV, with shutil.move patched to fail loudly: the park refuses, the entry stays in the root byte-identical, nothing is parked. Against the previous head this test fails by reaching the copy path, so it pins the blocker and not just the new helper.
• test_a_walked_park_base_yields_to_a_same_device_ancestor_not_a_tempdir — with the local base itself walked, the park climbs one ancestor out and the assertion is that it landed on the entry's own st_dev.
• test_no_qualifying_park_dir_aborts_before_the_move_and_reports_it — every candidate off-device: the batch aborts with the entry untouched, the failure text names the limitation, and the snapshot is kept.

Verified: 8/8 in that file; 91/91 across tests/tools/test_skill_manage_batch.py, tests/tools/test_skill_manager_tool.py, tests/agent/test_skill_applied_notifications.py and that file; 952 passed across the skill-themed sweep of tests/tools + tests/agent with the same 6 pre-existing environment failures as the unpatched head (proven by running the sweep both ways); ruff clean. One fixture change was needed to make the claim testable rather than decorative: HERMES_HOME in that test now sits a level below the temp dir, so "the ancestor out" and "the tempdir fallback" are distinguishable directories — a home directly inside TMPDIR cannot tell them apart, which is part of why the old path was never exercised.

This head is the reviewed head plus the one fix, so the exact-head review you ran stays applicable; the branch is still behind upstream main by its own merge and will pick that up as usual.