Skip to content

feat(operator): commit per-story park records for cross-machine confirm - #361

Merged
pbean merged 3 commits into
mainfrom
feat/operator-committed-park-records
Jul 28, 2026
Merged

feat(operator): commit per-story park records for cross-machine confirm#361
pbean merged 3 commits into
mainfrom
feat/operator-committed-park-records

Conversation

@pbean

@pbean pbean commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Part 1 of 2 for #356 (the second PR brings the validate check id, the message/doc rewrites, the merge_strategy="ff" acceptance parametrization, and the worktree fresh-clone E2E).

Problem

bmad-loop confirm only worked on the machine that parked the story. The park index (.bmad-loop/operator-actions.json) was machine-local by design, and spec_file comes 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.

  • Placement is the design: the record is written in _finalize_commit_phase, inside the commit window (after _close_declared_deferred, before finalize_commit), into the workspace — so git add -A folds 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 keeps merge_strategy = "ff" under branch_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.
  • One file per story: parks on different branches can never merge-conflict.
  • No stored commit sha (a commit cannot contain its own sha): resolve() derives provenance via the new verify.last_commit_forgit log over 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.
  • The legacy prune guards its temp too (review): _drop_legacy's rewrite arm mirrors record_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 literal operator-actions.json, never its .tmp sibling, so a stranded operator-actions.tmp is untracked residue that holds worktree_clean False and blocks the next run's preflight and the epic-boundary auto-sweep.
  • Failure semantics stay best-effort: an unwritable record degrades to an operator-index-failed journal line and the story parks recordless (raising would abort a story that genuinely finished); a failed commit restores the record the way _write_park_record found it (_restore_park_record in both except arms, mirroring _restore_deferred_closes).
  • Legacy fallback: 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_paths hardening: skips paths that neither exist on disk nor are tracked. git add hard-fails on a pathspec matching nothing and _land_confirmation swallows the GitError as 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

  • Full suite green; trunk check clean.
  • Ablation passes on every negative/placement gate (delete the gate, watch its test fail, restore by re-editing):
    • re-add a legacy-style git exclude to record_parktest_a_park_record_is_visible_to_git fails (tree stays clean)
    • delete the _write_park_record call → test_park_commits_a_record_for_confirm fails (no record anywhere)
    • delete both _restore_park_record calls → test_a_failed_commit_restores_the_park_record fails (orphaned record survives)
    • narrow _park_spec_relpath to (OSError, ValueError)test_an_unresolvable_spec_path_still_records_the_park fails on 3.13 and 3.11
    • narrow _write_park_record to except OSErrortest_a_runtime_error_writing_the_record_degrades_like_an_oserror fails (story aborts at COMMITTING)
    • revert the commit_paths filter → test_confirm_commits_even_without_a_committed_record fails ("✓ confirmed" printed, nothing committed)
    • drop record from _land_confirmation's commit list → test_confirm_flips_spec_board_record_and_commits fails on the name-only assertion
    • key drop on the computed path only → test_drop_matches_the_records_own_key_not_its_filename fails
    • drop the tmp unlink in record_park's except arm → test_a_failed_record_write_leaves_no_tmp_residue fails
    • drop the unlink in _drop_legacy's except arm → test_a_failed_legacy_prune_leaves_no_tmp_residue fails (the temp survives as untracked residue)
    • make _derive_commit return ""test_resolve_derives_the_commit_from_the_record_history fails (the legacy stored-sha test stays green)
  • Fresh-clone acceptance at the CLI layer: 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

  • New Features
    • Parked story actions are now stored as committed, per-story records that travel with the story branch and can be confirmed from another clone.
  • Bug Fixes
    • Confirmation now records/removes parked obligations within the same commit, with hardened handling for missing paths/records, write failures, legacy data, and drift detection.
    • Updated guidance when a parked story isn’t available locally to pull the branch containing the park record.
  • Documentation
    • Updated changelog and confirmation documentation to reflect the new provenance/commit derivation model.
  • Tests
    • Expanded CLI/engine/operator-record coverage for clone, legacy, and failure scenarios.

@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

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

Changes

Committed park record persistence

Layer / File(s) Summary
Per-story record persistence and provenance
src/bmad_loop/operatoractions.py, src/bmad_loop/verify.py, tests/test_operatoractions.py
Per-story records replace the primary machine-local index, retain legacy compatibility, derive commit provenance from git history, and cover malformed, legacy, atomic-write, drift, and resolution behavior.
Commit-window writing and rollback
src/bmad_loop/engine.py, tests/test_engine.py, tests/test_engine_worktree.py
Story commit finalization writes park records, restores them after failures, handles fallback paths and write errors, and verifies records survive resume and worktree merges.
Confirmation, deletion, and path handling
src/bmad_loop/cli.py, src/bmad_loop/verify.py, tests/test_cli.py, CHANGELOG.md, src/bmad_loop/documents.py
Confirmation messaging, provenance output, record deletion, missing-path handling, schema documentation, changelog entries, fresh-clone behavior, and legacy-record compatibility are updated and tested.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related issues

  • Issue 356 — Replaces machine-local operator indexing with committed per-story records for confirmation from clones.
  • Issue 335 — Implements the per-story operator parking and confirmation design.

Possibly related PRs

Poem

A rabbit tucked records where story commits grow,
So clones can find what operators know.
Old indexes fade, but their echoes remain,
Confirm hops branches like springtime rain.
One JSON carrot, safely in tow!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 72.58% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: committed per-story park records enabling cross-machine confirm.
✨ 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 feat/operator-committed-park-records

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

Comment @coderabbitai help to get the list of available commands.

Comment thread src/bmad_loop/operatoractions.py
@greptile-apps

greptile-apps Bot commented Jul 28, 2026

Copy link
Copy Markdown

Greptile Summary

This PR replaces the machine-local operator-actions index (.bmad-loop/operator-actions.json, excluded from git) with per-story committed JSON records under .bmad-loop/operator/<key>.json, so a park commit carries its own confirmation data to every clone. The legacy store is kept readable and prunable for in-flight parks from older versions, but is never written again.

  • Engine writes the record inside the commit window (_write_park_record, just before finalize_commit's git add -A), captures prior content for restore, and cleans up on both failure arms (_restore_park_record mirrors _restore_deferred_closes).
  • verify.commit_paths gains a missing-but-untracked filter that prevents git add hard-failing on a legacy-only park path, and a new last_commit_for helper derives the park's commit sha from the record file's own git history rather than storing it (the record cannot name the commit it rides).
  • operatoractions is rewritten around per-file atomic writes with temp-cleanup on BaseException; drop removes both the record file and any legacy entry; load merges both stores with records winning their key.

Confidence Score: 5/5

Safe 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 record_park and _drop_legacy, the commit_paths never-committed filter, and the fresh-clone acceptance criterion end-to-end. The _write_park_record / _restore_park_record pair faithfully mirrors the existing _restore_deferred_closes pattern. No logic gaps were found.

Files Needing Attention: No files require special attention.

Important Files Changed

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

@pbean

pbean commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator Author

Re: the out-of-diff finding on the drift message (operatoractions.py:310-311) — valid, and it was understated: nine more live user-facing strings said "the index" for the committed per-story store. Handled in the stacked #362, which already owns the message/doc rewrites, so the vocabulary lands as one diff.

The string itself is now "no spec file could be located for it" rather than the suggested "the record carries no spec file for it", for two reasons:

  • load() merges legacy index entries alongside records and resolve() builds a ParkedStory from either, so an entry reaching this branch may not be a record at all — "record" over-claims. The cli.py strings use "the park entry" for the same reason.
  • _spec_path returns None in two cases: an empty spec_file, and one that fails resolve_spec_path. "carries no spec file" (and the original "the index records no spec file") are both false in the second case; "could be located" is true in both.

@pbean
pbean merged commit f8080f8 into main Jul 28, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant