diff --git a/docs/architecture/on-demand-agent-lifecycle.md b/docs/architecture/on-demand-agent-lifecycle.md index 3c6040e70..cb6c9cdbf 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 5afbe0c97..aa931ead4 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 cf2cadca2..43d0da75f 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 5f023d73d..5a942ee9a 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 4dbade1ac..47c178508 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")