Never let a filename cost the working tree

docs/architecture/on-demand-agent-lifecycle.md

@@ -202,16 +202,24 @@ Under orchestrator ownership the worktree becomes a hot path.
    record says the snapshot was *not* pushed and asks for escalation
    rather than reassuring the successor that nothing was lost. The ask
    also scales with *what* the snapshot captured: when every captured path
-   is an **orchestrator-written** state file, the record softens to "read
-   it if you need it" so a routine respawn does not train the #3509
-   message into background noise. The allowlist is
-   `.egg-state/agent-outputs/*/brc-memory*.md` (rewritten on every
-   `brc_ack`/`brc_nack`), `.egg-state/agent-outputs/consensus-confirmed`,
-   and `.egg-state/agent-outputs/<pipeline-id>-apply-handoff.json`;
-   matching is segment-wise so `*` does not cross `/`. Files written by
-   *agents* into the same directory — `<pipeline>-wontdo.json`,
-   `<identifier>-tester-output.json` — are deliberately excluded: they are
-   agent output, and losing them warrants the imperative. Anything else —
+   is a **state file the next event regenerates and some other store
+   durably holds**, the record softens to "read it if you need it" so a
+   routine respawn does not train the #3509 message into background noise.
+   The membership test is regeneration, not authorship — the dominant
+   member is written by the *sandbox* on the agent's own tool call and
+   holds agent-authored prose. The allowlist is
+   `.egg-state/agent-outputs/*/brc-memory*.md` (rewritten by
+   `sandbox/egg_agent_tools/handlers/brc_memory.py` on every
+   `brc_ack`/`brc_nack`, with the orchestrator message history as the
+   durable backstop — see
+   [brc-memory.md](brc-memory.md)),
+   `.egg-state/agent-outputs/consensus-confirmed`, and
+   `.egg-state/agent-outputs/<pipeline-id>-apply-handoff.json`;
+   matching is segment-wise so `*` does not cross `/`. Agent *output* in
+   the same directory — `<pipeline>-wontdo.json`,
+   `<identifier>-tester-output.json` — is deliberately excluded: nothing
+   rewrites it on the next event and no other store holds it, so losing it
+   warrants the imperative. Anything else —
    including an unrecognised or unknown file set — keeps the imperative
    "inspect it before starting work", as does a snapshot flagged partial
    (a truncated capture's path list omits whatever failed to stage, so it
@@ -223,8 +231,19 @@ Under orchestrator ownership the worktree becomes a hot path.
    reconstruct the decision instead of regexing the prose; the two derived
    fields diverge whenever a machine-state-only path set is disqualified
    by a commit stack, a truncated capture, or a failed salvage push. The
-   threshold selects wording only; the snapshot itself is always taken. A
-   snapshot whose `git add -A` reported errors is marked incomplete in
+   threshold selects wording only; the snapshot itself is always taken —
+   including when the path list cannot be read at all. The staged-path
+   read uses `-z` so `wip_paths` carries real bytes rather than
+   `core.quotePath` C-quoted tokens, which means a filename that is not
+   valid UTF-8 would be undecodable under `subprocess`'s strict `text=True`
+   decode; the read passes `errors="replace"`, so one bad name costs one
+   name (a U+FFFD in `wip_paths`, which matches no softening glob) rather
+   than the whole path set. Anything that still defeats that read — a
+   timeout on a large staged set, a non-zero `diff` against a locked index
+   — logs a WARNING and commits blind (`wip_paths`/`wip_files` become
+   `null`, so the record takes the imperative) rather than letting a
+   metadata read cost the working tree. A
+   snapshot whose `git add -A` did not complete cleanly is marked incomplete in
    both its commit message and the bus record, since a truncated snapshot
    is otherwise indistinguishable downstream from a complete one. The same
    marker rides the #2807 crash-salvage commit

docs/reference/agent-recovery.md

@@ -380,7 +380,16 @@ git switch <target-branch>
 git cherry-pick <recovered-base>..recovered/<scope>
 ```
 
-**Review before replaying: a recovery ref may hold un-reviewed working-tree residue.** Both working-tree snapshot paths ([#2807](https://github.com/jwbron/egg/issues/2807) restart, [#3639](https://github.com/jwbron/egg/issues/3639) re-attach) stage with `git add -A`, so a snapshot commit contains everything the agent left in the worktree that is not `.gitignore`d — scratch dumps, logs, stray state files — with no agent or human intent behind any of it. Recovery refs are a preservation mechanism, not an endorsement: read the diff before cherry-picking, and expect a snapshot-only ref to sometimes hold nothing you want. Reading the diff before replaying is the control here: **nothing in this repo enables GitHub push protection**, so do not assume a snapshot containing a secret is stopped on the way out. (Where push protection *is* enabled on the receiving repo it rejects such a push rather than leaking it, and the discard is then recorded with `salvage_error` set instead of a recovery ref — the failure branch described above.)
+**Review before replaying: a recovery ref may hold un-reviewed working-tree residue.** Both working-tree snapshot paths ([#2807](https://github.com/jwbron/egg/issues/2807) restart, [#3639](https://github.com/jwbron/egg/issues/3639) re-attach) stage with `git add -A`, so a snapshot commit contains everything the agent left in the worktree that is not `.gitignore`d — scratch dumps, logs, stray state files — with no agent or human intent behind any of it. Recovery refs are a preservation mechanism, not an endorsement: read the diff before cherry-picking, and expect a snapshot-only ref to sometimes hold nothing you want. **A snapshot may also be incomplete**: when `git add -A` did not complete cleanly the commit holds only what reached the index, and its message carries an ``INCOMPLETE: `git add -A` did not complete cleanly while staging`` paragraph saying so — `git log -1 <ref>` before you conclude the ref holds everything the working tree did. On the [#3639](https://github.com/jwbron/egg/issues/3639) re-attach path a bus record repeats that warning, but on the [#2807](https://github.com/jwbron/egg/issues/2807) crash-salvage path (`commit_working_tree`) there is no bus record at all, so the commit message is the only channel a triager gets. To find truncated snapshots from either path, fetch the recovery namespace first — both paths push to `egg/recovered/*` on **origin**, so a fresh clone has no local ref for them and `git log --all` alone would report zero:
+
+```bash
+git fetch origin 'refs/heads/egg/recovered/*:refs/remotes/origin/egg/recovered/*'
+git log --all --grep 'INCOMPLETE: `git add -A`'
+```
+
+The grep token — the leading `INCOMPLETE:` plus the backticked `git add -A` — is byte-identical in both paths' suffixes (`_WIP_COMMIT_PARTIAL_SUFFIX`, `_UNCOMMITTED_SALVAGE_PARTIAL_SUFFIX`) and is pinned against this file by `test_partial_suffixes_share_one_grep_token`. Keep the backticks: they are part of the commit message, so a pattern without them matches nothing and the zero results read as "no truncated snapshots".
+
+Reading the diff before replaying is the control here: **nothing in this repo enables GitHub push protection**, so do not assume a snapshot containing a secret is stopped on the way out. (Where push protection *is* enabled on the receiving repo it rejects such a push rather than leaking it, and the discard is then recorded with `salvage_error` set instead of a recovery ref — the failure branch described above.)
 
 Operators may delete `egg/recovered/*` refs manually after replay (`git push origin --delete <ref>`). For automatic cleanup of refs left behind by replays that never came, see [Recovery Ref Cleanup](#recovery-ref-cleanup) below.
 

orchestrator/agent_salvage.py

@@ -98,13 +98,22 @@
 # there), which is the one thing a triager reading a lone commit message cannot
 # infer. The grep token — the leading ``INCOMPLETE:`` and the ``git add -A``
 # phrase — is identical in both, so one search finds every truncated snapshot
-# regardless of which path took it. Change one, change the other.
+# regardless of which path took it, and ``docs/reference/agent-recovery.md``
+# quotes it verbatim for triagers. Change one, change the other — and the
+# runbook.
+#
+# "did not complete cleanly" rather than "reported errors" (#3639 re-review
+# NB-6): the shared wording has to hold on the re-attach path too, where a
+# ``TimeoutExpired`` sets ``partial`` without git ever reporting an exit
+# status. Here the add is run with ``check=False`` and ``partial`` really is
+# ``returncode != 0``, but a claim the commit message cannot make on both
+# paths is not one worth keeping on either.
 _UNCOMMITTED_SALVAGE_PARTIAL_SUFFIX = (
     "\n"
     "\n"
-    "INCOMPLETE: `git add -A` reported errors while staging, so files\n"
-    "present in the crashed agent's working tree may be missing from\n"
-    "this commit."
+    "INCOMPLETE: `git add -A` did not complete cleanly while staging, so\n"
+    "files present in the crashed agent's working tree may be missing\n"
+    "from this commit."
 )
 _SALVAGE_COMMIT_NAME = "egg-salvage"
 _SALVAGE_COMMIT_EMAIL = "egg-salvage@localhost"
@@ -241,13 +250,23 @@ def _run_git(
     ``commit.gpgsign=true`` from the clone's config: there is no signing key
     in the orchestrator image, so every salvage commit would otherwise fail
     and lose the working tree it exists to save.
+
+    ``core.quotePath=true`` is pinned rather than inherited (#3639 re-review
+    NB-3). Every call here decodes with ``text=True`` and no ``errors=``, so
+    git output that echoes a filename verbatim raises ``UnicodeDecodeError``
+    the moment a path in the worktree is not valid UTF-8. The default
+    C-quote-encodes those bytes to ASCII and is what keeps that from
+    happening; a worktree that inherited ``quotePath=false`` would break it.
+    Costs nothing — this path has no ``-z`` read to make quoting a problem.
     """
     cmd = [
         "git",
         "-c",
         "core.hooksPath=/dev/null",
         "-c",
         "commit.gpgsign=false",
+        "-c",
+        "core.quotePath=true",
         "-C",
         str(cwd),
         *args,
@@ -698,10 +717,27 @@ def commit_working_tree(worktree: AgentWorktree) -> str | None:
             return None
         head = _run_git("rev-parse", "HEAD", cwd=worktree.repo_path, check=False)
         head_sha = (head.stdout or "").strip() if head.returncode == 0 else None
-    except (OSError, subprocess.SubprocessError) as e:
+    # Deliberately broader than the ``(OSError, subprocess.SubprocessError)``
+    # the read helpers above use, and broader than it needs to be today. The
+    # docstring promises this never raises, and the class that would break
+    # that promise is not a subprocess error: ``_run_git`` decodes with
+    # ``text=True`` and no ``errors=``, so a git command that echoes a
+    # filename whose bytes are not valid UTF-8 raises ``UnicodeDecodeError``
+    # (a ``ValueError``) from inside ``subprocess.run``. Unreachable on this
+    # path today — it has no ``-z`` call and the default ``core.quotePath``
+    # keeps git's output ASCII — but the re-attach path shipped exactly that
+    # bug by adding ``-z`` (#3639 re-review B1), and letting it escape here
+    # would abort the committed-but-unpushed salvage that follows.
+    except Exception as e:
         logger.warning(
             "Salvage: capturing uncommitted working tree raised; continuing",
             worktree_id=worktree.worktree_id,
+            # The breadth above is the point, but it makes an
+            # ``AttributeError`` from a future refactor render identically to
+            # a subprocess failure. The class name is the one field that
+            # separates "the worktree was hostile" from "this code is broken"
+            # (#3639 re-review NB-5).
+            error_type=type(e).__name__,
             error=str(e),
         )
         return None