From 4f7e0a8a2d538066cfabf1f1a8f70cfaa1b2927a Mon Sep 17 00:00:00 2001 From: pbean Date: Mon, 27 Jul 2026 13:06:57 -0700 Subject: [PATCH 1/2] feat(recovery): surface the attempt-preserve ref in defer notification and status --- CHANGELOG.md | 9 +++++ README.md | 4 +-- docs/FEATURES.md | 2 +- src/bmad_loop/cli.py | 2 ++ src/bmad_loop/documents.py | 4 +++ src/bmad_loop/engine.py | 41 +++++++++++++++++++++-- src/bmad_loop/model.py | 14 ++++++++ src/bmad_loop/recovery_flow.py | 12 +++++++ tests/test_cli.py | 31 ++++++++++++++++++ tests/test_engine.py | 51 +++++++++++++++++++++++++++++ tests/test_engine_worktree.py | 11 +++++++ tests/test_model.py | 11 +++++++ tests/test_recovery_flow.py | 60 ++++++++++++++++++++++++++++++++++ 13 files changed, 246 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 87da55d1..7b85cb5d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,15 @@ breaking changes may land in a minor release. ### 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), + but the ref only ever reached `journal.jsonl` — the notification carried a bare reason, leaving + the operator to find their work with `git log --all`. The notice now names the ref and the + `git merge --ff-only` command that restores it; an isolated unit's notice names its kept-failed + branch instead. New `preserve_ref` on each task, projected into `bmad-loop status` and + `--json` (additive; schema stays 1). Reported verbatim from run state — `status` never runs + git, so a ref since pruned by `scm.preserve_keep` is still named. + - **Stories can close deferred-work entries (#234).** The ledger was one-way: entries are filed automatically but were only ever marked resolved by hand. A story can now declare the entries its work closes — `closes_deferred: [DW-5, DW-6]`, on its `stories.yaml` entry (stories mode) diff --git a/README.md b/README.md index 5ec4d9bc..29e52dbe 100644 --- a/README.md +++ b/README.md @@ -462,7 +462,7 @@ By default work happens **in place** on the checked-out branch (`[scm] isolation - **`branch_per`** — `story` (a branch per story) or `run` (one shared branch across the run; this forces `delete_branch = false` so the shared branch survives between units). - **`target_branch`** — the branch every unit merges into; empty means the branch checked out at run start. A configured branch is created if missing (a detached HEAD or unborn repo pauses the run rather than merging onto an unreferenced commit). - **`keep_failed`** (default on) — a deferred/escalated unit's worktree + branch stay mounted for inspection, and its full diff (tracked + untracked) is preserved to `run_dir/failed//changes.patch`. `failed_diff_max_mb` caps the per-file size of untracked files in that patch (oversized files skipped with a marker); `failed_diff_unlimited` lifts the cap. -- **`rollback_on_failure`** (default off) — governs in-place recovery when `isolation = "none"`. Off pauses a failed unattended attempt with manual-recovery steps and leaves the tree untouched; on auto-reverts the attempt's tracked changes and removes only the untracked files the run created (never a blanket `git clean`). A resolved escalation's re-drive always auto-recovers regardless of this flag. Under `isolation = "worktree"` the worktree is disposable, so a failed attempt's commits are parked on `attempt-preserve/*` refs and the worktree resets regardless — **`preserve_keep`** (default 20) bounds how many of those recovery refs are kept per family at run start, pruning the oldest (`0` = never prune). +- **`rollback_on_failure`** (default off) — governs in-place recovery when `isolation = "none"`. Off pauses a failed unattended attempt with manual-recovery steps and leaves the tree untouched; on auto-reverts the attempt's tracked changes and removes only the untracked files the run created (never a blanket `git clean`). A resolved escalation's re-drive always auto-recovers regardless of this flag. Under `isolation = "worktree"` the worktree is disposable, so a failed attempt's commits are parked on `attempt-preserve/*` refs and the worktree resets regardless — **`preserve_keep`** (default 20) bounds how many of those recovery refs are kept per family at run start, pruning the oldest (`0` = never prune). Whenever an attempt is parked, the ref is named in the story's defer notification (with the `git merge --ff-only` line that recovers it) and in `bmad-loop status`/`--json` as `preserve_ref` — reported verbatim from run state, so a ref that a later run's `preserve_keep` pruning has since deleted is still shown. - **`commit_message_template`** — when set, the message used for story/bundle commits (`{story_key}` / `{run_id}` substituted). - **`seed_adapter_defaults`** (default on) / **`worktree_seed`** — a worktree checks out _tracked_ files only, so a project's gitignored MCP/CLI configs (`.mcp.json`, `.claude/settings.json`, `.codex/config.toml`, `.gemini/settings.json`) are absent from a fresh worktree — without them an isolated session can't reach its MCP server and stalls on readiness. With `seed_adapter_defaults` on, each loaded adapter's own configs are copied in from the main repo before the session launches (the defaults live in each CLI profile's `seed_files`); `worktree_seed` adds extra project-relative paths on top. Seeding is copy-when-absent and runs before the signal-hook merge, so a seeded `settings.json` keeps its real content and just gains the Stop hook — and the seeded paths are shielded from the unit's `git add -A`. @@ -550,7 +550,7 @@ Each run drives its agents inside a dedicated tmux session, `bmad-loop-` ### Scripting `status` -`bmad-loop status --json` is the **supported machine-readable surface**: stdout becomes a single JSON document (nothing else is printed), so `bmad-loop status --json | jq '.tasks[].phase'` just works. The document carries `schema_version` (currently 1; changes are additive, anything breaking bumps it), the run's identity and state (`run_id`, `run_type`, `source`, `started_at`, a derived `status` — `finished`/`paused`/`crashed`/`stopped`/`in-progress` — beside the raw flags and `paused_stage`/`paused_reason`/`paused_story_key`, plus a boolean `graceful_stop_pending` — true while a graceful stop (`stop --graceful`) is on disk and the engine is still live), the snapshot's `cache_read_weight`, run-level `tokens` (`raw` + `weighted`), and per-story `tasks` entries: `story_key`, `epic`, `phase`, `attempt`, `review_cycle`, `commit_sha`, `defer_reason`, and a `tokens` object with the four raw counters plus derived `raw` and `weighted`. Everything except `graceful_stop_pending` is computed from the run's `state.json` alone — the weight comes from the persisted policy snapshot, so the numbers match what the run actually enforced; `graceful_stop_pending` additionally reflects the pending-request control file and the live-engine check, which `state.json` does not record. On error (no runs, unknown ref) stdout stays empty and the exit code is 1. The human-readable text output, by contrast, is **best-effort and not a stable interface** — parse the JSON, not the text. +`bmad-loop status --json` is the **supported machine-readable surface**: stdout becomes a single JSON document (nothing else is printed), so `bmad-loop status --json | jq '.tasks[].phase'` just works. The document carries `schema_version` (currently 1; changes are additive, anything breaking bumps it), the run's identity and state (`run_id`, `run_type`, `source`, `started_at`, a derived `status` — `finished`/`paused`/`crashed`/`stopped`/`in-progress` — beside the raw flags and `paused_stage`/`paused_reason`/`paused_story_key`, plus a boolean `graceful_stop_pending` — true while a graceful stop (`stop --graceful`) is on disk and the engine is still live), the snapshot's `cache_read_weight`, run-level `tokens` (`raw` + `weighted`), and per-story `tasks` entries: `story_key`, `epic`, `phase`, `attempt`, `review_cycle`, `commit_sha`, `defer_reason`, `preserve_ref` (the recovery ref a rolled-back attempt was parked on, else `null`), and a `tokens` object with the four raw counters plus derived `raw` and `weighted`. Everything except `graceful_stop_pending` is computed from the run's `state.json` alone — the weight comes from the persisted policy snapshot, so the numbers match what the run actually enforced; `graceful_stop_pending` additionally reflects the pending-request control file and the live-engine check, which `state.json` does not record. On error (no runs, unknown ref) stdout stays empty and the exit code is 1. The human-readable text output, by contrast, is **best-effort and not a stable interface** — parse the JSON, not the text. ### Scripting `validate` diff --git a/docs/FEATURES.md b/docs/FEATURES.md index cdbea702..243c53f3 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -59,7 +59,7 @@ 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. +- 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; isolated, the kept-failed unit branch — and the ref is projected as `preserve_ref` in `status` and `--json` (#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/cli.py b/src/bmad_loop/cli.py index 65d1b77a..94158d92 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -1579,6 +1579,8 @@ def cmd_status(args: argparse.Namespace) -> int: # to `if weighted` now that weighted is the displayed value. tokens = f"{task.tokens.weighted_total(weight):,}t ({raw:,} raw)" if raw else "-" extra = task.defer_reason or task.commit_sha or "" + if task.preserve_ref: + extra = f"{extra} [{task.preserve_ref}]".lstrip() print( f" {key:40s} {task.phase:16s} dev×{task.attempt} review×{task.review_cycle} " f"{tokens} {extra}" diff --git a/src/bmad_loop/documents.py b/src/bmad_loop/documents.py index 7c335ef3..cb51baf3 100644 --- a/src/bmad_loop/documents.py +++ b/src/bmad_loop/documents.py @@ -234,6 +234,10 @@ def status_document(state: RunState, *, graceful_stop_pending: bool = False) -> "tokens": tokens, "commit_sha": task.commit_sha, "defer_reason": task.defer_reason, + # the recovery ref a rolled-back attempt's work was parked on, or + # null. Reported verbatim from state.json — never re-validated + # against git here (retention may since have pruned it). + "preserve_ref": task.preserve_ref, "adapters_used": adapters_used, } ) diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index baa9e4d2..58b8f022 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -2991,6 +2991,31 @@ def _record_review_budget_followup(self, task: StoryTask, damped: bool = False) note, ) + def _defer_recovery_note(self, task: StoryTask) -> str: + """Message tail naming where the deferred attempt's work survives (#333). + + A bare reason leaves the operator to find the rolled-back work themselves + (the reporter used `git log --all`). In-place, the auto-rollback parked the + attempt on `task.preserve_ref`, which fast-forwards straight back. Isolated, + nothing is rolled back: a kept-failed unit's branch stays mounted. + + Empty when there is nothing to point at — a clean-tree defer, a preservation + failure, rollback off, or `keep_failed` off — so the notice can never + advertise a ref that was not created. The pointer is a name, not a promise: + `scm.preserve_keep` prunes the oldest refs at a later run's start, and + nothing here re-validates it (this must stay git-free — `status` reads + `state.json` only).""" + if self._isolated: + if self.policy.scm.keep_failed and task.branch: + return f" — failed work kept on branch `{task.branch}`" + return "" + if not task.preserve_ref: + return "" + return ( + f" — attempt work parked at `{task.preserve_ref}`; recover with " + f'`git -C "{self.workspace.root}" merge --ff-only {task.preserve_ref}`' + ) + def _defer(self, task: StoryTask, reason: str) -> None: task.defer_reason = reason advance(task, Phase.DEFERRED) @@ -2999,7 +3024,12 @@ def _defer(self, task: StoryTask, reason: str) -> None: # 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) - gates.notify(self.policy, self.run_dir, f"story deferred: {task.story_key}", reason) + gates.notify( + self.policy, + self.run_dir, + f"story deferred: {task.story_key}", + reason + self._defer_recovery_note(task), + ) self._save() return if task.baseline_commit: @@ -3018,12 +3048,17 @@ 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) + 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, + reason + self._defer_recovery_note(task), ) self._save() diff --git a/src/bmad_loop/model.py b/src/bmad_loop/model.py index 8e8c6ea0..dacd0fa7 100644 --- a/src/bmad_loop/model.py +++ b/src/bmad_loop/model.py @@ -174,6 +174,18 @@ class StoryTask: spec_file: str | None = None commit_sha: str | None = None defer_reason: str | None = None + # the recovery ref this attempt's work was parked on by the last auto-rollback + # — an `attempt-preserve/*` branch (commits above baseline) or, when the tree + # was also dirty, the `refs/attempt-preserve-dirty/*` snapshot, which is + # parented at the attempt's HEAD and therefore subsumes the branch (last + # writer wins, so one `git merge --ff-only ` recovers the whole attempt). + # Set by RecoveryFlow, cleared at the top of every auto-rollback so it can + # never name a *previous* attempt's ref; read by `_defer` (notification) and + # projected into `status`. None = nothing was parked (clean tree, preserve + # failure, or an isolated unit, whose branch stays mounted instead). Not + # cleared on success — a mid-retry rollback's breadcrumb stays readable. + # Survives the resume serialization round-trip. + preserve_ref: str | None = None # set by runs.rearm_escalation: this task was re-armed out of ESCALATED for a # clean rebuild against the corrected spec (not a failed attempt). Lets the # resume-time manual-recovery notice describe the real cause; cleared once the @@ -266,6 +278,7 @@ def to_dict(self) -> dict[str, Any]: "spec_file": self._serialized_spec_file(), "commit_sha": self.commit_sha, "defer_reason": self.defer_reason, + "preserve_ref": self.preserve_ref, "rearmed": self.rearmed, "resolved_redrive": self.resolved_redrive, "plan_checkpoint_pending": self.plan_checkpoint_pending, @@ -313,6 +326,7 @@ def from_dict(cls, d: dict[str, Any]) -> "StoryTask": spec_file=d.get("spec_file"), commit_sha=d.get("commit_sha"), defer_reason=d.get("defer_reason"), + preserve_ref=d.get("preserve_ref"), rearmed=bool(d.get("rearmed", False)), resolved_redrive=bool(d.get("resolved_redrive", False)), plan_checkpoint_pending=bool(d.get("plan_checkpoint_pending", False)), diff --git a/src/bmad_loop/recovery_flow.py b/src/bmad_loop/recovery_flow.py index 892bbdac..daa97006 100644 --- a/src/bmad_loop/recovery_flow.py +++ b/src/bmad_loop/recovery_flow.py @@ -163,6 +163,12 @@ def rollback_or_pause(self, task: StoryTask, *, cause: str = "stopped") -> None: # worktree recorded) still targets the main checkout and must pause. in_unit_worktree = workspace.root != self.paths.repo_root if resolved or in_unit_worktree or self.policy.scm.rollback_on_failure: + # `preserve_ref` names where *this* rollback parked the attempt. Clear + # it first: a later attempt that parks nothing (clean tree, no commits + # above baseline, or a preserve failure) must not inherit the previous + # attempt's ref — the defer notice would then send the operator to work + # that is not the deferred attempt's. + task.preserve_ref = None self.journal.append( "rollback-auto", story_key=task.story_key, @@ -332,6 +338,7 @@ def preserve_attempt_commits(self, task: StoryTask, *, allow_pause: bool) -> Non # the operator to blindly `reset --hard` (that would discard them). self.pause_for_manual_recovery(task, baseline, preserve_failed=True) return # re-drive: never pause — proceed to the (human-directed) reset + task.preserve_ref = ref self.journal.append( "attempt-commits-preserved", story_key=task.story_key, ref=ref, count=len(commits) ) @@ -371,6 +378,11 @@ def preserve_attempt_worktree(self, task: StoryTask) -> None: ) return if parked: + # Last writer wins over preserve_attempt_commits' branch on purpose: + # the snapshot is commit-tree'd parented at the attempt's HEAD + # (verify.snapshot_worktree), so it already contains the commits that + # branch points at — one ref recovers the whole attempt. + task.preserve_ref = parked self.journal.append("attempt-worktree-preserved", story_key=task.story_key, ref=parked) def pause_for_manual_recovery( diff --git a/tests/test_cli.py b/tests/test_cli.py index 940689bb..8f300979 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1093,6 +1093,37 @@ def test_status_json_defer_and_commit_are_separate_fields(project, capsys): assert entry_done["defer_reason"] is None +def test_status_json_carries_the_preserve_ref(project, capsys): + """#333: the recovery ref a rolled-back attempt was parked on is reported + verbatim from state.json — status never runs git, so a ref a later run's + `preserve_keep` pruning deleted is still named rather than silently dropped.""" + from bmad_loop.model import Phase, StoryTask + + deferred = StoryTask(story_key="1-1-login", epic=1, phase=Phase.DEFERRED) + deferred.defer_reason = "review did not converge within budget" + deferred.preserve_ref = "refs/attempt-preserve-dirty/run-1-abcd1234-2" + done = StoryTask(story_key="1-2-logout", epic=1, phase=Phase.DONE) + _make_run_with_tokens(project, {"1-1-login": deferred, "1-2-logout": done}, weight=0.1) + + doc = _status_json(project, capsys) + entry_deferred, entry_done = doc["tasks"] + assert entry_deferred["preserve_ref"] == "refs/attempt-preserve-dirty/run-1-abcd1234-2" + assert entry_done["preserve_ref"] is None + + +def test_status_text_names_the_preserve_ref_beside_the_reason(project, capsys): + from bmad_loop.model import Phase, StoryTask + + deferred = StoryTask(story_key="1-1-login", epic=1, phase=Phase.DEFERRED) + deferred.defer_reason = "review did not converge within budget" + deferred.preserve_ref = "attempt-preserve/run-1-abcd1234" + _make_run_with_tokens(project, {"1-1-login": deferred}, weight=0.1) + + assert cli.main(["status", "--project", str(project.project)]) == 0 + out = capsys.readouterr().out + assert "review did not converge within budget [attempt-preserve/run-1-abcd1234]" in out + + def test_status_json_stories_mode_is_pure_json(project, capsys): """--json must skip every text trailer (stories board, backlog, decisions nudge) — the stories-mode board would otherwise corrupt the document.""" diff --git a/tests/test_engine.py b/tests/test_engine.py index d28e1781..fa133d3f 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -3231,6 +3231,57 @@ def test_budget_exhausted_unfinalized_defers(project): assert not spec_path(project, "1-1-a").exists() stashed = engine.run_dir / "deferred" / "1-1-a" / "spec-1-1-a.md" assert stashed.is_file() and "status: 'in-progress'" in stashed.read_text() + # #333: the rollback parked the attempt on a recovery ref — run state names it + # and the notification hands the operator the one command that restores it, + # instead of leaving them to hunt through `git log --all`. + ref = task.preserve_ref + assert ref and ref.startswith("refs/attempt-preserve-dirty/") + git(project.project, "rev-parse", "--verify", ref) + attention = (engine.run_dir / "ATTENTION").read_text() + assert "story deferred: 1-1-a" in attention + assert f"attempt work parked at `{ref}`" in attention + assert f'git -C "{project.project}" merge --ff-only {ref}' in attention + deferred_entry = [e for e in engine.journal.entries() if e["kind"] == "story-deferred"][-1] + assert deferred_entry["preserve_ref"] == ref + + +def test_defer_notification_stays_bare_when_nothing_was_parked(project): + """Ablation target: make `_defer_recovery_note`'s tail unconditional and this + fails. Over-budget sessions touch no files, so the exhaustion defer's rollback + finds a clean tree and parks nothing — there is no ref to name, and the notice + must not advertise one (a `merge --ff-only` onto a ref that was never created + is worse than silence).""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + engine, _ = make_engine( + project, + [ + SessionResult(status="over_budget", budget_weighted=5_000_000), + SessionResult(status="over_budget", budget_weighted=6_000_000), + ], + ) + summary = engine.run() + + assert summary.deferred == 1 + task = engine.state.tasks["1-1-a"] + assert task.phase == Phase.DEFERRED and task.preserve_ref is None + assert "rollback-skipped-clean" in [e["kind"] for e in engine.journal.entries()] + attention = (engine.run_dir / "ATTENTION").read_text() + assert "story deferred: 1-1-a" in attention + assert "attempt work parked" not in attention and "merge --ff-only" not in attention + + +def test_defer_recovery_note_is_uniform_across_ref_families(project): + """A commits branch and a dirty snapshot both fast-forward, so the recovery + line is the same command either way — the caller never has to know which + family parked the attempt.""" + engine, _ = make_engine(project, []) + task = StoryTask(story_key="1-1-a", epic=1) + assert engine._defer_recovery_note(task) == "" + for ref in ("attempt-preserve/test-run-0badc0de", "refs/attempt-preserve-dirty/test-run-de-1"): + task.preserve_ref = ref + note = engine._defer_recovery_note(task) + assert f"parked at `{ref}`" in note + assert f'git -C "{project.project}" merge --ff-only {ref}' in note def test_budget_exhausted_defer_reason_names_last_status(project): diff --git a/tests/test_engine_worktree.py b/tests/test_engine_worktree.py index a0670fa5..b6e310f2 100644 --- a/tests/test_engine_worktree.py +++ b/tests/test_engine_worktree.py @@ -315,6 +315,13 @@ def test_worktree_defer_keeps_failed_unit(project): # the main repo is untouched by the failed unit assert "change for 1-1-a" not in (project.project / "src.txt").read_text() assert worktree_clean(project.project) + # #333: nothing was rolled back here, so the notification points at the kept + # branch rather than a recovery ref — and `preserve_ref` stays unset, because + # a live unit branch is not a parked snapshot of a discarded attempt. + assert task.preserve_ref is None + attention = (engine.run_dir / "ATTENTION").read_text() + assert "story deferred: 1-1-a" in attention + assert "failed work kept on branch `bmad-loop/test-run/1-1-a`" in attention def test_worktree_defer_without_keep_drops_worktree_but_saves_patch(project): @@ -332,6 +339,10 @@ def test_worktree_defer_without_keep_drops_worktree_but_saves_patch(project): # not kept → worktree removed, branch deleted assert not branch_exists(project.project, "bmad-loop/test-run/1-1-a") assert [p.resolve() for p in worktree_list(project.project)] == [project.project.resolve()] + # #333: the branch is gone, so the notification must not name it — the patch + # in the run dir is the only surviving artifact. + attention = (engine.run_dir / "ATTENTION").read_text() + assert "story deferred: 1-1-a" in attention and "kept on branch" not in attention def test_worktree_defer_then_next_story_succeeds(project): diff --git a/tests/test_model.py b/tests/test_model.py index b6c6789f..999d8af0 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -171,6 +171,17 @@ def test_restore_patch_defaults_none_for_legacy_state(): assert StoryTask.from_dict(doc).restore_patch is None +def test_preserve_ref_round_trips(): + task = StoryTask(story_key="1-1-a", epic=1, preserve_ref="attempt-preserve/run-1-abcd1234") + assert StoryTask.from_dict(task.to_dict()).preserve_ref == "attempt-preserve/run-1-abcd1234" + + +def test_preserve_ref_defaults_none_for_legacy_state(): + doc = StoryTask(story_key="1-1-a", epic=1).to_dict() + del doc["preserve_ref"] # state.json from before the field existed + assert StoryTask.from_dict(doc).preserve_ref is None + + def test_stopped_round_trips(): state = _state(stopped=True) assert RunState.from_dict(state.to_dict()).stopped is True diff --git a/tests/test_recovery_flow.py b/tests/test_recovery_flow.py index 7eb1a19b..8c1ed8d1 100644 --- a/tests/test_recovery_flow.py +++ b/tests/test_recovery_flow.py @@ -281,6 +281,7 @@ def test_preserve_attempt_commits_parks_committed_work(project): ref = flow.journal.fields("attempt-commits-preserved")["ref"] assert ref.startswith("attempt-preserve/") git(repo, "rev-parse", "--verify", ref) # the recovery branch exists + assert task.preserve_ref == ref # #333: the ref reaches run state, not just the journal def test_preserve_attempt_commits_noop_without_commits(project): @@ -292,6 +293,7 @@ def test_preserve_attempt_commits_noop_without_commits(project): assert "attempt-commits-preserved" not in flow.journal.events() assert flow.calls.pauses == [] + assert task.preserve_ref is None # nothing parked → nothing to point the operator at def test_preserve_attempt_commits_pauses_when_ref_fails_and_allowed(project, monkeypatch): @@ -314,6 +316,8 @@ def no_ref(*a, **k): assert "attempt-preserve-failed" in flow.journal.events() # preserve_failed=True routes through the committed-work notice assert "could not be auto-preserved" in flow.calls.pauses[-1][0] + # the ref never took — run state must not name one (#333) + assert task.preserve_ref is None def test_preserve_attempt_commits_no_pause_on_redrive(project, monkeypatch): @@ -347,6 +351,62 @@ def test_preserve_attempt_worktree_snapshots_dirty_tree(project): ref = flow.journal.fields("attempt-worktree-preserved")["ref"] assert ref.startswith("refs/attempt-preserve-dirty/") git(repo, "rev-parse", "--verify", ref) # the snapshot ref exists + assert task.preserve_ref == ref # #333 + + +def test_dirty_preserve_ref_wins_and_subsumes_the_commits_branch(project): + """Both families fire on one rollback: commits above baseline are parked on an + `attempt-preserve/*` branch and the still-dirty tree on a dirty snapshot. The + dirty ref is the one `preserve_ref` keeps, because it is committed parented at + the attempt's HEAD and therefore already contains the branch — so the single + `git merge --ff-only ` the defer notice prints recovers the + whole attempt, not half of it.""" + 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") + (repo / "src.txt").write_text("committed then edited\n") # uncommitted on top + + flow.rollback_or_pause(task) + + branch = flow.journal.fields("attempt-commits-preserved")["ref"] + dirty = flow.journal.fields("attempt-worktree-preserved")["ref"] + assert task.preserve_ref == dirty != branch + # subsumption: the branch tip is an ancestor of the dirty snapshot + git(repo, "merge-base", "--is-ancestor", branch, dirty) + + +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 + nothing — here the ref simply fails to take on a pause-free re-drive — must not + leave the *earlier* attempt's ref standing, or the defer notice sends the + operator to work that is not the deferred attempt's.""" + 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("attempt 1\n") + git(repo, "add", "-A") + git(repo, "commit", "-q", "-m", "attempt 1") + + flow.rollback_or_pause(task) + stale = task.preserve_ref + assert stale and stale.startswith("attempt-preserve/") + + task.attempt = 1 + (repo / "src.txt").write_text("attempt 2\n") + git(repo, "add", "-A") + git(repo, "commit", "-q", "-m", "attempt 2") + monkeypatch.setattr(verify, "preserve_commits", lambda *a, **k: None) # ref did not take + + flow.rollback_or_pause(task, cause="resolved") # re-drive: never pauses + + assert "attempt-preserve-failed" in flow.journal.events() + assert task.preserve_ref is None # --------------------------------------------------------------- safe_reset From 89444a6f4b3b11b7ad0e8fdfc1923ef90f8631d0 Mon Sep 17 00:00:00 2001 From: pbean Date: Mon, 27 Jul 2026 14:14:18 -0700 Subject: [PATCH 2/2] fix(recovery): flag a commits-only park, name isolated preserve refs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects found reviewing the automated feedback on #338. `preserve_attempt_worktree` catches `verify.GitError` and returns without touching `preserve_ref`, so a failed dirty snapshot leaves the commits branch standing while `safe_reset` destroys the uncommitted half anyway. The notice then offered the uniform `merge --ff-only` line for what is only half the attempt — contradicting `_defer_recovery_note`'s own docstring, which claimed the note is empty on a preservation failure. A ref name cannot carry this: a commits-only ref is complete when the tree was clean. New `StoryTask.preserve_partial` records it and downgrades the claim. `preserve_ref` is not isolation-scoped: a unit worktree's own dev-retry rollback parks on the same shared refs, so a deferred isolated unit can carry both a kept-failed branch and an earlier attempt's ref. The isolated notice named only the branch while `status --json` reported the ref — #333's own failure mode. It now names both, without a merge command: that ref is not fast-forwardable there, and offering it would land a discarded attempt on the operator's branch. The isolated `story-deferred` entry gains `preserve_ref=` to match the in-place arm. `test_worktree_defer_keeps_failed_unit`'s `preserve_ref is None` passed for the wrong reason — its script never rolls back. Its premise is now pinned. Declined from the bot findings: the README wording (the bullet never mentions `keep_failed`, and "parked" vs "stay mounted" are already disjoint) and the journal `""`-vs-`null` divergence (`rollback-auto` two lines up already writes `baseline=... or ""`; no consumer joins the two). --- CHANGELOG.md | 5 +- README.md | 2 +- docs/FEATURES.md | 2 +- src/bmad_loop/engine.py | 53 +++++++++++++---- src/bmad_loop/model.py | 30 +++++++--- src/bmad_loop/recovery_flow.py | 22 +++++-- tests/test_engine.py | 24 ++++++++ tests/test_engine_worktree.py | 105 +++++++++++++++++++++++++-------- tests/test_model.py | 17 +++++- tests/test_recovery_flow.py | 41 +++++++++++++ 10 files changed, 245 insertions(+), 56 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b85cb5d..6fd24178 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,8 +13,9 @@ breaking changes may land in a minor release. 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 the operator to find their work with `git log --all`. The notice now names the ref and the - `git merge --ff-only` command that restores it; an isolated unit's notice names its kept-failed - branch instead. New `preserve_ref` on each task, projected into `bmad-loop status` and + `git merge --ff-only` command that restores it — downgraded to commits-only when the dirty + snapshot could not be captured; an isolated unit's notice names its kept-failed branch and any + earlier rolled-back attempt's ref. New `preserve_ref` on each task, projected into `bmad-loop status` and `--json` (additive; schema stays 1). Reported verbatim from run state — `status` never runs git, so a ref since pruned by `scm.preserve_keep` is still named. diff --git a/README.md b/README.md index 29e52dbe..ada7418b 100644 --- a/README.md +++ b/README.md @@ -462,7 +462,7 @@ By default work happens **in place** on the checked-out branch (`[scm] isolation - **`branch_per`** — `story` (a branch per story) or `run` (one shared branch across the run; this forces `delete_branch = false` so the shared branch survives between units). - **`target_branch`** — the branch every unit merges into; empty means the branch checked out at run start. A configured branch is created if missing (a detached HEAD or unborn repo pauses the run rather than merging onto an unreferenced commit). - **`keep_failed`** (default on) — a deferred/escalated unit's worktree + branch stay mounted for inspection, and its full diff (tracked + untracked) is preserved to `run_dir/failed//changes.patch`. `failed_diff_max_mb` caps the per-file size of untracked files in that patch (oversized files skipped with a marker); `failed_diff_unlimited` lifts the cap. -- **`rollback_on_failure`** (default off) — governs in-place recovery when `isolation = "none"`. Off pauses a failed unattended attempt with manual-recovery steps and leaves the tree untouched; on auto-reverts the attempt's tracked changes and removes only the untracked files the run created (never a blanket `git clean`). A resolved escalation's re-drive always auto-recovers regardless of this flag. Under `isolation = "worktree"` the worktree is disposable, so a failed attempt's commits are parked on `attempt-preserve/*` refs and the worktree resets regardless — **`preserve_keep`** (default 20) bounds how many of those recovery refs are kept per family at run start, pruning the oldest (`0` = never prune). Whenever an attempt is parked, the ref is named in the story's defer notification (with the `git merge --ff-only` line that recovers it) and in `bmad-loop status`/`--json` as `preserve_ref` — reported verbatim from run state, so a ref that a later run's `preserve_keep` pruning has since deleted is still shown. +- **`rollback_on_failure`** (default off) — governs in-place recovery when `isolation = "none"`. Off pauses a failed unattended attempt with manual-recovery steps and leaves the tree untouched; on auto-reverts the attempt's tracked changes and removes only the untracked files the run created (never a blanket `git clean`). A resolved escalation's re-drive always auto-recovers regardless of this flag. Under `isolation = "worktree"` the worktree is disposable, so a failed attempt's commits are parked on `attempt-preserve/*` refs and the worktree resets regardless — **`preserve_keep`** (default 20) bounds how many of those recovery refs are kept per family at run start, pruning the oldest (`0` = never prune). Whenever an attempt is parked, the ref is named in the story's defer notification (with the `git merge --ff-only` line that recovers it, flagged as commits-only when the uncommitted snapshot could not be captured) and in `bmad-loop status`/`--json` as `preserve_ref` — reported verbatim from run state, so a ref that a later run's `preserve_keep` pruning has since deleted is still shown. - **`commit_message_template`** — when set, the message used for story/bundle commits (`{story_key}` / `{run_id}` substituted). - **`seed_adapter_defaults`** (default on) / **`worktree_seed`** — a worktree checks out _tracked_ files only, so a project's gitignored MCP/CLI configs (`.mcp.json`, `.claude/settings.json`, `.codex/config.toml`, `.gemini/settings.json`) are absent from a fresh worktree — without them an isolated session can't reach its MCP server and stalls on readiness. With `seed_adapter_defaults` on, each loaded adapter's own configs are copied in from the main repo before the session launches (the defaults live in each CLI profile's `seed_files`); `worktree_seed` adds extra project-relative paths on top. Seeding is copy-when-absent and runs before the signal-hook merge, so a seeded `settings.json` keeps its real content and just gains the Stop hook — and the seeded paths are shielded from the unit's `git add -A`. diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 243c53f3..b9df1f77 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -59,7 +59,7 @@ 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; isolated, the kept-failed unit branch — and the ref is projected as `preserve_ref` in `status` and `--json` (#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; 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 58b8f022..292f7a9a 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -2997,24 +2997,46 @@ def _defer_recovery_note(self, task: StoryTask) -> str: A bare reason leaves the operator to find the rolled-back work themselves (the reporter used `git log --all`). In-place, the auto-rollback parked the attempt on `task.preserve_ref`, which fast-forwards straight back. Isolated, - nothing is rolled back: a kept-failed unit's branch stays mounted. - - Empty when there is nothing to point at — a clean-tree defer, a preservation - failure, rollback off, or `keep_failed` off — so the notice can never - advertise a ref that was not created. The pointer is a name, not a promise: - `scm.preserve_keep` prunes the oldest refs at a later run's start, and - nothing here re-validates it (this must stay git-free — `status` reads + *this* defer rolls nothing back — a kept-failed unit's branch stays mounted — + but an EARLIER attempt's in-worktree dev-retry rollback parks on the same + shared refs, so `preserve_ref` can be set here too. Both are named; the + isolated arm deliberately prints no `merge --ff-only` line, because that ref + is not fast-forwardable from either tree (the unit branch has since moved on + with the deferred attempt's commits, and fast-forwarding it into the + operator's checkout would land a *discarded* attempt on their branch). + + Empty when there is nothing to point at — a clean-tree defer that parked + nothing, a commits-preserve failure, rollback off, or `keep_failed` off with + no earlier ref — so the notice can never advertise a ref that was not + created. A *worktree*-snapshot failure is the one case that still points + somewhere: `preserve_partial` then downgrades the claim to the committed + half instead of hiding a ref that does exist. The pointer is a name, not a + promise: `scm.preserve_keep` prunes the oldest refs at a later run's start, + and nothing here re-validates it (this must stay git-free — `status` reads `state.json` only).""" if self._isolated: + note = "" if self.policy.scm.keep_failed and task.branch: - return f" — failed work kept on branch `{task.branch}`" - return "" + note = f" — failed work kept on branch `{task.branch}`" + if task.preserve_ref: + half = " (commits only)" if task.preserve_partial else "" + note += ( + f" — an earlier rolled-back attempt is parked at `{task.preserve_ref}`{half}" + ) + return note if not task.preserve_ref: return "" - return ( - f" — attempt work parked at `{task.preserve_ref}`; recover with " - f'`git -C "{self.workspace.root}" merge --ff-only {task.preserve_ref}`' + recover = ( + f'; recover with `git -C "{self.workspace.root}" ' + f"merge --ff-only {task.preserve_ref}`" ) + if task.preserve_partial: + return ( + f" — attempt COMMITS parked at `{task.preserve_ref}`{recover}; the " + f"uncommitted changes could not be captured (journal: " + f"attempt-worktree-preserve-failed) and did not survive the rollback" + ) + return f" — attempt work parked at `{task.preserve_ref}`{recover}" def _defer(self, task: StoryTask, reason: str) -> None: task.defer_reason = reason @@ -3023,7 +3045,12 @@ 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) + 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, diff --git a/src/bmad_loop/model.py b/src/bmad_loop/model.py index dacd0fa7..bb3d35f4 100644 --- a/src/bmad_loop/model.py +++ b/src/bmad_loop/model.py @@ -178,14 +178,28 @@ class StoryTask: # — an `attempt-preserve/*` branch (commits above baseline) or, when the tree # was also dirty, the `refs/attempt-preserve-dirty/*` snapshot, which is # parented at the attempt's HEAD and therefore subsumes the branch (last - # writer wins, so one `git merge --ff-only ` recovers the whole attempt). - # Set by RecoveryFlow, cleared at the top of every auto-rollback so it can - # never name a *previous* attempt's ref; read by `_defer` (notification) and - # projected into `status`. None = nothing was parked (clean tree, preserve - # failure, or an isolated unit, whose branch stays mounted instead). Not - # cleared on success — a mid-retry rollback's breadcrumb stays readable. - # Survives the resume serialization round-trip. + # writer wins, so one `git merge --ff-only ` recovers the whole attempt + # — unless `preserve_partial` is set). Set by RecoveryFlow, cleared at the top + # of every auto-rollback so it can never name a *previous* attempt's ref; read + # by `_defer` (notification) and projected into `status`. None = the last + # auto-rollback parked nothing (no commits above baseline and a clean or + # uncapturable tree, or the ref failed to take). Isolation-INDEPENDENT: a unit + # worktree's own dev-retry rollback parks on the same shared refs, so a + # deferred isolated unit can carry BOTH a kept-failed branch (the final + # attempt) and a preserve_ref (an earlier, rolled-back one) — `_defer` names + # both. The unit branch itself is never written here: a live branch is not a + # parked snapshot. Not cleared on success — a mid-retry rollback's breadcrumb + # stays readable. Survives the resume serialization round-trip. preserve_ref: str | None = None + # set when the auto-rollback's *worktree* snapshot was attempted and raised + # (journalled `attempt-worktree-preserve-failed`), so `preserve_ref` names an + # `attempt-preserve/*` commits branch ALONE and the reset that followed + # discarded the uncommitted half. False both when the snapshot succeeded (the + # dirty ref subsumes the branch) and when the tree was clean (nothing to + # capture, so the commits branch IS the whole attempt) — the ref name alone + # cannot tell those apart, which is why this is recorded rather than derived. + # Cleared with `preserve_ref`. Survives the resume serialization round-trip. + preserve_partial: bool = False # set by runs.rearm_escalation: this task was re-armed out of ESCALATED for a # clean rebuild against the corrected spec (not a failed attempt). Lets the # resume-time manual-recovery notice describe the real cause; cleared once the @@ -279,6 +293,7 @@ def to_dict(self) -> dict[str, Any]: "commit_sha": self.commit_sha, "defer_reason": self.defer_reason, "preserve_ref": self.preserve_ref, + "preserve_partial": self.preserve_partial, "rearmed": self.rearmed, "resolved_redrive": self.resolved_redrive, "plan_checkpoint_pending": self.plan_checkpoint_pending, @@ -327,6 +342,7 @@ def from_dict(cls, d: dict[str, Any]) -> "StoryTask": commit_sha=d.get("commit_sha"), defer_reason=d.get("defer_reason"), preserve_ref=d.get("preserve_ref"), + preserve_partial=bool(d.get("preserve_partial", False)), rearmed=bool(d.get("rearmed", False)), resolved_redrive=bool(d.get("resolved_redrive", False)), plan_checkpoint_pending=bool(d.get("plan_checkpoint_pending", False)), diff --git a/src/bmad_loop/recovery_flow.py b/src/bmad_loop/recovery_flow.py index daa97006..e7c09eea 100644 --- a/src/bmad_loop/recovery_flow.py +++ b/src/bmad_loop/recovery_flow.py @@ -164,11 +164,15 @@ def rollback_or_pause(self, task: StoryTask, *, cause: str = "stopped") -> None: in_unit_worktree = workspace.root != self.paths.repo_root if resolved or in_unit_worktree or self.policy.scm.rollback_on_failure: # `preserve_ref` names where *this* rollback parked the attempt. Clear - # it first: a later attempt that parks nothing (clean tree, no commits - # above baseline, or a preserve failure) must not inherit the previous + # it first: a later attempt that parks nothing (no commits above + # baseline, or a preserve failure) must not inherit the previous # attempt's ref — the defer notice would then send the operator to work - # that is not the deferred attempt's. + # that is not the deferred attempt's. A *clean*-tree rollback never gets + # here (the `rollback-skipped-clean` return above fires first), so it + # deliberately keeps the earlier ref: nothing new was parked, and that + # ref is then the only place the story's work survives. task.preserve_ref = None + task.preserve_partial = False self.journal.append( "rollback-auto", story_key=task.story_key, @@ -376,12 +380,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. + task.preserve_partial = True return if parked: # Last writer wins over preserve_attempt_commits' branch on purpose: # the snapshot is commit-tree'd parented at the attempt's HEAD # (verify.snapshot_worktree), so it already contains the commits that - # branch points at — one ref recovers the whole attempt. + # branch points at — one ref recovers the whole attempt. That holds on + # this success path only; the `except` above records the case where the + # snapshot failed and the commits branch is all that survived. task.preserve_ref = parked self.journal.append("attempt-worktree-preserved", story_key=task.story_key, ref=parked) diff --git a/tests/test_engine.py b/tests/test_engine.py index fa133d3f..12ba8bcc 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -3284,6 +3284,30 @@ def test_defer_recovery_note_is_uniform_across_ref_families(project): assert f'git -C "{project.project}" merge --ff-only {ref}' in note +def test_defer_recovery_note_flags_a_commits_only_park(project): + """When the dirty snapshot failed, `preserve_ref` names the commits branch + alone and the reset discarded the rest — the notice must say so. It still + prints the merge command: the committed half IS recoverable, and scaring the + operator off a valid recovery would be its own defect. + + Ablation targets: delete the `if task.preserve_partial:` branch and the first + half fails; make it unconditional and the second half fails.""" + engine, _ = make_engine(project, []) + ref = "attempt-preserve/test-run-0badc0de" + task = StoryTask(story_key="1-1-a", epic=1, preserve_ref=ref, preserve_partial=True) + + note = engine._defer_recovery_note(task) + assert f"attempt COMMITS parked at `{ref}`" in note + assert "did not survive the rollback" in note + assert "attempt-worktree-preserve-failed" in note # names the breadcrumb + assert f'git -C "{project.project}" merge --ff-only {ref}' in note + + task.preserve_partial = False # a complete park keeps the unqualified wording + note = engine._defer_recovery_note(task) + assert f" — attempt work parked at `{ref}`" in note + assert "did not survive" not in note and "COMMITS" not in note + + def test_budget_exhausted_defer_reason_names_last_status(project): """The exhaustion defer reason reflects the last completed pass's real status (issue #160). Every review pass leaves the spec non-terminal (in-progress), so diff --git a/tests/test_engine_worktree.py b/tests/test_engine_worktree.py index b6e310f2..1ade3d77 100644 --- a/tests/test_engine_worktree.py +++ b/tests/test_engine_worktree.py @@ -93,6 +93,35 @@ def effect(spec): return effect +def wt_bad_dev(project, story_key): + """Dev session that commits inside the unit worktree and then claims a foreign + baseline. `verify_dev` rejects that NON-fixably, so the retry routes through + `_rollback_or_pause` — which, inside a mounted worktree, auto-recovers and parks + the attempt on an `attempt-preserve/*` ref (#161). The one composable way to + reach an in-worktree rollback; `wt_dev_effect` never does.""" + + def effect(spec): + cwd = spec.cwd + wt = project.rebased(cwd) + src = cwd / "src.txt" + src.write_text(src.read_text() + "bad attempt\n") + git(cwd, "add", "-A") + git(cwd, "commit", "-q", "-m", "bad attempt work") + sp = wt.implementation_artifacts / f"spec-{story_key}.md" + write_spec(sp, "in-review", "0" * 40) + return SessionResult( + status="completed", + result_json={ + "workflow": "auto-dev", + "story_key": story_key, + "spec_file": str(sp), + "escalations": [], + }, + ) + + return effect + + def wt_review_effect(project, story_key, clean: bool, patched: int = 0): """Follow-up review pass in a worktree — a bmad-dev-auto re-invocation on the done spec. ``clean=True`` converges; ``clean=False`` keeps recommending.""" @@ -315,9 +344,11 @@ def test_worktree_defer_keeps_failed_unit(project): # the main repo is untouched by the failed unit assert "change for 1-1-a" not in (project.project / "src.txt").read_text() assert worktree_clean(project.project) - # #333: nothing was rolled back here, so the notification points at the kept - # branch rather than a recovery ref — and `preserve_ref` stays unset, because - # a live unit branch is not a parked snapshot of a discarded attempt. + # #333: this script never rolls back (dev succeeds; the reviews plateau), so + # nothing was parked and the notice points at the kept branch alone. Pin that + # premise — a bare `is None` would also pass if isolation simply never parked, + # which is false: see test_isolated_defer_names_the_earlier_rolled_back_attempt. + assert "rollback-auto" not in journal_kinds(engine) assert task.preserve_ref is None attention = (engine.run_dir / "ATTENTION").read_text() assert "story deferred: 1-1-a" in attention @@ -1399,31 +1430,10 @@ def test_dev_retry_in_worktree_auto_recovers_instead_of_pausing(project): touched. rollback_on_failure gates isolation="none" recovery only.""" commit_sprint(project, {"1-1-a": "ready-for-dev"}) - def bad_dev(spec): - # commits work in the worktree, then claims a foreign baseline — the - # non-fixable verify retry that routes through _rollback_or_pause - cwd = spec.cwd - wt = project.rebased(cwd) - src = cwd / "src.txt" - src.write_text(src.read_text() + "bad attempt\n") - git(cwd, "add", "-A") - git(cwd, "commit", "-q", "-m", "bad attempt work") - sp = wt.implementation_artifacts / "spec-1-1-a.md" - write_spec(sp, "in-review", "0" * 40) - return SessionResult( - status="completed", - result_json={ - "workflow": "auto-dev", - "story_key": "1-1-a", - "spec_file": str(sp), - "escalations": [], - }, - ) - engine, _ = make_engine( project, [ - bad_dev, + wt_bad_dev(project, "1-1-a"), wt_dev_effect(project, "1-1-a"), wt_review_effect(project, "1-1-a", clean=True), ], @@ -1445,6 +1455,51 @@ def bad_dev(spec): assert worktree_clean(project.project) +def test_isolated_defer_names_the_earlier_rolled_back_attempt(project): + """#333: an isolated defer is NOT the only place a story's work can live. The + first attempt's non-fixable retry rolled back *inside* the unit worktree and + parked its commits on a shared `attempt-preserve/*` ref; the second attempt + exhausts the budget and defers with its own work kept on the unit branch. Both + survive, so the notice must name both — `status --json` already reports the ref, + and a notice that mentioned only the branch is exactly the "hunt with + `git log --all`" failure #333 was filed for. + + No `merge --ff-only` line here on purpose: that ref is not fast-forwardable from + either tree, and offering it would land a discarded attempt on the operator's + branch. + + Ablations: drop the `if task.preserve_ref:` clause in `_defer_recovery_note`'s + isolated arm → the both-named assertion fails; drop `preserve_ref=` from the + isolated `story-deferred` emit → the journal assertion fails; route the isolated + arm through the in-place tail → the no-command assertion fails.""" + commit_sprint(project, {"1-1-a": "ready-for-dev"}) + engine, _ = make_engine( + project, + [wt_bad_dev(project, "1-1-a"), wt_bad_dev(project, "1-1-a")], + policy=wt_policy(), + ) + summary = engine.run() + + assert summary.deferred == 1 and not summary.paused + task = engine.state.tasks["1-1-a"] + assert task.phase == Phase.DEFERRED + # falsifies the "an isolated unit parks nothing" reading: the in-worktree + # rollback wrote a real ref, and it is reachable from the MAIN repo because + # linked worktrees share the common git dir's ref namespace + ref = task.preserve_ref + assert ref and ref.startswith(("attempt-preserve/", "refs/attempt-preserve-dirty/")) + git(project.project, "rev-parse", "--verify", ref) + + attention = (engine.run_dir / "ATTENTION").read_text() + assert "story deferred: 1-1-a" in attention + assert "failed work kept on branch `bmad-loop/test-run/1-1-a`" in attention + assert f"an earlier rolled-back attempt is parked at `{ref}`" in attention + assert "merge --ff-only" not in attention + + deferred_entry = [e for e in engine.journal.entries() if e["kind"] == "story-deferred"][-1] + assert deferred_entry["preserve_ref"] == ref + + # ------------------------------------ story-declared deferred-work closure (#234) diff --git a/tests/test_model.py b/tests/test_model.py index 999d8af0..6ff09369 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -172,14 +172,25 @@ def test_restore_patch_defaults_none_for_legacy_state(): def test_preserve_ref_round_trips(): - task = StoryTask(story_key="1-1-a", epic=1, preserve_ref="attempt-preserve/run-1-abcd1234") - assert StoryTask.from_dict(task.to_dict()).preserve_ref == "attempt-preserve/run-1-abcd1234" + task = StoryTask( + story_key="1-1-a", + epic=1, + preserve_ref="attempt-preserve/run-1-abcd1234", + preserve_partial=True, + ) + restored = StoryTask.from_dict(task.to_dict()) + assert restored.preserve_ref == "attempt-preserve/run-1-abcd1234" + # the partial marker must survive resume too, or a resumed run's defer notice + # silently re-acquires the whole-attempt promise for a commits-only ref + assert restored.preserve_partial is True def test_preserve_ref_defaults_none_for_legacy_state(): doc = StoryTask(story_key="1-1-a", epic=1).to_dict() del doc["preserve_ref"] # state.json from before the field existed - assert StoryTask.from_dict(doc).preserve_ref is None + del doc["preserve_partial"] + restored = StoryTask.from_dict(doc) + assert restored.preserve_ref is None and restored.preserve_partial is False def test_stopped_round_trips(): diff --git a/tests/test_recovery_flow.py b/tests/test_recovery_flow.py index 8c1ed8d1..97d89018 100644 --- a/tests/test_recovery_flow.py +++ b/tests/test_recovery_flow.py @@ -377,6 +377,47 @@ def test_dirty_preserve_ref_wins_and_subsumes_the_commits_branch(project): assert task.preserve_ref == dirty != branch # subsumption: the branch tip is an ancestor of the dirty snapshot git(repo, "merge-base", "--is-ancestor", branch, dirty) + # Ablation target for the partial marker: hoist `preserve_partial = True` out of + # preserve_attempt_worktree's `except` and this fails. A park that captured + # everything must never be libelled as commits-only. + assert task.preserve_partial is False + + +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. + + 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)) + task = _task(repo) + (repo / "src.txt").write_text("committed\n") + git(repo, "add", "-A") + 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) + + flow.rollback_or_pause(task) + + 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/") + assert task.preserve_partial is True + # why it matters: the reset ran regardless, so the tree is back at baseline and + # the ref the notice names carries the committed half ONLY — the uncommitted + # edit survives nowhere, which is exactly what the un-downgraded notice's + # `merge --ff-only` would have implied it could restore + assert "committed" not in (repo / "src.txt").read_text() + assert git(repo, "show", f"{task.preserve_ref}:src.txt") == "committed" def test_rollback_clears_a_previous_attempts_preserve_ref(project, monkeypatch):