From f8fa4cd288282ed4a50f954d8593aa11343e9ccd Mon Sep 17 00:00:00 2001 From: Kadir Eren Altintas Date: Mon, 21 Sep 2026 17:50:21 +0300 Subject: [PATCH 1/7] stabilize CI and repository hygiene --- .github/workflows/ci.yml | 11 +++ README.md | 8 ++ alembic.ini | 2 +- docs/ci-health.md | 68 +++++++++++++ docs/evaluation.md | 6 +- patchfrog/analysis/analyzers/base.py | 26 +++++ patchfrog/analysis/analyzers/clang_tidy.py | 11 ++- patchfrog/analysis/analyzers/cppcheck.py | 11 ++- patchfrog/analysis/analyzers/ruff.py | 11 ++- patchfrog/analysis/analyzers/semgrep.py | 34 +++++-- patchfrog/ops/health.py | 9 +- patchfrog/publishing/planner.py | 45 ++++++++- patchfrog/publishing/service.py | 27 ++++++ .../test_analysis_run_concurrency.py | 22 +---- tests/integration/test_context_concurrency.py | 22 +---- .../integration/test_feedback_concurrency.py | 24 +---- .../test_publishing_concurrency.py | 22 +---- .../test_publishing_persistence.py | 33 ++----- .../test_publishing_service_dry_run.py | 33 +++++++ .../test_repository_index_concurrency.py | 25 +---- .../test_review_config_bundle_adversarial.py | 22 +---- .../test_review_context_bundle_cascade.py | 22 +---- ...st_review_memory_generation_concurrency.py | 22 +---- .../test_review_run_concurrency.py | 22 +---- tests/integration/test_toolchain_identity.py | 21 +--- tests/support/postgres.py | 41 ++++++++ tests/unit/test_analyzer_binary_resolution.py | 38 ++++++++ tests/unit/test_publishing_planner.py | 95 +++++++++++++++++++ 28 files changed, 482 insertions(+), 251 deletions(-) create mode 100644 docs/ci-health.md create mode 100644 tests/support/postgres.py create mode 100644 tests/unit/test_analyzer_binary_resolution.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index db5971e..caa0cd0 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -74,6 +74,12 @@ jobs: VgAFWZ9YEAZPVROqM6i76BU= -----END PRIVATE KEY----- GITHUB_WEBHOOK_SECRET: ci-placeholder-not-a-real-secret + # The database-specific suite must fail if the declared CI service is + # unavailable or unmigrated. Local runs may skip these tests when no + # compatible Postgres is present. + PATCHFROG_REQUIRE_POSTGRES: "1" + SEMGREP_ENABLE_VERSION_CHECK: "0" + SEMGREP_SEND_METRICS: "off" steps: - uses: actions/checkout@v4 @@ -88,6 +94,11 @@ jobs: - name: ruff run: ruff check . + - name: Verify required static analyzers + run: | + ruff --version + semgrep --version + - name: mypy --strict run: mypy . --strict diff --git a/README.md b/README.md index 2d4d46d..4a3a6e6 100644 --- a/README.md +++ b/README.md @@ -87,6 +87,14 @@ mypy . --strict pytest ``` +The default suite is deterministic and never requires provider credentials or +makes paid provider calls. Real-Postgres concurrency/schema tests skip locally +when the documented test database is unavailable; CI requires that database and +fails if it cannot be used. Host-isolation and distributed-verifier cases are +reported as optional skips when their explicit `bwrap`/`prlimit`/Redis +prerequisites are absent. See [`docs/ci-health.md`](docs/ci-health.md) for the +suite contracts and reproducible commands. + ## Architecture and brand See [`docs/brand.md`](docs/brand.md) for identity/tone guidelines and asset usage, [`docs/product-boundary.md`](docs/product-boundary.md) for the self-hosted vs. PatchFrog Cloud architecture, and the `docs/` directory for phase-by-phase design notes. diff --git a/alembic.ini b/alembic.ini index 003e661..f95b20a 100644 --- a/alembic.ini +++ b/alembic.ini @@ -1,7 +1,7 @@ [alembic] script_location = migrations prepend_sys_path = . -version_path_separator = os +path_separator = os [loggers] keys = root,sqlalchemy,alembic diff --git a/docs/ci-health.md b/docs/ci-health.md new file mode 100644 index 0000000..fc1b18f --- /dev/null +++ b/docs/ci-health.md @@ -0,0 +1,68 @@ +# CI health and test-suite contracts + +PatchFrog's default test path is deterministic: it uses fake/oracle providers, +recorded fixtures, local repositories, and bundled Semgrep rules. It does not +need provider credentials and must not make paid provider calls. + +## Suite classification + +| Suite | Contract | Main/CI expectation | +| --- | --- | --- | +| Deterministic local | Unit and SQLite-backed integration tests, fixture repositories, fake/oracle providers | Green | +| External static tools | Ruff and Semgrep are runtime dependencies; cppcheck and clang-tidy are optional system capabilities | CI verifies Ruff/Semgrep before tests; missing optional tools are reported unsupported | +| Real PostgreSQL | Advisory-lock, concurrency, uniqueness, and cascade tests against the migrated `patchfrog` database on localhost | Local skip only when unavailable; CI sets `PATCHFROG_REQUIRE_POSTGRES=1`, so unavailable/misconfigured/unmigrated is a failure | +| Host isolation | Tests requiring functional `bwrap` plus `prlimit` | Explicit optional skip when the host lacks the capability | +| Distributed verifier | Tests requiring isolation plus a reachable local Redis worker round trip | Explicit optional skip when either prerequisite is absent | + +Semgrep runs with telemetry and version checks disabled and uses isolated, +writable state. Its rules are repository-bundled, so analysis never downloads a +registry configuration. An installed-but-broken required analyzer is not +treated as a clean analysis result: discovery records it as unavailable and CI's +explicit version check fails the job. + +## Reproduce locally + +Run the deterministic suite and allow unavailable infrastructure cases to report +as skips: + +```bash +.venv/bin/pytest -q +.venv/bin/ruff check . +.venv/bin/mypy . --strict +``` + +Run the CI database contract after starting and migrating the declared service: + +```bash +docker compose up -d postgres redis +DATABASE_URL=postgresql+asyncpg://patchfrog:patchfrog@localhost:5432/patchfrog alembic upgrade head +PATCHFROG_REQUIRE_POSTGRES=1 .venv/bin/pytest -q +``` + +`PATCHFROG_REQUIRE_POSTGRES=1` is deliberately a CI/infrastructure assertion: +it converts any connection, authentication, or missing-schema problem into a +test failure. Without it, the same database-only cases skip with an actionable +reason while the deterministic suite continues. + +## Failure classification + +- A deterministic assertion, unexpected exception, analyzer execution failure, + or evaluation regression is a product/test regression and fails. +- An analyzer binary that is declared required but missing or unusable is an + external-tool availability failure and fails CI during verification. +- A real-Postgres test without a compatible local database is an explicit local + infrastructure skip; the same condition fails CI. +- Missing host sandbox or Redis capability for an optional integration is an + explicit skip, never a clean-product assertion. +- There are no unconditional `xfail` entries masking known product failures. + +Expected status for main is green: Ruff clean, strict mypy clean, deterministic +tests passing, required static analyzers verified, real-Postgres tests passing +in CI, and only capability-specific optional integrations skipped where their +documented host prerequisites do not exist. + +Two non-failing upstream/runtime warnings may still be visible: an MCP SDK +Pydantic forward-reference warning during one governance-tool test, and a rare +Python asyncio subprocess-transport finalizer warning after a deliberately +killed analyzer process. Neither changes a test result or represents an ignored +assertion; they remain visible so dependency/runtime upgrades can remove them. diff --git a/docs/evaluation.md b/docs/evaluation.md index fd806f2..eeb1a38 100644 --- a/docs/evaluation.md +++ b/docs/evaluation.md @@ -311,9 +311,9 @@ report's "Static analyzer coverage" table), never silently treated as ## Security review quality (post-Phase-8 refinement) -A separate refinement on top of Phase 8 (branch `feat/security-review-quality`) -extended the existing AI-finding representation with explicit security-quality -concepts, rather than building a parallel security-only reviewer stack. +The security-quality refinement now on main extended the existing AI-finding +representation with explicit security-quality concepts, rather than building a +parallel security-only reviewer stack. **Analysis representation.** `AIReviewFinding` (`patchfrog/review/domain.py`) already distinguished `message` (identification) and severity/confidence/ diff --git a/patchfrog/analysis/analyzers/base.py b/patchfrog/analysis/analyzers/base.py index 9a593fa..5f9321b 100644 --- a/patchfrog/analysis/analyzers/base.py +++ b/patchfrog/analysis/analyzers/base.py @@ -9,13 +9,39 @@ from __future__ import annotations +import os +import shutil +import sys from dataclasses import dataclass from enum import StrEnum +from pathlib import Path from typing import Protocol from patchfrog.analysis.domain import AnalysisContext, AnalyzerCapabilities, AnalyzerResult +def resolve_analyzer_binary(name: str) -> str | None: + """Resolve an analyzer from ``PATH`` or the active Python environment. + + Invoking ``.venv/bin/pytest`` does not itself prepend ``.venv/bin`` to + ``PATH``. Runtime analyzer dependencies installed into that same + environment must nevertheless be discoverable without requiring a + shell-activation side effect. System analyzers continue to resolve + through ``PATH`` first. + """ + + binary = shutil.which(name) + if binary is not None: + return binary + # Do not resolve the interpreter symlink: ``.venv/bin/python`` commonly + # points at ``/usr/bin/python``, while its sibling console scripts live + # in the venv directory named by ``sys.executable`` itself. + environment_binary = Path(sys.executable).parent / name + if environment_binary.is_file() and os.access(environment_binary, os.X_OK): + return str(environment_binary) + return None + + class AnalyzerAvailability(StrEnum): """Whether an analyzer's binary is actually usable in this environment.""" diff --git a/patchfrog/analysis/analyzers/clang_tidy.py b/patchfrog/analysis/analyzers/clang_tidy.py index 81015d1..f98259e 100644 --- a/patchfrog/analysis/analyzers/clang_tidy.py +++ b/patchfrog/analysis/analyzers/clang_tidy.py @@ -25,11 +25,14 @@ from __future__ import annotations import re -import shutil import time from pathlib import Path -from patchfrog.analysis.analyzers.base import AnalyzerAvailability, AnalyzerDiscoveryResult +from patchfrog.analysis.analyzers.base import ( + AnalyzerAvailability, + AnalyzerDiscoveryResult, + resolve_analyzer_binary, +) from patchfrog.analysis.domain import ( AnalysisContext, AnalyzerCapabilities, @@ -121,7 +124,7 @@ class ClangTidyAnalyzer: ) async def discover(self) -> AnalyzerDiscoveryResult: - binary = shutil.which(_BINARY) + binary = resolve_analyzer_binary(_BINARY) if binary is None: return AnalyzerDiscoveryResult( availability=AnalyzerAvailability.UNAVAILABLE, reason="clang-tidy binary not found on PATH" @@ -150,7 +153,7 @@ async def analyze(self, context: AnalysisContext) -> AnalyzerResult: error=discovery.reason, ) version = discovery.version - binary = shutil.which(_BINARY) + binary = resolve_analyzer_binary(_BINARY) assert binary is not None compile_db_dir = find_compilation_database(context.checkout_path) diff --git a/patchfrog/analysis/analyzers/cppcheck.py b/patchfrog/analysis/analyzers/cppcheck.py index ded0860..43d882c 100644 --- a/patchfrog/analysis/analyzers/cppcheck.py +++ b/patchfrog/analysis/analyzers/cppcheck.py @@ -8,12 +8,15 @@ from __future__ import annotations -import shutil import time import xml.etree.ElementTree as ET from pathlib import Path -from patchfrog.analysis.analyzers.base import AnalyzerAvailability, AnalyzerDiscoveryResult +from patchfrog.analysis.analyzers.base import ( + AnalyzerAvailability, + AnalyzerDiscoveryResult, + resolve_analyzer_binary, +) from patchfrog.analysis.domain import ( AnalysisContext, AnalyzerCapabilities, @@ -92,7 +95,7 @@ class CppcheckAnalyzer: ) async def discover(self) -> AnalyzerDiscoveryResult: - binary = shutil.which(_BINARY) + binary = resolve_analyzer_binary(_BINARY) if binary is None: return AnalyzerDiscoveryResult( availability=AnalyzerAvailability.UNAVAILABLE, reason="cppcheck binary not found on PATH" @@ -121,7 +124,7 @@ async def analyze(self, context: AnalysisContext) -> AnalyzerResult: error=discovery.reason, ) version = discovery.version - binary = shutil.which(_BINARY) + binary = resolve_analyzer_binary(_BINARY) assert binary is not None targets = _target_paths(context) diff --git a/patchfrog/analysis/analyzers/ruff.py b/patchfrog/analysis/analyzers/ruff.py index 057347b..540d303 100644 --- a/patchfrog/analysis/analyzers/ruff.py +++ b/patchfrog/analysis/analyzers/ruff.py @@ -11,11 +11,14 @@ from __future__ import annotations import json -import shutil import time from pathlib import Path -from patchfrog.analysis.analyzers.base import AnalyzerAvailability, AnalyzerDiscoveryResult +from patchfrog.analysis.analyzers.base import ( + AnalyzerAvailability, + AnalyzerDiscoveryResult, + resolve_analyzer_binary, +) from patchfrog.analysis.domain import ( AnalysisContext, AnalyzerCapabilities, @@ -82,7 +85,7 @@ class RuffAnalyzer: ) async def discover(self) -> AnalyzerDiscoveryResult: - binary = shutil.which(_BINARY) + binary = resolve_analyzer_binary(_BINARY) if binary is None: return AnalyzerDiscoveryResult( availability=AnalyzerAvailability.UNAVAILABLE, reason="ruff binary not found on PATH" @@ -110,7 +113,7 @@ async def analyze(self, context: AnalysisContext) -> AnalyzerResult: error=discovery.reason, ) version = discovery.version - binary = shutil.which(_BINARY) + binary = resolve_analyzer_binary(_BINARY) assert binary is not None # discover() already confirmed this targets = _target_paths(context) diff --git a/patchfrog/analysis/analyzers/semgrep.py b/patchfrog/analysis/analyzers/semgrep.py index b7efb0f..e0fa16f 100644 --- a/patchfrog/analysis/analyzers/semgrep.py +++ b/patchfrog/analysis/analyzers/semgrep.py @@ -10,11 +10,15 @@ from __future__ import annotations import json -import shutil +import tempfile import time from pathlib import Path -from patchfrog.analysis.analyzers.base import AnalyzerAvailability, AnalyzerDiscoveryResult +from patchfrog.analysis.analyzers.base import ( + AnalyzerAvailability, + AnalyzerDiscoveryResult, + resolve_analyzer_binary, +) from patchfrog.analysis.domain import ( AnalysisContext, AnalyzerCapabilities, @@ -29,6 +33,10 @@ from patchfrog.domain.code import Language, SourceSpan _BINARY = "semgrep" +_OFFLINE_ENV = { + "SEMGREP_ENABLE_VERSION_CHECK": "0", + "SEMGREP_SEND_METRICS": "off", +} BUNDLED_RULES_PATH = Path(__file__).parent / "semgrep_rules" / "patchfrog-rules.yml" @@ -55,13 +63,19 @@ class SemgrepAnalyzer: ) async def discover(self) -> AnalyzerDiscoveryResult: - binary = shutil.which(_BINARY) + binary = resolve_analyzer_binary(_BINARY) if binary is None: return AnalyzerDiscoveryResult( availability=AnalyzerAvailability.UNAVAILABLE, reason="semgrep binary not found on PATH" ) try: - result = await run_sandboxed([binary, "--version"], cwd=Path.cwd(), timeout_seconds=15) + with tempfile.TemporaryDirectory(prefix="patchfrog-semgrep-") as state_dir: + result = await run_sandboxed( + [binary, "--version"], + cwd=Path.cwd(), + timeout_seconds=15, + extra_env={"HOME": state_dir, **_OFFLINE_ENV}, + ) except AnalyzerSubprocessError as exc: return AnalyzerDiscoveryResult(availability=AnalyzerAvailability.UNAVAILABLE, reason=str(exc)) if result.exit_code != 0: @@ -84,7 +98,7 @@ async def analyze(self, context: AnalysisContext) -> AnalyzerResult: error=discovery.reason, ) version = discovery.version - binary = shutil.which(_BINARY) + binary = resolve_analyzer_binary(_BINARY) assert binary is not None if not (context.languages & self.capabilities.languages): @@ -107,9 +121,13 @@ async def analyze(self, context: AnalysisContext) -> AnalyzerResult: *targets, ] try: - result = await run_sandboxed( - args, cwd=context.checkout_path, timeout_seconds=context.config.timeout_seconds - ) + with tempfile.TemporaryDirectory(prefix="patchfrog-semgrep-") as state_dir: + result = await run_sandboxed( + args, + cwd=context.checkout_path, + timeout_seconds=context.config.timeout_seconds, + extra_env={"HOME": state_dir, **_OFFLINE_ENV}, + ) except AnalyzerSubprocessError as exc: return AnalyzerResult( analyzer="semgrep", diff --git a/patchfrog/ops/health.py b/patchfrog/ops/health.py index 8f50e05..5493153 100644 --- a/patchfrog/ops/health.py +++ b/patchfrog/ops/health.py @@ -18,6 +18,7 @@ from dataclasses import dataclass from pathlib import Path +from typing import Protocol, cast import redis.asyncio as redis from alembic.config import Config @@ -28,6 +29,10 @@ _ALEMBIC_INI_PATH = Path(__file__).resolve().parents[2] / "alembic.ini" +class _AsyncClosable(Protocol): + async def aclose(self) -> None: ... + + @dataclass(frozen=True, slots=True) class ReadinessCheck: name: str @@ -80,7 +85,9 @@ async def check_redis(redis_url: str) -> ReadinessCheck: except Exception as exc: return ReadinessCheck(name="redis", healthy=False, detail=str(exc)) finally: - await client.close() + # ``types-redis`` still exposes the deprecated ``close`` surface; + # runtime redis-py 5+ provides the warning-free async API. + await cast(_AsyncClosable, client).aclose() async def check_readiness(*, engine: AsyncEngine, redis_url: str) -> ReadinessReport: diff --git a/patchfrog/publishing/planner.py b/patchfrog/publishing/planner.py index 545de62..e65a0d2 100644 --- a/patchfrog/publishing/planner.py +++ b/patchfrog/publishing/planner.py @@ -12,7 +12,24 @@ Selection funnel, most to least preferred: -1. Findings below ``config.min_severity`` are omitted outright. +1. Findings below ``config.min_severity`` are omitted outright -- + *unless* the finding is high-confidence evidence from one of the two + specialist roles (:class:`~patchfrog.analysis.domain.FindingCategory.CORRECTNESS` + or ``SECURITY``) with :class:`~patchfrog.analysis.domain.Confidence.HIGH`. + Severity and confidence are orthogonal (see + :class:`patchfrog.analysis.domain.Confidence`'s own docstring): severity + is how *bad* an issue is, confidence is how *sure* the detection is. + ``min_severity`` exists to cut low-priority noise, never to discard a + proposal that has already survived reviewer proposal, deterministic + validation, confidence aggregation (:mod:`patchfrog.review.confidence` + -- critic-downgraded when a critic verdict exists, reviewer-reported + otherwise, e.g. when the critic call itself failed and fell back per + :mod:`patchfrog.review.orchestration`'s documented fail-open policy) + and dedup as a proven defect (see + :meth:`~patchfrog.publishing.planner.PublicationPlanner.build_plan`'s + ``_bypasses_severity_floor`` use below) -- such a finding still goes + through steps 2-4 exactly like any other eligible finding, so it can + still end up ``OMITTED`` by a cap, just never by severity alone. 2. Findings that map to a real diff line (see :mod:`patchfrog.publishing.diff_mapper`) are ranked (severity desc, confidence desc, then stable path/line) and the top @@ -34,7 +51,7 @@ from collections.abc import Mapping, Sequence from uuid import UUID -from patchfrog.analysis.domain import Confidence, Severity +from patchfrog.analysis.domain import Confidence, FindingCategory, Severity from patchfrog.diff.models import DiffFile from patchfrog.domain.pull_request import ChangedFile from patchfrog.publishing.body import ( @@ -74,6 +91,28 @@ def _meets_min_severity(severity: Severity, *, minimum: Severity) -> bool: return _SEVERITY_RANK[severity] >= _SEVERITY_RANK[minimum] +#: The two specialist roles (:mod:`patchfrog.review.agents.roles`) whose +#: findings represent hard, reviewer-proposed-then-critic-verified +#: evidence of an actual defect rather than a style/maintainability +#: opinion -- see the module docstring's funnel step 1. +_HARD_EVIDENCE_CATEGORIES = frozenset({FindingCategory.CORRECTNESS, FindingCategory.SECURITY}) + + +def _bypasses_severity_floor(finding: PublishableFinding) -> bool: + """A finding that survived reviewer proposal, validation, confidence + aggregation, and dedup with :class:`Confidence.HIGH` in one of the two + specialist categories is proven evidence of a real defect, not noise -- + ``config.min_severity`` must never be the sole reason it is discarded + with zero visibility (see the funnel docstring above). This is a + narrow, category+confidence-scoped carve-out, not a change to the + configured threshold itself: everything else is still governed by + ``config.min_severity`` exactly as before, and a bypassing finding is + still fully subject to steps 2-4 (mapping, caps) below -- it can still + end up ``OMITTED`` by a cap, just never by severity alone.""" + + return finding.confidence is Confidence.HIGH and finding.category in _HARD_EVIDENCE_CATEGORIES + + class PublicationPlanner: """Stateless; safe to reuse across calls.""" @@ -176,7 +215,7 @@ def build_plan( ) ) continue - if not _meets_min_severity(finding.severity, minimum=config.min_severity): + if not _meets_min_severity(finding.severity, minimum=config.min_severity) and not _bypasses_severity_floor(finding): omitted.append(self._to_comment(finding, snapshot, PublicationDisposition.OMITTED, position=None, reason="below minimum severity threshold")) continue eligible.append(finding) diff --git a/patchfrog/publishing/service.py b/patchfrog/publishing/service.py index 423ba38..2e53a97 100644 --- a/patchfrog/publishing/service.py +++ b/patchfrog/publishing/service.py @@ -61,6 +61,7 @@ from patchfrog.publishing.config import PublicationConfig from patchfrog.publishing.domain import ( DiffSide, + PublishableFinding, ReviewInputSnapshot, ReviewPublicationComment, ReviewPublicationMode, @@ -235,6 +236,7 @@ async def publish( ), ) + self._log_omitted_findings(publication_id, findings, plan) await self._persist_plan_comments(publication_id, plan) if plan.status is ReviewPublicationStatus.STALE: @@ -325,6 +327,31 @@ async def publish( ) return self._result_from_model(model, reconciled=False, errors=()) + @staticmethod + def _log_omitted_findings( + publication_id: uuid.UUID, findings: list[PublishableFinding], plan: ReviewPublicationPlan + ) -> None: + """One structured log line per finding the planner discarded to + ``OMITTED`` -- finding id, severity/category, and the planner's own + fixed-vocabulary omission reason only (e.g. "below minimum + severity threshold", "exceeded max_summary_findings cap"). Never + the finding's title/message/reasoning_summary/suggested_fix + (repository/provider-derived free text) -- see + :class:`patchfrog.publishing.domain.PublishableFinding`.""" + + if not plan.omitted: + return + category_by_id = {f.finding_id: f.category.value for f in findings} + for comment in plan.omitted: + logger.info( + "review_publish_finding_omitted", + publication_id=str(publication_id), + finding_id=str(comment.finding_id), + severity=comment.severity.value, + category=category_by_id.get(comment.finding_id, "unknown"), + omission_reason=comment.reason, + ) + async def _persist_plan_comments(self, publication_id: uuid.UUID, plan: ReviewPublicationPlan) -> None: all_comments: list[ReviewPublicationComment] = [ *plan.inline_comments, *plan.summary_only, *plan.omitted, *plan.already_reported, diff --git a/tests/integration/test_analysis_run_concurrency.py b/tests/integration/test_analysis_run_concurrency.py index fbc4eba..0933b45 100644 --- a/tests/integration/test_analysis_run_concurrency.py +++ b/tests/integration/test_analysis_run_concurrency.py @@ -25,37 +25,21 @@ import uuid from pathlib import Path -import pytest from sqlalchemy import select, text -from sqlalchemy.exc import OperationalError, ProgrammingError -from sqlalchemy.ext.asyncio import AsyncEngine, async_sessionmaker, create_async_engine +from sqlalchemy.ext.asyncio import async_sessionmaker from patchfrog.analysis.service import StaticAnalysisService from patchfrog.indexing.service import RepositoryIndexingService from patchfrog.persistence.models.analysis import AnalysisRunModel from patchfrog.persistence.repositories import RepositoryRepository from tests.support.git_repo import materialize_fixture_repo - -_POSTGRES_URL = "postgresql+asyncpg://patchfrog:patchfrog@localhost:5432/patchfrog" - - -async def _postgres_available() -> AsyncEngine | None: - engine = create_async_engine(_POSTGRES_URL) - try: - async with engine.begin() as conn: - await conn.execute(text("SELECT 1 FROM findings LIMIT 1")) - except (OperationalError, ProgrammingError): - await engine.dispose() - return None - return engine +from tests.support.postgres import postgres_engine_or_skip async def test_two_concurrent_analysis_runs_never_produce_two_succeeded_rows( tmp_path: Path, ) -> None: - engine = await _postgres_available() - if engine is None: - pytest.skip("real PostgreSQL not reachable at localhost:5432 (docker compose up -d postgres)") + engine = await postgres_engine_or_skip("findings") session_factory = async_sessionmaker(engine, expire_on_commit=False) full_name = f"analysis-concurrency-test/{uuid.uuid4().hex[:8]}" diff --git a/tests/integration/test_context_concurrency.py b/tests/integration/test_context_concurrency.py index d50b2f2..9af4cbd 100644 --- a/tests/integration/test_context_concurrency.py +++ b/tests/integration/test_context_concurrency.py @@ -22,10 +22,8 @@ import uuid from pathlib import Path -import pytest from sqlalchemy import select, text -from sqlalchemy.exc import OperationalError, ProgrammingError -from sqlalchemy.ext.asyncio import AsyncEngine, async_sessionmaker, create_async_engine +from sqlalchemy.ext.asyncio import async_sessionmaker from patchfrog.context.domain import ContextTargetType from patchfrog.context.service import ContextService @@ -33,27 +31,13 @@ from patchfrog.persistence.models.context import ContextBundleModel, ContextItemModel from patchfrog.persistence.repositories import RepositoryRepository from tests.support.git_repo import materialize_fixture_repo - -_POSTGRES_URL = "postgresql+asyncpg://patchfrog:patchfrog@localhost:5432/patchfrog" - - -async def _postgres_available() -> AsyncEngine | None: - engine = create_async_engine(_POSTGRES_URL) - try: - async with engine.begin() as conn: - await conn.execute(text("SELECT 1 FROM context_bundles LIMIT 1")) - except (OperationalError, ProgrammingError): - await engine.dispose() - return None - return engine +from tests.support.postgres import postgres_engine_or_skip async def test_two_concurrent_context_generations_never_produce_two_succeeded_bundles( tmp_path: Path, ) -> None: - engine = await _postgres_available() - if engine is None: - pytest.skip("real PostgreSQL not reachable at localhost:5432 (docker compose up -d postgres)") + engine = await postgres_engine_or_skip("context_bundles") session_factory = async_sessionmaker(engine, expire_on_commit=False) full_name = f"context-concurrency-test/{uuid.uuid4().hex[:8]}" diff --git a/tests/integration/test_feedback_concurrency.py b/tests/integration/test_feedback_concurrency.py index 9c8966f..0b51c40 100644 --- a/tests/integration/test_feedback_concurrency.py +++ b/tests/integration/test_feedback_concurrency.py @@ -20,10 +20,8 @@ import uuid from datetime import UTC, datetime -import pytest -from sqlalchemy import select, text -from sqlalchemy.exc import OperationalError, ProgrammingError -from sqlalchemy.ext.asyncio import AsyncEngine, async_sessionmaker, create_async_engine +from sqlalchemy import select +from sqlalchemy.ext.asyncio import async_sessionmaker from patchfrog.feedback.domain import ( ActorIdentity, @@ -36,25 +34,11 @@ from patchfrog.persistence.models.feedback import FeedbackEventModel from patchfrog.persistence.repositories import RepositoryRepository from patchfrog.persistence.repositories.feedback import FeedbackEventRepository - -_POSTGRES_URL = "postgresql+asyncpg://patchfrog:patchfrog@localhost:5432/patchfrog" - - -async def _postgres_available() -> AsyncEngine | None: - engine = create_async_engine(_POSTGRES_URL) - try: - async with engine.begin() as conn: - await conn.execute(text("SELECT 1 FROM feedback_events LIMIT 1")) - except (OperationalError, ProgrammingError): - await engine.dispose() - return None - return engine +from tests.support.postgres import postgres_engine_or_skip async def test_two_concurrent_ingestions_of_the_same_raw_event_never_duplicate() -> None: - engine = await _postgres_available() - if engine is None: - pytest.skip("real PostgreSQL not reachable at localhost:5432 (docker compose up -d postgres)") + engine = await postgres_engine_or_skip("feedback_events") session_factory = async_sessionmaker(engine, expire_on_commit=False) full_name = f"feedback-concurrency-test/{uuid.uuid4().hex[:8]}" diff --git a/tests/integration/test_publishing_concurrency.py b/tests/integration/test_publishing_concurrency.py index 88c0d0e..abab902 100644 --- a/tests/integration/test_publishing_concurrency.py +++ b/tests/integration/test_publishing_concurrency.py @@ -18,35 +18,21 @@ import uuid from pathlib import Path -import pytest from sqlalchemy import text -from sqlalchemy.exc import OperationalError, ProgrammingError -from sqlalchemy.ext.asyncio import AsyncEngine, async_sessionmaker, create_async_engine +from sqlalchemy.ext.asyncio import async_sessionmaker from patchfrog.domain.pull_request import PullRequestMetadata from patchfrog.publishing.config import PublicationConfig from patchfrog.publishing.domain import ReviewPublicationMode, ReviewPublicationStatus from patchfrog.publishing.fake_publisher import FakeReviewPublisher from patchfrog.publishing.service import ReviewPublicationService +from tests.support.postgres import postgres_engine_or_skip from tests.support.publishing import ( finding_json, scripted_findings_response, setup_reviewed_pull_request, ) -_POSTGRES_URL = "postgresql+asyncpg://patchfrog:patchfrog@localhost:5432/patchfrog" - - -async def _postgres_available() -> AsyncEngine | None: - engine = create_async_engine(_POSTGRES_URL) - try: - async with engine.begin() as conn: - await conn.execute(text("SELECT 1 FROM review_publications LIMIT 1")) - except (OperationalError, ProgrammingError): - await engine.dispose() - return None - return engine - def _pr_metadata(*, number: int, head_sha: str) -> PullRequestMetadata: return PullRequestMetadata( @@ -56,9 +42,7 @@ def _pr_metadata(*, number: int, head_sha: str) -> PullRequestMetadata: async def test_two_concurrent_publish_attempts_write_exactly_one_github_review(tmp_path: Path) -> None: - engine = await _postgres_available() - if engine is None: - pytest.skip("real PostgreSQL not reachable at localhost:5432 (docker compose up -d postgres)") + engine = await postgres_engine_or_skip("review_publications") session_factory = async_sessionmaker(engine, expire_on_commit=False) repository_id = None diff --git a/tests/integration/test_publishing_persistence.py b/tests/integration/test_publishing_persistence.py index 0ae9338..adc78a1 100644 --- a/tests/integration/test_publishing_persistence.py +++ b/tests/integration/test_publishing_persistence.py @@ -17,12 +17,10 @@ import pytest from sqlalchemy import select, text -from sqlalchemy.exc import IntegrityError, OperationalError, ProgrammingError +from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import ( - AsyncEngine, AsyncSession, async_sessionmaker, - create_async_engine, ) from patchfrog.persistence.models.publishing import ( @@ -35,27 +33,16 @@ ReviewPublicationMode, ReviewPublicationStatus, ) +from tests.support.postgres import postgres_engine_or_skip from tests.support.publishing import ( finding_json, scripted_findings_response, setup_reviewed_pull_request, ) -_POSTGRES_URL = "postgresql+asyncpg://patchfrog:patchfrog@localhost:5432/patchfrog" _TEST_POLICY_FINGERPRINT = PublicationConfig(enabled=True).fingerprint() -async def _postgres_available() -> AsyncEngine | None: - engine = create_async_engine(_POSTGRES_URL) - try: - async with engine.begin() as conn: - await conn.execute(text("SELECT 1 FROM review_publications LIMIT 1")) - except (OperationalError, ProgrammingError): - await engine.dispose() - return None - return engine - - async def _cleanup(session_factory: async_sessionmaker[AsyncSession], repository_id: uuid.UUID) -> None: async with session_factory() as session: await session.execute( @@ -83,9 +70,7 @@ async def test_db_level_unique_constraint_rejects_second_published_row(tmp_path: discipline (spec section 19: "Use DB-level protection where appropriate").""" - engine = await _postgres_available() - if engine is None: - pytest.skip("real PostgreSQL not reachable at localhost:5432 (docker compose up -d postgres)") + engine = await postgres_engine_or_skip("review_publications") session_factory = async_sessionmaker(engine, expire_on_commit=False) repository_id = None @@ -133,9 +118,7 @@ async def test_dry_run_rows_never_block_a_later_published_row(tmp_path: Path) -> uniqueness guarantee -- confirms the partial index correctly scopes on (review_run_id, mode) together, not review_run_id alone.""" - engine = await _postgres_available() - if engine is None: - pytest.skip("real PostgreSQL not reachable at localhost:5432 (docker compose up -d postgres)") + engine = await postgres_engine_or_skip("review_publications") session_factory = async_sessionmaker(engine, expire_on_commit=False) repository_id = None @@ -174,9 +157,7 @@ async def test_dry_run_rows_never_block_a_later_published_row(tmp_path: Path) -> async def test_comment_fingerprint_uniqueness_within_a_publication(tmp_path: Path) -> None: - engine = await _postgres_available() - if engine is None: - pytest.skip("real PostgreSQL not reachable at localhost:5432 (docker compose up -d postgres)") + engine = await postgres_engine_or_skip("review_publications") session_factory = async_sessionmaker(engine, expire_on_commit=False) repository_id = None @@ -229,9 +210,7 @@ async def test_comment_fingerprint_uniqueness_within_a_publication(tmp_path: Pat async def test_deleting_review_run_cascades_to_publications_and_comments(tmp_path: Path) -> None: - engine = await _postgres_available() - if engine is None: - pytest.skip("real PostgreSQL not reachable at localhost:5432 (docker compose up -d postgres)") + engine = await postgres_engine_or_skip("review_publications") session_factory = async_sessionmaker(engine, expire_on_commit=False) repository_id = None diff --git a/tests/integration/test_publishing_service_dry_run.py b/tests/integration/test_publishing_service_dry_run.py index 4fbbc80..3dc2205 100644 --- a/tests/integration/test_publishing_service_dry_run.py +++ b/tests/integration/test_publishing_service_dry_run.py @@ -79,6 +79,39 @@ async def test_dry_run_plans_but_never_writes_to_github( assert comments[0].github_comment_id is None +async def test_dry_run_high_confidence_low_severity_correctness_finding_is_published_not_omitted( + session_factory: async_sessionmaker[AsyncSession], tmp_path: Path +) -> None: + """Regression: PR #57 production evidence showed a real, critic- + verified, high-confidence correctness finding (severity=low) being + discarded outright by the default min_severity=MEDIUM floor before + ever being considered for inline/summary placement -- GitHub received + nothing despite the review run itself reporting accepted_count=1. See + patchfrog.publishing.planner._bypasses_severity_floor.""" + + reviewed = await setup_reviewed_pull_request( + session_factory, + full_name="test/pr-57-severity-floor", + changed_lines=[14], + response_factory=lambda req: scripted_findings_response( + [finding_json(severity="low", confidence="high", category="correctness")] + ), + tmp_root=tmp_path, + ) + assert len(reviewed.findings) == 1 + + publisher = FakeReviewPublisher( + pull_request=_pr_metadata(number=reviewed.pull_request_number, head_sha=reviewed.commit_sha), + changed_files=reviewed.changed_files, + ) + service = ReviewPublicationService(session_factory=session_factory, publisher=publisher) + result = await service.publish(review_run_id=reviewed.review_run_id, mode=ReviewPublicationMode.DRY_RUN) + + assert result.status is ReviewPublicationStatus.DRY_RUN + assert result.omitted == 0 + assert result.planned_inline + result.summary_only == 1 + + async def test_dry_run_with_no_findings_is_skipped( session_factory: async_sessionmaker[AsyncSession], tmp_path: Path ) -> None: diff --git a/tests/integration/test_repository_index_concurrency.py b/tests/integration/test_repository_index_concurrency.py index 21dbcb6..6cec007 100644 --- a/tests/integration/test_repository_index_concurrency.py +++ b/tests/integration/test_repository_index_concurrency.py @@ -33,39 +33,20 @@ import uuid from pathlib import Path -import pytest from sqlalchemy import select, text -from sqlalchemy.exc import OperationalError, ProgrammingError -from sqlalchemy.ext.asyncio import AsyncEngine, async_sessionmaker, create_async_engine +from sqlalchemy.ext.asyncio import async_sessionmaker from patchfrog.indexing.service import RepositoryIndexingService from patchfrog.persistence.models.repository_index import RepositoryIndexModel from patchfrog.persistence.repositories import RepositoryRepository from tests.support.git_repo import materialize_fixture_repo - -_POSTGRES_URL = "postgresql+asyncpg://patchfrog:patchfrog@localhost:5432/patchfrog" - - -async def _postgres_available() -> AsyncEngine | None: - """Connectivity + migrated-schema check only — never creates schema - itself. See the module docstring for why.""" - - engine = create_async_engine(_POSTGRES_URL) - try: - async with engine.begin() as conn: - await conn.execute(text("SELECT 1 FROM repository_indexes LIMIT 1")) - except (OperationalError, ProgrammingError): - await engine.dispose() - return None - return engine +from tests.support.postgres import postgres_engine_or_skip async def test_two_concurrent_indexing_runs_never_leave_two_active_indexes( tmp_path: Path, ) -> None: - engine = await _postgres_available() - if engine is None: - pytest.skip("real PostgreSQL not reachable at localhost:5432 (docker compose up -d postgres)") + engine = await postgres_engine_or_skip("repository_indexes") session_factory = async_sessionmaker(engine, expire_on_commit=False) full_name = f"concurrency-test/{uuid.uuid4().hex[:8]}" diff --git a/tests/integration/test_review_config_bundle_adversarial.py b/tests/integration/test_review_config_bundle_adversarial.py index 82cc1b9..359cc01 100644 --- a/tests/integration/test_review_config_bundle_adversarial.py +++ b/tests/integration/test_review_config_bundle_adversarial.py @@ -12,14 +12,10 @@ import uuid from pathlib import Path -import pytest from sqlalchemy import select, text -from sqlalchemy.exc import OperationalError, ProgrammingError from sqlalchemy.ext.asyncio import ( - AsyncEngine, AsyncSession, async_sessionmaker, - create_async_engine, ) from patchfrog.diff.models import DiffFile, DiffHunk, DiffLine, DiffLineType @@ -33,8 +29,7 @@ from patchfrog.review.providers.fake import FakeLLMProvider, ScriptedResponse from patchfrog.review.service import PullRequestReviewService, persist_malformed_config_failure from tests.support.git_repo import materialize_fixture_repo - -_POSTGRES_URL = "postgresql+asyncpg://patchfrog:patchfrog@localhost:5432/patchfrog" +from tests.support.postgres import postgres_engine_or_skip def _diff_marking_lines(file_path: str, lines: list[int]) -> DiffFile: @@ -49,17 +44,6 @@ def _diff_marking_lines(file_path: str, lines: list[int]) -> DiffFile: return DiffFile(path=file_path, hunks=(hunk,)) -async def _postgres_available() -> AsyncEngine | None: - engine = create_async_engine(_POSTGRES_URL) - try: - async with engine.begin() as conn: - await conn.execute(text("SELECT 1 FROM review_runs LIMIT 1")) - except (OperationalError, ProgrammingError): - await engine.dispose() - return None - return engine - - async def test_concurrent_duplicate_malformed_config_delivery_never_crashes(tmp_path: Path) -> None: """Two 'concurrent Celery deliveries' both hitting the exact same malformed .patchfrog.yml for the same commit must both resolve @@ -67,9 +51,7 @@ async def test_concurrent_duplicate_malformed_config_delivery_never_crashes(tmp_ pg_advisory_xact_lock every other review-run identity uses -- no crash, no unhandled exception, no partial/duplicate row corruption.""" - engine = await _postgres_available() - if engine is None: - pytest.skip("real PostgreSQL not reachable at localhost:5432 (docker compose up -d postgres)") + engine = await postgres_engine_or_skip("review_runs") session_factory = async_sessionmaker(engine, expire_on_commit=False) full_name = f"malformed-concurrency-test/{uuid.uuid4().hex[:8]}" diff --git a/tests/integration/test_review_context_bundle_cascade.py b/tests/integration/test_review_context_bundle_cascade.py index 3176485..82aceca 100644 --- a/tests/integration/test_review_context_bundle_cascade.py +++ b/tests/integration/test_review_context_bundle_cascade.py @@ -26,10 +26,8 @@ import uuid from pathlib import Path -import pytest from sqlalchemy import select, text -from sqlalchemy.exc import OperationalError, ProgrammingError -from sqlalchemy.ext.asyncio import AsyncEngine, async_sessionmaker, create_async_engine +from sqlalchemy.ext.asyncio import async_sessionmaker from patchfrog.diff.models import DiffFile, DiffHunk, DiffLine, DiffLineType from patchfrog.indexing.service import RepositoryIndexingService @@ -39,8 +37,7 @@ from patchfrog.review.providers.fake import FakeLLMProvider, ScriptedResponse from patchfrog.review.service import PullRequestReviewService from tests.support.git_repo import materialize_fixture_repo - -_POSTGRES_URL = "postgresql+asyncpg://patchfrog:patchfrog@localhost:5432/patchfrog" +from tests.support.postgres import postgres_engine_or_skip def _diff_marking_lines(file_path: str, lines: list[int]) -> DiffFile: @@ -55,23 +52,10 @@ def _diff_marking_lines(file_path: str, lines: list[int]) -> DiffFile: return DiffFile(path=file_path, hunks=(hunk,)) -async def _postgres_available() -> AsyncEngine | None: - engine = create_async_engine(_POSTGRES_URL) - try: - async with engine.begin() as conn: - await conn.execute(text("SELECT 1 FROM review_candidates LIMIT 1")) - except (OperationalError, ProgrammingError): - await engine.dispose() - return None - return engine - - async def test_deleting_a_review_run_cascades_to_candidates_but_not_into_the_context_bundle( tmp_path: Path, ) -> None: - engine = await _postgres_available() - if engine is None: - pytest.skip("real PostgreSQL not reachable at localhost:5432 (docker compose up -d postgres)") + engine = await postgres_engine_or_skip("review_candidates") session_factory = async_sessionmaker(engine, expire_on_commit=False) full_name = f"bundle-cascade-test/{uuid.uuid4().hex[:8]}" diff --git a/tests/integration/test_review_memory_generation_concurrency.py b/tests/integration/test_review_memory_generation_concurrency.py index b00bca4..adb07be 100644 --- a/tests/integration/test_review_memory_generation_concurrency.py +++ b/tests/integration/test_review_memory_generation_concurrency.py @@ -22,14 +22,10 @@ import uuid from datetime import UTC, datetime -import pytest from sqlalchemy import select, text -from sqlalchemy.exc import OperationalError, ProgrammingError from sqlalchemy.ext.asyncio import ( - AsyncEngine, AsyncSession, async_sessionmaker, - create_async_engine, ) from patchfrog.persistence.models.pull_request import PullRequestModel @@ -41,19 +37,7 @@ from patchfrog.review.domain import ReviewRunStatus from patchfrog.review_memory.config import NO_MEMORY_CONTEXT_FINGERPRINT from patchfrog.review_memory.domain import IncrementalRunMode - -_POSTGRES_URL = "postgresql+asyncpg://patchfrog:patchfrog@localhost:5432/patchfrog" - - -async def _postgres_available() -> AsyncEngine | None: - engine = create_async_engine(_POSTGRES_URL) - try: - async with engine.begin() as conn: - await conn.execute(text("SELECT 1 FROM review_generations LIMIT 1")) - except (OperationalError, ProgrammingError): - await engine.dispose() - return None - return engine +from tests.support.postgres import postgres_engine_or_skip async def _make_review_run( @@ -71,9 +55,7 @@ async def _make_review_run( async def test_concurrent_generation_creation_never_collides_sequence_number() -> None: - engine = await _postgres_available() - if engine is None: - pytest.skip("real PostgreSQL not reachable at localhost:5432 (docker compose up -d postgres)") + engine = await postgres_engine_or_skip("review_generations") session_factory = async_sessionmaker(engine, expire_on_commit=False) full_name = f"review-memory-concurrency-test/{uuid.uuid4().hex[:8]}" diff --git a/tests/integration/test_review_run_concurrency.py b/tests/integration/test_review_run_concurrency.py index e452493..7ad680b 100644 --- a/tests/integration/test_review_run_concurrency.py +++ b/tests/integration/test_review_run_concurrency.py @@ -24,10 +24,8 @@ import uuid from pathlib import Path -import pytest from sqlalchemy import select, text -from sqlalchemy.exc import OperationalError, ProgrammingError -from sqlalchemy.ext.asyncio import AsyncEngine, async_sessionmaker, create_async_engine +from sqlalchemy.ext.asyncio import async_sessionmaker from patchfrog.diff.models import DiffFile, DiffHunk, DiffLine, DiffLineType from patchfrog.indexing.service import RepositoryIndexingService @@ -37,8 +35,7 @@ from patchfrog.review.providers.fake import FakeLLMProvider, ScriptedResponse from patchfrog.review.service import PullRequestReviewService from tests.support.git_repo import materialize_fixture_repo - -_POSTGRES_URL = "postgresql+asyncpg://patchfrog:patchfrog@localhost:5432/patchfrog" +from tests.support.postgres import postgres_engine_or_skip def _diff_marking_lines(file_path: str, lines: list[int]) -> DiffFile: @@ -53,21 +50,8 @@ def _diff_marking_lines(file_path: str, lines: list[int]) -> DiffFile: return DiffFile(path=file_path, hunks=(hunk,)) -async def _postgres_available() -> AsyncEngine | None: - engine = create_async_engine(_POSTGRES_URL) - try: - async with engine.begin() as conn: - await conn.execute(text("SELECT 1 FROM review_runs LIMIT 1")) - except (OperationalError, ProgrammingError): - await engine.dispose() - return None - return engine - - async def test_two_concurrent_reviews_never_produce_two_succeeded_runs(tmp_path: Path) -> None: - engine = await _postgres_available() - if engine is None: - pytest.skip("real PostgreSQL not reachable at localhost:5432 (docker compose up -d postgres)") + engine = await postgres_engine_or_skip("review_runs") session_factory = async_sessionmaker(engine, expire_on_commit=False) full_name = f"review-concurrency-test/{uuid.uuid4().hex[:8]}" diff --git a/tests/integration/test_toolchain_identity.py b/tests/integration/test_toolchain_identity.py index 24449f3..fcdf0d1 100644 --- a/tests/integration/test_toolchain_identity.py +++ b/tests/integration/test_toolchain_identity.py @@ -18,12 +18,9 @@ import pytest from sqlalchemy import select, text -from sqlalchemy.exc import OperationalError, ProgrammingError from sqlalchemy.ext.asyncio import ( - AsyncEngine, AsyncSession, async_sessionmaker, - create_async_engine, ) from patchfrog.analysis.analyzers.base import AnalyzerAvailability, AnalyzerDiscoveryResult @@ -42,8 +39,7 @@ from patchfrog.persistence.models.analysis import AnalysisRunModel from patchfrog.persistence.repositories import RepositoryRepository from tests.support.git_repo import materialize_fixture_repo - -_POSTGRES_URL = "postgresql+asyncpg://patchfrog:patchfrog@localhost:5432/patchfrog" +from tests.support.postgres import postgres_engine_or_skip class _VersionedStubAnalyzer: @@ -205,17 +201,6 @@ async def test_changing_only_semgrep_ruleset_content_creates_a_distinct_canonica assert len({r.toolchain_fingerprint for r in succeeded}) == 2 -async def _postgres_available() -> AsyncEngine | None: - engine = create_async_engine(_POSTGRES_URL) - try: - async with engine.begin() as conn: - await conn.execute(text("SELECT 1 FROM analysis_runs LIMIT 1")) - except (OperationalError, ProgrammingError): - await engine.dispose() - return None - return engine - - async def test_concurrent_identical_toolchain_identity_remains_race_safe_in_real_postgres( tmp_path: Path, ) -> None: @@ -225,9 +210,7 @@ async def test_concurrent_identical_toolchain_identity_remains_race_safe_in_real the advisory lock is keyed by the combined identity now, not the old config-only one.""" - engine = await _postgres_available() - if engine is None: - pytest.skip("real PostgreSQL not reachable at localhost:5432 (docker compose up -d postgres)") + engine = await postgres_engine_or_skip("analysis_runs") session_factory = async_sessionmaker(engine, expire_on_commit=False) full_name = f"toolchain-concurrency-test/{uuid.uuid4().hex[:8]}" diff --git a/tests/support/postgres.py b/tests/support/postgres.py new file mode 100644 index 0000000..32d25d9 --- /dev/null +++ b/tests/support/postgres.py @@ -0,0 +1,41 @@ +"""Shared prerequisite contract for tests that require real PostgreSQL. + +Local deterministic runs skip these tests when the expected migrated test +database is absent or incompatible. CI sets ``PATCHFROG_REQUIRE_POSTGRES=1`` +because it declares the service itself; under that contract an unavailable, +misconfigured, or unmigrated database is an infrastructure failure, not a +pass. +""" + +from __future__ import annotations + +import os + +import pytest +from asyncpg import PostgresError # type: ignore[import-untyped] +from sqlalchemy import text +from sqlalchemy.exc import SQLAlchemyError +from sqlalchemy.ext.asyncio import AsyncEngine, create_async_engine + +POSTGRES_TEST_URL = "postgresql+asyncpg://patchfrog:patchfrog@localhost:5432/patchfrog" + + +async def postgres_engine_or_skip(required_table: str) -> AsyncEngine: + """Return a usable migrated Postgres engine or apply the explicit policy.""" + + engine = create_async_engine(POSTGRES_TEST_URL) + try: + async with engine.begin() as conn: + await conn.execute(text(f'SELECT 1 FROM "{required_table}" LIMIT 1')) + # asyncpg authentication/startup failures can escape before SQLAlchemy + # wraps them; both layers are part of this prerequisite boundary. + except (SQLAlchemyError, PostgresError, OSError) as exc: + await engine.dispose() + detail = f"real PostgreSQL prerequisite unavailable or unmigrated ({type(exc).__name__})" + if os.environ.get("PATCHFROG_REQUIRE_POSTGRES") == "1": + raise RuntimeError(detail) from exc + pytest.skip(f"{detail}; run `docker compose up -d postgres` and `alembic upgrade head`") + return engine + + +__all__ = ["postgres_engine_or_skip"] diff --git a/tests/unit/test_analyzer_binary_resolution.py b/tests/unit/test_analyzer_binary_resolution.py new file mode 100644 index 0000000..26db32d --- /dev/null +++ b/tests/unit/test_analyzer_binary_resolution.py @@ -0,0 +1,38 @@ +from __future__ import annotations + +import os +import sys +from pathlib import Path + +import pytest + +from patchfrog.analysis.analyzers.base import resolve_analyzer_binary + + +def test_resolves_console_script_beside_active_interpreter_when_not_on_path( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + interpreter = tmp_path / "python" + analyzer = tmp_path / "example-analyzer" + interpreter.write_text("") + analyzer.write_text("#!/bin/sh\n") + analyzer.chmod(0o755) + monkeypatch.setattr(sys, "executable", str(interpreter)) + monkeypatch.setenv("PATH", "") + + assert resolve_analyzer_binary("example-analyzer") == str(analyzer) + + +def test_non_executable_sibling_is_not_reported_available( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + interpreter = tmp_path / "python" + analyzer = tmp_path / "example-analyzer" + interpreter.write_text("") + analyzer.write_text("not executable") + analyzer.chmod(0o644) + monkeypatch.setattr(sys, "executable", str(interpreter)) + monkeypatch.setenv("PATH", "") + + assert not os.access(analyzer, os.X_OK) + assert resolve_analyzer_binary("example-analyzer") is None diff --git a/tests/unit/test_publishing_planner.py b/tests/unit/test_publishing_planner.py index 97c6440..d5b8306 100644 --- a/tests/unit/test_publishing_planner.py +++ b/tests/unit/test_publishing_planner.py @@ -235,6 +235,101 @@ def test_max_summary_findings_cap_produces_omitted() -> None: assert all("cap" in c.reason for c in plan.omitted) +def test_high_confidence_correctness_finding_below_severity_floor_is_not_silently_omitted() -> None: + """A finding that already survived reviewer proposal + critic + verification + dedup with Confidence.HIGH in a specialist category + (correctness/security) must not be discarded by config.min_severity + alone -- see patchfrog.publishing.planner's module docstring, funnel + step 1, and _bypasses_severity_floor.""" + + finding = _finding(line=5, severity=Severity.LOW, confidence=Confidence.HIGH) + plan = _build([finding], config=PublicationConfig()) # production default: min_severity=MEDIUM + + assert plan.omitted == () + assert len(plan.inline_comments) + len(plan.summary_only) == 1 + + +def test_medium_confidence_finding_below_severity_floor_is_still_omitted() -> None: + """The severity-floor bypass is scoped to Confidence.HIGH only -- + noise controls for lower-confidence findings are unaffected.""" + + finding = _finding(line=5, severity=Severity.LOW, confidence=Confidence.MEDIUM) + plan = _build([finding], config=PublicationConfig()) + + assert len(plan.omitted) == 1 + assert plan.omitted[0].reason == "below minimum severity threshold" + + +def test_high_confidence_non_specialist_category_below_severity_floor_is_still_omitted() -> None: + """The bypass only applies to the two specialist roles (correctness, + security) -- a high-confidence style/maintainability nit is still + governed by config.min_severity exactly as before.""" + + finding = PublishableFinding( + finding_id=uuid.uuid4(), + title="nit", + message="nit message", + category=FindingCategory.STYLE, + severity=Severity.LOW, + confidence=Confidence.HIGH, + file_path="src/billing.py", + start_line=5, + end_line=5, + reasoning_summary="because", + ) + plan = _build([finding], config=PublicationConfig()) + + assert len(plan.omitted) == 1 + + +def test_pr_57_swapped_counts_finding_is_not_silently_omitted() -> None: + """Regression fixture: production evidence for PR #57 head + 44b9ec0a0c68c1073f62e92b4369619bc84e46e7 showed the reviewer accepting + a real, critic-verified finding (category=correctness, confidence=high, + severity=low) for the intentional swapped inline/summary-only counts in + format_summary_body -- but the default min_severity=MEDIUM floor + discarded it outright before it was ever considered for inline/summary + placement, so GitHub received nothing despite review_run_completed + reporting accepted_count=1 (status=skipped_no_findings, omitted=1). + An accepted, high-confidence correctness finding must land inline or + summary-only, never omitted by severity alone.""" + + changed_files, diff_files = _whole_file_changed_files(path="patchfrog/publishing/body.py") + finding = PublishableFinding( + finding_id=uuid.uuid4(), + title="Inline/summary-only finding counts are swapped in the review summary body", + message=( + "format_summary_body labels len(summary_only_findings) as " + "'Published inline' and len(inline_findings) as 'Summary-only' -- the two are transposed." + ), + category=FindingCategory.CORRECTNESS, + severity=Severity.LOW, + confidence=Confidence.HIGH, + file_path="patchfrog/publishing/body.py", + start_line=9, + end_line=9, + reasoning_summary="the two len() calls are swapped relative to their labels", + suggested_fix=None, + ) + + plan = PublicationPlanner().build_plan( + publication_id=uuid.uuid4(), + snapshot=_snapshot(), + findings=[finding], + changed_files=changed_files, + diff_files=diff_files, + config=PublicationConfig(), # production defaults, including min_severity=MEDIUM + mode=ReviewPublicationMode.DRY_RUN, + current_head_sha=_HEAD_SHA, + ) + + assert plan.omitted == () + assert plan.status in (ReviewPublicationStatus.DRY_RUN, ReviewPublicationStatus.PLANNED) + assert len(plan.inline_comments) + len(plan.summary_only) == 1 + placed = plan.inline_comments[0] if plan.inline_comments else plan.summary_only[0] + assert placed.finding_id == finding.finding_id + + def test_selection_prefers_higher_severity_under_inline_cap() -> None: low = _finding(line=2, severity=Severity.LOW, title="low") critical = _finding(line=3, severity=Severity.CRITICAL, title="critical") From f2efe48fcde22dd8a695e5d4ea1ae32775de77aa Mon Sep 17 00:00:00 2001 From: Kadir Eren Altintas Date: Mon, 21 Sep 2026 20:50:09 +0300 Subject: [PATCH 2/7] add enforceable review cost budgets --- .env.example | 8 + apps/worker/tasks/review_pull_request.py | 31 +- docs/deployment.md | 8 + docs/model-routing.md | 2 + docs/quality-cost-guard.md | 27 ++ .../versions/0032_review_cost_budget.py | 42 +++ patchfrog/cli.py | 8 +- patchfrog/config/settings.py | 32 +++ patchfrog/ops/metrics.py | 9 + patchfrog/persistence/models/review.py | 9 + .../persistence/repositories/review_run.py | 27 ++ patchfrog/review/budget.py | 272 ++++++++++++++++++ patchfrog/review/config.py | 17 +- patchfrog/review/config_resolution.py | 19 ++ patchfrog/review/critic.py | 19 ++ patchfrog/review/domain.py | 2 + patchfrog/review/orchestration.py | 61 +++- patchfrog/review/provider.py | 57 ++++ .../review/providers/anthropic_provider.py | 21 +- patchfrog/review/providers/gemini_provider.py | 19 +- patchfrog/review/providers/openai_provider.py | 25 +- patchfrog/review/service.py | 89 +++++- patchfrog/routing/domain.py | 4 + patchfrog/routing/router.py | 41 ++- tests/integration/test_review_idempotency.py | 5 + .../test_review_orchestration_end_to_end.py | 34 +++ ...view_pull_request_model_router_fallback.py | 9 +- ...ew_pull_request_provider_trust_boundary.py | 5 +- .../test_change_intelligence_versioning.py | 2 +- .../test_contract_intelligence_versioning.py | 2 +- .../test_cross_pr_intelligence_versioning.py | 2 +- ...test_cross_repo_intelligence_versioning.py | 2 +- ...test_executable_verification_versioning.py | 2 +- ...historical_regression_memory_versioning.py | 2 +- .../test_intent_verification_versioning.py | 2 +- tests/unit/test_model_router.py | 54 +++- .../test_repository_learnings_versioning.py | 2 +- tests/unit/test_review_budget.py | 101 +++++++ tests/unit/test_review_config.py | 5 +- .../test_review_openai_provider_contract.py | 35 ++- tests/unit/test_review_operator_hard_caps.py | 3 +- tests/unit/test_review_retry_budget.py | 35 +++ tests/unit/test_settings.py | 9 + .../unit/test_test_intelligence_versioning.py | 2 +- ...test_trajectory_intelligence_versioning.py | 2 +- 45 files changed, 1103 insertions(+), 61 deletions(-) create mode 100644 migrations/versions/0032_review_cost_budget.py create mode 100644 patchfrog/review/budget.py create mode 100644 tests/unit/test_review_budget.py create mode 100644 tests/unit/test_review_retry_budget.py diff --git a/.env.example b/.env.example index 257029e..83b58cf 100644 --- a/.env.example +++ b/.env.example @@ -35,6 +35,14 @@ GITHUB_API_TIMEOUT_SECONDS=10 # PATCHFROG_REVIEW_MODEL=claude-opus-5 # e.g. gemini-3.6-flash for gemini # PATCHFROG_REVIEW_CRITIC_MODEL= # optional; defaults to the reviewer model # PATCHFROG_REVIEW_REQUEST_TIMEOUT_SECONDS= # optional; 30s default (120s default for gemini) +# PATCHFROG_ROUTER_CHEAP_PROVIDER= # optional cheap route for small reviews +# PATCHFROG_ROUTER_CHEAP_MODEL= # optional model for the cheap provider +# PATCHFROG_MAX_PROVIDER_CALLS=500 +# PATCHFROG_MAX_RETRY_ATTEMPTS=200 +# PATCHFROG_MAX_TOTAL_OUTPUT_TOKENS=250000 +# PATCHFROG_MAX_ESTIMATED_COST_USD= # optional; requires pricing below +# PATCHFROG_MAX_REVIEW_ELAPSED_SECONDS= # optional hard provider-work ceiling +# PATCHFROG_PROVIDER_PRICING='{"provider/model":{"input_usd_per_million_tokens":1.0,"output_usd_per_million_tokens":4.0}}' # # Only the credential for the provider actually selected above needs to # be set. Never set either of these in .patchfrog.yml or any diff --git a/apps/worker/tasks/review_pull_request.py b/apps/worker/tasks/review_pull_request.py index ec953cb..09e462e 100644 --- a/apps/worker/tasks/review_pull_request.py +++ b/apps/worker/tasks/review_pull_request.py @@ -48,6 +48,7 @@ PullRequestRepository, RepositoryRepository, ) +from patchfrog.review.budget import PricingCatalog from patchfrog.review.config import MalformedReviewConfigError from patchfrog.review.config_resolution import ( apply_operator_hard_caps, @@ -62,7 +63,7 @@ ) from patchfrog.review_memory.config_resolution import resolve_repository_incremental_config from patchfrog.review_memory.service import IncrementalReviewMemoryService -from patchfrog.routing.router import ModelRouter +from patchfrog.routing.router import ModelRouter, is_small_review logger = structlog.get_logger(__name__) @@ -176,7 +177,9 @@ async def _review_pull_request( # docs/governance-policy.md's "Model Router integration (Z14)". runtime_config = resolve_review_runtime_config(settings) route_plan = ModelRouter(settings=settings, allowed_providers=settings.allowed_providers).route( - runtime_config=runtime_config, critic_enabled=review_config.critic_enabled + runtime_config=runtime_config, + critic_enabled=review_config.critic_enabled, + prefer_low_cost=is_small_review(diff_files), ) # Every role maps to the same provider instance in v1 (the # router routes once per run, not once per candidate -- see @@ -234,6 +237,7 @@ async def _review_pull_request( route_plan=route_plan, verifier_dispatcher=verifier_dispatcher, verification_snapshot_root=settings.verification_snapshot_root, + pricing_catalog=PricingCatalog.from_config(settings.provider_pricing), ) summary = await service.review_pull_request( repository_id=repository_id, @@ -267,9 +271,26 @@ async def _review_pull_request( } metrics.reviews_completed_total.labels(status=summary.status.value).inc() metrics.review_duration_seconds.observe(summary.duration_ms / 1000) - metrics.provider_calls_total.labels(**provider_labels, role="reviewer").inc(summary.candidates_reviewed) - metrics.provider_input_tokens_total.labels(**provider_labels).inc(summary.reviewer_usage.input_tokens) - metrics.provider_output_tokens_total.labels(**provider_labels).inc(summary.reviewer_usage.output_tokens) + if summary.budget is not None and not summary.reused_existing_run: + for cost_metric in summary.budget.by_model: + cost_labels = {"provider": cost_metric.provider, "model": cost_metric.model} + metrics.provider_calls_total.labels(**cost_labels, role="all").inc(cost_metric.call_count) + metrics.provider_retries_total.labels(**cost_labels).inc(cost_metric.retry_count) + metrics.provider_input_tokens_total.labels(**cost_labels).inc(cost_metric.input_tokens) + metrics.provider_output_tokens_total.labels(**cost_labels).inc(cost_metric.output_tokens) + metrics.provider_estimated_cost_usd_total.labels(**cost_labels).inc( + cost_metric.estimated_cost_usd + ) + if summary.budget.termination_reason is not None: + metrics.review_budget_terminations_total.labels( + reason=summary.budget.termination_reason.value + ).inc() + elif summary.budget is None and not summary.reused_existing_run: # Historical summaries. + metrics.provider_calls_total.labels(**provider_labels, role="reviewer").inc( + summary.candidates_reviewed + ) + metrics.provider_input_tokens_total.labels(**provider_labels).inc(summary.reviewer_usage.input_tokens) + metrics.provider_output_tokens_total.labels(**provider_labels).inc(summary.reviewer_usage.output_tokens) metrics.findings_generated_total.inc(summary.proposals_count) metrics.findings_suppressed_total.labels(reason="duplicate").inc(summary.suppressed_duplicate_count) for tier, count in summary.candidates_by_tier.items(): diff --git a/docs/deployment.md b/docs/deployment.md index c55c057..7550af8 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -178,6 +178,8 @@ Two further environment variables, both optional, both never |---|---| | `PATCHFROG_ROUTER_FALLBACK_PROVIDER` | A single provider to fall back to if `PATCHFROG_REVIEW_PROVIDER` has no credential configured. Bounded to exactly one hop -- no chain to a further fallback. | | `PATCHFROG_ROUTER_CRITIC_PROVIDER` | Explicitly pin the critic role to a specific provider family (must also have its own credential set). If unset and more than one provider is configured, the router auto-selects a different family than the reviewer for family diversity; with only one provider configured, the critic always uses that same family. | +| `PATCHFROG_ROUTER_CHEAP_PROVIDER` | Optional provider used for small reviews (at most 3 files and 80 changed lines), but only when credentialed and allowed by governance policy. | +| `PATCHFROG_ROUTER_CHEAP_MODEL` | Optional model for the cheap provider; must match that provider family. | ``` PATCHFROG_REVIEW_PROVIDER=anthropic @@ -216,6 +218,12 @@ credentials -- **never** `.patchfrog.yml`-controlled: | `PATCHFROG_MAX_OUTPUT_TOKENS_PER_CANDIDATE` | Hard ceiling on `ReviewConfig.max_output_tokens_per_candidate` | `16000` | | `PATCHFROG_MAX_CONCURRENT_REVIEW_REQUESTS` | Hard ceiling on `ReviewConfig.max_concurrent_requests` | `16` | | `PATCHFROG_MAX_REVIEW_RETRIES` | Hard ceiling on `ReviewConfig.max_retries` | `5` | +| `PATCHFROG_MAX_PROVIDER_CALLS` | Hard ceiling on all reviewer, critic, retry, and fallback calls | `500` | +| `PATCHFROG_MAX_RETRY_ATTEMPTS` | Review-wide retry/fallback ceiling | `200` | +| `PATCHFROG_MAX_TOTAL_OUTPUT_TOKENS` | Review-wide estimated/reported output-token ceiling | `250000` | +| `PATCHFROG_MAX_ESTIMATED_COST_USD` | Optional estimated-dollar ceiling; requires matching pricing entries | unset | +| `PATCHFROG_MAX_REVIEW_ELAPSED_SECONDS` | Optional hard wall-clock ceiling for provider work | unset | +| `PATCHFROG_PROVIDER_PRICING` | JSON object keyed by `provider/model`, with input/output USD per million tokens | `{}` | `patchfrog.review.config_resolution.apply_operator_hard_caps` computes `effective = min(repo_intent, operator_hard_cap)` per field, applied by diff --git a/docs/model-routing.md b/docs/model-routing.md index c011701..3b0562f 100644 --- a/docs/model-routing.md +++ b/docs/model-routing.md @@ -251,6 +251,8 @@ this explicitly. | `PATCHFROG_REVIEW_PROVIDER` | Preferred/primary provider family (unchanged from before this milestone; now also accepts `openai`) | | `PATCHFROG_ROUTER_FALLBACK_PROVIDER` | Optional, single fallback family if the preferred one has no credential | | `PATCHFROG_ROUTER_CRITIC_PROVIDER` | Optional, explicit critic family (overrides auto-diversity) | +| `PATCHFROG_ROUTER_CHEAP_PROVIDER` | Optional low-cost family for deterministically small reviews; governance and credential checks still apply | +| `PATCHFROG_ROUTER_CHEAP_MODEL` | Optional operator-selected model for the low-cost family | Self-host: the operator controls which providers are available and how routing behaves. Future PatchFrog Cloud: production model-routing diff --git a/docs/quality-cost-guard.md b/docs/quality-cost-guard.md index c459af2..39133cb 100644 --- a/docs/quality-cost-guard.md +++ b/docs/quality-cost-guard.md @@ -215,6 +215,28 @@ candidate, not just the one that triggered it. ## Global run budget and reservation +`patchfrog.review.budget.ReviewBudget` is the single, provider-neutral +ledger shared by reviewer roles, critic calls, retries, and one-hop +fallbacks. It reserves estimated input/output tokens and operator-supplied +model pricing before every provider call, then reconciles successful +calls to provider-reported usage. Its ceilings cover provider calls, +retry attempts, input tokens, output tokens, estimated USD, and optional +elapsed time. Exhaustion produces a typed terminal reason and a partial +run; it never silently becomes a clean review. A dollar ceiling without +a `provider/model` pricing entry fails closed as `pricing_unavailable`. + +Pricing is operator data, not a vendor table embedded in PatchFrog. For +example: + +```text +PATCHFROG_PROVIDER_PRICING={"openai/gpt-example":{"input_usd_per_million_tokens":1.0,"output_usd_per_million_tokens":4.0}} +PATCHFROG_MAX_ESTIMATED_COST_USD=0.25 +``` + +The persisted breakdown contains only provider, model, call/retry counts, +token counts, and estimated cost. Prompts, source text, responses, keys, +and secret values are never part of cost telemetry. + `max_total_input_tokens` is a true run-level guard across **every** provider call that consumes input tokens: both specialist roles' input, *and* critic input (previously unguarded -- a real gap this milestone @@ -287,6 +309,11 @@ controlled, exactly like provider/model credentials): | `PATCHFROG_MAX_OUTPUT_TOKENS_PER_CANDIDATE` | 16,000 | | `PATCHFROG_MAX_CONCURRENT_REVIEW_REQUESTS` | 16 | | `PATCHFROG_MAX_REVIEW_RETRIES` | 5 | +| `PATCHFROG_MAX_PROVIDER_CALLS` | 500 | +| `PATCHFROG_MAX_RETRY_ATTEMPTS` | 200 | +| `PATCHFROG_MAX_TOTAL_OUTPUT_TOKENS` | 250,000 | +| `PATCHFROG_MAX_ESTIMATED_COST_USD` | unset | +| `PATCHFROG_MAX_REVIEW_ELAPSED_SECONDS` | unset | `patchfrog.review.config_resolution.apply_operator_hard_caps` computes `effective = min(repo_intent, operator_hard_cap)` independently per diff --git a/migrations/versions/0032_review_cost_budget.py b/migrations/versions/0032_review_cost_budget.py new file mode 100644 index 0000000..4211b44 --- /dev/null +++ b/migrations/versions/0032_review_cost_budget.py @@ -0,0 +1,42 @@ +"""Add first-class review budget and cost telemetry + +Revision ID: 0032_review_cost_budget +Revises: 0031_critic_rejection_category +Create Date: 2026-09-21 + +""" +from __future__ import annotations + +from collections.abc import Sequence + +import sqlalchemy as sa +from alembic import op + +revision: str = "0032_review_cost_budget" +down_revision: str | None = "0031_critic_rejection_category" +branch_labels: Sequence[str] | None = None +depends_on: Sequence[str] | None = None + + +def upgrade() -> None: + op.add_column("review_runs", sa.Column("provider_calls", sa.Integer(), nullable=False, server_default="0")) + op.add_column("review_runs", sa.Column("retry_attempts", sa.Integer(), nullable=False, server_default="0")) + op.add_column("review_runs", sa.Column("budget_input_tokens", sa.Integer(), nullable=False, server_default="0")) + op.add_column("review_runs", sa.Column("budget_output_tokens", sa.Integer(), nullable=False, server_default="0")) + op.add_column("review_runs", sa.Column("estimated_cost_usd", sa.Float(), nullable=False, server_default="0")) + op.add_column("review_runs", sa.Column("budget_elapsed_seconds", sa.Float(), nullable=False, server_default="0")) + op.add_column("review_runs", sa.Column("budget_termination_reason", sa.String(length=32), nullable=True)) + op.add_column( + "review_runs", sa.Column("provider_cost_breakdown", sa.Text(), nullable=False, server_default="[]") + ) + + +def downgrade() -> None: + op.drop_column("review_runs", "provider_cost_breakdown") + op.drop_column("review_runs", "budget_termination_reason") + op.drop_column("review_runs", "budget_elapsed_seconds") + op.drop_column("review_runs", "estimated_cost_usd") + op.drop_column("review_runs", "budget_output_tokens") + op.drop_column("review_runs", "budget_input_tokens") + op.drop_column("review_runs", "retry_attempts") + op.drop_column("review_runs", "provider_calls") diff --git a/patchfrog/cli.py b/patchfrog/cli.py index f4ae14e..4236b6f 100644 --- a/patchfrog/cli.py +++ b/patchfrog/cli.py @@ -132,6 +132,7 @@ ReviewRunNotAssociatedWithPullRequestError, ) from patchfrog.repository.git import GitError, run_git +from patchfrog.review.budget import PricingCatalog from patchfrog.review.candidates import ReviewCandidateGenerator from patchfrog.review.config import MalformedReviewConfigError, ReviewConfig from patchfrog.review.config_resolution import ( @@ -155,7 +156,7 @@ from patchfrog.review_memory.domain import IncrementalPlan, ReviewMemoryFinding from patchfrog.review_memory.queries import ReviewMemoryQueryService from patchfrog.review_memory.service import IncrementalReviewMemoryService -from patchfrog.routing.router import ModelRouter +from patchfrog.routing.router import ModelRouter, is_small_review from patchfrog.telemetry.beta_summary import BetaSummary, compute_beta_summary, parse_since from patchfrog.telemetry.collector import collect_review_telemetry from patchfrog.telemetry.reporting import render_markdown_snapshot, snapshot_to_dict @@ -421,12 +422,15 @@ async def _review_local( # patchfrog.routing.router). runtime_config = resolve_review_runtime_config(settings) route_plan = ModelRouter(settings=settings).route( - runtime_config=runtime_config, critic_enabled=config.critic_enabled + runtime_config=runtime_config, + critic_enabled=config.critic_enabled, + prefer_low_cost=is_small_review(diff_files), ) service = PullRequestReviewService( session_factory=session_factory, route_plan=route_plan, + pricing_catalog=PricingCatalog.from_config(settings.provider_pricing), ) if not incremental: diff --git a/patchfrog/config/settings.py b/patchfrog/config/settings.py index 6bfc3b2..2ac5a79 100644 --- a/patchfrog/config/settings.py +++ b/patchfrog/config/settings.py @@ -87,6 +87,12 @@ class Settings(BaseSettings): router_critic_provider: str | None = Field( default=None, alias="PATCHFROG_ROUTER_CRITIC_PROVIDER" ) + router_cheap_provider: str | None = Field( + default=None, alias="PATCHFROG_ROUTER_CHEAP_PROVIDER" + ) + router_cheap_model: str | None = Field( + default=None, alias="PATCHFROG_ROUTER_CHEAP_MODEL" + ) #: Milestone Z14 (governance): deployment-wide provider allowlist fed #: straight into ModelRouter's own `allowed_providers` parameter -- #: `None` (default, unset) means no restriction. A comma-separated @@ -117,6 +123,20 @@ class Settings(BaseSettings): ) review_max_concurrent_requests: int = Field(default=16, alias="PATCHFROG_MAX_CONCURRENT_REVIEW_REQUESTS") review_max_retries: int = Field(default=5, alias="PATCHFROG_MAX_REVIEW_RETRIES") + review_max_provider_calls: int = Field(default=500, alias="PATCHFROG_MAX_PROVIDER_CALLS") + review_max_retry_attempts: int = Field(default=200, alias="PATCHFROG_MAX_RETRY_ATTEMPTS") + review_max_total_output_tokens: int = Field( + default=250_000, alias="PATCHFROG_MAX_TOTAL_OUTPUT_TOKENS" + ) + review_max_estimated_cost_usd: float | None = Field( + default=None, alias="PATCHFROG_MAX_ESTIMATED_COST_USD" + ) + review_max_elapsed_seconds: float | None = Field( + default=None, alias="PATCHFROG_MAX_REVIEW_ELAPSED_SECONDS" + ) + provider_pricing: dict[str, dict[str, float]] = Field( + default_factory=dict, alias="PATCHFROG_PROVIDER_PRICING" + ) # -- Public beta operational limits (patchfrog.ops) -- # All optional with conservative defaults; never required for @@ -236,6 +256,9 @@ def _validate_review_request_timeout_seconds(cls, value: float | None) -> float "review_max_output_tokens_per_candidate", "review_max_concurrent_requests", "review_max_retries", + "review_max_provider_calls", + "review_max_retry_attempts", + "review_max_total_output_tokens", ) @classmethod def _validate_review_hard_caps_positive(cls, value: int, info: ValidationInfo) -> int: @@ -243,6 +266,15 @@ def _validate_review_hard_caps_positive(cls, value: int, info: ValidationInfo) - raise ValueError(f"{info.field_name} must be positive, got {value!r}") return value + @field_validator("review_max_estimated_cost_usd", "review_max_elapsed_seconds") + @classmethod + def _validate_optional_review_hard_caps_positive( + cls, value: float | None, info: ValidationInfo + ) -> float | None: + if value is not None and value <= 0: + raise ValueError(f"{info.field_name} must be positive, got {value!r}") + return value + @model_validator(mode="after") def _resolve_private_key(self) -> Settings: inline, path = self.github_private_key, self.github_private_key_path diff --git a/patchfrog/ops/metrics.py b/patchfrog/ops/metrics.py index 92b2eca..a4b22c9 100644 --- a/patchfrog/ops/metrics.py +++ b/patchfrog/ops/metrics.py @@ -90,6 +90,15 @@ provider_output_tokens_total = Counter( "patchfrog_provider_output_tokens_total", "LLM provider output tokens produced", ["provider", "model"] ) +provider_retries_total = Counter( + "patchfrog_provider_retries_total", "LLM provider retry and fallback attempts", ["provider", "model"] +) +provider_estimated_cost_usd_total = Counter( + "patchfrog_provider_estimated_cost_usd_total", "Estimated LLM provider cost in USD", ["provider", "model"] +) +review_budget_terminations_total = Counter( + "patchfrog_review_budget_terminations_total", "Reviews stopped by a cost budget", ["reason"] +) findings_generated_total = Counter( "patchfrog_findings_generated_total", "Findings proposed by the AI reviewer, before validation/critic" diff --git a/patchfrog/persistence/models/review.py b/patchfrog/persistence/models/review.py index 30b09a7..484ba9d 100644 --- a/patchfrog/persistence/models/review.py +++ b/patchfrog/persistence/models/review.py @@ -172,6 +172,15 @@ class ReviewRunModel(Base): #: which never captured per-role latency at all -- never fabricated. reviewer_latency_ms: Mapped[float] = mapped_column(Float, default=0.0) + provider_calls: Mapped[int] = mapped_column(Integer, default=0) + retry_attempts: Mapped[int] = mapped_column(Integer, default=0) + budget_input_tokens: Mapped[int] = mapped_column(Integer, default=0) + budget_output_tokens: Mapped[int] = mapped_column(Integer, default=0) + estimated_cost_usd: Mapped[float] = mapped_column(Float, default=0.0) + budget_elapsed_seconds: Mapped[float] = mapped_column(Float, default=0.0) + budget_termination_reason: Mapped[str | None] = mapped_column(String(32), nullable=True) + provider_cost_breakdown: Mapped[str] = mapped_column(Text, default="[]") + duration_ms: Mapped[float | None] = mapped_column(Float, nullable=True) error_message: Mapped[str | None] = mapped_column(Text, nullable=True) started_at: Mapped[datetime] = mapped_column(DateTime(timezone=True)) diff --git a/patchfrog/persistence/repositories/review_run.py b/patchfrog/persistence/repositories/review_run.py index 819b9cf..5c663d6 100644 --- a/patchfrog/persistence/repositories/review_run.py +++ b/patchfrog/persistence/repositories/review_run.py @@ -18,6 +18,7 @@ from patchfrog.persistence.models.review import ReviewRunModel from patchfrog.repository_learnings.telemetry import RepositoryLearningsSummary from patchfrog.review.agents.roles import AgentRole +from patchfrog.review.budget import ReviewBudgetSnapshot from patchfrog.review.domain import ReviewRunStatus from patchfrog.review.effort_types import ReviewEffortTier from patchfrog.review_memory.config import NO_MEMORY_CONTEXT_FINGERPRINT @@ -225,6 +226,7 @@ async def mark_succeeded( cross_pr_intelligence: CrossPRIntelligenceSummary | None = None, cross_repo_intelligence: CrossRepoIntelligenceSummary | None = None, executable_verification: ExecutableVerificationSummary | None = None, + budget: ReviewBudgetSnapshot | None = None, ) -> ReviewRunModel: """Mark a run succeeded or partial. Returns the *canonical* run for this identity -- if a concurrent run already claimed @@ -285,6 +287,31 @@ async def mark_succeeded( model.retries_consumed = retries_consumed model.reviewer_latency_ms = reviewer_latency_ms model.calls_by_role = json.dumps({role.value: count for role, count in (calls_by_role or {}).items()}) + if budget is not None: + model.provider_calls = budget.provider_calls + model.retry_attempts = budget.retry_attempts + model.budget_input_tokens = budget.input_tokens + model.budget_output_tokens = budget.output_tokens + model.estimated_cost_usd = budget.estimated_cost_usd + model.budget_elapsed_seconds = budget.elapsed_seconds + model.budget_termination_reason = ( + budget.termination_reason.value if budget.termination_reason is not None else None + ) + model.provider_cost_breakdown = json.dumps( + [ + { + "provider": metric.provider, + "model": metric.model, + "call_count": metric.call_count, + "retry_count": metric.retry_count, + "input_tokens": metric.input_tokens, + "output_tokens": metric.output_tokens, + "estimated_cost_usd": metric.estimated_cost_usd, + } + for metric in budget.by_model + ], + sort_keys=True, + ) model.duration_ms = duration_ms if change_intelligence is not None: model.change_unit_count = change_intelligence.change_unit_count diff --git a/patchfrog/review/budget.py b/patchfrog/review/budget.py new file mode 100644 index 0000000..6f373bf --- /dev/null +++ b/patchfrog/review/budget.py @@ -0,0 +1,272 @@ +"""Provider-neutral review budget and deterministic cost accounting.""" + +from __future__ import annotations + +import asyncio +import time +from collections.abc import Callable, Mapping +from dataclasses import dataclass +from enum import StrEnum +from types import MappingProxyType + +from patchfrog.review.provider import ProviderIdentity, ProviderUsage + + +class BudgetTerminationReason(StrEnum): + PROVIDER_CALLS = "max_provider_calls" + RETRIES = "max_retry_attempts" + INPUT_TOKENS = "max_input_tokens" + OUTPUT_TOKENS = "max_output_tokens" + ESTIMATED_COST = "max_estimated_cost" + PRICING_UNAVAILABLE = "pricing_unavailable" + ELAPSED_TIME = "max_elapsed_time" + + +class BudgetExceeded(RuntimeError): + def __init__(self, reason: BudgetTerminationReason) -> None: + super().__init__(f"review budget exhausted: {reason.value}") + self.reason = reason + + +@dataclass(frozen=True, slots=True) +class ModelPricing: + input_usd_per_million_tokens: float + output_usd_per_million_tokens: float + + def estimate(self, *, input_tokens: int, output_tokens: int) -> float: + return ( + input_tokens * self.input_usd_per_million_tokens + + output_tokens * self.output_usd_per_million_tokens + ) / 1_000_000 + + +class PricingCatalog: + """Immutable operator-supplied pricing; no vendor/model is hardcoded.""" + + def __init__(self, prices: Mapping[str, ModelPricing] | None = None) -> None: + self._prices = MappingProxyType(dict(prices or {})) + + @staticmethod + def key(identity: ProviderIdentity) -> str: + return f"{identity.provider}/{identity.model}" + + def get(self, identity: ProviderIdentity) -> ModelPricing | None: + return self._prices.get(self.key(identity)) + + @classmethod + def from_config(cls, raw: Mapping[str, Mapping[str, float]]) -> PricingCatalog: + prices: dict[str, ModelPricing] = {} + for key, values in raw.items(): + input_rate = float(values.get("input_usd_per_million_tokens", 0.0)) + output_rate = float(values.get("output_usd_per_million_tokens", 0.0)) + if input_rate < 0 or output_rate < 0: + raise ValueError(f"provider pricing rates must be non-negative for {key!r}") + prices[key] = ModelPricing(input_rate, output_rate) + return cls(prices) + + +@dataclass(frozen=True, slots=True) +class CostBudget: + max_provider_calls: int + max_retry_attempts: int + max_input_tokens: int + max_output_tokens: int + max_estimated_cost_usd: float | None = None + max_elapsed_seconds: float | None = None + + +@dataclass(frozen=True, slots=True) +class CallReservation: + identity: ProviderIdentity + estimated_input_tokens: int + estimated_output_tokens: int + estimated_cost_usd: float + + +@dataclass(frozen=True, slots=True) +class ProviderCostMetric: + provider: str + model: str + call_count: int + retry_count: int + input_tokens: int + output_tokens: int + estimated_cost_usd: float + + +@dataclass(frozen=True, slots=True) +class ReviewBudgetSnapshot: + provider_calls: int + retry_attempts: int + input_tokens: int + output_tokens: int + estimated_cost_usd: float + elapsed_seconds: float + termination_reason: BudgetTerminationReason | None + by_model: tuple[ProviderCostMetric, ...] + + +@dataclass(slots=True) +class _MutableMetric: + provider: str + model: str + call_count: int = 0 + retry_count: int = 0 + input_tokens: int = 0 + output_tokens: int = 0 + estimated_cost_usd: float = 0.0 + + +class ReviewBudget: + """Atomic ledger shared by every reviewer, critic, retry, and fallback.""" + + def __init__( + self, + limits: CostBudget, + *, + pricing: PricingCatalog | None = None, + monotonic: Callable[[], float] = time.monotonic, + ) -> None: + self.limits = limits + self._pricing = pricing or PricingCatalog() + self._monotonic = monotonic + self._started_at = self._now() + self._lock = asyncio.Lock() + self._provider_calls = 0 + self._retry_attempts = 0 + self._input_tokens = 0 + self._output_tokens = 0 + self._estimated_cost_usd = 0.0 + self._termination_reason: BudgetTerminationReason | None = None + self._by_model: dict[str, _MutableMetric] = {} + + def _now(self) -> float: + return float(self._monotonic()) + + def _cost(self, identity: ProviderIdentity, input_tokens: int, output_tokens: int) -> float: + price = self._pricing.get(identity) + return price.estimate(input_tokens=input_tokens, output_tokens=output_tokens) if price else 0.0 + + def _deny(self, reason: BudgetTerminationReason) -> None: + if self._termination_reason is None: + self._termination_reason = reason + raise BudgetExceeded(reason) + + async def reserve_call( + self, + identity: ProviderIdentity, + *, + estimated_input_tokens: int, + estimated_output_tokens: int, + is_retry: bool, + ) -> CallReservation: + async with self._lock: + if self._termination_reason is not None: + raise BudgetExceeded(self._termination_reason) + elapsed = self._now() - self._started_at + if self.limits.max_elapsed_seconds is not None and elapsed >= self.limits.max_elapsed_seconds: + self._deny(BudgetTerminationReason.ELAPSED_TIME) + if self._provider_calls + 1 > self.limits.max_provider_calls: + self._deny(BudgetTerminationReason.PROVIDER_CALLS) + if is_retry and self._retry_attempts + 1 > self.limits.max_retry_attempts: + self._deny(BudgetTerminationReason.RETRIES) + if self._input_tokens + estimated_input_tokens > self.limits.max_input_tokens: + self._deny(BudgetTerminationReason.INPUT_TOKENS) + if self._output_tokens + estimated_output_tokens > self.limits.max_output_tokens: + self._deny(BudgetTerminationReason.OUTPUT_TOKENS) + price = self._pricing.get(identity) + if self.limits.max_estimated_cost_usd is not None and price is None: + self._deny(BudgetTerminationReason.PRICING_UNAVAILABLE) + estimated_cost = self._cost(identity, estimated_input_tokens, estimated_output_tokens) + if ( + self.limits.max_estimated_cost_usd is not None + and self._estimated_cost_usd + estimated_cost > self.limits.max_estimated_cost_usd + ): + self._deny(BudgetTerminationReason.ESTIMATED_COST) + + self._provider_calls += 1 + self._retry_attempts += int(is_retry) + self._input_tokens += estimated_input_tokens + self._output_tokens += estimated_output_tokens + self._estimated_cost_usd += estimated_cost + key = self._pricing.key(identity) + metric = self._by_model.setdefault( + key, _MutableMetric(provider=identity.provider, model=identity.model) + ) + metric.call_count += 1 + metric.retry_count += int(is_retry) + metric.input_tokens += estimated_input_tokens + metric.output_tokens += estimated_output_tokens + metric.estimated_cost_usd += estimated_cost + return CallReservation(identity, estimated_input_tokens, estimated_output_tokens, estimated_cost) + + async def terminate(self, reason: BudgetTerminationReason) -> None: + """Record an external ceiling (for example an asyncio wall-clock timeout).""" + + async with self._lock: + if self._termination_reason is None: + self._termination_reason = reason + + async def reconcile(self, reservation: CallReservation, usage: ProviderUsage) -> None: + """Replace a successful call's conservative reservation with actual usage.""" + + async with self._lock: + actual_cost = self._cost(reservation.identity, usage.input_tokens, usage.output_tokens) + input_delta = usage.input_tokens - reservation.estimated_input_tokens + output_delta = usage.output_tokens - reservation.estimated_output_tokens + cost_delta = actual_cost - reservation.estimated_cost_usd + self._input_tokens = max(0, self._input_tokens + input_delta) + self._output_tokens = max(0, self._output_tokens + output_delta) + self._estimated_cost_usd = max(0.0, self._estimated_cost_usd + cost_delta) + metric = self._by_model[self._pricing.key(reservation.identity)] + metric.input_tokens = max(0, metric.input_tokens + input_delta) + metric.output_tokens = max(0, metric.output_tokens + output_delta) + metric.estimated_cost_usd = max(0.0, metric.estimated_cost_usd + cost_delta) + if self._termination_reason is None: + if self._input_tokens >= self.limits.max_input_tokens: + self._termination_reason = BudgetTerminationReason.INPUT_TOKENS + elif self._output_tokens >= self.limits.max_output_tokens: + self._termination_reason = BudgetTerminationReason.OUTPUT_TOKENS + elif ( + self.limits.max_estimated_cost_usd is not None + and self._estimated_cost_usd >= self.limits.max_estimated_cost_usd + ): + self._termination_reason = BudgetTerminationReason.ESTIMATED_COST + + async def snapshot(self) -> ReviewBudgetSnapshot: + async with self._lock: + metrics = tuple( + ProviderCostMetric( + provider=m.provider, + model=m.model, + call_count=m.call_count, + retry_count=m.retry_count, + input_tokens=m.input_tokens, + output_tokens=m.output_tokens, + estimated_cost_usd=m.estimated_cost_usd, + ) + for _, m in sorted(self._by_model.items()) + ) + return ReviewBudgetSnapshot( + provider_calls=self._provider_calls, + retry_attempts=self._retry_attempts, + input_tokens=self._input_tokens, + output_tokens=self._output_tokens, + estimated_cost_usd=self._estimated_cost_usd, + elapsed_seconds=max(0.0, self._now() - self._started_at), + termination_reason=self._termination_reason, + by_model=metrics, + ) + + +__all__ = [ + "BudgetExceeded", + "BudgetTerminationReason", + "CallReservation", + "CostBudget", + "ModelPricing", + "PricingCatalog", + "ProviderCostMetric", + "ReviewBudget", + "ReviewBudgetSnapshot", +] diff --git a/patchfrog/review/config.py b/patchfrog/review/config.py index c877b00..28cc3ee 100644 --- a/patchfrog/review/config.py +++ b/patchfrog/review/config.py @@ -54,7 +54,7 @@ #: boundary. (Previously bumped to 3 because `provider`/`model`/ #: `critic_model`/`request_timeout_seconds` were removed from #: repository-controlled config entirely -- see module docstring.) -CONFIG_SCHEMA_VERSION = 4 +CONFIG_SCHEMA_VERSION = 5 #: Bumped whenever patchfrog.review.prompt's system/user prompt templates #: change materially enough that a prior run's proposals can no longer be @@ -186,6 +186,9 @@ DEFAULT_MAX_CONCURRENT_REQUESTS = 4 DEFAULT_MIN_FINAL_CONFIDENCE: Confidence = Confidence.MEDIUM DEFAULT_MAX_RETRIES = 2 +DEFAULT_MAX_PROVIDER_CALLS = 200 +DEFAULT_MAX_RETRY_ATTEMPTS = 100 +DEFAULT_MAX_TOTAL_OUTPUT_TOKENS = 100_000 #: Fields that select PatchFrog's AI provider/model/timeout -- an #: operator/deployment concern, never a repository one (see @@ -219,6 +222,11 @@ class ReviewConfig(BaseModel): max_concurrent_requests: int = DEFAULT_MAX_CONCURRENT_REQUESTS min_final_confidence: Confidence = DEFAULT_MIN_FINAL_CONFIDENCE max_retries: int = DEFAULT_MAX_RETRIES + max_provider_calls: int = DEFAULT_MAX_PROVIDER_CALLS + max_retry_attempts: int = DEFAULT_MAX_RETRY_ATTEMPTS + max_total_output_tokens: int = DEFAULT_MAX_TOTAL_OUTPUT_TOKENS + max_estimated_cost_usd: float | None = None + max_elapsed_seconds: float | None = None def fingerprint(self) -> str: """A deterministic fingerprint of repository-controlled review @@ -235,6 +243,13 @@ def fingerprint(self) -> str: "max_input_tokens_per_candidate": self.max_input_tokens_per_candidate, "max_output_tokens_per_candidate": self.max_output_tokens_per_candidate, "max_total_input_tokens": self.max_total_input_tokens, + "max_total_output_tokens": self.max_total_output_tokens, + "max_provider_calls": self.max_provider_calls, + "max_retry_attempts": self.max_retry_attempts, + "max_estimated_cost_usd": self.max_estimated_cost_usd, + "max_elapsed_seconds": self.max_elapsed_seconds, + "max_concurrent_requests": self.max_concurrent_requests, + "max_retries": self.max_retries, "min_final_confidence": self.min_final_confidence.value, } canonical = json.dumps(payload, sort_keys=True, separators=(",", ":")) diff --git a/patchfrog/review/config_resolution.py b/patchfrog/review/config_resolution.py index 3825f54..1b664ab 100644 --- a/patchfrog/review/config_resolution.py +++ b/patchfrog/review/config_resolution.py @@ -110,4 +110,23 @@ def apply_operator_hard_caps(repo_config: ReviewConfig, *, settings: Settings) - ), min_final_confidence=repo_config.min_final_confidence, max_retries=min(repo_config.max_retries, settings.review_max_retries), + max_provider_calls=min(repo_config.max_provider_calls, settings.review_max_provider_calls), + max_retry_attempts=min(repo_config.max_retry_attempts, settings.review_max_retry_attempts), + max_total_output_tokens=min( + repo_config.max_total_output_tokens, settings.review_max_total_output_tokens + ), + max_estimated_cost_usd=_optional_min( + repo_config.max_estimated_cost_usd, settings.review_max_estimated_cost_usd + ), + max_elapsed_seconds=_optional_min( + repo_config.max_elapsed_seconds, settings.review_max_elapsed_seconds + ), ) + + +def _optional_min(left: float | None, right: float | None) -> float | None: + if left is None: + return right + if right is None: + return left + return min(left, right) diff --git a/patchfrog/review/critic.py b/patchfrog/review/critic.py index 8b9810f..0c0f67b 100644 --- a/patchfrog/review/critic.py +++ b/patchfrog/review/critic.py @@ -14,6 +14,7 @@ import json from patchfrog.analysis.domain import Confidence, Severity +from patchfrog.review.budget import ReviewBudget from patchfrog.review.domain import ( AIReviewFinding, CriticDecision, @@ -41,6 +42,10 @@ def identity(self) -> ProviderIdentity: return self._provider.identity + @property + def max_output_tokens(self) -> int: + return self._max_output_tokens + async def critique( self, validated: ValidatedFinding, @@ -49,6 +54,9 @@ async def critique( context_text: str, conflicting_finding: AIReviewFinding | None = None, executable_verification_text: str = "", + budget: ReviewBudget | None = None, + estimated_input_tokens: int = 0, + is_retry: bool = False, ) -> CriticVerdict: system_prompt, user_prompt = build_critic_prompt( candidate=candidate, @@ -64,7 +72,18 @@ async def critique( schema_name="critic_verdict", max_output_tokens=self._max_output_tokens, ) + reservation = None + if budget is not None: + reservation = await budget.reserve_call( + self._provider.identity, + estimated_input_tokens=estimated_input_tokens, + estimated_output_tokens=self._max_output_tokens, + is_retry=is_retry, + ) result = await self._provider.generate_structured(request) + if reservation is not None: + assert budget is not None + await budget.reconcile(reservation, result.usage) try: payload = json.loads(result.raw_json) diff --git a/patchfrog/review/domain.py b/patchfrog/review/domain.py index a4c005d..3559cf9 100644 --- a/patchfrog/review/domain.py +++ b/patchfrog/review/domain.py @@ -27,6 +27,7 @@ from patchfrog.analysis.domain import Confidence, FindingCategory, Severity from patchfrog.review.agents.roles import AgentRole +from patchfrog.review.budget import ReviewBudgetSnapshot from patchfrog.review.effort_types import ReviewEffortTier @@ -372,3 +373,4 @@ class ReviewRunSummary: #: wall-clock measurement. See :mod:`patchfrog.telemetry`'s module #: docstring for why the two are never conflated. reviewer_latency_ms: float = 0.0 + budget: ReviewBudgetSnapshot | None = None diff --git a/patchfrog/review/orchestration.py b/patchfrog/review/orchestration.py index 9e8f9e9..6650603 100644 --- a/patchfrog/review/orchestration.py +++ b/patchfrog/review/orchestration.py @@ -74,6 +74,7 @@ from patchfrog.review.agents.evidence import CandidateEvidencePackage from patchfrog.review.agents.proposal import AgentProposal from patchfrog.review.agents.roles import AgentRole +from patchfrog.review.budget import BudgetExceeded, ReviewBudget from patchfrog.review.critic import CriticService from patchfrog.review.critic_selection import CriticSelectionInput, CriticSelectionPolicy from patchfrog.review.domain import ( @@ -260,6 +261,7 @@ def __init__( effort_policy: ReviewEffortPolicy | None = None, reviewer_fallback_providers: Mapping[AgentRole, LLMProvider] | None = None, critic_fallback: CriticService | None = None, + review_budget: ReviewBudget | None = None, ) -> None: """``reviewer_fallback_providers``/``critic_fallback`` (Milestone U runtime-failover correction): an optional, distinct provider used @@ -292,6 +294,7 @@ def __init__( #: entirely from the ``effort_decision`` the caller passes into #: :meth:`review_candidate`, never re-derived here. self._effort_policy = effort_policy or ReviewEffortPolicy() + self._review_budget = review_budget async def review_candidate( self, @@ -386,21 +389,35 @@ async def review_candidate( usage_by_role: dict[AgentRole, TokenUsage] = {} failed_roles: list[AgentRole] = [] + budget_exhausted = False fallback_used_roles: list[AgentRole] = [] executed_provider_by_role: dict[AgentRole, str] = {} proposals: list[AgentProposal] = [] retries_consumed = 0 actual_input_total = 0 reviewer_latency_ms = 0.0 + calls_by_role: dict[AgentRole, int] = {} for role, outcome in zip(selected_roles, results, strict=True): if isinstance(outcome, BaseException): + if isinstance(outcome, BudgetExceeded): + budget_exhausted = True + failed_roles.append(role) + log.warning("review_budget_exhausted", stage="provider_call", reason=outcome.reason.value) + continue + calls_by_role[role] = 1 if isinstance(outcome, (ProviderFatalError, ProviderTransientError, ResponseSchemaError)): failed_roles.append(role) - log.warning("agent_role_call_failed", role=role.value, error=str(outcome)) + log.warning( + "agent_role_call_failed", + role=role.value, + error_type=type(outcome).__name__, + provider_failure_kind=getattr(outcome, "kind", None), + ) continue raise outcome + calls_by_role[role] = 1 validated, usage, retries_used, latency_ms, used_fallback, served_by = outcome usage_by_role[role] = usage executed_provider_by_role[role] = served_by @@ -423,11 +440,10 @@ async def review_candidate( 0, budget_state["used_input_tokens"] - combined_estimate + actual_input_total ) - calls_by_role = dict.fromkeys(selected_roles, 1) - if selected_roles and len(failed_roles) == len(selected_roles): return CandidateOrchestrationResult( - failed=True, + failed=not budget_exhausted, + skipped_budget=budget_exhausted, error=f"all selected agent roles failed: {[r.value for r in failed_roles]}", failed_roles=tuple(failed_roles), calls_by_role=calls_by_role, @@ -566,8 +582,23 @@ async def _call_role( max_output_tokens=max_output_tokens, ) + attempt_number = 0 + async def _attempt(provider: LLMProvider) -> tuple[list[ValidatedFinding], TokenUsage, float, str]: + nonlocal attempt_number + reservation = None + if self._review_budget is not None: + reservation = await self._review_budget.reserve_call( + provider.identity, + estimated_input_tokens=role_estimate, + estimated_output_tokens=max_output_tokens, + is_retry=attempt_number > 0, + ) + attempt_number += 1 result = await provider.generate_structured(request) + if reservation is not None: + assert self._review_budget is not None + await self._review_budget.reconcile(reservation, result.usage) validated = parse_and_validate_response(result.raw_json, context=validation_context) usage = TokenUsage( input_tokens=result.usage.input_tokens, @@ -747,18 +778,28 @@ async def _critique( critic_fallback_used = False critic_executed_provider: str | None = None for i, verdict_outcome in zip(reserved, verdicts, strict=True): - critic_calls += 1 if isinstance(verdict_outcome, BaseException): + if isinstance(verdict_outcome, BudgetExceeded): + result[i] = result[i].suppressed(CRITIC_BUDGET_EXHAUSTED) + logger.warning( + "review_budget_exhausted", + stage="critic_call", + reason=verdict_outcome.reason.value, + ) + continue + critic_calls += 1 if isinstance(verdict_outcome, (ProviderFatalError, ProviderTransientError, ResponseSchemaError)): # Safe fallback -- no verdict, deterministic validation # already ran. See module docstring / spec section 20. logger.warning( "agent_critic_failed", role=proposals[i].role.value, - error=str(verdict_outcome), + error_type=type(verdict_outcome).__name__, + provider_failure_kind=getattr(verdict_outcome, "kind", None), ) continue raise verdict_outcome + critic_calls += 1 verdict, usage, retries_used, used_fallback, served_by = verdict_outcome retries_consumed += retries_used actual_total += usage.input_tokens @@ -822,13 +863,21 @@ async def _critique_one( proposal, all_proposals=all_proposals, contradiction_indices=contradiction_indices ) + attempt_number = 0 + async def _attempt(service: CriticService) -> tuple[CriticVerdict, TokenUsage, str]: + nonlocal attempt_number + is_retry = attempt_number > 0 + attempt_number += 1 verdict = await service.critique( proposal.validated, candidate=candidate_evidence.candidate, context_text=candidate_evidence.context_text, conflicting_finding=conflicting, executable_verification_text=executable_verification_text, + budget=self._review_budget, + estimated_input_tokens=fallback_estimate, + is_retry=is_retry, ) usage = TokenUsage( input_tokens=verdict.input_tokens, diff --git a/patchfrog/review/provider.py b/patchfrog/review/provider.py index d3aa2b2..86bd4c4 100644 --- a/patchfrog/review/provider.py +++ b/patchfrog/review/provider.py @@ -19,6 +19,7 @@ from __future__ import annotations from dataclasses import dataclass, field +from enum import StrEnum from typing import Any, Protocol @@ -64,17 +65,73 @@ class ProviderError(Exception): """Base class for every provider failure.""" +class ProviderFailureKind(StrEnum): + INSUFFICIENT_QUOTA = "insufficient_quota" + AUTHENTICATION = "authentication_failure" + INVALID_MODEL = "invalid_model" + RATE_LIMIT = "rate_limit" + TRANSIENT_SERVER = "transient_server_error" + TIMEOUT = "timeout" + INVALID_REQUEST = "invalid_request" + REFUSAL = "refusal" + UNKNOWN = "unknown" + + class ProviderTransientError(ProviderError): """A failure that is safe to retry with bounded backoff: rate limits, server-side overload, or a dropped connection. Never raised for anything that would repeat identically on retry.""" + kind: ProviderFailureKind = ProviderFailureKind.TRANSIENT_SERVER + class ProviderFatalError(ProviderError): """A failure that must never be retried: an auth failure, a malformed request (HTTP 400), or a response that doesn't parse against the requested schema. Retrying would just repeat the same failure.""" + kind: ProviderFailureKind = ProviderFailureKind.UNKNOWN + + +class ProviderInsufficientQuotaError(ProviderFatalError): + kind = ProviderFailureKind.INSUFFICIENT_QUOTA + + +class ProviderAuthenticationError(ProviderFatalError): + kind = ProviderFailureKind.AUTHENTICATION + + +class ProviderInvalidModelError(ProviderFatalError): + kind = ProviderFailureKind.INVALID_MODEL + + +class ProviderRateLimitError(ProviderTransientError): + kind = ProviderFailureKind.RATE_LIMIT + + +class ProviderServerError(ProviderTransientError): + kind = ProviderFailureKind.TRANSIENT_SERVER + + +class ProviderTimeoutError(ProviderTransientError): + kind = ProviderFailureKind.TIMEOUT + + +def indicates_insufficient_quota(value: object) -> bool: + """Conservative cross-provider signal for permanent credit exhaustion.""" + + message = str(value).lower() + return any( + marker in message + for marker in ( + "insufficient_quota", + "credit balance", + "billing quota", + "quota exhausted", + "resource_exhausted: quota", + ) + ) + @dataclass(frozen=True, slots=True) class ProviderIdentity: diff --git a/patchfrog/review/providers/anthropic_provider.py b/patchfrog/review/providers/anthropic_provider.py index 158088a..a2e2286 100644 --- a/patchfrog/review/providers/anthropic_provider.py +++ b/patchfrog/review/providers/anthropic_provider.py @@ -34,12 +34,19 @@ import anthropic from patchfrog.review.provider import ( + ProviderAuthenticationError, ProviderFatalError, ProviderIdentity, + ProviderInsufficientQuotaError, + ProviderInvalidModelError, + ProviderRateLimitError, ProviderRequest, ProviderResult, + ProviderServerError, + ProviderTimeoutError, ProviderTransientError, ProviderUsage, + indicates_insufficient_quota, ) #: Anthropic's own SDK already retries connection errors/408/409/429/5xx @@ -90,15 +97,21 @@ async def generate_structured(self, request: ProviderRequest) -> ProviderResult: }, ) except anthropic.RateLimitError as exc: - raise ProviderTransientError(f"rate limited: {exc}") from exc + if indicates_insufficient_quota(exc): + raise ProviderInsufficientQuotaError(f"quota exhausted: {exc}") from exc + raise ProviderRateLimitError(f"rate limited: {exc}") from exc + except anthropic.APITimeoutError as exc: + raise ProviderTimeoutError(f"timeout: {exc}") from exc except anthropic.APIConnectionError as exc: raise ProviderTransientError(f"connection error: {exc}") from exc except anthropic.APIStatusError as exc: if exc.status_code in (502, 503, 504) or exc.status_code >= 500: - raise ProviderTransientError(f"server error {exc.status_code}: {exc}") from exc + raise ProviderServerError(f"server error {exc.status_code}: {exc}") from exc + if exc.status_code in (401, 403): + raise ProviderAuthenticationError(f"authentication error {exc.status_code}: {exc}") from exc + if exc.status_code == 404: + raise ProviderInvalidModelError(f"model not found: {exc}") from exc raise ProviderFatalError(f"API error {exc.status_code}: {exc}") from exc - except anthropic.APITimeoutError as exc: - raise ProviderTransientError(f"timeout: {exc}") from exc latency_ms = (time.monotonic() - start) * 1000 diff --git a/patchfrog/review/providers/gemini_provider.py b/patchfrog/review/providers/gemini_provider.py index 4cd4546..c39747d 100644 --- a/patchfrog/review/providers/gemini_provider.py +++ b/patchfrog/review/providers/gemini_provider.py @@ -60,12 +60,19 @@ from google.genai import types as genai_types from patchfrog.review.provider import ( + ProviderAuthenticationError, ProviderFatalError, ProviderIdentity, + ProviderInsufficientQuotaError, + ProviderInvalidModelError, + ProviderRateLimitError, ProviderRequest, ProviderResult, + ProviderServerError, + ProviderTimeoutError, ProviderTransientError, ProviderUsage, + indicates_insufficient_quota, ) _DEFAULT_TIMEOUT_SECONDS = 30.0 @@ -229,7 +236,7 @@ async def generate_structured(self, request: ProviderRequest) -> ProviderResult: ) except genai_errors.ClientError as exc: if exc.code in (401, 403): - raise ProviderFatalError(f"authentication error {exc.code}: {exc}") from exc + raise ProviderAuthenticationError(f"authentication error {exc.code}: {exc}") from exc if exc.code == 429: # Gemini uses 429/RESOURCE_EXHAUSTED for both ordinary # per-minute rate limiting and daily quota exhaustion -- @@ -238,10 +245,16 @@ async def generate_structured(self, request: ProviderRequest) -> ProviderResult: # Anthropic's own RateLimitError handling; a *persistent* # 429 across retries is a session/operator-level signal # to stop, not something this adapter can detect alone. - raise ProviderTransientError(f"rate limited or quota exhausted: {exc}") from exc + if indicates_insufficient_quota(exc): + raise ProviderInsufficientQuotaError(f"quota exhausted: {exc}") from exc + raise ProviderRateLimitError(f"rate limited: {exc}") from exc + if exc.code == 404: + raise ProviderInvalidModelError(f"model not found: {exc}") from exc raise ProviderFatalError(f"invalid request {exc.code}: {exc}") from exc except genai_errors.ServerError as exc: - raise ProviderTransientError(f"server error {exc.code}: {exc}") from exc + raise ProviderServerError(f"server error {exc.code}: {exc}") from exc + except httpx.TimeoutException as exc: + raise ProviderTimeoutError(f"timeout: {exc}") from exc except httpx.RequestError as exc: # Connection failure, DNS error, or a timeout at the # transport layer (the SDK is httpx-based) -- raised before diff --git a/patchfrog/review/providers/openai_provider.py b/patchfrog/review/providers/openai_provider.py index 0680894..c10e5d2 100644 --- a/patchfrog/review/providers/openai_provider.py +++ b/patchfrog/review/providers/openai_provider.py @@ -49,12 +49,19 @@ import openai from patchfrog.review.provider import ( + ProviderAuthenticationError, ProviderFatalError, ProviderIdentity, + ProviderInsufficientQuotaError, + ProviderInvalidModelError, + ProviderRateLimitError, ProviderRequest, ProviderResult, + ProviderServerError, + ProviderTimeoutError, ProviderTransientError, ProviderUsage, + indicates_insufficient_quota, ) _DEFAULT_TIMEOUT_SECONDS = 30.0 @@ -139,16 +146,20 @@ async def generate_structured(self, request: ProviderRequest) -> ProviderResult: }, ) except openai.RateLimitError as exc: - raise ProviderTransientError(f"rate limited: {exc}") from exc + if indicates_insufficient_quota(exc): + raise ProviderInsufficientQuotaError(f"quota exhausted: {exc}") from exc + raise ProviderRateLimitError(f"rate limited: {exc}") from exc + except openai.APITimeoutError as exc: + raise ProviderTimeoutError(f"timeout: {exc}") from exc except openai.APIConnectionError as exc: - # Covers openai.APITimeoutError too -- APITimeoutError is a - # subclass of APIConnectionError in this SDK, so catching the - # parent alone is sufficient and avoids an unreachable - # duplicate except clause. - raise ProviderTransientError(f"connection/timeout error: {exc}") from exc + raise ProviderTransientError(f"connection error: {exc}") from exc except openai.InternalServerError as exc: - raise ProviderTransientError(f"server error: {exc}") from exc + raise ProviderServerError(f"server error: {exc}") from exc except openai.APIStatusError as exc: + if exc.status_code in (401, 403): + raise ProviderAuthenticationError(f"authentication error {exc.status_code}: {exc}") from exc + if exc.status_code == 404: + raise ProviderInvalidModelError(f"model not found: {exc}") from exc raise ProviderFatalError(f"API error {exc.status_code}: {exc}") from exc latency_ms = (time.monotonic() - start) * 1000 diff --git a/patchfrog/review/service.py b/patchfrog/review/service.py index 18d8a67..12a0322 100644 --- a/patchfrog/review/service.py +++ b/patchfrog/review/service.py @@ -155,6 +155,14 @@ from patchfrog.review.agents.evidence import CandidateEvidencePackage from patchfrog.review.agents.proposal import AgentProposal from patchfrog.review.agents.roles import AgentRole +from patchfrog.review.budget import ( + BudgetTerminationReason, + CostBudget, + PricingCatalog, + ProviderCostMetric, + ReviewBudget, + ReviewBudgetSnapshot, +) from patchfrog.review.candidates import ReviewCandidateGenerator, summarize_static_finding from patchfrog.review.confidence import aggregate, meets_minimum from patchfrog.review.config import MalformedReviewConfigError, ReviewConfig, ReviewModelIdentity @@ -366,6 +374,7 @@ class _CandidateOutcome: __slots__ = ( "calls_by_role", "candidate", + "completed", "context_bundle_id", "context_text", "critic_calls", @@ -386,6 +395,7 @@ class _CandidateOutcome: def __init__(self, candidate: ReviewCandidate) -> None: self.candidate = candidate + self.completed = False self.context_text = "" self.context_bundle_id: uuid.UUID | None = None self.diff_excerpt = "" @@ -426,6 +436,7 @@ def __init__( effort_decision_override: ReviewEffortDecision | None = None, verifier_dispatcher: VerifierDispatcher | None = None, verification_snapshot_root: str | None = None, + pricing_catalog: PricingCatalog | None = None, ) -> None: """``route_plan`` (Milestone U, Model Router): when given, its own ``reviewer_providers``/``critic_provider`` govern every provider @@ -485,6 +496,7 @@ def __init__( self._effort_decision_override = effort_decision_override self._verifier_dispatcher = verifier_dispatcher self._verification_snapshot_root = verification_snapshot_root + self._pricing_catalog = pricing_catalog or PricingCatalog() self._queries = query_service or RepositoryQueryService() self._candidates = candidate_generator or ReviewCandidateGenerator(query_service=self._queries) self._context_service = context_service or ContextService(session_factory=session_factory) @@ -1040,6 +1052,17 @@ def _requires_critic(c: ReviewCandidate) -> bool: budget_lock = asyncio.Lock() budget_state = {"used_input_tokens": 0} + review_budget = ReviewBudget( + CostBudget( + max_provider_calls=config.max_provider_calls, + max_retry_attempts=config.max_retry_attempts, + max_input_tokens=config.max_total_input_tokens, + max_output_tokens=config.max_total_output_tokens, + max_estimated_cost_usd=config.max_estimated_cost_usd, + max_elapsed_seconds=config.max_elapsed_seconds, + ), + pricing=self._pricing_catalog, + ) semaphore = asyncio.Semaphore(max(1, config.max_concurrent_requests)) verification_budget = VerificationBudget() @@ -1083,6 +1106,7 @@ def _requires_critic(c: ReviewCandidate) -> bool: max_retries=config.max_retries, reviewer_fallback_providers=self._reviewer_fallback_providers, critic_fallback=self._critic_fallback, + review_budget=review_budget, ) async def _process(outcome: _CandidateOutcome) -> None: @@ -1116,13 +1140,29 @@ async def _process(outcome: _CandidateOutcome) -> None: verification_budget=verification_budget, staged_artifact=staged_artifact, ) + outcome.completed = True try: - await asyncio.gather(*(_process(o) for o in outcomes)) + work = asyncio.gather(*(_process(o) for o in outcomes)) + try: + if config.max_elapsed_seconds is None: + await work + else: + async with asyncio.timeout(config.max_elapsed_seconds): + await work + except TimeoutError: + await review_budget.terminate(BudgetTerminationReason.ELAPSED_TIME) + for outcome in outcomes: + if not outcome.completed: + outcome.skipped_budget = True + outcome.error = "review elapsed-time budget exhausted" + log.warning("review_budget_exhausted", stage="review", reason="max_elapsed_time") finally: if staged_artifact is not None: shutil.rmtree(staged_artifact.path, ignore_errors=True) + budget_snapshot = await review_budget.snapshot() + all_final: list[FinalAIFinding] = [f for o in outcomes for f in o.final] dedup_result = deduplicate(tuple(all_final)) kept_object_ids = {id(f) for f in dedup_result.kept} @@ -1358,7 +1398,7 @@ async def _process(outcome: _CandidateOutcome) -> None: if candidates_reviewed == 0 and candidates_failed > 0: run_status = ReviewRunStatus.FAILED - elif candidates_failed > 0: + elif candidates_failed > 0 or budget_snapshot.termination_reason is not None: run_status = ReviewRunStatus.PARTIAL else: run_status = ReviewRunStatus.SUCCEEDED @@ -1408,6 +1448,7 @@ async def _process(outcome: _CandidateOutcome) -> None: executable_verification=summarize_executable_verification( executable_verification_reports, version=EXECUTABLE_VERIFICATION_VERSION ), + budget=budget_snapshot, ) await session.commit() @@ -1420,6 +1461,14 @@ async def _process(outcome: _CandidateOutcome) -> None: rejected_count=rejected_count, suppressed_duplicate_count=suppressed_duplicate_count, duration_ms=duration_ms, + provider_calls=budget_snapshot.provider_calls, + retry_attempts=budget_snapshot.retry_attempts, + estimated_input_tokens=budget_snapshot.input_tokens, + estimated_output_tokens=budget_snapshot.output_tokens, + estimated_cost_usd=budget_snapshot.estimated_cost_usd, + budget_termination_reason=( + budget_snapshot.termination_reason.value if budget_snapshot.termination_reason else None + ), ) return ReviewRunSummary( @@ -1444,6 +1493,7 @@ async def _process(outcome: _CandidateOutcome) -> None: critic_calls=critic_calls_total, retries_consumed=retries_total, reviewer_latency_ms=reviewer_latency_ms_total, + budget=budget_snapshot, ) async def _review_candidate( @@ -1839,6 +1889,40 @@ def _summary_from_model(run: ReviewRunModel, *, reused: bool) -> ReviewRunSummar role_call_counts = {AgentRole(k): v for k, v in json.loads(run.calls_by_role).items()} except (json.JSONDecodeError, ValueError): role_call_counts = {} + try: + raw_costs = json.loads(run.provider_cost_breakdown) + by_model = tuple( + ProviderCostMetric( + provider=str(item["provider"]), + model=str(item["model"]), + call_count=int(item["call_count"]), + retry_count=int(item["retry_count"]), + input_tokens=int(item["input_tokens"]), + output_tokens=int(item["output_tokens"]), + estimated_cost_usd=float(item["estimated_cost_usd"]), + ) + for item in raw_costs + ) + except (json.JSONDecodeError, KeyError, TypeError, ValueError): + by_model = () + try: + termination_reason = ( + BudgetTerminationReason(run.budget_termination_reason) + if run.budget_termination_reason is not None + else None + ) + except ValueError: + termination_reason = None + budget = ReviewBudgetSnapshot( + provider_calls=run.provider_calls, + retry_attempts=run.retry_attempts, + input_tokens=run.budget_input_tokens, + output_tokens=run.budget_output_tokens, + estimated_cost_usd=run.estimated_cost_usd, + elapsed_seconds=run.budget_elapsed_seconds, + termination_reason=termination_reason, + by_model=by_model, + ) return ReviewRunSummary( run_id=run.id, @@ -1881,4 +1965,5 @@ def _summary_from_model(run: ReviewRunModel, *, reused: bool) -> ReviewRunSummar critic_calls=run.critic_calls, retries_consumed=run.retries_consumed, reviewer_latency_ms=run.reviewer_latency_ms, + budget=budget, ) diff --git a/patchfrog/routing/domain.py b/patchfrog/routing/domain.py index 5a09aa0..b8e1be1 100644 --- a/patchfrog/routing/domain.py +++ b/patchfrog/routing/domain.py @@ -67,6 +67,10 @@ class RouteReason(StrEnum): #: ``allowed_providers`` -- having a credential is never itself #: permission to use a provider. PROVIDER_EXCLUDED_BY_POLICY = "provider_excluded_by_policy" + #: A small, bounded diff used the operator's explicitly configured + #: low-cost provider/model route. Repository content cannot nominate + #: the provider; it only supplies the deterministic size signal. + CHEAP_ROUTE_USED = "cheap_route_used" @dataclass(frozen=True, slots=True) diff --git a/patchfrog/routing/router.py b/patchfrog/routing/router.py index 5d26349..f78251b 100644 --- a/patchfrog/routing/router.py +++ b/patchfrog/routing/router.py @@ -69,6 +69,7 @@ from collections.abc import Mapping from patchfrog.config.settings import Settings +from patchfrog.diff.models import DiffFile from patchfrog.review.agents.roles import AgentRole from patchfrog.review.provider import LLMProvider from patchfrog.review.provider_factory import MissingProviderCredentialsError, has_credentials @@ -79,6 +80,7 @@ DEFAULT_MODEL_BY_PROVIDER, SUPPORTED_PROVIDERS, ReviewRuntimeConfig, + model_matches_provider_family, ) from patchfrog.routing.capabilities import supports_structured_output from patchfrog.routing.domain import ReviewRoutePlan, RouteReason @@ -135,7 +137,13 @@ def __init__(self, *, settings: Settings, allowed_providers: frozenset[str] | No self._settings = settings self._allowed_providers = allowed_providers - def route(self, *, runtime_config: ReviewRuntimeConfig, critic_enabled: bool) -> ReviewRoutePlan: + def route( + self, + *, + runtime_config: ReviewRuntimeConfig, + critic_enabled: bool, + prefer_low_cost: bool = False, + ) -> ReviewRoutePlan: reasons: list[RouteReason] = [] configured = [ @@ -162,9 +170,15 @@ def route(self, *, runtime_config: ReviewRuntimeConfig, critic_enabled: bool) -> "without calling a provider." ) - reviewer_family, config_fallback_used = self._select_reviewer_family( - configured, preferred=runtime_config.provider, reasons=reasons - ) + cheap_family = self._settings.router_cheap_provider + if prefer_low_cost and cheap_family is not None and cheap_family in configured: + reviewer_family = cheap_family + config_fallback_used = False + reasons.append(RouteReason.CHEAP_ROUTE_USED) + else: + reviewer_family, config_fallback_used = self._select_reviewer_family( + configured, preferred=runtime_config.provider, reasons=reasons + ) diversity_available = len(configured) > 1 critic_family, diversity_used = self._select_critic_family( configured, reviewer_family, critic_enabled=critic_enabled, reasons=reasons @@ -193,9 +207,17 @@ def _model_for(family: str, *, explicit_model: str) -> str: return explicit_model return DEFAULT_MODEL_BY_PROVIDER[family] + reviewer_model = _model_for(reviewer_family, explicit_model=runtime_config.model) + if prefer_low_cost and reviewer_family == cheap_family and self._settings.router_cheap_model is not None: + reviewer_model = self._settings.router_cheap_model + if not model_matches_provider_family(reviewer_family, reviewer_model): + raise ValueError( + f"PATCHFROG_ROUTER_CHEAP_MODEL={reviewer_model!r} does not look like a " + f"{reviewer_family!r} model" + ) reviewer_provider = _build_provider( reviewer_family, - _model_for(reviewer_family, explicit_model=runtime_config.model), + reviewer_model, settings=self._settings, timeout_seconds=timeout_seconds, ) @@ -313,4 +335,11 @@ def _select_critic_family( return reviewer_family, False -__all__ = ["ModelRouter", "NoProviderConfiguredError"] +def is_small_review(diff_files: list[DiffFile]) -> bool: + """Deterministic run-level cheap-route signal; no source text is inspected.""" + + changed_lines = sum(len(file.added_lines) + len(file.deleted_lines) for file in diff_files) + return len(diff_files) <= 3 and changed_lines <= 80 + + +__all__ = ["ModelRouter", "NoProviderConfiguredError", "is_small_review"] diff --git a/tests/integration/test_review_idempotency.py b/tests/integration/test_review_idempotency.py index 32f9221..8069516 100644 --- a/tests/integration/test_review_idempotency.py +++ b/tests/integration/test_review_idempotency.py @@ -84,6 +84,11 @@ async def test_repeated_identical_review_reuses_the_run_and_never_calls_the_prov ) assert second.reused_existing_run is True assert len(provider.calls) == calls_after_first # no double-charge on retry + assert first.budget is not None + assert second.budget is not None + assert first.budget.provider_calls == calls_after_first + assert second.budget.provider_calls == calls_after_first + assert second.budget.by_model == first.budget.by_model async with session_factory() as session: runs = ( diff --git a/tests/integration/test_review_orchestration_end_to_end.py b/tests/integration/test_review_orchestration_end_to_end.py index 5ea8dc4..49d1f70 100644 --- a/tests/integration/test_review_orchestration_end_to_end.py +++ b/tests/integration/test_review_orchestration_end_to_end.py @@ -23,6 +23,7 @@ from patchfrog.persistence.models.review import ReviewRunModel from patchfrog.persistence.repositories import AIFindingProposalRepository, RepositoryRepository from patchfrog.review.agents.roles import AgentRole +from patchfrog.review.budget import BudgetTerminationReason, ModelPricing, PricingCatalog from patchfrog.review.config import ReviewConfig from patchfrog.review.domain import ProposalStatus, ReviewRunStatus from patchfrog.review.provider import ProviderFatalError, ProviderTransientError @@ -485,6 +486,39 @@ async def test_global_token_budget_cannot_be_exceeded_by_two_agents( assert len(reviewer.calls) == 0 +async def test_monetary_ceiling_stops_gracefully_before_provider_work( + session_factory: async_sessionmaker[AsyncSession], +) -> None: + repository_id, commit_sha, root_path = await _setup( + session_factory, full_name="test/orch-monetary-budget" + ) + diff_files = [_diff_marking_lines("src/billing.py", [14])] + reviewer = FakeLLMProvider(response_factory=lambda req: _NO_FINDINGS) + service = PullRequestReviewService( + session_factory=session_factory, + reviewer_provider=reviewer, + pricing_catalog=PricingCatalog( + {"fake/fake-model-1": ModelPricing(1_000_000.0, 1_000_000.0)} + ), + ) + + summary = await service.review_local( + repository_id=repository_id, + root_path=root_path, + repository_full_name="test/orch-monetary-budget", + commit_sha=commit_sha, + diff_files=diff_files, + config=ReviewConfig(max_estimated_cost_usd=0.01), + ) + + assert summary.status is ReviewRunStatus.PARTIAL + assert summary.candidates_skipped_budget == 1 + assert reviewer.calls == [] + assert summary.budget is not None + assert summary.budget.termination_reason is BudgetTerminationReason.ESTIMATED_COST + assert summary.budget.provider_calls == 0 + + async def test_contradictory_proposals_are_suppressed_when_critic_cannot_resolve( session_factory: async_sessionmaker[AsyncSession], ) -> None: diff --git a/tests/integration/test_review_pull_request_model_router_fallback.py b/tests/integration/test_review_pull_request_model_router_fallback.py index ce42a41..705e314 100644 --- a/tests/integration/test_review_pull_request_model_router_fallback.py +++ b/tests/integration/test_review_pull_request_model_router_fallback.py @@ -122,13 +122,18 @@ async def _run_until_routing( _real_route = ModelRouter.route # save before patching -- avoids self-recursion def _capture_and_stop_after_real_routing( - self: ModelRouter, *, runtime_config: Any, critic_enabled: bool + self: ModelRouter, *, runtime_config: Any, critic_enabled: bool, prefer_low_cost: bool = False ) -> Any: # Calls the *real* ModelRouter.route (not a stand-in) so this # exercises actual provider-selection/fallback/policy logic -- # only the task's continuation past routing is short-circuited, # so no repository index fixture is needed to reach this point. - plan = _real_route(self, runtime_config=runtime_config, critic_enabled=critic_enabled) + plan = _real_route( + self, + runtime_config=runtime_config, + critic_enabled=critic_enabled, + prefer_low_cost=prefer_low_cost, + ) captured["route_plan"] = plan raise _StoppedAfterRouting() diff --git a/tests/integration/test_review_pull_request_provider_trust_boundary.py b/tests/integration/test_review_pull_request_provider_trust_boundary.py index 47f9ec4..95e98ce 100644 --- a/tests/integration/test_review_pull_request_provider_trust_boundary.py +++ b/tests/integration/test_review_pull_request_provider_trust_boundary.py @@ -202,8 +202,11 @@ class _StopAfterProviderResolution(Exception): captured: dict[str, Any] = {} - def _capture_route(self: Any, *, runtime_config: Any, critic_enabled: bool) -> Any: + def _capture_route( + self: Any, *, runtime_config: Any, critic_enabled: bool, prefer_low_cost: bool = False + ) -> Any: captured["runtime_config"] = runtime_config + captured["prefer_low_cost"] = prefer_low_cost raise _StopAfterProviderResolution() monkeypatch.setattr(ModelRouter, "route", _capture_route) diff --git a/tests/unit/test_change_intelligence_versioning.py b/tests/unit/test_change_intelligence_versioning.py index 7072297..f622abf 100644 --- a/tests/unit/test_change_intelligence_versioning.py +++ b/tests/unit/test_change_intelligence_versioning.py @@ -19,7 +19,7 @@ _PRE_CI_PROMPT_VERSION = 3 _PRE_CI_POLICY_VERSION = 4 _PRE_CI_ENGINE_VERSION = 3 -_PRE_CI_CONFIG_SCHEMA_VERSION = 4 +_PRE_CI_CONFIG_SCHEMA_VERSION = 5 def test_review_prompt_version_bumped_for_change_intelligence_section() -> None: diff --git a/tests/unit/test_contract_intelligence_versioning.py b/tests/unit/test_contract_intelligence_versioning.py index 6a21617..a632231 100644 --- a/tests/unit/test_contract_intelligence_versioning.py +++ b/tests/unit/test_contract_intelligence_versioning.py @@ -22,7 +22,7 @@ _PRE_CK_PROMPT_VERSION = 4 _PRE_CK_POLICY_VERSION = 4 _PRE_CK_ENGINE_VERSION = 3 -_PRE_CK_CONFIG_SCHEMA_VERSION = 4 +_PRE_CK_CONFIG_SCHEMA_VERSION = 5 _PRE_CK_TELEMETRY_SCHEMA_VERSION = 2 _PRE_CK_CHANGE_INTELLIGENCE_VERSION = 1 diff --git a/tests/unit/test_cross_pr_intelligence_versioning.py b/tests/unit/test_cross_pr_intelligence_versioning.py index 1cc1730..2ca220b 100644 --- a/tests/unit/test_cross_pr_intelligence_versioning.py +++ b/tests/unit/test_cross_pr_intelligence_versioning.py @@ -34,7 +34,7 @@ _PRE_Q_PROMPT_VERSION = 10 _PRE_Q_POLICY_VERSION = 4 _PRE_Q_ENGINE_VERSION = 3 -_PRE_Q_CONFIG_SCHEMA_VERSION = 4 +_PRE_Q_CONFIG_SCHEMA_VERSION = 5 _PRE_Q_QUALITY_COST_POLICY_VERSION = 2 _PRE_Q_TELEMETRY_SCHEMA_VERSION = 8 _PRE_Q_CHANGE_INTELLIGENCE_VERSION = 1 diff --git a/tests/unit/test_cross_repo_intelligence_versioning.py b/tests/unit/test_cross_repo_intelligence_versioning.py index 1b9c91c..169adfa 100644 --- a/tests/unit/test_cross_repo_intelligence_versioning.py +++ b/tests/unit/test_cross_repo_intelligence_versioning.py @@ -35,7 +35,7 @@ _PRE_R_PROMPT_VERSION = 11 _PRE_R_POLICY_VERSION = 4 _PRE_R_ENGINE_VERSION = 3 -_PRE_R_CONFIG_SCHEMA_VERSION = 4 +_PRE_R_CONFIG_SCHEMA_VERSION = 5 _PRE_R_QUALITY_COST_POLICY_VERSION = 3 _PRE_R_TELEMETRY_SCHEMA_VERSION = 9 _PRE_R_CHANGE_INTELLIGENCE_VERSION = 1 diff --git a/tests/unit/test_executable_verification_versioning.py b/tests/unit/test_executable_verification_versioning.py index 4484884..e272e6c 100644 --- a/tests/unit/test_executable_verification_versioning.py +++ b/tests/unit/test_executable_verification_versioning.py @@ -43,7 +43,7 @@ _PRE_S_PROMPT_VERSION = 12 _PRE_S_POLICY_VERSION = 4 _PRE_S_ENGINE_VERSION = 3 -_PRE_S_CONFIG_SCHEMA_VERSION = 4 +_PRE_S_CONFIG_SCHEMA_VERSION = 5 _PRE_S_QUALITY_COST_POLICY_VERSION = 4 _PRE_S_TELEMETRY_SCHEMA_VERSION = 10 _PRE_S_CHANGE_INTELLIGENCE_VERSION = 1 diff --git a/tests/unit/test_historical_regression_memory_versioning.py b/tests/unit/test_historical_regression_memory_versioning.py index 0f24b4c..2749f95 100644 --- a/tests/unit/test_historical_regression_memory_versioning.py +++ b/tests/unit/test_historical_regression_memory_versioning.py @@ -25,7 +25,7 @@ _PRE_HRM_PROMPT_VERSION = 7 _PRE_HRM_POLICY_VERSION = 4 _PRE_HRM_ENGINE_VERSION = 3 -_PRE_HRM_CONFIG_SCHEMA_VERSION = 4 +_PRE_HRM_CONFIG_SCHEMA_VERSION = 5 _PRE_HRM_TELEMETRY_SCHEMA_VERSION = 5 _PRE_HRM_CHANGE_INTELLIGENCE_VERSION = 1 _PRE_HRM_CONTRACT_INTELLIGENCE_VERSION = 1 diff --git a/tests/unit/test_intent_verification_versioning.py b/tests/unit/test_intent_verification_versioning.py index 5c5923e..68db62d 100644 --- a/tests/unit/test_intent_verification_versioning.py +++ b/tests/unit/test_intent_verification_versioning.py @@ -23,7 +23,7 @@ _PRE_IV_PROMPT_VERSION = 5 _PRE_IV_POLICY_VERSION = 4 _PRE_IV_ENGINE_VERSION = 3 -_PRE_IV_CONFIG_SCHEMA_VERSION = 4 +_PRE_IV_CONFIG_SCHEMA_VERSION = 5 _PRE_IV_TELEMETRY_SCHEMA_VERSION = 3 _PRE_IV_CHANGE_INTELLIGENCE_VERSION = 1 _PRE_IV_CONTRACT_INTELLIGENCE_VERSION = 1 diff --git a/tests/unit/test_model_router.py b/tests/unit/test_model_router.py index 2c182f8..442cc9e 100644 --- a/tests/unit/test_model_router.py +++ b/tests/unit/test_model_router.py @@ -14,6 +14,7 @@ import pytest from patchfrog.config.settings import Settings +from patchfrog.diff.parser import build_diff_file from patchfrog.review.agents.roles import AgentRole from patchfrog.review.provider_factory import MissingProviderCredentialsError from patchfrog.review.providers.anthropic_provider import AnthropicLLMProvider @@ -21,7 +22,7 @@ from patchfrog.review.providers.openai_provider import OpenAILLMProvider from patchfrog.review.runtime_config import DEFAULT_MODEL_BY_PROVIDER, ReviewRuntimeConfig from patchfrog.routing.domain import RouteReason -from patchfrog.routing.router import ModelRouter, NoProviderConfiguredError +from patchfrog.routing.router import ModelRouter, NoProviderConfiguredError, is_small_review _ADAPTER_CLASS = { "anthropic": AnthropicLLMProvider, @@ -57,6 +58,55 @@ def _runtime_config( ) +def test_small_review_uses_explicit_operator_cheap_route() -> None: + router = ModelRouter( + settings=_settings( + ANTHROPIC_API_KEY="fake-not-real", + GEMINI_API_KEY="fake-not-real", + PATCHFROG_ROUTER_CHEAP_PROVIDER="gemini", + PATCHFROG_ROUTER_CHEAP_MODEL="gemini-cheap-test", + ) + ) + + plan = router.route( + runtime_config=_runtime_config(provider="anthropic"), + critic_enabled=True, + prefer_low_cost=True, + ) + + assert plan.reviewer_provider_family == "gemini" + assert plan.reviewer_providers[AgentRole.CORRECTNESS].identity.model == "gemini-cheap-test" + assert RouteReason.CHEAP_ROUTE_USED in plan.reasons + + +def test_cheap_route_cannot_bypass_allowed_provider_policy() -> None: + router = ModelRouter( + settings=_settings( + ANTHROPIC_API_KEY="fake-not-real", + GEMINI_API_KEY="fake-not-real", + PATCHFROG_ROUTER_CHEAP_PROVIDER="gemini", + ), + allowed_providers=frozenset({"anthropic"}), + ) + + plan = router.route( + runtime_config=_runtime_config(provider="anthropic"), + critic_enabled=False, + prefer_low_cost=True, + ) + + assert plan.reviewer_provider_family == "anthropic" + assert RouteReason.CHEAP_ROUTE_USED not in plan.reasons + + +def test_small_review_signal_is_bounded_by_files_and_changed_lines() -> None: + tiny = build_diff_file("a.py", "@@ -1 +1 @@\n-old\n+new\n") + assert is_small_review([tiny]) is True + + large_patch = "@@ -1,81 +1,81 @@\n" + "".join(f"-old{i}\n+new{i}\n" for i in range(41)) + assert is_small_review([build_diff_file("a.py", large_patch)]) is False + + # -- Single-provider configurations (items 1-3 of the required matrix) -- @@ -348,7 +398,7 @@ def test_route_signature_takes_no_repository_content_parameter() -> None: # ReviewRuntimeConfig -- nothing shaped like diff/PR/comment content # can reach routing decisions, by construction of this signature. params = set(inspect.signature(ModelRouter.route).parameters) - assert params == {"self", "runtime_config", "critic_enabled"} + assert params == {"self", "runtime_config", "critic_enabled", "prefer_low_cost"} def test_reviewer_providers_mapping_covers_both_existing_roles() -> None: diff --git a/tests/unit/test_repository_learnings_versioning.py b/tests/unit/test_repository_learnings_versioning.py index a5289e9..3097991 100644 --- a/tests/unit/test_repository_learnings_versioning.py +++ b/tests/unit/test_repository_learnings_versioning.py @@ -26,7 +26,7 @@ _PRE_RL_PROMPT_VERSION = 8 _PRE_RL_POLICY_VERSION = 4 _PRE_RL_ENGINE_VERSION = 3 -_PRE_RL_CONFIG_SCHEMA_VERSION = 4 +_PRE_RL_CONFIG_SCHEMA_VERSION = 5 _PRE_RL_TELEMETRY_SCHEMA_VERSION = 6 _PRE_RL_CHANGE_INTELLIGENCE_VERSION = 1 _PRE_RL_CONTRACT_INTELLIGENCE_VERSION = 1 diff --git a/tests/unit/test_review_budget.py b/tests/unit/test_review_budget.py new file mode 100644 index 0000000..efdc666 --- /dev/null +++ b/tests/unit/test_review_budget.py @@ -0,0 +1,101 @@ +from __future__ import annotations + +import pytest + +from patchfrog.review.budget import ( + BudgetExceeded, + BudgetTerminationReason, + CostBudget, + ModelPricing, + PricingCatalog, + ReviewBudget, +) +from patchfrog.review.provider import ProviderIdentity, ProviderUsage + +_IDENTITY = ProviderIdentity(provider="fake", model="cheap") + + +def _limits(**overrides: object) -> CostBudget: + values: dict[str, object] = { + "max_provider_calls": 4, + "max_retry_attempts": 2, + "max_input_tokens": 10_000, + "max_output_tokens": 2_000, + "max_estimated_cost_usd": None, + "max_elapsed_seconds": 60.0, + } + values.update(overrides) + return CostBudget(**values) # type: ignore[arg-type] + + +async def test_fake_pricing_accounts_by_provider_and_model() -> None: + budget = ReviewBudget( + _limits(), + pricing=PricingCatalog( + {"fake/cheap": ModelPricing(input_usd_per_million_tokens=1.0, output_usd_per_million_tokens=2.0)} + ), + ) + reservation = await budget.reserve_call( + _IDENTITY, estimated_input_tokens=1_000, estimated_output_tokens=200, is_retry=False + ) + await budget.reconcile(reservation, ProviderUsage(input_tokens=800, output_tokens=100)) + + snapshot = await budget.snapshot() + assert snapshot.provider_calls == 1 + assert snapshot.input_tokens == 800 + assert snapshot.output_tokens == 100 + assert snapshot.estimated_cost_usd == pytest.approx(0.001) + assert snapshot.by_model[0].provider == "fake" + assert snapshot.by_model[0].model == "cheap" + + +async def test_cost_ceiling_stops_before_overspend() -> None: + budget = ReviewBudget( + _limits(max_estimated_cost_usd=0.0005), + pricing=PricingCatalog( + {"fake/cheap": ModelPricing(input_usd_per_million_tokens=1.0, output_usd_per_million_tokens=1.0)} + ), + ) + + with pytest.raises(BudgetExceeded) as raised: + await budget.reserve_call( + _IDENTITY, estimated_input_tokens=500, estimated_output_tokens=100, is_retry=False + ) + + assert raised.value.reason is BudgetTerminationReason.ESTIMATED_COST + snapshot = await budget.snapshot() + assert snapshot.provider_calls == 0 + assert snapshot.termination_reason is BudgetTerminationReason.ESTIMATED_COST + + +async def test_monetary_ceiling_requires_operator_pricing() -> None: + budget = ReviewBudget(_limits(max_estimated_cost_usd=1.0)) + + with pytest.raises(BudgetExceeded) as raised: + await budget.reserve_call( + _IDENTITY, estimated_input_tokens=1, estimated_output_tokens=1, is_retry=False + ) + + assert raised.value.reason is BudgetTerminationReason.PRICING_UNAVAILABLE + + +async def test_review_wide_retry_budget_is_bounded() -> None: + budget = ReviewBudget(_limits(max_retry_attempts=1)) + await budget.reserve_call( + _IDENTITY, estimated_input_tokens=10, estimated_output_tokens=10, is_retry=True + ) + with pytest.raises(BudgetExceeded) as raised: + await budget.reserve_call( + _IDENTITY, estimated_input_tokens=10, estimated_output_tokens=10, is_retry=True + ) + assert raised.value.reason is BudgetTerminationReason.RETRIES + + +async def test_elapsed_ceiling_is_checked_before_call() -> None: + ticks = iter((10.0, 12.1, 12.1)) + budget = ReviewBudget(_limits(max_elapsed_seconds=2.0), monotonic=lambda: next(ticks)) + with pytest.raises(BudgetExceeded) as raised: + await budget.reserve_call( + _IDENTITY, estimated_input_tokens=1, estimated_output_tokens=1, is_retry=False + ) + assert raised.value.reason is BudgetTerminationReason.ELAPSED_TIME diff --git a/tests/unit/test_review_config.py b/tests/unit/test_review_config.py index 1ec3980..c66f27d 100644 --- a/tests/unit/test_review_config.py +++ b/tests/unit/test_review_config.py @@ -228,8 +228,9 @@ def test_config_schema_version_bumped_for_operator_boundary_change() -> None: # Milestone C bumped 2 -> 3 (provider/model fields removed from # repository config); Milestone F (Quality + Cost Guard) bumped # 3 -> 4 (max_output_tokens_per_candidate's effective repo-facing - # meaning changed -- see patchfrog.review.config's module comment). - assert CONFIG_SCHEMA_VERSION == 4 + # meaning changed); beta cost controls bumped 4 -> 5 because the + # repository-visible budget contract is now first class. + assert CONFIG_SCHEMA_VERSION == 5 # -- Trust boundary: provider/model/critic_model/request_timeout_seconds diff --git a/tests/unit/test_review_openai_provider_contract.py b/tests/unit/test_review_openai_provider_contract.py index fe2f958..19f3896 100644 --- a/tests/unit/test_review_openai_provider_contract.py +++ b/tests/unit/test_review_openai_provider_contract.py @@ -23,7 +23,17 @@ import openai import pytest -from patchfrog.review.provider import ProviderFatalError, ProviderRequest, ProviderTransientError +from patchfrog.review.provider import ( + ProviderAuthenticationError, + ProviderFatalError, + ProviderInsufficientQuotaError, + ProviderInvalidModelError, + ProviderRateLimitError, + ProviderRequest, + ProviderServerError, + ProviderTimeoutError, + ProviderTransientError, +) from patchfrog.review.providers.openai_provider import OpenAILLMProvider, _sanitize_schema_name _MODEL = "gpt-6-astra" @@ -128,7 +138,18 @@ async def test_rate_limit_is_transient() -> None: provider = _provider( handler=_handler_returning(429, {"error": {"message": "rate limited", "type": "rate_limit_error"}}) ) - with pytest.raises(ProviderTransientError): + with pytest.raises(ProviderRateLimitError): + await provider.generate_structured(_REQUEST) + + +async def test_insufficient_quota_is_permanent() -> None: + provider = _provider( + handler=_handler_returning( + 429, + {"error": {"message": "You exceeded your current quota", "code": "insufficient_quota"}}, + ) + ) + with pytest.raises(ProviderInsufficientQuotaError): await provider.generate_structured(_REQUEST) @@ -136,13 +157,13 @@ async def test_server_error_is_transient() -> None: provider = _provider( handler=_handler_returning(500, {"error": {"message": "server error", "type": "server_error"}}) ) - with pytest.raises(ProviderTransientError): + with pytest.raises(ProviderServerError): await provider.generate_structured(_REQUEST) async def test_timeout_is_transient() -> None: provider = _provider(handler=_handler_raising(httpx2.TimeoutException("timed out"))) - with pytest.raises(ProviderTransientError): + with pytest.raises(ProviderTimeoutError): await provider.generate_structured(_REQUEST) @@ -164,7 +185,7 @@ async def test_auth_failure_401_is_fatal_never_retried() -> None: provider = _provider( handler=_handler_returning(401, {"error": {"message": "unauthorized", "type": "auth_error"}}) ) - with pytest.raises(ProviderFatalError): + with pytest.raises(ProviderAuthenticationError): await provider.generate_structured(_REQUEST) @@ -172,7 +193,7 @@ async def test_permission_denied_403_is_fatal_never_retried() -> None: provider = _provider( handler=_handler_returning(403, {"error": {"message": "forbidden", "type": "permission_error"}}) ) - with pytest.raises(ProviderFatalError): + with pytest.raises(ProviderAuthenticationError): await provider.generate_structured(_REQUEST) @@ -180,7 +201,7 @@ async def test_unknown_model_404_is_fatal_never_retried() -> None: provider = _provider( handler=_handler_returning(404, {"error": {"message": "model not found", "type": "invalid_request_error"}}) ) - with pytest.raises(ProviderFatalError): + with pytest.raises(ProviderInvalidModelError): await provider.generate_structured(_REQUEST) diff --git a/tests/unit/test_review_operator_hard_caps.py b/tests/unit/test_review_operator_hard_caps.py index b5c5f48..756ebb4 100644 --- a/tests/unit/test_review_operator_hard_caps.py +++ b/tests/unit/test_review_operator_hard_caps.py @@ -90,7 +90,8 @@ def test_operator_caps_are_never_read_from_repo_config_fields() -> None: assert set(ReviewConfig.model_fields) == { "critic_enabled", "max_candidates", "max_input_tokens_per_candidate", "max_output_tokens_per_candidate", "max_total_input_tokens", "max_concurrent_requests", - "min_final_confidence", "max_retries", + "min_final_confidence", "max_retries", "max_provider_calls", "max_retry_attempts", + "max_total_output_tokens", "max_estimated_cost_usd", "max_elapsed_seconds", } diff --git a/tests/unit/test_review_retry_budget.py b/tests/unit/test_review_retry_budget.py new file mode 100644 index 0000000..24de5e2 --- /dev/null +++ b/tests/unit/test_review_retry_budget.py @@ -0,0 +1,35 @@ +from __future__ import annotations + +import pytest + +from patchfrog.review.provider import ( + ProviderInsufficientQuotaError, + ProviderServerError, +) +from patchfrog.review.retry import call_with_retry + + +async def test_insufficient_quota_has_zero_retries() -> None: + calls = 0 + + async def attempt() -> None: + nonlocal calls + calls += 1 + raise ProviderInsufficientQuotaError("credit balance exhausted") + + with pytest.raises(ProviderInsufficientQuotaError): + await call_with_retry(attempt, max_retries=5, base_delay=0) + assert calls == 1 + + +async def test_transient_server_error_uses_bounded_retry() -> None: + calls = 0 + + async def attempt() -> str: + nonlocal calls + calls += 1 + raise ProviderServerError("temporary") + + with pytest.raises(ProviderServerError): + await call_with_retry(attempt, max_retries=2, base_delay=0) + assert calls == 3 diff --git a/tests/unit/test_settings.py b/tests/unit/test_settings.py index 90dac2c..0bd2a64 100644 --- a/tests/unit/test_settings.py +++ b/tests/unit/test_settings.py @@ -109,3 +109,12 @@ def test_allowed_providers_blank_string_is_no_restriction(monkeypatch: pytest.Mo monkeypatch.setenv("PATCHFROG_ALLOWED_PROVIDERS", " , ") settings = Settings(_env_file=None) assert settings.allowed_providers is None + + +def test_provider_pricing_parses_json_without_secrets(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv( + "PATCHFROG_PROVIDER_PRICING", + '{"fake/cheap":{"input_usd_per_million_tokens":1.25,"output_usd_per_million_tokens":4.5}}', + ) + settings = Settings(_env_file=None) + assert settings.provider_pricing["fake/cheap"]["input_usd_per_million_tokens"] == 1.25 diff --git a/tests/unit/test_test_intelligence_versioning.py b/tests/unit/test_test_intelligence_versioning.py index b073b22..6a16a91 100644 --- a/tests/unit/test_test_intelligence_versioning.py +++ b/tests/unit/test_test_intelligence_versioning.py @@ -24,7 +24,7 @@ _PRE_TI_PROMPT_VERSION = 6 _PRE_TI_POLICY_VERSION = 4 _PRE_TI_ENGINE_VERSION = 3 -_PRE_TI_CONFIG_SCHEMA_VERSION = 4 +_PRE_TI_CONFIG_SCHEMA_VERSION = 5 _PRE_TI_TELEMETRY_SCHEMA_VERSION = 4 _PRE_TI_CHANGE_INTELLIGENCE_VERSION = 1 _PRE_TI_CONTRACT_INTELLIGENCE_VERSION = 1 diff --git a/tests/unit/test_trajectory_intelligence_versioning.py b/tests/unit/test_trajectory_intelligence_versioning.py index f3d3ea6..ad7a6ad 100644 --- a/tests/unit/test_trajectory_intelligence_versioning.py +++ b/tests/unit/test_trajectory_intelligence_versioning.py @@ -33,7 +33,7 @@ _PRE_P_PROMPT_VERSION = 9 _PRE_P_POLICY_VERSION = 4 _PRE_P_ENGINE_VERSION = 3 -_PRE_P_CONFIG_SCHEMA_VERSION = 4 +_PRE_P_CONFIG_SCHEMA_VERSION = 5 _PRE_P_QUALITY_COST_POLICY_VERSION = 1 _PRE_P_TELEMETRY_SCHEMA_VERSION = 7 _PRE_P_CHANGE_INTELLIGENCE_VERSION = 1 From 8a041eb509afcf9d82e721e9a89f166b3d2c1839 Mon Sep 17 00:00:00 2001 From: Kadir Eren Altintas Date: Mon, 21 Sep 2026 21:28:58 +0300 Subject: [PATCH 3/7] add deterministic reviewer readiness evaluation --- .env.example | 1 + docs/agent-orchestration.md | 17 +- docs/deployment.md | 1 + docs/evaluation.md | 10 + patchfrog/cli.py | 55 ++++- patchfrog/config/settings.py | 5 + patchfrog/evaluation/beta_readiness.py | 209 ++++++++++++++++++ patchfrog/evaluation/domain.py | 10 +- patchfrog/evaluation/runner.py | 14 +- patchfrog/review/agents/cross_role.py | 7 + patchfrog/review/config.py | 3 + patchfrog/review/config_resolution.py | 1 + patchfrog/review/critic_policy.py | 20 ++ patchfrog/review/domain.py | 4 + patchfrog/review/orchestration.py | 6 + patchfrog/review/service.py | 27 ++- patchfrog/telemetry/domain.py | 2 + tests/fixtures/evaluation/beta_readiness.yaml | 22 ++ .../beta-api-return-shape-break/case.yaml | 18 ++ .../repo/src/users.py | 7 + .../beta-behavior-preserving-rename/case.yaml | 10 + .../repo/src/pricing.py | 3 + .../cases/beta-comment-only-change/case.yaml | 10 + .../repo/src/limits.py | 3 + .../beta-generated-fixture-context/case.yaml | 10 + .../repo/fixtures/generated_values.py | 2 + .../cases/beta-safe-refactor/case.yaml | 10 + .../beta-safe-refactor/repo/src/totals.py | 6 + .../cases/beta-schema-mismatch/case.yaml | 18 ++ .../beta-schema-mismatch/repo/src/events.py | 9 + .../cases/beta-test-only-change/case.yaml | 10 + .../repo/tests/test_slug.py | 3 + .../test_beta_readiness_evaluation.py | 81 +++++++ .../test_review_failure_recovery.py | 40 +++- tests/unit/test_beta_readiness_evaluation.py | 144 ++++++++++++ .../test_evaluation_telemetry_identity.py | 4 +- tests/unit/test_review_agents_cross_role.py | 15 ++ tests/unit/test_review_operator_hard_caps.py | 2 +- tests/unit/test_telemetry_domain.py | 8 + 39 files changed, 812 insertions(+), 15 deletions(-) create mode 100644 patchfrog/evaluation/beta_readiness.py create mode 100644 patchfrog/review/critic_policy.py create mode 100644 tests/fixtures/evaluation/beta_readiness.yaml create mode 100644 tests/fixtures/evaluation/cases/beta-api-return-shape-break/case.yaml create mode 100644 tests/fixtures/evaluation/cases/beta-api-return-shape-break/repo/src/users.py create mode 100644 tests/fixtures/evaluation/cases/beta-behavior-preserving-rename/case.yaml create mode 100644 tests/fixtures/evaluation/cases/beta-behavior-preserving-rename/repo/src/pricing.py create mode 100644 tests/fixtures/evaluation/cases/beta-comment-only-change/case.yaml create mode 100644 tests/fixtures/evaluation/cases/beta-comment-only-change/repo/src/limits.py create mode 100644 tests/fixtures/evaluation/cases/beta-generated-fixture-context/case.yaml create mode 100644 tests/fixtures/evaluation/cases/beta-generated-fixture-context/repo/fixtures/generated_values.py create mode 100644 tests/fixtures/evaluation/cases/beta-safe-refactor/case.yaml create mode 100644 tests/fixtures/evaluation/cases/beta-safe-refactor/repo/src/totals.py create mode 100644 tests/fixtures/evaluation/cases/beta-schema-mismatch/case.yaml create mode 100644 tests/fixtures/evaluation/cases/beta-schema-mismatch/repo/src/events.py create mode 100644 tests/fixtures/evaluation/cases/beta-test-only-change/case.yaml create mode 100644 tests/fixtures/evaluation/cases/beta-test-only-change/repo/tests/test_slug.py create mode 100644 tests/integration/test_beta_readiness_evaluation.py create mode 100644 tests/unit/test_beta_readiness_evaluation.py diff --git a/.env.example b/.env.example index 83b58cf..7b961c7 100644 --- a/.env.example +++ b/.env.example @@ -43,6 +43,7 @@ GITHUB_API_TIMEOUT_SECONDS=10 # PATCHFROG_MAX_ESTIMATED_COST_USD= # optional; requires pricing below # PATCHFROG_MAX_REVIEW_ELAPSED_SECONDS= # optional hard provider-work ceiling # PATCHFROG_PROVIDER_PRICING='{"provider/model":{"input_usd_per_million_tokens":1.0,"output_usd_per_million_tokens":4.0}}' +# PATCHFROG_CRITIC_FAILURE_POLICY=hold_for_review # optional; default fail_open # # Only the credential for the provider actually selected above needs to # be set. Never set either of these in .patchfrog.yml or any diff --git a/docs/agent-orchestration.md b/docs/agent-orchestration.md index eb790a3..89952f9 100644 --- a/docs/agent-orchestration.md +++ b/docs/agent-orchestration.md @@ -223,10 +223,19 @@ A single specialist role failing (a transient or fatal provider error) never fails the whole candidate if the other role's result is usable -- the run proceeds with whatever validated, useful work exists. Only when **every** selected role fails for a candidate is that candidate marked -failed. A critic failure falls back to no-critic aggregation, exactly as -before -- except for a proposal inside an unresolved contradiction -group, where a missing verdict is treated the same as "not confidently -resolved" and the group is suppressed rather than defaulting to accept. +failed. Critic failure behavior is explicit through +`CriticFailurePolicy`: `fail_open` (the compatibility default) falls back +to deterministic validation and reviewer-confidence aggregation; +`hold_for_review` suppresses the affected proposal until critic +verification succeeds. Fail-open is suitable when findings are advisory, +the proposal has already passed deterministic evidence checks, and losing +recall is costlier than delaying independent verification. Hold-for-review +is safer for mandatory checks, security-sensitive repositories, or any +workflow where an unverified finding must not publish. An operator can +force the policy with `PATCHFROG_CRITIC_FAILURE_POLICY`; a repository may +only choose the policy when no operator override is present. Unexpected +programming errors still fail loudly under both policies. Unresolved +contradiction groups and critic-budget exhaustion remain fail-closed. ## Context Engine depth (superseded) diff --git a/docs/deployment.md b/docs/deployment.md index 7550af8..b35fce3 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -224,6 +224,7 @@ credentials -- **never** `.patchfrog.yml`-controlled: | `PATCHFROG_MAX_ESTIMATED_COST_USD` | Optional estimated-dollar ceiling; requires matching pricing entries | unset | | `PATCHFROG_MAX_REVIEW_ELAPSED_SECONDS` | Optional hard wall-clock ceiling for provider work | unset | | `PATCHFROG_PROVIDER_PRICING` | JSON object keyed by `provider/model`, with input/output USD per million tokens | `{}` | +| `PATCHFROG_CRITIC_FAILURE_POLICY` | Optional operator override: `fail_open` or `hold_for_review` | unset (`fail_open` repository default) | `patchfrog.review.config_resolution.apply_operator_hard_caps` computes `effective = min(repo_intent, operator_hard_cap)` per field, applied by diff --git a/docs/evaluation.md b/docs/evaluation.md index eeb1a38..8e1f576 100644 --- a/docs/evaluation.md +++ b/docs/evaluation.md @@ -88,6 +88,12 @@ run can measure, and every report labels which one it is was used. Never read a `pipeline_correctness` precision/recall number as "how good is the AI reviewer" — it isn't measuring that. +The beta-readiness profile additionally reports candidate recall, +accepted-finding recall, false-positive and false-negative rates, critic +rejection and critic false-negative rates, and repeated-run variance. +The default oracle remains a plumbing/guardrail benchmark, not a claim +about a live model's reasoning quality. + If `ANTHROPIC_API_KEY` is not set in the environment, `--provider live` fails fast with a clear `MissingProviderCredentialsError` message. This is expected in most dev/CI environments; the fake-provider path is not @@ -104,6 +110,10 @@ python -m patchfrog.cli eval run python -m patchfrog.cli eval run --tag security --language python --difficulty hard python -m patchfrog.cli eval run --case py-inverted-boundary --case c-memory-leak +# Explicit 20-case beta profile, repeated deterministically with fake +# providers. This consumes no provider credits: +python -m patchfrog.cli eval run --beta-readiness --repeat 2 + # Static analyzers only, no LLM at all: python -m patchfrog.cli eval run --mode static_only diff --git a/patchfrog/cli.py b/patchfrog/cli.py index 4236b6f..d729c2b 100644 --- a/patchfrog/cli.py +++ b/patchfrog/cli.py @@ -50,6 +50,10 @@ RepositoryRelationKind, RepositoryRelationProvenance, ) +from patchfrog.evaluation.beta_readiness import ( + compute_beta_readiness_metrics, + load_beta_profile, +) from patchfrog.evaluation.domain import ( CaseStatus, EvaluationCase, @@ -1734,7 +1738,18 @@ async def _eval_run_async(args: argparse.Namespace) -> dict[str, Any]: all_cases = load_all_cases(DEFAULT_CASES_ROOT) validate_and_raise(all_cases, cases_root=DEFAULT_CASES_ROOT) - cases = _filter_cases(all_cases, args) + beta_expectations = load_beta_profile() if args.beta_readiness else () + if args.repeat < 1: + raise ValueError("--repeat must be at least 1") + if not beta_expectations and args.repeat != 1: + raise ValueError("--repeat is only supported with --beta-readiness") + if beta_expectations: + if args.case or args.tag or args.language or args.difficulty: + raise ValueError("--beta-readiness cannot be combined with case/tag/language/difficulty filters") + beta_ids = {item.case_id for item in beta_expectations} + cases = [case for case in all_cases if case.id in beta_ids] + else: + cases = _filter_cases(all_cases, args) if not cases: raise ValueError("no benchmark cases matched the given --case/--tag/--language/--difficulty filters") @@ -1866,6 +1881,33 @@ async def _eval_run_async(args: argparse.Namespace) -> dict[str, Any]: report = build_report(result, cases_by_id=cases_by_id, fixture_info=fixture_info) report["benchmark_label"] = "pipeline_correctness" if args.provider == "fake" else "ai_quality" report["ai_quality_measured"] = args.provider == "live" + if beta_expectations: + repeated_results = [tuple(case_results)] + for _ in range(1, args.repeat): + repeated_results.append( + tuple( + await runner.run_suite( + cases, + cases_root=DEFAULT_CASES_ROOT, + mode=mode, + reviewer_provider_factory=provider_factory, + critic_provider_factory=provider_factory, + critic_enabled=critic_enabled_flag, + context_config_override=context_override, + timeout_seconds=args.timeout, + ) + ) + ) + report["beta_readiness"] = { + "profile_cases": [asdict(item) for item in beta_expectations], + "metrics": asdict( + compute_beta_readiness_metrics( + repeated_results, expectations=beta_expectations + ) + ), + "repeat_count": args.repeat, + "provider_mode": args.provider, + } if critic_comparison_report is not None: report["critic_comparison"] = critic_comparison_report if context_ablation_report is not None: @@ -2408,6 +2450,17 @@ def main(argv: list[str] | None = None) -> int: "benchmark, never AI quality). 'live' calls the real configured provider and requires " "ANTHROPIC_API_KEY.", ) + eval_run_parser.add_argument( + "--beta-readiness", + action="store_true", + help="Run the committed 20-case beta profile with explicit candidate/critic/publishability expectations", + ) + eval_run_parser.add_argument( + "--repeat", + type=int, + default=1, + help="Repeat a beta-readiness run to measure deterministic variance (default: 1)", + ) eval_run_parser.add_argument( "--context-ablation", action="store_true", help="Additionally run normal vs. target-only vs. no-extra-context and report the quality delta", diff --git a/patchfrog/config/settings.py b/patchfrog/config/settings.py index 2ac5a79..5e66ad2 100644 --- a/patchfrog/config/settings.py +++ b/patchfrog/config/settings.py @@ -12,6 +12,8 @@ from pydantic import Field, ValidationInfo, field_validator, model_validator from pydantic_settings import BaseSettings, SettingsConfigDict +from patchfrog.review.critic_policy import CriticFailurePolicy + class Settings(BaseSettings): """Application settings loaded from environment variables / .env file.""" @@ -134,6 +136,9 @@ class Settings(BaseSettings): review_max_elapsed_seconds: float | None = Field( default=None, alias="PATCHFROG_MAX_REVIEW_ELAPSED_SECONDS" ) + critic_failure_policy: CriticFailurePolicy | None = Field( + default=None, alias="PATCHFROG_CRITIC_FAILURE_POLICY" + ) provider_pricing: dict[str, dict[str, float]] = Field( default_factory=dict, alias="PATCHFROG_PROVIDER_PRICING" ) diff --git a/patchfrog/evaluation/beta_readiness.py b/patchfrog/evaluation/beta_readiness.py new file mode 100644 index 0000000..0df1ffb --- /dev/null +++ b/patchfrog/evaluation/beta_readiness.py @@ -0,0 +1,209 @@ +"""Deterministic beta-readiness evaluation profile and metrics.""" + +from __future__ import annotations + +from collections.abc import Mapping, Sequence +from dataclasses import dataclass +from enum import StrEnum +from pathlib import Path + +import yaml + +from patchfrog.analysis.domain import Confidence, FindingCategory +from patchfrog.evaluation.domain import CaseResult, MatchOutcome + +DEFAULT_BETA_PROFILE = Path(__file__).resolve().parents[2] / "tests" / "fixtures" / "evaluation" / "beta_readiness.yaml" + + +class CriticExpectedOutcome(StrEnum): + ACCEPT = "accept" + NOT_REQUIRED = "not_required" + NOT_RUN = "not_run" + + +class PublishabilityExpectation(StrEnum): + PUBLISH = "publish" + DO_NOT_PUBLISH = "do_not_publish" + + +@dataclass(frozen=True, slots=True) +class BetaCaseExpectation: + case_id: str + should_produce_candidate: bool + expected_category: FindingCategory | None + minimum_confidence: Confidence | None + critic_expected_outcome: CriticExpectedOutcome + publishability: PublishabilityExpectation + + +@dataclass(frozen=True, slots=True) +class BetaReadinessMetrics: + cases: int + expectation_pass_rate: float + candidate_recall: float + accepted_finding_recall: float + false_positive_rate: float + false_negative_rate: float + critic_rejection_rate: float + critic_false_negative_rate: float + repeated_run_variance: float + + +def load_beta_profile(path: Path = DEFAULT_BETA_PROFILE) -> tuple[BetaCaseExpectation, ...]: + raw = yaml.safe_load(path.read_text()) + rows = raw.get("cases", []) if isinstance(raw, dict) else [] + expectations = tuple( + BetaCaseExpectation( + case_id=str(row["case_id"]), + should_produce_candidate=bool(row["should_produce_candidate"]), + expected_category=( + FindingCategory(row["expected_category"]) + if row.get("expected_category") is not None + else None + ), + minimum_confidence=( + Confidence(row["minimum_confidence"]) + if row.get("minimum_confidence") is not None + else None + ), + critic_expected_outcome=CriticExpectedOutcome(row["critic_expected_outcome"]), + publishability=PublishabilityExpectation(row["publishability"]), + ) + for row in rows + ) + if not 15 <= len(expectations) <= 20: + raise ValueError(f"beta-readiness profile must contain 15-20 cases, found {len(expectations)}") + ids = [item.case_id for item in expectations] + if len(set(ids)) != len(ids): + raise ValueError("beta-readiness profile contains duplicate case ids") + return expectations + + +def compute_beta_readiness_metrics( + runs: Sequence[Sequence[CaseResult]], + *, + expectations: Sequence[BetaCaseExpectation], +) -> BetaReadinessMetrics: + if not runs: + raise ValueError("at least one evaluation run is required") + expected_by_id = {item.case_id: item for item in expectations} + first = {result.case_id: result for result in runs[0]} + if set(first) != set(expected_by_id): + raise ValueError("evaluation results do not match the beta-readiness profile") + + candidate_expected = sum(item.should_produce_candidate for item in expectations) + publish_expected = sum(item.publishability is PublishabilityExpectation.PUBLISH for item in expectations) + candidate_hits = accepted_hits = false_positives = true_positives = missed = 0 + critic_calls = critic_rejections = critic_false_negatives = 0 + expectations_passed = 0 + + for case_id, expectation in expected_by_id.items(): + result = first[case_id] + proposal_matches = any( + outcome.outcome is MatchOutcome.TRUE_POSITIVE + and outcome.prediction.category is expectation.expected_category + for outcome in result.proposal_outcomes + ) + if expectation.should_produce_candidate and proposal_matches: + candidate_hits += 1 + + accepted_match = any( + outcome.outcome is MatchOutcome.TRUE_POSITIVE + and outcome.prediction.category is expectation.expected_category + and ( + expectation.minimum_confidence is None + or (outcome.prediction.confidence is not None + and _confidence_rank(outcome.prediction.confidence) + >= _confidence_rank(expectation.minimum_confidence)) + ) + for outcome in result.predictions + ) + if expectation.publishability is PublishabilityExpectation.PUBLISH: + if accepted_match: + accepted_hits += 1 + true_positives += 1 + else: + missed += 1 + if proposal_matches and result.critic_rejections: + critic_false_negatives += 1 + else: + false_positives += sum( + outcome.outcome in (MatchOutcome.FALSE_POSITIVE, MatchOutcome.UNSUPPORTED) + for outcome in result.predictions + ) + candidate_ok = ( + proposal_matches + if expectation.should_produce_candidate + else not result.proposals_before_validation + ) + publish_ok = accepted_match is ( + expectation.publishability is PublishabilityExpectation.PUBLISH + ) + if expectation.critic_expected_outcome is CriticExpectedOutcome.ACCEPT: + critic_ok = result.critic_calls > 0 and result.critic_rejections == 0 and accepted_match + elif expectation.critic_expected_outcome is CriticExpectedOutcome.NOT_RUN: + critic_ok = result.critic_calls == 0 + else: + critic_ok = True + expectations_passed += candidate_ok and publish_ok and critic_ok + critic_calls += result.critic_calls + critic_rejections += result.critic_rejections + + return BetaReadinessMetrics( + cases=len(expectations), + expectation_pass_rate=_ratio(expectations_passed, len(expectations)), + candidate_recall=_ratio(candidate_hits, candidate_expected), + accepted_finding_recall=_ratio(accepted_hits, publish_expected), + false_positive_rate=_ratio(false_positives, true_positives + false_positives), + false_negative_rate=_ratio(missed, true_positives + missed), + critic_rejection_rate=_ratio(critic_rejections, critic_calls), + critic_false_negative_rate=_ratio(critic_false_negatives, publish_expected), + repeated_run_variance=_repeated_run_variance(runs), + ) + + +def _confidence_rank(confidence: Confidence) -> int: + return {Confidence.LOW: 0, Confidence.MEDIUM: 1, Confidence.HIGH: 2}[confidence] + + +def _ratio(numerator: int, denominator: int) -> float: + return numerator / denominator if denominator else 0.0 + + +def _result_signature(result: CaseResult) -> tuple[object, ...]: + return ( + result.status, + tuple( + (p.outcome, p.prediction.category, p.prediction.file_path, p.prediction.start_line) + for p in result.predictions + ), + tuple((p.category, p.file_path, p.start_line) for p in result.proposals_before_validation), + tuple(p.outcome for p in result.proposal_outcomes), + result.critic_calls, + result.critic_rejections, + ) + + +def _repeated_run_variance(runs: Sequence[Sequence[CaseResult]]) -> float: + if len(runs) < 2: + return 0.0 + by_run: list[Mapping[str, CaseResult]] = [ + {result.case_id: result for result in run} for run in runs + ] + case_ids = set(by_run[0]) + varied = sum( + len({_result_signature(run[case_id]) for run in by_run}) > 1 + for case_id in case_ids + ) + return _ratio(varied, len(case_ids)) + + +__all__ = [ + "DEFAULT_BETA_PROFILE", + "BetaCaseExpectation", + "BetaReadinessMetrics", + "CriticExpectedOutcome", + "PublishabilityExpectation", + "compute_beta_readiness_metrics", + "load_beta_profile", +] diff --git a/patchfrog/evaluation/domain.py b/patchfrog/evaluation/domain.py index 882f829..5299fec 100644 --- a/patchfrog/evaluation/domain.py +++ b/patchfrog/evaluation/domain.py @@ -26,7 +26,7 @@ #: Bumped whenever the benchmark corpus (fixtures/ground truth) changes #: materially -- a case added, removed, or re-labeled. -EVALUATION_BENCHMARK_VERSION = 1 +EVALUATION_BENCHMARK_VERSION = 2 #: Bumped whenever this package's own logic (matcher/metrics/regression) #: changes materially -- never for a fixture/label change alone. @@ -39,7 +39,11 @@ #: :mod:`patchfrog.evaluation.regression`), and the evaluation cost/ #: efficiency reporting shape changed materially enough that a v1-shaped #: baseline is no longer directly comparable. -EVALUATION_ENGINE_VERSION = 2 +#: +#: Bumped to 3 for the beta-readiness profile: case results now preserve +#: pre-critic proposal outcomes and critic-rejection counts, enabling +#: candidate/accepted recall and critic false-negative measurements. +EVALUATION_ENGINE_VERSION = 3 class EvaluationMode(StrEnum): @@ -340,6 +344,7 @@ class CaseResult: #: confidence filtering -- used to compare pre- vs. post-validation #: hallucination rate (see :mod:`patchfrog.evaluation.metrics`). proposals_before_validation: tuple[PredictedFinding, ...] = field(default_factory=tuple) + proposal_outcomes: tuple[PredictionOutcome, ...] = field(default_factory=tuple) error: str | None = None critic_enabled: bool = True candidates_generated: int = 0 @@ -371,6 +376,7 @@ class CaseResult: reviewer_thinking_tokens: int = 0 critic_thinking_tokens: int = 0 retries_consumed: int = 0 + critic_rejections: int = 0 @property def is_error(self) -> bool: diff --git a/patchfrog/evaluation/runner.py b/patchfrog/evaluation/runner.py index 533f485..1f44df1 100644 --- a/patchfrog/evaluation/runner.py +++ b/patchfrog/evaluation/runner.py @@ -82,7 +82,7 @@ REVIEW_PROMPT_VERSION, ReviewConfig, ) -from patchfrog.review.domain import ReviewRunSummary +from patchfrog.review.domain import ProposalStatus, ReviewRunSummary from patchfrog.review.effort import uniform_baseline_decision from patchfrog.review.effort_types import ReviewEffortTier from patchfrog.review.provider import LLMProvider, ProviderError @@ -395,6 +395,7 @@ async def _run_case_inner( reviewer_output_tokens_by_role: dict[AgentRole, int] = {} candidates_by_tier: dict[ReviewEffortTier, int] = {} candidates_escalated = critic_calls = retries_consumed = 0 + critic_rejections = 0 reviewer_thinking_tokens = critic_thinking_tokens = 0 if mode in (EvaluationMode.AI_ONLY, EvaluationMode.FULL_PIPELINE): @@ -455,18 +456,27 @@ async def _run_case_inner( proposals_predicted = [ _ai_to_predicted(p, candidate_by_id.get(p.candidate_id)) for p in proposals ] + critic_rejections = sum(p.status is ProposalStatus.REJECTED_CRITIC for p in proposals) all_predictions = static_predictions + ai_predictions prediction_outcomes, expected_outcomes = match_case( case=case, mode=mode, predictions=all_predictions, valid_file_paths=fixture_info.valid_file_paths, file_line_counts=fixture_info.file_line_counts, ) + proposal_outcomes, _ = match_case( + case=case, + mode=mode, + predictions=proposals_predicted, + valid_file_paths=fixture_info.valid_file_paths, + file_line_counts=fixture_info.file_line_counts, + ) status = CaseStatus.PASSED if not prediction_outcomes else CaseStatus.COMPLETED_WITH_FINDINGS return CaseResult( case_id=case.id, mode=mode, status=status, duration_ms=(time.monotonic() - start) * 1000, predictions=prediction_outcomes, expected_outcomes=expected_outcomes, proposals_before_validation=tuple(proposals_predicted), critic_enabled=critic_enabled, + proposal_outcomes=proposal_outcomes, candidates_generated=candidates_generated, candidates_reviewed=candidates_reviewed, candidates_skipped=candidates_skipped, provider_calls=provider_calls, reviewer_input_tokens=reviewer_input_tokens, reviewer_output_tokens=reviewer_output_tokens, @@ -480,6 +490,7 @@ async def _run_case_inner( reviewer_thinking_tokens=reviewer_thinking_tokens, critic_thinking_tokens=critic_thinking_tokens, retries_consumed=retries_consumed, + critic_rejections=critic_rejections, ) finally: shutil.rmtree(repo_root, ignore_errors=True) @@ -542,4 +553,3 @@ def factory(case: EvaluationCase) -> LLMProvider: ) return factory - diff --git a/patchfrog/review/agents/cross_role.py b/patchfrog/review/agents/cross_role.py index 5af077c..9a0b7d0 100644 --- a/patchfrog/review/agents/cross_role.py +++ b/patchfrog/review/agents/cross_role.py @@ -81,6 +81,13 @@ def is_contradiction(a: AgentProposal, b: AgentProposal) -> bool: if a.role == b.role: return False fa, fb = a.validated.finding, b.validated.finding + # Two agents can emit the exact same finding text. Security wording + # commonly contains both a negated protection ("not sanitized") and a + # risk term ("injection"), so running identical text through the lexical + # opposition heuristic would otherwise manufacture a contradiction with + # itself. Exact agreement always wins over heuristic classification. + if fa == fb: + return False if not _overlaps_location(fa, fb) or not _shared_verbatim_evidence(fa, fb): return False diff --git a/patchfrog/review/config.py b/patchfrog/review/config.py index 28cc3ee..cf69511 100644 --- a/patchfrog/review/config.py +++ b/patchfrog/review/config.py @@ -34,6 +34,7 @@ from pydantic import BaseModel, ConfigDict from patchfrog.analysis.domain import Confidence +from patchfrog.review.critic_policy import CriticFailurePolicy logger = structlog.get_logger(__name__) @@ -215,6 +216,7 @@ class ReviewConfig(BaseModel): model_config = ConfigDict(extra="ignore") critic_enabled: bool = True + critic_failure_policy: CriticFailurePolicy = CriticFailurePolicy.FAIL_OPEN max_candidates: int = DEFAULT_MAX_CANDIDATES max_input_tokens_per_candidate: int = DEFAULT_MAX_INPUT_TOKENS_PER_CANDIDATE max_output_tokens_per_candidate: int = DEFAULT_MAX_OUTPUT_TOKENS_PER_CANDIDATE @@ -239,6 +241,7 @@ def fingerprint(self) -> str: payload = { "schema_version": CONFIG_SCHEMA_VERSION, "critic_enabled": self.critic_enabled, + "critic_failure_policy": self.critic_failure_policy.value, "max_candidates": self.max_candidates, "max_input_tokens_per_candidate": self.max_input_tokens_per_candidate, "max_output_tokens_per_candidate": self.max_output_tokens_per_candidate, diff --git a/patchfrog/review/config_resolution.py b/patchfrog/review/config_resolution.py index 1b664ab..d254896 100644 --- a/patchfrog/review/config_resolution.py +++ b/patchfrog/review/config_resolution.py @@ -99,6 +99,7 @@ def apply_operator_hard_caps(repo_config: ReviewConfig, *, settings: Settings) - return ReviewConfig( critic_enabled=repo_config.critic_enabled, + critic_failure_policy=settings.critic_failure_policy or repo_config.critic_failure_policy, max_candidates=min(repo_config.max_candidates, settings.review_max_candidates), max_input_tokens_per_candidate=repo_config.max_input_tokens_per_candidate, max_output_tokens_per_candidate=min( diff --git a/patchfrog/review/critic_policy.py b/patchfrog/review/critic_policy.py new file mode 100644 index 0000000..828fb13 --- /dev/null +++ b/patchfrog/review/critic_policy.py @@ -0,0 +1,20 @@ +"""Explicit policy for typed critic failures.""" + +from enum import StrEnum + + +class CriticFailurePolicy(StrEnum): + """What to do when a selected critic cannot return a valid verdict. + + ``FAIL_OPEN`` preserves PatchFrog's historical behavior: deterministic + validation and confidence aggregation may still accept the reviewer + proposal. ``HOLD_FOR_REVIEW`` suppresses the proposal until a critic can + verify it; this is safer for mandatory/high-risk review environments. + Unexpected programming errors always propagate under either policy. + """ + + FAIL_OPEN = "fail_open" + HOLD_FOR_REVIEW = "hold_for_review" + + +__all__ = ["CriticFailurePolicy"] diff --git a/patchfrog/review/domain.py b/patchfrog/review/domain.py index 3559cf9..97dc69a 100644 --- a/patchfrog/review/domain.py +++ b/patchfrog/review/domain.py @@ -270,6 +270,10 @@ class ProposalStatus(StrEnum): #: :data:`patchfrog.review.orchestration.CRITIC_BUDGET_EXHAUSTED`. #: Suppressed rather than published unverified. SUPPRESSED_BUDGET = "suppressed_budget" + #: A selected critic failed and the effective + #: ``CriticFailurePolicy`` required holding the proposal rather than + #: accepting it on reviewer confidence alone. + SUPPRESSED_CRITIC_FAILURE = "suppressed_critic_failure" @dataclass(frozen=True, slots=True) diff --git a/patchfrog/review/orchestration.py b/patchfrog/review/orchestration.py index 6650603..902797e 100644 --- a/patchfrog/review/orchestration.py +++ b/patchfrog/review/orchestration.py @@ -76,6 +76,7 @@ from patchfrog.review.agents.roles import AgentRole from patchfrog.review.budget import BudgetExceeded, ReviewBudget from patchfrog.review.critic import CriticService +from patchfrog.review.critic_policy import CriticFailurePolicy from patchfrog.review.critic_selection import CriticSelectionInput, CriticSelectionPolicy from patchfrog.review.domain import ( AIReviewFinding, @@ -112,6 +113,7 @@ #: outcome, never "publish because the reviewer call was already paid #: for." CRITIC_BUDGET_EXHAUSTED = "critic_budget_exhausted" +CRITIC_FAILURE_HOLD = "critic_failure_hold" @dataclass(slots=True) @@ -262,6 +264,7 @@ def __init__( reviewer_fallback_providers: Mapping[AgentRole, LLMProvider] | None = None, critic_fallback: CriticService | None = None, review_budget: ReviewBudget | None = None, + critic_failure_policy: CriticFailurePolicy = CriticFailurePolicy.FAIL_OPEN, ) -> None: """``reviewer_fallback_providers``/``critic_fallback`` (Milestone U runtime-failover correction): an optional, distinct provider used @@ -295,6 +298,7 @@ def __init__( #: :meth:`review_candidate`, never re-derived here. self._effort_policy = effort_policy or ReviewEffortPolicy() self._review_budget = review_budget + self._critic_failure_policy = critic_failure_policy async def review_candidate( self, @@ -797,6 +801,8 @@ async def _critique( error_type=type(verdict_outcome).__name__, provider_failure_kind=getattr(verdict_outcome, "kind", None), ) + if self._critic_failure_policy is CriticFailurePolicy.HOLD_FOR_REVIEW: + result[i] = result[i].suppressed(CRITIC_FAILURE_HOLD) continue raise verdict_outcome critic_calls += 1 diff --git a/patchfrog/review/service.py b/patchfrog/review/service.py index 12a0322..8379224 100644 --- a/patchfrog/review/service.py +++ b/patchfrog/review/service.py @@ -182,7 +182,11 @@ ) from patchfrog.review.effort import ReviewEffortDecision, ReviewEffortPolicy from patchfrog.review.effort_types import ReviewEffortTier -from patchfrog.review.orchestration import CRITIC_BUDGET_EXHAUSTED, AgentOrchestrator +from patchfrog.review.orchestration import ( + CRITIC_BUDGET_EXHAUSTED, + CRITIC_FAILURE_HOLD, + AgentOrchestrator, +) from patchfrog.review.provider import LLMProvider from patchfrog.review.redaction import redact_secrets from patchfrog.routing.domain import ReviewRoutePlan @@ -1107,6 +1111,7 @@ def _requires_critic(c: ReviewCandidate) -> bool: reviewer_fallback_providers=self._reviewer_fallback_providers, critic_fallback=self._critic_fallback, review_budget=review_budget, + critic_failure_policy=config.critic_failure_policy, ) async def _process(outcome: _CandidateOutcome) -> None: @@ -1328,6 +1333,26 @@ async def _process(outcome: _CandidateOutcome) -> None: ) continue + if agent_proposal.suppressed_reason == CRITIC_FAILURE_HOLD: + proposal = await self._proposal_repo.create( + session, + review_run_id=run_id, + candidate_id=candidate_model.id, + finding=validated.finding, + status=ProposalStatus.SUPPRESSED_CRITIC_FAILURE, + validation_detail="critic verification failed under hold-for-review policy", + agent_role=agent_proposal.role, + validation_outcome=validated.outcome, + ) + _log_finding_candidate_diagnostics( + log, + finding_id=proposal.id, + finding=validated.finding, + status=ProposalStatus.SUPPRESSED_CRITIC_FAILURE, + verdict=None, + ) + continue + final = next( (f for f in outcome.final if f.finding is validated.finding), None ) diff --git a/patchfrog/telemetry/domain.py b/patchfrog/telemetry/domain.py index 0d8f702..66a0866 100644 --- a/patchfrog/telemetry/domain.py +++ b/patchfrog/telemetry/domain.py @@ -148,6 +148,7 @@ class FindingLifecycleOutcome(StrEnum): SUPPRESSED_DUPLICATE = "suppressed_duplicate" SUPPRESSED_CONTRADICTION = "suppressed_contradiction" SUPPRESSED_BUDGET = "suppressed_budget" + SUPPRESSED_CRITIC_FAILURE = "suppressed_critic_failure" BELOW_CONFIDENCE_THRESHOLD = "below_confidence_threshold" ACCEPTED_FINAL = "accepted_final" @@ -159,6 +160,7 @@ class FindingLifecycleOutcome(StrEnum): ProposalStatus.SUPPRESSED_DUPLICATE: FindingLifecycleOutcome.SUPPRESSED_DUPLICATE, ProposalStatus.SUPPRESSED_CONTRADICTION: FindingLifecycleOutcome.SUPPRESSED_CONTRADICTION, ProposalStatus.SUPPRESSED_BUDGET: FindingLifecycleOutcome.SUPPRESSED_BUDGET, + ProposalStatus.SUPPRESSED_CRITIC_FAILURE: FindingLifecycleOutcome.SUPPRESSED_CRITIC_FAILURE, } diff --git a/tests/fixtures/evaluation/beta_readiness.yaml b/tests/fixtures/evaluation/beta_readiness.yaml new file mode 100644 index 0000000..f146a28 --- /dev/null +++ b/tests/fixtures/evaluation/beta_readiness.yaml @@ -0,0 +1,22 @@ +version: 1 +cases: + - {case_id: hard-py-caller-callee-argument-swap, should_produce_candidate: true, expected_category: correctness, minimum_confidence: high, critic_expected_outcome: accept, publishability: publish} + - {case_id: py-off-by-one-loop, should_produce_candidate: true, expected_category: correctness, minimum_confidence: high, critic_expected_outcome: not_required, publishability: publish} + - {case_id: hard-py-cross-file-contract-violation, should_produce_candidate: true, expected_category: correctness, minimum_confidence: high, critic_expected_outcome: accept, publishability: publish} + - {case_id: py-wrong-boolean-branch, should_produce_candidate: true, expected_category: correctness, minimum_confidence: high, critic_expected_outcome: not_required, publishability: publish} + - {case_id: hard-py-stale-cache-invalidation, should_produce_candidate: true, expected_category: correctness, minimum_confidence: high, critic_expected_outcome: not_required, publishability: publish} + - {case_id: py-ignored-error-return, should_produce_candidate: true, expected_category: api_misuse, minimum_confidence: high, critic_expected_outcome: not_required, publishability: publish} + - {case_id: c-file-handle-leak, should_produce_candidate: true, expected_category: resource_management, minimum_confidence: high, critic_expected_outcome: not_required, publishability: publish} + - {case_id: py-auth-bypass, should_produce_candidate: true, expected_category: security, minimum_confidence: high, critic_expected_outcome: accept, publishability: publish} + - {case_id: py-path-traversal, should_produce_candidate: true, expected_category: security, minimum_confidence: high, critic_expected_outcome: accept, publishability: publish} + - {case_id: secq-e-command-injection, should_produce_candidate: true, expected_category: security, minimum_confidence: high, critic_expected_outcome: accept, publishability: publish} + - {case_id: secq-a-credential-log-exposure, should_produce_candidate: true, expected_category: security, minimum_confidence: high, critic_expected_outcome: accept, publishability: publish} + - {case_id: beta-api-return-shape-break, should_produce_candidate: true, expected_category: api_misuse, minimum_confidence: high, critic_expected_outcome: accept, publishability: publish} + - {case_id: py-wrong-argument-semantics, should_produce_candidate: true, expected_category: api_misuse, minimum_confidence: high, critic_expected_outcome: not_required, publishability: publish} + - {case_id: beta-schema-mismatch, should_produce_candidate: true, expected_category: api_misuse, minimum_confidence: high, critic_expected_outcome: accept, publishability: publish} + - {case_id: py-claimed-intentional-swapped-counts, should_produce_candidate: true, expected_category: correctness, minimum_confidence: high, critic_expected_outcome: not_required, publishability: publish} + - {case_id: beta-safe-refactor, should_produce_candidate: false, expected_category: null, minimum_confidence: null, critic_expected_outcome: not_run, publishability: do_not_publish} + - {case_id: beta-test-only-change, should_produce_candidate: false, expected_category: null, minimum_confidence: null, critic_expected_outcome: not_run, publishability: do_not_publish} + - {case_id: beta-comment-only-change, should_produce_candidate: false, expected_category: null, minimum_confidence: null, critic_expected_outcome: not_run, publishability: do_not_publish} + - {case_id: beta-behavior-preserving-rename, should_produce_candidate: false, expected_category: null, minimum_confidence: null, critic_expected_outcome: not_run, publishability: do_not_publish} + - {case_id: beta-generated-fixture-context, should_produce_candidate: false, expected_category: null, minimum_confidence: null, critic_expected_outcome: not_run, publishability: do_not_publish} diff --git a/tests/fixtures/evaluation/cases/beta-api-return-shape-break/case.yaml b/tests/fixtures/evaluation/cases/beta-api-return-shape-break/case.yaml new file mode 100644 index 0000000..6843c53 --- /dev/null +++ b/tests/fixtures/evaluation/cases/beta-api-return-shape-break/case.yaml @@ -0,0 +1,18 @@ +case: + id: beta-api-return-shape-break + title: User serializer breaks its documented API return shape + description: serialize_user promises id and display_name but returns name instead. + language: python + difficulty: medium + tags: [beta-readiness, contract, api] +expected: + - id: ef1 + category: api_misuse + file: src/users.py + symbol: serialize_user + issue_family: api_return_shape_break + severity: high + line: 7 + ground_truth_source: ai_expected + notes: The public response contract requires display_name, not name. +forbidden: [] diff --git a/tests/fixtures/evaluation/cases/beta-api-return-shape-break/repo/src/users.py b/tests/fixtures/evaluation/cases/beta-api-return-shape-break/repo/src/users.py new file mode 100644 index 0000000..27b150c --- /dev/null +++ b/tests/fixtures/evaluation/cases/beta-api-return-shape-break/repo/src/users.py @@ -0,0 +1,7 @@ +def serialize_user(user: dict[str, object]) -> dict[str, object]: + """Return the public API shape: {"id": ..., "display_name": ...}.""" + return { + "id": user["id"], + # Clients read display_name; this silently changes the response contract. + "name": user["display_name"], + } diff --git a/tests/fixtures/evaluation/cases/beta-behavior-preserving-rename/case.yaml b/tests/fixtures/evaluation/cases/beta-behavior-preserving-rename/case.yaml new file mode 100644 index 0000000..5353ba9 --- /dev/null +++ b/tests/fixtures/evaluation/cases/beta-behavior-preserving-rename/case.yaml @@ -0,0 +1,10 @@ +case: + id: beta-behavior-preserving-rename + title: Local rename preserves behavior + description: A clearer local variable name does not alter the result. + language: python + difficulty: easy + tags: [beta-readiness, false-positive-trap, rename] +expected: [] +forbidden: + - {reason: Renaming a local leaves the calculation unchanged, category: correctness} diff --git a/tests/fixtures/evaluation/cases/beta-behavior-preserving-rename/repo/src/pricing.py b/tests/fixtures/evaluation/cases/beta-behavior-preserving-rename/repo/src/pricing.py new file mode 100644 index 0000000..d310f07 --- /dev/null +++ b/tests/fixtures/evaluation/cases/beta-behavior-preserving-rename/repo/src/pricing.py @@ -0,0 +1,3 @@ +def discounted(price: int, discount: int) -> int: + discounted_price = price - discount + return max(0, discounted_price) diff --git a/tests/fixtures/evaluation/cases/beta-comment-only-change/case.yaml b/tests/fixtures/evaluation/cases/beta-comment-only-change/case.yaml new file mode 100644 index 0000000..8365eaf --- /dev/null +++ b/tests/fixtures/evaluation/cases/beta-comment-only-change/case.yaml @@ -0,0 +1,10 @@ +case: + id: beta-comment-only-change + title: Comment-only clarification must not create a finding + description: The code is correct and the comment only explains an inclusive bound. + language: python + difficulty: easy + tags: [beta-readiness, false-positive-trap, comment-only] +expected: [] +forbidden: + - {reason: The inclusive comparison matches the documented behavior, category: correctness} diff --git a/tests/fixtures/evaluation/cases/beta-comment-only-change/repo/src/limits.py b/tests/fixtures/evaluation/cases/beta-comment-only-change/repo/src/limits.py new file mode 100644 index 0000000..48789eb --- /dev/null +++ b/tests/fixtures/evaluation/cases/beta-comment-only-change/repo/src/limits.py @@ -0,0 +1,3 @@ +def within_limit(value: int, limit: int) -> bool: + # The configured limit is inclusive by product definition. + return value <= limit diff --git a/tests/fixtures/evaluation/cases/beta-generated-fixture-context/case.yaml b/tests/fixtures/evaluation/cases/beta-generated-fixture-context/case.yaml new file mode 100644 index 0000000..4d88392 --- /dev/null +++ b/tests/fixtures/evaluation/cases/beta-generated-fixture-context/case.yaml @@ -0,0 +1,10 @@ +case: + id: beta-generated-fixture-context + title: Generated fixture content is not reviewable production code + description: Deterministic generated data should not produce an actionable finding. + language: python + difficulty: easy + tags: [beta-readiness, false-positive-trap, generated, fixture] +expected: [] +forbidden: + - {reason: This committed file is generated fixture data, category: maintainability} diff --git a/tests/fixtures/evaluation/cases/beta-generated-fixture-context/repo/fixtures/generated_values.py b/tests/fixtures/evaluation/cases/beta-generated-fixture-context/repo/fixtures/generated_values.py new file mode 100644 index 0000000..8517d0f --- /dev/null +++ b/tests/fixtures/evaluation/cases/beta-generated-fixture-context/repo/fixtures/generated_values.py @@ -0,0 +1,2 @@ +# @generated by tools/build_fixture.py; do not edit by hand. +VALUES = ("alpha", "beta", "gamma") diff --git a/tests/fixtures/evaluation/cases/beta-safe-refactor/case.yaml b/tests/fixtures/evaluation/cases/beta-safe-refactor/case.yaml new file mode 100644 index 0000000..72b3117 --- /dev/null +++ b/tests/fixtures/evaluation/cases/beta-safe-refactor/case.yaml @@ -0,0 +1,10 @@ +case: + id: beta-safe-refactor + title: Equivalent helper extraction is not a defect + description: A direct arithmetic expression is extracted without changing behavior. + language: python + difficulty: easy + tags: [beta-readiness, false-positive-trap, safe-refactor] +expected: [] +forbidden: + - {reason: The helper extraction preserves the exact calculation, category: correctness} diff --git a/tests/fixtures/evaluation/cases/beta-safe-refactor/repo/src/totals.py b/tests/fixtures/evaluation/cases/beta-safe-refactor/repo/src/totals.py new file mode 100644 index 0000000..4b0043c --- /dev/null +++ b/tests/fixtures/evaluation/cases/beta-safe-refactor/repo/src/totals.py @@ -0,0 +1,6 @@ +def subtotal(price: int, quantity: int) -> int: + return price * quantity + + +def invoice_total(price: int, quantity: int, shipping: int) -> int: + return subtotal(price, quantity) + shipping diff --git a/tests/fixtures/evaluation/cases/beta-schema-mismatch/case.yaml b/tests/fixtures/evaluation/cases/beta-schema-mismatch/case.yaml new file mode 100644 index 0000000..5832d47 --- /dev/null +++ b/tests/fixtures/evaluation/cases/beta-schema-mismatch/case.yaml @@ -0,0 +1,18 @@ +case: + id: beta-schema-mismatch + title: Event payload uses a field outside the consumer schema + description: The producer emits account_id while the consumer schema requires user_id. + language: python + difficulty: medium + tags: [beta-readiness, contract, schema] +expected: + - id: ef1 + category: api_misuse + file: src/events.py + symbol: build_login_event + issue_family: schema_field_mismatch + severity: high + line: 8 + ground_truth_source: ai_expected + notes: LOGIN_EVENT_SCHEMA requires user_id, but the producer emits account_id. +forbidden: [] diff --git a/tests/fixtures/evaluation/cases/beta-schema-mismatch/repo/src/events.py b/tests/fixtures/evaluation/cases/beta-schema-mismatch/repo/src/events.py new file mode 100644 index 0000000..bd69517 --- /dev/null +++ b/tests/fixtures/evaluation/cases/beta-schema-mismatch/repo/src/events.py @@ -0,0 +1,9 @@ +LOGIN_EVENT_SCHEMA = {"required": ("event", "user_id")} + + +def build_login_event(user_id: str) -> dict[str, str]: + """Build a payload accepted by LOGIN_EVENT_SCHEMA.""" + return { + "event": "login", + "account_id": user_id, + } diff --git a/tests/fixtures/evaluation/cases/beta-test-only-change/case.yaml b/tests/fixtures/evaluation/cases/beta-test-only-change/case.yaml new file mode 100644 index 0000000..d4f6637 --- /dev/null +++ b/tests/fixtures/evaluation/cases/beta-test-only-change/case.yaml @@ -0,0 +1,10 @@ +case: + id: beta-test-only-change + title: Test-only coverage addition is not a production defect + description: A focused unit test documents already-correct behavior. + language: python + difficulty: easy + tags: [beta-readiness, false-positive-trap, test-only] +expected: [] +forbidden: + - {reason: The assertion correctly describes the tested contract, category: correctness} diff --git a/tests/fixtures/evaluation/cases/beta-test-only-change/repo/tests/test_slug.py b/tests/fixtures/evaluation/cases/beta-test-only-change/repo/tests/test_slug.py new file mode 100644 index 0000000..3b3251e --- /dev/null +++ b/tests/fixtures/evaluation/cases/beta-test-only-change/repo/tests/test_slug.py @@ -0,0 +1,3 @@ +def test_slug_normalizes_spaces() -> None: + value = "hello world".replace(" ", "-") + assert value == "hello-world" diff --git a/tests/integration/test_beta_readiness_evaluation.py b/tests/integration/test_beta_readiness_evaluation.py new file mode 100644 index 0000000..8973f0b --- /dev/null +++ b/tests/integration/test_beta_readiness_evaluation.py @@ -0,0 +1,81 @@ +from __future__ import annotations + +from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker + +from patchfrog.evaluation.beta_readiness import ( + CriticExpectedOutcome, + compute_beta_readiness_metrics, + load_beta_profile, +) +from patchfrog.evaluation.domain import EvaluationMode, MatchOutcome +from patchfrog.evaluation.fixtures import DEFAULT_CASES_ROOT, load_all_cases +from patchfrog.evaluation.runner import EvaluationRunner, oracle_reviewer_provider_factory + + +async def test_beta_readiness_profile_is_deterministic_without_live_providers( + session_factory: async_sessionmaker[AsyncSession], +) -> None: + expectations = load_beta_profile() + expected_ids = {item.case_id for item in expectations} + cases = [case for case in load_all_cases() if case.id in expected_ids] + provider_factory = oracle_reviewer_provider_factory(cases_root=DEFAULT_CASES_ROOT) + runner = EvaluationRunner(session_factory=session_factory) + + first = await runner.run_suite( + cases, + cases_root=DEFAULT_CASES_ROOT, + mode=EvaluationMode.AI_ONLY, + reviewer_provider_factory=provider_factory, + critic_provider_factory=provider_factory, + critic_enabled=True, + ) + second = await runner.run_suite( + cases, + cases_root=DEFAULT_CASES_ROOT, + mode=EvaluationMode.AI_ONLY, + reviewer_provider_factory=provider_factory, + critic_provider_factory=provider_factory, + critic_enabled=True, + ) + metrics = compute_beta_readiness_metrics([first, second], expectations=expectations) + results_by_id = {result.case_id: result for result in first} + failures: list[str] = [] + for expectation in expectations: + result = results_by_id[expectation.case_id] + proposal_match = any( + outcome.outcome is MatchOutcome.TRUE_POSITIVE + and outcome.prediction.category is expectation.expected_category + for outcome in result.proposal_outcomes + ) + accepted_match = any( + outcome.outcome is MatchOutcome.TRUE_POSITIVE + and outcome.prediction.category is expectation.expected_category + for outcome in result.predictions + ) + if proposal_match is not expectation.should_produce_candidate: + failures.append(f"{expectation.case_id}: candidate={proposal_match}") + should_publish = expectation.publishability.value == "publish" + if accepted_match is not should_publish: + failures.append(f"{expectation.case_id}: publishable={accepted_match}") + if ( + expectation.critic_expected_outcome is CriticExpectedOutcome.ACCEPT + and not (result.critic_calls > 0 and result.critic_rejections == 0 and accepted_match) + ): + failures.append( + f"{expectation.case_id}: critic_calls={result.critic_calls}, " + f"critic_rejections={result.critic_rejections}" + ) + if ( + expectation.critic_expected_outcome is CriticExpectedOutcome.NOT_RUN + and result.critic_calls != 0 + ): + failures.append(f"{expectation.case_id}: critic_calls={result.critic_calls}") + + assert failures == [] + assert metrics.expectation_pass_rate == 1.0 + assert metrics.candidate_recall == 1.0 + assert metrics.accepted_finding_recall == 1.0 + assert metrics.false_positive_rate == 0.0 + assert metrics.false_negative_rate == 0.0 + assert metrics.critic_false_negative_rate == 0.0 + assert metrics.repeated_run_variance == 0.0 diff --git a/tests/integration/test_review_failure_recovery.py b/tests/integration/test_review_failure_recovery.py index 17bcb41..fe87ea5 100644 --- a/tests/integration/test_review_failure_recovery.py +++ b/tests/integration/test_review_failure_recovery.py @@ -14,9 +14,10 @@ from patchfrog.diff.models import DiffFile, DiffHunk, DiffLine, DiffLineType from patchfrog.indexing.service import RepositoryIndexingService -from patchfrog.persistence.repositories import RepositoryRepository +from patchfrog.persistence.repositories import AIFindingProposalRepository, RepositoryRepository from patchfrog.review.config import ReviewConfig -from patchfrog.review.domain import ReviewRunStatus +from patchfrog.review.critic_policy import CriticFailurePolicy +from patchfrog.review.domain import ProposalStatus, ReviewRunStatus from patchfrog.review.provider import ProviderFatalError, ProviderRequest from patchfrog.review.providers.fake import FakeLLMProvider, ScriptedResponse from patchfrog.review.service import PullRequestReviewService @@ -168,6 +169,41 @@ async def test_critic_schema_failure_falls_back_to_no_critic_aggregation( assert summary.accepted_count == 1 # survived on reviewer confidence alone, no critic ceiling applied +async def test_critic_schema_failure_can_hold_finding_for_review( + session_factory: async_sessionmaker[AsyncSession], +) -> None: + repository_id, commit_sha, root_path = await _setup( + session_factory, full_name="test/review-fail-hold" + ) + diff_files = [_diff_marking_lines("src/billing.py", [14])] + reviewer = FakeLLMProvider( + response_factory=lambda req: ScriptedResponse( + raw_json=json.dumps({"findings": [_backwards_comparison_finding()]}) + ) + ) + critic = FakeLLMProvider([ScriptedResponse(raw_json="not valid json")]) + service = PullRequestReviewService( + session_factory=session_factory, reviewer_provider=reviewer, critic_provider=critic + ) + + summary = await service.review_local( + repository_id=repository_id, + root_path=root_path, + repository_full_name="test/review-fail-hold", + commit_sha=commit_sha, + diff_files=diff_files, + config=ReviewConfig(critic_failure_policy=CriticFailurePolicy.HOLD_FOR_REVIEW), + ) + + assert summary.status is ReviewRunStatus.SUCCEEDED + assert summary.accepted_count == 0 + async with session_factory() as session: + proposals = await AIFindingProposalRepository().list_for_run( + session, review_run_id=summary.run_id + ) + assert [p.status for p in proposals] == [ProposalStatus.SUPPRESSED_CRITIC_FAILURE] + + async def test_untyped_critic_exception_is_not_gracefully_degraded( session_factory: async_sessionmaker[AsyncSession], ) -> None: diff --git a/tests/unit/test_beta_readiness_evaluation.py b/tests/unit/test_beta_readiness_evaluation.py new file mode 100644 index 0000000..da8e07d --- /dev/null +++ b/tests/unit/test_beta_readiness_evaluation.py @@ -0,0 +1,144 @@ +from __future__ import annotations + +from dataclasses import replace + +from patchfrog.analysis.domain import Confidence, FindingCategory, Severity +from patchfrog.evaluation.beta_readiness import ( + BetaCaseExpectation, + CriticExpectedOutcome, + PublishabilityExpectation, + compute_beta_readiness_metrics, + load_beta_profile, +) +from patchfrog.evaluation.domain import ( + CaseResult, + CaseStatus, + EvaluationMode, + MatchOutcome, + PredictedFinding, + PredictionOutcome, + PredictionSource, +) +from patchfrog.evaluation.fixtures import DEFAULT_CASES_ROOT, load_all_cases, validate_and_raise + + +def _prediction(case_id: str, category: FindingCategory) -> PredictedFinding: + return PredictedFinding( + source=PredictionSource.AI, + category=category, + severity=Severity.HIGH, + confidence=Confidence.HIGH, + title=case_id, + message="deterministic fixture finding", + file_path="src/example.py", + start_line=1, + end_line=1, + symbol_qualified_name="example", + evidence_text="example", + ) + + +def _perfect_result(expectation: BetaCaseExpectation) -> CaseResult: + case_id = expectation.case_id + category = expectation.expected_category + should_produce = expectation.should_produce_candidate + should_publish = expectation.publishability is PublishabilityExpectation.PUBLISH + assert category is not None or not should_produce + proposal = _prediction(case_id, category) if should_produce and category is not None else None + prediction = ( + PredictionOutcome( + prediction=proposal, + outcome=MatchOutcome.TRUE_POSITIVE, + matched_expected_id="ef1", + detail="matched", + ) + if should_publish and proposal is not None + else None + ) + critic_calls = int(expectation.critic_expected_outcome is CriticExpectedOutcome.ACCEPT) + return CaseResult( + case_id=case_id, + mode=EvaluationMode.AI_ONLY, + status=CaseStatus.COMPLETED_WITH_FINDINGS if prediction else CaseStatus.PASSED, + duration_ms=1.0, + predictions=(prediction,) if prediction else (), + proposals_before_validation=(proposal,) if proposal else (), + proposal_outcomes=(prediction,) if prediction else (), + critic_calls=critic_calls, + ) + + +def test_beta_profile_has_twenty_valid_explicit_cases() -> None: + expectations = load_beta_profile() + all_cases = load_all_cases() + validate_and_raise(all_cases, cases_root=DEFAULT_CASES_ROOT) + + assert len(expectations) == 20 + assert {item.case_id for item in expectations} <= {case.id for case in all_cases} + assert all( + item.expected_category is not None and item.minimum_confidence is not None + for item in expectations + if item.should_produce_candidate + ) + + +def test_beta_metrics_cover_recall_false_positives_critic_and_variance() -> None: + expectations = load_beta_profile() + first = tuple(_perfect_result(item) for item in expectations) + metrics = compute_beta_readiness_metrics([first, first], expectations=expectations) + + assert metrics.expectation_pass_rate == 1.0 + assert metrics.candidate_recall == 1.0 + assert metrics.accepted_finding_recall == 1.0 + assert metrics.false_positive_rate == 0.0 + assert metrics.false_negative_rate == 0.0 + assert metrics.critic_rejection_rate == 0.0 + assert metrics.critic_false_negative_rate == 0.0 + assert metrics.repeated_run_variance == 0.0 + + changed = list(first) + changed[0] = replace(changed[0], predictions=()) + varied = compute_beta_readiness_metrics([first, tuple(changed)], expectations=expectations) + assert varied.repeated_run_variance == 1 / 20 + + +def test_beta_metrics_expose_false_positive_and_critic_false_negative() -> None: + expectations = load_beta_profile() + results = [_perfect_result(item) for item in expectations] + + positive_index = next( + index + for index, item in enumerate(expectations) + if item.publishability is PublishabilityExpectation.PUBLISH + ) + positive = results[positive_index] + results[positive_index] = replace( + positive, + predictions=(), + critic_calls=1, + critic_rejections=1, + ) + + negative_index = next( + index + for index, item in enumerate(expectations) + if item.publishability is PublishabilityExpectation.DO_NOT_PUBLISH + ) + false_positive = PredictionOutcome( + prediction=_prediction(expectations[negative_index].case_id, FindingCategory.CORRECTNESS), + outcome=MatchOutcome.FALSE_POSITIVE, + matched_expected_id=None, + detail="unexpected finding", + ) + results[negative_index] = replace( + results[negative_index], + predictions=(false_positive,), + ) + + metrics = compute_beta_readiness_metrics([tuple(results)], expectations=expectations) + + assert metrics.false_positive_rate > 0 + assert metrics.false_negative_rate > 0 + assert metrics.critic_rejection_rate > 0 + assert metrics.critic_false_negative_rate > 0 + assert metrics.expectation_pass_rate < 1 diff --git a/tests/unit/test_evaluation_telemetry_identity.py b/tests/unit/test_evaluation_telemetry_identity.py index 448c309..30c4cb3 100644 --- a/tests/unit/test_evaluation_telemetry_identity.py +++ b/tests/unit/test_evaluation_telemetry_identity.py @@ -26,8 +26,8 @@ def _identity(**kwargs: object) -> dict[str, object]: return asdict(identity) -def test_evaluation_engine_version_is_2() -> None: - assert EVALUATION_ENGINE_VERSION == 2 +def test_evaluation_engine_version_is_3() -> None: + assert EVALUATION_ENGINE_VERSION == 3 def test_quality_cost_policy_version_participates_in_identity() -> None: diff --git a/tests/unit/test_review_agents_cross_role.py b/tests/unit/test_review_agents_cross_role.py index a39406a..dfdb967 100644 --- a/tests/unit/test_review_agents_cross_role.py +++ b/tests/unit/test_review_agents_cross_role.py @@ -167,6 +167,21 @@ def test_agreement_is_not_a_contradiction() -> None: assert is_contradiction(a, b) is False +def test_identical_risk_finding_is_agreement_not_lexical_contradiction() -> None: + finding = _finding( + category=FindingCategory.SECURITY, + message="Input is not sanitized, enabling command injection", + reasoning_summary="Untrusted input reaches an unsafe shell sink", + ) + correctness = _proposal(AgentRole.CORRECTNESS, finding) + security = _proposal(AgentRole.SECURITY, finding) + + assert is_contradiction(correctness, security) is False + result = group_cross_role((correctness, security)) + assert not result.contradiction_indices + assert sum(p.suppressed_reason == CROSS_ROLE_DUPLICATE for p in result.proposals) == 1 + + def test_group_cross_role_merges_exact_duplicate_to_one_survivor() -> None: """Required scenario 4: exact duplicate across agents -> one result.""" diff --git a/tests/unit/test_review_operator_hard_caps.py b/tests/unit/test_review_operator_hard_caps.py index 756ebb4..67fe943 100644 --- a/tests/unit/test_review_operator_hard_caps.py +++ b/tests/unit/test_review_operator_hard_caps.py @@ -88,7 +88,7 @@ def test_operator_caps_are_never_read_from_repo_config_fields() -> None: assert not hasattr(ReviewConfig(), "operator_max_candidates") assert set(ReviewConfig.model_fields) == { - "critic_enabled", "max_candidates", "max_input_tokens_per_candidate", + "critic_enabled", "critic_failure_policy", "max_candidates", "max_input_tokens_per_candidate", "max_output_tokens_per_candidate", "max_total_input_tokens", "max_concurrent_requests", "min_final_confidence", "max_retries", "max_provider_calls", "max_retry_attempts", "max_total_output_tokens", "max_estimated_cost_usd", "max_elapsed_seconds", diff --git a/tests/unit/test_telemetry_domain.py b/tests/unit/test_telemetry_domain.py index 047983f..7719998 100644 --- a/tests/unit/test_telemetry_domain.py +++ b/tests/unit/test_telemetry_domain.py @@ -54,6 +54,14 @@ def test_suppressed_budget_classifies_correctly() -> None: assert outcome is FindingLifecycleOutcome.SUPPRESSED_BUDGET +def test_suppressed_critic_failure_classifies_correctly() -> None: + outcome = classify_lifecycle_outcome( + status=ProposalStatus.SUPPRESSED_CRITIC_FAILURE, + critic_decision=None, + ) + assert outcome is FindingLifecycleOutcome.SUPPRESSED_CRITIC_FAILURE + + def test_accepted_with_no_verdict_is_accepted_final() -> None: outcome = classify_lifecycle_outcome(status=ProposalStatus.ACCEPTED, critic_decision=None) assert outcome is FindingLifecycleOutcome.ACCEPTED_FINAL From 376fd18a061bb8580164891f28e03dc3a61ada07 Mon Sep 17 00:00:00 2001 From: Kadir Eren Altintas Date: Mon, 21 Sep 2026 21:43:42 +0300 Subject: [PATCH 4/7] add idempotent GitHub review status UX --- apps/worker/tasks/process_pull_request.py | 81 +++++------ apps/worker/tasks/publish_review.py | 59 +++++++- apps/worker/tasks/review_pull_request.py | 156 ++++++++++++++++++++++ docs/github-review-ux.md | 30 +++++ docs/onboarding.md | 2 +- docs/production-e2e.md | 3 +- docs/quickstart.md | 3 +- patchfrog/domain/github_check.py | 47 +++++++ patchfrog/github/client.py | 103 ++++++++++++++ patchfrog/ops/doctor.py | 3 +- patchfrog/ops/orchestrator.py | 44 ++++++ patchfrog/publishing/checks.py | 150 +++++++++++++++++++++ tests/integration/test_orchestrator.py | 17 ++- tests/unit/test_github_client_checks.py | 108 +++++++++++++++ tests/unit/test_review_checks.py | 138 +++++++++++++++++++ 15 files changed, 900 insertions(+), 44 deletions(-) create mode 100644 docs/github-review-ux.md create mode 100644 patchfrog/domain/github_check.py create mode 100644 patchfrog/publishing/checks.py create mode 100644 tests/unit/test_github_client_checks.py create mode 100644 tests/unit/test_review_checks.py diff --git a/apps/worker/tasks/process_pull_request.py b/apps/worker/tasks/process_pull_request.py index 53baad5..a0cc372 100644 --- a/apps/worker/tasks/process_pull_request.py +++ b/apps/worker/tasks/process_pull_request.py @@ -25,6 +25,7 @@ from patchfrog.ops import metrics from patchfrog.ops.orchestrator import schedule_pipeline_if_eligible from patchfrog.persistence.database import create_engine, create_session_factory +from patchfrog.publishing.checks import github_check_publisher from patchfrog.services.pull_request_ingestion import ( IngestionOutcome, IngestionOutcomeStatus, @@ -109,44 +110,48 @@ async def _ingest(event: PullRequestWebhookEvent, settings: Settings) -> Ingesti else: outcome = await service.ingest(event) - if outcome.status is IngestionOutcomeStatus.SUCCEEDED and event.action is not PullRequestEventAction.CLOSED: - # Only opened/reopened/synchronize ever reach here -- every - # one of those actions means "there is a commit that should - # be reviewed", so scheduling is unconditional on the action - # itself; patchfrog.ops.eligibility is what actually decides - # whether this specific installation/repository/PR may - # proceed. - # - # This call must never be allowed to propagate: ingestion's - # delivery_id uniqueness constraint means a re-delivered (or - # Celery-retried) webhook for an already-SUCCEEDED ingestion - # is recognized as a DUPLICATE and short-circuits before - # reaching this line again -- so a transient failure here - # (e.g. Redis briefly unreachable) would otherwise leave a - # successfully-ingested PR that silently never gets - # reviewed, undetectable by `ops failed`/`ops stale` (both - # only ever look at `review_runs`, and no such row would - # exist). Caught, logged with everything needed to manually - # recover, and surfaced on the one metric built for exactly - # this shape of outcome instead. - try: - await schedule_pipeline_if_eligible( - session_factory, - settings=settings, - repository_ref=event.repository, - commit_sha=event.head_sha, - pull_request_number=event.pull_request_number, - ) - except Exception as exc: - logger.error( - "pipeline_scheduling_failed", - github_delivery_id=event.delivery_id, - repository=event.repository.full_name, - pull_request_number=event.pull_request_number, - commit_sha=event.head_sha, - error=str(exc), - ) - metrics.reviews_skipped_total.labels(reason="scheduling_failed").inc() + if outcome.status is IngestionOutcomeStatus.SUCCEEDED and event.action is not PullRequestEventAction.CLOSED: + # Only opened/reopened/synchronize ever reach here -- every + # one of those actions means "there is a commit that should + # be reviewed", so scheduling is unconditional on the action + # itself; patchfrog.ops.eligibility is what actually decides + # whether this specific installation/repository/PR may + # proceed. + # + # This call must never be allowed to propagate: ingestion's + # delivery_id uniqueness constraint means a re-delivered (or + # Celery-retried) webhook for an already-SUCCEEDED ingestion + # is recognized as a DUPLICATE and short-circuits before + # reaching this line again -- so a transient failure here + # (e.g. Redis briefly unreachable) would otherwise leave a + # successfully-ingested PR that silently never gets + # reviewed, undetectable by `ops failed`/`ops stale` (both + # only ever look at `review_runs`, and no such row would + # exist). Caught, logged with everything needed to manually + # recover, and surfaced on the one metric built for exactly + # this shape of outcome instead. + try: + await schedule_pipeline_if_eligible( + session_factory, + settings=settings, + repository_ref=event.repository, + commit_sha=event.head_sha, + pull_request_number=event.pull_request_number, + check_publisher=github_check_publisher( + client=github_client, + installation_id=event.repository.installation.id, + ), + ) + except Exception as exc: + logger.error( + "pipeline_scheduling_failed", + github_delivery_id=event.delivery_id, + repository=event.repository.full_name, + pull_request_number=event.pull_request_number, + commit_sha=event.head_sha, + error=str(exc), + ) + metrics.reviews_skipped_total.labels(reason="scheduling_failed").inc() return outcome finally: diff --git a/apps/worker/tasks/publish_review.py b/apps/worker/tasks/publish_review.py index e27a754..0e63ad2 100644 --- a/apps/worker/tasks/publish_review.py +++ b/apps/worker/tasks/publish_review.py @@ -36,13 +36,21 @@ from apps.worker.celery_app import celery_app from patchfrog.config.settings import Settings, get_settings +from patchfrog.domain.pull_request import PullRequestRef from patchfrog.github.auth import InstallationTokenProvider from patchfrog.github.client import GitHubClient +from patchfrog.merge_readiness.service import MergeReadinessService from patchfrog.ops import metrics from patchfrog.persistence.database import create_engine, create_session_factory from patchfrog.persistence.models.pull_request import PullRequestModel from patchfrog.persistence.models.repository import RepositoryModel from patchfrog.persistence.models.review import ReviewRunModel +from patchfrog.persistence.repositories.ai_finding import AIFindingRepository +from patchfrog.publishing.checks import ( + ReviewCheckState, + ReviewCheckUpdate, + github_check_publisher, +) from patchfrog.publishing.config_resolution import resolve_repository_publication_config from patchfrog.publishing.domain import ( ReviewPublicationMode, @@ -119,7 +127,56 @@ async def _publish_review( # alone (see patchfrog.publishing.queries.get_current_active_findings), # so a publish retry/redelivery always recomputes it fresh -- # nothing to pass through here. - return await service.publish(review_run_id=review_run_id, mode=mode, config=config) + result = await service.publish(review_run_id=review_run_id, mode=mode, config=config) + + async with session_factory() as session: + findings = await AIFindingRepository().list_for_run( + session, + review_run_id=review_run_id, + ) + readiness = await MergeReadinessService().evaluate( + session, + repository_id=repository.id, + pull_request_number=pull_request.github_pr_number, + ) + + if result.status is ReviewPublicationStatus.FAILED: + check_state = ReviewCheckState.FAILED + elif result.status is ReviewPublicationStatus.STALE: + check_state = ReviewCheckState.SKIPPED + elif run.status.value == "partial": + check_state = ReviewCheckState.PARTIAL + elif findings: + check_state = ReviewCheckState.COMPLETED_WITH_FINDINGS + else: + check_state = ReviewCheckState.COMPLETED_CLEAN + + try: + await github_check_publisher( + client=github_client, + installation_id=repository.installation_id, + ).reconcile( + ref=PullRequestRef( + owner=repository.owner, + repository=repository.name, + number=pull_request.github_pr_number, + ), + head_sha=run.commit_sha, + update=ReviewCheckUpdate( + state=check_state, + accepted_findings=len(findings), + detail=result.errors[0] if result.errors else result.status.value, + merge_readiness=readiness.decision if readiness is not None else None, + ), + ) + except Exception as exc: + logger.error( + "review_check_reconciliation_failed", + review_run_id=str(review_run_id), + publication_status=result.status.value, + error_type=type(exc).__name__, + ) + return result finally: await engine.dispose() diff --git a/apps/worker/tasks/review_pull_request.py b/apps/worker/tasks/review_pull_request.py index 09e462e..e217466 100644 --- a/apps/worker/tasks/review_pull_request.py +++ b/apps/worker/tasks/review_pull_request.py @@ -40,6 +40,7 @@ from patchfrog.executable_verification.dispatch import VerifierDispatcher from patchfrog.github.auth import InstallationTokenProvider from patchfrog.github.client import GitHubClient +from patchfrog.merge_readiness.service import MergeReadinessService from patchfrog.ops import metrics from patchfrog.ops.errors import classify_exception from patchfrog.persistence.database import create_engine, create_session_factory @@ -48,6 +49,11 @@ PullRequestRepository, RepositoryRepository, ) +from patchfrog.publishing.checks import ( + ReviewCheckState, + ReviewCheckUpdate, + github_check_publisher, +) from patchfrog.review.budget import PricingCatalog from patchfrog.review.config import MalformedReviewConfigError from patchfrog.review.config_resolution import ( @@ -128,7 +134,26 @@ async def _review_pull_request( ref = PullRequestRef(owner=owner, repository=name, number=pull_request_number) current_metadata = await github_client.get_pull_request(installation_id=installation_id, ref=ref) + check_publisher = github_check_publisher( + client=github_client, + installation_id=installation_id, + ) if current_metadata.head_sha != head_sha: + try: + await check_publisher.reconcile( + ref=ref, + head_sha=head_sha, + update=ReviewCheckUpdate( + state=ReviewCheckState.SKIPPED, + detail="Superseded by a newer pull request head.", + ), + ) + except Exception as exc: + logger.error( + "review_check_superseded_publish_failed", + repository=full_name, + error_type=type(exc).__name__, + ) logger.info( "review_skipped_superseded", repository=full_name, @@ -139,6 +164,19 @@ async def _review_pull_request( metrics.reviews_skipped_total.labels(reason="superseded").inc() return None + try: + await check_publisher.reconcile( + ref=ref, + head_sha=head_sha, + update=ReviewCheckUpdate(state=ReviewCheckState.RUNNING), + ) + except Exception as exc: + logger.error( + "review_check_running_publish_failed", + repository=full_name, + error_type=type(exc).__name__, + ) + metrics.reviews_started_total.inc() changed_files = await github_client.list_pull_request_files( installation_id=installation_id, ref=ref @@ -331,6 +369,19 @@ def review_pull_request_task( except Exception as exc: category, _retryable, _detail = classify_exception(exc) metrics.reviews_failed_total.labels(error_category=category.value).inc() + asyncio.run( + _publish_terminal_check( + owner=owner, + name=name, + github_repository_id=github_repository_id, + installation_id=installation_id, + pull_request_number=pull_request_number, + head_sha=head_sha, + state=ReviewCheckState.FAILED, + detail=f"Review failed ({category.value}).", + settings=settings, + ) + ) raise if summary is None: return "skipped: superseded by a newer commit" @@ -344,9 +395,11 @@ def review_pull_request_task( reused_existing_run=summary.reused_existing_run, ) + publication_scheduled = False if summary.status is not ReviewRunStatus.FAILED: if asyncio.run(_publication_allowed(installation_id=installation_id, settings=settings)): publish_review_task.delay(review_run_id=str(summary.run_id), publish=True) + publication_scheduled = True logger.info("publish_scheduled", review_run_id=str(summary.run_id), repository=full_name) else: logger.info( @@ -355,12 +408,115 @@ def review_pull_request_task( repository=full_name, ) + if not publication_scheduled: + state = ( + ReviewCheckState.FAILED + if summary.status is ReviewRunStatus.FAILED + else ReviewCheckState.PARTIAL + if summary.status is ReviewRunStatus.PARTIAL + else ReviewCheckState.COMPLETED_WITH_FINDINGS + if summary.accepted_count + else ReviewCheckState.COMPLETED_CLEAN + ) + asyncio.run( + _publish_terminal_check( + owner=owner, + name=name, + github_repository_id=github_repository_id, + installation_id=installation_id, + pull_request_number=pull_request_number, + head_sha=head_sha, + state=state, + accepted_findings=summary.accepted_count, + detail=( + f"Budget stopped the review: {summary.budget.termination_reason.value}." + if summary.budget is not None and summary.budget.termination_reason is not None + else None + ), + settings=settings, + ) + ) + return ( f"status={summary.status.value} accepted={summary.accepted_count} " f"rejected={summary.rejected_count} reviewed={summary.candidates_reviewed}" ) +async def _publish_terminal_check( + *, + owner: str, + name: str, + github_repository_id: int, + installation_id: int, + pull_request_number: int, + head_sha: str, + state: ReviewCheckState, + settings: Settings, + accepted_findings: int = 0, + detail: str | None = None, +) -> None: + """Best-effort terminal UX; never masks the engine's own outcome.""" + + readiness = None + engine = create_engine(settings.database_url) + try: + session_factory = create_session_factory(engine) + async with session_factory() as session: + repository = await RepositoryRepository().get_by_github_id( + session, + github_repository_id=github_repository_id, + ) + if repository is not None: + readiness = await MergeReadinessService().evaluate( + session, + repository_id=repository.id, + pull_request_number=pull_request_number, + ) + except Exception as exc: + logger.warning( + "review_check_merge_readiness_unavailable", + repository=f"{owner}/{name}", + error_type=type(exc).__name__, + ) + finally: + await engine.dispose() + + try: + async with httpx.AsyncClient(timeout=settings.github_api_timeout_seconds) as http_client: + token_provider = InstallationTokenProvider( + http_client=http_client, + app_id=settings.github_app_id, + private_key=settings.github_private_key, + api_base_url=settings.github_api_base_url, + ) + client = GitHubClient( + http_client=http_client, + token_provider=token_provider, + api_base_url=settings.github_api_base_url, + timeout_seconds=settings.github_api_timeout_seconds, + ) + await github_check_publisher(client=client, installation_id=installation_id).reconcile( + ref=PullRequestRef(owner=owner, repository=name, number=pull_request_number), + head_sha=head_sha, + update=ReviewCheckUpdate( + state=state, + accepted_findings=accepted_findings, + detail=detail, + merge_readiness=readiness.decision if readiness is not None else None, + ), + ) + except Exception as exc: + logger.error( + "review_check_terminal_publish_failed", + repository=f"{owner}/{name}", + pull_request_number=pull_request_number, + head_sha=head_sha, + state=state.value, + error_type=type(exc).__name__, + ) + + async def _publication_allowed(*, installation_id: int, settings: Settings) -> bool: """The beta-specific publication gate (spec sections 11/35): *both* the process-wide kill switch and this specific installation's own diff --git a/docs/github-review-ux.md b/docs/github-review-ux.md new file mode 100644 index 0000000..b1f5088 --- /dev/null +++ b/docs/github-review-ux.md @@ -0,0 +1,30 @@ +# GitHub review lifecycle + +PatchFrog uses one GitHub Check Run named `PatchFrog review` per repository, +pull request, and exact head SHA. The stable external identity is reconciled on +retries, so repeated webhook delivery updates the existing check instead of +creating duplicate comments or checks. A new head receives a new identity; a +superseded old head is completed as skipped. + +The visible lifecycle is: + +| Engine outcome | GitHub Check | +| --- | --- | +| accepted for scheduling | queued | +| review executing | in progress | +| completed with accepted findings | completed; success, neutral, or action required from merge readiness | +| completed with no accepted findings | completed; “No actionable findings were accepted” | +| provider/budget-limited partial run | completed; neutral/partial | +| engine or publication failure | completed; failure | +| superseded/ineligible work | completed; skipped | + +Inline-capable findings remain inline review comments. Valid findings that +cannot map to a GitHub diff line remain in the review summary with their +structured mapping reason; they are never silently discarded. The Check Run +does not duplicate finding text. It reports execution state and the existing +public engine's merge-readiness decision. + +Check Runs and pull-request reviews both use the same installation token, so +the GitHub App identity is preserved. Installations must grant +`checks: write` in addition to `contents: read`, `metadata: read`, and +`pull_requests: write`. No user token or Cloud-only identity is introduced. diff --git a/docs/onboarding.md b/docs/onboarding.md index d24fbef..cafaff5 100644 --- a/docs/onboarding.md +++ b/docs/onboarding.md @@ -181,5 +181,5 @@ and the current free-tier data-policy/quota caveats. Every event PatchFrog reacts to during onboarding (`installation`/`installation_repositories`/`pull_request`) and every GitHub API call during processing works with the App's existing -`contents:read`/`metadata:read`/`pull_requests:write` grant. No new +`contents:read`/`metadata:read`/`pull_requests:write`/`checks:write` grant. No new permission was requested for public-beta readiness. diff --git a/docs/production-e2e.md b/docs/production-e2e.md index 1d7f656..b14e4fe 100644 --- a/docs/production-e2e.md +++ b/docs/production-e2e.md @@ -54,7 +54,8 @@ pull_request event Deliberately minimal -- confirmed live via `GET /app` against the real configured App: -- Permissions: `contents: read`, `metadata: read`, `pull_requests: write` +- Permissions: `contents: read`, `metadata: read`, `pull_requests: write`, + `checks: write` - Subscribed events: `pull_request` only No `issues`, `pull_request_review_comment`, or diff --git a/docs/quickstart.md b/docs/quickstart.md index 9ae0143..7d723a2 100644 --- a/docs/quickstart.md +++ b/docs/quickstart.md @@ -55,7 +55,8 @@ below produce real values for them. - **Webhook secret**: generate a real random value (e.g. `openssl rand -hex 32`) and save it -- this becomes `GITHUB_WEBHOOK_SECRET`. - **Permissions** (repository): `Contents: Read-only`, `Metadata: - Read-only`, `Pull requests: Read and write`. Nothing else. + Read-only`, `Pull requests: Read and write`, `Checks: Read and write`. + Nothing else. - **Subscribe to events**: `Pull request` only. - **Where can this GitHub App be installed?**: "Only on this account" is simplest for a first self-hosted instance. diff --git a/patchfrog/domain/github_check.py b/patchfrog/domain/github_check.py new file mode 100644 index 0000000..838d0d1 --- /dev/null +++ b/patchfrog/domain/github_check.py @@ -0,0 +1,47 @@ +"""Typed GitHub Check Run transport models.""" + +from __future__ import annotations + +from dataclasses import dataclass +from enum import StrEnum + + +class GitHubCheckStatus(StrEnum): + QUEUED = "queued" + IN_PROGRESS = "in_progress" + COMPLETED = "completed" + + +class GitHubCheckConclusion(StrEnum): + ACTION_REQUIRED = "action_required" + FAILURE = "failure" + NEUTRAL = "neutral" + SUCCESS = "success" + SKIPPED = "skipped" + + +@dataclass(frozen=True, slots=True) +class GitHubCheckOutput: + title: str + summary: str + + +@dataclass(frozen=True, slots=True) +class GitHubCheckRun: + id: int + name: str + head_sha: str + external_id: str + status: GitHubCheckStatus + conclusion: GitHubCheckConclusion | None + + +@dataclass(frozen=True, slots=True) +class GitHubCheckRunInput: + name: str + head_sha: str + external_id: str + status: GitHubCheckStatus + output: GitHubCheckOutput + conclusion: GitHubCheckConclusion | None = None + details_url: str | None = None diff --git a/patchfrog/github/client.py b/patchfrog/github/client.py index 798bec1..ed28bc7 100644 --- a/patchfrog/github/client.py +++ b/patchfrog/github/client.py @@ -14,6 +14,12 @@ import httpx from patchfrog.domain.github import InstallationRepositoryStub +from patchfrog.domain.github_check import ( + GitHubCheckConclusion, + GitHubCheckRun, + GitHubCheckRunInput, + GitHubCheckStatus, +) from patchfrog.domain.github_feedback import ( GitHubActor, GitHubActorType, @@ -211,6 +217,46 @@ async def create_pull_request_review( data = await self._post_json(installation_id=installation_id, path=path, json_body=payload) return _parse_submitted_review(data) + async def list_check_runs( + self, *, installation_id: int, ref: PullRequestRef, head_sha: str + ) -> list[GitHubCheckRun]: + path = f"/repos/{ref.owner}/{ref.repository}/commits/{head_sha}/check-runs" + data = await self._get_json( + installation_id=installation_id, + path=path, + params={"check_name": "PatchFrog review", "filter": "all", "per_page": 100}, + ) + if not isinstance(data, dict) or not isinstance(data.get("check_runs"), list): + raise GitHubResponseError("Malformed check-runs response from GitHub") + return [_parse_check_run(item) for item in data["check_runs"]] + + async def create_check_run( + self, *, installation_id: int, ref: PullRequestRef, check: GitHubCheckRunInput + ) -> GitHubCheckRun: + path = f"/repos/{ref.owner}/{ref.repository}/check-runs" + data = await self._post_json( + installation_id=installation_id, + path=path, + json_body=_serialize_check_run(check, include_head_sha=True), + ) + return _parse_check_run(data) + + async def update_check_run( + self, + *, + installation_id: int, + ref: PullRequestRef, + check_run_id: int, + check: GitHubCheckRunInput, + ) -> GitHubCheckRun: + path = f"/repos/{ref.owner}/{ref.repository}/check-runs/{check_run_id}" + data = await self._patch_json( + installation_id=installation_id, + path=path, + json_body=_serialize_check_run(check, include_head_sha=False), + ) + return _parse_check_run(data) + async def list_pull_request_review_comments( self, *, installation_id: int, ref: PullRequestRef ) -> list[GitHubReviewComment]: @@ -408,6 +454,32 @@ async def _post_json( except ValueError as exc: raise GitHubResponseError(f"GitHub returned malformed JSON for {path}") from exc + async def _patch_json( + self, *, installation_id: int, path: str, json_body: dict[str, Any] + ) -> Any: + token = await self._token_provider.get_token(installation_id) + url = f"{self._api_base_url}{path}" + try: + response = await self._http_client.patch( + url, + json=json_body, + headers={ + "Authorization": f"Bearer {token}", + "Accept": "application/vnd.github+json", + "X-GitHub-Api-Version": "2022-11-28", + }, + timeout=self._timeout, + ) + except httpx.TimeoutException as exc: + raise GitHubTimeoutError(f"Timed out calling GitHub API: {path}") from exc + except httpx.HTTPError as exc: + raise GitHubTimeoutError(f"Network error calling GitHub API: {path}") from exc + _raise_for_status(response) + try: + return response.json() + except ValueError as exc: + raise GitHubResponseError(f"GitHub returned malformed JSON for {path}") from exc + def _raise_for_status(response: httpx.Response) -> None: status = response.status_code @@ -501,6 +573,37 @@ def _parse_submitted_review(data: dict[str, Any]) -> GitHubSubmittedReview: raise GitHubResponseError("Malformed review response from GitHub") from exc +def _parse_check_run(data: dict[str, Any]) -> GitHubCheckRun: + try: + conclusion = data.get("conclusion") + return GitHubCheckRun( + id=data["id"], + name=data["name"], + head_sha=data["head_sha"], + external_id=data.get("external_id") or "", + status=GitHubCheckStatus(data["status"]), + conclusion=GitHubCheckConclusion(conclusion) if conclusion is not None else None, + ) + except (KeyError, TypeError, ValueError) as exc: + raise GitHubResponseError("Malformed check-run response from GitHub") from exc + + +def _serialize_check_run(check: GitHubCheckRunInput, *, include_head_sha: bool) -> dict[str, Any]: + payload: dict[str, Any] = { + "name": check.name, + "external_id": check.external_id, + "status": check.status.value, + "output": {"title": check.output.title, "summary": check.output.summary}, + } + if include_head_sha: + payload["head_sha"] = check.head_sha + if check.conclusion is not None: + payload["conclusion"] = check.conclusion.value + if check.details_url is not None: + payload["details_url"] = check.details_url + return payload + + def _parse_actor(data: dict[str, Any] | None) -> GitHubActor: if not data: return GitHubActor(login="", actor_type=GitHubActorType.USER) diff --git a/patchfrog/ops/doctor.py b/patchfrog/ops/doctor.py index 80a45c1..72e8e98 100644 --- a/patchfrog/ops/doctor.py +++ b/patchfrog/ops/doctor.py @@ -237,7 +237,8 @@ def _webhook_route_check() -> DoctorCheck: status=DoctorStatus.PASS, detail=( "expected route: POST /webhooks/github -- GitHub App must subscribe to the `pull_request` event only, " - "with permissions contents:read, metadata:read, pull_requests:write (see docs/quickstart.md)" + "with permissions contents:read, metadata:read, pull_requests:write, checks:write " + "(see docs/quickstart.md)" ), ) diff --git a/patchfrog/ops/orchestrator.py b/patchfrog/ops/orchestrator.py index e033b80..11767f7 100644 --- a/patchfrog/ops/orchestrator.py +++ b/patchfrog/ops/orchestrator.py @@ -22,8 +22,10 @@ from patchfrog.config.settings import Settings from patchfrog.domain.github import RepositoryRef +from patchfrog.domain.pull_request import PullRequestRef from patchfrog.ops.eligibility import EligibilityDecision, check_eligibility from patchfrog.persistence.repositories import RepositoryRepository +from patchfrog.publishing.checks import ReviewCheckPublisher, ReviewCheckState, ReviewCheckUpdate logger = structlog.get_logger(__name__) @@ -35,6 +37,7 @@ async def schedule_pipeline_if_eligible( repository_ref: RepositoryRef, commit_sha: str, pull_request_number: int, + check_publisher: ReviewCheckPublisher | None = None, ) -> EligibilityDecision: """Called once, right after a `pull_request` webhook event is successfully ingested. Decides whether to kick off the @@ -73,6 +76,26 @@ async def schedule_pipeline_if_eligible( reason=decision.reason.value if decision.reason else "unknown", detail=decision.detail, ) + if check_publisher is not None: + try: + await check_publisher.reconcile( + ref=PullRequestRef( + owner=repository_ref.owner, + repository=repository_ref.name, + number=pull_request_number, + ), + head_sha=commit_sha, + update=ReviewCheckUpdate( + state=ReviewCheckState.SKIPPED, + detail=decision.detail, + ), + ) + except Exception as exc: + logger.warning( + "review_check_skip_publish_failed", + repository=repository_ref.full_name, + error_type=type(exc).__name__, + ) return decision # Imported here, not at module level -- patchfrog/ never imports from @@ -81,6 +104,27 @@ async def schedule_pipeline_if_eligible( # so the dependency is scoped to exactly where it's needed. from apps.worker.tasks.run_review_pipeline import run_review_pipeline_task + if check_publisher is not None: + try: + await check_publisher.reconcile( + ref=PullRequestRef( + owner=repository_ref.owner, + repository=repository_ref.name, + number=pull_request_number, + ), + head_sha=commit_sha, + update=ReviewCheckUpdate(state=ReviewCheckState.QUEUED), + ) + except Exception as exc: + # A presentation failure must not discard an otherwise valid + # review job; the running stage retries reconciliation. + logger.warning( + "review_check_queue_publish_failed", + repository=repository_ref.full_name, + pull_request_number=pull_request_number, + error_type=type(exc).__name__, + ) + run_review_pipeline_task.delay( github_repository_id=repository_ref.github_repository_id, owner=repository_ref.owner, diff --git a/patchfrog/publishing/checks.py b/patchfrog/publishing/checks.py new file mode 100644 index 0000000..aa2826c --- /dev/null +++ b/patchfrog/publishing/checks.py @@ -0,0 +1,150 @@ +"""Idempotent GitHub Check Run lifecycle for one PR head. + +This is presentation only: review and merge-readiness decisions continue +to come from the public engine. The stable external id lets a synchronize +retry update the same check instead of creating duplicate comments/checks. +""" + +from __future__ import annotations + +from dataclasses import dataclass +from enum import StrEnum +from typing import Protocol + +from patchfrog.domain.github_check import ( + GitHubCheckConclusion, + GitHubCheckOutput, + GitHubCheckRun, + GitHubCheckRunInput, + GitHubCheckStatus, +) +from patchfrog.domain.pull_request import PullRequestRef +from patchfrog.github.client import GitHubClient +from patchfrog.merge_readiness.domain import MergeReadinessDecision + +CHECK_NAME = "PatchFrog review" + + +class ReviewCheckState(StrEnum): + QUEUED = "queued" + RUNNING = "running" + COMPLETED_WITH_FINDINGS = "completed_with_findings" + COMPLETED_CLEAN = "completed_clean" + PARTIAL = "partial" + FAILED = "failed" + SKIPPED = "skipped" + + +@dataclass(frozen=True, slots=True) +class ReviewCheckUpdate: + state: ReviewCheckState + accepted_findings: int = 0 + detail: str | None = None + merge_readiness: MergeReadinessDecision | None = None + details_url: str | None = None + + +class CheckRunClient(Protocol): + async def list_check_runs(self, *, installation_id: int, ref: PullRequestRef, head_sha: str) -> list[GitHubCheckRun]: ... + + async def create_check_run( + self, *, installation_id: int, ref: PullRequestRef, check: GitHubCheckRunInput + ) -> GitHubCheckRun: ... + + async def update_check_run( + self, *, installation_id: int, ref: PullRequestRef, check_run_id: int, check: GitHubCheckRunInput + ) -> GitHubCheckRun: ... + + +class ReviewCheckPublisher: + def __init__(self, *, client: CheckRunClient, installation_id: int) -> None: + self._client = client + self._installation_id = installation_id + + async def reconcile( + self, + *, + ref: PullRequestRef, + head_sha: str, + update: ReviewCheckUpdate, + ) -> GitHubCheckRun: + check = build_check_input(ref=ref, head_sha=head_sha, update=update) + existing = await self._client.list_check_runs( + installation_id=self._installation_id, + ref=ref, + head_sha=head_sha, + ) + match = next( + (item for item in existing if item.name == CHECK_NAME and item.external_id == check.external_id), + None, + ) + if match is None: + return await self._client.create_check_run( + installation_id=self._installation_id, + ref=ref, + check=check, + ) + return await self._client.update_check_run( + installation_id=self._installation_id, + ref=ref, + check_run_id=match.id, + check=check, + ) + + +def build_check_input( + *, ref: PullRequestRef, head_sha: str, update: ReviewCheckUpdate +) -> GitHubCheckRunInput: + external_id = f"patchfrog-review:{ref.owner}/{ref.repository}:{ref.number}:{head_sha}" + status, conclusion, title, summary = _presentation(update) + return GitHubCheckRunInput( + name=CHECK_NAME, + head_sha=head_sha, + external_id=external_id, + status=status, + conclusion=conclusion, + output=GitHubCheckOutput(title=title, summary=summary), + details_url=update.details_url, + ) + + +def _presentation( + update: ReviewCheckUpdate, +) -> tuple[GitHubCheckStatus, GitHubCheckConclusion | None, str, str]: + detail = update.detail + if update.state is ReviewCheckState.QUEUED: + return GitHubCheckStatus.QUEUED, None, "PatchFrog review queued", detail or "Waiting to start." + if update.state is ReviewCheckState.RUNNING: + return GitHubCheckStatus.IN_PROGRESS, None, "PatchFrog is reviewing this change", detail or "Review is in progress." + if update.state is ReviewCheckState.FAILED: + return GitHubCheckStatus.COMPLETED, GitHubCheckConclusion.FAILURE, "PatchFrog review failed", detail or "The review did not complete." + if update.state is ReviewCheckState.SKIPPED: + return GitHubCheckStatus.COMPLETED, GitHubCheckConclusion.SKIPPED, "PatchFrog review skipped", detail or "This head was not reviewed." + if update.state is ReviewCheckState.PARTIAL: + return GitHubCheckStatus.COMPLETED, GitHubCheckConclusion.NEUTRAL, "PatchFrog review is partial", detail or "Some review work could not complete." + + readiness = update.merge_readiness + if readiness is MergeReadinessDecision.BLOCKED: + conclusion = GitHubCheckConclusion.ACTION_REQUIRED + elif readiness is MergeReadinessDecision.HUMAN_REVIEW_REQUIRED: + conclusion = GitHubCheckConclusion.NEUTRAL + else: + conclusion = GitHubCheckConclusion.SUCCESS + readiness_text = f" Merge readiness: {readiness.value}." if readiness is not None else "" + if update.state is ReviewCheckState.COMPLETED_CLEAN: + return ( + GitHubCheckStatus.COMPLETED, + conclusion, + "PatchFrog found no actionable findings", + (detail or "PatchFrog reviewed this change. No actionable findings were accepted.") + readiness_text, + ) + return ( + GitHubCheckStatus.COMPLETED, + conclusion, + f"PatchFrog accepted {update.accepted_findings} finding(s)", + (detail or "Review completed with actionable findings.") + readiness_text, + ) + + +def github_check_publisher(*, client: GitHubClient, installation_id: int) -> ReviewCheckPublisher: + return ReviewCheckPublisher(client=client, installation_id=installation_id) diff --git a/tests/integration/test_orchestrator.py b/tests/integration/test_orchestrator.py index 9d977d0..0845cd5 100644 --- a/tests/integration/test_orchestrator.py +++ b/tests/integration/test_orchestrator.py @@ -8,7 +8,7 @@ from __future__ import annotations -from typing import Any +from typing import Any, cast import pytest from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker @@ -19,6 +19,7 @@ from patchfrog.ops.orchestrator import schedule_pipeline_if_eligible from patchfrog.persistence.models.installation import InstallationModel from patchfrog.persistence.models.repository import RepositoryModel +from patchfrog.publishing.checks import ReviewCheckPublisher, ReviewCheckState, ReviewCheckUpdate _GITHUB_INSTALLATION_ID = 55667788 _GITHUB_REPOSITORY_ID = 998877 @@ -57,6 +58,14 @@ def _repository_ref() -> RepositoryRef: ) +class _RecordingCheckPublisher: + def __init__(self) -> None: + self.updates: list[ReviewCheckUpdate] = [] + + async def reconcile(self, **kwargs: Any) -> None: + self.updates.append(cast(ReviewCheckUpdate, kwargs["update"])) + + async def test_ineligible_repository_never_enqueues( session_factory: async_sessionmaker[AsyncSession], _stub_celery_delay: list[dict[str, Any]], @@ -69,16 +78,19 @@ async def test_ineligible_repository_never_enqueues( session.add(repo) await session.commit() + checks = _RecordingCheckPublisher() decision = await schedule_pipeline_if_eligible( session_factory, settings=_settings(), repository_ref=_repository_ref(), commit_sha="a" * 40, pull_request_number=1, + check_publisher=cast(ReviewCheckPublisher, checks), ) assert decision.eligible is False assert _stub_celery_delay == [] + assert checks.updates[0].state is ReviewCheckState.SKIPPED async def test_unknown_repository_never_enqueues( @@ -115,12 +127,14 @@ async def test_eligible_repository_enqueues_with_correct_arguments( ) await session.commit() + checks = _RecordingCheckPublisher() decision = await schedule_pipeline_if_eligible( session_factory, settings=_settings(), repository_ref=_repository_ref(), commit_sha="b" * 40, pull_request_number=2, + check_publisher=cast(ReviewCheckPublisher, checks), ) assert decision.eligible is True @@ -130,3 +144,4 @@ async def test_eligible_repository_enqueues_with_correct_arguments( assert call["installation_id"] == _GITHUB_INSTALLATION_ID assert call["commit_sha"] == "b" * 40 assert call["pull_request_number"] == 2 + assert checks.updates[0].state is ReviewCheckState.QUEUED diff --git a/tests/unit/test_github_client_checks.py b/tests/unit/test_github_client_checks.py new file mode 100644 index 0000000..3892ccf --- /dev/null +++ b/tests/unit/test_github_client_checks.py @@ -0,0 +1,108 @@ +from __future__ import annotations + +import json +from collections.abc import AsyncIterator + +import httpx +import pytest +import respx + +from patchfrog.domain.github_check import ( + GitHubCheckConclusion, + GitHubCheckOutput, + GitHubCheckRunInput, + GitHubCheckStatus, +) +from patchfrog.domain.pull_request import PullRequestRef +from patchfrog.github.client import GitHubClient + +API_BASE = "https://api.github.com" +REF = PullRequestRef(owner="octo", repository="repo", number=3) + + +class _TokenProvider: + async def get_token(self, installation_id: int) -> str: + return "token" + + +@pytest.fixture +async def http_client() -> AsyncIterator[httpx.AsyncClient]: + async with httpx.AsyncClient() as client: + yield client + + +def _client(http_client: httpx.AsyncClient) -> GitHubClient: + return GitHubClient( + http_client=http_client, + token_provider=_TokenProvider(), # type: ignore[arg-type] + api_base_url=API_BASE, + ) + + +def _input(status: GitHubCheckStatus, conclusion: GitHubCheckConclusion | None = None) -> GitHubCheckRunInput: + return GitHubCheckRunInput( + name="PatchFrog review", + head_sha="a" * 40, + external_id="stable-id", + status=status, + conclusion=conclusion, + output=GitHubCheckOutput(title="title", summary="summary"), + ) + + +@respx.mock +async def test_create_and_update_check_run_use_checks_api(http_client: httpx.AsyncClient) -> None: + response = { + "id": 91, + "name": "PatchFrog review", + "head_sha": "a" * 40, + "external_id": "stable-id", + "status": "completed", + "conclusion": "success", + } + create = respx.post(f"{API_BASE}/repos/octo/repo/check-runs").mock( + return_value=httpx.Response(201, json=response) + ) + update = respx.patch(f"{API_BASE}/repos/octo/repo/check-runs/91").mock( + return_value=httpx.Response(200, json=response) + ) + client = _client(http_client) + check = _input(GitHubCheckStatus.COMPLETED, GitHubCheckConclusion.SUCCESS) + + await client.create_check_run(installation_id=5, ref=REF, check=check) + await client.update_check_run(installation_id=5, ref=REF, check_run_id=91, check=check) + + create_payload = json.loads(create.calls.last.request.content) + update_payload = json.loads(update.calls.last.request.content) + assert create_payload["head_sha"] == "a" * 40 + assert create_payload["conclusion"] == "success" + assert "head_sha" not in update_payload + + +@respx.mock +async def test_list_check_runs_preserves_external_identity(http_client: httpx.AsyncClient) -> None: + respx.get(f"{API_BASE}/repos/octo/repo/commits/{'a' * 40}/check-runs").mock( + return_value=httpx.Response( + 200, + json={ + "total_count": 1, + "check_runs": [ + { + "id": 91, + "name": "PatchFrog review", + "head_sha": "a" * 40, + "external_id": "stable-id", + "status": "in_progress", + "conclusion": None, + } + ], + }, + ) + ) + checks = await _client(http_client).list_check_runs( + installation_id=5, + ref=REF, + head_sha="a" * 40, + ) + assert checks[0].external_id == "stable-id" + assert checks[0].status is GitHubCheckStatus.IN_PROGRESS diff --git a/tests/unit/test_review_checks.py b/tests/unit/test_review_checks.py new file mode 100644 index 0000000..f478775 --- /dev/null +++ b/tests/unit/test_review_checks.py @@ -0,0 +1,138 @@ +from __future__ import annotations + +from dataclasses import dataclass, field + +from patchfrog.domain.github_check import GitHubCheckRun, GitHubCheckRunInput +from patchfrog.domain.pull_request import PullRequestRef +from patchfrog.merge_readiness.domain import MergeReadinessDecision +from patchfrog.publishing.checks import ( + ReviewCheckPublisher, + ReviewCheckState, + ReviewCheckUpdate, + build_check_input, +) + + +@dataclass +class _FakeCheckClient: + checks: list[GitHubCheckRun] = field(default_factory=list) + creates: list[GitHubCheckRunInput] = field(default_factory=list) + updates: list[tuple[int, GitHubCheckRunInput]] = field(default_factory=list) + + async def list_check_runs( + self, *, installation_id: int, ref: PullRequestRef, head_sha: str + ) -> list[GitHubCheckRun]: + return list(self.checks) + + async def create_check_run( + self, *, installation_id: int, ref: PullRequestRef, check: GitHubCheckRunInput + ) -> GitHubCheckRun: + self.creates.append(check) + created = GitHubCheckRun( + id=41, + name=check.name, + head_sha=check.head_sha, + external_id=check.external_id, + status=check.status, + conclusion=check.conclusion, + ) + self.checks.append(created) + return created + + async def update_check_run( + self, + *, + installation_id: int, + ref: PullRequestRef, + check_run_id: int, + check: GitHubCheckRunInput, + ) -> GitHubCheckRun: + self.updates.append((check_run_id, check)) + return GitHubCheckRun( + id=check_run_id, + name=check.name, + head_sha=check.head_sha, + external_id=check.external_id, + status=check.status, + conclusion=check.conclusion, + ) + + +_REF = PullRequestRef(owner="octo", repository="repo", number=7) +_SHA = "a" * 40 + + +def test_lifecycle_has_distinct_user_visible_states() -> None: + expected = { + ReviewCheckState.QUEUED: ("queued", None), + ReviewCheckState.RUNNING: ("in_progress", None), + ReviewCheckState.COMPLETED_WITH_FINDINGS: ("completed", "action_required"), + ReviewCheckState.COMPLETED_CLEAN: ("completed", "success"), + ReviewCheckState.PARTIAL: ("completed", "neutral"), + ReviewCheckState.FAILED: ("completed", "failure"), + ReviewCheckState.SKIPPED: ("completed", "skipped"), + } + for state, (status, conclusion) in expected.items(): + readiness = ( + MergeReadinessDecision.BLOCKED + if state is ReviewCheckState.COMPLETED_WITH_FINDINGS + else MergeReadinessDecision.READY + ) + check = build_check_input( + ref=_REF, + head_sha=_SHA, + update=ReviewCheckUpdate(state=state, accepted_findings=1, merge_readiness=readiness), + ) + assert check.status.value == status + assert (check.conclusion.value if check.conclusion else None) == conclusion + + +def test_clean_result_is_not_indistinguishable_from_failure() -> None: + clean = build_check_input( + ref=_REF, + head_sha=_SHA, + update=ReviewCheckUpdate(state=ReviewCheckState.COMPLETED_CLEAN), + ) + failed = build_check_input( + ref=_REF, + head_sha=_SHA, + update=ReviewCheckUpdate(state=ReviewCheckState.FAILED, detail="provider unavailable"), + ) + assert "No actionable findings" in clean.output.summary + assert clean.conclusion != failed.conclusion + assert "provider unavailable" in failed.output.summary + + +async def test_same_head_reconciles_existing_check_instead_of_spamming() -> None: + client = _FakeCheckClient() + publisher = ReviewCheckPublisher(client=client, installation_id=99) + await publisher.reconcile( + ref=_REF, + head_sha=_SHA, + update=ReviewCheckUpdate(state=ReviewCheckState.QUEUED), + ) + await publisher.reconcile( + ref=_REF, + head_sha=_SHA, + update=ReviewCheckUpdate(state=ReviewCheckState.COMPLETED_CLEAN), + ) + assert len(client.creates) == 1 + assert len(client.updates) == 1 + assert client.updates[0][0] == 41 + + +async def test_new_head_creates_distinct_check_identity() -> None: + client = _FakeCheckClient() + publisher = ReviewCheckPublisher(client=client, installation_id=99) + await publisher.reconcile( + ref=_REF, + head_sha=_SHA, + update=ReviewCheckUpdate(state=ReviewCheckState.SKIPPED, detail="superseded head"), + ) + await publisher.reconcile( + ref=_REF, + head_sha="b" * 40, + update=ReviewCheckUpdate(state=ReviewCheckState.QUEUED), + ) + assert len(client.creates) == 2 + assert client.creates[0].external_id != client.creates[1].external_id From 531f1cd892940f17cd3daaeab8e87acbd06663d1 Mon Sep 17 00:00:00 2001 From: Kadir Eren Altintas Date: Tue, 22 Sep 2026 23:40:10 +0300 Subject: [PATCH 5/7] add Gemini-free-tier-safe provider rate limiting and retry-after handling Production evidence: PatchFrog's own specialist-role fan-out and bounded retries can exceed Gemini's free-tier per-minute quota on their own, with no external traffic. Adds an opt-in, process-wide sliding-window requests-per-minute limiter (patchfrog/review/rate_limiter.py) wrapping real provider construction in both the production (routing/router.py) and CLI (review/provider_factory.py) paths, gated by a new operator-only PATCHFROG_PROVIDER_RATE_LIMIT_RPM setting (unset = unthrottled, unchanged default). Also teaches call_with_retry to honor a provider-reported retry-after hint (Gemini's RetryInfo.retryDelay, Anthropic/OpenAI's Retry-After header) instead of blind exponential backoff, capped so one delay can't consume a whole run's elapsed-time budget. Co-Authored-By: Claude Sonnet 5 --- .env.example | 8 + patchfrog/config/settings.py | 14 ++ patchfrog/review/provider.py | 39 ++++- patchfrog/review/provider_factory.py | 29 ++-- .../review/providers/anthropic_provider.py | 5 +- patchfrog/review/providers/gemini_provider.py | 33 +++- patchfrog/review/providers/openai_provider.py | 5 +- patchfrog/review/rate_limiter.py | 160 ++++++++++++++++++ patchfrog/review/retry.py | 25 ++- patchfrog/routing/router.py | 37 +++- tests/unit/test_model_router.py | 45 +++++ tests/unit/test_provider_rate_limiter.py | 148 ++++++++++++++++ ...test_review_anthropic_provider_contract.py | 21 ++- .../test_review_gemini_provider_contract.py | 46 ++++- .../test_review_openai_provider_contract.py | 19 ++- tests/unit/test_review_retry_budget.py | 65 ++++++- 16 files changed, 669 insertions(+), 30 deletions(-) create mode 100644 patchfrog/review/rate_limiter.py create mode 100644 tests/unit/test_provider_rate_limiter.py diff --git a/.env.example b/.env.example index 7b961c7..6876a37 100644 --- a/.env.example +++ b/.env.example @@ -45,6 +45,14 @@ GITHUB_API_TIMEOUT_SECONDS=10 # PATCHFROG_PROVIDER_PRICING='{"provider/model":{"input_usd_per_million_tokens":1.0,"output_usd_per_million_tokens":4.0}}' # PATCHFROG_CRITIC_FAILURE_POLICY=hold_for_review # optional; default fail_open # +# Optional requests-per-minute ceiling, keyed like PATCHFROG_PROVIDER_PRICING +# above ("provider/model", falling back to a bare "provider" entry). Every +# reviewer/critic/retry/fallback call for that provider shares one +# process-wide sliding-window limiter -- see patchfrog/review/rate_limiter.py. +# Unset means unthrottled (today's behavior). A Gemini free-tier deployment +# (observed ceiling: 5 requests/minute/project/model) should set: +# PATCHFROG_PROVIDER_RATE_LIMIT_RPM='{"gemini":5}' +# # Only the credential for the provider actually selected above needs to # be set. Never set either of these in .patchfrog.yml or any # repository-controlled file; environment/secret-manager only. Leave diff --git a/patchfrog/config/settings.py b/patchfrog/config/settings.py index 5e66ad2..f612d1e 100644 --- a/patchfrog/config/settings.py +++ b/patchfrog/config/settings.py @@ -142,6 +142,20 @@ class Settings(BaseSettings): provider_pricing: dict[str, dict[str, float]] = Field( default_factory=dict, alias="PATCHFROG_PROVIDER_PRICING" ) + #: Operator-configured requests-per-minute ceiling, keyed the same + #: way as provider_pricing above (``"provider/model"``, falling back + #: to a bare ``"provider"`` entry -- see + #: :func:`patchfrog.review.rate_limiter.resolve_rate_limit_rpm`). + #: Unset (the default) means unthrottled -- exactly today's + #: behavior. A free-tier deployment sets e.g. + #: ``PATCHFROG_PROVIDER_RATE_LIMIT_RPM={"gemini":5}`` to keep every + #: reviewer/critic/retry call for that provider under its quota; see + #: :mod:`patchfrog.review.rate_limiter` for why this lives here + #: (operator/deployment concern) rather than in a repository's + #: ``.patchfrog.yml``. + provider_rate_limit_rpm: dict[str, int] = Field( + default_factory=dict, alias="PATCHFROG_PROVIDER_RATE_LIMIT_RPM" + ) # -- Public beta operational limits (patchfrog.ops) -- # All optional with conservative defaults; never required for diff --git a/patchfrog/review/provider.py b/patchfrog/review/provider.py index 86bd4c4..7be1727 100644 --- a/patchfrog/review/provider.py +++ b/patchfrog/review/provider.py @@ -62,7 +62,22 @@ class ProviderResult: class ProviderError(Exception): - """Base class for every provider failure.""" + """Base class for every provider failure. + + ``retry_after_seconds`` (default ``None``) is an optional, provider- + reported hint for how long to wait before trying again -- Gemini's + ``google.rpc.RetryInfo.retryDelay`` or an HTTP ``Retry-After`` header + (Anthropic/OpenAI). When present, :func:`patchfrog.review.retry.call_with_retry` + honors it instead of its own exponential backoff, since the provider + itself is the authority on how long its rate limit lasts. Always + ``None`` for a fatal error (never retried, so irrelevant) and for a + transient error whose provider didn't report a delay -- exponential + backoff remains the fallback in that case. + """ + + def __init__(self, message: str, *, retry_after_seconds: float | None = None) -> None: + super().__init__(message) + self.retry_after_seconds = retry_after_seconds class ProviderFailureKind(StrEnum): @@ -133,6 +148,28 @@ def indicates_insufficient_quota(value: object) -> bool: ) +def retry_after_seconds_from_http_response(value: object) -> float | None: + """Extract a ``Retry-After`` header (seconds) from an SDK exception + that carries an ``httpx.Response`` on ``.response`` -- Anthropic's + and OpenAI's ``RateLimitError`` both do. Defensive by construction + (only ``getattr``, never an attribute-error): a provider SDK that + doesn't expose ``.response``/``.headers`` this way, or a response + without the header, simply yields ``None`` -- the caller then falls + back to exponential backoff, never a crash.""" + + headers = getattr(getattr(value, "response", None), "headers", None) + if headers is None: + return None + raw = headers.get("retry-after") + if raw is None: + return None + try: + seconds = float(raw) + except (TypeError, ValueError): + return None + return seconds if seconds >= 0 else None + + @dataclass(frozen=True, slots=True) class ProviderIdentity: """The exact provider/model identity a request was served by -- folded diff --git a/patchfrog/review/provider_factory.py b/patchfrog/review/provider_factory.py index 4c31dc3..7e8583b 100644 --- a/patchfrog/review/provider_factory.py +++ b/patchfrog/review/provider_factory.py @@ -20,6 +20,11 @@ from patchfrog.review.providers.anthropic_provider import AnthropicLLMProvider from patchfrog.review.providers.gemini_provider import GeminiLLMProvider from patchfrog.review.providers.openai_provider import OpenAILLMProvider +from patchfrog.review.rate_limiter import ( + RateLimitedProvider, + default_rate_limiter_registry, + resolve_rate_limit_rpm, +) from patchfrog.review.runtime_config import SUPPORTED_PROVIDERS, ReviewRuntimeConfig @@ -59,32 +64,34 @@ def _build(provider: str, model: str, *, settings: Settings, timeout_seconds: fl "(never in .patchfrog.yml) before running a real AI review. " "Use --dry-run to build candidates/context without calling the provider." ) - return AnthropicLLMProvider( + client: LLMProvider = AnthropicLLMProvider( api_key=settings.anthropic_api_key, model=model, timeout_seconds=timeout_seconds ) - if provider == "gemini": + elif provider == "gemini": if not settings.gemini_api_key: raise MissingProviderCredentialsError( "GEMINI_API_KEY is not set. Set it in the environment or a secret store " "(never in .patchfrog.yml) before running a real AI review. " "Use --dry-run to build candidates/context without calling the provider." ) - return GeminiLLMProvider( - api_key=settings.gemini_api_key, model=model, timeout_seconds=timeout_seconds - ) - if provider == "openai": + client = GeminiLLMProvider(api_key=settings.gemini_api_key, model=model, timeout_seconds=timeout_seconds) + elif provider == "openai": if not settings.openai_api_key: raise MissingProviderCredentialsError( "OPENAI_API_KEY is not set. Set it in the environment or a secret store " "(never in .patchfrog.yml) before running a real AI review. " "Use --dry-run to build candidates/context without calling the provider." ) - return OpenAILLMProvider( - api_key=settings.openai_api_key, model=model, timeout_seconds=timeout_seconds + client = OpenAILLMProvider(api_key=settings.openai_api_key, model=model, timeout_seconds=timeout_seconds) + else: + raise ValueError( + f"unsupported review provider: {provider!r} (supported: {', '.join(SUPPORTED_PROVIDERS)})" ) - raise ValueError( - f"unsupported review provider: {provider!r} (supported: {', '.join(SUPPORTED_PROVIDERS)})" - ) + rpm = resolve_rate_limit_rpm(settings.provider_rate_limit_rpm, provider=provider, model=model) + if rpm is None: + return client + limiter = default_rate_limiter_registry().get(provider=provider, model=model, requests_per_minute=rpm) + return RateLimitedProvider(client, limiter) def has_credentials(provider: str, *, settings: Settings) -> bool: diff --git a/patchfrog/review/providers/anthropic_provider.py b/patchfrog/review/providers/anthropic_provider.py index a2e2286..240f019 100644 --- a/patchfrog/review/providers/anthropic_provider.py +++ b/patchfrog/review/providers/anthropic_provider.py @@ -47,6 +47,7 @@ ProviderTransientError, ProviderUsage, indicates_insufficient_quota, + retry_after_seconds_from_http_response, ) #: Anthropic's own SDK already retries connection errors/408/409/429/5xx @@ -99,7 +100,9 @@ async def generate_structured(self, request: ProviderRequest) -> ProviderResult: except anthropic.RateLimitError as exc: if indicates_insufficient_quota(exc): raise ProviderInsufficientQuotaError(f"quota exhausted: {exc}") from exc - raise ProviderRateLimitError(f"rate limited: {exc}") from exc + raise ProviderRateLimitError( + f"rate limited: {exc}", retry_after_seconds=retry_after_seconds_from_http_response(exc) + ) from exc except anthropic.APITimeoutError as exc: raise ProviderTimeoutError(f"timeout: {exc}") from exc except anthropic.APIConnectionError as exc: diff --git a/patchfrog/review/providers/gemini_provider.py b/patchfrog/review/providers/gemini_provider.py index c39747d..4408ad4 100644 --- a/patchfrog/review/providers/gemini_provider.py +++ b/patchfrog/review/providers/gemini_provider.py @@ -75,6 +75,36 @@ indicates_insufficient_quota, ) + +def _parse_retry_delay_seconds(details: object) -> float | None: + """Extract ``google.rpc.RetryInfo.retryDelay`` (e.g. ``"34s"``) from a + Gemini ``ClientError``'s parsed error body, when the API included one + -- Gemini's 429 responses for a per-minute rate limit typically do. + Defensive by construction: any unexpected shape (missing/malformed + ``details``, a non-string delay) simply yields ``None``, so the + caller falls back to exponential backoff rather than raising here.""" + + if not isinstance(details, dict): + return None + error_body = details.get("error", details) + if not isinstance(error_body, dict): + return None + for item in error_body.get("details") or []: + if not isinstance(item, dict): + continue + type_url = item.get("@type") + if not isinstance(type_url, str) or not type_url.endswith("RetryInfo"): + continue + raw_delay = item.get("retryDelay") + if not isinstance(raw_delay, str) or not raw_delay.endswith("s"): + continue + try: + return float(raw_delay[:-1]) + except ValueError: + continue + return None + + _DEFAULT_TIMEOUT_SECONDS = 30.0 #: Disables the SDK's own built-in retries (default 5 attempts with @@ -247,7 +277,8 @@ async def generate_structured(self, request: ProviderRequest) -> ProviderResult: # to stop, not something this adapter can detect alone. if indicates_insufficient_quota(exc): raise ProviderInsufficientQuotaError(f"quota exhausted: {exc}") from exc - raise ProviderRateLimitError(f"rate limited: {exc}") from exc + retry_after = _parse_retry_delay_seconds(getattr(exc, "details", None)) + raise ProviderRateLimitError(f"rate limited: {exc}", retry_after_seconds=retry_after) from exc if exc.code == 404: raise ProviderInvalidModelError(f"model not found: {exc}") from exc raise ProviderFatalError(f"invalid request {exc.code}: {exc}") from exc diff --git a/patchfrog/review/providers/openai_provider.py b/patchfrog/review/providers/openai_provider.py index c10e5d2..11a21d1 100644 --- a/patchfrog/review/providers/openai_provider.py +++ b/patchfrog/review/providers/openai_provider.py @@ -62,6 +62,7 @@ ProviderTransientError, ProviderUsage, indicates_insufficient_quota, + retry_after_seconds_from_http_response, ) _DEFAULT_TIMEOUT_SECONDS = 30.0 @@ -148,7 +149,9 @@ async def generate_structured(self, request: ProviderRequest) -> ProviderResult: except openai.RateLimitError as exc: if indicates_insufficient_quota(exc): raise ProviderInsufficientQuotaError(f"quota exhausted: {exc}") from exc - raise ProviderRateLimitError(f"rate limited: {exc}") from exc + raise ProviderRateLimitError( + f"rate limited: {exc}", retry_after_seconds=retry_after_seconds_from_http_response(exc) + ) from exc except openai.APITimeoutError as exc: raise ProviderTimeoutError(f"timeout: {exc}") from exc except openai.APIConnectionError as exc: diff --git a/patchfrog/review/rate_limiter.py b/patchfrog/review/rate_limiter.py new file mode 100644 index 0000000..9d55ca4 --- /dev/null +++ b/patchfrog/review/rate_limiter.py @@ -0,0 +1,160 @@ +"""Process-wide provider request-rate limiter. + +A free-tier provider quota (e.g. Gemini's observed +``GenerateRequestsPerMinutePerProjectPerModel`` ceiling of 5) is a hard +per-minute cap PatchFrog's own concurrency can blow through on its own, +with no external traffic at all: specialist-role fan-out +(:mod:`patchfrog.review.orchestration` runs reviewer roles concurrently +via ``asyncio.gather``), critic verification, and bounded retries can +each add another provider call within the same second. This module +makes exceeding a configured ceiling structurally impossible for any +call that goes through it: every call acquires a slot from a shared, +per ``(provider, model)`` sliding-window limiter *before* the network +request is made. Acquisition blocks (a single computed ``asyncio.sleep``, +never a poll loop) rather than raising -- a burst is smoothed into legal +spacing, never rejected outright. + +**Scope (v1, honest boundary, not a silent gap)**: state lives in +process memory, keyed by ``(provider, model)`` -- Gemini's own quota +unit -- and is shared by every review run in one worker process for +that process's lifetime. It does not coordinate across multiple worker +processes or machines; a self-hosted deployment running a single +low-concurrency worker against one free-tier project (exactly the case +this exists to protect) gets correct enforcement. A distributed +(Redis-backed) limiter would be a deliberate, separate future addition +for a multi-process deployment, not something this module claims to do. +""" + +from __future__ import annotations + +import asyncio +import time +from collections import deque +from collections.abc import Awaitable, Callable, Mapping + +from patchfrog.review.provider import LLMProvider, ProviderIdentity, ProviderRequest, ProviderResult + +_WINDOW_SECONDS = 60.0 + + +class ProviderRateLimiter: + """Sliding-window limiter: never lets more than ``requests_per_minute`` + calls through in any trailing 60-second window.""" + + def __init__( + self, + *, + requests_per_minute: int, + monotonic: Callable[[], float] = time.monotonic, + sleep: Callable[[float], Awaitable[None]] = asyncio.sleep, + ) -> None: + if requests_per_minute <= 0: + raise ValueError("requests_per_minute must be positive") + self._rpm = requests_per_minute + self._monotonic = monotonic + self._sleep = sleep + self._lock = asyncio.Lock() + self._timestamps: deque[float] = deque() + + @property + def requests_per_minute(self) -> int: + return self._rpm + + async def acquire(self) -> None: + """Block until a slot is available, then record the call. + + Runs the wait *inside* the lock -- deliberately, so concurrent + acquirers are serialized into legal spacing rather than all + waking at once and re-violating the window together (the exact + failure mode a naive "check, release lock, then sleep" version + would have). + """ + + async with self._lock: + self._evict_expired() + if len(self._timestamps) >= self._rpm: + wait = _WINDOW_SECONDS - (self._monotonic() - self._timestamps[0]) + if wait > 0: + await self._sleep(wait) + self._evict_expired() + self._timestamps.append(self._monotonic()) + + def _evict_expired(self) -> None: + cutoff = self._monotonic() - _WINDOW_SECONDS + while self._timestamps and self._timestamps[0] <= cutoff: + self._timestamps.popleft() + + +class RateLimitedProvider: + """Wraps any real :class:`LLMProvider` with a shared + :class:`ProviderRateLimiter`, acquiring a slot before every + ``generate_structured`` call. Reviewer, critic, retry, and fallback + calls all resolve to calls on the same wrapped instance (see + :mod:`patchfrog.review.service`/:mod:`patchfrog.review.orchestration`), + so wrapping once at construction covers every call site without + threading a limiter through the orchestration layer.""" + + def __init__(self, inner: LLMProvider, limiter: ProviderRateLimiter) -> None: + self._inner = inner + self._limiter = limiter + + @property + def identity(self) -> ProviderIdentity: + return self._inner.identity + + async def generate_structured(self, request: ProviderRequest) -> ProviderResult: + await self._limiter.acquire() + return await self._inner.generate_structured(request) + + +class ProviderRateLimiterRegistry: + """Process-wide store of one limiter per ``(provider, model)``.""" + + def __init__(self) -> None: + self._limiters: dict[tuple[str, str], ProviderRateLimiter] = {} + + def get(self, *, provider: str, model: str, requests_per_minute: int) -> ProviderRateLimiter: + key = (provider, model) + limiter = self._limiters.get(key) + if limiter is None or limiter.requests_per_minute != requests_per_minute: + limiter = ProviderRateLimiter(requests_per_minute=requests_per_minute) + self._limiters[key] = limiter + return limiter + + +_default_registry = ProviderRateLimiterRegistry() + + +def default_rate_limiter_registry() -> ProviderRateLimiterRegistry: + """The process-wide registry real provider construction wires + through (see :func:`patchfrog.routing.router._build_provider`). + A test that needs isolation constructs its own + :class:`ProviderRateLimiterRegistry` instead of using this one.""" + + return _default_registry + + +def resolve_rate_limit_rpm(limits: Mapping[str, int], *, provider: str, model: str) -> int | None: + """Look up a configured RPM ceiling for ``provider``/``model``. + + Mirrors :meth:`patchfrog.review.budget.PricingCatalog.key`'s + ``"{provider}/{model}"`` convention: a model-specific entry wins, + falling back to a provider-wide entry, falling back to ``None`` + (unlimited -- no wrapping applied) when neither is configured. Never + a default ceiling of its own: an operator who sets nothing gets + today's unthrottled behavior, unchanged. + """ + + specific = limits.get(f"{provider}/{model}") + if specific is not None: + return specific + return limits.get(provider) + + +__all__ = [ + "ProviderRateLimiter", + "ProviderRateLimiterRegistry", + "RateLimitedProvider", + "default_rate_limiter_registry", + "resolve_rate_limit_rpm", +] diff --git a/patchfrog/review/retry.py b/patchfrog/review/retry.py index e969d39..586e647 100644 --- a/patchfrog/review/retry.py +++ b/patchfrog/review/retry.py @@ -13,6 +13,14 @@ from patchfrog.review.provider import ProviderTransientError +#: Upper bound on any single retry delay, including a provider-reported +#: ``retry_after_seconds`` hint -- a provider that reports (or a bug that +#: computes) an absurdly long delay must never stall a review run for +#: that long; :class:`~patchfrog.review.budget.ReviewBudget`'s own +#: ``max_elapsed_seconds`` ceiling is the real backstop, but capping the +#: delay here keeps one retry from single-handedly consuming most of it. +MAX_RETRY_DELAY_SECONDS = 65.0 + async def call_with_retry[T]( coro_factory: Callable[[], Awaitable[T]], *, max_retries: int, base_delay: float = 0.5 @@ -23,6 +31,16 @@ async def call_with_retry[T]( just reproduce the identical failure. A fatal error therefore never consumes any of the caller's retry allowance. + A transient failure that carries a provider-reported + ``retry_after_seconds`` (see :class:`~patchfrog.review.provider.ProviderError`; + Gemini's ``RetryInfo.retryDelay``, Anthropic/OpenAI's ``Retry-After`` + header) waits exactly that long instead of guessing via exponential + backoff -- the provider is the authority on its own rate-limit + window, so honoring it is strictly more accurate than blind doubling. + Falls back to ``base_delay * 2**attempt`` when a transient failure + didn't report one. Either way the delay is capped at + :data:`MAX_RETRY_DELAY_SECONDS`. + Returns ``(result, retries_used)`` -- the Quality + Cost Guard (:mod:`patchfrog.review.effort`) needs the actual retry count consumed by each candidate for cost/audit accounting, not just the @@ -33,8 +51,11 @@ async def call_with_retry[T]( while True: try: return await coro_factory(), attempt - except ProviderTransientError: + except ProviderTransientError as exc: if attempt >= max_retries: raise - await asyncio.sleep(base_delay * (2**attempt)) + delay = exc.retry_after_seconds + if delay is None: + delay = base_delay * (2**attempt) + await asyncio.sleep(min(max(delay, 0.0), MAX_RETRY_DELAY_SECONDS)) attempt += 1 diff --git a/patchfrog/routing/router.py b/patchfrog/routing/router.py index f78251b..70d666d 100644 --- a/patchfrog/routing/router.py +++ b/patchfrog/routing/router.py @@ -76,6 +76,11 @@ from patchfrog.review.providers.anthropic_provider import AnthropicLLMProvider from patchfrog.review.providers.gemini_provider import GeminiLLMProvider from patchfrog.review.providers.openai_provider import OpenAILLMProvider +from patchfrog.review.rate_limiter import ( + RateLimitedProvider, + default_rate_limiter_registry, + resolve_rate_limit_rpm, +) from patchfrog.review.runtime_config import ( DEFAULT_MODEL_BY_PROVIDER, SUPPORTED_PROVIDERS, @@ -97,12 +102,32 @@ class NoProviderConfiguredError(MissingProviderCredentialsError): def _build_provider(provider: str, model: str, *, settings: Settings, timeout_seconds: float) -> LLMProvider: if provider == "anthropic": - return AnthropicLLMProvider(api_key=settings.anthropic_api_key, model=model, timeout_seconds=timeout_seconds) - if provider == "gemini": - return GeminiLLMProvider(api_key=settings.gemini_api_key, model=model, timeout_seconds=timeout_seconds) - if provider == "openai": - return OpenAILLMProvider(api_key=settings.openai_api_key, model=model, timeout_seconds=timeout_seconds) - raise ValueError(f"unsupported provider: {provider!r}") # pragma: no cover -- filtered out upstream + client: LLMProvider = AnthropicLLMProvider( + api_key=settings.anthropic_api_key, model=model, timeout_seconds=timeout_seconds + ) + elif provider == "gemini": + client = GeminiLLMProvider(api_key=settings.gemini_api_key, model=model, timeout_seconds=timeout_seconds) + elif provider == "openai": + client = OpenAILLMProvider(api_key=settings.openai_api_key, model=model, timeout_seconds=timeout_seconds) + else: + raise ValueError(f"unsupported provider: {provider!r}") # pragma: no cover -- filtered out upstream + return _apply_rate_limit(client, provider=provider, model=model, settings=settings) + + +def _apply_rate_limit(client: LLMProvider, *, provider: str, model: str, settings: Settings) -> LLMProvider: + """Wrap ``client`` in :class:`~patchfrog.review.rate_limiter.RateLimitedProvider` + when the operator configured an RPM ceiling for this provider/model + (``PATCHFROG_PROVIDER_RATE_LIMIT_RPM`` -- see + :mod:`patchfrog.review.rate_limiter`'s module docstring for why this + exists). Unset means unthrottled, unchanged from before this wrap + existed -- this function is a no-op for every deployment that hasn't + opted in.""" + + rpm = resolve_rate_limit_rpm(settings.provider_rate_limit_rpm, provider=provider, model=model) + if rpm is None: + return client + limiter = default_rate_limiter_registry().get(provider=provider, model=model, requests_per_minute=rpm) + return RateLimitedProvider(client, limiter) class ProviderNotAllowedByPolicyError(NoProviderConfiguredError): diff --git a/tests/unit/test_model_router.py b/tests/unit/test_model_router.py index 442cc9e..9105de3 100644 --- a/tests/unit/test_model_router.py +++ b/tests/unit/test_model_router.py @@ -133,6 +133,51 @@ def test_only_one_provider_configured_routes_reviewer_and_critic_to_it(provider: assert plan.critic_provider.identity.model == DEFAULT_MODEL_BY_PROVIDER[provider] +def test_no_rate_limit_configured_leaves_provider_unwrapped() -> None: + router = ModelRouter(settings=_settings(GEMINI_API_KEY="fake-not-real")) + plan = router.route(runtime_config=_runtime_config(provider="gemini"), critic_enabled=False) + + # Unset PATCHFROG_PROVIDER_RATE_LIMIT_RPM must be a true no-op -- + # every pre-existing router test above already asserts isinstance + # against the real adapter class without configuring a rate limit, + # so this only makes the invariant explicit. + assert isinstance(plan.reviewer_providers[AgentRole.CORRECTNESS], GeminiLLMProvider) + + +def test_configured_rate_limit_wraps_reviewer_and_critic_providers() -> None: + from patchfrog.review.rate_limiter import RateLimitedProvider + + router = ModelRouter( + settings=_settings( + GEMINI_API_KEY="fake-not-real", + PATCHFROG_PROVIDER_RATE_LIMIT_RPM={"gemini": 5}, + ) + ) + plan = router.route(runtime_config=_runtime_config(provider="gemini"), critic_enabled=True) + + reviewer = plan.reviewer_providers[AgentRole.CORRECTNESS] + assert isinstance(reviewer, RateLimitedProvider) + assert isinstance(reviewer._inner, GeminiLLMProvider) + # identity still reports the real underlying model -- the wrap must + # be transparent to every caller that only ever reads .identity. + assert reviewer.identity.model == DEFAULT_MODEL_BY_PROVIDER["gemini"] + assert isinstance(plan.critic_provider, RateLimitedProvider) + + +def test_rate_limit_keyed_by_model_does_not_apply_to_a_different_model() -> None: + from patchfrog.review.rate_limiter import RateLimitedProvider + + router = ModelRouter( + settings=_settings( + GEMINI_API_KEY="fake-not-real", + PATCHFROG_PROVIDER_RATE_LIMIT_RPM={"gemini/some-other-model": 5}, + ) + ) + plan = router.route(runtime_config=_runtime_config(provider="gemini"), critic_enabled=False) + + assert not isinstance(plan.reviewer_providers[AgentRole.CORRECTNESS], RateLimitedProvider) + + # -- Multi-provider combinations (items 4-7 of the required matrix) -- diff --git a/tests/unit/test_provider_rate_limiter.py b/tests/unit/test_provider_rate_limiter.py new file mode 100644 index 0000000..f4181fa --- /dev/null +++ b/tests/unit/test_provider_rate_limiter.py @@ -0,0 +1,148 @@ +from __future__ import annotations + +import pytest + +from patchfrog.review.provider import ( + ProviderIdentity, + ProviderRequest, + ProviderResult, + ProviderUsage, +) +from patchfrog.review.rate_limiter import ( + ProviderRateLimiter, + ProviderRateLimiterRegistry, + RateLimitedProvider, + resolve_rate_limit_rpm, +) + + +class _FakeClock: + """Deterministic, manually-advanced monotonic clock -- no real time + ever passes in these tests.""" + + def __init__(self) -> None: + self.now = 0.0 + + def __call__(self) -> float: + return self.now + + def advance(self, seconds: float) -> None: + self.now += seconds + + +class _SleepRecorder: + """Records every requested delay instead of actually sleeping, and + advances the fake clock by that amount -- proves the limiter issues + exactly one computed sleep per throttled acquire, never a poll loop.""" + + def __init__(self, clock: _FakeClock) -> None: + self.clock = clock + self.calls: list[float] = [] + + async def __call__(self, seconds: float) -> None: + self.calls.append(seconds) + self.clock.advance(seconds) + + +async def test_first_n_calls_within_capacity_never_wait() -> None: + clock = _FakeClock() + sleeper = _SleepRecorder(clock) + limiter = ProviderRateLimiter(requests_per_minute=5, monotonic=clock, sleep=sleeper) + + for _ in range(5): + await limiter.acquire() + + assert sleeper.calls == [] + + +async def test_sixth_call_waits_for_the_oldest_slot_to_expire() -> None: + clock = _FakeClock() + sleeper = _SleepRecorder(clock) + limiter = ProviderRateLimiter(requests_per_minute=5, monotonic=clock, sleep=sleeper) + + for _ in range(5): + await limiter.acquire() + await limiter.acquire() + + assert sleeper.calls == [60.0] + + +async def test_never_busy_loops_at_most_one_sleep_per_acquire() -> None: + clock = _FakeClock() + sleeper = _SleepRecorder(clock) + limiter = ProviderRateLimiter(requests_per_minute=2, monotonic=clock, sleep=sleeper) + + sleeps_per_acquire: list[int] = [] + for _ in range(6): + before = len(sleeper.calls) + await limiter.acquire() + sleeps_per_acquire.append(len(sleeper.calls) - before) + + # A polling implementation would call sleep an unbounded number of + # times per acquire (a wait loop). This limiter computes the exact + # wait once and sleeps exactly that long -- never more than one + # sleep call per acquire. + assert all(count <= 1 for count in sleeps_per_acquire) + assert sum(sleeps_per_acquire) >= 1 # capacity 2 with 6 calls must throttle at least once + + +async def test_spacing_calls_across_the_window_never_waits() -> None: + clock = _FakeClock() + sleeper = _SleepRecorder(clock) + limiter = ProviderRateLimiter(requests_per_minute=5, monotonic=clock, sleep=sleeper) + + for _ in range(5): + await limiter.acquire() + clock.advance(60.0) + await limiter.acquire() + + assert sleeper.calls == [] + + +def test_requests_per_minute_must_be_positive() -> None: + with pytest.raises(ValueError): + ProviderRateLimiter(requests_per_minute=0) + + +async def test_registry_shares_one_limiter_per_provider_model() -> None: + registry = ProviderRateLimiterRegistry() + first = registry.get(provider="gemini", model="gemini-3.6-flash", requests_per_minute=5) + second = registry.get(provider="gemini", model="gemini-3.6-flash", requests_per_minute=5) + other_model = registry.get(provider="gemini", model="gemini-2.5-flash", requests_per_minute=5) + + assert first is second + assert first is not other_model + + +def test_resolve_rate_limit_rpm_prefers_model_specific_entry() -> None: + limits = {"gemini": 10, "gemini/gemini-3.6-flash": 5} + assert resolve_rate_limit_rpm(limits, provider="gemini", model="gemini-3.6-flash") == 5 + assert resolve_rate_limit_rpm(limits, provider="gemini", model="some-other-model") == 10 + assert resolve_rate_limit_rpm(limits, provider="openai", model="gpt-x") is None + + +async def test_rate_limited_provider_acquires_before_delegating() -> None: + clock = _FakeClock() + sleeper = _SleepRecorder(clock) + limiter = ProviderRateLimiter(requests_per_minute=1, monotonic=clock, sleep=sleeper) + + order: list[str] = [] + + class _RecordingInner: + identity = ProviderIdentity(provider="gemini", model="gemini-3.6-flash") + + async def generate_structured(self, request: ProviderRequest) -> ProviderResult: + order.append("inner_call") + return ProviderResult(raw_json="{}", usage=ProviderUsage(), latency_ms=1.0) + + wrapped = RateLimitedProvider(_RecordingInner(), limiter) + request = ProviderRequest( + system_prompt="s", user_prompt="u", json_schema={}, schema_name="x", max_output_tokens=10 + ) + + await wrapped.generate_structured(request) + await wrapped.generate_structured(request) + + assert order == ["inner_call", "inner_call"] + assert sleeper.calls == [60.0] + assert wrapped.identity == ProviderIdentity(provider="gemini", model="gemini-3.6-flash") diff --git a/tests/unit/test_review_anthropic_provider_contract.py b/tests/unit/test_review_anthropic_provider_contract.py index 91511b1..cdd6fd9 100644 --- a/tests/unit/test_review_anthropic_provider_contract.py +++ b/tests/unit/test_review_anthropic_provider_contract.py @@ -9,7 +9,12 @@ import pytest import respx -from patchfrog.review.provider import ProviderFatalError, ProviderRequest, ProviderTransientError +from patchfrog.review.provider import ( + ProviderFatalError, + ProviderRateLimitError, + ProviderRequest, + ProviderTransientError, +) from patchfrog.review.providers.anthropic_provider import AnthropicLLMProvider _MESSAGES_URL = "https://api.anthropic.com/v1/messages" @@ -60,6 +65,20 @@ async def test_rate_limit_is_transient() -> None: await _provider().generate_structured(_REQUEST) +@respx.mock +async def test_rate_limit_captures_retry_after_header() -> None: + respx.post(_MESSAGES_URL).mock( + return_value=httpx.Response( + 429, + json={"type": "error", "error": {"type": "rate_limit_error", "message": "slow down"}}, + headers={"retry-after": "7"}, + ) + ) + with pytest.raises(ProviderRateLimitError) as excinfo: + await _provider().generate_structured(_REQUEST) + assert excinfo.value.retry_after_seconds == 7.0 + + @respx.mock async def test_server_error_is_transient() -> None: respx.post(_MESSAGES_URL).mock( diff --git a/tests/unit/test_review_gemini_provider_contract.py b/tests/unit/test_review_gemini_provider_contract.py index 9bcfc6d..4064a31 100644 --- a/tests/unit/test_review_gemini_provider_contract.py +++ b/tests/unit/test_review_gemini_provider_contract.py @@ -13,7 +13,12 @@ import pytest import respx -from patchfrog.review.provider import ProviderFatalError, ProviderRequest, ProviderTransientError +from patchfrog.review.provider import ( + ProviderFatalError, + ProviderRateLimitError, + ProviderRequest, + ProviderTransientError, +) from patchfrog.review.providers.gemini_provider import GeminiLLMProvider _MODEL = "gemini-3.6-flash" @@ -63,8 +68,13 @@ def _success_body( } -def _error_body(*, code: int, status: str, message: str = "error") -> dict[str, object]: - return {"error": {"code": code, "message": message, "status": status}} +def _error_body( + *, code: int, status: str, message: str = "error", details: list[dict[str, object]] | None = None +) -> dict[str, object]: + error: dict[str, object] = {"code": code, "message": message, "status": status} + if details is not None: + error["details"] = details + return {"error": error} async def test_success_returns_text_and_usage() -> None: @@ -97,6 +107,36 @@ async def test_rate_limit_or_quota_is_transient() -> None: await _provider().generate_structured(_REQUEST) +async def test_rate_limit_captures_google_rpc_retry_delay_when_provided() -> None: + with respx.mock: + respx.post(_GENERATE_URL).mock( + return_value=httpx.Response( + 429, + json=_error_body( + code=429, + status="RESOURCE_EXHAUSTED", + message="Resource has been exhausted (e.g. check quota).", + details=[ + {"@type": "type.googleapis.com/google.rpc.RetryInfo", "retryDelay": "34s"}, + ], + ), + ) + ) + with pytest.raises(ProviderRateLimitError) as excinfo: + await _provider().generate_structured(_REQUEST) + assert excinfo.value.retry_after_seconds == 34.0 + + +async def test_rate_limit_without_retry_info_has_no_retry_after() -> None: + with respx.mock: + respx.post(_GENERATE_URL).mock( + return_value=httpx.Response(429, json=_error_body(code=429, status="RESOURCE_EXHAUSTED")) + ) + with pytest.raises(ProviderRateLimitError) as excinfo: + await _provider().generate_structured(_REQUEST) + assert excinfo.value.retry_after_seconds is None + + async def test_server_error_is_transient() -> None: with respx.mock: respx.post(_GENERATE_URL).mock( diff --git a/tests/unit/test_review_openai_provider_contract.py b/tests/unit/test_review_openai_provider_contract.py index 19f3896..1ffa841 100644 --- a/tests/unit/test_review_openai_provider_contract.py +++ b/tests/unit/test_review_openai_provider_contract.py @@ -103,9 +103,11 @@ def _multi_message_body( } -def _handler_returning(status_code: int, json_body: dict[str, object]) -> _Handler: +def _handler_returning( + status_code: int, json_body: dict[str, object], *, headers: dict[str, str] | None = None +) -> _Handler: def handler(request: httpx2.Request) -> httpx2.Response: - return httpx2.Response(status_code, json=json_body) + return httpx2.Response(status_code, json=json_body, headers=headers) return handler @@ -142,6 +144,19 @@ async def test_rate_limit_is_transient() -> None: await provider.generate_structured(_REQUEST) +async def test_rate_limit_captures_retry_after_header() -> None: + provider = _provider( + handler=_handler_returning( + 429, + {"error": {"message": "rate limited", "type": "rate_limit_error"}}, + headers={"retry-after": "3"}, + ) + ) + with pytest.raises(ProviderRateLimitError) as excinfo: + await provider.generate_structured(_REQUEST) + assert excinfo.value.retry_after_seconds == 3.0 + + async def test_insufficient_quota_is_permanent() -> None: provider = _provider( handler=_handler_returning( diff --git a/tests/unit/test_review_retry_budget.py b/tests/unit/test_review_retry_budget.py index 24de5e2..eb3deeb 100644 --- a/tests/unit/test_review_retry_budget.py +++ b/tests/unit/test_review_retry_budget.py @@ -4,9 +4,10 @@ from patchfrog.review.provider import ( ProviderInsufficientQuotaError, + ProviderRateLimitError, ProviderServerError, ) -from patchfrog.review.retry import call_with_retry +from patchfrog.review.retry import MAX_RETRY_DELAY_SECONDS, call_with_retry async def test_insufficient_quota_has_zero_retries() -> None: @@ -33,3 +34,65 @@ async def attempt() -> str: with pytest.raises(ProviderServerError): await call_with_retry(attempt, max_retries=2, base_delay=0) assert calls == 3 + + +async def test_retry_after_hint_is_honored_over_exponential_backoff(monkeypatch: pytest.MonkeyPatch) -> None: + sleep_calls: list[float] = [] + + async def fake_sleep(seconds: float) -> None: + sleep_calls.append(seconds) + + monkeypatch.setattr("patchfrog.review.retry.asyncio.sleep", fake_sleep) + + calls = 0 + + async def attempt() -> str: + nonlocal calls + calls += 1 + if calls == 1: + raise ProviderRateLimitError("rate limited", retry_after_seconds=12.5) + return "ok" + + result, retries = await call_with_retry(attempt, max_retries=2, base_delay=1.0) + assert result == "ok" + assert retries == 1 + assert sleep_calls == [12.5] + + +async def test_retry_after_hint_is_capped(monkeypatch: pytest.MonkeyPatch) -> None: + sleep_calls: list[float] = [] + + async def fake_sleep(seconds: float) -> None: + sleep_calls.append(seconds) + + monkeypatch.setattr("patchfrog.review.retry.asyncio.sleep", fake_sleep) + + async def attempt() -> None: + raise ProviderRateLimitError("rate limited", retry_after_seconds=10_000.0) + + with pytest.raises(ProviderRateLimitError): + await call_with_retry(attempt, max_retries=1, base_delay=1.0) + assert sleep_calls == [MAX_RETRY_DELAY_SECONDS] + + +async def test_missing_retry_after_falls_back_to_exponential_backoff(monkeypatch: pytest.MonkeyPatch) -> None: + sleep_calls: list[float] = [] + + async def fake_sleep(seconds: float) -> None: + sleep_calls.append(seconds) + + monkeypatch.setattr("patchfrog.review.retry.asyncio.sleep", fake_sleep) + + calls = 0 + + async def attempt() -> str: + nonlocal calls + calls += 1 + if calls <= 2: + raise ProviderServerError("temporary") + return "ok" + + result, retries = await call_with_retry(attempt, max_retries=2, base_delay=0.5) + assert result == "ok" + assert retries == 2 + assert sleep_calls == [0.5, 1.0] From a49b8c8aa92d4f7c7e912066063cdbab044d114d Mon Sep 17 00:00:00 2001 From: Kadir Eren Altintas Date: Tue, 22 Sep 2026 23:40:19 +0300 Subject: [PATCH 6/7] enable GitHub review publication for this repository No .patchfrog.yml existed in this repo, so publish.enabled fell back to its safe-by-default False (patchfrog/publishing/config.py) -- production logged review_publish_disabled_by_config on every run. Adds .patchfrog.yml with publish.enabled: true and nothing else (no other publication control weakened). Closes the one gap found while auditing GitHub-publication test coverage: nothing previously exercised PUBLISH mode with publication disabled by config and asserted the explicit SKIPPED_DISABLED outcome (never a silent success) against a fake GitHub publisher. Co-Authored-By: Claude Sonnet 5 --- .patchfrog.yml | 15 ++ .../test_publishing_service_publish_e2e.py | 41 ++++ .../final_beta_readiness/latest-summary.md | 183 ++++++++++++++++++ 3 files changed, 239 insertions(+) create mode 100644 .patchfrog.yml create mode 100644 validation/final_beta_readiness/latest-summary.md diff --git a/.patchfrog.yml b/.patchfrog.yml new file mode 100644 index 0000000..e1ce29d --- /dev/null +++ b/.patchfrog.yml @@ -0,0 +1,15 @@ +# PatchFrog repository review configuration. +# +# Controls review *behavior* only (candidate/token/confidence budgets, +# publication policy) -- never provider, model, credentials, or Cloud +# routing, which stay operator/deployment-controlled (see +# patchfrog/review/runtime_config.py, patchfrog/config/settings.py, and +# CLAUDE.md's "Source-available / Cloud boundary" section). +# +# publish.enabled defaults to false (patchfrog/publishing/config.py) -- +# a repository can opt itself OUT of publication even when an operator +# requests it, but can never opt itself IN just by content in this +# file; that decision is always made by the operator running PatchFrog. +# This repo is the operator's own dogfooding target, so it opts in here. +publish: + enabled: true diff --git a/tests/integration/test_publishing_service_publish_e2e.py b/tests/integration/test_publishing_service_publish_e2e.py index d6a9ea7..527d1ad 100644 --- a/tests/integration/test_publishing_service_publish_e2e.py +++ b/tests/integration/test_publishing_service_publish_e2e.py @@ -64,6 +64,47 @@ async def test_real_publish_writes_exactly_one_github_review( assert find_marker(call.body) == result.publication_id +async def test_publish_mode_with_publication_disabled_by_config_is_explicitly_skipped( + session_factory: async_sessionmaker[AsyncSession], tmp_path: Path +) -> None: + """Production evidence (PR #57/beta-readiness): a repository whose + ``.patchfrog.yml`` ``publish.enabled`` is ``false`` (or unset -- see + :data:`patchfrog.publishing.config.DEFAULT_ENABLED`) must never + silently succeed or silently do nothing indistinguishable from + success when ``PUBLISH`` mode is requested -- it gets an explicit + ``SKIPPED_DISABLED`` status and the fake GitHub publisher is never + called at all.""" + + reviewed = await setup_reviewed_pull_request( + session_factory, + full_name="test/publish-disabled-by-config", + changed_lines=[14], + response_factory=lambda req: scripted_findings_response([finding_json()]), + tmp_root=tmp_path, + ) + assert len(reviewed.findings) >= 1 + + publisher = FakeReviewPublisher( + pull_request=_pr_metadata(number=reviewed.pull_request_number, head_sha=reviewed.commit_sha), + changed_files=reviewed.changed_files, + ) + service = ReviewPublicationService(session_factory=session_factory, publisher=publisher) + + result = await service.publish( + review_run_id=reviewed.review_run_id, + mode=ReviewPublicationMode.PUBLISH, + # enabled defaults to False (patchfrog.publishing.config.DEFAULT_ENABLED) + # -- deliberately not overridden here, exercising the exact + # production default an operator sees before opting a repository in. + config=PublicationConfig(min_severity=Severity.INFO), + ) + + assert result.status is ReviewPublicationStatus.SKIPPED_DISABLED + assert result.github_review_id is None + assert result.published_inline == 0 + assert publisher.publish_calls == [] # never wrote to GitHub + + async def test_retrying_the_same_review_run_after_success_is_idempotent( session_factory: async_sessionmaker[AsyncSession], tmp_path: Path ) -> None: diff --git a/validation/final_beta_readiness/latest-summary.md b/validation/final_beta_readiness/latest-summary.md new file mode 100644 index 0000000..a224ec1 --- /dev/null +++ b/validation/final_beta_readiness/latest-summary.md @@ -0,0 +1,183 @@ +# Final beta-readiness pass (pre-AA) + +Baseline: `main` @ `dcb5fe206e53dc537f9c89b82794907edb364f7d` (Fix critic +false-negative on findings the diff claims are intentional, #62). Branch: +`stabilize/beta-readiness`, already four commits ahead of that baseline +before this pass (`f8fa4cd` CI/repo hygiene, `f2efe48` review cost budgets, +`8a041eb` deterministic reviewer readiness evaluation, `376fd18` idempotent +GitHub review status UX) -- this document covers the audit and the +additional work this pass added on top of those, not a restart. + +## 0. Problem restated + +Production evidence on `kadireren7/patchfrog` (this repository, self- +reviewing itself): the engine reaches publication but logs +`review_publish_disabled_by_config` -- no `.patchfrog.yml` exists in this +repo at all, and `publish.enabled` defaults to `False` +(`patchfrog/publishing/config.py`). Separately, production is Gemini-only +and the Gemini free tier enforces +`GenerateRequestsPerMinutePerProjectPerModel` (observed ceiling: 5), which +PatchFrog's own specialist-role fan-out (`asyncio.gather` in +`patchfrog/review/orchestration.py`) and retries can exceed on their own +with zero external traffic. Four goals: (A) make GitHub publication +actually turn on for this repo, (B) make the small-PR path safe under a +low Gemini RPM budget, (C) get CI trustworthy (no unexplained red Xs), (D) +prove the GitHub publication lifecycle end-to-end with deterministic, +fake-provider tests -- never a live paid-API call. + +## 1. Audit findings (what already existed before this pass) + +- **Goal A**: `patchfrog/publishing/config.py` already has the exact + supported config surface (`publish.enabled`, safe-by-default `False`, + `.patchfrog.yml`/`.patchfrog.yaml`, `yaml.safe_load` only). No code + change was needed to make publication *possible* -- only a config file. + `docs/github-review-ux.md` (added in `376fd18`) and `docs/onboarding.md` + already document the `checks: write` GitHub App permission requirement + correctly. +- **Goal B**: `patchfrog/review/effort.py`'s existing LIGHT tier already + drops the SECURITY role for a small/low-signal candidate, leaving one + reviewer role by default (pre-existing, Quality + Cost Guard, + `QUALITY_COST_POLICY_VERSION`). `patchfrog/review/orchestration.py`'s + `_critique` already returns immediately with zero critic calls when + there is no valid proposal to critique (`to_critique` empty -> + `return proposals, 0, 0, ...`) -- "no candidate, no provider call" + already holds for the critic. `patchfrog/review/provider.py` already + classified `ProviderInsufficientQuotaError` (fatal, never retried) + separately from `ProviderRateLimitError` (transient, bounded-retried). + What did **not** exist: any proactive requests-per-minute ceiling + (nothing stopped concurrent role fan-out from bursting past a 5 RPM + quota), and no provider surfaced or honored a `Retry-After`/ + `retryDelay` hint -- every retry used blind exponential backoff. +- **Goal C**: `.github/workflows/ci.yml` (rewritten in `f8fa4cd`) already + runs a real Postgres service (`PATCHFROG_REQUIRE_POSTGRES=1`, so the DB + suite never silently skips in CI), verifies `semgrep --version` as a + required tool, runs `mypy --strict`, checks Alembic has exactly one + head, and builds both Docker images plus a Celery task-registration + check. No debug/scratch artifacts were found needing cleanup, and no + documentation names a required-check string that could drift from + `ci.yml`'s actual job id (`test`) -- so item 8 (branch-protection/check- + name doc consistency) had nothing to reconcile. Separately (informational, + not actioned): `main` currently has **no branch protection rule + configured at all** (`gh api .../branches/main/protection` -> 404 "Branch + not protected") -- today a red CI X is advisory only, never blocking. Not + changed here: enabling branch protection is a repository-administration + decision with real blast radius (every future PR's merge gate), out of + this pass's narrow scope. +- **Goal D**: extensive existing coverage: `tests/unit/test_review_checks.py` + (clean vs. failed vs. partial vs. superseded Check Run states, idempotent + reconcile on the same head, distinct identity on a new head), + `tests/unit/test_publishing_planner.py` (inline vs. summary-only + mapping), `tests/integration/test_publishing_service_publish_e2e.py` + (real publish writes exactly one review, retry is idempotent), + `tests/integration/test_publishing_idempotency.py` (GitHub write + failure -> `ReviewPublicationStatus.FAILED`, retry-safe), and + `tests/integration/test_ops_eligibility_db.py` (kill switch/suspended + installation/unselected repository/beta-pending all block before any + review starts). The one genuine gap: nothing exercised + `ReviewPublicationService.publish(mode=PUBLISH, config=...)` with + `publish.enabled=False` and asserted it never writes to GitHub. + +## 2. What this pass added + +**Goal A** -- `.patchfrog.yml` at the repo root with `publish.enabled: +true`; verified via `load_publication_config` that it resolves correctly. +No other publication control touched (min_severity/caps/frog_marker/ +post_clean_summary all untouched defaults) -- narrowest safe scope. + +**Goal B** -- new `patchfrog/review/rate_limiter.py`: +`ProviderRateLimiter` (async sliding-window limiter: never more than N +calls in any trailing 60s window; blocks with a single computed +`asyncio.sleep`, never a poll loop), `RateLimitedProvider` (wraps any real +`LLMProvider`, acquiring a slot before every `generate_structured` call -- +reviewer/critic/retry/fallback calls all share one wrapped instance), +`ProviderRateLimiterRegistry` (process-wide, one limiter per +`(provider, model)` -- Gemini's own quota unit). Wired into both real +provider-construction points (`patchfrog/routing/router.py`'s +`_build_provider`, the production path, and `patchfrog/review/ +provider_factory.py`'s `_build`, the CLI path) via a new operator-only +setting, `PATCHFROG_PROVIDER_RATE_LIMIT_RPM` (JSON dict, keyed like the +existing `PATCHFROG_PROVIDER_PRICING`: `"provider/model"` falling back to +`"provider"`) -- unset means unthrottled, unchanged from before. Scope is +explicit and documented, not hidden: single-process in-memory state, so a +multi-process worker pool gets one independent limiter per process, not +one shared across a deployment; correct for the self-hosted single-worker +case this exists to protect, not claimed to be more. + +`patchfrog/review/provider.py`'s `ProviderError` gained an optional +`retry_after_seconds` field; Gemini's adapter now parses +`google.rpc.RetryInfo.retryDelay` out of a 429's error body +(`_parse_retry_delay_seconds`), and Anthropic/OpenAI now read the standard +HTTP `Retry-After` header (`retry_after_seconds_from_http_response`) -- +both defensive-by-construction (malformed/missing -> `None`, never a +crash). `patchfrog/review/retry.py`'s `call_with_retry` now honors that +hint over blind exponential backoff when present, capped at +`MAX_RETRY_DELAY_SECONDS = 65.0` so one absurd delay can't single-handedly +consume a run's `max_elapsed_seconds` budget. + +**Goal C** -- no code changes were needed (see audit above); this pass's +contribution was running every gate for real, against a real Postgres +service, and reconciling the results: +- `ruff check .` -- clean. +- `semgrep --version` / `ruff --version` -- both present. +- `mypy . --strict` -- clean, 660 source files (663 after this pass's + additions). +- `alembic upgrade head` against a real `postgres:16-alpine` container -- + applies cleanly; `alembic heads` -- exactly one (`0032_review_cost_budget`). +- `pytest -q` -- 2523 passed, 0 skipped, 0 failed, run with `.env` + temporarily moved aside (see note below). +- Both Docker images (`api`, `worker`) build clean; Celery task + registration inside the built worker image reports exactly the expected + 9 tasks. +- `python -m patchfrog.cli eval run` (oracle provider, 5-case sanity + subset, same as CI) -- precision 1.0 / recall 1.0 / f1 1.0, zero live + provider calls. + +**Local-environment finding, not a CI bug**: running the suite from this +checkout with the developer's own `.env` present (it sets real +`ANTHROPIC_API_KEY`/`GEMINI_API_KEY`) causes 23 spurious failures -- +`patchfrog.config.settings.Settings.model_config` sets `env_file=".env"` +unconditionally, so pydantic-settings loads it as a fallback source +*underneath* `os.environ` regardless of what the shell has or hasn't +exported, and several tests (`test_provider_factory.py`, +`test_model_router.py`, `test_ops_doctor.py`) assert "credential absent" +behavior that a real `.env` in the repo root silently defeats. GitHub +Actions CI never has a `.env` file, so it never hits this -- confirmed by +re-running the exact same suite with `.env` moved aside (restored +immediately after each run): 0 failures, 2523 passed. Classified as a +local-development-environment artifact of `env_file=".env"` being a +deliberate, working-as-designed convenience for `docker compose`/local +CLI use, not a product regression -- not changed here. + +**Goal D** -- one new test, +`test_publish_mode_with_publication_disabled_by_config_is_explicitly_skipped` +(`tests/integration/test_publishing_service_publish_e2e.py`): calls +`ReviewPublicationService.publish(mode=PUBLISH, config=PublicationConfig())` +(the real production default, `enabled=False`) against a fake GitHub +publisher and asserts `result.status is ReviewPublicationStatus. +SKIPPED_DISABLED`, `github_review_id is None`, and +`publisher.publish_calls == []` -- explicit skip, never a silent success. + +## 3. Cloud boundary + +`patchfrog-cloud` was not touched. Every change in this pass is either +review-behavior config (`.patchfrog.yml`) or engine code that determines +*how PatchFrog reviews code* (rate limiting, retry classification, +publication gating) -- squarely source-available-engine territory per +`docs/product-boundary.md`, never hosted-SaaS lifecycle/accounting/ +GitHub-App-installation-state/deployment config. No reason to cross the +boundary arose. + +## 4. Files changed this pass + +- `.patchfrog.yml` (new) +- `patchfrog/review/rate_limiter.py` (new) +- `patchfrog/review/provider.py`, `patchfrog/review/retry.py`, + `patchfrog/review/provider_factory.py`, `patchfrog/routing/router.py`, + `patchfrog/config/settings.py` +- `patchfrog/review/providers/{gemini,anthropic,openai}_provider.py` +- `.env.example` (documents `PATCHFROG_PROVIDER_RATE_LIMIT_RPM`) +- Tests: `tests/unit/test_provider_rate_limiter.py` (new), + `tests/unit/test_review_retry_budget.py`, + `tests/unit/test_review_{gemini,anthropic,openai}_provider_contract.py`, + `tests/unit/test_model_router.py`, + `tests/integration/test_publishing_service_publish_e2e.py` From 0f4c0d1cfb6e1bce40ce94b3d8d90264f5f9c69e Mon Sep 17 00:00:00 2001 From: Kadir Eren Altintas Date: Wed, 23 Sep 2026 20:01:00 +0300 Subject: [PATCH 7/7] fix semgrep resolution so CI's required-analyzer check passes CI run 35781841389 failed at "Verify required static analyzers": `semgrep --version` crashed with ModuleNotFoundError: pkg_resources. Newer semgrep releases pin mcp==1.29.0. Once mcp 1.30.0 was published, pip kept the newest mcp allowed by our own `mcp>=1.29,<2.0` and backtracked semgrep from 1.177.0 to 1.136.0, the last pre-mcp release. That release depends on opentelemetry-instrumentation 0.46b0, which imports pkg_resources; setuptools 84 (pulled in via `setuptools>=16`) no longer ships it (removed in 82). The Docker images install the same dependency set, so the runtime semgrep analyzer was broken there too. Raise the semgrep floor to 1.173 (opentelemetry 0.58b0, no pkg_resources) so the resolver can no longer select the broken line. The analyzer verification step is unchanged. --- pyproject.toml | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 970853b..9e7c486 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -29,7 +29,13 @@ dependencies = [ # discover() (see patchfrog.analysis.analyzers.cppcheck/clang_tidy) # rather than declared here. "ruff>=0.7,<1.0", - "semgrep>=1.100,<2.0", + # Floor is load-bearing: newer semgrep pins mcp exactly (e.g. + # mcp==1.29.0), so when a newer mcp is released pip otherwise + # backtracks semgrep to a pre-mcp release (<=1.136) whose + # opentelemetry-instrumentation 0.46b0 imports pkg_resources -- + # removed in setuptools>=82 -- and `semgrep` crashes at import. + # 1.173 is the validated line using opentelemetry 0.58b0. + "semgrep>=1.173,<2.0", "PyYAML>=6.0,<7.0", # Executable Verification (Milestone S): pytest is invoked as the # existing-targeted-test verification adapter's own subprocess at