[v0.5.0] benchmark scoring — correctness scorer with manual-adjudication hooks#100
Conversation
…ooks Applies the methodology's Correctness Scoring rubric (docs/benchmarks/ PUBLIC-BENCHMARK-METHODOLOGY.md) to a run's scoring placeholders (issue #75) against the answer-key corpus (reusing benchmarks/corpus.py's validate_corpus rather than duplicating it). Plumbing only: the automatic pass decides only the two judgment-free cases (failed-cell passthrough at 0.0, empty/blank-answer at 0.0); every other succeeded cell is queued to an adjudication queue file for a human, who is ingested back via ingest_adjudication_verdicts. A record once tagged scoring_method: "human" is never overwritten by a later automatic pass. Also computes a structural grounding_evidence_present fact and the methodology's correct-but-ungrounded flag (reported separately, never folded into the score), plus per-category/per-tool-model rollups. Refs #63.
Wires benchmarks.scoring.score_run and .ingest_adjudication_verdicts into `python -m benchmarks` as `score` and `adjudicate`, following the existing argparse subparsers pattern (run/report/validate-corpus). Both take --corpus/--schema so a run can only be scored against a schema-valid answer-key corpus, matching `validate-corpus`'s own gate. Refs #63.
Covers: rubric application for each score value (via the automatic empty-answer 0.0 path and human-ingested 1.0/0.5), per-category rollups, the correct-but-ungrounded flag (both derived and grounded cases), adjudication-queue emission with answer-key context, human-verdict ingest, the never-overwrite-a-human-verdict guarantee, failed-cell passthrough, denominator invariance, and the `score`/`adjudicate` CLI subcommands -- plus clean-refusal cases (missing scoring files, missing transcript, unknown corpus id, invalid/missing verdict fields, verdict against a failed cell). Reuses the synthetic fixture corpus from tests/benchmarks/fixtures/ corpus.sample.yml (issue #94/#97) as the answer-key source; every scoring/transcript record is hand-authored. No real corpus questions, no network, no LLM-as-judge. Refs #63.
ayhammouda
added
agent-pr-opened
📝 WalkthroughWalkthroughAdds a new pytest suite (tests/benchmarks/test_scoring.py) covering benchmarks/scoring.py: automatic scoring of failed/blank/answered cells, grounding evidence detection, category rollups, denominator invariance, human adjudication ingest, refusal/error paths, and CLI integration for the score and adjudicate subcommands. ChangesBenchmark Scoring Test Suite
Estimated code review effort: 2 (Simple) | ~15 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/benchmarks/test_scoring.py (2)
236-279: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd test coverage for per-tool-model rollup values.
The PR summary states per-tool-model rollups are written to
scoring-rollups.json, andon_disk == rollupsat line 279 provides structural validation, but no test asserts specificby_tool_modelvalues. All tests use a single competitor ("tool-a"), so even ifby_tool_modelwere checked, it would only have one entry. A test with two or more competitors would meaningfully validate the per-tool-model breakdown and catch regressions in that feature.♻️ Suggested additional test
+def test_score_run_produces_per_tool_model_rollups(tmp_path: Path) -> None: + cells = [ + ( + _scoring_record(competitor_id="tool-a", corpus_id="SYN-EX-001"), + _transcript_record(competitor_id="tool-a", corpus_id="SYN-EX-001", answer=""), + ), + ( + _scoring_record(competitor_id="tool-b", corpus_id="SYN-EX-001"), + _transcript_record(competitor_id="tool-b", corpus_id="SYN-EX-001", answer=""), + ), + ( + _scoring_record(competitor_id="tool-b", corpus_id="SYN-EX-002"), + _transcript_record( + competitor_id="tool-b", corpus_id="SYN-EX-002", answer="pending answer" + ), + ), + ] + run_dir = _build_run_dir(tmp_path, cells) + + result = _score(run_dir) + rollups = result["rollups"] + + assert rollups["by_tool_model"]["tool-a"]["total_cells"] == 1 + assert rollups["by_tool_model"]["tool-a"]["scored_cells"] == 1 + assert rollups["by_tool_model"]["tool-b"]["total_cells"] == 2 + assert rollups["by_tool_model"]["tool-b"]["scored_cells"] == 1 + assert rollups["by_tool_model"]["tool-b"]["pending_manual_scoring_cells"] == 1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/benchmarks/test_scoring.py` around lines 236 - 279, The current scoring rollup test only validates overall and per-category aggregates, so it does not actually verify the new by_tool_model breakdown. Update test_score_run_produces_per_category_rollups to include at least two competitors with distinct tool/model identifiers in the _scoring_record and _transcript_record fixtures, then assert the specific rollup values under result["rollups"]["by_tool_model"] and the persisted scoring-rollups.json for each tool/model entry.
214-230: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueConsider testing grounding evidence with error tool calls.
The test validates that non-error tool calls (
is_error: False) setgrounding_evidence_presenttoTrue, but no test coversis_error: True. If the scorer is expected to ignore failed tool calls when inferring grounding evidence, that behavior is untested. Adding a companion case would close this gap.♻️ Suggested additional test
+def test_score_run_does_not_count_error_tool_calls_as_grounding_evidence( + tmp_path: Path, +) -> None: + scoring = _scoring_record(competitor_id="tool-a", corpus_id="SYN-EX-001") + transcript = _transcript_record( + competitor_id="tool-a", + corpus_id="SYN-EX-001", + answer="Ungrounded candidate answer.", + tool_calls=[ + {"tool": "search_docs", "arguments": {}, "result": None, "is_error": True} + ], + ) + run_dir = _build_run_dir(tmp_path, [(scoring, transcript)]) + + _score(run_dir) + + updated = _read_scoring(run_dir, "tool-a", "SYN-EX-001") + assert updated["grounding_evidence_present"] is False + assert _read_queue(run_dir)["cells"][0]["grounding_evidence_present"] is False🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/benchmarks/test_scoring.py` around lines 214 - 230, The grounding-evidence test only covers successful tool calls, so add a companion case in test_score_run_detects_grounding_evidence_from_tool_calls that uses a transcript with is_error set to True and verifies how _score/_read_scoring handle it. Reuse the existing helpers (_scoring_record, _transcript_record, _build_run_dir, _score, _read_scoring, _read_queue) so the new assertion clearly checks whether failed tool calls should or should not set grounding_evidence_present.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/benchmarks/test_scoring.py`:
- Around line 236-279: The current scoring rollup test only validates overall
and per-category aggregates, so it does not actually verify the new
by_tool_model breakdown. Update test_score_run_produces_per_category_rollups to
include at least two competitors with distinct tool/model identifiers in the
_scoring_record and _transcript_record fixtures, then assert the specific rollup
values under result["rollups"]["by_tool_model"] and the persisted
scoring-rollups.json for each tool/model entry.
- Around line 214-230: The grounding-evidence test only covers successful tool
calls, so add a companion case in
test_score_run_detects_grounding_evidence_from_tool_calls that uses a transcript
with is_error set to True and verifies how _score/_read_scoring handle it. Reuse
the existing helpers (_scoring_record, _transcript_record, _build_run_dir,
_score, _read_scoring, _read_queue) so the new assertion clearly checks whether
failed tool calls should or should not set grounding_evidence_present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 89b2a23f-74cb-4438-be43-b80ce218f489
⛔ Files ignored due to path filters (2)
benchmarks/__main__.pyis excluded by none and included by nonebenchmarks/scoring.pyis excluded by none and included by none
📒 Files selected for processing (1)
tests/benchmarks/test_scoring.py
…ding Addresses both CodeRabbit nitpicks on PR #100 (additive changes to the new test file only): - test_score_run_produces_per_tool_model_rollups: two competitors, one with an explicit `id:provider/model` tool_model_key, asserting specific by_tool_model_key totals/scored/pending counts in the returned rollups and the persisted scoring-rollups.json. Adapted from CodeRabbit's template: the real rollup key is `by_tool_model_key`, not `by_tool_model`. - test_score_run_does_not_count_error_tool_calls_as_grounding_evidence: a transcript whose only tool call has is_error: True does not set grounding_evidence_present, in the scoring record or the queue entry. Refs #63.
|
@coderabbitai Both nitpicks from the review are addressed in f37987c:
Full validation gate re-run after the change: ruff (src/tests + benchmarks) clean, pyright (src + benchmarks) 0 errors, full pytest 426 passed, tests/benchmarks 119 passed, tests/benchmarks/test_scoring.py 21 passed, doctor all checks passed. |
|
Tip For best results, initiate chat on the files or code changes.
Great to see the full validation gate (ruff, pyright, pytest across the whole suite and the new scoring tests, doctor checks) passing clean. Appreciate the thorough confirmation. (‡•ᴥ•)ノ |
|
Tip For best results, initiate chat on the files or code changes.
Clean validation gate (ruff, pyright, full pytest suite, doctor) confirms this is good to go. ᕦ(ò_óˇ)ᕤ |
…ding (#102) Addresses both CodeRabbit nitpicks on PR #100 (additive changes to the new test file only): - test_score_run_produces_per_tool_model_rollups: two competitors, one with an explicit `id:provider/model` tool_model_key, asserting specific by_tool_model_key totals/scored/pending counts in the returned rollups and the persisted scoring-rollups.json. Adapted from CodeRabbit's template: the real rollup key is `by_tool_model_key`, not `by_tool_model`. - test_score_run_does_not_count_error_tool_calls_as_grounding_evidence: a transcript whose only tool call has is_error: True does not set grounding_evidence_present, in the scoring record or the queue entry. Refs #63.
1 participant
Refs #63.
Closes #88.
Summary
Adds
benchmarks/scoring.py, the correctness scorer with manual-adjudicationhooks called out in issue #88 (work package 5 of
docs/benchmarks/PUBLIC-BENCHMARK-METHODOLOGY.md). It turns a run's scoringplaceholders (written by the runner, issue #75:
scoring/<competitor_id>/<corpus_id>.json) into finalized records byapplying the methodology's rubric against the answer-key corpus, reusing
benchmarks/corpus.py'svalidate_corpus(issue #94/#97) rather thanduplicating corpus loading/validation.
This is plumbing only, per the issue's explicit instruction ("Actual
scoring of real answers stays human-adjudicated"). The automatic pass
(
score_run) only ever decides the two judgment-free cases:score: 0.0; the scorerre-emits it untouched in substance (same
score/requires_manual_scoring),tagged with scorer provenance metadata.
under the rubric with no interpretation required, scored
0.0.Every other succeeded cell is left with
score: None/requires_manual_scoring: Trueand written to an adjudication queue file(
<run-dir>/adjudication/queue.json) with the candidate answer, anyrecorded tool calls, and the corpus answer key/citations/expected
properties, so a human can apply the rubric. A new
python -m benchmarks adjudicateCLI subcommand ingests a human verdicts file(
{competitor_id, corpus_id, score, correct_but_ungrounded?, adjudicator?, notes?}) and re-emits final records taggedscoring_method: "human".A record once tagged
scoring_method: "human"is never touched again by alater
score_runcall (covered bytest_score_run_never_overwrites_a_human_verdict).Also computes a structural
grounding_evidence_presentfact (did thetranscript record at least one successful tool call?) and the methodology's
correct_but_ungroundedflag from it — set when a score is> 0.0but nogrounding evidence was recorded. That flag is a separate field; it never
changes the score itself. Per-category and per-tool-model-key rollups
(
total_cells,scored_cells,mean_score,pending_manual_scoring_cells,failed_cells,correct_but_ungrounded_cells,human_scored_cells,automatic_scored_cells) are written to<run-dir>/scoring-rollups.json.benchmarks/__main__.pygains two subcommands following the existingargparse-subparsers pattern:
score(runsscore_run) andadjudicate(runs
ingest_adjudication_verdicts), both gated on--corpus/--schemathe same way
validate-corpusis.benchmarks/report.py(issue #74/#90) is not modified — it alreadyreads
scoring/<competitor>/<qid>.jsondirectly and treats any record witha numeric
score+requires_manual_scoring: Falseas finalized, so ascored run is picked up unchanged.
Why this approach (design calls not fully prescribed in the issue)
The issue leaves the automatic-pass decision boundary and the queue/verdict
file shapes for the agent to design. Given the issue's own instruction that
"real-answer scoring stays human," I deliberately made the automatic pass as
narrow as possible — it never attempts a semantic judgment of correctness
(no keyword/property matching against free-text answers, no LLM-as-judge).
It only resolves the two cases where the methodology's rubric already
requires no interpretation (failed cell, empty answer). Everything else is
queued. This keeps the "automatic pass" honestly plumbing rather than a
disguised auto-grader, and keeps the tests fixture-only per the pipeline's
constraints.
grounding_evidence_presentis a purely structural signal (a successfultool call was recorded), not a semantic judgment that the retrieved content
actually supports the answer text — that nuance stays with the human
adjudicator via the optional
correct_but_ungroundedoverride in theverdicts file.
Recovery-clause check
Issue #88's recovery clause: "If the corpus schema (#71) lacks fields the
rubric needs (expected answer properties, required citations), stop and
comment here." Re-verified against current main:
docs/benchmarks/ corpus.schema.json(merged in #97 @ 1300cd3) already declaresanswer_key,citations, andexpected_propertiesas required questionfields — exactly what the rubric needs. The recovery clause does not fire;
no schema fields were invented.
Why this triggered supervisor review
Diff size. This PR is 1372 lines across 3 files (
benchmarks/scoring.py+599,
benchmarks/__main__.py+81,tests/benchmarks/test_scoring.py+692 including the CodeRabbit follow-up tests), over the §7 500-line
threshold (counting all non-generated files including tests, per the
precedent set on #97/#98). No forbidden-territory
paths were touched, no new dependency was added,
pyproject.toml/uv.lockare untouched, and no network/async was introduced — diff size is the only
trigger. Opening the PR with the
supervisor-reviewlabel per policy;not requesting merge.
Acceptance criteria
produces per-cell records with
score ∈ {1.0, 0.5, 0.0}, per-categoryrollups, and the methodology's "correct-but-ungrounded" flag (marked
and reported separately, never folded into the score). See
score_run/ingest_adjudication_verdictsinbenchmarks/scoring.pyand the
_compute_rollups/_rollup_statsper-category output.emitted to an adjudication queue file; a CLI subcommand
(
python -m benchmarks adjudicate) ingests human verdicts back andre-emits final records. Automatic scoring never overwrites a human
verdict (
score_runskips any record taggedscoring_method: "human"; seetest_score_run_never_overwrites_a_human_verdict).denominator stays the full corpus count per tool×model cell. One-line
note: "pass through untouched" is interpreted as value-untouched —
score/requires_manual_scoring/included_in_correctness_denominatorare preserved exactly; the scorer additionally tags the record with
scoring_method: "automatic"and ascored_attimestamp forprovenance uniformity with every other finalized record. See
test_score_run_passes_failed_cells_through_at_zeroandtest_score_run_denominator_includes_every_scoring_file.tests/benchmarks/) use fixture answer keys andtranscripts only — no LLM-as-judge, no network, no real corpus
questions authored.
tests/benchmarks/test_scoring.pyreuses thesynthetic fixture corpus (
tests/benchmarks/fixtures/corpus.sample.yml,issue [v0.5.0] benchmark corpus — mechanical slice: schema, validator, placeholder fixture (split from #71) #94/[v0.5.0] benchmark corpus — mechanical slice: schema, validator, placeholder fixture (split from #71) #97) and hand-authors every scoring/transcript record.
Refs #63, neverCloses #63. No README,PyPI, or launch-copy file touched.
Validation gate (all commands from the issue + the worker brief)
Re-run in full after the CodeRabbit follow-up commit (f37987c):
CodeRabbit triage
Triaged — 2 findings, both Trivial nitpicks, both valid, both addressed in
follow-up PR #102. (The initial auto-review was rate limited; the
re-triggered review completed with 2 nitpicks on
tests/benchmarks/test_scoring.pyand no blocking findings — but ~1 minuteafter this PR was merged, so the fixes land via #102 rather than this
branch.
benchmarks/scoring.pyandbenchmarks/__main__.pywere excludedby the repo's
.coderabbit.yamlpath filters.)test_score_run_produces_per_tool_model_rollups: two competitors, one carrying an explicitid:provider/modeltool_model_key; asserts per-entrytotal_cells/scored_cells/pending_manual_scoring_cellsand the persistedscoring-rollups.json. Adapted from the suggested diff — the real rollup key isby_tool_model_key, notby_tool_model.is_error: Truetool calls untested for grounding-evidence inferencetest_score_run_does_not_count_error_tool_calls_as_grounding_evidence: an errored tool call leavesgrounding_evidence_presentFalse in both the scoring record and the queue entry, matching_has_grounding_evidence's successful-calls-only behavior.Both changes are purely additive to this PR's own new test file — no merged
tests touched. Detailed reply posted on this PR's thread; because this PR
merged before the review completed, the fixes land via follow-up PR #102.
Scope notes
human-led).
execution.
benchmarks/report.py,pyproject.toml,uv.lock,.github/workflows/,and all forbidden-territory paths in AGENT-EXECUTION-PIPELINE.md §2 are
untouched.
tests/benchmarks/and elsewhere are untouched(additive-only change).