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

CHANGELOG.md

@@ -31,6 +31,21 @@ breaking changes may land in a minor release.
 
 ### Fixed
 
+- **Unit keys with git-ref-illegal characters no longer break worktree runs.** `unit_branch_name`
+  built `bmad-loop/<run_id>/<unit_key>` from the raw ids, so a key or `--run-id` carrying `:`, `..`,
+  `@{`, a space or a trailing `.lock` cleared the (already-sanitized) worktree dir only to die at
+  `git worktree add` with _"is not a valid branch name"_. Both segments now go through a new
+  `platform_util.safe_ref_segment` — identity for clean ids, `-<hex8>` digest suffix otherwise, on
+  git's alphabet rather than Windows' (`CON` is a legal ref; `a..b` is a legal filename). A
+  `git check-ref-format` oracle test pins the agreement; the `attempt-preserve` recovery-ref slugs
+  now reuse the same sanitizer instead of their own inline one. (closes #102)
+- **The deferred-artifact stash overwrites its target atomically.** A story deferring a second time
+  re-stashes the same spec filename over the previous one. `shutil.move` fell back to a non-atomic
+  `copy2` there — which tears the stash on a mid-copy crash and fails outright on Windows when an
+  AV/indexer handle turns the rename into a sharing violation. The stash now stages a copy inside the
+  destination dir and routes through `platform_util.atomic_replace`, inheriting its win32 retry; the
+  source removal gets the same retry via a new `platform_util.retrying_unlink`, since Windows denies a
+  delete against an open handle exactly as it denies a rename-over. (closes #101)
 - **A finished session whose final `Stop` hook was lost no longer loses its work.** A dev/review
   session that wrote its terminal spec but never delivered the `Stop` ended `stalled` — or `timeout`,
   when hooks were misconfigured and no event ever arrived — and the on-disk result was discarded.

src/bmad_loop/engine.py

@@ -39,7 +39,7 @@
     SessionRecord,
     StoryTask,
 )
-from .platform_util import safe_segment
+from .platform_util import atomic_replace, retrying_unlink, safe_ref_segment, safe_segment
 from .plugins import HookBus, HookContext, PluginRegistry
 from .policy import Policy
 from .runs import kill_session
@@ -978,11 +978,12 @@ def _preserve_attempt_commits(self, task: StoryTask, *, allow_pause: bool) -> No
         if not commits:
             return
         head = verify.rev_parse_head(self.workspace.root)  # the tip the recovery ref parks at
-        # run_id can be an arbitrary user `--run-id`; keep the ref component git-safe
-        # and length-bounded so an exotic/overlong id can't blow the ref-name limit,
-        # fail `git branch`, and drop the recovery ref (which on a re-drive would then
-        # reset past the work anyway).
-        slug = "".join(c if (c.isalnum() or c in "_-") else "-" for c in self.state.run_id)[:64]
+        # run_id can be an arbitrary user `--run-id`; ref-sanitize it (same
+        # identity-for-clean-ids / digest-for-dirty contract as the unit branches) so
+        # an exotic/overlong id can't blow the ref-name limit, fail `git branch`, and
+        # drop the recovery ref (which on a re-drive would then reset past the work
+        # anyway).
+        slug = safe_ref_segment(self.state.run_id)
         try:
             ref = verify.preserve_commits(
                 self.workspace.root,
@@ -1017,9 +1018,9 @@ def _preserve_attempt_worktree(self, task: StoryTask) -> None:
         baseline = task.baseline_commit
         if not baseline:
             return
-        # Same git-safe, length-bounded slug as _preserve_attempt_commits so an
-        # exotic/overlong --run-id can't blow the ref-name limit and drop the ref.
-        slug = "".join(c if (c.isalnum() or c in "_-") else "-" for c in self.state.run_id)[:64]
+        # Same ref-sanitized slug as _preserve_attempt_commits so an exotic/overlong
+        # --run-id can't blow the ref-name limit and drop the ref.
+        slug = safe_ref_segment(self.state.run_id)
         # ``baseline_commit`` is fixed across the whole dev retry loop, so keying the
         # ref on the baseline alone would make a 2nd dirty rollback reuse the name and
         # orphan the 1st attempt's snapshot. ``task.attempt`` only ever increments
@@ -2337,19 +2338,47 @@ def _defer(self, task: StoryTask, reason: str) -> None:
     def _stash_deferred_artifacts(self, task: StoryTask) -> None:
         """Move the deferred story's spec out of the artifacts dir into the run
         dir: a leftover in-review spec would confuse the next attempt, but the
-        work in it is worth keeping for the human."""
+        work in it is worth keeping for the human.
+
+        A story that defers twice re-stashes the same filename, so the target may
+        exist. `shutil.move` survived that on Windows only by accident: `os.rename`
+        raises FileExistsError over an existing target, `move` catches *any* OSError
+        and falls back to `copy2` + `unlink`. Two real hazards ride on that fallback
+        (#101) — it re-fails outright when an AV/indexer handle turns the rename into
+        a sharing violation (WinError 5/32) and `copy2` then cannot open the same
+        locked target, and it is non-atomic, so a crash mid-copy leaves a truncated
+        stash. Staging a copy inside `dest` and `atomic_replace`-ing it onto the
+        target overwrites in one step, carries #98's win32 retry, and — because the
+        staging copy lives in `dest` — keeps the replace same-filesystem, preserving
+        `shutil.move`'s cross-device tolerance.
+
+        Both halves of the move are retried: Windows denies a delete against an open
+        handle just as it denies a rename-over, so an unretried `unlink` would fail
+        the run on the very hazard the replace now rides out. The order is
+        replace-then-unlink because `_defer` calls this before the rollback and the
+        `story-deferred` journal append — a failure here aborts the deferral, so it
+        must be able to leave a duplicate spec, never a hole where the work was."""
         if not task.spec_file:
             return
         spec_path = Path(task.spec_file)
         if not spec_path.is_file():
             return
         dest = self.run_dir / "deferred" / safe_segment(task.story_key)
         dest.mkdir(parents=True, exist_ok=True)
-        shutil.move(str(spec_path), str(dest / spec_path.name))
+        target = dest / spec_path.name
+        tmp = dest / (spec_path.name + ".tmp")
+        shutil.copy2(spec_path, tmp)
+        try:
+            atomic_replace(tmp, target)
+        except BaseException:
+            with contextlib.suppress(OSError):  # the copy is disposable; keep the real error
+                tmp.unlink(missing_ok=True)
+            raise
+        retrying_unlink(spec_path)
         self.journal.append(
             "deferred-artifacts-stashed",
             story_key=task.story_key,
-            stashed_to=str(dest / spec_path.name),
+            stashed_to=str(target),
         )
 
     def _escalate(self, task: StoryTask, reason: str) -> None:

src/bmad_loop/platform_util.py

@@ -5,9 +5,15 @@
 delegate to it. ``detach_kwargs`` stays a real implementation — it is spawn
 configuration, not a process-lifecycle primitive, so it does not belong on the
 host. On Linux/macOS — and WSL, which *is* Linux — these preserve today's exact
-behavior. The win32 file-replace and path-segment helpers below (``atomic_replace``,
-``safe_segment``) are exercised by the platform tests; the pid kill/liveness
-Windows branch degrades gracefully and is not yet exercised.
+behavior. The file-replace and segment helpers below (``atomic_replace``,
+``safe_segment``, ``safe_ref_segment``) are exercised by the platform tests; the pid
+kill/liveness Windows branch degrades gracefully and is not yet exercised.
+
+``safe_segment`` and ``safe_ref_segment`` share a contract but not a rule set: the
+first coerces a Windows *filename* segment, the second a *git ref* component, and
+neither alphabet contains the other (``CON`` is a legal ref and an illegal filename;
+``a..b`` is the reverse). Consumers that derive both a directory and a branch from
+the same key must run both.
 """
 
 from __future__ import annotations
@@ -20,6 +26,7 @@
 import sys
 import time
 from pathlib import Path, PurePosixPath, PureWindowsPath
+from typing import Callable
 
 from .process_host import get_process_host
 
@@ -46,6 +53,12 @@
 _ILLEGAL_SEGMENT_CHARS = re.compile(r'[<>:"/\\|?*\x00-\x1f]')
 _MAX_SEGMENT = 120  # keep segment (incl. any collision suffix) well under the 255 limit
 
+# git-check-ref-format(1) rejects these anywhere in a ref component: ASCII control
+# chars and space (\x00-\x20), DEL, and `~ ^ : ? * [ \`. `/` is added because it
+# would split one component into two. `]`, `-`, `<`, `>`, `"` and `|` are all legal
+# in a ref and deliberately absent — this is not _ILLEGAL_SEGMENT_CHARS.
+_ILLEGAL_REF_CHARS = re.compile(r"[\x00-\x20\x7f~^:?*\[\\/]")
+
 
 def terminate_pid(pid: int) -> None:
     """Politely terminate ``pid``. Back-compat shim over
@@ -100,36 +113,61 @@ def has_parent_ref(value: str | Path) -> bool:
     return ".." in PurePosixPath(text).parts or ".." in PureWindowsPath(text).parts
 
 
-def atomic_replace(tmp: Path, target: Path) -> None:
-    """``os.replace(tmp, target)``, retried on the transient Windows sharing
-    violation a concurrent reader of ``target`` triggers (WinError 5/32). Gated to
-    win32 so a real POSIX EACCES/EPERM surfaces immediately instead of after a
-    pointless backoff. Worst-case total wait is ~5 s of jittered exponential
-    backoff before the final failure propagates."""
+def _retry_on_sharing_violation(op: Callable[[], None]) -> None:
+    """Run ``op``, retrying the transient Windows sharing violation a concurrent
+    handle on the file triggers (WinError 5/32). Gated to win32 so a real POSIX
+    EACCES/EPERM surfaces immediately instead of after a pointless backoff.
+    Worst-case total wait is ~5 s of jittered exponential backoff before the final
+    failure propagates."""
     for attempt in range(_REPLACE_ATTEMPTS):
         try:
-            os.replace(tmp, target)
+            op()
             return
         except OSError as exc:
             last = attempt == _REPLACE_ATTEMPTS - 1
-            # a retryable rename-over-open denial, not a genuine permission fault
+            # a retryable open-handle denial, not a genuine permission fault
             winerror = getattr(exc, "winerror", None)
             retryable = isinstance(exc, PermissionError) or winerror in (5, 32)
-            # portability: only Windows raises this on rename-over-open; elsewhere a
-            # permission error is real and must surface at once.
+            # portability: only Windows denies a rename/delete over an open handle;
+            # elsewhere a permission error is real and must surface at once.
             if sys.platform != "win32" or last or not retryable:
                 raise
             delay = min(_REPLACE_CAP_S, _REPLACE_BASE_S * 2**attempt)
             time.sleep(delay + random.uniform(0, _REPLACE_BASE_S))  # nosec B311 - retry jitter
 
 
+def atomic_replace(tmp: Path, target: Path) -> None:
+    """``os.replace(tmp, target)``, retried on the transient Windows sharing
+    violation a concurrent reader of ``target`` triggers."""
+    _retry_on_sharing_violation(lambda: os.replace(tmp, target))
+
+
+def retrying_unlink(path: Path) -> None:
+    """``path.unlink()`` with the same win32 retry as :func:`atomic_replace`.
+
+    Windows denies a *delete* against an open handle exactly as it denies a
+    rename-over, so the second half of a staged move is no safer than the first:
+    an AV/indexer scanning the just-written source file fails the unlink. Pair the
+    two whenever a move must not half-apply."""
+    _retry_on_sharing_violation(path.unlink)
+
+
 def _is_reserved_basename(seg: str) -> bool:
     """True if ``seg``'s basename (before the first dot, trailing spaces trimmed —
     ``CON .txt`` counts) is a Windows reserved device name."""
     stem = seg.split(".", 1)[0].rstrip(" ")
     return stem.upper() in _RESERVED_BASENAMES
 
 
+def _digest_suffix(name: str) -> str:
+    """The ``-<hex8>`` collision suffix both sanitizers append to changed input."""
+    digest = hashlib.sha1(
+        name.encode("utf-8", "surrogatepass"),
+        usedforsecurity=False,  # collision-resistance suffix, not a credential
+    ).hexdigest()
+    return "-" + digest[:8]
+
+
 def safe_segment(name: str) -> str:
     """Coerce ``name`` into a single Windows-legal path segment, returning legal
     input unchanged (identity for clean keys — the common case, e.g. a story key
@@ -152,9 +190,55 @@ def safe_segment(name: str) -> str:
         cleaned = "_"
     if cleaned == name:
         return name  # already a legal segment — keep it byte-identical
-    digest = hashlib.sha1(
-        name.encode("utf-8", "surrogatepass"),
-        usedforsecurity=False,  # collision-resistance suffix, not a credential
-    ).hexdigest()
-    suffix = "-" + digest[:8]
+    suffix = _digest_suffix(name)
+    return cleaned[: _MAX_SEGMENT - len(suffix)] + suffix
+
+
+def _is_clean_ref_segment(seg: str) -> bool:
+    """True if ``seg`` already satisfies git's rules for one ref component.
+
+    Mirrors ``git check-ref-format``'s per-component checks. The length cap is
+    ours, not git's: it keeps a branch segment in lockstep with the ``safe_segment``
+    directory built from the same key."""
+    return (
+        bool(seg)
+        and len(seg) <= _MAX_SEGMENT
+        and not _ILLEGAL_REF_CHARS.search(seg)
+        and ".." not in seg
+        and "@{" not in seg
+        and seg != "@"
+        and not seg.startswith(".")
+        and not seg.endswith((".", ".lock"))
+    )
+
+
+def safe_ref_segment(name: str) -> str:
+    """Coerce ``name`` into a single git-ref-legal component, returning legal input
+    unchanged (identity for clean keys — the common case, e.g. a story key like
+    ``3-2-digest-delivery`` or an auto-generated run id).
+
+    Same contract as :func:`safe_segment` — identity for clean input, a short digest
+    of the raw name appended whenever anything changed, never raises — but git's
+    alphabet, not Windows': replaces control chars, space, DEL and ``~^:?*[\\/`` with
+    ``_``, rewrites ``..`` → ``__`` and ``@{`` → ``_{``, escapes a leading ``.``, and
+    caps the length. Trailing ``.`` and trailing ``.lock`` are ref-illegal but need no
+    rewrite: they only reach the coercion path, and the ``-<hex8>`` suffix appended
+    there is itself the fix. A lone ``@`` is coerced to ``_`` even though git only
+    forbids it as a whole ref name, so the contract holds for any caller.
+
+    A leading ``-`` is deliberately preserved: it is legal in a ref component, and
+    the git porcelain's separate "branch name must not start with ``-``" check reads
+    the whole name, which callers always prefix (``bmad-loop/<run_id>/<segment>``).
+
+    Digest collision resistance is probabilistic, and clean-key identity is the
+    stronger contract — so a clean name that happens to look sanitized-plus-digest
+    passes through verbatim."""
+    if _is_clean_ref_segment(name):
+        return name  # already a legal ref component — keep it byte-identical
+    cleaned = _ILLEGAL_REF_CHARS.sub("_", name).replace("..", "__").replace("@{", "_{")
+    if cleaned.startswith("."):
+        cleaned = "_" + cleaned[1:]
+    if not cleaned or cleaned == "@":
+        cleaned = "_"
+    suffix = _digest_suffix(name)
     return cleaned[: _MAX_SEGMENT - len(suffix)] + suffix

src/bmad_loop/workspace.py

@@ -21,7 +21,7 @@
 
 from . import verify
 from .bmadconfig import ProjectPaths
-from .platform_util import safe_segment
+from .platform_util import safe_ref_segment, safe_segment
 
 # Per-unit worktrees live under the run dir (.bmad-loop/runs/<run_id>/worktrees/),
 # which `bmad-loop init` already gitignores — so unit checkouts never show up as
@@ -62,10 +62,19 @@ class UnitWorkspace:
 
 def unit_branch_name(run_id: str, unit_key: str, branch_per: str) -> str:
     """branch_per=run shares one branch across the whole run; branch_per=story
-    gives each unit its own branch."""
+    gives each unit its own branch.
+
+    Both segments are ref-sanitized: `--run-id` is user-suppliable and a unit key is
+    a sprint-board / ledger id, so either can carry ref-illegal sequences (`:`, `..`,
+    `@{`, a trailing `.lock`) that git rejects at branch-creation time. Clean ids —
+    every auto-generated run id and every conventional story key — pass through
+    byte-identical. This is the single source of the name: `open_unit_workspace` is
+    the sole caller and stores the result on `task.branch`, which every consumer
+    (`_merge_local`, `discard_worktree`, `close_unit_workspace`) reuses.
+    """
     if branch_per == "run":
-        return f"bmad-loop/{run_id}"
-    return f"bmad-loop/{run_id}/{unit_key}"
+        return f"bmad-loop/{safe_ref_segment(run_id)}"
+    return f"bmad-loop/{safe_ref_segment(run_id)}/{safe_ref_segment(unit_key)}"
 
 
 def open_unit_workspace(