From eaadd15c6bb469b51ea7be633aa6cae99913fc64 Mon Sep 17 00:00:00 2001 From: pbean Date: Mon, 27 Jul 2026 08:34:14 -0700 Subject: [PATCH 1/2] fix(deferredwork): sanitize free text at the ledger writer chokepoint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- CHANGELOG.md | 26 ++ src/bmad_loop/cli.py | 11 +- .../skills/bmad-loop-sweep/automation-mode.md | 15 + .../bmad-loop-sweep/deferred-work-format.md | 16 + src/bmad_loop/decisions.py | 8 +- src/bmad_loop/deferredwork.py | 164 +++++++- src/bmad_loop/tui/app.py | 7 +- tests/test_cli.py | 28 ++ tests/test_decisions.py | 53 +++ tests/test_deferredwork.py | 362 ++++++++++++++++++ tests/test_sweep.py | 123 +++++- 11 files changed, 805 insertions(+), 8 deletions(-) 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..fd512ce3 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -1496,7 +1496,16 @@ 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, ValueError) as e: + # ValueError is the ledger writers' `date` precondition. It cannot + # fire from the strftime above, but the TUI's copy of this call + # degrades to a per-decision notification, and the same programmer + # bug must not instead surface here as an argparse traceback — this + # is the UI a human is more likely answering from. + 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..e46915a9 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,94 @@ 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; the callers that need a non-empty result supply something + identifiable instead (see :func:`append_entry`, :func:`append_decision`).""" + 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 +333,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 +362,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 +427,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 +456,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 +518,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..e48c48ec 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -553,6 +553,34 @@ def ask(self, decision): assert decisions.pending_missed_decisions(project.project) == [] +def test_decisions_reports_a_bad_date_instead_of_a_traceback(project, capsys, monkeypatch): + """`apply_pre_answer`'s `date` precondition raises `ValueError`. The TUI's copy + of this call degrades to a per-decision notification; this path must degrade + too, rather than letting the same programmer bug reach argparse as a + traceback in the UI a human is more likely answering from.""" + 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_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"], From 7416149a37a0c141ec908b9169e65fd9554424ef Mon Sep 17 00:00:00 2001 From: pbean Date: Mon, 27 Jul 2026 09:26:49 -0700 Subject: [PATCH 2/2] fix(cli): attribute a failed pre-answer to its decision (#305 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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-` in that message — the loop has already printed an outcome line per answered decision, and a bare `error: ` 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-)`, 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`. --- src/bmad_loop/cli.py | 23 ++++++++++++---- src/bmad_loop/deferredwork.py | 11 ++++++-- tests/test_cli.py | 52 +++++++++++++++++++++++++++++++---- 3 files changed, 73 insertions(+), 13 deletions(-) diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index fd512ce3..65d1b77a 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -1498,12 +1498,23 @@ def cmd_decisions(args: argparse.Namespace) -> int: option = prompter.ask(decision) try: decisions.apply_pre_answer(project, decision, option, date=today) - except (OSError, ValueError) as e: - # ValueError is the ledger writers' `date` precondition. It cannot - # fire from the strftime above, but the TUI's copy of this call - # degrades to a per-decision notification, and the same programmer - # bug must not instead surface here as an argparse traceback — this - # is the UI a human is more likely answering from. + 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": diff --git a/src/bmad_loop/deferredwork.py b/src/bmad_loop/deferredwork.py index e46915a9..566d55da 100644 --- a/src/bmad_loop/deferredwork.py +++ b/src/bmad_loop/deferredwork.py @@ -248,8 +248,15 @@ def _one_line(value: str) -> str: 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; the callers that need a non-empty result supply something - identifiable instead (see :func:`append_entry`, :func:`append_decision`).""" + 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() diff --git a/tests/test_cli.py b/tests/test_cli.py index e48c48ec..940689bb 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -553,11 +553,15 @@ def ask(self, decision): assert decisions.pending_missed_decisions(project.project) == [] -def test_decisions_reports_a_bad_date_instead_of_a_traceback(project, capsys, monkeypatch): - """`apply_pre_answer`'s `date` precondition raises `ValueError`. The TUI's copy - of this call degrades to a per-decision notification; this path must degrade - too, rather than letting the same programmer bug reach argparse as a - traceback in the UI a human is more likely answering from.""" +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 @@ -581,6 +585,44 @@ def ask(self, decision): 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