feat: packaged CI gate — JUnit XML output + composite GitHub Action#37
Conversation
|
I'll analyze this and get back to you. |
186fa90 to
f6c02fe
Compare
3360e6a to
3b00991
Compare
uipreliga
left a comment
There was a problem hiding this comment.
Review: coder_eval — pr:37
Scope: pr:37 · branch feat/ci-gate-junit-action · 3b00991 · 2026-07-23T02:44Z · workflow variant
Change class: complex — new JUnit XML report generator (path-containment, FinalStatus classification, best-effort graceful-degrade parsing) plus CLI wiring and a composite GitHub Action with release automation
coder_eval is in excellent health (9.5/10) with clean security, type safety, and architecture across the board; the real risks are concentrated in the new JUnit/CI-action surface — a documented-but-nonexistent env var that silently drops the Bedrock backend, plus several JUnit-detail defects that misinform CI readers without ever changing a task's score or final_status — so the bottom line is a strong PR that needs targeted fixes to its CI-facing edges before merge.
Summary
| Axis | Score | 🔴 | 🟠 | 🟡 | 🔵 | Top Issue |
|---|---|---|---|---|---|---|
| 1. Code Quality & Style | 9.5 / 10 | 0 | 0 | 1 | 0 | Three new reports_junit.py functions land in the CC 10-20 band |
| 2. Type Safety | 9.9 / 10 | 0 | 0 | 0 | 1 | _load_task_json declares dict[str, Any] |
| 3. Test Health | 9.4 / 10 | 0 | 0 | 1 | 1 | JUnit writer's key-coupling to eval_result_to_task_dict is untyped (RunSummary.task_results is list[dict[str, Any]]) and has no parity test — all JUnit tests use the synthetic _row() helper |
| 4. Security | 10 / 10 | 0 | 0 | 0 | 0 | — |
| 5. Architecture & Design | 9.8 / 10 | 0 | 0 | 0 | 2 | Per-task score gate implemented as inline Python in action.yml duplicates the package's scored-row interpretation and is CI-vendor-locked |
| 6. Error Handling & Resilience | 9.5 / 10 | 0 | 0 | 1 | 0 | Root time attribute not guarded against NaN/inf, unlike per-testcase time — emits silently-invalid JUnit XML |
| 7. API Surface & Maintainability | 9 / 10 | 0 | 1 | 0 | 0 | Action/README document a non-existent env var CODER_EVAL_API_BACKEND — the Bedrock-via-action config path is silently ignored |
| 8. Evaluation Harness Quality | 9 / 10 | 0 | 0 | 2 | 0 | JUnit failure body mislabels informational (non-gating) criteria as [FAIL], diverging from reports.py |
Overall Score: 9.5 / 10 · Weakest Axis: API Surface & Maintainability at 9 / 10
Totals: 🔴 0 · 🟠 1 · 🟡 5 · 🔵 4 across 8 axes.
Blockers
- [Axis 7] Action/README document a non-existent env var CODER_EVAL_API_BACKEND — the Bedrock-via-action config path is silently ignored (
README.md:148) — The Settings class (src/coder_eval/config.py:84) is a pydantic-settings BaseSettings with model_config at line 147 that sets NO env_prefix; fieldapi_backend(config.py:98) therefore reads the env varAPI_BACKEND(case-insensitive), andextra="ignore"means any unrecognized env var is silently dropped. Both new doc surfaces tell users to setCODER_EVAL_API_BACKENDinstead:
- README.md:148 (copy-pasteable action example):
CODER_EVAL_API_BACKEND=bedrock - action.yml:55 (the
envinput description):... set ANTHROPIC_API_KEY, CODER_EVAL_API_BACKEND, model vars ...
BecauseCODER_EVAL_API_BACKENDis not a Settings field and has no AliasChoices (only the telemetry fields do), it is ignored — the run silently falls back toApiBackend.DIRECT. A user following the README's Bedrock example thus runs on Direct Anthropic (wrong backend/model, or a confusing missing-ANTHROPIC_API_KEY auth error) with no indication their backend selection was dropped. This is the ONLY documented way to select the backend through the composite action (there is nobackendinput; onlymodel,extra-args,env). By contrast run_command.py:224 and docs/tutorials/02-ci-pipeline.md:240 correctly referenceAPI_BACKEND/--backend. Fix both toAPI_BACKEND=bedrock(README.md:148 and action.yml:55). Optionally add a CE lint / alias so the framework's canonical env vars are documented from one source.
Non-blocking, but please consider before merge
- [Axis 1] Three new reports_junit.py functions land in the CC 10-20 band (
src/coder_eval/reports_junit.py:183) — Per the Axis-1 anchor, cyclomatic complexity 10-20 outside a hot module is Medium. radon confirms three new functions in this band:_task_case(line 183) C(17),_criteria_body(line 145) C(14), and_suite_gate_suite(line 271) C(11)._task_casein particular interleaves variant/task_id resolution, replicate-index formatting (has_replicate = isinstance(replicate_index, int) and not isinstance(replicate_index, bool)), classname derivation, finite/non-negative duration validation (valid_duration = ... and math.isfinite(duration) and duration >= 0), a properties-emission loop, and the status->category->failure/error branching in one body. The complexity is driven by defensive isinstance guards over untyped dict rows, so it is not a correctness defect, but it exceeds the readability bar for a single function. Consider extracting the duration-validation and properties-emission blocks (e.g._time_attr(row)and_case_properties(case, row)) so each function stays comfortably under 10; this also isolates the well-tested edge-case logic. Low real-world impact given the code is well-tested and localized. - [Axis 3] JUnit writer's key-coupling to eval_result_to_task_dict is untyped (RunSummary.task_results is list[dict[str, Any]]) and has no parity test — all JUnit tests use the synthetic _row() helper (
tests/test_reports_junit.py:39) — Every JUnit test builds its input via the local_row(...)helper (line 39) and thewrite_run_jsonfixture, both of which hand-craft dicts whose keys (status,variant_id,task_path,replicate_index,weighted_score,model_used,total_tokens,total_cost_usd,visible_turns,duration) are copied from what the writer reads — not from the real producer.generate_junit_xmlconsumesRunSummary.task_results, which in production is built byreports_experiment.eval_result_to_task_dict(batch.py:628). No test feeds realeval_result_to_task_dictoutput throughgenerate_junit_xml, so the writer's key-coupling to the producer is unverified. A rename in the producer (e.g.visible_turns,total_cost_usd, ortask_path) would silently drop the property/classname; a rename ofstatuswould route_status_ofto "" and mis-bucket every task as an error, all with zero failing JUnit assertions. Add one integration test that constructs anEvaluationResult, runs it througheval_result_to_task_dict(orbuild_run_summary) into a realrun.json, and asserts the resulting JUnit tree (grouping, classname, properties, status classification) — a full-field parity check rather than synthetic rows. - [Axis 6] Root time attribute not guarded against NaN/inf, unlike per-testcase time — emits silently-invalid JUnit XML (
src/coder_eval/reports_junit.py:335) — In generate_junit_xml the root time is set unconditionally:root.set("time", f"{summary.total_duration_seconds:.3f}")(line 335). RunSummary.total_duration_seconds is a plainfloatwith noallow_inf_nan=False/finite validator, and both json.loads and pydantic v2 accept a JSONNaN/Infinityliteral, so a blob-pulled/older/corrupt run.json survives RunSummary validation and produces<testsuites name="r1" time="nan" ...>(confirmed by direct reproduction) — an invalid document many JUnit ingesters reject wholesale. This contradicts the module's own contract: _task_case (lines 199-205) deliberately guards the same value per row (valid_duration = isinstance(duration, int|float) and not isinstance(duration, bool) and math.isfinite(duration) and duration >= 0 ... time_str = f"{duration:.3f}" if valid_duration else "0.000") and is covered by test_malformed_row_types_degrade_gracefully, but the root time path was never given the same guard. Apply the identical finite/non-negative guard to summary.total_duration_seconds before setting the root time (fall back to "0.000"), and add a test asserting the root time is finite for a NaN/inf total_duration_seconds run.json. Severity Medium (not High/Critical): does not affect any task score/final_status and is only reachable via a corrupt/foreign run.json since coder-eval itself always computes a finite duration; matches the Medium anchor 'inconsistent error context (some paths wrap, some don't)'. - [Axis 8] JUnit failure body mislabels informational (non-gating) criteria as [FAIL], diverging from reports.py (
src/coder_eval/reports_junit.py:165) — In_criteria_body, line 165 computespassed = isinstance(score, int | float) and isinstance(threshold, int | float) and score >= thresholdfrom score>=threshold alone and, on the else branch, emits[FAIL] {ctype}: .... It never consults thegatingfield on the CriterionResult. Per the documented contract onCriterionResult.gating(models/results.py: 'False marks an informational criterion ... excluded from the score and the pass/fail gate, so every display surface must render it as informational rather than failed'), a non-gating criterion below its threshold must NOT be rendered as failed. The parallel path in reports.py:_compute_suite_rollup explicitly guards this —if not cr.gating: continue(with the comment 'Informational (weight: 0) criteria cannot fail a row, so they must not supply the reason the row is reported as failed'). The JUnit body diverges: on an already-failed testcase it will surface an informational criterion as[FAIL] ... < threshold, misleading a CI reader into thinking an informational metric caused the failure. This does NOT change the testcase's pass/fail (that comes from FinalStatus via_category_of) nor any score, so it is a display-contract defect, not a scoring-correctness one. Fix: readcrit.get('gating', True)and render non-gating criteria as[INFO](or skip them from the failure cause), mirroring reports.py. - [Axis 8] Dataset-row tasks (task_id contains '/') never load task.json, degrading JUnit detail for the whole dataset/activation suite (
src/coder_eval/reports_junit.py:106) —_is_safe_component(line 106) returns False for any value containing '/':return bool(value) and value not in {'.', '..'} and '/' not in value and '\\' not in value, and_load_task_json(line 119) bails to None whennot _is_safe_component(task_id). But dataset expansion setstask_id = f'{task.task_id}/{row_id}'(orchestration/task_loader.py:433), and the on-disk layout isrun_dir/variant_id/task_id/NN/task.json(path_utils.build_task_run_dir:37) — i.e. the '/' in a dataset task_id is a legitimate structural path separator, andrun_dir / variant / task_idalready resolves to the correct nested dir. As written, EVERY dataset-derived row (the flagship nightly activation suite is dataset-backed per CLAUDE.md) fails the '/' check, so_criteria_bodyalways falls back to the status-only body (status=... weighted_score=...) and never emits per-criterion detail — even though task.json exists and the belt-and-bracescandidate.resolve().is_relative_to(run_dir.resolve())check (line 137) already prevents traversal escapes. The testcase is still classified correctly (no scoring impact), so this is a detail-degradation, not a correctness bug. Fix: stop treating an internal '/' as unsafe — reject only absolute paths and '..' components (e.g. check'..' not in PurePosixPath(value).parts and not value.startswith('/')) and rely on the existing resolve-containment guard, so nested dataset task.json paths load.
Nits
- [Axis 2] _load_task_json declares dict[str, Any] | None return but can return any JSON type (
src/coder_eval/reports_junit.py:109) — Line 109 declaresdef _load_task_json(...) -> dict[str, Any] | None:, but line 140 returnsjson.loads(candidate.read_text(encoding="utf-8")), which is typedAnyand at runtime yields whatever the file's top-level JSON is (alist,int, orstrfor a corrupted/blob-pulled task.json whose root is not an object). Pyright does not flag this becauseAnyis assignable todict. The annotation is therefore narrower than reality. There is no runtime bug: the sole caller_criteria_body(line 153-154) re-narrows viacriteria = data.get(...) if isinstance(data, dict) else None, so a non-dict falls through safely. Tighten the annotation to-> dict[str, Any] | list[Any] | ... | Noneor returnjson.loads(...) if isinstance(<parsed>, dict) else Noneinside the helper so the declared type is honest and the isinstance guard cannot be dropped by a future caller without a type error. - [Axis 3] Symlink-escape containment branch (reports_junit.py:137) is untested; the test that claims to cover escape only exercises the earlier name guard (
tests/test_reports_junit.py:520) —test_task_json_lookup_cannot_escape_run_dir(line 520) usesvariant_id="../outside"andtask_id=".."(line 542). Both are rejected by_is_safe_component(contains// equals..), so_load_task_jsonreturnsNoneat the early guard and thecandidate.resolve().is_relative_to(run_dir.resolve())symlink-containment check at reports_junit.py:137 (a coverage-missing line) never executes. The actual scenario line 137 guards — a legitimately-named<variant>/<task_id>/NN/task.jsonthat is a symlink pointing outsiderun_dir— is uncovered. Add a case that creates such a symlink and asserts the outside content is not read (status-only fallback body). - [Axis 5] Per-task score gate implemented as inline Python in action.yml duplicates the package's scored-row interpretation and is CI-vendor-locked (
action.yml:155) — The new minimum-task-score gate lives entirely in the composite action as embedded Python:for r in data.get("task_results", []): s = r.get("weighted_score") ... if isinstance(s, (int, float)) and not isinstance(s, bool) and math.isfinite(s)(action.yml ~line 181). This re-implements the same scored-row filtering the package already owns (reports_junit._task_case / reports_stats scored-row aggregation apply the identicalisinstance(... int|float) and not bool and math.isfiniteinterpretation ofweighted_score). Because it lives in action.yml rather than as acoder-eval runcapability, (a) it is not reachable from the Azure DevOps path the JUnit output explicitly targets (docstring namesPublishTestResults@2), and (b) it is a second interpretation of run.json's scoring semantics maintained outside the SSOT models, which can silently drift ifweighted_score/task_resultssemantics change. Consider hoisting the floor into the CLI (e.g.coder-eval run --min-task-score) so the action becomes a thin caller and the interpretation stays in one tested place. Low: a new self-contained feature rather than a worsening of an existing path, and shell/YAML cannot import the Python helper directly. - [Axis 5] reports_junit.py couples the reports layer to evaluation/judge_context for a trivial string helper while a sibling renderer keeps its own (
src/coder_eval/reports_junit.py:30) — Line 30 isfrom .evaluation.judge_context import truncate— the new reports module reaches into the judge-specificevaluation/judge_context.pyfor a generic string truncation helper, introducing a reports->evaluation dependency. Meanwhile the parallel rendererreports_html.pydefines its own local_truncate(reports_html.py:250def _truncate(text, limit)). This is pattern drift between sibling renderers and pulls a general utility out of a judge-context module. No import cycle exists (judge_context does not import reports), so this is a Low layering/DRY nit: movetruncateto a shared util (e.g. formatting.py) or have both renderers share one helper.
What's Missing
Tests:
- 🟠 The new
minimum-task-scoregate (inline Python in action.yml, ~lines 155-205) has zero test coverage. This is the branch that turns a per-task score into a CI pass/fail verdict, yet nothing exercises it: no test trips the floor (a BELOW task failing the step), no test covers the [0.0,1.0]/non-numeric validation branches, the missing-run.json fail-closed branch, the NaN/inf skip (fail-closed against a blob-pulled run.json), or the 'no scored tasks' branch. Because the logic lives as embedded Python in YAML it is invisible to pytest/coverage. Add a test that runs the gate script against fixture run.json files (below-floor, at-floor, empty, malformed) and asserts exit status. This is new public scoring behavior shipped untested — the most common reviewer block per Agent 3 ('the gate that turns a gap into a score'). (trigger: action.yml) - 🟡 No integration/parity test feeds real
eval_result_to_task_dictoutput throughgenerate_junit_xml. Every JUnit test builds input via the synthetic_row()helper (tests/test_reports_junit.py), which hand-copies the keys the writer reads, so the untyped producer->consumer key-coupling (RunSummary.task_results is list[dict[str,Any]]) is unverified — a producer rename ofstatus/task_path/total_cost_usd/model_used/total_tokens/weighted_scorewould silently drop a property/classname or mis-bucket every row, with no failing assertion. Add one test that runs a real EvaluationResult -> eval_result_to_task_dict -> run.json -> generate_junit_xml and asserts grouping, classname, properties, and status classification. (trigger: tests/test_reports_junit.py) (restates: Axis 3: JUnit writer key-coupling untested) - 🟡 The root
<testsuites time=...>NaN/inf guard is both un-added and untested._task_caseguards the per-testcase time (valid_duration finite/non-negative check) and is covered by test_malformed_row_types_degrade_gracefully, but generate_junit_xml sets the root time unconditionally from summary.total_duration_seconds (which has no allow_inf_nan=False validator), so a corrupt/blob-pulled run.json emits time="nan" — invalid JUnit — with no test asserting root-time finiteness. Add the same finite/non-negative guard plus a regression test. (trigger: src/coder_eval/reports_junit.py) (restates: Axis 6: root testsuites time NaN/inf unguarded) - 🔵 The symlink-escape containment branch (reports_junit.py:136,
candidate.resolve().is_relative_to(run_dir.resolve())) is untested; test_task_json_lookup_cannot_escape_run_dir uses../outside/..which are rejected by the earlier_is_safe_componentname guard, so line 136 never executes. Add a case with a legitimately-named<variant>/<task_id>/NN/task.jsonsymlink pointing outside run_dir and assert the outside content is not read. (trigger: tests/test_reports_junit.py) (restates: Axis 3: symlink-escape branch untested) - 🔵 The generic env-passthrough parser in action.yml (NAME=VALUE loop: CRLF trimming, left-trim, comment/blank skipping, name-regex validation, secret-safe positional error reporting, and the deliberate non-write to $GITHUB_ENV) is entirely new, security-relevant shell logic with no test. Nothing verifies a missing
=is rejected, an invalid name is rejected, a secret VALUE is never echoed, or that forwarded vars don't bleed into later steps. Add a workflow-level or bats-style test over the parsing loop. (trigger: action.yml)
Parallel paths:
- 🟡 reports_junit.py
_criteria_bodycomputes pass/fail from score>=threshold alone and renders sub-threshold criteria as[FAIL], diverging from the parallel renderer path reports.py_compute_suite_rollup, which guardsif not cr.gating: continue. A non-gating/informational criterion below threshold is thus mislabeled[FAIL]in the JUnit failure body — a fifth renderer that didn't adopt the gating contract the other renderers honor. Readcrit.get('gating', True)and render non-gating as[INFO](or skip). Does not change the testcase verdict (that comes from FinalStatus via _category_of), so it is a display-contract divergence, not a scoring bug. (trigger: src/coder_eval/reports_junit.py) (restates: Axis 8: informational criteria mislabeled [FAIL]) - 🔵
truncatewas pulled from evaluation/judge_context into the new reports-layer module (reports_junit.py:30), introducing a reports->evaluation dependency, while the sibling renderer reports_html.py keeps its own local_truncate. The generic helper was not hoisted to a shared util, so the parallel renderers now drift on how they truncate. Move truncate to a shared formatting util used by both. (trigger: src/coder_eval/reports_junit.py) (restates: Axis 5: reports_junit couples to judge_context truncate)
Downstream consumers:
- 🔵 The action.yml score gate re-implements run.json's scored-row
weighted_scoreinterpretation (isinstance int|float, not bool, math.isfinite) outside the SSOT models — a second interpretation of task_results scoring semantics maintained in embedded Python, which can silently drift from reports_junit._task_case / reports_stats if those semantics change. It is also unreachable from the Azure DevOps PublishTestResults@2 path the JUnit output explicitly targets. Consider hoisting the floor into the CLI (coder-eval run --min-task-score) so the interpretation stays in one tested place and the action becomes a thin caller. (trigger: action.yml) (restates: Axis 5: per-task score gate duplicates scored-row logic)
Daily/nightly:
- 🟠 The action's only documented way to select a backend —
CODER_EVAL_API_BACKEND(README.md:148, action.yml:55 env description) — is not a Settings field (Settings has no env_prefix and readsAPI_BACKEND; extra='ignore' silently drops the unknown var). So the nightly/CI--backend bedrockjudge route selected through the composite action silently falls back to ApiBackend.DIRECT. A user following the README Bedrock example runs on Direct Anthropic (wrong backend/model, or a confusing missing-ANTHROPIC_API_KEY error) with no signal. Fix both surfaces toAPI_BACKEND=bedrock. (trigger: README.md) (restates: Axis 7: CODER_EVAL_API_BACKEND env var doesn't exist) - 🟡
_is_safe_componentrejects any value containing '/', and dataset expansion sets task_id = '/<row_id>' (task_loader.py:433) with a matching nested on-disk layout (path_utils.py:37). So every dataset-derived row in the flagship dataset-backed nightly activation suite fails the '/' check and degrades to a status-only JUnit body (no per-criterion detail), even though task.json exists and the resolve-containment guard already prevents traversal. The PR does not state this blast radius on the nightly. Reject only absolute paths and '..' components (rely on the existing resolve-containment check) so nested dataset task.json paths load; classification is unaffected (detail-degradation only). (trigger: src/coder_eval/reports_junit.py) (restates: Axis 8: dataset-row tasks never load task.json)
Harness & Lint Improvements
Static checks (lint / type):
- [ruff] Enable ruff's mccabe gate: add "C901" to the
selectlist in pyproject.toml and set[tool.ruff.lint.mccabe] max-complexity = 10. reports_junit._task_case C(17), _criteria_body C(14), and _suite_gate_suite C(11) would all trip the gate inmake checkbefore review. (ruff already runs PLR0912/PLR0915 but neither is a cyclomatic-complexity gate.) Prevents: Finding #1 (Axis 1): three new reports_junit.py functions land in the CC 10-20 band. - [ce-lint] New rule CE026 (layering, AST ImportFrom check mirroring CE004/no_cli_imports_in_core.py): the reports layer (reports.py, reports_junit.py, reports_html.py, reports_experiment.py, reports_stats.py) must not import from coder_eval.evaluation.*. Slot the rule in tests/lint/rules/ce026_no_reports_import_evaluation.py and register it in tests/lint/runner.py. Would have flagged
from .evaluation.judge_context import truncateat reports_junit.py:30 and forced the shared string helper into a util module. Prevents: Finding #6 (Axis 5): reports layer couples to evaluation/judge_context for a trivial string helper. - [ce-lint] New rule CE027 (doc/config env-var parity): scan README.md, action.yml, and docs/ for env-var tokens (uppercase
NAME=assignments andCODER_EVAL_*/API_*references) and cross-check each against the field env names + AliasChoices ofcoder_eval.config.Settings(the rule imports Settings directly). Flag any documented env var with no backing Settings field, sinceextra="ignore"silently drops it at runtime. Would have caught the non-existentCODER_EVAL_API_BACKENDin README.md:148 and action.yml:55 (real field isAPI_BACKEND). Prevents: Finding #8 (Axis 7, High): action/README document a non-existent env var CODER_EVAL_API_BACKEND. - [ce-lint] New rule CE028 (finite-float on run.json-sourced models): any float field on the pydantic models that parse an external/blob-pulled run.json (RunSummary and the task-row models) must declare
allow_inf_nan=False(or an equivalent finite validator), so a JSONNaN/Infinityliteral fails validation instead of surviving into the writer. This fixes the class at the parse seam and covers BOTH the guarded per-row time and the unguarded root time in one place. Prevents: Finding #7 (Axis 6): root time attribute unguarded against NaN/inf emits invalid JUnit XML.
Harness improvements (not statically reachable):
- Add a producer→consumer integration/parity test: build a real EvaluationResult, run it through reports_experiment.eval_result_to_task_dict (or build_run_summary) into a real run.json, feed that through generate_junit_xml, and assert grouping / classname / properties / status classification on the resulting tree. Replaces the synthetic
_row()helper as the source of the writer's key contract. Why not static: RunSummary.task_results islist[dict[str, Any]](models/results.py:880), so the writer's dependence on producer key names (status,task_path,total_cost_usd,model_used,total_tokens,weighted_score) is untyped — pyright/pydantic cannot see a rename; only running real producer output through the consumer surfaces the drift. Prevents: Finding #3 (Axis 3): JUnit writer's key-coupling to eval_result_to_task_dict has no parity test. - Add a JUnit test that creates a validly-named
<variant>/<task_id>/NN/task.jsonwhich is a symlink pointing OUTSIDE run_dir, and assert the outside content is not read (status-only fallback body), exercising thecandidate.resolve().is_relative_to(run_dir.resolve())containment branch at reports_junit.py:136. Why not static: The guarded branch only executes for a name that passes_is_safe_componentyet resolves outside run_dir — that requires a real on-disk symlink and pathlib resolve() at runtime; no static check can reach a symlink-containment outcome. Prevents: Finding #4 (Axis 3): symlink-escape containment branch is untested (existing test only hits the name guard). - Add a JUnit test with a dataset-style nested task_id (containing '/', e.g.
suite/row_0) laid out on disk asrun_dir/variant/suite/row_0/NN/task.json, asserting _load_task_json loads it and per-criterion detail is emitted (not the status-only fallback). This also pins the intended fix (reject only absolute paths and '..' components, rely on resolve-containment). Why not static: Correctness depends on the '/' in a dataset task_id being a real nested-directory separator matching path_utils.build_task_run_dir — verifiable only against an actual run-dir layout at runtime, not by grep/AST. Prevents: Finding #10 (Axis 8): dataset-row tasks (task_id contains '/') never load task.json, degrading JUnit detail for the whole activation suite. - Add a shared/parity test (or extract one shared criterion-render helper used by both reports.py suite-rollup and reports_junit._criteria_body) that feeds a CriterionResult with gating=False below threshold through both surfaces and asserts neither renders it as the failure cause — reports_junit should emit [INFO], matching reports.py's
if not cr.gating: continue. Why not static: The contract is semantic — 'a non-gating criterion below threshold must render as informational, not failed' — which requires evaluating a populated CriterionResult through the rendering logic and comparing the two surfaces; a lint grep for[FAIL]without agatingreference is too false-positive-prone to be the gate. Prevents: Finding #9 (Axis 8): JUnit failure body mislabels informational (non-gating) criteria as [FAIL]. - Hoist the minimum-task-score floor into the package/CLI (e.g.
coder-eval run --min-task-score) so the composite action becomes a thin caller, and back it with a unit test on the CLI. The action.yml then stops re-implementing the scored-row interpretation (isinstance(...int|float) and not bool and math.isfinite). Why not static: The duplication lives as Python embedded inside action.yml shell/YAML, which no ruff/pyright/CE rule can parse or tie back to the package helper; it is an architecture/design decision to keep run.json scoring semantics in one tested place rather than a mechanically-detectable code pattern. Prevents: Finding #5 (Axis 5): per-task score gate implemented as inline Python in action.yml duplicates the package's scored-row interpretation.
Top 5 Priority Actions
- Fix the non-existent env var CODER_EVAL_API_BACKEND at README.md:148 and action.yml:55 to API_BACKEND (or add an AliasChoices) — it is currently the only action-level way to select Bedrock and is silently dropped by Settings' extra="ignore", so users get a Direct-Anthropic run with wrong backend/model or a confusing auth error.
- Stop treating an internal '/' as unsafe in _is_safe_component/_load_task_json (reports_junit.py:106,119) and rely on the existing resolve-containment guard, so dataset/activation-suite rows (task_id = suite/row_id) load task.json instead of degrading every row to status-only JUnit detail.
- Consult the CriterionResult.gating field in _criteria_body (reports_junit.py:165) and render non-gating criteria as [INFO] rather than [FAIL], mirroring reports.py's
if not cr.gating: continue, so informational metrics below threshold no longer appear to CI readers as the failure cause. - Apply the same finite/non-negative guard used per-testcase (reports_junit.py:199-205) to the root time at reports_junit.py:335, so a corrupt/blob-pulled run.json with NaN/inf total_duration_seconds cannot emit time="nan" and produce JUnit XML that ingesters reject wholesale.
- Add an integration parity test that runs a real EvaluationResult through eval_result_to_task_dict → run.json → generate_junit_xml (tests/test_reports_junit.py currently only uses the synthetic _row helper), asserting grouping/classname/properties/status so a producer-side rename of status/task_path/total_cost_usd/etc. can no longer silently mis-bucket or drop JUnit fields.
Stats: 0 🔴 · 1 🟠 · 5 🟡 · 4 🔵 across 8 axes reviewed.
New src/coder_eval/reports_junit.py: generate_junit_xml(run_dir) reads the run.json spine (RunSummary), optional */*/suite.json gates (SuiteRollup), and per-failed-row task.json (best-effort plain-dict) to build a JUnit XML string; write_junit_xml is the thin persist wrapper. Counts always equal emitted children; status classification goes through FinalStatus.category; all agent-derived text is scrubbed of illegal XML 1.0 chars. Production code never parses XML. Adds defusedxml (dev-only) for test-side round-trip parsing and a shared write_run_json conftest fixture reused by later phases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add --junit-xml to `coder-eval run` (writes the JUnit report after the run summary is persisted and before the failure exit-code gate, so a red run still produces a report; write errors propagate). Add 'junit' to `coder-eval report --format` (regenerates from any run dir; defaults to <run-dir>/junit.xml, -o overrides; missing run.json → clean red error). Output-only — does not enter the config merge. Docs: CI-pipeline tutorial JUnit section + CLAUDE.md tree entry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add action.yml at the repo root: a composite action that installs a pinned coder-eval (or the local checkout via `version: local`), runs the suite, always emits run-dir/junit-path outputs and a run.md job summary, then exits with coder-eval's real exit code. Inputs cross into bash via env: only. release.yml now bumps the action's `version:` default inside the amended release commit (anchored on a `# <-- kept in sync` comment, with a loud grep guard) and force-moves the `v<major>` tag after pushing the release tag. pr-checks.yml gains a fork-gated `action-dogfood` job that exercises the action via `uses: ./` on one Haiku task and verifies the JUnit + run.json output — the sensor for this surface, since it isn't covered by pytest. The action is deliberately agent-agnostic: it installs no coding-agent runtime, so callers provide the Claude CLI (as the dogfood job does). Documented in the README and the CI tutorial along with the pull_request_target security caveat. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Companion to the pyproject dev-group addition in 1/3. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Harden reports_junit.py against schema-skewed / crafted run.json rows, which are untyped dicts (RunSummary.task_results is list[dict[str, Any]]) and may be blob-pulled from elsewhere: - Path containment: a crafted variant_id/task_id could steer the task.json lookup outside the run dir (absolute value discards run_dir, ".." walks up). Reject unsafe path components and verify containment after resolve(). - UnicodeDecodeError is a ValueError but NOT a json.JSONDecodeError, so a task.json with undecodable bytes aborted the whole report instead of falling back. Catch ValueError. - Ambiguous replicate: with replicate_index absent and several replicate dirs, the writer attributed replicate 00's failure detail to the row. Degrade to the status-only body instead of misattributing. - bool is an int subclass, so a bool replicate_index rendered as "[01]" and a bool duration as a time. Exclude bool explicitly. - Guard non-finite/negative durations (NaN would emit an invalid time="nan"). - Coerce non-str variant_id/task_path rather than crashing on dict keys/Path(). - Skipped testcases now use the suffix-stripped path, not the bare stem, so two skipped tasks sharing a basename keep distinct JUnit identities. - A row with no status reads as "<missing>" rather than "None". Found by the final multi-model review (gpt-5.6, gemini-3.1-pro, gpt-5.3-codex); each fix has a regression test that fails without it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
_skipped_suite built the testcase name with str(Path(path).with_suffix("")),
which yields "tasks\opt" on Windows and "tasks/opt" on Linux. That made the
JUnit testcase identity depend on the OS that generated the report, so the same
logical run would split into two identities in CI history/flake tracking (and it
broke the Windows smoke job).
Normalize separators and parse with PurePosixPath so the emitted name is always
"/"-separated. Adds a regression test feeding a Windows-style path.
Also switch the action's model in the README/tutorial examples from Haiku to
Sonnet (docs only; the dogfood job stays on Haiku for per-PR cost).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
Replace the single `anthropic-api-key` input with a generic `env` NAME=VALUE passthrough (exported for the run step only, never $GITHUB_ENV) and add an optional `minimum-task-score` floor read from the always-written run.json spine. Both default off, so existing behavior is unchanged. README, tutorial, and the dogfood job route ANTHROPIC_API_KEY through `env`. Code-review hardening (gemini-3.1-pro + gpt-5.3-codex + opus): - score gate skips non-finite weighted_score (json.loads parses NaN; NaN makes min()/>= order-dependent and could mask a below-floor task) - minimum-task-score validated (numeric, finite, in [0.0, 1.0]) with a clean ::error:: instead of a raw traceback - malformed env entries reported by position, never echoing the raw line, so a mis-prefixed secret can't leak into logs Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Code-review fixes to the JUnit XML CI gate, plus a new doc/config lint rule and a producer→consumer parity test. reports_junit.py: - Load task.json for dataset rows: task_id "<suite>/<row_id>" is a real nested dir, so validate it as a contained relative path (_is_safe_relpath) instead of rejecting any '/'. Rejects absolute/backslash/'..'/Windows-drive values; the resolve()-containment check remains the backstop. - Guard the root <testsuites> time against NaN/inf like per-testcase time via a shared _time_attr helper; also degrade (not crash) on a pathologically large integer duration that overflows float()/'.3f'. - Render informational (gating=False) criteria as [INFO], not [FAIL]/[PASS], mirroring reports.py; only an explicit JSON `false` is informational (null / non-bool fails safe to gating). Docs: fix CODER_EVAL_API_BACKEND -> API_BACKEND (the real Settings env name; the CODER_EVAL_-prefixed spelling was silently dropped by Settings extra="ignore", selecting no backend). CE027 (tests/lint/doc_env_parity.py): new rule flagging framework-prefixed env-var assignments in README/action.yml/docs that no Settings field/alias or src/ consumer backs — the class that produced the API_BACKEND bug. Assignment-scoped with a hardened boundary and a real-consumer src scan to avoid false positives. Tests: dataset nested-load, nested '..'/drive rejection, informational [INFO] (passing + null-gating), root/huge-int time degradation, and a producer→consumer parity test running real eval_result_to_task_dict output through generate_junit_xml. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
3b00991 to
aa8ef40
Compare
Review responses — addressed in
|
…exes PR #37 shipped a packaged CI gate — a composite GitHub Action (action.yml), JUnit XML output (reports_junit.py + `run --junit-xml` / `report -f junit`), and an optional per-task score floor — but the docs site had no reference for it (only the walkthrough tutorial and the README section). - docs/CI_GATE.md (new): reference for the composite Action (inputs/outputs, env-passthrough credentials, minimum-task-score floor, security), JUnit output (both entry points, what it's built from, GitHub/Azure ingestion). - Add it to the index surfaces: docs/index.md, mkdocs.yml nav (Running & scaling), llms.txt, and the README documentation table. - tutorials/02-ci-pipeline.md: repoint the action reference link from the off-site ../../README.md (broke the strict docs build and the Starlight link rewriter) to the new on-site CI_GATE.md. mkdocs build --strict passes clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Makes coder-eval consumable as a CI gate: a JUnit XML report generated from any run directory, plus a composite GitHub Action published from the repo root.
What's in it
1.
src/coder_eval/reports_junit.py(new) —generate_junit_xml(run_dir)reads a finalized run directory and returns JUnit XML;write_junit_xmlis the thin persist wrapper (mirrors thebuild_run_summary/write_run_summaryseam).run.jsonspine via theRunSummarymodel (SSOT, not hand-parsed), optional*/*/suite.jsongates viaSuiteRollup, and per-failed-rowtask.jsonas a plain dict (best-effort, so schema skew in blob-pulled/older runs degrades to a status-only body instead of aborting).<testsuite>per variant, one<testcase>per task row ([NN]suffix for replicates);skippedandsuite-gatessynthetic suites. Count attributes are derived from the emitted children, so they can't drift.FinalStatus(value).category— an explicit allowlist with an explicit unknown-value fallback, never a denylist (CE018).defusedxmlis added dev-only for test-side round-trips.2. CLI wiring — output-only flags; neither enters the 5-layer config merge.
coder-eval run --junit-xml PATH— written after the run summary is persisted and before the failure exit-code gate, so a red run still emits a report. A write error propagates (a silently missing file would make a downstream reporter fail confusingly).coder-eval report <run-dir> -f junit [-o PATH]— regenerates from any existing run dir (defaults to<run-dir>/junit.xml).3.
action.yml+ release automation + dogfoodversion: localescape hatch, always emitsrun-dir/junit-pathoutputs and appendsrun.mdto$GITHUB_STEP_SUMMARY, then exits with coder-eval's real exit code. Every input crosses into bash viaenv:— zero${{ }}interpolation into script bodies.release.ymlbumps the action's pinnedversion:default inside the amended release commit (anchored on a# <-- kept in synccomment, with agrepguard that fails the release loudly rather than shipping a stale pin) and force-moves thev<major>tag.pr-checks.ymlgains a fork-gatedaction-dogfoodjob — the sensor for this surface, since it isn't reachable from pytest.Design note (deviation from the original plan)
The action is deliberately agent-agnostic: it installs
coder-evalbut no coding-agent runtime.claude-agent-sdkdoes not bundle theclaudebinary, so callers provide it — the dogfood job installs Node +@anthropic-ai/claude-codeitself, and the README/tutorial document this. Keeps the action usable for codex/antigravity consumers instead of forcing a Claude CLI install on everyone.Review fixes
A multi-model review (gpt-5.6, gemini-3.1-pro, gpt-5.3-codex) found several robustness gaps around the untyped
task_resultsrow dicts, all fixed with regression tests that fail without the fix:variant_id/task_idcould steer thetask.jsonlookup outside the run dir (an absolute value discardsrun_dir;..walks up). A test confirmed a real leak before the fix.UnicodeDecodeErroris aValueErrorbut not ajson.JSONDecodeError, so undecodable bytes aborted the whole report despite the documented graceful degrade.replicate_indexabsent and several replicate dirs, replicate 00's failure detail was misattributed to the row; now degrades instead.boolis anintsubclass — a boolreplicate_indexrendered as[01]; NaN/inf durations emitted an invalidtime="nan".variant_id/task_pathcrashed the dict key /Path(); same-basename skipped tasks collapsed into one JUnit identity.Testing
make verifygreen: 3454 passed, 2 skipped (both pre-existing environment skips), 90.98% coverage (threshold 80%);reports_junit.py~98%.tmp_path, no agents, no API.sed(proven to rewrite only the version line and preserve the trailing comment), an empirical simulation of the action's argv building underset -uo pipefail, andactionlint(clean on all additions).What can't be verified until this PR runs
action-dogfood— this PR both adds the job and is its first subject. Composite-action quirks (setup-uvPATH propagation,$GITHUB_ACTION_PATHinstall, output plumbing) are only provable in CI.🤖 Generated with Claude Code