fix(adapters,engine,escalation): synthesize marker-less terminal specs + review.on_timeout salvage

CHANGELOG.md

@@ -9,6 +9,17 @@ breaking changes may land in a minor release.
 
 ### Added
 
+- **`review.on_timeout` policy knob (#271).** A timeout-like review verdict (`timeout` /
+  `stalled` / `over_budget`) previously always burned a review cycle (RETRY) until
+  `max_review_cycles`, then deferred — even when the dev product was already finalized and
+  verify-green. New modes: `"retry"` (default, unchanged), `"salvage-if-done"` (commit the
+  verified dev product, reset a mid-review `in-review` interrupt forward, refile the outstanding
+  follow-up recommendation to deferred work — origin `review-timeout-salvage` — and journal
+  `review-timeout-salvage` instead of burning another pass), and `"defer"` (give up on the first
+  timeout-like verdict). A salvaged timeout never re-arms `followup_review_recommended` nor
+  spends a damping grant; env-fault (#194) and `crashed` verdicts keep their own routing in
+  every mode.
+
 - **Transport failures pause instead of burning attempts (#194).** A session whose coding CLI
   lost its API connection stays alive but idle, printing `API Error: Unable to connect …` while it
   idles out the session clock — indistinguishable from a real wall-clock timeout, so two such
@@ -54,6 +65,21 @@ breaking changes may land in a minor release.
   `subprocess.returncode` sees `130` rather than `-2`). A Ctrl+C _during_ a run is unchanged:
   the engine still finalizes it as a resumable `stopped` run (rc `0`).
 
+### Fixed
+
+- **A finished story whose session omitted `## Auto Run Result` no longer livelocks and
+  DEFER-drops (#224).** The review HALT intermittently finalizes the spec's frontmatter to
+  `status: done` without appending the terminal marker the harvest scan keys on; every Stop
+  then read `no-artifact`, stall nudges re-invoked an already-exited workflow (#149), and after
+  `max_review_cycles` the finished, verify-passing work was rolled back. The scan path now
+  synthesizes the result from the authoritative terminal frontmatter (exactly as stories mode
+  already did) once the spec's (path, mtime, status) fingerprint holds stable across two
+  resultless Stops — several candidates refuse to guess — and the post-kill reconcile applies
+  the same fallback on a single sighting once the window is provably dead. Synthesized results
+  carry `synthesized_from_frontmatter` and journal `session-synthesized-from-frontmatter`;
+  new resultless-stop breadcrumb verdicts `terminal-frontmatter-pending` and
+  `ambiguous-frontmatter` record the fingerprint's progress.
+
 ## [0.9.0] — 2026-07-21
 
 ### Added

src/bmad_loop/adapters/generic.py

@@ -39,6 +39,7 @@
 from ..process_host import ProcessHostError, get_process_host
 from ..signals import SignalWatcher
 from ..tokens import read_usage as tally_usage
+from ..verify import read_frontmatter
 from .base import CodingCLIAdapter, SessionHandle, SessionResult, SessionSpec
 from .multiplexer import MultiplexerError, TerminalMultiplexer, get_multiplexer
 from .profile import CLIProfile
@@ -52,6 +53,17 @@
 RESULT_GRACE_S = 15.0
 RESULT_POLL_S = 0.5
 KILL_POLL_S = 0.5
+# Missing-marker fallback (#224): how many consecutive resultless-Stop
+# observations of an IDENTICAL (path, mtime, status) fingerprint a marker-less
+# terminal spec must survive before it is synthesized as this session's result.
+# One observation is not enough: right after a review launch the spec still
+# carries the dev pass's `done` frontmatter, and the review's first write can
+# bump its mtime past the launch floor before the status flips to `in-review` —
+# harvesting on that single sighting would score a review that never ran (#261).
+# Two stable sightings bracket a full stall-grace + nudge with zero writes, which
+# a session mid-edit cannot produce. A dead window skips the counter entirely:
+# the kill settled liveness, so the frontmatter is as final as it will ever get.
+FM_FALLBACK_MIN_OBS = 2
 # Post-mortem transport-failure classification (#194): how much of the tee'd
 # pane log's tail to scan, how long an evidence excerpt to keep, and which
 # non-completed statuses are eligible. over_budget is excluded — a budget
@@ -996,6 +1008,11 @@ def _configure_dev_knobs(self) -> None:
         self._stop_nudges = 0
         self._stall_grace_s = float(self.policy.limits.dev_stall_grace_s)
         self._stall_nudges = int(self.policy.limits.dev_stall_nudges)
+        # Missing-marker fingerprint observations (#224):
+        # task_id -> (path, mtime_ns, frontmatter status, observation count).
+        # Task ids are unique per session, so entries never need resetting
+        # between sessions; the dict lives for the adapter's lifetime.
+        self._fm_fallback_obs: dict[str, tuple[str, int, str, int]] = {}
 
     def _probe_alive(self, handle: SessionHandle) -> bool | None:
         """Liveness of the session's native surface (tmux window, server
@@ -1022,12 +1039,14 @@ def _result_json(self, handle: SessionHandle, spec: SessionSpec, *, wait: bool)
         return sr.result_json if sr is not None else None
 
     def _synth_result(
-        self, handle: SessionHandle, spec: SessionSpec, *, wait: bool
+        self, handle: SessionHandle, spec: SessionSpec, *, wait: bool, dead_window: bool = False
     ) -> devcontract.SynthResult | None:
         # Stories mode (folder+id dispatch): the story spec lives at a
         # deterministic id-keyed path, so resolve it directly instead of the
         # mtime-floor scan. The engine exports BMAD_LOOP_SPEC_FOLDER only for
         # stories runs, so sprint/sweep runs keep the scan path below unchanged.
+        # (dead_window is a scan-path refinement — stories read-back is already
+        # frontmatter-authoritative, so it needs no missing-marker fallback.)
         if spec.env.get("BMAD_LOOP_SPEC_FOLDER"):
             return self._stories_synth_result(handle, spec, wait=wait)
         # Mirror the base _await_result poll: the skill's terminal spec may not be
@@ -1039,26 +1058,126 @@ def _synth_result(
             for artifacts in search_dirs:
                 spec_path = devcontract.find_result_artifact(artifacts, since_ns=handle.launched_ns)
                 if spec_path is not None:
-                    story_key = spec.env.get("BMAD_LOOP_STORY_KEY") or None
-                    # Bundle dev sessions: the orchestrator exports the bundle's
-                    # owned dw ids (the generic skill never authors them). Stamp
-                    # them onto the result so verify_dev_bundle's cross-check passes.
-                    raw_dw_ids = (spec.env.get("BMAD_LOOP_DW_IDS") or "").split(",")
-                    dw_ids = [tok for tok in (i.strip() for i in raw_dw_ids) if tok]
-                    return devcontract.synthesize_result(
-                        spec_path, story_key=story_key, dw_ids=dw_ids or None
-                    )
+                    return self._synthesize_from(spec_path, spec)
             if not wait or time.monotonic() >= deadline:
-                if wait:
-                    self._note_resultless_stop(
-                        handle.task_id,
-                        "no-artifact",
-                        "no result artifact newer than session launch under: "
-                        + ", ".join(str(d) for d in search_dirs),
-                    )
-                return None
+                return self._frontmatter_fallback(
+                    handle, spec, search_dirs, wait=wait, dead_window=dead_window
+                )
             time.sleep(RESULT_POLL_S)
 
+    def _synthesize_from(self, spec_path: Path, spec: SessionSpec) -> devcontract.SynthResult:
+        """Shared synthesis call for the marker scan and the missing-marker
+        fallback, so both stamp the session's story key and — for bundle dev
+        sessions, where the orchestrator exports the bundle's owned dw ids (the
+        generic skill never authors them) — the dw_ids verify_dev_bundle
+        cross-checks."""
+        story_key = spec.env.get("BMAD_LOOP_STORY_KEY") or None
+        raw_dw_ids = (spec.env.get("BMAD_LOOP_DW_IDS") or "").split(",")
+        dw_ids = [tok for tok in (i.strip() for i in raw_dw_ids) if tok]
+        return devcontract.synthesize_result(spec_path, story_key=story_key, dw_ids=dw_ids or None)
+
+    def _frontmatter_fallback(
+        self,
+        handle: SessionHandle,
+        spec: SessionSpec,
+        search_dirs: list[Path],
+        *,
+        wait: bool,
+        dead_window: bool,
+    ) -> devcontract.SynthResult | None:
+        """Missing-marker rescue (#224): synthesize from a spec this session
+        finalized to a terminal frontmatter ``status:`` without appending the
+        ``## Auto Run Result`` marker the scan keys on. Without this, such a
+        spec is invisible to the harvest: every Stop reads ``no-artifact``, the
+        stall nudges re-invoke a skill that has already exited its workflow, and
+        a finished story rides to timeout — where the engine's review RETRY
+        strips the spec and reproduces the omission until the story defers.
+
+        Trust model: a terminal frontmatter under a live window is weaker
+        evidence than the marker, so the live path harvests only a fingerprint
+        (path, mtime, status) that held stable across ``FM_FALLBACK_MIN_OBS``
+        resultless Stops; a dead window (post-kill reconcile) harvests on one
+        sighting, liveness having been settled by the kill. Several candidates
+        mean the scan cannot know which spec is this session's — refuse to
+        guess. Every synthesized result still runs the engine's full
+        deterministic verify downstream, the same #61 trust model as the
+        post-kill rescue. With this in place a marker-less ``done`` spec
+        completes here and never reaches the review-timeout path, so the
+        ``review.on_timeout`` salvage (#271) only ever sees the complementary
+        case: a review that died with a NON-terminal frontmatter.
+
+        Owns the give-up breadcrumb: exactly one of ``no-artifact``,
+        ``ambiguous-frontmatter``, or ``terminal-frontmatter-pending`` per
+        wait=True pass (none on a harvest). A plain wait=False read (the crash
+        path) is compare-only — it may harvest an already-stable fingerprint but
+        never records observations or breadcrumbs.
+        """
+        task_id = handle.task_id
+        candidates: list[Path] = []
+        for artifacts in search_dirs:
+            candidates.extend(
+                devcontract.find_frontmatter_candidates(artifacts, since_ns=handle.launched_ns)
+            )
+        if not candidates:
+            # No marker-less terminal spec either (the common resultless Stop —
+            # e.g. a review that flipped to `in-review` and is mid-work): clear
+            # any stale fingerprint so a later terminal state starts over.
+            self._fm_fallback_obs.pop(task_id, None)
+            if wait:
+                self._note_resultless_stop(
+                    task_id,
+                    "no-artifact",
+                    "no result artifact newer than session launch under: "
+                    + ", ".join(str(d) for d in search_dirs),
+                )
+            return None
+        if len(candidates) > 1:
+            if wait:
+                self._note_resultless_stop(
+                    task_id,
+                    "ambiguous-frontmatter",
+                    f"{len(candidates)} terminal marker-less candidates: "
+                    + ", ".join(str(p) for p in candidates),
+                )
+            return None
+        path = candidates[0]
+        try:
+            mtime_ns = path.stat().st_mtime_ns
+            fm_status = str(read_frontmatter(path).get("status", "")).strip().lower()
+        except OSError:
+            # Torn mid-write read: not evidence of anything — same degrade as
+            # the read-back doctrine everywhere else on this path.
+            if wait:
+                self._note_resultless_stop(
+                    task_id, "no-artifact", f"unreadable marker-less candidate {path}"
+                )
+            return None
+        fingerprint = (str(path), mtime_ns, fm_status)
+        prev = self._fm_fallback_obs.get(task_id)
+        stable = prev is not None and prev[:3] == fingerprint
+        observations = (prev[3] + 1) if (stable and prev is not None) else 1
+        if dead_window or (stable and observations >= FM_FALLBACK_MIN_OBS):
+            sr = self._synthesize_from(path, spec)
+            if sr.result_json is not None:
+                sr.result_json["synthesized_from_frontmatter"] = True
+                self._note_lifecycle(
+                    task_id,
+                    "frontmatter-synthesized",
+                    spec=str(path),
+                    status=fm_status,
+                    dead_window=dead_window,
+                )
+            return sr
+        if wait:
+            self._fm_fallback_obs[task_id] = (*fingerprint, observations)
+            self._note_resultless_stop(
+                task_id,
+                "terminal-frontmatter-pending",
+                f"{path} frontmatter status={fm_status!r} with no '## Auto Run Result'"
+                f" marker; observation {observations}/{FM_FALLBACK_MIN_OBS} before synthesis",
+            )
+        return None
+
     def _stories_synth_result(
         self, handle: SessionHandle, spec: SessionSpec, *, wait: bool
     ) -> devcontract.SynthResult | None:
@@ -1189,7 +1308,11 @@ def _post_kill_reconcile(
         if alive is None:
             return result  # liveness unknowable: unknown is not dead
         try:
-            sr = self._synth_result(handle, spec, wait=False)
+            # dead_window: the probe above settled liveness, so the missing-
+            # marker fallback (#224) may synthesize from a terminal frontmatter
+            # on a single sighting — the gates below still refuse anything but
+            # a self-consistent, escalation-free successful terminal.
+            sr = self._synth_result(handle, spec, wait=False, dead_window=True)
         except (OSError, UnicodeDecodeError):
             # An unreadable artifact is not evidence a session finished. This
             # hook runs right after run()'s finally-kill — the moment a spec the

src/bmad_loop/data/settings/core.toml

@@ -45,6 +45,13 @@ options_ref = "REVIEW_TRIGGER_MODES"
 default_ref = "ReviewPolicy.trigger"
 label = "review trigger"
 description = "recommended: run the 2nd-opinion review only when bmad-dev-auto flags it · always: run it every story (both bounded by limits.max_review_cycles)"
+[[section.field]]
+key = "on_timeout"
+kind = "select"
+options_ref = "REVIEW_ON_TIMEOUT_MODES"
+default_ref = "ReviewPolicy.on_timeout"
+label = "review timeout policy"
+description = "retry: burn a review cycle per timeout (default) · salvage-if-done: commit the verified dev product and refile the follow-up · defer: give up on the first timeout"
 
 [[section]]
 name = "stories"

src/bmad_loop/devcontract.py

@@ -324,6 +324,51 @@ def find_result_artifact(impl_artifacts: Path, *, since_ns: int) -> Path | None:
     return best[1] if best else None
 
 
+def find_frontmatter_candidates(impl_artifacts: Path, *, since_ns: int) -> list[Path]:
+    """Missing-marker fallback scan (#224): specs this session finalized to a
+    terminal frontmatter ``status:`` WITHOUT appending the ``## Auto Run Result``
+    marker `find_result_artifact` keys on. The skill's HALT instructions make the
+    append unconditional, but compliance is intermittent — without this scan such
+    a spec is invisible to the harvest and a finished story rides stall-nudges to
+    timeout, then loses its work to a retry/DEFER cycle.
+
+    A candidate must be modified at/after `since_ns` (same session-launch floor
+    as the marker scan), carry ZERO real (non-fenced) marker headings, not be the
+    no-spec fallback file (that one is already matched by name on the normal
+    path), and have frontmatter ``status:`` of ``done`` or ``blocked``. Returns
+    ALL matches, most-recent first — the caller refuses to guess between several
+    and must apply its own stability fingerprint before synthesizing, because a
+    terminal frontmatter under a live window is weaker evidence than the marker.
+    """
+    if not impl_artifacts.is_dir():
+        return []
+    found: list[tuple[int, Path]] = []
+    for path in impl_artifacts.glob("*.md"):
+        if path.name.startswith(FALLBACK_RESULT_PREFIX):
+            continue
+        try:
+            mtime_ns = path.stat().st_mtime_ns
+        except OSError:
+            continue
+        if mtime_ns < since_ns:
+            continue
+        try:
+            text = path.read_text(encoding="utf-8")
+        except (OSError, UnicodeDecodeError):
+            continue
+        if _section_headings(text):
+            continue  # carries a real marker — the normal scan's territory
+        try:
+            fm = read_frontmatter(path)
+        except OSError:
+            continue
+        if str(fm.get("status", "")).strip().lower() not in (DONE, BLOCKED):
+            continue
+        found.append((mtime_ns, path))
+    found.sort(key=lambda t: t[0], reverse=True)
+    return [p for _, p in found]
+
+
 def reset_spec_status(spec_path: Path, new_status: str) -> bool:
     """Rewrite the frontmatter ``status:`` value of a spec in place.