fix(file-ops): decouple BOM detection from pre_content, add V4A backward compat
Bug 1 (UTF-8 BOM loss on V4A UPDATE): _file_has_bom() trusted pre_content for BOM detection, but the most common pre_content provider — read_file_raw() — deliberately strips BOMs so the agent never sees U+FEFF glyphs. Passing BOM-stripped content through pre_content caused a false-negative: the method returned False and write_file() silently removed the marker on rewrite. Fix: _file_has_bom() now always probes the first 3 bytes on disk (head -c 3), ignoring pre_content for BOM purposes. pre_content is still used by two other consumers — line-ending detection and lint/LSP delta computation — neither of which is affected by BOM stripping. Bug 2 (backward compatibility): _apply_update() called write_file(path, content, pre_content=...) as a keyword argument. Duck-typed file_ops implementations that only implement the two-argument write_file(path, content) contract would raise TypeError. Fix: wrap the call in try/except TypeError, falling back to the two-argument form when the keyword is not accepted. Also declare tomli in pyproject.toml (pre-existing conditional import for pre-3.11 Python, caught by the pre-commit dep scan after staging file_operations.py). Tests: Add TestV4ABomRoundTrip with two cases: - UPDATE on BOM-bearing file preserves the marker - UPDATE on plain file does not inject a BOM Addresses teknium1 review on PR #55661.
This commit is contained in:
parent
cb3e8e9fb1
commit
eb78ab235f
|
|
@ -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",
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue