From 3ea1c11c19d8d80a1d3c64f8a818bf307ced90f1 Mon Sep 17 00:00:00 2001 From: Frank Bria <136862992+frankbria@users.noreply.github.com> Date: Fri, 7 Aug 2026 22:57:31 -0700 Subject: [PATCH 1/7] fix(e2b): contain sandbox downloads and close the unscanned-upload leak (#967) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The E2B sandbox is a trust boundary in both directions, and neither was enforced. The coding agent has arbitrary command execution in the sandbox before we read `git status --porcelain`, so it can shadow the git binary — every path in that output is attacker-controlled input. Inbound: `local = workspace_path / rel_path` had no containment check, then `mkdir(parents=True)` and a write. Two ways out, both confirmed working: `../` segments, and an absolute path — `Path("/ws") / "/etc/passwd"` is `/etc/passwd`, because pathlib discards the left side. The issue's worked example (~/.ssh/authorized_keys) reproduced. - `_safe_local_path` resolves both sides and requires the candidate to be under the workspace root. Resolving both closes the symlink route (a symlink inside the tree pointing out is rejected) without falsely rejecting a workspace reached *through* a symlink. - Absolute paths are rejected outright, including ones that happen to resolve inside the workspace: porcelain paths are always relative, and the remote read would be built as `/workspace//abs/path` anyway. - Rejections are logged as warnings, counted, and emitted — never silently dropped. The check runs before the read, so a rejected path costs no round trip and creates no directory. Parsing uses `--porcelain -z`, which emits each path as raw bytes. Verified against real git: `--porcelain` alone renders `café.txt` as `"caf\303\251.txt"` — quotes and all — and splits renames on `" -> "`, which a filename containing that string would break. `-z` has neither problem, and paths are taken verbatim: a file genuinely named `"a.py"` keeps its quotes rather than being rewritten to `a.py` over the top of a different file. Containment, not parsing, is the security boundary. Outbound: the scanner and the uploader kept separate exclusion sets, and the scanner's was the *wider* one — it skipped dist/build/.tox/.eggs while the uploader shipped them, so a .env baked into a build artifact reached the third-party sandbox unscanned, defeating the abort-on-secrets contract. There is now one `EXCLUDED_DIRS`, and build output is not in it. Existing porcelain fixtures updated to the -z wire format. --- codeframe/adapters/e2b/adapter.py | 121 +++++-- codeframe/adapters/e2b/credential_scanner.py | 19 +- tests/adapters/test_e2b_adapter.py | 6 +- tests/adapters/test_e2b_trust_boundary_967.py | 313 ++++++++++++++++++ 4 files changed, 424 insertions(+), 35 deletions(-) create mode 100644 tests/adapters/test_e2b_trust_boundary_967.py diff --git a/codeframe/adapters/e2b/adapter.py b/codeframe/adapters/e2b/adapter.py index 2c55da5b..c304ef15 100644 --- a/codeframe/adapters/e2b/adapter.py +++ b/codeframe/adapters/e2b/adapter.py @@ -9,10 +9,10 @@ import logging import os import time -from pathlib import Path +from pathlib import Path, PurePosixPath from typing import Callable -from codeframe.adapters.e2b.credential_scanner import scan_path +from codeframe.adapters.e2b.credential_scanner import EXCLUDED_DIRS, scan_path from codeframe.core.adapters.agent_adapter import ( AgentEvent, AgentResult, @@ -34,6 +34,36 @@ _INSTALL_CMD = "pip install codeframe --quiet" + + +def _safe_local_path(workspace_root: Path, rel_path: str) -> Path | None: + """Resolve *rel_path* inside *workspace_root*, or return None to reject. + + The agent has arbitrary command execution in the sandbox before this runs + and can shadow the git binary, so every porcelain path is hostile input + (#967). Two ways out exist without this check: ``..`` segments, and an + absolute path — ``Path("/ws") / "/etc/passwd"`` is ``/etc/passwd``, because + pathlib discards the left side. Both used to reach ``mkdir(parents=True)`` + and a write. + + Both sides are resolved, which also closes the symlink route: a symlink + inside the workspace pointing outward resolves outward and is rejected, + while a workspace *reached* through a symlink is not falsely rejected. + + Returns: + The absolute local path, or None if it escapes the workspace. + """ + if not rel_path or PurePosixPath(rel_path).is_absolute() or Path(rel_path).is_absolute(): + return None + + root = workspace_root.resolve() + candidate = (root / rel_path).resolve() + + if candidate == root or root not in candidate.parents: + return None + return candidate + + class E2BAgentAdapter: """Runs a CodeFrame task inside an E2B Linux sandbox. @@ -257,14 +287,11 @@ def _upload_workspace( emit: Callable[[str, str, dict | None], None], ) -> int: """Upload workspace files to sandbox, returning the count uploaded.""" - _EXCLUDED = frozenset({ - "__pycache__", ".git", ".mypy_cache", ".pytest_cache", - ".ruff_cache", "node_modules", ".venv", "venv", - }) - uploaded = 0 for path in sorted(workspace_path.rglob("*")): - if any(part in _EXCLUDED for part in path.parts): + # The SAME set the credential scanner skips (#967) — a directory + # the scanner does not read must not be a directory we ship. + if any(part in EXCLUDED_DIRS for part in path.parts): continue if not path.is_file(): continue @@ -295,36 +322,33 @@ def _download_changed_files( Returns: Tuple of (list of relative file paths, count downloaded). """ + # -z: NUL-separated, so a filename containing " -> " (the rename + # separator in the default format) can no longer split a path in half. status_result = sbx.commands.run( - f"cd {_SANDBOX_WORKSPACE} && git status --porcelain", + f"cd {_SANDBOX_WORKSPACE} && git status --porcelain -z", timeout=30, ) if status_result.exit_code != 0 or not status_result.stdout.strip(): return [], 0 - changed: list[str] = [] - for line in status_result.stdout.splitlines(): - line = line.strip() - if not line: - continue - # porcelain format: XY filename (or "XY old -> new" for renames) - parts = line.split(None, 1) - if len(parts) < 2: - continue - xy, filepath = parts - # Handle renames: "R old -> new" — take the new name after " -> " - if " -> " in filepath: - filepath = filepath.split(" -> ", 1)[1] - changed.append(filepath.strip()) + changed, rejected = self._parse_porcelain(status_result.stdout) downloaded = 0 modified_files: list[str] = [] for rel_path in changed: - remote = f"{_SANDBOX_WORKSPACE}/{rel_path}" - local = workspace_path / rel_path + # Contain BEFORE the read and before any mkdir — a rejected path + # must cost nothing and create nothing (#967). + local = _safe_local_path(workspace_path, rel_path) + if local is None: + rejected += 1 + logger.warning( + "Rejected sandbox path outside the workspace: %r", rel_path + ) + continue + remote = f"{_SANDBOX_WORKSPACE}/{rel_path}" try: content = sbx.files.read(remote) local.parent.mkdir(parents=True, exist_ok=True) @@ -339,4 +363,51 @@ def _download_changed_files( logger.warning("Failed to download %s: %s", rel_path, exc) emit("progress", f"Downloaded {downloaded} changed file(s)") + if rejected: + # Counted and surfaced, never silently dropped: a rejection here + # means the sandbox tried to write outside the workspace. + emit( + "progress", + f"Rejected {rejected} path(s) outside the workspace — " + "the sandbox tried to write somewhere it may not", + ) return modified_files, downloaded + + @staticmethod + def _parse_porcelain(stdout: str) -> tuple[list[str], int]: + """Parse ``git status --porcelain -z`` into paths, hostile input assumed. + + ``-z`` is deliberate: it emits each path as raw bytes, so there is no + C-quoting to decode (``--porcelain`` alone would render a file named + ``café.txt`` as ``"caf\\303\\251.txt"``, quotes and all) and no + ``" -> "`` rename separator to be ambiguous with a filename that + contains that string. + + Returns: + Tuple of (paths, count rejected as unparseable). + """ + entries = [e for e in stdout.split("\0") if e] + paths: list[str] = [] + rejected = 0 + + index = 0 + while index < len(entries): + entry = entries[index] + index += 1 + # "XY PATH" — exactly two status characters and a space. + if len(entry) < 4 or entry[2] != " ": + continue + status, raw = entry[:2], entry[3:] + + # A rename/copy is "XY new\0old" — consume the old name, keep new. + if status[0] in ("R", "C") or status[1] in ("R", "C"): + index += 1 + + # Verbatim: -z output is NOT C-quoted (that is the whole point of + # the flag), so a file genuinely named `"a.py"` must keep its + # quotes. Decoding here would silently rewrite it to `a.py` and + # clobber a different file. Whatever the name turns out to be, + # _safe_local_path is what decides where it may land. + paths.append(raw) + + return paths, rejected diff --git a/codeframe/adapters/e2b/credential_scanner.py b/codeframe/adapters/e2b/credential_scanner.py index 088a5477..056db0ba 100644 --- a/codeframe/adapters/e2b/credential_scanner.py +++ b/codeframe/adapters/e2b/credential_scanner.py @@ -13,8 +13,17 @@ logger = logging.getLogger(__name__) -# Directories that are always excluded from scanning and upload counts -_EXCLUDED_DIRS = frozenset({ +# Directories skipped by BOTH the scanner and the uploader (#967). +# +# One constant, deliberately: these two used to disagree, and the uploader's +# smaller set was the wider one — the scanner skipped ``dist``/``build``/ +# ``.tox``/``.eggs`` while the uploader happily shipped them, so a ``.env`` or +# key baked into a build artifact reached the third-party sandbox unscanned. +# Anything listed here is never scanned, which means it must also never be +# uploaded. Only add entries that can hold no secret worth protecting. +# +# Build output is NOT excluded, precisely because it can contain one. +EXCLUDED_DIRS = frozenset({ "__pycache__", ".git", ".mypy_cache", @@ -23,10 +32,6 @@ "node_modules", ".venv", "venv", - ".tox", - "dist", - "build", - ".eggs", }) # High-risk filename/extension patterns (case-insensitive glob-style matching) @@ -83,7 +88,7 @@ def scan_path(root: Path) -> ScanResult: for path in sorted(root.rglob("*")): # Skip excluded directories - if any(part in _EXCLUDED_DIRS for part in path.parts): + if any(part in EXCLUDED_DIRS for part in path.parts): continue if not path.is_file(): diff --git a/tests/adapters/test_e2b_adapter.py b/tests/adapters/test_e2b_adapter.py index d9a8cf94..3b696ec7 100644 --- a/tests/adapters/test_e2b_adapter.py +++ b/tests/adapters/test_e2b_adapter.py @@ -269,8 +269,8 @@ def test_successful_execution_returns_completed(self, mock_create, tmp_path): # Adapter runs: git-combined (1 call), pip install, cf work start, git status diff_result = MagicMock() diff_result.exit_code = 0 - # git status --porcelain format: "XY filename" - diff_result.stdout = " M main.py\n" + # `git status --porcelain -z`: "XY filename" records, NUL-terminated (#967) + diff_result.stdout = " M main.py\x00" sbx.commands.run.side_effect = [ MagicMock(exit_code=0, stdout="", stderr=""), # git init+add+commit MagicMock(exit_code=0, stdout="installed", stderr=""), # pip install @@ -375,7 +375,7 @@ def test_new_files_downloaded_via_porcelain(self, mock_create, tmp_path): # porcelain: modified file + untracked new file status_result = MagicMock() status_result.exit_code = 0 - status_result.stdout = " M existing.py\n?? new_module.py\n" + status_result.stdout = " M existing.py\x00?? new_module.py\x00" sbx.commands.run.side_effect = [ MagicMock(exit_code=0, stdout="", stderr=""), # git combined MagicMock(exit_code=0, stdout="", stderr=""), # pip install diff --git a/tests/adapters/test_e2b_trust_boundary_967.py b/tests/adapters/test_e2b_trust_boundary_967.py new file mode 100644 index 00000000..b69650d3 --- /dev/null +++ b/tests/adapters/test_e2b_trust_boundary_967.py @@ -0,0 +1,313 @@ +"""The E2B sandbox is a trust boundary in both directions (issue #967). + +The coding agent has arbitrary command execution inside the sandbox before +``_download_changed_files`` runs, so the output of ``git status --porcelain`` +is attacker-controlled — the agent can shadow the git binary. Every path in it +must be treated as hostile input, and the containment check is the actual +security boundary rather than anything git promises. + +Outbound, the pre-upload credential scan and the uploader disagreed about which +directories to skip, so a secret baked into a build artifact was shipped to a +third-party sandbox unscanned — defeating the adapter's abort-on-secrets +contract. + +These tests are written against the local filesystem, not a real sandbox: the +question is only ever "what did we write, and where". +""" + +from __future__ import annotations + +from pathlib import Path +from unittest.mock import MagicMock + +import pytest + +pytestmark = pytest.mark.v2 + + +def _sbx(*entries: str, content: str | bytes = "pwned") -> MagicMock: + """A sandbox whose git reports *entries* and whose files all read back. + + Entries are given the way `git status --porcelain -z` emits them: one + ``XY PATH`` record per argument, NUL-terminated. + """ + sbx = MagicMock() + stdout = "".join(e + "\0" for e in entries) + sbx.commands.run.return_value = MagicMock(exit_code=0, stdout=stdout, stderr="") + sbx.files.read.return_value = content + return sbx + + +def _download(sbx: MagicMock, workspace: Path): + from codeframe.adapters.e2b.adapter import E2BAgentAdapter + + adapter = E2BAgentAdapter(timeout_minutes=5) + return adapter._download_changed_files(sbx, workspace, lambda *a, **k: None) + + +@pytest.fixture +def workspace(tmp_path: Path) -> Path: + ws = tmp_path / "ws" + ws.mkdir() + return ws + + +# ───────────────────────────────────────────────────────────────────────────── +# Containment: nothing lands outside the workspace +# ───────────────────────────────────────────────────────────────────────────── + + +class TestNothingEscapesTheWorkspace: + def test_dotdot_path_writes_nothing_outside(self, workspace, tmp_path): + """The canonical case from the issue.""" + outside = tmp_path / "evil" + _download(_sbx(" M ../evil"), workspace) + assert not outside.exists(), "wrote outside the workspace" + + def test_deep_dotdot_path_writes_nothing_outside(self, workspace, tmp_path): + _download(_sbx(" M a/../../../evil2"), workspace) + assert not (tmp_path.parent / "evil2").exists() + assert not (tmp_path / "evil2").exists() + + def test_absolute_path_is_rejected(self, workspace, tmp_path): + """`workspace / '/abs'` is `/abs` — pathlib discards the left side.""" + target = tmp_path / "absolute-pwned" + _download(_sbx(f" M {target}"), workspace) + assert not target.exists() + + def test_an_absolute_path_inside_the_workspace_is_still_rejected(self, workspace): + """The case the containment check alone does NOT catch. + + An absolute path that happens to resolve inside the workspace passes + the relative_to() test, but porcelain paths are always workspace- + relative — an absolute one means the output is not what we asked for, + and the remote read would be built as `/workspace//abs/path` anyway. + """ + target = workspace / "inside.py" + files, count = _download(_sbx(f" M {target}"), workspace) + assert count == 0, files + assert not target.exists() + + def test_home_relative_traversal_cannot_reach_ssh(self, workspace, tmp_path): + """The issue's worked example: ~/.ssh/authorized_keys.""" + fake_home = tmp_path / "home" + (fake_home / ".ssh").mkdir(parents=True) + rel = Path("..") / "home" / ".ssh" / "authorized_keys" + _download(_sbx(f" M {rel}"), workspace) + assert not (fake_home / ".ssh" / "authorized_keys").exists() + + def test_a_symlink_out_of_the_tree_is_not_a_way_out(self, workspace, tmp_path): + """Containment must resolve, not just string-check for '..'.""" + outside_dir = tmp_path / "outside" + outside_dir.mkdir() + (workspace / "link").symlink_to(outside_dir, target_is_directory=True) + + _download(_sbx(" M link/escaped.txt"), workspace) + assert not (outside_dir / "escaped.txt").exists() + + def test_no_directories_are_created_outside_either(self, workspace, tmp_path): + """mkdir(parents=True) runs before the write — it must not run at all.""" + _download(_sbx(" M ../made/up/dirs/file.txt"), workspace) + assert not (tmp_path / "made").exists() + + +# ───────────────────────────────────────────────────────────────────────────── +# The guard must not break the feature it protects +# ───────────────────────────────────────────────────────────────────────────── + + +class TestLegitimateFilesStillArrive: + def test_a_nested_file_downloads(self, workspace): + files, count = _download(_sbx(" M src/pkg/mod.py", content="real content"), workspace) + assert count == 1 + assert files == ["src/pkg/mod.py"] + assert (workspace / "src" / "pkg" / "mod.py").read_text() == "real content" + + def test_a_workspace_reached_through_a_symlink_is_not_a_false_reject( + self, tmp_path + ): + """Resolve BOTH sides, or a symlinked workspace rejects everything.""" + real = tmp_path / "real" + real.mkdir() + link = tmp_path / "link" + link.symlink_to(real, target_is_directory=True) + + files, count = _download(_sbx(" M ok.py", content="x"), link) + assert count == 1, "a symlinked workspace root rejected a legitimate file" + assert (real / "ok.py").read_text() == "x" + + def test_good_files_survive_alongside_a_rejected_one(self, workspace, tmp_path): + files, count = _download(_sbx(" M ../evil3", "M good.py", content="x"), workspace) + assert not (tmp_path / "evil3").exists() + assert files == ["good.py"] + assert count == 1 + + +# ───────────────────────────────────────────────────────────────────────────── +# Rejections are visible (AC2) +# ───────────────────────────────────────────────────────────────────────────── + + +class TestRejectionsAreWarnedAndCounted: + def test_a_rejected_path_is_logged_as_a_warning(self, workspace, caplog): + import logging + + with caplog.at_level(logging.WARNING): + _download(_sbx(" M ../evil4"), workspace) + + assert any("evil4" in r.getMessage() for r in caplog.records), caplog.text + assert "workspace" in caplog.text.lower() or "outside" in caplog.text.lower() + + def test_a_rejected_path_is_counted(self, workspace): + from codeframe.adapters.e2b.adapter import E2BAgentAdapter + + adapter = E2BAgentAdapter(timeout_minutes=5) + emitted: list[tuple] = [] + adapter._download_changed_files( + _sbx(" M ../evil5", " M ../evil6", " M ok.py"), + workspace, + lambda *a, **k: emitted.append(a), + ) + blob = " ".join(str(a) for a in emitted).lower() + assert "2" in blob and ("reject" in blob or "outside" in blob), emitted + + def test_a_rejected_path_is_never_read_from_the_sandbox(self, workspace): + """Reject before the read, not after — no needless round trip.""" + sbx = _sbx(" M ../evil7") + _download(sbx, workspace) + assert sbx.files.read.call_count == 0 + + +# ───────────────────────────────────────────────────────────────────────────── +# Hostile porcelain parsing (AC3) +# ───────────────────────────────────────────────────────────────────────────── + + +class TestPorcelainParsing: + def test_a_literally_quoted_filename_keeps_its_quotes(self, workspace): + """-z emits paths verbatim, so quotes in a name are part of the name. + + Verified against real git: a file named `"quoted".py` comes back as + `?? "quoted".py\\0` under -z, and as `?? "\\"quoted\\".py"` without it. + Decoding the -z form would rewrite `"a.py"` to `a.py` and clobber a + different file. + """ + files, count = _download(_sbx(' M "a.py"', content="x"), workspace) + assert files == ['"a.py"'] + assert (workspace / '"a.py"').exists() + assert not (workspace / "a.py").exists(), "stripped quotes that were real" + + def test_a_utf8_name_arrives_verbatim(self, workspace): + """No octal escaping under -z — the bytes are the name.""" + files, count = _download(_sbx(" M café.txt", content="x"), workspace) + assert files == ["café.txt"] + assert (workspace / "café.txt").exists() + + def test_a_name_containing_a_tab_survives(self, workspace): + """--porcelain would have quoted this one; -z does not.""" + files, _ = _download(_sbx(" M tab\tname.txt", content="x"), workspace) + assert files == ["tab\tname.txt"] + + def test_a_quoted_traversal_is_still_contained(self, workspace, tmp_path): + """Decoding must not be a way around the containment check.""" + _download(_sbx(' M "../evil8"'), workspace) + assert not (tmp_path / "evil8").exists() + + def test_a_bizarre_name_is_contained_rather_than_interpreted(self, workspace): + """Hostile input is not guessed at — it is just kept inside the tree.""" + _download(_sbx(' M "bad\\777\\777"', content="x"), workspace) + written = [p for p in workspace.rglob("*") if p.is_file()] + assert len(written) == 1 + assert workspace.resolve() in written[0].resolve().parents + + def test_a_rename_keeps_the_new_name_only(self, workspace): + from codeframe.adapters.e2b.adapter import E2BAgentAdapter + + adapter = E2BAgentAdapter(timeout_minutes=5) + sbx = _sbx(content="x") + # -z renames are "XY new\0old\0" + sbx.commands.run.return_value = MagicMock( + exit_code=0, stdout="R new.py\x00old.py\x00", stderr="" + ) + files, _ = adapter._download_changed_files(sbx, workspace, lambda *a, **k: None) + assert files == ["new.py"] + assert not (workspace / "old.py").exists() + + def test_a_filename_containing_the_rename_arrow_is_not_mangled(self, workspace): + """' -> ' inside a name used to split the path in half.""" + from codeframe.adapters.e2b.adapter import E2BAgentAdapter + + adapter = E2BAgentAdapter(timeout_minutes=5) + sbx = _sbx(content="x") + sbx.commands.run.return_value = MagicMock( + exit_code=0, stdout=" M a -> b.py\x00", stderr="" + ) + files, _ = adapter._download_changed_files(sbx, workspace, lambda *a, **k: None) + assert files == ["a -> b.py"] + + def test_porcelain_is_requested_nul_separated(self, workspace): + """-z is what removes the separator ambiguity above.""" + sbx = _sbx(" M ok.py", content="x") + _download(sbx, workspace) + command = sbx.commands.run.call_args[0][0] + assert "-z" in command, command + + +# ───────────────────────────────────────────────────────────────────────────── +# Outbound: one exclusion set (AC5) +# ───────────────────────────────────────────────────────────────────────────── + + +class TestUploaderAndScannerAgree: + def test_they_are_literally_the_same_constant(self): + from codeframe.adapters.e2b import adapter as adapter_mod + from codeframe.adapters.e2b.credential_scanner import EXCLUDED_DIRS + + assert adapter_mod.EXCLUDED_DIRS is EXCLUDED_DIRS + + @pytest.mark.parametrize("directory", [".tox", "dist", "build", ".eggs"]) + def test_build_output_is_scanned_not_skipped(self, directory): + """A skipped directory is an unscanned upload — the whole bug.""" + from codeframe.adapters.e2b.credential_scanner import EXCLUDED_DIRS + + assert directory not in EXCLUDED_DIRS + + def test_a_secret_under_dist_makes_the_scan_dirty(self, tmp_path): + from codeframe.adapters.e2b.credential_scanner import scan_path + + (tmp_path / "dist").mkdir() + (tmp_path / "dist" / ".env").write_text("SECRET=leaked") + + result = scan_path(tmp_path) + assert not result.is_clean + assert any("dist" in b for b in result.blocked_files), result.blocked_files + + def test_a_secret_under_dist_aborts_the_run(self, tmp_path): + """AC5, end to end: the adapter's abort-on-secrets contract holds.""" + import os + from unittest.mock import patch + + from codeframe.adapters.e2b.adapter import E2BAgentAdapter + + (tmp_path / "main.py").write_text("print(1)\n") + (tmp_path / "dist").mkdir() + (tmp_path / "dist" / "bundle.env").write_text("AKIA" + "IOSFODNN7EXAMPLE\n") # split: pre-commit secret scan + + adapter = E2BAgentAdapter(timeout_minutes=5) + with patch.dict(os.environ, {"E2B_API_KEY": "test-key"}): + with patch("e2b.Sandbox.create") as create: + result = adapter.run( + task_id="t-1", prompt="p", workspace_path=tmp_path + ) + assert create.call_count == 0, "created a sandbox despite a secret" + + assert result.status == "failed" + assert "credential" in (result.error or "").lower() + assert result.cloud_metadata["credential_scan_blocked"] == 1 + + def test_the_real_junk_dirs_are_still_skipped(self): + """Sharing the constant must not start uploading .git and node_modules.""" + from codeframe.adapters.e2b.credential_scanner import EXCLUDED_DIRS + + for directory in ("__pycache__", ".git", "node_modules", ".venv"): + assert directory in EXCLUDED_DIRS From ff01dbf6a4dc900f60e2b461d08867450c3479b4 Mon Sep 17 00:00:00 2001 From: Frank Bria <136862992+frankbria@users.noreply.github.com> Date: Fri, 7 Aug 2026 23:28:14 -0700 Subject: [PATCH 2/7] fix(e2b): count and warn on malformed porcelain records (#967) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both reviewers caught the same leftover: `_parse_porcelain`'s `rejected` counter was never incremented. Deleting the C-unquoter removed the only thing that incremented it, and I left the variable and its docstring claim behind — so a malformed record was silently dropped, contradicting this PR's own "never silently dropped" framing for AC2. Real git cannot emit a record that isn't "XY PATH", so a malformed one means the sandbox's git is not git — precisely the case worth surfacing. Now warned, counted, and folded into the count emitted to the user alongside containment rejections. --- codeframe/adapters/e2b/adapter.py | 4 +++ tests/adapters/test_e2b_trust_boundary_967.py | 31 +++++++++++++++++++ 2 files changed, 35 insertions(+) diff --git a/codeframe/adapters/e2b/adapter.py b/codeframe/adapters/e2b/adapter.py index c304ef15..8b788bfe 100644 --- a/codeframe/adapters/e2b/adapter.py +++ b/codeframe/adapters/e2b/adapter.py @@ -396,6 +396,10 @@ def _parse_porcelain(stdout: str) -> tuple[list[str], int]: index += 1 # "XY PATH" — exactly two status characters and a space. if len(entry) < 4 or entry[2] != " ": + # Counted and warned, not dropped: real git cannot emit this, + # so a malformed record means the sandbox's git is not git. + rejected += 1 + logger.warning("Rejected malformed porcelain record: %r", entry) continue status, raw = entry[:2], entry[3:] diff --git a/tests/adapters/test_e2b_trust_boundary_967.py b/tests/adapters/test_e2b_trust_boundary_967.py index b69650d3..a4d700ac 100644 --- a/tests/adapters/test_e2b_trust_boundary_967.py +++ b/tests/adapters/test_e2b_trust_boundary_967.py @@ -245,6 +245,37 @@ def test_a_filename_containing_the_rename_arrow_is_not_mangled(self, workspace): files, _ = adapter._download_changed_files(sbx, workspace, lambda *a, **k: None) assert files == ["a -> b.py"] + def test_a_malformed_record_is_counted_and_warned_not_dropped(self, workspace, caplog): + """AC2 applies to parse rejections too, not just containment ones.""" + import logging + + with caplog.at_level(logging.WARNING): + files, count = _download(_sbx("xx", " M ok.py", content="x"), workspace) + + assert files == ["ok.py"] + assert count == 1 + assert any("xx" in r.getMessage() for r in caplog.records), caplog.text + + def test_the_parse_reject_count_reaches_the_user(self, workspace): + from codeframe.adapters.e2b.adapter import E2BAgentAdapter + + emitted: list[str] = [] + E2BAgentAdapter(timeout_minutes=5)._download_changed_files( + _sbx("xx", "y", " M ../outside", content="x"), + workspace, + lambda kind, msg, *a: emitted.append(msg), + ) + # 2 unparseable + 1 escaping the workspace + assert any("3" in m and "reject" in m.lower() for m in emitted), emitted + + def test_parse_rejects_are_reported_separately_from_containment(self): + """_parse_porcelain's own return value must mean something.""" + from codeframe.adapters.e2b.adapter import E2BAgentAdapter + + paths, rejected = E2BAgentAdapter._parse_porcelain("xx\0 M ok.py\0y\0") + assert paths == ["ok.py"] + assert rejected == 2 + def test_porcelain_is_requested_nul_separated(self, workspace): """-z is what removes the separator ambiguity above.""" sbx = _sbx(" M ok.py", content="x") From 1ee6c222d919a6af4a7a8d5230877cde1ac7e6bc Mon Sep 17 00:00:00 2001 From: Frank Bria <136862992+frankbria@users.noreply.github.com> Date: Fri, 7 Aug 2026 23:48:15 -0700 Subject: [PATCH 3/7] fix(e2b): a faked rename header can no longer swallow the next record (#967) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit claude-review: under this PR's own threat model, a shadowed git can emit `R fake.py\0 M real_change.py\0` — the parser accepted the rename header and unconditionally ate the following well-formed entry as its "old path", so a real change disappeared with no log and no count. Not an escape (nothing is written outside the workspace, and an attacker who controls the output could simply omit the entry instead), but it is a silent drop under the invariant this PR just established for AC2. Honest git always follows a rename header with a bare path, so a field that itself looks like `XY PATH` was never half of a rename pair. The consumption is now conditional on that and warns when it declines. The honest case is pinned separately so the guard cannot start treating every rename's old name as a download. --- codeframe/adapters/e2b/adapter.py | 20 +++++++++++--- tests/adapters/test_e2b_trust_boundary_967.py | 27 +++++++++++++++++++ 2 files changed, 44 insertions(+), 3 deletions(-) diff --git a/codeframe/adapters/e2b/adapter.py b/codeframe/adapters/e2b/adapter.py index 8b788bfe..6e8d1511 100644 --- a/codeframe/adapters/e2b/adapter.py +++ b/codeframe/adapters/e2b/adapter.py @@ -386,6 +386,10 @@ def _parse_porcelain(stdout: str) -> tuple[list[str], int]: Returns: Tuple of (paths, count rejected as unparseable). """ + def _looks_like_record(entry: str) -> bool: + # "XY PATH" — exactly two status characters and a space. + return len(entry) >= 4 and entry[2] == " " + entries = [e for e in stdout.split("\0") if e] paths: list[str] = [] rejected = 0 @@ -394,8 +398,7 @@ def _parse_porcelain(stdout: str) -> tuple[list[str], int]: while index < len(entries): entry = entries[index] index += 1 - # "XY PATH" — exactly two status characters and a space. - if len(entry) < 4 or entry[2] != " ": + if not _looks_like_record(entry): # Counted and warned, not dropped: real git cannot emit this, # so a malformed record means the sandbox's git is not git. rejected += 1 @@ -404,8 +407,19 @@ def _parse_porcelain(stdout: str) -> tuple[list[str], int]: status, raw = entry[:2], entry[3:] # A rename/copy is "XY new\0old" — consume the old name, keep new. + # Honest git always follows the header with a bare path, so a + # field that itself looks like a record was never a rename pair: + # a shadowed git could otherwise fake a rename header purely to + # make this parser eat the next real entry, dropping it silently. if status[0] in ("R", "C") or status[1] in ("R", "C"): - index += 1 + if index < len(entries) and not _looks_like_record(entries[index]): + index += 1 + else: + logger.warning( + "Rename record %r is not followed by an old path; " + "not consuming the next entry", + entry, + ) # Verbatim: -z output is NOT C-quoted (that is the whole point of # the flag), so a file genuinely named `"a.py"` must keep its diff --git a/tests/adapters/test_e2b_trust_boundary_967.py b/tests/adapters/test_e2b_trust_boundary_967.py index a4d700ac..336b306a 100644 --- a/tests/adapters/test_e2b_trust_boundary_967.py +++ b/tests/adapters/test_e2b_trust_boundary_967.py @@ -276,6 +276,33 @@ def test_parse_rejects_are_reported_separately_from_containment(self): assert paths == ["ok.py"] assert rejected == 2 + def test_a_faked_rename_cannot_swallow_the_next_real_record(self, workspace): + """A shadowed git can emit a rename header to eat the following entry. + + Honest git always follows `R new` with a bare old path, so a field + that itself looks like `XY PATH` was never a rename pair. + """ + from codeframe.adapters.e2b.adapter import E2BAgentAdapter + + paths, _ = E2BAgentAdapter._parse_porcelain("R fake.py\0 M real_change.py\0") + assert "real_change.py" in paths, paths + + def test_an_honest_rename_still_consumes_its_old_path(self, workspace): + """The guard must not turn every rename's old name into a download.""" + from codeframe.adapters.e2b.adapter import E2BAgentAdapter + + paths, _ = E2BAgentAdapter._parse_porcelain("R new.py\0old.py\0 M other.py\0") + assert paths == ["new.py", "other.py"] + + def test_a_swallowed_record_is_at_least_warned_about(self, workspace, caplog): + import logging + + from codeframe.adapters.e2b.adapter import E2BAgentAdapter + + with caplog.at_level(logging.WARNING): + E2BAgentAdapter._parse_porcelain("R fake.py\0 M real_change.py\0") + assert "rename" in caplog.text.lower(), caplog.text + def test_porcelain_is_requested_nul_separated(self, workspace): """-z is what removes the separator ambiguity above.""" sbx = _sbx(" M ok.py", content="x") From dcc86f19be78b4ab700914a564d8b6ec104356a6 Mon Sep 17 00:00:00 2001 From: Frank Bria <136862992+frankbria@users.noreply.github.com> Date: Sat, 8 Aug 2026 00:08:05 -0700 Subject: [PATCH 4/7] fix(e2b): match the porcelain status alphabet, not just the shape (#967) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit claude-review edge case: `_looks_like_record` tested only "space at index 2", so an honest rename whose OLD name happens to fit that shape — e.g. `v1 notes.txt` — was refused as the rename's old path and then re-parsed as a record with status `v1`, producing a spurious warning and a failed download of `notes.txt`. No data loss (the new name was still captured), but it read as an oversight next to how deliberately every other parsing edge case here is pinned. Real XY status characters come from a small alphabet, which disambiguates the two cases outright rather than documenting the collision. Checking it also tightens the top-level malformed-record test: `v1 notes.txt` standing alone is now correctly rejected instead of parsed as status `v1`. The adversarial case is unaffected — ` M real_change.py` still reads as a record, so a faked rename header still cannot swallow it. --- codeframe/adapters/e2b/adapter.py | 17 ++++++++-- tests/adapters/test_e2b_trust_boundary_967.py | 31 +++++++++++++++++++ 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/codeframe/adapters/e2b/adapter.py b/codeframe/adapters/e2b/adapter.py index 6e8d1511..02f334c9 100644 --- a/codeframe/adapters/e2b/adapter.py +++ b/codeframe/adapters/e2b/adapter.py @@ -36,6 +36,10 @@ +#: The XY status characters `git status --porcelain` can emit. Used to tell a +#: real status record from a bare path that merely has a space at index 2. +_PORCELAIN_STATUS_CHARS = frozenset(" MADRCUT?!") + def _safe_local_path(workspace_root: Path, rel_path: str) -> Path | None: """Resolve *rel_path* inside *workspace_root*, or return None to reject. @@ -387,8 +391,17 @@ def _parse_porcelain(stdout: str) -> tuple[list[str], int]: Tuple of (paths, count rejected as unparseable). """ def _looks_like_record(entry: str) -> bool: - # "XY PATH" — exactly two status characters and a space. - return len(entry) >= 4 and entry[2] == " " + # "XY PATH" — two status characters then a space. The status + # alphabet matters, not just the shape: a rename whose OLD name is + # something like "v1 notes.txt" also has a space at index 2, and a + # pure shape check would refuse to consume it as the old path and + # then re-parse it as a record with status "v1". + return ( + len(entry) >= 4 + and entry[2] == " " + and entry[0] in _PORCELAIN_STATUS_CHARS + and entry[1] in _PORCELAIN_STATUS_CHARS + ) entries = [e for e in stdout.split("\0") if e] paths: list[str] = [] diff --git a/tests/adapters/test_e2b_trust_boundary_967.py b/tests/adapters/test_e2b_trust_boundary_967.py index 336b306a..663a47cf 100644 --- a/tests/adapters/test_e2b_trust_boundary_967.py +++ b/tests/adapters/test_e2b_trust_boundary_967.py @@ -303,6 +303,37 @@ def test_a_swallowed_record_is_at_least_warned_about(self, workspace, caplog): E2BAgentAdapter._parse_porcelain("R fake.py\0 M real_change.py\0") assert "rename" in caplog.text.lower(), caplog.text + def test_an_old_path_that_looks_shaped_like_a_record_is_still_consumed(self): + """Renaming `v1 notes.txt` must not confuse the rename heuristic. + + Its third character is a space, so a pure shape check would decline to + consume it and then re-parse it as a bogus record with status `v1`. + Real status characters come from a small alphabet, which disambiguates. + """ + from codeframe.adapters.e2b.adapter import E2BAgentAdapter + + paths, rejected = E2BAgentAdapter._parse_porcelain( + "R final.py\0v1 notes.txt\0 M other.py\0" + ) + assert paths == ["final.py", "other.py"], paths + assert rejected == 0 + + @pytest.mark.parametrize("entry", ["v1 notes.txt", "hello world", "ab cd"]) + def test_a_non_status_prefix_is_not_a_record(self, entry): + from codeframe.adapters.e2b.adapter import E2BAgentAdapter + + paths, rejected = E2BAgentAdapter._parse_porcelain(entry + "\0") + assert paths == [] + assert rejected == 1 + + @pytest.mark.parametrize("status", [" M", "M ", "??", "A ", " D", "R ", "!!", "UU"]) + def test_real_status_pairs_are_recognised(self, status): + from codeframe.adapters.e2b.adapter import E2BAgentAdapter + + paths, rejected = E2BAgentAdapter._parse_porcelain(f"{status} f.py\0") + assert rejected == 0 + assert paths == ["f.py"] + def test_porcelain_is_requested_nul_separated(self, workspace): """-z is what removes the separator ambiguity above.""" sbx = _sbx(" M ok.py", content="x") From cd1a4eee45a89d805abf82db07d4cd0af31175b0 Mon Sep 17 00:00:00 2001 From: Frank Bria <136862992+frankbria@users.noreply.github.com> Date: Sat, 8 Aug 2026 00:29:18 -0700 Subject: [PATCH 5/7] fix(e2b): an unresolvable path rejects one entry, not the whole download (#967) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit claude-review flagged this as speculative; it is real, and worse than it looked. `Path.resolve()` is not total — a symlink loop raises RuntimeError on Python 3.11 and 3.12, both inside this project's `requires-python`. Since `_safe_local_path` was called outside the per-file try/except, one loop anywhere under the workspace aborted the entire download loop instead of rejecting one path. It did not reproduce locally because 3.13 resolves loops quietly, so a real-symlink test passes on a 3.13 dev box while the crash stays live for most users. The test forces the raise instead, pinning the handling on every supported version. An unresolvable path is precisely one we must not write to, so rejecting (warned, like every other rejection here) is the right answer rather than propagating. Also collapses the stray blank lines around _PORCELAIN_STATUS_CHARS. --- codeframe/adapters/e2b/adapter.py | 15 ++++++++---- tests/adapters/test_e2b_trust_boundary_967.py | 24 +++++++++++++++++++ 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/codeframe/adapters/e2b/adapter.py b/codeframe/adapters/e2b/adapter.py index 02f334c9..1bf24d2e 100644 --- a/codeframe/adapters/e2b/adapter.py +++ b/codeframe/adapters/e2b/adapter.py @@ -34,12 +34,11 @@ _INSTALL_CMD = "pip install codeframe --quiet" - - #: The XY status characters `git status --porcelain` can emit. Used to tell a #: real status record from a bare path that merely has a space at index 2. _PORCELAIN_STATUS_CHARS = frozenset(" MADRCUT?!") + def _safe_local_path(workspace_root: Path, rel_path: str) -> Path | None: """Resolve *rel_path* inside *workspace_root*, or return None to reject. @@ -60,8 +59,16 @@ def _safe_local_path(workspace_root: Path, rel_path: str) -> Path | None: if not rel_path or PurePosixPath(rel_path).is_absolute() or Path(rel_path).is_absolute(): return None - root = workspace_root.resolve() - candidate = (root / rel_path).resolve() + try: + root = workspace_root.resolve() + candidate = (root / rel_path).resolve() + except (OSError, RuntimeError, ValueError) as exc: + # resolve() is not total: a symlink loop raises RuntimeError on Python + # 3.11/3.12 (3.13 resolves it quietly), and a bad path can raise + # OSError. Reject this one entry rather than aborting the whole + # download — an unresolvable path is exactly one we must not write to. + logger.warning("Could not resolve sandbox path %r: %s", rel_path, exc) + return None if candidate == root or root not in candidate.parents: return None diff --git a/tests/adapters/test_e2b_trust_boundary_967.py b/tests/adapters/test_e2b_trust_boundary_967.py index 663a47cf..e846b904 100644 --- a/tests/adapters/test_e2b_trust_boundary_967.py +++ b/tests/adapters/test_e2b_trust_boundary_967.py @@ -105,6 +105,30 @@ def test_a_symlink_out_of_the_tree_is_not_a_way_out(self, workspace, tmp_path): _download(_sbx(" M link/escaped.txt"), workspace) assert not (outside_dir / "escaped.txt").exists() + def test_a_failing_resolve_rejects_one_entry_not_the_whole_download( + self, workspace, monkeypatch + ): + """`Path.resolve()` can raise, and one bad entry must not abort the run. + + Version-dependent in the wild: a symlink loop raises RuntimeError on + Python 3.11/3.12 (both supported here) but resolves quietly on 3.13, so + a real-symlink test would silently pass on a 3.13 dev box while the + crash stays live for most users. Forcing the raise pins the handling on + every version. + """ + real_resolve = Path.resolve + + def exploding_resolve(self, *args, **kwargs): + if self.name == "boom": + raise RuntimeError(f"Symlink loop from {self}") + return real_resolve(self, *args, **kwargs) + + monkeypatch.setattr(Path, "resolve", exploding_resolve) + + files, count = _download(_sbx(" M boom", " M good.py", content="x"), workspace) + assert files == ["good.py"], files + assert count == 1 + def test_no_directories_are_created_outside_either(self, workspace, tmp_path): """mkdir(parents=True) runs before the write — it must not run at all.""" _download(_sbx(" M ../made/up/dirs/file.txt"), workspace) From edf8e59459ac120a7fdc28e765b7b64a3133a37c Mon Sep 17 00:00:00 2001 From: Frank Bria <136862992+frankbria@users.noreply.github.com> Date: Sat, 8 Aug 2026 00:51:02 -0700 Subject: [PATCH 6/7] fix(e2b): --no-renames removes the paired-field ambiguity entirely (#967) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit claude-review was right that the status-alphabet fix narrowed the rename-old-path misparse without closing it, and right that my last comment overstated it. `AD HOC.txt` is a plausible filename whose first two characters are both real status characters followed by a space, so the lookahead heuristic still misread it as a record. Rather than narrow the heuristic again, remove the thing it was guessing about. `git status --porcelain -z --no-renames` reports a rename as an independent delete + add — verified against real git: -z R new.py\0AD HOC.txt\0 -z --no-renames D AD HOC.txt\0A new.py\0 There is no paired field, so there is no lookahead, so both failure directions disappear at once: a hostile `R` header cannot swallow the record after it, and no old filename can be misread as a record. Every entry is a record. The rename branch and its heuristic are deleted. The delete half of that pair would otherwise be fetched and fail, so deleted paths are now skipped — they are not in the sandbox to read, and trying only produced a misleading "Failed to download" for a file that is meant to be gone. Applying deletions locally remains the parked #966 defect; this only stops us fetching a path we know is absent. --- codeframe/adapters/e2b/adapter.py | 50 +++++++-------- tests/adapters/test_e2b_trust_boundary_967.py | 64 ++++++++++--------- 2 files changed, 58 insertions(+), 56 deletions(-) diff --git a/codeframe/adapters/e2b/adapter.py b/codeframe/adapters/e2b/adapter.py index 1bf24d2e..55e52433 100644 --- a/codeframe/adapters/e2b/adapter.py +++ b/codeframe/adapters/e2b/adapter.py @@ -333,10 +333,8 @@ def _download_changed_files( Returns: Tuple of (list of relative file paths, count downloaded). """ - # -z: NUL-separated, so a filename containing " -> " (the rename - # separator in the default format) can no longer split a path in half. status_result = sbx.commands.run( - f"cd {_SANDBOX_WORKSPACE} && git status --porcelain -z", + f"cd {_SANDBOX_WORKSPACE} && git status --porcelain -z --no-renames", timeout=30, ) @@ -388,21 +386,26 @@ def _download_changed_files( def _parse_porcelain(stdout: str) -> tuple[list[str], int]: """Parse ``git status --porcelain -z`` into paths, hostile input assumed. - ``-z`` is deliberate: it emits each path as raw bytes, so there is no - C-quoting to decode (``--porcelain`` alone would render a file named - ``café.txt`` as ``"caf\\303\\251.txt"``, quotes and all) and no - ``" -> "`` rename separator to be ambiguous with a filename that - contains that string. + Two flags carry the weight here, both verified against real git: + + ``-z`` emits each path as raw bytes, so there is no C-quoting to decode + (``--porcelain`` alone renders ``café.txt`` as ``"caf\\303\\251.txt"``, + quotes and all) and no ``" -> "`` rename separator to collide with a + filename containing that string. + + ``--no-renames`` reports a rename as an independent delete + add + instead of ``R new\\0old\\0``. That removes the paired field entirely, + and with it a whole class of ambiguity: there is no lookahead to guess + at, so a hostile ``R`` header cannot make the parser swallow the record + after it, and an old filename shaped like a status record (``AD + HOC.txt``, ``v1 notes.txt``) cannot be misread as one. Every entry is a + record. Returns: Tuple of (paths, count rejected as unparseable). """ def _looks_like_record(entry: str) -> bool: - # "XY PATH" — two status characters then a space. The status - # alphabet matters, not just the shape: a rename whose OLD name is - # something like "v1 notes.txt" also has a space at index 2, and a - # pure shape check would refuse to consume it as the old path and - # then re-parse it as a record with status "v1". + # "XY PATH" — two status characters then a space. return ( len(entry) >= 4 and entry[2] == " " @@ -426,20 +429,13 @@ def _looks_like_record(entry: str) -> bool: continue status, raw = entry[:2], entry[3:] - # A rename/copy is "XY new\0old" — consume the old name, keep new. - # Honest git always follows the header with a bare path, so a - # field that itself looks like a record was never a rename pair: - # a shadowed git could otherwise fake a rename header purely to - # make this parser eat the next real entry, dropping it silently. - if status[0] in ("R", "C") or status[1] in ("R", "C"): - if index < len(entries) and not _looks_like_record(entries[index]): - index += 1 - else: - logger.warning( - "Rename record %r is not followed by an old path; " - "not consuming the next entry", - entry, - ) + # A deleted file is not in the sandbox to read. Skipping it avoids + # a misleading "Failed to download" for a file that is meant to be + # gone. Applying the deletion locally is a separate, parked defect + # (#966) — this only stops us fetching a path we know is absent. + if "D" in status: + logger.debug("Skipping deleted path: %r", raw) + continue # Verbatim: -z output is NOT C-quoted (that is the whole point of # the flag), so a file genuinely named `"a.py"` must keep its diff --git a/tests/adapters/test_e2b_trust_boundary_967.py b/tests/adapters/test_e2b_trust_boundary_967.py index e846b904..432fda2b 100644 --- a/tests/adapters/test_e2b_trust_boundary_967.py +++ b/tests/adapters/test_e2b_trust_boundary_967.py @@ -300,48 +300,48 @@ def test_parse_rejects_are_reported_separately_from_containment(self): assert paths == ["ok.py"] assert rejected == 2 - def test_a_faked_rename_cannot_swallow_the_next_real_record(self, workspace): - """A shadowed git can emit a rename header to eat the following entry. + def test_renames_are_disabled_so_there_is_no_paired_field(self, workspace): + """--no-renames is what removes the whole ambiguity class. - Honest git always follows `R new` with a bare old path, so a field - that itself looks like `XY PATH` was never a rename pair. + Verified against real git: renaming `AD HOC.txt` to `new.py` gives + `R new.py\\0AD HOC.txt\\0` normally, but `D AD HOC.txt\\0A new.py\\0` + with --no-renames — two independent records and no field to consume. """ - from codeframe.adapters.e2b.adapter import E2BAgentAdapter - - paths, _ = E2BAgentAdapter._parse_porcelain("R fake.py\0 M real_change.py\0") - assert "real_change.py" in paths, paths - - def test_an_honest_rename_still_consumes_its_old_path(self, workspace): - """The guard must not turn every rename's old name into a download.""" - from codeframe.adapters.e2b.adapter import E2BAgentAdapter - - paths, _ = E2BAgentAdapter._parse_porcelain("R new.py\0old.py\0 M other.py\0") - assert paths == ["new.py", "other.py"] - - def test_a_swallowed_record_is_at_least_warned_about(self, workspace, caplog): - import logging + sbx = _sbx(" M ok.py", content="x") + _download(sbx, workspace) + command = sbx.commands.run.call_args[0][0] + assert "--no-renames" in command, command + def test_every_entry_is_a_record_so_nothing_can_be_swallowed(self): + """A hostile `R` header can no longer eat the record after it.""" from codeframe.adapters.e2b.adapter import E2BAgentAdapter - with caplog.at_level(logging.WARNING): - E2BAgentAdapter._parse_porcelain("R fake.py\0 M real_change.py\0") - assert "rename" in caplog.text.lower(), caplog.text + paths, _ = E2BAgentAdapter._parse_porcelain("R fake.py\0 M real_change.py\0") + assert paths == ["fake.py", "real_change.py"], paths - def test_an_old_path_that_looks_shaped_like_a_record_is_still_consumed(self): - """Renaming `v1 notes.txt` must not confuse the rename heuristic. + def test_an_odd_old_filename_can_no_longer_be_misparsed(self): + """`AD HOC.txt` and `v1 notes.txt` are just paths on their own records. - Its third character is a space, so a pure shape check would decline to - consume it and then re-parse it as a bogus record with status `v1`. - Real status characters come from a small alphabet, which disambiguates. + Both used to be misparsed by the lookahead heuristic — the first + because `A`/`D` are real status characters, the second because index 2 + is a space. With no lookahead there is nothing left to guess. """ from codeframe.adapters.e2b.adapter import E2BAgentAdapter paths, rejected = E2BAgentAdapter._parse_porcelain( - "R final.py\0v1 notes.txt\0 M other.py\0" + "A AD HOC.txt\0?? v1 notes.txt\0" ) - assert paths == ["final.py", "other.py"], paths + assert paths == ["AD HOC.txt", "v1 notes.txt"], paths assert rejected == 0 + @pytest.mark.parametrize("status", ["D ", " D", "AD", "MD"]) + def test_a_deleted_file_is_not_fetched(self, workspace, status): + """It is not in the sandbox to read — trying only logs a false failure.""" + from codeframe.adapters.e2b.adapter import E2BAgentAdapter + + paths, _ = E2BAgentAdapter._parse_porcelain(f"{status} gone.py\0 M kept.py\0") + assert paths == ["kept.py"], paths + @pytest.mark.parametrize("entry", ["v1 notes.txt", "hello world", "ab cd"]) def test_a_non_status_prefix_is_not_a_record(self, entry): from codeframe.adapters.e2b.adapter import E2BAgentAdapter @@ -352,11 +352,17 @@ def test_a_non_status_prefix_is_not_a_record(self, entry): @pytest.mark.parametrize("status", [" M", "M ", "??", "A ", " D", "R ", "!!", "UU"]) def test_real_status_pairs_are_recognised(self, status): + """Tightening the alphabet must not start rejecting valid records. + + Recognition, not download: a `D` pair is a valid record that is then + deliberately skipped (the file is gone), which the deletion test + covers. + """ from codeframe.adapters.e2b.adapter import E2BAgentAdapter paths, rejected = E2BAgentAdapter._parse_porcelain(f"{status} f.py\0") assert rejected == 0 - assert paths == ["f.py"] + assert paths == ([] if "D" in status else ["f.py"]) def test_porcelain_is_requested_nul_separated(self, workspace): """-z is what removes the separator ambiguity above.""" From 7ef3ed31579b0c334121f1865526da82e55648b4 Mon Sep 17 00:00:00 2001 From: Frank Bria <136862992+frankbria@users.noreply.github.com> Date: Sat, 8 Aug 2026 00:51:49 -0700 Subject: [PATCH 7/7] fix(e2b): the reject count message now matches what it counts (#967) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Spotted while re-running the demo: the emitted line said "Rejected N path(s) outside the workspace — the sandbox tried to write somewhere it may not", but that count folds in unparseable records too, which are a different thing and not necessarily an attempted escape. Reworded to name both causes and point at the per-path log lines. --- codeframe/adapters/e2b/adapter.py | 4 ++-- tests/adapters/test_e2b_trust_boundary_967.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/codeframe/adapters/e2b/adapter.py b/codeframe/adapters/e2b/adapter.py index 55e52433..28a2ffcf 100644 --- a/codeframe/adapters/e2b/adapter.py +++ b/codeframe/adapters/e2b/adapter.py @@ -377,8 +377,8 @@ def _download_changed_files( # means the sandbox tried to write outside the workspace. emit( "progress", - f"Rejected {rejected} path(s) outside the workspace — " - "the sandbox tried to write somewhere it may not", + f"Rejected {rejected} sandbox path(s) — unparseable, or " + "outside the workspace (see log for each)", ) return modified_files, downloaded diff --git a/tests/adapters/test_e2b_trust_boundary_967.py b/tests/adapters/test_e2b_trust_boundary_967.py index 432fda2b..88a74105 100644 --- a/tests/adapters/test_e2b_trust_boundary_967.py +++ b/tests/adapters/test_e2b_trust_boundary_967.py @@ -193,7 +193,7 @@ def test_a_rejected_path_is_counted(self, workspace): lambda *a, **k: emitted.append(a), ) blob = " ".join(str(a) for a in emitted).lower() - assert "2" in blob and ("reject" in blob or "outside" in blob), emitted + assert "2" in blob and "reject" in blob, emitted def test_a_rejected_path_is_never_read_from_the_sandbox(self, workspace): """Reject before the read, not after — no needless round trip."""