From 2e1982f83d2b912a0eda9ec6e224cc33b863e915 Mon Sep 17 00:00:00 2001 From: Neo Guyver Date: Tue, 7 Jul 2026 13:35:15 -0700 Subject: [PATCH] Fail closed on invalid JSON/YAML/TOML writes instead of writing then reporting write_file() previously called _atomic_write() first and only ran the JSON/YAML/TOML/Python syntax check afterward as an informational lint delta -- a parse failure never set the top-level `error` key, so a corrupt structured-data write still landed on disk (and file_tools.py's files_modified gating, which keys off `error`, silently reported it as a successful modification). Move the in-process syntax check for JSON/YAML/TOML ahead of _atomic_write() and refuse the write outright on a parse failure: no temp file, no rename, nothing touches disk, and the result carries a top-level `error` so callers correctly see it as unmodified. Deliberately scoped to _FAIL_CLOSED_INPROC_EXTS (JSON/YAML/TOML), not all of LINTERS_INPROC -- .py is excluded because this codebase's own test fixtures (TestPatchReplacePostWriteVerification et al.) write arbitrary non-Python text through *.py paths purely to exercise write-mechanics; a hard block there broke 3 previously-passing tests during development. Python keeps its pre-existing non-blocking lint-delta report. Adds tests/tools/test_write_file_syntax_gate.py: invalid JSON/YAML/YML/ TOML refused with nothing written (new file) and nothing modified (existing file); valid JSON/YAML still written byte-for-byte; a non-linted extension with garbage content is unaffected; invalid Python is confirmed NOT hard-refused (still just reported). --- tests/tools/test_write_file_syntax_gate.py | 101 +++++++++++++++++++++ tools/file_operations.py | 75 +++++++++++++-- 2 files changed, 169 insertions(+), 7 deletions(-) create mode 100644 tests/tools/test_write_file_syntax_gate.py diff --git a/tests/tools/test_write_file_syntax_gate.py b/tests/tools/test_write_file_syntax_gate.py new file mode 100644 index 00000000000..89aa4d7ced2 --- /dev/null +++ b/tests/tools/test_write_file_syntax_gate.py @@ -0,0 +1,101 @@ +"""Tests for the fail-closed pre-write syntax gate on write_file. + +Structured formats with an in-process linter (JSON/YAML/TOML) are validated +BEFORE any bytes touch disk: a candidate write that doesn't parse is refused +outright -- nothing lands on disk -- instead of being written and merely +reported afterward via the post-write lint delta. + +These run against a REAL LocalEnvironment (actual shell commands / actual +files under tmp_path), matching the existing pattern in +tests/tools/test_file_write_safety.py::TestAtomicWrite. +""" + +import json +from pathlib import Path + +import pytest + +from tools.environments.local import LocalEnvironment +from tools.file_operations import ShellFileOperations + + +@pytest.fixture +def ops(tmp_path: Path): + env = LocalEnvironment(cwd=str(tmp_path)) + return ShellFileOperations(env, cwd=str(tmp_path)) + + +class TestFailClosedSyntaxGate: + def test_invalid_json_refused_file_not_created(self, ops, tmp_path: Path): + target = tmp_path / "config.json" + res = ops.write_file(str(target), '{"a": 1,') # truncated / invalid + assert res.error is not None + assert "json" in res.error.lower() + assert not target.exists(), "invalid JSON must NOT be written to disk" + + def test_invalid_json_refused_existing_file_not_modified(self, ops, tmp_path: Path): + target = tmp_path / "config.json" + target.write_text('{"a": 1}') + res = ops.write_file(str(target), '{"a": 1,') + assert res.error is not None + assert target.read_text() == '{"a": 1}', ( + "existing valid file must be left untouched by a refused write" + ) + + def test_invalid_yaml_refused_file_not_created(self, ops, tmp_path: Path): + target = tmp_path / "config.yaml" + res = ops.write_file(str(target), 'key: "unclosed\n') + assert res.error is not None + assert "yaml" in res.error.lower() + assert not target.exists(), "invalid YAML must NOT be written to disk" + + def test_invalid_yml_extension_also_refused(self, ops, tmp_path: Path): + target = tmp_path / "config.yml" + res = ops.write_file(str(target), 'key: "unclosed\n') + assert res.error is not None + assert not target.exists() + + def test_valid_json_written_exactly(self, ops, tmp_path: Path): + target = tmp_path / "config.json" + content = json.dumps({"a": 1, "b": [1, 2, 3]}) + res = ops.write_file(str(target), content) + assert res.error is None, res.error + assert target.read_text() == content + + def test_valid_yaml_written_exactly(self, ops, tmp_path: Path): + target = tmp_path / "config.yaml" + content = "a: 1\nb:\n - 1\n - 2\n" + res = ops.write_file(str(target), content) + assert res.error is None, res.error + assert target.read_text() == content + + def test_non_linted_extension_with_garbage_still_written(self, ops, tmp_path: Path): + """Behavior for extensions with NO in-process linter is unchanged -- + garbage content is written as-is, no refusal.""" + target = tmp_path / "notes.txt" + garbage = "{{{ not json, not yaml, not anything ]]] <<<" + res = ops.write_file(str(target), garbage) + assert res.error is None, res.error + assert target.read_text() == garbage + + def test_invalid_python_is_NOT_hard_refused(self, ops, tmp_path: Path): + """Deliberate scope decision: .py keeps the pre-existing NON-BLOCKING + lint-delta report rather than a hard refusal (see + ``_FAIL_CLOSED_INPROC_EXTS`` in tools/file_operations.py for why -- + this codebase's own test suite writes arbitrary non-Python content + through *.py paths as generic write-mechanics fixtures).""" + target = tmp_path / "broken.py" + bad_python = "def foo(:\n pass\n" + res = ops.write_file(str(target), bad_python) + assert res.error is None, res.error + assert target.read_text() == bad_python + # Still surfaced via the (non-blocking) lint report: + assert res.lint is not None + assert res.lint.get("status") == "error" + assert "SyntaxError" in res.lint.get("output", "") + + def test_invalid_toml_refused_file_not_created(self, ops, tmp_path: Path): + target = tmp_path / "config.toml" + res = ops.write_file(str(target), "[section\nk = 'v'") + assert res.error is not None + assert not target.exists() diff --git a/tools/file_operations.py b/tools/file_operations.py index 78bdd8d63ca..a3e7161d28d 100644 --- a/tools/file_operations.py +++ b/tools/file_operations.py @@ -677,6 +677,21 @@ LINTERS_INPROC = { '.toml': _lint_toml_inproc, } +# Subset of LINTERS_INPROC that the pre-write fail-closed gate in +# ``write_file`` (see below) refuses on, rather than merely reporting. +# Deliberately excludes ``.py``: unlike JSON/YAML/TOML (atomic structured +# data blobs where "doesn't parse" always means "corrupt"), ``.py`` is +# used throughout this codebase's own test fixtures as a generic +# stand-in extension for arbitrary non-Python text content (e.g. +# ``tests/tools/test_file_operations.py``'s +# ``TestPatchReplacePostWriteVerification`` writes "hello world" / +# "hi world" through a ``*.py`` path purely to exercise write-mechanics, +# not Python validity). Hard-refusing on invalid Python would treat that +# established, exercised pattern as an error and break it. Python source +# keeps the existing (unchanged) post-write lint-delta *report* — still +# visible to the caller, just not a write-blocking refusal. +_FAIL_CLOSED_INPROC_EXTS = frozenset({'.json', '.yaml', '.yml', '.toml'}) + # Max limits for read operations MAX_LINES = 2000 MAX_LINE_LENGTH = 2000 @@ -1316,12 +1331,21 @@ class ShellFileOperations(FileOperations): files. The content never appears in the shell command string — only the file path does. - After the write, runs a post-first / pre-lazy lint check via - ``_check_lint_delta()``. If the new content is clean, the lint - call is O(one parse). If the new content has errors, the pre-write - content is linted too and only errors newly introduced by this - write are surfaced — pre-existing problems are filtered out so - the agent isn't distracted chasing them. + Before anything touches disk, a fail-closed syntax gate runs + against the CANDIDATE content: if ``path``'s extension is in + ``_FAIL_CLOSED_INPROC_EXTS`` (JSON/YAML/TOML — structured data + formats where a parse failure always means corruption) and the + candidate content doesn't parse, the write is refused outright. + No temp file, no rename, nothing on disk changes. + + After a write that clears the gate, runs a post-first / pre-lazy + lint check via ``_check_lint_delta()``. If the new content is + clean, the lint call is O(one parse). If the new content has + errors the gate didn't already catch (i.e. errors from a linter + outside ``_FAIL_CLOSED_INPROC_EXTS``, such as Python), the + pre-write content is linted too and only errors newly introduced + by this write are surfaced — pre-existing problems are filtered + out so the agent isn't distracted chasing them. Args: path: File path to write @@ -1337,6 +1361,44 @@ class ShellFileOperations(FileOperations): if _is_write_denied(path): return WriteResult(error=f"Write denied: '{path}' is a protected system/credential file.") + # ── Fail-closed pre-write syntax gate ─────────────────────────── + # Validate the CANDIDATE content BEFORE any bytes touch disk — + # previously this only ran as a post-write lint *report* that the + # caller could ignore (or that ``files_modified`` gating wouldn't + # catch, since a lint failure never set the top-level ``error`` + # key). A structured-format write that doesn't even parse (mashed + # quotes, truncated generation, wrong indentation dialect) is a + # corrupt write, not a style nit — refuse it outright instead of + # writing first and reporting the damage afterward. + # + # Scope: only extensions in ``_FAIL_CLOSED_INPROC_EXTS`` (JSON/ + # YAML/TOML). ``.py`` deliberately keeps its pre-existing, + # non-blocking lint-delta *report* instead of a hard refusal — see + # ``_FAIL_CLOSED_INPROC_EXTS``'s docstring above for why. Extensions + # with no in-process linter at all (including ones only covered by + # a shell linter) are completely unaffected — this gate never runs + # for them, so behavior there is unchanged. + # + # Checked against the raw ``content`` argument, before the + # BOM/CRLF preservation shims below run. Those shims exist purely + # to match the on-disk file's existing conventions; linting + # post-shim would false-positive a JSONDecodeError on a + # legitimately BOM-marked JSON file purely because this method + # re-adds the marker the read layer strips — see + # ``_file_has_bom``/``_UTF8_BOM`` below. + ext = os.path.splitext(path)[1].lower() + inproc_linter = LINTERS_INPROC.get(ext) if ext in _FAIL_CLOSED_INPROC_EXTS else None + if inproc_linter is not None: + _ok, _lint_err = inproc_linter(content) + if not _ok and _lint_err != "__SKIP__": + return WriteResult( + error=( + f"Refusing to write '{path}': candidate content fails " + f"{ext} syntax validation ({_lint_err}). The file was " + "NOT created or modified. Fix the content and retry." + ) + ) + # Capture pre-write content. Two consumers want it: # # 1. The lint-delta layer (for in-process linters like ast.parse @@ -1352,7 +1414,6 @@ class ShellFileOperations(FileOperations): # the UNION of in-process lint coverage and LSP coverage. For # extensions outside both sets (binaries, opaque formats), # skipping the read keeps the hot path fast. - ext = os.path.splitext(path)[1].lower() pre_content: Optional[str] = None want_pre = ext in LINTERS_INPROC or self._lsp_handles_extension(ext) if want_pre: