fix(platform): overwrite-safe deferred stash + ref-safe unit branch names

[pbean]

Closes #101, Closes #102

Both issues came out of the consumer audit of PR #98 — the win32 hardening pass that added platform_util.atomic_replace and safe_segment. That audit asked a narrow question of the merged code: for every site #98 hardened, is every consumer of that site hardened too? Two gaps fell out. #98 swept the package's rename-over sites but skipped the one whose target is a possibly-existing file rather than a fresh temp-rename (the deferred-artifact stash), and it sanitized the unit worktree directory while leaving the unit branch built from the raw key — hardening the dir/branch pair inconsistently, which reads as a false sense of safety. This PR closes both.

#101 — deferred stash

_stash_deferred_artifacts moved a re-deferred story's spec straight over the prior stash with shutil.move. Now it stages a copy2 inside the destination dir and routes through atomic_replace.

The issue's stated mechanism is wrong and the PR does not reproduce it. #101 claims shutil.move "raises FileExistsError" on Windows when the target exists. It does not: move wraps os.rename in a bare except OSError and falls back to copy2 + unlink, which overwrites. Verified against the CPython source and by emulating Windows rename semantics. Two real hazards ride on that fallback instead, and they are what this change fixes:

1. Sharing violation. An AV/indexer handle on the target makes os.rename fail with WinError 5/32; move falls back to copy2, which opens the same locked target and fails too, propagating PermissionError and crashing the defer flow. atomic_replace retries the replace until the handle clears.
2. Non-atomicity. The copy2 fallback truncates the live target before writing, so a crash mid-copy leaves a torn stash. Stage-then-replace never exposes a partial file.

Both halves of the move are retried, not just the replace (added in review): Windows denies a delete against an open handle exactly as it denies a rename-over, so an unretried spec_path.unlink() would fail after the stash had already landed — and _defer calls this before the rollback, the story-deferred journal append, and _save(), so that raise aborts the entire deferral. atomic_replace's backoff is extracted into _retry_on_sharing_violation and reused by a new platform_util.retrying_unlink. Suppressing the unlink instead would silently leave the stale in-review spec this function exists to remove.

Test consequences worth flagging for review: the plan's suggested "pre-create the dest file → stash replaces it" test is a characterization test on every platform, not a regression guard — old code passes it too. The two tests that actually fail on shutil.move are test_stash_deferred_artifacts_survives_a_win32_sharing_violation (simulated WinError 32, asserts the retry lands) and ..._keeps_source_and_cleans_tmp_on_replace_failure (source survives a failed replace; no .tmp residue). Both were confirmed red against the pre-change body.

#102 — ref-safe branch names

New platform_util.safe_ref_segment, applied to both segments of unit_branch_name. Same contract as safe_segment — identity for clean input, -<hex8> digest suffix whenever anything changed, never raises, pure Python (git can only validate, not sanitize) — but on git's alphabet, which is disjoint from Windows' in both directions: CON is a legal ref and an illegal filename; a..b is the reverse. A consumer deriving both a directory and a branch from one key must therefore run both sanitizers, and the module docstring says so.

Sanitizes control chars/space/DEL and ~^:?*[\/ → _, .. → __, @{ → _{, leading . → _, lone @ → _, caps length. Trailing . and trailing .lock are ref-illegal but get no rewrite — flagging them un-clean routes them to the digest path, and the -<hex8> suffix is itself the fix. A leading - is deliberately preserved: it is legal in a component, and the porcelain's separate "must not start with -" check reads the whole name, which callers always prefix.

Unlike #101, this issue's mechanism reproduces exactly as written — reverting unit_branch_name fails the new test with fatal: 'bmad-loop/test-run/story/1:2..3@{now}.lock' is not a valid branch name.

Oracle test. git check-ref-format --branch is run over a 42-entry corpus (clean keys, one entry per coercion rule, adversarial combinations like story/1:2..3@{now}.lock, ...., ..lock, a.lock.lock, @{u}, 500 chars), in each of the three positions unit_branch_name places a segment — bmad-loop/rid/{}, bmad-loop/{}/1-1-a, bmad-loop/{}. Pure-Python sanitization is only as good as its agreement with git; this pins it. Plus an end-to-end test where real git mounts a worktree for a dirty unit key.

The clean-key branch-name tests (test_branch_per_story_naming, test_branch_per_run_naming) are unchanged and still assert byte-identical names.

Verification

• .venv/bin/python -m pytest — 1922 passed, 1 skipped.
• Full trunk check (no filter) — clean.
• New tests confirmed red against the pre-change implementations for both fixes.
• Windows: _stash_deferred_artifacts shutil.move raises FileExistsError when a story re-defers #101's target platform is Windows; the windows CI leg is the one that matters here.

Summary by CodeRabbit

• Bug Fixes
  • Worktree and branch creation now handle run IDs and unit keys with special characters more reliably, preventing invalid branch-name failures.
  • Deferred artifact storage is now safer and more resilient, reducing the risk of corrupted or partial copies during interruptions and on Windows.

[coderabbitai]

Caution

Review failed

An error occurred during the review process. Please try again later.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/deferred-stash-and-ref-safe-branches

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/bmad_loop/engine.py`:
- Around line 2337-2374: The source cleanup in _stash_deferred_artifacts still
unlinks spec_path unguarded after atomic_replace succeeds, so a Windows sharing
violation can escape and fail the run even though the deferred artifact was
already stashed. Update _stash_deferred_artifacts to treat spec_path.unlink() as
best-effort cleanup like the tmp unlink path: wrap it in the same
suppressed/retry handling or convert it to a non-fatal logged cleanup failure,
while keeping the successful stash and journal append behavior intact.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: be3f9c3c-84f9-4639-a9e3-bffe3d7bb5fb

📥 Commits

Reviewing files that changed from the base of the PR and between 975c044 and 97dd818.

📒 Files selected for processing (7)

• CHANGELOG.md
• src/bmad_loop/engine.py
• src/bmad_loop/platform_util.py
• src/bmad_loop/workspace.py
• tests/test_engine.py
• tests/test_engine_worktree.py
• tests/test_platform_util.py

[pbean]

@coderabbitai Addressed in f2b965a, though with a different remedy than suggested — worth writing down why.

The finding lands: after atomic_replace succeeds, spec_path.unlink() was unretried, and Windows denies a delete against an open handle exactly as it denies a rename-over. So the staged move was half-hardened. It matters more than "the run fails" suggests: _defer calls _stash_deferred_artifacts before _rollback_or_pause, before the story-deferred journal append, and before _save() (engine.py:2313 vs :2318-2334) — a raise there aborts the entire deferral, not just the stash.

One correction to the framing: this is not a regression the PR introduces. shutil.move's own Windows path is copy2(src, dst) + os.unlink(src), with that unlink equally unguarded, so old code could already land the copy and then die on the delete. The PR neither created nor fixed that — it just made the omission incoherent, since the function now advertises win32 hardening.

I did not take the suggested fix (suppress / log the unlink failure). Silently leaving the source spec in place recreates precisely the condition this function exists to prevent — "a leftover in-review spec would confuse the next attempt" — and it would do so quietly, on Windows only, at the exact moment a human is already being asked to look at a deferral. Trading a loud crash for a silent stale artifact is a bad trade here.

Instead: atomic_replace's win32 backoff is extracted into _retry_on_sharing_violation and reused by a new platform_util.retrying_unlink, so both halves of the move ride out the same transient lock. A permanent failure still propagates, preserving today's semantics. Replace-then-unlink ordering is now documented as deliberate — a failure must be able to leave a duplicate spec, never a hole where the work was.

Covered by test_stash_deferred_artifacts_retries_a_locked_source_spec (WinError 32 on the source delete, retried, stash intact) plus retrying_unlink unit tests for the retry, the POSIX no-retry path, and non-sharing-violation errors propagating unswallowed. All confirmed red against a plain spec_path.unlink().

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail
git log --oneline -5
git show f2b965a --stat || true
git show f2b965a -- src/bmad_loop/platform_util.py | head -150

Length of output: 6714

---

@pbean Confirmed — f2b965a extracts _retry_on_sharing_violation out of atomic_replace and reuses it for the new retrying_unlink, applied to spec_path.unlink() in the stash flow. The reasoning holds up:

• Suppressing/logging the unlink failure would have silently reintroduced the exact leftover-spec hazard the function exists to prevent, just gated to Windows and to the worst possible moment (mid-deferral, pre-rollback, pre-journal-append). Loud failure-with-retry is the right tradeoff over silent staleness.
• Your correction on framing is fair — shutil.move's own POSIX-fallback path (copy2 + os.unlink) had the identical unguarded unlink, so this was a pre-existing gap that #103 merely made inconsistent with its own stated hardening, not one it introduced.
• Replace-then-unlink ordering being documented as deliberate (duplicate-over-hole) is the correct invariant to pin down explicitly, since it's exactly the kind of ordering assumption that's easy to accidentally invert during a future refactor.
• Sharing the backoff between atomic_replace and retrying_unlink via _retry_on_sharing_violation avoids duplicating the win32-gating/jitter logic, and keeps both halves of the move on the same retry contract.

Nice catch and a solid remedy. Thanks for writing down the rationale.

---

✏️ Learnings added

Learnt from: pbean
URL: https://github.com/bmad-code-org/bmad-loop/pull/103

Timestamp: 2026-07-09T17:18:57.796Z
Learning: In bmad-loop's src/bmad_loop/platform_util.py, `atomic_replace` (used for win32-safe target replacement) and `retrying_unlink` (used for deleting the source file after a staged move) both delegate to a shared `_retry_on_sharing_violation(op)` helper that retries only on Windows sharing-violation-style errors (PermissionError or winerror 5/32), with jittered exponential backoff, gated to sys.platform == "win32". In `Engine._stash_deferred_artifacts` (src/bmad_loop/engine.py), the deliberate ordering is: atomic_replace the staged copy into place first, then retrying_unlink the source spec — so a failure can leave a duplicate spec file, but never a hole where the work was. Do not suggest suppressing/logging failures from `retrying_unlink`/`atomic_replace` in this codebase; that would silently leave a stale in-review spec, which is the exact failure mode `_stash_deferred_artifacts` is meant to prevent. A permanent (non-transient) failure must still propagate and abort the deferral.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.