diff --git a/CHANGELOG.md b/CHANGELOG.md index 7fad34c..daa03de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -125,6 +125,25 @@ story `, the same annotation a sweep bundle writes. Both sprint and stories ### Fixed +- **A deferred finding appended after the last ledger entry is no longer lost (#304).** Found and + first fixed by [@Haven2026](https://github.com/Haven2026) in #274. The inner dev session appends + each review defer as a flat `- source_spec:` / `summary:` / `evidence:` block; landing after the + last canonical `### DW-` entry, it was absorbed into that entry's span, and `parse_legacy()` + masks canonical spans before scanning — so the block was invisible to the sweep's migration + trigger and leftovers read, to `bmad-loop sweep --dry-run`, and to the TUI's legacy view. The session + did its job and recorded the defer; nothing downstream could ever see it. Canonical spans now end + at a flat block. + + The boundary recognizes the flat shape exactly as the legacy parser does — the opening line + alone, either bullet marker, whatever `summary:`/`evidence:` lines follow — because a stricter + boundary silently leaves the bug in place for the partial blocks that parser accepts. It is + searched from the entry's own `status:` line and never above it: truncating over the status would + leave the entry reading as neither open nor done, trading a lost flat block for a lost tracked + entry. + + `append_entry` also now writes the `location:` field (defaulting to `n/a`, as the orchestrator's + own refiles take it), which the canonical format documents and the writer had been omitting. + - **`return_attached_client` no longer claims a return that never happened (#227).** It discarded `switch_client`'s result and answered `True` unconditionally, so a failed switch plus a failed `-l` fallback still journaled the return and sent the sweep unattended while the human sat in diff --git a/src/bmad_loop/deferredwork.py b/src/bmad_loop/deferredwork.py index 9c104fa..15c9abc 100644 --- a/src/bmad_loop/deferredwork.py +++ b/src/bmad_loop/deferredwork.py @@ -21,6 +21,16 @@ HEADING_RE = re.compile(r"^### (DW-\d+): (.+?)\s*$", re.MULTILINE) ANY_HEADING_RE = re.compile(r"^#{1,6} ", re.MULTILINE) +# The flat appender's opening line, in the two forms this module needs it: as a +# bullet in the raw ledger (FLAT_ENTRY_RE, the canonical-span boundary in +# parse_ledger) and as bullet *content* after `_BULLET_RE` has stripped the +# marker (`_FLAT_SOURCE_RE`, legacy section). One shape, two anchors — they have +# to agree, or a block the legacy parser recognizes stays invisible to it (#304). +# Keyed on the opening line alone, deliberately: also requiring the block's +# `summary:`/`evidence:` lines would narrow the boundary below the parser's own +# recognition, leaving the bug in place for every partial shape it accepts. +_FLAT_SOURCE_BODY = r"source_spec:[ \t]" +FLAT_ENTRY_RE = re.compile(rf"^[-*][ \t]+{_FLAT_SOURCE_BODY}", re.IGNORECASE | re.MULTILINE) STATUS_RE = re.compile(r"^status:[ \t]*(.*)$", re.MULTILINE) @@ -49,6 +59,18 @@ def parse_ledger(text: str) -> list[DWEntry]: other = ANY_HEADING_RE.search(text, m.end(), end) if other: end = other.start() + # ...and at a flat appender block, which belongs to no canonical entry + # (#304). This span is what parse_legacy() masks out before scanning, so + # absorbing the block hides the finding from every reader of the ledger. + # Searched from the entry's own `status:` line, never from above it: + # truncating over the status leaves the entry reading as neither open nor + # done (open_ids() drops it, classify() calls it malformed), which trades + # one lost flat block for one lost tracked entry. An entry with no status + # line has nothing to protect, so the whole span is fair game. + status_m = STATUS_RE.search(text, m.end(), end) + flat = FLAT_ENTRY_RE.search(text, status_m.end() if status_m else m.end(), end) + if flat: + end = flat.start() body = text[m.start() : end] status_m = STATUS_RE.search(body) entries.append( @@ -276,6 +298,7 @@ def append_entry( origin: str, source_spec: str, reason: str, + location: str = "n/a", status: str = "open", severity: str | None = None, ) -> str | None: @@ -295,7 +318,12 @@ def append_entry( ): return None dw_id = f"DW-{next_seq(text)}" - lines = [f"### {dw_id}: {title}", f"origin: {origin}", f"source_spec: `{source_spec}`"] + lines = [ + f"### {dw_id}: {title}", + f"origin: {origin}", + f"location: {location}", + f"source_spec: `{source_spec}`", + ] if severity: lines.append(f"severity: {severity}") lines.append(f"reason: {reason}") @@ -386,8 +414,10 @@ class LegacyEntry: # summary: # evidence: # We recognize it so the `summary` becomes the title (not the source_spec path) -# and the entry migrates cleanly into the canonical `### DW-` shape. -_FLAT_SOURCE_RE = re.compile(r"^source_spec:[ \t]", re.IGNORECASE) +# and the entry migrates cleanly into the canonical `### DW-` shape. The +# opening line comes from the same `_FLAT_SOURCE_BODY` as FLAT_ENTRY_RE, which +# bounds canonical spans on it — see that constant for why they must not drift. +_FLAT_SOURCE_RE = re.compile(rf"^{_FLAT_SOURCE_BODY}", re.IGNORECASE) _FLAT_SUMMARY_RE = re.compile(r"^[ \t]*summary:[ \t]*(.*)$", re.IGNORECASE | re.MULTILINE) _BULLET_RE = re.compile(r"^[-*][ \t]+(.*)$") _ITEM_ID_RE = re.compile( diff --git a/tests/test_deferredwork.py b/tests/test_deferredwork.py index 0d419d5..c61cefa 100644 --- a/tests/test_deferredwork.py +++ b/tests/test_deferredwork.py @@ -304,6 +304,178 @@ def test_mixed_ledger_keeps_both_views_separate(): assert all("DW-" not in e.body for e in entries) +def test_flat_append_after_canonical_stays_visible_to_legacy_parser(): + text = ( + "# Deferred Work\n\n" + "### DW-1: canonical\n\n" + "origin: test\nlocation: n/a\nreason: test\nstatus: open\n\n" + "- source_spec: `spec-next.md`\n" + " summary: later flat finding\n" + " evidence: must not be swallowed by DW-1\n" + ) + + (canonical,) = parse_ledger(text) + (legacy,) = parse_legacy(text) + + assert "later flat finding" not in canonical.body + assert legacy.title == "later flat finding" + + +# The canonical-span boundary has to recognize every flat shape parse_legacy +# does (#304): a boundary keyed on the full three-line block leaves the bug in +# place for the partial ones — and those are not hypothetical, they are the +# shapes test_flat_appender_missing_summary_falls_back and +# test_flat_appender_in_done_section_is_done already pin. Each case asserts both +# halves of the fix: the block reaches parse_legacy AND DW-1 keeps its status. +FLAT_TAIL_SHAPES = { + "full-block": "- source_spec: `s.md`\n summary: finding\n evidence: e\n", + "no-summary": "- source_spec: `s.md`\n evidence: orphaned note\n", + "no-evidence": "- source_spec: `s.md`\n summary: finding\n", + "swapped-order": "- source_spec: `s.md`\n evidence: e\n summary: finding\n", + "field-interleaved": "- source_spec: `s.md`\n severity: high\n summary: finding\n", + "star-bullet": "* source_spec: `s.md`\n summary: finding\n evidence: e\n", + "tab-after-marker": "-\tsource_spec: `s.md`\n summary: finding\n evidence: e\n", + "padded-marker": "- source_spec: `s.md`\n summary: finding\n evidence: e\n", + "uppercase-field": "- SOURCE_SPEC: `s.md`\n summary: finding\n evidence: e\n", +} + + +@pytest.mark.parametrize("shape", sorted(FLAT_TAIL_SHAPES), ids=sorted(FLAT_TAIL_SHAPES)) +def test_flat_append_visible_for_every_shape_parse_legacy_accepts(shape): + text = ( + "# Deferred Work\n\n### DW-1: canonical\n\n" + "origin: test\nlocation: n/a\nreason: test\nstatus: open\n\n" + FLAT_TAIL_SHAPES[shape] + ) + + (canonical,) = parse_ledger(text) + (legacy,) = parse_legacy(text) + + assert canonical.open, "the boundary must not cost DW-1 its status" + assert "source_spec" not in canonical.body + assert legacy.body.lstrip().startswith(("- ", "* ", "-\t")) + + +def test_flat_boundary_never_truncates_above_the_entry_status_line(): + """A flat-shaped bullet *inside* an entry, ahead of its `status:`, must not + move the span end above that status: an entry parsing as neither open nor + done is a lost tracked entry, the same failure class in the other direction + (open_ids() drops it, classify() reports it malformed).""" + text = ( + "### DW-1: canonical\n\norigin: test\nlocation: n/a\n" + "reason: the appender writes this shape, quoted here as evidence\n" + "- source_spec: `quoted-example.md`\n" + " summary: quoted, not a real finding\n" + " evidence: prose inside DW-1, not an appended block\n" + "status: open\n" + ) + + (canonical,) = parse_ledger(text) + + assert canonical.status == "open" and canonical.open + assert "quoted-example.md" in canonical.body + assert open_ids(text) == {"DW-1"} + assert parse_legacy(text) == [] + + +def test_flat_boundary_still_applies_after_a_quoted_block_inside_the_entry(): + """The in-entry bullet does not grant immunity to what follows it: a real + appended block after the entry's `status:` is still bounded out.""" + text = ( + "### DW-1: canonical\n\norigin: test\nreason: prose\n" + "- source_spec: `quoted-example.md`\n summary: quoted\n evidence: e\n" + "status: open\n\n" + "- source_spec: `real.md`\n summary: real finding\n evidence: e\n" + ) + + (canonical,) = parse_ledger(text) + (legacy,) = parse_legacy(text) + + assert canonical.open + assert "quoted-example.md" in canonical.body and "real finding" not in canonical.body + assert legacy.title == "real finding" + + +def test_flat_boundary_exposes_the_block_when_the_entry_has_no_status_line(): + """No status line means nothing to protect — the entry is already malformed, + so bounding the block out of it costs nothing and rescues the finding.""" + text = "# Deferred Work\n\n### DW-1: malformed\n\norigin: test\nreason: test\n\n" + ( + "- source_spec: `s.md`\n summary: rescued finding\n evidence: e\n" + ) + + (canonical,) = parse_ledger(text) + (legacy,) = parse_legacy(text) + + assert canonical.status == "" and not canonical.open + assert legacy.title == "rescued finding" + + +def test_flat_append_boundary_accepts_crlf(): + text = "\r\n".join( + [ + "# Deferred Work", + "", + "### DW-1: canonical", + "", + "origin: test", + "location: n/a", + "reason: test", + "status: open", + "", + "- source_spec: `spec-next.md`", + " summary: later flat finding", + " evidence: must not be swallowed by DW-1", + "", + ] + ) + + (canonical,) = parse_ledger(text) + (legacy,) = parse_legacy(text) + + assert canonical.open + assert "later flat finding" not in canonical.body + assert legacy.title == "later flat finding" + + +@pytest.mark.parametrize("gap", ["\n", ""], ids=["blank-line", "no-blank-line"]) +def test_flat_append_between_two_canonical_entries(gap): + """The realistic sequence: the inner session defers (flat) and the + orchestrator then refiles a canonical entry at EOF, sandwiching the block.""" + text = ( + "# Deferred Work\n\n### DW-1: a\n\norigin: t\nreason: t\nstatus: open\n" + + gap + + "- source_spec: `s.md`\n summary: sandwiched finding\n evidence: e\n" + + "\n### DW-2: b\n\norigin: t\nreason: t\nstatus: open\n" + ) + + assert open_ids(text) == {"DW-1", "DW-2"} + (legacy,) = parse_legacy(text) + assert legacy.title == "sandwiched finding" + + +def test_two_flat_blocks_after_the_last_entry_are_both_visible(): + block = "- source_spec: `s.md`\n summary: %s\n evidence: e\n" + text = ( + "# Deferred Work\n\n### DW-1: a\n\norigin: t\nreason: t\nstatus: open\n\n" + + (block % "first") + + (block % "second") + ) + + assert open_ids(text) == {"DW-1"} + assert [e.title for e in parse_legacy(text)] == ["first", "second"] + + +def test_flat_append_followed_by_a_legacy_section_keeps_both(): + text = ( + "# Deferred Work\n\n### DW-1: a\n\norigin: t\nreason: t\nstatus: open\n\n" + "- source_spec: `s.md`\n summary: flat finding\n evidence: e\n" + "\n## Deferred from: an older review (2026-06-01)\n\n" + "- W1 — a freeform legacy item\n" + ) + + assert open_ids(text) == {"DW-1"} + assert [e.title for e in parse_legacy(text)] == ["flat finding", "a freeform legacy item"] + + def test_legacy_item_does_not_swallow_masked_canonical_neighbor(): text = ( "## Deferred from: somewhere (2026-06-01)\n\n" @@ -419,11 +591,31 @@ def test_append_entry_numbers_and_writes(tmp_path): assert "DW-5" in entries and entries["DW-5"].open body = entries["DW-5"].body assert "origin: review-budget-followup" in body + assert "location: n/a" in body assert "source_spec: `spec-foo.md`" in body assert "severity: low" in body assert "follow-up still recommended for dw-x" in body +def test_append_entry_preserves_a_supplied_location(tmp_path): + """`n/a` is only the default the orchestrator's own refiles take — the field + is documented as `` and must round-trip verbatim.""" + p = tmp_path / "deferred-work.md" + assert ( + append_entry( + p, + title="follow-up", + origin="code-review", + source_spec="spec-foo.md", + reason="still open", + location="src/foo.py:12", + ) + == "DW-1" + ) + (entry,) = parse_ledger(p.read_text()) + assert "location: src/foo.py:12" in entry.body + + def test_append_entry_idempotent_for_open_origin_and_spec(tmp_path): p = tmp_path / "deferred-work.md" p.write_text("# Deferred Work\n")