feat(review): escalate deliberate sprint-status regressions at review-verify

[pbean]

Closes #334

The livelock

The orchestrator — not the dev session — advances sprint-status to done at dev time (engine.py:2080, its sole sprint_advance site; verify_dev then asserts the write landed, and advance never regresses). A review session that judges the story unfinished and writes the board back to an earlier stage is doing the right thing, but nothing in the review loop re-advances it. So verify_review's sprint != "done" check returned a non-fixable retry, the loop continued, and every remaining cycle re-read the identical failure — until max_review_cycles ran out and the story deferred with its work rolled back.

Reported by Wojtek Diset against 0.9.0: three review passes burned on a structural disagreement, then a rollback onto an attempt-preserve ref.

The fix

verify_review gains a kw-only sprint_reached_done: bool = False. Only Engine._verify_review passes it (not self._dev_review_enabled() — exactly the case where _post_dev_state_sync targeted done); the sweep and stories gates are separate functions and are untouched. When the flag is set, the knob is escalate, and the fresh sprint status is in STATUS_ORDER at an index below done, the gate returns VerifyOutcome.escalate(..., contradiction=True) — a reason naming both sides, both ways out, and the opt-out knob. That flows into the existing not outcome.retryable → self._escalate routing at both call sites, so the run pauses and paused_reason carries the text to bmad-loop resolve.

One site needed an edit: the budget-exhaustion rescue (engine.py:1528) tested self._verify_review(task).ok inline. When every cycle recommends its own follow-up the in-loop verify never runs, so the rescue is the first read of the board — a contradiction there would have fallen through to the did not converge defer that rolls the work back naming neither side. It now captures the outcome and escalates on contradiction.

New knob [review] on_status_contradiction = "escalate" (default) | "retry", following the #271 on_timeout template (constant, ReviewPolicy field, parse + validation, POLICY_TEMPLATE, core.toml, OPTIONS_ENUM). Default escalate because the released behavior is the defect — it burns cycles and then discards the work; retry reproduces it verbatim for anyone who wants it back.

Deliberate scope boundaries

• Sprint-status only, never spec status. The spec's frontmatter legitimately cycles (in-review / in-progress) while a review patches; the board is orchestrator-owned truth. status: blocked remains the sanctioned reviewer hand-back channel and pauses before this gate — no overlap.
• No review-prompt change. Prohibiting the revert would make a correct reviewer comply, and the story would commit without sign-off. The contradiction would become invisible rather than resolved — strictly worse than the livelock.
• Conservative on every uncertainty. No launch flag, a missing board entry (None), or a token outside STATUS_ORDER (hand-edited or future board) all fall back to the legacy retry. A wrong escalation halts an otherwise healthy run.
• No changes to escalation.py, resolve.py, sweep.py, stories_engine.py, or the TUI. The journal keeps the review-verify-failed kind and only gains a contradiction= field, so the TUI's substring colorizer is unaffected.

Known limitation (documented, out of scope)

A review that regresses both the spec status and the sprint board never reaches _verify_review — the loop's status == "done" gate at engine.py:1462 short-circuits first — so it still lands in the non-terminal-status exhaustion defer.

Tests

test_policy.py (default / parse / invalid / template), test_verify.py ×4 (escalates · retry-mode legacy · no launch flag · missing-or-unknown token), test_engine.py ×4 (sign-off regression escalates in 2 scripted sessions · retry-mode 3-cycle legacy defer + rollback · a non-regression verify failure keeps its fixable-retry routing · rescue-site contradiction with max_followup_reviews=5 escalates instead of deferring).

Three ablations run before trusting them (repo rule):

Ablation	Expected to fail	Result
Delete the escalate branch	the 3 escalate tests	✅ — and the engine one fails exactly as designed: ScriptExhausted: no scripted result for session 1-1-a-review-2 (the ablated engine asks for the review cycle the fix prevents)
Ignore the knob (always escalate)	the 2 retry-mode tests	✅
Drop the launch-flag + STATUS_ORDER guards	the 3 must-not-escalate tests	✅

Full suite 3363 passed, 24 skipped; trunk check clean; pyright@1.1.411 0 errors.

[coderabbitai]

Warning

Review limit reached

@pbean, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 32 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 4a15b9e1-88be-48f6-95ff-bf41f4b15326

📥 Commits

Reviewing files that changed from the base of the PR and between 3c68153 and ad1b628.

📒 Files selected for processing (11)

• CHANGELOG.md
• README.md
• src/bmad_loop/data/settings/core.toml
• src/bmad_loop/engine.py
• src/bmad_loop/model.py
• src/bmad_loop/policy.py
• src/bmad_loop/verify.py
• tests/test_engine.py
• tests/test_policy.py
• tests/test_settings_schema.py
• tests/test_verify.py

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/review-status-contradiction

---

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.}

[greptile-apps]

Greptile Summary

This PR fixes a livelock (#334) where a review session that writes sprint-status backward from done caused the orchestrator to burn all review cycles replaying an un-resolvable failure, then defer and roll back the work. The fix adds contradiction detection at the review-verify gate: when the orchestrator's launch-time sign-off is confirmed and the board is found regressed, the run pauses with both sides named instead of looping.

• New _is_signoff_regression helper in verify.py decides whether a non-done sprint status is a genuine regression (board was at done post-dev, knob is escalate, status is a recognized lifecycle token earlier than done) vs. a stage the story simply never reached — falling back to the legacy retry on every uncertainty.
• New policy knob [review] on_status_contradiction = \"escalate\" | \"retry\" lets operators opt out to the old burn-cycles behavior; default is escalate because the released behavior is the defect.
• Rescue-gate fix (engine.py:1527): the budget-exhaustion path now captures and escalates contradictions instead of falling through to the \"did not converge\" defer that named neither side of the disagreement.

Confidence Score: 5/5

Safe to merge — all three primary escalating paths are correctly wired, the new knob defaults conservatively, and the fix is guarded against every uncertainty the PR description enumerates.

The contradiction detection is conservative by design: missing launch flag, unknown board token, None status, and the retry knob all fall through to the existing retry path unchanged. _escalate always raises RunPaused so no affected code path falls through silently. The four new engine tests include a correctly-described ablation table and cover the rescue-gate case that was previously an unaddressed gap. The only finding is an observability gap in the timeout-salvage journal entry — a very narrow path requiring both a review timeout and a board regression simultaneously — with no impact on correctness.

Files Needing Attention: src/bmad_loop/engine.py around the _salvage_review_timeout journal append (line 1621) — the contradiction field is missing there, unlike the three paths the PR explicitly covers.

Important Files Changed

Filename	Overview
src/bmad_loop/verify.py	Adds sprint_reached_done kw-only param to verify_review and new _is_signoff_regression helper; logic is conservative on every uncertainty (None status, unknown token, missing flag, retry knob all fall through to ordinary retry)
src/bmad_loop/engine.py	Three call-site changes: _verify_review wrapper passes sprint_reached_done, both in-loop journal entries gain contradiction, and the rescue gate now captures the outcome and escalates on contradiction before falling through to the defer
src/bmad_loop/model.py	Adds contradiction: bool = False field to VerifyOutcome and threads it through VerifyOutcome.escalate(); correctly defaults to False so existing callers are unaffected
src/bmad_loop/policy.py	New REVIEW_ON_STATUS_CONTRADICTION_MODES constant, on_status_contradiction field on ReviewPolicy, parse + validation in loads, and POLICY_TEMPLATE entry — consistent with the on_timeout precedent
tests/test_engine.py	Four targeted engine tests: escalates on regression (default), legacy defer with retry mode, ordinary fixable-retry not swallowed, and rescue-gate escalation — all with described ablation results
tests/test_verify.py	Four unit tests cover escalation, retry-mode legacy path, missing launch flag, and both unrecognized-status cases (absent story entry and hand-edited token)
tests/test_policy.py	Validates default, all valid modes, template round-trip, and invalid-value rejection for the new knob
tests/test_settings_schema.py	Adds REVIEW_ON_STATUS_CONTRADICTION_MODES to the schema validator's OPTIONS_ENUM, ensuring the settings TUI and schema test stay in lockstep

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[review session completes] --> B{spec status == done?}
    B -- No --> C[retry / defer as before]
    B -- Yes --> D{sprint status == done?}
    D -- Yes --> E[verify commands]
    E -- pass --> F[commit / clean=True]
    E -- fail --> G[fixable retry or escalate]
    D -- No --> H{_is_signoff_regression?}
    H -- "No: no launch flag / retry mode / unknown token / None" --> I[VerifyOutcome.retry]
    H -- "Yes: sprint_reached_done=True, escalate mode, known token < done" --> J["VerifyOutcome.escalate contradiction=True"]
    J --> K{call site}
    K -- in-loop path --> L["journal review-verify-failed contradiction=True → _escalate → RunPaused"]
    K -- rescue gate --> M["journal review-verify-failed contradiction=True → _escalate → RunPaused"]
    K -- skip-review-and-commit --> N["journal review-verify-failed contradiction=True → _escalate → RunPaused"]
    K -- timeout salvage --> O["journal review-timeout-salvage-failed (no contradiction field) → _escalate → RunPaused"]

Loading

_{Reviews (2): Last reviewed commit: "fix(review): journal the rescue-gate con..." | Re-trigger Greptile}

[pbean]

Addressed the Greptile P2: the rescue-gate escalation now emits review-verify-failed with contradiction=True like the two in-loop gates (ad1b628). The claim checked out — that path reached _escalate directly, so a consumer keying on the new flag saw only story-escalated, which defeats the point of adding the field. The non-contradiction arm keeps its existing silence: its outcome is described by the did not converge defer reason, and emitting there would change unrelated behavior.

Ablated: dropping the new append fails the rescue-gate test's journal assertion.