fix(verify): stop treating ledger/spec-only stories as "no changes since baseline" - #79
Merged
Merged
Conversation
…nce baseline" verify_dev/verify_dev_bundle's proof-of-work gate blanket-excluded the whole implementation_artifacts/planning_artifacts folders from has_changes_since, so a story whose entire authorized scope is ledger/spec reconciliation (e.g. deferred-work.md sync) always produced a false "no changes" verdict and got rolled back/deferred, no matter how much real, reviewed work landed. Add verify_dev_exclude_relpaths(paths, spec_path): excludes only the session's own claimed spec file and the sprint-status ledger (the two things every session rewrites regardless of real work), not the whole artifact folders. Sibling content (deferred-work.md, other stories' specs) now counts. Wired into both has_changes_since call sites in place of artifact_relpaths, which stays as-is for Engine's rollback-protection use (a different job). output_folder is deliberately excluded from the new helper: in the standard layout it's the parent dir of implementation_artifacts, so excluding it as a prefix would swallow that folder's content right back out of view.
WalkthroughAdds ChangesVerify Exclude Gating
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Dev as verify_dev/verify_dev_bundle
participant Exclude as verify_dev_exclude_relpaths
participant Git as has_changes_since
Dev->>Exclude: compute exclude(paths, spec_path)
Exclude-->>Dev: (sprint_status, spec_path) relpaths
Dev->>Git: has_changes_since(baseline_commit, exclude=relpaths)
Git-->>Dev: changed files outside exclude set
alt real work detected
Dev-->>Dev: proof-of-work gate passes
else no changes since baseline
Dev-->>Dev: proof-of-work gate fails
end
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: Tightens the dev/bundle proof-of-work check so ledger/spec-only stories aren’t falsely treated as “no changes since baseline”.
🤖 Was this summary useful? React with 👍 or 👎 |
| out: list[str] = [] | ||
| for path in (paths.sprint_status, spec_path): | ||
| try: | ||
| rel = path.relative_to(paths.project).as_posix() |
There was a problem hiding this comment.
verify_dev_exclude_relpaths: because spec_path isn’t normalized before relative_to(...).as_posix(), a spec_file containing .. segments could yield an exclude string that doesn’t match git’s normalized path, letting a spec-only status flip be counted as “real work”. Consider normalizing (resolve()) before deriving rel so the exclude set is stable and reliable.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
verify_dev_exclude_relpaths compared spec_path against paths.project via a lexical relative_to(), but spec_path is derived from a session-reported (untrusted) spec_file string. An un-normalized '..'/'.' segment in that string still resolves to the real on-disk file (the OS resolves it when checking spec_path.is_file()), but as a raw string it would not match git's own normalized path output — silently defeating the exclude and letting a bare status flip on the session's own spec count as real work. Resolve both spec_path and paths.project before deriving the relpath, same as spec_within_roots already does for the same session-controlled-path reason. Two new tests: the exclude helper normalizes a '..'-laden path to the same entry as the plain one, and an end-to-end verify_dev regression confirming the bare status flip still retries as "no changes" when claimed through such a path. Addresses review feedback from PR #79 (Augment).
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
pbean
added a commit
that referenced
this pull request
Jul 7, 2026
…ular #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 #79 narrowed verify_dev/verify_dev_bundle to a file-granular exclude (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>
pbean
added a commit
that referenced
this pull request
Jul 7, 2026
…ular #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 #79 narrowed verify_dev/verify_dev_bundle to a file-granular exclude (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>
pbean
added a commit
that referenced
this pull request
Jul 7, 2026
…ular #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>
pbean
added a commit
that referenced
this pull request
Jul 8, 2026
…ular #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>
pbean
added a commit
that referenced
this pull request
Jul 8, 2026
…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>
pbean
added a commit
that referenced
this pull request
Jul 8, 2026
…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.
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
2 participants
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.
What
verify_dev/verify_dev_bundle's proof-of-work gate now excludes only the session's own spec file and the sprint-status ledger fromhas_changes_since, via a newverify_dev_exclude_relpaths(paths, spec_path), instead of the wholeimplementation_artifacts/planning_artifactsfolders.Why
The old exclusion existed so a session couldn't fake completion by only flipping its own spec's
status:field, but it hid everything under those folders, including sibling content like the deferred-work ledger. A story whose entire authorized scope is ledger/spec reconciliation (no src/tests touched, by design) therefore always false-negatived "no changes since baseline" and got rolled back or deferred, no matter how much real, reviewed work landed.How
verify_dev_exclude_relpaths: excludespaths.sprint_statusand the claimedspec_pathonly (file, not folder, granularity).has_changes_sincecall sites, replacingartifact_relpaths(paths)(which stays as-is forEngine's rollback-protection use, a different job).output_folderdeliberately stays out of the new exclude set: it's the parent dir ofimplementation_artifactsin the standard layout, so excluding it as a prefix would swallow that folder's content right back out of view.Testing
Seven new tests in
tests/test_verify.py: exclude-helper granularity plus a..-segment normalization guard on the helper, a directhas_changes_sincerepro (ledger content counts, own-spec + sprint bookkeeping alone doesn't), both theverify_devandverify_dev_bundlegates passing end-to-end on a ledger-only diff, and regression guards that a bare own-spec status flip still retries as "no changes" — both directly and when the spec is claimed via a..-laden path. Full suite: 1412 passed, 7 skipped; ruff clean.Related
Independent of the other pending PR (
fix/resolve-baseline-advance); different files, either can land first.Summary by CodeRabbit
Bug Fixes
Tests