fix(hermes): follow a failing test's imports to the module under test
The bounded patcher could only ever fix defects whose failure output names the source file. A pytest assertion that fails inside a test names only the test: on ariadne build 404 the defective file appeared zero times in the whole 174KB console, and candidate selection returned ariadne/app.py, the wrong file entirely. Admit test files as readable candidates and follow their absolute imports back to the module they exercise. Reading widens; writing does not. The patch validator gates on allowed_path_prefixes alone, so a test file can now be read for context and still never be patched - which also stops the classic bad fix of silencing a failing test instead of repairing the code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
parent
8b7e23baeb
commit
d75d4d503e
@ -59,6 +59,16 @@ _WORKSPACE_PREFIX = re.compile(r"^.*?/workspace/[^/]+/")
|
|||||||
_REPEATED_SLASHES = re.compile(r"/{2,}")
|
_REPEATED_SLASHES = re.compile(r"/{2,}")
|
||||||
_NOISE_SEPARATORS = ("?", "#")
|
_NOISE_SEPARATORS = ("?", "#")
|
||||||
|
|
||||||
|
# Absolute imports only. A relative import gives no repository-root path, and
|
||||||
|
# guessing one would fetch the wrong file.
|
||||||
|
_IMPORT_RE = re.compile(
|
||||||
|
r"^[ \t]*(?:from[ \t]+(?P<module>[A-Za-z_][\w.]*)[ \t]+import[ \t]+(?P<names>[^\n#]+)"
|
||||||
|
r"|import[ \t]+(?P<plain>[A-Za-z_][\w.]*))",
|
||||||
|
re.MULTILINE,
|
||||||
|
)
|
||||||
|
_NAME_SPLIT = re.compile(r"[ \t]*,[ \t]*")
|
||||||
|
_IMPORT_FANOUT = 4
|
||||||
|
|
||||||
|
|
||||||
def extract_candidate_paths(bundle: dict, cfg: dict) -> list[str]:
|
def extract_candidate_paths(bundle: dict, cfg: dict) -> list[str]:
|
||||||
"""Rank the repository files that a build's console failures implicate.
|
"""Rank the repository files that a build's console failures implicate.
|
||||||
@ -173,7 +183,7 @@ def _line_paths(line: str, cfg: dict) -> list[tuple[str, int]]:
|
|||||||
for _name, pattern in FILE_REFERENCE_PATTERNS:
|
for _name, pattern in FILE_REFERENCE_PATTERNS:
|
||||||
for match in pattern.finditer(line):
|
for match in pattern.finditer(line):
|
||||||
path = _normalize(match.group(1))
|
path = _normalize(match.group(1))
|
||||||
if not _is_allowed(path, cfg):
|
if not _is_allowed(path, cfg, readable=True):
|
||||||
continue
|
continue
|
||||||
column = match.start(1)
|
column = match.start(1)
|
||||||
if column < columns.get(path, column + 1):
|
if column < columns.get(path, column + 1):
|
||||||
@ -194,8 +204,84 @@ def _normalize(raw: str) -> str:
|
|||||||
return path
|
return path
|
||||||
|
|
||||||
|
|
||||||
def _is_allowed(path: str, cfg: dict) -> bool:
|
def imported_source_paths(text: str, cfg: dict) -> list[str]:
|
||||||
"""Gate one normalized path on safety and the configured allowlists."""
|
"""Return the in-repo modules a Python source file imports.
|
||||||
|
|
||||||
|
Inputs: the contents of a fetched file (in practice the failing test) and
|
||||||
|
the same cfg used for candidate selection. Outputs: repository-relative
|
||||||
|
paths derived from its import statements, in first-seen order, filtered by
|
||||||
|
the configured prefixes and suffixes exactly like any other candidate.
|
||||||
|
|
||||||
|
A pytest assertion that fails inside a test names only the test file, so
|
||||||
|
the defective source is often absent from the console entirely. Its import
|
||||||
|
list is the one deterministic link back to the module under test. Never
|
||||||
|
raises; returns [] on any parse problem.
|
||||||
|
"""
|
||||||
|
|
||||||
|
try:
|
||||||
|
seen: list[str] = []
|
||||||
|
for match in _IMPORT_RE.finditer(str(text)):
|
||||||
|
for path in _module_paths(match):
|
||||||
|
if _is_allowed(path, cfg) and path not in seen:
|
||||||
|
seen.append(path)
|
||||||
|
return seen
|
||||||
|
except Exception:
|
||||||
|
return []
|
||||||
|
|
||||||
|
|
||||||
|
def _module_paths(match: re.Match[str]) -> list[str]:
|
||||||
|
"""Turn one import statement into the repository paths it could name.
|
||||||
|
|
||||||
|
`from a.b import c` may mean either the module `a/b.py` or the submodule
|
||||||
|
`a/b/c.py`, and both conventions are common, so both are offered and the
|
||||||
|
allowlist and the fetch decide which exists.
|
||||||
|
"""
|
||||||
|
|
||||||
|
plain = match.group("plain")
|
||||||
|
if plain:
|
||||||
|
return [f"{plain.replace('.', '/')}.py"]
|
||||||
|
module = match.group("module")
|
||||||
|
base = module.replace(".", "/")
|
||||||
|
paths = [f"{base}.py"]
|
||||||
|
for name in _NAME_SPLIT.split(match.group("names") or ""):
|
||||||
|
cleaned = name.strip().split(" as ")[0].strip().strip("()")
|
||||||
|
if cleaned and cleaned.isidentifier():
|
||||||
|
paths.append(f"{base}/{cleaned}.py")
|
||||||
|
return paths
|
||||||
|
|
||||||
|
|
||||||
|
def queue_imported_sources(
|
||||||
|
cfg: dict, path: str, contents: str, queue: list[str], queued: set[str]
|
||||||
|
) -> None:
|
||||||
|
"""Follow a failing test's imports back to the module it exercises.
|
||||||
|
|
||||||
|
Inputs: the candidate cfg, the path and contents just fetched, and the
|
||||||
|
caller's fetch queue plus the set of paths already queued, both mutated in
|
||||||
|
place. Only test files are followed, and only into paths the write
|
||||||
|
allowlist already permits, so this widens which files may be read and
|
||||||
|
never which files may be written. Never raises.
|
||||||
|
"""
|
||||||
|
|
||||||
|
if not is_test_path(path, cfg):
|
||||||
|
return
|
||||||
|
# Both import conventions are guessed, so a busy test file can name more
|
||||||
|
# modules than are worth fetching. The context budget already bounds what
|
||||||
|
# is kept; this bounds what is asked for.
|
||||||
|
room = max(0, _max_candidates(cfg) * _IMPORT_FANOUT - len(queued))
|
||||||
|
for extra in imported_source_paths(contents, cfg)[:room]:
|
||||||
|
if extra not in queued:
|
||||||
|
queued.add(extra)
|
||||||
|
queue.append(extra)
|
||||||
|
|
||||||
|
|
||||||
|
def _is_allowed(path: str, cfg: dict, *, readable: bool = False) -> bool:
|
||||||
|
"""Gate one normalized path on safety and the configured allowlists.
|
||||||
|
|
||||||
|
`readable` additionally admits test files. A failing test is the evidence
|
||||||
|
that explains the defect and is often the only file the console names, but
|
||||||
|
the patch validator gates writes on `allowed_path_prefixes` alone, so a
|
||||||
|
test admitted here can be read and never patched.
|
||||||
|
"""
|
||||||
|
|
||||||
if not _is_safe(path):
|
if not _is_safe(path):
|
||||||
return False
|
return False
|
||||||
@ -203,7 +289,9 @@ def _is_allowed(path: str, cfg: dict) -> bool:
|
|||||||
prefixes = [str(item) for item in (cfg.get("allowed_path_prefixes") or [])]
|
prefixes = [str(item) for item in (cfg.get("allowed_path_prefixes") or [])]
|
||||||
if not any(path.endswith(suffix) for suffix in suffixes):
|
if not any(path.endswith(suffix) for suffix in suffixes):
|
||||||
return False
|
return False
|
||||||
return any(path.startswith(prefix) for prefix in prefixes)
|
if any(path.startswith(prefix) for prefix in prefixes):
|
||||||
|
return True
|
||||||
|
return readable and is_test_path(path, cfg)
|
||||||
|
|
||||||
|
|
||||||
def _is_safe(path: str) -> bool:
|
def _is_safe(path: str) -> bool:
|
||||||
|
|||||||
@ -323,7 +323,9 @@ def _fetch_within_budget(cfg: dict, candidates: list[str]) -> tuple[dict[str, st
|
|||||||
fetched: dict[str, str] = {}
|
fetched: dict[str, str] = {}
|
||||||
first_error: str | None = None
|
first_error: str | None = None
|
||||||
used = 0
|
used = 0
|
||||||
for path in candidates:
|
queue = list(candidates)
|
||||||
|
queued = set(queue)
|
||||||
|
for path in queue:
|
||||||
contents, error = hermes_code_repair.fetch_file(cfg, path)
|
contents, error = hermes_code_repair.fetch_file(cfg, path)
|
||||||
if contents is None:
|
if contents is None:
|
||||||
first_error = first_error or str(error or "unknown error")
|
first_error = first_error or str(error or "unknown error")
|
||||||
@ -336,6 +338,7 @@ def _fetch_within_budget(cfg: dict, candidates: list[str]) -> tuple[dict[str, st
|
|||||||
continue
|
continue
|
||||||
fetched[path] = contents
|
fetched[path] = contents
|
||||||
used += len(contents)
|
used += len(contents)
|
||||||
|
hermes_code_candidates.queue_imported_sources(cfg, path, contents, queue, queued)
|
||||||
return fetched, first_error
|
return fetched, first_error
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@ -81,7 +81,20 @@ def test_suffix_outside_the_allowlist_is_rejected() -> None:
|
|||||||
|
|
||||||
def test_prefix_outside_the_allowlist_is_rejected() -> None:
|
def test_prefix_outside_the_allowlist_is_rejected() -> None:
|
||||||
assert _paths("vendor/ledger.py:14: boom") == []
|
assert _paths("vendor/ledger.py:14: boom") == []
|
||||||
assert _paths("tests/test_ledger.py:2: boom", allowed_path_prefixes=["src/"]) == []
|
assert _paths("vendor/helper.py:2: boom", allowed_path_prefixes=["src/"]) == []
|
||||||
|
|
||||||
|
|
||||||
|
def test_test_files_are_readable_outside_the_write_allowlist() -> None:
|
||||||
|
"""The failing test explains the defect and is often the only path named.
|
||||||
|
|
||||||
|
Reading it is what lets its imports lead back to the module under test.
|
||||||
|
Writing to it stays barred by the patch validator, which gates on
|
||||||
|
allowed_path_prefixes alone.
|
||||||
|
"""
|
||||||
|
|
||||||
|
assert _paths("tests/test_ledger.py:2: boom", allowed_path_prefixes=["src/"]) == [
|
||||||
|
"tests/test_ledger.py"
|
||||||
|
]
|
||||||
|
|
||||||
|
|
||||||
def test_empty_allowlists_accept_nothing() -> None:
|
def test_empty_allowlists_accept_nothing() -> None:
|
||||||
@ -210,3 +223,56 @@ def test_is_test_path_honours_configured_markers() -> None:
|
|||||||
def test_is_test_path_never_raises() -> None:
|
def test_is_test_path_never_raises() -> None:
|
||||||
assert module.is_test_path(None, {}) is False
|
assert module.is_test_path(None, {}) is False
|
||||||
assert module.is_test_path("src/a.py", {"test_path_markers": [None]}) is False
|
assert module.is_test_path("src/a.py", {"test_path_markers": [None]}) is False
|
||||||
|
|
||||||
|
|
||||||
|
_CFG = {"allowed_suffixes": [".py"], "allowed_path_prefixes": ["ariadne/"], "max_candidates": 3}
|
||||||
|
|
||||||
|
|
||||||
|
def test_imports_lead_from_a_failing_test_to_the_module_under_test() -> None:
|
||||||
|
"""The defective source is often absent from the console entirely."""
|
||||||
|
|
||||||
|
source = "\n".join(
|
||||||
|
[
|
||||||
|
"from __future__ import annotations",
|
||||||
|
"import httpx",
|
||||||
|
"from ariadne.utils.errors import safe_error_detail",
|
||||||
|
"from ariadne.services import mailu",
|
||||||
|
]
|
||||||
|
)
|
||||||
|
paths = module.imported_source_paths(source, _CFG)
|
||||||
|
|
||||||
|
assert "ariadne/utils/errors.py" in paths
|
||||||
|
assert "ariadne/services/mailu.py" in paths
|
||||||
|
# Third-party and stdlib imports are outside the allowlist.
|
||||||
|
assert not any(path.startswith("httpx") for path in paths)
|
||||||
|
|
||||||
|
|
||||||
|
def test_imported_paths_stay_inside_the_write_allowlist() -> None:
|
||||||
|
"""Reading a test must not make test files patchable."""
|
||||||
|
|
||||||
|
source = "from tests.helpers import build_fixture\nfrom ariadne.app import create_app"
|
||||||
|
paths = module.imported_source_paths(source, _CFG)
|
||||||
|
|
||||||
|
assert "ariadne/app.py" in paths
|
||||||
|
assert not any(path.startswith("tests/") for path in paths)
|
||||||
|
|
||||||
|
|
||||||
|
def test_relative_imports_are_not_guessed() -> None:
|
||||||
|
"""A relative import gives no repository-root path to resolve."""
|
||||||
|
|
||||||
|
assert module.imported_source_paths("from . import errors", _CFG) == []
|
||||||
|
assert module.imported_source_paths("from .errors import safe", _CFG) == []
|
||||||
|
|
||||||
|
|
||||||
|
def test_queue_imported_sources_only_follows_tests() -> None:
|
||||||
|
"""A source file's imports are not chased; only the failing test's are."""
|
||||||
|
|
||||||
|
source = "from ariadne.utils.errors import safe_error_detail"
|
||||||
|
|
||||||
|
queue, queued = ["ariadne/app.py"], {"ariadne/app.py"}
|
||||||
|
module.queue_imported_sources(_CFG, "ariadne/app.py", source, queue, queued)
|
||||||
|
assert queue == ["ariadne/app.py"]
|
||||||
|
|
||||||
|
queue, queued = ["tests/test_utils.py"], {"tests/test_utils.py"}
|
||||||
|
module.queue_imported_sources(_CFG, "tests/test_utils.py", source, queue, queued)
|
||||||
|
assert "ariadne/utils/errors.py" in queue
|
||||||
|
|||||||
Loading…
x
Reference in New Issue
Block a user