[antonwolfy]

Summary

Follow-up to the review of #80 (point 1). #80 made dokimi/records.py the one reader of answer and eval records, and AGENTS.md states the rule, but nothing enforced it for the next reader. The author agreed a guard needs an allow-list and left it for a follow-up. This is that follow-up.

Changes

• tests/test_records_guard.py counts, per module of dokimi/dokimi/, the reads of the persisted condition keys with_docs and without_docs outside dokimi.records. One pattern does it: an x["with_docs"] subscript not assigned to, or an x.get("with_docs"), .pop or .setdefault call. Assignments and dict-literal keys are writes and don't match.
  • Reads that may stay are in READ_BASELINE, one entry per module with its read count and reason. More reads than the count fail, so a new read fails wherever it is, a listed module included, and the message says to go through dokimi.records. Fewer reads than the count fail as stale, with the count to lower to, so the baseline only shrinks per read.
  • Four modules remain, and none reads a persisted record by its keys:
    • eval.answerer (2) counts the answers it just built;
    • eval.question_text (2) takes the record dict the judges pass as Record.raw;
    • report.generator (6) reads the statistics dict it computed;
    • report.model_compare (17) reads the score rows extract_scores built.
  • Unit tests of the pattern and the counting: reads found, assignments and dict literals not, other keys ignored, the reader module exempt, and a new read, an extra read in a listed module and a stale count each reported.
  • The file is 121 lines. The first version tracked functions with ast (212 lines); per-module counts catch the same reads with one pattern.
• orchestrator/pipeline.py: the run summary (the per-condition and delta means) was the one site that read persisted records by their keys (e["with_docs"]["aggregate"]). It now reads through records() and Record.score / Record.delta. tests/test_pipeline_summary.py pins the numbers, and it passes on both the old code and the new.
• AGENTS.md: the records.py bullet names the test.
• Changelog fragment (Changed).

Risk

• The pipeline summary's numbers are unchanged. One difference: records() raises on an evaluation with no question_id, while the old comprehension skipped such a row. The judge writes question_id on every row, error rows included, so a run does not produce one.
• Out of scope, as the test docstring says: reads through records.CONTEXT, "with_docs" in x tests, and .raw reads such as rec.raw.get("question_text"). A pattern can't tell those apart from other .raw attributes, such as Task.raw in repos.

Testing

On a Linux review host, at 8abd0e2 (on main 506f7ba):

• 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). 7,392 tests passed, 1 skipped; changed-line coverage 100% of 5 lines.
• Planting r.get("with_docs") in dokimi/utils.py fails the guard and names the site. A third read in Answerer.generate_answers, which is in the baseline with 2, fails it as new. Dropping one of that function's two reads fails it as stale.
• Rebased onto main 92d92d3 (test(llm): pin provider-call budgets for judge, panel, answer and validation paths #170, tests only) before opening; with the ten follow-up branches merged together, the full suite (7,447 passed, 1 skipped), the growth check, mypy and ruff pass.
• Review fixes, at 0b3af09: make ci-local 29 passed, 0 failed, 7 skipped; 7,415 tests passed, 1 skipped; changed-line coverage 100% of 5 lines.

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)

[vchamarthi]

@exai-bot /review

[exai-bot]

Note

AI-generated review of 351a19fd1c99, requested by @vchamarthi, 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, 8 of 93 turns, 0.7 of 48 minutes, 202k in / 4k out tokens.

This PR adds an AST-based test that holds readers of with_docs/without_docs outside dokimi.records to a shrinking baseline. It also moves the pipeline run summary onto records(), Record.score and Record.delta, and adds a test that pins the summary numbers. The pipeline change is behaviour-preserving for judge-written rows. The judge already calls records(evaluations, kind="evaluations") on the same list, so the new RecordError path adds no new failure mode. I found no real defects. I did not run anything, but the baseline keys match the read sites I grepped.

1 finding is on the changed lines, as comments below.

For a human reviewer

• Whether the five baseline exceptions are acceptable to keep, mainly eval.question_text.has_answer_to_judge, which reads persisted record dicts.

Not verified

I did not run the tests. That includes whether test_pipeline_summary.py gets through _read_question_inputs and _save_questions_artifact with the minimal cached question file it writes.

[vchamarthi]

Not approving yet: 1 item to fix before merge.

CI: 2 checks pass, 18 are still pending, 1 skipped at 351a19f. The pipeline change is correct; one point on the guard: it lets new reads through in the five baselined functions.

CI at send time: 0 failing, 12 pending.

[codecov-commenter]

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

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.98%. Comparing base (92d92d3) to head (0b3af09).
⚠️ Report is 14 commits behind head on main.
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #203      +/-   ##
==========================================
+ Coverage   91.78%   91.98%   +0.20%     
==========================================
  Files         223      223              
  Lines       27623    27711      +88     
  Branches     4549     4553       +4     
==========================================
+ Hits        25354    25491     +137     
+ Misses       1606     1572      -34     
+ Partials      663      648      -15     

Files with missing lines	Coverage Δ
dokimi/dokimi/orchestrator/pipeline.py	80.66% <100.00%> (+1.26%)	⬆️

... and 15 files with indirect coverage changes

🚀 New features to boost your workflow:

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

[exai-bot]

Note

AI-generated review of 351a19fd1c99, requested by @vchamarthi, 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, 7 of 93 turns, 0.6 of 48 minutes, 161k in / 3k out tokens.

Re-review of 351a19fd1c99: 1 new finding.

1 finding is on the changed lines, as comments below.

[exai-bot]

Note

AI-generated review of 1b1e718ef26b, requested by @vchamarthi, 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, 10 of 93 turns, 0.8 of 48 minutes, 214k in / 4k out tokens.

Re-review of 1b1e718ef26b: nothing new.

Coverage

• Held back 1 finding the verifier refuted.

Held back 1 finding

Grade	Category	Why held	Finding	Where	Evidence
-	-	refuted	Summary step can now abort the run with RecordError after the expensive steps	dokimi/dokimi/orchestrator/pipeline.py:384	The pipeline's evaluations come straight from judge.evaluate_answers (pipeline.py:376, 647-686). judge.py ends that method with records(evaluations, kind="evaluations") over the same list, so a ma [cut]

[napetrov]

The pipeline summary move onto dokimi.records and its test look good. Question on the guard: 283 lines of AST scanning plus a baseline of 27 reads (5 entries, 15 in generate_section_report alone) to hold one naming rule, and the docstring already lists what it misses (reads via the constants, in tests, .raw). Is this weight worth it versus a simpler grep-style check alongside test_import_architecture.py? Alternatively, move the remaining readers first so the baseline is near-empty and the guard stays small.

[antonwolfy]

@napetrov fair. 0b3af09 replaces the AST scanner with one pattern: an ["with_docs"] subscript not assigned to, or a .get/.pop/.setdefault("with_docs") call, counted per module.

• It finds the same 27 reads on the same lines.
• A new read still fails wherever it is, in a listed module too, since that module's count goes up. A count above what is left fails as stale.
• The baseline is four modules (2, 2, 6, 17), and the file goes from 212 to 121 lines.
• With per-module keys, refactor(report): split generate_section_report into per-section helpers (complexity 53 -> 4) #169's split of generate_section_report no longer touches the baseline.

On moving the readers first: the 17 in report/model_compare.py read the per-question score rows that extract_scores builds itself, not stored records. Renaming those keys would change the report's own data without protecting anything more, so they stay listed with that reason.

[exai-bot]

Note

AI-generated review of 0b3af098b8ca, requested by @vchamarthi, 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, 6 of 93 turns, 0.6 of 48 minutes, 148k in / 3k out tokens.

Re-review of 0b3af098b8ca: 1 new finding.

1 finding is on the changed lines, as comments below.

[exai-bot]

Nit (plausible, maintainability): Pattern also counts reads quoted in comments and docstrings

READ runs over raw source text, so a comment or docstring that quotes r.get("with_docs") counts as a read. Adding one in a listed module fails the guard as 'too many', and removing one fails it as 'stale'. Both are harmless and the message makes the cause findable. The test docstring could say that comments count, or the scan could strip comments first.

[napetrov]

Confirmed in 0b3af09: per-module counts, same 27 reads, 121 lines, stale-count check kept. The model_compare exception is reasoned. LGTM.