Skip to content

Commit 61111cf

Browse files
akshayliveclaude
andcommitted
fix(criteria): fix declaration-order regression, judge-sandbox leak, and silent misuse scoring
Addresses the second review pass on PR #60 — three real scoring/correctness blockers plus the contract/API-surface follow-ups: - check_all_async now splits criteria into maximal CONTIGUOUS runs of the same kind (sync vs native-async) and executes the runs strictly in declaration order, instead of always running the whole sync batch before any async criterion. The previous scheme inverted declaration order for e.g. [llm_judge, run_command] — the judge, though declared first, always ran after run_command, silently grading post-mutation sandbox state instead of the pre-mutation state check_all's serial order would give it. Adjacent judges in the same run still gather concurrently (the actual GH #55 fix); results are built from a dict proven total over all runs, replacing the previous assert-guarded cast with a real KeyError on any future gap. Every captured sibling exception in a run is now logged (not silently dropped) before the first is re-raised. - SubAgentRunner.run_async no longer leaks a full sandbox copy when cancelled mid-copytree: judge_dir is bound with a plain synchronous tempfile.mkdtemp (one syscall, not cancellable, not worth to_thread's cancellation-window cost), and the copytree/rmtree to_thread calls are wrapped in asyncio.shield + tracked in a pending list that `finally` awaits BEFORE its own rmtree — so an orphaned worker thread can no longer recreate files after cleanup already ran. Applied the same await-in-finally fix to simulation/user_simulator.py's scratch-dir teardown (same class of leak, previously only fixed in sub_agent.py). - BaseCriterion._check_impl's derived asyncio.run bridge now detects a running event loop and raises a new CheckerMisuseError (which handle_criterion_errors(_async) escalate, like JudgeInfrastructureError) instead of letting asyncio.run's RuntimeError get silently swallowed into a scored-0.0 CriterionResult — a library/embedder calling the public sync check()/check_all() on an async-only checker from async host code now gets a loud, named error instead of a wrong score. - __init_subclass__ now enforces "exactly one", not just "at least one": a checker overriding BOTH _check_impl and _check_impl_async is also rejected (two live implementations that could drift into different scores depending on which entry point ran). Added an `abstract=True` class-kwarg escape hatch for intentional abstract intermediate bases, and a __new__ guard so BaseCriterion itself can't be instantiated directly (lost when @AbstractMethod was dropped). - Promoted the native-async capability check to a public, typed BaseCriterion.is_native_async() classmethod; SuccessChecker._is_native_async and test_registry.py now call it instead of comparing `_check_impl_async` identity across package boundaries. Decorated check()/check_async() with @typing.final so the "these are FINAL" docstring contract is pyright-enforced. Extracted the duplicated 18-line error-capture tail out of handle_criterion_errors/_async into one shared _failed_result() helper. - Tests: reversed-declaration-order regression test (real llm_judge + run_command checkers) proving the contiguous-run fix; a cancel-during-copy test on SubAgentRunner reproducing and closing the leak; a loud-CheckerMisuseError-from-a-running-loop test; both-overridden / abstract=True / direct-instantiation tests for the tightened __init_subclass__ contract; a thread-affinity assertion (not just score) for the derived async bridge; replaced two inert TestNativeAsyncDetection tests (injected into _checker_instances, which classification never reads) with registry-based ones that actually exercise the class-based dispatch rule; a checker-__init__-failure test closing the last uncovered branch in _check_single_async; one check_all_async happy-path test each for llm_judge/agent_judge (production's actual entry point — every other test in both files still drives the derived sync bridge). - Fixed stale ``check_all`` (vs check_all_async) mentions in CLAUDE.md, early_stop.py, reports.py, and orchestrator.py; documented the must-not-block-the-loop obligation and the CheckerMisuseError caveat in docs/EXTENDING.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent a3c46a1 commit 61111cf

15 files changed

Lines changed: 681 additions & 140 deletions

CLAUDE.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,7 @@ action.yml # Published composite GitHub Action (coder-ev
141141
- **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.
142142
- **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.
143143
- **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.
144-
- **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.
144+
- **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.
145145

146146
## Success Criteria (14 types)
147147

docs/EXTENDING.md

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -174,19 +174,33 @@ class MyChecker(BaseCriterion[MyCriterion]):
174174

175175
Notes:
176176

177-
- **Do not override `check()` / `check_async()`** — both are final and wrap
177+
- **Do not override `check()` / `check_async()`** — both are `@final` and wrap
178178
`_check_impl` / `_check_impl_async` with error handling (an exception becomes
179179
a score-0.0 result with the error captured; a `JudgeInfrastructureError`
180180
escalates instead).
181181
- Implement exactly ONE of `_check_impl` (plain sync — the common case, shown
182182
above) or `_check_impl_async` (genuine async I/O — an async HTTP client or
183183
subprocess bridge; see `llm_judge`/`agent_judge`). Whichever you implement,
184184
`BaseCriterion` derives the other for free (`asyncio.to_thread` / `asyncio.run`),
185-
so there is no need to hand-maintain both. Overriding neither raises
186-
`TypeError` immediately at class-definition time. `SuccessChecker.check_all_async`
185+
so there is no need to hand-maintain both. Overriding neither, or overriding
186+
BOTH, raises `TypeError` immediately at class-definition time (a shared
187+
abstract base for a family of checkers that intentionally implements neither
188+
can opt out with the `abstract=True` class keyword — every one of ITS
189+
subclasses is still checked normally). `SuccessChecker.check_all_async`
187190
— the orchestrator's entry point — awaits every `_check_impl_async`-native
188191
checker directly on the event loop (concurrently with its siblings), and runs
189192
everything else through one `asyncio.to_thread` slot.
193+
- If your checker overrides `_check_impl_async`, it MUST NOT do blocking work
194+
(file I/O, subprocess calls) directly on the event loop — that would stall
195+
every sibling judge criterion gathered alongside it. Offload blocking calls
196+
with `await asyncio.to_thread(...)` (see `llm_judge`/`agent_judge`, which do
197+
this for their sandbox/reference file reads).
198+
- The derived sync bridge (`_check_impl`'s default `asyncio.run(...)` call) can
199+
only run when no event loop is already running — calling the public sync
200+
`check()`/`check_all()` on an async-only checker from inside a running loop
201+
raises `CheckerMisuseError` (escalates, like `JudgeInfrastructureError`)
202+
rather than returning a wrong score. Always reach for `check_async()` /
203+
`check_all_async()` from async code.
190204
- Return `score` in `[0.0, 1.0]` — binary criteria use `0.0`/`1.0`; fractional ones
191205
anything in between.
192206
- For **suite-level metrics** on dataset-backed tasks, override

src/coder_eval/criteria/base.py

Lines changed: 110 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,9 @@
99
from collections.abc import Awaitable, Callable
1010
from dataclasses import dataclass
1111
from functools import wraps
12-
from typing import TYPE_CHECKING, Any, ClassVar, Concatenate, Literal, ParamSpec
12+
from typing import TYPE_CHECKING, Any, ClassVar, Concatenate, Literal, ParamSpec, final
1313

14-
from coder_eval.errors import JudgeInfrastructureError
14+
from coder_eval.errors import CheckerMisuseError, JudgeInfrastructureError
1515
from coder_eval.models import BaseSuccessCriterion, CriterionAggregate, CriterionResult
1616

1717

@@ -52,6 +52,36 @@ class CheckContext:
5252
# uninitialized local; a plain `typing.ParamSpec` is unambiguous to both tools.
5353
P = ParamSpec("P")
5454

55+
# Exceptions that must escalate rather than be captured into a scored-0.0
56+
# CriterionResult — a judge-infra outage or a checker-contract misuse is not an
57+
# agent failure. Shared by both handle_criterion_errors(_async) wrappers below.
58+
_ESCALATING_EXCEPTIONS: tuple[type[Exception], ...] = (JudgeInfrastructureError, CheckerMisuseError)
59+
60+
61+
def _failed_result(owner: Any, criterion: BaseSuccessCriterion, exc: Exception, method: str) -> CriterionResult:
62+
"""Build the failed ``CriterionResult`` for a captured (non-escalating)
63+
checker exception, and log it. Shared by the sync/async
64+
``handle_criterion_errors(_async)`` wrapper tails so the two decorators
65+
differ only in ``def``/``async def`` and ``return``/``return await``.
66+
"""
67+
exc_info = f"{exc.__class__.__name__}: {exc}"
68+
tb = ""
69+
if os.getenv("CODER_EVAL_DEBUG") == "1":
70+
tb = "\n" + "".join(traceback.format_exc(limit=5))
71+
72+
criterion_type = criterion.type
73+
logger.error(
74+
f"Error in {owner.__class__.__name__}.{method}() for criterion type '{criterion_type}': {exc_info}",
75+
exc_info=True, # Adds full stack trace to logs
76+
)
77+
return CriterionResult(
78+
criterion_type=criterion_type,
79+
description=criterion.description,
80+
score=0.0,
81+
details=f"Error during check: {exc_info}{tb}",
82+
error=exc_info, # Include exception type and message
83+
)
84+
5585

5686
def handle_criterion_errors( # noqa: UP047
5787
func: Callable[Concatenate[Any, BaseSuccessCriterion, P], CriterionResult],
@@ -78,28 +108,13 @@ def wrapper(
78108
) -> CriterionResult:
79109
try:
80110
return func(self, criterion, *args, **kwargs)
81-
except JudgeInfrastructureError:
82-
# Judge infra failure is NOT an agent failure — do not score it 0.0.
83-
# Propagates to Orchestrator.run()'s broad except → FinalStatus.ERROR.
111+
except _ESCALATING_EXCEPTIONS:
112+
# Judge infra failure / checker-contract misuse is NOT an agent
113+
# failure — do not score it 0.0. Propagates to Orchestrator.run()'s
114+
# broad except → FinalStatus.ERROR.
84115
raise
85116
except Exception as e:
86-
exc_info = f"{e.__class__.__name__}: {e}"
87-
tb = ""
88-
if os.getenv("CODER_EVAL_DEBUG") == "1":
89-
tb = "\n" + "".join(traceback.format_exc(limit=5))
90-
91-
criterion_type = criterion.type
92-
logger.error(
93-
f"Error in {self.__class__.__name__}.check() for criterion type '{criterion_type}': {exc_info}",
94-
exc_info=True, # Adds full stack trace to logs
95-
)
96-
return CriterionResult(
97-
criterion_type=criterion_type,
98-
description=criterion.description,
99-
score=0.0,
100-
details=f"Error during check: {exc_info}{tb}",
101-
error=exc_info, # Include exception type and message
102-
)
117+
return _failed_result(self, criterion, e, "check")
103118

104119
return wrapper
105120

@@ -123,26 +138,10 @@ async def wrapper(
123138
) -> CriterionResult:
124139
try:
125140
return await func(self, criterion, *args, **kwargs)
126-
except JudgeInfrastructureError:
141+
except _ESCALATING_EXCEPTIONS:
127142
raise
128143
except Exception as e:
129-
exc_info = f"{e.__class__.__name__}: {e}"
130-
tb = ""
131-
if os.getenv("CODER_EVAL_DEBUG") == "1":
132-
tb = "\n" + "".join(traceback.format_exc(limit=5))
133-
134-
criterion_type = criterion.type
135-
logger.error(
136-
f"Error in {self.__class__.__name__}.check_async() for criterion type '{criterion_type}': {exc_info}",
137-
exc_info=True,
138-
)
139-
return CriterionResult(
140-
criterion_type=criterion_type,
141-
description=criterion.description,
142-
score=0.0,
143-
details=f"Error during check: {exc_info}{tb}",
144-
error=exc_info,
145-
)
144+
return _failed_result(self, criterion, e, "check_async")
146145

147146
return wrapper
148147

@@ -169,10 +168,12 @@ class derives the other automatically:
169168
base's default ``_check_impl`` derives a sync call by running the async
170169
one to completion on a fresh event loop (``asyncio.run``).
171170
172-
``__init_subclass__`` enforces that a checker overrides at least one of
171+
``__init_subclass__`` enforces that a checker overrides EXACTLY ONE of
173172
the two, at class-definition time — overriding neither would recurse
174173
forever between the defaults (``asyncio.run`` <-> ``asyncio.to_thread``)
175-
the first time either is called.
174+
the first time either is called, and overriding both would let the two
175+
implementations silently drift into different scores depending on which
176+
entry point (``check`` vs ``check_async``) ran.
176177
177178
``check()`` / ``check_async()`` are FINAL — they apply centralized error
178179
handling and must not be overridden; implement ``_check_impl`` /
@@ -206,19 +207,67 @@ def _check_impl(
206207
# live_verdict; CE025 enforces that the two stay consistent.
207208
live_stop_polarities: ClassVar[frozenset[str]] = frozenset()
208209

209-
def __init_subclass__(cls, **kwargs: Any) -> None:
210+
def __new__(cls, *args: Any, **kwargs: Any) -> "BaseCriterion[C]":
211+
"""Block direct instantiation of ``BaseCriterion`` itself.
212+
213+
``__init_subclass__`` below only runs for SUBCLASSES, so with no
214+
``@abstractmethod`` left on this class (both ``_check_impl*`` methods
215+
have concrete default bodies, by design — that's what lets each derive
216+
the other), plain ``ABCMeta`` no longer blocks ``BaseCriterion()``
217+
directly. This restores that guarantee without reintroducing an
218+
abstract method that would break the "override at least one" contract.
219+
"""
220+
if cls is BaseCriterion:
221+
raise TypeError("BaseCriterion is abstract and cannot be instantiated directly")
222+
return super().__new__(cls)
223+
224+
def __init_subclass__(cls, *, abstract: bool = False, **kwargs: Any) -> None:
210225
"""Enforce the ``_check_impl`` / ``_check_impl_async`` override contract
211226
at class-definition time (module import), regardless of which entry
212227
point later registers the class — closing the gap where a subclass
213228
registered via ``CriterionRegistry.register`` directly (bypassing the
214229
``register_criterion`` decorator) escaped the check, and turning the
215230
mutual-recursion failure mode (``asyncio.run`` <-> ``asyncio.to_thread``
216231
exhausting OS threads) into an immediate, clearly-named ``TypeError``.
232+
233+
Enforces "exactly one", not just "at least one": overriding BOTH is
234+
also rejected — a checker with two live implementations (sync-path
235+
`_check_impl` and async-path `_check_impl_async`) is free to have them
236+
drift into different scores for identical agent output depending on
237+
which entry point (``check`` vs ``check_async``) happened to run it,
238+
which is exactly the class of bug this derivation design exists to
239+
eliminate.
240+
241+
Pass ``abstract=True`` on a class that intentionally implements
242+
neither (e.g. a shared abstract base for a family of related
243+
checkers) to opt out of the check for that one class; every one of
244+
ITS subclasses is still checked normally.
217245
"""
218246
super().__init_subclass__(**kwargs)
219-
if cls._check_impl is BaseCriterion._check_impl and cls._check_impl_async is BaseCriterion._check_impl_async:
247+
if abstract:
248+
return
249+
overrides_sync = cls._check_impl is not BaseCriterion._check_impl
250+
overrides_async = cls._check_impl_async is not BaseCriterion._check_impl_async
251+
if not overrides_sync and not overrides_async:
220252
raise TypeError(f"{cls.__name__} must override _check_impl or _check_impl_async")
253+
if overrides_sync and overrides_async:
254+
msg = f"{cls.__name__} must override exactly one of _check_impl / _check_impl_async, not both"
255+
raise TypeError(msg)
256+
257+
@classmethod
258+
def is_native_async(cls) -> bool:
259+
"""Whether this checker class makes genuine async I/O — i.e. overrides
260+
``_check_impl_async`` itself rather than inheriting the base's
261+
to-thread-wrapped-sync default.
262+
263+
Public + typed so dispatch code (``SuccessChecker._is_native_async``)
264+
and anything else that needs to classify a checker doesn't have to
265+
reach into ``_check_impl_async`` (a "protected" name) on another
266+
class from a different package.
267+
"""
268+
return cls._check_impl_async is not BaseCriterion._check_impl_async
221269

270+
@final
222271
@handle_criterion_errors
223272
def check(
224273
self,
@@ -283,8 +332,23 @@ def _check_impl(
283332
CriterionResult with score (0.0-1.0), details, and error info
284333
285334
Raises:
286-
Any exception - will be caught by @handle_criterion_errors decorator
335+
CheckerMisuseError: this bridge is called from inside a running
336+
event loop (``asyncio.run`` cannot start a nested loop) — this
337+
is a caller mistake (the async-primary surface should have
338+
been awaited instead), not an agent failure, so it escalates
339+
rather than silently scoring 0.0.
340+
Any other exception - will be caught by @handle_criterion_errors
287341
"""
342+
try:
343+
asyncio.get_running_loop()
344+
except RuntimeError:
345+
pass
346+
else:
347+
msg = (
348+
f"{type(self).__name__} implements only _check_impl_async; "
349+
f"call check_async()/check_all_async() from an event loop, not check()/check_all()"
350+
)
351+
raise CheckerMisuseError(msg)
288352
return asyncio.run(
289353
self._check_impl_async(
290354
criterion,
@@ -295,6 +359,7 @@ def _check_impl(
295359
)
296360
)
297361

362+
@final
298363
@handle_criterion_errors_async
299364
async def check_async(
300365
self,

0 commit comments

Comments
 (0)