fix(engine): resume replays completed sessions persisted at DEV_VERIFY/REVIEW_VERIFY instead of demanding a rollback#116
Conversation
…VERIFY instead of demanding rollback A host death in the post-verify decision window persists the task at DEV_VERIFY (or REVIEW_VERIFY one phase later) with the completed session's record durably saved but the decision's action unapplied. The resume replay matcher only knew the *_RUNNING phases, so those tasks fell through to resume-restart -> _rollback_or_pause, pausing with a bare 'git reset --hard <baseline>' instruction that would discard the attempt's finished -- possibly already-pushed -- commits. _resumable_session now matches DEV_VERIFY (dev, attempt) and REVIEW_VERIFY (review, review_cycle); the existing replay arms re-enter the normal reconcile -> verify -> decide pipeline with no budget burned and the persisted baseline kept. The spec-approval-gate arm stays first, so a DEV_VERIFY task with a verified spec keeps _resume_after_dev_verify. The rollback-OFF manual-recovery notice now probes commits_above(baseline) and, when the attempt committed work, leads with saving/checking those commits (they may already be pushed) instead of instructing a blind reset; the probe is advisory and falls back to the classic notice on a git fault. Fixes #100. Refs #99.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughResume now replays durably recorded dev or review results from verify phases, while rollback-off manual recovery identifies committed work above baseline and reports it without resetting the tree. ChangesResume and recovery behavior
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Operator
participant Engine
participant SessionRecord
participant VerifyDecide
Operator->>Engine: resume persisted task
Engine->>SessionRecord: load completed result_json
Engine->>VerifyDecide: replay recorded result
VerifyDecide-->>Engine: return verify/decide outcome
Engine-->>Operator: finish or retry task
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
Fixes #100. Refs #99 (closed duplicate), #115 (follow-up).
The bug
A host death (crash/OOM/kill) in the post-verify decision window persists the task at
DEV_VERIFY— the_save()right after the verify/decide pass — with the completed dev session's record (status: completed,result_json.status: done) already durably saved, but the decision's action unapplied andcommit_shastill null.task.spec_fileis only set on full verify success, so when verify had not passed at death, resume found:DEV_VERIFY and task.spec_file→_resume_after_dev_verify): skipped, no spec_file_resumable_sessionreplay): skipped — the matcher only knewDEV_RUNNING/REVIEW_RUNNING_rollback_or_pause(cause="stopped")→ with rollback OFF (the default) a pause whose notice instructs a baregit reset --hard <baseline>— past the attempt's finished, possibly already-pushed commits, without ever consulting the recorded evidence or git.Note the routing never reads
commit_sha(the issue's suggested trigger) — it is write-only bookkeeping; the actual keys arephase+spec_file.The fix
Routing:
_resumable_sessionnow also matchesDEV_VERIFY→ (dev,task.attempt) andREVIEW_VERIFY→ (review,task.review_cycle). The existing replay arms re-enter the normal reconcile → verify → decide pipeline: the persisted baseline is kept, no attempt/cycle budget is burned, and completion authority is unchanged (only acompletedrecord with a recorded result replays — anything less still takes resume-restart). When verify passes on replay the story proceeds to review/commit andcommit_shais stamped; when the original failure reproduces, the normal retry/defer/escalate semantics apply — behavior parity with a live run, minus the destructive instruction.Notice hardening: the rollback-OFF manual-recovery notice now probes
verify.commits_above(baseline)(advisory, GitError-safe). When the attempt committed work, the notice leads with saving the commits (git branch my-rescue HEAD, names<baseline>..HEAD) and checking whether they are already integrated or pushed — a reset is only ever the last, optional step. This also covers the notice-wording ask from #99 and de-fangs theCOMMITTING-window sibling tracked as #115.Scope
REVIEW_VERIFYis the same crash window one phase later (persisted by the save right after the review session, before the decision acts); it shares the two-line matcher fix and is covered by its own test.COMMITTINGwindow is real but needs different machinery (no session record exists to replay on the skip-review path) — split out as resume-restart discards verified work when the host dies in the COMMITTING window (no session record to replay) #115.SweepEnginehand-rolls bundle recovery outside_finish_inflight(documented there as a resume-fidelity change of its own); it does inherit the hardened notice.Tests
test_resume_dev_verify_replays_recorded_dev_result_to_done— the [BUG] bmad-loop resume requests rollback of already-committed work when orchestrator dies after successful dev session #100 regression: hand-built crash state in the issue's exact shape (phasedev-verify, emptyspec_file, completed/done record, commits above baseline, rollback OFF) resumes to DONE with nothing re-run, no rollback journal,commit_shastamped, work preserved.test_resume_dev_verify_replay_verify_still_failing_retries_normally— a reproducing verify failure re-enters the budgeted retry path with the commits parked on a recovery ref.test_resume_dev_verify_record_incomplete_still_restarts— stalled / result-less records atDEV_VERIFYkeep resume-restart.test_resume_review_verify_replays_recorded_review— crash injected after theREVIEW_VERIFYsave; resume replays to DONE,review_cycleunburned.test_resume_dev_verify_replay_stories_mode— stories-mode parity (replay after operator repair, no plan-halt leg triggered).test_manual_recovery_notice_names_committed_work/test_manual_recovery_notice_probe_failure_falls_back— the new notice branch and its advisory-probe fallback.All new tests fail without the fix. Full suite: 2058 passed, 1 skipped.
E2E validation
Drove the real
bmad-loop resumeCLI against a sandbox repo whosestate.jsonwas hand-written to the reporter's diagnostic dump shape (real git repo, attempt commit above baseline, finalized spec on disk, fake-CLI profile): the run resumed straight tostory-done— journalresume-verify→story-done,commit_shastamped, the attempt's work squash-committed, and noreset --hardinstruction anywhere in the output.Summary by CodeRabbit