Skip to content

feat(repo): replay grades record each credited hit; repo report --hits lists them - #297

Merged
napetrov merged 13 commits into
intel:mainfrom
antonwolfy:feat/replay-hit-pairs
Oct 30, 2026
Merged

napetrov merged 13 commits into
intel:mainfrom
antonwolfy:feat/replay-hit-pairs

Conversation

@antonwolfy

@antonwolfy antonwolfy commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Found while evaluating IntelPython/dpnp#3099 (AGENTS.md guidance) with dokimi: 26 dokimi:review_replay tasks, 156 cells, 210 credited thread hits. Two of the evaluation's readings could not be checked from the batch.

A credited hit names its thread, not the finding that hit it. grade_replay credits a thread to the nearest finding on the same file within window lines, one to one. That is by design: location only, no judge. But grade.json keeps only details.threads_hit, the thread ids. To check a hit, the dpnp audit had to rerun the matching by hand next to gold.json and answer.txt. Doing that turned up hits credited by chance, which the report had quoted as evidence that the guidance works:

task gold thread credited finding
pr3098-t2 asks to add a TODO complains that the TODO is repeated five times and has no issue URL
pr3066-t1 (two credited findings) one of them is about something else
pr2841-t43 credited, in the report, to a rule from a file the run never opened

The grade.json of the cell that credited pr3098-t2 (guidance:rev:<sha>, rep 2), on main:

metrics: {"n_findings": 5, "n_severe": 2, "n_threads": 2, "recall_all": 1.0, "recall": 1.0, "recall_addressed": null, "precision_proxy": 0.4}
details: {"threads_hit": ["pr3098-t1", "pr3098-t2"], "recall_by_kind": {"unknown": "2/2"}, "decoys_flagged": [], "grader_version": "1"}

Nothing in it says which finding got the credit, or what it said.

Parse errors are not counted per arm. In pr3014, arm guidance:none, rep 2, the answer's findings JSON was {"file": "dpnp/dpnp_iface_indexing.py", "line": 1:1, ...}, which does not parse. That cell is graded parse_error: true with every recall 0, as contract §4.5 says: the agent failed the task. But nothing outside the grader reads parse_error, so repo report shows only the lower score. On main, repo report of a batch holding that cell (and the six pr3098 cells) gives its row as:

task                  arm                                                    model                             n  result                  grader_fail  missing      cost   turns
review_replay_pr3014  guidance:none                                          us.anthropic.claude-sonnet-5-5    1  score 0.00                        0        0    0.1154     4.0

A treatment that makes answers harder to parse would read as a recall loss.

Related: #291 proposes a judged recall over these pairs.

Changes

  • grade_replay (repos/graders/review.py) adds details.matches: one object per credited pair, in threads_hit order. Each holds the thread (thread, thread_file, thread_line, comment), the finding (finding = its index among the answer's objects, file repository-relative, line as matched, severity, message) and distance. Strings are cut to 200 characters. _replay_pairs carries the thread along; the sort key, the matching and every metric are unchanged. The module is still standard library only and parses as Python 3.8. On the same cell, this branch writes, among its two matches:
    {"thread": "pr3098-t2", "thread_file": ".github/workflows/conda-package.yml", "thread_line": 54, "comment": "The conda-forge matrix (`conda-package-cf.yml`) still covers 3.12–3.14, while the Intel matrix now covers 3.13–3.15\nSince conda-forge dpctl 0.22.1 doesn't support Python 3.15 yet, could we add a TODO ", "finding": 4, "file": ".github/workflows/conda-package.yml", "line": 54, "severity": "minor", "message": "The TODO about missing cp315t builds has no full issue URL.", "distance": 0}
    
    The thread asks for a TODO; the finding that got its credit says an existing TODO lacks an issue URL.
  • VERSION stays "1": its rule is "bump when a result for the same inputs changes", and no metric or pass changes. graders_digest() hashes the grader sources, so it changes with this PR anyway. A batch pinned with the old digest refuses to resume (_pin_batch), and repo compare warns when compared batches differ in graders. So old and new grades are not mixed silently. A reader tells them apart by whether matches is present.
  • repo report --hits (commands/repo_run.py; load_hits, format_hits in repos/report.py) prints each credited pair instead of the table, grouped by task, then arm/model/rep. Each pair takes two lines: the thread id, its anchor and the gold comment, then the finding's index, location, severity, distance and message. Text is flattened to one line and cut to 100 characters. A cell is a replay cell when its grade.json names dokimi:review_replay. A replay cell whose result.json says grader_failure lists no hit, since the table scores it 0. A cell graded before matches existed is counted on one line (N cell(s) graded without hit details), not treated as an error. A batch with no replay cell prints <batch>: no dokimi:review_replay cell. --hits --json gives {batch, replay_cells, without_matches, hits}, one record per pair in hits (task, arm, model, rep, cell and the match fields). Those five come from the cell's result.json: a match key of the same name does not replace them (5210ebe). Without --hits the output is unchanged. On those seven cells (excerpt):
    <batch>: 8 credited hit(s) in 7 dokimi:review_replay cell(s)
    review_replay_pr3098
    ...
      guidance:rev:d8fb2838c7da9c73ae6e68048955ed5837977b11  us.anthropic.claude-sonnet-5-5  r2
        pr3098-t1 .github/workflows/conda-package.yml:33  This also removes Python 3.12 from all matrices here and in `cron-run-tests.yaml` including uploa...
          finding 0 .github/workflows/conda-package.yml:33 major, 0 line(s) away: Python 3.12 is removed from the test matrices (here and at the other three matrices in this file)...
        pr3098-t2 .github/workflows/conda-package.yml:54  The conda-forge matrix (`conda-package-cf.yml`) still covers 3.12–3.14, while the Intel matrix no...
          finding 4 .github/workflows/conda-package.yml:54 minor, 0 line(s) away: The TODO about missing cp315t builds has no full issue URL.
    
  • The default repo report notes, under a row, how many of its cells have metrics.parse_error true. The note goes in the row's existing notes line, so it reads as the agent's failure, not the grader's; on the pr3014 row above, a score-only row, it adds 1 agent answer(s) whose findings JSON did not parse (scored 0): the cell is in the row's mean as a score of 0. A gate row says (counted as failed) instead. A cell whose result.json says grader_failure is counted under grader_fail, not as a parse error. --json gives parse_errors on every row. A batch with no such cell prints nothing new. The row notes moved from format_report into _row_notes.
  • repo report reads each cell's grade.json (for parse_errors, and for --hits) through grading.read_grade_json, the reader read_grade uses, now factored out of it (9092ed4). A link, FIFO or other non-regular file is not read (lstat, then O_NOFOLLOW), and a file nested too deep to parse is no grade, as are invalid JSON and a non-object. So one such file left in a cell no longer stops, hangs or misleads the report of the whole batch. read_grade gets the RecursionError case too: such a cell is now a grader failure (grade.json is not JSON: ...) where its grading used to raise.
  • After fix(cli): run, compare, arms, tasks, explore and repo exit with a message on bad input or output #238, repo report refuses a result.json of the wrong shape with a ✗ message naming it (exit 1). --hits also sorts by rep, which the table never reads, so a cell without it stopped --hits with a KeyError traceback. load_hits now refuses it the same way, with ✗ <cell>/result.json has no usable \rep`, exit 1 (0a7539a). test_hits_refuse_a_cell_with_no_rep_as_the_table_refuses_a_malformed_cellfails without the check (theKeyError`) and passes with it.
  • --hits is a new spelling for the flag registry of test(cli): flag registry, ratchet contract test, CLI snapshot (#94 F1a) #254: tests/data/cli_flags_allowlist.txt lists it (no registry concept names a view switch), and docs/reference/cli-interface.json is regenerated with it (29aff4f).
  • Contract §4.5 (the details column and a bullet on the matches fields), §9 (--hits and the parse-error count), and the grader and report module docstrings.
  • Changelog fragment (Added).

Risk

  • grade.json of a dokimi:review_replay cell gains details.matches, up to two 200-character strings per credited thread. details is stored and never aggregated (repo_grade.v1 allows any object there). No metric changes.
  • repo report --json rows gain parse_errors. The text table gains a note only on rows that have a parse error.
  • A grade.json nested too deep to parse now makes its cell a grader failure, where its grading raised RecursionError.
  • The grader sources change, so graders_digest() changes. A batch started before this PR with built-in graders refuses to resume (batch ... was run from graders ...; use another --batch), as with any grader change. repo compare across such batches warns about the graders stamp.

Testing

On a Linux review host, at 3d19b8f (on main e966422):

  • make ci-local: 27 passed, 1 failed, 7 skipped. The failure is pip-audit, which reports 10 known CVEs against filelock 3.32.7 in requirements-ci.lock.txt. This PR does not touch that file, and main has the same pin; ci(deps): document and ignore ten more filelock 3.32.7 advisories #288 documents and ignores those advisories. Skipped: external-catalog-smoke (needs docker), test-latest, docs.yml:deploy and pip-audit.yml:report (job conditions false on a PR), cli-surface / Package the result, conda-forge-deps (no micromamba), semgrep secrets (runs on the PR). 10191 tests passed, 4 skipped; changed-line coverage 100% (74 lines).
  • tests/test_repo_builtin_graders.py::test_replay_matches_record_each_credited_pair_nearest_first: a two-thread, three-finding answer (plus one non-object entry). The finding at line 4 is near both threads and takes t_doc (distance 0). t_op then goes to the finding at distance 1, not to the earlier one at distance 4. The test checks the full matches, their threads_hit order when credit order differs, the 200-character cut and the repository-relative file. It also checks that the entry script writes them under details and that the result passes check_grade.
  • tests/test_repo_report_hits.py (new): a batch graded by the real grader, with two replay tasks, three arm/rep cells, an old-format grade, the dpnp parse-error answer ("line": 1:1) and a review_seeds cell. It checks --hits text and JSON, the graded without hit details count, the CLI flag, that the default table is unchanged, the no-replay-cell batch, and the per-row parse-error count and note (not a grader failure).
  • Against main's code: the new grader test fails (no matches). The report test file fails to import (format_hits does not exist). The other 48 tests of test_repo_builtin_graders.py pass there.
  • Mutants, each in a fresh copy of the commit; each one makes its test fail (13 of 13):
    • grader: drop the distance from the nearest-first sort; matches in credit order instead of threads_hit order; no 200-character cut; the finding's raw file instead of the repository-relative one;
    • report: count parse errors as grader failures; drop the parse-error note;
    • --hits: count a parse-error cell as graded without hit details; take cells of every grader; drop the arm/model/rep header; don't cut a long message; drop the "no dokimi:review_replay cell" line;
    • CLI: ignore --hits; print the hits in place of the default table.
      An unmutated copy passes the same tests (53 passed).
  • Bad inputs: a missing batch directory exits 1 (as before). An unreadable or non-object grade.json, details: null, and non-object entries in matches are skipped without an error. A non-boolean parse_error is not counted. Review round 1 (8e43a2b) turns these into a test, which dropping any of the six guards makes fail.
  • The real cells: the six pr3098 cells and the pr3014 parse-error cell of the dpnp batch, regraded with this branch's grader. Every metric equals the original grade.json's, and repo report / repo report --hits give the output quoted above.
  • At 8e43a2b: make ci-local passes apart from pip-audit (the same filelock advisories) and the two gitleaks steps. The host's proxy answers GitHub release downloads with 504, so CI's gitleaks install fails there; the installed gitleaks 8.30.1 finds no leaks in the PR's commits.
  • main merged in after fix(cli): run, compare, arms, tasks, explore and repo exit with a message on bad input or output #238 (f0fe902). The conflicts were in summarize, where the parse_errors count moved into fix(cli): run, compare, arms, tasks, explore and repo exit with a message on bad input or output #238's reading_cells block, and in cmd_repo_report, where --hits now runs inside the same CellFileError handling as the table. Then 0a7539a (below). On that commit, the report, grader, run and compare tests with the guard tests: 578 passed; ruff and mypy are clean.
  • Merged together on main f0f95a2 with the other PRs from this evaluation (fix(repo): scan says why it is incomplete #295 at 4fa7783, feat(repo): compare records the start context, filters tasks, and flags inconclusive rows #296 at cad52dd, feat(repo): replay grades record each credited hit; repo report --hits lists them #297 at 0a7539a, fix(novelty): a capped run says how far the cap reached #299 at fa453d8), on a workstation: the full suite has 10482 passed and 57 skipped, and the same 5 failures and 1 collection error as main alone there (10456 passed). All six come from that machine's setup: no docker, no hypothesis, and two tests that depend on its git and shell settings.
  • Review comments 4416180649 and 4416180698, 89f282a (test(repo): pin "" for missing match text and rep order of --hits). On a Linux review host, at 89f282a (on main f0f95a2). Both mutants the comments named survived at 0a7539a; the tightened tests now catch them, 3 of 3:
    • test_replay_matches_record_each_credited_pair_nearest_first also grades a gold thread with no comment against a finding with only file and line, and expects a literal match with comment, severity and message all "". It fails when _cut turns None into "None".
    • test_hits_list_each_credited_pair_by_task_arm_and_rep adds a rep-10 cell next to rep 2; by directory name r10 comes first. The test fails when load_hits drops its sort, and when it sorts the rep as a string.
    • make ci-local: 25 passed, 3 failed, 7 skipped. None of the failures comes from this change. Install gitleaks: the download returned HTTP 504. Scan commits then had no gitleaks binary. pip-audit reports 10 known CVEs against filelock 3.32.7 in requirements-ci.lock.txt, the same pin as on main. Skipped: external-catalog-smoke (needs docker), test-latest, docs.yml:deploy and pip-audit.yml:report (job conditions false on a PR), cli-surface / Package the result, conda-forge-deps (no micromamba), semgrep secrets (runs on the PR). 10525 tests passed, 4 skipped; changed-line coverage 100% (78 lines).
  • Review comments 4418950452, 4418950465, 4418950473, 4418950480, 4420760178 and 4420760207: 9092ed4 (fix(repo): report reads grade.json as the host does; --hits skips grader failures), 414855f (fix(repo): parse_errors skips grader failures; score rows say scored 0), 6421d62 (docs(cli): repo report --hits help says by task, arm, model and rep), e53ab20 (test(repo): FIFO grade test skips without mkfifo and fails on a block). On a Linux review host, at e53ab20 (on main f0f95a2):
    • New tests: test_parse_errors_leave_out_a_cell_the_table_counts_as_a_grader_failure (a parse-error grade in a grader-failure cell and in a graded cell: grader_failures 1, parse_errors 1) and test_the_parse_error_note_of_a_gate_row_says_counted_as_failed. test_report_counts_answers_that_did_not_parse_per_row now expects (scored 0) on the replay row. The FIFO test is skipped without os.mkfifo, and it records whether the first join timed out, unblocks every waiting read (bounded), then asserts it did not.
    • The test file copied onto 9092ed4: the new parse_errors test and the changed note test fail (2 failed, 9 passed); at e53ab20, 11 passed.
    • 9092ed4 checked by reverting it in pieces: _grade_doc back on read_text fails the nested/linked grade test and the FIFO test; read_grade_json without the RecursionError catch fails the nested grade test; load_hits without the grader_failure skip fails test_hits_credit_no_match_of_a_cell_the_table_scores_as_a_grader_failure (3 of 3).
    • Mutants of this round, each in a fresh copy of e53ab20; each one makes its test fail (6 of 6): parse_errors counts grader-failure cells again; the note's kind check flipped; the note always (counted as failed); the note always (scored 0); only load_hits reads grade.json with read_text (at 9092ed4 the FIFO test passed with this mutant; now it fails with "a grade.json that is a FIFO blocked the report"); the table and --hits both read it with read_text (the test file fails in about 11 s: the cleanup loop ends both blocked reads, where the old cleanup left the second one waiting and took 22 s). An unmutated copy passes (11 passed).
    • make ci-local: 27 passed, 1 failed, 7 skipped. The failure is pip-audit: 10 known CVEs against filelock 3.32.7 in requirements-ci.lock.txt, which this PR does not touch; main ignores them since ci(deps): document and ignore ten more filelock 3.32.7 advisories #288, which this branch predates, and CI tests the merge with main. Skipped: external-catalog-smoke (needs docker), test-latest, docs.yml:deploy and pip-audit.yml:report (job conditions false on a PR), cli-surface / Package the result, conda-forge-deps (no micromamba), semgrep secrets (runs on the PR). 10530 tests passed, 4 skipped; changed-line coverage 100% (86 lines). The skipif was not run on Windows.
  • Merge with main (test(cli): flag registry, ratchet contract test, CLI snapshot (#94 F1a) #254, ci: fail comment blocks over two lines that a PR adds or edits #298) and review comment 4422546939: 29aff4f (test(cli): allowlist repo report --hits and refresh the CLI snapshot), 5210ebe (fix(repo): repo report --hits keeps the cell's key over a match's). On a Linux review host, at 5210ebe (on main 83f18fb):
    • --hits has an allowlist row with its reason (no registry concept names a view switch), and docs/reference/cli-interface.json is regenerated; the snapshot diff is the --hits option only. tests/test_cli_flags.py: 2 failed at 040508d (--hits neither canonical nor allowlisted; the snapshot is stale), 16 passed at 29aff4f. scripts/check_comment_blocks.py --base origin/main --head HEAD finds no comment block over two lines among the lines the PR adds or edits.
    • New test test_a_match_key_named_like_the_cell_key_does_not_move_the_hit grades a cell whose match carries task, arm, model, rep and cell keys and checks that --hits --json and the --hits text both show the cell's own values. Copied onto 040508d, it fails (the hit is listed under other-task, guidance:raw, m-9, rep 99; 1 failed, 11 passed); at 5210ebe, 12 passed. A mutant that puts only rep back in front of the match also fails it (1 of 1).
    • make ci-local: 28 passed, 2 failed, 7 skipped. Neither failure comes from this change: Install gitleaks got HTTP 504 on the download, so Scan commits had no gitleaks binary. A gitleaks already on the host scanned the branch's commits instead: no leaks found. Skipped: external-catalog-smoke (needs docker), test-latest, docs.yml:deploy and pip-audit.yml:report (job conditions false on a PR), cli-surface / Package the result, conda-forge-deps (no micromamba), semgrep secrets (runs on the PR). 10652 tests passed, 4 skipped; changed-line coverage 100% (86 lines). An earlier run at the same commit also failed test_reference_checks.py::test_the_regex_limit_is_shared_by_every_pattern_of_one_check, a wall-clock bound (1.55 s against 1.5 s) in a file this PR does not touch; it passes alone and in the second full run.
    • Not yet run merged together with the other follow-up branches.

Revert

A plain revert. grade.json files written meanwhile keep details.matches, which nothing else reads. Batches started after this PR are pinned to its graders digest, so resuming them after a revert asks for another --batch.

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
  • Self-reviewed per "Before opening a PR" in AGENTS.md
  • No ad-hoc run artifacts committed
  • New files carry the SPDX license header (or are exempt in .github/.licenserc.yaml)

A dokimi:review_replay grade kept only the ids of the threads it
credited (details.threads_hit), not which finding hit each one. To check
a hit, the evaluation of IntelPython/dpnp#3099 had to rerun the
location matching by hand next to gold.json and answer.txt, and that is
how it found credits by chance: a finding on the right lines about
something else. Separately, an answer whose findings JSON does not parse
is graded parse_error with every recall 0, but no summary showed it, so
an arm that makes answers harder to parse read as a recall loss.

- grade_replay adds details.matches: per credited pair, in threads_hit
  order, the thread (id, file, line, comment cut to 200 characters), the
  finding (its index, repository-relative file, line as matched,
  severity, message cut to 200) and their distance. The matching and
  the metrics are unchanged, so VERSION stays 1; the graders digest a
  batch is pinned to changes with the source as before.
- repo report --hits lists those pairs by task, arm, model and rep, two
  lines each (--json: records with task, arm, model, rep and cell),
  counts the cells graded before matches existed, and says so when the
  batch has no review_replay cell. The default report is unchanged.
- repo report counts, per row, the cells whose grade.json has
  parse_error, as a note under the row ("agent answer(s) whose findings
  JSON did not parse") and as parse_errors in --json.
- Contract §4.5 and §9, the grader docstring and a changelog fragment.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
@github-actions github-actions Bot added the cli-interface-change Changes dokimi's user-facing CLI surface label Oct 29, 2026
@github-actions

Copy link
Copy Markdown

CLI interface change detected

This PR changes dokimi's user-facing CLI surface (commands and/or options):

~ `dokimi repo report` parameters changed:
    + added option `--hits`

Updated on every push; the cli-interface-change label is on while the diff is non-empty.

@vchamarthi

vchamarthi commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

✅ OpenCodeReview: Review complete: 0 finding(s) across 7 selected item(s).

Review details
  • Commit reviewed: 89f282a against merge base f0f95a2
  • Coverage: 7 files in 3 groups
  • Engine: OpenCodeReview v1.13.3, us.anthropic.claude-sonnet-5-5
  • Usage: 404k tokens (299k from cache), 34s
  • Tool calls: 24 (17 code_search, 6 file_read, 1 file_read_diff)
  • Replay hit pairs: grader, report, command logic: 3 files
  • Tests for hit pairs in graders and reports: 2 files
  • Docs and changelog for replay hit pairs: 2 files

Comment thread dokimi/dokimi/repos/graders/review.py
Comment thread dokimi/tests/test_repo_report_hits.py
@codecov-commenter

codecov-commenter commented Oct 29, 2026 •

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.57%. Comparing base (83f18fb) to head (5210ebe).
⚠️ Report is 2 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     #297      +/-   ##
==========================================
+ Coverage   93.52%   93.57%   +0.04%     
==========================================
  Files         274      275       +1     
  Lines       35625    35772     +147     
  Branches     6052     6066      +14     
==========================================
+ Hits        33320    33475     +155     
+ Misses       1489     1481       -8     
  Partials      816      816              
Files with missing lines Coverage Δ
dokimi/dokimi/commands/repo_run.py 94.04% <100.00%> (+0.07%) ⬆️
dokimi/dokimi/repos/graders/review.py 100.00% <100.00%> (ø)
dokimi/dokimi/repos/grading.py 91.45% <100.00%> (+1.15%) ⬆️
dokimi/dokimi/repos/report.py 97.94% <100.00%> (+0.48%) ⬆️

... and 4 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 antonwolfy self-assigned this Oct 29, 2026
…tches

A field added to details that leaves every metric and pass as they were
does not bump VERSION; a reader tells such a grade by the field being
present.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Covers a grade.json that is not JSON or not an object, details that is
not an object, non-object and textless entries in matches, metrics that
is not an object and a non-boolean parse_error.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Comment thread dokimi/tests/test_repo_builtin_graders.py
Comment thread dokimi/tests/test_repo_report_hits.py Outdated
Resolve the conflicts with intel#238: summarize keeps the parse_errors count
inside the new reading_cells block, and cmd_repo_report runs --hits
inside the same CellFileError handling as the table.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
After intel#238, repo report refuses a result.json of the wrong shape with a
CellFileError naming it (exit 1). load_cells checks task, arm and model;
--hits also sorts by rep, which the table never reads, so a cell without
it stopped --hits with a KeyError traceback. load_hits now refuses it
the same way: "<cell>/result.json has no usable `rep`".

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Review comment 4416180649: test_replay_matches_record_each_credited_pair_
nearest_first now grades a thread with no `comment` against a finding
with no `severity` or `message` and compares the match with a literal
whose three fields are "". Before, every input supplied those strings,
so `_cut` could turn None into "None" and the suite still passed.

Review comment 4416180698: test_hits_list_each_credited_pair_by_task_
arm_and_rep adds a replay cell with rep 10 next to rep 2. By directory
name r10 sorts before r2, so the expected listing, rep 1, 2, then 10,
now fails if load_hits drops its (task, arm, model, rep) sort or sorts
the rep as a string.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
@antonwolfy

Copy link
Copy Markdown
Contributor Author

Review round in 89f282a, tests only. Both newer OpenCodeReview points held: a mutant survived each at 0a7539a, and both are caught now.

  • _cut keeps "" for a thread with no comment and a finding with no severity or message, not "None".
  • --hits follows rep order, pinned with a rep-10 cell.

The two earlier threads were answered in 29fed9c and 8e43a2b and are resolved.

@vlad-perevezentsev vlad-perevezentsev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review at 89f282a. The grader change is right: matches is built from the same nearest-first matching, and the metrics do not change. One thing should be fixed before merge: plain repo report now reads every cell's grade.json without the guards read_grade uses. The details are in the line comments.

What I ran, at 89f282a:

  • make ci-local --offline: 0 failed, 17 passed, 13 skipped (network or Docker only).
  • A hand-written cell whose grade.json is "[" * 200000: summarize and load_hits raise RecursionError. When grade.json is a FIFO, summarize hangs (timeout 10 exits 124). When it is a symlink, it is followed.
  • A replay cell that result.json marks grader_failure, with a valid grade.json: the table scores it 0.0, and load_hits still lists its hit.

Checked, no issues

  • Matching and scores: the sort key is still (distance, finding, _id_key(thread)), so threads_hit and every metric are as before. matches follows threads_hit order. Keeping VERSION at "1" holds, as answered in the thread: no metric or pass changes for the same inputs.
  • Earlier threads: the "" for missing text and the rep-10 ordering test are in 89f282a, as the replies say.
  • Docs and changelog: contract §4.5 and §9 describe matches, --hits, --hits --json and parse_errors as the code does. The fragment is under Added and says what changed.

Comment thread dokimi/dokimi/repos/report.py Outdated
Comment thread dokimi/dokimi/repos/report.py
Comment thread dokimi/dokimi/commands/repo_run.py Outdated
Comment thread dokimi/dokimi/repos/report.py Outdated
@exai-bot

exai-bot commented Oct 30, 2026 •

Copy link
Copy Markdown

✅ OpenCodeReview: Review complete: 0 finding(s) across 10 selected item(s).

Review details
  • Commit reviewed: 5210ebe against merge base 83f18fb
  • Coverage: 10 files in 1 groups
  • Engine: OpenCodeReview v1.13.3, us.openai.gpt-6.1-sol
  • Usage: 125k tokens (60k from cache), 19s
  • Tool calls: 7 (6 file_read, 1 code_search)
  • Replay hit-pair grading and reporting, CLI support, documentation, and tests: 10 files

…der failures

`repo report` reads every cell's grade.json for parse_errors, and --hits
for the matches. _grade_doc read it with a plain read_text, so a FIFO
blocked the whole table, a link out of the batch was followed, and a
deeply nested file raised RecursionError. It now goes through
grading.read_grade_json, the reader read_grade uses (lstat, O_NOFOLLOW,
and RecursionError as no grade). read_grade gets the RecursionError
case too: it used to crash the grading of such a cell.

--hits also credits no match of a cell whose result says grader_failure,
which the table scores as 0.

Signed-off-by: Nikolay Petrov <nikolay.a.petrov@intel.com>
Copilot AI balanced review requested due to automatic review settings October 30, 2026 05:29
@napetrov
napetrov requested a review from a team as a code owner October 30, 2026 05:29

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The FIFO test uses an unguarded Windows-incompatible os.mkfifo call and can falsely pass despite a blocking regression because it does not assert that the initial join completed without timing out.

2 open issues
What changed in this PR

2 new findings.

This PR makes dokimi’s location-based replay credits auditable without changing scoring, and distinguishes answer parse failures from lower review recall.

Changes:

  • Records each credited thread/finding pair in details.matches.
  • Adds repo report --hits text and JSON output, plus per-row parse-error counts.
  • Shares guarded grade-file reading and adds regression tests and documentation.
File Description
dokimi/​tests/​test_repo_report_hits.py Tests hit reports, parse-error counts, and malformed grade handling.
dokimi/​tests/​test_repo_builtin_graders.py Verifies credited pairs, ordering, normalization, and text limits.
dokimi/​dokimi/​repos/​report.py Loads and formats hits; reports parse-error counts.
dokimi/​dokimi/​repos/​grading.py Extracts shared guarded JSON reading and handles excessive nesting.
dokimi/​dokimi/​repos/​graders/​review.py Records credited pairs while preserving matching and metrics.
dokimi/​dokimi/​commands/​repo_run.py Adds and dispatches repo report --hits.
dokimi/​docs/​repo-evaluation-contract.md Documents match records and report output.
dokimi/​changelog.d/​20261029_120000_antonwolfy_replay-hit-pairs.md Announces replay-hit details and parse-error reporting.

Review effort: Balanced

Comment thread dokimi/tests/test_repo_report_hits.py
Comment thread dokimi/tests/test_repo_report_hits.py Outdated
@napetrov

Copy link
Copy Markdown
Contributor

Two blocking threads from the review are handled in 9092ed4a on this PR's branch (pushed).

  • report.py:326 (blocking, grade.json read): _grade_doc now reads through grading.read_grade_json, the function read_grade uses (lstat, O_NOFOLLOW, non-regular file is no grade). A FIFO no longer blocks the table and a link out of the batch is not followed. RecursionError on a deeply nested grade.json counts as no grade, in the report and in read_grade (which did not catch it before). Test: test_a_grade_nested_too_deep_or_linked_out_of_the_batch_is_no_grade and test_a_grade_that_is_a_fifo_is_no_grade_and_does_not_block_the_report, both red on the previous head.
  • report.py:349 (--hits credits grader_failure cells): load_hits credits no match of a cell whose result says grader_failure, matching the table's score of 0. Test: test_hits_credit_no_match_of_a_cell_the_table_scores_as_a_grader_failure.

Not in this push: the parse_errors part of the 349 suggestion (leaving grader_failure cells out of the count), since such a cell's answer may still not parse and is counted as failed; the two nits (repo_run.py:554 help text, report.py:311 note wording). Say if you want any of them in this PR and I will add them.

Review comment 4418950465 (the parse_errors half): a cell whose
result.json says grader_failure was counted both in the row's
grader_failures and, when its grade.json had metrics.parse_error, in
parse_errors. summarize now leaves it out of parse_errors, as 9092ed4
leaves it out of --hits.

Review comment 4418950480: the parse-error note said "(counted as
failed)" on every row, but a score-only row (every
dokimi:review_replay row) has no failure count; the cell is in the mean
as a score of 0. The note now says "(scored 0)" on a score row and keeps
"(counted as failed)" on a gate row.

The module docstring, contract §9 and the changelog fragment say both.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Review comment 4418950473: the --hits help said the pairs are listed
"by task, arm and rep", but load_hits sorts and format_hits groups them
by task, arm, model and rep, as contract §9 and the changelog fragment
already say. The help now says so too.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Review comment 4420760178: the test called os.mkfifo unguarded, an
AttributeError on Windows. It is now skipped where os.mkfifo is missing,
as tests/test_task_format_detect.py does.

Review comment 4420760207: a report that opened the FIFO could still
pass, because the cleanup opened the write end, the blocked read got EOF
and the empty file read as no grade. The test now records whether the
first join timed out, then opens the write end (non-blocking, a bounded
number of times) until every blocked read has ended, and only then
asserts that it did not time out.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
@antonwolfy

Copy link
Copy Markdown
Contributor Author

Review round in 414855f, 6421d62 and e53ab20, on top of 9092ed4, which fixed the two blocking points.

  • parse_errors leaves out a cell already counted as a grader failure, as --hits does since 9092ed4.
  • A score-only row says (scored 0); a gate row keeps (counted as failed).
  • The --hits help says "by task, arm, model and rep".
  • The FIFO test is skipped where os.mkfifo is missing, and it now fails when the read blocks instead of passing after cleanup.

Every thread is answered and resolved; the Testing section has the mutants and make ci-local.

Comment thread dokimi/dokimi/repos/report.py Outdated
Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
The merge with main brings the flag registry of intel#254: its ratchet test
fails a spelling that is neither canonical in dokimi/cli_flags.py nor on
tests/data/cli_flags_allowlist.txt, and CI fails a stale
docs/reference/cli-interface.json. `--hits` switches repo report from the
table to the list of credited replay hits; no registry concept names a
view switch, so it gets an allowlist row with that reason, and the
snapshot is regenerated with what `make cli-snapshot` runs (the only
change is the --hits option).

intel#298's scripts/check_comment_blocks.py --base origin/main --head HEAD
finds no comment block over two lines among the lines this PR adds or
edits, so no comment changes.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>
Review comment 4422546939: load_hits built each hit as the cell's task,
arm, model, rep and cell followed by **match, so a details.matches entry
with a key of one of those names moved the hit to another task or
repetition in both --hits and --hits --json. The match is now spread
first and the result.json values last. format_hits only reads these
records, so it needs no change.

A new test grades a cell whose match carries all five colliding keys and
checks that the JSON records and the text listing show the cell's own
task, arm, model, rep and cell. The contract (§9) and the changelog
fragment say the five come from result.json.

Signed-off-by: Anton Volkov <antonwolfy@gmail.com>

@vlad-perevezentsev vlad-perevezentsev left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I checked 5210ebe. All four points of my first round are fixed, and so is exai-bot's **m point: load_hits now puts the cell's result.json task, arm, model, rep and cell after the match's keys.

What I ran, at 5210ebe:

  • make ci-local --offline: 0 failed, 19 passed, 13 skipped (network or Docker only).
  • test_a_match_key_named_like_the_cell_key_does_not_move_the_hit fails on 5210ebe's parent and passes at the head.
  • At e53ab20, the new tests/test_repo_report_hits.py on 89f282a: the nested/linked grade, FIFO, grader-failure, parse-error and (scored 0) tests fail there.
  • The first round's repro again: a "[" * 200000 and a FIFO grade.json are no grade, and summarize and load_hits return.

Checked, no issues

  • read_grade_json: read_grade and the report now share one reader. read_grade also treats a nested-too-deep grade as no grade now, where it raised before.
  • Grader failures: --hits and parse_errors both leave out a cell that result.json marks grader_failure, so they agree with the table. The docstring, contract §9 and the fragment say so.
  • FIFO test: it is skipped without os.mkfifo, and it fails when the read blocks, as the replies to Copilot say.

@napetrov
napetrov merged commit e64b192 into intel:main Oct 30, 2026
21 checks passed
@antonwolfy
antonwolfy deleted the feat/replay-hit-pairs branch October 30, 2026 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cli-interface-change Changes dokimi's user-facing CLI surface

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants