From 94de03e177a9735dfa78fc4f7faa3d9b59fe1353 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 26 Aug 2026 10:01:58 -0700 Subject: [PATCH 01/13] fix(sprintstatus): answer a no-op advance before taking the board lock (#736) --- src/bmad_loop/sprintstatus.py | 89 ++++++++++-- tests/test_sprintstatus_advance.py | 225 +++++++++++++++++++++++++++-- 2 files changed, 288 insertions(+), 26 deletions(-) diff --git a/src/bmad_loop/sprintstatus.py b/src/bmad_loop/sprintstatus.py index 5d44f7e6..97d5675c 100644 --- a/src/bmad_loop/sprintstatus.py +++ b/src/bmad_loop/sprintstatus.py @@ -11,9 +11,13 @@ TUI) runs the same sole writer, and :func:`advance` is a read-modify-write of the whole board, so two of them would both read, both edit, and let the last atomic write win. :func:`advance` therefore serializes itself cross-process on the -board's state-root sidecar lock, holding it across every read AND the write. -Readers stay lock-free: the publish is an atomic replace, so a reader sees either -the old board entire or the new one. +board's state-root sidecar lock, and holds it across every read that decides the +PUBLISHED BYTES as well as the write itself. That invariant is deliberately +narrower than "every read": one advisory pre-lock probe may answer a +read-dependent no-op — an absent row, or a row already at or past target — +without acquiring at all (#736), because such a call publishes nothing and so has +no bytes for the hold to protect. Readers stay lock-free: the publish is an +atomic replace, so a reader sees either the old board entire or the new one. """ from __future__ import annotations @@ -270,6 +274,30 @@ def _board_lock(path: Path) -> Iterator[None]: yield +def _row_at_or_past(current: str, target: str) -> bool: + """Is a row at ``current`` already at or past ``target`` in :data:`STATUS_ORDER`? + + The never-regress comparison :func:`_advance_locked` makes, factored out so + that :func:`advance`'s advisory pre-lock probe and the authoritative locked + decision run one body and cannot drift apart (#736). A probe that answered + this question even slightly differently from the writer would either skip a + write the board needed or take a lock it did not. + + Deliberately NOT :func:`~bmad_loop.engine._at_or_past`, the reader-side twin: + that one counts an exact match OUTSIDE ``STATUS_ORDER`` as reached, which is + right for reading what :func:`advance` RETURNED and wrong as input to its + WRITE decision. An off-order status equal to ``target`` is a no-op owned by + :func:`_set_mapping_value` under the lock — it refuses a value it already + holds — and routing it through this predicate instead would hand the answer + to a pre-lock probe on a comparison the writer does not make. + """ + return ( + current in STATUS_ORDER + and target in STATUS_ORDER + and STATUS_ORDER.index(current) >= STATUS_ORDER.index(target) + ) + + def advance(path: Path, story_key: str, target: str, *, now: str | None = None) -> str | None: """Advance a story's sprint-status to `target` for the generic-skill path. @@ -321,9 +349,31 @@ def advance(path: Path, story_key: str, target: str, *, now: str | None = None) intra-call TOCTOU, since the never-regress decision and the bytes it is applied to now come from one hold rather than from two independent reads. - The missing-board check runs BEFORE the lock, so asking about a board that - does not exist leaves no sidecar behind; the locked half rechecks, deletion - being able to race the pre-lock answer. Acquisition failure surfaces as + Two answers are reached BEFORE the lock. The missing-board check runs first, + so asking about a board that does not exist leaves no sidecar behind. Then an + ADVISORY probe (#736) reads the row once and answers the two cases in which + this call would write nothing at all: an absent row (``None``) and a row + already at or past ``target`` (the current status, via :func:`_row_at_or_past` + — the same predicate the locked body applies). Acquiring for those was the + defect: an idempotent replay — ``bmad-loop confirm`` against a story the board + already records as done is a designed path, not an error + (:meth:`~bmad_loop.model.ParkedStory.resumable` accepts it), as is + ``_carry_board_advance``'s routine no-op on a tracked board — could fail on + lock contention, or on a :class:`~bmad_loop.runs.StateRootError` from + :func:`~bmad_loop.runs.lock_path_for`, for work it was never going to do. + + The probe is advisory in the strict sense: only a "would write nothing" + answer is acted on, and such a call simply linearizes at the probe's read + rather than at an acquisition. Every other outcome — including ANY exception + raised while probing — falls through to the locked path, which re-reads, + re-decides authoritatively and raises on the channel it always did. So the + probe can neither authorize a write nor add a failure mode the hold lacks: a + malformed board still raises :class:`SprintStatusError` from under the lock. + ``now`` needs no handling here, because both no-op arms of + :func:`_advance_locked` return before the ``last_updated`` write; a + probe-satisfied early-out is write-equivalent to the locked answer. + + Acquisition failure — for the calls that do reach the lock — surfaces as ``OSError`` (or :class:`~bmad_loop.runs.StateRootError` when no state root can be derived) on the channel callers already route this function's raises through — the engine's crash/escalation handling, the CLI's failure exit — so @@ -334,6 +384,19 @@ def advance(path: Path, story_key: str, target: str, *, now: str | None = None) """ if not path.is_file(): return None # no board, nothing to serialize against — take no lock + try: + current = story_status(path, story_key) + if current is None: + return None # absent row — nothing this call would write + if _row_at_or_past(current, target): + return current # already at or past target — never regress, no write + except Exception: # nosec B110 - ADVISORY probe: a fault here must decide nothing + # Broad by design, and the swallow is the point: narrowing the catch would + # let the probe invent a failure mode the locked path does not have. An + # unreadable board decides nothing here — the path below re-reads, + # re-decides, and raises on the channel callers already route this + # function's raises through. + pass with _board_lock(path): return _advance_locked(path, story_key, target, now=now) @@ -344,19 +407,17 @@ def _advance_locked( """:func:`advance`'s read-modify-write, run with the board's lock already held. Split out so the hold is exactly the file I/O and so every read inside it sees - one board. The leading ``is_file`` check repeats the caller's: the pre-lock - answer is taken without exclusion, and a delete can land between it and the - acquisition.""" + one board. The reads below repeat work the caller's pre-lock answers may + already have done, and deliberately: those answers are taken without + exclusion, so a delete can land between the ``is_file`` check and the + acquisition, and the advisory probe's row (#736) can be stale by the time the + lock is held. Only what this function reads decides the published bytes.""" if not path.is_file(): return None current = story_status(path, story_key) if current is None: return None - if ( - current in STATUS_ORDER - and target in STATUS_ORDER - and STATUS_ORDER.index(current) >= STATUS_ORDER.index(target) - ): + if _row_at_or_past(current, target): return current # already at or past target — never regress text = path.read_bytes().decode("utf-8") diff --git a/tests/test_sprintstatus_advance.py b/tests/test_sprintstatus_advance.py index 0e680ef8..7c7d4f4f 100644 --- a/tests/test_sprintstatus_advance.py +++ b/tests/test_sprintstatus_advance.py @@ -670,8 +670,16 @@ def test_status_in_bytes_raises_rather_than_calling_an_unreadable_board_absent(t # makes two of them serialize rather than trade last-write-wins. -def test_advance_holds_the_lock_across_every_read_and_the_write(tmp_path, monkeypatch): - """The hold spans the whole read-modify-write, not just the write. +def test_the_reads_that_decide_the_published_bytes_are_inside_the_lock(tmp_path, monkeypatch): + """The hold spans every read that decides the bytes — and only those (#736). + + Formerly `test_advance_holds_the_lock_across_every_read_and_the_write`, which + pinned the stricter claim that ALL THREE reads sit inside the hold. #736 + relaxed it deliberately: `advance` now runs ONE advisory pre-lock read to + answer the calls that would write nothing, so an idempotent replay no longer + fails on contention for work it was never going to do. What survives — and is + the whole protection — is that the reads feeding the published bytes still + happen after the acquisition. A lock taken around the atomic write alone excludes nobody that matters: the bytes being published were computed from a read that happened OUTSIDE it, so @@ -679,14 +687,22 @@ def test_advance_holds_the_lock_across_every_read_and_the_write(tmp_path, monkey ordering is recorded from the calls themselves rather than inferred from the result, because a lost update leaves a board that looks perfectly well-formed. - `load` is the probe for two of the three reads — the `story_status` - never-regress read and the epic-lift read both go through it — and the - writer spy is the third event. Advancing a `backlog` story of a `backlog` - epic is what makes the epic lift fire, so all three are present. - - Ablation: move `with _board_lock(path):` down to wrap only the - `atomic_write_bytes` call and this reddens — the two `load` events sort ahead - of `lock-enter`.""" + `load` is the probe for three of the four reads — the advisory probe, the + inside-the-lock `story_status` never-regress read, and the epic-lift read all + go through it — and the writer spy is the fourth event. Advancing a `backlog` + story of a `backlog` epic is what makes the epic lift fire, and a `backlog` + row is exactly what the advisory probe declines to answer, so the fall-through + and all three inside events are present. + + Ablation A: make `_advance_locked` reuse the probe's answer instead of its own + read (hoist the `current = story_status(...)` out and pass it in) and this + reddens — the inside segment loses a `load`, and with it the guarantee that + the never-regress decision saw the board the write is applied to. + + Ablation B: move `with _board_lock(path):` down to wrap only the + `atomic_write_bytes` call and this reddens — the two deciding `load` events + sort ahead of `lock-enter`, so the prefix is no longer the single advisory + read.""" p = _write(tmp_path) events: list[str] = [] real_lock, real_load = sprintstatus._board_lock, sprintstatus.load @@ -713,8 +729,10 @@ def spy_write(path, data, **kwargs): assert sprintstatus.advance(p, "3-2-digest-delivery", "in-progress") == "in-progress" assert events.count("lock-enter") == 1 and events.count("lock-exit") == 1 - assert events[0] == "lock-enter" and events[-1] == "lock-exit" - assert events[1:-1] == ["load", "load", "write"] # both reads AND the write, inside + enter = events.index("lock-enter") + assert events[-1] == "lock-exit" + assert events[:enter] == ["load"] # exactly one advisory read, and nothing else + assert events[enter + 1 : -1] == ["load", "load", "write"] # the deciding reads, inside def test_a_racing_writers_flip_survives_a_concurrent_advance(tmp_path): @@ -903,3 +921,186 @@ def spy_file_lock(path, **kwargs): assert sprintstatus.advance(tmp_path / "nope.yaml", "3-1-login", "done") is None assert entered == [] + + +def test_advance_takes_no_lock_when_the_row_is_already_at_or_past_target(tmp_path, monkeypatch): + """A never-regress no-op answers from an advisory read, without acquiring (#736). + + The defect this closes: `advance` acquired before `_advance_locked` could + discover there was nothing to write, so an idempotent replay — `bmad-loop + confirm` against a story the board already records as done is a DESIGNED + path, not an error — could fail on lock contention for work it never had. + Both shapes of the comparison are exercised: a row strictly PAST target + (`4-1-thing` sits at `review`, asked for `in-progress`) and a row exactly AT + it (`3-1-login` is `done`, asked for `done`). + + The board bytes are the second oracle. A probe that answered the no-op but + still went on to rewrite the file would satisfy the acquisition count alone, + and "no lock" would then be describing an unserialized write rather than a + no-op. + + Ablation: delete the probe from `advance` and this reddens on the count — + both calls acquire, since discovering the no-op is the locked body's job + again.""" + p = _write(tmp_path) + before = p.read_bytes() # as they landed, not `SPRINT.encode()` — CRLF on Windows + entered: list[Path] = [] + + @contextlib.contextmanager + def spy_file_lock(path, **kwargs): + entered.append(path) # pragma: no cover — must not be reached + with real_file_lock(path, **kwargs): + yield + + monkeypatch.setattr(sprintstatus, "file_lock", spy_file_lock) + + assert sprintstatus.advance(p, "4-1-thing", "in-progress") == "review" # past target + assert sprintstatus.advance(p, "3-1-login", "done") == "done" # exactly at target + + assert entered == [] + assert p.read_bytes() == before # a no-op, not an unserialized write + + +def test_advance_takes_no_lock_for_an_absent_row(tmp_path, monkeypatch): + """A story the board does not carry is answered before the lock too (#736). + + Sibling of the missing-board row above, one level in: the board exists, so + the `is_file` guard passes, but the story is not on it. `advance`'s contract + is `None` there, and a `None` return writes nothing, so there is no reason to + mint a sidecar — or to fail on one — for a row that does not exist. The + engine asks about stories it has not confirmed are on the board. + + Ablation: delete the probe's `if current is None: return None` early-out and + this reddens on the count — the absent-row answer moves back under the + hold.""" + p = _write(tmp_path) + entered: list[Path] = [] + + @contextlib.contextmanager + def spy_file_lock(path, **kwargs): + entered.append(path) # pragma: no cover — must not be reached + with real_file_lock(path, **kwargs): + yield + + monkeypatch.setattr(sprintstatus, "file_lock", spy_file_lock) + + assert sprintstatus.advance(p, "9-9-ghost", "done") is None + + assert entered == [] + + +def test_a_probe_satisfied_noop_succeeds_when_no_state_root_is_derivable(tmp_path, monkeypatch): + """The no-op stops risking a failure mode it had no work to earn (#736). + + `_board_lock` names its sidecar through `runs.lock_path_for`, which raises + `StateRootError` when no state root can be derived — and `StateRootError` is + NOT an `OSError`, so it escapes on its own taxonomy. Before the probe, that + made an already-done board's replay fail outright. Now it cannot: the answer + is reached without ever asking for a lock path. + + The second half is the load-bearing half. A probe that made the lock + optional, rather than unnecessary, would be a far worse bug than the one + being fixed — so the same fault on a call that really does write must still + surface. `3-2-digest-delivery` is `backlog`, so advancing it to `in-progress` + is a genuine write and has to raise. + + Patching the module attribute reaches the real call site because + `_board_lock`'s `from . import runs` is deliberately lazy (the import cycle + runs → verify → sprintstatus forbids a top-level one), so the lookup happens + per call against the patched module. + + Ablation: delete the probe and the FIRST call raises `StateRootError` — the + behavior #736 filed.""" + p = _write(tmp_path) + + def no_state_root(data_path): + raise runs.StateRootError("no state root") + + monkeypatch.setattr(runs, "lock_path_for", no_state_root) + + assert sprintstatus.advance(p, "3-1-login", "done") == "done" # probe-satisfied, no lock + + with pytest.raises(runs.StateRootError): + sprintstatus.advance(p, "3-2-digest-delivery", "in-progress") # a real write still needs it + + +def test_a_malformed_board_probe_falls_through_and_raises_from_under_the_lock( + tmp_path, monkeypatch +): + """The probe is advisory: a fault in it decides nothing (#736). + + An unreadable board makes `story_status` raise inside the probe, and the + probe swallows it — deliberately broadly, because narrowing the catch would + let the probe invent a failure the locked path does not have. The call then + falls through and the locked body raises `SprintStatusError` on exactly the + channel `cli.py`'s error routing already expects. + + The ACQUISITION COUNT is the oracle, not the raise. `pytest.raises` alone + survives removing the try/except entirely — the probe's own uncaught + `SprintStatusError` is the same class, from the same reader, and would pass + this row while never having reached the lock at all. Only `len(entered) == 1` + tells the two apart. + + Ablation: remove the probe's `try`/`except Exception` and this reddens on the + count — `entered == []`, because the probe raised before the acquisition.""" + p = tmp_path / "sprint-status.yaml" + p.write_text("development_status: []\n", encoding="utf-8") + entered: list[Path] = [] + + @contextlib.contextmanager + def spy_file_lock(path, **kwargs): + entered.append(path) + with real_file_lock(path, **kwargs): + yield + + monkeypatch.setattr(sprintstatus, "file_lock", spy_file_lock) + + with pytest.raises(sprintstatus.SprintStatusError): + sprintstatus.advance(p, "3-2-digest-delivery", "in-progress") + + assert len(entered) == 1 # it raised from UNDER the hold, not instead of taking it + + +def test_the_authoritative_never_regress_decision_is_made_under_the_lock(tmp_path): + """The probe's row may be stale by acquisition time, and is then discarded (#736). + + The probe reads outside all exclusion, so between it and the hold a rival can + move the row anywhere — including PAST the target this call is carrying. If + that stale answer were carried into `_advance_locked` instead of being + re-read, the never-regress test would be applied to a status the board no + longer has and the rival's forward progress would be rewritten backwards. The + published bytes are decided under the hold precisely so this cannot happen. + + The rival runs from inside the `_board_lock` spy, BEFORE it enters the real + lock, which is what a second process actually gets to do; running it after + would nest a blocking acquisition on a second fd and self-deadlock. The + one-shot latch stops the rival's own `advance` from recursing into another + rival. Both calls target the SAME row — that is the point, unlike the + lost-update row above, which needs two different rows. + + Ablation: thread the probe's answer into `_advance_locked` (hoist its + `current = story_status(...)` read and pass the probe's value in) and this + reddens — the call decides against the stale `backlog`, writes, and the board + comes back `in-progress` with the rival's `done` gone.""" + p = _write(tmp_path) + real_lock = sprintstatus._board_lock + raced: list[str | None] = [] + + @contextlib.contextmanager + def racing_lock(path): + if not raced: + # Latch FIRST: the rival's own `advance` re-enters this spy, and a + # latch set only on the way out would stage a rival per rival. + raced.append(None) + # a rival takes the row all the way to `done` while we are still queued + raced[0] = sprintstatus.advance(path, "3-2-digest-delivery", "done") + with real_lock(path): + yield + + with pytest.MonkeyPatch.context() as mp: + mp.setattr(sprintstatus, "_board_lock", racing_lock) + # our probe saw `backlog`; by the time we hold the lock the row is `done` + assert sprintstatus.advance(p, "3-2-digest-delivery", "in-progress") == "done" + + assert raced == ["done"] # the rival really wrote, so there was progress to lose + assert sprintstatus.story_status(p, "3-2-digest-delivery") == "done" # NOT regressed From defb1238d99d0ce3ee783c83be8f506222964493 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 26 Aug 2026 10:10:03 -0700 Subject: [PATCH 02/13] refactor(deferredwork): extract the pure decision loops from the locked mutators (#736) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Each locked read->edit->write folded its ids (or specs, or eligible entries) inline, so the decision only existed inside the hold. Extract each fold to a module-level pure helper — `_apply_done_many`, `_apply_open_many`, `_apply_appends`, `_eligible_for_archive` — and rewire the locked bodies through them, the argument `_apply_append`'s extraction already makes for the batched appender. Zero behavior change: the helpers are the same statements, the reads and the `atomic_write_text` call sites do not move. This is the shared body an advisory pre-lock probe needs so probe and authority cannot drift; the probes themselves land next. `_apply_done_many` carries `undo_owner` deliberately — the reopenable arm's LINE_BREAK refusal is part of the decision, so a hand-rolled scan for open entries would answer differently. --- src/bmad_loop/deferredwork.py | 123 ++++++++++++++++++++++++++-------- 1 file changed, 94 insertions(+), 29 deletions(-) diff --git a/src/bmad_loop/deferredwork.py b/src/bmad_loop/deferredwork.py index a5d53141..f2987219 100644 --- a/src/bmad_loop/deferredwork.py +++ b/src/bmad_loop/deferredwork.py @@ -845,6 +845,35 @@ def _apply_done( return _insert_after_status(text, entry, tail) +def _apply_done_many( + text: str, + dw_ids: Sequence[str], + date: str, + note: str, + notes: Sequence[str] | None, + undo_owner: str | None, +) -> tuple[str, list[str]]: + """Fold every id in `dw_ids` through :func:`_apply_done` *within* `text`, + returning the new text and the ids actually flipped, in the order given. + + Pure — text in, text out, no `Path` and no I/O — and it is ONE body for the + advisory pre-lock probe and the locked pass, so the two cannot drift: the + argument :func:`_apply_append`'s extraction already makes for the batched + appender. The whole decision lives here, `undo_owner` included, because the + reopenable arm's LINE_BREAK refusal (:func:`_apply_done`) can be the only + reason a batch flips nothing — a probe that scanned for open entries by hand + would answer "would write" where this answers "would not".""" + marked: list[str] = [] + for index, dw_id in enumerate(dw_ids): + entry_note = note if notes is None else notes[index] + updated = _apply_done(text, dw_id, date, entry_note, undo_owner=undo_owner) + if updated is None: + continue + text = updated + marked.append(dw_id) + return text, marked + + def _mark_done_many( path: Path, dw_ids: Sequence[str], @@ -885,14 +914,7 @@ def _mark_done_many( if not path.is_file(): return [] text = path.read_text(encoding="utf-8") - marked: list[str] = [] - for index, dw_id in enumerate(dw_ids): - entry_note = note if notes is None else notes[index] - updated = _apply_done(text, dw_id, date, entry_note, undo_owner=undo_owner) - if updated is None: - continue - text = updated - marked.append(dw_id) + text, marked = _apply_done_many(text, dw_ids, date, note, notes, undo_owner) if not marked: return [] atomic_write_text(path, text) @@ -1061,6 +1083,27 @@ def _apply_open(text: str, dw_id: str, note: str, undo_owner: str) -> str | None return text +def _apply_open_many( + text: str, dw_ids: Sequence[str], note: str, undo_owner: str +) -> tuple[str, list[str]]: + """Fold every id in `dw_ids` through :func:`_apply_open` *within* `text`, + returning the new text and the ids actually reopened, in the order given. + + Pure — text in, text out, no `Path` and no I/O — and ONE body for the + advisory pre-lock probe and the locked pass, so the two cannot drift. The + `undo_owner` match is part of the decision: an entry closed by a different + operation is skipped here, which is what makes "no id was eligible" a + question only this fold can answer.""" + reopened: list[str] = [] + for dw_id in dw_ids: + updated = _apply_open(text, dw_id, note, undo_owner) + if updated is None: + continue + text = updated + reopened.append(dw_id) + return text, reopened + + def mark_open_many(path: Path, dw_ids: Sequence[str], note: str, operation_id: str) -> list[str]: """Undo every close in `dw_ids` written by :func:`mark_done_many_reopenable` under `operation_id`, in ONE read and ONE atomic write. Returns the ids @@ -1087,13 +1130,7 @@ def mark_open_many(path: Path, dw_ids: Sequence[str], note: str, operation_id: s if not path.is_file(): return [] text = path.read_text(encoding="utf-8") - reopened: list[str] = [] - for dw_id in dw_ids: - updated = _apply_open(text, dw_id, note, undo_owner) - if updated is None: - continue - text = updated - reopened.append(dw_id) + text, reopened = _apply_open_many(text, dw_ids, note, undo_owner) if not reopened: return [] atomic_write_text(path, text) @@ -1318,6 +1355,23 @@ def _apply_append(text: str, spec: EntrySpec) -> tuple[str, str | None]: return text + sep + block, dw_id +def _apply_appends(text: str, specs: Sequence[EntrySpec]) -> tuple[str, list[str | None]]: + """Fold every spec through :func:`_apply_append` *within* `text`, returning + the new text and one minted id per spec — None where the spec deduped + against an open entry that already carries its marker. + + Pure — text in, text out, no `Path` and no I/O — and ONE body for the + advisory pre-lock probe and the locked pass, so the two cannot drift. Each + spec sees the text the previous one produced, which is what makes ids + sequential and lets two identical specs in one call dedupe against each + other; see :func:`_apply_append` for why that evolution is load-bearing.""" + minted: list[str | None] = [] + for spec in specs: + text, dw_id = _apply_append(text, spec) + minted.append(dw_id) + return text, minted + + def append_entries(path: Path, specs: Sequence[EntrySpec]) -> list[str | None]: """Append every entry in `specs` in ONE read and ONE atomic write, returning each spec's minted id — or None in its position when that spec deduped @@ -1395,10 +1449,7 @@ def append_entries_published( return [], None with ledger_lock(path): text = path.read_text(encoding="utf-8") if path.is_file() else "" - minted: list[str | None] = [] - for spec in specs: - text, dw_id = _apply_append(text, spec) - minted.append(dw_id) + text, minted = _apply_appends(text, specs) if all(dw_id is None for dw_id in minted): return minted, None path.parent.mkdir(parents=True, exist_ok=True) @@ -1610,6 +1661,29 @@ def _close_date(entry: DWEntry) -> str | None: return _iso_date_or_none(parts[1]) +def _eligible_for_archive(text: str, before: str | None) -> list[tuple[DWEntry, str]]: + """Every entry in `text` :func:`archive_closed` would move, paired with its + close date, in ledger order. + + Pure — text in, entries out, no `Path` and no I/O — and ONE body for the + advisory pre-lock probe and the locked pass, so the two cannot drift. Three + skips make up the decision: an entry that is not done, or done without a + date, has nothing to compare or to stamp a stub with; `before` excludes + entries closed on or after the cutoff; and a stub from a prior run is + already archived.""" + to_archive: list[tuple[DWEntry, str]] = [] + for entry in parse_ledger(text): + close_date = _close_date(entry) + if close_date is None: + continue # not done, or done without a date + if before is not None and close_date >= before: + continue # closed on or after the cutoff + if _is_stub(entry): + continue # stub from a prior archive_closed run + to_archive.append((entry, close_date)) + return to_archive + + def archive_closed( path: Path, *, @@ -1677,16 +1751,7 @@ def archive_closed( if not path.is_file(): return [] text = path.read_text(encoding="utf-8") - to_archive: list[tuple[DWEntry, str]] = [] - for entry in parse_ledger(text): - close_date = _close_date(entry) - if close_date is None: - continue # not done, or done without a date - if before is not None and close_date >= before: - continue # closed on or after the cutoff - if _is_stub(entry): - continue # stub from a prior archive_closed run - to_archive.append((entry, close_date)) + to_archive = _eligible_for_archive(text, before) if not to_archive: return [] archived_ids = [e.id for e, _ in to_archive] From 3dd1b56c6cc0d973f296041f259bfa245cd979a8 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 26 Aug 2026 10:20:53 -0700 Subject: [PATCH 03/13] fix(deferredwork): answer read-dependent no-ops before taking the ledger lock (#736) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Third instance of the class #726 closed twice here already: a lock taken for an operation that will not write turns a previously successful no-op into a failure. The acquisition can raise `OSError`, and deriving the sidecar path raises `runs.StateRootError` wherever no state root is nameable — so a replayed rollback, a re-run defer and `sweep --archive` over a ledger holding nothing closed could all fail at a lock they had no write to serialize. Two guards and five advisory probes, all above the lock and below every validation: - Missing-ledger `is_file` guards on `_mark_done_many`, `mark_open_many` and `record_decision`, the pattern `archive_closed` already kept. The rechecks under the hold stay — creation can race the answer. `append_entries_published` deliberately gets none: an absent ledger there means CREATE, which is a write. - One advisory pre-lock read per mutator, running the same pure decision helper the locked pass runs. Only a "would write nothing" answer is acted on, and such a call linearizes at the probe read — it publishes no bytes. Every other answer, and any fault during the probe, falls through to the hold, which re-reads and decides authoritatively. `archive_closed`'s probe sits above the `dry_run` branch, so a nothing-eligible dry run skips the lock too; an eligible one still runs under the hold, where the one code path is. Tests: NOOP_MUTATORS, the deliberate inverse of LOCKED_MUTATORS, drives `test_a_read_dependent_noop_takes_no_lock` and `test_a_noop_mutation_succeeds_when_no_state_root_is_derivable` over all ten public entry points, plus `test_mutators_take_no_lock_for_a_missing_ledger`. `test_a_failing_probe_read_falls_through_to_the_locked_path` faults only the probe read, which is what keeps the under-lock no-write guards ablation-provable now that the probe answers those same inputs first — the two rewritten tests say so and point at it. 18 ablations run singly; every one reddened its intended oracle. --- src/bmad_loop/deferredwork.py | 119 +++++++++++++- tests/test_deferredwork.py | 281 ++++++++++++++++++++++++++++++++-- 2 files changed, 380 insertions(+), 20 deletions(-) diff --git a/src/bmad_loop/deferredwork.py b/src/bmad_loop/deferredwork.py index f2987219..36323cab 100644 --- a/src/bmad_loop/deferredwork.py +++ b/src/bmad_loop/deferredwork.py @@ -25,6 +25,21 @@ LLM session writes this file directly and does NOT take the lock — orchestrator writes are sequenced against sessions today, so the exposure this closes is orchestrator-vs-orchestrator. + +What the hold covers is every read that decides the PUBLISHED BYTES, which is +not quite every read (#736). A mutator handed work that turns out to be a no-op +— ids that are all already done, a decision on an entry that is not there, +specs that all dedupe, nothing eligible to archive — may answer from ONE +advisory read taken before the lock, running the same pure decision helper the +locked pass runs so the two cannot drift. Only a "would write nothing" answer +is acted on, and such a call linearizes at the probe read: it publishes no +bytes, so there is nothing for a rival to interleave with. Every other answer, +and any fault during the probe, falls through to the hold, which re-reads and +decides authoritatively. This is what keeps a no-op from failing on a lock it +never needed — an `OSError` from acquisition, or a +:class:`~bmad_loop.runs.StateRootError` from deriving the sidecar path where no +state root exists — which a replayed rollback, a re-run sweep and +``sweep --archive`` all reach routinely. """ from __future__ import annotations @@ -891,6 +906,12 @@ def _mark_done_many( last-write-wins. Validation stays ABOVE the lock, so a programmer bug reports itself without first waiting on another process. + A batch that would flip nothing — every id missing, already done, or refused + by the reopenable arm's line-break guard — is answered from the advisory + pre-lock probe instead, with no acquisition at all (#736). The probe folds + the ids through :func:`_apply_done_many`, the same helper the locked pass + uses, so it cannot answer "no write" where the authority would write. + ``notes`` supplies a per-id resolution note, positionally paired with ``dw_ids``; ``note`` is the fallback for every id when it is None. A length mismatch raises before any I/O rather than closing a prefix under the wrong @@ -910,6 +931,21 @@ def _mark_done_many( # primitive replaced took no lock at all when handed nothing, and that # identity is part of what "byte-identical to the serial sequence" buys. return [] + if not path.is_file(): + # No ledger, no entry to flip, so no write and no lock — the order + # `archive_closed` already keeps for its own missing-ledger case. The + # recheck under the hold below stays: creation can race this answer. + return [] + try: + # ADVISORY pre-lock probe (#736): one read, and the same pure decision + # the locked pass makes. Only a "would write nothing" answer is acted on + # — the call then serializes at this read. Anything else, including any + # fault here, falls through to the hold, which re-reads and decides. + probe = path.read_text(encoding="utf-8") + if not _apply_done_many(probe, dw_ids, date, note, notes, undo_owner)[1]: + return [] + except Exception: # nosec B110 - ADVISORY probe: a fault here must decide nothing + pass with ledger_lock(path): if not path.is_file(): return [] @@ -1118,14 +1154,29 @@ def mark_open_many(path: Path, dw_ids: Sequence[str], note: str, operation_id: s lock once per id, leaving a rival writer a window between every pair of undos in what a rollback needs to be one step. - Nothing is written when no id was eligible, so a replayed rollback over - already-reopened entries leaves the file untouched rather than rewriting it - byte-for-byte.""" + Nothing is written when no id was eligible, and no lock is taken either + (#736): a replayed rollback over already-reopened entries is answered from + one advisory read, so it leaves the file untouched rather than rewriting it + byte-for-byte, and cannot fail on a lock it had no write to serialize.""" undo_owner = _operation_digest(operation_id) if not dw_ids: # No ids, no lock — see `_mark_done_many`. The `operation_id` above is # still validated, so an empty reopen cannot smuggle a bad one through. return [] + if not path.is_file(): + # No ledger, no close to undo — see `_mark_done_many`. Rechecked under + # the hold below. + return [] + try: + # ADVISORY pre-lock probe (#736): one read, and the same pure decision + # the locked pass makes. Only a "would write nothing" answer is acted on + # — the call then serializes at this read. Anything else, including any + # fault here, falls through to the hold, which re-reads and decides. + probe = path.read_text(encoding="utf-8") + if not _apply_open_many(probe, dw_ids, note, undo_owner)[1]: + return [] + except Exception: # nosec B110 - ADVISORY probe: a fault here must decide nothing + pass with ledger_lock(path): if not path.is_file(): return [] @@ -1215,6 +1266,12 @@ def record_decision( checked before the ``is_file`` short-circuit so an absent ledger cannot hide the bug. + A missing ledger, and a `dw_id` no entry carries, are both answered False + without taking the lock (#736) — there is no write to serialize, and the + TUI decision modal reaching a stale id should not fail on an acquisition. + The probe runs :func:`_apply_decision`, the same helper the locked pass + runs, which is None exactly when the entry is missing. + 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 @@ -1222,6 +1279,20 @@ def record_decision( zero-byte ledger where every entry used to be (#328). """ _require_iso_date(date) + if not path.is_file(): + # No ledger, no entry to record against — see `_mark_done_many`. + # Rechecked under the hold below. + return False + try: + # ADVISORY pre-lock probe (#736): one read, and the same pure decision + # the locked pass makes. Only a "would write nothing" answer is acted on + # — the call then serializes at this read. Anything else, including any + # fault here, falls through to the hold, which re-reads and decides. + probe = path.read_text(encoding="utf-8") + if _apply_decision(probe, dw_id, date, label, detail) is None: + return False + except Exception: # nosec B110 - ADVISORY probe: a fault here must decide nothing + pass with ledger_lock(path): if not path.is_file(): return False @@ -1426,8 +1497,12 @@ def append_entries_published( prefix that happened to precede it. Validating above the lock also means a programmer bug reports itself without first waiting on another process. - Nothing is written when every spec dedupes, so a replayed defer leaves the - file untouched rather than rewriting it byte-for-byte. + Nothing is written when every spec dedupes, and no lock is taken either + (#736): a replayed defer is answered from one advisory read that runs + :func:`_apply_appends`, the same helper the locked pass runs, so it leaves + the file untouched rather than rewriting it byte-for-byte. Deliberately NO + missing-ledger guard, unlike its sibling mutators: an absent ledger here + means CREATE, which is a write, and a write must take the lock. 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 @@ -1447,6 +1522,19 @@ def append_entries_published( if not specs: # Nothing to serialize against, so nothing to take a lock for. return [], None + try: + # ADVISORY pre-lock probe (#736): one read — shaped exactly like the + # locked one, absence included — and the same pure decision the locked + # pass makes. Only a "would write nothing" answer is acted on, and here + # that is every spec deduping, which is also the only case where the + # published text is the text already on disk. Anything else, including + # any fault here, falls through to the hold, which re-reads and decides. + probe = path.read_text(encoding="utf-8") if path.is_file() else "" + minted = _apply_appends(probe, specs)[1] + if all(dw_id is None for dw_id in minted): + return minted, None + except Exception: # nosec B110 - ADVISORY probe: a fault here must decide nothing + pass with ledger_lock(path): text = path.read_text(encoding="utf-8") if path.is_file() else "" text, minted = _apply_appends(text, specs) @@ -1731,8 +1819,13 @@ def archive_closed( modal, ``sweep --archive`` — serialize here rather than trading last-write-wins. ONE acquisition spans BOTH writes — the archive sibling has no lock of its own precisely because it is only ever - written under its ledger's lock — and ``dry_run`` runs inside the hold too, - so there is one code path rather than a locked and an unlocked one. + written under its ledger's lock — and an ELIGIBLE ``dry_run`` runs inside + the hold too, so there is one code path rather than a locked and an unlocked + one. A run with nothing eligible is the exception, and only because it is + not a code path at all: the advisory pre-lock probe (#736) answers it with + the empty list before either branch is reached, so ``sweep --archive`` over + a ledger holding nothing closed keeps reporting success where the state root + cannot be derived or the lock cannot be taken. """ if before is not None: _require_iso_date(before) @@ -1747,6 +1840,18 @@ def archive_closed( # taken for a file that is not there. Rechecked under the hold below, # deletion being able to race this answer. return [] + try: + # ADVISORY pre-lock probe (#736): one read, and the same pure decision + # the locked pass makes. Only a "would write nothing" answer is acted on + # — the call then serializes at this read. Above the `dry_run` branch on + # purpose, so a nothing-eligible dry run skips the lock too; an ELIGIBLE + # dry run still runs under the hold, where the one code path is. Anything + # else, including any fault here, falls through to that hold. + probe = path.read_text(encoding="utf-8") + if not _eligible_for_archive(probe, before): + return [] + except Exception: # nosec B110 - ADVISORY probe: a fault here must decide nothing + pass with ledger_lock(path): if not path.is_file(): return [] diff --git a/tests/test_deferredwork.py b/tests/test_deferredwork.py index 42c13e8a..64668b08 100644 --- a/tests/test_deferredwork.py +++ b/tests/test_deferredwork.py @@ -3459,17 +3459,24 @@ def test_mark_open_many_matches_serial_mark_open_bytes(tmp_path, monkeypatch): def test_mark_open_many_writes_nothing_when_no_id_is_eligible(tmp_path, monkeypatch): - """A replayed rollback over already-open entries leaves the file untouched. - - Ablation: drop the `if not reopened: return []` guard — the write spy fires - and the row reds.""" + """A replayed rollback over already-open entries leaves the file untouched — + and, since #736, takes no lock to establish that. + + Ablation: delete `mark_open_many`'s pre-lock probe block — the acquisition + assertion reds. NOT the `if not reopened: return []` guard, whose documented + ablation used to live here and now goes GREEN: the probe answers these exact + inputs above the lock, so the write spy never reaches that guard. Its + ablation moved to `test_a_failing_probe_read_falls_through_to_the_locked_path`, + which faults the probe read so the same inputs reach the hold.""" path = write_ledger(tmp_path) - writes = [] + writes, acquisitions = [], [] _counting_write(monkeypatch, writes) - assert mark_open_many(path, ["DW-1", "DW-99"], "by dw-a", OPERATION_ID) == [] + with _counting_lock(monkeypatch, acquisitions): + assert mark_open_many(path, ["DW-1", "DW-99"], "by dw-a", OPERATION_ID) == [] assert writes == [] + assert acquisitions == [] def test_record_decision_close_matches_the_serial_pair_bytes(tmp_path, monkeypatch): @@ -3565,17 +3572,25 @@ def test_record_decision_records_a_decision_on_an_already_done_entry(tmp_path): def test_record_decision_returns_false_for_a_missing_entry(tmp_path, monkeypatch): - """A missing id records nothing and writes nothing. - - Ablation: write unconditionally after the appliers — the write spy fires.""" + """A missing id records nothing, writes nothing, and takes no lock (#736). + + Ablation: delete `record_decision`'s pre-lock probe block — the acquisition + assertion reds. The ablation this docstring used to carry ("write + unconditionally after the appliers") now goes GREEN: the probe answers a + missing id above the lock, so the write spy never reaches the under-lock + `if updated is None` guard. That guard's ablation moved to + `test_a_failing_probe_read_falls_through_to_the_locked_path`, which faults + the probe read so this same call reaches the hold.""" path = write_ledger(tmp_path) before = path.read_text(encoding="utf-8") - writes = [] + writes, acquisitions = [], [] _counting_write(monkeypatch, writes) - assert record_decision(path, "DW-99", "2026-06-11", "keep", "x", close_note="y") is False + with _counting_lock(monkeypatch, acquisitions): + assert record_decision(path, "DW-99", "2026-06-11", "keep", "x", close_note="y") is False assert writes == [] + assert acquisitions == [] assert path.read_text(encoding="utf-8") == before @@ -3667,6 +3682,11 @@ def _unavailable_lock(path, **kwargs): yield # pragma: no cover — unreachable +# Every row here must be seeded to WRITE. A no-op row would grade nothing: the +# advisory pre-lock probe (#736) answers a read-dependent no-op before the +# acquisition these tests spy on, so the hold, the nesting and the +# raise-on-failure claims would all pass vacuously. `NOOP_MUTATORS` below is the +# deliberate inverse, and grades the absence of that same acquisition. LOCKED_MUTATORS = { "append_decision": lambda p: append_decision(p, "DW-1", "2026-06-11", "keep", "later"), "append_entries": lambda p: append_entries( @@ -3853,8 +3873,12 @@ def test_scripted_interleave_loses_no_update(tmp_path, monkeypatch): reverts B's append. Ablation: hoist `_mark_done_many`'s `path.read_text` above its - `with ledger_lock(path):` — A's read then happens before the spy fires, A - writes its stale snapshot, and DW-4 is gone from the final ledger.""" + `with ledger_lock(path):` AND WRITE FROM IT — A's read then happens before + the spy fires, A writes its stale snapshot, and DW-4 is gone from the final + ledger. The hoist alone is no longer the ablation: the advisory probe (#736) + already reads above the lock. It decides nothing here — DW-1 is open, so the + probe declines to answer and the under-lock read stays authoritative — which + is exactly the property this row keeps grading.""" path = write_ledger(tmp_path) real_lock = deferredwork.ledger_lock rival_ran = [] @@ -3950,6 +3974,237 @@ def test_lock_acquisition_failure_raises_and_writes_nothing(tmp_path, monkeypatc assert (archive.read_text(encoding="utf-8") if archive.is_file() else None) == archive_before +# --------------------------- read-dependent no-ops take no lock (#736) +# +# A lock taken for an operation that will not write turns a previously +# successful no-op into a failure: the acquisition itself can raise `OSError`, +# and deriving the sidecar path raises `runs.StateRootError` wherever no state +# root is nameable. #726 closed two instances of that class here — the missing +# ledger and the empty batch, both answerable without reading. This section +# grades the third, where only a READ can tell that the call would write +# nothing: every id already done, an id no entry carries, every spec deduping, +# nothing eligible to archive. Each is answered from ONE advisory pre-lock read +# that runs the same pure decision helper the locked pass runs; every other +# answer, and any fault during the probe, falls through to the hold. + +_DEDUPE_SPEC = { + "title": "already appended by the seeder", + "origin": "probe-noop", + "source_spec": "spec-probe-noop.md", + "reason": "so the row dedupes and writes nothing", +} + +# The deliberate inverse of `LOCKED_MUTATORS`: every row is seeded to write +# NOTHING. Pairs are (call, expected result). The return value is graded +# alongside the acquisition count because the count alone is satisfiable by a +# probe that skipped the lock while answering the WRONG no-op value — and each +# mutator's no-op answer is part of its frozen contract. +NOOP_MUTATORS = { + # No entry carries DW-99, so the decision line has nowhere to go. + "append_decision": ( + lambda p: append_decision(p, "DW-99", "2026-06-11", "keep", "later"), + False, + ), + # The seeder already appended this spec's open twin, so it dedupes. + "append_entries": (lambda p: append_entries(p, [EntrySpec(**_DEDUPE_SPEC)]), [None]), + "append_entry": (lambda p: append_entry(p, **_DEDUPE_SPEC), None), + # DW-2 closed 2026-05-25, on or after the cutoff; DW-1 and DW-3 are open. + "archive_closed": (lambda p: archive_closed(p, before="2026-05-01"), []), + "mark_done": (lambda p: mark_done(p, "DW-99", "2026-06-11", "fixed"), False), + # DW-2 is already done, and DW-99 does not exist. + "mark_done_many": ( + lambda p: mark_done_many(p, ["DW-2", "DW-99"], "2026-06-11", "fixed"), + [], + ), + "mark_done_many_reopenable": ( + lambda p: mark_done_many_reopenable(p, ["DW-2"], "2026-06-11", "fixed", OPERATION_ID), + [], + ), + "mark_open": (lambda p: mark_open(p, "DW-99", "by dw-a", OPERATION_ID), False), + # DW-1 is open and carries no undo marker; DW-99 does not exist. + "mark_open_many": ( + lambda p: mark_open_many(p, ["DW-1", "DW-99"], "by dw-a", OPERATION_ID), + [], + ), + "record_decision": ( + lambda p: record_decision(p, "DW-99", "2026-06-11", "keep", "x", close_note="y"), + False, + ), +} + +# The append rows dedupe against an entry that has to be on disk first; every +# other row is already a no-op against the plain fixture. +_NEEDS_A_DEDUPE_TWIN = {"append_entries", "append_entry"} + +# One row per PROBED LEAF, keyed into `NOOP_MUTATORS` above. The wrapper rows +# there reach these same five bodies, so faulting the probe once per leaf covers +# every probe in the module without re-grading a delegation. +PROBED_LEAVES = [ + "append_entries", + "archive_closed", + "mark_done_many", + "mark_open_many", + "record_decision", +] + + +def _noop_seed_for(tmp_path: Path, name: str) -> Path: + """The ledger `name`'s no-op call needs, written before any spy is installed.""" + path = write_ledger(tmp_path) + if name in _NEEDS_A_DEDUPE_TWIN: + assert append_entry(path, **_DEDUPE_SPEC) == "DW-4" + return path + + +@pytest.mark.parametrize("name", sorted(NOOP_MUTATORS)) +def test_a_read_dependent_noop_takes_no_lock(tmp_path, monkeypatch, name): + """A call a read proves would write nothing acquires nothing. + + The exact inverse of `test_every_mutator_holds_the_ledger_lock`, over the + same public surface: there the input is seeded to write and the acquisition + is mandatory; here it is seeded to no-op and the acquisition is a defect. + Both readings of "the lock is load-bearing" have to hold, or the fix has + traded one failure for another. + + Nothing landing on disk is asserted as well as nothing acquiring, and it is + not redundant: the probe reaches its answer through the same pure helper the + locked pass folds, so a helper that reported "no write" while the authority + would have written would show up here as changed bytes rather than as a + count. + + Ablation: delete this mutator's pre-lock `try:` probe block — the spy counts + one and the row reds on `acquisitions == []`.""" + path = _noop_seed_for(tmp_path, name) + before = path.read_text(encoding="utf-8") + call, expected = NOOP_MUTATORS[name] + acquisitions = [] + + with _counting_lock(monkeypatch, acquisitions): + assert call(path) == expected + + assert acquisitions == [] + assert path.read_text(encoding="utf-8") == before + assert not (path.parent / ARCHIVE_REL).exists() + + +@pytest.mark.parametrize("name", PROBED_LEAVES) +def test_a_failing_probe_read_falls_through_to_the_locked_path(tmp_path, monkeypatch, name): + """A probe that cannot read decides nothing: the call takes the lock and the + under-lock guards refuse the write, exactly as before the probe existed. + + This is what keeps those under-lock guards ablation-provable. Their own + tests used to reach them with these very inputs; the probe now answers first, + so the write-spy oracle fires above the lock and those ablations go green. + Faulting the probe read — and only the probe read, the under-lock one + succeeds — routes the same call back through the hold, where the guard is + the only thing standing between it and a pointless rewrite. + + The acquisition count is the load-bearing assertion, not the return value. + A probe whose fault escaped instead of falling through would raise; a probe + that answered anyway would leave the count at zero. Only `== [path]` says + "fell through to the hold" rather than "never needed it" (S1 found the + matching trap one module over, where `pytest.raises` stayed green with the + `except` deleted). + + Ablations, singly: (A) delete this leaf's `except Exception:` — the injected + `PermissionError` escapes and the row reds; (B) delete this leaf's + under-lock no-write guard (`if not marked:` / `if not reopened:` / + `if updated is None:` / `if all(dw_id is None ...)` / `if not to_archive:`) + — the write spy fires and the row reds.""" + path = _noop_seed_for(tmp_path, name) + before = path.read_text(encoding="utf-8") + call, expected = NOOP_MUTATORS[name] + real, fired = Path.read_text, [] + + def raise_once_then_delegate(self, *a, **kw): + # Keyed on the ledger: the probe read is the FIRST read of this path, so + # the fault lands there and the under-lock read gets the real file. + if self == path and not fired: + fired.append(self) + raise PermissionError(13, "Permission denied") + return real(self, *a, **kw) + + acquisitions, writes = [], [] + _counting_write(monkeypatch, writes) + monkeypatch.setattr(Path, "read_text", raise_once_then_delegate) + + with _counting_lock(monkeypatch, acquisitions): + assert call(path) == expected + + assert fired # the fault really fired (a green row proves nothing otherwise) + assert acquisitions == [path] + assert writes == [] + assert path.read_text(encoding="utf-8") == before + + +@pytest.mark.parametrize( + ("call", "expected"), + [ + pytest.param( + lambda p: mark_done_many(p, ["DW-1"], "2026-06-11", "fixed"), [], id="mark_done_many" + ), + pytest.param( + lambda p: mark_open_many(p, ["DW-1"], "by dw-a", OPERATION_ID), [], id="mark_open_many" + ), + pytest.param( + lambda p: record_decision(p, "DW-1", "2026-06-11", "keep", "x"), + False, + id="record_decision", + ), + ], +) +def test_mutators_take_no_lock_for_a_missing_ledger(tmp_path, monkeypatch, call, expected): + """No ledger means no write, and so no lock — `archive_closed`'s rule, now + kept by its three siblings too. + + These ids are real and these arguments are valid, so nothing but the absent + file can be answering: it is the `is_file` guard under test, not the probe + beneath it (which would fault on the same missing file and fall through). + The recheck under the hold stays in each body, creation being able to race + this answer. + + Ablation: move that mutator's `is_file` guard back below its + `with ledger_lock(path):` — the spy fires and the row reds.""" + path = tmp_path / "deferred-work.md" # deliberately never created + acquisitions = [] + + with _counting_lock(monkeypatch, acquisitions): + assert call(path) == expected + + assert acquisitions == [] + assert not path.exists() # and nothing was created on the way past + + +@pytest.mark.parametrize("name", sorted(NOOP_MUTATORS)) +def test_a_noop_mutation_succeeds_when_no_state_root_is_derivable(tmp_path, monkeypatch, name): + """With nowhere to put a lock file, a no-op still succeeds; a write still fails. + + `runs.StateRootError` is raised while DERIVING the sidecar path, before any + OS lock is attempted, so it reaches every caller of `ledger_lock` in an + environment that names no state root — and it is not an `OSError`, so no + caller's net catches it. Answering the no-op above the acquisition is what + stops that environment from failing calls that were never going to write. + + The write-shaped control is not decoration: it is what says the patch is + live. Without it a `lock_path_for` stub that silently never fired would make + every row above vacuously green. + + Ablation: delete any probe — that row raises `StateRootError` instead of + returning, and reds.""" + path = _noop_seed_for(tmp_path, name) + call, expected = NOOP_MUTATORS[name] + + def no_state_root(_path): + raise runs.StateRootError("no state root in this environment") + + monkeypatch.setattr(runs, "lock_path_for", no_state_root) + + assert call(path) == expected + + with pytest.raises(runs.StateRootError): + mark_done(path, "DW-1", "2026-06-11", "a call that would really write") + + # The child of the two-process acceptance test. Appends 8 entries with distinct # origins (so the idempotence scan never dedupes one away) through the real # `append_entry`, after a file rendezvous with the parent. It reports the ids it From b2d43318b6bb9299adfa6a370644d1bc5b8a2270 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 26 Aug 2026 10:34:08 -0700 Subject: [PATCH 04/13] fix(engine): anchor the reset-owned ledger restore on the committed baseline blob (#735) --- src/bmad_loop/engine.py | 140 ++++++++++++++++---- tests/test_engine.py | 283 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 399 insertions(+), 24 deletions(-) diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index be3aab01..010bef4b 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -4577,33 +4577,52 @@ def _legacy_ledger_changed_before_harvest(self, task: StoryTask) -> bool: ) return False - def _ledger_is_gits_to_restore(self, task: StoryTask) -> bool: - """Whether git owns the active ledger and reset is responsible for it. - - Probe failures degrade toward keeping the file: uncertainty must never - authorize deleting a tracked ledger that ``reset --hard`` restored. + def _ledger_rel(self) -> tuple[str | None, Exception | None]: + """Name the active ledger relative to the workspace root, for a git probe. + + Three answers, and never a raise. ``(rel, None)`` derived. ``(None, + fault)`` — resolution itself failed, so the ledger's scope is unknown. + ``(None, None)`` — proven external: it resolved cleanly and still fell + outside the root, so no revision of this repo can name it. + + The fault answer is deliberately left undecided here, because the two + consumers degrade in OPPOSITE directions: + :meth:`_ledger_is_gits_to_restore` keeps the file, while + :meth:`_ledger_baseline_text` withholds the write anchor. """ ledger = self.workspace.paths.deferred_work root = self.workspace.root try: - rel = ledger.relative_to(root).as_posix() + return ledger.relative_to(root).as_posix(), None except ValueError: try: - rel = ledger.resolve().relative_to(root.resolve()).as_posix() + return ledger.resolve().relative_to(root.resolve()).as_posix(), None except (OSError, RuntimeError) as e: - self.journal.append( - "ledger-scope-probe-failed", - story_key=task.story_key, - error=str(e), - ) - return True + return None, e except ValueError: - # An external ledger was outside the reset's reach. A None - # snapshot means this harvest created it, so it remains ours to - # unlink. - return False + return None, None + + def _ledger_is_gits_to_restore(self, task: StoryTask) -> bool: + """Whether git owns the active ledger and reset is responsible for it. + + Probe failures degrade toward keeping the file: uncertainty must never + authorize deleting a tracked ledger that ``reset --hard`` restored. + """ + rel, fault = self._ledger_rel() + if fault is not None: + self.journal.append( + "ledger-scope-probe-failed", + story_key=task.story_key, + error=str(fault), + ) + return True + if rel is None: + # An external ledger was outside the reset's reach. A None + # snapshot means this harvest created it, so it remains ours to + # unlink. + return False try: - return verify.path_tracked(root, rel) + return verify.path_tracked(self.workspace.root, rel) except (verify.GitError, OSError, RuntimeError) as e: self.journal.append( "ledger-tracked-probe-failed", @@ -4612,6 +4631,61 @@ def _ledger_is_gits_to_restore(self, task: StoryTask) -> bool: ) return True + def _ledger_baseline_text(self, task: StoryTask) -> tuple[bool, str | None]: + """The ledger text ``reset --hard`` republishes, taken from git itself (#735). + + Answers ``(True, text)`` when the baseline commit carries the ledger, + ``(True, None)`` when it determinately does not — the reset leaves no + tracked file behind — and ``(False, None)`` when no anchor could be + derived at all: no baseline commit, no repo-relative name, or a probe + that failed. + + Newlines are normalized to LF because the only thing this text is ever + compared against is :meth:`_ledger_text`, which reads in Python's + universal-newline mode. The blob comes back with the path's working-tree + filters applied, so under ``core.autocrlf=true`` it is CRLF; without + this normalization the reset-owned write arm would go silently + never-true on Windows and every such restore would degrade to a skip. + + **The fault direction is INVERTED from + :meth:`_ledger_is_gits_to_restore`, deliberately.** That probe degrades + to ``True`` because its consumer is an unlink, and uncertainty must never + delete. This one degrades to NO anchor because its only consumer is a + write arm, and uncertainty must never write. Copying the other probe's + degrade here reopens #735 through the repair itself. + + Nothing may escape. ``verify.GitError`` is a plain ``Exception`` and the + attempt's net is ``(OSError, StateRootError)``, so a probe fault leaking + out of here would replace an in-flight ``RunPaused`` in that ``finally`` + with a secondary repair failure. + """ + if not task.baseline_commit: + return False, None + rel, fault = self._ledger_rel() + if rel is None: + if fault is not None: + self.journal.append( + "ledger-baseline-probe-failed", + story_key=task.story_key, + error=str(fault), + ) + return False, None + try: + blob = verify.worktree_file_bytes_at_revision( + self.workspace.root, task.baseline_commit, rel + ) + if blob is None: + return True, None + text = blob.decode("utf-8") + except (verify.GitError, OSError, RuntimeError, UnicodeDecodeError) as e: + self.journal.append( + "ledger-baseline-probe-failed", + story_key=task.story_key, + error=str(e), + ) + return False, None + return True, text.replace("\r\n", "\n").replace("\r", "\n") + def _restore_ledger(self, task: StoryTask, snapshot: str | None) -> None: """Retract this attempt's engine ledger writes, without taking a concurrent writer's work with them (#286). @@ -4624,10 +4698,16 @@ def _restore_ledger(self, task: StoryTask, snapshot: str | None) -> None: * ``post_engine_ledger_digest`` — the bytes THIS engine last published. Matching it means the file on disk is the harvest append this restore exists to retract. - * the post-rollback observation, on a ledger git owns. ``reset --hard`` - republishes a tracked ledger's committed bytes, which are nobody's - concurrent write; restoring the snapshot over them is what puts back - the session's own ledger edits the reset erased. + * the ledger's committed blob at ``task.baseline_commit``, on a ledger + git owns. That blob is exactly what ``reset --hard`` republished, it is + nobody's concurrent write, and restoring the snapshot over it is what + puts back the session's own ledger edits the reset erased. The anchor + is read out of git rather than off the working tree because **a + post-reset observation may justify a SKIP, never a WRITE**: a rival + writing a tracked ledger inside the reset window would otherwise BE the + observation this arm trusts, and the restore would overwrite it (#735). + A probe that cannot answer withholds the anchor, so an unprovable + baseline degrades to the same journaled skip rather than a write. Neither anchor holding means the text belongs to somebody else, and the restore degrades to a journaled skip rather than a write. **A retraction @@ -4654,7 +4734,11 @@ def _restore_ledger(self, task: StoryTask, snapshot: str | None) -> None: ledger = self.workspace.paths.deferred_work # Read IMMEDIATELY after `_rollback_or_pause` returned: only pure Python # runs between the reset and this line, so the compare window below is - # file-I/O-only rather than spanning the rollback's git spawns. + # file-I/O-only rather than spanning the rollback's git spawns. This + # observation authorizes ONLY the skip that follows — declining to act is + # safe whoever wrote those bytes. It is never a write anchor: it is taken + # after the very reset it would attest to, so a rival that landed inside + # that window becomes the observation itself (#735). observed = self._ledger_text() if observed == snapshot: return @@ -4668,6 +4752,14 @@ def _restore_ledger(self, task: StoryTask, snapshot: str | None) -> None: # None snapshot could do, so return before taking a lock no write # would ever use. return + # The WRITE anchor derives from the committed blob, never from an + # observation a rival could have authored (#735). Probed here, before the + # lock, for the same reason as the one above: it spawns git, and + # `ledger_lock` may cover file I/O only. Only a ledger git owns can be + # reset-owned at all, so an untracked, ignored or external one skips the + # spawn. A fault degrades to NO anchor — the inverse of the gits probe + # above, whose consumer is an unlink; this one's is a write. + anchored, expected = self._ledger_baseline_text(task) if gits else (False, None) diverged = False with deferredwork.ledger_lock(ledger): # PURE TEXT ONLY under the hold. Every `deferredwork` mutator takes @@ -4677,7 +4769,7 @@ def _restore_ledger(self, task: StoryTask, snapshot: str | None) -> None: if current == snapshot: return ours = _digest_of(current) == task.post_engine_ledger_digest - reset_owned = current == observed and gits + reset_owned = anchored and current == expected if snapshot is None: # `gits` is False on this arm — the guard above returned # otherwise — so the file is untracked, ignored or external and diff --git a/tests/test_engine.py b/tests/test_engine.py index 47a97c5e..0638cf15 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -12718,6 +12718,228 @@ def failed_resolve(self, *args, **kwargs): assert event["story_key"] == task.story_key +def test_ledger_baseline_text_reads_the_committed_blob(project, monkeypatch): + """The reset-owned write anchor is the committed blob, read before the lock. + + ``reset --hard `` republishes exactly this blob, so the blob — and + not an observation of the working tree taken after that reset — is what a + reset-owned restore is entitled to overwrite (#735). + + The probe spawns git and `ledger_lock` is contracted to cover file I/O only + (#286), so the spy grades WHERE the call happens as well as what it answers. + + Ablation: move the `_ledger_baseline_text(task)` call in `_restore_ledger` + inside the `with deferredwork.ledger_lock(ledger):` block and the `held` row + reds. + """ + project.deferred_work.parent.mkdir(parents=True, exist_ok=True) + committed = "# Deferred Work\n\n## DW-1 committed at baseline\n" + project.deferred_work.write_text(committed, encoding="utf-8") + git(project.project, "add", "-A") + git(project.project, "commit", "-q", "-m", "track deferred-work") + engine, _ = make_engine(project, [], policy=_harvest_policy()) + task = StoryTask(story_key="1-1-a", epic=1) + task.baseline_commit = rev_parse_head(project.project) + task.baseline_untracked = [] + + assert engine._ledger_baseline_text(task) == (True, committed) + + held: list[bool] = [] + real_blob = verify.worktree_file_bytes_at_revision + + def spying_blob(*args, **kwargs): + held.append(bool(getattr(deferredwork._LOCK_STATE, "held", False))) + return real_blob(*args, **kwargs) + + monkeypatch.setattr(verify, "worktree_file_bytes_at_revision", spying_blob) + engine._restore_ledger(task, committed + "\n## DW-2 this session's edit\n") + + assert held == [False] + + +def test_ledger_baseline_text_normalizes_committed_crlf(project): + """A CRLF blob is normalized to LF, because `_ledger_text` reads universal. + + `worktree_file_bytes_at_revision` applies the path's working-tree filters, so + under `core.autocrlf=true` the baseline blob comes back CRLF while + `_ledger_text`'s `read_text` has already turned the same file on disk into + LF. Comparing them raw makes `reset_owned` silently NEVER-true on Windows: + every tracked restore would degrade to a skip, and no Linux run would ever + say so. This row is that Windows guard, made Linux-visible by committing the + CRLF bytes directly. + + Ablation: drop the `.replace("\\r\\n", "\\n").replace("\\r", "\\n")` tail and + both rows red here, on Linux. + """ + project.deferred_work.parent.mkdir(parents=True, exist_ok=True) + project.deferred_work.write_bytes(b"# Deferred Work\r\n\r\n## DW-1 crlf at baseline\r\n") + git(project.project, "add", "-A") + git(project.project, "commit", "-q", "-m", "track a crlf deferred-work") + engine, _ = make_engine(project, [], policy=_harvest_policy()) + task = StoryTask(story_key="1-1-a", epic=1) + task.baseline_commit = rev_parse_head(project.project) + task.baseline_untracked = [] + + anchored, expected = engine._ledger_baseline_text(task) + assert (anchored, expected) == (True, "# Deferred Work\n\n## DW-1 crlf at baseline\n") + # The point of the normalization: the anchor must equal what the ONLY thing + # it is ever compared against reads back off those same bytes. + assert expected == engine._ledger_text() + + +def test_ledger_baseline_text_reports_absence_at_baseline(project): + """A baseline that does not carry the ledger is determinate, not a fault. + + `reset --hard` leaves no tracked file there, so `None` IS the expected + post-reset state and the anchor still holds — which is what lets a restore + put the session's ledger back over an absence rather than calling it + divergence. + """ + engine, _ = make_engine(project, [], policy=_harvest_policy()) + task = StoryTask(story_key="1-1-a", epic=1) + task.baseline_commit = rev_parse_head(project.project) + task.baseline_untracked = [] + # Committed AFTER the baseline was stamped: tracked now, absent at baseline. + project.deferred_work.parent.mkdir(parents=True, exist_ok=True) + project.deferred_work.write_text("# Deferred Work\n", encoding="utf-8") + git(project.project, "add", "-A") + git(project.project, "commit", "-q", "-m", "track deferred-work after the baseline") + + assert engine._ledger_baseline_text(task) == (True, None) + assert [ + e for e in engine.journal.entries() if e["kind"] == "ledger-baseline-probe-failed" + ] == [] + + +def test_ledger_baseline_text_degrades_without_a_baseline(project): + """No baseline commit is a determinate no-anchor, and NOT a probe fault. + + Nothing failed — there is simply no revision to derive an expected state + from — so the write arm stands down silently rather than filing a fault row + an operator would have to triage. + """ + project.deferred_work.parent.mkdir(parents=True, exist_ok=True) + project.deferred_work.write_text("# Deferred Work\n", encoding="utf-8") + engine, _ = make_engine(project, [], policy=_harvest_policy()) + task = StoryTask(story_key="1-1-a", epic=1) + + assert task.baseline_commit is None + assert engine._ledger_baseline_text(task) == (False, None) + assert [ + e for e in engine.journal.entries() if e["kind"] == "ledger-baseline-probe-failed" + ] == [] + + +def test_ledger_baseline_probe_failure_degrades_and_journals(project, monkeypatch): + """A probe that cannot answer withholds the anchor — the INVERSE degrade. + + `_ledger_is_gits_to_restore` degrades to True because its consumer is an + unlink and uncertainty must never delete. This probe's only consumer is a + write arm, so uncertainty must never write; copying the other direction here + would reopen #735 through the error path itself. + + The catch also has to live INSIDE the helper: `verify.GitError` is a plain + `Exception` and the attempt's net is `(OSError, StateRootError)`, so an + escape would replace an in-flight `RunPaused` in that `finally`. + + Ablation: delete the `except (verify.GitError, OSError, RuntimeError, + UnicodeDecodeError)` arm and the GitError escapes — this row reds on the + raise rather than on the tuple. + """ + project.deferred_work.parent.mkdir(parents=True, exist_ok=True) + project.deferred_work.write_text("# Deferred Work\n", encoding="utf-8") + git(project.project, "add", "-A") + git(project.project, "commit", "-q", "-m", "track deferred-work") + engine, _ = make_engine(project, [], policy=_harvest_policy()) + task = StoryTask(story_key="1-1-a", epic=1) + task.baseline_commit = rev_parse_head(project.project) + task.baseline_untracked = [] + + def fail_probe(*args, **kwargs): + raise GitError("injected baseline probe failure") + + monkeypatch.setattr(verify, "worktree_file_bytes_at_revision", fail_probe) + + assert engine._ledger_baseline_text(task) == (False, None) + (event,) = [e for e in engine.journal.entries() if e["kind"] == "ledger-baseline-probe-failed"] + assert event["story_key"] == "1-1-a" + assert "injected baseline probe failure" in event["error"] + + +def test_restore_ledger_reset_owned_write_uses_the_blob_anchor(project): + """POSITIVE CONTROL: the reset-owned write arm still fires on the new anchor. + + Without this row a normalization slip or a mis-derived rel would make + `expected` never equal `current`, every tracked restore would quietly degrade + to a skip, and every negative test around it would stay green — the anchor + would be dead and nothing would say so. + + The digest anchor is deliberately NOT ours here, so the write is attributable + to `reset_owned` alone. + + Ablation: hardcode `reset_owned = False` in `_restore_ledger` and both the + written bytes and the empty-journal row red. + """ + project.deferred_work.parent.mkdir(parents=True, exist_ok=True) + committed = "# Deferred Work\n\n## DW-1 committed at baseline\n" + project.deferred_work.write_text(committed, encoding="utf-8") + git(project.project, "add", "-A") + git(project.project, "commit", "-q", "-m", "track deferred-work") + engine, _ = make_engine(project, [], policy=_harvest_policy()) + task = StoryTask(story_key="1-1-a", epic=1) + task.baseline_commit = rev_parse_head(project.project) + task.baseline_untracked = [] + # What `reset --hard` erased: this session's own ledger edits, which the + # snapshot exists to put back over the republished committed bytes. + snapshot = committed + "\n## DW-2 this session's own edit\n" + task.post_engine_ledger_digest = _digest_of("bytes this engine never published") + + engine._restore_ledger(task, snapshot) + + assert project.deferred_work.read_text(encoding="utf-8") == snapshot + assert [ + e for e in engine.journal.entries() if e["kind"] == "ledger-restore-skipped-diverged" + ] == [] + + +def test_restore_ledger_probe_failure_never_writes(project, monkeypatch): + """DIRECTION PIN: an unprovable baseline skips, it never falls back to the + observation. + + The same inputs as the positive control above, with only the probe faulted. + The tempting degrade — trust `current == observed` when the blob could not be + read — is exactly the #735 defect, reintroduced through the error path. The + restore also has to come back normally: a fault that raised here would + replace an in-flight `RunPaused`. + + Ablation: make `_ledger_baseline_text`'s except arm return + `(True, self._ledger_text())` and the write fires — the bytes row reds. + """ + project.deferred_work.parent.mkdir(parents=True, exist_ok=True) + committed = "# Deferred Work\n\n## DW-1 committed at baseline\n" + project.deferred_work.write_text(committed, encoding="utf-8") + git(project.project, "add", "-A") + git(project.project, "commit", "-q", "-m", "track deferred-work") + engine, _ = make_engine(project, [], policy=_harvest_policy()) + task = StoryTask(story_key="1-1-a", epic=1) + task.baseline_commit = rev_parse_head(project.project) + task.baseline_untracked = [] + snapshot = committed + "\n## DW-2 this session's own edit\n" + task.post_engine_ledger_digest = _digest_of("bytes this engine never published") + + def fail_probe(*args, **kwargs): + raise GitError("injected baseline probe failure") + + monkeypatch.setattr(verify, "worktree_file_bytes_at_revision", fail_probe) + + engine._restore_ledger(task, snapshot) + + assert project.deferred_work.read_text(encoding="utf-8") == committed + kinds = [e["kind"] for e in engine.journal.entries()] + assert "ledger-baseline-probe-failed" in kinds + assert "ledger-restore-skipped-diverged" in kinds + + def test_pre_harvest_ledger_restore_is_atomic_on_publication_failure(project, monkeypatch): """A failed rollback publish leaves the current ledger byte-intact.""" engine, _ = make_engine(project, [], policy=_harvest_policy()) @@ -12900,6 +13122,67 @@ def test_nonfixable_retry_leaves_tracked_ledger_at_its_baseline_bytes(project): assert persisted.pre_harvest_ledger is None +def test_rejected_attempt_restore_leaves_a_rival_that_wrote_inside_the_reset_window( + project, monkeypatch +): + """THE #735 DEFECT PROOF. A rival that writes a TRACKED ledger between + `reset --hard` returning and the restore's observation read is not + reset-owned, and the snapshot must not be republished over it. + + The anchor this replaces was `current == observed and gits`, with `observed` + read once the rollback returned. A rival landing inside that window BECOMES + `observed`, so the comparison holds later and labels the rival's bytes "what + reset put back". The blob anchor is taken from `task.baseline_commit` + instead, which no rival can author. + + The oracle is the rival's SURVIVAL and the journal kind, never the restored + bytes: this ledger is tracked, so `reset --hard` republishes its committed + text whether or not this code runs at all, and a byte assertion would pass + for the wrong reason (proven by control in #726 session 6). + + Ablation: restore `reset_owned = current == observed and gits` and the + snapshot overwrites the rival — the survival row AND the diverged row red. + """ + project.deferred_work.parent.mkdir(parents=True, exist_ok=True) + before = "# Deferred Work\n\ntracked baseline\n" + project.deferred_work.write_text(before, encoding="utf-8") + git(project.project, "add", "-A") + git(project.project, "commit", "-q", "-m", "track deferred-work") + write_sprint(project, {"epic-1": "backlog", "1-1-a": "ready-for-dev"}) + engine, _ = make_engine( + project, + [ + _baseline_liar_effect(project, deferred=[HARVEST_A]), + dev_effect(project, "1-1-a", followup_review=False), + ], + policy=_harvest_policy(attempts=2), + ) + + real_rollback = engine._rollback_or_pause + landed: list[bool] = [] + + def rollback_then_rival(task, **kwargs): + real_rollback(task, **kwargs) + # After the reset returned, before `_restore_ledger` reads `observed`: + # exactly the window #735 describes. One-shot, so a later attempt's + # rollback cannot file it twice. + if not landed: + landed.append(True) + with project.deferred_work.open("a", encoding="utf-8") as f: + f.write("\n### DW-9: filed by another process\n\nstatus: open\n") + + monkeypatch.setattr(engine, "_rollback_or_pause", rollback_then_rival) + + assert engine.run().done == 1 + + assert landed == [True] + assert "DW-9: filed by another process" in project.deferred_work.read_text(encoding="utf-8") + (event,) = [ + e for e in engine.journal.entries() if e["kind"] == "ledger-restore-skipped-diverged" + ] + assert event["story_key"] == "1-1-a" + + @pytest.mark.parametrize( "ledger_state", [ From bf34d389182de80a96ea49f0d316ff7fd258c499 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 26 Aug 2026 10:48:07 -0700 Subject: [PATCH 05/13] fix(engine,sweep): anchor the defer and migration restores on the committed baseline (#735) --- src/bmad_loop/engine.py | 47 ++++++++++--- src/bmad_loop/sweep.py | 35 ++++++++-- tests/test_engine.py | 148 ++++++++++++++++++++++++++++++++++++++-- tests/test_sweep.py | 133 +++++++++++++++++++++++++++++++++++- 4 files changed, 339 insertions(+), 24 deletions(-) diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index 010bef4b..baa9e824 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -6335,9 +6335,11 @@ def _defer(self, task: StoryTask, reason: str) -> None: raise # The reset reverts a *tracked* ledger's uncommitted edits, so the # review-found entries it erased are real knowledge worth putting - # back. The restore is compare-and-set against the post-reset - # observation and gated on git owning the file, and it merges rather - # than overwrites when another writer interleaved. A foreign write + # back. The restore is compare-and-set against the ledger's committed + # blob at the baseline — the text the reset republished, never an + # observation of the tree a rival could have authored (#735) — and + # gated on git owning the file; it merges rather than overwrites when + # another writer interleaved. A foreign write # that landed BEFORE the reset is the reset's casualty, not the # restore's: the snapshot predates both, so nothing here can tell # that write apart from the session's own erased edits. @@ -6356,9 +6358,10 @@ def _restore_defer_ledger(self, task: StoryTask, snapshot: str) -> None: The window being repaired spans ``_rollback_or_pause``'s git spawns, so a lock cannot cover it — :func:`deferredwork.ledger_lock` is contracted - never to span a subprocess. Compare-and-set stands in: the post-reset - observation is the expected state, and anything else found under the lock - belongs to somebody else. + never to span a subprocess. Compare-and-set stands in, anchored on the + ledger's committed blob at ``task.baseline_commit`` — the text the reset + republished — and anything else found under the lock belongs to somebody + else. Three refusals, in order of how much they know: @@ -6369,9 +6372,17 @@ def _restore_defer_ledger(self, task: StoryTask, snapshot: str) -> None: foreign writer and the correct restore is no write at all. The guard this replaces compared ``current != snapshot`` and overwrote on exactly that difference: it ARMED the lost update it reads like it prevents. - * the text moved between the observation and the lock — another writer - landed inside the window, so the snapshot is republished by APPENDING - the entries disk has since lost, never by overwriting what arrived. + * the text under the lock is not the one the reset republished. The + anchor is read out of git rather than off the working tree because **a + post-reset observation may justify a SKIP, never a WRITE**: a rival + writing a tracked ledger inside the reset window would otherwise BE the + observation this arm trusts, and the overwrite would take that rival's + entries with it (#735). Every other case — a writer who landed inside + the window, or a baseline no probe could read — republishes the + snapshot by APPENDING the entries disk has since lost, never by + overwriting what arrived. Unlike :meth:`_restore_ledger`, this site can + degrade all the way to that merge instead of to a skip: appending + cannot destroy anybody's write. Write and lock faults propagate, as the unguarded write here always did: a repair write that could not be serialized must fail loudly. @@ -6379,7 +6390,12 @@ def _restore_defer_ledger(self, task: StoryTask, snapshot: str) -> None: ledger = self.workspace.paths.deferred_work # Read IMMEDIATELY after `_rollback_or_pause` returned: only pure Python # runs between the reset and this line, so the compare window below is - # file-I/O-only rather than spanning the rollback's git spawns. + # file-I/O-only rather than spanning the rollback's git spawns. This + # observation authorizes ONLY the skip that follows — declining to act is + # safe whoever wrote those bytes. It is never the write anchor: taken + # after the very reset it would attest to, a rival that landed inside + # that window becomes the observation itself (#735), which is what the + # blob probe below exists to replace. observed = self._ledger_text() if observed == snapshot: return @@ -6388,6 +6404,15 @@ def _restore_defer_ledger(self, task: StoryTask, snapshot: str) -> None: # byte of the delta above is a live foreign write and there is # nothing of ours to restore over it. return + # The WRITE anchor derives from the committed blob, never from an + # observation a rival could have authored (#735). Probed here, before the + # lock, because it spawns git and `ledger_lock` may cover file I/O only. + # `gits` is already established above, so this only ever runs on a ledger + # `reset --hard` could actually have republished. No anchor degrades to + # the merge below, which is append-only and therefore cannot destroy a + # rival's write — the reason this site can absorb a probe fault the way + # `_restore_ledger`'s degrade-to-skip has to. + anchored, expected = self._ledger_baseline_text(task) merged: list[str] = [] flat_remainder = False with deferredwork.ledger_lock(ledger): @@ -6397,7 +6422,7 @@ def _restore_defer_ledger(self, task: StoryTask, snapshot: str) -> None: current = self._ledger_text() if current == snapshot: return - if current == observed: + if anchored and current == expected: ledger.parent.mkdir(parents=True, exist_ok=True) atomic_write_text(ledger, snapshot) return diff --git a/src/bmad_loop/sweep.py b/src/bmad_loop/sweep.py index 5dad1b87..8c005c86 100644 --- a/src/bmad_loop/sweep.py +++ b/src/bmad_loop/sweep.py @@ -983,7 +983,15 @@ def _ensure_migration(self, text: str) -> None: if crits: details = "; ".join(str(e.get("detail", e.get("type", "?"))) for e in crits) self._escalate(task, f"CRITICAL escalation from migration session: {details}") - new_text = ledger.read_text(encoding="utf-8") if ledger.is_file() else "" + # Split so ABSENCE survives: `new_text` stays `str` for + # `validate_migration`, while `rewrite` keeps the difference between + # "the session emptied the ledger" and "the session deleted it". On + # an untracked ledger the restore below has no blob to anchor on and + # this rejected rewrite — the exact text this attempt graded — is the + # anchor instead, so flattening `None` to `""` here would make the + # deleted-ledger case indistinguishable from a rival's empty write. + rewrite = ledger.read_text(encoding="utf-8") if ledger.is_file() else None + new_text = rewrite if rewrite is not None else "" if result.status != "completed": errors = [session_failure_reason("migration", result)] else: @@ -1028,16 +1036,24 @@ def _ensure_migration(self, text: str) -> None: # covers tracked files, the explicit write covers an untracked # ledger that `git reset` cannot restore self._safe_reset(task) - # Read IMMEDIATELY after the reset returned: only pure Python runs - # between them, so the compare below is file-I/O-only rather than - # spanning the reset's git spawns, which no lock may cover (#286). - observed = ledger.read_text(encoding="utf-8") if ledger.is_file() else None + # The WRITE anchor derives from the committed blob, never from an + # observation of the tree taken after the very reset it would attest + # to: a rival writing a tracked ledger inside that window would BE + # the observation, and this restore would overwrite it (#735). Probed + # BEFORE the lock — it spawns git, and `ledger_lock` may cover file + # I/O only, which no reset window can (#286). A ledger git does not + # own has no blob to anchor on, and `reset --hard` cannot have + # touched it either, so there the anchor is the rejected rewrite this + # attempt actually graded — down to `None == None` when the session + # deleted the ledger outright. No anchor at all withholds the write. + anchored, committed = self._ledger_baseline_text(task) + expected = committed if committed is not None else rewrite diverged = False with deferredwork.ledger_lock(ledger): # PURE TEXT ONLY under the hold — `ledger_lock` is not reentrant # and every mutator takes it. current = ledger.read_text(encoding="utf-8") if ledger.is_file() else None - if current == observed: + if anchored and current == expected: ledger.parent.mkdir(parents=True, exist_ok=True) atomic_write_text(ledger, text) else: @@ -1048,6 +1064,13 @@ def _ensure_migration(self, text: str) -> None: # half-broken ledger, and the migration input a human must fix is # no longer the one this attempt was graded against — the same # call `migrate-duplicate-ids` makes about a corrupt ledger. + # A baseline probe that could not answer lands here too, and + # deliberately: without an anchor there is no proof the text on + # disk is the reset's own work rather than somebody's live write, + # and an unprovable restore is exactly the overwrite this arm + # exists to refuse. The escalation is the right recovery for both + # — the resume above resets the attempt budget and re-reads the + # ledger, which is what a rival-corrupted migration input needs. # Journaled outside the hold; `_escalate` raises. self.journal.append( "sweep-migration-restore-diverged", diff --git a/tests/test_engine.py b/tests/test_engine.py index 0638cf15..cfe005d1 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -6410,7 +6410,18 @@ def test_review_leg_reconciles_finalize_tail_death_followup_true(project): def test_defer_preserves_deferred_work_additions(project): """Review sessions append real knowledge to deferred-work.md; a plateau - defer's git reset must not erase it.""" + defer's git reset must not erase it. + + Doubles as the POSITIVE CONTROL for the blob anchor (#735): with no rival + anywhere, the reset-owned write arm still has to fire. The last row is what + earns it that job — the merge this site degrades to ALSO republishes DW-1, so + a normalization slip or a mis-derived rel that made `expected` never equal + `current` would leave the entry assertion green over an anchor that is dead, + and every negative test around it green with it. + + Ablation: hardcode `anchored = False` in `_restore_defer_ledger` and the + diverged row appears. + """ from conftest import git from conftest import review_effect as make_review @@ -6419,9 +6430,16 @@ def test_defer_preserves_deferred_work_additions(project): git(project.project, "commit", "-q", "-m", "seed deferred-work") write_sprint(project, {"1-1-a": "ready-for-dev"}) + filed: list[bool] = [] + def reviewing_with_defer(spec): - with project.deferred_work.open("a") as f: - f.write("\n### DW-1: pre-existing flaky retry\n\nstatus: open\n") + # latched: the review budget spends three sessions, but the finding is + # filed once — three copies of one heading are a duplicate-id ledger, and + # the merge the ablation above forces reports the ids it moved. + if not filed: + filed.append(True) + with project.deferred_work.open("a") as f: + f.write("\n### DW-1: pre-existing flaky retry\n\nstatus: open\n") return make_review(project, "1-1-a", clean=False, patched=1, finalized=False)(spec) engine, _ = make_engine( @@ -6431,6 +6449,8 @@ def reviewing_with_defer(spec): summary = engine.run() assert summary.deferred == 1 assert "DW-1: pre-existing flaky retry" in project.deferred_work.read_text() + kinds = [e["kind"] for e in engine.journal.entries()] + assert "defer-ledger-restore-diverged" not in kinds @contextlib.contextmanager @@ -6471,8 +6491,8 @@ def test_defer_restore_merges_a_concurrent_append(project, monkeypatch): observation, before the lock — which is exactly the interleaving the old `current != snapshot` guard overwrote wholesale. - Ablation: delete the `current == observed` arm so the restore always writes - the snapshot, and the rival entry vanishes. + Ablation: delete the `anchored and current == expected` arm so the restore + always writes the snapshot, and the rival entry vanishes. """ from conftest import git from conftest import review_effect as make_review @@ -6511,6 +6531,124 @@ def reviewing_with_defer(spec): assert event["dw_ids"] == ["DW-1"] and event["flat_remainder"] is False +def test_defer_restore_merges_a_rival_that_wrote_inside_the_reset_window(project, monkeypatch): + """#735. The rival lands one window EARLIER than the twin above: between + `reset --hard` returning and the restore's observation read. + + That window is the one the old anchor was blind to. A rival writing a TRACKED + ledger there BECOMES `observed`, so `current == observed` holds under the + lock, labels the rival's bytes "what the reset put back", and overwrites them + with the snapshot. The anchor is the ledger's committed blob at + `task.baseline_commit` instead — the text the reset actually republished, and + the one thing in this comparison no rival can author. + + The oracle is the rival's SURVIVAL and the journal row, never the restored + bytes: this ledger is tracked, so `reset --hard` puts its committed text back + whether or not this code runs at all, and a byte assertion would pass for the + wrong reason (proven by control in #726 session 6). + + Ablation: revert the write arm to `current == observed` and DW-2 vanishes + under the snapshot — the entry row reds, and the merge's `dw_ids` row with it. + """ + from conftest import git + from conftest import review_effect as make_review + + project.deferred_work.write_text("# Deferred Work\n") + git(project.project, "add", "-A") + git(project.project, "commit", "-q", "-m", "seed deferred-work") + write_sprint(project, {"1-1-a": "ready-for-dev"}) + + filed: list[bool] = [] + + def reviewing_with_defer(spec): + if not filed: # one finding, three review sessions — see the twin above + filed.append(True) + with project.deferred_work.open("a") as f: + f.write("\n### DW-1: review-found flaky retry\n\nstatus: open\n") + return make_review(project, "1-1-a", clean=False, patched=1, finalized=False)(spec) + + engine, _ = make_engine( + project, + [dev_effect(project, "1-1-a")] + [reviewing_with_defer for _ in range(3)], + ) + + real_rollback = engine._rollback_or_pause + landed: list[bool] = [] + + def rollback_then_rival(task, **kwargs): + real_rollback(task, **kwargs) + # After the reset returned, before `_restore_defer_ledger` reads + # `observed`: exactly the window #735 describes. One-shot, so a rollback + # on any other path cannot file it twice. + if not landed: + landed.append(True) + with project.deferred_work.open("a", encoding="utf-8") as f: + f.write("\n### DW-2: filed by another process\n\nstatus: open\n") + + monkeypatch.setattr(engine, "_rollback_or_pause", rollback_then_rival) + + summary = engine.run() + + assert summary.deferred == 1 and landed == [True] + entries = _ledger_entries(project) + assert entries["DW-1"].title == "review-found flaky retry" + assert entries["DW-2"].title == "filed by another process" + (event,) = [e for e in engine.journal.entries() if e["kind"] == "defer-ledger-restore-diverged"] + assert event["story_key"] == "1-1-a" and event["dw_ids"] == ["DW-1"] + + +def test_defer_restore_probe_failure_degrades_to_the_merge(project, monkeypatch): + """DIRECTION PIN, #735: an unprovable baseline merges, it never falls back to + the observation. + + No rival at all here — the only difference from the positive control is a + faulted probe. The tempting degrade (trust `current == observed` when the + blob could not be read) is the defect itself, reintroduced through the error + path. This site can afford the strict direction where `_restore_ledger` + cannot afford anything softer: the merge is append-only, so refusing to + overwrite still republishes every entry the reset erased. + + Ablation: have `_ledger_baseline_text`'s except arm return `(True, + self._ledger_text())` and the write arm fires — both journal rows red. + """ + from conftest import git + from conftest import review_effect as make_review + + project.deferred_work.write_text("# Deferred Work\n") + git(project.project, "add", "-A") + git(project.project, "commit", "-q", "-m", "seed deferred-work") + write_sprint(project, {"1-1-a": "ready-for-dev"}) + + filed: list[bool] = [] + + def reviewing_with_defer(spec): + if not filed: # one finding, three review sessions — see the twin above + filed.append(True) + with project.deferred_work.open("a") as f: + f.write("\n### DW-1: review-found flaky retry\n\nstatus: open\n") + return make_review(project, "1-1-a", clean=False, patched=1, finalized=False)(spec) + + engine, _ = make_engine( + project, + [dev_effect(project, "1-1-a")] + [reviewing_with_defer for _ in range(3)], + ) + + def fail_probe(*args, **kwargs): + raise GitError("injected baseline probe failure") + + monkeypatch.setattr(verify, "worktree_file_bytes_at_revision", fail_probe) + + summary = engine.run() + + # the knowledge still comes back — via the merge, not via a write it could + # not prove it was entitled to make + assert summary.deferred == 1 + assert _ledger_entries(project)["DW-1"].title == "review-found flaky retry" + kinds = [e["kind"] for e in engine.journal.entries()] + assert "ledger-baseline-probe-failed" in kinds + assert "defer-ledger-restore-diverged" in kinds + + def test_merge_reports_an_id_collision_instead_of_dropping_the_entry(project): """A rival that mints OUR id is reported, never silently accepted (#286). diff --git a/tests/test_sweep.py b/tests/test_sweep.py index 200c2204..c9fe7c31 100644 --- a/tests/test_sweep.py +++ b/tests/test_sweep.py @@ -3935,8 +3935,9 @@ def test_migration_restore_escalates_on_divergence(project, monkeypatch): and a migration input that changed underneath the attempt it was graded against is a human problem — the same call `migrate-duplicate-ids` makes. - Ablation: drop the `current == observed` arm so the restore always writes, - and the rival's line is clobbered while no escalation is raised. + Ablation: drop the `anchored and current == expected` arm so the restore + always writes, and the rival's line is clobbered while no escalation is + raised. """ write_legacy_ledger(project, LEGACY_LEDGER) engine, adapter = make_sweep(project, [_half_migrated_effect(project)] * 2) @@ -3955,6 +3956,134 @@ def test_migration_restore_escalates_on_divergence(project, monkeypatch): assert len(adapter.sessions) == 1 +@contextlib.contextmanager +def _rival_appending_safe_reset(monkeypatch, engine, ledger, addition): + """A `_safe_reset` spy that lands one foreign append the instant the real + reset returns, and still really resets. + + That instant is where the migration restore's window opens: `_safe_reset` + returning is the last event before the restore decides what the ledger is + supposed to contain. A rival there is precisely what #735 describes, and it + is the half the old `current == observed` anchor could not see — the rival + became the observation. One-shot, because this wrapper is re-entered on every + migration attempt and the retry must not file it twice. + """ + real_reset = engine._safe_reset + landed: list[bool] = [] + + def reset_then_rival(task, **kwargs): + real_reset(task, **kwargs) + if not landed: + landed.append(True) + with ledger.open("a", encoding="utf-8") as f: + f.write(addition) + + monkeypatch.setattr(engine, "_safe_reset", reset_then_rival) + yield landed + + +def test_migration_restore_escalates_when_a_rival_writes_inside_the_reset_window( + project, monkeypatch +): + """#735, tracked. The twin above lands its rival at the lock, where the old + anchor already refused. This one lands it in the window the old anchor was + blind to: between `_safe_reset` returning and the read that graded it. + + A rival writing a TRACKED ledger there BECOMES `observed`, so the compare + holds under the lock and the pre-migration text is written straight over it — + and the run then RETRIES over a ledger no human has looked at. The anchor is + the committed blob at `task.baseline_commit` instead, which is what the reset + republished and which no rival can author. + + Ablation: restore the `observed` read after `_safe_reset` and compare + `current == observed` — the rival's line is clobbered, the escalation never + raises, and the second attempt dispatches. Every row below reds. + """ + write_legacy_ledger(project, LEGACY_LEDGER) + engine, adapter = make_sweep(project, [_half_migrated_effect(project)] * 2) + rival = "- **Filed by another process** — `other.txt` needs a look\n" + with _rival_appending_safe_reset(monkeypatch, engine, project.deferred_work, rival) as landed: + summary = engine.run() + + assert summary.paused and landed == [True] + assert engine.state.tasks["sweep-migrate"].phase == Phase.ESCALATED + assert "changed underneath the failed migration attempt" in engine.state.paused_reason + assert "sweep-migration-restore-diverged" in journal_kinds(engine) + # the rival's line stands, and the refusal did not paper the rewrite over + text = project.deferred_work.read_text(encoding="utf-8") + assert "Filed by another process" in text and text != LEGACY_LEDGER + # the refusal is terminal for this run: the second attempt never dispatches + assert len(adapter.sessions) == 1 + + +def test_migration_restore_escalates_for_an_untracked_rival_inside_the_reset_window( + project, monkeypatch +): + """#735, untracked. An untracked ledger has no blob to anchor on, and + `reset --hard` cannot have put anything back into it either — so the anchor + is the rejected rewrite this attempt actually graded, which is the one text + here that predates the window. + + Same rival, same window, same refusal: the point is that losing the blob does + NOT send this site back to trusting the observation. `_ledger_baseline_text` + answers determinate absence (`(True, None)`) rather than "no anchor", and the + rewrite fills the slot. + + Ablation: restore the `observed` read after `_safe_reset` and compare + `current == observed` — the rival is clobbered and the run retries. + """ + write_legacy_ledger(project, LEGACY_LEDGER, commit=False) + engine, adapter = make_sweep(project, [_half_migrated_effect(project)] * 2) + rival = "- **Filed by another process** — `other.txt` needs a look\n" + with _rival_appending_safe_reset(monkeypatch, engine, project.deferred_work, rival) as landed: + summary = engine.run() + + assert summary.paused and landed == [True] + assert engine.state.tasks["sweep-migrate"].phase == Phase.ESCALATED + assert "changed underneath the failed migration attempt" in engine.state.paused_reason + assert "sweep-migration-restore-diverged" in journal_kinds(engine) + text = project.deferred_work.read_text(encoding="utf-8") + assert "Filed by another process" in text and text != LEGACY_LEDGER + assert len(adapter.sessions) == 1 + + +def test_migration_restore_escalates_when_the_baseline_probe_fails(project, monkeypatch): + """DIRECTION PIN, #735: an unprovable baseline escalates, it never falls back + to the observation. + + No rival anywhere — the quiet fixture of the positive control below, with + only the probe faulted. Two failed probes' worth of uncertainty is the most + this site can have, and the answer to it is the same one a rival gets: refuse + the write and put it in front of a human. Unlike the defer restore there is + no append-only merge to degrade into — republishing the pre-migration text is + an overwrite or it is nothing — so the escalation IS the degrade, and the + resume it routes to resets the attempt budget and re-reads the ledger. + + Ablation: invert the fault direction — have `_ledger_baseline_text`'s except + arm return `(True, self._ledger_text())` — and the restore writes, the run + retries, and every row below reds. + """ + write_legacy_ledger(project, LEGACY_LEDGER) + engine, adapter = make_sweep(project, [_half_migrated_effect(project)] * 2) + + def fail_probe(*args, **kwargs): + raise verify.GitError("injected baseline probe failure") + + monkeypatch.setattr(verify, "worktree_file_bytes_at_revision", fail_probe) + + summary = engine.run() + + assert summary.paused + assert engine.state.tasks["sweep-migrate"].phase == Phase.ESCALATED + assert "changed underneath the failed migration attempt" in engine.state.paused_reason + kinds = journal_kinds(engine) + assert "ledger-baseline-probe-failed" in kinds + assert "sweep-migration-restore-diverged" in kinds + # escalated on the FIRST failed attempt: an unprovable restore does not get + # to spend the retry budget over a ledger nobody has graded + assert len(adapter.sessions) == 1 + + def test_migration_restore_quiet_path_unchanged(project): """#286. With no interleaving writer the restore still puts the pre-migration text back byte for byte, and journals no divergence — the CAS From 47ad336f9254d259b0acde8df6c4a2d9949ef3f9 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 26 Aug 2026 11:05:33 -0700 Subject: [PATCH 06/13] docs(features,changelog): document the no-op probes and blob anchors (#735, #736) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CHANGELOG gains the two consolidated `### Fixed` entries: the #736 advisory pre-lock probes and the #735 blob-derived restore anchors, each naming its released-behavior delta. docs/FEATURES.md: the rollback-paths sentence now describes the restores as compare-and-set whose WRITE anchors on the committed baseline blob, with the post-reset observation authorizing only the skips, and names the degrade each site takes when no anchor can be derived. The board-lock and ledger-lock bullets both gain the relaxed invariant: the hold covers every read that decides the published bytes, and a read-dependent no-op is answered by one advisory pre-lock read that takes no lock. Two test changes the completeness audit forced, no mechanism code: - tests/test_engine.py: `test_defer_skips_restore_for_a_ledger_the_reset_never_touched` had gone VACUOUS. Its ablation ("delete the `_ledger_is_gits_to_restore` gate and the rival entry is clobbered") passed green on HEAD, because the #735 write arm is now `anchored and current == expected` and an untracked ledger has no blob at the baseline, so the arm cannot fire with or without the gate. Adds the oracle that does grade it — the gate short-circuits above the baseline probe, so above the git spawn and the lock — and rewrites the record to say why the data oracles no longer discriminate. Re-ablated: reds on `assert probed == []`. - tests/test_sprintstatus_advance.py: requalified the record in `test_a_racing_writers_flip_survives_a_concurrent_advance` that said hoisting read#1 above the lock does not redden the row. Still true, but it now describes production — the #736 probe IS that hoisted read — so it says so, and says why this row is what makes it safe. --- CHANGELOG.md | 41 ++++++++++++++++++++++++++++++ docs/FEATURES.md | 4 +-- tests/test_engine.py | 30 ++++++++++++++++++++-- tests/test_sprintstatus_advance.py | 10 +++++++- 4 files changed, 80 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e55a57fe..b6fd585c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -233,6 +233,47 @@ decisions` and the TUI decision modal now also catch the state-root failure that created inside the window went with it. A tracked ledger absent at snapshot time is still never deleted, and is now answered before any lock is taken. A write or lock fault is journaled as `ledger-restore-failed` and preserves an in-flight pause, as before. +- **A read-dependent no-op is answered before its lock is taken** (#736). A lock acquired for + work that turns out to write nothing turns a previously successful no-op into a failure: + `bmad-loop confirm` replayed against a story the board already records as `done`, a rollback + replayed over entries already reopened, `sweep --archive` over a ledger holding nothing + closed. Acquisition can fail, and so can deriving the sidecar path where no state root + exists — on work that was never going to happen. #726 fixed two instances of the shape (a + missing ledger, an empty batch); the rest of the class is swept here. `sprintstatus.advance` + and the five read-dependent `deferred-work.md` mutators each take ONE advisory read before + acquiring, running the same pure decision helper the locked pass runs so the probe cannot + answer "no write" where the authority would write. Only a would-write-nothing answer is + acted on, and such a call linearizes at that read: it publishes no bytes, so there is + nothing for a rival to interleave with. Every other answer — and any fault while probing — + falls through to the hold, which re-reads and decides authoritatively, so a malformed board + still raises from under the lock and the probe adds no failure mode the locked path lacks. + `mark_done_many`, `mark_open_many` and `record_decision` also answer a missing ledger + without acquiring, as `archive_closed` already did; `append_entries_published` deliberately + does not, because an absent ledger there means CREATE, which is a write. + With nothing eligible to archive, `bmad-loop sweep --archive` therefore now exits 0 ("no + closed entries to archive") where a dead lock or an underivable state root made it exit 1. + An ELIGIBLE archive under a dead lock still fails exactly as before, as does every write + arm. +- **A ledger restore that spans `git reset --hard` anchors on the committed baseline blob, + not on an observation of the tree** (#735). The three restores no lock may cover — the + rejected attempt's retraction, a rolled-back defer's, and the sweep's failed-migration + rewrite — each compared against the ledger as observed the instant the rollback returned. + That read is taken after the very reset it attests to, so a rival writing a tracked ledger + inside the window BECOMES the observation, the compare then holds, and the restore + overwrites the rival's entries. Each write arm now takes its expected text from the + ledger's committed blob at the run's baseline commit — what `reset --hard` actually + republished, which is nobody's concurrent write — probed out of git before the lock, since + a lock may never span a subprocess. The post-reset observation is kept only where it was + always safe: authorizing a skip, never a write. Where no anchor can be derived — no + baseline commit, an external ledger, or a probe fault, journaled + `ledger-baseline-probe-failed` — each site degrades in its own direction rather than + writing: the retraction skips (`ledger-restore-skipped-diverged`), the defer restore merges + by appending the entries disk has since lost (`defer-ledger-restore-diverged`), and the + sweep escalates for a human (`sweep-migration-restore-diverged`). Those doubly-uncertain + cases previously still wrote. What stays unfixable in principle, and is documented rather + than claimed: a rival whose text is byte-equal to the committed blob — or, on the sweep's + untracked ledger, to the rewrite the attempt just rejected — is indistinguishable from the + reset's own work. ### Security diff --git a/docs/FEATURES.md b/docs/FEATURES.md index c5c6c5d0..68b23b75 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -36,7 +36,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se - Automated per-story pipeline: `dev → verify → review → verify → commit`, end-to-end, no human in the loop. - Deterministic control flow in plain Python — story selection, retry budgets, gate checks, and completion checks are code, not an LLM session. - Owns `sprint-status.yaml`, the single source of truth: `bmad-sprint-planning` generates it, and while a run is in flight the orchestrator is its sole writer (`sprintstatus.advance` — idempotent, never-regress) while the dev and review sessions it dispatches are told never to write or revert it; your own BMAD skill runs still edit the board outside a run. Selects the next `ready-for-dev` story; advances by epic/story. -- Two orchestrator processes can no longer interleave a board advance (#286/#469): `sprintstatus.advance` holds an advisory cross-process lock across all three of its reads and its write, so the never-regress decision and the bytes that decision is applied to can no longer be separated by somebody else's write. Being the board's sole write path was never mutual exclusion on its own — a second `bmad-loop` process runs that same sole writer. Readers stay lock-free, a board that does not exist is still reported missing without a lock being created at all, and a lock that cannot be taken fails the advance on the channel that already carries its errors rather than rewriting the board unserialized. Same sidecar mechanism as the deferred-work ledger, described under _Deferred-work sweeps_. +- Two orchestrator processes can no longer interleave a board advance (#286/#469): `sprintstatus.advance` holds an advisory cross-process lock across all three of its reads and its write, so the never-regress decision and the bytes that decision is applied to can no longer be separated by somebody else's write. Being the board's sole write path was never mutual exclusion on its own — a second `bmad-loop` process runs that same sole writer. Readers stay lock-free, a board that does not exist is still reported missing without a lock being created at all, and a lock that cannot be taken fails the advance on the channel that already carries its errors rather than rewriting the board unserialized. The hold covers every read that decides the **published bytes**, which is deliberately narrower than every read the call makes: an advance that would write nothing — an absent row, or a row already at or past target — is answered from one advisory pre-lock read and takes no lock at all (#736), so an idempotent replay such as `bmad-loop confirm` against a story the board already records as done cannot fail on contention, or on a state root it never needed. The probe runs the same never-regress predicate the locked body applies, so it cannot answer "no write" where the writer would write; every other answer, and any fault while probing, falls through to the hold, which re-reads and decides authoritatively. Same sidecar mechanism as the deferred-work ledger, described under _Deferred-work sweeps_. - Scoping flags: `--epic N`, `--story KEY`, `--max-stories N`, `--dry-run` (prints the plan, spawns nothing). ### Spec + implementation (dev stage) @@ -136,7 +136,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se - Repeat mode (`--repeat` / `[sweep] repeat`): re-triages after each cycle to absorb newly generated deferred work, stopping when a cycle does nothing addressable or hits `max_cycles`. - Ledger archiving (`sweep --archive`, optionally `--before DATE` to bound by close date; `--dry-run` previews): moves closed (`status: done `) entries to sibling `deferred-work-archive.md` (body preserved, an `archived: ` marker appended), leaving an id-preserving stub (`status: done ` + `archived: `) so grep and `closes_deferred` cross-references keep resolving and the live ledger stays proportional to open work rather than all history. The stub keeps load-bearing field lines — `gate:` (validate's closed-gate report keeps speaking), `origin:`/`source_spec:` (the harvest-replay dedupe stays status-agnostic), and the reopenable-close undo tail (a paused sweep's bundle rollback still works). Reopening an archived stub — what that rollback does — demotes its `archived:` stamp to `archived-body:` rather than dropping it: the entry is live work again, so the stamp would be a lie and its shape would strand the entry outside every future archive, but the body its close moved out is still in the archive file and that line is what a later triage follows to it. Deterministic, no LLM, atomic writes with crash-safe ordering (archive before ledger; a retry keyed on id + close date completes the move without duplicate bodies). Refuses while any engine run is live or its liveness is unverifiable — it is the one out-of-band ledger writer. When the ledger is tracked, the move is durable only once both files are committed; a gitignored ledger — the default shape — or an artifact dir outside the repo has nothing to commit. Unrelated to `bmad-loop archive ` (run-tarball archiving). - Sweeps are their own resumable runs (`bmad-loop resume `). An escalated bundle resolves like a story escalation, including intent-gap patch-restore: `bmad-loop resolve --restore-patch ` re-arms the bundle spec to `in-review` and the re-driven bundle session resumes review on the re-applied patch instead of re-implementing. -- Ledger writes serialize across processes (#286/#469). Every orchestrator mutation of `deferred-work.md` — an append, a close, a reopen, a recorded decision, `sweep --archive`'s two-file rewrite — holds an advisory lock for its whole read-modify-write, so a second `bmad-loop run`, a run plus a sweep, or a run plus the TUI decision modal can no longer both read, both edit, and let the last atomic write win (lost entries, silently reverted closures, two appenders minting the same `DW-`); multi-row work is batched into one locked pass rather than one per row. The lock is a sidecar under the state root (`/locks/-.lock`), never beside the ledger, because the ledger is tracked by design and the engine stages with `git add -A`; it is keyed on the resolved path, so every spelling of one file contends on one lock while two worktrees' in-tree ledgers correctly get their own. Readers stay lock-free — every writer already replaces the file atomically, so a reader sees one whole version or another. The wait is platform-asymmetric: POSIX blocks, while Windows bounds it at roughly ten seconds and then surfaces contention as an error rather than proceeding unlocked. A dev or review session's own ledger writes are deliberately outside this — the orchestrator sequences its writes against the sessions it dispatches. The rollback paths that span a `git reset --hard` cannot be covered by a lock at all, so each is instead compare-and-set against the ledger as observed the instant the rollback returned, degrading to a journaled `defer-ledger-restore-diverged`, `ledger-restore-skipped-diverged` or `sweep-migration-restore-diverged` rather than writing over a concurrent writer; and a failed commit reopens exactly the entries the story itself closed, journaling `deferred-close-rolled-back` with their ids and `deferred-close-reopen-unmatched` for an entry whose undo marker a foreign edit has displaced. +- Ledger writes serialize across processes (#286/#469). Every orchestrator mutation of `deferred-work.md` — an append, a close, a reopen, a recorded decision, `sweep --archive`'s two-file rewrite — holds an advisory lock for its whole read-modify-write, so a second `bmad-loop run`, a run plus a sweep, or a run plus the TUI decision modal can no longer both read, both edit, and let the last atomic write win (lost entries, silently reverted closures, two appenders minting the same `DW-`); multi-row work is batched into one locked pass rather than one per row. The lock is a sidecar under the state root (`/locks/-.lock`), never beside the ledger, because the ledger is tracked by design and the engine stages with `git add -A`; it is keyed on the resolved path, so every spelling of one file contends on one lock while two worktrees' in-tree ledgers correctly get their own. Readers stay lock-free — every writer already replaces the file atomically, so a reader sees one whole version or another. The wait is platform-asymmetric: POSIX blocks, while Windows bounds it at roughly ten seconds and then surfaces contention as an error rather than proceeding unlocked. A dev or review session's own ledger writes are deliberately outside this — the orchestrator sequences its writes against the sessions it dispatches. Here too the hold covers every read that decides the **published bytes** rather than every read: a mutator whose work turns out to be a no-op — ids already done, a decision on an entry that is not there, specs that all dedupe, nothing eligible to archive, or no ledger at all — is answered from one advisory pre-lock read running the same pure decision helper the locked pass runs, and takes no lock (#736), so a replayed rollback or a `sweep --archive` over a ledger holding nothing closed cannot fail on a lock it had no write to serialize. Only a would-write-nothing answer is acted on; anything else, and any fault while probing, falls through to the hold. `append_entries_published` deliberately keeps no missing-ledger answer — an absent ledger there means create, which is a write. The rollback paths that span a `git reset --hard` cannot be covered by a lock at all, so each is instead compare-and-set — and the **write** is anchored on the ledger's committed blob at the run's baseline commit, the text that reset republished, read out of git before the lock rather than off a working tree a rival could have written inside the reset window (#735); the sweep's untracked ledger, which has no blob, anchors instead on the rejected rewrite the attempt itself graded. A post-reset observation still authorizes the **skips** — declining to act is safe whoever wrote those bytes — and never a write. Divergence, and an anchor no probe could derive (journaled `ledger-baseline-probe-failed`, from an unreadable blob, an external ledger or a run with no baseline commit), degrade each site in its own direction rather than writing over a concurrent writer: `defer-ledger-restore-diverged` merges by appending the entries disk has since lost, `ledger-restore-skipped-diverged` skips, and `sweep-migration-restore-diverged` escalates for a human to re-run the sweep. And a failed commit reopens exactly the entries the story itself closed, journaling `deferred-close-rolled-back` with their ids and `deferred-close-reopen-unmatched` for an entry whose undo marker a foreign edit has displaced. ### Stories mode (folder+id dispatch) diff --git a/tests/test_engine.py b/tests/test_engine.py index cfe005d1..bdb1b833 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -6709,8 +6709,20 @@ def test_defer_skips_restore_for_a_ledger_the_reset_never_touched(project, monke The guard this replaces compared disk against the snapshot and overwrote on exactly that difference: it ARMED the lost update it reads like it prevents. - Ablation: delete the `_ledger_is_gits_to_restore` gate and the rival entry is - clobbered by the snapshot. + Ablation: delete the `_ledger_is_gits_to_restore` gate and `probed` stops + being empty — an untracked ledger reaches the baseline probe, spawning git + and then taking the ledger lock for a restore with nothing to restore. + + That probe count is the oracle, and deliberately, because the DATA oracles + below no longer grade this gate at all. Since #735 the write arm is + `anchored and current == expected`, and an untracked ledger has no blob at + the baseline, so `expected` is None and the arm cannot fire whether the gate + runs or not: control falls through to the append-only merge, which finds + nothing the snapshot has and disk has lost, and writes nothing. Deleting the + gate used to clobber DW-2 — the consequence this docstring claimed — and now + costs only a spawn and an acquisition. The rival's survival is still asserted + because it is the behavior that matters; it is simply no longer this + ablation's discriminator. """ from conftest import review_effect as make_review @@ -6744,6 +6756,18 @@ def rollback_then_rival(task, **kwargs): monkeypatch.setattr(engine, "_rollback_or_pause", rollback_then_rival) + probed: list[str] = [] + real_baseline = engine._ledger_baseline_text + + def recording_baseline(task): + # The gate's first observable: reaching this at all means the restore is + # about to spawn git and take the ledger lock for a file `reset --hard` + # never touched. + probed.append(task.story_key) + return real_baseline(task) + + monkeypatch.setattr(engine, "_ledger_baseline_text", recording_baseline) + writes: list[Path] = [] real_write = platform_util.atomic_write_text @@ -6756,6 +6780,8 @@ def recording_write(path, text, **kwargs): summary = engine.run() assert summary.deferred == 1 + # short-circuited above the probe, so above the lock too + assert probed == [] # the restore returned before writing: nothing of ours was owed here assert project.deferred_work not in writes entries = _ledger_entries(project) diff --git a/tests/test_sprintstatus_advance.py b/tests/test_sprintstatus_advance.py index 7c7d4f4f..cce63b8d 100644 --- a/tests/test_sprintstatus_advance.py +++ b/tests/test_sprintstatus_advance.py @@ -759,7 +759,15 @@ def test_a_racing_writers_flip_survives_a_concurrent_advance(tmp_path): `atomic_write_bytes` call alone — and this reddens, `4-1-thing` coming back `review`. Hoisting read#1 (`story_status`) alone does NOT redden this row: that read decides never-regress, it does not produce the bytes published over - the rival, and its position is graded by the ordering row above instead.""" + the rival, and its position is graded by the ordering row above instead. + + Read#1 above the lock is no longer hypothetical — it is what `advance` does + (#736): the advisory probe reads exactly that, and this row is why doing so + is safe. The probe declines to answer a row that must move (`3-2-digest- + delivery` is `backlog`, below its target), so this call falls through to the + hold and recomputes read#1 there, which is the read the rival's flip has to + be visible to. Only a would-write-nothing answer is ever taken from the + probe, and such a call publishes no bytes for a rival to lose.""" p = _write(tmp_path) real_lock = sprintstatus._board_lock raced: list[str | None] = [] From aca471a2afe360309c8ef67e74fc8f38eaa8a37a Mon Sep 17 00:00:00 2001 From: t Date: Wed, 26 Aug 2026 12:09:07 -0700 Subject: [PATCH 07/13] fix(engine): anchor a proven-external ledger rather than refusing (#735) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_ledger_rel` answers three ways on purpose — derived, fault, and proven external — but `_ledger_baseline_text` collapsed the last two into "no anchor". Proven external is determinate, not uncertain: the path resolved cleanly and still fell outside the root, so no revision of this repo can name it and `reset --hard` cannot have republished it. That is the same determinate absence as a baseline commit that lacks the path, and it now answers the same way. Both engine sites gate on `_ledger_is_gits_to_restore` before probing, so neither could reach the collapsed answer. The sweep's migration restore does not, and its own comment already claimed the not-git-owned case anchors on the rejected rewrite — for an untracked ledger inside the tree it did, via `(True, None)`; for an external one it did not. A failed migration over an `implementation_artifacts` dir configured outside the repo tree — a supported shape `ProjectPaths.rebased` deliberately leaves put — therefore stranded the half-migrated rewrite on disk and escalated with "the ledger changed underneath the failed migration attempt", which nothing had. Reported by CodeRabbit on PR #737; reproduced before the fix. --- CHANGELOG.md | 17 ++++++++++------- docs/FEATURES.md | 2 +- src/bmad_loop/engine.py | 21 +++++++++++++++++---- tests/test_sweep.py | 29 +++++++++++++++++++++++++++++ 4 files changed, 57 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b6fd585c..a7c423d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -263,17 +263,20 @@ decisions` and the TUI decision modal now also catch the state-root failure that overwrites the rival's entries. Each write arm now takes its expected text from the ledger's committed blob at the run's baseline commit — what `reset --hard` actually republished, which is nobody's concurrent write — probed out of git before the lock, since - a lock may never span a subprocess. The post-reset observation is kept only where it was - always safe: authorizing a skip, never a write. Where no anchor can be derived — no - baseline commit, an external ledger, or a probe fault, journaled - `ledger-baseline-probe-failed` — each site degrades in its own direction rather than - writing: the retraction skips (`ledger-restore-skipped-diverged`), the defer restore merges + a lock may never span a subprocess. A ledger git never had — untracked, or configured + outside the repo tree, which is a supported shape — has no blob to anchor on, but is + equally beyond `reset --hard`'s reach; that is determinate absence rather than + uncertainty, so the sweep's restore anchors it on the rewrite the attempt actually graded. + The post-reset observation is kept only where it was always safe: authorizing a skip, + never a write. Where no anchor can be derived — no baseline commit, or a probe fault, + journaled `ledger-baseline-probe-failed` — each site degrades in its own direction rather + than writing: the retraction skips (`ledger-restore-skipped-diverged`), the defer restore merges by appending the entries disk has since lost (`defer-ledger-restore-diverged`), and the sweep escalates for a human (`sweep-migration-restore-diverged`). Those doubly-uncertain cases previously still wrote. What stays unfixable in principle, and is documented rather than claimed: a rival whose text is byte-equal to the committed blob — or, on the sweep's - untracked ledger, to the rewrite the attempt just rejected — is indistinguishable from the - reset's own work. + untracked or external ledger, to the rewrite the attempt just rejected — is + indistinguishable from the reset's own work. ### Security diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 68b23b75..c2bc69c7 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -36,7 +36,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se - Automated per-story pipeline: `dev → verify → review → verify → commit`, end-to-end, no human in the loop. - Deterministic control flow in plain Python — story selection, retry budgets, gate checks, and completion checks are code, not an LLM session. - Owns `sprint-status.yaml`, the single source of truth: `bmad-sprint-planning` generates it, and while a run is in flight the orchestrator is its sole writer (`sprintstatus.advance` — idempotent, never-regress) while the dev and review sessions it dispatches are told never to write or revert it; your own BMAD skill runs still edit the board outside a run. Selects the next `ready-for-dev` story; advances by epic/story. -- Two orchestrator processes can no longer interleave a board advance (#286/#469): `sprintstatus.advance` holds an advisory cross-process lock across all three of its reads and its write, so the never-regress decision and the bytes that decision is applied to can no longer be separated by somebody else's write. Being the board's sole write path was never mutual exclusion on its own — a second `bmad-loop` process runs that same sole writer. Readers stay lock-free, a board that does not exist is still reported missing without a lock being created at all, and a lock that cannot be taken fails the advance on the channel that already carries its errors rather than rewriting the board unserialized. The hold covers every read that decides the **published bytes**, which is deliberately narrower than every read the call makes: an advance that would write nothing — an absent row, or a row already at or past target — is answered from one advisory pre-lock read and takes no lock at all (#736), so an idempotent replay such as `bmad-loop confirm` against a story the board already records as done cannot fail on contention, or on a state root it never needed. The probe runs the same never-regress predicate the locked body applies, so it cannot answer "no write" where the writer would write; every other answer, and any fault while probing, falls through to the hold, which re-reads and decides authoritatively. Same sidecar mechanism as the deferred-work ledger, described under _Deferred-work sweeps_. +- Two orchestrator processes can no longer interleave a board advance (#286/#469): `sprintstatus.advance` holds an advisory cross-process lock across every read that decides the **published bytes** and across the write itself, so the never-regress decision and the bytes that decision is applied to can no longer be separated by somebody else's write. Being the board's sole write path was never mutual exclusion on its own — a second `bmad-loop` process runs that same sole writer. Readers stay lock-free, a board that does not exist is still reported missing without a lock being created at all, and a lock that cannot be taken fails the advance on the channel that already carries its errors rather than rewriting the board unserialized. That scope is deliberately narrower than every read the call makes: an advance that would write nothing — an absent row, or a row already at or past target — is answered from one advisory pre-lock read and takes no lock at all (#736), so an idempotent replay such as `bmad-loop confirm` against a story the board already records as done cannot fail on contention, or on a state root it never needed. The probe runs the same never-regress predicate the locked body applies, so it cannot answer "no write" where the writer would write; every other answer, and any fault while probing, falls through to the hold, which re-reads and decides authoritatively. Same sidecar mechanism as the deferred-work ledger, described under _Deferred-work sweeps_. - Scoping flags: `--epic N`, `--story KEY`, `--max-stories N`, `--dry-run` (prints the plan, spawns nothing). ### Spec + implementation (dev stage) diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index baa9e824..47309f08 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -4636,9 +4636,10 @@ def _ledger_baseline_text(self, task: StoryTask) -> tuple[bool, str | None]: Answers ``(True, text)`` when the baseline commit carries the ledger, ``(True, None)`` when it determinately does not — the reset leaves no - tracked file behind — and ``(False, None)`` when no anchor could be - derived at all: no baseline commit, no repo-relative name, or a probe - that failed. + tracked file behind, whether because the commit lacks the path or + because the ledger is proven external and no revision of this repo can + name it — and ``(False, None)`` when no anchor could be derived at all: + no baseline commit, or a probe that failed. Newlines are normalized to LF because the only thing this text is ever compared against is :meth:`_ledger_text`, which reads in Python's @@ -4669,7 +4670,19 @@ def _ledger_baseline_text(self, task: StoryTask) -> tuple[bool, str | None]: story_key=task.story_key, error=str(fault), ) - return False, None + return False, None + # PROVEN external — it resolved cleanly and still fell outside the + # root — which is determinate absence, not uncertainty: no revision + # of this repo can name the path, so `reset --hard` cannot have + # republished it. Same answer as a baseline commit that does not + # carry the ledger, and for the same reason; the caller supplies the + # anchor for a file git never had. Collapsing this into the fault + # answer withholds the anchor from a SUPPORTED shape — an + # `implementation_artifacts` dir configured outside the repo tree, + # which `ProjectPaths.rebased` deliberately leaves put — and strands + # the sweep's migration restore on an unprovable-anchor refusal that + # the evidence does not support. + return True, None try: blob = verify.worktree_file_bytes_at_revision( self.workspace.root, task.baseline_commit, rel diff --git a/tests/test_sweep.py b/tests/test_sweep.py index c9fe7c31..9a44c3a0 100644 --- a/tests/test_sweep.py +++ b/tests/test_sweep.py @@ -31,6 +31,7 @@ from bmad_loop import verify from bmad_loop.adapters.base import SessionResult from bmad_loop.adapters.mock import MockAdapter +from bmad_loop.bmadconfig import ProjectPaths from bmad_loop.journal import Journal, load_state, save_state from bmad_loop.model import PAUSE_STORY_GATE, Phase, RunState, StoryTask, TokenUsage from bmad_loop.policy import ( @@ -5391,3 +5392,31 @@ def test_reopen_after_defer_uses_one_lock(project, monkeypatch): assert len(reopened) == 1 assert reopened[0]["dw_ids"] == ["DW-1", "DW-2", "DW-3"] assert reopened[0]["story_key"] == "dw-fix" + + +def test_migration_restore_writes_back_an_external_ledger(project, tmp_path): + """#735 follow-up. An `implementation_artifacts` dir configured OUTSIDE the + repo tree is a supported shape — `ProjectPaths.rebased` deliberately leaves + such dirs put, because they are shared rather than per-checkout — so the + ledger can resolve outside `workspace.root`. + + `git reset --hard` provably cannot have touched a path no revision of this + repo can even name, which is the same proof the untracked-inside-the-tree + case relies on. So the anchor is the rejected rewrite this attempt graded, + the restore puts the pre-migration text back, and no divergence is journaled. + Escalating here would strand the half-migrated rewrite on disk for a + configuration that is not ambiguous at all. + """ + external = ProjectPaths( + project=project.project, + implementation_artifacts=tmp_path / "shared-artifacts" / "implementation-artifacts", + planning_artifacts=project.planning_artifacts, + ) + external.implementation_artifacts.mkdir(parents=True) + write_legacy_ledger(external, LEGACY_LEDGER, commit=False) + engine, _ = make_sweep(external, [_half_migrated_effect(external)] * 2) + summary = engine.run() + + assert summary.paused + assert external.deferred_work.read_text(encoding="utf-8") == LEGACY_LEDGER + assert "sweep-migration-restore-diverged" not in journal_kinds(engine) From 6a7a508bcf8f892f4fea6f22b387d86d67665c85 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 26 Aug 2026 12:23:28 -0700 Subject: [PATCH 08/13] fix(engine): stop a symlinked ledger's blob anchor being never-true (#735) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Git stores a tracked symlink as a blob holding the TARGET PATHNAME, and `worktree_file_bytes_at_revision` filters on type `blob` without looking at the mode. A ledger symlinked into the repo therefore anchored on a pathname compared against ledger text — silently never-true, the same failure mode the newline normalization already guards against. That shape is supported on purpose: `atomic_write_text` follows symlinks by DEFAULT precisely so such a ledger "keeps being a symlink and the real file is what gets rewritten". `reset --hard` restores the link and cannot reach through it, so the reset republishes no ledger text there — the existing `verify.path_is_non_regular_at_revision` names exactly that shape, and it now answers determinate absence like a proven-external ledger, leaving each caller its own anchor. Reproduced before the fix, and the new test ablated against it. Also condenses the two #735/#736 changelog entries (21 and 24 continuation lines) toward the section's median of 5, per AGENTS.md's terse/scannable rule. Headlines stay bold noun phrases, which is the section's actual form in 154 of 154 entries. Both reported by codex on PR #737. --- CHANGELOG.md | 68 +++++++++++++++-------------------------- src/bmad_loop/engine.py | 27 +++++++++++++--- tests/test_sweep.py | 36 ++++++++++++++++++++++ 3 files changed, 82 insertions(+), 49 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a7c423d3..4ae8b94d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -233,50 +233,30 @@ decisions` and the TUI decision modal now also catch the state-root failure that created inside the window went with it. A tracked ledger absent at snapshot time is still never deleted, and is now answered before any lock is taken. A write or lock fault is journaled as `ledger-restore-failed` and preserves an in-flight pause, as before. -- **A read-dependent no-op is answered before its lock is taken** (#736). A lock acquired for - work that turns out to write nothing turns a previously successful no-op into a failure: - `bmad-loop confirm` replayed against a story the board already records as `done`, a rollback - replayed over entries already reopened, `sweep --archive` over a ledger holding nothing - closed. Acquisition can fail, and so can deriving the sidecar path where no state root - exists — on work that was never going to happen. #726 fixed two instances of the shape (a - missing ledger, an empty batch); the rest of the class is swept here. `sprintstatus.advance` - and the five read-dependent `deferred-work.md` mutators each take ONE advisory read before - acquiring, running the same pure decision helper the locked pass runs so the probe cannot - answer "no write" where the authority would write. Only a would-write-nothing answer is - acted on, and such a call linearizes at that read: it publishes no bytes, so there is - nothing for a rival to interleave with. Every other answer — and any fault while probing — - falls through to the hold, which re-reads and decides authoritatively, so a malformed board - still raises from under the lock and the probe adds no failure mode the locked path lacks. - `mark_done_many`, `mark_open_many` and `record_decision` also answer a missing ledger - without acquiring, as `archive_closed` already did; `append_entries_published` deliberately - does not, because an absent ledger there means CREATE, which is a write. - With nothing eligible to archive, `bmad-loop sweep --archive` therefore now exits 0 ("no - closed entries to archive") where a dead lock or an underivable state root made it exit 1. - An ELIGIBLE archive under a dead lock still fails exactly as before, as does every write - arm. -- **A ledger restore that spans `git reset --hard` anchors on the committed baseline blob, - not on an observation of the tree** (#735). The three restores no lock may cover — the - rejected attempt's retraction, a rolled-back defer's, and the sweep's failed-migration - rewrite — each compared against the ledger as observed the instant the rollback returned. - That read is taken after the very reset it attests to, so a rival writing a tracked ledger - inside the window BECOMES the observation, the compare then holds, and the restore - overwrites the rival's entries. Each write arm now takes its expected text from the - ledger's committed blob at the run's baseline commit — what `reset --hard` actually - republished, which is nobody's concurrent write — probed out of git before the lock, since - a lock may never span a subprocess. A ledger git never had — untracked, or configured - outside the repo tree, which is a supported shape — has no blob to anchor on, but is - equally beyond `reset --hard`'s reach; that is determinate absence rather than - uncertainty, so the sweep's restore anchors it on the rewrite the attempt actually graded. - The post-reset observation is kept only where it was always safe: authorizing a skip, - never a write. Where no anchor can be derived — no baseline commit, or a probe fault, - journaled `ledger-baseline-probe-failed` — each site degrades in its own direction rather - than writing: the retraction skips (`ledger-restore-skipped-diverged`), the defer restore merges - by appending the entries disk has since lost (`defer-ledger-restore-diverged`), and the - sweep escalates for a human (`sweep-migration-restore-diverged`). Those doubly-uncertain - cases previously still wrote. What stays unfixable in principle, and is documented rather - than claimed: a rival whose text is byte-equal to the committed blob — or, on the sweep's - untracked or external ledger, to the rewrite the attempt just rejected — is - indistinguishable from the reset's own work. +- **A read-dependent no-op is answered before its lock is taken** (#736). Taking a lock for + work that turns out to write nothing could fail a call that used to succeed — a replayed + `bmad-loop confirm` against a story the board already records as `done`, a replayed + rollback, `sweep --archive` over a ledger holding nothing closed — either on contention or + on deriving a sidecar path where no state root exists. `sprintstatus.advance` and the five + read-dependent `deferred-work.md` mutators now take one advisory read first, running the + same pure decision helper the locked pass runs; only a writes-nothing answer skips the lock, + and every other answer, plus any probe fault, falls through to the hold and decides there. + `sweep --archive` with nothing eligible now exits 0 rather than 1; an eligible archive under + a dead lock still fails as before. `append_entries_published` deliberately still locks on a + missing ledger, where absence means create. +- **A ledger restore that spans `git reset --hard` anchors on the committed baseline blob** + (#735). The three restores no lock may cover — the rejected attempt's retraction, a rolled + back defer's, and the sweep's failed-migration rewrite — compared the ledger against an + observation read after the reset returned. A rival writing a tracked ledger inside that + window became the observation, so the compare held and the restore overwrote its entries. + Each write arm now takes its expected text from the ledger's blob at the baseline commit, + probed before the lock, since a lock may never span a subprocess. Where the reset + republishes no text at all — an untracked ledger, one configured outside the repo tree, or + one symlinked into it, whose blob is a target pathname rather than ledger text — the anchor + is the rewrite the attempt actually graded. Where no anchor can be derived, the retraction + skips, the defer restore merges what disk has lost, and the sweep escalates; those + doubly-uncertain cases previously still wrote. A rival whose text is byte-equal to the + anchor stays indistinguishable from the reset's own work. ### Security diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index 47309f08..a8ba9d7f 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -4635,11 +4635,12 @@ def _ledger_baseline_text(self, task: StoryTask) -> tuple[bool, str | None]: """The ledger text ``reset --hard`` republishes, taken from git itself (#735). Answers ``(True, text)`` when the baseline commit carries the ledger, - ``(True, None)`` when it determinately does not — the reset leaves no - tracked file behind, whether because the commit lacks the path or - because the ledger is proven external and no revision of this repo can - name it — and ``(False, None)`` when no anchor could be derived at all: - no baseline commit, or a probe that failed. + ``(True, None)`` when it determinately does not — the reset republishes + no ledger text, whether because the commit lacks the path, because the + ledger is proven external and no revision of this repo can name it, or + because the baseline holds a non-regular entry (a symlink, whose blob is + a pathname rather than ledger text) — and ``(False, None)`` when no + anchor could be derived at all: no baseline commit, or a probe failed. Newlines are normalized to LF because the only thing this text is ever compared against is :meth:`_ledger_text`, which reads in Python's @@ -4684,6 +4685,22 @@ def _ledger_baseline_text(self, task: StoryTask) -> tuple[bool, str | None]: # the evidence does not support. return True, None try: + if verify.path_is_non_regular_at_revision( + self.workspace.root, task.baseline_commit, rel + ): + # A symlink, gitlink or tree at the baseline is a path whose + # CONTENTS the reset never republished: `reset --hard` restores + # the link itself and cannot reach through it to revert what it + # points at. Behind mode 120000 the blob is the TARGET PATHNAME, + # so trusting it here would compare a pathname against ledger + # text and leave the anchor silently never-true — the same + # failure mode the newline normalization above exists to prevent, + # and one that would escalate every failed migration over a + # ledger symlinked into the repo. That shape is supported on + # purpose: `atomic_write_text` follows symlinks by DEFAULT so + # such a ledger keeps being a symlink. Determinate absence of + # republished text, exactly like a proven-external ledger. + return True, None blob = verify.worktree_file_bytes_at_revision( self.workspace.root, task.baseline_commit, rel ) diff --git a/tests/test_sweep.py b/tests/test_sweep.py index 9a44c3a0..11e6e56d 100644 --- a/tests/test_sweep.py +++ b/tests/test_sweep.py @@ -5420,3 +5420,39 @@ def test_migration_restore_writes_back_an_external_ledger(project, tmp_path): assert summary.paused assert external.deferred_work.read_text(encoding="utf-8") == LEGACY_LEDGER assert "sweep-migration-restore-diverged" not in journal_kinds(engine) + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") +def test_migration_restore_writes_back_a_symlinked_ledger(project, tmp_path): + """#735 follow-up. A ledger symlinked into the repo is a supported shape — + `atomic_write_text` follows symlinks BY DEFAULT precisely so such a ledger + "keeps being a symlink and the real file is what gets rewritten". + + Git stores that tracked symlink as a blob holding the TARGET PATHNAME, so a + baseline anchor taken from the blob would compare a pathname against ledger + text and never be true — escalating every failed migration over a shape that + is not ambiguous at all. `reset --hard` restores the link, never what it + points at, so the reset republished no ledger text and the anchor is the + rejected rewrite this attempt graded, as for an untracked or external ledger. + + Ablation: drop the `path_is_non_regular_at_revision` arm from + `_ledger_baseline_text` and this reddens on the restored-text assertion, with + `sweep-migration-restore-diverged` journaled and the half-migrated rewrite + left standing. + """ + target = tmp_path / "shared" / "deferred-work.md" + target.parent.mkdir(parents=True) + target.write_text(LEGACY_LEDGER, encoding="utf-8") + if project.deferred_work.is_symlink() or project.deferred_work.exists(): + project.deferred_work.unlink() + project.deferred_work.symlink_to(target) + git(project.project, "add", "-A") + git(project.project, "commit", "-q", "-m", "symlinked ledger") + engine, _ = make_sweep(project, [_half_migrated_effect(project)] * 2) + summary = engine.run() + + assert summary.paused + # the link survived the round trip, and the real file holds the restored text + assert project.deferred_work.is_symlink() + assert target.read_text(encoding="utf-8") == LEGACY_LEDGER + assert "sweep-migration-restore-diverged" not in journal_kinds(engine) From b5e1938ab612bf194f64c12634ca6abab357fb53 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 26 Aug 2026 12:39:14 -0700 Subject: [PATCH 09/13] fix(engine,sweep): split the baseline anchor's three states (#735) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_ledger_baseline_text` answered a bool, which merged two different facts into one write authorization: * the baseline determinately has no ledger, so `reset --hard` DELETED it and a missing file on disk is the reset's own work; and * the reset republished no text here at all (proven external, or a non-regular baseline entry such as a symlink), where a missing file means somebody deleted the target. The symlink arm added in 6a7a508b returned the first for the second. A symlinked ledger IS tracked, so unlike the external case it does not short-circuit at `_ledger_is_gits_to_restore` — it reached both engine restore arms, where `None == None` then read a rival's deletion as reset-owned and wrote the snapshot back over it. That inverts this program's own rule: observation may justify a skip, never a write. `_LedgerAnchor` now carries BASELINE / NO_RESET_CONTENT / NONE. Only BASELINE authorizes a reset-owned write; the sweep accepts either, because it supplies its own anchor (the rejected rewrite) and so never compares against a bare `None`. The defer restore degrades to its append-only merge, which cannot destroy a rival's write. New direction pin ablated: widening the arm to `is not NONE` undoes the rival's deletion and reddens the test. Reported by codex on PR #737. --- src/bmad_loop/engine.py | 77 +++++++++++++++++++++++++++++++---------- src/bmad_loop/sweep.py | 10 ++++-- tests/test_engine.py | 68 ++++++++++++++++++++++++++++++++---- 3 files changed, 127 insertions(+), 28 deletions(-) diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index a8ba9d7f..3c5d9cb9 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -20,6 +20,7 @@ import time import traceback from dataclasses import dataclass +from enum import StrEnum from pathlib import Path from typing import TYPE_CHECKING, Callable, NamedTuple, NoReturn, Protocol, Sequence @@ -477,6 +478,35 @@ class _ArmedClose(NamedTuple): exact: bool +class _LedgerAnchor(StrEnum): + """How much authority a baseline probe established for a ledger restore (#735). + + Three states, because the domain has three and a boolean silently merged two + of them into a write: + + ``BASELINE`` — ``reset --hard`` republished the baseline's own content at + this path, so the accompanying text (or its determinate absence, ``None``) + is what the reset itself put there. Only this state may authorize a + reset-owned WRITE. + + ``NO_RESET_CONTENT`` — the path is real but the reset republished no ledger + TEXT for it: a ledger proven external, or one the baseline holds as a + non-regular entry (a symlink, whose blob is a target pathname and whose + target the reset cannot reach through). There is nothing of the reset's to + compare against, so a caller with an anchor of its own — the sweep's + rejected rewrite — may use it, and a caller without one must not treat a + missing file as the reset's work. ``None`` here means "no text to offer", + NEVER "the reset deleted it". + + ``NONE`` — nothing could be derived: no baseline commit, or a probe that + faulted. Authorizes nothing. + """ + + NONE = "none" + BASELINE = "baseline" + NO_RESET_CONTENT = "no-reset-content" + + class Engine: # The engine that installed the process-wide stop handlers. Signal handling is # single-owner per process; only this engine reinstalls/restores them. Run @@ -4631,16 +4661,17 @@ def _ledger_is_gits_to_restore(self, task: StoryTask) -> bool: ) return True - def _ledger_baseline_text(self, task: StoryTask) -> tuple[bool, str | None]: + def _ledger_baseline_text(self, task: StoryTask) -> tuple[_LedgerAnchor, str | None]: """The ledger text ``reset --hard`` republishes, taken from git itself (#735). - Answers ``(True, text)`` when the baseline commit carries the ledger, - ``(True, None)`` when it determinately does not — the reset republishes - no ledger text, whether because the commit lacks the path, because the - ledger is proven external and no revision of this repo can name it, or - because the baseline holds a non-regular entry (a symlink, whose blob is - a pathname rather than ledger text) — and ``(False, None)`` when no - anchor could be derived at all: no baseline commit, or a probe failed. + Answers ``(BASELINE, text)`` when the baseline commit carries the + ledger, and ``(BASELINE, None)`` when it determinately does not — the + reset removed it, so a missing file IS the reset's own work. + ``(NO_RESET_CONTENT, None)`` when the path is real but the reset + republished no text for it: proven external, or a non-regular baseline + entry such as a symlink. ``(NONE, None)`` when nothing could be derived + at all: no baseline commit, or a probe that failed. :class:`_LedgerAnchor` + carries why only the first of those may authorize a write. Newlines are normalized to LF because the only thing this text is ever compared against is :meth:`_ledger_text`, which reads in Python's @@ -4662,7 +4693,7 @@ def _ledger_baseline_text(self, task: StoryTask) -> tuple[bool, str | None]: with a secondary repair failure. """ if not task.baseline_commit: - return False, None + return _LedgerAnchor.NONE, None rel, fault = self._ledger_rel() if rel is None: if fault is not None: @@ -4671,7 +4702,7 @@ def _ledger_baseline_text(self, task: StoryTask) -> tuple[bool, str | None]: story_key=task.story_key, error=str(fault), ) - return False, None + return _LedgerAnchor.NONE, None # PROVEN external — it resolved cleanly and still fell outside the # root — which is determinate absence, not uncertainty: no revision # of this repo can name the path, so `reset --hard` cannot have @@ -4683,7 +4714,7 @@ def _ledger_baseline_text(self, task: StoryTask) -> tuple[bool, str | None]: # which `ProjectPaths.rebased` deliberately leaves put — and strands # the sweep's migration restore on an unprovable-anchor refusal that # the evidence does not support. - return True, None + return _LedgerAnchor.NO_RESET_CONTENT, None try: if verify.path_is_non_regular_at_revision( self.workspace.root, task.baseline_commit, rel @@ -4700,12 +4731,12 @@ def _ledger_baseline_text(self, task: StoryTask) -> tuple[bool, str | None]: # purpose: `atomic_write_text` follows symlinks by DEFAULT so # such a ledger keeps being a symlink. Determinate absence of # republished text, exactly like a proven-external ledger. - return True, None + return _LedgerAnchor.NO_RESET_CONTENT, None blob = verify.worktree_file_bytes_at_revision( self.workspace.root, task.baseline_commit, rel ) if blob is None: - return True, None + return _LedgerAnchor.BASELINE, None text = blob.decode("utf-8") except (verify.GitError, OSError, RuntimeError, UnicodeDecodeError) as e: self.journal.append( @@ -4713,8 +4744,8 @@ def _ledger_baseline_text(self, task: StoryTask) -> tuple[bool, str | None]: story_key=task.story_key, error=str(e), ) - return False, None - return True, text.replace("\r\n", "\n").replace("\r", "\n") + return _LedgerAnchor.NONE, None + return _LedgerAnchor.BASELINE, text.replace("\r\n", "\n").replace("\r", "\n") def _restore_ledger(self, task: StoryTask, snapshot: str | None) -> None: """Retract this attempt's engine ledger writes, without taking a concurrent @@ -4789,7 +4820,7 @@ def _restore_ledger(self, task: StoryTask, snapshot: str | None) -> None: # reset-owned at all, so an untracked, ignored or external one skips the # spawn. A fault degrades to NO anchor — the inverse of the gits probe # above, whose consumer is an unlink; this one's is a write. - anchored, expected = self._ledger_baseline_text(task) if gits else (False, None) + anchor, expected = self._ledger_baseline_text(task) if gits else (_LedgerAnchor.NONE, None) diverged = False with deferredwork.ledger_lock(ledger): # PURE TEXT ONLY under the hold. Every `deferredwork` mutator takes @@ -4799,7 +4830,11 @@ def _restore_ledger(self, task: StoryTask, snapshot: str | None) -> None: if current == snapshot: return ours = _digest_of(current) == task.post_engine_ledger_digest - reset_owned = anchored and current == expected + # BASELINE only: `NO_RESET_CONTENT` carries `None` meaning "no text + # to offer", so pairing it with a missing file would read a rival's + # deletion as the reset's own work and write the snapshot back over + # it. Observation may justify a skip, never a write. + reset_owned = anchor is _LedgerAnchor.BASELINE and current == expected if snapshot is None: # `gits` is False on this arm — the guard above returned # otherwise — so the file is untracked, ignored or external and @@ -6442,7 +6477,7 @@ def _restore_defer_ledger(self, task: StoryTask, snapshot: str) -> None: # the merge below, which is append-only and therefore cannot destroy a # rival's write — the reason this site can absorb a probe fault the way # `_restore_ledger`'s degrade-to-skip has to. - anchored, expected = self._ledger_baseline_text(task) + anchor, expected = self._ledger_baseline_text(task) merged: list[str] = [] flat_remainder = False with deferredwork.ledger_lock(ledger): @@ -6452,7 +6487,11 @@ def _restore_defer_ledger(self, task: StoryTask, snapshot: str) -> None: current = self._ledger_text() if current == snapshot: return - if anchored and current == expected: + # BASELINE only, for the reason `_restore_ledger` states: a + # `NO_RESET_CONTENT` anchor plus a missing file is a rival's + # deletion, not the reset's. The append-only merge below is the + # right degrade — it cannot destroy a rival's write. + if anchor is _LedgerAnchor.BASELINE and current == expected: ledger.parent.mkdir(parents=True, exist_ok=True) atomic_write_text(ledger, snapshot) return diff --git a/src/bmad_loop/sweep.py b/src/bmad_loop/sweep.py index 8c005c86..ffb95697 100644 --- a/src/bmad_loop/sweep.py +++ b/src/bmad_loop/sweep.py @@ -18,7 +18,7 @@ from typing import Any, Callable, Iterable from . import deferredwork, gates, verify -from .engine import Engine, RunPaused, _ArmedClose +from .engine import Engine, RunPaused, _ArmedClose, _LedgerAnchor from .escalation import critical_escalations, env_fault_pause_reason, session_failure_reason from .model import PAUSE_STORY_GATE, Phase, StoryTask from .platform_util import ( @@ -1046,14 +1046,18 @@ def _ensure_migration(self, text: str) -> None: # touched it either, so there the anchor is the rejected rewrite this # attempt actually graded — down to `None == None` when the session # deleted the ledger outright. No anchor at all withholds the write. - anchored, committed = self._ledger_baseline_text(task) + anchor, committed = self._ledger_baseline_text(task) expected = committed if committed is not None else rewrite diverged = False with deferredwork.ledger_lock(ledger): # PURE TEXT ONLY under the hold — `ledger_lock` is not reentrant # and every mutator takes it. current = ledger.read_text(encoding="utf-8") if ledger.is_file() else None - if anchored and current == expected: + # Either anchor will do HERE, unlike the engine's two restores: + # this site supplies its own text for the no-reset-content case + # (`rewrite`, which it graded), so `expected` is never the bare + # `None` that would read a rival's deletion as the reset's work. + if anchor is not _LedgerAnchor.NONE and current == expected: ledger.parent.mkdir(parents=True, exist_ok=True) atomic_write_text(ledger, text) else: diff --git a/tests/test_engine.py b/tests/test_engine.py index bdb1b833..7f2a1e25 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -37,7 +37,14 @@ from bmad_loop import deferredwork, platform_util, runs, verify from bmad_loop.adapters.base import SessionResult from bmad_loop.adapters.mock import MockAdapter -from bmad_loop.engine import Engine, RunPaused, RunStopped, _digest_of, _run_depth +from bmad_loop.engine import ( + Engine, + RunPaused, + RunStopped, + _digest_of, + _LedgerAnchor, + _run_depth, +) from bmad_loop.journal import LOGS_DIR, VERIFY_DIR, Journal, load_state from bmad_loop.model import ( PAUSE_EPIC_BOUNDARY, @@ -12906,7 +12913,7 @@ def test_ledger_baseline_text_reads_the_committed_blob(project, monkeypatch): task.baseline_commit = rev_parse_head(project.project) task.baseline_untracked = [] - assert engine._ledger_baseline_text(task) == (True, committed) + assert engine._ledger_baseline_text(task) == (_LedgerAnchor.BASELINE, committed) held: list[bool] = [] real_blob = verify.worktree_file_bytes_at_revision @@ -12945,7 +12952,10 @@ def test_ledger_baseline_text_normalizes_committed_crlf(project): task.baseline_untracked = [] anchored, expected = engine._ledger_baseline_text(task) - assert (anchored, expected) == (True, "# Deferred Work\n\n## DW-1 crlf at baseline\n") + assert (anchored, expected) == ( + _LedgerAnchor.BASELINE, + "# Deferred Work\n\n## DW-1 crlf at baseline\n", + ) # The point of the normalization: the anchor must equal what the ONLY thing # it is ever compared against reads back off those same bytes. assert expected == engine._ledger_text() @@ -12969,7 +12979,7 @@ def test_ledger_baseline_text_reports_absence_at_baseline(project): git(project.project, "add", "-A") git(project.project, "commit", "-q", "-m", "track deferred-work after the baseline") - assert engine._ledger_baseline_text(task) == (True, None) + assert engine._ledger_baseline_text(task) == (_LedgerAnchor.BASELINE, None) assert [ e for e in engine.journal.entries() if e["kind"] == "ledger-baseline-probe-failed" ] == [] @@ -12988,7 +12998,7 @@ def test_ledger_baseline_text_degrades_without_a_baseline(project): task = StoryTask(story_key="1-1-a", epic=1) assert task.baseline_commit is None - assert engine._ledger_baseline_text(task) == (False, None) + assert engine._ledger_baseline_text(task) == (_LedgerAnchor.NONE, None) assert [ e for e in engine.journal.entries() if e["kind"] == "ledger-baseline-probe-failed" ] == [] @@ -13024,7 +13034,7 @@ def fail_probe(*args, **kwargs): monkeypatch.setattr(verify, "worktree_file_bytes_at_revision", fail_probe) - assert engine._ledger_baseline_text(task) == (False, None) + assert engine._ledger_baseline_text(task) == (_LedgerAnchor.NONE, None) (event,) = [e for e in engine.journal.entries() if e["kind"] == "ledger-baseline-probe-failed"] assert event["story_key"] == "1-1-a" assert "injected baseline probe failure" in event["error"] @@ -13066,6 +13076,52 @@ def test_restore_ledger_reset_owned_write_uses_the_blob_anchor(project): ] == [] +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") +def test_restore_ledger_never_reads_a_symlink_deletion_as_reset_owned(project, tmp_path): + """DIRECTION PIN (#735). A tracked ledger symlink whose TARGET a rival deleted + inside the reset window must not be written back over. + + `reset --hard` restores the link and cannot reach through it, so it never + republished any ledger text here — the anchor is `NO_RESET_CONTENT`, whose + `None` means "no text to offer", not "the reset removed it". Pairing that + `None` with the `None` a dangling link reads back would make the two compare + equal and undo the deletion. Only a `BASELINE` anchor may spend a missing + file as proof, because only there did the reset actually delete it — which is + exactly what `test_ledger_baseline_text_answers_determinate_absence` pins. + + The digest anchor is deliberately NOT ours, so any write would be + attributable to `reset_owned` alone. + + Ablation: widen the arm to `anchor is not _LedgerAnchor.NONE` and this reddens + on the restored-file assertion — the rival's deletion is undone. + """ + target = tmp_path / "shared" / "deferred-work.md" + target.parent.mkdir(parents=True) + target.write_text("# Deferred Work\n\n## DW-1 at baseline\n", encoding="utf-8") + project.deferred_work.parent.mkdir(parents=True, exist_ok=True) + if project.deferred_work.is_symlink() or project.deferred_work.exists(): + project.deferred_work.unlink() + project.deferred_work.symlink_to(target) + git(project.project, "add", "-A") + git(project.project, "commit", "-q", "-m", "track a symlinked deferred-work") + engine, _ = make_engine(project, [], policy=_harvest_policy()) + task = StoryTask(story_key="1-1-a", epic=1) + task.baseline_commit = rev_parse_head(project.project) + task.baseline_untracked = [] + task.post_engine_ledger_digest = _digest_of("bytes this engine never published") + # The rival's deletion, landing inside the window the reset opened. + target.unlink() + + engine._restore_ledger(task, "# Deferred Work\n\n## DW-2 this session's edit\n") + + # the deletion stands; the link was not spent as a channel to undo it + assert not target.exists() + assert project.deferred_work.is_symlink() + assert [ + e for e in engine.journal.entries() if e["kind"] == "ledger-restore-skipped-diverged" + ] != [] + + def test_restore_ledger_probe_failure_never_writes(project, monkeypatch): """DIRECTION PIN: an unprovable baseline skips, it never falls back to the observation. From 696fdbbc12e980d94476b4e9de07aa768714d139 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 26 Aug 2026 12:43:29 -0700 Subject: [PATCH 10/13] test(engine): pin the lexical-first ordering in _ledger_rel (#552, #735) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `_ledger_rel` tries the lexical `relative_to` before falling back to `resolve()`, and nothing in the suite said why. A reviewer proposed collapsing it to resolve-first on PR #737; the whole suite stayed green under that change, which is exactly the gap this row closes. Resolve-first regresses the #552 shape: a registered-but-not-serving WSL UNC provider raises WinError 64 on a path that is perfectly nameable lexically, so a derived rel becomes `(None, fault)`. The fault degrades then cost real behavior — the baseline anchor drops to NONE, the retraction skips, the defer restore falls to its merge, and the sweep escalates, all for a ledger sitting in an ordinary place in the repo. Ablated against that exact reorder: both assertions red. --- tests/test_engine.py | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/test_engine.py b/tests/test_engine.py index 7f2a1e25..28422565 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -25,6 +25,7 @@ fault_read_text, generic_dev_effect, git, + refuse_to_resolve, review_effect, set_sprint, spec_path, @@ -12889,6 +12890,39 @@ def failed_resolve(self, *args, **kwargs): assert event["story_key"] == task.story_key +def test_ledger_rel_derives_lexically_before_resolving(project, monkeypatch): + """DIRECTION PIN (#552). `_ledger_rel` tries the LEXICAL `relative_to` first and + only falls back to `resolve()`. That ordering is load-bearing, not stylistic. + + A registered-but-not-serving WSL UNC provider makes `resolve()` raise WinError + 64 on a path that is perfectly nameable lexically. Resolving FIRST would turn + that into `(None, fault)` — and the fault degrades cost real behavior: the + baseline anchor drops to `NONE`, so the retraction skips, the defer restore + falls to its merge, and the sweep escalates, all for a ledger sitting in an + ordinary place inside the repo. + + Ablation: reorder `_ledger_rel` to `return ledger.resolve().relative_to( + root.resolve()).as_posix(), None` first (the shape a reviewer proposed on PR + #737 to make symlinked artifact dirs classify as external). Both assertions + red — and NOTHING else in the suite does, which is why this row exists. + """ + project.deferred_work.parent.mkdir(parents=True, exist_ok=True) + committed = "# Deferred Work\n\n## DW-1 committed at baseline\n" + project.deferred_work.write_text(committed, encoding="utf-8") + git(project.project, "add", "-A") + git(project.project, "commit", "-q", "-m", "track deferred-work") + engine, _ = make_engine(project, [], policy=_harvest_policy()) + task = StoryTask(story_key="1-1-a", epic=1) + task.baseline_commit = rev_parse_head(project.project) + task.baseline_untracked = [] + refuse_to_resolve(monkeypatch, project.deferred_work, project.project) + + # named lexically, with no fault raised + assert engine._ledger_rel() == ("_bmad-output/implementation-artifacts/deferred-work.md", None) + # and the anchor stays authoritative rather than degrading to no-anchor + assert engine._ledger_baseline_text(task) == (_LedgerAnchor.BASELINE, committed) + + def test_ledger_baseline_text_reads_the_committed_blob(project, monkeypatch): """The reset-owned write anchor is the committed blob, read before the lock. From 5cf58d792359162cbcbe63b8f183ebf6fdf22c20 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 26 Aug 2026 13:05:23 -0700 Subject: [PATCH 11/13] docs(features): correct the baseline-probe outcomes (#735) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ledger-lock bullet listed "an external ledger" and "a run with no baseline commit" as causes of a journaled `ledger-baseline-probe-failed` that degrades every site. Neither is true: * an external ledger answers NO_RESET_CONTENT, journals nothing, and the sweep restore still completes on the rejected rewrite — only the engine's two restores stand down; and * a run with no baseline commit answers NONE and deliberately journals nothing, so the write arm stands down silently rather than filing a row an operator would have to triage. Only an unreadable blob or a failed path resolution emit that event. As written the reference sent operators looking for a row that is never logged. The anchor sentence above it also still said "untracked", which the last two commits widened to untracked, external, or symlinked. Reported by codex on PR #737. Docs only — no behavior change. --- docs/FEATURES.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/FEATURES.md b/docs/FEATURES.md index c2bc69c7..bb0f6573 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -136,7 +136,7 @@ See [README.md](../README.md) for the narrative overview and [setup-guide.md](se - Repeat mode (`--repeat` / `[sweep] repeat`): re-triages after each cycle to absorb newly generated deferred work, stopping when a cycle does nothing addressable or hits `max_cycles`. - Ledger archiving (`sweep --archive`, optionally `--before DATE` to bound by close date; `--dry-run` previews): moves closed (`status: done `) entries to sibling `deferred-work-archive.md` (body preserved, an `archived: ` marker appended), leaving an id-preserving stub (`status: done ` + `archived: `) so grep and `closes_deferred` cross-references keep resolving and the live ledger stays proportional to open work rather than all history. The stub keeps load-bearing field lines — `gate:` (validate's closed-gate report keeps speaking), `origin:`/`source_spec:` (the harvest-replay dedupe stays status-agnostic), and the reopenable-close undo tail (a paused sweep's bundle rollback still works). Reopening an archived stub — what that rollback does — demotes its `archived:` stamp to `archived-body:` rather than dropping it: the entry is live work again, so the stamp would be a lie and its shape would strand the entry outside every future archive, but the body its close moved out is still in the archive file and that line is what a later triage follows to it. Deterministic, no LLM, atomic writes with crash-safe ordering (archive before ledger; a retry keyed on id + close date completes the move without duplicate bodies). Refuses while any engine run is live or its liveness is unverifiable — it is the one out-of-band ledger writer. When the ledger is tracked, the move is durable only once both files are committed; a gitignored ledger — the default shape — or an artifact dir outside the repo has nothing to commit. Unrelated to `bmad-loop archive ` (run-tarball archiving). - Sweeps are their own resumable runs (`bmad-loop resume `). An escalated bundle resolves like a story escalation, including intent-gap patch-restore: `bmad-loop resolve --restore-patch ` re-arms the bundle spec to `in-review` and the re-driven bundle session resumes review on the re-applied patch instead of re-implementing. -- Ledger writes serialize across processes (#286/#469). Every orchestrator mutation of `deferred-work.md` — an append, a close, a reopen, a recorded decision, `sweep --archive`'s two-file rewrite — holds an advisory lock for its whole read-modify-write, so a second `bmad-loop run`, a run plus a sweep, or a run plus the TUI decision modal can no longer both read, both edit, and let the last atomic write win (lost entries, silently reverted closures, two appenders minting the same `DW-`); multi-row work is batched into one locked pass rather than one per row. The lock is a sidecar under the state root (`/locks/-.lock`), never beside the ledger, because the ledger is tracked by design and the engine stages with `git add -A`; it is keyed on the resolved path, so every spelling of one file contends on one lock while two worktrees' in-tree ledgers correctly get their own. Readers stay lock-free — every writer already replaces the file atomically, so a reader sees one whole version or another. The wait is platform-asymmetric: POSIX blocks, while Windows bounds it at roughly ten seconds and then surfaces contention as an error rather than proceeding unlocked. A dev or review session's own ledger writes are deliberately outside this — the orchestrator sequences its writes against the sessions it dispatches. Here too the hold covers every read that decides the **published bytes** rather than every read: a mutator whose work turns out to be a no-op — ids already done, a decision on an entry that is not there, specs that all dedupe, nothing eligible to archive, or no ledger at all — is answered from one advisory pre-lock read running the same pure decision helper the locked pass runs, and takes no lock (#736), so a replayed rollback or a `sweep --archive` over a ledger holding nothing closed cannot fail on a lock it had no write to serialize. Only a would-write-nothing answer is acted on; anything else, and any fault while probing, falls through to the hold. `append_entries_published` deliberately keeps no missing-ledger answer — an absent ledger there means create, which is a write. The rollback paths that span a `git reset --hard` cannot be covered by a lock at all, so each is instead compare-and-set — and the **write** is anchored on the ledger's committed blob at the run's baseline commit, the text that reset republished, read out of git before the lock rather than off a working tree a rival could have written inside the reset window (#735); the sweep's untracked ledger, which has no blob, anchors instead on the rejected rewrite the attempt itself graded. A post-reset observation still authorizes the **skips** — declining to act is safe whoever wrote those bytes — and never a write. Divergence, and an anchor no probe could derive (journaled `ledger-baseline-probe-failed`, from an unreadable blob, an external ledger or a run with no baseline commit), degrade each site in its own direction rather than writing over a concurrent writer: `defer-ledger-restore-diverged` merges by appending the entries disk has since lost, `ledger-restore-skipped-diverged` skips, and `sweep-migration-restore-diverged` escalates for a human to re-run the sweep. And a failed commit reopens exactly the entries the story itself closed, journaling `deferred-close-rolled-back` with their ids and `deferred-close-reopen-unmatched` for an entry whose undo marker a foreign edit has displaced. +- Ledger writes serialize across processes (#286/#469). Every orchestrator mutation of `deferred-work.md` — an append, a close, a reopen, a recorded decision, `sweep --archive`'s two-file rewrite — holds an advisory lock for its whole read-modify-write, so a second `bmad-loop run`, a run plus a sweep, or a run plus the TUI decision modal can no longer both read, both edit, and let the last atomic write win (lost entries, silently reverted closures, two appenders minting the same `DW-`); multi-row work is batched into one locked pass rather than one per row. The lock is a sidecar under the state root (`/locks/-.lock`), never beside the ledger, because the ledger is tracked by design and the engine stages with `git add -A`; it is keyed on the resolved path, so every spelling of one file contends on one lock while two worktrees' in-tree ledgers correctly get their own. Readers stay lock-free — every writer already replaces the file atomically, so a reader sees one whole version or another. The wait is platform-asymmetric: POSIX blocks, while Windows bounds it at roughly ten seconds and then surfaces contention as an error rather than proceeding unlocked. A dev or review session's own ledger writes are deliberately outside this — the orchestrator sequences its writes against the sessions it dispatches. Here too the hold covers every read that decides the **published bytes** rather than every read: a mutator whose work turns out to be a no-op — ids already done, a decision on an entry that is not there, specs that all dedupe, nothing eligible to archive, or no ledger at all — is answered from one advisory pre-lock read running the same pure decision helper the locked pass runs, and takes no lock (#736), so a replayed rollback or a `sweep --archive` over a ledger holding nothing closed cannot fail on a lock it had no write to serialize. Only a would-write-nothing answer is acted on; anything else, and any fault while probing, falls through to the hold. `append_entries_published` deliberately keeps no missing-ledger answer — an absent ledger there means create, which is a write. The rollback paths that span a `git reset --hard` cannot be covered by a lock at all, so each is instead compare-and-set — and the **write** is anchored on the ledger's committed blob at the run's baseline commit, the text that reset republished, read out of git before the lock rather than off a working tree a rival could have written inside the reset window (#735); a ledger the reset republishes no text for — untracked, configured outside the repo tree, or symlinked into it, where the committed blob is a target pathname rather than ledger text — has no baseline text to anchor on, so the sweep anchors instead on the rejected rewrite the attempt itself graded, while the engine's two restores decline to read a missing file there as the reset's own work. A post-reset observation still authorizes the **skips** — declining to act is safe whoever wrote those bytes — and never a write. Divergence, and an anchor no probe could derive — an unreadable blob or a failed path resolution, both journaled `ledger-baseline-probe-failed`, or a run with no baseline commit, which stands down silently rather than filing a row an operator would have to triage — degrade each site in its own direction rather than writing over a concurrent writer: `defer-ledger-restore-diverged` merges by appending the entries disk has since lost, `ledger-restore-skipped-diverged` skips, and `sweep-migration-restore-diverged` escalates for a human to re-run the sweep. And a failed commit reopens exactly the entries the story itself closed, journaling `deferred-close-rolled-back` with their ids and `deferred-close-reopen-unmatched` for an entry whose undo marker a foreign edit has displaced. ### Stories mode (folder+id dispatch) From e4dc35d30d2fba4fe17317759cfdff858cbb4202 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 26 Aug 2026 13:19:20 -0700 Subject: [PATCH 12/13] fix(engine): stop the defer merge resurrecting a deleted ledger (#735) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gating the DIRECT overwrite on a BASELINE anchor was not enough at this site, because it degrades to an append-only merge rather than to a skip. That merge is immune to a rival's WRITE — it only ever adds — but it read `current or ""`, so a ledger that is GONE looked like one where every snapshot entry is merely missing, and it wrote them all back. Recreating a file a rival deleted is the same overwrite wearing different clothes. A tracked symlink is the shape that reaches it: git owns it, so the `_ledger_is_gits_to_restore` gate lets it through, while `reset --hard` restores only the link and never the target a rival unlinked. The merge now runs only on a ledger that is actually there. A missing one falls through to the divergence journal — the arm above already claimed the single absence that IS the reset's own work, a baseline that determinately lacks the ledger. `collided` joins the other two accumulators at their pre-lock initialization, since the merge no longer always runs. Ablated: restoring `current or ""` recreates the target and reddens. Reported by codex on PR #737. --- src/bmad_loop/engine.py | 27 +++++++++++++++++------ tests/test_engine.py | 47 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 7 deletions(-) diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index 3c5d9cb9..2aea1465 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -6476,9 +6476,11 @@ def _restore_defer_ledger(self, task: StoryTask, snapshot: str) -> None: # `reset --hard` could actually have republished. No anchor degrades to # the merge below, which is append-only and therefore cannot destroy a # rival's write — the reason this site can absorb a probe fault the way - # `_restore_ledger`'s degrade-to-skip has to. + # `_restore_ledger`'s degrade-to-skip has to. That immunity covers a + # rival's WRITE only; a rival's DELETION is refused at the merge itself. anchor, expected = self._ledger_baseline_text(task) merged: list[str] = [] + collided: list[str] = [] flat_remainder = False with deferredwork.ledger_lock(ledger): # PURE TEXT ONLY under the hold. Every `deferredwork` mutator takes @@ -6495,12 +6497,23 @@ def _restore_defer_ledger(self, task: StoryTask, snapshot: str) -> None: ledger.parent.mkdir(parents=True, exist_ok=True) atomic_write_text(ledger, snapshot) return - restored, merged, flat_remainder, collided = self._merge_snapshot_entries( - current or "", snapshot - ) - if restored is not None: - ledger.parent.mkdir(parents=True, exist_ok=True) - atomic_write_text(ledger, restored) + if current is not None: + restored, merged, flat_remainder, collided = self._merge_snapshot_entries( + current, snapshot + ) + if restored is not None: + ledger.parent.mkdir(parents=True, exist_ok=True) + atomic_write_text(ledger, restored) + # A MISSING ledger falls straight through to the divergence journal. + # The arm above already claimed the only absence that IS the reset's + # own work (a baseline determinately lacking the ledger, where + # `None == None` holds), so reaching here with no file means somebody + # removed it after the reset — or through a symlink the reset cannot + # reach at all. Merging `current or ""` would read that deletion as + # "every entry is merely missing" and write them all back, recreating + # the file: the append-only merge cannot destroy a rival's WRITE, but + # it can resurrect what a rival DELETED, which is the same overwrite + # wearing different clothes. # Only the divergent arm falls through to here. Journaled outside the # hold: the lock covers this ledger's read-modify-write and nothing else. self.journal.append( diff --git a/tests/test_engine.py b/tests/test_engine.py index 28422565..1788d5b5 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -13111,6 +13111,53 @@ def test_restore_ledger_reset_owned_write_uses_the_blob_anchor(project): @pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") +def test_defer_restore_never_resurrects_a_deleted_symlink_target(project, tmp_path): + """DIRECTION PIN (#735), the merge arm's twin of + `test_restore_ledger_never_reads_a_symlink_deletion_as_reset_owned`. + + Gating the DIRECT overwrite on a `BASELINE` anchor is not enough here, + because this site degrades to an append-only merge rather than to a skip. + That merge is immune to a rival's WRITE — it only ever adds — but it read + `current or ""`, so a ledger that is GONE looked like a ledger where every + snapshot entry is merely missing, and it wrote them all back. Recreating a + file a rival deleted is the same overwrite wearing different clothes. + + A tracked symlink is the shape that reaches it: it is git-owned, so the + `_ledger_is_gits_to_restore` gate above lets it through, while `reset --hard` + restores only the link and can never reach the target a rival unlinked. + + Ablation: restore `current or ""` as the merge input and drop the + `current is not None` guard — the target comes back and this reddens. + """ + target = tmp_path / "shared" / "deferred-work.md" + target.parent.mkdir(parents=True) + target.write_text("# Deferred Work\n", encoding="utf-8") + project.deferred_work.parent.mkdir(parents=True, exist_ok=True) + if project.deferred_work.is_symlink() or project.deferred_work.exists(): + project.deferred_work.unlink() + project.deferred_work.symlink_to(target) + git(project.project, "add", "-A") + git(project.project, "commit", "-q", "-m", "track a symlinked deferred-work") + engine, _ = make_engine(project, [], policy=_harvest_policy()) + task = StoryTask(story_key="1-1-a", epic=1) + task.baseline_commit = rev_parse_head(project.project) + task.baseline_untracked = [] + snapshot = ( + "# Deferred Work\n\n### DW-1: review found this\n\n" + "origin: review, 2026-08-26\nlocation: src.txt\nreason: needs a look.\nstatus: open\n" + ) + # The rival's deletion, landing inside the window the reset opened. + target.unlink() + + engine._restore_defer_ledger(task, snapshot) + + assert not target.exists() + assert project.deferred_work.is_symlink() + (event,) = [e for e in engine.journal.entries() if e["kind"] == "defer-ledger-restore-diverged"] + assert event["dw_ids"] == [] + + def test_restore_ledger_never_reads_a_symlink_deletion_as_reset_owned(project, tmp_path): """DIRECTION PIN (#735). A tracked ledger symlink whose TARGET a rival deleted inside the reset window must not be written back over. From b168a96eb62e423f30a465f7acb60da4f6f697c5 Mon Sep 17 00:00:00 2001 From: t Date: Wed, 26 Aug 2026 13:33:25 -0700 Subject: [PATCH 13/13] fix(sweep): accept an already-restored ledger instead of escalating (#735, #736) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A restore that finds the ledger already holding the text it would write is DONE, not divergent — the #736 principle applied at the restore. Reachable with no rival at all. A migration session that atomic-saves — write-temp-then-rename, how most editors and many CLIs write — replaces a tracked symlink with a regular file. `reset --hard` puts the link back, and the external target it can never reach was therefore never rewritten, so the ledger is already correct. But `rewrite`, read off that regular file, is the rejected migration text, so demanding the anchor reported a divergence that did not happen, escalated, and spent the attempt budget: the second attempt never dispatched. Scoped to a NO_RESET_CONTENT anchor deliberately. On BASELINE the reset republishes the committed text, so `current == text` is the ORDINARY post-reset state; accepting it there retires the divergence check and the probe-fault escalation with it — which is exactly what the first, unscoped attempt did, and what two existing tests caught. Ablated: dropping the arm journals the divergence, restores the "changed underneath" accusation, and drops the run back to one session. Reported by codex on PR #737. --- src/bmad_loop/sweep.py | 23 +++++++++++++-- tests/test_sweep.py | 63 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 84 insertions(+), 2 deletions(-) diff --git a/src/bmad_loop/sweep.py b/src/bmad_loop/sweep.py index ffb95697..ca835227 100644 --- a/src/bmad_loop/sweep.py +++ b/src/bmad_loop/sweep.py @@ -1053,11 +1053,30 @@ def _ensure_migration(self, text: str) -> None: # PURE TEXT ONLY under the hold — `ledger_lock` is not reentrant # and every mutator takes it. current = ledger.read_text(encoding="utf-8") if ledger.is_file() else None - # Either anchor will do HERE, unlike the engine's two restores: + if anchor is _LedgerAnchor.NO_RESET_CONTENT and current == text: + # ALREADY the text this restore exists to write, so it is + # done and there is nothing to escalate. Reachable without + # any rival: a session that atomic-SAVES the ledger replaces + # a tracked symlink with a regular file, `reset --hard` puts + # the link back, and the external target it cannot reach was + # never rewritten — so the ledger is correct while `rewrite`, + # read off the regular file, is not what is on disk. Demanding + # the anchor here would escalate a finished restore and spend + # the attempt budget on it. + # + # Scoped to NO_RESET_CONTENT deliberately. On a BASELINE + # anchor the reset republishes the committed text, so + # `current == text` is the ORDINARY post-reset state and + # accepting it there would retire the divergence check and + # the probe-fault escalation along with it. Only where the + # reset restored no text of its own is "already correct" + # information the anchor cannot supply. + pass + # Either anchor will do below, unlike the engine's two restores: # this site supplies its own text for the no-reset-content case # (`rewrite`, which it graded), so `expected` is never the bare # `None` that would read a rival's deletion as the reset's work. - if anchor is not _LedgerAnchor.NONE and current == expected: + elif anchor is not _LedgerAnchor.NONE and current == expected: ledger.parent.mkdir(parents=True, exist_ok=True) atomic_write_text(ledger, text) else: diff --git a/tests/test_sweep.py b/tests/test_sweep.py index 11e6e56d..c6e57479 100644 --- a/tests/test_sweep.py +++ b/tests/test_sweep.py @@ -5456,3 +5456,66 @@ def test_migration_restore_writes_back_a_symlinked_ledger(project, tmp_path): assert project.deferred_work.is_symlink() assert target.read_text(encoding="utf-8") == LEGACY_LEDGER assert "sweep-migration-restore-diverged" not in journal_kinds(engine) + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") +def test_migration_restore_accepts_an_already_restored_symlink_ledger(project, tmp_path): + """#735/#736. A restore that finds the ledger already holding the text it + would write is DONE, not divergent. + + Reachable with no rival at all. A migration session that atomic-saves — + write-temp-then-rename, which is how most editors and many CLIs write — + replaces the tracked symlink with a regular file. `reset --hard` puts the + link back, and the external target it can never reach was therefore never + rewritten, so the ledger is already correct. But `rewrite`, read off that + regular file, is the rejected migration text, so demanding the anchor here + reports a divergence that did not happen, escalates, and spends the attempt + budget: the second attempt never dispatches. + + This is the #736 principle at the restore: an operation with nothing to write + must not fail. + + Ablation: drop the `current == text` arm and this reddens on all three — + `sweep-migration-restore-diverged` is journaled, the paused reason becomes + the "changed underneath" accusation, and only one session runs. + """ + target = tmp_path / "shared" / "deferred-work.md" + target.parent.mkdir(parents=True) + target.write_text(LEGACY_LEDGER, encoding="utf-8") + if project.deferred_work.is_symlink() or project.deferred_work.exists(): + project.deferred_work.unlink() + project.deferred_work.symlink_to(target) + git(project.project, "add", "-A") + git(project.project, "commit", "-q", "-m", "symlinked ledger") + + manifest = legacy_manifest() + half = ( + "# Deferred Work\n\n" + "### DW-1: Old fixed thing\n\norigin: migrated, 2026-06-12\nlocation: n/a\n" + "reason: repaired.\nstatus: done 2026-04-06\n\n" + "## Deferred from: epic 1 review (2026-04-06)\n\n" + "- **Open legacy thing here** — `src.txt` mishandles em-dashes\n" + ) + + def atomic_save_effect(spec): + # the rename an atomic save performs: the symlink is REPLACED, so the + # external target keeps the pre-migration text throughout. + project.deferred_work.unlink() + project.deferred_work.write_text(half, encoding="utf-8") + return SessionResult( + status="completed", + result_json={ + "workflow": "deferred-sweep-migrate", + "mapping": [{"key": manifest[0]["key"], "dw_id": "DW-1"}], + "escalations": [], + }, + ) + + engine, adapter = make_sweep(project, [atomic_save_effect] * 2) + summary = engine.run() + + assert summary.paused # on the attempt cap, having actually retried + assert "sweep-migration-restore-diverged" not in journal_kinds(engine) + assert "changed underneath the failed migration attempt" not in engine.state.paused_reason + assert target.read_text(encoding="utf-8") == LEGACY_LEDGER + assert len(adapter.sessions) == 2