fix(verify,plugins): verbatim untracked paths; post_story hooks after worktree teardown (#783, #779)

CHANGELOG.md

@@ -30,6 +30,15 @@ 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; 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`

docs/plugin-authoring-guide.md

@@ -384,6 +384,11 @@ 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 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
 
 | Stage                              | When                             | Mutable surface                                                                      |

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,
     )

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:

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)
@@ -7525,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
@@ -7543,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

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])

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.

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=<removed>)` 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.