fix(engine): close resume-replay edges from #62 (final-cycle replay, stale result_json, record bloat)

[pbean]

Summary

Maintainer follow-up to #62 (which closed #57), closing the three non-blocking edges called out in that PR's review. Each only surfaces on the new crash-in-the-post-session-window replay path #62 introduced, so none regress steady-state behavior.

(a) Final-review-cycle replay dropped a recorded clean review to defer. _review_and_commit's loop was guarded by while task.review_cycle < max_review_cycles. A host death in the post-session window of the last allowed review cycle resumes with review_cycle already == max_review_cycles, so the replayed continuation evaluated the guard as false, never entered the loop body, and the durably-recorded clean review was dropped — the story fell through to defer. The guard now also enters while a resume_result is pending; the replay branch already skips the review_cycle += 1 re-increment, so no extra budget is burned, and resume_result is nulled once consumed so every later iteration falls back to the normal budget guard.

(b) SessionRecord.result_json aliased the dict _reconcile_generic_terminal_status mutates in place. _run_session stored result_json=result.result_json (same object) and saved it before _dev_phase ran reconcile, which mutates that dict in place. A crash between reconcile's spec write and the dev-decision _save() persisted a pre-reconcile dict; on replay reconcile early-returns (fm_status == success_status) and never re-folded followup_review_recommended, so task.followup_review_recommended read stale and the follow-up review could be silently skipped. Two-part fix:

• Persist a defensive shallow copy at record time so the durable record is a stable snapshot of what the adapter returned, decoupled from downstream in-place mutation (HookContext already copies; reconcile only touches top-level keys).

• On reconcile's idempotent early-return, heal the stale dict from the already-read frontmatter (sync status, fold followup_review_recommended).

  Scoping note: the heal folds followup_review_recommended only when the frontmatter carries the key, rather than defaulting an absent key to False. On the generic path the spec frontmatter is exactly the source devcontract.synthesize_result already derives the flag from, so a present key can never disagree with the session's own value — but when it is absent (a signal the session may have carried only in its result.json), defaulting to False would clobber a real recommendation on the ordinary, non-resumed path. Gating on key presence is behaviorally identical for real generic sessions and avoids that clobber.

(c) Triage/sweep + labeled plugin-workflow sessions persisted never-consumed result_json (state.json bloat). _run_session recorded result_json for every session, but _resumable_session only ever matches dev/review task ids under DEV_RUNNING/REVIEW_RUNNING. role="triage" sweep sessions and labeled plugin-workflow sessions can never be consumed on resume, so their payloads were pure bloat. The same defensive-copy site gates them to None via resumable = label is None and role in ("dev", "review").

Tests

Added to tests/test_engine.py, reusing the existing make_engine/resume_engine/dev_effect/review_effect helpers and #62's crashing-_emit pattern:

• final-cycle clean replay → DONE: max_review_cycles=1, review completes clean, host dies at its post_session; resume drives the story to DONE, review_cycle == 1, zero re-run sessions, no resume-restart, no defer.
• final-cycle dirty replay → defer: same setup, non-convergent review; the replay is consumed, the loop exits on the normal budget guard (no fresh session, review_cycle == 1), and the story defers.
• reconcile early-return heal: frontmatter done + followup_review_recommended: true, stale dict → status synced and followup folded; idempotent path journals nothing.
• record isolation: mutating result.result_json after _run_session leaves both the in-memory and on-disk record's result_json unchanged.
• resumable-role gating: dev/review records persist result_json; triage and labeled records persist None.

Full suite: 1410 passed, 1 skipped. Full trunk check clean on both modified files.

Refs #62, #57

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Resumed sessions now continue correctly at the final review cycle instead of stopping early.
  • Finalized results now restore the expected status and review recommendation data more reliably when resuming.
  • Saved session results are protected from later in-memory changes, and only resumable session types keep stored result details.

• Tests

  • Added regression coverage for resume behavior, result reconciliation, and persistent result handling.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a6ad3be9-5f6e-4d48-829e-c20b087593f3

📥 Commits

Reviewing files that changed from the base of the PR and between 534e118 and f31221d.

📒 Files selected for processing (2)

• src/bmad_loop/engine.py
• tests/test_engine.py

---

Walkthrough

The review loop's resume condition now permits loop entry when a resume_result is present even after the review-cycle budget is exhausted. Terminal-status reconciliation heals in-memory result_json from frontmatter on resume. Session persistence now stores result_json only for dev/review roles, as a defensive copy.

Changes

Resume Durability and result_json Persistence

Layer / File(s)	Summary
Review loop resume condition src/bmad_loop/engine.py, tests/test_engine.py	The _review_and_commit while-loop guard now also allows entry when resume_result is present, avoiding extra budget consumption on resume; tests cover clean and dirty final-cycle replays.
Terminal status reconciliation healing src/bmad_loop/engine.py, tests/test_engine.py	The already-finalized early-return path in _reconcile_generic_terminal_status now refolds status and, conditionally, followup_review_recommended from frontmatter into result_json when resuming; a test verifies idempotent healing of a stale resumed dict.
Session persistence of result_json src/bmad_loop/engine.py, tests/test_engine.py	_run_session now persists result_json (as a shallow copy) only for dev/review sessions and None otherwise; tests confirm mutation isolation and role-based persistence restrictions.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related issues

• Hardening: reconcile seam is unreachable for stalled sessions — a finished-but-lost-Stop session cannot be recovered as completed #61: Both changes affect Engine._reconcile_generic_terminal_status and terminal-session recovery logic, indicating overlapping code paths for resume/reconciliation handling.

Poem

A rabbit hops through code so deep,
Where sessions crash mid-review-sleep,
Now resume finds its way back true,
result_json copied, safe and new.
No extra cycle spent in vain —
Just clean replay, again, again! 🐇✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR does not implement the linked #57 durability barrier before post_session or the explicit KeyboardInterrupt teardown path.	Add the pre-post_session save/durability barrier and explicit KeyboardInterrupt handling that stops, kills, and marks the run stopped.

✅ 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 matches the main engine resume/replay and result_json fixes in the PR.
Out of Scope Changes check	✅ Passed	The changes stay focused on engine replay, session record persistence, and related regression tests.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ 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/pr62-followups

---

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

[augmentcode]

🤖 Augment PR Summary

Summary: Fixes three resume/replay edge cases introduced in #62 so recorded session results are correctly replayed after a crash.

Changes:

• Allow `_review_and_commit` to enter the loop when a `resume_result` exists even if `review_cycle` is already at `max_review_cycles` (final-cycle replay).
• Persist a defensive snapshot of `SessionRecord.result_json` for resumable dev/review sessions; omit it for triage and labeled workflow sessions to avoid state.json bloat.
• On `_reconcile_generic_terminal_status`’s idempotent early-return path, heal stale resumed `result_json` by syncing `status` and (when present) folding `followup_review_recommended` from frontmatter.

Tests:

• Add regression tests covering final-cycle clean/dirty replay, reconcile healing, record isolation from later mutation, and resumable-role gating.

_{🤖 Was this summary useful? React with 👍 or 👎}

[augmentcode]

Review completed. No suggestions at this time.

Comment augment review to trigger a new review at any time.