feat(operator): commit per-story park records for cross-machine confirm - #361
Conversation
WalkthroughParked operator actions now use committed per-story JSON records, with legacy index compatibility. Engine commit handling writes and restores records safely, while confirmation derives provenance, removes records in the confirmation commit, and works across fresh clones. ChangesCommitted park record persistence
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
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 |
Greptile SummaryThis PR replaces the machine-local operator-actions index (
Confidence Score: 5/5Safe to merge — the change is additive within the commit window, falls back gracefully to the legacy index, and every failure arm restores state. Every edge case identified in the PR description is covered by a dedicated ablation-tested test: commit-window failure and record restore, legacy-index interop, temp-file residue cleanup on write failure in both Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| src/bmad_loop/operatoractions.py | Core module rewrite: replaces machine-local shared index with per-story committed JSON records; legacy store read/drop compatibility preserved; atomic writes with temp-file cleanup on all paths. |
| src/bmad_loop/engine.py | New _write_park_record / _restore_park_record pair wired into the commit window; prior-text capture and restore semantics mirror _restore_deferred_closes; _index_park removed cleanly. |
| src/bmad_loop/verify.py | Two targeted additions: last_commit_for derives park provenance via git log -n 1 --format=%H; commit_paths gains a missing-but-untracked filter that prevents git add hard-failing on a legacy-only park record path. |
| src/bmad_loop/cli.py | Error message updated for the new cross-machine model; _land_confirmation resolves the record path before dropping it and adds it to commit_paths so the deletion rides the confirm commit. |
| tests/test_operatoractions.py | Comprehensive new tests covering record visibility to git, temp-residue cleanup, legacy merge/drop/prune, derived provenance, mangled-file drift, and safe_segment-keyed drop. |
| tests/test_engine.py | New tests pin: record lands in the park commit, commit failure restores the record, unresolvable spec still writes a record, and a RuntimeError degrades rather than aborting the commit. |
| tests/test_cli.py | Two significant new tests: legacy-park commit_paths filter and the fresh-clone acceptance criterion end-to-end. |
Reviews (2): Last reviewed commit: "fix(operator): unlink the legacy prune t..." | Re-trigger Greptile
|
Re: the out-of-diff finding on the drift message ( The string itself is now
|
Part 1 of 2 for #356 (the second PR brings the
validatecheck id, the message/doc rewrites, themerge_strategy="ff"acceptance parametrization, and the worktree fresh-clone E2E).Problem
bmad-loop confirmonly worked on the machine that parked the story. The park index (.bmad-loop/operator-actions.json) was machine-local by design, andspec_filecomes only from the dev session's result JSON — it is not derivable from a story key — so a teammate, a fresh clone, or CI saw a parked spec and a parked board with no way to confirm either.Design
One committed JSON record per parked story at
.bmad-loop/operator/<safe_segment(key)>.json, replacing the machine-local index._finalize_commit_phase, inside the commit window (after_close_declared_deferred, beforefinalize_commit), into the workspace — sogit add -Afolds it into the park's own commit, and the feat(cli): bmad-loop confirm completes awaiting-operator stories #355 merge-back delivers it to the target under every merge strategy. Nothing lands on the target branch during the window, which is what keepsmerge_strategy = "ff"underbranch_per = "run"intact (the regression that killed the issue's own commit-the-shared-index sketch), leaves the epic-boundary auto-sweep's clean-tree precondition undisturbed, and lets a crash re-drive through the same COMMITTING resume arm that re-derives the park itself.resolve()derives provenance via the newverify.last_commit_for—git logover the record's own path — which under a squash merge names the commit that actually carries the park on this branch.""for an uncommitted record; legacy entries keep their stored sha._drop_legacy's rewrite arm mirrorsrecord_park's unlink-on-raise, for a stronger reason — no commit window absorbs this one..bmad-loop/is not ignored and the pre-bmad-loop confirm only works on the machine that parked the story #356 exclude line was the anchored literaloperator-actions.json, never its.tmpsibling, so a strandedoperator-actions.tmpis untracked residue that holdsworktree_cleanFalse and blocks the next run's preflight and the epic-boundary auto-sweep.operator-index-failedjournal line and the story parks recordless (raising would abort a story that genuinely finished); a failed commit restores the record the way_write_park_recordfound it (_restore_park_recordin both except arms, mirroring_restore_deferred_closes).load()still reads the old machine-local store (a record wins its key),drop()prunes it, nothing writes it anymore — an in-flight park from an older version stays confirmable where it was written.verify.commit_pathshardening: skips paths that neither exist on disk nor are tracked.git addhard-fails on a pathspec matching nothing and_land_confirmationswallows theGitErroras best-effort — a legacy-only park would silently lose its spec+board commit behind a "✓ confirmed". A missing-but-tracked path stays in (that is a deletion to stage), which is how the record's deletion rides the confirm commit.Testing
trunk checkclean.record_park→test_a_park_record_is_visible_to_gitfails (tree stays clean)_write_park_recordcall →test_park_commits_a_record_for_confirmfails (no record anywhere)_restore_park_recordcalls →test_a_failed_commit_restores_the_park_recordfails (orphaned record survives)_park_spec_relpathto(OSError, ValueError)→test_an_unresolvable_spec_path_still_records_the_parkfails on 3.13 and 3.11_write_park_recordtoexcept OSError→test_a_runtime_error_writing_the_record_degrades_like_an_oserrorfails (story aborts at COMMITTING)commit_pathsfilter →test_confirm_commits_even_without_a_committed_recordfails ("✓ confirmed" printed, nothing committed)recordfrom_land_confirmation's commit list →test_confirm_flips_spec_board_record_and_commitsfails on the name-only assertiondropon the computed path only →test_drop_matches_the_records_own_key_not_its_filenamefailsrecord_park's except arm →test_a_failed_record_write_leaves_no_tmp_residuefails_drop_legacy's except arm →test_a_failed_legacy_prune_leaves_no_tmp_residuefails (the temp survives as untracked residue)_derive_commitreturn""→test_resolve_derives_the_commit_from_the_record_historyfails (the legacy stored-sha test stays green)test_confirm_works_on_a_fresh_clone_of_a_parked_repo(park → clone →confirm --list+confirm→ board done, confirm commit present in the clone).Summary by CodeRabbit