From a98ec461af6fe142c42362d0d6e98f4960f01d9e Mon Sep 17 00:00:00 2001 From: Gergo Magyar Date: Tue, 8 Sep 2026 19:19:43 +0000 Subject: [PATCH] fix(eval): an uninvoked skill must not move the arm's quality median Found by the offline sweep: a skill-not-invoked row still carried its score into the arm's quality median. aggregate()'s filter dropped EXCLUDED_ERROR_KINDS and evidence_valid=False, and skill-not-invoked is neither, so an arm could be credited for a review it never performed with the skill under test - which is the one thing an arm exists to measure. Excluded from the QUALITY metrics only. Cost and duration still count that row, because the session really ran and really was billed, and the promotion gate still sees it, because it has its own vocabulary for a candidate that never loaded its skill. Two wider fixes were tried and abandoned, both because the tests said so rather than because I reasoned it out first. Reusing the health guard's evidence_failed predicate also excluded transcript-missing rows, but test_aggregate_excludes_session_error_rows_from_medians pins those as counting: that session ran, only its transcript is unverifiable. Excluding the row from `valid` outright turned a candidate whose skill never loaded from keep_incumbent into insufficient_evidence - the safety property held either way, but the decision vocabulary is promotion semantics and not mine to change on a measurement fix. Mutation-checked: putting the rows back into the quality median fails the new test. Both directions asserted, since a filter that excludes everything would also pass - a wrong-but-valid review still moves quality, because being wrong is exactly what a quality median should reflect. 682 eval tests pass, 16 skipped. --- eval/tests/test_offline_sweep_integration.py | 11 ++++------ eval/tests/test_workflow_bench.py | 23 ++++++++++++++++++++ eval/workflow_bench/runner.py | 12 +++++++--- 3 files changed, 36 insertions(+), 10 deletions(-) diff --git a/eval/tests/test_offline_sweep_integration.py b/eval/tests/test_offline_sweep_integration.py index b86c95b6b..dd3f258a1 100644 --- a/eval/tests/test_offline_sweep_integration.py +++ b/eval/tests/test_offline_sweep_integration.py @@ -262,12 +262,9 @@ def test_a_review_that_never_invoked_its_skill_is_not_a_measurement(bench, monke assert row["error_kind"] == "skill-not-invoked" assert code not in (None, 0), "the sweep must not report success on unusable evidence" - # Recorded here as the behaviour actually is, not as I first assumed. The - # row still carries a score, and aggregate() still counts it toward the arm - # median: its filter drops EXCLUDED_ERROR_KINDS and evidence_valid=False, - # and "skill-not-invoked" is neither. The health guard is what stops the - # sweep, so a single-run sweep cannot promote on it - but in a mixed run the - # median for an arm would include a cell whose skill never ran. Pinned so - # the behaviour cannot change silently either way. + # The row still carries its own score - the artifact was well formed - but + # aggregate() now keeps it out of the arm's QUALITY median, since a cell + # whose skill never ran did not measure that skill. It still counts for + # cost, because the session ran and was billed. assert row["review_weighted_f1"] == 1.0 assert row["review_evidence_valid"] is True diff --git a/eval/tests/test_workflow_bench.py b/eval/tests/test_workflow_bench.py index a53b2b8cd..b8b70c0d6 100644 --- a/eval/tests/test_workflow_bench.py +++ b/eval/tests/test_workflow_bench.py @@ -1052,3 +1052,26 @@ def test_a_raising_packed_cell_still_persists_its_settled_siblings(): ) assert (1, "review") in folded, "the sibling that completed was never recorded" + + +def test_an_uninvoked_skill_does_not_move_the_quality_median_but_still_costs(): + """An arm measures a SKILL; a cell where the skill never ran did not measure it. + + The row's evidence is well formed, so the old filter kept it and its score + moved the arm's quality median - an arm could be credited for a review it + never performed with the skill under test. Cost and duration still count: + that session really ran and really was billed. + """ + + good = record(review_weighted_f1=1.0, cost_usd=2.0) + uninvoked = record( + review_weighted_f1=0.0, cost_usd=4.0, error_kind="skill-not-invoked", skill_invoked=False + ) + agg = aggregate([good, uninvoked]) + + assert agg["review_weighted_f1"] == 1.0, "the uninvoked cell must not drag quality" + assert agg["cost_usd"] == 3.0, "but it was still billed, so it counts for cost" + + # A wrong-but-valid review is a quality result and must still count. + wrong = record(review_weighted_f1=0.0, cost_usd=2.0, resolved=False, error_kind="oracle-failed") + assert aggregate([good, wrong])["review_weighted_f1"] == 0.5 diff --git a/eval/workflow_bench/runner.py b/eval/workflow_bench/runner.py index a2223a20a..25a2d5f5a 100644 --- a/eval/workflow_bench/runner.py +++ b/eval/workflow_bench/runner.py @@ -1591,11 +1591,17 @@ def aggregate(records: list[dict[str, Any]]) -> dict[str, Any]: "review_category_accuracy", "review_grounded_evidence", ) - if any("review_weighted_f1" in record for record in valid): + # QUALITY metrics only: a cell whose skill never ran did not measure the + # skill, so its score must not move the arm's quality median. It stays in + # `valid` for cost and duration, because that session really did run and + # really was billed - and it stays visible to the promotion gate, which has + # its own vocabulary for a candidate that never loaded its skill. + scored = [record for record in valid if record.get("error_kind") != "skill-not-invoked"] + if any("review_weighted_f1" in record for record in scored): for metric in review_metrics: - values = [record[metric] for record in valid if record.get(metric) is not None] + values = [record[metric] for record in scored if record.get(metric) is not None] reducer = min if metric == "review_blocker_recall" else statistics.median - out[metric] = reducer(values) if values and len(values) == len(valid) else None + out[metric] = reducer(values) if values and len(values) == len(scored) else None verdicts = [record.get("review_verdict_correct") for record in valid] out["review_verdict_correct"] = ( all(value is True for value in verdicts)