diff --git a/docs/architecture/on-demand-agent-lifecycle.md b/docs/architecture/on-demand-agent-lifecycle.md index 09905ccce..b084ed5b2 100644 --- a/docs/architecture/on-demand-agent-lifecycle.md +++ b/docs/architecture/on-demand-agent-lifecycle.md @@ -183,7 +183,76 @@ Under orchestrator ownership the worktree becomes a hot path. resuming agent with no session memory can find and resume its prior work instead of silently re-deriving it (falls back to log-only when `pipeline_id` context is unavailable, or record-only when the salvage - push itself fails). + push itself fails). **Uncommitted work is snapshotted first** + ([#3639](https://github.com/jwbron/egg/issues/3639)): a dirty tree is + committed (`git add -A` plus a `[salvage] pre-reset working-tree state` + commit) *before* the `reset --hard`, so it becomes an ordinary orphan + that the salvage + record path above recovers. Without that step a + session that worked for hours without committing had nothing for the + orphan detector to find and lost everything on a routine respawn. The + snapshot does not relax the residue policy: the tree still hard-resets + 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. 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. The ask + also scales with *what* the snapshot captured: when every captured path + is a **state file the next event regenerates and some other store + durably holds**, the record softens to "read it if you need it" so a + routine respawn does not train the #3509 message into background noise. + The membership test is regeneration, not authorship — the dominant + member is written by the *sandbox* on the agent's own tool call and + holds agent-authored prose. The allowlist is + `.egg-state/agent-outputs/*/brc-memory*.md` (rewritten by + `sandbox/egg_agent_tools/handlers/brc_memory.py` on every + `brc_ack`/`brc_nack`, with the orchestrator message history as the + durable backstop — see + [brc-memory.md](brc-memory.md)), + `.egg-state/agent-outputs/consensus-confirmed`, and + `.egg-state/agent-outputs/-apply-handoff.json`; + matching is segment-wise so `*` does not cross `/`. Agent *output* in + the same directory — `-wontdo.json`, + `-tester-output.json` — is deliberately excluded: nothing + rewrites it on the next event and no other store holds it, so losing it + warrants the imperative. Anything else — + including an unrecognised or unknown file set — keeps the imperative + "inspect it before starting work", as does a snapshot flagged partial + (a truncated capture's path list omits whatever failed to stage, so it + cannot establish that the snapshot holds nothing but state files). The + bus record's metadata carries the inputs *and* the outcome as separate + fields — `wip_paths` (capped), `wip_partial`, + `wip_machine_state_only` (the path predicate alone) and `wip_softened` + (whether the body actually softened) — so a triage consumer can + reconstruct the decision instead of regexing the prose; the two derived + fields diverge whenever a machine-state-only path set is disqualified + by a commit stack, a truncated capture, or a failed salvage push. The + threshold selects wording only; the snapshot itself is always taken — + including when the path list cannot be read at all. The staged-path + read uses `-z` so `wip_paths` carries real bytes rather than + `core.quotePath` C-quoted tokens, which means a filename that is not + valid UTF-8 would be undecodable under `subprocess`'s strict `text=True` + decode; the read passes `errors="replace"`, so one bad name costs one + name (a U+FFFD in `wip_paths`) rather than the whole path set. That + replacement does not move the softening decision in either direction: + every non-`*` character in the softening globs is ASCII and replacement + only substitutes non-ASCII for non-ASCII, so a replaced path matches + exactly the globs its raw bytes would. Anything that still defeats that read — a + timeout on a large staged set, a non-zero `diff` against a locked index + — logs a WARNING and commits blind (`wip_paths`/`wip_files` become + `null`, so the record takes the imperative) rather than letting a + metadata read cost the working tree. A + snapshot whose `git add -A` did not complete cleanly is marked incomplete in + both its commit message and the bus record, since a truncated snapshot + is otherwise indistinguishable downstream from a complete one. The same + marker rides the #2807 crash-salvage commit + (`commit_working_tree`), which pushes to `egg/recovered/…` with no bus + record at all — there the commit message is the only channel a triager + ever sees. 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 ec5577050..370a6c90a 100644 --- a/docs/reference/agent-recovery.md +++ b/docs/reference/agent-recovery.md @@ -305,6 +305,8 @@ Agent restart preserves the agent's git worktree, including any committed work o **Uncommitted work is also captured before respawn** ([#2807](https://github.com/jwbron/egg/issues/2807)): auto-salvage runs with `salvage_uncommitted=True`, which stages and commits the agent's dirty working tree (using identity `egg-salvage ` and commit message `[salvage] pre-crash working-tree state (#2807)`) before pushing to `egg/recovered/…`. By the time the gateway's subsequent `git reset --hard` runs during worktree reuse, that state has already been committed and pushed — so the reset only abandons the synthetic salvage commit locally, and it remains recoverable via the pushed `egg/recovered/…` ref. +The **event-loop respawn** path (worktree re-attach, not operator restart) gets the same protection from [#3639](https://github.com/jwbron/egg/issues/3639): `_clean_reused_worktree` commits a dirty tree as `[salvage] pre-reset working-tree state (#3639)` (same identity, so one `[salvage]` grep finds either snapshot) before its `reset --hard`, which feeds the existing [#3509](https://github.com/jwbron/egg/issues/3509) recovery-ref push and bus record. Until that landed, this path was the one gap in the preservation story: it salvaged commits only, so a session that worked for hours without committing lost the entire working tree on the next respawn, logged at INFO as `cleaned and synced`. + **Implementation detail:** `spawn_agent_container()` always calls the gateway to create (or reuse) the per-agent worktree when `repos` is provided, regardless of whether `repo_volumes` was passed by the caller. This ensures both the initial spawn path and the restart path (which does not pass `repo_volumes`) correctly mount the agent's worktree. See issue [#1597](https://github.com/jwbron/egg/issues/1597) for the fix that resolved a bug where the restart path skipped worktree creation. ## Phase-Level Restart @@ -360,7 +362,7 @@ When an agent's pushes to its assigned branch are wedged — gateway branch-allo | **API (read)** | `GET /api/v1/pipelines/{id}/local-commits[?agent_role=&slice_id=]` — list unpushed commits per worktree (read-only) | | **API (write)** | `POST /api/v1/pipelines/{id}/salvage[?agent_role=&slice_id=]` — push HEAD to `egg/recovered/...` | | **MCP tool** | `list_agent_local_commits(task_id, agent_role?, slice_id?)` and `salvage_agent_commits(task_id, agent_role?, slice_id?)` | -| **Auto-salvage** | Best-effort, automatic — runs from `kubernetes_spawner.cleanup_pipeline` (skipped when `preserve_worktrees=True`, since the worktree survives and there's nothing to mirror), from `restart_phase` (always runs against the worktrees of the roles being restarted), from **agent restart** (`restart_agent_job`, [#2807](https://github.com/jwbron/egg/issues/2807)) with `salvage_uncommitted=True` — which also commits the dirty working tree onto the work branch before the recovery push, so uncommitted edits survive the respawn's `git reset --hard` — and from **worktree re-attach** (`_clean_reused_worktree`'s dirty-discard reset, [#3509](https://github.com/jwbron/egg/issues/3509)): `salvage_discarded_tip` pushes the doomed HEAD to a recovery ref *before* the hard-reset runs (the tip is otherwise unreachable to every other salvage path once the reset moves the worktree branch), and a message-bus system message durably records the discarded tip + recovery ref so a resuming agent with no session memory can find its prior work. Requires `pipeline_id` (plus `agent_role`/`slice_id` for ref scoping) and the pipeline's real gateway `mode` — omitting `mode` risks a "public" push being denied on a private-mode pipeline, silently degrading to record-only. Failures are logged and never block cleanup or restart | +| **Auto-salvage** | Best-effort, automatic — runs from `kubernetes_spawner.cleanup_pipeline` (skipped when `preserve_worktrees=True`, since the worktree survives and there's nothing to mirror), from `restart_phase` (always runs against the worktrees of the roles being restarted), from **agent restart** (`restart_agent_job`, [#2807](https://github.com/jwbron/egg/issues/2807)) with `salvage_uncommitted=True` — which also commits the dirty working tree onto the work branch before the recovery push, so uncommitted edits survive the respawn's `git reset --hard` — and from **worktree re-attach** (`_clean_reused_worktree`'s dirty-discard reset, [#3509](https://github.com/jwbron/egg/issues/3509)): a dirty tree is first committed as a `[salvage] pre-reset working-tree state (#3639)` snapshot so uncommitted work is salvageable at all ([#3639](https://github.com/jwbron/egg/issues/3639)), then `salvage_discarded_tip` pushes the doomed HEAD to a recovery ref *before* the hard-reset runs (the tip is otherwise unreachable to every other salvage path once the reset moves the worktree branch), and a message-bus system message durably records the discarded tip + recovery ref so a resuming agent with no session memory can find its prior work. Requires `pipeline_id` (plus `agent_role`/`slice_id` for ref scoping) and the pipeline's real gateway `mode` — omitting `mode` risks a "public" push being denied on a private-mode pipeline, silently degrading to record-only. Failures are logged and never block cleanup or restart | ### Recovery Workflow @@ -378,6 +380,17 @@ 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 may also be incomplete**: when `git add -A` did not complete cleanly the commit holds only what reached the index, and its message carries an ``INCOMPLETE: `git add -A` did not complete cleanly while staging`` paragraph saying so — `git log -1 ` before you conclude the ref holds everything the working tree did. On the [#3639](https://github.com/jwbron/egg/issues/3639) re-attach path a bus record repeats that warning, but on the [#2807](https://github.com/jwbron/egg/issues/2807) crash-salvage path (`commit_working_tree`) there is no bus record at all, so the commit message is the only channel a triager gets. To find truncated snapshots from either path, fetch the recovery namespace first — both paths push to `egg/recovered/*` on **origin**, so a fresh clone has no local ref for them and `git log --all` alone would report zero: + +```bash +git fetch origin 'refs/heads/egg/recovered/*:refs/remotes/origin/egg/recovered/*' +git log --all --grep 'INCOMPLETE: `git add -A`' +``` + +The grep token — the leading `INCOMPLETE:` plus the backticked `git add -A` — is byte-identical in both paths' suffixes (`_WIP_COMMIT_PARTIAL_SUFFIX`, `_UNCOMMITTED_SALVAGE_PARTIAL_SUFFIX`) and is pinned against this file by `test_partial_suffixes_share_one_grep_token`. Keep the backticks: they are part of the commit message, so a pattern without them matches nothing and the zero results read as "no truncated snapshots". + +Reading the diff before replaying is the control here: **nothing in this repo enables GitHub push protection**, so do not assume a snapshot containing a secret is stopped on the way out. (Where push protection *is* enabled on the receiving repo it rejects such a push rather than leaking it, and the discard is then recorded with `salvage_error` set instead of a recovery ref — the failure branch described above.) + 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..c38726b02 100644 --- a/orchestrator/agent_salvage.py +++ b/orchestrator/agent_salvage.py @@ -85,6 +85,36 @@ # without a configured user, so an explicit identity is required for the # commit to succeed. _UNCOMMITTED_SALVAGE_MESSAGE = "[salvage] pre-crash working-tree state (#2807)" +# Appended when ``git add -A`` reported errors, mirroring the re-attach path's +# ``kubernetes_spawner._worktree._WIP_COMMIT_PARTIAL_SUFFIX`` (#3639). A +# truncated snapshot is otherwise indistinguishable downstream from a complete +# one — same subject, same ``egg/recovered/...`` ref. This path is the worse of +# the two: unlike the re-attach path it writes no message-bus record, so the +# commit message is the only channel anyone triaging that recovery ref ever +# sees. +# +# The near-duplication is deliberate. The two texts differ only in naming whose +# working tree was truncated ("crashed agent's" here, "previous session's" +# there), which is the one thing a triager reading a lone commit message cannot +# infer. The grep token — the leading ``INCOMPLETE:`` and the ``git add -A`` +# phrase — is identical in both, so one search finds every truncated snapshot +# regardless of which path took it, and ``docs/reference/agent-recovery.md`` +# quotes it verbatim for triagers. Change one, change the other — and the +# runbook. +# +# "did not complete cleanly" rather than "reported errors" (#3639 re-review +# NB-6): the shared wording has to hold on the re-attach path too, where a +# ``TimeoutExpired`` sets ``partial`` without git ever reporting an exit +# status. Here the add is run with ``check=False`` and ``partial`` really is +# ``returncode != 0``, but a claim the commit message cannot make on both +# paths is not one worth keeping on either. +_UNCOMMITTED_SALVAGE_PARTIAL_SUFFIX = ( + "\n" + "\n" + "INCOMPLETE: `git add -A` did not complete cleanly while staging, so\n" + "files present in the crashed agent's working tree may be missing\n" + "from this commit." +) _SALVAGE_COMMIT_NAME = "egg-salvage" _SALVAGE_COMMIT_EMAIL = "egg-salvage@localhost" @@ -215,13 +245,48 @@ 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. + + ``core.quotePath=true`` is pinned rather than inherited (#3639 re-review + NB-3) and ``errors="replace"`` is passed to the decode. The two are + complementary, not redundant: the pin keeps *most* of git's output ASCII + for the ``commit`` / ``status`` / ``diff`` calls here, and costs nothing + because this path has no ``-z`` read to make quoting a problem. It does + **not** cover ``git add``, which echoes the raw path in at least two of + its stderr messages regardless of ``quotePath`` (#3639 re-review B1):: + + error: unable to index file 'caf\\xe9.txt' + error: 'nested-caf\\xe9/' does not have a commit checked out + + Under a strict decode those bytes raise ``UnicodeDecodeError`` from + inside :func:`subprocess.run` — before this function returns — which + :func:`commit_working_tree` would swallow into "continuing", losing the + whole working tree over one filename. That is #3639 itself. The + non-strict decode is why it cannot happen; since no call here reads + ``-z`` output, replacement can only touch names that would otherwise + crash. """ - cmd = ["git", "-c", "core.hooksPath=/dev/null", "-C", str(cwd), *args] + cmd = [ + "git", + "-c", + "core.hooksPath=/dev/null", + "-c", + "commit.gpgsign=false", + "-c", + "core.quotePath=true", + "-C", + str(cwd), + *args, + ] return subprocess.run( cmd, capture_output=True, text=True, + errors="replace", check=check, timeout=timeout, ) @@ -598,6 +663,12 @@ def commit_working_tree(worktree: AgentWorktree) -> str | None: state. Staging and committing it onto the local work branch *before* salvage lets the recovery-ref push capture it. + A commit whose ``git add -A`` reported errors carries + ``_UNCOMMITTED_SALVAGE_PARTIAL_SUFFIX``: this path pushes to + ``egg/recovered/...`` for manual triage but records nothing on the + message bus, so the commit message is the only place a human or agent + reading that ref can learn the snapshot is truncated. + Best-effort: returns the new commit SHA on success, ``None`` when there is nothing to commit or the commit fails. Never raises — a failure here must not stop the committed-but-unpushed salvage that @@ -608,13 +679,35 @@ def commit_working_tree(worktree: AgentWorktree) -> str | None: if not _has_uncommitted_changes(worktree.repo_path): return None try: - add = _run_git("add", "-A", cwd=worktree.repo_path, check=False) - if add.returncode != 0: + add = _run_git("add", "-A", "--ignore-errors", cwd=worktree.repo_path, check=False) + partial = add.returncode != 0 + if partial: + # Not fatal, same as the re-attach path's snapshot (#3639): per + # ``git-add(1)`` an unindexable entry (unreadable file, fifo, a + # filter that is not installed in the orchestrator image) aborts + # the add and exits non-zero with a partially populated index, and + # ``--ignore-errors`` still exits non-zero after skipping it. + # Returning here would discard the other N-1 files this helper + # exists to capture; commit whatever reached the index instead. logger.warning( - "Salvage: git add -A failed; skipping uncommitted capture", + "Salvage: git add -A reported errors; committing whatever reached the index", worktree_id=worktree.worktree_id, stderr=(add.stderr or "").strip(), ) + # Distinguish "the add put nothing in the index" from "the commit + # itself failed" before attempting it. Without this the operator sees + # `commit ... failed` with a "nothing added to commit" stderr and goes + # looking at the commit, when the cause was the add above — the same + # misattribution the re-attach path's empty-index guard exists to + # prevent (#3639 re-review). + staged = _run_git("diff", "--cached", "--name-only", cwd=worktree.repo_path, check=False) + if staged.returncode == 0 and not (staged.stdout or "").strip(): + logger.warning( + "Salvage: nothing staged to commit (ignored files, submodule-only " + "dirt, or a failed add); skipping the working-tree snapshot", + worktree_id=worktree.worktree_id, + add_failed=partial, + ) return None commit = _run_git( "-c", @@ -623,7 +716,7 @@ def commit_working_tree(worktree: AgentWorktree) -> str | None: f"user.email={_SALVAGE_COMMIT_EMAIL}", "commit", "-m", - _UNCOMMITTED_SALVAGE_MESSAGE, + _UNCOMMITTED_SALVAGE_MESSAGE + (_UNCOMMITTED_SALVAGE_PARTIAL_SUFFIX if partial else ""), cwd=worktree.repo_path, check=False, ) @@ -636,10 +729,31 @@ def commit_working_tree(worktree: AgentWorktree) -> str | None: return None head = _run_git("rev-parse", "HEAD", cwd=worktree.repo_path, check=False) head_sha = (head.stdout or "").strip() if head.returncode == 0 else None - except (OSError, subprocess.SubprocessError) as e: + # Deliberately broader than the ``(OSError, subprocess.SubprocessError)`` + # the read helpers above use, and broader than it needs to be today. The + # docstring promises this never raises, and the class that would break + # that promise is not a subprocess error: a git command that echoes a + # filename whose bytes are not valid UTF-8 raises ``UnicodeDecodeError`` + # (a ``ValueError``) from inside ``subprocess.run``, before ``_run_git`` + # returns. That is a live input class on this path — ``git add``'s + # stderr echoes the raw path in messages ``core.quotePath`` does not + # cover (#3639 re-review B1) — so ``_run_git`` decodes with + # ``errors="replace"`` and the raise cannot happen there. This handler is + # the second layer: catching it here rather than letting it escape keeps + # a future decode gap from aborting the committed-but-unpushed salvage + # that follows. Note what it costs when it *does* fire — the caller reads + # a WARNING about a hostile worktree, not about lost work — which is why + # the non-strict decode, not this handler, is the fix for the case above. + except Exception as e: logger.warning( "Salvage: capturing uncommitted working tree raised; continuing", worktree_id=worktree.worktree_id, + # The breadth above is the point, but it makes an + # ``AttributeError`` from a future refactor render identically to + # a subprocess failure. The class name is the one field that + # separates "the worktree was hostile" from "this code is broken" + # (#3639 re-review NB-5). + error_type=type(e).__name__, error=str(e), ) return None @@ -650,6 +764,7 @@ def commit_working_tree(worktree: AgentWorktree) -> str | None: worktree_id=worktree.worktree_id, agent_role=worktree.agent_role, head_sha=head_sha, + partial=partial, ) return head_sha diff --git a/orchestrator/kubernetes_spawner/_worktree.py b/orchestrator/kubernetes_spawner/_worktree.py index 9efe6264a..67740ecde 100644 --- a/orchestrator/kubernetes_spawner/_worktree.py +++ b/orchestrator/kubernetes_spawner/_worktree.py @@ -5,9 +5,13 @@ """ import os +from collections.abc import Callable +from fnmatch import fnmatchcase from pathlib import Path +from typing import Any, NamedTuple import kubernetes_spawner as _pkg +from agent_salvage import _SALVAGE_COMMIT_EMAIL, _SALVAGE_COMMIT_NAME from kubernetes_spawner import ( logger, ) @@ -392,12 +396,18 @@ def _clean_reused_worktree( policy, silently degrading auto-salvage to record-only — the exact silent-loss class this hook exists to prevent. - Only the committed tip (``HEAD``) is salvaged. The ``git reset - --hard`` + ``git clean -fd`` above run *before* orphan detection, so - uncommitted tracked edits and untracked files in a killed-mid-event - worktree are gone before salvage runs; the recovery ref is therefore - a commit snapshot, not a full worktree snapshot. Capturing dirty - working-tree state is #2807's domain, deliberately out of scope here. + Only the committed tip (``HEAD``) is salvaged, so a dirty tree with + no commits used to fall outside every preservation path: the ``git + reset --hard`` + ``git clean -fd`` above run *before* orphan + detection, and orphan detection then found nothing to save. That is + the #3639 loss (110 minutes across 33 modified files, discarded + silently on a routine respawn). The gap is closed one step earlier: + :func:`_preserve_dirty_tree` commits the dirty tree BEFORE the reset, + so the snapshot becomes an ordinary orphan and rides the salvage + + record path above. Preservation is best-effort and never blocks the + reset; when it cannot run (no ``branch``, or the commit fails) the + discard is logged at WARNING with the file count rather than the + pre-#3639 silence. Returns ``True`` on success, ``False`` on any failure (the caller falls back to create-with-retry — never allow a half-cleaned @@ -408,7 +418,13 @@ def _clean_reused_worktree( """ import subprocess as _sp - def _git(repo_dir: Path, *args: str, timeout: int = 30, check: bool = True): + def _git( + repo_dir: Path, + *args: str, + timeout: int = 30, + check: bool = True, + errors: str | None = None, + ): return _sp.run( [ "git", @@ -418,10 +434,35 @@ 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", + # Pinned, not inherited (#3639 re-review NB-3). Every command + # here that does not opt into ``errors="replace"`` runs under + # a strict UTF-8 decode, so any git output that echoes a + # filename verbatim raises ``UnicodeDecodeError`` from inside + # ``run`` the moment a path in this tree is not valid UTF-8. + # (The two calls that *do* opt in are the ones git leaves + # unquoted regardless of this setting: the ``-z`` staged read, + # and ``add``, whose stderr echoes raw paths.) The default + # ``core.quotePath=true`` C-quote-encodes those bytes to ASCII + # and is what keeps ``status --porcelain`` and ``clean -fd`` + # decodable; a worktree that inherited ``quotePath=false`` + # would break them — ``clean -fd`` failing *after* the snapshot + # commit but *before* the salvage push loses the commit + # entirely. ``-z`` reads are unaffected either way (git never + # quotes NUL-terminated output), so this costs nothing and + # turns an inherited default into an invariant. + "-c", + "core.quotePath=true", *args, ], capture_output=True, text=True, + errors=errors, timeout=timeout, check=check, ) @@ -442,10 +483,59 @@ 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: - was_dirty = bool(_git(d, "status", "--porcelain").stdout.strip()) + 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 + # preservation path can reach: ``reset --hard`` erases it and the + # orphan detector below then finds nothing to salvage. Committing + # here turns it into an ordinary orphan that the existing #3509 + # salvage + record path pushes to ``egg/recovered/...``. The commit + # does NOT change the residue policy: ``was_dirty`` is already + # latched above, so the tree still hard-resets to the origin tip + # and the successor never inherits a killed-mid-event working set. + wip_commit: str | None = None + wip_files: int | None = None + wip_paths: tuple[str, ...] | None = None + wip_partial = False + if was_dirty: + if branch: + snapshot = _preserve_dirty_tree( + _git, + d, + agent_worktree_id=agent_worktree_id, + repo=n, + n_entries=len(dirty_entries), + state_unknown=state_unknown, + ) + if snapshot is not None: + wip_commit, wip_files = snapshot.sha, snapshot.n_files + wip_paths, wip_partial = snapshot.paths, snapshot.partial + else: + # No branch ⇒ no origin tip to reset to and no salvage + # target, so a snapshot commit would just become the + # successor's HEAD: un-vetted residue promoted to committed + # state, which is exactly what R6 exists to prevent. Discard + # as before, but say so. + logger.warning( + "Worktree re-attach: discarding uncommitted work " + "(no branch to sync or salvage against)", + agent_worktree_id=agent_worktree_id, + repo=n, + dirty_entries=len(dirty_entries), + dirty_state_unknown=state_unknown, + ) # reset --hard try: @@ -579,6 +669,25 @@ def _git(repo_dir: Path, *args: str, timeout: int = 30, check: bool = True): discarded_commits=orphans[:20], recovery_ref=recovery_ref, salvage_error=salvage_error, + wip_commit=wip_commit, + # Completeness rides with the sha so "which recovery + # refs are truncated" is one query over this WARNING, + # not a join back to the earlier _preserve_dirty_tree + # line by worktree id. Named ``wip_*`` to match the bus + # record's metadata keys exactly (#3639 re-review + # NB-1): one query shape has to work over both, and a + # log field called ``preserved_files`` next to a + # metadata field called ``wip_files`` guarantees it + # does not. + wip_partial=wip_partial, + wip_files=wip_files, + # ``wip_files`` is ``None`` when the staged-path read + # failed — the exact path this WARNING exists to make + # queryable. A bare ``None`` renders as ``wip_files=``, + # which reads as *zero files preserved* rather than + # *unknown*, so the distinction gets its own boolean + # (the same reason ``dirty_state_unknown`` exists). + wip_files_unknown=(wip_commit is not None and wip_files is None), ) if pipeline_id: _record_discarded_tip( @@ -594,6 +703,10 @@ def _git(repo_dir: Path, *args: str, timeout: int = 30, check: bool = True): was_dirty=was_dirty, recovery_ref=recovery_ref, salvage_error=salvage_error, + wip_commit=wip_commit, + wip_files=wip_files, + wip_paths=wip_paths, + wip_partial=wip_partial, ) try: _git(d, "reset", "--hard", f"origin/{branch}") @@ -619,6 +732,409 @@ def _git(repo_dir: Path, *args: str, timeout: int = 30, check: bool = True): return True +# 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. + +# Paths whose loss in a discard costs nothing durable: each is rebuilt by the +# next event and has a durable record elsewhere, so a respawn that trips this +# path over nothing else is routine and :func:`_record_discarded_tip` may +# soften its ask. Matching the noise source *by name* rather than by a file +# count is deliberate: a count threshold scores one rewritten 400-line module +# identically to one stray memory file, and telling the agent that ref "is as +# likely to be a leftover state file as work" is the one framing that could +# talk it out of fetching a real #3639 loss. Anything not on this list — +# including an unknown file set — takes the imperative. +# +# Membership test is "mechanically regenerated on the next event, with a +# durable backstop elsewhere", not "written by orchestrator code" and not +# "looks mechanical". The narrower "orchestrator-written" rule would exclude +# the dominant member below, whose *writer* is in the sandbox and whose +# content is agent-authored prose: +# * ``/brc-memory-.md`` — the dominant case. Written by +# ``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. It qualifies because the next ack/nack +# rewrites it and ``docs/architecture/brc-memory.md`` names the durable +# backstop: the orchestrator message history, rehydrated by +# ``reconstruct_tracker_from_messages``. +# * ``consensus-confirmed`` — ``routes/signals/_consensus_confirm`` writes the +# marker the gateway reads back (``gateway/session_manager``); the +# consensus state it mirrors lives in the tracker. +# * ``-apply-handoff.json`` — ``routes/pipelines/_ledger`` +# writes the applier's *input* handoff just before APPLY spawns, and +# rewrites it on the next spawn from the ledger it was derived from. +# Deliberately NOT listed, though they sit in the same directory and look +# alike: ``-wontdo.json`` and ``-tester-output.json`` are +# agent *output* with no regeneration path — nothing rewrites them on the next +# event and no other store holds them — so a discard that loses them is a real +# loss, worth the imperative. That is the line to test a new entry against. +_MACHINE_STATE_FILE_GLOBS = ( + ".egg-state/agent-outputs/*/brc-memory*.md", + ".egg-state/agent-outputs/consensus-confirmed", + ".egg-state/agent-outputs/*-apply-handoff.json", +) + +# Above this many paths the soft branch states a count instead of naming each +# one: a bus body is read by an agent with a context budget, and a per-role +# memory file for a wide roster would otherwise inline a dozen paths to say +# "nothing here". ``wip_paths`` rides in the metadata either way. +_SOFT_BRANCH_MAX_NAMED_PATHS = 4 + +# Cap on the ``wip_paths`` list carried in the bus record's metadata. The +# #3639 incident was 33 files; a cap in that neighbourhood keeps the whole +# path set for realistic discards while bounding a pathological one. +_METADATA_MAX_PATHS = 50 + +_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" + "Snapshot taken by the orchestrator's worktree re-attach before the R6\n" + "dirty-state reset, which would otherwise discard it. This is a\n" + "mechanical checkpoint of a previous session's working tree, not\n" + "reviewed work." +) +# Appended when ``git add -A`` did not complete cleanly. A truncated snapshot +# is otherwise indistinguishable downstream from a complete one — same subject, +# same ``egg/recovered/...`` ref — and the only other record is an +# orchestrator WARNING nobody who cherry-picks this commit will read. +# +# "did not complete cleanly" rather than "reported errors" (#3639 re-review +# NB-6): 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. The neutral phrasing covers both without +# losing the fact that staging is where the truncation happened. +# +# Deliberately a near-duplicate of the #2807 restart path's +# ``agent_salvage._UNCOMMITTED_SALVAGE_PARTIAL_SUFFIX`` rather than a shared +# constant: the two differ only in naming whose working tree was truncated +# ("previous session's" here, "crashed agent's" there), and that provenance is +# the one thing a triager reading a lone commit message cannot infer. The +# grep token — the leading ``INCOMPLETE:`` and the ``git add -A`` phrase — is +# identical in both, so one search still finds every truncated snapshot +# regardless of which path took it, and +# ``docs/reference/agent-recovery.md`` quotes it verbatim for triagers. +# Change one, change the other — and the runbook. +_WIP_COMMIT_PARTIAL_SUFFIX = ( + "\n" + "\n" + "INCOMPLETE: `git add -A` did not complete cleanly while staging, so\n" + "files present in the previous session's working tree may be missing\n" + "from this commit." +) + + +class _DirtySnapshot(NamedTuple): + """The commit :func:`_preserve_dirty_tree` made, and what is in it. + + ``paths`` and ``n_files`` are carried alongside the sha because they + are the only signal that separates the #3639 incident (110 minutes, 33 + files) from a respawn whose worktree held one stray + ``brc-memory-*.md``. The message a resuming agent reads is worded off + them — see :func:`_record_discarded_tip`. + + ``partial`` is set when ``git add -A`` did not complete cleanly, so + the commit may be missing files the previous session's tree held. It + rides all the way to the bus message: a snapshot that is silently + truncated is a worse failure than one the agent is told is truncated, + and the orchestrator WARNING that records it is not a surface the + resuming agent reads. + + ``paths``/``n_files`` are ``None`` when the staged-path list could not + be read at all — the ``diff --cached`` timed out, exited non-zero, or + raised (see :func:`_preserve_dirty_tree`). + That degrades the *wording* to the imperative, never the snapshot: the + commit is taken either way, because a path list is a nicety and the + working tree is the thing #3639 exists to save. + """ + + sha: str + n_files: int | None + paths: tuple[str, ...] | None + partial: bool = False + + +def _preserve_dirty_tree( + git: Callable[..., Any], + repo_dir: Path, + *, + agent_worktree_id: str, + repo: str, + n_entries: int, + state_unknown: bool = False, +) -> _DirtySnapshot | None: + """Commit a re-attached worktree's dirty state before the R6 reset (#3639). + + ``_clean_reused_worktree``'s ``git reset --hard`` + ``git clean -fd`` + are the only step in the re-attach path that no preservation hook can + see behind: #3509's auto-salvage runs after them and operates on + commits, so a session that worked for hours without committing had its + entire output erased on the next respawn, silently. Committing the + tree here (tracked edits *and* non-ignored untracked files, via ``git + add -A``) converts that state into a commit ahead of the origin tip, + the exact shape the orphan detector already salvages to + ``egg/recovered/...`` and records on the message bus. + + Returns a :class:`_DirtySnapshot` (the commit's SHA, the files it + captured, and whether the capture was partial), or ``None`` when + nothing was preserved (a tree with no committable change, or a failed + commit). Nothing short of "no commit exists" returns ``None``: a + staged-path list that cannot be read — *for any reason*, decode + failure or timeout or a non-zero ``diff`` — degrades the snapshot's + *metadata* (``paths``/``n_files`` become ``None``) and never its + existence. Undecodable bytes cost less than that: they are replaced + per-path, so a single latin-1 filename leaves the other N-1 names + intact. + 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". Such a snapshot is marked ``partial`` + and says so in both its commit message and the bus record, so a + resuming agent that cherry-picks it knows files may be missing. + + 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. + + This does not call ``agent_salvage.commit_working_tree`` (#2807's + equivalent for the restart path) even though the two are otherwise the + same operation: that helper's ``_run_git`` omits ``safe.directory=*``, + which the re-attach path needs because the orchestrator's uid differs + from the host uid owning the worktree. Reusing it would fail on + "dubious ownership" exactly in production and silently degrade back to + the discard this exists to prevent. The caller's ``git`` closure + carries the right config, so it is threaded in as a parameter. + """ + partial = False + try: + try: + # ``errors="replace"`` for the same reason the staged read below + # uses it, one layer earlier (#3639 re-review NB-3): ``git add`` + # echoes the raw path in stderr messages that ``core.quotePath`` + # does not cover (``unable to index file ''``, ``'/' + # does not have a commit checked out``), so a non-UTF-8 name in + # the tree raises ``UnicodeDecodeError`` inside ``run``. Under a + # strict decode that lands in the handler below and stamps + # ``INCOMPLETE:`` on a snapshot that may be perfectly complete — + # and disqualifies the soft branch — over a filename git only + # mentioned in passing. + git(repo_dir, "add", "-A", "--ignore-errors", timeout=120, errors="replace") + except Exception as add_error: # partial index beats no index + partial = True + logger.warning( + "Worktree re-attach: `git add -A` did not complete cleanly; " + "committing whatever reached the index", + agent_worktree_id=agent_worktree_id, + repo=repo, + dirty_entries=n_entries, + dirty_state_unknown=state_unknown, + # A non-zero exit and a 120s ``TimeoutExpired`` both land here + # and mean different things to a triager; the commit message's + # ``INCOMPLETE:`` paragraph is deliberately neutral between + # them, so the distinction has to live in the log. + error_type=type(add_error).__name__, + error=str(add_error), + ) + # ``-z`` (NUL-terminated, unmunged bytes) rather than the newline + # form: with the default ``core.quotePath=true`` git C-quote-encodes + # any path holding non-ASCII or control characters, so + # ``.egg-state/agent-outputs/coder/brc-memory-café.md`` comes back as + # the literal token ``".egg-state/.../brc-memory-caf\303\251.md"``, + # double quotes included — and ``splitlines()`` additionally misparses + # a path containing a newline. Those encoded names would flow verbatim + # into :func:`_path_matches_glob` (harmless: the leading quote fails + # every glob, so the record takes the imperative — the safe default) + # and into the ``wip_paths`` bus metadata, which is a machine-readable + # field a consumer matches its own paths against. NUL separation makes + # the field mean what its name says. + # + # ``-z`` moves the failure mode down a layer. Unmunged bytes reach the + # caller's ``subprocess.run(..., text=True)``, which decodes as strict + # UTF-8 by default: a filename that is not valid UTF-8 (a latin-1 name + # from an extracted archive, a fixture written with raw bytes) would + # raise ``UnicodeDecodeError`` *inside* ``run``, before the split. + # + # ``errors="replace"`` decodes the bad bytes to U+FFFD instead of + # raising, so one bad name costs one name rather than the whole path + # set (#3639 re-review NB-2) — the shape + # ``routes/pipelines/_worktree_sync`` already uses for its own ``-z`` + # read. Replacement is byte-local and never synthesises a NUL, so + # replacing across the whole stream is equivalent to decoding each + # path separately. ``surrogateescape`` was rejected: lone surrogates + # are unencodable by ``json.dumps`` and by a UTF-8 DB driver, which + # would move the failure downstream into the message bus. + # + # Replacement does not perturb :func:`_path_matches_glob` at all — a + # replaced path matches exactly the globs the raw bytes would (#3639 + # re-review NB-2). It is NOT that a U+FFFD fails every glob: the + # ``*`` in ``brc-memory*.md`` swallows one happily. The property is + # that replacement is confined to non-ASCII: every byte in an invalid + # sequence is >= 0x80, U+FFFD is non-ASCII too, and 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 is preserved for the same reason: + # ``/`` is 0x2F and never appears inside an invalid sequence. Keep the + # globs ASCII and this stays true of a new entry. + # + # The handler is deliberately ``Exception`` and not + # ``UnicodeDecodeError`` (#3639 re-review B1). This read is *metadata + # only*: whatever makes it fail — a decode this replace pass does not + # cover, the 60s timeout expiring on a large staged set, a non-zero + # exit from an index another git process is still holding — must not + # reach the outer handler, which abandons the commit and hands the + # whole working tree to the reset. That is #3639 itself, with the + # trigger moved from "one bad filename" to "the metadata read was slow + # or exited non-zero". The cost is strictly one-directional: ``staged + # is None`` can only lose the softening and the size claim, never make + # the record quieter or wronger. A genuinely empty index is unaffected + # — the ``git commit`` below then exits non-zero and falls through to + # the outer handler exactly as it does today. + try: + staged_out = git( + repo_dir, + "diff", + "--cached", + "--name-only", + "-z", + timeout=60, + errors="replace", + ).stdout + staged: list[str] | None = [p for p in staged_out.split("\0") if p] + except Exception as read_error: + logger.warning( + "Worktree re-attach: staged-path list could not be read " + "(undecodable filename, timeout, or a failed diff); " + "committing the snapshot without a path set", + agent_worktree_id=agent_worktree_id, + repo=repo, + dirty_entries=n_entries, + dirty_state_unknown=state_unknown, + error_type=type(read_error).__name__, + error=str(read_error), + ) + staged = None + # Only a *known*-empty index skips the commit. ``staged is None`` means + # "could not tell", and this helper never discards a tree on a maybe. + if staged is not None and not staged: + logger.warning( + "Worktree re-attach: dirty tree held no committable change " + "(ignored files, submodule-only dirt, or a failed add); " + "discarding it", + agent_worktree_id=agent_worktree_id, + repo=repo, + dirty_entries=n_entries, + dirty_state_unknown=state_unknown, + ) + return None + git( + repo_dir, + "-c", + f"user.name={_WIP_COMMIT_AUTHOR_NAME}", + "-c", + f"user.email={_WIP_COMMIT_AUTHOR_EMAIL}", + "commit", + "--no-verify", + "-m", + _WIP_COMMIT_MESSAGE + (_WIP_COMMIT_PARTIAL_SUFFIX if partial else ""), + timeout=120, + ) + sha = git(repo_dir, "rev-parse", "HEAD").stdout.strip() + 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, + dirty_entries=n_entries, + dirty_state_unknown=state_unknown, + error=str(e), + ) + return None + + paths = tuple(staged) if staged is not None else None + logger.warning( + "Worktree re-attach: auto-committed uncommitted work before hard reset (#3639)", + agent_worktree_id=agent_worktree_id, + repo=repo, + dirty_entries=n_entries, + dirty_state_unknown=state_unknown, + # ``wip_*`` throughout, matching the discard WARNING and the bus + # record's metadata keys so one query shape spans all three + # (#3639 re-review NB-1). + wip_files=len(paths) if paths is not None else None, + wip_files_unknown=paths is None, + wip_partial=partial, + wip_commit=sha, + ) + return _DirtySnapshot( + sha=sha, + n_files=len(paths) if paths is not None else None, + paths=paths, + partial=partial, + ) + + +def _path_matches_glob(path: str, glob: str) -> bool: + """Segment-wise glob match where ``*`` does not cross ``/``. + + ``fnmatch`` is the wrong primitive for a discriminator whose whole job + is to match the noise source *precisely*: its ``*`` crosses separators, + so ``.egg-state/agent-outputs/a/b/c/brc-memory-x.md`` matches + ``.../*/brc-memory*.md``. Matching segment-by-segment with equal depth + keeps a deeper path off the soft branch. + + :func:`fnmatch.fnmatchcase` rather than :func:`fnmatch.fnmatch` is a + smaller point and not a behaviour change on the deployment platform: + ``fnmatch`` normalises case through ``os.path.normcase``, which is the + identity on POSIX, so both are case-sensitive on Linux. The explicit + form states the intended semantics on every platform rather than + inheriting them from the host. + """ + segments = path.split("/") + patterns = glob.split("/") + if len(segments) != len(patterns): + return False + return all(fnmatchcase(s, p) for s, p in zip(segments, patterns, strict=True)) + + +def _is_machine_state_only(paths: tuple[str, ...] | None) -> bool: + """True when every captured path is regenerated state with a backstop. + + "Machine state" here means the file is rewritten by the next event and + the thing it carries survives elsewhere (``_MACHINE_STATE_FILE_GLOBS`` + documents the test per member) — *not* that the orchestrator wrote it. + The dominant member, ``brc-memory-.md``, is written by the + sandbox on the agent's own tool call and holds agent-authored prose; it + qualifies on regeneration, not provenance. + + The discriminator behind :func:`_record_discarded_tip`'s soft wording. + An empty or unknown path set is False: softening must be earned by + evidence, never fall out of missing evidence. + """ + if not paths: + return False + return all( + any(_path_matches_glob(p, glob) for glob in _MACHINE_STATE_FILE_GLOBS) for p in paths + ) + + def _record_discarded_tip( *, pipeline_id: str, @@ -633,6 +1149,10 @@ def _record_discarded_tip( was_dirty: bool, recovery_ref: str | None, salvage_error: str | None, + wip_commit: str | None = None, + wip_files: int | None = None, + wip_paths: tuple[str, ...] | None = None, + wip_partial: bool = False, ) -> None: """Durably record a dirty-discard's orphaned tip where the agent looks (#3509). @@ -645,12 +1165,120 @@ def _record_discarded_tip( reset target, and the recovery ref there as a system message to the role. + ``wip_commit`` is set when the tip being discarded is the automatic + snapshot :func:`_preserve_dirty_tree` took of the previous session's + uncommitted work (#3639). The message calls that out, because a + 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. + + ``wip_paths`` is what that snapshot contains, and it is what decides + how hard the message pushes. "Snapshot-only" on its own is a bad proxy + for "trivial": the #3639 incident (110 minutes, 33 modified files, + zero commits) is snapshot-only too, so keying the soft wording off the + commit count alone would soften precisely the case this record exists + for. The ask is softened only when every captured path is a known + machine-maintained state file — one the next event rewrites and whose + contents survive elsewhere (``_MACHINE_STATE_FILE_GLOBS``) — since the + noise source is known by name, and matching it by name is strictly + sharper than any size threshold. On the imperative branches ``wip_files`` is + stated outright rather than asking the reader whether anything is + "missing": a memory-less agent has no baseline against which that + question means anything, which is this function's own premise. (The + soft branch states the paths themselves, so the count is redundant + there and is not rendered.) + + ``wip_partial`` marks a snapshot whose ``git add -A`` did not complete + cleanly — a non-zero exit, or a raise (timeout, undecodable stderr) with + no exit status at all, so the neutral phrasing is the only checkable one. + It is surfaced because an agent that cherry-picks a silently truncated + snapshot fails worse than one told the snapshot is truncated, and it + also *disqualifies* the soft branch: a truncated capture's path list + omits whatever failed to stage, so it cannot establish that the + snapshot holds nothing but state files. + 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. But the *contents* of the snapshot, not its snapshot-ness, are what + # make it ignorable — #3639 itself was 33 files with no commits, and a + # single rewritten source module is the same loss one file wide. Only a + # capture consisting entirely of regenerated-with-a-backstop state files softens; + # an unknown or unrecognised file set counts as substantial, so the soft + # wording is an opt-in for the demonstrably trivial case, never a default. + # + # ``wip_partial`` disqualifies the soft branch for the same reason + # ``wip_paths=None`` does. When ``git add -A`` did not complete cleanly the + # path list is *by construction* only the subset that reached the index — the + # files that failed to stage are absent from it — so "every captured path + # is a state file" says nothing about what was in the tree. A partial + # capture is missing evidence by another name, and softening must never + # fall out of missing evidence. + snapshot_only = bool(wip_commit) and n_commits == 1 + machine_state_only = _is_machine_state_only(wip_paths) + trivial_snapshot = snapshot_only and not wip_partial and machine_state_only + # ``wip_files``/``wip_paths`` are assigned together off ``_DirtySnapshot`` + # and are both ``None`` on the one production path where the snapshot + # exists but its contents could not be read: a filename whose bytes are + # not valid UTF-8 makes the staged-path list undecodable, and + # ``_preserve_dirty_tree`` commits anyway rather than lose the tree over a + # name. So the ``None`` arms — ``_is_machine_state_only(None) is False`` + # above, and the ``snapshot_size`` fallback just below — are live + # degradation paths, not merely defensive: knowing the sha but not the + # contents must take the imperative rather than crash or soften. + snapshot_size = ( + f"{wip_files} file(s) of uncommitted work" if wip_files is not None else "uncommitted work" + ) + + if recovery_ref and trivial_snapshot: + # ``trivial_snapshot`` implies a non-empty ``wip_paths``. + paths = wip_paths or () + # The descriptor stays out of the sentence's grammar: an apposition + # ("— a state file the orchestrator rewrites on every BRC ack/nack") + # is singular on a plural subject, and it hardcodes one member's + # provenance into a message keyed off a tuple designed to grow, so + # the second entry makes the clause false. + if len(paths) == 1: + named = f"only `{paths[0]}`" + elif len(paths) <= _SOFT_BRANCH_MAX_NAMED_PATHS: + named = f"only {len(paths)} files (" + ", ".join(f"`{p}`" for p in paths) + ")" + else: + named = f"only {len(paths)} files" + # "rewritten by the step that produces it rather than restored before + # you start" and not "rebuilt on your next event" (#3639 re-review + # NB-7): nothing restores these files ahead of the agent. brc-memory + # is rewritten by the agent's *own* ``brc_ack``/``brc_nack`` — i.e. + # after it has already redone the review — and the other two entries + # are rewritten by whichever orchestrator step next produces them. The + # softening still holds (none of it is lost work), but the sentence + # must not promise the agent it will find the file waiting. + recovery_text = ( + f"The snapshot holds {named} — machine-maintained coordination state, " + "rewritten by the step that produces it rather than restored " + "before you start, and durably recorded elsewhere. It is " + f"preserved on remote ref {recovery_ref}; run `git fetch origin " + f"{recovery_ref}` to read it if you need it." + ) + elif recovery_ref and snapshot_only: + recovery_text = ( + f"The snapshot holds {snapshot_size} and is preserved on remote ref " + f"{recovery_ref}; run `git fetch origin {recovery_ref}` and inspect it " + "before starting work. If it contains completed work, build on it " + "(cherry-pick or reset) instead of 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 " @@ -658,19 +1286,73 @@ 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." + ) + if recovery_ref and trivial_snapshot: + # ``trivial_snapshot`` already implies a truthy ``wip_commit`` (via + # ``snapshot_only``), so this arm needs no separate conjunct for it. + # The softening has to survive the whole body: restating "AUTOMATIC + # snapshot ... treat it as a WIP checkpoint to review" here would put + # an imperative in the last sentence the reader sees, undoing the + # branch above — which is exactly the noise this case exists to + # suppress. + wip_text = f" Commit {wip_commit} is that snapshot; it is on the recovery ref above." + elif 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, 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 " + f"changes your previous session left behind ({snapshot_size}, #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 = "" + partial_text = ( + " WARNING: `git add -A` did not complete cleanly while taking this " + "snapshot, so it may be INCOMPLETE — files the previous session's " + "working tree held may be missing from it." + if wip_commit and wip_partial + else "" + ) + count_text = f"{n_commits} unpushed commit(s)" + # ``recovery_text`` already names the snapshot and its contents on the + # trivial branch; repeating it in the opening clause is the third + # restatement of the same fact in one message. That is only true under + # ``recovery_ref`` — with the salvage push failed, ``recovery_text`` is + # the escalation prose, which never names the snapshot, so dropping the + # clarifier there would leave the opening clause silent about what the + # discarded commit actually was. + if wip_commit and not (trivial_snapshot and recovery_ref): + count_text += ( + " (one of which is an automatic snapshot of uncommitted work)" + if n_commits > 1 + else " (an automatic snapshot of uncommitted work)" ) 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 ".") + " " + recovery_text + + wip_text + + partial_text ) try: store = get_message_store() @@ -694,6 +1376,35 @@ def _record_discarded_tip( "was_dirty": was_dirty, "recovery_ref": recovery_ref, "salvage_error": salvage_error, + "wip_commit": wip_commit, + # The body makes a size claim and a completeness claim; + # both belong in the metadata so a consumer (or a triage + # query like "discards over N files") reads them + # structurally instead of regexing the prose. ``wip_paths`` + # and the derived verdict ride along because they are what + # the wording decision is *made from* — without them a + # consumer can see a softened body and cannot reconstruct + # why it was softened. The path list is capped: a bus + # record is not the place to inline an arbitrarily wide + # working tree, and ``wip_files`` already carries the + # untruncated count. + # + # The two derived fields are kept distinct because they + # answer different questions and diverge on real inputs. + # ``wip_machine_state_only`` is the *path predicate* alone, + # so it stays true for a multi-commit discard, a truncated + # capture, or a failed salvage push — cases where the + # wording is not softened. ``wip_softened`` is the actual + # verdict the body took, gated on ``recovery_ref`` exactly + # as the soft branch is. Reporting the verdict under the + # predicate's name would contradict ``wip_paths`` in one + # direction and the body in the other. + "wip_files": wip_files, + "wip_partial": wip_partial, + "wip_paths": list(wip_paths[:_METADATA_MAX_PATHS]) if wip_paths else None, + "wip_paths_truncated": bool(wip_paths and len(wip_paths) > _METADATA_MAX_PATHS), + "wip_machine_state_only": machine_state_only, + "wip_softened": bool(recovery_ref and trivial_snapshot), }, ) ) diff --git a/orchestrator/tests/test_agent_salvage.py b/orchestrator/tests/test_agent_salvage.py index dfc743e97..00a1d621c 100644 --- a/orchestrator/tests/test_agent_salvage.py +++ b/orchestrator/tests/test_agent_salvage.py @@ -4,6 +4,7 @@ import subprocess from pathlib import Path +from types import SimpleNamespace from unittest.mock import MagicMock, patch import pytest @@ -590,6 +591,163 @@ def test_commit_working_tree_captures_dirty_state(self, tmp_path: Path) -> None: # Working tree is clean again — everything was captured. assert _git("status", "--porcelain", cwd=wt.repo_path).stdout.strip() == "" + def test_commit_working_tree_survives_a_partial_add( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """A non-zero ``git add`` must not discard the files that did stage. + + Per ``git-add(1)`` an unindexable entry aborts the add and exits + non-zero with a partially populated index, and ``--ignore-errors`` + still exits non-zero after skipping it. Bailing there would throw away + the other N-1 files — the same defect fixed on the #3639 re-attach + path, on this #2807 sibling. + + What this pins is the behaviour change (a non-zero add does not abort + the commit), not a genuinely partial index: the closure lets the real + ``add`` run and stage everything, then forces the exit code. Producing + a truly unindexable entry needs a fifo or an unreadable file, which is + too environment-dependent for CI. + """ + import agent_salvage + + wt = self._clean_worktree(tmp_path) + self._dirty(wt.repo_path) + real_run_git = agent_salvage._run_git + saw_ignore_errors = False + + def _flaky_run_git(*args: str, **kwargs: object): + nonlocal saw_ignore_errors + result = real_run_git(*args, **kwargs) + if args and args[0] == "add": + saw_ignore_errors = "--ignore-errors" in args + result.returncode = 1 # index populated, exit code still bad + return result + + monkeypatch.setattr(agent_salvage, "_run_git", _flaky_run_git) + sha = commit_working_tree(wt) + + assert saw_ignore_errors, "add must pass --ignore-errors to skip bad entries" + assert sha is not None + assert _git("rev-parse", "HEAD", cwd=wt.repo_path).stdout.strip() == sha + assert ( + _git("show", "HEAD:new_feature.py", cwd=wt.repo_path).stdout + == "def added():\n return 42\n" + ) + # ...and it says so. This path pushes to egg/recovered/ for manual + # triage but writes no bus record, so unlike the #3639 re-attach + # sibling the commit message is the ONLY channel anyone reading that + # ref sees — an untagged truncated commit is indistinguishable from a + # complete one. + assert "INCOMPLETE" in _git("log", "-1", "--format=%B", cwd=wt.repo_path).stdout + + def test_add_stderr_decode_is_non_strict(self, monkeypatch: pytest.MonkeyPatch) -> None: + """``_run_git`` must never decode git's output strictly (R9 B1). + + The ``core.quotePath=true`` pin keeps the ``commit``/``status``/ + ``diff`` output ASCII, but it does **not** cover ``git add``: two of + its stderr messages echo the path raw regardless of the setting. A + strict decode therefore raises ``UnicodeDecodeError`` from inside + ``subprocess.run`` on a worktree holding a non-UTF-8 name, and + :func:`commit_working_tree`'s ``except Exception`` turns the whole + lost working tree into a "continuing" WARNING — #3639 on the #2807 + path. Pinned at the argv level as well as end-to-end below, because + the end-to-end fixture needs a filesystem that accepts raw bytes. + """ + import agent_salvage + + seen: dict[str, object] = {} + + def _capture(cmd, **kwargs): + seen.update(kwargs) + seen["cmd"] = cmd + return SimpleNamespace(returncode=0, stdout="", stderr="") + + monkeypatch.setattr(agent_salvage.subprocess, "run", _capture) + agent_salvage._run_git("add", "-A", cwd=Path("/tmp"), check=False) + + assert seen["errors"] == "replace" + # The pin is complementary, not superseded: it still keeps the + # quoted-output calls ASCII. + assert "core.quotePath=true" in seen["cmd"] + + def test_hostile_filename_does_not_cost_the_whole_working_tree(self, tmp_path: Path) -> None: + """One undecodable name must not discard hours of uncommitted work (R9 B1). + + The seed is a nested repo with no commit checked out whose directory + name is latin-1: ``git add -A --ignore-errors`` reports ``error: + '/' does not have a commit checked out`` with the path echoed + **raw** — a message ``core.quotePath`` does not quote — and keeps + going, so the real work still reaches the index. The only thing that + decides whether it is committed or handed to the gateway's reset is + whether ``_run_git`` can decode that stderr. + + A latin-1 name is not exotic here: extracted archives (CP437/latin-1 + entry names), fixtures written with raw bytes, and agent-cloned + nested repos all produce them. + """ + import os + + wt = self._clean_worktree(tmp_path) + (wt.repo_path / "work.py").write_text("def hours_of_work():\n return 1\n") + nested = wt.repo_path / os.fsdecode(b"nested-caf\xe9") + nested.mkdir() + _git("init", "-q", cwd=nested) + + sha = commit_working_tree(wt) + + assert sha is not None, "the tree was lost to one filename" + assert ( + _git("show", f"{sha}:work.py", cwd=wt.repo_path).stdout + == "def hours_of_work():\n return 1\n" + ) + # The add did report an error, so the snapshot is honestly labelled. + assert "INCOMPLETE" in _git("log", "-1", "--format=%B", cwd=wt.repo_path).stdout + + def test_complete_salvage_commit_is_not_marked_incomplete(self, tmp_path: Path) -> None: + """The clean path must not carry the truncation marker.""" + wt = self._clean_worktree(tmp_path) + self._dirty(wt.repo_path) + + sha = commit_working_tree(wt) + + assert sha is not None + message = _git("log", "-1", "--format=%B", cwd=wt.repo_path).stdout + assert "[salvage] pre-crash working-tree state (#2807)" in message + assert "INCOMPLETE" not in message + + def test_total_add_failure_is_reported_as_an_add_failure( + self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: + """An empty index must not surface as "the commit failed". + + When the add stages nothing, ``git commit`` fails with "nothing added + to commit" — so without an explicit guard the operator reads + ``commit of uncommitted working tree failed`` and goes looking at the + commit, when the cause was the add. Mirrors the re-attach path's + empty-index guard (#3639 re-review). + """ + import agent_salvage + + wt = self._clean_worktree(tmp_path) + self._dirty(wt.repo_path) + real_run_git = agent_salvage._run_git + attempted = [] + + def _failing_add(*args: str, **kwargs: object): + attempted.append(args) + if args and args[0] == "add": + # Nothing reaches the index at all. + return SimpleNamespace(returncode=1, stdout="", stderr="fatal: unable to index") + return real_run_git(*args, **kwargs) + + monkeypatch.setattr(agent_salvage, "_run_git", _failing_add) + assert commit_working_tree(wt) is None + # Bailed before the commit, so the misleading commit-failure log is + # never emitted. + assert not any(a and a[0] == "commit" for a in attempted) + # ...and the tree is left dirty for the caller's own handling. + assert _git("status", "--porcelain", cwd=wt.repo_path).stdout.strip() != "" + def test_salvage_pushes_uncommitted_edits_when_flag_set(self, tmp_path: Path) -> None: """salvage_uncommitted=True: dirty edits land in the pushed HEAD.""" wt = self._clean_worktree(tmp_path) diff --git a/orchestrator/tests/test_kubernetes_spawner.py b/orchestrator/tests/test_kubernetes_spawner.py index ceecd3c63..7a3b7efeb 100644 --- a/orchestrator/tests/test_kubernetes_spawner.py +++ b/orchestrator/tests/test_kubernetes_spawner.py @@ -10,6 +10,7 @@ import hashlib import io import json +import os import shutil import statistics import subprocess @@ -17,6 +18,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 @@ -3815,25 +3817,37 @@ def _push(**kwargs): assert cleaned is True assert _git(repo, "rev-parse", "HEAD").stdout.strip() == origin_head - expected_ref = f"egg/recovered/pipe-1/slice-4-coder/{orphan_head[:12]}" + # #3639: the dirt is committed before the reset, so the doomed tip is + # the WIP snapshot sitting on top of the predecessor's own commit. + wip = head_at_push["sha"] + assert wip != orphan_head + assert _git(repo, "rev-parse", f"{wip}^").stdout.strip() == orphan_head + + expected_ref = f"egg/recovered/pipe-1/slice-4-coder/{wip[:12]}" mock_gateway.push_worktree_branch.assert_called_once() kwargs = mock_gateway.push_worktree_branch.call_args.kwargs assert kwargs["pipeline_id"] == "pipe-1" assert kwargs["repo_path"] == str(repo) assert kwargs["branch"] == expected_ref assert kwargs["ref"] is None - assert head_at_push["sha"] == orphan_head # pushed before the reset msg = get_store.return_value.add_message.call_args.args[0] assert msg.pipeline_id == "pipe-1" assert msg.from_role == "orchestrator" assert msg.to_role == "coder" - assert msg.metadata["discarded_tip"] == orphan_head + assert msg.metadata["discarded_tip"] == wip assert msg.metadata["remote_tip"] == origin_head assert msg.metadata["recovery_ref"] == expected_ref - assert msg.metadata["discarded_commit_count"] == 1 + # The predecessor's own commit AND the WIP snapshot above it. + assert msg.metadata["discarded_commit_count"] == 2 + assert msg.metadata["wip_commit"] == wip assert expected_ref in msg.body - assert orphan_head 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) @@ -3849,9 +3863,19 @@ def test_salvage_failure_still_resets_and_records_tip(self, spawner, mock_gatewa assert _git(repo, "rev-parse", "HEAD").stdout.strip() == origin_head # The tip is still recorded durably even though the push failed. msg = get_store.return_value.add_message.call_args.args[0] - assert msg.metadata["discarded_tip"] == orphan_head + discarded = msg.metadata["discarded_tip"] + 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) @@ -3867,7 +3891,7 @@ def test_record_failure_does_not_block_reuse(self, spawner, mock_gateway, tmp_pa assert _git(repo, "rev-parse", "HEAD").stdout.strip() == origin_head def test_legacy_call_without_context_is_log_only(self, spawner, mock_gateway, tmp_path): - repo, origin_head, _ = self._seed_orphan(tmp_path) + repo, origin_head, _orphan_head = self._seed_orphan(tmp_path) with ( patch("kubernetes_spawner.WORKTREE_BASE_DIR", tmp_path), @@ -3968,8 +3992,13 @@ def test_spawn_event_job_threads_pipeline_context( wait_for_gateway=False, ) + # The salvaged tip is the #3639 WIP snapshot of the dirty tree, which + # sits directly on the predecessor's own unpushed commit. kwargs = mock_gateway.push_worktree_branch.call_args.kwargs - assert kwargs["branch"] == f"egg/recovered/pipe-1/slice-4-coder/{orphan_head[:12]}" + prefix = "egg/recovered/pipe-1/slice-4-coder/" + assert kwargs["branch"].startswith(prefix) + salvaged = kwargs["branch"][len(prefix) :] + assert _git(repo, "rev-parse", f"{salvaged}^").stdout.strip() == orphan_head def test_spawn_event_job_threads_private_mode( self, spawner, mock_k8s_client, mock_gateway, tmp_path @@ -4013,6 +4042,1276 @@ def test_spawn_event_job_threads_private_mode( assert mock_gateway.push_worktree_branch.call_args.kwargs["mode"] == "private" +class TestDirtyTreePreservedBeforeReset: + """Uncommitted work survives the re-attach reset (#3639). + + The #3506/#3509 machinery preserves *commits*: a killed-mid-event + worktree whose session never committed had its entire working set + erased by ``reset --hard`` + ``clean -fd``, with the orphan detector + finding nothing to salvage (the #3639 incident: 110 minutes across 33 + modified files). The dirty tree is now committed BEFORE the reset, so + it becomes an ordinary orphan the existing salvage path recovers, + without relaxing the R6 rule that the successor starts at the origin + tip. + """ + + _MEMORY_FILE = ".egg-state/agent-outputs/coder/brc-memory-pipe-1.md" + + def _seed_dirty(self, tmp_path, *, files=2, machine_state_only=False): + """Worktree with dirt only; no commits ahead of the origin tip. + + ``commit.gpgsign`` is turned on in the repo's own config so the + production closure's ``-c commit.gpgsign=false`` is load-bearing: + without it every snapshot below fails to commit (there is no signing + key in the orchestrator image), which is exactly the regression that + would silently un-fix #3639. + + ``machine_state_only`` seeds the one shape the message is allowed to + soften for: an untracked ``brc-memory-.md`` and nothing + else. + """ + repo, _ = _make_worktree(tmp_path, _WT_ID, "repo", _BRANCH, with_origin=True) + _git(repo, "config", "commit.gpgsign", "true") + origin_head = _git(repo, "rev-parse", f"origin/{_BRANCH}").stdout.strip() + if machine_state_only: + memory = repo / self._MEMORY_FILE + memory.parent.mkdir(parents=True) + memory.write_text("# BRC memory\n\nRound 1: ACKed coder.\n") + return repo, origin_head + (repo / "seed.txt").write_text("hours of uncommitted edits\n") # tracked + if files > 1: + (repo / "new_module.py").write_text("def added():\n return 1\n") # untracked + return repo, origin_head + + def test_uncommitted_work_is_salvaged_not_destroyed(self, spawner, mock_gateway, tmp_path): + """The #3639 regression: dirt with zero commits is preserved.""" + repo, origin_head = 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, + ): + cleaned = spawner._clean_reused_worktree(_WT_ID, _BRANCH, _REPOS, **_PIPE_CTX) + + assert cleaned is True + # R6 unchanged: the successor still starts at the origin tip with a + # clean tree; none of the residue is visible to it. + assert _git(repo, "rev-parse", "HEAD").stdout.strip() == origin_head + assert _git(repo, "status", "--porcelain").stdout.strip() == "" + assert (repo / "seed.txt").read_text() == "seed\n" + assert not (repo / "new_module.py").exists() + + # ... but the work now exists as a pushed snapshot commit. + mock_gateway.push_worktree_branch.assert_called_once() + msg = get_store.return_value.add_message.call_args.args[0] + wip = msg.metadata["wip_commit"] + assert wip == msg.metadata["discarded_tip"] + assert msg.metadata["discarded_commit_count"] == 1 + assert _git(repo, "rev-parse", f"{wip}^").stdout.strip() == origin_head + # Both the tracked edit and the untracked file are in the snapshot. + assert _git(repo, "show", f"{wip}:seed.txt").stdout == "hours of uncommitted edits\n" + assert "def added():" in _git(repo, "show", f"{wip}:new_module.py").stdout + # The resuming agent is told the top commit is a machine snapshot. + assert wip in msg.body + assert "AUTOMATIC snapshot" in msg.body + # This is the #3639 shape (multi-file working set, zero commits), so + # it keeps the imperative ask AND the actionable instruction: being + # snapshot-only must not by itself soften the message. + assert "2 file(s) of uncommitted work" in msg.body + assert "inspect it before starting work" in msg.body + assert "build on it (cherry-pick or reset)" in msg.body + + def test_machine_state_only_snapshot_softens_the_ask(self, spawner, mock_gateway, tmp_path): + """A pure state-file snapshot reads as "read it if you need it". + + ``brc-memory-.md`` is rewritten into the worktree on + every ``brc_ack``/``brc_nack``, so a respawn that trips this path over + that file alone is routine. Keeping the imperative there is how + #3509's message gets trained into background noise — but the + softening keys off *which* files were captured, never off a heuristic + applied to whether the snapshot is taken at all. + """ + repo, _origin_head = self._seed_dirty(tmp_path, machine_state_only=True) + 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 + + msg = get_store.return_value.add_message.call_args.args[0] + assert msg.metadata["discarded_commit_count"] == 1 + assert msg.metadata["wip_files"] == 1 + # The file is named outright: a memory-less agent cannot evaluate "is + # anything missing?", so the message must not ask it to. + assert f"only `{self._MEMORY_FILE}`" in msg.body + assert "read it if you need it" in msg.body + # The softening must survive the whole body, not just its first half. + assert "inspect it before starting work" not in msg.body + assert "Treat it as a WIP checkpoint" not in msg.body + # The record is still emitted and the ref still pushed — wording only. + mock_gateway.push_worktree_branch.assert_called_once() + assert repo.exists() + + def test_one_substantial_file_keeps_the_imperative(self, spawner, mock_gateway, tmp_path): + """A single rewritten source file is #3639 one file wide, not noise.""" + repo, _origin_head = self._seed_dirty(tmp_path, files=1) + 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 + + msg = get_store.return_value.add_message.call_args.args[0] + assert msg.metadata["wip_files"] == 1 + assert "1 file(s) of uncommitted work" in msg.body + assert "inspect it before starting work" in msg.body + assert repo.exists() + + def test_snapshot_only_salvage_failure_escalates(self, spawner, mock_gateway, tmp_path): + """#3639 during a gateway outage: the worst cell of the 2x2. + + ``recovery_ref is None`` *and* snapshot-only — uncommitted work with + no commits behind it, and the push that would have preserved it + failed. The message must escalate rather than point at a ref that + does not exist. + """ + repo, origin_head = self._seed_dirty(tmp_path) + mock_gateway.push_worktree_branch.side_effect = RuntimeError("gateway down") + + 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, _BRANCH, _REPOS, **_PIPE_CTX) + + assert cleaned is True + assert _git(repo, "rev-parse", "HEAD").stdout.strip() == origin_head + + msg = get_store.return_value.add_message.call_args.args[0] + assert msg.metadata["recovery_ref"] is None + assert msg.metadata["wip_commit"] == msg.metadata["discarded_tip"] + assert msg.metadata["discarded_commit_count"] == 1 + # No false reassurance, and no pointer at a ref that was never pushed. + assert "nothing was lost" not in msg.body.lower() + assert "egg/recovered/" not in msg.body + assert "was NOT" in msg.body and "local object store" in msg.body + assert "Escalate" in msg.body + # The size is named here too, so the operator being escalated to + # knows what is at stake before touching the reflog. + assert "2 file(s) of uncommitted work" 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_partial_suffixes_share_one_grep_token(self): + """One search must find a truncated snapshot from either path (R7 N3). + + The two ``INCOMPLETE:`` suffixes are deliberate near-duplicates — + they differ only in naming whose working tree was truncated — and + both comment blocks instruct "change one, change the other". A + comment does not fail when someone edits one of them, and + ``docs/reference/agent-recovery.md`` tells triagers to grep for this + exact token, so pin the shared prefix instead. + + The runbook is pinned too (R8 B2). Coupling the two constants to + each other but not to the doc is what let the doc's copy of the + token drift into a pattern that matches nothing — a false negative + that reads exactly like "no truncated snapshots", which is the one + conclusion that paragraph exists to prevent. + """ + import agent_salvage + from kubernetes_spawner import _worktree + + shared = "\n\nINCOMPLETE: `git add -A` did not complete cleanly while staging, so\nfiles " + assert _worktree._WIP_COMMIT_PARTIAL_SUFFIX.startswith(shared) + assert agent_salvage._UNCOMMITTED_SALVAGE_PARTIAL_SUFFIX.startswith(shared) + # Near-duplicate, not duplicate: the provenance clause is the one + # thing a triager reading a lone commit message cannot infer. + assert ( + _worktree._WIP_COMMIT_PARTIAL_SUFFIX + != agent_salvage._UNCOMMITTED_SALVAGE_PARTIAL_SUFFIX + ) + + # The runbook must quote the token verbatim — backticks included. + # ``git log --grep`` matches the commit message, so a copy that lost + # them in transit (a single-backtick code span cannot contain a + # backtick; backslash escapes do not work inside code spans) renders + # a command that silently finds nothing. + runbook = ( + Path(__file__).resolve().parents[2] / "docs" / "reference" / "agent-recovery.md" + ).read_text() + quoted = shared.strip().split(", so")[0] + assert quoted in runbook, f"agent-recovery.md no longer quotes: {quoted!r}" + assert "git log --all --grep 'INCOMPLETE: `git add -A`'" in runbook + # ``--all`` does include ``refs/remotes/``, which is precisely why the + # fetch matters: both snapshot paths push to origin, so a fresh clone + # has no ref under ``refs/remotes/origin/egg/recovered/`` until the + # namespace is fetched and ``--all`` walks nothing. The runbook must + # name the fetch or the grep is a false negative there (R9 NB-4). + assert "refs/heads/egg/recovered/*:refs/remotes/origin/egg/recovered/*" in runbook + + 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.""" + repo, _origin_head = self._seed_dirty(tmp_path) + pushed_head = {} + + def _push(**kwargs): + pushed_head["sha"] = _git(repo, "rev-parse", "HEAD").stdout.strip() + return _FakePushResult(ok=True) + + mock_gateway.push_worktree_branch.side_effect = _push + + with ( + patch("kubernetes_spawner.WORKTREE_BASE_DIR", tmp_path), + patch("message_store.get_message_store"), + ): + cleaned = spawner._clean_reused_worktree(_WT_ID, _BRANCH, _REPOS, **_PIPE_CTX) + + assert cleaned is True + kwargs = mock_gateway.push_worktree_branch.call_args.kwargs + # Pushed while the snapshot was still HEAD, before the reset. + 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. + + The genuinely partial index (N-1 of N files present) is asserted only + at this unit level, by a git closure that reports a smaller staged set + than the entry count: producing a real unindexable entry needs a fifo + or an unreadable file, which is too environment-dependent for CI. What + the real-git counterpart pins is the surrounding behaviour, not the + partial staging itself. + """ + 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": + assert "-z" in args + return SimpleNamespace(stdout="a.py\0b.py\0", returncode=0) + return SimpleNamespace(stdout="deadbeefcafe\n", returncode=0) + + snapshot = _preserve_dirty_tree( + _flaky_git, tmp_path, agent_worktree_id=_WT_ID, repo="repo", n_entries=3 + ) + assert snapshot is not None + assert snapshot.sha == "deadbeefcafe" + # The file set is what the bus message is worded off, so it must + # reflect what actually reached the index, not the pre-add entry count. + assert snapshot.n_files == 2 + assert snapshot.paths == ("a.py", "b.py") + # ``partial`` is derived from the ``add`` raising, NOT from comparing + # ``len(paths)`` against ``n_entries`` — a count cross-check would be + # unsound anyway, since porcelain collapses an untracked directory to + # one entry, so ``len(paths) > n_entries`` is normal. What it pins: + # the failed add is carried forward, because an agent that + # cherry-picks a silently-truncated snapshot believes it recovered + # everything. + assert snapshot.partial is True + commit_args = next(a for a in seen if a[0] == "commit" or "commit" in a) + assert any("INCOMPLETE" in a for a in commit_args if isinstance(a, str)) + assert any("commit" in a for a in seen) + + def test_complete_snapshot_is_not_marked_partial(self, tmp_path): + """The clean path must not carry the truncation warning.""" + from kubernetes_spawner._worktree import _preserve_dirty_tree + + seen = [] + + def _fake_git(_repo_dir, *args, **_kwargs): + seen.append(args) + if args[0] == "diff": + return SimpleNamespace(stdout="a.py\0b.py\0", returncode=0) + return SimpleNamespace(stdout="deadbeefcafe\n", returncode=0) + + snapshot = _preserve_dirty_tree( + _fake_git, tmp_path, agent_worktree_id=_WT_ID, repo="repo", n_entries=2 + ) + assert snapshot is not None + assert snapshot.partial is False + commit_args = next(a for a in seen if "commit" in a) + assert not any("INCOMPLETE" in a for a in commit_args if isinstance(a, str)) + + def test_unusual_path_bytes_survive_the_staged_file_parse(self, tmp_path): + """``wip_paths`` must be real paths, not C-quoted tokens (R6 #1). + + ``git diff --cached --name-only`` without ``-z`` honours + ``core.quotePath`` (default true), so a non-ASCII name comes back + double-quoted and backslash-escaped, and a name containing a newline + splits across two ``splitlines()`` entries. Both corruptions flow + into the ``wip_paths`` bus metadata, which exists so a consumer can + match paths structurally instead of regexing the body. ``-z`` emits + the bytes unmunged with NUL terminators, so the field means what its + name says. + """ + from kubernetes_spawner._worktree import _preserve_dirty_tree + + seen = [] + raw = ( + ".egg-state/agent-outputs/coder/brc-memory-café.md", + "src/we\nird.py", + ) + + def _fake_git(_repo_dir, *args, **_kwargs): + seen.append(args) + if args[0] == "diff": + return SimpleNamespace(stdout="\0".join(raw) + "\0", returncode=0) + return SimpleNamespace(stdout="deadbeefcafe\n", returncode=0) + + snapshot = _preserve_dirty_tree( + _fake_git, tmp_path, agent_worktree_id=_WT_ID, repo="repo", n_entries=2 + ) + assert snapshot is not None + assert snapshot.paths == raw + assert snapshot.n_files == 2 + # The flag has to be on the request, not just implied by the parse: + # dropping it would silently reintroduce the quoting. + diff_args = next(a for a in seen if a[0] == "diff") + assert "-z" in diff_args + + def test_undecodable_staged_path_does_not_cost_the_commit(self, tmp_path): + """An undecodable filename degrades the metadata, never the snapshot. + + The unit half of R7 B1. ``-z`` is what makes ``wip_paths`` mean real + paths, but it also hands raw bytes to the caller's + ``subprocess.run(..., text=True)``, which decodes as strict UTF-8 — + so a filename that is not valid UTF-8 raises ``UnicodeDecodeError`` + *inside* ``run``, before the ``split("\\0")``. Letting that reach the + outer handler would abandon the commit and hand the whole working + tree to the reset: #3639 reintroduced, over a filename. + + The closure below raises exactly what ``subprocess.run`` raises, so + the assertion is about the layer the ``-z`` change moved the failure + into, not about a pre-decoded fixture. + """ + from kubernetes_spawner._worktree import _preserve_dirty_tree + + seen = [] + + def _undecodable_git(_repo_dir, *args, **_kwargs): + seen.append(args) + if args[0] == "diff": + raise UnicodeDecodeError( + "utf-8", b"src/caf\xe9.md", 7, 8, "invalid continuation byte" + ) + return SimpleNamespace(stdout="deadbeefcafe\n", returncode=0) + + snapshot = _preserve_dirty_tree( + _undecodable_git, tmp_path, agent_worktree_id=_WT_ID, repo="repo", n_entries=33 + ) + + # The commit is the point: it must exist. + assert snapshot is not None + assert snapshot.sha == "deadbeefcafe" + assert any("commit" in a for a in seen) + # Only the path metadata degrades — and it degrades to "unknown", + # which ``_record_discarded_tip`` reads as "take the imperative". + assert snapshot.paths is None + assert snapshot.n_files is None + # An unreadable path list is not a truncated capture: ``partial`` + # means ``git add -A`` did not complete cleanly, and it did here. + assert snapshot.partial is False + + @pytest.mark.parametrize( + "read_error", + [ + pytest.param( + subprocess.TimeoutExpired(cmd=["git", "diff", "--cached"], timeout=60), + id="timeout", + ), + pytest.param( + subprocess.CalledProcessError( + returncode=128, cmd=["git", "diff", "--cached"], stderr="index.lock exists" + ), + id="nonzero-exit", + ), + pytest.param(RuntimeError("closure blew up"), id="unexpected"), + ], + ) + def test_any_failed_staged_path_read_keeps_the_commit(self, tmp_path, read_error): + """*Any* failure of the metadata read degrades metadata, not the tree. + + R8 B1. The R7 fix caught ``UnicodeDecodeError`` specifically, which + left two production triggers still costing the whole working tree: + the read is ``git diff --cached --name-only -z`` with ``timeout=60`` + run immediately after ``git add -A`` staged the entire dirty tree + (33 files was the small case), and it inherits ``check=True`` while + the preceding add's own failure is swallowed into ``partial`` — so a + contended node or a still-held ``index.lock`` reaches the outer + handler, which abandons the commit and hands the tree to + ``reset --hard``. That is #3639 with the trigger moved from "one bad + filename" to "the metadata read was slow". + + Parametrised over the classes rather than pinned to one: the + invariant is about the *category* of failure (this read is metadata, + the commit is the point), and a test named for the invariant that + covers a single exception class is what made the gap invisible. + """ + from kubernetes_spawner._worktree import _preserve_dirty_tree + + seen = [] + + def _failing_read_git(_repo_dir, *args, **_kwargs): + seen.append(args) + if args[0] == "diff": + raise read_error + return SimpleNamespace(stdout="deadbeefcafe\n", returncode=0) + + snapshot = _preserve_dirty_tree( + _failing_read_git, tmp_path, agent_worktree_id=_WT_ID, repo="repo", n_entries=33 + ) + + assert snapshot is not None + assert snapshot.sha == "deadbeefcafe" + assert any("commit" in a for a in seen) + # "Could not tell", not "nothing was there" — the record then takes + # the imperative rather than reassuring the agent. + assert snapshot.paths is None + assert snapshot.n_files is None + assert snapshot.partial is False + + def test_undecodable_bytes_cost_one_name_not_the_path_set(self, tmp_path): + """A latin-1 filename degrades its own entry and no others (R8 NB-2). + + The read passes ``errors="replace"``, so the strict-UTF-8 decode + that used to raise inside ``subprocess.run`` now yields U+FFFD for + the bad bytes. Discarding all 33 paths for one bad byte was + avoidable; ``routes/pipelines/_worktree_sync`` already reads its own + ``-z`` output this way. The replacement does not move the softening + decision either way (R9 NB-2): every non-``*`` character in + ``_MACHINE_STATE_FILE_GLOBS`` is ASCII and replacement only ever + substitutes non-ASCII for non-ASCII, so a replaced path matches + exactly the globs its raw bytes would. + """ + from kubernetes_spawner._worktree import _preserve_dirty_tree + + # What ``subprocess.run(..., text=True, errors="replace")`` hands + # back for ``b"caf\\xe9.md\\0good.py\\0"``. + replaced = b"caf\xe9.md\0good.py\0".decode("utf-8", errors="replace") + + def _replacing_git(_repo_dir, *args, **kwargs): + if args[0] == "diff": + assert kwargs.get("errors") == "replace", "the read must not decode strictly" + return SimpleNamespace(stdout=replaced, returncode=0) + return SimpleNamespace(stdout="deadbeefcafe\n", returncode=0) + + snapshot = _preserve_dirty_tree( + _replacing_git, tmp_path, agent_worktree_id=_WT_ID, repo="repo", n_entries=2 + ) + + assert snapshot is not None + assert snapshot.n_files == 2 + assert snapshot.paths is not None + # The good name survives intact — that is the whole point. + assert "good.py" in snapshot.paths + assert snapshot.paths[0].startswith("caf") and snapshot.paths[0].endswith(".md") + assert "�" in snapshot.paths[0] + + def test_undecodable_filename_is_salvaged_end_to_end(self, spawner, mock_gateway, tmp_path): + """Real git, real non-UTF-8 filename: the 33 files still survive. + + The regression R7 B1 describes needs no exotic setup — one file whose + name is latin-1 (an extracted archive, a fixture written with raw + bytes) alongside hours of ordinary work. ``status --porcelain`` + honours ``core.quotePath`` so the entry point still sees ASCII and + enters the dirty path; the ``-z`` read is where the bytes escape. + This is the only real-git coverage of ``-z``: both other seeds are + ASCII, so a fixture-level test cannot fail on this. Since R8 NB-2 + the read decodes with ``errors="replace"``, so the assertion moved + from "the path list degrades to ``None``" to "the *other* names + survive and only the bad one is replaced" — a strictly smaller + degradation for the same commit. + """ + repo, origin_head = self._seed_dirty(tmp_path) + # ``os.fsdecode`` of invalid UTF-8 yields surrogate escapes, which the + # filesystem round-trips back to the original bytes on Linux. + (repo / os.fsdecode(b"caf\xe9.md")).write_bytes(b"latin-1 name\n") + 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, + ): + cleaned = spawner._clean_reused_worktree(_WT_ID, _BRANCH, _REPOS, **_PIPE_CTX) + + assert cleaned is True + msg = get_store.return_value.add_message.call_args.args[0] + wip = msg.metadata["wip_commit"] + # The snapshot exists and holds the work, not just the odd filename. + assert wip is not None + assert _git(repo, "show", f"{wip}:seed.txt").stdout == "hours of uncommitted edits\n" + assert "def added():" in _git(repo, "show", f"{wip}:new_module.py").stdout + # ... and it was pushed to a recovery ref like any other snapshot. + mock_gateway.push_worktree_branch.assert_called_once() + # Only the bad name degrades: the other two are reported as-is and + # the count is complete, so the agent is told what it actually has. + paths = msg.metadata["wip_paths"] + assert msg.metadata["wip_files"] == 3 + assert set(paths) >= {"seed.txt", "new_module.py"} + assert any("�" in p for p in paths), paths + # The imperative here is earned by ``seed.txt``/``new_module.py``, + # which match no softening glob — NOT by the replaced name, which + # matches whatever its raw bytes would (R9 NB-2). This asserts the + # end-to-end default is unchanged, not the replacement's effect; + # ``test_replacement_does_not_move_the_softening_decision`` covers that. + assert msg.metadata["wip_machine_state_only"] is False + assert msg.metadata["wip_softened"] is False + assert "inspect it before starting work" in msg.body + assert "3 file(s) of uncommitted work" in msg.body + # R6 still holds: the successor starts clean at the origin tip. + assert _git(repo, "rev-parse", "HEAD").stdout.strip() == origin_head + assert _git(repo, "status", "--porcelain").stdout.strip() == "" + + def test_discard_warning_carries_snapshot_completeness(self, spawner, mock_gateway, tmp_path): + """The discard WARNING's ``wip_*`` fields are the query surface (R8 NB-4). + + "Which recovery refs are truncated, and which hold an unknown file + set" has to be answerable from this one line — that is why the + completeness fields ride with the sha instead of living on the + earlier ``_preserve_dirty_tree`` line. Two things have to hold for + that to work: the names must match the bus record's metadata keys + (``wip_files``, not ``preserved_files``), and "unknown" must be + distinguishable from "zero", since a bare ``None`` renders as + ``wip_files=`` and reads as *nothing was preserved*. + + The snapshot is stubbed rather than provoked: the read-failure path + it represents is covered by + :meth:`test_any_failed_staged_path_read_keeps_the_commit`, and what + is under test here is the log line, not how the metadata went + missing. + """ + from kubernetes_spawner import _worktree + + repo, _origin_head = self._seed_dirty(tmp_path) + # The WARNING is gated on `if orphans:`, so the worktree needs a real + # commit ahead of origin as well as the dirt. + (repo / "ahead.txt").write_text("committed but unpushed\n") + _git(repo, "add", "ahead.txt") + _git(repo, "commit", "-m", "ahead of origin") + mock_gateway.push_worktree_branch.return_value = _FakePushResult(ok=True) + + unknown = _worktree._DirtySnapshot( + sha="beefbeefbeef", n_files=None, paths=None, partial=True + ) + with ( + patch("kubernetes_spawner.WORKTREE_BASE_DIR", tmp_path), + patch("message_store.get_message_store"), + patch("kubernetes_spawner._worktree._preserve_dirty_tree", return_value=unknown), + patch("kubernetes_spawner._worktree.logger") as log, + ): + assert spawner._clean_reused_worktree(_WT_ID, _BRANCH, _REPOS, **_PIPE_CTX) is True + + discard = next( + c for c in log.warning.call_args_list if "unpushed local commits" in c.args[0] + ) + assert discard.kwargs["wip_commit"] == "beefbeefbeef" + assert discard.kwargs["wip_partial"] is True + assert discard.kwargs["wip_files"] is None + # The whole point: unknown, not zero. + assert discard.kwargs["wip_files_unknown"] is True + # The off-pattern name must not come back — a consumer querying + # `wip_files` on the bus and `preserved_files` in the log is the + # failure this rename fixed. + assert "preserved_files" not in discard.kwargs + + 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. + + 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") + _git(repo, "commit", "-m", "add gitignore") + _git(repo, "push", "origin", _BRANCH) + origin_head = _git(repo, "rev-parse", f"origin/{_BRANCH}").stdout.strip() + (repo / "build").mkdir() + (repo / "build" / "artifact.o").write_text("binary\n") + + 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, _BRANCH, _REPOS, **_PIPE_CTX) + + assert cleaned is True + assert _git(repo, "rev-parse", "HEAD").stdout.strip() == origin_head + mock_gateway.push_worktree_branch.assert_not_called() + get_store.return_value.add_message.assert_not_called() + + def test_preservation_failure_still_resets(self, spawner, mock_gateway, tmp_path): + """A failed snapshot must not block reuse; the reset still runs. + + ``_preserve_dirty_tree`` reports failure by returning ``None`` (it + swallows its own git errors, asserted below); the discard must then + proceed exactly as it did pre-#3639 rather than fall back to + recreate, which would destroy the same state with less visibility. + """ + repo, origin_head = self._seed_dirty(tmp_path) + + with ( + patch("kubernetes_spawner.WORKTREE_BASE_DIR", tmp_path), + patch("kubernetes_spawner._worktree._preserve_dirty_tree", return_value=None), + patch("message_store.get_message_store") as get_store, + ): + cleaned = spawner._clean_reused_worktree(_WT_ID, _BRANCH, _REPOS, **_PIPE_CTX) + + assert cleaned is True + assert _git(repo, "rev-parse", "HEAD").stdout.strip() == origin_head + assert _git(repo, "status", "--porcelain").stdout.strip() == "" + mock_gateway.push_worktree_branch.assert_not_called() + get_store.return_value.add_message.assert_not_called() + + def test_preserve_helper_swallows_git_failures(self, tmp_path): + """``_preserve_dirty_tree`` never raises: callers must reach the reset.""" + from kubernetes_spawner._worktree import _preserve_dirty_tree + + def _boom(*_args, **_kwargs): + raise RuntimeError("git index locked") + + assert ( + _preserve_dirty_tree( + _boom, tmp_path, agent_worktree_id=_WT_ID, repo="repo", n_entries=3 + ) + is None + ) + + def test_clean_tree_is_not_snapshotted(self, spawner, mock_gateway, tmp_path): + """No dirt ⇒ no snapshot commit (the common path is untouched).""" + repo, _ = _make_worktree(tmp_path, _WT_ID, "repo", _BRANCH, with_origin=True) + origin_head = _git(repo, "rev-parse", f"origin/{_BRANCH}").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, _BRANCH, _REPOS, **_PIPE_CTX) + + assert cleaned is True + assert _git(repo, "rev-parse", "HEAD").stdout.strip() == origin_head + mock_gateway.push_worktree_branch.assert_not_called() + get_store.return_value.add_message.assert_not_called() + + +class TestDiscardedTipMessageWording: + """The 2x2 the resuming agent actually reads (#3639 / #3509). + + ``_record_discarded_tip`` composes off two independent axes — did the + salvage push succeed (``recovery_ref``), and is the discard nothing but + the machine-made snapshot (``n_commits``/``wip_paths``). The end-to-end + tests above drive real git through three of the cells; these pin all + four plus the machine-state discriminator directly, so a wording + regression is caught without a worktree. + """ + + _MEMORY_FILE = ".egg-state/agent-outputs/coder/brc-memory-pipe-1.md" + + _BASE = { + "pipeline_id": "pipe-1", + "agent_worktree_id": _WT_ID, + "repo": "repo", + "branch": _BRANCH, + "agent_role": "coder", + "slice_id": "slice-4", + "discarded_tip": "aaaa1111", + "remote_tip": "bbbb2222", + "was_dirty": True, + } + + def _message(self, **overrides): + from kubernetes_spawner._worktree import _record_discarded_tip + + kwargs = {**self._BASE, "salvage_error": None, **overrides} + with patch("message_store.get_message_store") as get_store: + _record_discarded_tip(**kwargs) + return get_store.return_value.add_message.call_args.args[0] + + def _body(self, **overrides): + return self._message(**overrides).body + + def test_multi_file_snapshot_keeps_the_imperative(self): + body = self._body( + n_commits=1, + recovery_ref="egg/recovered/x", + wip_commit="aaaa1111", + wip_files=33, + wip_paths=tuple(f"src/mod_{i}.py" for i in range(33)), + ) + # The #3639 incident itself is snapshot-only; being snapshot-only must + # not be what softens the ask. + assert "33 file(s) of uncommitted work" in body + assert "inspect it before starting work" in body + assert "build on it (cherry-pick or reset)" in body + + def test_machine_state_only_snapshot_is_softened(self): + """A capture that is nothing but regenerated state relaxes. + + The softening has to hold across the *whole* body: a trailing "treat + it as a WIP checkpoint to review" would put an imperative in the last + sentence the agent reads and undo the branch entirely. + """ + body = self._body( + n_commits=1, + recovery_ref="egg/recovered/x", + wip_commit="aaaa1111", + wip_files=1, + wip_paths=(self._MEMORY_FILE,), + ) + assert f"only `{self._MEMORY_FILE}`" in body + assert "read it if you need it" in body + assert "inspect it before starting work" not in body + assert "Treat it as a WIP checkpoint" not in body + # And the opening clause does not restate it a third time. + assert "an automatic snapshot of uncommitted work" not in body + + def test_one_substantial_file_keeps_the_imperative(self): + """A single rewritten module is #3639 one file wide, not noise. + + This is why the discriminator matches the noise source by name + instead of counting files: on a count threshold this case scores + identically to the memory file above and gets talked out of fetching + real work. + """ + body = self._body( + n_commits=1, + recovery_ref="egg/recovered/x", + wip_commit="aaaa1111", + wip_files=1, + wip_paths=("orchestrator/kubernetes_spawner/_worktree.py",), + ) + assert "1 file(s) of uncommitted work" in body + assert "inspect it before starting work" in body + assert "Treat it as a WIP checkpoint" in body + + def test_mixed_snapshot_keeps_the_imperative(self): + """One real file alongside the memory file is not a trivial capture.""" + body = self._body( + n_commits=1, + recovery_ref="egg/recovered/x", + wip_commit="aaaa1111", + wip_files=2, + wip_paths=(self._MEMORY_FILE, "orchestrator/agent_salvage.py"), + ) + assert "inspect it before starting work" in body + + def test_unknown_paths_are_treated_as_substantial(self): + """No path set ⇒ the imperative. Soft wording is opt-in, never a default. + + This is a live production path, not merely defensive (R7 B1): a + filename whose bytes are not valid UTF-8 makes the staged-path read + undecodable, and ``_preserve_dirty_tree`` commits blind rather than + lose the tree over a name — so the record knows the sha but not the + contents. It must degrade to the loud branch rather than the quiet + one. The end-to-end path is pinned by + ``TestDirtyTreePreservedBeforeReset``'s undecodable-filename cases. + """ + body = self._body( + n_commits=1, + recovery_ref="egg/recovered/x", + wip_commit="aaaa1111", + wip_files=None, + wip_paths=None, + ) + assert "inspect it before starting work" in body + assert "read it if you need it" not in body + + def test_multi_commit_discard_describes_the_stack(self): + body = self._body( + n_commits=3, + recovery_ref="egg/recovered/x", + wip_commit="aaaa1111", + wip_files=2, + wip_paths=("a.py", "b.py"), + ) + assert "The full commit stack is preserved" in body + assert "one of which is an automatic snapshot" in body + assert "nothing was lost" in body + + def test_snapshot_only_push_failure_escalates(self): + """The untested cell: #3639 during a gateway outage.""" + body = self._body( + n_commits=1, + recovery_ref=None, + salvage_error="gateway down", + wip_commit="aaaa1111", + wip_files=33, + wip_paths=tuple(f"src/mod_{i}.py" for i in range(33)), + ) + assert "nothing was lost" not in body.lower() + assert "egg/recovered/" not in body + assert "33 file(s) of uncommitted work" in body + assert "was NOT" in body and "local object store" in body + assert "Escalate" in body + assert "salvage_agent_commits cannot recover them" in body + + def test_partial_snapshot_is_flagged_to_the_reader(self): + """A truncated capture must not look identical to a complete one. + + The WARNING that records the failed ``add`` goes to orchestrator + logs, which the resuming agent never reads — so an agent that + cherry-picks a silently-truncated snapshot believes it recovered + everything. + """ + msg = self._message( + n_commits=1, + recovery_ref="egg/recovered/x", + wip_commit="aaaa1111", + wip_files=5, + wip_paths=tuple(f"src/mod_{i}.py" for i in range(5)), + wip_partial=True, + ) + assert "INCOMPLETE" in msg.body + assert msg.metadata["wip_partial"] is True + + def test_partial_machine_state_snapshot_keeps_the_imperative(self): + """A truncated capture cannot earn the soft branch (R4 blocking #1). + + When ``git add -A`` does not complete cleanly, ``wip_paths`` is by construction + only the subset that reached the index — whatever failed to stage is + absent from it. "Every captured path is a state file" then says + nothing about the working tree, so this is the same missing evidence + as ``wip_paths=None`` and must degrade the same way. Without this the + body contradicts itself: "holds only X, not agent work" followed by + "files may be missing from it", on the one input where the captured + set is least representative. + """ + body = self._body( + n_commits=1, + recovery_ref="egg/recovered/x", + wip_commit="aaaa1111", + wip_files=1, + wip_paths=(self._MEMORY_FILE,), + wip_partial=True, + ) + assert "inspect it before starting work" in body + assert "Treat it as a WIP checkpoint" in body + assert "INCOMPLETE" in body + assert "read it if you need it" not in body + + def test_plural_machine_state_snapshot_names_each_path(self): + """Two memory files must not read as a singular apposition (R4 #3). + + The descriptor stays out of the sentence's grammar so the clause + survives both a plural subject and a second entry in + ``_MACHINE_STATE_FILE_GLOBS`` whose provenance is not BRC ack/nack. + """ + paths = ( + self._MEMORY_FILE, + ".egg-state/agent-outputs/tester/brc-memory-pipe-1.md", + ) + body = self._body( + n_commits=1, + recovery_ref="egg/recovered/x", + wip_commit="aaaa1111", + wip_files=2, + wip_paths=paths, + ) + assert "only 2 files" in body + for path in paths: + assert f"`{path}`" in body + assert "a state file the orchestrator rewrites" not in body + assert "inspect it before starting work" not in body + + def test_wide_roster_snapshot_states_a_count_instead_of_naming_paths(self): + """Past ``_SOFT_BRANCH_MAX_NAMED_PATHS`` the body stops enumerating (R5 #3). + + This is the branch a real roster hits: five BRC roles each leaving a + memory file is five paths, and inlining a dozen of them to say + "nothing here" spends the reader's context budget on noise. The full + list still rides in the metadata. + """ + paths = tuple( + f".egg-state/agent-outputs/{role}/brc-memory-pipe-1.md" + for role in ("coder", "tester", "reviewer_code", "reviewer_design", "documenter") + ) + msg = self._message( + n_commits=1, + recovery_ref="egg/recovered/x", + wip_commit="aaaa1111", + wip_files=len(paths), + wip_paths=paths, + ) + assert "holds only 5 files — machine-maintained coordination state" in msg.body + for path in paths: + assert f"`{path}`" not in msg.body + assert "read it if you need it" in msg.body + assert msg.metadata["wip_paths"] == list(paths) + # Dropping the enumeration is a rendering choice inside the soft + # branch, not an exit from it: the verdict the metadata reports has + # to still say softened (R6 #2). + assert msg.metadata["wip_softened"] is True + assert msg.metadata["wip_machine_state_only"] is True + + def test_exactly_max_named_paths_still_enumerates(self): + """``len(paths) == _SOFT_BRANCH_MAX_NAMED_PATHS`` is the last inlining case. + + The cap is a ``<=``, so four paths enumerate and five do not. 1, 2, + and 5 are covered elsewhere; this pins the boundary itself so an + off-by-one in either direction shows up as a test failure rather than + as one path silently dropped from — or a wide roster silently + inlined into — the body (R6 #2). + """ + from kubernetes_spawner._worktree import _SOFT_BRANCH_MAX_NAMED_PATHS + + paths = tuple( + f".egg-state/agent-outputs/{role}/brc-memory-pipe-1.md" + for role in ("coder", "tester", "reviewer_code", "documenter") + ) + assert len(paths) == _SOFT_BRANCH_MAX_NAMED_PATHS + msg = self._message( + n_commits=1, + recovery_ref="egg/recovered/x", + wip_commit="aaaa1111", + wip_files=len(paths), + wip_paths=paths, + ) + assert f"only {len(paths)} files (" in msg.body + for path in paths: + assert f"`{path}`" in msg.body + assert "read it if you need it" in msg.body + assert msg.metadata["wip_softened"] is True + + def test_flat_orchestrator_state_files_are_machine_state(self): + """``agent-outputs/`` residue is not only the per-role memory file. + + ``consensus-confirmed`` and the applier's *input* handoff are written + by orchestrator code too, so a respawn whose only dirt is a memory + file plus one of them is the same noise the softening exists for. + """ + body = self._body( + n_commits=1, + recovery_ref="egg/recovered/x", + wip_commit="aaaa1111", + wip_files=3, + wip_paths=( + self._MEMORY_FILE, + ".egg-state/agent-outputs/consensus-confirmed", + ".egg-state/agent-outputs/pipe-1-apply-handoff.json", + ), + ) + assert "read it if you need it" in body + assert "inspect it before starting work" not in body + + def test_agent_written_outputs_are_not_machine_state(self): + """The applier's and tester's own outputs are agent work, not residue. + + They sit in the same directory and look alike, which is exactly why + the discriminator is an explicit allowlist rather than a prefix + match on ``.egg-state/agent-outputs/``. + """ + for path in ( + ".egg-state/agent-outputs/pipe-1-wontdo.json", + ".egg-state/agent-outputs/pipe-1-tester-output.json", + ): + body = self._body( + n_commits=1, + recovery_ref="egg/recovered/x", + wip_commit="aaaa1111", + wip_files=1, + wip_paths=(path,), + ) + assert "inspect it before starting work" in body, path + + def test_glob_star_does_not_cross_a_path_separator(self): + """The discriminator matches by name, so it must match precisely (R4 #5). + + ``fnmatch``'s ``*`` crosses ``/``, so a deeper path or a lookalike + directory would slip onto the soft branch — the first two cases are + what the old primitive genuinely got wrong. The third pins + case-sensitivity as intended semantics on every platform rather than + as a regression: ``os.path.normcase`` is the identity on POSIX, so + ``fnmatch`` already rejected it on the deployment platform. Nothing + writes any of these today — the point is that the primitive cannot be + the thing that lets one through later. + """ + for path in ( + ".egg-state/agent-outputs/a/b/c/brc-memory-x.md", + ".egg-state/agent-outputs/coder/brc-memory-p1.md/evil.md", + ".egg-state/Agent-Outputs/coder/brc-memory-p1.md", + ): + body = self._body( + n_commits=1, + recovery_ref="egg/recovered/x", + wip_commit="aaaa1111", + wip_files=1, + wip_paths=(path,), + ) + assert "inspect it before starting work" in body, path + + def test_replacement_does_not_move_the_softening_decision(self): + """A U+FFFD in a path matches exactly what its raw bytes would (R9 NB-2). + + The comment on the ``errors="replace"`` read used to claim a replaced + name "still fails every glob", which is false — the ``*`` in + ``brc-memory*.md`` swallows a U+FFFD happily, and the first case here + softens. The property that actually holds is neutrality: every + non-``*`` character in ``_MACHINE_STATE_FILE_GLOBS`` is ASCII and + replacement only ever substitutes non-ASCII (U+FFFD) for non-ASCII + (bytes >= 0x80), so a literal position can neither gain nor lose a + match and ``*`` regions are length-agnostic. Segment count survives + for the same reason: ``/`` is 0x2F and never appears inside an + invalid sequence. Keep the globs ASCII and a new entry inherits this. + """ + replaced_memory = b".egg-state/agent-outputs/coder/brc-memory-caf\xe9.md".decode( + "utf-8", errors="replace" + ) + replaced_output = b".egg-state/agent-outputs/pipe-caf\xe9-wontdo.json".decode( + "utf-8", errors="replace" + ) + assert "�" in replaced_memory and "�" in replaced_output + + # A state file whose name went through replacement still softens... + soft = self._body( + n_commits=1, + recovery_ref="egg/recovered/x", + wip_commit="aaaa1111", + wip_files=1, + wip_paths=(replaced_memory,), + ) + assert "read it if you need it" in soft + assert "inspect it before starting work" not in soft + + # ...and agent output whose name did still takes the imperative. + hard = self._body( + n_commits=1, + recovery_ref="egg/recovered/x", + wip_commit="aaaa1111", + wip_files=1, + wip_paths=(replaced_output,), + ) + assert "inspect it before starting work" in hard + + def test_trivial_snapshot_with_failed_push_still_names_the_snapshot(self): + """The opening-clause suppression is conditional on ``recovery_ref`` (R4 #4). + + It is justified by ``recovery_text`` already naming the snapshot — + true only on the pushed branch. With the salvage push failed, + ``recovery_text`` is the escalation prose, which never names it, so + dropping the clarifier would leave the reader with a bare commit + count and no statement of what was discarded. + """ + body = self._body( + n_commits=1, + recovery_ref=None, + salvage_error="gateway down", + wip_commit="aaaa1111", + wip_files=1, + wip_paths=(self._MEMORY_FILE,), + ) + assert "(an automatic snapshot of uncommitted work)" in body + assert "Escalate" in body + + def test_metadata_carries_the_size_and_completeness_claims(self): + """The body makes both claims; a consumer must not have to regex prose.""" + msg = self._message( + n_commits=1, + recovery_ref="egg/recovered/x", + wip_commit="aaaa1111", + wip_files=33, + wip_paths=tuple(f"src/mod_{i}.py" for i in range(33)), + ) + assert msg.metadata["wip_files"] == 33 + assert msg.metadata["wip_partial"] is False + + def test_metadata_carries_the_softening_inputs(self): + """A consumer seeing a softened body must be able to reconstruct why. + + ``wip_files``/``wip_partial`` describe the snapshot; ``wip_paths``, + ``wip_machine_state_only`` and ``wip_softened`` are what the wording + was *decided from* and what it decided, and without them the + softening is unauditable downstream. + """ + soft = self._message( + n_commits=1, + recovery_ref="egg/recovered/x", + wip_commit="aaaa1111", + wip_files=1, + wip_paths=(self._MEMORY_FILE,), + ) + assert soft.metadata["wip_paths"] == [self._MEMORY_FILE] + assert soft.metadata["wip_paths_truncated"] is False + assert soft.metadata["wip_machine_state_only"] is True + assert soft.metadata["wip_softened"] is True + + loud = self._message( + n_commits=1, + recovery_ref="egg/recovered/x", + wip_commit="aaaa1111", + wip_files=1, + wip_paths=("src/feature.py",), + ) + assert loud.metadata["wip_machine_state_only"] is False + assert loud.metadata["wip_softened"] is False + + def test_metadata_predicate_and_verdict_diverge(self): + """The path predicate and the wording verdict are separate fields (R5 #1). + + Each of the three cases below has machine-state-only paths but a body + that is *not* softened, for a different reason. Reporting the verdict + under ``wip_machine_state_only`` would contradict ``wip_paths`` in the + same dict; reporting the predicate as the verdict would tell a triage + query filtering for softened records that the escalation body below + was softened. Both fields, both honest. + """ + machine_state = { + "recovery_ref": "egg/recovered/x", + "wip_commit": "aaaa1111", + "wip_files": 1, + "wip_paths": (self._MEMORY_FILE,), + } + + # A stack of the agent's own commits rode along with the snapshot: + # the paths are still state files, the discard is not trivial. + multi = self._message(**{**machine_state, "n_commits": 3}) + assert multi.metadata["wip_machine_state_only"] is True + assert multi.metadata["wip_softened"] is False + assert "inspect it before starting work" in multi.body + + # The capture is truncated, so the path list does not describe the + # tree — softening cannot be earned from it. + partial = self._message(**{**machine_state, "n_commits": 1, "wip_partial": True}) + assert partial.metadata["wip_machine_state_only"] is True + assert partial.metadata["wip_softened"] is False + assert "inspect it before starting work" in partial.body + + # The salvage push failed: the body is the loudest one this function + # emits, so the verdict must not read as softened. + unpushed = self._message( + **{ + **machine_state, + "n_commits": 1, + "recovery_ref": None, + "salvage_error": "gateway down", + } + ) + assert unpushed.metadata["wip_machine_state_only"] is True + assert unpushed.metadata["wip_softened"] is False + assert "Escalate to an operator" in unpushed.body + + def test_metadata_path_list_is_capped(self): + """A pathological working tree must not inline itself into the bus. + + ``wip_files`` still carries the untruncated count, and the truncation + is flagged so a consumer does not read the capped list as complete. + """ + msg = self._message( + n_commits=1, + recovery_ref="egg/recovered/x", + wip_commit="aaaa1111", + wip_files=200, + wip_paths=tuple(f"src/mod_{i}.py" for i in range(200)), + ) + assert len(msg.metadata["wip_paths"]) == 50 + assert msg.metadata["wip_paths_truncated"] is True + assert msg.metadata["wip_files"] == 200 + + def test_no_snapshot_body_is_unchanged(self): + """A pure commit discard says nothing about snapshots.""" + body = self._body(n_commits=2, recovery_ref="egg/recovered/x", wip_commit=None) + assert "snapshot" not in body + assert "The full commit stack is preserved" in body + + class TestSpawnEventJobSessionReuse: """Per-role gateway-session reuse across one-shot event spawns (#3064 slice-4).