diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c583170..f944b358 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,13 @@ +## v0.9.0 (2026-07-28) + +### Features + +- **criteria**: Async-primary BaseCriterion contract with CheckerMisuseError escalation + ([#60](https://github.com/UiPath/coder_eval/pull/60)) + ## v0.8.10 (2026-07-24) ### Bug Fixes diff --git a/CLAUDE.md b/CLAUDE.md index cc00eadc..82f395b9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -46,7 +46,7 @@ coder_eval/ │ ├── criteria/ # Criterion checker plugins (one file per type) │ ├── __init__.py # CriterionRegistry with auto-discovery -│ ├── base.py # BaseCriterion (incl. default aggregate()) + @handle_criterion_errors +│ ├── base.py # BaseCriterion (async _check_impl_async is primary; sync _check_impl derives from it, or vice versa) + @handle_criterion_errors(_async) │ ├── _classification_aggregate.py # Shared overlay: accuracy / P/R/F1 / confusion matrix │ ├── classification_match.py # File-based label matcher │ ├── command_executed.py @@ -141,7 +141,7 @@ action.yml # Published composite GitHub Action (coder-ev - **Reconciliation message (stream self-reconciles to the turn total)**: The per-message stream consistently under-reports the authoritative turn total — a fixed prompt slice (~512 input tokens on Claude) is billed on no SDK-emitted message, and sub-agent input/cache only partially bubbles up. So `EventCollector.build_turn_record` appends one synthetic `ReconciliationMessage` (`role="reconciliation"`, in the `TranscriptMessage` union) per turn, carrying the per-bucket residual = `token_usage` − Σ(assistant message buckets). The invariant: **summing the four token buckets across `TurnRecord.messages` (assistant + reconciliation) equals `token_usage` exactly**, for both Claude and Codex (Codex's stream is already complete after `_recover_subagent_tool_calls`, so its residual is usually 0 and no entry is emitted). This is what lets the evalboard SUM the message stream as the source of truth instead of reading a separate aggregate ("agent tokens"): `selectTokenTotals` returns the stream sum whenever a reconciliation entry is present, and the timeline renders it as its own row. It is agent-agnostic (booked at the single `EventCollector` seam), carries no cost (cost stays on `token_usage`), and is excluded from generation/turn counts and the cost simulator. The Python `token_usage`/`total_token_usage` aggregate is unchanged and still authoritative for budget/judges/reports. - **sandbox isolation**: Tasks that don't need MCP servers should set `setting_sources: []` in their `agent:` block to isolate the sandbox from the host project's CLAUDE.md and settings. Without this, the host project's CLAUDE.md (often 20 KB+) is injected into every API call, inflating cache-creation tokens and cost significantly. - **Run-time caps (non-criterion enforcement)**: `TaskDefinition.run_limits` (`RunLimits` model) is the single namespace for all run-time caps — `max_turns` / `task_timeout` / `turn_timeout` (structural) and `max_input_tokens` / `max_output_tokens` / `max_total_tokens` / `max_usd` (cumulative budget). Token/USD breaches abort with `FinalStatus.TOKEN_BUDGET_EXCEEDED` or `COST_BUDGET_EXCEEDED` (both `category == "failed"`). Structural caps are set from the CLI via `-D run_limits.max_turns=…` / `-D run_limits.task_timeout=…` / `-D run_limits.turn_timeout=…` (field-merged into `run_limits`); budget caps via `-D run_limits.max_usd=…` etc. or YAML. Layered config uses field-merge — a variant block overrides individual keys without replacing the task's block. -- **Early stop on criterion (opt-in)**: `run_limits.stop_early` (default off) ends a single-shot Claude run early once the run's **armed** criteria are decided, so a raised `max_turns` isn't wasted on the smoke flavor. A criterion is armed by `stop_when: pass|fail|decided|auto`; only criteria that can decide from a partial trajectory may arm (non-empty `live_stop_polarities` ClassVar + `live_verdict` override — currently `skill_triggered`, `command_executed`; CE025 keeps the two consistent). `decided` arms **both** polarities; `auto` arms whichever polarities **this instance** can decide — the value for dataset-fanned criteria whose positive/distractor role flips per row. Stop rule: the pass-stop fires when every **pass-armed** criterion live-passes (fail-armed distractors are not required to pass; zero pass-armed ⇒ never pass-stops); the fail-stop fires on the first fail-armed live-fail but is **deferred while any pass-armed criterion is undecided** — a distractor misfire must not truncate a positive row's recall signal, so the latched misfire fires once the positives resolve (or the run continues to the cap). A fail-stop is therefore verdict-preserving; a pass-stop can miss a *later* distractor misfire, so authoritative P/R/F1 comes from a `stop_early: false` run. Driven by `orchestration/early_stop.py::EarlyStopWatcher` through the Claude agent's cooperative `should_stop` seam (tool-call granularity, no SIGKILL); live verdicts only *trigger* the stop — the standard `check_all` on the frozen trajectory is authoritative. An early-stopped run gates on the **armed subset** (`EvaluationResult.armed_criteria_passed`); a completed run gates on the full set. Every unsupported use is a hard error at resolution (plan *and* run), and a runtime verdict bug **fails open** to a full run. Surfaces: `EarlyStopInfo`, report notes/badges, `stopped_early` run.json rows, `EarlyStopped`/`EarlyStopReason` telemetry dims. Worked rationale: docs/TASK_DEFINITION_GUIDE.md § `stop_early`. Defaults off ⇒ behavior byte-for-behavior unchanged. +- **Early stop on criterion (opt-in)**: `run_limits.stop_early` (default off) ends a single-shot Claude run early once the run's **armed** criteria are decided, so a raised `max_turns` isn't wasted on the smoke flavor. A criterion is armed by `stop_when: pass|fail|decided|auto`; only criteria that can decide from a partial trajectory may arm (non-empty `live_stop_polarities` ClassVar + `live_verdict` override — currently `skill_triggered`, `command_executed`; CE025 keeps the two consistent). `decided` arms **both** polarities; `auto` arms whichever polarities **this instance** can decide — the value for dataset-fanned criteria whose positive/distractor role flips per row. Stop rule: the pass-stop fires when every **pass-armed** criterion live-passes (fail-armed distractors are not required to pass; zero pass-armed ⇒ never pass-stops); the fail-stop fires on the first fail-armed live-fail but is **deferred while any pass-armed criterion is undecided** — a distractor misfire must not truncate a positive row's recall signal, so the latched misfire fires once the positives resolve (or the run continues to the cap). A fail-stop is therefore verdict-preserving; a pass-stop can miss a *later* distractor misfire, so authoritative P/R/F1 comes from a `stop_early: false` run. Driven by `orchestration/early_stop.py::EarlyStopWatcher` through the Claude agent's cooperative `should_stop` seam (tool-call granularity, no SIGKILL); live verdicts only *trigger* the stop — the standard `check_all_async` on the frozen trajectory is authoritative. An early-stopped run gates on the **armed subset** (`EvaluationResult.armed_criteria_passed`); a completed run gates on the full set. Every unsupported use is a hard error at resolution (plan *and* run), and a runtime verdict bug **fails open** to a full run. Surfaces: `EarlyStopInfo`, report notes/badges, `stopped_early` run.json rows, `EarlyStopped`/`EarlyStopReason` telemetry dims. Worked rationale: docs/TASK_DEFINITION_GUIDE.md § `stop_early`. Defaults off ⇒ behavior byte-for-behavior unchanged. ## Success Criteria (14 types) @@ -182,7 +182,7 @@ Per-task (single iteration; simulation mode runs a multi-turn dialog): agent.communicate with execute_with_retry, per-attempt turn_timeout, and on_attempt_error → preserves crashed=True partial TurnRecords on AgentCrashError / TurnTimeoutError) - 2. SuccessChecker.check_all() → List[CriterionResult] + 2. SuccessChecker.check_all_async() → List[CriterionResult] Cleanup: Stop agent, save EvaluationResult, generate reports ``` diff --git a/action.yml b/action.yml index 2e8cf2b9..eebdbaf8 100644 --- a/action.yml +++ b/action.yml @@ -33,7 +33,7 @@ inputs: version: description: coder-eval version to install from PyPI, or "local" to install from the action checkout required: false - default: "0.8.10" # <-- kept in sync with releases by release.yml + default: "0.9.0" # <-- kept in sync with releases by release.yml run-dir: description: Run directory (--run-dir) required: false diff --git a/docs/EXTENDING.md b/docs/EXTENDING.md index c0eb1693..aa3ba39e 100644 --- a/docs/EXTENDING.md +++ b/docs/EXTENDING.md @@ -174,8 +174,35 @@ class MyChecker(BaseCriterion[MyCriterion]): Notes: -- **Do not override `check()`** — it's final and wraps `_check_impl` with error - handling (an exception becomes a score-0.0 result with the error captured). +- **Do not override `check()` / `check_async()`** — both are `@final` and wrap + `_check_impl` / `_check_impl_async` with error handling (an exception becomes + a score-0.0 result with the error captured; a `JudgeInfrastructureError` + escalates instead). +- Implement exactly ONE of `_check_impl` (plain sync — the common case, shown + above) or `_check_impl_async` (genuine async I/O — an async HTTP client or + subprocess bridge; see `llm_judge`/`agent_judge`). Whichever you implement, + `BaseCriterion` derives the other for free (`asyncio.to_thread` / `asyncio.run`), + so there is no need to hand-maintain both. Overriding neither, or overriding + BOTH, raises `TypeError` immediately at class-definition time (a shared + abstract base for a family of checkers that intentionally implements neither + can opt out with the `abstract=True` class keyword — every one of ITS + subclasses is still checked normally). `SuccessChecker.check_all_async` + — the orchestrator's entry point — awaits every `_check_impl_async`-native + checker directly on the event loop instead of pinning a thread, and offloads + everything else to `asyncio.to_thread`. Criteria currently run SEQUENTIALLY + (strictly in declaration order, matching the sync `check_all`); running + multiple judge criteria concurrently is a follow-up. +- If your checker overrides `_check_impl_async`, it MUST NOT do blocking work + (file I/O, subprocess calls) directly on the event loop — that would stall + the orchestrator's own loop for the duration of the call. Offload blocking + calls with `await asyncio.to_thread(...)` (see `llm_judge`/`agent_judge`, + which do this for their sandbox/reference file reads). +- The derived sync bridge (`_check_impl`'s default `asyncio.run(...)` call) can + only run when no event loop is already running — calling the public sync + `check()`/`check_all()` on an async-only checker from inside a running loop + raises `CheckerMisuseError` (escalates, like `JudgeInfrastructureError`) + rather than returning a wrong score. Always reach for `check_async()` / + `check_all_async()` from async code. - Return `score` in `[0.0, 1.0]` — binary criteria use `0.0`/`1.0`; fractional ones anything in between. - For **suite-level metrics** on dataset-backed tasks, override diff --git a/pyproject.toml b/pyproject.toml index 9a522b79..024c3088 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "coder-eval" -version = "0.8.10" +version = "0.9.0" description = "Evaluate, benchmark, and A/B-test AI coding agents (Claude Code, Codex, Gemini/Antigravity) with sandboxed, reproducible YAML task suites." readme = "README.md" license = "Apache-2.0" diff --git a/src/coder_eval/__init__.py b/src/coder_eval/__init__.py index 32e2aa5b..baab5ced 100644 --- a/src/coder_eval/__init__.py +++ b/src/coder_eval/__init__.py @@ -1,3 +1,3 @@ """coder_eval - A framework for evaluating AI coding agents.""" -__version__ = "0.8.10" +__version__ = "0.9.0" diff --git a/src/coder_eval/criteria/agent_judge.py b/src/coder_eval/criteria/agent_judge.py index 88b0b412..20db8dab 100644 --- a/src/coder_eval/criteria/agent_judge.py +++ b/src/coder_eval/criteria/agent_judge.py @@ -12,6 +12,7 @@ from __future__ import annotations +import asyncio import logging from typing import TYPE_CHECKING @@ -96,7 +97,7 @@ class AgentJudgeChecker(BaseCriterion[AgentJudgeCriterion]): criterion_type = "agent_judge" - def _check_impl( + async def _check_impl_async( self, criterion: AgentJudgeCriterion, sandbox: Sandbox, @@ -136,15 +137,22 @@ def _check_impl( # to the prompt envelope (fast verdict, narrow tool surface); when empty, the # judge inspects the sandbox copy via its tools. Both modes compose — the judge # can ``Read`` anything else even when files are pre-attached. - judge_ctx = JudgeContextBuilder( - files=criterion.files, - include_reference=criterion.include_reference, - include_agent_output=criterion.include_agent_output, - include_tool_calls=criterion.include_tool_calls, - include_dialog=criterion.include_dialog, - max_dialog_chars=criterion.max_dialog_chars, - max_file_chars=criterion.max_file_chars, - ).build(sandbox, reference_code, turn_records) + # .build() does synchronous file I/O — offload to a worker thread so it + # doesn't stall the event loop (see llm_judge.py's identical comment). + judge_ctx = await asyncio.to_thread( + JudgeContextBuilder( + files=criterion.files, + include_reference=criterion.include_reference, + include_agent_output=criterion.include_agent_output, + include_tool_calls=criterion.include_tool_calls, + include_dialog=criterion.include_dialog, + max_dialog_chars=criterion.max_dialog_chars, + max_file_chars=criterion.max_file_chars, + ).build, + sandbox, + reference_code, + turn_records, + ) # Mount the reference directory only when the criterion opted into seeing it. # When include_reference=False the judge MUST NOT see the grading material, so @@ -175,7 +183,7 @@ def _check_impl( ) try: - turn = runner.run( + turn = await runner.run_async( user_msg, max_turns=criterion.max_turns, turn_timeout=float(criterion.turn_timeout), diff --git a/src/coder_eval/criteria/base.py b/src/coder_eval/criteria/base.py index 6d6753c2..503c31f1 100644 --- a/src/coder_eval/criteria/base.py +++ b/src/coder_eval/criteria/base.py @@ -1,16 +1,17 @@ """Base criterion checker interface with error handling.""" +import asyncio import logging import os import statistics import traceback -from abc import ABC, abstractmethod -from collections.abc import Callable +from abc import ABC +from collections.abc import Awaitable, Callable from dataclasses import dataclass from functools import wraps -from typing import TYPE_CHECKING, Any, ClassVar, Literal +from typing import TYPE_CHECKING, Any, ClassVar, Concatenate, Literal, ParamSpec, final -from coder_eval.errors import JudgeInfrastructureError +from coder_eval.errors import CheckerMisuseError, JudgeInfrastructureError from coder_eval.models import BaseSuccessCriterion, CriterionAggregate, CriterionResult @@ -45,55 +46,102 @@ class CheckContext: reference_dir: "Path | None" = None -def handle_criterion_errors(func: Callable[..., CriterionResult]) -> Callable[..., CriterionResult]: +# Module-level ParamSpec (rather than the PEP 695 `def f[**P](...)` form ruff's +# UP047 prefers) — CodeQL's Python extractor doesn't yet parse PEP 695 type +# parameters referenced via `P.args`/`P.kwargs` and flags `P` as a potentially +# uninitialized local; a plain `typing.ParamSpec` is unambiguous to both tools. +P = ParamSpec("P") + +# Exceptions that must escalate rather than be captured into a scored-0.0 +# CriterionResult — a judge-infra outage or a checker-contract misuse is not an +# agent failure. Shared by both handle_criterion_errors(_async) wrappers below. +_ESCALATING_EXCEPTIONS: tuple[type[Exception], ...] = (JudgeInfrastructureError, CheckerMisuseError) + + +def _failed_result(owner: Any, criterion: BaseSuccessCriterion, exc: Exception, method: str) -> CriterionResult: + """Build the failed ``CriterionResult`` for a captured (non-escalating) + checker exception, and log it. Shared by the sync/async + ``handle_criterion_errors(_async)`` wrapper tails so the two decorators + differ only in ``def``/``async def`` and ``return``/``return await``. + """ + exc_info = f"{exc.__class__.__name__}: {exc}" + tb = "" + if os.getenv("CODER_EVAL_DEBUG") == "1": + tb = "\n" + "".join(traceback.format_exc(limit=5)) + + criterion_type = criterion.type + logger.error( + f"Error in {owner.__class__.__name__}.{method}() for criterion type '{criterion_type}': {exc_info}", + exc_info=True, # Adds full stack trace to logs + ) + return CriterionResult( + criterion_type=criterion_type, + description=criterion.description, + score=0.0, + details=f"Error during check: {exc_info}{tb}", + error=exc_info, # Include exception type and message + ) + + +def handle_criterion_errors( # noqa: UP047 + func: Callable[Concatenate[Any, BaseSuccessCriterion, P], CriterionResult], +) -> Callable[Concatenate[Any, BaseSuccessCriterion, P], CriterionResult]: """Decorator to handle errors in criterion checkers. Wraps checker methods to catch exceptions and return a failed CriterionResult with error details instead of raising. This is the CENTRALIZED error handling that was in evaluator.py. + + Typed with ``ParamSpec``/``Concatenate`` rather than ``Callable[..., ...]`` + so the decorated method's parameter list (sandbox, reference_code, + turn_records, context) stays visible to callers instead of erasing to + ``(...) -> CriterionResult``. """ @wraps(func) def wrapper( self: Any, criterion: BaseSuccessCriterion, - sandbox: "Sandbox", - reference_code: str | None = None, - turn_records: list["TurnRecord"] | None = None, - context: "CheckContext | None" = None, + *args: P.args, + **kwargs: P.kwargs, ) -> CriterionResult: try: - return func( - self, - criterion, - sandbox, - reference_code, - turn_records=turn_records, - context=context, - ) - except JudgeInfrastructureError: - # Judge infra failure is NOT an agent failure — do not score it 0.0. - # Propagates to Orchestrator.run()'s broad except → FinalStatus.ERROR. + return func(self, criterion, *args, **kwargs) + except _ESCALATING_EXCEPTIONS: + # Judge infra failure / checker-contract misuse is NOT an agent + # failure — do not score it 0.0. Propagates to Orchestrator.run()'s + # broad except → FinalStatus.ERROR. raise except Exception as e: - exc_info = f"{e.__class__.__name__}: {e}" - tb = "" - if os.getenv("CODER_EVAL_DEBUG") == "1": - tb = "\n" + "".join(traceback.format_exc(limit=5)) - - criterion_type = criterion.type - logger.error( - f"Error in {self.__class__.__name__}.check() for criterion type '{criterion_type}': {exc_info}", - exc_info=True, # Adds full stack trace to logs - ) - return CriterionResult( - criterion_type=criterion_type, - description=criterion.description, - score=0.0, - details=f"Error during check: {exc_info}{tb}", - error=exc_info, # Include exception type and message - ) + return _failed_result(self, criterion, e, "check") + + return wrapper + + +def handle_criterion_errors_async( # noqa: UP047 + func: Callable[Concatenate[Any, BaseSuccessCriterion, P], Awaitable[CriterionResult]], +) -> Callable[Concatenate[Any, BaseSuccessCriterion, P], Awaitable[CriterionResult]]: + """Async twin of :func:`handle_criterion_errors`, for ``check_async``. + + Same contract: infra failures escalate, everything else is captured into a + failed ``CriterionResult`` instead of propagating. Same ``ParamSpec``/ + ``Concatenate`` typing rationale applies (see the sync twin's docstring). + """ + + @wraps(func) + async def wrapper( + self: Any, + criterion: BaseSuccessCriterion, + *args: P.args, + **kwargs: P.kwargs, + ) -> CriterionResult: + try: + return await func(self, criterion, *args, **kwargs) + except _ESCALATING_EXCEPTIONS: + raise + except Exception as e: + return _failed_result(self, criterion, e, "check_async") return wrapper @@ -101,11 +149,35 @@ def wrapper( class BaseCriterion[C: BaseSuccessCriterion](ABC): """Abstract base class for all criterion checkers. - Each criterion checker must: - 1. Define `criterion_type` as a ClassVar[str] matching the discriminator - 2. Implement `_check_impl()` with the actual checking logic - - The `check()` method is final and applies centralized error handling. + The checking logic's PRIMARY surface is async (``_check_impl_async``) — + every criterion, at bottom, is "read some inputs, produce a score," and + async is the strictly more general shape: it covers both a criterion that + never awaits anything (a CPU/file-bound check) and one that awaits genuine + I/O (an LLM judge call). A checker implements exactly ONE of the two + ``_check_impl*`` methods — whichever is its natural form — and the base + class derives the other automatically: + + - CPU/file-bound criteria (file_exists, command_executed, ...) override + ``_check_impl`` (plain sync code, no event loop to think about). The + base's default ``_check_impl_async`` offloads it to a worker thread via + ``asyncio.to_thread`` so it never blocks the event loop. + - Criteria that make genuine async I/O (llm_judge, agent_judge) override + ONLY ``_check_impl_async`` (using an async HTTP client / subprocess + bridge) — there is no reason to hand-maintain a second, sync-client + implementation just for the rarely-used direct-sync-call path. The + base's default ``_check_impl`` derives a sync call by running the async + one to completion on a fresh event loop (``asyncio.run``). + + ``__init_subclass__`` enforces that a checker overrides EXACTLY ONE of + the two, at class-definition time — overriding neither would recurse + forever between the defaults (``asyncio.run`` <-> ``asyncio.to_thread``) + the first time either is called, and overriding both would let the two + implementations silently drift into different scores depending on which + entry point (``check`` vs ``check_async``) ran. + + ``check()`` / ``check_async()`` are FINAL — they apply centralized error + handling and must not be overridden; implement ``_check_impl`` / + ``_check_impl_async`` instead. Type parameter C binds the checker to its specific criterion model for better IDE support and static type checking. @@ -135,6 +207,67 @@ def _check_impl( # live_verdict; CE025 enforces that the two stay consistent. live_stop_polarities: ClassVar[frozenset[str]] = frozenset() + def __new__(cls, *args: Any, **kwargs: Any) -> "BaseCriterion[C]": + """Block direct instantiation of ``BaseCriterion`` itself. + + ``__init_subclass__`` below only runs for SUBCLASSES, so with no + ``@abstractmethod`` left on this class (both ``_check_impl*`` methods + have concrete default bodies, by design — that's what lets each derive + the other), plain ``ABCMeta`` no longer blocks ``BaseCriterion()`` + directly. This restores that guarantee without reintroducing an + abstract method that would break the "override at least one" contract. + """ + if cls is BaseCriterion: + raise TypeError("BaseCriterion is abstract and cannot be instantiated directly") + return super().__new__(cls) + + def __init_subclass__(cls, *, abstract: bool = False, **kwargs: Any) -> None: + """Enforce the ``_check_impl`` / ``_check_impl_async`` override contract + at class-definition time (module import), regardless of which entry + point later registers the class — closing the gap where a subclass + registered via ``CriterionRegistry.register`` directly (bypassing the + ``register_criterion`` decorator) escaped the check, and turning the + mutual-recursion failure mode (``asyncio.run`` <-> ``asyncio.to_thread`` + exhausting OS threads) into an immediate, clearly-named ``TypeError``. + + Enforces "exactly one", not just "at least one": overriding BOTH is + also rejected — a checker with two live implementations (sync-path + `_check_impl` and async-path `_check_impl_async`) is free to have them + drift into different scores for identical agent output depending on + which entry point (``check`` vs ``check_async``) happened to run it, + which is exactly the class of bug this derivation design exists to + eliminate. + + Pass ``abstract=True`` on a class that intentionally implements + neither (e.g. a shared abstract base for a family of related + checkers) to opt out of the check for that one class; every one of + ITS subclasses is still checked normally. + """ + super().__init_subclass__(**kwargs) + if abstract: + return + overrides_sync = cls._check_impl is not BaseCriterion._check_impl + overrides_async = cls._check_impl_async is not BaseCriterion._check_impl_async + if not overrides_sync and not overrides_async: + raise TypeError(f"{cls.__name__} must override _check_impl or _check_impl_async") + if overrides_sync and overrides_async: + msg = f"{cls.__name__} must override exactly one of _check_impl / _check_impl_async, not both" + raise TypeError(msg) + + @classmethod + def is_native_async(cls) -> bool: + """Whether this checker class makes genuine async I/O — i.e. overrides + ``_check_impl_async`` itself rather than inheriting the base's + to-thread-wrapped-sync default. + + Public + typed so dispatch code (``SuccessChecker._is_native_async``) + and anything else that needs to classify a checker doesn't have to + reach into ``_check_impl_async`` (a "protected" name) on another + class from a different package. + """ + return cls._check_impl_async is not BaseCriterion._check_impl_async + + @final @handle_criterion_errors def check( self, @@ -147,7 +280,7 @@ def check( """Execute the criterion check with centralized error handling. This method is FINAL - subclasses must NOT override it. - Implement _check_impl() instead. + Implement _check_impl() (or _check_impl_async()) instead. Args: criterion: The specific criterion definition (Pydantic model) @@ -170,7 +303,6 @@ def check( context=context, ) - @abstractmethod def _check_impl( self, criterion: C, @@ -180,10 +312,12 @@ def _check_impl( turn_records: list["TurnRecord"] | None = None, context: "CheckContext | None" = None, ) -> CriterionResult: - """Implement the actual criterion checking logic. + """Sync checking logic. Override this OR ``_check_impl_async`` (not both). - Subclasses override this method, NOT check(). Every checker shares this - uniform signature; non-judge checkers accept ``context`` and ignore it. + Base default: runs ``_check_impl_async`` to completion on a fresh event + loop (``asyncio.run``) — the bridge for checkers whose natural form is + async (they override ``_check_impl_async`` only). Override THIS instead + when the checker's natural form is plain sync CPU/file-bound code. Args: criterion: The specific criterion definition (Pydantic model) @@ -198,9 +332,82 @@ def _check_impl( CriterionResult with score (0.0-1.0), details, and error info Raises: - Any exception - will be caught by @handle_criterion_errors decorator + CheckerMisuseError: this bridge is called from inside a running + event loop (``asyncio.run`` cannot start a nested loop) — this + is a caller mistake (the async-primary surface should have + been awaited instead), not an agent failure, so it escalates + rather than silently scoring 0.0. + Any other exception - will be caught by @handle_criterion_errors """ - pass + try: + asyncio.get_running_loop() + except RuntimeError: + pass + else: + msg = ( + f"{type(self).__name__} implements only _check_impl_async; " + f"call check_async()/check_all_async() from an event loop, not check()/check_all()" + ) + raise CheckerMisuseError(msg) + return asyncio.run( + self._check_impl_async( + criterion, + sandbox, + reference_code, + turn_records=turn_records, + context=context, + ) + ) + + @final + @handle_criterion_errors_async + async def check_async( + self, + criterion: C, + sandbox: "Sandbox", + reference_code: str | None = None, + turn_records: list["TurnRecord"] | None = None, + context: "CheckContext | None" = None, + ) -> CriterionResult: + """Async twin of ``check()``. This method is FINAL - subclasses must NOT + override it. Implement ``_check_impl`` / ``_check_impl_async`` instead. + """ + return await self._check_impl_async( + criterion, + sandbox, + reference_code, + turn_records=turn_records, + context=context, + ) + + async def _check_impl_async( + self, + criterion: C, + sandbox: "Sandbox", + reference_code: str | None = None, + *, + turn_records: list["TurnRecord"] | None = None, + context: "CheckContext | None" = None, + ) -> CriterionResult: + """Async checking logic — the PRIMARY implementation surface. + + Base default: offloads ``_check_impl`` to a worker thread via + ``asyncio.to_thread`` so a CPU/file-bound sync checker never blocks the + event loop. Override THIS instead when the checker makes genuine async + I/O (``llm_judge``, ``agent_judge``) — an async HTTP client / subprocess + bridge that actually yields control while waiting. Leave ``_check_impl`` + unimplemented in that case: the base default derives a sync call from + this one via ``asyncio.run()``, so no second (sync-client) implementation + is needed. + """ + return await asyncio.to_thread( + self._check_impl, + criterion, + sandbox, + reference_code, + turn_records=turn_records, + context=context, + ) def live_verdict( self, @@ -295,6 +502,12 @@ def register_criterion(cls: type[BaseCriterion[Any]]) -> type[BaseCriterion[Any] Moved here from __init__.py to prevent circular import issues. + The ``_check_impl`` / ``_check_impl_async`` override contract is enforced + by ``BaseCriterion.__init_subclass__`` at class-definition time (before + this decorator ever runs), so it applies uniformly regardless of which + entry point registers the class — this decorator, or + ``CriterionRegistry.register`` called directly. + Usage: @register_criterion class MyChecker(BaseCriterion[MyCriterion]): diff --git a/src/coder_eval/criteria/llm_judge.py b/src/coder_eval/criteria/llm_judge.py index 64d0abfa..ac0335d7 100644 --- a/src/coder_eval/criteria/llm_judge.py +++ b/src/coder_eval/criteria/llm_judge.py @@ -1,11 +1,12 @@ """LLM-as-a-judge success criterion checker.""" +import asyncio import logging from typing import TYPE_CHECKING from coder_eval.criteria.base import BaseCriterion, CheckContext, register_criterion -from coder_eval.evaluation.judge_anthropic import invoke_anthropic_judge -from coder_eval.evaluation.judge_bedrock import invoke_bedrock_judge +from coder_eval.evaluation.judge_anthropic import invoke_anthropic_judge_async +from coder_eval.evaluation.judge_bedrock import invoke_bedrock_judge_async from coder_eval.evaluation.judge_context import ( DIALOG_HEADER, JudgeContext, @@ -58,7 +59,7 @@ class LLMJudgeChecker(BaseCriterion[LLMJudgeCriterion]): criterion_type = "llm_judge" - def _check_impl( + async def _check_impl_async( self, criterion: LLMJudgeCriterion, sandbox: "Sandbox", @@ -82,15 +83,23 @@ def _check_impl( details="(skipped: enabled=false)", ) - judge_ctx = JudgeContextBuilder( - files=criterion.files, - include_reference=criterion.include_reference, - include_agent_output=criterion.include_agent_output, - include_tool_calls=criterion.include_tool_calls, - include_dialog=criterion.include_dialog, - max_dialog_chars=criterion.max_dialog_chars, - max_file_chars=criterion.max_file_chars, - ).build(sandbox, reference_code, turn_records) + # .build() does synchronous file I/O (reading sandbox/reference files) — offload + # to a worker thread so it doesn't stall the event loop this checker otherwise + # never blocks (that's the whole point of it being native-async). + judge_ctx = await asyncio.to_thread( + JudgeContextBuilder( + files=criterion.files, + include_reference=criterion.include_reference, + include_agent_output=criterion.include_agent_output, + include_tool_calls=criterion.include_tool_calls, + include_dialog=criterion.include_dialog, + max_dialog_chars=criterion.max_dialog_chars, + max_file_chars=criterion.max_file_chars, + ).build, + sandbox, + reference_code, + turn_records, + ) user_msg = _render_user_message(criterion.prompt, judge_ctx) @@ -116,7 +125,7 @@ def _check_impl( # Attribute the judge's API call to ``JudgeCriterionResult.token_usage`` # from the usage the backend reported in its response. - verdict, parse_error, raw_verdict_text, response_usage = _invoke_tool_channel( + verdict, parse_error, raw_verdict_text, response_usage = await _invoke_tool_channel( criterion=criterion, route=route, system_msg=_SYSTEM_PROMPT, @@ -163,14 +172,14 @@ def _maybe_transcript() -> JudgeTranscript | None: ) -def _invoke_tool_channel( +async def _invoke_tool_channel( *, criterion: LLMJudgeCriterion, route: "ApiRoute | None", system_msg: str, user_msg: str, ) -> tuple[JudgeVerdict | None, str | None, str, TokenUsage | None]: - """Dispatch the tool-channel invocation by route. + """Dispatch the tool-channel invocation by route, via non-blocking async clients. Returns ``(verdict, parse_error, raw_verdict_text, response_usage)``. ``raw_verdict_text`` is the JSON-dumped verdict for the transcript when @@ -182,7 +191,7 @@ def _invoke_tool_channel( response_usage: TokenUsage | None match route: case BedrockRoute(): - response = invoke_bedrock_judge( + response = await invoke_bedrock_judge_async( route=route, model=criterion.model, system=system_msg, @@ -194,7 +203,7 @@ def _invoke_tool_channel( verdict, err = extract_verdict_from_anthropic_response(response) response_usage = token_usage_from_anthropic_dict(response) case DirectRoute(): - anthropic_response = invoke_anthropic_judge( + anthropic_response = await invoke_anthropic_judge_async( model=criterion.model, system=system_msg, user=user_msg, @@ -211,8 +220,8 @@ def _invoke_tool_channel( # keeps the match exhaustive so pyright flags any future route member.) return None, "llm_judge: evaluation route must be Bedrock/Direct, got LiteLLM", "(litellm route)", None case None: - # Handled by the unconfigured-arm guard in _check_impl before dispatch; - # defensive only. + # Handled by the unconfigured-arm guard in _check_impl_async before + # dispatch; defensive only. return None, "llm_judge: no usable API route", "(no route)", None if verdict is not None: diff --git a/src/coder_eval/errors/__init__.py b/src/coder_eval/errors/__init__.py index 1d2484e3..5e52f062 100644 --- a/src/coder_eval/errors/__init__.py +++ b/src/coder_eval/errors/__init__.py @@ -9,6 +9,7 @@ from .agent import AgentConfigError, AgentCrashError, format_timeout_reason, truncate_crash_message from .budget import BudgetExceededError +from .checker_misuse import CheckerMisuseError from .judge import JudgeInfrastructureError from .timeout import EvaluationTimeoutError, TaskTimeoutError, TurnTimeoutError @@ -17,6 +18,7 @@ "AgentConfigError", "AgentCrashError", "BudgetExceededError", + "CheckerMisuseError", "EvaluationTimeoutError", "JudgeInfrastructureError", "TaskTimeoutError", diff --git a/src/coder_eval/errors/checker_misuse.py b/src/coder_eval/errors/checker_misuse.py new file mode 100644 index 00000000..2b3afdd8 --- /dev/null +++ b/src/coder_eval/errors/checker_misuse.py @@ -0,0 +1,13 @@ +"""Typed error for a criterion checker's extension-point contract being misused.""" + + +class CheckerMisuseError(RuntimeError): + """Raised when a checker's checking surface is invoked in a way its contract + forbids — e.g. calling the derived sync ``_check_impl`` (``asyncio.run`` + bridge) on an async-only checker from inside a running event loop. + + Deliberately NOT converted to a scored-0.0 ``CriterionResult``: + ``handle_criterion_errors`` / ``handle_criterion_errors_async`` re-raise it + (same as ``JudgeInfrastructureError``) so a caller's mistake is a loud crash, + not a silently wrong score. + """ diff --git a/src/coder_eval/evaluation/checker.py b/src/coder_eval/evaluation/checker.py index 60515ca0..2e111788 100644 --- a/src/coder_eval/evaluation/checker.py +++ b/src/coder_eval/evaluation/checker.py @@ -5,13 +5,13 @@ the criteria registry. """ +import asyncio import logging from pathlib import Path from typing import TYPE_CHECKING, Any from ..criteria import BaseCriterion, CriterionRegistry, init_criteria -from ..criteria.base import CheckContext -from ..errors import JudgeInfrastructureError +from ..criteria.base import _ESCALATING_EXCEPTIONS, CheckContext from ..models import CriteriaResults, CriterionResult, SuccessCriteria, SuccessCriterion, TurnRecords from ..sandbox import Sandbox @@ -99,6 +99,28 @@ def __init__( if init_registry: init_criteria(validate=validate_registry) + def _resolve_refs( + self, + reference_code: str | None, + turn_records: TurnRecords | None, + reference_dir: Path | None, + ) -> tuple[str | None, TurnRecords | None, Path | None]: + """Persist reference_code / reference_dir / turn_records for subsequent calls + that don't pass them explicitly (backward compat), and resolve the effective + values for THIS call. Shared by ``check`` / ``check_all`` / ``check_all_async`` + so the persist-then-resolve preamble lives in exactly one place. + """ + if reference_code is not None: + self._reference_code = reference_code + if reference_dir is not None: + self._reference_dir = reference_dir + if turn_records is not None: + self._turn_records = turn_records + ref_code = reference_code if reference_code is not None else self._reference_code + ref_dir = reference_dir if reference_dir is not None else self._reference_dir + records = turn_records if turn_records is not None else self._turn_records + return ref_code, records, ref_dir + def check( self, criterion: SuccessCriterion, @@ -120,16 +142,7 @@ def check( Returns: CriterionResult with score """ - # Persist reference_code / reference_dir for subsequent calls (backward compat) - if reference_code is not None: - self._reference_code = reference_code - if reference_dir is not None: - self._reference_dir = reference_dir - if turn_records is not None: - self._turn_records = turn_records - ref_code = reference_code if reference_code is not None else self._reference_code - ref_dir = reference_dir if reference_dir is not None else self._reference_dir - records = turn_records if turn_records is not None else self._turn_records + ref_code, records, ref_dir = self._resolve_refs(reference_code, turn_records, reference_dir) return self._check_single(criterion, ref_code, records, ref_dir) def check_all( @@ -151,22 +164,94 @@ def check_all( Returns: List of criterion results with scores """ - # Persist for subsequent check() calls - if reference_code is not None: - self._reference_code = reference_code - if reference_dir is not None: - self._reference_dir = reference_dir - if turn_records is not None: - self._turn_records = turn_records - ref_code = reference_code if reference_code is not None else self._reference_code - ref_dir = reference_dir if reference_dir is not None else self._reference_dir - records = turn_records if turn_records is not None else self._turn_records - results = [] + ref_code, records, ref_dir = self._resolve_refs(reference_code, turn_records, reference_dir) + return self._check_all_sync(criteria, ref_code, records, ref_dir) + + async def check_all_async( + self, + criteria: SuccessCriteria, + reference_code: str | None = None, + turn_records: TurnRecords | None = None, + reference_dir: Path | None = None, + ) -> CriteriaResults: + """Async twin of ``check_all`` — the orchestrator's entry point. + + Runs every criterion SEQUENTIALLY, strictly in declaration order — + the same order/isolation guarantee ``check_all`` provides. A checker + is "native async" when it overrides ``_check_impl_async`` itself (see + ``BaseCriterion``) — that's the criteria making genuine async I/O + (``llm_judge``, ``agent_judge``); those are awaited directly on the + event loop instead of pinning a thread-pool thread for the network + wait. Everything else (CPU/file-bound criteria that only override the + sync ``_check_impl``) is offloaded to a worker thread via + ``asyncio.to_thread`` so it doesn't block the loop either. Neither of + those is about concurrency between criteria — each criterion is fully + awaited before the next one starts, exactly like ``check_all``. + + Concurrent dispatch of adjacent judge criteria (the actual GH #55 + motivation — firing multiple judges' LLM calls at once instead of + serializing them) is DELIBERATELY NOT done here; that scheduling + change is scoped to a follow-up PR so it can be reviewed (and its + sandbox-mutation-ordering implications tested) on its own. This + method exists as the async entry point the orchestrator now calls + unconditionally, with sequential semantics identical to ``check_all`` + in the meantime. + + Args: + criteria: List of criterion definitions. + reference_code: Optional reference code (string form). + turn_records: Optional turn records for command inspection. + reference_dir: Optional resolved path to a reference directory. + Only consumed by ``agent_judge``; non-judge criteria ignore it. + + Returns: + List of criterion results with scores, in the same order as ``criteria``. + """ + ref_code, records, ref_dir = self._resolve_refs(reference_code, turn_records, reference_dir) + + results: list[CriterionResult] = [] for criterion in criteria: - result = self._check_single(criterion, ref_code, records, ref_dir) - results.append(result) + if self._is_native_async(criterion.type): + results.append(await self._check_single_async(criterion, ref_code, records, ref_dir)) + else: + results.append(await asyncio.to_thread(self._check_single, criterion, ref_code, records, ref_dir)) return results + def _is_native_async(self, criterion_type: str) -> bool: + """Whether this criterion type's checker makes genuine async I/O. + + Delegates to the checker class's own public ``is_native_async()`` + classmethod (see ``BaseCriterion``) rather than comparing + ``_check_impl_async`` identity here — that keeps the sync/async + classification part of the documented, typed plugin contract instead + of this module reaching into another package's protected member. An + unregistered type resolves to False so its ``KeyError`` surfaces + through the shared sync error-handling path in ``_check_single`` / + ``_check_single_async``. + + Reads the registered CLASS (``CriterionRegistry.get_checker``), not a + constructed instance — this classification runs in + ``check_all_async`` before any per-criterion error boundary, so it must + never invoke a checker's ``__init__`` (a constructor failure would + otherwise escape ``check_all_async`` entirely instead of scoring that + one criterion 0, which is the contract ``_check_single`` / + ``_check_single_async`` provide). + """ + try: + checker_class = CriterionRegistry.get_checker(criterion_type) + except KeyError: + return False + return checker_class.is_native_async() + + def _check_all_sync( + self, + criteria: SuccessCriteria, + reference_code: str | None, + turn_records: TurnRecords | None, + reference_dir: Path | None, + ) -> CriteriaResults: + return [self._check_single(criterion, reference_code, turn_records, reference_dir) for criterion in criteria] + def _get_checker_instance(self, criterion_type: str) -> BaseCriterion[Any]: """Get or create a checker instance (V3: cached). @@ -181,6 +266,68 @@ def _get_checker_instance(self, criterion_type: str) -> BaseCriterion[Any]: self._checker_instances[criterion_type] = checker_class() return self._checker_instances[criterion_type] + def _finalize_result(self, criterion: SuccessCriterion, result: CriterionResult) -> CriterionResult: + """Stamp pass_threshold/gating onto a checker's result and log it. Shared + by the sync/async success paths in ``_check_single`` / ``_check_single_async`` + so the two only differ in how they invoke the checker.""" + criterion_type = criterion.type + result.pass_threshold = criterion.pass_threshold + result.gating = criterion.is_gating + + logger.debug(f"Criterion '{criterion_type}' score: {result.score:.2f}") + if result.score < criterion.pass_threshold: + # An informational (weight: 0) criterion cannot fail the task, so + # logging it as FAILED contradicts the final status. Say what it is. + logger.info( + "Criterion '%s' %s (score=%.2f, threshold=%.2f): %s", + criterion_type, + "FAILED" if criterion.is_gating else "below threshold (informational)", + result.score, + criterion.pass_threshold, + _short_failure_reason(result), + ) + return result + + def _missing_checker_result(self, criterion: SuccessCriterion) -> CriterionResult: + """Failed result for an unregistered criterion type. Shared by the sync/async + KeyError branches in ``_check_single`` / ``_check_single_async``.""" + criterion_type = criterion.type + # ERROR record carries enough context — skip the companion FAILED line. + logger.error(f"No checker found for criterion type '{criterion_type}'") + return CriterionResult( + criterion_type=criterion_type, + description=criterion.description, + score=0.0, + details=f"No checker registered for criterion type '{criterion_type}'", + error=f"Unsupported criterion type: '{criterion_type}'", + pass_threshold=criterion.pass_threshold, + gating=criterion.is_gating, + ) + + def _error_result(self, criterion: SuccessCriterion, exc: Exception) -> CriterionResult: + """Failed result for any other checker exception, including checker __init__ + failures. Shared by the sync/async generic-Exception branches in + ``_check_single`` / ``_check_single_async``.""" + criterion_type = criterion.type + logger.exception(f"Checker failure for criterion '{criterion_type}': {exc}") + failed = CriterionResult( + criterion_type=criterion_type, + description=criterion.description, + score=0.0, + details="Error running checker", + error=str(exc), + pass_threshold=criterion.pass_threshold, + gating=criterion.is_gating, + ) + logger.info( + "Criterion '%s' %s (score=0.00, threshold=%.2f): %s", + criterion_type, + "FAILED" if criterion.is_gating else "errored (informational)", + criterion.pass_threshold, + _short_failure_reason(failed), + ) + return failed + def _check_single( self, criterion: SuccessCriterion, @@ -199,12 +346,9 @@ def _check_single( Returns: CriterionResult with score """ - criterion_type = criterion.type - # V3: Broader exception handling - catches checker constructor failures too try: - # Get cached instance - checker = self._get_checker_instance(criterion_type) + checker = self._get_checker_instance(criterion.type) context = CheckContext(route=self.route, reference_dir=reference_dir) result = checker.check( criterion, @@ -213,55 +357,48 @@ def _check_single( turn_records=turn_records, context=context, ) - result.pass_threshold = criterion.pass_threshold - result.gating = criterion.is_gating - - logger.debug(f"Criterion '{criterion_type}' score: {result.score:.2f}") - if result.score < criterion.pass_threshold: - # An informational (weight: 0) criterion cannot fail the task, so - # logging it as FAILED contradicts the final status. Say what it is. - logger.info( - "Criterion '%s' %s (score=%.2f, threshold=%.2f): %s", - criterion_type, - "FAILED" if criterion.is_gating else "below threshold (informational)", - result.score, - criterion.pass_threshold, - _short_failure_reason(result), - ) - return result - + return self._finalize_result(criterion, result) except KeyError: - # No checker registered for this type - return failed result for consistency. - # ERROR record carries enough context — skip the companion FAILED line. - logger.error(f"No checker found for criterion type '{criterion_type}'") - return CriterionResult( - criterion_type=criterion_type, - description=criterion.description, - score=0.0, - details=f"No checker registered for criterion type '{criterion_type}'", - error=f"Unsupported criterion type: '{criterion_type}'", - pass_threshold=criterion.pass_threshold, - gating=criterion.is_gating, - ) - except JudgeInfrastructureError: - raise # judge infra failure escalates to FinalStatus.ERROR; do not score it + return self._missing_checker_result(criterion) + except _ESCALATING_EXCEPTIONS: + raise # judge infra failure / checker misuse escalates to FinalStatus.ERROR; do not score it except Exception as e: # V3: Catch ALL exceptions, including checker __init__ failures - logger.exception(f"Checker failure for criterion '{criterion_type}': {e}") - failed = CriterionResult( - criterion_type=criterion_type, - description=criterion.description, - score=0.0, - details="Error running checker", - error=str(e), - pass_threshold=criterion.pass_threshold, - gating=criterion.is_gating, - ) - logger.info( - "Criterion '%s' %s (score=0.00, threshold=%.2f): %s", - criterion_type, - "FAILED" if criterion.is_gating else "errored (informational)", - criterion.pass_threshold, - _short_failure_reason(failed), + return self._error_result(criterion, e) + + async def _check_single_async( + self, + criterion: SuccessCriterion, + reference_code: str | None, + turn_records: TurnRecords | None = None, + reference_dir: Path | None = None, + ) -> CriterionResult: + """Async twin of ``_check_single`` — used only for native-async checkers, + dispatched via ``check_async`` instead of ``check``. Shares the result + shaping and logging via ``_finalize_result`` / ``_missing_checker_result`` / + ``_error_result``, so the two methods differ only in how they invoke the + checker (``checker.check`` vs. ``await checker.check_async``). + """ + try: + checker = self._get_checker_instance(criterion.type) + context = CheckContext(route=self.route, reference_dir=reference_dir) + result = await checker.check_async( + criterion, + self.sandbox, + reference_code, + turn_records=turn_records, + context=context, ) - return failed + return self._finalize_result(criterion, result) + except KeyError: + # Deliberate dead-code defence: an unregistered type resolves to + # `_is_native_async(...) is False` (see that method), so + # `_check_single_async` is only ever invoked for already-registered + # types — this arm exists only to keep the two `_check_single*` + # methods' exception shape identical, in case that invariant ever + # changes. + return self._missing_checker_result(criterion) + except _ESCALATING_EXCEPTIONS: + raise + except Exception as e: + return self._error_result(criterion, e) diff --git a/src/coder_eval/evaluation/judge_anthropic.py b/src/coder_eval/evaluation/judge_anthropic.py index 287e9dfe..c8dfde82 100644 --- a/src/coder_eval/evaluation/judge_anthropic.py +++ b/src/coder_eval/evaluation/judge_anthropic.py @@ -6,19 +6,26 @@ converted to a dict via ``model_dump`` so the caller can reuse ``extract_verdict_from_anthropic_response`` — Anthropic SDK and Bedrock share Anthropic's native message shape (content blocks with ``type: tool_use``). + +Async on purpose: this is llm_judge's only implementation of the network +call (there is no sync twin) — ``AsyncAnthropic`` lets the call yield the +event loop instead of blocking a thread-pool thread for the wait, so +``SuccessChecker.check_all_async`` awaits it directly without pinning a +thread. (``check_all_async`` currently runs criteria sequentially; running +several judges concurrently is deferred to a follow-up PR.) """ from __future__ import annotations from typing import Any -from anthropic import Anthropic, APIError +from anthropic import APIError, AsyncAnthropic from coder_eval.errors import JudgeInfrastructureError from coder_eval.evaluation.judge_models import to_anthropic_alias -def invoke_anthropic_judge( +async def invoke_anthropic_judge_async( *, model: str, system: str, @@ -36,10 +43,10 @@ def invoke_anthropic_judge( between this SDK call and the Bedrock httpx-direct call. """ alias = to_anthropic_alias(model) - client = Anthropic(timeout=timeout_seconds) - with client: + client = AsyncAnthropic(timeout=timeout_seconds) + async with client: try: - response = client.messages.create( + response = await client.messages.create( model=alias, system=system, messages=[{"role": "user", "content": user}], diff --git a/src/coder_eval/evaluation/judge_bedrock.py b/src/coder_eval/evaluation/judge_bedrock.py index 6627534b..4e364a0c 100644 --- a/src/coder_eval/evaluation/judge_bedrock.py +++ b/src/coder_eval/evaluation/judge_bedrock.py @@ -12,12 +12,19 @@ agent_judge uses the Claude Code SDK subprocess instead — the two paths intentionally do not share an HTTP client. + +Async on purpose: this is llm_judge's only implementation of the network +call (there is no sync twin) — ``httpx.AsyncClient`` lets the call yield the +event loop instead of blocking a thread-pool thread for the wait, so +``SuccessChecker.check_all_async`` awaits it directly without pinning a +thread. (``check_all_async`` currently runs criteria sequentially; running +several judges concurrently is deferred to a follow-up PR.) """ from __future__ import annotations +import asyncio import logging -import time from typing import Any import httpx @@ -39,7 +46,7 @@ def _is_retryable_status(status_code: int) -> bool: return status_code == 429 or status_code >= 500 -def invoke_bedrock_judge( +async def invoke_bedrock_judge_async( *, route: BedrockRoute, model: str, @@ -80,35 +87,34 @@ def invoke_bedrock_judge( "Accept": "application/json", } - # invoke_bedrock_judge is sync (called via asyncio.to_thread), so a plain - # time.sleep between attempts is correct here. attempts = _JUDGE_RETRY.max_retries + 1 last_failure = "" last_exc: Exception | None = None - for attempt in range(attempts): - if attempt: - time.sleep(compute_backoff(_JUDGE_RETRY, attempt - 1)) - try: - response = httpx.post(url, headers=headers, json=body, timeout=timeout_seconds) - except httpx.HTTPError as e: - last_failure = f"Bedrock invoke transport error: {e}" - last_exc = e - logger.warning("Bedrock judge attempt %d/%d failed: %s", attempt + 1, attempts, last_failure) - continue - if _is_retryable_status(response.status_code): - last_failure = f"Bedrock invoke failed: {response.status_code} {response.text[:500]}" - last_exc = None - logger.warning("Bedrock judge attempt %d/%d failed: %s", attempt + 1, attempts, last_failure) - continue - if response.status_code >= 300: - raise JudgeInfrastructureError(f"Bedrock invoke failed: {response.status_code} {response.text[:500]}") - try: - data = response.json() - except ValueError as e: - # A malformed/truncated 2xx body (flaky proxy/gateway) is infra, not - # agent quality — escalate like the non-dict arm below. - raise JudgeInfrastructureError(f"Bedrock response is not valid JSON: {e}") from e - if not isinstance(data, dict): - raise JudgeInfrastructureError(f"Bedrock response is not a JSON object: {str(data)[:500]}") - return data + async with httpx.AsyncClient() as client: + for attempt in range(attempts): + if attempt: + await asyncio.sleep(compute_backoff(_JUDGE_RETRY, attempt - 1)) + try: + response = await client.post(url, headers=headers, json=body, timeout=timeout_seconds) + except httpx.HTTPError as e: + last_failure = f"Bedrock invoke transport error: {e}" + last_exc = e + logger.warning("Bedrock judge attempt %d/%d failed: %s", attempt + 1, attempts, last_failure) + continue + if _is_retryable_status(response.status_code): + last_failure = f"Bedrock invoke failed: {response.status_code} {response.text[:500]}" + last_exc = None + logger.warning("Bedrock judge attempt %d/%d failed: %s", attempt + 1, attempts, last_failure) + continue + if response.status_code >= 300: + raise JudgeInfrastructureError(f"Bedrock invoke failed: {response.status_code} {response.text[:500]}") + try: + data = response.json() + except ValueError as e: + # A malformed/truncated 2xx body (flaky proxy/gateway) is infra, not + # agent quality — escalate like the non-dict arm below. + raise JudgeInfrastructureError(f"Bedrock response is not valid JSON: {e}") from e + if not isinstance(data, dict): + raise JudgeInfrastructureError(f"Bedrock response is not a JSON object: {str(data)[:500]}") + return data raise JudgeInfrastructureError(f"{last_failure} (after {attempts} attempts)") from last_exc diff --git a/src/coder_eval/evaluation/judge_usage.py b/src/coder_eval/evaluation/judge_usage.py index 312a889a..4727cf25 100644 --- a/src/coder_eval/evaluation/judge_usage.py +++ b/src/coder_eval/evaluation/judge_usage.py @@ -34,8 +34,8 @@ def _coerce_int(value: Any) -> int: def token_usage_from_anthropic_dict(resp: dict[str, Any]) -> TokenUsage | None: """Extract usage from an Anthropic / Bedrock-invoke Messages response dict. - Both ``invoke_anthropic_judge`` (``response.model_dump()``) and - ``invoke_bedrock_judge`` (parsed ``/invoke`` JSON) carry an Anthropic-shaped + Both ``invoke_anthropic_judge_async`` (``response.model_dump()``) and + ``invoke_bedrock_judge_async`` (parsed ``/invoke`` JSON) carry an Anthropic-shaped ``usage`` block. Returns ``None`` when usage is missing or carries no tokens. """ u = resp.get("usage") diff --git a/src/coder_eval/evaluation/sub_agent.py b/src/coder_eval/evaluation/sub_agent.py index f90721ef..88d1bc2e 100644 --- a/src/coder_eval/evaluation/sub_agent.py +++ b/src/coder_eval/evaluation/sub_agent.py @@ -109,14 +109,37 @@ def __init__( # submit_verdict tool). NOT routed through ``sdk_options`` — # ``mcp_servers`` is in ``_FRAMEWORK_OWNED_SDK_FIELDS``. self._extra_mcp_servers = extra_mcp_servers or {} - # Public attribute so the criterion can read it after ``run()`` returns. + # Public attribute so the criterion can read it after ``run_async()`` returns. # When the caller passes ``capture=None`` the runner doesn't expose one, # matching the opt-in-per-construction contract for the verdict channel. self.capture = capture - def run(self, user_msg: str, *, max_turns: int | None, turn_timeout: float) -> TurnRecord: + async def run_async(self, user_msg: str, *, max_turns: int | None, turn_timeout: float) -> TurnRecord: """Copy sandbox → start agent → communicate → stop. Kill on any exception. + Async so a genuine network/subprocess wait yields the event loop instead + of pinning a thread-pool thread — lets ``SuccessChecker.check_all_async`` + await this directly without pinning a thread. (``check_all_async`` + currently runs criteria sequentially; running this concurrently with + other judge-type criteria is deferred to a follow-up PR.) The blocking + filesystem work (``copytree``/``rmtree``) is pushed to a worker thread + via ``asyncio.to_thread`` so it doesn't block the loop either. + + Cancellation safety: ``run_async`` is awaited directly on the + orchestrator's own loop (not under its own ``asyncio.run`` on a worker + thread), so it is reachable by cancellation (e.g. the ``task_timeout`` + watchdog cancelling the orchestrator task) at any ``await`` — including + mid-``copytree``. ``asyncio.to_thread`` is NOT itself cancellable (the + worker thread keeps running after the awaiting coroutine raises + ``CancelledError``), so every such call here is wrapped in + ``asyncio.shield`` and tracked in ``self._pending`` — the ``finally`` + below awaits any still-in-flight one BEFORE ``rmtree``, so an orphan + thread can never recreate files in ``judge_dir`` after cleanup already + ran. ``judge_dir`` itself is bound with a plain synchronous + ``tempfile.mkdtemp`` (a single fast syscall — offloading it via + ``to_thread`` only widens the cancellation window for no benefit, since + a bare syscall isn't itself an await point cancellation can land on). + Raises ``TurnTimeoutError`` when the agent exceeds ``turn_timeout``. """ # Narrow via local var — checked in __init__ but pyright doesn't track that. @@ -124,13 +147,16 @@ def run(self, user_msg: str, *, max_turns: int | None, turn_timeout: float) -> T assert src_dir is not None, "sandbox not initialized" judge_dir = Path(tempfile.mkdtemp(prefix="sub_agent_")) + pending: list[asyncio.Task[Any]] = [] try: # Copy the sandbox into an isolated temp dir. The sub-agent never touches # the original sandbox, so later criteria are unaffected by whatever it # does. Symlinks are skipped (vs preserved) so a malicious # `creds -> /root/.aws/credentials` plant can't leak host files to a # Bash-enabled sub-agent. - shutil.copytree( + await self._shielded_to_thread( + pending, + shutil.copytree, src_dir, judge_dir, symlinks=True, @@ -156,7 +182,7 @@ def run(self, user_msg: str, *, max_turns: int | None, turn_timeout: float) -> T if self._reference_dir is not None: ref_dest = judge_dir / "_reference" if ref_dest.exists(): - shutil.rmtree(ref_dest, ignore_errors=True) + await self._shielded_to_thread(pending, shutil.rmtree, ref_dest, ignore_errors=True) # Deliberately NO ``dirs_exist_ok=True`` here. The rmtree above # is the canonical clear; if any file survives (read-only flag, # ENOTEMPTY race, hostile permission bits), we want copytree to @@ -164,7 +190,9 @@ def run(self, user_msg: str, *, max_turns: int | None, turn_timeout: float) -> T # into agent-planted content under the same path. Loud failure on # a partial-rmtree edge case is preferred over silently grading # against a tampered ``_reference/``. - shutil.copytree( + await self._shielded_to_thread( + pending, + shutil.copytree, self._reference_dir, ref_dest, symlinks=True, @@ -182,18 +210,13 @@ def run(self, user_msg: str, *, max_turns: int | None, turn_timeout: float) -> T max_turns, self._agent_config.allowed_tools, ) - # Safe because callers run check_all via asyncio.to_thread, so this - # invocation is on a worker thread with no active event loop. A direct - # async caller would get RuntimeError — acceptable for the architecture. - turn = asyncio.run( - self._run_agent( - agent, - judge_dir, - user_msg, - max_turns, - turn_timeout, - plugin_tools_dir=self._sandbox.plugin_tools_dir, - ) + turn = await self._run_agent( + agent, + judge_dir, + user_msg, + max_turns, + turn_timeout, + plugin_tools_dir=self._sandbox.plugin_tools_dir, ) logger.info( "sub_agent: finished (duration=%.1fs, tokens=%s)", @@ -202,7 +225,36 @@ def run(self, user_msg: str, *, max_turns: int | None, turn_timeout: float) -> T ) return turn finally: - shutil.rmtree(judge_dir, ignore_errors=True) + # Any shielded to_thread call above keeps running on its worker thread even + # after a cancellation unwinds us into this `finally` — awaiting it here + # (not itself re-cancelled: asyncio delivers a given cancel() as a single + # CancelledError at the point it lands, not to every subsequent await in the + # same coroutine) lets it actually finish BEFORE we rmtree, so an orphan + # thread can never recreate files in judge_dir after cleanup already ran. + if pending: + await asyncio.gather(*(t for t in pending if not t.done()), return_exceptions=True) + # Deliberately NOT `await asyncio.to_thread(...)` here: a bare await inside + # `finally` is itself cancellable — cancelling right as this line is reached + # would skip the cleanup and leak the (now up-to-date) sandbox copy with no + # reaper. rmtree is a best-effort, bounded filesystem op; call it + # synchronously so cancellation can't interrupt it mid-cleanup. + shutil.rmtree(judge_dir, ignore_errors=True) # noqa: CE002 + + @staticmethod + async def _shielded_to_thread(pending: list[asyncio.Task[Any]], func: Any, *args: Any, **kwargs: Any) -> Any: + """``await asyncio.to_thread(func, *args, **kwargs)``, but shielded from + cancellation and tracked in ``pending`` so ``run_async``'s ``finally`` + can wait for it to actually finish before cleaning up ``judge_dir``. + + ``asyncio.shield`` makes the AWAIT here cancellable (a cancellation + still propagates to the caller immediately, unwinding into `finally` + as normal) while the underlying worker-thread task keeps running + independently in the background — ``pending`` is how `finally` finds + it again to wait for it rather than leaving it to race the cleanup. + """ + task = asyncio.ensure_future(asyncio.to_thread(func, *args, **kwargs)) + pending.append(task) + return await asyncio.shield(task) @staticmethod async def _run_agent( diff --git a/src/coder_eval/evaluation/verdict_tool.py b/src/coder_eval/evaluation/verdict_tool.py index 91a7f2a2..9b887851 100644 --- a/src/coder_eval/evaluation/verdict_tool.py +++ b/src/coder_eval/evaluation/verdict_tool.py @@ -132,8 +132,8 @@ def extract_verdict_from_anthropic_response( ``{"type": "tool_use", "name": "submit_verdict"}`` and validates the last one's ``input`` against ``JudgeVerdict``. Used by: - * The Bedrock httpx path (``invoke_bedrock_judge``) — raw JSON dict. - * The Anthropic SDK Direct path (``invoke_anthropic_judge``) — + * The Bedrock httpx path (``invoke_bedrock_judge_async``) — raw JSON dict. + * The Anthropic SDK Direct path (``invoke_anthropic_judge_async``) — response converted via ``Message.model_dump()``. Both backends share Anthropic's native message shape. diff --git a/src/coder_eval/orchestration/early_stop.py b/src/coder_eval/orchestration/early_stop.py index 2ca5252f..fc9f0e5d 100644 --- a/src/coder_eval/orchestration/early_stop.py +++ b/src/coder_eval/orchestration/early_stop.py @@ -25,7 +25,7 @@ loop before the result message is ever pulled. Live verdicts only *trigger* the stop; the authoritative scores always come -from the standard ``check_all`` on the frozen trajectory after the cut. +from the standard ``check_all_async`` on the frozen trajectory after the cut. Precision trade-off: a pass-stop cuts the run the instant every *pass-armed* criterion is decided, so a *fail-armed* criterion (e.g. a distractor) that would diff --git a/src/coder_eval/orchestrator.py b/src/coder_eval/orchestrator.py index f5a5206e..286e5991 100644 --- a/src/coder_eval/orchestrator.py +++ b/src/coder_eval/orchestrator.py @@ -1319,7 +1319,7 @@ def _accumulate_judge_usage( every judge call across the dialog. Ledger key is ``(position, criterion_type)`` — a stable criterion - identity. ``check_all`` rebuilds ``success_criteria_results`` in the + identity. ``check_all_async`` rebuilds ``success_criteria_results`` in the same order as ``task.success_criteria`` every turn (the positional alignment ``calculate_weighted_score`` enforces via ``zip(strict=True)``), so ``position`` is stable across turns; pairing it with the type makes @@ -1375,8 +1375,7 @@ async def _evaluation_loop(self) -> bool: task_file=self.task_file, cached_reference=self._reference_code, ) - criteria_results = await asyncio.to_thread( - self.success_checker.check_all, + criteria_results = await self.success_checker.check_all_async( self.task.success_criteria, reference_code=reference_code, reference_dir=reference_dir, @@ -1421,7 +1420,7 @@ async def _evaluation_loop(self) -> bool: self.result.iterations.append(turn_record) self._sync_sandbox_command_path_with_agent() - # Record early-stop info (if the watcher tripped) BEFORE check_all, so it + # Record early-stop info (if the watcher tripped) BEFORE check_all_async, so it # survives even if a checker raises. None on a full run or when unarmed. self.result.early_stop = self._early_stop_watcher.info if self._early_stop_watcher is not None else None @@ -1434,8 +1433,7 @@ async def _evaluation_loop(self) -> bool: task_file=self.task_file, cached_reference=self._reference_code, ) - criteria_results = await asyncio.to_thread( - self.success_checker.check_all, + criteria_results = await self.success_checker.check_all_async( self.task.success_criteria, reference_code=reference_code, reference_dir=reference_dir, @@ -1514,7 +1512,7 @@ async def _run_dialog_criteria_check( The block lifted verbatim from the three identical sites (per-turn, budget-gate fallback, end-of-dialog): (re)load the reference, run - ``check_all`` off the event loop, fold this turn's judge usage into the + ``check_all_async``, fold this turn's judge usage into the dialog-wide accumulator, store the results, and recompute the weighted score. Whether to additionally set ``all_passed`` and emit a ``CriteriaCheckEvent`` is left to the caller — those differ across the sites (the budget-gate fallback @@ -1527,8 +1525,7 @@ async def _run_dialog_criteria_check( task_file=self.task_file, cached_reference=self._reference_code, ) - criteria_results = await asyncio.to_thread( - self.success_checker.check_all, + criteria_results = await self.success_checker.check_all_async( self.task.success_criteria, reference_code=reference_code, reference_dir=reference_dir, diff --git a/src/coder_eval/reports.py b/src/coder_eval/reports.py index d5cce11f..1fabbb17 100644 --- a/src/coder_eval/reports.py +++ b/src/coder_eval/reports.py @@ -764,7 +764,7 @@ def _compute_suite_rollup( # Drive each criterion's aggregate() + evaluate suite_thresholds. Per-row # results are sliced per criterion INSTANCE by position: SuccessChecker. - # check_all appends one CriterionResult per criterion in declared order + # check_all_async appends one CriterionResult per criterion in declared order # (evaluation/checker.py), so row.success_criteria_results[i] belongs to # task_criteria[i]. Aggregating per-instance (not pooled by type) is what # lets a task stack many criteria of the SAME type — e.g. activation's diff --git a/src/coder_eval/simulation/user_simulator.py b/src/coder_eval/simulation/user_simulator.py index 418ef688..dca83d57 100644 --- a/src/coder_eval/simulation/user_simulator.py +++ b/src/coder_eval/simulation/user_simulator.py @@ -19,7 +19,6 @@ from __future__ import annotations -import asyncio import logging import shutil import tempfile @@ -232,9 +231,18 @@ async def _remove_scratch_dir(self) -> None: Shared by stop() and start()'s failure path so the rmtree idiom lives in one place. Best-effort (ignore_errors=True); idempotent (safe to call twice). + + Deliberately NOT `await asyncio.to_thread(...)`: both callers (`stop()`, + and `start()`'s `except BaseException` arm) are cancellation-reachable, + and a bare await inside that path is itself cancellable — cancelling + right as this line is reached would skip the cleanup and leak the + (empty, but still real) scratch dir with no reaper. The scratch dir + never has real content (the simulator has no tools to write into it), + so a plain synchronous rmtree is cheap and bounded — same rationale as + `SubAgentRunner.run_async`'s `finally` cleanup. """ if self._scratch_dir is not None and self._scratch_dir.exists(): - await asyncio.to_thread(shutil.rmtree, self._scratch_dir, ignore_errors=True) + shutil.rmtree(self._scratch_dir, ignore_errors=True) # noqa: CE002 self._scratch_dir = None async def start(self) -> None: diff --git a/tests/test_agent_judge_criterion.py b/tests/test_agent_judge_criterion.py index a4b63c91..cfd6b720 100644 --- a/tests/test_agent_judge_criterion.py +++ b/tests/test_agent_judge_criterion.py @@ -7,7 +7,7 @@ Test pattern: tests mock ``ClaudeCodeAgent`` via ``_AGENT_PATCH_PATH`` and pass a JSON ``agent_output`` for legacy convenience; an autouse fixture -wraps ``SubAgentRunner.run`` so that JSON-shaped agent output is parsed +wraps ``SubAgentRunner.run_async`` so that JSON-shaped agent output is parsed into the runner's ``VerdictCapture``, simulating what the real ``submit_verdict`` tool call would do. """ @@ -58,14 +58,14 @@ def _make_mock_agent(agent_output: str) -> MagicMock: return agent -# Original ``SubAgentRunner.run`` — wrapped below so JSON-shaped agent_output +# Original ``SubAgentRunner.run_async`` — wrapped below so JSON-shaped agent_output # in tests populates the runner's ``VerdictCapture`` exactly as the real # ``submit_verdict`` tool call would. -_orig_run = SubAgentRunner.run +_orig_run_async = SubAgentRunner.run_async -def _run_with_capture_simulation(self, user_msg, *, max_turns, turn_timeout): - turn = _orig_run(self, user_msg, max_turns=max_turns, turn_timeout=turn_timeout) +async def _run_with_capture_simulation(self, user_msg, *, max_turns, turn_timeout): + turn = await _orig_run_async(self, user_msg, max_turns=max_turns, turn_timeout=turn_timeout) if self.capture is not None and self.capture.verdict is None and self.capture.error is None: try: data = _json.loads(turn.agent_output) @@ -93,7 +93,7 @@ def _simulate_capture_population(monkeypatch: pytest.MonkeyPatch) -> None: criterion sees the verdict via the standard ``extract_verdict_from_capture`` path. """ - monkeypatch.setattr(SubAgentRunner, "run", _run_with_capture_simulation) + monkeypatch.setattr(SubAgentRunner, "run_async", _run_with_capture_simulation) @pytest.fixture @@ -131,6 +131,25 @@ def test_agent_judge_happy_path(sandbox: Sandbox, direct_route: DirectRoute) -> assert "duration:" in (result.details or "") +async def test_agent_judge_happy_path_via_check_all_async(sandbox: Sandbox, direct_route: DirectRoute) -> None: + """Same happy path, but through check_all_async — the orchestrator's ACTUAL + entry point. Every other test in this file drives the derived sync + `.check()` bridge (`_check_impl` -> `asyncio.run(_check_impl_async(...))` + on a fresh loop), which production never takes for this criterion — this + is the one test that exercises agent_judge on the loop it actually runs + on, including a real SubAgentRunner.run_async coroutine (mocked at the + ClaudeCodeAgent boundary, not at run_async itself).""" + criterion = AgentJudgeCriterion(description="grade the project", prompt="Is this code valid?") + mock_agent = _make_mock_agent('{"score": 0.85, "rationale": "looks good"}') + with patch(_AGENT_PATCH_PATH, return_value=mock_agent): + results = await SuccessChecker(sandbox, init_registry=False, route=direct_route).check_all_async([criterion]) + + assert len(results) == 1 + assert results[0].score == 0.85 + assert results[0].error is None + assert "looks good" in (results[0].details or "") + + def test_agent_judge_score_clamped_high(sandbox: Sandbox, direct_route: DirectRoute) -> None: criterion = AgentJudgeCriterion(description="x", prompt="grade") mock_agent = _make_mock_agent('{"score": 1.7, "rationale": "too high"}') @@ -929,7 +948,7 @@ def test_agent_judge_mounts_reference_dir_when_include_reference_true( patch("coder_eval.criteria.agent_judge.SubAgentRunner") as mock_runner_cls, ): mock_runner = MagicMock() - mock_runner.run.return_value = _make_turn('{"score": 0.7, "rationale": "ok"}') + mock_runner.run_async = AsyncMock(return_value=_make_turn('{"score": 0.7, "rationale": "ok"}')) mock_runner_cls.return_value = mock_runner SuccessChecker(sandbox, init_registry=False, route=direct_route).check(criterion, reference_dir=ref_root) @@ -938,7 +957,7 @@ def test_agent_judge_mounts_reference_dir_when_include_reference_true( assert runner_kwargs["reference_dir"] == ref_root # Prompt envelope points the judge at _reference/. - user_msg = mock_runner.run.call_args.args[0] + user_msg = mock_runner.run_async.call_args.args[0] assert "_reference/" in user_msg assert "Use Read / Glob / Grep to browse" in user_msg @@ -958,13 +977,13 @@ def test_agent_judge_skips_reference_dir_when_include_reference_false( patch("coder_eval.criteria.agent_judge.SubAgentRunner") as mock_runner_cls, ): mock_runner = MagicMock() - mock_runner.run.return_value = _make_turn('{"score": 0.5, "rationale": "ok"}') + mock_runner.run_async = AsyncMock(return_value=_make_turn('{"score": 0.5, "rationale": "ok"}')) mock_runner_cls.return_value = mock_runner SuccessChecker(sandbox, init_registry=False, route=direct_route).check(criterion, reference_dir=ref_root) runner_kwargs = mock_runner_cls.call_args.kwargs assert runner_kwargs["reference_dir"] is None - user_msg = mock_runner.run.call_args.args[0] + user_msg = mock_runner.run_async.call_args.args[0] assert "_reference/" not in user_msg @@ -1050,8 +1069,8 @@ def test_agent_judge_integration_real_sdk(tmp_path: Path) -> None: def _patch_runner_with_capture(verdict_payload: dict | None, agent_output: str = "Verdict submitted."): - """Return a patch context manager that swaps ``SubAgentRunner.run`` for a stub - that populates the runner's ``capture`` and returns a synthetic turn. + """Return a patch context manager that swaps ``SubAgentRunner.run_async`` for a + stub that populates the runner's ``capture`` and returns a synthetic turn. When ``verdict_payload`` is None the capture stays empty (simulates the judge failing to call submit_verdict). @@ -1059,14 +1078,14 @@ def _patch_runner_with_capture(verdict_payload: dict | None, agent_output: str = from coder_eval.evaluation.sub_agent import SubAgentRunner from coder_eval.models import JudgeVerdict - def _stub_run(self, user_msg, *, max_turns, turn_timeout): + async def _stub_run(self, user_msg, *, max_turns, turn_timeout): if verdict_payload is not None: self.capture.verdict = JudgeVerdict.model_validate(verdict_payload) self.capture.error = None self.capture.called_count += 1 return _make_turn(agent_output) - return patch.object(SubAgentRunner, "run", _stub_run) + return patch.object(SubAgentRunner, "run_async", _stub_run) def test_agent_judge_tool_channel_happy_path(sandbox: Sandbox, direct_route: DirectRoute) -> None: @@ -1091,7 +1110,7 @@ def test_agent_judge_tool_channel_overwrites_on_retry(sandbox: Sandbox, direct_r """LAST-call discipline at the criterion layer — the final verdict wins.""" criterion = AgentJudgeCriterion(description="x", prompt="grade") - def _stub_run(self, user_msg, *, max_turns, turn_timeout): + async def _stub_run(self, user_msg, *, max_turns, turn_timeout): # Simulate two calls — final one wins. self.capture.verdict = JudgeVerdict(score=0.2, rationale="first") self.capture.called_count += 1 @@ -1099,7 +1118,7 @@ def _stub_run(self, user_msg, *, max_turns, turn_timeout): self.capture.called_count += 1 return _make_turn("Verdict submitted.") - with patch.object(SubAgentRunner, "run", _stub_run): + with patch.object(SubAgentRunner, "run_async", _stub_run): result = SuccessChecker(sandbox, init_registry=False, route=direct_route).check(criterion) assert result.score == 0.9 assert "second" in (result.details or "") @@ -1147,10 +1166,10 @@ def test_agent_judge_timeout_returns_judge_criterion_result(sandbox: Sandbox, di criterion = AgentJudgeCriterion(description="x", prompt="grade", turn_timeout=10) - def _raise(self, user_msg, *, max_turns, turn_timeout): + async def _raise(self, user_msg, *, max_turns, turn_timeout): raise TurnTimeoutError(10.0, task_id="t", iteration=1) - with patch.object(SubAgentRunner, "run", _raise): + with patch.object(SubAgentRunner, "run_async", _raise): result = SuccessChecker(sandbox, init_registry=False, route=direct_route).check(criterion) assert result.score == 0.0 assert "TurnTimeoutError" in (result.error or "") diff --git a/tests/test_check_all_async.py b/tests/test_check_all_async.py new file mode 100644 index 00000000..822fcaa1 --- /dev/null +++ b/tests/test_check_all_async.py @@ -0,0 +1,635 @@ +"""Tests for SuccessChecker.check_all_async. + +BaseCriterion's primary surface is async: a checker overrides exactly one of +``_check_impl`` (sync CPU/file-bound) or ``_check_impl_async`` (genuine async +I/O — llm_judge / agent_judge), and the base derives the other. A checker +counts as "native async" for dispatch purposes iff it overrides +``_check_impl_async`` itself. + +``check_all_async`` currently runs every criterion SEQUENTIALLY, strictly in +declaration order — the same order/isolation guarantee ``check_all`` (the +sync twin) provides. Concurrent dispatch of adjacent judge criteria (the +GH #55 motivation) is intentionally deferred to a follow-up PR; this module +pins the sequential contract in the meantime: + - every criterion is fully awaited before the next one starts, regardless + of whether it's native-async or sync-offloaded-to-a-thread, + - declaration order is preserved exactly (matches ``check_all``), + - the sync path's error-handling contract carries over unchanged (KeyError / + generic Exception captured into a failed CriterionResult; + JudgeInfrastructureError escalates and stops any remaining criteria). +""" + +import asyncio +import threading +import time +from unittest.mock import AsyncMock, patch + +import pytest + +from coder_eval.criteria.base import BaseCriterion +from coder_eval.errors import JudgeInfrastructureError +from coder_eval.evaluation.checker import SuccessChecker +from coder_eval.models import ( + CriterionResult, + FileExistsCriterion, + LLMJudgeCriterion, + RunCommandCriterion, + SandboxConfig, +) +from coder_eval.models.routing import DirectRoute +from coder_eval.sandbox import Sandbox + + +SLEEP_SECONDS = 0.2 + + +@pytest.fixture +def sandbox(tmp_path): + config = SandboxConfig(driver="tempdir") + sb = Sandbox(config, task_id="check_all_async_test") + try: + sb.setup() + yield sb + finally: + sb.cleanup() + + +@pytest.fixture +def checker(sandbox): + return SuccessChecker(sandbox, init_registry=True, validate_registry=False) + + +class _SleepyAsyncChecker(BaseCriterion[LLMJudgeCriterion]): + """Stand-in for llm_judge/agent_judge: overrides _check_impl_async only, + and sleeps on the event loop (not a thread) — the "native async" shape. + + Optionally appends markers to a shared ``events`` list (for ordering + assertions) instead of relying on wall-clock timing. + """ + + criterion_type = "llm_judge" + + def __init__( + self, + sleep_seconds: float = SLEEP_SECONDS, + events: list[str] | None = None, + label: str = "async", + ): + self.sleep_seconds = sleep_seconds + self.calls = 0 + self.events = events + self.label = label + + async def _check_impl_async(self, criterion, sandbox, reference_code=None, *, turn_records=None, context=None): + self.calls += 1 + if self.events is not None: + self.events.append(f"{self.label}-start") + await asyncio.sleep(self.sleep_seconds) + if self.events is not None: + self.events.append(f"{self.label}-end") + return CriterionResult(criterion_type=self.criterion_type, description=criterion.description, score=1.0) + + +class _SleepyThreadChecker(BaseCriterion[FileExistsCriterion]): + """Stand-in for a CPU/file-bound checker: overrides _check_impl only (plain + sync code) and blocks a thread, not async-native.""" + + criterion_type = "file_exists" + + def __init__(self, sleep_seconds: float = SLEEP_SECONDS, events: list[str] | None = None, label: str = "sync"): + self.sleep_seconds = sleep_seconds + self.events = events + self.label = label + + def _check_impl(self, criterion, sandbox, reference_code=None, *, turn_records=None, context=None): + if self.events is not None: + self.events.append(f"{self.label}-start") + time.sleep(self.sleep_seconds) + if self.events is not None: + self.events.append(f"{self.label}-end") + return CriterionResult(criterion_type=self.criterion_type, description=criterion.description, score=1.0) + + +class _RaisingAsyncChecker(BaseCriterion[LLMJudgeCriterion]): + criterion_type = "llm_judge" + + def __init__(self, exc: Exception): + self.exc = exc + + async def _check_impl_async(self, criterion, sandbox, reference_code=None, *, turn_records=None, context=None): + raise self.exc + + +def _llm_criterion(description: str) -> LLMJudgeCriterion: + return LLMJudgeCriterion(description=description, prompt="grade it") + + +def _file_criterion(description: str, path: str = "x.txt") -> FileExistsCriterion: + return FileExistsCriterion(description=description, path=path) + + +class TestSequentialExecution: + @pytest.mark.asyncio + async def test_two_judge_criteria_run_sequentially_not_concurrently(self, checker): + """check_all_async currently runs every criterion SEQUENTIALLY — + concurrent dispatch of adjacent judges (the GH #55 motivation) is + deferred to a follow-up PR. Two llm_judge-type criteria must NOT + overlap: the first must fully finish (start AND end) before the + second starts, proven deterministically via ordered event markers.""" + events: list[str] = [] + fake = _SleepyAsyncChecker(sleep_seconds=0.01, events=events) + + checker._checker_instances["llm_judge"] = fake + criteria = [_llm_criterion("a"), _llm_criterion("b")] + results = await asyncio.wait_for(checker.check_all_async(criteria), timeout=5.0) + + assert fake.calls == 2 + assert events == ["async-start", "async-end", "async-start", "async-end"], ( + f"judge criteria overlapped instead of running sequentially: {events}" + ) + assert [r.score for r in results] == [1.0, 1.0] + + @pytest.mark.asyncio + async def test_sync_and_async_criteria_never_overlap_and_preserve_declaration_order(self, checker): + """Sync and native-async criteria must never overlap — several + first-party sync criteria mutate the sandbox (run_command, + uipath_eval) while judge criteria read it, so overlapping the two + would make a judge's score depend on how far a concurrent + sandbox-mutating command happened to get. Declaration order must be + preserved exactly, matching check_all's serial semantics. + + Proven deterministically via ordered event markers instead of a + wall-clock margin, for BOTH orderings. + """ + events: list[str] = [] + async_fake = _SleepyAsyncChecker(sleep_seconds=0.01, events=events, label="async") + sync_fake = _SleepyThreadChecker(sleep_seconds=0.01, events=events, label="sync") + checker._checker_instances["llm_judge"] = async_fake + checker._checker_instances["file_exists"] = sync_fake + + criteria = [_llm_criterion("judge"), _file_criterion("file")] + results = await asyncio.wait_for(checker.check_all_async(criteria), timeout=5.0) + + assert events == ["async-start", "async-end", "sync-start", "sync-end"], ( + f"declared order [judge, file] was not preserved: {events}" + ) + assert [r.criterion_type for r in results] == ["llm_judge", "file_exists"] + assert [r.score for r in results] == [1.0, 1.0] + + async def test_sync_before_async_declaration_order_also_preserved(self, checker): + """Same guarantee, reversed declaration order — [file, judge].""" + events: list[str] = [] + async_fake = _SleepyAsyncChecker(sleep_seconds=0.01, events=events, label="async") + sync_fake = _SleepyThreadChecker(sleep_seconds=0.01, events=events, label="sync") + checker._checker_instances["llm_judge"] = async_fake + checker._checker_instances["file_exists"] = sync_fake + + criteria = [_file_criterion("file"), _llm_criterion("judge")] + results = await asyncio.wait_for(checker.check_all_async(criteria), timeout=5.0) + + assert events == ["sync-start", "sync-end", "async-start", "async-end"], ( + f"declared order [file, judge] was not preserved: {events}" + ) + assert [r.criterion_type for r in results] == ["file_exists", "llm_judge"] + assert [r.score for r in results] == [1.0, 1.0] + + @pytest.mark.asyncio + async def test_run_command_writes_are_visible_to_subsequent_llm_judge(self, tmp_path): + """End-to-end regression test (real registered run_command + llm_judge + checkers, not fakes): a sandbox-mutating sync criterion must complete + BEFORE a sandbox-reading judge criterion declared after it starts, so + the judge's view of the sandbox is deterministic.""" + config = SandboxConfig(driver="tempdir") + sandbox = Sandbox(config, task_id="ordering_regression_test") + sandbox.setup() + try: + real_checker = SuccessChecker(sandbox, init_registry=True, validate_registry=False, route=DirectRoute()) + run_criterion = RunCommandCriterion( + description="write file", + command="sleep 0.2 && printf hello > built.txt", + ) + judge_criterion = LLMJudgeCriterion( + description="judge", + prompt="grade it", + files=["built.txt"], + include_agent_output=False, + include_tool_calls=False, + include_dialog=False, + ) + + def fake_invoke(**kwargs): + saw_hello = "hello" in kwargs["user"] + return { + "content": [ + { + "type": "tool_use", + "name": "submit_verdict", + "input": {"score": 1.0 if saw_hello else 0.0, "rationale": "ok", "findings": []}, + } + ] + } + + with patch( + "coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", + new=AsyncMock(side_effect=fake_invoke), + ): + results = await real_checker.check_all_async([run_criterion, judge_criterion]) + + assert results[0].score == 1.0, "run_command should have succeeded" + assert results[1].score == 1.0, ( + "llm_judge did not see the file run_command wrote — the sandbox-mutating " + "sync batch and the sandbox-reading judge batch overlapped" + ) + finally: + sandbox.cleanup(preserve=False) + + @pytest.mark.asyncio + async def test_llm_judge_declared_before_run_command_sees_pre_mutation_state(self, tmp_path): + """Reversed declaration order — [llm_judge, run_command] — must match + check_all's strict declaration-order semantics: the judge, declared + first, must see PRE-mutation state, exactly like it would running + serially (which is exactly what check_all_async does today).""" + config = SandboxConfig(driver="tempdir") + sandbox = Sandbox(config, task_id="reversed_ordering_regression_test") + sandbox.setup() + try: + real_checker = SuccessChecker(sandbox, init_registry=True, validate_registry=False, route=DirectRoute()) + judge_criterion = LLMJudgeCriterion( + description="judge", + prompt="grade it", + files=["built.txt"], + include_agent_output=False, + include_tool_calls=False, + include_dialog=False, + ) + run_criterion = RunCommandCriterion( + description="write file", + command="sleep 0.2 && printf hello > built.txt", + ) + + def fake_invoke(**kwargs): + saw_hello = "hello" in kwargs["user"] + return { + "content": [ + { + "type": "tool_use", + "name": "submit_verdict", + "input": {"score": 1.0 if saw_hello else 0.0, "rationale": "ok", "findings": []}, + } + ] + } + + sandbox2 = Sandbox(config, task_id="reversed_ordering_regression_test_sync_twin") + sandbox2.setup() + try: + sync_checker = SuccessChecker( + sandbox2, init_registry=True, validate_registry=False, route=DirectRoute() + ) + with patch( + "coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", + new=AsyncMock(side_effect=fake_invoke), + ): + async_results = await real_checker.check_all_async([judge_criterion, run_criterion]) + # check_all is the fully-sync surface: run it off the test's own + # event loop (asyncio.to_thread) so its derived asyncio.run bridge + # doesn't see a running loop and raise CheckerMisuseError. A + # separate sandbox/checker avoids reusing built.txt from the + # check_all_async run above, which would let the sync run's + # judge see already-mutated state regardless of ordering. + sync_results = await asyncio.to_thread(sync_checker.check_all, [judge_criterion, run_criterion]) + finally: + sandbox2.cleanup(preserve=False) + + assert async_results[1].score == 1.0, "run_command should have succeeded" + assert async_results[0].score == 0.0, "llm_judge declared BEFORE run_command must see PRE-mutation state" + assert [r.score for r in async_results] == [r.score for r in sync_results], ( + "check_all_async must agree with check_all's declaration-order semantics" + ) + finally: + sandbox.cleanup(preserve=False) + + @pytest.mark.asyncio + async def test_result_order_preserved_regardless_of_dispatch_path(self, checker): + """Interleaved sync/async criteria must come back in declaration order.""" + checker._checker_instances["llm_judge"] = _SleepyAsyncChecker(sleep_seconds=0.01) + checker._checker_instances["file_exists"] = _SleepyThreadChecker(sleep_seconds=0.01) + + criteria = [ + _file_criterion("f1"), + _llm_criterion("j1"), + _file_criterion("f2"), + _llm_criterion("j2"), + ] + results = await checker.check_all_async(criteria) + assert [r.description for r in results] == ["f1", "j1", "f2", "j2"] + assert [r.criterion_type for r in results] == ["file_exists", "llm_judge", "file_exists", "llm_judge"] + + +class TestNativeAsyncErrorHandling: + @pytest.mark.asyncio + async def test_generic_exception_captured_as_failed_result(self, checker): + checker._checker_instances["llm_judge"] = _RaisingAsyncChecker(ValueError("boom")) + results = await checker.check_all_async([_llm_criterion("j")]) + assert len(results) == 1 + assert results[0].score == 0.0 + assert "ValueError: boom" in (results[0].error or "") + + @pytest.mark.asyncio + async def test_judge_infrastructure_error_propagates(self, checker): + checker._checker_instances["llm_judge"] = _RaisingAsyncChecker(JudgeInfrastructureError("down")) + with pytest.raises(JudgeInfrastructureError, match="down"): + await checker.check_all_async([_llm_criterion("j")]) + + @pytest.mark.asyncio + async def test_judge_infrastructure_error_stops_remaining_criteria(self, checker): + """Sequential dispatch means a JudgeInfrastructureError from one + criterion propagates immediately, exactly like check_all's serial + list-building — criteria declared AFTER the raising one never even + start (there is nothing concurrent to orphan, since nothing runs + concurrently).""" + ran: list[str] = [] + + class _TrackingAsyncChecker(_SleepyAsyncChecker): + async def _check_impl_async( + self, criterion, sandbox, reference_code=None, *, turn_records=None, context=None + ): + ran.append(criterion.description) + return await super()._check_impl_async( + criterion, sandbox, reference_code, turn_records=turn_records, context=context + ) + + checker._checker_instances["llm_judge"] = _RaisingAsyncChecker(JudgeInfrastructureError("down")) + checker._checker_instances["agent_judge"] = _TrackingAsyncChecker(sleep_seconds=0.0) + + from coder_eval.models import AgentJudgeCriterion + + criteria = [_llm_criterion("j"), AgentJudgeCriterion(description="never_runs", prompt="grade it")] + with pytest.raises(JudgeInfrastructureError, match="down"): + await checker.check_all_async(criteria) + assert ran == [], "criterion declared after the raising one should never have started" + + @pytest.mark.asyncio + async def test_empty_criteria_returns_empty(self, checker): + assert await checker.check_all_async([]) == [] + + @pytest.mark.asyncio + async def test_native_async_checker_constructor_failure_scores_zero_not_abort(self, checker): + """_is_native_async's own docstring names this as its design + rationale — classification reads the registered CLASS precisely so it + never has to construct the checker, meaning a constructor failure is + caught by _check_single_async's ordinary error boundary (same as any + other checker exception) and scores that ONE criterion 0.0, instead of + escaping check_all_async and aborting the whole task.""" + from coder_eval.criteria import CriterionRegistry + + class _BoomOnInitChecker(BaseCriterion[LLMJudgeCriterion]): + criterion_type = "boom_on_init_test" + + def __init__(self): + raise RuntimeError("ctor boom") + + async def _check_impl_async( + self, criterion, sandbox, reference_code=None, *, turn_records=None, context=None + ): + raise NotImplementedError + + CriterionRegistry.register(_BoomOnInitChecker) + try: + criterion = _llm_criterion("j").model_copy(update={"type": "boom_on_init_test"}) + results = await checker.check_all_async([criterion]) + finally: + CriterionRegistry._checkers.pop("boom_on_init_test", None) + + assert len(results) == 1 + assert results[0].score == 0.0 + assert "ctor boom" in (results[0].error or "") + + def test_checker_misuse_error_escalates_through_success_checker_not_scored_zero(self, checker): + """CheckerMisuseError must escalate through SuccessChecker.check / check_all + exactly like JudgeInfrastructureError, not fall into _check_single's generic + `except Exception` arm and get scored 0.0. The prior fix only pinned this at + the BaseCriterion.check() layer (test_async_only_checker_sync_bridge_raises_loudly_from_a_running_loop); + this test exercises it through the SuccessChecker entry point a caller + actually uses, since _check_single/_check_single_async each have their own + except clauses that must also name CheckerMisuseError.""" + from coder_eval.errors import CheckerMisuseError + + checker._checker_instances["llm_judge"] = _SleepyAsyncChecker(sleep_seconds=0.0) + + async def call_check_from_running_loop(): + checker.check(_llm_criterion("j")) + + with pytest.raises(CheckerMisuseError, match="check_async"): + asyncio.run(call_check_from_running_loop()) + + +class TestCheckAllAsyncMatchesSyncBehavior: + @pytest.mark.asyncio + async def test_all_sync_criteria_still_produce_same_results_as_check_all(self, checker): + criteria = [_file_criterion("f1", path="missing.txt")] + sync_results = checker.check_all(criteria) + async_results = await checker.check_all_async(criteria) + assert [r.score for r in sync_results] == [r.score for r in async_results] + + +class TestNativeAsyncDetection: + def test_sync_only_checker_registered_class_is_not_native_async(self, checker): + """_is_native_async classifies the REGISTERED CLASS, not whatever + instance happens to be cached in _checker_instances — register a + throwaway sync-only class under a fresh type name (rather than + injecting into _checker_instances, which _is_native_async never + reads) so the assertion actually exercises the class-based path.""" + from coder_eval.criteria import CriterionRegistry + + class _ThrowawaySyncChecker(BaseCriterion[FileExistsCriterion]): + criterion_type = "throwaway_sync_test" + + def _check_impl(self, criterion, sandbox, reference_code=None, *, turn_records=None, context=None): + raise NotImplementedError + + CriterionRegistry.register(_ThrowawaySyncChecker) + try: + assert checker._is_native_async("throwaway_sync_test") is False + finally: + CriterionRegistry._checkers.pop("throwaway_sync_test", None) + + def test_async_only_checker_registered_class_is_native_async(self, checker): + """Same as above, for a throwaway async-only class.""" + from coder_eval.criteria import CriterionRegistry + + class _ThrowawayAsyncChecker(BaseCriterion[LLMJudgeCriterion]): + criterion_type = "throwaway_async_test" + + async def _check_impl_async( + self, criterion, sandbox, reference_code=None, *, turn_records=None, context=None + ): + raise NotImplementedError + + CriterionRegistry.register(_ThrowawayAsyncChecker) + try: + assert checker._is_native_async("throwaway_async_test") is True + finally: + CriterionRegistry._checkers.pop("throwaway_async_test", None) + + def test_checker_instances_injection_does_not_affect_classification(self, checker): + """Pin the documented class-not-instance rule directly: injecting a + NATIVE-ASYNC fake under a SYNC type's registered name must not flip + the classification — dispatch (_get_checker_instance) and + classification (_is_native_async) intentionally read different + sources, and this test would catch them silently diverging.""" + checker._checker_instances["file_exists"] = _SleepyAsyncChecker(sleep_seconds=0.0) + assert checker._is_native_async("file_exists") is False + + def test_unregistered_type_is_not_native_async(self, checker): + assert checker._is_native_async("does_not_exist") is False + + def test_registered_llm_judge_and_agent_judge_are_native_async(self, checker): + """Pin native-async dispatch for the *real* registered checkers — the + one-line assertion that encodes the whole point of this module: if a + future refactor moves llm_judge/agent_judge back to overriding only + _check_impl, they would silently serialize behind one to_thread slot + again, and only this test would catch it.""" + assert checker._is_native_async("llm_judge") is True + assert checker._is_native_async("agent_judge") is True + + def test_is_native_async_classmethod_on_checker_class(self): + """BaseCriterion.is_native_async() is the public, typed capability + SuccessChecker._is_native_async delegates to.""" + assert _SleepyAsyncChecker.is_native_async() is True + assert _SleepyThreadChecker.is_native_async() is False + + +class TestBaseCriterionSyncAsyncDerivation: + """Direct unit coverage of BaseCriterion's cross-derivation, bypassing SuccessChecker.""" + + def test_sync_only_checker_gets_async_for_free(self): + checker = _SleepyThreadChecker(sleep_seconds=0.0) + result = asyncio.run(checker.check_async(_file_criterion("f"), sandbox=None)) + assert result.score == 1.0 + + def test_sync_only_checker_derived_async_never_blocks_the_loop(self): + """The contract _check_impl_async's base default promises is "offloads + _check_impl to a worker thread ... so a CPU/file-bound sync checker + never blocks the event loop" — assert the thread-affinity property + itself, not just the score, so a future rewrite that accidentally + calls _check_impl directly on the loop (same score, lost guarantee) + would fail this test.""" + + class _IdentRecordingChecker(BaseCriterion[FileExistsCriterion]): + criterion_type = "file_exists" + + def __init__(self): + self.check_impl_thread_ident: int | None = None + + def _check_impl(self, criterion, sandbox, reference_code=None, *, turn_records=None, context=None): + self.check_impl_thread_ident = threading.get_ident() + return CriterionResult(criterion_type=self.criterion_type, description=criterion.description, score=1.0) + + checker = _IdentRecordingChecker() + + async def run() -> int: + loop_ident = threading.get_ident() + await checker.check_async(_file_criterion("f"), sandbox=None) + return loop_ident + + loop_ident = asyncio.run(run()) + assert checker.check_impl_thread_ident is not None + assert checker.check_impl_thread_ident != loop_ident, "_check_impl ran on the event loop's own thread" + + def test_async_only_checker_gets_sync_for_free(self): + checker = _SleepyAsyncChecker(sleep_seconds=0.0) + result = checker.check(_llm_criterion("j"), sandbox=None) + assert result.score == 1.0 + + def test_async_only_checker_sync_bridge_raises_loudly_from_a_running_loop(self): + """The derived sync `_check_impl` (asyncio.run bridge) must raise a + loud, named CheckerMisuseError — not silently return a score-0.0 + CriterionResult — when called from inside a running event loop, e.g. + a library/embedder using the public check()/check_all() sync surface + from async host code. Without this guard, asyncio.run's RuntimeError + gets caught by @handle_criterion_errors and turns a real score into a + misleading 0.0.""" + from coder_eval.errors import CheckerMisuseError + + checker = _SleepyAsyncChecker(sleep_seconds=0.0) + + async def call_from_running_loop(): + checker.check(_llm_criterion("j"), sandbox=None) + + with pytest.raises(CheckerMisuseError, match="check_async"): + asyncio.run(call_from_running_loop()) + + def test_checker_overriding_both_impls_raises_at_class_definition(self): + """The exactly-ONE contract's other half: overriding BOTH _check_impl + and _check_impl_async is also rejected — a checker with two live + implementations could silently drift into different scores for the + same input depending on which entry point (check vs check_async) ran.""" + import types + + from coder_eval.models import FileExistsCriterion + + def _check_impl(self, criterion, sandbox, reference_code=None, *, turn_records=None, context=None): + raise NotImplementedError + + async def _check_impl_async(self, criterion, sandbox, reference_code=None, *, turn_records=None, context=None): + raise NotImplementedError + + with pytest.raises(TypeError, match="not both"): + types.new_class( + "_BothChecker", + (BaseCriterion[FileExistsCriterion],), + exec_body=lambda ns: ns.update( + criterion_type="both_test", _check_impl=_check_impl, _check_impl_async=_check_impl_async + ), + ) + + def test_abstract_intermediate_base_opts_out_of_the_override_check(self): + """A shared abstract base for a family of checkers (implements neither + _check_impl) can opt out with abstract=True; a concrete subclass of it + is still checked normally.""" + import types + + from coder_eval.models import FileExistsCriterion + + intermediate = types.new_class( + "_AbstractFamilyBase", + (BaseCriterion[FileExistsCriterion],), + kwds={"abstract": True}, + exec_body=lambda ns: None, + ) + + # The intermediate itself defines neither impl and did not raise. + assert intermediate._check_impl is BaseCriterion._check_impl + + # A concrete subclass that ALSO overrides neither is still rejected. + with pytest.raises(TypeError, match="must override _check_impl"): + types.new_class( + "_StillNeitherChecker", + (intermediate,), + exec_body=lambda ns: ns.update(criterion_type="still_neither_test"), + ) + + def test_base_criterion_cannot_be_instantiated_directly(self): + with pytest.raises(TypeError, match="abstract"): + BaseCriterion() + + def test_defining_a_checker_overriding_neither_raises_at_class_definition(self): + """The override contract is enforced by __init_subclass__ at + class-definition time — before register_criterion (or any other entry + point, e.g. CriterionRegistry.register called directly) ever runs. + + Built via ``type(...)`` rather than a ``class`` statement so no name is + bound to the (never fully constructed) class — __init_subclass__ raises + before the class object exists, so there would be nothing to reference + afterward anyway. + """ + import types + + from coder_eval.models import FileExistsCriterion + + with pytest.raises(TypeError, match="must override _check_impl"): + types.new_class( + "_NeitherChecker", + (BaseCriterion[FileExistsCriterion],), + exec_body=lambda ns: ns.update(criterion_type="neither_test"), + ) diff --git a/tests/test_early_stop.py b/tests/test_early_stop.py index 71cb8c7d..09fe6024 100644 --- a/tests/test_early_stop.py +++ b/tests/test_early_stop.py @@ -24,7 +24,7 @@ from datetime import datetime from pathlib import Path from typing import Any -from unittest.mock import MagicMock, patch +from unittest.mock import AsyncMock, MagicMock, patch import pytest import typer @@ -1379,7 +1379,9 @@ async def _run_wiring( orch.sandbox = sandbox checker = MagicMock() - checker.check_all = MagicMock(return_value=[_crit_result(c.type, s) for c, s in zip(criteria, scores, strict=True)]) + checker.check_all_async = AsyncMock( + return_value=[_crit_result(c.type, s) for c, s in zip(criteria, scores, strict=True)] + ) orch.success_checker = checker if stop_early: diff --git a/tests/test_judge_anthropic.py b/tests/test_judge_anthropic.py index b2c5e68c..a5b9dfc2 100644 --- a/tests/test_judge_anthropic.py +++ b/tests/test_judge_anthropic.py @@ -7,11 +7,11 @@ from __future__ import annotations from typing import Any -from unittest.mock import MagicMock, patch +from unittest.mock import AsyncMock, MagicMock, patch import pytest -from coder_eval.evaluation.judge_anthropic import invoke_anthropic_judge +from coder_eval.evaluation.judge_anthropic import invoke_anthropic_judge_async from coder_eval.evaluation.verdict_tool import SUBMIT_VERDICT_ANTHROPIC_TOOL @@ -29,11 +29,13 @@ def _make_response(*, score: float = 0.5, rationale: str = "ok") -> MagicMock: def _make_client(response: MagicMock | None = None) -> MagicMock: client = MagicMock() - client.messages.create.return_value = response if response is not None else _make_response() + client.messages.create = AsyncMock(return_value=response if response is not None else _make_response()) + client.__aenter__ = AsyncMock(return_value=client) + client.__aexit__ = AsyncMock(return_value=None) return client -def _invoke(**overrides): +async def _invoke(**overrides): defaults = { "model": "anthropic.claude-sonnet-4-6", "system": "s", @@ -43,13 +45,13 @@ def _invoke(**overrides): "tool_spec": SUBMIT_VERDICT_ANTHROPIC_TOOL, } defaults.update(overrides) - return invoke_anthropic_judge(**defaults) + return await invoke_anthropic_judge_async(**defaults) -def test_invoke_anthropic_judge_direct_uses_default_client() -> None: +async def test_invoke_anthropic_judge_direct_uses_default_client() -> None: client = _make_client(_make_response(score=0.42)) - with patch("coder_eval.evaluation.judge_anthropic.Anthropic", return_value=client) as ctor: - result = _invoke() + with patch("coder_eval.evaluation.judge_anthropic.AsyncAnthropic", return_value=client) as ctor: + result = await _invoke() # DirectRoute path: no base_url, no api_key — env-driven. ctor.assert_called_once() kwargs = ctor.call_args.kwargs @@ -64,22 +66,22 @@ def test_invoke_anthropic_judge_direct_uses_default_client() -> None: assert result["content"][0]["type"] == "tool_use" -def test_invoke_anthropic_judge_strips_v1_suffix() -> None: +async def test_invoke_anthropic_judge_strips_v1_suffix() -> None: client = _make_client() - with patch("coder_eval.evaluation.judge_anthropic.Anthropic", return_value=client): - _invoke(model="anthropic.claude-opus-4-6-v1") + with patch("coder_eval.evaluation.judge_anthropic.AsyncAnthropic", return_value=client): + await _invoke(model="anthropic.claude-opus-4-6-v1") assert client.messages.create.call_args.kwargs["model"] == "claude-opus-4-6" -def test_invoke_anthropic_judge_raises_on_empty_model() -> None: +async def test_invoke_anthropic_judge_raises_on_empty_model() -> None: with pytest.raises(ValueError): - _invoke(model="") + await _invoke(model="") -def test_invoke_anthropic_judge_passes_temperature_and_max_tokens() -> None: +async def test_invoke_anthropic_judge_passes_temperature_and_max_tokens() -> None: client = _make_client() - with patch("coder_eval.evaluation.judge_anthropic.Anthropic", return_value=client): - _invoke(temperature=0.7, max_tokens=321, system="sys", user="usr") + with patch("coder_eval.evaluation.judge_anthropic.AsyncAnthropic", return_value=client): + await _invoke(temperature=0.7, max_tokens=321, system="sys", user="usr") kwargs: dict[str, Any] = client.messages.create.call_args.kwargs assert kwargs["temperature"] == 0.7 assert kwargs["max_tokens"] == 321 @@ -87,7 +89,7 @@ def test_invoke_anthropic_judge_passes_temperature_and_max_tokens() -> None: assert kwargs["messages"] == [{"role": "user", "content": "usr"}] -def test_invoke_anthropic_judge_wraps_api_error() -> None: +async def test_invoke_anthropic_judge_wraps_api_error() -> None: import httpx from anthropic import APIConnectionError @@ -97,8 +99,8 @@ def test_invoke_anthropic_judge_wraps_api_error() -> None: sdk_error = APIConnectionError(request=httpx.Request("POST", "https://api.anthropic.com")) client.messages.create.side_effect = sdk_error with ( - patch("coder_eval.evaluation.judge_anthropic.Anthropic", return_value=client), + patch("coder_eval.evaluation.judge_anthropic.AsyncAnthropic", return_value=client), pytest.raises(JudgeInfrastructureError, match="Anthropic judge API error") as excinfo, ): - _invoke() + await _invoke() assert excinfo.value.__cause__ is sdk_error diff --git a/tests/test_judge_bedrock.py b/tests/test_judge_bedrock.py index 1ce7ddcd..da2bc3b6 100644 --- a/tests/test_judge_bedrock.py +++ b/tests/test_judge_bedrock.py @@ -7,13 +7,13 @@ from __future__ import annotations from typing import Any -from unittest.mock import MagicMock +from unittest.mock import AsyncMock, MagicMock import pytest from coder_eval.errors import JudgeInfrastructureError from coder_eval.evaluation import judge_bedrock -from coder_eval.evaluation.judge_bedrock import invoke_bedrock_judge +from coder_eval.evaluation.judge_bedrock import invoke_bedrock_judge_async from coder_eval.evaluation.verdict_tool import SUBMIT_VERDICT_ANTHROPIC_TOOL from coder_eval.models.routing import BedrockRoute @@ -30,7 +30,11 @@ def _make_response(*, status_code: int = 200, json_data: Any = None, text: str = def no_sleep(monkeypatch: pytest.MonkeyPatch) -> list[float]: """Capture backoff sleeps instead of actually sleeping.""" sleeps: list[float] = [] - monkeypatch.setattr(judge_bedrock.time, "sleep", lambda s: sleeps.append(s)) + + async def fake_sleep(s: float) -> None: + sleeps.append(s) + + monkeypatch.setattr(judge_bedrock.asyncio, "sleep", fake_sleep) return sleeps @@ -46,7 +50,16 @@ def _tool_use_response(score: float = 0.5, rationale: str = "ok") -> dict[str, A } -def _invoke(monkeypatch_post, **overrides): # convenience wrapper +def _make_async_client(post_side_effect) -> MagicMock: + """Mock ``httpx.AsyncClient`` — supports the ``async with`` + repeated ``.post(...)`` shape.""" + client = MagicMock() + client.__aenter__ = AsyncMock(return_value=client) + client.__aexit__ = AsyncMock(return_value=None) + client.post = AsyncMock(side_effect=post_side_effect) + return client + + +async def _invoke(**overrides): defaults = { "route": _route(), "model": "anthropic.claude-sonnet-4-6", @@ -57,10 +70,10 @@ def _invoke(monkeypatch_post, **overrides): # convenience wrapper "tool_spec": SUBMIT_VERDICT_ANTHROPIC_TOOL, } defaults.update(overrides) - return invoke_bedrock_judge(**defaults) + return await invoke_bedrock_judge_async(**defaults) -def test_invoke_bedrock_judge_happy_path(monkeypatch: pytest.MonkeyPatch) -> None: +async def test_invoke_bedrock_judge_happy_path(monkeypatch: pytest.MonkeyPatch) -> None: captured: dict[str, Any] = {} def fake_post(url: str, *, headers: dict[str, str], json: dict[str, Any], timeout: float) -> MagicMock: @@ -70,8 +83,8 @@ def fake_post(url: str, *, headers: dict[str, str], json: dict[str, Any], timeou captured["timeout"] = timeout return _make_response(status_code=200, json_data=_tool_use_response(score=0.5)) - monkeypatch.setattr(judge_bedrock.httpx, "post", fake_post) - result = _invoke(monkeypatch, max_tokens=42) + monkeypatch.setattr(judge_bedrock.httpx, "AsyncClient", lambda: _make_async_client(fake_post)) + result = await _invoke(max_tokens=42) assert result["content"][0]["type"] == "tool_use" assert captured["url"] == ( @@ -84,41 +97,45 @@ def fake_post(url: str, *, headers: dict[str, str], json: dict[str, Any], timeou assert captured["json"]["max_tokens"] == 42 -def test_invoke_bedrock_judge_raises_on_4xx(monkeypatch: pytest.MonkeyPatch) -> None: +async def test_invoke_bedrock_judge_raises_on_4xx(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setattr( judge_bedrock.httpx, - "post", - lambda *a, **kw: _make_response(status_code=400, text='{"message":"bad model"}'), + "AsyncClient", + lambda: _make_async_client(lambda *a, **kw: _make_response(status_code=400, text='{"message":"bad model"}')), ) with pytest.raises(JudgeInfrastructureError, match="Bedrock invoke failed: 400") as excinfo: - _invoke(monkeypatch) + await _invoke() assert "bad model" in str(excinfo.value) -def test_invoke_bedrock_judge_raises_on_5xx(monkeypatch: pytest.MonkeyPatch, no_sleep: list[float]) -> None: +async def test_invoke_bedrock_judge_raises_on_5xx(monkeypatch: pytest.MonkeyPatch, no_sleep: list[float]) -> None: calls: list[int] = [] def counting_post(*a: Any, **kw: Any) -> MagicMock: calls.append(1) return _make_response(status_code=500, text="upstream error") - monkeypatch.setattr(judge_bedrock.httpx, "post", counting_post) + monkeypatch.setattr(judge_bedrock.httpx, "AsyncClient", lambda: _make_async_client(counting_post)) with pytest.raises(JudgeInfrastructureError, match="Bedrock invoke failed: 500"): - _invoke(monkeypatch) + await _invoke() # Exactly 1 initial call + max_retries retries — read from the constant, don't hardcode. assert len(calls) == judge_bedrock._JUDGE_RETRY.max_retries + 1 assert len(no_sleep) == judge_bedrock._JUDGE_RETRY.max_retries -def test_invoke_bedrock_judge_raises_on_non_dict_response(monkeypatch: pytest.MonkeyPatch) -> None: - monkeypatch.setattr(judge_bedrock.httpx, "post", lambda *a, **kw: _make_response(json_data=["not a dict"])) +async def test_invoke_bedrock_judge_raises_on_non_dict_response(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr( + judge_bedrock.httpx, + "AsyncClient", + lambda: _make_async_client(lambda *a, **kw: _make_response(json_data=["not a dict"])), + ) with pytest.raises(JudgeInfrastructureError, match="not a JSON object"): - _invoke(monkeypatch) + await _invoke() -def test_invoke_bedrock_judge_raises_on_empty_model() -> None: +async def test_invoke_bedrock_judge_raises_on_empty_model() -> None: with pytest.raises(ValueError): - invoke_bedrock_judge( + await invoke_bedrock_judge_async( route=_route(), model="", system="s", @@ -129,32 +146,36 @@ def test_invoke_bedrock_judge_raises_on_empty_model() -> None: ) -def test_invoke_bedrock_judge_wraps_transport_error(monkeypatch: pytest.MonkeyPatch, no_sleep: list[float]) -> None: +async def test_invoke_bedrock_judge_wraps_transport_error( + monkeypatch: pytest.MonkeyPatch, no_sleep: list[float] +) -> None: import httpx as _httpx def raising_post(*a: Any, **kw: Any) -> MagicMock: raise _httpx.ConnectTimeout("connection timed out") - monkeypatch.setattr(judge_bedrock.httpx, "post", raising_post) + monkeypatch.setattr(judge_bedrock.httpx, "AsyncClient", lambda: _make_async_client(raising_post)) with pytest.raises(JudgeInfrastructureError, match="Bedrock invoke transport error") as excinfo: - _invoke(monkeypatch) + await _invoke() assert "connection timed out" in str(excinfo.value) assert isinstance(excinfo.value.__cause__, _httpx.ConnectTimeout) -def test_invoke_bedrock_judge_strips_v1_suffix_in_url(monkeypatch: pytest.MonkeyPatch) -> None: +async def test_invoke_bedrock_judge_strips_v1_suffix_in_url(monkeypatch: pytest.MonkeyPatch) -> None: captured: dict[str, Any] = {} def fake_post(url: str, **kw: Any) -> MagicMock: captured["url"] = url return _make_response(json_data=_tool_use_response()) - monkeypatch.setattr(judge_bedrock.httpx, "post", fake_post) - _invoke(monkeypatch, model="anthropic.claude-opus-4-6-v1") + monkeypatch.setattr(judge_bedrock.httpx, "AsyncClient", lambda: _make_async_client(fake_post)) + await _invoke(model="anthropic.claude-opus-4-6-v1") assert "/model/eu.anthropic.claude-opus-4-6/invoke" in captured["url"] -def test_invoke_bedrock_judge_retries_429_then_succeeds(monkeypatch: pytest.MonkeyPatch, no_sleep: list[float]) -> None: +async def test_invoke_bedrock_judge_retries_429_then_succeeds( + monkeypatch: pytest.MonkeyPatch, no_sleep: list[float] +) -> None: responses = iter( [ _make_response(status_code=429, text="throttled"), @@ -168,28 +189,30 @@ def sequenced_post(*a: Any, **kw: Any) -> MagicMock: calls.append(1) return next(responses) - monkeypatch.setattr(judge_bedrock.httpx, "post", sequenced_post) - result = _invoke(monkeypatch) + monkeypatch.setattr(judge_bedrock.httpx, "AsyncClient", lambda: _make_async_client(sequenced_post)) + result = await _invoke() assert result["content"][0]["input"]["score"] == 0.9 assert len(calls) == 3 assert len(no_sleep) == 2 -def test_invoke_bedrock_judge_403_fails_immediately(monkeypatch: pytest.MonkeyPatch, no_sleep: list[float]) -> None: +async def test_invoke_bedrock_judge_403_fails_immediately( + monkeypatch: pytest.MonkeyPatch, no_sleep: list[float] +) -> None: calls: list[int] = [] def counting_post(*a: Any, **kw: Any) -> MagicMock: calls.append(1) return _make_response(status_code=403, text="forbidden") - monkeypatch.setattr(judge_bedrock.httpx, "post", counting_post) + monkeypatch.setattr(judge_bedrock.httpx, "AsyncClient", lambda: _make_async_client(counting_post)) with pytest.raises(JudgeInfrastructureError, match="Bedrock invoke failed: 403"): - _invoke(monkeypatch) + await _invoke() assert len(calls) == 1 assert no_sleep == [] -def test_invoke_bedrock_judge_retries_connect_error_then_succeeds( +async def test_invoke_bedrock_judge_retries_connect_error_then_succeeds( monkeypatch: pytest.MonkeyPatch, no_sleep: list[float] ) -> None: import httpx as _httpx @@ -202,18 +225,18 @@ def flaky_post(*a: Any, **kw: Any) -> MagicMock: raise _httpx.ConnectError("connection refused") return _make_response(status_code=200, json_data=_tool_use_response()) - monkeypatch.setattr(judge_bedrock.httpx, "post", flaky_post) - result = _invoke(monkeypatch) + monkeypatch.setattr(judge_bedrock.httpx, "AsyncClient", lambda: _make_async_client(flaky_post)) + result = await _invoke() assert result["content"][0]["type"] == "tool_use" assert len(calls) == 2 assert len(no_sleep) == 1 -def test_invoke_bedrock_judge_malformed_json_body_escalates(monkeypatch: pytest.MonkeyPatch) -> None: +async def test_invoke_bedrock_judge_malformed_json_body_escalates(monkeypatch: pytest.MonkeyPatch) -> None: import json as _json response = _make_response(status_code=200) response.json.side_effect = _json.JSONDecodeError("Expecting value", doc="", pos=0) - monkeypatch.setattr(judge_bedrock.httpx, "post", lambda *a, **kw: response) + monkeypatch.setattr(judge_bedrock.httpx, "AsyncClient", lambda: _make_async_client(lambda *a, **kw: response)) with pytest.raises(JudgeInfrastructureError, match="not valid JSON"): - _invoke(monkeypatch) + await _invoke() diff --git a/tests/test_llm_judge_criterion.py b/tests/test_llm_judge_criterion.py index a18b9318..8abcfea3 100644 --- a/tests/test_llm_judge_criterion.py +++ b/tests/test_llm_judge_criterion.py @@ -6,7 +6,7 @@ from datetime import datetime from pathlib import Path from typing import Any -from unittest.mock import patch +from unittest.mock import AsyncMock, patch import pytest @@ -25,9 +25,9 @@ def _make_judge_response(content: str) -> dict: """Return an Anthropic-shaped response dict the judge's dict extractor parses. - The judge now dispatches through ``invoke_anthropic_judge`` / - ``invoke_bedrock_judge``, both of which return an Anthropic-native message - dict (content blocks). ``extract_verdict_from_anthropic_response`` walks + The judge now dispatches through ``invoke_anthropic_judge_async`` / + ``invoke_bedrock_judge_async``, both of which return an Anthropic-native + message dict (content blocks). ``extract_verdict_from_anthropic_response`` walks ``response["content"]`` for a ``submit_verdict`` ``tool_use`` block and validates its ``input`` against ``JudgeVerdict``. @@ -98,7 +98,7 @@ def test_judge_happy_path_files_only(sandbox: Sandbox, tmp_path: Path) -> None: files=["main.py"], ) resp = _make_judge_response('{"score": 0.85, "rationale": "mostly correct"}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp): + with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp)): checker = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()) result = checker.check(criterion) @@ -107,10 +107,35 @@ def test_judge_happy_path_files_only(sandbox: Sandbox, tmp_path: Path) -> None: assert "mostly correct" in (result.details or "") +async def test_judge_happy_path_via_check_all_async(sandbox: Sandbox, tmp_path: Path) -> None: + """Same happy path, but through check_all_async — the orchestrator's ACTUAL + entry point (orchestrator.py awaits it directly on the live loop). Every + other test in this file drives the derived sync `.check()` bridge + (`_check_impl` -> `asyncio.run(_check_impl_async(...))` on a fresh loop), + which production never takes for this criterion — this is the one test + that exercises llm_judge on the loop it actually runs on.""" + (tmp_path / "main.py").write_text("print('hello')") + + criterion = LLMJudgeCriterion( + description="grade main.py", + prompt="Is this code valid?", + files=["main.py"], + ) + resp = _make_judge_response('{"score": 0.85, "rationale": "mostly correct"}') + with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp)): + checker = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()) + results = await checker.check_all_async([criterion]) + + assert len(results) == 1 + assert results[0].score == 0.85 + assert results[0].error is None + assert "mostly correct" in (results[0].details or "") + + def test_judge_score_clamped_high(sandbox: Sandbox) -> None: criterion = LLMJudgeCriterion(description="x", prompt="grade") resp = _make_judge_response('{"score": 1.7, "rationale": "too high"}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp): + with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp)): result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion) assert result.score == 1.0 @@ -118,7 +143,7 @@ def test_judge_score_clamped_high(sandbox: Sandbox) -> None: def test_judge_score_clamped_low(sandbox: Sandbox) -> None: criterion = LLMJudgeCriterion(description="x", prompt="grade") resp = _make_judge_response('{"score": -0.3, "rationale": "too low"}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp): + with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp)): result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion) assert result.score == 0.0 @@ -127,7 +152,7 @@ def test_judge_no_tool_call_maps_to_score_zero(sandbox: Sandbox) -> None: """When the model emits text instead of a submit_verdict tool call, score=0 with diagnostic.""" criterion = LLMJudgeCriterion(description="x", prompt="grade") resp = _make_judge_response("not json at all") - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp): + with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp)): result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion) assert result.score == 0.0 assert result.error == "Judge did not call submit_verdict" @@ -136,7 +161,7 @@ def test_judge_no_tool_call_maps_to_score_zero(sandbox: Sandbox) -> None: def test_judge_non_numeric_score(sandbox: Sandbox) -> None: criterion = LLMJudgeCriterion(description="x", prompt="grade") resp = _make_judge_response('{"score": "great", "rationale": "oops"}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp): + with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp)): result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion) assert result.score == 0.0 assert result.error is not None @@ -146,7 +171,7 @@ def test_judge_non_numeric_score(sandbox: Sandbox) -> None: def test_judge_missing_score_key(sandbox: Sandbox) -> None: criterion = LLMJudgeCriterion(description="x", prompt="grade") resp = _make_judge_response('{"rationale": "forgot score"}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp): + with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp)): result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion) assert result.score == 0.0 assert result.error is not None @@ -162,7 +187,7 @@ def test_judge_rejects_non_finite_score(sandbox: Sandbox, raw_score: str) -> Non """ criterion = LLMJudgeCriterion(description="x", prompt="grade") resp = _make_judge_response(f'{{"score": {raw_score}, "rationale": "oops"}}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp): + with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp)): result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion) assert result.score == 0.0 assert result.error is not None @@ -181,7 +206,9 @@ def test_judge_missing_file_marker(sandbox: Sandbox, tmp_path: Path) -> None: files=["present.py", "missing.py"], ) resp = _make_judge_response('{"score": 0.5, "rationale": "partial"}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp) as m_anthropic: + with patch( + "coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp) + ) as m_anthropic: result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion) user_msg = m_anthropic.call_args.kwargs["user"] @@ -192,8 +219,10 @@ def test_judge_missing_file_marker(sandbox: Sandbox, tmp_path: Path) -> None: def test_judge_llm_exception_maps_to_score_zero(sandbox: Sandbox) -> None: criterion = LLMJudgeCriterion(description="x", prompt="grade") with patch( - "coder_eval.criteria.llm_judge.invoke_anthropic_judge", - side_effect=RuntimeError("gateway down"), + "coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", + new=AsyncMock( + side_effect=RuntimeError("gateway down"), + ), ): result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion) assert result.score == 0.0 @@ -209,7 +238,9 @@ def test_judge_include_reference_true_keeps_reference_in_prompt_only(sandbox: Sa include_reference=True, ) resp = _make_judge_response('{"score": 0.7, "rationale": "ok"}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp) as m_anthropic: + with patch( + "coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp) + ) as m_anthropic: result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check( criterion, reference_code=sentinel ) @@ -227,7 +258,9 @@ def test_judge_include_reference_true_no_reference_set(sandbox: Sandbox) -> None include_reference=True, ) resp = _make_judge_response('{"score": 0.4, "rationale": "no ref"}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp) as m_anthropic: + with patch( + "coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp) + ) as m_anthropic: result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion, reference_code=None) user_msg = m_anthropic.call_args.kwargs["user"] @@ -241,7 +274,9 @@ def test_judge_include_reference_false_omits_reference(sandbox: Sandbox) -> None # is for non-judge consumers (reference_comparison) and shouldn't reach the LLM. criterion = LLMJudgeCriterion(description="x", prompt="grade", include_reference=False) resp = _make_judge_response('{"score": 0.5, "rationale": "ok"}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp) as m_anthropic: + with patch( + "coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp) + ) as m_anthropic: SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion, reference_code=sentinel) user_msg = m_anthropic.call_args.kwargs["user"] @@ -252,7 +287,9 @@ def test_judge_include_agent_output_true(sandbox: Sandbox) -> None: criterion = LLMJudgeCriterion(description="x", prompt="grade", include_agent_output=True) turn = _make_turn(agent_output="I did X") resp = _make_judge_response('{"score": 0.6, "rationale": "ok"}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp) as m_anthropic: + with patch( + "coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp) + ) as m_anthropic: SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion, turn_records=[turn]) user_msg = m_anthropic.call_args.kwargs["user"] @@ -265,7 +302,9 @@ def test_judge_include_tool_calls_true(sandbox: Sandbox) -> None: cmd = _make_cmd(tool_name="Bash", params={"command": "ls"}) turn = _make_turn(commands=[cmd]) resp = _make_judge_response('{"score": 0.55, "rationale": "ok"}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp) as m_anthropic: + with patch( + "coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp) + ) as m_anthropic: SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion, turn_records=[turn]) user_msg = m_anthropic.call_args.kwargs["user"] @@ -281,7 +320,9 @@ def test_judge_include_dialog_renders_all_turns(sandbox: Sandbox) -> None: TurnRecord(iteration=2, user_input="make it red", agent_output="done"), ] resp = _make_judge_response('{"score": 0.8, "rationale": "ok"}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp) as m_anthropic: + with patch( + "coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp) + ) as m_anthropic: SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion, turn_records=turns) user_msg = m_anthropic.call_args.kwargs["user"] @@ -297,7 +338,9 @@ def test_judge_include_dialog_omitted_when_false(sandbox: Sandbox) -> None: criterion = LLMJudgeCriterion(description="x", prompt="grade") turn = TurnRecord(iteration=1, user_input="add a button", agent_output="added") resp = _make_judge_response('{"score": 0.8, "rationale": "ok"}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp) as m_anthropic: + with patch( + "coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp) + ) as m_anthropic: SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion, turn_records=[turn]) user_msg = m_anthropic.call_args.kwargs["user"] @@ -308,7 +351,9 @@ def test_judge_include_dialog_omitted_when_false(sandbox: Sandbox) -> None: def test_judge_include_dialog_no_turns_records_degraded_note(sandbox: Sandbox) -> None: criterion = LLMJudgeCriterion(description="x", prompt="grade", include_dialog=True) resp = _make_judge_response('{"score": 0.5, "rationale": "ok"}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp) as m_anthropic: + with patch( + "coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp) + ) as m_anthropic: result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion, turn_records=None) user_msg = m_anthropic.call_args.kwargs["user"] @@ -324,7 +369,7 @@ def test_judge_trajectory_toggles_without_turn_records(sandbox: Sandbox) -> None include_tool_calls=True, ) resp = _make_judge_response('{"score": 0.3, "rationale": "ok"}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp): + with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp)): result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion, turn_records=None) assert result.error is None @@ -338,7 +383,9 @@ def test_judge_trajectory_toggles_with_empty_commands(sandbox: Sandbox) -> None: criterion = LLMJudgeCriterion(description="x", prompt="grade", include_tool_calls=True) turn = _make_turn(commands=[]) resp = _make_judge_response('{"score": 0.4, "rationale": "ok"}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp) as m_anthropic: + with patch( + "coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp) + ) as m_anthropic: SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion, turn_records=[turn]) user_msg = m_anthropic.call_args.kwargs["user"] @@ -360,7 +407,9 @@ def test_judge_file_truncation(sandbox: Sandbox, tmp_path: Path) -> None: max_file_chars=limit, ) resp = _make_judge_response('{"score": 0.5, "rationale": "ok"}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp) as m_anthropic: + with patch( + "coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp) + ) as m_anthropic: SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion) user_msg = m_anthropic.call_args.kwargs["user"] @@ -384,7 +433,9 @@ def test_judge_agent_output_truncation(sandbox: Sandbox) -> None: ) turn = _make_turn(agent_output=long_output) resp = _make_judge_response('{"score": 0.2, "rationale": "ok"}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp) as m_anthropic: + with patch( + "coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp) + ) as m_anthropic: SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion, turn_records=[turn]) user_msg = m_anthropic.call_args.kwargs["user"] @@ -408,7 +459,7 @@ def test_judge_counts_toward_weighted_score(sandbox: Sandbox, tmp_path: Path) -> weight=2.0, ) resp = _make_judge_response('{"score": 0.5, "rationale": "half"}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp): + with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp)): checker = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()) results = checker.check_all([fe, judge]) @@ -438,7 +489,7 @@ def test_judge_reference_not_in_details(sandbox: Sandbox) -> None: sentinel = "REFERENCE_LEAK_CANARY_XYZ" criterion = LLMJudgeCriterion(description="x", prompt="grade", include_reference=True) resp = _make_judge_response('{"score": 0.9, "rationale": "great"}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp): + with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp)): result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check( criterion, reference_code=sentinel ) @@ -458,7 +509,7 @@ def test_judge_parse_error_scrubs_reference_from_error_field(sandbox: Sandbox) - sentinel = "REFERENCE_LEAK_VIA_ERROR_456" criterion = LLMJudgeCriterion(description="x", prompt="grade", include_reference=True) resp = _make_judge_response(f'{{"score": "{sentinel}", "rationale": "ok"}}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp): + with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp)): result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check( criterion, reference_code=sentinel ) @@ -473,7 +524,7 @@ def test_judge_reference_not_leaked_on_parse_failure(sandbox: Sandbox) -> None: criterion = LLMJudgeCriterion(description="x", prompt="grade", include_reference=True) # Unparseable response that mentions the reference — simulates a misbehaving model. resp = _make_judge_response(f"Sorry, here is what you gave me: {sentinel}. no json") - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp): + with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp)): result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check( criterion, reference_code=sentinel ) @@ -490,8 +541,8 @@ def test_judge_no_route_is_unconfigured(sandbox: Sandbox) -> None: """No route -> UNCONFIGURED arm; neither invoker is called.""" criterion = LLMJudgeCriterion(description="x", prompt="grade") with ( - patch("coder_eval.criteria.llm_judge.invoke_bedrock_judge") as m_bedrock, - patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge") as m_anthropic, + patch("coder_eval.criteria.llm_judge.invoke_bedrock_judge_async", new=AsyncMock()) as m_bedrock, + patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock()) as m_anthropic, ): result = SuccessChecker(sandbox, init_registry=False).check(criterion) assert result.score == 0.0 @@ -512,8 +563,10 @@ def test_judge_bedrock_route_uses_bedrock_invoker(sandbox: Sandbox) -> None: route = BedrockRoute(bearer_token="t", region="eu-north-1") criterion = LLMJudgeCriterion(description="x", prompt="grade") with ( - patch("coder_eval.criteria.llm_judge.invoke_bedrock_judge", return_value=_tool_use_block(0.7)) as m_bedrock, - patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge") as m_anthropic, + patch( + "coder_eval.criteria.llm_judge.invoke_bedrock_judge_async", new=AsyncMock(return_value=_tool_use_block(0.7)) + ) as m_bedrock, + patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock()) as m_anthropic, ): result = SuccessChecker(sandbox, init_registry=False, route=route).check(criterion) assert result.score == 0.7 @@ -533,8 +586,11 @@ def test_judge_direct_route_uses_anthropic_invoker(sandbox: Sandbox) -> None: route = DirectRoute() criterion = LLMJudgeCriterion(description="x", prompt="grade") with ( - patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=_tool_use_block(0.5)) as m_anthropic, - patch("coder_eval.criteria.llm_judge.invoke_bedrock_judge") as m_bedrock, + patch( + "coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", + new=AsyncMock(return_value=_tool_use_block(0.5)), + ) as m_anthropic, + patch("coder_eval.criteria.llm_judge.invoke_bedrock_judge_async", new=AsyncMock()) as m_bedrock, ): result = SuccessChecker(sandbox, init_registry=False, route=route).check(criterion) assert result.score == 0.5 @@ -548,8 +604,10 @@ def test_judge_bedrock_invoke_runtime_error_maps_to_score_zero(sandbox: Sandbox) route = BedrockRoute(bearer_token="t", region="eu-north-1") criterion = LLMJudgeCriterion(description="x", prompt="grade") with patch( - "coder_eval.criteria.llm_judge.invoke_bedrock_judge", - side_effect=RuntimeError("Bedrock invoke failed: 403 forbidden"), + "coder_eval.criteria.llm_judge.invoke_bedrock_judge_async", + new=AsyncMock( + side_effect=RuntimeError("Bedrock invoke failed: 403 forbidden"), + ), ): result = SuccessChecker(sandbox, init_registry=False, route=route).check(criterion) assert result.score == 0.0 @@ -563,8 +621,10 @@ def test_judge_anthropic_invoke_runtime_error_maps_to_score_zero(sandbox: Sandbo route = DirectRoute() criterion = LLMJudgeCriterion(description="x", prompt="grade") with patch( - "coder_eval.criteria.llm_judge.invoke_anthropic_judge", - side_effect=RuntimeError("Anthropic API connection refused"), + "coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", + new=AsyncMock( + side_effect=RuntimeError("Anthropic API connection refused"), + ), ): result = SuccessChecker(sandbox, init_registry=False, route=route).check(criterion) assert result.score == 0.0 @@ -587,8 +647,8 @@ def test_judge_direct_route_with_no_transport_fails_with_clear_error(sandbox: Sa route = DirectRoute(judge_transport=None) criterion = LLMJudgeCriterion(description="x", prompt="grade") with ( - patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge") as m_anthropic, - patch("coder_eval.criteria.llm_judge.invoke_bedrock_judge") as m_bedrock, + patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock()) as m_anthropic, + patch("coder_eval.criteria.llm_judge.invoke_bedrock_judge_async", new=AsyncMock()) as m_bedrock, ): result = SuccessChecker(sandbox, init_registry=False, route=route).check(criterion) assert isinstance(result, JudgeCriterionResult) @@ -635,8 +695,10 @@ def test_judge_bedrock_route_threads_model_unchanged(sandbox: Sandbox) -> None: route = BedrockRoute(bearer_token="t", region="eu-north-1") criterion = LLMJudgeCriterion(description="x", prompt="grade", model="anthropic.claude-opus-4-6-v1") with patch( - "coder_eval.criteria.llm_judge.invoke_bedrock_judge", - return_value=_tool_use_block(0.9), + "coder_eval.criteria.llm_judge.invoke_bedrock_judge_async", + new=AsyncMock( + return_value=_tool_use_block(0.9), + ), ) as m_bedrock: SuccessChecker(sandbox, init_registry=False, route=route).check(criterion) assert m_bedrock.call_args.kwargs["model"] == "anthropic.claude-opus-4-6-v1" @@ -655,7 +717,7 @@ def test_judge_empty_rationale_returns_judge_result_with_error(sandbox: Sandbox) criterion = LLMJudgeCriterion(description="x", prompt="grade") resp = _make_judge_response('{"score": 0.5, "rationale": " "}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp): + with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp)): result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion) assert isinstance(result, JudgeCriterionResult) @@ -671,7 +733,7 @@ def test_judge_persists_findings(sandbox: Sandbox) -> None: '"findings": ["main.py:5 missing return — issue", "no docstrings — minor deviation"]}' ) resp = _make_judge_response(verdict) - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp): + with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp)): result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion) assert result.score == 0.7 @@ -685,7 +747,9 @@ def test_judge_prompt_requires_findings(sandbox: Sandbox) -> None: """The system + user prompts must instruct the model to emit findings.""" criterion = LLMJudgeCriterion(description="x", prompt="grade") resp = _make_judge_response('{"score": 0.5, "rationale": "ok"}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp) as m_anthropic: + with patch( + "coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp) + ) as m_anthropic: SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion) kwargs = m_anthropic.call_args.kwargs @@ -702,7 +766,7 @@ def test_judge_transcript_captures_raw_verdict_by_default(sandbox: Sandbox) -> N """Tool-channel transcript stores the JSON-dumped verdict (no internal spaces) as raw_verdict.""" criterion = LLMJudgeCriterion(description="x", prompt="grade") resp = _make_judge_response('{"score": 0.5, "rationale": "ok", "findings": ["a"]}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp): + with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp)): result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion) transcript = getattr(result, "transcript", None) @@ -716,7 +780,7 @@ def test_judge_transcript_captures_raw_verdict_by_default(sandbox: Sandbox) -> N def test_judge_capture_transcript_false_drops_transcript(sandbox: Sandbox) -> None: criterion = LLMJudgeCriterion(description="x", prompt="grade", capture_transcript=False) resp = _make_judge_response('{"score": 0.5, "rationale": "ok"}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp): + with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp)): result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion) assert getattr(result, "transcript", None) is None @@ -729,7 +793,7 @@ def test_judge_transcript_truncation_marks_truncated(sandbox: Sandbox) -> None: raw = f'{{"score": 0.5, "rationale": "ok", "findings": ["{big_finding}"]}}' criterion = LLMJudgeCriterion(description="x", prompt="grade", max_transcript_chars=200) resp = _make_judge_response(raw) - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp): + with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp)): result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion) transcript = getattr(result, "transcript", None) @@ -744,7 +808,7 @@ def test_judge_scrubs_reference_from_findings(sandbox: Sandbox) -> None: raw = f'{{"score": 0.5, "rationale": "ok", "findings": ["echoed {sentinel} in main.py"]}}' criterion = LLMJudgeCriterion(description="x", prompt="grade", include_reference=True) resp = _make_judge_response(raw) - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp): + with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp)): result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check( criterion, reference_code=sentinel ) @@ -761,7 +825,7 @@ def test_judge_legacy_two_field_verdict_still_parses(sandbox: Sandbox) -> None: form must still parse — findings defaults empty, score/rationale stand.""" criterion = LLMJudgeCriterion(description="x", prompt="grade") resp = _make_judge_response('{"score": 0.42, "rationale": "ok"}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp): + with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp)): result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion) assert result.score == 0.42 @@ -773,7 +837,7 @@ def test_judge_no_tool_call_still_carries_transcript(sandbox: Sandbox) -> None: """Even when the model emits no submit_verdict call, the transcript records the diagnostic.""" criterion = LLMJudgeCriterion(description="x", prompt="grade") resp = _make_judge_response("totally not json") - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp): + with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp)): result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion) assert result.score == 0.0 @@ -789,7 +853,9 @@ def test_judge_enabled_false_short_circuits(sandbox: Sandbox) -> None: """enabled=False: no LLM call, returns a skipped result with score=1.0.""" criterion = LLMJudgeCriterion(description="x", prompt="grade", enabled=False) resp = _make_judge_response('{"score": 0.5, "rationale": "should not be called"}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp) as m_anthropic: + with patch( + "coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp) + ) as m_anthropic: result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion) assert result.score == 1.0 @@ -811,7 +877,7 @@ def test_judge_transcript_captures_prompts(sandbox: Sandbox) -> None: can see exactly what the judge was told.""" criterion = LLMJudgeCriterion(description="x", prompt="rubric body XYZ") resp = _make_judge_response('{"score": 0.5, "rationale": "ok"}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp): + with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp)): result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion) transcript = getattr(result, "transcript", None) @@ -826,7 +892,9 @@ def test_judge_prompt_capture_scrubs_reference(sandbox: Sandbox) -> None: sentinel = "REF_LEAK_VIA_PROMPT_111" criterion = LLMJudgeCriterion(description="x", prompt="grade", include_reference=True) resp = _make_judge_response('{"score": 0.5, "rationale": "ok"}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp) as m_anthropic: + with patch( + "coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp) + ) as m_anthropic: result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check( criterion, reference_code=sentinel ) @@ -849,7 +917,9 @@ def test_judge_agent_output_empty_does_not_emit_block(sandbox: Sandbox) -> None: criterion = LLMJudgeCriterion(description="x", prompt="grade", include_agent_output=True) turn = _make_turn(agent_output="") resp = _make_judge_response('{"score": 0.5, "rationale": "ok"}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp) as m_anthropic: + with patch( + "coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp) + ) as m_anthropic: result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion, turn_records=[turn]) user_msg = m_anthropic.call_args.kwargs["user"] @@ -865,7 +935,7 @@ def test_judge_agent_output_empty_does_not_emit_block(sandbox: Sandbox) -> None: def test_llm_judge_tool_channel_happy_path(sandbox: Sandbox) -> None: criterion = LLMJudgeCriterion(description="x", prompt="grade") resp = _make_judge_response('{"score": 0.6, "rationale": "ok", "findings": ["a"]}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp): + with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp)): result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion) assert result.score == 0.6 assert result.error is None @@ -875,7 +945,7 @@ def test_llm_judge_tool_channel_did_not_call(sandbox: Sandbox) -> None: criterion = LLMJudgeCriterion(description="x", prompt="grade") # Model returned text only, no tool_use block. resp = {"content": [{"type": "text", "text": "no verdict here"}]} - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp): + with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp)): result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion) assert result.score == 0.0 assert result.error == "Judge did not call submit_verdict" @@ -884,7 +954,7 @@ def test_llm_judge_tool_channel_did_not_call(sandbox: Sandbox) -> None: def test_llm_judge_tool_channel_invalid_args(sandbox: Sandbox) -> None: criterion = LLMJudgeCriterion(description="x", prompt="grade") resp = {"content": [{"type": "tool_use", "name": "submit_verdict", "input": {"score": "not numeric"}}]} - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp): + with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp)): result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion) assert result.score == 0.0 assert result.error is not None @@ -899,7 +969,7 @@ def test_llm_judge_tool_channel_last_call_wins(sandbox: Sandbox) -> None: {"type": "tool_use", "name": "submit_verdict", "input": {"score": 0.9, "rationale": "second"}}, ] } - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp): + with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp)): result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion) assert result.score == 0.9 @@ -908,7 +978,7 @@ def test_llm_judge_tool_channel_transcript_carries_structured_verdict(sandbox: S """Tool channel transcript stores the JSON-dumped verdict, not the agent's raw text.""" criterion = LLMJudgeCriterion(description="x", prompt="grade") resp = _make_judge_response('{"score": 0.42, "rationale": "headline", "findings": ["f1"]}') - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp): + with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp)): result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion) transcript = getattr(result, "transcript", None) assert transcript is not None @@ -934,7 +1004,9 @@ def test_llm_judge_tool_channel_bedrock(sandbox: Sandbox) -> None: {"type": "tool_use", "name": "submit_verdict", "input": {"score": 0.81, "rationale": "ok"}}, ] } - with patch("coder_eval.criteria.llm_judge.invoke_bedrock_judge", return_value=bedrock_response) as mock_invoke: + with patch( + "coder_eval.criteria.llm_judge.invoke_bedrock_judge_async", new=AsyncMock(return_value=bedrock_response) + ) as mock_invoke: result = SuccessChecker( sandbox, init_registry=False, route=BedrockRoute(bearer_token="t", region="us-east-1") ).check(criterion) @@ -946,7 +1018,7 @@ def test_llm_judge_tool_channel_bedrock(sandbox: Sandbox) -> None: def test_llm_judge_tool_channel_anthropic_direct(sandbox: Sandbox) -> None: - """DirectRoute (anthropic transport) uses invoke_anthropic_judge returning a dict.""" + """DirectRoute (anthropic transport) uses invoke_anthropic_judge_async returning a dict.""" from coder_eval.models.routing import DirectRoute criterion = LLMJudgeCriterion(description="x", prompt="grade") @@ -955,7 +1027,9 @@ def test_llm_judge_tool_channel_anthropic_direct(sandbox: Sandbox) -> None: {"type": "tool_use", "name": "submit_verdict", "input": {"score": 0.33, "rationale": "ok"}}, ] } - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=anthropic_dict) as mock_invoke: + with patch( + "coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=anthropic_dict) + ) as mock_invoke: result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute(judge_transport="anthropic")).check( criterion ) @@ -992,7 +1066,7 @@ def test_judge_usage_direct_anthropic_from_response(sandbox: Sandbox) -> None: criterion = LLMJudgeCriterion(description="x", prompt="grade") resp = _anthropic_response(score=0.7, usage={"input_tokens": 1234, "output_tokens": 56}) - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp): + with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp)): result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute(judge_transport="anthropic")).check( criterion ) @@ -1011,7 +1085,7 @@ def test_judge_usage_bedrock_from_response(sandbox: Sandbox) -> None: resp = _anthropic_response( score=0.6, usage={"input_tokens": 900, "output_tokens": 40, "cache_read_input_tokens": 100} ) - with patch("coder_eval.criteria.llm_judge.invoke_bedrock_judge", return_value=resp): + with patch("coder_eval.criteria.llm_judge.invoke_bedrock_judge_async", new=AsyncMock(return_value=resp)): result = SuccessChecker( sandbox, init_registry=False, route=BedrockRoute(bearer_token="t", region="us-east-1") ).check(criterion) @@ -1029,7 +1103,7 @@ def test_judge_usage_none_when_response_has_no_usage(sandbox: Sandbox) -> None: criterion = LLMJudgeCriterion(description="x", prompt="grade") resp = _anthropic_response(score=0.7, usage=None) - with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge", return_value=resp): + with patch("coder_eval.criteria.llm_judge.invoke_anthropic_judge_async", new=AsyncMock(return_value=resp)): result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute(judge_transport="anthropic")).check( criterion ) diff --git a/tests/test_orchestrator.py b/tests/test_orchestrator.py index be8de5e9..7eee4c19 100644 --- a/tests/test_orchestrator.py +++ b/tests/test_orchestrator.py @@ -1658,7 +1658,7 @@ async def test_evaluation_loop_breaks_on_max_turns_exhausted(tmp_path): # Mock success checker that always fails mock_checker = MagicMock() - mock_checker.check_all = MagicMock( + mock_checker.check_all_async = AsyncMock( return_value=[CriterionResult(criterion_type="file_exists", description="test", score=0.0)] ) orchestrator.success_checker = mock_checker @@ -1775,7 +1775,7 @@ async def crash_then_succeed_impl(_prompt, **kwargs): orchestrator.sandbox = mock_sandbox mock_checker = MagicMock() - mock_checker.check_all = MagicMock( + mock_checker.check_all_async = AsyncMock( return_value=[CriterionResult(criterion_type="file_exists", description="x", score=1.0)] ) orchestrator.success_checker = mock_checker @@ -1878,7 +1878,7 @@ async def timeout_impl(_prompt, **kwargs): orchestrator.sandbox = mock_sandbox mock_checker = MagicMock() - mock_checker.check_all = MagicMock( + mock_checker.check_all_async = AsyncMock( return_value=[CriterionResult(criterion_type="file_exists", description="x", score=1.0)] ) orchestrator.success_checker = mock_checker @@ -2110,7 +2110,7 @@ async def test_evaluation_loop_evaluate_only_loads_reference(tmp_path): "include_reference=True but reference not set" in the judge_context log. """ from datetime import datetime - from unittest.mock import MagicMock + from unittest.mock import AsyncMock, MagicMock from coder_eval.models import ( CriterionResult, @@ -2170,15 +2170,15 @@ async def test_evaluation_loop_evaluate_only_loads_reference(tmp_path): assert orchestrator.agent is None mock_checker = MagicMock() - mock_checker.check_all = MagicMock( + mock_checker.check_all_async = AsyncMock( return_value=[CriterionResult(criterion_type="file_exists", description="x", score=1.0)] ) orchestrator.success_checker = mock_checker await orchestrator._evaluation_loop() - mock_checker.check_all.assert_called_once() - kwargs = mock_checker.check_all.call_args.kwargs + mock_checker.check_all_async.assert_called_once() + kwargs = mock_checker.check_all_async.call_args.kwargs assert kwargs["reference_code"] == "REFERENCE_CONTENT" # turn_records is empty in evaluate-only mode but the kwarg should still be wired. assert kwargs["turn_records"] == [] diff --git a/tests/test_reference_evaluator.py b/tests/test_reference_evaluator.py index 5a733ee5..2d33bebc 100644 --- a/tests/test_reference_evaluator.py +++ b/tests/test_reference_evaluator.py @@ -1,5 +1,7 @@ """Tests for evaluator reference code support.""" +import pytest + from coder_eval.evaluation.checker import SuccessChecker from coder_eval.models import ( ReferenceComparisonCriterion, @@ -26,6 +28,28 @@ def test_check_all_accepts_reference_code(self, tmp_path): sandbox.cleanup(preserve=False) + @pytest.mark.asyncio + async def test_check_all_async_persists_reference_code_and_dir(self, tmp_path): + """check_all_async — the orchestrator's actual entry point — must persist + reference_code/reference_dir the same way the sync check_all does, so a + later check()/check_all() call on the same checker without an explicit + reference still sees it.""" + config = SandboxConfig(driver="tempdir", python=None) + sandbox = Sandbox(config, "test") + sandbox.setup() + + checker = SuccessChecker(sandbox) + reference_code = "def foo(): pass" + reference_dir = tmp_path / "ref" + reference_dir.mkdir() + + results = await checker.check_all_async([], reference_code=reference_code, reference_dir=reference_dir) + assert results == [] + assert checker._reference_code == reference_code + assert checker._reference_dir == reference_dir + + sandbox.cleanup(preserve=False) + def test_reference_comparison_without_reference(self, tmp_path): """reference_comparison fails without reference code.""" config = SandboxConfig(driver="tempdir", python=None) diff --git a/tests/test_registry.py b/tests/test_registry.py index d554e5dc..3db67087 100644 --- a/tests/test_registry.py +++ b/tests/test_registry.py @@ -58,14 +58,25 @@ def test_all_criterion_checkers_accept_context_kwarg(): import inspect from coder_eval.criteria import CriterionRegistry, init_criteria + from coder_eval.criteria.base import BaseCriterion init_criteria(validate=False) checkers = {t: CriterionRegistry.get_checker(t) for t in CriterionRegistry.list_types()} assert checkers, "registry discovered no checkers" for criterion_type, checker_cls in checkers.items(): - params = inspect.signature(checker_cls._check_impl).parameters + # Inspect whichever of _check_impl / _check_impl_async this checker actually + # overrides (see BaseCriterion) — a checker that overrides only the async + # surface (llm_judge, agent_judge) inherits BaseCriterion._check_impl + # unchanged, so inspecting that would vacuously pass regardless of what the + # checker's real override declares. + impl = ( + checker_cls._check_impl + if checker_cls._check_impl is not BaseCriterion._check_impl + else (checker_cls._check_impl_async) + ) + params = inspect.signature(impl).parameters assert "context" in params, ( - f"{checker_cls.__name__} (_check_impl for '{criterion_type}') missing 'context' kwarg" + f"{checker_cls.__name__} (checking impl for '{criterion_type}') missing 'context' kwarg" ) # No checker should still declare the removed route/reference_dir/proxy kwargs. for removed in ("route", "reference_dir", "proxy"): diff --git a/tests/test_route_seam_exhaustiveness.py b/tests/test_route_seam_exhaustiveness.py index e1b41f19..45265fb1 100644 --- a/tests/test_route_seam_exhaustiveness.py +++ b/tests/test_route_seam_exhaustiveness.py @@ -58,17 +58,24 @@ def test_format_routing_handles_every_route(): assert out and out.startswith(ROUTE_NAMES[type(r)]) -def test_invoke_tool_channel_handles_every_route(monkeypatch): +async def test_invoke_tool_channel_handles_every_route(monkeypatch): """The llm_judge dispatch must handle every route type — an unhandled member would fall past all cases and raise UnboundLocalError. Network-touching arms (Bedrock/Direct) are stubbed so this only checks dispatch coverage.""" - monkeypatch.setattr(llm_judge, "invoke_bedrock_judge", lambda **_: {}) - monkeypatch.setattr(llm_judge, "invoke_anthropic_judge", lambda **_: {}) + + async def _stub_bedrock(**_: object) -> dict[str, object]: + return {} + + async def _stub_anthropic(**_: object) -> dict[str, object]: + return {} + + monkeypatch.setattr(llm_judge, "invoke_bedrock_judge_async", _stub_bedrock) + monkeypatch.setattr(llm_judge, "invoke_anthropic_judge_async", _stub_anthropic) monkeypatch.setattr(llm_judge, "extract_verdict_from_anthropic_response", lambda _resp: (None, "stub")) monkeypatch.setattr(llm_judge, "token_usage_from_anthropic_dict", lambda _resp: None) criterion = MagicMock() for r in _INSTANCES: - result = _invoke_tool_channel(criterion=criterion, route=r, system_msg="s", user_msg="u") # type: ignore[arg-type] + result = await _invoke_tool_channel(criterion=criterion, route=r, system_msg="s", user_msg="u") # type: ignore[arg-type] # (verdict, parse_error, raw_text, response_usage) — a 4-tuple means the # route matched an explicit arm rather than falling through. assert isinstance(result, tuple) and len(result) == 4 diff --git a/tests/test_run_limits_orchestrator.py b/tests/test_run_limits_orchestrator.py index 77ca2394..b586e17e 100644 --- a/tests/test_run_limits_orchestrator.py +++ b/tests/test_run_limits_orchestrator.py @@ -225,7 +225,7 @@ async def _run_eval_loop_with_turn(self, task: TaskDefinition, tmp_path, turn: T orch.agent = mock_agent mock_checker = MagicMock() - mock_checker.check_all = MagicMock( + mock_checker.check_all_async = AsyncMock( return_value=[CriterionResult(criterion_type="file_exists", description="x", score=1.0)] ) orch.success_checker = mock_checker @@ -361,7 +361,7 @@ async def test_dialog_aborts_with_run_limit_stop_reason(self, tmp_path): orch.agent = mock_agent mock_checker = MagicMock() - mock_checker.check_all = MagicMock( + mock_checker.check_all_async = AsyncMock( return_value=[CriterionResult(criterion_type="file_exists", description="x", score=0.0)] ) orch.success_checker = mock_checker @@ -475,7 +475,7 @@ async def test_warning_does_not_abort_run(self, tmp_path, caplog): mock_agent.communicate = AsyncMock(return_value=turn) orch.agent = mock_agent mock_checker = MagicMock() - mock_checker.check_all = MagicMock( + mock_checker.check_all_async = AsyncMock( return_value=[CriterionResult(criterion_type="file_exists", description="x", score=1.0)] ) orch.success_checker = mock_checker @@ -521,7 +521,7 @@ async def test_warning_fires_in_simulation_and_does_not_abort(self, tmp_path, ca orch.agent = mock_agent mock_checker = MagicMock() - mock_checker.check_all = MagicMock( + mock_checker.check_all_async = AsyncMock( return_value=[CriterionResult(criterion_type="file_exists", description="x", score=1.0)] ) orch.success_checker = mock_checker @@ -574,7 +574,7 @@ async def test_warning_fires_when_single_simulation_turn_exceeds(self, tmp_path, orch.agent = mock_agent mock_checker = MagicMock() - mock_checker.check_all = MagicMock( + mock_checker.check_all_async = AsyncMock( return_value=[CriterionResult(criterion_type="file_exists", description="x", score=1.0)] ) orch.success_checker = mock_checker diff --git a/tests/test_sub_agent_runner.py b/tests/test_sub_agent_runner.py index 00d8fe3b..a40a31aa 100644 --- a/tests/test_sub_agent_runner.py +++ b/tests/test_sub_agent_runner.py @@ -67,7 +67,7 @@ def _make_mock_agent() -> MagicMock: # --- happy path + sandbox isolation --- -def test_runner_happy_path(sandbox: Sandbox, tmp_path: Path) -> None: +async def test_runner_happy_path(sandbox: Sandbox, tmp_path: Path) -> None: (tmp_path / "Main.xaml").write_text("") runner = SubAgentRunner( @@ -78,7 +78,7 @@ def test_runner_happy_path(sandbox: Sandbox, tmp_path: Path) -> None: ) mock_agent = _make_mock_agent() with patch("coder_eval.evaluation.sub_agent.ClaudeCodeAgent", return_value=mock_agent): - turn = runner.run("grade this", max_turns=10, turn_timeout=30.0) + turn = await runner.run_async("grade this", max_turns=10, turn_timeout=30.0) assert turn.agent_output == '{"score": 1.0, "rationale": "ok"}' mock_agent.start.assert_awaited_once() @@ -86,7 +86,7 @@ def test_runner_happy_path(sandbox: Sandbox, tmp_path: Path) -> None: mock_agent.stop.assert_awaited() -def test_runner_mounts_reference_dir_at_underscore_reference(sandbox: Sandbox, tmp_path: Path) -> None: +async def test_runner_mounts_reference_dir_at_underscore_reference(sandbox: Sandbox, tmp_path: Path) -> None: """When reference_dir is provided, copy it into the judge's working dir at _reference/.""" (tmp_path / "Main.xaml").write_text("") @@ -123,7 +123,7 @@ async def capture_files(_msg: str, **_kw: object) -> TurnRecord: return _make_turn() mock_agent.communicate.side_effect = capture_files - runner.run("grade", max_turns=10, turn_timeout=30.0) + await runner.run_async("grade", max_turns=10, turn_timeout=30.0) assert captured["has_reference_dir"] == "True" assert captured["has_main"] == "True" @@ -131,7 +131,7 @@ async def capture_files(_msg: str, **_kw: object) -> TurnRecord: assert captured["has_subdir"] == "True" -def test_runner_handles_sandbox_side_reference_collision(sandbox: Sandbox, tmp_path: Path) -> None: +async def test_runner_handles_sandbox_side_reference_collision(sandbox: Sandbox, tmp_path: Path) -> None: """Regression for bug_002: when the sandbox already contains _reference/ (template-staged or agent-planted), the second copytree must not raise FileExistsError. Default ignore_patterns includes _reference, and the @@ -174,14 +174,14 @@ async def capture_state(_msg: str, **_kw: object) -> TurnRecord: mock_agent.communicate.side_effect = capture_state # Must not raise — this is the regression assertion. - runner.run("grade", max_turns=10, turn_timeout=30.0) + await runner.run_async("grade", max_turns=10, turn_timeout=30.0) # _reference/ contains the REAL reference content, NOT the agent-planted file. assert captured["ref_main_content"] == "" assert captured["agent_planted_present"] == "False" -def test_runner_skips_reference_when_not_provided(sandbox: Sandbox, tmp_path: Path) -> None: +async def test_runner_skips_reference_when_not_provided(sandbox: Sandbox, tmp_path: Path) -> None: """No reference_dir → no _reference/ in the judge's working directory.""" (tmp_path / "Main.xaml").write_text("") @@ -207,12 +207,12 @@ async def capture_no_ref(_msg: str, **_kw: object) -> TurnRecord: return _make_turn() mock_agent.communicate.side_effect = capture_no_ref - runner.run("grade", max_turns=10, turn_timeout=30.0) + await runner.run_async("grade", max_turns=10, turn_timeout=30.0) assert captured["has_reference_dir"] == "False" -def test_runner_starts_in_temp_copy_not_original(sandbox: Sandbox, tmp_path: Path) -> None: +async def test_runner_starts_in_temp_copy_not_original(sandbox: Sandbox, tmp_path: Path) -> None: (tmp_path / "Main.xaml").write_text("") runner = SubAgentRunner( @@ -223,14 +223,14 @@ def test_runner_starts_in_temp_copy_not_original(sandbox: Sandbox, tmp_path: Pat ) mock_agent = _make_mock_agent() with patch("coder_eval.evaluation.sub_agent.ClaudeCodeAgent", return_value=mock_agent): - runner.run("grade", max_turns=10, turn_timeout=30.0) + await runner.run_async("grade", max_turns=10, turn_timeout=30.0) start_arg = mock_agent.start.call_args.args[0] assert start_arg != str(sandbox.sandbox_dir) assert "sub_agent_" in start_arg -def test_runner_cleans_up_on_success(sandbox: Sandbox) -> None: +async def test_runner_cleans_up_on_success(sandbox: Sandbox) -> None: runner = SubAgentRunner( sandbox=sandbox, agent_config=_make_agent_config(), @@ -245,13 +245,106 @@ async def capture_start(path: str, **_kwargs: object) -> None: mock_agent.start.side_effect = capture_start with patch("coder_eval.evaluation.sub_agent.ClaudeCodeAgent", return_value=mock_agent): - runner.run("grade", max_turns=10, turn_timeout=30.0) + await runner.run_async("grade", max_turns=10, turn_timeout=30.0) assert captured["path"] assert not Path(captured["path"]).exists() -def test_runner_cleans_up_on_communicate_exception(sandbox: Sandbox) -> None: +async def test_runner_cleans_up_when_cancelled_mid_communicate(sandbox: Sandbox) -> None: + """run_async is awaited directly on the orchestrator's own loop (not under its + own asyncio.run on a worker thread), so a cancellation — e.g. the task_timeout + watchdog cancelling the orchestrator task — can land while communicate() is in + flight. The finally-block cleanup must still remove judge_dir (it's a plain + synchronous shutil.rmtree, not an awaited to_thread call, precisely so it can't + itself be interrupted by the same cancellation).""" + import asyncio + + runner = SubAgentRunner( + sandbox=sandbox, + agent_config=_make_agent_config(), + ignore_patterns=[], + route=DirectRoute(), + ) + mock_agent = _make_mock_agent() + captured: dict[str, str] = {} + started = asyncio.Event() + + async def capture_start(path: str, **_kwargs: object) -> None: + captured["path"] = path + + async def hang_forever(*_args: object, **_kwargs: object) -> TurnRecord: + started.set() + await asyncio.sleep(3600) + raise AssertionError("should have been cancelled before waking up") + + mock_agent.start.side_effect = capture_start + mock_agent.communicate.side_effect = hang_forever + + with patch("coder_eval.evaluation.sub_agent.ClaudeCodeAgent", return_value=mock_agent): + task = asyncio.ensure_future(runner.run_async("grade", max_turns=10, turn_timeout=30.0)) + await started.wait() + task.cancel() + with pytest.raises(asyncio.CancelledError): + _ = await task # the await's effect is propagating the cancellation raised above + + assert captured["path"] + assert not Path(captured["path"]).exists() + + +async def test_runner_cleans_up_when_cancelled_mid_copytree(sandbox: Sandbox) -> None: + """Cancellation reaching mid-``copytree`` (not just mid-``communicate``) + must not leak ``judge_dir``. Reproduces the exact race the shield + + finally-awaits-pending fix addresses: ``asyncio.to_thread`` is not itself + cancellable, so without shielding + waiting for it in ``finally``, an + orphan worker thread can keep copying files into ``judge_dir`` — and even + finish AFTER ``finally``'s ``rmtree`` already ran, leaving a full, + permanent copy behind.""" + import asyncio + import shutil + import tempfile + import threading + import time + + copy_started = threading.Event() + real_copytree = shutil.copytree + + def slow_copytree(*args: object, **kwargs: object) -> object: + copy_started.set() + time.sleep(0.3) + return real_copytree(*args, **kwargs) # type: ignore[arg-type] + + captured_dir: dict[str, Path] = {} + real_mkdtemp = tempfile.mkdtemp + + def capture_mkdtemp(*args: object, **kwargs: object) -> str: + d = real_mkdtemp(*args, **kwargs) # type: ignore[arg-type] + captured_dir["path"] = Path(d) + return d + + runner = SubAgentRunner( + sandbox=sandbox, + agent_config=_make_agent_config(), + ignore_patterns=[], + route=DirectRoute(), + ) + + with ( + patch("coder_eval.evaluation.sub_agent.shutil.copytree", side_effect=slow_copytree), + patch("coder_eval.evaluation.sub_agent.tempfile.mkdtemp", side_effect=capture_mkdtemp), + ): + task = asyncio.ensure_future(runner.run_async("grade", max_turns=10, turn_timeout=30.0)) + while not copy_started.is_set(): + await asyncio.sleep(0.01) + task.cancel() + with pytest.raises(asyncio.CancelledError): + _ = await task + + assert captured_dir.get("path") is not None + assert not captured_dir["path"].exists(), "orphan copytree thread recreated judge_dir after cleanup already ran" + + +async def test_runner_cleans_up_on_communicate_exception(sandbox: Sandbox) -> None: runner = SubAgentRunner( sandbox=sandbox, agent_config=_make_agent_config(), @@ -271,14 +364,14 @@ async def capture_start(path: str, **_kwargs: object) -> None: patch("coder_eval.evaluation.sub_agent.ClaudeCodeAgent", return_value=mock_agent), pytest.raises(RuntimeError, match="boom"), ): - runner.run("grade", max_turns=10, turn_timeout=30.0) + await runner.run_async("grade", max_turns=10, turn_timeout=30.0) assert not Path(captured["path"]).exists() mock_agent.kill.assert_awaited() mock_agent.stop.assert_awaited() -def test_runner_cleans_up_on_start_failure(sandbox: Sandbox) -> None: +async def test_runner_cleans_up_on_start_failure(sandbox: Sandbox) -> None: runner = SubAgentRunner( sandbox=sandbox, agent_config=_make_agent_config(), @@ -292,12 +385,12 @@ def test_runner_cleans_up_on_start_failure(sandbox: Sandbox) -> None: patch("coder_eval.evaluation.sub_agent.ClaudeCodeAgent", return_value=mock_agent), pytest.raises(RuntimeError, match="claude binary not found"), ): - runner.run("grade", max_turns=10, turn_timeout=30.0) + await runner.run_async("grade", max_turns=10, turn_timeout=30.0) mock_agent.kill.assert_awaited() -def test_runner_propagates_turn_timeout(sandbox: Sandbox) -> None: +async def test_runner_propagates_turn_timeout(sandbox: Sandbox) -> None: runner = SubAgentRunner( sandbox=sandbox, agent_config=_make_agent_config(), @@ -317,7 +410,7 @@ async def capture_start(path: str, **_kwargs: object) -> None: patch("coder_eval.evaluation.sub_agent.ClaudeCodeAgent", return_value=mock_agent), pytest.raises(TurnTimeoutError), ): - runner.run("grade", max_turns=10, turn_timeout=30.0) + await runner.run_async("grade", max_turns=10, turn_timeout=30.0) assert not Path(captured["path"]).exists() @@ -378,7 +471,7 @@ def test_ignore_callable_still_honors_patterns(tmp_path: Path) -> None: @_SKIP_NO_SYMLINK -def test_runner_copytree_drops_top_level_symlinks(sandbox: Sandbox, tmp_path: Path) -> None: +async def test_runner_copytree_drops_top_level_symlinks(sandbox: Sandbox, tmp_path: Path) -> None: """End-to-end: a malicious symlink in the sandbox does not land in the judge workspace.""" import os @@ -399,14 +492,14 @@ async def capture_start(path: str, **_kwargs: object) -> None: mock_agent.start.side_effect = capture_start with patch("coder_eval.evaluation.sub_agent.ClaudeCodeAgent", return_value=mock_agent): - runner.run("grade", max_turns=10, turn_timeout=30.0) + await runner.run_async("grade", max_turns=10, turn_timeout=30.0) assert "real.txt" in captured["entries"] assert "leak" not in captured["entries"] @_SKIP_NO_SYMLINK -def test_runner_copytree_drops_nested_symlinks(sandbox: Sandbox, tmp_path: Path) -> None: +async def test_runner_copytree_drops_nested_symlinks(sandbox: Sandbox, tmp_path: Path) -> None: """Nested symlinks are stripped too, not just top-level ones.""" import os @@ -429,13 +522,13 @@ async def capture_start(path: str, **_kwargs: object) -> None: mock_agent.start.side_effect = capture_start with patch("coder_eval.evaluation.sub_agent.ClaudeCodeAgent", return_value=mock_agent): - runner.run("grade", max_turns=10, turn_timeout=30.0) + await runner.run_async("grade", max_turns=10, turn_timeout=30.0) assert "keep.txt" in captured["sub_entries"] assert "nested_leak" not in captured["sub_entries"] -def test_runner_copytree_honors_patterns(sandbox: Sandbox, tmp_path: Path) -> None: +async def test_runner_copytree_honors_patterns(sandbox: Sandbox, tmp_path: Path) -> None: (tmp_path / ".git").mkdir() (tmp_path / ".git" / "HEAD").write_text("ref: refs/heads/main\n") (tmp_path / "Main.xaml").write_text("") @@ -455,13 +548,13 @@ async def capture_start(path: str, **_kwargs: object) -> None: mock_agent.start.side_effect = capture_start with patch("coder_eval.evaluation.sub_agent.ClaudeCodeAgent", return_value=mock_agent): - runner.run("grade", max_turns=10, turn_timeout=30.0) + await runner.run_async("grade", max_turns=10, turn_timeout=30.0) assert captured["has_git"] == "False" assert captured["has_main"] == "True" -def test_runner_excludes_nested_claude_and_mcp(sandbox: Sandbox, tmp_path: Path) -> None: +async def test_runner_excludes_nested_claude_and_mcp(sandbox: Sandbox, tmp_path: Path) -> None: """Nested .claude/ and .mcp.json planted by a compromised agent must not reach the copy.""" import os @@ -487,7 +580,7 @@ async def capture_start(path: str, **_kwargs: object) -> None: mock_agent.start.side_effect = capture_start with patch("coder_eval.evaluation.sub_agent.ClaudeCodeAgent", return_value=mock_agent): - runner.run("grade", max_turns=10, turn_timeout=30.0) + await runner.run_async("grade", max_turns=10, turn_timeout=30.0) assert ".claude" not in captured["sub_entries"] assert ".mcp.json" not in captured["sub_entries"] @@ -497,7 +590,9 @@ async def capture_start(path: str, **_kwargs: object) -> None: # --- reference_ignore_patterns split (Phase 4 / finding #9) --- -def test_runner_reference_dir_with_nested_underscore_reference_preserved(sandbox: Sandbox, tmp_path: Path) -> None: +async def test_runner_reference_dir_with_nested_underscore_reference_preserved( + sandbox: Sandbox, tmp_path: Path +) -> None: """A nested ``_reference/`` inside the user's reference dir is NOT stripped. Regression for finding #9: the pre-fix code reused the sandbox-side @@ -539,13 +634,13 @@ async def capture_state(_msg: str, **_kw: object) -> TurnRecord: return _make_turn() mock_agent.communicate.side_effect = capture_state - runner.run("grade", max_turns=10, turn_timeout=30.0) + await runner.run_async("grade", max_turns=10, turn_timeout=30.0) assert captured["inner_present"] == "True" assert captured["inner_content"] == "CUSTOMER-CONTENT" -def test_runner_reference_ignore_patterns_explicit(sandbox: Sandbox, tmp_path: Path) -> None: +async def test_runner_reference_ignore_patterns_explicit(sandbox: Sandbox, tmp_path: Path) -> None: """Explicit reference_ignore_patterns are honored on the reference-side copy.""" (tmp_path / "Main.xaml").write_text("") @@ -578,7 +673,7 @@ async def capture_state(_msg: str, **_kw: object) -> TurnRecord: return _make_turn() mock_agent.communicate.side_effect = capture_state - runner.run("grade", max_turns=10, turn_timeout=30.0) + await runner.run_async("grade", max_turns=10, turn_timeout=30.0) assert captured["keep_present"] == "True" assert captured["log_present"] == "False" diff --git a/tests/test_timeout_orchestrator.py b/tests/test_timeout_orchestrator.py index cac28b1b..395ffdba 100644 --- a/tests/test_timeout_orchestrator.py +++ b/tests/test_timeout_orchestrator.py @@ -189,7 +189,7 @@ async def test_no_timeout_when_none(tmp_path) -> None: mock_agent.communicate = AsyncMock(return_value=_make_turn_record()) orchestrator.agent = mock_agent - orchestrator.success_checker.check_all = MagicMock( # type: ignore[union-attr] + orchestrator.success_checker.check_all_async = AsyncMock( # type: ignore[union-attr] return_value=[CriterionResult(criterion_type="file_exists", description="test", score=1.0)] ) @@ -325,7 +325,7 @@ async def flaky_communicate(_prompt, **kwargs): orchestrator.sandbox = mock_sandbox mock_checker = MagicMock() - mock_checker.check_all = MagicMock( + mock_checker.check_all_async = AsyncMock( return_value=[CriterionResult(criterion_type="file_exists", description="x", score=1.0)] ) orchestrator.success_checker = mock_checker diff --git a/uv.lock b/uv.lock index 5282dca9..67784f37 100644 --- a/uv.lock +++ b/uv.lock @@ -449,7 +449,7 @@ wheels = [ [[package]] name = "coder-eval" -version = "0.8.10" +version = "0.9.0" source = { editable = "." } dependencies = [ { name = "anthropic" },