fix(frontmatter): read a YAML-null status as blank, not "none" - #368
Conversation
A bare `status:` line parses as null and status_of stringified it, so every status gate saw the token "none" — which devcontract.RECONCILABLE_FROM then treated as a deliberate custom status and refused to reconcile, contradicting its own comment and _FM_STATUS_RE's writer side. Normalize None to "" at status_of (a literal `status: none` still reads "none") and retire the engine-local workaround at the reconcile. Closes #358
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
WalkthroughYAML-null or bare ChangesBlank status behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
Greptile SummaryThis PR fixes a bug where a bare
Confidence Score: 5/5Safe to merge — the fix is a minimal one-liner at the single normalization choke point, backed by targeted new tests that explicitly cover the previously untested YAML-null case and preserve the deliberate-custom-token boundary. The change is narrow: one expression in status_of, a local workaround removal in the engine, a display fallback in operatoractions, and a comment update. Every changed branch is exercised by new or restructured tests, the old parametrize ternary that silently collapsed the YAML-null case to a missing-key test is corrected, and the ablation section of the PR description confirms both the allowlist guard and the status_of guard are individually load-bearing. No call site semantics change for any status other than YAML-null. Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| src/bmad_loop/frontmatter.py | Core fix: adds a None guard before str() in status_of so YAML-null reads "" instead of "none"; minimal, correct, and well-documented. |
| src/bmad_loop/engine.py | Retires local None-normalization workaround in _reconcile_generic_terminal_status; now delegates to verify.status_of, which is the correct single-point fix. |
| src/bmad_loop/devcontract.py | Comment-only update to RECONCILABLE_FROM documentation reflecting that status_of now handles YAML-null normalization. |
| src/bmad_loop/operatoractions.py | Drift message now renders "(blank)" instead of an empty tail when spec_status is ""; correctly uses the truthiness of "" to trigger the fallback. |
| tests/test_verify.py | Test restructured to pass dicts directly, explicitly adding {"status": None} as a distinct row from {} (missing key) and {"status": "none"} (literal token). |
| tests/test_engine.py | Adds test_generic_reconcile_leaves_unknown_custom_status asserting byte-identical spec survival for a deliberate custom token; docstring fix on the null-status test. |
| tests/test_operatoractions.py | New test verifies that a blank YAML-null status renders the drift message as "its spec now says status: (blank)" and marks the story as non-confirmable. |
| CHANGELOG.md | Accurate changelog entry describing the bug, root cause, fix, and rendering side-effects. |
Reviews (1): Last reviewed commit: "fix(frontmatter): read a YAML-null statu..." | Re-trigger Greptile
Closes #358.
devcontract.RECONCILABLE_FROM's comment claims""covers "a blank or missing frontmatterstatus:". A missing one did. A blank one did not: YAML parses a barestatus:as null,status_ofdidstr(fm.get("status", "")), andstr(None)is"none"— so the blank-status shape landed in the "unknown custom token" arm and was left untouched, whiledevcontract._FM_STATUS_RE's own comment says the writer side deliberately fills exactly that shape ("a bmad-dev-auto template can leave it blank").The fix
Guard
Nonebeforestr()infrontmatter.status_of, the single point all spec-frontmatter status gates read through. A YAML-null status now reads"", the same as a missing key.The other candidate from the issue — adding
"none"toRECONCILABLE_FROM— is not taken. It would be dead code (the sole consumer already hand-normalizedNonefirst), it leaves a stringifiedNoneas a load-bearing token, and it breaks the byte-exact set pin intests/test_devcontract.py.The deliberate-token semantic is preserved exactly. PyYAML resolves only
~/null/Null/NULL/ empty as null, so a literalstatus: noneis the string"none"and still reads back as"none". The guard only touches values that were never written as a token at all.Blast radius
status_ofis deliberately wide by design, so I measured it. Of its 12 call sites, zero flip a branch on"none"→""— no gate, allowlist, or comparison changes verdict. Only message text moves, in two places, both handled here:operatoractions.committed_driftrenderedf"its spec now says status: {self.spec_status}", which would now trail off afterstatus:on a blank spec. It renders(blank). (Not!r— six assertions acrosstest_operatoractions.pyandtest_cli.pypin the exact stringits spec now says status: done.)stories.state_labellabels a bare-status storypresent— its documentedKIND_PRESENTfallback — instead ofnone, which was never a documented value; the fallback was being dodged because"none"is truthy. Textstatusboard anddry-runonly;--jsoncarries no story labels. Classification iswedgedeither way, so the engine's behavior is unchanged.engine._reconcile_generic_terminal_statusalready hand-normalizedNone→""locally right before itsRECONCILABLE_FROMcheck, with a comment naming this exact defect. That workaround is retired — it now reads throughstatus_oflike everything else.Tests
test_status_of_normalizeshad a misleading(None, "")row: the parametrize ternary turned it into{}, so it characterized the missing key and{"status": None}was never covered. Restructured to pass dicts directly, with rows for{},{"status": None}(the fix), and{"status": "none"}(the deliberate token).test_generic_reconcile_leaves_unknown_custom_status: a spec atstatus: needs-triagewith a proseStatus: doneis left byte-identical and journals no reconcile.test_generic_reconcile_advances_bare_null_frontmatter_status— it described the retired engine-local workaround. The test stays green.committed_driftrow pinning(blank).Ablations (per AGENTS.md — a negative assertion passes for every reason a value could be absent):
fm_status not in devcontract.RECONCILABLE_FROMguard → the new unknown-token test FAILS,needs-triagerewritten todone.reset_spec_status's line regex matchesneeds-triagehappily, so the allowlist is the only thing standing between a deliberate token and the repair write.status_ofguard (keeping the engine workaround removed) → the{"status": None}row,test_generic_reconcile_advances_bare_null_frontmatter_status, and the new(blank)drift test all FAIL.Both restored by re-editing, never
git checkout <file>.Full suite green (3679 passed, 24 skipped),
trunk check --no-fixclean, pyright 1.1.411 clean.Summary by CodeRabbit
status:values are now treated as blank rather than"none".status: (blank).presentinstead ofnone.