From 4bebc07f9daf3dc521f246ff8c9c6efb0e689421 Mon Sep 17 00:00:00 2001 From: "egg-reviewer[bot]" <261018737+egg-reviewer[bot]@users.noreply.github.com> Date: Sun, 26 Jul 2026 22:58:00 +0000 Subject: [PATCH] Fix contradictory salvage-failure message and harden the #3639 snapshot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review feedback on #3644. Blocking: the 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, the failure branch says the snapshot was NOT pushed, and it no longer points at salvage_agent_commits (which provably cannot see an unreachable sha). Hardening: `git add -A` gains `--ignore-errors` and a non-zero add no longer discards the files that did stage; both git closures pass `commit.gpgsign=false`; the snapshot identity is imported from agent_salvage rather than duplicated; a snapshot-only discard reads as "if any of that work is missing" rather than the imperative reserved for losing the agent's own commits; a status-read failure is reported as `dirty_state_unknown` rather than zero entries. Tests: pin both message branches, the snapshot's commit identity, the `branch is None` skip, the empty-index branch, and partial-add recovery. --- .../architecture/on-demand-agent-lifecycle.md | 8 +- docs/reference/agent-recovery.md | 2 + orchestrator/agent_salvage.py | 17 ++- orchestrator/kubernetes_spawner/_worktree.py | 131 ++++++++++++---- orchestrator/tests/test_kubernetes_spawner.py | 141 +++++++++++++++++- 5 files changed, 268 insertions(+), 31 deletions(-) diff --git a/docs/architecture/on-demand-agent-lifecycle.md b/docs/architecture/on-demand-agent-lifecycle.md index 3c6040e708..cb6c9cdbf6 100644 --- a/docs/architecture/on-demand-agent-lifecycle.md +++ b/docs/architecture/on-demand-agent-lifecycle.md @@ -194,7 +194,13 @@ Under orchestrator ownership the worktree becomes a hot path. to the origin tip, so the successor inherits nothing uncommitted; it only makes the discarded state recoverable. Ignored files are excluded, and a failed snapshot logs at WARNING with the file count and proceeds - with the reset rather than blocking reuse. + with the reset rather than blocking reuse. The snapshot is skipped + entirely when the re-attach carries no `branch`: with no origin tip to + reset to and no salvage target, the commit would simply become the + successor's HEAD — un-vetted residue promoted to committed state, which + is what R6 exists to prevent. When the salvage push fails the bus + record says the snapshot was *not* pushed and asks for escalation + rather than reassuring the successor that nothing was lost. 3. **Before handing off to spawn:** translate the validated paths from orchestrator-local (under `WORKTREE_BASE_DIR`) to host paths, matching what the create path already gets from the gateway. An untranslated diff --git a/docs/reference/agent-recovery.md b/docs/reference/agent-recovery.md index 5afbe0c972..aa931ead40 100644 --- a/docs/reference/agent-recovery.md +++ b/docs/reference/agent-recovery.md @@ -380,6 +380,8 @@ git switch git cherry-pick ..recovered/ ``` +**Review before replaying: a recovery ref may hold un-reviewed working-tree residue.** Both working-tree snapshot paths ([#2807](https://github.com/jwbron/egg/issues/2807) restart, [#3639](https://github.com/jwbron/egg/issues/3639) re-attach) stage with `git add -A`, so a snapshot commit contains everything the agent left in the worktree that is not `.gitignore`d — scratch dumps, logs, stray state files — with no agent or human intent behind any of it. Recovery refs are a preservation mechanism, not an endorsement: read the diff before cherry-picking, and expect a snapshot-only ref to sometimes hold nothing you want. (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.) + Operators may delete `egg/recovered/*` refs manually after replay (`git push origin --delete `). For automatic cleanup of refs left behind by replays that never came, see [Recovery Ref Cleanup](#recovery-ref-cleanup) below. ### Recovery Ref Cleanup diff --git a/orchestrator/agent_salvage.py b/orchestrator/agent_salvage.py index cf2cadca21..43d0da75f2 100644 --- a/orchestrator/agent_salvage.py +++ b/orchestrator/agent_salvage.py @@ -215,9 +215,22 @@ def _run_git( ``core.hooksPath=/dev/null`` mirrors ``StateStore._run_git`` — the orchestrator runs git on agent-controlled worktrees and must never - execute their hooks. + execute their hooks. ``commit.gpgsign=false`` keeps + :func:`commit_working_tree` working in a worktree that inherited + ``commit.gpgsign=true`` from the clone's config: there is no signing key + in the orchestrator image, so every salvage commit would otherwise fail + and lose the working tree it exists to save. """ - cmd = ["git", "-c", "core.hooksPath=/dev/null", "-C", str(cwd), *args] + cmd = [ + "git", + "-c", + "core.hooksPath=/dev/null", + "-c", + "commit.gpgsign=false", + "-C", + str(cwd), + *args, + ] return subprocess.run( cmd, capture_output=True, diff --git a/orchestrator/kubernetes_spawner/_worktree.py b/orchestrator/kubernetes_spawner/_worktree.py index 5f023d73d0..5a942ee9a4 100644 --- a/orchestrator/kubernetes_spawner/_worktree.py +++ b/orchestrator/kubernetes_spawner/_worktree.py @@ -10,6 +10,7 @@ from typing import Any import kubernetes_spawner as _pkg +from agent_salvage import _SALVAGE_COMMIT_EMAIL, _SALVAGE_COMMIT_NAME from kubernetes_spawner import ( logger, ) @@ -426,6 +427,12 @@ def _git(repo_dir: Path, *args: str, timeout: int = 30, check: bool = True): "core.hooksPath=/dev/null", "-c", "safe.directory=*", + # A worktree that inherits ``commit.gpgsign=true`` from the + # clone's config would fail every #3639 snapshot commit (no + # signing key in the orchestrator image), losing exactly the + # work the snapshot exists to save. + "-c", + "commit.gpgsign=false", *args, ], capture_output=True, @@ -450,12 +457,18 @@ def _git(repo_dir: Path, *args: str, timeout: int = 30, check: bool = True): # BEFORE the discard below erases the evidence: dirt is the # killed-mid-event signature that disqualifies the fast-forward # keep in the sync step (#3506). Unknown state counts as dirty. + # ``state_unknown`` distinguishes "status failed, assume dirty" from + # "status reported zero entries": without it the downstream WARNINGs + # report ``discarded_dirty_entries=0``, which reads as "nothing was + # there" on the one path where we genuinely do not know. + state_unknown = False try: dirty_entries = _git(d, "status", "--porcelain").stdout.strip().splitlines() was_dirty = bool(dirty_entries) except Exception: dirty_entries = [] was_dirty = True + state_unknown = True # #3639: snapshot the dirty tree into a commit BEFORE the reset. # Uncommitted work is otherwise the one class of agent output no @@ -475,6 +488,7 @@ def _git(repo_dir: Path, *args: str, timeout: int = 30, check: bool = True): agent_worktree_id=agent_worktree_id, repo=n, n_entries=len(dirty_entries), + state_unknown=state_unknown, ) else: # No branch ⇒ no origin tip to reset to and no salvage @@ -488,6 +502,7 @@ def _git(repo_dir: Path, *args: str, timeout: int = 30, check: bool = True): agent_worktree_id=agent_worktree_id, repo=n, discarded_dirty_entries=len(dirty_entries), + dirty_state_unknown=state_unknown, ) # reset --hard @@ -664,16 +679,15 @@ def _git(repo_dir: Path, *args: str, timeout: int = 30, check: bool = True): return True -# Identity + message for the synthetic commit that captures a re-attached -# worktree's dirty state. Mirrors the #2807 restart-path convention -# (``agent_salvage._SALVAGE_COMMIT_NAME`` / ``_SALVAGE_COMMIT_EMAIL`` / -# ``_UNCOMMITTED_SALVAGE_MESSAGE``) so one ``[salvage]`` grep finds every -# machine-made working-tree snapshot regardless of which path took it. The -# values are duplicated rather than imported: ``kubernetes_spawner.agent_salvage`` -# is a patched seam in the suite, and reading constants off a Mock would -# silently produce a garbage commit identity. -_WIP_COMMIT_AUTHOR_NAME = "egg-salvage" -_WIP_COMMIT_AUTHOR_EMAIL = "egg-salvage@localhost" +# Identity for the synthetic commit that captures a re-attached worktree's +# dirty state. Bound from the #2807 restart-path constants at import time so +# one ``[salvage]`` grep finds every machine-made working-tree snapshot +# regardless of which path took it, and so the two identities cannot drift. +# The module-level ``from`` import binds the real values and is unaffected by +# the suite's ``patch("kubernetes_spawner.agent_salvage")`` seam — that +# rebinds the package *attribute*, not what is already bound here. +_WIP_COMMIT_AUTHOR_NAME = _SALVAGE_COMMIT_NAME +_WIP_COMMIT_AUTHOR_EMAIL = _SALVAGE_COMMIT_EMAIL _WIP_COMMIT_MESSAGE = ( "[salvage] pre-reset working-tree state (#3639)\n" "\n" @@ -691,6 +705,7 @@ def _preserve_dirty_tree( agent_worktree_id: str, repo: str, n_entries: int, + state_unknown: bool = False, ) -> str | None: """Commit a re-attached worktree's dirty state before the R6 reset (#3639). @@ -705,12 +720,22 @@ def _preserve_dirty_tree( ``egg/recovered/...`` and records on the message bus. Returns the snapshot commit's SHA, or ``None`` when nothing was - preserved (an ignored-files-only tree, or a failed add/commit). + preserved (a tree with no committable change, or a failed commit). Strictly best-effort: every failure logs at WARNING and returns ``None`` so the caller proceeds with the reset. Blocking reuse instead would only send the spawn down the create-with-retry path, which discards the same state with less visibility. + A failing ``git add -A`` is *not* treated as fatal. Per ``git-add(1)`` + the default behaviour on an unindexable entry (unreadable file, fifo, + a filter that is not installed in the orchestrator image) is to abort + the whole add and exit non-zero, leaving a partially populated index — + so returning early there would discard the other N-1 files. The add + passes ``--ignore-errors`` to keep going past the bad entry, and on a + non-zero exit the helper still commits whatever reached the index: + partial preservation strictly beats none for a helper whose purpose is + "never lose the working tree". + Ignored files (``.gitignore``) are deliberately not captured: they are build output and caches, not agent work, and sweeping them in would make the recovery ref unpushably large. @@ -725,15 +750,27 @@ def _preserve_dirty_tree( carries the right config, so it is threaded in as a parameter. """ try: - git(repo_dir, "add", "-A", timeout=120) + try: + git(repo_dir, "add", "-A", "--ignore-errors", timeout=120) + except Exception as add_error: # partial index beats no index + logger.warning( + "Worktree re-attach: `git add -A` reported errors; committing " + "whatever reached the index", + agent_worktree_id=agent_worktree_id, + repo=repo, + dirty_entries=n_entries, + dirty_state_unknown=state_unknown, + error=str(add_error), + ) staged = git(repo_dir, "diff", "--cached", "--name-only", timeout=60).stdout.strip() if not staged: logger.warning( "Worktree re-attach: dirty tree held no committable change " - "(ignored files only); discarding it", + "(ignored files, or submodule-only dirt); discarding it", agent_worktree_id=agent_worktree_id, repo=repo, discarded_dirty_entries=n_entries, + dirty_state_unknown=state_unknown, ) return None git( @@ -749,13 +786,14 @@ def _preserve_dirty_tree( timeout=120, ) sha = git(repo_dir, "rev-parse", "HEAD").stdout.strip() - except Exception as e: # noqa: BLE001 # preservation must never block the reset + except Exception as e: # preservation must never block the reset logger.warning( "Worktree re-attach: could not preserve uncommitted work; " "the hard reset below WILL discard it", agent_worktree_id=agent_worktree_id, repo=repo, discarded_dirty_entries=n_entries, + dirty_state_unknown=state_unknown, error=str(e), ) return None @@ -804,12 +842,32 @@ def _record_discarded_tip( resuming agent must read such a commit as a mechanical checkpoint to inspect rather than as reviewed work it already proposed. + The reassurance that nothing was lost is conditional on + ``recovery_ref``: when the salvage push failed, the snapshot exists + only in the local object store and the message must say so and ask for + escalation. Telling a memory-less agent "nothing was lost" on the one + path where the work is a ``gc`` away from gone would suppress exactly + the escalation this record exists to trigger. + Best-effort: a record failure is logged and swallowed; it must not block the re-attach path. """ from message_store import Message, MessageType, get_message_store - if recovery_ref: + # A discard whose only casualty is the machine-made snapshot is not the + # same event as losing a stack of the agent's own commits: the imperative + # "inspect it before starting work" is what turns #3509's message into + # background noise when every respawn with a stray memory file triggers + # it. Soften the ask without suppressing the record. + snapshot_only = bool(wip_commit) and wip_commit == discarded_tip and n_commits == 1 + + if recovery_ref and snapshot_only: + recovery_text = ( + f"The snapshot is preserved on remote ref {recovery_ref}; if any of " + f"that work is missing, run `git fetch origin {recovery_ref}` and " + "inspect it before re-deriving it." + ) + elif recovery_ref: recovery_text = ( f"The full commit stack is preserved on remote ref {recovery_ref}; " f"run `git fetch origin {recovery_ref}` and inspect it before " @@ -817,24 +875,43 @@ def _record_discarded_tip( "(cherry-pick or reset) instead of re-deriving it." ) else: + # NOT salvage_agent_commits: it enumerates worktree branches, which + # the reset below has already moved off the discarded tip + # (``salvage_discarded_tip``'s own docstring says so), so it provably + # cannot see this sha. The worktree's HEAD reflog can. recovery_text = ( f"Automatic salvage FAILED ({salvage_error or 'unknown error'}); 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." + "commits survive only in this worktree's local git object store " + "until gc, unreachable from any ref. salvage_agent_commits cannot " + "recover them (it inspects worktree branches the reset has already " + f"moved, #3509) — ask an operator to fetch {discarded_tip} directly " + "out of the worktree (`git reflog`) before re-deriving any work." ) - wip_text = ( - ( + 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). Nothing was " - "lost, but treat it as a WIP checkpoint to review, not as work you " - "already proposed." + "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." + ) + 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 to an " + "operator before re-deriving any work." + ) + else: + wip_text = "" + count_text = f"{n_commits} unpushed commit(s)" + if wip_commit: + count_text += ( + " (one of which is an automatic snapshot of uncommitted work)" + if n_commits > 1 + else " (an automatic snapshot of uncommitted work)" ) - if wip_commit - else "" - ) body = ( - f"Worktree re-attach discarded {n_commits} unpushed commit(s) from " + f"Worktree re-attach discarded {count_text} from " f"{repo} (worktree {agent_worktree_id}). Your previous tip was " f"{discarded_tip}; the worktree was reset to {remote_tip}" + (f" (origin/{branch})." if branch else ".") diff --git a/orchestrator/tests/test_kubernetes_spawner.py b/orchestrator/tests/test_kubernetes_spawner.py index 4dbade1ac2..47c178508b 100644 --- a/orchestrator/tests/test_kubernetes_spawner.py +++ b/orchestrator/tests/test_kubernetes_spawner.py @@ -17,6 +17,7 @@ from dataclasses import dataclass from datetime import UTC, datetime from pathlib import Path +from types import SimpleNamespace from unittest.mock import MagicMock, patch import pytest @@ -3841,6 +3842,11 @@ def _push(**kwargs): assert msg.metadata["wip_commit"] == wip assert expected_ref in msg.body assert wip in msg.body + # A real commit was lost alongside the snapshot, so the message keeps + # the imperative ask and names the snapshot within the count. + assert "one of which is an automatic snapshot" in msg.body + assert "inspect it before starting work" in msg.body + assert "nothing was lost" in msg.body def test_salvage_failure_still_resets_and_records_tip(self, spawner, mock_gateway, tmp_path): repo, origin_head, orphan_head = self._seed_orphan(tmp_path) @@ -3860,6 +3866,15 @@ def test_salvage_failure_still_resets_and_records_tip(self, spawner, mock_gatewa assert _git(repo, "rev-parse", f"{discarded}^").stdout.strip() == orphan_head assert msg.metadata["recovery_ref"] is None assert "gateway down" in msg.metadata["salvage_error"] + # #3639: the body must NOT reassure a memory-less agent that nothing + # was lost on the one path where the snapshot was never pushed — that + # would suppress the escalation the preceding sentence just asked for. + assert "Nothing was lost" not in msg.body + assert "nothing was lost" not in msg.body + assert "was NOT" in msg.body and "local object store" in msg.body + assert "Escalate" in msg.body + # And it must not point at a tool that provably cannot see the sha. + assert "salvage_agent_commits cannot recover them" in msg.body def test_record_failure_does_not_block_reuse(self, spawner, mock_gateway, tmp_path): repo, origin_head, _ = self._seed_orphan(tmp_path) @@ -4079,6 +4094,70 @@ def test_uncommitted_work_is_salvaged_not_destroyed(self, spawner, mock_gateway, # The resuming agent is told the top commit is a machine snapshot. assert wip in msg.body assert "AUTOMATIC snapshot" in msg.body + # Snapshot-only discard: the ask is "if any of that work is missing", + # not the imperative reserved for losing the agent's own commits — a + # respawn with one stray memory file must not read like data loss. + assert "if any of that work is missing" in msg.body + assert "inspect it before starting work" not in msg.body + + def test_snapshot_commit_identity_matches_the_restart_path( + self, spawner, mock_gateway, tmp_path + ): + """The snapshot is greppable by ``[salvage]`` + the #2807 identity. + + ``docs/reference/agent-recovery.md`` promises one ``[salvage]`` grep + finds every machine-made working-tree snapshot regardless of which + path took it. Pin both halves: the constants agree with + ``agent_salvage``'s, and the commit git actually produces carries + them. + """ + import agent_salvage + from kubernetes_spawner import _worktree + + assert _worktree._WIP_COMMIT_AUTHOR_NAME == agent_salvage._SALVAGE_COMMIT_NAME + assert _worktree._WIP_COMMIT_AUTHOR_EMAIL == agent_salvage._SALVAGE_COMMIT_EMAIL + assert _worktree._WIP_COMMIT_MESSAGE.startswith("[salvage]") + + repo, _ = self._seed_dirty(tmp_path) + mock_gateway.push_worktree_branch.return_value = _FakePushResult(ok=True) + + with ( + patch("kubernetes_spawner.WORKTREE_BASE_DIR", tmp_path), + patch("message_store.get_message_store") as get_store, + ): + assert spawner._clean_reused_worktree(_WT_ID, _BRANCH, _REPOS, **_PIPE_CTX) is True + + wip = get_store.return_value.add_message.call_args.args[0].metadata["wip_commit"] + ident = _git(repo, "show", "-s", "--format=%an|%ae|%s", wip).stdout.strip() + author, email, subject = ident.split("|") + assert author == agent_salvage._SALVAGE_COMMIT_NAME + assert email == agent_salvage._SALVAGE_COMMIT_EMAIL + assert subject.startswith("[salvage]") + + def test_no_branch_takes_no_snapshot(self, spawner, mock_gateway, tmp_path): + """``branch is None`` ⇒ no snapshot commit, and HEAD does not move. + + With no branch there is no origin tip to reset to and no salvage + target, so a snapshot would simply become the successor's HEAD: + un-vetted residue promoted to committed state, which is what R6 + exists to prevent. The dirt is discarded as it was pre-#3639. + """ + repo, _origin_head = self._seed_dirty(tmp_path) + head_before = _git(repo, "rev-parse", "HEAD").stdout.strip() + + with ( + patch("kubernetes_spawner.WORKTREE_BASE_DIR", tmp_path), + patch("message_store.get_message_store") as get_store, + ): + cleaned = spawner._clean_reused_worktree(_WT_ID, None, _REPOS, **_PIPE_CTX) + + assert cleaned is True + # No snapshot was committed: HEAD is unmoved and the tree is clean. + assert _git(repo, "rev-parse", "HEAD").stdout.strip() == head_before + assert _git(repo, "status", "--porcelain").stdout.strip() == "" + assert not (repo / "new_module.py").exists() + mock_gateway.push_worktree_branch.assert_not_called() + get_store.return_value.add_message.assert_not_called() def test_snapshot_is_pushed_to_a_recovery_ref(self, spawner, mock_gateway, tmp_path): """The snapshot rides the existing #3509 recovery-ref namespace.""" @@ -4103,8 +4182,68 @@ def _push(**kwargs): assert kwargs["branch"] == (f"egg/recovered/pipe-1/slice-4-coder/{pushed_head['sha'][:12]}") assert kwargs["ref"] is None + def test_no_committable_change_takes_no_snapshot(self, tmp_path): + """The ``not staged`` branch: an empty index ⇒ no commit at all. + + Reached in production by dirt that ``git add -A`` cannot stage — a + dirty submodule whose gitlink is unchanged shows as ` M sub` in + ``status --porcelain`` yet stages nothing. Driven here with a git + closure whose ``diff --cached`` is empty, because the end-to-end + ignored-files case never reaches this helper at all (ignored files do + not appear in ``status --porcelain``, so ``was_dirty`` is False). + """ + from kubernetes_spawner._worktree import _preserve_dirty_tree + + calls = [] + + def _fake_git(_repo_dir, *args, **_kwargs): + calls.append(args) + return SimpleNamespace(stdout="", returncode=0) + + assert ( + _preserve_dirty_tree( + _fake_git, tmp_path, agent_worktree_id=_WT_ID, repo="repo", n_entries=1 + ) + is None + ) + assert not any("commit" in a for a in calls) + + def test_partial_add_failure_still_commits_what_was_staged(self, tmp_path): + """A non-zero ``git add`` must not discard the files that did stage. + + ``git-add(1)`` aborts on the first unindexable entry and exits + non-zero with a partially populated index; returning early there + would throw away the other N-1 files this helper exists to save. + """ + from kubernetes_spawner._worktree import _preserve_dirty_tree + + seen = [] + + def _flaky_git(_repo_dir, *args, **_kwargs): + seen.append(args) + if args[0] == "add": + assert "--ignore-errors" in args + raise RuntimeError("error: unable to index file 'broken.sock'") + if args[0] == "diff": + return SimpleNamespace(stdout="a.py\nb.py\n", returncode=0) + return SimpleNamespace(stdout="deadbeefcafe\n", returncode=0) + + assert ( + _preserve_dirty_tree( + _flaky_git, tmp_path, agent_worktree_id=_WT_ID, repo="repo", n_entries=3 + ) + == "deadbeefcafe" + ) + assert any("commit" in a for a in seen) + def test_ignored_only_dirt_is_discarded_without_a_commit(self, spawner, mock_gateway, tmp_path): - """Build output is not agent work: no snapshot, nothing salvaged.""" + """Build output is not agent work: no snapshot, nothing salvaged. + + Ignored files are absent from ``status --porcelain``, so this asserts + the outer ``if was_dirty:`` guard never fires — the helper is not + reached. The helper's own empty-index branch is pinned by + :meth:`test_no_committable_change_takes_no_snapshot`. + """ repo, _ = _make_worktree(tmp_path, _WT_ID, "repo", _BRANCH, with_origin=True) (repo / ".gitignore").write_text("build/\n") _git(repo, "add", "-A")