feat(resolve): intent-gap patch-restore re-drive + inline-review-layer preflight (re-cut)#86
Merged
Merged
Conversation
…r preflight Integrate the July 4-6 upstream bmad-dev-auto changes into bmad-loop. Phase A - provisioning + doc alignment: - install: require bmad-review-verification-gap (BMAD-METHOD #2550) and a bmad-dev-auto customize.toml marker in the base-skill preflight; the three inline step-04 review hunters are now all bmm prerequisites - devcontract: refresh the in-review reconcile note (restore path re-arms TO in-review pre-session; reconcile semantics unchanged) - verify: document finalize_commit's intentional residual-artifact squash (#2563) - docs: README skill table, FEATURES review-layer + resolve notes, CHANGELOG Phase B - intent-gap patch-restore re-drive (BMAD-METHOD #2564): A review-stage intent-gap halt now saves the reverted attempt as a patch file. When the attempted reading was correct, bmad-loop resolve re-arms the spec to in-review and re-applies that patch onto the baseline before the re-drive, so the session resumes review (step-04) on the restored diff instead of re-implementing from scratch. - model.StoryTask.restore_patch latch; verify.apply_patch; runs.rearm_escalation restore branch (in-review vs ready-for-dev + restore journal field) - engine: Engine._restore_patch applied in the dev loop gated on feedback-is-None (escalates rather than dispatch on apply failure; cleared on commit) - cli: --restore-patch flag + strict validation + resolution.json passthrough - resolve.read_resolution; bmad-loop-resolve SKILL.md guidance Sweep-workflow extension tracked in #75. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Re-cut of 29ba684 onto post-stories main (supersedes closed PR #76). Conflicts (model.py, runs.py, test_model.py, CHANGELOG) resolved as a union of both features, folding the restore branch into the stories-hardened re-arm: the in-review flip joins the UnicodeDecodeError-guarded status-flip path, story-escalation-resolved carries baseline+restore, and a patch-restore re-arm on a sentinel-wedged story is rejected outright before any mutation (T1: a sentinel is a pre-planning halt — never lay implementation onto the tree ahead of a planning leg).
… (T1) The guard itself landed inside the re-cut's runs.py merge resolution. Covers both sides: rejection leaves the sentinel on disk, the task ESCALATED, no latch persisted and nothing journaled; a real stories spec (sentinel_kind unset) stays a legitimate restore target.
Restore is an in-place-only recovery: a worktree re-drive discards and re-mounts the unit's worktree (engine._finish_inflight), so a latched patch would silently never restore. Reject before any re-arm, mirroring the missing/outside-project rejections; also add the audit's missing outside-project-but-existing-file rejection test.
…nced re-arm baseline (T2) Since #78 re-arm advances the baseline to the post-resolve HEAD while the saved patch diffs from the ORIGINAL baseline, 'applies cleanly by construction' held only when the resolve session left the patched files untouched. Docstring + rearm docstring already state the conditional; the resolve SKILL.md now warns that resolution commits overlapping the patch's files make the restore fail loudly (omit restore_patch when the resolution already carries the work). Combined engine test drives the full interaction: resolution commit rewrites the patched line, restore apply fails, the story re-escalates with no session dispatched and the resolution commit untouched.
The intent-gap patch file is untracked halt residue under the protected artifact dirs — it survives every reset, so with #79's file-granular excludes its mere presence would let a restore re-drive whose session produced nothing pass the 'no changes since baseline' gate. Thread the task's restore_patch into verify_dev_exclude_relpaths at all three proof-of-work call sites (sprint, bundle, stories) so the gate keys on the applied work, not the patch that carried it.
The two lines the source plan targeted but 29ba684 missed: the README review-disabled paragraph and the FEATURES capability-matrix row still said two-layer / 2 parallel layers. Also note the new restore guards in the CHANGELOG bullet.
…folds The stories/sprint sandbox scaffolds were written on the stories branch before the intent-gap preflight upgrade landed, so they hand-roll the OLD base-skill set — the run now fails preflight wanting bmad-review-verification-gap and bmad-dev-auto's customize.toml marker. Install both stubs in both scaffolds.
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds patch-restore recovery for intent-gap review halts, including CLI re-arm support, engine patch reapplication, restore-aware verification gates, and updated review-layer requirements and documentation. ChangesIntent-gap patch-restore feature
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/runs.py`:
- Line 569: The comment in the story escalation guard uses a non-ASCII
multiplication symbol, which Ruff flags. Update the wording in the relevant T1
guard comment inside the runs-related logic to use the ASCII character x instead
of ×, keeping the rest of the note unchanged.
In `@tests/test_engine.py`:
- Line 2229: The docstring in the test around the baseline advance scenario uses
a non-ASCII multiplication sign, which Ruff flags. Update the docstring in the
affected test to replace the `×` character with plain ASCII `x` while keeping
the rest of the wording unchanged.
In `@tests/test_resolve.py`:
- Line 341: The docstring in the story test uses a non-ASCII multiplication
symbol, which Ruff flags as invalid style. Update the affected docstring in the
test around the story/patch-restore description to use the ASCII character x
instead of ×, keeping the wording otherwise unchanged.
🪄 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
Run ID: 730ac306-29f7-4aa9-8593-5c2c7b5efc30
📒 Files selected for processing (20)
CHANGELOG.mdREADME.mddocs/FEATURES.mdsrc/bmad_loop/cli.pysrc/bmad_loop/data/skills/bmad-loop-resolve/SKILL.mdsrc/bmad_loop/devcontract.pysrc/bmad_loop/engine.pysrc/bmad_loop/install.pysrc/bmad_loop/model.pysrc/bmad_loop/resolve.pysrc/bmad_loop/runs.pysrc/bmad_loop/verify.pytests/test_cli.pytests/test_engine.pytests/test_install.pytests/test_model.pytests/test_resolve.pytests/test_runs.pytests/test_stories_e2e.pytests/test_verify.py
Collaborator
Author
|
augment review |
|
Looks like you're out of credits for this review. Head to your account settings to top up and keep reviews running. https://app.augmentcode.com/account/subscription |
…abbit) CodeRabbit (Ruff RUF002/RUF003) flagged the U+00D7 MULTIPLICATION SIGN in three comment/docstring sites; swap them to ASCII 'x'. Also fixes two sibling occurrences the review missed in the same PR (tests/test_verify.py T4 docstrings). Intentional × count glyphs (CLI/TUI) and – en-dash parsing regexes are left untouched.
Collaborator
Author
|
Addressed in e6ef573. All three RUF002/RUF003 ambiguous-
While fixing, I also caught two sibling occurrences the review didn't flag — same class, introduced by this PR's T4 verify tests — and fixed them in the same commit:
Deliberately left untouched: the intentional |
…ter (review F1) A bare '/bmad-dev-auto <key>' never takes step-01's spec-pointer intent check, so the in-review status the re-arm set could not route to step-04 — and the freeform/epic path's version-control sanity check HALTs blocked on the very diff _restore_patch just applied, re-escalating every sprint-mode restore after one burned session. Point the dispatch at task.spec_file (preserved by re-arm for exactly this) so step-01 EARLY EXITs to step-04 before that check. Stories mode already early-exits via folder+id dispatch and is untouched.
…e re-arm (review F2) The in-review route skips step-03 — the only step that stamps baseline_revision — so the re-driven step-04 diffed (and on an intent-gap/bad-spec re-triage, reverted) 'since' the ORIGINAL pre-attempt sha, clawing back the non-overlapping resolve-session commits the baseline advance explicitly blesses as the re-drive's starting point. rearm_escalation now re-stamps the field to the advanced task baseline on the restore path (loud RearmError on failure); from-scratch re-arms leave it alone (step-03 owns it there). New verify.set_frontmatter_field does the same line surgery as set_frontmatter_status but inserts a missing key.
…ly re-driving from scratch (review F3) read_resolution collapsed a present-but-unparseable resolution.json into None — indistinguishable from 'nothing recorded' and quieter than absence (the produced-marker warning keys on existence). Likewise --restore-patch '' (unset shell var) fell through 'if not raw' as a plain re-arm, even masking a recorded restore. Both silently consumed the escalation (a re-armed task can't be re-resolved), losing the human's confirmed restore decision. read_resolution now returns None only for an ABSENT marker and raises ResolutionError otherwise; _resolve_restore_patch errors on an empty flag and on an empty or non-string restore_patch field.
The latch was assigned unconditionally while the in-review routing flip lives under 'if task.spec_file:' — so an escalation with no recorded spec (ambiguous two-file wedge, unknown --story selector, session died before naming one) latched the patch with no routing target, and the engine would lay it onto the tree before a planning leg. Rejected at the T1 seam, before any mutation; from-scratch re-arms unaffected.
…oots (review F5) bmad-dev-auto contractually saves the intent-gap patch under implementation_artifacts, and artifact dirs configured outside the project tree are a supported layout (bmadconfig keeps them absolute; verify special-cases them throughout) — but the containment check was a bare is_relative_to(project), so that layout rejected every legitimate restore with an error blaming the path. Validate against the same trusted roots as verify.spec_within_roots instead. Knock-ons are safe: the T4 proof-of-work exclude skips out-of-project paths (which git can't count as changes anyway) and git apply takes absolute patch paths. Restore tests now provision the minimal _bmad config every real project has.
…t would invalidate (review F6) The worktree-isolation rejection ran only AFTER resolve.run_session — the agent (with no isolation signal in its context and none in SKILL.md) would legitimately negotiate a restore, record it, and only then hit an rc=1 whose naive retry unlinks resolution.json and repeats the whole session; the only deterministic escape (--no-interactive) was unnamed. Now: the explicit --restore-patch flag is validated before the session (policy loaded once, killing the double TOML parse); build_context exports restore_supported and SKILL.md tells the agent never to negotiate a restore when it is false (both reseeds updated); the rejection message names the --no-interactive escape; and the guard keys on the recorded task.worktree_path as well as live policy, so a policy edit between escalation and resolve can't skew it.
The LOW-4 prose sweep (84d4da1) claimed completion but missed docs/setup-guide.md and the _require_base_skills docstring.
…opped silently (review F8) The escalation modal's Re-arm button is enabled by resolution.json's existence yet re-armed with no restore latch — the always-assign semantics actively select from-scratch — so a restore_patch the human confirmed through the interactive resolve flow was discarded without a word (reachable via resolve -> 'n' at the confirm -> TUI Re-arm). Auto-honoring from the TUI would be worse (a consumed marker is indistinguishable from a fresh one on disk), so Re-arm stays a plain from-scratch re-drive; but the modal hint now flags the recorded restore and the re-arm notifies that only 'bmad-loop resolve' applies it. An unreadable marker counts as recorded (it may carry one).
…(scenario 7) The fake dev CLI grows the #2564 choreography: a committed .intent-gap-<story> marker makes the first dispatch save the attempted change as a patch, revert, and block; an in-review re-drive asserts the two orchestrator contracts the way real step-01 would — the prompt must carry an explicit spec pointer (F1) and the attempt must already be restored onto the tree — blocking loudly otherwise, then resumes review to done without re-implementing. The test also pins the F2 re-stamp (spec baseline_revision == post-re-arm HEAD) and that the restored line lands exactly once in the story commit.
This was referenced Jul 8, 2026
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 `@src/bmad_loop/resolve.py`:
- Around line 58-61: `read_resolution` currently misses `UnicodeDecodeError`
from `path.read_text(encoding="utf-8")`, so corrupt UTF-8 in `resolution.json`
escapes instead of becoming `ResolutionError`. Update the `except` handling in
`resolve.read_resolution` to catch `UnicodeDecodeError` alongside the existing
unreadable/JSON cases, and keep wrapping it in `ResolutionError` with the same
path context so `cli._resolve_restore_patch` and `tui.app._restore_recorded`
continue to handle it cleanly.
🪄 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
Run ID: 6fdfed7c-abcb-46b2-b059-a7d010135f35
📒 Files selected for processing (14)
docs/setup-guide.mdsrc/bmad_loop/cli.pysrc/bmad_loop/data/skills/bmad-loop-resolve/SKILL.mdsrc/bmad_loop/engine.pysrc/bmad_loop/resolve.pysrc/bmad_loop/runs.pysrc/bmad_loop/tui/app.pysrc/bmad_loop/tui/screens/modals.pysrc/bmad_loop/verify.pytests/test_cli.pytests/test_engine.pytests/test_resolve.pytests/test_stories_e2e.pytests/test_tui_app.py
✅ Files skipped from review due to trivial changes (2)
- docs/setup-guide.md
- src/bmad_loop/data/skills/bmad-loop-resolve/SKILL.md
🚧 Files skipped from review as they are similar to previous changes (3)
- src/bmad_loop/engine.py
- src/bmad_loop/cli.py
- tests/test_engine.py
…ot UnicodeDecodeError (CodeRabbit) UnicodeDecodeError is a ValueError, not an OSError, so it escaped read_resolution's except tuple as a crash instead of the clean fail-loud abort F3 added — same bug class as the read_frontmatter / stories read-path hardening.
Collaborator
Author
|
augment review |
|
Looks like you're out of credits for this review. Head to your account settings to top up and keep reviews running. https://app.augmentcode.com/account/subscription |
|
✅ Action performedReview finished.
|
This was referenced Jul 8, 2026
pbean
added a commit
that referenced
this pull request
Jul 9, 2026
…k bundles (#75) (#93) An escalated sweep bundle can now be resolved with `bmad-loop resolve <run> --restore-patch <path>`, re-arming the bundle spec to `in-review` and re-applying the saved patch on the re-drive — the same recovery sprint/story runs got in #86. Bundles lack sprint-status entries and SweepEngine overrides `_loop` wholesale, so it never reached the base `_finish_inflight` re-drive wiring; two small deltas close the gap (the apply seam, latch-clear on commit, proof-of-work exclusion, rearm, and CLI resolve->resume were already inherited/pre-wired): - `_generic_bundle_prompt`: point a restore re-drive at the in-review bundle spec (task.spec_file) instead of the fresh intent.md, so step-01's spec-pointer intent check early-exits before its version-control sanity check HALTs blocked on the restored dirty tree (mirrors the sprint F1 fix, engine.py:2133-2141). - `_run_bundle` recovery: latch `resolved_redrive` and pass `cause="resolved"` for a re-armed task, mirroring `_finish_inflight` (engine.py:1151-1157). This is a correctness fix — without the latch a non-critical exhaustion mid-re-drive silently DEFERs the human-resolved escalation (escalation.py:110). It also repairs the pre-existing from-scratch sweep re-drive, which had the same bug. Tests: 4 engine cases in test_sweep.py (restore prompt, restore re-drive -> done + latch cleared + prompt-points-at-spec, pause-not-defer exhaustion for the restore and from-scratch paths) + an escalating-dev conftest helper; E2E scenario 8 drives the real sweep/resolve/resume binaries through tmux (the only coverage of the sweep-specific CLI resume path). All new tests fail without the fix. Full suite 1695 passed / 1 skipped; trunk clean.
This was referenced Jul 9, 2026
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.
Re-cut of
29ba684(the tip offeat/intent-gap-patch-restore) onto post-storiesmain(#65 / #77 / #82). Supersedes #76, which was closed unmerged pending the post-hoc audit — this PR carries the original work plus the audit's close-out items for it. Sweep-workflow extension remains tracked in #75.What the original commit does (unchanged in intent)
bmad-loop validate/run-start now requirebmad-review-verification-gap(BMAD-METHOD#2550) and acustomize.tomlmarker inbmad-dev-auto; docs aligned.intent gaphalt saved the attempted change as a patch and the human confirms the reading was correct,bmad-loop resolvelatches the patch (--restore-patch/resolution.json), re-arms the spec toin-review, and the engine re-applies it onto the baseline before each fresh dispatch, so the session resumes review instead of re-implementing.Re-cut conflict resolution (runs.py, model.py — as the audit mapped)
Union of both features, folding the restore branch into the stories-hardened re-arm: the
in-reviewflip joins the UnicodeDecodeError-guarded status-flip path,story-escalation-resolvedcarriesbaselineandrestore, and the stories sentinel-clear branch is preserved.Audit close-out items (new in this PR, one commit each)
runs.rearm_escalation+ tests): a patch-restore re-arm on a sentinel-wedged stories task is rejected before any mutation — a sentinel is a pre-planning halt, so there is no attempt to restore and the re-dispatch is a planning leg. A real stories spec stays a legitimate restore target.cmd_resolverejects--restore-patchfor worktree-isolation runs (restore is in-place-only; the latch would silently never restore) + the missing outside-project-but-existing-file rejection test (LOW-5).verify.apply_patchdocstring rewritten, resolveSKILL.mdwarns that resolution commits overlapping the patch's files make the restore fail loudly (omitrestore_patchwhen the resolution already carries the work), and a combined engine test drives the full interaction (resolution commit → failed apply → re-escalation, no session dispatched, resolution commit untouched).Final-review round (adversarial multi-agent review of this PR, one commit each)
The review re-verified every audit close-out claim above against the diff (all real) and every CI/CodeRabbit signal (green), then found and fixed:
in-review→ step-04) never fired and its dirty-tree check HALTedblockedon the very diff the orchestrator had just applied — every sprint restore burned one session and insta-re-escalated. The dispatch now points attask.spec_filewhen a restore is latched (stories folder+id dispatch already early-exits and is untouched).task.baseline_commit, but only step-03 stampsbaseline_revisionand the in-review route skips it — the re-driven step-04 would diff (and on an intent-gap/bad-spec re-triage, revert) since the ORIGINAL sha, clawing back the non-overlapping resolve commits the baseline advance blesses. Re-arm now re-stampsbaseline_revisionon the restore path (newverify.set_frontmatter_field).resolution.json(quieter than an absent one) and--restore-patch ""both silently downgraded to a from-scratch re-arm that consumes the escalation — the confirmed restore decision was unrecoverable. Both now abort loudly (resolve.ResolutionError).--storyselector, session died pre-spec) latched a restore with noin-reviewrouting target; rejected at the T1 seam.is_relative_to(project)check; containment now uses the same trusted roots asverify.spec_within_roots.build_contextexportsrestore_supported, SKILL.md tells the agent never to negotiate an unsupported restore, the error names the--no-interactiveescape, and the guard also keys on the recordedtask.worktree_path._require_base_skillsdocstring) → three.restore_patchit ignored); the modal now flags it and the re-arm notifies that onlybmad-loop resolvehonors a restore.resolve --restore-patch→ resume, through the real CLI + tmux; the fake skill blocks loudly unless the prompt carries the spec pointer AND the tree was restored.Known residuals filed as issues (pre-existing or design-scale, deliberately not in this PR): #88 (from-scratch re-drives can vacuously pass proof-of-work on untracked halt residue — needs snapshot subtraction in the gate), #89 (the baseline-match verify gate reads
baseline_commit, which the generic skill never writes — dead in production, masked by fixtures), #90 (an abandoned restore attempt's untracked files get baselined at the next re-arm and swept into the corrected story's commit), #91 (cleanup batch: precondition centralization, path-normalizer dedup, test-fixture dedup).Verification
trunk check(no filter): clean.run/resolve --restore-patch/resumebinaries.Summary by CodeRabbit
bmad-loop resolve --restore-patchfor patch-based re-arm/resume.