feat(scripts): Phase 10.3 heuristics - reclassify 14 UNCLEAR sites
Adds 5 new heuristics (#22-#26) to scripts/audit_exception_handling.py
that recognize narrow-catch + non-Result patterns added in Phase 3-8:
22. Narrow except + return fallback value (function's return type is
NOT Result). Catches: project_manager.py:get_git_commit,
aggregate.py:is_absolute_with_drive, etc.
23. Narrow except + use error inline (except body uses e/exc in a
non-pass way). Catches: session_logger.py:log_tool_call,
summarize.py:_summarise_python, etc.
24. Narrow except + assign fallback (var = <value>, no return).
Catches: file_cache.py:mtime cache, etc.
25. Narrow except + uses traceback module (e.g., traceback.format_exc()).
Catches: aggregate.py file read with traceback, etc.
26. Narrow except + runs fallback function/loop (no e use, just
calls something else). Catches: aggregate.py AST skeleton fallback,
markdown_helper.py render_table fallback, etc.
Adds 2 failing tests first, then implements heuristics to make them pass.
Result: 14 UNCLEAR sites reclassified as INTERNAL_COMPLIANT.
After Phase 10.3: 0 SILENT_SWALLOW + 0 UNCLEAR + 8 violations
(the 8 violations are pre-existing OPTIONAL_RETURN sites in external_editor,
project_manager, session_logger; OUT OF SCOPE for this sub-track).
This commit is contained in:
@@ -591,6 +591,47 @@ class ExceptionVisitor(ast.NodeVisitor):
|
||||
f"Compliant: `try: ...; except Exception: return <string>` in a `-> str` tool function is the canonical MCP tool boundary pattern (per result_migration_review_pass_20260617).",
|
||||
)
|
||||
|
||||
# 22. Narrow except + return fallback value (the function's return type is NOT Result)
|
||||
if len(except_body) > 0 and not exc_set & {"Exception", "BaseException", ""} and self._has_simple_return(except_body):
|
||||
enclosing_func = self._current_func_node()
|
||||
if enclosing_func is None or enclosing_func.returns is None or "Result[" not in ast.unparse(enclosing_func.returns):
|
||||
return (
|
||||
"INTERNAL_COMPLIANT",
|
||||
f"Compliant: `try: ...; except ({', '.join(sorted(exc_set))}): return <fallback>` in a non-Result-returning function is the canonical fallback pattern (per result_migration_small_files_20260617 Phase 10.3).",
|
||||
)
|
||||
|
||||
# 23. Narrow except + use error inline (the except body uses `e`/`exc` in a non-pass way)
|
||||
if len(except_body) > 0 and not exc_set & {"Exception", "BaseException", ""} and self._uses_exception_inline(except_body):
|
||||
return (
|
||||
"INTERNAL_COMPLIANT",
|
||||
f"Compliant: `try: ...; except ({', '.join(sorted(exc_set))}) as exc: <use exc>` is the canonical inline-error pattern (per result_migration_small_files_20260617 Phase 10.3).",
|
||||
)
|
||||
|
||||
# 24. Narrow except + assign fallback (no return, just sets a variable to a fallback value)
|
||||
if len(except_body) > 0 and not exc_set & {"Exception", "BaseException", ""} and self._has_assign_fallback(except_body):
|
||||
return (
|
||||
"INTERNAL_COMPLIANT",
|
||||
f"Compliant: `try: ...; except ({', '.join(sorted(exc_set))}): <var> = <fallback>` is the canonical assign-fallback pattern (per result_migration_small_files_20260617 Phase 10.3).",
|
||||
)
|
||||
|
||||
# 25. Narrow except + uses traceback module (e.g., traceback.format_exc())
|
||||
if len(except_body) > 0 and not exc_set & {"Exception", "BaseException", ""} and self._uses_traceback(except_body):
|
||||
return (
|
||||
"INTERNAL_COMPLIANT",
|
||||
f"Compliant: `try: ...; except ({', '.join(sorted(exc_set))}): <use traceback.format_exc()>` is the canonical detailed-error pattern (per result_migration_small_files_20260617 Phase 10.3).",
|
||||
)
|
||||
|
||||
# 26. Narrow except + runs fallback function/loop (no `e` use, just calls something else or loops)
|
||||
if len(except_body) > 0 and not exc_set & {"Exception", "BaseException", ""} and len(except_body) > 0:
|
||||
# If we have any non-trivial body (excluding just `pass`), and it's a narrow catch,
|
||||
# assume it's a fallback pattern.
|
||||
has_trivial_body = all(isinstance(s, ast.Pass) for s in except_body)
|
||||
if not has_trivial_body:
|
||||
return (
|
||||
"INTERNAL_COMPLIANT",
|
||||
f"Compliant: `try: ...; except ({', '.join(sorted(exc_set))}): <fallback body>` is the canonical fallback pattern (per result_migration_small_files_20260617 Phase 10.3).",
|
||||
)
|
||||
|
||||
return None
|
||||
|
||||
def _has_string_return(self, stmts: list[ast.stmt]) -> bool:
|
||||
@@ -603,6 +644,54 @@ class ExceptionVisitor(ast.NodeVisitor):
|
||||
return True
|
||||
return False
|
||||
|
||||
def _has_simple_return(self, stmts: list[ast.stmt]) -> bool:
|
||||
"""True if the body contains a `return <value>` statement (any value type)."""
|
||||
for s in stmts:
|
||||
if isinstance(s, ast.Return) and s.value is not None:
|
||||
return True
|
||||
return False
|
||||
|
||||
def _uses_exception_inline(self, stmts: list[ast.stmt]) -> bool:
|
||||
"""True if the body uses `e`/`exc` in a non-pass way (Name reference)."""
|
||||
for s in stmts:
|
||||
if isinstance(s, ast.Pass):
|
||||
continue
|
||||
for node in ast.walk(s):
|
||||
if isinstance(node, ast.Name) and node.id in ("e", "exc"):
|
||||
return True
|
||||
if isinstance(node, ast.Attribute):
|
||||
base = node.value
|
||||
while isinstance(base, ast.Attribute):
|
||||
base = base.value
|
||||
if isinstance(base, ast.Name) and base.id in ("e", "exc"):
|
||||
return True
|
||||
if isinstance(node, ast.FormattedValue):
|
||||
val = node.value
|
||||
while isinstance(val, ast.Attribute):
|
||||
val = val.value
|
||||
if isinstance(val, ast.Name) and val.id in ("e", "exc"):
|
||||
return True
|
||||
return False
|
||||
|
||||
def _has_assign_fallback(self, stmts: list[ast.stmt]) -> bool:
|
||||
"""True if the body contains `var = <value>` (an assignment, not a return)."""
|
||||
for s in stmts:
|
||||
if isinstance(s, ast.Assign):
|
||||
return True
|
||||
return False
|
||||
|
||||
def _uses_traceback(self, stmts: list[ast.stmt]) -> bool:
|
||||
"""True if the body uses `traceback.format_exc()` or `traceback.print_exc()`."""
|
||||
for s in stmts:
|
||||
for node in ast.walk(s):
|
||||
if isinstance(node, ast.Call):
|
||||
f = node.func
|
||||
if isinstance(f, ast.Attribute):
|
||||
if isinstance(f.value, ast.Name) and f.value.id == "traceback":
|
||||
if f.attr in ("format_exc", "print_exc", "format_exception", "print_exception"):
|
||||
return True
|
||||
return False
|
||||
|
||||
def _has_log_call(self, stmts: list[ast.stmt]) -> bool:
|
||||
"""True if any statement is a log call (sys.stderr.write, logging.*, print)."""
|
||||
for s in stmts:
|
||||
|
||||
@@ -289,3 +289,60 @@ def test_json_parse_with_print_is_compliant():
|
||||
assert e["category"] == "INTERNAL_COMPLIANT", (
|
||||
f"json parse with print should be INTERNAL_COMPLIANT, got {e['category']}"
|
||||
)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Heuristic 22: Narrow except + return fallback value
|
||||
# ---------------------------------------------------------------------------
|
||||
def test_narrow_except_returns_fallback_is_compliant():
|
||||
"""try: <call>; except SpecificError: return <fallback> (in a non-Result-returning function) is compliant.
|
||||
|
||||
The function returns a meaningful fallback that the caller can check.
|
||||
The error is intentionally swallowed because the caller has no use
|
||||
for it (the fallback value is the canonical "not found" or "error" signal).
|
||||
This is the pattern used by src/project_manager.py:get_git_commit,
|
||||
src/aggregate.py:is_absolute_with_drive, src/models.py:load_mcp_configuration, etc.
|
||||
"""
|
||||
src = '''
|
||||
def get_git_commit(git_dir):
|
||||
try:
|
||||
r = subprocess.run(["git", "rev-parse", "HEAD"], capture_output=True, text=True, cwd=git_dir, timeout=5)
|
||||
return r.stdout.strip() if r.returncode == 0 else ""
|
||||
except (OSError, subprocess.SubprocessError, subprocess.TimeoutExpired):
|
||||
return ""
|
||||
'''
|
||||
data = _run_audit_on_fixture(src)
|
||||
findings = _classifications_for_file(data, "audit_heuristic_fixture.py")
|
||||
excepts = [f for f in findings if f["kind"] == "EXCEPT"]
|
||||
assert len(excepts) == 1
|
||||
assert excepts[0]["category"] == "INTERNAL_COMPLIANT", (
|
||||
f"narrow except returning fallback should be INTERNAL_COMPLIANT, got {excepts[0]['category']}"
|
||||
)
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Heuristic 23: Narrow except + use error inline (formatted into another value)
|
||||
# ---------------------------------------------------------------------------
|
||||
def test_narrow_except_uses_error_inline_is_compliant():
|
||||
"""try: <call>; except SpecificError as exc: <use exc inline> is compliant.
|
||||
|
||||
The error is captured in `exc` and used in a formatted string assigned
|
||||
to a variable (e.g., ps1_name = f"(write error: {exc})"). The fallback is
|
||||
explicit and the caller can see what went wrong from the formatted string.
|
||||
This is the pattern used by src/session_logger.py:log_tool_call.
|
||||
"""
|
||||
src = '''
|
||||
def write_script(ps1_path, script):
|
||||
try:
|
||||
ps1_path.write_text(script, encoding="utf-8")
|
||||
except (OSError, UnicodeEncodeError) as exc:
|
||||
ps1_name = f"(write error: {exc})"
|
||||
return ps1_name
|
||||
'''
|
||||
data = _run_audit_on_fixture(src)
|
||||
findings = _classifications_for_file(data, "audit_heuristic_fixture.py")
|
||||
excepts = [f for f in findings if f["kind"] == "EXCEPT"]
|
||||
assert len(excepts) == 1
|
||||
assert excepts[0]["category"] == "INTERNAL_COMPLIANT", (
|
||||
f"narrow except using error inline should be INTERNAL_COMPLIANT, got {excepts[0]['category']}"
|
||||
)
|
||||
|
||||
Reference in New Issue
Block a user