Skip to content

feat(stories): spec-to-loop engine, adapter, CLI + HITL checkpoints (Phase 2 + 3) - #77

Merged
pbean merged 24 commits into
mainfrom
feat/spec-to-loop-stories-phase2
Jul 8, 2026
Merged

feat(stories): spec-to-loop engine, adapter, CLI + HITL checkpoints (Phase 2 + 3)#77
pbean merged 24 commits into
mainfrom
feat/spec-to-loop-stories-phase2

Conversation

@pbean

@pbean pbean commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Phase 2 + 3 of the spec-to-loop contract (BMAD-METHOD #2549, merged): the stories-mode engine, adapter read-back, [stories] policy, preflight/CLI, resolve escalation context, and the per-story human-in-the-loop checkpoints — plus the audit-remediation and final-review fixes (below). Stacked on #65 #65 (Phase 1) merged as c6d033d — this branch is rebased onto main and the PR retargeted accordingly. The TUI surface, prose docs, and the sandbox E2E matrix are the stacked #82 (Phase 4). Remaining merge order: #77#82.

Re-pinned to the merged #2549 text before coding: field names, the seven blocking-condition strings, and the Halt after planning. phrasing all match verbatim — no drift.

Engine + adapter

  • StoriesEngine(Engine) — a thin override layer, same pattern SweepEngine proves:
    • _pick_next: linear schedule from stories.py, re-validated fresh every pick (id-stability rule F), a within-run skip set mirroring the sprint engine's base_skip, and a blocked/sentinel/ambiguous story → pause for resolve (PAUSE_ESCALATION, cannot leapfrog).
    • _dev_prompt: folder+id dispatch — /bmad-dev-auto Spec folder: <rel>. Story id: <id>. + verbatim invoke_dev_with, folder always project-relative.
    • _post_dev_state_sync no-op (no sprint board); _verify_dev_artifactsverify_dev_stories; _verify_reviewverify_review_stories (drops the sprint-status gate).
  • Deterministic adapter read-backGenericDevAdapter 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 + CLIinstall.missing_stories_support content-probes bmad-dev-auto for folder+id dispatch (fail loud, not at dispatch time); run --spec <folder> forces stories mode; --dry-run prints the linear schedule (checkpoint badges, live on-disk state); --story <id> filters.
  • Shared status projectionstories.resolve_spec_folder + a pure, disk-derived story_rows / StoryRow / state_label read model; bmad-loop status is mode-aware and run --dry-run is refactored onto the same helper so CLI and (in feat(stories): TUI HITL surface + docs + E2E matrix (Phase 4) #82) the TUI agree on every state string.
  • Commit hygieneverify_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 and independent — a story may set both flags 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 (the 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 / build-test), then the run pauses at PAUSE_PLAN_CHECKPOINT. Resume re-drives leg 2 via _resume_after_dev_verify.
  • done_checkpoint — after a story commits, _after_story pauses at PAUSE_STORY_CHECKPOINT, skipped when the story was the last to dispatch, and honoring --max-stories durably.
  • Blocked/sentinel/ambiguous wedge_pause_wedged records an ESCALATED task, so resolve / rearm_escalation and the resolved re-drive flow through the same machinery as an in-run escalation. resolve.build_context carries the spec_folder + story entry + sentinel indicator so the resolve agent sees the manifest intent; the bundled bmad-loop-resolve SKILL documents sentinels + the preserved copy.
  • Sentinel recovery (runs.rearm_escalation) — a fixed-slug <id>-unresolved.md / <id>-ambiguous.md is preserved under {run_dir}/sentinels/, journaled sentinel-detected / sentinel-cleared with its recorded blocking condition, then deleted so the re-dispatch starts clean.

Audit remediation on this branch (post-hoc review, 2026-07-06)

Interaction bugs found by the audit of the long build sessions, each its own commit + audit-named test:

  • MAJOR-A — a bare resume can no longer leapfrog a wedged story (the skip set is DONE/deferred tasks only, never ESCALATED).
  • MAJOR-B — a spec_checkpoint story can never commit without a plan review: the obligation is persisted at pick time and cleared only when the pause actually raises; an overrun-to-done skill still pauses.
  • MAJOR-C — injected [workflows.*]/TEA gate sessions no longer inherit the story-spec env (only primary dev/review sessions export BMAD_LOOP_SPEC_FOLDER), preserving the completion-marker contract.
  • MINOR-G — sentinel deletion on re-arm is gated on state.source == "stories", so a sprint spec named <key>-unresolved.md is never deleted.
  • T3verify_dev_stories proof-of-work ported onto fix(verify): stop treating ledger/spec-only stories as "no changes since baseline" #79's file-granular verify_dev_exclude_relpaths.
  • Plus the Wave-1b batch: stories-aware validate, --story selector preflight (pause not crash), dry-run parity, StoriesErrorStoriesModeError, --max-stories cap durability, journal polish, case-only id rejection.

Final-review close-out (2026-07-07)

A last adversarial pass (three independent review lenses over the full diff) before merge:

  • A bare resume can no longer bypass an in-run escalation (1b0c320). The MAJOR-A skip-set design re-derives an ESCALATED story from disk — correct for pick-time wedges, but an in-run escalation (a CRITICAL proof-of-work GitError fires after the status gate passed; a resolved-redrive exhaustion pause) leaves the spec at a resumable/done status, so a bare resume silently re-dispatched it (destroying the escalation record + resolved_redrive guard) or leapfrogged it at done. A new _repause_inrun_escalation guard (keyed on attempt > 0) re-pauses before disk classification, mutating nothing; rearm still discharges it to PENDING.
  • UnicodeDecodeError sweep round 3 (a456381): the adapter's synthesize_result read-back (open augment thread), verify.read_frontmatter at the source (open augment thread — also covers _verify_shared_gates and the pre-existing sprint/bundle paths), and rearm_escalation's non-sentinel status flip (found by the pass — now a clean RearmError, nothing persisted, escalation stays armed).
  • Dry-run --max-stories parity (f9e466e): the preview counted already-done stories against the cap while the run's durable dispatch count skips them — on a partially-complete manifest the previewed and driven story sets were disjoint. story_rows now caps on dispatchable entries; a non-positive cap previews the empty schedule.
  • done_checkpoint survives an unreadable manifest (4418245): _after_story now mirrors _schedule_complete's conservative default (pause for review) instead of silently dropping the checkpoint when stories.yaml goes unreadable between commit and check.
  • bmad-loop-resolve SKILL doc (69cf675): documents the multi-file ambiguous-id wedge (>1 <id>-*.md match — not a sentinel, no auto-clear on re-arm; the cleanup is the resolution) as distinct from the single-file <id>-ambiguous.md sentinel.
  • Sprint-mode --max-stories durability test (98e2546): the e56f9db gate rewire replaced the shared base counter but was only regression-tested via stories mode.

Tests

StoriesEngine happy path / scheduling / prompt seams / crash-resume; plan-checkpoint and story-checkpoint pause/resume round-trips, 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/mode-detection/selector-preflight; RunState round-trip; verify_dev_stories / verify_review_stories; the shared status-projection read model. Full suite green; trunk check (no filter) clean. (The 2 local test_module_skills_sync[bmad-loop-setup] failures are pre-existing gitignored-workspace drift and skip on CI.)

The TUI HITL surface, prose docs / CHANGELOG, and the full sandbox E2E matrix are the stacked #82 (Phase 4).

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added “stories mode” across run/validate/status/resume with folder+id dispatch and --story selection.
    • Introduced stories checkpoints (plan review + completion) with pause/resume persistence.
    • Extended dry-run/status to render the stories schedule and checkpoint markers.
  • Bug Fixes

    • Improved deterministic story-spec resolution and re-arming via sentinel preservation/clearing.
    • Hardened handling for missing, ambiguous, blocked, and non-UTF-8/partial artifacts; stories execution limits now survive resume.
  • Documentation

    • Updated policy/settings UI and stories-mode guidance (including sentinel semantics).

@coderabbitai

coderabbitai Bot commented Jul 6, 2026

Copy link
Copy Markdown

Review Change Stack

Walkthrough

This PR adds a stories-mode flow alongside sprint-status dispatch. It introduces new policy/model settings, a stories scheduler and projection layer, deterministic spec-based readback, a new engine variant with checkpoint pauses, sentinel rearm handling, verify/resolve/install updates, and CLI support for validate/run/resume/status/dry-run.

Changes

Stories Mode Feature

Layer / File(s) Summary
Policy and state contracts
src/bmad_loop/policy.py, src/bmad_loop/model.py, src/bmad_loop/data/settings/core.toml, tests/test_policy.py, tests/test_model.py, tests/test_settings_schema.py
Adds stories-mode policy parsing and defaults, persisted run/task fields, pause-stage constants, and the stories settings UI section.
Stories scheduling and table projection
src/bmad_loop/stories.py, tests/test_stories.py
Extends scheduling with skip support and adds manifest/spec-folder resolution, state labeling, and row projection helpers.
Deterministic readback and verification
src/bmad_loop/adapters/generic.py, src/bmad_loop/verify.py, src/bmad_loop/install.py, tests/test_generic_tmux.py, tests/test_verify.py, tests/test_install.py
Routes dev-result readback through stories-mode spec resolution, narrows verify exclusions, adds stories review verification, and probes upstream skill support for folder-plus-id dispatch.
Engine seams and CLI routing
src/bmad_loop/engine.py, src/bmad_loop/cli.py, tests/test_cli.py, tests/test_engine.py
Adds engine hooks for durable dispatch counting, session env, resume, and post-story handling, then wires stories-mode selection through validate, run, resume, status, and dry-run flows.
StoriesEngine dispatch and verification
src/bmad_loop/stories_engine.py, tests/test_stories_engine.py
Implements folder-plus-id dispatch, manifest-driven scheduling, selector and wedge pauses, prompt/env overrides, and stories-mode dev/review verification.
Checkpoint pauses and resume flow
src/bmad_loop/stories_engine.py, tests/test_stories_engine.py
Adds plan-checkpoint and story-checkpoint pause/resume behavior, durable plan-review flags, and done-checkpoint guards.
Sentinel clearing and resolve context
src/bmad_loop/runs.py, src/bmad_loop/resolve.py, src/bmad_loop/data/skills/bmad-loop-resolve/SKILL.md, tests/test_resolve.py
Handles sentinel rearm by preserving the sentinel, deleting the original, journaling resolution, and enriching resolve context with stories-mode data.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant StoriesEngine
  participant GenericDevAdapter
  participant Stories
  participant Runs

  CLI->>StoriesEngine: run(spec_folder)
  StoriesEngine->>Stories: schedule(skip)
  Stories-->>StoriesEngine: next story ref
  StoriesEngine->>GenericDevAdapter: dispatch dev session (SPEC_FOLDER, STORY_KEY)
  GenericDevAdapter->>Stories: resolve_story_spec(id)
  Stories-->>GenericDevAdapter: PRESENT / SENTINEL
  GenericDevAdapter-->>StoriesEngine: result JSON
  alt sentinel or blocked story
    StoriesEngine->>Runs: rearm_escalation / pause
  else plan checkpoint
    StoriesEngine->>StoriesEngine: pause at PAUSE_PLAN_CHECKPOINT
  else done checkpoint
    StoriesEngine->>StoriesEngine: pause at PAUSE_STORY_CHECKPOINT
  end
  StoriesEngine-->>CLI: persist run state
Loading

Possibly related issues

Possibly related PRs

Poem

A rabbit found a stories trail today,
With specs and sentinels cleared away.
Hop, pause, resume — then onward bound,
With checkpoint bells and journal sound.
🐇✨

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main stories-mode engine, adapter, CLI, and checkpoint changes.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/spec-to-loop-stories-phase2

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@augmentcode

augmentcode Bot commented Jul 6, 2026

Copy link
Copy Markdown
🤖 Augment PR Summary

Summary: This PR introduces a full “stories mode” alternative to sprint-status scanning, enabling deterministic folder+id dispatch driven by a typed stories.yaml manifest.

Changes:

  • Added StoriesEngine to pick stories from stories.yaml, dispatch via “Spec folder + Story id”, and pause for resolve on wedged/ambiguous/sentinel states.
  • Extended the generic adapter read-back to resolve story specs by id when BMAD_LOOP_SPEC_FOLDER is set (avoids mtime scanning) and supports plan-halt synthesis.
  • Introduced per-story HITL checkpoints: plan checkpoint (PAUSE_PLAN_CHECKPOINT) and post-commit story checkpoint (PAUSE_STORY_CHECKPOINT).
  • Added [stories] policy section + settings schema, and pinned source/spec_folder into RunState for correct resume/resolve behavior.
  • Updated CLI (run/validate/status) for stories mode, including --spec, --story filtering, and stories-mode queue validation.
  • Hardened stories paths (manifest/spec/sentinel) against non‑UTF‑8 inputs and improved sentinel re-arm behavior (preserve + delete sentinels safely).
  • Added extensive unit tests covering scheduling, pause/resume, checkpoints, adapter read-back, sentinel handling, and verify behavior.

Technical Notes: Keeps sprint-mode behavior as the default; stories mode requires newer bmad-dev-auto support (content probe) and avoids sprint board synchronization by design.

🤖 Was this summary useful? React with 👍 or 👎

@augmentcode augmentcode Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. 4 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

base = folder if folder.is_absolute() else Path(spec.cwd) / folder
deadline = time.monotonic() + RESULT_GRACE_S
while True:
state = stories.resolve_story_spec(base, story_key)

@augmentcode augmentcode Bot Jul 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/bmad_loop/adapters/generic.py:510: _stories_result_json() no longer uses handle.launched_ns (unlike the mtime-scan path), so an already-terminal story spec from a previous step (notably the pre-existing done spec when starting a follow-up review session) could be misread as this session’s completion even if the session produced no new output.

Severity: high

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c0f6215. `_stories_result_json` now applies the same session-launch mtime floor the mtime-scan path uses (`st_mtime_ns >= handle.launched_ns`), so a spec left terminal by a prior step — notably the dev's `done` spec a follow-up review session re-opens — is no longer read as this session's completion. Confirmed reachable: the review session also runs `GenericDevAdapter` with `BMAD_LOOP_SPEC_FOLDER` set. Regression test added in tests/test_generic_tmux.py.

result = devcontract.synthesize_result(state.path, story_key=story_key or None)
if result.result_json is not None:
return result.result_json
if not wait or time.monotonic() >= deadline:

@augmentcode augmentcode Bot Jul 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/bmad_loop/adapters/generic.py:515: If resolve_story_spec() yields KIND_AMBIGUOUS (multiple <id>-*.md matches), this currently just waits and then returns None, which will look like a generic stall rather than an actionable “ambiguous story file” error requiring operator intervention.

Severity: medium

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in c0f6215. A `KIND_AMBIGUOUS` read-back now returns `None` immediately instead of polling the full grace; the engine's next `_pick_next` re-classifies AMBIGUOUS into an actionable wedge for resolve.

Comment thread src/bmad_loop/cli.py Outdated
print(e, file=sys.stderr)
return 1
if stories_on:
problem = _validate_stories_folder(paths, spec_folder)

@augmentcode augmentcode Bot Jul 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/bmad_loop/cli.py:323: In stories mode, a bad --story id isn’t validated here (only in _dry_run_stories), so the run can start, create state, and then crash later when StoriesEngine calls stories.schedule(..., selector=...) and gets StoriesError for an unknown id.

Severity: medium

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already addressed in a07875d — `cmd_run` now calls `_validate_stories_folder(paths, spec_folder, selector=args.story)` (cli.py:372), which rejects an unknown `--story` id before the run starts. This diff hunk predates that commit, so no further change is needed.

Comment thread src/bmad_loop/cli.py
help="force stories mode: dispatch the epic spec folder's stories.yaml by "
"folder+id (overrides [stories].source)",
)
run_p.add_argument("--epic", type=int, help="only stories from this epic (sprint mode)")

@augmentcode augmentcode Bot Jul 6, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/bmad_loop/cli.py:1384: When --spec forces stories mode, --epic is silently ignored (StoriesEngine disables epic_filter), which can surprise callers who pass both flags expecting filtering to apply.

Severity: low

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 34b0498. `run` now prints a stderr note when `--epic` is passed in stories mode (pointing to `--story` for per-id filtering) instead of silently ignoring the flag.

@pbean
pbean force-pushed the feat/spec-to-loop-stories-phase2 branch from e94b265 to 3171264 Compare July 7, 2026 00:40
@pbean pbean changed the title feat(stories): engine + adapter for folder+id dispatch (Phase 2) feat(stories): engine, adapter + HITL checkpoints for folder+id dispatch (Phase 2) Jul 7, 2026
@pbean
pbean force-pushed the feat/spec-to-loop-stories-phase1 branch from b7e2b97 to 032027a Compare July 7, 2026 03:46
@pbean
pbean force-pushed the feat/spec-to-loop-stories-phase2 branch 4 times, most recently from 3b55655 to 67b75e3 Compare July 7, 2026 04:29
pbean added a commit that referenced this pull request Jul 7, 2026
…ry-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>
@pbean
pbean force-pushed the feat/spec-to-loop-stories-phase2 branch from 2d018c5 to 4d4841f Compare July 7, 2026 06:43
@pbean pbean changed the title feat(stories): engine, adapter + HITL checkpoints for folder+id dispatch (Phase 2) feat(stories): spec-to-loop engine, adapter, CLI + HITL checkpoints (Phase 2 + 3) Jul 7, 2026
pbean added a commit that referenced this pull request Jul 7, 2026
…ry-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>
@pbean
pbean force-pushed the feat/spec-to-loop-stories-phase2 branch from 4d4841f to 457ae90 Compare July 7, 2026 16:16
pbean added a commit that referenced this pull request Jul 7, 2026
The presentation + E2E-matrix portion of stories mode, stacked on the Phase 2/3
engine/CLI/resolve layer in #77. Split from the original single Phase-4 commit so
the shared status-projection helpers + mode-aware CLI status/dry-run land in #77
(the code Phase 2/3 depends on) and this branch carries only the TUI surface, the
prose docs, and the E2E test matrix.

TUI (every action calls the same CLI code paths — no duplicated logic):
- Stories board replaces the sprint tree for a stories-mode run (state glyph, id,
  independent spec/done checkpoint cell, title); per-run pause-kind badge + a
  global "N need attention" count on the runs table.
- `p` opens the stage-appropriate viewer: plan-checkpoint spec review (Approve &
  resume / Request replan), story-checkpoint summary card (Continue / Stop),
  escalation with story context + blocking condition + sentinel indicator
  (Resolve / Re-arm & resume), and a gate spec viewer the spec-approval/epic
  pauses reuse.
- Start-run modal gains a source select (prefilled from [stories]) + spec-folder
  input with a live schedule preview; buttons docked so they stay reachable.

Docs: README dual-flow section + command/keybinding updates, FEATURES.md,
tui-guide HITL pages, CHANGELOG [Unreleased].

E2E (tests/test_stories_e2e.py — real run/resume/resolve binaries through real
tmux with a scripted fake claude): honest coverage map —
- (1) two-story happy path, (2) spec_checkpoint two-leg plan-halt + resume,
  (4) blocked → resolve → re-dispatch run as full sandbox CLI E2E here.
- (3) done_checkpoint and (5) worktree isolation are covered deterministically at
  the engine level (test_stories_engine.py), honestly documented as such.
- (6) sprint-mode regression is added as a real sandbox E2E in a follow-up commit
  on this branch (Wave-1c item 12); prior to it, only the engine-level preflight
  additivity test existed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
pbean added a commit that referenced this pull request Jul 7, 2026
…ry-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>
@pbean
pbean force-pushed the feat/spec-to-loop-stories-phase2 branch from 457ae90 to b51f218 Compare July 7, 2026 18:38
@pbean

pbean commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

@CodeRabbit review
augment review

@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown

@pbean I'll review the changes now.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@augmentcode augmentcode Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. 1 suggestion posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

Comment thread src/bmad_loop/engine.py
self._save()
started += 1
self._run_story(task)
self._after_story(task)

@augmentcode augmentcode Bot Jul 7, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/bmad_loop/engine.py:743: Since _after_story() can now raise a pause after completing work (e.g., stories checkpoints), resuming re-enters _loop() with the local started counter reset, which can let --max-stories dispatch more stories than intended after a pause/resume where completion happened in _finish_inflight. Consider making the max-stories guard consult durable run state so pauses/resumes can’t bypass the cap.

Severity: high

Other Locations
  • src/bmad_loop/engine.py:723

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in e56f9db. The dispatch gate now consults a durable `Engine._dispatched_count()` (= `len(state.tasks)`) instead of the `_loop`-local `started` counter, so a checkpoint pause/resume can no longer reset the count and dispatch past `--max-stories`. `StoriesEngine._max_stories_reached()` (the done_checkpoint skip-if-last guard) uses the same count so the two never drift. Regression test added.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/bmad_loop/install.py`:
- Around line 92-118: The content probe in missing_stories_support can still
crash when probe.read_text(encoding="utf-8") hits a non-UTF-8 or binary file
because UnicodeDecodeError is not handled by the current except OSError. Update
the read path in missing_stories_support so decode failures are treated like
missing/invalid skill content and appended to problems, similar to the existing
not-found case, rather than escaping the preflight. Use the existing symbols
missing_stories_support, STORIES_PROBE_SKILL, and STORIES_PROBE_FILE to locate
and adjust the probe handling.

In `@src/bmad_loop/runs.py`:
- Around line 565-570: The sentinel handling in runs.py is too filename-based,
so a real stories spec matching <story_key>-unresolved.md or
<story_key>-ambiguous.md can be mistaken for a sentinel and deleted. Update the
logic around _sentinel_condition and the re-arm path in the task/spec_file
handling to determine sentinel-ness from the stories resolver or sentinel kind
metadata/structure, not the basename alone, and apply the same fix anywhere the
same check is duplicated later in the file.

In `@tests/test_resolve.py`:
- Around line 276-289: The test in test_resolve.py is exercising the wrong
source path because _escalated_run() still defaults to sprint-status, so it
never validates stories-mode non-sentinel re-arming. Update the test to pass
source="stories" when calling _escalated_run(), so runs.rearm_escalation and
load_state verify the intended stories branch while still asserting the spec
remains and its status flips to ready-for-dev.
🪄 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: b0c8503d-6cb1-4ac0-bef6-d2e162d20616

📥 Commits

Reviewing files that changed from the base of the PR and between d7e0267 and b51f218.

📒 Files selected for processing (23)
  • src/bmad_loop/adapters/generic.py
  • src/bmad_loop/cli.py
  • src/bmad_loop/data/settings/core.toml
  • src/bmad_loop/data/skills/bmad-loop-resolve/SKILL.md
  • src/bmad_loop/engine.py
  • src/bmad_loop/install.py
  • src/bmad_loop/model.py
  • src/bmad_loop/policy.py
  • src/bmad_loop/resolve.py
  • src/bmad_loop/runs.py
  • src/bmad_loop/stories.py
  • src/bmad_loop/stories_engine.py
  • src/bmad_loop/verify.py
  • tests/test_cli.py
  • tests/test_generic_tmux.py
  • tests/test_install.py
  • tests/test_model.py
  • tests/test_policy.py
  • tests/test_resolve.py
  • tests/test_settings_schema.py
  • tests/test_stories.py
  • tests/test_stories_engine.py
  • tests/test_verify.py

Comment thread src/bmad_loop/install.py
Comment thread src/bmad_loop/runs.py Outdated
Comment thread tests/test_resolve.py
@pbean

pbean commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

@CodeRabbit review
augment review

@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown

@pbean I'll review the changes now.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@augmentcode augmentcode Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. 2 suggestions posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

Comment thread src/bmad_loop/resolve.py
"invoke_dev_with": entry.invoke_dev_with,
}
try:
st = stories.resolve_story_spec(folder, story_key)

@augmentcode augmentcode Bot Jul 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/bmad_loop/resolve.py:119: _stories_context() only catches OSError around resolve_story_spec(), but read_frontmatter(...).read_text(encoding="utf-8") can also raise UnicodeDecodeError for a corrupted/non-UTF-8 story spec/sentinel and crash build_context. Consider handling decode failures here so bmad-loop resolve still gets a best-effort context instead of blowing up.

Severity: medium

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 528171f. _stories_context() was best-effort by contract but except OSError missed UnicodeDecodeError (a ValueError); it now catches (OSError, UnicodeDecodeError) at all three reads — the load_stories guard, resolve_story_spec() (whose internal read_frontmatter reads a PRESENT spec), and the sentinel read_text. A non-UTF-8 spec/sentinel now degrades to folder-only context instead of crashing build_context. Regression tests added in tests/test_resolve.py (invalid-UTF-8 PRESENT spec + sentinel).

Comment thread src/bmad_loop/runs.py Outdated
dest_dir.mkdir(parents=True, exist_ok=True)
condition = ""
if spec_path.is_file():
condition = recorded_blocking_condition(spec_path.read_text(encoding="utf-8"))

@augmentcode augmentcode Bot Jul 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/bmad_loop/runs.py:641: _clear_sentinel() reads the sentinel via read_text(encoding="utf-8") without catching UnicodeDecodeError, so a binary/corrupted sentinel could crash rearm_escalation and leave the run stuck. Consider treating decode failures like an unreadable sentinel (e.g., preserve/delete without parsing the blocking-condition text, or journal an empty condition) so re-arm remains robust.

Severity: medium

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 528171f. _clear_sentinel() now wraps the blocking-condition read_text in except (OSError, UnicodeDecodeError) and defaults condition = "", while keeping shutil.copy2 + unlink outside the try — so a binary/corrupted sentinel is still preserved+deleted and re-arm completes (empty recorded condition) rather than wedging the run. Regression test added (test_rearm_tolerates_non_utf8_sentinel). The same pass also swept an unflagged sibling with the identical defect — stories_engine._journal_sentinel_detected() (tests/test_stories_engine.py::test_sentinel_detected_tolerates_non_utf8_sentinel).

pbean added a commit that referenced this pull request Jul 8, 2026
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).
@pbean

pbean commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator Author

augment review

@augmentcode augmentcode Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review completed. 1 suggestion posted.

Fix All in Augment

Comment augment review to trigger a new review at any time.

Comment thread src/bmad_loop/cli.py
folder = stories_mod.resolve_spec_folder(paths.project, spec_folder)
try:
story_set = stories_mod.load_stories(folder)
except stories_mod.StoriesError as e:

@augmentcode augmentcode Bot Jul 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/bmad_loop/cli.py:314: This only catches StoriesError, but stories_mod.load_stories() can also raise UnicodeDecodeError from read_text(encoding="utf-8"), which would crash stories-mode preflight/dry-run instead of producing a clean “stories mode: …” error. Consider handling decode failures consistently (either here or by wrapping them inside stories.load_stories).

Severity: medium

Other Locations
  • src/bmad_loop/cli.py:354
  • src/bmad_loop/cli.py:523
  • src/bmad_loop/cli.py:560
  • src/bmad_loop/stories_engine.py:151

Fix This in Augment

🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 52df1bd. This was the same UnicodeDecodeError bug class as 528171f, which missed exactly these two read paths. Rather than patch each of the 5 call sites (cli.py:314/354/523/560, stories_engine.py:141/151), I fixed it at the two source functions — DRY, keeps load_stories's "raises StoriesError" contract, and transparently also covers verify.py's review-path resolve_story_spec:

  • load_stories (stories.py:120) — the manifest read_text now converts UnicodeDecodeErrorStoriesError("stories.yaml is not valid UTF-8: …"), so every caller's existing except StoriesError renders the clean stories mode: … message instead of a traceback.
  • resolve_story_spec (stories.py:319) — a non-UTF-8 PRESENT spec's frontmatter read (read_frontmatter, verify.py:857) now degrades to status="" on (OSError, UnicodeDecodeError). That classifies as wedged (_classify) so the engine pauses for resolve — never silently skips — and state_label renders it as present, so --dry-run/status tables don't crash either.

verify.read_frontmatter itself is left untouched (shared with the sprint-status path — kept the change stories-scoped). Regression tests added per site: non-UTF-8 manifest (test_stories.py::test_load_non_utf8_raises_stories_error), non-UTF-8 PRESENT spec (test_resolve_non_utf8_present_degrades_to_unknown_status), and a CLI clean-error boundary (test_cli.py::test_stories_non_utf8_manifest_clean_error_not_crash).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant