fix: normalize generated whitespace
This commit is contained in:
@@ -9,7 +9,8 @@ Laravel production database and does not merge changes automatically.
|
|||||||
2. An administrator reviews it and explicitly applies `ai:queued`, either through the LAI admin dashboard or directly in Gitea.
|
2. An administrator reviews it and explicitly applies `ai:queued`, either through the LAI admin dashboard or directly in Gitea.
|
||||||
3. This worker replaces the label with `ai:running`, clones `main`, and creates `ai/issue-N`.
|
3. This worker replaces the label with `ai:running`, clones `main`, and creates `ai/issue-N`.
|
||||||
4. mini-SWE-agent 2.4.6 implements the issue in a credential-free, unprivileged command environment.
|
4. mini-SWE-agent 2.4.6 implements the issue in a credential-free, unprivileged command environment.
|
||||||
5. The worker rejects protected paths, symlinks, empty patches, and oversized diffs.
|
5. The worker deterministically removes trailing spaces from newly changed text, then rejects any
|
||||||
|
remaining whitespace errors, protected paths, symlinks, empty patches, and oversized diffs.
|
||||||
6. Deterministic setup and test commands run with isolated test database credentials.
|
6. Deterministic setup and test commands run with isolated test database credentials.
|
||||||
7. A pull request containing `Fixes #N`, the original problem, changed files, and checks is opened.
|
7. A pull request containing `Fixes #N`, the original problem, changed files, and checks is opened.
|
||||||
8. The issue is labeled `ai:ready` or `ai:failed` and receives a status comment.
|
8. The issue is labeled `ai:ready` or `ai:failed` and receives a status comment.
|
||||||
|
|||||||
@@ -2,6 +2,7 @@ from __future__ import annotations
|
|||||||
|
|
||||||
import os
|
import os
|
||||||
import pwd
|
import pwd
|
||||||
|
import re
|
||||||
import shutil
|
import shutil
|
||||||
import subprocess
|
import subprocess
|
||||||
import tempfile
|
import tempfile
|
||||||
@@ -14,6 +15,8 @@ from .config import Settings
|
|||||||
from .models import ChangeSummary
|
from .models import ChangeSummary
|
||||||
from .security import redact
|
from .security import redact
|
||||||
|
|
||||||
|
_TRAILING_WHITESPACE = re.compile(r"^(.*):(\d+): trailing whitespace\.$")
|
||||||
|
|
||||||
|
|
||||||
class GitWorkspace:
|
class GitWorkspace:
|
||||||
def __init__(self, settings: Settings, issue_number: int) -> None:
|
def __init__(self, settings: Settings, issue_number: int) -> None:
|
||||||
@@ -123,6 +126,51 @@ class GitWorkspace:
|
|||||||
half = limit // 2
|
half = limit // 2
|
||||||
return output[:half] + "\n...[diff truncated]...\n" + output[-half:]
|
return output[:half] + "\n...[diff truncated]...\n" + output[-half:]
|
||||||
|
|
||||||
|
def normalize_changed_whitespace(self, summary: ChangeSummary) -> tuple[str, ...]:
|
||||||
|
"""Remove trailing spaces only from new files and newly changed lines."""
|
||||||
|
changed_files = set(summary.files)
|
||||||
|
changed_lines: dict[str, set[int] | None] = {}
|
||||||
|
try:
|
||||||
|
self._git("diff", "HEAD", "--check")
|
||||||
|
except subprocess.CalledProcessError as exception:
|
||||||
|
output = "\n".join(filter(None, (exception.stdout, exception.stderr)))
|
||||||
|
for line in output.splitlines():
|
||||||
|
match = _TRAILING_WHITESPACE.match(line)
|
||||||
|
if match is None or match.group(1) not in changed_files:
|
||||||
|
continue
|
||||||
|
changed_lines.setdefault(match.group(1), set()).add(int(match.group(2)))
|
||||||
|
|
||||||
|
untracked = set(
|
||||||
|
self._git(
|
||||||
|
"ls-files",
|
||||||
|
"--others",
|
||||||
|
"--exclude-standard",
|
||||||
|
).stdout.splitlines()
|
||||||
|
)
|
||||||
|
for filename in summary.files:
|
||||||
|
if filename in untracked:
|
||||||
|
changed_lines[filename] = None
|
||||||
|
|
||||||
|
normalized: list[str] = []
|
||||||
|
for filename in summary.files:
|
||||||
|
if filename not in changed_lines:
|
||||||
|
continue
|
||||||
|
path = self.path / filename
|
||||||
|
if not path.is_file() or path.is_symlink():
|
||||||
|
continue
|
||||||
|
with suppress(OSError):
|
||||||
|
content = path.read_bytes()
|
||||||
|
if b"\0" in content:
|
||||||
|
continue
|
||||||
|
cleaned = self._strip_trailing_whitespace(
|
||||||
|
content,
|
||||||
|
changed_lines[filename],
|
||||||
|
)
|
||||||
|
if cleaned != content:
|
||||||
|
path.write_bytes(cleaned)
|
||||||
|
normalized.append(filename)
|
||||||
|
return tuple(normalized)
|
||||||
|
|
||||||
def check_diff(self, summary: ChangeSummary) -> None:
|
def check_diff(self, summary: ChangeSummary) -> None:
|
||||||
try:
|
try:
|
||||||
self._git("diff", "HEAD", "--check")
|
self._git("diff", "HEAD", "--check")
|
||||||
@@ -160,6 +208,25 @@ class GitWorkspace:
|
|||||||
f"Diff whitespace validation failed: {filename} has trailing whitespace"
|
f"Diff whitespace validation failed: {filename} has trailing whitespace"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
@staticmethod
|
||||||
|
def _strip_trailing_whitespace(
|
||||||
|
content: bytes,
|
||||||
|
changed_lines: set[int] | None,
|
||||||
|
) -> bytes:
|
||||||
|
cleaned: list[bytes] = []
|
||||||
|
for line_number, line in enumerate(content.splitlines(keepends=True), start=1):
|
||||||
|
if changed_lines is not None and line_number not in changed_lines:
|
||||||
|
cleaned.append(line)
|
||||||
|
continue
|
||||||
|
if line.endswith(b"\r\n"):
|
||||||
|
body, ending = line[:-2], b"\r\n"
|
||||||
|
elif line.endswith((b"\n", b"\r")):
|
||||||
|
body, ending = line[:-1], line[-1:]
|
||||||
|
else:
|
||||||
|
body, ending = line, b""
|
||||||
|
cleaned.append(body.rstrip(b" \t") + ending)
|
||||||
|
return b"".join(cleaned)
|
||||||
|
|
||||||
def commit(self) -> str:
|
def commit(self) -> str:
|
||||||
self._git("add", "--all")
|
self._git("add", "--all")
|
||||||
self._git(
|
self._git(
|
||||||
|
|||||||
@@ -98,6 +98,14 @@ class Worker:
|
|||||||
raise RuntimeError(f"Agent stopped with status {result.exit_status}")
|
raise RuntimeError(f"Agent stopped with status {result.exit_status}")
|
||||||
|
|
||||||
summary = workspace.summary()
|
summary = workspace.summary()
|
||||||
|
normalized = workspace.normalize_changed_whitespace(summary)
|
||||||
|
if normalized:
|
||||||
|
logger.info(
|
||||||
|
"Normalized trailing whitespace in %s changed file(s) for issue #%s",
|
||||||
|
len(normalized),
|
||||||
|
issue.number,
|
||||||
|
)
|
||||||
|
summary = workspace.summary()
|
||||||
validate_changes(workspace, summary, self.settings)
|
validate_changes(workspace, summary, self.settings)
|
||||||
workspace.check_diff(summary)
|
workspace.check_diff(summary)
|
||||||
for command in self.settings.test_commands:
|
for command in self.settings.test_commands:
|
||||||
|
|||||||
@@ -9,6 +9,57 @@ from lai_automation.git import GitWorkspace
|
|||||||
from lai_automation.models import ChangeSummary
|
from lai_automation.models import ChangeSummary
|
||||||
|
|
||||||
|
|
||||||
|
def test_normalize_changed_whitespace_cleans_new_text_file(
|
||||||
|
settings,
|
||||||
|
tmp_path: Path,
|
||||||
|
) -> None:
|
||||||
|
workspace = GitWorkspace(replace(settings, workspace_root=tmp_path), 1)
|
||||||
|
workspace.path.mkdir(parents=True)
|
||||||
|
changed = workspace.path / "new.php"
|
||||||
|
changed.write_bytes(b"<?php \r\nreturn true;\t")
|
||||||
|
workspace._git = Mock(
|
||||||
|
side_effect=[
|
||||||
|
subprocess.CompletedProcess([], 0, "", ""),
|
||||||
|
subprocess.CompletedProcess([], 0, "new.php\n", ""),
|
||||||
|
]
|
||||||
|
)
|
||||||
|
|
||||||
|
normalized = workspace.normalize_changed_whitespace(
|
||||||
|
ChangeSummary(("new.php",), 2, 0)
|
||||||
|
)
|
||||||
|
|
||||||
|
assert normalized == ("new.php",)
|
||||||
|
assert changed.read_bytes() == b"<?php\r\nreturn true;"
|
||||||
|
|
||||||
|
|
||||||
|
def test_normalize_changed_whitespace_only_cleans_reported_tracked_lines(
|
||||||
|
settings,
|
||||||
|
tmp_path: Path,
|
||||||
|
) -> None:
|
||||||
|
workspace = GitWorkspace(replace(settings, workspace_root=tmp_path), 1)
|
||||||
|
workspace.path.mkdir(parents=True)
|
||||||
|
changed = workspace.path / "tracked.php"
|
||||||
|
changed.write_bytes(b"<?php \nlegacy whitespace \n")
|
||||||
|
workspace._git = Mock(
|
||||||
|
side_effect=[
|
||||||
|
subprocess.CalledProcessError(
|
||||||
|
2,
|
||||||
|
["git", "diff", "--check"],
|
||||||
|
output="tracked.php:1: trailing whitespace.\n",
|
||||||
|
stderr="",
|
||||||
|
),
|
||||||
|
subprocess.CompletedProcess([], 0, "", ""),
|
||||||
|
]
|
||||||
|
)
|
||||||
|
|
||||||
|
normalized = workspace.normalize_changed_whitespace(
|
||||||
|
ChangeSummary(("tracked.php",), 1, 1)
|
||||||
|
)
|
||||||
|
|
||||||
|
assert normalized == ("tracked.php",)
|
||||||
|
assert changed.read_bytes() == b"<?php\nlegacy whitespace \n"
|
||||||
|
|
||||||
|
|
||||||
def test_check_diff_accepts_clean_changed_file(settings, tmp_path: Path) -> None:
|
def test_check_diff_accepts_clean_changed_file(settings, tmp_path: Path) -> None:
|
||||||
workspace = GitWorkspace(replace(settings, workspace_root=tmp_path), 1)
|
workspace = GitWorkspace(replace(settings, workspace_root=tmp_path), 1)
|
||||||
workspace.path.mkdir(parents=True)
|
workspace.path.mkdir(parents=True)
|
||||||
|
|||||||
@@ -68,6 +68,9 @@ def test_successful_issue_creates_pr_and_ready_label(
|
|||||||
workspace.has_symlink.return_value = False
|
workspace.has_symlink.return_value = False
|
||||||
workspace.has_path.return_value = False
|
workspace.has_path.return_value = False
|
||||||
workspace.commit.return_value = "abc123"
|
workspace.commit.return_value = "abc123"
|
||||||
|
workspace.normalize_changed_whitespace.return_value = (
|
||||||
|
"tests/Feature/FixTest.php",
|
||||||
|
)
|
||||||
|
|
||||||
Worker(settings, gitea, agent).process(issue, labels())
|
Worker(settings, gitea, agent).process(issue, labels())
|
||||||
|
|
||||||
@@ -85,6 +88,8 @@ def test_successful_issue_creates_pr_and_ready_label(
|
|||||||
]
|
]
|
||||||
)
|
)
|
||||||
workspace.push.assert_called_once()
|
workspace.push.assert_called_once()
|
||||||
|
workspace.normalize_changed_whitespace.assert_called_once()
|
||||||
|
assert workspace.summary.call_count == 3
|
||||||
assert workspace.check_diff.call_count == 2
|
assert workspace.check_diff.call_count == 2
|
||||||
assert gitea.create_pull_request.call_args.args[0] == "ai/issue-7"
|
assert gitea.create_pull_request.call_args.args[0] == "ai/issue-7"
|
||||||
assert "Fixes #7" in gitea.create_pull_request.call_args.args[2]
|
assert "Fixes #7" in gitea.create_pull_request.call_args.args[2]
|
||||||
@@ -115,6 +120,7 @@ def test_failed_tests_prevent_push_and_mark_issue_failed(
|
|||||||
workspace.summary.return_value = ChangeSummary(("app/Fix.php",), 2, 0)
|
workspace.summary.return_value = ChangeSummary(("app/Fix.php",), 2, 0)
|
||||||
workspace.has_symlink.return_value = False
|
workspace.has_symlink.return_value = False
|
||||||
workspace.has_path.return_value = False
|
workspace.has_path.return_value = False
|
||||||
|
workspace.normalize_changed_whitespace.return_value = ()
|
||||||
workspace.run_trusted.side_effect = [
|
workspace.run_trusted.side_effect = [
|
||||||
Mock(),
|
Mock(),
|
||||||
RuntimeError("tests failed with token=hidden"),
|
RuntimeError("tests failed with token=hidden"),
|
||||||
|
|||||||
Reference in New Issue
Block a user