diff --git a/docs/architecture/on-demand-agent-lifecycle.md b/docs/architecture/on-demand-agent-lifecycle.md index 816e33742..b084ed5b2 100644 --- a/docs/architecture/on-demand-agent-lifecycle.md +++ b/docs/architecture/on-demand-agent-lifecycle.md @@ -237,8 +237,11 @@ Under orchestrator ownership the worktree becomes a hot path. `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`, which matches no softening glob) rather - than the whole path set. Anything that still defeats that read — a + 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 diff --git a/orchestrator/agent_salvage.py b/orchestrator/agent_salvage.py index e82399a90..c38726b02 100644 --- a/orchestrator/agent_salvage.py +++ b/orchestrator/agent_salvage.py @@ -252,12 +252,23 @@ def _run_git( 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. + 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", @@ -275,6 +286,7 @@ def _run_git( cmd, capture_output=True, text=True, + errors="replace", check=check, timeout=timeout, ) @@ -720,14 +732,18 @@ def commit_working_tree(worktree: AgentWorktree) -> str | None: # 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 + # 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``. 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. + # (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", diff --git a/orchestrator/kubernetes_spawner/_worktree.py b/orchestrator/kubernetes_spawner/_worktree.py index 3d8cc6204..67740ecde 100644 --- a/orchestrator/kubernetes_spawner/_worktree.py +++ b/orchestrator/kubernetes_spawner/_worktree.py @@ -441,10 +441,13 @@ def _git( "-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 + # 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`` @@ -922,7 +925,17 @@ def _preserve_dirty_tree( partial = False try: try: - git(repo_dir, "add", "-A", "--ignore-errors", timeout=120) + # ``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( @@ -966,9 +979,19 @@ def _preserve_dirty_tree( # 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. + # 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 @@ -1171,7 +1194,9 @@ def _record_discarded_tip( 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`` reported errors. + ``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 @@ -1195,8 +1220,8 @@ def _record_discarded_tip( # 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`` reported errors the path - # list is *by construction* only the subset that reached the index — the + # ``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 @@ -1299,9 +1324,9 @@ def _record_discarded_tip( else: wip_text = "" partial_text = ( - " WARNING: `git add -A` reported errors while taking this snapshot, so " - "it may be INCOMPLETE — files the previous session's working tree held " - "may be missing from it." + " 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 "" ) diff --git a/orchestrator/tests/test_agent_salvage.py b/orchestrator/tests/test_agent_salvage.py index 4a3718253..00a1d621c 100644 --- a/orchestrator/tests/test_agent_salvage.py +++ b/orchestrator/tests/test_agent_salvage.py @@ -640,6 +640,69 @@ def _flaky_run_git(*args: str, **kwargs: object): # 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) diff --git a/orchestrator/tests/test_kubernetes_spawner.py b/orchestrator/tests/test_kubernetes_spawner.py index c7dd1b89c..7a3b7efeb 100644 --- a/orchestrator/tests/test_kubernetes_spawner.py +++ b/orchestrator/tests/test_kubernetes_spawner.py @@ -4279,9 +4279,11 @@ def test_partial_suffixes_share_one_grep_token(self): 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. + # ``--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): @@ -4572,9 +4574,11 @@ def test_undecodable_bytes_cost_one_name_not_the_path_set(self, tmp_path): 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. + ``-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 @@ -4642,8 +4646,11 @@ def test_undecodable_filename_is_salvaged_end_to_end(self, spawner, mock_gateway 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. + # 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 @@ -4959,7 +4966,7 @@ def test_partial_snapshot_is_flagged_to_the_reader(self): 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`` reports errors, ``wip_paths`` is by construction + 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 @@ -5131,6 +5138,49 @@ def test_glob_star_does_not_cross_a_path_separator(self): ) 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).