diff --git a/scripts/audit_exception_handling.py b/scripts/audit_exception_handling.py index c064b168..e11a33d5 100644 --- a/scripts/audit_exception_handling.py +++ b/scripts/audit_exception_handling.py @@ -771,8 +771,8 @@ class ExceptionVisitor(ast.NodeVisitor): for handler in node.handlers: category, hint = self._classify_except(handler, node) self._add_finding("EXCEPT", handler.lineno, self._snippet(handler), category, hint) - for child in handler.body if node.handlers else []: - self.visit(child) + for child in handler.body: + self.visit(child) for child in node.orelse: self.visit(child) for child in node.finalbody: diff --git a/tests/test_audit_exception_handling_bug_fixes.py b/tests/test_audit_exception_handling_bug_fixes.py new file mode 100644 index 00000000..407c3f6d --- /dev/null +++ b/tests/test_audit_exception_handling_bug_fixes.py @@ -0,0 +1,227 @@ +"""Tests for the 3 audit-script bugs documented in +docs/reports/RESULT_MIGRATION_REVIEW_PASS_20260617.md section 4.4. + +Bug 1: visit_Try walker only walks children of the LAST except handler. + Raises in non-last except handlers are missed. +Bug 2: render_json filters out compliant findings in non-verbose mode. + INTERNAL_COMPLIANT sites are not in the per-file findings list. +Bug 3: render_json truncates per-file list to --top (default 15). + Low-violation files with UNCLEAR sites are not in the per-file list. + +Each test uses the subprocess pattern from test_audit_exception_handling_heuristics.py: +create a fixture file, run the audit, parse the JSON, assert. +""" +from __future__ import annotations + +import json +import subprocess +import sys +import textwrap +from pathlib import Path + +ROOT = Path(__file__).resolve().parents[1] +SCRIPT = ROOT / "scripts" / "audit_exception_handling.py" + + +def _run_audit_on_fixture(source: str, top: int = 15, verbose: bool = False) -> dict: + """Run the audit script on a fixture and return the parsed JSON output.""" + import tempfile + tmpdir = Path(tempfile.mkdtemp(prefix="audit_bugfix_fixture_")) + fixture = tmpdir / "audit_bugfix_fixture.py" + fixture.write_text(textwrap.dedent(source), encoding="utf-8") + try: + cmd = [sys.executable, str(SCRIPT), "--json", "--src", str(tmpdir), + "--top", str(top), "--include-baseline"] + if verbose: + cmd.append("--verbose") + result = subprocess.run( + cmd, + capture_output=True, + text=True, + check=False, + cwd=str(ROOT), + ) + finally: + if fixture.exists(): + fixture.unlink() + if tmpdir.exists(): + tmpdir.rmdir() + if result.returncode not in (0, 1): + raise RuntimeError(f"audit failed: {result.stderr}") + return json.loads(result.stdout) + + +def _classifications_for_file(data: dict, filename_suffix: str) -> list[dict]: + """Return all findings for files whose path ends with `filename_suffix`.""" + return [ + f + for file_info in data.get("files", []) + for f in file_info.get("findings", []) + if file_info["filename"].endswith(filename_suffix) + ] + + +def _has_raise_finding(findings: list[dict], target_line: int) -> bool: + """True if there is a RAISE-kind finding at the target line.""" + return any( + f.get("kind") == "RAISE" and f.get("line") == target_line + for f in findings + ) + + +# --------------------------------------------------------------------------- +# Bug 1: visit_Try walker only walks children of the LAST except handler +# --------------------------------------------------------------------------- +def test_visit_try_walks_first_handler_raise(): + """A raise in the FIRST except handler must be detected. + + The audit script's visit_Try has a bug: the `for child in handler.body` + loop is OUTSIDE the `for handler in node.handlers` loop. So `handler` is + bound to the LAST handler, and only the last handler's body is walked. + + Per docs/reports/RESULT_MIGRATION_REVIEW_PASS_20260617.md section 4.4 #1: + src/rag_engine.py:31 (raise in the FIRST except of a 2-handler try) + is missed by the audit. After the fix, both raises should be reported. + """ + src = ''' + def func(): + try: + from foo import bar + return bar() + except ImportError as e: + if e.name == "foo": + raise RuntimeError("missing foo") from e + raise + except Exception as e: + sys.stderr.write(f"other error: {e}") + raise e + ''' + data = _run_audit_on_fixture(src, verbose=True) + findings = _classifications_for_file(data, "audit_bugfix_fixture.py") + raise_lines = sorted(f["line"] for f in findings if f.get("kind") == "RAISE") + # The first except's raise is at line 8 (raise RuntimeError). + # The second except's raise is at line 12 (raise e). + # After the fix, BOTH lines should be in the findings. + # Before the fix, only line 12 (last handler) is reported. + # Use --verbose to bypass the Bug 2 (render_json filter) which hides + # INTERNAL_PROGRAMMER_RAISE findings in non-verbose mode. + assert 8 in raise_lines, ( + f"visit_Try walker bug: first except handler's raise at line 8 " + f"is not in findings. Got raise lines: {raise_lines}" + ) + + +def test_visit_try_walks_middle_handler_raise(): + """A raise in the MIDDLE except handler must be detected (3-handler try).""" + src = ''' + def func(): + try: + from foo import bar + return bar() + except ImportError as e: + pass + except ValueError as e: + raise ValueError("rewrapped") from e + except Exception as e: + raise e + ''' + data = _run_audit_on_fixture(src, verbose=True) + findings = _classifications_for_file(data, "audit_bugfix_fixture.py") + raise_lines = sorted(f["line"] for f in findings if f.get("kind") == "RAISE") + # middle handler's `raise ValueError(...)` is at line 9 in the dedented source. + # last handler's `raise e` is at line 11. + # Both must be in the findings after the visit_Try fix. + assert 9 in raise_lines, ( + f"visit_Try walker bug: middle except handler's raise at line 9 " + f"is not in findings. Got raise lines: {raise_lines}" + ) + assert 11 in raise_lines, ( + f"last handler's raise at line 11 must also be detected. " + f"Got raise lines: {raise_lines}" + ) + + +# --------------------------------------------------------------------------- +# Bug 2: render_json filters out INTERNAL_COMPLIANT findings in non-verbose mode +# --------------------------------------------------------------------------- +def test_render_json_includes_compliant_findings(): + """INTERNAL_COMPLIANT findings must appear in the per-file list. + + Per docs/reports/RESULT_MIGRATION_REVIEW_PASS_20260617.md section 4.4 #2: + The render_json filter `if f.category in VIOLATION_CATEGORIES or + f.category in ("UNCLEAR", "INTERNAL_RETHROW")` excludes + INTERNAL_COMPLIANT findings from the per-file list in non-verbose mode. + + The fix: include all findings in the per-file list (totals are right; + per-file list should match). + """ + src = ''' + def func(items, target): + try: + idx = items.index(target) + except ValueError: + idx = 0 + return idx + ''' + data = _run_audit_on_fixture(src) + findings = _classifications_for_file(data, "audit_bugfix_fixture.py") + categories = [f.get("category") for f in findings] + # The list.index/ValueError fallback is INTERNAL_COMPLIANT per the + # heuristic added in the review pass. It must appear in the per-file list. + assert "INTERNAL_COMPLIANT" in categories, ( + f"INTERNAL_COMPLIANT finding is filtered out of the per-file list. " + f"Got categories: {categories}" + ) + + +# --------------------------------------------------------------------------- +# Bug 3: render_json truncates per-file list to --top (default 15) +# --------------------------------------------------------------------------- +def test_render_json_no_truncation_with_20_files(): + """The per-file list is NOT truncated to top 15 by default. + + Per docs/reports/RESULT_MIGRATION_REVIEW_PASS_20260617.md section 4.4 #3: + The per-file list is truncated to top 15 by default. With 20 files, + the 5 lowest-ranked files are excluded from the output. + + The fix: increase the default --top to >= total file count. + """ + # Create 20 files each with 1 violation. Run audit with default --top. + # All 20 files should appear in the per-file list. + # With the bug (top=15), only 15 of the 20 files appear. + import tempfile + tmpdir = Path(tempfile.mkdtemp(prefix="audit_bugfix_truncation_")) + try: + for i in range(20): + src = f''' + def func_{i:02d}(): + try: + return some_undefined() + except Exception as e: + logger.error("{{e}}") + return None + ''' + (tmpdir / f"truncation_{i:02d}.py").write_text(textwrap.dedent(src), encoding="utf-8") + result = subprocess.run( + [sys.executable, str(SCRIPT), "--json", "--src", str(tmpdir), + "--include-baseline"], + capture_output=True, + text=True, + check=False, + cwd=str(ROOT), + ) + if result.returncode not in (0, 1): + raise RuntimeError(f"audit failed: {result.stderr}") + data = json.loads(result.stdout) + filenames = [f["filename"] for f in data.get("files", [])] + truncation_files = [fn for fn in filenames if "truncation_" in fn] + finally: + for f in tmpdir.glob("*.py"): + f.unlink() + if tmpdir.exists(): + tmpdir.rmdir() + # All 20 files should be in the output. With the bug, only 15 appear. + assert len(truncation_files) == 20, ( + f"per-file list is truncated to top 15. Got {len(truncation_files)}/20 " + f"files: {truncation_files}" + )