Skip to content

feat(criteria): make async the primary criterion-checking surface - #60

Open
akshaylive wants to merge 1 commit into
mainfrom
akshaya/async_criteria_v2
Open

feat(criteria): make async the primary criterion-checking surface#60
akshaylive wants to merge 1 commit into
mainfrom
akshaya/async_criteria_v2

Conversation

@akshaylive

Copy link
Copy Markdown
Collaborator

Summary

Redesign of the fix for #55, replacing PR #58. That PR made judge criteria (llm_judge/agent_judge) concurrent by bolting an async path onto a sync-first BaseCriterion — each judge checker ended up with two near-duplicate implementations (a sync client + an async client) plus a supports_native_async marker flag to tell SuccessChecker which path to dispatch through. This PR inverts that relationship instead of layering on top of it:

  • BaseCriterion._check_impl_async is now the primary implementation surface. A checker overrides exactly one of _check_impl (sync) or _check_impl_async (async) — whichever is its natural form — and the base class derives the other automatically:
    • CPU/file-bound checkers (file_exists, command_executed, ...) keep overriding _check_impl only; the default _check_impl_async offloads it via asyncio.to_thread.
    • llm_judge/agent_judge now override only _check_impl_async, using AsyncAnthropic, httpx.AsyncClient, and a new SubAgentRunner.run_async — no sync client duplicated. The default _check_impl derives a sync call via asyncio.run(...) for any caller that still wants one (tests, etc.).
  • register_criterion enforces that a checker overrides at least one of the two (overriding neither would recurse forever between the defaults).
  • SuccessChecker.check_all_async no longer needs an explicit "is this checker async-native" flag — it detects it by introspecting whether _check_impl_async is overridden, then gathers those checkers concurrently with each other while the remaining sync checkers still share one asyncio.to_thread slot (unchanged behavior for the 12 non-judge criteria).
  • SubAgentRunner.run() (sync) is removed — nothing calls it anymore now that agent_judge is async-only; run_async() is the sole entrypoint.

Net effect on behavior is identical to #58 (judge criteria run concurrently instead of serializing, and don't pin a thread-pool thread for the network wait) — this PR just gets there with one implementation per criterion instead of two.

Test plan

  • make check / make typecheck / make lint clean
  • make verify — 3582 passed, 2 skipped, 90.99% coverage
  • tests/test_check_all_async.py — judge criteria overlap instead of serializing, sync/async batches overlap, ordering preserved, error handling parity, _is_native_async detection, register_criterion guard, and direct sync↔async derivation on BaseCriterion
  • tests/test_judge_anthropic.py / tests/test_judge_bedrock.py rewritten against the async invokers
  • tests/test_sub_agent_runner.py converted to run_async

Closes #55 (supersedes #58 — closing that PR in favor of this one)

🤖 Generated with Claude Code

BaseCriterion previously required every checker to implement the sync
_check_impl, with async only a to-thread-wrapped afterthought — so the
prior fix for judge-criteria serialization (llm_judge/agent_judge) had to
hand-maintain two near-duplicate implementations (a sync client + an async
client) side by side.

Flip the relationship: _check_impl_async is now the primary surface, with
each of _check_impl / _check_impl_async deriving a default for the other
(to_thread for sync-only checkers, asyncio.run for async-only ones).
register_criterion enforces overriding at least one. llm_judge and
agent_judge now implement ONLY _check_impl_async (AsyncAnthropic,
httpx.AsyncClient, SubAgentRunner.run_async) — no sync client duplicated.
SubAgentRunner.run() (sync) is removed since nothing calls it anymore;
run_async() is the sole entrypoint.

SuccessChecker.check_all_async detects "native async" checkers by
introspecting whether _check_impl_async is overridden (no more explicit
supports_native_async flag) and gathers them concurrently with each other
while the remaining sync criteria still share one to_thread slot — fixing
GH #55 (judge criteria serializing / pinning threads inside check_all).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown

Claude finished @akshaylive's task in 1m 26s —— View job


Code Review for PR #60: feat(criteria): make async the primary criterion-checking surface

Working on review...

Review Checklist

  • Read .github/code_review.md for review standards
  • Read CLAUDE.md for project conventions
  • Get full diff with git diff origin/main...HEAD
  • Review each changed file in full context
  • Check correctness and design
  • Verify backward compatibility
  • Check error handling in async paths
  • Perform cross-file consistency checks
  • Complete "What's Missing" analysis
  • Post final review

@uipreliga uipreliga left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: coder_eval — pr:60 (20 files) axis:1,2,3,4,5,6,7,8

Scope: pr:60 (20 files) axis:1,2,3,4,5,6,7,8 · branch akshaya/async_criteria_v2 (PR #60 head; local review branch docs/dedupe-brand-in-page-title) · 7dcb8b6 · 2026-07-28T05:32Z · workflow variant

Change class: complex — reshapes the criterion extension-point contract (removes the _check_impl abstractmethod in favor of a two-surface sync/async pair that derive from each other) and adds a concurrent check_all_async gather path in the orchestrator's evaluation loop, so correctness requires reasoning about event loops, thread offload, ordering, and error-handling parity.

The async-criteria split lands on a genuinely strong base — clean architecture, zero security findings, and an 8.9/10 overall — but the concurrent dispatch at src/coder_eval/evaluation/checker.py:220 breaks two harness guarantees that main provided for free (declaration-order sandbox observation, and no criterion work outliving the check phase), so identical agent output can now score differently and orphaned credentialed sub-agents can keep spending against a sandbox being torn down; fix the gather's ordering and cancellation semantics plus the now-unenforced _check_impl/_check_impl_async contract, and this is ready to ship.

Summary

Axis Score 🔴 🟠 🟡 🔵 Top Issue
1. Code Quality & Style 9.8 / 10 0 0 0 2 Partition bookkeeping in check_all_async: dead guard, O(n^2) membership, # type: ignore prefill
2. Type Safety 7.9 / 10 0 2 0 1 The new 'override one of _check_impl / _check_impl_async' contract has no static or complete runtime enforcement, and native-async capability is inferred from a fragile private-method identity probe duplicated at checker.py:236 and base.py:439
3. Test Health 8.4 / 10 0 1 1 1 test_registry's context-kwarg conformance guard is provably vacuous for llm_judge/agent_judge, and no test pins native-async dispatch for the registered judge checkers
4. Security 10 / 10 0 0 0 0
5. Architecture & Design 10 / 10 0 0 0 0
6. Error Handling & Resilience 8.8 / 10 0 1 0 2 check_all_async's asyncio.gather orphans sibling criterion work on JudgeInfrastructureError — abandoned sync batch thread and credentialed agent_judge sub-agents keep running/spending against a sandbox being torn down, with no test covering it
7. API Surface & Maintainability 9 / 10 0 0 2 0 Criterion extension-point contract changed without updating its authoring docs (EXTENDING.md / CLAUDE.md still describe the pre-PR _check_impl surface)
8. Evaluation Harness Quality 6.9 / 10 1 0 0 1 Concurrent dispatch overlaps sandbox-reading judge criteria with the sandbox-mutating sync criterion batch, so identical agent output can score differently (composition regression vs. main's serialized check_all)

Overall Score: 8.9 / 10 · Weakest Axis: Evaluation Harness Quality at 6.9 / 10
Totals: 🔴 1 · 🟠 4 · 🟡 3 · 🔵 7 across 8 axes.

Blockers

  1. [Axis 2] The new 'override one of _check_impl / _check_impl_async' contract has no static or complete runtime enforcement, and native-async capability is inferred from a fragile private-method identity probe duplicated at checker.py:236 and base.py:439 (src/coder_eval/criteria/base.py:245) — The PR changes from abc import ABC, abstractmethod to from abc import ABC (base.py:8) and drops the decorator that sat directly above def _check_impl( — on origin/main that was @abstractmethod at base.py:173, here base.py:245 is bare:
    def _check_impl(
        self,
        criterion: C,

BaseCriterion[C: BaseSuccessCriterion](ABC) (base.py:153) therefore has zero abstract methods, so the type system no longer expresses the contract at all. Verified under this repo's own pyright config (uv run pyright <file>, standard mode, in the PR worktree):

  • class Incomplete(BaseCriterion[FileExistsCriterion]): criterion_type = "incomplete" followed by Incomplete()0 errors, 0 warnings.
  • BaseCriterion[FileExistsCriterion]() (instantiating the "abstract" base directly) → 0 errors.
  • Control probe proving the check is enabled in this config: the same shape with an @abstractmethod yields error: Cannot instantiate abstract class "Sub" ... (reportAbstractUsage).

The runtime failure mode is also not a clear contract error. Running Incomplete()._check_impl(crit, None) under the worktree venv mutually recurses between the two base defaults — base.py:276 return asyncio.run( → base.py:326 return await asyncio.to_thread(asyncio.run … — and dies with RuntimeError: can't start new thread (thread/event-loop exhaustion), i.e. it burns OS threads before failing and never names the missing override.

The only remaining guard is runtime-only and on one code path — base.py:439:

    if cls._check_impl is BaseCriterion._check_impl and cls._check_impl_async is BaseCriterion._check_impl_async:
        raise TypeError(f"{cls.__name__} must override _check_impl or _check_impl_async")

That lives in register_criterion only. CriterionRegistry.register (src/coder_eval/criteria/init.py:25, and exported in that module's __all__) bypasses it entirely, as does any out-of-tree plugin class that is instantiated without going through the decorator; in-tree files are covered only because lint rule CE003 (tests/lint/rules/register_criterion_required.py) forces the decorator on criteria/*.py, which does not apply to plugins.

Fix: move the guard from register_criterion to BaseCriterion.__init_subclass__ so it fires at class-definition time on every subclass regardless of registration path, and restore a static signal — e.g. keep one of the two @abstractmethod and let only the other be derived, or declare the pair on a Protocol/__init_subclass__ check that pyright can see. At minimum the mutual recursion should be made impossible by construction rather than caught by a decorator that half the entry points skip.
2. [Axis 2] handle_criterion_errors_async erases check_async's signature — the new async checking surface types as (...) -> Awaitable[CriterionResult], so arity/argument-type errors at its call sites are not statically caught (latent; same shape as the pre-existing sync twin) (src/coder_eval/criteria/base.py:103) — base.py:102-104 declares the decorator with a fully erased parameter list, and its wrapper takes self: Any:

def handle_criterion_errors_async(
    func: Callable[..., Awaitable[CriterionResult]],
) -> Callable[..., Awaitable[CriterionResult]]:
    async def wrapper(
        self: Any,

(lines 103, 112-113). It is applied at base.py:286 to async def check_async( (base.py:287) — the method this PR promotes to the primary surface and the one SuccessChecker._check_single_async calls (evaluation/checker.py:362 result = await checker.check_async().

Verified with pyright under the repo's config: reveal_type(FileExistsChecker().check_async)information: Type of "checker.check_async" is "(...) -> Awaitable[CriterionResult]". Consequently this probe file type-checks with 0 errors, 0 warnings:

checker = FileExistsChecker()
crit = LLMJudgeCriterion(description="d", prompt="p")
await checker.check_async(crit, sandbox, 1, 2, 3, 4, 5)

Two distinct holes in one call: the wrong criterion model is passed to a BaseCriterion[FileExistsCriterion] checker (the C binding that base.py:182-183 advertises as the point of the type parameter is gone), and four surplus positional args are accepted — yet the wrapper at base.py:112-119 accepts exactly five parameters after self, so that call raises TypeError at runtime with nothing static catching it.

Fix: preserve the declared signature. Simplest is to delete the decorator and inline the same try/except JudgeInfrastructureError: raise / except Exception: body into check_async (keeps criterion: C, sandbox: "Sandbox" visible to callers, and removes the self: Any). Alternatively type it with a PEP 695 ParamSpec — def handle_criterion_errors_async[**P](func: Callable[P, Awaitable[CriterionResult]]) -> Callable[P, Awaitable[CriterionResult]] with async def wrapper(*args: P.args, **kwargs: P.kwargs) — or a Protocol with the exact call signature. The pre-existing sync twin at base.py:49 has the identical shape and deserves the same fix, but this PR is what makes the erased surface the primary one.
3. [Axis 3] test_registry's context-kwarg conformance guard is provably vacuous for llm_judge/agent_judge, and no test pins native-async dispatch for the registered judge checkers (src/coder_eval/criteria/llm_judge.py:60) — This PR moves llm_judge and agent_judge from def _check_impl to async def _check_impl_async (llm_judge.py:60, agent_judge.py:99) and makes check_all_async the orchestrator's entry point (orchestrator.py:1349, 1407, 1499). Two guards were silently lost:

(1) No test drives a real judge checker through the production path. Every test in tests/test_llm_judge_criterion.py and tests/test_agent_judge_criterion.py still calls the SYNC bridge — e.g. result = SuccessChecker(sandbox, init_registry=False, route=DirectRoute()).check(criterion) (test_llm_judge_criterion.py:105, test_agent_judge_criterion.py:948, 1102, 1153) — which routes _check_singlechecker.check → base _check_implasyncio.run(self._check_impl_async(...)) on a fresh, dedicated loop. Production instead awaits _check_impl_async directly on the orchestrator's live loop. grep -rn check_all_async tests/ returns only tests/test_check_all_async.py (which injects fake checkers into checker._checker_instances, e.g. line 103 checker._checker_instances["llm_judge"] = fake) plus AsyncMock'd orchestrator tests. So the only two criteria that spawn real network calls / SDK subprocesses have zero coverage on the loop they now actually run on, and nothing asserts checker._is_native_async("llm_judge") is True for the registered checker — a future refactor back to _check_impl would silently serialize both judges behind one to_thread slot with no failing test.

(2) tests/test_registry.py:66 is now a no-op for these two. params = inspect.signature(checker_cls._check_impl).parameters resolves to BaseCriterion._check_impl for llm_judge and agent_judge (verified: LLMJudgeChecker._check_impl is BaseCriterion._check_impl → True), so the assertion "missing 'context' kwarg" and the removed-kwarg assertions at lines 67-74 can never fail for them.

Fix: (a) extend test_all_criterion_checkers_accept_context_kwarg to inspect _check_impl_async whenever cls._check_impl is BaseCriterion._check_impl (and assert every checker overrides exactly one of the two); (b) add a test asserting SuccessChecker(...)._is_native_async("llm_judge") is True and ... ("agent_judge") is True against the real registry — this is the one-line guard that pins the whole point of the PR; (c) convert at least one llm_judge and one agent_judge test to await checker.check_all_async([criterion]) so the real checker runs on a live loop. (b) is also a good CE0nn lint-rule candidate: "a registered criterion must override exactly one of _check_impl / _check_impl_async, and its overriding method must accept context."
4. [Axis 6] check_all_async's asyncio.gather orphans sibling criterion work on JudgeInfrastructureError — abandoned sync batch thread and credentialed agent_judge sub-agents keep running/spending against a sandbox being torn down, with no test covering it (src/coder_eval/evaluation/checker.py:220) — Line 220 is await asyncio.gather(run_sync_batch(), *(run_async_one(i) for i in native_async_indices)) with the default return_exceptions=False. Per asyncio's documented contract, when one child raises "other awaitables in the aws sequence won't be cancelled and will continue to run" — and gather does not wait for them. _check_single_async deliberately re-raises JudgeInfrastructureError (checker.py:395-396), so a Bedrock retry-exhaustion / 4xx (judge_bedrock.py raises it on 403/400 and after _JUDGE_RETRY.max_retries) propagates out of check_all_async immediately while the siblings keep running. I verified both directions against the PR tree: (a) with [llm_judge(raises), file_exists(sync sleep 0.5s)], check_all_async raised at t=0.05s while the to_thread sync batch kept executing and only finished at t=0.51s — i.e. after the orchestrator's except/finally path had already run _cleanup(), which does await asyncio.to_thread(self.sandbox.cleanup, preserve=False) (orchestrator.py:2140) and, under the default MOVE_ON_WRITE, self.sandbox.preserve_to(...) (orchestrator.py:2109) — so run_command/uipath_eval criteria and their subprocesses are still reading/writing the sandbox directory while it is being moved or deleted; (b) with [llm_judge(raises), agent_judge-like], the sibling native-async coroutine was NOT cancelled and ran to completion orphaned (its finally only ran when it finished) — in production that is a real SubAgentRunner holding a Claude Code SDK subprocess plus a full mkdtemp copy of the sandbox (sub_agent.py:133-147), continuing to spend API budget for the rest of the batch with its result discarded and its token usage never folded into _accumulate_judge_usage. On main this could not happen: the orchestrator awaited a single asyncio.to_thread(self.success_checker.check_all, ...), so the thread had already finished before the exception surfaced. Fix: results_or_exc = await asyncio.gather(..., return_exceptions=True) (or an asyncio.TaskGroup), then re-raise the first JudgeInfrastructureError only after every child has settled — keeping the fail-loud escalation but guaranteeing no criterion work outlives check_all_async. Add a regression test with two criteria (the existing test_judge_infrastructure_error_propagates in tests/test_check_all_async.py:157-161 uses a single criterion, so the abort-with-siblings semantics is untested).
5. [Axis 8] Concurrent dispatch overlaps sandbox-reading judge criteria with the sandbox-mutating sync criterion batch, so identical agent output can score differently (composition regression vs. main's serialized check_all) (src/coder_eval/evaluation/checker.py:220) — await asyncio.gather(run_sync_batch(), *(run_async_one(i) for i in native_async_indices)) (checker.py:220) drops the guarantee that criteria observe the sandbox in declaration order. The sync batch runs in a to_thread slot while llm_judge/agent_judge read the SAME sandbox on the loop. Two first-party sync criteria mutate that sandbox: run_command (sandbox.run_commandsubprocess.run(command, shell=True, cwd=self.sandbox_dir), sandbox.py:933-943; the documented example at docs/TASK_DEFINITION_GUIDE.md:693 is command: "python app.py") and uipath_eval (--output-file {output_filename} written into the sandbox, uipath_eval.py:66-73). The judges read it synchronously at the top of their coroutine: ).build(sandbox, reference_code, turn_records) at llm_judge.py:92 and agent_judge.py:147 (→ sandbox.get_file_content / host_path.read_text), and agent_judge additionally copytrees the whole sandbox (sub_agent.py:140-146).

I reproduced the score change against this PR's code with criteria [RunCommandCriterion(command="sleep 0.4 && echo hi > built.txt"), LLMJudgeCriterion(...)] and a judge stub that reads built.txt:

  • check_all_async[('run_command', 1.0), ('llm_judge', 0.0, 'saw built.txt=False')]
  • old serial check_all[('run_command', 1.0), ('llm_judge', 1.0, 'saw built.txt=True')]
    Same sandbox, same agent output, different score → different weighted score and final_status. With a faster command the outcome flips run-to-run, and agent_judge's copytree over a concurrently-written tree can raise FileNotFoundError, which handle_criterion_errors_async (base.py:141-147) converts into a silent score=0.0.

Fix: either serialize judge criteria against the sandbox-mutating batch (run the sync batch to completion before the native-async batch when any criterion can write the sandbox), or snapshot the sandbox once before dispatch and have all criteria read the snapshot. If concurrency is kept, the docstring claim at checker.py:180-189 must be extended with the new ordering hazard, docs/TASK_DEFINITION_GUIDE.md must state that criteria are no longer order-isolated, and tests/test_check_all_async.py needs a determinism test for the mutate-vs-read overlap — it currently asserts only result ORDER (test_result_order_preserved_regardless_of_dispatch_path), so the full suite being green does not cover this class. n/a

Non-blocking, but please consider before merge

  1. [Axis 3] check_all_async's reference_code / reference_dir persistence (checker.py:192,194) is uncovered — no test passes a reference to the new async entry point that all 3 orchestrator call sites use (src/coder_eval/evaluation/checker.py:192) — Coverage reports checker.py lines 192 and 194 missing (reproduced locally: src/coder_eval/evaluation/checker.py 146 11 48 7 90.72% 55->51, 57->61, 59->57, 159, 161, 192, 194, 385-386, 397-415). Those are the reference-persist assignments in the new public check_all_async:
191    if reference_code is not None:
192        self._reference_code = reference_code
193    if reference_dir is not None:
194        self._reference_dir = reference_dir

Line 196 (self._turn_records = turn_records) IS covered, so the gap is specific: no test ever calls check_all_async with a reference at all. Yet all three production call sites do — orchestrator.py:1349-1353 / :1407 / :1499 pass reference_code=reference_code, reference_dir=reference_dir, and those orchestrator tests replace the checker with AsyncMock (test_orchestrator.py:2138) so they only assert kwarg forwarding, never the handling. The old sync twin does have coverage via tests/test_reference_evaluator.py:24 (checker.check_all([], reference_code=reference_code)), so the tested path and the shipped path have diverged: any drift between check_all's block (lines 156-164) and check_all_async's copy (lines 191-199) would silently change every reference_comparison score, llm_judge include_reference prompt, and agent_judge _reference/ mount without a failing test. Fix: add a check_all_async case to tests/test_reference_evaluator.py asserting the reference reaches the checker (e.g. a reference_comparison criterion scored via await checker.check_all_async([c], reference_code=...), plus agent_judge's reference_dir forwarding).
2. [Axis 7] Criterion extension-point contract changed without updating its authoring docs (EXTENDING.md / CLAUDE.md still describe the pre-PR _check_impl surface) (src/coder_eval/criteria/base.py:175) — The PR touches no .md file (diffstat: 20 files, all .py), yet it changes the documented implementation contract described at base.py:175 ("register_criterion enforces that a checker overrides at least one of the two"). Stale surfaces, verified at PR HEAD:

  • docs/EXTENDING.md:164 — the canonical "Step 2 — the checker" sample still shows only def _check_impl(self, criterion, sandbox, reference_code=None, *, turn_records=None, context=None), and docs/EXTENDING.md:177 says "Do not override check() — it's final and wraps _check_impl with error handling". Nothing mentions check_async, _check_impl_async, that async is now the primary surface, or that register_criterion can now raise TypeError. Its "Notes" list does document aggregate and live_verdict/live_stop_polarities, so the omission is drift, not a deliberate scope line.
  • CLAUDE.md:185 — the Evaluation Flow still reads 2. SuccessChecker.check_all() → List[CriterionResult], but all three orchestrator call sites are now await self.success_checker.check_all_async( (orchestrator.py:1349, 1407, 1499). CLAUDE.md's "Adding a New Criterion" (4 steps) and the criteria/base.py tree blurb ("BaseCriterion (incl. default aggregate()) + @handle_criterion_errors") likewise never mention the async twin or handle_criterion_errors_async.
    No doc-parity lint rule reaches this: CE027 covers env vars, CE028 doc indexes, CE029 YAML examples, CE030 model-field parity for TaskDefinition/RunLimits/Dataset/SimulationConfig, CE031 dead config fields — none checks the extension-point contract in docs/EXTENDING.md.

Fix: update docs/EXTENDING.md §2 (which method to implement and why, the both/neither rule, the new registration TypeError) and CLAUDE.md's Evaluation Flow + "Adding a New Criterion", and consider a CE032-style doc-parity rule asserting that every _check_impl* name in criteria/base.py appears in docs/EXTENDING.md.
3. [Axis 7] Sync/async twin duplication introduced by the async split: _check_single_async is a verbatim clone of _check_single (incl. its 32-line error tail, partly unreachable and untested), the reference/turn-record persist preamble is triplicated, and handle_criterion_errors_async clones its sync twin (src/coder_eval/evaluation/checker.py:346) — _check_single_async (checker.py:346-415) is a near-verbatim copy of _check_single (checker.py:261-345); a line diff of the two bodies shows the only functional difference is checker.py:286 result = checker.check( vs checker.py:362 result = await checker.check_async(. Everything else — the CheckContext construction, result.pass_threshold/result.gating assignment, the two log statements, and all three except arms — is duplicated, and the copy silently drops the rationale comments that explain the behaviour, e.g. _check_single has "# An informational (weight: 0) criterion cannot fail the task, so logging it as FAILED contradicts the final status." and raise # judge infra failure escalates to FinalStatus.ERROR; do not score it while the async copy has neither. The reference/turn-record persist preamble is likewise now triplicated verbatim: check (checker.py:124-133), check_all (155-164), check_all_async (191-199). The resulting public surface for one feature on SuccessChecker is check (sync, single — self-described at checker.py:110 as a "backward compatibility wrapper", in a project whose CLAUDE.md Design Principles say "Greenfield project: No worries about backward compatibility"), check_all (sync), check_all_async (async), with no async single-criterion twin, plus _check_all_sync / _check_single / _check_single_async / _is_native_async; check_all_async also omits the Args:/Returns: block its sync twin carries at checker.py:145-153.

Fix: collapse the duplication — make _check_single_async the single implementation and have _check_single be asyncio.run(self._check_single_async(...)), or factor the shared pre/post/except handling into one helper parameterised by the invoke callable; extract the 9-line persist preamble into one _absorb_defaults(...) helper used by all three entry points; and, since nothing in src/ calls SuccessChecker.check or check_all any more (only tests do), decide explicitly whether the sync entry points remain supported API (then document the async-context constraint) or are deleted per the greenfield principle.

Nits

  1. [Axis 1] Partition bookkeeping in check_all_async: dead guard, O(n^2) membership, # type: ignore prefill (src/coder_eval/evaluation/checker.py:201) — Three small smells in the new dispatch block: (1) if not criteria: return [] (checker.py:201-202) is unreachable-by-necessity — with empty criteria, native_async_indices/sync_indices are empty, results is [], run_sync_batch returns immediately, and the function already returns []; (2) sync_indices = [i for i in range(len(criteria)) if i not in native_async_indices] (checker.py:205) does list-membership inside a loop (O(n^2)) where the partition is naturally one pass, e.g. native = {...} / sync_indices = [i for i in range(len(criteria)) if i not in native], or a single for i, c in enumerate(criteria) appending to two lists; (3) the [None] * len(criteria) prefill forces return results # type: ignore[return-value] (checker.py:221) — the invariant "every slot filled" is asserted in a comment rather than to the type checker. Building the two ordered sub-lists and re-interleaving them (or cast after an explicit assert all(r is not None for r in results)) keeps the deliberate single-thread-slot semantics for sync criteria while removing the escape hatch.
  2. [Axis 1] Test docstrings still name the pre-rename invoke_anthropic_judge / invoke_bedrock_judge (tests/test_llm_judge_criterion.py:28) — The PR renames both judge invokers to *_async, and every call site was updated, but three docstrings in the same PR still name the old symbols: "The judge now dispatches through ``invoke_anthropic_judge`` / ``invoke_bedrock_judge``, both of which return an Anthropic-native message dict" (tests/test_llm_judge_criterion.py:28-29) and """DirectRoute (anthropic transport) uses invoke_anthropic_judge returning a dict.""" (tests/test_llm_judge_criterion.py:996). Since the same file patches coder_eval.criteria.llm_judge.invoke_anthropic_judge_async (line 437), the docstrings now point at names that no longer exist — append _async in all three places.
  3. [Axis 2] New test doubles for the changed extension point carry no annotations at all, and pyright excludes tests/ — the one place a _check_impl* signature drift would be caught is untyped (tests/test_check_all_async.py:60) — The three stand-ins that model the new contract declare fully unannotated overrides — async def _check_impl_async(self, criterion, sandbox, reference_code=None, *, turn_records=None, context=None): (lines 60 and 86) and def _check_impl(self, criterion, sandbox, reference_code=None, *, turn_records=None, context=None): (line 75). Because pyproject.toml's [tool.pyright] block sets exclude = [... "tests" ...] (pyproject.toml:216), nothing checks them against BaseCriterion, so a future base-signature change silently leaves these doubles compatible-looking while production overrides break. The same gap lets line 197 pass a None sandbox — asyncio.run(checker.check_async(_file_criterion("f"), sandbox=None)) — where the base declares sandbox: "Sandbox". Annotate the doubles to mirror the base exactly (criterion: LLMJudgeCriterion, sandbox: Sandbox, turn_records: list[TurnRecord] | None, context: CheckContext | None, -> CriterionResult) so the compatibility assertion is real rather than incidental.
  4. [Axis 3] Concurrency proofs rely on wall-clock margins (100 ms of slack) rather than a deterministic barrier (tests/test_check_all_async.py:111) — assert elapsed < SLEEP_SECONDS * 1.5, f"judge criteria serialized: took {elapsed:.3f}s" (line 111, and the same pattern at line 127) proves concurrency by timing: with SLEEP_SECONDS = 0.2 (line 31), concurrent is ~0.2 s and the bound is 0.3 s, leaving 100 ms of slack — and pytest runs with -n auto (pyproject.toml:286), so these execute under full CPU oversubscription. Line 127's case is the tighter one: it also needs a ThreadPoolExecutor thread to spin up for asyncio.to_thread before its time.sleep(0.2) starts. I could not make either flake (5 runs under 12 concurrent busy-loop processes all passed), so this is informational only. A deterministic barrier removes the timing dependency entirely: have _SleepyAsyncChecker increment a shared in-flight counter, await an asyncio.Event set by the last arrival, and assert the peak in-flight count == 2 — that fails loudly if the paths serialize, regardless of machine speed.
  5. [Axis 6] _is_native_async constructs the checker outside the per-criterion error boundary, so a checker-construction failure aborts the whole task instead of scoring one criterion 0 (src/coder_eval/evaluation/checker.py:204) — native_async_indices = [i for i, c in enumerate(criteria) if self._is_native_async(c.type)] (checker.py:204) runs before any try/except, and _is_native_async only guards except KeyError: return False around checker = self._get_checker_instance(criterion_type) (checker.py:232-235) — which instantiates the checker class (self._checker_instances[criterion_type] = checker_class(), checker.py:258). Any non-KeyError raised by a checker's constructor therefore escapes check_all_async entirely and lands as a whole-task FinalStatus.ERROR, contradicting the guarantee _check_single explicitly documents at checker.py:326 (# V3: Catch ALL exceptions, including checker __init__ failures) and implements at checker.py:325-344 (score 0.0, details="Error running checker"). A constructor that raises KeyError is worse: _is_native_async swallows it, routes the criterion to the sync batch, and the result claims "No checker registered for criterion type '<type>'" — a misleading diagnostic for a type that is registered. No in-tree checker defines __init__, so this is latent today. Fix: wrap the _is_native_async classification in the same try/except boundary (or derive it from the registered class via CriterionRegistry.get_checker(type)._check_impl_async without instantiating), and narrow the KeyError catch to CriterionRegistry.get_checker only.
  6. [Axis 6] SubAgentRunner's temp-dir cleanup is now an await inside finally, making judge-sandbox-copy removal cancellation-dependent rather than guaranteed (src/coder_eval/evaluation/sub_agent.py:209) — The finally now reads await asyncio.to_thread(shutil.rmtree, judge_dir, ignore_errors=True) (sub_agent.py:209), replacing main's plain shutil.rmtree(judge_dir, ignore_errors=True). Because run_async is awaited directly on the orchestrator's loop (agent_judge.py:178 turn = await runner.run_async(...)) instead of running under its own asyncio.run on a worker thread, this cleanup is now reachable by cancellation — the task_timeout ThreadedWatchdog cancels the orchestrator task (orchestrator.py:471 asyncio_task_to_cancel=asyncio.current_task()), and _run_agent's cleanup (except BaseException: await agent.kill() / finally: await agent.stop(), sub_agent.py:229-235) is likewise await-based. A single cancel still lets these awaits complete, but if cancellation arrives while the finally is being entered (or after the default executor is shut down) the mkdtemp copy of the whole sandbox is left behind in the OS temp dir with no reaper. Existing tests cover success/crash/timeout cleanup (tests/test_sub_agent_runner.py:233, 254, 281, 300) but not cancellation. Fix: make the last-resort cleanup non-cancellable — either call shutil.rmtree(judge_dir, ignore_errors=True) directly in the finally (it is best-effort and bounded) or wrap the offload in asyncio.shield(...), and add a test that cancels the run_async task and asserts judge_dir is gone.
  7. [Axis 8] Stale docstring: _run_dialog_criteria_check still claims criteria run off the event loop (src/coder_eval/orchestrator.py:1486) — orchestrator.py:1486 still reads "check_all off the event loop, fold this turn's judge usage into the" while the call three lines below is now criteria_results = await self.success_checker.check_all_async(...) (orchestrator.py:1499) — the judge criteria run ON the loop. Update the docstring to say check_all_async and drop "off the event loop", so the next reader doesn't assume the check phase is still thread-isolated (the assumption that motivates finding 1 and 4). n/a

What's Missing

Parallel paths:

  • 🟠 Both native-async criteria still do their file I/O synchronously on the event loop — JudgeContextBuilder(...).build(sandbox, ...) at llm_judge.py:92 and agent_judge.py:147 (→ sandbox.get_file_content / host_path.read_text in judge_context.py:282/296), and SubAgentRunner's pre-copy path — while every sync criterion got a free asyncio.to_thread hop from the new base default (_check_impl_async, base.py:326). The PR moved these two off the worker thread without wrapping their blocking reads, so they now stall the loop (and, in simulation every_turn mode, the live agent stream) and read the sandbox before their first await; CE002 (tests/lint/rules/no_blocking_io_in_async.py) cannot see it because the blocking calls sit one module away in judge_context.py, so its _BLOCKING_* allowlist needs the wrapper (or the build needs to_thread). (trigger: src/coder_eval/criteria/llm_judge.py)
  • 🟡 The authoring docs for the extension point this PR redefines were not touched: docs/EXTENDING.md:164/177 still shows only _check_impl and "wraps _check_impl with error handling", and CLAUDE.md:49/185 still says @handle_criterion_errors / SuccessChecker.check_all()git grep check_async docs CLAUDE.md README.md returns zero hits at PR HEAD. (trigger: src/coder_eval/criteria/base.py) (restates: Axis 7: Criterion extension-point contract changed without updating its authoring docs)
  • 🟡 The new both-defaults guard was added only to the register_criterion decorator (base.py:439-440); the parallel registration entry point CriterionRegistry.register (criteria/init.py:25, exported in __all__) and direct instantiation were not updated, so the mutual-recursion contract is unenforced on those paths — the guard belongs on BaseCriterion.__init_subclass__. (trigger: src/coder_eval/criteria/base.py) _(restates: Axis 2: The new 'override one of _check_impl / check_impl_async' contract has no static or complete runtime enforcement)
  • 🟡 All three production call sites moved to check_all_async (orchestrator.py:1349/1407/1499) but the sync twins SuccessChecker.check (checker.py:110) and check_all (checker.py:136) were left byte-identical and undocumented — no src/ caller remains, their persist preamble is now triplicated (124-133 / 155-164 / 191-199), and there is no async single-criterion twin, so the tested path and the shipped path have permanently diverged. (trigger: src/coder_eval/evaluation/checker.py) (restates: Axis 7: Sync/async twin duplication introduced by the async split)

Tests:

  • 🟠 No test drives a registered llm_judge / agent_judge through check_all_async — the new file injects fakes into checker._checker_instances (test_check_all_async.py:103) and every judge test still uses the sync .check() bridge, so nothing pins _is_native_async("llm_judge"/"agent_judge") is True against the real registry (the one-line assertion that encodes the whole point of the PR). (trigger: tests/test_check_all_async.py) (restates: Axis 3: test_registry's context-kwarg conformance guard is vacuous for llm_judge/agent_judge and no test pins native-async dispatch)
  • 🟠 check_all_async's reference-persist branches (checker.py:192, 194) are reported missing by coverage — no test passes reference_code= / reference_dir= to the async entry point that all three orchestrator sites use, even though the sync twin has that coverage via tests/test_reference_evaluator.py:24. (trigger: src/coder_eval/evaluation/checker.py) (restates: Axis 3: check_all_async's reference_code / reference_dir persistence is uncovered)
  • 🟠 test_judge_infrastructure_error_propagates (test_check_all_async.py:158) uses a single criterion, so the new abort-with-siblings semantics of the asyncio.gather at checker.py:220 is untested — a two-criterion regression test (judge raises + slow sibling, asserting no work outlives check_all_async) is the missing guard. (trigger: tests/test_check_all_async.py) (restates: Axis 6: check_all_async's asyncio.gather orphans sibling criterion work on JudgeInfrastructureError)
  • 🟠 The new file asserts only result order (test_result_order_preserved_regardless_of_dispatch_path, :132) and deliberately locks in the overlap (:115); there is no determinism test for a sandbox-mutating sync criterion (run_command / uipath_eval) overlapping a sandbox-reading judge, which is exactly the scoring change the PR introduces. (trigger: tests/test_check_all_async.py) (restates: Axis 8: Concurrent dispatch overlaps sandbox-reading judge criteria with the sandbox-mutating sync batch)
  • 🟡 Cancellation is now reachable in the check phase (the task_timeout watchdog cancels the orchestrator task at orchestrator.py:471 while judges run on the loop), but no test cancels a check_all_async or SubAgentRunner.run_async task and asserts the mkdtemp judge copy is removed — existing cleanup tests cover only success/crash/timeout (test_sub_agent_runner.py:233/254/281/300). (trigger: src/coder_eval/evaluation/sub_agent.py) (restates: Axis 6: SubAgentRunner's temp-dir cleanup is now an await inside finally)
  • 🟡 No test covers the new sync bridge (_check_implasyncio.run, base.py:276) being hit from inside a running loop; every judge test calls .check() from a sync test where no loop exists, so the silent-0.0 failure mode below has no regression guard. (trigger: src/coder_eval/criteria/base.py)
  • 🔵 A single cached checker instance can now execute in the worker thread (_check_all_sync) and on the loop (_check_single_async) within one gather, but nothing (test or CEnnn rule) asserts checkers hold no per-check instance state — a stateless-checker invariant that used to be implied by serial execution is now load-bearing and unstated. (trigger: src/coder_eval/evaluation/checker.py)

Downstream consumers:

  • 🟠 The retained sync API now silently mis-scores judges when called from inside a running loop: verified in the PR worktree that SuccessChecker.check(LLMJudgeCriterion(...)) under asyncio.run returns score: 0.0 | error: RuntimeError: asyncio.run() cannot be called from a running event loop (base.py:276 asyncio.run swallowed by handle_criterion_errors, base.py:79-97) — a false agent failure, not an infra error. llm_judge newly acquires this (on main its _check_impl was pure sync); check/check_all docstrings (checker.py:110, 136) still advertise no constraint, and there is no get_running_loop() guard or re-raise for out-of-tree/eval-runner consumers of the sync surface. (trigger: src/coder_eval/criteria/base.py) (restates: Axis 7: Sync/async twin duplication introduced by the async split)
  • 🟡 Judge spend accounting has no path for orphaned/cancelled judges: an abandoned llm_judge/agent_judge still bills tokens but its JudgeCriterionResult.token_usage is discarded, so _accumulate_judge_usage (orchestrator.py:1277), run.json judge cost, and the evalboard cost views undercount — the counting consumers were not updated for the new partial-completion state. (trigger: src/coder_eval/orchestrator.py) (restates: Axis 6: check_all_async's asyncio.gather orphans sibling criterion work on JudgeInfrastructureError)

Daily/nightly:

  • 🟠 The PR rewrites the nightly's judge transport (--backend bedrock: sync httpx.post → a per-call httpx.AsyncClient() with no pooling, retry via asyncio.sleep) and makes judge calls fan out concurrently, but says nothing about nightly blast radius: batch.py:81's semaphore caps tasks only, so concurrent judge requests now scale as --max-parallel × judges-per-task with no cap, raising 429/throttle exposure on a path where retry exhaustion raises JudgeInfrastructureError → whole-task FinalStatus.ERROR (and now orphans siblings). No semaphore, no measurement, and no note on the external coder-eval-uipath / eval-runner nightly. (trigger: src/coder_eval/evaluation/judge_bedrock.py)
  • 🟡 Because criteria no longer observe the sandbox in declaration order, nightly scores for any task pairing a mutating sync criterion with a judge can move against history with no agent change — the PR neither states this nor proposes a re-baseline, and docs/TASK_DEFINITION_GUIDE.md still implies criteria are order-isolated. (trigger: src/coder_eval/evaluation/checker.py) (restates: Axis 8: Concurrent dispatch overlaps sandbox-reading judge criteria with the sandbox-mutating sync batch)

Harness & Lint Improvements

Static checks (lint / type):

  • [ce-lint] CE026 — asyncio.gather must declare return_exceptions= explicitly (or use asyncio.TaskGroup). New rule file tests/lint/rules/ce026_gather_explicit_return_exceptions.py (class GatherExplicitReturnExceptions(BaseRule), id = "CE026"), scoped to src/coder_eval/, wired into ALL_RULES in tests/lint/runner.py and added to [tool.ruff.lint].external in pyproject.toml. Mechanism is a direct clone of CE015 (create_subprocess_* must pass limit=): flag any ast.Call whose func is asyncio.gather with no return_exceptions keyword, message = "gather() with the default return_exceptions=False abandons sibling tasks on the first exception (they are neither cancelled nor awaited); pass return_exceptions=True and re-raise after all children settle, or use asyncio.TaskGroup". CE026 is the next free number (rules/ tops out at CE025; CE027-CE031 are the doc-surface pytest classes). Baseline: src/coder_eval/orchestration/batch.py:171 already complies; src/coder_eval/orchestrator.py:2001 (gather inside asyncio.wait_for for stdout/stderr pumps) needs either an explicit kwarg or a # noqa: CE026 with the reason, so the rule ships with exactly one site to triage. Prevents: The critical/high cluster at src/coder_eval/evaluation/checker.py:220await asyncio.gather(run_sync_batch(), *(run_async_one(i) ...)) with default return_exceptions=False: the reproduced orphaned to_thread sync batch still writing the sandbox while _cleanup() moves/deletes it (orchestrator.py:2109/2140), and the orphaned agent_judge sub-agent holding a Claude Code SDK subprocess + mkdtemp sandbox copy and spending budget after the result is discarded (A6-high, cross-reported by A3/A4/A5/A8).
  • [ce-lint] CE032 — a BaseCriterion subclass must override exactly one of _check_impl / _check_impl_async, and the overriding method must accept a context keyword parameter. New tests/lint/rules/ce032_criterion_impl_contract.py, scoped by path regex [/\\]coder_eval[/\\]criteria[/\\] with base.py exempted (same scoping as CE025), wired into ALL_RULES. For each ClassDef whose bases mention BaseCriterion: count FunctionDef/AsyncFunctionDef members named _check_impl / _check_impl_async; violate on zero ("overriding neither mutually recurses between the two base defaults until RuntimeError: can't start new thread"), violate on both ("the both-overridden case is dispatch-ambiguous and is not rejected by register_criterion"), and violate when the single override lacks a context kwarg or declares a stale one (route / reference_dir / proxy). This is the AST half of the guard that currently lives only at criteria/base.py:439 inside register_criterion — which CriterionRegistry.register (criteria/init.py:25, exported in __all__) bypasses entirely. Prevents: A2-high (criteria/base.py:245 lost its @abstractmethod, BaseCriterion now has zero abstract methods, and the sole runtime guard is skippable) and A3-high part (2): tests/test_registry.py:66 inspects _check_impl only, which resolves to BaseCriterion._check_impl for the two criteria that actually moved to async — so its "missing 'context' kwarg" and removed-kwarg assertions are provably vacuous for llm_judge/agent_judge, and re-adding a route= kwarg to LLMJudgeChecker._check_impl_async passes the suite today.
  • [pyright] Restore a statically visible abstract seam and turn on reportImplicitOverride = "error". (a) Keep exactly one @abstractmethod in the criterion contract — e.g. declare _check_impl_async abstract on BaseCriterion and let _check_impl's default be the derived one (or split a tiny CriterionImpl Protocol/ABC that declares the pair) — so pyright's already-enabled reportAbstractUsage (standard mode) once again errors on an incomplete subclass at edit time. Verified today: class Incomplete(BaseCriterion[FileExistsCriterion]) + Incomplete() gives 0 errors, and the control probe with @abstractmethod gives Cannot instantiate abstract class, so the check is live and only the declaration was removed. (b) Add reportImplicitOverride = "error" to [tool.pyright] so every _check_impl* / check* override must carry @override, making signature drift on the extension point a type error rather than an incidental pass. Prevents: A2-high: the loss of the static signal for a missing override, plus the mutual asyncio.runasyncio.to_thread recursion (base.py:276 ⟷ base.py:326) that currently burns OS threads and dies with RuntimeError: can't start new thread instead of naming the missing override. (b) also backstops A2-low (test doubles at tests/test_check_all_async.py:60/75/86 that only look signature-compatible).
  • [pyright] Type-check tests/. Drop "tests" from [tool.pyright] exclude (pyproject.toml:216) and add an executionEnvironments entry for tests with a relaxed profile (typeCheckingMode = "basic", reportMissingParameterType = "none" initially, reportArgumentType/reportIncompatibleMethodOverride = "error"), so test doubles that subclass production ABCs are checked against them while unannotated fixture plumbing stays quiet. Tighten in a second pass once the baseline is clean. Prevents: A2-low: the three BaseCriterion stand-ins in tests/test_check_all_async.py (lines 60, 75, 86) declare fully unannotated _check_impl* overrides, and line 197 passes sandbox=None where the base declares sandbox: "Sandbox" — the one place a _check_impl* signature drift would be caught is the one place pyright never looks. Also the general class behind A3-high (test doubles diverging from the real registered checkers).
  • [ce-lint] CE033 — no signature-erasing decorators in src/. New tests/lint/rules/ce033_no_signature_erasing_decorator.py, wired into ALL_RULES, scoped to src/coder_eval/. Flag any FunctionDef that both takes a parameter annotated Callable[..., T] and returns Callable[..., T] (i.e. a decorator factory that erases its target's signature), and any def wrapper(self: Any, ...) inside such a factory. Remediation named in the message: inline the try/except JudgeInfrastructureError: raise / except Exception: body into the method, or type the decorator with PEP 695 ParamSpec / a Protocol carrying the exact call signature. Scope is tight — grep -rn 'Callable\[\.\.\.' src/ shows only criteria/base.py:48 (+ the new async twin) is a decorator factory; the other five hits are plain parameter annotations and would not fire. Prevents: A2-high (base.py:102-104 / 112-119 / 286): handle_criterion_errors_async erases check_async, so reveal_type(FileExistsChecker().check_async) is (...) -> Awaitable[CriterionResult] and a call passing the wrong criterion model plus four surplus positional args type-checks with 0 errors while raising TypeError at runtime. Fires on the pre-existing sync twin (handle_criterion_errors at base.py:48) too, which is the point — the fix should cover both.
  • [ce-lint] CE034 — sync/async twins must delegate, not duplicate. New tests/lint/rules/ce034_async_twin_delegates.py, scoped to src/coder_eval/, wired into ALL_RULES. Within one ClassDef (or module, for free functions), when both foo and foo_async exist, require that one body textually references the other (call/attribute on the twin name); if neither references the other and both bodies exceed ~10 statements, violate with "sync/async twins must share one implementation — have the sync twin call asyncio.run(self.foo_async(...)) or factor the shared pre/post/except handling into one helper parameterised by the invoke callable". BaseCriterion._check_impl / _check_impl_async already delegate to each other and pass unchanged. Prevents: A7-medium: _check_single_async (checker.py:346-415) is a byte-identical clone of _check_single (261-344) apart from checker.check( vs await checker.check_async( at 286/362, with every rationale comment silently dropped in the copy; plus handle_criterion_errors / handle_criterion_errors_async. (The triplicated 9-line reference/turn-record persist preamble at checker.py:124-133 / 155-164 / 191-199 is the same theme but a different shape — it is what the _absorb_defaults(...) extraction in the finding addresses; CE034 as specified does not reach it.)
  • [ce-lint] CE035 — last-resort cleanup in a finally block must not be cancellation-dependent, paired with a finally-block carve-out in CE002. New tests/lint/rules/ce035_finally_cleanup_not_cancellable.py: inside an ast.Try.finalbody in an async function, flag await asyncio.to_thread(<fs-teardown>) where <fs-teardown> is shutil.rmtree / os.remove / os.unlink / Path.unlink etc., message = "a bare await in finally is skippable under cancellation; call the bounded best-effort teardown synchronously or wrap it in asyncio.shield(...)". Simultaneously add the reciprocal exemption to tests/lint/rules/no_blocking_io_in_async.py (CE002) for those same calls inside a finalbody — recording the interaction matters because CE002's own shutil coverage is what pushed the to_thread wrap in, and without the carve-out the two rules contradict each other. Prevents: A6-low at src/coder_eval/evaluation/sub_agent.py:209await asyncio.to_thread(shutil.rmtree, judge_dir, ignore_errors=True) in the finally, now reachable by the task-timeout watchdog's cancel (orchestrator.py:471) because run_async is awaited on the orchestrator loop, leaving a full mkdtemp copy of the sandbox in the OS temp dir with no reaper.
  • [ce-lint] CE036 — tests must not assert on measured wall-clock elapsed time. New tests/lint/rules/ce036_no_wallclock_assertions.py, scoped to tests/, wired into ALL_RULES. Track names bound to a difference of time.perf_counter() / time.monotonic() / time.time() calls and flag assert <that name> <cmp> <literal>; message points at the deterministic alternative (shared in-flight counter + asyncio.Event barrier, assert peak concurrency). 6 existing candidate sites in tests/ → ship with # noqa: CE036 on any that are deliberately load/perf tests. Prevents: A3-low at tests/test_check_all_async.py:111 and :127 — assert elapsed < SLEEP_SECONDS * 1.5 proves concurrency with 100 ms of slack while pytest runs -n auto (pyproject addopts) under full CPU oversubscription, and line 127 additionally waits on a ThreadPoolExecutor thread spin-up before its time.sleep(0.2) begins.
  • [ce-lint] CE037 — extension-point doc parity (whole-tree / Markdown rule). Wire as a dedicated @pytest.mark.lint class TestCE037ExtensionPointDocParity in tests/test_custom_lint.py alongside CE027-CE031 (it reasons over Markdown + a src/ AST, so it is not a per-file BaseRule), with a helper module tests/lint/doc_extension_parity.py. Assert: (a) every overridable seam declared on BaseCriterion in src/coder_eval/criteria/base.py (methods matching _check_impl*, check/check_async, aggregate, live_verdict) is mentioned as inline code in docs/EXTENDING.md; (b) the SuccessChecker entry point actually called from src/coder_eval/orchestrator.py appears in CLAUDE.md's Evaluation Flow block; (c) an EXEMPT table with reasons, mirroring CE030's escape hatch. Prevents: A7-medium: the PR's diffstat is 20 files, all .py, while git grep 'check_all_async\|_check_impl_async\|check_async' -- docs CLAUDE.md README.md returns zero hits — docs/EXTENDING.md:164/177 still documents only _check_impl and "check() … wraps _check_impl", CLAUDE.md:185 still says SuccessChecker.check_all() while all three orchestrator sites are await …check_all_async( (orchestrator.py:1349/1407/1499), and CLAUDE.md:49's tree blurb never mentions handle_criterion_errors_async or the new registration TypeError.
  • [ce-lint] CE038 — a project symbol named in a docstring must exist. New tests/lint/rules/ce038_docstring_symbol_exists.py, scoped to src/coder_eval/ + tests/, wired into ALL_RULES. Extract single/double-backticked tokens from docstrings that match a project-symbol shape (snake_case containing _, length ≥ 4, not a stdlib/allowlisted name) and fail when the token resolves to no top-level def/class/assignment anywhere under src/coder_eval/**, emitting a did-you-mean against the closest existing name. Ship with an allowlist + # noqa: CE038; the FP risk is real (prose words with underscores, third-party symbols) so the allowlist is part of the rule, not an afterthought. Prevents: A1-low at tests/test_llm_judge_criterion.py:28-29 and :996 (docstrings still name invoke_anthropic_judge / invoke_bedrock_judge after both were renamed *_async, in the same file that patches the _async names at line 437 — did-you-mean would land exactly), and A8-low at orchestrator.py:1486 (docstring says check_all "off the event loop" while line 1499 awaits check_all_async).
  • [pyright] reportUnnecessaryTypeIgnoreComment = "error" now, and evaluate enableTypeIgnoreComments = false as the follow-up. Adding the first is free and immediate. The second makes # type: ignore inert so the underlying error surfaces — 20 sites in src/ today, so land it behind a cleanup pass rather than in one commit; where an ignore must stay, require the narrow cast(...)-after-assert form instead. Prevents: A1-low at src/coder_eval/evaluation/checker.py:221return results # type: ignore[return-value], where the [None] * len(criteria) prefill means the "every slot filled" invariant is asserted in a comment rather than to the type checker; building the two ordered sub-lists and re-interleaving (or cast after assert all(r is not None ...)) removes the escape hatch.
  • [ruff] Add "PGH" to [tool.ruff.lint] select (currently E,F,I,N,W,UP,B,SIM,RUF,PLR0915,PLR0912). PGH003 forbids blanket # type: ignore without a rule code and PGH004 the bare # noqa, so any new suppression has to name what it is suppressing — the cheap, zero-migration companion to the pyright change above. Prevents: A1-low (checker.py:221's # type: ignore[return-value] is coded and would survive PGH003, but the rule stops the pattern degrading to blanket ignores as the async split is cleaned up) — this is the guardrail entry, not a direct catch, and is listed as such.

Harness improvements (not statically reachable):

  • Patch-coverage gate in make verify: add a diff-cover step (uv run diff-cover coverage.xml --compare-branch=origin/main --fail-under=100 for src/ lines) after the existing pytest --cov-fail-under=80 line in the Makefile (Makefile:57), and mirror it in CI. The repo already emits coverage.xml, so this is a one-line addition on existing plumbing. Why not static: Coverage is runtime execution data — no linter or type checker can know that a line was never executed by the suite. The existing project-wide 80% floor is insensitive to a two-line gap in a new function, which is exactly how this slipped through. Prevents: A3-medium: src/coder_eval/evaluation/checker.py:192 and :194 (the reference_code/reference_dir persist assignments inside the brand-new check_all_async) are reported missing in the reproduced coverage line, while all three production call sites pass those kwargs — no test ever calls check_all_async with a reference at all. Would also have surfaced the untested duplicated tail at checker.py:397-415.
  • Sync/async parity matrix over the criterion entry points: parametrize the criterion test suite on the entry point via a fixture (params=["sync", "async"]checker.check_all([...]) vs asyncio.run(checker.check_all_async([...]))) and assert identical CriterionResult lists (score, passed, pass_threshold, gating, details) for the same sandbox + criteria. Convert at least one llm_judge and one agent_judge case in tests/test_llm_judge_criterion.py / tests/test_agent_judge_criterion.py to the async entry point, and extend tests/test_reference_evaluator.py with a check_all_async(..., reference_code=..., reference_dir=...) case. Why not static: Requires executing both paths against a real sandbox and diffing the resulting objects; the equivalence of two code paths is a semantic property, and CE034 (twin delegation) can only forbid the copy-paste shape, not prove the copy behaves the same. Prevents: A3-high part (1) — every judge test goes through the sync .check() bridge (_check_implasyncio.run on a fresh loop) while production awaits _check_impl_async on the orchestrator's live loop, so the only two criteria that make real network calls / spawn SDK subprocesses have zero coverage on the loop they now run on. Also A7-medium (twin drift) and A3-medium (reference persistence).
  • Registry conformance test against the real registry: parametrize over every entry in CriterionRegistry and assert (a) SuccessChecker(...)._is_native_async(t) matches whether the registered class overrides _check_impl_async, pinning llm_judge/agent_judge as True; (b) the overridden _check_impl* — resolved as "whichever is not the base method" — accepts context and none of route/reference_dir/proxy. This replaces the currently vacuous inspection at tests/test_registry.py:66. Why not static: The registry is populated at runtime by pkgutil auto-discovery plus out-of-tree plugins, so classes CE032 never sees as AST (third-party checkers) are only observable after import; and _is_native_async's behaviour is an identity comparison evaluated at runtime, not a declaration. Prevents: A3-high both parts — nothing anywhere in the suite asserts native-async dispatch for a registered judge (tests/test_check_all_async.py:186 asserts it against an injected fake), and for llm_judge/agent_judge the test_registry guard resolves to BaseCriterion._check_impl and cannot fail.
  • Criterion-composition determinism test: a harness test that pairs a sandbox-mutating criterion with a sandbox-reading judge — e.g. [RunCommandCriterion(command="sleep 0.4 && echo hi > built.txt"), LLMJudgeCriterion(...)] with a judge stub that reads built.txt — and asserts (a) the scores are identical across N repeats and (b) identical between check_all and check_all_async. Pair it with a declared-and-consulted mutates_sandbox: ClassVar[bool] on BaseCriterion (CE014/CE025-style "declare it once on the class") so the dispatcher can order writers before readers instead of overlapping them. Why not static: Whether a criterion mutates the sandbox is semantic (it is subprocess.run(command, shell=True, cwd=sandbox_dir) behind a user-supplied string), and the score divergence only manifests when a real thread and a real coroutine interleave against a real filesystem. A linter can enforce that the flag is declared, never that it is correct. Prevents: The critical A8/A5 finding at checker.py:220, reproduced as run_command+llm_judge scoring 1.0/1.0 serially and 1.0/0.0 under gather on identical agent output — plus agent_judge's copytree (sub_agent.py:140) racing the worker thread's writes into a FileNotFoundError that handle_criterion_errors_async converts to a silent score=0.0. tests/test_check_all_async.py currently asserts result order only, so a green suite says nothing about this class.
  • Leak assertions as an autouse fixture: snapshot tempfile.gettempdir() entries matching the harness's mkdtemp prefixes and threading.active_count() before/after each orchestrator- and judge-level test, failing on residue; plus two explicit tests — one that cancels a SubAgentRunner.run_async task mid-flight and asserts judge_dir is gone, and one that raises JudgeInfrastructureError from a judge alongside a slow sibling and asserts no criterion work is still running when check_all_async returns (sentinel written by the sibling after the raise). Why not static: Leaked directories, surviving subprocesses and thread-pool exhaustion are process/filesystem state after execution; CE035 can forbid the cancellable finally shape but cannot prove nothing leaks, and the orphaned-sibling case needs a real event loop plus a real worker thread to observe. Prevents: A6-low (sub_agent.py:209 finally-await leaving a full sandbox copy in the OS temp dir with no reaper — existing tests at tests/test_sub_agent_runner.py:233/254/281/300 cover success/crash/timeout but not cancellation), A6-high (the existing test_judge_infrastructure_error_propagates at tests/test_check_all_async.py:158-161 uses a single criterion, so abort-with-siblings is untested), and the thread-exhaustion arm of A2-high (RuntimeError: can't start new thread).
  • Ship a deterministic-concurrency helper alongside the CE036 ban: a small ConcurrencyProbe in a shared test-helpers module (in-flight counter, asyncio.Event released by the last arrival, peak_in_flight property) so a concurrency proof reads assert probe.peak_in_flight == 2 instead of timing a wall clock. Retrofit tests/test_check_all_async.py:111 and :127. Why not static: The lint rule can forbid the timing assertion but cannot supply the replacement; without a ready-made barrier helper the ban just gets # noqa'd, and the barrier itself has to live in test code because it must be awaited from inside a fake checker. Prevents: A3-low — the 100 ms-of-slack assertions under -n auto (I could not make them flake, but the margin is the only thing holding, and line 127 also depends on a ThreadPoolExecutor thread spin-up).
  • Duplicate-block detector as a make verify step: add a token-window duplication check over src/ (e.g. uv run pylint --disable=all --enable=duplicate-code --min-similarity-lines=25 src/coder_eval, or jscpd) with a checked-in baseline so only new duplication fails. Why not static: It is a whole-tree similarity metric with a tunable threshold and a baseline file, not a per-AST predicate the BaseRule runner models — the CE runner parses one file at a time and has no notion of cross-function token windows. Listed here rather than in the static bucket for that reason. Prevents: A7-medium — the ~60-line verbatim _check_single_async clone of _check_single (checker.py:346-415 vs 261-344) and the triplicated persist preamble (124-133 / 155-164 / 191-199), neither of which CE034's name-pairing heuristic reaches in the free-function/preamble shape.

Top 5 Priority Actions

  1. Serialize the sandbox-mutating sync batch ahead of the native-async judges (or snapshot the sandbox once before dispatch) at src/coder_eval/evaluation/checker.py:220 — reproduced: a run_command writing built.txt alongside an llm_judge flips that criterion from 1.0 to 0.0 for identical agent output, changing the weighted score and final_status, and agent_judge's copytree can race into a silent score=0.0.
  2. Make the gather at src/coder_eval/evaluation/checker.py:220 settle all children before re-raising (return_exceptions=True or a TaskGroup) — today a JudgeInfrastructureError from src/coder_eval/evaluation/checker.py:395 escapes while the abandoned to_thread sync batch (verified still running ~0.46s later) and orphaned SubAgentRunner subprocesses read/write the sandbox that _cleanup is already moving or deleting (src/coder_eval/orchestrator.py:2109,2140).
  3. Restore real enforcement of the new extension contract: move the both-defaults guard out of register_criterion (src/coder_eval/criteria/base.py:439) into BaseCriterion.__init_subclass__ so it fires on every subclass including the CriterionRegistry.register path, keep one @abstractmethod so pyright flags a missing override again, and stop erasing the checking surface's signature in handle_criterion_errors_async (src/coder_eval/criteria/base.py:103) — as shipped, an incomplete checker type-checks cleanly and dies with RuntimeError: can't start new thread instead of naming the missing method.
  4. Close the three lost test guards: make tests/test_registry.py:66 fall back to _check_impl_async (it is provably vacuous for llm_judge/agent_judge, the only CheckContext consumers), assert _is_native_async("llm_judge"/"agent_judge") is True against the real registry, and cover check_all_async's reference persistence at src/coder_eval/evaluation/checker.py:192,194 (both lines uncovered while all three orchestrator call sites pass those kwargs).
  5. Update the extension-point docs and collapse the twins: docs/EXTENDING.md:164,177 and CLAUDE.md:49,185 still describe only the pre-PR sync surface (zero mentions of check_async/_check_impl_async/check_all_async anywhere in the doc tree), src/coder_eval/orchestrator.py:1486 still claims criteria run "off the event loop", and _check_single_async (src/coder_eval/evaluation/checker.py:346) is a verbatim clone of _check_single differing only in one await, with the persist preamble triplicated across three entry points.

Stats: 1 🔴 · 4 🟠 · 3 🟡 · 7 🔵 across 8 axes reviewed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Judge criteria (llm_judge/agent_judge) serialize and pin threads during check_all

2 participants