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

[pbean]

Closes #224. Closes #271.

Part A — #224: a finished story whose session omitted ## Auto Run Result no longer livelocks and DEFER-drops

The review HALT intermittently finalizes the spec's frontmatter to status: done without appending the terminal marker find_result_artifact keys on. On the sprint/sweep scan path that spec was invisible: every Stop read no-artifact, stall nudges re-invoked an already-exited workflow (the #149 livelock), _post_kill_reconcile couldn't rescue it (its read-back hits the same locator), and each RETRY re-stripped the marker (#160) and reproduced the omission — until max_review_cycles deferred and rolled back finished, verify-passing work. Observed twice consecutively in one run (stories 3-10/3-11).

Fix — synthesize from the authoritative terminal frontmatter, exactly what stories mode already trusts (devcontract.synthesize_result):

• devcontract.find_frontmatter_candidates: scan for specs with terminal frontmatter (done/blocked), zero real (non-fenced) markers, mtime at/after the session-launch floor. Returns all matches; the caller refuses to guess between several.
• Live wait loop: harvests only a (path, mtime_ns, status) fingerprint that held stable across 2 consecutive resultless Stops — a single sighting is not enough, because right after a review launch the spec still carries the dev pass's done frontmatter and the review's first write can bump mtime before the in-review flip (the Session read-back adopts another story's spec: a review that produced nothing is scored done #261 premature-harvest hazard). Any fingerprint change restarts the count; a status flip clears it.
• _post_kill_reconcile backstop: a provably dead window harvests on one sighting (dead_window=True); the existing gates (self-consistent, escalation-free, done-only) still apply, and every rescue runs the full deterministic verify downstream (the Hardening: reconcile seam is unreachable for stalled sessions — a finished-but-lost-Stop session cannot be recovered as completed #61 trust model).
• Observability: results carry synthesized_from_frontmatter, the engine journals session-synthesized-from-frontmatter, and new resultless-stop breadcrumb verdicts terminal-frontmatter-pending / ambiguous-frontmatter record the fingerprint's progress.
• Stories mode is untouched (already frontmatter-authoritative — that's the precedent this follows).

The upstream skill fix (make the review HALT's marker append reliably executed) remains a BMAD-METHOD concern; this is the orchestrator-side safety net the issue argues for given the observed intermittency.

Part B — #271: review.on_timeout policy knob

decide_review_session treated every non-completed review verdict identically: RETRY until max_review_cycles, then DEFER — even when the dev product was already good enough to commit. New knob (validated, settings-UI wired, default preserves today's behavior byte-for-byte):

• retry (default) — unchanged.
• salvage-if-done — on a timeout-like verdict (timeout/stalled/over_budget, the same set the post-kill reconcile treats as rescue-eligible), if the spec frontmatter reads done (or in-review, the mid-review interrupt — reset forward, partial terminal section stripped) and _verify_review passes: commit the work, refile the outstanding follow-up to deferred work (origin review-timeout-salvage), journal review-timeout-salvage. A timeout never re-arms followup_review_recommended nor spends a damping grant. Not applicable → falls back through the extracted review_retry_or_exhaust; a failing verify journals review-timeout-salvage-failed first.
• defer — give up on the first timeout-like verdict.

Guard rails: env-fault (#194) still PAUSEs before any mode; crashed keeps the default routing in every mode; the resolved_redrive latch still re-escalates instead of deferring; worktree-isolated runs never salvage (a defer there already preserves the unit's diff). Interaction: with Part A in place, a marker-less done spec completes in the adapter and never reaches the timeout path — salvage only ever sees the complementary case (a review that died with non-terminal frontmatter or never wrote the spec).

Tests

• test_devcontract.py: candidate-finder matrix (terminal statuses, marker/fence exclusion, no-spec-fallback filename, mtime floor, unreadable files).
• test_generic_tmux.py: two-stable-Stops harvest, premature-harvest guard (mtime bump resets), status-flip clears observations, blocked→CRITICAL, ambiguity never harvests, crash-path compare-only semantics, post-kill backstop (done rescued / blocked & ambiguous & pre-launch kept), wait-loop end-to-end via scripted Stops.
• test_escalation.py: {timeout, stalled, over_budget} × {retry, salvage-if-done, defer} matrix; crashed and env-fault invariant under every mode; default-mode reasons byte-compatible.
• test_engine.py: salvage commit + refile + no-rearm, in-review reset variant, verify-fail fallback, non-terminal-spec fallback, isolation/no-spec guards, journal event.
• test_policy.py / test_settings_schema.py: knob parse/validation + core.toml sync.

Full suite: 2847 passed; the 2 test_module_skills_sync.py failures are pre-existing on main (verified). trunk check clean.

Summary by CodeRabbit

• New Features

  • Added configurable review timeout handling: retry, salvage completed work, or defer.
  • Completed work can now be recovered when terminal results are missing, preventing review sessions from stalling.
  • Salvaged reviews can commit verified work and defer follow-up recommendations.

• Bug Fixes

  • Fixed livelocks caused by missing terminal result markers.
  • Improved handling of ambiguous, incomplete, or blocked terminal outcomes.

[coderabbitai]

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 56.34% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main changes: marker-less spec synthesis plus review timeout salvage.
Linked Issues check	✅ Passed	The PR implements both linked objectives: marker-less terminal spec synthesis and configurable review timeout salvage/defer routing.
Out of Scope Changes check	✅ Passed	All code and test changes map to the two linked objectives; no unrelated feature work stands out.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/marker-synthesize-timeout-salvage-224-271

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

tests/test_policy.py (1)

48-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Escape the . in the match= pattern. match= is a regex; the unescaped . matches any character (e.g. reviewXon_timeout would also match). Static analysis flagged this (RUF043).

♻️ Tighten the match pattern

-    with pytest.raises(policy.PolicyError, match="review.on_timeout"):
+    with pytest.raises(policy.PolicyError, match=r"review\.on_timeout"):

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_policy.py` around lines 48 - 50, Update the match pattern in
test_review_on_timeout_invalid to escape the dot in “review.on_timeout”,
ensuring pytest treats it as a literal period rather than a regex wildcard.

Source: Linters/SAST tools

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/bmad_loop/data/settings/core.toml`:
- Line 48: Remove the standalone duplicate [[section.field]] declaration in the
settings configuration, leaving only the populated field entry for on_timeout.
Ensure consumers no longer receive an empty section.field object without key or
metadata.

---

Nitpick comments:
In `@tests/test_policy.py`:
- Around line 48-50: Update the match pattern in test_review_on_timeout_invalid
to escape the dot in “review.on_timeout”, ensuring pytest treats it as a literal
period rather than a regex wildcard.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: f4270715-7592-48f4-ae64-806f17beb995

📥 Commits

Reviewing files that changed from the base of the PR and between 216c0b8 and 71e18ce.

📒 Files selected for processing (12)

• CHANGELOG.md
• src/bmad_loop/adapters/generic.py
• src/bmad_loop/data/settings/core.toml
• src/bmad_loop/devcontract.py
• src/bmad_loop/engine.py
• src/bmad_loop/escalation.py
• src/bmad_loop/policy.py
• tests/test_devcontract.py
• tests/test_engine.py
• tests/test_escalation.py
• tests/test_generic_tmux.py
• tests/test_policy.py