diff --git a/.env.example b/.env.example index 257029e..6876a37 100644 --- a/.env.example +++ b/.env.example @@ -35,6 +35,23 @@ 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}}' +# 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 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/.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/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/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 ec953cb..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,12 @@ 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 ( apply_operator_hard_caps, @@ -62,7 +69,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__) @@ -127,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, @@ -138,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 @@ -176,7 +215,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 +275,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 +309,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(): @@ -310,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" @@ -323,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( @@ -334,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/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/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/deployment.md b/docs/deployment.md index c55c057..b35fce3 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,13 @@ 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_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 fd806f2..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 @@ -311,9 +321,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/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/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/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/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/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/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/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/cli.py b/patchfrog/cli.py index f4ae14e..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, @@ -132,6 +136,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 +160,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 +426,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: @@ -1730,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") @@ -1862,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: @@ -2404,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 6bfc3b2..f612d1e 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.""" @@ -87,6 +89,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 +125,37 @@ 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" + ) + 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" + ) + #: 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 @@ -236,6 +275,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 +285,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/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/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/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/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/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/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/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/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/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/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/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..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__) @@ -54,7 +55,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 +187,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 @@ -212,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 @@ -219,6 +224,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 @@ -231,10 +241,18 @@ 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, "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..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( @@ -110,4 +111,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/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 a4c005d..97dc69a 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 @@ -269,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) @@ -372,3 +377,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..902797e 100644 --- a/patchfrog/review/orchestration.py +++ b/patchfrog/review/orchestration.py @@ -74,7 +74,9 @@ 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_policy import CriticFailurePolicy from patchfrog.review.critic_selection import CriticSelectionInput, CriticSelectionPolicy from patchfrog.review.domain import ( AIReviewFinding, @@ -111,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) @@ -260,6 +263,8 @@ 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, + critic_failure_policy: CriticFailurePolicy = CriticFailurePolicy.FAIL_OPEN, ) -> None: """``reviewer_fallback_providers``/``critic_fallback`` (Milestone U runtime-failover correction): an optional, distinct provider used @@ -292,6 +297,8 @@ 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 + self._critic_failure_policy = critic_failure_policy async def review_candidate( self, @@ -386,21 +393,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 +444,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 +586,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 +782,30 @@ 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), ) + 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 verdict, usage, retries_used, used_fallback, served_by = verdict_outcome retries_consumed += retries_used actual_total += usage.input_tokens @@ -822,13 +869,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..7be1727 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 @@ -61,7 +62,34 @@ 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): + 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): @@ -69,12 +97,78 @@ class ProviderTransientError(ProviderError): 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", + ) + ) + + +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: 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 158088a..240f019 100644 --- a/patchfrog/review/providers/anthropic_provider.py +++ b/patchfrog/review/providers/anthropic_provider.py @@ -34,12 +34,20 @@ import anthropic from patchfrog.review.provider import ( + ProviderAuthenticationError, ProviderFatalError, ProviderIdentity, + ProviderInsufficientQuotaError, + ProviderInvalidModelError, + ProviderRateLimitError, ProviderRequest, ProviderResult, + ProviderServerError, + ProviderTimeoutError, ProviderTransientError, ProviderUsage, + indicates_insufficient_quota, + retry_after_seconds_from_http_response, ) #: Anthropic's own SDK already retries connection errors/408/409/429/5xx @@ -90,15 +98,23 @@ 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}", 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: 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..4408ad4 100644 --- a/patchfrog/review/providers/gemini_provider.py +++ b/patchfrog/review/providers/gemini_provider.py @@ -60,14 +60,51 @@ 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, ) + +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 @@ -229,7 +266,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 +275,17 @@ 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 + 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 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..11a21d1 100644 --- a/patchfrog/review/providers/openai_provider.py +++ b/patchfrog/review/providers/openai_provider.py @@ -49,12 +49,20 @@ import openai from patchfrog.review.provider import ( + ProviderAuthenticationError, ProviderFatalError, ProviderIdentity, + ProviderInsufficientQuotaError, + ProviderInvalidModelError, + ProviderRateLimitError, ProviderRequest, ProviderResult, + ProviderServerError, + ProviderTimeoutError, ProviderTransientError, ProviderUsage, + indicates_insufficient_quota, + retry_after_seconds_from_http_response, ) _DEFAULT_TIMEOUT_SECONDS = 30.0 @@ -139,16 +147,22 @@ 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}", 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: - # 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/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/review/service.py b/patchfrog/review/service.py index 18d8a67..8379224 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 @@ -174,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 @@ -366,6 +378,7 @@ class _CandidateOutcome: __slots__ = ( "calls_by_role", "candidate", + "completed", "context_bundle_id", "context_text", "critic_calls", @@ -386,6 +399,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 +440,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 +500,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 +1056,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 +1110,8 @@ 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, + critic_failure_policy=config.critic_failure_policy, ) async def _process(outcome: _CandidateOutcome) -> None: @@ -1116,13 +1145,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} @@ -1288,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 ) @@ -1358,7 +1423,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 +1473,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 +1486,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 +1518,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 +1914,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 +1990,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..70d666d 100644 --- a/patchfrog/routing/router.py +++ b/patchfrog/routing/router.py @@ -69,16 +69,23 @@ 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 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, ReviewRuntimeConfig, + model_matches_provider_family, ) from patchfrog.routing.capabilities import supports_structured_output from patchfrog.routing.domain import ReviewRoutePlan, RouteReason @@ -95,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): @@ -135,7 +162,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 +195,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 +232,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 +360,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/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/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 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_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_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_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_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/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_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/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_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/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_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_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/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_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_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_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_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_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_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..9105de3 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) -- @@ -83,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) -- @@ -348,7 +443,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_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_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") 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_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_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_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_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 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_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 fe2f958..1ffa841 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" @@ -93,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 @@ -128,7 +140,31 @@ 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_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( + 429, + {"error": {"message": "You exceeded your current quota", "code": "insufficient_quota"}}, + ) + ) + with pytest.raises(ProviderInsufficientQuotaError): await provider.generate_structured(_REQUEST) @@ -136,13 +172,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 +200,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 +208,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 +216,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..67fe943 100644 --- a/tests/unit/test_review_operator_hard_caps.py +++ b/tests/unit/test_review_operator_hard_caps.py @@ -88,9 +88,10 @@ 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", + "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..eb3deeb --- /dev/null +++ b/tests/unit/test_review_retry_budget.py @@ -0,0 +1,98 @@ +from __future__ import annotations + +import pytest + +from patchfrog.review.provider import ( + ProviderInsufficientQuotaError, + ProviderRateLimitError, + ProviderServerError, +) +from patchfrog.review.retry import MAX_RETRY_DELAY_SECONDS, 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 + + +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] 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_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 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 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`