Просмотр исходного кода

file_ops: Use tmp file for original linting (#3681)

Fix a potential issue that might lead to file corruption when edit linting is enabled

#3124 introduces a feature for editing: running linter twice before and after the change and only extract new errors introduced by the agent. This has some potential issues and I am working on #3649 to address them, but I feel like I am not gonna finish it in the next few days, and that PR has become harder and harder to review, thus this PR, which only focuses on a small improvement.

So what's the issue? When we run linters on the original file before our edits, we need to copy the original file and use a temporary file to lint, because linting may have side-effect (e.g. modifying the file in-place). I used the word "may" because:

Flake8 has no side-effect, so not a problem as of now.
We don't enforce this or document this "no side-effect" as a requirement for linter implementation, so side-effect is allowed.
Regardless, the "after-edit-linting" uses the same approach: backup the file before linting to avoid data corruption. We should keep our "before-edit-linting" consistent.

Why no new unittest that reproduces the issue? Well, as I have mentioned earlier, flake8 has no side-effect, so technically it's not a bug but a flaw. Therefore, there's no way to write a test that reproduces the issue.
Boxuan Li 1 год назад
Родитель
Сommit
75d5591816
1 измененных файлов с 5 добавлено и 1 удалено
  1. 5 1
      openhands/runtime/plugins/agent_skills/file_ops/file_ops.py

+ 5 - 1
openhands/runtime/plugins/agent_skills/file_ops/file_ops.py

@@ -465,7 +465,11 @@ def _edit_file_impl(
         # lint the original file
         enable_auto_lint = os.getenv('ENABLE_AUTO_LINT', 'false').lower() == 'true'
         if enable_auto_lint:
-            original_lint_error, _ = _lint_file(file_name)
+            # Copy the original file to a temporary file (with the same ext) and lint it
+            suffix = os.path.splitext(file_name)[1]
+            with tempfile.NamedTemporaryFile(suffix=suffix) as orig_file_clone:
+                shutil.copy2(file_name, orig_file_clone.name)
+                original_lint_error, _ = _lint_file(orig_file_clone.name)
 
         # Create a temporary file
         with tempfile.NamedTemporaryFile('w', delete=False) as temp_file: