From 25943e04388f5183a57a744bf9c32d045399974f Mon Sep 17 00:00:00 2001 From: t Date: Mon, 14 Sep 2026 07:20:53 -0700 Subject: [PATCH 01/17] sweep dw-decision-dw-308: DW-308 via bmad-loop --- src/bmad_loop/recovery_flow.py | 108 +++++++- tests/test_recovery_flow.py | 490 ++++++++++++++++++++++++++++++++- 2 files changed, 581 insertions(+), 17 deletions(-) diff --git a/src/bmad_loop/recovery_flow.py b/src/bmad_loop/recovery_flow.py index ee38dab8b..5acb8ae2e 100644 --- a/src/bmad_loop/recovery_flow.py +++ b/src/bmad_loop/recovery_flow.py @@ -60,6 +60,11 @@ class _OwnedSpecAuthorityError(RuntimeError): """A previously canonical owned-spec name became unsafe to restore.""" +def _target_stat_version(observed: os.stat_result) -> tuple[int, int, int]: + """Mutation-sensitive fields subordinate to an already-bound target inode.""" + return observed.st_size, observed.st_mtime_ns, observed.st_ctime_ns + + class RecoveryFlow: """Roll back or pause a stopped/abandoned attempt, parking any work it did on named recovery refs before the reset. @@ -318,13 +323,77 @@ def target_stat_at(parent_fd: int) -> os.stat_result | None: raise _OwnedSpecAuthorityError(authority_message) return observed - def require_same_target_at(parent_fd: int, expected: os.stat_result | None) -> None: + def read_target_at( + parent_fd: int, + ) -> tuple[os.stat_result, bytes] | None: observed = target_stat_at(parent_fd) + if observed is None: + return None + flags = os.O_RDONLY | os.O_NOFOLLOW | os.O_NONBLOCK + try: + target_fd = os.open(spec_path.name, flags, dir_fd=parent_fd) + except OSError as exc: + if exc.errno in { + errno.ELOOP, + errno.ENOENT, + errno.ENOTDIR, + errno.ENXIO, + errno.ENODEV, + }: + raise _OwnedSpecAuthorityError(authority_message) from exc + raise + try: + before = os.fstat(target_fd) + if ( + not stat.S_ISREG(before.st_mode) + or not os.path.samestat(observed, before) + or _target_stat_version(observed) != _target_stat_version(before) + ): + raise _OwnedSpecAuthorityError(authority_message) + + os.lseek(target_fd, 0, os.SEEK_SET) + chunks: list[bytes] = [] + remaining = before.st_size + 1 + while remaining: + chunk = os.read(target_fd, min(1024 * 1024, remaining)) + if not chunk: + break + chunks.append(chunk) + remaining -= len(chunk) + contents = b"".join(chunks) + + after = os.fstat(target_fd) + named = target_stat_at(parent_fd) + if ( + len(contents) != before.st_size + or not os.path.samestat(before, after) + or _target_stat_version(before) != _target_stat_version(after) + or named is None + or not os.path.samestat(after, named) + or _target_stat_version(after) != _target_stat_version(named) + ): + raise _OwnedSpecAuthorityError(authority_message) + return after, contents + finally: + os.close(target_fd) + + def require_same_target_at( + parent_fd: int, expected: tuple[os.stat_result, bytes] | None + ) -> None: + observed = read_target_at(parent_fd) if expected is None: if observed is not None: raise _OwnedSpecAuthorityError(authority_message) return - if observed is None or not os.path.samestat(expected, observed): + if observed is None: + raise _OwnedSpecAuthorityError(authority_message) + expected_stat, expected_bytes = expected + observed_stat, observed_bytes = observed + if ( + not os.path.samestat(expected_stat, observed_stat) + or _target_stat_version(expected_stat) != _target_stat_version(observed_stat) + or expected_bytes != observed_bytes + ): raise _OwnedSpecAuthorityError(authority_message) def verify_published_inode(parent_fd: int, published_fd: int) -> None: @@ -411,14 +480,39 @@ def fallback_target_stat() -> os.stat_result | None: raise _OwnedSpecAuthorityError(authority_message) return observed - def require_same_fallback_target(expected: os.stat_result | None) -> None: + def read_fallback_target() -> tuple[os.stat_result, bytes] | None: + before = fallback_target_stat() + if before is None: + return None + contents = spec_path.read_bytes() + after = fallback_target_stat() + if ( + after is None + or len(contents) != before.st_size + or not os.path.samestat(before, after) + or _target_stat_version(before) != _target_stat_version(after) + ): + raise _OwnedSpecAuthorityError(authority_message) + return after, contents + + def require_same_fallback_target( + expected: tuple[os.stat_result, bytes] | None, + ) -> None: fallback_parent_is_canonical() - observed = fallback_target_stat() + observed = read_fallback_target() if expected is None: if observed is not None: raise _OwnedSpecAuthorityError(authority_message) return - if observed is None or not os.path.samestat(expected, observed): + if observed is None: + raise _OwnedSpecAuthorityError(authority_message) + expected_stat, expected_bytes = expected + observed_stat, observed_bytes = observed + if ( + not os.path.samestat(expected_stat, observed_stat) + or _target_stat_version(expected_stat) != _target_stat_version(observed_stat) + or expected_bytes != observed_bytes + ): raise _OwnedSpecAuthorityError(authority_message) def verify_fallback_bytes(_published_fd: int | None) -> None: @@ -461,7 +555,7 @@ def verify_fallback_bytes(_published_fd: int | None) -> None: f"attempt-owned spec target could not be revalidated: {spec_path}" ) try: - expected = target_stat_at(parent_fd) + expected = read_target_at(parent_fd) def validate_target() -> None: require_same_target_at(parent_fd, expected) @@ -482,7 +576,7 @@ def verify_published(published_fd: int) -> None: os.close(parent_fd) return - expected = fallback_target_stat() + expected = read_fallback_target() def validate_fallback_target() -> None: require_same_fallback_target(expected) diff --git a/tests/test_recovery_flow.py b/tests/test_recovery_flow.py index 42ec6b4bb..72e4a871f 100644 --- a/tests/test_recovery_flow.py +++ b/tests/test_recovery_flow.py @@ -227,7 +227,9 @@ def fail_write(*_args, **_kwargs): @pytest.mark.skipif( not platform_util.DIR_FD_ANCHORED_WRITES, reason="dir-fd anchoring is POSIX-only" ) -def test_owned_spec_restore_does_not_translate_readback_value_error(tmp_path, monkeypatch, failure): +def test_owned_spec_restore_does_not_translate_content_read_value_error( + tmp_path, monkeypatch, failure +): spec = tmp_path.resolve() / "owned.md" spec.write_bytes(b"operator bytes\n") @@ -240,11 +242,11 @@ def fail_readback(fd, size): RecoveryFlow._restore_attempt_owned_spec_bytes(spec, b"snapshot bytes\n") assert excinfo.value is failure - assert spec.read_bytes() == b"snapshot bytes\n" + assert spec.read_bytes() == b"operator bytes\n" @pytest.mark.parametrize("failure", NUL_PATH_RESOLVE_FAULTS) -def test_owned_spec_restore_fallback_preserves_raw_readback_value_error( +def test_owned_spec_restore_fallback_preserves_raw_content_read_value_error( tmp_path, monkeypatch, failure ): monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) @@ -265,7 +267,7 @@ def fail_readback(path: Path) -> bytes: assert excinfo.value is failure with spec.open("rb") as fh: - assert fh.read() == b"snapshot bytes\n" + assert fh.read() == b"operator bytes\n" def test_owned_spec_restore_preserves_byte_hostile_snapshot(tmp_path): @@ -509,7 +511,7 @@ def bounded_read(fd: int, size: int) -> bytes: RecoveryFlow._restore_attempt_owned_spec_bytes(spec, snapshot) assert spec.read_bytes() == snapshot + b"x" - assert requested == [len(snapshot) + 1] + assert requested[-1:] == [len(snapshot) + 1] def test_owned_spec_restore_fallback_snapshot_plus_suffix_is_a_genuine_mismatch( @@ -569,7 +571,45 @@ def fail_read(_fd, _size): RecoveryFlow._restore_attempt_owned_spec_bytes(spec, b"snapshot bytes") assert excinfo.value is failure - assert spec.read_bytes() == b"snapshot bytes" + assert spec.read_bytes() == b"operator bytes" + + +@pytest.mark.skipif( + not platform_util.DIR_FD_ANCHORED_WRITES, reason="dir-fd anchoring is POSIX-only" +) +def test_owned_spec_restore_preserves_raw_postpublication_os_read_failure(tmp_path, monkeypatch): + spec = tmp_path.resolve() / "owned.md" + spec.write_bytes(b"operator bytes") + snapshot = b"snapshot bytes" + failure = OSError("ordinary postpublication read failed") + real_read = os.read + real_write = recovery_flow.atomic_write_bytes_at + published = False + + def mark_published(dir_fd, name, data, **kwargs): + verify_after = kwargs["_after_replace"] + + def mark_then_verify(published_fd: int) -> None: + nonlocal published + published = True + verify_after(published_fd) + + kwargs["_after_replace"] = mark_then_verify + return real_write(dir_fd, name, data, **kwargs) + + def fail_after_publish(fd: int, size: int) -> bytes: + if published: + raise failure + return real_read(fd, size) + + monkeypatch.setattr(recovery_flow, "atomic_write_bytes_at", mark_published) + monkeypatch.setattr(recovery_flow.os, "read", fail_after_publish) + + with pytest.raises(OSError) as excinfo: + RecoveryFlow._restore_attempt_owned_spec_bytes(spec, snapshot) + + assert excinfo.value is failure + assert spec.read_bytes() == snapshot def test_owned_spec_restore_preserves_raw_fallback_read_failure(tmp_path, monkeypatch): @@ -592,7 +632,47 @@ def fail_read(path: Path) -> bytes: assert excinfo.value is failure with spec.open("rb") as fh: - assert fh.read() == b"snapshot bytes" + assert fh.read() == b"operator bytes" + + +def test_owned_spec_restore_fallback_preserves_raw_postpublication_read_failure( + tmp_path, monkeypatch +): + monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) + monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) + spec = tmp_path.resolve() / "owned.md" + spec.write_bytes(b"operator bytes") + snapshot = b"snapshot bytes" + failure = OSError("ordinary postpublication read failed") + real_read_bytes = Path.read_bytes + real_write = recovery_flow.atomic_write_bytes_confined + published = False + + def mark_published(path, data, **kwargs): + verify_after = kwargs["_after_replace"] + + def mark_then_verify(published_fd: int | None) -> None: + nonlocal published + published = True + verify_after(published_fd) + + kwargs["_after_replace"] = mark_then_verify + return real_write(path, data, **kwargs) + + def fail_after_publish(path: Path) -> bytes: + if path == spec and published: + raise failure + return real_read_bytes(path) + + monkeypatch.setattr(recovery_flow, "atomic_write_bytes_confined", mark_published) + monkeypatch.setattr(Path, "read_bytes", fail_after_publish) + + with pytest.raises(OSError) as excinfo: + RecoveryFlow._restore_attempt_owned_spec_bytes(spec, snapshot) + + assert excinfo.value is failure + with spec.open("rb") as fh: + assert fh.read() == snapshot @pytest.mark.skipif( @@ -806,6 +886,221 @@ def replace_then_validate() -> None: assert list(parent.glob("*.tmp")) == [] +@pytest.mark.skipif( + not platform_util.DIR_FD_ANCHORED_WRITES, reason="dir-fd anchoring is POSIX-only" +) +def test_owned_spec_restore_refuses_in_place_target_edit_after_staging(tmp_path, monkeypatch): + parent = tmp_path.resolve() / "artifacts" + parent.mkdir() + spec = parent / "owned.md" + spec.write_bytes(b"operator bytes") + original_inode = spec.stat().st_ino + competing = b"competing data" + real_write = recovery_flow.atomic_write_bytes_at + monkeypatch.setattr(recovery_flow, "_target_stat_version", lambda _observed: (0, 0, 0)) + + def edit_before_publish(dir_fd, name, data, **kwargs): + validate = kwargs["_before_replace"] + + def edit_then_validate() -> None: + with spec.open("r+b") as fh: + fh.write(competing) + fh.truncate() + assert spec.stat().st_ino == original_inode + validate() + + kwargs["_before_replace"] = edit_then_validate + return real_write(dir_fd, name, data, **kwargs) + + monkeypatch.setattr(recovery_flow, "atomic_write_bytes_at", edit_before_publish) + + with pytest.raises(_OwnedSpecAuthorityError, match="became unsafe"): + RecoveryFlow._restore_attempt_owned_spec_bytes(spec, b"snapshot") + + assert spec.read_bytes() == competing + assert list(parent.glob("*.tmp")) == [] + + +@pytest.mark.skipif( + not platform_util.DIR_FD_ANCHORED_WRITES, reason="dir-fd anchoring is POSIX-only" +) +def test_owned_spec_restore_refuses_in_place_edit_during_initial_content_read( + tmp_path, monkeypatch +): + parent = tmp_path.resolve() / "artifacts" + parent.mkdir() + spec = parent / "owned.md" + original = b"operator bytes" + competing = original + b"x" + spec.write_bytes(original) + real_read = os.read + mutated = False + + def mutate_then_read(fd: int, size: int) -> bytes: + nonlocal mutated + if not mutated: + mutated = True + with spec.open("ab") as fh: + fh.write(b"x") + return real_read(fd, size) + + monkeypatch.setattr(recovery_flow.os, "read", mutate_then_read) + + with pytest.raises(_OwnedSpecAuthorityError, match="became unsafe"): + RecoveryFlow._restore_attempt_owned_spec_bytes(spec, b"snapshot") + + assert mutated is True + assert spec.read_bytes() == competing + assert list(parent.glob("*.tmp")) == [] + + +@pytest.mark.skipif( + not platform_util.DIR_FD_ANCHORED_WRITES, reason="dir-fd anchoring is POSIX-only" +) +def test_owned_spec_restore_refuses_short_initial_content_sample(tmp_path, monkeypatch): + parent = tmp_path.resolve() / "artifacts" + parent.mkdir() + spec = parent / "owned.md" + original = b"operator bytes" + spec.write_bytes(original) + monkeypatch.setattr(recovery_flow.os, "read", lambda _fd, _size: b"") + + with pytest.raises(_OwnedSpecAuthorityError, match="became unsafe"): + RecoveryFlow._restore_attempt_owned_spec_bytes(spec, b"snapshot") + + assert spec.read_bytes() == original + assert list(parent.glob("*.tmp")) == [] + + +@pytest.mark.skipif( + not platform_util.DIR_FD_ANCHORED_WRITES, reason="dir-fd anchoring is POSIX-only" +) +def test_owned_spec_restore_compares_bounded_multichunk_content_after_staging( + tmp_path, monkeypatch +): + chunk_size = 1024 * 1024 + prefix = b"a" * chunk_size + original = prefix + b"operator bytes" + competing = prefix + b"competing data" + parent = tmp_path.resolve() / "artifacts" + parent.mkdir() + spec = parent / "owned.md" + spec.write_bytes(original) + original_inode = spec.stat().st_ino + real_read = os.read + real_write = recovery_flow.atomic_write_bytes_at + requests: list[int] = [] + monkeypatch.setattr(recovery_flow, "_target_stat_version", lambda _observed: (0, 0, 0)) + + def bounded_read(fd: int, size: int) -> bytes: + requests.append(size) + return real_read(fd, size) + + def edit_suffix_before_publish(dir_fd, name, data, **kwargs): + validate = kwargs["_before_replace"] + + def edit_then_validate() -> None: + with spec.open("r+b") as fh: + fh.seek(chunk_size) + fh.write(b"competing data") + fh.truncate() + assert spec.stat().st_ino == original_inode + validate() + + kwargs["_before_replace"] = edit_then_validate + return real_write(dir_fd, name, data, **kwargs) + + monkeypatch.setattr(recovery_flow.os, "read", bounded_read) + monkeypatch.setattr(recovery_flow, "atomic_write_bytes_at", edit_suffix_before_publish) + + with pytest.raises(_OwnedSpecAuthorityError, match="became unsafe"): + RecoveryFlow._restore_attempt_owned_spec_bytes(spec, b"snapshot") + + assert requests == [chunk_size, 15, 1] * 3 + assert max(requests) == chunk_size + assert spec.read_bytes() == competing + assert list(parent.glob("*.tmp")) == [] + + +@pytest.mark.skipif( + not platform_util.DIR_FD_ANCHORED_WRITES, reason="dir-fd anchoring is POSIX-only" +) +def test_owned_spec_restore_refuses_name_replacement_during_prepublication_read( + tmp_path, monkeypatch +): + parent = tmp_path.resolve() / "artifacts" + parent.mkdir() + spec = parent / "owned.md" + original = b"operator bytes" + competing = b"replacement" + spec.write_bytes(original) + real_write = recovery_flow.atomic_write_bytes_at + real_read = os.read + + def replace_during_validation(dir_fd, name, data, **kwargs): + validate = kwargs["_before_replace"] + + def replace_then_validate() -> None: + replaced = False + + def replace_then_read(fd: int, size: int) -> bytes: + nonlocal replaced + if not replaced: + replaced = True + spec.unlink() + spec.write_bytes(competing) + return real_read(fd, size) + + monkeypatch.setattr(recovery_flow.os, "read", replace_then_read) + validate() + + kwargs["_before_replace"] = replace_then_validate + return real_write(dir_fd, name, data, **kwargs) + + monkeypatch.setattr(recovery_flow, "atomic_write_bytes_at", replace_during_validation) + + with pytest.raises(_OwnedSpecAuthorityError, match="became unsafe"): + RecoveryFlow._restore_attempt_owned_spec_bytes(spec, b"snapshot") + + assert spec.read_bytes() == competing + assert list(parent.glob("*.tmp")) == [] + + +@pytest.mark.skipif( + not platform_util.DIR_FD_ANCHORED_WRITES, reason="dir-fd anchoring is POSIX-only" +) +def test_owned_spec_restore_preserves_raw_content_comparison_failure(tmp_path, monkeypatch): + parent = tmp_path.resolve() / "artifacts" + parent.mkdir() + spec = parent / "owned.md" + original = b"operator bytes" + spec.write_bytes(original) + failure = OSError("ordinary comparison read failed") + real_write = recovery_flow.atomic_write_bytes_at + + def fail_before_publish(dir_fd, name, data, **kwargs): + validate = kwargs["_before_replace"] + + def fail_then_validate() -> None: + def fail_read(_fd: int, _size: int) -> bytes: + raise failure + + monkeypatch.setattr(recovery_flow.os, "read", fail_read) + validate() + + kwargs["_before_replace"] = fail_then_validate + return real_write(dir_fd, name, data, **kwargs) + + monkeypatch.setattr(recovery_flow, "atomic_write_bytes_at", fail_before_publish) + + with pytest.raises(OSError) as excinfo: + RecoveryFlow._restore_attempt_owned_spec_bytes(spec, b"snapshot") + + assert excinfo.value is failure + assert spec.read_bytes() == original + assert list(parent.glob("*.tmp")) == [] + + def test_owned_spec_restore_fallback_refuses_target_replacement_after_staging( tmp_path, monkeypatch ): @@ -836,6 +1131,138 @@ def replace_then_validate() -> None: assert list(parent.glob("*.tmp")) == [] +def test_owned_spec_restore_fallback_refuses_in_place_target_edit_after_staging( + tmp_path, monkeypatch +): + monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) + monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) + parent = tmp_path.resolve() / "artifacts" + parent.mkdir() + spec = parent / "owned.md" + spec.write_bytes(b"operator bytes") + original_inode = spec.stat().st_ino + competing = b"competing data" + real_write = recovery_flow.atomic_write_bytes_confined + monkeypatch.setattr(recovery_flow, "_target_stat_version", lambda _observed: (0, 0, 0)) + + def edit_before_publish(path, data, **kwargs): + validate = kwargs["_before_replace"] + + def edit_then_validate() -> None: + with spec.open("r+b") as fh: + fh.write(competing) + fh.truncate() + assert spec.stat().st_ino == original_inode + validate() + + kwargs["_before_replace"] = edit_then_validate + return real_write(path, data, **kwargs) + + monkeypatch.setattr(recovery_flow, "atomic_write_bytes_confined", edit_before_publish) + + with pytest.raises(_OwnedSpecAuthorityError, match="became unsafe"): + RecoveryFlow._restore_attempt_owned_spec_bytes(spec, b"snapshot") + + assert spec.read_bytes() == competing + assert list(parent.glob("*.tmp")) == [] + + +def test_owned_spec_restore_fallback_refuses_in_place_edit_during_initial_content_read( + tmp_path, monkeypatch +): + monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) + monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) + parent = tmp_path.resolve() / "artifacts" + parent.mkdir() + spec = parent / "owned.md" + original = b"operator bytes" + competing = original + b"x" + spec.write_bytes(original) + real_read_bytes = Path.read_bytes + mutated = False + + def mutate_then_read(path: Path) -> bytes: + nonlocal mutated + if path == spec and not mutated: + mutated = True + with spec.open("ab") as fh: + fh.write(b"x") + return real_read_bytes(path) + + monkeypatch.setattr(Path, "read_bytes", mutate_then_read) + + with pytest.raises(_OwnedSpecAuthorityError, match="became unsafe"): + RecoveryFlow._restore_attempt_owned_spec_bytes(spec, b"snapshot") + + assert mutated is True + assert real_read_bytes(spec) == competing + assert list(parent.glob("*.tmp")) == [] + + +def test_owned_spec_restore_fallback_refuses_short_initial_content_sample(tmp_path, monkeypatch): + monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) + monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) + parent = tmp_path.resolve() / "artifacts" + parent.mkdir() + spec = parent / "owned.md" + original = b"operator bytes" + spec.write_bytes(original) + real_read_bytes = Path.read_bytes + + def short_read_bytes(path: Path) -> bytes: + if path == spec: + return b"operator" + return real_read_bytes(path) + + monkeypatch.setattr(Path, "read_bytes", short_read_bytes) + + with pytest.raises(_OwnedSpecAuthorityError, match="became unsafe"): + RecoveryFlow._restore_attempt_owned_spec_bytes(spec, b"snapshot") + + assert real_read_bytes(spec) == original + assert list(parent.glob("*.tmp")) == [] + + +def test_owned_spec_restore_fallback_preserves_raw_content_comparison_failure( + tmp_path, monkeypatch +): + monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) + monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) + parent = tmp_path.resolve() / "artifacts" + parent.mkdir() + spec = parent / "owned.md" + original = b"operator bytes" + spec.write_bytes(original) + failure = OSError("ordinary comparison read failed") + real_write = recovery_flow.atomic_write_bytes_confined + real_read_bytes = Path.read_bytes + + def fail_before_publish(path, data, **kwargs): + validate = kwargs["_before_replace"] + + def fail_then_validate() -> None: + def fail_read_bytes(read_path: Path) -> bytes: + if read_path == spec: + raise failure + return real_read_bytes(read_path) + + monkeypatch.setattr(Path, "read_bytes", fail_read_bytes) + validate() + + kwargs["_before_replace"] = fail_then_validate + return real_write(path, data, **kwargs) + + monkeypatch.setattr(recovery_flow, "atomic_write_bytes_confined", fail_before_publish) + + with pytest.raises(OSError) as excinfo: + RecoveryFlow._restore_attempt_owned_spec_bytes(spec, b"snapshot") + + assert excinfo.value is failure + with spec.open("rb") as fh: + assert fh.read() == original + assert list(parent.glob("*.tmp")) == [] + + @pytest.mark.skipif( not platform_util.DIR_FD_ANCHORED_WRITES, reason="dir-fd anchoring is POSIX-only" ) @@ -874,16 +1301,30 @@ def test_owned_spec_restore_rejects_live_name_replacement_during_readback(tmp_pa spec.write_bytes(b"operator bytes") snapshot = b"snapshot bytes" real_read = os.read + real_write = recovery_flow.atomic_write_bytes_at replaced = False + published = False + + def mark_published(dir_fd, name, data, **kwargs): + verify_after = kwargs["_after_replace"] + + def mark_then_verify(published_fd: int) -> None: + nonlocal published + published = True + verify_after(published_fd) + + kwargs["_after_replace"] = mark_then_verify + return real_write(dir_fd, name, data, **kwargs) def replace_name_then_read(fd: int, size: int) -> bytes: nonlocal replaced - if not replaced: + if not replaced and published: replaced = True spec.unlink() spec.write_bytes(snapshot) return real_read(fd, size) + monkeypatch.setattr(recovery_flow, "atomic_write_bytes_at", mark_published) monkeypatch.setattr(recovery_flow.os, "read", replace_name_then_read) with pytest.raises(_OwnedSpecAuthorityError, match="became unsafe"): @@ -902,15 +1343,29 @@ def test_owned_spec_restore_rejects_in_place_mutation_during_readback(tmp_path, snapshot = b"snapshot bytes" real_read = os.read mutated = False + published = False + real_write = recovery_flow.atomic_write_bytes_at + + def mark_published(dir_fd, name, data, **kwargs): + verify_after = kwargs["_after_replace"] + + def mark_then_verify(published_fd: int) -> None: + nonlocal published + published = True + verify_after(published_fd) + + kwargs["_after_replace"] = mark_then_verify + return real_write(dir_fd, name, data, **kwargs) def mutate_then_read(fd: int, size: int) -> bytes: nonlocal mutated - if not mutated: + if not mutated and published: mutated = True with spec.open("ab") as fh: fh.write(b"x") return real_read(fd, size) + monkeypatch.setattr(recovery_flow, "atomic_write_bytes_at", mark_published) monkeypatch.setattr(recovery_flow.os, "read", mutate_then_read) # Ablation: removing the before/after metadata comparison turns this into a @@ -930,15 +1385,30 @@ def test_owned_spec_restore_fallback_rejects_in_place_mutation(tmp_path, monkeyp snapshot = b"snapshot bytes" real_read_bytes = Path.read_bytes mutated = False + published = False + + real_write = recovery_flow.atomic_write_bytes_confined + + def mark_published(path, data, **kwargs): + verify_after = kwargs["_after_replace"] + + def mark_then_verify(published_fd: int | None) -> None: + nonlocal published + published = True + verify_after(published_fd) + + kwargs["_after_replace"] = mark_then_verify + return real_write(path, data, **kwargs) def mutate_then_read(path: Path) -> bytes: nonlocal mutated - if path == spec and not mutated: + if path == spec and published and not mutated: mutated = True with spec.open("ab") as fh: fh.write(b"x") return real_read_bytes(path) + monkeypatch.setattr(recovery_flow, "atomic_write_bytes_confined", mark_published) monkeypatch.setattr(Path, "read_bytes", mutate_then_read) with pytest.raises(_OwnedSpecAuthorityError, match="became unsafe"): From 119d03cd0f68693c3fc2856b04bff36c2977f43a Mon Sep 17 00:00:00 2001 From: t Date: Mon, 14 Sep 2026 09:04:10 -0700 Subject: [PATCH 02/17] sweep dw-decision-dw-309: DW-309 via bmad-loop --- docs/FEATURES.md | 2 +- src/bmad_loop/recovery_flow.py | 193 +++++++---- tests/test_recovery_flow.py | 610 +++++++++++++++++++++++++++++++-- 3 files changed, 703 insertions(+), 102 deletions(-) diff --git a/docs/FEATURES.md b/docs/FEATURES.md index f18d80d8d..0f9c5bc68 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -193,7 +193,7 @@ Result` section. Every other spec keeps warn-and-continue, and the record says w (#705). Sweep migration and triage tasks make the same rollover automatically when an `ESCALATED` task restarts with a fresh attempt budget; mid-flight, non-escalated restarts keep their current generation because their continuing attempt counter already provides a fresh id. -- Attempt-owned sprint-spec recovery (#123, #630): a bound plain attempt whose only residue is its own lifecycle flip is normalized back to its pre-attempt lifecycle status, proven Git-clean, and retried. Every bound retry chain snapshots its first spec input byte-for-byte and retains it across both dev-verification and review-verification repair sessions; a resolved re-drive therefore retains the operator-corrected `ready-for-dev` input rather than a failed child's later body. Repair entry points validate retained authority before constructing a prompt that can reset the spec. A non-fixable retry parks the failed child first, restores that snapshot, and re-establishes the promised route after resetting sibling residue. The same snapshot restores pre-launch operator edits when a plain child puts a tracked spec back at Git baseline. Git-ignored and pre-existing-untracked bound specs use the byte snapshot as their dirtiness oracle and are force-included only in the private recovery ref before restoration; index-only force-adds and cached removals also trigger cleanup and restore baseline index ownership. That real repair reports `rollback-owned-spec-restored`, never `rollback-skipped-clean`. Missing, unreadable, deleted, retargeted, changed external, or unsafe legacy authority pauses once with spec-specific adoption instructions and clears the unusable pair so manual recovery can converge; recovery also refuses a reset whose baseline would replace the canonical path or a parent directory with a symlink, tree, file, or other unsafe shape. An initial Sprint binding fault may safely degrade to an unbound bare-key launch; an existing Stories folder+id target instead aborts unless it can be snapshotted. Once an explicit binding is durable, a later snapshot fault aborts before child launch while retaining that authority for recovery. Fresh sprint tasks with no recorded path remain bare-key dispatches; other substantive changes or sibling residue follow rollback policy; Stories remains folder+id; Sweep remains intent-bundle routing; snapshots are retired after commit; and recovery never auto-commits the human correction. +- Attempt-owned sprint-spec recovery (#123, #630): a bound plain attempt whose only residue is its own lifecycle flip is normalized back to its pre-attempt lifecycle status, proven Git-clean, and retried. Every bound retry chain snapshots its first spec input byte-for-byte and retains it across both dev-verification and review-verification repair sessions; a resolved re-drive therefore retains the operator-corrected `ready-for-dev` input rather than a failed child's later body. Repair entry points validate retained authority before constructing a prompt that can reset the spec. A non-fixable retry parks the failed child first, restores that snapshot, and re-establishes the promised route after resetting sibling residue. Descriptor-capable restoration retains the staged inode across publication and verifies that exact inode before accepting it. A no-descriptor fallback still publishes the snapshot byte-for-byte, but performs no post-publication path readback, refuses to accept or normalize the unverifiable result, and pauses for manual adoption. The same snapshot restores pre-launch operator edits when a plain child puts a tracked spec back at Git baseline. Git-ignored and pre-existing-untracked bound specs use the byte snapshot as their dirtiness oracle and are force-included only in the private recovery ref before restoration; index-only force-adds and cached removals also trigger cleanup and restore baseline index ownership. That real repair reports `rollback-owned-spec-restored`, never `rollback-skipped-clean`. Missing, unreadable, deleted, retargeted, changed external, or unsafe legacy authority pauses once with spec-specific adoption instructions and clears the unusable pair so manual recovery can converge; recovery also refuses a reset whose baseline would replace the canonical path or a parent directory with a symlink, tree, file, or other unsafe shape. An initial Sprint binding fault may safely degrade to an unbound bare-key launch; an existing Stories folder+id target instead aborts unless it can be snapshotted. Once an explicit binding is durable, a later snapshot fault aborts before child launch while retaining that authority for recovery. Fresh sprint tasks with no recorded path remain bare-key dispatches; other substantive changes or sibling residue follow rollback policy; Stories remains folder+id; Sweep remains intent-bundle routing; snapshots are retired after commit; and recovery never auto-commits the human correction. - Intent-gap patch-restore (BMAD-METHOD#2564): when review halts on an `intent gap`, the dev primitive saves the attempted change as a patch file (referenced from the halt output) before reverting the tree. If that reading turns out to be correct, the resolve agent adds `"restore_patch": ""` to its `resolution.json`; the orchestrator re-arms the spec to `in-review` (not `ready-for-dev`) and re-applies the patch after every reset, so the re-driven session resumes _review_ on the restored diff instead of re-implementing. `bmad-loop resolve --no-interactive --restore-patch ` does the same by hand. A patch that fails to apply escalates rather than dispatching onto a half-restored tree. Sweep bundles get the same recovery. ### Git worktree isolation (opt-in) diff --git a/src/bmad_loop/recovery_flow.py b/src/bmad_loop/recovery_flow.py index 5acb8ae2e..ef3547165 100644 --- a/src/bmad_loop/recovery_flow.py +++ b/src/bmad_loop/recovery_flow.py @@ -57,7 +57,16 @@ def attempt_preserve_ref_name(run_id: str, tip: str) -> str: class _OwnedSpecAuthorityError(RuntimeError): - """A previously canonical owned-spec name became unsafe to restore.""" + """A previously canonical owned spec lost trustworthy restore authority.""" + + def __init__( + self, + message: str, + *, + published_without_verification: bool = False, + ) -> None: + super().__init__(message) + self.published_without_verification = published_without_verification def _target_stat_version(observed: os.stat_result) -> tuple[int, int, int]: @@ -516,32 +525,16 @@ def require_same_fallback_target( raise _OwnedSpecAuthorityError(authority_message) def verify_fallback_bytes(_published_fd: int | None) -> None: - before = fallback_target_stat() - if before is None: - raise _OwnedSpecAuthorityError(authority_message) - restored = spec_path.read_bytes() - after = fallback_target_stat() - if after is None: - raise _OwnedSpecAuthorityError(authority_message) - before_signature = ( - before.st_dev, - before.st_ino, - before.st_size, - before.st_mtime_ns, - before.st_ctime_ns, - ) - after_signature = ( - after.st_dev, - after.st_ino, - after.st_size, - after.st_mtime_ns, - after.st_ctime_ns, + # The fallback writer no longer owns an inode-bound descriptor once + # publication completes. Reopening or even observing the final name + # would make a concurrent replacement authoritative. Publication has + # already happened, so refuse it without touching the target path and + # let the recovery state machine require explicit manual adoption. + raise _OwnedSpecAuthorityError( + "attempt-owned spec bytes were published but cannot be verified " + f"without descriptor-relative writes: {spec_path}", + published_without_verification=True, ) - if before_signature != after_signature: - raise _OwnedSpecAuthorityError(authority_message) - if restored != snapshot: - raise verify.FrontmatterWriteError(mismatch_message) - fallback_parent_is_canonical() # `require_writable_target=True` (#597): the spec this puts back is # operator-editable, and a temp-and-replace write needs write permission @@ -612,6 +605,71 @@ def _restore_attempt_owned_spec( # it is the only permitted difference from the exact snapshot. cls._normalize_attempt_owned_spec(spec_path, target_status, confine_root=confine_root) + @staticmethod + def _owned_spec_restore_problem( + exc: _OwnedSpecAuthorityError, + *, + unsafe_context: str, + expected_status: str | None = None, + ) -> str: + if exc.published_without_verification: + status_guidance = ( + f"; the adopted spec must have lifecycle status {expected_status!r}" + if expected_status is not None + else "" + ) + return ( + f"restoration bytes were published {unsafe_context}, but this platform " + "cannot verify the resulting file without descriptor-relative writes" + f"{status_guidance}; manual adoption is required" + ) + return f"its path became unsafe {unsafe_context} ({exc})" + + def _restore_attempt_owned_spec_bytes_or_pause( + self, + task: StoryTask, + spec_path: Path, + snapshot: bytes, + *, + unsafe_context: str, + ) -> None: + try: + self._restore_attempt_owned_spec_bytes(spec_path, snapshot) + except _OwnedSpecAuthorityError as exc: + self.pause_for_owned_spec_recovery( + task, + str(spec_path), + self._owned_spec_restore_problem(exc, unsafe_context=unsafe_context), + ) + + def _restore_attempt_owned_spec_or_pause( + self, + task: StoryTask, + spec_path: Path, + snapshot: bytes, + target_status: str, + *, + confine_root: Path, + unsafe_context: str, + ) -> None: + try: + self._restore_attempt_owned_spec( + spec_path, + snapshot, + target_status, + confine_root=confine_root, + ) + except _OwnedSpecAuthorityError as exc: + self.pause_for_owned_spec_recovery( + task, + str(spec_path), + self._owned_spec_restore_problem( + exc, + unsafe_context=unsafe_context, + expected_status=target_status, + ), + ) + def pause_for_owned_spec_recovery( self, task: StoryTask, @@ -999,11 +1057,13 @@ def rollback_or_pause(self, task: StoryTask, *, cause: str = "stopped") -> None: else: if restore_redrive_snapshot: assert task.dispatched_spec_snapshot is not None - self._restore_attempt_owned_spec( + self._restore_attempt_owned_spec_or_pause( + task, spec_path, task.dispatched_spec_snapshot, target_status, confine_root=workspace.paths.project, + unsafe_context="while restoring the pre-attempt retry input", ) owned_snapshot_restored = True else: @@ -1068,8 +1128,13 @@ def rollback_or_pause(self, task: StoryTask, *, cause: str = "stopped") -> None: dirty = True normalized_status = None elif spec_path.read_bytes() != task.dispatched_spec_snapshot: - self._restore_attempt_owned_spec_bytes( - spec_path, task.dispatched_spec_snapshot + self._restore_attempt_owned_spec_bytes_or_pause( + task, + spec_path, + task.dispatched_spec_snapshot, + unsafe_context=( + "while restoring the pre-launch operator input" + ), ) owned_snapshot_restored = True normalized_status = None @@ -1098,15 +1163,12 @@ def rollback_or_pause(self, task: StoryTask, *, cause: str = "stopped") -> None: # recovery policy below. If baseline-status normalization did # not prove the checkout clean, put its spec back byte-for-byte # before that policy claims the tree was left untouched. - try: - self._restore_attempt_owned_spec_bytes(spec_path, original_spec) - except _OwnedSpecAuthorityError as exc: - self.pause_for_owned_spec_recovery( - task, - str(spec_path), - "its path became unsafe while undoing a tentative " - f"lifecycle repair ({exc})", - ) + self._restore_attempt_owned_spec_bytes_or_pause( + task, + spec_path, + original_spec, + unsafe_context="while undoing a tentative lifecycle repair", + ) normalized_status = None if ( owned_snapshot_restored @@ -1236,30 +1298,31 @@ def rollback_or_pause(self, task: StoryTask, *, cause: str = "stopped") -> None: assert task.dispatched_spec_snapshot is not None if redrive: target_status = "in-review" if task.restore_patch else "ready-for-dev" - self._restore_attempt_owned_spec( + self._restore_attempt_owned_spec_or_pause( + task, owned_spec[0], task.dispatched_spec_snapshot, target_status, confine_root=workspace.paths.project, + unsafe_context="before the baseline reset", ) else: - self._restore_attempt_owned_spec_bytes( - owned_spec[0], task.dispatched_spec_snapshot + self._restore_attempt_owned_spec_bytes_or_pause( + task, + owned_spec[0], + task.dispatched_spec_snapshot, + unsafe_context="before the baseline reset", ) owned_snapshot_restored = True self.safe_reset(task, preserve=protected) if restore_attempt_snapshot and owned_spec and owned_exclude and not redrive: assert task.dispatched_spec_snapshot is not None - try: - self._restore_attempt_owned_spec_bytes( - owned_spec[0], task.dispatched_spec_snapshot - ) - except _OwnedSpecAuthorityError as exc: - self.pause_for_owned_spec_recovery( - task, - str(owned_spec[0]), - f"its path became unsafe after the baseline reset ({exc})", - ) + self._restore_attempt_owned_spec_bytes_or_pause( + task, + owned_spec[0], + task.dispatched_spec_snapshot, + unsafe_context="after the baseline reset", + ) owned_snapshot_restored = True if redrive and task.baseline_commit and owned_spec: # A sibling source/artifact change bypasses the earlier spec-only @@ -1279,19 +1342,14 @@ def rollback_or_pause(self, task: StoryTask, *, cause: str = "stopped") -> None: owned_spec[1], ) if restore_redrive_snapshot and task.dispatched_spec_snapshot is not None: - try: - self._restore_attempt_owned_spec( - owned_spec[0], - task.dispatched_spec_snapshot, - target_status, - confine_root=workspace.paths.project, - ) - except _OwnedSpecAuthorityError as exc: - self.pause_for_owned_spec_recovery( - task, - str(owned_spec[0]), - f"its path became unsafe after the baseline reset ({exc})", - ) + self._restore_attempt_owned_spec_or_pause( + task, + owned_spec[0], + task.dispatched_spec_snapshot, + target_status, + confine_root=workspace.paths.project, + unsafe_context="after the baseline reset", + ) else: self._normalize_attempt_owned_spec( owned_spec[0], @@ -1364,7 +1422,12 @@ def rollback_or_pause(self, task: StoryTask, *, cause: str = "stopped") -> None: # restoring the byte-exact pre-launch operator input cannot destroy # evidence even though sibling residue still requires manual policy. assert task.dispatched_spec_snapshot is not None - self._restore_attempt_owned_spec_bytes(owned_spec[0], task.dispatched_spec_snapshot) + self._restore_attempt_owned_spec_bytes_or_pause( + task, + owned_spec[0], + task.dispatched_spec_snapshot, + unsafe_context="before the ordinary manual-recovery pause", + ) restored_before_pause = str(owned_spec[0]) self.journal.append( "rollback-owned-spec-restored", diff --git a/tests/test_recovery_flow.py b/tests/test_recovery_flow.py index 72e4a871f..59b813edc 100644 --- a/tests/test_recovery_flow.py +++ b/tests/test_recovery_flow.py @@ -33,6 +33,10 @@ from bmad_loop.workspace import Workspace QUIET = NotifyPolicy(desktop=False, file=True) +requires_descriptor_restoration = pytest.mark.skipif( + not platform_util.DIR_FD_ANCHORED_WRITES, + reason="automatic restore requires descriptor-relative writes", +) def _policy(**scm) -> Policy: @@ -87,6 +91,7 @@ def _fake_workspace(root: Path, *, output=None, impl=None, plan=None): return SimpleNamespace(root=root, paths=paths) +@requires_descriptor_restoration def test_owned_spec_restore_recreates_missing_canonical_parents(tmp_path): spec = tmp_path.resolve() / "new" / "deep" / "owned.md" snapshot = b"---\nstatus: ready-for-dev\n---\n\noperator input\n" @@ -96,6 +101,20 @@ def test_owned_spec_restore_recreates_missing_canonical_parents(tmp_path): assert spec.read_bytes() == snapshot +def test_owned_spec_restore_forced_fallback_recreates_missing_canonical_parents( + tmp_path, monkeypatch +): + monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) + monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) + spec = tmp_path.resolve() / "new" / "deep" / "owned.md" + snapshot = b"---\nstatus: ready-for-dev\n---\n\noperator input\n" + + with pytest.raises(_OwnedSpecAuthorityError, match="cannot be verified"): + RecoveryFlow._restore_attempt_owned_spec_bytes(spec, snapshot) + + assert spec.read_bytes() == snapshot + + @pytest.mark.parametrize("resolve_fault", NUL_PATH_RESOLVE_FAULTS) def test_owned_spec_restore_translates_value_error_family_before_write( tmp_path, monkeypatch, resolve_fault @@ -270,6 +289,7 @@ def fail_readback(path: Path) -> bytes: assert fh.read() == b"operator bytes\n" +@requires_descriptor_restoration def test_owned_spec_restore_preserves_byte_hostile_snapshot(tmp_path): spec = tmp_path.resolve() / "owned.md" spec.write_bytes(b"old") @@ -280,6 +300,19 @@ def test_owned_spec_restore_preserves_byte_hostile_snapshot(tmp_path): assert spec.read_bytes() == snapshot +def test_owned_spec_restore_forced_fallback_preserves_byte_hostile_snapshot(tmp_path, monkeypatch): + monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) + monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) + spec = tmp_path.resolve() / "owned.md" + spec.write_bytes(b"old") + snapshot = b"---\r\nstatus: caf\xe9\r\n---\r\n\x00tail" + + with pytest.raises(_OwnedSpecAuthorityError, match="cannot be verified"): + RecoveryFlow._restore_attempt_owned_spec_bytes(spec, snapshot) + + assert spec.read_bytes() == snapshot + + @pytest.mark.skipif( not platform_util.DIR_FD_ANCHORED_WRITES, reason="dir-fd anchoring is POSIX-only" ) @@ -514,9 +547,7 @@ def bounded_read(fd: int, size: int) -> bytes: assert requested[-1:] == [len(snapshot) + 1] -def test_owned_spec_restore_fallback_snapshot_plus_suffix_is_a_genuine_mismatch( - tmp_path, monkeypatch -): +def test_owned_spec_restore_fallback_cannot_accept_postpublication_mismatch(tmp_path, monkeypatch): monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) spec = tmp_path.resolve() / "owned.md" @@ -529,12 +560,40 @@ def write_suffix(path, data, **kwargs): monkeypatch.setattr(recovery_flow, "atomic_write_bytes_confined", write_suffix) - with pytest.raises(verify.FrontmatterWriteError): + with pytest.raises(_OwnedSpecAuthorityError, match="cannot be verified") as excinfo: RecoveryFlow._restore_attempt_owned_spec_bytes(spec, snapshot) + assert excinfo.value.published_without_verification is True assert spec.read_bytes() == snapshot + b"x" +def test_owned_spec_restore_fallback_does_not_normalize_unverified_publication( + tmp_path, monkeypatch +): + monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) + monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) + spec = tmp_path.resolve() / "owned.md" + snapshot = b"---\nstatus: ready-for-dev\n---\n\noperator input\n" + normalizations: list[str] = [] + + monkeypatch.setattr( + RecoveryFlow, + "_normalize_attempt_owned_spec", + staticmethod(lambda *_args, **_kwargs: normalizations.append("normalized")), + ) + + with pytest.raises(_OwnedSpecAuthorityError, match="cannot be verified"): + RecoveryFlow._restore_attempt_owned_spec( + spec, + snapshot, + "ready-for-dev", + confine_root=tmp_path, + ) + + assert normalizations == [] + assert spec.read_bytes() == snapshot + + def test_owned_spec_restore_translates_only_confined_writer_refusal(tmp_path, monkeypatch): monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) spec = tmp_path.resolve() / "owned.md" @@ -635,44 +694,121 @@ def fail_read(path: Path) -> bytes: assert fh.read() == b"operator bytes" -def test_owned_spec_restore_fallback_preserves_raw_postpublication_read_failure( - tmp_path, monkeypatch -): - monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) - monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) +def _assert_fallback_replacement_is_not_observed( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + *, + force_fallback: bool, +) -> None: + if force_fallback: + monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) + monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) + else: + assert not recovery_flow.DIR_FD_ANCHORED_WRITES + assert not platform_util.DIR_FD_ANCHORED_WRITES spec = tmp_path.resolve() / "owned.md" spec.write_bytes(b"operator bytes") snapshot = b"snapshot bytes" - failure = OSError("ordinary postpublication read failed") + replacement = b"concurrent regular-file replacement" real_read_bytes = Path.read_bytes + real_path_open = Path.open + real_path_resolve = Path.resolve + real_lstat = os.lstat + real_os_open = os.open + real_os_stat = os.stat real_write = recovery_flow.atomic_write_bytes_confined published = False + target_observations: list[str] = [] - def mark_published(path, data, **kwargs): + def substitute_before_callback(path, data, **kwargs): verify_after = kwargs["_after_replace"] - def mark_then_verify(published_fd: int | None) -> None: + def substitute_then_verify(published_fd: int | None) -> None: nonlocal published + spec.unlink() + spec.write_bytes(replacement) published = True verify_after(published_fd) - kwargs["_after_replace"] = mark_then_verify + kwargs["_after_replace"] = substitute_then_verify return real_write(path, data, **kwargs) - def fail_after_publish(path: Path) -> bytes: + def reject_read_after_publish(path: Path) -> bytes: if path == spec and published: - raise failure + target_observations.append("read") + raise AssertionError("published target was reopened") return real_read_bytes(path) - monkeypatch.setattr(recovery_flow, "atomic_write_bytes_confined", mark_published) - monkeypatch.setattr(Path, "read_bytes", fail_after_publish) + def reject_lstat_after_publish(path, *args, **kwargs): + if Path(path) == spec and published: + target_observations.append("lstat") + raise AssertionError("published target was observed") + return real_lstat(path, *args, **kwargs) - with pytest.raises(OSError) as excinfo: + def reject_path_open_after_publish(path: Path, *args, **kwargs): + if path == spec and published: + target_observations.append("Path.open") + raise AssertionError("published target was opened") + return real_path_open(path, *args, **kwargs) + + def reject_os_open_after_publish(path, *args, **kwargs): + if not isinstance(path, int) and Path(path) == spec and published: + target_observations.append("os.open") + raise AssertionError("published target was opened") + return real_os_open(path, *args, **kwargs) + + def reject_os_stat_after_publish(path, *args, **kwargs): + if not isinstance(path, int) and Path(path) == spec and published: + target_observations.append("os.stat") + raise AssertionError("published target was observed") + return real_os_stat(path, *args, **kwargs) + + def reject_resolve_after_publish(path: Path, *args, **kwargs): + if path == spec and published: + target_observations.append("Path.resolve") + raise AssertionError("published target was resolved") + return real_path_resolve(path, *args, **kwargs) + + monkeypatch.setattr( + recovery_flow, + "atomic_write_bytes_confined", + substitute_before_callback, + ) + monkeypatch.setattr(Path, "read_bytes", reject_read_after_publish) + monkeypatch.setattr(Path, "open", reject_path_open_after_publish) + monkeypatch.setattr(Path, "resolve", reject_resolve_after_publish) + monkeypatch.setattr(recovery_flow.os, "lstat", reject_lstat_after_publish) + monkeypatch.setattr(recovery_flow.os, "open", reject_os_open_after_publish) + monkeypatch.setattr(recovery_flow.os, "stat", reject_os_stat_after_publish) + + with pytest.raises(_OwnedSpecAuthorityError, match="cannot be verified") as excinfo: RecoveryFlow._restore_attempt_owned_spec_bytes(spec, snapshot) - assert excinfo.value is failure - with spec.open("rb") as fh: - assert fh.read() == snapshot + assert excinfo.value.published_without_verification is True + assert target_observations == [] + with real_path_open(spec, "rb") as fh: + assert fh.read() == replacement + + +def test_owned_spec_restore_forced_fallback_never_observes_replaced_target_after_publication( + tmp_path, monkeypatch +): + _assert_fallback_replacement_is_not_observed( + tmp_path, + monkeypatch, + force_fallback=True, + ) + + +@pytest.mark.skipif(sys.platform != "win32", reason="native Windows coverage") +def test_owned_spec_restore_native_windows_never_observes_replaced_target_after_publication( + tmp_path, monkeypatch +): + _assert_fallback_replacement_is_not_observed( + tmp_path, + monkeypatch, + force_fallback=False, + ) @pytest.mark.skipif( @@ -716,7 +852,8 @@ def record_root(path, data, **kwargs): monkeypatch.setattr(recovery_flow, "atomic_write_bytes_confined", record_root) - RecoveryFlow._restore_attempt_owned_spec_bytes(spec, snapshot) + with pytest.raises(_OwnedSpecAuthorityError, match="cannot be verified"): + RecoveryFlow._restore_attempt_owned_spec_bytes(spec, snapshot) assert roots == [Path(spec.anchor)] assert spec.read_bytes() == snapshot @@ -1377,14 +1514,14 @@ def mutate_then_read(fd: int, size: int) -> bytes: assert spec.read_bytes() == snapshot + b"x" -def test_owned_spec_restore_fallback_rejects_in_place_mutation(tmp_path, monkeypatch): +def test_owned_spec_restore_fallback_does_not_reopen_published_target(tmp_path, monkeypatch): monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) spec = tmp_path.resolve() / "owned.md" spec.write_bytes(b"operator bytes") snapshot = b"snapshot bytes" real_read_bytes = Path.read_bytes - mutated = False + postpublication_reads = 0 published = False real_write = recovery_flow.atomic_write_bytes_confined @@ -1400,22 +1537,20 @@ def mark_then_verify(published_fd: int | None) -> None: kwargs["_after_replace"] = mark_then_verify return real_write(path, data, **kwargs) - def mutate_then_read(path: Path) -> bytes: - nonlocal mutated - if path == spec and published and not mutated: - mutated = True - with spec.open("ab") as fh: - fh.write(b"x") + def record_read(path: Path) -> bytes: + nonlocal postpublication_reads + if path == spec and published: + postpublication_reads += 1 return real_read_bytes(path) monkeypatch.setattr(recovery_flow, "atomic_write_bytes_confined", mark_published) - monkeypatch.setattr(Path, "read_bytes", mutate_then_read) + monkeypatch.setattr(Path, "read_bytes", record_read) - with pytest.raises(_OwnedSpecAuthorityError, match="became unsafe"): + with pytest.raises(_OwnedSpecAuthorityError, match="cannot be verified"): RecoveryFlow._restore_attempt_owned_spec_bytes(spec, snapshot) - assert mutated is True - assert real_read_bytes(spec) == snapshot + b"x" + assert postpublication_reads == 0 + assert real_read_bytes(spec) == snapshot @pytest.mark.skipif( @@ -1576,6 +1711,36 @@ def _status(spec: Path) -> str: return verify.status_of(verify.read_frontmatter(spec)) +def _assert_owned_spec_manual_adoption_pause( + flow: RecoveryFlow, + task: StoryTask, + spec: Path, + *, + stage: str, + expected_status: str | None = None, +) -> None: + assert task.dispatched_spec_file is None + assert task.dispatched_spec_snapshot is None + assert flow.calls.saves == 1 + assert len(flow.calls.pauses) == 1 + assert "manual adoption is required" in flow.calls.pauses[0][0] + assert flow.journal.events().count("rollback-owned-spec-manual-required") == 1 + status_guidance = ( + f"; the adopted spec must have lifecycle status {expected_status!r}" + if expected_status is not None + else "" + ) + assert flow.journal.fields("rollback-owned-spec-manual-required") == { + "story_key": task.story_key, + "spec": str(spec.resolve()), + "problem": ( + f"restoration bytes were published {stage}, but this platform cannot verify " + "the resulting file without descriptor-relative writes" + f"{status_guidance}; manual adoption is required" + ), + } + + # --------------------------------------------------------------- protected paths @@ -1874,6 +2039,35 @@ def test_plain_owned_spec_with_substantive_residue_still_pauses(project): assert flow.calls.emits == [] +def test_plain_owned_spec_forced_fallback_pauses_while_undoing_lifecycle_repair( + project, monkeypatch +): + repo = project.project + spec = _tracked_spec(project) + task = _task(repo) + task.dispatched_spec_file = str(spec) + child = b"---\nstatus: in-progress\n---\n\nfailed child body\n" + spec.write_bytes(child) + flow = _make_flow( + workspace=Workspace.default(project), policy=_policy(rollback_on_failure=False) + ) + monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) + monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) + + with pytest.raises(_Pause, match="undoing a tentative lifecycle repair"): + flow.rollback_or_pause(task) + + assert spec.read_bytes() == child + assert "rollback-manual-required" not in flow.journal.events() + assert "rollback-owned-spec-normalized" not in flow.journal.events() + _assert_owned_spec_manual_adoption_pause( + flow, + task, + spec, + stage="while undoing a tentative lifecycle repair", + ) + + def test_bound_spec_exclusion_does_not_hide_sibling_source_residue(project): """T10/source: source debris keeps the ordinary rollback policy reachable. @@ -1969,6 +2163,7 @@ def test_unbound_spec_flip_retains_existing_dirty_policy(project): assert "rollback-owned-spec-normalized" not in flow.journal.events() +@requires_descriptor_restoration def test_plain_tracked_snapshot_restores_operator_bytes_child_reverted_to_baseline(project): """Git-clean child output cannot erase dirty input present before launch.""" repo = project.project @@ -2000,6 +2195,37 @@ def test_plain_tracked_snapshot_restores_operator_bytes_child_reverted_to_baseli assert flow.calls.pauses == [] +def test_plain_tracked_snapshot_forced_fallback_pauses_after_publication(project, monkeypatch): + repo = project.project + spec = _tracked_spec(project) + baseline = spec.read_bytes() + operator = baseline.replace(b"baseline intent", b"operator input outside HEAD") + spec.write_bytes(operator) + task = _task(repo) + task.dispatched_spec_file = str(spec) + task.dispatched_spec_snapshot = operator + spec.write_bytes(baseline) + flow = _make_flow( + workspace=Workspace.default(project), policy=_policy(rollback_on_failure=False) + ) + monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) + monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) + + with pytest.raises(_Pause, match="manual adoption is required"): + flow.rollback_or_pause(task) + + assert spec.read_bytes() == operator + assert "rollback-owned-spec-restored" not in flow.journal.events() + assert "rollback-skipped-clean" not in flow.journal.events() + _assert_owned_spec_manual_adoption_pause( + flow, + task, + spec, + stage="while restoring the pre-launch operator input", + ) + + +@requires_descriptor_restoration def test_plain_auto_reset_restores_unchanged_prelaunch_operator_spec(project): """Sibling rollback cannot erase tracked operator input the child inherited. @@ -2033,6 +2259,39 @@ def test_plain_auto_reset_restores_unchanged_prelaunch_operator_spec(project): } +def test_plain_forced_fallback_pauses_after_completed_baseline_reset(project, monkeypatch): + repo = project.project + spec = _tracked_spec(project) + baseline = spec.read_bytes() + operator = baseline.replace(b"baseline intent", b"operator input outside HEAD") + spec.write_bytes(operator) + task = _task(repo) + task.dispatched_spec_file = str(spec) + task.dispatched_spec_snapshot = operator + source = repo / "src.txt" + source.write_text("failed child sibling\n") + flow = _make_flow( + workspace=Workspace.default(project), policy=_policy(rollback_on_failure=True) + ) + monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) + monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) + + with pytest.raises(_Pause, match="after the baseline reset"): + flow.rollback_or_pause(task) + + assert source.read_text() == "original\n" + assert spec.read_bytes() == operator + assert "rollback-auto" in flow.journal.events() + assert flow.calls.emits == ["pre_rollback"] + _assert_owned_spec_manual_adoption_pause( + flow, + task, + spec, + stage="after the baseline reset", + ) + + +@requires_descriptor_restoration def test_plain_manual_pause_restores_operator_spec_child_put_at_baseline_with_sibling(project): """A sibling does not hide a provably baseline-shaped child spec deletion. @@ -2075,6 +2334,39 @@ def test_plain_manual_pause_restores_operator_spec_child_put_at_baseline_with_si assert task.preserve_ref is None +def test_plain_sibling_residue_forced_fallback_preempts_generic_manual_pause(project, monkeypatch): + repo = project.project + spec = _tracked_spec(project) + baseline = spec.read_bytes() + operator = baseline.replace(b"baseline intent", b"operator input outside HEAD") + task = _task(repo) + task.dispatched_spec_file = str(spec) + task.dispatched_spec_snapshot = operator + spec.write_bytes(baseline) + source = repo / "src.txt" + source.write_text("failed child sibling\n") + flow = _make_flow( + workspace=Workspace.default(project), policy=_policy(rollback_on_failure=False) + ) + monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) + monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) + + with pytest.raises(_Pause, match="before the ordinary manual-recovery pause"): + flow.rollback_or_pause(task) + + assert spec.read_bytes() == operator + assert source.read_text() == "failed child sibling\n" + assert "rollback-owned-spec-restored" not in flow.journal.events() + assert "rollback-manual-required" not in flow.journal.events() + _assert_owned_spec_manual_adoption_pause( + flow, + task, + spec, + stage="before the ordinary manual-recovery pause", + ) + + +@requires_descriptor_restoration def test_latched_redrive_reports_owned_corrected_spec_as_still_dirty(project): """T12: failed child body edits restore the pre-attempt human correction. @@ -2113,6 +2405,41 @@ def test_latched_redrive_reports_owned_corrected_spec_as_still_dirty(project): assert flow.calls.emits == ["pre_rollback", "post_rollback"] +def test_latched_redrive_snapshot_equal_forced_fallback_pauses_for_retry_input( + project, monkeypatch +): + repo = project.project + spec = _tracked_spec(project) + baseline = spec.read_bytes() + corrected = baseline.replace(b"baseline intent", b"human corrected intent") + spec.write_bytes(corrected) + task = _task(repo) + task.dispatched_spec_file = str(spec) + task.dispatched_spec_snapshot = corrected + task.resolved_redrive = True + flow = _make_flow( + workspace=Workspace.default(project), policy=_policy(rollback_on_failure=False) + ) + monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) + monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) + + with pytest.raises(_Pause, match="pre-attempt retry input"): + flow.rollback_or_pause(task) + + assert spec.read_bytes() == corrected + assert task.preserve_ref is None + assert "rollback-owned-spec-normalized" not in flow.journal.events() + assert "rollback-skipped-clean" not in flow.journal.events() + _assert_owned_spec_manual_adoption_pause( + flow, + task, + spec, + stage="while restoring the pre-attempt retry input", + expected_status="ready-for-dev", + ) + + +@requires_descriptor_restoration def test_latched_redrive_restores_preexisting_untracked_spec_by_snapshot(project): """Git's baseline-untracked name set cannot hide child edits to its contents. @@ -2148,6 +2475,81 @@ def test_latched_redrive_restores_preexisting_untracked_spec_by_snapshot(project assert flow.calls.pauses == [] +def test_latched_redrive_forced_fallback_pauses_after_preservation(project, monkeypatch): + repo = project.project + spec = project.implementation_artifacts / "untracked-redrive-fallback.md" + spec.parent.mkdir(parents=True, exist_ok=True) + corrected = b"---\nstatus: ready-for-dev\n---\n\nhuman corrected input\n" + child = b"---\nstatus: done\n---\n\nfailed child input\n" + spec.write_bytes(corrected) + task = _task(repo) + rel = spec.relative_to(repo).as_posix() + task.baseline_untracked = [rel] + task.dispatched_spec_file = str(spec) + task.dispatched_spec_snapshot = corrected + task.resolved_redrive = True + spec.write_bytes(child) + flow = _make_flow( + workspace=Workspace.default(project), policy=_policy(rollback_on_failure=False) + ) + monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) + monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) + + with pytest.raises(_Pause, match="manual adoption is required"): + flow.rollback_or_pause(task) + + assert spec.read_bytes() == corrected + assert task.preserve_ref is not None + assert git(repo, "show", f"{task.preserve_ref}:{rel}").encode() == child.rstrip(b"\n") + assert "attempt-worktree-preserved" in flow.journal.events() + assert "rollback-owned-spec-restored" not in flow.journal.events() + assert "post_rollback" not in flow.calls.emits + _assert_owned_spec_manual_adoption_pause( + flow, + task, + spec, + stage="before the baseline reset", + expected_status="ready-for-dev", + ) + + +def test_latched_redrive_forced_fallback_pauses_after_completed_baseline_reset( + project, monkeypatch +): + repo = project.project + source = repo / "redrive-source.txt" + source.write_text("baseline source\n") + spec = _tracked_spec(project) + corrected = b"---\nstatus: ready-for-dev\n---\n\nhuman corrected intent\n" + spec.write_bytes(corrected) + task = _task(repo) + task.dispatched_spec_file = str(spec) + task.dispatched_spec_snapshot = corrected + task.resolved_redrive = True + source.write_text("failed child sibling\n") + flow = _make_flow( + workspace=Workspace.default(project), policy=_policy(rollback_on_failure=True) + ) + monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) + monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) + + with pytest.raises(_Pause, match="after the baseline reset"): + flow.rollback_or_pause(task) + + assert source.read_text() == "baseline source\n" + assert spec.read_bytes() == corrected + assert "rollback-auto" in flow.journal.events() + assert flow.calls.emits == ["pre_rollback"] + _assert_owned_spec_manual_adoption_pause( + flow, + task, + spec, + stage="after the baseline reset", + expected_status="ready-for-dev", + ) + + +@requires_descriptor_restoration def test_latched_redrive_restores_ignored_spec_as_untracked_after_child_commit(project): """A failed child cannot turn restored ignored input into a staged addition.""" repo = project.project @@ -2183,6 +2585,7 @@ def test_latched_redrive_restores_ignored_spec_as_untracked_after_child_commit(p @pytest.mark.parametrize("git_invisible", ["baseline-untracked", "ignored"]) @pytest.mark.parametrize("child_index", ["staged", "committed"]) +@requires_descriptor_restoration def test_plain_reset_recreates_force_added_git_invisible_snapshot( project, git_invisible, child_index ): @@ -2220,7 +2623,52 @@ def test_plain_reset_recreates_force_added_git_invisible_snapshot( assert "rollback-owned-spec-restored" in flow.journal.events() +@pytest.mark.parametrize("git_invisible", ["baseline-untracked", "ignored"]) +def test_plain_git_invisible_snapshot_forced_fallback_pauses_before_reset( + project, monkeypatch, git_invisible +): + repo = project.project + spec = project.implementation_artifacts / f"fallback-{git_invisible}.md" + rel = spec.relative_to(repo).as_posix() + if git_invisible == "ignored": + (repo / ".gitignore").write_text(f"/{rel}\n") + git(repo, "add", ".gitignore") + git(repo, "commit", "-q", "-m", "ignore fallback owned spec") + spec.parent.mkdir(parents=True, exist_ok=True) + original = b"---\nstatus: ready-for-dev\n---\n\noperator input\n" + child = b"---\nstatus: done\n---\n\nfailed child input\n" + spec.write_bytes(original) + task = _task(repo) + task.baseline_untracked = [rel] if git_invisible == "baseline-untracked" else [] + task.dispatched_spec_file = str(spec) + task.dispatched_spec_snapshot = original + spec.write_bytes(child) + git(repo, "add", "-f", rel) + flow = _make_flow( + workspace=Workspace.default(project), policy=_policy(rollback_on_failure=True) + ) + monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) + monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) + + with pytest.raises(_Pause, match="manual adoption is required"): + flow.rollback_or_pause(task) + + assert spec.read_bytes() == original + assert verify.index_path_changed_since(repo, task.baseline_commit, rel) + assert task.preserve_ref is not None + assert git(repo, "show", f"{task.preserve_ref}:{rel}").encode() == child.rstrip(b"\n") + assert "rollback-owned-spec-restored" not in flow.journal.events() + assert "post_rollback" not in flow.calls.emits + _assert_owned_spec_manual_adoption_pause( + flow, + task, + spec, + stage="before the baseline reset", + ) + + @pytest.mark.parametrize("baseline_present", [False, True], ids=["force-add", "cached-remove"]) +@requires_descriptor_restoration def test_latched_redrive_resets_index_only_owned_spec_mutation(project, baseline_present): """Snapshot-equal bytes cannot hide a child-authored index ownership change.""" repo = project.project @@ -2237,7 +2685,8 @@ def test_latched_redrive_resets_index_only_owned_spec_mutation(project, baseline rel = spec.relative_to(repo).as_posix() task = _task(repo) task.dispatched_spec_file = str(spec) - task.dispatched_spec_snapshot = spec.read_bytes() + snapshot = spec.read_bytes() + task.dispatched_spec_snapshot = snapshot task.resolved_redrive = True if baseline_present: git(repo, "rm", "--cached", rel) @@ -2250,13 +2699,66 @@ def test_latched_redrive_resets_index_only_owned_spec_mutation(project, baseline flow.rollback_or_pause(task) - assert spec.read_bytes() == task.dispatched_spec_snapshot + assert spec.read_bytes() == snapshot assert verify.path_tracked(repo, rel) is baseline_present assert not verify.index_path_changed_since(repo, task.baseline_commit, rel) assert "rollback-auto" in flow.journal.events() assert "rollback-owned-spec-restored" in flow.journal.events() +@pytest.mark.parametrize("baseline_present", [False, True], ids=["force-add", "cached-remove"]) +def test_latched_redrive_index_only_forced_fallback_pauses_after_preservation( + project, monkeypatch, baseline_present +): + repo = project.project + if baseline_present: + spec = _tracked_spec(project, name="tracked-index-only-fallback.md") + else: + spec = project.implementation_artifacts / "ignored-index-only-fallback.md" + rel = spec.relative_to(repo).as_posix() + (repo / ".gitignore").write_text(f"/{rel}\n") + git(repo, "add", ".gitignore") + git(repo, "commit", "-q", "-m", "ignore fallback index-only spec") + spec.parent.mkdir(parents=True, exist_ok=True) + spec.write_text("---\nstatus: ready-for-dev\n---\n\noperator input\n") + rel = spec.relative_to(repo).as_posix() + task = _task(repo) + task.dispatched_spec_file = str(spec) + snapshot = spec.read_bytes() + task.dispatched_spec_snapshot = snapshot + task.resolved_redrive = True + if baseline_present: + git(repo, "rm", "--cached", rel) + else: + git(repo, "add", "-f", rel) + flow = _make_flow( + workspace=Workspace.default(project), policy=_policy(rollback_on_failure=False) + ) + monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) + monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) + + with pytest.raises(_Pause, match="before the baseline reset"): + flow.rollback_or_pause(task) + + assert spec.read_bytes() == snapshot + assert verify.index_path_changed_since(repo, task.baseline_commit, rel) + if baseline_present: + assert task.preserve_ref is None # cached removal has no child bytes to park + assert "attempt-worktree-preserved" not in flow.journal.events() + else: + assert task.preserve_ref is not None + assert "attempt-worktree-preserved" in flow.journal.events() + assert "rollback-owned-spec-restored" not in flow.journal.events() + assert "post_rollback" not in flow.calls.emits + _assert_owned_spec_manual_adoption_pause( + flow, + task, + spec, + stage="before the baseline reset", + expected_status="ready-for-dev", + ) + + @pytest.mark.skipif(sys.platform == "win32", reason="directory symlinks may need elevation") def test_plain_reset_refuses_baseline_parent_retarget_before_mutation(project, tmp_path): """A reset cannot turn canonical snapshot authority into an external write.""" @@ -2496,6 +2998,7 @@ def retarget_after_reset(reset_task, *, preserve=()): @pytest.mark.parametrize("git_invisible", ["baseline-untracked", "ignored"]) +@requires_descriptor_restoration def test_plain_attempt_restores_and_parks_git_invisible_owned_spec(project, git_invisible): """A plain child cannot hide body edits in Git's untracked blind spots. @@ -2562,6 +3065,7 @@ def test_unchanged_ignored_owned_spec_with_snapshot_is_a_clean_noop(project): assert task.preserve_ref is None +@requires_descriptor_restoration def test_latched_redrive_reset_normalizes_preserved_spec_after_sibling_residue(project): """A non-fixable retry re-establishes the route its next prompt declares. @@ -2802,6 +3306,7 @@ def test_plain_attempt_refuses_to_overwrite_changed_external_spec(project, tmp_p assert task.dispatched_spec_snapshot is None +@requires_descriptor_restoration def test_latched_redrive_parks_child_commit_and_restores_operator_snapshot(project): """Committed child body edits cannot hide behind the retained correction. @@ -2863,6 +3368,7 @@ def test_latched_redrive_refuses_restore_when_child_commit_cannot_be_parked(proj assert "attempt-worktree-preserved" not in flow.journal.events() +@requires_descriptor_restoration def test_latched_redrive_preserves_uncommitted_child_bytes_above_child_commit(project): """The dirty preserve ref retains the child's latest uncommitted spec body. @@ -2936,6 +3442,7 @@ def fault_post_normalization_probe(*args, **kwargs): assert b"stale bytes" not in spec.read_bytes() +@requires_descriptor_restoration def test_patch_restore_redrive_normalizes_owned_spec_to_in_review(project): """T13: the restore latch selects `in-review`, never from-scratch readiness. @@ -2965,6 +3472,37 @@ def test_patch_restore_redrive_normalizes_owned_spec_to_in_review(project): assert flow.calls.emits == ["pre_rollback", "post_rollback"] +def test_patch_restore_redrive_forced_fallback_requires_in_review_adoption(project, monkeypatch): + repo = project.project + spec = _tracked_spec(project, status="in-review") + task = _task(repo) + task.dispatched_spec_file = str(spec) + task.restore_patch = "intent-gap.patch" + task.resolved_redrive = True + restored = b"---\nstatus: in-review\n---\n\nrestored human correction\n" + task.dispatched_spec_snapshot = restored + spec.write_text("---\nstatus: in-progress\n---\n\nrestored human correction\n") + flow = _make_flow( + workspace=Workspace.default(project), policy=_policy(rollback_on_failure=False) + ) + monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) + monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) + + with pytest.raises(_Pause, match="lifecycle status 'in-review'"): + flow.rollback_or_pause(task) + + assert spec.read_bytes() == restored + assert "rollback-owned-spec-normalized" not in flow.journal.events() + assert "post_rollback" not in flow.calls.emits + _assert_owned_spec_manual_adoption_pause( + flow, + task, + spec, + stage="before the baseline reset", + expected_status="in-review", + ) + + def test_owned_spec_without_visible_status_fails_the_post_write_oracle(project): """T14/False: a writer no-op is not repair success without the target oracle. From 545e3bc15699a9f152ca641ddbf8bf575455eab3 Mon Sep 17 00:00:00 2001 From: t Date: Mon, 14 Sep 2026 10:26:13 -0700 Subject: [PATCH 03/17] sweep dw2-fail-closed-fallback-spec-restore: DW-310 via bmad-loop --- docs/FEATURES.md | 2 +- src/bmad_loop/recovery_flow.py | 214 +++++------ src/bmad_loop/sweep.py | 5 +- tests/test_recovery_flow.py | 677 +++++++-------------------------- 4 files changed, 225 insertions(+), 673 deletions(-) diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 0f9c5bc68..f590566d5 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -193,7 +193,7 @@ Result` section. Every other spec keeps warn-and-continue, and the record says w (#705). Sweep migration and triage tasks make the same rollover automatically when an `ESCALATED` task restarts with a fresh attempt budget; mid-flight, non-escalated restarts keep their current generation because their continuing attempt counter already provides a fresh id. -- Attempt-owned sprint-spec recovery (#123, #630): a bound plain attempt whose only residue is its own lifecycle flip is normalized back to its pre-attempt lifecycle status, proven Git-clean, and retried. Every bound retry chain snapshots its first spec input byte-for-byte and retains it across both dev-verification and review-verification repair sessions; a resolved re-drive therefore retains the operator-corrected `ready-for-dev` input rather than a failed child's later body. Repair entry points validate retained authority before constructing a prompt that can reset the spec. A non-fixable retry parks the failed child first, restores that snapshot, and re-establishes the promised route after resetting sibling residue. Descriptor-capable restoration retains the staged inode across publication and verifies that exact inode before accepting it. A no-descriptor fallback still publishes the snapshot byte-for-byte, but performs no post-publication path readback, refuses to accept or normalize the unverifiable result, and pauses for manual adoption. The same snapshot restores pre-launch operator edits when a plain child puts a tracked spec back at Git baseline. Git-ignored and pre-existing-untracked bound specs use the byte snapshot as their dirtiness oracle and are force-included only in the private recovery ref before restoration; index-only force-adds and cached removals also trigger cleanup and restore baseline index ownership. That real repair reports `rollback-owned-spec-restored`, never `rollback-skipped-clean`. Missing, unreadable, deleted, retargeted, changed external, or unsafe legacy authority pauses once with spec-specific adoption instructions and clears the unusable pair so manual recovery can converge; recovery also refuses a reset whose baseline would replace the canonical path or a parent directory with a symlink, tree, file, or other unsafe shape. An initial Sprint binding fault may safely degrade to an unbound bare-key launch; an existing Stories folder+id target instead aborts unless it can be snapshotted. Once an explicit binding is durable, a later snapshot fault aborts before child launch while retaining that authority for recovery. Fresh sprint tasks with no recorded path remain bare-key dispatches; other substantive changes or sibling residue follow rollback policy; Stories remains folder+id; Sweep remains intent-bundle routing; snapshots are retired after commit; and recovery never auto-commits the human correction. +- Attempt-owned sprint-spec recovery (#123, #630): a bound plain attempt whose only residue is its own lifecycle flip is normalized back to its pre-attempt lifecycle status, proven Git-clean, and retried. Every bound retry chain snapshots its first spec input byte-for-byte and retains it across both dev-verification and review-verification repair sessions; a resolved re-drive therefore retains the operator-corrected `ready-for-dev` input rather than a failed child's later body. Repair entry points validate retained authority before constructing a prompt that can reset the spec. A non-fixable retry parks the failed child first, restores that snapshot, and re-establishes the promised route after resetting sibling residue. Descriptor-capable restoration retains the staged inode across publication and verifies that exact inode before accepting it. A platform without descriptor-relative writes refuses before staging or lifecycle normalization, leaves the current attempt-owned spec bytes untouched by restoration, and pauses for manual adoption. On descriptor-capable platforms, the same snapshot restores pre-launch operator edits when a plain child puts a tracked spec back at Git baseline. Git-ignored and pre-existing-untracked bound specs use the byte snapshot as their dirtiness oracle and are force-included only in the private recovery ref before restoration; index-only force-adds and cached removals also trigger cleanup and restore baseline index ownership. That real repair reports `rollback-owned-spec-restored`, never `rollback-skipped-clean`. Missing, unreadable, deleted, retargeted, changed external, or unsafe legacy authority pauses once with spec-specific adoption instructions and clears the unusable pair so manual recovery can converge; recovery also refuses a reset whose baseline would replace the canonical path or a parent directory with a symlink, tree, file, or other unsafe shape. An initial Sprint binding fault may safely degrade to an unbound bare-key launch; an existing Stories folder+id target instead aborts unless it can be snapshotted. Once an explicit binding is durable, a later snapshot fault aborts before child launch while retaining that authority for recovery. Fresh sprint tasks with no recorded path remain bare-key dispatches; other substantive changes or sibling residue follow rollback policy; Stories remains folder+id; Sweep remains intent-bundle routing; snapshots are retired after commit; and recovery never auto-commits the human correction. - Intent-gap patch-restore (BMAD-METHOD#2564): when review halts on an `intent gap`, the dev primitive saves the attempted change as a patch file (referenced from the halt output) before reverting the tree. If that reading turns out to be correct, the resolve agent adds `"restore_patch": ""` to its `resolution.json`; the orchestrator re-arms the spec to `in-review` (not `ready-for-dev`) and re-applies the patch after every reset, so the re-driven session resumes _review_ on the restored diff instead of re-implementing. `bmad-loop resolve --no-interactive --restore-patch ` does the same by hand. A patch that fails to apply escalates rather than dispatching onto a half-restored tree. Sweep bundles get the same recovery. ### Git worktree isolation (opt-in) diff --git a/src/bmad_loop/recovery_flow.py b/src/bmad_loop/recovery_flow.py index ef3547165..0d5d0a0de 100644 --- a/src/bmad_loop/recovery_flow.py +++ b/src/bmad_loop/recovery_flow.py @@ -25,9 +25,7 @@ from .model import Phase from .platform_util import ( DIR_FD_ANCHORED_WRITES, - UnconfinedWriteError, atomic_write_bytes_at, - atomic_write_bytes_confined, open_dir_confined, safe_ref_segment, ) @@ -63,10 +61,10 @@ def __init__( self, message: str, *, - published_without_verification: bool = False, + safe_restoration_unavailable: bool = False, ) -> None: super().__init__(message) - self.published_without_verification = published_without_verification + self.safe_restoration_unavailable = safe_restoration_unavailable def _target_stat_version(observed: os.stat_result) -> tuple[int, int, int]: @@ -226,7 +224,17 @@ def _normalize_attempt_owned_spec( confinement exists for. It reaches the spec-writer chokepoint rule stated in `frontmatter.set_frontmatter_status` — an artifacts folder configured outside the project is a trusted repair target here - (`_attempt_owned_spec`) and keeps the plain no-follow write.""" + (`_attempt_owned_spec`) when descriptor-relative writes are available.""" + # A path-based fallback cannot retain publication authority across the + # final replace. Refuse before any repair write so a substituted parent + # or target cannot redirect staging, publication, or cleanup. + if not DIR_FD_ANCHORED_WRITES: + raise _OwnedSpecAuthorityError( + "safe automatic attempt-owned spec restoration is unavailable because " + "it cannot be verified without descriptor-relative writes: " + f"{spec_path}", + safe_restoration_unavailable=True, + ) verify.set_frontmatter_status(spec_path, target_status, confine_root=confine_root) if verify.status_of(verify.read_frontmatter(spec_path)) != target_status: raise verify.FrontmatterWriteError( @@ -283,6 +291,14 @@ def _restore_attempt_owned_spec_bytes(spec_path: Path, snapshot: bytes) -> None: f"attempt-owned spec target could not be revalidated: {spec_path}" ) from exc + if not DIR_FD_ANCHORED_WRITES: + raise _OwnedSpecAuthorityError( + "safe automatic attempt-owned spec restoration is unavailable because " + "it cannot be verified without descriptor-relative writes: " + f"{spec_path}", + safe_restoration_unavailable=True, + ) + # Creation is a repair write. Preserve the established typed translation # for OS/symlink-loop failures, but let a ValueError from mkdir itself # escape raw rather than misclassifying it as an authority probe failure. @@ -467,127 +483,36 @@ def verify_published_inode(parent_fd: int, published_fd: int) -> None: # replacing the retained descriptor as authority. verify_parent_authority(parent_fd) - def fallback_parent_is_canonical() -> None: - try: - if ( - not parent.is_dir() - or parent.is_symlink() - or parent.resolve(strict=True) != parent - ): - raise _OwnedSpecAuthorityError(authority_message) - except _OwnedSpecAuthorityError: - raise - except (OSError, RuntimeError, ValueError) as exc: - raise _OwnedSpecAuthorityError(authority_message) from exc - - def fallback_target_stat() -> os.stat_result | None: - try: - observed = os.lstat(spec_path) - except FileNotFoundError: - return None - if not stat.S_ISREG(observed.st_mode): - raise _OwnedSpecAuthorityError(authority_message) - return observed - - def read_fallback_target() -> tuple[os.stat_result, bytes] | None: - before = fallback_target_stat() - if before is None: - return None - contents = spec_path.read_bytes() - after = fallback_target_stat() - if ( - after is None - or len(contents) != before.st_size - or not os.path.samestat(before, after) - or _target_stat_version(before) != _target_stat_version(after) - ): - raise _OwnedSpecAuthorityError(authority_message) - return after, contents - - def require_same_fallback_target( - expected: tuple[os.stat_result, bytes] | None, - ) -> None: - fallback_parent_is_canonical() - observed = read_fallback_target() - if expected is None: - if observed is not None: - raise _OwnedSpecAuthorityError(authority_message) - return - if observed is None: - raise _OwnedSpecAuthorityError(authority_message) - expected_stat, expected_bytes = expected - observed_stat, observed_bytes = observed - if ( - not os.path.samestat(expected_stat, observed_stat) - or _target_stat_version(expected_stat) != _target_stat_version(observed_stat) - or expected_bytes != observed_bytes - ): - raise _OwnedSpecAuthorityError(authority_message) - - def verify_fallback_bytes(_published_fd: int | None) -> None: - # The fallback writer no longer owns an inode-bound descriptor once - # publication completes. Reopening or even observing the final name - # would make a concurrent replacement authoritative. Publication has - # already happened, so refuse it without touching the target path and - # let the recovery state machine require explicit manual adoption. - raise _OwnedSpecAuthorityError( - "attempt-owned spec bytes were published but cannot be verified " - f"without descriptor-relative writes: {spec_path}", - published_without_verification=True, - ) - # `require_writable_target=True` (#597): the spec this puts back is # operator-editable, and a temp-and-replace write needs write permission # on the parent directory, never on the entry it replaces. Anchor from # the filesystem root rather than the project so configured external # artifact roots remain valid repair targets. - if DIR_FD_ANCHORED_WRITES: - parent_fd = open_dir_confined(Path(spec_path.anchor), parent, search_only=True) - if parent_fd is None: - raise _OwnedSpecAuthorityError( - f"attempt-owned spec target could not be revalidated: {spec_path}" - ) - try: - expected = read_target_at(parent_fd) - - def validate_target() -> None: - require_same_target_at(parent_fd, expected) - - def verify_published(published_fd: int) -> None: - verify_published_inode(parent_fd, published_fd) - - atomic_write_bytes_at( - parent_fd, - spec_path.name, - snapshot, - _require_writable_target=True, - _before_staging=validate_target, - _before_replace=validate_target, - _after_replace=verify_published, - ) - finally: - os.close(parent_fd) - return + parent_fd = open_dir_confined(Path(spec_path.anchor), parent, search_only=True) + if parent_fd is None: + raise _OwnedSpecAuthorityError( + f"attempt-owned spec target could not be revalidated: {spec_path}" + ) + try: + expected = read_target_at(parent_fd) - expected = read_fallback_target() + def validate_target() -> None: + require_same_target_at(parent_fd, expected) - def validate_fallback_target() -> None: - require_same_fallback_target(expected) + def verify_published(published_fd: int) -> None: + verify_published_inode(parent_fd, published_fd) - try: - atomic_write_bytes_confined( - spec_path, + atomic_write_bytes_at( + parent_fd, + spec_path.name, snapshot, - confine_root=Path(spec_path.anchor), - require_writable_target=True, - _before_staging=validate_fallback_target, - _before_replace=validate_fallback_target, - _after_replace=verify_fallback_bytes, + _require_writable_target=True, + _before_staging=validate_target, + _before_replace=validate_target, + _after_replace=verify_published, ) - except UnconfinedWriteError as exc: - raise _OwnedSpecAuthorityError( - f"attempt-owned spec target could not be revalidated: {spec_path}" - ) from exc + finally: + os.close(parent_fd) @classmethod def _restore_attempt_owned_spec( @@ -612,15 +537,15 @@ def _owned_spec_restore_problem( unsafe_context: str, expected_status: str | None = None, ) -> str: - if exc.published_without_verification: + if exc.safe_restoration_unavailable: status_guidance = ( f"; the adopted spec must have lifecycle status {expected_status!r}" if expected_status is not None else "" ) return ( - f"restoration bytes were published {unsafe_context}, but this platform " - "cannot verify the resulting file without descriptor-relative writes" + f"safe automatic restoration is unavailable {unsafe_context} because " + "this platform lacks descriptor-relative writes" f"{status_guidance}; manual adoption is required" ) return f"its path became unsafe {unsafe_context} ({exc})" @@ -670,6 +595,32 @@ def _restore_attempt_owned_spec_or_pause( ), ) + def _normalize_attempt_owned_spec_or_pause( + self, + task: StoryTask, + spec_path: Path, + target_status: str, + *, + confine_root: Path, + unsafe_context: str, + ) -> None: + try: + self._normalize_attempt_owned_spec( + spec_path, + target_status, + confine_root=confine_root, + ) + except _OwnedSpecAuthorityError as exc: + self.pause_for_owned_spec_recovery( + task, + str(spec_path), + self._owned_spec_restore_problem( + exc, + unsafe_context=unsafe_context, + expected_status=target_status, + ), + ) + def pause_for_owned_spec_recovery( self, task: StoryTask, @@ -741,13 +692,14 @@ def rollback_or_pause(self, task: StoryTask, *, cause: str = "stopped") -> None: through instead of re-pausing on the still-set ``baseline_commit``. A ``cause="resolved"`` re-drive is human-initiated (the operator ran the - resolve workflow and re-armed the story), so it always auto-recovers and - never pauses, regardless of ``scm.rollback_on_failure``. For the entire - re-drive (``task.resolved_redrive``, latched at resume and cleared once the - correction is committed) the BMAD artifact folders are preserved through - every reset — so a later mid-re-drive retry/defer reset can't silently - revert the correction. Whole folders never participate in the dirtiness - decision; sibling artifact residue remains visible there. + resolve workflow and re-armed the story), so it bypasses the policy pause + and selects auto-recovery regardless of ``scm.rollback_on_failure``. + Unsafe attempt-owned authority can still require manual recovery. For the + entire re-drive (``task.resolved_redrive``, latched at resume and cleared + once the correction is committed) the BMAD artifact folders are preserved + through every reset — so a later mid-re-drive retry/defer reset can't + silently revert the correction. Whole folders never participate in the + dirtiness decision; sibling artifact residue remains visible there. Otherwise (a stopped/abandoned attempt) recovery depends on where the attempt ran. Inside a mounted unit worktree it auto-recovers instead of @@ -1067,10 +1019,14 @@ def rollback_or_pause(self, task: StoryTask, *, cause: str = "stopped") -> None: ) owned_snapshot_restored = True else: - self._normalize_attempt_owned_spec( + self._normalize_attempt_owned_spec_or_pause( + task, spec_path, target_status, confine_root=workspace.paths.project, + unsafe_context=( + "while restoring the attempt-owned lifecycle status" + ), ) normalized_status = target_status @@ -1351,10 +1307,12 @@ def rollback_or_pause(self, task: StoryTask, *, cause: str = "stopped") -> None: unsafe_context="after the baseline reset", ) else: - self._normalize_attempt_owned_spec( + self._normalize_attempt_owned_spec_or_pause( + task, owned_spec[0], target_status, confine_root=workspace.paths.project, + unsafe_context="after the baseline reset", ) try: checkout_dirty = verify.attempt_dirty( diff --git a/src/bmad_loop/sweep.py b/src/bmad_loop/sweep.py index dd48ea848..c4d5f9813 100644 --- a/src/bmad_loop/sweep.py +++ b/src/bmad_loop/sweep.py @@ -3147,8 +3147,9 @@ def _recover_inflight_bundle(self, task: StoryTask) -> bool: if not restart_isolated and task.baseline_commit: # latch resolved_redrive so the corrected spec + restored diff stay # protected through every reset of this re-drive, not just this - # first one; cause="resolved" keeps a human-initiated re-arm - # pause-free regardless of scm.rollback_on_failure + # first one; cause="resolved" keeps a human-initiated re-arm clear of + # the policy pause regardless of scm.rollback_on_failure. Unsafe + # attempt-owned authority may still require manual recovery. task.resolved_redrive = task.resolved_redrive or task.rearmed self._rollback_or_pause(task, cause="resolved" if task.rearmed else "stopped") task.rearmed = False # past rollback (only reached when not paused) diff --git a/tests/test_recovery_flow.py b/tests/test_recovery_flow.py index 59b813edc..e5e31b2a4 100644 --- a/tests/test_recovery_flow.py +++ b/tests/test_recovery_flow.py @@ -101,18 +101,92 @@ def test_owned_spec_restore_recreates_missing_canonical_parents(tmp_path): assert spec.read_bytes() == snapshot -def test_owned_spec_restore_forced_fallback_recreates_missing_canonical_parents( - tmp_path, monkeypatch -): +def test_owned_spec_restore_forced_fallback_refuses_before_path_writer(tmp_path, monkeypatch): monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) - spec = tmp_path.resolve() / "new" / "deep" / "owned.md" + spec = tmp_path.resolve() / "owned.md" + original = b"operator bytes\n" + spec.write_bytes(original) snapshot = b"---\nstatus: ready-for-dev\n---\n\noperator input\n" + path_writer_calls: list[Path] = [] + + def path_writer_is_forbidden(path, *_args, **_kwargs): + path_writer_calls.append(path) + raise AssertionError("generic confined writer was called") - with pytest.raises(_OwnedSpecAuthorityError, match="cannot be verified"): + monkeypatch.setattr(platform_util, "_atomic_write_confined", path_writer_is_forbidden) + + with pytest.raises(_OwnedSpecAuthorityError, match="restoration is unavailable") as excinfo: RecoveryFlow._restore_attempt_owned_spec_bytes(spec, snapshot) - assert spec.read_bytes() == snapshot + assert excinfo.value.safe_restoration_unavailable is True + assert path_writer_calls == [] + assert spec.read_bytes() == original + + +def test_owned_spec_restore_forced_fallback_does_not_create_missing_parents(tmp_path, monkeypatch): + monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) + monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) + first_missing_parent = tmp_path.resolve() / "new" + spec = first_missing_parent / "deep" / "owned.md" + + def path_writer_is_forbidden(*_args, **_kwargs): + raise AssertionError("generic confined writer was called") + + monkeypatch.setattr(platform_util, "_atomic_write_confined", path_writer_is_forbidden) + + with pytest.raises(_OwnedSpecAuthorityError, match="restoration is unavailable"): + RecoveryFlow._restore_attempt_owned_spec_bytes(spec, b"snapshot bytes\n") + + assert not first_missing_parent.exists() + assert list(tmp_path.rglob("*.tmp")) == [] + + +def test_owned_spec_normalization_forced_fallback_refuses_before_writer(tmp_path, monkeypatch): + monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) + monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) + spec = tmp_path.resolve() / "owned.md" + original = b"---\nstatus: in-progress\n---\n\noperator bytes\n" + spec.write_bytes(original) + writer_calls: list[tuple] = [] + monkeypatch.setattr( + verify, + "set_frontmatter_status", + lambda *args, **kwargs: writer_calls.append((args, kwargs)), + ) + + with pytest.raises(_OwnedSpecAuthorityError, match="restoration is unavailable") as excinfo: + RecoveryFlow._normalize_attempt_owned_spec( + spec, + "ready-for-dev", + confine_root=tmp_path, + ) + + assert excinfo.value.safe_restoration_unavailable is True + assert writer_calls == [] + assert spec.read_bytes() == original + + +@pytest.mark.skipif(sys.platform != "win32", reason="native Windows coverage") +def test_owned_spec_restore_native_windows_refuses_before_path_writer(tmp_path, monkeypatch): + assert not recovery_flow.DIR_FD_ANCHORED_WRITES + assert not platform_util.DIR_FD_ANCHORED_WRITES + spec = tmp_path.resolve() / "owned.md" + original = b"operator bytes\n" + spec.write_bytes(original) + path_writer_calls: list[Path] = [] + + def path_writer_is_forbidden(path, *_args, **_kwargs): + path_writer_calls.append(path) + raise AssertionError("generic confined writer was called") + + monkeypatch.setattr(platform_util, "_atomic_write_confined", path_writer_is_forbidden) + + with pytest.raises(_OwnedSpecAuthorityError, match="restoration is unavailable"): + RecoveryFlow._restore_attempt_owned_spec_bytes(spec, b"snapshot bytes\n") + + assert path_writer_calls == [] + assert spec.read_bytes() == original @pytest.mark.parametrize("resolve_fault", NUL_PATH_RESOLVE_FAULTS) @@ -176,6 +250,7 @@ def test_owned_spec_restore_validates_missing_target_spelling_before_write( recovery_flow, "atomic_write_bytes_confined", lambda path, *_args, **_kwargs: writes.append(path), + raising=False, ) monkeypatch.setattr( recovery_flow, @@ -193,6 +268,7 @@ def test_owned_spec_restore_validates_missing_target_spelling_before_write( @pytest.mark.parametrize("failure", NUL_PATH_RESOLVE_FAULTS) +@requires_descriptor_restoration def test_owned_spec_restore_does_not_translate_parent_mkdir_value_error( tmp_path, monkeypatch, failure ): @@ -222,6 +298,7 @@ def fail_mkdir(path, *args, **kwargs): *NUL_PATH_RESOLVE_FAULTS, ], ) +@requires_descriptor_restoration def test_owned_spec_restore_does_not_translate_atomic_repair_write_failure( tmp_path, monkeypatch, failure ): @@ -232,7 +309,6 @@ def test_owned_spec_restore_does_not_translate_atomic_repair_write_failure( def fail_write(*_args, **_kwargs): raise failure - monkeypatch.setattr(recovery_flow, "atomic_write_bytes_confined", fail_write) monkeypatch.setattr(recovery_flow, "atomic_write_bytes_at", fail_write) with pytest.raises(type(failure)) as excinfo: @@ -264,31 +340,6 @@ def fail_readback(fd, size): assert spec.read_bytes() == b"operator bytes\n" -@pytest.mark.parametrize("failure", NUL_PATH_RESOLVE_FAULTS) -def test_owned_spec_restore_fallback_preserves_raw_content_read_value_error( - tmp_path, monkeypatch, failure -): - monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) - monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) - spec = tmp_path.resolve() / "owned.md" - spec.write_bytes(b"operator bytes\n") - real_read_bytes = Path.read_bytes - - def fail_readback(path: Path) -> bytes: - if path == spec: - raise failure - return real_read_bytes(path) - - monkeypatch.setattr(Path, "read_bytes", fail_readback) - - with pytest.raises(type(failure)) as excinfo: - RecoveryFlow._restore_attempt_owned_spec_bytes(spec, b"snapshot bytes\n") - - assert excinfo.value is failure - with spec.open("rb") as fh: - assert fh.read() == b"operator bytes\n" - - @requires_descriptor_restoration def test_owned_spec_restore_preserves_byte_hostile_snapshot(tmp_path): spec = tmp_path.resolve() / "owned.md" @@ -300,19 +351,6 @@ def test_owned_spec_restore_preserves_byte_hostile_snapshot(tmp_path): assert spec.read_bytes() == snapshot -def test_owned_spec_restore_forced_fallback_preserves_byte_hostile_snapshot(tmp_path, monkeypatch): - monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) - monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) - spec = tmp_path.resolve() / "owned.md" - spec.write_bytes(b"old") - snapshot = b"---\r\nstatus: caf\xe9\r\n---\r\n\x00tail" - - with pytest.raises(_OwnedSpecAuthorityError, match="cannot be verified"): - RecoveryFlow._restore_attempt_owned_spec_bytes(spec, snapshot) - - assert spec.read_bytes() == snapshot - - @pytest.mark.skipif( not platform_util.DIR_FD_ANCHORED_WRITES, reason="dir-fd anchoring is POSIX-only" ) @@ -547,72 +585,6 @@ def bounded_read(fd: int, size: int) -> bytes: assert requested[-1:] == [len(snapshot) + 1] -def test_owned_spec_restore_fallback_cannot_accept_postpublication_mismatch(tmp_path, monkeypatch): - monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) - monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) - spec = tmp_path.resolve() / "owned.md" - spec.write_bytes(b"operator bytes") - snapshot = b"snapshot bytes" - real_write = recovery_flow.atomic_write_bytes_confined - - def write_suffix(path, data, **kwargs): - return real_write(path, data + b"x", **kwargs) - - monkeypatch.setattr(recovery_flow, "atomic_write_bytes_confined", write_suffix) - - with pytest.raises(_OwnedSpecAuthorityError, match="cannot be verified") as excinfo: - RecoveryFlow._restore_attempt_owned_spec_bytes(spec, snapshot) - - assert excinfo.value.published_without_verification is True - assert spec.read_bytes() == snapshot + b"x" - - -def test_owned_spec_restore_fallback_does_not_normalize_unverified_publication( - tmp_path, monkeypatch -): - monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) - monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) - spec = tmp_path.resolve() / "owned.md" - snapshot = b"---\nstatus: ready-for-dev\n---\n\noperator input\n" - normalizations: list[str] = [] - - monkeypatch.setattr( - RecoveryFlow, - "_normalize_attempt_owned_spec", - staticmethod(lambda *_args, **_kwargs: normalizations.append("normalized")), - ) - - with pytest.raises(_OwnedSpecAuthorityError, match="cannot be verified"): - RecoveryFlow._restore_attempt_owned_spec( - spec, - snapshot, - "ready-for-dev", - confine_root=tmp_path, - ) - - assert normalizations == [] - assert spec.read_bytes() == snapshot - - -def test_owned_spec_restore_translates_only_confined_writer_refusal(tmp_path, monkeypatch): - monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) - spec = tmp_path.resolve() / "owned.md" - original = b"operator bytes" - spec.write_bytes(original) - refusal = UnconfinedWriteError("refused filesystem walk") - - def refuse(*_args, **_kwargs): - raise refusal - - monkeypatch.setattr(recovery_flow, "atomic_write_bytes_confined", refuse) - - with pytest.raises(_OwnedSpecAuthorityError) as excinfo: - RecoveryFlow._restore_attempt_owned_spec_bytes(spec, b"snapshot bytes") - - assert excinfo.value.__cause__ is refusal - assert spec.read_bytes() == original - - @pytest.mark.skipif( not platform_util.DIR_FD_ANCHORED_WRITES, reason="dir-fd anchoring is POSIX-only" ) @@ -671,146 +643,6 @@ def fail_after_publish(fd: int, size: int) -> bytes: assert spec.read_bytes() == snapshot -def test_owned_spec_restore_preserves_raw_fallback_read_failure(tmp_path, monkeypatch): - monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) - monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) - spec = tmp_path.resolve() / "owned.md" - spec.write_bytes(b"operator bytes") - failure = OSError("ordinary read failed") - real_read_bytes = Path.read_bytes - - def fail_read(path: Path) -> bytes: - if path == spec: - raise failure - return real_read_bytes(path) - - monkeypatch.setattr(Path, "read_bytes", fail_read) - - with pytest.raises(OSError) as excinfo: - RecoveryFlow._restore_attempt_owned_spec_bytes(spec, b"snapshot bytes") - - assert excinfo.value is failure - with spec.open("rb") as fh: - assert fh.read() == b"operator bytes" - - -def _assert_fallback_replacement_is_not_observed( - tmp_path: Path, - monkeypatch: pytest.MonkeyPatch, - *, - force_fallback: bool, -) -> None: - if force_fallback: - monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) - monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) - else: - assert not recovery_flow.DIR_FD_ANCHORED_WRITES - assert not platform_util.DIR_FD_ANCHORED_WRITES - spec = tmp_path.resolve() / "owned.md" - spec.write_bytes(b"operator bytes") - snapshot = b"snapshot bytes" - replacement = b"concurrent regular-file replacement" - real_read_bytes = Path.read_bytes - real_path_open = Path.open - real_path_resolve = Path.resolve - real_lstat = os.lstat - real_os_open = os.open - real_os_stat = os.stat - real_write = recovery_flow.atomic_write_bytes_confined - published = False - target_observations: list[str] = [] - - def substitute_before_callback(path, data, **kwargs): - verify_after = kwargs["_after_replace"] - - def substitute_then_verify(published_fd: int | None) -> None: - nonlocal published - spec.unlink() - spec.write_bytes(replacement) - published = True - verify_after(published_fd) - - kwargs["_after_replace"] = substitute_then_verify - return real_write(path, data, **kwargs) - - def reject_read_after_publish(path: Path) -> bytes: - if path == spec and published: - target_observations.append("read") - raise AssertionError("published target was reopened") - return real_read_bytes(path) - - def reject_lstat_after_publish(path, *args, **kwargs): - if Path(path) == spec and published: - target_observations.append("lstat") - raise AssertionError("published target was observed") - return real_lstat(path, *args, **kwargs) - - def reject_path_open_after_publish(path: Path, *args, **kwargs): - if path == spec and published: - target_observations.append("Path.open") - raise AssertionError("published target was opened") - return real_path_open(path, *args, **kwargs) - - def reject_os_open_after_publish(path, *args, **kwargs): - if not isinstance(path, int) and Path(path) == spec and published: - target_observations.append("os.open") - raise AssertionError("published target was opened") - return real_os_open(path, *args, **kwargs) - - def reject_os_stat_after_publish(path, *args, **kwargs): - if not isinstance(path, int) and Path(path) == spec and published: - target_observations.append("os.stat") - raise AssertionError("published target was observed") - return real_os_stat(path, *args, **kwargs) - - def reject_resolve_after_publish(path: Path, *args, **kwargs): - if path == spec and published: - target_observations.append("Path.resolve") - raise AssertionError("published target was resolved") - return real_path_resolve(path, *args, **kwargs) - - monkeypatch.setattr( - recovery_flow, - "atomic_write_bytes_confined", - substitute_before_callback, - ) - monkeypatch.setattr(Path, "read_bytes", reject_read_after_publish) - monkeypatch.setattr(Path, "open", reject_path_open_after_publish) - monkeypatch.setattr(Path, "resolve", reject_resolve_after_publish) - monkeypatch.setattr(recovery_flow.os, "lstat", reject_lstat_after_publish) - monkeypatch.setattr(recovery_flow.os, "open", reject_os_open_after_publish) - monkeypatch.setattr(recovery_flow.os, "stat", reject_os_stat_after_publish) - - with pytest.raises(_OwnedSpecAuthorityError, match="cannot be verified") as excinfo: - RecoveryFlow._restore_attempt_owned_spec_bytes(spec, snapshot) - - assert excinfo.value.published_without_verification is True - assert target_observations == [] - with real_path_open(spec, "rb") as fh: - assert fh.read() == replacement - - -def test_owned_spec_restore_forced_fallback_never_observes_replaced_target_after_publication( - tmp_path, monkeypatch -): - _assert_fallback_replacement_is_not_observed( - tmp_path, - monkeypatch, - force_fallback=True, - ) - - -@pytest.mark.skipif(sys.platform != "win32", reason="native Windows coverage") -def test_owned_spec_restore_native_windows_never_observes_replaced_target_after_publication( - tmp_path, monkeypatch -): - _assert_fallback_replacement_is_not_observed( - tmp_path, - monkeypatch, - force_fallback=False, - ) - - @pytest.mark.skipif( not platform_util.DIR_FD_ANCHORED_WRITES, reason="dir-fd anchoring is POSIX-only" ) @@ -836,29 +668,6 @@ def record_root(root, target, **kwargs): assert spec.read_bytes() == snapshot -def test_owned_spec_restore_fallback_supports_external_target(tmp_path, monkeypatch): - monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) - monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) - external = tmp_path.resolve() / "trusted-external-artifacts" - external.mkdir() - spec = external / "owned.md" - snapshot = b"snapshot bytes" - roots: list[Path] = [] - real_write = recovery_flow.atomic_write_bytes_confined - - def record_root(path, data, **kwargs): - roots.append(kwargs["confine_root"]) - return real_write(path, data, **kwargs) - - monkeypatch.setattr(recovery_flow, "atomic_write_bytes_confined", record_root) - - with pytest.raises(_OwnedSpecAuthorityError, match="cannot be verified"): - RecoveryFlow._restore_attempt_owned_spec_bytes(spec, snapshot) - - assert roots == [Path(spec.anchor)] - assert spec.read_bytes() == snapshot - - @pytest.mark.skipif( not platform_util.DIR_FD_ANCHORED_WRITES, reason="dir-fd anchoring is POSIX-only" ) @@ -880,26 +689,6 @@ def refuse(_dir_fd, _name): assert spec.read_bytes() == original -def test_owned_spec_restore_preserves_fallback_writable_target_refusal(tmp_path, monkeypatch): - monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) - monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) - spec = tmp_path.resolve() / "owned.md" - original = b"operator bytes" - spec.write_bytes(original) - failure = PermissionError("target is read-only") - - def refuse(_target, *, follow_symlinks): - raise failure - - monkeypatch.setattr(platform_util, "_refuse_unwritable_target", refuse) - - with pytest.raises(PermissionError) as excinfo: - RecoveryFlow._restore_attempt_owned_spec_bytes(spec, b"snapshot bytes") - - assert excinfo.value is failure - assert spec.read_bytes() == original - - @pytest.mark.skipif( not platform_util.DIR_FD_ANCHORED_WRITES, reason="dir-fd anchoring is POSIX-only" ) @@ -938,41 +727,6 @@ def unexpected_stage(*_args, **_kwargs): assert spec.read_bytes() == appeared -def test_owned_spec_restore_fallback_refuses_missing_target_appearing_before_staging( - tmp_path, monkeypatch -): - monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) - monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) - parent = tmp_path.resolve() / "artifacts" - parent.mkdir() - spec = parent / "owned.md" - appeared = b"new owner bytes" - real_write = recovery_flow.atomic_write_bytes_confined - later_calls: list[str] = [] - - def appear_before_writer(path, data, **kwargs): - spec.write_bytes(appeared) - return real_write(path, data, **kwargs) - - def unexpected_probe(*_args, **_kwargs): - later_calls.append("writable-probe") - raise AssertionError("writable probe ran after authority loss") - - def unexpected_stage(*_args, **_kwargs): - later_calls.append("temp") - raise AssertionError("temp creation ran after authority loss") - - monkeypatch.setattr(recovery_flow, "atomic_write_bytes_confined", appear_before_writer) - monkeypatch.setattr(platform_util, "_refuse_unwritable_target", unexpected_probe) - monkeypatch.setattr(platform_util, "_mkstemp_beside", unexpected_stage) - - with pytest.raises(_OwnedSpecAuthorityError, match="became unsafe"): - RecoveryFlow._restore_attempt_owned_spec_bytes(spec, b"snapshot") - - assert later_calls == [] - assert spec.read_bytes() == appeared - - def _replace_target(spec: Path, data: bytes) -> None: """Swap the entry at `spec` for a NEW file holding `data`, distinguishable from the original by the identity the restore guard compares (`samestat`). @@ -1238,168 +992,6 @@ def fail_read(_fd: int, _size: int) -> bytes: assert list(parent.glob("*.tmp")) == [] -def test_owned_spec_restore_fallback_refuses_target_replacement_after_staging( - tmp_path, monkeypatch -): - monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) - monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) - parent = tmp_path.resolve() / "artifacts" - parent.mkdir() - spec = parent / "owned.md" - spec.write_bytes(b"operator bytes") - real_write = recovery_flow.atomic_write_bytes_confined - - def replace_before_publish(path, data, **kwargs): - validate = kwargs["_before_replace"] - - def replace_then_validate() -> None: - _replace_target(spec, b"replacement") - validate() - - kwargs["_before_replace"] = replace_then_validate - return real_write(path, data, **kwargs) - - monkeypatch.setattr(recovery_flow, "atomic_write_bytes_confined", replace_before_publish) - - with pytest.raises(_OwnedSpecAuthorityError, match="became unsafe"): - RecoveryFlow._restore_attempt_owned_spec_bytes(spec, b"snapshot") - - assert spec.read_bytes() == b"replacement" - assert list(parent.glob("*.tmp")) == [] - - -def test_owned_spec_restore_fallback_refuses_in_place_target_edit_after_staging( - tmp_path, monkeypatch -): - monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) - monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) - parent = tmp_path.resolve() / "artifacts" - parent.mkdir() - spec = parent / "owned.md" - spec.write_bytes(b"operator bytes") - original_inode = spec.stat().st_ino - competing = b"competing data" - real_write = recovery_flow.atomic_write_bytes_confined - monkeypatch.setattr(recovery_flow, "_target_stat_version", lambda _observed: (0, 0, 0)) - - def edit_before_publish(path, data, **kwargs): - validate = kwargs["_before_replace"] - - def edit_then_validate() -> None: - with spec.open("r+b") as fh: - fh.write(competing) - fh.truncate() - assert spec.stat().st_ino == original_inode - validate() - - kwargs["_before_replace"] = edit_then_validate - return real_write(path, data, **kwargs) - - monkeypatch.setattr(recovery_flow, "atomic_write_bytes_confined", edit_before_publish) - - with pytest.raises(_OwnedSpecAuthorityError, match="became unsafe"): - RecoveryFlow._restore_attempt_owned_spec_bytes(spec, b"snapshot") - - assert spec.read_bytes() == competing - assert list(parent.glob("*.tmp")) == [] - - -def test_owned_spec_restore_fallback_refuses_in_place_edit_during_initial_content_read( - tmp_path, monkeypatch -): - monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) - monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) - parent = tmp_path.resolve() / "artifacts" - parent.mkdir() - spec = parent / "owned.md" - original = b"operator bytes" - competing = original + b"x" - spec.write_bytes(original) - real_read_bytes = Path.read_bytes - mutated = False - - def mutate_then_read(path: Path) -> bytes: - nonlocal mutated - if path == spec and not mutated: - mutated = True - with spec.open("ab") as fh: - fh.write(b"x") - return real_read_bytes(path) - - monkeypatch.setattr(Path, "read_bytes", mutate_then_read) - - with pytest.raises(_OwnedSpecAuthorityError, match="became unsafe"): - RecoveryFlow._restore_attempt_owned_spec_bytes(spec, b"snapshot") - - assert mutated is True - assert real_read_bytes(spec) == competing - assert list(parent.glob("*.tmp")) == [] - - -def test_owned_spec_restore_fallback_refuses_short_initial_content_sample(tmp_path, monkeypatch): - monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) - monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) - parent = tmp_path.resolve() / "artifacts" - parent.mkdir() - spec = parent / "owned.md" - original = b"operator bytes" - spec.write_bytes(original) - real_read_bytes = Path.read_bytes - - def short_read_bytes(path: Path) -> bytes: - if path == spec: - return b"operator" - return real_read_bytes(path) - - monkeypatch.setattr(Path, "read_bytes", short_read_bytes) - - with pytest.raises(_OwnedSpecAuthorityError, match="became unsafe"): - RecoveryFlow._restore_attempt_owned_spec_bytes(spec, b"snapshot") - - assert real_read_bytes(spec) == original - assert list(parent.glob("*.tmp")) == [] - - -def test_owned_spec_restore_fallback_preserves_raw_content_comparison_failure( - tmp_path, monkeypatch -): - monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) - monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) - parent = tmp_path.resolve() / "artifacts" - parent.mkdir() - spec = parent / "owned.md" - original = b"operator bytes" - spec.write_bytes(original) - failure = OSError("ordinary comparison read failed") - real_write = recovery_flow.atomic_write_bytes_confined - real_read_bytes = Path.read_bytes - - def fail_before_publish(path, data, **kwargs): - validate = kwargs["_before_replace"] - - def fail_then_validate() -> None: - def fail_read_bytes(read_path: Path) -> bytes: - if read_path == spec: - raise failure - return real_read_bytes(read_path) - - monkeypatch.setattr(Path, "read_bytes", fail_read_bytes) - validate() - - kwargs["_before_replace"] = fail_then_validate - return real_write(path, data, **kwargs) - - monkeypatch.setattr(recovery_flow, "atomic_write_bytes_confined", fail_before_publish) - - with pytest.raises(OSError) as excinfo: - RecoveryFlow._restore_attempt_owned_spec_bytes(spec, b"snapshot") - - assert excinfo.value is failure - with spec.open("rb") as fh: - assert fh.read() == original - assert list(parent.glob("*.tmp")) == [] - - @pytest.mark.skipif( not platform_util.DIR_FD_ANCHORED_WRITES, reason="dir-fd anchoring is POSIX-only" ) @@ -1514,45 +1106,6 @@ def mutate_then_read(fd: int, size: int) -> bytes: assert spec.read_bytes() == snapshot + b"x" -def test_owned_spec_restore_fallback_does_not_reopen_published_target(tmp_path, monkeypatch): - monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) - monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) - spec = tmp_path.resolve() / "owned.md" - spec.write_bytes(b"operator bytes") - snapshot = b"snapshot bytes" - real_read_bytes = Path.read_bytes - postpublication_reads = 0 - published = False - - real_write = recovery_flow.atomic_write_bytes_confined - - def mark_published(path, data, **kwargs): - verify_after = kwargs["_after_replace"] - - def mark_then_verify(published_fd: int | None) -> None: - nonlocal published - published = True - verify_after(published_fd) - - kwargs["_after_replace"] = mark_then_verify - return real_write(path, data, **kwargs) - - def record_read(path: Path) -> bytes: - nonlocal postpublication_reads - if path == spec and published: - postpublication_reads += 1 - return real_read_bytes(path) - - monkeypatch.setattr(recovery_flow, "atomic_write_bytes_confined", mark_published) - monkeypatch.setattr(Path, "read_bytes", record_read) - - with pytest.raises(_OwnedSpecAuthorityError, match="cannot be verified"): - RecoveryFlow._restore_attempt_owned_spec_bytes(spec, snapshot) - - assert postpublication_reads == 0 - assert real_read_bytes(spec) == snapshot - - @pytest.mark.skipif( not platform_util.DIR_FD_ANCHORED_WRITES, reason="dir-fd anchoring is POSIX-only" ) @@ -1734,8 +1287,8 @@ def _assert_owned_spec_manual_adoption_pause( "story_key": task.story_key, "spec": str(spec.resolve()), "problem": ( - f"restoration bytes were published {stage}, but this platform cannot verify " - "the resulting file without descriptor-relative writes" + f"safe automatic restoration is unavailable {stage} because " + "this platform lacks descriptor-relative writes" f"{status_guidance}; manual adoption is required" ), } @@ -1896,6 +1449,7 @@ def boom(*a, **k): assert "rollback-skipped-clean" not in flow.journal.events() +@requires_descriptor_restoration def test_bound_lifecycle_only_spec_is_normalized_and_reads_git_clean(project): """T8: the one-file attempt binding recognizes only its own lifecycle delta. @@ -1953,6 +1507,7 @@ def normalization_is_forbidden(*_args, **_kwargs): ("baseline_status", "attempt_status"), [("draft", "in-progress"), ("in-progress", "in-review"), ("in-review", "done")], ) +@requires_descriptor_restoration def test_plain_bound_lifecycle_change_restores_baseline_status( project, baseline_status, attempt_status ): @@ -1979,6 +1534,7 @@ def test_plain_bound_lifecycle_change_restores_baseline_status( assert flow.calls.pauses == [] +@requires_descriptor_restoration def test_plain_bound_lifecycle_commit_is_parked_and_reset_before_retry(project): """A baseline-shaped checkout is not clean while attempt commits remain. @@ -2054,7 +1610,7 @@ def test_plain_owned_spec_forced_fallback_pauses_while_undoing_lifecycle_repair( monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) - with pytest.raises(_Pause, match="undoing a tentative lifecycle repair"): + with pytest.raises(_Pause, match="attempt-owned lifecycle status"): flow.rollback_or_pause(task) assert spec.read_bytes() == child @@ -2064,7 +1620,8 @@ def test_plain_owned_spec_forced_fallback_pauses_while_undoing_lifecycle_repair( flow, task, spec, - stage="while undoing a tentative lifecycle repair", + stage="while restoring the attempt-owned lifecycle status", + expected_status="ready-for-dev", ) @@ -2195,7 +1752,7 @@ def test_plain_tracked_snapshot_restores_operator_bytes_child_reverted_to_baseli assert flow.calls.pauses == [] -def test_plain_tracked_snapshot_forced_fallback_pauses_after_publication(project, monkeypatch): +def test_plain_tracked_snapshot_forced_fallback_pauses_before_write(project, monkeypatch): repo = project.project spec = _tracked_spec(project) baseline = spec.read_bytes() @@ -2214,14 +1771,15 @@ def test_plain_tracked_snapshot_forced_fallback_pauses_after_publication(project with pytest.raises(_Pause, match="manual adoption is required"): flow.rollback_or_pause(task) - assert spec.read_bytes() == operator + assert spec.read_bytes() == baseline assert "rollback-owned-spec-restored" not in flow.journal.events() assert "rollback-skipped-clean" not in flow.journal.events() _assert_owned_spec_manual_adoption_pause( flow, task, spec, - stage="while restoring the pre-launch operator input", + stage="while restoring the attempt-owned lifecycle status", + expected_status="ready-for-dev", ) @@ -2280,7 +1838,7 @@ def test_plain_forced_fallback_pauses_after_completed_baseline_reset(project, mo flow.rollback_or_pause(task) assert source.read_text() == "original\n" - assert spec.read_bytes() == operator + assert spec.read_bytes() == baseline assert "rollback-auto" in flow.journal.events() assert flow.calls.emits == ["pre_rollback"] _assert_owned_spec_manual_adoption_pause( @@ -2354,7 +1912,7 @@ def test_plain_sibling_residue_forced_fallback_preempts_generic_manual_pause(pro with pytest.raises(_Pause, match="before the ordinary manual-recovery pause"): flow.rollback_or_pause(task) - assert spec.read_bytes() == operator + assert spec.read_bytes() == baseline assert source.read_text() == "failed child sibling\n" assert "rollback-owned-spec-restored" not in flow.journal.events() assert "rollback-manual-required" not in flow.journal.events() @@ -2498,7 +2056,7 @@ def test_latched_redrive_forced_fallback_pauses_after_preservation(project, monk with pytest.raises(_Pause, match="manual adoption is required"): flow.rollback_or_pause(task) - assert spec.read_bytes() == corrected + assert spec.read_bytes() == child assert task.preserve_ref is not None assert git(repo, "show", f"{task.preserve_ref}:{rel}").encode() == child.rstrip(b"\n") assert "attempt-worktree-preserved" in flow.journal.events() @@ -2653,7 +2211,7 @@ def test_plain_git_invisible_snapshot_forced_fallback_pauses_before_reset( with pytest.raises(_Pause, match="manual adoption is required"): flow.rollback_or_pause(task) - assert spec.read_bytes() == original + assert spec.read_bytes() == child assert verify.index_path_changed_since(repo, task.baseline_commit, rel) assert task.preserve_ref is not None assert git(repo, "show", f"{task.preserve_ref}:{rel}").encode() == child.rstrip(b"\n") @@ -2997,6 +2555,38 @@ def retarget_after_reset(reset_task, *, preserve=()): assert (victim_parent / "owned.md").read_bytes() == decoy # nothing escaped +def test_resolved_cause_forced_fallback_pauses_after_completed_reset(project, monkeypatch): + repo = project.project + spec = _tracked_spec(project) + remaining = b"---\nstatus: done\n---\n\nhuman corrected intent\n" + spec.write_bytes(remaining) + task = _task(repo) + task.dispatched_spec_file = str(spec) + task.dispatched_spec_snapshot = b"stale bytes from the abandoned attempt" + source = repo / "src.txt" + source.write_text("failed attempt residue\n") + flow = _make_flow( + workspace=Workspace.default(project), policy=_policy(rollback_on_failure=False) + ) + monkeypatch.setattr(recovery_flow, "DIR_FD_ANCHORED_WRITES", False) + monkeypatch.setattr(platform_util, "DIR_FD_ANCHORED_WRITES", False) + + with pytest.raises(_Pause, match="after the baseline reset"): + flow.rollback_or_pause(task, cause="resolved") + + assert source.read_text() == "original\n" + assert spec.read_bytes() == remaining + assert "rollback-auto" in flow.journal.events() + assert flow.calls.emits == ["pre_rollback"] + _assert_owned_spec_manual_adoption_pause( + flow, + task, + spec, + stage="after the baseline reset", + expected_status="ready-for-dev", + ) + + @pytest.mark.parametrize("git_invisible", ["baseline-untracked", "ignored"]) @requires_descriptor_restoration def test_plain_attempt_restores_and_parks_git_invisible_owned_spec(project, git_invisible): @@ -3400,6 +2990,7 @@ def test_latched_redrive_preserves_uncommitted_child_bytes_above_child_commit(pr assert "failed child committed body A" not in preserved +@requires_descriptor_restoration def test_post_normalization_probe_fault_cannot_authorize_owned_dirty(project, monkeypatch): """A failed pre-reset re-probe cannot bypass the resolved reset. @@ -3491,7 +3082,7 @@ def test_patch_restore_redrive_forced_fallback_requires_in_review_adoption(proje with pytest.raises(_Pause, match="lifecycle status 'in-review'"): flow.rollback_or_pause(task) - assert spec.read_bytes() == restored + assert spec.read_text() == "---\nstatus: in-progress\n---\n\nrestored human correction\n" assert "rollback-owned-spec-normalized" not in flow.journal.events() assert "post_rollback" not in flow.calls.emits _assert_owned_spec_manual_adoption_pause( @@ -3503,6 +3094,7 @@ def test_patch_restore_redrive_forced_fallback_requires_in_review_adoption(proje ) +@requires_descriptor_restoration def test_owned_spec_without_visible_status_fails_the_post_write_oracle(project): """T14/False: a writer no-op is not repair success without the target oracle. @@ -3527,6 +3119,7 @@ def test_owned_spec_without_visible_status_fails_the_post_write_oracle(project): assert flow.calls.pauses == [] +@requires_descriptor_restoration def test_owned_spec_with_unsafe_status_shape_propagates_writer_error(project): """T14/write: repair-write refusal is never caught as failed observation. From f74eda6a79512d191dff409f548f7ebf7bc5919c Mon Sep 17 00:00:00 2001 From: t Date: Mon, 14 Sep 2026 13:16:19 -0700 Subject: [PATCH 04/17] fix(sweep): bind migration ledger publication bytes --- docs/FEATURES.md | 2 +- src/bmad_loop/engine.py | 14 ++ src/bmad_loop/sweep.py | 110 ++++++++++- src/bmad_loop/verify.py | 267 ++++++++++++++++++++++++++ tests/test_sweep.py | 307 +++++++++++++++++++++++++++++ tests/test_verify.py | 416 ++++++++++++++++++++++++++++++++++++++++ 6 files changed, 1106 insertions(+), 10 deletions(-) diff --git a/docs/FEATURES.md b/docs/FEATURES.md index f590566d5..2a23de1a8 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -252,7 +252,7 @@ Result` section. Every other spec keeps warn-and-continue, and the record says w ### Deferred-work sweeps - Skills accumulate an append-only ledger (`deferred-work.md`, `DW-` entries): split-off goals, pre-existing findings, "needs human decision" items. -- Legacy-ledger migration recovery (DW-296/DW-297) persists a current-format marker plus run-owned `migrate-baseline.md` and `migrate-rewrite.md` text snapshots. The baseline and its exact reconstructed manifest are durable before dispatch; the rewrite becomes authoritative only after deterministic validation. A result-publication fault therefore leaves `sweep-migrate` nonterminal, and cycle-one resume compare-and-set restores the accepted legacy text before redispatching without overwriting concurrent ledger bytes or resetting past an advanced HEAD. Once `migrate-result.json` is durable the task enters `committing`: resume validates the baseline/manifest/rewrite/result set and retries only the exact-path publication tail, treating `DONE` as earned only by a commit or by a clean outcome that still contains the accepted rewrite. Missing, nonregular, unreadable, malformed, or mutually inconsistent current-format records required by the task's persisted recovery phase escalate without reset or publication; a missing result during `triage-verify` instead triggers the intentional restore-and-redispatch path, while unmarked pre-upgrade tasks retain the older reset-and-reread route. +- Legacy-ledger migration recovery (DW-296/DW-297) persists a current-format marker plus run-owned `migrate-baseline.md` and `migrate-rewrite.md` text snapshots. The baseline and its exact reconstructed manifest are durable before dispatch; migration rechecks the live cycle input before and after publishing those records and once more after executable pre-session hooks at the actual adapter-launch boundary (DW-316). Drift or a read fault first persists `PENDING` with no baseline authority and refunds an attempt that launched no adapter, then retires the stale records, so even a cleanup fault cannot resurrect stale dispatch authority. The rewrite becomes authoritative only after deterministic validation. A result-publication fault therefore leaves `sweep-migrate` nonterminal, and cycle-one resume compare-and-set restores the accepted legacy text before redispatching without overwriting concurrent ledger bytes or resetting past an advanced HEAD. Once `migrate-result.json` is durable the task enters `committing`: resume validates the baseline/manifest/rewrite/result set and retries only the exact-path publication tail. For migration, that tail stages in an isolated hook-observed candidate, validates its exact parent, one-path scope, Git-normalized accepted blob, decoded live text, and regular resolved target before publishing through an expected-old `HEAD` CAS (DW-311). Post-CAS target-index synchronization is target-local replayable housekeeping; resume can recognize the validated ledger transition beneath unrelated first-parent descendants without moving them. `DONE` is earned only by that accepted commit or by a clean outcome that still contains the accepted rewrite. Missing, nonregular, unreadable, malformed, or mutually inconsistent current-format records required by the task's persisted recovery phase escalate without reset or publication; a missing result during `triage-verify` instead triggers the intentional restore-and-redispatch path, while unmarked pre-upgrade tasks retain the older reset-and-reread route. - Ledger read contract (DW-146/DW-279): `read_for_write` returns `None` for the existing metadata-absence cases and nonregular targets. Nonabsence metadata faults and all text-read `OSError` failures, including disappearance/type-change races after a successful probe, raise `LedgerReadFault(LedgerReadError)` with the original exception chained as `__cause__`. Invalid UTF-8 still raises `LedgerReadError` with a `UnicodeDecodeError` cause. A refused authoritative read publishes nothing. Pre-lock presence probes, lock acquisition and writes retain raw `OSError`; observation reads retain their empty-text-plus-attributed-fault degradation. Consumers distinguish OS refusal from decoding before handling the parent exception. - Frontmatter harvest bridge (BMAD-METHOD#2640/#2651; shipped 0.9.1, hardened #433): since BMAD-METHOD 6.10.1-next.33 the unattended primitive records defer-triaged review findings in its spec's frontmatter `deferred:` list (summary/evidence, optional location/severity) and writes nothing to the ledger. The orchestrator harvests them itself — post-session but _above_ the artifact gate, so before verification and before the attempt is accepted — into canonical `### DW-` entries, so `deferred-work.md` stays the sweep's sole read surface. Entries therefore appear even when the attempt goes on to fail verification: a fixable retry deliberately keeps them (the attribution reference moves onto the kept tree), and `_harvest_gate_exclude` stops the engine's own append from counting as the session's proof of work. Dedupe key is the fingerprinted `origin: spec-deferred ` plus `source_spec:`, scanned across entries of _every_ status, so a replay neither doubles an entry nor re-opens a closed one. Era-agnostic (the gate is the field's presence, never the skill name resolved on disk) and bounded to sessions bmad-loop drove to a success status — `in-review` with the follow-up review enabled, else `done`, plus an operator park; a plan-halt checkpoint keeps its notes for the implementation pass. A spec outside the orchestrator-owned roots is refused (`spec-deferrals-skipped-out-of-tree`) and an unreadable one retries the session rather than accepting it with findings silently dropped; `deferred:` items that will not parse are journaled (`spec-deferrals-malformed`) and filed as one low-severity entry naming the spec. A ledger whose bytes do not decode is routed at every one of the engine's own four `read_for_write` sites (DW-231) according to what the read was about to do: the observation reads — the proof-of-work digest, the pre-harvest snapshot, the defer snapshot and the two restores' compare-and-set probes — degrade to a typed answer nothing can write back or anchor a write on (the digest hashes the raw bytes, so "did the ledger change" stays exact; the snapshots stay unarmed; a restore skips and journals) and journal `ledger-read-degraded` naming the site; the two reads that precede a publish — this harvest's append and the isolated unit's carry into the main ledger — normally journal `ledger-read-refused`, raise an `ACTION REQUIRED` notice naming the ledger, and pause the run at `escalation`; a sweep's terminal post-merge harvest carry instead journals `sweep-bundle-close-refused` and pauses at `story-gate`, while its direct pre-terminal defer carry retains the engine route. Both routes leave the task's phase untouched, so `bmad-loop resume` after the hand repair retries the write — resume recovery replays the recorded session result where one exists (the dev and review legs) and otherwise re-drives the leg (the unlatched review-timeout salvage and fix legs) — rather than, as `_escalate` would, demanding a `bmad-loop resolve` session and a clean rebuild over a fault that is not the story's. Bare, the first of those reads ended a story run as `run-crash` with the completed session's work on disk. The route also covers the window INSIDE each write, for decode faults (DW-259) and OS metadata/text-read faults (DW-279): every `deferredwork` mutator takes its own locked `read_for_write` — after the routed pre-read at the harvest and the harvest carry, after an observation snapshot at the commit-boundary close, and with no pre-read at all at the review-timeout salvage refile (`deferredwork.append_entry`) and the isolated close carry — so a `LedgerReadError` raised from the mutator call itself — the harvest's seen-again mark and append, the commit-boundary `closes_deferred:` close, the salvage refile, the isolated unit's harvest carry and close carry — pauses through its owning repair route under a site name ending in `-locked` (`spec-deferrals-harvest-mark-locked`, `spec-deferrals-harvest-append-locked`, `story-close-locked`, `review-timeout-salvage-refile-locked`, `harvest-carry-append-locked`, `story-close-carry-locked`); the locked read fires ahead of every write, so a pause there proves the mutator wrote nothing, which is what lets the commit-boundary close disarm its rollback first rather than journal a `deferred-close-rollback-failed` against bytes it cannot read. The notice names both kinds of write (findings to file, a declared close to record); `bmad-loop resume` re-drives the close through the COMMITTING arm, and a pending salvage refile through its persisted retry latch (DW-278): resume verifies the preserved product again, refiles the outstanding follow-up, and commits without new dev/review sessions or additional attempt/cycle charges under either rollback policy; ordinary commit gates still run, including any configured `pre_commit_gate` workflow sessions. An unrepaired ledger pauses again with the latch retained; a failed salvage verification follows the usual retry/exhaust routing. Successful refile records the publication while retaining recovery authority through notification and commit gates — the latch is set at every salvage's handoff save, the first fault-free one included, not only after a repair pause, so a host lost between that save and the commit replays the salvage rather than restarting it; the durable COMMITTING transition clears the latch. Legacy and unlatched timeouts keep the baseline restart or manual recovery behavior governed by `scm.rollback_on_failure`. The replay is the story engine's: a sweep bundle's recovery (`_recover_inflight_bundle`) has no session-replay arm at all, so a latched bundle restarts as every other post-session bundle does and its restart clears the latch, so the abandoned product's salvage cannot force a review on the replacement attempt. `LedgerReadFault(LedgerReadError)` wraps OS metadata/text-read failures with the original `OSError` as `__cause__`, so the locked-read catches cover them without catching lock/write failures. Pre-lock presence probes, lock acquisition and writes keep their raw `OSError` behavior. A read the OS refuses (EACCES, EIO, a symlink cycle) is routed the same way at the same four sites (DW-258): the observation reads degrade to a typed answer that carries NO digest — nothing is read from a ledger the OS refused — so the proof-of-work digest becomes an `` sentinel and the attribution compare treats a sentinel on either side as UNKNOWN, which is never credited: the ledger path stays excluded from proof of work, so the engine's own harvest append after the hand repair cannot pass a session that wrote nothing (a session whose only work was a ledger edit over a refused baseline is rolled back and retried over a readable one); the publish reads pause with the same `ACTION REQUIRED` notice, whose repair sentence now names both repairs (valid UTF-8; the path's permissions or storage). Where no publish read is reached — a spec with no findings — the story completes over the refused ledger, and a declared `closes_deferred:` then journals `deferred-close-ledger-unavailable` and sends a best-effort notice through the configured notification channels (including ATTENTION when file notifications are enabled), naming the story, every unapplied declared ID, and the fault (DW-277). The snapshot outage writes nothing to the ledger and does not pause or crash the story. Pre-rename primitives and the attended `bmad-build` still append flat `- source_spec:` blocks directly, which `sweep --migrate` normalizes. - Story-declared closure (`closes_deferred: [DW-5, DW-6]`, human-authored on a `stories.yaml` entry or in a story spec's frontmatter — the two are unioned): when the story commits, each declared entry flips to `status: done ` + `resolution: resolved by story ` — the annotation a sweep bundle writes — so the ledger stops being one-way. Written at the commit boundary, so an in-repo ledger carries the annotation in the story's own commit and a story that fails, is rejected by review, or escalates closes nothing. Declared, never inferred from a diff; re-read at the commit so a declaration edited after implementation still counts; idempotent across a resume; an unknown id, an unreadable entry status or a non-list declaration in a story spec is journaled, never fatal, and `bmad-loop validate` warns about all of them before the run starts. (A non-list `closes_deferred` in `stories.yaml` is different: the manifest is a schema the parser owns, so it is refused outright, before the run.) An artifact dir outside the repo cannot be committed — the annotation is written anyway and journaled (`deferred-close-external-ledger`). If the advisory ledger snapshot cannot be read, the declared closes remain unapplied and open entries stay open; the outage is journaled and notified as above. Restore ledger readability (valid UTF-8 and accessible permissions or storage), then run `bmad-loop sweep`: the next sweep re-triages those IDs against the actual code and can close verified resolutions as `already_resolved`, with the completed story's commit serving as evidence. diff --git a/src/bmad_loop/engine.py b/src/bmad_loop/engine.py index e7380f17c..a93612cb3 100644 --- a/src/bmad_loop/engine.py +++ b/src/bmad_loop/engine.py @@ -6510,6 +6510,7 @@ def _run_session( label: str | None = None, spec_snapshot: SpecSnapshot | None = None, preserve_dispatched_spec_snapshot: bool = False, + prelaunch_validator: Callable[[], None] | None = None, ) -> SessionResult: # ``label`` names a non-standard session (a plugin-provided workflow) so # its task_id stays distinct from the role's own dev/review attempts. @@ -6575,6 +6576,12 @@ def _run_session( if sctx is not None: veto = sctx.resolved_veto() if veto is not None: + # A veto prevents adapter launch but does not undo executable + # hook side effects. Callers whose durable launch authority is + # bound to mutable input must validate that input before the + # early return just as they do on the normal launch path. + if prelaunch_validator is not None: + prelaunch_validator() self.journal.append( "plugin-veto", stage=sctx.stage, @@ -6658,6 +6665,13 @@ def _run_session( / f"{self._dev_skill(role)}-result-{task_id}.md" ) prompt += WORKFLOW_COMPLETION_CONTRACT.format(marker_path=marker_path) + # Optional transaction boundary for callers whose durable launch + # authority is tied to mutable workspace input. It deliberately runs + # after every executable session hook and every prompt/snapshot repair, + # but before either the session-start record or adapter launch. The + # default keeps all existing callers byte-for-byte inert. + if prelaunch_validator is not None: + prelaunch_validator() spec = SessionSpec( task_id=task_id, role=role, diff --git a/src/bmad_loop/sweep.py b/src/bmad_loop/sweep.py index c4d5f9813..9d8497b78 100644 --- a/src/bmad_loop/sweep.py +++ b/src/bmad_loop/sweep.py @@ -3341,6 +3341,7 @@ def _restore_accepted_migration(self, task: StoryTask, baseline: str, rewrite: s def _finish_migration_commit( self, task: StoryTask, + baseline: str, manifest: list[dict[str, Any]], rewrite: str, ) -> bool: @@ -3363,6 +3364,8 @@ def _finish_migration_commit( "chore(sweep): migrate legacy deferred-work entries to DW format", path=self.workspace.paths.deferred_work, family="ledger", + accepted_text=rewrite, + accepted_baseline_text=baseline, ) if task.migration_ledger_doubt_owned and not self.state.sweep_ledger_in_doubt: # A Git-only unavailable outcome arms no ledger doubt. Retire the @@ -3402,6 +3405,41 @@ def _finish_migration_commit( self._emit("post_migrate", task) return True + def _migration_input_is_current(self, expected: str) -> bool: + """Whether the authoritative ledger still equals this cycle's input.""" + try: + return deferredwork.read_for_write(self.workspace.paths.deferred_work) == expected + except (deferredwork.LedgerReadError, OSError): + return False + + def _retire_migration_dispatch_authority( + self, + task: StoryTask, + *, + refund_attempt: bool, + ) -> NoReturn: + """Persist no-launch authority before best-effort record retirement. + + A cleanup fault is intentionally allowed to propagate only after the + durable state says PENDING with no baseline or current-format marker. + Thus leftover files are inert evidence, never recovery authority. + """ + task.phase = Phase.PENDING + task.baseline_commit = None + task.baseline_untracked = None + task.migration_recovery_format = 0 + if refund_attempt and task.attempt > 0: + task.attempt -= 1 + self._save() + for name in ( + _MIGRATE_BASELINE_RECORD, + _MIGRATE_MANIFEST_RECORD, + _MIGRATE_REWRITE_RECORD, + _MIGRATE_RESULT_RECORD, + ): + self._remove_migration_record(self.run_dir / name) + raise RuntimeError("migration ledger changed before adapter launch") + def _ensure_migration(self, text: str) -> None: """Pre-DW-format ledger content (older BMAD-method projects) blocks a sweep: open_ids() cannot see it and mark_done() cannot flip it. One @@ -3436,7 +3474,7 @@ def _ensure_migration(self, text: str) -> None: ) assert rewrite is not None self._migration_result_evidence(task, baseline, manifest, rewrite) - self._finish_migration_commit(task, manifest, rewrite) + self._finish_migration_commit(task, baseline, manifest, rewrite) return elif task.phase == Phase.TRIAGE_VERIFY and task.migration_recovery_format not in ( 0, @@ -3465,7 +3503,7 @@ def _ensure_migration(self, text: str) -> None: self._migration_result_evidence(task, baseline, manifest, rewrite) advance(task, Phase.COMMITTING) self._save() - self._finish_migration_commit(task, manifest, rewrite) + self._finish_migration_commit(task, baseline, manifest, rewrite) return # Validation completed and the accepted rewrite became durable, # but result publication did not. Put the accepted legacy input @@ -3570,6 +3608,12 @@ def _ensure_migration(self, text: str) -> None: manifest = self._migration_manifest(text) manifest_path = self.run_dir / _MIGRATE_MANIFEST_RECORD confine_root = _project_of_run_dir(self.run_dir) + # Bind the recovery records to the same authoritative input `_loop` + # supplied. This reread is deliberately immediately before the first + # publication and outside the ledger lock: records and Git work must + # never occur while that short file-I/O lock is held. + if not self._migration_input_is_current(text): + self._retire_migration_dispatch_authority(task, refund_attempt=False) try: atomic_write_text_confined( self.run_dir / _MIGRATE_BASELINE_RECORD, @@ -3597,6 +3641,11 @@ def _ensure_migration(self, text: str) -> None: self._save() raise + # A writer may have landed after the first comparison or either durable + # record write. Refuse before claiming TRIAGE_RUNNING ownership. + if not self._migration_input_is_current(text): + self._retire_migration_dispatch_authority(task, refund_attempt=False) + feedback: Path | None = None while True: task.attempt += 1 @@ -3608,6 +3657,11 @@ def _ensure_migration(self, text: str) -> None: prompt=self._migrate_prompt(manifest_path, feedback), seq=task.attempt, session_stage="pre_migrate_session", + prelaunch_validator=lambda: ( + None + if self._migration_input_is_current(text) + else self._retire_migration_dispatch_authority(task, refund_attempt=True) + ), ) advance(task, Phase.TRIAGE_VERIFY) self._save() @@ -3679,7 +3733,12 @@ def _ensure_migration(self, text: str) -> None: ) advance(task, Phase.COMMITTING) self._save() - self._finish_migration_commit(task, durable_manifest, durable_rewrite) + self._finish_migration_commit( + task, + durable_baseline, + durable_manifest, + durable_rewrite, + ) return # never re-prompt over a half-broken rewrite; the baseline reset # covers tracked files, the explicit write covers an untracked @@ -5423,7 +5482,13 @@ def _apply_decision_effect( ) def _commit_ledger( - self, message: str, *, path: Path, family: Literal["ledger", "store"] + self, + message: str, + *, + path: Path, + family: Literal["ledger", "store"], + accepted_text: str | None = None, + accepted_baseline_text: str | None = None, ) -> _LedgerCommitOutcome: """Publish the orchestrator bookkeeping FILE a phase just wrote: that one file reaches HEAD, and everything else the enclosing repository is @@ -5726,11 +5791,38 @@ def _commit_ledger( # the RESOLVED target, and a refused publish must spawn no git at all. refusal = verify.unpublishable_target(target, family) if refusal is None: - # Preserve the clean short-circuit without catching journal write faults. - clean = verify.path_clean(root, target.name) - if not clean: - attempted = True - sha = verify.commit_paths(root, message, [target]) + if accepted_text is None: + if accepted_baseline_text is not None: + raise RuntimeError( + "accepted migration baseline supplied without accepted rewrite" + ) + # Preserve the generic clean short-circuit without catching + # journal write faults. + clean = verify.path_clean(root, target.name) + if not clean: + attempted = True + sha = verify.commit_paths(root, message, [target]) + else: + if accepted_baseline_text is None: + raise RuntimeError( + "accepted migration rewrite supplied without its baseline" + ) + # Migration alone carries durable byte authority. Keep the + # lexical path as the live identity so a redirected configured + # symlink cannot be hidden by the resolved Git operand. Not + # `attempted`: the re-raise arm below exists so a refused + # ledger commit cannot degrade into the next cycle's dirty + # baseline, and `_finish_migration_commit` already ends the + # run on `unavailable` — through this arm's journal row, which + # keeps the sanitized diagnosis a bare raise would drop. + sha = verify.commit_path_bound( + root, + message, + target, + accepted_text=accepted_text, + baseline_text=accepted_baseline_text, + live_path=path, + ) except verify.GitError as e: if attempted and family == "ledger": raise # a ledger commit git was asked to make failed: publication failed diff --git a/src/bmad_loop/verify.py b/src/bmad_loop/verify.py index 10c51ad6a..2ad93fb25 100644 --- a/src/bmad_loop/verify.py +++ b/src/bmad_loop/verify.py @@ -9660,3 +9660,270 @@ def commit_paths(repo: Path, message: str, paths: list[Path]) -> str | None: if rc != 0: raise GitError(f"git commit failed: {out}") return rev_parse_head(repo) + + +def _bound_live_ledger_identity( + live_path: Path, + target: Path, + accepted_text: str, + *, + identity: tuple[int, int] | None = None, +) -> tuple[int, int]: + """Prove the live ledger is the accepted regular target without disclosing it.""" + try: + resolved = live_path.resolve(strict=True) + before = target.lstat() + if resolved != target or not S_ISREG(before.st_mode): + raise GitError("accepted publication target changed shape") + observed = live_path.read_text(encoding="utf-8") + after = target.lstat() + if ( + not S_ISREG(after.st_mode) + or (before.st_dev, before.st_ino) != (after.st_dev, after.st_ino) + or live_path.resolve(strict=True) != target + or observed != accepted_text + ): + raise GitError("accepted publication target changed during validation") + except GitError: + raise + except (OSError, RuntimeError, UnicodeDecodeError, ValueError) as exc: + raise GitError("accepted publication target could not be validated") from exc + current = (after.st_dev, after.st_ino) + if identity is not None and current != identity: + raise GitError("accepted publication target identity changed") + return current + + +def _bound_changed_paths(repo: Path, parent: str, revision: str) -> set[str]: + proc = git_bytes( + repo, + "diff-tree", + "--no-commit-id", + "--name-only", + "-z", + "-r", + parent, + revision, + "--", + ) + if proc.returncode != 0: + raise GitError(f"git candidate scope probe failed in {repo}") + return {os.fsdecode(item) for item in proc.stdout.split(b"\0") if item} + + +class _BoundCandidateMismatch(GitError): + """A candidate was fully observed and structurally rejected.""" + + +def _bound_parent(repo: Path, revision: str) -> str: + rc, lineage, _detail = _git_out(repo, "rev-list", "--parents", "--max-count=1", revision) + if rc != 0: + raise GitError(f"git candidate parent probe failed in {repo}") + parts = lineage.split() + if not parts: + raise GitError(f"git candidate parent probe returned no evidence in {repo}") + if parts[0] != revision: + raise GitError(f"git candidate parent probe returned malformed evidence in {repo}") + if len(parts) != 2: + raise _BoundCandidateMismatch("exact-path candidate does not have exactly one parent") + return parts[1] + + +def _validate_bound_candidate( + repo: Path, + revision: str, + parent: str, + rel: str, + accepted_oid: str, + baseline_oid: str, +) -> None: + if _bound_parent(repo, revision) != parent: + raise _BoundCandidateMismatch("exact-path candidate has an unexpected parent") + if _bound_changed_paths(repo, parent, revision) != {rel}: + raise _BoundCandidateMismatch( + "exact-path candidate changed paths outside its declared scope" + ) + committed = revision_blob_oids(repo, revision, (rel,)) + if committed.get(rel) != accepted_oid: + raise _BoundCandidateMismatch("exact-path candidate does not contain the accepted ledger") + parent_blob = revision_blob_oids(repo, parent, (rel,)).get(rel) + if parent_blob not in (None, baseline_oid): + raise _BoundCandidateMismatch( + "exact-path candidate parent does not contain the accepted baseline" + ) + + +def _accepted_bound_transition( + repo: Path, + head: str, + rel: str, + accepted_oid: str, + baseline_oid: str, +) -> str | None: + """Find the newest accepted ledger transition in first-parent ancestry.""" + rc, out, _detail = _git_out( + repo, + "rev-list", + "--first-parent", + head, + "--", + *_literal_specs([rel]), + ) + if rc != 0: + raise GitError(f"git accepted-transition probe failed in {repo}") + if not out: + return None + # `rev-list` is newest-first. A later commit may touch the ledger as part of + # a wider change (for example a mode-only edit beside an unrelated file) + # without invalidating the earlier exact one-path migration transition. + for candidate in out.splitlines(): + try: + parent = _bound_parent(repo, candidate) + _validate_bound_candidate(repo, candidate, parent, rel, accepted_oid, baseline_oid) + except _BoundCandidateMismatch: + continue + return candidate + return None + + +def _bound_index_oid(repo: Path, rel: str) -> str | None: + return staged_blob_oids(repo, (rel,)).get(rel) + + +def _synchronize_bound_index( + repo: Path, + head: str, + rel: str, + accepted_oid: str, + observed_index_oid: str | None, +) -> None: + # A target-local reset preserves every unrelated real-index entry. Refuse + # an observed same-path writer instead of overwriting it between validation + # and housekeeping; the published commit remains authoritative and replayable. + if _bound_index_oid(repo, rel) != observed_index_oid: + raise GitError("real index target changed during exact-path publication") + rc, out = _git(repo, "reset", head, "--", *_literal_specs([rel])) + if rc != 0: + raise GitError(f"git target-local index synchronization failed in {repo}: {out}") + if _bound_index_oid(repo, rel) != accepted_oid: + raise GitError("real index target synchronization did not retain accepted content") + + +def commit_path_bound( + repo: Path, + message: str, + path: Path, + *, + accepted_text: str, + baseline_text: str, + live_path: Path | None = None, +) -> str | None: + """Publish one accepted ledger transition through a validated candidate. + + The candidate is committed in a detached temporary worktree, so ordinary Git + hooks run without moving the authoritative checkout. Its parent, exact path + delta, Git-clean-filtered blob, live decoded text, and path identity are all + validated before an expected-old ``HEAD`` update-ref publishes it. Once that + CAS succeeds, target-only real-index reconciliation is replayable housekeeping: + no later fault rolls the truthful commit back. + """ + try: + rc, top, _detail = _git_out(repo, "rev-parse", "--show-toplevel") + if rc != 0: + raise GitError(f"git repository root probe failed in {repo}") + repo_root = Path(top).resolve() + target = path.resolve(strict=True) + rel = target.relative_to(repo_root).as_posix() + except GitError: + raise + except (OSError, RuntimeError, ValueError) as exc: + raise GitError("exact-path publication target could not be resolved safely") from exc + + lexical = live_path if live_path is not None else path + accepted_bytes = accepted_text.encode("utf-8") + baseline_bytes = baseline_text.encode("utf-8") + accepted_oid = git_normalized_blob_oid_for_bytes(repo_root, rel, accepted_bytes) + baseline_oid = git_normalized_blob_oid_for_bytes(repo_root, rel, baseline_bytes) + identity = _bound_live_ledger_identity(lexical, target, accepted_text) + head = rev_parse_head(repo_root) + + accepted = _accepted_bound_transition(repo_root, head, rel, accepted_oid, baseline_oid) + authority_parent = _bound_parent(repo_root, accepted) if accepted is not None else head + head_blob = revision_blob_oids(repo_root, head, (rel,)).get(rel) + parent_has_target = rel in revision_blob_oids(repo_root, authority_parent, (rel,)) + observed_index_oid = _bound_index_oid(repo_root, rel) + allowed_index_oids: set[str | None] = {accepted_oid, baseline_oid} + if not parent_has_target: + # Original absence is the one accepted no-entry shape. A tracked + # parent's absent real-index entry is a foreign staged deletion and is + # refused rather than silently re-added. + allowed_index_oids.add(None) + if observed_index_oid not in allowed_index_oids: + raise GitError("real index holds foreign content at the publication target") + + if accepted is not None: + _bound_live_ledger_identity(lexical, target, accepted_text, identity=identity) + _synchronize_bound_index(repo_root, head, rel, accepted_oid, observed_index_oid) + return accepted + + # Preserve generic clean/ignored behavior when no migration transition needs + # replay. In particular, an ignored ledger never earns a synthetic commit. + clean = path_clean(repo_root, rel) + ignored_untracked = clean and head_blob is None and path_ignored(repo_root, target) + if clean and (head_blob == accepted_oid or ignored_untracked): + _bound_live_ledger_identity(lexical, target, accepted_text, identity=identity) + return None + + active_error: BaseException | None = None + candidate: str | None = None + with tempfile.TemporaryDirectory() as td: + candidate_root = Path(td) / "candidate" + rc, out = _git( + repo_root, + "worktree", + "add", + "--detach", + str(candidate_root), + head, + ) + if rc != 0: + raise GitError(f"git detached candidate checkout failed in {repo_root}: {out}") + try: + candidate_path = candidate_root / rel + candidate_path.parent.mkdir(parents=True, exist_ok=True) + candidate_path.write_bytes(accepted_bytes) + rc, out = _git(candidate_root, "add", "--", *_literal_specs([rel])) + if rc != 0: + raise GitError(f"git exact-path candidate staging failed in {repo_root}: {out}") + if staged_blob_oid(candidate_root, rel) != accepted_oid: + raise GitError("exact-path candidate staging changed accepted content") + _bound_live_ledger_identity(lexical, target, accepted_text, identity=identity) + rc, out = _git(candidate_root, "commit", "-m", message) + if rc != 0: + raise GitError(f"git exact-path candidate commit failed in {repo_root}: {out}") + candidate = rev_parse_head(candidate_root) + _validate_bound_candidate(repo_root, candidate, head, rel, accepted_oid, baseline_oid) + _bound_live_ledger_identity(lexical, target, accepted_text, identity=identity) + rc, out = _git(repo_root, "update-ref", "HEAD", candidate, head) + if rc != 0: + raise GitError("authoritative HEAD changed during exact-path publication") + except BaseException as exc: + active_error = exc + raise + finally: + rc, out = _git( + repo_root, + "worktree", + "remove", + "--force", + str(candidate_root), + ) + if rc != 0 and active_error is None: + raise GitError(f"git detached candidate cleanup failed in {repo_root}: {out}") + + assert candidate is not None + published_head = rev_parse_head(repo_root) + if published_head != candidate: + raise GitError("authoritative HEAD changed after exact-path publication") + _synchronize_bound_index(repo_root, published_head, rel, accepted_oid, observed_index_oid) + return candidate diff --git a/tests/test_sweep.py b/tests/test_sweep.py index 1e0be30fc..05da32ea1 100644 --- a/tests/test_sweep.py +++ b/tests/test_sweep.py @@ -11454,6 +11454,18 @@ def test_every_sweep_ledger_commit_names_its_own_tree(): } assert mismatched == {}, f"_commit_ledger calls declaring the wrong family: {mismatched}" + accepted_bindings = { + node.lineno: { + kw.arg for kw in node.keywords if kw.arg in {"accepted_text", "accepted_baseline_text"} + } + for node in calls + if any(kw.arg in {"accepted_text", "accepted_baseline_text"} for kw in node.keywords) + } + assert len(accepted_bindings) == 1 + binding_line, binding_args = next(iter(accepted_bindings.items())) + assert published[binding_line] == "self.workspace.paths.deferred_work" + assert binding_args == {"accepted_text", "accepted_baseline_text"} + # ------------------- DW-222/223/224: the DW-193 publisher's remaining residuals — # a decision-phase close stranded at the no-open exit, a terminal cycle stop that @@ -31729,3 +31741,298 @@ def record_merge(repo, operand, **kwargs): assert not summary.paused and not summary.crashed assert hooks == ["pre_integrate", "pre_merge", "post_merge"] assert operands == [task.commit_sha] + + +# ------------------------ migration byte-binding transaction boundaries + + +@pytest.mark.parametrize("record_name", ["migrate-baseline.md", "migrate-manifest.json"]) +def test_migration_refuses_rival_after_recovery_record_publication( + project, monkeypatch, record_name +): + write_legacy_ledger(project, LEGACY_LEDGER) + engine, adapter = make_sweep( + project, [migrate_effect(project, migrated_ledger(), _valid_migration_mapping())] + ) + rival = LEGACY_LEDGER + "\n\n" + real_write = sweep_mod.atomic_write_text_confined + landed = [] + + def publish_then_rival(path, text, **kwargs): + result = real_write(path, text, **kwargs) + if path.name == record_name and not landed: + landed.append(True) + project.deferred_work.write_text(rival, encoding="utf-8") + return result + + monkeypatch.setattr(sweep_mod, "atomic_write_text_confined", publish_then_rival) + summary = engine.run() + + assert summary.crashed and adapter.sessions == [] and landed == [True] + persisted = load_state(engine.run_dir).tasks["sweep-migrate"] + assert persisted.phase == Phase.PENDING + assert persisted.attempt == 0 + assert persisted.baseline_commit is None and persisted.baseline_untracked is None + assert persisted.migration_recovery_format == 0 + assert project.deferred_work.read_text(encoding="utf-8") == rival + assert list(engine.run_dir.glob("migrate-*")) == [] + + +@pytest.mark.parametrize( + "fault_at", + [2, 3, 4], + ids=["setup-reread", "post-record-reread", "post-hook-prelaunch-reread"], +) +@pytest.mark.parametrize("fault_type", [deferredwork.LedgerReadError, OSError]) +def test_migration_reread_fault_retires_predispatch_authority( + project, monkeypatch, fault_at, fault_type +): + write_legacy_ledger(project, LEGACY_LEDGER) + engine, adapter = make_sweep( + project, [migrate_effect(project, migrated_ledger(), _valid_migration_mapping())] + ) + real_read = deferredwork.read_for_write + reads = [] + + def fault_selected_read(path): + reads.append(path) + if len(reads) == fault_at: + raise fault_type("injected migration reread fault") + return real_read(path) + + monkeypatch.setattr(deferredwork, "read_for_write", fault_selected_read) + summary = engine.run() + + assert summary.crashed and adapter.sessions == [] + persisted = load_state(engine.run_dir).tasks["sweep-migrate"] + assert persisted.phase == Phase.PENDING and persisted.attempt == 0 + assert persisted.baseline_commit is None and persisted.baseline_untracked is None + assert persisted.migration_recovery_format == 0 + assert list(engine.run_dir.glob("migrate-*")) == [] + + +def test_migration_refuses_rival_between_cycle_read_and_setup_reread(project, monkeypatch): + write_legacy_ledger(project, LEGACY_LEDGER) + engine, adapter = make_sweep( + project, [migrate_effect(project, migrated_ledger(), _valid_migration_mapping())] + ) + rival = LEGACY_LEDGER + "\n\n" + real_cycle_read = engine._read_cycle_ledger + + def read_then_rival(path): + answer = real_cycle_read(path) + project.deferred_work.write_text(rival, encoding="utf-8") + return answer + + monkeypatch.setattr(engine, "_read_cycle_ledger", read_then_rival) + summary = engine.run() + + assert summary.crashed and adapter.sessions == [] + persisted = load_state(engine.run_dir).tasks["sweep-migrate"] + assert persisted.phase == Phase.PENDING and persisted.attempt == 0 + assert persisted.baseline_commit is None and persisted.baseline_untracked is None + assert project.deferred_work.read_text(encoding="utf-8") == rival + + +@pytest.mark.parametrize("stage", ["pre_migrate_session", "pre_session"]) +def test_migration_hook_rival_is_refused_at_the_true_launch_boundary(project, stage): + write_legacy_ledger(project, LEGACY_LEDGER) + rival = LEGACY_LEDGER + "\n\n" + + class MutatingPlugin(Plugin): + def on_pre_migrate_session(self, _context): + if stage == "pre_migrate_session": + project.deferred_work.write_text(rival, encoding="utf-8") + + def on_pre_session(self, _context): + if stage == "pre_session": + project.deferred_work.write_text(rival, encoding="utf-8") + + manifest = PluginManifest(name=f"migration-{stage}") + registry = PluginRegistry( + [LoadedPlugin(manifest=manifest, instance=MutatingPlugin(manifest, {}))] + ) + engine, adapter = make_sweep( + project, + [migrate_effect(project, migrated_ledger(), _valid_migration_mapping())], + registry=registry, + ) + + summary = engine.run() + + assert summary.crashed and adapter.sessions == [] + persisted = load_state(engine.run_dir).tasks["sweep-migrate"] + assert persisted.phase == Phase.PENDING and persisted.attempt == 0 + assert persisted.baseline_commit is None and persisted.baseline_untracked is None + assert persisted.migration_recovery_format == 0 + assert project.deferred_work.read_text(encoding="utf-8") == rival + + +def test_migration_mutating_veto_retires_authority_without_adapter_launch(project): + write_legacy_ledger(project, LEGACY_LEDGER) + rival = LEGACY_LEDGER + "\n\n" + + class MutatingVetoPlugin(Plugin): + def on_pre_migrate_session(self, context): + project.deferred_work.write_text(rival, encoding="utf-8") + context.veto("defer", "migration input changed") + + manifest = PluginManifest(name="migration-mutating-veto") + registry = PluginRegistry( + [LoadedPlugin(manifest=manifest, instance=MutatingVetoPlugin(manifest, {}))] + ) + engine, adapter = make_sweep( + project, + [migrate_effect(project, migrated_ledger(), _valid_migration_mapping())], + registry=registry, + ) + + summary = engine.run() + + assert summary.crashed and adapter.sessions == [] + persisted = load_state(engine.run_dir).tasks["sweep-migrate"] + assert persisted.phase == Phase.PENDING and persisted.attempt == 0 + assert persisted.baseline_commit is None and persisted.baseline_untracked is None + assert persisted.migration_recovery_format == 0 + assert project.deferred_work.read_text(encoding="utf-8") == rival + assert list(engine.run_dir.glob("migrate-*")) == [] + + +def test_migration_normal_launch_runs_the_prelaunch_validator_once(project, monkeypatch): + write_legacy_ledger(project, LEGACY_LEDGER) + mapping = _valid_migration_mapping() + checks = [] + + def migrate_after_three_checks(spec): + # Setup owns the first two comparisons; a normal `_run_session` path + # contributes exactly one post-hook check before reaching the adapter. + assert len(checks) == 3 + return migrate_effect(project, migrated_ledger(), mapping)(spec) + + plan = triage_result(["DW-2"], skip=[{"id": "DW-2", "reason": "later"}]) + engine, _adapter = make_sweep( + project, + [migrate_after_three_checks, triage_effect(plan)], + ) + real_check = engine._migration_input_is_current + + def count_check(expected): + checks.append(expected) + return real_check(expected) + + monkeypatch.setattr(engine, "_migration_input_is_current", count_check) + + summary = engine.run() + + assert not summary.crashed and not summary.paused + assert checks == [LEGACY_LEDGER, LEGACY_LEDGER, LEGACY_LEDGER] + + +def test_migration_cleanup_fault_follows_durable_no_authority_state(project, monkeypatch): + write_legacy_ledger(project, LEGACY_LEDGER) + engine, adapter = make_sweep( + project, [migrate_effect(project, migrated_ledger(), _valid_migration_mapping())] + ) + real_write = sweep_mod.atomic_write_text_confined + rival = LEGACY_LEDGER + "\n\n" + + def manifest_then_rival(path, text, **kwargs): + result = real_write(path, text, **kwargs) + if path.name == "migrate-manifest.json": + project.deferred_work.write_text(rival, encoding="utf-8") + return result + + real_remove = engine._remove_migration_record + + def cleanup_fault(path): + if project.deferred_work.read_text(encoding="utf-8") == rival: + raise OSError("record cleanup fault") + return real_remove(path) + + monkeypatch.setattr(sweep_mod, "atomic_write_text_confined", manifest_then_rival) + monkeypatch.setattr(engine, "_remove_migration_record", cleanup_fault) + summary = engine.run() + + assert summary.crashed and adapter.sessions == [] + persisted = load_state(engine.run_dir).tasks["sweep-migrate"] + assert persisted.phase == Phase.PENDING and persisted.attempt == 0 + assert persisted.baseline_commit is None and persisted.baseline_untracked is None + assert persisted.migration_recovery_format == 0 + assert project.deferred_work.read_text(encoding="utf-8") == rival + + +@pytest.mark.parametrize("window", ["before-staging", "after-staging"]) +def test_migration_rival_during_bound_publication_replays_commit_only(project, monkeypatch, window): + write_legacy_ledger(project, LEGACY_LEDGER) + mapping = _valid_migration_mapping() + engine, first_adapter = make_sweep( + project, [migrate_effect(project, migrated_ledger(), mapping)] + ) + original_head = verify.rev_parse_head(project.project) + accepted = migrated_ledger() + rival = accepted + "\n\n" + real_git = verify._git + landed = [] + + def inject_rival(git_repo, *args, **kwargs): + before_stage = window == "before-staging" and args[:2] == ("worktree", "add") + after_stage = ( + window == "after-staging" and args[:1] == ("commit",) and git_repo != project.project + ) + if (before_stage or after_stage) and not landed: + landed.append(True) + project.deferred_work.write_text(rival, encoding="utf-8") + return real_git(git_repo, *args, **kwargs) + + monkeypatch.setattr(verify, "_git", inject_rival) + first = engine.run() + + assert first.crashed and landed == [True] and len(first_adapter.sessions) == 1 + assert engine.state.tasks["sweep-migrate"].phase == Phase.COMMITTING + assert verify.rev_parse_head(project.project) == original_head + assert project.deferred_work.read_text(encoding="utf-8") == rival + + monkeypatch.setattr(verify, "_git", real_git) + project.deferred_work.write_text(accepted, encoding="utf-8") + plan = triage_result(["DW-2"], skip=[{"id": "DW-2", "reason": "later"}]) + resumed, resumed_adapter = resume_sweep(project, engine, [triage_effect(plan)]) + second = resumed.run() + + assert not second.crashed and not second.paused + assert resumed.state.tasks["sweep-migrate"].phase == Phase.DONE + assert len(resumed_adapter.sessions) == 1 + assert "--migrate" not in resumed_adapter.sessions[0].prompt + assert project.deferred_work.read_text(encoding="utf-8") == accepted + + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") +def test_migration_forwards_lexical_symlink_identity_to_bound_publisher(project, monkeypatch): + write_legacy_ledger(project, LEGACY_LEDGER) + ledger = project.deferred_work + target = ledger.with_name("migration-ledger-target.md") + ledger.rename(target) + ledger.symlink_to(target.name) + git(project.project, "add", "-A") + git(project.project, "commit", "-q", "-m", "track symlinked migration ledger") + plan = triage_result(["DW-2"], skip=[{"id": "DW-2", "reason": "later"}]) + engine, _adapter = make_sweep( + project, + [ + migrate_effect(project, migrated_ledger(), _valid_migration_mapping()), + triage_effect(plan), + ], + ) + real_publish = verify.commit_path_bound + seen = [] + + def record_publish(repo, message, path, **kwargs): + seen.append((path, kwargs.get("live_path"))) + return real_publish(repo, message, path, **kwargs) + + monkeypatch.setattr(verify, "commit_path_bound", record_publish) + summary = engine.run() + + assert not summary.crashed and not summary.paused + assert seen == [(target.resolve(), ledger)] + assert ledger.is_symlink() + assert target.read_text(encoding="utf-8") == migrated_ledger() diff --git a/tests/test_verify.py b/tests/test_verify.py index e56c6b00a..3976d81d7 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -8047,6 +8047,422 @@ def test_verify_dev_stories_roots_its_exclude_on_the_code_tree(project, tmp_path assert paths.project != paths.repo_root +# ------------------------------------------------ accepted exact-path publication + + +def _bound_publish_inputs(project): + repo = project.project + path = repo / "src.txt" + baseline = path.read_text(encoding="utf-8") + accepted = "accepted migration ledger\n" + path.write_text(accepted, encoding="utf-8") + return repo, path, baseline, accepted + + +def test_commit_path_bound_publishes_only_accepted_path_and_preserves_real_index(project): + repo, path, baseline, accepted = _bound_publish_inputs(project) + unrelated = repo / "operator.txt" + unrelated.write_text("staged operator work\n", encoding="utf-8") + git(repo, "add", "--", unrelated.name) + + sha = verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + + assert sha == verify.rev_parse_head(repo) + assert git(repo, "show", "--format=", "--name-only", sha) == "src.txt" + assert git(repo, "show", f"{sha}:src.txt") == accepted.rstrip("\n") + assert git(repo, "diff", "--cached", "--name-only") == unrelated.name + + +@pytest.mark.parametrize("index_flag", ["--assume-unchanged", "--skip-worktree"]) +def test_commit_path_bound_does_not_accept_a_false_clean_index_flag(project, index_flag): + repo, path, baseline, accepted = _bound_publish_inputs(project) + # Apply the hiding bit after the working-tree edit: both flags can suppress + # porcelain even though HEAD still contains the legacy baseline. + git(repo, "update-index", index_flag, "--", path.name) + assert verify.path_clean(repo, path.name) + + sha = verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + + assert sha is not None + assert git(repo, "show", f"{sha}:src.txt") == accepted.rstrip("\n") + + +def test_commit_path_bound_refuses_a_candidate_whose_parent_is_not_the_bound_baseline(project): + repo, path, _baseline, accepted = _bound_publish_inputs(project) + original_head = verify.rev_parse_head(repo) + wrong_baseline = "different claimed baseline\n" + shadow = repo / "wrong-baseline.txt" + shadow.write_text(wrong_baseline, encoding="utf-8") + wrong_oid = git(repo, "hash-object", "-w", "--", str(shadow)) + shadow.unlink() + git(repo, "update-index", "--cacheinfo", "100644", wrong_oid, path.name) + + with pytest.raises(verify.GitError, match="parent.*accepted baseline"): + verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=wrong_baseline, + ) + + assert verify.rev_parse_head(repo) == original_head + assert path.read_text(encoding="utf-8") == accepted + + +def test_commit_path_bound_rejects_hook_mutation_before_authoritative_publication(project): + repo, path, baseline, accepted = _bound_publish_inputs(project) + original_head = verify.rev_parse_head(repo) + hook = repo / ".git" / "hooks" / "pre-commit" + hook.write_text("#!/bin/sh\nprintf 'hook bytes\\n' > src.txt\ngit add -- src.txt\n") + hook.chmod(0o755) + + with pytest.raises(verify.GitError, match="accepted ledger"): + verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + + assert verify.rev_parse_head(repo) == original_head + assert path.read_text(encoding="utf-8") == accepted + + +def test_commit_path_bound_rejects_hook_added_paths_before_authoritative_publication(project): + repo, path, baseline, accepted = _bound_publish_inputs(project) + original_head = verify.rev_parse_head(repo) + hook = repo / ".git" / "hooks" / "pre-commit" + hook.write_text( + "#!/bin/sh\nprintf 'extra hook path\\n' > hook-extra.txt\ngit add -- hook-extra.txt\n" + ) + hook.chmod(0o755) + + with pytest.raises(verify.GitError, match="outside its declared scope"): + verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + + assert verify.rev_parse_head(repo) == original_head + assert not (repo / "hook-extra.txt").exists() + + +def test_bound_candidate_refuses_a_merge_commit_even_with_an_exact_path_delta(project): + repo = project.project + path = repo / "src.txt" + rel = path.name + baseline = path.read_text(encoding="utf-8") + original_branch = git(repo, "rev-parse", "--abbrev-ref", "HEAD") + git(repo, "checkout", "-q", "-b", "bound-side") + accepted = "accepted merge candidate\n" + path.write_text(accepted, encoding="utf-8") + git(repo, "add", "--", rel) + git(repo, "commit", "-q", "-m", "side ledger change") + git(repo, "checkout", "-q", original_branch) + git(repo, "merge", "-q", "--no-ff", "bound-side", "-m", "merge ledger candidate") + candidate = verify.rev_parse_head(repo) + first_parent = git(repo, "rev-parse", "HEAD^1") + accepted_oid = verify.git_normalized_blob_oid_for_bytes(repo, rel, accepted.encode()) + baseline_oid = verify.git_normalized_blob_oid_for_bytes(repo, rel, baseline.encode()) + + with pytest.raises(verify.GitError, match="exactly one parent"): + verify._validate_bound_candidate( + repo, + candidate, + first_parent, + rel, + accepted_oid, + baseline_oid, + ) + + +def test_commit_path_bound_refuses_a_tracked_target_index_deletion(project): + repo, path, baseline, accepted = _bound_publish_inputs(project) + original_head = verify.rev_parse_head(repo) + git(repo, "rm", "--cached", "--", path.name) + + with pytest.raises(verify.GitError, match="foreign content"): + verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + + assert verify.rev_parse_head(repo) == original_head + assert git(repo, "ls-files", "--", path.name) == "" + + +def test_commit_path_bound_expected_old_cas_preserves_concurrent_head(project, monkeypatch): + repo, path, baseline, accepted = _bound_publish_inputs(project) + real_git = verify._git + concurrent_head = [] + + def advance_before_cas(git_repo, *args, **kwargs): + if args[:2] == ("update-ref", "HEAD") and not concurrent_head: + rival = repo / "concurrent.txt" + rival.write_text("keep concurrent commit\n", encoding="utf-8") + git(repo, "add", "--", rival.name) + git(repo, "commit", "-q", "-m", "concurrent commit") + concurrent_head.append(verify.rev_parse_head(repo)) + return real_git(git_repo, *args, **kwargs) + + monkeypatch.setattr(verify, "_git", advance_before_cas) + + with pytest.raises(verify.GitError, match="HEAD changed"): + verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + + assert verify.rev_parse_head(repo) == concurrent_head[0] + assert (repo / "concurrent.txt").read_text(encoding="utf-8") == "keep concurrent commit\n" + + +def test_commit_path_bound_replays_accepted_ancestor_beneath_unrelated_descendant(project): + repo, path, baseline, accepted = _bound_publish_inputs(project) + published = verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + descendant = repo / "descendant.txt" + descendant.write_text("keep descendant\n", encoding="utf-8") + git(repo, "add", "--", descendant.name) + git(repo, "commit", "-q", "-m", "unrelated descendant") + descendant_head = verify.rev_parse_head(repo) + + replayed = verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + + assert replayed == published + assert verify.rev_parse_head(repo) == descendant_head + assert git(repo, "diff", "--cached", "--name-only") == "" + + +def test_commit_path_bound_skips_newer_invalid_same_ledger_descendant(project): + repo, path, baseline, accepted = _bound_publish_inputs(project) + published = verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + extra = repo / "descendant-extra.txt" + extra.write_text("wider descendant\n", encoding="utf-8") + git(repo, "update-index", "--chmod=+x", "--", path.name) + git(repo, "add", "--", extra.name) + git(repo, "commit", "-q", "-m", "multi-path same-ledger descendant") + descendant_head = verify.rev_parse_head(repo) + assert set(git(repo, "show", "--format=", "--name-only", "HEAD").splitlines()) == { + path.name, + extra.name, + } + + replayed = verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + + assert replayed == published + assert verify.rev_parse_head(repo) == descendant_head + assert git(repo, "show", "HEAD:src.txt") == accepted.rstrip("\n") + + +def test_commit_path_bound_propagates_probe_failure_on_a_newer_candidate(project, monkeypatch): + repo, path, baseline, accepted = _bound_publish_inputs(project) + verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + git(repo, "update-index", "--chmod=+x", "--", path.name) + git(repo, "commit", "-q", "-m", "newer ledger mode transition") + descendant_head = verify.rev_parse_head(repo) + + def unavailable_scope(*_args, **_kwargs): + raise verify.GitError("candidate scope unavailable") + + monkeypatch.setattr(verify, "_bound_changed_paths", unavailable_scope) + + with pytest.raises(verify.GitError, match="candidate scope unavailable"): + verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + + assert verify.rev_parse_head(repo) == descendant_head + + +def test_commit_path_bound_keeps_published_commit_when_index_sync_faults(project, monkeypatch): + repo, path, baseline, accepted = _bound_publish_inputs(project) + original_head = verify.rev_parse_head(repo) + real_git = verify._git + faulted = [] + + def fail_target_sync(git_repo, *args, **kwargs): + if args[:1] == ("reset",) and not faulted: + faulted.append(True) + return 1, "injected sync fault" + return real_git(git_repo, *args, **kwargs) + + monkeypatch.setattr(verify, "_git", fail_target_sync) + with pytest.raises(verify.GitError, match="index synchronization"): + verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + + published = verify.rev_parse_head(repo) + assert published != original_head + monkeypatch.setattr(verify, "_git", real_git) + assert ( + verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + == published + ) + assert git(repo, "diff", "--cached", "--name-only") == "" + + +def test_commit_path_bound_refuses_foreign_target_index_change_before_sync(project, monkeypatch): + repo, path, baseline, accepted = _bound_publish_inputs(project) + shadow = repo / "foreign-index.txt" + shadow.write_text("foreign staged ledger\n", encoding="utf-8") + foreign_oid = git(repo, "hash-object", "-w", "--", str(shadow)) + shadow.unlink() + real_git = verify._git + published = [] + + def change_index_after_cas(git_repo, *args, **kwargs): + result = real_git(git_repo, *args, **kwargs) + if args[:2] == ("update-ref", "HEAD") and result[0] == 0 and not published: + published.append(args[2]) + git(repo, "update-index", "--cacheinfo", "100644", foreign_oid, path.name) + return result + + monkeypatch.setattr(verify, "_git", change_index_after_cas) + + with pytest.raises(verify.GitError, match="real index target changed"): + verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + + assert verify.rev_parse_head(repo) == published[0] + assert verify.staged_blob_oid(repo, path.name) == foreign_oid + + +def test_commit_path_bound_rejects_lossy_filter_live_text_drift(project, monkeypatch): + repo = project.project + attributes = repo / ".gitattributes" + attributes.write_text("src.txt filter=collapse\n", encoding="utf-8") + git(repo, "config", "filter.collapse.clean", "sed s/rival/accepted/") + git(repo, "add", ".gitattributes") + git(repo, "commit", "-q", "-m", "configure lossy filter") + repo, path, baseline, _accepted = _bound_publish_inputs(project) + accepted = "accepted\n" + path.write_text(accepted, encoding="utf-8") + original_head = verify.rev_parse_head(repo) + real_git = verify._git + + def rival_after_staging(git_repo, *args, **kwargs): + if args[:1] == ("commit",) and git_repo != repo: + path.write_text("rival\n", encoding="utf-8") + return real_git(git_repo, *args, **kwargs) + + assert verify.git_normalized_blob_oid_for_bytes( + repo, "src.txt", accepted.encode() + ) == verify.git_normalized_blob_oid_for_bytes(repo, "src.txt", b"rival\n") + monkeypatch.setattr(verify, "_git", rival_after_staging) + + with pytest.raises(verify.GitError, match="target changed"): + verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + + assert verify.rev_parse_head(repo) == original_head + assert path.read_text(encoding="utf-8") == "rival\n" + + +def test_commit_path_bound_rejects_post_staging_symlink_substitution(project, monkeypatch): + repo, path, baseline, accepted = _bound_publish_inputs(project) + original_head = verify.rev_parse_head(repo) + replacement = repo / "replacement.txt" + replacement.write_text(accepted, encoding="utf-8") + real_git = verify._git + + def substitute_after_staging(git_repo, *args, **kwargs): + if args[:1] == ("commit",) and git_repo != repo: + path.unlink() + path.symlink_to(replacement.name) + return real_git(git_repo, *args, **kwargs) + + monkeypatch.setattr(verify, "_git", substitute_after_staging) + with pytest.raises(verify.GitError, match="target changed"): + verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + + assert verify.rev_parse_head(repo) == original_head + assert path.is_symlink() + + def test_stories_relpaths_follows_the_root_it_is_given(project, tmp_path): """Same rule for the stories-mode exclude: rooted where git runs.""" paths = _repo_root_override(project, tmp_path) From 8c7e927b6d27b5079d77c73a32704b4b36acb900 Mon Sep 17 00:00:00 2001 From: t Date: Mon, 14 Sep 2026 17:31:25 -0700 Subject: [PATCH 05/17] sweep dw2-migration-ledger-byte-binding: DW-311, DW-316 via bmad-loop --- docs/FEATURES.md | 2 +- src/bmad_loop/verify.py | 584 ++++++++++++++++++++++++++++---- tests/test_sweep.py | 39 +++ tests/test_verify.py | 723 ++++++++++++++++++++++++++++++++++++++-- 4 files changed, 1267 insertions(+), 81 deletions(-) diff --git a/docs/FEATURES.md b/docs/FEATURES.md index 2a23de1a8..9fde99cc0 100644 --- a/docs/FEATURES.md +++ b/docs/FEATURES.md @@ -252,7 +252,7 @@ Result` section. Every other spec keeps warn-and-continue, and the record says w ### Deferred-work sweeps - Skills accumulate an append-only ledger (`deferred-work.md`, `DW-` entries): split-off goals, pre-existing findings, "needs human decision" items. -- Legacy-ledger migration recovery (DW-296/DW-297) persists a current-format marker plus run-owned `migrate-baseline.md` and `migrate-rewrite.md` text snapshots. The baseline and its exact reconstructed manifest are durable before dispatch; migration rechecks the live cycle input before and after publishing those records and once more after executable pre-session hooks at the actual adapter-launch boundary (DW-316). Drift or a read fault first persists `PENDING` with no baseline authority and refunds an attempt that launched no adapter, then retires the stale records, so even a cleanup fault cannot resurrect stale dispatch authority. The rewrite becomes authoritative only after deterministic validation. A result-publication fault therefore leaves `sweep-migrate` nonterminal, and cycle-one resume compare-and-set restores the accepted legacy text before redispatching without overwriting concurrent ledger bytes or resetting past an advanced HEAD. Once `migrate-result.json` is durable the task enters `committing`: resume validates the baseline/manifest/rewrite/result set and retries only the exact-path publication tail. For migration, that tail stages in an isolated hook-observed candidate, validates its exact parent, one-path scope, Git-normalized accepted blob, decoded live text, and regular resolved target before publishing through an expected-old `HEAD` CAS (DW-311). Post-CAS target-index synchronization is target-local replayable housekeeping; resume can recognize the validated ledger transition beneath unrelated first-parent descendants without moving them. `DONE` is earned only by that accepted commit or by a clean outcome that still contains the accepted rewrite. Missing, nonregular, unreadable, malformed, or mutually inconsistent current-format records required by the task's persisted recovery phase escalate without reset or publication; a missing result during `triage-verify` instead triggers the intentional restore-and-redispatch path, while unmarked pre-upgrade tasks retain the older reset-and-reread route. +- Legacy-ledger migration recovery (DW-296/DW-297) persists a current-format marker plus run-owned `migrate-baseline.md` and `migrate-rewrite.md` text snapshots. The baseline and its exact reconstructed manifest are durable before dispatch; migration rechecks the live cycle input before and after publishing those records and once more after executable pre-session hooks at the actual adapter-launch boundary (DW-316). Drift or a read fault first persists `PENDING` with no baseline authority and refunds an attempt that launched no adapter, then retires the stale records, so even a cleanup fault cannot resurrect stale dispatch authority. The rewrite becomes authoritative only after deterministic validation. A result-publication fault therefore leaves `sweep-migrate` nonterminal, and cycle-one resume compare-and-set restores the accepted legacy text before redispatching without overwriting concurrent ledger bytes or resetting past an advanced HEAD. Once `migrate-result.json` is durable the task enters `committing`: resume validates the baseline/manifest/rewrite/result set and retries only the exact-path publication tail. For migration, that tail captures the checkout's immediate and terminal branch identity, refuses rival committed ledger content, stages an isolated hook-observed candidate, and validates its exact parent, one-path scope, Git-normalized accepted blob, decoded live text, and regular resolved target. It then prepares an expected-old transaction against only the captured terminal branch, proves that ref is still direct while its lock is held, and commits the transaction without updating symbolic `HEAD` (DW-311). A lost commit acknowledgement is settled only by re-observing and fully validating the candidate or by a later retry from unchanged authority; it is never compensated. Post-publication target-index synchronization brackets target-local resets with bounded checkout observations, preserves unrelated stages, repairs toward the newest observed committed tree, and refuses a moving attempt for replay. Resume can recognize the validated ledger transition beneath unrelated first-parent descendants without moving them. `DONE` is earned only by that accepted commit or by a clean outcome that still contains the accepted rewrite. Missing, nonregular, unreadable, malformed, or mutually inconsistent current-format records required by the task's persisted recovery phase escalate without reset or publication; a missing result during `triage-verify` instead triggers the intentional restore-and-redispatch path, while unmarked pre-upgrade tasks retain the older reset-and-reread route. - Ledger read contract (DW-146/DW-279): `read_for_write` returns `None` for the existing metadata-absence cases and nonregular targets. Nonabsence metadata faults and all text-read `OSError` failures, including disappearance/type-change races after a successful probe, raise `LedgerReadFault(LedgerReadError)` with the original exception chained as `__cause__`. Invalid UTF-8 still raises `LedgerReadError` with a `UnicodeDecodeError` cause. A refused authoritative read publishes nothing. Pre-lock presence probes, lock acquisition and writes retain raw `OSError`; observation reads retain their empty-text-plus-attributed-fault degradation. Consumers distinguish OS refusal from decoding before handling the parent exception. - Frontmatter harvest bridge (BMAD-METHOD#2640/#2651; shipped 0.9.1, hardened #433): since BMAD-METHOD 6.10.1-next.33 the unattended primitive records defer-triaged review findings in its spec's frontmatter `deferred:` list (summary/evidence, optional location/severity) and writes nothing to the ledger. The orchestrator harvests them itself — post-session but _above_ the artifact gate, so before verification and before the attempt is accepted — into canonical `### DW-` entries, so `deferred-work.md` stays the sweep's sole read surface. Entries therefore appear even when the attempt goes on to fail verification: a fixable retry deliberately keeps them (the attribution reference moves onto the kept tree), and `_harvest_gate_exclude` stops the engine's own append from counting as the session's proof of work. Dedupe key is the fingerprinted `origin: spec-deferred ` plus `source_spec:`, scanned across entries of _every_ status, so a replay neither doubles an entry nor re-opens a closed one. Era-agnostic (the gate is the field's presence, never the skill name resolved on disk) and bounded to sessions bmad-loop drove to a success status — `in-review` with the follow-up review enabled, else `done`, plus an operator park; a plan-halt checkpoint keeps its notes for the implementation pass. A spec outside the orchestrator-owned roots is refused (`spec-deferrals-skipped-out-of-tree`) and an unreadable one retries the session rather than accepting it with findings silently dropped; `deferred:` items that will not parse are journaled (`spec-deferrals-malformed`) and filed as one low-severity entry naming the spec. A ledger whose bytes do not decode is routed at every one of the engine's own four `read_for_write` sites (DW-231) according to what the read was about to do: the observation reads — the proof-of-work digest, the pre-harvest snapshot, the defer snapshot and the two restores' compare-and-set probes — degrade to a typed answer nothing can write back or anchor a write on (the digest hashes the raw bytes, so "did the ledger change" stays exact; the snapshots stay unarmed; a restore skips and journals) and journal `ledger-read-degraded` naming the site; the two reads that precede a publish — this harvest's append and the isolated unit's carry into the main ledger — normally journal `ledger-read-refused`, raise an `ACTION REQUIRED` notice naming the ledger, and pause the run at `escalation`; a sweep's terminal post-merge harvest carry instead journals `sweep-bundle-close-refused` and pauses at `story-gate`, while its direct pre-terminal defer carry retains the engine route. Both routes leave the task's phase untouched, so `bmad-loop resume` after the hand repair retries the write — resume recovery replays the recorded session result where one exists (the dev and review legs) and otherwise re-drives the leg (the unlatched review-timeout salvage and fix legs) — rather than, as `_escalate` would, demanding a `bmad-loop resolve` session and a clean rebuild over a fault that is not the story's. Bare, the first of those reads ended a story run as `run-crash` with the completed session's work on disk. The route also covers the window INSIDE each write, for decode faults (DW-259) and OS metadata/text-read faults (DW-279): every `deferredwork` mutator takes its own locked `read_for_write` — after the routed pre-read at the harvest and the harvest carry, after an observation snapshot at the commit-boundary close, and with no pre-read at all at the review-timeout salvage refile (`deferredwork.append_entry`) and the isolated close carry — so a `LedgerReadError` raised from the mutator call itself — the harvest's seen-again mark and append, the commit-boundary `closes_deferred:` close, the salvage refile, the isolated unit's harvest carry and close carry — pauses through its owning repair route under a site name ending in `-locked` (`spec-deferrals-harvest-mark-locked`, `spec-deferrals-harvest-append-locked`, `story-close-locked`, `review-timeout-salvage-refile-locked`, `harvest-carry-append-locked`, `story-close-carry-locked`); the locked read fires ahead of every write, so a pause there proves the mutator wrote nothing, which is what lets the commit-boundary close disarm its rollback first rather than journal a `deferred-close-rollback-failed` against bytes it cannot read. The notice names both kinds of write (findings to file, a declared close to record); `bmad-loop resume` re-drives the close through the COMMITTING arm, and a pending salvage refile through its persisted retry latch (DW-278): resume verifies the preserved product again, refiles the outstanding follow-up, and commits without new dev/review sessions or additional attempt/cycle charges under either rollback policy; ordinary commit gates still run, including any configured `pre_commit_gate` workflow sessions. An unrepaired ledger pauses again with the latch retained; a failed salvage verification follows the usual retry/exhaust routing. Successful refile records the publication while retaining recovery authority through notification and commit gates — the latch is set at every salvage's handoff save, the first fault-free one included, not only after a repair pause, so a host lost between that save and the commit replays the salvage rather than restarting it; the durable COMMITTING transition clears the latch. Legacy and unlatched timeouts keep the baseline restart or manual recovery behavior governed by `scm.rollback_on_failure`. The replay is the story engine's: a sweep bundle's recovery (`_recover_inflight_bundle`) has no session-replay arm at all, so a latched bundle restarts as every other post-session bundle does and its restart clears the latch, so the abandoned product's salvage cannot force a review on the replacement attempt. `LedgerReadFault(LedgerReadError)` wraps OS metadata/text-read failures with the original `OSError` as `__cause__`, so the locked-read catches cover them without catching lock/write failures. Pre-lock presence probes, lock acquisition and writes keep their raw `OSError` behavior. A read the OS refuses (EACCES, EIO, a symlink cycle) is routed the same way at the same four sites (DW-258): the observation reads degrade to a typed answer that carries NO digest — nothing is read from a ledger the OS refused — so the proof-of-work digest becomes an `` sentinel and the attribution compare treats a sentinel on either side as UNKNOWN, which is never credited: the ledger path stays excluded from proof of work, so the engine's own harvest append after the hand repair cannot pass a session that wrote nothing (a session whose only work was a ledger edit over a refused baseline is rolled back and retried over a readable one); the publish reads pause with the same `ACTION REQUIRED` notice, whose repair sentence now names both repairs (valid UTF-8; the path's permissions or storage). Where no publish read is reached — a spec with no findings — the story completes over the refused ledger, and a declared `closes_deferred:` then journals `deferred-close-ledger-unavailable` and sends a best-effort notice through the configured notification channels (including ATTENTION when file notifications are enabled), naming the story, every unapplied declared ID, and the fault (DW-277). The snapshot outage writes nothing to the ledger and does not pause or crash the story. Pre-rename primitives and the attended `bmad-build` still append flat `- source_spec:` blocks directly, which `sweep --migrate` normalizes. - Story-declared closure (`closes_deferred: [DW-5, DW-6]`, human-authored on a `stories.yaml` entry or in a story spec's frontmatter — the two are unioned): when the story commits, each declared entry flips to `status: done ` + `resolution: resolved by story ` — the annotation a sweep bundle writes — so the ledger stops being one-way. Written at the commit boundary, so an in-repo ledger carries the annotation in the story's own commit and a story that fails, is rejected by review, or escalates closes nothing. Declared, never inferred from a diff; re-read at the commit so a declaration edited after implementation still counts; idempotent across a resume; an unknown id, an unreadable entry status or a non-list declaration in a story spec is journaled, never fatal, and `bmad-loop validate` warns about all of them before the run starts. (A non-list `closes_deferred` in `stories.yaml` is different: the manifest is a schema the parser owns, so it is refused outright, before the run.) An artifact dir outside the repo cannot be committed — the annotation is written anyway and journaled (`deferred-close-external-ledger`). If the advisory ledger snapshot cannot be read, the declared closes remain unapplied and open entries stay open; the outage is journaled and notified as above. Restore ledger readability (valid UTF-8 and accessible permissions or storage), then run `bmad-loop sweep`: the next sweep re-triages those IDs against the actual code and can close verified resolutions as `already_resolved`, with the completed story's commit serving as evidence. diff --git a/src/bmad_loop/verify.py b/src/bmad_loop/verify.py index 2ad93fb25..5cfed0fe2 100644 --- a/src/bmad_loop/verify.py +++ b/src/bmad_loop/verify.py @@ -10,6 +10,7 @@ import hashlib import locale import os +import queue import re import shlex import shutil @@ -17,6 +18,8 @@ import subprocess import sys import tempfile +import threading +import time from collections.abc import Callable, Collection, Iterable from dataclasses import dataclass from pathlib import Path, PurePosixPath, PureWindowsPath @@ -134,6 +137,20 @@ class GitTimeoutError(GitError): operator anything.""" +class _GitCommitIndeterminate(GitError): + """A prepared ref transaction may have committed but lost its acknowledgement.""" + + +@dataclass(frozen=True) +class _PreparedRefUpdate: + """One direct-ref CAS executed through ``git update-ref --stdin``.""" + + ref: str + new_oid: str + old_oid: str + validate_while_prepared: Callable[[float], None] + + class RollbackPreflightError(GitError): """Rollback cleanup paths could not be proven safe before mutation.""" @@ -1588,8 +1605,9 @@ def _run_git( *, env: dict[str, str] | None = ..., binary: Literal[False] = ..., - timeout_s: int | None = ..., + timeout_s: float | None = ..., input_data: None = ..., + prepared_update: None = ..., ) -> subprocess.CompletedProcess[str]: ... @@ -1600,19 +1618,34 @@ def _run_git( *, env: dict[str, str] | None = ..., binary: Literal[True], - timeout_s: int | None = ..., + timeout_s: float | None = ..., input_data: bytes | None = ..., + prepared_update: None = ..., ) -> subprocess.CompletedProcess[bytes]: ... +@overload +def _run_git( + cmd: list[str], + repo: Path, + *, + env: dict[str, str] | None = ..., + binary: Literal[False] = ..., + timeout_s: float | None = ..., + input_data: None = ..., + prepared_update: _PreparedRefUpdate, +) -> subprocess.CompletedProcess[str]: ... + + def _run_git( cmd: list[str], repo: Path, *, env: dict[str, str] | None = None, binary: bool = False, - timeout_s: int | None = None, + timeout_s: float | None = None, input_data: bytes | None = None, + prepared_update: _PreparedRefUpdate | None = None, ) -> subprocess.CompletedProcess[str] | subprocess.CompletedProcess[bytes]: """Sole spawn point for git subprocesses. Three failures are raised by `subprocess.run` *before* any return code exists — a timeout (#156), a @@ -1644,10 +1677,170 @@ def _run_git( inherited environment and any explicit `env` (the `_git_env` callers' throwaway `GIT_INDEX_FILE` / synthetic identity vars are preserved by the spread). + `prepared_update` is the one interactive mode. It owns the complete + ``update-ref --stdin`` process lifecycle: start, queue, prepare, the caller's + lock-held validation, commit/abort, bounded pipe reads, and termination. It + never returns or embeds the child's output, because ref names and object IDs + in that protocol are migration authority rather than operator diagnostics. + `timeout_s` overrides the module bound for this one call — the interactive callers' seam (#390): a TUI render or install's best-effort probe keeps its own short deadline while standing inside the chokepoint.""" effective_timeout_s = _git_timeout_s if timeout_s is None else timeout_s + child_env = {**(env if env is not None else os.environ), "LC_ALL": "C"} + + if prepared_update is not None: + if binary or input_data is not None: + raise ValueError("prepared git execution is text-only") + deadline = time.monotonic() + effective_timeout_s + proc: subprocess.Popen[str] | None = None + prepared = False + commit_attempted = False + + def remaining() -> float: + return max(0.0, deadline - time.monotonic()) + + def stop_child() -> bool: + if proc is None: + return True + if proc.poll() is None: + try: + proc.terminate() + proc.wait(timeout=min(1.0, remaining())) + except (OSError, subprocess.TimeoutExpired): + try: + proc.kill() + proc.wait(timeout=1.0) + except (OSError, subprocess.TimeoutExpired): + pass + for stream in (proc.stdin, proc.stdout, proc.stderr): + if stream is not None: + try: + stream.close() + except OSError: + pass + return proc.poll() is not None + + try: + try: + proc = subprocess.Popen( + cmd, + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + errors="replace", + env=child_env, + ) + except OSError as exc: + raise GitSpawnError(f"git {cmd[3]} failed to spawn in {repo}") from exc + assert proc.stdin is not None + assert proc.stdout is not None + assert proc.stderr is not None + child_stdin = proc.stdin + child_stdout = proc.stdout + child_stderr = proc.stderr + + responses: queue.Queue[str | None] = queue.Queue() + + def read_responses() -> None: + try: + for line in child_stdout: + responses.put(line) + finally: + responses.put(None) + + def discard_stderr() -> None: + try: + while child_stderr.read(8192): + pass + except (OSError, ValueError): + pass + + threading.Thread(target=read_responses, daemon=True).start() + threading.Thread(target=discard_stderr, daemon=True).start() + + def send(line: str, *, begins_commit: bool = False) -> None: + nonlocal commit_attempted + if remaining() <= 0: + raise GitTimeoutError( + f"git update-ref timed out after {effective_timeout_s}s in {repo}" + ) + if begins_commit: + # A broken pipe after this point cannot prove whether Git read + # the command. Observation/replay, never rollback, decides. + commit_attempted = True + child_stdin.write(line) + child_stdin.flush() + + def expect(label: str) -> None: + try: + response = responses.get(timeout=remaining()) + except queue.Empty as exc: + raise GitTimeoutError( + f"git update-ref timed out after {effective_timeout_s}s in {repo}" + ) from exc + if response != f"{label}: ok\n": + raise GitError(f"git prepared ref transaction failed in {repo}") + + send("start\n") + expect("start") + send("option no-deref\n") + send( + f"update {prepared_update.ref} {prepared_update.new_oid} " + f"{prepared_update.old_oid}\n" + ) + send("prepare\n") + expect("prepare") + prepared = True + prepared_update.validate_while_prepared(remaining()) + send("commit\n", begins_commit=True) + expect("commit") + child_stdin.close() + try: + proc.wait(timeout=remaining()) + except subprocess.TimeoutExpired as exc: + raise GitTimeoutError( + f"git update-ref timed out after {effective_timeout_s}s in {repo}" + ) from exc + if proc.returncode != 0: + raise GitError(f"git prepared ref transaction failed in {repo}") + return subprocess.CompletedProcess(cmd, 0, "", "") + except BaseException as exc: + if prepared and not commit_attempted and proc is not None and proc.poll() is None: + try: + send("abort\n") + expect("abort") + child_stdin.close() + proc.wait(timeout=remaining()) + except BaseException as abort_exc: + stop_child() + if not isinstance(exc, GitError): + raise exc + raise GitError( + f"git prepared ref transaction abort failed in {repo}" + ) from abort_exc + stopped = stop_child() + if isinstance(exc, (KeyboardInterrupt, SystemExit)): + raise + if not stopped: + if not isinstance( + exc, (GitError, BrokenPipeError, OSError, UnicodeError, ValueError) + ): + raise + raise GitError( + f"git prepared ref transaction process did not terminate in {repo}" + ) from exc + if commit_attempted: + raise _GitCommitIndeterminate( + f"git prepared ref transaction acknowledgement was lost in {repo}" + ) from exc + if isinstance(exc, (BrokenPipeError, OSError, UnicodeError, ValueError)): + raise GitError(f"git prepared ref transaction failed in {repo}") from exc + raise + finally: + stop_child() + try: return subprocess.run( cmd, @@ -1655,7 +1848,7 @@ def _run_git( text=not binary and input_data is None, input=input_data, timeout=effective_timeout_s, - env={**(env if env is not None else os.environ), "LC_ALL": "C"}, + env=child_env, ) except subprocess.TimeoutExpired as exc: raise GitTimeoutError( @@ -9694,6 +9887,117 @@ def _bound_live_ledger_identity( return current +@dataclass(frozen=True) +class _BoundCheckoutIdentity: + immediate_ref: str | None + terminal_ref: str | None + oid: str + + +_BOUND_IDENTITY_PROBE_LIMIT = 3 +_BOUND_INDEX_RECONCILE_LIMIT = 3 + + +def _bound_symbolic_ref( + repo: Path, ref: str, *, recurse: bool, required: bool = True +) -> str | None: + args = ["symbolic-ref", "--quiet"] + if not recurse: + args.append("--no-recurse") + args.append(ref) + rc, value, _detail = _git_out(repo, *args) + if rc == 1: + if required: + raise GitError("exact-path publication requires an attached direct branch") + return None + if rc != 0 or not value: + raise GitError("exact-path publication branch identity could not be validated") + return value + + +def _bound_direct_ref_probe(repo: Path, ref: str, *, timeout_s: float | None = None) -> None: + proc = _run_git( + ["git", "-C", str(repo), "symbolic-ref", "--quiet", "--no-recurse", ref], + repo, + timeout_s=timeout_s, + ) + rc = proc.returncode + if rc == 0: + raise GitError("exact-path publication branch changed ref kind") + if rc != 1: + raise GitError("exact-path publication branch kind could not be validated") + + +def _bound_head_oid(repo: Path) -> str: + try: + return rev_parse_head(repo) + except GitError as exc: + raise GitError("exact-path publication branch value could not be validated") from exc + + +def _bound_checkout_identity(repo: Path, *, require_branch: bool = True) -> _BoundCheckoutIdentity: + """Capture one stable checkout identity without disclosing its ref names.""" + for _attempt in range(_BOUND_IDENTITY_PROBE_LIMIT): + immediate = _bound_symbolic_ref(repo, "HEAD", recurse=False, required=require_branch) + terminal = _bound_symbolic_ref(repo, "HEAD", recurse=True, required=require_branch) + if (immediate is None) != (terminal is None): + continue + if terminal is not None and not terminal.startswith("refs/heads/"): + raise GitError("exact-path publication requires a terminal branch") + if terminal is not None: + _bound_direct_ref_probe(repo, terminal) + oid = _bound_head_oid(repo) + if ( + _bound_symbolic_ref(repo, "HEAD", recurse=False, required=require_branch) == immediate + and _bound_symbolic_ref(repo, "HEAD", recurse=True, required=require_branch) == terminal + and _bound_head_oid(repo) == oid + ): + return _BoundCheckoutIdentity(immediate, terminal, oid) + raise GitError("exact-path publication checkout changed during identity capture") + + +def _bound_ref_oid(repo: Path, ref: str) -> str: + _bound_direct_ref_probe(repo, ref) + rc, oid, _detail = _git_out(repo, "rev-parse", "--verify", f"{ref}^{{commit}}") + if rc != 0 or not oid: + raise GitError("exact-path publication branch value could not be validated") + _bound_direct_ref_probe(repo, ref) + return oid + + +@dataclass(frozen=True) +class _BoundGitEntry: + mode: str + kind: str + oid: str + + +def _bound_tree_entry(repo: Path, revision: str, rel: str) -> _BoundGitEntry | None: + try: + entry = _entry_at_revision(repo, revision, rel) + except GitError as exc: + raise GitError(f"committed publication target could not be observed in {repo}") from exc + if entry is None: + return None + return _BoundGitEntry(*entry) + + +def _bound_tree_blob(repo: Path, revision: str, rel: str) -> str | None: + entry = _bound_tree_entry(repo, revision, rel) + if entry is None: + return None + if entry.kind != "blob" or entry.mode not in {"100644", "100755"}: + raise GitError("committed publication target is not a regular file") + return entry.oid + + +def _preflight_bound_tree_blob( + current_oid: str | None, baseline_oid: str, accepted_oid: str +) -> None: + if current_oid not in (None, baseline_oid, accepted_oid): + raise GitError("committed publication target holds rival content") + + def _bound_changed_paths(repo: Path, parent: str, revision: str) -> set[str]: proc = git_bytes( repo, @@ -9743,14 +10047,21 @@ def _validate_bound_candidate( raise _BoundCandidateMismatch( "exact-path candidate changed paths outside its declared scope" ) - committed = revision_blob_oids(repo, revision, (rel,)) - if committed.get(rel) != accepted_oid: + committed = _bound_tree_entry(repo, revision, rel) + if committed is None or committed.kind != "blob" or committed.oid != accepted_oid: raise _BoundCandidateMismatch("exact-path candidate does not contain the accepted ledger") - parent_blob = revision_blob_oids(repo, parent, (rel,)).get(rel) - if parent_blob not in (None, baseline_oid): + parent_entry = _bound_tree_entry(repo, parent, rel) + if parent_entry is not None and ( + parent_entry.kind != "blob" + or parent_entry.mode not in {"100644", "100755"} + or parent_entry.oid != baseline_oid + ): raise _BoundCandidateMismatch( "exact-path candidate parent does not contain the accepted baseline" ) + expected_mode = "100644" if parent_entry is None else parent_entry.mode + if committed.mode != expected_mode: + raise _BoundCandidateMismatch("exact-path candidate changed the publication target mode") def _accepted_bound_transition( @@ -9786,27 +10097,133 @@ def _accepted_bound_transition( return None -def _bound_index_oid(repo: Path, rel: str) -> str | None: - return staged_blob_oids(repo, (rel,)).get(rel) +def _bound_index_entry(repo: Path, rel: str) -> _BoundGitEntry | None: + try: + proc = git_bytes(repo, "ls-files", "-s", "-z", "--", *_literal_specs([rel])) + except GitError as exc: + raise GitError(f"publication target index could not be observed in {repo}") from exc + if proc.returncode != 0: + raise GitError(f"publication target index could not be observed in {repo}") + records = [record for record in proc.stdout.split(b"\0") if record] + if not records: + return None + if len(records) != 1 or b"\t" not in records[0]: + raise GitError("publication target index is not one exact entry") + header, actual_path = records[0].split(b"\t", 1) + if actual_path != os.fsencode(rel): + raise GitError("publication target index is not one exact entry") + try: + mode, oid, stage = header.decode("ascii", "strict").split() + except (UnicodeDecodeError, ValueError) as exc: + raise GitError("publication target index evidence is malformed") from exc + if stage != "0" or not re.fullmatch(r"[0-9a-f]{40}|[0-9a-f]{64}", oid): + raise GitError("publication target index is not an unambiguous stage-zero entry") + kind = "commit" if mode == "160000" else "blob" + return _BoundGitEntry(mode, kind, oid) def _synchronize_bound_index( repo: Path, - head: str, + expected_checkout: _BoundCheckoutIdentity, rel: str, - accepted_oid: str, - observed_index_oid: str | None, + observed_index_entry: _BoundGitEntry | None, ) -> None: - # A target-local reset preserves every unrelated real-index entry. Refuse - # an observed same-path writer instead of overwriting it between validation - # and housekeeping; the published commit remains authoritative and replayable. - if _bound_index_oid(repo, rel) != observed_index_oid: + """Align only the target entry to a stably observed checkout tree. + + A checkout can move independently of the captured publication branch. Each + reset is therefore bracketed by a complete object-plus-ref observation. A + move is repaired toward the newest observation but still refuses the attempt, + leaving commit-only replay to decide authority. The explicit bound prevents + a hostile ref mover from turning housekeeping into a livelock. + """ + expected_index_entry = observed_index_entry + moved = False + newest = _bound_checkout_identity(repo, require_branch=False) + if newest != expected_checkout: + moved = True + + for _attempt in range(_BOUND_INDEX_RECONCILE_LIMIT): + if _bound_index_entry(repo, rel) != expected_index_entry: + raise GitError("real index target changed during exact-path publication") + target = newest + target_entry = _bound_tree_entry(repo, target.oid, rel) + if target_entry is not None and target_entry.kind == "tree": + raise GitError("committed publication target became a directory") + rc, _out = _git(repo, "reset", target.oid, "--", *_literal_specs([rel])) + if rc != 0: + raise GitError(f"git target-local index synchronization failed in {repo}") + expected_index_entry = target_entry + if _bound_index_entry(repo, rel) != target_entry: + raise GitError("real index target synchronization did not match committed content") + newest = _bound_checkout_identity(repo, require_branch=False) + if newest == target: + if moved: + raise GitError("checkout changed during target index reconciliation") + return + moved = True + + # One final target-local repair makes the index correspond to the latest + # observation even when the movement never settled inside the retry bound. + if _bound_index_entry(repo, rel) != expected_index_entry: raise GitError("real index target changed during exact-path publication") - rc, out = _git(repo, "reset", head, "--", *_literal_specs([rel])) + newest_entry = _bound_tree_entry(repo, newest.oid, rel) + if newest_entry is not None and newest_entry.kind == "tree": + raise GitError("committed publication target became a directory") + rc, _out = _git(repo, "reset", newest.oid, "--", *_literal_specs([rel])) if rc != 0: - raise GitError(f"git target-local index synchronization failed in {repo}: {out}") - if _bound_index_oid(repo, rel) != accepted_oid: - raise GitError("real index target synchronization did not retain accepted content") + raise GitError(f"git target-local index synchronization failed in {repo}") + if _bound_index_entry(repo, rel) != newest_entry: + raise GitError("real index target synchronization did not match committed content") + raise GitError("checkout did not stabilize during target index reconciliation") + + +def _publish_bound_candidate( + repo: Path, + captured: _BoundCheckoutIdentity, + candidate: str, + rel: str, + accepted_oid: str, + baseline_oid: str, +) -> None: + terminal_ref = captured.terminal_ref + assert terminal_ref is not None + + def validate_terminal_kind(remaining_s: float) -> None: + _bound_direct_ref_probe( + repo, + terminal_ref, + timeout_s=remaining_s, + ) + + update = _PreparedRefUpdate( + ref=terminal_ref, + new_oid=candidate, + old_oid=captured.oid, + validate_while_prepared=validate_terminal_kind, + ) + try: + _run_git( + ["git", "-C", str(repo), "update-ref", "--stdin"], + repo, + prepared_update=update, + ) + except _GitCommitIndeterminate as exc: + observed = _bound_ref_oid(repo, terminal_ref) + if observed == candidate: + _validate_bound_candidate( + repo, + candidate, + captured.oid, + rel, + accepted_oid, + baseline_oid, + ) + return + if observed == captured.oid: + raise GitError( + "exact-path publication acknowledgement was lost before ref movement" + ) from exc + raise GitError("captured branch changed during exact-path publication") from exc def commit_path_bound( @@ -9823,9 +10240,10 @@ def commit_path_bound( The candidate is committed in a detached temporary worktree, so ordinary Git hooks run without moving the authoritative checkout. Its parent, exact path delta, Git-clean-filtered blob, live decoded text, and path identity are all - validated before an expected-old ``HEAD`` update-ref publishes it. Once that - CAS succeeds, target-only real-index reconciliation is replayable housekeeping: - no later fault rolls the truthful commit back. + validated before a prepared transaction publishes it to the originally + captured terminal direct branch. Once that transaction commits, target-only + real-index reconciliation is replayable housekeeping: no later fault rolls the + truthful commit back. """ try: rc, top, _detail = _git_out(repo, "rev-parse", "--show-toplevel") @@ -9842,76 +10260,119 @@ def commit_path_bound( lexical = live_path if live_path is not None else path accepted_bytes = accepted_text.encode("utf-8") baseline_bytes = baseline_text.encode("utf-8") - accepted_oid = git_normalized_blob_oid_for_bytes(repo_root, rel, accepted_bytes) - baseline_oid = git_normalized_blob_oid_for_bytes(repo_root, rel, baseline_bytes) + try: + accepted_oid = git_normalized_blob_oid_for_bytes(repo_root, rel, accepted_bytes) + baseline_oid = git_normalized_blob_oid_for_bytes(repo_root, rel, baseline_bytes) + except GitError as exc: + raise GitError("publication target content could not be normalized by Git") from exc identity = _bound_live_ledger_identity(lexical, target, accepted_text) - head = rev_parse_head(repo_root) - - accepted = _accepted_bound_transition(repo_root, head, rel, accepted_oid, baseline_oid) - authority_parent = _bound_parent(repo_root, accepted) if accepted is not None else head - head_blob = revision_blob_oids(repo_root, head, (rel,)).get(rel) - parent_has_target = rel in revision_blob_oids(repo_root, authority_parent, (rel,)) - observed_index_oid = _bound_index_oid(repo_root, rel) - allowed_index_oids: set[str | None] = {accepted_oid, baseline_oid} + captured = _bound_checkout_identity(repo_root) + + head_entry = _bound_tree_entry(repo_root, captured.oid, rel) + head_blob = _bound_tree_blob(repo_root, captured.oid, rel) + _preflight_bound_tree_blob(head_blob, baseline_oid, accepted_oid) + accepted = ( + _accepted_bound_transition(repo_root, captured.oid, rel, accepted_oid, baseline_oid) + if head_blob == accepted_oid + else None + ) + authority_parent = _bound_parent(repo_root, accepted) if accepted is not None else captured.oid + parent_has_target = _bound_tree_entry(repo_root, authority_parent, rel) is not None + observed_index_entry = _bound_index_entry(repo_root, rel) + expected_mode = "100644" if head_entry is None else head_entry.mode + allowed_index_entries: set[_BoundGitEntry | None] = { + _BoundGitEntry(expected_mode, "blob", accepted_oid), + _BoundGitEntry(expected_mode, "blob", baseline_oid), + } if not parent_has_target: # Original absence is the one accepted no-entry shape. A tracked # parent's absent real-index entry is a foreign staged deletion and is # refused rather than silently re-added. - allowed_index_oids.add(None) - if observed_index_oid not in allowed_index_oids: + allowed_index_entries.add(None) + if observed_index_entry not in allowed_index_entries: raise GitError("real index holds foreign content at the publication target") if accepted is not None: _bound_live_ledger_identity(lexical, target, accepted_text, identity=identity) - _synchronize_bound_index(repo_root, head, rel, accepted_oid, observed_index_oid) + _synchronize_bound_index(repo_root, captured, rel, observed_index_entry) return accepted # Preserve generic clean/ignored behavior when no migration transition needs # replay. In particular, an ignored ledger never earns a synthetic commit. - clean = path_clean(repo_root, rel) - ignored_untracked = clean and head_blob is None and path_ignored(repo_root, target) + try: + clean = path_clean(repo_root, rel) + ignored_untracked = clean and head_blob is None and path_ignored(repo_root, target) + except GitError as exc: + raise GitError("publication target cleanliness could not be validated") from exc if clean and (head_blob == accepted_oid or ignored_untracked): _bound_live_ledger_identity(lexical, target, accepted_text, identity=identity) return None active_error: BaseException | None = None candidate: str | None = None + try: + has_non_tree_parent = path_has_non_tree_ancestor_at_revision(repo_root, captured.oid, rel) + except GitError as exc: + raise GitError("candidate publication parent shape could not be validated") from exc + if has_non_tree_parent: + raise GitError("candidate publication path has a non-directory committed parent") with tempfile.TemporaryDirectory() as td: candidate_root = Path(td) / "candidate" - rc, out = _git( + rc, _out = _git( repo_root, "worktree", "add", "--detach", str(candidate_root), - head, + captured.oid, ) if rc != 0: - raise GitError(f"git detached candidate checkout failed in {repo_root}: {out}") + raise GitError(f"git detached candidate checkout failed in {repo_root}") try: candidate_path = candidate_root / rel - candidate_path.parent.mkdir(parents=True, exist_ok=True) - candidate_path.write_bytes(accepted_bytes) - rc, out = _git(candidate_root, "add", "--", *_literal_specs([rel])) + try: + candidate_path.parent.mkdir(parents=True, exist_ok=True) + candidate_path.write_bytes(accepted_bytes) + except (OSError, RuntimeError, ValueError) as exc: + raise GitError("exact-path candidate content could not be written") from exc + rc, _out = _git(candidate_root, "add", "--", *_literal_specs([rel])) if rc != 0: - raise GitError(f"git exact-path candidate staging failed in {repo_root}: {out}") - if staged_blob_oid(candidate_root, rel) != accepted_oid: + raise GitError(f"git exact-path candidate staging failed in {repo_root}") + staged_entry = _bound_index_entry(candidate_root, rel) + if staged_entry != _BoundGitEntry(expected_mode, "blob", accepted_oid): raise GitError("exact-path candidate staging changed accepted content") _bound_live_ledger_identity(lexical, target, accepted_text, identity=identity) - rc, out = _git(candidate_root, "commit", "-m", message) + if _bound_checkout_identity(repo_root) != captured: + raise GitError("checkout changed before exact-path candidate hooks") + rc, _out = _git(candidate_root, "commit", "-m", message) if rc != 0: - raise GitError(f"git exact-path candidate commit failed in {repo_root}: {out}") - candidate = rev_parse_head(candidate_root) - _validate_bound_candidate(repo_root, candidate, head, rel, accepted_oid, baseline_oid) + raise GitError(f"git exact-path candidate commit failed in {repo_root}") + try: + candidate = rev_parse_head(candidate_root) + except GitError as exc: + raise GitError("exact-path candidate identity could not be validated") from exc + _validate_bound_candidate( + repo_root, + candidate, + captured.oid, + rel, + accepted_oid, + baseline_oid, + ) _bound_live_ledger_identity(lexical, target, accepted_text, identity=identity) - rc, out = _git(repo_root, "update-ref", "HEAD", candidate, head) - if rc != 0: - raise GitError("authoritative HEAD changed during exact-path publication") + _publish_bound_candidate( + repo_root, + captured, + candidate, + rel, + accepted_oid, + baseline_oid, + ) except BaseException as exc: active_error = exc raise finally: - rc, out = _git( + rc, _out = _git( repo_root, "worktree", "remove", @@ -9919,11 +10380,14 @@ def commit_path_bound( str(candidate_root), ) if rc != 0 and active_error is None: - raise GitError(f"git detached candidate cleanup failed in {repo_root}: {out}") + raise GitError(f"git detached candidate cleanup failed in {repo_root}") assert candidate is not None - published_head = rev_parse_head(repo_root) - if published_head != candidate: - raise GitError("authoritative HEAD changed after exact-path publication") - _synchronize_bound_index(repo_root, published_head, rel, accepted_oid, observed_index_oid) + _bound_live_ledger_identity(lexical, target, accepted_text, identity=identity) + expected_checkout = _BoundCheckoutIdentity( + captured.immediate_ref, + captured.terminal_ref, + candidate, + ) + _synchronize_bound_index(repo_root, expected_checkout, rel, observed_index_entry) return candidate diff --git a/tests/test_sweep.py b/tests/test_sweep.py index 05da32ea1..c8fbbc991 100644 --- a/tests/test_sweep.py +++ b/tests/test_sweep.py @@ -31775,6 +31775,7 @@ def publish_then_rival(path, text, **kwargs): assert persisted.baseline_commit is None and persisted.baseline_untracked is None assert persisted.migration_recovery_format == 0 assert project.deferred_work.read_text(encoding="utf-8") == rival + assert _records(engine, "session-start") == [] assert list(engine.run_dir.glob("migrate-*")) == [] @@ -32005,6 +32006,44 @@ def inject_rival(git_repo, *args, **kwargs): assert project.deferred_work.read_text(encoding="utf-8") == accepted +def test_migration_prepared_publication_fault_is_sanitized_and_replayable(project, monkeypatch): + write_legacy_ledger(project, LEGACY_LEDGER) + mapping = _valid_migration_mapping() + accepted = migrated_ledger() + engine, first_adapter = make_sweep(project, [migrate_effect(project, accepted, mapping)]) + original_head = verify.rev_parse_head(project.project) + real_run_git = verify._run_git + faulted = [] + + def fail_prepared_transaction(cmd, git_repo, **kwargs): + if kwargs.get("prepared_update") is not None and not faulted: + faulted.append(True) + raise verify.GitError(f"git prepared ref transaction failed in {project.project}") + return real_run_git(cmd, git_repo, **kwargs) + + monkeypatch.setattr(verify, "_run_git", fail_prepared_transaction) + first = engine.run() + + assert first.crashed and len(first_adapter.sessions) == 1 + assert engine.state.tasks["sweep-migrate"].phase == Phase.COMMITTING + assert verify.rev_parse_head(project.project) == original_head + failures = _records(engine, "sweep-ledger-commit-unavailable") + assert len(failures) == 1 + assert failures[0]["error"] == f"git prepared ref transaction failed in {project.project}" + assert original_head not in failures[0]["error"] + assert accepted not in failures[0]["error"] + + monkeypatch.setattr(verify, "_run_git", real_run_git) + plan = triage_result(["DW-2"], skip=[{"id": "DW-2", "reason": "later"}]) + resumed, resumed_adapter = resume_sweep(project, engine, [triage_effect(plan)]) + second = resumed.run() + + assert not second.crashed and not second.paused + assert resumed.state.tasks["sweep-migrate"].phase == Phase.DONE + assert len(resumed_adapter.sessions) == 1 + assert "--migrate" not in resumed_adapter.sessions[0].prompt + + @pytest.mark.skipif(sys.platform == "win32", reason="POSIX symlinks") def test_migration_forwards_lexical_symlink_identity_to_bound_publisher(project, monkeypatch): write_legacy_ledger(project, LEGACY_LEDGER) diff --git a/tests/test_verify.py b/tests/test_verify.py index 3976d81d7..d9f396338 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -7,6 +7,7 @@ import os import subprocess import sys +import time from pathlib import Path import pytest @@ -8099,17 +8100,12 @@ def test_commit_path_bound_does_not_accept_a_false_clean_index_flag(project, ind assert git(repo, "show", f"{sha}:src.txt") == accepted.rstrip("\n") -def test_commit_path_bound_refuses_a_candidate_whose_parent_is_not_the_bound_baseline(project): +def test_commit_path_bound_refuses_current_tree_outside_the_bound_baseline(project): repo, path, _baseline, accepted = _bound_publish_inputs(project) original_head = verify.rev_parse_head(repo) wrong_baseline = "different claimed baseline\n" - shadow = repo / "wrong-baseline.txt" - shadow.write_text(wrong_baseline, encoding="utf-8") - wrong_oid = git(repo, "hash-object", "-w", "--", str(shadow)) - shadow.unlink() - git(repo, "update-index", "--cacheinfo", "100644", wrong_oid, path.name) - with pytest.raises(verify.GitError, match="parent.*accepted baseline"): + with pytest.raises(verify.GitError, match="committed publication target holds rival"): verify.commit_path_bound( repo, "chore: bound ledger", @@ -8211,23 +8207,23 @@ def test_commit_path_bound_refuses_a_tracked_target_index_deletion(project): assert git(repo, "ls-files", "--", path.name) == "" -def test_commit_path_bound_expected_old_cas_preserves_concurrent_head(project, monkeypatch): +def test_commit_path_bound_terminal_ref_cas_preserves_concurrent_head(project, monkeypatch): repo, path, baseline, accepted = _bound_publish_inputs(project) - real_git = verify._git + real_run_git = verify._run_git concurrent_head = [] - def advance_before_cas(git_repo, *args, **kwargs): - if args[:2] == ("update-ref", "HEAD") and not concurrent_head: + def advance_before_cas(cmd, git_repo, **kwargs): + if kwargs.get("prepared_update") is not None and not concurrent_head: rival = repo / "concurrent.txt" rival.write_text("keep concurrent commit\n", encoding="utf-8") git(repo, "add", "--", rival.name) git(repo, "commit", "-q", "-m", "concurrent commit") concurrent_head.append(verify.rev_parse_head(repo)) - return real_git(git_repo, *args, **kwargs) + return real_run_git(cmd, git_repo, **kwargs) - monkeypatch.setattr(verify, "_git", advance_before_cas) + monkeypatch.setattr(verify, "_run_git", advance_before_cas) - with pytest.raises(verify.GitError, match="HEAD changed"): + with pytest.raises(verify.GitError, match="prepared ref transaction failed"): verify.commit_path_bound( repo, "chore: bound ledger", @@ -8375,17 +8371,18 @@ def test_commit_path_bound_refuses_foreign_target_index_change_before_sync(proje shadow.write_text("foreign staged ledger\n", encoding="utf-8") foreign_oid = git(repo, "hash-object", "-w", "--", str(shadow)) shadow.unlink() - real_git = verify._git + real_run_git = verify._run_git published = [] - def change_index_after_cas(git_repo, *args, **kwargs): - result = real_git(git_repo, *args, **kwargs) - if args[:2] == ("update-ref", "HEAD") and result[0] == 0 and not published: - published.append(args[2]) + def change_index_after_cas(cmd, git_repo, **kwargs): + result = real_run_git(cmd, git_repo, **kwargs) + update = kwargs.get("prepared_update") + if update is not None and not published: + published.append(update.new_oid) git(repo, "update-index", "--cacheinfo", "100644", foreign_oid, path.name) return result - monkeypatch.setattr(verify, "_git", change_index_after_cas) + monkeypatch.setattr(verify, "_run_git", change_index_after_cas) with pytest.raises(verify.GitError, match="real index target changed"): verify.commit_path_bound( @@ -8463,6 +8460,692 @@ def substitute_after_staging(git_repo, *args, **kwargs): assert path.is_symlink() +def test_commit_path_bound_refuses_detached_head_before_candidate_hooks(project): + repo, path, baseline, accepted = _bound_publish_inputs(project) + original = verify.rev_parse_head(repo) + marker = repo / "bound-hook-ran" + hook = repo / ".git" / "hooks" / "pre-commit" + hook.write_text(f"#!/bin/sh\nprintf ran > '{marker}'\n") + hook.chmod(0o755) + git(repo, "checkout", "-q", "--detach") + + with pytest.raises(verify.GitError, match="attached direct branch"): + verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + + assert verify.rev_parse_head(repo) == original + assert not marker.exists() + + +def test_commit_path_bound_refuses_same_oid_checkout_switch_before_hooks(project, monkeypatch): + repo, path, baseline, accepted = _bound_publish_inputs(project) + original = verify.rev_parse_head(repo) + marker = repo / "bound-hook-ran" + hook = repo / ".git" / "hooks" / "pre-commit" + hook.write_text(f"#!/bin/sh\nprintf ran > '{marker}'\n") + hook.chmod(0o755) + real_identity = verify._bound_checkout_identity + captures = [] + + def switch_on_revalidation(git_repo): + if captures: + git(repo, "branch", "same-object-rival", original) + git(repo, "symbolic-ref", "HEAD", "refs/heads/same-object-rival") + captures.append(True) + return real_identity(git_repo) + + monkeypatch.setattr(verify, "_bound_checkout_identity", switch_on_revalidation) + + with pytest.raises(verify.GitError, match="checkout changed before.*hooks"): + verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + + assert verify.rev_parse_head(repo) == original + assert not marker.exists() + + +def test_commit_path_bound_publishes_terminal_branch_through_symbolic_chain(project): + repo, path, baseline, accepted = _bound_publish_inputs(project) + terminal = git(repo, "symbolic-ref", "HEAD") + alias = "refs/heads/publication-alias" + git(repo, "symbolic-ref", alias, terminal) + git(repo, "symbolic-ref", "HEAD", alias) + + published = verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + + assert git(repo, "symbolic-ref", "--no-recurse", "HEAD") == alias + assert git(repo, "symbolic-ref", "--no-recurse", alias) == terminal + assert git(repo, "rev-parse", terminal) == published + + +def test_commit_path_bound_lock_held_direct_ref_proof_is_load_bearing(project, monkeypatch): + repo, path, baseline, accepted = _bound_publish_inputs(project) + original = verify.rev_parse_head(repo) + real_probe = verify._bound_direct_ref_probe + probes = [] + + def fail_publication_probe(git_repo, ref, *, timeout_s=None): + probes.append(ref) + if len(probes) == 3: + raise verify.GitError("exact-path publication branch changed ref kind") + return real_probe(git_repo, ref, timeout_s=timeout_s) + + monkeypatch.setattr(verify, "_bound_direct_ref_probe", fail_publication_probe) + + with pytest.raises(verify.GitError, match="changed ref kind"): + verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + + assert verify.rev_parse_head(repo) == original + assert git(repo, "worktree", "list", "--porcelain").count("worktree ") == 1 + + +def test_commit_path_bound_refuses_terminal_ref_converted_to_same_oid_symref_before_prepare( + project, monkeypatch +): + repo, path, baseline, accepted = _bound_publish_inputs(project) + original = verify.rev_parse_head(repo) + terminal = git(repo, "symbolic-ref", "HEAD") + referent = "refs/heads/publication-referent" + git(repo, "branch", referent.removeprefix("refs/heads/"), original) + real_probe = verify._bound_direct_ref_probe + probes = [] + + def convert_terminal_inside_prepared_window(git_repo, ref, *, timeout_s=None): + probes.append(ref) + if len(probes) == 3: + terminal_path = repo / ".git" / terminal + terminal_path.write_text(f"ref: {referent}\n", encoding="ascii") + return real_probe(git_repo, ref, timeout_s=timeout_s) + + monkeypatch.setattr(verify, "_bound_direct_ref_probe", convert_terminal_inside_prepared_window) + + with pytest.raises(verify.GitError, match="changed ref kind"): + verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + + assert git(repo, "symbolic-ref", "--no-recurse", terminal) == referent + assert git(repo, "rev-parse", terminal) == original + assert git(repo, "rev-parse", referent) == original + + +def test_commit_path_bound_does_not_publish_new_head_selected_while_prepared(project, monkeypatch): + repo, path, baseline, accepted = _bound_publish_inputs(project) + original = verify.rev_parse_head(repo) + captured_branch = git(repo, "symbolic-ref", "HEAD") + rival_branch = "refs/heads/same-object-rival" + git(repo, "branch", rival_branch.removeprefix("refs/heads/"), original) + real_probe = verify._bound_direct_ref_probe + probes = [] + + def switch_head_inside_prepared_window(git_repo, ref, *, timeout_s=None): + probes.append(ref) + if len(probes) == 3: + (repo / ".git" / "HEAD").write_text(f"ref: {rival_branch}\n", encoding="ascii") + return real_probe(git_repo, ref, timeout_s=timeout_s) + + monkeypatch.setattr(verify, "_bound_direct_ref_probe", switch_head_inside_prepared_window) + + with pytest.raises(verify.GitError, match="checkout changed during.*reconciliation"): + verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + + assert git(repo, "rev-parse", rival_branch) == original + assert git(repo, "rev-parse", captured_branch) != original + assert verify.rev_parse_head(repo) == original + + +def test_commit_path_bound_repairs_index_after_detach_during_prepared(project, monkeypatch): + repo, path, baseline, accepted = _bound_publish_inputs(project) + original = verify.rev_parse_head(repo) + captured_branch = git(repo, "symbolic-ref", "HEAD") + original_index = verify._bound_index_entry(repo, path.name) + real_probe = verify._bound_direct_ref_probe + probes = [] + + def detach_head_inside_prepared_window(git_repo, ref, *, timeout_s=None): + probes.append(ref) + if len(probes) == 3: + (repo / ".git" / "HEAD").write_text(f"{original}\n", encoding="ascii") + return real_probe(git_repo, ref, timeout_s=timeout_s) + + monkeypatch.setattr(verify, "_bound_direct_ref_probe", detach_head_inside_prepared_window) + + with pytest.raises(verify.GitError, match="checkout changed during.*reconciliation"): + verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + + assert git(repo, "rev-parse", captured_branch) != original + assert verify.rev_parse_head(repo) == original + assert verify._bound_index_entry(repo, path.name) == original_index + + +def test_prepared_ref_transaction_redacts_protocol_authority(project): + repo = project.project + oid = verify.rev_parse_head(repo) + secret_ref = "refs/heads/secret ref" + update = verify._PreparedRefUpdate(secret_ref, oid, oid, lambda _remaining: None) + + with pytest.raises(verify.GitError) as raised: + verify._run_git( + ["git", "-C", str(repo), "update-ref", "--stdin"], + repo, + prepared_update=update, + ) + + evidence = str(raised.value) + assert secret_ref not in evidence + assert oid not in evidence + + +def test_prepared_ref_transaction_start_failure_changes_no_ref(project): + repo = project.project + oid = verify.rev_parse_head(repo) + ref = git(repo, "symbolic-ref", "HEAD") + update = verify._PreparedRefUpdate(ref, oid, oid, lambda _remaining: None) + + with pytest.raises(verify.GitError, match="prepared ref transaction failed"): + verify._run_git( + ["git", "-C", str(repo), "update-ref", "--stdin", "--invalid-option"], + repo, + prepared_update=update, + ) + + assert verify.rev_parse_head(repo) == oid + + +def test_prepared_ref_transaction_spawn_failure_is_typed_and_chained(project, monkeypatch): + repo = project.project + oid = verify.rev_parse_head(repo) + ref = git(repo, "symbolic-ref", "HEAD") + cause = OSError(errno.EMFILE, "injected descriptor exhaustion") + + def fail_spawn(*_args, **_kwargs): + raise cause + + monkeypatch.setattr(verify.subprocess, "Popen", fail_spawn) + update = verify._PreparedRefUpdate(ref, oid, oid, lambda _remaining: None) + with pytest.raises(verify.GitSpawnError) as raised: + verify._run_git( + ["git", "-C", str(repo), "update-ref", "--stdin"], + repo, + prepared_update=update, + ) + + assert raised.value.__cause__ is cause + + +def test_prepared_ref_transaction_propagates_non_git_validation_fault(project): + repo = project.project + oid = verify.rev_parse_head(repo) + ref = git(repo, "symbolic-ref", "HEAD") + fault = RuntimeError("non-git validation fault") + + def fail_validation(_remaining): + raise fault + + update = verify._PreparedRefUpdate(ref, oid, oid, fail_validation) + with pytest.raises(RuntimeError) as raised: + verify._run_git( + ["git", "-C", str(repo), "update-ref", "--stdin"], + repo, + prepared_update=update, + ) + + assert raised.value is fault + assert verify.rev_parse_head(repo) == oid + + +def test_prepared_ref_transaction_abort_failure_changes_no_ref(project, monkeypatch): + repo = project.project + oid = verify.rev_parse_head(repo) + ref = git(repo, "symbolic-ref", "HEAD") + real_popen = subprocess.Popen + children = [] + + def record_child(*args, **kwargs): + child = real_popen(*args, **kwargs) + children.append(child) + return child + + def break_abort(_remaining): + children[0].stdin.close() + raise verify.GitError("injected prepared validation fault") + + monkeypatch.setattr(verify.subprocess, "Popen", record_child) + update = verify._PreparedRefUpdate(ref, oid, oid, break_abort) + with pytest.raises(verify.GitError, match="transaction abort failed"): + verify._run_git( + ["git", "-C", str(repo), "update-ref", "--stdin"], + repo, + prepared_update=update, + ) + + assert verify.rev_parse_head(repo) == oid + + +def test_commit_path_bound_recovers_lost_commit_acknowledgement(project, monkeypatch): + repo, path, baseline, accepted = _bound_publish_inputs(project) + real_run_git = verify._run_git + faulted = [] + + def lose_ack_after_commit(cmd, git_repo, **kwargs): + result = real_run_git(cmd, git_repo, **kwargs) + if kwargs.get("prepared_update") is not None and not faulted: + faulted.append(True) + raise verify._GitCommitIndeterminate("injected acknowledgement loss") + return result + + monkeypatch.setattr(verify, "_run_git", lose_ack_after_commit) + published = verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + + assert faulted == [True] + assert published == verify.rev_parse_head(repo) + assert git(repo, "show", f"{published}:src.txt") == accepted.rstrip("\n") + + +def test_commit_path_bound_retries_after_lost_ack_with_unchanged_ref(project, monkeypatch): + repo, path, baseline, accepted = _bound_publish_inputs(project) + original = verify.rev_parse_head(repo) + real_run_git = verify._run_git + + def lose_ack_without_commit(cmd, git_repo, **kwargs): + if kwargs.get("prepared_update") is not None: + raise verify._GitCommitIndeterminate("injected acknowledgement loss") + return real_run_git(cmd, git_repo, **kwargs) + + monkeypatch.setattr(verify, "_run_git", lose_ack_without_commit) + with pytest.raises(verify.GitError, match="lost before ref movement"): + verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + assert verify.rev_parse_head(repo) == original + + monkeypatch.setattr(verify, "_run_git", real_run_git) + assert verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) == verify.rev_parse_head(repo) + + +def test_commit_path_bound_refuses_committed_rival_before_candidate_hooks(project): + repo, path, baseline, accepted = _bound_publish_inputs(project) + path.write_text("committed rival ledger\n", encoding="utf-8") + git(repo, "add", "--", path.name) + git(repo, "commit", "-q", "-m", "rival ledger") + rival_head = verify.rev_parse_head(repo) + path.write_text(accepted, encoding="utf-8") + marker = repo / "bound-hook-ran" + hook = repo / ".git" / "hooks" / "pre-commit" + hook.write_text(f"#!/bin/sh\nprintf ran > '{marker}'\n") + hook.chmod(0o755) + + with pytest.raises(verify.GitError, match="committed publication target holds rival"): + verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + + assert verify.rev_parse_head(repo) == rival_head + assert not marker.exists() + + +def test_commit_path_bound_repairs_index_to_moved_checkout_then_refuses(project, monkeypatch): + repo, path, baseline, accepted = _bound_publish_inputs(project) + original = verify.rev_parse_head(repo) + unrelated = repo / "operator.txt" + unrelated.write_text("staged operator work\n", encoding="utf-8") + git(repo, "add", "--", unrelated.name) + real_git = verify._git + moved = [] + + def move_head_during_first_reset(git_repo, *args, **kwargs): + result = real_git(git_repo, *args, **kwargs) + if args[:1] == ("reset",) and not moved: + git(repo, "branch", "index-race", original) + git(repo, "symbolic-ref", "HEAD", "refs/heads/index-race") + moved.append(True) + return result + + monkeypatch.setattr(verify, "_git", move_head_during_first_reset) + with pytest.raises(verify.GitError, match="checkout changed during.*reconciliation"): + verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + + assert moved == [True] + assert verify.rev_parse_head(repo) == original + assert verify.staged_blob_oid(repo, path.name) == git(repo, "rev-parse", f"{original}:src.txt") + assert git(repo, "diff", "--cached", "--name-only") == unrelated.name + + +def test_commit_path_bound_refuses_staged_mode_only_target_change(project): + repo, path, baseline, accepted = _bound_publish_inputs(project) + original = verify.rev_parse_head(repo) + git(repo, "update-index", "--chmod=+x", "--", path.name) + + with pytest.raises(verify.GitError, match="real index holds foreign content"): + verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + + assert verify.rev_parse_head(repo) == original + assert git(repo, "ls-files", "-s", "--", path.name).startswith("100755 ") + + +def test_commit_path_bound_preserves_tracked_executable_target_mode(project): + repo = project.project + path = repo / "src.txt" + git(repo, "update-index", "--chmod=+x", "--", path.name) + git(repo, "commit", "-q", "-m", "track executable ledger") + baseline = path.read_text(encoding="utf-8") + accepted = "accepted executable migration ledger\n" + path.write_text(accepted, encoding="utf-8") + + published = verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + + assert published == verify.rev_parse_head(repo) + assert git(repo, "ls-tree", published, "--", path.name).startswith("100755 blob ") + assert git(repo, "ls-files", "-s", "--", path.name).startswith("100755 ") + assert git(repo, "diff", "--cached", "--name-only") == "" + + +def test_commit_path_bound_rejects_hook_mode_mutation(project): + repo, path, baseline, accepted = _bound_publish_inputs(project) + original = verify.rev_parse_head(repo) + hook = repo / ".git" / "hooks" / "pre-commit" + hook.write_text("#!/bin/sh\ngit update-index --chmod=+x -- src.txt\n") + hook.chmod(0o755) + + with pytest.raises(verify.GitError, match="changed the publication target mode"): + verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + + assert verify.rev_parse_head(repo) == original + + +def test_commit_path_bound_refuses_committed_symlink_parent_before_candidate_write(project): + repo = project.project + parent = repo / "ledger-parent" + outside = repo / "outside-target" + outside.mkdir() + parent.symlink_to(outside.name) + git(repo, "add", "--", parent.name) + git(repo, "commit", "-q", "-m", "track ledger parent symlink") + parent.unlink() + parent.mkdir() + path = parent / "ledger.md" + accepted = "accepted migration ledger\n" + path.write_text(accepted, encoding="utf-8") + + with pytest.raises(verify.GitError, match="non-directory committed parent"): + verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text="legacy migration ledger\n", + ) + + assert not (outside / "ledger.md").exists() + + +def test_prepared_ref_timeout_before_commit_is_not_indeterminate(project): + repo = project.project + oid = verify.rev_parse_head(repo) + ref = git(repo, "symbolic-ref", "HEAD") + + def expire_deadline(_remaining): + time.sleep(0.02) + + update = verify._PreparedRefUpdate(ref, oid, oid, expire_deadline) + with pytest.raises(verify.GitError) as raised: + verify._run_git( + ["git", "-C", str(repo), "update-ref", "--stdin"], + repo, + timeout_s=0.01, + prepared_update=update, + ) + + assert not isinstance(raised.value, verify._GitCommitIndeterminate) + assert verify.rev_parse_head(repo) == oid + + +def test_commit_path_bound_recovers_post_commit_timeout(project, monkeypatch): + repo, path, baseline, accepted = _bound_publish_inputs(project) + hook = repo / ".git" / "hooks" / "reference-transaction" + hook.write_text( + "#!/bin/sh\n" + "if [ \"$1\" = committed ] && grep -q ' refs/heads/'; then\n" + " sleep 2\n" + "fi\n" + ) + hook.chmod(0o755) + monkeypatch.setattr(verify, "_git_timeout_s", 1) + + published = verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + + assert published == verify.rev_parse_head(repo) + assert git(repo, "show", f"{published}:src.txt") == accepted.rstrip("\n") + assert git(repo, "diff", "--cached", "--name-only") == "" + + +def test_commit_path_bound_uses_fractional_remaining_timeout_at_one_second(project, monkeypatch): + repo, path, baseline, accepted = _bound_publish_inputs(project) + hook = repo / ".git" / "hooks" / "reference-transaction" + hook.write_text( + "#!/bin/sh\n" + "if [ \"$1\" = prepared ] && grep -q ' refs/heads/'; then\n" + " sleep 0.2\n" + "fi\n" + ) + hook.chmod(0o755) + monkeypatch.setattr(verify, "_git_timeout_s", 1) + + published = verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + + assert published == verify.rev_parse_head(repo) + + +def test_commit_path_bound_recovers_real_git_commit_ack_loss(project): + if sys.platform == "win32": + pytest.skip("POSIX signal injection") + repo, path, baseline, accepted = _bound_publish_inputs(project) + hook = repo / ".git" / "hooks" / "reference-transaction" + hook.write_text( + "#!/bin/sh\n" + "if [ \"$1\" = committed ] && grep -q ' refs/heads/'; then\n" + ' kill -KILL "$PPID"\n' + "fi\n" + ) + hook.chmod(0o755) + + published = verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + + assert published == verify.rev_parse_head(repo) + assert git(repo, "show", f"{published}:src.txt") == accepted.rstrip("\n") + + +def test_commit_path_bound_refuses_rival_after_lost_commit_acknowledgement(project, monkeypatch): + repo, path, baseline, accepted = _bound_publish_inputs(project) + real_run_git = verify._run_git + rival_head = [] + + def replace_transaction_with_rival(cmd, git_repo, **kwargs): + if kwargs.get("prepared_update") is not None and not rival_head: + rival = repo / "rival.txt" + rival.write_text("rival branch commit\n", encoding="utf-8") + git(repo, "add", "--", rival.name) + git(repo, "commit", "-q", "-m", "rival branch commit") + rival_head.append(verify.rev_parse_head(repo)) + raise verify._GitCommitIndeterminate("injected acknowledgement loss") + return real_run_git(cmd, git_repo, **kwargs) + + monkeypatch.setattr(verify, "_run_git", replace_transaction_with_rival) + with pytest.raises(verify.GitError, match="captured branch changed"): + verify.commit_path_bound( + repo, + "chore: bound ledger", + path, + accepted_text=accepted, + baseline_text=baseline, + ) + + assert verify.rev_parse_head(repo) == rival_head[0] + + +def test_bound_index_reconciliation_is_bounded_and_repairs_latest_observation(project, monkeypatch): + repo = project.project + path = repo / "src.txt" + rel = path.name + commits = [] + for number in range(4): + path.write_text(f"moving target {number}\n", encoding="utf-8") + git(repo, "add", "--", rel) + git(repo, "commit", "-q", "-m", f"moving target {number}") + commits.append(verify.rev_parse_head(repo)) + unrelated = repo / "operator.txt" + unrelated.write_text("staged operator work\n", encoding="utf-8") + git(repo, "add", "--", unrelated.name) + observed_index = verify._bound_index_entry(repo, rel) + branch = git(repo, "symbolic-ref", "HEAD") + observations = [verify._BoundCheckoutIdentity(branch, branch, commit) for commit in commits] + + def keep_moving(_repo, *, require_branch=True): + assert require_branch is False + return observations.pop(0) + + monkeypatch.setattr(verify, "_bound_checkout_identity", keep_moving) + expected = verify._BoundCheckoutIdentity(branch, branch, commits[0]) + with pytest.raises(verify.GitError, match="checkout did not stabilize"): + verify._synchronize_bound_index(repo, expected, rel, observed_index) + + assert observations == [] + assert verify._bound_index_entry(repo, rel) == verify._bound_tree_entry(repo, commits[-1], rel) + assert git(repo, "diff", "--cached", "--name-only") == unrelated.name + + +def test_bound_index_reconciliation_can_align_a_moved_symlink_entry(project, monkeypatch): + repo = project.project + path = repo / "src.txt" + rel = path.name + original = verify.rev_parse_head(repo) + original_index = verify._bound_index_entry(repo, rel) + target = repo / "symlink-target.txt" + target.write_text("target\n", encoding="utf-8") + path.unlink() + path.symlink_to(target.name) + git(repo, "add", "--", rel) + git(repo, "commit", "-q", "-m", "move target to symlink") + symlink_commit = verify.rev_parse_head(repo) + git(repo, "reset", "--hard", original) + branch = git(repo, "symbolic-ref", "HEAD") + moved = verify._BoundCheckoutIdentity(branch, branch, symlink_commit) + observations = [moved, moved] + + def observe_moved(_repo, *, require_branch=True): + assert require_branch is False + return observations.pop(0) + + monkeypatch.setattr(verify, "_bound_checkout_identity", observe_moved) + expected = verify._BoundCheckoutIdentity(branch, branch, original) + with pytest.raises(verify.GitError, match="checkout changed during.*reconciliation"): + verify._synchronize_bound_index(repo, expected, rel, original_index) + + entry = verify._bound_index_entry(repo, rel) + assert entry is not None and entry.mode == "120000" + assert entry == verify._bound_tree_entry(repo, symlink_commit, rel) + + def test_stories_relpaths_follows_the_root_it_is_given(project, tmp_path): """Same rule for the stories-mode exclude: rooted where git runs.""" paths = _repo_root_override(project, tmp_path) From 2f4e64ea237a955df572c5c18049fea68d56d09a Mon Sep 17 00:00:00 2001 From: pbean Date: Wed, 16 Sep 2026 11:19:12 -0700 Subject: [PATCH 06/17] docs(changelog): wave5 S12 entries --- CHANGELOG.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 736f4b427..de8697d2e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -468,6 +468,23 @@ breaking changes may land in a minor release. ### Fixed +- Bind legacy migration dispatch and publication to the ledger bytes actually accepted + (DW-311, DW-316). Retire stale recovery authority when the ledger changes before + the true post-hook adapter-launch boundary, then publish a validated one-path, + clean-filter-normalized candidate through a prepared expected-old transaction on + the captured terminal direct branch. Resolve lost commit acknowledgements by + deterministic replay, and reconcile the target index against bounded stable checkout + observations while preserving unrelated stages. + +- Refuse no-descriptor attempt-owned spec restoration before staging or lifecycle + normalization, preserving the existing target bytes for manual recovery (DW-310). + +- Fail closed after no-descriptor attempt-owned spec publication and pause for + manual adoption instead of trusting a path-based readback (DW-309). + +- Refuse attempt-owned spec recovery when final prepublication validation observes an + existing target was edited in place, preserving the competing bytes (DW-308). + - Restore the accepted commit chain and index when the post-squash HEAD identity probe fails, retaining the probe fault if rollback also fails (DW-305). From c371dc865437b2205d0767b5da07b259cf4d53ac Mon Sep 17 00:00:00 2001 From: pbean Date: Wed, 16 Sep 2026 13:23:34 -0700 Subject: [PATCH 07/17] fix(verify): make wave5 S12 Windows-clean MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause (production, verify._run_git prepared_update mode): the `update-ref --stdin` child is spawned with text=True, whose TextIOWrapper (newline=None) writes os.linesep for "\n" — "\r\n" on Windows — so git read `start\r`, died with `unknown command`, and every prepared transaction raised "git prepared ref transaction failed". Pin newline="\n" on the command stream only (the reply stream keeps universal-newline reading). Reproduced on Linux by reconfiguring the wrapper to "\r\n" before the pin: git rc=128 unpinned, "start: ok" pinned. Cascades of that bug (recover with the fix, no test change): - tests/test_verify.py: the 19 test_commit_path_bound_* rows and the two test_prepared_ref_transaction_* rows (expect("start") failed first, so the match= regexes never saw their real fault). - tests/test_sweep.py: test_migration_normal_launch_runs_the_prelaunch_validator_once, test_migration_prepared_publication_fault_is_sanitized_and_replayable, test_migration_rival_during_bound_publication_replays_commit_only[before-/after-staging], test_migration_escalation_resume_retries, test_mixed_ledger_migration_preserves_canonical_open_set, test_only_revalidates_against_compacted_post_migration_ids, test_severity_selector_applies_to_the_post_migration_ledger, test_sweep_migrates_legacy_then_triages_and_runs_bundle (crash_error "migration ledger publication unavailable", phase stuck in COMMITTING). Separate cause A (DW-310 design, not the newline bug): on a host without descriptor-relative writes recovery_flow refuses attempt-owned spec restoration and lifecycle normalization and pauses for manual adoption, so a rollback that must put the bound spec back cannot converge on Windows. Confirmed on Linux by monkeypatching recovery_flow.DIR_FD_ANCHORED_WRITES=False over the whole engine suite: exactly these seven rows pause with "attempt-owned spec needs manual recovery". Mark them with the same `requires_descriptor_restoration` skip test_recovery_flow.py already uses: - tests/test_engine.py: test_bound_fixable_chain_restores_first_snapshot_before_fresh_retry[plain|resolved-redrive], test_fixable_retry_chain_snapshot_reaches_phase_baseline, test_intent_gap_restore_reapplies_after_mid_redrive_rollback, test_nonfixable_chain_rollback_rebases_ledger_proof_reference, test_resolved_redrive_owned_dirty_spec_routes_explicitly_and_converges, test_resolved_redrive_reescalates_instead_of_deferring. Separate cause B (core.autocrlf=true): safe_rollback's `preserve` round-trips the artifact folder through `git stash create` + `git checkout`, so the LF spec the refused restoration left alone reads back CRLF. Compare newline-normalized; the assertion still distinguishes the operator content from the snapshot. Reproduced and re-verified on Linux with `git config core.autocrlf true`. - tests/test_recovery_flow.py: test_latched_redrive_forced_fallback_pauses_after_completed_baseline_reset, test_resolved_cause_forced_fallback_pauses_after_completed_reset. --- src/bmad_loop/verify.py | 11 ++++++++++- tests/test_engine.py | 16 ++++++++++++++++ tests/test_recovery_flow.py | 13 +++++++++++-- 3 files changed, 37 insertions(+), 3 deletions(-) diff --git a/src/bmad_loop/verify.py b/src/bmad_loop/verify.py index 5cfed0fe2..e367cac13 100644 --- a/src/bmad_loop/verify.py +++ b/src/bmad_loop/verify.py @@ -8,6 +8,7 @@ from __future__ import annotations import hashlib +import io import locale import os import queue @@ -1734,9 +1735,17 @@ def stop_child() -> bool: ) except OSError as exc: raise GitSpawnError(f"git {cmd[3]} failed to spawn in {repo}") from exc - assert proc.stdin is not None + # `text=True` wraps every pipe in a `TextIOWrapper(newline=None)`, + # whose WRITE side translates "\n" to `os.linesep` — "\r\n" on + # Windows — so `update-ref --stdin` would read `start\r` and die with + # `unknown command` before acknowledging. The protocol's terminator is + # LF on every host, so pin it on the command stream only: the reply + # stream keeps universal-newline reading, which folds either ending + # into the `"