Never let a filename cost the working tree

[james-in-a-box]

Never let a filename cost the working tree

Round-4 review fixes for #3644. The blocking finding (B1) is a real regression the R6 change introduced: switching the snapshot's staged-path read to git diff --cached --name-only -z moved the failure mode below git's output layer. Without -z, core.quotePath guarantees ASCII output; with it, unmunged path bytes reach the caller's subprocess.run(..., text=True), which decodes as strict UTF-8 — so a single non-UTF-8 filename raises UnicodeDecodeError inside run. The outer best-effort handler swallowed it, abandoned the snapshot commit, and handed the entire working tree to the hard reset. That is #3639 itself, triggered by one filename.

The fix keeps -z — R6 bought real metadata fidelity and giving it back would be the wrong trade — and instead catches the decode at the read, logging a WARNING and committing blind. _DirtySnapshot.paths/n_files widen to None, which the existing wording layer already reads as "take the imperative". Only a known-empty index still skips the commit; staged is None means "could not tell", and this helper never discards a tree on a maybe. errors="surrogateescape" was deliberately not used: lone surrogates are unencodable by a UTF-8 DB driver and by json.dumps, which would move the failure downstream into the message bus.

N1 corrects a provenance claim the softening rule did not have: brc-memory is written by the sandbox on the agent's own brc_ack/brc_nack tool call and holds agent-authored prose. The membership rule is now regeneration-plus-backstop, which admits brc-memory on its actual merits and still excludes wontdo.json / tester-output.json. N2 widens commit_working_tree's except to Exception (its docstring promises it never raises; the class that would break that is UnicodeDecodeError, not a subprocess error). N3 couples the two INCOMPLETE: grep tokens with a test, N4 documents incomplete snapshots and the grep, N5 carries wip_partial/preserved_files on the discard WARNING.

Issue: #3639

Test Plan

• Automated: test_undecodable_staged_path_does_not_cost_the_commit (unit — asserts the commit is still taken and only paths/n_files degrade to None), test_undecodable_filename_is_salvaged_end_to_end (real-git — a latin-1 filename in the tree, asserting the WIP commit holds the edits, the message takes the imperative wording, and the worktree lands clean at the origin tip), test_partial_suffixes_share_one_grep_token (N3).
• Manual: none required.
• Local caveat: git init is gateway-blocked in the agent container, so the 10 real-git cases in TestDirtyTreePreservedBeforeReset — including the new end-to-end one — cannot execute here and fail with CalledProcessError on _make_worktree. Same caveat as prior rounds; CI is ground truth. The 28 non-real-git cases pass locally, including both new unit tests.
• make lint: ruff and format clean. Mypy reports 3 pre-existing errors in shared/egg_agent/client.py, a file this PR does not touch.

Manual Steps

• None.

[egg-reviewer]

No agent-mode design concerns.

This is orchestrator git-plumbing (_preserve_dirty_tree, commit_working_tree) plus the wording of the discarded-tip bus record. No prompt assembly, no LLM invocation, no model identifiers, no output parsing — the four linter-adjacent anti-patterns (EGG100/EGG200/EGG201, direct API calls) have no surface here.

The one agent-facing surface, _record_discarded_tip, is a good instance of the guide's "orientation, not pre-fetching" pattern and this PR strengthens it:

• The record hands the agent a pointer (git fetch origin <recovery_ref>) rather than baking snapshot contents into the message — the agent pulls what it needs. orchestrator/kubernetes_spawner/_worktree.py:1146-1152.
• The B1 fix keeps the snapshot when the path list is undecodable and degrades only the metadata. The resulting wip_paths=None routes through _is_machine_state_only(None) is False to the imperative branch, so an unknown file set tells the agent to go look rather than reassuring it that nothing was lost. That's the right default under guideline 5 — the orchestrator declines to decide relevance on incomplete information and lets the agent judge.
• The _MACHINE_STATE_FILE_GLOBS softening selects wording only; the ref is always pushed and always named, so nothing is withheld from the agent in either branch (_worktree.py:1118-1121). The N1 re-derivation from "orchestrator-written" to "regenerated with a durable backstop" is a provenance correction, not a widening of what gets hidden — wontdo.json / tester-output.json stay excluded, and the criterion stated at _worktree.py:733-738 is testable against a new entry.
• Message body stays natural-language prose; the structured fields alongside it (wip_paths, wip_files, wip_partial) are genuinely machine-consumed by downstream matching, which is the sanctioned case under guideline 2 rather than JSON-for-humans.

Docs changes are consistent with the code: on-demand-agent-lifecycle.md and agent-recovery.md both describe the degradation honestly ("commits blind", "git log -1 <ref> before you conclude the ref holds everything").

Correctness of the UnicodeDecodeError handling, the except Exception widening, and the real-git test coverage are outside this lens — the general review bot's scope.

— Authored by egg

[egg-reviewer]

Review: PR #3656 — Never let a filename cost the working tree

I traced B1 through the real execution path and confirmed the diagnosis is correct: UnicodeDecodeError is a ValueError, the outer except Exception catches it, and the pre-fix behaviour was to abandon the snapshot commit and hand the tree to the reset. The fix is the right shape. N1's provenance correction is also correct and I verified it against the source rather than taking the claim at face value.

Two blocking findings. Neither is in the reasoning — both are in how far the fix reaches.

---

Blocking

B-1 — The decode guard is class-specific; any other failure of the same metadata-only read still costs the whole working tree, and the new docstring claims otherwise

orchestrator/kubernetes_spawner/_worktree.py:913-926

except UnicodeDecodeError as decode_error:

The new docstring (_worktree.py:842-845) promises:

Nothing short of "no commit exists" returns None: a staged-path list that cannot be read degrades the snapshot's metadata (paths/n_files become None) and never its existence.

and docs/architecture/on-demand-agent-lifecycle.md:234-235 promises:

The threshold selects wording only; the snapshot itself is always taken — including when the path list cannot be read at all.

Neither holds. I ran the production helper against the same seam the new unit test uses, varying only the exception class the diff read raises:

UnicodeDecodeError     -> snapshot=TAKEN     commit_attempted=True
TimeoutExpired(60s)    -> snapshot=ABANDONED commit_attempted=False
CalledProcessError     -> snapshot=ABANDONED commit_attempted=False

The two abandoned cases fall to the outer except Exception at _worktree.py:948, log "could not preserve uncommitted work; the hard reset below WILL discard it", and return None before the commit is ever attempted. reset --hard + clean -fd then destroy the tree and the orphan detector finds nothing to salvage. That is #3639 verbatim, with the trigger moved from "one bad filename" to "the metadata read was slow or exited non-zero".

Both triggers are concrete:

• Timeout. The read is git diff --cached --name-only -z with timeout=60, called immediately after git add -A staged the entire dirty tree. 33 files is the small case; a hostPath-mounted worktree with a large staged set on a contended node can exceed 60s.
• Non-zero exit. The call inherits check=True. The git add -A above is wrapped in except Exception → partial = True (_worktree.py:880-890), so an add that timed out at 120s is swallowed and execution continues while the git process may still hold index.lock. The following diff --cached returning non-zero for any reason (locked or half-written index) is then CalledProcessError → total loss.

The asymmetry argument the PR makes for commit_working_tree in this same diff applies here with more force:

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 […]

That reasoning widened the handler on the path where the bug is unreachable (agent_salvage.py:689, no -z, ASCII output) while leaving the path where the bug shipped on a single exception class. The priority is inverted: commit_working_tree's promise is about not raising; _preserve_dirty_tree's promise is about not losing the working tree.

Fix — widen the read's handler. The cost is strictly one-directional: staged = None can only lose the softening (_is_machine_state_only(None) is False) and the size claim. There is no input for which widening makes the record quieter or wronger — it can only trade path metadata for a commit. And if the index genuinely is empty, the git commit below still exits non-zero and falls through to the outer handler exactly as today, so the known-empty case does not regress.

try:
    staged_out = git(repo_dir, "diff", "--cached", "--name-only", "-z", timeout=60).stdout
    staged: list[str] | None = [p for p in staged_out.split("\0") if p]
except Exception as read_error:
    logger.warning(
        "Worktree re-attach: staged-path list could not be read (non-UTF-8 "
        "filename, timeout, or a failed diff); committing the snapshot "
        "without a path set",
        agent_worktree_id=agent_worktree_id,
        repo=repo,
        dirty_entries=n_entries,
        dirty_state_unknown=state_unknown,
        undecodable=isinstance(read_error, UnicodeDecodeError),
        error=str(read_error),
    )
    staged = None

Add the sibling unit test with subprocess.TimeoutExpired. test_undecodable_staged_path_does_not_cost_the_commit pins only the class that was fixed, which is what makes this gap invisible — a test named for the invariant ("does not cost the commit") that only covers one of the three ways to lose it.

B-2 — The INCOMPLETE: grep this PR adds to the runbook (N4) is malformed Markdown and, as rendered, silently matches nothing

docs/reference/agent-recovery.md:383. Two broken code spans on the new sentence:

`INCOMPLETE: \`git add -A\` reported errors while staging`
`git log --all --grep 'INCOMPLETE: `git add -A`'`

Per CommonMark, backslash escapes do not work inside code spans, and a single-backtick span cannot contain a backtick. Both parse as two adjacent code spans with the inner backticks consumed as delimiters:

• the first renders as INCOMPLETE: \ + plain git add -A\ + reported errors while staging
• the second renders as git log --all --grep 'INCOMPLETE: + plain git add -A + '

So the command an operator copies out of the rendered page is git log --all --grep 'INCOMPLETE: git add -A' — backticks gone. The commit-message token does contain them (_WIP_COMMIT_PARTIAL_SUFFIX = "INCOMPLETE: `git add -A` reported errors while staging…"), so that pattern matches nothing. Zero results reads as "no truncated snapshots" — the precise false negative this paragraph was added to prevent, and on the #2807 path the same paragraph correctly notes the commit message is the only channel a triager gets.

Fix — double-backtick fences:

``INCOMPLETE: `git add -A` reported errors while staging``
``git log --all --grep 'INCOMPLETE: `git add -A`'``

Worth correcting in the same edit: git log --all covers local refs only. Both snapshot paths push to egg/recovered/* on origin, so a triager on a fresh clone has no ref for them until they fetch. As written, "finds every truncated snapshot from either path" is false on the common triage setup — either name the fetch (git fetch origin 'refs/heads/egg/recovered/*:refs/remotes/origin/egg/recovered/*') or drop the "from either path" claim.

Note that N3 cannot catch this: test_partial_suffixes_share_one_grep_token hardcodes the literal rather than reading the doc, so the constants stay coupled to each other but not to the runbook. Asserting the token is a substring of docs/reference/agent-recovery.md would close it.

---

Non-blocking

NB-1 — N5's new log fields are named off-pattern, and None renders as blank. _worktree.py:648-653 emits wip_partial=…, preserved_files=wip_files on a line whose sibling snapshot field is wip_commit, and whose bus-record counterpart calls the count wip_files. N5's stated goal is "one query over this WARNING"; as written a consumer queries wip_files on the bus and preserved_files in the log. Rename to wip_files. Second-order: running the real logger with wip_files=None renders preserved_files= (empty), which reads as zero files preserved rather than unknown — on the exact B1 path N5 exists to make queryable. This is the ambiguity dirty_state_unknown was introduced elsewhere to avoid; consider a companion wip_files_unknown=True.

NB-2 — Discarding all 33 paths for one bad byte is avoidable, and this repo already has the better pattern. orchestrator/routes/pipelines/_worktree_sync.py:1326-1352 performs the same -z read in bytes mode (no text=True, deleted.stdout.split(b"\0")) for exactly this reason, with a comment saying so. Decoding per-path (errors="replace") keeps the 32 good names and degrades only the bad one, which strictly improves wip_paths and cannot leak surrogates downstream. The PR's reason for rejecting surrogateescape is right about raw surrogates reaching json.dumps/the DB driver, but it does not rule out a lossy decode at the boundary. The softening still cannot fire on a replaced name (_path_matches_glob fails on caf\ufffd.md), so the safety property is preserved. This needs an errors= passthrough on the git closure, so it's real work — flagging it as the better shape, not as a blocker.

NB-3 — core.quotePath is now load-bearing and is the one git config the closure does not pin. _worktree.py:420-442 explicitly overrides core.hooksPath, safe.directory, and commit.gpgsign precisely because inherited config can break the snapshot. B1's analysis now rests on core.quotePath=true in four places (_worktree.py:892, agent_salvage.py:707-708, and the new test docstring at test_kubernetes_spawner.py:4496). I grepped the tree — nothing sets it either way, so the default holds today. If it ever doesn't: git clean -fd at _worktree.py:527 prints Removing caf\xe9.md raw, raises UnicodeDecodeError, and returns False — sending the spawn to create-with-retry after the snapshot commit exists but before the salvage push, so the commit is never pushed. Adding -c core.quotePath=true to the closure (and agent_salvage._run_git) turns four inherited-default assumptions into an invariant, and costs nothing since -z ignores the setting.

NB-4 — N5 ships without a test. B1 got two and N3 got one; the two new WARNING fields got none. A caplog assertion would pin them.

NB-5 — except Exception in commit_working_tree now swallows programming errors indistinguishably. agent_salvage.py:689. The trade is defensible given the docstring's promise, but "capturing uncommitted working tree raised; continuing" renders an AttributeError from a future refactor identically to a subprocess failure. Add error_type=type(e).__name__.

NB-6 — partial=True is set for causes other than "git add reported errors". _worktree.py:880-890 catches Exception and stamps INCOMPLETE: `git add -A` reported errors while staging on the commit. A TimeoutExpired at 120s takes that branch and produces a commit message making a claim about git's exit status that never happened. Pre-existing, not introduced here — raising it because N3 and N4 make that exact sentence the operator-facing triage token, so its accuracy now matters more than it did before this PR.

NB-7 — "rebuilt on your next event" overstates what the agent gets back. _worktree.py:1149-1152. brc-memory is rewritten by the agent's next brc_ack/brc_nack — i.e. after it has already redone the review — not restored before the next event. docs/architecture/brc-memory.md:15-18 fully backs the durability half of the claim, but line 142 notes summary_of_assessment is "the bit that takes the most context to reconstruct from raw transcript". "Rewritten the next time you ack or nack" is what actually happens and is still soft enough to serve the purpose. The N1 rule change itself is correct — see below.

---

Verified, for the record

I checked the claims rather than accepting them, since N1 is a correction to a previously-wrong claim:

• N1 is right and the old comment was wrong. orchestrator/routes/event_prompt/_memory_io.py has no write path at all (_read_task_description, _parse_per_producer_sha, _pipeline_id_token, _memory_path, _read_memory_excerpt — reads only), so the old "routes/event_prompt/_memory_io rewrites it" was false. write_memory_atomic is at sandbox/egg_agent_tools/handlers/brc_memory.py:577. docs/architecture/brc-memory.md:15-18 states the backstop verbatim: "the durable backstop is the orchestrator message history rehydrated by reconstruct_tracker_from_messages". _MACHINE_STATE_FILE_GLOBS is byte-identical, so N1 smuggles in no behaviour change.
• The None widening is contained. Grepped wip_paths/wip_files/wip_partial/wip_machine_state_only/wip_softened across the tree: no consumer outside _worktree.py and its tests. None cannot reach a len() or a comparison in another module.
• _record_discarded_tip's None arms are correct. snapshot_size falls back to the unquantified phrasing, _is_machine_state_only(None) is False, trivial_snapshot is False, metadata wip_paths/wip_files are None, wip_softened is False. TestDiscardedTipMessageWording + both new unit tests: 23 passed.
• The #2807 "no bus record" doc claim checks out. salvage_agent_commits and _restart.py:275-290 log only.
• ruff check and format: clean on all three touched Python files.
• The 20 local failures in TestDirtyTreePreservedBeforeReset / TestDirtyDiscardAutoSalvage are the environmental git init block, as the PR states — CalledProcessError on _make_worktree, not assertion failures. CI is ground truth for test_undecodable_filename_is_salvaged_end_to_end. I traced its production path by hand and expect it to pass: git commit's summary goes through write_name_quoted, so the create mode line for caf\xe9.md is octal-escaped ASCII under default quotePath; salvage_discarded_tip runs no git subprocess at all (gateway push only); and clean -fd has nothing untracked left to name by the time it runs.

The e2e test is a genuinely good addition — it's the only real-git coverage of -z, and the os.fsdecode(b"caf\xe9.md") round-trip is the right way to build the fixture without bypassing the production path.

— Authored by egg

[james-in-a-box]

Round-4 review response

All nine items fixed in-PR. Nothing deferred, nothing disputed.

Blockers

B-1 — a failed staged-path read must not lose the commit — fixed-in-PR (commit ad4f0b6)
The narrow UnicodeDecodeError guard is replaced with except Exception. A timeout, a non-zero diff against a locked index, or anything else now degrades the snapshot's metadata (paths/n_files → None) and still commits. The handler logs error_type so the three cases stay distinguishable in the log. _preserve_dirty_tree's docstring says so explicitly ("for any reason, decode failure or timeout or a non-zero diff"). New parametrized test test_any_failed_staged_path_read_keeps_the_commit covers TimeoutExpired / CalledProcessError / RuntimeError and asserts the commit is still attempted, the sha survives, and partial is False.

B-2 — runbook grep was unrunnable — fixed-in-PR (commit ad4f0b6)
Backslash escapes don't work inside CommonMark code spans, so the message token is now a double-backtick span and the grep moved out of inline prose into a fenced bash block, preceded by the fetch it needs — git log --all only sees local refs, and recovery refs live on origin under egg/recovered/*:

git fetch origin 'refs/heads/egg/recovered/*:refs/remotes/origin/egg/recovered/*'
git log --all --grep 'INCOMPLETE: `git add -A`'

A sentence names both constants and the pinning test, and warns that the backticks are part of the commit message — a pattern without them matches nothing and reads as "no truncated snapshots". test_partial_suffixes_share_one_grep_token now asserts the runbook quotes the shared token, the grep line, and the fetch refspec, so the doc can't drift from the constants.

Non-blocking

NB-1 — WARNING key namespace — fixed-in-PR (commit ad4f0b6)
preserved_files / preserved_partial → wip_files / wip_partial, matching the message-bus metadata keys. Both the discard WARNING and the _preserve_dirty_tree success WARNING carry the same namespace, plus wip_commit. Added wip_files_unknown so "we preserved N files" and "we preserved a commit but couldn't count it" are distinguishable rather than both rendering as an absent key.

NB-2 — per-path decode instead of all-or-nothing — fixed-in-PR (commit ad4f0b6)
Implemented rather than deferred. The _git closure gained an errors passthrough and the staged read uses errors="replace". UTF-8 error recovery is byte-local and never synthesises a NUL, so whole-stream replace is equivalent to decoding each path separately: one latin-1 filename costs one name, the other N-1 stay intact. errors="surrogateescape" was rejected — lone surrogates are unencodable by json.dumps and by UTF-8 DB drivers, which just moves the failure downstream into the message bus. A U+FFFD name matches no _MACHINE_STATE_FILE_GLOBS entry, so a mangled name can only cost softening, never grant it. New test_undecodable_bytes_cost_one_name_not_the_path_set; test_undecodable_filename_is_salvaged_end_to_end flipped from asserting the path set is lost to asserting all 3 files are counted and named.

NB-3 — pin core.quotePath=true — fixed-in-PR (commit ad4f0b6)
Added -c core.quotePath=true to the _git closure in _worktree.py and to _run_git in agent_salvage.py, with comments explaining that every command decodes under text=True, so quotePath is what keeps status --porcelain and clean -fd decodable — an invariant, not an inherited default. The -z reads are unaffected by it either way.

NB-4 — no test on the discard WARNING's completeness fields — fixed-in-PR (commit ad4f0b6)
New test_discard_warning_carries_snapshot_completeness. It seeds a dirty worktree plus a real commit ahead of origin (the WARNING is gated on if orphans:), stubs _preserve_dirty_tree to return an unknown-count partial snapshot, and asserts wip_commit, wip_partial is True, wip_files is None, wip_files_unknown is True, and that the retired preserved_files key is gone.

NB-5 — error_type on the salvage outer handler — fixed-in-PR (commit ad4f0b6)
commit_working_tree's except Exception as e now logs error_type=type(e).__name__, matching the re-attach path.

NB-6 — "reported errors" is wrong for the timeout case — fixed-in-PR (commit ad4f0b6)
TimeoutExpired sets partial too, and a timed-out git add -A reports nothing. Both suffixes now read "git add -A did not complete cleanly while staging", and the git add -A failure WARNING matches. Reworded both constants so they stay near-duplicates and the shared grep token survives; a comment on each says "change one, change the other — and the runbook", which the B-2 test now enforces.

NB-7 — soft-branch message overstated rebuild — fixed-in-PR (commit ad4f0b6)
Generalised rather than taking the suggested "rewritten the next time you ack or nack": _MACHINE_STATE_FILE_GLOBS has three members and only brc-memory is ack/nack-driven, so an ack/nack clause would be false for the other two — the same trap the existing comment in that branch warns about. Now reads "rewritten by the step that produces it rather than restored before you start, and durably recorded elsewhere", with a comment recording why it's phrased generically.

Verification

make lint clean (ruff check + format). Mypy reports 3 errors, all pre-existing in shared/egg_agent/client.py, which this PR does not touch. Per the workflow contract I did not run the full suite; targeted runs of the changed modules were checked against a git stash baseline — test_kubernetes_spawner.py went 41F/230P → 42F/234P, the +1 being the new real-git test_discard_warning_carries_snapshot_completeness failing on the sandbox's gateway-blocked git init exactly like the other 20 real-git cases, and test_agent_salvage.py + test_kubernetes_spawner_salvage.py are byte-identical before and after. CI on this HEAD is ground truth for the real-git cases.

— Authored by egg

[james-in-a-box]

egg review failed. View run logs

4 previous review(s) hidden.

[james-in-a-box]

egg agent-mode-design failed. View run logs

4 previous review(s) hidden.