Skip to content

feat(stories): spec-to-loop contract layer (Phase 1) - #65

Merged
pbean merged 4 commits into
mainfrom
feat/spec-to-loop-stories-phase1
Jul 7, 2026
Merged

feat(stories): spec-to-loop contract layer (Phase 1)#65
pbean merged 4 commits into
mainfrom
feat/spec-to-loop-stories-phase1

Conversation

@pbean

@pbean pbean commented Jul 5, 2026

Copy link
Copy Markdown
Collaborator

Phase 1 — pure contract layer for "stories mode"

First phase of adopting BMAD-METHOD PR #2549 (stories.yaml + folder+id dispatch) as an opt-in stories mode alongside the default sprint mode. This phase is pure: no engine or sprint-mode edits, so there is zero risk to the existing sprint flow. Engine/adapter/HITL wiring is gated on the upstream merge and lands in Phases 2–4.

What's here

src/bmad_loop/stories.py (new) — the strict, typed parser the orchestrator reads stories.yaml through:

  • StoryEntry / Stories dataclasses + load_stories(spec_folder). Validates required fields, unique ids, prefix-free ids, no status key, and the id charset ^[A-Za-z0-9]+(-[A-Za-z0-9]+)*$. Ids are str()-normalized before validation (defense against an LLM-authored unquoted id: 1 → int or id: 3.5 → float). spec_checkpoint/done_checkpoint are independent bools (default false); invoke_dev_with is verbatim free text. No depends_on / DAG — the list is strictly linear.
  • resolve_story_spec(spec_folder, id) → deterministic id-keyed on-disk state (pending / present+status / ambiguous / sentinel).
  • schedule(stories, states, selector)linear scan returning the first PENDING-or-resumable entry, skipping done, and stopping on a blocked/sentinel/ambiguous entry — distinguishing run-complete from run-wedged.

devcontract.synthesize_result — a plan_halt expected-terminal seam. A Halt after planning. dispatch leaves the spec at ready-for-dev; with plan_halt=True that becomes a success terminal (carrying a plan_halt marker), while the default keeps ready-for-dev non-terminal (died-mid-flight) exactly as today. It composes with _reconcile_generic_terminal_status — that path only reconciles a spec whose prose says done, so a plan-halt ready-for-dev (no such prose) is never clobbered. Default path is byte-identical.

verify.verify_dev_stories — modeled on verify_dev: keeps the spec-exists / workflow / status-expected / baseline / proof-of-work gates, drops the sprint-status gate, resolves the spec deterministically by id, and asserts the resolved filename's id prefix equals the task id.

Tests

  • tests/test_stories.py (new, 47 tests): parse/normalize incl. unquoted-int ids, float/charset/prefix-free/duplicate/status-key rejections, checkpoint bool defaults, linear schedule with resume states + blocked/sentinel/ambiguous stops + selector, resolve_story_spec.
  • tests/test_devcontract.py: ready-for-dev expected-terminal both ways + reconcile-composition guard.
  • tests/test_verify.py: verify_dev_stories gates incl. the no-sprint-gate differentiator.
  • tests/fixtures/stories.yaml: the gist's dogfooded example (quoted ids, both checkpoint flags, invoke_dev_with, no depends_on).

Full suite green (1472 passed, 1 skipped); trunk check clean.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Added “stories” mode support with a strictly validated stories.yaml, deterministic story scheduling, and automatic story-spec discovery with clear retry/wedge behaviors.
    • Introduced an optional “halt after planning” outcome using ready-for-dev, including result marking when plan-halt is enabled.
  • Bug Fixes
    • Improved dev verification reliability with centralized workflow/status/baseline/proof-of-work gating and stories-mode specific checks.
    • Refined plan-halt synthesis so success/failure paths match expectations, including blocked/overrun cases.
  • Tests
    • Added comprehensive stories contract, scheduling, verification, and plan-halt test coverage, plus new fixture data.

@coderabbitai

coderabbitai Bot commented Jul 5, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 64d10768-278a-40fd-9b77-f4bb67ef5e43

📥 Commits

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

📒 Files selected for processing (4)
  • src/bmad_loop/stories.py
  • src/bmad_loop/verify.py
  • tests/test_stories.py
  • tests/test_verify.py
🚧 Files skipped from review as they are similar to previous changes (3)
  • tests/test_verify.py
  • tests/test_stories.py
  • src/bmad_loop/stories.py

Walkthrough

This PR adds stories-mode support: plan-halt synthesis handling, a strict stories.yaml contract with deterministic story resolution and scheduling, and a verify_dev_stories gate. Fixtures and tests cover the new contract and flow.

Changes

Stories Mode Feature

Layer / File(s) Summary
Plan-halt terminal status
src/bmad_loop/devcontract.py, tests/test_devcontract.py
Adds PLAN_HALT_STATUS, extends synthesize_result(..., plan_halt=False), and updates terminal handling plus result enrichment for ready-for-dev outcomes.
stories.yaml contract
src/bmad_loop/stories.py, tests/fixtures/stories.yaml, tests/test_stories.py
Adds strict manifest models, parsing and ID validation, duplicate/prefix checks, find_entry(), a fixture manifest, and loader/lookup tests.
Story state resolution and scheduling
src/bmad_loop/stories.py, tests/test_stories.py
Adds StoryState, resolve_story_spec(), Schedule, schedule(), and _classify() for linear story dispatch and on-disk resolution.
Stories-mode verification gate
src/bmad_loop/verify.py, tests/test_verify.py
Adds verify_dev_stories() with story-spec resolution, workflow/status/baseline checks, proof-of-work validation, and coverage for success and failure paths.

Estimated code review effort: 3 (Moderate) | ~30 minutes

Poem

I hopped through stories, neat and bright,
With ready-for-dev glowing light.
A plan can halt, or keep on going,
While tests keep every path from blowing.
🐇 One tidy hop, then hop again!

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR changes stories-mode verification and plan-halt behavior, but linked issue #39 is about CLI engine liveness tri-state handling. Align the PR with issue #39 by implementing shared liveness probing for resolve/delete/archive, or relink the issue to this stories-mode work.
Out of Scope Changes check ⚠️ Warning Most changes add a stories-mode contract layer and verify paths, which are unrelated to the linked CLI liveness issue. Remove or separate the stories-mode and plan-halt work, or update the PR scope to match the intended issue.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and accurately summarizes the main changes around the stories contract layer.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ 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-phase1

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 5, 2026

Copy link
Copy Markdown
🤖 Augment PR Summary

Summary: Adds an opt-in “stories mode” contract layer (Phase 1) for reading/validating stories.yaml, resolving id-keyed story specs on disk, and verifying dev runs against that model.

Changes:

  • Introduces src/bmad_loop/stories.py with strict YAML parsing/validation (id normalization/charset, uniqueness, prefix-free ids, forbid status) plus deterministic <id>-*.md resolution and a linear scheduler.
  • Adds a plan_halt seam to devcontract.synthesize_result() so ready-for-dev can be treated as a success terminal only when explicitly requested, and marks successful plan-halt results.
  • Factors shared dev verification gates into _verify_shared_gates() to reduce drift between dev/bundle/stories verifiers.
  • Adds verify_dev_stories() to verify stories-mode runs by resolving the spec deterministically by story id and dropping the sprint-status gate.

Tests: Adds extensive coverage for stories parsing/scheduling/spec resolution, the plan-halt terminal behavior, and stories-mode verification paths.

🤖 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. 2 suggestions posted.

Fix All in Augment

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

Comment thread src/bmad_loop/stories.py Outdated
Comment thread src/bmad_loop/verify.py

@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.

🧹 Nitpick comments (3)
src/bmad_loop/verify.py (2)

1113-1113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Undocumented deferred import.

from . import stories is placed inside the function body with no comment. This is presumably to avoid a circular import (stories.py imports read_frontmatter/status_of from this module at the top level). Worth a one-line comment so a future refactor doesn't hoist it to module scope and reintroduce the cycle.

🤖 Prompt for 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.

In `@src/bmad_loop/verify.py` at line 1113, Add a brief one-line comment above the
deferred import inside the verify flow to explain that `from . import stories`
is intentionally kept local to avoid a circular dependency with `stories.py`
importing `read_frontmatter` and `status_of` from this module. Keep the import
in the function body and make the intent explicit so future refactors don’t move
it to module scope and reintroduce the cycle.

1094-1169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicate gate logic between verify_dev and verify_dev_stories.

Workflow-tag, expected-status, baseline-match, and proof-of-work checks (Lines 1137-1166) are copy-pasted from verify_dev (Lines 991-1020). Consider factoring the shared post-resolution gates into a helper both functions call, taking the resolved spec_path as a parameter, so the sprint-mode and stories-mode gates can't silently drift apart.

♻️ Sketch of a shared helper
def _verify_common_gates(
    spec_path: Path, rj: dict, task: StoryTask, paths: ProjectPaths, review_enabled: bool
) -> VerifyOutcome | None:
    """Shared workflow/status/baseline/proof-of-work gates. Returns a failing
    VerifyOutcome, or None when all gates pass (caller then sets task.spec_file)."""
    workflow = rj.get("workflow")
    if workflow != DEV_WORKFLOW:
        return VerifyOutcome.retry(...)
    expected = "in-review" if review_enabled else "done"
    fm = read_frontmatter(spec_path)
    status = status_of(fm)
    if status != expected:
        return VerifyOutcome.retry(...)
    ...
    return None
🤖 Prompt for 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.

In `@src/bmad_loop/verify.py` around lines 1094 - 1169, The post-resolution
verification logic is duplicated between verify_dev_stories and verify_dev,
which risks drift in the workflow/status/baseline/proof-of-work gates. Extract
the shared checks into a helper such as _verify_common_gates that takes
spec_path, rj, task, paths, and review_enabled, performs the common gates, and
returns either a failing VerifyOutcome or None. Then have verify_dev_stories
call that helper after stories.resolve_story_spec and only set task.spec_file /
pass when it succeeds, keeping the stories-specific resolution and id-prefix
validation in verify_dev_stories.
tests/test_stories.py (1)

49-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Unescaped regex metacharacters in pytest.raises(match=...).

Ruff RUF043: these match= strings contain a literal . (in "stories.yaml") that is a regex metacharacter but isn't escaped. It happens to still match correctly, but re.escape() makes intent explicit and avoids future accidental collisions.

🔧 Suggested fix
-    with pytest.raises(stories.StoriesError, match="no stories.yaml found"):
+    with pytest.raises(stories.StoriesError, match=re.escape("no stories.yaml found")):

(apply similarly at lines 214 and 310)

Also applies to: 214-214, 310-310

🤖 Prompt for 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.

In `@tests/test_stories.py` at line 49, The pytest.raises(match=...) assertions in
stories tests use literal error text containing regex metacharacters, so the
match strings should be escaped to satisfy Ruff RUF043. Update the match
arguments in the relevant tests in stories-related functions to use a regex-safe
form, such as escaping the literal “stories.yaml” text, and apply the same fix
to the other matching assertions noted in the review. Keep the expected message
content the same while making the regex intent explicit.

Source: Linters/SAST tools

🤖 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.

Nitpick comments:
In `@src/bmad_loop/verify.py`:
- Line 1113: Add a brief one-line comment above the deferred import inside the
verify flow to explain that `from . import stories` is intentionally kept local
to avoid a circular dependency with `stories.py` importing `read_frontmatter`
and `status_of` from this module. Keep the import in the function body and make
the intent explicit so future refactors don’t move it to module scope and
reintroduce the cycle.
- Around line 1094-1169: The post-resolution verification logic is duplicated
between verify_dev_stories and verify_dev, which risks drift in the
workflow/status/baseline/proof-of-work gates. Extract the shared checks into a
helper such as _verify_common_gates that takes spec_path, rj, task, paths, and
review_enabled, performs the common gates, and returns either a failing
VerifyOutcome or None. Then have verify_dev_stories call that helper after
stories.resolve_story_spec and only set task.spec_file / pass when it succeeds,
keeping the stories-specific resolution and id-prefix validation in
verify_dev_stories.

In `@tests/test_stories.py`:
- Line 49: The pytest.raises(match=...) assertions in stories tests use literal
error text containing regex metacharacters, so the match strings should be
escaped to satisfy Ruff RUF043. Update the match arguments in the relevant tests
in stories-related functions to use a regex-safe form, such as escaping the
literal “stories.yaml” text, and apply the same fix to the other matching
assertions noted in the review. Keep the expected message content the same while
making the regex intent explicit.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d0fc846e-8174-423c-8c66-4a7ca85a0f14

📥 Commits

Reviewing files that changed from the base of the PR and between abd09d6 and b7e2b97.

📒 Files selected for processing (7)
  • src/bmad_loop/devcontract.py
  • src/bmad_loop/stories.py
  • src/bmad_loop/verify.py
  • tests/fixtures/stories.yaml
  • tests/test_devcontract.py
  • tests/test_stories.py
  • tests/test_verify.py

pbean and others added 2 commits July 6, 2026 20:00
Pure contract layer for the spec-to-loop "stories mode" (BMAD-METHOD PR
flow.

- stories.py: StoryEntry/Stories + load_stories() — a strict typed parser
  for the flat, linear stories.yaml (str()-normalized ids as int/float
  coercion defense, charset + prefix-free + no-status + unique validation,
  independent spec_checkpoint/done_checkpoint bools, verbatim
  invoke_dev_with; NO depends_on/DAG). Plus resolve_story_spec() (id-keyed
  disk state) and a linear schedule() distinguishing run-complete from
  run-wedged (blocked/sentinel stops the scan).
- devcontract.synthesize_result: plan_halt seam so a `Halt after planning.`
  dispatch treats frontmatter status ready-for-dev as success-terminal
  (with a plan_halt marker), while the default keeps it non-terminal
  (died-mid-flight). Composes with _reconcile_generic_terminal_status,
  which only reconciles done-prose specs and so never clobbers this leg.
- verify.verify_dev_stories: verify_dev minus the sprint-status gate, with
  deterministic id-keyed resolution and an id-prefix assertion.
- Tests + a stories.yaml fixture from the gist's dogfooded example.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…fest" wording

Two contract-layer hardenings surfaced by the post-hoc stories-mode audit:

- Reject ids that differ only by case. Story specs resolve by the `<id>-*.md`
  glob, which is case-insensitive on Windows/macOS filesystems (both in the CI
  matrix), so `Auth` and `auth` — or `Auth` and `auth-2` — would cross-match the
  same files and make resolution filesystem-dependent. load_stories now rejects
  equal-casefold duplicates, and _validate_prefix_free folds case so a
  case-insensitive prefix (`Auth` vs `auth-2`) is caught too.
- The forbidden-'status'-key error said "never in the manifest"; "manifest" is
  the plugin layer's word for plugin.toml. Use the locked user-facing vocabulary
  "stories.yaml" instead.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@pbean
pbean force-pushed the feat/spec-to-loop-stories-phase1 branch from b7e2b97 to 032027a Compare July 7, 2026 03:46

@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.

🧹 Nitpick comments (1)
tests/test_stories.py (1)

49-49: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Escape regex metacharacters in pytest.raises(match=...).

Static analysis flags unescaped . in match patterns (e.g., "stories.yaml"), which is technically a wildcard rather than a literal dot. Functionally harmless here, but use re.escape() for precision.

🧹 Proposed fix
-    with pytest.raises(stories.StoriesError, match="no stories.yaml found"):
+    with pytest.raises(stories.StoriesError, match=re.escape("no stories.yaml found")):

Apply similarly at lines 243 and 339.

Also applies to: 243-243, 339-339

🤖 Prompt for 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.

In `@tests/test_stories.py` at line 49, The pytest.raises match strings in the
Stories tests use unescaped regex metacharacters, so update the affected
assertions to use re.escape() for the literal error text. Apply this in the
tests around stories.StoriesError and the other matching checks in the same test
module, especially the assertions in the relevant test functions near the
current match patterns, so the regex matches the exact message rather than
treating punctuation like "." as wildcards.

Source: Linters/SAST tools

🤖 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.

Nitpick comments:
In `@tests/test_stories.py`:
- Line 49: The pytest.raises match strings in the Stories tests use unescaped
regex metacharacters, so update the affected assertions to use re.escape() for
the literal error text. Apply this in the tests around stories.StoriesError and
the other matching checks in the same test module, especially the assertions in
the relevant test functions near the current match patterns, so the regex
matches the exact message rather than treating punctuation like "." as
wildcards.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5934f5ad-251d-443c-91bc-c9de4c97f7b8

📥 Commits

Reviewing files that changed from the base of the PR and between b7e2b97 and 032027a.

📒 Files selected for processing (7)
  • src/bmad_loop/devcontract.py
  • src/bmad_loop/stories.py
  • src/bmad_loop/verify.py
  • tests/fixtures/stories.yaml
  • tests/test_devcontract.py
  • tests/test_stories.py
  • tests/test_verify.py
🚧 Files skipped from review as they are similar to previous changes (6)
  • tests/fixtures/stories.yaml
  • src/bmad_loop/devcontract.py
  • src/bmad_loop/verify.py
  • tests/test_devcontract.py
  • tests/test_verify.py
  • src/bmad_loop/stories.py

@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.

✅ 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/verify.py Outdated
…ories (PR #65 review)

Address the augmentcode review on the Phase 1 contract layer:
- resolve_story_spec: guard the `<id>-*.md` glob with ID_RE so a non-charset id
  (glob metachar / path separator) fails clean to PENDING instead of an injected
  or mis-widened match (medium).
- verify_dev_stories: normalize `story_key` via str().strip() before the
  filename-prefix check, matching the resolver, so a padded key can't spuriously
  fail resolution (low).
- verify_dev_stories: honor the devcontract plan_halt terminal — a `plan_halt`
  leg expects `ready-for-dev` and skips proof-of-work (a plan writes only its own
  spec). Keeps Phase 1 consistent with the plan_halt seam it already ships (medium).

Nitpicks: document the deferred `from . import stories` (verify<->stories cycle
avoidance); re.escape() the literal-dot pytest.raises match patterns.

Tests: metachar/path-sep id -> PENDING; whitespace story_key passes; plan_halt
accepts ready-for-dev + skips proof-of-work, and rejects a non-plan status.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@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.

🧹 Nitpick comments (1)
src/bmad_loop/verify.py (1)

1135-1139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicated literal risks silent drift with devcontract.PLAN_HALT_STATUS.

The comment explains the duplication is intentional to avoid an import cycle, but nothing guards against the two literals diverging later (e.g., if devcontract's plan-halt status token changes). A cheap cross-module test asserting verify.PLAN_HALT_STATUS == devcontract.PLAN_HALT_STATUS would catch drift without reintroducing the import cycle.

Suggested test addition
def test_plan_halt_status_matches_devcontract():
    from bmad_loop import devcontract
    assert verify.PLAN_HALT_STATUS == devcontract.PLAN_HALT_STATUS
🤖 Prompt for 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.

In `@src/bmad_loop/verify.py` around lines 1135 - 1139, Add a cross-module test to
guard the duplicated plan-halt status literal from drifting. Update the test
suite around verify.PLAN_HALT_STATUS so it imports bmad_loop.devcontract inside
the test and asserts verify.PLAN_HALT_STATUS matches
devcontract.PLAN_HALT_STATUS, using the existing PLAN_HALT_STATUS symbol to
locate the check without changing the no-cycle design.
🤖 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.

Nitpick comments:
In `@src/bmad_loop/verify.py`:
- Around line 1135-1139: Add a cross-module test to guard the duplicated
plan-halt status literal from drifting. Update the test suite around
verify.PLAN_HALT_STATUS so it imports bmad_loop.devcontract inside the test and
asserts verify.PLAN_HALT_STATUS matches devcontract.PLAN_HALT_STATUS, using the
existing PLAN_HALT_STATUS symbol to locate the check without changing the
no-cycle design.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5b8c4733-9c1e-4b4e-913c-34f80c373f66

📥 Commits

Reviewing files that changed from the base of the PR and between 032027a and 3fcc182.

📒 Files selected for processing (4)
  • src/bmad_loop/stories.py
  • src/bmad_loop/verify.py
  • tests/test_stories.py
  • tests/test_verify.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • tests/test_stories.py
  • src/bmad_loop/stories.py

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

pbean commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

Addressed the CodeRabbit nitpicks in 3fcc182:

  • Undocumented deferred import (verify.py): added a comment on from . import stories inside verify_dev_stories explaining it stays local to avoid the verify↔stories import cycle (stories imports read_frontmatter/status_of from this module at top level).
  • Regex metacharacters in pytest.raises(match=...) (test_stories.py): wrapped the literal-dot messages (no stories.yaml found, story id not found in stories.yaml) in re.escape().
  • Duplicate gate logic between verify_dev / verify_dev_stories: declining for this PR. Extracting a shared helper would have to touch sprint-mode verify_dev, which is out of scope for the stories contract layer and adds risk; the mode-isolated duplication is intentional. Worth revisiting if a third mode makes a shared verifier clearly net-positive.

@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 latest changes.

✅ 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/stories.py Outdated
# caller already handles, matching the module's fail-loud-not-slip rule.
return StoryState(kind=KIND_PENDING)
stories_dir = Path(spec_folder) / STORIES_SUBDIR
matches = sorted(stories_dir.glob(f"{sid}-*.md")) if stories_dir.is_dir() else []

@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.

In resolve_story_spec() (src/bmad_loop/stories.py:290), the <id>-*.md glob can match a differently-cased filename on Windows/macOS (case-insensitive FS), which would make story resolution/scheduling OS-dependent. Consider filtering matches to those whose name starts with the exact-case f"{sid}-" before deciding PRESENT/AMBIGUOUS.

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 d7e0267: resolve_story_spec now filters the <id>-*.md glob to names starting with the exact-case <id>- prefix, so resolution is deterministic across filesystems — a case-insensitive FS (Windows CI, macOS) can no longer match a differently-cased file that a case-sensitive FS never would. This also keeps it in step with the exact-case sentinel comparison and verify's id-prefix gate (a wrong-case hit would previously resolve here but then fail those as a spurious retry). Covered by test_resolve_wrong_case_id_is_pending.

Comment thread src/bmad_loop/verify.py
# Generic path always self-finalizes to done (no in-review handoff); the
# review_enabled arm mirrors verify_dev for symmetry. A plan-halt leg instead
# expects the ready-for-dev plan gate (the plan is done, not the code).
if plan_halt:

@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.

In verify_dev_stories() (src/bmad_loop/verify.py:1205), when plan_halt=True you accept ready-for-dev as a success terminal and skip proof-of-work, but there’s no cross-check that result_json actually carries the plan_halt marker. If a caller accidentally passes plan_halt=True, a died-mid-flight ready-for-dev could be treated as a successful plan-halt leg.

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 d7e0267: a plan_halt=True leg now also requires result.json to carry the plan_halt marker devcontract.synthesize_result emits only on a clean plan-halt (plan_halt and status==ready-for-dev and not escalations). A died-mid-flight ready-for-dev never carries it, so it can no longer be promoted to a "successful plan" by a caller that flips the flag — this mirrors the defensive id-prefix gate just above. Covered by test_verify_dev_stories_plan_halt_requires_marker.

…+ shared verify gates (PR #65 review)

Second-round PR #65 review fixes.

- resolve_story_spec (stories.py): filter the `<id>-*.md` glob to the exact-case
  `<id>-` prefix so resolution is deterministic across filesystems. On a
  case-insensitive FS (Windows CI, macOS) the glob would otherwise match a
  differently-cased file that a case-sensitive FS never would — and a wrong-case
  hit would then fail the exact-case sentinel comparison / verify id-prefix gate
  as a spurious retry. (augment comment 3538118749)
  Covered by test_resolve_wrong_case_id_is_pending.

- verify_dev_stories (verify.py): a plan_halt leg now also requires result.json
  to carry the `plan_halt` marker devcontract emits on a clean plan-halt, so a
  died-mid-flight `ready-for-dev` can't be promoted to a "successful plan" by a
  caller that flips plan_halt=True. Mirrors the defensive id-prefix gate above.
  (augment comment 3538118753) Covered by
  test_verify_dev_stories_plan_halt_requires_marker.

- Extract _verify_shared_gates: the workflow/status/baseline/proof-of-work gates
  copy-pasted across verify_dev / verify_dev_bundle / verify_dev_stories are
  factored into one helper (expected_status + proof_exclude params;
  proof_exclude=None skips proof-of-work for a plan leg) so sprint- and
  stories-mode gates can't drift. Behavior-preserving; existing suite is the net.
  (CodeRabbit nitpick)

- Add test_plan_halt_status_matches_devcontract: guards verify.PLAN_HALT_STATUS
  (a deliberate literal copy, kept to avoid an import cycle) against drifting
  from devcontract.PLAN_HALT_STATUS. (CodeRabbit nitpick)

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
pbean added a commit that referenced this pull request Jul 7, 2026
… 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>
@pbean

pbean commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator Author

Note on the pre-merge Linked Issues / Out of Scope Changes warnings: these are false positives. This PR is Phase 1 of the stories-mode stack (pure stories.yaml contract layer — parser + plan_halt seam + verify_dev_stories, no engine/sprint edits). It has no linked issueclosingIssuesReferences is empty and there are no connected-issue events. #39 (which the check references) is an unrelated, already-merged PR about CLI liveness tri-state on win32, not an issue this PR addresses. Nothing to relink; flagging so the scope reads correctly for reviewers and the heuristic realigns on the next pass.

@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/verify.py
task,
paths,
expected_status=expected,
proof_exclude=None if plan_halt else artifact_relpaths(paths),

@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/verify.py:1231 — In verify_dev_stories(), the proof-of-work exclusion uses artifact_relpaths(paths), which excludes the entire planning/implementation artifacts trees. This differs from verify_dev/verify_dev_bundle’s narrower exclude and may cause false “no changes since baseline” retries when legitimate work is artifact-only (e.g., deferred-work ledger or other story specs).

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.

Valid — this is the #79 false-negative (KNOWN-BUG-ledger-only-story-false-no-changes) re-surfacing in stories mode: the whole-folder artifact_relpaths exclude swallows a ledger/spec-only diff under implementation_artifacts so it reads as "no changes since baseline".

Fixed in the stacked Phase 2 (PR #77) by d55993f, which swaps artifact_relpaths(paths)verify_dev_exclude_relpaths(paths, spec_path) + _stories_relpaths(paths.project, spec_folder) (the file-granular post-#79 contract: only the session's own spec + sprint_status, plus the spec folder's stories/ + stories.yaml). Regression test: tests/test_verify.py::test_verify_dev_stories_ledger_only_counts_as_real_work.

Kept in Phase 2 rather than back-ported here because the fix requires _stories_relpaths (a Phase 2 helper) and verify_dev_stories is not yet wired to any engine in this pure contract layer — the exclude breadth has no runtime effect until Phase 2 makes it reachable.

@pbean
pbean merged commit c6d033d into main Jul 7, 2026
9 checks passed
pbean added a commit that referenced this pull request Jul 8, 2026
… 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>
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
pbean deleted the feat/spec-to-loop-stories-phase1 branch July 9, 2026 16:08
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