From 21aa3626d7cf379176ff16555bca1ddc491b297f Mon Sep 17 00:00:00 2001 From: Jarek Potiuk Date: Wed, 23 Sep 2026 00:11:38 +0200 Subject: [PATCH] fix(skill-evals): retry a dropped grader verdict, and correct the step 1f expectations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two of the cases left red by #1340 were the fixture disagreeing with the documented process rather than the model getting it wrong, and verifying that turned up a third problem in the runner itself. **A verdict the grader drops is no longer a failure.** `batch_grade_prose_fields` sends one rubric prompt covering every pair, and the grader intermittently omits a path from a larger batch. That pair was then recorded as `(False, "grader did not return a verdict")` — a FAIL indistinguishable in the report from a real mismatch, for a reason that has nothing to do with the candidate output. It is the mechanism behind cases that appear to flap between runs of unchanged code. The dropped subset is now re-asked once; silence that survives the retry is still reported, and a verdict the grader actively gives is never re-asked, so a NO cannot become a YES by asking twice. The structural-assertion path already drew this distinction with a tri-state verdict; the prose path now does too. **`case-7` expected the wrong step.** The process table in `gather.md` — the table this step tells the model to cross-reference — puts "Archive URL captured" at `14 → 15`, and the step's output spec says in as many words that when two consecutive rows both apply the answer is the inclusive hyphenated range. The tracker in this fixture is still open with the archive URL populated, so it sits on that row. The model answered `"14-15"` on every run; the fixture asserted `"14"`. **`case-5` asserted a field the spec calls explanatory.** `key_signals` exists so a reviewer can see why a step was chosen, and which facts are worth naming is a judgement call. The model named five signals that subsume the expected three and the case failed on the difference. Cases 1, 2 and 6 in this same step already omit the field, so 3, 4 and 5 now match the step's own majority and the spec records that it is emitted but not graded. Same fragility #1340 removed from the guardrails `evidence` field. `case-7`'s `step_description` is also shortened to the one-line shape the spec asks for; the first attempt enumerated every action in the combined apply, which made a prose-graded field fail on whichever sub-action the model left out. The verifying run is 43/45 with no dropped verdicts, against 42/45 and two dropped verdicts before. The two that remain are the grader's judgement on free text — one on whether "valid" implies "CVE-worthy", one on whether naming the confirmation step over-describes the action — and both have passed on other runs. They are left alone deliberately: rewording a fixture until a rubric happens to agree tunes the test to the model. Narrowing what the prose fields assert is the real fix, and it wants deciding on its own terms rather than inside this change. Generated-by: Claude Opus 5 --- .../case-3-valid-no-cve/expected.json | 3 +- .../case-4-cve-allocated-no-pr/expected.json | 3 +- .../expected.json | 3 +- .../expected.json | 5 +-- .../fixtures/output-spec.md | 4 ++ tools/skill-evals/src/skill_evals/runner.py | 32 ++++++++++++++- tools/skill-evals/tests/_grader_drops_one.py | 39 +++++++++++++++++++ tools/skill-evals/tests/test_runner.py | 34 ++++++++++++++++ 8 files changed, 113 insertions(+), 10 deletions(-) create mode 100644 tools/skill-evals/tests/_grader_drops_one.py diff --git a/tools/skill-evals/evals/security-issue-sync/step-1f-process-step/fixtures/case-3-valid-no-cve/expected.json b/tools/skill-evals/evals/security-issue-sync/step-1f-process-step/fixtures/case-3-valid-no-cve/expected.json index 8c7d5a79d..e38e12f07 100644 --- a/tools/skill-evals/evals/security-issue-sync/step-1f-process-step/fixtures/case-3-valid-no-cve/expected.json +++ b/tools/skill-evals/evals/security-issue-sync/step-1f-process-step/fixtures/case-3-valid-no-cve/expected.json @@ -1,5 +1,4 @@ { "process_step": "6", - "step_description": "Consensus reached: issue is valid and CVE-worthy, but no CVE has been allocated yet", - "key_signals": ["team consensus recorded in comments", "no CVE tool link", "no cve allocated label", "scope label airflow set"] + "step_description": "Consensus reached: issue is valid and CVE-worthy, but no CVE has been allocated yet" } diff --git a/tools/skill-evals/evals/security-issue-sync/step-1f-process-step/fixtures/case-4-cve-allocated-no-pr/expected.json b/tools/skill-evals/evals/security-issue-sync/step-1f-process-step/fixtures/case-4-cve-allocated-no-pr/expected.json index eb08ba666..ff28fa742 100644 --- a/tools/skill-evals/evals/security-issue-sync/step-1f-process-step/fixtures/case-4-cve-allocated-no-pr/expected.json +++ b/tools/skill-evals/evals/security-issue-sync/step-1f-process-step/fixtures/case-4-cve-allocated-no-pr/expected.json @@ -1,5 +1,4 @@ { "process_step": "7", - "step_description": "CVE allocated, no fix PR opened yet", - "key_signals": ["cve allocated label set", "CVE-2025-44812 in CVE tool link field", "no PR with the fix"] + "step_description": "CVE allocated, no fix PR opened yet" } diff --git a/tools/skill-evals/evals/security-issue-sync/step-1f-process-step/fixtures/case-5-pr-merged-awaiting-release/expected.json b/tools/skill-evals/evals/security-issue-sync/step-1f-process-step/fixtures/case-5-pr-merged-awaiting-release/expected.json index 49a3251af..109439c36 100644 --- a/tools/skill-evals/evals/security-issue-sync/step-1f-process-step/fixtures/case-5-pr-merged-awaiting-release/expected.json +++ b/tools/skill-evals/evals/security-issue-sync/step-1f-process-step/fixtures/case-5-pr-merged-awaiting-release/expected.json @@ -1,5 +1,4 @@ { "process_step": "11", - "step_description": "Fix PR merged into upstream; release carrying the fix has not shipped yet", - "key_signals": ["pr merged label set", "fix PR merged at 2025-10-14", "release 3.0.3 not yet on PyPI"] + "step_description": "Fix PR merged into upstream; release carrying the fix has not shipped yet" } diff --git a/tools/skill-evals/evals/security-issue-sync/step-1f-process-step/fixtures/case-7-announced-awaiting-cveorg/expected.json b/tools/skill-evals/evals/security-issue-sync/step-1f-process-step/fixtures/case-7-announced-awaiting-cveorg/expected.json index faca7ddd4..9a690f159 100644 --- a/tools/skill-evals/evals/security-issue-sync/step-1f-process-step/fixtures/case-7-announced-awaiting-cveorg/expected.json +++ b/tools/skill-evals/evals/security-issue-sync/step-1f-process-step/fixtures/case-7-announced-awaiting-cveorg/expected.json @@ -1,5 +1,4 @@ { - "process_step": "14", - "step_description": "Advisory sent and Public advisory URL populated; announced label set; awaiting cve.org propagation before closing", - "key_signals": ["announced label set", "Public advisory URL populated", "cve.org state is RESERVED not PUBLISHED"] + "process_step": "14-15", + "step_description": "Archive URL captured — the combined apply is due, and the tracker is still open" } diff --git a/tools/skill-evals/evals/security-issue-sync/step-1f-process-step/fixtures/output-spec.md b/tools/skill-evals/evals/security-issue-sync/step-1f-process-step/fixtures/output-spec.md index 1a5ecf0d9..aba979e6f 100644 --- a/tools/skill-evals/evals/security-issue-sync/step-1f-process-step/fixtures/output-spec.md +++ b/tools/skill-evals/evals/security-issue-sync/step-1f-process-step/fixtures/output-spec.md @@ -20,6 +20,10 @@ Use the exact step number strings from the table (e.g. `"1-2"`, `"3"`, `"4"`, `"5/6"`, `"6"`, `"7"`, `"11"`, `"12"`, `"13"`, `"14"`, `"15"`). `key_signals` lists the labels, body-field values, or observable facts that determined the step — helps reviewers understand why the step was chosen. +Always emit it. +It is explanatory rather than asserted: which facts are worth naming, and +how many, is a judgement call, so the cases grade `process_step` and +`step_description` and leave this field to the reader. If the observed state simultaneously satisfies two CONSECUTIVE numbered rows of the table — i.e. the tracker has just completed the earlier row's diff --git a/tools/skill-evals/src/skill_evals/runner.py b/tools/skill-evals/src/skill_evals/runner.py index 3a67e992f..3f73c48fe 100644 --- a/tools/skill-evals/src/skill_evals/runner.py +++ b/tools/skill-evals/src/skill_evals/runner.py @@ -610,9 +610,38 @@ def batch_grade_prose_fields( failure (timeout, OSError, non-zero exit, unparsable output, missing path in the verdict), every pair without a clean verdict is returned as ``(False, )``. + + A verdict the grader simply *omits* is retried once, on its own, before + being reported. One prompt covers every pair, so a grader that drops a + path — which it does intermittently on larger batches — fails that pair + for a reason that has nothing to do with the candidate output, and is + indistinguishable in the report from a real mismatch. Re-asking for the + dropped subset removes that class of false red. A pair the grader + actively judges is never re-asked: only silence is retried, so a `NO` + cannot be turned into a `YES` by asking twice. """ if not pairs: return {} + graded = _batch_grade_once(pairs, grader_cli, timeout) + missing = [pair for pair in pairs if pair[0] not in graded] + if missing: + graded.update(_batch_grade_once(missing, grader_cli, timeout, final=True)) + return graded + + +def _batch_grade_once( + pairs: list[tuple[str, object, object]], + grader_cli: str, + timeout: int, + *, + final: bool = False, +) -> dict[str, tuple[bool, str]]: + """One grader round-trip. + + Pairs the grader returned no verdict for are omitted from the result + unless ``final``, in which case they are reported as a failure so the + caller never silently drops a field. + """ prompt = BATCH_GRADER_RUBRIC.format(fields_block=_format_batch_fields_block(pairs)) try: stdout, stderr, rc = run_cli(grader_cli, prompt, timeout=timeout) @@ -629,7 +658,8 @@ def batch_grade_prose_fields( for path, _, _ in pairs: entry = verdict.get(path) if not isinstance(entry, dict) or "match" not in entry: - result[path] = (False, f"grader did not return a verdict for {path}") + if final: + result[path] = (False, f"grader did not return a verdict for {path}") continue match = bool(entry.get("match")) reason = str(entry.get("reason", "")).strip() diff --git a/tools/skill-evals/tests/_grader_drops_one.py b/tools/skill-evals/tests/_grader_drops_one.py new file mode 100644 index 000000000..d3382e29b --- /dev/null +++ b/tools/skill-evals/tests/_grader_drops_one.py @@ -0,0 +1,39 @@ +#!/usr/bin/env python3 +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +"""Mock batched grader that silently drops one path on its first call. + +Stands in for the real grader's intermittent habit of omitting a field from +a larger batch. The first invocation returns a verdict for every ``Field:`` +path except the last one; every later invocation verdicts everything it is +asked about. Call state is kept in the file named by +``GRADER_DROP_STATE_FILE``. +""" + +from __future__ import annotations + +import json +import os +import re +import sys +from pathlib import Path + + +def main() -> None: + paths = re.findall(r"^Field: (\S+)$", sys.stdin.read(), flags=re.MULTILINE) + state = Path(os.environ["GRADER_DROP_STATE_FILE"]) + first_call = not state.exists() + state.write_text((state.read_text() if state.exists() else "") + "call\n") + answered = paths[:-1] if first_call and len(paths) > 1 else paths + print(json.dumps({p: {"match": True, "reason": "ok"} for p in answered})) + + +if __name__ == "__main__": + main() diff --git a/tools/skill-evals/tests/test_runner.py b/tools/skill-evals/tests/test_runner.py index 2dd3f6f5b..05d4557c5 100644 --- a/tools/skill-evals/tests/test_runner.py +++ b/tools/skill-evals/tests/test_runner.py @@ -1437,6 +1437,40 @@ def test_batch_grade_grader_failure_marks_all_fail(): assert ok is False +def test_batch_grade_retries_a_dropped_verdict(tmp_path: Path): + """A path the grader silently omits is re-asked, not failed. + + The grader drops fields from larger batches intermittently, which has + nothing to do with the candidate output and is indistinguishable in the + report from a real mismatch. + """ + state = tmp_path / "drop-state" + inner = ( + f"GRADER_DROP_STATE_FILE={shlex.quote(str(state))} " + f"python3 {shlex.quote(str(_TESTS_DIR / '_grader_drops_one.py'))}" + ) + grader = f"bash -c {shlex.quote(inner)}" + pairs = [("$.a", "x", "y"), ("$.b", "x", "y")] + result = batch_grade_prose_fields(pairs, grader, timeout=5) + assert result["$.a"] == (True, "") + assert result["$.b"] == (True, "") + assert state.read_text().count("call") == 2 + + +def test_batch_grade_reports_a_verdict_missing_twice(tmp_path: Path): + """Silence that survives the retry is still reported, never dropped.""" + script = _TESTS_DIR / "_grader_empty.py" + script.write_text("import sys; sys.stdin.read(); print('{}')\n") + try: + pairs = [("$.a", "x", "y")] + result = batch_grade_prose_fields(pairs, f"python3 {shlex.quote(str(script))}", timeout=5) + ok, note = result["$.a"] + assert ok is False + assert "did not return a verdict" in note + finally: + script.unlink() + + # --------------------------------------------------------------------------- # compare_with_grader (uses the batched grader path) # ---------------------------------------------------------------------------