Do not let git add's raw stderr cost the crashed agent's working tree

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

@@ -237,8 +237,11 @@ Under orchestrator ownership the worktree becomes a hot path.
    `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
+   name (a U+FFFD in `wip_paths`) rather than the whole path set. That
+   replacement does not move the softening decision in either direction:
+   every non-`*` character in the softening globs is ASCII and replacement
+   only substitutes non-ASCII for non-ASCII, so a replaced path matches
+   exactly the globs its raw bytes would. 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

orchestrator/agent_salvage.py

@@ -252,12 +252,23 @@ def _run_git(
     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.
+    NB-3) and ``errors="replace"`` is passed to the decode. The two are
+    complementary, not redundant: the pin keeps *most* of git's output ASCII
+    for the ``commit`` / ``status`` / ``diff`` calls here, and costs nothing
+    because this path has no ``-z`` read to make quoting a problem. It does
+    **not** cover ``git add``, which echoes the raw path in at least two of
+    its stderr messages regardless of ``quotePath`` (#3639 re-review B1)::
+
+        error: unable to index file 'caf\\xe9.txt'
+        error: 'nested-caf\\xe9/' does not have a commit checked out
+
+    Under a strict decode those bytes raise ``UnicodeDecodeError`` from
+    inside :func:`subprocess.run` — before this function returns — which
+    :func:`commit_working_tree` would swallow into "continuing", losing the
+    whole working tree over one filename. That is #3639 itself. The
+    non-strict decode is why it cannot happen; since no call here reads
+    ``-z`` output, replacement can only touch names that would otherwise
+    crash.
     """
     cmd = [
         "git",
@@ -275,6 +286,7 @@ def _run_git(
         cmd,
         capture_output=True,
         text=True,
+        errors="replace",
         check=check,
         timeout=timeout,
     )
@@ -720,14 +732,18 @@ def commit_working_tree(worktree: AgentWorktree) -> str | None:
     # 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
+    # that promise is not a subprocess error: 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.
+    # (a ``ValueError``) from inside ``subprocess.run``, before ``_run_git``
+    # returns. That is a live input class on this path — ``git add``'s
+    # stderr echoes the raw path in messages ``core.quotePath`` does not
+    # cover (#3639 re-review B1) — so ``_run_git`` decodes with
+    # ``errors="replace"`` and the raise cannot happen there. This handler is
+    # the second layer: catching it here rather than letting it escape keeps
+    # a future decode gap from aborting the committed-but-unpushed salvage
+    # that follows. Note what it costs when it *does* fire — the caller reads
+    # a WARNING about a hostile worktree, not about lost work — which is why
+    # the non-strict decode, not this handler, is the fix for the case above.
     except Exception as e:
         logger.warning(
             "Salvage: capturing uncommitted working tree raised; continuing",

orchestrator/kubernetes_spawner/_worktree.py

@@ -441,10 +441,13 @@ def _git(
                 "-c",
                 "commit.gpgsign=false",
                 # Pinned, not inherited (#3639 re-review NB-3). Every command
-                # here runs under ``text=True`` with a strict UTF-8 decode, so
-                # any git output that echoes a filename verbatim raises
-                # ``UnicodeDecodeError`` from inside ``run`` the moment a path
-                # in this tree is not valid UTF-8. The default
+                # here that does not opt into ``errors="replace"`` runs under
+                # a strict UTF-8 decode, so any git output that echoes a
+                # filename verbatim raises ``UnicodeDecodeError`` from inside
+                # ``run`` the moment a path in this tree is not valid UTF-8.
+                # (The two calls that *do* opt in are the ones git leaves
+                # unquoted regardless of this setting: the ``-z`` staged read,
+                # and ``add``, whose stderr echoes raw paths.) The default
                 # ``core.quotePath=true`` C-quote-encodes those bytes to ASCII
                 # and is what keeps ``status --porcelain`` and ``clean -fd``
                 # decodable; a worktree that inherited ``quotePath=false``
@@ -922,7 +925,17 @@ def _preserve_dirty_tree(
     partial = False
     try:
         try:
-            git(repo_dir, "add", "-A", "--ignore-errors", timeout=120)
+            # ``errors="replace"`` for the same reason the staged read below
+            # uses it, one layer earlier (#3639 re-review NB-3): ``git add``
+            # echoes the raw path in stderr messages that ``core.quotePath``
+            # does not cover (``unable to index file '<path>'``, ``'<path>/'
+            # does not have a commit checked out``), so a non-UTF-8 name in
+            # the tree raises ``UnicodeDecodeError`` inside ``run``. Under a
+            # strict decode that lands in the handler below and stamps
+            # ``INCOMPLETE:`` on a snapshot that may be perfectly complete —
+            # and disqualifies the soft branch — over a filename git only
+            # mentioned in passing.
+            git(repo_dir, "add", "-A", "--ignore-errors", timeout=120, errors="replace")
         except Exception as add_error:  # partial index beats no index
             partial = True
             logger.warning(
@@ -966,9 +979,19 @@ def _preserve_dirty_tree(
         # replacing across the whole stream is equivalent to decoding each
         # path separately. ``surrogateescape`` was rejected: lone surrogates
         # are unencodable by ``json.dumps`` and by a UTF-8 DB driver, which
-        # would move the failure downstream into the message bus. A replaced
-        # name still fails every glob in :func:`_path_matches_glob`, so it can
-        # only cost the softened wording, never grant it.
+        # would move the failure downstream into the message bus.
+        #
+        # Replacement does not perturb :func:`_path_matches_glob` at all — a
+        # replaced path matches exactly the globs the raw bytes would (#3639
+        # re-review NB-2). It is NOT that a U+FFFD fails every glob: the
+        # ``*`` in ``brc-memory*.md`` swallows one happily. The property is
+        # that replacement is confined to non-ASCII: every byte in an invalid
+        # sequence is >= 0x80, U+FFFD is non-ASCII too, and every non-``*``
+        # character in ``_MACHINE_STATE_FILE_GLOBS`` is ASCII — so a literal
+        # position can neither gain nor lose a match, and ``*`` regions are
+        # length-agnostic. Segment count is preserved for the same reason:
+        # ``/`` is 0x2F and never appears inside an invalid sequence. Keep the
+        # globs ASCII and this stays true of a new entry.
         #
         # The handler is deliberately ``Exception`` and not
         # ``UnicodeDecodeError`` (#3639 re-review B1). This read is *metadata
@@ -1171,7 +1194,9 @@ def _record_discarded_tip(
     soft branch states the paths themselves, so the count is redundant
     there and is not rendered.)
 
-    ``wip_partial`` marks a snapshot whose ``git add -A`` reported errors.
+    ``wip_partial`` marks a snapshot whose ``git add -A`` did not complete
+    cleanly — a non-zero exit, or a raise (timeout, undecodable stderr) with
+    no exit status at all, so the neutral phrasing is the only checkable one.
     It is surfaced because an agent that cherry-picks a silently truncated
     snapshot fails worse than one told the snapshot is truncated, and it
     also *disqualifies* the soft branch: a truncated capture's path list
@@ -1195,8 +1220,8 @@ def _record_discarded_tip(
     # wording is an opt-in for the demonstrably trivial case, never a default.
     #
     # ``wip_partial`` disqualifies the soft branch for the same reason
-    # ``wip_paths=None`` does. When ``git add -A`` reported errors the path
-    # list is *by construction* only the subset that reached the index — the
+    # ``wip_paths=None`` does. When ``git add -A`` did not complete cleanly the
+    # path list is *by construction* only the subset that reached the index — the
     # files that failed to stage are absent from it — so "every captured path
     # is a state file" says nothing about what was in the tree. A partial
     # capture is missing evidence by another name, and softening must never
@@ -1299,9 +1324,9 @@ def _record_discarded_tip(
     else:
         wip_text = ""
     partial_text = (
-        " WARNING: `git add -A` reported errors while taking this snapshot, so "
-        "it may be INCOMPLETE — files the previous session's working tree held "
-        "may be missing from it."
+        " WARNING: `git add -A` did not complete cleanly while taking this "
+        "snapshot, so it may be INCOMPLETE — files the previous session's "
+        "working tree held may be missing from it."
         if wip_commit and wip_partial
         else ""
     )

orchestrator/tests/test_agent_salvage.py

@@ -640,6 +640,69 @@ def _flaky_run_git(*args: str, **kwargs: object):
         # complete one.
         assert "INCOMPLETE" in _git("log", "-1", "--format=%B", cwd=wt.repo_path).stdout
 
+    def test_add_stderr_decode_is_non_strict(self, monkeypatch: pytest.MonkeyPatch) -> None:
+        """``_run_git`` must never decode git's output strictly (R9 B1).
+
+        The ``core.quotePath=true`` pin keeps the ``commit``/``status``/
+        ``diff`` output ASCII, but it does **not** cover ``git add``: two of
+        its stderr messages echo the path raw regardless of the setting. A
+        strict decode therefore raises ``UnicodeDecodeError`` from inside
+        ``subprocess.run`` on a worktree holding a non-UTF-8 name, and
+        :func:`commit_working_tree`'s ``except Exception`` turns the whole
+        lost working tree into a "continuing" WARNING — #3639 on the #2807
+        path. Pinned at the argv level as well as end-to-end below, because
+        the end-to-end fixture needs a filesystem that accepts raw bytes.
+        """
+        import agent_salvage
+
+        seen: dict[str, object] = {}
+
+        def _capture(cmd, **kwargs):
+            seen.update(kwargs)
+            seen["cmd"] = cmd
+            return SimpleNamespace(returncode=0, stdout="", stderr="")
+
+        monkeypatch.setattr(agent_salvage.subprocess, "run", _capture)
+        agent_salvage._run_git("add", "-A", cwd=Path("/tmp"), check=False)
+
+        assert seen["errors"] == "replace"
+        # The pin is complementary, not superseded: it still keeps the
+        # quoted-output calls ASCII.
+        assert "core.quotePath=true" in seen["cmd"]
+
+    def test_hostile_filename_does_not_cost_the_whole_working_tree(self, tmp_path: Path) -> None:
+        """One undecodable name must not discard hours of uncommitted work (R9 B1).
+
+        The seed is a nested repo with no commit checked out whose directory
+        name is latin-1: ``git add -A --ignore-errors`` reports ``error:
+        '<name>/' does not have a commit checked out`` with the path echoed
+        **raw** — a message ``core.quotePath`` does not quote — and keeps
+        going, so the real work still reaches the index. The only thing that
+        decides whether it is committed or handed to the gateway's reset is
+        whether ``_run_git`` can decode that stderr.
+
+        A latin-1 name is not exotic here: extracted archives (CP437/latin-1
+        entry names), fixtures written with raw bytes, and agent-cloned
+        nested repos all produce them.
+        """
+        import os
+
+        wt = self._clean_worktree(tmp_path)
+        (wt.repo_path / "work.py").write_text("def hours_of_work():\n    return 1\n")
+        nested = wt.repo_path / os.fsdecode(b"nested-caf\xe9")
+        nested.mkdir()
+        _git("init", "-q", cwd=nested)
+
+        sha = commit_working_tree(wt)
+
+        assert sha is not None, "the tree was lost to one filename"
+        assert (
+            _git("show", f"{sha}:work.py", cwd=wt.repo_path).stdout
+            == "def hours_of_work():\n    return 1\n"
+        )
+        # The add did report an error, so the snapshot is honestly labelled.
+        assert "INCOMPLETE" in _git("log", "-1", "--format=%B", cwd=wt.repo_path).stdout
+
     def test_complete_salvage_commit_is_not_marked_incomplete(self, tmp_path: Path) -> None:
         """The clean path must not carry the truncation marker."""
         wt = self._clean_worktree(tmp_path)

orchestrator/tests/test_kubernetes_spawner.py

@@ -4279,9 +4279,11 @@ def test_partial_suffixes_share_one_grep_token(self):
         quoted = shared.strip().split(", so")[0]
         assert quoted in runbook, f"agent-recovery.md no longer quotes: {quoted!r}"
         assert "git log --all --grep 'INCOMPLETE: `git add -A`'" in runbook
-        # ``--all`` is local refs only and both snapshot paths push to origin,
-        # so the runbook must name the fetch or the grep is a false negative
-        # on a fresh clone.
+        # ``--all`` does include ``refs/remotes/``, which is precisely why the
+        # fetch matters: both snapshot paths push to origin, so a fresh clone
+        # has no ref under ``refs/remotes/origin/egg/recovered/`` until the
+        # namespace is fetched and ``--all`` walks nothing. The runbook must
+        # name the fetch or the grep is a false negative there (R9 NB-4).
         assert "refs/heads/egg/recovered/*:refs/remotes/origin/egg/recovered/*" in runbook
 
     def test_no_branch_takes_no_snapshot(self, spawner, mock_gateway, tmp_path):
@@ -4572,9 +4574,11 @@ def test_undecodable_bytes_cost_one_name_not_the_path_set(self, tmp_path):
         that used to raise inside ``subprocess.run`` now yields U+FFFD for
         the bad bytes. Discarding all 33 paths for one bad byte was
         avoidable; ``routes/pipelines/_worktree_sync`` already reads its own
-        ``-z`` output this way. A replaced name still fails every glob in
-        ``_path_matches_glob``, so this can only cost the softened wording,
-        never grant it.
+        ``-z`` output this way. The replacement does not move the softening
+        decision either way (R9 NB-2): every non-``*`` character in
+        ``_MACHINE_STATE_FILE_GLOBS`` is ASCII and replacement only ever
+        substitutes non-ASCII for non-ASCII, so a replaced path matches
+        exactly the globs its raw bytes would.
         """
         from kubernetes_spawner._worktree import _preserve_dirty_tree
 
@@ -4642,8 +4646,11 @@ def test_undecodable_filename_is_salvaged_end_to_end(self, spawner, mock_gateway
         assert msg.metadata["wip_files"] == 3
         assert set(paths) >= {"seed.txt", "new_module.py"}
         assert any("�" in p for p in paths), paths
-        # A replaced name matches no softening glob, so the record still
-        # takes the imperative — the safe default is unchanged by NB-2.
+        # The imperative here is earned by ``seed.txt``/``new_module.py``,
+        # which match no softening glob — NOT by the replaced name, which
+        # matches whatever its raw bytes would (R9 NB-2). This asserts the
+        # end-to-end default is unchanged, not the replacement's effect;
+        # ``test_replacement_does_not_move_the_softening_decision`` covers that.
         assert msg.metadata["wip_machine_state_only"] is False
         assert msg.metadata["wip_softened"] is False
         assert "inspect it before starting work" in msg.body
@@ -4959,7 +4966,7 @@ def test_partial_snapshot_is_flagged_to_the_reader(self):
     def test_partial_machine_state_snapshot_keeps_the_imperative(self):
         """A truncated capture cannot earn the soft branch (R4 blocking #1).
 
-        When ``git add -A`` reports errors, ``wip_paths`` is by construction
+        When ``git add -A`` does not complete cleanly, ``wip_paths`` is by construction
         only the subset that reached the index — whatever failed to stage is
         absent from it. "Every captured path is a state file" then says
         nothing about the working tree, so this is the same missing evidence
@@ -5131,6 +5138,49 @@ def test_glob_star_does_not_cross_a_path_separator(self):
             )
             assert "inspect it before starting work" in body, path
 
+    def test_replacement_does_not_move_the_softening_decision(self):
+        """A U+FFFD in a path matches exactly what its raw bytes would (R9 NB-2).
+
+        The comment on the ``errors="replace"`` read used to claim a replaced
+        name "still fails every glob", which is false — the ``*`` in
+        ``brc-memory*.md`` swallows a U+FFFD happily, and the first case here
+        softens. The property that actually holds is neutrality: every
+        non-``*`` character in ``_MACHINE_STATE_FILE_GLOBS`` is ASCII and
+        replacement only ever substitutes non-ASCII (U+FFFD) for non-ASCII
+        (bytes >= 0x80), so a literal position can neither gain nor lose a
+        match and ``*`` regions are length-agnostic. Segment count survives
+        for the same reason: ``/`` is 0x2F and never appears inside an
+        invalid sequence. Keep the globs ASCII and a new entry inherits this.
+        """
+        replaced_memory = b".egg-state/agent-outputs/coder/brc-memory-caf\xe9.md".decode(
+            "utf-8", errors="replace"
+        )
+        replaced_output = b".egg-state/agent-outputs/pipe-caf\xe9-wontdo.json".decode(
+            "utf-8", errors="replace"
+        )
+        assert "�" in replaced_memory and "�" in replaced_output
+
+        # A state file whose name went through replacement still softens...
+        soft = self._body(
+            n_commits=1,
+            recovery_ref="egg/recovered/x",
+            wip_commit="aaaa1111",
+            wip_files=1,
+            wip_paths=(replaced_memory,),
+        )
+        assert "read it if you need it" in soft
+        assert "inspect it before starting work" not in soft
+
+        # ...and agent output whose name did still takes the imperative.
+        hard = self._body(
+            n_commits=1,
+            recovery_ref="egg/recovered/x",
+            wip_commit="aaaa1111",
+            wip_files=1,
+            wip_paths=(replaced_output,),
+        )
+        assert "inspect it before starting work" in hard
+
     def test_trivial_snapshot_with_failed_push_still_names_the_snapshot(self):
         """The opening-clause suppression is conditional on ``recovery_ref`` (R4 #4).