Private
Public Access
fix(audit): tighten _is_fastapi_handler BOUNDARY_FASTAPI heuristic (Phase 7 Task 7.6+7.8)
The previous heuristic over-applied BOUNDARY_FASTAPI to ALL try/except inside _api_* handlers, regardless of whether the except body actually raises HTTPException. This was the laundering pattern that allowed L242 and L256 in _api_generate to be classified compliant while only doing sys.stderr.write. Per Phase 7 spec 22.5.5 (FR5), BOUNDARY_FASTAPI now requires: - The except body contains ast.Raise(exc=HTTPException(...)), OR - The except body contains return Result(...) Otherwise: - INTERNAL_SILENT_SWALLOW if the body has logging (the strict-violation case per error_handling.md:530 'logging is NOT a drain') - INTERNAL_COMPLIANT if the body returns Result New helpers: - _except_body_drains_via_http_exception_or_result(handler) - _except_body_has_logging(body) 5 regression-guard tests in tests/test_audit_heuristics.py lock the behavior so the heuristic does not regress the 13 BOUNDARY_FASTAPI sites in src/app_controller.py. TIER-2 READ conductor/code_styleguides/error_handling.md end-to-end before this commit.
This commit is contained in:
@@ -330,6 +330,57 @@ class ExceptionVisitor(ast.NodeVisitor):
|
||||
return True
|
||||
return False
|
||||
|
||||
def _except_body_drains_via_http_exception_or_result(self, handler: ast.ExceptHandler) -> bool:
|
||||
"""Phase 7 FR5: does the except body actually drain errors via
|
||||
`raise HTTPException(...)` or `return Result(...)`?
|
||||
|
||||
This is the canonical BOUNDARY_FASTAPI pattern: a `_api_*` handler
|
||||
must raise HTTPException (so the framework converts to HTTP response)
|
||||
or return a Result (propagated to a caller that raises HTTPException).
|
||||
|
||||
Per error_handling.md:534, BOUNDARY_FASTAPI only applies to actual
|
||||
HTTPException raises. Without this check, the heuristic over-applied
|
||||
to logging-only except bodies (e.g. `_api_generate` L242 and L256
|
||||
pre-Phase-7)."""
|
||||
for node in ast.walk(ast.Module(body=handler.body, type_ignores=[])):
|
||||
# 1. raise HTTPException(...)
|
||||
if isinstance(node, ast.Raise) and node.exc is not None:
|
||||
exc = node.exc
|
||||
if isinstance(exc, ast.Call) and isinstance(exc.func, ast.Name):
|
||||
if exc.func.id == "HTTPException":
|
||||
return True
|
||||
if isinstance(exc, ast.Call) and isinstance(exc.func, ast.Attribute):
|
||||
if exc.func.attr == "HTTPException":
|
||||
return True
|
||||
# 2. return Result(...)
|
||||
if isinstance(node, ast.Return) and node.value is not None:
|
||||
if isinstance(node.value, ast.Call):
|
||||
func = node.value.func
|
||||
if isinstance(func, ast.Name) and func.id == "Result":
|
||||
return True
|
||||
if isinstance(func, ast.Attribute) and func.attr == "Result":
|
||||
return True
|
||||
return False
|
||||
|
||||
def _except_body_has_logging(self, body: list) -> bool:
|
||||
"""Phase 7 FR5: does the except body contain logging (debug/log/warn/error)
|
||||
or print/sys.stderr.write calls?
|
||||
|
||||
Used to distinguish INTERNAL_SILENT_SWALLOW (logging-only, violation)
|
||||
from INTERNAL_COMPLIANT (try/finally cleanup or empty body)."""
|
||||
for node in ast.walk(ast.Module(body=body, type_ignores=[])):
|
||||
if isinstance(node, ast.Call):
|
||||
func = node.func
|
||||
func_str = ast.unparse(func)
|
||||
# logging.getLogger(...).debug/log/info/warn/error or just print
|
||||
if ".debug(" in func_str or ".info(" in func_str or ".warning(" in func_str or ".error(" in func_str:
|
||||
return True
|
||||
if ".log(" in func_str:
|
||||
return True
|
||||
if func_str == "print" or "sys.stderr.write" in func_str:
|
||||
return True
|
||||
return False
|
||||
|
||||
def _classify_except(self, handler: ast.ExceptHandler, try_node: ast.Try) -> tuple[str, str]:
|
||||
exc_type = handler.type
|
||||
exc_name = ast.unparse(exc_type) if exc_type is not None else "Exception"
|
||||
@@ -391,10 +442,41 @@ class ExceptionVisitor(ast.NodeVisitor):
|
||||
)
|
||||
|
||||
# 2. FastAPI _api_* handler with broad catch (per app_controller pattern)
|
||||
# Phase 7 FR5: tightened to require the except body to actually raise
|
||||
# HTTPException or return a Result. Without this check, ALL nested
|
||||
# try/except inside `_api_*` handlers were classified BOUNDARY_FASTAPI
|
||||
# even when the body only logged to stderr (the very pattern Phase 6
|
||||
# was supposed to eliminate per error_handling.md:530 "logging is NOT a drain").
|
||||
if self._is_fastapi_handler() and exc_name in ("Exception", "BaseException", ""):
|
||||
if self._except_body_drains_via_http_exception_or_result(handler):
|
||||
return (
|
||||
"BOUNDARY_FASTAPI",
|
||||
"Compliant: FastAPI _api_* handler catches and converts to HTTPException at the framework boundary. This is the FastAPI-idiomatic pattern.",
|
||||
)
|
||||
# Re-classify: the `_api_*` name heuristic does NOT justify
|
||||
# classifying logging-only or Result-returning as BOUNDARY_FASTAPI.
|
||||
# The user's principle (error_handling.md:530) requires a real drain.
|
||||
if is_silent or self._except_body_has_logging(body):
|
||||
return (
|
||||
"INTERNAL_SILENT_SWALLOW",
|
||||
f"Strict-violation (Phase 7 FR5): _api_* handler's except body only "
|
||||
f"logs/prints (no HTTPException raise, no Result return). Per "
|
||||
f"error_handling.md:530 'logging is NOT a drain'. Migrate to "
|
||||
f"Result[T] propagation with a real drain point.",
|
||||
)
|
||||
if self._returns_result(body):
|
||||
return (
|
||||
"INTERNAL_COMPLIANT",
|
||||
"Compliant: _api_* handler's except body returns Result[data=..., errors=[...]] (Phase 6+ canonical pattern).",
|
||||
)
|
||||
# Default to internal_silent_swallow (logging-only fallback) for
|
||||
# safety; the heuristic tightened check already excluded the
|
||||
# logging-only case via the `_except_body_has_logging` branch above.
|
||||
return (
|
||||
"BOUNDARY_FASTAPI",
|
||||
"Compliant: FastAPI _api_* handler catches and converts to HTTPException at the framework boundary. This is the FastAPI-idiomatic pattern.",
|
||||
"INTERNAL_SILENT_SWALLOW",
|
||||
"Strict-violation (Phase 7 FR5): _api_* handler's except body does not "
|
||||
"raise HTTPException or return Result. Per error_handling.md:530, "
|
||||
"logging is NOT a drain. Migrate to Result[T] propagation.",
|
||||
)
|
||||
|
||||
# 3. Inside a *_result function with broad catch (likely SDK boundary)
|
||||
|
||||
Reference in New Issue
Block a user