fix(deferredwork): atomic ledger writes + surrogate-safe sanitizer (#328, #329) - #580
Conversation
…h atomic_write_text (#328) `Path.write_text` opens 'w' (truncate) and only THEN encodes, so any failure in that window — an unencodable value, ENOSPC, EIO — left a zero-byte ledger with every deferred-work entry gone. `mark_done_many` already wrote through `atomic_write_text`; its siblings did not, and an asymmetric guard between siblings is itself unreadable. Five write sites now build their replacement beside the target and replace atomically, so a failure raises with the original intact: - deferredwork.append_decision — the decision record - deferredwork.append_entry — the new-entry append (creates the ledger) - engine._defer — the post-rollback ledger restore - sweep._ensure_migration — the migration-failure ledger restore - sweep._write_intent — the run-dir bundles/<dir>/intent.md write Visible diff beyond durability: a ledger `append_entry` creates from nothing now lands mode 0600 (mkstemp's private mode) instead of the umask default — deliberate, per the helper's docstring, and the safer end for a file that can carry unreleased-finding text. The call sites stay bare: repair writes must raise, never degrade.
…the sanitizer chokepoint (#329) `_one_line` collapses line breaks and documents a stronger contract than it delivered — "Sanitizes; never raises". A lone surrogate defeated that without ever being a line break: it has no UTF-8 encoding at all, so `atomic_write_text`'s strict encode raises `UnicodeEncodeError`, a `ValueError` subclass, from inside the close paths that call these writers bare. That is precisely the outcome the sanitizer exists to prevent. The revival chain is real and reachable, not theoretical: - `sweep._ensure_triage` caches the session's result.json with `json.dumps`, whose `ensure_ascii` keeps the code point a harmless `\ud800` escape on disk - a later cycle in the same method reloads it and `json.loads` revives the actual surrogate - `validate_triage` passes it (`.strip()` does not remove it) into `ResolvedEntry.evidence` - `_close_resolved` interpolates that into the `mark_done` note, which reaches `_one_line` and then the encode `neutralize_surrogates` lands next to `atomic_write_text`, whose strict encode it exists to protect, and `_one_line` runs it first — so every free-text ledger field is covered at one chokepoint (`append_decision` label/detail, `append_entry` title/origin/source_spec/reason/location, `_apply_done` note), and the ledger is strict-UTF-8-read, so nothing already on disk can carry one in. `sweep._write_intent` gets the same pass over the whole document rather than per field: it writes run-dir intent.md with no sanitization today, and the same triage prose reaches it. Line breaks are deliberately kept there — that file is markdown, so `_one_line`'s collapse would be damage. The verbatim ledger blocks it re-attaches are read from a strict-UTF-8 file and pass through byte-unchanged. Each surrogate becomes U+FFFD, not the `?` that `encode("utf-8", "replace")` would yield: the replacement stays visible and unambiguous, where a `?` is indistinguishable from one the author typed and a strip would let the value vanish silently. A surrogate-only title therefore stays truthy, so `append_entry`'s `(untitled DW-<n>)` substitution deliberately does not fire for it. Sanitizing stays in front of the idempotence scan, so a replayed defer whose origin carries a surrogate still matches the stored line instead of burning a fresh id. Refusing the text upstream at `validate_triage` was the alternative and only moves the stoppage to a pause — the same doctrine #305 settled on, so the same answer.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
WalkthroughThe change adds centralized lone-surrogate replacement and switches deferred-work ledger and bundle-intent persistence to atomic writes. Regression tests cover encoding failures, filesystem failures, data preservation, line-break handling, and migration restoration. ChangesDeferred-work persistence
Estimated code review effort: 3 (Moderate) | ~20 minutes Mergeability Score: ⚪ Minimal · up to The change is merge-ready after normal checks and review; no actionable merge-blocking risk remains. Possibly related issues
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
CI note, so the red-then-green history on this PR is not mistaken for a fixed defect That test is unrelated to this diff (this branch touches no TUI code; All 10 checks are green as of |
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Closes #328
Closes #329
Problem
Two hazards on the same code path, and they compound.
#328 — a failed write empties the ledger.
Path.write_textopens the file'w'(truncate) and only then encodes and writes. Any failure inside that window — an
unencodable value,
ENOSPC,EIO— leaves a zero-byte file where every hand-authoreddeferred-work entry used to be.
mark_done_manyalready wrote throughatomic_write_text; its siblings did not, and an asymmetric guard between siblings isitself unreadable.
#329 — a lone surrogate defeats the sanitizer's never-raises contract.
_one_linecollapses line breaks and documents "Sanitizes; never raises". A lonesurrogate is not a line break, so it sailed through untouched — and it has no UTF-8
encoding at all, so
atomic_write_text's strict encode raisesUnicodeEncodeError(a
ValueErrorsubclass) from close paths that call these writers bare. The revivalchain is reachable, not theoretical:
sweep._ensure_triagecaches the session'sresult.jsonwithjson.dumps, whoseensure_asciikeeps the code point a harmless\ud800escape on disk; a later cycle reloads it andjson.loadsrevives the realsurrogate;
validate_triagepasses it through (.strip()does not remove it) intoResolvedEntry.evidence;_close_resolvedinterpolates that into themark_donenote, which reaches
_one_lineand then the encode.The coupling. Before this branch, that same surrogate through an append writer did
both at once: the truncate had already landed when the encode raised, so the run
crashed and the ledger it was appending to went to zero bytes in a single call. #328
alone would keep the bytes but still crash the sweep; #329 alone would stop the crash
but leave every other failure mode (
ENOSPC,EIO) able to empty the file. Fixing onewithout the other leaves half the failure standing, which is why they ship together.
tests/test_deferredwork.py::test_append_entry_encode_failure_cannot_truncate_the_ledgerpins exactly that compounding.
Fix
src/bmad_loop/platform_util.py— newneutralize_surrogates(text): replaces everycode point in
[\ud800-\udfff]with U+FFFD. Matched as a code-point range, not by anencode round trip — a Python
strholds astral characters as one code point, so U+1D11Eis never touched. Replace rather than strip (a vanished value reads as absent, not as
damaged) and rather than
encode("utf-8", "replace")(that yields?, indistinguishablefrom one the author typed). Clean text returns the identical object, so a clean write
stays byte-identical. It lands next to
atomic_write_text, the strict encode it existsto protect.
src/bmad_loop/deferredwork.py—_one_linerunsneutralize_surrogatesfirst, soevery free-text ledger field is covered at one chokepoint:
append_decision'slabel/detail,append_entry'stitle/origin/source_spec/reason/location,and
_apply_done'snote. Both passes keep their own fast path, so a clean value isscanned twice and copied never. Sanitizing stays in front of the idempotence scan, or a
replayed defer whose origin carries a surrogate would match nothing on disk and burn a
fresh id every run.
append_decisionandappend_entrynow write throughatomic_write_text, matchingmark_done_many.src/bmad_loop/engine.py—_defer's post-rollback ledger restore writes throughatomic_write_text. A repair write must never be the thing that empties the file itexists to put back.
src/bmad_loop/sweep.py—_ensure_migration's migration-failure ledger restorewrites through
atomic_write_text._write_intentwritesbundles/<dir>/intent.mdthrough
atomic_write_textand neutralizes surrogates over the whole document ratherthan per field — line breaks are legitimate markdown there, so
_one_line's collapsewould be damage. The verbatim ledger blocks it re-attaches come from a strict-UTF-8 read
and pass through byte-unchanged.
Call sites stay bare — repair writes raise, never degrade.
Scope note
#328 names the
append_decision/append_entryasymmetry againstmark_done_many. Thetwo engine/sweep restore writes and the
intent.mdwrite were folded in with maintainerapproval: same hazard class, same one-line remedy, and leaving them out would have
recreated the asymmetry the issue is about one layer over. Five write sites in total,
enumerated in the Fix section. Nothing else in the diff.
Deliberately not touched: the
json.dumpswrites insweep.py(manifest,migrate-result.json, cachedresult.json, decision answers). Those are ASCII-only byensure_asciiand are machine-minted caches, not operator-curated state — that is thevery property that lets a surrogate hide on disk as an escape in the first place.
Visible behavior diffs
append_entrynow lands mode0600instead of0644 & ~umask.atomic_write_texthas no prior mode to carry over for a freshtarget, so it keeps
mkstemp's private mode — deliberate, per that helper's docstring,and the safer end for a file that can carry unreleased-finding text. Verified on this
box under
umask 022:oct(stat.S_IMODE(...)) == '0o600'.�(REPLACEMENT CHARACTER) in ledger fields and inintent.mdrather than crashing the write. A surrogate-only title therefore staystruthy, so
append_entry's(untitled DW-<n>)substitution deliberately does notfire for it — the field says something unencodable was here instead of going silently
blank.
atomic_write_textkeepswrite_text's translating-newline default, so line endingsdo not shift on Windows.
Scope of the durability claim: the ledger writers are now crash-free on text content.
That covers
append_decision,append_entry, and themark_done/mark_done_many/mark_done_many_reopenablefamily — every caller-supplied free-text field theyinterpolate routes through
_one_line— plusmark_open, which interpolates no callertext at all (it writes back a status line it decoded from hex under a strict UTF-8
decode). It is not a claim about all close paths or all writes in the orchestrator:
the JSON caches above and every writer outside
deferredwork.pyare untouched andunaudited here.
Testing
Full suite green locally (
uv run pytest -q -n auto: 5388 passed, 44 skipped, 5 xfailed),uv run pyright0 errors,trunk check --allclean.Every guard below was ablated — the fix reverted in place, the target run, the file
restored from a
cpbackup (nevergit checkout, which would have taken the rest of thebranch with it). One line of evidence each; the tree was verified pristine afterwards.
#328 — atomic writes
test_deferredwork.py::test_append_decision_write_failure_raises_and_keeps_the_ledgerappend_decision→ barewrite_text:Failed: DID NOT RAISE OSError(the write no longer routes through the patched helper).test_deferredwork.py::test_append_entry_write_failure_raises_and_keeps_the_ledgerappend_entry→ barewrite_text:Failed: DID NOT RAISE OSError.test_deferredwork.py::test_append_entry_encode_failure_cannot_truncate_the_ledgerassert b'' == b'# Deferred ...pec-2-3.md)\n'— the raise still happens, the bytes are what reddens.test_engine.py::test_defer_ledger_restore_write_failure_propagates_and_keeps_the_ledger_deferrestore → barewrite_text:assert (False) … crashed=False— the failure never reaches the run.test_sweep.py::test_migration_restore_write_failure_propagates_and_keeps_the_ledger_ensure_migrationrestore → barewrite_text:assert (False) … crashed=False. Compares the restored ledger as text, not bytes — the fixture reaches disk viawrite_text, so a byte assertion would read CRLF and redden on Windows only.#329 — surrogate neutralization. Wiring and predicate are separate ablation axes and
were ablated separately.
test_platform_util.py::test_neutralize_surrogates_replaces_only_lone_surrogates(10 rows)UnicodeEncodeError;clean,empty,astral-untouched,mixed-non-ascii,range-boundariesstay green, which is what proves the range is not over-broad.test_platform_util.py::test_neutralize_surrogates_returns_clean_text_untouchedis-identity fast path, so a clean ledger write stays byte-identical.test_platform_util.py::test_neutralize_surrogates_makes_atomic_write_text_survive_a_surrogateUnicodeEncodeError … surrogates not allowed.test_deferredwork.py::test_append_entry_writes_a_finding_carrying_a_lone_surrogateneutralize_surrogatescall from_one_line:UnicodeEncodeError: 'utf-8' codec can't encode character '\ud800'. No monkeypatching in the test — the value survives a real writer, a real strict encode and a real strict read back.test_deferredwork.py::test_append_decision_writes_a_label_and_detail_carrying_a_lone_surrogateUnicodeEncodeError.test_deferredwork.py::test_mark_done_writes_a_note_carrying_a_lone_surrogateUnicodeEncodeError. Uses the exact stringsweep._close_resolvedbuilds.test_deferredwork.py::test_append_entry_idempotence_survives_sanitizing(3 rows)line-breakrow stays green — sanitizing must stay ahead of the idempotence scan for both passes, not just the older one.test_sweep.py::test_write_intent_neutralizes_surrogates_and_keeps_the_markdownneutralize_surrogatesfrom_write_intent:UnicodeEncodeError. Also assertskeep�this\n\nsecond para— the paragraph break either side survives, so the document-wide pass is not collapsing markdown.No POSIX-only assertions were added. The
0600mode diff is asserted nowhere in thesuite (mode semantics differ on Windows); it is documented above from a local probe and
in
atomic_write_text's docstring.Summary by CodeRabbit