diff --git a/CHANGELOG.md b/CHANGELOG.md index a77f01c..7f14ea8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -164,6 +164,13 @@ story `, 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 diff --git a/docs/FEATURES.md b/docs/FEATURES.md index db7d164..640eed4 100644 --- a/docs/FEATURES.md +++ b/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 ` 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 984bbc6..8bbac82 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -3081,6 +3081,25 @@ 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) @@ -3088,19 +3107,7 @@ def _defer(self, task: StoryTask, reason: str) -> None: # 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) @@ -3108,7 +3115,25 @@ def _defer(self, task: StoryTask, reason: str) -> None: 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 diff --git a/tests/test_engine.py b/tests/test_engine.py index a52b19c..2529797 100644 --- a/tests/test_engine.py +++ b/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."""