[antonwolfy]

Summary

Follow-up to the review of #189, where wrong-shaped contents inside a result file were left for a follow-up (review, reply). Given as --pairwise, {"verdicts": {"skill": {"q1": {}}}} ends in KeyError: 'score'.

Other shapes of --pairwise and --checklist files end the same way. The traceback also comes after --judges has made its paid calls. The measures were built only after the judge loop, although the comment above the file reads says a bad file costs no paid calls.

On main, through eval cross-judge --no-stored:

File	Result
{"verdicts": {"skill": {"q1": {}}}}	KeyError: 'score'
{"verdicts": {"skill": {"q1": 5}}}	TypeError
{"verdicts": {"skill": {"q1": {"score": "x"}}}}	ValueError
{"verdicts": {"skill": 5}}	AttributeError
{"questions": [5]}	AttributeError
{"questions": [{"arms": {"skill": {"coverage": 1}}}]}	KeyError: 'question_id'
a question with "coverage": "x"	exit 0 alone; TypeError once another measure is compared with it

Changes

• cross_judge.pairwise_measure and coverage_measure raise ValueError naming the first bad entry, for example verdicts.skill.q1 has no score that is a finite number or questions[1] is not a checklist question: .... A score and a coverage go through _number: a finite JSON number, not a bool or a string, and an integer past a float's range is caught too. A checklist question needs a question_id that is not null or a bool, and an arms object, before its arms are read.
• cmd_eval_cross_judge builds both measures when it reads the files, before the first judge call, through _result_measure. A bad entry prints Error: <file>: <entry> ... and exits 2, the code an unreadable file gets since fix(eval): cross-judge exits 2 on an unreadable input file #189. The measures keep their place in the report.
• cmd_eval_cross_judge goes from complexity 32 to 30, and its growth entry follows.
• Changelog fragment (Fixed).

Risk

Files these two readers took before and now refuse: a checklist coverage that is not a finite number or null (before, it passed only while nothing was compared with it, and crashed otherwise), a pairwise score that is a numeric string, a bool, NaN or infinite, and a checklist question without arms or with a null question_id. eval pairwise writes an int score and eval checklist a question id, an arms object and a float or null coverage, so no file they wrote reads differently. Valid files give the same report, and the existing cross-judge tests pass unchanged.

Testing

On a Linux review host, at cb23dd5 (on main 54ce188):

• make ci-local: 29 passed, 0 failed, 7 skipped (the Docker catalog smoke, test-latest, the cli-surface packaging step, conda-forge-deps without micromamba, docs deploy, the pip-audit report, semgrep). 8,224 tests passed, 2 skipped; changed-line coverage 100%.
• The cases from the reviews of 0494bac (a numeric string, bool, NaN, infinite or too large score; a null, bool or missing question_id or arms; a bool, NaN or too large coverage) fail at 0494bac (12 failed) and pass here; a checklist arm with a null coverage still reads. 11 mutants of that fix each fail a test: _number taking a bool, a string or a non-finite value, OverflowError uncaught for a score or a coverage, a null or bool question_id taken, a missing arms skipped, a null coverage refused, the coverage unchecked, and AttributeError uncaught.
• Merged into main 59de48a: clean, and the growth check passes.
• test_cross_judge_names_a_result_entry_of_the_wrong_shape_before_any_judge_call (11 cases, in tests/test_record_readers.py: tests/test_judge_reliability.py is at its growth cap) runs with --judges and a recording judge. Each case exits 2 with the entry in the message, and no judge call is made. With main's code all 11 fail. They also all fail with only the new measure functions, because the judges were still called first.
• 8 mutants each fail a test: the arm-object check off, KeyError or ValueError dropped from the verdict catch, the coverage type check off, AttributeError or KeyError dropped from the question catch, _result_measure not catching, and the file measures not added to the report.
• With the four follow-up branches merged together on main 54ce188 (0a5717c): the full suite (8,232 passed, 2 skipped), the growth check, mypy and ruff pass.

Checklist

• Tests added/updated and pytest passes
• ruff, mypy, and schema checks pass
• Docs updated (or N/A) — see the docs-drift checklist in CONTRIBUTING.md
• No ad-hoc run artifacts committed
• New files carry the SPDX license header (or are exempt in .github/.licenserc.yaml)

[codecov-commenter]

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

❌ Patch coverage is 96.96970% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 92.95%. Comparing base (54ce188) to head (cb23dd5).
⚠️ Report is 19 commits behind head on main.

Files with missing lines	Patch %	Lines
dokimi/dokimi/commands/judge_reliability.py	90.00%	0 Missing and 1 partial ⚠️
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #233      +/-   ##
==========================================
+ Coverage   92.25%   92.95%   +0.69%     
==========================================
  Files         239      241       +2     
  Lines       29937    30672     +735     
  Branches     4918     5060     +142     
==========================================
+ Hits        27618    28510     +892     
+ Misses       1632     1460     -172     
- Partials      687      702      +15     

Files with missing lines	Coverage Δ
dokimi/dokimi/eval/cross_judge.py	99.53% <100.00%> (+0.05%)	⬆️
dokimi/dokimi/commands/judge_reliability.py	92.79% <90.00%> (+3.16%)	⬆️

... and 43 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[antonwolfy]

@exai-bot /review

[exai-bot]

Note

AI-generated review of 0494bac8a7a2, requested by @antonwolfy, using the default review skill. It can be wrong: verify a finding before acting on it. React with a thumbs up or down on a finding to tell the bot whether it helped.
Model us.anthropic.claude-sonnet-5-5, effort default, 2 of 80 turns, 0.2 of 40 minutes, 34k in / 1k out tokens.

The change moves construction of the --pairwise and --checklist measures ahead of the judge loop. pairwise_measure and coverage_measure now raise ValueError naming the first bad entry, and _result_measure turns that into Error: <file>: <entry> ... with exit 2. I found no defects. The exception catches cover the wrong shapes listed in the description, and the report keeps its measure order. The test runs 11 wrong-shaped files with a recording judge and asserts both exit 2 and that no judge call was made.

No issues found.

[napetrov]

Reviewed at 0494bac. No fixes pushed.

Evidence on the PR head: pytest tests/test_record_readers.py tests/test_judge_reliability.py 222 passed; ruff check . passes; scripts/check_growth.py passes. I also probed pairwise_measure / coverage_measure directly with extra shapes beyond the 11 test cases.

Open items (low severity, not blocking; both pre-existing behaviour, not introduced by this PR):

1. {"score": "nan"} is accepted and stored as nan (float("nan") succeeds), and a checklist coverage of NaN is accepted too. A NaN score will propagate into the agreement statistics rather than exit 2. If "numeric" is meant as finite, a math.isfinite check would close it; otherwise fine to leave.
2. coverage: true passes the new type check (bool is an int) and is stored as True; score: true becomes 1.0. Probably harmless, mentioned only because the new check reads as a strict number check.

Confirmed working: a non-object verdicts / questions of the wrong kind is still stopped earlier by _load_json_object, and arms that is not an object now gives questions[i] ... with exit 2.

[napetrov]

Follow-up to the earlier review at 0494bac (no new code findings).

Land-time note: dokimi/scripts/growth_baseline.json is also changed by #235, #236 and #239. Whichever of these lands later will conflict on it or carry a stale count, so the baseline should be regenerated on the final base at merge time rather than hand-merged.

[vchamarthi]

Approved. 1 non-blocking comment.

No blockers. The malformed-entry check now runs before the first judge call and the measure order is unchanged. One test-id nit.

• Nit: test_record_readers.py:153 uses the id arm-not-an-object twice; rename the checklist one (e.g. arm-entry-not-an-object) so the ids stay distinct.

  ids at lines 153-155 contains "arm-not-an-object" at positions 5 and 9, for different flags and shapes. Pytest suffixes duplicates, so it runs, but -k selection by id is ambiguous. (dokimi/tests/test_record_readers.py:154)

CI at send time: 0 failing, 0 pending.

[napetrov]

Approving at 0494bac: reviewed, CI green, no blocking items open.

[antonwolfy]

@vchamarthi the duplicate test id is now arm-entry-not-an-object (cb23dd5). @napetrov your NaN and true notes are fixed in the same commit; see the thread on cross_judge.py:263. #236 has landed since your land-time note: this branch still merges cleanly with main, and the growth check passes on the merge.