diff --git a/CHANGELOG.md b/CHANGELOG.md index d7a4bd7..a2f04df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,20 @@ 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 (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 + `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..4a3eac3 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: @@ -387,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 82e0d84..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 | @@ -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/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 fb5acba..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 @@ -805,6 +805,98 @@ 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, + 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. 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 + # "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: + 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 + # 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-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(): + patch = project / patch + patch = patch.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 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 + + def cmd_resolve(args: argparse.Namespace) -> int: from .model import PAUSE_ESCALATION, Phase @@ -852,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( @@ -875,16 +981,27 @@ def cmd_resolve(args: argparse.Namespace) -> int: file=sys.stderr, ) + # 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}?"): 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 +1638,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..46279d7 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,55 @@ 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. + +**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 + 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. + + **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. + ## 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..1a8dc9d 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) @@ -2072,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/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..e428e8d 100644 --- a/src/bmad_loop/resolve.py +++ b/src/bmad_loop/resolve.py @@ -38,6 +38,35 @@ 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. 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, 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") + return doc + + 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. @@ -63,7 +92,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 = { @@ -76,6 +105,11 @@ 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/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index 790300c..3c3d83a 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,30 @@ 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 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 + # 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." + ) + # 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 @@ -554,6 +600,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 +625,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 @@ -621,8 +673,32 @@ def rearm_escalation(run_dir: Path, story_key: str | None = None) -> str: 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", 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/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/src/bmad_loop/verify.py b/src/bmad_loop/verify.py index da0e70d..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), @@ -946,7 +985,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 +1006,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 +1140,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 +1183,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 +1296,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) ), ) @@ -1398,6 +1450,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 +1490,33 @@ 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. 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}") + 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..1a4c3a2 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -812,10 +812,28 @@ def test_archive_refuses_live_run_without_force(tmp_path, monkeypatch, capsys): assert run_dir.exists() -def _escalated_run(project, run_id="r1", *, story="s1", spec_file=None): +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, 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, @@ -971,6 +989,369 @@ 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") + _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)) + + 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") + _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) + 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_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() + _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" + 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_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 + 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_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 + + 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)) + + 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_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", diff --git a/tests/test_engine.py b/tests/test_engine.py index b7ab620..1d16e3b 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -2067,6 +2067,223 @@ 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_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 + 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_intent_gap_restore_escalates_when_resolution_commits_overlap(project): + """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 + 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( 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_resolve.py b/tests/test_resolve.py index e70dbad..fe9d601 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 @@ -140,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 @@ -337,6 +373,136 @@ 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 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 + 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_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 + 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 + + +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 @@ -425,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( 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_stories_e2e.py b/tests/test_stories_e2e.py index 756a113..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" @@ -159,10 +196,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 +265,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") @@ -377,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 → 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( diff --git a/tests/test_verify.py b/tests/test_verify.py index 8665ee2..00c8a0c 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") @@ -1066,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 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") + 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 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 + 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