Windows: _stash_deferred_artifacts shutil.move raises FileExistsError when a story re-defers

[pbean]

Summary

_stash_deferred_artifacts (engine.py, ~line 2338) moves a deferred story's spec into run_dir/deferred/<story_key>/<spec_name> via shutil.move. On the same filesystem that falls back to os.rename, which on Windows raises FileExistsError when the target already exists — i.e. when the same story defers a second time with the same spec filename. POSIX rename(2) silently overwrites, which is why this has never bitten on Linux/macOS.

This is another instance of the "POSIX-safe idiom, Windows-unsafe" class from #74 (H-list), surfaced while auditing PR #98 — it's the one rename-over site in the package that #98's atomic_replace sweep did not cover (the target is a possibly-existing file, not a fresh temp-rename).

Fix direction

Give the move overwrite semantics and the win32 retry in one shot: copy/write into a temp path inside the dest dir, then route through platform_util.atomic_replace (which wraps os.replace — overwriting — with the win32 sharing-violation retry from #98). Alternatively os.replace directly if the source is same-filesystem-guaranteed.

Notes

• The stash is a write-only forensic artifact (no reader recomposes the path), so the blast radius is the defer flow crashing mid-run on Windows, not data corruption.
• The two stdlib-only hook scripts' os.replace calls (data/bmad_loop_hook.py, data/bmad_loop_probe_hook.py) were also checked and are fine as-is: time_ns-unique target names, never concurrently read, and they can't import the package by design.

Refs: #74, #98

[coderabbitai]

🔗 Related PRs

#53 - fix(adapters): complete dev sessions only on Stop or window death [merged]
#65 - feat(stories): spec-to-loop contract layer (Phase 1) [merged]
#77 - feat(stories): spec-to-loop engine, adapter, CLI + HITL checkpoints (Phase 2 + 3) [merged]
#86 - feat(resolve): intent-gap patch-restore re-drive + inline-review-layer preflight (re-cut) [merged]
#95 - feat(adapters): post-kill reconcile rescues finished stalled/timeout sessions [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]

Correction while implementing the fix (PR #103): the mechanism above is wrong — shutil.move does not raise FileExistsError here, and a re-defer does not crash on Windows.

shutil.move wraps its os.rename in a bare except OSError. FileExistsError is an OSError, so the raise is caught and move falls back to copy_function(src, real_dst) + os.unlink(src) — i.e. copy2, which overwrites. Confirmed against the CPython source and by emulating Windows rename semantics (os.rename raising FileExistsError over an existing target): move succeeds, dst holds the new content, src is gone.

The fix direction the issue proposes is still right, but for two different reasons — both consequences of that copy2 fallback rather than of an uncaught raise:

1. Sharing violation (the real crash). When an AV/indexer handle on the target denies the rename with WinError 5/32, move falls back to copy2, which opens that same locked target and fails too. The PermissionError propagates and the defer flow dies. This is precisely what atomic_replace's retry exists for.
2. Non-atomicity. The fallback truncates the live target before rewriting it, so a crash mid-copy leaves a torn stash. Stage-then-replace never exposes a partial file.

Practical impact on the fix: none — stage-a-copy-in-dest + atomic_replace addresses both. Practical impact on the tests: the "pre-create the dest file → assert the stash replaces it" test suggested in the plan is a characterization test on every platform, not a regression guard, because old code passes it too (on Linux via rename, on Windows via the copy2 fallback). PR #103 keeps it for documentation value but adds the two tests that actually go red against shutil.move: a simulated WinError-32 sharing violation that the retry rescues, and a failed-replace path that must leave the source spec intact and no .tmp residue.

Leaving this here so the "POSIX-safe idiom, Windows-unsafe" H-list from #74 doesn't inherit a mis-stated instance.