[BUG] Review session timeout always RETRY until max_review_cycles; no salvage/converge exit

[Haven2026]

Description

decide_review_session treats any non-completed review session status the same way: if review_cycle < max_review_cycles, return RETRY; else DEFER/PAUSE.

There is no branch for:

• timeout / timeout_fired vs crash vs user-stop,
• “prior review cycle already left the story status: done + verify-green — converge and refile residual follow-up instead of burning another full review,”
• “do not spend max_followup_reviews after a pure timeout with no new review artifact.”

Result under unattended multi-story runs: a 90m session timeout on review-1 (or a heavy codex second-opinion) becomes review-retry → review-2 → review-3… until the outer cycle budget dies, even when the product tree from dev is already good enough to commit/salvage.

Steps to reproduce

1. Policy: review.enabled = true, review.trigger = "recommended" (or always), limits.session_timeout_min = 90, limits.max_review_cycles = 3, limits.max_followup_reviews = 1.
2. Adapter review on a slower/heavier model (field: codex second opinion) so a single review pass can hit session timeout.
3. Dev pass finishes with a green-enough tree and often already followup_review_recommended from inline review.
4. Watch journal: review session non-completed → review-retry → next cycle → repeat until cycles exhausted or human stops the run.

Expected behavior

At least one of (policy-selectable is fine):

1. Timeout exit class: after N timeouts (default 1) on review, if the last known good state is spec status: done (and optional verify commands green), converge: commit / refile lingering follow-up to deferred-work, do not open another full review pass solely because the session timed out.
2. No follow-up grant after timeout: a timeout that produced no successful review result_json should not consume or re-arm followup_review_recommended damping in a way that schedules another independent pass with empty product delta.
3. Journal a distinct event e.g. review-timeout-salvage vs bare review-retry so external supervisors can act without guessing.

Actual behavior

From installed 0.8.1 escalation.decide_review_session:

if result.status != "completed":
    reason = f"review session {result.status}"
    if budget_left:
        return Decision(Action.RETRY, reason)
    return Decision(_exhausted_action(task), _exhaust_reason(task, reason))

Engine then journals review-retry and continues the review loop. Field story (health-matrix closeout style epic endgame): review-1 timeout → review-2 completes with follow-up flag → review-3 still scheduled while wall-clock and tokens burn — human/AI salvage had to stop the run and ship product via external PR because the engine would not exit the review stack.

Which area is this for?

Orchestrator / control loop

bmad-loop Version

0.8.1

Which coding CLI are you using?

Codex (codex)

(also observed with mixed claude dev + codex review)

Operating System

macOS

Relevant log output

# pattern (sanitized):
# review-session status=timeout (or non-completed)
# journal: review-retry reason="review session timeout"
# review_cycle increments
# eventually max_review_cycles → DEFER, or human bmad-loop stop
# worktree still holds good product code; origin may never see it without salvage

No diagnose dump attached (contains project paths). Happy to produce a sanitized bmad-loop diagnose excerpt if a maintainer wants a specific run shape.

Confirm

• I've searched for existing issues
• If I attached a diagnose dump, I reviewed it and confirmed it contains no PII, secrets, or project-specific information
• I'm using the latest version

Related

• Review HALT that omits ## Auto Run Result re-triggers the #149 livelock, then DEFER-drops a finished story #224 — review HALT / missing Auto Run Result livelock then DEFER-drop (adjacent: review leg failure modes that destroy finished work)
• Field report: a week of unattended overnight runs — what held, and three gaps #232 — unattended overnight field report (supervision above the run)
• Session read-back adopts another story's spec: a review that produced nothing is scored done #261 — review scored done with wrong/empty product (review result integrity)
• Our enhancement ask for remote closeout: will cross-link after filing

Notes on philosophy fit

Still deterministic Python — classify session end reason + prior terminal status; no LLM in the control loop. Optional policy key e.g. review.on_timeout = "retry" | "salvage-if-done" | "defer" keeps default = today’s retry for safety.

Willing to PR a minimal change after design nod (CONTRIBUTING: large/behavior change → Discord first). Suggested minimal patch surface: escalation.decide_review_session + engine journal + one unit test matrix on SessionResult.status.

[coderabbitai]

🔗 Related PRs

#63 - fix(engine): close resume-replay edges from #62 (final-cycle replay, stale result_json, record bloat) [merged]
#151 - fix(adapters): bound dev/review stall nudges + diagnose result-less Stops [merged]
#173 - fix: keep committed work across a dev retry; accept the baseline it advances past (#172) [open]
#175 - fix(adapters,engine): timely + observable session-timeout teardown [merged]
#177 - fix(engine): strip the stale Auto Run Result before every review launch (#160) [merged]

---

📝 Issue Planner

_{Check the box below or use the @coderabbitai plan command to generate an implementation plan and prompts that you can use with your favorite coding assistant.}

• Create Plan

---

🧪 Issue enrichment is currently in open beta.

You can configure auto-planning by selecting labels in the issue_enrichment configuration.

To disable automatic issue enrichment, add the following to your .coderabbit.yaml:

issue_enrichment:
  auto_enrich:
    enabled: false

💬 Have feedback or questions? Drop into our discord!

[Haven2026]

Companion feature (remote Integration Closeout / done ≠ origin): #270. Same field campaign; stop→salvage PR path is the current external mitigation.

[pbean]

Thanks for the detailed report and the design sketch — it matched what we wanted almost exactly, so rather than a design nod we went ahead and implemented it in #273 together with the adjacent #224 fix (the two interact: once #224's missing-marker synthesis lands, a review that finalized done without the marker completes in the adapter and never reaches the timeout path, so the salvage only ever sees the complementary case — a review that died with non-terminal frontmatter or never wrote the spec).

What landed, relative to your ask:

1. review.on_timeout = "retry" | "salvage-if-done" | "defer", default retry (today's behavior, byte-compatible reasons) — your suggested shape verbatim.
2. Timeout-like set = timeout/stalled/over_budget (the same set the post-kill reconcile treats as rescue-eligible); crashed and env-fault (Dev session lost to an API/transport failure is charged as a story timeout, exhausting the attempt budget #194) keep their own routing in every mode.
3. salvage-if-done gates on the spec frontmatter (done, or the in-review mid-interrupt which is reset forward) plus the full _verify_review gate, then commits and refiles the outstanding follow-up to deferred work (origin review-timeout-salvage). A timeout never re-arms followup_review_recommended nor spends a damping grant.
4. Distinct journal events: review-timeout-salvage / review-timeout-salvage-failed, plus a desktop/file notification on salvage.

One deliberate scoping note: worktree-isolated runs never salvage — a defer there already preserves the unit's worktree + diff, and committing into the main repo would be wrong (same rationale as the existing budget-exhaustion rescue). Your #270 remote-closeout ask stays complementary for the done-≠-origin gap. Happy to adjust anything in review on #273.