Skip to content

Commit aa8ef40

Browse files
uipreligaclaude
andcommitted
fix(reports): JUnit CI-gate review fixes + CE027 env-var lint
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>
1 parent b669a38 commit aa8ef40

6 files changed

Lines changed: 541 additions & 19 deletions

File tree

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ it can't leak into later steps). Set whatever the run needs, Anthropic or not:
145145
tasks: tests/tasks/**/*.yaml
146146
minimum-task-score: "0.8" # fail the build if any task scores below 0.8
147147
env: |
148-
CODER_EVAL_API_BACKEND=bedrock
148+
API_BACKEND=bedrock
149149
AWS_BEARER_TOKEN_BEDROCK=${{ secrets.BEDROCK_TOKEN }}
150150
```
151151

action.yml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ inputs:
5252
the coder-eval process only (scoped to the run step; NOT written to
5353
$GITHUB_ENV, so nothing leaks into later job steps). This is the sole
5454
channel for credentials and backend config: set ANTHROPIC_API_KEY,
55-
CODER_EVAL_API_BACKEND, model vars, EVALBOARD_*, plugin paths, etc. Names
55+
API_BACKEND, model vars, EVALBOARD_*, plugin paths, etc. Names
5656
must match ^[A-Za-z_][A-Za-z0-9_]*$; blank lines and `#` comments are
5757
ignored. Wire values from repository secrets (secrets.MY_KEY) — never
5858
inline a secret literal.

src/coder_eval/reports_junit.py

Lines changed: 70 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@
2424
import math
2525
import re
2626
import xml.etree.ElementTree as ET
27-
from pathlib import Path, PurePosixPath
27+
from pathlib import Path, PurePosixPath, PureWindowsPath
2828
from typing import Any, Literal
2929

3030
from .evaluation.judge_context import truncate
@@ -99,13 +99,60 @@ def _is_safe_component(value: str) -> bool:
9999
"""True when ``value`` is usable as a single, contained path segment.
100100
101101
``run.json`` rows are untyped and may be blob-pulled from elsewhere, so a
102-
crafted ``variant_id``/``task_id`` must not steer the lookup outside the run
103-
directory (an absolute value would discard ``run_dir`` entirely, and ``..``
104-
would walk up).
102+
crafted ``variant_id`` must not steer the lookup outside the run directory
103+
(an absolute value would discard ``run_dir`` entirely, and ``..`` would walk
104+
up).
105105
"""
106106
return bool(value) and value not in {".", ".."} and "/" not in value and "\\" not in value
107107

108108

109+
def _is_safe_relpath(value: str) -> bool:
110+
"""True when ``value`` is a contained relative path — possibly *nested*.
111+
112+
Unlike :func:`_is_safe_component`, an internal ``/`` is allowed: dataset
113+
expansion rewrites a row's ``task_id`` to ``"<suite>/<row_id>"``
114+
(``task_loader.expand_dataset``) and the on-disk layout is correspondingly
115+
nested (``run_dir/<variant>/<suite>/<row_id>/<NN>/task.json``, see
116+
``path_utils.build_task_run_dir``). Rejecting the ``/`` would degrade every
117+
dataset-derived row (e.g. the activation suite) to a status-only body.
118+
119+
Only an absolute path, a Windows drive/UNC prefix, or a ``.``/``..``
120+
component could escape ``run_dir``; all are rejected here, and the
121+
``resolve()``-containment check in :func:`_load_task_json` is the
122+
belt-and-braces backstop (symlinks included).
123+
"""
124+
# A Windows drive-qualified value (``C:/x``) reads as a plain relative path
125+
# on POSIX but is absolute on Windows, so reject it explicitly rather than
126+
# leaning on the resolve-containment backstop alone.
127+
if not value or value.startswith("/") or "\\" in value or PureWindowsPath(value).drive:
128+
return False
129+
parts = PurePosixPath(value).parts
130+
return bool(parts) and all(p not in {".", ".."} for p in parts)
131+
132+
133+
def _time_attr(value: Any) -> str:
134+
"""Serialize a duration as a JUnit ``time`` attribute string.
135+
136+
The value must be a finite, non-negative number; NaN/inf/negative/non-numeric
137+
all fall back to ``"0.000"``. NaN/inf would otherwise serialize as
138+
``"nan"``/``"inf"`` and make the document invalid for JUnit ingesters. Shared
139+
by the per-testcase time and the root ``<testsuites>`` time so both are
140+
guarded identically.
141+
142+
Rows are untyped, so ``value`` may be a pathologically large JSON integer
143+
(hundreds of digits) that overflows on the ``float()`` conversion / ``.3f``
144+
format — the ``OverflowError`` is caught and degraded rather than aborting
145+
the whole report, matching this module's degrade-don't-crash contract.
146+
"""
147+
if isinstance(value, bool) or not isinstance(value, int | float) or value < 0:
148+
return "0.000"
149+
try:
150+
as_float = float(value)
151+
except (OverflowError, ValueError):
152+
return "0.000"
153+
return f"{as_float:.3f}" if math.isfinite(as_float) else "0.000"
154+
155+
109156
def _load_task_json(run_dir: Path, row: dict[str, Any], variant: str) -> dict[str, Any] | None:
110157
"""Best-effort load of a failed row's ``task.json`` as a plain dict.
111158
@@ -116,7 +163,7 @@ def _load_task_json(run_dir: Path, row: dict[str, Any], variant: str) -> dict[st
116163
``None`` so the caller falls back to a status-only body.
117164
"""
118165
task_id = str(row.get("task_id", "<unknown>"))
119-
if not _is_safe_component(variant) or not _is_safe_component(task_id):
166+
if not _is_safe_component(variant) or not _is_safe_relpath(task_id):
120167
return None
121168

122169
replicate_index = row.get("replicate_index")
@@ -162,11 +209,23 @@ def _criteria_body(row: dict[str, Any], run_dir: Path, variant: str) -> str:
162209
description = str(crit.get("description", ""))
163210
score = crit.get("score")
164211
threshold = crit.get("pass_threshold")
212+
# Only an explicit JSON ``false`` marks an informational criterion; a
213+
# missing key or a schema-skewed value (null / non-bool) fails safe
214+
# to gating — matching CriterionResult.gating's default and this
215+
# module's isinstance-guarded, degrade-don't-crash reads of untyped
216+
# rows. Informational criteria are excluded from the score/gate, so
217+
# they are labelled [INFO] regardless of pass/fail and never rendered
218+
# as the failure cause (mirrors reports.py `_compute_suite_rollup`'s
219+
# `if not cr.gating: continue`).
220+
informational = crit.get("gating", True) is False
221+
score_str = f"{score:.2f}" if isinstance(score, int | float) else str(score)
222+
if informational:
223+
lines.append(f"[INFO] {ctype}: score {score_str}{description}")
224+
continue
165225
passed = isinstance(score, int | float) and isinstance(threshold, int | float) and score >= threshold
166226
if passed:
167227
lines.append(f"[PASS] {ctype}: {description}")
168228
continue
169-
score_str = f"{score:.2f}" if isinstance(score, int | float) else str(score)
170229
thr_str = f"{threshold:.2f}" if isinstance(threshold, int | float) else str(threshold)
171230
lines.append(f"[FAIL] {ctype}: score {score_str} < threshold {thr_str}{description}")
172231
detail = crit.get("details") or crit.get("error")
@@ -193,16 +252,7 @@ def _task_case(row: dict[str, Any], run_dir: Path) -> ET.Element:
193252
task_path = row.get("task_path")
194253
classname = (Path(task_path).stem or variant) if isinstance(task_path, str) and task_path else variant
195254

196-
# time must be a finite, non-negative float: NaN/inf would serialize as
197-
# "nan"/"inf" and make the report invalid for JUnit ingesters.
198-
duration = row.get("duration")
199-
valid_duration = (
200-
isinstance(duration, int | float)
201-
and not isinstance(duration, bool)
202-
and math.isfinite(duration)
203-
and duration >= 0
204-
)
205-
time_str = f"{duration:.3f}" if valid_duration else "0.000"
255+
time_str = _time_attr(row.get("duration"))
206256

207257
case = ET.Element(
208258
"testcase",
@@ -332,7 +382,10 @@ def generate_junit_xml(run_dir: Path) -> str:
332382
summary = RunSummary.model_validate_json(run_json.read_text(encoding="utf-8"))
333383

334384
root = ET.Element("testsuites", {"name": _xml_safe(summary.run_id)})
335-
root.set("time", f"{summary.total_duration_seconds:.3f}")
385+
# Guard the root time identically to per-testcase time: a corrupt/blob-pulled
386+
# run.json can carry a NaN/inf total_duration_seconds (RunSummary has no
387+
# finite validator), which would emit an invalid time="nan".
388+
root.set("time", _time_attr(summary.total_duration_seconds))
336389

337390
# Group task rows by variant, preserving first-seen order.
338391
grouped: dict[str, list[dict[str, Any]]] = {}

tests/lint/doc_env_parity.py

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
"""CE027 — documented framework env vars must be backed by a real consumer.
2+
3+
``coder_eval.config.Settings`` sets no ``env_prefix`` and uses ``extra="ignore"``,
4+
so a documented env var whose name does not match a ``Settings`` field (or one of
5+
its ``AliasChoices``) is **silently dropped** at runtime with zero signal — the
6+
exact failure mode behind the ``CODER_EVAL_API_BACKEND`` doc bug (the real field
7+
is ``API_BACKEND``, so the ``CODER_EVAL_``-prefixed spelling selected no backend
8+
and the run fell back to Direct Anthropic).
9+
10+
This rule scans the doc/config surfaces (``README.md``, ``action.yml``,
11+
``docs/**``) for env-var **assignments** (``NAME=value`` — the copy-pasteable,
12+
dangerous form) carrying a **framework-owned prefix** and flags any whose name is
13+
neither a ``Settings`` env name/alias nor referenced anywhere in ``src/`` — the
14+
framework also reads a handful of vars directly via ``os.getenv`` (e.g.
15+
``CODER_EVAL_SKILLS_DIR``, ``CODEX_BASE_URL``, ``CODER_EVAL_IN_CONTAINER``), and
16+
those are legitimately documentable.
17+
18+
Scope note: only *assignments* are checked, not bare prose mentions. Prose
19+
scanning is too false-positive-prone (markdown links like
20+
``CODEX_AGENT_GUIDE.md``, secret RHS references like ``secrets.BEDROCK_TOKEN``,
21+
regex-pattern examples like ``API_KEY = "…"``), and the assignment form is the
22+
one users actually copy into a workflow, so it carries the real risk.
23+
24+
It is intentionally NOT a ``BaseRule`` registered in ``tests/lint/runner.py``:
25+
that runner is AST-only and walks ``.py`` files, whereas this rule reasons over
26+
Markdown/YAML doc surfaces. It is wired as a dedicated test in
27+
``tests/test_custom_lint.py::TestCE027DocEnvVarParity``.
28+
"""
29+
30+
from __future__ import annotations
31+
32+
import re
33+
from pathlib import Path
34+
35+
from pydantic import AliasChoices
36+
37+
38+
# Framework-owned env-var name prefixes. A documented token starting with one of
39+
# these is owned by coder-eval and MUST be consumed by it. Deliberately EXCLUDES
40+
# broad third-party namespaces (AWS_, ANTHROPIC_, GEMINI_, GITHUB_, EVALBOARD_,
41+
# PLUGIN_) whose vars are consumed by SDKs / CI, not necessarily via Settings, so
42+
# scanning them would produce false positives on legitimately-external names.
43+
FRAMEWORK_ENV_PREFIXES: tuple[str, ...] = (
44+
"CODER_EVAL_",
45+
"API_",
46+
"BEDROCK_",
47+
"CODEX_",
48+
"ANTIGRAVITY_",
49+
"TELEMETRY_",
50+
)
51+
52+
_PREFIX_ALT = "|".join(FRAMEWORK_ENV_PREFIXES)
53+
54+
# A framework-prefixed env-var ASSIGNMENT in doc/config text: ``NAME=`` where NAME
55+
# carries a framework prefix. The negative lookbehind rejects a name embedded in
56+
# a larger token — attribute access (``secrets.BEDROCK_TOKEN``), a hyphenated
57+
# token (``X-API_KEY=``), a path/URL segment (``dir/API_X=``, ``http://API_Y=``),
58+
# or a Windows path (``C:\\API_Z=``). The single ``=`` (not ``==``) with no space
59+
# before it also rejects ``API_KEY = "…"`` regex-pattern examples.
60+
_ENV_ASSIGNMENT = re.compile(r"(?<![\w./:\\-])((?:" + _PREFIX_ALT + r")[A-Z0-9_]*[A-Z0-9])=(?!=)")
61+
62+
# How the framework actually *consumes* an env var, so a documented assignment is
63+
# only "backed" if some module reads it: a direct ``os.getenv("NAME")`` /
64+
# ``os.environ["NAME"]`` / ``os.environ.get("NAME")`` read, or the NAME side of an
65+
# inline ``"NAME=VALUE"`` literal handed to a child process (e.g. docker
66+
# ``--env NAME=1``). This is stricter than "any uppercase literal" so an unrelated
67+
# constant that merely spells a var name cannot silently mask a documented-but-
68+
# unconsumed assignment.
69+
_SRC_ENV_READ = re.compile(r"""(?:getenv\(\s*|environ(?:\.get\(\s*|\[\s*))['"]([A-Z][A-Z0-9_]{2,})['"]""")
70+
_SRC_ENV_VALUE = re.compile(r"""['"]([A-Z][A-Z0-9_]{2,})=[^'"]*['"]""")
71+
72+
73+
def settings_env_names() -> set[str]:
74+
"""Uppercased env names Settings actually reads: field names + AliasChoices."""
75+
from coder_eval.config import Settings
76+
77+
names: set[str] = set()
78+
for field_name, field in Settings.model_fields.items():
79+
names.add(field_name.upper())
80+
alias = field.validation_alias
81+
if isinstance(alias, AliasChoices):
82+
names.update(str(c).upper() for c in alias.choices if isinstance(c, str))
83+
elif isinstance(alias, str):
84+
names.add(alias.upper())
85+
return names
86+
87+
88+
def src_env_literals(src_root: Path) -> set[str]:
89+
"""Env-var names ``src/`` actually consumes: direct ``os.getenv``/``os.environ``
90+
reads plus the NAME side of inline ``"NAME=VALUE"`` child-process literals."""
91+
names: set[str] = set()
92+
for py in src_root.rglob("*.py"):
93+
text = py.read_text(encoding="utf-8")
94+
names.update(_SRC_ENV_READ.findall(text))
95+
names.update(_SRC_ENV_VALUE.findall(text))
96+
return names
97+
98+
99+
def scan_doc_env_assignments(text: str) -> set[str]:
100+
"""Framework-prefixed env-var names *assigned* (``NAME=…``) in a doc file."""
101+
return set(_ENV_ASSIGNMENT.findall(text))
102+
103+
104+
def find_unbacked_env_vars(doc_paths: list[Path], src_root: Path) -> dict[str, list[str]]:
105+
"""Map each doc path to the framework-prefixed env vars it assigns that
106+
nothing in the framework consumes (would be silently dropped at runtime)."""
107+
valid = settings_env_names() | src_env_literals(src_root)
108+
findings: dict[str, list[str]] = {}
109+
for path in doc_paths:
110+
if not path.is_file():
111+
continue
112+
unbacked = sorted(t for t in scan_doc_env_assignments(path.read_text(encoding="utf-8")) if t not in valid)
113+
if unbacked:
114+
findings[str(path)] = unbacked
115+
return findings
116+
117+
118+
def default_doc_paths(repo_root: Path) -> list[Path]:
119+
"""The doc/config surfaces CE027 scans: README, the published action, docs/**."""
120+
paths = [repo_root / "README.md", repo_root / "action.yml"]
121+
docs = repo_root / "docs"
122+
if docs.is_dir():
123+
for suffix in ("*.md", "*.yaml", "*.yml"):
124+
paths.extend(sorted(docs.rglob(suffix)))
125+
return paths

tests/test_custom_lint.py

Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -630,3 +630,80 @@ def test_compliant_class_not_flagged(self):
630630
def test_exempt_modules_not_scoped(self):
631631
for path in ("src/coder_eval/models/results.py", "src/coder_eval/models/telemetry.py"):
632632
assert not self._run(self._VIOLATING, path)
633+
634+
635+
@pytest.mark.lint
636+
class TestCE027DocEnvVarParity:
637+
"""CE027 — documented framework env-var assignments must be backed by a
638+
real consumer (a Settings field/alias or an os.getenv read in src/).
639+
640+
`Settings` sets no `env_prefix` and uses `extra="ignore"`, so a documented
641+
`NAME=value` whose name matches no field is silently dropped at runtime — the
642+
`CODER_EVAL_API_BACKEND` (real field: `API_BACKEND`) doc bug. Scans real
643+
Markdown/YAML surfaces, so it lives here rather than in the AST-only runner.
644+
"""
645+
646+
REPO_ROOT = Path(__file__).parent.parent
647+
648+
def test_repo_docs_have_no_unbacked_env_vars(self):
649+
from tests.lint.doc_env_parity import default_doc_paths, find_unbacked_env_vars
650+
651+
findings = find_unbacked_env_vars(default_doc_paths(self.REPO_ROOT), self.REPO_ROOT / "src")
652+
assert not findings, (
653+
"\nDocumented framework env-var assignment(s) that no Settings field/alias or "
654+
"src/ reference backs — they are silently dropped at runtime (Settings uses "
655+
'extra="ignore"). Fix the spelling to a real env name, or add the consumer:\n\n'
656+
+ "\n".join(f" {path}: {', '.join(names)}" for path, names in sorted(findings.items()))
657+
)
658+
659+
def test_catches_the_coder_eval_api_backend_shadow(self):
660+
# The exact regression: a CODER_EVAL_-prefixed spelling of a real field.
661+
from tests.lint.doc_env_parity import scan_doc_env_assignments, settings_env_names, src_env_literals
662+
663+
valid = settings_env_names() | src_env_literals(self.REPO_ROOT / "src")
664+
found = scan_doc_env_assignments(" CODER_EVAL_API_BACKEND=bedrock\n")
665+
assert "CODER_EVAL_API_BACKEND" in found
666+
assert "CODER_EVAL_API_BACKEND" not in valid # would be flagged
667+
668+
def test_real_backend_field_is_backed(self):
669+
from tests.lint.doc_env_parity import settings_env_names
670+
671+
assert "API_BACKEND" in settings_env_names()
672+
673+
@pytest.mark.parametrize(
674+
"line",
675+
[
676+
"AWS_BEARER_TOKEN_BEDROCK=${{ secrets.BEDROCK_TOKEN }}", # secret RHS, not an assignment of BEDROCK_TOKEN
677+
"[Codex Agent Guide](docs/CODEX_AGENT_GUIDE.md)", # markdown link, not an assignment
678+
'pattern: "^API_KEY = \\"\\\\w+\\""', # regex example with a space before '='
679+
"the CODEX_MODEL setting selects the model", # bare prose mention, not an assignment
680+
"X-API_KEY=v", # hyphenated token — prefix embedded, not a standalone name
681+
"dir/API_THING=v", # path segment — prefix embedded
682+
"http://API_HOST=1", # URL segment — prefix embedded
683+
],
684+
)
685+
def test_non_assignment_shapes_are_not_flagged(self, line: str):
686+
from tests.lint.doc_env_parity import scan_doc_env_assignments, settings_env_names, src_env_literals
687+
688+
valid = settings_env_names() | src_env_literals(self.REPO_ROOT / "src")
689+
unbacked = [t for t in scan_doc_env_assignments(line) if t not in valid]
690+
assert unbacked == [], f"false positive on non-assignment shape: {unbacked}"
691+
692+
def test_name_side_of_env_value_literal_counts_as_backed(self):
693+
# `--env CODER_EVAL_IN_CONTAINER=1` in src makes the doc assignment backed.
694+
from tests.lint.doc_env_parity import src_env_literals
695+
696+
names = src_env_literals(self.REPO_ROOT / "src")
697+
assert "CODER_EVAL_IN_CONTAINER" in names
698+
699+
def test_src_scan_requires_a_real_consumer_not_any_literal(self, tmp_path: Path):
700+
# A bare uppercase constant that no code reads must NOT count as "backed",
701+
# or it could silently mask a documented-but-unconsumed assignment.
702+
from tests.lint.doc_env_parity import src_env_literals
703+
704+
(tmp_path / "m.py").write_text(
705+
'CONST = "CODER_EVAL_BOGUS"\nx = os.getenv("CODER_EVAL_REAL")\n', encoding="utf-8"
706+
)
707+
names = src_env_literals(tmp_path)
708+
assert "CODER_EVAL_REAL" in names # a genuine os.getenv read is backed
709+
assert "CODER_EVAL_BOGUS" not in names # a bare constant is not

0 commit comments

Comments
 (0)