fix(skills_sync): keep bundled skill copies writable on Nix store

[legacynode]

What does this PR do?

shutil.copy2 and shutil.copytree both preserve source-file mode bits by default. When bundled skills are sourced from /nix/store/.../share/hermes-agent/skills (mode 0444/0555 because the Nix store is read-only) — or any other read-only filesystem (squashfs, OCI image layer) — the user copy in ~/.hermes/skills/ inherits those bits and later edits via skill_manage, the curator, or even the shutil.rmtree(*.bak) cleanup at the end of an update fail with PermissionError.

The rmtree cleanup uses ignore_errors=True, so the failure is silently swallowed and stale *.bak directories accumulate over time.

This PR fixes the root cause at the copy boundary so user copies are always owner-writable, regardless of the source filesystem.

Related Issue

No tracking issue. Salvages closed PR #20135 (closed by author, not maintainer-rejected) and extends its coverage.

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

tools/skills_sync.py — three new helpers (PR #20135 architecture):

• _ensure_owner_writable(path) — adds the owner-write bit on a single file or directory without touching other mode bits (executable scripts keep +x).
• _copy_file_writable(src, dst) — drop-in replacement for shutil.copy2 that finishes with _ensure_owner_writable. Also serves as the copy_function= for shutil.copytree so per-file modes are corrected during the copy itself.
• _copytree_writable(src, dst) — drop-in replacement for shutil.copytree that uses _copy_file_writable per file, then sweeps the destination tree to grant the owner-write bit on directories (copytree reapplies source-directory metadata after files are copied, so the per-file override is not enough on its own).

Wrappers used at every call site that copies from a potentially read-only bundled source:

• tools/skills_sync.py — sync_skills (new + update paths), restore_official_optional_skill, the DESCRIPTION.md copy, and the reset_bundled_skill rmtree (which previously failed on user copies created by older Hermes builds that already inherited the bits). The update path also restores writability on the *.bak backup so the rmtree(backup, ignore_errors=True) cleanup succeeds rather than silently leaking the directory.
• hermes_cli/profiles.py — --clone and --clone-all flows. Cloning from a profile that itself still carries inherited read-only mode bits would otherwise propagate them to the new profile.
• hermes_cli/profile_distribution.py — apply_distribution. Distributed payloads can originate from the same read-only Nix store as the primary install.

AGENTS.md — new "Known Pitfalls" entry pointing future contributors at the helpers + the list of call sites that already use them.

How to Test

Reproduction

# 1. Mock a Nix-store-style read-only bundled tree
mkdir -p /tmp/ro-bundled/category/test-skill
echo "---\nname: test-skill\n---\n# Test\n" > /tmp/ro-bundled/category/test-skill/SKILL.md
chmod -R a-w /tmp/ro-bundled
chmod 0555 /tmp/ro-bundled /tmp/ro-bundled/category /tmp/ro-bundled/category/test-skill

# 2. Sync it (HERMES_BUNDLED_SKILLS pointing at the read-only tree)
HERMES_BUNDLED_SKILLS=/tmp/ro-bundled python -c "from tools.skills_sync import sync_skills; print(sync_skills())"

# 3. Try to edit the user copy — without the fix this raises PermissionError
echo "added line" >> ~/.hermes/skills/category/test-skill/SKILL.md

Tests

pytest tests/tools/test_skills_sync.py — 59 tests pass (50 pre-existing + 9 new):

• TestEnsureOwnerWritable — unit tests for the helper, including executable-bit preservation property and idempotency on already-writable targets.
• TestCopyFileWritable — verifies file copies from 0444 / 0555 sources end up writable while preserving +x.
• TestMakeTreeOwnerWritable — verifies the post-copytree directory sweep on a depth-first locked-down tree.
• TestCopytreeWritable — end-to-end: copy a Nix-store-style read-only source tree and assert a skill_manage-style append on a copied file succeeds (the regression: previously raised PermissionError).
• TestSyncSkillsReadOnlyBundledSource — full sync_skills run against a read-only fake bundled source, asserting the user copy is writable.

pytest tests/hermes_cli/test_profiles.py tests/hermes_cli/test_profile_distribution.py — 171 tests pass, no regressions in the profile flows touched by this PR.

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits (fix(skills_sync):)
• I searched for existing PRs — closest is fix(skills): make Nix store skill copies writable #20135, closed by author. This PR salvages the architecture from that PR and extends coverage.
• My PR contains only changes related to this fix (no unrelated commits)
• I've run scripts/run_tests.sh and the affected suites pass
• I've added tests for my changes (5 new test classes, 9 new tests)
• I've tested on my platform: NixOS WSL2 (where the bug actually manifests)

Documentation & Housekeeping

• I've updated AGENTS.md with a Known Pitfalls entry — N/A for cli-config.yaml.example
• N/A for tool descriptions/schemas (no tool behavior changed)
• Cross-platform impact considered: Linux (Nix store), macOS (squashfs / dmg-mounted bundled), Windows (no Nix, but OCI image layers are also read-only). Helpers are no-ops on already-writable sources.

Symptoms in the wild

This PR was developed against a NixOS WSL2 install with the services.hermes-agent module. Concrete user-visible failures the fix addresses:

• skill_manage write_file / patch fails with PermissionError on ~/.hermes/skills/<...>/.SKILL.md.tmp.<random>.
• 87+ *.bak directories accumulate in ~/.hermes/skills/ because the shutil.rmtree(backup, ignore_errors=True) cleanup silently swallows the inherited read-only mode bits.
• Every directory under ~/.hermes/skills/ ends up mode 0555 — multiplied across N profiles (a clone-driven layout with one Kanban-worker profile per role brings the total close to 2.5k unwritable directories).

[teknium1]

Heads up — the read-only skill-sync issue (#34860) was fixed on main via 83a7d0b + 8ae0802. _rmtree_writable now makes read-only files and directories (Nix-store r-xr-xr-x) writable before removal, and reset_bundled_skill deletes the user copy before dropping the manifest entry to avoid a limbo state. Your draft's keep-copies-writable-on-copy approach is no longer needed for that issue, but feel free to repurpose or close. Thanks for the early report.

[legacynode]

Thanks for the heads up — I've merged main and verified both fixes coexist cleanly (61 tests pass, 0 conflicts).

Your fix handles "how do we remove read-only trees". This PR handles "how do we prevent read-only trees from being created in the first place." They're complementary — yours treats the symptoms, this one prevents the disease.

You're right that 83a7d0b60 + 8ae0802d5 fix the removal side: _rmtree_writable now chmods files and parent dirs before rmtree, so .bak cleanup and reset_bundled_skill no longer silently fail.

This PR fixes a different problem — the copy side. I verified this on a live NixOS system:

Real Nix store file modes:

/nix/store/.../skills/                   → 555 (r-xr-xr-x)
/nix/store/.../skills/.../SKILL.md       → 444 (r--r--)

End-to-end test — skill_manage write_file simulation:

Scenario	write_file	patch	rmtree
Main (plain shutil.copytree)	❌ PermissionError	❌ PermissionError	✅
PR (_copytree_writable)	✅	✅	✅

After sync_skills from /nix/store, shutil.copytree still produces 444/0555 user copies on main:

# main: tools/skills_sync.py lines 474, 514, 551
shutil.copytree(skill_src, dest)   # inherits source modes
shutil.copy2(desc_md, dest_desc)   # inherits source modes

Those copies are then unwritable for skill_manage write_file / patch / curator edits. Same applies to hermes_cli/profiles.py (7 copy2/copytree calls) and hermes_cli/profile_distribution.py (3 calls).

[teknium1]

Thanks for isolating the copy-mode issue; the premise remains valid on current main: tools/skills_sync.py:609, :655, and :703 still use shutil.copytree/copy2.

Problems

• The wrappers only run for fresh or replacement copies. A previously installed read-only skill with an unchanged bundled hash reaches the no-update path at tools/skills_sync.py:642-685, so it stays unwritable. Please add a migration path and regression test for that state.
• The profile clone code has changed since this PR. b7192b1cb added symlinks=True at hermes_cli/profiles.py:1061 and :1099 to prevent recursive traversal. A post-copy chmod walk must not follow copied symlinks and modify their external targets.

Suggested changes

• Repair tracked, bundled-identical existing skill trees during sync, with a 0444/0555 upgrade regression test.
• Rework the profile repair traversal to skip symlinks or use lstat-safe handling, and test clone-all plus skills cloning with external links.

Automated hermes-sweeper review.

[legacynode]

Thanks for the detailed review — both points are addressed.

Problem 1 — pre-existing hash-identical read-only copies never reached

Confirmed: the "bundled unchanged, user unchanged" no-op branch (and the v1-migration branch that sets a baseline hash) never repaired an existing copy, since content parity means it never routes through _copytree_writable. Both branches now unconditionally sweep _make_tree_owner_writable(dest) before falling through to skipped. Added test_preexisting_hash_identical_readonly_copy_is_repaired, which pre-seeds a 0444/0555 user copy with a matching manifest hash and asserts the sync repairs it in place without re-copying (skipped, not copied/updated).

Problem 2 — symlink traversal in the repair sweep

Confirmed and reproduced for real, not just in the unit test: os.chmod follows symlinks by default, so a naive _make_tree_owner_writable walking a copytree(..., symlinks=True, ...) result would chmod through a copied symlink into whatever external file it still points at. I verified this concretely — built a real read-only target outside the profile, ran the old (unguarded) chmod logic against a symlink pointing at it, and watched its mode flip from 0555 to 0755. _ensure_owner_writable now skips path.is_symlink() entries; re-ran the same setup with the guard in place and the external target's mode is untouched (0555/0444, unchanged). Added test_clone_config_skills_with_external_symlink_does_not_chmod_target as the regression test, and a paragraph in the AGENTS.md pitfall entry documenting why the guard exists.

Re-verified end to end

Beyond the unit tests (305 passing via scripts/run_tests.sh, run both in a plain uv venv and inside nix develop), I reproduced the original bug against unpatched main with a real Nix-store derivation (nix-build, actual 0444/0555 mode bits, not a chmod simulation) piped through HERMES_BUNDLED_SKILLS: sync_skills() on main leaves the user copy at 0444, and a skill_manage-style append raises PermissionError exactly as described. Same setup against this branch: copy lands at 0644/0755, and the append succeeds.

[legacynode]

Rebased onto current main. Diffed against @alt-glitch's salvage (#69473) — tools/skills_sync.py, hermes_cli/profiles.py, hermes_cli/profile_distribution.py, and both touched test files are byte-identical; the only difference in AGENTS.md is unrelated main drift (kanban toolset docs, test-retry policy) that landed between the two salvage attempts, not a change to this fix's content. Confirms the salvage carried the fix over correctly with no regressions. Mergeable/clean again.