refactor(restore): one latch validator, one path normalizer, one exclusion site

CHANGELOG.md

@@ -29,6 +29,21 @@ breaking changes may land in a minor release.
   `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.
 
+### Changed
+
+- **The patch-restore seam is now one validator, one path normalizer, and one exclusion site.**
+  `runs.validate_restore_latch` holds every latch precondition (sentinel wedge, spec-less escalation,
+  worktree isolation) — the worktree check lived only in the CLI, so `rearm_escalation` called
+  programmatically could latch a patch the re-drive can never honor; it now rejects it too.
+  `verify.resolve_restore_path` replaces four copies of the maybe-relative→absolute join, and the
+  shared verify gate derives the restore-patch proof-of-work exclusion from the task instead of
+  threading it in from three call sites. The resolve context's `restore_supported` signal is now the
+  validator's verdict too, so the agent never negotiates a restore for a sentinel-wedged or spec-less
+  escalation either. Otherwise behavior-neutral. (closes #91)
+- **Test helper fidelity.** `make_engine` seeds the launching scope (`max_stories`, `story_filter`,
+  `epic_filter`) on `RunState` like `cmd_run` does, so resume tests no longer silently ran uncapped;
+  the three `_escalated_run` fixtures collapse into one parameterized conftest builder. (closes #84)
+
 ### Fixed
 
 - **An abandoned patch-restore no longer smuggles its files into the corrected story's commit.**

src/bmad_loop/cli.py

@@ -811,6 +811,7 @@ def _resolve_restore_patch(
     story_key: str,
     args: argparse.Namespace,
     pol,
+    state,
     task,
 ) -> tuple[str | None, str | None]:
     """Determine the intent-gap patch-restore latch (BMAD-METHOD #2564) for a re-arm.
@@ -825,7 +826,12 @@ def _resolve_restore_patch(
     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."""
+    when a restore was (or may have been) asked for.
+
+    The run-state preconditions come from ``runs.validate_restore_latch``, shared
+    verbatim with ``runs.rearm_escalation``; only the CLI-side halves live here —
+    path resolution against ``--project`` and trusted-roots containment, which need
+    the loaded bmad config."""
     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
@@ -859,27 +865,21 @@ def _resolve_restore_patch(
             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()
+    # The state-side preconditions (sentinel wedge, spec-less escalation, worktree
+    # isolation) are the same set rearm_escalation enforces — run them here so an
+    # unhonorable restore aborts BEFORE the interactive resolve session rather than
+    # after. The live policy's isolation mode is the one input run state can't
+    # carry, so pass it: a policy edit between escalation and resolve can't skew
+    # the guard.
+    err = runs.validate_restore_latch(
+        state, task, story_key, worktree_isolation=pol.scm.isolation == "worktree"
+    )
+    if err is not None:
+        return None, err
+    # `.resolve()` on top of the shared normalizer: this is the one consumer that
+    # feeds a containment check (spec_within_roots), which needs `..`/symlinks
+    # collapsed. The resolved absolute path is what gets latched.
+    patch = verify.resolve_restore_path(raw, project).resolve()
     # Same trusted-roots shape as the frontmatter reconcile's spec_within_roots:
     # bmad-dev-auto saves the patch under implementation_artifacts, and artifact
     # dirs configured OUTSIDE the project tree are a supported layout — a bare
@@ -984,7 +984,9 @@ def cmd_resolve(args: argparse.Namespace) -> int:
     # 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)
+        restore_patch, err = _resolve_restore_patch(
+            project, run_dir, story_key, args, pol, state, task
+        )
         if err is not None:
             print(err, file=sys.stderr)
             return 1
@@ -1014,7 +1016,9 @@ def cmd_resolve(args: argparse.Namespace) -> int:
     # 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)
+        restore_patch, err = _resolve_restore_patch(
+            project, run_dir, story_key, args, pol, state, task
+        )
         if err is not None:
             print(err, file=sys.stderr)
             return 1

src/bmad_loop/data/skills/bmad-loop-resolve/SKILL.md

@@ -122,7 +122,9 @@ 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
+worktree, so an in-place restore can never land; an escalation with no recorded
+spec: a restored patch has no review to resume; a pre-planning sentinel wedge:
+there is no attempted implementation to restore), **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.

src/bmad_loop/engine.py

@@ -905,9 +905,7 @@ def _restore_patch(self, task: StoryTask) -> None:
         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
+        patch = verify.resolve_restore_path(task.restore_patch, self.workspace.root)
         try:
             verify.apply_patch(self.workspace.root, patch)
         except verify.GitError as e:
@@ -917,8 +915,11 @@ def _restore_patch(self, task: StoryTask) -> None:
                 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
+            # Call-site invariant: `_dev_phase` advances the task to DEV_RUNNING
+            # immediately before dispatch, and this runs on that path only — so the
+            # step to DEV_VERIFY is unconditional. It is required because `_escalate`
+            # cannot transition out of DEV_RUNNING directly.
+            advance(task, Phase.DEV_VERIFY)
             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)
 

src/bmad_loop/resolve.py

@@ -23,6 +23,7 @@
 from .adapters.base import SessionSpec
 from .model import RunState
 from .platform_util import safe_segment
+from .runs import validate_restore_latch
 
 RESOLVE_DIR = "resolve"
 
@@ -96,6 +97,16 @@ def _gather_escalations(run_dir: Path, state: RunState, story_key: str) -> list[
 def build_context(state: RunState, run_dir: Path, story_key: str, *, isolation: str = "") -> Path:
     """Write resolve/<story_key>/context.json for the resolve skill to read."""
     task = state.tasks.get(story_key)
+    # Patch-restore availability (#2564): the shared `validate_restore_latch`
+    # verdict, not a local copy of one leg. Any of them — worktree isolation (the
+    # re-drive discards and re-mounts the unit's worktree), a spec-less escalation,
+    # a pre-planning sentinel wedge — means the orchestrator would reject a
+    # `restore_patch` after the session; told to the agent up front so it never
+    # negotiates a restore it can't honor.
+    restore_supported = task is not None and (
+        validate_restore_latch(state, task, story_key, worktree_isolation=isolation == "worktree")
+        is None
+    )
     context = {
         "story_key": story_key,
         "run_id": state.run_id,
@@ -106,11 +117,7 @@ def build_context(state: RunState, run_dir: Path, story_key: str, *, isolation:
         # 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 ""),
+        "restore_supported": restore_supported,
     }
     # 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

src/bmad_loop/runs.py

@@ -13,7 +13,7 @@
 from . import devcontract, verify
 from .adapters.multiplexer import get_multiplexer
 from .journal import STATE_FILE, Journal, load_state, save_state
-from .model import PAUSE_ESCALATION, Phase
+from .model import PAUSE_ESCALATION, Phase, RunState, StoryTask
 from .platform_util import atomic_replace
 from .process_host import get_process_host
 
@@ -506,6 +506,66 @@ class RearmError(Exception):
     """The run/story is not in a re-armable escalation state."""
 
 
+def validate_restore_latch(
+    state: RunState, task: StoryTask, story_key: str, *, worktree_isolation: bool = False
+) -> str | None:
+    """Every precondition an intent-gap patch-restore latch (BMAD-METHOD #2564) must
+    satisfy, in one place. Returns an operator-facing error string, or None to latch.
+
+    The single seam for both entry points: `rearm_escalation` (which performs the
+    latch, and is also reachable programmatically — a TUI restore, a future caller)
+    and `cli._resolve_restore_patch` (which fails fast *before* the interactive
+    resolve session, so an unhonorable restore doesn't cost an agent conversation).
+    Splitting these let a non-CLI caller bypass the worktree half; keeping them here
+    means a caller cannot latch a patch the engine could never honor.
+
+    The CLI knows one thing this cannot: the *live* policy's isolation mode, which
+    may have been edited between escalation and resolve. It passes that as
+    `worktree_isolation`; the recorded `task.worktree_path` (how the unit actually
+    executed) is checked here either way, so both entry points reject a
+    worktree-isolation restore and the CLI additionally catches a policy flip.
+
+    Path resolution and trusted-roots containment stay CLI-side: they need
+    `--project` and the loaded bmad config, neither of which run state carries.
+    """
+    # 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 rearm_escalation's sentinel-clear branch.
+    if state.source == "stories" and task.sentinel_kind:
+        return (
+            f"story {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, 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.
+    if not task.spec_file:
+        return (
+            f"story {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."
+        )
+    # 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.
+    if worktree_isolation or task.worktree_path:
+        return (
+            "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"
+        )
+    return None
+
+
 def rearm_escalation(
     run_dir: Path, story_key: str | None = None, *, restore_patch: str | None = None
 ) -> str:
@@ -545,13 +605,11 @@ def rearm_escalation(
     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.
+    Returns the re-armed story key. Raises RearmError when the run is not paused at
+    the escalation stage, the target story is not escalated, or a supplied
+    `restore_patch` fails `validate_restore_latch` (the shared precondition set —
+    sentinel wedge, spec-less escalation, worktree isolation).
     """
     state = load_state(run_dir)
     if state.paused_stage != PAUSE_ESCALATION:
@@ -567,30 +625,15 @@ def rearm_escalation(
         raise RearmError(f"run {run_dir.name} has no task for story {key}")
     if task.phase != Phase.ESCALATED:
         raise RearmError(f"story {key} is not escalated (phase: {task.phase})")
-    # T1 guard (stories 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."
-        )
+    # Patch-restore preconditions (T1 guard + spec-less wedge + worktree isolation),
+    # rejected here before any task mutation so the escalation stays armed for a
+    # corrected resolve. `cli._resolve_restore_patch` runs the same validator ahead
+    # of the interactive session; this call is what makes a programmatic caller
+    # (TUI restore parity, scripts) unable to bypass it.
+    if restore_patch:
+        err = validate_restore_latch(state, task, key)
+        if err is not None:
+            raise RearmError(err)
 
     journal = Journal(run_dir)
     # Read before the unconditional overwrite below: they describe the restore
@@ -752,9 +795,7 @@ def _stale_restore_residue(
     """
     if not old_latch:
         return set()
-    patch_path = Path(old_latch)
-    if not patch_path.is_absolute():
-        patch_path = repo / patch_path
+    patch_path = verify.resolve_restore_path(old_latch, repo)
 
     residue: set[str] = set()
     try: