feat(cli): bmad-loop confirm completes awaiting-operator stories - #355
Merged
Conversation
Part 3 of 4 of the awaiting-operator program (#335): the exit from the park that part 2 built the entry for. `bmad-loop confirm <story-key>` acknowledges each owed action in turn (--yes skips the prompts), writes the spec's `## Operator Confirmation` audit section, advances spec and board to done through sprintstatus.advance, and commits the pair. Nothing is re-driven; --reverify re-runs the project's [verify] commands first and blocks the confirmation if they fail. --list and --json project the parked set. Also lands what part 2 split out at the 800-LOC cap: the park now writes the `.bmad-loop/operator-actions.json` index and notifies with the actions plus the command that ends them. The index is machine-local and registered in the repo's local git exclude — committing it from inside a story's commit window would either shift HEAD past the commit the park just stamped or, under worktree isolation, advance the target branch and break an `scm.merge_strategy = "ff"` merge-back. The spec and board stay the committed truth; confirm refuses an index entry that disagrees with them, and validate reports the drift both ways (operator.registry-stale, operator.actions-malformed).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds a machine-local park index, engine notifications, operator-confirmation auditing, validation warnings, and the ChangesOperator confirmation lifecycle
Estimated code review effort: 4 (Complex) | ~60 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 |
This was referenced Jul 28, 2026
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryThis PR implements
Confidence Score: 4/5Safe to merge. The new command, index, and confirm flow are solid and well-covered. The one area worth a second look is engine.py: three callers of reset_spec_status now propagate FrontmatterWriteError to the engine crash handler for unusual spec shapes by deliberate design. Every new code path has comprehensive tests including ablation tests. The commit sequencing in _apply_confirmation is carefully ordered for recoverability, and ParkedStory.resumable handles the partial-write state. The only open question is whether FrontmatterWriteError in _salvage_review_timeout should crash the engine or return False; the author chose crash, peers flagged it in the prior round, and the author documented the choice rather than changing it. Files Needing Attention: src/bmad_loop/engine.py — specifically _salvage_review_timeout at line 1637, which lets FrontmatterWriteError propagate to the crash handler rather than returning False and letting the timeout-routing logic decide what to do next.
|
| Filename | Overview |
|---|---|
| src/bmad_loop/operatoractions.py | New module: machine-local operator-actions index with ParkedStory dataclass; clean separation of drift(), committed_drift(), and resumable for precise blame-assignment; git-exclude registration via _worktree_local_exclude; all failure modes degrade gracefully. |
| src/bmad_loop/cli.py | Adds cmd_confirm with per-action prompts, drift refusal, spec+board writes ordered for recoverability, and _resume_confirmation for interrupted-confirmation state; _validate_operator_registry reports drift in both directions without blocking runs. |
| src/bmad_loop/frontmatter.py | New _edit_frontmatter_block unifies three independent line-scanners with a YAML-oracle verification pass; FrontmatterWriteError raised (not returned False) for unwritable shapes; set_frontmatter_status now uses this path with comprehensive parametrized tests. |
| src/bmad_loop/devcontract.py | reset_spec_status now delegates to _edit_frontmatter_block; new append_operator_confirmation and has_operator_confirmation; OPERATOR_CONFIRM_HEADING/RE constants kept co-located so writer and reader cannot silently diverge. |
| src/bmad_loop/engine.py | Adds _index_park (best-effort, catches OSError+RuntimeError) and _notify_park after AWAITING_OPERATOR advance; _salvage_review_timeout and _reconcile_generic_terminal_status intentionally leave FrontmatterWriteError uncaught per repair-write doctrine, propagating to engine crash handler for unusual spec shapes. |
| src/bmad_loop/runs.py | rearm_escalation now catches FrontmatterWriteError from both reset_spec_status and set_frontmatter_field, converting to RearmError with appropriate remediation message. |
| src/bmad_loop/tui/app.py | TUI replan path now catches FrontmatterWriteError alongside OSError, preventing an unhandled raise from crashing the Textual dashboard instead of showing an error notification. |
| tests/test_frontmatter.py | New file with characterisation tests pinning deliberate non-preservations plus behaviour tests for every YAML shape the old scanner mishandled; anchor-alias collateral-damage check is the sole gate for the other-keys-unchanged oracle. |
| tests/test_cli.py | 773-line addition covering confirm happy path, per-action prompts, drift refusals, resumable recovery, --yes/--reverify, --list/--json, validate operator warnings, all ablation gates exercised individually. |
| tests/test_operatoractions.py | 449-line test file covering record/drop, resolve, drift/committed_drift/resumable predicates, cross-machine misuse, and actions_of normalizer parity; every predicate branch tested independently. |
Sequence Diagram
sequenceDiagram
participant H as Human
participant CLI as cli_confirm
participant OA as operatoractions
participant FM as frontmatter
participant SS as sprintstatus
participant DC as devcontract
H->>CLI: bmad-loop confirm key
CLI->>OA: resolve(project, paths)
OA-->>CLI: ParkedStory list
CLI->>CLI: story.resumable check
CLI->>CLI: story.drift refusal check
loop per action unless yes flag
CLI->>H: action done prompt
H-->>CLI: confirm
end
CLI->>DC: append_operator_confirmation(spec, actions)
DC-->>CLI: True
CLI->>FM: set_frontmatter_status(spec, done)
FM->>FM: _edit_frontmatter_block oracle verify
FM-->>CLI: True
CLI->>SS: advance(sprint_status, key, done)
SS-->>CLI: done
CLI->>OA: drop(project, key)
CLI->>H: confirmed spec and board done
Reviews (4): Last reviewed commit: "docs(engine,devcontract): name the repai..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_engine.py (1)
1438-1468: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid degrade-not-crash test, but only exercises the
OSErrorbranch.Consider a follow-up test that also covers the worktree-isolation park path (
_park_spec_relpathre-rooting an absolute in-worktree spec path), since that's a distinct branch none of the three new tests exercise.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_engine.py` around lines 1438 - 1468, The existing test only covers record_park raising OSError and does not exercise worktree isolation. Add a follow-up test around the park flow that supplies an absolute spec path inside the worktree, verifies _park_spec_relpath re-roots it correctly, and confirms parking still completes with the expected journal/state outcome.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bmad_loop/cli.py`:
- Around line 1671-1672: Check the boolean results from
append_operator_confirmation and frontmatter.set_frontmatter_status before
calling sprintstatus.advance or reporting success. Escalate when either write is
skipped, while preserving successful idempotent re-runs where
set_frontmatter_status returns False because status is already “done”; do not
advance or remove the story for absent files, missing frontmatter/status, or
other unwritten changes.
In `@src/bmad_loop/engine.py`:
- Around line 1875-1916: Update _park_spec_relpath to catch RuntimeError from
Path.resolve alongside the existing path-resolution exceptions and return the
original spec_file fallback. Extend _index_park’s best-effort exception handling
to catch RuntimeError as well as OSError, allowing symlink-loop failures to be
journaled instead of escaping; ensure the corresponding _finalize_commit_phase
flow preserves this non-fatal behavior.
---
Nitpick comments:
In `@tests/test_engine.py`:
- Around line 1438-1468: The existing test only covers record_park raising
OSError and does not exercise worktree isolation. Add a follow-up test around
the park flow that supplies an absolute spec path inside the worktree, verifies
_park_spec_relpath re-roots it correctly, and confirms parking still completes
with the expected journal/state outcome.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 642a8d3c-7fc9-4c8e-b617-9657863f10bb
📒 Files selected for processing (15)
CHANGELOG.mdREADME.mddocs/FEATURES.mdsrc/bmad_loop/checks.pysrc/bmad_loop/cli.pysrc/bmad_loop/data/settings/core.tomlsrc/bmad_loop/devcontract.pysrc/bmad_loop/documents.pysrc/bmad_loop/engine.pysrc/bmad_loop/operatoractions.pysrc/bmad_loop/policy.pytests/test_cli.pytests/test_devcontract.pytests/test_engine.pytests/test_operatoractions.py
The `## Operator Confirmation` section stops being prose the moment `confirm` learns to resume an interrupted confirmation off it: the heading on disk becomes the machine-readable acknowledgment. Extract the writer's inline literal as OPERATOR_CONFIRM_HEADING plus a reader pattern, and add a fence-aware `has_operator_confirmation()`. Fence-aware for #52's reason, with higher stakes than on the auto-run path: a frozen intent quoting an example section must not let `confirm` conclude a human already signed off and skip straight to advancing the board. `_section_headings` takes the pattern as a parameter rather than growing a second copy of `_fenced`'s open-marker walk. refs #335
`set_frontmatter_status` scanned for `line.lstrip().startswith("status:")` and
broke on the first match, so the writer and `read_frontmatter` disagreed about
what a spec says. Ten shapes all read as a real status: three were a silent
no-op (a quoted key, whitespace before the colon, a flow mapping) and five
corrupted the spec (a block scalar, a value continued on the next line, a nested
key of the same name, a decoy inside another key's literal block, an anchor
another key aliases). Nothing verified what the edit meant, and no caller reads
the return value, so every one of them was invisible.
Enumerating the shapes by pattern is a losing game — mine and both reviewers'
enumerations each missed cases the next one caught. Instead: make the trial edit,
re-parse it with yaml.safe_load as an ORACLE (never a serializer, so the
formatting-preserving minimal edit survives), and keep it only if the block still
parses as a mapping, its top-level status is exactly the target, and every other
key is unchanged. Candidates are iterated rather than broken on, which is what
fixes the wrong-target write.
Split the return contract by meaning: False is "nothing to change" (unchanged, so
no existing test moves), True is a verified rewrite, and a status the reader can
see but no edit can safely move now raises FrontmatterWriteError. False never
again means "I failed" — a silent refusal is the same failure relocated.
The reader still degrades an unparseable block to {} and the writer now raises on
it; that asymmetry is AGENTS.md's observe-degrade / repair-raise doctrine, and is
pinned as a test against the reader's own.
refs #335
…ied edit `verify.set_frontmatter_field` and `devcontract.reset_spec_status` are the same line-scan family as `set_frontmatter_status`, each with its own variant of the same defect: - `set_frontmatter_field` appended a missing key on a SCAN miss, so a quoted `"baseline_revision":` was missed and a second one written — the spec carried the field twice and the reader resolved the wrong one. The insert is now gated on what `read_frontmatter` sees. - `reset_spec_status` reads only a `[A-Za-z-]*` value, so it filled `status: |` to `status: done|` and `status: 123` to `status: done123`, and its `.sub(count=1)` rewrote a nested `meta.status:` in preference to the real key. Both now route through `frontmatter._edit_frontmatter_block`. What is shared is the VERIFICATION, not the rendering: `reset_spec_status` keeps its own renderer because its tests pin quote-style and inline-comment preservation, while `set_frontmatter_status`'s callers read the result back as a bare `status: done`. Formatting is the part all three already had right. refs #335
…g it Audit of every caller that discarded a frontmatter write result, now that one can raise: - `rearm_escalation`'s status flip gets its own `FrontmatterWriteError` arm. This is the case the silent `False` cost most: the spec reads fine, the flip does nothing, the re-drive is dispatched, step-01 sees the unchanged terminal status and routes the session to "ingest as context, do not resume" — the story re-wedges with nothing on the record. It now aborts before `save_state`, so the escalation stays armed. - The `baseline_revision` re-stamp joins its existing tuple; same remedy sentence, and aborting is the point — a stale spec baseline is the hazard that block closes. - The TUI replan handler caught `OSError` only, so an unwritable status would have taken the dashboard down instead of notifying. The engine's four `reset_spec_status` call sites need no change: they already treat it as a raising repair write (its `read_text` has always been unguarded, and `engine.py:1637` says so in a comment). refs #335
`confirm` writes the audit section, then the spec status, then the board, then drops the index entry. Interrupted between the spec half and the board half it leaves a signed-off spec at `done` with the entry still pointing at it — which reads to `drift()` as a stale index, so a re-run refuses the very state a re-run exists to clear. Adds the third reading (`confirmation_recorded`, fence-aware) and the `resumable` predicate over it. The board arm accepts `done` as well as `awaiting-operator`: the interruption message tells the human to fix the board by hand, and a strict test would strand the entry if they did. Splits `committed_drift()` off `drift()` so a caller that already reported the index's own fault can still report a co-occurring board disagreement. No command behavior change yet. refs #335
`_apply_confirmation` discarded both spec-write results and printed `✓ confirmed` regardless, so a spec whose status the writer could not rewrite still advanced the board, dropped the index entry, and left nothing that could find the story again. Each write now has to answer for itself: the audit append's False is fatal and says so honestly, the status writer's FrontmatterWriteError names the shape that blocked it and the fact that a re-run is safe, and the spec is read BACK from disk rather than trusted — symmetric with the board half, which has always checked `landed`. Splits the board/index/commit tail into `_land_confirmation`, which is exactly what an interrupted confirmation still owes, and covers the previously untested `landed != "done"` branch (a quoted board key reaches it with no mock). refs #335
A board write that raised or returned unchanged left a signed-off spec at `done` with the index entry still pointing at it. To `drift()` that reads as an ordinary stale entry, so `confirm` refused the one state re-running it exists to clear — and nothing else would ever drop the entry, leaving `validate` to warn about it forever. `cmd_confirm` now checks `resumable` before the drift refusal and enters the board/index/commit tail directly: no re-prompt, and no second audit section (the one on disk IS the acknowledgment being finished). `--reverify` still runs when asked, and says "NOT advanced" rather than "NOT confirmed", since the confirmation itself already happened. refs #335
`--list`, `--json` and `validate` all read `drift()`, which calls an interrupted confirmation stale — so each told a human that confirm would refuse a story confirm now finishes. Adds the `resumable` arm to all three, under a new `operator.confirm-interrupted` check id: the remedy inverts (re-run confirm vs discard the entry), and check ids split where the remedy does. `_validate_operator_registry` also drops the `continue` after the malformed-actions warning, which silently swallowed a co-occurring board disagreement — `drift()` checks the board before the empty-actions cause, so nothing ever reported it. The malformed arm reports `committed_drift()` rather than string-matching the prose, so a park whose only fault IS the unreadable list is not reported twice. CONFIRM_SCHEMA_VERSION stays 1: additive-only, and every pre-existing field produces the same value for every input. refs #335
`_park_spec_relpath` resolves inside a try catching `(OSError, ValueError)`,
and below 3.13 `Path.resolve` reports a symlink loop as `RuntimeError` —
neither. It is called as an argument expression inside `_index_park`'s own
try, whose handler was `except OSError` only, so the type escaped both.
Severe because of where: `_index_park` runs at the tail of
`_finalize_commit_phase`, outside every try there and after
`advance(AWAITING_OPERATOR)`, so an escape skipped `_notify_park`,
`_emit("post_commit")` and `self._save()` — the park phase was never
persisted, over a write that is best-effort by design.
Both guards, not one: widening only `_index_park` loses the index entry, and
per #356 the entry is the only route from a story key back to its spec.
`_index_park` still needs its own arm because `record_park` reaches
`install._worktree_local_exclude`, which has a bare `.resolve()` outside its
only try.
Also covers `_park_spec_relpath`'s previously untested worktree branch.
Faults are INJECTED: probed on this box, 3.11/3.12 raise RuntimeError on a
real loop and 3.14 returns normally, so a loop-based test would be a false
green locally and red only on the CI legs.
refs #335
Both spec-write refusals land after the audit section is already on disk and both end by asking for a re-run — and `append_operator_confirmation` accumulates, so taking that advice quietly records two sign-offs for one event. Found by smoke-testing a real project, not by the tests, which assert substrings and cannot see what the advice leads to. Only the human can decide whether the first record should stay, so both refusals now say what the re-run will do. refs #335
This was referenced Jul 28, 2026
The byte-exact "every OTHER byte unchanged" assertions failed on both Windows legs. Not a test bug: `set_frontmatter_status` writes with `write_text`, whose universal-newline handling relays EVERY line ending in the file to os.linesep, so on Windows an all-LF spec comes back all-CRLF — from a writer whose contract is "only the status value changes". Fixing the relay is filed as #357 and stays there: it needs `_replace_value` to carry each line's own ending (today it appends a bare \n, which would leave a CRLF file with one LF line), its own CRLF characterization, and it is shared with `devcontract.reset_spec_status`. Modelling it keeps the byte-exact gate — the half that catches a wrong-target write, which no substring assertion has — instead of weakening the assertion. Verified under emulated Windows text-mode semantics: all 27 pass, and reverting the helper to identity fails exactly the 13 tests CI reported. refs #335
Greptile filed a P1 saying the three engine `reset_spec_status` call sites (1637, 2133, 3111) must catch `FrontmatterWriteError` like the siblings guarded in 283a410, because the raise is "a regression introduced by the frontmatter writer hardening that shipped in this same PR". Both premises are false. The weird shapes did not return a silent False pre-PR. At 8a3cf91 a `status: {inline: mapping}` returned TRUE after writing `status: in-progress{inline: mapping}` — a spec whose frontmatter no longer parses; `status: |` and `status: 123` corrupted the same way, and a quoted `"status":` key appended a second status line. The verified edit converts silent corruption reported as success into a loud refusal. Nor is the propagation path new: `reset_spec_status` opened with an unguarded `read_text` from these exact lines, and `strip_auto_run_result` — which raises by identical documented design — sits on the very next line at 1638 and 3112, equally uncaught. `FrontmatterWriteError` adds a type to an already-raising path. Swallowing would be the worse bug. At 3111 it dispatches a repair at a charged attempt against a spec still reading `done`; step-01 ingests such a spec as context and does not resume, so the story re-wedges with nothing on the record — verbatim why `runs.rearm_escalation` aborts instead. So the behavior is unchanged. What the review exposed is that only 1637 says so: a reader arriving at 2133 or 3111 cold has nothing to distinguish a deliberate raise from an oversight, and concluded oversight. Both now carry the doctrine comment, and `reset_spec_status`'s docstring stops citing a stale `engine.py:2115` (the reader is 2133) and the wrong call-site count, naming the function instead so it cannot go stale again. One test pins the decision, which nothing did: a future broad `except` at these sites would have passed the whole suite. Inverse ablation — wrapping 3111 in `except FrontmatterWriteError: pass` fails it on "DID NOT RAISE", and the restore is byte-identical by sha256. refs #335
Collaborator
Author
Re: Greptile P1 — "
|
status: shape |
pre-PR result |
|---|---|
status: {inline: mapping} |
True — wrote status: in-progress{inline: mapping}; frontmatter then fails to parse at all |
status: | |
True — wrote status: in-progress| |
status: 123 |
True — wrote status: in-progress123 |
"status": draft |
True — appended a second status: line |
The prior behavior was silent data corruption reported as success. The verified edit turns that into a loud refusal. Restoring the old return would restore the bug — and issue #96 already ruled on exactly this function: "The repair path deliberately keeps the opposite contract. reset_spec_status and strip_auto_run_result still raise on a present-but-unreadable spec: silently skipping a rewrite the caller believes succeeded would leave the re-opened spec carrying its stale terminal section."
2. The propagation path is not new either
Pre-PR, reset_spec_status opened with an unguarded text = spec_path.read_text(encoding="utf-8") immediately after its is_file() guard — so these exact lines could always raise OSError into that same handler. And devcontract.strip_auto_run_result, which raises by identical documented design, sits on the very next line at engine.py:1638 and 3112, equally uncaught, and has for far longer. FrontmatterWriteError adds an exception type to an already-raising path; it does not create a propagation path.
This was audited two commits before you reviewed, in 283a410:
The engine's four
reset_spec_statuscall sites need no change: they already treat it as a raising repair write (itsread_texthas always been unguarded, andengine.py:1637says so in a comment).
3. Swallowing would be the worse failure
At 3111 specifically, catching it dispatches a repair session at a charged attempt against a spec still reading done. Step-01 routes such a spec to "ingest as context, do not resume", so the story re-wedges with nothing on the record explaining why. That is verbatim the reasoning in runs.py:844, where the sibling writer chose to abort rather than degrade. Worth noting too that the "crash" is a recorded, resumable stop: _run_inner's finally always saves, crash.txt holds the traceback, and clear_pause() clears crashed so a resume re-arms like a stopped run.
Two corrections
- There is no
_reconcile_prosein this repo. The method at2133is_reconcile_generic_terminal_status(engine.py:2057);"reconcile-prose"is only the journal site string atengine.py:2128, which is where the name likely came from. - Reachability is wider than argued, which cuts the other way. Because
_FM_STATUS_REcaptures only[A-Za-z-]*,reset_spec_statusalso refusesstatus: needs_reviewandstatus: ready for dev— far likelier in a real spec than a flow mapping, and previously corrupted silently. That strengthens the case for the refusal, not against it.
What did change — 556267e
The finding is still evidence of a defect: only 1637 carried the doctrine comment. A careful reader arriving at 2133 or 3111 cold had nothing distinguishing a deliberate raise from an oversight, and concluded oversight. So:
2133and3111now carry the doctrine comment1637had.reset_spec_status's docstring stops citingengine.py:2115(stale — the return-reader is2133) and the wrong call-site count, naming the function instead so it cannot go stale again.- One test pins the decision, which nothing did — a future broad
exceptat these sites would have passed the entire suite. Inverse ablation: wrapping3111inexcept FrontmatterWriteError: passfails it onDID NOT RAISE, and the restore is byte-identical bysha256sum -c.
Scope note: the test pins _reset_spec_for_repair only — the site centred on, and the one with the strongest argument. A test apiece for the other two would triple the diff to pin the same single decision; happy to add them if a maintainer prefers.
The byte-preservation gap in set_frontmatter_status remains tracked separately in #357.
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.
Part 3 of 4 of the awaiting-operator program —
refs #335 (3/4). Part 4 (the review-demotion knob) is still outstanding, so #335 stays open after this merges.Builds on #347 (vocabulary) and #348 (park entry).
What this adds
bmad-loop confirm <story-key>— the exit from a park:--yesskips the prompts). One blanket "all done?" invites exactly the failure this state exists to prevent — a story marked done with some obligations quietly unmet.## Operator Confirmationaudit section. This is the whole audit trail for the part of a story that happened outside the repo: the commit shows what the agent did; nothing in git can show that someone bought the domain. Actions are restated rather than referenced, so a later reader can see whether the spec was edited between the park and the sign-off.done— an ordinary forward move throughsprintstatus.advance, sinceawaiting-operatorsits immediately belowdoneinSTATUS_ORDER. No invariant exception.verify.commit_paths(best-effort, asdecisions.apply_pre_answer).--reverifyre-runs the project's[verify]commands first and blocks the confirmation on failure — the external action may have changed what the tests see.--list/--jsonproject the parked set (CONFIRM_SCHEMA_VERSION = 1, builder indocuments.py).It touches no run state:
statemachine.pyalready documentsAWAITING_OPERATORas terminal with no legal transition, precisely because confirm completes a story out of band.Also lands what part 2 split out
#348 deferred the registry and the park notification at the 800-LOC cap. Both are here, since they are this command's data store and its prompt:
operatoractions.py—.bmad-loop/operator-actions.json, mirroringdecisions.py's store shape.bmad-loop confirm <key>. Notify-only: a park is non-blocking, so nothing may read as "the run stopped for you".The one design call worth a reviewer's attention
The index is machine-local and never committed, registered in the repository's local git exclude (reusing
install._worktree_local_exclude, which worktree provisioning already uses and which resolves--git-common-dir, so it lands in the main repo even from a linked worktree).This is a deviation from
decisions.json, which is committed, so it needs justifying. The index is written from inside a story's commit window, where every way of committing it is wrong:HEADpast thecommit_shathe park just stamped, and underscm.isolation = "worktree"advances the target branch while the unit branch is still out, so anscm.merge_strategy = "ff"merge-back fails outright;git add -Asweeps one story's bookkeeping into another story's commit.Excluding it sidesteps all three.
verify.dirty_pathsalready ignores.bmad-loop/wholesale, so the merge path was never at risk;worktree_cleandoes not, which is what the exclude buys.The cost, stated plainly because
confirmhas to answer for it: the index lives on the machine that ran the orchestrator, so that is where a park is confirmed. A fresh clone cannot rebuild one —spec_fileis reported by the dev session in its result JSON and is not derivable from a story key, so the path to a parked story's spec survives only in the index.confirmrefuses and says so rather than guessing with a glob. Cross-machine confirmation needs a committed index plus the park-commit sequencing to go with it; I'll file that as a follow-up rather than fake it here.Because the index can still drift from the truth it points at,
confirmrefuses a drifted entry (never flipping a board on the index's word alone) andvalidatewarns in both directions —operator.registry-stale(entry the committed state moved past, or a board parked with no entry) andoperator.actions-malformed(parked spec declaring nothing readable). Warnings only: confirm already refuses, so a stale index must not block a run that would otherwise start. Both ids registered inchecks.VALIDATE_CHECKS.Design questions answered
Per #335's "proceed on the proposed defaults unless a maintainer says otherwise":
--reverifyopt-in (the original design's answer).actions_ofreads through the samefrontmatter.operator_actions_ofnormalizer as the spec, so a v2[{action:, check:}]shape collapses to()on both sides rather than disagreeing.gates.notify's normal file sink.Epic-retro amendment
Not needed: the
(M awaiting operator)run-summary clause landed in part 1 (RunSummary.render, appended only when non-zero)._epic_boundarycarries no counts to amend.Ablations (repo rule)
Eight run, each confirmed to fail its test, each source file restored byte-identical afterwards:
_exclude_from_gitin_write_storedrift()armactions-malformedarmcmd_confirmdrift guard_reverifyreturn ignoredsprintstatus.advancein_apply_confirmation_index_park/_notify_parkcallsVerification
uv run pytest -q -n auto→ 3573 passed, 24 skipped. Fulltrunk checkclean. Also smoke-tested against a real sandbox project:--list,--json,--yes, the interactive accept and decline paths, the resulting spec/board/commit, and the second-confirm refusal.Review response (second push)
Both bots posted four inline findings. All four were valid, two were worse than reported, and validating them surfaced a pre-existing defect family in
frontmatter.pythat is the reachability proof for the first. Every thread has a reply with the detail; the short version:8660f61— assert the resulting state, not the return value; the spec is read back from disk97cc210+21af3b5— the half-applied state is now recoverable, not just documentedecc98e3—committed_drift()split offdrift(), no prose string-matchingRuntimeErrorescapes the park guards3b6f40b— both escape paths, fault injected rather than built from a real symlink loopThe frontmatter family (
c196b3f,38b065a,9f3382b,283a410) is deliberately kept as isolated commits so it can still be lifted into its own PR ahead of this one if preferred.set_frontmatter_statusscanned for the status line withlstrip().startswith("status:")while every reader parses the block as YAML — silent no-ops on quoted keys and flow mappings, and outright corruption on block scalars, continued values and nested keys. It now makes the trial edit, re-parses it withyaml.safe_loadas an oracle, and keeps it only if the top-levelstatusis the target and every other key is unchanged.Two things the tests could not have found:
378aeb3).RuntimeErrorforOSErrorin non-strict mode — it stopped raising at all, and the project venv here is 3.13. A real-symlink-loop test would have been a false green on the default test interpreter.CONFIRM_SCHEMA_VERSIONstays 1: evolution is additive-only, and every pre-existing field produces the same value for every input. Pinned as== 1in the new JSON test so the no-bump is deliberate.Follow-ups filed as issues: #357, #358, #359, #360.
Verification (updated)
uv run pytest -q -n auto→ 3644 passed, 24 skipped (branch point was 3573). Fulltrunk check --no-fixclean over 22 files —--no-fixbecause ruff's F401 autofix deletes the re-export atverify.py:23.uvx pyright@1.1.411(the CI pin) clean. Park tests re-run explicitly on 3.11, where the symlink-loop type is live.Every negative assertion was ablated: the gate deleted, the matching test confirmed to FAIL, then restored by targeted replacement and verified byte-identical by
sha256sum -c. Where defense in depth means an ablation bites on the message rather than the outcome, the test asserts the specific wording and the docstring records that — noted because the plan for this predicted otherwise.Summary by CodeRabbit
New Features
bmad-loop confirm <story-key>to complete stories parked for human operator action, including--list,--yes,--reverify, and--json.done, and removes only the local machine’s operator-action park record.Bug Fixes
validatenow reports additional operator drift/staleness issues as warnings (including interrupted confirmations) and marks drift inconfirm --list.Documentation
--jsonoutput.