fix(runs): advance the re-arm baseline so a resolved re-drive cannot orphan resolution commits - #78
Conversation
rearm_escalation flipped the story back to PENDING but left baseline_commit at the escalated attempt's pre-attempt snapshot. When the resolve session had committed authorized work on the project branch (e.g. a baseline fixture the escalation was about), the redrive's reset-to-baseline in engine._rollback_or_pause parked those commits on an attempt-preserve ref and rebuilt against the unresolved tree — the re-driven dev session immediately hit the very gap the human had just resolved and re-escalated. Advance baseline_commit to the project's HEAD and refresh the untracked snapshot at re-arm time, so resolution work counts as the rebuild's starting point rather than attempt debris. Best-effort: a git failure keeps the old baseline. The story-escalation-resolved journal event now records the adopted baseline.
WalkthroughThe ChangesRearm escalation baseline update
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant rearm_escalation
participant GitRepo
participant Journal
Caller->>rearm_escalation: rearm_escalation(task)
rearm_escalation->>GitRepo: rev_parse_head()
GitRepo-->>rearm_escalation: HEAD commit or GitError
rearm_escalation->>GitRepo: untracked_files()
GitRepo-->>rearm_escalation: untracked file list or GitError
rearm_escalation->>rearm_escalation: update baseline_commit / baseline_untracked
rearm_escalation->>Journal: append("story-escalation-resolved", baseline)
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 |
🤖 Augment PR SummarySummary: This PR fixes escalation re-drive behavior so a completed resolve session can’t be rolled back as “attempt debris.” Changes:
Technical Notes: Baseline advancement is best-effort (git failures retain the old baseline) to keep re-arm resilient. 🤖 Was this summary useful? React with 👍 or 👎 |
| # just loses this protection). | ||
| try: | ||
| repo = Path(state.project) | ||
| task.baseline_commit = verify.rev_parse_head(repo) |
There was a problem hiding this comment.
At task.baseline_commit = verify.rev_parse_head(repo): if rev_parse_head() succeeds but untracked_files() fails, the baseline commit will advance while baseline_untracked stays stale, which can make verify.safe_rollback() treat pre-existing untracked files as run-created and delete them.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| repo = Path(state.project) | ||
| task.baseline_commit = verify.rev_parse_head(repo) | ||
| task.baseline_untracked = sorted(verify.untracked_files(repo)) | ||
| except verify.GitError: |
There was a problem hiding this comment.
Catching only verify.GitError means other git-call failures (e.g. subprocess.TimeoutExpired/OSError from subprocess.run) would still raise and make rearm_escalation fail, despite the “best-effort” intent described above this block.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_resolve.py (1)
178-207: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winLGTM! Good coverage of the success and non-repo tolerance paths.
Consider also adding a case where
rev_parse_headsucceeds butuntracked_filesfails, to cover the partial-update risk flagged inruns.py(baseline_commitadvancing whilebaseline_untrackedstays stale).
🤖 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`:
- Around line 550-566: The best-effort baseline refresh in runs.py can leave
task.baseline_commit and task.baseline_untracked out of sync if
verify.rev_parse_head succeeds but verify.untracked_files raises GitError.
Update the logic in the baseline refresh block so both git reads are completed
first and only then assign task.baseline_commit and task.baseline_untracked
together, keeping the pair consistent for attempt_dirty and safe_rollback. If
either git call fails, leave both existing baseline values 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: d3012ab2-2cc7-45a4-85ac-4c0be7d47ed9
📒 Files selected for processing (2)
src/bmad_loop/runs.pytests/test_resolve.py
…on partial git failure Both git reads in rearm_escalation's baseline refresh now land in locals before either task field is assigned, and the except widens from GitError to Exception (matching verify._prune_refs' best-effort contract for the whole subprocess surface: a timeout or missing-binary OSError must not fail re-arm either). Previously, rev_parse_head succeeding while untracked_files raised would advance baseline_commit alone, leaving baseline_untracked stale and the two fields describing different points in time - exactly the pairing attempt_dirty/safe_rollback rely on to tell pre-existing untracked files from run-created ones. Addresses review feedback from PR #78 (CodeRabbit + Augment, converging on the same finding).
…xcept CI's trunk check (bandit) flagged the bare except/pass in rearm_escalation's baseline refresh as B110 (try/except/pass). It is deliberately best-effort (a git failure must not fail re-arm), matching the same pattern already used elsewhere in the codebase (e.g. verify._prune_refs, Engine's teardown handlers) - add the same noqa/nosec suppression with a short inline reason. Verified locally with bandit (via uvx): 0 issues, 1 potential issue specifically suppressed by the new nosec marker.
…er spec block (MINOR-G) rearm_escalation's fixed-slug sentinel handling (delete + preserve a copy, rather than a status flip) ran for every run source. A *sprint* spec that merely happened to be named `<key>-unresolved.md` / `<key>-ambiguous.md` would be DELETED instead of re-opened — the one shared-path change from the stories work reachable from default sprint mode. Gate _sentinel_condition on `state.source == "stories"`; a sprint spec is now always status-flipped and kept, whatever its name. Also move #78's baseline-advance (HEAD + untracked snapshot) to AFTER the spec block so a just-cleared stories sentinel — an untracked file removed there — is not captured into baseline_untracked as a phantom pre-existing untracked file (the "order snapshot after sentinel-clear" merge note). The advance's own atomic-pair guarantee is unchanged. Sprint regression test: a sentinel-shaped sprint spec is flipped, not deleted; the stories sentinel tests now pin source="stories". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…er spec block (MINOR-G) rearm_escalation's fixed-slug sentinel handling (delete + preserve a copy, rather than a status flip) ran for every run source. A *sprint* spec that merely happened to be named `<key>-unresolved.md` / `<key>-ambiguous.md` would be DELETED instead of re-opened — the one shared-path change from the stories work reachable from default sprint mode. Gate _sentinel_condition on `state.source == "stories"`; a sprint spec is now always status-flipped and kept, whatever its name. Also move #78's baseline-advance (HEAD + untracked snapshot) to AFTER the spec block so a just-cleared stories sentinel — an untracked file removed there — is not captured into baseline_untracked as a phantom pre-existing untracked file (the "order snapshot after sentinel-clear" merge note). The advance's own atomic-pair guarantee is unchanged. Sprint regression test: a sentinel-shaped sprint spec is flipped, not deleted; the stories sentinel tests now pin source="stories". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…er spec block (MINOR-G) rearm_escalation's fixed-slug sentinel handling (delete + preserve a copy, rather than a status flip) ran for every run source. A *sprint* spec that merely happened to be named `<key>-unresolved.md` / `<key>-ambiguous.md` would be DELETED instead of re-opened — the one shared-path change from the stories work reachable from default sprint mode. Gate _sentinel_condition on `state.source == "stories"`; a sprint spec is now always status-flipped and kept, whatever its name. Also move #78's baseline-advance (HEAD + untracked snapshot) to AFTER the spec block so a just-cleared stories sentinel — an untracked file removed there — is not captured into baseline_untracked as a phantom pre-existing untracked file (the "order snapshot after sentinel-clear" merge note). The advance's own atomic-pair guarantee is unchanged. Sprint regression test: a sentinel-shaped sprint spec is flipped, not deleted; the stories sentinel tests now pin source="stories". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…er spec block (MINOR-G) rearm_escalation's fixed-slug sentinel handling (delete + preserve a copy, rather than a status flip) ran for every run source. A *sprint* spec that merely happened to be named `<key>-unresolved.md` / `<key>-ambiguous.md` would be DELETED instead of re-opened — the one shared-path change from the stories work reachable from default sprint mode. Gate _sentinel_condition on `state.source == "stories"`; a sprint spec is now always status-flipped and kept, whatever its name. Also move #78's baseline-advance (HEAD + untracked snapshot) to AFTER the spec block so a just-cleared stories sentinel — an untracked file removed there — is not captured into baseline_untracked as a phantom pre-existing untracked file (the "order snapshot after sentinel-clear" merge note). The advance's own atomic-pair guarantee is unchanged. Sprint regression test: a sentinel-shaped sprint spec is flipped, not deleted; the stories sentinel tests now pin source="stories". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…Phase 2 + 3) (#77) * feat(stories): engine, adapter + HITL checkpoints for folder+id dispatch (Phase 2) Wire "stories mode" (BMAD-METHOD #2549) into the loop as an opt-in engine variant alongside the default sprint-status flow — building on the Phase 1 contract layer. Covers the folder+id engine/adapter plus the human-in-the-loop plan/story checkpoints and sentinel recovery. Engine + adapter: - StoriesEngine(Engine): a thin override layer like SweepEngine. `_pick_next` runs the linear schedule from stories.py (re-validated fresh every pick, within-run skip set mirroring sprint's base_skip, blocked/sentinel/ambiguous → pause for resolve). `_dev_prompt` emits the folder+id dispatch (`/bmad-dev-auto Spec folder: <rel>. Story id: <id>.` + verbatim `invoke_dev_with`). `_post_dev_state_sync` is a no-op (no sprint board), `_verify_dev_artifacts`→verify_dev_stories, and `_verify_review`→ verify_review_stories (drops the sprint-status gate). - Deterministic adapter read-back: GenericDevAdapter resolves the id-keyed story spec via stories.resolve_story_spec when BMAD_LOOP_SPEC_FOLDER is set (new env seam on Engine._run_session), skipping the mtime scan; a relative folder is rebased against spec.cwd for worktree isolation. - [stories] policy (source = sprint-status|stories, spec_folder; no continue_independent) mirroring [review] — dataclass, validation, core.toml schema section, template. RunState pins source + spec_folder so resume/resolve rebuild the right engine without re-reading policy. - Preflight content-probe (install.missing_stories_support): stories mode needs a bmad-dev-auto whose step-01 carries folder+id dispatch; fail loud, not at dispatch time. `run --spec <folder>` forces stories mode; `--dry-run` prints the linear schedule (checkpoints, live on-disk state); `--story <id>` filters. - verify_dev_stories proof-of-work also excludes the spec folder's stories/ + stories.yaml so a spec-only story never reads as implementation work. HITL checkpoints (per-story, independent — a story may set both and pause twice): - spec_checkpoint (two-leg plan-halt): `_plan_halt_leg` reads on-disk state — leg 1 dispatches `Halt after planning.` + BMAD_LOOP_PLAN_HALT (adapter synthesizes ready-for-dev as a `plan_halt` terminal), verify_dev_stories( plan_halt=True) gates the plan (ready-for-dev, no proof-of-work, no build/test via the `_run_verify_commands_after_dev` seam), then StoriesEngine pauses at PAUSE_PLAN_CHECKPOINT. Resume re-drives leg 2 (plain folder+id → implement) via `_resume_after_dev_verify`, keyed off StoryTask.plan_checkpoint_pending; the on-disk status (not a flag) keeps prompt + env in lock-step. - done_checkpoint: after a story commits, `_after_story` pauses at PAUSE_STORY_CHECKPOINT — skipped when the story was the last to dispatch. Fires from both _loop and _finish_inflight, always after worktree integration, so a committed unit is merged before the run stops. - Blocked/sentinel/ambiguous wedge (_pause_wedged) records an ESCALATED task (spec path attached), so `resolve`/rearm_escalation and the resolved re-drive flow through the same machinery as an in-run escalation — no defer-and-continue. - Sentinel recovery in runs.rearm_escalation: a fixed-slug <id>-unresolved.md / <id>-ambiguous.md is preserved under {run_dir}/sentinels/, journaled `sentinel-cleared` with its blocking condition, then deleted so the re-dispatch starts clean (PENDING → re-plan). - New pause consts PAUSE_PLAN_CHECKPOINT / PAUSE_STORY_CHECKPOINT + StoryTask .plan_checkpoint_pending (serialized). Journal events: plan-halt, checkpoint-pause, sentinel-cleared, stories-validated. Base engine seams are no-ops for sprint/sweep; the TUI keeps reusing the CLI resume/resolve paths. Tests: StoriesEngine happy path / scheduling / prompt seams / resume round-trip, plan-checkpoint pause/resume round-trip, story-checkpoint pause incl. skip-if-last, additive spec+done double-pause, sentinel re-arm (preserved copy + journal + clean re-dispatch), adapter id-keyed + plan_halt read-back, [stories] policy matrix, install probe, CLI dry-run/validate, RunState + plan_checkpoint_pending round-trip, verify_dev_stories plan_halt gate, verify_review_stories. Full sandbox E2E matrix is deferred to Phase 4. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(verify): port stories proof-of-work exclusions onto the file-granular #79 contract (T3) verify_dev_stories excluded the whole implementation_artifacts/planning_artifacts folders from its proof-of-work gate (the pre-#79 artifact_relpaths blanket). After (verify_dev_exclude_relpaths: only the session's own spec + the sprint-status ledger), stories mode kept the exact false-negative #79 fixed: a story whose entire authorized scope is ledger/spec reconciliation (e.g. deferred-work.md under implementation_artifacts) always read as "no changes since baseline" and got rolled back/deferred. Swap artifact_relpaths for verify_dev_exclude_relpaths in verify_dev_stories, keeping _stories_relpaths (the spec folder's stories/ subdir + stories.yaml hold only specs and the human-authored manifest, never implementation work). Test: a stories-mode story whose only diff is the deferred-work ledger now passes proof-of-work. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(stories): stop a bare resume leapfrogging a wedged story (MAJOR-A) _compute_schedule built the scheduler skip set from every recorded task (`set(self.state.tasks)`), including the ESCALATED task _pause_wedged persists for a blocked/sentinel/ambiguous story. Because schedule() consults the skip set before classifying a story's on-disk state, a plain `bmad-loop resume` that never resolved the wedge would skip past the blocked story and dispatch the next one onto a tree missing the blocked story's work — violating the linear "a blocked story cannot be leapfrogged" invariant the branch documents. Restrict the skip set to stories actually retired this run — DONE or DEFERRED — matching schedule()'s own documented contract. An unresolved wedge is left out, so resume re-classifies it from disk (still blocked) and re-pauses on the same story; once `resolve` re-arms it to PENDING it re-dispatches normally. Engine test: wedge → bare resume with sessions available → still pauses on the same story, no dev session runs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(stories): a spec_checkpoint story can never commit without a plan review (MAJOR-B) The plan-review pause keyed entirely off ephemeral, result-derived state: the per-leg plan_checkpoint_pending (set from result.json's plan_halt marker) and _plan_halt_leg (which reads the on-disk spec status). Both go stale the moment the plan reaches ready-for-dev on disk, so the checkpoint was silently skipped whenever the pause was not consumed on that same leg: (a) host dies after the plan is written but before the durable session record → resume re-drives on the already-planned spec straight to implementation; (b) leg 1's verify fails non-fixably → the tree resets but the (rollback-kept) plan survives, so attempt 2 is an implement leg; (c) the skill overruns `Halt after planning.` and drives leg 1 to done. Latch a durable obligation, StoryTask.plan_review_owed, at the story's first dispatch — before the session runs, keyed off the entry's spec_checkpoint flag, not the leg's status/result — and persist it so it survives all three. Clear it ONLY when a plan-review pause actually raises. After any dev leg that did not itself pause, if the obligation is still owed, pause before _review_and_commit with a distinct "plan review owed but already implemented" message (reusing PAUSE_PLAN_CHECKPOINT so CLI/TUI resume is unchanged; plan_checkpoint_pending stays unset so resume commits the approved work rather than re-driving). Tests cover all three scenarios. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(stories): withhold story-spec env from injected workflow sessions (MAJOR-C) _extra_session_env exported BMAD_LOOP_SPEC_FOLDER for every dev/review-role session, including injected plugin-workflow sessions (e.g. a TEA pre_commit_gate). The generic adapter treats that env as a signal to short-circuit to id-keyed story-spec synthesis — but at pre_commit_gate the story spec is already `done`, so a gate session that did no work would read `completed:done` and bypass the completion-marker + monotonic stall-nudge contract that the TEA-livelock fix depends on. Stories-mode only; no sprint regression. Thread the session `label` (None for the primary dev/review session, set for an injected workflow) from the _run_session call site into _extra_session_env, and export the story-spec env only for primary sessions. A labeled workflow session then keeps the generic marker contract. Unit test (label withholds the env) + an integration test (a pre_commit_gate workflow session in a stories run carries no BMAD_LOOP_SPEC_FOLDER, while the primary dev session still does). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(runs): gate sentinel-clear on stories mode; snapshot baseline after spec block (MINOR-G) rearm_escalation's fixed-slug sentinel handling (delete + preserve a copy, rather than a status flip) ran for every run source. A *sprint* spec that merely happened to be named `<key>-unresolved.md` / `<key>-ambiguous.md` would be DELETED instead of re-opened — the one shared-path change from the stories work reachable from default sprint mode. Gate _sentinel_condition on `state.source == "stories"`; a sprint spec is now always status-flipped and kept, whatever its name. Also move #78's baseline-advance (HEAD + untracked snapshot) to AFTER the spec block so a just-cleared stories sentinel — an untracked file removed there — is not captured into baseline_untracked as a phantom pre-existing untracked file (the "order snapshot after sentinel-clear" merge note). The advance's own atomic-pair guarantee is unchanged. Sprint regression test: a sentinel-shaped sprint spec is flipped, not deleted; the stories sentinel tests now pin source="stories". Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(stories): shared status-projection helpers + mode-aware status/dry-run The Phase-2/3-scope portion of the original Phase-4 commit (67b75e3), split out so #77 stays pure Phase 2+3 and the TUI surface + docs + E2E matrix ride the stacked Phase-4 branch. These pieces are core infrastructure that Phase-2/3 code (the stories-aware validate + --story preflight + dry-run parity in item 8, the resolve escalation context in item 9, and the engine folder-render in item 10) already depends on, so they belong below those commits on this branch. - stories.py: the disk-derived status projection shared by the CLI and TUI — resolve_spec_folder (folder anchoring), state_label, StoryRow, story_rows — plus the pure, engine/RunState-free read model they build on. - cli.py: `status` is mode-aware (a stories run prints its board via story_rows) and `run --dry-run` is refactored onto the same helper so the two never drift. - tests: test_cli / test_stories / test_stories_engine / test_install coverage for the projection + the mode-aware status/dry-run paths. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(cli): stories-aware validate + selector preflight + dry-run parity Wave-1b item 8 + the CLI parts of item 10 (audit plan your-two-plans-were-delightful-phoenix.md). - `bmad-loop validate` is now mode-aware: in stories mode (`[stories].source == "stories"` or `--spec <folder>`) it skips the sprint-status gate — which a stories-only project fails on — and instead validates stories.yaml + SPEC.md and probes bmad-dev-auto for folder+id dispatch, with remediation text (MAJOR, flagged by 3 audit agents). `validate --spec` forces it; TUI `v` inherits. - `--story` selector preflight: an unknown id fails the run at preflight (exit 1) instead of crashing the scheduler mid-flight (MINOR-E). - dry-run parity (NOTE): render the project-relative folder the engine actually dispatches, and emit a pending spec_checkpoint story's leg-1 `Halt after planning.` + BMAD_LOOP_PLAN_HALT markers. - stories.py: shared relativize_spec_folder / is_plan_halt_leg / recorded_blocking_condition helpers + PLAN_PRODUCED_STATUSES so run, dry-run, validate and resolve all agree on the same strings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(resolve): stories escalation context + SKILL sentinel guidance Wave-1b item 9 + the sentinel-cleared journal fix (MINOR-5). - resolve.build_context adds a `stories` block in stories mode: the spec folder, the manifest entry (title/description/checkpoints/invoke_dev_with), and — for a sentinel-escalated story — a sentinel indicator with its kind + recorded blocking condition. The resolver now sees the manifest intent and knows a sentinel has no frozen spec to edit (MAJOR-2). Sprint mode is unchanged. - bmad-loop-resolve SKILL.md: document the stories context block and add a "sentinels and the preserved copy" section (resolve the upstream ambiguity; the orchestrator preserves+deletes the sentinel on re-arm). Reseeded to the .claude/.agents working-tree copies. - runs._clear_sentinel journals sentinel-cleared with the recorded blocking condition parsed from `## Auto Run Result`, plus the fixed slug as sentinel_kind — not the slug alone (MINOR-5). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * feat(stories): engine polish — selector/sentinel/max-stories (item 10) Wave-1b item 10 (audit plan your-two-plans-were-delightful-phoenix.md), the engine portion. The doc-note + TUI + e2e-assert parts of item 10 target Phase-4-introduced text/code, so they ride the stacked Phase-4 branch. - rename engine-side StoriesError -> StoriesModeError, distinct from the contract-side stories.StoriesError so the parse vs drive seams never conflate. - an unknown --story id mid-run pauses for resolve instead of crashing the run (MINOR-E); keyed on the selector so rearm/TUI act on it uniformly. - emit sentinel-detected at read-back (pick-time wedge + post-dev verify), carrying the recorded blocking condition (MINOR-6) — no longer only the later stories-wedged/escalation trace. - done_checkpoint skip-if-last honors --max-stories durably from state (MINOR-F): a bounded run no longer leapfrogs past its cap after a checkpoint pause/resume. - _entry_for journals a one-time stories-manifest-unreadable warning per story when the manifest is hand-broken (NOTE-10). - tests: gate+checkpoint additive double-pause (MINOR-4) plus the audit-named test per fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(stories): re-add PR #65 review-fix regression tests after phase1 rebase Rebasing phase2 onto the updated phase1 (PR #65 review fixes) kept the source fixes (id-guard, story_key normalization, deferred-import comment) but favored phase2's overlapping test additions, dropping two new-test functions. Re-add them and finish CR-3: - test_resolve_charset_invalid_id_is_pending_not_glob: a metachar/path-sep id resolves to PENDING, never an injected glob match. - test_verify_dev_stories_whitespace_story_key: a padded story_key still resolves and passes (normalized id feeds the filename-prefix check). - re.escape() the phase2-added `no stories.yaml found` match pattern too. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(install): catch UnicodeDecodeError in stories-support probe (C1) read_text(encoding="utf-8") raises UnicodeDecodeError (a ValueError, not an OSError) on a binary/non-UTF-8 step-01 file, so the content probe's except OSError let it escape and crash the whole preflight. Catch it alongside OSError and report the tree as a problem, mirroring the missing-file case. * fix(stories): launch-floor + ambiguous guard in adapter read-back (A1, A2) A1: _stories_result_json read back the id-keyed story spec with no mtime floor, so a spec left terminal by a prior step — notably the dev's `done` spec that a follow-up review session (also a GenericDevAdapter with BMAD_LOOP_SPEC_FOLDER set) re-opens — was misread as THIS session's completion even when the session produced nothing. Require the spec's mtime to be >= handle.launched_ns, the same launch floor devcontract.find_result_artifact applies on the mtime-scan path; stories-mode dev/review read-back now matches sprint mode. A2: a KIND_AMBIGUOUS match (>1 <id>-*.md file) now returns None immediately instead of polling the full grace — waiting can't collapse the anomaly; the next _pick_next re-classifies it into an actionable wedge for resolve. * fix(engine): make --max-stories cap durable across pause/resume (A5) The _loop dispatch gate counted a local `started` that resets to 0 every time the loop is re-entered. With HITL checkpoints, a healthy run now pauses (plan/story checkpoint) and resumes routinely, so after a checkpoint pause the reset counter let the run dispatch past its --max-stories cap. max_stories is already persisted on RunState and re-passed on resume, so only the counter was non-durable. Gate on a new Engine._dispatched_count() = len(state.tasks) — every picked story is recorded before its session runs (the same set _pick_next keys base_skip on), so the task count is the durable dispatch tally that survives resume. StoriesEngine's _max_stories_reached() (the done_checkpoint skip-if-last guard) now consults the same count, so skip-if-last fires exactly when the loop will stop — no drift. Sprint-mode behavior is identical in-run and only gains resume durability. * fix(stories): clear sentinels by recorded verdict, not basename (C2, C3) C2: rearm_escalation identified a sentinel purely from the spec_file basename (<key>-unresolved.md / <key>-ambiguous.md), so a real story spec that happened to match the convention — or a non-sentinel escalation whose spec matched — was DELETED on re-arm. Data loss for a legitimately-named spec. Record the sentinel verdict at detection time instead: StoriesEngine stamps a new durable StoryTask.sentinel_kind in both detection points (_pause_wedged pick-time wedge and _verify_dev_artifacts post-dev read-back). rearm_escalation now clears a sentinel only when task.sentinel_kind is set (stories mode, defensively re-confirming the on-disk name still matches the recorded slug), and clears the field once discharged. A spec the run never classified as a sentinel is status-flipped and kept. C3: test_rearm_non_sentinel_spec_still_flips_status now runs with source="stories" so it actually exercises the stories-mode sentinel branch (it defaulted to sprint-status, which skips that branch entirely). Adds a sentinel-named-but-never- detected regression, a model round-trip, engine detection assertions, and a full detection->rearm E2E. * fix(cli): warn when --epic is passed with stories mode (A4) --epic has no effect in stories mode (StoriesEngine nulls epic_filter — the stories.yaml manifest is a single flat schedule), so `run --spec ... --epic N` silently dropped the flag. Print a one-line stderr note pointing to --story for per-id filtering instead of surprising the caller with an unfiltered run. * fix(stories): catch UnicodeDecodeError in spec/sentinel read paths Non-UTF-8 story specs/sentinels raised UnicodeDecodeError (a ValueError, not OSError), so the existing `except OSError` guards missed it and crashed the escalation-resolution + re-arm flows instead of degrading gracefully. Broaden the guards across the stories-mode reads: - resolve._stories_context: load_stories, resolve_story_spec, and the sentinel blocking-condition read now tolerate a decode failure (best-effort context). - runs._clear_sentinel: wrap the blocking-condition read; still preserve+delete the sentinel so re-arm completes with an empty recorded condition. - stories_engine._journal_sentinel_detected: same defect, unflagged sibling swept in the same pass. Same bug class as the install.py stories-support probe (74f9b35). Addresses the two open augmentcode threads on PR #77. Regression tests per site (invalid UTF-8). * fix(stories): convert non-UTF-8 manifest/spec reads to clean errors load_stories read the manifest and resolve_story_spec read a PRESENT spec's frontmatter with read_text(encoding="utf-8") but neither guarded the UnicodeDecodeError (a ValueError, not caught by their except yaml.YAMLError / except OSError). A binary/non-UTF-8 stories.yaml or story spec therefore crashed stories-mode preflight, --dry-run, status, validate, and the engine loop with a traceback instead of the clean "stories mode: ..." error the mode contracts on — since every call site (cli.py, stories_engine.py) catches only StoriesError. Fix at the two source functions rather than each of the 5 call sites (DRY, keeps load_stories's "raises StoriesError" contract, also covers verify.py's review-path resolve_story_spec): - load_stories: UnicodeDecodeError -> StoriesError ("not valid UTF-8"). - resolve_story_spec: an undecodable PRESENT spec degrades to status="", which _classify treats as wedged (-> pause for resolve, never silent skip) and state_label renders as "present". Same bug class as the 528171f sweep, which missed these two paths. Leaves verify.read_frontmatter (shared with the sprint path) untouched. Addresses the open augmentcode thread on PR #77. Regression tests per site (non-UTF-8 manifest + PRESENT spec + clean-error CLI boundary). * fix(stories): re-pause an in-run escalation on bare resume, never re-derive it from disk _compute_schedule deliberately leaves ESCALATED tasks out of the skip set (MAJOR-A) so a pick-time wedge re-classifies from disk and re-pauses. That is only safe when the escalation's subject IS the disk state. An *in-run* escalation is not: a CRITICAL verify outcome (e.g. a proof-of-work GitError, which fires only after the status gate passed at in-review/done) or a resolved-redrive exhaustion pause escalates with the spec at a resumable or done status. A bare `bmad-loop resume` (the CLI gates only on liveness) then skipped the terminal ESCALATED task in _finish_inflight, re-classified the story from disk, and either re-dispatched it — overwriting the escalated task with a fresh StoryTask, destroying the escalation record and the resolved_redrive guard (a later exhaustion would DEFER the human-resolved work) — or, at done, dispatched the NEXT story past the unresolved escalation, the exact leapfrog MAJOR-A forbids. New _repause_inrun_escalation guard in _pick_next: a task at ESCALATED with attempt > 0 (a session ran — pick-time wedge/unknown-selector tasks stay at 0 and keep their designed fix-by-hand-then-resume lifecycle) re-raises the escalation pause before disk classification, mutating nothing so resolve still has the full record. rearm_escalation resets the task to PENDING, after which the guard no longer matches and the normal re-drive proceeds. Sprint mode is unaffected (its base_skip includes ESCALATED; proceeding past is its designed defer-and-continue). Regression tests: bare resume with an in-run-escalated spec at in-review (re-pauses on itself, task + resolved_redrive survive) and at done (story 2 never dispatched). * fix(stories): sweep the last three UnicodeDecodeError read paths Round 3 of the 528171f/52df1bd bug class (UnicodeDecodeError is a ValueError, so except-OSError guards miss it). Three sites remained: - adapters/generic._stories_result_json: devcontract.synthesize_result re-reads the resolved spec as UTF-8, and 52df1bd's degrade in resolve_story_spec keeps an undecodable spec kind=PRESENT with a path — so the read-back poll crashed on the very state the engine wedges-and-pauses on. Treat the decode failure as no-result-yet: a torn mid-write glimpse is retried by the poll, a genuinely corrupt spec expires the grace result-less and the next _pick_next raises the actionable wedge. (Closes the open augment thread on generic.py.) - verify.read_frontmatter: fixed at the source rather than the flagged verify_review_stories call site — an undecodable file now degrades exactly like unparseable YAML ({} → status "" → clean retry). Covers verify_review_stories AND _verify_shared_gates (verify_dev_stories), and de-crashes the pre-existing sprint/bundle verify paths for free. (Closes the open augment thread on verify.py.) - runs.rearm_escalation non-sentinel flip: set_frontmatter_status / strip_auto_run_result both re-read the spec as UTF-8, so re-arming an undecodable PRESENT-spec escalation aborted `bmad-loop resolve` with a traceback after mutating in-memory state. Convert to an actionable RearmError raised BEFORE save_state — nothing persists, the escalation stays armed, and the message says to fix/replace the file and re-run resolve. Regression tests per site (invalid UTF-8 bytes): read-back returns None, read_frontmatter returns {}, verify_review_stories retries clean, rearm fails clean and stays armed. * fix(stories): make the dry-run --max-stories cap count dispatchable stories, like the run story_rows truncated the RAW manifest list (entries[:max_stories]), counting already-done stories against the cap, while the real run's cap is the durable dispatch count (len(state.tasks)) that only ever counts stories it drives. On a partially-complete manifest the preview was disjoint from reality: [1..4] with 1-2 done and --max-stories 2, dry-run printed stories 1-2 (both done), the run dispatched 3-4. The docstring already claimed run-limit parity. The cap now counts entries whose on-disk state is not done (_classify), so done rows before the cap stay in view as skipped context and the preview stops after the cap's worth of driveable stories. A non-positive cap now previews an empty schedule — the run dispatches nothing — instead of Python's negative-slice dropping entries from the END of the list. Covers cmd_status and run --dry-run via the shared projection; regression test with a partially-done manifest + zero/negative caps. * fix(stories): fire the done_checkpoint even when the manifest goes unreadable after commit _after_story resolved the entry via _entry_for, which conflates "manifest unreadable" with "id absent" (both return None) — so a stories.yaml that became unreadable between the commit and the after-story check silently dropped the human review pause. Its sibling guard _schedule_complete treats the exact same fault the opposite, deliberately conservative way ("not complete" so the checkpoint still fires). Distinguish the two in _after_story: a readable manifest without the id (or without done_checkpoint) still skips, an unreadable one journals stories-manifest-unreadable and pauses. The run cannot proceed past a broken manifest anyway — the next pick halts on it loud — so the only thing a skip could save is the review itself. Regression test: a dev session that corrupts stories.yaml before commit still lands PAUSE_STORY_CHECKPOINT on the committed story. * docs(resolve-skill): distinguish the multi-file ambiguous-id wedge from the ambiguous sentinel SKILL.md documented the single-file <id>-ambiguous.md sentinel but nothing about the OTHER ambiguous state: >1 file matching <id>-*.md wedges the id with no sentinel — no stories.sentinel context block, no spec path, and no auto-preserve-and-delete on re-arm, so a bare re-arm just re-wedges on the same duplicates. The only prose containing "ambiguous" implied the deletable sentinel, inviting the resolver to conflate the two and leave the duplicate in place. New subsection: how to recognize the state (escalation reason + missing sentinel block) and that the cleanup IS the resolution — merge/remove/rename with the human until at most one file matches, then marker + re-arm as usual. Mirrors re-copied to .claude/.agents skill trees (untracked dogfood copies). * test(engine): sprint-mode --max-stories durability across a pause/resume The e56f9db fix rewired the SHARED dispatch gate (base Engine._dispatched_count replaced the resume-reset _loop counter) but was regression-tested only through stories mode. Lock the sprint path in too: cap=2, one story committed before an epic-boundary gate pause, resume dispatches exactly one more — the third ready-for-dev story never dispatches. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…r preflight (re-cut) (#86) * feat(resolve): intent-gap patch-restore re-drive + inline-review-layer 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). * test(resolve): a sentinel-wedged story rejects a patch-restore re-arm (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. * fix(cli): reject --restore-patch for worktree-isolation runs (B4d) 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. * docs(verify): reconcile apply_patch's clean-apply claim with the advanced 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. * fix(verify): a latched restore patch never counts as proof-of-work (T4) 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. * docs: finish the three-layer review prose sweep (audit LOW-4) 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. * test(e2e): provision the new review-layer prereqs in the sandbox scaffolds 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. * style: replace ambiguous × with ASCII x in comments/docstrings (CodeRabbit) 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. * fix(engine): sprint restore re-drive dispatches an explicit spec pointer (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. * fix(resolve): re-stamp the spec's baseline_revision at a patch-restore 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. * fix(resolve): corrupt or empty restore inputs abort instead of silently 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. * fix(resolve): a spec-less escalation rejects a restore latch (review F4) 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. * fix(cli): restore-patch containment accepts the configured artifact roots (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. * fix(cli): validate a restore before the interactive resolve session it 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. * docs: two remaining 'two review hunters' references -> three (review F7) The LOW-4 prose sweep (84d4da1) claimed completion but missed docs/setup-guide.md and the _require_base_skills docstring. * fix(tui): a recorded restore decision is surfaced at re-arm, never dropped 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). * test(e2e): sprint-mode intent-gap patch-restore through the real CLI (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. * style: black line-join in the test_cli config helper (trunk fmt) * fix(resolve): a non-UTF-8 resolution marker raises ResolutionError, not 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.
What
rearm_escalationnow advancesbaseline_committo the project's current HEAD (and refreshes the untracked-files snapshot) instead of leaving it at the pre-attempt snapshot.Why
A resolve session can legitimately commit work (e.g. the fixture the escalation asked for). The re-drive's reset-to-baseline still targeted the old baseline, so it parked that commit on an
attempt-preserve/*ref and rebuilt against the unresolved tree, and the re-driven session hit the same gap and re-escalated verbatim. This complements other recent fixes for staleAuto Run Resulthandling and resume durability.Rebased onto v0.8.1; the only conflict was a docstring/adjacent-lines overlap in
rearm_escalationwith an upstream addition to the same function, resolved by keeping both. Independent of the other pending PR (ledger-only proof-of-work fix); different files, either can land first.How
baseline_commit = verify.rev_parse_head(project), re-snapshotbaseline_untracked.GitErrorleaves the old baseline in place.Testing
Three new tests in
tests/test_resolve.py(adopts HEAD + re-snapshots; all-or-nothing on a partial git failure — no half-advanced baseline; stays stale outside a repo). Full suite: 1407 passed, 7 skipped; ruff clean.Summary by CodeRabbit
Bug Fixes
Tests