fix(engine): emit defer record when the rollback pauses (#342)

CHANGELOG.md

@@ -164,6 +164,13 @@ story <id>`, the same annotation a sweep bundle writes. Both sprint and stories
 
 ### Fixed
 
+- **A pause inside a defer's rollback no longer loses the defer record (#342).** `_defer` advanced
+  the task to terminal DEFERRED before recovering the tree, so a rollback that paused instead
+  (rollback OFF — the default — or a preserve/snapshot failure) unwound past the tail forever: no
+  `story-deferred` journal entry, no defer notification, an under-counted diagnose `defer_count`.
+  The record is now emitted before the pause re-raises; its notice points at the ACTION REQUIRED
+  manual-recovery notice instead of describing a rollback that never ran.
+
 - **Spawn-level `OSError` is translated at the git chokepoint (#343).** `_run_git` translated only
   a timeout, so an EMFILE/ENOMEM/ENOENT out of `subprocess.run` bypassed all 19 OSError-blind
   `except GitError` guards and crashed the run — under exactly the resource pressure the recovery

docs/FEATURES.md

@@ -60,7 +60,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se
 
 - 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.
 - 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).
+- 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). When the recovery itself pauses the run (rollback OFF, or a preserve failure refusing the reset), the defer record still lands before the pause — its notice then points at the ACTION REQUIRED manual-recovery notice instead of naming a ref (#342).
 - 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 <run-id>` 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.

src/bmad_loop/engine.py

@@ -3081,34 +3081,59 @@ def _defer_recovery_note(self, task: StoryTask) -> str:
             )
         return f" — attempt work parked at `{task.preserve_ref}`{recover}"
 
+    def _record_defer(self, task: StoryTask, reason: str, note: str | None = None) -> None:
+        """Journal + notify + persist the defer decision — the one record shape all
+        three emit sites share (#342). ``note`` overrides the recovery-note tail on
+        the rollback-pause path, where the reset never ran and the standard note's
+        parked/destroyed claims would be wrong."""
+        self.journal.append(
+            "story-deferred",
+            story_key=task.story_key,
+            reason=reason,
+            preserve_ref=task.preserve_ref or "",
+        )
+        gates.notify(
+            self.policy,
+            self.run_dir,
+            f"story deferred: {task.story_key}",
+            reason + (self._defer_recovery_note(task) if note is None else note),
+        )
+        self._save()
+
     def _defer(self, task: StoryTask, reason: str) -> None:
         task.defer_reason = reason
         advance(task, Phase.DEFERRED)
         if self._isolated:
             # the failed work lives in the unit's worktree; the diff is captured
             # and the worktree kept/dropped by _integrate_unit. Don't touch the
             # tree here (no reset into the main repo — there's nothing to undo).
-            self.journal.append(
-                "story-deferred",
-                story_key=task.story_key,
-                reason=reason,
-                preserve_ref=task.preserve_ref or "",
-            )
-            gates.notify(
-                self.policy,
-                self.run_dir,
-                f"story deferred: {task.story_key}",
-                reason + self._defer_recovery_note(task),
-            )
-            self._save()
+            self._record_defer(task, reason)
             return
         if task.baseline_commit:
             self._stash_deferred_artifacts(task)
             deferred_work = self.workspace.paths.deferred_work
             snapshot = (
                 deferred_work.read_text(encoding="utf-8") if deferred_work.is_file() else None
             )
-            self._rollback_or_pause(task)
+            try:
+                self._rollback_or_pause(task)
+            except RunPaused:
+                # Narrow, deliberate catch of the unwind-to-the-top pause (#342):
+                # the pause already persisted Phase.DEFERRED (terminal), so resume
+                # will never re-enter this method — the defer record is emitted
+                # now or never. Every pause path fires BEFORE safe_reset, so the
+                # tree is untouched: the standard recovery note's parked/destroyed
+                # claims would be wrong here — the ACTION REQUIRED notice just
+                # above this one in ATTENTION is the authoritative pointer.
+                # Re-raised untouched: the run still pauses.
+                self._record_defer(
+                    task,
+                    reason,
+                    note=" — the tree was NOT rolled back: the run paused for manual "
+                    "recovery first (see the ACTION REQUIRED notice for where the "
+                    "attempt's work is)",
+                )
+                raise
             # reset reverts tracked deferred-work.md edits; restore review-found
             # defer entries — they are real knowledge worth keeping
             if snapshot is not None:
@@ -3118,19 +3143,7 @@ def _defer(self, task: StoryTask, reason: str) -> None:
                 if current != snapshot:
                     deferred_work.parent.mkdir(parents=True, exist_ok=True)
                     deferred_work.write_text(snapshot, encoding="utf-8")
-        self.journal.append(
-            "story-deferred",
-            story_key=task.story_key,
-            reason=reason,
-            preserve_ref=task.preserve_ref or "",
-        )
-        gates.notify(
-            self.policy,
-            self.run_dir,
-            f"story deferred: {task.story_key}",
-            reason + self._defer_recovery_note(task),
-        )
-        self._save()
+        self._record_defer(task, reason)
 
     def _stash_deferred_artifacts(self, task: StoryTask) -> None:
         """Move the deferred story's spec out of the artifacts dir into the run

tests/test_engine.py

@@ -4045,6 +4045,95 @@ def test_rollback_off_pauses_with_manual_notice(project):
     assert "rollback-manual-required" in kinds
 
 
+def test_defer_record_survives_rollback_pause(project):
+    """#342: a defer whose rollback pauses (production default: rollback OFF +
+    dirty tree) must still land the story-deferred journal entry and the defer
+    notification — the pause persists terminal DEFERRED, so resume never
+    re-enters _defer and the record is emitted now or never.
+
+    Ablation target: delete the `except RunPaused` arm in Engine._defer and
+    this fails (no story-deferred entry, no defer line in ATTENTION)."""
+    write_sprint(project, {"1-1-a": "ready-for-dev"})
+    policy = Policy(
+        gates=GatesPolicy(mode="none"),
+        notify=QUIET,
+        scm=ScmPolicy(rollback_on_failure=False),
+    )
+    engine, _ = make_engine(
+        project,
+        [dev_effect(project, "1-1-a")]
+        + [
+            review_effect(project, "1-1-a", clean=False, patched=1, finalized=False)
+            for _ in range(3)
+        ],
+        policy=policy,
+    )
+    summary = engine.run()
+
+    assert summary.paused
+    assert load_state(engine.run_dir).paused_stage == PAUSE_ESCALATION
+    assert engine.state.tasks["1-1-a"].phase == Phase.DEFERRED
+    kinds = [e["kind"] for e in engine.journal.entries()]
+    # the pause's manual-recovery record, then the defer record, then the
+    # run-paused entry _run_inner appends while unwinding
+    assert (
+        kinds.index("rollback-manual-required")
+        < kinds.index("story-deferred")
+        < kinds.index("run-paused")
+    )
+    deferred_entry = next(e for e in engine.journal.entries() if e["kind"] == "story-deferred")
+    assert deferred_entry["preserve_ref"] == ""  # rollback OFF parks nothing
+    attention = (engine.run_dir / "ATTENTION").read_text()
+    assert "story deferred: 1-1-a" in attention
+    # the louder manual-recovery notice still comes first
+    assert attention.index("ACTION REQUIRED") < attention.index("story deferred: 1-1-a")
+
+
+def test_defer_record_on_snapshot_failure_pause_stays_honest(project, monkeypatch):
+    """#342 on the #340 path: rollback ON, commits parked, then the uncommitted
+    snapshot fails and the reset is refused. The defer record must land, name
+    the parked commits ref, and must NOT reuse _defer_recovery_note's partial
+    wording — 'did not survive the rollback' is false here: the reset never ran
+    and the tree is untouched.
+
+    Ablation target: delete the `except RunPaused` arm in Engine._defer and
+    this fails."""
+
+    def dev_with_commit(spec):
+        result = dev_effect(project, "1-1-a")(spec)
+        (project.project / "impl.txt").write_text("committed work\n")
+        git(project.project, "add", "impl.txt")
+        git(project.project, "commit", "-q", "-m", "attempt work")
+        return result
+
+    write_sprint(project, {"1-1-a": "ready-for-dev"})
+    engine, _ = make_engine(
+        project,
+        [dev_with_commit]
+        + [
+            review_effect(project, "1-1-a", clean=False, patched=1, finalized=False)
+            for _ in range(3)
+        ],
+    )
+
+    def _fail(*a, **k):
+        raise GitError("simulated write-tree failure")
+
+    monkeypatch.setattr("bmad_loop.verify.snapshot_worktree", _fail)
+    summary = engine.run()
+
+    assert summary.paused
+    deferred_entry = next(e for e in engine.journal.entries() if e["kind"] == "story-deferred")
+    # the committed half IS parked — the record names it factually
+    assert deferred_entry["preserve_ref"].startswith("attempt-preserve/")
+    attention = (engine.run_dir / "ATTENTION").read_text()
+    assert "story deferred: 1-1-a" in attention
+    assert "uncommitted work could not be auto-preserved" in attention
+    # the pause-aware note, not the standard one: nothing was destroyed
+    assert "the tree was NOT rolled back" in attention
+    assert "did not survive the rollback" not in attention
+
+
 def test_rollback_on_preserves_preexisting_untracked(project):
     """With rollback_on_failure=True the auto-rollback reverts tracked changes
     but never deletes untracked files that predate the attempt."""