mirror of
https://github.com/NousResearch/hermes-agent.git
synced 2026-05-12 03:42:08 +00:00
feat(file_tools): post-write delta lint on write_file + patch, add JSON/YAML/TOML/Python in-process linters (#20191)
Closes the gap where write_file skipped the post-edit syntax check that patch already ran, so silent file corruption (bad quote escaping, truncated writes, etc.) would persist on disk until a later read. ## Changes tools/file_operations.py: - Add in-process linters for .py, .json, .yaml, .toml (LINTERS_INPROC). Python uses ast.parse, JSON/YAML/TOML use stdlib/PyYAML parsers. Zero subprocess overhead; preferred over shell linters when both apply. - _check_lint() now accepts optional content and routes to in-process linter first. Shell linter (py_compile, node --check, tsc, go vet, rustfmt) remains the fallback for languages without an in-process equivalent. - New _check_lint_delta() implements the post-first/pre-lazy pattern borrowed from Cline and OpenCode: lint post-write state first; only if errors are found AND pre-content was captured does it lint the pre-state and diff. If the pre-existing file had the SAME errors the edit didn't introduce anything new, so the file is reported as 'still broken, pre-existing' with success=False but a message explaining the errors were pre-existing. If the edit introduced genuinely new errors, those are surfaced and pre-existing ones are filtered out. - WriteResult gains a lint field. - write_file() captures pre-content for in-process-lintable extensions and calls _check_lint_delta after a successful write. - patch_replace() switches from _check_lint to _check_lint_delta, reusing the pre-edit content it already has in scope. tools/file_tools.py: - Update write_file schema description to mention the post-write lint. tests/tools/test_file_operations_edge_cases.py: - Update existing brace-path tests to use .js (shell linter) now that .py is in-process. - Add TestCheckLintInproc (9 tests) covering Python/JSON/YAML/TOML in-process linters. - Add TestCheckLintDelta (5 tests) covering the post-first/pre-lazy short-circuit, new-file path, and the single-error-parser caveat. ## Performance In-process linters are microseconds per call (ast.parse, json.loads). The hot path (clean write) runs exactly one lint — matches main's cost for patch. Pre-state capture is skipped when the file has no applicable linter. Measured 4.89ms/write average over 100 .py writes including lint. ## Inspiration - Cline's DiffViewProvider.getNewDiagnosticProblems() — filters pre-write diagnostics from post-write diagnostics (src/integrations/editor/DiffViewProvider.ts). - OpenCode's WriteTool — runs lsp.diagnostics() after write and appends errors to tool output (packages/opencode/src/tool/write.ts). - Claude Code's DiagnosticTrackingService — captures baseline via beforeFileEdited() and returns new-diagnostics-only from getNewDiagnostics() (src/services/diagnosticTracking.ts). ## Validation - tests/tools/test_file_operations.py + test_file_operations_edge_cases.py + test_file_tools.py + test_file_tools_live.py + test_file_write_safety.py + test_write_deny.py + test_patch_parser.py + test_file_ops_cwd_tracking.py: 228 passed locally. - Live E2E reproduction of the tips.py corruption incident: broken content written; lint field surfaces 'SyntaxError: invalid syntax. Perhaps you forgot a comma? (line 6, column 5)' — the exact error that would have self-corrected the bug on the next turn.
This commit is contained in:
parent
b93643c8fe
commit
5168226d60
3 changed files with 371 additions and 37 deletions
|
|
@ -82,7 +82,11 @@ class TestIsLikelyBinary:
|
|||
|
||||
|
||||
class TestCheckLintBracePaths:
|
||||
"""Verify _check_lint handles file paths with curly braces safely."""
|
||||
"""Verify _check_lint handles file paths with curly braces safely.
|
||||
|
||||
Uses ``.js`` to exercise the shell-linter path since ``.py`` now goes
|
||||
through the in-process ast.parse linter (see TestCheckLintInproc).
|
||||
"""
|
||||
|
||||
@pytest.fixture()
|
||||
def ops(self):
|
||||
|
|
@ -95,12 +99,12 @@ class TestCheckLintBracePaths:
|
|||
with patch.object(ops, "_has_command", return_value=True), \
|
||||
patch.object(ops, "_exec") as mock_exec:
|
||||
mock_exec.return_value = MagicMock(exit_code=0, stdout="")
|
||||
result = ops._check_lint("/tmp/test_file.py")
|
||||
result = ops._check_lint("/tmp/test_file.js")
|
||||
|
||||
assert result.success is True
|
||||
# Verify the command was built correctly
|
||||
cmd_arg = mock_exec.call_args[0][0]
|
||||
assert "'/tmp/test_file.py'" in cmd_arg
|
||||
assert "'/tmp/test_file.js'" in cmd_arg
|
||||
|
||||
def test_path_with_curly_braces(self, ops):
|
||||
"""Path containing ``{`` and ``}`` must not raise KeyError/ValueError."""
|
||||
|
|
@ -108,7 +112,7 @@ class TestCheckLintBracePaths:
|
|||
patch.object(ops, "_exec") as mock_exec:
|
||||
mock_exec.return_value = MagicMock(exit_code=0, stdout="")
|
||||
# This would raise KeyError with .format() but works with .replace()
|
||||
result = ops._check_lint("/tmp/{test}_file.py")
|
||||
result = ops._check_lint("/tmp/{test}_file.js")
|
||||
|
||||
assert result.success is True
|
||||
cmd_arg = mock_exec.call_args[0][0]
|
||||
|
|
@ -119,7 +123,7 @@ class TestCheckLintBracePaths:
|
|||
with patch.object(ops, "_has_command", return_value=True), \
|
||||
patch.object(ops, "_exec") as mock_exec:
|
||||
mock_exec.return_value = MagicMock(exit_code=0, stdout="")
|
||||
result = ops._check_lint("/tmp/{{var}}.py")
|
||||
result = ops._check_lint("/tmp/{{var}}.js")
|
||||
|
||||
assert result.success is True
|
||||
|
||||
|
|
@ -131,7 +135,7 @@ class TestCheckLintBracePaths:
|
|||
def test_missing_linter_skipped(self, ops):
|
||||
"""When the linter binary is not installed, skip gracefully."""
|
||||
with patch.object(ops, "_has_command", return_value=False):
|
||||
result = ops._check_lint("/tmp/test.py")
|
||||
result = ops._check_lint("/tmp/test.js")
|
||||
assert result.skipped is True
|
||||
|
||||
def test_lint_failure_returns_output(self, ops):
|
||||
|
|
@ -142,12 +146,122 @@ class TestCheckLintBracePaths:
|
|||
exit_code=1,
|
||||
stdout="SyntaxError: invalid syntax",
|
||||
)
|
||||
result = ops._check_lint("/tmp/bad.py")
|
||||
result = ops._check_lint("/tmp/bad.js")
|
||||
|
||||
assert result.success is False
|
||||
assert "SyntaxError" in result.output
|
||||
|
||||
|
||||
class TestCheckLintInproc:
|
||||
"""Verify in-process linters (.py via ast.parse, .json, .yaml, .toml).
|
||||
|
||||
These bypass the shell linter table entirely and parse content
|
||||
directly in Python — no subprocess, no toolchain dependency.
|
||||
"""
|
||||
|
||||
@pytest.fixture()
|
||||
def ops(self):
|
||||
obj = ShellFileOperations.__new__(ShellFileOperations)
|
||||
obj._command_cache = {}
|
||||
return obj
|
||||
|
||||
def test_python_inproc_clean(self, ops):
|
||||
"""Valid Python content passes in-process ast.parse."""
|
||||
result = ops._check_lint("/tmp/ok.py", content="x = 1\n")
|
||||
assert result.success is True
|
||||
assert not result.skipped
|
||||
assert result.output == ""
|
||||
|
||||
def test_python_inproc_syntax_error(self, ops):
|
||||
"""Invalid Python content fails with SyntaxError + line info."""
|
||||
result = ops._check_lint("/tmp/bad.py", content="def foo(:\n pass\n")
|
||||
assert result.success is False
|
||||
assert "SyntaxError" in result.output
|
||||
assert "line" in result.output.lower()
|
||||
|
||||
def test_python_inproc_content_explicit(self, ops):
|
||||
"""When content is passed explicitly, the file is not re-read."""
|
||||
with patch.object(ops, "_exec") as mock_exec:
|
||||
result = ops._check_lint("/tmp/explicit.py", content="y = 2\n")
|
||||
# _exec must not have been called — content was supplied
|
||||
mock_exec.assert_not_called()
|
||||
assert result.success is True
|
||||
|
||||
def test_json_inproc_clean(self, ops):
|
||||
result = ops._check_lint("/tmp/a.json", content='{"a": 1}')
|
||||
assert result.success is True
|
||||
|
||||
def test_json_inproc_error(self, ops):
|
||||
result = ops._check_lint("/tmp/b.json", content='{"a": 1')
|
||||
assert result.success is False
|
||||
assert "JSONDecodeError" in result.output
|
||||
|
||||
def test_yaml_inproc_clean(self, ops):
|
||||
result = ops._check_lint("/tmp/a.yaml", content="a: 1\nb: 2\n")
|
||||
assert result.success is True
|
||||
|
||||
def test_yaml_inproc_error(self, ops):
|
||||
result = ops._check_lint("/tmp/b.yaml", content='key: "unclosed\n')
|
||||
assert result.success is False
|
||||
assert "YAMLError" in result.output
|
||||
|
||||
def test_toml_inproc_clean(self, ops):
|
||||
result = ops._check_lint("/tmp/a.toml", content='[section]\nk = "v"\n')
|
||||
assert result.success is True
|
||||
|
||||
def test_toml_inproc_error(self, ops):
|
||||
result = ops._check_lint("/tmp/b.toml", content='[section\nk = "v"')
|
||||
assert result.success is False
|
||||
assert "TOMLDecodeError" in result.output
|
||||
|
||||
|
||||
class TestCheckLintDelta:
|
||||
"""Verify _check_lint_delta() filters pre-existing errors from post-edit output."""
|
||||
|
||||
@pytest.fixture()
|
||||
def ops(self):
|
||||
obj = ShellFileOperations.__new__(ShellFileOperations)
|
||||
obj._command_cache = {}
|
||||
return obj
|
||||
|
||||
def test_clean_post_no_pre_lint(self, ops):
|
||||
"""Hot path: post-write is clean, pre-lint should be skipped entirely."""
|
||||
with patch.object(ops, "_check_lint", wraps=ops._check_lint) as wrapped:
|
||||
r = ops._check_lint_delta("/tmp/a.py", pre_content="x = 0\n", post_content="x = 1\n")
|
||||
# Post-lint called exactly once (clean), pre-lint never called.
|
||||
assert wrapped.call_count == 1
|
||||
assert r.success is True
|
||||
|
||||
def test_new_file_reports_all_errors(self, ops):
|
||||
"""No pre-content means no delta refinement — all post errors surface."""
|
||||
r = ops._check_lint_delta("/tmp/new.py", pre_content=None, post_content="def x(:\n")
|
||||
assert r.success is False
|
||||
assert "SyntaxError" in r.output
|
||||
|
||||
def test_broken_file_becomes_good(self, ops):
|
||||
"""Post-clean short-circuits without any delta refinement."""
|
||||
r = ops._check_lint_delta("/tmp/fix.py", pre_content="def x(:\n", post_content="def x():\n pass\n")
|
||||
assert r.success is True
|
||||
|
||||
def test_introduces_new_error_filters_pre(self, ops):
|
||||
"""Delta filter drops pre-existing errors, surfaces only new ones."""
|
||||
pre = 'def a(:\n pass\n' # line 1 broken
|
||||
post = 'def a():\n pass\n\ndef b(:\n pass\n' # line 1 fixed, line 4 broken
|
||||
r = ops._check_lint_delta("/tmp/d.py", pre_content=pre, post_content=post)
|
||||
assert r.success is False
|
||||
assert "New lint errors" in r.output or "line 4" in r.output
|
||||
|
||||
def test_pre_existing_remains_flagged_but_not_new(self, ops):
|
||||
"""Single-error parsers (ast) may miss that post is OK — be cautious."""
|
||||
# Pre has line-1 error, post keeps it (and doesn't add anything new)
|
||||
pre = 'def a(:\n pass\n'
|
||||
post = 'def a(:\n pass\n\nprint(42)\n' # still line 1 broken
|
||||
r = ops._check_lint_delta("/tmp/d.py", pre_content=pre, post_content=post)
|
||||
# File is still broken — don't lie and claim success — but flag it as pre-existing
|
||||
assert r.success is False
|
||||
assert "pre-existing" in (r.message or "").lower()
|
||||
|
||||
|
||||
# =========================================================================
|
||||
# Pagination bounds
|
||||
# =========================================================================
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue