feat(engine): park dev-declared awaiting-operator stories through commit - #348
Merged
Conversation
Makes the state PR-1 named reachable. A dev session whose story needs an action only a human can take outside the repo finishes and COMMITS everything an agent can do, enumerates what is owed in the spec's `operator_actions:` frontmatter, and parks — the run moves on instead of stopping. The two outcomes such a story had before were both dishonest: `done` hides the outstanding work behind a green board, `blocked` halts the run over work the loop was never going to do. - devcontract: `awaiting-operator` is a terminal beside done/blocked, folding a validated action list into the result; no escalation is synthesized, which is what keeps it out of the run-halting pause channel - verify_dev/verify_review accept the (awaiting-operator, awaiting-operator) pair, selected by the OBSERVED spec status, and refuse a park that enumerates nothing with a fixable retry naming the shape - the park rides the normal commit path: review loop skipped (nothing in the diff for it to converge on), same deterministic gates, and `_finalize_commit_phase` picks the final phase from the task's actions - actions are latched before advance(COMMITTING), so the crash-resume arm re-derives AWAITING_OPERATOR with no code of its own - worktree units merge on a park like a done one — it carries a commit - `[operator] enabled` (default true); stories mode and sweep bundles opt out, their verify tails cannot gate the pair refs #335 (2/4)
WalkthroughStories can now commit and continue when human-only actions remain, using an ChangesAwaiting-operator parking
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 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 SummaryAdds the entry path for the
Confidence Score: 5/5Safe to merge. The park path is well-gated at both the dev-verify and review-verify layers, crash recovery is correct, and subclass opt-outs are typed. Every invariant is enforced by a separate gate layer with a corresponding ablation test. The repair path for blank-action parks routes through Action.RETRY in _drive_dev, so task.operator_actions is always set from already-validated frontmatter before _park_awaiting_operator is called. 3498 tests pass and pyright reports zero errors. Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| src/bmad_loop/engine.py | Core park entry path: _park_awaiting_operator, _finalize_commit_phase final-phase rule, sprint mirror, dev prompt injection, and _operator_park_enabled override seam. Crash-safety is correct — actions are latched before advance(COMMITTING). |
| src/bmad_loop/verify.py | Adds AWAITING_OPERATOR constant, _operator_actions_gate, and the (awaiting-operator, awaiting-operator) pair acceptance in verify_dev and verify_review. The fm= passthrough preserves the single-read contract. |
| src/bmad_loop/frontmatter.py | Adds operator_actions_of: strict container check, lenient scalar normalization, dict/list/None items dropped. Deduplication via dict.fromkeys preserves order. |
| src/bmad_loop/devcontract.py | AWAITING_OPERATOR added as a third terminal; operator_actions folded into result only on that status. No escalation synthesized — deliberate to preserve the done-vs-park distinction. |
| src/bmad_loop/worktree_flow.py | integrate_unit now merges on DONE or AWAITING_OPERATOR. A parked story carries a committed change that must not be stranded on a torn-down unit branch. |
| src/bmad_loop/policy.py | Adds OperatorPolicy with enabled=True default. Template and dataclass kept in sync (tested). |
| src/bmad_loop/stories_engine.py | Overrides _operator_park_enabled to False — explicit typed opt-out. Stories mode has no sprint board. |
| src/bmad_loop/sweep.py | Overrides _operator_park_enabled to False — bundles carry no sprint-status entry, so the verify pair does not exist. |
Reviews (2): Last reviewed commit: "docs(test): drop the split-out registry ..." | Re-trigger Greptile
Greptile review, two P2s. `verify_review` read `policy.operator.enabled` directly while `verify_dev` took an engine-supplied `operator_park`. `_operator_park_enabled` is an override seam, so a mode that opts out of parking while still reaching the base gate would have found it accepting a park the engine refuses to take — and PR-4's demotion knob edits exactly there. One flag, conservative default, with the ablation to prove the gate bites. The settings description and policy template also promised `.bmad-loop/operator-actions.json`, which now ships with `bmad-loop confirm` in part 3 — they name the spec frontmatter instead. refs #335 (2/4)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/test_engine.py`:
- Around line 1250-1254: Update the test docstring describing the end-to-end
session scenario to remove the claim that the registry and notification record
what is owed. Keep the statements about the story committing, the board reaching
the token, and the run advancing to the next story, matching the assertions
covered by this test.
🪄 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: fd491a75-0cb2-4613-b7c0-a5ebabda7d14
📒 Files selected for processing (17)
CHANGELOG.mdsrc/bmad_loop/data/settings/core.tomlsrc/bmad_loop/devcontract.pysrc/bmad_loop/engine.pysrc/bmad_loop/frontmatter.pysrc/bmad_loop/policy.pysrc/bmad_loop/stories_engine.pysrc/bmad_loop/sweep.pysrc/bmad_loop/verify.pysrc/bmad_loop/worktree_flow.pytests/conftest.pytests/test_devcontract.pytests/test_engine.pytests/test_engine_worktree.pytests/test_policy.pytests/test_settings_schema.pytests/test_verify.py
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.
refs #335 (2/4)
Part 2 of the four-PR
awaiting-operatorprogram: the entry path. PR-1 named the state at every layer with no writer; this makes it reachable.bmad-loop confirmis part 3, review-demotion composition is part 4.#335 stays open after this merges — parts 3 and 4 are still outstanding.
The change
A dev session whose story needs an action only a human can take outside the repo — buy a domain, publish a DNS record, grant an API key — completes everything an agent can do, commits it, enumerates what is owed under
operator_actions:in the spec frontmatter, and parks. The run continues to the next story.Before this, such a story had two outcomes and both were dishonest:
donehides the outstanding work behind a green board (the reporter's original 40-unticked-entries artifact), andblockedhalts the entire run over work the loop was never going to do.frontmatter.pyoperator_actions_of— strict container, lenient scalar items. Lives here becauseverifycannot importdevcontractdevcontract.pyAWAITING_OPERATORterminal besideDONE/BLOCKED(synthesis, missing-marker scan); folds the validated list into the result.RECONCILABLE_FROMunchangedverify.pyverify_dev/verify_reviewaccept the(awaiting-operator, awaiting-operator)pair and require a non-empty list on itengine.py_park_awaiting_operator, the final-phase rule in_finalize_commit_phase, the sprint mirror, the injected prompt contract,_operator_park_enabledworktree_flow.pyintegrate_unitmerges onDONEorAWAITING_OPERATORpolicy.py+core.toml[operator] enabled = truestories_engine.py,sweep.pyDesign notes
No
_defermachinery. A park is a success that commits — no stash, no rollback, no ledger snapshot to unwind. What separates it fromDEFERRED/ESCALATEDis precisely that it carries a commit.The pair is selected by the observed spec status. The skill decides whether it parked; the gate then holds it to the matching board state and to a non-empty action list. A park declaring nothing is refused with
fixable=True, so the reason goes to a repair session as feedback rather than throwing a real tree away — a park is defined by owing at least one action, and confirming a blank later would be meaningless.The park still verifies. It skips only the review loop, because a review pass is
bmad-dev-autore-invoked to second-guess the diff, and a park's outstanding work is not in the diff at all — every cycle would either re-park (budget burned, no progress) or "fix" the park away by finalizingdone, which is the exact false-green the state exists to prevent. It delegates to the same skip-review commit path, so it clears the spec/board pair, the action list, and the project's verify commands. No commit path in the engine skips verification.The board mirror is a forward advance.
awaiting-operatorsits immediately belowdoneinSTATUS_ORDER, so_post_dev_state_synctargeting it is an ordinary forward move through the sole writer — no exception to never-regress, and part 3'sconfirmadvances the same board the same way. Mirrored on the status alone, before the actions are validated:verify_devowns the "declared nothing" retry, and its feedback reads far better against a board that already agrees with the spec.Crash-resume needed no new code. The actions are latched onto the task before
advance(COMMITTING), so the existing#115resume arm re-enters_finalize_commit_phaseand its final-phase rule (AWAITING_OPERATORifftask.operator_actions) reaches the same verdict the pre-crash run would have.The dev prompt instruction is engine-injected, and interim. The durable home is upstream —
bmad-dev-auto's spec template and step-03/04 finalize rules — and that PR is not landed (part of the program's upstream track). The "never useblockedfor this" clause is the load-bearing half:blockedis the skill's existing escape for "I cannot finish", it is exactly the shape a human-only action tempts, and it halts the run. The instruction is deliberately backtick-free — it is appended after the repair prompt's feedback pointer, and the last backticked token in a dev prompt is by convention that path.Commit message suffix
(awaiting operator)is appended to a renderedcommit_message_templatetoo: the template governs the message's shape, but whether the story is finished is a fact about the commit, andgit logis where an operator looks long after the run summary scrolled past.The sketch's open questions — choices made here
_close_declared_deferredat park commit, or only at confirm?_finalize_commit_phase, and it is right: the code that closes those entries is in this commit. Closure is advisory and sweep-re-verified.check:command stays a deliberate v2 question. Non-scalar items are dropped rather thanstr()-ed, so a v2-shaped declaration reads as malformed and gets the retry, instead of handing a human a line of Python repr as their instruction.verify_review_storiesstill demandsdone; parking there would commit on the spec's word alone.StoriesEngine(andSweepEngine, for the same reason on bundles) override_operator_park_enabledtoFalse— an explicit opt-out, not an accident of where the branch sits.[operator] enableddefault-on or opt-in?Scope note — registry + notification moved to part 3
Whole-PR size with tests came to ~1040 lines, over the 800 cap. Per the phase plan's own instruction I split out
operatoractions.py(the.bmad-loop/operator-actions.jsonregistry writer) and the park notification, which now land withbmad-loop confirmin part 3. That is also the better seam: the registry isconfirm's data store and the notification is its prompt, so shipping them here would have added a store nothing reads.For now a parked story's obligations live in its spec frontmatter and in the
story-awaiting-operatorjournal entry, which carries the actions themselves rather than a count. The run summary,status, and the TUI already surface the phase (part 1). Noted on #335.Tests
At the lowest layer that catches each regression:
devcontractunit (terminal acceptance, the seven action shapes, no synthesized escalation, fold-only-on-park, the missing-marker scan);verifyunit (pair selection both directions, malformed → fixable retry, policy-off, regression arm stays scoped to thedonepair); engine mock-adapter E2E (park commits +commit_shastamped + board reaches the token + run continues to the next story + no rollback/preserve-ref, malformed park repaired not committed, policy-off defers); worktree merge; crash-resume throughCOMMITTING.Ablations run (repo rule) — each confirmed to make the named test fail, then reverted:
_review_and_commit→ the park E2E fails requesting an unscripted review session, leaving the story atREVIEW_RUNNING. The E2E usesreview.trigger = "always"precisely so this bites — under therecommendeddefault a story recommending no follow-up already skips the loop, and the branch could have been deleted with no test noticing.integrate_unitback to== Phase.DONE→ the worktree test fails with the story's change absent from the target branch.advance(COMMITTING)→ the resumed story landsDONE, silently dropping the obligation.verify_reviewignoreoperator_park→ the "refused without the engine flag" test fails by verifying a park green.Behavior changes to existing paths
[operator] enabled(two exact-prompt assertions updated to match).engine.py) accepts the new terminal, kept in lockstep withdevcontract.is_frontmatter_candidate— a status accepted by the scan but refused by the repair would leave a park harvested but permanently un-markered._verify_shared_gatesgained an optionalfm=passthrough soverify_devcan read frontmatter once to choose the expected pair without breaking the single-read contract.Review round
Both Greptile P2s accepted, plus one CodeRabbit finding.
verify_reviewreadpolicy.operator.enableddirectly whileverify_devtook an engine-supplied flag. Not a live bug — every other_commitcaller is gated ondone, and both subclasses override_verify_review— but_operator_park_enabledis an override seam, so the asymmetry meant a mode opting out of parking could still find the base gate accepting a park the engine refuses to take, and part 4's demotion knob edits exactly there. Now one flag, conservative default, with ablation 5 proving it bites..bmad-loop/operator-actions.json, which moved to part 3 in the size split — they name the spec frontmatter instead. Same for a stale test docstring (CodeRabbit).uv run pytest -q -n auto: 3499 passed. Fulltrunk check: clean.pyright@1.1.411: 0 errors.