From 1d2e364a85b3b09be5617f934c6b04cd94476f9e Mon Sep 17 00:00:00 2001 From: "egg-reviewer[bot]" <261018737+egg-reviewer[bot]@users.noreply.github.com> Date: Mon, 27 Jul 2026 01:09:19 +0000 Subject: [PATCH 1/2] Never let a filename cost the working tree (#3639 re-review B1, N1-N5) B1: the R6 `diff --cached --name-only -z` read moved the failure mode below the output layer of git. Without `-z`, `core.quotePath` guarantees ASCII; with it, unmunged bytes reach `subprocess.run(..., text=True)` and a non-UTF-8 filename raises `UnicodeDecodeError` *inside* `run`. The outer best-effort handler swallowed it, abandoned the snapshot commit, and handed the whole tree to the hard reset -- #3639 itself, over a filename. Keep `-z` (R6 bought real metadata fidelity) but catch the decode at the read and commit blind: `_DirtySnapshot.paths`/`n_files` widen to `None`, which the existing wording layer already reads as "take the imperative". Only a *known*-empty index still skips the commit. N1: the machine-state softening rule claimed provenance it does not have. brc-memory is written by the sandbox on the agent own `brc_ack`/`brc_nack` tool call and holds agent-authored prose. Replace "written by orchestrator code" with the rule that actually holds -- regenerated by the next event, with a durable backstop elsewhere -- which admits brc-memory on its merits and still excludes `wontdo.json` / `tester-output.json` (agent output, no regeneration path). Propagated to the agent-facing bus message, both docstrings, and on-demand-agent-lifecycle.md. N2: widen the except in `commit_working_tree` to `Exception`. Its docstring promises it never raises, and the class that would break that promise is `UnicodeDecodeError`, not a subprocess error. N3: couple the two `INCOMPLETE:` grep tokens with a test. N4: document incomplete snapshots + the grep in agent-recovery.md. N5: carry `wip_partial`/`preserved_files` on the discard WARNING so completeness is one query, not a join back by worktree id. --- .../architecture/on-demand-agent-lifecycle.md | 37 +++-- docs/reference/agent-recovery.md | 2 +- orchestrator/agent_salvage.py | 13 +- orchestrator/kubernetes_spawner/_worktree.py | 141 +++++++++++++----- orchestrator/tests/test_kubernetes_spawner.py | 128 +++++++++++++++- 5 files changed, 266 insertions(+), 55 deletions(-) diff --git a/docs/architecture/on-demand-agent-lifecycle.md b/docs/architecture/on-demand-agent-lifecycle.md index 4eafd6a57..c97560e20 100644 --- a/docs/architecture/on-demand-agent-lifecycle.md +++ b/docs/architecture/on-demand-agent-lifecycle.md @@ -202,16 +202,24 @@ Under orchestrator ownership the worktree becomes a hot path. 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 an **orchestrator-written** state file, the record softens to "read - it if you need it" so a routine respawn does not train the #3509 - message into background noise. The allowlist is - `.egg-state/agent-outputs/*/brc-memory*.md` (rewritten on every - `brc_ack`/`brc_nack`), `.egg-state/agent-outputs/consensus-confirmed`, - and `.egg-state/agent-outputs/-apply-handoff.json`; - matching is segment-wise so `*` does not cross `/`. Files written by - *agents* into the same directory — `-wontdo.json`, - `-tester-output.json` — are deliberately excluded: they are - agent output, and losing them warrants the imperative. Anything else — + 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 @@ -223,7 +231,14 @@ Under orchestrator ownership the worktree becomes a hot path. 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. A + 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 makes the read undecodable under `subprocess`'s strict + `text=True` decode; that logs a WARNING and commits blind + (`wip_paths`/`wip_files` become `null`, so the record takes the + imperative) rather than letting a filename cost the working tree. A snapshot whose `git add -A` reported errors 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 diff --git a/docs/reference/agent-recovery.md b/docs/reference/agent-recovery.md index d2df36a57..077b1b6ab 100644 --- a/docs/reference/agent-recovery.md +++ b/docs/reference/agent-recovery.md @@ -380,7 +380,7 @@ 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. 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.) +**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` reported errors the commit holds only what reached the index, and its message carries an `INCOMPLETE: \`git add -A\` reported errors 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. `git log --all --grep 'INCOMPLETE: `git add -A`'` finds every truncated snapshot from either path. 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. diff --git a/orchestrator/agent_salvage.py b/orchestrator/agent_salvage.py index db356aab9..510d0fffc 100644 --- a/orchestrator/agent_salvage.py +++ b/orchestrator/agent_salvage.py @@ -698,7 +698,18 @@ 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: ``_run_git`` decodes with + # ``text=True`` and no ``errors=``, so a git command that echoes a + # filename whose bytes are not valid UTF-8 raises ``UnicodeDecodeError`` + # (a ``ValueError``) from inside ``subprocess.run``. Unreachable on this + # path today — it has no ``-z`` call and the default ``core.quotePath`` + # keeps git's output ASCII — but the re-attach path shipped exactly that + # bug by adding ``-z`` (#3639 re-review B1), and letting it escape here + # would abort the committed-but-unpushed salvage that follows. + except Exception as e: logger.warning( "Salvage: capturing uncommitted working tree raised; continuing", worktree_id=worktree.worktree_id, diff --git a/orchestrator/kubernetes_spawner/_worktree.py b/orchestrator/kubernetes_spawner/_worktree.py index 1835d59c7..914440002 100644 --- a/orchestrator/kubernetes_spawner/_worktree.py +++ b/orchestrator/kubernetes_spawner/_worktree.py @@ -645,6 +645,12 @@ def _git(repo_dir: Path, *args: str, timeout: int = 30, check: bool = True): 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. + wip_partial=wip_partial, + preserved_files=wip_files, ) if pipeline_id: _record_discarded_tip( @@ -697,27 +703,39 @@ def _git(repo_dir: Path, *args: str, timeout: int = 30, check: bool = True): # the suite's ``patch("kubernetes_spawner.agent_salvage")`` seam — that # rebinds the package *attribute*, not what is already bound here. -# Paths whose presence in a snapshot carries no agent work: the *orchestrator* -# writes them into the worktree itself, 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 +# 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 "written by orchestrator code", not "looks mechanical": -# * ``/brc-memory-.md`` — ``routes/event_prompt/_memory_io`` -# rewrites it on every ``brc_ack``/``brc_nack``; the dominant case. +# 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``). +# 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. +# 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 -# written by the applier and tester *agents*. They are agent output, and a -# discard that loses them is a loss worth the imperative. +# 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", @@ -782,11 +800,18 @@ class _DirtySnapshot(NamedTuple): 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 — a filename whose bytes are not valid UTF-8 makes the + ``diff --cached`` read undecodable (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 - paths: tuple[str, ...] + n_files: int | None + paths: tuple[str, ...] | None partial: bool = False @@ -814,7 +839,10 @@ def _preserve_dirty_tree( 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). + commit). Nothing short of "no commit exists" returns ``None``: a + staged-path list that cannot be read degrades the snapshot's + *metadata* (``paths``/``n_files`` become ``None``) and never its + existence. 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 @@ -872,9 +900,34 @@ def _preserve_dirty_tree( # 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. - staged_out = git(repo_dir, "diff", "--cached", "--name-only", "-z", timeout=60).stdout - staged = [p for p in staged_out.split("\0") if p] - if not staged: + # + # ``-z`` moves the failure mode down a layer, so it is caught here + # rather than by the outer handler. Unmunged bytes reach the caller's + # ``subprocess.run(..., text=True)``, which decodes as strict UTF-8: a + # filename that is not valid UTF-8 (a latin-1 name from an extracted + # archive, a fixture written with raw bytes) raises + # ``UnicodeDecodeError`` *inside* ``run``, before the split. Letting + # that reach the outer ``except`` would abandon the commit and hand + # the whole working tree to the reset — #3639 itself, over a filename. + # Commit blind instead: an unknown path set costs the softened wording + # (``_is_machine_state_only(None)`` is False) and nothing else. + try: + staged_out = git(repo_dir, "diff", "--cached", "--name-only", "-z", timeout=60).stdout + staged: list[str] | None = [p for p in staged_out.split("\0") if p] + except UnicodeDecodeError as decode_error: + logger.warning( + "Worktree re-attach: staged-path list is not decodable " + "(non-UTF-8 filename); 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=str(decode_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); " @@ -910,18 +963,23 @@ def _preserve_dirty_tree( ) return None - paths = tuple(staged) + 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, - preserved_files=len(paths), + preserved_files=len(paths) if paths is not None else None, preserved_partial=partial, wip_commit=sha, ) - return _DirtySnapshot(sha=sha, n_files=len(paths), paths=paths, partial=partial) + 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: @@ -948,7 +1006,14 @@ def _path_matches_glob(path: str, glob: str) -> bool: def _is_machine_state_only(paths: tuple[str, ...] | None) -> bool: - """True when every captured path is an orchestrator-written state file. + """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 @@ -1010,9 +1075,10 @@ def _record_discarded_tip( 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-written state file (``_MACHINE_STATE_FILE_GLOBS``) — the noise - source is known by name, so matching it by name is strictly sharper - than any size threshold. On the imperative branches ``wip_files`` is + 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 @@ -1038,7 +1104,7 @@ def _record_discarded_tip( # 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 orchestrator-written state files softens; + # 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. # @@ -1052,12 +1118,15 @@ def _record_discarded_tip( 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``, - # so in production a truthy ``wip_commit`` always carries both. The ``None`` - # arms — ``_is_machine_state_only(None) is False`` above, and the - # ``snapshot_size`` fallback just below — are defensive only: they exist so - # a future caller that knows the sha but not the contents degrades to the - # imperative rather than crashing or softening. + # ``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" ) @@ -1077,10 +1146,10 @@ def _record_discarded_tip( else: named = f"only {len(paths)} files" recovery_text = ( - f"The snapshot holds {named} — orchestrator-written state, rewritten " - "mechanically, not agent work. It is preserved on remote ref " - f"{recovery_ref}; run `git fetch origin {recovery_ref}` to read it if " - "you need it." + f"The snapshot holds {named} — machine-maintained coordination state, " + "rebuilt on your next event 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 = ( diff --git a/orchestrator/tests/test_kubernetes_spawner.py b/orchestrator/tests/test_kubernetes_spawner.py index c97e4b9a4..9c786c9ed 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 @@ -4238,6 +4239,31 @@ def test_snapshot_commit_identity_matches_the_restart_path( 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. + """ + import agent_salvage + from kubernetes_spawner import _worktree + + shared = ( + "\n\nINCOMPLETE: `git add -A` reported errors while staging, so files\npresent in the " + ) + 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 + ) + def test_no_branch_takes_no_snapshot(self, spawner, mock_gateway, tmp_path): """``branch is None`` ⇒ no snapshot commit, and HEAD does not move. @@ -4418,6 +4444,93 @@ def _fake_git(_repo_dir, *args, **_kwargs): 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`` reported errors, and the add succeeded here. + assert snapshot.partial is False + + 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. + """ + 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() + # The path list is what degrades. Unknown contents ⇒ the imperative, + # and the size claim falls back to the unquantified phrasing. + assert msg.metadata["wip_paths"] is None + assert msg.metadata["wip_files"] is None + 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 "file(s) of uncommitted work" not 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_ignored_only_dirt_is_discarded_without_a_commit(self, spawner, mock_gateway, tmp_path): """Build output is not agent work: no snapshot, nothing salvaged. @@ -4551,7 +4664,7 @@ def test_multi_file_snapshot_keeps_the_imperative(self): 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 orchestrator-written state relaxes. + """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 @@ -4604,10 +4717,13 @@ def test_mixed_snapshot_keeps_the_imperative(self): def test_unknown_paths_are_treated_as_substantial(self): """No path set ⇒ the imperative. Soft wording is opt-in, never a default. - Defensive only: ``wip_paths`` and ``wip_commit`` are assigned together - off ``_DirtySnapshot``, so production never reaches this. It pins that - a future caller that knows the sha but not the contents degrades to - the loud branch rather than the quiet one. + 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, @@ -4735,7 +4851,7 @@ def test_wide_roster_snapshot_states_a_count_instead_of_naming_paths(self): wip_files=len(paths), wip_paths=paths, ) - assert "holds only 5 files — orchestrator-written state" in msg.body + 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 From ad4f0b6d0e4d7d2015ae5c959fb67cc0dce15c35 Mon Sep 17 00:00:00 2001 From: "egg-reviewer[bot]" <261018737+egg-reviewer[bot]@users.noreply.github.com> Date: Mon, 27 Jul 2026 01:40:05 +0000 Subject: [PATCH 2/2] Degrade the staged-path read, not the snapshot (#3639 r4) Round-4 review fixes for the worktree re-attach preservation path. The staged-path read now decodes with errors="replace" and catches any exception, so a timeout or non-zero diff degrades only the snapshot's metadata while an undecodable filename costs one name instead of the whole path set. core.quotePath=true is pinned explicitly in both _git closures rather than inherited. The discard and success WARNINGs use a consistent wip_* key namespace with an explicit wip_files_unknown flag, and both partial-commit suffixes share one greppable token that the runbook now quotes verbatim inside a fenced block alongside the origin/egg/recovered fetch git log --all needs. --- .../architecture/on-demand-agent-lifecycle.md | 14 +- docs/reference/agent-recovery.md | 11 +- orchestrator/agent_salvage.py | 33 ++- orchestrator/kubernetes_spawner/_worktree.py | 173 ++++++++++++---- orchestrator/tests/test_kubernetes_spawner.py | 193 +++++++++++++++++- 5 files changed, 365 insertions(+), 59 deletions(-) diff --git a/docs/architecture/on-demand-agent-lifecycle.md b/docs/architecture/on-demand-agent-lifecycle.md index c97560e20..816e33742 100644 --- a/docs/architecture/on-demand-agent-lifecycle.md +++ b/docs/architecture/on-demand-agent-lifecycle.md @@ -235,11 +235,15 @@ Under orchestrator ownership the worktree becomes a hot path. 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 makes the read undecodable under `subprocess`'s strict - `text=True` decode; that logs a WARNING and commits blind - (`wip_paths`/`wip_files` become `null`, so the record takes the - imperative) rather than letting a filename cost the working tree. A - snapshot whose `git add -A` reported errors is marked incomplete in + 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`, which matches no softening glob) rather + than the whole path set. 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 diff --git a/docs/reference/agent-recovery.md b/docs/reference/agent-recovery.md index 077b1b6ab..370a6c90a 100644 --- a/docs/reference/agent-recovery.md +++ b/docs/reference/agent-recovery.md @@ -380,7 +380,16 @@ 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` reported errors the commit holds only what reached the index, and its message carries an `INCOMPLETE: \`git add -A\` reported errors 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. `git log --all --grep 'INCOMPLETE: `git add -A`'` finds every truncated snapshot from either path. 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.) +**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. diff --git a/orchestrator/agent_salvage.py b/orchestrator/agent_salvage.py index 510d0fffc..e82399a90 100644 --- a/orchestrator/agent_salvage.py +++ b/orchestrator/agent_salvage.py @@ -98,13 +98,22 @@ # 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. Change one, change the other. +# 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` reported errors while staging, so files\n" - "present in the crashed agent's working tree may be missing from\n" - "this commit." + "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" @@ -241,6 +250,14 @@ def _run_git( ``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). Every call here decodes with ``text=True`` and no ``errors=``, so + git output that echoes a filename verbatim raises ``UnicodeDecodeError`` + the moment a path in the worktree is not valid UTF-8. The default + C-quote-encodes those bytes to ASCII and is what keeps that from + happening; a worktree that inherited ``quotePath=false`` would break it. + Costs nothing — this path has no ``-z`` read to make quoting a problem. """ cmd = [ "git", @@ -248,6 +265,8 @@ def _run_git( "core.hooksPath=/dev/null", "-c", "commit.gpgsign=false", + "-c", + "core.quotePath=true", "-C", str(cwd), *args, @@ -713,6 +732,12 @@ def commit_working_tree(worktree: AgentWorktree) -> str | None: 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 diff --git a/orchestrator/kubernetes_spawner/_worktree.py b/orchestrator/kubernetes_spawner/_worktree.py index 914440002..3d8cc6204 100644 --- a/orchestrator/kubernetes_spawner/_worktree.py +++ b/orchestrator/kubernetes_spawner/_worktree.py @@ -418,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", @@ -434,10 +440,26 @@ def _git(repo_dir: Path, *args: str, timeout: int = 30, check: bool = True): # work the snapshot exists to save. "-c", "commit.gpgsign=false", + # Pinned, not inherited (#3639 re-review NB-3). Every command + # here runs under ``text=True`` with 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 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, ) @@ -648,9 +670,21 @@ def _git(repo_dir: Path, *args: str, timeout: int = 30, check: bool = True): # 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. + # 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, - preserved_files=wip_files, + 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( @@ -763,11 +797,18 @@ def _git(repo_dir: Path, *args: str, timeout: int = 30, check: bool = True): "mechanical checkpoint of a previous session's working tree, not\n" "reviewed work." ) -# Appended when ``git add -A`` reported errors. A truncated snapshot is -# otherwise indistinguishable downstream from a complete one — same subject, +# 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 @@ -775,13 +816,15 @@ def _git(repo_dir: Path, *args: str, timeout: int = 30, check: bool = True): # 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. Change one, change the other. +# 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` reported errors while staging, so files\n" - "present in the previous session's working tree may be missing from\n" - "this commit." + "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." ) @@ -794,16 +837,16 @@ class _DirtySnapshot(NamedTuple): ``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`` reported errors, 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. + ``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 — a filename whose bytes are not valid UTF-8 makes the - ``diff --cached`` read undecodable (see :func:`_preserve_dirty_tree`). + 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. @@ -840,9 +883,12 @@ def _preserve_dirty_tree( 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 degrades the snapshot's + 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. + 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 @@ -880,12 +926,17 @@ def _preserve_dirty_tree( except Exception as add_error: # partial index beats no index partial = True logger.warning( - "Worktree re-attach: `git add -A` reported errors; committing " - "whatever reached the index", + "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 @@ -901,28 +952,59 @@ def _preserve_dirty_tree( # 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, so it is caught here - # rather than by the outer handler. Unmunged bytes reach the caller's - # ``subprocess.run(..., text=True)``, which decodes as strict UTF-8: a - # filename that is not valid UTF-8 (a latin-1 name from an extracted - # archive, a fixture written with raw bytes) raises - # ``UnicodeDecodeError`` *inside* ``run``, before the split. Letting - # that reach the outer ``except`` would abandon the commit and hand - # the whole working tree to the reset — #3639 itself, over a filename. - # Commit blind instead: an unknown path set costs the softened wording - # (``_is_machine_state_only(None)`` is False) and nothing else. + # ``-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. A replaced + # name still fails every glob in :func:`_path_matches_glob`, so it can + # only cost the softened wording, never grant it. + # + # 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).stdout + 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 UnicodeDecodeError as decode_error: + except Exception as read_error: logger.warning( - "Worktree re-attach: staged-path list is not decodable " - "(non-UTF-8 filename); committing the snapshot without a path set", + "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=str(decode_error), + error_type=type(read_error).__name__, + error=str(read_error), ) staged = None # Only a *known*-empty index skips the commit. ``staged is None`` means @@ -970,8 +1052,12 @@ def _preserve_dirty_tree( repo=repo, dirty_entries=n_entries, dirty_state_unknown=state_unknown, - preserved_files=len(paths) if paths is not None else None, - preserved_partial=partial, + # ``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( @@ -1145,9 +1231,18 @@ def _record_discarded_tip( 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, " - "rebuilt on your next event and durably recorded elsewhere. It is " + "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." ) diff --git a/orchestrator/tests/test_kubernetes_spawner.py b/orchestrator/tests/test_kubernetes_spawner.py index 9c786c9ed..c7dd1b89c 100644 --- a/orchestrator/tests/test_kubernetes_spawner.py +++ b/orchestrator/tests/test_kubernetes_spawner.py @@ -4248,13 +4248,17 @@ def test_partial_suffixes_share_one_grep_token(self): 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` reported errors while staging, so files\npresent in the " - ) + 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 @@ -4264,6 +4268,22 @@ def test_partial_suffixes_share_one_grep_token(self): != 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`` is local refs only and both snapshot paths push to origin, + # so the runbook must name the fetch or the grep is a false negative + # on a fresh clone. + 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. @@ -4484,9 +4504,102 @@ def _undecodable_git(_repo_dir, *args, **_kwargs): 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`` reported errors, and the add succeeded here. + # 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. A replaced name still fails every glob in + ``_path_matches_glob``, so this can only cost the softened wording, + never grant it. + """ + 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. @@ -4496,7 +4609,11 @@ def test_undecodable_filename_is_salvaged_end_to_end(self, spawner, mock_gateway 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. + 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 @@ -4519,18 +4636,74 @@ def test_undecodable_filename_is_salvaged_end_to_end(self, spawner, mock_gateway 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() - # The path list is what degrades. Unknown contents ⇒ the imperative, - # and the size claim falls back to the unquantified phrasing. - assert msg.metadata["wip_paths"] is None - assert msg.metadata["wip_files"] is None + # 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 + # A replaced name matches no softening glob, so the record still + # takes the imperative — the safe default is unchanged by NB-2. 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 "file(s) of uncommitted work" not 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.