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

[james-in-a-box]

Round-9 review fixes for #3644 (base: issue-3639-preserve-dirty-worktree).

The blocking finding is the same failure class the parent PR fixes, on the #2807 sibling path. core.quotePath=true keeps most of git's output ASCII, but not git add's stderr: unable to index file '<path>' and '<path>/' does not have a commit checked out echo the path raw. On a worktree holding a latin-1 name, subprocess.run(..., text=True) raised UnicodeDecodeError before _run_git returned, commit_working_tree's except Exception logged "continuing", and the gateway's reset destroyed hours of uncommitted work — #3639 exactly, and the parent PR's own comments asserted it was unreachable.

_run_git now decodes with errors="replace". The quotePath pin stays (still correct for the commit/status/diff calls) with its claim narrowed to what it actually covers. The re-attach path's add gets the same keyword so an undecodable name cannot stamp INCOMPLETE: on a complete snapshot and disqualify the soft branch.

Also corrects three claims the reviewer falsified with real git: the reported errors wording on the agent-facing bus body and two comments (partial is set by a raise with no exit status, so only the neutral phrasing is checkable); the "a replaced name fails every glob" reasoning in four places (a * swallows U+FFFD happily — the true property is match-neutrality, since every non-* glob character is ASCII and replacement only substitutes non-ASCII for non-ASCII); and the --all note in test_partial_suffixes_share_one_grep_token.

Issue: #3639

Test Plan

• Automated: test_add_stderr_decode_is_non_strict pins the decode contract at the argv level; test_hostile_filename_does_not_cost_the_whole_working_tree drives a real failing add against a nested repo with a latin-1 directory name and asserts the work is still committed (fails without the fix); test_replacement_does_not_move_the_softening_decision pins the corrected NB-2 property in both directions.
• Manual: none.

Manual Steps

• None.

Authored-by: egg

[egg-reviewer]

No agent-mode design concerns.

This PR is scoped to subprocess decoding on the worktree-salvage paths (_run_git's errors="replace", the git add keyword in _preserve_dirty_tree) plus comment/test-docstring corrections. No prompt assembly, no LLM invocation path, no model identifiers, and no structured-output or post-processing surface is touched.

Worth noting positively, since the diff does edit agent-facing text: the re-attach bus body in _record_discarded_tip stays on the right side of guidelines 1 and 5 — it hands the recovering agent lightweight metadata (snapshot sha, recovery ref, staged path list, file count) and a git fetch origin <ref> plus "inspect it before starting work", rather than baking the snapshot diff into the message. The wip_partial change keeps that shape: the softened branch is still gated on positive evidence, and a snapshot whose git add -A did not complete cleanly falls back to the imperative that sends the agent to look for itself, which is the correct default when the path list is known-incomplete.

— Authored by egg

[egg-reviewer]

Review: PR #3660 — non-strict decode on the #2807 salvage path

Verdict: no blocking issue confirmed. The core fix is right and the B1 diagnosis holds up under scrutiny — I verified the four relevant git format strings exist in the installed git (2.34.1) and that they take the path as a bare %s, not a quote_path result:

'%s' does not have a commit checked out
unable to index file '%s'
adding embedded git repository: %s
HEAD is now at %s

Six findings below, all non-blocking. Two of them are falsifiable claims in the new prose — which matters more than usual on a PR whose stated purpose is correcting three claims a reviewer falsified with real git.

---

1. The forward-looking glob rule is wrong: "ASCII" is not the invariant, "literals and * only" is

orchestrator/kubernetes_spawner/_worktree.py:993-994:

# ``/`` is 0x2F and never appears inside an invalid sequence. Keep the
# globs ASCII and this stays true of a new entry.

and the same rule again in orchestrator/tests/test_kubernetes_spawner.py (test_replacement_does_not_move_the_softening_decision docstring): "Keep the globs ASCII and a new entry inherits this."

The neutrality argument for the current globs is correct — I checked it: _MACHINE_STATE_FILE_GLOBS is literals + * only, replacement never consumes an ASCII byte, so the ASCII skeleton is preserved. But the stated generalization does not follow, because replacement is not length-preserving and ? / […] are ASCII:

b"\xf0\x9f\x98".decode("utf-8", "replace")         -> 1 char (maximal-subpart collapse)
b"\xf0\x9f\x98".decode("utf-8", "surrogateescape") -> 3 chars
fnmatchcase("x\udcf0\udc9f\udc98.md", "x???.md")   -> True
fnmatchcase("x�.md",             "x???.md")   -> False

A future _MACHINE_STATE_FILE_GLOBS entry like brc-memory-??.md is ASCII, satisfies the rule as written, and breaks neutrality — a replaced path would lose a match its raw bytes had. The claim the code actually depends on is that * regions are length-agnostic, which is only true because there is no other length-sensitive metacharacter in play.

Suggested wording: "Keep the globs to ASCII literals and * — ? and […] are ASCII but length-sensitive, and replacement collapses a truncated multi-byte sequence to a single U+FFFD." Same edit in both places, and it's worth an assertion in the new test that the globs contain no ? or [, so the rule is enforced rather than requested.

2. _worktree.py's add change has no test at all — asymmetric with its sibling

orchestrator/kubernetes_spawner/_worktree.py:938 is one of the two production changes in this PR, and nothing covers it. The agent_salvage sibling got both an argv-level pin (test_add_stderr_decode_is_non_strict) and a real-git end-to-end (test_hostile_filename_does_not_cost_the_whole_working_tree); the _preserve_dirty_tree add got neither.

test_undecodable_bytes_cost_one_name_not_the_path_set asserts kwargs.get("errors") == "replace" only on the diff arm. Every fake git closure in TestDirtyTreePreservedBeforeReset takes **_kwargs and never inspects the add arm — _fake_git, _flaky_git, _replacing_git, _failing_read_git all swallow it. Delete errors="replace" from line 938 and the suite is still green.

Worse, the case the new comment names — "stamps INCOMPLETE: on a snapshot that may be perfectly complete" — has a clean real-git seed the PR didn't use, and it isn't either of the two messages the docstrings cite. Both unable to index file '%s' and '%s' does not have a commit checked out accompany a non-zero exit, so under a strict decode the snapshot would have been marked partial anyway. The message that produces a false INCOMPLETE is the third one:

warning: adding embedded git repository: <raw path>

check_embedded_repo() in builtin/add.c — raw path, and git add exits 0. A nested repo that does have a commit checked out (unlike the agent_salvage seed, which deliberately has none) gives you exactly the false-positive scenario, end to end, in the class that already has real-git seeds (_seed_dirty, test_undecodable_filename_is_salvaged_end_to_end). That's the test this change is missing, and it would also make the third message visible in the docstrings.

3. errors="replace" is opt-in in _git but unconditional in _run_git — and the justification for opt-in is not exhaustive

orchestrator/kubernetes_spawner/_worktree.py:441-452, new text:

(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.)

core.quotePath governs path quoting. It says nothing about commit messages, author names, or remote sideband, and three remaining strict calls carry those:

• _worktree.py:542 and :712 — git reset --hard prints HEAD is now at <abbrev> <subject> (print_new_head_line() → pp_commit_easy(CMIT_FMT_ONELINE, …)). Commit subjects are raw object bytes; quotePath never touched them.
• _worktree.py:573 — git fetch relays remote: sideband bytes verbatim.

Blast radius at :573 is the #3639 shape exactly: _preserve_dirty_tree has already made the WIP snapshot commit, the salvage push at :630 has not run yet, and a decode failure hits except Exception → return False → _try_reuse_worktree returns None → create-with-retry deletes the worktree and the unpushed snapshot with it.

I can't produce a non-UTF-8 commit subject here to reproduce it (git init is gateway-blocked in this sandbox), so this is PLAUSIBLE, not confirmed — hence non-blocking. But note :542 is safe only by accident: at that point HEAD is the WIP commit, whose subject is ASCII. That invariant is nowhere written down and evaporates the moment _preserve_dirty_tree returns None.

Given agent_salvage._run_git just made the decode unconditional, the asymmetry is the finding. Suggest flipping _git's signature to errors: str | None = "replace" and letting the quotePath pin claim only what it actually does. That deletes the per-call footgun and the enumeration that has now been wrong twice.

4. The _run_git docstring understates the fix — list_unpushed_commits was also exposed

orchestrator/agent_salvage.py:253-256:

the pin keeps most of git's output ASCII for the commit / status / diff calls here

since no call here reads -z output, replacement can only touch names that would otherwise crash

list_unpushed_commits (agent_salvage.py:524-539) runs:

"--format=%H%x1f%s%x1f%an%x1f%aI",

%s and %an are raw commit-object bytes — not paths, never covered by quotePath. Before this PR, one non-UTF-8 commit subject anywhere in the salvage range made that subprocess.run raise UnicodeDecodeError, and neither handler catches it (except subprocess.CalledProcessError at :540, except (OSError, subprocess.SubprocessError) at :548 — UnicodeDecodeError is a ValueError), so it escaped list_unpushed_commits and salvage_worktree outright. This PR fixes that too.

That's a strictly bigger win than the docstring claims, and the enumeration style ("the calls here are commit/status/diff") is precisely what let B1 hide. Suggest stating the rule instead of the list: core.quotePath quotes paths; it has no bearing on commit messages, author names, or remote sideband, so the decode must be non-strict unconditionally.

5. commit_working_tree's "never raises" contract still has a hole outside the try

agent_salvage.py:677-681:

    if not _is_git_worktree(worktree.repo_path):
        return None
    if not _has_uncommitted_changes(worktree.repo_path):
        return None
    try:

_has_uncommitted_changes (:646-652) catches only (OSError, subprocess.SubprocessError). salvage_worktree calls commit_working_tree(worktree) unguarded, so anything else raised there propagates past the docstring's "Never raises" promise and past the new except Exception block, whose comment now describes itself as "the second layer". The second layer doesn't cover the first two calls. errors="replace" closes the specific UnicodeDecodeError route through it, so this is latent rather than live — but it's a one-line fix to move both guards inside the try, and the comment currently implies coverage it doesn't have.

6. Two smaller drift items

• agent_salvage.py:88 still opens with "Appended when git add -A reported errors" while lines 105-110 of the same comment block explain why the shared wording is "did not complete cleanly". The phrasing is technically accurate on this path (partial = add.returncode != 0, no raise), but the block contradicts its own opening sentence. Same for commit_working_tree's docstring at :665-668. The PR normalised this in four places in _worktree.py and in the bus body; these two are the leftovers.
• orchestrator/agent_salvage_cleanup.py:83-98 — docstring says "Mirrors agent_salvage._run_git." After this PR it mirrors neither the core.quotePath=true pin nor errors="replace". Its two call sites read %cI and %(refname), so the exposure is negligible, but the claim is now false.

---

What I verified and what I did not

Verified: the four git format strings and their bare-%s shape in git 2.34.1; that UTF-8 errors="replace" never consumes an ASCII byte (so the segment-count and literal-position arguments hold); that the ? counterexample in finding 1 breaks neutrality; that test_replacement_does_not_move_the_softening_decision drives the production _record_discarded_tip via _message rather than a hand-built body; that both new fixture paths (os.fsdecode round-trip, git init -q suppressing its non-ASCII stdout) work on a Linux UTF-8/surrogateescape filesystem encoding; that test_hostile_filename_does_not_cost_the_whole_working_tree fails without the fix (status --porcelain is quoted so it reaches the add, which then raises inside run and is swallowed into return None).

Not verified: the tests themselves. There is no .venv in this container and git init is refused by the gateway, so I could not execute either new real-git test. CI is the ground truth for those.

— Authored by egg

[james-in-a-box]

egg is addressing review feedback...

[james-in-a-box]

Round-10: all six findings fixed in PR (commit f3b7f4d)

Every item was actionable and none needed a decision I couldn't make, so all six landed here.

1. Forward-looking glob rule is wrong — "ASCII" isn't the invariant — fixed-in-PR (commit f3b7f4d)

Agreed, and the counterexample is decisive. Replacement collapses a truncated multi-byte sequence to a single U+FFFD, so length-sensitivity is the real axis and ? / […] are ASCII but length-sensitive. Rule reworded to "ASCII literals and *" in all three places (_worktree.py, the test docstring, and docs/architecture/on-demand-agent-lifecycle.md, which carried the same generalization), with your fnmatchcase counterexample inline.

Taken the further suggestion too: test_replacement_does_not_move_the_softening_decision now asserts _MACHINE_STATE_FILE_GLOBS contains no ? or [, and that each glob minus its *s is ASCII — enforced, not requested.

2. _worktree.py's add change has no test — fixed-in-PR (commit f3b7f4d)

Agreed on both halves, and the seed you identified is the right one. Two tests:

• test_add_decode_is_non_strict — argv-level pin on the add arm, the sibling the diff arm already had. You were right that deleting the keyword left the suite green; it now doesn't.
• test_embedded_repo_warning_does_not_forge_a_partial_snapshot — real git, _seed_dirty + a nested repo with a commit checked out under a latin-1 directory name. adding embedded git repository: <raw path> exits 0, so it is the only one of the three that manufactures a false INCOMPLETE: on a complete capture; asserts wip_partial is False, no INCOMPLETE in the commit message, and no may be INCOMPLETE in the bus body. The third message is now named in the _worktree.py and agent_salvage.py docstrings alongside the other two.

3. errors="replace" opt-in in _git vs unconditional in _run_git — fixed-in-PR (commit f3b7f4d)

Agreed — the enumeration was the bug, twice. _git's signature is now errors: str | None = "replace", so reset --hard (:542, :712), fetch (:573), status, clean, and rev-parse are all covered. The quotePath comment now claims only path quoting and explicitly disclaims the rest, calling out the :573 blast radius you traced (post-snapshot, pre-push) as the reason it's a default and not a per-call opt-in.

One deliberate non-removal: _preserve_dirty_tree keeps the explicit errors="replace" at both call sites, because git is a parameter there — the closure's default doesn't govern an injected callable, and that call site is what finding #2's argv-level test pins. Commented as such.

4. _run_git docstring understates the fix — list_unpushed_commits was exposed — fixed-in-PR (commit f3b7f4d)

Agreed. Docstring restructured to state the rule (quotePath quotes paths; commit messages, author names, and sideband are outside it, so the decode must be non-strict unconditionally) with the two exposure classes as illustrations rather than an enumeration. list_unpushed_commits's %s/%an is now named, including that UnicodeDecodeError is a ValueError and so escaped both of its handlers and salvage_worktree outright before this PR.

5. commit_working_tree's "never raises" hole outside the try — fixed-in-PR (commit f3b7f4d)

Agreed. Both _is_git_worktree and _has_uncommitted_changes moved inside the try. The handler comment now says it spans every git call in the body including the guards — a "second layer" that skips the first two calls isn't one.

6a. Two "reported errors" leftovers — fixed-in-PR (commit f3b7f4d)

Agreed. agent_salvage.py:88 and commit_working_tree's docstring both now read "did not complete cleanly", matching the four places _worktree.py normalised. (The logger.warning on that path keeps "reported errors" — partial = add.returncode != 0 there, so it's checkable, and it's a log field rather than the shared commit-message wording the block is about.)

6b. agent_salvage_cleanup._run_git's stale "Mirrors" claim — fixed-in-PR (commit f3b7f4d)

Agreed the claim was false; fixed by making it true rather than by narrowing it. Added the core.quotePath=true pin and errors="replace". Not purely cosmetic: %(refname) is raw ref bytes that quotePath doesn't cover, so a single non-UTF-8 recovery ref would have raised out of the sweep. Docstring notes commit.gpgsign=false as the one setting deliberately not mirrored (this helper never commits).

---

Verification

make lint — ruff check and format clean. The three mypy errors are pre-existing in shared/egg_agent/client.py, untouched by this branch.

Per-test, not the full suite: the three fixture-level tests (test_replacement_does_not_move_the_softening_decision, test_add_decode_is_non_strict, test_undecodable_bytes_cost_one_name_not_the_path_set) pass. Failure lists before/after this commit differ by exactly one entry — the new test_embedded_repo_warning_does_not_forge_a_partial_snapshot, which fails at _make_worktree's git init with the same gateway refusal that already fails the 11 sibling real-git tests in that class. Same constraint you hit; CI is the ground truth for both new real-git tests.

— Authored by egg

[james-in-a-box]

egg feedback addressed. View run logs

2 previous review(s) hidden.