From ba3b56e7e6003b1736442b5a70ac64a63e220bc5 Mon Sep 17 00:00:00 2001 From: Team Pixel Date: Fri, 24 Jul 2026 22:07:37 +0000 Subject: [PATCH] Surface errors instead of swallowing them Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- README.md | 3 +- run_demo.py | 27 ++-- src/analyzer.py | 121 ++++++++++++------ src/cli.py | 114 ++++++++++++----- src/code_fetcher.py | 72 +++++++++-- src/config.py | 31 ++++- src/differential.py | 19 ++- src/engine/api.py | 19 ++- src/engine/models.py | 9 ++ src/errors.py | 30 +++++ src/parser.py | 8 +- src/report_generator.py | 8 +- src/spec_fetcher.py | 93 +++++++++++--- src/verifier.py | 17 +++ tests/test_error_handling.py | 240 +++++++++++++++++++++++++++++++++++ 15 files changed, 694 insertions(+), 117 deletions(-) create mode 100644 src/errors.py create mode 100644 tests/test_error_handling.py diff --git a/README.md b/README.md index 8ab82e2..9386c2a 100644 --- a/README.md +++ b/README.md @@ -413,7 +413,8 @@ The returned dict has the structure: "tool_version": "1.5.0", "ruleset": "ethereum", "findings": [{"id", "severity", "title", "message", "file", "line", "hint"}], - "summary": {"high": 0, "med": 0, "low": 0, "info": 5, "files_scanned": 12} + "summary": {"high": 0, "med": 0, "low": 0, "info": 5, "files_scanned": 12, "files_skipped": 1}, + "errors": [{"file": "vendor/unreadable.go", "error": "Permission denied"}] } ``` diff --git a/run_demo.py b/run_demo.py index f96c914..4db7bdc 100644 --- a/run_demo.py +++ b/run_demo.py @@ -131,8 +131,12 @@ def print_banner(): print(BANNER) -def run_demo(eip_number: int = 1559, client: str = "go-ethereum"): - """Run a demonstration of PRSpec capabilities for any supported EIP.""" +def run_demo(eip_number: int = 1559, client: str = "go-ethereum") -> bool: + """Run a demonstration of PRSpec capabilities for any supported EIP. + + Returns ``True`` when the demo completed and ``False`` when it bailed out, + so the process exit status reflects what actually happened. + """ print_banner() # Import PRSpec components @@ -146,7 +150,7 @@ def run_demo(eip_number: int = 1559, client: str = "go-ethereum"): except ImportError as e: print(f"Import error: {e}") print("Make sure you've installed requirements: pip install -r requirements.txt") - return + return False # Check for Rich library try: @@ -166,7 +170,7 @@ def run_demo(eip_number: int = 1559, client: str = "go-ethereum"): print(f" ✓ Config loaded from: {config.config_path}") except Exception as e: print(f" Error loading config: {e}") - return + return False # Check API key print("\nChecking API credentials...") @@ -176,7 +180,7 @@ def run_demo(eip_number: int = 1559, client: str = "go-ethereum"): except ValueError as e: print(f" {e}") print(" Please set GEMINI_API_KEY in your .env file") - return + return False # Validate EIP support spec_fetcher = SpecFetcher(github_token=config.github_token) @@ -186,7 +190,7 @@ def run_demo(eip_number: int = 1559, client: str = "go-ethereum"): if eip_number not in spec_fetcher.supported_eips(): print(f" EIP-{eip_number} is not in the registry. " f"Supported: {spec_fetcher.supported_eips()}") - return + return False eip_title = spec_fetcher.get_eip_title(eip_number) print(f"\nTarget: EIP-{eip_number} ({eip_title}) -- {client}") @@ -214,7 +218,7 @@ def run_demo(eip_number: int = 1559, client: str = "go-ethereum"): print(f" ✓ Consensus spec: {len(spec_data['consensus_spec'])} characters") except Exception as e: print(f" Error fetching spec: {e}") - return + return False # Fetch client implementation print(f"\nFetching {client} implementation...") @@ -235,7 +239,7 @@ def run_demo(eip_number: int = 1559, client: str = "go-ethereum"): code_files = SAMPLE_CODE[eip_number] else: print(" No sample code available for this EIP. Exiting.") - return + return False # Parse the code language = code_fetcher.client_language(client) @@ -342,6 +346,7 @@ def _analyze_file(file_path, code_content): print("=" * 60) print("\nReports are in the 'output' directory.") print() + return True def quick_test(): @@ -387,6 +392,8 @@ def quick_test(): args = arg_parser.parse_args() if args.test: - quick_test() + ok = quick_test() else: - run_demo(eip_number=args.eip, client=args.client) + ok = run_demo(eip_number=args.eip, client=args.client) + + sys.exit(0 if ok else 1) diff --git a/src/analyzer.py b/src/analyzer.py index 2ae2b4d..e449b98 100644 --- a/src/analyzer.py +++ b/src/analyzer.py @@ -1,6 +1,7 @@ """LLM-based spec compliance analysis (Gemini, OpenAI, and Azure AI backends).""" import json +import logging import re import time from abc import ABC, abstractmethod @@ -9,6 +10,10 @@ import requests +from .errors import AnalysisError + +logger = logging.getLogger(__name__) + @dataclass class AnalysisResult: @@ -18,14 +23,24 @@ class AnalysisResult: issues: List[Dict[str, Any]] summary: str raw_response: Optional[str] = None + # Set when the analysis failed; carries the underlying error so callers + # (reports, CLI exit codes) can distinguish a failure from a verdict. + error: Optional[str] = None def to_dict(self) -> Dict[str, Any]: - return { + data: Dict[str, Any] = { "status": self.status, "confidence": self.confidence, "issues": self.issues, "summary": self.summary, } + if self.error: + data["error"] = self.error + return data + + @property + def failed(self) -> bool: + return self.status == "ERROR" @property def has_issues(self) -> bool: @@ -161,6 +176,16 @@ def _parse_json_response(self, response_text: str) -> Dict[str, Any]: # Strip surrounding prose — some models wrap JSON in explanation text text = text.strip() + if not text: + logger.error("LLM returned an empty response") + return { + "status": "ERROR", + "confidence": 0, + "issues": [], + "summary": "The model returned an empty response.", + "error": "empty response", + } + try: return json.loads(text) except json.JSONDecodeError: @@ -206,14 +231,32 @@ def _parse_json_response(self, response_text: str) -> Dict[str, Any]: except json.JSONDecodeError: continue + logger.error( + "Could not parse a JSON object out of a %d-char model response; " + "first 200 chars: %s", len(text), text[:200], + ) return { "status": "ERROR", "confidence": 0, "issues": [], "summary": f"Failed to parse response ({len(text)} chars). " - f"The model output may have been truncated." + f"The model output may have been truncated.", + "error": "unparseable response", } + @staticmethod + def _result_from_payload(result: Dict[str, Any], + raw_response: Optional[str]) -> "AnalysisResult": + """Build an :class:`AnalysisResult` from a parsed model payload.""" + return AnalysisResult( + status=result.get("status", "UNCERTAIN"), + confidence=result.get("confidence", 0), + issues=result.get("issues", []), + summary=result.get("summary", ""), + raw_response=raw_response, + error=result.get("error"), + ) + class GeminiAnalyzer(BaseAnalyzer): """Gemini-backed analyzer. Uses the large context window to compare @@ -225,8 +268,10 @@ def __init__(self, api_key: str, model: str = "gemini-2.5-pro", try: from google import genai # type: ignore[import-untyped] from google.genai import types as genai_types # type: ignore[import-untyped] - except ImportError: - raise ImportError("google-genai not installed. Run: pip install google-genai") + except ImportError as e: + raise ImportError( + "google-genai not installed. Run: pip install google-genai" + ) from e self._genai_types = genai_types self.client = genai.Client(api_key=api_key) @@ -249,22 +294,25 @@ def analyze_compliance(self, spec_text: str, code_text: str, ), ) - result = self._parse_json_response(response.text) + if response.text is None: + raise AnalysisError( + "Gemini returned no text (the response may have been " + "blocked or truncated)" + ) - return AnalysisResult( - status=result.get("status", "UNCERTAIN"), - confidence=result.get("confidence", 0), - issues=result.get("issues", []), - summary=result.get("summary", ""), - raw_response=response.text - ) + result = self._parse_json_response(response.text) + return self._result_from_payload(result, response.text) except Exception as e: + logger.exception( + "Gemini analysis of %s failed", context.get("file_name", "") + ) return AnalysisResult( status="ERROR", confidence=0, issues=[], - summary=f"Gemini analysis failed: {str(e)}" + summary=f"Gemini analysis failed: {str(e)}", + error=f"{type(e).__name__}: {e}", ) def get_model_info(self) -> Dict[str, Any]: @@ -286,8 +334,8 @@ def __init__(self, api_key: str, model: str = "gpt-4-turbo-preview", try: from openai import OpenAI self.client = OpenAI(api_key=api_key) - except ImportError: - raise ImportError("openai not installed. Run: pip install openai") + except ImportError as e: + raise ImportError("openai not installed. Run: pip install openai") from e self.model = model self.max_tokens = max_tokens @@ -316,22 +364,19 @@ def analyze_compliance(self, spec_text: str, code_text: str, ) response_text = response.choices[0].message.content - result = self._parse_json_response(response_text) - - return AnalysisResult( - status=result.get("status", "UNCERTAIN"), - confidence=result.get("confidence", 0), - issues=result.get("issues", []), - summary=result.get("summary", ""), - raw_response=response_text - ) + result = self._parse_json_response(response_text or "") + return self._result_from_payload(result, response_text) except Exception as e: + logger.exception( + "OpenAI analysis of %s failed", context.get("file_name", "") + ) return AnalysisResult( status="ERROR", confidence=0, issues=[], - summary=f"OpenAI analysis failed: {str(e)}" + summary=f"OpenAI analysis failed: {str(e)}", + error=f"{type(e).__name__}: {e}", ) def get_model_info(self) -> Dict[str, Any]: @@ -398,7 +443,7 @@ def _post_with_retry(self, body: dict, headers: dict) -> requests.Response: ``HTTPError`` if every attempt is exhausted. """ last_exc: Optional[Exception] = None - for attempt in range(self.max_retries + 1): + for attempt in range(max(self.max_retries, 0) + 1): response = self.session.post( self.endpoint, json=body, headers=headers, timeout=120 ) @@ -415,10 +460,17 @@ def _post_with_retry(self, body: dict, headers: dict) -> requests.Response: retry_after = response.headers.get("Retry-After") try: delay = float(retry_after) if retry_after is not None else 2.0 ** attempt - except ValueError: + except (TypeError, ValueError): + logger.warning("Ignoring malformed Retry-After header: %r", retry_after) delay = 2.0 ** attempt + logger.warning( + "Azure AI returned %s; retrying in %.1fs (attempt %d/%d)", + response.status_code, delay, attempt + 1, self.max_retries, + ) time.sleep(delay) + if last_exc is None: # pragma: no cover - defensive + raise AnalysisError("Azure AI request loop exited without a response") raise last_exc def analyze_compliance(self, spec_text: str, code_text: str, @@ -452,21 +504,18 @@ def analyze_compliance(self, spec_text: str, code_text: str, if block.get("type") == "text" ) result = self._parse_json_response(response_text) - - return AnalysisResult( - status=result.get("status", "UNCERTAIN"), - confidence=result.get("confidence", 0), - issues=result.get("issues", []), - summary=result.get("summary", ""), - raw_response=response_text - ) + return self._result_from_payload(result, response_text) except Exception as e: + logger.exception( + "Azure AI analysis of %s failed", context.get("file_name", "") + ) return AnalysisResult( status="ERROR", confidence=0, issues=[], - summary=f"Azure AI analysis failed: {str(e)}" + summary=f"Azure AI analysis failed: {str(e)}", + error=f"{type(e).__name__}: {e}", ) def get_model_info(self) -> Dict[str, Any]: diff --git a/src/cli.py b/src/cli.py index 9bf9dbe..e7d22e2 100644 --- a/src/cli.py +++ b/src/cli.py @@ -1,8 +1,9 @@ """Command-line interface for PRSpec.""" +import logging from concurrent.futures import ThreadPoolExecutor, as_completed from datetime import datetime -from typing import Optional +from typing import List, Optional import click @@ -25,6 +26,8 @@ from .report_generator import ReportGenerator, ReportMetadata from .spec_fetcher import SpecFetcher +logger = logging.getLogger(__name__) + BANNER = """[cyan] ██████╗ ██████╗ ███████╗██████╗ ███████╗ ██████╗ ██╔══██╗██╔══██╗██╔════╝██╔══██╗██╔════╝██╔════╝ @@ -42,6 +45,39 @@ def cli(): pass +def _configure_logging(verbose: bool = False) -> None: + """Send library warnings to stderr so degraded runs are visible.""" + logging.basicConfig( + level=logging.DEBUG if verbose else logging.WARNING, + format="%(levelname)s %(name)s: %(message)s", + ) + + +def _warn(message: str) -> None: + """Print a warning that is visible with or without rich.""" + if RICH_AVAILABLE: + console.print(f"[yellow]Warning:[/yellow] {message}") + else: + click.echo(f"Warning: {message}", err=True) + + +def _fail(e: Exception, verbose: bool = False) -> None: + """Report a command failure and abort with a non-zero exit status.""" + logger.debug("Command failed", exc_info=True) + if RICH_AVAILABLE: + console.print(f"[red]Error:[/red] {e}") + else: + click.echo(f"Error: {e}", err=True) + if verbose: + import traceback + detail = traceback.format_exc() + if RICH_AVAILABLE: + console.print(f"[dim]{detail}[/dim]") + else: + click.echo(detail, err=True) + raise click.Abort() + + def _analyze_one_file(analyzer, spec_text, file_path, code_content, context): """Analyze a single file — designed to run inside a thread pool.""" result = analyzer.analyze_compliance(spec_text, code_content, context) @@ -88,9 +124,14 @@ def _run_analysis(eip: int, client: str, cfg, llm_provider: str, # Prefer the focused fork-to-fork diff; falls back to the full spec file. spec_data = spec_fetcher.fetch_eip_spec(eip, mode="diff") eip_title = spec_data.get("title", f"EIP-{eip}") + for warning in spec_data.get("warnings", []): + _warn(warning) # --- Fetch implementation code (generic for any EIP) --- - code_files = code_fetcher.fetch_eip_implementation(client, eip) + outcome = code_fetcher.fetch_eip_files(client, eip) + code_files = outcome.files + for path, err in outcome.failures.items(): + _warn(f"{client}: skipping {path} — could not fetch it ({err})") language = CodeFetcher.client_language(client) # --- Build analyzer --- @@ -135,6 +176,8 @@ def _run_analysis(eip: int, client: str, cfg, llm_provider: str, file_order = list(code_files.keys()) results.sort(key=lambda r: file_order.index(r["file_name"])) + _report_analysis_errors(results, client, eip) + # --- Adversarial verification (optional) --- if verify: from .verifier import VerificationEngine @@ -151,6 +194,29 @@ def _run_analysis(eip: int, client: str, cfg, llm_provider: str, return results, analyzer +def _report_analysis_errors(results: List[dict], client: str, eip: int) -> None: + """Surface per-file analysis failures; abort when every file failed. + + Files whose analysis errored carry no verdict, so folding them into the + report as UNCERTAIN would hide the failure. + """ + failed = [r for r in results if r.get("status") == "ERROR"] + if not failed: + return + + for result in failed: + _warn( + f"analysis of {result['file_name']} failed: " + f"{result.get('error') or result.get('summary', 'unknown error')}" + ) + + if len(failed) == len(results): + raise click.ClickException( + f"Every file analysis failed for EIP-{eip} on {client}; " + f"no report was produced." + ) + + @cli.command() @click.option('--eip', '-e', default=1559, help='EIP number to check (default: 1559)') @click.option('--client', '-c', default='go-ethereum', help='Client to analyze (default: go-ethereum)') @@ -173,6 +239,7 @@ def analyze(eip: int, client: str, provider: Optional[str], output: str, prspec analyze --eip 4844 --client go-ethereum --output html prspec analyze --eip 1559 --client nethermind --verify """ + _configure_logging(verbose) try: # Load configuration cfg = Config(config) @@ -249,18 +316,10 @@ def on_file_done(fname): else: click.echo(f"\nReport saved to: {report_path}") + except click.ClickException: + raise except Exception as e: - if RICH_AVAILABLE: - console.print(f"[red]Error:[/red] {str(e)}") - if verbose: - import traceback - console.print(f"[dim]{traceback.format_exc()}[/dim]") - else: - click.echo(f"Error: {str(e)}", err=True) - if verbose: - import traceback - click.echo(traceback.format_exc(), err=True) - raise click.Abort() + _fail(e, verbose) @cli.command() @@ -289,6 +348,7 @@ def diff(eip: int, clients: Optional[str], provider: Optional[str], output: str, """ from .differential import ClientAnalysis, DifferentialEngine + _configure_logging(verbose) try: cfg = Config(config) llm_provider = provider if provider else cfg.llm_provider @@ -371,17 +431,7 @@ def diff(eip: int, clients: Optional[str], provider: Optional[str], output: str, except click.ClickException: raise except Exception as e: - if RICH_AVAILABLE: - console.print(f"[red]Error:[/red] {str(e)}") - if verbose: - import traceback - console.print(f"[dim]{traceback.format_exc()}[/dim]") - else: - click.echo(f"Error: {str(e)}", err=True) - if verbose: - import traceback - click.echo(traceback.format_exc(), err=True) - raise click.Abort() + _fail(e, verbose) @cli.command() @@ -406,7 +456,7 @@ def fetch_spec(eip: int): click.echo("\n...[Truncated]") except Exception as e: - click.echo(f"Error: {str(e)}", err=True) + _fail(e) @cli.command() @@ -420,9 +470,13 @@ def list_files(client: str, eip: int): prspec list-files --client go-ethereum --eip 1559 prspec list-files --client go-ethereum --eip 4844 """ + _configure_logging() try: code_fetcher = CodeFetcher() - files = code_fetcher.fetch_eip_implementation(client, eip) + outcome = code_fetcher.fetch_eip_files(client, eip) + files = outcome.files + for path, err in outcome.failures.items(): + _warn(f"could not fetch {path}: {err}") if RICH_AVAILABLE: from rich.table import Table @@ -441,7 +495,7 @@ def list_files(client: str, eip: int): click.echo(f" - {path} ({len(content.split(chr(10)))} lines)") except Exception as e: - click.echo(f"Error: {str(e)}", err=True) + _fail(e) @cli.command() @@ -474,7 +528,7 @@ def list_eips(): click.echo(f" EIP-{eip_num}: {title}") except Exception as e: - click.echo(f"Error: {str(e)}", err=True) + _fail(e) @cli.command() @@ -493,7 +547,7 @@ def clear_cache(): click.echo("Cache cleared successfully") except Exception as e: - click.echo(f"Error: {str(e)}", err=True) + _fail(e) @cli.command() @@ -555,7 +609,7 @@ def check_config(): click.echo(f" {name}: {status}") except Exception as e: - click.echo(f"Error: {str(e)}", err=True) + _fail(e) def main(): diff --git a/src/code_fetcher.py b/src/code_fetcher.py index c0e10dd..41d5a81 100644 --- a/src/code_fetcher.py +++ b/src/code_fetcher.py @@ -1,17 +1,36 @@ """Fetches implementation files from Ethereum client repos (geth, Nethermind, Besu).""" +import logging import tempfile +from dataclasses import dataclass, field from pathlib import Path from typing import Any, Dict, List, Optional import requests +from .errors import CodeFetchError + try: from git import Repo GIT_AVAILABLE = True except ImportError: GIT_AVAILABLE = False +logger = logging.getLogger(__name__) + + +@dataclass +class FetchOutcome: + """Files fetched for an EIP/client pair, plus the ones that failed. + + ``failures`` maps a file path to the error that prevented fetching it, so + callers can report partial results instead of silently analyzing a subset + of the implementation. + """ + + files: Dict[str, str] = field(default_factory=dict) + failures: Dict[str, str] = field(default_factory=dict) + class CodeFetcher: """Fetches code from Ethereum client implementations""" @@ -295,7 +314,10 @@ def fetch_file(self, owner: str, repo: str, path: str, cache_file = self.cache_dir / cache_key if use_cache and cache_file.exists(): - return cache_file.read_text(encoding="utf-8") + try: + return cache_file.read_text(encoding="utf-8") + except OSError as e: + logger.warning("Ignoring unreadable cache entry %s: %s", cache_file, e) # Use raw GitHub URL url = f"https://raw.githubusercontent.com/{owner}/{repo}/{branch}/{path}" @@ -303,7 +325,10 @@ def fetch_file(self, owner: str, repo: str, path: str, response.raise_for_status() content = response.text - cache_file.write_text(content, encoding="utf-8") + try: + cache_file.write_text(content, encoding="utf-8") + except OSError as e: + logger.warning("Could not write cache entry %s: %s", cache_file, e) return content @@ -314,8 +339,13 @@ def fetch_geth_file(self, path: str, branch: str = "master", # ---- Generic EIP implementation fetcher ---- - def fetch_eip_implementation(self, client: str, eip_number: int) -> Dict[str, str]: - """Fetch all registered implementation files for an EIP/client pair.""" + def fetch_eip_files(self, client: str, eip_number: int) -> FetchOutcome: + """Fetch the registered implementation files for an EIP/client pair. + + Returns both the files that were fetched and, per path, the error for + the ones that were not. Raises :class:`CodeFetchError` when no file + could be fetched at all — an empty analysis is a failure, not a result. + """ if client not in self.CLIENTS: raise ValueError( f"Unknown client: {client}. " @@ -338,15 +368,37 @@ def fetch_eip_implementation(self, client: str, eip_number: int) -> Dict[str, st owner, repo = url_parts[-2], url_parts[-1] branch = client_info.get("branch", "master") - files: Dict[str, str] = {} + outcome = FetchOutcome() for file_path in file_paths: try: - content = self.fetch_file(owner, repo, file_path, branch=branch) - files[file_path] = content - except requests.HTTPError as e: - files[file_path] = f"# Error fetching file: {e}" + outcome.files[file_path] = self.fetch_file( + owner, repo, file_path, branch=branch + ) + except requests.RequestException as e: + # Never hand the error text to the analyzer as if it were + # source code — record the failure and drop the file. + logger.warning( + "Failed to fetch %s from %s/%s@%s: %s", + file_path, owner, repo, branch, e, + ) + outcome.failures[file_path] = str(e) + + if not outcome.files: + detail = "; ".join(f"{p}: {err}" for p, err in outcome.failures.items()) + raise CodeFetchError( + f"Could not fetch any EIP-{eip_number} implementation file for " + f"{client} ({detail})" + ) + + return outcome + + def fetch_eip_implementation(self, client: str, eip_number: int) -> Dict[str, str]: + """Fetch all registered implementation files for an EIP/client pair. - return files + Files that cannot be fetched are omitted (and logged); see + :meth:`fetch_eip_files` to inspect those failures. + """ + return self.fetch_eip_files(client, eip_number).files # ---- Legacy convenience methods ---- diff --git a/src/config.py b/src/config.py index 1bb2dfd..fb52fea 100644 --- a/src/config.py +++ b/src/config.py @@ -1,5 +1,6 @@ """Configuration management for PRSpec.""" +import logging import os from pathlib import Path from typing import Any, Dict, Optional @@ -7,6 +8,10 @@ import yaml from dotenv import load_dotenv +from .errors import ConfigError + +logger = logging.getLogger(__name__) + class Config: """Configuration manager for PRSpec""" @@ -38,9 +43,29 @@ def _find_config_file(self) -> str: raise FileNotFoundError("config.yaml not found") def _load_config(self) -> Dict[str, Any]: - """Load configuration from YAML file""" - with open(self.config_path, 'r') as f: - return yaml.safe_load(f) + """Load configuration from YAML file. + + Raises :class:`ConfigError` when the file cannot be read or does not + contain a YAML mapping — an empty or malformed config would otherwise + surface much later as a confusing ``AttributeError``. + """ + try: + with open(self.config_path, 'r') as f: + loaded = yaml.safe_load(f) + except OSError as e: + raise ConfigError(f"Could not read config file {self.config_path}: {e}") from e + except yaml.YAMLError as e: + raise ConfigError(f"Invalid YAML in {self.config_path}: {e}") from e + + if loaded is None: + logger.warning("Config file %s is empty; using defaults", self.config_path) + return {} + if not isinstance(loaded, dict): + raise ConfigError( + f"Config file {self.config_path} must contain a mapping, " + f"got {type(loaded).__name__}" + ) + return loaded @property def llm_provider(self) -> str: diff --git a/src/differential.py b/src/differential.py index f6267ad..089d36b 100644 --- a/src/differential.py +++ b/src/differential.py @@ -12,11 +12,14 @@ from __future__ import annotations +import logging from dataclasses import dataclass, field from typing import Any, Dict, List, Optional from .verifier import confirmed_issues +logger = logging.getLogger(__name__) + # --------------------------------------------------------------------------- # Result summarisation (kept standalone so the engine has no dependency on the # report generator). @@ -54,7 +57,13 @@ def summarize_results(results: List[Dict[str, Any]], confidences.append(int(result.get("confidence", 0) or 0)) statuses.append(str(result.get("status", "UNKNOWN"))) - if "MISSING" in statuses or high > 0: + failed = sum(1 for s in statuses if s == "ERROR") + + if statuses and failed == len(statuses): + # Nothing was actually analyzed; reporting that as UNCERTAIN would + # present a failed run as an inconclusive one. + overall = "ANALYSIS FAILED" + elif "MISSING" in statuses or high > 0: overall = "ISSUES FOUND" elif "PARTIAL_MATCH" in statuses or med > 0: overall = "PARTIAL" @@ -65,6 +74,7 @@ def summarize_results(results: List[Dict[str, Any]], return { "overall_status": overall, + "failed_files": failed, "average_confidence": round(sum(confidences) / len(confidences)) if confidences else 0, "files_analyzed": len(results), "total_issues": total_issues, @@ -250,8 +260,15 @@ def synthesize(self, analyzer: Any, differential: DifferentialResult, "focus_areas": self.focus_areas, } result = analyzer.analyze_compliance(spec_text, findings_blob, context) + if getattr(result, "failed", False): + logger.warning( + "Differential synthesis unavailable: %s", + getattr(result, "error", None) or result.summary, + ) + return None return result.summary or None except Exception: + logger.exception("Differential synthesis failed for EIP-%s", differential.eip) return None # ---- row builders ---- diff --git a/src/engine/api.py b/src/engine/api.py index 6a11745..837aec0 100644 --- a/src/engine/api.py +++ b/src/engine/api.py @@ -11,16 +11,19 @@ from __future__ import annotations import json +import logging import os from pathlib import Path from typing import Any, Dict, List, Optional from ..parser import CodeParser +logger = logging.getLogger(__name__) + # Attempt to read the package version; fall back to "dev". try: from src import __version__ as _prspec_version -except Exception: # pragma: no cover +except ImportError: # pragma: no cover _prspec_version = "dev" # --------------------------------------------------------------------------- @@ -87,7 +90,8 @@ def _get_eip_keywords() -> Dict[int, List[str]]: try: from ..parser import CodeParser as _CP return dict(_CP.EIP_KEYWORDS) - except Exception: + except (ImportError, AttributeError) as e: + logger.warning("Falling back to the built-in EIP keyword map: %s", e) return dict(_DEFAULT_EIP_KEYWORDS) @@ -144,6 +148,7 @@ def scan_path( eip_keywords = _get_eip_keywords() findings: List[Dict[str, Any]] = [] + skipped: Dict[str, str] = {} finding_id = 0 for fpath in files: @@ -153,7 +158,9 @@ def scan_path( try: content = Path(fpath).read_text(errors="replace") - except OSError: + except OSError as e: + logger.warning("Skipping unreadable file %s: %s", fpath, e) + skipped[fpath] = str(e) continue blocks = parser.parse_file(content, lang) @@ -210,8 +217,12 @@ def scan_path( "med": med, "low": low, "info": info, - "files_scanned": len(files), + "files_scanned": len(files) - len(skipped), + "files_skipped": len(skipped), }, + # Per-file read errors, so a scan that silently covered less than the + # whole tree is visible to the caller. + "errors": [{"file": f, "error": err} for f, err in sorted(skipped.items())], } if output == "json-pretty": diff --git a/src/engine/models.py b/src/engine/models.py index 7dcc037..3b9f743 100644 --- a/src/engine/models.py +++ b/src/engine/models.py @@ -15,6 +15,13 @@ class Finding(TypedDict, total=False): hint: str +class ScanError(TypedDict): + """A file that could not be scanned, and why.""" + + file: str + error: str + + class Summary(TypedDict): """Aggregate counts for a scan run.""" @@ -23,6 +30,7 @@ class Summary(TypedDict): low: int info: int files_scanned: int + files_skipped: int class ScanResult(TypedDict): @@ -33,3 +41,4 @@ class ScanResult(TypedDict): ruleset: str findings: List[Finding] summary: Summary + errors: List[ScanError] diff --git a/src/errors.py b/src/errors.py new file mode 100644 index 0000000..66e9071 --- /dev/null +++ b/src/errors.py @@ -0,0 +1,30 @@ +"""Exception types raised by PRSpec. + +Every failure that is worth distinguishing from a generic ``Exception`` gets a +type here so callers (and the CLI) can react to it instead of guessing from a +message string. +""" + + +class PRSpecError(Exception): + """Base class for every PRSpec-specific failure.""" + + +class ConfigError(PRSpecError): + """The configuration file is missing, unreadable, or malformed.""" + + +class FetchError(PRSpecError): + """A remote artefact could not be retrieved.""" + + +class SpecFetchError(FetchError): + """No specification source could be fetched for an EIP.""" + + +class CodeFetchError(FetchError): + """No implementation file could be fetched for an EIP/client pair.""" + + +class AnalysisError(PRSpecError): + """An LLM backend returned a response that cannot be used.""" diff --git a/src/parser.py b/src/parser.py index f19685f..65eaf12 100644 --- a/src/parser.py +++ b/src/parser.py @@ -1,9 +1,12 @@ """Source code parser — extracts functions, classes, and EIP-relevant blocks.""" +import logging import re from dataclasses import dataclass from typing import Any, Dict, List, Optional +logger = logging.getLogger(__name__) + @dataclass class CodeBlock: @@ -58,8 +61,11 @@ def _init_tree_sitter(self): self._ts_parsers["python"] = Parser(py_lang) self._ts_parsers["go"] = Parser(go_lang) - except (ImportError, TypeError): + except (ImportError, TypeError) as e: # TypeError handles older tree-sitter API gracefully + logger.warning( + "tree-sitter unavailable (%s); falling back to regex parsing", e + ) self.use_tree_sitter = False def parse_file(self, content: str, language: str, diff --git a/src/report_generator.py b/src/report_generator.py index 539589a..e1ce625 100644 --- a/src/report_generator.py +++ b/src/report_generator.py @@ -416,8 +416,13 @@ def _generate_summary(self, results: List[Dict[str, Any]]) -> Dict[str, Any]: confidences.append(result.get('confidence', 0)) statuses.append(result.get('status', 'UNKNOWN')) + failed_files = sum(1 for s in statuses if s == 'ERROR') + # Determine overall status - if 'MISSING' in statuses or high_severity > 0: + if statuses and failed_files == len(statuses): + # Every file errored: that is a failed run, not an uncertain one. + overall_status = "ANALYSIS FAILED" + elif 'MISSING' in statuses or high_severity > 0: overall_status = "ISSUES FOUND" elif 'PARTIAL_MATCH' in statuses or medium_severity > 0: overall_status = "PARTIAL" @@ -430,6 +435,7 @@ def _generate_summary(self, results: List[Dict[str, Any]]) -> Dict[str, Any]: "overall_status": overall_status, "average_confidence": round(sum(confidences) / len(confidences)) if confidences else 0, "files_analyzed": len(results), + "failed_files": failed_files, "total_issues": total_issues, "high_severity": high_severity, "medium_severity": medium_severity, diff --git a/src/spec_fetcher.py b/src/spec_fetcher.py index 84f6fab..b76c9e1 100644 --- a/src/spec_fetcher.py +++ b/src/spec_fetcher.py @@ -1,11 +1,16 @@ """Fetches Ethereum EIP specs, execution specs, and consensus specs from GitHub.""" import difflib +import logging from pathlib import Path -from typing import Dict, List, Optional +from typing import Any, Dict, List, Optional import requests +from .errors import SpecFetchError + +logger = logging.getLogger(__name__) + class SpecFetcher: """Fetches Ethereum specifications from GitHub and other sources""" @@ -136,8 +141,9 @@ def fetch_eip(self, eip_number: int, use_cache: bool = True) -> str: cache_file = self.cache_dir / f"eip-{eip_number}.md" # Check cache - if use_cache and cache_file.exists(): - return cache_file.read_text(encoding="utf-8") + cached = self._read_cache(cache_file, use_cache) + if cached is not None: + return cached # Fetch from GitHub url = f"https://raw.githubusercontent.com/ethereum/EIPs/master/EIPS/eip-{eip_number}.md" @@ -145,26 +151,44 @@ def fetch_eip(self, eip_number: int, use_cache: bool = True) -> str: response.raise_for_status() content = response.text - - # Cache the result - cache_file.write_text(content, encoding="utf-8") + self._write_cache(cache_file, content) return content + # ---- Cache helpers (best-effort: a broken cache must not break a fetch) ---- + + @staticmethod + def _read_cache(cache_file: Path, use_cache: bool) -> Optional[str]: + if not (use_cache and cache_file.exists()): + return None + try: + return cache_file.read_text(encoding="utf-8") + except OSError as e: + logger.warning("Ignoring unreadable cache entry %s: %s", cache_file, e) + return None + + @staticmethod + def _write_cache(cache_file: Path, content: str) -> None: + try: + cache_file.write_text(content, encoding="utf-8") + except OSError as e: + logger.warning("Could not write cache entry %s: %s", cache_file, e) + def fetch_execution_spec(self, file_path: str, branch: str = "master", use_cache: bool = True) -> str: """Fetch a Python file from ethereum/execution-specs.""" cache_file = self.cache_dir / f"exec_spec_{file_path.replace('/', '_')}" - if use_cache and cache_file.exists(): - return cache_file.read_text(encoding="utf-8") + cached = self._read_cache(cache_file, use_cache) + if cached is not None: + return cached url = f"https://raw.githubusercontent.com/ethereum/execution-specs/{branch}/{file_path}" response = self.session.get(url) response.raise_for_status() content = response.text - cache_file.write_text(content, encoding="utf-8") + self._write_cache(cache_file, content) return content @@ -173,15 +197,16 @@ def fetch_consensus_spec(self, file_path: str, branch: str = "dev", """Fetch a file from ethereum/consensus-specs.""" cache_file = self.cache_dir / f"consensus_spec_{file_path.replace('/', '_')}" - if use_cache and cache_file.exists(): - return cache_file.read_text(encoding="utf-8") + cached = self._read_cache(cache_file, use_cache) + if cached is not None: + return cached url = f"https://raw.githubusercontent.com/ethereum/consensus-specs/{branch}/{file_path}" response = self.session.get(url) response.raise_for_status() content = response.text - cache_file.write_text(content, encoding="utf-8") + self._write_cache(cache_file, content) return content @@ -213,7 +238,11 @@ def fetch_execution_spec_diff(self, eip_number: int, branch: str = "master", try: new_src = self.fetch_execution_spec(new_path, branch, use_cache) old_src = self.fetch_execution_spec(old_path, branch, use_cache) - except (requests.HTTPError, requests.ConnectionError): + except requests.RequestException as e: + logger.warning( + "Fork diff for EIP-%s unavailable (%s vs %s): %s", + eip_number, new_path, old_path, e, + ) return None diff = difflib.unified_diff( @@ -239,12 +268,16 @@ def fetch_eip_spec(self, eip_number: int, mode: str = "full") -> Dict[str, str]: info = self.EIP_REGISTRY.get(eip_number, {}) title = info.get("title", f"EIP-{eip_number}") - result: Dict[str, Optional[str]] = { + result: Dict[str, Any] = { "eip_markdown": self.fetch_eip(eip_number), "execution_spec": None, "execution_spec_mode": "full", "consensus_spec": None, "title": title, + # Non-fatal degradations (e.g. a spec file that could not be + # fetched) so callers can tell the user the analysis is thinner + # than it should be instead of silently proceeding. + "warnings": [], } # Prefer the focused fork diff when asked for it. @@ -255,24 +288,44 @@ def fetch_eip_spec(self, eip_number: int, mode: str = "full") -> Dict[str, str]: result["execution_spec_mode"] = "diff" # Fall back to (or default to) the first whole spec file that fetches. + exec_paths = info.get("execution_spec_paths", []) if result["execution_spec"] is None: - for path in info.get("execution_spec_paths", []): + exec_errors: List[str] = [] + for path in exec_paths: try: result["execution_spec"] = self.fetch_execution_spec(path) break - except (requests.HTTPError, requests.ConnectionError): - continue + except requests.RequestException as e: + logger.warning("Could not fetch execution spec %s: %s", path, e) + exec_errors.append(f"{path}: {e}") + if result["execution_spec"] is None and exec_paths: + result["warnings"].append( + f"No execution spec available for EIP-{eip_number} " + f"({'; '.join(exec_errors)})" + ) # Try consensus spec paths; concatenate all that succeed + consensus_paths = info.get("consensus_spec_paths", []) consensus_parts: List[str] = [] - for path in info.get("consensus_spec_paths", []): + for path in consensus_paths: try: consensus_parts.append(self.fetch_consensus_spec(path)) - except (requests.HTTPError, requests.ConnectionError): - continue + except requests.RequestException as e: + logger.warning("Could not fetch consensus spec %s: %s", path, e) + result["warnings"].append(f"Consensus spec {path} unavailable: {e}") if consensus_parts: result["consensus_spec"] = "\n\n---\n\n".join(consensus_parts) + # An EIP with registered spec sources but nothing fetched leaves the + # analysis with prose only; that is a failure worth propagating. + if (exec_paths or consensus_paths) and not ( + result["execution_spec"] or result["consensus_spec"] + ): + raise SpecFetchError( + f"No specification source could be fetched for EIP-{eip_number}: " + + "; ".join(result["warnings"]) + ) + return result # ---- Legacy convenience methods ---- diff --git a/src/verifier.py b/src/verifier.py index a47eb31..544cf01 100644 --- a/src/verifier.py +++ b/src/verifier.py @@ -20,12 +20,15 @@ from __future__ import annotations +import logging import re from concurrent.futures import ThreadPoolExecutor, as_completed from dataclasses import dataclass, field from difflib import SequenceMatcher from typing import Any, Dict, List, Optional +logger = logging.getLogger(__name__) + # --------------------------------------------------------------------------- # Spec grounding (deterministic, no API) # --------------------------------------------------------------------------- @@ -207,6 +210,20 @@ def _one_round(self, refutation_spec: str, code_text: str, refutation_spec, code_text, context ) except Exception: + # A failed round must not masquerade as a considered verdict, so + # it votes 'unsure' — but it is never silent. + logger.exception( + "Skeptic round failed for %s; counting the vote as unsure", + context.get("file_name", ""), + ) + return "unsure" + + if getattr(result, "failed", False): + logger.warning( + "Skeptic round for %s returned an ERROR result: %s", + context.get("file_name", ""), + getattr(result, "error", None) or getattr(result, "summary", ""), + ) return "unsure" status = str(getattr(result, "status", "")).upper() diff --git a/tests/test_error_handling.py b/tests/test_error_handling.py new file mode 100644 index 0000000..4cf05f7 --- /dev/null +++ b/tests/test_error_handling.py @@ -0,0 +1,240 @@ +"""Tests for error propagation: failures must surface, not be swallowed.""" + +import json +import sys +import tempfile +import unittest +from pathlib import Path +from unittest.mock import Mock, patch + +import requests + +sys.path.insert(0, str(Path(__file__).parent.parent)) + +from src.analyzer import AnalysisResult, BaseAnalyzer # noqa: E402 +from src.code_fetcher import CodeFetcher # noqa: E402 +from src.config import Config # noqa: E402 +from src.differential import summarize_results # noqa: E402 +from src.engine import scan_path # noqa: E402 +from src.errors import CodeFetchError, ConfigError, SpecFetchError # noqa: E402 +from src.report_generator import ReportGenerator # noqa: E402 +from src.spec_fetcher import SpecFetcher # noqa: E402 +from src.verifier import VerificationEngine # noqa: E402 + + +class TestCodeFetcherErrors(unittest.TestCase): + """Fetch failures must never look like source code.""" + + def setUp(self): + self.tmp = tempfile.mkdtemp(prefix="prspec_err_code_") + self.fetcher = CodeFetcher(cache_dir=self.tmp) + + def test_failed_file_is_reported_not_inlined(self): + def fake_fetch(owner, repo, path, branch="master", use_cache=True): + if path.endswith("transaction.go"): + raise requests.HTTPError("404 Not Found") + return "package types" + + with patch.object(self.fetcher, "fetch_file", side_effect=fake_fetch): + outcome = self.fetcher.fetch_eip_files("go-ethereum", 1559) + + self.assertIn("core/types/transaction.go", outcome.failures) + self.assertNotIn("core/types/transaction.go", outcome.files) + for content in outcome.files.values(): + self.assertNotIn("Error fetching file", content) + + def test_all_files_failing_raises(self): + with patch.object(self.fetcher, "fetch_file", + side_effect=requests.ConnectionError("no network")): + with self.assertRaises(CodeFetchError): + self.fetcher.fetch_eip_files("go-ethereum", 1559) + + def test_connection_errors_are_collected_too(self): + """Non-HTTP request errors used to escape uncaught.""" + def fake_fetch(owner, repo, path, branch="master", use_cache=True): + if path.endswith("transaction.go"): + raise requests.ConnectionError("connection reset") + return "package types" + + with patch.object(self.fetcher, "fetch_file", side_effect=fake_fetch): + outcome = self.fetcher.fetch_eip_files("go-ethereum", 1559) + + self.assertIn("core/types/transaction.go", outcome.failures) + + def test_unreadable_cache_falls_back_to_network(self): + with patch.object(Path, "read_text", side_effect=OSError("permission denied")), \ + patch.object(self.fetcher.session, "get") as mock_get: + mock_get.return_value = Mock(text="fresh", raise_for_status=Mock()) + cache_file = Path(self.tmp) / "ethereum_go-ethereum_a.go_master" + cache_file.write_bytes(b"stale") + content = self.fetcher.fetch_file("ethereum", "go-ethereum", "a.go") + + self.assertEqual(content, "fresh") + + +class TestSpecFetcherErrors(unittest.TestCase): + def setUp(self): + self.tmp = tempfile.mkdtemp(prefix="prspec_err_spec_") + self.fetcher = SpecFetcher(cache_dir=self.tmp) + + def test_unfetchable_specs_raise(self): + with patch.object(self.fetcher, "fetch_eip", return_value="# EIP-1559"), \ + patch.object(self.fetcher, "fetch_execution_spec", + side_effect=requests.ConnectionError("no network")): + with self.assertRaises(SpecFetchError): + self.fetcher.fetch_eip_spec(1559) + + def test_partial_spec_failure_is_warned_about(self): + def fake_exec(path, branch="master", use_cache=True): + if "london" in path: + raise requests.HTTPError("404") + return "FORK FILE" + + with patch.object(self.fetcher, "fetch_eip", return_value="# EIP-1559"), \ + patch.object(self.fetcher, "fetch_execution_spec", side_effect=fake_exec): + result = self.fetcher.fetch_eip_spec(1559) + + self.assertEqual(result["execution_spec"], "FORK FILE") + self.assertEqual(result["warnings"], []) + + def test_consensus_failure_recorded_as_warning(self): + with patch.object(self.fetcher, "fetch_eip", return_value="# EIP-4844"), \ + patch.object(self.fetcher, "fetch_execution_spec", return_value="FORK"), \ + patch.object(self.fetcher, "fetch_consensus_spec", + side_effect=requests.HTTPError("404")): + result = self.fetcher.fetch_eip_spec(4844) + + self.assertTrue(result["warnings"]) + self.assertIsNone(result["consensus_spec"]) + + +class _StubAnalyzer(BaseAnalyzer): + """Concrete analyzer used to exercise the shared parsing helpers.""" + + def analyze_compliance(self, spec_text, code_text, context): + raise NotImplementedError + + +class TestAnalyzerErrorReporting(unittest.TestCase): + def test_error_result_carries_error_field(self): + result = AnalysisResult(status="ERROR", confidence=0, issues=[], + summary="boom", error="HTTPError: 500") + self.assertTrue(result.failed) + self.assertEqual(result.to_dict()["error"], "HTTPError: 500") + + def test_successful_result_has_no_error_key(self): + result = AnalysisResult(status="FULL_MATCH", confidence=90, + issues=[], summary="ok") + self.assertFalse(result.failed) + self.assertNotIn("error", result.to_dict()) + + def test_empty_model_response_is_an_error(self): + with self.assertLogs("src.analyzer", level="ERROR"): + parsed = _StubAnalyzer()._parse_json_response("") + self.assertEqual(parsed["status"], "ERROR") + self.assertIn("error", parsed) + + def test_unparseable_response_is_an_error(self): + with self.assertLogs("src.analyzer", level="ERROR"): + parsed = _StubAnalyzer()._parse_json_response("not json at all") + self.assertEqual(parsed["status"], "ERROR") + self.assertIn("error", parsed) + + def test_valid_response_still_parses(self): + payload = json.dumps({"status": "FULL_MATCH", "confidence": 95, + "issues": [], "summary": "fine"}) + parsed = _StubAnalyzer()._parse_json_response(payload) + self.assertEqual(parsed["status"], "FULL_MATCH") + + +class _ExplodingAnalyzer: + """Analyzer stub whose every call raises.""" + + def _build_refutation_prompt(self, finding, spec_text, context): + return spec_text + + def analyze_compliance(self, spec_text, code_text, context): + raise RuntimeError("backend down") + + +class TestVerifierErrorVisibility(unittest.TestCase): + def test_failed_round_votes_unsure_and_logs(self): + engine = VerificationEngine(_ExplodingAnalyzer(), rounds=2) + with self.assertLogs("src.verifier", level="ERROR"): + verdict = engine.verify_finding( + {"spec_reference": "base fee"}, "base fee spec", "code", {} + ) + self.assertEqual(verdict.votes["unsure"], 2) + + +class TestSummaryFailureVisibility(unittest.TestCase): + def test_all_error_results_are_not_uncertain(self): + results = [{"status": "ERROR", "confidence": 0, "issues": [], + "summary": "failed", "file_name": "a.go"}] + self.assertEqual(summarize_results(results)["overall_status"], + "ANALYSIS FAILED") + gen_summary = ReportGenerator(output_dir=tempfile.mkdtemp())._generate_summary(results) + self.assertEqual(gen_summary["overall_status"], "ANALYSIS FAILED") + self.assertEqual(gen_summary["failed_files"], 1) + + def test_partial_failure_keeps_verdict_but_counts_failures(self): + results = [ + {"status": "ERROR", "confidence": 0, "issues": [], "file_name": "a.go"}, + {"status": "FULL_MATCH", "confidence": 90, "issues": [], "file_name": "b.go"}, + ] + summary = summarize_results(results) + self.assertEqual(summary["failed_files"], 1) + self.assertNotEqual(summary["overall_status"], "ANALYSIS FAILED") + + +class TestConfigErrors(unittest.TestCase): + def test_malformed_yaml_raises_config_error(self): + path = Path(tempfile.mkdtemp()) / "config.yaml" + path.write_text("llm: [unclosed\n") + with self.assertRaises(ConfigError): + Config(str(path)) + + def test_non_mapping_config_raises_config_error(self): + path = Path(tempfile.mkdtemp()) / "config.yaml" + path.write_text("- just\n- a\n- list\n") + with self.assertRaises(ConfigError): + Config(str(path)) + + def test_empty_config_falls_back_to_defaults(self): + path = Path(tempfile.mkdtemp()) / "config.yaml" + path.write_text("") + cfg = Config(str(path)) + self.assertEqual(cfg.output_config.get("directory"), "output") + + +class TestEngineScanErrors(unittest.TestCase): + def test_unreadable_file_is_reported(self): + tmp = Path(tempfile.mkdtemp(prefix="prspec_err_engine_")) + (tmp / "ok.go").write_text("func CalcBaseFee() {}\n") + (tmp / "bad.go").write_text("func Other() {}\n") + + real_read_text = Path.read_text + + def flaky_read(self, *args, **kwargs): + if self.name == "bad.go": + raise OSError("permission denied") + return real_read_text(self, *args, **kwargs) + + with patch.object(Path, "read_text", flaky_read): + result = scan_path(str(tmp)) + + self.assertEqual(result["summary"]["files_skipped"], 1) + self.assertEqual(result["summary"]["files_scanned"], 1) + self.assertEqual(len(result["errors"]), 1) + self.assertIn("bad.go", result["errors"][0]["file"]) + + def test_clean_scan_reports_no_errors(self): + tmp = Path(tempfile.mkdtemp(prefix="prspec_err_engine_ok_")) + (tmp / "ok.go").write_text("func CalcBaseFee() {}\n") + result = scan_path(str(tmp)) + self.assertEqual(result["errors"], []) + self.assertEqual(result["summary"]["files_skipped"], 0) + + +if __name__ == "__main__": + unittest.main()