provision_worktree crashes on a dangling symlink in a skill tree

[pbean]

Found while reviewing PR #406 (the skills seed gate). Not a regression from that PR — the copy loop is byte-identical on release/0.9.x.

What happens

provision_worktree's BASE_SKILLS copy loop (src/bmad_loop/install.py) skips a destination that already exists:

for skill in BASE_SKILLS:
    dst = tree_dir / skill
    if dst.exists():
        continue
    src = (repo_root / tree / skill).resolve()
    if not src.is_relative_to(repo_root) or not src.is_dir():
        continue
    _copy_traversable(src, dst)

Path.exists() follows symlinks, so a dangling symlink answers False — but the path is still occupied, and the copy then raises.

Measured

Worktree carries a tracked dangling symlink at <tree>/bmad-review; the repo has a real dir there:

dst.exists() on dangling symlink -> False | is_symlink -> True
provision_worktree -> RAISED FileExistsError: [Errno 17] File exists: .../wt/.claude/skills/bmad-review

Why it matters

Provisioning has no failure vocabulary of its own — every containment violation in that loop is a bare continue, and the two seed-shortfall conditions report through the worktree-seed-skipped channel so the engine can decide escalate-vs-defer. An unhandled FileExistsError bypasses all of it and takes the run out through a path the state machine never sees.

The trigger is ordinary: a committed symlink whose target is only present on the machine that authored it (a shared BMad install that moved) is dangling in every other checkout, and git stores and checks out the dangling link happily.

Shape of a fix

Test dst.is_symlink() or dst.exists() rather than dst.exists() alone, and decide deliberately which side of the skip a dangling link belongs on. Note the same .exists() reasoning is used for MODULE_SKILLS one loop above.

Related: the seed-shortfall reporting in that loop is base_skills_seed_incomplete in the same file.

[coderabbitai]

🔗 Related PRs

#130 - fix: preserve seed file modes + pause on verify env faults (rc 126/127) [merged]
#233 - fix: report worktree_seed entries skipped as no-ops [merged]
#257 - feat(worktree): seed the absent children of an existing seed dir (#235) [merged]
#283 - fix(validate): accept merged bmad-review, drop unreleased verification-gap requirement (#260) [merged]
#373 - fix(install): guard the worktree exclude's filesystem tail (#359) [merged]

---

📝 Issue Planner

_{Check the box below or use the @coderabbitai plan command to generate an implementation plan and prompts that you can use with your favorite coding assistant.}

• Create Plan

---

🧪 Issue enrichment is currently in open beta.

You can configure auto-planning by selecting labels in the issue_enrichment configuration.

To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

issue_enrichment:
  auto_enrich:
    enabled: false

💬 Have feedback or questions? Drop into our discord!

[pbean]

Behaviour change from PR #406 — flagging it here rather than letting a filed issue
change shape silently. The issue stays open; one of its two shapes is converted, the
other still raises.

The BASE_SKILLS copy loop quoted above is gone. It is now a per-FILE merge
(_merge_traversable) with a destination-containment leg, so the dangling-symlink
question is asked one level down — of the leaf, not the directory. Measured on the new
code, both shapes, worktree carrying a dangling symlink at <tree>/bmad-review while
the repo has a real dir there:

The shape this issue measured — the link points OUTSIDE the worktree (a shared BMad
install that moved, which is the trigger the issue names):

provision_worktree -> ['.claude/skills/bmad-review']     # was: RAISED FileExistsError
escaped outside the worktree? -> False
base_skills_seed_incomplete(...) -> ['.claude/skills/bmad-review']

dst.resolve() lands outside the worktree, so the leaf returns without writing and
without raising, and the completeness gate then reports the skill through the
worktree-seed-skipped channel — the structured path the issue says is bypassed. The
engine escalates and pauses before dispatch instead of the run going out through an
unhandled FileExistsError.

The sibling shape — the link dangles but points INSIDE the worktree — still raises:

provision_worktree -> RAISED FileExistsError: [Errno 17] File exists:
    .../wt/.claude/skills/bmad-review

Containment passes, dst.exists() is False (it follows the link), and the raise
moves from dst.mkdir() in _copy_traversable to dst.parent.mkdir() in
_merge_traversable. Same defect, same lack of a failure vocabulary, one frame over.

So the fix shape the issue proposes — test dst.is_symlink() or dst.exists(), and
decide deliberately which side of the skip a dangling link belongs on — still applies,
now at the leaf in _merge_traversable rather than at the directory. The
MODULE_SKILLS loop noted at the end of the issue shares the same helper, so it is one
decision, not two.