From d75d4d503e6d04d207ba8fa4183c7546ff3e90de Mon Sep 17 00:00:00 2001 From: codex Date: Thu, 6 Aug 2026 01:19:45 -0300 Subject: [PATCH] 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 --- ariadne/services/hermes_code_candidates.py | 96 +++++++++++++++++++++- ariadne/services/hermes_code_flow.py | 5 +- tests/test_hermes_code_candidates.py | 68 ++++++++++++++- 3 files changed, 163 insertions(+), 6 deletions(-) diff --git a/ariadne/services/hermes_code_candidates.py b/ariadne/services/hermes_code_candidates.py index b5d04eb..f6e729e 100644 --- a/ariadne/services/hermes_code_candidates.py +++ b/ariadne/services/hermes_code_candidates.py @@ -59,6 +59,16 @@ _WORKSPACE_PREFIX = re.compile(r"^.*?/workspace/[^/]+/") _REPEATED_SLASHES = re.compile(r"/{2,}") _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[A-Za-z_][\w.]*)[ \t]+import[ \t]+(?P[^\n#]+)" + r"|import[ \t]+(?P[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]: """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 match in pattern.finditer(line): path = _normalize(match.group(1)) - if not _is_allowed(path, cfg): + if not _is_allowed(path, cfg, readable=True): continue column = match.start(1) if column < columns.get(path, column + 1): @@ -194,8 +204,84 @@ def _normalize(raw: str) -> str: return path -def _is_allowed(path: str, cfg: dict) -> bool: - """Gate one normalized path on safety and the configured allowlists.""" +def imported_source_paths(text: str, cfg: dict) -> list[str]: + """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): 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 [])] if not any(path.endswith(suffix) for suffix in suffixes): 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: diff --git a/ariadne/services/hermes_code_flow.py b/ariadne/services/hermes_code_flow.py index b77eee1..31c8ee6 100644 --- a/ariadne/services/hermes_code_flow.py +++ b/ariadne/services/hermes_code_flow.py @@ -323,7 +323,9 @@ def _fetch_within_budget(cfg: dict, candidates: list[str]) -> tuple[dict[str, st fetched: dict[str, str] = {} first_error: str | None = None 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) if contents is None: 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 fetched[path] = contents used += len(contents) + hermes_code_candidates.queue_imported_sources(cfg, path, contents, queue, queued) return fetched, first_error diff --git a/tests/test_hermes_code_candidates.py b/tests/test_hermes_code_candidates.py index cb5c5b2..f032119 100644 --- a/tests/test_hermes_code_candidates.py +++ b/tests/test_hermes_code_candidates.py @@ -81,7 +81,20 @@ def test_suffix_outside_the_allowlist_is_rejected() -> None: def test_prefix_outside_the_allowlist_is_rejected() -> None: 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: @@ -210,3 +223,56 @@ def test_is_test_path_honours_configured_markers() -> None: def test_is_test_path_never_raises() -> None: assert module.is_test_path(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