From f362d0ed3819609cd8b74890f4d069d11dcf076f Mon Sep 17 00:00:00 2001 From: pbean Date: Fri, 10 Jul 2026 06:59:54 -0700 Subject: [PATCH] fix(engine): replay recorded sessions persisted at DEV_VERIFY/REVIEW_VERIFY instead of demanding rollback A host death in the post-verify decision window persists the task at DEV_VERIFY (or REVIEW_VERIFY one phase later) with the completed session's record durably saved but the decision's action unapplied. The resume replay matcher only knew the *_RUNNING phases, so those tasks fell through to resume-restart -> _rollback_or_pause, pausing with a bare 'git reset --hard ' instruction that would discard the attempt's finished -- possibly already-pushed -- commits. _resumable_session now matches DEV_VERIFY (dev, attempt) and REVIEW_VERIFY (review, review_cycle); the existing replay arms re-enter the normal reconcile -> verify -> decide pipeline with no budget burned and the persisted baseline kept. The spec-approval-gate arm stays first, so a DEV_VERIFY task with a verified spec keeps _resume_after_dev_verify. The rollback-OFF manual-recovery notice now probes commits_above(baseline) and, when the attempt committed work, leads with saving/checking those commits (they may already be pushed) instead of instructing a blind reset; the probe is advisory and falls back to the classic notice on a git fault. Fixes #100. Refs #99. --- CHANGELOG.md | 9 ++ src/bmad_loop/engine.py | 73 ++++++++--- tests/test_engine.py | 231 +++++++++++++++++++++++++++++++++++ tests/test_stories_engine.py | 38 ++++++ 4 files changed, 335 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2cd291e..28261bf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,15 @@ breaking changes may land in a minor release. ### Fixed +- **Resume no longer asks for a rollback of a completed session's committed work.** A host death + in the post-verify decision window left the task persisted at `DEV_VERIFY`/`REVIEW_VERIFY`, + where the resume replay matcher (which only knew the `*_RUNNING` phases) missed the + durably-recorded completed session and fell through to resume-restart — pausing with a + `git reset --hard ` instruction that would discard the attempt's finished, possibly + already-pushed commits. Those phases now replay the recorded result through the normal + verify/decide pipeline, and the rollback-OFF manual-recovery notice detects commits above + baseline and leads with saving/checking them instead of a bare reset (#100). + - **The TUI no longer crashes on a private-mode CSI sequence in an adapter log.** The gemini CLI's startup burst includes XTMODKEYS `CSI > 4 ; ? m`; the marker byte sat _inside_ the params, so the private-marker strip filter missed it and pyte 0.8.2 raised a `TypeError` diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index 16edb2e..46b78f2 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -1046,15 +1046,27 @@ def _pause_for_manual_recovery( self, task: StoryTask, baseline: str, *, preserve_failed: bool = False ) -> None: """Leave the tree untouched, surface bold manual-recovery instructions, and - pause the run. Always raises RunPaused. Reached either (a, default) the OFF - path for a stopped/abandoned in-place attempt, or (b, ``preserve_failed``) - rollback is ON/resolved but the attempt's commits above baseline could not be - parked on a recovery ref, so an automatic ``reset --hard`` would silently - discard them — a distinct notice that names the at-risk commits and never - tells the operator to blindly reset. A *resolved* escalation never reaches - here — `_rollback_or_pause` auto-recovers that human-initiated re-drive - regardless of `scm.rollback_on_failure`.""" + pause the run. Always raises RunPaused. Three 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 + a recovery ref, so an automatic ``reset --hard`` would silently discard + them — a distinct notice that names the at-risk commits and never tells the + operator to blindly reset; (c) the OFF path but the attempt COMMITTED work + 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`.""" short = baseline[:12] or "" + commits: list[str] = [] + if baseline: + # advisory probe: a git fault here must not block the pause itself + try: + commits = verify.commits_above(self.workspace.root, baseline) + except verify.GitError: + commits = [] if preserve_failed: notice = ( "**ACTION REQUIRED — commits could not be auto-preserved**\n" @@ -1070,6 +1082,22 @@ def _pause_for_manual_recovery( "files.\n" f"Then run `bmad-loop resume {self.state.run_id}`." ) + elif commits: + notice = ( + "**ACTION REQUIRED — manual recovery needed (committed work present)**\n" + f"Story **{task.story_key}**'s attempt was stopped with auto-rollback " + "OFF, and it **committed work above its baseline**. **Your commits " + "are intact at the current HEAD.** They may already be integrated " + "or pushed to a remote — do NOT reset before checking.\n" + f" 1. **Save them first** — e.g. `git branch my-rescue HEAD` (the " + f"commits are `{short}..HEAD`).\n" + " 2. Check whether they are already integrated (merged, pushed to " + "a remote, referenced by open PRs) before discarding anything.\n" + " 3. Only if you decide to discard the attempt: " + f"`git reset --hard {short}`, then review/remove leftover untracked " + "files.\n" + f"Then run `bmad-loop resume {self.state.run_id}`." + ) else: why = ( f"Story **{task.story_key}**'s attempt was stopped and auto-rollback " @@ -1090,7 +1118,12 @@ def _pause_for_manual_recovery( "`[scm] rollback_on_failure` (it discards the attempt's uncommitted " "work but never deletes pre-existing untracked files)." ) - self.journal.append("rollback-manual-required", story_key=task.story_key, baseline=baseline) + self.journal.append( + "rollback-manual-required", + story_key=task.story_key, + baseline=baseline, + commits=len(commits), + ) gates.notify( self.policy, self.run_dir, @@ -1176,14 +1209,22 @@ def _finish_inflight(self) -> None: def _resumable_session(self, task: StoryTask) -> tuple[str, SessionResult] | None: """The in-flight session's durably-recorded result, when complete enough - to act on: the task died mid-phase but its current attempt/cycle record - is ``completed`` and carries the parsed result. Consumes only evidence - the adapter vouched for at session end — no artifact re-scan, no - loosening of completion authority. Anything less returns None and the - caller falls through to resume-restart.""" - if task.phase == Phase.DEV_RUNNING: + to act on: the task died mid-phase (``*_RUNNING``) or in the post-verify + decision window (``*_VERIFY`` — persisted by the save right after the + verify/decide pass, before the decision's action completed) but its + current attempt/cycle record is ``completed`` and carries the parsed + result. Consumes only evidence the adapter vouched for at session end — + no artifact re-scan, no loosening of completion authority. Anything less + returns None and the caller falls through to resume-restart (#100: that + restart used to discard a completed-``done`` attempt's commits). + + DEV_VERIFY reaches this matcher only when ``task.spec_file`` is empty + (verify did not fully pass before the death): _finish_inflight checks + the spec-approval-gate arm first, so a DEV_VERIFY task WITH a verified + spec keeps its _resume_after_dev_verify recovery.""" + if task.phase in (Phase.DEV_RUNNING, Phase.DEV_VERIFY): role, seq = "dev", task.attempt - elif task.phase == Phase.REVIEW_RUNNING: + elif task.phase in (Phase.REVIEW_RUNNING, Phase.REVIEW_VERIFY): role, seq = "review", task.review_cycle else: return None diff --git a/tests/test_engine.py b/tests/test_engine.py index fb54fb2..71deac8 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -411,6 +411,180 @@ def crashing_emit(stage, *args, **kwargs): assert "resume-verify" in kinds +def _dev_verify_crash_state(project, engine, spec_status: str) -> tuple[str, Path]: + """Persist the exact state.json shape from issue #100: a task at DEV_VERIFY + with no verified spec (verify had not passed when the host died), whose + completed dev session record — result on disk, commits above baseline — is + durable. Returns (baseline, spec_path).""" + baseline = rev_parse_head(project.project) + # the attempt committed its work above baseline, as the reporter's session + # did (only the work — sweeping the still-untracked sprint board into the + # commit would make a later baseline reset delete it) + src = project.project / "src.txt" + src.write_text(src.read_text() + "change for 1-1-a\n") + git(project.project, "add", "src.txt") + git(project.project, "commit", "-q", "-m", "attempt work for 1-1-a") + sp = spec_path(project, "1-1-a") + write_spec(sp, spec_status, baseline) + + task = StoryTask(story_key="1-1-a", epic=1, phase=Phase.DEV_VERIFY, attempt=1) + task.baseline_commit = baseline + task.baseline_untracked = [] + task.record_session( + SessionRecord( + task_id="1-1-a-dev-1", + role="dev", + status="completed", + result_json={ + "workflow": "auto-dev", + "story_key": "1-1-a", + "spec_file": str(sp), + "baseline_commit": baseline, + "escalations": [], + "followup_review_recommended": False, + }, + ) + ) + engine.state.tasks[task.story_key] = task + engine._save() + return baseline, sp + + +def test_resume_dev_verify_replays_recorded_dev_result_to_done(project): + """#100: the host died after persisting DEV_VERIFY but before the decision's + action completed — spec_file empty, completed/done dev record on disk, + commits above baseline. Resume must replay that record through the normal + verify/decide pipeline instead of demanding a manual rollback (`git reset + --hard `) of finished, possibly already-pushed work.""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + policy = Policy( + gates=GatesPolicy(mode="none"), + notify=QUIET, + # the issue's environment: the production default, where the old + # resume-restart arm paused with the destructive reset instruction + scm=ScmPolicy(rollback_on_failure=False), + ) + engine, _ = make_engine(project, [], policy=policy) + baseline, _sp = _dev_verify_crash_state(project, engine, "done") + src = project.project / "src.txt" + + resumed, adapter = resume_engine(project, engine, []) + summary = resumed.run() + + assert summary.done == 1 and not summary.paused and not summary.crashed + final = load_state(resumed.run_dir).tasks["1-1-a"] + assert final.phase == Phase.DONE + assert final.attempt == 1 # the replay burned no attempt budget + assert final.commit_sha # the field the issue found null — now stamped + assert adapter.sessions == [] # nothing re-run — the record was replayed + # the attempt's committed work survived (squashed into the story commit) + assert "change for 1-1-a" in src.read_text() + assert rev_parse_head(project.project) != baseline + kinds = [e["kind"] for e in resumed.journal.entries()] + assert "resume-verify" in kinds + assert "resume-restart" not in kinds + assert "rollback-manual-required" not in kinds + + +def test_resume_dev_verify_replay_verify_still_failing_retries_normally(project): + """When the replayed record's verify failure reproduces (the spec is still + short of done), the replay re-enters the normal retry path — commits parked + on a recovery ref, reset, then a fresh budgeted attempt — instead of + resume-restart's evidence-blind discard.""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + engine, _ = make_engine(project, []) # helper default: rollback ON + _dev_verify_crash_state(project, engine, "in-progress") + + resumed, adapter = resume_engine( + project, + engine, + [dev_effect(project, "1-1-a"), review_effect(project, "1-1-a", clean=True)], + ) + summary = resumed.run() + + assert summary.done == 1 and not summary.crashed + final = load_state(resumed.run_dir).tasks["1-1-a"] + assert final.phase == Phase.DONE + assert final.attempt == 2 # the replay burned no budget; the fresh attempt did + assert [s.role for s in adapter.sessions] == ["dev", "review"] + kinds = [e["kind"] for e in resumed.journal.entries()] + assert "resume-verify" in kinds + assert "resume-restart" not in kinds + assert "attempt-commits-preserved" in kinds # committed work parked, not orphaned + + +@pytest.mark.parametrize( + "record", + [ + SessionRecord(task_id="1-1-a-dev-1", role="dev", status="stalled"), + # completed but without a recorded result (legacy state.json shape) + SessionRecord(task_id="1-1-a-dev-1", role="dev", status="completed"), + ], +) +def test_resume_dev_verify_record_incomplete_still_restarts(project, record): + """DEV_VERIFY without a verified spec joins the replay matcher only for a + completed record WITH a recorded result — anything less keeps today's + resume-restart (no artifact re-scan, no loosening of completion authority).""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + engine, _ = make_engine(project, []) + task = StoryTask(story_key="1-1-a", epic=1, phase=Phase.DEV_VERIFY, attempt=1) + task.record_session(record) + engine.state.tasks[task.story_key] = task + engine._save() + + resumed, _ = resume_engine( + project, + engine, + [dev_effect(project, "1-1-a"), review_effect(project, "1-1-a", clean=True)], + ) + summary = resumed.run() + + assert summary.done == 1 + kinds = [e["kind"] for e in resumed.journal.entries()] + assert "resume-restart" in kinds + assert "resume-verify" not in kinds + + +def test_resume_review_verify_replays_recorded_review(project): + """A host death in the post-review-verify decision window (REVIEW_VERIFY + persisted by the save right after the review session, decision not yet + acted on) replays the recorded review pass instead of resume-restart's + rollback — the same #100 window one phase later.""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + engine, _ = make_engine( + project, + [dev_effect(project, "1-1-a"), review_effect(project, "1-1-a", clean=True)], + ) + + def crashing_emit(stage, *args, **kwargs): + if stage == "post_review_result": + raise RuntimeError("host died in the review decision window") + return original_emit(stage, *args, **kwargs) + + original_emit = engine._emit + engine._emit = crashing_emit + assert engine.run().crashed + crashed = load_state(engine.run_dir).tasks["1-1-a"] + assert crashed.phase == Phase.REVIEW_VERIFY + assert crashed.review_cycle == 1 + assert crashed.sessions[-1].result_json is not None + + resumed, adapter = resume_engine(project, engine, []) + summary = resumed.run() + + assert summary.done == 1 and not summary.crashed + final = load_state(resumed.run_dir).tasks["1-1-a"] + assert final.phase == Phase.DONE + assert final.review_cycle == 1 # the replay burned no review-budget slot + assert adapter.sessions == [] # neither dev nor review re-run + entries = resumed.journal.entries() + verifies = [e for e in entries if e["kind"] == "resume-verify"] + assert verifies and verifies[-1]["role"] == "review" + kinds = [e["kind"] for e in entries] + assert "resume-restart" not in kinds + assert "rollback-manual-required" not in kinds + + def test_reconcile_early_return_heals_stale_resumed_dict(project): """On the idempotent early-return path (frontmatter already at the success status), the reconcile still syncs a stale *resumed* result dict from the @@ -1788,6 +1962,63 @@ def test_manual_recovery_wording_stopped(project): assert "attempt was stopped" in stopped.value.reason +def test_manual_recovery_notice_names_committed_work(project): + """#100: rollback OFF + an attempt that COMMITTED above baseline. The pause + notice must lead with saving/checking the commits — which may already be + pushed — never with a bare `git reset --hard` that would discard them.""" + policy = Policy( + gates=GatesPolicy(mode="none"), + notify=QUIET, + scm=ScmPolicy(rollback_on_failure=False), + ) + engine, _ = make_engine(project, [], policy=policy) + task = StoryTask(story_key="1-1-a", epic=1) + task.baseline_commit = rev_parse_head(project.project) + task.baseline_untracked = [] + src = project.project / "src.txt" + src.write_text(src.read_text() + "committed attempt work\n") + git(project.project, "add", "-A") + git(project.project, "commit", "-q", "-m", "attempt commit") + attempt_head = rev_parse_head(project.project) + + with pytest.raises(RunPaused) as paused: + engine._rollback_or_pause(task) + + assert rev_parse_head(project.project) == attempt_head # tree untouched + reason = paused.value.reason + assert "failed" not in reason # a stopped attempt is not described as "failed" + assert f"{task.baseline_commit[:12]}..HEAD" in reason + assert "intact" in reason + assert "pushed" in reason + # save-the-commits comes before any reset instruction + assert reason.index("git branch") < reason.index("reset --hard") + manual = [e for e in engine.journal.entries() if e["kind"] == "rollback-manual-required"] + assert manual and manual[-1]["commits"] == 1 + + +def test_manual_recovery_notice_probe_failure_falls_back(project, monkeypatch): + """The commits probe is advisory: a git fault must neither block the pause + nor change the classic no-commits notice.""" + policy = Policy( + gates=GatesPolicy(mode="none"), + notify=QUIET, + scm=ScmPolicy(rollback_on_failure=False), + ) + engine, _ = make_engine(project, [], policy=policy) + task = StoryTask(story_key="1-1-a", epic=1) + baseline = rev_parse_head(project.project) + + def boom(repo, base): + raise GitError("probe failed") + + monkeypatch.setattr(verify, "commits_above", boom) + with pytest.raises(RunPaused) as paused: + engine._pause_for_manual_recovery(task, baseline) + + assert "attempt was stopped" in paused.value.reason + assert "manual rollback" in paused.value.reason.lower() + + def test_rollback_preserves_committed_attempt_work(project): """rollback_on_failure ON + an attempt that committed its work: the hard reset parks those commits under a recovery ref instead of orphaning them.""" diff --git a/tests/test_stories_engine.py b/tests/test_stories_engine.py index d698e9d..d7e46aa 100644 --- a/tests/test_stories_engine.py +++ b/tests/test_stories_engine.py @@ -720,6 +720,44 @@ def plan_then_die(spec): assert task.commit_sha is None # never committed un-reviewed +def test_resume_dev_verify_replay_stories_mode(project): + """Stories-mode parity for the #100 resume arm: a story persisted at + DEV_VERIFY without a verified spec (verify failed, then the host died mid + retry-reset) replays its completed dev record instead of resume-restart. + After the operator repaired the story spec, the replay verifies green and + commits — no session re-run, no attempt budget burned.""" + setup_stories(project, [entry("1")]) + engine, _ = make_engine(project, [stories_dev_effect(final_status="in-progress")]) + + with pytest.MonkeyPatch.context() as mp: + + def boom(*args, **kwargs): + raise RuntimeError("host died during the retry reset") + + mp.setattr("bmad_loop.verify.safe_rollback", boom) + assert engine.run().crashed + + crashed = load_state(engine.run_dir).tasks["1"] + assert crashed.phase == Phase.DEV_VERIFY + assert not crashed.spec_file # verify had not passed — the #100 shape + assert crashed.sessions[-1].result_json is not None + + # the operator repaired the story spec before resuming + write_spec(story_spec(project, "1"), "done", crashed.baseline_commit) + + resumed, adapter = resume_engine(project, engine, []) + summary = resumed.run() + + assert summary.done == 1 and not summary.crashed + final = load_state(resumed.run_dir).tasks["1"] + assert final.phase == Phase.DONE + assert final.attempt == 1 # the replay burned no attempt budget + assert adapter.sessions == [] # the dev session was not re-run + kinds = [e["kind"] for e in resumed.journal.entries()] + assert "resume-verify" in kinds + assert "resume-restart" not in kinds + + def test_plan_review_owed_after_non_fixable_retry_becomes_implement_leg(project): """MAJOR-B(b): leg 1 plans (ready-for-dev) but fails verify non-fixably (wrong workflow tag), so the tree resets and attempt 2 re-dispatches. The plan survived