diff --git a/CHANGELOG.md b/CHANGELOG.md index daa03de6..87da55d1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -125,6 +125,32 @@ story `, the same annotation a sweep bundle writes. Both sprint and stories ### Fixed +- **The orchestrator's ledger writers no longer inject lines from a multiline value (#305).** Found + by [@Haven2026](https://github.com/Haven2026) in #274. The deferred-work ledger is line-oriented, + but `deferredwork`'s mutators interpolated their arguments verbatim: a resolution note of + `fixed\n### DW-99: fake\nstatus: open` minted a phantom entry, a note containing `\n- source_spec:` + truncated the entry's span and re-surfaced its tail as a phantom legacy item, and a mid-note break + left one entry carrying two `status:` lines. The writers now collapse line breaks in free text to a + space. They sanitize rather than raise — the sweep's close paths call them bare, so a `ValueError` + would end the run as crashed — and nothing upstream rejects on a break either: a formatting defect + must not cost a triage attempt. The break set is `str.splitlines()`'s, not `\n` alone, since + `parse_legacy` scans with it. A value that never carried a break is written byte-for-byte as + before, and a title that sanitizes away is named rather than left blank. + + **New exception paths.** The orchestrator-owned fields are now validated where they never were: + `mark_done`, `mark_done_many`, `append_decision` and `append_entry` raise `ValueError` on a `date` + that is not ISO `YYYY-MM-DD`, and `append_entry` on a `status` outside `open`/`done ` or a + `severity` outside `critical|high|medium|low`. All four previously accepted anything and wrote it. + A bad value there is a programmer bug, not model output. + + This covers the orchestrator's writers only. Two producers still write ledger markdown directly and + bypass them: the inner dev session's flat appends, and the sweep's migration session, which rewrites + the whole file (its validation checks legacy leftovers, duplicate ids, statuses and numbering — not + breaks inside a field value). Sanitizing also makes a break inert, not the text around it: a + `DW-` token surviving as prose still counts toward `next_seq`. The sweep skill now states the + single-line expectation as guidance; an existing install picks it up on + `bmad-loop init --force-skills` (a plain `init` keeps skills that already exist). + - **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 diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index 42ed4411..65d1b77a 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -1496,7 +1496,27 @@ def cmd_decisions(args: argparse.Namespace) -> int: today = time.strftime("%Y-%m-%d") for decision in pending: option = prompter.ask(decision) - decisions.apply_pre_answer(project, decision, option, date=today) + try: + decisions.apply_pre_answer(project, decision, option, date=today) + except (OSError, bmadconfig.BmadConfigError, ValueError) as e: + # What this buys is the `{decision.id}` in the message, and only + # that: `main`'s tail catches BmadConfigError by name and everything + # else through a bare `except Exception`, so none of these ever + # reached argparse as a traceback, and the exit code is 1 either way. + # The loop is what makes attribution worth a handler — it has already + # printed an outcome line per answered decision, and a bare + # `error: ` after them does not say which one did not land. + # + # ValueError is the ledger writers' `date` precondition, unreachable + # from the strftime above and here for the same reason as the TUI's + # copy of this call. BmadConfigError is the reachable one: + # `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 + # this command succeeded. Leaving it out gave the likelier failure the + # worse message. + print(f"error: could not record {decision.id}: {e}", file=sys.stderr) + return 1 if option.effect == "close": outcome = "closed now" elif option.effect == "build": diff --git a/src/bmad_loop/data/skills/bmad-loop-sweep/automation-mode.md b/src/bmad_loop/data/skills/bmad-loop-sweep/automation-mode.md index b4415998..03985042 100644 --- a/src/bmad_loop/data/skills/bmad-loop-sweep/automation-mode.md +++ b/src/bmad_loop/data/skills/bmad-loop-sweep/automation-mode.md @@ -65,6 +65,21 @@ field-by-field, and will kill this session after your final turn. `build|close|keep-open`, `intent` required when effect is `build`, `recommendation` must be one of the option keys. +- Write `already_resolved[].evidence` and an option's `label` and `resolution` + as a **single line** — the orchestrator copies each onto one line of the + line-oriented deferred-work ledger. This is guidance, not a validation rule: + a break is collapsed to a space rather than rejected, so it costs you nothing + but reads worse. + + Both `intent` fields stay 2-6 sentences as asked for above, newlines and all, + but for two different reasons. A **bundle's** `intent` is rendered to its own + intent file and never reaches the ledger. A **build option's** `intent` does + reach it — the ledger `decision:` line takes `resolution or intent`, and a + build option normally has no `resolution`, so its intent is usually exactly + what lands there. It is exempt anyway because the orchestrator flattens it on + write, and holding it to one line would flatten the brief that drives the + whole dev bundle to buy nothing. + - **Migration sessions** (`--migrate`, see `./migration-mode.md`) use this result schema instead: diff --git a/src/bmad_loop/data/skills/bmad-loop-sweep/deferred-work-format.md b/src/bmad_loop/data/skills/bmad-loop-sweep/deferred-work-format.md index e84f89c0..da91285e 100644 --- a/src/bmad_loop/data/skills/bmad-loop-sweep/deferred-work-format.md +++ b/src/bmad_loop/data/skills/bmad-loop-sweep/deferred-work-format.md @@ -35,6 +35,22 @@ reason: status: open ``` +**Every field line is exactly one line, and so is the title.** The format is +line-oriented: readers find each field by scanning for `:` at the start of +a line, and an entry ends at whichever comes first — the next `### DW-` +entry, any other `#` .. `######` heading, or a `- source_spec:` flat-append +bullet. A value carrying a line break therefore does not wrap; it becomes new +ledger content, and three things can follow: + +- a break followed by `### ` mints an entry nobody filed; +- a break before a `status:` line leaves one entry carrying two, so the ledger + no longer says one thing about it; +- a break followed by `- source_spec:` cuts the entry short at that bullet, and + everything after it re-surfaces as a phantom _legacy_ item. + +Keep breaks out of field values, along with `### ` and a leading +`- source_spec:`. If a reason needs two sentences, write them on one line. + `severity:` is optional — entries written before this field existed have none and that is fine; readers must treat a missing or unrecognized value as "unspecified". Use `critical` for correctness/security issues, `high` for diff --git a/src/bmad_loop/decisions.py b/src/bmad_loop/decisions.py index 8b54e259..357fef3f 100644 --- a/src/bmad_loop/decisions.py +++ b/src/bmad_loop/decisions.py @@ -140,7 +140,13 @@ def apply_pre_answer( the open set now), while `build`/`keep-open` are saved to the pre-answer store for the next sweep to consume. When `commit`, the ledger and store are committed on their own (only those paths) — best effort, so a non-git or - dirty tree never blocks the on-disk record.""" + dirty tree never blocks the on-disk record. + + Precondition: `date` is ISO `YYYY-MM-DD`. The ledger writers raise + `ValueError` on anything else (it would otherwise land a `status:` line that + reads as neither open nor done), so a caller building the date itself must + either guarantee the format or catch it. The option's own free text carries + no such precondition — it is sanitized, never refused.""" paths = bmadconfig.load_paths(project) ledger = paths.deferred_work detail = option.resolution or option.intent diff --git a/src/bmad_loop/deferredwork.py b/src/bmad_loop/deferredwork.py index 15c9abc9..566d55da 100644 --- a/src/bmad_loop/deferredwork.py +++ b/src/bmad_loop/deferredwork.py @@ -15,6 +15,7 @@ import re from collections.abc import Sequence from dataclasses import dataclass +from datetime import date as calendar_date from pathlib import Path from .platform_util import atomic_write_text @@ -32,6 +33,20 @@ _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) +# Everything `str.splitlines()` splits on, not `\n` alone (#305). The writers +# below interpolate their arguments into a line-oriented file, so a break in a +# value injects ledger lines. The C1/Unicode members are load-bearing rather +# than decorative: `parse_legacy` scans with `splitlines()` while `parse_ledger` +# matches with `re.MULTILINE`, so a U+2028 splits an entry for one reader and is +# invisible to the other — the two then disagree about what the ledger says. +LINE_BREAK_RE = re.compile(r"[\n\r\v\f\x1c-\x1e\x85\u2028\u2029]+") +# The writers' date shape. Deliberately a separate literal from the legacy +# parser's `_DATE_TOKEN_RE`, which happens to look similar today: that one +# decides whether a freeform heading is a dated section, and tightening what the +# orchestrator will *write* must never quietly retune what `parse_legacy` reads. +# Spelled `[0-9]` rather than `\d`, which also matches Arabic-Indic, fullwidth +# and mathematical digit forms — the ledger's readers understand none of them. +_ISO_DATE_RE = re.compile(r"[0-9]{4}-[0-9]{2}-[0-9]{2}") @dataclass(frozen=True) @@ -203,10 +218,101 @@ def _insert_after_status(text: str, entry: DWEntry, line: str) -> str: return text[:insert_at] + "\n" + line + text[insert_at:] +def _one_line(value: str) -> str: + """Collapse every run of line-break characters in `value` to a single space. + + The whole of the #305 fix. These writers interpolate their arguments into a + line-oriented file, so a value carrying a break mints a phantom + `### DW-` entry, truncates the entry's span at :data:`FLAT_ENTRY_RE` and + re-surfaces the tail as a legacy item, or leaves the entry carrying two + `status:` lines. + + Note what the last shape does *not* do: `STATUS_RE` takes the first match, so + an injected `status:` never changes what `parse_ledger` reports. A test that + asserts on `entry.status` therefore passes with this guard deleted — the + observable is the line structure. + + Sanitizes; never raises, and nothing upstream rejects on a break either. The + close paths call these writers bare (`sweep._close_resolved`, + `decisions.apply_pre_answer`), so a `ValueError` would end the sweep as + crashed; refusing the same text back at `validate_triage` only moved the + stoppage to a pause. Collapsing is lossless enough — the ledger wants one + line anyway — so this is the fix, and the skill docs are guidance that + reduces occurrences without gating on them. + + A value with no break is returned **untouched**, so an existing ledger is + never reformatted and a clean write is byte-identical to before the guard. + The trailing `.strip()` removes all surrounding whitespace, not merely the + space a leading or trailing break left behind — which is why it must stay on + the far side of that fast path. + + A break-only value therefore sanitizes to `""`. Keeping it non-empty *here* + could only yield bare whitespace, which trades an unfindable entry for an + unidentifiable one, so each caller handles its own empties — and by two + different strategies, which is why neither belongs in this helper. + :func:`append_entry` **substitutes**, naming a vanished title + `(untitled DW-)` so the id it just burned stays findable. + :func:`append_decision` **drops**, shedding the ` — ` separator along with an + empty detail rather than promising one that is not there. Its `label` needs + neither: every member of :data:`LINE_BREAK_RE` is `str.isspace()`, and + `validate_triage` builds each `DecisionOption` with `.strip() or key`, so a + break-only label has already become the option key before it arrives.""" + if not LINE_BREAK_RE.search(value): + return value + return LINE_BREAK_RE.sub(" ", value).strip() + + +def _require_iso_date(value: str) -> None: + """Raise unless `value` is a strict ISO `YYYY-MM-DD` calendar date. + + Raising is right here and wrong for free text: `date` is orchestrator-owned + (`Engine._today()`), never model-authored, so a bad value is a programmer + bug. Letting it through writes a `status:` line that reads as neither open + nor done, which `classify` reports as malformed and `open_ids` drops — the + entry silently leaves the sweep's world. + + The regex is not redundant with `date.fromisoformat`: since 3.11 that also + accepts `20260611` and ISO week dates, neither of which the ledger's own + readers recognize, and it is the regex — via `[0-9]` — that pins the digits + to ASCII. `fromisoformat` in turn rejects the well-shaped impossible day + (`2026-02-30`) that no pattern can catch.""" + if not _ISO_DATE_RE.fullmatch(value): + raise ValueError(f"date must be YYYY-MM-DD: {value!r}") + try: + calendar_date.fromisoformat(value) + except ValueError as exc: + raise ValueError(f"date must be YYYY-MM-DD: {value!r}") from exc + + +def _require_canonical_status(status: str) -> None: + """Raise unless `status` is exactly `open` or `done YYYY-MM-DD`. + + Two halves with two different dependents. The *first word* is what + :attr:`DWEntry.open` and :func:`classify` branch on, so anything but `open` + or `done` makes an entry unreadable to both. The *date* is invisible to them + — they read `status.split()[0]` and cannot tell `done 2026-02-30` from a real + day — but it is not invisible downstream: the whole status value is carried + verbatim to readers (the TUI's deferred pane, the `--json` projections), so a + malformed date is rendered to a human as though it were one.""" + if status == "open": + return + if status.startswith("done "): + _require_iso_date(status.removeprefix("done ")) + return + raise ValueError(f"status must be 'open' or 'done YYYY-MM-DD': {status!r}") + + def _apply_done(text: str, dw_id: str, date: str, note: str) -> str | None: """Flip one entry to `status: done ` + a resolution note *within* `text`. None when the entry is missing or not open. The entry is re-located after the - status rewrite because that edit shifts every later span offset.""" + status rewrite because that edit shifts every later span offset. + + The note is sanitized here, at the point of interpolation, rather than on + :func:`mark_done`: that is a one-id wrapper over :func:`mark_done_many`, which + `Engine._apply_deferred_closes` calls directly, so a wrapper-side guard would + never see a story close (#305). `date` is validated by the sole caller, at its + entry, so the check does not depend on a ledger existing.""" + note = _one_line(note) entry = _find_entry(text, dw_id) if entry is None or not entry.open: return None @@ -234,7 +340,12 @@ def mark_done_many(path: Path, dw_ids: Sequence[str], date: str, note: str) -> l The write goes through :func:`~bmad_loop.platform_util.atomic_write_text` rather than a bare tmp+replace: swapping a fresh inode over the ledger otherwise resets its mode (a ``0600`` ledger silently becoming world-readable) - and turns a symlinked ledger into a regular file.""" + and turns a symlinked ledger into a regular file. + + ``date`` is validated before the ``is_file`` short-circuit so a programmer bug + fails the same way whether or not a ledger happens to exist — a guard that + only fires when the file is present is one an absent fixture hides.""" + _require_iso_date(date) if not path.is_file(): return [] text = path.read_text(encoding="utf-8") @@ -258,13 +369,28 @@ def mark_done(path: Path, dw_id: str, date: str, note: str) -> bool: def append_decision(path: Path, dw_id: str, date: str, label: str, detail: str) -> bool: - """Record a human decision on an entry without changing its status.""" + """Record a human decision on an entry without changing its status. + + `label` and `detail` come from a triage session's `DecisionOption`, so they + are sanitized to one line rather than refused — see :func:`_one_line`. This + is also where a build option's `intent` gets flattened, since it reaches the + ledger only as `detail = option.resolution or option.intent`. + + Precondition: `date` is ISO `YYYY-MM-DD`; anything else raises `ValueError`, + checked before the ``is_file`` short-circuit so an absent ledger cannot hide + the bug.""" + _require_iso_date(date) if not path.is_file(): return False text = path.read_text(encoding="utf-8") entry = _find_entry(text, dw_id) if entry is None: return False + label = _one_line(label) + # Sanitize before the emptiness test, never after: a break-only detail + # collapses to "" and must then drop the separator with it, or the entry + # carries a dangling `— ` promising a detail that is not there. + detail = _one_line(detail) detail_part = f" — {detail}" if detail else "" text = _insert_after_status(text, entry, f"decision: {date} {label}{detail_part}") path.write_text(text, encoding="utf-8") @@ -308,7 +434,26 @@ def append_entry( Idempotent: returns None without writing when an open entry already carries the same `origin:` marker and `source_spec:` — so re-running the same defer (e.g. a second sweep of the same story) never duplicates the entry. Creates - the ledger (and parent dir) if it does not yet exist.""" + the ledger (and parent dir) if it does not yet exist. + + Free text is sanitized (:func:`_one_line`) **before** the idempotence scan, + which compares the caller's value against the stored one via + :func:`field_line_present`: sanitizing afterwards would compare a raw value + against a sanitized line, so every replay of the same multiline defer would + miss its own entry and append another. `status` and `severity` are + orchestrator-owned enumerations and raise instead.""" + _require_canonical_status(status) + # The whitelist is derived from the legacy parser's alias table (defined + # below; resolved at call time) so what this writer emits and what + # `field_severity` normalizes to cannot drift apart. + if severity and severity not in _CANONICAL_SEVERITIES: + raise ValueError(f"severity must be one of {sorted(_CANONICAL_SEVERITIES)}: {severity!r}") + given_title = bool(title) + title = _one_line(title) + origin = _one_line(origin) + source_spec = _one_line(source_spec) + reason = _one_line(reason) + location = _one_line(location) text = path.read_text(encoding="utf-8") if path.is_file() else "" for entry in parse_ledger(text): if ( @@ -318,6 +463,20 @@ def append_entry( ): return None dw_id = f"DW-{next_seq(text)}" + if given_title and not title.strip(): + # A break-only title sanitizes to nothing, and `### DW-: ` is a + # heading `HEADING_RE`'s `(.+?)` does not match: the caller is handed an + # id no reader can find while `next_seq` has already burned it. + # + # Tested with `.strip()`, not `not title`: a title of `" "` carries no + # break at all, so `_one_line` returns it unchanged by the byte-identity + # fast path and it stays truthy. It parses, but renders blank in + # `status`, `--json` and the TUI — the unidentifiable half of the same + # problem, reached without ever touching the sanitizer. + # + # Scoped to a title that *had* content: an already-empty one keeps its + # long-standing behavior, and the invariant is about non-empty values. + title = f"(untitled {dw_id})" lines = [ f"### {dw_id}: {title}", f"origin: {origin}", @@ -366,6 +525,10 @@ def append_entry( "minor": "low", "trivial": "low", } +# What every alias above normalizes to, and so the only values `append_entry` may +# write. Derived rather than restated: a hand-copied whitelist drifts the moment +# an alias is added for a new canonical level. +_CANONICAL_SEVERITIES = frozenset(SEVERITY_ALIASES.values()) SEVERITY_FIELD_RE = re.compile( r"^[ \t]*(?:[-*][ \t]+)?(?:\*\*)?(?:severity|priority)[ \t]*:[ \t]*(?:\*\*)?[ \t]*" diff --git a/src/bmad_loop/tui/app.py b/src/bmad_loop/tui/app.py index 32c6ec7a..05a3feab 100644 --- a/src/bmad_loop/tui/app.py +++ b/src/bmad_loop/tui/app.py @@ -337,7 +337,12 @@ def _record_decision(self, decision: object, option: object) -> bool: option, # pyright: ignore[reportArgumentType] date=time.strftime("%Y-%m-%d"), ) - except (OSError, bmadconfig.BmadConfigError) as e: + except (OSError, bmadconfig.BmadConfigError, ValueError) as e: + # ValueError is the ledger writers' date precondition. It cannot fire + # from the strftime above, but an uncaught one here escapes into the + # Textual event loop and takes the dashboard down mid-walk — the + # per-decision notification is the right degradation for a modal the + # human is still stepping through. self.notify( f"failed to record {decision.id}: {e}", # pyright: ignore[reportAttributeAccessIssue] severity="error", diff --git a/tests/test_cli.py b/tests/test_cli.py index 22b5e592..940689bb 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -553,6 +553,76 @@ def ask(self, decision): assert decisions.pending_missed_decisions(project.project) == [] +def test_decisions_names_the_decision_a_bad_date_failed(project, capsys, monkeypatch): + """`apply_pre_answer`'s `date` precondition raises `ValueError`, and the loop's + handler must attribute it to the decision that did not land. + + Only the `could not record DW-1` assertion bites. `main`'s tail catches every + exception this handler does — BmadConfigError by name, the rest through a bare + `except Exception` — so deleting the handler still exits 1 and still prints the + `date must be YYYY-MM-DD` text. Those two assertions are the message's shape, + not the guard; the id prefix is the whole observable.""" + from conftest import write_ledger + + from bmad_loop import decisions + + install_bmad_config(project) + write_ledger(project, {"DW-1": "open"}) + _make_run_with_decision(project) + + class _StubPrompter: + def ask(self, decision): + return decision.option("1") + + monkeypatch.setattr("bmad_loop.sweep.DecisionPrompter", lambda *a, **k: _StubPrompter()) + monkeypatch.setattr("bmad_loop.cli.time.strftime", lambda *_a: "13/06/2026") + + assert cli.main(["decisions", "--project", str(project.project)]) == 1 + + captured = capsys.readouterr() + assert "could not record DW-1" in captured.err + assert "date must be YYYY-MM-DD" in captured.err + assert decisions.load_pre_answers(project.project) == {} # nothing recorded + + +def test_decisions_names_the_decision_a_broken_config_failed(project, capsys, monkeypatch): + """The reachable half of the same handler, and the reason `BmadConfigError` + belongs in its tuple beside the unreachable `ValueError`. + + `apply_pre_answer` re-reads the BMAD config on every call while `prompter.ask` + blocks on the human, so the config can break *after* the read at the top of + this command succeeded — reproduced here by removing it from inside `ask` + rather than by patching `apply_pre_answer` to raise, so the live `load_paths` + call is what fails. + + Asserted on the `DW-1` prefix, not on the exit code: `main` catches + `BmadConfigError` by name, so without this tuple entry the command still exits + 1 and still prints the config message — just bare, after a run of per-decision + outcome lines that then do not say which decision it belongs to.""" + from conftest import write_ledger + + from bmad_loop import decisions + + install_bmad_config(project) + write_ledger(project, {"DW-1": "open"}) + _make_run_with_decision(project) + config = project.project / "_bmad" / "bmm" / "config.yaml" + + class _StubPrompter: + def ask(self, decision): + config.unlink() # the human broke it while this prompt was blocking + return decision.option("1") + + monkeypatch.setattr("bmad_loop.sweep.DecisionPrompter", lambda *a, **k: _StubPrompter()) + + assert cli.main(["decisions", "--project", str(project.project)]) == 1 + + captured = capsys.readouterr() + assert "could not record DW-1" in captured.err + assert "BMAD config not found" in captured.err + assert decisions.load_pre_answers(project.project) == {} # nothing recorded + + def test_status_surfaces_missed_decision_count(project, capsys): from conftest import write_ledger, write_sprint diff --git a/tests/test_decisions.py b/tests/test_decisions.py index 069383cb..a4c6ace5 100644 --- a/tests/test_decisions.py +++ b/tests/test_decisions.py @@ -2,6 +2,7 @@ import json +import pytest from conftest import install_bmad_config, write_ledger from bmad_loop import decisions, deferredwork @@ -133,6 +134,58 @@ def test_apply_pre_answer_build_records_store_and_ledger(project): assert "chore(decisions): pre-answer DW-1" in _git_log(project) +def test_apply_pre_answer_sanitizes_a_multiline_detail(project): + """The human-decision writer path (`decisions.py:146-150`), called bare: it + must sanitize rather than raise. Pins `detail = option.resolution or + option.intent` on its fallback branch — the resolution is empty here, so an + option `intent` is what reaches the ledger.""" + install_bmad_config(project) + write_ledger(project, {"DW-1": "open"}) + from bmad_loop.sweep import Decision + + opt = DecisionOption( + key="1", label="Build\ncap", effect="build", intent="widen the field.\nThen backfill." + ) + d = Decision(id="DW-1", question="build it?", context="", options=(opt,), recommendation="1") + + decisions.apply_pre_answer(project.project, d, opt, date="2026-06-13") + + text = project.deferred_work.read_text(encoding="utf-8") + entries = {e.id: e for e in deferredwork.parse_ledger(text)} + assert set(entries) == {"DW-1"} # no phantom entry minted + assert ( + "decision: 2026-06-13 Build cap — widen the field. Then backfill." in entries["DW-1"].body + ) + assert len([line for line in text.splitlines() if line.startswith("decision:")]) == 1 + assert entries["DW-1"].open # build stays open until a sweep builds it + + +def test_apply_pre_answer_raises_on_a_bad_date_leaving_nothing_written(project): + """The documented precondition, and that it fires before *any* of the four + side effects `apply_pre_answer` chains: `append_decision`, `mark_done`, + `record_pre_answer` and `commit_paths`. A raise partway through would leave a + ledger annotation with no store entry, or either with no commit. + + Both callers catch it — the TUI degrades to a per-decision notification and + `bmad-loop decisions` to an error line — so the failure a human sees must + correspond to nothing having happened.""" + install_bmad_config(project) + write_ledger(project, {"DW-1": "open"}) + from bmad_loop.sweep import Decision + + opt = DecisionOption(key="1", label="Build", effect="build", intent="do it") + d = Decision(id="DW-1", question="?", context="", options=(opt,), recommendation="1") + ledger_before = project.deferred_work.read_text(encoding="utf-8") + store_before = decisions.load_pre_answers(project.project) + + with pytest.raises(ValueError, match="date must be YYYY-MM-DD"): + decisions.apply_pre_answer(project.project, d, opt, date="13/06/2026") + + assert project.deferred_work.read_text(encoding="utf-8") == ledger_before + assert decisions.load_pre_answers(project.project) == store_before + assert not decisions.store_path(project.project).exists() + + def test_apply_pre_answer_close_marks_done_no_store(project): install_bmad_config(project) write_ledger(project, {"DW-1": "open"}) diff --git a/tests/test_deferredwork.py b/tests/test_deferredwork.py index c61cefa4..2bfdf287 100644 --- a/tests/test_deferredwork.py +++ b/tests/test_deferredwork.py @@ -5,6 +5,9 @@ import pytest from bmad_loop.deferredwork import ( + _ISO_DATE_RE, + LINE_BREAK_RE, + SEVERITY_ALIASES, append_decision, append_entry, classify, @@ -141,6 +144,211 @@ def test_append_decision_missing_file(tmp_path): assert not mark_done(tmp_path / "nope.md", "DW-1", "2026-06-11", "x") +# ------------------------------------------------- line-break injection (#305) +# +# The ledger is line-oriented and every mutator interpolates its arguments, so a +# value carrying a break injects ledger lines. Free text is SANITIZED, never +# refused, and nothing upstream rejects it either: the close paths call these +# writers bare (sweep._close_resolved, decisions.apply_pre_answer), so a raise +# ends the sweep run as crashed. Only the orchestrator-owned enumerables (date, +# status, severity) raise. + +# Derived from the oracle, never typed. A literal U+2028 in a source file is +# trivially normalized to a plain space by an editor or a tool, and the two are +# then indistinguishable by inspection — which silently turns a line-break case +# into a whitespace case that passes for the wrong reason. The bound is safe: +# the exhaustive test below proves the whole split set lives under it. +BREAK_CHARS = tuple(chr(c) for c in range(0x2030) if len(("a" + chr(c) + "b").splitlines()) > 1) +# Multi-character runs, which `BREAK_CHARS` cannot reach: every entry there is +# one character, and the break-only cases collapse to "" before the quantifier +# matters. Without these, deleting `+` from LINE_BREAK_RE leaves the suite +# green while every CRLF value silently gains a second space — and CRLF is the +# common real-world break, being what a Windows-authored result.json carries. +BREAK_RUNS = ("\r\n", "\n\n") + + +def test_line_break_set_is_exactly_what_str_splitlines_splits_on(): + """`LINE_BREAK_RE` must cover every character `str.splitlines()` splits on, + because `parse_legacy` scans the ledger with it while `parse_ledger` matches + with `re.MULTILINE` — a member missed here splits an entry for one reader and + is invisible to the other. + + Checked by enumeration rather than by listing the members: written as source + escapes these are easy to get wrong, and a `\\u2028` silently normalized to a + plain space would widen the class to collapse ordinary spaces — quietly + breaking the byte-identity guarantee instead of the injection guard.""" + 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))} + assert matches == splits + assert " " not in matches and "\t" not in matches # ordinary whitespace is not a break + + +@pytest.mark.parametrize("brk", [*BREAK_CHARS, *BREAK_RUNS]) +def test_mark_done_many_sanitizes_an_injected_note(tmp_path, brk): + """Driven through `mark_done_many`, not the `mark_done` wrapper: that is the + entry point `Engine._apply_deferred_closes` uses, so a guard placed on the + wrapper would be inert for every story close.""" + path = write_ledger(tmp_path) + before = len(parse_ledger(path.read_text(encoding="utf-8"))) + + note = f"fixed{brk}### DW-99: injected{brk}status: open" + + assert mark_done_many(path, ["DW-1"], "2026-06-11", note) == ["DW-1"] + + text = path.read_text(encoding="utf-8") + entries = {e.id: e for e in parse_ledger(text)} + assert len(entries) == before # no phantom DW-99 minted + assert set(entries) == {"DW-1", "DW-2", "DW-3"} + assert entries["DW-1"].status == "done 2026-06-11" + # the injected text survives as prose on one line — sanitized, not dropped + assert "resolution: fixed ### DW-99: injected status: open" in entries["DW-1"].body + + +@pytest.mark.parametrize("brk", [*BREAK_CHARS, *BREAK_RUNS]) +def test_mark_done_sanitizes_a_note_that_would_double_the_status_line(tmp_path, brk): + """The quiet half of the bug, and the half a status assertion cannot see: + `STATUS_RE` takes the FIRST match, so an injected `status:` line leaves both + `entry.status` and `entry.open` reporting exactly what they should. The damage + is structural — the entry ends up carrying two status lines, so the ledger no + longer says one thing about it, and which line wins depends on which reader + looks. Asserted by counting lines the way `str.splitlines()` does, which is + why the U+2028 case belongs here too.""" + path = write_ledger(tmp_path) + + assert mark_done(path, "DW-1", "2026-06-11", f"fixed{brk}status: open") + + (entry,) = [e for e in parse_ledger(path.read_text(encoding="utf-8")) if e.id == "DW-1"] + assert entry.status == "done 2026-06-11" + assert [line for line in entry.body.splitlines() if line.startswith("status:")] == [ + "status: done 2026-06-11" + ] + + +@pytest.mark.parametrize("run", BREAK_RUNS) +def test_a_multi_character_break_run_collapses_to_exactly_one_space(tmp_path, run): + """`LINE_BREAK_RE`'s `+` quantifier, which nothing else pins: a run of breaks + must become ONE space, not one per character. CRLF is the case that matters — + it is what a Windows-authored `result.json` carries, so it is the most likely + break to reach these writers at all.""" + path = write_ledger(tmp_path) + + assert mark_done(path, "DW-1", "2026-06-11", f"fixed{run}in src/foo.py") + + (entry,) = [e for e in parse_ledger(path.read_text(encoding="utf-8")) if e.id == "DW-1"] + assert "resolution: fixed in src/foo.py" in entry.body + + +def test_mark_done_sanitizes_a_note_that_would_truncate_the_entry(tmp_path): + """The sharpest shape: a break followed by the flat appender's opening line. + `FLAT_ENTRY_RE` bounds a canonical entry on exactly that line (#304), so an + unsanitized note cuts DW-1's span short there and the tail re-surfaces as a + phantom *legacy* item — visible to the sweep's migration trigger, `--dry-run` + leftovers and the TUI as work nobody filed.""" + path = write_ledger(tmp_path) + + assert mark_done(path, "DW-1", "2026-06-11", "fixed\n- source_spec: `x.md`") + + text = path.read_text(encoding="utf-8") + (entry,) = [e for e in parse_ledger(text) if e.id == "DW-1"] + assert "resolution: fixed - source_spec: `x.md`" in entry.body + assert parse_legacy(text) == [] + + +@pytest.mark.parametrize( + "date", + [ + "nope", + "2026-6-11", + "20260611", + "2026-02-30", + "2026-06-11\nx", + "\u0662\u0660\u0662\u0666-\u0660\u0666-\u0660\u0669", + ], + ids=["prose", "unpadded", "compact", "impossible-day", "with-break", "arabic-indic-digits"], +) +def test_mark_done_many_raises_on_a_bad_date_without_writing(tmp_path, date): + """`date` is orchestrator-owned (`Engine._today()`), so a bad value is a + programmer bug — the one place in these writers that raises. The digits are + pinned to ASCII by the pattern itself (`[0-9]`, not `\\d`), so the shape + check refuses an Arabic-Indic date rather than leaning on `fromisoformat`.""" + path = write_ledger(tmp_path) + snapshot = path.read_text(encoding="utf-8") + + with pytest.raises(ValueError, match="date must be YYYY-MM-DD"): + mark_done_many(path, ["DW-1"], date, "fixed") + + assert path.read_text(encoding="utf-8") == snapshot + + +def test_the_date_pattern_itself_refuses_non_ascii_digits(): + """`\\d` matches Arabic-Indic, fullwidth and mathematical digit forms; `[0-9]` + does not. `date.fromisoformat` refuses them a moment later either way, so this + pins the *pattern* rather than the outcome — the docstring's claim is about + the regex, and only a direct assertion can hold it to that.""" + assert _ISO_DATE_RE.fullmatch("2026-06-09") + for foreign in ( + "\u0662\u0660\u0662\u0666-\u0660\u0666-\u0660\u0669", + "\uff12\uff10\uff12\uff16-\uff10\uff16-\uff10\uff19", + ): + assert not _ISO_DATE_RE.fullmatch(foreign) + + +def test_bad_date_raises_even_with_no_ledger_on_disk(tmp_path): + """Validated at function entry, ahead of the `is_file()` short-circuit: a + guard that only fires when the ledger happens to exist is one an absent + fixture hides.""" + missing = tmp_path / "nope.md" + + with pytest.raises(ValueError, match="date must be YYYY-MM-DD"): + mark_done_many(missing, ["DW-1"], "nope", "fixed") + with pytest.raises(ValueError, match="date must be YYYY-MM-DD"): + append_decision(missing, "DW-1", "nope", "Keep", "detail") + + +def test_append_decision_sanitizes_label_and_detail(tmp_path): + path = write_ledger(tmp_path) + + assert append_decision( + path, "DW-3", "2026-06-11", "Keep\ncap", f"Keep{BREAK_CHARS[-1]}status: done 2026-01-01" + ) + + text = path.read_text(encoding="utf-8") + entries = {e.id: e for e in parse_ledger(text)} + assert "decision: 2026-06-11 Keep cap — Keep status: done 2026-01-01" in entries["DW-3"].body + assert len([line for line in text.splitlines() if line.startswith("decision:")]) == 1 + assert entries["DW-3"].open # the injected `status:` did not close it + + +@pytest.mark.parametrize("detail", [*BREAK_CHARS, " \n ", "\r\n\t"]) +def test_append_decision_drops_the_separator_for_a_break_only_detail(tmp_path, detail): + """A detail that sanitizes to nothing must take the ` — ` with it. The + ordering is the guard: sanitizing after the emptiness test would leave the + entry carrying a dangling separator promising a detail that is not there.""" + path = write_ledger(tmp_path) + + assert append_decision(path, "DW-3", "2026-06-11", "Keep cap", detail) + + (line,) = [ + ln for ln in path.read_text(encoding="utf-8").splitlines() if ln.startswith("decision:") + ] + assert line == "decision: 2026-06-11 Keep cap" + assert "—" not in line + + +def test_writers_leave_a_clean_value_byte_identical(tmp_path): + """No reformatting of values that never carried a break — the sanitizer must + not become an incidental whitespace normalizer for existing ledgers.""" + path = write_ledger(tmp_path) + padded = " fixed in src/foo.py\t" + + assert mark_done(path, "DW-1", "2026-06-11", padded) + assert append_decision(path, "DW-3", "2026-06-11", " Keep ", padded) + + text = path.read_text(encoding="utf-8") + assert f"resolution: {padded}" in text + assert f"decision: 2026-06-11 Keep — {padded}" in text + + # ------------------------------------------------------------------- legacy # # Fixtures are condensed verbatim from four real pre-DW project ledgers, @@ -669,6 +877,160 @@ def test_append_entry_idempotency_ignores_incidental_substring(tmp_path): assert new_id == "DW-2" # not suppressed by the incidental mentions +@pytest.mark.parametrize("field", ["title", "origin", "source_spec", "reason", "location"]) +@pytest.mark.parametrize("brk", [*BREAK_CHARS, *BREAK_RUNS]) +def test_append_entry_sanitizes_free_text_into_one_line(tmp_path, field, brk): + """One canonical entry, every field on its own line — no injected heading and + no second `status:` line, whichever field carried the break.""" + p = tmp_path / "deferred-work.md" + values = { + "title": "follow-up", + "origin": "code-review", + "source_spec": "spec-foo.md", + "reason": "still open", + "location": "src/foo.py:12", + } + values[field] += f"{brk}### DW-99: injected{brk}status: done 2026-01-01" + + assert append_entry(p, **values) == "DW-1" + + text = p.read_text(encoding="utf-8") + (entry,) = parse_ledger(text) # one entry: no phantom DW-99 heading + assert entry.id == "DW-1" and entry.open # not closed by the injected status + lines = text.splitlines() + assert len(lines) == 6 # heading + origin/location/source_spec/reason/status + assert sum(line.startswith("### ") for line in lines) == 1 + assert sum(line.startswith("status:") for line in lines) == 1 + + +@pytest.mark.parametrize("title", [*BREAK_CHARS, *BREAK_RUNS, " \n ", "\r\n\t", " ", "\t", "\xa0"]) +def test_append_entry_names_an_entry_with_no_usable_title(tmp_path, title): + """Two routes to the same unusable heading. A break-only title collapses to + nothing, and `### DW-1: ` is a heading `HEADING_RE`'s `(.+?)` does not match: + the caller is handed an id no reader can find, with `next_seq` already past + it. A whitespace-only title never reaches the sanitizer at all — no break, so + the byte-identity fast path returns it unchanged and still truthy — and + parses to a heading that renders blank in `status`, `--json` and the TUI. + Both get a name; neither gets a space.""" + p = tmp_path / "deferred-work.md" + + assert append_entry(p, title=title, origin="o", source_spec="s.md", reason="r") == "DW-1" + + text = p.read_text(encoding="utf-8") + (entry,) = parse_ledger(text) # findable, and the id was not burned + assert entry.id == "DW-1" + assert entry.title == "(untitled DW-1)" # identifiable, not blank + assert next_seq(text) == 2 + assert len(text.splitlines()) == 6 + + +def test_append_entry_leaves_an_already_empty_title_as_it_was(tmp_path): + """The substitution is scoped to a title that *had* content and lost it to + sanitizing. An empty title in, empty title out — that hole predates this + guard, and widening the fix to cover it would change bytes for a value that + never carried a break.""" + p = tmp_path / "deferred-work.md" + + assert append_entry(p, title="", origin="o", source_spec="s.md", reason="r") == "DW-1" + + assert p.read_text(encoding="utf-8").startswith("### DW-1: \n") + + +def test_append_entry_idempotence_survives_sanitizing(tmp_path): + """Sanitizing must happen BEFORE the idempotence scan: that scan compares the + caller's value against the stored line via `field_line_present`, so + sanitizing afterwards would compare raw against sanitized and append a fresh + entry on every replay of the same defer.""" + p = tmp_path / "deferred-work.md" + dirty = "spec-foo.md\nstatus: open" + + first = append_entry( + p, title="t", origin="review-budget-followup", source_spec=dirty, reason="r" + ) + again = append_entry( + p, title="t2", origin="review-budget-followup", source_spec=dirty, reason="r2" + ) + + assert first == "DW-1" + assert again is None + assert len(parse_ledger(p.read_text(encoding="utf-8"))) == 1 + + +@pytest.mark.parametrize("status", ["", "done", "closed", "open\n### DW-99: injected", "OPEN"]) +def test_append_entry_raises_on_a_noncanonical_status_without_writing(tmp_path, status): + p = tmp_path / "deferred-work.md" + + with pytest.raises(ValueError, match="status must be"): + append_entry(p, title="t", origin="o", source_spec="s.md", reason="r", status=status) + + assert not p.exists() + + +def test_append_entry_raises_on_an_impossible_done_date_without_writing(tmp_path): + p = tmp_path / "deferred-work.md" + + with pytest.raises(ValueError, match="date must be YYYY-MM-DD"): + append_entry( + p, title="t", origin="o", source_spec="s.md", reason="r", status="done 2026-02-30" + ) + + assert not p.exists() + + +@pytest.mark.parametrize("severity", ["urgent", "blocker", "low\nseverity: high"]) +def test_append_entry_raises_on_a_noncanonical_severity_without_writing(tmp_path, severity): + """The whitelist is `SEVERITY_ALIASES`'s normalization targets. `blocker` is + an inbound alias the *parser* accepts from LLM-written ledgers, not a value + this writer may emit — so it raises like any other programmer bug.""" + p = tmp_path / "deferred-work.md" + + with pytest.raises(ValueError, match="severity must be one of"): + append_entry(p, title="t", origin="o", source_spec="s.md", reason="r", severity=severity) + + assert not p.exists() + + +def test_append_entry_accepts_every_canonical_severity(tmp_path): + """Pins the whitelist against its source: a `SEVERITY_ALIASES` value that the + writer refuses would mean the two have drifted.""" + for i, severity in enumerate(sorted(set(SEVERITY_ALIASES.values())), start=1): + p = tmp_path / f"ledger-{i}.md" + assert ( + append_entry( + p, title="t", origin="o", source_spec="s.md", reason="r", severity=severity + ) + == "DW-1" + ) + assert f"severity: {severity}" in p.read_text(encoding="utf-8") + + +def test_append_entry_leaves_a_clean_value_byte_identical(tmp_path): + p = tmp_path / "deferred-work.md" + + assert ( + append_entry( + p, + title="follow-up for dw-x", + origin="review-budget-followup", + source_spec="spec-foo.md", + reason="review budget exhausted, work committed", + location="src/foo.py:12", + severity="low", + ) + == "DW-1" + ) + + assert p.read_text(encoding="utf-8") == ( + "### DW-1: follow-up for dw-x\n" + "origin: review-budget-followup\n" + "location: src/foo.py:12\n" + "source_spec: `spec-foo.md`\n" + "severity: low\n" + "reason: review budget exhausted, work committed\n" + "status: open\n" + ) + + def test_field_line_present_matches_field_not_substring(): body = ( "### DW-1: x\norigin: review-budget-followup\n" diff --git a/tests/test_sweep.py b/tests/test_sweep.py index 8e0d1303..e079f858 100644 --- a/tests/test_sweep.py +++ b/tests/test_sweep.py @@ -35,7 +35,16 @@ StageAdapterPolicy, SweepPolicy, ) -from bmad_loop.sweep import DecisionPrompter, SweepEngine, validate_migration, validate_triage +from bmad_loop.sweep import ( + Decision, + DecisionOption, + DecisionPrompter, + ResolvedEntry, + SweepEngine, + TriagePlan, + validate_migration, + validate_triage, +) from bmad_loop.tui import launch from bmad_loop.verify import worktree_clean @@ -189,6 +198,118 @@ def test_validate_triage_bad_fields(): assert "recommendation" in joined +# --------------------------------- line breaks in ledger-bound text (#305) +# +# validate_triage deliberately does NOT gate on line breaks. The sanitizer in +# deferredwork already neutralizes them losslessly, so a rejection here would buy +# no integrity — it would only spend a triage attempt, and the second failure +# escalates the run to a human who has nothing to resolve. Acceptance is the +# behavior under test; the two live writer paths below are where it lands. + + +@pytest.mark.parametrize("field", ["evidence", "label", "resolution"]) +def test_validate_triage_accepts_multiline_free_text(field): + """A formatting-only defect must never cost a triage attempt or pause a run. + Deleting the plan over one line break trains nothing — the feedback file dies + with the session — and the escalation pages a human whose only remedy is to + re-roll the same dice.""" + injected = "fixed in abc123\n### DW-99: fake\nstatus: open" + if field == "evidence": + rj = triage_result(["DW-1"], already_resolved=[{"id": "DW-1", "evidence": injected}]) + else: + option = {"key": "1", "label": "build it", "effect": "build", "intent": "do x"} + option[field] = injected + rj = triage_result( + ["DW-1"], + decisions=[ + { + "id": "DW-1", + "question": "renegotiate?", + "context": "ctx", + "options": [option, {"key": "2", "label": "keep", "effect": "keep-open"}], + "recommendation": "1", + } + ], + ) + + plan, errors = validate_triage(rj, {"DW-1"}) + + assert errors == [] + assert plan is not None + + +def test_close_resolved_sanitizes_an_injected_evidence(project): + """First live writer path (`evidence` -> the `resolution:` note). The run + completes and the ledger keeps its shape — no raise into `_cycle`'s bare + call, which would end the sweep as crashed.""" + write_ledger(project, {"DW-1": "open", "DW-2": "open"}) + engine, _adapter = make_sweep(project, []) + before = len(ledger_entries(project)) + plan = TriagePlan( + open_ids=frozenset({"DW-1", "DW-2"}), + already_resolved=(ResolvedEntry("DW-1", "fixed\n### DW-99: fake\nstatus: open"),), + ) + + assert engine._close_resolved(plan) == 1 + + entries = ledger_entries(project) + assert len(entries) == before # no phantom entry minted + assert set(entries) == {"DW-1", "DW-2"} + assert entries["DW-1"].status.startswith("done ") + assert entries["DW-2"].open + assert "already resolved: fixed ### DW-99: fake status: open" in entries["DW-1"].body + + +@pytest.mark.parametrize( + ("resolution", "intent", "expected_detail"), + [ + ("close it\n### DW-99: fake", "unused\nintent", "close it ### DW-99: fake"), + ("", "widen the field.\nThen backfill.", "widen the field. Then backfill."), + ], + ids=["resolution-wins", "intent-when-no-resolution"], +) +def test_apply_decision_effect_sanitizes_the_ledger_detail( + project, resolution, intent, expected_detail +): + """Second live writer path, and the one that carries an option's `intent` to + the ledger. Pins `detail = option.resolution or option.intent`: the + resolution wins when present, the intent is the fallback, and either way + `append_decision` flattens it — which is why gating `intent` upstream would + prevent nothing while flattening the brief that drives a dev bundle.""" + write_ledger(project, {"DW-1": "open"}) + engine, _adapter = make_sweep(project, []) + option = DecisionOption( + key="1", label="Keep\ncap", effect="keep-open", intent=intent, resolution=resolution + ) + decision = Decision(id="DW-1", question="?", context="", options=(option,), recommendation="1") + + engine._apply_decision_effect(decision, option) + + text = project.deferred_work.read_text(encoding="utf-8") + decision_lines = [line for line in text.splitlines() if line.startswith("decision:")] + assert len(decision_lines) == 1 + assert decision_lines[0].endswith(f"Keep cap — {expected_detail}") + assert ledger_entries(project)["DW-1"].open # keep-open does not close + + +def test_apply_decision_effect_close_sanitizes_the_close_note(project): + write_ledger(project, {"DW-1": "open"}) + engine, _adapter = make_sweep(project, []) + option = DecisionOption( + key="1", label="Close", effect="close", resolution="superseded\n### DW-99: fake" + ) + decision = Decision(id="DW-1", question="?", context="", options=(option,), recommendation="1") + + engine._apply_decision_effect(decision, option) + + entries = ledger_entries(project) + assert set(entries) == {"DW-1"} # no phantom entry from the close note + assert entries["DW-1"].status.startswith("done ") + assert ( + "resolution: closed by human decision: superseded ### DW-99: fake" in entries["DW-1"].body + ) + + def test_validate_triage_unknown_id(): rj = triage_result( ["DW-1"],