From a273498ac73c5ec927657bc4134b062845797e78 Mon Sep 17 00:00:00 2001 From: pbean Date: Mon, 6 Jul 2026 13:18:38 -0700 Subject: [PATCH 01/19] feat(resolve): intent-gap patch-restore re-drive + inline-review-layer preflight MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Integrate the July 4-6 upstream bmad-dev-auto changes into bmad-loop. Phase A - provisioning + doc alignment: - install: require bmad-review-verification-gap (BMAD-METHOD #2550) and a bmad-dev-auto customize.toml marker in the base-skill preflight; the three inline step-04 review hunters are now all bmm prerequisites - devcontract: refresh the in-review reconcile note (restore path re-arms TO in-review pre-session; reconcile semantics unchanged) - verify: document finalize_commit's intentional residual-artifact squash (#2563) - docs: README skill table, FEATURES review-layer + resolve notes, CHANGELOG Phase B - intent-gap patch-restore re-drive (BMAD-METHOD #2564): A review-stage intent-gap halt now saves the reverted attempt as a patch file. When the attempted reading was correct, bmad-loop resolve re-arms the spec to in-review and re-applies that patch onto the baseline before the re-drive, so the session resumes review (step-04) on the restored diff instead of re-implementing from scratch. - model.StoryTask.restore_patch latch; verify.apply_patch; runs.rearm_escalation restore branch (in-review vs ready-for-dev + restore journal field) - engine: Engine._restore_patch applied in the dev loop gated on feedback-is-None (escalates rather than dispatch on apply failure; cleared on commit) - cli: --restore-patch flag + strict validation + resolution.json passthrough - resolve.read_resolution; bmad-loop-resolve SKILL.md guidance Sweep-workflow extension tracked in #75. Co-Authored-By: Claude Fable 5 Re-cut of 29ba684 onto post-stories main (supersedes closed PR #76). Conflicts (model.py, runs.py, test_model.py, CHANGELOG) resolved as a union of both features, folding the restore branch into the stories-hardened re-arm: the in-review flip joins the UnicodeDecodeError-guarded status-flip path, story-escalation-resolved carries baseline+restore, and a patch-restore re-arm on a sentinel-wedged story is rejected outright before any mutation (T1: a sentinel is a pre-planning halt — never lay implementation onto the tree ahead of a planning leg). --- CHANGELOG.md | 11 ++ README.md | 21 ++- docs/FEATURES.md | 5 +- src/bmad_loop/cli.py | 54 +++++- .../data/skills/bmad-loop-resolve/SKILL.md | 36 +++- src/bmad_loop/devcontract.py | 13 +- src/bmad_loop/engine.py | 52 +++++- src/bmad_loop/install.py | 27 +-- src/bmad_loop/model.py | 10 ++ src/bmad_loop/resolve.py | 15 ++ src/bmad_loop/runs.py | 65 +++++-- src/bmad_loop/verify.py | 28 ++++ tests/test_cli.py | 88 ++++++++++ tests/test_engine.py | 158 ++++++++++++++++++ tests/test_install.py | 25 ++- tests/test_model.py | 11 ++ tests/test_runs.py | 63 +++++++ tests/test_verify.py | 43 +++++ 18 files changed, 683 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d7a4bd7..ea34030 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,17 @@ breaking changes may land in a minor release. - **Stories mode — a second planning pipeline that drives the loop off a typed `stories.yaml` (folder+id dispatch) instead of `sprint-status.yaml`.** Opt in with `[stories] source = "stories"` + `spec_folder`, or per run with `bmad-loop run --spec ` (overrides policy); `--story` then filters by story id. Each entry dispatches by folder + id — the dev skill creates-or-resumes the story spec at `/stories/-.md` and the orchestrator reads that id-keyed path back deterministically (no shared board to line-edit, no result-artifact mtime-scan). Strictly linear schedule (list order, no `depends_on`); `bmad-loop run --dry-run --spec ` and `bmad-loop status` print the board (id · live disk state · checkpoint markers · title). Sprint mode is unchanged and remains the default. Requires a `bmad-dev-auto` new enough for folder+id dispatch — the run preflight checks and remediates. - **Per-story human checkpoints (stories mode).** Independent `spec_checkpoint` (pause before code to review the plan — dev halts at `ready-for-dev`; approve to implement, or request a replan that resets the spec to `draft`) and `done_checkpoint` (pause after the story commits, skipped when it is the last story); both additive to `gates.mode`. A blocked story escalates + resolves as in sprint mode, with a pre-planning-halt sentinel auto-deleted (a copy preserved under the run dir) on re-arm. - **TUI human-in-the-loop surface for stories mode.** The sprint tree is replaced by a stories board (id · live disk state · spec/done checkpoint markers · title) when a stories-mode run is selected; paused runs carry a per-run pause-kind badge and the run list shows a global _⚑ N need attention_ count; `p` opens the stage-appropriate viewer — plan-checkpoint spec review (Approve & resume / Request replan), story-checkpoint summary card (Continue / Stop), escalation with story context (Resolve / Re-arm & resume), and a gate spec viewer that the existing spec-approval/epic pauses reuse. The start-run modal gains a source select + spec-folder field with a live schedule preview. Every TUI action calls the same code paths as the CLI. +- **Intent-gap patch-restore recovery.** When review halts on an `intent gap`, `bmad-dev-auto` + now saves the attempted change as a patch before reverting (BMAD-METHOD#2564). If the attempted + reading was correct, `bmad-loop resolve` re-arms the spec to `in-review` and re-applies the patch + onto baseline after every reset, so the re-driven session resumes review on the restored diff + instead of re-implementing. New `--restore-patch ` flag for the `--no-interactive` path; a + patch that fails to apply escalates instead of running on a half-restored tree. +- **Preflight covers the inline review layers.** `bmad-loop validate` (and run-start) now require the + three upstream review-hunter skills `bmad-dev-auto`'s step-04 invokes — `bmad-review-adversarial-general`, + `bmad-review-edge-case-hunter`, and `bmad-review-verification-gap` (new in BMAD-METHOD#2550) — plus a + `customize.toml` in `bmad-dev-auto` (its review-layer config, BMAD-METHOD#2535/#2550). A pre-July bmm + install missing any is reported with remediation before a run stalls. ## [0.8.1] — 2026-07-05 diff --git a/README.md b/README.md index 338fcb0..c0bb006 100644 --- a/README.md +++ b/README.md @@ -40,7 +40,7 @@ Inspired by the original [bmad-automator](https://github.com/bmad-code-org/bmad- - **Python 3.11+**, **tmux**, and a supported coding CLI — `claude` by default; `codex` and `gemini` via [profiles](#other-coding-clis). - **Linux or macOS** (or **Windows via WSL**, which _is_ Linux — it runs as-is). tmux is the one terminal-multiplexer backend today, but it now sits behind a pluggable **registry** of OS seams (transport, process lifecycle, hook interpreter), so a native-Windows backend slots in as new files + a registration line each, with no engine edits — see [Porting bmad-loop to a new OS](docs/porting-to-a-new-os.md). Native Windows is not yet shipped. -- A **BMAD v6 project** (`_bmad/bmm/config.yaml`, a `sprint-status.yaml` from `bmad-sprint-planning`) with the upstream `bmad-dev-auto` skill and the bmad-loop skill module from this repo installed (`bmad-loop-resolve`, `bmad-loop-sweep` — see [Installing the skill module](#installing-the-skill-module)). Standard BMAD skills stay untouched. +- A **BMAD v6 project** (`_bmad/bmm/config.yaml`, a `sprint-status.yaml` from `bmad-sprint-planning`) with the upstream `bmad-dev-auto` skill (and the three review-hunter skills its step-04 invokes inline: `bmad-review-adversarial-general`, `bmad-review-edge-case-hunter`, `bmad-review-verification-gap`) and the bmad-loop skill module from this repo installed (`bmad-loop-resolve`, `bmad-loop-sweep` — see [Installing the skill module](#installing-the-skill-module)). Standard BMAD skills stay untouched. ## Quick start @@ -238,14 +238,19 @@ Bundle dev sessions can themselves append new deferred entries (split-off goals, ## Installing the skill module -The orchestrator drives the upstream `bmad-dev-auto` skill as its inner dev primitive — unmodified, so there is no fork to keep in sync; it both implements and (re-invoked on the done spec) runs the follow-up review — plus its own bundled `bmad-loop-*` skills for escalation, sweep, and setup. Your standard BMAD install is never modified. The three bundled skills ship in the `bmad-loop` wheel (canonical source: `src/bmad_loop/data/skills/`, BMAD module code `bmad-loop`) so `bmad-loop init` lays them down for you; `bmad-dev-auto` is a prerequisite installed by the BMad Method (bmm) module: +The orchestrator drives the upstream `bmad-dev-auto` skill as its inner dev primitive — unmodified, so there is no fork to keep in sync; it both implements and (re-invoked on the done spec) runs the follow-up review — plus its own bundled `bmad-loop-*` skills for escalation, sweep, and setup. Your standard BMAD install is never modified. The three bundled skills ship in the `bmad-loop` wheel (canonical source: `src/bmad_loop/data/skills/`, BMAD module code `bmad-loop`) so `bmad-loop init` lays them down for you; `bmad-dev-auto` and the three review-hunter skills its step-04 invokes inline are prerequisites installed by the BMad Method (bmm) module: -| Skill | Role | -| ------------------- | ------------------------------------------------------------------------------------------- | -| `bmad-dev-auto` | unattended implementation + follow-up review (**upstream** — bmm prerequisite, not bundled) | -| `bmad-loop-resolve` | interactive CRITICAL-escalation resolution (`/bmad-loop-resolve `) | -| `bmad-loop-sweep` | deferred-work ledger triage (automation-only) | -| `bmad-loop-setup` | registers the module in `_bmad/` config + help | +| Skill | Role | +| --------------------------------- | ----------------------------------------------------------------------------------------------- | +| `bmad-dev-auto` | unattended implementation + follow-up review (**upstream** — bmm prerequisite, not bundled) | +| `bmad-review-adversarial-general` | inline step-04 review layer (**upstream** — bmm prerequisite, not bundled) | +| `bmad-review-edge-case-hunter` | inline step-04 review layer (**upstream** — bmm prerequisite, not bundled) | +| `bmad-review-verification-gap` | inline step-04 review layer, newest (BMAD-METHOD#2550) (**upstream** — bmm prereq, not bundled) | +| `bmad-loop-resolve` | interactive CRITICAL-escalation resolution (`/bmad-loop-resolve `) | +| `bmad-loop-sweep` | deferred-work ledger triage (automation-only) | +| `bmad-loop-setup` | registers the module in `_bmad/` config + help | + +`bmad-loop validate` preflights all four upstream skills; a target project on a pre-July bmm install missing `bmad-review-verification-gap` (or a `bmad-dev-auto` without its `customize.toml` review-layer config) is reported with bmm-module remediation before any run starts. **Via uv + `bmad-loop init` (self-sufficient).** Installing the tool and running `init` is all you need — `init` installs the `bmad-loop-*` skills into `.claude/skills/` (claude) and/or `.agents/skills/` (codex/gemini) for the CLIs you select, alongside the hooks and policy: diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 82e0d84..766c7ab 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -50,9 +50,9 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se ### Adversarial review (review stage) - The follow-up review is a re-invocation of `bmad-dev-auto` on the `done` spec — a fresh-context session with no anchoring bias from the implementer (BMAD-METHOD#2508 routes a `done` spec to a fresh step-04 review pass), so there is no separate review skill. -- Two parallel adversarial layers (Blind Hunter / Edge Case Hunter) → verify findings against code → triage → auto-apply patches → log → defer ambiguity → commit. +- Parallel adversarial layers resolved from the skill's `customize.toml` (defaults: Adversarial-General, Edge-Case-Hunter, and Verification-Gap — the third added by BMAD-METHOD#2550) → verify findings against code → triage → auto-apply patches → log → defer ambiguity → commit. Each layer invokes an upstream `bmad-review-*` skill, so all three are bmm prerequisites the `bmad-loop validate` preflight checks for. - Bounded review loop (`limits.max_review_cycles`, default 3 cycles); done when the pass finishes `done` and no longer recommends a follow-up. This bound is also the oscillation guard for skill-recommended follow-up review. -- Optional (`[review].enabled`, default `true`): set `false` to skip the follow-up review session. The dev pass's own inline review (same two layers, in-context) is then the only review and it finalizes the story to `done` — one session per story instead of two. Verify commands still gate the commit. Applies to story runs and deferred-work sweeps alike. +- Optional (`[review].enabled`, default `true`): set `false` to skip the follow-up review session. The dev pass's own inline review (same layers, in-context) is then the only review and it finalizes the story to `done` — one session per story instead of two. Verify commands still gate the commit. Applies to story runs and deferred-work sweeps alike. - Trigger (`[review].trigger`, default `recommended`): when review is enabled, decides _when_ the follow-up pass runs. `recommended` runs it only when `bmad-dev-auto` sets `followup_review_recommended` on a `done` spec (it self-reviews inline and flags an independent pass only when its review-driven changes were significant — BMAD-METHOD#2505). `always` runs it on every story (pre-0.7.0 behavior). ### Failure handling & resilience @@ -61,6 +61,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se - Plateau-defer: when review won't converge, the story is skipped, the spec stashed into the run dir, deferred-work preserved, the run continues. - Typed escalations: `CRITICAL` pauses the run + notifies (desktop + `ATTENTION` file); `PREFERENCE` is journaled and continues. - 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. +- Intent-gap patch-restore (BMAD-METHOD#2564): when review halts on an `intent gap`, `bmad-dev-auto` saves the attempted change as a patch file (in the implementation-artifacts folder, referenced from the halt output) before reverting the tree. If the attempted reading turns out to be correct, the resolve agent adds `"restore_patch": ""` to its `resolution.json`; the orchestrator then re-arms the spec to `in-review` (not `ready-for-dev`) and re-applies the patch onto the baseline after every reset, so the re-driven session resumes _review_ on the restored diff instead of re-implementing from scratch. A hand-driven `bmad-loop resolve --no-interactive --restore-patch ` does the same. A patch that fails to apply escalates rather than dispatching a session onto a half-restored tree. ### Git worktree isolation (opt-in) diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index fb5acba..6fcb2e8 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -805,6 +805,38 @@ def _confirm(question: str) -> bool: return ans in ("y", "yes") +def _resolve_restore_patch( + project: Path, run_dir: Path, story_key: str, args: argparse.Namespace +) -> tuple[str | None, str | None]: + """Determine the intent-gap patch-restore latch (BMAD-METHOD #2564) for a re-arm. + + Precedence: the explicit ``--restore-patch`` flag (hand-driven recovery) wins; + otherwise, on the interactive path, the resolve agent may have recorded a + ``restore_patch`` field in resolution.json. Returns ``(latch, error)``: a + validated absolute patch path to latch (None = ordinary from-scratch re-drive), + or an error string when a supplied path is missing / outside the project — the + caller aborts strictly rather than silently re-driving from scratch when the + operator asked to restore.""" + raw = getattr(args, "restore_patch", None) + if raw is None and args.interactive: + doc = resolve.read_resolution(run_dir, story_key) + if doc is not None: + val = doc.get("restore_patch") + raw = str(val) if val else None + if not raw: + return None, None + patch = Path(raw) + if not patch.is_absolute(): + patch = project / patch + patch = patch.resolve() + if not patch.is_file() or not patch.is_relative_to(project.resolve()): + return None, ( + f"restore patch {raw!r} is not a file under the project — refusing to " + "re-arm (fix the path, or re-run without a restore to re-drive from scratch)" + ) + return str(patch), None + + def cmd_resolve(args: argparse.Namespace) -> int: from .model import PAUSE_ESCALATION, Phase @@ -875,16 +907,26 @@ def cmd_resolve(args: argparse.Namespace) -> int: file=sys.stderr, ) + # intent-gap patch-restore latch (#2564): validate a supplied patch path before + # prompting, so a bad path fails loudly instead of after a confirmation. + restore_patch, err = _resolve_restore_patch(project, run_dir, story_key, args) + if err is not None: + print(err, file=sys.stderr) + return 1 + # confirm-then-resume (args.resume: None = ask, True = auto, False = re-arm only) if args.resume is None and not _confirm(f"re-arm {story_key} and resume run {args.run_id}?"): print("cancelled — run is still paused at the escalation") return 0 try: - runs.rearm_escalation(run_dir, story_key) + runs.rearm_escalation(run_dir, story_key, restore_patch=restore_patch) except runs.RearmError as e: print(f"error: {e}", file=sys.stderr) return 1 - print(f"re-armed {story_key}") + print( + f"re-armed {story_key}" + + (" (restoring the attempted change for review)" if restore_patch else "") + ) if args.resume is False: print(f"resume when ready: bmad-loop resume {args.run_id}") return 0 @@ -1521,6 +1563,14 @@ def add(name: str, func, help: str, *, aliases=()) -> argparse.ArgumentParser: action="store_false", help="skip the resolve agent (spec already fixed by hand); just re-arm + resume", ) + resolve_p.add_argument( + "--restore-patch", + metavar="PATH", + help="intent-gap patch-restore (#2564): re-arm the spec to `in-review` and " + "re-apply this saved patch before the re-drive, resuming review on the " + "attempted change instead of re-implementing (hand-driven; the interactive " + "agent supplies it via resolution.json)", + ) resolve_p.add_argument( "--resume", action=argparse.BooleanOptionalAction, diff --git a/src/bmad_loop/data/skills/bmad-loop-resolve/SKILL.md b/src/bmad_loop/data/skills/bmad-loop-resolve/SKILL.md index 952d1cc..ea32d58 100644 --- a/src/bmad_loop/data/skills/bmad-loop-resolve/SKILL.md +++ b/src/bmad_loop/data/skills/bmad-loop-resolve/SKILL.md @@ -81,10 +81,14 @@ action of a successful resolution. Schema: "story_key": "", "decision": "", "spec_file": "", - "spec_updated": true + "spec_updated": true, + "restore_patch": "" } ``` +`restore_patch` is **optional** and used only for the intent-gap patch-restore +case below — omit it entirely for an ordinary resolution. + ## What you MUST do 1. **Read the context**, then read the **frozen spec** at `spec_file` in full — @@ -107,6 +111,36 @@ action of a successful resolution. Schema: orchestrator will offer to **re-arm the story and resume the run** (a clean rebuild against the corrected spec). +## Special case: a review-stage `intent gap` with a saved patch + +When the escalation came from the **review step** halting on an `intent gap`, the +dev session first **saved its attempted change as a patch file** (in the +implementation-artifacts folder) before reverting the tree — the escalation +`detail` and the spec's `## Review Triage Log` reference the patch path. That +patch is concrete evidence: it shows exactly which reading of the intent the run +implemented. Use it two ways: + +- **As evidence.** Read the patch (and the diff it represents) to see what the + guessed reading produced — often clearer input for writing the clarification + than the questions alone. +- **When the attempted reading was actually correct.** Sometimes the run's guess + is the right one and only the _intent_ was silent. Present this as an explicit + option to the human: _"the implementation read it as X, which is in fact what we + want — amend the intent to say X, and resume **review** on the already-written + change instead of re-implementing it."_ If the human chooses this: + 1. Still **amend the intent** in the spec so it unambiguously says X (step 4 + above is unchanged — the frozen intent must match the restored code). + 2. Add **`"restore_patch": ""`** to `resolution.json` + (copy the path verbatim from the escalation detail / triage log). + + The orchestrator then re-arms the spec to `in-review`, re-applies the patch onto + the baseline, and re-dispatches — the session resumes at the review step on the + restored diff. **Do NOT `git apply` the patch yourself and do NOT set the spec + status** — the orchestrator does both deterministically at re-arm. + +If the attempted reading was wrong (the common case), omit `restore_patch` +entirely: the orchestrator re-drives from scratch against the corrected intent. + ## What you MUST NOT do - **Do NOT** write the orchestrator's `result.json` — that is a dev/review diff --git a/src/bmad_loop/devcontract.py b/src/bmad_loop/devcontract.py index 66fe1cb..c1b3d3b 100644 --- a/src/bmad_loop/devcontract.py +++ b/src/bmad_loop/devcontract.py @@ -57,11 +57,14 @@ # allowlist: anything else (already-`done`, `blocked`, or an unknown custom token) # is left untouched, so reconciliation can never override a status the skill set on # purpose. `""` covers a blank or missing frontmatter `status:` — `reset_spec_status` -# fills/inserts the line in that case. `in-review` is included because on the sole -# (generic `bmad-dev-auto`) path it is only ever the transient marker step-04 sets at -# its start; the skill self-finalizes to `done`. The legacy `bmad-loop-dev` fork that -# used `in-review` as a deliberate review-handoff terminal is retired, so nothing -# leaves `in-review` on purpose anymore. +# fills/inserts the line in that case. `in-review` is included because step-04 sets +# it transiently at the start of a review pass; the skill self-finalizes to `done`, +# so a spec left AT `in-review` with a prose `done` result is a mid-review interrupt +# safe to reconcile forward. The intent-gap patch-restore re-drive (BMAD-METHOD +# #2564) deliberately re-arms the spec TO `in-review` before a session (so step-01 +# routes straight to step-04 on the restored diff) — but that is the pre-session +# status the re-driven skill then advances past; it never LEAVES a spec at +# `in-review` as a terminal, so the reconcile allowlist semantics are unchanged. RECONCILABLE_FROM = frozenset({"", "draft", "ready-for-dev", "in-progress", "in-review"}) # The leading `---\n …frontmatter… \n---` block, captured in three parts so the diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index 39bf888..c080859 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -874,6 +874,44 @@ def _safe_reset(self, task: StoryTask, *, preserve: tuple[str, ...] = ()) -> Non preserve=preserve, ) + def _restore_patch(self, task: StoryTask) -> None: + """Re-apply the latched intent-gap patch (BMAD-METHOD #2564) onto the + baseline tree so the re-driven session resumes review (step-04) on the + restored diff instead of re-implementing. No-op unless a restore is latched. + + Applied from inside `_dev_phase`'s loop, right before each dispatch that + runs against a fresh baseline (the first attempt and every non-fixable + rollback retry — the loop gates this on ``feedback is None``, so a + fixable-feedback retry that KEEPS the attempt's tree is never double-applied, + and the patch file's own untracked/tracked content is excluded from + `baseline_untracked` because that snapshot is taken before the first apply). + This is the plan's "apply after every baseline reset" seam, placed here + rather than in `_rollback_or_pause` so the patch always lands after the + clean baseline_untracked snapshot — avoiding a mid-re-drive reset preserving + the patch's own new files and then colliding with the re-apply. + + On apply failure we escalate rather than dispatch a session onto a + half-restored tree; the task is mid-dispatch (DEV_RUNNING), so step it to + the escalatable DEV_VERIFY phase first (`_escalate` raises RunPaused).""" + if not task.restore_patch: + return + patch = Path(task.restore_patch) + if not patch.is_absolute(): + patch = self.workspace.root / patch + try: + verify.apply_patch(self.workspace.root, patch) + except verify.GitError as e: + self.journal.append( + "attempt-restore-failed", + story_key=task.story_key, + patch=task.restore_patch, + error=str(e), + ) + if task.phase == Phase.DEV_RUNNING: + advance(task, Phase.DEV_VERIFY) # PENDING/DEV_RUNNING can't escalate directly + self._escalate(task, f"intent-gap restore patch failed to apply: {e}") + self.journal.append("attempt-restored", story_key=task.story_key, patch=task.restore_patch) + def _prune_preserve_refs(self) -> None: """Bounded retention for both recovery-ref families at run start — the attempt-preserve/* branches and the refs/attempt-preserve-dirty/* @@ -1403,6 +1441,15 @@ def _dev_phase(self, task: StoryTask, resume_result: SessionResult | None = None result = resume_result resume_result = None else: + # intent-gap patch-restore (#2564): re-lay the saved attempt onto + # the baseline before dispatch so the re-driven session resumes + # review on the restored diff. `feedback is None` ⇒ the tree is at + # baseline (fresh attempt or a non-fixable rollback below), NOT a + # fixable-feedback retry that kept the attempt's tree — so this + # never double-applies. No-op unless a restore is latched; escalates + # (never dispatches) if the patch fails to apply. + if feedback is None: + self._restore_patch(task) result = self._run_session( task, role="dev", @@ -1697,8 +1744,11 @@ def _commit(self, task: StoryTask) -> None: sha = verify.finalize_commit(self.workspace.root, task.baseline_commit, message) task.commit_sha = sha or task.baseline_commit # the corrected spec is now durable in HEAD; later attempts need no - # special preservation, so drop the re-drive latch. + # special preservation, so drop the re-drive latch. The restored diff + # is likewise committed, so clear its latch too — a subsequent re-arm + # (if any) decides afresh whether to restore again. task.resolved_redrive = False + task.restore_patch = None except verify.GitError as e: self._escalate(task, f"commit failed: {e}") advance(task, Phase.DONE) diff --git a/src/bmad_loop/install.py b/src/bmad_loop/install.py index ade4320..5c64ea4 100644 --- a/src/bmad_loop/install.py +++ b/src/bmad_loop/install.py @@ -67,14 +67,20 @@ # BMad Method (bmm) module installs them. Each must exist in every active CLI skill # tree and carry its marker files (a half-installed or pre-automation skill is # caught by the `bmad-loop validate` preflight). `{skill: (marker-rel-path, ...)}`. -# - bmad-dev-auto: the inner dev primitive — always required. -# - the two review hunters bmad-dev-auto's step-04 invokes inline on EVERY dev +# - bmad-dev-auto: the inner dev primitive — always required. Markers pin BOTH a +# step file (catches a truncated copy) AND customize.toml, the layer/handoff +# config step-04 resolves review_layers from (BMAD-METHOD #2535/#2550): a +# pre-July bmm install predating it would let every dev run's step-04 fail. +# - the three review hunters bmad-dev-auto's step-04 invokes inline on EVERY dev # run (and on each follow-up review re-invocation) — also always required, no -# longer gated on a separate review session. +# longer gated on a separate review session. bmad-review-verification-gap is +# the newest layer (BMAD-METHOD #2550): a target project missing it makes the +# verification-gap review layer fail on every run. DEV_BASE_SKILLS = { - "bmad-dev-auto": ("step-04-review.md",), + "bmad-dev-auto": ("step-04-review.md", "customize.toml"), "bmad-review-adversarial-general": (), "bmad-review-edge-case-hunter": (), + "bmad-review-verification-gap": (), } # Every non-bundled skill that might need copying into an isolated worktree. BASE_SKILLS = dict(DEV_BASE_SKILLS) @@ -125,12 +131,13 @@ def missing_stories_support(project: Path, trees: Sequence[str]) -> list[str]: def missing_base_skills(project: Path, trees: Sequence[str]) -> list[str]: """Problems for the upstream skills the orchestrator drives but doesn't bundle. - The dev primitive (bmad-dev-auto) and the two adversarial review hunters it - invokes inline are installed by the BMad Method module, not by `bmad-loop - init`. Each must exist in every active CLI skill tree and carry its marker - files. Returns one human-readable problem string per missing/incomplete skill; - empty list means OK. Run as a preflight so a missing skill fails loudly with - remediation instead of stalling as an `Unknown command` until the run times out. + The dev primitive (bmad-dev-auto) and the three review hunters it invokes + inline — adversarial-general, edge-case-hunter, and verification-gap — are + installed by the BMad Method module, not by `bmad-loop init`. Each must exist + in every active CLI skill tree and carry its marker files. Returns one + human-readable problem string per missing/incomplete skill; empty list means + OK. Run as a preflight so a missing skill fails loudly with remediation instead + of stalling as an `Unknown command` until the run times out. """ required = dict(DEV_BASE_SKILLS) problems: list[str] = [] diff --git a/src/bmad_loop/model.py b/src/bmad_loop/model.py index 27a4a14..c555de4 100644 --- a/src/bmad_loop/model.py +++ b/src/bmad_loop/model.py @@ -181,6 +181,14 @@ class StoryTask: # non-sentinel escalation whose spec matches the convention, is status-flipped and # kept, never deleted. "" = not a sentinel. Survives the round-trip. sentinel_kind: str = "" + # intent-gap patch-restore re-drive (BMAD-METHOD #2564): a repo-relative-or- + # absolute path to the patch file bmad-dev-auto saved of the reverted attempt. + # Latched by runs.rearm_escalation when the human confirms the attempted reading + # was correct; the engine re-applies it onto the baseline after every reset of + # the re-drive so the re-driven session resumes review (step-04) on the restored + # diff, and clears it once the corrected work commits. None = ordinary + # from-scratch re-drive. Survives the resume serialization round-trip. + restore_patch: str | None = None # sweep bundles only: the deferred-work ids this task closes and the # rendered intent file handed to dev sessions dw_ids: list[str] = field(default_factory=list) @@ -235,6 +243,7 @@ def to_dict(self) -> dict[str, Any]: "plan_checkpoint_pending": self.plan_checkpoint_pending, "plan_review_owed": self.plan_review_owed, "sentinel_kind": self.sentinel_kind, + "restore_patch": self.restore_patch, "dw_ids": self.dw_ids, "bundle_file": self.bundle_file, "worktree_path": self.worktree_path, @@ -280,6 +289,7 @@ def from_dict(cls, d: dict[str, Any]) -> "StoryTask": plan_checkpoint_pending=bool(d.get("plan_checkpoint_pending", False)), plan_review_owed=bool(d.get("plan_review_owed", False)), sentinel_kind=str(d.get("sentinel_kind", "")), + restore_patch=d.get("restore_patch"), dw_ids=[str(i) for i in d.get("dw_ids", [])], bundle_file=d.get("bundle_file"), worktree_path=str(d.get("worktree_path", "")), diff --git a/src/bmad_loop/resolve.py b/src/bmad_loop/resolve.py index 4543711..aa5b437 100644 --- a/src/bmad_loop/resolve.py +++ b/src/bmad_loop/resolve.py @@ -38,6 +38,21 @@ def resolution_path(run_dir: Path, story_key: str) -> Path: return _story_dir(run_dir, story_key) / "resolution.json" +def read_resolution(run_dir: Path, story_key: str) -> dict[str, Any] | None: + """Parse the resolve agent's ``resolution.json`` marker, or None when it is + absent or unreadable. The caller reads the optional ``restore_patch`` field + (the intent-gap patch-restore path, BMAD-METHOD #2564) from the returned dict; + it validates that path itself before acting on it.""" + path = resolution_path(run_dir, story_key) + if not path.is_file(): + return None + try: + doc = json.loads(path.read_text(encoding="utf-8")) + except (OSError, json.JSONDecodeError): + return None + return doc if isinstance(doc, dict) else None + + def _gather_escalations(run_dir: Path, state: RunState, story_key: str) -> list[dict[str, Any]]: """The CRITICAL escalations recorded by this story's sessions, newest first. diff --git a/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index 790300c..833ef3c 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -505,7 +505,9 @@ class RearmError(Exception): """The run/story is not in a re-armable escalation state.""" -def rearm_escalation(run_dir: Path, story_key: str | None = None) -> str: +def rearm_escalation( + run_dir: Path, story_key: str | None = None, *, restore_patch: str | None = None +) -> str: """Re-arm an escalation-paused story so the next resume re-drives it. Flips the escalated task out of its terminal ESCALATED phase back to @@ -514,11 +516,27 @@ def rearm_escalation(run_dir: Path, story_key: str | None = None) -> str: spec. The baseline itself is advanced to the project's current HEAD (and the untracked snapshot refreshed) so commits and files the resolve session produced count as the rebuild's starting point, not as attempt debris to - roll back. Deterministically sets that spec's status to `ready-for-dev` so - the dev session routes straight to implement, and strips the escalated - attempt's stale `## Auto Run Result` section so the re-drive cannot read - as terminal from its first save. Does NOT clear the pause; the caller - resumes the run separately. + roll back. Strips the escalated attempt's stale `## Auto Run Result` + section so the re-drive cannot read as terminal from its first save, and + sets the spec's frontmatter status so step-01 routes to the right stage. + Does NOT clear the pause; the caller resumes the run separately. + + Two re-drive modes, selected by `restore_patch`: + + - **from-scratch** (default, ``restore_patch=None``): status → ``ready-for-dev`` + so the dev session re-implements from a clean baseline. Assigning None also + clears any stale latch from a prior restore attempt the human abandoned. + - **patch-restore** (BMAD-METHOD #2564, ``restore_patch`` set): the human + confirmed the escalated attempt's reading was correct. Status → ``in-review`` + so step-01 routes straight to step-04, and the path is latched onto the task + (`task.restore_patch`) so the engine re-applies the saved patch onto the + baseline before dispatching — the re-driven session resumes review on the + restored diff instead of re-implementing. The status is set here + deterministically; the resolve agent must NOT set it. Because the baseline + advances (above) while the patch was diffed from the OLD baseline, a resolve + session that committed changes to the patched files makes the re-drive's + apply fail — the engine then escalates loudly instead of dispatching on a + half-restored tree (see verify.apply_patch). Stories mode: when the escalated spec is a fixed-slug sentinel (`-unresolved.md` / `-ambiguous.md`, written by a pre-planning HALT), @@ -526,6 +544,10 @@ def rearm_escalation(run_dir: Path, story_key: str | None = None) -> str: Instead preserve a copy under `{run_dir}/sentinels/`, journal `sentinel-cleared` with the blocking condition, and delete it, so the re-dispatch resolves to a clean PENDING and re-plans from scratch (leg 1 again for a spec_checkpoint id). + A patch-restore re-arm is rejected for a sentinel-wedged story: a sentinel is a + pre-planning halt, so there is no attempted implementation to restore and the + re-dispatch is a planning leg — laying implementation onto the tree before a + planning session is never safe. Returns the re-armed story key. Raises RearmError when the run is not paused at the escalation stage or the target story is not escalated. @@ -544,6 +566,18 @@ def rearm_escalation(run_dir: Path, story_key: str | None = None) -> str: raise RearmError(f"run {run_dir.name} has no task for story {key}") if task.phase != Phase.ESCALATED: raise RearmError(f"story {key} is not escalated (phase: {task.phase})") + # T1 guard (stories × patch-restore): a sentinel-wedged story escalated BEFORE + # planning — there is no attempted implementation to restore, and its re-arm + # re-dispatches a planning leg. Keyed on the recorded detection verdict + # (task.sentinel_kind), not the on-disk basename, mirroring the sentinel-clear + # branch below. Rejected here, before any task mutation, so the escalation + # stays armed for a corrected resolve. + if restore_patch and state.source == "stories" and task.sentinel_kind: + raise RearmError( + f"story {key} is wedged on a pre-planning {task.sentinel_kind} sentinel — " + "there is no attempted implementation to restore, and the re-drive starts " + "at planning. Re-run resolve without a restore patch for a clean re-plan." + ) journal = Journal(run_dir) # deliberate reset, not a normal state-machine transition (mirrors @@ -554,6 +588,9 @@ def rearm_escalation(run_dir: Path, story_key: str | None = None) -> str: task.defer_reason = None task.rearmed = True # resume-time recovery notice describes a clean rebuild, # not a failed attempt (engine._finish_inflight clears it once the rebuild runs) + # Always (re)assign the latch: a None restore_patch clears a stale one left by + # a prior restore attempt the human then chose to redo from scratch. + task.restore_patch = restore_patch if task.spec_file: spec_path = Path(task.spec_file) @@ -576,9 +613,12 @@ def rearm_escalation(run_dir: Path, story_key: str | None = None) -> str: task.sentinel_kind = "" # verdict discharged; the re-dispatch is clean else: try: - # route /bmad-dev-auto to re-implement (decision table: ready-for-dev - # -> step-03); independent of the resolve agent having set it. - verify.set_frontmatter_status(spec_path, "ready-for-dev") + # Route /bmad-dev-auto via the spec's frontmatter status (decision + # table): patch-restore -> in-review -> step-04 (resume review on + # the restored diff); from-scratch -> ready-for-dev -> step-03 + # (re-implement). Independent of the resolve agent having set it. + target_status = "in-review" if restore_patch else "ready-for-dev" + verify.set_frontmatter_status(spec_path, target_status) # drop the stale `## Auto Run Result` section along with the status flip # (mirrors engine._reset_spec_for_repair): find_result_artifact keys on # that heading, so leaving it would let the re-driven session's first @@ -622,7 +662,12 @@ def rearm_escalation(run_dir: Path, story_key: str | None = None) -> str: pass save_state(run_dir, state) - journal.append("story-escalation-resolved", story_key=key, baseline=task.baseline_commit or "") + journal.append( + "story-escalation-resolved", + story_key=key, + baseline=task.baseline_commit or "", + restore=bool(restore_patch), + ) return key diff --git a/src/bmad_loop/verify.py b/src/bmad_loop/verify.py index da0e70d..c997a7d 100644 --- a/src/bmad_loop/verify.py +++ b/src/bmad_loop/verify.py @@ -1398,6 +1398,15 @@ def finalize_commit(repo: Path, baseline: str | None, message: str) -> str | Non keeping the index (`reset --soft`), then commit the accumulated index. The working tree is never touched, so a failure leaves the chain intact. + Residual-artifacts note (BMAD-METHOD #2563): the skill now commits every file + of the reviewed diff and deliberately leaves unrelated `git status` residue + uncommitted (files outside the change's scope). The `add -A` here sweeps that + residue into the story commit too — an intentional divergence from the skill's + scoped commit. The loop must end each story on a clean tree because story + N+1's step-01 HALTs on a dirty tree, so the orchestrator squashes EVERYTHING + since baseline (skill commits + its own bookkeeping + any residue) into the one + story commit rather than leaving the tree dirty for the next story to trip on. + Returns the new HEAD sha, or None when there is nothing to finalize: no version control (`baseline` falsy or NO_VCS) or the tree already equals `baseline` (no skill commits and no bookkeeping delta).""" @@ -1429,6 +1438,25 @@ def finalize_commit(repo: Path, baseline: str | None, message: str) -> str | Non return rev_parse_head(repo) +def apply_patch(repo: Path, patch_path: Path) -> None: + """Apply a saved patch to `repo`'s working tree (`git apply`), raising on failure. + + The intent-gap patch-restore re-drive (BMAD-METHOD #2564) uses this to re-lay + the attempted change bmad-dev-auto saved before reverting: the patch is a diff + from the story's baseline, and the caller has just reset the tree to that same + baseline, so it applies cleanly by construction. New files in the patch are + created (they land untracked, matching how the original attempt sat before its + revert). A non-zero `git apply` — a conflict, a missing/corrupt patch, a tree + that has drifted from baseline — raises `GitError` with git's output; the caller + escalates rather than dispatch a session onto a half-applied tree. + """ + if not patch_path.is_file(): + raise GitError(f"restore patch not found: {patch_path}") + rc, out = _git(repo, "apply", str(patch_path)) + if rc != 0: + raise GitError(f"git apply {patch_path} failed: {out}") + + def commit_paths(repo: Path, message: str, paths: list[Path]) -> str | None: """Commit exactly `paths` (and nothing else), leaving any unrelated working or staged changes untouched. Unlike commit_story's `add -A`, this is safe to diff --git a/tests/test_cli.py b/tests/test_cli.py index ea69b4a..83fa07d 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -971,6 +971,94 @@ def test_resolve_rearm_only_skips_resume(tmp_path, monkeypatch, capsys): assert "resume when ready" in capsys.readouterr().out +def test_resolve_no_interactive_restore_patch_latches_in_review(tmp_path, monkeypatch, capsys): + from bmad_loop.journal import load_state + from bmad_loop.model import Phase + + spec = tmp_path / "spec.md" + spec.write_text("---\nstatus: blocked\n---\n", encoding="utf-8") + patch = tmp_path / "artifacts" / "attempt.patch" + patch.parent.mkdir(parents=True) + patch.write_text("diff", encoding="utf-8") + run_dir = _escalated_run(tmp_path, "r1", spec_file=str(spec)) + + monkeypatch.setattr(cli, "_resume_paused_run", lambda proj, rd: 0) + rc = cli.main( + [ + "resolve", + "--project", + str(tmp_path), + "r1", + "--no-interactive", + "--restore-patch", + str(patch), + "--resume", + ] + ) + assert rc == 0 + assert "restoring the attempted change" in capsys.readouterr().out + task = load_state(run_dir).tasks["s1"] + assert task.phase == Phase.PENDING + assert task.restore_patch == str(patch.resolve()) # validated + latched (absolute) + assert "in-review" in spec.read_text() # restore mode routes step-01 -> step-04 + + +def test_resolve_restore_patch_missing_file_rejected(tmp_path, monkeypatch, capsys): + from bmad_loop.journal import load_state + from bmad_loop.model import Phase + + spec = tmp_path / "spec.md" + spec.write_text("---\nstatus: blocked\n---\n", encoding="utf-8") + run_dir = _escalated_run(tmp_path, "r1", spec_file=str(spec)) + called: list = [] + monkeypatch.setattr(cli, "_resume_paused_run", lambda proj, rd: called.append(rd) or 0) + rc = cli.main( + [ + "resolve", + "--project", + str(tmp_path), + "r1", + "--no-interactive", + "--restore-patch", + str(tmp_path / "nope.patch"), + "--resume", + ] + ) + assert rc == 1 + assert "not a file under the project" in capsys.readouterr().err + assert called == [] # never resumed + assert load_state(run_dir).tasks["s1"].phase == Phase.ESCALATED # not re-armed + + +def test_resolve_interactive_restore_patch_from_resolution_json(tmp_path, monkeypatch): + from bmad_loop import resolve + from bmad_loop.journal import load_state + + spec = tmp_path / "spec.md" + spec.write_text("---\nstatus: blocked\n---\n", encoding="utf-8") + patch = tmp_path / "artifacts" / "attempt.patch" + patch.parent.mkdir(parents=True) + patch.write_text("diff", encoding="utf-8") + run_dir = _escalated_run(tmp_path, "r1", spec_file=str(spec)) + + def fake_session(adapter, project, rd, story_key, *, model=""): + # the resolve agent records a restore_patch in its output marker + marker = resolve.resolution_path(rd, story_key) + marker.parent.mkdir(parents=True, exist_ok=True) + marker.write_text(json.dumps({"restore_patch": str(patch)}), encoding="utf-8") + return True + + monkeypatch.setattr(cli, "_make_adapters", lambda *a, **k: {"dev": object()}) + monkeypatch.setattr(resolve, "build_context", lambda *a, **k: None) + monkeypatch.setattr(resolve, "run_session", fake_session) + monkeypatch.setattr(cli, "_resume_paused_run", lambda proj, rd: 0) + rc = cli.main(["resolve", "--project", str(tmp_path), "r1", "--resume"]) + assert rc == 0 + task = load_state(run_dir).tasks["s1"] + assert task.restore_patch == str(patch.resolve()) # picked up from resolution.json + assert "in-review" in spec.read_text() + + def test_sweep_command_parses_flags(): parser_args = [ "sweep", diff --git a/tests/test_engine.py b/tests/test_engine.py index b7ab620..c2caa3a 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -2067,6 +2067,164 @@ def halt_blocked(spec): assert read_frontmatter(sp)["status"] == "ready-for-dev" # re-drive will not HALT +# -------------------------------------------------- intent-gap patch-restore (#2564) + + +def _escalate_with_patch(project, story_key, patch_path): + """A dev effect that halts on an intent gap the way bmad-dev-auto #2564 does: + it saves the attempted change as a patch under the protected artifacts, reverts + the tree, and escalates blocked.""" + + def effect(spec): + repo = project.project + baseline = rev_parse_head(repo) + src = repo / "src.txt" + src.write_text("original\nattempted reading\n") # the attempted change + patch_path.parent.mkdir(parents=True, exist_ok=True) + patch_path.write_text(git(repo, "diff", "HEAD") + "\n", encoding="utf-8") # save it + src.write_text("original\n") # revert before halting (tree back at baseline) + sp = spec_path(project, story_key) + write_spec(sp, "blocked", baseline) + return SessionResult( + status="completed", + result_json={ + "workflow": "auto-dev", + "story_key": story_key, + "spec_file": str(sp), + "escalations": [ + { + "type": "blocked", + "severity": "CRITICAL", + "detail": f"intent gap; saved patch: {patch_path}", + } + ], + }, + ) + + return effect + + +def _restoring_dev_effect(project, story_key, seen): + """A re-driven dev effect that records what the tree looked like when it ran + (so a test can assert the restored diff was present) then finalizes to done.""" + + def effect(spec): + repo = project.project + seen.append((repo / "src.txt").read_text()) + baseline = rev_parse_head(repo) + sp = spec_path(project, story_key) + write_spec(sp, "done", baseline) + return SessionResult( + status="completed", + result_json={ + "workflow": "auto-dev", + "story_key": story_key, + "spec_file": str(sp), + "baseline_commit": baseline, + "escalations": [], + "followup_review_recommended": False, + }, + ) + + return effect + + +def test_restore_patch_applies_onto_baseline(project): + """_restore_patch re-lays the saved diff onto the baseline tree and journals + attempt-restored, leaving the phase untouched on success.""" + engine, _ = make_engine(project, []) + repo = project.project + task = StoryTask(story_key="1-1-a", epic=1) + task.baseline_commit = rev_parse_head(repo) + src = repo / "src.txt" + src.write_text("original\nattempted\n") + patch = project.implementation_artifacts / "attempt.patch" + patch.write_text(git(repo, "diff", "HEAD") + "\n", encoding="utf-8") + src.write_text("original\n") # tree at baseline + task.restore_patch = str(patch) + task.phase = Phase.DEV_RUNNING + + engine._restore_patch(task) + + assert src.read_text() == "original\nattempted\n" + assert task.phase == Phase.DEV_RUNNING # success does not advance the phase + assert "attempt-restored" in [e["kind"] for e in engine.journal.entries()] + + +def test_restore_patch_failure_escalates_without_dispatch(project): + """A patch that will not apply escalates (never dispatches onto a half-restored + tree): the task ends ESCALATED, attempt-restore-failed is journaled, and the + success marker is not.""" + engine, _ = make_engine(project, []) + task = StoryTask(story_key="1-1-a", epic=1) + task.baseline_commit = rev_parse_head(project.project) + patch = project.implementation_artifacts / "bad.patch" + patch.parent.mkdir(parents=True, exist_ok=True) + patch.write_text("--- a/src.txt\n+++ b/src.txt\n@@ -1 +1 @@\n-nope\n+x\n", encoding="utf-8") + task.restore_patch = str(patch) + task.phase = Phase.DEV_RUNNING + + with pytest.raises(RunPaused): + engine._restore_patch(task) + + assert task.phase == Phase.ESCALATED + kinds = [e["kind"] for e in engine.journal.entries()] + assert "attempt-restore-failed" in kinds + assert "attempt-restored" not in kinds + + +def test_intent_gap_restore_redrive_applies_patch_and_lands_done(project): + """End-to-end: a resolved escalation with a restore patch re-applies the + attempted change onto the baseline before the re-driven session runs, so the + session resumes on the restored diff and the story lands done; the latch clears + on commit.""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + patch = project.implementation_artifacts / "attempt.patch" + engine, _ = make_engine(project, [_escalate_with_patch(project, "1-1-a", patch)]) + assert engine.run().escalated == 1 + + rearm_escalation(engine.run_dir, restore_patch=str(patch)) # human confirmed the reading + sp = spec_path(project, "1-1-a") + assert read_frontmatter(sp)["status"] == "in-review" # routes step-01 -> step-04 + assert load_state(engine.run_dir).tasks["1-1-a"].restore_patch == str(patch) + + seen: list[str] = [] + resumed, _ = resume_engine(project, engine, [_restoring_dev_effect(project, "1-1-a", seen)]) + summary = resumed.run() + + assert summary.done == 1 and not summary.paused + assert seen == ["original\nattempted reading\n"] # the session saw the RESTORED code + assert "attempt-restored" in [e["kind"] for e in resumed.journal.entries()] + task = load_state(resumed.run_dir).tasks["1-1-a"] + assert task.phase == Phase.DONE + assert task.restore_patch is None # latch cleared on commit + + +def test_intent_gap_restore_reapplies_after_mid_redrive_rollback(project): + """A non-fixable retry inside the restore re-drive resets to baseline (clearing + the restored code), so the patch is re-applied before the next dispatch; the + saved patch file under the protected artifacts survives the reset.""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + patch = project.implementation_artifacts / "attempt.patch" + engine, _ = make_engine(project, [_escalate_with_patch(project, "1-1-a", patch)]) + assert engine.run().escalated == 1 + rearm_escalation(engine.run_dir, restore_patch=str(patch)) + + seen: list[str] = [] + resumed, _ = resume_engine( + project, + engine, + [SessionResult(status="stalled"), _restoring_dev_effect(project, "1-1-a", seen)], + ) + summary = resumed.run() + + assert summary.done == 1 and not summary.paused + assert seen == ["original\nattempted reading\n"] # the surviving retry ran on the restored tree + kinds = [e["kind"] for e in resumed.journal.entries()] + assert kinds.count("attempt-restored") == 2 # applied, rolled back on stall, re-applied + assert patch.is_file() # protected patch file survived the reset + + def test_dev_stall_retries_then_succeeds(project): write_sprint(project, {"1-1-a": "ready-for-dev"}) engine, adapter = make_engine( diff --git a/tests/test_install.py b/tests/test_install.py index 9b38ebe..3bea913 100644 --- a/tests/test_install.py +++ b/tests/test_install.py @@ -534,24 +534,43 @@ def test_provision_worktree_copies_base_skills_from_repo(tmp_path): def test_missing_base_skills_reports_absent_and_incomplete(tmp_path): claude = get_profile("claude") - # nothing installed → dev primitive + both inline review hunters reported + # nothing installed → dev primitive + all three inline review hunters reported # missing (the hunters are always required — bmad-dev-auto's step-04 invokes # them on every run, regardless of the orchestrator's follow-up review) problems = missing_base_skills(tmp_path, [claude.skill_tree]) - assert len(problems) == 3 + assert len(problems) == 4 assert all("install the BMad Method" in p for p in problems) # install everything → no problems _install_base_skills(tmp_path, claude.skill_tree) assert missing_base_skills(tmp_path, [claude.skill_tree]) == [] - # remove the dev primitive's marker → reported as incomplete + # remove the dev primitive's step-file marker → reported as incomplete (tmp_path / claude.skill_tree / "bmad-dev-auto" / "step-04-review.md").unlink() problems = missing_base_skills(tmp_path, [claude.skill_tree]) assert len(problems) == 1 assert "incomplete" in problems[0] assert "step-04-review.md" in problems[0] + # restore it, then drop customize.toml (the review-layer config marker, + # BMAD-METHOD #2535/#2550) → a pre-July bmm install is caught as incomplete + (tmp_path / claude.skill_tree / "bmad-dev-auto" / "step-04-review.md").write_text("x\n") + (tmp_path / claude.skill_tree / "bmad-dev-auto" / "customize.toml").unlink() + problems = missing_base_skills(tmp_path, [claude.skill_tree]) + assert len(problems) == 1 + assert "incomplete" in problems[0] + assert "customize.toml" in problems[0] + + # the newest review layer (verification-gap) reported by name when absent + _install_base_skills(tmp_path, claude.skill_tree) # re-complete everything + import shutil as _shutil + + _shutil.rmtree(tmp_path / claude.skill_tree / "bmad-review-verification-gap") + problems = missing_base_skills(tmp_path, [claude.skill_tree]) + assert len(problems) == 1 + assert "bmad-review-verification-gap" in problems[0] + assert "install the BMad Method" in problems[0] + def test_missing_stories_support_probes_step01_content(tmp_path): from bmad_loop.install import ( diff --git a/tests/test_model.py b/tests/test_model.py index 2df7969..8e57830 100644 --- a/tests/test_model.py +++ b/tests/test_model.py @@ -125,6 +125,17 @@ def test_sentinel_kind_defaults_empty_for_legacy_state(): assert StoryTask.from_dict(doc).sentinel_kind == "" +def test_restore_patch_round_trips(): + task = StoryTask(story_key="1-1-a", epic=1, restore_patch="artifacts/attempt.patch") + assert StoryTask.from_dict(task.to_dict()).restore_patch == "artifacts/attempt.patch" + + +def test_restore_patch_defaults_none_for_legacy_state(): + doc = StoryTask(story_key="1-1-a", epic=1).to_dict() + del doc["restore_patch"] # state.json from before the field existed + assert StoryTask.from_dict(doc).restore_patch 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_runs.py b/tests/test_runs.py index 6d8db53..8ce2351 100644 --- a/tests/test_runs.py +++ b/tests/test_runs.py @@ -530,6 +530,69 @@ def test_delete_run(tmp_path): assert not run_dir.exists() +def _escalated_run(tmp_path, spec_text, *, restore_patch_stale=None): + from bmad_loop.model import PAUSE_ESCALATION, Phase, StoryTask + + spec = tmp_path / "spec.md" + spec.write_text(spec_text, encoding="utf-8") + task = StoryTask( + story_key="1-1-a", + epic=1, + phase=Phase.ESCALATED, + attempt=2, + spec_file=str(spec), + restore_patch=restore_patch_stale, + ) + run_dir = _make_state_run( + tmp_path, + "r1", + paused_reason="CRITICAL escalation", + paused_stage=PAUSE_ESCALATION, + paused_story_key="1-1-a", + tasks={"1-1-a": task}, + ) + return run_dir, spec + + +_SPEC_WITH_ARR = ( + "---\ntitle: t\nstatus: blocked\n---\n\n## Intent\n\nbody\n" + "\n## Auto Run Result\n\n- Status: blocked\n\nboom\n" +) + + +def test_rearm_restore_mode_sets_in_review_strips_arr_and_latches(tmp_path): + from bmad_loop.journal import Journal + from bmad_loop.model import Phase + + run_dir, spec = _escalated_run(tmp_path, _SPEC_WITH_ARR) + runs.rearm_escalation(run_dir, restore_patch="artifacts/attempt.patch") + + task = load_state(run_dir).tasks["1-1-a"] + assert task.phase == Phase.PENDING and task.attempt == 0 + assert task.restore_patch == "artifacts/attempt.patch" + text = spec.read_text() + assert "status: in-review" in text # in-review routes step-01 -> step-04 + assert "## Auto Run Result" not in text # stale terminal section stripped + entry = [e for e in Journal(run_dir).entries() if e["kind"] == "story-escalation-resolved"][-1] + assert entry["restore"] is True + + +def test_rearm_plain_mode_sets_ready_for_dev_and_clears_stale_latch(tmp_path): + from bmad_loop.journal import Journal + from bmad_loop.model import Phase + + # a stale latch from a prior restore attempt the human then chose to redo fresh + run_dir, spec = _escalated_run(tmp_path, _SPEC_WITH_ARR, restore_patch_stale="old.patch") + runs.rearm_escalation(run_dir) # no restore_patch => from-scratch + + task = load_state(run_dir).tasks["1-1-a"] + assert task.phase == Phase.PENDING + assert task.restore_patch is None # stale latch cleared + assert "status: ready-for-dev" in spec.read_text() + entry = [e for e in Journal(run_dir).entries() if e["kind"] == "story-escalation-resolved"][-1] + assert entry["restore"] is False + + def test_archive_run(tmp_path): run_dir = _make_state_run(tmp_path, "20260611-100000-aaaa") (run_dir / "journal.jsonl").write_text('{"kind":"x"}\n') diff --git a/tests/test_verify.py b/tests/test_verify.py index 8665ee2..a3c37bc 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -988,6 +988,49 @@ def test_commit_paths_noop_when_unchanged(project): assert verify.commit_paths(project.project, "noop", [project.project.parent / "x"]) is None +def test_apply_patch_replays_saved_diff(project): + """A patch saved off the baseline re-applies cleanly onto that same baseline — + tracked edits AND new (untracked) files — reproducing the reverted attempt.""" + repo = project.project + baseline = verify.rev_parse_head(repo) + # the "attempt": edit a tracked file + add a new file, then capture the diff + (repo / "src.txt").write_text("original\nattempted change\n") + (repo / "new_module.py").write_text("print('hi')\n") + patch = project.implementation_artifacts / "attempt.patch" + # git diff HEAD includes new files with --binary-safe text; -N stages intent so + # untracked files appear in the diff (mirrors how the skill saves the attempt) + git(repo, "add", "-N", "new_module.py") + patch.write_text(git(repo, "diff", "HEAD") + "\n", encoding="utf-8") + # revert the attempt back to baseline (as the skill does before halting) + git(repo, "reset", "-q", "--hard", baseline) + (repo / "new_module.py").unlink(missing_ok=True) + assert (repo / "src.txt").read_text() == "original\n" + + verify.apply_patch(repo, patch) + + assert (repo / "src.txt").read_text() == "original\nattempted change\n" + assert (repo / "new_module.py").read_text() == "print('hi')\n" + + +def test_apply_patch_missing_file_raises(project): + with pytest.raises(verify.GitError, match="restore patch not found"): + verify.apply_patch(project.project, project.implementation_artifacts / "nope.patch") + + +def test_apply_patch_conflict_raises(project): + """A patch that does not apply against the current tree raises GitError with + git's output — the caller escalates rather than dispatch onto a broken tree.""" + repo = project.project + patch = project.implementation_artifacts / "bad.patch" + # a diff against content the tree does not have + patch.write_text( + "--- a/src.txt\n+++ b/src.txt\n@@ -1 +1 @@\n-something-else\n+patched\n", + encoding="utf-8", + ) + with pytest.raises(verify.GitError, match="git apply"): + verify.apply_patch(repo, patch) + + def test_read_frontmatter_tolerates_garbage(project): p = project.project / "x.md" p.write_text("no frontmatter here") From 6129b07019bfe69efd70868c2b18c859c2a0df5b Mon Sep 17 00:00:00 2001 From: pbean Date: Tue, 7 Jul 2026 23:06:27 -0700 Subject: [PATCH 02/19] test(resolve): a sentinel-wedged story rejects a patch-restore re-arm (T1) The guard itself landed inside the re-cut's runs.py merge resolution. Covers both sides: rejection leaves the sentinel on disk, the task ESCALATED, no latch persisted and nothing journaled; a real stories spec (sentinel_kind unset) stays a legitimate restore target. --- tests/test_resolve.py | 48 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/tests/test_resolve.py b/tests/test_resolve.py index e70dbad..c72517e 100644 --- a/tests/test_resolve.py +++ b/tests/test_resolve.py @@ -337,6 +337,54 @@ def test_rearm_sprint_spec_named_like_a_sentinel_is_not_deleted(tmp_path): assert not (run_dir / "sentinels").exists() # no sentinel preservation in sprint mode +def test_rearm_rejects_restore_patch_on_a_sentinel(tmp_path): + """T1 (stories × patch-restore): a sentinel-wedged story escalated BEFORE + planning — there is no attempted implementation to restore, and its re-arm + re-dispatches a planning leg, so laying a patch onto the tree first is never + safe. Re-arm must reject loudly BEFORE mutating anything: the sentinel stays + on disk, the task stays ESCALATED, and no latch is persisted.""" + key = "6-4-cli-list-command" + stories_dir = tmp_path / "stories" + stories_dir.mkdir(parents=True) + sentinel = stories_dir / f"{key}-unresolved.md" + sentinel.write_text( + "---\nstatus: blocked\n---\n\n## Auto Run Result\n\nStatus: blocked\nintent too vague\n", + encoding="utf-8", + ) + run_dir, _, _ = _escalated_run( + tmp_path, spec_file=str(sentinel), source="stories", sentinel_kind="unresolved" + ) + + with pytest.raises(runs.RearmError, match="sentinel"): + runs.rearm_escalation(run_dir, restore_patch="artifacts/attempt.patch") + + assert sentinel.is_file() # nothing deleted, copy NOT preserved — no clear happened + task = load_state(run_dir).tasks[key] + assert task.phase == Phase.ESCALATED # not re-armed; the escalation stays armed + assert task.restore_patch is None # no latch persisted + assert task.sentinel_kind == "unresolved" # detection verdict intact for a retry + # nothing was journaled at all — no sentinel-cleared, no story-escalation-resolved + assert not (run_dir / "journal.jsonl").exists() + + +def test_rearm_restore_patch_on_a_real_stories_spec_is_allowed(tmp_path): + """The T1 guard keys on the recorded sentinel verdict, not on stories mode: + a review-stage intent gap on a REAL stories spec (sentinel_kind unset) is a + legitimate restore target and re-arms to in-review like sprint mode.""" + key = "6-4-cli-list-command" + stories_dir = tmp_path / "stories" + stories_dir.mkdir(parents=True) + spec = stories_dir / f"{key}-slug.md" + spec.write_text("---\nstatus: blocked\n---\n\n## Intent\n\nx\n", encoding="utf-8") + run_dir, _, _ = _escalated_run(tmp_path, spec_file=str(spec), source="stories") + + runs.rearm_escalation(run_dir, restore_patch="artifacts/attempt.patch") + task = load_state(run_dir).tasks[key] + assert task.phase == Phase.PENDING + assert task.restore_patch == "artifacts/attempt.patch" + assert verify.read_frontmatter(spec)["status"] == "in-review" # restore routing + + # -------------------------------------------------- non-UTF-8 robustness (bug class) # A story spec / sentinel is agent- or human-authored, so it can contain non-UTF-8 # bytes. `read_text(encoding="utf-8")` raises UnicodeDecodeError (a ValueError, NOT an From af3cdb56b985dbeb25e61640ac54d576075e59fd Mon Sep 17 00:00:00 2001 From: pbean Date: Tue, 7 Jul 2026 23:06:27 -0700 Subject: [PATCH 03/19] fix(cli): reject --restore-patch for worktree-isolation runs (B4d) Restore is an in-place-only recovery: a worktree re-drive discards and re-mounts the unit's worktree (engine._finish_inflight), so a latched patch would silently never restore. Reject before any re-arm, mirroring the missing/outside-project rejections; also add the audit's missing outside-project-but-existing-file rejection test. --- src/bmad_loop/cli.py | 20 +++++++++++-- tests/test_cli.py | 71 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 3 deletions(-) diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index 6fcb2e8..c1d590d 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -814,9 +814,10 @@ def _resolve_restore_patch( otherwise, on the interactive path, the resolve agent may have recorded a ``restore_patch`` field in resolution.json. Returns ``(latch, error)``: a validated absolute patch path to latch (None = ordinary from-scratch re-drive), - or an error string when a supplied path is missing / outside the project — the - caller aborts strictly rather than silently re-driving from scratch when the - operator asked to restore.""" + or an error string when a supplied path is missing / outside the project, or + the run's isolation mode can't restore in place — the caller aborts strictly + rather than silently re-driving from scratch when the operator asked to + restore.""" raw = getattr(args, "restore_patch", None) if raw is None and args.interactive: doc = resolve.read_resolution(run_dir, story_key) @@ -825,6 +826,19 @@ def _resolve_restore_patch( raw = str(val) if val else None if not raw: return None, None + # Restore is an in-place-only recovery: a worktree-isolation re-drive discards + # and re-mounts the unit's worktree (engine._finish_inflight), so a patch laid + # onto the project tree has nowhere durable to land and the engine's apply seam + # is never reached. Reject up front instead of latching a patch that would + # silently never restore. + pol = policy_mod.load(_policy_path(project)) + if pol.scm.isolation == "worktree": + return None, ( + "restore patch is unsupported for worktree-isolation runs (the re-drive " + "discards and re-mounts the unit's worktree, so an in-place restore has " + "nothing durable to land on) — re-run without a restore patch for a " + "plain re-drive" + ) patch = Path(raw) if not patch.is_absolute(): patch = project / patch diff --git a/tests/test_cli.py b/tests/test_cli.py index 83fa07d..2581b00 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1030,6 +1030,77 @@ def test_resolve_restore_patch_missing_file_rejected(tmp_path, monkeypatch, caps assert load_state(run_dir).tasks["s1"].phase == Phase.ESCALATED # not re-armed +def test_resolve_restore_patch_outside_project_rejected(tmp_path, monkeypatch, capsys): + """A patch that EXISTS but sits outside the project root is rejected the same + as a missing one: an absolute path escaping the workspace must never be + latched (the engine would lay whatever it points at onto the tree).""" + from bmad_loop.journal import load_state + from bmad_loop.model import Phase + + project = tmp_path / "proj" + project.mkdir() + outside = tmp_path / "outside.patch" # a real file, wrong side of the fence + outside.write_text("diff", encoding="utf-8") + spec = project / "spec.md" + spec.write_text("---\nstatus: blocked\n---\n", encoding="utf-8") + run_dir = _escalated_run(project, "r1", spec_file=str(spec)) + called: list = [] + monkeypatch.setattr(cli, "_resume_paused_run", lambda proj, rd: called.append(rd) or 0) + rc = cli.main( + [ + "resolve", + "--project", + str(project), + "r1", + "--no-interactive", + "--restore-patch", + str(outside), + "--resume", + ] + ) + assert rc == 1 + assert "not a file under the project" in capsys.readouterr().err + assert called == [] # never resumed + task = load_state(run_dir).tasks["s1"] + assert task.phase == Phase.ESCALATED and task.restore_patch is None # not re-armed + + +def test_resolve_restore_patch_worktree_isolation_rejected(tmp_path, monkeypatch, capsys): + """B4d: restore is an in-place-only recovery — a worktree-isolation re-drive + discards and re-mounts the unit's worktree, so a latched patch would silently + never restore. Rejected up front: no re-arm, no latch, spec untouched.""" + from bmad_loop.journal import load_state + from bmad_loop.model import Phase + + _write_policy(tmp_path, '[scm]\nisolation = "worktree"\n') + spec = tmp_path / "spec.md" + spec.write_text("---\nstatus: blocked\n---\n", encoding="utf-8") + patch = tmp_path / "artifacts" / "attempt.patch" + patch.parent.mkdir(parents=True) + patch.write_text("diff", encoding="utf-8") + run_dir = _escalated_run(tmp_path, "r1", spec_file=str(spec)) + called: list = [] + monkeypatch.setattr(cli, "_resume_paused_run", lambda proj, rd: called.append(rd) or 0) + rc = cli.main( + [ + "resolve", + "--project", + str(tmp_path), + "r1", + "--no-interactive", + "--restore-patch", + str(patch), + "--resume", + ] + ) + assert rc == 1 + assert "worktree" in capsys.readouterr().err + assert called == [] # never resumed + task = load_state(run_dir).tasks["s1"] + assert task.phase == Phase.ESCALATED and task.restore_patch is None # not re-armed + assert "status: blocked" in spec.read_text() # spec status untouched + + def test_resolve_interactive_restore_patch_from_resolution_json(tmp_path, monkeypatch): from bmad_loop import resolve from bmad_loop.journal import load_state From 97a405cf0b65db0c62ffc90eb2247aadea073e04 Mon Sep 17 00:00:00 2001 From: pbean Date: Tue, 7 Jul 2026 23:07:23 -0700 Subject: [PATCH 04/19] docs(verify): reconcile apply_patch's clean-apply claim with the advanced re-arm baseline (T2) Since #78 re-arm advances the baseline to the post-resolve HEAD while the saved patch diffs from the ORIGINAL baseline, 'applies cleanly by construction' held only when the resolve session left the patched files untouched. Docstring + rearm docstring already state the conditional; the resolve SKILL.md now warns that resolution commits overlapping the patch's files make the restore fail loudly (omit restore_patch when the resolution already carries the work). Combined engine test drives the full interaction: resolution commit rewrites the patched line, restore apply fails, the story re-escalates with no session dispatched and the resolution commit untouched. --- .../data/skills/bmad-loop-resolve/SKILL.md | 10 ++++++ src/bmad_loop/verify.py | 22 ++++++++---- tests/test_engine.py | 35 +++++++++++++++++++ 3 files changed, 60 insertions(+), 7 deletions(-) diff --git a/src/bmad_loop/data/skills/bmad-loop-resolve/SKILL.md b/src/bmad_loop/data/skills/bmad-loop-resolve/SKILL.md index ea32d58..2d566cf 100644 --- a/src/bmad_loop/data/skills/bmad-loop-resolve/SKILL.md +++ b/src/bmad_loop/data/skills/bmad-loop-resolve/SKILL.md @@ -138,6 +138,16 @@ implemented. Use it two ways: restored diff. **Do NOT `git apply` the patch yourself and do NOT set the spec status** — the orchestrator does both deterministically at re-arm. + **The restore must not overlap resolution commits.** Re-arm advances the + re-drive's baseline to the branch's post-resolve HEAD, but the saved patch was + diffed from the ORIGINAL baseline — so if this session left commits that touch + the patch's own files, the restore's `git apply` fails and the story + re-escalates (loudly, by design: the orchestrator never silently merges the + resolution with the stale attempt). If the resolution work already includes or + supersedes the attempted change, **omit `restore_patch`** — the commits survive + re-arm as the re-drive's starting point, so a from-scratch re-drive builds + directly on them. + If the attempted reading was wrong (the common case), omit `restore_patch` entirely: the orchestrator re-drives from scratch against the corrected intent. diff --git a/src/bmad_loop/verify.py b/src/bmad_loop/verify.py index c997a7d..e3f445a 100644 --- a/src/bmad_loop/verify.py +++ b/src/bmad_loop/verify.py @@ -1442,13 +1442,21 @@ def apply_patch(repo: Path, patch_path: Path) -> None: """Apply a saved patch to `repo`'s working tree (`git apply`), raising on failure. The intent-gap patch-restore re-drive (BMAD-METHOD #2564) uses this to re-lay - the attempted change bmad-dev-auto saved before reverting: the patch is a diff - from the story's baseline, and the caller has just reset the tree to that same - baseline, so it applies cleanly by construction. New files in the patch are - created (they land untracked, matching how the original attempt sat before its - revert). A non-zero `git apply` — a conflict, a missing/corrupt patch, a tree - that has drifted from baseline — raises `GitError` with git's output; the caller - escalates rather than dispatch a session onto a half-applied tree. + the attempted change bmad-dev-auto saved before reverting. New files in the + patch are created (they land untracked, matching how the original attempt sat + before its revert). + + A clean apply is likely but NOT guaranteed: the patch was diffed from the + story's ORIGINAL baseline, while re-arm advances the re-drive's baseline to the + project's post-resolve HEAD (runs.rearm_escalation) — so the apply holds only + while the resolve session left the patched files untouched. A resolve session + that committed changes to those files makes `git apply` fail, deliberately + loudly: silently merging the human's resolution with the stale attempt could + reproduce the very gap being resolved. A non-zero `git apply` — that overlap, a + missing/corrupt patch, any other drift — raises `GitError` with git's output; + the caller escalates rather than dispatch a session onto a half-applied tree, + and the human re-resolves (typically re-arming without a restore, since the + resolution commits already carry the overlapping work). """ if not patch_path.is_file(): raise GitError(f"restore patch not found: {patch_path}") diff --git a/tests/test_engine.py b/tests/test_engine.py index c2caa3a..fbba0f5 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -2225,6 +2225,41 @@ def test_intent_gap_restore_reapplies_after_mid_redrive_rollback(project): assert patch.is_file() # protected patch file survived the reset +def test_intent_gap_restore_escalates_when_resolution_commits_overlap(project): + """T2 (patch-restore × #78 baseline advance): re-arm adopts the resolve + session's commits as the re-drive's baseline, but the saved patch was diffed + from the ORIGINAL baseline — so a resolution commit that rewrote the patched + lines makes the restore's `git apply` fail. The engine must escalate loudly + with no session dispatched (never silently merge the human's resolution with + the stale attempt), and the resolution commit must survive untouched.""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + patch = project.implementation_artifacts / "attempt.patch" + engine, _ = make_engine(project, [_escalate_with_patch(project, "1-1-a", patch)]) + assert engine.run().escalated == 1 + + # the resolve session commits its own fix REWRITING the line the patch's + # context expects, then the human latches the restore anyway + repo = project.project + (repo / "src.txt").write_text("corrected by resolution\n") + git(repo, "add", "src.txt") + git(repo, "commit", "-q", "-m", "resolution: overlapping fix") + rearm_escalation(engine.run_dir, restore_patch=str(patch)) + + seen: list[str] = [] + resumed, _ = resume_engine(project, engine, [_restoring_dev_effect(project, "1-1-a", seen)]) + summary = resumed.run() + + assert summary.paused + assert seen == [] # no session ever dispatched onto a half-restored tree + kinds = [e["kind"] for e in resumed.journal.entries()] + assert "attempt-restore-failed" in kinds + assert "attempt-restored" not in kinds + task = load_state(resumed.run_dir).tasks["1-1-a"] + assert task.phase == Phase.ESCALATED # re-escalated: the human re-resolves + # git apply is all-or-nothing: the overlapping resolution commit is untouched + assert (repo / "src.txt").read_text() == "corrected by resolution\n" + + def test_dev_stall_retries_then_succeeds(project): write_sprint(project, {"1-1-a": "ready-for-dev"}) engine, adapter = make_engine( From 66e09cf703ff4404185b49aeb67ea952f252d9c7 Mon Sep 17 00:00:00 2001 From: pbean Date: Tue, 7 Jul 2026 23:07:23 -0700 Subject: [PATCH 05/19] fix(verify): a latched restore patch never counts as proof-of-work (T4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The intent-gap patch file is untracked halt residue under the protected artifact dirs — it survives every reset, so with #79's file-granular excludes its mere presence would let a restore re-drive whose session produced nothing pass the 'no changes since baseline' gate. Thread the task's restore_patch into verify_dev_exclude_relpaths at all three proof-of-work call sites (sprint, bundle, stories) so the gate keys on the applied work, not the patch that carried it. --- src/bmad_loop/verify.py | 23 ++++++++++++++++++----- tests/test_verify.py | 36 ++++++++++++++++++++++++++++++++++++ 2 files changed, 54 insertions(+), 5 deletions(-) diff --git a/src/bmad_loop/verify.py b/src/bmad_loop/verify.py index e3f445a..2278cc8 100644 --- a/src/bmad_loop/verify.py +++ b/src/bmad_loop/verify.py @@ -946,7 +946,9 @@ def artifact_relpaths(paths: ProjectPaths) -> tuple[str, ...]: return tuple(out) -def verify_dev_exclude_relpaths(paths: ProjectPaths, spec_path: Path) -> tuple[str, ...]: +def verify_dev_exclude_relpaths( + paths: ProjectPaths, spec_path: Path, restore_patch: str | None = None +) -> tuple[str, ...]: """Repo-relative posix paths the dev/bundle proof-of-work gate excludes from `has_changes_since` — file-granularity, unlike `artifact_relpaths`' whole-folder exclusion (still used as-is by `Engine._protected_relpaths` for rollback @@ -965,15 +967,26 @@ def verify_dev_exclude_relpaths(paths: ProjectPaths, spec_path: Path) -> tuple[s reconciliation registers as real work instead of a permanent false "no changes since baseline". + `restore_patch` (the task's latched intent-gap patch file, BMAD-METHOD #2564) + is excluded too when set: the patch is untracked halt residue under the + protected artifact dirs that survives every reset, so counting it would let a + restore re-drive whose session produced nothing pass the gate on the patch + file's mere presence — the gate must key on the APPLIED work (the tracked diff + from baseline), not on the orchestrator-owned patch that carried it. + `spec_path` comes from a session-reported (untrusted) `spec_file` string, so it is `.resolve()`d before deriving the relpath, same as `spec_within_roots`: an un-normalized `..`/`.` segment would still resolve to the real on-disk file (the OS resolves it), but as a raw string it wouldn't match git's own normalized path output, silently defeating this exclude and letting a bare status flip on the session's own spec count as real work.""" + candidates: list[Path] = [paths.sprint_status, spec_path] + if restore_patch: + p = Path(restore_patch) + candidates.append(p if p.is_absolute() else paths.project / p) out: list[str] = [] project = paths.project.resolve() - for path in (paths.sprint_status, spec_path): + for path in candidates: try: rel = path.resolve().relative_to(project).as_posix() except ValueError: @@ -1088,7 +1101,7 @@ def verify_dev( task, paths, expected_status="in-review" if review_enabled else "done", - proof_exclude=verify_dev_exclude_relpaths(paths, spec_path), + proof_exclude=verify_dev_exclude_relpaths(paths, spec_path, task.restore_patch), ) if gate is not None: return gate @@ -1131,7 +1144,7 @@ def verify_dev_bundle( task, paths, expected_status="in-review" if review_enabled else "done", - proof_exclude=verify_dev_exclude_relpaths(paths, spec_path), + proof_exclude=verify_dev_exclude_relpaths(paths, spec_path, task.restore_patch), ) if gate is not None: return gate @@ -1244,7 +1257,7 @@ def verify_dev_stories( proof_exclude=( None if plan_halt - else verify_dev_exclude_relpaths(paths, spec_path) + else verify_dev_exclude_relpaths(paths, spec_path, task.restore_patch) + _stories_relpaths(paths.project, spec_folder) ), ) diff --git a/tests/test_verify.py b/tests/test_verify.py index a3c37bc..3ef9731 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -1109,6 +1109,42 @@ def test_verify_dev_exclude_relpaths_is_file_granular(project): assert "_bmad-output/implementation-artifacts/deferred-work.md" not in rels +def test_verify_dev_exclude_relpaths_includes_latched_restore_patch(project): + """T4 (patch-restore × #79): a latched intent-gap patch file joins the + file-granular excludes — absolute or project-relative, both derive the same + repo-relative entry; no latch leaves the excludes unchanged.""" + sp = spec_path(project, "1-1-a") + patch = project.implementation_artifacts / "attempt.patch" + rel = "_bmad-output/implementation-artifacts/attempt.patch" + assert rel in verify.verify_dev_exclude_relpaths(project, sp, str(patch)) + assert rel in verify.verify_dev_exclude_relpaths(project, sp, rel) + assert rel not in verify.verify_dev_exclude_relpaths(project, sp) + + +def test_verify_dev_latched_restore_patch_is_not_proof_of_work(project): + """T4 (patch-restore × #79): the latched patch file is untracked halt residue + under the protected artifact dirs — it survives every reset, so counting it + would let a restore re-drive whose session produced nothing pass the + proof-of-work gate on the patch's mere presence. The gate must key on the + APPLIED work (tracked diff from baseline), not the patch that carried it.""" + write_sprint(project, {"1-1-a": "review"}) + sp = spec_path(project, "1-1-a") + write_spec(sp, "in-review", "NO_VCS") + git(project.project, "add", "-A") + git(project.project, "commit", "-q", "-m", "baseline") + task = make_task(project) + patch = project.implementation_artifacts / "attempt.patch" + patch.write_text("stale attempt diff\n", encoding="utf-8") # untracked residue + + # control: unlatched, the residue is indistinguishable from session work and + # passes the gate — exactly the vacuous pass the latch exclusion prevents + assert verify.verify_dev(task, project, dev_result(sp)).ok + + task.restore_patch = str(patch) + out = verify.verify_dev(task, project, dev_result(sp)) + assert not out.ok and "no changes" in out.reason + + def test_verify_dev_exclude_relpaths_normalizes_dotdot_segments(project): """A spec_path with a lexical '..' hop (as an un-normalized session-reported spec_file could produce) must resolve to the same exclude entry as the plain From 84d4da11ee3fd84478bfc39bc658a567a64ceec8 Mon Sep 17 00:00:00 2001 From: pbean Date: Tue, 7 Jul 2026 23:07:30 -0700 Subject: [PATCH 06/19] docs: finish the three-layer review prose sweep (audit LOW-4) The two lines the source plan targeted but 29ba684 missed: the README review-disabled paragraph and the FEATURES capability-matrix row still said two-layer / 2 parallel layers. Also note the new restore guards in the CHANGELOG bullet. --- CHANGELOG.md | 5 ++++- README.md | 2 +- docs/FEATURES.md | 2 +- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ea34030..a2f04df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +17,10 @@ breaking changes may land in a minor release. reading was correct, `bmad-loop resolve` re-arms the spec to `in-review` and re-applies the patch onto baseline after every reset, so the re-driven session resumes review on the restored diff instead of re-implementing. New `--restore-patch ` flag for the `--no-interactive` path; a - patch that fails to apply escalates instead of running on a half-restored tree. + patch that fails to apply escalates instead of running on a half-restored tree (a resolve session + that committed over the patched lines triggers exactly this — re-resolve without a restore). + Restore is rejected up front for worktree-isolation runs and for stories-mode pre-planning + sentinels, and the latched patch file itself never counts as proof-of-work. - **Preflight covers the inline review layers.** `bmad-loop validate` (and run-start) now require the three upstream review-hunter skills `bmad-dev-auto`'s step-04 invokes — `bmad-review-adversarial-general`, `bmad-review-edge-case-hunter`, and `bmad-review-verification-gap` (new in BMAD-METHOD#2550) — plus a diff --git a/README.md b/README.md index c0bb006..4a3eac3 100644 --- a/README.md +++ b/README.md @@ -392,7 +392,7 @@ low_frame_rate = false # true = cap to 15fps + disable animations (= bmad-lo > In **stories mode** the `stories.yaml` list is flat — there are no epics — so the default `per-epic` gate never fires (nothing to bound). Use the per-story `spec_checkpoint` / `done_checkpoint` flags for HITL there, or `per-story-spec-approval` for a run-global spec gate. `none` and `per-story-spec-approval` behave the same as in sprint mode. -**Review:** `[review].enabled = false` drops the separate fresh-context review session; the dev pass instead runs `bmad-dev-auto`'s own internal two-layer review (Blind Hunter / Edge Case Hunter) and finalizes the story straight to `done` — one session per story instead of two, verify commands still gating the commit. Governs deferred-work sweeps too. When review is enabled, `[review].trigger` decides _when_ that separate pass runs: `recommended` (default) only when the `bmad-dev-auto` session flags `followup_review_recommended` — it already triple-reviews inline and recommends an independent pass only when its changes were significant; `always` runs it every story. The follow-up loop is bounded by `limits.max_review_cycles` (default 3), which caps oscillation. +**Review:** `[review].enabled = false` drops the separate fresh-context review session; the dev pass instead runs `bmad-dev-auto`'s own internal three-layer review (Blind Hunter / Edge Case Hunter / Verification Gap) and finalizes the story straight to `done` — one session per story instead of two, verify commands still gating the commit. Governs deferred-work sweeps too. When review is enabled, `[review].trigger` decides _when_ that separate pass runs: `recommended` (default) only when the `bmad-dev-auto` session flags `followup_review_recommended` — it already triple-reviews inline and recommends an independent pass only when its changes were significant; `always` runs it every story. The follow-up loop is bounded by `limits.max_review_cycles` (default 3), which caps oscillation. `bmad-loop init` (without `--cli`) registers hooks for every CLI profile the policy references, so a dual-client setup needs no extra flags. diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 766c7ab..4b7484d 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -14,7 +14,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se | Dual planning pipelines | Same loop from either `sprint-status.yaml` (sprint mode, default) or a typed `stories.yaml` dispatched by folder+id (stories mode, opt-in) | Sprint boards need `bmad-sprint-planning`; a `bmad-spec` Story Breakdown has no board | | Per-story human checkpoints | Stories-mode `spec_checkpoint` pauses to review the plan before code; `done_checkpoint` pauses after the commit; both independent, both surfaced in the TUI | Coarse run-global gates can't ask for a plan review on _this_ story only | | Trust-nothing verification | Checks on-disk artifacts (spec status, baseline-commit match, non-empty diff, sprint sync) + runs your test/lint commands before commit | Agents claim success without working code; broken builds slip through | -| Fresh-context adversarial review | Dev and review are separate sessions; review uses 2 parallel layers (Blind Hunter / Edge Case Hunter) | Self-review anchoring bias; implementer marks own work correct | +| Fresh-context adversarial review | Dev and review are separate sessions; review uses 3 parallel layers (Blind Hunter / Edge Case Hunter / Verification Gap) | Self-review anchoring bias; implementer marks own work correct | | Hook-based transport | Coding-agent hooks write structured event files; skills write `result.json` | Brittle terminal pane-scraping | | Resumable state machine | Every run is on-disk state, resumable after gate/escalation/crash | Long unattended runs lost to interruptions | | Plateau-defer | Stuck stories are skipped, stashed, and the run continues | One unconvergeable story blocking a whole sprint | From 18d2d679161a701cbebafe6239d46ad1ee82fe8b Mon Sep 17 00:00:00 2001 From: pbean Date: Tue, 7 Jul 2026 23:11:03 -0700 Subject: [PATCH 07/19] test(e2e): provision the new review-layer prereqs in the sandbox scaffolds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The stories/sprint sandbox scaffolds were written on the stories branch before the intent-gap preflight upgrade landed, so they hand-roll the OLD base-skill set — the run now fails preflight wanting bmad-review-verification-gap and bmad-dev-auto's customize.toml marker. Install both stubs in both scaffolds. --- tests/test_stories_e2e.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/tests/test_stories_e2e.py b/tests/test_stories_e2e.py index 756a113..e18488e 100644 --- a/tests/test_stories_e2e.py +++ b/tests/test_stories_e2e.py @@ -159,10 +159,15 @@ def _scaffold(root: Path, entries: list[dict]) -> None: dev.mkdir(parents=True) (dev / "SKILL.md").write_text("# bmad-dev-auto\n", encoding="utf-8") (dev / "step-04-review.md").write_text("x\n", encoding="utf-8") + (dev / "customize.toml").write_text("# review layers\n", encoding="utf-8") (dev / "step-01-clarify-and-route.md").write_text( "This is a **folder+id dispatch** router.\n", encoding="utf-8" ) - for hunter in ("bmad-review-adversarial-general", "bmad-review-edge-case-hunter"): + for hunter in ( + "bmad-review-adversarial-general", + "bmad-review-edge-case-hunter", + "bmad-review-verification-gap", + ): (skills / hunter).mkdir(parents=True) (skills / hunter / "SKILL.md").write_text(f"# {hunter}\n", encoding="utf-8") @@ -223,10 +228,15 @@ def _scaffold_sprint(root: Path, story_key: str) -> None: dev.mkdir(parents=True) (dev / "SKILL.md").write_text("# bmad-dev-auto\n", encoding="utf-8") (dev / "step-04-review.md").write_text("x\n", encoding="utf-8") + (dev / "customize.toml").write_text("# review layers\n", encoding="utf-8") (dev / "step-01-clarify-and-route.md").write_text( "This is a **folder+id dispatch** router.\n", encoding="utf-8" ) - for hunter in ("bmad-review-adversarial-general", "bmad-review-edge-case-hunter"): + for hunter in ( + "bmad-review-adversarial-general", + "bmad-review-edge-case-hunter", + "bmad-review-verification-gap", + ): (skills / hunter).mkdir(parents=True) (skills / hunter / "SKILL.md").write_text(f"# {hunter}\n", encoding="utf-8") From e6ef5735269c636c1bc867cf0eacf949f62685c3 Mon Sep 17 00:00:00 2001 From: pbean Date: Wed, 8 Jul 2026 00:27:19 -0700 Subject: [PATCH 08/19] =?UTF-8?q?style:=20replace=20ambiguous=20=C3=97=20w?= =?UTF-8?q?ith=20ASCII=20x=20in=20comments/docstrings=20(CodeRabbit)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit (Ruff RUF002/RUF003) flagged the U+00D7 MULTIPLICATION SIGN in three comment/docstring sites; swap them to ASCII 'x'. Also fixes two sibling occurrences the review missed in the same PR (tests/test_verify.py T4 docstrings). Intentional × count glyphs (CLI/TUI) and – en-dash parsing regexes are left untouched. --- src/bmad_loop/runs.py | 2 +- tests/test_engine.py | 2 +- tests/test_resolve.py | 2 +- tests/test_verify.py | 4 ++-- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index 833ef3c..c4986e0 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -566,7 +566,7 @@ def rearm_escalation( raise RearmError(f"run {run_dir.name} has no task for story {key}") if task.phase != Phase.ESCALATED: raise RearmError(f"story {key} is not escalated (phase: {task.phase})") - # T1 guard (stories × patch-restore): a sentinel-wedged story escalated BEFORE + # T1 guard (stories x patch-restore): a sentinel-wedged story escalated BEFORE # planning — there is no attempted implementation to restore, and its re-arm # re-dispatches a planning leg. Keyed on the recorded detection verdict # (task.sentinel_kind), not the on-disk basename, mirroring the sentinel-clear diff --git a/tests/test_engine.py b/tests/test_engine.py index fbba0f5..e0c7158 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -2226,7 +2226,7 @@ def test_intent_gap_restore_reapplies_after_mid_redrive_rollback(project): def test_intent_gap_restore_escalates_when_resolution_commits_overlap(project): - """T2 (patch-restore × #78 baseline advance): re-arm adopts the resolve + """T2 (patch-restore x #78 baseline advance): re-arm adopts the resolve session's commits as the re-drive's baseline, but the saved patch was diffed from the ORIGINAL baseline — so a resolution commit that rewrote the patched lines makes the restore's `git apply` fail. The engine must escalate loudly diff --git a/tests/test_resolve.py b/tests/test_resolve.py index c72517e..1ff6732 100644 --- a/tests/test_resolve.py +++ b/tests/test_resolve.py @@ -338,7 +338,7 @@ def test_rearm_sprint_spec_named_like_a_sentinel_is_not_deleted(tmp_path): def test_rearm_rejects_restore_patch_on_a_sentinel(tmp_path): - """T1 (stories × patch-restore): a sentinel-wedged story escalated BEFORE + """T1 (stories x patch-restore): a sentinel-wedged story escalated BEFORE planning — there is no attempted implementation to restore, and its re-arm re-dispatches a planning leg, so laying a patch onto the tree first is never safe. Re-arm must reject loudly BEFORE mutating anything: the sentinel stays diff --git a/tests/test_verify.py b/tests/test_verify.py index 3ef9731..00c8a0c 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -1110,7 +1110,7 @@ def test_verify_dev_exclude_relpaths_is_file_granular(project): def test_verify_dev_exclude_relpaths_includes_latched_restore_patch(project): - """T4 (patch-restore × #79): a latched intent-gap patch file joins the + """T4 (patch-restore x #79): a latched intent-gap patch file joins the file-granular excludes — absolute or project-relative, both derive the same repo-relative entry; no latch leaves the excludes unchanged.""" sp = spec_path(project, "1-1-a") @@ -1122,7 +1122,7 @@ def test_verify_dev_exclude_relpaths_includes_latched_restore_patch(project): def test_verify_dev_latched_restore_patch_is_not_proof_of_work(project): - """T4 (patch-restore × #79): the latched patch file is untracked halt residue + """T4 (patch-restore x #79): the latched patch file is untracked halt residue under the protected artifact dirs — it survives every reset, so counting it would let a restore re-drive whose session produced nothing pass the proof-of-work gate on the patch's mere presence. The gate must key on the From 8634ff3ad7130a3206f84a7fdbd9367630a2f8e6 Mon Sep 17 00:00:00 2001 From: pbean Date: Wed, 8 Jul 2026 01:02:38 -0700 Subject: [PATCH 09/19] fix(engine): sprint restore re-drive dispatches an explicit spec pointer (review F1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A bare '/bmad-dev-auto ' never takes step-01's spec-pointer intent check, so the in-review status the re-arm set could not route to step-04 — and the freeform/epic path's version-control sanity check HALTs blocked on the very diff _restore_patch just applied, re-escalating every sprint-mode restore after one burned session. Point the dispatch at task.spec_file (preserved by re-arm for exactly this) so step-01 EARLY EXITs to step-04 before that check. Stories mode already early-exits via folder+id dispatch and is untouched. --- src/bmad_loop/engine.py | 16 +++++++++++++++- tests/test_engine.py | 24 ++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 1 deletion(-) diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index c080859..1a8dc9d 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -2122,8 +2122,22 @@ def _generic_dev_prompt(self, task: StoryTask, feedback: Path | None) -> str: `--feedback` flag: feedback is inlined as freeform intent pointing at the existing spec. On a repair re-invocation the spec is first re-opened (status → `in-progress`) so the skill's step-01 re-enters implement/review - on it rather than ingesting a finalized spec as mere context.""" + on it rather than ingesting a finalized spec as mere context. + + A patch-restore re-drive (#2564) must point at the spec explicitly: only + step-01's spec-pointer intent check EARLY EXITs on the `in-review` status + the re-arm set — and it exits before step-01's version-control sanity + check, which would otherwise HALT `blocked` on the very diff + `_restore_patch` just laid onto the tree. A bare story key takes the + freeform/epic path instead, where that dirty-tree check runs first.""" if feedback is None: + if task.restore_patch and task.spec_file: + return ( + f"/bmad-dev-auto Resume review of the in-review spec at " + f"`{task.spec_file}`. The attempted change was restored onto " + f"the working tree after an intent-gap resolution; review it " + f"against the amended spec." + ) return f"/bmad-dev-auto {task.story_key}" self._reset_spec_for_repair(task) spec_ref = task.spec_file or task.story_key diff --git a/tests/test_engine.py b/tests/test_engine.py index e0c7158..1d16e3b 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -2200,6 +2200,30 @@ def test_intent_gap_restore_redrive_applies_patch_and_lands_done(project): assert task.restore_patch is None # latch cleared on commit +def test_restore_redrive_prompt_points_at_the_spec(project): + """The sprint-mode restore re-drive must dispatch an explicit spec-file + pointer: only step-01's spec-pointer intent check EARLY EXITs on the + `in-review` status (to step-04) BEFORE its version-control sanity check — a + bare story key takes the freeform/epic path, whose dirty-tree check HALTs + `blocked` on the very diff _restore_patch just laid onto the tree.""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + patch = project.implementation_artifacts / "attempt.patch" + engine, _ = make_engine(project, [_escalate_with_patch(project, "1-1-a", patch)]) + assert engine.run().escalated == 1 + + rearm_escalation(engine.run_dir, restore_patch=str(patch)) + seen: list[str] = [] + resumed, adapter = resume_engine( + project, engine, [_restoring_dev_effect(project, "1-1-a", seen)] + ) + assert resumed.run().done == 1 + + prompt = adapter.sessions[0].prompt + spec = load_state(resumed.run_dir).tasks["1-1-a"].spec_file + assert spec and f"`{spec}`" in prompt # explicit pointer -> step-01 EARLY EXIT + assert prompt != "/bmad-dev-auto 1-1-a" # never the bare key on a restore + + def test_intent_gap_restore_reapplies_after_mid_redrive_rollback(project): """A non-fixable retry inside the restore re-drive resets to baseline (clearing the restored code), so the patch is re-applied before the next dispatch; the From 4eaa76048cfa77a14d8f1f75136ae1da8a41e3f9 Mon Sep 17 00:00:00 2001 From: pbean Date: Wed, 8 Jul 2026 01:06:13 -0700 Subject: [PATCH 10/19] fix(resolve): re-stamp the spec's baseline_revision at a patch-restore re-arm (review F2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The in-review route skips step-03 — the only step that stamps baseline_revision — so the re-driven step-04 diffed (and on an intent-gap/bad-spec re-triage, reverted) 'since' the ORIGINAL pre-attempt sha, clawing back the non-overlapping resolve-session commits the baseline advance explicitly blesses as the re-drive's starting point. rearm_escalation now re-stamps the field to the advanced task baseline on the restore path (loud RearmError on failure); from-scratch re-arms leave it alone (step-03 owns it there). New verify.set_frontmatter_field does the same line surgery as set_frontmatter_status but inserts a missing key. --- src/bmad_loop/runs.py | 19 ++++++++++ src/bmad_loop/verify.py | 39 ++++++++++++++++++++ tests/test_resolve.py | 80 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 138 insertions(+) diff --git a/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index c4986e0..a4f1a42 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -661,6 +661,25 @@ def rearm_escalation( except Exception: # noqa: BLE001 # nosec B110 - best-effort git read, must not fail re-arm pass + # Patch-restore only: re-stamp the spec's own baseline to the advanced one. + # The in-review route skips step-03 — the only step that stamps + # `baseline_revision` — so without this the re-driven step-04 would build its + # review diff (and, on an intent-gap/bad-spec re-triage, revert) "since" the + # ORIGINAL pre-attempt sha, clawing back the very resolve-session commits the + # advance above just blessed as the re-drive's starting point. Loud on + # failure: a silently stale spec baseline is exactly the hazard being closed + # (the spec block above already proved the file readable, so this is remote). + if restore_patch and task.spec_file and task.baseline_commit: + try: + verify.set_frontmatter_field( + Path(task.spec_file), "baseline_revision", task.baseline_commit + ) + except (OSError, UnicodeDecodeError) as e: + raise RearmError( + f"cannot re-stamp baseline_revision on {task.spec_file} " + f"({e.__class__.__name__}: {e}) — fix the file, then re-run resolve" + ) from e + save_state(run_dir, state) journal.append( "story-escalation-resolved", diff --git a/src/bmad_loop/verify.py b/src/bmad_loop/verify.py index 2278cc8..2346f62 100644 --- a/src/bmad_loop/verify.py +++ b/src/bmad_loop/verify.py @@ -922,6 +922,45 @@ def set_frontmatter_status(path: Path, status: str) -> bool: return True +def set_frontmatter_field(path: Path, key: str, value: str) -> bool: + """Rewrite (or insert) a scalar ``:`` line in a spec's `---`…`---` + frontmatter block. + + Same minimal in-place line surgery as `set_frontmatter_status` (no YAML + round-trip) so the spec's formatting, comments, and field order survive. + Unlike the status helper, a missing key is INSERTED as the block's last + line: callers assert a field's value whether or not the skill wrote one + (the patch-restore re-arm re-stamps ``baseline_revision``, which only the + skill's step-03 writes). Returns True when the file was rewritten; False + when it has no frontmatter or already carries the exact value. + """ + if not path.is_file(): + return False + text = path.read_text(encoding="utf-8") + if not text.startswith("---"): + return False + parts = text.split("---", 2) + if len(parts) < 3: + return False + block_lines = parts[1].splitlines(keepends=True) + replaced = False + for i, line in enumerate(block_lines): + stripped = line.lstrip() + if stripped.startswith(f"{key}:"): + indent = line[: len(line) - len(stripped)] + newline = "\n" if line.endswith("\n") else "" + block_lines[i] = f"{indent}{key}: {value}{newline}" + replaced = True + break + if not replaced: + block_lines.append(f"{key}: {value}\n") + rebuilt = parts[0] + "---" + "".join(block_lines) + "---" + parts[2] + if rebuilt == text: # already at the target value — idempotent no-op + return False + path.write_text(rebuilt, encoding="utf-8") + return True + + def artifact_relpaths(paths: ProjectPaths) -> tuple[str, ...]: """Repo-relative posix prefixes of the orchestrator-owned BMAD artifact folders (the output root and the implementation/planning artifact dirs), diff --git a/tests/test_resolve.py b/tests/test_resolve.py index 1ff6732..f521794 100644 --- a/tests/test_resolve.py +++ b/tests/test_resolve.py @@ -100,6 +100,24 @@ def test_set_frontmatter_status_no_frontmatter(tmp_path): assert verify.set_frontmatter_status(spec, "ready-for-dev") is False +def test_set_frontmatter_field_replaces_inserts_idempotent(tmp_path): + spec = tmp_path / "spec.md" + spec.write_text(SPEC, encoding="utf-8") + assert verify.set_frontmatter_field(spec, "owner", "winston") is True + assert verify.read_frontmatter(spec)["owner"] == "winston" + # unlike set_frontmatter_status, a missing key is INSERTED (block's last line) + assert verify.set_frontmatter_field(spec, "baseline_revision", "abc123") is True + fm = verify.read_frontmatter(spec) + assert fm["baseline_revision"] == "abc123" + assert fm["status"] == "in-review" and fm["title"] == "List command" # untouched + # idempotent: already at the target value + assert verify.set_frontmatter_field(spec, "baseline_revision", "abc123") is False + # no frontmatter block -> no write + bare = tmp_path / "bare.md" + bare.write_text("# just a heading\n", encoding="utf-8") + assert verify.set_frontmatter_field(bare, "baseline_revision", "abc123") is False + + # ----------------------------------------------------------- build_context @@ -385,6 +403,68 @@ def test_rearm_restore_patch_on_a_real_stories_spec_is_allowed(tmp_path): assert verify.read_frontmatter(spec)["status"] == "in-review" # restore routing +def _resolve_repo(tmp_path): + """A tiny real repo so rearm's baseline advance has a HEAD to read.""" + git(tmp_path, "init", "-q", "-b", "main") + git(tmp_path, "config", "user.email", "test@test") + git(tmp_path, "config", "user.name", "test") + (tmp_path / ".gitignore").write_text(".bmad-loop/runs/\n") + (tmp_path / "src.txt").write_text("original\n") + git(tmp_path, "add", "-A") + git(tmp_path, "commit", "-q", "-m", "initial") + return git(tmp_path, "rev-parse", "HEAD") + + +def test_rearm_restore_patch_restamps_spec_baseline(tmp_path): + """The in-review route skips step-03 — the only step that stamps + `baseline_revision` — so the patch-restore re-arm re-stamps it to the + advanced baseline itself. Otherwise the re-driven step-04 would build its + review diff (and, on an intent-gap/bad-spec re-triage, revert) "since" the + ORIGINAL pre-attempt sha, clawing back the very resolve-session commits the + baseline advance blesses as the re-drive's starting point.""" + key = "6-4-cli-list-command" + old_head = _resolve_repo(tmp_path) + spec = tmp_path / "spec.md" + spec.write_text( + f"---\nstatus: blocked\nbaseline_revision: {old_head}\n---\n\n## Intent\n\nx\n", + encoding="utf-8", + ) + run_dir, _, _ = _escalated_run(tmp_path, spec_file=str(spec)) + # the resolve session leaves a commit NOT overlapping the patch (blessed input) + (tmp_path / "fixture.txt").write_text("resolution fixture\n") + git(tmp_path, "add", "-A") + git(tmp_path, "commit", "-q", "-m", "resolution fixture") + new_head = git(tmp_path, "rev-parse", "HEAD") + + runs.rearm_escalation(run_dir, restore_patch="artifacts/attempt.patch") + + fm = verify.read_frontmatter(spec) + assert fm["baseline_revision"] == new_head # step-04 diffs from the ADVANCED baseline + assert fm["status"] == "in-review" + assert load_state(run_dir).tasks[key].baseline_commit == new_head + + +def test_rearm_from_scratch_leaves_spec_baseline_alone(tmp_path): + """A from-scratch re-arm routes ready-for-dev -> step-03, which re-stamps + `baseline_revision` itself — the re-arm must not touch it.""" + old_head = _resolve_repo(tmp_path) + spec = tmp_path / "spec.md" + spec.write_text( + f"---\nstatus: blocked\nbaseline_revision: {old_head}\n---\n\n## Intent\n\nx\n", + encoding="utf-8", + ) + run_dir, _, _ = _escalated_run(tmp_path, spec_file=str(spec)) + (tmp_path / "fixture.txt").write_text("resolution fixture\n") + git(tmp_path, "add", "-A") + git(tmp_path, "commit", "-q", "-m", "resolution fixture") + + runs.rearm_escalation(run_dir) # no restore + + fm = verify.read_frontmatter(spec) + assert fm["baseline_revision"] == old_head # untouched; step-03 owns the stamp + assert fm["status"] == "ready-for-dev" + + # -------------------------------------------------- non-UTF-8 robustness (bug class) # A story spec / sentinel is agent- or human-authored, so it can contain non-UTF-8 # bytes. `read_text(encoding="utf-8")` raises UnicodeDecodeError (a ValueError, NOT an From 9e50b537d7905ced858538f9bcf6d1cd0bf285c8 Mon Sep 17 00:00:00 2001 From: pbean Date: Wed, 8 Jul 2026 01:08:48 -0700 Subject: [PATCH 11/19] fix(resolve): corrupt or empty restore inputs abort instead of silently re-driving from scratch (review F3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit read_resolution collapsed a present-but-unparseable resolution.json into None — indistinguishable from 'nothing recorded' and quieter than absence (the produced-marker warning keys on existence). Likewise --restore-patch '' (unset shell var) fell through 'if not raw' as a plain re-arm, even masking a recorded restore. Both silently consumed the escalation (a re-armed task can't be re-resolved), losing the human's confirmed restore decision. read_resolution now returns None only for an ABSENT marker and raises ResolutionError otherwise; _resolve_restore_patch errors on an empty flag and on an empty or non-string restore_patch field. --- src/bmad_loop/cli.py | 42 ++++++++++++++---- src/bmad_loop/resolve.py | 25 ++++++++--- tests/test_cli.py | 92 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 144 insertions(+), 15 deletions(-) diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index c1d590d..bb754a1 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -814,16 +814,42 @@ def _resolve_restore_patch( otherwise, on the interactive path, the resolve agent may have recorded a ``restore_patch`` field in resolution.json. Returns ``(latch, error)``: a validated absolute patch path to latch (None = ordinary from-scratch re-drive), - or an error string when a supplied path is missing / outside the project, or - the run's isolation mode can't restore in place — the caller aborts strictly - rather than silently re-driving from scratch when the operator asked to - restore.""" + or an error string when a supplied path is missing / outside the project, the + run's isolation mode can't restore in place, or the restore input itself is + corrupt (unreadable resolution.json, empty/non-string value) — the caller + aborts strictly rather than silently re-driving from scratch when a restore + was (or may have been) asked for.""" raw = getattr(args, "restore_patch", None) + if raw is not None and not raw.strip(): + # `--restore-patch ""` is a classic unset-shell-var slip. Treating it as + # "no restore" would silently re-drive from scratch (and even mask a + # restore the resolve agent recorded) — and a re-arm consumes the + # escalation, so the dropped decision would be unrecoverable. + return None, ( + "--restore-patch got an empty path (unset shell variable?) — pass the " + "saved patch path, or drop the flag entirely for a from-scratch re-drive" + ) if raw is None and args.interactive: - doc = resolve.read_resolution(run_dir, story_key) - if doc is not None: - val = doc.get("restore_patch") - raw = str(val) if val else None + try: + doc = resolve.read_resolution(run_dir, story_key) + except resolve.ResolutionError as e: + return None, ( + f"{e} — the recorded resolution (and any restore_patch decision in " + "it) cannot be read; fix or delete the file, or re-run with " + "--no-interactive [--restore-patch ] to decide by hand" + ) + val = None if doc is None else doc.get("restore_patch") + if val is not None: + # the schema says omit the field for an ordinary resolution; an empty + # or non-string value is a corrupted recorded decision, not "none" + if not isinstance(val, str) or not val.strip(): + return None, ( + f"resolution.json for {story_key} carries an invalid " + f"restore_patch value {val!r} — expected a non-empty path (or " + "the field omitted); fix the file, or re-run with " + "--no-interactive [--restore-patch ] to decide by hand" + ) + raw = val if not raw: return None, None # Restore is an in-place-only recovery: a worktree-isolation re-drive discards diff --git a/src/bmad_loop/resolve.py b/src/bmad_loop/resolve.py index aa5b437..99fcfc1 100644 --- a/src/bmad_loop/resolve.py +++ b/src/bmad_loop/resolve.py @@ -38,19 +38,30 @@ def resolution_path(run_dir: Path, story_key: str) -> Path: return _story_dir(run_dir, story_key) / "resolution.json" +class ResolutionError(Exception): + """resolution.json exists but cannot be used (unreadable / not a JSON object).""" + + def read_resolution(run_dir: Path, story_key: str) -> dict[str, Any] | None: - """Parse the resolve agent's ``resolution.json`` marker, or None when it is - absent or unreadable. The caller reads the optional ``restore_patch`` field - (the intent-gap patch-restore path, BMAD-METHOD #2564) from the returned dict; - it validates that path itself before acting on it.""" + """Parse the resolve agent's ``resolution.json`` marker. Returns None ONLY + when the marker is absent; a present-but-unusable marker (unreadable, bad + JSON, non-object top level) raises ResolutionError instead. The file is the + agent's recorded decision — possibly including a ``restore_patch`` (the + intent-gap patch-restore path, BMAD-METHOD #2564) — and a re-arm consumes + the escalation, so collapsing corruption into "nothing recorded" would + silently downgrade a confirmed restore to an unrecoverable from-scratch + re-drive. The caller validates the ``restore_patch`` path itself before + acting on it.""" path = resolution_path(run_dir, story_key) if not path.is_file(): return None try: doc = json.loads(path.read_text(encoding="utf-8")) - except (OSError, json.JSONDecodeError): - return None - return doc if isinstance(doc, dict) else None + except (OSError, json.JSONDecodeError) as e: + raise ResolutionError(f"resolution marker {path} is unreadable ({e})") from e + if not isinstance(doc, dict): + raise ResolutionError(f"resolution marker {path} is not a JSON object") + return doc def _gather_escalations(run_dir: Path, state: RunState, story_key: str) -> list[dict[str, Any]]: diff --git a/tests/test_cli.py b/tests/test_cli.py index 2581b00..1e005eb 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -1130,6 +1130,98 @@ def fake_session(adapter, project, rd, story_key, *, model=""): assert "in-review" in spec.read_text() +def test_resolve_corrupt_resolution_json_aborts_loudly(tmp_path, monkeypatch, capsys): + """A present-but-unparseable resolution.json may carry the agent's recorded + restore decision, and a re-arm consumes the escalation — so corruption must + abort (no re-arm, no resume), never silently downgrade to a from-scratch + re-drive quieter than an absent marker.""" + from bmad_loop import resolve + from bmad_loop.journal import load_state + from bmad_loop.model import Phase + + spec = tmp_path / "spec.md" + spec.write_text("---\nstatus: blocked\n---\n", encoding="utf-8") + run_dir = _escalated_run(tmp_path, "r1", spec_file=str(spec)) + + def fake_session(adapter, project, rd, story_key, *, model=""): + marker = resolve.resolution_path(rd, story_key) + marker.parent.mkdir(parents=True, exist_ok=True) + marker.write_text('{"restore_patch": "artifacts/attempt.patch",}', encoding="utf-8") + return True + + monkeypatch.setattr(cli, "_make_adapters", lambda *a, **k: {"dev": object()}) + monkeypatch.setattr(resolve, "build_context", lambda *a, **k: None) + monkeypatch.setattr(resolve, "run_session", fake_session) + called: list = [] + monkeypatch.setattr(cli, "_resume_paused_run", lambda proj, rd: called.append(rd) or 0) + rc = cli.main(["resolve", "--project", str(tmp_path), "r1", "--resume"]) + assert rc == 1 + assert "unreadable" in capsys.readouterr().err + assert called == [] # never resumed + task = load_state(run_dir).tasks["s1"] + assert task.phase == Phase.ESCALATED and task.restore_patch is None # not re-armed + assert "status: blocked" in spec.read_text() # spec untouched + + +def test_resolve_empty_restore_patch_field_aborts_loudly(tmp_path, monkeypatch, capsys): + """`"restore_patch": ""` in resolution.json (schema says omit the field) is a + corrupted recorded decision, not "no restore" — abort, don't re-arm.""" + from bmad_loop import resolve + from bmad_loop.journal import load_state + from bmad_loop.model import Phase + + spec = tmp_path / "spec.md" + spec.write_text("---\nstatus: blocked\n---\n", encoding="utf-8") + run_dir = _escalated_run(tmp_path, "r1", spec_file=str(spec)) + + def fake_session(adapter, project, rd, story_key, *, model=""): + marker = resolve.resolution_path(rd, story_key) + marker.parent.mkdir(parents=True, exist_ok=True) + marker.write_text(json.dumps({"restore_patch": ""}), encoding="utf-8") + return True + + monkeypatch.setattr(cli, "_make_adapters", lambda *a, **k: {"dev": object()}) + monkeypatch.setattr(resolve, "build_context", lambda *a, **k: None) + monkeypatch.setattr(resolve, "run_session", fake_session) + called: list = [] + monkeypatch.setattr(cli, "_resume_paused_run", lambda proj, rd: called.append(rd) or 0) + rc = cli.main(["resolve", "--project", str(tmp_path), "r1", "--resume"]) + assert rc == 1 + assert "invalid" in capsys.readouterr().err + assert called == [] + assert load_state(run_dir).tasks["s1"].phase == Phase.ESCALATED # not re-armed + + +def test_resolve_empty_restore_patch_flag_aborts_loudly(tmp_path, monkeypatch, capsys): + """`--restore-patch ""` (unset shell variable) must abort, not silently + re-drive from scratch: the flag said restore, and a re-arm would consume the + escalation with the decision dropped.""" + from bmad_loop.journal import load_state + from bmad_loop.model import Phase + + spec = tmp_path / "spec.md" + spec.write_text("---\nstatus: blocked\n---\n", encoding="utf-8") + run_dir = _escalated_run(tmp_path, "r1", spec_file=str(spec)) + called: list = [] + monkeypatch.setattr(cli, "_resume_paused_run", lambda proj, rd: called.append(rd) or 0) + rc = cli.main( + [ + "resolve", + "--project", + str(tmp_path), + "r1", + "--no-interactive", + "--restore-patch", + "", + "--resume", + ] + ) + assert rc == 1 + assert "empty path" in capsys.readouterr().err + assert called == [] + assert load_state(run_dir).tasks["s1"].phase == Phase.ESCALATED # not re-armed + + def test_sweep_command_parses_flags(): parser_args = [ "sweep", From ba9add8e6bf60a08cb9ece5288d1dec0718cca2d Mon Sep 17 00:00:00 2001 From: pbean Date: Wed, 8 Jul 2026 01:09:35 -0700 Subject: [PATCH 12/19] fix(resolve): a spec-less escalation rejects a restore latch (review F4) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The latch was assigned unconditionally while the in-review routing flip lives under 'if task.spec_file:' — so an escalation with no recorded spec (ambiguous two-file wedge, unknown --story selector, session died before naming one) latched the patch with no routing target, and the engine would lay it onto the tree before a planning leg. Rejected at the T1 seam, before any mutation; from-scratch re-arms unaffected. --- src/bmad_loop/runs.py | 12 ++++++++++++ tests/test_resolve.py | 20 ++++++++++++++++++++ 2 files changed, 32 insertions(+) diff --git a/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index a4f1a42..3c3d83a 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -578,6 +578,18 @@ def rearm_escalation( "there is no attempted implementation to restore, and the re-drive starts " "at planning. Re-run resolve without a restore patch for a clean re-plan." ) + # Same seam, broader shape: a restore only works through the spec's in-review + # flip below, so an escalation with NO recorded spec (an ambiguous two-file + # wedge, an unknown --story selector, a session that died before naming one) + # has no routing target — the latch would stick, the flip would be skipped, + # and the engine would lay the patch onto the tree before a planning leg. + # Rejected before any mutation, like the sentinel guard above. + if restore_patch and not task.spec_file: + raise RearmError( + f"story {key} has no recorded spec file, so a restored patch has no " + "review to resume (the re-drive starts at planning). Re-run resolve " + "without a restore patch for a from-scratch re-drive." + ) journal = Journal(run_dir) # deliberate reset, not a normal state-machine transition (mirrors diff --git a/tests/test_resolve.py b/tests/test_resolve.py index f521794..20ceeba 100644 --- a/tests/test_resolve.py +++ b/tests/test_resolve.py @@ -385,6 +385,26 @@ def test_rearm_rejects_restore_patch_on_a_sentinel(tmp_path): assert not (run_dir / "journal.jsonl").exists() +def test_rearm_rejects_restore_patch_without_a_spec_file(tmp_path): + """A restore only works through the spec's in-review flip, so an escalated + task with NO recorded spec (ambiguous two-file wedge, unknown --story + selector, session died before naming one) has no routing target: the latch + would stick, the flip would be skipped, and the engine would lay the patch + onto the tree before a planning leg. Rejected before any mutation.""" + run_dir, _, _ = _escalated_run(tmp_path, spec_file=None) + + with pytest.raises(runs.RearmError, match="no recorded spec file"): + runs.rearm_escalation(run_dir, restore_patch="artifacts/attempt.patch") + + task = load_state(run_dir).tasks["6-4-cli-list-command"] + assert task.phase == Phase.ESCALATED # not re-armed; the escalation stays armed + assert task.restore_patch is None # no latch persisted + assert not (run_dir / "journal.jsonl").exists() # nothing journaled + + runs.rearm_escalation(run_dir) # a from-scratch re-arm remains available + assert load_state(run_dir).tasks["6-4-cli-list-command"].phase == Phase.PENDING + + def test_rearm_restore_patch_on_a_real_stories_spec_is_allowed(tmp_path): """The T1 guard keys on the recorded sentinel verdict, not on stories mode: a review-stage intent gap on a REAL stories spec (sentinel_kind unset) is a From 7e3ee7a645b1f0192fbb81e542f9a68b3285f12e Mon Sep 17 00:00:00 2001 From: pbean Date: Wed, 8 Jul 2026 01:13:24 -0700 Subject: [PATCH 13/19] fix(cli): restore-patch containment accepts the configured artifact roots (review F5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit bmad-dev-auto contractually saves the intent-gap patch under implementation_artifacts, and artifact dirs configured outside the project tree are a supported layout (bmadconfig keeps them absolute; verify special-cases them throughout) — but the containment check was a bare is_relative_to(project), so that layout rejected every legitimate restore with an error blaming the path. Validate against the same trusted roots as verify.spec_within_roots instead. Knock-ons are safe: the T4 proof-of-work exclude skips out-of-project paths (which git can't count as changes anyway) and git apply takes absolute patch paths. Restore tests now provision the minimal _bmad config every real project has. --- src/bmad_loop/cli.py | 15 +++++++++--- tests/test_cli.py | 56 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 3 deletions(-) diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index bb754a1..fb72e49 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -869,10 +869,19 @@ def _resolve_restore_patch( if not patch.is_absolute(): patch = project / patch patch = patch.resolve() - if not patch.is_file() or not patch.is_relative_to(project.resolve()): + # Same trusted-roots shape as the frontmatter reconcile's spec_within_roots: + # bmad-dev-auto saves the patch under implementation_artifacts, and artifact + # dirs configured OUTSIDE the project tree are a supported layout — a bare + # is_relative_to(project) check would reject every legitimate restore there. + try: + paths = bmadconfig.load_paths(project) + except bmadconfig.BmadConfigError as e: + return None, f"cannot validate the restore patch path against the project config: {e}" + if not patch.is_file() or not verify.spec_within_roots(patch, paths): return None, ( - f"restore patch {raw!r} is not a file under the project — refusing to " - "re-arm (fix the path, or re-run without a restore to re-drive from scratch)" + f"restore patch {raw!r} is not a file under the project or its " + "configured artifact roots — refusing to re-arm (fix the path, or " + "re-run without a restore to re-drive from scratch)" ) return str(patch), None diff --git a/tests/test_cli.py b/tests/test_cli.py index 1e005eb..7f505b0 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -812,6 +812,18 @@ def test_archive_refuses_live_run_without_force(tmp_path, monkeypatch, capsys): assert run_dir.exists() +def _write_bmad_config(project, impl="{project-root}/artifacts"): + """Minimal _bmad/bmm/config.yaml — restore-patch validation resolves the + artifact roots through bmadconfig.load_paths, like every real project.""" + cfg = project / "_bmad" / "bmm" + cfg.mkdir(parents=True, exist_ok=True) + (cfg / "config.yaml").write_text( + f"implementation_artifacts: '{impl}'\n" + "planning_artifacts: '{project-root}/planning'\n", + encoding="utf-8", + ) + + def _escalated_run(project, run_id="r1", *, story="s1", spec_file=None): from bmad_loop.model import Phase, StoryTask @@ -977,6 +989,7 @@ def test_resolve_no_interactive_restore_patch_latches_in_review(tmp_path, monkey spec = tmp_path / "spec.md" spec.write_text("---\nstatus: blocked\n---\n", encoding="utf-8") + _write_bmad_config(tmp_path) patch = tmp_path / "artifacts" / "attempt.patch" patch.parent.mkdir(parents=True) patch.write_text("diff", encoding="utf-8") @@ -1009,6 +1022,7 @@ def test_resolve_restore_patch_missing_file_rejected(tmp_path, monkeypatch, caps spec = tmp_path / "spec.md" spec.write_text("---\nstatus: blocked\n---\n", encoding="utf-8") + _write_bmad_config(tmp_path) run_dir = _escalated_run(tmp_path, "r1", spec_file=str(spec)) called: list = [] monkeypatch.setattr(cli, "_resume_paused_run", lambda proj, rd: called.append(rd) or 0) @@ -1039,6 +1053,7 @@ def test_resolve_restore_patch_outside_project_rejected(tmp_path, monkeypatch, c project = tmp_path / "proj" project.mkdir() + _write_bmad_config(project) outside = tmp_path / "outside.patch" # a real file, wrong side of the fence outside.write_text("diff", encoding="utf-8") spec = project / "spec.md" @@ -1065,6 +1080,46 @@ def test_resolve_restore_patch_outside_project_rejected(tmp_path, monkeypatch, c assert task.phase == Phase.ESCALATED and task.restore_patch is None # not re-armed +def test_resolve_restore_patch_in_outside_project_artifacts_allowed(tmp_path, monkeypatch): + """Artifact dirs configured OUTSIDE the project tree are a supported layout + (bmadconfig keeps them absolute; verify special-cases them throughout), and + bmad-dev-auto saves the intent-gap patch in implementation_artifacts — so a + patch there is a legitimate restore target: the containment check uses the + same trusted roots as spec_within_roots, not a bare under-project test.""" + from bmad_loop.journal import load_state + from bmad_loop.model import Phase + + project = tmp_path / "proj" + project.mkdir() + shared = tmp_path / "shared-artifacts" # sibling dir, outside the project tree + shared.mkdir() + _write_bmad_config(project, impl=str(shared)) + patch = shared / "attempt.patch" + patch.write_text("diff", encoding="utf-8") + spec = project / "spec.md" + spec.write_text("---\nstatus: blocked\n---\n", encoding="utf-8") + run_dir = _escalated_run(project, "r1", spec_file=str(spec)) + + monkeypatch.setattr(cli, "_resume_paused_run", lambda proj, rd: 0) + rc = cli.main( + [ + "resolve", + "--project", + str(project), + "r1", + "--no-interactive", + "--restore-patch", + str(patch), + "--resume", + ] + ) + assert rc == 0 + task = load_state(run_dir).tasks["s1"] + assert task.phase == Phase.PENDING + assert task.restore_patch == str(patch.resolve()) # latched despite being out-of-project + assert "in-review" in spec.read_text() + + def test_resolve_restore_patch_worktree_isolation_rejected(tmp_path, monkeypatch, capsys): """B4d: restore is an in-place-only recovery — a worktree-isolation re-drive discards and re-mounts the unit's worktree, so a latched patch would silently @@ -1107,6 +1162,7 @@ def test_resolve_interactive_restore_patch_from_resolution_json(tmp_path, monkey spec = tmp_path / "spec.md" spec.write_text("---\nstatus: blocked\n---\n", encoding="utf-8") + _write_bmad_config(tmp_path) patch = tmp_path / "artifacts" / "attempt.patch" patch.parent.mkdir(parents=True) patch.write_text("diff", encoding="utf-8") From 137cb6e55279d61e13f3eb3edf3fe298c8efaaf7 Mon Sep 17 00:00:00 2001 From: pbean Date: Wed, 8 Jul 2026 01:18:05 -0700 Subject: [PATCH 14/19] fix(cli): validate a restore before the interactive resolve session it would invalidate (review F6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The worktree-isolation rejection ran only AFTER resolve.run_session — the agent (with no isolation signal in its context and none in SKILL.md) would legitimately negotiate a restore, record it, and only then hit an rc=1 whose naive retry unlinks resolution.json and repeats the whole session; the only deterministic escape (--no-interactive) was unnamed. Now: the explicit --restore-patch flag is validated before the session (policy loaded once, killing the double TOML parse); build_context exports restore_supported and SKILL.md tells the agent never to negotiate a restore when it is false (both reseeds updated); the rejection message names the --no-interactive escape; and the guard keys on the recorded task.worktree_path as well as live policy, so a policy edit between escalation and resolve can't skew it. --- src/bmad_loop/cli.py | 74 +++++++++++------ .../data/skills/bmad-loop-resolve/SKILL.md | 11 ++- src/bmad_loop/resolve.py | 8 +- tests/test_cli.py | 79 ++++++++++++++++++- tests/test_resolve.py | 18 +++++ 5 files changed, 162 insertions(+), 28 deletions(-) diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index fb72e49..ec07ccb 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -806,19 +806,26 @@ def _confirm(question: str) -> bool: def _resolve_restore_patch( - project: Path, run_dir: Path, story_key: str, args: argparse.Namespace + project: Path, + run_dir: Path, + story_key: str, + args: argparse.Namespace, + pol, + task, ) -> tuple[str | None, str | None]: """Determine the intent-gap patch-restore latch (BMAD-METHOD #2564) for a re-arm. Precedence: the explicit ``--restore-patch`` flag (hand-driven recovery) wins; otherwise, on the interactive path, the resolve agent may have recorded a - ``restore_patch`` field in resolution.json. Returns ``(latch, error)``: a - validated absolute patch path to latch (None = ordinary from-scratch re-drive), - or an error string when a supplied path is missing / outside the project, the - run's isolation mode can't restore in place, or the restore input itself is - corrupt (unreadable resolution.json, empty/non-string value) — the caller - aborts strictly rather than silently re-driving from scratch when a restore - was (or may have been) asked for.""" + ``restore_patch`` field in resolution.json. The flag path is fully knowable + before the interactive session, so cmd_resolve validates it FIRST and only + falls back here post-session for the resolution.json read. Returns + ``(latch, error)``: a validated absolute patch path to latch (None = ordinary + from-scratch re-drive), or an error string when a supplied path is missing / + outside the trusted roots, the run can't restore in place, or the restore + input itself is corrupt (unreadable resolution.json, empty/non-string value) + — the caller aborts strictly rather than silently re-driving from scratch + when a restore was (or may have been) asked for.""" raw = getattr(args, "restore_patch", None) if raw is not None and not raw.strip(): # `--restore-patch ""` is a classic unset-shell-var slip. Treating it as @@ -853,17 +860,21 @@ def _resolve_restore_patch( if not raw: return None, None # Restore is an in-place-only recovery: a worktree-isolation re-drive discards - # and re-mounts the unit's worktree (engine._finish_inflight), so a patch laid - # onto the project tree has nowhere durable to land and the engine's apply seam - # is never reached. Reject up front instead of latching a patch that would - # silently never restore. - pol = policy_mod.load(_policy_path(project)) - if pol.scm.isolation == "worktree": + # the unit's worktree (engine._finish_inflight — taking a patch saved inside it + # along) and re-mounts a fresh one, so the re-apply could only fail on a + # destroyed patch file. Reject up front instead of latching a patch that can + # never restore. Checked against BOTH the recorded run state + # (task.worktree_path — how the escalated unit actually executed) and the live + # policy (how the resume will execute), so a policy edit between escalation + # and resolve can't skew the guard. + if pol.scm.isolation == "worktree" or getattr(task, "worktree_path", ""): return None, ( "restore patch is unsupported for worktree-isolation runs (the re-drive " "discards and re-mounts the unit's worktree, so an in-place restore has " - "nothing durable to land on) — re-run without a restore patch for a " - "plain re-drive" + "nothing durable to land on) — re-arm from scratch instead: drop " + "--restore-patch, or if the resolve agent recorded the restore in " + "resolution.json, re-run with --no-interactive (which ignores that " + "marker) instead of repeating the agent session" ) patch = Path(raw) if not patch.is_absolute(): @@ -933,11 +944,25 @@ def cmd_resolve(args: argparse.Namespace) -> int: print(f"no escalated story to resolve in run {args.run_id}", file=sys.stderr) return 1 + pol = policy_mod.load(_policy_path(project)) + + # intent-gap patch-restore latch (#2564), explicit-flag path: everything about + # it (isolation mode, path containment) is knowable NOW — validate before the + # interactive resolve session, not after a whole agent conversation the abort + # would throw away. The resolution.json path can only be validated + # post-session (below); build_context tells the agent up front when a restore + # can't be honored so it never negotiates one. + restore_patch: str | None = None + if args.restore_patch is not None: + restore_patch, err = _resolve_restore_patch(project, run_dir, story_key, args, pol, task) + if err is not None: + print(err, file=sys.stderr) + return 1 + if args.interactive: - pol = policy_mod.load(_policy_path(project)) adapters = _make_adapters(project, run_dir, pol) model = pol.adapter.resolved("dev").model - resolve.build_context(state, run_dir, story_key) + resolve.build_context(state, run_dir, story_key, isolation=pol.scm.isolation) print(f"launching resolve agent for {story_key} — converse, fix the spec, then exit…") try: produced = resolve.run_session( @@ -956,12 +981,13 @@ def cmd_resolve(args: argparse.Namespace) -> int: file=sys.stderr, ) - # intent-gap patch-restore latch (#2564): validate a supplied patch path before - # prompting, so a bad path fails loudly instead of after a confirmation. - restore_patch, err = _resolve_restore_patch(project, run_dir, story_key, args) - if err is not None: - print(err, file=sys.stderr) - return 1 + # resolution.json restore latch: only exists after the session ran, so this + # arm of the validation cannot be hoisted above it. + if args.restore_patch is None: + restore_patch, err = _resolve_restore_patch(project, run_dir, story_key, args, pol, task) + if err is not None: + print(err, file=sys.stderr) + return 1 # confirm-then-resume (args.resume: None = ask, True = auto, False = re-arm only) if args.resume is None and not _confirm(f"re-arm {story_key} and resume run {args.run_id}?"): diff --git a/src/bmad_loop/data/skills/bmad-loop-resolve/SKILL.md b/src/bmad_loop/data/skills/bmad-loop-resolve/SKILL.md index 2d566cf..46279d7 100644 --- a/src/bmad_loop/data/skills/bmad-loop-resolve/SKILL.md +++ b/src/bmad_loop/data/skills/bmad-loop-resolve/SKILL.md @@ -118,7 +118,16 @@ dev session first **saved its attempted change as a patch file** (in the implementation-artifacts folder) before reverting the tree — the escalation `detail` and the spec's `## Review Triage Log` reference the patch path. That patch is concrete evidence: it shows exactly which reading of the intent the run -implemented. Use it two ways: +implemented. + +**First check `restore_supported` in the context file.** When it is `false` +(worktree-isolation runs: the re-drive discards and re-mounts the unit's +worktree, so an in-place restore can never land), **never offer the restore +option and never record `restore_patch`** — the orchestrator would reject the +resolution and this whole session's negotiation would be wasted. The patch is +still available as evidence. + +Use the patch two ways: - **As evidence.** Read the patch (and the diff it represents) to see what the guessed reading produced — often clearer input for writing the clarification diff --git a/src/bmad_loop/resolve.py b/src/bmad_loop/resolve.py index 99fcfc1..bc4609b 100644 --- a/src/bmad_loop/resolve.py +++ b/src/bmad_loop/resolve.py @@ -89,7 +89,7 @@ def _gather_escalations(run_dir: Path, state: RunState, story_key: str) -> list[ return found -def build_context(state: RunState, run_dir: Path, story_key: str) -> Path: +def build_context(state: RunState, run_dir: Path, story_key: str, *, isolation: str = "") -> Path: """Write resolve//context.json for the resolve skill to read.""" task = state.tasks.get(story_key) context = { @@ -102,6 +102,12 @@ def build_context(state: RunState, run_dir: Path, story_key: str) -> Path: # as_posix so the context contract is the same string on every OS (the # path is consumed by the agent, and Python/tools accept '/' on Windows). "resolution_path": resolution_path(run_dir, story_key).as_posix(), + # Patch-restore availability (#2564): a worktree-isolation re-drive + # discards and re-mounts the unit's worktree, so an in-place restore can + # never land — the orchestrator rejects a `restore_patch` up front. Told + # to the agent here so it never negotiates a restore it can't honor. + "restore_supported": isolation != "worktree" + and not (task.worktree_path if task else ""), } # Stories mode: hand the resolver the manifest intent (the story entry) and a # sentinel indicator, so it sees WHAT the story is meant to do and WHETHER the diff --git a/tests/test_cli.py b/tests/test_cli.py index 7f505b0..9b7dbf3 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -824,10 +824,17 @@ def _write_bmad_config(project, impl="{project-root}/artifacts"): ) -def _escalated_run(project, run_id="r1", *, story="s1", spec_file=None): +def _escalated_run(project, run_id="r1", *, story="s1", spec_file=None, worktree_path=""): from bmad_loop.model import Phase, StoryTask - task = StoryTask(story_key=story, epic=1, phase=Phase.ESCALATED, attempt=1, spec_file=spec_file) + task = StoryTask( + story_key=story, + epic=1, + phase=Phase.ESCALATED, + attempt=1, + spec_file=spec_file, + worktree_path=worktree_path, + ) return _make_run_with_state( project, run_id, @@ -1156,6 +1163,74 @@ def test_resolve_restore_patch_worktree_isolation_rejected(tmp_path, monkeypatch assert "status: blocked" in spec.read_text() # spec status untouched +def test_resolve_restore_patch_flag_rejected_before_the_session(tmp_path, monkeypatch, capsys): + """The explicit --restore-patch flag is fully knowable pre-session (isolation + mode, path containment), so on a worktree-isolation run it must be rejected + BEFORE launching the interactive resolve agent — not after a whole + conversation the abort would throw away.""" + from bmad_loop import resolve + + _write_policy(tmp_path, '[scm]\nisolation = "worktree"\n') + spec = tmp_path / "spec.md" + spec.write_text("---\nstatus: blocked\n---\n", encoding="utf-8") + _write_bmad_config(tmp_path) + patch = tmp_path / "artifacts" / "attempt.patch" + patch.parent.mkdir(parents=True) + patch.write_text("diff", encoding="utf-8") + _escalated_run(tmp_path, "r1", spec_file=str(spec)) + + def never(*a, **k): + raise AssertionError("interactive resolve session launched despite a doomed restore") + + monkeypatch.setattr(cli, "_make_adapters", never) + monkeypatch.setattr(resolve, "run_session", never) + # interactive is the default: no --no-interactive here + rc = cli.main( + ["resolve", "--project", str(tmp_path), "r1", "--restore-patch", str(patch), "--resume"] + ) + assert rc == 1 + err = capsys.readouterr().err + assert "worktree" in err + assert "--no-interactive" in err # the deterministic escape is named + + +def test_resolve_restore_patch_rejected_for_worktree_executed_task(tmp_path, monkeypatch, capsys): + """The guard keys on the recorded run state too: a task that actually + executed in a worktree (task.worktree_path) rejects a restore even when the + policy has since been flipped back to in-place — the patch was saved inside + the (discarded) worktree, so there is nothing durable to restore.""" + from bmad_loop.journal import load_state + from bmad_loop.model import Phase + + spec = tmp_path / "spec.md" + spec.write_text("---\nstatus: blocked\n---\n", encoding="utf-8") + _write_bmad_config(tmp_path) + patch = tmp_path / "artifacts" / "attempt.patch" + patch.parent.mkdir(parents=True) + patch.write_text("diff", encoding="utf-8") + run_dir = _escalated_run( + tmp_path, "r1", spec_file=str(spec), worktree_path=str(tmp_path / "wt" / "s1") + ) + called: list = [] + monkeypatch.setattr(cli, "_resume_paused_run", lambda proj, rd: called.append(rd) or 0) + rc = cli.main( + [ + "resolve", + "--project", + str(tmp_path), + "r1", + "--no-interactive", + "--restore-patch", + str(patch), + "--resume", + ] + ) + assert rc == 1 + assert "worktree" in capsys.readouterr().err + assert called == [] + assert load_state(run_dir).tasks["s1"].phase == Phase.ESCALATED # not re-armed + + def test_resolve_interactive_restore_patch_from_resolution_json(tmp_path, monkeypatch): from bmad_loop import resolve from bmad_loop.journal import load_state diff --git a/tests/test_resolve.py b/tests/test_resolve.py index 20ceeba..a17813e 100644 --- a/tests/test_resolve.py +++ b/tests/test_resolve.py @@ -158,6 +158,24 @@ def test_build_context_no_session_files(tmp_path): assert ctx["paused_reason"].startswith("CRITICAL") +def test_build_context_restore_supported_signal(tmp_path): + """The agent must know up front when a patch-restore can't be honored + (worktree isolation / a worktree-executed task), so it never negotiates a + restore the orchestrator will reject after the session.""" + run_dir, state, task = _escalated_run(tmp_path, with_session=False) + key = "6-4-cli-list-command" + + path = resolve.build_context(state, run_dir, key) + assert json.loads(path.read_text(encoding="utf-8"))["restore_supported"] is True + + path = resolve.build_context(state, run_dir, key, isolation="worktree") + assert json.loads(path.read_text(encoding="utf-8"))["restore_supported"] is False + + task.worktree_path = str(tmp_path / "wt") # recorded worktree execution + path = resolve.build_context(state, run_dir, key) + assert json.loads(path.read_text(encoding="utf-8"))["restore_supported"] is False + + # ----------------------------------------------------------- rearm_escalation From 1a465d9c9455451568c9cf9d356014e62d28626a Mon Sep 17 00:00:00 2001 From: pbean Date: Wed, 8 Jul 2026 01:19:11 -0700 Subject: [PATCH 15/19] docs: two remaining 'two review hunters' references -> three (review F7) The LOW-4 prose sweep (84d4da1) claimed completion but missed docs/setup-guide.md and the _require_base_skills docstring. --- docs/setup-guide.md | 2 +- src/bmad_loop/cli.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/setup-guide.md b/docs/setup-guide.md index f6d2b6d..b9b23e5 100644 --- a/docs/setup-guide.md +++ b/docs/setup-guide.md @@ -85,7 +85,7 @@ installs all the bundled skills together (`bmad-loop-resolve`, `bmad-loop-sweep` the orchestrator normalizes the ledger to. The dev primitive `bmad-dev-auto` is **not** bundled: it is the upstream skill the orchestrator drives (for both implementation and the follow-up review), installed by the BMad Method (bmm) -module. `bmad-loop validate` checks it — plus the two adversarial review hunters it +module. `bmad-loop validate` checks it — plus the three review hunters it invokes inline — are present before a run starts. ## Choosing which CLIs to drive diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index ec07ccb..abda40a 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -256,7 +256,7 @@ def cmd_validate(args: argparse.Namespace) -> int: def _require_base_skills(project: Path, pol, *, require_stories: bool = False) -> bool: """Preflight the upstream skills the orchestrator drives (bmad-dev-auto + the - two adversarial review hunters it invokes inline). + three review hunters it invokes inline). Returns True when everything is in place; otherwise prints the problems and returns False so the caller can abort before spawning any session (a missing From 7386caaadf76311ce452bfdb7fe23861ba8ac987 Mon Sep 17 00:00:00 2001 From: pbean Date: Wed, 8 Jul 2026 01:22:05 -0700 Subject: [PATCH 16/19] fix(tui): a recorded restore decision is surfaced at re-arm, never dropped silently (review F8) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The escalation modal's Re-arm button is enabled by resolution.json's existence yet re-armed with no restore latch — the always-assign semantics actively select from-scratch — so a restore_patch the human confirmed through the interactive resolve flow was discarded without a word (reachable via resolve -> 'n' at the confirm -> TUI Re-arm). Auto-honoring from the TUI would be worse (a consumed marker is indistinguishable from a fresh one on disk), so Re-arm stays a plain from-scratch re-drive; but the modal hint now flags the recorded restore and the re-arm notifies that only 'bmad-loop resolve' applies it. An unreadable marker counts as recorded (it may carry one). --- src/bmad_loop/tui/app.py | 29 ++++++++++++++- src/bmad_loop/tui/screens/modals.py | 14 ++++++- tests/test_tui_app.py | 58 +++++++++++++++++++++++++++++ 3 files changed, 98 insertions(+), 3 deletions(-) diff --git a/src/bmad_loop/tui/app.py b/src/bmad_loop/tui/app.py index f968c7b..72fdbac 100644 --- a/src/bmad_loop/tui/app.py +++ b/src/bmad_loop/tui/app.py @@ -588,6 +588,7 @@ def _review_escalation(self, run_id: str, run_dir: Path, state: RunState) -> Non story_key = state.paused_story_key or "?" spec_path, spec_text = self._paused_spec(state) title, description = self._story_context(state, story_key) + restore_recorded = self._restore_recorded(run_dir, story_key) modal = EscalationModal( story_key=story_key, title=title, @@ -596,6 +597,7 @@ def _review_escalation(self, run_id: str, run_dir: Path, state: RunState) -> Non sentinel_kind=self._sentinel_kind(state, story_key), resolution_ready=resolve.resolution_path(run_dir, story_key).is_file(), engine_live=_engine_possibly_live(run_dir), + restore_recorded=restore_recorded, ) def done(verb: str | None) -> None: @@ -604,10 +606,25 @@ def done(verb: str | None) -> None: return self._launch_resolve(run_id) elif verb == "rearm": - self._do_rearm(run_id, run_dir, story_key) + self._do_rearm(run_id, run_dir, story_key, restore_recorded=restore_recorded) self.push_screen(modal, done) + @staticmethod + def _restore_recorded(run_dir: Path, story_key: str) -> bool: + """True when resolution.json records — or, being unreadable, MAY record — + a restore_patch. The TUI re-arm path is a plain from-scratch re-drive + (only the CLI resolve flow honors the latch, because a stale marker is + indistinguishable from a fresh one here), so a recorded restore must be + surfaced rather than silently dropped.""" + if not resolve.resolution_path(run_dir, story_key).is_file(): + return False + try: + doc = resolve.read_resolution(run_dir, story_key) + except resolve.ResolutionError: + return True # can't prove it carries no restore — surface the warning + return bool(doc and doc.get("restore_patch")) + # --------------------------------------------------- shared pause code paths def _do_resume(self, run_id: str) -> None: @@ -657,7 +674,9 @@ def _do_replan(self, run_id: str, spec_path: Path) -> None: self.notify("plan reset to draft — the next dispatch re-plans") self._do_resume(run_id) - def _do_rearm(self, run_id: str, run_dir: Path, story_key: str) -> None: + def _do_rearm( + self, run_id: str, run_dir: Path, story_key: str, *, restore_recorded: bool = False + ) -> None: """Re-arm a resolved escalation + resume — the `resolve --no-interactive` path (rearm_escalation handles sentinel auto-delete-with-preservation).""" if self._resolve_blocked_by_liveness(run_id, run_dir): @@ -667,6 +686,12 @@ def _do_rearm(self, run_id: str, run_dir: Path, story_key: str) -> None: except RearmError as e: self.notify(f"re-arm failed: {e}", severity="error") return + if restore_recorded: + self.notify( + "recorded restore patch NOT honored — this re-arm re-drives from " + "scratch (only `bmad-loop resolve` applies a restore)", + severity="warning", + ) self.notify(f"re-armed {story_key}") self._do_resume(run_id) diff --git a/src/bmad_loop/tui/screens/modals.py b/src/bmad_loop/tui/screens/modals.py index a29da1e..568863c 100644 --- a/src/bmad_loop/tui/screens/modals.py +++ b/src/bmad_loop/tui/screens/modals.py @@ -551,6 +551,7 @@ def __init__( sentinel_kind: str, resolution_ready: bool, engine_live: bool, + restore_recorded: bool = False, ): super().__init__() self._story_key = story_key @@ -560,6 +561,7 @@ def __init__( self._sentinel_kind = sentinel_kind self._resolution_ready = resolution_ready self._engine_live = engine_live + self._restore_recorded = restore_recorded def compose(self) -> ComposeResult: head = Text() @@ -588,7 +590,17 @@ def compose(self) -> ComposeResult: Text("engine may still be live — stop it before resolving", style="yellow") ) hint = Text() - if self._resolution_ready: + if self._restore_recorded: + # honoring the latch from here would be unsafe (a stale marker is + # indistinguishable from a fresh one), so Re-arm stays a plain + # from-scratch re-drive — but never a silent drop of the decision. + hint.append( + "⚠ the resolution records a restore patch — Re-arm here re-drives " + "from scratch and drops it; run `bmad-loop resolve` to honor the " + "restore", + style="yellow", + ) + elif self._resolution_ready: hint.append("resolution recorded — re-arm & resume when ready", style="green") else: hint.append( diff --git a/tests/test_tui_app.py b/tests/test_tui_app.py index d9ebf5c..193afac 100644 --- a/tests/test_tui_app.py +++ b/tests/test_tui_app.py @@ -1709,6 +1709,64 @@ async def test_escalation_rearm_resumes_when_resolution_ready(project, monkeypat await until(pilot, lambda: rearms == ["1"] and calls == ["20260611-100000-aaaa"]) +def test_restore_recorded_helper(tmp_path): + """review F8: absent marker / no restore field -> False; a recorded + restore_patch -> True; an UNREADABLE marker -> True (it may carry one, so + the warning must err toward surfacing).""" + from bmad_loop import resolve + + assert BmadLoopApp._restore_recorded(tmp_path, "1") is False # absent + marker = resolve.resolution_path(tmp_path, "1") + marker.parent.mkdir(parents=True, exist_ok=True) + marker.write_text("{}", encoding="utf-8") + assert BmadLoopApp._restore_recorded(tmp_path, "1") is False # no restore field + marker.write_text('{"restore_patch": "artifacts/a.patch"}', encoding="utf-8") + assert BmadLoopApp._restore_recorded(tmp_path, "1") is True + marker.write_text('{"restore_patch": "artifacts/a.patch",}', encoding="utf-8") + assert BmadLoopApp._restore_recorded(tmp_path, "1") is True # corrupt -> conservative + + +async def test_escalation_rearm_warns_when_restore_recorded(project, monkeypatch): + """review F8: a resolution.json carrying restore_patch still enables Re-arm + (it IS a recorded resolution) but the modal flags it and the re-arm notifies + that the restore is NOT honored here — only `bmad-loop resolve` applies a + latch — so the human's confirmed decision is never dropped silently.""" + from bmad_loop import resolve, runs + + calls: list[str] = [] + rearms: list[str] = [] + notes: list[str] = [] + monkeypatch.setattr(launch, "tmux_available", lambda: True) + monkeypatch.setattr(launch, "resume_detached", lambda proj, rid: calls.append(rid)) + monkeypatch.setattr(data, "liveness", lambda run_dir: "dead") + monkeypatch.setattr( + runs, "rearm_escalation", lambda rd, sk: rearms.append(sk) or "ready-for-dev" + ) + orig_notify = BmadLoopApp.notify + monkeypatch.setattr( + BmadLoopApp, + "notify", + lambda self, msg, **kw: notes.append(str(msg)) or orig_notify(self, msg, **kw), + ) + run_dir, _spec = _stories_paused_run( + project.project, + stage="escalation", + spec_status="blocked", + spec_checkpoint=False, + blocked_result="Blocked: intent gap; saved patch: artifacts/attempt.patch", + ) + marker = resolve.resolution_path(run_dir, "1") + marker.parent.mkdir(parents=True, exist_ok=True) + marker.write_text('{"restore_patch": "artifacts/attempt.patch"}', encoding="utf-8") + app = BmadLoopApp(project.project) + async with app.run_test() as pilot: + await _open_review(app, pilot, EscalationModal) + assert app.screen._restore_recorded is True # the modal shows the warning hint + await pilot.click(await ready(pilot, "#act-rearm")) + await until(pilot, lambda: rearms == ["1"] and calls == ["20260611-100000-aaaa"]) + assert any("NOT honored" in n for n in notes) # the drop was surfaced, not silent + + async def test_escalation_rearm_disabled_without_resolution(project, monkeypatch): monkeypatch.setattr(data, "liveness", lambda run_dir: "dead") _stories_paused_run( From f64ea034d3cde6cc0b2474aecde8b0ef2171273f Mon Sep 17 00:00:00 2001 From: pbean Date: Wed, 8 Jul 2026 01:25:51 -0700 Subject: [PATCH 17/19] test(e2e): sprint-mode intent-gap patch-restore through the real CLI (scenario 7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The fake dev CLI grows the #2564 choreography: a committed .intent-gap- marker makes the first dispatch save the attempted change as a patch, revert, and block; an in-review re-drive asserts the two orchestrator contracts the way real step-01 would — the prompt must carry an explicit spec pointer (F1) and the attempt must already be restored onto the tree — blocking loudly otherwise, then resumes review to done without re-implementing. The test also pins the F2 re-stamp (spec baseline_revision == post-re-arm HEAD) and that the restored line lands exactly once in the story commit. --- tests/test_stories_e2e.py | 105 +++++++++++++++++++++++++++++++++++--- 1 file changed, 98 insertions(+), 7 deletions(-) diff --git a/tests/test_stories_e2e.py b/tests/test_stories_e2e.py index e18488e..d170ac0 100644 --- a/tests/test_stories_e2e.py +++ b/tests/test_stories_e2e.py @@ -15,12 +15,16 @@ stories read-back, git commit, and the resolve/resume dance. Covers, through the real binary: (1) two-story happy path, (2) `spec_checkpoint` -two-leg plan-halt + resume, (4) blocked → resolve → re-dispatch, and (6) +two-leg plan-halt + resume, (4) blocked → resolve → re-dispatch, (6) sprint-mode regression (the new folder+id-capable dev skill installed, yet a plain sprint run drives dev → verify → commit → sprint-status advance untouched -by the stories wiring). Scenarios (3) `done_checkpoint` and (5) worktree -isolation are covered deterministically at the engine level in -test_stories_engine.py; here we prove the end-to-end CLI stack. +by the stories wiring), and (7) sprint-mode intent-gap patch-restore (halt saves +the attempt as a patch → `resolve --restore-patch` re-arms to in-review + +re-stamps the spec baseline → resume re-applies the patch and dispatches an +explicit spec pointer, resuming review instead of re-implementing). Scenarios +(3) `done_checkpoint` and (5) worktree isolation are covered deterministically +at the engine level in test_stories_engine.py; here we prove the end-to-end CLI +stack. """ from __future__ import annotations @@ -46,6 +50,7 @@ set -e rd="$BMAD_LOOP_RUN_DIR"; tid="$BMAD_LOOP_TASK_ID" story="$BMAD_LOOP_STORY_KEY"; folder="$BMAD_LOOP_SPEC_FOLDER" +prompt="${1:-}" ts=$(date +%s%N) mkdir -p "$rd/events" printf '{"ts": %s, "event": "SessionStart", "task_id": "%s", "session_id": "fake-1"}' \ @@ -56,12 +61,44 @@ # installed, but a plain sprint run must still work. Write the result artifact # the orchestrator scans by mtime under implementation-artifacts, make a real # code change, and Stop — the orchestrator (not the skill) advances sprint-status. +# Routes like step-01: an in-review spec is a patch-restore re-drive (#2564); a +# committed `.intent-gap-` marker makes the first dispatch halt the way +# bmad-dev-auto's review does on an intent gap (save attempt as patch, revert, +# block); otherwise a plain implement-to-done. if [ -z "$folder" ]; then impl="_bmad-output/implementation-artifacts" mkdir -p "$impl" - echo "impl for $story" >> src.txt - printf -- '---\ntitle: %s\nstatus: done\nbaseline_commit: %s\n---\n\n## Intent\n\nx\n\n## Auto Run Result\n\n- Status: done\n\nSummary: sprint.\n' \ - "$story" "$baseline" > "$impl/spec-$story.md" + spec="$impl/spec-$story.md" + patch="$impl/attempt-$story.patch" + status="" + [ -f "$spec" ] && status=$(sed -n 's/^status:[[:space:]]*//p' "$spec" | head -1 | tr -d "'\" ") + if [ "$status" = "in-review" ]; then + # Patch-restore re-drive: resume REVIEW on the restored diff — never + # re-implement. Enforce the two orchestrator-side contracts the way the + # real step-01 would: the prompt must point at the spec explicitly (an + # in-review spec only routes to step-04 through the spec-pointer intent + # check), and the attempted change must already be back on the tree. + if ! printf '%s' "$prompt" | grep -qF "$spec"; then + printf -- '---\ntitle: %s\nstatus: blocked\nbaseline_commit: %s\n---\n\n## Auto Run Result\n\n- Status: blocked\n\nprompt lacks the spec pointer.\n' \ + "$story" "$baseline" > "$spec" + elif ! grep -q "attempted reading" src.txt; then + printf -- '---\ntitle: %s\nstatus: blocked\nbaseline_commit: %s\n---\n\n## Auto Run Result\n\n- Status: blocked\n\ntree was not restored.\n' \ + "$story" "$baseline" > "$spec" + else + printf -- '---\ntitle: %s\nstatus: done\nbaseline_commit: %s\n---\n\n## Intent\n\nx\n\n## Auto Run Result\n\n- Status: done\n\nSummary: reviewed the restored change.\n' \ + "$story" "$baseline" > "$spec" + fi + elif [ -f ".intent-gap-$story" ] && [ ! -f "$patch" ]; then + echo "attempted reading for $story" >> src.txt + git diff HEAD > "$patch" + git checkout -- src.txt + printf -- '---\ntitle: %s\nstatus: blocked\nbaseline_commit: %s\n---\n\n## Intent\n\nx\n\n## Auto Run Result\n\n- Status: blocked\n\nintent gap; saved patch: %s\n' \ + "$story" "$baseline" "$patch" > "$spec" + else + echo "impl for $story" >> src.txt + printf -- '---\ntitle: %s\nstatus: done\nbaseline_commit: %s\n---\n\n## Intent\n\nx\n\n## Auto Run Result\n\n- Status: done\n\nSummary: sprint.\n' \ + "$story" "$baseline" > "$spec" + fi ts2=$(( ts + 1 )) printf '{"ts": %s, "event": "Stop", "task_id": "%s", "session_id": "fake-1"}' \ "$ts2" "$tid" > "$rd/events/$ts2-$tid-Stop.json" @@ -387,6 +424,60 @@ def test_e2e_blocked_resolve_redispatch(tmp_path): assert _commit_count(root) == base + 2 +def test_e2e_sprint_intent_gap_patch_restore(tmp_path): + # Scenario 7 (review F1/F2, end-to-end): a sprint-mode intent-gap halt saves + # the attempted change as a patch and reverts; `resolve --restore-patch` + # re-arms the spec to in-review and re-stamps its baseline; resume re-applies + # the patch onto the tree and dispatches an EXPLICIT spec pointer, so the + # (fake) skill resumes review on the restored diff instead of re-implementing. + # The fake blocks loudly if the prompt lacks the pointer or the tree was not + # restored, so a `done` landing proves both contracts held. + root = tmp_path / "sbx" + story = "1-1-thing" + _scaffold_sprint(root, story) + (root / f".intent-gap-{story}").write_text("", encoding="utf-8") + _git(root, "add", "-A") + _git(root, "commit", "-q", "-m", "poison: intent gap") + base = _commit_count(root) + impl = root / "_bmad-output" / "implementation-artifacts" + spec = impl / f"spec-{story}.md" + patch = impl / f"attempt-{story}.patch" + + # leg 1: the dev session halts on the intent gap — patch saved, tree reverted + proc = _run(root, "run") + assert proc.returncode == 0, proc.stderr or proc.stdout + assert "status: blocked" in spec.read_text(encoding="utf-8") + assert patch.is_file() # the attempted change survives the revert + assert "attempted reading" not in (root / "src.txt").read_text(encoding="utf-8") + run_id = _run_id(root) + + # the human confirms the attempted reading: latch the restore + resolve = _run( + root, "resolve", run_id, "--no-interactive", "--no-resume", "--restore-patch", str(patch) + ) + assert resolve.returncode == 0, resolve.stderr or resolve.stdout + text = spec.read_text(encoding="utf-8") + assert "status: in-review" in text # restore routing: step-01 -> step-04 + head = subprocess.run( + ["git", "-C", str(root), "rev-parse", "HEAD"], capture_output=True, text=True + ).stdout.strip() + assert f"baseline_revision: {head}" in text # F2: spec baseline re-stamped + + # resume: patch re-applied, review resumed, story lands done + committed + resume = _run(root, "resume", run_id) + assert resume.returncode == 0, resume.stderr or resume.stdout + final = spec.read_text(encoding="utf-8") + assert "status: done" in final, final # fake blocks loudly on a broken contract + assert _sprint_status(root, story) == "done" + assert _commit_count(root) == base + 1 + src = (root / "src.txt").read_text(encoding="utf-8") + assert src.count("attempted reading") == 1 # restored from the patch, not re-implemented + # F1: the re-drive dispatch pointed at the spec, never the bare story key + run_dir = root / ".bmad-loop" / "runs" / run_id + prompts = [p.read_text(encoding="utf-8") for p in (run_dir / "tasks").glob("*/prompt.txt")] + assert any(str(spec) in p for p in prompts) + + def test_e2e_sprint_mode_regression(tmp_path): # Scenario 6 (audit MAJOR-2): the new folder+id-capable bmad-dev-auto skill is # installed, but this is a plain SPRINT-mode run. It must drive dev → verify → From f0b43a19522c423edc5baed8b8669c0af0da49fc Mon Sep 17 00:00:00 2001 From: pbean Date: Wed, 8 Jul 2026 01:26:49 -0700 Subject: [PATCH 18/19] style: black line-join in the test_cli config helper (trunk fmt) --- src/bmad_loop/resolve.py | 3 +-- tests/test_cli.py | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/src/bmad_loop/resolve.py b/src/bmad_loop/resolve.py index bc4609b..8945b1c 100644 --- a/src/bmad_loop/resolve.py +++ b/src/bmad_loop/resolve.py @@ -106,8 +106,7 @@ def build_context(state: RunState, run_dir: Path, story_key: str, *, isolation: # discards and re-mounts the unit's worktree, so an in-place restore can # never land — the orchestrator rejects a `restore_patch` up front. Told # to the agent here so it never negotiates a restore it can't honor. - "restore_supported": isolation != "worktree" - and not (task.worktree_path if task else ""), + "restore_supported": isolation != "worktree" and not (task.worktree_path if task else ""), } # Stories mode: hand the resolver the manifest intent (the story entry) and a # sentinel indicator, so it sees WHAT the story is meant to do and WHETHER the diff --git a/tests/test_cli.py b/tests/test_cli.py index 9b7dbf3..1a4c3a2 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -818,8 +818,7 @@ def _write_bmad_config(project, impl="{project-root}/artifacts"): cfg = project / "_bmad" / "bmm" cfg.mkdir(parents=True, exist_ok=True) (cfg / "config.yaml").write_text( - f"implementation_artifacts: '{impl}'\n" - "planning_artifacts: '{project-root}/planning'\n", + f"implementation_artifacts: '{impl}'\n" "planning_artifacts: '{project-root}/planning'\n", encoding="utf-8", ) From f5c1b363db9b64cf8fd477e86e20e17ee54b7db1 Mon Sep 17 00:00:00 2001 From: pbean Date: Wed, 8 Jul 2026 01:44:32 -0700 Subject: [PATCH 19/19] fix(resolve): a non-UTF-8 resolution marker raises ResolutionError, not UnicodeDecodeError (CodeRabbit) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit UnicodeDecodeError is a ValueError, not an OSError, so it escaped read_resolution's except tuple as a crash instead of the clean fail-loud abort F3 added — same bug class as the read_frontmatter / stories read-path hardening. --- src/bmad_loop/resolve.py | 5 ++++- tests/test_resolve.py | 11 +++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/bmad_loop/resolve.py b/src/bmad_loop/resolve.py index 8945b1c..e428e8d 100644 --- a/src/bmad_loop/resolve.py +++ b/src/bmad_loop/resolve.py @@ -57,7 +57,10 @@ def read_resolution(run_dir: Path, story_key: str) -> dict[str, Any] | None: return None try: doc = json.loads(path.read_text(encoding="utf-8")) - except (OSError, json.JSONDecodeError) as e: + except (OSError, UnicodeDecodeError, json.JSONDecodeError) as e: + # UnicodeDecodeError is a ValueError, not an OSError — a non-UTF-8 marker + # must raise the clean ResolutionError, not crash (same class as the + # read_frontmatter / stories read-path hardening). raise ResolutionError(f"resolution marker {path} is unreadable ({e})") from e if not isinstance(doc, dict): raise ResolutionError(f"resolution marker {path} is not a JSON object") diff --git a/tests/test_resolve.py b/tests/test_resolve.py index a17813e..fe9d601 100644 --- a/tests/test_resolve.py +++ b/tests/test_resolve.py @@ -591,6 +591,17 @@ def test_rearm_tolerates_non_utf8_sentinel(tmp_path): assert cleared[0]["sentinel_kind"] == "unresolved" and cleared[0]["condition"] == "" +def test_read_resolution_non_utf8_marker_is_resolution_error(tmp_path): + """UnicodeDecodeError is a ValueError, not an OSError — a non-UTF-8 marker + must surface as the clean ResolutionError every consumer handles (CLI abort, + TUI conservative warning), never an uncaught decode crash.""" + marker = resolve.resolution_path(tmp_path, "6-4-cli-list-command") + marker.parent.mkdir(parents=True) + marker.write_bytes(_BAD_UTF8) + with pytest.raises(resolve.ResolutionError, match="unreadable"): + resolve.read_resolution(tmp_path, "6-4-cli-list-command") + + def test_rearm_rejects_non_escalation_stage(tmp_path): run_dir = tmp_path / ".bmad-loop" / "runs" / "r1" save_state(