diff --git a/pyproject.toml b/pyproject.toml index 8d6a3eed9d601..da91e38984e9e 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -45,6 +45,11 @@ dependencies = [ "rich==14.3.3", "tenacity==9.1.4", "pyyaml==6.0.3", + # tomli — TOML parser used as pre-3.11 fallback for stdlib tomllib + # (tools/file_operations.py: _lint_toml_inproc). On 3.11+ this is a + # no-op install; the platform marker keeps it off the resolution for + # current Python so it doesn't add a dependency to every install. + "tomli==2.4.1; python_version < '3.11'", "ruamel.yaml==0.18.17", "requests==2.33.0", # CVE-2026-25645 "jinja2==3.1.6", diff --git a/tests/tools/test_patch_parser.py b/tests/tools/test_patch_parser.py index 6623ec25bbcf2..3487edee39096 100644 --- a/tests/tools/test_patch_parser.py +++ b/tests/tools/test_patch_parser.py @@ -751,3 +751,109 @@ class TestCrlfPatchBody: assert result.success is True, getattr(result, "error", None) assert "\r" not in fo.files["f.py"] assert fo.files["f.py"] == "def f():\n x = 2\n return x\n" + + +class TestV4ABomRoundTrip: + """V4A patches must not silently strip a UTF-8 BOM on UPDATE. + + ``read_file_raw`` deliberately strips the BOM (the agent should + never see U+FEFF), but the underlying ``write_file`` must restore + it on rewrite — otherwise a V4A patch turns an existing BOM-bearing + file into a plain UTF-8 file. Regression for teknium1 review on + PR #55661. + """ + + BOM = "\ufeff" + + def _file_ops_for_update(self, file_path: str, original_bytes: bytes): + """Build a FakeFileOps whose ``write_file`` writes real bytes to + ``file_path``, simulating BOM-preserving behaviour like the real + ``FileOperations.write_file`` (which probes disk for the marker).""" + from pathlib import Path + from tools.file_operations import _has_bom, _UTF8_BOM + + target = Path(file_path) + _bom = self.BOM # capture for inner class + + class FakeFileOps: + def read_file_raw(self, path): + # Simulate BOM-stripped read — same as the real + # read_file_raw which strips the marker before returning. + decoded = original_bytes.decode("utf-8") + if decoded.startswith(_bom): + decoded = decoded[1:] + return SimpleNamespace(content=decoded, error=None) + + def write_file(self, path, content, pre_content=None): + # Simulate real write_file: probe the target for a BOM + # (the real impl calls _file_has_bom → head -c 3) and + # prepend if the original had one. + had_bom = target.exists() and target.read_bytes().startswith( + _bom.encode("utf-8") + ) + if had_bom and not _has_bom(content): + content = _UTF8_BOM + content + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text(content, encoding="utf-8") + return SimpleNamespace(error=None) + + return FakeFileOps() + + def test_update_preserves_bom(self, tmp_path): + """A V4A UPDATE on a BOM-bearing file keeps the BOM.""" + from tools.patch_parser import parse_v4a_patch, apply_v4a_operations + + target = tmp_path / "bom_config.py" + original = self.BOM + "setting = 'old'\n" + target.write_text(original, encoding="utf-8") + + patch = """\ +*** Begin Patch +*** Update File: bom_config.py +@@ setting @@ +-setting = 'old' ++setting = 'new' +*** End Patch""" + + ops, err = parse_v4a_patch(patch) + assert err is None + + file_ops = self._file_ops_for_update(str(target), original.encode("utf-8")) + result = apply_v4a_operations(ops, file_ops) + + assert result.success is True + raw = target.read_bytes() + assert raw.startswith( + self.BOM.encode("utf-8") + ), "BOM was stripped by V4A round-trip" + assert b"setting = 'new'" in raw + assert b"setting = 'old'" not in raw + + def test_update_no_bom_when_original_had_none(self, tmp_path): + """A V4A UPDATE on a plain file must NOT inject a BOM.""" + from tools.patch_parser import parse_v4a_patch, apply_v4a_operations + + target = tmp_path / "plain.py" + original = "print('hello')\n" + target.write_text(original, encoding="utf-8") + + patch = """\ +*** Begin Patch +*** Update File: plain.py +@@ print @@ +-print('hello') ++print('world') +*** End Patch""" + + ops, err = parse_v4a_patch(patch) + assert err is None + + file_ops = self._file_ops_for_update(str(target), original.encode("utf-8")) + result = apply_v4a_operations(ops, file_ops) + + assert result.success is True + raw = target.read_bytes() + assert not raw.startswith( + self.BOM.encode("utf-8") + ), "BOM was injected on a plain file" + assert b"print('world')" in raw diff --git a/tools/file_operations.py b/tools/file_operations.py index 5b9ea997ebf88..688bdcf8e539e 100644 --- a/tools/file_operations.py +++ b/tools/file_operations.py @@ -1114,13 +1114,16 @@ class ShellFileOperations(FileOperations): def _file_has_bom(self, path: str, pre_content: Optional[str] = None) -> bool: """Whether the file on disk starts with a UTF-8 BOM. - Uses ``pre_content`` if we already read the file (zero extra exec - calls); otherwise issues a tiny ``head -c 3`` to sample just the - marker. A missing/empty file returns False (new writes get no BOM + Always probes the first 3 bytes on disk — do NOT trust + ``pre_content`` for BOM detection because the most common + provider (``read_file_raw``) deliberately strips BOMs so the + agent never sees U+FEFF glyphs. Passing BOM-stripped content + through ``pre_content`` would cause a false-negative and + silently remove the marker on rewrite. + + A missing/empty file returns False (new writes get no BOM unless the caller explicitly includes one). """ - if pre_content is not None: - return _has_bom(pre_content) head_cmd = f"head -c 3 {self._escape_shell_arg(path)} 2>/dev/null" head_result = self._exec(head_cmd) if head_result.exit_code != 0 or not head_result.stdout: @@ -1443,8 +1446,12 @@ class ShellFileOperations(FileOperations): pre_content: Pre-edit file content if the caller already has it (e.g. patch_replace read the file for fuzzy matching). When provided, skips a redundant ``cat`` subprocess to - re-read the file for lint baseline / line-ending / BOM - detection. When None, reads from disk as before. + re-read the file for lint baseline / line-ending + detection. BOM detection always probes disk (the most + common provider — ``read_file_raw`` — strips BOMs, so + trusting ``pre_content`` for BOM would cause false + negatives and silent marker loss on rewrite). When + None, reads from disk as before. Returns: WriteResult with bytes written, lint summary, or error. diff --git a/tools/patch_parser.py b/tools/patch_parser.py index 1813dca6c39e9..b0b3458867f75 100644 --- a/tools/patch_parser.py +++ b/tools/patch_parser.py @@ -683,9 +683,15 @@ def _apply_update(op: PatchOperation, file_ops: Any) -> Tuple[bool, str, Optiona new_content = new_content.rstrip('\n') + '\n' + insert_text + '\n' # Write new content — pass current_content (already read above) to avoid - # a redundant cat subprocess inside write_file. - write_result = file_ops.write_file(op.file_path, new_content, - pre_content=current_content) + # a redundant cat subprocess inside write_file. Fall back to the + # two-argument form when the file_ops implementation doesn't accept + # ``pre_content`` (duck-typed callers that only implement the basic + # ``write_file(path, content)`` contract). + try: + write_result = file_ops.write_file(op.file_path, new_content, + pre_content=current_content) + except TypeError: + write_result = file_ops.write_file(op.file_path, new_content) if write_result.error: return False, write_result.error, None, None