feat(criteria): run judge criteria concurrently instead of serializing them

[akshaylive]

Summary

• llm_judge and agent_judge each made a blocking network/subprocess call inside SuccessChecker.check_all's single asyncio.to_thread offload, so a task stacking multiple judge-type criteria ran their LLM calls serially and pinned a thread-pool thread for the full wait (Judge criteria (llm_judge/agent_judge) serialize and pin threads during check_all #55).
• BaseCriterion gains an async check_async/_check_impl_async path (default: offloads the sync _check_impl to a worker thread) plus a supports_native_async marker.
• llm_judge/agent_judge override it with genuine non-blocking I/O: AsyncAnthropic, httpx.AsyncClient (Bedrock), and a new SubAgentRunner.run_async.
• SuccessChecker.check_all_async gathers all native-async criteria concurrently with each other while the remaining sync criteria still share a single to_thread slot; the orchestrator's three call sites now use it instead of wrapping the whole check_all in asyncio.to_thread.

Test plan

• make check / make typecheck / make lint clean on all touched files
• make test — 3576 passed, 2 skipped
• New tests/test_check_all_async.py proves judge criteria overlap instead of serializing, the sync/async batches overlap, result ordering is preserved, and error handling (KeyError / generic exception / JudgeInfrastructureError) matches the sync path

Closes #55

🤖 Generated with Claude Code

[github-actions]

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

---

Code Review in Progress

Todo List:

• Read .github/code_review.md for review standards
• Read CLAUDE.md for project conventions
• Get full diff with git diff origin/main...HEAD
• Read and analyze each changed file
• Check cross-file consistency
• Analyze "what's missing"
• Format and post review

Starting review of PR #58: Run judge criteria concurrently instead of serializing them...

[uipreliga]

Review: coder_eval — pr:58

Scope: pr:58 · branch akshaya/async_criteria · c34b030 · 2026-07-28T08:13Z · workflow variant

Change class: complex — introduces a parallel async criterion-checking path (new check_async/_check_impl_async seam, asyncio.gather over native-async judges, new async Anthropic/Bedrock/sub-agent I/O clients) and changes the orchestrator's three evaluation call sites; correctness requires concurrency and error-propagation reasoning.

The codebase is healthy — clean typing, near-perfect test health, no security findings, and a well-factored plugin/merge architecture — but the new judge-concurrency seam is where that quality breaks down: one asyncio.gather at src/coder_eval/evaluation/checker.py:225 overlaps sandbox-mutating sync criteria with judges reading/snapshotting the same directory and abandons in-flight judge subprocesses on infrastructure errors, so identical agent output can now score differently, while every newly-live async judge path (including the Bedrock nightly route) ships at 0% coverage behind ~300 lines of hand-copied sync/async twins; fix that one seam and cover the async half and this is a 9.5+ change.

Summary

Axis	Score	🔴	🟠	🟡	🔵	Top Issue
1. Code Quality & Style	8.4 / 10	0	1	1	1	Judge stack forked into hand-maintained sync/async twins (~280 near-verbatim lines across 6 sites), with the sync halves now unreachable from production
2. Type Safety	9.8 / 10	0	0	0	2	New _build_result helper takes 8 same-typed positional parameters, so an argument transposition on the judge scoring path type-checks silently
3. Test Health	9.9 / 10	0	0	0	1	Concurrency tests gate on wall-clock with a 100 ms absolute margin under -n auto
4. Security	10 / 10	0	0	0	0	—
5. Architecture & Design	9.4 / 10	0	0	1	1	Native-async judge paths call the blocking sync _prepare (file reads + reference-dir rglob) directly on the event loop
6. Error Handling & Resilience	8.9 / 10	0	1	0	1	asyncio.gather without return_exceptions/cancellation at checker.py:225 abandons in-flight sibling criteria on JudgeInfrastructureError, orphaning an evaluator-credentialed judge subprocess and sandbox copy into orchestrator teardown
7. API Surface & Maintainability	9.5 / 10	0	0	1	0	New async criterion-authoring seam (_check_impl_async / supports_native_async) and the new same-instance concurrent-reentry contract are absent from docs/EXTENDING.md § 2 and CLAUDE.md, whose Evaluation Flow still names the now-unused check_all()
8. Evaluation Harness Quality	8 / 10	0	2	0	0	check_all_async overlaps the sandbox-mutating sync criterion batch with judge criteria reading/copying the same sandbox dir, so what a judge grades depends on interleaving

Overall Score: 9.2 / 10 · Weakest Axis: Evaluation Harness Quality at 8 / 10
Totals: 🔴 0 · 🟠 4 · 🟡 3 · 🔵 6 across 8 axes.

Blockers

1. [Axis 1] Judge stack forked into hand-maintained sync/async twins (~280 near-verbatim lines across 6 sites), with the sync halves now unreachable from production (src/coder_eval/evaluation/checker.py:336) — _check_single_async (checker.py:336-405) is a line-for-line copy of _check_single (checker.py:251-335); diff of the two spans shows the only logic difference is result = checker.check( → result = await checker.check_async( (checker.py:276 vs :352) — the ~50 lines of threshold assignment, logger.info("Criterion '%s' %s (score=%.2f, threshold=%.2f): %s", ...), the except KeyError block, except JudgeInfrastructureError: raise, and the except Exception block are identical (the copy also strips every explanatory comment, e.g. # V3: Catch ALL exceptions, including checker __init__ failures). The same copy-paste is repeated at four more sites in this PR: criteria/base.py:102-152 (handle_criterion_errors_async vs :49-99, differing only in await func( and the .check()→.check_async() log string); evaluation/sub_agent.py:207-271 (run_async vs run at :117-205 — and the copy silently drops the load-bearing security rationale comments, including "Deliberately NO dirs_exist_ok=True here" and the symlink/_reference defense-in-depth notes, so a future editor of run_async has no record of why those flags are set); evaluation/judge_bedrock.py:118-181 (whole request body + retry loop + response validation duplicated from :67-115, differing only in await client.post and await asyncio.sleep); evaluation/judge_anthropic.py:58-90 vs :21-55. Compounding it: after this PR nothing in src/ calls the sync entry points any more — grep -rn 'success_checker\.\|checker\.check' src/coder_eval/ returns only the three check_all_async call sites in orchestrator.py:1349/1407/1499, so SuccessChecker.check_all, LLMJudgeChecker._check_impl, AgentJudgeChecker._check_impl, SubAgentRunner.run, invoke_bedrock_judge and invoke_anthropic_judge are production-dead, while grep -rn 'invoke_bedrock_judge_async\|invoke_anthropic_judge_async\|run_async' tests/ returns nothing — i.e. the duplicated half that still has all the tests is the half that no longer runs. Fix: keep one implementation per concern — implement the sync entry points as thin bridges over the async ones (def run(...): return asyncio.run(self.run_async(...)) in sub_agent.py; likewise for the two invoke_*_judge functions and the judges' _check_impl), and extract the shared result-finalization/error-mapping into helpers used by both _check_single/_check_single_async and both decorators.
2. [Axis 6] asyncio.gather without return_exceptions/cancellation at checker.py:225 abandons in-flight sibling criteria on JudgeInfrastructureError, orphaning an evaluator-credentialed judge subprocess and sandbox copy into orchestrator teardown (src/coder_eval/evaluation/checker.py:225) — await asyncio.gather(run_sync_batch(), *(run_async_one(i) for i in native_async_indices)) (checker.py:225) has no return_exceptions=True and is not a structured-concurrency scope. _check_single_async deliberately re-raises JudgeInfrastructureError (checker.py:385-386), and invoke_bedrock_judge_async raises it on any non-retryable HTTP status (judge_bedrock.py:173: raise JudgeInfrastructureError(f"Bedrock invoke failed: {response.status_code} ...") — i.e. an expired/invalid bearer token → 401/403) or on retry exhaustion (judge_bedrock.py:181). Per asyncio semantics the exception propagates immediately to the awaiter while the other children keep running: I confirmed this empirically (gather raised infra down printed before the sibling's finally ran 0.5s later). So on a task with llm_judge + agent_judge, a fast llm_judge 403 abandons the in-flight agent_judge: its SubAgentRunner.run_async task is not a child of the orchestrator task, so even the task-timeout watchdog's loop.call_soon_threadsafe(self._task_to_cancel.cancel) (agents/watchdog.py:103) does not reach it. It keeps a live Claude Code CLI subprocess and a full sub_agent_* copy of the sandbox in tmp for up to turn_timeout (default 300s, models/criteria.py:927-930), burning Sonnet tokens that are never attributed to any task, and — because run_batch shares ONE event loop across tasks gated by asyncio.Semaphore(config.max_parallel) (orchestration/batch.py:81) — it occupies no semaphore slot, so a nightly batch runs more concurrent judge subprocesses than max_parallel. The sync path had no such window: check_all ran criteria serially, so a raise simply meant the later criteria never started. Fix: use asyncio.TaskGroup (cancels and awaits every sibling on first error, so run_async's finally: await asyncio.to_thread(shutil.rmtree, judge_dir, ...) and _run_agent's await agent.kill() actually run; unwrap with except* JudgeInfrastructureError), or gather(..., return_exceptions=True) and re-raise the first JudgeInfrastructureError only after every result has settled. orchestration/batch.py:171 already uses return_exceptions=True as the in-repo precedent. Also add a test: the new test_judge_infrastructure_error_propagates (tests/test_check_all_async.py:160-163) uses a single criterion, so it cannot detect an orphaned sibling — assert that a second, slow native-async criterion is cancelled/awaited when the first raises.
3. [Axis 8] check_all_async overlaps the sandbox-mutating sync criterion batch with judge criteria reading/copying the same sandbox dir, so what a judge grades depends on interleaving (src/coder_eval/evaluation/checker.py:225) — await asyncio.gather(run_sync_batch(), *(run_async_one(i) for i in native_async_indices)) (checker.py:225) runs the sync batch concurrently with the judge batch against ONE shared sandbox directory. The sync batch is not read-only: RunCommandChecker._check_impl does sandbox.run_command(criterion.command, timeout=criterion.timeout) (criteria/run_command.py:68) which is subprocess.run(command, shell=True, cwd=self.sandbox_dir, ...) (sandbox.py:933-943), and UiPathEvalChecker does sandbox.run_command(cmd) writing uipath_eval_output_<hex>.json into the sandbox (criteria/uipath_eval.py:67-73). Concurrently, agent_judge snapshots that same directory — await asyncio.to_thread(shutil.copytree, src_dir, judge_dir, symlinks=True, ignore=..., dirs_exist_ok=True) (sub_agent.py:223-230) — and llm_judge reads files out of it via _collect_sandbox_file → sandbox.file_exists(path) / sandbox.get_file_content(path) (judge_context.py:289-302). Two concrete failures on a task that mixes run_command/uipath_eval with a judge (the pattern docs/TASK_DEFINITION_GUIDE.md:930 explicitly recommends: "consider running it alongside cheaper structural checks rather than as the sole gate"): (a) copytree walks a tree the sibling command is writing — a transient __pycache__/.pytest_cache/uv temp file that vanishes between os.scandir and copy2 makes copytree raise FileNotFoundError, which handle_criterion_errors_async (base.py:130-148) converts to score=0.0, flipping a gating judge and the task's final_status to FAILURE for identical agent output; (b) with no crash, the judge's copy either contains or omits uipath_eval_output_*.json / build artifacts depending on interleaving, and llm_judge intermittently records a graded file in ctx.missing_files (judge_context.py:291) — so the LLM verdict itself varies run to run. Before this PR check_all was strictly serial in YAML order (_check_all_sync, checker.py:236-237), making the ordering deterministic; sub_agent.py:128-130 even states the invariant this breaks in reverse: "The sub-agent never touches the original sandbox, so later criteria are unaffected by whatever it does." Fix: do not overlap the sync batch with the judge batch — await run_sync_batch() first, then await asyncio.gather(*(run_async_one(i) ...)) — or gate the overlap on the sync set containing no sandbox-mutating criterion type (run_command, uipath_eval). Judges may still run concurrently with each other, since each takes its own mkdtemp copy.
4. [Axis 8] Every newly added async path (llm_judge/agent_judge _check_impl_async, invoke_anthropic/bedrock_judge_async, SubAgentRunner.run_async, base to_thread seam, check_all_async forwarding) has zero test coverage while the suites drive the now-dead sync twins (src/coder_eval/evaluation/sub_agent.py:207) — All three orchestrator call sites now use check_all_async (orchestrator.py:1349, 1407, 1499) and both judges declare supports_native_async = True (llm_judge.py:62, agent_judge.py:102), so SuccessChecker.check_all, LLMJudgeChecker._check_impl, AgentJudgeChecker._check_impl, SubAgentRunner.run (sub_agent.py:117), invoke_bedrock_judge and invoke_anthropic_judge have no remaining caller in src/ (verified by grep: the only runner.run( / invoke_bedrock_judge( / invoke_anthropic_judge( hits outside their own definitions are in tests/). Meanwhile every existing test drives exactly those dead entry points: all 18 tests in tests/test_sub_agent_runner.py call runner.run("grade", ...) (lines 81, 126, 177, 210, 226, 248, 274, 295, 320, 402, 432, 458, 490, 542, 581), including the security-floor tests test_runner_copytree_drops_nested_symlinks (line 409), test_runner_excludes_nested_claude_and_mcp (line 464) and test_runner_handles_sandbox_side_reference_collision (line 134); and tests/test_llm_judge_criterion.py:103,413 / tests/test_agent_judge_criterion.py:418 use checker.check() / checker.check_all(). The live twin async def run_async (sub_agent.py:207) is a full hand-copy of run() and is entirely uncovered (coverage: sub_agent.py missing 218-270), as are invoke_bedrock_judge_async (judge_bedrock.py missing 136-181 — the nightly's --backend bedrock route), invoke_anthropic_judge_async (missing 75-90) and _invoke_tool_channel_async (llm_judge.py missing 285-316). I diffed the twins and found no behavioural drift today, but any future hardening applied to one side (e.g. tightening the _reference clear at sub_agent.py:232-243) will silently change nightly scores with a green suite. Fix: parametrize tests/test_sub_agent_runner.py, tests/test_llm_judge_criterion.py and tests/test_agent_judge_criterion.py over sync/async (@pytest.mark.parametrize("mode", ["sync", "async"]) dispatching run/run_async and check/check_async), or delete the now-dead sync twins so there is a single implementation.

Non-blocking, but please consider before merge

1. [Axis 1] supports_native_async is a second source of truth for information derivable from the _check_impl_async override, with nothing guarding the pair (src/coder_eval/criteria/base.py:197) — base.py:197 adds supports_native_async: ClassVar[bool] = False, and the only two subclasses that set it (llm_judge.py:62, agent_judge.py:102) are exactly the two that override _check_impl_async (llm_judge.py:88, agent_judge.py:137). The flag is therefore fully derivable: type(checker)._check_impl_async is not BaseCriterion._check_impl_async. Because it is a separate declaration, it can drift, and the drift is silent in the dangerous direction: a criterion (in-tree or a coder_eval.plugins third party) that overrides _check_impl_async with a real async client but forgets the flag is routed by checker.py:209 (native_async_indices = [i for i, c in enumerate(criteria) if _is_native_async(c.type)]) into run_sync_batch → _check_all_sync → _check_single → checker.check(...) (checker.py:276), i.e. its blocking _check_impl twin runs and the async implementation is never executed — no error, just the concurrency win quietly lost. This repo already rejects this shape twice over: criteria/__init__.py::validate_registry's docstring ("the previous fallback had already rotted ... proving a second source of truth cannot be trusted"), and lint rule tests/lint/rules/ce025_live_verdict_consistency.py, which exists solely to keep the identical ClassVar↔override pair (live_stop_polarities / live_verdict) consistent. Fix: drop the ClassVar and derive it in _is_native_async (checker.py:201-207) from the override identity; if it is kept for readability, add a CE0nn rule mirroring CE025 that fails when supports_native_async = True appears without a _check_impl_async override and vice versa.
2. [Axis 5] Native-async judge paths call the blocking sync _prepare (file reads + reference-dir rglob) directly on the event loop (src/coder_eval/criteria/agent_judge.py:146) — setup = _prepare(criterion, sandbox, reference_code, turn_records, context) (agent_judge.py:146, and llm_judge.py:97) is called with no to_thread, yet _prepare runs JudgeContextBuilder(...).build(...) — text = path.read_text(encoding="utf-8") (judge_context.py:191), content = host_path.read_text(encoding="utf-8") (judge_context.py:282) — and collect_reference_secrets(reference_dir) (agent_judge.py:296), which rglob-reads up to _MAX_REFERENCE_FILES = 200 / _MAX_REFERENCE_BYTES = 2 * 1024 * 1024 (judge_context.py:124-125). Since run_batch runs all tasks on one loop (await asyncio.gather(*coroutines, return_exceptions=True), orchestration/batch.py:171), this stalls sibling tasks' streaming and watchdogs — a regression versus the old all-in-to_thread design. Wrap with await asyncio.to_thread(_prepare, ...). CE002 (tests/lint/rules/no_blocking_io_in_async.py) misses it because the reads are one frame away in a sync helper — consider extending the rule.
3. [Axis 7] New async criterion-authoring seam (_check_impl_async / supports_native_async) and the new same-instance concurrent-reentry contract are absent from docs/EXTENDING.md § 2 and CLAUDE.md, whose Evaluation Flow still names the now-unused check_all() (src/coder_eval/criteria/base.py:289) — check_async (base.py:267), _check_impl_async (base.py:289) and supports_native_async (base.py:197) extend BaseCriterion, which docs/EXTENDING.md § "2. Custom success criteria" advertises as the third-party extension point — but the PR changes no docs (git diff origin/main...pr-58 --name-only -- docs CLAUDE.md returns 0 files). Three surfaces are now stale/incomplete: (1) docs/EXTENDING.md's "Notes:" list (line 175) still tells authors only "- Do not override check() — it's final and wraps _check_impl with error handling" (line 177) and documents the early-stop pair at line 186, with nothing about the async seam — an author writing a network-bound criterion has no way to know _check_impl_async + supports_native_async exist; (2) CLAUDE.md's "Adding a New Criterion" 4-step list (CLAUDE.md:221-224) likewise omits it; (3) CLAUDE.md's Evaluation Flow now misstates the live path — line 185 reads "2. SuccessChecker.check_all() → List[CriterionResult]" while all three orchestrator call sites (orchestrator.py:1349/1407/1499) call check_all_async. Separately, the change silently adds a NEW contract obligation that no doc states: _get_checker_instance returns one cached instance per type (checker.py:245-249, "Checker instance (reused within this evaluation run)"), and check_all_async now fires run_async_one(i) for every native-async index in one asyncio.gather (checker.py:225), so two criteria of the same type re-enter the SAME checker instance concurrently — verified by the new test asserting fake.calls == 2 on a single instance (tests/test_check_all_async.py:104-113). Previously a checker instance was only ever entered serially; an out-of-tree checker that stashes per-call state on self (no in-tree checker does — grep "self.[a-z_]* *=" src/coder_eval/criteria/*.py outside base.py returns nothing) would now corrupt one criterion's result with another's. Add to docs/EXTENDING.md's notes list: the async seam (when to override _check_impl_async and set supports_native_async), and an explicit "checker instances are cached per type and may be entered concurrently — keep them stateless/reentrant" line; fix CLAUDE.md:185 and CLAUDE.md:221-224. Note no lint rule guards docs/EXTENDING.md (CE030 covers only the YAML-facing models TaskDefinition/RunLimits/Dataset/SimulationConfig per tests/lint/doc_schema_parity.py:44-45, and CE028 covers only the generated docs-index tables), so this class of drift is currently unguarded.

Nits

1. [Axis 1] check_all_async index bookkeeping: O(n²) partition, None-sentinel result list requiring a type: ignore (src/coder_eval/evaluation/checker.py:210) — checker.py:209-212 builds the partition with a list-membership scan and a nullable result buffer: sync_indices = [i for i in range(len(criteria)) if i not in native_async_indices] (quadratic in the criteria count) followed by results: list[CriterionResult | None] = [None] * len(criteria), which then forces return results # type: ignore[return-value] at :226. A single-pass partition (for i, c in enumerate(criteria): (async_indices if _is_native_async(c.type) else sync_indices).append(i)) removes the membership scan, and gathering the two groups into separate typed lists and merging them removes both the None sentinel and the type: ignore. Noting explicitly: radon's C(13) grade on this function is not by itself a defect — the function reads clearly and the "sync criteria share one to_thread slot" design (which is what forces the index bookkeeping) is a deliberate, correct choice, since concurrent run_command criteria would race in the shared sandbox.
2. [Axis 2] New _build_result helper takes 8 same-typed positional parameters, so an argument transposition on the judge scoring path type-checks silently (src/coder_eval/criteria/llm_judge.py:175) — _build_result (llm_judge.py:175-184) declares (criterion, judge_ctx, user_msg: str, scrub_key: str | None, verdict, parse_error: str | None, raw_verdict_text: str, judge_usage) — four adjacent slots that are all str / str | None — and both new call sites pass all eight positionally (llm_judge.py:84-86 and llm_judge.py:108-110, identical arg lists). Nothing in the type system distinguishes user_msg from raw_verdict_text or scrub_key from parse_error. Add a * after criterion so the risky arguments are keyword-only, matching this module's own convention (_invoke_tool_channel at llm_judge.py:225-231 and _invoke_tool_channel_async at llm_judge.py:276-282 are both *-only) and the sibling judge (agent_judge.py:341-350, which puts * before scrub_secrets/system_prompt/user_msg/capture and is called with keywords at agent_judge.py:127-135 and 160-168). Candidate CE lint rule: flag a new module-private helper with >4 positional-or-keyword parameters where two or more share an identical annotation.
3. [Axis 2] handle_criterion_errors_async erases the new check_async seam's signature to Callable[..., Awaitable[CriterionResult]], so its call site is unchecked and the "FINAL" contract is unenforceable (src/coder_eval/criteria/base.py:102) — base.py:102-104 types the new decorator as def handle_criterion_errors_async(func: Callable[..., Awaitable[CriterionResult]]) -> Callable[..., Awaitable[CriterionResult]]:. Because it is applied at base.py:266 to async def check_async (base.py:267-274), the generic C-bound criterion parameter and the whole parameter list collapse to ..., so the only production call site — await checker.check_async(criterion, self.sandbox, reference_code, turn_records=turn_records, context=context) at src/coder_eval/evaluation/checker.py:346-352 — gets no arity or keyword-name checking (a typo'd turn_record= would pass pyright and land in **-less code as a TypeError only at runtime). The docstring at base.py:275 says "This method is FINAL - subclasses must NOT override it", but the erased callable type also means reportIncompatibleMethodOverride (enabled as warning in pyproject.toml:254) can never fire on a subclass that overrides check_async with a wrong signature. Preserve the signature with ParamSpec + Concatenate (def handle_criterion_errors_async[**P](func: Callable[P, Awaitable[CriterionResult]]) -> Callable[P, Awaitable[CriterionResult]], with the wrapper taking *args: P.args, **kwargs: P.kwargs and reading criterion from args[0]), and add @typing.final to check_async (and, for consistency, to the pre-existing check at base.py:200) so the FINAL contract is machine-checked rather than documented. Note this same erasure exists on the pre-existing sync handle_criterion_errors (base.py:49) — the fix should cover both twins together.
4. [Axis 3] Concurrency tests gate on wall-clock with a 100 ms absolute margin under -n auto (tests/test_check_all_async.py:113) — assert elapsed < SLEEP_SECONDS * 1.5, f"judge criteria serialized: took {elapsed:.3f}s" (line 113, and the same at line 129) allows only 0.1 s of slack over the 0.2 s SLEEP_SECONDS, while pyproject.toml:286 sets addopts = ["-n", "auto", ...] so these run alongside 3600+ other tests on a contended CI box; line 129's variant additionally depends on asyncio.to_thread acquiring a default-executor worker. I could not make them fail locally (7/7 passed on 3 consecutive runs, and 3/3 with 16 busy-spin processes competing), so this is a latent flake rather than an observed one. Prefer an event-based proof of overlap (e.g. each fake awaits a shared asyncio.Barrier/Event that only resolves if both are entered concurrently, with a generous timeout) over a duration comparison.
5. [Axis 5] Stale comments/docstrings still assert the removed check_all-in-a-thread / to_thread execution model (src/coder_eval/orchestrator.py:1486) — orchestrator.py:1486 still says "run check_all off the event loop" while the body awaits check_all_async at :1499; sub_agent.py:185 still justifies its bare asyncio.run(...) with "Safe because callers run check_all via asyncio.to_thread, so this invocation is on a worker thread with no active event loop" (no production caller does that any more); judge_bedrock.py:84 still says "invoke_bedrock_judge is sync (called via asyncio.to_thread), so a plain time.sleep between attempts is correct here". Update all three, or delete them along with the dead twins.
6. [Axis 6] Dead except KeyError arm duplicated into _check_single_async (and the new test module claims coverage of it) (src/coder_eval/evaluation/checker.py:374) — _check_single_async's except KeyError: → "No checker registered for criterion type ..." (checker.py:374-384) is unreachable. _check_single_async runs only for indices in native_async_indices, and membership in that list required _is_native_async → _get_checker_instance(criterion_type) to have already succeeded and cached the instance (checker.py:203, 246-249); the cache hit at checker.py:350 therefore cannot raise KeyError, and any KeyError raised inside the checker is already absorbed by handle_criterion_errors_async (criteria/base.py:126) before it can reach this handler. The arm is dead defensive code that also mislabels the cause if it ever did fire. tests/test_check_all_async.py:10-11 nonetheless states the async path must "preserve the sync path's error-handling contract (KeyError / generic Exception captured into a failed CriterionResult)" while TestNativeAsyncErrorHandling (lines 150-167) contains no KeyError/unregistered-type case at all. Fix: delete the unreachable arm (unregistered types are already routed to the sync batch by _is_native_async returning False) and drop the KeyError claim from the test docstring, or add the test that actually exercises an unregistered type end-to-end through check_all_async.

What's Missing

Parallel paths:

• 🟠 🟠 The sync half of the judge stack was duplicated rather than replaced: SuccessChecker.check_all/_check_single, handle_criterion_errors, LLMJudgeChecker._check_impl, AgentJudgeChecker._check_impl, _invoke_tool_channel, invoke_bedrock_judge, invoke_anthropic_judge and SubAgentRunner.run all still exist as hand-maintained twins (~320 near-verbatim lines across 6 sites), and the judge-specific ones now have no production caller — every future hardening must be applied twice, with only one side under test. Triggered by src/coder_eval/evaluation/checker.py:336, criteria/base.py:102, evaluation/sub_agent.py:207, judge_bedrock.py:118, judge_anthropic.py:58, criteria/llm_judge.py:276. (trigger: src/coder_eval/evaluation/checker.py) (restates: Axis 1: Judge stack forked into hand-maintained sync/async twins)
• 🟡 🟡 The second class of blocking-wait criterion was not given the new seam and is actively mislabelled: check_all_async's docstring (checker.py:176-178) calls the sync batch "file/command/trajectory checks — CPU/file-bound, no I/O wait", but uipath_eval shells out to uv run uipath eval with no timeout (criteria/uipath_eval.py:73) and run_command runs arbitrary shell (criteria/run_command.py:68) — both are network/subprocess-bound, still pin the single shared to_thread slot, and still serialize with each other. Either apply _check_impl_async (asyncio.create_subprocess_shell) there too, or correct the docstring to say the sync batch is deliberately serialized because it mutates the shared sandbox. (trigger: src/coder_eval/evaluation/checker.py)
• 🟡 🟡 The registry-conformance guards that exist precisely for this shape were not extended: criteria/__init__.py::validate_registry still validates only _check_impl/union membership, and no CEnnn rule was added alongside tests/lint/rules/ce025_live_verdict_consistency.py (which guards the structurally identical live_stop_polarities↔live_verdict ClassVar/override pair). Triggered by the new supports_native_async ClassVar at src/coder_eval/criteria/base.py:197. (trigger: src/coder_eval/criteria/base.py) (restates: Axis 1: supports_native_async is a second source of truth)
• 🟡 🟡 CE002 (tests/lint/rules/no_blocking_io_in_async.py) was not extended even though the PR introduces exactly the pattern it cannot see — blocking read_text/rglob reached from an async def via a sync helper one frame away (_prepare at agent_judge.py:146 / llm_judge.py:97). Its own docstring invites extending the allowlist "when a new blocking pattern is found in code review"; a whole new async surface landed with the rule untouched. (trigger: src/coder_eval/criteria/agent_judge.py) (restates: Axis 5: Native-async judge paths call the blocking sync _prepare on the event loop)
• 🟡 🟡 The comments documenting the old execution model were not updated on the other side of the move: orchestrator.py:1486 ("run check_all off the event loop"), sub_agent.py:185 ("Safe because callers run check_all via asyncio.to_thread" — the justification for a bare asyncio.run() that no production caller now satisfies), judge_bedrock.py:84 ("called via asyncio.to_thread, so a plain time.sleep ... is correct"), and reports.py:767 ("check_all appends one CriterionResult per criterion"). (trigger: src/coder_eval/orchestrator.py) (restates: Axis 5: Stale comments/docstrings still assert the removed to_thread execution model)
• 🔵 🔵 SuccessChecker.check (the singular-criterion public wrapper, checker.py:103) got no check_async twin, so BaseCriterion.check_async is reachable in production only through check_all_async; any single-criterion consumer (tests, an out-of-tree re-scoring script, coder_eval_uipath) still runs an llm_judge through the fully blocking _check_impl. Either add the twin or state that check() is deliberately sync-only. (trigger: src/coder_eval/evaluation/checker.py)

Tests:

• 🟠 🟠 Zero tests exercise the async code production now takes: SubAgentRunner.run_async (missing 218-270), invoke_bedrock_judge_async (missing 136-181), invoke_anthropic_judge_async (missing 75-90), _invoke_tool_channel_async (llm_judge missing 285-316), both _check_impl_asyncs, and the base to_thread default (base.py:309). No test references run_async, AsyncAnthropic or httpx.AsyncClient; the existing judge suites all drive the now-unreachable sync twins. (trigger: src/coder_eval/evaluation/sub_agent.py) (restates: Axis 8: Every newly added async path has zero test coverage)
• 🟠 🟠 No test covers cancellation of an in-flight judge — a shape the PR newly makes reachable. Previously the judge ran inside a non-cancellable to_thread, so SubAgentRunner.run's synchronous finally: shutil.rmtree(...) always ran; run_async's cleanup is itself await asyncio.to_thread(shutil.rmtree, judge_dir, ...) (sub_agent.py:270) and _run_agent's teardown also awaits, so a task-timeout/watchdog cancel (or the gather's first-error path) can skip cleanup and leave a sub_agent_* sandbox copy plus a live Claude Code CLI subprocess behind. Add a test that cancels a check_all_async containing an agent_judge and asserts the temp dir is gone and the agent was killed. (trigger: src/coder_eval/evaluation/sub_agent.py)
• 🟠 🟠 test_judge_infrastructure_error_propagates (tests/test_check_all_async.py:160-163) uses a single criterion, so nothing asserts what happens to siblings when one native-async criterion escalates — the exact regression the bare gather at checker.py:225 introduces. Needs a two-criterion test asserting the slow sibling is cancelled and awaited (cleanup ran) before the error surfaces. (trigger: tests/test_check_all_async.py) (restates: Axis 6: asyncio.gather without return_exceptions abandons in-flight sibling criteria)
• 🟡 🟡 The only sync/async parity test (test_all_sync_criteria_still_produce_same_results_as_check_all, tests/test_check_all_async.py:172-177) compares a single file_exists criterion — i.e. it never compares the twins that were actually duplicated. Parametrize tests/test_sub_agent_runner.py, test_llm_judge_criterion.py, test_agent_judge_criterion.py, test_judge_bedrock.py and test_judge_anthropic.py over run/run_async and check/check_async so drift between the twins fails the suite. (trigger: tests/test_check_all_async.py) (restates: Axis 1: Judge stack forked into hand-maintained sync/async twins)
• 🟡 🟡 The one integration test that mixes a judge with a non-judge criterion (tests/test_llm_judge_criterion.py:413, checker.check_all([fe, judge])) still drives the serial sync route, so the interleaved production path — and the sandbox-overlap hazard it introduces — is untested end-to-end. Same for tests/test_error_handling.py:739 (test_check_all_propagates_judge_infrastructure_error), which only proves the sync escalation. (trigger: src/coder_eval/evaluation/checker.py) (restates: Axis 8: check_all_async overlaps the sandbox-mutating sync batch with judge criteria)
• 🔵 🔵 The two concurrency proofs assert elapsed < SLEEP_SECONDS * 1.5 (lines 113, 129) under -n auto; replace the duration comparison with an event/barrier-based overlap proof so the guarantee is asserted structurally rather than by timing. (trigger: tests/test_check_all_async.py) (restates: Axis 3: Concurrency tests gate on wall-clock with a 100 ms absolute margin)
• 🔵 🔵 The new module's docstring claims the async path preserves "KeyError / generic Exception captured into a failed CriterionResult" (tests/test_check_all_async.py:10-11) but TestNativeAsyncErrorHandling has no unregistered-type case — either add a test routing an unregistered type through check_all_async (it goes to the sync batch via _is_native_async returning False) or drop the claim and the unreachable arm at checker.py:374. (trigger: src/coder_eval/evaluation/checker.py) (restates: Axis 6: Dead except KeyError arm duplicated into _check_single_async)

Downstream consumers:

• 🟡 🟡 Judge cost/token attribution is a downstream consumer of the new failure mode and wasn't revisited: _accumulate_judge_usage (orchestrator.py:1277, called at :1505) folds usage only from the returned results, so a judge abandoned by the gather raise (or cancelled mid-flight) is billed by the provider — including a full Sonnet sub-agent run — while contributing nothing to judge_usage_accum, the task's max_usd budget gate, or the run's reported cost. (trigger: src/coder_eval/evaluation/checker.py) (restates: Axis 6: asyncio.gather without return_exceptions abandons in-flight sibling criteria)
• 🟠 🟠 Consumers that relied on declared-order execution (not just result order) were not audited: check_all_async guarantees result ordering but no longer sequencing, so a judge listed after a run_command/uipath_eval criterion no longer reliably sees that command's artifact (uipath_eval_output_*.json, a test/coverage report) — a pattern docs/TASK_DEFINITION_GUIDE.md:929 explicitly recommends. Either restore ordering between the sync and judge batches or document the loss of execution ordering in the task guide. (trigger: src/coder_eval/evaluation/checker.py) (restates: Axis 8: check_all_async overlaps the sandbox-mutating sync batch with judge criteria)

Display & mapping dicts:

• 🔵 🔵 "Which criterion types are judges" is already enumerated twice by hand — _JUDGE_CRITERION_TYPES = frozenset({"llm_judge", "agent_judge"}) (models/results.py:228, with a convention comment requiring same-PR updates) and the literal tuple at reports_html.py:430 that gates judge cards — and this PR adds a third de-facto enumeration (supports_native_async) without unifying them. A future native-async judge (in-tree or plugin) would get concurrency but fall through to result_kind="basic" and render no judge card. (trigger: src/coder_eval/criteria/base.py) (restates: Axis 1: supports_native_async is a second source of truth)

Daily/nightly:

• 🟠 🟠 Unstated blast radius: judge fan-out is now unbounded. The only concurrency limiter in the repo is the task-level asyncio.Semaphore(config.max_parallel) (orchestration/batch.py:81), so peak concurrent Bedrock judge calls become max_parallel × judges-per-task, and concurrent agent_judge runs each hold their own Claude Code CLI subprocess plus a full sub_agent_* copy of the sandbox. On the nightly this raises 429-throttle exhaustion (→ JudgeInfrastructureError → row FinalStatus.ERROR) and, under driver: docker, pushes against max_pids/--pids-limit, max_memory_mb and temp-disk headroom. The PR says nothing about a judge-level cap or the nightly's expected concurrency. (trigger: src/coder_eval/evaluation/checker.py)
• 🟠 🟠 The nightly's judge route flips to untested code with no statement in the PR: --backend bedrock now resolves to invoke_bedrock_judge_async (judge_bedrock.py:118, 0% covered), and the in-container path (cli/run_task_internal_command.py:190 → Orchestrator → check_all_async) inherits it as well. State the nightly impact explicitly, and note that no persisted-schema/wire change is involved so no lockstep image rebuild is required (that reassurance is currently absent). (trigger: src/coder_eval/evaluation/judge_bedrock.py) (restates: Axis 8: Every newly added async path has zero test coverage)
• 🟡 🟡 Timing/latency semantics change for the external coder-eval-uipath / eval-runner dashboards without a note: judge waits now overlap each other and the sync batch, so EvaluationResult.duration_seconds (models/results.py:462) and every trend built on it step-change at this commit, and per-criterion JudgeCriterionResult.duration_seconds windows now overlap rather than sum to the evaluation phase. Any consumer inferring serialized criterion timing from task.json needs to be told. (trigger: src/coder_eval/orchestrator.py)
• 🟡 🟡 The sandbox-overlap race is assessed only against in-repo tasks (none of which mix a judge with run_command/uipath_eval); the nightly suites and tasks live in the external repo, so the PR cannot claim "no impact" without checking there — a nightly task pairing a judge with a build/test command would start scoring nondeterministically with a green CI here. (trigger: src/coder_eval/evaluation/checker.py) (restates: Axis 8: check_all_async overlaps the sandbox-mutating sync batch with judge criteria)

Harness & Lint Improvements

Abridged to fit GitHub's 65,536-character comment limit — the full proposals (rule bodies, exact wiring, measured hit lists) are in tmp/code-review-260728-0113/00-summary.md.

Static checks (lint / type):

• [ce-lint] CE026 — capability ClassVar <-> override parity (generalize CE025). New rule tests/lint/rules/ce026_capability_flag_override_parity.py, wired into ALL_RULES in tests/lint/runner.py (CE026 is the only free number below the doc-surface block CE027-CE031). …
• [ce-lint] CE032 — asyncio.gather must be exception-safe. New per-file rule tests/lint/rules/ce032_gather_return_exceptions.py in ALL_RULES. Forbidden pattern: a call to asyncio.gather(...) inside src/coder_eval/ that passes neither return_exceptions=True nor ** kwargs, unless the awaiting frame is an asyncio.TaskGroup scope. …
• [ce-lint] CE033 — no hand-maintained sync/async twins. Whole-tree rule (needs a cross-file caller index, so wire it as a dedicated @pytest.mark.lint test class alongside CE027-CE031 in tests/lint/, not as a BaseRule). …
• [ce-lint] CE034 — sandbox-mutating checkers must declare it, and the batcher must read the declaration. New rule in two halves: (a) in src/coder_eval/criteria/, a checker class whose body reaches sandbox.run_command(, sandbox.write_file(, or any subprocess/shutil write must declare mutates_sandbox: ClassVar[bool] = True (current would-be hits: run_command.py:68, uipath_eval.py:66-73); …
• [ce-lint] CE035 — decorators must not erase the wrapped signature. New per-file rule in ALL_RULES: in src/coder_eval/, a function whose return annotation is a Callable[...] and which takes a parameter annotated Callable[..., X] (the decorator shape) is a violation — use PEP 695 [**P] with Callable[P, X] (plus Concatenate when the first parameter is read positionally). …
• [ce-lint] CE036 — no ambiguous positional tail on module-private helpers. New per-file rule in ALL_RULES: in src/coder_eval/, a module-private (_-prefixed) function or method with >=5 positional-or-keyword parameters where two or more share an identical annotation must move the ambiguous parameters after * (keyword-only). …
• [ce-lint] CE037 — criterion-authoring seam <-> docs/EXTENDING.md parity. Extend the CE030 machinery (tests/lint/doc_schema_parity.py) from YAML-facing models to the advertised extension seam: add a DOCUMENTED_SEAMS: list[tuple[type, str]] = [(BaseCriterion, "docs/EXTENDING.md")] table asserting that every subclass-facing member of BaseCriterion — _check_impl, _check_impl_async, supports_native_async, …
• [ce-lint] CE038 — no wall-clock duration assertions in tests/. New per-file rule scoped to tests/: flag an assert whose comparison operand derives from a time.perf_counter() / time.monotonic() / time.time() subtraction compared against a literal or literal-scaled bound. …
• [ce-lint] Extend CE002 (tests/lint/rules/no_blocking_io_in_async.py) one frame. Its docstring already invites extension "when a new blocking pattern is found in code review". Add a same-module call-graph pass: build the set of module-level sync defs whose bodies (transitively, within the file) hit _BLOCKING_BUILTINS/_BLOCKING_ATTRS/_BLOCKING_METHODS or rglob/glob/iterdir/stat, then flag a direct …
• [ruff] Enable the ASYNC rule family. In pyproject.toml [tool.ruff.lint], add "ASYNC" to select and ignore = ["ASYNC109"] (measured on the PR HEAD: the only pre-existing src/ hits are 6 x ASYNC109 on the intentional timeout: parameter of the Agent.communicate contract — agent.py:192, agents/*_agent.py — plus a single ASYNC240 at orchestrator.py:1728, fixable or noqa-able in the same commit). …
• [pyright] Flip reportIncompatibleMethodOverride from "warning" to "error" (pyproject.toml:254) and add @typing.final to BaseCriterion.check and check_async. make typecheck runs bare uv run pyright (Makefile:37), which exits non-zero on errors only — so at warning level this setting reports and CI still passes, i.e. it currently gates nothing. …

Harness improvements (not statically reachable):

• Parametrize the judge/sub-agent suites over sync <-> async (a mode fixture). tests/test_sub_agent_runner.py (15 of its 18 tests drive runner.run, including the security floor: test_runner_copytree_drops_nested_symlinks:409, test_runner_excludes_nested_claude_and_mcp:464, test_runner_handles_sandbox_side_reference_collision:134), tests/test_llm_judge_criterion.py (:103, :413), …
• Add a changed-lines coverage gate to CI. Keep the global 80 % floor and add a diff-coverage step (diff-cover coverage.xml --compare-branch=origin/main --fail-under=90, or equivalent) to the verify job, so a PR that adds uncovered production paths fails on its own diff instead of being carried by the existing 3624-test suite. Why not static: Coverage is a runtime measurement of which lines a test run executed; …
• Event-loop responsiveness test for check_all_async. Add tests/test_check_all_async.py::test_loop_stays_responsive_during_judge_prep: register a fake native-async criterion whose _prepare-equivalent does a real time.sleep(0.3) (or reads a large temp tree), run check_all_async alongside a heartbeat coroutine ticking every 10 ms, and assert the heartbeat recorded >= N ticks (event-based, not a …
• Scoring-determinism test over the mixed-criteria shape, plus an in-repo task that exercises it. (a) Build a sandbox where a run_command criterion writes files (a report plus a uipath_eval_output_*.json-shaped artifact) alongside a stubbed judge criterion that snapshots/reads the sandbox; …
• Orphan-resource assertion in teardown, plus a structured-concurrency acceptance test. An autouse session-scoped fixture (or a run_batch post-condition in the integration tests) asserting that no sub_agent_* directories survive under the temp root and that no Claude Code CLI child processes outlive the batch. …
• Add a seam-change item to the PR template / plan checklist: "Did this change add or alter a BaseCriterion / Agent / plugin-SPI seam? If so, the docs/EXTENDING.md section-2 note and CLAUDE.md's extension steps are part of the diff." This is the process backstop for the slice CE037 cannot phrase mechanically — when an author should choose _check_impl_async over _check_impl, and why cached checker instances must …

Top 5 Priority Actions

1. Stop overlapping the sandbox-mutating sync batch with the judge batch at src/coder_eval/evaluation/checker.py:225 — await run_sync_batch() first, then gather the judges (each already takes its own mkdtemp copy or does a pure read) — because today a run_command/uipath_eval criterion writing the sandbox while agent_judge runs shutil.copytree (src/coder_eval/evaluation/sub_agent.py:223) or llm_judge reads files (src/coder_eval/evaluation/judge_context.py:290) can raise shutil.Error → score=0.0, or silently hand the judge a snapshot missing the very artifact it is meant to grade, flipping final_status for identical agent output; the new test_async_batch_overlaps_sync_batch assertion in tests/test_check_all_async.py must be relaxed as part of the fix.
2. Replace the bare asyncio.gather at src/coder_eval/evaluation/checker.py:225 with an asyncio.TaskGroup (or return_exceptions=True plus a deferred re-raise of the first JudgeInfrastructureError) so a fast judge failure — e.g. an expired token → 403 at src/coder_eval/evaluation/judge_bedrock.py:173 — cancels and awaits sibling judges instead of orphaning a credentialed Claude Code subprocess and a full sub_agent_* sandbox copy for up to turn_timeout (300s) outside any max_parallel semaphore slot, following the existing return_exceptions=True precedent at src/coder_eval/orchestration/batch.py:171.
3. Cover the async twins production now actually takes — SubAgentRunner.run_async (src/coder_eval/evaluation/sub_agent.py:207, missing 218-270), invoke_bedrock_judge_async (src/coder_eval/evaluation/judge_bedrock.py:118, missing 136-181), invoke_anthropic_judge_async, _invoke_tool_channel_async, and both judges' _check_impl_async — ideally by parametrizing tests/test_sub_agent_runner.py, tests/test_llm_judge_criterion.py and tests/test_agent_judge_criterion.py over sync/async, since the security-floor tests (symlink stripping, nested .claude/.mcp exclusion, _reference collision, temp-dir cleanup) all currently drive the now-unreachable sync run().
4. Offload the blocking judge setup with await asyncio.to_thread(_prepare, ...) at src/coder_eval/criteria/agent_judge.py:146 and src/coder_eval/criteria/llm_judge.py:97, because _prepare does read_text file collection plus a ≤200-file/≤2 MB reference rglob (src/coder_eval/evaluation/judge_context.py:124) directly on the shared run_batch event loop, stalling every sibling task's streaming and watchdogs — and extend CE002 so it catches blocking IO one sync frame away.
5. Collapse the hand-maintained sync/async duplication instead of growing it: make the sync entry points thin asyncio.run bridges over the async ones and extract shared result-finalization/error-mapping across src/coder_eval/evaluation/checker.py:336 vs :251, src/coder_eval/criteria/base.py:102 vs :49, sub_agent.py:207 vs :117, judge_bedrock.py:118 vs :43, judge_anthropic.py:58 vs :21 and llm_judge.py:276 vs :225 (~300 near-verbatim lines, with load-bearing security rationale comments dropped from the copies); while there, derive supports_native_async from the _check_impl_async override or add a CE0nn rule mirroring CE025 at src/coder_eval/criteria/base.py:197, document the async seam and the new same-instance concurrent-reentry contract in docs/EXTENDING.md:175 and CLAUDE.md:185/221-224, and delete the stale to_thread comments at src/coder_eval/orchestrator.py:1486, sub_agent.py:185 and judge_bedrock.py:84 plus the unreachable except KeyError arm at checker.py:374.

---

Stats: 0 🔴 · 4 🟠 · 3 🟡 · 6 🔵 across 8 axes reviewed.

[akshaylive]

Closing in favor of #60 (keeping async default)