diff --git a/CHANGELOG.md b/CHANGELOG.md index 23e333ec..64da7b78 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/src/bmad_loop/adapters/generic.py b/src/bmad_loop/adapters/generic.py index ae58c6f1..99be2fc5 100644 --- a/src/bmad_loop/adapters/generic.py +++ b/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 diff --git a/src/bmad_loop/data/settings/core.toml b/src/bmad_loop/data/settings/core.toml index d353de5b..edb8069a 100644 --- a/src/bmad_loop/data/settings/core.toml +++ b/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" diff --git a/src/bmad_loop/devcontract.py b/src/bmad_loop/devcontract.py index 25e9e011..2b2a6121 100644 --- a/src/bmad_loop/devcontract.py +++ b/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. diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index 134af02e..04fa3eba 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -29,6 +29,7 @@ decide_review_session, env_fault_pause_reason, preference_escalations, + review_retry_or_exhaust, ) from .journal import Journal, save_state from .model import ( @@ -1328,6 +1329,27 @@ def _review_and_commit( "review-retry", story_key=task.story_key, reason=decision.reason ) continue + if decision.action == Action.SALVAGE: + # review.on_timeout = "salvage-if-done" (#271): the session hit a + # timeout-like verdict, but the dev product may already be + # finalized and verify-green — converge (commit + refile the + # outstanding follow-up) instead of burning another review cycle + # on an empty delta. When salvage is not applicable, fall back + # through the default retry/exhaust routing. + if self._salvage_review_timeout(task, result): + return + fallback = review_retry_or_exhaust( + task, self.policy, f"{decision.reason}; salvage not applicable" + ) + if fallback.action == Action.PAUSE: + self._escalate(task, fallback.reason) + if fallback.action == Action.DEFER: + self._defer(task, fallback.reason) + return + self.journal.append( + "review-retry", story_key=task.story_key, reason=fallback.reason + ) + continue rj = result.result_json or {} for pref in preference_escalations(rj): @@ -1468,6 +1490,105 @@ def _review_and_commit( self._commit(task) + def _salvage_review_timeout(self, task: StoryTask, result: SessionResult) -> bool: + """``review.on_timeout = "salvage-if-done"`` (#271): try to converge a + timed-out review by committing the already-finalized dev product instead + of burning another review cycle. Returns True when the story committed; + False means salvage was not applicable and the caller falls back to the + default retry/exhaust routing. The cycle the timed-out session charged is + deliberately not refunded — salvage changes what the *next* cycle costs, + not what this one did. + + Applicability, all deterministic: not worktree-isolated (a defer there + already keeps the unit's worktree + diff, and committing into the main + repo would be wrong — same scoping as the budget-exhaustion rescue); a + spec is recorded and its frontmatter reads ``done`` (the review never got + far enough to touch it — rare once the adapter's missing-marker fallback + (#224) completes those sessions, but a review that never wrote the spec + at all still lands here) or ``in-review`` (the mid-review interrupt: the + dying pass flipped the transient marker and died — reset it forward, + stripping any partial terminal section so the next launch's mtime-floor + scan can't misread it). Anything else — ``blocked``, ``in-progress``, a + custom token — was set deliberately or means unfinished dev work: never + salvage over it. The commit is gated on the same authoritative + ``_verify_review`` as every other converge path, so salvage can never + ship unverified work; a timeout that produced no review result neither + re-arms ``followup_review_recommended`` nor spends a damping grant — the + outstanding recommendation is refiled to deferred work instead.""" + if self._isolated or not task.spec_file: + return False + spec_path = Path(task.spec_file) + fm = self._observed_frontmatter(spec_path, task.story_key, "review-timeout-salvage") + if fm is None: + return False + fm_status = str(fm.get("status", "")).strip().lower() + if fm_status not in ("done", "in-review"): + return False + reset_from: str | None = None + if fm_status == "in-review": + # Repair-write doctrine: these raise on an unreadable spec rather + # than silently proceeding stale (see _reset_spec_for_repair). + reset_from = fm_status + devcontract.reset_spec_status(spec_path, "done") + devcontract.strip_auto_run_result(spec_path) + outcome = self._verify_review(task) + if not outcome.ok: + self.journal.append( + "review-timeout-salvage-failed", + story_key=task.story_key, + cycle=task.review_cycle, + reason=outcome.reason, + env_fault=outcome.env_fault, + ) + if not outcome.retryable: + # escalate-grade failure (environment fault, git error): another + # review cycle would replay it — pause the run (mirrors the + # review loop's own verify-failed routing). + self._escalate(task, outcome.reason) + return False + refiled: str | None = None + if task.followup_review_recommended: + # Refile BEFORE _commit so the ledger edit squashes into the story + # commit (mirrors _record_review_budget_followup's ordering). A new + # origin string: the review-budget-followup origin's wording and + # re-review cap are load-bearing for that path and must not blur. + refiled = deferredwork.append_entry( + self.workspace.paths.deferred_work, + title=( + f"Follow-up review still outstanding for {task.story_key}" + " after a review timeout" + ), + origin="review-timeout-salvage", + source_spec=spec_path.name if task.spec_file else task.story_key, + reason=( + f"The review session ended {result.status} with the story already " + f"finalized (status: done, verify green). Per review.on_timeout = " + f"'salvage-if-done' the work was committed by bmad-loop run " + f"{self.state.run_id} without another review pass; this entry " + f"preserves the outstanding follow-up recommendation for a " + f"deliberate later review." + ), + severity="low", + ) + task.followup_review_recommended = False + self.journal.append( + "review-timeout-salvage", + story_key=task.story_key, + cycle=task.review_cycle, + session_status=result.status, + reset_from=reset_from, + refiled=refiled, + ) + gates.notify( + self.policy, + self.run_dir, + f"review timeout salvaged, work committed: {task.story_key}", + f"review session {result.status}; the finalized, verify-green dev product " + f"was committed and any outstanding follow-up refiled to deferred work.", + ) + self._commit(task) + return True + def _skip_review_and_commit(self, task: StoryTask) -> None: """review.enabled = false: no separate review session runs. The bmad-dev-auto session ran its own inline review and finalized the @@ -2020,6 +2141,14 @@ def _run_session( # completion in the journal; leave a breadcrumb for forensics. if result.result_json is not None and result.result_json.get("post_kill_reconciled"): self.journal.append("session-rescued-post-kill", task_id=task_id, role=role) + # Same forensics need for a missing-marker synthesis (#224): the + # result is real, but the marker-append the skill owes was skipped. + if result.result_json is not None and result.result_json.get( + "synthesized_from_frontmatter" + ): + self.journal.append( + "session-synthesized-from-frontmatter", task_id=task_id, role=role + ) # Only dev/review sessions are resumable — `_resumable_session` matches # exactly those task ids under DEV_RUNNING/REVIEW_RUNNING. For everything # else (triage/sweep, labeled plugin-workflow sessions) the payload is diff --git a/src/bmad_loop/escalation.py b/src/bmad_loop/escalation.py index 998e06f1..af68d249 100644 --- a/src/bmad_loop/escalation.py +++ b/src/bmad_loop/escalation.py @@ -24,6 +24,18 @@ class Action(StrEnum): RETRY = "retry" DEFER = "defer" PAUSE = "pause" + # review.on_timeout = "salvage-if-done" only (#271): the engine attempts to + # commit the already-finalized dev product instead of burning another review + # cycle; when salvage is not applicable it falls back through + # `review_retry_or_exhaust`. Produced only by `decide_review_session`. + SALVAGE = "salvage" + + +# Timeout-like review verdicts review.on_timeout governs (#271): deliberately the +# same set `_post_kill_reconcile` treats as rescue-eligible. `crashed` is excluded +# — a hard window death is cheap to retry and already honors on-disk artifacts via +# the crash-path read-back — and env-fault (#194) short-circuits before this. +REVIEW_TIMEOUT_STATUSES = frozenset({"timeout", "stalled", "over_budget"}) @dataclass(frozen=True) @@ -117,7 +129,6 @@ def decide_review_session(task: StoryTask, result: SessionResult, policy: Policy details = "; ".join(str(e.get("detail", e.get("type", "?"))) for e in crits) return Decision(Action.PAUSE, f"CRITICAL escalation from review session: {details}") - budget_left = task.review_cycle < policy.limits.max_review_cycles if result.status != "completed": if result.env_fault: # transport/API failure (#194): pause rather than charge a review @@ -127,12 +138,29 @@ def decide_review_session(task: StoryTask, result: SessionResult, policy: Policy env_fault_pause_reason("review", result), ) reason = f"review session {result.status}" - if budget_left: - return Decision(Action.RETRY, reason) - return Decision(_exhausted_action(task), _exhaust_reason(task, reason)) + if result.status in REVIEW_TIMEOUT_STATUSES: + mode = policy.review.on_timeout + if mode == "defer": + return Decision( + _exhausted_action(task), + _exhaust_reason(task, f"{reason} (review.on_timeout=defer)"), + ) + if mode == "salvage-if-done": + return Decision(Action.SALVAGE, reason) + return review_retry_or_exhaust(task, policy, reason) return Decision(Action.PROCEED) +def review_retry_or_exhaust(task: StoryTask, policy: Policy, reason: str) -> Decision: + """The default routing for a failed review session: RETRY while the outer + cycle budget lasts, then plateau-defer (or re-escalate mid re-drive). Module- + level so the engine can fall back through it when a SALVAGE attempt turns out + not to be applicable (#271).""" + if task.review_cycle < policy.limits.max_review_cycles: + return Decision(Action.RETRY, reason) + return Decision(_exhausted_action(task), _exhaust_reason(task, reason)) + + def _exhausted_action(task: StoryTask) -> Action: """What a budget-exhausted, non-CRITICAL failure resolves to. Normally a plateau-defer (skip the story, keep the run alive). But a story mid re-drive diff --git a/src/bmad_loop/policy.py b/src/bmad_loop/policy.py index c6366713..81f9f364 100644 --- a/src/bmad_loop/policy.py +++ b/src/bmad_loop/policy.py @@ -28,6 +28,7 @@ SESSION_BUDGET_MODES = {"off", "warn", "enforce"} SWEEP_AUTO_MODES = {"never", "per-epic", "run-end"} REVIEW_TRIGGER_MODES = {"always", "recommended"} +REVIEW_ON_TIMEOUT_MODES = {"retry", "salvage-if-done", "defer"} # Where the run gets its story queue. "sprint-status" (default) is the classic # flow — bmad-sprint-planning writes sprint-status.yaml from prose epics. # "stories" is the opt-in folder+id dispatch flow (BMAD-METHOD #2549): a typed, @@ -181,6 +182,18 @@ class ReviewPolicy: # recommending an independent follow-up — by converging + refiling once the # damping grant is spent instead of looping to the outer cap. trigger: str = "recommended" + # What a timeout-like review verdict (timeout / stalled / over_budget — the + # same set the post-kill reconcile treats as rescue-eligible) costs (#271): + # "retry" (default) — today's behavior: burn a review cycle per timeout + # until limits.max_review_cycles, then defer. + # "salvage-if-done" — when the spec's frontmatter shows the dev product + # already finalized (`done`, or the `in-review` mid-review interrupt, + # reset forward) and the deterministic verify gate passes, commit the + # work and refile any outstanding follow-up review to deferred work + # instead of burning another full review pass on an empty delta. + # "defer" — give up on the first timeout-like verdict (no retries). + # `crashed` and env-fault (#194) verdicts keep their own routing in every mode. + on_timeout: str = "retry" @dataclass(frozen=True) @@ -752,11 +765,17 @@ def loads(text: str, plugin_schemas: dict[str, Any] | None = None) -> Policy: review = ReviewPolicy( enabled=bool(review_d.get("enabled", ReviewPolicy.enabled)), trigger=str(review_d.get("trigger", ReviewPolicy.trigger)).strip(), + on_timeout=str(review_d.get("on_timeout", ReviewPolicy.on_timeout)).strip(), ) if review.trigger not in REVIEW_TRIGGER_MODES: raise PolicyError( f"review.trigger must be one of {sorted(REVIEW_TRIGGER_MODES)}: got {review.trigger!r}" ) + if review.on_timeout not in REVIEW_ON_TIMEOUT_MODES: + raise PolicyError( + f"review.on_timeout must be one of {sorted(REVIEW_ON_TIMEOUT_MODES)}:" + f" got {review.on_timeout!r}" + ) stories = StoriesPolicy( source=str(stories_d.get("source", StoriesPolicy.source)).strip(), spec_folder=str(stories_d.get("spec_folder", StoriesPolicy.spec_folder)).strip(), @@ -1009,6 +1028,12 @@ def _fold_deprecated_engine( # limits.max_followup_reviews (a round that finalizes the story yet keeps # recommending a follow-up converges + refiles once the grant is spent) either way. trigger = "recommended" +# What a timeout-like review verdict (timeout/stalled/over_budget) costs: +# "retry" -> burn a review cycle per timeout until max_review_cycles (default). +# "salvage-if-done" -> if the dev product is already finalized and verify passes, +# commit it and refile the outstanding follow-up to deferred work. +# "defer" -> give up on the first timeout-like verdict. +on_timeout = "retry" [stories] # Story-queue source. "sprint-status" (default) walks sprint-status.yaml written diff --git a/tests/test_devcontract.py b/tests/test_devcontract.py index b41d3f81..c06b85e8 100644 --- a/tests/test_devcontract.py +++ b/tests/test_devcontract.py @@ -715,3 +715,72 @@ def test_strip_auto_run_result_ignores_heading_in_mismatched_fence_char(tmp_path sp.write_text(original, encoding="utf-8") assert devcontract.strip_auto_run_result(sp) is False assert sp.read_text() == original + + +# ------------------------------- find_frontmatter_candidates (#224) +# +# The missing-marker fallback scan: terminal-frontmatter specs with NO real +# `## Auto Run Result` heading, written at/after the session-launch floor. + + +def _write(dirpath: Path, name: str, text: str, mtime_ns: int | None = None) -> Path: + path = dirpath / name + path.write_text(text, encoding="utf-8") + if mtime_ns is not None: + os.utime(path, ns=(mtime_ns, mtime_ns)) + return path + + +def test_frontmatter_candidates_finds_done_and_blocked(tmp_path): + _write(tmp_path, "spec-a.md", "---\nstatus: done\n---\n\nbody\n", 2_000) + _write(tmp_path, "spec-b.md", "---\nstatus: blocked\n---\n\nbody\n", 3_000) + found = devcontract.find_frontmatter_candidates(tmp_path, since_ns=0) + # most-recent first + assert [p.name for p in found] == ["spec-b.md", "spec-a.md"] + + +def test_frontmatter_candidates_excludes_marker_bearing_spec(tmp_path): + _write( + tmp_path, + "spec-a.md", + "---\nstatus: done\n---\n\n## Auto Run Result\n\nStatus: done\n", + ) + assert devcontract.find_frontmatter_candidates(tmp_path, since_ns=0) == [] + + +def test_frontmatter_candidates_includes_fence_quoted_heading_only(tmp_path): + """A heading quoted inside a fence is documentation (#52) — the spec has no + REAL marker, so it IS a missing-marker candidate.""" + p = _write( + tmp_path, + "spec-a.md", + "---\nstatus: done\n---\n\n```\n## Auto Run Result\n\nStatus: done\n```\n", + ) + assert devcontract.find_frontmatter_candidates(tmp_path, since_ns=0) == [p] + + +def test_frontmatter_candidates_excludes_no_spec_fallback_file(tmp_path): + """bmad-dev-auto-result-*.md is the skill's no-spec fallback — matched by + name on the normal scan, so the fallback scan must not double-claim it.""" + _write(tmp_path, "bmad-dev-auto-result-x.md", "---\nstatus: done\n---\n\nbody\n") + assert devcontract.find_frontmatter_candidates(tmp_path, since_ns=0) == [] + + +def test_frontmatter_candidates_enforces_mtime_floor(tmp_path): + _write(tmp_path, "spec-a.md", "---\nstatus: done\n---\n\nbody\n", 1_000) + assert devcontract.find_frontmatter_candidates(tmp_path, since_ns=2_000) == [] + + +def test_frontmatter_candidates_excludes_non_terminal_status(tmp_path): + for status in ("draft", "in-progress", "in-review", "ready-for-dev", ""): + _write(tmp_path, "spec-a.md", f"---\nstatus: {status}\n---\n\nbody\n") + assert devcontract.find_frontmatter_candidates(tmp_path, since_ns=0) == [] + + +def test_frontmatter_candidates_skips_unreadable_file(tmp_path): + (tmp_path / "spec-a.md").write_bytes(b"\xff\xfe\x00\x01 not utf-8 \x80\x81") + assert devcontract.find_frontmatter_candidates(tmp_path, since_ns=0) == [] + + +def test_frontmatter_candidates_missing_dir_is_empty(tmp_path): + assert devcontract.find_frontmatter_candidates(tmp_path / "nope", since_ns=0) == [] diff --git a/tests/test_engine.py b/tests/test_engine.py index eebaf387..96441a89 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -45,6 +45,7 @@ LimitsPolicy, NotifyPolicy, Policy, + ReviewPolicy, ScmPolicy, StageAdapterPolicy, SweepPolicy, @@ -5603,3 +5604,166 @@ def crashing_emit(stage, *args, **kwargs): assert [s.role for s in adapter.sessions] == ["review"] # only the in-flight review ran stops = [e for e in resumed.journal.entries() if e["kind"] == "run-stop"] assert stops and stops[-1]["graceful"] is True + + +# ------------------------------- review.on_timeout = "salvage-if-done" (#271) + + +def _salvage_policy(**limits_kw) -> Policy: + return Policy( + gates=GatesPolicy(mode="none"), + notify=QUIET, + scm=ScmPolicy(rollback_on_failure=True), + limits=LimitsPolicy(**limits_kw), + review=ReviewPolicy(on_timeout="salvage-if-done"), + ) + + +def test_review_timeout_salvage_commits_and_refiles(project): + """A review timeout over an already-finalized, verify-green dev product + converges: the work commits, the outstanding follow-up refiles to deferred + work, and no further review cycle burns.""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + engine, _ = make_engine( + project, + [dev_effect(project, "1-1-a"), SessionResult(status="timeout")], + policy=_salvage_policy(), + ) + summary = engine.run() + + assert summary.done == 1 and summary.deferred == 0 + task = engine.state.tasks["1-1-a"] + (salvage,) = [e for e in engine.journal.entries() if e["kind"] == "review-timeout-salvage"] + assert salvage["session_status"] == "timeout" + assert salvage["cycle"] == 1 + assert salvage["reset_from"] is None # the review never touched the done spec + assert salvage["refiled"] and salvage["refiled"].startswith("DW-") + ledger = project.deferred_work.read_text(encoding="utf-8") + assert "origin: review-timeout-salvage" in ledger + assert "1-1-a" in ledger + # the timeout neither re-arms the recommendation nor spends a damping grant + assert task.followup_review_recommended is False + assert task.followup_reviews_spent == 0 + assert not [e for e in engine.journal.entries() if e["kind"] == "review-retry"] + + +def test_review_timeout_salvage_resets_in_review_spec(project): + """The mid-review interrupt: the dying pass flipped the transient in-review + marker. Salvage resets it forward to done and commits.""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + + def timeout_mid_review(spec): + sp = spec_path(project, "1-1-a") + write_spec(sp, "in-review", _spec_baseline(sp)) + return SessionResult(status="timeout") + + engine, _ = make_engine( + project, + [dev_effect(project, "1-1-a"), timeout_mid_review], + policy=_salvage_policy(), + ) + summary = engine.run() + + assert summary.done == 1 + (salvage,) = [e for e in engine.journal.entries() if e["kind"] == "review-timeout-salvage"] + assert salvage["reset_from"] == "in-review" + fm = read_frontmatter(spec_path(project, "1-1-a")) + assert str(fm.get("status")) == "done" + + +def test_review_timeout_salvage_verify_fail_falls_back(project): + """A failing verify gate refuses the salvage (journaled) and falls back to + the default retry/exhaust routing — never ships unverified work.""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + # green at dev-verify time; the dying review breaks it, so only the salvage + # gate (not the dev gate) sees the failure + marker = project.project / "verify-marker.txt" + marker.write_text("ok", encoding="utf-8") + policy = dataclasses.replace( + _salvage_policy(max_review_cycles=1), + verify=VerifyPolicy(commands=(_file_exists_cmd(marker),)), + ) + + def timeout_and_break_verify(spec): + marker.unlink() + return SessionResult(status="timeout") + + engine, _ = make_engine( + project, + [dev_effect(project, "1-1-a"), timeout_and_break_verify], + policy=policy, + ) + summary = engine.run() + + assert summary.deferred == 1 and summary.done == 0 + (failed,) = [ + e for e in engine.journal.entries() if e["kind"] == "review-timeout-salvage-failed" + ] + assert failed["cycle"] == 1 + task = engine.state.tasks["1-1-a"] + assert "salvage not applicable" in task.defer_reason + assert not [e for e in engine.journal.entries() if e["kind"] == "review-timeout-salvage"] + + +def test_review_timeout_salvage_nonterminal_spec_falls_back(project): + """A spec at a non-terminal status (unfinished dev work) is never salvaged + over — no salvage event at all, straight to the fallback routing.""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + + def timeout_mid_work(spec): + sp = spec_path(project, "1-1-a") + write_spec(sp, "in-progress", _spec_baseline(sp)) + return SessionResult(status="timeout") + + engine, _ = make_engine( + project, + [dev_effect(project, "1-1-a"), timeout_mid_work], + policy=_salvage_policy(max_review_cycles=1), + ) + summary = engine.run() + + assert summary.deferred == 1 + kinds = {e["kind"] for e in engine.journal.entries()} + assert "review-timeout-salvage" not in kinds + assert "review-timeout-salvage-failed" not in kinds + assert "salvage not applicable" in engine.state.tasks["1-1-a"].defer_reason + + +def test_review_timeout_salvage_skipped_under_isolation(project): + """Worktree isolation: a defer already preserves the unit's worktree + diff, + and committing into the main repo would be wrong — salvage never applies.""" + policy = dataclasses.replace( + _salvage_policy(), scm=ScmPolicy(rollback_on_failure=True, isolation="worktree") + ) + engine, _ = make_engine(project, [], policy=policy) + task = StoryTask(story_key="1-1-a", epic=1, spec_file=str(spec_path(project, "1-1-a"))) + assert engine._salvage_review_timeout(task, SessionResult(status="timeout")) is False + + +def test_review_timeout_salvage_requires_spec_file(project): + engine, _ = make_engine(project, [], policy=_salvage_policy()) + task = StoryTask(story_key="1-1-a", epic=1) # no spec recorded yet + assert engine._salvage_review_timeout(task, SessionResult(status="timeout")) is False + + +def test_session_synthesized_from_frontmatter_journaled(project): + """The adapter's missing-marker synthesis flag (#224) leaves the same style + of forensic breadcrumb as a post-kill rescue.""" + write_sprint(project, {"1-1-a": "ready-for-dev"}) + inner = dev_effect(project, "1-1-a") + + def synthesized_dev(spec): + result = inner(spec) + result.result_json["synthesized_from_frontmatter"] = True + return result + + engine, _ = make_engine( + project, + [synthesized_dev, review_effect(project, "1-1-a", clean=True)], + ) + summary = engine.run() + assert summary.done == 1 + (crumb,) = [ + e for e in engine.journal.entries() if e["kind"] == "session-synthesized-from-frontmatter" + ] + assert crumb["role"] == "dev" diff --git a/tests/test_escalation.py b/tests/test_escalation.py index 0cbc64b3..2c6ee8d5 100644 --- a/tests/test_escalation.py +++ b/tests/test_escalation.py @@ -2,9 +2,14 @@ resolved-escalation guard that re-escalates instead of silently deferring.""" from bmad_loop.adapters.base import SessionResult -from bmad_loop.escalation import Action, decide_dev, decide_review_session +from bmad_loop.escalation import ( + Action, + decide_dev, + decide_review_session, + review_retry_or_exhaust, +) from bmad_loop.model import StoryTask -from bmad_loop.policy import LimitsPolicy, NotifyPolicy, Policy +from bmad_loop.policy import LimitsPolicy, NotifyPolicy, Policy, ReviewPolicy from bmad_loop.verify import VerifyOutcome POLICY = Policy( @@ -121,3 +126,92 @@ def test_review_exhausted_reescalates_resolved_redrive(): decision = decide_review_session(task, crashed, POLICY) assert decision.action == Action.PAUSE assert "re-escalating instead of deferring" in decision.reason + + +# ------------------------------- review.on_timeout routing (#271) + + +def _policy(on_timeout: str) -> Policy: + return Policy( + limits=LimitsPolicy(max_dev_attempts=2, max_review_cycles=2), + notify=NotifyPolicy(desktop=False, file=True), + review=ReviewPolicy(on_timeout=on_timeout), + ) + + +def test_review_timeout_default_retry_matches_legacy_decisions(): + """on_timeout="retry" (the default) is byte-compatible with the pre-knob + routing for every timeout-like status.""" + for status in ("timeout", "stalled", "over_budget"): + result = SessionResult(status=status) + with_budget = decide_review_session(_task(review_cycle=1), result, _policy("retry")) + assert with_budget.action == Action.RETRY + assert with_budget.reason == f"review session {status}" + spent = decide_review_session(_task(review_cycle=2), result, _policy("retry")) + assert spent.action == Action.DEFER + + +def test_review_timeout_salvage_mode_routes_to_salvage(): + for status in ("timeout", "stalled", "over_budget"): + result = SessionResult(status=status) + # budget state is irrelevant: the engine owns the fallback routing + for cycle in (1, 2): + decision = decide_review_session( + _task(review_cycle=cycle), result, _policy("salvage-if-done") + ) + assert decision.action == Action.SALVAGE + assert decision.reason == f"review session {status}" + + +def test_review_timeout_defer_mode_gives_up_immediately(): + decision = decide_review_session( + _task(review_cycle=1), SessionResult(status="timeout"), _policy("defer") + ) + assert decision.action == Action.DEFER + assert "review.on_timeout=defer" in decision.reason + + +def test_review_timeout_defer_mode_reescalates_resolved_redrive(): + """The resolved_redrive latch outranks the defer mode — same contract as + budget exhaustion (never silently downgrade a human's correction).""" + decision = decide_review_session( + _task(review_cycle=1, resolved_redrive=True), + SessionResult(status="timeout"), + _policy("defer"), + ) + assert decision.action == Action.PAUSE + assert "re-escalating instead of deferring" in decision.reason + + +def test_review_crashed_unaffected_by_on_timeout_modes(): + """crashed is not a timeout-like verdict: every mode keeps the default + retry/exhaust routing for it.""" + crashed = SessionResult(status="crashed") + for mode in ("retry", "salvage-if-done", "defer"): + assert decide_review_session(_task(review_cycle=1), crashed, _policy(mode)).action == ( + Action.RETRY + ) + assert decide_review_session(_task(review_cycle=2), crashed, _policy(mode)).action == ( + Action.DEFER + ) + + +def test_review_env_fault_pauses_under_every_on_timeout_mode(): + """env-fault (#194) short-circuits before the on_timeout branch.""" + env_fault = SessionResult(status="timeout", env_fault=True) + for mode in ("retry", "salvage-if-done", "defer"): + decision = decide_review_session(_task(review_cycle=1), env_fault, _policy(mode)) + assert decision.action == Action.PAUSE + assert "environment fault" in decision.reason + + +def test_review_completed_proceeds_under_every_on_timeout_mode(): + for mode in ("retry", "salvage-if-done", "defer"): + assert decide_review_session(_task(), COMPLETED, _policy(mode)).action == Action.PROCEED + + +def test_review_retry_or_exhaust_helper_matches_budget_semantics(): + assert review_retry_or_exhaust(_task(review_cycle=1), POLICY, "r").action == Action.RETRY + assert review_retry_or_exhaust(_task(review_cycle=2), POLICY, "r").action == Action.DEFER + latched = review_retry_or_exhaust(_task(review_cycle=2, resolved_redrive=True), POLICY, "r") + assert latched.action == Action.PAUSE diff --git a/tests/test_generic_tmux.py b/tests/test_generic_tmux.py index 5f93defd..d3dde99f 100644 --- a/tests/test_generic_tmux.py +++ b/tests/test_generic_tmux.py @@ -9,6 +9,7 @@ import dataclasses import json +import os import shutil import subprocess import sys @@ -2245,7 +2246,7 @@ def test_post_kill_reconcile_synth_read_error_keeps_stall(tmp_path, monkeypatch) (impl / "spec-3-1-foo.md").write_text(_DONE_SPEC) for exc in (OSError("I/O error"), UnicodeDecodeError("utf-8", b"\xff", 0, 1, "invalid")): - def raising(handle, spec, *, wait, _exc=exc): + def raising(handle, spec, *, wait, dead_window=False, _exc=exc): raise _exc monkeypatch.setattr(adapter, "_synth_result", raising) @@ -2938,3 +2939,196 @@ def test_tmux_timeout_with_flushed_spec_rescued_post_kill(tmp_path): assert result.status == "completed" assert result.result_json["status"] == "done" assert result.result_json["post_kill_reconciled"] is True + + +# ------------------------------- missing-marker fallback (#224) +# +# A session (in practice: the follow-up review leg) can finalize the spec's +# frontmatter to a terminal status while omitting the `## Auto Run Result` +# marker find_result_artifact keys on. The scan-path fallback synthesizes from +# the frontmatter once the fingerprint holds stable across FM_FALLBACK_MIN_OBS +# resultless Stops (live), or on a single sighting under a dead window +# (post-kill reconcile). + +_MARKERLESS_DONE = "---\nstatus: done\nbaseline_revision: abc123\n---\n\n# Story\n\nAll done.\n" +_MARKERLESS_BLOCKED = "---\nstatus: blocked\n---\n\n# Story\n\nStuck.\n" + + +def test_frontmatter_fallback_synthesizes_on_second_stable_stop(tmp_path, monkeypatch): + adapter, impl = make_dev_adapter(tmp_path) + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + (impl / "spec-3-1-foo.md").write_text(_MARKERLESS_DONE) + # first resultless Stop: observation recorded, no harvest yet + assert adapter._result_json(_dev_handle(), _dev_spec(tmp_path), wait=True) is None + (crumb,) = _breadcrumbs(adapter) + assert crumb["verdict"] == "terminal-frontmatter-pending" + assert "spec-3-1-foo.md" in crumb["detail"] + # second Stop over the identical (path, mtime, status) fingerprint: harvest + rj = adapter._result_json(_dev_handle(), _dev_spec(tmp_path), wait=True) + assert rj["status"] == "done" + assert rj["workflow"] == "auto-dev" + assert rj["baseline_commit"] == "abc123" + assert rj["synthesized_from_frontmatter"] is True + assert rj["escalations"] == [] + # the harvest pass writes no breadcrumb + assert len(_breadcrumbs(adapter)) == 1 + + +def test_frontmatter_fallback_stamps_story_key_and_dw_ids(tmp_path, monkeypatch): + """The fallback shares _synthesize_from with the marker path, so bundle dev + sessions get their exported dw ids stamped for verify_dev_bundle.""" + adapter, impl = make_dev_adapter(tmp_path) + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + (impl / "spec-3-1-foo.md").write_text(_MARKERLESS_DONE) + spec = dataclasses.replace( + _dev_spec(tmp_path), + env={"BMAD_LOOP_STORY_KEY": "3-1", "BMAD_LOOP_DW_IDS": "DW-7, DW-9"}, + ) + assert adapter._result_json(_dev_handle(), spec, wait=True) is None + rj = adapter._result_json(_dev_handle(), spec, wait=True) + assert rj["story_key"] == "3-1" + assert rj["dw_ids"] == ["DW-7", "DW-9"] + + +def test_frontmatter_fallback_mtime_bump_resets_counter(tmp_path, monkeypatch): + """A spec still being written (the premature-harvest hazard: review launched + on a done spec, first edit bumped mtime before the in-review flip) must not + be harvested — any fingerprint change restarts the count.""" + adapter, impl = make_dev_adapter(tmp_path) + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + spec_file = impl / "spec-3-1-foo.md" + spec_file.write_text(_MARKERLESS_DONE) + os.utime(spec_file, ns=(1_000_000_000, 1_000_000_000)) + assert adapter._result_json(_dev_handle(), _dev_spec(tmp_path), wait=True) is None + os.utime(spec_file, ns=(2_000_000_000, 2_000_000_000)) # the session wrote again + assert adapter._result_json(_dev_handle(), _dev_spec(tmp_path), wait=True) is None + # now stable across two Stops -> harvest on the third call + rj = adapter._result_json(_dev_handle(), _dev_spec(tmp_path), wait=True) + assert rj["synthesized_from_frontmatter"] is True + verdicts = [c["verdict"] for c in _breadcrumbs(adapter)] + assert verdicts == ["terminal-frontmatter-pending", "terminal-frontmatter-pending"] + + +def test_frontmatter_fallback_status_flip_clears_observations(tmp_path, monkeypatch): + """done -> in-review (a review actually running) drops the candidate AND the + recorded fingerprint, so a later terminal state starts the count over.""" + adapter, impl = make_dev_adapter(tmp_path) + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + spec_file = impl / "spec-3-1-foo.md" + spec_file.write_text(_MARKERLESS_DONE) + os.utime(spec_file, ns=(1_000_000_000, 1_000_000_000)) + assert adapter._result_json(_dev_handle(), _dev_spec(tmp_path), wait=True) is None + spec_file.write_text("---\nstatus: in-review\n---\n\n# Story\n") + assert adapter._result_json(_dev_handle(), _dev_spec(tmp_path), wait=True) is None + assert adapter._fm_fallback_obs == {} + spec_file.write_text(_MARKERLESS_DONE) + os.utime(spec_file, ns=(3_000_000_000, 3_000_000_000)) + # back at one observation: not harvested yet + assert adapter._result_json(_dev_handle(), _dev_spec(tmp_path), wait=True) is None + rj = adapter._result_json(_dev_handle(), _dev_spec(tmp_path), wait=True) + assert rj["synthesized_from_frontmatter"] is True + + +def test_frontmatter_fallback_blocked_synthesizes_critical(tmp_path, monkeypatch): + """A marker-less blocked terminal synthesizes the same CRITICAL escalation + stories mode produces, routing decide_dev/decide_review_session to PAUSE.""" + adapter, impl = make_dev_adapter(tmp_path) + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + (impl / "spec-3-1-foo.md").write_text(_MARKERLESS_BLOCKED) + assert adapter._result_json(_dev_handle(), _dev_spec(tmp_path), wait=True) is None + rj = adapter._result_json(_dev_handle(), _dev_spec(tmp_path), wait=True) + assert rj["status"] == "blocked" + assert rj["synthesized_from_frontmatter"] is True + (esc,) = rj["escalations"] + assert esc["severity"] == "CRITICAL" + + +def test_frontmatter_fallback_ambiguous_never_harvests(tmp_path, monkeypatch): + adapter, impl = make_dev_adapter(tmp_path) + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + (impl / "spec-3-1-foo.md").write_text(_MARKERLESS_DONE) + (impl / "spec-3-2-bar.md").write_text(_MARKERLESS_DONE) + for _ in range(3): + assert adapter._result_json(_dev_handle(), _dev_spec(tmp_path), wait=True) is None + verdicts = {c["verdict"] for c in _breadcrumbs(adapter)} + assert verdicts == {"ambiguous-frontmatter"} + assert "2 terminal marker-less candidates" in _breadcrumbs(adapter)[0]["detail"] + + +def test_frontmatter_fallback_pre_launch_spec_is_no_artifact(tmp_path, monkeypatch): + """A marker-less terminal spec older than the session launch is prior state, + not this session's output — the plain no-artifact verdict stands.""" + adapter, impl = make_dev_adapter(tmp_path) + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + (impl / "spec-3-1-foo.md").write_text(_MARKERLESS_DONE) + handle = _dev_handle(launched_ns=time.time_ns() + 10**12) + assert adapter._result_json(handle, _dev_spec(tmp_path), wait=True) is None + (crumb,) = _breadcrumbs(adapter) + assert crumb["verdict"] == "no-artifact" + + +def test_frontmatter_fallback_wait_false_is_compare_only(tmp_path, monkeypatch): + """The crash path's read-once (wait=False, live window) neither records + observations nor writes breadcrumbs; it may only harvest a fingerprint the + live loop already saw and that still matches.""" + adapter, impl = make_dev_adapter(tmp_path) + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + spec_file = impl / "spec-3-1-foo.md" + spec_file.write_text(_MARKERLESS_DONE) + os.utime(spec_file, ns=(1_000_000_000, 1_000_000_000)) + # no prior observation: nothing harvested, nothing recorded, no crumb + assert adapter._result_json(_dev_handle(), _dev_spec(tmp_path), wait=False) is None + assert adapter._fm_fallback_obs == {} + assert _breadcrumbs(adapter) == [] + # one live observation, then the crash read over the unchanged state harvests + assert adapter._result_json(_dev_handle(), _dev_spec(tmp_path), wait=True) is None + rj = adapter._result_json(_dev_handle(), _dev_spec(tmp_path), wait=False) + assert rj["synthesized_from_frontmatter"] is True + + +def test_post_kill_reconcile_rescues_markerless_done_spec(tmp_path): + """The #224 backstop: dead window + terminal marker-less frontmatter rescues + on a single sighting — no live observations required.""" + adapter, impl = make_dev_adapter(tmp_path) + adapter._window_alive = lambda handle: False + (impl / "spec-3-1-foo.md").write_text(_MARKERLESS_DONE) + result = adapter._post_kill_reconcile(_dev_handle(), _dev_spec(tmp_path), _unvouched()) + assert result.status == "completed" + assert result.result_json["status"] == "done" + assert result.result_json["post_kill_reconciled"] is True + assert result.result_json["synthesized_from_frontmatter"] is True + + +def test_post_kill_reconcile_markerless_blocked_keeps_verdict(tmp_path): + """The post-kill done-only gate still refuses a blocked synthesis: blocked + carries no finished work, marker or no marker.""" + adapter, impl = make_dev_adapter(tmp_path) + adapter._window_alive = lambda handle: False + (impl / "spec-3-1-foo.md").write_text(_MARKERLESS_BLOCKED) + original = _unvouched() + assert adapter._post_kill_reconcile(_dev_handle(), _dev_spec(tmp_path), original) is original + + +def test_post_kill_reconcile_markerless_ambiguous_keeps_verdict(tmp_path): + adapter, impl = make_dev_adapter(tmp_path) + adapter._window_alive = lambda handle: False + (impl / "spec-3-1-foo.md").write_text(_MARKERLESS_DONE) + (impl / "spec-3-2-bar.md").write_text(_MARKERLESS_DONE) + original = _unvouched() + assert adapter._post_kill_reconcile(_dev_handle(), _dev_spec(tmp_path), original) is original + + +def test_wait_loop_completes_markerless_spec_on_second_stop(tmp_path, monkeypatch): + """End-to-end through wait_for_completion: two Stops over a stable + marker-less done spec complete the session instead of arming the stall + grace toward the #149 livelock.""" + monkeypatch.setattr(generic, "RESULT_GRACE_S", 0.0) + monkeypatch.setattr(generic, "RESULT_POLL_S", 0.0) + adapter, impl = make_dev_adapter(tmp_path) + (impl / "spec-3-1-foo.md").write_text(_MARKERLESS_DONE) + stop = _stop_event("3-1-dev-1", "sess-1", "/t.jsonl") + adapter.watcher = _ScriptedWatcher([stop, stop]) + result = adapter.wait_for_completion(_dev_handle(), _dev_spec(tmp_path)) + assert result.status == "completed" + assert result.result_json["status"] == "done" + assert result.result_json["synthesized_from_frontmatter"] is True diff --git a/tests/test_policy.py b/tests/test_policy.py index baea12a1..86f39c82 100644 --- a/tests/test_policy.py +++ b/tests/test_policy.py @@ -39,6 +39,17 @@ def test_review_trigger_invalid(): policy.loads('[review]\ntrigger = "sometimes"\n') +def test_review_on_timeout_default_and_parse(): + assert policy.loads("").review.on_timeout == "retry" + for mode in ("salvage-if-done", "defer"): + assert policy.loads(f'[review]\non_timeout = "{mode}"\n').review.on_timeout == mode + + +def test_review_on_timeout_invalid(): + with pytest.raises(policy.PolicyError, match="review.on_timeout"): + policy.loads('[review]\non_timeout = "salvage"\n') + + def test_stories_defaults(): pol = policy.loads("") assert pol.stories.source == "sprint-status"