fix(deferredwork): keep flat appends visible after canonical entries (#304) - #322
Conversation
(cherry picked from commit fbf7541)
The flat-block boundary has to recognize the shape exactly as parse_legacy does, or the bug survives in the shapes that parser accepts: keyed on the full three-line block it missed every partial one — including the no-summary and no-evidence shapes two existing tests already pin — plus `*` bullets and tab/padded markers. FLAT_ENTRY_RE and _FLAT_SOURCE_RE now consume one shared opening-line pattern so they cannot drift apart. The boundary is also searched from the entry's own `status:` line rather than from the heading. Truncating over the status leaves the entry reading as neither open nor done — open_ids() drops it, classify() reports it malformed — which trades one lost flat block for one lost tracked entry, the same failure class in the other direction. An entry with no status line has nothing to protect. Ablations: removing the boundary fails 17 tests; removing only the status anchor fails exactly the two guard tests.
|
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 (3)
WalkthroughDeferred-work parsing now excludes flat appended blocks from canonical entry spans, allowing legacy parsing to retain them. Canonical entry writing adds a ChangesDeferred-work ledger updates
Estimated code review effort: 3 (Moderate) | ~20 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 |
Greptile SummaryThis PR fixes a bug (#304) where a flat appender block landing after the last canonical
Confidence Score: 5/5The change is parser-only, touches no subprocess/git/mux/path construction, and the added location: field is backward-compatible (keyword-only default, ignored by all existing parsers). The fix correctly narrows parse_ledger()'s span computation, the status-anchor guard prevents the one new failure mode the simpler fix introduced, the shared _FLAT_SOURCE_BODY constant eliminates drift risk between the two recognizer regexes, and the 12 new tests discriminate this fix from the three-line boundary approach and from main. Files Needing Attention: No files require special attention. deferredwork.py is the only logic-bearing file and its changes are localized to parse_ledger() and append_entry().
|
| Filename | Overview |
|---|---|
| src/bmad_loop/deferredwork.py | Adds _FLAT_SOURCE_BODY / FLAT_ENTRY_RE to bound canonical spans at flat appender blocks; anchors flat search after the entry's status: line; adds location: field to append_entry output. |
| tests/test_deferredwork.py | Adds 12 new tests covering all flat-block shapes parse_legacy accepts, the status-anchor guard, CRLF handling, sandwiched blocks, two consecutive flat blocks, and the new location: field round-trip. |
| CHANGELOG.md | Adds changelog entry for #304 fix and the location: field addition; prose is accurate and cross-references #274. |
Reviews (1): Last reviewed commit: "fix(deferredwork): anchor the flat bound..." | Re-trigger Greptile
Closes #304. Continues #274 by @Haven2026, who found the bug and picked the right seam — their
commit is cherry-picked here with authorship intact, and #274 is closed in favour of this.
The bug
A flat appender block — the
- source_spec:/summary:/evidence:shape the innerbmad-dev-auto review step appends — landing after the last canonical
### DW-<n>entry is absorbedinto that entry's
parse_ledger()span.parse_legacy()masks canonical spans before scanning, sothe block is invisible to it, and therefore to the sweep's migration trigger (
sweep.py:437), itsleftovers read (
sweep.py:261/708),bmad-loop sweep --dry-run(cli.py:1073) and the TUIlegacy view (
tui/data.py:917). The session did its job and recorded the defer; nothing downstreamcould ever see it.
parse_ledger()is the right place to fix it: it is the single source of the spansparse_legacy'smasking consumes.
What changed relative to #274
Two corrections, both measured. My review on #274 endorsed the strict three-line
FLAT_ENTRY_REthere; that was wrong, and this is the retraction.
1. The boundary must recognize the flat shape exactly as
parse_legacydoes. Keyed on the fullthree-line block, it fixes 2 of 9 shapes the legacy parser accepts. Two of the misses are shapes
mainalready pins with named tests (test_flat_appender_missing_summary_falls_back,test_flat_appender_in_done_section_is_done).FLAT_ENTRY_REand_FLAT_SOURCE_REnow consume oneshared
_FLAT_SOURCE_BODYso they cannot drift.mainsource_spec+summary+evidencesource_spec+evidence(no summary)source_spec+summary(no evidence)summary/evidencein the other ordersummary*bullet marker (_BULLET_REaccepts it)-\t,-␣␣␣)SOURCE_SPEC:uppercased2. The boundary is searched from the entry's own
status:line, never above it. A flat-shapedbullet quoted inside an entry ahead of its status truncated the span over the status, leaving the
entry reading as neither open nor done —
open_ids()drops it,classify()reports it malformed.That is a new work-loss mode
maindoes not have: one lost flat block traded for one lost trackedentry, and in the two-block case the entry's own prose re-emitted as a phantom legacy finding. The
three-line regex did not prevent this; it only dodged the one fixture that happened to omit
summary:. An entry with no status line has nothing to protect, so its whole span stays fair game.Also kept from #274:
append_entrynow writes thelocation:field (defaultn/a, which is whatboth engine refile sites take), closing a real gap against the canonical shape in
deferred-work-format.md. Called out explicitly rather than left as a silent rider.Not carried over: #274's commits 2–4 added
ValueErrorraises inside the ledger mutators. Thatis a different problem with a different shape and it is tracked on #305 — raising there turns
bad LLM output into a mid-sweep crash (
sweep.py:911feeds triageevidenceintomark_done'snote,
decisions.py:150feedsresolution/intent, and_close_resolvedis called bare fromsweep.py:541), against a documented contract that a close is "never allowed to fail the story orcrash the run". #305 has the layered design and is still open for the taking.
Verification
uv run pytest -q -n auto— 3201 passed, 24 skipped.uvx pyright@1.1.411— 0 errors.trunk check --no-fix— clean.two guard tests fail. Substitute fix(deferredwork): keep flat appends visible after canonical entries #274's three-line boundary → 9 fail (the 7 shape misses plus both
guard tests), so the suite discriminates this fix from that one.
tests/test_deferredwork.pyparse identically to
main, both views — nothing about legacy or mixed ledgers moves.bmad-loop sweep --dry-runon a ledger whose tail is a canonical entry followedby a partial flat block (no
evidence:line — the shape the three-line boundary misses):mainprints
1 openand nothing else; this branch prints1 openplus1 legacy (pre-DW-format) entries, 1 open — a sweep would first migrate them.Linux/macOS/Windows-specific moves. CRLF ledgers are covered by a test.
Summary by CodeRabbit
Bug Fixes
New Features
locationfield, defaulting ton/a, while preserving supplied locations.