fix(deferredwork): atomic ledger writes + surrogate-safe sanitizer (#328, #329)

CHANGELOG.md

@@ -322,6 +322,24 @@ whose seams had diverged enough that several ports needed a different fix, and t
 
 ### Fixed
 
+- **A failed ledger write can no longer empty the deferred-work ledger (#328).** `Path.write_text`
+  truncates the file and only then encodes, so any failure in that window — an unencodable value,
+  `ENOSPC`, `EIO` — left a zero-byte ledger with every entry gone. `append_decision`,
+  `append_entry`, the post-rollback and migration-failure restores, and the bundle intent write now
+  go through `atomic_write_text` like their `mark_done_many` sibling: the replacement is built
+  beside the target, so a failure raises with the original untouched. A ledger `append_entry`
+  creates from nothing now lands `0600` (the helper's private temp mode) rather than the umask
+  default.
+
+- **A lone surrogate in triage text no longer crashes the sweep (#329).** A cached triage
+  `result.json` stores `\ud800` as an escape; the reload's `json.loads` revives the real code point
+  into an entry's evidence and on into the ledger note. It is not a line break, so the sanitizer
+  passed it through untouched, and it has no UTF-8 encoding at all — the strict encode raised
+  `UnicodeEncodeError` from a close path that calls the writers bare. Every free-text ledger field
+  is now neutralized at that same chokepoint, and the bundle `intent.md` gets the same pass over the
+  whole document (its line breaks are legitimate markdown, so only surrogates are touched). Each
+  becomes U+FFFD `�`, so the text stays visible rather than vanishing.
+
 - **A `#` inside a quoted sprint-status value is no longer rewritten into a comment (#366).** The
   board writer split value from inline comment with one fused pattern, so `3-2-x: "a # b"` advanced
   to `3-2-x: done # b"` — scalar text promoted into a comment the board never had. The split is now

src/bmad_loop/deferredwork.py

@@ -24,7 +24,7 @@
 
 from . import sprintstatus
 from .fences import fenced_spans
-from .platform_util import atomic_write_text
+from .platform_util import atomic_write_text, neutralize_surrogates
 
 HEADING_RE = re.compile(r"^### (DW-\d+): (.+?)\s*$", re.MULTILINE)
 ANY_HEADING_RE = re.compile(r"^#{1,6} ", re.MULTILINE)
@@ -618,11 +618,24 @@ def _one_line(value: str) -> str:
     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.
+    That contract covers one hazard more than the break collapse alone, which is
+    what the `neutralize_surrogates` pass in front of it buys (#329). A lone
+    surrogate is not a line break, so it sailed through untouched — but it has
+    no UTF-8 encoding, and `atomic_write_text`'s strict encode raises
+    `UnicodeEncodeError` (a `ValueError` subclass) on it, from inside those same
+    bare close-path calls. It arrives the way the break did: a triage
+    `result.json` is cached with `json.dumps`, whose `ensure_ascii` keeps the
+    code point a harmless `\\ud800` escape, and the reload's `json.loads` revives
+    the real thing into `ResolvedEntry.evidence` and on into the `mark_done`
+    note. Refusing it upstream would only move the stoppage again — same
+    doctrine, same answer.
+
+    A value with neither a break nor a surrogate is returned **untouched**, so an
+    existing ledger is never reformatted and a clean write is byte-identical to
+    before the guard; each pass keeps its own fast path, so the common value is
+    scanned twice and copied never. 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
@@ -634,7 +647,13 @@ def _one_line(value: str) -> str:
     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."""
+    break-only label has already become the option key before it arrives.
+
+    A surrogate-only value, by contrast, sanitizes to a truthy `"�"`, so neither
+    caller's empty-handling fires for it. That is the point of replacing rather
+    than stripping: a title reading `�` still says *something unencodable was
+    here*, where a vanished one would silently become `(untitled DW-<n>)`."""
+    value = neutralize_surrogates(value)
     if not LINE_BREAK_RE.search(value):
         return value
     return LINE_BREAK_RE.sub(" ", value).strip()
@@ -875,7 +894,13 @@ def append_decision(path: Path, dw_id: str, date: str, label: str, detail: str)
 
     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."""
+    the bug.
+
+    The write goes through :func:`~bmad_loop.platform_util.atomic_write_text` for
+    the reasons documented on :func:`mark_done_many`, plus one this sibling shares
+    with it: a bare ``Path.write_text`` truncates *before* it encodes, so any
+    failure between the two — an unencodable value, ``ENOSPC``, ``EIO`` — leaves a
+    zero-byte ledger where every entry used to be (#328)."""
     _require_iso_date(date)
     if not path.is_file():
         return False
@@ -890,7 +915,7 @@ def append_decision(path: Path, dw_id: str, date: str, label: str, detail: str)
     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")
+    atomic_write_text(path, text)
     return True
 
 
@@ -938,7 +963,13 @@ def append_entry(
     :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."""
+    orchestrator-owned enumerations and raise instead.
+
+    The write goes through :func:`~bmad_loop.platform_util.atomic_write_text` for
+    the reasons documented on :func:`mark_done_many`, plus one this sibling shares
+    with it: a bare ``Path.write_text`` truncates *before* it encodes, so any
+    failure between the two — an unencodable value, ``ENOSPC``, ``EIO`` — leaves a
+    zero-byte ledger where every entry used to be (#328)."""
     _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
@@ -993,7 +1024,7 @@ def append_entry(
     else:
         sep = "\n\n"
     path.parent.mkdir(parents=True, exist_ok=True)
-    path.write_text(text + sep + block, encoding="utf-8")
+    atomic_write_text(path, text + sep + block)
     return dw_id
 
 

src/bmad_loop/engine.py

@@ -5152,7 +5152,7 @@ def _defer(self, task: StoryTask, reason: str) -> None:
                 )
                 if current != snapshot:
                     deferred_work.parent.mkdir(parents=True, exist_ok=True)
-                    deferred_work.write_text(snapshot, encoding="utf-8")
+                    atomic_write_text(deferred_work, snapshot)
             # The restore deliberately keeps review-found ledger knowledge, but
             # it also replays this bundle's accepted close after the code was
             # discarded. Let the mode undo only the close it can identify as its

src/bmad_loop/platform_util.py

@@ -325,6 +325,38 @@ def _copy_xattrs(src: Path, dst: Path) -> None:
             continue
 
 
+# Matched as a code-point range rather than by an encode round trip, the same way
+# ``engine._TITLE_CONTROL_RE`` reaches these: a Python ``str`` holds code points,
+# so an astral character like U+1D11E is one code point *outside* this range and
+# is never touched. Only genuinely lone surrogates match.
+_SURROGATES_RE = re.compile(r"[\ud800-\udfff]")
+
+
+def neutralize_surrogates(text: str) -> str:
+    """Replace every lone surrogate in ``text`` with U+FFFD (``�``).
+
+    A surrogate is a legal ``str`` code point with **no UTF-8 encoding at all**,
+    so any strict encode — :func:`atomic_write_text`'s included — raises
+    ``UnicodeEncodeError`` on one. That is a ``ValueError`` subclass, which is
+    how a single unpaired code point reaches a caller as a crash rather than as
+    mangled text. They arrive from anywhere a decoder is allowed to mint them:
+    ``json.loads`` reviving a ``\\ud800`` escape, a double-quoted YAML scalar, a
+    ``surrogateescape`` decode of undecodable filesystem bytes.
+
+    Replace, not strip and not refuse. U+FFFD keeps the value **visible** — the
+    field still says *something unencodable was here* — where dropping the code
+    point would let it vanish silently and refusing would only move the stoppage
+    upstream. It is also why this is a substitution rather than the shorter
+    ``text.encode("utf-8", "replace").decode("utf-8")``: that spelling yields
+    ``"?"``, indistinguishable from a question mark the author actually typed.
+
+    Text with no surrogate is returned untouched — the identical object, so a
+    clean write stays byte-identical."""
+    if not _SURROGATES_RE.search(text):
+        return text
+    return _SURROGATES_RE.sub("�", text)
+
+
 def atomic_write_text(path: Path, text: str, *, follow_symlinks: bool = True) -> None:
     """Replace ``path``'s contents with ``text`` atomically, preserving what the
     replacement would otherwise silently discard.

src/bmad_loop/sweep.py

@@ -21,7 +21,7 @@
 from .engine import Engine
 from .escalation import critical_escalations, env_fault_pause_reason, session_failure_reason
 from .model import Phase, StoryTask
-from .platform_util import atomic_replace
+from .platform_util import atomic_replace, atomic_write_text, neutralize_surrogates
 from .statemachine import advance
 from .workspace import discard_worktree
 
@@ -793,7 +793,7 @@ def _ensure_migration(self, text: str) -> None:
             # ledger that `git reset` cannot restore
             self._safe_reset(task)
             ledger.parent.mkdir(parents=True, exist_ok=True)
-            ledger.write_text(text, encoding="utf-8")
+            atomic_write_text(ledger, text)
             if task.attempt >= self.policy.sweep.max_migration_attempts:
                 self._escalate(
                     task, "migration failed deterministic validation: " + "; ".join(errors)
@@ -1152,7 +1152,13 @@ def _write_intent(self, bundle: Bundle, dirname: str) -> Path:
         lines += ["", "## Ledger entries (verbatim)", "", "\n\n".join(blocks), ""]
         path = self.run_dir / "bundles" / dirname / "intent.md"
         path.parent.mkdir(parents=True, exist_ok=True)
-        path.write_text("\n".join(lines), encoding="utf-8")
+        # Surrogates are neutralized over the whole document, not per field: the
+        # triage-authored `intent`/`decision_note` are the ones that can revive one
+        # (#329), but a document-wide pass covers whatever prose is added here
+        # later. Line breaks are deliberately *kept* — this file is markdown, so
+        # `_one_line`'s collapse would be damage, and the ledger blocks are read
+        # back from a strict-UTF-8 file and so pass through byte-unchanged.
+        atomic_write_text(path, neutralize_surrogates("\n".join(lines)))
         return path
 
     def _ensure_bundle_intent(self, task: StoryTask) -> None:

tests/test_deferredwork.py

@@ -467,6 +467,23 @@ def test_append_decision_missing_file(tmp_path):
     assert not mark_done(tmp_path / "nope.md", "DW-1", "2026-06-11", "x")
 
 
+def test_append_decision_write_failure_raises_and_keeps_the_ledger(tmp_path, monkeypatch):
+    """#328. `Path.write_text` opens `'w'` (truncate) and only THEN encodes, so a
+    failure anywhere in that window left the whole ledger at zero bytes. The
+    atomic helper builds the replacement beside the target, so a raise leaves the
+    original exactly as it was."""
+    path = write_ledger(tmp_path)
+    before = path.read_bytes()
+
+    def boom(path, text):
+        raise OSError("disk full")
+
+    monkeypatch.setattr(deferredwork, "atomic_write_text", boom)
+    with pytest.raises(OSError, match="disk full"):
+        append_decision(path, "DW-3", "2026-06-11", "Keep cap", "frozen intent stands")
+    assert path.read_bytes() == before
+
+
 # ------------------------------------------------- line-break injection (#305)
 #
 # The ledger is line-oriented and every mutator interpolates its arguments, so a
@@ -1259,26 +1276,83 @@ def test_append_entry_leaves_an_already_empty_title_as_it_was(tmp_path):
     assert p.read_text(encoding="utf-8").startswith("### DW-1: \n")
 
 
-def test_append_entry_idempotence_survives_sanitizing(tmp_path):
+@pytest.mark.parametrize(
+    ("origin", "source_spec"),
+    [
+        ("review-budget-followup", "spec-foo.md\nstatus: open"),
+        ("review\ud800followup", "spec-foo.md"),
+        ("review-budget-followup", "spec-\udfff-foo.md"),
+    ],
+    ids=["line-break", "surrogate-origin", "surrogate-source-spec"],
+)
+def test_append_entry_idempotence_survives_sanitizing(tmp_path, origin, source_spec):
     """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."""
+    entry on every replay of the same defer.
+
+    Both sanitizing passes are in front of that scan, so both are covered here.
+    The surrogate rows would otherwise regress the same way the break row does —
+    a replayed defer whose origin carries a `\\ud800` matches nothing on disk
+    (the ledger stores the U+FFFD) and burns a fresh id every run."""
     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"
-    )
+    first = append_entry(p, title="t", origin=origin, source_spec=source_spec, reason="r")
+    again = append_entry(p, title="t2", origin=origin, source_spec=source_spec, reason="r2")
 
     assert first == "DW-1"
     assert again is None
     assert len(parse_ledger(p.read_text(encoding="utf-8"))) == 1
 
 
+# ------------------------- surrogate neutralization at the sanitizer chokepoint (#329)
+# Every row here calls a real writer with no monkeypatching: the value has to
+# survive `_one_line`, `atomic_write_text`'s strict UTF-8 encode, and the strict
+# read back. Delete the `neutralize_surrogates` call in `_one_line` and each one
+# fails with `UnicodeEncodeError` — the crash #329 filed, arriving from a close
+# path that calls these writers bare.
+
+
+def test_append_entry_writes_a_finding_carrying_a_lone_surrogate(tmp_path):
+    """A lone surrogate is not a line break, so it sailed past the break collapse
+    untouched and detonated in the encode. Note the title stays truthy — `�` is a
+    visible replacement, so `append_entry`'s `(untitled DW-<n>)` substitution
+    deliberately does NOT fire for it."""
+    p = tmp_path / "deferred-work.md"
+
+    dw_id = append_entry(p, title="\ud800", origin="o", source_spec="s.md", reason="x\udfffy")
+
+    assert dw_id == "DW-1"
+    text = p.read_text(encoding="utf-8")  # strict read: unencodable text never got here
+    (entry,) = parse_ledger(text)
+    assert entry.title == "�"  # replaced, not vanished into `(untitled DW-1)`
+    assert "reason: x�y" in entry.body
+
+
+def test_append_decision_writes_a_label_and_detail_carrying_a_lone_surrogate(tmp_path):
+    """`decisions.apply_pre_answer` calls this bare, so the raise would end the
+    sweep. Both interpolated fields go through the chokepoint."""
+    path = write_ledger(tmp_path)
+
+    assert append_decision(path, "DW-3", "2026-06-11", "\ud800", "a\ud800b")
+
+    entries = {e.id: e for e in parse_ledger(path.read_text(encoding="utf-8"))}
+    assert "decision: 2026-06-11 � — a�b" in entries["DW-3"].body
+
+
+def test_mark_done_writes_a_note_carrying_a_lone_surrogate(tmp_path):
+    """The exact string `sweep._close_resolved` builds — `f"already resolved:
+    {entry.evidence}"` — where `evidence` is the field the cached triage JSON
+    revives a surrogate into."""
+    path = write_ledger(tmp_path)
+
+    assert mark_done(path, "DW-1", "2026-06-11", "already resolved: \ud800evidence")
+
+    entries = {e.id: e for e in parse_ledger(path.read_text(encoding="utf-8"))}
+    assert entries["DW-1"].status == "done 2026-06-11"
+    assert "resolution: already resolved: �evidence" in entries["DW-1"].body
+
+
 @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"
@@ -1354,6 +1428,55 @@ def test_append_entry_leaves_a_clean_value_byte_identical(tmp_path):
     )
 
 
+def test_append_entry_write_failure_raises_and_keeps_the_ledger(tmp_path, monkeypatch):
+    """#328, the `append_entry` half — see the `append_decision` twin above."""
+    path = write_ledger(tmp_path)
+    before = path.read_bytes()
+
+    def boom(path, text):
+        raise OSError("disk full")
+
+    monkeypatch.setattr(deferredwork, "atomic_write_text", boom)
+    with pytest.raises(OSError, match="disk full"):
+        append_entry(
+            path,
+            title="new finding",
+            origin="review of spec-foo.md",
+            source_spec="spec-foo.md",
+            reason="out of scope",
+        )
+    assert path.read_bytes() == before
+
+
+def test_append_entry_encode_failure_cannot_truncate_the_ledger(tmp_path, monkeypatch):
+    """#328's worst case, reached without any injected OSError: an unencodable
+    value raises from inside the encode step itself. Under a bare `write_text`
+    the file is already truncated by then, so the raise and the data loss arrive
+    together — the exact compounding #329 describes."""
+    path = write_ledger(tmp_path)
+    before = path.read_bytes()
+    # Patching the sanitizer to the identity is now LOAD-BEARING: since #329,
+    # `_one_line` neutralizes surrogates, so without this patch the value would
+    # reach the write already encodable and nothing would raise. Keeping it pins
+    # the WRITE layer's defense independently of the sanitizer's — without it
+    # this row would quietly stop testing #328 and become a second test of
+    # #329's fix. Do not remove it.
+    monkeypatch.setattr(deferredwork, "_one_line", lambda v: v)
+
+    with pytest.raises(UnicodeEncodeError):
+        append_entry(
+            path,
+            title="\ud800",
+            origin="review of spec-foo.md",
+            source_spec="spec-foo.md",
+            reason="out of scope",
+        )
+
+    # The raise is NOT the invariant under test — a bare `write_text` raises here
+    # too. The bytes are: this is the assertion that reddens without the fix.
+    assert path.read_bytes() == before
+
+
 def test_field_line_present_matches_field_not_substring():
     body = (
         "### DW-1: x\norigin: review-budget-followup\n"