fix(engine): finish the commit on resume for tasks persisted in the COMMITTING window#117
Merged
Merged
Conversation
…OMMITTING window A host death after _commit persisted phase=committing but before the DONE save stamped commit_sha matched no resume arm — there is no COMMITTING-keyed session record to replay (_skip_review_and_commit reaches COMMITTING having run zero review sessions) — so _finish_inflight fell through to resume-restart and rolled back / paused over fully-verified work. Resume now finishes the commit in place. The post-gate body of _commit moves into _finalize_commit_phase, whose precondition is a persisted COMMITTING phase: the new resume arm calls it directly, deliberately NOT re-running the pre_commit_gate workflows (the persisted phase is durable proof they passed; a re-run would double-charge the session budget, and a blocking failure would need an illegal COMMITTING->DEFERRED move). The pre_commit hook does re-fire — the message is regenerated, a rewrite reaches the squash, and a pause veto still escalates (COMMITTING->ESCALATED is legal). finalize_commit needs no change: it is content-idempotent across both crash states (pre-squash chain and post-squash single commit), now pinned by a run-twice test. SweepEngine's _recover_inflight_bundle had the identical hole and gets the identical arm; a COMMITTING bundle journals resume-commit and closes instead of restarting. Closes #115
|
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 (10)
WalkthroughCOMMITTING tasks now resume by completing the existing commit instead of restarting verified work. The change covers isolated worktrees, sweep bundles, hook and workflow behavior, idempotent squashing, escalation, and crash-recovery tests. ChangesCOMMITTING Resume Recovery
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant PersistedRunState
participant Engine
participant Git
participant Worktree
PersistedRunState->>Engine: load task in COMMITTING
Engine->>Engine: record resume-commit
Engine->>Worktree: reopen isolated workspace
Engine->>Git: emit pre_commit and finalize commit
Git-->>Engine: return commit SHA
Engine->>PersistedRunState: save DONE and commit_sha
Engine->>Worktree: merge unit into main checkout
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Collaborator
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem (#115)
_commitpersistsphase: committing(the save right afteradvance(COMMITTING)) beforeverify.finalize_commitruns and the DONE save stampscommit_sha. A host death in that window leaves a task whose work is fully verified — dev, review, and deterministic gates all passed — sitting above baseline. On resume it matched no arm of_finish_inflight: not the spec-approval arm (wrong phase), not the #100 replay arm (there is no COMMITTING-keyed session record to replay; the_skip_review_and_commitpath reaches COMMITTING having run zero review sessions). It fell through to resume-restart →_rollback_or_pause, rolling back or pausing over finished work.Fix
Refactor: the post-gate body of
_commitmoves verbatim into_finalize_commit_phase(task), preconditioned on a persisted COMMITTING phase._commitis unchanged in behavior: gate check →advance(COMMITTING)→ save → helper.New resume arm (
_finish_inflight, between the replay arm and resume-restart): a task persisted at COMMITTING journalsresume-commitand re-enters_finalize_commit_phasedirectly (with the same isolated-worktree reopen/integrate wrapper as the other arms). Design points:pre_commit_gateworkflows are deliberately NOT re-run. The COMMITTING save lands only after the gate loop returns clean, so the persisted phase is durable proof they passed; the tree is unchanged since (crash — no sessions ran). A re-run would double-charge the session budget through_run_session, and a blocking failure would need an illegalCOMMITTING→DEFERREDmove.pre_commithook DOES re-fire — the commit message is regenerated, so a plugin rewrite reaches the squash, and a pause veto still escalates (COMMITTING→ESCALATEDis legal). TEA's gate enforcement re-reads its on-disk artifacts fail-open, so re-emitting is safe.finalize_commitneeds no change. It is content-idempotent across both crash states — pre-squash (skill commit chain above baseline) squashes normally; post-squash (squashed commit at HEAD, clean tree) re-squashes to an identical-content commit. Now pinned by a run-twice characterization test.commit_shapersisted at COMMITTING is always empty (stamped only by the DONE save) and is write-only — never routing.Sweep parity:
SweepEngine._recover_inflight_bundlehad the identical hole (a COMMITTING bundle hit the rollback+PENDING reset). It gets the identical arm and closes the bundle instead of restarting it.Tests
test_resume_committing_finishes_commit_to_done— the issue's environment (rollback OFF): resume → DONE, one squashed commit above baseline,commit_shastamped, zero new sessions, noresume-restart/rollback-manual-required.test_resume_committing_post_squash_still_one_commit— finalize completed just before the death: re-drive converges on exactly one commit, no stacked second squash.test_resume_committing_git_error_escalates— a rejecting hook during the re-drive → ESCALATED with the attempt chain intact at HEAD.test_worktree_resume_committing_finishes_and_merges— isolated flavor: finished inside the still-mounted worktree and merged back.test_pre_commit_gate_workflow_not_rerun_on_commit_resume— a blockingpre_commit_gateworkflow is not re-charged on the re-drive.test_pre_commit_hook_fires_on_commit_resume/test_pre_commit_pause_veto_on_commit_resume_escalates— hook re-fires; rewrite reaches git; pause veto escalates legally.test_finalize_commit_rerun_is_content_idempotent— run-twice pin (tree identity, single commit; no sha-inequality assertion — same-second re-runs re-mint the same sha).test_resume_committing_bundle_finishes_commit— sweep flavor, production-faithful: a real run crashed on thepre_commitemit, resumed to DONE with the ledger closed.Shared fabricator
committing_crash_statein conftest persists the exact state.json shape from the issue (attempt commit above baseline — only the work file, keeping the sprint board untracked as production does; specdone; sprint synced at dev time;review_cycle=0for skip-review fidelity).Full suite: 2067 passed, 1 skipped.
trunk checkclean.Closes #115
Summary by CodeRabbit