From 8c75316ff84ccd6d3fb8dfee4579c1f201ffa504 Mon Sep 17 00:00:00 2001 From: pbean Date: Mon, 27 Jul 2026 18:02:16 -0700 Subject: [PATCH 1/9] fix(recovery): pause when a failed worktree snapshot would lose work MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two preserve steps were asymmetric: preserve_attempt_commits refused to reset past commits it could not park, while a failed snapshot_worktree was journaled and safe_reset ran anyway, destroying the tracked edits and run-created untracked files the snapshot existed to capture. That guarded the more recoverable half — orphaned commits survive in the object store until gc, an uncommitted edit a reset --hard discards does not. preserve_attempt_worktree now takes the same required allow_pause its sibling does and refuses on the same terms, behind a _reset_would_destroy probe so a capture failure over a tree with nothing unparked left to lose still resets rather than halting an unattended run (#123). The probe reuses attempt_dirty against HEAD — commits are already parked or paused on by then — and fails safe on its own git fault (#156). New notice shape (d) names workspace.root, so an in-worktree pause targets the mounted tree rather than the operator's checkout (#161), and offers a git-free rescue: whatever broke the snapshot may still be breaking git. Closes #340 --- CHANGELOG.md | 17 +++- docs/FEATURES.md | 3 +- src/bmad_loop/engine.py | 16 +++- src/bmad_loop/recovery_flow.py | 147 ++++++++++++++++++++++++----- tests/test_engine.py | 52 ++++++++-- tests/test_recovery_flow.py | 167 +++++++++++++++++++++++++++++++-- 6 files changed, 353 insertions(+), 49 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 971b001..303b87b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,22 @@ breaking changes may land in a minor release. ## [Unreleased] -### Added +### Changed + +- **A failed worktree snapshot now blocks the rollback reset (#340).** The two preserve steps were + asymmetric: an auto-rollback refused to reset past commits it could not park, but a failed + *uncommitted*-work snapshot was journaled and the `reset --hard` ran anyway — destroying the + tracked edits and run-created untracked files the snapshot existed to capture. That protected the + more recoverable half; orphaned commits stay reachable by reflog until `gc`, while a discarded + uncommitted edit is gone. Both paths now refuse on the same terms, pausing with rescue + instructions that name the tree (the mounted worktree when there is one) and offer a git-free + copy-out, since the fault that broke the snapshot may still be breaking git. + + Gated on there being something left to lose: a capture failure over a tree whose content was all + committed still resets, so a git fault can't halt an unattended run over a harmless reset. A + resolved re-drive stays pause-free by contract — it journals, resets, and flags the park + commits-only (#338). Inert under the shipped defaults, where the snapshot is reached only on a + re-drive; it bites `scm.rollback_on_failure = true` and in-worktree dev retries. - **Defer notifications name where the work survives (#333).** A deferred story's rollback parked the attempt on an `attempt-preserve/*` branch (or a `refs/attempt-preserve-dirty/*` snapshot), diff --git a/docs/FEATURES.md b/docs/FEATURES.md index b9df1f7..db7d164 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -59,7 +59,8 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se ### Failure handling & resilience - Bounded dev retries (default 2): verify-failures keep the tree and feed the failing output to the next session via `--feedback`; other failures roll back to baseline. -- Plateau-defer: when review won't converge, the story is skipped, the spec stashed into the run dir, deferred-work preserved, the run continues. The defer notification names where the attempt survives — in place, the recovery ref the rollback parked it on plus the `git merge --ff-only` line that restores it, flagged as commits-only when the uncommitted snapshot could not be captured; isolated, the kept-failed unit branch plus any earlier rolled-back attempt's ref (named, not offered as a merge — it is not fast-forwardable there). That recovery ref is projected as `preserve_ref` in `status` and `--json`; the unit branch never is (#333). +- An auto-rollback parks the attempt before it resets — commits above baseline on an `attempt-preserve/*` branch, the uncommitted tree (tracked edits + run-created untracked files) on a `refs/attempt-preserve-dirty/*` snapshot — and **refuses the reset if it could not** (#340): the run pauses with rescue instructions naming the tree, rather than discarding work the safety net failed to capture. A resolved re-drive is pause-free by contract, so there it journals and proceeds. `scm.preserve_keep` (default 20) bounds retention of both ref families. +- Plateau-defer: when review won't converge, the story is skipped, the spec stashed into the run dir, deferred-work preserved, the run continues. The defer notification names where the attempt survives — in place, the recovery ref the rollback parked it on plus the `git merge --ff-only` line that restores it, flagged as commits-only when the uncommitted snapshot could not be captured (only reachable on a re-drive since #340 — a plain rollback refuses the reset instead); isolated, the kept-failed unit branch plus any earlier rolled-back attempt's ref (named, not offered as a merge — it is not fast-forwardable there). That recovery ref is projected as `preserve_ref` in `status` and `--json`; the unit branch never is (#333). - Typed escalations: `CRITICAL` pauses the run + notifies (desktop + `ATTENTION` file); `PREFERENCE` is journaled and continues. - Environment faults pause without burning budget (#194): a session whose coding CLI never reached the API — a deterministic verify command whose _environment_ is broken (`sh` reports `rc 126/127`; `cmd` has no such convention, so on Windows a missing tool is caught by its `is not recognized` message or by resolving the command's leading token, and a command naming a file `cmd` cannot execute — a `.sh`, anything outside `PATHEXT` — is a fault rather than the silent `rc 0` pass it used to be, #302), **or** a dev/review/fix/workflow/sweep session whose pane log matches the profile's `env_fault_patterns` (an `API Error … Connection refused`-class transport failure that idled out the session clock) — pauses the run with the matched evidence instead of charging the attempt/cycle and deferring the story as if its code were broken. Re-arm (or a sweep's escalation-resume) restores the budget, so a resume after the outage clears re-drives from a clean slate. Patterns are per-profile (seeded only for `claude`; extend/disable via a project profile overlay). - CRITICAL resolution: `bmad-loop resolve ` opens an interactive resolve agent seeded with the escalation + frozen spec; you disambiguate, it re-arms the story (`escalated → pending`, spec reset to `ready-for-dev`) and resumes. `--no-interactive` skips to re-arm if you fixed the spec yourself. diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index d35586b..0254022 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -765,14 +765,22 @@ def _prune_preserve_refs(self) -> None: def _preserve_attempt_commits(self, task: StoryTask, *, allow_pause: bool) -> None: self._recovery_flow.preserve_attempt_commits(task, allow_pause=allow_pause) - def _preserve_attempt_worktree(self, task: StoryTask) -> None: - self._recovery_flow.preserve_attempt_worktree(task) + def _preserve_attempt_worktree(self, task: StoryTask, *, allow_pause: bool) -> None: + self._recovery_flow.preserve_attempt_worktree(task, allow_pause=allow_pause) def _pause_for_manual_recovery( - self, task: StoryTask, baseline: str, *, preserve_failed: bool = False + self, + task: StoryTask, + baseline: str, + *, + preserve_failed: bool = False, + snapshot_failed: bool = False, ) -> None: self._recovery_flow.pause_for_manual_recovery( - task, baseline, preserve_failed=preserve_failed + task, + baseline, + preserve_failed=preserve_failed, + snapshot_failed=snapshot_failed, ) def _finish_inflight(self) -> None: diff --git a/src/bmad_loop/recovery_flow.py b/src/bmad_loop/recovery_flow.py index e7c09ee..dd72981 100644 --- a/src/bmad_loop/recovery_flow.py +++ b/src/bmad_loop/recovery_flow.py @@ -118,14 +118,22 @@ def rollback_or_pause(self, task: StoryTask, *, cause: str = "stopped") -> None: later mid-re-drive retry/defer reset can't silently revert the correction. Otherwise (a stopped/abandoned attempt) recovery depends on where the - attempt ran. Inside a mounted unit worktree it always auto-recovers: the - worktree is disposable, the attempt's work is parked on preserve refs - before the reset, and ``scm.rollback_on_failure`` gates *in-place* - (isolation="none") recovery only (#161). In the main checkout the flag - governs: OFF (default) leaves the working tree untouched and emits a bold - manual-recovery notice that pauses the run (stop-and-wait); ON does a + attempt ran. Inside a mounted unit worktree it auto-recovers instead of + pausing on policy: the worktree is disposable, the attempt's work is parked + on preserve refs before the reset, and ``scm.rollback_on_failure`` gates + *in-place* (isolation="none") recovery only (#161). In the main checkout the + flag governs: OFF (default) leaves the working tree untouched and emits a + bold manual-recovery notice that pauses the run (stop-and-wait); ON does a clean reset to baseline. Either way pre-existing untracked files are - preserved; there is no blanket ``git clean``.""" + preserved; there is no blanket ``git clean``. + + The two preserve steps are the one thing that can still pause a rollback the + branching above chose to auto-recover, worktree included: when the attempt's + committed or uncommitted work cannot be parked and the reset would destroy + it, they refuse rather than reset (#340). That is a preservation failure + rather than a policy decision, so it does not weaken #161 — the notice + targets ``workspace.root``, which is the mounted worktree when there is + one.""" workspace = self._workspace_get() resolved = cause == "resolved" # preserve the corrected spec for the whole re-drive, not just the first @@ -193,8 +201,9 @@ def rollback_or_pause(self, task: StoryTask, *, cause: str = "stopped") -> None: # Park the attempt's uncommitted diff too, so the reset below (and its # untracked cleanup) can't silently destroy in-progress work. Runs only # if preserve_attempt_commits did not pause (plain-rollback preserve - # failure); best-effort, never blocks. - self.preserve_attempt_worktree(task) + # failure), and refuses the reset on the same terms when a failed + # capture would cost unparked work (#340). + self.preserve_attempt_worktree(task, allow_pause=not redrive) self.safe_reset(task, preserve=protected) # Refresh the plugin's view of the now-reset tree (the Unity engine # re-imports assets). Observe-only for the same reason as pre_rollback. @@ -347,16 +356,33 @@ def preserve_attempt_commits(self, task: StoryTask, *, allow_pause: bool) -> Non "attempt-commits-preserved", story_key=task.story_key, ref=ref, count=len(commits) ) - def preserve_attempt_worktree(self, task: StoryTask) -> None: + def preserve_attempt_worktree(self, task: StoryTask, *, allow_pause: bool) -> None: """Before an auto-rollback's hard reset, park the attempt's *uncommitted* working-tree changes (tracked edits + run-created untracked files) under a named recovery ref, so `reset --hard baseline` and its untracked cleanup can't silently destroy in-progress work. Complements `_preserve_attempt_commits` (which parks *committed* work above baseline); together they cover the whole attempt. No-op when the tree is clean vs HEAD - — the intended non-destructive uncommitted-revert case. Best-effort: a - capture failure is journaled but never blocks the (human-directed re-drive - or policy-gated) reset — the recovery ref is a safety net, not a gate.""" + — the intended non-destructive uncommitted-revert case. + + A capture failure is a gate, not a footnote (#340 — this reverses the + original best-effort contract). The two preserve steps used to be + asymmetric: the commits path refused to reset past work it could not park, + while a failed snapshot journaled and let the reset run. That protected the + *more* recoverable half — orphaned commits stay in the object store, + reachable by reflog/`git fsck` until gc, whereas an uncommitted edit a + `reset --hard` discards is gone permanently. Both paths now refuse on the + same terms: with ``allow_pause`` (a plain rollback) pause for manual + recovery rather than reset; on a re-drive (``allow_pause=False``) the + caller's contract forbids pausing, so journal and let the human-directed + reset proceed. + + The refusal is gated on :meth:`_reset_would_destroy`, so a capture failure + over a tree with nothing left to lose (commits already parked, nothing + uncommitted) still resets instead of halting an unattended run. The failure + is journaled either way, and ``preserve_partial`` is latched either way — + on the re-drive path the reset still runs, so the defer notice must still + downgrade its claim to the committed half (#338).""" baseline = task.baseline_commit if not baseline: return @@ -380,14 +406,22 @@ def preserve_attempt_worktree(self, task: StoryTask) -> None: self.journal.append( "attempt-worktree-preserve-failed", story_key=task.story_key, error=str(exc) ) - # The reset below runs regardless (best-effort contract), so whatever was - # uncommitted is now gone. Latch that: `preserve_ref` may still name the - # commits branch parked just above, and the defer notice must offer it as - # the committed half rather than as the whole attempt. Set unconditionally - # — snapshot_worktree can raise before it can tell whether the tree was - # even dirty, so "could not capture" is the only honest state. Harmless - # when nothing was parked: the notice short-circuits on the ref first. + # Latch the partial marker before deciding pause-vs-reset: on the + # re-drive path below the reset still runs, so `preserve_ref` may name + # the commits branch parked just above and the defer notice must offer + # it as the committed half rather than as the whole attempt (#338). Set + # unconditionally — snapshot_worktree can raise before it can tell + # whether the tree was even dirty, so "could not capture" is the only + # honest state. Harmless when nothing was parked: the notice + # short-circuits on the ref first. task.preserve_partial = True + if not allow_pause: + return # re-drive: never pause — proceed to the (human-directed) reset + # Refuse the reset rather than destroy what the snapshot failed to save + # (#340) — but only when something unparked is actually at stake, so a + # git fault over a harmless reset can't halt an unattended run. + if self._reset_would_destroy(task): + self.pause_for_manual_recovery(task, baseline, snapshot_failed=True) return if parked: # Last writer wins over preserve_attempt_commits' branch on purpose: @@ -399,11 +433,39 @@ def preserve_attempt_worktree(self, task: StoryTask) -> None: task.preserve_ref = parked self.journal.append("attempt-worktree-preserved", story_key=task.story_key, ref=parked) + def _reset_would_destroy(self, task: StoryTask) -> bool: + """True when the pending `safe_reset` would still erase uncommitted work — + the decision input for refusing a rollback whose snapshot failed (#340). + + Probes `verify.attempt_dirty` against *HEAD* rather than the attempt + baseline. That reports exactly the tracked edits and run-created untracked + files `safe_rollback` is about to drop, and ignores commits above baseline, + which `preserve_attempt_commits` has already parked — or paused on — by the + time this runs. So a capture failure over a tree whose content was all + committed reads as nothing-to-lose and the reset proceeds: a snapshot fault + must not halt an unattended run when the reset itself is harmless (#123). + + No ``exclude`` is passed because this only runs on the plain-rollback path, + where `rollback_or_pause`'s ``protected`` is empty anyway. Fails safe: an + un-determinable probe reads as work-at-risk, mirroring the dirty check's + own git-fault doctrine (#156).""" + workspace = self._workspace_get() + try: + head = verify.rev_parse_head(workspace.root) + return verify.attempt_dirty(workspace.root, head, task.baseline_untracked) + except verify.GitError: + return True + def pause_for_manual_recovery( - self, task: StoryTask, baseline: str, *, preserve_failed: bool = False + self, + task: StoryTask, + baseline: str, + *, + preserve_failed: bool = False, + snapshot_failed: bool = False, ) -> None: """Leave the tree untouched, surface bold manual-recovery instructions, and - pause the run. Always raises RunPaused. Three notice shapes: (a, default) + pause the run. Always raises RunPaused. Four notice shapes: (a, default) the OFF path for a stopped/abandoned in-place attempt with no commits of its own — plain manual-rollback steps; (b, ``preserve_failed``) rollback is ON/resolved but the attempt's commits above baseline could not be parked on @@ -413,9 +475,17 @@ def pause_for_manual_recovery( above its baseline (#100: a completed session whose run died before the orchestrator folded the result) — instructing a bare ``reset --hard`` there would discard finished, possibly already-pushed commits, so this notice - tells the operator to save and check integration state first. A *resolved* - escalation never reaches here — `_rollback_or_pause` auto-recovers that - human-initiated re-drive regardless of `scm.rollback_on_failure`.""" + tells the operator to save and check integration state first; (d, + ``snapshot_failed``) the uncommitted-work snapshot could not be captured and + the reset would have destroyed it (#340) — names the at-risk *working tree* + rather than commits, and offers a git-free rescue because the fault that + broke the snapshot may still be breaking git. + + The two flags are mutually exclusive by construction: they are raised from + different call sites, and `preserve_attempt_commits` pauses before + `preserve_attempt_worktree` ever runs. A *resolved* escalation never + reaches here — `_rollback_or_pause` auto-recovers that human-initiated + re-drive regardless of `scm.rollback_on_failure`.""" workspace = self._workspace_get() short = baseline[:12] or "" # Name the tree every instruction targets. Usually the main checkout, @@ -447,6 +517,33 @@ def pause_for_manual_recovery( "untracked files.\n" f"Then run `bmad-loop resume {self.state.run_id}`." ) + elif snapshot_failed: + # Name the committed half when it survived, so the operator is not left + # assuming the whole attempt is at risk. + parked = ( + f"The attempt's *committed* work is already parked at " + f"`{task.preserve_ref}`.\n" + if task.preserve_ref + else "" + ) + notice = ( + "**ACTION REQUIRED — uncommitted work could not be auto-preserved**\n" + f"Story **{task.story_key}**'s attempt left uncommitted changes, but the " + "recovery snapshot could not be captured, so the automatic rollback was " + "refused rather than `reset --hard` past (and permanently destroy) them. " + "Unlike committed work, an uncommitted edit a reset discards is NOT " + f"recoverable from the reflog. **Your working tree at `{root}` is " + "untouched.**\n" + " 1. **Save what you want to keep** — copy the files out, or " + f'`git -C "{root}" diff > rescue.patch` plus any new untracked files.\n' + " 2. Check the cause — the journal's `attempt-worktree-preserve-failed` " + "entry carries git's own error; a full disk is the most common one.\n" + " 3. Only once your work is safe: " + f'`git -C "{root}" reset --hard {short}`, then review/remove leftover ' + "untracked files.\n" + f"{parked}" + f"Then run `bmad-loop resume {self.state.run_id}`." + ) elif commits: notice = ( "**ACTION REQUIRED — manual recovery needed (committed work present)**\n" diff --git a/tests/test_engine.py b/tests/test_engine.py index fdb4ce8..e5d9307 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -4602,15 +4602,14 @@ def _fail(*a, **k): def test_rollback_worktree_preserve_failure_journals_git_error(project, monkeypatch): - """When the uncommitted-work snapshot can't be captured, the best-effort path still - resets (rollback ON, no commits above baseline -> no pause) but journals the underlying - git error, so a post-mortem can see WHY preservation failed — not just that it did.""" - policy = Policy( - gates=GatesPolicy(mode="none"), - notify=QUIET, - scm=ScmPolicy(rollback_on_failure=True), - ) - engine, _ = make_engine(project, [], policy=policy) + """When the uncommitted-work snapshot can't be captured, the re-drive path still + resets (pause-free by contract) but journals the underlying git error, so a + post-mortem can see WHY preservation failed — not just that it did. + + Driven through `cause="resolved"` since #340: a plain rollback now refuses this + reset rather than destroying the uncommitted edit, so the re-drive is where the + journal-and-proceed behavior survives.""" + engine, _ = make_engine(project, []) repo = project.project task = StoryTask(story_key="1-1-a", epic=1) task.baseline_commit = rev_parse_head(repo) @@ -4622,7 +4621,7 @@ def _fail(*a, **k): monkeypatch.setattr("bmad_loop.verify.snapshot_worktree", _fail) - engine._rollback_or_pause(task) # best-effort: journals + proceeds, never raises + engine._rollback_or_pause(task, cause="resolved") # journals + proceeds, never raises assert rev_parse_head(repo) == task.baseline_commit # reset still happened entry = next( @@ -4631,6 +4630,39 @@ def _fail(*a, **k): assert "simulated commit-tree failure" in entry["error"] # underlying git detail preserved +def test_rollback_refuses_reset_when_the_dirty_snapshot_fails(project, monkeypatch): + """#340 end-to-end through a real Engine: an in-place auto-rollback whose dirty + snapshot fails pauses instead of resetting, leaving the uncommitted work on disk + for the operator to rescue. The engine-level twin of the RecoveryFlow seam test. + + Ablation target: delete the `pause_for_manual_recovery(..., snapshot_failed=True)` + call from preserve_attempt_worktree's `except` and this fails.""" + policy = Policy( + gates=GatesPolicy(mode="none"), + notify=QUIET, + scm=ScmPolicy(rollback_on_failure=True), + ) + engine, _ = make_engine(project, [], policy=policy) + 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") + + def _fail(*a, **k): + raise GitError("simulated write-tree failure") + + monkeypatch.setattr("bmad_loop.verify.snapshot_worktree", _fail) + + with pytest.raises(RunPaused) as paused: + engine._rollback_or_pause(task) + + assert (repo / "src.txt").read_text() == "uncommitted edit\n" # NOT reset + reason = paused.value.reason + assert "uncommitted work could not be auto-preserved" in reason + assert "rollback-manual-required" in [e["kind"] for e in engine.journal.entries()] + + def test_rollback_pauses_when_preserve_fails(project, monkeypatch): """Safety invariant: if the recovery ref can't be created while commits exist, the engine pauses for manual recovery rather than resetting past the work — and diff --git a/tests/test_recovery_flow.py b/tests/test_recovery_flow.py index 97d8901..eb30605 100644 --- a/tests/test_recovery_flow.py +++ b/tests/test_recovery_flow.py @@ -345,7 +345,7 @@ def test_preserve_attempt_worktree_snapshots_dirty_tree(project): task.attempt = 2 (repo / "src.txt").write_text("uncommitted edit\n") # tracked file dirtied - flow.preserve_attempt_worktree(task) + flow.preserve_attempt_worktree(task, allow_pause=False) assert "attempt-worktree-preserved" in flow.journal.events() ref = flow.journal.fields("attempt-worktree-preserved")["ref"] @@ -384,17 +384,23 @@ def test_dirty_preserve_ref_wins_and_subsumes_the_commits_branch(project): def test_snapshot_failure_leaves_a_commits_only_ref_flagged_partial(project, monkeypatch): - """The snapshot raises *after* the commits branch was already parked, and the - reset runs anyway (best-effort contract). `preserve_ref` then names the commits - branch alone, so the whole-attempt promise no longer holds — `preserve_partial` - records that, and the defer notice downgrades its claim instead of telling the - operator a `merge --ff-only` restores work the reset just destroyed. + """The snapshot raises *after* the commits branch was already parked, and on a + re-drive the reset runs anyway. `preserve_ref` then names the commits branch + alone, so the whole-attempt promise no longer holds — `preserve_partial` records + that, and the defer notice downgrades its claim instead of telling the operator + a `merge --ff-only` restores work the reset just destroyed. + + Since #340 a *plain* rollback refuses this reset, so the re-drive + (`cause="resolved"`, contractually pause-free) is the surviving path where the + partial park is reachable — which is exactly why #338's downgrade is narrowed + rather than superseded. `rollback_on_failure` is left OFF so the re-drive is + what carries the auto-recover arm here, not the policy flag. Ablation target: delete `task.preserve_partial = True` from preserve_attempt_worktree's `except verify.GitError` block and this fails.""" repo = project.project ws = Workspace.default(project) - flow = _make_flow(workspace=ws, policy=_policy(rollback_on_failure=True)) + flow = _make_flow(workspace=ws) task = _task(repo) (repo / "src.txt").write_text("committed\n") git(repo, "add", "-A") @@ -406,8 +412,9 @@ def _fail(*_a, **_k): monkeypatch.setattr(verify, "snapshot_worktree", _fail) - flow.rollback_or_pause(task) + flow.rollback_or_pause(task, cause="resolved") + assert flow.calls.pauses == [] # a re-drive never pauses, even on a preserve failure assert "attempt-worktree-preserve-failed" in flow.journal.events() assert task.preserve_ref == flow.journal.fields("attempt-commits-preserved")["ref"] assert task.preserve_ref.startswith("attempt-preserve/") @@ -420,6 +427,150 @@ def _fail(*_a, **_k): assert git(repo, "show", f"{task.preserve_ref}:src.txt") == "committed" +def _fail_snapshot(monkeypatch, exc=None): + """Make verify.snapshot_worktree raise, the way a full disk or a git timeout + inside `add -u`/`write-tree`/`update-ref` would.""" + + def _fail(*_a, **_k): + raise exc if exc is not None else verify.GitError("simulated commit-tree failure") + + monkeypatch.setattr(verify, "snapshot_worktree", _fail) + + +def test_snapshot_failure_pauses_when_work_would_be_lost(project, monkeypatch): + """#340: a failed dirty snapshot refuses the reset instead of destroying the + uncommitted work it existed to capture. Unlike the commits path's orphaned + objects, a tracked edit a `reset --hard` discards is unrecoverable, so the + safety net becomes a gate. + + Ablation target: delete the `pause_for_manual_recovery(..., snapshot_failed=True)` + call from preserve_attempt_worktree's `except` and this fails.""" + repo = project.project + ws = Workspace.default(project) + flow = _make_flow(workspace=ws, policy=_policy(rollback_on_failure=True)) + task = _task(repo) + (repo / "src.txt").write_text("uncommitted work\n") + (repo / "new.txt").write_text("run-created untracked\n") + _fail_snapshot(monkeypatch) + + with pytest.raises(_Pause): + flow.rollback_or_pause(task) + + assert "attempt-worktree-preserve-failed" in flow.journal.events() + # the whole point: the tree is untouched, so the work is still there to rescue + assert (repo / "src.txt").read_text() == "uncommitted work\n" + assert (repo / "new.txt").is_file() + notice = flow.calls.pauses[0][0] + assert "uncommitted work could not be auto-preserved" in notice + assert "attempt-worktree-preserve-failed" in notice # names the diagnosis breadcrumb + # rescue first, discard second — and step 3 is what lets the pause terminate + # (reset, resume, rollback-skipped-clean). Anchor on the command, not the bare + # phrase: the prose above it also says `reset --hard`, so index() would match there. + assert notice.index("Save what you want to keep") < notice.index( + f'git -C "{repo}" reset --hard' + ) + + +def test_snapshot_failure_proceeds_when_nothing_would_be_lost(project, monkeypatch): + """The refusal is gated on there being something left to lose. An attempt that + committed everything has a tree clean vs HEAD, so the reset destroys nothing and + a snapshot fault must not halt an unattended run over it (cf. #123's false-pause + complaint). + + INVERSE ablation — `pauses == []` would also pass if the pause were simply + unreachable. Make the pause unconditional (drop the `_reset_would_destroy` + guard) and this must fail.""" + repo = project.project + ws = Workspace.default(project) + flow = _make_flow(workspace=ws, policy=_policy(rollback_on_failure=True)) + task = _task(repo) + (repo / "src.txt").write_text("committed\n") + git(repo, "add", "-A") + git(repo, "commit", "-q", "-m", "attempt commit") # nothing left uncommitted + _fail_snapshot(monkeypatch) + + flow.rollback_or_pause(task) + + assert flow.calls.pauses == [] + assert "attempt-worktree-preserve-failed" in flow.journal.events() + assert rev_parse_head(repo) == task.baseline_commit # the harmless reset ran + # and the committed half is still recoverable by name + assert git(repo, "show", f"{task.preserve_ref}:src.txt") == "committed" + + +def test_snapshot_failure_probe_fault_pauses(project, monkeypatch): + """A git fault in the at-risk probe itself must read as work-at-risk, never as + permission to reset — the same fail-safe direction as the dirty check (#156). + + Ablation target: flip `_reset_would_destroy`'s `except verify.GitError` to + `return False` and this fails.""" + repo = project.project + ws = Workspace.default(project) + flow = _make_flow(workspace=ws, policy=_policy(rollback_on_failure=True)) + task = _task(repo) + (repo / "src.txt").write_text("uncommitted work\n") + _fail_snapshot(monkeypatch) + + def _fail_probe(*_a, **_k): + raise verify.GitError("simulated rev-parse failure") + + monkeypatch.setattr(verify, "rev_parse_head", _fail_probe) + + with pytest.raises(_Pause): + flow.rollback_or_pause(task) + + assert (repo / "src.txt").read_text() == "uncommitted work\n" # not reset + + +def test_snapshot_failure_never_pauses_on_redrive(project, monkeypatch): + """The re-drive's pause-free contract outranks the #340 gate: the operator + already directed this discard through the resolve workflow, so a failed capture + journals and lets the reset run. + + Ablation target: delete `if not allow_pause: return` from the `except` and this + fails.""" + repo = project.project + ws = Workspace.default(project) + flow = _make_flow(workspace=ws) + task = _task(repo) + (repo / "src.txt").write_text("uncommitted work\n") + _fail_snapshot(monkeypatch) + + flow.rollback_or_pause(task, cause="resolved") + + assert flow.calls.pauses == [] + assert task.preserve_partial is True # still latched — the notice must downgrade + assert (repo / "src.txt").read_text() == "original\n" # reset ran + + +def test_snapshot_failure_pause_names_the_unit_worktree(project, tmp_path, monkeypatch): + """#161 compatibility: a preserve-failure pause can fire while a unit worktree is + mounted, and every instruction must target that tree. Naming the main checkout + there quotes a HEAD the attempt never moved and invites a destructive reset of a + tree the operator never worked in. + + Ablation target: change `pause_for_manual_recovery`'s `root` back to + `self.paths.repo_root` and this fails.""" + repo = project.project + ws = Workspace.default(project) + main_checkout = tmp_path / "some-other-main-checkout" + flow = _make_flow( + workspace=ws, + paths=SimpleNamespace(repo_root=main_checkout), + policy=_policy(rollback_on_failure=False), # in-worktree recovery ignores it + ) + task = _task(repo) + (repo / "src.txt").write_text("worktree attempt\n") + _fail_snapshot(monkeypatch) + + with pytest.raises(_Pause): + flow.rollback_or_pause(task) + + notice = flow.calls.pauses[0][0] + assert str(repo) in notice + assert str(main_checkout) not in notice + + def test_rollback_clears_a_previous_attempts_preserve_ref(project, monkeypatch): """Ablation target: delete the `task.preserve_ref = None` at the top of RecoveryFlow's auto-recover arm and this fails. A later rollback that parks From 917c9450c1e0fc5a35e2d04c2f202d720ad92452 Mon Sep 17 00:00:00 2001 From: pbean Date: Mon, 27 Jul 2026 18:03:12 -0700 Subject: [PATCH 2/9] fix(verify): fail loud when the preserve snapshot cannot be taken MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit safe_rollback swallowed a failed `git stash create` into an empty snapshot, which disabled the entire preserve restore below it. The hard reset then reverted exactly the paths the caller asked to keep — a resolved re-drive's corrected spec — with no error raised or journalled anywhere, which is the regression the preserve argument exists to prevent. Raise before the reset, scoped to callers that requested a restore: with no preserve the snapshot is unused, so both sweep call sites keep degrading. A clean tree still exits rc 0 with empty output and is unaffected. --- CHANGELOG.md | 8 ++++++ src/bmad_loop/verify.py | 16 +++++++++-- tests/test_verify.py | 62 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 84 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 303b87b..8a2d094 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,14 @@ breaking changes may land in a minor release. commits-only (#338). Inert under the shipped defaults, where the snapshot is reached only on a re-drive; it bites `scm.rollback_on_failure = true` and in-worktree dev retries. +### Fixed + +- **`safe_rollback` no longer swallows a failed `git stash create`.** The empty snapshot silently + disabled the whole `preserve` restore, so the hard reset reverted exactly the paths the caller + asked to keep — a resolved re-drive's corrected spec — with no error anywhere. It now raises + before the reset, and only when a restore was actually requested, so the no-`preserve` callers + (both sweep sites) degrade as before. + - **Defer notifications name where the work survives (#333).** A deferred story's rollback parked the attempt on an `attempt-preserve/*` branch (or a `refs/attempt-preserve-dirty/*` snapshot), but the ref only ever reached `journal.jsonl` — the notification carried a bare reason, leaving diff --git a/src/bmad_loop/verify.py b/src/bmad_loop/verify.py index e0572b3..15065f7 100644 --- a/src/bmad_loop/verify.py +++ b/src/bmad_loop/verify.py @@ -529,8 +529,11 @@ def safe_rollback( resolve workflow just corrected. The `git reset --hard` would otherwise revert them (keep only guards untracked deletion). We snapshot the current tree with `git stash create`, reset, then restore those paths from the - snapshot. Untracked artifacts need no special handling: the reset leaves them - alone and the cleanup below skips `keep` dirs. + snapshot. If that snapshot cannot be taken we raise *before* the reset rather + than proceed with an empty one: skipping the restore would revert exactly what + `preserve` names, and it would do so silently. Untracked artifacts need no + special handling: the reset leaves them alone and the cleanup below skips + `keep` dirs. `policy.toml` (the operator's orchestration config) is *always* restored, regardless of `preserve`. It lives inside the kept `.bmad-loop` dir but is @@ -549,6 +552,15 @@ def safe_rollback( policy_content = policy_path.read_bytes() if policy_path.is_file() else None rc, out = _git(repo, "stash", "create") + # A failed `stash create` silently empties `snapshot`, which disables the whole + # preserve restore below — the reset would then revert the very paths the caller + # asked to keep (a resolved re-drive's corrected spec), with no error anywhere. + # Raise before the reset, and only when a restore was actually requested: with + # no `preserve` the snapshot is unused, so the degrade stays correct there (both + # sweep callers rely on it). A clean tree is not a failure — it exits rc 0 with + # empty output, which the line below keeps handling as "nothing to restore from". + if rc != 0 and preserve: + raise GitError(f"git stash create failed in {repo}: {out}") snapshot = out.strip() if rc == 0 else "" rc, out = _git(repo, "reset", "--hard", baseline) if rc != 0: diff --git a/tests/test_verify.py b/tests/test_verify.py index c40d269..945c4b1 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -1295,6 +1295,68 @@ def fake_git(r, *args): ) +def test_safe_rollback_raises_when_the_preserve_snapshot_cannot_be_taken(project, monkeypatch): + """A failed `git stash create` used to be swallowed into an empty snapshot, which + silently disabled the whole restore — so the reset reverted exactly the paths + `preserve` names (a resolved re-drive's corrected spec) with no error anywhere. + Raise before the reset instead. + + Ablation target: drop the `if rc != 0 and preserve: raise` from safe_rollback + and this fails — the spec comes back reverted, quietly.""" + repo = project.project + spec = project.implementation_artifacts / "spec-1-1-a.md" + spec.write_text("frozen: original\n") + git(repo, "add", "-A") + git(repo, "commit", "-q", "-m", "spec") + baseline = verify.rev_parse_head(repo) + snap = sorted(verify.untracked_files(repo)) + artifact_rel = project.implementation_artifacts.relative_to(repo).as_posix() + spec.write_text("frozen: corrected\n") + + real_git = verify._git + + def fake_git(r, *args): + if args[:2] == ("stash", "create"): + return 1, "fatal: unable to write temporary index" + return real_git(r, *args) + + monkeypatch.setattr(verify, "_git", fake_git) + with pytest.raises(verify.GitError, match="git stash create"): + verify.safe_rollback( + repo, + baseline, + baseline_untracked=snap, + keep=(".bmad-loop", artifact_rel), + preserve=(artifact_rel,), + ) + # raised BEFORE the reset, so the correction is still on disk to be re-tried + assert spec.read_text() == "frozen: corrected\n" + + +def test_safe_rollback_degrades_stash_failure_when_nothing_to_preserve(project, monkeypatch): + """The raise is scoped to callers that asked for a restore. With no `preserve` + the snapshot is unused, so a `stash create` failure stays a non-event — both + sweep call sites reset with no preserve and must not start failing. + + INVERSE ablation: drop the `and preserve` guard and this fails.""" + repo = project.project + baseline = verify.rev_parse_head(repo) + snap = sorted(verify.untracked_files(repo)) + (repo / "src.txt").write_text("dev attempt\n") + + real_git = verify._git + + def fake_git(r, *args): + if args[:2] == ("stash", "create"): + return 1, "fatal: unable to write temporary index" + return real_git(r, *args) + + monkeypatch.setattr(verify, "_git", fake_git) + verify.safe_rollback(repo, baseline, baseline_untracked=snap) # no preserve + + assert (repo / "src.txt").read_text() == "original\n" # reset still happened + + def test_safe_rollback_tolerates_empty_preserve_dir(project): """A `preserve` dir with no tracked content in the snapshot makes checkout exit non-zero ('did not match') — benign, must NOT raise.""" From f48a703027662baa75df22075a3dcfea986e89f4 Mon Sep 17 00:00:00 2001 From: pbean Date: Mon, 27 Jul 2026 18:03:45 -0700 Subject: [PATCH 3/9] fix(recovery): degrade a snapshot OSError into the typed preserve path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _run_git translates only a timeout, so a spawn-level EMFILE/ENOMEM from the snapshot's nine git children escaped as a bare OSError — past every except GitError guard, out of the middle of a rollback, into the crash handler. The sibling guards (is_ancestor, worktree_prune) already catch both. Widen the caller, not the reset: capturing the attempt is observation and may degrade, so it joins the journal-and-decide path; safe_reset is the repair write and still raises. --- CHANGELOG.md | 6 ++++++ src/bmad_loop/recovery_flow.py | 15 +++++++++++---- tests/test_recovery_flow.py | 23 +++++++++++++++++++++++ 3 files changed, 40 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8a2d094..9ffa0c5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,12 @@ breaking changes may land in a minor release. before the reset, and only when a restore was actually requested, so the no-`preserve` callers (both sweep sites) degrade as before. +- **A spawn-level `OSError` during the attempt snapshot no longer crashes the run.** `_run_git` + translates only a timeout, so an EMFILE/ENOMEM from `subprocess.run` escaped untyped out of the + middle of a rollback. Preservation is observation rather than a repair write, so it now degrades + into the same journal-and-decide path a `GitError` takes, keeping the errno as the breadcrumb; + `safe_reset` still raises. + - **Defer notifications name where the work survives (#333).** A deferred story's rollback parked the attempt on an `attempt-preserve/*` branch (or a `refs/attempt-preserve-dirty/*` snapshot), but the ref only ever reached `journal.jsonl` — the notification carried a bare reason, leaving diff --git a/src/bmad_loop/recovery_flow.py b/src/bmad_loop/recovery_flow.py index dd72981..02ba069 100644 --- a/src/bmad_loop/recovery_flow.py +++ b/src/bmad_loop/recovery_flow.py @@ -399,10 +399,17 @@ def preserve_attempt_worktree(self, task: StoryTask, *, allow_pause: bool) -> No parked = verify.snapshot_worktree( workspace.root, ref, baseline_untracked=task.baseline_untracked ) - except verify.GitError as exc: - # Keep the git failure detail (commit-tree/update-ref stderr): if the - # following reset destroys work, this is the only breadcrumb explaining - # why the safety-net snapshot couldn't be captured. + except (verify.GitError, OSError) as exc: + # OSError alongside GitError: `_run_git` only translates a timeout, so a + # spawn-level EMFILE/ENOMEM escapes untyped (the sibling guards at + # verify.is_ancestor / worktree_prune catch both). Uncaught it crashed the + # run here — after the commits ref, before the reset — which is the safe + # outcome reached the loudest possible way. Preservation is observation, + # so it degrades into the decision below; `safe_reset` is the repair + # write and still raises. + # Keep the failure detail (commit-tree/update-ref stderr, or the errno): + # if the reset that may follow destroys work, this is the only breadcrumb + # explaining why the safety-net snapshot couldn't be captured. self.journal.append( "attempt-worktree-preserve-failed", story_key=task.story_key, error=str(exc) ) diff --git a/tests/test_recovery_flow.py b/tests/test_recovery_flow.py index eb30605..aa5fd90 100644 --- a/tests/test_recovery_flow.py +++ b/tests/test_recovery_flow.py @@ -543,6 +543,29 @@ def test_snapshot_failure_never_pauses_on_redrive(project, monkeypatch): assert (repo / "src.txt").read_text() == "original\n" # reset ran +def test_snapshot_oserror_degrades_into_the_typed_path(project, monkeypatch): + """`_run_git` translates only a timeout, so a spawn-level OSError (EMFILE/ENOMEM) + escapes untyped. Preservation is observation, not a repair write, so it degrades + into the same journal-and-decide path a GitError takes rather than crashing the + run mid-rollback. + + Ablation target: narrow the `except` back to `verify.GitError` and this fails + with the raw OSError.""" + repo = project.project + ws = Workspace.default(project) + flow = _make_flow(workspace=ws, policy=_policy(rollback_on_failure=True)) + task = _task(repo) + (repo / "src.txt").write_text("uncommitted work\n") + _fail_snapshot(monkeypatch, OSError(24, "Too many open files")) + + with pytest.raises(_Pause): # the typed pause, not the OSError + flow.rollback_or_pause(task) + + entry = flow.journal.fields("attempt-worktree-preserve-failed") + assert "Too many open files" in entry["error"] # errno detail kept as a breadcrumb + assert (repo / "src.txt").read_text() == "uncommitted work\n" + + def test_snapshot_failure_pause_names_the_unit_worktree(project, tmp_path, monkeypatch): """#161 compatibility: a preserve-failure pause can fire while a unit worktree is mounted, and every instruction must target that tree. Naming the main checkout From 520a81c10db69fb047969f5297e542cb01b6280f Mon Sep 17 00:00:00 2001 From: pbean Date: Mon, 27 Jul 2026 18:07:28 -0700 Subject: [PATCH 4/9] docs(changelog): file the #340 entries under the existing Unreleased sections The first pass created duplicate Changed/Fixed headings inside Unreleased and dropped the Added heading, orphaning the #333 entry. Caught by markdownlint MD024. --- CHANGELOG.md | 58 ++++++++++++++++------------------ src/bmad_loop/recovery_flow.py | 3 +- 2 files changed, 29 insertions(+), 32 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ffa0c5..2ac7767 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,36 +7,7 @@ breaking changes may land in a minor release. ## [Unreleased] -### Changed - -- **A failed worktree snapshot now blocks the rollback reset (#340).** The two preserve steps were - asymmetric: an auto-rollback refused to reset past commits it could not park, but a failed - *uncommitted*-work snapshot was journaled and the `reset --hard` ran anyway — destroying the - tracked edits and run-created untracked files the snapshot existed to capture. That protected the - more recoverable half; orphaned commits stay reachable by reflog until `gc`, while a discarded - uncommitted edit is gone. Both paths now refuse on the same terms, pausing with rescue - instructions that name the tree (the mounted worktree when there is one) and offer a git-free - copy-out, since the fault that broke the snapshot may still be breaking git. - - Gated on there being something left to lose: a capture failure over a tree whose content was all - committed still resets, so a git fault can't halt an unattended run over a harmless reset. A - resolved re-drive stays pause-free by contract — it journals, resets, and flags the park - commits-only (#338). Inert under the shipped defaults, where the snapshot is reached only on a - re-drive; it bites `scm.rollback_on_failure = true` and in-worktree dev retries. - -### Fixed - -- **`safe_rollback` no longer swallows a failed `git stash create`.** The empty snapshot silently - disabled the whole `preserve` restore, so the hard reset reverted exactly the paths the caller - asked to keep — a resolved re-drive's corrected spec — with no error anywhere. It now raises - before the reset, and only when a restore was actually requested, so the no-`preserve` callers - (both sweep sites) degrade as before. - -- **A spawn-level `OSError` during the attempt snapshot no longer crashes the run.** `_run_git` - translates only a timeout, so an EMFILE/ENOMEM from `subprocess.run` escaped untyped out of the - middle of a rollback. Preservation is observation rather than a repair write, so it now degrades - into the same journal-and-decide path a `GitError` takes, keeping the errno as the breadcrumb; - `safe_reset` still raises. +### Added - **Defer notifications name where the work survives (#333).** A deferred story's rollback parked the attempt on an `attempt-preserve/*` branch (or a `refs/attempt-preserve-dirty/*` snapshot), @@ -130,6 +101,21 @@ story `, the same annotation a sweep bundle writes. Both sprint and stories ### Changed +- **A failed worktree snapshot now blocks the rollback reset (#340).** The two preserve steps were + asymmetric: an auto-rollback refused to reset past commits it could not park, but a failed + _uncommitted_-work snapshot was journaled and the `reset --hard` ran anyway — destroying the + tracked edits and run-created untracked files the snapshot existed to capture. That protected the + more recoverable half; orphaned commits stay reachable by reflog until `gc`, while a discarded + uncommitted edit is gone. Both paths now refuse on the same terms, pausing with rescue + instructions that name the tree (the mounted worktree when there is one) and offer a git-free + copy-out, since the fault that broke the snapshot may still be breaking git. + + Gated on there being something left to lose: a capture failure over a tree whose content was all + committed still resets, so a git fault can't halt an unattended run over a harmless reset. A + resolved re-drive stays pause-free by contract — it journals, resets, and flags the park + commits-only (#338). Inert under the shipped defaults, where the snapshot is reached only on a + re-drive; it bites `scm.rollback_on_failure = true` and in-worktree dev retries. + - **A review that revokes the sprint sign-off now escalates instead of burning review cycles (#334).** The orchestrator advances `sprint-status` to `done` at dev time; a review session that judges the story unfinished and writes the board back to an earlier stage contradicts that — and @@ -178,6 +164,18 @@ story `, the same annotation a sweep bundle writes. Both sprint and stories ### Fixed +- **`safe_rollback` no longer swallows a failed `git stash create`.** The empty snapshot silently + disabled the whole `preserve` restore, so the hard reset reverted exactly the paths the caller + asked to keep — a resolved re-drive's corrected spec — with no error anywhere. It now raises + before the reset, and only when a restore was actually requested, so the no-`preserve` callers + (both sweep sites) degrade as before. + +- **A spawn-level `OSError` during the attempt snapshot no longer crashes the run.** `_run_git` + translates only a timeout, so an EMFILE/ENOMEM from `subprocess.run` escaped untyped out of the + middle of a rollback. Preservation is observation rather than a repair write, so it now degrades + into the same journal-and-decide path a `GitError` takes, keeping the errno as the breadcrumb; + `safe_reset` still raises. + - **The orchestrator's ledger writers no longer inject lines from a multiline value (#305).** Found by [@Haven2026](https://github.com/Haven2026) in #274. The deferred-work ledger is line-oriented, but `deferredwork`'s mutators interpolated their arguments verbatim: a resolution note of diff --git a/src/bmad_loop/recovery_flow.py b/src/bmad_loop/recovery_flow.py index 02ba069..458f3a6 100644 --- a/src/bmad_loop/recovery_flow.py +++ b/src/bmad_loop/recovery_flow.py @@ -528,8 +528,7 @@ def pause_for_manual_recovery( # Name the committed half when it survived, so the operator is not left # assuming the whole attempt is at risk. parked = ( - f"The attempt's *committed* work is already parked at " - f"`{task.preserve_ref}`.\n" + f"The attempt's *committed* work is already parked at `{task.preserve_ref}`.\n" if task.preserve_ref else "" ) From a85d0e66bb2992f53e8525a542ceed54a7e061fa Mon Sep 17 00:00:00 2001 From: pbean Date: Mon, 27 Jul 2026 18:26:12 -0700 Subject: [PATCH 5/9] fix(recovery): fail safe on an OSError in the at-risk probe too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by CodeRabbit and Greptile independently on #341. The previous commit widened preserve_attempt_worktree to catch OSError, but _reset_would_destroy — which runs immediately after that fault, against the same git binary — still guarded verify.GitError alone. The EMFILE/ENOMEM that broke the capture is exactly what then breaks rev_parse_head/attempt_dirty, so the errno escaped the probe and crashed the rollback anyway. The asymmetry between the sibling handlers was the defect, not either half. Also collapse the duplicated snapshot-failure stub onto _fail_snapshot. --- src/bmad_loop/recovery_flow.py | 10 +++++-- tests/test_recovery_flow.py | 51 ++++++++++++++++++++++++---------- 2 files changed, 45 insertions(+), 16 deletions(-) diff --git a/src/bmad_loop/recovery_flow.py b/src/bmad_loop/recovery_flow.py index 458f3a6..c30c1fe 100644 --- a/src/bmad_loop/recovery_flow.py +++ b/src/bmad_loop/recovery_flow.py @@ -455,12 +455,18 @@ def _reset_would_destroy(self, task: StoryTask) -> bool: No ``exclude`` is passed because this only runs on the plain-rollback path, where `rollback_or_pause`'s ``protected`` is empty anyway. Fails safe: an un-determinable probe reads as work-at-risk, mirroring the dirty check's - own git-fault doctrine (#156).""" + own git-fault doctrine (#156). + + Catches ``OSError`` for the same reason the caller does, and it matters more + here: this runs immediately after a snapshot fault, against the same git + binary, so the EMFILE/ENOMEM that broke the capture is likely to break the + probe too. Guarding only `GitError` would undo the broadening one frame up + and crash the rollback anyway.""" workspace = self._workspace_get() try: head = verify.rev_parse_head(workspace.root) return verify.attempt_dirty(workspace.root, head, task.baseline_untracked) - except verify.GitError: + except (verify.GitError, OSError): return True def pause_for_manual_recovery( diff --git a/tests/test_recovery_flow.py b/tests/test_recovery_flow.py index aa5fd90..b17b1d3 100644 --- a/tests/test_recovery_flow.py +++ b/tests/test_recovery_flow.py @@ -383,6 +383,16 @@ def test_dirty_preserve_ref_wins_and_subsumes_the_commits_branch(project): assert task.preserve_partial is False +def _fail_snapshot(monkeypatch, exc=None): + """Make verify.snapshot_worktree raise, the way a full disk or a git timeout + inside `add -u`/`write-tree`/`update-ref` would.""" + + def _fail(*_a, **_k): + raise exc if exc is not None else verify.GitError("simulated commit-tree failure") + + monkeypatch.setattr(verify, "snapshot_worktree", _fail) + + def test_snapshot_failure_leaves_a_commits_only_ref_flagged_partial(project, monkeypatch): """The snapshot raises *after* the commits branch was already parked, and on a re-drive the reset runs anyway. `preserve_ref` then names the commits branch @@ -407,10 +417,7 @@ def test_snapshot_failure_leaves_a_commits_only_ref_flagged_partial(project, mon git(repo, "commit", "-q", "-m", "attempt commit") (repo / "src.txt").write_text("committed then edited\n") # the half that will be lost - def _fail(*_a, **_k): - raise verify.GitError("simulated commit-tree failure") - - monkeypatch.setattr(verify, "snapshot_worktree", _fail) + _fail_snapshot(monkeypatch) flow.rollback_or_pause(task, cause="resolved") @@ -427,16 +434,6 @@ def _fail(*_a, **_k): assert git(repo, "show", f"{task.preserve_ref}:src.txt") == "committed" -def _fail_snapshot(monkeypatch, exc=None): - """Make verify.snapshot_worktree raise, the way a full disk or a git timeout - inside `add -u`/`write-tree`/`update-ref` would.""" - - def _fail(*_a, **_k): - raise exc if exc is not None else verify.GitError("simulated commit-tree failure") - - monkeypatch.setattr(verify, "snapshot_worktree", _fail) - - def test_snapshot_failure_pauses_when_work_would_be_lost(project, monkeypatch): """#340: a failed dirty snapshot refuses the reset instead of destroying the uncommitted work it existed to capture. Unlike the commits path's orphaned @@ -522,6 +519,32 @@ def _fail_probe(*_a, **_k): assert (repo / "src.txt").read_text() == "uncommitted work\n" # not reset +def test_probe_oserror_also_fails_safe(project, monkeypatch): + """The probe runs immediately after a snapshot fault, against the same git + binary, so the EMFILE/ENOMEM that broke the capture is likely to break the probe + too. Catching only GitError here would undo the broadening one frame up and + crash the rollback anyway — the asymmetry, not either half, is the defect. + + Ablation target: narrow `_reset_would_destroy`'s `except` back to + `verify.GitError` and this fails with the raw OSError.""" + repo = project.project + ws = Workspace.default(project) + flow = _make_flow(workspace=ws, policy=_policy(rollback_on_failure=True)) + task = _task(repo) + (repo / "src.txt").write_text("uncommitted work\n") + _fail_snapshot(monkeypatch, OSError(24, "Too many open files")) + + def _fail_probe(*_a, **_k): + raise OSError(24, "Too many open files") + + monkeypatch.setattr(verify, "rev_parse_head", _fail_probe) + + with pytest.raises(_Pause): # the typed pause, not the OSError + flow.rollback_or_pause(task) + + assert (repo / "src.txt").read_text() == "uncommitted work\n" # not reset + + def test_snapshot_failure_never_pauses_on_redrive(project, monkeypatch): """The re-drive's pause-free contract outranks the #340 gate: the operator already directed this discard through the resolve workflow, so a failed capture From 25ffb3c7afbb3a3f06f6c32fc1d36c81d4804733 Mon Sep 17 00:00:00 2001 From: pbean Date: Mon, 27 Jul 2026 18:39:34 -0700 Subject: [PATCH 6/9] fix(recovery): keep the notice's advisory probe from swallowing the pause MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third link in the same chain, found by Greptile on #341. pause_for_manual_ recovery probes commits_above to choose its notice shape, guarded by a comment saying a git fault there must not block the pause itself — but it caught only verify.GitError, so an untranslated OSError lost the pause the caller had already decided to take. That probe is likelier to hit EMFILE on the new snapshot_failed path than anywhere else: the fault that broke the capture is still in force when we come to write the notice. Degrading to "no commits" costs only notice shape; the crash cost the pause. --- src/bmad_loop/recovery_flow.py | 9 ++++++-- tests/test_recovery_flow.py | 39 ++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/src/bmad_loop/recovery_flow.py b/src/bmad_loop/recovery_flow.py index c30c1fe..194f93c 100644 --- a/src/bmad_loop/recovery_flow.py +++ b/src/bmad_loop/recovery_flow.py @@ -510,10 +510,15 @@ def pause_for_manual_recovery( root = workspace.root commits: list[str] = [] if baseline: - # advisory probe: a git fault here must not block the pause itself + # Advisory probe: a git fault here must not block the pause itself — + # including an untranslated spawn-level OSError, which is *likelier* on + # the snapshot_failed path than anywhere else (the EMFILE/ENOMEM that + # broke the capture is still in force when we come to write the notice). + # Degrading to "no commits" only costs notice shape (c); crashing here + # would lose the pause the caller already decided to take. try: commits = verify.commits_above(root, baseline) - except verify.GitError: + except (verify.GitError, OSError): commits = [] if preserve_failed: notice = ( diff --git a/tests/test_recovery_flow.py b/tests/test_recovery_flow.py index b17b1d3..49cb835 100644 --- a/tests/test_recovery_flow.py +++ b/tests/test_recovery_flow.py @@ -589,6 +589,45 @@ def test_snapshot_oserror_degrades_into_the_typed_path(project, monkeypatch): assert (repo / "src.txt").read_text() == "uncommitted work\n" +def test_notice_probe_oserror_does_not_swallow_the_pause(project, monkeypatch): + """`pause_for_manual_recovery`'s advisory `commits_above` probe runs while the + fault that broke the snapshot is still in force, so it is the likeliest place + for a second EMFILE. Its own comment says a git fault there must not block the + pause — an uncaught OSError did exactly that, losing a pause the caller had + already decided to take. + + Ablation target: narrow that probe's `except` back to `verify.GitError` and this + fails with the raw OSError instead of pausing.""" + repo = project.project + ws = Workspace.default(project) + flow = _make_flow(workspace=ws, policy=_policy(rollback_on_failure=True)) + task = _task(repo) + (repo / "src.txt").write_text("uncommitted work\n") + _fail_snapshot(monkeypatch, OSError(24, "Too many open files")) + + # Only the *notice's* probe may fail: preserve_attempt_commits calls + # commits_above first, and breaking that would abort the rollback earlier and + # never reach the code under test. + real_commits_above = verify.commits_above + seen = {"n": 0} + + def _flaky(*a, **k): + seen["n"] += 1 + if seen["n"] == 1: + return real_commits_above(*a, **k) + raise OSError(24, "Too many open files") + + monkeypatch.setattr(verify, "commits_above", _flaky) + + with pytest.raises(_Pause): + flow.rollback_or_pause(task) + + assert seen["n"] >= 2 # the advisory probe really was reached and really failed + notice = flow.calls.pauses[0][0] + assert "uncommitted work could not be auto-preserved" in notice # shape (d) intact + assert (repo / "src.txt").read_text() == "uncommitted work\n" # not reset + + def test_snapshot_failure_pause_names_the_unit_worktree(project, tmp_path, monkeypatch): """#161 compatibility: a preserve-failure pause can fire while a unit worktree is mounted, and every instruction must target that tree. Naming the main checkout From 9edeb5ff216e769a6618fd6dd894506b0813b2eb Mon Sep 17 00:00:00 2001 From: pbean Date: Mon, 27 Jul 2026 19:16:33 -0700 Subject: [PATCH 7/9] docs(recovery): correct the preserve contract and guard-symmetry claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit snapshot_worktree's docstring still described the best-effort "journal and proceed" contract that #340 reversed, contradicting preserve_attempt_worktree in the same change. It also promised GitError on any failure, though _run_git translates only a timeout and the TemporaryDirectory can raise OSError before any git child is spawned. Also qualify "both paths now refuse on the same terms": the symmetry is in the refusal policy, not the guard — preserve_attempt_commits still catches GitError alone, so a spawn fault there crashes instead of refusing (#343). --- src/bmad_loop/recovery_flow.py | 5 ++++- src/bmad_loop/verify.py | 14 ++++++++++---- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/src/bmad_loop/recovery_flow.py b/src/bmad_loop/recovery_flow.py index 194f93c..91901e4 100644 --- a/src/bmad_loop/recovery_flow.py +++ b/src/bmad_loop/recovery_flow.py @@ -375,7 +375,10 @@ def preserve_attempt_worktree(self, task: StoryTask, *, allow_pause: bool) -> No same terms: with ``allow_pause`` (a plain rollback) pause for manual recovery rather than reset; on a re-drive (``allow_pause=False``) the caller's contract forbids pausing, so journal and let the human-directed - reset proceed. + reset proceed. The symmetry is in the *refusal policy*, not the guard: this + path catches ``(GitError, OSError)`` while `preserve_attempt_commits` still + catches ``GitError`` alone, so a spawn-level fault there crashes instead of + refusing — one instance of the systemic gap tracked in #343. The refusal is gated on :meth:`_reset_would_destroy`, so a capture failure over a tree with nothing left to lose (commits already parked, nothing diff --git a/src/bmad_loop/verify.py b/src/bmad_loop/verify.py index 15065f7..5f44929 100644 --- a/src/bmad_loop/verify.py +++ b/src/bmad_loop/verify.py @@ -453,10 +453,16 @@ def snapshot_worktree( Returns ``ref_name`` on success, or ``None`` when the tree is clean relative to HEAD (nothing to preserve — the intended non-destructive uncommitted-revert case). Raises :class:`GitError` on any git failure — the raise *surfaces* the - capture failure so the caller can decide: the commit-preservation caller - refuses to reset past unpreserved work, while the best-effort worktree caller - journals the failure and proceeds (the recovery ref is a safety net, not a - gate).""" + capture failure so the caller can decide. Since #340 the recovery ref is a + gate, not a safety net: the worktree caller's old best-effort "journal the + failure and proceed" contract is gone, and a plain rollback now refuses to + reset past work it could not park (only a re-drive, whose caller contract + forbids pausing, still journals and lets the human-directed reset run). + + Not every failure here is a :class:`GitError`: ``_run_git`` translates only a + timeout, so a spawn-level ``OSError`` escapes untyped, and the + ``TemporaryDirectory`` below can raise one outright (ENOSPC/EMFILE) before any + git child is spawned. Callers guard ``(GitError, OSError)`` (#343).""" head = rev_parse_head(repo) with tempfile.TemporaryDirectory() as td: env = {**os.environ, "GIT_INDEX_FILE": str(Path(td) / "index")} From 6383901eef0f3a641b259fb64a5be66ef1d3d081 Mon Sep 17 00:00:00 2001 From: pbean Date: Mon, 27 Jul 2026 19:31:25 -0700 Subject: [PATCH 8/9] fix(recovery): guard the commit-range enumeration in the rollback path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit preserve_attempt_commits read the range above baseline with no try at all, so an ordinary git timeout — which _run_git does translate, and which every sibling on this path already treats as routine — crashed the run mid-rollback. A spawn-level OSError did the same there and at the ref write, and at the rollback's dirty check one frame earlier. An un-determinable range now takes the preservation-failure path rather than the `not commits` early return, which would have let the reset run blind: it refuses under allow_pause and journals on a re-drive, matching the contract an un-parked ref already followed. Journalled as attempt-preserve-enumerate-failed so a post-mortem can distinguish it from a ref that would not take — only the latter can report a HEAD. These are the four frames upstream of the three #341 already covered; the repo-wide question of translating OSError at the _run_git chokepoint stays open in #343. Tests parametrize over exception *factories*: shared instances are built once at collection and re-raising mutates __traceback__, which under pytest-randomly made the cases couple and fail for the wrong reason. --- CHANGELOG.md | 10 +++ src/bmad_loop/recovery_flow.py | 54 ++++++++++----- tests/test_recovery_flow.py | 120 +++++++++++++++++++++++++++++++++ 3 files changed, 169 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ac7767..70966b7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -176,6 +176,16 @@ story `, the same annotation a sweep bundle writes. Both sprint and stories into the same journal-and-decide path a `GitError` takes, keeping the errno as the breadcrumb; `safe_reset` still raises. +- **A git fault while counting the attempt's commits no longer crashes the rollback (#343).** + `preserve_attempt_commits` enumerated the range above baseline with no guard at all, so an + ordinary git _timeout_ — already translated and treated as routine everywhere else — took the + run down mid-rollback; a spawn-level `OSError` did the same, there and at the ref write. An + un-determinable range now reads as "there may be work above baseline" and refuses the reset + (pausing for manual recovery, or journalling on a re-drive), never the clean-tree early return. + Journals `attempt-preserve-enumerate-failed`, distinct from `attempt-preserve-failed`, so a + post-mortem can tell "could not count the work" from "counted it but could not park it". The + rollback's dirty check degrades on an `OSError` for the same reason. + - **The orchestrator's ledger writers no longer inject lines from a multiline value (#305).** Found by [@Haven2026](https://github.com/Haven2026) in #274. The deferred-work ledger is line-oriented, but `deferredwork`'s mutators interpolated their arguments verbatim: a resolution note of diff --git a/src/bmad_loop/recovery_flow.py b/src/bmad_loop/recovery_flow.py index 91901e4..b002c0d 100644 --- a/src/bmad_loop/recovery_flow.py +++ b/src/bmad_loop/recovery_flow.py @@ -154,7 +154,7 @@ def rollback_or_pause(self, task: StoryTask, *, cause: str = "stopped") -> None: task.baseline_untracked, exclude=protected, ) - except verify.GitError as exc: + except (verify.GitError, OSError) as exc: self.journal.append( "rollback-dirty-check-failed", story_key=task.story_key, error=str(exc) ) @@ -315,19 +315,44 @@ def preserve_attempt_commits(self, task: StoryTask, *, allow_pause: bool) -> Non recoverable by name, not just the reflog). No-op when the attempt added no commits — an uncommitted-only revert is the intended, non-destructive case. - If commits exist but the ref cannot be created: with ``allow_pause`` (a - plain rollback) refuse to reset — pause for manual recovery rather than - destroy the work. On a re-drive (``allow_pause=False``) the caller's - contract forbids pausing, so journal the failure and let the reset proceed - (the re-drive is a human-directed discard of the failed attempt).""" + If commits exist but the ref cannot be created — or the range cannot be + enumerated at all, which is the same thing one step earlier: with + ``allow_pause`` (a plain rollback) refuse to reset — pause for manual + recovery rather than destroy the work. On a re-drive (``allow_pause=False``) + the caller's contract forbids pausing, so journal the failure and let the + reset proceed (the re-drive is a human-directed discard of the failed + attempt). The two failures journal under distinct events + (``attempt-preserve-enumerate-failed`` vs ``attempt-preserve-failed``) so a + post-mortem can tell "could not count the work" from "counted it but could + not park it" — only the latter can report a HEAD.""" baseline = task.baseline_commit if not baseline: return workspace = self._workspace_get() - commits = verify.commits_above(workspace.root, baseline) - if not commits: - return - head = verify.rev_parse_head(workspace.root) # the tip the recovery ref parks at + # Enumerating the range is what decides whether the reset is safe, so a + # fault here is not a no-op: an un-determinable range must read as "there + # may be work above baseline" and take the preservation-failure path below, + # never the `not commits` early return (which would let the reset run + # blind). These two calls carried no guard at all, so until #343 a plain + # git *timeout* — which `_run_git` does translate, and which every sibling + # here already treats as routine — crashed the rollback outright; OSError + # joins it because the translation stops at timeouts. The `not commits` + # return sits inside the try to keep the original call order: HEAD is still + # only read once there is something to park. + try: + commits = verify.commits_above(workspace.root, baseline) + if not commits: + return + head = verify.rev_parse_head(workspace.root) # the tip the ref parks at + except (verify.GitError, OSError) as exc: + self.journal.append( + "attempt-preserve-enumerate-failed", story_key=task.story_key, error=str(exc) + ) + if allow_pause: + # Same refusal as an un-parked ref: the notice must not tell the + # operator to `reset --hard` past work we could not even count. + self.pause_for_manual_recovery(task, baseline, preserve_failed=True) + return # re-drive: never pause — proceed to the (human-directed) reset # 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 @@ -341,7 +366,7 @@ def preserve_attempt_commits(self, task: StoryTask, *, allow_pause: bool) -> Non f"attempt-preserve/{slug}-{head[:8]}", commits=commits, ) - except verify.GitError: + except (verify.GitError, OSError): ref = None # branch creation failed — treat as a preservation failure if ref is None: # commits exist (just enumerated) but the ref did not take. @@ -375,10 +400,9 @@ def preserve_attempt_worktree(self, task: StoryTask, *, allow_pause: bool) -> No same terms: with ``allow_pause`` (a plain rollback) pause for manual recovery rather than reset; on a re-drive (``allow_pause=False``) the caller's contract forbids pausing, so journal and let the human-directed - reset proceed. The symmetry is in the *refusal policy*, not the guard: this - path catches ``(GitError, OSError)`` while `preserve_attempt_commits` still - catches ``GitError`` alone, so a spawn-level fault there crashes instead of - refusing — one instance of the systemic gap tracked in #343. + reset proceed. Both now guard ``(GitError, OSError)`` too — ``_run_git`` + translates only a timeout, so a spawn-level fault would otherwise crash the + rollback rather than refuse it (#343). The refusal is gated on :meth:`_reset_would_destroy`, so a capture failure over a tree with nothing left to lose (commits already parked, nothing diff --git a/tests/test_recovery_flow.py b/tests/test_recovery_flow.py index 49cb835..4d79a0d 100644 --- a/tests/test_recovery_flow.py +++ b/tests/test_recovery_flow.py @@ -263,6 +263,28 @@ def boom(*a, **k): assert "rollback-skipped-clean" not in flow.journal.events() +def test_rollback_dirty_check_oserror_degrades_to_dirty(project, monkeypatch): + # #343: `_run_git` translates only a timeout, so a spawn-level OSError reaches + # this guard untyped. It must degrade exactly like the GitError above — this is + # the first git call on the rollback path, so an unguarded OSError here crashes + # before any preserve step can run. + repo = project.project + ws = Workspace.default(project) + flow = _make_flow(workspace=ws, policy=_policy(rollback_on_failure=False)) + task = _task(repo) + + def boom(*a, **k): + raise OSError(24, "Too many open files") + + monkeypatch.setattr(verify, "attempt_dirty", boom) + + with pytest.raises(_Pause): + flow.rollback_or_pause(task) + + assert "rollback-dirty-check-failed" in flow.journal.events() + assert "rollback-skipped-clean" not in flow.journal.events() + + # --------------------------------------------------------------- preserve refs @@ -337,6 +359,104 @@ def test_preserve_attempt_commits_no_pause_on_redrive(project, monkeypatch): assert flow.calls.pauses == [] +def _commit_something(repo: Path) -> None: + (repo / "src.txt").write_text("committed\n") + git(repo, "add", "-A") + git(repo, "commit", "-q", "-m", "attempt commit") + + +@pytest.mark.parametrize( + "make_exc", + # Factories, not instances: a parametrized exception *instance* is built once at + # collection and shared by every case, and re-raising it mutates its + # __traceback__ — which under pytest-randomly's shuffling made these cases + # couple to each other and fail for the wrong reason. + [lambda: GitError("git log timed out"), lambda: OSError(24, "Too many open files")], + ids=["giterror", "oserror"], +) +def test_preserve_attempt_commits_pauses_when_range_unenumerable(project, monkeypatch, make_exc): + # #343: `commits_above` carried no guard at all, so even a plain GitError (a + # translated git timeout) crashed the rollback here. An un-determinable range + # must refuse the reset, never fall through the `not commits` early return. + repo = project.project + ws = Workspace.default(project) + flow = _make_flow(workspace=ws) + task = _task(repo) + _commit_something(repo) + + def boom(*a, **k): + raise make_exc() + + monkeypatch.setattr(verify, "commits_above", boom) + + with pytest.raises(_Pause): + flow.preserve_attempt_commits(task, allow_pause=True) + + assert "attempt-preserve-enumerate-failed" in flow.journal.events() + assert "could not be auto-preserved" in flow.calls.pauses[-1][0] + assert task.preserve_ref is None # nothing parked → nothing to point the operator at + + +def test_preserve_attempt_commits_pauses_when_head_read_fails(project, monkeypatch): + # The second unguarded call (#343): the range enumerated fine, but the tip the + # ref would park at could not be read — still work we cannot park. + repo = project.project + ws = Workspace.default(project) + flow = _make_flow(workspace=ws) + task = _task(repo) + _commit_something(repo) + + def boom(*a, **k): + raise GitError("git rev-parse timed out") + + monkeypatch.setattr(verify, "rev_parse_head", boom) + + with pytest.raises(_Pause): + flow.preserve_attempt_commits(task, allow_pause=True) + + assert "attempt-preserve-enumerate-failed" in flow.journal.events() + assert task.preserve_ref is None + + +def test_preserve_attempt_commits_unenumerable_no_pause_on_redrive(project, monkeypatch): + # The re-drive contract forbids pausing even here: a human directed the discard. + repo = project.project + ws = Workspace.default(project) + flow = _make_flow(workspace=ws) + task = _task(repo) + _commit_something(repo) + + def boom(*a, **k): + raise OSError(24, "Too many open files") + + monkeypatch.setattr(verify, "commits_above", boom) + + flow.preserve_attempt_commits(task, allow_pause=False) # must not raise + + assert "attempt-preserve-enumerate-failed" in flow.journal.events() + assert flow.calls.pauses == [] + + +def test_preserve_attempt_commits_ref_oserror_treated_as_failure(project, monkeypatch): + # Sibling of the GitError case above: the ref write can also fail untyped (#343). + repo = project.project + ws = Workspace.default(project) + flow = _make_flow(workspace=ws) + task = _task(repo) + _commit_something(repo) + + def boom(*a, **k): + raise OSError(12, "Cannot allocate memory") + + monkeypatch.setattr(verify, "preserve_commits", boom) + + with pytest.raises(_Pause): + flow.preserve_attempt_commits(task, allow_pause=True) + + assert "attempt-preserve-failed" in flow.journal.events() + assert task.preserve_ref is None + + def test_preserve_attempt_worktree_snapshots_dirty_tree(project): repo = project.project ws = Workspace.default(project) From bc52fbf4454f26484b60dce240e86fdab38a3373 Mon Sep 17 00:00:00 2001 From: pbean Date: Mon, 27 Jul 2026 19:42:21 -0700 Subject: [PATCH 9/9] fix(recovery): escalate on an OSError from the restore-patch apply MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The last OSError-blind guard in the collaborator. apply_patch reads the patch from disk, so an ENOENT/EACCES/ENOSPC arrives untyped and crashed straight past the escalation this branch exists to perform — leaving a half-restored tree with no attention file, on the dispatch path rather than the rollback one. recovery_flow.py is now uniformly OSError-safe; the remaining 19 blind guards across 8 other modules stay with the chokepoint question in #343. --- CHANGELOG.md | 4 +++- src/bmad_loop/recovery_flow.py | 7 ++++++- tests/test_recovery_flow.py | 23 +++++++++++++++++++++++ 3 files changed, 32 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 70966b7..fdba367 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -184,7 +184,9 @@ story `, the same annotation a sweep bundle writes. Both sprint and stories (pausing for manual recovery, or journalling on a re-drive), never the clean-tree early return. Journals `attempt-preserve-enumerate-failed`, distinct from `attempt-preserve-failed`, so a post-mortem can tell "could not count the work" from "counted it but could not park it". The - rollback's dirty check degrades on an `OSError` for the same reason. + rollback's dirty check degrades on an `OSError` for the same reason, and a restore-patch apply + that fails with one now escalates instead of crashing past its own escalation — closing every + such guard in the recovery collaborator. - **The orchestrator's ledger writers no longer inject lines from a multiline value (#305).** Found by [@Haven2026](https://github.com/Haven2026) in #274. The deferred-work ledger is line-oriented, diff --git a/src/bmad_loop/recovery_flow.py b/src/bmad_loop/recovery_flow.py index b002c0d..8cb601f 100644 --- a/src/bmad_loop/recovery_flow.py +++ b/src/bmad_loop/recovery_flow.py @@ -255,7 +255,12 @@ def restore_patch(self, task: StoryTask) -> None: patch = verify.resolve_restore_path(task.restore_patch, workspace.root) try: verify.apply_patch(workspace.root, patch) - except verify.GitError as e: + except (verify.GitError, OSError) as e: + # OSError joins GitError for the reason the rollback guards do (#343): + # `_run_git` translates only a timeout, and the patch file is read from + # disk here, so an ENOENT/EACCES/ENOSPC arrives untyped. Crashing would + # skip the escalation this branch exists to perform and leave the tree + # half-restored with no attention file. self.journal.append( "attempt-restore-failed", story_key=task.story_key, diff --git a/tests/test_recovery_flow.py b/tests/test_recovery_flow.py index 4d79a0d..46c0a12 100644 --- a/tests/test_recovery_flow.py +++ b/tests/test_recovery_flow.py @@ -965,3 +965,26 @@ def boom(repo, patch): assert flow.calls.escalates # routed through the engine's escalation assert "attempt-restore-failed" in flow.journal.events() assert "attempt-restored" not in flow.journal.events() # never reached on failure + + +def test_restore_patch_escalates_on_apply_oserror(project, monkeypatch): + # #343: the patch is read from disk, so an ENOENT/EACCES arrives untyped and + # must still escalate rather than crash past the escalation. + ws = Workspace.default(project) + flow = _make_flow(workspace=ws) + task = _task(project.project) + task.restore_patch = "patch.diff" + task.phase = Phase.DEV_RUNNING + monkeypatch.setattr(verify, "resolve_restore_path", lambda raw, root: Path(root) / raw) + + def boom(repo, patch): + raise OSError(2, "No such file or directory") + + monkeypatch.setattr(verify, "apply_patch", boom) + + with pytest.raises(_Pause): + flow.restore_patch(task) + + assert task.phase == Phase.DEV_VERIFY + assert flow.calls.escalates + assert "attempt-restore-failed" in flow.journal.events()