fix(skills): skill_manage follows directory symlinks; rollback and ownership stay on the link

[banozz0]

What does this PR do?

A skill installed as a directory symlink under skills.external_dirs (<root>/<skill> -> ../_src/<skill>, the layout a shared-skills tree uses across profiles) is visible to skill_view — iter_skill_index_files walks with followlinks=True — but skill_manage cannot patch it: _iter_skill_dirs used Path.rglob, which never descends into a linked directory. Same defect and same substitution as #67748, rebased onto the _iter_skill_dirs helper current main has (neither hunk of #67748 applies any more), plus two safeguards that only become reachable once discovery works.

Three changes, one seam (skill_manage(operations=[...])):

1. Discovery — _iter_skill_dirs walks with the shared iter_skill_index_files, so skill_manage resolves exactly what skill_view resolves and returns the lexical alias path. This also means skill_manage now honours the walker's _org mirror gating and support-dir pruning, which rglob did not — that is the module's documented contract, and it is what the reader side already does.
2. Batch rollback — snapshot/restore operate on the resolved link target. Before: _restore_snapshot renamed the alias aside and copytree'd the snapshot into a real directory at the alias, detaching the skill from its shared source; rmtree(aside) on a symlink is a swallowed error, so a .rollback-broken link was left behind too. A failed multi-op batch left the first op applied in the shared source while reporting a full rollback. A batch-created skill is still removed at its lexical path, so a pre-existing alias there is refused (loud rollback failure) rather than followed.
3. Ownership — is_external_skill_path checks lexical containment before resolved containment. Before: the alias resolved outside the external root → "not external" → with a curator-managed record and the read mark set, the background curator wrote to the shared source. The new predicate is a strict superset of the old one (it can only add Trues), and every caller treats "external" as read-only / ineligible.

Related Issue

Fixes #54195. Extends #67748 (see its discussion for the rollback and ownership analysis) rather than duplicating it; happy to fold this into that PR if one head is preferred.

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_tool.py — _iter_skill_dirs: Path.rglob → iter_skill_index_files (+3/−2).
• tools/skill_manager_batch.py — _real_skill_dir; _snapshot_skills and _rollback snapshot/restore on the resolved target; batch-created skills removed at the lexical path.
• agent/skill_utils.py — _lexical_for_skill_ownership; is_external_skill_path checks lexical, then resolved containment.
• tests/tools/test_skill_manager_tool.py — TestSymlinkedExternalSkill (5 tests) through the public skill_manage(operations=[...]) call: in-place patch with the alias preserved · failed single-op batch keeps the symlink and source · failed multi-op batch undoes the first op in the shared source · background review refuses the linked skill as externally owned · failed create-through-alias batch never deletes the link target. test_symlinked_skill_dir_refused (symlink delete refusal) is unchanged and still passes.

How to Test

1. On current main, apply only the test file: scripts/run_tests.sh tests/tools/test_skill_manager_tool.py -k TestSymlinkedExternalSkill → 3 failed, 2 passed (the two rollback tests only turn red once discovery works; with the discovery hunk alone applied it is 4 failed, 1 passed).
2. With this branch: the same command → 5 passed.
3. scripts/run_tests.sh tests/tools/test_skill_manager_tool.py tests/tools/test_skill_manage_batch.py tests/agent/test_skill_utils.py tests/agent/test_project_skills.py tests/agent/test_org_skill_namespace.py tests/tools/test_skill_usage.py tests/tools/test_skills_sync_client.py tests/tools/test_skills_tool.py (and the other 14 files that touch these seams) → 536 passed.
4. Real-world repro: point skills.external_dirs at a directory whose entries are symlinks to skill folders elsewhere, then skill_manage(operations=[{"name": "<skill>", "action": "patch", ...}]) — "not found" before, patched in place after, and the entry is still a symlink.

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
• I searched for existing PRs to make sure this isn't a duplicate
• My PR contains only changes related to this fix/feature (no unrelated commits)
• I've run the suite via scripts/run_tests.sh (macOS, Python 3.11): 48,959 passed / 287 failed / 632 skipped. Every failure is environmental and reproduces on pristine main in the same venv: acp and anthropic optional deps absent (collection errors), load-sensitive tests/tools/test_search_* cases that flip between runs, and a transient aiohttp import gap during the heavy 16-worker run (aiohttp is installed; the same files collect and run on re-run). Re-running exactly the 149 failing files on main vs this branch gives the same failure set apart from those search flips (155 vs 152). The 22 test files touching the changed seams: 536/536 green.
• I've added tests for my changes (required for bug fixes, strongly encouraged for features)
• I've tested on my platform: macOS 26 (Darwin 25.6), Python 3.11.15

Documentation & Housekeeping

• I've updated relevant documentation (README, docs/, docstrings) — docstrings on the three changed functions; 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 tests pytest.skip where directory symlinks are unavailable, matching the pattern in fix(skills): follow symlinks in skill management #67748
• I've updated tool descriptions/schemas if I changed tool behavior — N/A (behaviour matches the tool's existing description: "Existing skills are modified wherever they live")

🤖 Generated with Claude Code

https://claude.ai/code/session_01Dnv1xcpg2T2LGP4isptHAB

[yoyodine-industries]

Hi — related, and I think complementary rather than overlapping: this PR changes where the
rollback's park lives, not what discovery returns or which path snapshot/restore operate on.

_restore_snapshot parks the half-applied skill as a sibling of the entry
(<name>.rollback-broken, tools/skill_manager_batch.py), i.e. inside the skills root, and
removes it with shutil.rmtree(aside, ignore_errors=True). Independently of the alias-vs-target
question:

• iter_skill_index_files walks a root with os.walk(followlinks=True) and prunes only
  EXCLUDED_SKILL_DIRS, so a park left in the root is offered as a skill; if it declares the
  live skill's frontmatter name, that name resolves twice in one root.
• shutil.rmtree refuses a symlink ("Cannot call rmtree on a symbolic link") and
  ignore_errors=True swallows that — so a park that is a link is never removed, on the
  success path or the exception path.

With this PR's hunks applied the park becomes a real directory (good), but it is still parked
inside the skills root, so a process killed between the park and the copy-back leaves a loadable
shadow entry that nothing sweeps. Opened #118691 / #118692 for that half (park beside the root
on the same filesystem, symlink-aware removal, sweep of parks older than a day), with a test
that SIGKILLs a child mid-restore and asserts the loader walk yields nothing under the root.

No files touched here overlap with this PR (_snapshot_skills / _rollback are untouched;
the change is _restore_snapshot + helpers), so they should apply together — happy to fold it
in if a single head is preferred.