From 97dd818c7e5c128939bd6c8e8e52cf089ab09608 Mon Sep 17 00:00:00 2001 From: pbean Date: Thu, 9 Jul 2026 10:01:34 -0700 Subject: [PATCH 1/3] fix(platform): overwrite-safe deferred stash + ref-safe unit branch names MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #101 — _stash_deferred_artifacts staged a re-defer's spec straight over the prior stash with shutil.move. Its os.rename raises FileExistsError on Windows, which move catches and papers over with a non-atomic copy2 + unlink: that tears the stash on a mid-copy crash, and re-fails outright when an AV/indexer handle turns the rename into a sharing violation and copy2 cannot open the locked target either. Stage a copy inside the destination dir, then route through platform_util.atomic_replace — one-step overwrite, #98's win32 retry, and the replace stays same-filesystem so move's cross-device tolerance is preserved. #102 — unit_branch_name built bmad-loop// from the raw ids, so a key or --run-id carrying `:`, `..`, `@{`, a space or a trailing `.lock` passed #98's directory sanitization only to die at `git worktree add` with "is not a valid branch name". New platform_util.safe_ref_segment mirrors safe_segment's contract (identity for clean input, - digest suffix whenever anything changed, never raises) on git's alphabet rather than Windows' — the two rule sets are disjoint in both directions, so a consumer deriving both a directory and a branch from one key must run both. Applied to both segments; a `git check-ref-format` oracle test pins the agreement over a nasty corpus. --- CHANGELOG.md | 13 ++++ src/bmad_loop/engine.py | 29 +++++++- src/bmad_loop/platform_util.py | 83 +++++++++++++++++++-- src/bmad_loop/workspace.py | 17 ++++- tests/test_engine.py | 103 ++++++++++++++++++++++++++ tests/test_engine_worktree.py | 18 +++++ tests/test_platform_util.py | 131 +++++++++++++++++++++++++++++++++ 7 files changed, 378 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c0635f..d2f4438 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,19 @@ 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//` 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, `-` 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. (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. + (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. diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index 6c813de..1b03d8d 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -39,7 +39,7 @@ SessionRecord, StoryTask, ) -from .platform_util import safe_segment +from .platform_util import atomic_replace, safe_segment from .plugins import HookBus, HookContext, PluginRegistry from .policy import Policy from .runs import kill_session @@ -2337,7 +2337,19 @@ 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.""" if not task.spec_file: return spec_path = Path(task.spec_file) @@ -2345,11 +2357,20 @@ def _stash_deferred_artifacts(self, task: StoryTask) -> None: 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 + spec_path.unlink() 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: diff --git a/src/bmad_loop/platform_util.py b/src/bmad_loop/platform_util.py index eecc39c..45aea2f 100644 --- a/src/bmad_loop/platform_util.py +++ b/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 @@ -46,6 +52,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 @@ -130,6 +142,15 @@ def _is_reserved_basename(seg: str) -> bool: return stem.upper() in _RESERVED_BASENAMES +def _digest_suffix(name: str) -> str: + """The ``-`` 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 +173,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 ``-`` 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//``). + + 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 diff --git a/src/bmad_loop/workspace.py b/src/bmad_loop/workspace.py index ec6d6d8..df61a5c 100644 --- a/src/bmad_loop/workspace.py +++ b/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//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( diff --git a/tests/test_engine.py b/tests/test_engine.py index 312d175..b22c9d2 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -1,5 +1,6 @@ """Engine scenario tests against the mock adapter — no tmux, no LLM.""" +import os import re import signal from pathlib import Path @@ -2140,6 +2141,108 @@ def halt_blocked(spec): assert read_frontmatter(sp)["status"] == "ready-for-dev" # re-drive will not HALT +# ------------------------------------------------------ deferred-artifact stash + + +def test_stash_deferred_artifacts_moves_spec_into_run_dir(project): + engine, _ = make_engine(project, []) + task = StoryTask("1-1-a", 1) + sp = spec_path(project, "1-1-a") + sp.parent.mkdir(parents=True, exist_ok=True) + sp.write_text("first attempt\n", encoding="utf-8") + task.spec_file = str(sp) + + engine._stash_deferred_artifacts(task) + + dest = engine.run_dir / "deferred" / "1-1-a" + assert (dest / sp.name).read_text(encoding="utf-8") == "first attempt\n" + assert not sp.exists() + stashed = [e for e in engine.journal.entries() if e["kind"] == "deferred-artifacts-stashed"] + assert len(stashed) == 1 and stashed[0]["stashed_to"] == str(dest / sp.name) + + +def test_stash_deferred_artifacts_overwrites_a_prior_stash(project): + """A story that defers a second time re-stashes the same filename: the newest + spec wins, leaving no staging residue. Characterization — `shutil.move` also + overwrote here (via its copy2 fallback); the tests below pin what changed.""" + engine, _ = make_engine(project, []) + task = StoryTask("1-1-a", 1) + sp = spec_path(project, "1-1-a") + sp.parent.mkdir(parents=True, exist_ok=True) + sp.write_text("second attempt\n", encoding="utf-8") + task.spec_file = str(sp) + + dest = engine.run_dir / "deferred" / "1-1-a" + dest.mkdir(parents=True, exist_ok=True) + (dest / sp.name).write_text("first attempt\n", encoding="utf-8") + + engine._stash_deferred_artifacts(task) + + assert (dest / sp.name).read_text(encoding="utf-8") == "second attempt\n" + assert not sp.exists() + assert list(dest.iterdir()) == [dest / sp.name] # the staging copy is gone + + +def test_stash_deferred_artifacts_survives_a_win32_sharing_violation(project, monkeypatch): + """The real #101 hazard: an AV/indexer handle on the destination denies the + rename. `shutil.move` caught that OSError and fell back to `copy2`, which opens + that same locked target and fails too — crashing the defer flow. Routing through + `atomic_replace` retries the replace until the handle clears.""" + engine, _ = make_engine(project, []) + task = StoryTask("1-1-a", 1) + sp = spec_path(project, "1-1-a") + sp.parent.mkdir(parents=True, exist_ok=True) + sp.write_text("second attempt\n", encoding="utf-8") + task.spec_file = str(sp) + + dest = engine.run_dir / "deferred" / "1-1-a" + dest.mkdir(parents=True, exist_ok=True) + (dest / sp.name).write_text("first attempt\n", encoding="utf-8") + + monkeypatch.setattr(platform_util.sys, "platform", "win32") + monkeypatch.setattr(platform_util.time, "sleep", lambda _s: None) + calls, real_replace = {"n": 0}, os.replace + + def sharing_violation_once(src, dst): + calls["n"] += 1 + if calls["n"] == 1: + raise PermissionError(32, "The process cannot access the file") + real_replace(src, dst) + + monkeypatch.setattr(platform_util.os, "replace", sharing_violation_once) + engine._stash_deferred_artifacts(task) + + assert calls["n"] == 2 # denied once, retried, landed + assert (dest / sp.name).read_text(encoding="utf-8") == "second attempt\n" + assert not sp.exists() + assert list(dest.iterdir()) == [dest / sp.name] + + +def test_stash_deferred_artifacts_keeps_source_and_cleans_tmp_on_replace_failure( + project, monkeypatch +): + """The replace is the only step that can fail after staging: the source spec + must survive it (the stash is forensic — losing the work is worse than a crash) + and the staging copy must not linger.""" + engine, _ = make_engine(project, []) + task = StoryTask("1-1-a", 1) + sp = spec_path(project, "1-1-a") + sp.parent.mkdir(parents=True, exist_ok=True) + sp.write_text("work\n", encoding="utf-8") + task.spec_file = str(sp) + + def boom(_tmp, _target): + raise PermissionError(13, "Permission denied") + + monkeypatch.setattr("bmad_loop.engine.atomic_replace", boom) + with pytest.raises(PermissionError): + engine._stash_deferred_artifacts(task) + + assert sp.read_text(encoding="utf-8") == "work\n" + assert list((engine.run_dir / "deferred" / "1-1-a").iterdir()) == [] + assert "deferred-artifacts-stashed" not in [e["kind"] for e in engine.journal.entries()] + + # -------------------------------------------------- intent-gap patch-restore (#2564) diff --git a/tests/test_engine_worktree.py b/tests/test_engine_worktree.py index 427068c..f8fff7d 100644 --- a/tests/test_engine_worktree.py +++ b/tests/test_engine_worktree.py @@ -235,6 +235,24 @@ def test_branch_per_run_naming(project): assert branch_exists(project.project, "bmad-loop/test-run") +def test_dirty_unit_key_branch_is_created_by_real_git(project): + """#102: a unit key carrying ref-illegal sequences reached `git branch` raw and + blew up at worktree-mount time. `unit_branch_name` now ref-sanitizes both + segments, so real git accepts the name — while the worktree dir (safe_segment) + and the branch (safe_ref_segment) are each sanitized on their own alphabet.""" + from bmad_loop.workspace import open_unit_workspace, unit_branch_name + + commit_sprint(project, {"1-1-a": "ready-for-dev"}) + key = "story/1:2..3@{now}.lock" + run_dir = project.project / ".bmad-loop" / "runs" / "test-run" + unit = open_unit_workspace(project.project, project, "test-run", key, "main", "story", run_dir) + + assert unit.branch == unit_branch_name("test-run", key, "story") + assert unit.branch.startswith("bmad-loop/test-run/story_1_2__3_{now}.lock-") + assert branch_exists(project.project, unit.branch) # real git accepted the name + assert unit.path.is_dir() and unit.path.name != key # dir sanitized separately + + # ----------------------------------------------------------------- merge strategies diff --git a/tests/test_platform_util.py b/tests/test_platform_util.py index 48d3032..6cac987 100644 --- a/tests/test_platform_util.py +++ b/tests/test_platform_util.py @@ -8,6 +8,7 @@ from __future__ import annotations import os +import subprocess import sys import pytest @@ -197,3 +198,133 @@ def test_dirty_story_key_segment_is_creatable(tmp_path): d = resolve._story_dir(tmp_path, 'a:c."') d.mkdir(parents=True) assert d.is_dir() + + +# -------------------------------------------------------------- safe_ref_segment + +# Raw keys spanning every rule class, shared by the property tests and the git +# oracle. Only the sanitizer's *output* is ever handed to git, so NUL/DEL/tab in +# here never reach a subprocess argv. +_REF_CORPUS = [ + # clean — must survive the oracle byte-identical + "3-2-digest-delivery", + "epic1_story2", + "a.b.c", + "plain", + "CON", + "-leading-dash", + "ac", + 'a"b|c', + "a]b", + "@@", + "é-ünïcødé", + # one per coercion rule + "a:b", + "a b", + "a~b", + "a^b", + "a?b", + "a*b", + "a[b", + "a\\b", + "a/b", + "with\ttab", + "a\x7fb", + "a\x00b", + "a..b", + "a@{b", + ".hidden", + "x.", + "a.lock", + "@", + "", + "x" * 500, + # adversarial combinations + "...", + "....", + ".lock", + "..lock", + "a.lock.lock", + "@{u}", + "refs/heads/x", + "/lead", + "trail/", + "a//b", + " ", + "story/1:2..3@{now}.lock", +] + + +@pytest.mark.parametrize( + "value", + ["3-2-digest-delivery", "epic1_story2", "a.b.c", "plain", "CON", "-leading-dash", "ac"], +) +def test_safe_ref_segment_identity_for_clean_input(value): + # git's alphabet is not Windows': `CON` and `ac` are ref-legal (safe_segment + # rewrites both), and a leading `-` is legal inside the always-prefixed branch. + assert platform_util.safe_ref_segment(value) == value + + +@pytest.mark.parametrize( + "value, base", + [ + ("a:b", "a_b"), # colon + ("a b", "a_b"), # space + ("a~b", "a_b"), + ("a^b", "a_b"), + ("a?b", "a_b"), + ("a*b", "a_b"), + ("a[b", "a_b"), + ("a\\b", "a_b"), + ("a/b", "a_b"), # would split one component into two + ("with\ttab", "with_tab"), # control char + ("a\x7fb", "a_b"), # DEL + ("a..b", "a__b"), # ref-illegal, filename-legal + ("a@{b", "a_{b"), + (".hidden", "_hidden"), # leading dot + ("x.", "x."), # trailing dot: no rewrite, the digest suffix is the fix + ("a.lock", "a.lock"), # trailing .lock: ditto + ("@", "_"), # lone @ + ("", "_"), + ], +) +def test_safe_ref_segment_coerces_and_suffixes_changed_input(value, base): + out = platform_util.safe_ref_segment(value) + assert out != value + assert out.startswith(base + "-") # sanitized base + collision-suffix digest + + +def test_safe_ref_segment_distinct_dirty_keys_never_collide(): + # `a..b` and `a//b` sanitize to the same base — the digest keeps their unit + # branches (and so their merge targets) distinct + a = platform_util.safe_ref_segment("a..b") + b = platform_util.safe_ref_segment("a//b") + assert a.startswith("a__b-") and b.startswith("a__b-") + assert a != b + + +def test_safe_ref_segment_caps_length(): + assert len(platform_util.safe_ref_segment("x" * 500)) <= platform_util._MAX_SEGMENT + + +@pytest.mark.parametrize("value", _REF_CORPUS) +@pytest.mark.parametrize( + "template", + [ + "bmad-loop/rid/{}", # unit_key, branch_per=story + "bmad-loop/{}/1-1-a", # run_id, branch_per=story + "bmad-loop/{}", # run_id, branch_per=run + ], + ids=["unit_key", "run_id", "run_id_shared"], +) +def test_safe_ref_segment_output_passes_git_check_ref_format(value, template): + """Oracle: git itself validates every sanitized segment, in each position + `workspace.unit_branch_name` actually places it. Pure-Python sanitization is + only as good as its agreement with `git check-ref-format`.""" + branch = template.format(platform_util.safe_ref_segment(value)) + proc = subprocess.run( + ["git", "check-ref-format", "--branch", branch], + capture_output=True, + text=True, + ) + assert proc.returncode == 0, f"{value!r} -> {branch!r}: {proc.stderr.strip()}" From f2b965a415d7ab02c363891dac4a566ae0833b34 Mon Sep 17 00:00:00 2001 From: pbean Date: Thu, 9 Jul 2026 10:15:04 -0700 Subject: [PATCH 2/3] fix(platform): retry the source unlink in the deferred stash too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review catch on #103. Windows denies a *delete* against an open handle exactly as it denies a rename-over, so hardening only the replace left the staged move half-hardened: an AV/indexer handle on the just-written source spec fails `spec_path.unlink()` after the stash has already landed. `_defer` calls _stash_deferred_artifacts before the rollback and the `story-deferred` journal append, so that raise aborts the whole deferral. Suppressing the unlink is wrong — it silently leaves the stale in-review spec this function exists to remove. Instead extract atomic_replace's win32 backoff into _retry_on_sharing_violation and reuse it for a new retrying_unlink, giving both halves of the move the same transient-lock tolerance. A permanent failure still propagates, as before. Replace-then-unlink ordering is deliberate: a failure can leave a duplicate spec, never a hole where the work was. --- CHANGELOG.md | 5 ++-- src/bmad_loop/engine.py | 13 +++++++-- src/bmad_loop/platform_util.py | 37 +++++++++++++++++------- tests/test_engine.py | 31 ++++++++++++++++++++ tests/test_platform_util.py | 52 ++++++++++++++++++++++++++++++++++ 5 files changed, 123 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d2f4438..3d57217 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -42,8 +42,9 @@ breaking changes may land in a minor release. 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. - (closes #101) + 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. diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index 1b03d8d..fa964bd 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -39,7 +39,7 @@ SessionRecord, StoryTask, ) -from .platform_util import atomic_replace, safe_segment +from .platform_util import atomic_replace, retrying_unlink, safe_segment from .plugins import HookBus, HookContext, PluginRegistry from .policy import Policy from .runs import kill_session @@ -2349,7 +2349,14 @@ def _stash_deferred_artifacts(self, task: StoryTask) -> None: 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.""" + `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) @@ -2366,7 +2373,7 @@ def _stash_deferred_artifacts(self, task: StoryTask) -> None: with contextlib.suppress(OSError): # the copy is disposable; keep the real error tmp.unlink(missing_ok=True) raise - spec_path.unlink() + retrying_unlink(spec_path) self.journal.append( "deferred-artifacts-stashed", story_key=task.story_key, diff --git a/src/bmad_loop/platform_util.py b/src/bmad_loop/platform_util.py index 45aea2f..b763730 100644 --- a/src/bmad_loop/platform_util.py +++ b/src/bmad_loop/platform_util.py @@ -26,6 +26,7 @@ import sys import time from pathlib import Path, PurePosixPath, PureWindowsPath +from typing import Callable from .process_host import get_process_host @@ -112,29 +113,45 @@ 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.""" diff --git a/tests/test_engine.py b/tests/test_engine.py index b22c9d2..d31d1b4 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -2218,6 +2218,37 @@ def sharing_violation_once(src, dst): assert list(dest.iterdir()) == [dest / sp.name] +def test_stash_deferred_artifacts_retries_a_locked_source_spec(project, monkeypatch): + """Second half of the staged move. Windows denies the source *delete* against an + AV/indexer handle just as it denies the rename-over, and `_defer` calls this + before the rollback and the `story-deferred` journal append — so an unretried + unlink would abort the whole deferral after the stash had already landed.""" + engine, _ = make_engine(project, []) + task = StoryTask("1-1-a", 1) + sp = spec_path(project, "1-1-a") + sp.parent.mkdir(parents=True, exist_ok=True) + sp.write_text("work\n", encoding="utf-8") + task.spec_file = str(sp) + + monkeypatch.setattr(platform_util.sys, "platform", "win32") + monkeypatch.setattr(platform_util.time, "sleep", lambda _s: None) + calls, real_unlink = {"n": 0}, os.unlink + + def locked_once(path, **_kw): + calls["n"] += 1 + if calls["n"] == 1: + raise PermissionError(32, "The process cannot access the file") + real_unlink(path) + + monkeypatch.setattr(platform_util.os, "unlink", locked_once) + engine._stash_deferred_artifacts(task) + + assert calls["n"] == 2 # denied once, retried, removed + assert not sp.exists() + dest = engine.run_dir / "deferred" / "1-1-a" + assert (dest / sp.name).read_text(encoding="utf-8") == "work\n" + + def test_stash_deferred_artifacts_keeps_source_and_cleans_tmp_on_replace_failure( project, monkeypatch ): diff --git a/tests/test_platform_util.py b/tests/test_platform_util.py index 6cac987..465ed24 100644 --- a/tests/test_platform_util.py +++ b/tests/test_platform_util.py @@ -131,6 +131,58 @@ def denied(src, dst): assert sleeps == [] # zero backoff — a real POSIX error surfaces at once +# --------------------------------------------------------------- retrying_unlink + + +def test_retrying_unlink_retries_then_succeeds(tmp_path, monkeypatch): + # Windows denies a delete against an open handle exactly as it denies a + # rename-over, so the second half of a staged move needs the same backoff. + monkeypatch.setattr(platform_util.sys, "platform", "win32") + sleeps: list[float] = [] + monkeypatch.setattr(platform_util.time, "sleep", lambda s: sleeps.append(s)) + + victim = tmp_path / "spec.md" + victim.write_text("x", encoding="utf-8") + calls = {"n": 0} + real_unlink = os.unlink + + def flaky_unlink(path, **_kw): + calls["n"] += 1 + if calls["n"] <= 2: + raise PermissionError(32, "The process cannot access the file") + real_unlink(path) + + monkeypatch.setattr(platform_util.os, "unlink", flaky_unlink) + platform_util.retrying_unlink(victim) + + assert calls["n"] == 3 + assert len(sleeps) == 2 + assert not victim.exists() + + +def test_retrying_unlink_no_retry_on_posix(tmp_path, monkeypatch): + monkeypatch.setattr(platform_util.sys, "platform", "linux") + sleeps: list[float] = [] + monkeypatch.setattr(platform_util.time, "sleep", lambda s: sleeps.append(s)) + + def denied(_path, **_kw): + raise PermissionError(13, "Permission denied") + + monkeypatch.setattr(platform_util.os, "unlink", denied) + victim = tmp_path / "spec.md" + victim.write_text("x", encoding="utf-8") + + with pytest.raises(PermissionError): + platform_util.retrying_unlink(victim) + assert sleeps == [] # a real POSIX error surfaces at once + + +def test_retrying_unlink_propagates_missing_file(tmp_path): + # not a sharing violation — no retry, no swallow + with pytest.raises(FileNotFoundError): + platform_util.retrying_unlink(tmp_path / "gone.md") + + # ------------------------------------------------------------------ safe_segment From 5b18ffe782315e04d47686bf408c2756245b5641 Mon Sep 17 00:00:00 2001 From: pbean Date: Thu, 9 Jul 2026 10:48:50 -0700 Subject: [PATCH 3/3] refactor(engine): reuse safe_ref_segment for attempt-preserve slugs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both recovery-ref sites re-implemented ref sanitization inline (alnum/_- join, 64-char cap). Never illegal — but a second rule set to keep in agreement with git, and lossier than the real one: dots in a clean run id were mangled, and distinct exotic ids could collapse to the same slug. Route both through safe_ref_segment: identity for clean ids, digest suffix keeps dirty ids distinct, single source of truth. Characterization test only — no red-first guard is possible because the old inline slug also produced legal refs; the test pins the invariant (real git resolves the slugged ref) and the new digest-suffixed shape. --- CHANGELOG.md | 3 ++- src/bmad_loop/engine.py | 19 ++++++++++--------- tests/test_engine.py | 27 +++++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3d57217..36c25ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -37,7 +37,8 @@ breaking changes may land in a minor release. `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, `-` 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. (closes #102) + `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 diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index fa964bd..22ec1d1 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -39,7 +39,7 @@ SessionRecord, StoryTask, ) -from .platform_util import atomic_replace, retrying_unlink, 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 diff --git a/tests/test_engine.py b/tests/test_engine.py index d31d1b4..80639ce 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -1678,6 +1678,33 @@ def test_rollback_preserves_distinct_refs_across_repeated_dirty_rollbacks(projec assert git(repo, "show", f"{refs[1]}:src.txt") == "attempt 1 edit" +def test_rollback_preserve_ref_slug_survives_a_ref_illegal_run_id(project): + """A `--run-id` carrying ref-illegal sequences must not drop the recovery ref. + Characterization for the safe_ref_segment swap — the old inline alnum/`_-` slug + also kept the ref legal; what this pins is the invariant (real git accepts the + slugged ref and the snapshot stays reachable) plus the new digest-suffixed shape.""" + policy = Policy( + gates=GatesPolicy(mode="none"), + notify=QUIET, + scm=ScmPolicy(rollback_on_failure=True), + ) + engine, _ = make_engine(project, [], policy=policy) + engine.state.run_id = "story/1:2..3@{now}.lock" + repo = project.project + task = StoryTask(story_key="1-1-a", epic=1) + task.baseline_commit = rev_parse_head(repo) + task.baseline_untracked = [] + (repo / "src.txt").write_text("uncommitted edit\n") + + engine._rollback_or_pause(task) + + assert rev_parse_head(repo) == task.baseline_commit # reset happened + entry = next(e for e in engine.journal.entries() if e["kind"] == "attempt-worktree-preserved") + ref = entry["ref"] + assert ref.startswith("refs/attempt-preserve-dirty/story_1_2__3_{now}.lock-") + assert git(repo, "show", f"{ref}:src.txt") == "uncommitted edit" # real git resolves it + + def test_run_start_prunes_excess_preserve_refs(project): """Run start with scm.preserve_keep set and more attempt-preserve/* refs than the budget: the tail is deleted before the loop, only preserve_keep refs