Address review feedback on #3644

[james-in-a-box]

Address review feedback on #3644

The gateway blocks pushing to #3644's head branch (issue-3639-preserve-dirty-worktree is human-owned, not egg/-prefixed), so the review fixes land here as a single commit stacked onto #3644's branch. Merging this PR updates #3644 in place — no force-push needed.

Fixes the blocking finding: the message-bus record told a resuming agent "Nothing was lost" even when the salvage push had failed and the snapshot survived only in the local object store, suppressing the escalation the preceding sentence asked for. The reassurance is now conditional on the push succeeding, and the failure branch no longer points at salvage_agent_commits (which provably cannot see an unreachable sha).

Also hardens the snapshot path: git add -A --ignore-errors plus committing whatever staged when the add exits non-zero, commit.gpgsign=false on both git closures, the snapshot identity imported from agent_salvage instead of duplicated, softened wording for a snapshot-only discard, and dirty_state_unknown in place of a misleading discarded_dirty_entries=0.

Issue: #3639

Test Plan

• Automated: TestDirtyTreePreservedBeforeReset / TestDirtyDiscardAutoSalvage in orchestrator/tests/test_kubernetes_spawner.py — new cases pin both message branches (pushed vs. not-pushed), the snapshot's commit author/email/[salvage] subject against agent_salvage's constants, the branch is None skip (HEAD unmoved, no commit), the empty-index branch, and partial-add recovery.
• Manual: none required. The real-git tests cannot execute in the agent sandbox (git init is blocked by the gateway: "git init is not supported in the container"), so they are CI-verified. Locally: ruff check / ruff format --check clean; targeted fake-git tests pass; test_kubernetes_spawner.py shows 205 passed / 37 failed vs. a 203/35 baseline — the delta is exactly the 2 new real-git tests hitting the sandbox git init block, no regressions.

Manual Steps

• Pre-merge: merge into issue-3639-preserve-dirty-worktree (this PR's base) so Preserve a re-attached worktree's uncommitted work before the R6 reset (#3639) #3644 carries the fixes.
• Post-merge: none.

[egg-reviewer]

No agent-mode design concerns.

The changes are orchestrator-side git plumbing (_worktree.py, agent_salvage.py) plus tests and docs — no prompt assembly, no LLM invocation, no model identifiers, no post-processing of agent output. Nothing in the diff matches claude-*, anthropic, httpx, or claude --print, so EGG100/EGG200/EGG201 are not in play.

The one agent-facing surface here is the message-bus body in _record_discarded_tip, and it reads as aligned rather than constraining:

• It's natural language to a human-or-agent reader, not a structured schema the successor has to parse (guideline 2). The metadata dict alongside it is orchestrator bookkeeping, not agent output being re-parsed.
• It's orientation, not pre-fetching (guideline 1): the body hands over SHAs and a ref name and tells the agent where to look — git fetch origin {recovery_ref}, git reflog — rather than baking a diff into the message. The successor still decides what to pull and whether the snapshot is worth keeping.
• The conditional wording change is more aligned, not less. Making "nothing was lost" contingent on recovery_ref (_worktree.py, the if wip_commit and recovery_ref: / elif wip_commit: split) means the agent is told the truth about where the work actually lives and can act on it, instead of being reassured into skipping the escalation. Same for dropping the salvage_agent_commits pointer on the no-ref path — pointing an agent at a tool that provably cannot see the sha is exactly the kind of wrong pre-digested context guideline 1 warns about.
• The softened snapshot-only wording is a reasonable signal-to-noise call, in the spirit of the "rate limiting to avoid noise" carve-out: a respawn with one stray memory file shouldn't read like data loss and train the agent to ignore the record.

The new docs/reference/agent-recovery.md paragraph ("a recovery ref may hold un-reviewed working-tree residue") is worth calling out positively — it tells a future recovering agent to judge the snapshot rather than replay it mechanically, which is guideline 5 applied to the recovery path.

— Authored by egg

[egg-reviewer]

Review — PR #3647 (post-merge)

This PR merged before review completed, so this lands as follow-up. Finding 1 is a
blocking-class defect and should get a fix PR onto issue-3639-preserve-dirty-worktree
before #3644 merges.

The core fix is right and well-executed: the wip_text reassurance is now correctly
gated on recovery_ref, the salvage_agent_commits mis-direction is gone, and the
constant-duplication removal is a genuine improvement — the old comment's fear
("reading constants off a Mock") was unfounded because a module-level from agent_salvage import _SALVAGE_COMMIT_NAME binds the real value and is unaffected by
patch("kubernetes_spawner.agent_salvage"), which rebinds the package attribute.
I verified this holds (kubernetes_spawner/_worktree.py:12; no import cycle —
agent_salvage only reaches gateway_client under TYPE_CHECKING), and
test_snapshot_commit_identity_matches_the_restart_path pins both halves against real
git output rather than a fixture. That is the right shape.

---

1. BLOCKING — the partial-add path recreates the exact "nothing was lost" lie this PR exists to remove

orchestrator/kubernetes_spawner/_worktree.py:753-764

try:
    git(repo_dir, "add", "-A", "--ignore-errors", timeout=120)
except Exception as add_error:  # partial index beats no index
    logger.warning(...)          # <-- log only; nothing is returned to the caller

_preserve_dirty_tree returns str | None. A snapshot built from a partially populated
index is returned as a bare sha, byte-identical to a complete one. The caller stores it in
wip_commit (_worktree.py:485) and hands it to _record_discarded_tip, which — when the
salvage push succeeded — emits (_worktree.py:890-896):

Commit <sha> 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.

Failure scenario. A re-attached worktree holds 33 modified files. One is unreadable by the
orchestrator process — precisely the uid-mismatch condition _preserve_dirty_tree's own
docstring cites as the reason it must carry safe.directory=* (_worktree.py:744-750).
Per git-add(1), --ignore-errors continues past the bad entry and still exits non-zero.
So: 32 files stage, exit 1, the inner handler logs and falls through, the commit is created,
salvage_discarded_tip pushes it, recovery_ref is set. reset --hard then overwrites file 33
with its HEAD content (or clean -fd unlinks it if untracked). The resuming agent is told
nothing was lost. File 33 is gone, permanently, with an explicit reassurance covering it.

This is a regression introduced by this PR. Pre-PR, a failing git add -A propagated to
the outer handler → return None → wip_commit is None → the message correctly reported a
discard with no reassurance. The PR trades a correct "we lost it" for an incorrect "nothing
was lost." Partial preservation genuinely does beat none — the objection is not to committing
the partial index, it is to reporting it as complete on the one channel the module docstring
calls "the one channel that survives both failure modes" (_worktree.py:833-837).

Fix. Thread the partiality out of the helper and into the record:

# _preserve_dirty_tree
partial = False
try:
    git(repo_dir, "add", "-A", "--ignore-errors", timeout=120)
except Exception as add_error:
    partial = True
    logger.warning(...)
...
return sha, partial          # or a small dataclass / NamedTuple

and in _record_discarded_tip, when snapshot_partial:

Commit <sha> is an AUTOMATIC and INCOMPLETE snapshot — git add could not index every
file, so some of your previous session's work is NOT in it and was destroyed by the reset.
Escalate to an operator.

Also add "snapshot_partial": partial to the message metadata dict (_worktree.py:932-945)
so it is queryable, and extend
test_partial_add_failure_still_commits_what_was_staged to assert the message is qualified —
today that test asserts only that a commit happened, which is exactly the half that is safe.

---

Non-blocking

2. commit.gpgsign=false has zero coverage, and the suite structurally cannot catch a regression.
_worktree.py:434-435 and agent_salvage.py:228-229. The stated stake is total: "every salvage
commit would otherwise fail and lose the working tree it exists to save." But
test_kubernetes_spawner.py:3136-3147 — the _GIT_IDENT used by every _seed_* helper —
already passes "commit.gpgsign=false" on every seed call, so no test repo can ever carry
commit.gpgsign=true. Deleting the new production -c would leave the suite green. This is a
one-line fix in _seed_dirty:

_git(repo, "config", "commit.gpgsign", "true")   # no signing key in the image

then assert test_uncommitted_work_is_salvaged_not_destroyed still produces a snapshot. The
repo already treats this as a live hazard elsewhere (tests/tools/_select_tests_helpers.py:158,
orchestrator/tests/test_evidence_rescue.py:54 both disable it explicitly), so the hardening is
justified — it just isn't pinned.

3. state_unknown is threaded into four WARNINGs but never into the durable record, and is untested.
Nine references across _worktree.py; grep -rn "state_unknown" orchestrator/tests/ returns
nothing. More substantively, the flag is absent from the message-bus metadata dict
(_worktree.py:932-945), which carries was_dirty but no way to distinguish "dirty with 0
entries" from "we could not tell." The PR's own rationale (_worktree.py:461-463) is that
discarded_dirty_entries=0 "reads as 'nothing was there' on the one path where we genuinely do
not know" — that argument applies with more force to the bus record than to the logs, since the
record is what an agent and an operator actually read.

4. docs/reference/agent-recovery.md:382 over-claims GitHub push protection.

(A snapshot containing a secret is rejected by GitHub push protection rather than leaked …)

Stated as unconditional, it is only true for secrets matching a known partner pattern
(provider-issued tokens/keys) on a repo where push protection is actually enabled. A generic
credential — a DB_PASSWORD= line in a config dump, an internal service token, a customer
identifier — is not detected and is pushed. jwbron/egg is public, so egg/recovered/* refs
are world-readable for the full 90-day TTL. This repo gitignores .env / secrets.yaml
(.gitignore:29-34), which mitigates it here, but the sentence sits in a paragraph explicitly
generalising over both snapshot paths and every repo egg operates on. Suggest: "GitHub push
protection will reject a snapshot containing a recognised provider secret; generic credentials
are not detected, so treat a snapshot commit's contents as published."

5. "if any of that work is missing" asks the agent a question it structurally cannot answer.
_worktree.py:864-869. The snapshot-only softening is a reasonable noise trade, but the
conditional is addressed to an agent whose defining property — per this function's own docstring
(_worktree.py:828-837) — is that it has no session memory. It cannot know what is missing;
the work is always missing from its worktree. Consider keeping the softer tone but making the
action unconditional: "run git fetch origin <ref> and skim it before starting work; a
snapshot-only ref often holds nothing you need." Same noise reduction, no impossible predicate.

6. Nits.

• _worktree.py:887 — "salvage_agent_commits cannot recover them (… , #3509)" cites the wrong
  issue. salvage_agent_commits is #3368; the file itself says so at _worktree.py:380. The
  text this PR replaced had #3368 correct. An operator following the link lands on the wrong
  issue.
• test_kubernetes_spawner.py:4189 docstring — "a dirty submodule whose gitlink is unchanged
  shows as M sub". Git reports m (lowercase) for modified submodule content with an
  unchanged gitlink; M means new commits. Minor, but the docstring is the only thing
  documenting why this branch is reachable in production.
• _worktree.py:862 — wip_commit == discarded_tip in snapshot_only is always true when
  wip_commit is set: local_head is re-read at _worktree.py:560, after the snapshot commit
  at :485. Harmless, but it reads as a guard against a case that cannot occur.

---

Verified as correct (no action)

• No import cycle from the new from agent_salvage import … at _worktree.py:12; the constants
  resolve to the real values under the patch("kubernetes_spawner.agent_salvage") seam.
• git add --ignore-errors is the right flag and its non-zero-exit-with-partial-index semantics
  match the docstring at _worktree.py:729-737.
• The branch is None skip is sound: with branch falsy the whole if branch: block at
  _worktree.py:539 is bypassed, so a snapshot commit would indeed survive as the successor's
  HEAD. test_no_branch_takes_no_snapshot correctly pins HEAD-unmoved plus no push and no record.
• The else (push-failed) recovery text's git reflog advice is actionable: _record_discarded_tip
  runs before the reset --hard at _worktree.py:659, the worktree is reused rather than
  recreated, and both the HEAD and work-branch reflogs retain the sha.
• Removing # noqa: BLE001 is inert — ruff select is ["E","F","I","B","C4","UP"]
  (pyproject.toml:68), so neither BLE001 nor RUF100 is active.

— Authored by egg

[james-in-a-box]

egg review completed. View run logs

1 previous review(s) hidden.