feat(stories): spec-to-loop contract layer (Phase 1)#65
Conversation
Pure contract layer for the spec-to-loop "stories mode" (BMAD-METHOD PR #2549). No engine or sprint-mode edits — zero risk to the default sprint 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>
WalkthroughThis PR adds "stories mode" support: a ChangesStories Mode Feature
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant schedule
participant resolve_story_spec
participant verify_dev_stories
Caller->>schedule: schedule(stories, states)
schedule->>resolve_story_spec: resolve unresolved story state
resolve_story_spec-->>schedule: StoryState (pending/present/sentinel/ambiguous)
schedule-->>Caller: Schedule(next actionable entry)
Caller->>verify_dev_stories: verify_dev_stories(task, spec_folder)
verify_dev_stories->>resolve_story_spec: resolve(spec_folder, task.story_key)
resolve_story_spec-->>verify_dev_stories: StoryState
verify_dev_stories-->>Caller: VerifyOutcome
Estimated code review effort: 3 (Moderate) | ~30 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Augment PR SummarySummary: Introduces an opt-in “stories mode” contract layer to parse and reason about Changes:
Technical Notes: Story ids are normalized to strings (defense against unquoted YAML numbers), must be unique/prefix-free, and are restricted to 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| """ | ||
| sid = str(story_id).strip() | ||
| stories_dir = Path(spec_folder) / STORIES_SUBDIR | ||
| matches = sorted(stories_dir.glob(f"{sid}-*.md")) if stories_dir.is_dir() else [] |
There was a problem hiding this comment.
resolve_story_spec() builds a glob pattern from story_id (glob(f"{sid}-*.md")) without validating sid against ID_RE. If story_id can come from CLI/task input, glob metacharacters or path separators could cause unintended matches (or an incorrect AMBIGUOUS/SENTINEL outcome) instead of a clean failure.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
| spec_path = state.path | ||
| # The glob is `<id>-*.md`, so this holds by construction — assert it anyway as | ||
| # a defensive gate against a future resolver change silently widening the match. | ||
| if spec_path is None or not spec_path.name.startswith(f"{story_id}-"): |
There was a problem hiding this comment.
resolve_story_spec() normalizes ids via str(story_id).strip(), but this filename-prefix check uses the raw task.story_key. If story_key ever contains leading/trailing whitespace (or a non-str), the resolver could succeed while this check fails, causing a spurious retry; consider reusing the same normalized id for the prefix assertion/message.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/bmad_loop/verify.py (2)
1113-1113: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUndocumented deferred import.
from . import storiesis placed inside the function body with no comment. This is presumably to avoid a circular import (stories.py importsread_frontmatter/status_offrom 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 winDuplicate gate logic between
verify_devandverify_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 resolvedspec_pathas 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 valueUnescaped 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, butre.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
📒 Files selected for processing (7)
src/bmad_loop/devcontract.pysrc/bmad_loop/stories.pysrc/bmad_loop/verify.pytests/fixtures/stories.yamltests/test_devcontract.pytests/test_stories.pytests/test_verify.py
1 participant
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 readsstories.yamlthrough:StoryEntry/Storiesdataclasses +load_stories(spec_folder). Validates required fields, unique ids, prefix-free ids, nostatuskey, and the id charset^[A-Za-z0-9]+(-[A-Za-z0-9]+)*$. Ids arestr()-normalized before validation (defense against an LLM-authored unquotedid: 1→ int orid: 3.5→ float).spec_checkpoint/done_checkpointare independent bools (defaultfalse);invoke_dev_withis verbatim free text. Nodepends_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, skippingdone, and stopping on ablocked/sentinel/ambiguous entry — distinguishing run-complete from run-wedged.devcontract.synthesize_result— aplan_haltexpected-terminal seam. AHalt after planning.dispatch leaves the spec atready-for-dev; withplan_halt=Truethat becomes a success terminal (carrying aplan_haltmarker), while the default keepsready-for-devnon-terminal (died-mid-flight) exactly as today. It composes with_reconcile_generic_terminal_status— that path only reconciles a spec whose prose saysdone, so a plan-haltready-for-dev(no such prose) is never clobbered. Default path is byte-identical.verify.verify_dev_stories— modeled onverify_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-devexpected-terminal both ways + reconcile-composition guard.tests/test_verify.py:verify_dev_storiesgates incl. the no-sprint-gate differentiator.tests/fixtures/stories.yaml: the gist's dogfooded example (quoted ids, both checkpoint flags,invoke_dev_with, nodepends_on).Full suite green (1472 passed, 1 skipped);
trunk checkclean.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes