refactor: split frontmatter parsing + VerifyOutcome out of verify.py (#242)#253
refactor: split frontmatter parsing + VerifyOutcome out of verify.py (#242)#253pbean wants to merge 2 commits into
Conversation
…242) Pure domain modules imported backward into verify.py (a 1,791-line git/subprocess module): stories.py pulled read_frontmatter/status_of and escalation.py pulled VerifyOutcome, dragging the whole git surface into modules that have no need for it (assessment finding F-1). - New src/bmad_loop/frontmatter.py holds read_frontmatter, status_of, and set_frontmatter_status — pure file/markdown parsing, zero git imports. - VerifyOutcome (frozen dataclass, no git deps) moves to model.py. - verify.py re-exports all moved names (explicit noqa: F401 on the re-export-only set_frontmatter_status, which the F401 autofix would otherwise delete) so every existing verify.<name> / from .verify import <name> call site and test stays green. - stories.py and escalation.py import from the new homes. Behavior-preserving move only — no logic edits.
WalkthroughFrontmatter parsing and status mutation move into ChangesFrontmatter and verification refactor
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/frontmatter.py`:
- Around line 31-40: Replace delimiter-based split logic with one shared
boundary finder that recognizes opening and closing `---` only when they occupy
standalone lines. In `src/bmad_loop/frontmatter.py` lines 31-40, use it before
YAML parsing; reuse the same helper in `src/bmad_loop/frontmatter.py` lines
68-86 for status rewriting and in `src/bmad_loop/verify.py` lines 923-941 for
updating or appending fields, preserving scalar values containing `---` and
avoiding duplicated boundary logic.
🪄 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: bdeb7553-9a96-4305-a546-70b28658a021
📒 Files selected for processing (5)
src/bmad_loop/escalation.pysrc/bmad_loop/frontmatter.pysrc/bmad_loop/model.pysrc/bmad_loop/stories.pysrc/bmad_loop/verify.py
…review)
CodeRabbit flagged a pre-existing latent bug carried over verbatim by the split:
`text.split("---", 2)` treats a `---` SUBSTRING inside a scalar value (e.g.
`title: 'restore --- review'`) as the closing frontmatter delimiter. `read_frontmatter`
then `yaml.safe_load`s a truncated block -> YAMLError -> {}, so `status_of` reads ""
and status gates misfire; `set_frontmatter_field`'s insert path could jam a field
inside the scalar and corrupt the spec.
Add a shared `_split_frontmatter` boundary finder in frontmatter.py that recognizes
`---` ONLY as a standalone delimiter line, returning (before, block, after) with
`before + block + after == text` for byte-for-byte faithful rewrites. Wire it into
read_frontmatter + set_frontmatter_status (frontmatter.py) and set_frontmatter_field
(verify.py, imports the shared helper). Behavior is unchanged for every existing
spec (delimiters are standalone `---` lines); only the `---`-in-value case is fixed.
Regression tests for the read + both writer paths (previously unexercised).
Closes #242 (assessment finding F-1).
Problem
Pure domain modules imported backward into
verify.py— a 1,791-line git/subprocess module:stories.py→from .verify import read_frontmatter, status_ofescalation.py→from .verify import VerifyOutcomeThat drags the whole git/subprocess surface into modules that have no git needs, and leaves the pure frontmatter parsing impossible to unit-test or type-check in isolation.
Change (behavior-preserving move only — no logic edits)
src/bmad_loop/frontmatter.pyholdingread_frontmatter,status_of, andset_frontmatter_status— pure file/markdown parsing, stdlib + PyYAML only, zero git imports. (The three functions are moved verbatim.)VerifyOutcome(frozen dataclass, no git deps) moves tomodel.py.verify.pyre-exports all moved names so every existingverify.<name>andfrom .verify import <name>call site (devcontract, engine, runs, sweep, tests) and monkeypatch target stays valid. The re-export-onlyset_frontmatter_statuscarries an explicit# noqa: F401(ruff's F401 autofix would otherwise silently delete it);read_frontmatter/status_of/VerifyOutcomeare also used internally by verify so they need none.import yamlis dropped from verify (its only use moved out).stories.pyandescalation.pynow import from the new homes (.frontmatter/.model).set_frontmatter_fieldintentionally stays inverify.py— it is out of scope for F-1 (no pure-module imports it) and its move is not requested.Verification
test_module_skills_sync.py—bmad-loop-setup/assets/module.yaml) are pre-existing local-install drift in.agents/skills/.claude/skills(files not even tracked onmain); this branch touches no skill files.trunk check --no-fixon all changed files: no issues (confirms F401 does not flag the re-exports).No CHANGELOG entry — not user-visible.
Summary by CodeRabbit
New Features
Refactor
Update — review follow-up (commit
d455277)Beyond the verbatim move, this PR now also hardens a pre-existing delimiter bug the automated reviewer flagged:
text.split("---", 2)treated a---substring inside a scalar value (e.g.title: 'restore --- review') as the closing frontmatter delimiter — soread_frontmatterreturned{}(silently zeroingstatus) andset_frontmatter_field's insert path could corrupt the spec. That logic was pre-existing (moved verbatim fromverify.py), but rather than defer it, the fix adds a shared_split_frontmatterboundary finder infrontmatter.pythat recognizes---only as a standalone delimiter line (reconstructing the file byte-for-byte), used by all three sites, plus regression tests for the read + both writer paths. This second commit is deliberately separate so the pure move stays reviewable on its own. Full suite: 2719 passed (the same 2 unrelatedtest_module_skills_sync.pylocal-install failures).