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
1 participant
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).