fix(deferredwork): keep flat appends visible after canonical entries#274
fix(deferredwork): keep flat appends visible after canonical entries#274Haven2026 wants to merge 4 commits into
Conversation
|
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe deferred work parser now stops canonical entry bodies before flat appender blocks. Newly appended entries include an optional ChangesDeferred work ledger handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bmad_loop/deferredwork.py (1)
151-175: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject multiline
locationvalues before serialization.
locationis written verbatim into a line-oriented ledger. A value such assrc/foo.py\nstatus: closedcan inject fields or headings, changingparse_ledger()status, spans, and idempotency behavior. Reject\r/\nvalues and add a regression test for the invalid case.Suggested guard
def append_entry( path: Path, *, title: str, origin: str, source_spec: str, reason: str, location: str = "n/a", status: str = "open", severity: str | None = None, ) -> str | None: + if "\r" in location or "\n" in location: + raise ValueError("location must be a single line")🤖 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 `@src/bmad_loop/deferredwork.py` around lines 151 - 175, Validate the location argument in the deferred-work entry creation function before constructing or writing the lines, rejecting any value containing carriage-return or newline characters. Preserve normal single-line locations and add a regression test confirming multiline location values are rejected without serializing an entry.
🧹 Nitpick comments (1)
tests/test_deferredwork.py (1)
434-434: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover caller-supplied locations.
This assertion only verifies the
"n/a"default. Add a case withlocation="src/foo.py:12"to ensure supplied values are emitted unchanged.🤖 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_deferredwork.py` at line 434, Extend the test around the existing "location: n/a" assertion to cover a caller-supplied location, such as "src/foo.py:12", and verify that the generated body emits that value unchanged. Reuse the same deferred-work construction and body-generation path exercised by the existing assertion.
🤖 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.
Outside diff comments:
In `@src/bmad_loop/deferredwork.py`:
- Around line 151-175: Validate the location argument in the deferred-work entry
creation function before constructing or writing the lines, rejecting any value
containing carriage-return or newline characters. Preserve normal single-line
locations and add a regression test confirming multiline location values are
rejected without serializing an entry.
---
Nitpick comments:
In `@tests/test_deferredwork.py`:
- Line 434: Extend the test around the existing "location: n/a" assertion to
cover a caller-supplied location, such as "src/foo.py:12", and verify that the
generated body emits that value unchanged. Reuse the same deferred-work
construction and body-generation path exercised by the existing assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 58067a47-7ec1-4027-8baa-d7c1d096a730
📒 Files selected for processing (2)
src/bmad_loop/deferredwork.pytests/test_deferredwork.py
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/deferredwork.py`:
- Around line 21-24: Update FLAT_ENTRY_RE to accept both LF and CRLF line
boundaries by using \r?\n for each required line break, while preserving the
existing end-of-input handling. Add a parse_legacy() regression covering a CRLF
flat-appender block in a mixed ledger and verify the flat finding is returned
rather than masked by the canonical span.
🪄 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 Plus
Run ID: e198e4bb-c9ae-4b51-a4cf-c21c570295d7
📒 Files selected for processing (2)
src/bmad_loop/deferredwork.pytests/test_deferredwork.py
pbean
left a comment
pbean
left a comment
There was a problem hiding this comment.
Thanks for this — and welcome! You found a real one. I reproduced it on current main: a flat
appender block after the last canonical ### DW-<n> entry is absorbed into that entry's
parse_ledger() span, parse_legacy() masks it out, and the finding becomes invisible to the
sweep — silently lost work. I've filed #304 to track it, and your parse_ledger boundary
truncation is exactly the right seam for the fix (it's the single source of spans that
parse_legacy's masking consumes). The strict three-line FLAT_ENTRY_RE matching the shape
step-04-review.md instructs is a good call too — a looser boundary could truncate mid-entry and
orphan a status: line, which fails in the worse direction.
Two asks before this can land:
1. Rebase onto current main. deferredwork.py was rewritten the day after you opened this
(PR #300): mark_done is now a thin wrapper over a new mark_done_many (single atomic write via
atomic_write_text), and new parse_declaration/classify helpers sit in the middle of the
file. The PR is currently conflicting; the flat-boundary hunk and the tests apply nearly cleanly,
the validation hunks don't.
2. Trim this PR to the flat-boundary fix (+ the location: writer change). Please keep:
FLAT_ENTRY_RE+ theparse_ledgerboundary truncation, with your three boundary tests
(including the CRLF one — nice catch);- the
append_entrylocation:field with then/adefault and its tests — that closes a real
gap against the documented canonical shape indeferred-work-format.md.
And please drop the input-validation commits (9b2efe2f, 582e5662, 9c6d5c86) from this PR.
The injection vector they target is real, but raise-style validation in the mutators conflicts
with how the orchestrator has to fail on those paths: sweep.py passes LLM-authored triage
evidence into mark_done's note and decisions.py passes triage resolution/intent as
decision details, and validate_triage only strips — so a multiline value from a session would
now crash a live sweep mid-close, where the ledger-close doctrine is "never a gate, never crash
the run". There's also a placement trap post-rebase: the engine's story-close path calls
mark_done_many directly, so guards on mark_done alone would be inert exactly where they matter
most. I've split this out as #305 with the design that fits (reject at the validate_triage
schema boundary where errors become session feedback, sanitize line breaks at the writer
chokepoints) — it credits your commits as prior art, and it's yours if you'd like to take it.
Optional polish while you're in there: the boundary's first line accepts only - while the
legacy recognizer accepts [-*][ \t]+ bullets (_BULLET_RE/_FLAT_SOURCE_RE) — widening it
keeps the two in agreement about what counts as a flat block.
If you add Fixes #304 to the PR description it'll close the tracker on merge. CI on fork PRs
needs a maintainer approval per run — ping here (or just push) once rebased and I'll approve it.
2 participants
Summary
- source_spec:format emitted by inner BMAD sessionsparse_legacy()even after a canonical appendappend_entry()emit the canonicallocation:field with a safen/adefaultVerification
flat appendinvisible; writer missinglocation)uv run pytest tests/test_deferredwork.py -q— 33 passedGIT_CONFIG_COUNT=1 GIT_CONFIG_KEY_0=core.fsmonitor GIT_CONFIG_VALUE_0=false uv run pytest tests/test_deferredwork.py tests/test_sweep.py -q— 104 passeduv run ruff check src/bmad_loop/deferredwork.py tests/test_deferredwork.py— passedbmad-loop sweep --dry-runfixture reports 1 canonical open plus 1 visible legacy entryThe process-level Git config only disables a local macOS fsmonitor socket that otherwise cannot be copied by the test fixture; it does not affect parser behavior.
Summary by CodeRabbit
### DW-*entry bodies.locationfield (default: “n/a”), andappend_entrywrites it when provided.append_entry,mark_done, andappend_decision(rejects invalid status/severity and malformed/invalid dates and multiline inputs).