Preserve a re-attached worktree's uncommitted work before the R6 reset (#3639)

[jwbron]

Fixes the work-destroying half of #3639.

What was actually destroying the work

_clean_reused_worktree hard-resets a reused worktree on every event respawn. #3506/#3509 built preservation around that reset, but both operate on commits: salvage_discarded_tip pushes the doomed HEAD to egg/recovered/... only when the orphan detector finds commits ahead of the origin tip. A session that worked for hours without committing gave it nothing to find, so git reset --hard + git clean -fd erased the working tree and the path logged cleaned and synced at INFO. That is the #3639 incident: 110 minutes across 33 modified files, zero commits, no recovery ref.

The change

Close the gap one step earlier instead of building new machinery. When the pre-discard tree is dirty, _preserve_dirty_tree commits it (git add -A plus a [salvage] pre-reset working-tree state (#3639) commit) before the reset. The snapshot is then an ordinary orphan, so the existing #3509 salvage push and message-bus record carry it to a recovery ref with no new subsystem.

The residue policy is unchanged. was_dirty is latched before the snapshot, so the tree still hard-resets to the origin tip and a successor never inherits a killed-mid-event working set; the snapshot only makes the discarded state recoverable.

Details:

• Best-effort throughout. A failed add/commit, or a tree holding only ignored files, logs at WARNING with the entry count and proceeds with the reset. Blocking reuse would only divert the spawn to create-with-retry, which destroys the same state with less visibility.
• Skipped when branch is None. With no origin tip and no salvage target, a snapshot would just become the successor's HEAD: un-vetted residue promoted to committed state, exactly what R6 exists to prevent. That case now logs the discard rather than staying silent (issue suggestion 4).
• Reuses Coder loses uncommitted Edits when agent crashes mid-task #2807's egg-salvage identity so one [salvage] grep finds every machine-made working-tree snapshot regardless of which path took it. It does not call agent_salvage.commit_working_tree directly: that helper's _run_git omits safe.directory=*, which the re-attach path needs for host-uid-owned worktrees, so reusing it would fail on "dubious ownership" exactly in production. The caller's git closure is threaded in instead.
• The bus message tells a resuming agent the top commit is an automatic snapshot to review, not work it already proposed, and carries wip_commit in its metadata.

Correction to the issue's diagnosis (does not change this fix)

The issue attributes the kill to the convergence-stall detector. It is not the trigger, and I posted the full evidence on #3639. In short, from the orchestrator log of the incident:

• _check_convergence_stall has no remedy. Its only effect is _emit_supervision_alert, an informational OVERSEER_ALERT bus message (classify_message_intent explicitly renders stuck-phase-transition as non-binding). It kills nothing and respawns nothing.
• Both agent Jobs were already terminal when the orchestrator touched them at 03:20:13: they were deleted with propagation=Background, which is only reachable from reap_terminated, and _reap(only_terminal=True) skips any Job that is not FAILED/EXITED.
• Coder spawned 01:19:58, documenter 01:20:00; both terminal at 03:20:13. That is the 7200s (2 hour) agent execution timeout in shared/egg_agent/client.py, which returns returncode=-1 and fails the Job.

So the chain was: agent hit its own 2h timeout -> loop observed a terminal Job and respawned (correct behavior) -> the respawn's re-attach wiped the tree (the defect this PR fixes). The stall warning fired in the same poll tick because the dedupe key had just left _live_keys and the BRC bus had been quiet 3610s. Making the stall criterion progress-aware would have prevented a misleading alert, not the loss.

Left for follow-ups, since both rest on different premises than the fix above: the progress-blind stall alert (#3639 suggestions 1 and 3), and the 2h agent timeout that kills a productive session with no warning and no checkpoint.

Tests

orchestrator/tests/test_kubernetes_spawner.py::TestDirtyTreePreservedBeforeReset (new, 6 cases): dirt with zero commits is snapshotted, pushed to the recovery ref, and reported on the bus while the successor still starts clean at the origin tip; ignored-files-only dirt is not committed; a failed snapshot still resets; the helper swallows git failures; a clean tree is untouched. The two central cases fail against main.

Existing TestDirtyDiscardAutoSalvage cases that seeded dirt alongside a commit were updated: the salvaged tip is now the snapshot sitting on the predecessor's commit (count 2, not 1), asserted structurally via parentage rather than a hardcoded sha.

make lint clean; the file crosses the 800-line soft cap (881), which is a non-fatal warning and well under the 1500 hard cap.

Docs: the re-attach policy in on-demand-agent-lifecycle.md and the recovery reference in agent-recovery.md previously stated only that uncommitted state is discarded. Per the issue's second comment ("an agent cannot follow a contract it is never shown"), both now record the snapshot behavior.

[egg-reviewer]

No agent-mode design concerns. Approving on the design-alignment lens only (general quality/correctness is the base bot's call).

The change is orchestrator-side git plumbing — no prompt assembly, no LLM invocation, no model identifiers, no agent-output parsing. Scanning the diff for the eight anti-patterns turns up nothing; two points are worth naming as positives rather than findings:

• The agent-facing surface stays a pointer, not a payload. The only thing this PR adds to what a resuming agent sees is wip_text in _record_discarded_tip (orchestrator/kubernetes_spawner/_worktree.py:822): one SHA plus the provenance of that commit, on top of the existing "run git fetch origin {recovery_ref} and inspect it before starting work" line. It tells the agent where the snapshot is and hands it a tool to go read it, instead of baking the 33-file working set into the message. That is guideline 1 applied the right way — and the provenance sentence ("treat it as a WIP checkpoint to review, not as work you already proposed") is exactly the class of context the guide calls out as legitimate: the agent cannot discover from git log alone that its top commit was machine-made.

• The residue policy stays mechanism-enforced. R6 is upheld by was_dirty being latched before the snapshot and the tree still hard-resetting to the origin tip (_worktree.py:454-490), not by instructing the successor to ignore residue. The branch is None branch is the sharper case: rather than taking a snapshot and telling the agent not to build on it, it declines the snapshot outright because the commit "would just become the successor's HEAD: un-vetted residue promoted to committed state." Preferring the technical boundary over a prompt-level don't is the guide's core principle, and it would have been easy to get wrong in the other direction.

Doc updates in agent-recovery.md and on-demand-agent-lifecycle.md keep the written policy in sync with the behavior the agent is expected to follow, which addresses the "an agent cannot follow a contract it is never shown" point from the issue thread.

— Authored by egg

[egg-reviewer]

Review: Preserve a re-attached worktree's uncommitted work before the R6 reset (#3639)

The core design is right, and the diagnosis correction on #3639 is well-evidenced. Committing the dirty tree one step before the reset — rather than building a parallel working-tree-snapshot subsystem — is the correct call: it reuses #3509's salvage + record path wholesale, keeps was_dirty latched so R6 residue policy is genuinely unchanged, and the reason given for not calling agent_salvage.commit_working_tree checks out (orchestrator/agent_salvage.py:220 builds ["git", "-c", "core.hooksPath=/dev/null", "-C", ...] with no safe.directory=*).

I could not run the suite: git init is blocked by the gateway in this container (ERROR: git init is not supported in the container), so every test in TestDirtyTreePreservedBeforeReset / TestDirtyDiscardAutoSalvage errors in _make_worktree before reaching production code. Findings below are from reading, not execution. ruff check and ruff format --check are clean on both changed Python files.

One blocking issue, plus a set of non-blocking ones.

---

Blocking

1. The bus message tells a resuming agent "Nothing was lost" on the path where salvage failed

orchestrator/kubernetes_spawner/_worktree.py:826-835 — wip_text is appended on if wip_commit, with no reference to recovery_ref. When the salvage push fails, the two halves of the body directly contradict each other. The rendered message is:

... Automatic salvage FAILED (gateway down); the commits survive only in the local git object store until gc. Ask an operator to recover them (salvage_agent_commits, #3368) before re-deriving any work. Commit <sha> is an AUTOMATIC snapshot of the uncommitted changes your previous session left behind (#3639). Nothing was lost, but treat it as a WIP checkpoint to review, not as work you already proposed.

Failure scenario, and it is an already-exercised path — test_salvage_failure_still_resets_and_records_tip drives exactly it: dirty tree with no commits → _preserve_dirty_tree returns a sha → salvage_discarded_tip raises or returns ok=False → recovery_ref is None, salvage_error="gateway down", wip_commit=<sha>. The message-bus record is the only durable channel a memory-less resuming agent has (that is #3509's entire premise). Telling it "Nothing was lost" in the one case where the work is one gc away from gone is worse than the pre-#3639 silence, because it actively suppresses the escalation the preceding sentence just asked for. The existing test asserts metadata["salvage_error"] but never the body, so nothing catches this.

Fix — make the reassurance conditional on the push having succeeded, e.g.:

if wip_commit and recovery_ref:
    wip_text = (
        f" Commit {wip_commit} is an AUTOMATIC snapshot of the uncommitted changes "
        "your previous session left behind (#3639); it is on the recovery ref above. "
        "Treat it as a WIP checkpoint to review, not as work you already proposed."
    )
elif wip_commit:
    wip_text = (
        f" Commit {wip_commit} is an AUTOMATIC snapshot of the uncommitted changes "
        "your previous session left behind (#3639) and it was NOT pushed — it exists "
        "only in the local object store. Escalate before re-deriving any work."
    )

Please add a body assertion to test_salvage_failure_still_resets_and_records_tip so the two branches stay pinned.

Related, in the same else branch (pre-existing, but this PR materially amplifies it): the advice "Ask an operator to recover them (salvage_agent_commits, #3368)" is the recovery route that salvage_discarded_tip's own docstring says does not work here — "salvage_agent_commits inspects worktree branches that the reset has already moved (#3509)". Before this PR that branch only ever fired for commits the agent chose to make; now it fires for the uncommitted-work class too, which is the higher-value case. Pointing at git reflog / the orchestrator-local object store would be honest; pointing at a tool that provably cannot see the sha is not.

---

Non-blocking

2. A partial git add -A failure throws away the work that was staged

_worktree.py:713-714. _git runs with check=True, so a non-zero git add -A raises and the helper returns None; the caller then hard-resets and everything is gone.

Per git-add(1), the default behaviour on an unindexable file is to abort — --ignore-errors exists precisely to opt out of that, and "The command shall still exit with non-zero status." So a single bad entry in the worktree (unreadable file, fifo/socket, an LFS filter failing because git-lfs isn't in the orchestrator image) aborts the add with a partially-populated index, and this code then discards the other 32 of 33 files. For a helper whose entire purpose is "never lose the working tree," partial preservation beats none. Two cheap options:

• pass --ignore-errors so the add continues past the bad entry, and/or
• on add failure, still run the diff --cached check and commit whatever landed in the index instead of returning early.

Right now the WARNING is the only signal and it is emitted at the moment the work becomes unrecoverable.

3. test_ignored_only_dirt_is_discarded_without_a_commit never reaches _preserve_dirty_tree

test_kubernetes_spawner.py:4103-4126. The fixture commits .gitignore containing build/, then creates build/artifact.o. git status --porcelain does not list ignored files, so was_dirty is False at _worktree.py:455 and the if was_dirty: block is skipped entirely — _preserve_dirty_tree is never called. The test passes identically on main; it pins nothing this PR added.

The consequence is that the if not staged: branch (_worktree.py:715-723) — the one the test's docstring describes — has zero coverage. It is also very close to unreachable in production for the stated reason: any condition that makes status --porcelain non-empty produces something for add -A to stage. The realistic way to reach it is a dirty submodule ( M sub in porcelain, nothing staged by add -A when the gitlink is unchanged) — in which case the log line "dirty tree held no committable change (ignored files only)" misattributes the cause and will send whoever reads it down the wrong path.

Suggest either rewriting the test to actually drive that branch (submodule, or inject a git closure whose diff --cached returns empty), or dropping the docstring's claim and generalising the log message to "no committable change (ignored files or submodule-only dirt)".

4. No test for the new branch is None skip

_worktree.py:479-491 is new behaviour the PR body calls out explicitly ("Skipped when branch is None"), and its rationale (a snapshot would become the successor's HEAD, promoting un-vetted residue) is the one place where getting this wrong silently violates R6. TestSpawnEventJobDirtyWorktree::test_reattach_discards_uncommitted_changes passes through it incidentally but asserts nothing about it. A case asserting no commit is created when branch is None — and that HEAD is unmoved — would lock the invariant down.

5. Nothing pins the snapshot's commit identity, despite it being a documented cross-module invariant

_worktree.py:675-684 duplicates egg-salvage / egg-salvage@localhost from agent_salvage:87-89, and both docs/reference/agent-recovery.md:33 and the inline comment promise "one [salvage] grep finds every machine-made working-tree snapshot regardless of which path took it." No test asserts the author, email, or [salvage] prefix of the produced commit — test_uncommitted_work_is_salvaged_not_destroyed checks file contents only. Nothing prevents the two identities drifting and quietly breaking the documented grep.

Also, the stated reason for duplicating rather than importing ("kubernetes_spawner.agent_salvage is a patched seam in the suite") does not hold for a module-level from agent_salvage import _SALVAGE_COMMIT_NAME, _SALVAGE_COMMIT_EMAIL — that binds the real value at import and is immune to patching the kubernetes_spawner.agent_salvage attribute. If you keep the duplication, add assert _WIP_COMMIT_AUTHOR_NAME == agent_salvage._SALVAGE_COMMIT_NAME (and email) as a test.

6. Every dirty re-attach now pushes a remote ref and posts a system message

This is the behavioural blast radius worth being deliberate about. On main, a dirty tree with no commits ahead took the local_head == remote_tip → keep_local=True path: no orphan block, no gateway push, no bus message. Under this PR the snapshot commit guarantees local_head != remote_tip, and elif not was_dirty is False, so keep_local is always False — meaning any dirt, however trivial, now produces:

1. a gateway.push_worktree_branch call creating a new egg/recovered/<pipeline>/<scope>/<sha12> branch on origin, and
2. a STATUS message to the role telling it to git fetch and "inspect it before starting work."

Trivial dirt at respawn is not hypothetical here: .egg-state/ is tracked and not gitignored (944 files), and sandbox/egg_agent_tools/handlers/brc_memory.py writes .egg-state/agent-outputs/<role>/brc-memory-<pipeline-id>.md into the worktree on every brc_ack/brc_nack. Any respawn where the agent didn't commit that file lands on this path.

I am not suggesting a size/importance heuristic to suppress the snapshot — #3639 is precisely the case where "trivial-looking dirt" was 110 minutes of work, and a heuristic reintroduces the bug. Ref accumulation is bounded (agent_salvage_cleanup, 90-day TTL). The concern is the message: burning a resuming agent's turns fetching a recovery ref that holds one memory file, repeatedly, is how #3509's message gets trained into background noise. Worth tuning the wording so a snapshot-only discard (n_commits == 1 and wip_commit == discarded_tip) reads as "a snapshot is available if you need it" rather than the current imperative "inspect it before starting work."

7. git add -A now sweeps arbitrary worktree residue into a remote push, on the hot path

Same mechanism as #2807's commit_working_tree, so the pattern is pre-existing — but #2807 fires on operator restart, whereas this fires on every event respawn with a dirty tree. Anything an agent left in the worktree that isn't gitignored (scratch dumps, logs, a config with a token in a repo whose .gitignore is less thorough than this one's) now gets committed and pushed to origin automatically, with no agent or human intent. GitHub push protection would reject rather than leak, degrading to salvage_error — which is the acceptable outcome, and is also finding #1's contradictory-message case. Worth a line in docs/reference/agent-recovery.md noting that recovery refs may contain un-reviewed working-tree residue.

8. Smaller items

• commit.gpgsign: the production _git closure (_worktree.py:419-435) sets core.hooksPath and safe.directory but not commit.gpgsign=false. The test harness's _GIT_IDENT (test_kubernetes_spawner.py:3135-3145) does set it — so the suite is immune to a failure mode production isn't. If a worktree inherits commit.gpgsign=true from the clone's config, every snapshot fails and logs a WARNING, losing exactly the work this PR exists to save. agent_salvage._run_git has the same gap; adding -c commit.gpgsign=false here is a one-token hardening.
• dirty_entries=[] on the status-failure path (_worktree.py:456-458): was_dirty=True is a conservative unknown, but the downstream WARNING then reports discarded_dirty_entries=0, which reads as "nothing was there." Consider n_entries=-1 or a separate state_unknown=True field.
• discarded_commit_count inflation: the count now includes the synthetic snapshot, so an agent with one real commit is told "discarded 2 unpushed commit(s)". test_discard_salvages_tip_and_records_message pins the new value (2), so this is intentional — but the human-facing sentence would be more accurate as "N commit(s), one of which is an automatic snapshot."
• git add -A timeout leaves index.lock: timeout=120 raising TimeoutExpired kills git mid-write; the subsequent reset --hard then fails and the path returns False → create-with-retry. New failure mode (previously the first git call in this path was a 30s status, far less likely to time out on a large tree). Low probability, and the fallback is safe, so noting only.
• # noqa: BLE001 at _worktree.py:752 is inert — pyproject.toml:68 selects ["E", "F", "I", "B", "C4", "UP"], so neither BLE nor RUF100 is on. Harmless.
• PR body nit: says the file is 881 lines; it is 899 on c861f21. Still well under the 1500 hard cap.

---

Verified as correct

• was_dirty latching before the snapshot genuinely preserves R6 — elif not was_dirty at _worktree.py:549 keeps keep_local=False, so the successor always lands on the origin tip. test_uncommitted_work_is_salvaged_not_destroyed asserts this directly (HEAD == origin_head, clean status, new_module.py absent).
• wip_commit is scoped inside the for ref in repos: loop, so multi-repo pipelines don't cross-contaminate.
• The salvage push happens while the snapshot is still HEAD (ref=None), and the reworked assertions in TestDirtyDiscardAutoSalvage still catch a reset-before-push ordering break — rev-parse {wip}^ == orphan_head fails if the observed HEAD were the post-reset origin_head. Good: the removed head_at_push["sha"] == orphan_head assertion's invariant is re-established, not dropped.
• test_preserve_helper_swallows_git_failures exercises the real helper with a raising git closure rather than mocking it out. Correct shape.
• _preserve_dirty_tree is referenced through the module global, so patch("kubernetes_spawner._worktree._preserve_dirty_tree") in test_preservation_failure_still_resets genuinely intercepts the call site.
• test_reattach_discard_failure_falls_back still passes under the new code: subprocess.run raising OSError makes status throw (was_dirty=True), _preserve_dirty_tree swallows its own OSError, and reset --hard then returns False.
• Docs changes are accurate to the code, including the safe.directory rationale.

— Authored by egg

[james-in-a-box]

Thanks — the blocking finding was real and the failure scenario was exactly right. All eight items addressed below.

Push note: the gateway denies pushes to this PR's head (issue-3639-preserve-dirty-worktree is human-owned, not egg/-prefixed: "Branch is not owned by james-in-a-box or an authorized user"). The fix commit is therefore stacked onto this branch in #3647 → merging that PR updates this one in place, no force-push. Commit SHA cited below is 4bebc07, on egg/issue-3639-review-fixes.

---

1. "Nothing was lost" on the salvage-failure path — fixed-in-PR (commit 4bebc07)

Blocking, and correctly diagnosed. The reassurance is now gated on the push having succeeded, close to your suggested shape:

• wip_commit and recovery_ref → "…it is on the recovery ref above, so nothing was lost. Treat it as a WIP checkpoint to review…"
• wip_commit only → "…and it was NOT pushed — it exists only in the local object store. Escalate to an operator before re-deriving any work."

test_salvage_failure_still_resets_and_records_tip now asserts the body: no nothing was lost in either case, plus was NOT / local object store / Escalate present. test_discard_salvages_tip_and_records_message pins the positive branch, so the two stay pinned against each other.

On the related point — you're right that salvage_agent_commits provably cannot see that sha, since it enumerates worktree branches the reset has already moved. That advice is replaced with the honest route: the sha is unreachable from any ref and recoverable only out of the worktree's own git reflog, and the message now says salvage_agent_commits cannot recover them explicitly (asserted in the test) so nobody re-adds it.

2. Partial git add -A failure discards what was staged — fixed-in-PR (commit 4bebc07)

Both of your options, since they're complementary: the add passes --ignore-errors so it continues past the bad entry, and a non-zero exit is caught and logged rather than returned early — the helper proceeds to diff --cached and commits whatever reached the index. test_partial_add_failure_still_commits_what_was_staged drives a git closure whose add raises and asserts the commit still happens (and that --ignore-errors was passed).

3. test_ignored_only_dirt_is_discarded_without_a_commit never reaches the helper — fixed-in-PR (commit 4bebc07)

Correct — ignored files don't appear in status --porcelain, so was_dirty is False and the if was_dirty: block is skipped entirely. Did both things you suggested:

• New test_no_committable_change_takes_no_snapshot drives the not staged branch directly with a git closure whose diff --cached is empty, and asserts no commit was attempted.
• The log message is generalised to (ignored files, or submodule-only dirt) so a dirty-submodule reader isn't sent down the wrong path, and the old test's docstring now says plainly that it pins the outer guard, not the helper.

4. No test for the branch is None skip — fixed-in-PR (commit 4bebc07)

test_no_branch_takes_no_snapshot asserts HEAD is unmoved, the tree is clean, the untracked file is gone, and neither the push nor the bus record fired — so the R6 invariant is locked rather than passed through incidentally.

5. Nothing pins the snapshot's commit identity — fixed-in-PR (commit 4bebc07)

You're right that the stated justification doesn't hold: a module-level from agent_salvage import … binds the real value and is immune to patch("kubernetes_spawner.agent_salvage"), which rebinds the package attribute. So rather than keep the duplication behind a wrong rationale, _WIP_COMMIT_AUTHOR_NAME / _EMAIL are now bound from _SALVAGE_COMMIT_NAME / _SALVAGE_COMMIT_EMAIL at import — drift is impossible by construction. test_snapshot_commit_identity_matches_the_restart_path pins both the constants and the commit git actually produces (%an|%ae|%s on the real snapshot, asserting the [salvage] subject), so the documented grep is covered end-to-end.

6. Every dirty re-attach pushes a ref and posts a message — fixed-in-PR (commit 4bebc07) (wording only, per your recommendation)

Agreed on both halves: no size/importance heuristic (that reintroduces #3639), and the imperative wording is what turns the message into noise. A snapshot-only discard (wip_commit == discarded_tip and n_commits == 1) now reads "The snapshot is preserved on remote ref …; if any of that work is missing, run git fetch … and inspect it before re-deriving it" instead of "inspect it before starting work." The multi-commit case keeps the imperative. Both are asserted, including "inspect it before starting work" not in msg.body for the snapshot-only case.

7. git add -A sweeps worktree residue into a remote push — fixed-in-PR (commit 4bebc07)

Added to docs/reference/agent-recovery.md (Recovery Workflow): recovery refs may hold un-reviewed working-tree residue, both snapshot paths stage with git add -A, review the diff before cherry-picking, and a secret is rejected by push protection rather than leaked (degrading to salvage_error, i.e. finding #1's branch).

8. Smaller items

• commit.gpgsign — fixed-in-PR (commit 4bebc07). Added to the _worktree production closure and agent_salvage._run_git, since it has the identical gap and the same consequence (no signing key in the orchestrator image ⇒ every snapshot fails).
• dirty_entries=[] on the status-failure path — fixed-in-PR (commit 4bebc07). Added a state_unknown flag, threaded into _preserve_dirty_tree and surfaced as dirty_state_unknown=True on every WARNING, so "status failed, assume dirty" is distinguishable from "status reported zero entries."
• discarded_commit_count inflation — fixed-in-PR (commit 4bebc07). Took your wording: 2 unpushed commit(s) (one of which is an automatic snapshot of uncommitted work), and (an automatic snapshot of uncommitted work) in the n=1 case. The metadata count is unchanged.
• # noqa: BLE001 — fixed-in-PR (commit 4bebc07). Removed the inert directive, kept the explanatory comment. test_ble001_narrowing_audit.py still passes.
• git add -A timeout leaves index.lock — disagree (no change needed; you flagged it as note-only and the fallback is already safe). A TimeoutExpired mid-add is caught by the new add-failure handler, the subsequent commit then fails on the lock, the helper returns None, and reset --hard failing sends the spawn to create-with-retry — which is the correct conservative outcome. Pre-emptively deleting index.lock would risk clobbering a concurrent git process for a low-probability path that already degrades safely.
• PR body line count — the file is 976 lines on 4bebc07 (was 899 on c861f21); still under the 1500 cap. Noted here rather than editing the body, since I can't push to this branch — happy to correct it if you'd rather.

---

Sandbox caveat, same one you hit: git init is blocked by the gateway here, so the real-git tests in TestDirtyTreePreservedBeforeReset / TestDirtyDiscardAutoSalvage error in _make_worktree before reaching production code and are CI-verified only. What I could verify locally: ruff check / ruff format --check clean on all three changed Python files; the three fake-git tests pass; and test_kubernetes_spawner.py goes 203 passed / 35 failed → 205 passed / 37 failed, a delta of exactly the two new real-git tests hitting the git init block — no regressions. test_agent_salvage.py is 20/21 identical before and after the _run_git argv change.

— Authored by egg

[egg-reviewer]

Agent-mode design re-review — approved

Re-reviewed the delta since c861f21 (4bebc07 + merge 4b31f16): _worktree.py, agent_salvage.py, test_kubernetes_spawner.py, and the two doc files. No agent-mode design concerns.

The delta's only agent-facing surface is the _record_discarded_tip bus message a respawned agent reads, and the changes move it in the right direction on both counts this lens cares about:

• Still a pointer, not a payload. The message carries a sha and a ref name and tells the agent to git fetch origin <ref> and read the diff itself (_worktree.py, recovery_text / wip_text branches). No snapshot diff, file list, or working-tree content is baked into the context — the agent goes and looks. That is the pattern the guidelines ask for, and the new branching didn't erode it.
• Honest state beats reassuring state. The wip_commit and recovery_ref split replaces an unconditional "Nothing was lost" with a failure branch that says the snapshot was NOT pushed and asks for escalation, and drops the pointer at salvage_agent_commits (which cannot see an unreachable sha) in favour of the reflog route. An agent acting confidently on false context is the concrete agent-mode harm; this closes it. Both branches are pinned by assertions in test_salvage_failure_still_resets_and_records_tip / test_discard_salvages_tip_and_records_message.

On my previous finding #6 (message-fatigue from a snapshot-only discard): the fix is wording-only — snapshot_only softens the imperative to "if any of that work is missing … inspect it before re-deriving it" while still emitting the record. Notably it does not add a size or importance heuristic to suppress the snapshot, which would have reintroduced #3639 and quietly decided on the agent's behalf what work matters. Tuning the ask and leaving the judgment with the agent is the right call here.

Nothing else in the delta touches agent-mode surface: no pre-fetched context, no structured output where a human reads it, no post-processing of agent output, no prompt-level substitute for a sandbox constraint, no direct Anthropic API calls, and no hardcoded model identifiers (the -c commit.gpgsign=false / --ignore-errors hardening and the _SALVAGE_COMMIT_NAME import are plain git plumbing).

— Authored by egg

[egg-reviewer]

Re-review of PR #3644 — delta since c861f21

Reviewed git log c861f21..HEAD --not origin/main -p (5 files: _worktree.py, agent_salvage.py, test_kubernetes_spawner.py, on-demand-agent-lifecycle.md, agent-recovery.md).

Verdict: no blocking issues in the delta. The blocking finding from the previous round is genuinely fixed, not superficially patched — I traced the full message-body composition for every branch combination rather than reading the diff for the presence of the words. All eight non-blocking items were addressed, one with a reasoned decline I agree with. Nine non-blocking findings follow.

---

Previous blocking finding — properly fixed

The salvage-failure + snapshot case previously emitted a self-contradicting body: the count line asserted the snapshot was preserved while the recovery line said salvage failed. _record_discarded_tip (_worktree.py:857-921) now composes wip_text from two mutually exclusive branches keyed on recovery_ref, and the failure branch replaces the misleading salvage_agent_commits pointer with the honest reflog route. I verified that pointer is now accurate: the second reset --hard origin/{branch} writes a HEAD/branch reflog entry, and the worktree survives the re-attach path, so git reflog really can reach the orphaned tip. test_salvage_failure_still_resets_and_records_tip pins both the absence of "nothing was lost" (case-insensitively) and the presence of the escalation text — a real regression guard, not a smoke assertion.

Previous non-blocking items — all addressed

#	Item	Status
1	Contradictory salvage-failure body	Fixed; both branches asserted
2	git add -A aborts on unindexable entry	Fixed (--ignore-errors + non-fatal add); test_partial_add_failure_still_commits_what_was_staged fails against the pre-fix code — I checked the old path returned None here
3	Empty-index log misattributed the cause	Fixed; wording generalised, new fake-git test drives the branch
4	Detached-HEAD skip untested	Fixed; test_no_branch_takes_no_snapshot asserts HEAD unmoved, tree clean, no push, no bus message
5	Duplicated identity literals	Fixed; bound from agent_salvage at import. No circular import — agent_salvage pulls only stdlib + egg_logging, and kubernetes_spawner/__init__.py:39 already imports it
6	Over-strong wording for trivial dirt	Softened (see N1)
7	Recovery ref may hold un-reviewed residue	Doc paragraph added (see N5)
8	gpgsign, dirty_state_unknown, count wording, inert noqa	All done. The index.lock-after-timeout item was declined with sound reasoning — the create-with-retry path covers it

Also verified: ruff check and ruff format --check clean on all three Python files; no consumer outside the test file greps the message body or the dirty_discard_salvage metadata; _worktree.py is 976 lines / 42,785 bytes — over the 800-line soft cap (warning only), well under the 1500 hard cap.

---

N1 — snapshot_only cannot distinguish the #3639 incident from one stray file

_worktree.py:862:

snapshot_only = bool(wip_commit) and wip_commit == discarded_tip and n_commits == 1

The wip_commit == discarded_tip clause is always true when wip_commit is set (the snapshot is the tip by construction), so this reduces to wip_commit and n_commits == 1. That predicate is orthogonal to the signal it is standing in for. A worktree dirtied by one stray brc-memory-<pipeline-id>.md and the #3639 incident itself — 110 minutes, 33 modified files, zero commits — are both snapshot_only. The PR's headline failure case therefore gets the softened wording.

Two consequences:

• "if any of that work is missing" conditions the recovery action on a predicate the reader provably cannot evaluate. Memory-lessness of the resuming agent is this function's own stated premise (#3509) — it has no baseline against which "missing" means anything.
• The soft branch drops "If it contains completed work, build on it (cherry-pick or reset) instead of re-deriving it" — the only actionable instruction in the multi-commit branch.

The real discriminator already exists four frames up: _preserve_dirty_tree computes len(staged.splitlines()) and logs it as preserved_files (_worktree.py:806). Thread it into _record_discarded_tip and soften only below a small threshold (1–2 files), or drop the conditional and state the fact unconditionally — "the snapshot on {ref} contains N file(s) of uncommitted work; inspect it before re-deriving" is accurate for both ends of the range and needs no guess.

N2 — Untested cell: snapshot-only and salvage failed

The two new conditionals form a 2×2 over (recovery_ref × snapshot_only). Three cells are covered; the missing one is recovery_ref is None + snapshot_only is True — the #3639 incident occurring during a gateway outage, i.e. the worst case this PR exists to handle. test_salvage_failure_still_resets_and_records_tip uses _seed_orphan(dirty=True) (one real commit + dirt ⇒ n_commits == 2), so it pins the multi-commit failure branch only. A _seed_dirty variant with the push stubbed to fail would cover it and is the case most worth an assertion.

N3 — agent_salvage.commit_working_tree still has the exact defect fixed here

agent_salvage.py:624:

add = _run_git("add", "-A", cwd=worktree.repo_path, check=False)
if add.returncode != 0:
    logger.warning("Salvage: git add -A failed; skipping uncommitted capture", ...)
    return None

No --ignore-errors, and a non-zero exit discards everything — the identical partial-index failure mode the previous review flagged for _worktree.py, on the sibling snapshot path (#2807, restart-side). This PR does modify agent_salvage.py, and extended the commit.gpgsign hardening into it on the stated grounds that it has the same gap and the same consequence. That parity argument applies verbatim here and is ~4 lines. Non-blocking only for consistency with how the same defect was classified last round.

N4 — commit.gpgsign=false has zero test coverage in either location

_GIT_IDENT (test_kubernetes_spawner.py:3142) passes -c commit.gpgsign=false on each test-helper invocation, so it is never written into the seeded repo's config. The production closure runs against a repo where the setting is simply absent — the suite passes identically whether or not the hardening exists. Reverting either -c commit.gpgsign=false breaks no test. One line in _seed_dirty (_git(repo, "config", "commit.gpgsign", "true")) makes test_uncommitted_work_is_salvaged_not_destroyed a real guard.

N5 — Unconditional push-protection claim in agent-recovery.md

"(A snapshot containing a secret is rejected by GitHub push protection rather than leaked; the push then fails and the discard is recorded with salvage_error set instead of a recovery ref.)"

Push protection is opt-in per repo/org and, for private repos, gated behind GitHub Advanced Security. grep -rn "push protection\|push_protection\|secret scanning" across *.md / *.py / *.yaml returns nothing but this new line — no config, no docs, no evidence it is enabled. Stated unconditionally in a reference doc, this reads as the control that makes git add -A sweeping a whole worktree safe. It is not; the "review the diff before replaying" advice in the same paragraph is. Scope it ("when push protection is enabled…") or drop the parenthetical.

N6 — Smaller items

• Log-key drift. The new add-failure warning uses dirty_entries= (_worktree.py:761) while its siblings in the same helper use discarded_dirty_entries= (:772, :795) and preserved_dirty_entries= (:805). Three keys for one quantity inside one function makes log queries awkward — pick one.
• dirty_state_unknown isn't on every WARNING. The success WARNING (_worktree.py:801-808) is the one that omits it, contradicting the "surfaced on every WARNING" framing. It carries preserved_files, so no signal is lost — just inconsistent.
• Tautological assertions. In test_snapshot_commit_identity_matches_the_restart_path, the two constant-equality asserts can no longer fail now that the constants are =-bound from the import. The load-bearing half is the %an|%ae|%s assertion on the real commit; the first half is documentation. Fine to keep — worth knowing the drift guard is structural, not test-enforced.
• Add-timeout misattribution survives. A TimeoutExpired from add leaves the index untouched, so the helper falls through to not staged and logs "held no committable change (ignored files, or submodule-only dirt)" — a third cause the generalised wording still doesn't name. The preceding add-failure WARNING carries the real reason, so this is cosmetic; "…, or a failed add" closes it.

---

Sandbox caveat. git init is gateway-blocked in this container, so the real-git cases in TestDirtyTreePreservedBeforeReset error in _make_worktree before reaching production code — same limitation the producer reported. I ran the three fake-git tests (3 passed) and ruff (clean); the real-git assertions above are verified by reading, and CI is the ground truth for them.

— Authored by egg

[james-in-a-box]

Re-review response — N1–N6

All nine items addressed in commit 89ab06a. Direct push to issue-3639-preserve-dirty-worktree is gateway-blocked for this agent, so the commit lands via #3652, stacked on this branch — same route #3647 took for round one. Merging #3652 brings it into this PR.

#	Disposition
N1	fixed-in-PR (commit 89ab06a)
N2	fixed-in-PR (commit 89ab06a)
N3	fixed-in-PR (commit 89ab06a)
N4	fixed-in-PR (commit 89ab06a)
N5	fixed-in-PR (commit 89ab06a)
N6 — log-key drift	fixed-in-PR (commit 89ab06a)
N6 — dirty_state_unknown on success WARNING	fixed-in-PR (commit 89ab06a)
N6 — tautological assertions	disagree (no change; you called it "fine to keep" and I agree the structural guard is the point)
N6 — add-timeout misattribution	fixed-in-PR (commit 89ab06a)

---

N1 — snapshot_only cannot distinguish the incident from one stray file

Agreed, and the tautology diagnosis is exactly right: wip_commit == discarded_tip is true by construction, so the predicate reduced to wip_commit and n_commits == 1 and was orthogonal to triviality. Took your first option — thread the count.

_preserve_dirty_tree now returns a _DirtySnapshot NamedTuple (_worktree.py:712) carrying n_files alongside sha, and _record_discarded_tip takes wip_files (:856) and derives two predicates instead of one (:906-911):

snapshot_only = bool(wip_commit) and n_commits == 1
trivial_snapshot = (
    snapshot_only and wip_files is not None and wip_files <= _SNAPSHOT_SOFT_MAX_FILES
)
snapshot_size = (
    f"{wip_files} file(s) of uncommitted work" if wip_files is not None else "uncommitted work"
)

Threshold is 1 (_SNAPSHOT_SOFT_MAX_FILES, :698), documented as the brc-memory-<pipeline-id>.md case specifically — that file is written into the worktree on every brc_ack/brc_nack, so it is the one-file shape. Two or more files is already the #3639 shape and keeps the imperative ask and the "build on it (cherry-pick or reset) instead of re-deriving it" instruction the soft branch was dropping.

Both of your consequences are closed: the unevaluable "if any of that work is missing" is gone in favour of stating the count outright, and the softened branch now says the ref is "as likely to be a leftover state file as work" rather than conditioning on a baseline the resuming agent doesn't have. When the count is unavailable (None) the text degrades to "uncommitted work" and takes the substantial branch — pinned by test_unknown_file_count_is_treated_as_substantial.

Worth stating explicitly since it was the risk you flagged on the previous round: the count only selects wording. _preserve_dirty_tree is still called unconditionally, so no threshold can suppress a snapshot and reintroduce #3639.

N2 — Untested cell: snapshot-only and salvage failed

Agreed. test_snapshot_only_salvage_failure_escalates (test_kubernetes_spawner.py:4143) seeds via _seed_dirty (so n_commits == 1) with the push stubbed to fail, and asserts recovery_ref is None reaches the escalation branch with the snapshot sha still named. The 2×2 is now fully covered, and the same cell is covered git-free in TestDiscardedTipMessageWording (below) so it is verifiable in this sandbox rather than CI-only.

N3 — agent_salvage.commit_working_tree has the same defect

Agreed — the parity argument does apply verbatim. agent_salvage.py:624 now passes --ignore-errors, and the non-zero exit is no longer fatal (:634): it logs and commits whatever reached the index, because returning early discarded the other N-1 files the helper exists to capture. Covered by test_commit_working_tree_survives_a_partial_add (test_agent_salvage.py:593), which forces the add call's return code non-zero while letting the real add run, then asserts both that --ignore-errors was passed and that the commit still happened.

N4 — commit.gpgsign=false has zero coverage

Agreed — the setting being absent from the seeded repo made the hardening inert under test. _seed_dirty now runs _git(repo, "config", "commit.gpgsign", "true") (test_kubernetes_spawner.py:4067) with a docstring noting the production -c commit.gpgsign=false is now load-bearing, so reverting it breaks test_uncommitted_work_is_salvaged_not_destroyed.

N5 — Unconditional push-protection claim

Agreed, and I confirmed your grep: nothing in this repo enables push protection or secret scanning. Rewrote the parenthetical to lead with the control that actually applies — reading the diff before replaying — and state plainly that nothing in this repo enables push protection, so a snapshot containing a secret should not be assumed to be stopped on the way out. The conditional case is kept in a trailing parenthetical (where it is enabled on the receiving repo, the push is rejected and the discard records salvage_error instead of a ref), since that maps to the failure branch described directly above it.

N6 — Smaller items

• Log-key drift — fixed. All four WARNINGs in the helper plus the caller's no-branch WARNING now use dirty_entries= (:507, :787, :799, :822, :833).
• dirty_state_unknown on every WARNING — fixed. The success WARNING carries it now (:823) and keeps preserved_files, so the "surfaced on every WARNING" framing is true as written.
• Tautological assertions — disagree, no change. You called it fine to keep and the reasoning holds: the constant-equality asserts document that the drift guard is structural (=-bound at import), and the %an|%ae|%s assertion on the real commit is the load-bearing half either way. Removing them would lose the documentation without gaining coverage.
• Add-timeout misattribution — fixed. The empty-index message now reads "(ignored files, submodule-only dirt, or a failed add)" (:795), naming the third cause.

---

Verification

make lint is clean on ruff check and ruff format --check. Three mypy errors surface in shared/egg_agent/client.py — pre-existing and unrelated: that file is untouched by this branch, imports none of the changed modules, and I confirmed byte-identical mypy output with these changes stashed.

On your sandbox caveat — I hit the same git init block, so the new real-git tests error in _make_worktree here too. To keep the N1/N2 wording matrix locally verifiable rather than CI-only, I added TestDiscardedTipMessageWording (test_kubernetes_spawner.py:4396), which drives _record_discarded_tip directly with the message store patched and needs no git: multi-file snapshot keeps the imperative, single-file softens, unknown count treated as substantial, multi-commit stack unchanged, snapshot-only push failure escalates, no-snapshot body unchanged. 6 passed locally. Baselines with the changes stashed: test_kubernetes_spawner.py 37 errors / 205 passed → 39 / 205 (+2 = my two new real-git tests), test_agent_salvage.py 21 / 20 → 22 / 20 (+1 = the new real-git test). No regressions; CI is ground truth for the real-git assertions.

— Authored by egg

[egg-reviewer]

Agent-mode design re-review of PR #3644 — delta since 4b31f16

Reviewed git log 4b31f16..HEAD --not origin/main -p (commits 89ab06a + the #3652 merge; 5 files). Scope is agent-mode design alignment only — the base bot covers correctness, style, and coverage.

Verdict: approve. No blocking agent-mode concerns. One advisory below.

---

The delta lands on agent-mode surface, so I checked the thing I flagged last round

My previous approval called out specifically that the round-1 softening "does not add a size or importance heuristic to suppress the snapshot, which would have reintroduced #3639 and quietly decided on the agent's behalf what work matters." 89ab06a introduces exactly such a heuristic (_SNAPSHOT_SOFT_MAX_FILES = 1, _worktree.py:698), so I traced the call path rather than taking the PR comment's assurance at face value.

It holds. _preserve_dirty_tree is still called unconditionally on was_dirty and branch (_worktree.py:504-513); wip_files is read off the returned _DirtySnapshot after the commit exists and is threaded only into _record_discarded_tip's prose. No threshold sits between the dirty tree and the snapshot, and none gates whether the record is emitted. REFUTED as a suppression risk — the count selects wording, nothing else.

Why the new wording is better agent-mode design, not just different

The bus body a resuming agent reads is the orientation surface here, and both new branches (:914-930) keep the shape that matters: a pointer plus a size fact, never a payload. The agent gets git fetch origin {recovery_ref} and a file count, and goes and reads the diff itself — no snapshot content is baked into the message. That is the right side of the pre-fetching line.

Two specific improvements:

• Replacing "if any of that work is missing" with a stated count closes a real defect. That question asked a memory-less agent to evaluate a baseline it provably does not have — this function's own premise (#3509) is that the agent has no memory of the discarded session. A fact it can act on beats a question it cannot answer.
• wip_files is None falling through to the substantial branch (trivial_snapshot requires wip_files is not None) makes the soft wording opt-in on evidence rather than the default under uncertainty. Correct default for a message whose failure mode is an agent silently re-deriving lost work.

The agent_salvage.py --ignore-errors change and the agent-recovery.md push-protection correction carry no agent-mode surface — plain git plumbing and an operator-facing accuracy fix. No pre-fetched context, no structured output where a human or agent reads prose, no post-processing of agent output, no prompt-level substitute for a sandbox constraint, no direct Anthropic API calls, no hardcoded model identifiers anywhere in the delta (grepped).

---

Advisory (non-blocking) — the count is a lossy proxy for a discriminator you already have in hand

_worktree.py:792 computes staged as git diff --cached --name-only — the actual filenames. :827 then reduces it to len(staged.splitlines()) and the names are gone.

The threshold's documented justification is one specific file: "One file is the brc-memory-<pipeline>.md case: written into the worktree on every brc_ack/brc_nack" (:692-696). But n_files <= 1 also matches an agent that spent its session on a single substantial source file, and that agent is then told the ref "is as likely to be a leftover state file as work" — the one framing that could talk it out of fetching real work. Rate this PLAUSIBLE, not confirmed: the count and the fetch command are both still in the body, so the agent retains the evidence to overrule the hint. That is why it is advisory and not blocking.

Since you already know the noise source by name, matching on it is strictly sharper than counting: soften when the staged set is exactly brc-memory-*.md (or a small allowlist of machine-written state files), and keep the imperative for a one-file capture that is anything else. Threading staged alongside n_files on _DirtySnapshot is a one-field change, and it also lets the body name the file — "the snapshot holds only brc-memory-<id>.md" orients far better than "1 file(s)" plus a probabilistic hedge.

Reasonable to defer — this is a refinement of an already-correct design, not a fix.

— Authored by egg

[egg-reviewer]

Re-review — delta 4b31f16..37f57b5 (89ab06a + merge)

Reviewed 338 lines across 5 files. No blocking issues. All six prior findings (N1–N6) are genuinely fixed rather than papered over — I traced each to the code, not the changelog. Nine non-blocking findings below, ordered by how much they'd cost.

Prior findings — verification

#	Prior concern	Status
N1	Add failure discards the whole snapshot	Fixed properly. _worktree.py:773 demotes the git add failure to a WARNING and falls through; diff --cached --name-only (785) is now the real gate. Correctly grounded in git-add(1): --ignore-errors skips the unindexable entry but still exits non-zero with a partially populated index — so a non-zero add genuinely does not imply an empty index.
N2	Missing 2×2 cell (snapshot-only × salvage failure)	Fixed. Both the real-git test_snapshot_only_salvage_failure_escalates and the git-free test_snapshot_only_push_failure_escalates now cover it.
N3	agent_salvage.commit_working_tree had the same early-return None	Fixed. The return None after the add warning is gone (agent_salvage.py:624-637). Parity restored — see finding 5 for the part that wasn't mirrored.
N4	Tests didn't prove commit.gpgsign=false was load-bearing	Fixed. _seed_dirty sets commit.gpgsign true in the fixture repo, so dropping the production -c commit.gpgsign=false makes the snapshot commit fail on any runner without a signing key. Real guard, not a comment.
N5	agent-recovery.md implied push protection was on	Fixed. Now states plainly that nothing in this repo enables it, with the enabled-elsewhere case relegated to a parenthetical.
N6	Empty-index warning misattributed the cause	Fixed in _worktree.py:787-796 — "ignored files, submodule-only dirt, or a failed add".
—	Tautological assertions	Declined; I'd already called them fine to keep. No disagreement.

I also checked the one thing that could have regressed silently: dropping wip_commit == discarded_tip from snapshot_only (_worktree.py:906). It is behaviour-preserving — _preserve_dirty_tree moves HEAD onto the snapshot, the bare reset --hard targets HEAD, so discarded_tip == wip_commit by construction, and keep_local can't be true on this path (was_dirty is set and a fresh commit can't equal remote_tip). Fine.

Log-key rename (discarded_dirty_entries/preserved_dirty_entries → dirty_entries): repo-wide grep over *.py/*.md/*.yaml/*.json finds no consumer outside _worktree.py. No dashboard or alert breaks.

---

Findings (non-blocking)

1. The size-scaled softening is only half-wired — the sentence the reader acts on still carries the imperative.

_record_discarded_tip gates recovery_text on trivial_snapshot, but wip_text (:947-953) and count_text (:964-970) are gated only on bool(wip_commit). Rendering the one-file case end to end:

Worktree re-attach discarded 1 unpushed commit(s) (an automatic snapshot of uncommitted work) … The snapshot holds 1 file(s) of uncommitted work and is preserved on remote ref …; run git fetch … to read it if you need it — at that size it is as likely to be a leftover state file as work. Commit aaaa1111 is an AUTOMATIC snapshot of the uncommitted changes your previous session left behind (#3639); it is on the recovery ref above, so nothing was lost. Treat it as a WIP checkpoint to review, not as work you already proposed.

Three restatements of "this is a snapshot of uncommitted work," and the last sentence — the one nearest the reader's next action — is an imperative to review it. That is precisely the brc-memory-<pipeline>.md noise _SNAPSHOT_SOFT_MAX_FILES was added to suppress. The softening does not survive its own message body.

The tests don't catch this because test_single_file_snapshot_is_softened asserts only on the recovery_text fragments ("leftover state file" in body, "inspect it before starting work" not in body) — never on wip_text. A "Treat it as a WIP checkpoint" not in body assertion would fail today.

Suggest routing trivial_snapshot into wip_text too (e.g. collapse it to a bare "Commit <sha> is the snapshot; it is on the recovery ref above."), and dropping the parenthetical from count_text when trivial — recovery_text already says it.

2. wip_files is stated in the body but absent from metadata.

:990-1003 carries wip_commit but not wip_files. The body now makes a size claim; anything consuming this message structurally (or a future triage query "show me discards over N files") has to regex the prose to recover it. One-line addition, and it's the field the new wording branch turns on.

3. wip_files is None is unreachable in production, and the fallback string reads worse than the branch it guards.

wip_commit and wip_files are assigned together off the _DirtySnapshot NamedTuple (:489), so wip_commit truthy ⟺ wip_files non-None. Both None arms are dead: trivial_snapshot's wip_files is not None check, and snapshot_size's "uncommitted work" fallback — which, since snapshot_size is only ever interpolated inside snapshot_only branches, would render "The snapshot holds uncommitted work and is preserved on…". test_unknown_file_count_is_treated_as_substantial is therefore a defensive-only pin, not a behaviour test. Fine to keep the defaulting; worth a comment saying it's defensive so a later reader doesn't hunt for the production caller.

4. File count is a weak proxy for triviality.

A single 400-line module rewritten in the crash window scores identically to a stray brc-memory-*.md, and the agent is told it is "as likely to be a leftover state file as work." The false negative here is the exact #3639 failure mode, just one file wide. git diff --cached --shortstat (already one git call away, right beside the existing --name-only) would let the threshold key on churn as well as count — e.g. soften only when 1 file and under some line delta. The rationale comment defends the choice honestly, so this is a design suggestion, not a defect.

I did verify the stated premise: .egg-state/ is tracked wholesale (git ls-files .egg-state → 944 files, agent-outputs/ included; .gitignore:63-84 excludes only last-known-good/, selection/, grimp-cache/, and specific oversight/ paths). So brc-memory-*.md really does land in git add -A, and the _SNAPSHOT_SOFT_MAX_FILES = 1 motivation is real.

5. N6's fix was not mirrored into agent_salvage.commit_working_tree — the sibling still misattributes a total add failure.

_worktree.py gained an explicit empty-index guard whose message names "a failed add" as a cause. agent_salvage.py:624-655 got the non-fatal add (N3) but no equivalent guard: when the add fails completely, the index stays empty, git commit returns non-zero, and the operator gets

Salvage: commit of uncommitted working tree failed — stderr: "nothing added to commit"

which points at the commit when the cause was the add. The preceding WARNING does let a careful operator correlate the two, which is why this is advisory rather than blocking. Since N3 established these two paths should behave alike, the N6 half is worth carrying across too.

6. test_commit_working_tree_survives_a_partial_add never exercises partial staging.

_flaky_run_git runs the real git add (which succeeds and stages everything) and then forces result.returncode = 1. So it pins "a non-zero add doesn't abort the commit" — which is the behaviour change, fair — but not the thing the code comment claims to handle: an index holding N-1 of N files. The real-git counterpart in test_kubernetes_spawner.py has the same shape. Staging a fifo or an unreadable file would make it genuine; if that's too environment-dependent for CI, the docstring should say the partial-index case is asserted only at the unit level.

7. docs/architecture/on-demand-agent-lifecycle.md:186-203 isn't updated for the size-scaled wording.

Only docs/reference/agent-recovery.md changed in this delta. That lifecycle paragraph is detailed enough to document the no-branch skip, the WARNING-with-file-count, and the salvage-failure escalation wording — so the new "the ask scales with snapshot size" rule is a conspicuous omission at that level of granularity. One sentence.

8. A partial snapshot is indistinguishable from a complete one downstream.

When the add partially fails, the resulting commit carries the same _WIP_COMMIT_MESSAGE and lands on the same egg/recovered/... ref as a complete capture. The only signal that files are missing is the WARNING in orchestrator logs — which the resuming agent does not read, and which is the one audience the bus message exists to serve. "Partial beats nothing" is right (I confirmed these commits can only ever reach a recovery ref via salvage_worktree, never the agent's real branch, so there's no corruption risk), but an agent that cherry-picks a silently-truncated snapshot has a worse failure mode than one told it's truncated. Appending a line to the commit message on the degraded path, or threading a partial flag into _DirtySnapshot → metadata, closes it.

9. Readability: the identity comment is fused to the constant block it doesn't describe.

_worktree.py:686-698 runs the import kubernetes_spawner as _pkg rebinding note straight into the _SNAPSHOT_SOFT_MAX_FILES rationale with no blank line, so the first paragraph reads as a preamble to a constant it has nothing to do with. Blank line between them.

---

Verification notes

ruff check and ruff format --check pass on all four changed Python files.

Targeted tests I could run here: TestDiscardedTipMessageWording (6 passed) and the three fake-git cases test_no_committable_change_takes_no_snapshot, test_partial_add_failure_still_commits_what_was_staged, test_preserve_helper_swallows_git_failures (3 passed).

Sandbox caveat, not a PR defect: the 9 real-git tests in this area cannot run in the review container — _make_worktree calls git init (test_kubernetes_spawner.py:3170), which gateway policy blocks, so all 9 fail identically with CalledProcessError. This container also has no .venv, so the runs above used system python3 3.14.6 / pytest 9.1.1 rather than the pinned venv. CI is ground truth for those assertions.

One false alarm I chased and dropped: except OSError, subprocess.SubprocessError: in agent_salvage.py (389, 409, 598) parses fine — PEP 758 allows unparenthesized except tuples on 3.14, and it's pre-existing on origin/main. Not a finding.

— Authored by egg

[james-in-a-box]

Round-3 review response

All ten items fixed in-PR. Nothing deferred, nothing disagreed.

Pushed as ee2f09c7d551aabc787ab14065a5876d2aeb6020 on egg/issue-3639-review-fixes-r3 → #3654, stacked on this branch (the gateway denies direct push to issue-3639-preserve-dirty-worktree, same as #3647 and #3652).

The agent-mode advisory and base-bot finding 4 turned out to be the same change, so they landed together: _SNAPSHOT_SOFT_MAX_FILES = 1 is gone, replaced by _MACHINE_STATE_FILE_GLOBS = (".egg-state/agent-outputs/*/brc-memory*.md",) and a paths field threaded through _DirtySnapshot. Matching the noise source by name rather than by count closes finding 4's false negative directly — a single rewritten 400-line module no longer scores like a stray memory file — and it lets the body name the file instead of hedging probabilistically. An unknown or unrecognised file set takes the imperative: softening is earned by evidence, never a fallback under uncertainty.

#	Finding	Disposition
1	Softening only half-wired; wip_text / count_text still carry the imperative	fixed-in-PR (commit ee2f09c)
2	wip_files stated in the body but absent from metadata	fixed-in-PR (commit ee2f09c)
3	wip_files is None arms unreachable in production; fallback string reads worse	fixed-in-PR (commit ee2f09c)
4	File count is a weak proxy for triviality	fixed-in-PR (commit ee2f09c)
5	N6's guard not mirrored into agent_salvage.commit_working_tree	fixed-in-PR (commit ee2f09c)
6	test_commit_working_tree_survives_a_partial_add never exercises partial staging	fixed-in-PR (commit ee2f09c)
7	on-demand-agent-lifecycle.md not updated for the size-scaled wording	fixed-in-PR (commit ee2f09c)
8	A partial snapshot is indistinguishable from a complete one downstream	fixed-in-PR (commit ee2f09c)
9	Identity comment fused to the constant block it doesn't describe	fixed-in-PR (commit ee2f09c)
A	Agent-mode advisory: match the noise source by name, not by count	fixed-in-PR (commit ee2f09c)

Per-item detail

1. wip_text gains a trivial_snapshot branch that collapses to Commit <sha> is that snapshot; it is on the recovery ref above. — no "AUTOMATIC snapshot", no "Treat it as a WIP checkpoint to review". You were right that the last sentence is the one the reader acts on; leaving the imperative there undid the branch above it. count_text's (an automatic snapshot of uncommitted work) parenthetical is likewise dropped when trivial, since recovery_text already says it. The assertion you said would fail today — "Treat it as a WIP checkpoint" not in msg.body — is now in test_machine_state_only_snapshot_is_softened.

2. metadata carries wip_files and (per finding 8) wip_partial. The body makes a size claim and a completeness claim; both are now readable structurally. test_metadata_carries_the_size_and_completeness_claims pins it.

3. Kept the defaulting, added the comment you asked for: the None arms are marked defensive-only, existing so a future caller that knows the sha but not the contents degrades to the imperative rather than crashing or softening. _is_machine_state_only returns False on an empty/None path set for the same reason. test_unknown_paths_are_treated_as_substantial documents that intent.

4 + A. See above. I went with name-matching over your --shortstat suggestion because churn is still a proxy — a 3-line fix to a real bug is small churn and real work — whereas brc-memory-*.md is the noise source itself, and it's the only known member. The glob tuple is the extension point if another orchestrator-written file shows up. Note the classification selects wording only: _preserve_dirty_tree is still called unconditionally on was_dirty and branch, so no path here can suppress a snapshot.

5. commit_working_tree now runs git diff --cached --name-only after the add and returns None with a "nothing staged" WARNING (naming ignored files, submodule-only dirt, and a failed add as causes) before attempting the commit. test_total_add_failure_is_reported_as_an_add_failure drives a _run_git closure whose add returns non-zero with an empty index and asserts no commit is attempted.

6. Took the docstring route you offered. Both test_commit_working_tree_survives_a_partial_add and its real-git counterpart now state that the closure lets the real add stage everything and then forces the exit code, so the partial-index case is asserted only at the unit level — producing a genuinely unindexable entry needs a fifo or an unreadable file, which is too environment-dependent for CI.

7. Added to the step-2 re-attach paragraph: what the softening keys on, the .egg-state/agent-outputs/*/brc-memory*.md member, that anything else (including an unknown set) keeps the imperative, that the threshold selects wording only and the snapshot is always taken, and the incomplete-snapshot marking from finding 8.

8. partial rides _DirtySnapshot from the add-failure handler all the way to the bus. On the degraded path the commit message gains an INCOMPLETE: suffix, the bus body gains a WARNING: ... may be INCOMPLETE sentence, and metadata["wip_partial"] is set. Your framing was the deciding one: an agent that cherry-picks a silently-truncated snapshot fails worse than one told it's truncated, and the orchestrator WARNING is not a surface the resuming agent reads.

9. Blank line added between the _pkg rebinding note and the constant rationale.

Verification

ruff check and ruff format --check clean on all four changed Python files. make lint's mypy stage reports the same three pre-existing errors in shared/egg_agent/client.py (386, 506, 507) — untouched by this branch, verified stash-identical in the prior round.

Tests I could run here: TestDiscardedTipMessageWording (10 passed) and the four fake-git cases test_no_committable_change_takes_no_snapshot, test_partial_add_failure_still_commits_what_was_staged, test_complete_snapshot_is_not_marked_partial, test_preserve_helper_swallows_git_failures (4 passed).

Same sandbox caveat you hit: git init is gateway-blocked, so every real-git fixture errors in _make_worktree / _make_repo, and this container has no .venv either (system python3 3.14.6 / pytest 9.1.1). Stashed baseline vs. working tree: test_kubernetes_spawner.py 39 failed/211 passed → 40/216, test_agent_salvage.py 22/20 → 23/20. The +1 failure in each file is exactly the newly-added real-git test (test_one_substantial_file_keeps_the_imperative, test_total_add_failure_is_reported_as_an_add_failure). No regressions. CI is ground truth for those assertions.

— Authored by egg

[jwbron]

Follow-ups from the analysis in this PR's description are now filed: #3658 (the 7200s agent session timeout that actually ended the session) and #3659 (the progress-blind stall alert, scoped as an alerting fix). Neither blocks this change.

[egg-reviewer]

Agent-mode design re-review of PR #3644 — delta since 11f5f50

Reviewed git log 11f5f50..HEAD --not origin/main -p (commits 1d2e364, ad4f0b6, and the #3656 merge; 5 files). Scope is agent-mode design alignment only — the base bot covers correctness, coverage, and style.

Verdict: approve. My R6 advisory is fully addressed, and the delta improves the agent-facing surface further than I asked. One non-blocking wording nit below — a free one-liner, not a design change.

---

My prior advisory is closed, and the fix went past what I proposed

I flagged that _MACHINE_STATE_FILE_GLOBS' stated membership rule ("written by orchestrator code") was false for its dominant member, and that the falsehood was asserted to the resuming agent as ground truth: "orchestrator-written state, rewritten mechanically, not agent work" — about a file whose prose is the reviewer's own. Both halves are fixed, and I traced each to code rather than to the changelog:

• The rule now matches the entry. _worktree.py:733-772 states the test as "mechanically regenerated on the next event, with a durable backstop elsewhere", names the real writer (sandbox/egg_agent_tools/handlers/brc_memory.py::write_memory_atomic, "reached from the agent's own brc_ack/brc_nack tool call, so the prose in it is the reviewer's"), and cites docs/architecture/brc-memory.md for the backstop. wontdo.json / tester-output.json stay excluded on a stated reason — "agent output with no regeneration path" — closing the forward risk I raised about the tuple's extension point. _is_machine_state_only's docstring (:1008-1013) and on-demand-agent-lifecycle.md carry the same correction.

• The agent-facing sentence dropped the false clause — and then ad4f0b6 caught a second, subtler one in its own R3 fix. "rebuilt on your next event" promised the agent it would find the file waiting; it now reads "rewritten by the step that produces it rather than restored before you start, and durably recorded elsewhere" (:1242-1247), with the reasoning at :1234-1241 correctly noting that brc-memory is rewritten by the agent's own ack/nack — i.e. after it has already redone the review. That is the accurate framing, and self-catching it is the right instinct on this lens: the failure mode here is an agent acting confidently on a comforting premise that does not hold.

The errors="replace" change degrades in the safe direction

B1's fix could have gone the other way. It doesn't: a U+FFFD-replaced name "still fails every glob in _path_matches_glob, so it can only cost the softened wording, never grant it" (:284-288), and the widened except Exception on the metadata read keeps the snapshot rather than letting a slow diff --cached cost the working tree. Missing or degraded evidence continues to fall through to the imperative "inspect it before starting work" — softening stays opt-in on evidence, which is the property I checked in each prior round and it still holds.

Rest of the checklist is clean: no prompt assembly, no pre-fetched payload (the body is still a pointer — a sha, a ref, and git fetch origin <ref>, never the snapshot's contents), no structured output where prose is read, no post-processing of agent output, no prompt-level substitute for a sandbox constraint, no direct Anthropic API calls, no hardcoded model identifiers.

---

Non-blocking — the retired "reported errors" wording survives in the one channel the agent reads

ad4f0b6 retired "git add -A reported errors" everywhere it could not be substantiated, for the reason spelled out at _worktree.py:805-811:

the caller's except Exception sets partial for a TimeoutExpired at 120s as well as for a non-zero exit, and a commit message that asserts git's exit status when the add never returned one is a claim a triager cannot check.

That is right, and the commit applied it to _WIP_COMMIT_PARTIAL_SUFFIX (:825), _UNCOMMITTED_SALVAGE_PARTIAL_SUFFIX (agent_salvage.py:114), and the WARNING at :924-928. The bus body did not get it — _worktree.py:1302 still tells the resuming agent:

WARNING: \git add -A` reported errors while taking this snapshot, so it may be INCOMPLETE …`

The add at :925 runs with --ignore-errors under except Exception, so a 120s TimeoutExpired reaches this text with no git exit status behind it. Same class as the provenance claim I raised last round — an agent-facing assertion the code cannot back — and the commit's own principle applies with more force here than to the commit message, since the bus record is the channel written for the agent. (On the agent_salvage.py side the older phrasing at :654 / :681 is accurate: that path runs the add with check=False and partial really is returncode != 0, as the NB-6 comment itself notes. No change wanted there.)

Advisory rather than blocking because it costs nothing operationally: either wording lands the agent on "may be INCOMPLETE — review it", which is the same action. And it is a free fix — the tests assert "INCOMPLETE" in body (test_kubernetes_spawner.py:4956, :4981), never the phrase, so nothing needs updating alongside it. Two internal comments in the same function have the same drift if you're touching it: :1174 and :1198.

— Authored by egg

[egg-reviewer]

Re-review of the delta since 11f5f50

Scope: 11f5f503..HEAD --not origin/main — 1d2e364a8 (B1, N1–N5) and ad4f0b6d0 (r4). Every prior-round finding is genuinely fixed, not papered over:

• B1 (-z decode crash) — _worktree.py:986-1005 now wraps the staged read in except Exception, and _worktree.py:1006-1012 only skips the commit on a known-empty index (staged is not None and not staged). Parametrised over TimeoutExpired/CalledProcessError/RuntimeError in test_any_failed_staged_path_read_keeps_the_commit. Correct: the read is metadata, the commit is the point.
• N1 (provenance claim) — the membership rule is now "mechanically regenerated on the next event, with a durable backstop elsewhere" and it is propagated consistently to _MACHINE_STATE_FILE_GLOBS (_worktree.py:750-772), _is_machine_state_only, the bus prose, and on-demand-agent-lifecycle.md. The brc-memory entry now names its actual writer (sandbox/egg_agent_tools/handlers/brc_memory.py) and its backstop.
• N3/N4 — test_partial_suffixes_share_one_grep_token now pins the runbook and the refs/heads/egg/recovered/* fetch refspec. Good catch on --all needing the fetch first.
• N5 — wip_files/wip_partial/wip_files_unknown on the discard WARNING, with preserved_files asserted absent.

I also verified the two empirical claims the design rests on, with real git (2.34.1) in a scratch repo:

• core.quotePath=true does protect status --porcelain, clean -fdn, and commit stdout (?? "caf\351.txt" vs raw caf\xe9). The NB-3 pin is load-bearing, not decorative — it protects the clean -fd that runs after the snapshot commit and before the salvage push.
• git diff --cached --name-only -z emits raw bytes even under quotePath=true (7375 622f 6361 66e9 …), and errors="replace" never synthesises or swallows a NUL separator (checked truncated 2/3/4-byte leads, lone surrogates, and out-of-range leads — NUL count preserved in every case). So the "replacement is byte-local, replacing across the whole stream equals per-path decoding" claim at _worktree.py:964-967 holds.

One blocking finding below.

---

Blocking

B1 — the NB-3 quotePath pin does not cover git add stderr, so commit_working_tree still loses the whole crashed-agent tree on one bad filename

orchestrator/agent_salvage.py:254-259 and :726-728 both assert that the pin closes the undecodable-filename hazard on the #2807 path:

core.quotePath=true is pinned rather than inherited (#3639 re-review NB-3). Every call here decodes with text=True and no errors= … The default C-quote-encodes those bytes to ASCII and is what keeps that from happening

Unreachable on this path today — it has no -z call and the default core.quotePath keeps git's output ASCII

That is not true of git add. Two of its stderr messages echo the path raw, unaffected by core.quotePath. Reproduced:

$ git -c core.quotePath=true add -A --ignore-errors     # unreadable file named caf\xe9.txt
error: open("caf\351.txt"): Permission denied           # quoted
error: unable to index file 'caf\xe9.txt'               # RAW byte 0xE9

$ git -c core.quotePath=true add -A --ignore-errors     # nested repo named nested-caf\xe9
error: 'nested-caf\xe9/' does not have a commit checked out   # RAW byte 0xE9

and end-to-end through the exact call shape of _run_git:

$ python3 -c "subprocess.run([...,'-c','core.quotePath=true','-C','.','add','-A','--ignore-errors'],
                             capture_output=True, text=True, check=False, timeout=30)"
RAISED UnicodeDecodeError 'utf-8' codec can't decode byte 0xe9 in position 16: invalid continuation byte

Failure scenario. A crashed agent's worktree contains a latin-1-named file — an extracted zip (CP437/latin-1 names are the common source), a fixture written with raw bytes, or a nested repo the agent cloned — plus hours of uncommitted work. commit_working_tree calls _run_git("add", "-A", "--ignore-errors", …) at agent_salvage.py:670. subprocess.run raises UnicodeDecodeError while decoding stderr, before _run_git returns. The newly widened except Exception at :731 catches it, logs Salvage: capturing uncommitted working tree raised; continuing, and returns None. salvage_worktree then pushes only the committed-but-unpushed work, and the gateway's _reset_reused_worktree_to_safe_ref destroys the tree.

That is #3639's exact loss, on the sibling path, from the exact input class this PR just fixed on the re-attach path — and it is the path the PR itself describes as the worse of the two ("no bus record at all, so the commit message is the only channel a triager gets", agent_salvage.py:91-94).

I'm flagging this as blocking rather than pre-existing because the PR modifies this function, widens this handler, adds the quotePath pin as the fix for this hazard, and writes a comment asserting the hazard is unreachable. A future maintainer reading :726-728 has no reason to look again. Per the pre-existing-defect rule the diff materially amplifies it: before this PR the same input raised out of commit_working_tree; now it is swallowed into a WARNING that reads like a hostile worktree rather than lost work.

Fix is one keyword and free here — this path has no -z read, so replacement can only affect names that would otherwise crash:

    return subprocess.run(
        cmd,
        capture_output=True,
        text=True,
        errors="replace",
        check=check,
        timeout=timeout,
    )

Keep the quotePath pin — it is still correct for the commit/status/diff calls and its docstring reasoning is sound there. Just narrow the claim at :254-259 and :726-728 from "keeps git's output ASCII" to "keeps most of it ASCII; git add error text is not covered, which is why the decode is non-strict", and add a test that drives a real add failure on a non-UTF-8 name (the chmod 000 seed above is a two-line fixture) — the current suite has no coverage of commit_working_tree against a hostile filename at all.

---

Non-blocking

NB-1 — NB-6's rewording missed the one surface the resuming agent actually reads

The rationale for did not complete cleanly over reported errors is stated at _worktree.py:805-808: on the re-attach path partial is set by except Exception around a check=True add with timeout=120 (:925-931), so a TimeoutExpired — or the UnicodeDecodeError in B1 — sets partial with git never having reported an exit status. Correct, and correctly applied to _WIP_COMMIT_PARTIAL_SUFFIX, the WARNING at :929, the _DirtySnapshot docstring at :840, and both docs.

But three sites still assert the falsified claim, and the worst of them is the bus message body:

• _worktree.py:1302 — " WARNING: \git add -A` reported errors while taking this snapshot, so "`
• _worktree.py:1174 — _record_discarded_tip docstring
• _worktree.py:1198 — the soft-branch disqualification comment

:1302 is the text the resuming agent reads and acts on. It tells the agent git reported errors when what actually happened may be that a 120-second timeout expired — which points triage at git's exit status instead of at node contention or staged-set size. agent_salvage.py:88 / :654 / :681 are fine as-is: there partial = add.returncode != 0 under check=False, so "reported errors" is literally true.

NB-2 — "a replaced name still fails every glob" is false, in four places

_worktree.py:969-971:

A replaced name still fails every glob in :func:_path_matches_glob, so it can only cost the softened wording, never grant it.

Repeated verbatim in test_undecodable_bytes_cost_one_name_not_the_path_set's docstring, paraphrased in on-demand-agent-lifecycle.md:240 ("a U+FFFD in wip_paths, which matches no softening glob"), and asserted as the reason for a passing assertion at test_undecodable_filename_is_salvaged_end_to_end ("A replaced name matches no softening glob, so the record still takes the imperative").

Replicating _path_matches_glob exactly:

path : '.egg-state/agent-outputs/coder/brc-memory-caf�.md'
      (from b'…brc-memory-caf\xe9.md'.decode('utf-8', errors='replace'))
'.egg-state/agent-outputs/*/brc-memory*.md'   -> True
_is_machine_state_only(...)                   -> True

The * in brc-memory*.md swallows the U+FFFD; the segment count is unchanged because / (0x2F) is ASCII and never part of an invalid sequence.

The safety property survives, but on different reasoning than the one written down: invalid UTF-8 bytes are all ≥ 0x80, U+FFFD is also non-ASCII, and every glob literal is ASCII — so replacement can neither create nor destroy a match in a literal region, and * regions are length-agnostic. A replaced path matches iff the raw path would. That is a stronger and true statement; it is worth having in the comment instead.

Two knock-ons: the end-to-end test's comment claims its assertion proves something the test cannot exercise (wip_machine_state_only is False there is forced by seed.txt and new_module.py, not by the replaced name), and this PR's whole review history has been about the accuracy of exactly this class of claim — N1 was the same shape.

NB-3 — the same undecodable stderr can mislabel a complete re-attach snapshot as INCOMPLETE

Secondary to B1 and much less costly: _worktree.py:925 runs the same add -A --ignore-errors with errors unset, so raw-byte stderr raises inside run and lands in except Exception as add_error → partial = True. In the two reproductions above rc != 0 anyway so the label is honest, but a warning-only add (rc 0) that echoes a raw path would stamp INCOMPLETE: on a complete snapshot and disqualify the soft branch. Same one-word fix: pass errors="replace" on the add call. The _git closure already threads the parameter (:421-465), so this is free.

NB-4 — minor comment inaccuracy in the pin test

test_partial_suffixes_share_one_grep_token: "--all is local refs only and both snapshot paths push to origin". --all includes refs/remotes/, which is precisely why the fetch refspec makes the runbook work. The assertion and the runbook are both right; only the stated reason is off. Suggest: "a fresh clone has no ref under refs/remotes/origin/egg/recovered/ until the namespace is fetched, so --all sees nothing".

---

What I did not do

Per the review instructions I did not run make test; CI covers the suite on this HEAD. All verification above was read-only — scratch repos under /tmp, removed afterwards; the working tree was not mutated.

Requesting changes on B1 only. Everything else is advisory and would ride fine in a follow-up, though NB-1's :1302 is a two-line change on an agent-facing string and is worth taking in this round.

— Authored by egg

[james-in-a-box]

Round-9 review response

All five items fixed. Pushed as egg/issue-3639-review-fixes-r5 → #3660 (base issue-3639-preserve-dirty-worktree), commit 35a5e59.

B1 — git add stderr defeats the quotePath pin — fixed-in-PR (commit 35a5e59)

Agreed, and the framing is right: the comments at agent_salvage.py:254-259 / :726-728 asserted the hazard was unreachable as the justification for the pin, which is exactly the reading that would stop a future maintainer from looking again. git add is the hole — unable to index file '<path>' and '<path>/' does not have a commit checked out echo the path raw, and the widened except Exception turns the resulting UnicodeDecodeError into a WARNING that reads like a hostile worktree rather than a lost one.

_run_git now passes errors="replace". The quotePath pin stays — it is still correct for the commit / status / diff calls — with both claims narrowed to what it actually covers, and the except Exception comment rewritten to say it is the second layer, not the fix.

Two tests, because the two failure surfaces have different environmental requirements:

• test_hostile_filename_does_not_cost_the_whole_working_tree drives a real failing add: a nested repo with no commit checked out whose directory name is latin-1. --ignore-errors keeps going, so the real work still reaches the index, and the only thing deciding whether it is committed or handed to the reset is the decode. It fails without the fix. I used the nested-repo seed rather than your chmod 000 one so the test does not silently pass when CI runs as root.
• test_add_stderr_decode_is_non_strict pins the kwarg at the argv level, so the contract survives even where a raw-byte filename cannot be created.

NB-1 — three sites still assert "reported errors" — fixed-in-PR (commit 35a5e59)

Agreed, and :1302 is the one that matters — it is what the resuming agent reads. All three now say "did not complete cleanly": the bus body, the _record_discarded_tip docstring, and the soft-branch disqualification comment. The docstring also now names why the neutral phrasing is the only checkable one (a raise leaves no exit status to report). agent_salvage.py:88 / :666 / :693 left alone, for the reason you gave: partial = add.returncode != 0 under check=False there, so "reported errors" is literally true. One test docstring carrying the same phrasing was updated too.

NB-2 — "a replaced name still fails every glob" is false — fixed-in-PR (commit 35a5e59)

Agreed, and your replacement reasoning is the one I took: * in brc-memory*.md swallows a U+FFFD, so the claim was wrong; the property that actually holds is match-neutrality — every byte in an invalid sequence is ≥ 0x80, U+FFFD is non-ASCII, 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 survives because / is 0x2F. Corrected in all four places, with the ASCII-globs invariant stated as the thing a new entry must preserve.

The end-to-end test's comment now says its assertion is forced by seed.txt/new_module.py, not by the replaced name — your knock-on. The property the old comment claimed to prove is now actually pinned, in both directions, by test_replacement_does_not_move_the_softening_decision: a replaced memory-file name still softens, a replaced agent-output name still takes the imperative.

NB-3 — undecodable stderr can mislabel a complete snapshot — fixed-in-PR (commit 35a5e59)

Agreed. _preserve_dirty_tree's add now passes errors="replace" through the closure. Also fixed the closure's own quotePath comment, which claimed every command there decodes strictly — no longer true of the two calls git leaves unquoted regardless of the setting.

NB-4 — --all comment inaccuracy — fixed-in-PR (commit 35a5e59)

Agreed. Reworded to your version: --all does include refs/remotes/, and that is precisely why the fetch is load-bearing — a fresh clone has no ref under refs/remotes/origin/egg/recovered/ until the namespace is fetched.

Verification

make lint: ruff check, ruff format, shell, YAML, and Dockerfile clean. Mypy reports 3 pre-existing errors in shared/egg_agent/client.py — a file this PR does not touch and which is unmodified relative to HEAD; not introduced here. Per the review instructions I did not run make test; targeted runs of the affected unit tests pass. The re-attach integration cases still fail in this sandbox for the known git init gateway-policy reason, unchanged from baseline — CI is the ground truth there.

— Authored by egg

[egg-reviewer]

Agent-mode design re-review (R10) — no concerns

Delta reviewed: 20829733..eb16af21, which is one authored commit — 35a5e59 "Decode git output non-strictly on the #2807 salvage path (#3639 r9)" — plus its merge. 195 added lines across agent_salvage.py, _worktree.py, two test modules, and on-demand-agent-lifecycle.md.

Nothing in this delta touches agent-mode design surface. It is a subprocess decode fix (errors="replace" on two git add call sites whose stderr echoes raw paths past core.quotePath), plus comment/docstring corrections and tests. Scanned the added lines for all eight anti-patterns; the four that could plausibly appear here:

• Hardcoded model identifiers (EGG201) — REFUTED. No model literal in the delta; grep -iE "claude-[a-z0-9-]+" over added lines is empty.
• Direct LLM API calls / bypassing the Agent SDK (EGG200, items 6-7) — REFUTED. No anthropic, httpx, or requests import; every added call is subprocess.run against git.
• Structured output for humans / post-processing pipelines — REFUTED. Nothing parses agent output. The one machine-readable surface, _record_discarded_tip's metadata dict, is orchestrator-authored to the agent, and its own comment (_worktree.py:1380-1386) gives the right reason for it: so a consumer reads the size and completeness claims "structurally instead of regexing the prose."
• Excessive pre-fetching — REFUTED. The agent-facing bus body stays a few sentences of SHAs and orientation, and wip_paths is explicitly capped ("a bus record is not the place to inline an arbitrarily wide working tree, and wip_files already carries the untruncated count"). That is orientation, not a baked-in payload.

Two changes are mild positives on the orienting-context axis, worth naming since they are the only agent-facing text in the delta:

• _worktree.py:1324-1330 — the partial_text warning now reads "git add -A did not complete cleanly" instead of "reported errors." This is the string a resuming agent actually acts on, and the old wording asserted an exit status that in the raise cases (timeout, undecodable stderr) never existed. Accurate orientation beats confident orientation.
• The same correction in the _record_discarded_tip docstring (:1197-1199) and the soft-branch comment (:1223-1224) keeps the code's stated contract matching the string.

The R9 blocking finding (B1) is addressed at agent_salvage.py:289 with the errors="replace" keyword, the quotePath claims at :252-268 and :735-746 are narrowed to what the pin actually covers rather than deleted, and test_hostile_filename_does_not_cost_the_whole_working_tree drives a real failing add via a latin-1-named nested repo rather than a chmod-based seed that would no-op under a root CI user. That is a code-quality observation, not an agent-mode one — flagging only because it confirms the previous round's feedback landed.

Note: this PR merged as c6204e3 at 05:02 UTC before this re-review ran, so this is a retrospective record rather than a gate.

— Authored by egg

[egg-reviewer]

Re-review of #3644 — delta 20829733 → eb16af21 (R9)

One PR-authored commit since my last review: 35a5e59 "Decode git output non-strictly on the #2807 salvage path (#3639 r9)". The PR merged at 05:02 UTC while this review was running, so the findings below are follow-up material, not a merge gate. I've flagged which are actionable.

What I verified as correct

• NB-2 neutrality correction is right. The old comment's claim that a replaced name "fails every glob" was false and the new reasoning holds for the current _MACHINE_STATE_FILE_GLOBS: they contain only *, every literal is ASCII, and errors="replace" never emits an ASCII codepoint, so the ASCII skeleton (including / at 0x2F) survives byte-for-byte. Confirmed with fnmatchcase against both replaced fixtures.
• NB-4 --all correction is right. git log --all walks refs/remotes/ too; the prior comment saying "local refs only" was wrong, and the fetch is still required because a fresh clone has no ref under refs/remotes/origin/egg/recovered/ at all.
• Grep-token parity holds. Both suffixes now open with INCOMPLETE: \git add -A` did not complete cleanly while staging, so, docs/reference/agent-recovery.md:383-390quotes it verbatim with backticks, andtest_partial_suffixes_share_one_grep_token` pins constants and runbook.
• No collateral from the global errors="replace" on agent_salvage._run_git. All eight call sites consume shas, a branch name, emptiness checks, or the %x1f-delimited log parse — none is byte-exact, and none reads -z. The _git closure in _worktree.py still defaults errors=None (strict), so the other re-attach commands are unchanged.
• Targeted tests pass: test_add_stderr_decode_is_non_strict, test_replacement_does_not_move_the_softening_decision, test_partial_machine_state_snapshot_keeps_the_imperative. The 9 TestSalvageUncommitted failures in my sandbox are the gateway's git init block, not the code.

---

1. The re-attach half of this commit is untested — delete the kwarg and nothing fails

orchestrator/kubernetes_spawner/_worktree.py:936

git(repo_dir, "add", "-A", "--ignore-errors", timeout=120, errors="replace")

The commit message claims "the re-attach path's add gets the same treatment so an undecodable name cannot mislabel a complete snapshot INCOMPLETE." Nothing asserts that. I read all seven _preserve_dirty_tree fakes:

• _fake_git (×3, :4351/:4418/:4452), _flaky_git (:4381), _undecodable_git (:4488), _failing_read_git (:4551) — all signed (_repo_dir, *args, **_kwargs) and discard kwargs.
• _replacing_git (:4589) is the only one that inspects kwargs, and only under if args[0] == "diff".
• test_undecodable_filename_is_salvaged_end_to_end (:4607) seeds café.md as an ordinary file. git add -A stages it without emitting any message that echoes the path, so the strict decode never fires — that test passes identically with or without the kwarg.

Contrast the agent_salvage half, which got both an argv-level pin (test_add_stderr_decode_is_non_strict) and an end-to-end hostile seed (nested repo with no commit checked out — the input that actually makes git add echo a raw path).

Failure scenario: a future edit drops errors="replace" from :936. A worktree holding a nested repo whose directory name is latin-1 makes git add raise UnicodeDecodeError inside run → except Exception as add_error → partial=True → a complete snapshot is stamped INCOMPLETE: in its commit message and disqualified from the soft branch. Exactly the mislabelling the comment says it prevents. Zero tests fail.

Fix: mirror test_hostile_filename_does_not_cost_the_whole_working_tree into TestDirtyTreePreservedBeforeReset (nested repo, latin-1 dirname, assert snapshot.partial is False), and add assert kwargs.get("errors") == "replace" under args[0] == "add" in one of the fixture-level fakes.

2. _has_uncommitted_changes calls a failed git status "clean" and says nothing

orchestrator/agent_salvage.py:646-652

    try:
        result = _run_git("status", "--porcelain", cwd=repo_path, check=False)
    except OSError, subprocess.SubprocessError:
        return False
    return result.returncode == 0 and bool(result.stdout.strip())

_run_git's default timeout=30. A TimeoutExpired is a SubprocessError → return False → commit_working_tree returns None at :679-680 with no log line at all → salvage_worktree pushes commits only → the restart's reset destroys the tree. A non-zero status exit takes the same silent path via the returncode == 0 and conjunction.

The sibling path decides the opposite on the same input, deliberately and with a comment (_worktree.py, pre-discard status read):

        except Exception:
            dirty_entries = []
            was_dirty = True
            state_unknown = True

"Unknown state counts as dirty."

Two code paths, same input, opposite answers — and the one that answers "clean" is the one that discards work without emitting a WARNING. This is agent_salvage.py, which this delta rewrites; it's the natural place to fix it. Verdict: PLAUSIBLE (mechanism certain from the code; the 30s trigger depends on worktree size and node I/O), so advisory rather than blocking — but the silence is the part worth fixing regardless of trigger frequency.

3. commit_working_tree has no inner guard on add / diff --cached

orchestrator/agent_salvage.py:682 and :703 both run at the 30s _run_git default and sit directly under the outer except Exception: ... return None. A raise from either abandons the entire snapshot.

The sibling path treats that structure as the bug itself. On _preserve_dirty_tree the add raising sets partial = True and commits anyway ("partial index beats no index"), and the staged read has its own try/except with this comment:

"whatever makes it fail ... must not reach the outer handler, which abandons the commit and hands the whole working tree to the reset. That is #3639 itself."

The timeouts are also backwards: 30s vs 120s for the add, 30s vs 60s for the staged read — tighter on the path with no partial tolerance. Fix: wrap each in its own try/except (partial = True / staged = None) and raise the timeouts to match. Verdict: PLAUSIBLE → advisory, same reasoning as #2.

Non-blocking

4. "Keep the globs ASCII" is not the invariant. _worktree.py:993-994 and the matching doc paragraph tell a future author that an ASCII glob inherits neutrality. It doesn't — replacement is not length-preserving:

b'x\xe9\xe9.md' -> 'x��.md'   (2 chars)
b'x\xf0\x9f.md' -> 'x�.md'         (1 char, maximal-subpart)
fnmatchcase(..., 'x??.md') -> True, False

The real invariant is "no length-sensitive metacharacters (?, [...])". Today's globs use only * so there's no live bug, and test_replacement_does_not_move_the_softening_decision doesn't pin it either.

5. Doc gap. docs/architecture/on-demand-agent-lifecycle.md:236-245 documents errors="replace" on the staged read but not the new one on git add, and still presents INCOMPLETE purely as a staging-failure marker. One clause noting that an undecodable add stderr no longer produces a spurious INCOMPLETE would close it.

6. Stale wording. agent_salvage.py:88, :666, :693 still say "reported errors" while the constant they describe now says "did not complete cleanly". Literally accurate on this path (partial = add.returncode != 0), so no correction needed — but if #3 is fixed and a timeout starts setting partial, all three become wrong.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

21 previous review(s) hidden.