Release 0.7.8 — reconcile stale spec frontmatter status (no more false defers)#25
Conversation
….7.8 bmad-dev-auto can finalize a run in its `## Auto Run Result` prose (Status: done) but leave the spec frontmatter at the template default `draft`. Every gate reads frontmatter, so the sprint/ledger sync no-op'd and the story or sweep bundle falsely deferred — losing tested work on rollback. The orchestrator now reconciles the frontmatter to the success status from the terminal prose in _dev_phase before the gates run (_reconcile_generic_terminal_status, journaled spec-status-reconciled), covering both the story (sprint) and bundle (ledger) paths. Only a `done` outcome from a non-terminal status is reconciled, and every deterministic gate (worktree diff, dw-ids, verify commands, ledger) still runs — so prose never passes a gate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds generic-dev reconciliation for stale frontmatter when prose reports ChangesFrontmatter reconciliation feature
Version bump to 0.7.8
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: This PR releases Changes:
Technical Notes: Reconciliation is explicitly bookkeeping-only (allowlisted, never overrides blocked/terminal/unknown states) and all deterministic gates (diff, dw-id checks, verify commands, ledger checks) still run afterward. 🤖 Was this summary useful? React with 👍 or 👎 |
![augmentcode[bot]](https://avatars.githubusercontent.com/in/1027498?s=60&v=4){kind=small}
| # allowlist: anything else (already-terminal done/in-review, blocked, or an | ||
| # unknown custom token) is left untouched, so reconciliation can never override a | ||
| # status the skill set on purpose. | ||
| RECONCILABLE_FROM = frozenset({"", "draft", "ready-for-dev", "in-progress"}) |
There was a problem hiding this comment.
RECONCILABLE_FROM includes the empty string (""), but reset_spec_status()’s _FM_STATUS_RE requires a non-empty [A-Za-z-]+ value, so a spec with a missing/empty status: line won’t actually be reconciled (silent no-op). If "" is intended to cover that case, reconciliation can’t currently perform the rewrite.
Severity: low
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Confirmed and fixed in 61154ed. Rather than drop "" from the allowlist, we made it actionable:
_FM_STATUS_RE's value token was relaxed[A-Za-z-]+→[A-Za-z-]*, so a present-but-emptystatus:(e.g.status:/status: "") is now matched and filled.reset_spec_statusnow inserts astatus:line before the closing fence when the frontmatter has none (line-ending-aware), so a missing status is reconciled too.
Added test_reset_status_fills_empty_value and test_reset_status_inserts_missing_line to cover both. Thanks for spotting the silent no-op.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/automator/engine.py`:
- Around line 1041-1046: The reconciliation in the engine is making
orchestrator-owned spec rewrites count as proof-of-work, so the verify/no-change
gate can pass on bookkeeping-only edits. Update the change-detection path used
around the _reconcile_generic_terminal_status flow to ignore protected artifact
locations like _bmad-output, reusing the same protected-path logic as
_protected_relpaths() and rollback so reconciliation-only edits never register
as real work. Ensure the no-change check and related verification gates exclude
these orchestrator-managed paths before deciding whether the session has
meaningful changes.
🪄 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: b60b8082-a6b0-4a44-a31a-005fa2f6ef70
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.claude-plugin/marketplace.jsonCHANGELOG.mdmodule.yamlpyproject.tomlsrc/automator/__init__.pysrc/automator/data/skills/bmad-auto-setup/assets/module.yamlsrc/automator/devcontract.pysrc/automator/engine.pytests/conftest.pytests/test_devcontract.pytests/test_engine.pytests/test_sweep.py
Address PR #25 automated-reviewer findings. CodeRabbit (major): _reconcile_generic_terminal_status rewrites the spec under _bmad-output before the verify gate. `bmad-auto init` does not gitignore _bmad-output, so the skill's spec authorship + the reconcile rewrite registered as a git change — and since reconcile flips the frontmatter to the success status, the status backstop no longer caught empty sessions, letting no-code work pass `has_changes_since`. - has_changes_since gains an `exclude` param (mirrors attempt_dirty); new verify.artifact_relpaths() returns the orchestrator-owned artifact folders (same set as Engine._protected_relpaths). verify_dev and verify_dev_bundle pass it so only real source edits count as proof. - Rewrite test_generic_reconcile_does_not_bypass_no_change_gate to drop the _bmad-output gitignore hack (and the false #2522 premise); the gate now defers spec-only work on a real tracked tree. Augment (low): "" in RECONCILABLE_FROM was a no-op — reset_spec_status could not rewrite a blank/missing status. Make it actionable instead: relax _FM_STATUS_RE to fill an empty value, and insert a status: line when the frontmatter has none. Adds devcontract coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
augment review |
| if not spec_path.is_file(): | ||
| return | ||
| success_status = "in-review" if self._dev_review_enabled() else "done" | ||
| fm_status = verify.status_of(verify.read_frontmatter(spec_path)) |
There was a problem hiding this comment.
verify.status_of() will return the string "none" when frontmatter contains a bare status: with no value (YAML null), so this reconciliation won’t run even though RECONCILABLE_FROM is trying to cover blank statuses. Consider whether you want to treat YAML-null as equivalent to empty/missing here to avoid silent non-reconcile on that input.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Confirmed and fixed in 5de2aa1. You're right — a bare status: / status: null parses to YAML null, and verify.status_of stringifies that to "none" (via str(None)), which isn't in RECONCILABLE_FROM, so the most common blank form was silently skipped.
_reconcile_generic_terminal_status now reads the frontmatter once and normalizes a YAML-null/missing status to "" so it matches the allowlist member (kept status_of untouched — it's a shared gate chokepoint and "none" vs "" is immaterial to the other gates). A literal status: none string stays "none" and is still left alone.
Fixing this also surfaced a corruption bug in the round-1 fill code: a bare status: (no trailing space) filled to status:done — invalid YAML, the key drops on re-parse. reset_spec_status now guarantees key: value spacing. Added an engine test for the null-status reconcile and a devcontract test asserting the filled line re-parses cleanly. Thanks — good catch (medium was right; it had teeth).
| paths.planning_artifacts, | ||
| ): | ||
| try: | ||
| out.append(folder.relative_to(paths.project).as_posix()) |
There was a problem hiding this comment.
If any artifact folder is configured to equal paths.project, relative_to(...).as_posix() will yield ".", and passing that into the exclude list can effectively exclude the whole repo from the proof-of-work diff. Consider filtering out "."/empty prefixes in artifact_relpaths() so a misconfiguration can’t disable change detection.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Good defensive catch — fixed in 5de2aa1. If an artifact folder is configured to equal paths.project, relative_to(...).as_posix() yields ".", and :(exclude). would exclude the whole tree, disabling the proof-of-work gate.
artifact_relpaths now drops empty/"." results. I applied the same filter to the pre-existing Engine._protected_relpaths (it shares the identical pattern and the same latent gap) so the two stay in sync. Added verify tests covering the "." guard and the artifact-only-edit exclusion.
…nst "." Address PR #25 round-2 reviewer findings (Augment, both medium). YAML-null status never reconciled: a bare `status:` / `status: null` reads as the string "none" through verify.status_of (str(None)), which dodges the RECONCILABLE_FROM allowlist, so the most common "blank" template form was never reconciled. _reconcile_generic_terminal_status now normalizes a YAML-null/missing status to "" (a literal `status: none` string stays "none" and is still left alone). Routing null/bare into reset_spec_status surfaced a corruption bug: filling a bare `status:` (no trailing space) produced `status:done` — invalid YAML, the key is lost on re-parse. _repl now guarantees `key: value` spacing. Whole-tree exclude guard: artifact_relpaths (and, for parity, Engine._protected_relpaths) dropped a "." result that arises when an artifact folder is configured == the project/repo root, which as an exclude/keep prefix would disable change detection / the dirty check. Adds coverage: bare-null reconcile (engine), bare `status:` validity (devcontract), artifact_relpaths "." guard + artifact-only-edit exclusion (verify). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
augment review |
| pre = m.group("pre") | ||
| if not pre.endswith((" ", "\t")): | ||
| pre += " " | ||
| return f"{pre}{m.group('q')}{new_status}{m.group('q')}{m.group('rest')}" |
There was a problem hiding this comment.
When the original frontmatter line is comment-only (e.g. status: # intentionally blank), this rewrite will produce status: done# ..., which changes the inline comment into part of the scalar and can break/alter YAML parsing. Consider ensuring there’s always whitespace between the inserted value and rest when rest begins with #.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Confirmed and fixed in 34e6927. A blank value with a trailing inline comment (status: # tbd, which YAML parses as null, so the round-2 normalization routes it into the fill path) abutted the value to the comment — status: done# tbd — and since a # with no preceding space isn't a YAML comment, the value became the whole done# tbd scalar.
reset_spec_status._repl now re-inserts a separating space when rest begins with #. Verified across the cases:
status: # intentionally blank -> status: done # intentionally blank reparsed=done
status: # tbd -> status: done # tbd reparsed=done
status: 'x' # keep -> status: 'done' # keep reparsed=done
Added a devcontract test asserting the comment survives and the frontmatter re-parses to done. Thanks!
| spec_file = (result_json or {}).get("spec_file") | ||
| if not spec_file: | ||
| return | ||
| spec_path = verify.resolve_spec_path(str(spec_file), self.workspace.paths) |
There was a problem hiding this comment.
spec_path can be an absolute (or otherwise out-of-repo) path via resolve_spec_path(), and this reconciliation then writes to it via reset_spec_status(). Consider guarding that the resolved spec path is within the project/artifact roots before mutating, to avoid unintended edits if a session reports a surprising spec_file.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
There was a problem hiding this comment.
Good defensive catch — fixed in 34e6927. resolve_spec_path returns an absolute spec_file as-is with no containment check, and reconcile is the one path that mutates a file keyed off the session-supplied path.
Added verify.spec_within_roots(spec_path, paths) (mirrors the is_relative_to idiom already at verify.py:265) and gated the reconcile write on it: the spec must equal or sit under the project root or an artifact root (output_folder / implementation_artifacts / planning_artifacts — so a legitimately out-of-project artifact dir is still allowed). A spec outside those roots is left untouched and journaled as spec-reconcile-skipped-out-of-tree. The normal generic flow is unaffected — find_result_artifact globs implementation_artifacts, so spec_file is always an absolute path under that root. Added verify + engine tests for the skip path.
…f-tree spec Address PR #25 round-3 reviewer findings (Augment, both medium). Blank status + inline comment: filling a blank `status: # tbd` (parsed as YAML-null) abutted the value to the comment -> `status: done# tbd`, where the `#` is no longer a comment and the value becomes the whole `done# tbd` scalar. reset_spec_status._repl now re-inserts a separating space when `rest` begins with `#`, so the result stays valid YAML and the comment survives. Out-of-tree spec write: _reconcile_generic_terminal_status mutates a path taken from the session's result_json. resolve_spec_path returns an absolute path as-is with no containment check. New verify.spec_within_roots gates the reconcile write to the project/artifact roots (out-of-project artifact dirs still allowed); a surprising spec_file is left untouched and journaled as spec-reconcile-skipped-out-of-tree. Adds coverage: comment-preserving fill (devcontract), spec_within_roots (verify), out-of-tree reconcile skip (engine). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/test_verify.py (2)
749-757: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the out-of-project artifact-root case.
spec_within_roots()is supposed to allow specs under artifact dirs even when those dirs live outsideproject, but this test only covers roots nested insideproject. A future simplification to project-only containment would still pass here.Suggested test tweak
def test_spec_within_roots(project, tmp_path): """Specs under the project / artifact roots are writable; an out-of-tree absolute path is refused (guards the reconcile mutation).""" assert verify.spec_within_roots(project.implementation_artifacts / "spec-x.md", project) assert verify.spec_within_roots(project.project / "anywhere.md", project) assert verify.spec_within_roots(project.output_folder, project) # the root itself + import dataclasses + + external_impl = tmp_path / "external-artifacts" + external_impl.mkdir() + external_paths = dataclasses.replace(project, implementation_artifacts=external_impl) + assert verify.spec_within_roots(external_impl / "spec-x.md", external_paths) outside = tmp_path / "outside" / "spec.md"🤖 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_verify.py` around lines 749 - 757, The current test only covers roots nested inside project, so it misses the artifact-root scenario that should still be allowed. Update test_spec_within_roots in tests/test_verify.py to add a case using an artifact root path outside project and assert verify.spec_within_roots accepts a spec under that root; keep the existing checks for project-contained paths and out-of-tree absolute paths.
728-746: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise a root-level
_bmad-outputedit in this regression.This only proves exclusion for
implementation_artifacts. Ifartifact_relpaths()accidentally stops returningpaths.output_folder, this test still passes while bundle/ledger files directly under_bmad-outputstart tripping the no-changes gate.Suggested test tweak
def test_has_changes_since_excludes_artifact_only_edit(project): """A change confined to the artifact folders is not proof of dev work.""" baseline = verify.rev_parse_head(project.project) + (project.output_folder / "ledger.json").write_text("bookkeeping\n") (project.implementation_artifacts / "spec-x.md").write_text("bookkeeping\n") assert verify.has_changes_since(project.project, baseline) is True # unscoped🤖 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_verify.py` around lines 728 - 746, Extend the has_changes_since regression in test_has_changes_since_excludes_artifact_only_edit to also cover a direct root-level _bmad-output edit, not just implementation_artifacts. Use artifact_relpaths(project) and the existing verify.has_changes_since / verify.rev_parse_head flow, but add a write under project.paths.output_folder (or equivalent root output folder symbol) so the test proves artifact_relpaths() still excludes paths.output_folder and bundle/ledger files there do not count as changes.
🤖 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_verify.py`:
- Around line 749-757: The current test only covers roots nested inside project,
so it misses the artifact-root scenario that should still be allowed. Update
test_spec_within_roots in tests/test_verify.py to add a case using an artifact
root path outside project and assert verify.spec_within_roots accepts a spec
under that root; keep the existing checks for project-contained paths and
out-of-tree absolute paths.
- Around line 728-746: Extend the has_changes_since regression in
test_has_changes_since_excludes_artifact_only_edit to also cover a direct
root-level _bmad-output edit, not just implementation_artifacts. Use
artifact_relpaths(project) and the existing verify.has_changes_since /
verify.rev_parse_head flow, but add a write under project.paths.output_folder
(or equivalent root output folder symbol) so the test proves artifact_relpaths()
still excludes paths.output_folder and bundle/ledger files there do not count as
changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bff08987-b190-42a4-817c-9876712c93b3
📒 Files selected for processing (6)
src/automator/devcontract.pysrc/automator/engine.pysrc/automator/verify.pytests/test_devcontract.pytests/test_engine.pytests/test_verify.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/automator/devcontract.py
- src/automator/verify.py
- src/automator/engine.py
|
augment review |
…utput exclude Harden two regressions per CodeRabbit nitpicks on PR #25: - test_spec_within_roots: assert an artifact root configured OUTSIDE project is still accepted, guarding against a project-only containment regression. - test_has_changes_since_excludes_artifact_only_edit: also write directly under _bmad-output, proving artifact_relpaths covers output_folder (not just the impl subdir) so bundle/ledger edits never trip the no-change gate. Promote dataclasses to a module-level import. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai both nitpicks addressed in d0e4d57 (test-only).
Full suite green (1204 passed), trunk clean. Thanks for the catches. |
|
Tip For best results, initiate chat on the files or code changes.
The added coverage around out-of-project artifact roots in 🐇✨ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
1 participant
Summary
Fixes completed
bmad-dev-autowork being falsely deferred when the skill leaves the specfrontmatter
statusbehind its prose.The decoupled
bmad-dev-autoskill can finalize a run in its## Auto Run Resultprose(
Status: done) yet leave the YAML frontmatter at the template defaultdraft. Every orchestratorgate reads frontmatter, so
_post_dev_state_syncno-op'd (sprint/ledger never advanced) and thestory or sweep bundle deferred — and with
rollback_on_failureon, the tested work was reverted andlost. (Observed in the wild: a fully-implemented, 1442-test-green deferred-work bundle deferred with
spec status is 'draft', expected 'done'.)Fix (Approach: reconcile, not escalate)
engine._reconcile_generic_terminal_statusruns in the sharedEngine._dev_phaseimmediatelybefore
_post_dev_state_sync, so it covers both the story (sprint) and sweep-bundle (ledger)paths. When the terminal
## Auto Run ResultStatus isdoneand the frontmatter is at anon-terminal status (
devcontract.RECONCILABLE_FROM:""/draft/ready-for-dev/in-progress),it advances the frontmatter to the success status via
devcontract.reset_spec_statusand journalsspec-status-reconciled.This does not violate the "never trust prose for a gate" doctrine. Reconcile repairs a
bookkeeping field only; every deterministic gate still runs afterward against real on-disk/git state:
has_changes_since(baseline)— real worktree diffSo a spec that lied (no diff / wrong dw-ids / broken build) still fails its gate.
blockedis neverreconciled (guard: prose must be
done); an already-terminal or unknown frontmatter status is neveroverridden (idempotent, never-regress).
The companion upstream skill fix is bmad-code-org/BMAD-METHOD#2536 (make the Finalize step write
frontmatter
statusexplicitly); this orchestrator change is the durable backstop regardless ofskill version.
Tests
test_devcontract.py—RECONCILABLE_FROMguard + reset-from-each-reconcilable-valuetest_engine.py— advance / idempotent-when-already-done / skip-blocked-prose / does-not-bypass-the-no-change-gate (story path)test_sweep.py— the exact DW-bundle regression: stale-frontmatter bundle now closes the ledgerFull suite 1194 green,
trunk checkclean. Version bumped0.7.7 → 0.7.8(sync_version), CHANGELOG updated.🤖 Generated with Claude Code
Summary by CodeRabbit