fix(deferredwork): sanitize free text at the ledger writer chokepoint (#305) - #331
Conversation
The deferred-work ledger is line-oriented, but every mutator interpolated its string arguments verbatim, so a value carrying a line break injected ledger lines: a resolution note of `fixed\n### DW-99: fake\nstatus: open` minted a phantom entry, and a break followed by `- source_spec:` truncated the entry span and re-surfaced its tail as a legacy item. Collapse line breaks in free text at the point each writer serializes it, and never raise there — the close paths are called bare, so a ValueError would end the sweep run as crashed. The break set is `str.splitlines()`'s, not `\n` alone: `parse_legacy` scans with it, so a U+2028 splits an entry for that reader while `parse_ledger`'s `re.MULTILINE` never sees it. The guards sit in `_apply_done`, not the `mark_done` wrapper, because the story-close path calls `mark_done_many` directly; and ahead of `append_entry`'s idempotence scan, which compares the caller's raw value against the stored one. A value with no break is returned untouched, so a clean write stays byte-identical and no existing ledger is reformatted. Orchestrator-owned enumerable fields — `date`, and `append_entry`'s `status`/`severity` — are validated and raise instead, a new exception path on four released functions. No gate at the schema boundary: ablation showed it did no integrity work while one line break could discard a whole triage plan and escalate to RunPaused, paging a human with nothing to fix. The skill docs state the expectation as guidance instead. Coverage is partial by construction — the dev and migration sessions write the ledger directly (#326). Finding and prior art from @Haven2026 on #274. Closes #305
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
WalkthroughDeferred-work ledger writers now collapse multiline free text to single lines and validate dates, statuses, and severities. CLI and TUI decision recording handle validation failures, while sweep, decision, writer, and documentation tests cover sanitization and rejection behavior. ChangesLedger hardening
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
Greptile SummaryThis PR fixes a ledger-injection vulnerability in the deferred-work writers: because the ledger is line-oriented but mutators interpolated their arguments verbatim, a model-authored value containing a line break could mint phantom entries, inject extra
Confidence Score: 5/5Safe to merge. The sanitization is placed correctly at every interpolation point, values without line breaks are returned byte-for-byte unchanged, and the new validators fire before any file I/O. The fix is narrowly scoped to the three writer functions, placement decisions are each pinned by move-ablation tests, and Unicode line-break coverage is verified by exhaustive enumeration over all 0x110000 code points. Both callers that receive Files Needing Attention: No files require special attention. The ordering of
|
| Filename | Overview |
|---|---|
| src/bmad_loop/deferredwork.py | Core of the fix: adds _one_line, _require_iso_date, _require_canonical_status, LINE_BREAK_RE, _ISO_DATE_RE, and _CANONICAL_SEVERITIES. Sanitization is placed correctly in _apply_done (not mark_done), before the idempotence scan in append_entry, and before the empty-detail check in append_decision. |
| src/bmad_loop/decisions.py | Docstring update only: documents the date precondition and that free text is sanitized not refused. |
| src/bmad_loop/cli.py | Adds ValueError to the cmd_decisions exception handler so a failure is attributed to the specific decision, rather than surfacing as an unattributed traceback. |
| src/bmad_loop/tui/app.py | Adds ValueError to the TUI's apply_pre_answer handler to prevent an uncaught exception from crashing the dashboard. |
| tests/test_deferredwork.py | Adds 211 lines of tests covering the full Unicode line-break set by enumeration, all three injection shapes, idempotence-under-sanitization, and validator rejection before any write. |
| tests/test_sweep.py | Pins the two live writer paths and confirms validate_triage accepts multiline free text without raising. |
| tests/test_decisions.py | Tests multiline sanitization through apply_pre_answer and that a bad-date ValueError fires before any side-effect. |
| tests/test_cli.py | Tests cmd_decisions error handler attribution for both the ValueError and BmadConfigError paths. |
| src/bmad_loop/data/skills/bmad-loop-sweep/automation-mode.md | Documents the single-line expectation as guidance (not a validation rule) and exempts both intent fields with rationale. |
| src/bmad_loop/data/skills/bmad-loop-sweep/deferred-work-format.md | Adds a section explaining the three corruption shapes and directing authors to keep breaks out of field values. |
| CHANGELOG.md | Documents the bug, the fix, the new exception paths, and the remaining out-of-scope producers. |
Reviews (2): Last reviewed commit: "fix(cli): attribute a failed pre-answer ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_deferredwork.py (1)
170-183: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueSingle-pass the codepoint scan to halve this test's cost.
Two independent
range(0x110000)loops build ~2.2M throwaway strings; one loop over the same range collecting both sets keeps the oracle exact while cutting the work in half.♻️ Single-pass variant
- splits = {chr(c) for c in range(0x110000) if len(("a" + chr(c) + "b").splitlines()) > 1} - matches = {chr(c) for c in range(0x110000) if LINE_BREAK_RE.fullmatch(chr(c))} + splits: set[str] = set() + matches: set[str] = set() + for c in range(0x110000): + ch = chr(c) + if len(("a" + ch + "b").splitlines()) > 1: + splits.add(ch) + if LINE_BREAK_RE.fullmatch(ch): + matches.add(ch) assert matches == splits🤖 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` around lines 170 - 183, Update test_line_break_set_is_exactly_what_str_splitlines_splits_on to iterate over range(0x110000) once, compute each character’s splitlines() result and LINE_BREAK_RE.fullmatch result within that loop, and add the character to the corresponding set(s). Preserve the existing exact-set comparison and ordinary-space/tab assertions.
🤖 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/cli.py`:
- Around line 1499-1508: Extend the exception tuple in the decision-recording
try/except around decisions.apply_pre_answer to include
bmadconfig.BmadConfigError, matching the handling in _record_decision. Keep the
existing per-decision error message and return behavior unchanged.
In `@src/bmad_loop/data/skills/bmad-loop-sweep/deferred-work-format.md`:
- Line 45: Remove the trailing spaces inside both Markdown code spans in
deferred-work-format.md, including the spans around the ### examples, so the
closing backticks immediately follow the final character and MD038 passes.
---
Nitpick comments:
In `@tests/test_deferredwork.py`:
- Around line 170-183: Update
test_line_break_set_is_exactly_what_str_splitlines_splits_on to iterate over
range(0x110000) once, compute each character’s splitlines() result and
LINE_BREAK_RE.fullmatch result within that loop, and add the character to the
corresponding set(s). Preserve the existing exact-set comparison and
ordinary-space/tab assertions.
🪄 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: f9835be2-2df1-48df-bc61-6ad5d8233ce2
📒 Files selected for processing (11)
CHANGELOG.mdsrc/bmad_loop/cli.pysrc/bmad_loop/data/skills/bmad-loop-sweep/automation-mode.mdsrc/bmad_loop/data/skills/bmad-loop-sweep/deferred-work-format.mdsrc/bmad_loop/decisions.pysrc/bmad_loop/deferredwork.pysrc/bmad_loop/tui/app.pytests/test_cli.pytests/test_decisions.pytests/test_deferredwork.pytests/test_sweep.py
Adds `bmadconfig.BmadConfigError` to `cmd_decisions`' per-decision handler, beside the `ValueError` it already caught, and corrects what that handler is for. The review that prompted this said an escaping `BmadConfigError` would reach argparse as a traceback. It would not: `main`'s tail catches it by name and catches everything else through a bare `except Exception`, so the command already exited 1 with a message either way. What the handler actually buys is the `DW-<n>` in that message — the loop has already printed an outcome line per answered decision, and a bare `error: <msg>` after them does not say which one did not land. `BmadConfigError` is the reachable case, unlike the `ValueError` the handler was written for: `apply_pre_answer` re-reads the BMAD config on every call and `prompter.ask` blocks on the human in between, so a config removed or broken mid-prompt raises here even though the read at the top of the command succeeded. Leaving it out gave the likelier failure the worse message. The TUI sibling already caught it. Both tests assert the id prefix and say why: ablating the handler leaves the exit code and the message body intact, so those assertions are the message's shape, not the guard. The new one reproduces the window by removing the config from inside the stub prompter's `ask`, so the live `load_paths` call is what fails rather than a patched raise. Also corrects `_one_line`'s docstring, which cross-referenced `append_entry` and `append_decision` as if they handled an empty value the same way. They do not — one substitutes `(untitled DW-<n>)`, the other drops the ` — ` separator — and `append_decision`'s `label` needs neither, since every `LINE_BREAK_RE` member is `str.isspace()` and `validate_triage` builds each `DecisionOption` with `.strip() or key`.
|
Triaged the two bot reviews. Two findings were valid and are fixed in 7416149; two are declined with the evidence on their threads. Answering the two pre-merge check warnings here. Findings
The one worth reading is the first. CodeRabbit said an escaping On the codepoint-scan nitpickThe claim was that merging the two
Indistinguishable, and 0.15 s total inside a 3352-test suite. The dominant work is Pre-merge checksLinked Issues — "omits the validate_triage feedback layer." That layer is not missing, it was removed, and the removal is the argued part of this PR (see What is deliberately not here). Deleting its entire body left every ledger-integrity test green, because Docstring coverage 67.39%. No such gate exists in this repo (see Verification after 7416149
|
1 participant
Closes #305. Finding and prior art from @Haven2026 on #274.
The bug
The deferred-work ledger is line-oriented, but every mutator in
deferredwork.pyinterpolated itsstring arguments verbatim. A value carrying a line break injects ledger lines — and two live paths
feed model-authored text into these writers: a triage session's
already_resolved[].evidenceand a decision option's
resolution/intent.Three distinct corruptions, all reproduced and each pinned by its own test:
fixed\n### DW-99: fake\nstatus: openstatus:lines- source_spec:FLAT_ENTRY_RE, re-surfacing the tail as a phantom legacy itemWorth stating because it cost a worthless test during review:
STATUS_REtakes the first match,so an injected
status:never changes whatparse_ledgerreports. A test assertingentry.statusor
entry.openpasses with the guard deleted. The observables are line structure, not parsed state.The fix
One layer:
_one_linecollapses line breaks at the point each writer serializes its arguments, andnever raises — the close paths (
sweep._close_resolved,decisions.apply_pre_answer) are calledbare, so a
ValueErrorwould reach the run loop's catch-all and end the sweep as crashed.Three placement decisions carry the correctness, each pinned by a move ablation rather than only
a delete:
_apply_done, not themark_donewrapper.Engine._apply_deferred_closescallsmark_done_manydirectly, so a wrapper-side guard never sees a story close.append_entry's idempotence scan, which compares the caller's raw value against thestored one via
field_line_present— sanitizing afterwards would make every replay of the samemultiline defer append a duplicate.
append_decision's emptiness test, so the—separator drops with the detail.The break set is
str.splitlines()'s, not\nalone:parse_legacyscans withsplitlines()whileparse_ledgerusesre.MULTILINE, so aU+2028splits an entry for one reader and is invisible tothe other — the two then disagree about what the ledger says. A test enumerates all 0x110000 code
points asserting set-equality.
A value with no break is returned untouched, so a clean write is byte-identical and no existing
ledger is reformatted.
date, andappend_entry'sstatus/severity, are orchestrator-owned enumerables and raiseinstead — a new exception path on four released functions, called out in the CHANGELOG.
What is deliberately not here
No gate at the schema boundary. An earlier revision added one to
validate_triage; it wasremoved after ablation showed it did no integrity work — deleting its entire body left every
ledger-integrity test green, because
_one_linealready neutralizes the break losslessly. Its onlylive effect was cost: one line break discarded the whole triage plan, and with
max_triage_attempts = 2the second failure escalates toRunPaused— paging a human who hasnothing to fix, since the ledger is intact and the model cannot be edited.
validate_triageaccepting multiline free text is now required behavior with its own test and an inverse ablation.
The skill docs state the expectation as guidance instead, so a compliant session never emits a
break, and nothing gates on it.
Coverage is partial by construction
The dev session appends flat blocks to the ledger directly, and the migration session rewrites the
whole file — neither passes through these writers. Structured deferrals are #326.
Follow-ups filed from this review
next_seqscans prose, so aDW-<n>token steers id allocationappend_decision/append_entrybarewrite_textcan truncate the ledger to zero bytesBUNDLE_NAME_REaccepts a trailing newline;Bundle.nameis used raw as a path segmentVerification
trunk checkcleanfailedin theablated run — a bare nonzero exit lies (a stale node id reads as "failed", so does an
IndentationErrorin the patch)str.splitlines()oracle rather than typed: a literalU+2028normalizes to a plain space in transit, which silently turned five cases intospace-testing cases during development and would defeat any review-by-reading
Summary by CodeRabbit
Bug Fixes
Documentation