From 6ce158b9446c0892d0f368a8510ecfcc55c7a960 Mon Sep 17 00:00:00 2001 From: t Date: Tue, 22 Sep 2026 14:25:53 -0700 Subject: [PATCH 1/4] fix(verify): read untracked paths verbatim, NUL-delimited (#783) `untracked_files` read `ls-files --others` line by line and stripped each record. Under the default core.quotePath that yields git's C-quoted spelling of every non-ASCII (and newline-bearing) name and eats leading/trailing spaces, so `snapshot_worktree`'s `git add --` failed on a non-path, the rollback refused, and the cleanup plan got the wrong names. Now `ls-files -z` through `git_bytes` (the chokepoint's binary accessor): split on NUL, no strip, stderr never becomes a record (#442). Bytes rather than text mode because text mode's universal-newline read rewrites a `\r` in a name to `\n`. Decoding, as found: `_run_git` text mode decodes with the locale codec, strict, and translates UnicodeDecodeError into GitError (#377). But under the default quotePath git never emitted a raw high byte to this reader, so an invalid-UTF-8 name used to come back as its quoted token, not a GitError; only quotePath=false hosts got the GitError. Each record is now decoded strictly with the filesystem codec, and an undecodable one keeps the default-config answer (the quoted token, via `_c_quote_path`), which is inert and self-consistent across baseline and later reads. Raising instead would fail every baseline capture in a repo holding one such stray; surrogateescape would leak surrogates into state/journal JSON. Compatibility: a run persisted before this fix holds quoted/stripped records in `baseline_untracked`; an exact difference would make those pre-existing files deletion targets on resume. The rollback-side consumers (`snapshot_worktree`, `_rollback_cleanup_plan`, `attempt_dirty`) now go through `_created_untracked`, which also treats a path as baseline when any of its pre-fix spellings (git's quoting under either quotePath setting, split and stripped as the old reader did) is in the baseline. Spurious matches only spare a file. The proof-of-work gate keeps the exact difference: it fails open toward "work happened". --- CHANGELOG.md | 4 ++ src/bmad_loop/verify.py | 124 ++++++++++++++++++++++++++++---- tests/test_verify.py | 154 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 270 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b999ba5fb..a0d8bfbb0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,10 @@ breaking changes may land in a minor release. ### Fixed +- Read untracked paths verbatim so rollback snapshots and cleanup handle non-ASCII + and space-edged filenames; a resumed run's pre-fix baseline still protects the + files it listed (#783). + - Replace stale installed relay hooks when a project moves between Windows and POSIX. - Report stale or unverifiable Codex hook trust in `validate` and `probe-adapter` diff --git a/src/bmad_loop/verify.py b/src/bmad_loop/verify.py index 66b73e3a2..eed960020 100644 --- a/src/bmad_loop/verify.py +++ b/src/bmad_loop/verify.py @@ -4979,7 +4979,7 @@ def attempt_dirty( return True if baseline_untracked is None: return False - created = untracked_files(repo) - set(baseline_untracked) + created = _created_untracked(repo, baseline_untracked) created = {p for p in created if not _path_under_any(p, exclude)} return bool(created) @@ -5237,15 +5237,115 @@ def untracked_files(repo: Path) -> set[str]: plain `git clean -fd` (no -x) treats as removable. Ignored files are excluded, so they are never rollback candidates. - Reads stdout ALONE (`_git_out`): `ls-files` exits 0 while still writing to - stderr, and against `_git`'s merged stream that chatter splits into a phantom - untracked path — a PRISTINE tree answers with one, and because this function's - contract is what `git clean -fd` would remove, the phantom is a rollback - candidate. Silent, on every host whose git config warns (#442).""" - rc, out, detail = _git_out(repo, "ls-files", "--others", "--exclude-standard") - if rc != 0: + NUL-delimited and read VERBATIM (#783), because these names are operands: + `snapshot_worktree` hands them to `git add --` and the rollback cleanup + deletes them. The line-based read this replaces got `core.quotePath`'s + C-quoted spelling of every non-ASCII name — not a path, so the snapshot's + `add` failed and the rollback refused — and `.strip()` ate edge spaces. + Bytes rather than text mode, whose universal-newline read turns a `\\r` in a + name into `\\n`; each record is decoded strictly with the filesystem codec, + so it names the file `Path` and argv will reach. + + A record that codec cannot decode keeps its pre-#783 answer, the quoted + spelling (`_c_quote_path`): an inert token that matches itself across the + baseline and later reads, rather than a GitError that would fail every + baseline capture in a repo holding one such stray, or surrogates that would + leak into the state and journal JSON (see `_run_git`). + + stdout ALONE, as before (#442): `ls-files` exits 0 while still writing to + stderr, and a merged stream would turn that chatter into a phantom path.""" + proc = git_bytes(repo, "ls-files", "-z", "--others", "--exclude-standard") + if proc.returncode != 0: + detail = proc.stderr.decode("utf-8", "replace").strip() raise GitError(f"git ls-files --others failed in {repo}: {detail}") - return {line.strip() for line in out.splitlines() if line.strip()} + return {_decode_git_path(rel) for rel in proc.stdout.split(b"\0") if rel} + + +# quote.c `cq_lookup`: the bytes git escapes by letter; every other byte it must +# quote is written as a three-digit octal escape. +_CQ_LETTER_ESCAPES = { + 0x07: b"a", + 0x08: b"b", + 0x09: b"t", + 0x0A: b"n", + 0x0B: b"v", + 0x0C: b"f", + 0x0D: b"r", + 0x22: b'"', + 0x5C: b"\\", +} + + +def _c_quote_path(raw: bytes, *, quote_fully: bool) -> bytes: + """git's `quote_c_style` of one path, as non-`-z` porcelain prints it. + + Control bytes, DEL, `"` and `\\` always force quoting; bytes >= 0x80 do only + under ``quote_fully`` (`core.quotePath`, default true). A name needing none + comes back unchanged, otherwise double-quoted with every such byte escaped.""" + + def must_quote(b: int) -> bool: + return b < 0x20 or b == 0x7F or b in (0x22, 0x5C) or (quote_fully and b >= 0x80) + + if not any(must_quote(b) for b in raw): + return raw + out = bytearray(b'"') + for b in raw: + if not must_quote(b): + out.append(b) + elif b in _CQ_LETTER_ESCAPES: + out += b"\\" + _CQ_LETTER_ESCAPES[b] + else: + out += b"\\%03o" % b + out += b'"' + return bytes(out) + + +def _decode_git_path(raw: bytes) -> str: + """One `-z` path record as `untracked_files` answers it — see there.""" + try: + return raw.decode(sys.getfilesystemencoding()) + except UnicodeDecodeError: + return _c_quote_path(raw, quote_fully=True).decode("ascii") + + +def _pre_783_spellings(name: str) -> set[str]: + """Every record the pre-#783 line-based `untracked_files` could have + persisted for the file ``name``, under either `core.quotePath` setting: the + C-quoted rendering, split on lines and stripped as that reader did.""" + raw = name.encode(sys.getfilesystemencoding(), "surrogateescape") + spellings: set[str] = set() + for quote_fully in (True, False): + try: + rendered = _c_quote_path(raw, quote_fully=quote_fully).decode( + sys.getfilesystemencoding() + ) + except UnicodeDecodeError: + continue # that reader raised on this name, so persisted nothing for it + spellings.update(r.strip() for r in rendered.splitlines() if r.strip()) + return spellings + + +def _created_untracked(repo: Path, baseline_untracked: list[str]) -> set[str]: + """The untracked files this attempt created, for the ROLLBACK side — + `snapshot_worktree`, `_rollback_cleanup_plan` and the `attempt_dirty` gate + that mirrors them. + + A path counts only when neither its exact name nor any `_pre_783_spellings` + of it is in the baseline. A baseline persisted before #783 holds the quoted + or stripped records of the old reader, so an exact difference alone would + turn every pre-existing non-ASCII or space-edged file of a resumed run into + a deletion target. Matching the old spellings is exact where it matters: + each file's old records ARE its spellings, so every file a legacy baseline + listed stays protected. The only cost is spurious matches (a new ` a` beside + a baseline `a`), and those fail toward leaving a file alone, the direction + the rollback gates fail in. The proof-of-work gate (`_changes_since`) keeps + the exact difference: it fails open toward "work happened" instead.""" + baseline = set(baseline_untracked) + return { + path + for path in untracked_files(repo) - baseline + if baseline.isdisjoint(_pre_783_spellings(path)) + } def path_tracked(repo: Path, rel: str) -> bool: @@ -5877,7 +5977,7 @@ def snapshot_worktree( are left untouched: seed the temp index from HEAD, ``add -u`` the tracked edits/deletions, then stage only the untracked files *this run* created — ``untracked_files(repo)`` minus ``baseline_untracked`` (the snapshot taken - when the baseline was captured). This mirrors :func:`safe_rollback`'s scope + when the baseline was captured; :func:`_created_untracked`). This mirrors :func:`safe_rollback`'s scope exactly: the snapshot holds precisely what the reset would destroy and never a pre-existing user untracked file. When ``baseline_untracked`` is ``None`` (a pre-upgrade/resumed run with no snapshot) no untracked file is staged — matching @@ -5931,7 +6031,7 @@ def snapshot_worktree( if baseline_untracked is None: new: list[str] = [] else: - new = sorted(untracked_files(repo) - set(baseline_untracked)) + new = sorted(_created_untracked(repo, baseline_untracked)) if new: rc, out = _git_env(repo, "add", "--", *new, env=env) if rc != 0: @@ -6033,7 +6133,7 @@ def _rollback_cleanup_plan( if baseline_untracked is None: return _RollbackCleanupPlan(repo_root=None, keep_roots=(), targets=()) - created = untracked_files(repo) - set(baseline_untracked) + created = _created_untracked(repo, baseline_untracked) try: repo_root = repo.resolve() keep_roots = tuple((repo_root / rel).resolve() for rel in keep) diff --git a/tests/test_verify.py b/tests/test_verify.py index 3f39dd549..8c1392a5b 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -4480,6 +4480,160 @@ def test_safe_rollback_keep_dir_protects_run_created(project): assert (out / "fresh-artifact.md").exists() # protected by keep even though new +# Names the pre-#783 line-based `untracked_files` did not answer verbatim: git +# C-quotes a non-ASCII or newline-bearing name, and `.strip()` ate edge spaces. +# Windows forbids a newline in a name and silently drops a trailing space. +_POSIX_NAME = pytest.mark.skipif(sys.platform == "win32", reason="illegal Windows filename") +_VERBATIM_NAMES = [ + pytest.param("spec-能力.md", id="unicode"), + pytest.param(" lead.txt", id="leading-space"), + pytest.param("trail.txt ", id="trailing-space", marks=_POSIX_NAME), + pytest.param("new\nline.txt", id="newline", marks=_POSIX_NAME), +] + + +def _quote_path_default(repo): + """Pin `core.quotePath` to git's default, whatever the host's global config says.""" + git(repo, "config", "core.quotePath", "true") + + +def _tree_names(repo, ref): + """Every path in ``ref``'s tree, verbatim (NUL-delimited, undecoded by git).""" + out = subprocess.run( + ["git", "-C", str(repo), "ls-tree", "-r", "-z", "--name-only", ref], + capture_output=True, + check=True, + ).stdout + return {os.fsdecode(rel) for rel in out.split(b"\0") if rel} + + +def _pre_783_listing(repo): + """What the pre-#783 reader persisted: git's line-based listing, lines stripped.""" + out = subprocess.run( + ["git", "-C", str(repo), "ls-files", "--others", "--exclude-standard"], + capture_output=True, + text=True, + check=True, + ).stdout + return sorted(line.strip() for line in out.splitlines() if line.strip()) + + +@pytest.mark.parametrize("name", _VERBATIM_NAMES) +def test_untracked_files_answers_exact_on_disk_names(project, name): + """#783: the answer is the file's own name — not its C-quoted spelling, not + split at a newline, not stripped of an edge space.""" + repo = project.project + _quote_path_default(repo) + (repo / name).write_text("x\n") + assert name in verify.untracked_files(repo) + + +@pytest.mark.parametrize("name", _VERBATIM_NAMES) +def test_snapshot_worktree_parks_verbatim_named_untracked(project, name): + """The snapshot's `git add --` is handed the real name, so it succeeds and + parks the file; the quoted spelling made `add` fail and the rollback refuse.""" + repo = project.project + _quote_path_default(repo) + baseline_untracked = sorted(verify.untracked_files(repo)) + (repo / name).write_text("x\n") + + ref = verify.snapshot_worktree( + repo, "refs/attempt-preserve-dirty/verbatim", baseline_untracked=baseline_untracked + ) + assert ref is not None + assert name in _tree_names(repo, ref) + + +@pytest.mark.parametrize("name", _VERBATIM_NAMES) +def test_safe_rollback_removes_only_run_created_verbatim_names(project, name): + """A run-created file of that name is deleted; the operator's pre-existing one + in the baseline and a keep-dir-protected one both survive.""" + repo = project.project + _quote_path_default(repo) + (repo / "pre").mkdir() + (repo / "pre" / name).write_text("operator's\n") + baseline = verify.rev_parse_head(repo) + snap = sorted(verify.untracked_files(repo)) + (repo / name).write_text("run-created\n") + (repo / "_bmad-output").mkdir(exist_ok=True) + (repo / "_bmad-output" / name).write_text("kept\n") + + verify.safe_rollback( + repo, baseline, baseline_untracked=snap, keep=(".bmad-loop", "_bmad-output") + ) + assert not (repo / name).exists() + assert (repo / "pre" / name).read_text() == "operator's\n" + assert (repo / "_bmad-output" / name).read_text() == "kept\n" + + +def test_safe_rollback_legacy_quoted_baseline_protects_preexisting(project): + """A run persisted before #783 holds the old reader's quoted/stripped records. + Resumed after the fix, those must still protect the files they listed: an exact + difference would make each one a run-created deletion target (and park it, and + read the tree dirty). + + Ablation target: make `_created_untracked` an exact difference and the operator's + files are deleted.""" + repo = project.project + _quote_path_default(repo) + names = ["spec-能力.md", " lead.txt"] + if sys.platform != "win32": + names += ["trail.txt ", "new\nline.txt"] + for name in names: + (repo / name).write_text("operator's\n") + baseline = verify.rev_parse_head(repo) + legacy = _pre_783_listing(repo) + assert not set(names) & set(legacy) # the old spellings, not the names + + (repo / "junk.txt").write_text("run-created\n") + verify.safe_rollback(repo, baseline, baseline_untracked=legacy, keep=(".bmad-loop",)) + assert not (repo / "junk.txt").exists() + for name in names: + assert (repo / name).read_text() == "operator's\n" + assert not verify.attempt_dirty(repo, baseline, legacy) # the gate agrees + + +def _touch_bytes_name(repo, raw): + """Create an empty file whose name is the raw bytes ``raw`` (`Path` takes str only).""" + with open(os.path.join(os.fsencode(repo), raw), "wb"): + pass + + +@pytest.mark.skipif(sys.platform != "linux", reason="needs arbitrary-byte filenames") +@pytest.mark.parametrize("quote_path", ["true", "false"]) +def test_c_quote_path_matches_git_for_every_byte(project, quote_path): + """`_c_quote_path` is the legacy-baseline oracle, so pin it to git itself: one + file per byte a name can hold, compared with git's line-based listing.""" + repo = project.project + git(repo, "config", "core.quotePath", quote_path) + before = set(os.listdir(os.fsencode(repo))) + for b in range(1, 256): + if b != ord("/"): + _touch_bytes_name(repo, bytes([b, ord("x")])) + raws = set(os.listdir(os.fsencode(repo))) - before + out = subprocess.run( + ["git", "-C", str(repo), "ls-files", "--others", "--exclude-standard"], + capture_output=True, + check=True, + ).stdout + want = {line for line in out.split(b"\n") if line} + got = {verify._c_quote_path(raw, quote_fully=quote_path == "true") for raw in raws} + assert got <= want and len(got) == len(raws) + + +@pytest.mark.skipif(sys.platform != "linux", reason="needs a non-UTF-8 filename") +def test_untracked_files_keeps_quoted_token_for_undecodable_name(project): + """A name the filesystem codec cannot decode keeps the pre-#783 answer (git's + quoted spelling) instead of raising — so a stray latin-1 file cannot fail every + baseline capture — and that token still matches itself across two reads.""" + repo = project.project + _quote_path_default(repo) + _touch_bytes_name(repo, b"lat\xff.txt") + assert verify.untracked_files(repo) >= {'"lat\\377.txt"'} + baseline = verify.rev_parse_head(repo) + assert not verify.attempt_dirty(repo, baseline, sorted(verify.untracked_files(repo))) + + def test_safe_rollback_none_snapshot_removes_nothing(project): repo = project.project baseline = verify.rev_parse_head(repo) From c557fa191403af5ff12d636bb93a25eb607040a1 Mon Sep 17 00:00:00 2001 From: t Date: Tue, 22 Sep 2026 14:30:43 -0700 Subject: [PATCH 2/4] fix(plugins): run post_story hooks from the repo root after worktree teardown (#779) post_story fires after _run_isolated has merged the unit and removed its worktree, but the bus still chose ctx.worktree as the declarative hook's cwd. subprocess.run raised FileNotFoundError before the shell started, so the journal logged plugin-hook-error and the hook never ran. For post_story alone, a worktree that no longer exists falls back to ctx.repo_root. Every other stage keeps its cwd, and with it the error and fail-closed semantics, since the project root is the wrong tree mid-story. The context and BMAD_LOOP_WORKTREE keep the original path so a hook can still tell which unit it was. In-process Python hooks get no cwd from the bus and are unchanged. --- CHANGELOG.md | 3 ++ docs/plugin-authoring-guide.md | 4 +++ src/bmad_loop/plugins/bus.py | 7 ++++ tests/test_engine_worktree.py | 30 ++++++++++++++++- tests/test_hook_bus.py | 61 ++++++++++++++++++++++++++++++++++ 5 files changed, 104 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a0d8bfbb0..a2cba1f14 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,9 @@ breaking changes may land in a minor release. and space-edged filenames; a resumed run's pre-fix baseline still protects the files it listed (#783). +- Run a declarative `post_story` hook from the repo root once the unit's worktree + is torn down, instead of failing on the removed cwd (#779). + - Replace stale installed relay hooks when a project moves between Windows and POSIX. - Report stale or unverifiable Codex hook trust in `validate` and `probe-adapter` diff --git a/docs/plugin-authoring-guide.md b/docs/plugin-authoring-guide.md index dc5125de2..2d3e2f977 100644 --- a/docs/plugin-authoring-guide.md +++ b/docs/plugin-authoring-guide.md @@ -384,6 +384,10 @@ there. | `pre_integrate` | before integrating a finished unit | — | | `pre_merge` / `post_merge` | around the local branch merge | — | +`post_story` fires after an isolated unit's worktree is torn down, so a declarative +`post_story` hook then runs from the repo root, and `BMAD_LOOP_WORKTREE` still names +the removed worktree. + ### Dev | Stage | When | Mutable surface | diff --git a/src/bmad_loop/plugins/bus.py b/src/bmad_loop/plugins/bus.py index e4d465314..14d032c73 100644 --- a/src/bmad_loop/plugins/bus.py +++ b/src/bmad_loop/plugins/bus.py @@ -196,6 +196,13 @@ def _dispatch_declarative(self, lp: LoadedPlugin, hook: Any, ctx: HookContext) - cmd = lp.manifest.render(hook.cmd) env = _hook_env(ctx, lp) cwd = ctx.worktree or ctx.repo_root or None + # post_story fires after an isolated unit merged and its worktree was + # removed, so that path no longer exists and subprocess would raise + # before the shell starts (#779). Only there does the project root stand + # in; any other stage keeps the error — the root is the wrong tree for a + # hook mid-story. ctx and BMAD_LOOP_WORKTREE keep the original path. + if hook.stage == "post_story" and ctx.worktree and not os.path.isdir(ctx.worktree): + cwd = ctx.repo_root or None try: rc, output = self._runner(cmd, cwd=cwd, env=env, timeout=hook.timeout_sec) except _HookError as e: diff --git a/tests/test_engine_worktree.py b/tests/test_engine_worktree.py index 7a2be2875..29775c5e6 100644 --- a/tests/test_engine_worktree.py +++ b/tests/test_engine_worktree.py @@ -16,6 +16,7 @@ import pytest from conftest import ( _OK, + _RUN, NUL_PATH_RESOLVE_FAULTS, UNDECODABLE_LEDGER, _exists_run, @@ -5536,7 +5537,9 @@ def test_story_label_stripped_matches_the_task_id(raw, story_key, expected): # ------------------------------------------------ per_worktree engine plugin -def _write_stub_plugin(project, name, *, ready=_OK, setup=_OK, teardown=_OK, seed_globs=None): +def _write_stub_plugin( + project, name, *, ready=_OK, setup=_OK, teardown=_OK, seed_globs=None, post_story=None +): """A project-local *declarative* plugin whose lifecycle hooks are shell stubs (no real Unity) — proving a generic data-only plugin can gate the engine's per_worktree flow. A blocking hook's non-zero exit vetoes (defers) the unit. @@ -5558,6 +5561,8 @@ def _write_stub_plugin(project, name, *, ready=_OK, setup=_OK, teardown=_OK, see "[hooks.pre_worktree_teardown]", f"cmd = '{teardown}'", ] + if post_story is not None: + lines += ["[hooks.post_story]", f"cmd = '{post_story}'"] (plug_dir / "plugin.toml").write_text("\n".join(lines) + "\n") @@ -5704,6 +5709,29 @@ def test_per_worktree_teardown_runs_on_pause(project): assert "pre_worktree_teardown" in _hook_stages(engine) +def test_post_story_hook_runs_from_repo_root_after_worktree_teardown(project): + """#779: post_story fires after the unit merged and its worktree was removed. + The declarative hook used to get that deleted path as cwd, so subprocess + raised before the shell started and only `plugin-hook-error` was journalled. + It now runs from the project root; the marker records the cwd it ran in.""" + commit_sprint(project, {"1-1-a": "ready-for-dev"}) + marker = "post-story-cwd" + record = f'cd > "{_RUN}\\{marker}"' if sys.platform == "win32" else f'pwd > "{_RUN}/{marker}"' + _write_stub_plugin(project, "stub", post_story=record) + engine, _ = make_engine( + project, + [wt_dev_effect(project, "1-1-a"), wt_review_effect(project, "1-1-a", clean=True)], + policy=_pw_policy(), + ) + summary = engine.run() + + assert summary.done == 1 + assert len(worktree_list(project.project)) == 1 # the unit's worktree is gone + assert "plugin-hook-error" not in journal_kinds(engine) + ran_in = (engine.run_dir / marker).read_text().strip() + assert Path(ran_in).samefile(project.repo_root) + + def _leaking_dev_effect(project, story_key, *, leak_name, in_branch_set): """A dev effect that does the normal worktree work AND simulates a per_worktree Unity Editor leaking an asset write into the *main* checkout before merge. diff --git a/tests/test_hook_bus.py b/tests/test_hook_bus.py index 59c990fd8..335c6a259 100644 --- a/tests/test_hook_bus.py +++ b/tests/test_hook_bus.py @@ -288,6 +288,67 @@ def boom(*a, **k): assert closed_ctx.resolved_veto().action == "defer" +def _cwd_runner(seen): + def runner(cmd, *, cwd, env, timeout): + seen["cwd"] = cwd + seen["worktree_env"] = env.get("BMAD_LOOP_WORKTREE") + return 0, "" + + return runner + + +def test_post_story_with_removed_worktree_runs_from_repo_root(tmp_path): + """#779: post_story fires after the isolated unit's worktree was torn down; + the hook runs from the project root instead of a deleted cwd, while the + context and BMAD_LOOP_WORKTREE still name the original worktree.""" + gone = str(tmp_path / "wt-gone") + root = str(tmp_path) + seen = {} + c = ctx("post_story", worktree=gone, repo_root=root) + HookBus(registry_of(declarative("post_story")), runner=_cwd_runner(seen)).emit("post_story", c) + assert seen == {"cwd": root, "worktree_env": gone} + assert c.worktree == gone + + +def test_post_story_with_live_worktree_runs_in_the_worktree(tmp_path): + wt = tmp_path / "wt" + wt.mkdir() + seen = {} + c = ctx("post_story", worktree=str(wt), repo_root=str(tmp_path)) + HookBus(registry_of(declarative("post_story")), runner=_cwd_runner(seen)).emit("post_story", c) + assert seen == {"cwd": str(wt), "worktree_env": str(wt)} + + +@pytest.mark.parametrize("stage", ["pre_commit_gate", "post_dev_phase"]) +def test_other_stages_never_fall_back_to_repo_root(tmp_path, stage): + """The #779 fallback is post_story's alone: mid-story the project root is + the wrong tree, so a missing worktree keeps its cwd (and its error).""" + gone = str(tmp_path / "wt-gone") + seen = {} + c = ctx(stage, worktree=gone, repo_root=str(tmp_path)) + HookBus(registry_of(declarative(stage)), runner=_cwd_runner(seen)).emit(stage, c) + assert seen["cwd"] == gone + + +def test_real_runner_executes_post_story_after_worktree_removal(tmp_path): + """Through the real subprocess transport, the post_story hook runs: before + #779 `subprocess.run(cwd=)` raised and only plugin-hook-error was + journalled.""" + wt = tmp_path / "wt" + wt.mkdir() + wt.rmdir() + marker = tmp_path / "ran" + m = manifest( + "d", + hooks=(HookSpec(stage="post_story", cmd=f"\"{sys.executable}\" -c \"open('ran', 'w')\""),), + ) + journal = _FakeJournal() + c = ctx("post_story", worktree=str(wt), repo_root=str(tmp_path)) + HookBus(registry_of(LoadedPlugin(manifest=m)), journal).emit("post_story", c) + assert marker.is_file() + assert "plugin-hook-error" not in journal.kinds() + + def test_real_subprocess_runner_reports_exit_code(tmp_path): # _run_subprocess runs via the host shell (cmd on Windows, sh on POSIX); use a # command that prints "hi" and exits 7 on each. From 4ea68d9eacf7fc95fbbee375797343cac345fc7b Mon Sep 17 00:00:00 2001 From: t Date: Tue, 22 Sep 2026 17:21:53 -0700 Subject: [PATCH 3/4] fix(verify,diagnostics): verbatim untracked names in capture_diff; count hook errors in diagnose (#783, #779) capture_diff still listed untracked files with the line-based `ls-files` read that #783 replaced in untracked_files. Under core.quotePath a non-ASCII name came back C-quoted (and `.strip()` ate edge spaces); `git diff --no-index` could not open that spelling and exited 1 with empty stdout, the code the loop tolerates as "the files differ", so the file was silently left out of the failed-unit forensic patch. The leg now iterates `sorted(untracked_files(repo))`, which also carries the #442 stdout-alone read. A name the filesystem codec cannot decode still arrives as the quoted token and stays omitted, the same residual #783 left. diagnose's plugin-errors total counted only `plugin-error`, so a run whose hook bus journaled `plugin-hook-error` summarized as 0 plugin errors beside a histogram showing plugin-hook-error=1. Both kinds now count. --- CHANGELOG.md | 4 +++- src/bmad_loop/diagnostics.py | 2 +- src/bmad_loop/verify.py | 24 ++++++++++-------------- tests/test_diagnostics.py | 17 +++++++++++++++++ tests/test_verify.py | 13 +++++++++++++ 5 files changed, 44 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a2cba1f14..f1fda8abb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,11 +32,13 @@ breaking changes may land in a minor release. - Read untracked paths verbatim so rollback snapshots and cleanup handle non-ASCII and space-edged filenames; a resumed run's pre-fix baseline still protects the - files it listed (#783). + files it listed; failed-unit diff capture includes them too (#783). - Run a declarative `post_story` hook from the repo root once the unit's worktree is torn down, instead of failing on the removed cwd (#779). +- Count `plugin-hook-error` entries in `diagnose`'s plugin-errors total (#779). + - Replace stale installed relay hooks when a project moves between Windows and POSIX. - Report stale or unverifiable Codex hook trust in `validate` and `probe-adapter` diff --git a/src/bmad_loop/diagnostics.py b/src/bmad_loop/diagnostics.py index 85ce388c8..0809fc1d4 100644 --- a/src/bmad_loop/diagnostics.py +++ b/src/bmad_loop/diagnostics.py @@ -1110,7 +1110,7 @@ def summarize_journal( ), escalation_count=kinds.get("story-escalated", 0) + kinds.get("preference-escalation", 0), defer_count=kinds.get("story-deferred", 0), - plugin_error_count=kinds.get("plugin-error", 0), + plugin_error_count=kinds.get("plugin-error", 0) + kinds.get("plugin-hook-error", 0), per_alias_event_counts={a: dict(c) for a, c in per_alias.items()}, entries=scrubbed, ) diff --git a/src/bmad_loop/verify.py b/src/bmad_loop/verify.py index eed960020..b335a7a63 100644 --- a/src/bmad_loop/verify.py +++ b/src/bmad_loop/verify.py @@ -7625,13 +7625,15 @@ def capture_diff(repo: Path, baseline: str, *, max_file_bytes: int | None = None Unlike `_git`, the tracked diff is read from stdout alone and left verbatim (no strip, no stderr merge) so the patch stays applyable, as is the - `--no-index` spawn below it. The untracked leg now matches those two - (`_git_out`, #442): its `ls-files` exits 0 while still - warning on stderr, so against the merged stream the warning splits off as a - phantom rel. Measured, that phantom is inert here — `diff --no-index` cannot - access it and exits 1, exactly the code the loop below already tolerates as - "the files differ", with empty stdout — so this leg is converted for the same - reason its two neighbours read stdout alone, not on a demonstrated corruption. + `--no-index` spawn below it. The untracked names come from `untracked_files`, + which inherits the same stdout-alone read (#442) and returns each name + verbatim (#783). The line-based read it replaces got `core.quotePath`'s + C-quoted spelling of a non-ASCII name; `--no-index` cannot open that + spelling and exits 1 with empty stdout, the code the loop tolerates as "the + files differ", so the file silently dropped out of the patch. Residual: a + name the filesystem codec cannot decode still arrives as `untracked_files`' + quoted token, `--no-index` cannot open it, and it stays omitted — the same + residual #783 left. max_file_bytes caps the size of each *untracked* file included: a file larger than the cap is skipped and replaced with a one-line marker naming it and its @@ -7643,13 +7645,7 @@ def capture_diff(repo: Path, baseline: str, *, max_file_bytes: int | None = None raise GitError(f"git diff {baseline} failed in {repo}: {proc.stderr.strip()}") parts = [proc.stdout] - rc, out, detail = _git_out(repo, "ls-files", "--others", "--exclude-standard") - if rc != 0: - raise GitError(f"git ls-files --others failed in {repo}: {detail}") - for rel in out.splitlines(): - rel = rel.strip() - if not rel: - continue + for rel in sorted(untracked_files(repo)): if max_file_bytes is not None: try: size = (repo / rel).stat().st_size diff --git a/tests/test_diagnostics.py b/tests/test_diagnostics.py index f022e99f5..331de18c3 100644 --- a/tests/test_diagnostics.py +++ b/tests/test_diagnostics.py @@ -1970,6 +1970,23 @@ def test_unreadable_line_is_counted_but_does_not_move_the_clock(tmp_path): assert scrubbed["bytes"] == len("not json at all") +def test_summarize_journal_counts_plugin_hook_errors(tmp_path): + """The plugin-errors total counts both failure kinds the plugin layer journals: + `plugin-error` and the hook bus's `plugin-hook-error` (#779). Counting only the + first read 0 plugin errors beside a histogram showing plugin-hook-error=1.""" + (tmp_path / "journal.jsonl").write_text( + '{"ts": 100.0, "kind": "plugin-error"}\n' + '{"ts": 110.0, "kind": "plugin-hook-error"}\n' + '{"ts": 120.0, "kind": "plugin-hook-error"}\n', + encoding="utf-8", + ) + entries = Journal(tmp_path).entries() + pseudo = sanitize.Pseudonymizer(salt=b"fixed") + summary = diagnostics.summarize_journal(entries, pseudo, {}, cap=10) + + assert summary.plugin_error_count == 3 + + def test_structure_is_preserved(project): run_dir = _seed_run(project.project) diag, _pseudo, _combined = _render_all([run_dir]) diff --git a/tests/test_verify.py b/tests/test_verify.py index 8c1392a5b..858eb1147 100644 --- a/tests/test_verify.py +++ b/tests/test_verify.py @@ -4593,6 +4593,19 @@ def test_safe_rollback_legacy_quoted_baseline_protects_preexisting(project): assert not verify.attempt_dirty(repo, baseline, legacy) # the gate agrees +@pytest.mark.parametrize("name", _VERBATIM_NAMES) +def test_capture_diff_includes_verbatim_named_untracked(project, name): + """The failed-unit patch includes the file: `--no-index` is handed the real + name. The quoted spelling made it exit 1 with empty stdout, the code the loop + tolerates as "the files differ", so the file silently dropped out.""" + repo = project.project + _quote_path_default(repo) + base = verify.rev_parse_head(repo) + (repo / name).write_text("verbatim content\n") + + assert "+verbatim content" in verify.capture_diff(repo, base) + + def _touch_bytes_name(repo, raw): """Create an empty file whose name is the raw bytes ``raw`` (`Path` takes str only).""" with open(os.path.join(os.fsencode(repo), raw), "wb"): From ba18b33ce1dfee4e21be9e4ee4c641a0d546b903 Mon Sep 17 00:00:00 2001 From: t Date: Tue, 22 Sep 2026 18:04:22 -0700 Subject: [PATCH 4/4] docs(plugins): post_story runs in a kept worktree when teardown keeps it (#779) --- docs/plugin-authoring-guide.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/plugin-authoring-guide.md b/docs/plugin-authoring-guide.md index 2d3e2f977..7bbae9b96 100644 --- a/docs/plugin-authoring-guide.md +++ b/docs/plugin-authoring-guide.md @@ -384,9 +384,10 @@ there. | `pre_integrate` | before integrating a finished unit | — | | `pre_merge` / `post_merge` | around the local branch merge | — | -`post_story` fires after an isolated unit's worktree is torn down, so a declarative -`post_story` hook then runs from the repo root, and `BMAD_LOOP_WORKTREE` still names -the removed worktree. +`post_story` fires after an isolated unit's worktree teardown. If the worktree was +removed, a declarative `post_story` hook runs from the repo root and +`BMAD_LOOP_WORKTREE` still names the removed worktree; if it was kept (a deferred +unit under `keep_failed`), the hook runs from that worktree. ### Dev