From 889d2c94cedb2cc6c8f2517cb7e6e4688a480df9 Mon Sep 17 00:00:00 2001 From: pbean Date: Thu, 9 Jul 2026 12:37:46 -0700 Subject: [PATCH 1/2] fix(rearm): exclude an abandoned restore's residue from the new baseline MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-arming a story whose previous re-drive had already applied a restore patch snapshotted that patch's new (untracked) files as pre-existing, so every later rollback preserved them and finalize_commit's `add -A` swept the abandoned attempt into the corrected story's commit. rearm_escalation now captures the old latch/baseline before the unconditional overwrite, parses the stale patch's creations (verify.patch_new_files) and subtracts them from the refreshed baseline_untracked. Nothing is deleted here — the re-drive's own reset removes what the snapshot stops blessing. Deliberately not a `git apply -R`: it fails on any drift and misbehaves on the committed variant. Commits the escalated attempt left below the advanced baseline share their range with the resolve session's own blessed commits, so they cannot be reverted mechanically: they are journaled and echoed to stderr for the human to classify. Best-effort throughout — a missing or unreadable patch degrades to the pre-fix behavior rather than wedging the resolve. Closes #90 --- CHANGELOG.md | 11 +++ src/bmad_loop/cli.py | 32 ++++++++ src/bmad_loop/runs.py | 92 +++++++++++++++++++++- src/bmad_loop/verify.py | 45 +++++++++++ tests/test_cli.py | 30 ++++++++ tests/test_portability_guard.py | 9 ++- tests/test_runs.py | 131 +++++++++++++++++++++++++++++++- tests/test_verify.py | 88 +++++++++++++++++++++ 8 files changed, 433 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ea8e4c5..06981de 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -31,6 +31,17 @@ breaking changes may land in a minor release. ### Fixed +- **An abandoned patch-restore no longer smuggles its files into the corrected story's commit.** + Re-arming a story whose previous re-drive had already applied a restore patch snapshotted that + patch's new (untracked) files as _pre-existing_, so every later rollback preserved them and + `finalize_commit`'s `add -A` swept the abandoned attempt into the corrected commit. The re-arm now + parses the old latch (`verify.patch_new_files`) and subtracts its creations from the refreshed + baseline snapshot — the re-drive's own reset then removes them. Best-effort: a missing or + unreadable patch degrades to the old behavior instead of failing the resolve. Commits the + escalated attempt left below the advanced baseline can't be reverted mechanically (the resolve + session's own commits share that range), so they are journaled and echoed to stderr for the human + to classify. New journal events: `stale-restore-excluded` / `-unparseable` / `-commits`. + (closes #90) - **Baseline-era untracked residue no longer vacuously satisfies the proof-of-work gate.** `has_changes_since` counted every untracked file. After an intent-gap halt the saved patch is untracked residue under the artifact dirs every reset deliberately protects, so a from-scratch diff --git a/src/bmad_loop/cli.py b/src/bmad_loop/cli.py index abda40a..3683d02 100644 --- a/src/bmad_loop/cli.py +++ b/src/bmad_loop/cli.py @@ -897,6 +897,36 @@ def _resolve_restore_patch( return str(patch), None +def _echo_stale_restore(run_dir: Path, seen_entries: int) -> None: + """Surface the `stale-restore-*` events a just-completed re-arm journaled about + the restore attempt it abandoned (runs._stale_restore_residue). The commits + variant is the one the human must act on — nothing else will.""" + for entry in Journal(run_dir).entries()[seen_entries:]: + kind = entry.get("kind", "") + if kind == "stale-restore-excluded": + files = ", ".join(entry.get("files", [])) + print( + f"note: excluded the abandoned restore's new files from the " + f"re-drive baseline: {files}", + file=sys.stderr, + ) + elif kind == "stale-restore-unparseable": + print( + f"warning: could not read the abandoned restore patch " + f"({entry.get('patch', '?')}) — its new files may be swept into the " + "next commit; check `git status` before resuming", + file=sys.stderr, + ) + elif kind == "stale-restore-commits": + n = len(entry.get("commits", [])) + print( + f"warning: {n} commit(s) sit below the re-drive's new baseline " + f"({entry.get('old_baseline', '?')[:12]}..) — if any came from the " + "abandoned attempt rather than your resolve, revert them now", + file=sys.stderr, + ) + + def cmd_resolve(args: argparse.Namespace) -> int: from .model import PAUSE_ESCALATION, Phase @@ -993,11 +1023,13 @@ def cmd_resolve(args: argparse.Namespace) -> int: if args.resume is None and not _confirm(f"re-arm {story_key} and resume run {args.run_id}?"): print("cancelled — run is still paused at the escalation") return 0 + seen_entries = len(Journal(run_dir).entries()) try: runs.rearm_escalation(run_dir, story_key, restore_patch=restore_patch) except runs.RearmError as e: print(f"error: {e}", file=sys.stderr) return 1 + _echo_stale_restore(run_dir, seen_entries) print( f"re-armed {story_key}" + (" (restoring the attempted change for review)" if restore_patch else "") diff --git a/src/bmad_loop/runs.py b/src/bmad_loop/runs.py index 0000140..65ab45e 100644 --- a/src/bmad_loop/runs.py +++ b/src/bmad_loop/runs.py @@ -593,6 +593,10 @@ def rearm_escalation( ) journal = Journal(run_dir) + # Read before the unconditional overwrite below: they describe the restore + # attempt this re-arm is abandoning, and the residue block needs both. + old_latch = task.restore_patch + old_baseline = task.baseline_commit # deliberate reset, not a normal state-machine transition (mirrors # engine._finish_inflight): a clean re-attempt against the corrected spec. task.phase = Phase.PENDING @@ -650,6 +654,21 @@ def rearm_escalation( f"(it must be readable UTF-8), then re-run resolve" ) from e + # A previous restore latch is being replaced (or re-latched onto the same + # patch): the abandoned attempt applied that patch, so its NEW files sit + # untracked in the tree right now. The refresh below would capture them as + # "pre-existing" — after which every rollback preserves them and + # finalize_commit's `add -A` sweeps the abandoned attempt into the corrected + # story's commit. Subtract them instead (issue #90). + # + # Runs after the spec block for the same reason the refresh does (a cleared + # sentinel must not be snapshotted), and before it because it feeds it. + # Nothing is deleted here: the re-drive's reset (verify.safe_rollback) removes + # whatever the refreshed snapshot no longer blesses, at the right moment. + stale_residue = _stale_restore_residue( + Path(state.project), journal, key, old_latch, old_baseline + ) + # Advance the attempt baseline to the project's current HEAD and refresh the # untracked snapshot: whatever the human-driven resolve session left on the # branch (a committed fixture, a corrected ledger, ...) is authorized input @@ -668,7 +687,7 @@ def rearm_escalation( try: repo = Path(state.project) head = verify.rev_parse_head(repo) - untracked = sorted(verify.untracked_files(repo)) + untracked = sorted(verify.untracked_files(repo) - stale_residue) task.baseline_commit = head task.baseline_untracked = untracked except Exception: # noqa: BLE001 # nosec B110 - best-effort git read, must not fail re-arm @@ -703,6 +722,77 @@ def rearm_escalation( return key +def _stale_restore_residue( + repo: Path, + journal: Journal, + story_key: str, + old_latch: str | None, + old_baseline: str | None, +) -> set[str]: + """The untracked files an abandoned patch-restore attempt left in the tree — + to be subtracted from the re-arm's refreshed `baseline_untracked` (issue #90). + + Empty when no restore was latched. Deliberately *not* a `git apply -R`: the + re-drive's own reset already reverts the patch's tracked hunks, an `apply -R` + fails outright on any drift the resolve session introduced, and it misbehaves + on the committed variant below. Only the patch's new files are durable + contamination, and naming them is enough — `verify.safe_rollback` deletes + whatever the refreshed snapshot stops blessing. + + Also journals (warn-only) the commits sitting between the OLD baseline and the + new one: a commit the escalated re-drive session made now becomes the next + re-drive's permanent starting point, and no reset revisits it. It is not + mechanically reversible — the resolve session's own blessed commits live in the + same range and reverting those would claw back the human's resolution — so the + human is the classifier. `bmad-loop resolve` echoes these to stderr. + + Best-effort throughout: a deleted or unreadable patch, a non-repo project, a + bad old baseline — none may wedge a resolve. Every failure degrades to the + pre-#90 behavior and says so in the journal. + """ + if not old_latch: + return set() + patch_path = Path(old_latch) + if not patch_path.is_absolute(): + patch_path = repo / patch_path + + residue: set[str] = set() + try: + residue = verify.patch_new_files(patch_path) + except (OSError, UnicodeDecodeError) as e: + # degrade to the pre-#90 snapshot rather than wedge the resolve + journal.append( + "stale-restore-unparseable", + story_key=story_key, + patch=str(patch_path), + error=f"{e.__class__.__name__}: {e}", + ) + else: + if residue: + journal.append( + "stale-restore-excluded", + story_key=story_key, + patch=str(patch_path), + files=sorted(residue), + ) + + # Independent of the parse above — an unreadable patch must not also cost the + # human the only notice they get about the committed variant. + if old_baseline: + try: + shas = verify.commits_above(repo, old_baseline) + except Exception: # noqa: BLE001 # nosec B110 - warn-only, must not fail re-arm + shas = [] + if shas: + journal.append( + "stale-restore-commits", + story_key=story_key, + old_baseline=old_baseline, + commits=shas, + ) + return residue + + def _sentinel_condition(spec_path: Path, story_key: str) -> str | None: """The blocking condition (``unresolved`` / ``ambiguous``) iff ``spec_path`` is a fixed-slug pre-planning-halt sentinel for ``story_key``, else None.""" diff --git a/src/bmad_loop/verify.py b/src/bmad_loop/verify.py index ecd2605..b131b5f 100644 --- a/src/bmad_loop/verify.py +++ b/src/bmad_loop/verify.py @@ -26,6 +26,11 @@ GIT_TIMEOUT_S = 120 COMMAND_TIMEOUT_S = 30 * 60 +# How git's own diff format names the absent side of a creation/deletion. A +# protocol token git emits verbatim on every platform, Windows included — never +# opened, never joined onto. Only `patch_new_files` reads it. +_DIFF_ABSENT = "/dev/null" # portability: git diff-format token, not a real path + # result.json `workflow` value for the dev pass. A machine contract: the # orchestrator forges this value in `devcontract` when synthesizing the dev # result from the spec the bmad-dev-auto session leaves on disk; a mismatch @@ -1550,6 +1555,46 @@ def apply_patch(repo: Path, patch_path: Path) -> None: raise GitError(f"git apply {patch_path} failed: {out}") +def patch_new_files(patch_path: Path) -> set[str]: + """Repo-relative posix paths the saved patch *creates* — the untracked residue + an `apply_patch` leaves behind (see `runs.rearm_escalation`). + + Text-parse, not `git apply --numstat`: the caller runs after the tree has moved + on, so the patch may no longer apply, and a creation list must still come back. + Within each `diff --git` block, an old-side `---` header naming `_DIFF_ABSENT` + marks a creation, and the `+++ b/` after it names the file. Deletions (the + absent token on the *new* side) are never returned — the caller feeds this to an + *exclusion* set, and excluding a path the human later re-created would make the + next rollback delete their file. For the same reason every ambiguous entry is + skipped rather than guessed: quoted paths (`+++ "b/wéird"`, core.quotePath), + renames, and non-`git diff` unified diffs with no `diff --git` header yield fewer + results, never wrong ones. Under-reporting degrades to the pre-#90 behavior; + over-reporting deletes user data. + + Raises OSError / UnicodeDecodeError when the patch cannot be read; the caller + decides (rearm treats it as best-effort and journals `stale-restore-unparseable`). + """ + new_files: set[str] = set() + in_hunk = False # past the first `@@`, a `--- x` line is content, not a header + creating = False + for line in patch_path.read_text(encoding="utf-8").splitlines(): + if line.startswith("diff --git "): + in_hunk = creating = False + elif line.startswith("@@"): + in_hunk = True + elif in_hunk: + continue + elif line.startswith("--- "): + creating = line[4:].strip() == _DIFF_ABSENT + elif line.startswith("+++ ") and creating: + creating = False + target = line[4:].split("\t", 1)[0].strip() + if target == _DIFF_ABSENT or target.startswith('"'): + continue # a delete-then-create pair, or a quoted path we won't guess + new_files.add(target[2:] if target.startswith("b/") else target) + return new_files + + def commit_paths(repo: Path, message: str, paths: list[Path]) -> str | None: """Commit exactly `paths` (and nothing else), leaving any unrelated working or staged changes untouched. Unlike commit_story's `add -A`, this is safe to diff --git a/tests/test_cli.py b/tests/test_cli.py index 1a4c3a2..0ca5667 100644 --- a/tests/test_cli.py +++ b/tests/test_cli.py @@ -929,6 +929,36 @@ def test_resolve_no_interactive_rearms_and_resumes(tmp_path, monkeypatch, capsys assert "ready-for-dev" in spec.read_text() +def test_resolve_echoes_this_rearms_stale_restore_events(tmp_path, monkeypatch, capsys): + """#90's journal entries reach the operator. The commits variant is warn-only — + stderr is the only place it ever surfaces. Entries from *earlier* re-arms are + already-acted-on history and must not be replayed.""" + from bmad_loop import runs + from bmad_loop.journal import Journal + + run_dir = _escalated_run(tmp_path, "r1") + Journal(run_dir).append("stale-restore-excluded", story_key="s1", files=["FROM-LAST-TIME.txt"]) + + def fake_rearm(rd, key, *, restore_patch=None): + journal = Journal(rd) + journal.append("stale-restore-excluded", story_key=key, patch="a.patch", files=["new.txt"]) + journal.append("stale-restore-unparseable", story_key=key, patch="b.patch", error="OSErr") + journal.append("stale-restore-commits", story_key=key, old_baseline="f" * 40, commits=["c"]) + return key + + monkeypatch.setattr(runs, "rearm_escalation", fake_rearm) + monkeypatch.setattr(cli, "_resume_paused_run", lambda proj, rd: 0) + assert ( + cli.main(["resolve", "--project", str(tmp_path), "r1", "--no-interactive", "--resume"]) == 0 + ) + + err = capsys.readouterr().err + assert "excluded the abandoned restore's new files from the re-drive baseline: new.txt" in err + assert "could not read the abandoned restore patch (b.patch)" in err + assert "1 commit(s) sit below the re-drive's new baseline (ffffffffffff..)" in err + assert "FROM-LAST-TIME.txt" not in err + + def test_resolve_interactive_runs_session_then_rearms(tmp_path, monkeypatch): from bmad_loop import resolve from bmad_loop.journal import load_state diff --git a/tests/test_portability_guard.py b/tests/test_portability_guard.py index 3c6268c..6025eac 100644 --- a/tests/test_portability_guard.py +++ b/tests/test_portability_guard.py @@ -31,13 +31,16 @@ # *are* the sanctioned spot (their module docstrings say so). TMUX_BACKENDS = {"adapters/tmux_base.py", "adapters/tmux_backend.py"} -# Platform-guarded files that may name a bare POSIX path, each on a line carrying -# a `# portability:` ack (and guarded by a sys.platform branch). process_host.py's -# Linux identity reader walks `/proc//stat`. +# Files that may name a bare POSIX path, each on a line carrying a `# portability:` +# ack. process_host.py's Linux identity reader walks `/proc//stat` behind a +# sys.platform branch; the Unity teardown scripts are POSIX-only. verify.py is the +# one non-platform case: git's *diff format* spells an absent file `/dev/null` on +# every platform, so `patch_new_files` compares against it as a protocol token. PATH_ALLOW = { "data/plugins/unity/unity_cleanup.py", "data/plugins/unity/unity_teardown.py", "process_host.py", + "verify.py", } # The two detach helpers that legitimately request POSIX `start_new_session`. diff --git a/tests/test_runs.py b/tests/test_runs.py index 8ce2351..cef6346 100644 --- a/tests/test_runs.py +++ b/tests/test_runs.py @@ -7,6 +7,7 @@ import tarfile import pytest +from conftest import git from bmad_loop import runs from bmad_loop.adapters import tmux_base @@ -530,11 +531,23 @@ def test_delete_run(tmp_path): assert not run_dir.exists() -def _escalated_run(tmp_path, spec_text, *, restore_patch_stale=None): +def _escalated_run(tmp_path, spec_text, *, restore_patch_stale=None, git_project=False): + """`git_project=True` makes `state.project` a real repo with the spec committed, + so rearm's baseline snapshot refresh actually runs — in a bare tmp_path its + best-effort `except` swallows every git call and the refresh silently no-ops.""" from bmad_loop.model import PAUSE_ESCALATION, Phase, StoryTask spec = tmp_path / "spec.md" spec.write_text(spec_text, encoding="utf-8") + baseline = None + if git_project: + (tmp_path / ".gitignore").write_text(".bmad-loop/\n") # keep run state out of the snapshot + git(tmp_path, "init", "-q", "-b", "main") + git(tmp_path, "config", "user.email", "test@test") + git(tmp_path, "config", "user.name", "test") + git(tmp_path, "add", "-A") + git(tmp_path, "commit", "-q", "-m", "initial") + baseline = git(tmp_path, "rev-parse", "HEAD") task = StoryTask( story_key="1-1-a", epic=1, @@ -542,6 +555,7 @@ def _escalated_run(tmp_path, spec_text, *, restore_patch_stale=None): attempt=2, spec_file=str(spec), restore_patch=restore_patch_stale, + baseline_commit=baseline, ) run_dir = _make_state_run( tmp_path, @@ -593,6 +607,121 @@ def test_rearm_plain_mode_sets_ready_for_dev_and_clears_stale_latch(tmp_path): assert entry["restore"] is False +# --------------------------------------------- #90: abandoned restore-latch residue + + +def _stale_restore_tree(tmp_path, *, latch="artifacts/attempt.patch"): + """An escalation whose latched restore already applied: `newfile.txt` is the + patch's untracked creation, `human.txt` is the resolve session's own file.""" + run_dir, spec = _escalated_run( + tmp_path, _SPEC_WITH_ARR, restore_patch_stale=latch, git_project=True + ) + patch = tmp_path / "artifacts" / "attempt.patch" + patch.parent.mkdir(parents=True, exist_ok=True) + patch.write_text( + "diff --git a/newfile.txt b/newfile.txt\n" + "new file mode 100644\n" + "index 0000000..1111111\n" + "--- /dev/null\n" + "+++ b/newfile.txt\n" + "@@ -0,0 +1 @@\n" + "+from the abandoned attempt\n", + encoding="utf-8", + ) + (tmp_path / "newfile.txt").write_text("from the abandoned attempt\n") # the applied residue + (tmp_path / "human.txt").write_text("from the resolve session\n") + return run_dir, spec, patch + + +def _kinds(run_dir, prefix="stale-restore-"): + from bmad_loop.journal import Journal + + return [e for e in Journal(run_dir).entries() if e["kind"].startswith(prefix)] + + +def test_rearm_excludes_stale_restore_residue_from_baseline_snapshot(tmp_path): + """The abandoned attempt's applied new files must NOT be blessed as + pre-existing, or finalize_commit's `add -A` sweeps them into the corrected + story's commit. The resolve session's own untracked file still is.""" + run_dir, _spec, patch = _stale_restore_tree(tmp_path) + + runs.rearm_escalation(run_dir) # from-scratch re-arm replaces the latch + + task = load_state(run_dir).tasks["1-1-a"] + assert "human.txt" in task.baseline_untracked + assert "newfile.txt" not in task.baseline_untracked + assert (tmp_path / "newfile.txt").exists() # rearm deletes nothing; the re-drive's reset does + excluded = _kinds(run_dir, "stale-restore-excluded") + assert len(excluded) == 1 + assert excluded[0]["files"] == ["newfile.txt"] + assert excluded[0]["patch"] == str(patch) + + +def test_rearm_re_latching_the_same_patch_still_excludes_its_residue(tmp_path): + """Re-arming a restore onto the same patch: the first application's files are + still residue (and `git apply` would otherwise fail with 'already exists').""" + run_dir, _spec, _patch = _stale_restore_tree(tmp_path) + + runs.rearm_escalation(run_dir, restore_patch="artifacts/attempt.patch") + + task = load_state(run_dir).tasks["1-1-a"] + assert task.restore_patch == "artifacts/attempt.patch" + assert "human.txt" in task.baseline_untracked + assert "newfile.txt" not in task.baseline_untracked + assert _kinds(run_dir, "stale-restore-excluded") + + +def test_rearm_missing_stale_patch_degrades_loudly_without_raising(tmp_path): + """A deleted patch file must never wedge resolve: journal the degrade and fall + back to the pre-#90 snapshot (everything untracked counts as pre-existing).""" + run_dir, _spec, patch = _stale_restore_tree(tmp_path) + patch.unlink() + (tmp_path / "committed.txt").write_text("from the escalated attempt\n") + git(tmp_path, "add", "committed.txt") + git(tmp_path, "commit", "-q", "-m", "attempt commit") + + runs.rearm_escalation(run_dir) # must not raise RearmError + + task = load_state(run_dir).tasks["1-1-a"] + assert {"human.txt", "newfile.txt"} <= set(task.baseline_untracked) # full snapshot + unparseable = _kinds(run_dir, "stale-restore-unparseable") + assert len(unparseable) == 1 + assert "FileNotFoundError" in unparseable[0]["error"] + assert not _kinds(run_dir, "stale-restore-excluded") + # the unreadable patch must not also cost the human the commits warning + assert _kinds(run_dir, "stale-restore-commits") + + +def test_rearm_without_a_stale_latch_journals_no_stale_restore_events(tmp_path): + run_dir, _spec = _escalated_run(tmp_path, _SPEC_WITH_ARR, git_project=True) + (tmp_path / "human.txt").write_text("from the resolve session\n") + + runs.rearm_escalation(run_dir, restore_patch="artifacts/attempt.patch") + + assert "human.txt" in load_state(run_dir).tasks["1-1-a"].baseline_untracked + assert _kinds(run_dir) == [] + + +def test_rearm_warns_about_commits_below_the_refreshed_baseline(tmp_path): + """The worse variant: commits made above the OLD baseline become the re-drive's + permanent starting point. Warn-only — a mechanical revert would claw back the + resolve session's own blessed commits, which live in the same range.""" + run_dir, _spec, _patch = _stale_restore_tree(tmp_path) + (tmp_path / "committed.txt").write_text("from the escalated attempt\n") + git(tmp_path, "add", "committed.txt") + git(tmp_path, "commit", "-q", "-m", "attempt commit") + old_baseline = load_state(run_dir).tasks["1-1-a"].baseline_commit + + runs.rearm_escalation(run_dir) + + task = load_state(run_dir).tasks["1-1-a"] + assert task.baseline_commit != old_baseline # baseline advanced past the commit + warned = _kinds(run_dir, "stale-restore-commits") + assert len(warned) == 1 + assert warned[0]["old_baseline"] == old_baseline + assert warned[0]["commits"] == [git(tmp_path, "rev-parse", "HEAD")] + + def test_archive_run(tmp_path): run_dir = _make_state_run(tmp_path, "20260611-100000-aaaa") (run_dir / "journal.jsonl").write_text('{"kind":"x"}\n') diff --git a/tests/test_verify.py b/tests/test_verify.py index 6afeeb1..319b87b 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -1031,6 +1031,94 @@ def test_apply_patch_conflict_raises(project): verify.apply_patch(repo, patch) +def _saved_patch(project, name="attempt.patch"): + """Capture the working tree as a restore patch, the way the skill saves one.""" + repo = project.project + git(repo, "add", "-AN") # intent-to-add so new files appear in `git diff HEAD` + patch = project.implementation_artifacts / name + patch.write_text(git(repo, "diff", "HEAD") + "\n", encoding="utf-8") + return patch + + +def test_patch_new_files_names_created_files(project): + repo = project.project + (repo / "new_module.py").write_text("print('hi')\n") + (repo / "pkg").mkdir() + (repo / "pkg" / "deep.txt").write_text("nested\n") + assert verify.patch_new_files(_saved_patch(project)) == {"new_module.py", "pkg/deep.txt"} + + +def test_patch_new_files_ignores_modifications(project): + repo = project.project + (repo / "src.txt").write_text("original\nedited\n") + assert verify.patch_new_files(_saved_patch(project)) == set() + + +def test_patch_new_files_never_returns_deletions(project): + """A deleted file must not land in the exclusion set: the human may re-create + that path, and excluding it would make the next rollback delete their copy.""" + repo = project.project + git(repo, "rm", "-q", "src.txt") + assert verify.patch_new_files(_saved_patch(project)) == set() + + +def test_patch_new_files_multi_file_patch(project): + """One patch carrying a creation, a modification and a deletion — only the + creation comes back.""" + repo = project.project + (repo / "created.txt").write_text("new\n") + (repo / ".gitignore").write_text("*.log\n") + git(repo, "rm", "-q", "src.txt") + assert verify.patch_new_files(_saved_patch(project)) == {"created.txt"} + + +def test_patch_new_files_ignores_header_lookalikes_in_hunk_bodies(project): + """Hunk *content* that reads like a file header (a removed `-- /dev/null` line + renders as `--- /dev/null`) must not be mistaken for a creation.""" + patch = project.implementation_artifacts / "tricky.patch" + patch.write_text( + "diff --git a/real.txt b/real.txt\n" + "new file mode 100644\n" + "index 0000000..1111111\n" + "--- /dev/null\n" + "+++ b/real.txt\n" + "@@ -0,0 +1 @@\n" + "+hello\n" + "diff --git a/mod.txt b/mod.txt\n" + "index 1111111..2222222 100644\n" + "--- a/mod.txt\n" + "+++ b/mod.txt\n" + "@@ -1 +1 @@\n" + "--- /dev/null\n" # a removed line whose content is `-- /dev/null` + "+++ b/evil.txt\n" # an added line whose content is `++ b/evil.txt` + "\\ No newline at end of file\n", + encoding="utf-8", + ) + assert verify.patch_new_files(patch) == {"real.txt"} + + +def test_patch_new_files_skips_quoted_paths(project): + """core.quotePath output is skipped rather than guessed — under-reporting is + safe, a wrong path would get a user file deleted.""" + patch = project.implementation_artifacts / "quoted.patch" + patch.write_text( + 'diff --git "a/w\\303\\251ird.txt" "b/w\\303\\251ird.txt"\n' + "new file mode 100644\n" + "--- /dev/null\n" + '+++ "b/w\\303\\251ird.txt"\n' + "@@ -0,0 +1 @@\n" + "+x\n", + encoding="utf-8", + ) + assert verify.patch_new_files(patch) == set() + + +def test_patch_new_files_missing_patch_raises_oserror(project): + """The caller (rearm) turns this into a journaled best-effort degrade.""" + with pytest.raises(OSError): + verify.patch_new_files(project.implementation_artifacts / "gone.patch") + + def test_read_frontmatter_tolerates_garbage(project): p = project.project / "x.md" p.write_text("no frontmatter here") From 0c70428f1a14d9925c5f2c7be4a68bc3792bc4f7 Mon Sep 17 00:00:00 2001 From: pbean Date: Thu, 9 Jul 2026 13:06:40 -0700 Subject: [PATCH 2/2] fix(verify): mirror git apply's -p1 when parsing patch creations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit patch_new_files only stripped the standard b/ prefix, but the restore patch is saved by the skill session under the user's git config — diff.mnemonicPrefix=true emits w/ (and --no-index emits 2/), leaving a wrong path in the exclusion set so the #90 fix silently no-ops. apply_patch runs plain `git apply` (default -p1), which strips the first component whatever it is; the parser now does the same. Targets -p1 cannot strip (no slash, e.g. --no-prefix output) are skipped: that apply failed outright, so no residue exists — recording them verbatim could exclude (and later delete) a same-named file the human created. --- src/bmad_loop/verify.py | 15 +++++++++++---- tests/test_verify.py | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 4 deletions(-) diff --git a/src/bmad_loop/verify.py b/src/bmad_loop/verify.py index b131b5f..a89c1b1 100644 --- a/src/bmad_loop/verify.py +++ b/src/bmad_loop/verify.py @@ -1562,7 +1562,12 @@ def patch_new_files(patch_path: Path) -> set[str]: Text-parse, not `git apply --numstat`: the caller runs after the tree has moved on, so the patch may no longer apply, and a creation list must still come back. Within each `diff --git` block, an old-side `---` header naming `_DIFF_ABSENT` - marks a creation, and the `+++ b/` after it names the file. Deletions (the + marks a creation, and the `+++ /` after it names the file. The + prefix is stripped by mirroring what `apply_patch`'s plain `git apply` (default + -p1) did when it laid the residue down: drop the first path component whatever + it is — `b/` standard, `w/`/`i/`/`c/` under diff.mnemonicPrefix, `2/` from + --no-index. A target -p1 cannot strip (no `/`, e.g. --no-prefix output) is + skipped: that apply failed outright, so no residue exists. Deletions (the absent token on the *new* side) are never returned — the caller feeds this to an *exclusion* set, and excluding a path the human later re-created would make the next rollback delete their file. For the same reason every ambiguous entry is @@ -1589,9 +1594,11 @@ def patch_new_files(patch_path: Path) -> set[str]: elif line.startswith("+++ ") and creating: creating = False target = line[4:].split("\t", 1)[0].strip() - if target == _DIFF_ABSENT or target.startswith('"'): - continue # a delete-then-create pair, or a quoted path we won't guess - new_files.add(target[2:] if target.startswith("b/") else target) + if target == _DIFF_ABSENT or target.startswith('"') or "/" not in target: + continue # delete-then-create pair, quoted path, or un-strippable target + rel = target.split("/", 1)[1] # mirror `git apply`'s default -p1 + if rel: + new_files.add(rel) return new_files diff --git a/tests/test_verify.py b/tests/test_verify.py index 319b87b..92571dd 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -1113,6 +1113,43 @@ def test_patch_new_files_skips_quoted_paths(project): assert verify.patch_new_files(patch) == set() +def test_patch_new_files_strips_mnemonic_prefixes(project): + """`diff.mnemonicPrefix=true` in the user's config makes `git diff HEAD` emit + `c/`/`w/` instead of `a/`/`b/`. `apply_patch`'s plain `git apply` (-p1) strips + the first component whatever it is, so the parser must mirror that or the + recorded residue is `w/` and the exclusion silently no-ops.""" + repo = project.project + (repo / "new_module.py").write_text("print('hi')\n") + (repo / "pkg").mkdir() + (repo / "pkg" / "deep.txt").write_text("nested\n") + git(repo, "add", "-AN") + patch = project.implementation_artifacts / "mnemonic.patch" + patch.write_text( + git(repo, "-c", "diff.mnemonicPrefix=true", "diff", "HEAD") + "\n", encoding="utf-8" + ) + assert "+++ w/new_module.py" in patch.read_text(encoding="utf-8") # fixture sanity + assert verify.patch_new_files(patch) == {"new_module.py", "pkg/deep.txt"} + + +def test_patch_new_files_skips_unstrippable_targets(project): + """A prefixless single-component target (`git diff --no-prefix`) cannot survive + `git apply`'s -p1 strip — the apply would have failed, so no residue can exist. + Recording it verbatim could exclude (and later delete) a same-named file the + human created; skip it instead.""" + patch = project.implementation_artifacts / "noprefix.patch" + patch.write_text( + "diff --git newfile.txt newfile.txt\n" + "new file mode 100644\n" + "index 0000000..1111111\n" + "--- /dev/null\n" + "+++ newfile.txt\n" + "@@ -0,0 +1 @@\n" + "+x\n", + encoding="utf-8", + ) + assert verify.patch_new_files(patch) == set() + + def test_patch_new_files_missing_patch_raises_oserror(project): """The caller (rearm) turns this into a journaled best-effort degrade.""" with pytest.raises(OSError):