diff --git a/ariadne/services/hermes_code_defects.py b/ariadne/services/hermes_code_defects.py index f8129ef..d52896f 100644 --- a/ariadne/services/hermes_code_defects.py +++ b/ariadne/services/hermes_code_defects.py @@ -18,6 +18,8 @@ from typing import Any LINT_VIOLATION = "lint_violation" +UNDEFINED_NAME = "undefined_name" +FAILING_ASSERTION = "failing_assertion" # Each pattern must capture path, line, rule and message. Anything that cannot # name all four is not actionable enough to narrow the patch instruction, so it @@ -47,6 +49,21 @@ _PATTERNS: tuple[tuple[str, re.Pattern[str]], ...] = ( ) _ESLINT_FILE = re.compile(r"^(?P[\w./\-]+\.(?:js|jsx|ts|tsx))$") + +# A missing or misspelled name is named exactly by the runtime or compiler, so +# the fix is one identifier or one import rather than a judgement call. +_NAME_PATTERNS: tuple[tuple[str, re.Pattern[str]], ...] = ( + ("python", re.compile(r"NameError: name '(?P[^']+)' is not defined")), + ("python", re.compile(r"ModuleNotFoundError: No module named '(?P[^']+)'")), + ( + "python", + re.compile(r"ImportError: cannot import name '(?P[^']+)' from '(?P[^']+)'"), + ), + ("go", re.compile(r"^(?P[\w./\-]+\.go):(?P\d+):\d+:\s+undefined:\s+(?P\w+)")), + ("rust", re.compile(r"cannot find (?:value|function|type) `(?P[^`]+)` in this scope")), +) +# The Python traceback names the file on the line above the error itself. +_PY_FRAME = re.compile(r'^\s*File "(?P[^"]+)", line (?P\d+)') _MAX_DEFECTS = 20 @@ -122,6 +139,98 @@ def _scan(lines: list[str], cfg: dict) -> list[dict[str, Any]]: return found +def extract_name_defects(bundle: dict, cfg: dict) -> list[dict[str, Any]]: + """Return the undefined or misspelled names the build reported. + + Inputs: the evidence bundle and the per-job code cfg. Outputs: at most + twenty {"category", "tool", "symbol", "path", "line", "message"} dicts. + + A Python traceback names the file on a frame line above the error, so the + most recent frame is carried forward and attached to the error when it + arrives. A defect whose file is unknown or outside the write allowlist is + dropped, since the patcher could not act on it. Never raises. + """ + + try: + return _scan_names(_evidence_lines(bundle), cfg) + except Exception: + return [] + + +def extract_assertion_defects(bundle: dict, cfg: dict) -> list[dict[str, Any]]: + """Return the failing tests the build published as structured results. + + Inputs: the evidence bundle, whose `jenkins.failed_tests` is populated when + the build published test results, and the per-job code cfg. Outputs: at + most twenty {"category", "test", "class", "message"} dicts. + + Carries no path on purpose: the failing test names the symptom, and the + defect is usually in the code under test rather than in the test itself. + Naming a path here would point the patcher at the test and invite it to + weaken the assertion instead of fixing the cause. Never raises. + """ + + try: + jenkins = bundle.get("jenkins") if isinstance(bundle.get("jenkins"), dict) else {} + tests = jenkins.get("failed_tests") + found = [] + for test in (tests if isinstance(tests, list) else [])[:_MAX_DEFECTS]: + if not isinstance(test, dict) or not str(test.get("name") or "").strip(): + continue + found.append( + { + "category": FAILING_ASSERTION, + "test": str(test.get("name")).strip(), + "class": str(test.get("className") or "").strip(), + "message": " ".join(str(test.get("errorDetails") or "").split())[:400], + } + ) + return found + except Exception: + return [] + + +def _scan_names(lines: list[str], cfg: dict) -> list[dict[str, Any]]: + """Match undefined-name shapes, carrying the last traceback frame.""" + + found: list[dict[str, Any]] = [] + seen: set[tuple[str, str]] = set() + frame_path, frame_line = "", 0 + for raw in lines: + line = raw.rstrip() + frame = _PY_FRAME.match(line) + if frame: + frame_path, frame_line = frame.group("path"), int(frame.group("line")) + continue + for tool, pattern in _NAME_PATTERNS: + match = pattern.search(line) + if not match: + continue + groups = match.groupdict() + path = groups.get("path") or frame_path + line_no = int(groups["line"]) if groups.get("line") else frame_line + if not path or not _is_writable(path, cfg): + break + key = (path, groups["symbol"]) + if key in seen: + break + seen.add(key) + found.append( + { + "category": UNDEFINED_NAME, + "tool": tool, + "symbol": groups["symbol"], + "path": path, + "line": line_no, + "message": line.strip()[:200], + } + ) + break + if len(found) >= _MAX_DEFECTS: + break + return found + + def _is_writable(path: str, cfg: dict) -> bool: """Report whether a diagnostic's file is one the patcher may write to.""" @@ -132,24 +241,56 @@ def _is_writable(path: str, cfg: dict) -> bool: return bool(prefixes) and any(path.startswith(p) for p in prefixes) +def extract_defects(bundle: dict, cfg: dict) -> list[dict[str, Any]]: + """Return every mechanically-fixable defect the evidence names. + + Inputs: the evidence bundle and the per-job code cfg. Outputs: the lint, + undefined-name and failing-assertion defects together, most precisely + located first, so the instruction leads with the defect whose fix is least + open to interpretation. + """ + + return ( + extract_lint_defects(bundle, cfg) + + extract_name_defects(bundle, cfg) + + extract_assertion_defects(bundle, cfg) + ) + + def defect_instruction(defects: list[dict[str, Any]]) -> str: """Render the defects as a precise instruction for the patch prompt. - Inputs: the defects from `extract_lint_defects`. Outputs: a prompt - fragment naming each one, or "" when there are none so the caller falls - back to the open-ended request. Naming the rule and line is the point: - it converts "repair this build" into a change whose correctness can be - read off the same evidence. + Inputs: the defects from `extract_defects`. Outputs: a prompt fragment + naming each one, or "" when there are none so the caller falls back to the + open-ended request. Naming the rule, symbol or assertion is the point: it + converts "repair this build" into a change whose correctness can be read + off the same evidence. """ if not defects: return "" - lines = [ - "A linter reported these violations; fix exactly these and nothing else:", - ] + lines: list[str] = ["The build reported these specific defects; address exactly these:"] for defect in defects: + lines.append(_defect_line(defect)) + if any(d["category"] == LINT_VIOLATION for d in defects): + lines.append("Do not reformat unrelated lines and do not suppress the rule.") + if any(d["category"] == FAILING_ASSERTION for d in defects): lines.append( - f"- {defect['path']} line {defect['line']}: {defect['rule']} {defect['message']}" + "Fix the code under test so the assertion holds. Do not weaken, skip or delete " + "the test, and do not change its expected values." ) - lines.append("Do not reformat unrelated lines and do not suppress the rule.") return "\n".join(lines) + + +def _defect_line(defect: dict[str, Any]) -> str: + """Render one defect as a single instruction bullet.""" + + category = defect["category"] + if category == LINT_VIOLATION: + return f"- {defect['path']} line {defect['line']}: {defect['rule']} {defect['message']}" + if category == UNDEFINED_NAME: + return ( + f"- {defect['path']} line {defect['line']}: the name {defect['symbol']!r} is not " + f"defined or not importable ({defect['message']})" + ) + return f"- test {defect['class']}.{defect['test']} failed: {defect['message']}" diff --git a/ariadne/services/hermes_code_prompt.py b/ariadne/services/hermes_code_prompt.py index db8c463..58c7d1d 100644 --- a/ariadne/services/hermes_code_prompt.py +++ b/ariadne/services/hermes_code_prompt.py @@ -40,7 +40,7 @@ def build_patch_prompt( compact = json.dumps(bundle, separators=(",", ":"), ensure_ascii=True) listing = "\n".join(f"- {path}" for path in fetched) instruction = hermes_code_defects.defect_instruction( - hermes_code_defects.extract_lint_defects(bundle, code_cfg) + hermes_code_defects.extract_defects(bundle, code_cfg) ) sections = "\n\n".join( _FILE_SECTION_TEMPLATE.format(path=path, contents=contents) diff --git a/tests/test_hermes_code_defects.py b/tests/test_hermes_code_defects.py index 68da59e..7a50071 100644 --- a/tests/test_hermes_code_defects.py +++ b/tests/test_hermes_code_defects.py @@ -120,3 +120,154 @@ def test_the_patch_prompt_is_unchanged_without_diagnostics() -> None: assert "A linter reported these violations" not in prompt assert "MINIMAL source fix" in prompt + + +# --- category 2: undefined or misspelled names --------------------------------- + + +def test_python_nameerror_takes_its_file_from_the_traceback_frame() -> None: + """The error line names the symbol; the frame above names the file.""" + + found = module.extract_name_defects( + _bundle(' File "ariadne/app.py", line 42, in build', "NameError: name 'setttings' is not defined"), + CFG, + ) + assert found[0]["symbol"] == "setttings" + assert (found[0]["path"], found[0]["line"]) == ("ariadne/app.py", 42) + assert found[0]["category"] == module.UNDEFINED_NAME + + +def test_import_errors_are_recognised() -> None: + for text in ( + "ModuleNotFoundError: No module named 'httpx'", + "ImportError: cannot import name 'safe_error' from 'ariadne.utils.errors'", + ): + found = module.extract_name_defects( + _bundle(' File "ariadne/app.py", line 3, in ', text), CFG + ) + assert found, text + assert found[0]["category"] == module.UNDEFINED_NAME + + +def test_go_undefined_names_carry_their_own_location() -> None: + found = module.extract_name_defects(_bundle("src/main.go:12:5: undefined: doThing"), CFG) + assert (found[0]["symbol"], found[0]["path"], found[0]["line"]) == ("doThing", "src/main.go", 12) + + +def test_a_name_defect_without_a_known_file_is_dropped() -> None: + """With no frame there is no file, so the patcher could not act on it.""" + + assert module.extract_name_defects(_bundle("NameError: name 'x' is not defined"), CFG) == [] + + +def test_name_defects_outside_the_write_allowlist_are_dropped() -> None: + found = module.extract_name_defects( + _bundle(' File "vendor/thing.py", line 1, in x', "NameError: name 'y' is not defined"), CFG + ) + assert found == [] + + +# --- category 3: failing assertions -------------------------------------------- + + +def test_failing_tests_come_from_structured_results() -> None: + bundle = { + "jenkins": { + "console_failures": [], + "console_tail": "", + "failed_tests": [ + { + "name": "test_discount", + "className": "tests.test_discount", + "errorDetails": "AssertionError: assert 0.0 == 90.0", + } + ], + } + } + found = module.extract_assertion_defects(bundle, CFG) + assert found[0]["test"] == "test_discount" + assert found[0]["class"] == "tests.test_discount" + assert "assert 0.0 == 90.0" in found[0]["message"] + + +def test_an_assertion_defect_carries_no_path() -> None: + """Naming the test file would invite weakening the test instead of fixing the cause.""" + + bundle = {"jenkins": {"failed_tests": [{"name": "t", "className": "c", "errorDetails": "boom"}]}} + assert "path" not in module.extract_assertion_defects(bundle, CFG)[0] + + +def test_assertion_instruction_forbids_weakening_the_test() -> None: + bundle = {"jenkins": {"failed_tests": [{"name": "t", "className": "c", "errorDetails": "boom"}]}} + text = module.defect_instruction(module.extract_assertion_defects(bundle, CFG)) + assert "Fix the code under test" in text + assert "Do not weaken, skip or delete" in text + + +def test_malformed_failed_tests_are_skipped() -> None: + bundle = {"jenkins": {"failed_tests": ["nope", {}, {"name": " "}, {"name": "ok"}]}} + found = module.extract_assertion_defects(bundle, CFG) + assert [f["test"] for f in found] == ["ok"] + + +def test_extract_defects_combines_every_category() -> None: + bundle = { + "jenkins": { + "console_failures": [ + {"text": "ariadne/app.py:12:5: E501 Line too long\n" + ' File "ariadne/app.py", line 42, in build\n' + "NameError: name 'setttings' is not defined"} + ], + "console_tail": "", + "failed_tests": [{"name": "t", "className": "c", "errorDetails": "boom"}], + } + } + cats = {d["category"] for d in module.extract_defects(bundle, CFG)} + assert cats == {module.LINT_VIOLATION, module.UNDEFINED_NAME, module.FAILING_ASSERTION} + + +def test_name_and_assertion_extraction_never_raise_on_hostile_input() -> None: + for bad in (None, 7, {"jenkins": {"console_failures": "no"}}, {"jenkins": {"failed_tests": 5}}): + assert module.extract_name_defects(bad, CFG) == [] + assert module.extract_assertion_defects(bad, CFG) == [] + assert module.extract_name_defects(_bundle("NameError: name 'x' is not defined"), None) == [] + + +def test_name_extraction_is_bounded() -> None: + lines = [] + for i in range(40): + lines.append(f' File "ariadne/f{i}.py", line {i + 1}, in fn') + lines.append(f"NameError: name 'sym{i}' is not defined") + assert len(module.extract_name_defects(_bundle(*lines), CFG)) == module._MAX_DEFECTS + + +def test_duplicate_name_defects_are_collapsed() -> None: + lines = [' File "ariadne/app.py", line 4, in fn', "NameError: name 'zz' is not defined"] * 3 + assert len(module.extract_name_defects(_bundle(*lines), CFG)) == 1 + + +def test_a_name_defect_with_no_prefixes_configured_is_dropped() -> None: + """An empty allowlist must deny, never permit everything.""" + + empty = {"allowed_path_prefixes": [], "allowed_suffixes": [".py"]} + found = module.extract_name_defects( + _bundle(' File "ariadne/app.py", line 1, in fn', "NameError: name 'q' is not defined"), empty + ) + assert found == [] + + +def test_undefined_name_instruction_names_the_symbol() -> None: + found = module.extract_name_defects( + _bundle(' File "ariadne/app.py", line 42, in build', "NameError: name 'setttings' is not defined"), + CFG, + ) + text = module.defect_instruction(found) + assert "'setttings' is not defined or not importable" in text + assert "ariadne/app.py line 42" in text + + +def test_a_lint_defect_with_a_disallowed_suffix_is_dropped() -> None: + """Suffix and prefix are separate gates; both must hold.""" + + cfg = {"allowed_path_prefixes": ["ariadne/"], "allowed_suffixes": [".go"]} + assert module.extract_lint_defects(_bundle("ariadne/app.py:1:1: E501 Long"), cfg) == []