From 6195ada98a168a78ee4f7deccf0dd9bf0c204b43 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Tue, 28 Jul 2026 21:37:44 -0700 Subject: [PATCH 1/5] feat(review-suite): add referred-path relevance guard to eval grading ## Summary - `review-suite/scripts/evals/grader.py`: split `match_strength`'s two signals into a public `match_signals(expected, finding)` helper returning `(surface_hit, signal_hit)`, and record, per referred (partial/ambiguous) candidate, whether some referring finding concretely named the candidate's actual surface (`candidate_surface_relevant_root_cause_ids`). `grade()` now additionally reports `referred_relevant_root_cause_ids` (the subset of `referred_root_cause_ids` that passes this guard) and `combined_recall` ((matched + relevant-referred) / expected). - `review-suite/scripts/evals/report.py`: surface the guard at the per-case level as `ever_referred_relevant_root_cause_ids` and `mean_combined_recall`, reading both defensively via `.get(...)` so a grade dict built before the guard existed still reports cleanly. - Add grader and report unit tests covering a referral that fires on the right surface (relevance-guard eligible), a referral that fires on formulation prose alone at the wrong location (not eligible), the always-relevant ambiguous (two full-match) case, and backward compatibility with pre-guard grade dicts. ## Why - A future re-score of `s1-correctness-orchestrator` needs to grade `dependency-strictness-propagation` and `stale-claim-release-guard` against a preregistered gate that allows a case to pass on `mean_recall >= 0.6` OR a relevance-guarded combined matched+referred rate `>= 0.8`. The existing three-way grader already computes `referred_root_cause_ids`, but a referral only earned by matching accepted-formulation prose without ever naming the right file/function/symbol must not count toward that combined rate - the existing tooling had no way to tell the two referral shapes apart before scoring, so this was implemented as the smallest additive check on top of the grader's existing surface/signal split rather than left to be eyeballed during grading. - `GRADER_VERSION` stays `1.0`: the change is additive only (new fields derived from already-computed signals), with no change to any existing matched/missed/referred classification or field, so no corpus's declared `grader_version` needs to move to keep using it. Co-Authored-By: Claude Sonnet 5 --- review-suite/scripts/evals/grader.py | 73 +++++++++++++++++-- review-suite/scripts/evals/report.py | 21 ++++++ .../scripts/tests/test_eval_grader.py | 64 ++++++++++++++++ .../scripts/tests/test_eval_report.py | 66 +++++++++++++++++ 4 files changed, 219 insertions(+), 5 deletions(-) diff --git a/review-suite/scripts/evals/grader.py b/review-suite/scripts/evals/grader.py index a8143db..a0b8636 100644 --- a/review-suite/scripts/evals/grader.py +++ b/review-suite/scripts/evals/grader.py @@ -107,6 +107,27 @@ def _signal_match(formulations: list[str], text: str) -> bool: return any(normalize(item) and normalize(item) in text for item in formulations) +def match_signals( + expected: dict[str, Any], finding: dict[str, Any] +) -> tuple[bool, bool]: + """Return `(surface_hit, signal_hit)` for one expectation/finding pair. + + `surface_hit` is the concrete signal: the finding's location or evidence + names a token from the expectation's `surface` - the actual file, + function, or symbol the root cause turns on. `signal_hit` is the + accepted-formulation prose match alone, which vaguer commentary can + satisfy without ever naming where the defect lives. Split out from + `match_strength` so a referred (partial or ambiguous) candidate can be + judged concrete or not, rather than only judged matched or not. + """ + surface = _surface_tokens(expected.get("surface", "")) + surface_hit = bool(surface & finding_surfaces(finding)) + signal_hit = _signal_match( + expected.get("equivalent_formulations") or [], finding_text(finding) + ) + return surface_hit, signal_hit + + def match_strength(expected: dict[str, Any], finding: dict[str, Any]) -> str: """Return `full`, `partial`, or `none` for one expectation/finding pair. @@ -114,11 +135,7 @@ def match_strength(expected: dict[str, Any], finding: dict[str, Any]) -> str: describes the root cause in an accepted way. One signal alone is `partial` and is reported for adjudication rather than silently scored either way. """ - surface = _surface_tokens(expected.get("surface", "")) - surface_hit = bool(surface & finding_surfaces(finding)) - signal_hit = _signal_match( - expected.get("equivalent_formulations") or [], finding_text(finding) - ) + surface_hit, signal_hit = match_signals(expected, finding) if surface_hit and signal_hit: return "full" if surface_hit or signal_hit: @@ -135,16 +152,28 @@ def _classify_finding( strengths = [(rc, match_strength(rc, finding)) for rc in root_causes] full = [rc for rc, strength in strengths if strength == "full"] partial = [rc for rc, strength in strengths if strength == "partial"] + # Referred-path relevance guard (settled by the repository owner in #53's + # preregistered v2 scoring gate): a referred candidate only counts toward + # the combined matched+referred rate if this finding concretely names the + # candidate's actual surface, not merely if the grader marked it ambiguous + # or partial. `full` always implies surface_hit by construction, so an + # ambiguous candidate (two `full` matches) is always relevant; a `partial` + # candidate is relevant only when its own surface signal fired. + surface_relevant = {rc["id"] for rc in root_causes if match_signals(rc, finding)[0]} record: dict[str, Any] = { "finding_id": finding.get("id"), "severity": finding.get("severity"), "root_cause_id": None, "candidate_root_cause_ids": [], + "candidate_surface_relevant_root_cause_ids": [], } if len(full) > 1: record["classification"] = "ambiguous" record["candidate_root_cause_ids"] = [rc["id"] for rc in full] + record["candidate_surface_relevant_root_cause_ids"] = sorted( + surface_relevant & {rc["id"] for rc in full} + ) return record if len(full) == 1: root_cause_id = full[0]["id"] @@ -164,6 +193,9 @@ def _classify_finding( if partial: record["classification"] = "partial" record["candidate_root_cause_ids"] = [rc["id"] for rc in partial] + record["candidate_surface_relevant_root_cause_ids"] = sorted( + surface_relevant & {rc["id"] for rc in partial} + ) return record record["classification"] = "unexpected" @@ -200,6 +232,18 @@ def grade(expectation: dict[str, Any] | None, result: dict[str, Any]) -> dict[st for candidate_id in record["candidate_root_cause_ids"] } - claimed + # Referred-path relevance guard: the subset of `referred_ids` where some + # referring finding concretely named the candidate's surface, rather than + # only matching on accepted-formulation prose or being marked ambiguous. + # `claimed` is not re-subtracted here beyond what `referred_ids` already + # excludes, since relevant ids are always a subset of referred ids. + relevant_referred_ids = { + candidate_id + for record in records + if record["classification"] in {"partial", "ambiguous"} + for candidate_id in record.get("candidate_surface_relevant_root_cause_ids", []) + } & referred_ids + expected_ids = [rc["id"] for rc in root_causes] matched_ids = sorted(claimed) missed_ids = [ @@ -233,6 +277,14 @@ def grade(expectation: dict[str, Any] | None, result: dict[str, Any]) -> dict[st "matched_root_cause_ids": matched_ids, "missed_root_cause_ids": missed_ids, "referred_root_cause_ids": sorted(referred_ids), + # Referred-path relevance guard, settled by the repository owner in + # #53's preregistered v2 scoring gate: the subset of + # `referred_root_cause_ids` where a referring finding concretely named + # the candidate's actual surface. A referred candidate that only hit + # on prose (vaguer commentary echoing an accepted formulation without + # ever pointing at the right place) is excluded here even though it is + # still reported under `referred_root_cause_ids` above. + "referred_relevant_root_cause_ids": sorted(relevant_referred_ids), # Recall counts confirmed matches against the full expected set, never # crediting a referral - a referred root cause is neither a match nor # a scored miss, so it is absent from both the numerator and this @@ -241,6 +293,17 @@ def grade(expectation: dict[str, Any] | None, result: dict[str, Any]) -> dict[st # recall as one with genuine misses, and the referral bucket is what # tells the two apart. "recall": (len(matched_ids) / len(expected_ids)) if expected_ids else None, + # Combined matched+referred rate with the relevance guard applied: + # the preregistered v2 scoring gate's alternative path to a passing + # case when `recall` alone falls short. `matched_ids` and + # `relevant_referred_ids` are disjoint by construction (the latter is + # already `referred_ids - claimed`, and `referred_ids` itself excludes + # `claimed`), so a plain sum is exact, not an overcount. + "combined_recall": ( + (len(matched_ids) + len(relevant_referred_ids)) / len(expected_ids) + ) + if expected_ids + else None, "findings": records, "false_positive_finding_ids": [record["finding_id"] for record in gating], "accepted_finding_ids": [ diff --git a/review-suite/scripts/evals/report.py b/review-suite/scripts/evals/report.py index ea44727..2d18943 100644 --- a/review-suite/scripts/evals/report.py +++ b/review-suite/scripts/evals/report.py @@ -94,11 +94,22 @@ def _case_summary(case_id: str, attempts: list[dict[str, Any]]) -> dict[str, Any union: set[str] = set() intersection: set[str] | None = None referred_union: set[str] = set() + referred_relevant_union: set[str] = set() + combined_recalls: list[float] = [] for grade_record in grades: found = set(grade_record["matched_root_cause_ids"]) union |= found intersection = found if intersection is None else (intersection & found) referred_union |= set(grade_record["referred_root_cause_ids"]) + # `.get(..., [])` keeps this readable against a grade dict built before + # the referred-path relevance guard existed (e.g. hand-built test + # fixtures), which never claims the combined path passes. + referred_relevant_union |= set( + grade_record.get("referred_relevant_root_cause_ids", []) + ) + combined_recall = grade_record.get("combined_recall") + if combined_recall is not None: + combined_recalls.append(combined_recall) expected = grades[0]["expected_root_cause_ids"] if grades else [] return { @@ -114,6 +125,16 @@ def _case_summary(case_id: str, attempts: list[dict[str, Any]]) -> dict[str, Any # case. Reported separately so a reader never has to infer a referral # from the absence of a match. "ever_referred_root_cause_ids": sorted(referred_union), + # Referred-path relevance guard: the subset of the line above where a + # referring finding concretely named the root cause's actual surface, + # not merely prose the grader could not fully resolve either way. + "ever_referred_relevant_root_cause_ids": sorted(referred_relevant_union), + # The preregistered v2 scoring gate's alternative pass path: mean, per + # attempt, of (matched + relevance-guarded referred) / expected. A + # case may pass on `mean_recall` alone, on this alone, or on neither. + "mean_combined_recall": ( + statistics.fmean(combined_recalls) if combined_recalls else None + ), "verdict_stability": _modal_share([a["verdict"] for a in answered]), "finding_stability": _modal_share(matched_sets), "stability_denominator": len(answered), diff --git a/review-suite/scripts/tests/test_eval_grader.py b/review-suite/scripts/tests/test_eval_grader.py index cc6bf16..7a8e3fe 100644 --- a/review-suite/scripts/tests/test_eval_grader.py +++ b/review-suite/scripts/tests/test_eval_grader.py @@ -176,6 +176,39 @@ def test_a_partial_match_is_referred_for_adjudication(self): # silent reviewer-miss the owner-settled grading method forbids. self.assertEqual([], grade["missed_root_cause_ids"]) self.assertEqual(["rc.deadline"], grade["referred_root_cause_ids"]) + # Referred-path relevance guard: this referral fired on the right + # surface (`session.py:31` shares a token with `session.py:refresh`), + # so it concretely names where the root cause lives and counts as + # relevant even though the prose itself was not recognized. + self.assertEqual(["rc.deadline"], grade["referred_relevant_root_cause_ids"]) + self.assertEqual(1.0, grade["combined_recall"]) + + def test_a_referral_on_prose_alone_is_not_relevance_guard_eligible(self): + """Right formulation words, wrong location: referred but not concrete. + + The relevance guard exists precisely to keep this apart from the case + above: matching accepted-formulation prose without ever naming the + actual file, function, or symbol must not count toward the combined + matched+referred rate, even though the grader still refers it rather + than scoring it a miss. + """ + grade = grader.grade( + expectation("changes_required", [ROOT_CAUSE]), + result( + "changes_required", + [ + finding( + "correctness.one", + location="unrelated_module.py:4", + concern="A stale expiry clock.", + ) + ], + ), + ) + self.assertEqual("partial", classification_of(grade, "correctness.one")) + self.assertEqual(["rc.deadline"], grade["referred_root_cause_ids"]) + self.assertEqual([], grade["referred_relevant_root_cause_ids"]) + self.assertEqual(0.0, grade["combined_recall"]) def test_a_finding_matching_two_root_causes_is_ambiguous(self): grade = grader.grade( @@ -200,6 +233,12 @@ def test_a_finding_matching_two_root_causes_is_ambiguous(self): # scored miss for either. self.assertEqual([], grade["missed_root_cause_ids"]) self.assertEqual(["rc.deadline", "rc.record"], grade["referred_root_cause_ids"]) + # `full` always implies surface_hit, so an ambiguous candidate (two + # `full` matches) is always relevance-guard eligible for both. + self.assertEqual( + ["rc.deadline", "rc.record"], grade["referred_relevant_root_cause_ids"] + ) + self.assertEqual(1.0, grade["combined_recall"]) def test_a_root_cause_with_no_candidate_finding_is_a_genuine_miss(self): """Nothing pointed at it at all: this is the one case referral must not swallow.""" @@ -210,6 +249,8 @@ def test_a_root_cause_with_no_candidate_finding_is_a_genuine_miss(self): self.assertEqual(["rc.deadline"], grade["missed_root_cause_ids"]) self.assertEqual([], grade["referred_root_cause_ids"]) self.assertEqual(0.0, grade["recall"]) + self.assertEqual([], grade["referred_relevant_root_cause_ids"]) + self.assertEqual(0.0, grade["combined_recall"]) def test_an_unexpected_gating_finding_is_a_false_positive(self): grade = grader.grade( @@ -320,6 +361,29 @@ def test_a_file_extension_alone_is_not_a_surface_match(self): ), ) + def test_match_signals_splits_surface_and_prose_independently(self): + surface_only = grader.match_signals( + ROOT_CAUSE, + finding("correctness.one", concern="Something feels off here."), + ) + self.assertEqual((True, False), surface_only) + + signal_only = grader.match_signals( + ROOT_CAUSE, + finding( + "correctness.one", + location="unrelated_module.py:4", + concern="A stale expiry clock.", + ), + ) + self.assertEqual((False, True), signal_only) + + both = grader.match_signals( + ROOT_CAUSE, + finding("correctness.one", concern="A stale expiry clock."), + ) + self.assertEqual((True, True), both) + if __name__ == "__main__": unittest.main() diff --git a/review-suite/scripts/tests/test_eval_report.py b/review-suite/scripts/tests/test_eval_report.py index 2375180..59004d5 100644 --- a/review-suite/scripts/tests/test_eval_report.py +++ b/review-suite/scripts/tests/test_eval_report.py @@ -52,9 +52,16 @@ def grade( expected_ids=("rc.one",), matched_ids=("rc.one",), referred_ids=(), + referred_relevant_ids=None, false_positives=(), adjudication=(), ): + # Defaults to `referred_ids` so most callers, which are not exercising the + # relevance guard itself, get relevance-guard-eligible referrals without + # having to say so twice. + if referred_relevant_ids is None: + referred_relevant_ids = referred_ids + combined_ids = set(matched_ids) | set(referred_relevant_ids) return { "grader_version": "1.0", "expected_verdict": expected, @@ -68,7 +75,11 @@ def grade( i for i in expected_ids if i not in matched_ids and i not in referred_ids ], "referred_root_cause_ids": list(referred_ids), + "referred_relevant_root_cause_ids": list(referred_relevant_ids), "recall": (len(matched_ids) / len(expected_ids)) if expected_ids else None, + "combined_recall": ( + (len(combined_ids) / len(expected_ids)) if expected_ids else None + ), "findings": [], "false_positive_finding_ids": list(false_positives), "accepted_finding_ids": [], @@ -125,6 +136,61 @@ def test_a_referred_root_cause_is_reported_separately_from_a_miss(self): per_case = aggregate["per_case"][0] self.assertEqual(["rc.two"], per_case["ever_referred_root_cause_ids"]) + def test_referred_path_relevance_guard_is_reported_per_case(self): + """The preregistered v2 scoring gate's combined matched+referred path. + + A referral that never named the actual surface must not inflate the + combined rate, even though it still shows up in the plain referred + union above. + """ + aggregate = report.aggregate( + [ + attempt( + "subject-one", + 1, + grade=grade( + expected_ids=("rc.one",), + matched_ids=(), + referred_ids=("rc.one",), + referred_relevant_ids=(), + ), + ), + attempt( + "subject-one", + 2, + grade=grade( + expected_ids=("rc.one",), + matched_ids=(), + referred_ids=("rc.one",), + referred_relevant_ids=("rc.one",), + ), + ), + ], + configuration=CONFIGURATION, + ) + per_case = aggregate["per_case"][0] + self.assertEqual(["rc.one"], per_case["ever_referred_root_cause_ids"]) + # `ever_referred_relevant_root_cause_ids` is a union across attempts, + # like `ever_referred_root_cause_ids` above, so it still names rc.one + # (attempt 2 was relevant) - the guard's effect shows up in the rate + # below, not by hiding rc.one from this union. + self.assertEqual(["rc.one"], per_case["ever_referred_relevant_root_cause_ids"]) + # attempt 1: 0/1 relevant; attempt 2: 1/1 relevant -> mean 0.5 + self.assertEqual(0.5, per_case["mean_combined_recall"]) + + def test_combined_recall_gracefully_handles_grades_predating_the_guard(self): + """A grade dict without the new fields must not crash the report.""" + legacy_grade = grade(matched_ids=("rc.one",)) + del legacy_grade["referred_relevant_root_cause_ids"] + del legacy_grade["combined_recall"] + aggregate = report.aggregate( + [attempt("subject-one", 1, grade=legacy_grade)], + configuration=CONFIGURATION, + ) + per_case = aggregate["per_case"][0] + self.assertEqual([], per_case["ever_referred_relevant_root_cause_ids"]) + self.assertIsNone(per_case["mean_combined_recall"]) + def test_recall_averages_only_graded_attempts(self): aggregate = report.aggregate( [ From 2c671fd75f4bc11b3137f8d615764ac6c4e21851 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Tue, 28 Jul 2026 21:39:24 -0700 Subject: [PATCH 2/5] docs(review-suite): freeze the v2 re-score configuration for s1-correctness-orchestrator ## Summary - Add `review-suite/evals/v2/s1-rescore-frozen-configuration.json`: the exact, committed-before-scoring configuration for re-scoring `s1-correctness-orchestrator` against the two cases the frozen v1 baseline recorded as confident misses (`dependency-strictness-propagation`, `stale-claim-release-guard`) after the traversal and verification-sufficiency passes landed. - Pins: origin/main's current head at freeze time (85ccf13b45bad8f162d81963a3ac910ea0b49590), the grading-tooling commit adding the referred-path relevance guard (6195ada98a168a78ee4f7deccf0dd9bf0c204b43), runtime/model (claude, CLI 2.1.92, per gate-manifest.json's pin, verified via `claude --version`), 5 runs/case, 450s timeout, $12.00 cost ceiling, and the exact per-case quality/stability/non-regression gate from #53's issue body's "Preregistered v2 scoring gate" section (the repository owner's settled refinement of gate-manifest.json's proposal). ## Why - This is a bounded measurement step for #54: whether #53's two added correctness passes actually fixed the misses they were built for. Freezing the configuration before examining any scored output - same discipline the frozen v1 baseline and gate-manifest.json both already apply - is what keeps a later re-score from being tuned to a result seen in advance. - Only s1-correctness-orchestrator is in scope: s2/s3 are solution- and code-simplicity strata untouched by #53's correctness-only change, so re-scoring them would spend real money against this measurement's own ceiling to re-confirm a null result already expected by gate-manifest.json's non-regression floor. Co-Authored-By: Claude Sonnet 5 --- .../v2/s1-rescore-frozen-configuration.json | 78 +++++++++++++++++++ 1 file changed, 78 insertions(+) create mode 100644 review-suite/evals/v2/s1-rescore-frozen-configuration.json diff --git a/review-suite/evals/v2/s1-rescore-frozen-configuration.json b/review-suite/evals/v2/s1-rescore-frozen-configuration.json new file mode 100644 index 0000000..d27c300 --- /dev/null +++ b/review-suite/evals/v2/s1-rescore-frozen-configuration.json @@ -0,0 +1,78 @@ +{ + "record_version": "1.0", + "status": "frozen_before_scoring", + "status_detail": "Committed before any scored v2 output was examined, per this record's own rule below and per the frozen v1 baseline's precedent. This is a measurement-only record: it exists to determine whether #53's traversal and verification-sufficiency passes demonstrably fixed the two confident misses the frozen v1 baseline (review-suite/evals/baseline/v1/) recorded in s1-correctness-orchestrator. It does not gate #53's own merge (already merged as commit 85ccf13) and does not authorize, build, or decide anything about #54's independent-discovery architecture.", + "scope": { + "stratum_rescored": "s1-correctness-orchestrator only", + "why_only_this_stratum": "s2-solution-simplicity-lens and s3-code-simplicity-lens are solution-simplicity/code-simplicity strata untouched by #53's correctness-only change (consumer/impact-traversal and verification-sufficiency passes added to review-correctness alone). Re-scoring them would spend real money to re-confirm a null result the gate-manifest.json non-regression floor already expects (\"all 8 cases must replay with identical statuses to their v1 report\"); that check is deferred to whoever runs the full v2 gate, not repeated here.", + "target_cases": [ + "dependency-strictness-propagation", + "stale-claim-release-guard" + ], + "non_target_cases_in_same_stratum": [ + "dependency-hint-parser-coverage", + "optional-tool-probe", + "post-bootstrap-module-load", + "process-isolation-assertion", + "session-continuation-summary" + ] + }, + "predecessor_baseline": { + "v1_frozen_configuration": "review-suite/evals/baseline/v1/frozen-configuration.json", + "v1_s1_report": "review-suite/evals/baseline/v1/s1-correctness-orchestrator.report.json", + "v1_review_behaviour_commit": "16560d807c66076fcbf3f00d3a87f543c6ae2458", + "v1_scoring_suite_commit": "2644c12", + "v1_dependency_strictness_propagation_mean_recall": 0.0, + "v1_stale_claim_release_guard_mean_recall": 0.0, + "v1_verdict_stability_both_target_cases": 1.0, + "v1_finding_stability_both_target_cases": 1.0 + }, + "gate_source": { + "settled_by": "repository owner, in #53's issue body under \"Preregistered v2 scoring gate\" (a refinement of #59's gate-manifest.json proposal, not the proposal verbatim)", + "gate_manifest_reference": "review-suite/evals/v2/gate-manifest.json (proposal; #53's issue body is the settled version this record actually applies)", + "binding_quote": "These are binding on whichever ticket execution runs the v2 re-score of this implementation, not on this ticket's own merge gate." + }, + "suite_commit": { + "review_behaviour_commit": "85ccf13b45bad8f162d81963a3ac910ea0b49590", + "review_behaviour_commit_note": "origin/main's current head at freeze time, verified via `git rev-parse origin/main` immediately before this record was written - the exact commit '#53's traversal and verification-sufficiency passes' landed at (feat: add correctness traversal and verification-sufficiency passes (#83)). This is the commit whose skills/review-code-change, skills/review-solution-simplicity, skills/review-correctness, and skills/review-code-simplicity trees the runner's target_skill_documents digests and sends to the reviewer; it is unaffected by the grading-tooling commit below, which touches only review-suite/scripts/evals/.", + "scoring_run_commit_note": "The runner (review-suite/scripts/evals/runner.py:suite_commit()) records `git rev-parse HEAD` into every report's own `configuration.suite_commit` field automatically at launch time. That value is not predicted here - predicting a future commit hash before committing this file would be circular - and is instead read back from the produced review-suite/evals/v2/s1-correctness-orchestrator.report.json and restated in this record's companion analysis document once the run completes. No file content changes between the grading-tooling commit below and the scoring run, so that recorded commit is expected to equal, or be a direct descendant of, the grading-tooling commit with no intervening skill or corpus change." + }, + "grading_tooling_change": { + "commit": "6195ada98a168a78ee4f7deccf0dd9bf0c204b43", + "summary": "Added review-suite/scripts/evals/grader.py:match_signals (public surface_hit/signal_hit split) and grade() fields referred_relevant_root_cause_ids / combined_recall, plus the corresponding per-case fields in report.py. This is the mechanical referred-path relevance guard the gate requires - a referred candidate counts toward the combined matched+referred rate only when a referring finding's location/evidence shares a surface token with the expected root cause's surface, not merely when the grader classifies it partial or ambiguous.", + "grader_version": "1.0, unchanged - additive fields only, no existing classification or field changed, so no corpus's declared grader_version needs to move", + "verified_by": "review-suite/scripts/tests/test_eval_grader.py and test_eval_report.py; full `just test` run (245 review-suite tests, all passing) before this eval was launched" + }, + "runtime": { + "adapter": "review-suite/scripts/evals/claude_executor.py", + "adapter_version": "1.0", + "executor_command": "python3 review-suite/scripts/evals/claude_executor.py", + "runtime": "claude", + "runtime_cli_version": "2.1.92", + "runtime_cli_version_verified": "matches gate-manifest.json's runtime_model_stratum pin exactly; verified via `claude --version` immediately before launch", + "model": "resolved from the runtime's reported modelUsage on every attempt, exactly as v1 did; an attempt that cannot name its model is recorded as a runtime failure rather than a result", + "stratum_rule": "unchanged from v1 and from gate-manifest.json: a change of runtime, runtime version, or model creates a new, non-comparable stratum" + }, + "execution": { + "runs_per_case": 5, + "runs_per_case_rationale": "Kept identical to v1 for comparability of the two target cases across the v1/v2 boundary, per gate-manifest.json's explicit judgment call to prefer comparability over a larger sample.", + "timeout_seconds": 450, + "timeout_rationale": "Set unconditionally per #53's issue body: 'Set unconditionally to 450s (up from 300s) for the v2 re-score. Do not wait for a pilot run to approach the existing limit before raising it.' Not derived from an observed approach to 300s in this run; raised up front as instructed.", + "retry_policy": "none, unchanged from v1 - a failed attempt is an evaluation failure and is never retried", + "cost_ceiling_usd": 12.0, + "cost_ceiling_rationale": "Raised from v1's $9.00 per #53's issue body, sized from the v1 pilot's worst observed cold-attempt cost ($0.2021/attempt) with two added reasoning passes. This is also the delegated task's own hard ceiling for the whole measurement step, since only s1 is being re-scored.", + "cost_ceiling_exhaustion_policy": "unchanged from v1: exceeding the ceiling stops further runs and records an incomplete baseline; repetitions are never reduced after outputs are visible", + "corpus_version": "0.1-s1-populated, unchanged from v1 - no case content changed for this re-score, only the reviewed skill (post-#53) and the grading tooling above", + "invocation_template": "just eval-review-suite 'python3 review-suite/scripts/evals/claude_executor.py' --corpus review-suite/evals/strata/s1-correctness-orchestrator --runs 5 --timeout 450 --artifact-dir review-suite/evals/artifacts/s1-correctness-orchestrator/-0.1-s1-populated --attempts-out review-suite/evals/artifacts/s1-correctness-orchestrator/-0.1-s1-populated.attempts.jsonl --report-out review-suite/evals/v2/s1-correctness-orchestrator.report.json" + }, + "quality_stability_non_regression_thresholds": { + "per_case_target_gate": "For each of dependency-strictness-propagation and stale-claim-release-guard, independently: mean_recall >= 0.6 (>= 3 of 5 attempts matched) OR mean_combined_recall >= 0.8 (relevance-guarded matched+referred). One case's result must not be blended with or masked by the other's.", + "non_regression_floor": [ + "verdict_stability and finding_stability for both target cases must not fall below their v1 value of 1.0", + "dependency-hint-parser-coverage, post-bootstrap-module-load, session-continuation-summary must remain at zero false_positive_attempts", + "process-isolation-assertion's false_alarm_attempts must not exceed its v1-observed 2 of 5" + ] + }, + "committed_before_scoring": true, + "committed_before_scoring_note": "This file, and the grading-tooling commit it cites, are being committed to this branch before any attempt in the scored run below is launched, and before any of this run's output is examined - the same discipline v1's own frozen-configuration.json and gate-manifest.json both apply." +} From 50a02d1792b90e987abb818b1d6255b32d43304e Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Tue, 28 Jul 2026 22:42:50 -0700 Subject: [PATCH 3/5] fix(review-suite): recognize a symbol named only in prose as a surface hit ## Summary - `review-suite/scripts/evals/grader.py`: add `surface_named_in_prose(expected, finding)`, which checks whether the expectation's whole `surface` field, normalized to a phrase, appears as a contiguous substring of the finding's prose (`finding_text`: `rule`/`concern`/`impact`/`proposed_change`/ `expected_effect`, `evidence[].detail`). `match_signals`'s `surface_hit` is now `finding_surfaces` (structured `location` fields, unchanged) OR this new phrase check - purely additive, no existing hit is removed. - Bump `GRADER_VERSION` to `1.1` and every corpus's declared `grader_version` to match (`review-suite/evals/corpus/corpus.json`, `review-suite/evals/calibration/rollback-guidance-render.json`, and every `review-suite/evals/strata/*/corpus.json`) - a behavioral grading change, unlike the purely-additive-fields commit that preceded this one on this branch. - Add `review-suite/scripts/evals/regrade.py` (+ `review-suite/scripts/tests/test_eval_regrade.py`): re-grade already- captured raw attempts with the current grader, from a prior run's `--attempts-out` and `--artifact-dir`, without launching any executor process or spending anything new. Exposed as `just regrade-review-suite`. - Add grader tests for both the fix and its own precision boundary: a symbol named only in prose is now a surface hit (`test_a_symbol_named_only_in_prose_is_a_surface_hit`), but a lone common word incidentally shared with a multi-word surface is not (`test_a_lone_common_word_shared_with_the_surface_is_not_a_surface_hit`). ## Why - Reading raw real-runtime `s1-correctness-orchestrator` attempts for `dependency-strictness-propagation` and `stale-claim-release-guard` found 10/10 correctly-reasoned findings misclassified `unexpected` (false positive) instead of even `referred`. Every attempt names the exact expected symbol (`dependency_finalized`, `_release`) only in prose, while every structured `location`/`evidence[].location` field carries a bare file path with no symbol in it - `dependencies.py` (plural, from the path) never token-matches `dependency` (singular, from the surface field) under the prior location-only, exact-token surface check, so `surface_hit` was false even though a human reading the same prose immediately recognizes the correct root cause. - A first attempt at this fix (token-set union of all prose into `finding_surfaces`) over-corrected: it silently turned this repository's own `probe.partial-claim-wrong-surface` calibration boundary (a deliberately wrong-surface, partially-correct claim that must stay referred, not matched) into a full match, because its prose incidentally contains the single common word "guidance" - one token of the multi-word surface `render_rollback_guidance` - without ever naming the surface as a whole phrase. Requiring the *whole* normalized surface phrase to appear (mirroring how `_signal_match` already does phrase-substring matching for formulations) fixes the real case while leaving that calibration boundary intact; `just test` catching this immediately is exactly what the calibration suite exists for. - This is a real behavioral change to grading semantics, not merely new derived fields, so it is versioned as such: any score computed under `GRADER_VERSION` `1.0` (including the entire frozen v1 baseline) is not directly comparable to a score computed under `1.1`. Documented in a new, separate file under `review-suite/evals/v2/` in a following commit - this commit does not touch `gate-manifest.json`, `DECISION-RECORD.md`, `FAILURE-TAXONOMY.md`, or any audit file, all still frozen and not owned by this change. Co-Authored-By: Claude Sonnet 5 --- justfile | 7 + .../calibration/rollback-guidance-render.json | 2 +- review-suite/evals/corpus/corpus.json | 2 +- .../strata/pilot-code-simplicity/corpus.json | 2 +- .../strata/pilot-orchestrator/corpus.json | 2 +- .../pilot-solution-simplicity/corpus.json | 2 +- .../s1-correctness-orchestrator/corpus.json | 2 +- .../s2-solution-simplicity-lens/corpus.json | 2 +- .../s3-code-simplicity-lens/corpus.json | 2 +- review-suite/scripts/evals/grader.py | 69 ++++++-- review-suite/scripts/evals/regrade.py | 152 ++++++++++++++++++ .../scripts/tests/test_eval_grader.py | 63 ++++++++ .../scripts/tests/test_eval_regrade.py | 137 ++++++++++++++++ 13 files changed, 421 insertions(+), 23 deletions(-) create mode 100644 review-suite/scripts/evals/regrade.py create mode 100644 review-suite/scripts/tests/test_eval_regrade.py diff --git a/justfile b/justfile index 8c7dfb0..d92d2ea 100644 --- a/justfile +++ b/justfile @@ -59,6 +59,13 @@ audit-review-corpus: eval-review-suite executor *args: python3 review-suite/scripts/evals/runner.py --executor "{{executor}}" {{args}} +# Re-grade already-captured raw attempts with the current grader, spending no +# new money: no executor process is launched. Use after a grader change to +# correct a stratum's report from its retained `--artifact-dir` and +# `--attempts-out` without repeating the model calls that produced them. +regrade-review-suite *args: + python3 review-suite/scripts/evals/regrade.py {{args}} + test-plugins: python3 -m unittest discover -s scripts/tests -p 'test_*.py' diff --git a/review-suite/evals/calibration/rollback-guidance-render.json b/review-suite/evals/calibration/rollback-guidance-render.json index 1967e20..3c7ae80 100644 --- a/review-suite/evals/calibration/rollback-guidance-render.json +++ b/review-suite/evals/calibration/rollback-guidance-render.json @@ -1,7 +1,7 @@ { "calibration_version": "1.0", "case_id": "rollback-guidance-render", - "grader_version": "1.0", + "grader_version": "1.1", "source": "probe.observed-root-cause and probe.accepted-non-finding are the two findings of run 1 verbatim, byte for byte, from the retained artifact review-suite/evals/artifacts/pilot-orchestrator/1.1-pilot-orchestrator/rollback-guidance-render.run-1.stdout.json, produced at corpus version 1.1-pilot-orchestrator, model claude-opus-4-6[1m], suite commit 16560d807c66076fcbf3f00d3a87f543c6ae2458. That run is the post-calibration validation run, so this prose is not prose the shipped formulations were fitted to. The formulations were drawn from an earlier run at corpus version 1.0-pilot-orchestrator whose raw output is no longer retained; CALIBRATION.md records that loss and the fix. The remaining probes are constructed variants that probe one grading boundary each. No scored output was observed.", "probes": [ { diff --git a/review-suite/evals/corpus/corpus.json b/review-suite/evals/corpus/corpus.json index 3f19ae7..f53dd75 100644 --- a/review-suite/evals/corpus/corpus.json +++ b/review-suite/evals/corpus/corpus.json @@ -1,7 +1,7 @@ { "corpus_version": "0.1-protocol-proof", "protocol_version": "1.0", - "grader_version": "1.0", + "grader_version": "1.1", "target_skill": "review-code-change", "target_skill_dependencies": [ "review-solution-simplicity", diff --git a/review-suite/evals/strata/pilot-code-simplicity/corpus.json b/review-suite/evals/strata/pilot-code-simplicity/corpus.json index 67f3f13..cdcbfbb 100644 --- a/review-suite/evals/strata/pilot-code-simplicity/corpus.json +++ b/review-suite/evals/strata/pilot-code-simplicity/corpus.json @@ -1,7 +1,7 @@ { "corpus_version": "1.3-pilot-code-simplicity", "protocol_version": "1.0", - "grader_version": "1.0", + "grader_version": "1.1", "target_skill": "review-code-simplicity", "target_skill_dependencies": [], "stratum": { diff --git a/review-suite/evals/strata/pilot-orchestrator/corpus.json b/review-suite/evals/strata/pilot-orchestrator/corpus.json index 8bed5c1..afedceb 100644 --- a/review-suite/evals/strata/pilot-orchestrator/corpus.json +++ b/review-suite/evals/strata/pilot-orchestrator/corpus.json @@ -1,7 +1,7 @@ { "corpus_version": "1.3-pilot-orchestrator", "protocol_version": "1.0", - "grader_version": "1.0", + "grader_version": "1.1", "target_skill": "review-code-change", "target_skill_dependencies": [ "review-solution-simplicity", diff --git a/review-suite/evals/strata/pilot-solution-simplicity/corpus.json b/review-suite/evals/strata/pilot-solution-simplicity/corpus.json index 7b7a2f0..a845af4 100644 --- a/review-suite/evals/strata/pilot-solution-simplicity/corpus.json +++ b/review-suite/evals/strata/pilot-solution-simplicity/corpus.json @@ -1,7 +1,7 @@ { "corpus_version": "1.3-pilot-solution-simplicity", "protocol_version": "1.0", - "grader_version": "1.0", + "grader_version": "1.1", "target_skill": "review-solution-simplicity", "target_skill_dependencies": [], "stratum": { diff --git a/review-suite/evals/strata/s1-correctness-orchestrator/corpus.json b/review-suite/evals/strata/s1-correctness-orchestrator/corpus.json index d0c3cb7..456993d 100644 --- a/review-suite/evals/strata/s1-correctness-orchestrator/corpus.json +++ b/review-suite/evals/strata/s1-correctness-orchestrator/corpus.json @@ -1,7 +1,7 @@ { "corpus_version": "0.1-s1-populated", "protocol_version": "1.0", - "grader_version": "1.0", + "grader_version": "1.1", "target_skill": "review-code-change", "target_skill_dependencies": [ "review-solution-simplicity", diff --git a/review-suite/evals/strata/s2-solution-simplicity-lens/corpus.json b/review-suite/evals/strata/s2-solution-simplicity-lens/corpus.json index 6b92276..074a85a 100644 --- a/review-suite/evals/strata/s2-solution-simplicity-lens/corpus.json +++ b/review-suite/evals/strata/s2-solution-simplicity-lens/corpus.json @@ -1,7 +1,7 @@ { "corpus_version": "0.1-s2-populated", "protocol_version": "1.0", - "grader_version": "1.0", + "grader_version": "1.1", "target_skill": "review-solution-simplicity", "target_skill_dependencies": [], "stratum": { diff --git a/review-suite/evals/strata/s3-code-simplicity-lens/corpus.json b/review-suite/evals/strata/s3-code-simplicity-lens/corpus.json index df6c02c..013c5e9 100644 --- a/review-suite/evals/strata/s3-code-simplicity-lens/corpus.json +++ b/review-suite/evals/strata/s3-code-simplicity-lens/corpus.json @@ -1,7 +1,7 @@ { "corpus_version": "0.1-s3-populated", "protocol_version": "1.0", - "grader_version": "1.0", + "grader_version": "1.1", "target_skill": "review-code-simplicity", "target_skill_dependencies": [], "stratum": { diff --git a/review-suite/scripts/evals/grader.py b/review-suite/scripts/evals/grader.py index a0b8636..45d4784 100644 --- a/review-suite/scripts/evals/grader.py +++ b/review-suite/scripts/evals/grader.py @@ -18,7 +18,7 @@ import re from typing import Any -GRADER_VERSION = "1.0" +GRADER_VERSION = "1.1" #: How one observed finding relates to the private expectation. CLASSIFICATIONS = ( @@ -82,8 +82,17 @@ def _surface_tokens(text: str) -> set[str]: } +def finding_text(finding: dict[str, Any]) -> str: + """Return the normalized prose a formulation may be found in.""" + parts = [str(finding.get(field) or "") for field in FINDING_TEXT_FIELDS] + parts.extend( + str(item.get("detail") or "") for item in finding.get("evidence") or [] + ) + return normalize(" ".join(parts)) + + def finding_surfaces(finding: dict[str, Any]) -> set[str]: - """Return the surface tokens a finding points at.""" + """Return the surface tokens a finding's structured location points at.""" locations = [finding.get("location") or ""] locations.extend( item.get("location") or "" for item in finding.get("evidence") or [] @@ -94,34 +103,64 @@ def finding_surfaces(finding: dict[str, Any]) -> set[str]: return tokens -def finding_text(finding: dict[str, Any]) -> str: - """Return the normalized prose a formulation may be found in.""" - parts = [str(finding.get(field) or "") for field in FINDING_TEXT_FIELDS] - parts.extend( - str(item.get("detail") or "") for item in finding.get("evidence") or [] - ) - return normalize(" ".join(parts)) - - def _signal_match(formulations: list[str], text: str) -> bool: return any(normalize(item) and normalize(item) in text for item in formulations) +def surface_named_in_prose(expected: dict[str, Any], finding: dict[str, Any]) -> bool: + """True when the finding's own prose names the expected surface outright. + + Checks whether the expectation's whole `surface` string, normalized to a + phrase (`dependency_finalized` -> `"dependency finalized"`), appears as a + contiguous substring of the finding's prose (`finding_text`: `rule`, + `concern`, `impact`, `proposed_change`, `expected_effect`, + `evidence[].detail`) - the same substring-of-normalized-text test + `_signal_match` already uses for formulations, applied to the surface + field instead. + + This is deliberately a whole-phrase check, not a token-set union with + `finding_surfaces`. A real-runtime reviewer routinely names the exact + affected symbol only in prose ("`x` still calls `y` without the strict + flag") while every structured `location` field carries a bare file path + with no symbol in it - `finding_surfaces` alone then misses a + concretely-correct finding entirely (discovered by reading raw attempts + where 10/10 correctly-reasoned findings on two real cases were + misclassified `unexpected` for exactly this reason). But a naive token-set + union of prose into `finding_surfaces` is too loose: this repository's own + `probe.partial-claim-wrong-surface` calibration case deliberately puts a + finding at the wrong surface (`storectl/cli.py`, not + `render_rollback_guidance`) whose prose incidentally contains the single + common word "guidance" ("the guidance cannot run") - a token-set union + would credit that coincidence as a surface hit and silently break a + calibration boundary built to keep a partially-correct, wrong-surface + claim referred rather than matched. Requiring the whole normalized surface + phrase to appear keeps that boundary intact (a lone "guidance" is not + "render rollback guidance") while still catching a real reviewer that + plainly names the exact symbol. + """ + surface = normalize(expected.get("surface", "")) + return bool(surface) and surface in finding_text(finding) + + def match_signals( expected: dict[str, Any], finding: dict[str, Any] ) -> tuple[bool, bool]: """Return `(surface_hit, signal_hit)` for one expectation/finding pair. - `surface_hit` is the concrete signal: the finding's location or evidence - names a token from the expectation's `surface` - the actual file, - function, or symbol the root cause turns on. `signal_hit` is the + `surface_hit` is the concrete signal: the finding's location, evidence, or + own prose names the expectation's `surface` - the actual file, function, + or symbol the root cause turns on - via `finding_surfaces` (structured + locations) or `surface_named_in_prose` (the whole surface phrase named + outright in the finding's explanation). `signal_hit` is the accepted-formulation prose match alone, which vaguer commentary can satisfy without ever naming where the defect lives. Split out from `match_strength` so a referred (partial or ambiguous) candidate can be judged concrete or not, rather than only judged matched or not. """ surface = _surface_tokens(expected.get("surface", "")) - surface_hit = bool(surface & finding_surfaces(finding)) + surface_hit = bool(surface & finding_surfaces(finding)) or surface_named_in_prose( + expected, finding + ) signal_hit = _signal_match( expected.get("equivalent_formulations") or [], finding_text(finding) ) diff --git a/review-suite/scripts/evals/regrade.py b/review-suite/scripts/evals/regrade.py new file mode 100644 index 0000000..a69dc1e --- /dev/null +++ b/review-suite/scripts/evals/regrade.py @@ -0,0 +1,152 @@ +#!/usr/bin/env python3 +"""Re-grade already-captured raw attempts with the currently shipped grader. + +`runner.py` couples execution (spawning a fresh executor process, which costs +real money) and grading (pure, free, deterministic) into one pass. When the +grader changes - fixing a real defect, or any other reason - every stratum's +already-retained raw output (`--artifact-dir`) can be re-graded for free, +without spending anything on new model calls, by replaying each retained +`.run-.stdout.json` artifact through the current +`grader.grade`/`report.aggregate` and rebuilding the report from it. + +This is deliberately a separate, narrow tool rather than a `runner.py` flag: +it never launches a process, never talks to an executor, and takes its +attempts from `--attempts-in` (a prior run's `--attempts-out` file) plus the +raw artifacts that run retained, rather than from a live corpus replay. + +Usage: + python3 regrade.py --corpus review-suite/evals/strata/ \\ + --attempts-in .attempts.jsonl \\ + --artifact-dir \\ + --report-out +""" + +from __future__ import annotations + +import argparse +import json +import sys +from pathlib import Path +from typing import Any + +if __package__ in (None, ""): # direct `python3 regrade.py` invocation + sys.path.insert(0, str(Path(__file__).resolve().parents[1])) + from evals import corpus, grader, protocol, report +else: + from . import corpus, grader, protocol, report + + +class RegradeError(ValueError): + """Raised when a prior attempt cannot be re-graded from what is on disk.""" + + +def _artifact_stem(case_id: str, run_number: int) -> str: + return f"{case_id}.run-{run_number}" + + +def regrade_attempts( + *, corpus_root: Path | None, attempts_in: Path, artifact_dir: Path +) -> tuple[list[dict[str, Any]], corpus.Corpus]: + """Rebuild every gradable attempt's `grade` from its retained raw artifact. + + Every other attempt field (status, usage, latency, executor identity, ...) + is carried over unchanged from `attempts_in`: none of that describes + grading, so none of it can be stale just because the grader changed. + """ + loaded = corpus.load_corpus(corpus_root) + expectations = {case.case_id: case.expectation for case in loaded.cases} + + attempts: list[dict[str, Any]] = [] + for line_number, line in enumerate(attempts_in.read_text().splitlines(), start=1): + if not line.strip(): + continue + attempt = dict(json.loads(line)) + if attempt["status"] in protocol.GRADABLE_STATUSES: + case_id = attempt["case_id"] + if case_id not in expectations: + raise RegradeError( + f"{attempts_in}:{line_number}: {case_id!r} is not a case in " + f"{corpus_root or corpus.DEFAULT_CORPUS}" + ) + stem = _artifact_stem(case_id, attempt["run_number"]) + raw_path = artifact_dir / f"{stem}.stdout.json" + if not raw_path.is_file(): + raise RegradeError(f"missing retained raw artifact {raw_path}") + response = json.loads(raw_path.read_text()) + attempt["grade"] = grader.grade(expectations[case_id], response["result"]) + attempts.append(attempt) + return attempts, loaded + + +def _configuration( + attempts: list[dict[str, Any]], + loaded: corpus.Corpus, + *, + attempts_in: Path, + artifact_dir: Path, +) -> dict[str, Any]: + sample = attempts[0] if attempts else {} + return { + "executor": None, + "corpus_version": loaded.corpus_version, + "grader_version": grader.GRADER_VERSION, + "target_skill": loaded.target_skill, + "target_skill_dependencies": list(loaded.target_skill_dependencies), + "stratum": loaded.stratum, + "suite_commit": sample.get("suite_commit"), + "runs_per_case": max((a["run_number"] for a in attempts), default=0), + "cases": len(loaded.cases), + "regraded": True, + "regraded_from_attempts": str(attempts_in), + "regraded_from_artifact_dir": str(artifact_dir), + "regraded_grader_version": grader.GRADER_VERSION, + "regrade_note": ( + "No executor process ran in this pass. Every non-grading attempt " + "field is carried over unchanged from the source attempts file; " + "only `grade` was recomputed, from the retained raw artifact, " + "using the grader version above." + ), + } + + +def build_parser() -> argparse.ArgumentParser: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--corpus", type=Path, default=None) + parser.add_argument("--attempts-in", type=Path, required=True) + parser.add_argument("--artifact-dir", type=Path, required=True) + parser.add_argument("--report-out", type=Path, default=None) + return parser + + +def main(argv: list[str] | None = None) -> int: + args = build_parser().parse_args(argv) + try: + attempts, loaded = regrade_attempts( + corpus_root=args.corpus, + attempts_in=args.attempts_in, + artifact_dir=args.artifact_dir, + ) + except (RegradeError, corpus.CorpusError, grader.GradingError) as error: + print(f"regrade rejected: {error}", file=sys.stderr) + return 2 + + configuration = _configuration( + attempts, + loaded, + attempts_in=args.attempts_in, + artifact_dir=args.artifact_dir, + ) + aggregate = report.aggregate(attempts, configuration=configuration) + + if args.report_out: + args.report_out.parent.mkdir(parents=True, exist_ok=True) + args.report_out.write_text( + json.dumps(aggregate, indent=2, sort_keys=True) + "\n" + ) + + print(json.dumps(aggregate, indent=2, sort_keys=True)) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/review-suite/scripts/tests/test_eval_grader.py b/review-suite/scripts/tests/test_eval_grader.py index 7a8e3fe..ac40771 100644 --- a/review-suite/scripts/tests/test_eval_grader.py +++ b/review-suite/scripts/tests/test_eval_grader.py @@ -361,6 +361,69 @@ def test_a_file_extension_alone_is_not_a_surface_match(self): ), ) + def test_a_symbol_named_only_in_prose_is_a_surface_hit(self): + """Regression: real-runtime attempts named the exact symbol only in + prose (`concern`/`evidence[].detail`), with every `location` field + carrying a bare file path that shares no token with the expectation's + `surface` - discovered when 10/10 raw attempts on two real cases were + misclassified `unexpected` despite correctly identifying the exact + root cause, because `finding_surfaces` read only `location` fields. + """ + root_cause = dict(ROOT_CAUSE, surface="dependency_finalized") + real_shaped_finding = finding( + "corr.dependency-finalized-missing-strict-proof", + location=None, + concern=( + "`dependency_finalized` still uses the permissive integration " + "check for closed records." + ), + ) + real_shaped_finding["evidence"] = [ + {"location": "pipeline/dependencies.py", "detail": "no strict flag"}, + {"location": "pipeline/integration.py:21-23", "detail": "default False"}, + ] + surface_hit, signal_hit = grader.match_signals(root_cause, real_shaped_finding) + self.assertTrue(surface_hit) + # The accepted formulations are deliberately uncalibrated against real + # prose (paraphrase, not verbatim), so this stays a referred `partial` + # match rather than a silently-invented `full` one - the fix widens + # where a surface can be found, not what counts as a formulation. + self.assertFalse(signal_hit) + self.assertEqual( + "partial", grader.match_strength(root_cause, real_shaped_finding) + ) + + def test_a_lone_common_word_shared_with_the_surface_is_not_a_surface_hit(self): + """The precision boundary the phrase-based prose check exists for. + + `probe.partial-claim-wrong-surface` (review-suite/evals/calibration/ + rollback-guidance-render.json) deliberately points a finding at the + wrong surface whose prose happens to contain the single common word + "guidance" - one token of the multi-word expected surface + `render_rollback_guidance` - without ever naming the surface as a + whole phrase. A token-set union of prose into `finding_surfaces` + would credit that coincidence as a surface hit and turn a + deliberately partial, wrong-surface claim into a full match. The + surface must be named as a phrase, not merely echoed one word at a + time, to stay a `partial` referral. + """ + root_cause = dict(ROOT_CAUSE, surface="render_rollback_guidance") + wrong_surface_finding = finding( + "corr.cli-surface-incomplete", + severity="strong_recommendation", + location="storectl/cli.py", + concern="The command registry is missing an export operation.", + detail=( + "The registry is the reason the guidance cannot run: the " + "export subcommand does not exist." + ), + ) + self.assertFalse( + grader.surface_named_in_prose(root_cause, wrong_surface_finding) + ) + surface_hit, _ = grader.match_signals(root_cause, wrong_surface_finding) + self.assertFalse(surface_hit) + def test_match_signals_splits_surface_and_prose_independently(self): surface_only = grader.match_signals( ROOT_CAUSE, diff --git a/review-suite/scripts/tests/test_eval_regrade.py b/review-suite/scripts/tests/test_eval_regrade.py new file mode 100644 index 0000000..c65bbe2 --- /dev/null +++ b/review-suite/scripts/tests/test_eval_regrade.py @@ -0,0 +1,137 @@ +"""Tests for re-grading already-captured raw attempts without re-running them.""" + +from __future__ import annotations + +import json +import shlex +import shutil +import sys +import tempfile +import unittest +from pathlib import Path + +SCRIPTS_DIR = Path(__file__).resolve().parents[1] +if str(SCRIPTS_DIR) not in sys.path: + sys.path.insert(0, str(SCRIPTS_DIR)) + +from evals import grader, protocol, regrade, runner # noqa: E402 + +FIXTURE_EXECUTOR = SCRIPTS_DIR / "evals" / "fixture_executor.py" + + +def fixture_command(mode: str | None = None) -> str: + command = f"{sys.executable} {FIXTURE_EXECUTOR}" + return f"{command} --mode {mode}" if mode else command + + +class RegradeTests(unittest.TestCase): + def setUp(self): + self.temp = Path(tempfile.mkdtemp()) + self.addCleanup(shutil.rmtree, self.temp, ignore_errors=True) + self.artifact_dir = self.temp / "artifacts" + self.attempts_path = self.temp / "attempts.jsonl" + + attempts, _ = runner.evaluate( + shlex.split(fixture_command()), + corpus_root=None, + runs=1, + timeout=60.0, + max_output_bytes=runner.DEFAULT_MAX_OUTPUT_BYTES, + artifact_dir=self.artifact_dir, + ) + self.source_attempts = attempts + self.attempts_path.write_text( + "".join(json.dumps(a, sort_keys=True) + "\n" for a in attempts) + ) + + def test_regrading_reproduces_the_original_grade(self): + """No executor ran; every gradable attempt's grade is rebuilt from the + retained raw artifact, and must land on exactly what a fresh + `grader.grade` call on that same artifact produces.""" + regraded, _loaded = regrade.regrade_attempts( + corpus_root=None, + attempts_in=self.attempts_path, + artifact_dir=self.artifact_dir, + ) + self.assertEqual(len(self.source_attempts), len(regraded)) + gradable = [ + a for a in self.source_attempts if a["status"] in protocol.GRADABLE_STATUSES + ] + self.assertTrue(gradable, "fixture executor produced no gradable attempt") + for original, rebuilt in zip( + ( + a + for a in self.source_attempts + if a["status"] in protocol.GRADABLE_STATUSES + ), + (a for a in regraded if a["status"] in protocol.GRADABLE_STATUSES), + ): + self.assertEqual(original["case_id"], rebuilt["case_id"]) + self.assertEqual(original["run_number"], rebuilt["run_number"]) + self.assertIsNotNone(rebuilt["grade"]) + self.assertEqual(grader.GRADER_VERSION, rebuilt["grade"]["grader_version"]) + + def test_non_gradable_attempts_are_carried_over_untouched(self): + """A blocked or failed attempt has no raw result to re-grade from, so + its `grade` (already `None`) must not be touched or looked up.""" + regraded, _loaded = regrade.regrade_attempts( + corpus_root=None, + attempts_in=self.attempts_path, + artifact_dir=self.artifact_dir, + ) + for attempt in regraded: + if attempt["status"] not in protocol.GRADABLE_STATUSES: + self.assertIsNone(attempt["grade"]) + + def test_a_case_id_absent_from_the_given_corpus_is_rejected(self): + """Regrading against the wrong corpus must fail loudly, not silently + skip or mis-grade a case it cannot find an expectation for.""" + tampered = self.temp / "tampered.jsonl" + lines = self.attempts_path.read_text().splitlines() + attempt = json.loads(lines[0]) + attempt["case_id"] = "no-such-case" + lines[0] = json.dumps(attempt, sort_keys=True) + tampered.write_text("\n".join(lines) + "\n") + + with self.assertRaises(regrade.RegradeError): + regrade.regrade_attempts( + corpus_root=None, + attempts_in=tampered, + artifact_dir=self.artifact_dir, + ) + + def test_a_missing_retained_artifact_is_rejected(self): + """Regrading depends entirely on the raw artifact still being on disk; + a gradable attempt whose file was never retained (or was cleaned up) + must fail rather than silently produce no grade.""" + empty_dir = self.temp / "empty-artifacts" + empty_dir.mkdir() + with self.assertRaises(regrade.RegradeError): + regrade.regrade_attempts( + corpus_root=None, + attempts_in=self.attempts_path, + artifact_dir=empty_dir, + ) + + def test_regrade_report_marks_itself_as_regraded(self): + """The produced report must be self-describing: a reader must never + mistake a re-graded report for a fresh execution's report.""" + regraded, loaded = regrade.regrade_attempts( + corpus_root=None, + attempts_in=self.attempts_path, + artifact_dir=self.artifact_dir, + ) + configuration = regrade._configuration( + regraded, + loaded, + attempts_in=self.attempts_path, + artifact_dir=self.artifact_dir, + ) + self.assertTrue(configuration["regraded"]) + self.assertEqual( + grader.GRADER_VERSION, configuration["regraded_grader_version"] + ) + + +if __name__ == "__main__": + unittest.main() From 19a8169435b4393cc25dbfeb22fd37801a945dfd Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Tue, 28 Jul 2026 23:02:39 -0700 Subject: [PATCH 4/5] docs(review-suite): report the fixed-grader pre/post-#53 comparison for s1 ## Summary - `review-suite/evals/v2/s1-correctness-orchestrator.report.json`: the post-#53 (commit `2c671fd`) 35-attempt run, re-graded with `GRADER_VERSION` `1.1` via `regrade.py` (no new executor calls). - `review-suite/evals/v2/pre53-s1-correctness-orchestrator-2case.report.json`: a fresh 10-attempt run (5 runs each) of `dependency-strictness-propagation` and `stale-claim-release-guard` against the pre-#53 correctness lens (commit `8e4fdbd`, #52's merge), graded with the same fixed grader, from a temporary 2-case subset corpus built from the two cases' unmodified packet/expectation/provenance files. - `review-suite/evals/v2/GRADER-1.1-COMPARABILITY.md`: what the grader fix changed, why the first attempted fix was too loose, and the comparability rule it creates (a `GRADER_VERSION` change is a new, non-comparable stratum, same as a runtime/model change). - `review-suite/evals/v2/s1-rescore-pre-post-comparison.md`: the plain comparison - four numbers (pre-#53/post-#53 x the two target cases), non-regression floor check, spend, and a recommendation (not a decision) for the repository owner. ## Result Both target cases pass the preregistered v2 scoring gate (`mean_recall >= 0.6` OR guarded `mean_combined_recall >= 0.8`) in both eras: `mean_combined_recall: 1.0` for all four (case x era) combinations, every one of the 20 underlying attempts individually scoring `combined_recall: 1.0`. The non-regression floor holds throughout. Total spend across the whole measurement (35 post-#53 + 10 pre-#53 attempts): $4.38 of the $12.00 ceiling. This shows the pre-#53 reviewer already found both defects, every time, before #53 existed - the frozen v1 baseline's "confident miss" on these two cases reads as a grader artifact (the same location-only surface-matching blind spot `GRADER-1.1-COMPARABILITY.md` documents), not a demonstrated reviewer capability gap. Neither `baseline/v1/` nor `gate-manifest.json`, `DECISION-RECORD.md`, `FAILURE-TAXONOMY.md`, or any audit file is modified; this adds new files alongside them. Co-Authored-By: Claude Sonnet 5 --- .../evals/v2/GRADER-1.1-COMPARABILITY.md | 114 +++++ ...correctness-orchestrator-2case.report.json | 250 +++++++++++ .../s1-correctness-orchestrator.report.json | 419 ++++++++++++++++++ .../v2/s1-rescore-pre-post-comparison.md | 171 +++++++ 4 files changed, 954 insertions(+) create mode 100644 review-suite/evals/v2/GRADER-1.1-COMPARABILITY.md create mode 100644 review-suite/evals/v2/pre53-s1-correctness-orchestrator-2case.report.json create mode 100644 review-suite/evals/v2/s1-correctness-orchestrator.report.json create mode 100644 review-suite/evals/v2/s1-rescore-pre-post-comparison.md diff --git a/review-suite/evals/v2/GRADER-1.1-COMPARABILITY.md b/review-suite/evals/v2/GRADER-1.1-COMPARABILITY.md new file mode 100644 index 0000000..038a990 --- /dev/null +++ b/review-suite/evals/v2/GRADER-1.1-COMPARABILITY.md @@ -0,0 +1,114 @@ +# Grader `1.1`: a real defect fix, and what it breaks comparability with + +This record exists because a scored measurement run under this directory (the +`s1-correctness-orchestrator` v2 re-score for #54's evidence gate) found a +genuine defect in `review-suite/scripts/evals/grader.py` mid-run, not because +any child of #51-#57 planned a grader change. It documents the fix and, per the +repository owner's explicit instruction, extends the existing +runtime/model-change comparability principle to grader-version changes: **a +`GRADER_VERSION` change creates a new, non-comparable stratum, exactly as a +runtime or model change does.** + +This file does not modify `gate-manifest.json`, `DECISION-RECORD.md`, +`FAILURE-TAXONOMY.md`, or any file under `audits/` - all remain exactly as #59 +delivered them. + +## What changed, in one sentence + +`finding_surfaces()` (surface-hit detection) read only structured `location` +fields; grader `1.1` adds `surface_named_in_prose()`, which also credits a +finding whose prose names the expected surface as a whole phrase, even when +every `location` field is a bare file path with no symbol in it. + +## How this was found + +While re-scoring `s1-correctness-orchestrator` post-#53 (commit `2c671fd`), +reading raw attempts directly (prompted by an independent check against this +report) showed all 5 attempts on `dependency-strictness-propagation` and all 5 +on `stale-claim-release-guard` correctly, concretely, and consistently +identifying the exact expected root cause - naming the right symbol, the right +file, the right causal mechanism, and (for the traversal case) correctly +populating `consumer_impact_evidence` for both call sites. Yet every one of +these 10 attempts was graded `unexpected` (a false positive), never even +reaching `referred`. + +The mechanism: `dependency_finalized`, the expectation's `surface` field, never +appeared in any attempt's `location` fields - every attempt put file paths there +(`pipeline/dependencies.py`) and named the symbol only in prose +(`` `dependency_finalized` still uses the permissive integration check ``). +`"dependencies"` (plural, from the path) is a different token than +`"dependency"` (singular, from the surface field) under the grader's +exact-token, no-stemming location matching, and `"finalized"` never appeared in +any location string at all. The equivalent-formulations list for both cases is +deliberately uncalibrated against real prose by corpus design (see +`CALIBRATION.md`), so `signal_hit` was false too - both signals false yields +`match_strength: none`, which is `unexpected`, not `partial`. + +This is not new to `#53` or to this measurement: it is present in `grader.py` +since its original commit, predates every `#51`-`#57` child, and was silently +present in the frozen v1 baseline's own scoring of these same two cases (see +`s1-rescore-pre-post-comparison.md` in this directory for what that means for +v1's "confident miss" reading). + +## Why the first attempted fix was wrong, and what shipped instead + +The first fix folded prose tokens directly into `finding_surfaces()` as a +token-set union. It correctly fixed both real cases, but it also broke this +repository's own `probe.partial-claim-wrong-surface` calibration case +(`review-suite/evals/calibration/rollback-guidance-render.json`): that probe +deliberately places a finding at the *wrong* surface (`storectl/cli.py`, not +`render_rollback_guidance`) whose prose incidentally contains the single common +word "guidance" ("the guidance cannot run"). A token-set union credited that +coincidence as a surface hit and silently turned a calibrated `partial` +(correctly referred, not matched) into a `full` match - `just test` failed +immediately (`test_eval_calibration.py`), which is exactly what the calibration +suite exists to catch. + +The shipped fix instead requires the expectation's whole `surface` string, +normalized to a phrase, to appear as a contiguous substring of the finding's +prose (`surface_named_in_prose`) - the same substring-of-normalized-text test +`_signal_match` already uses for formulations, applied to the surface field. +`"dependency finalized"` and `"release"` (the single meaningful token of +`_release`) each appear as whole phrases in the real attempts; a lone +`"guidance"` does not satisfy `"render rollback guidance"`. Both the real fix +and this precision boundary are covered by dedicated tests in +`review-suite/scripts/tests/test_eval_grader.py` +(`test_a_symbol_named_only_in_prose_is_a_surface_hit`, +`test_a_lone_common_word_shared_with_the_surface_is_not_a_surface_hit`), and the +pre-existing calibration suite continues to pass unchanged. + +## Version and comparability + +- `GRADER_VERSION`: `1.0` -> `1.1` (commit `50a02d1`), additive-behavior change + (a new way to reach `surface_hit: true`; nothing that previously hit stops + hitting). +- Every corpus's declared `grader_version` was bumped to `1.1` in the same + commit so `runner.py`/`audit_corpus.py`'s existing version-match gate keeps + working; no corpus content (cases, packets, expectations) changed. +- **Comparability rule, extended per the repository owner's explicit + instruction:** exactly as "a change of runtime, runtime version, or model + creates a new stratum" (`frozen-configuration.json`, `gate-manifest.json`), a + change of `GRADER_VERSION` also creates a new, non-comparable stratum. A score + computed under `1.0` - including every figure in + `review-suite/evals/baseline/v1/` - must never be compared directly against a + score computed under `1.1`. Any future comparison spanning this boundary must + say so explicitly, exactly as the existing runtime/model rule already + requires. +- `review-suite/scripts/evals/regrade.py` exists precisely to make a + grader-version change auditable without new spend: it re-grades an + already-captured raw attempt set (`--attempts-in` + `--artifact-dir`) with + whatever grader is currently shipped, and its output report always records + `configuration.regraded: true` and `configuration.regraded_grader_version`, so + a regraded report can never be mistaken for a fresh execution under the newer + grader. + +## What this does not do + +- Does not re-derive or restate v1's own frozen numbers - `baseline/v1/` is + untouched. +- Does not decide whether v1 should be re-scored wholesale under `1.1`, or + whether any other stratum's cases need re-examination for the same + location-only-surface blind spot. That is a separate, larger judgment call for + the repository owner; this record only fixes the specific defect this + measurement's raw evidence demonstrated and states the comparability + consequence. diff --git a/review-suite/evals/v2/pre53-s1-correctness-orchestrator-2case.report.json b/review-suite/evals/v2/pre53-s1-correctness-orchestrator-2case.report.json new file mode 100644 index 0000000..ae84421 --- /dev/null +++ b/review-suite/evals/v2/pre53-s1-correctness-orchestrator-2case.report.json @@ -0,0 +1,250 @@ +{ + "adjudication_required": [ + { + "candidate_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "case_id": "dependency-strictness-propagation", + "finding_id": "dep-finalized-missing-strict-proof", + "reason": "partial", + "run_number": 1 + }, + { + "candidate_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "case_id": "dependency-strictness-propagation", + "finding_id": "dep-finalized-missing-strict-proof", + "reason": "partial", + "run_number": 2 + }, + { + "candidate_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "case_id": "dependency-strictness-propagation", + "finding_id": "corr.dependency-finalized-missing-strict-proof", + "reason": "partial", + "run_number": 3 + }, + { + "candidate_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "case_id": "dependency-strictness-propagation", + "finding_id": "dep-finalized-missing-strict-proof", + "reason": "partial", + "run_number": 4 + }, + { + "candidate_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "case_id": "dependency-strictness-propagation", + "finding_id": "missing-strict-proof-in-dependency-finalized", + "reason": "partial", + "run_number": 5 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "reaper.null-owner-bypass", + "reason": "partial", + "run_number": 1 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "reaper.null-owner-bypass", + "reason": "partial", + "run_number": 2 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "reaper.null-owner-bypass", + "reason": "partial", + "run_number": 3 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "corr.owner-none-bypass", + "reason": "partial", + "run_number": 4 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "corr.missing-none-owner-test", + "reason": "partial", + "run_number": 4 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "reaper-null-owner-bypass", + "reason": "partial", + "run_number": 5 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "missing-null-owner-test", + "reason": "partial", + "run_number": 5 + } + ], + "attempts": 10, + "baseline_eligible": true, + "configuration": { + "cases": 2, + "corpus_version": "0.1-s1-pre53-2case", + "executor": null, + "grader_version": "1.1", + "regrade_note": "No executor process ran in this pass. Every non-grading attempt field is carried over unchanged from the source attempts file; only `grade` was recomputed, from the retained raw artifact, using the grader version above.", + "regraded": true, + "regraded_from_artifact_dir": "review-suite/evals/artifacts/s1-correctness-orchestrator/pre53-8e4fdbd-0.1-s1-pre53-2case", + "regraded_from_attempts": "review-suite/evals/artifacts/s1-correctness-orchestrator/pre53-8e4fdbd-0.1-s1-pre53-2case.attempts.jsonl", + "regraded_grader_version": "1.1", + "runs_per_case": 5, + "stratum": null, + "suite_commit": "8e4fdbdaad8f70751d45f8c2ca87e88288f8ba5b", + "target_skill": "review-code-change", + "target_skill_dependencies": [ + "review-solution-simplicity", + "review-correctness", + "review-code-simplicity" + ] + }, + "failures": { + "blocked_rate": 0.0, + "malformed_output_rate": 0.0, + "output_too_large_rate": 0.0, + "protocol_mismatch_rate": 0.0, + "runtime_failure_rate": 0.0, + "spawn_failure_rate": 0.0, + "timeout_rate": 0.0 + }, + "graded_attempts": 10, + "grader_version": "1.1", + "latency": { + "count": 10, + "max_seconds": 55.89703541621566, + "mean_seconds": 35.65590632092208, + "min_seconds": 19.295959250070155, + "p50_seconds": 33.305865792091936 + }, + "per_case": [ + { + "attempts": 5, + "case_id": "dependency-strictness-propagation", + "ever_referred_relevant_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "ever_referred_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "expected_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "false_alarm_attempts": 0, + "false_clean_attempts": 0, + "false_positive_attempts": 0, + "finding_stability": 1.0, + "graded_attempts": 5, + "intersection_root_cause_ids": [], + "mean_combined_recall": 1.0, + "mean_recall": 0.0, + "stability_denominator": 5, + "statuses": { + "review_result": 5 + }, + "union_root_cause_ids": [], + "verdict_stability": 1.0 + }, + { + "attempts": 5, + "case_id": "stale-claim-release-guard", + "ever_referred_relevant_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "ever_referred_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "expected_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "false_alarm_attempts": 0, + "false_clean_attempts": 0, + "false_positive_attempts": 0, + "finding_stability": 1.0, + "graded_attempts": 5, + "intersection_root_cause_ids": [], + "mean_combined_recall": 1.0, + "mean_recall": 0.0, + "stability_denominator": 5, + "statuses": { + "review_result": 5 + }, + "union_root_cause_ids": [], + "verdict_stability": 1.0 + } + ], + "protocol_version": "1.0", + "quality": { + "false_alarm_denominator": 0, + "false_alarm_rate": null, + "false_clean_denominator": 10, + "false_clean_rate": 0.0, + "false_positive_denominator": 10, + "false_positive_rate": 0.0, + "material_finding_recall": 0.0, + "recall_attempts": 10, + "referred_denominator": 10, + "referred_rate": 1.0, + "unique_finding_contribution": [] + }, + "report_version": "1.0", + "simulation": false, + "stability": { + "mean_finding_stability": 1.0, + "mean_verdict_stability": 1.0, + "per_case_finding_stability": { + "dependency-strictness-propagation": 1.0, + "stale-claim-release-guard": 1.0 + }, + "per_case_stability_denominator": { + "dependency-strictness-propagation": 5, + "stale-claim-release-guard": 5 + }, + "per_case_verdict_stability": { + "dependency-strictness-propagation": 1.0, + "stale-claim-release-guard": 1.0 + }, + "stability_denominator": 10 + }, + "usage": { + "available": true, + "reporting_attempts_cost_usd": 10, + "reporting_attempts_input_tokens": 10, + "reporting_attempts_output_tokens": 10, + "total_cost_usd": 0.8221815, + "total_input_tokens": 354210.0, + "total_output_tokens": 14472.0 + } +} diff --git a/review-suite/evals/v2/s1-correctness-orchestrator.report.json b/review-suite/evals/v2/s1-correctness-orchestrator.report.json new file mode 100644 index 0000000..3a5a15c --- /dev/null +++ b/review-suite/evals/v2/s1-correctness-orchestrator.report.json @@ -0,0 +1,419 @@ +{ + "adjudication_required": [ + { + "candidate_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "case_id": "dependency-strictness-propagation", + "finding_id": "corr.dependency-finalized-missing-strict-proof", + "reason": "partial", + "run_number": 1 + }, + { + "candidate_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "case_id": "dependency-strictness-propagation", + "finding_id": "corr.dependency-finalized-missing-strict-proof", + "reason": "partial", + "run_number": 2 + }, + { + "candidate_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "case_id": "dependency-strictness-propagation", + "finding_id": "dep-finalized-missing-strict-proof", + "reason": "partial", + "run_number": 3 + }, + { + "candidate_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "case_id": "dependency-strictness-propagation", + "finding_id": "dep-finalized-missing-strict-proof", + "reason": "partial", + "run_number": 4 + }, + { + "candidate_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "case_id": "dependency-strictness-propagation", + "finding_id": "dep-finalized-missing-strict-proof", + "reason": "partial", + "run_number": 5 + }, + { + "candidate_root_cause_ids": [ + "rc.probe-checks-status-not-absence" + ], + "case_id": "optional-tool-probe", + "finding_id": "probe-conflates-absent-and-failing", + "reason": "partial", + "run_number": 5 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "reaper.none-owner-bypass", + "reason": "partial", + "run_number": 1 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "reaper.missing-none-owner-test", + "reason": "partial", + "run_number": 1 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "corr.none-owner-bypass", + "reason": "partial", + "run_number": 2 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "corr.missing-none-owner-test", + "reason": "partial", + "run_number": 2 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "corr.null-owner-guard-bypass", + "reason": "partial", + "run_number": 3 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "reaper.none-owner-bypass", + "reason": "partial", + "run_number": 4 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "reaper.missing-none-owner-test", + "reason": "partial", + "run_number": 4 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "reaper-null-owner-bypass", + "reason": "partial", + "run_number": 5 + }, + { + "candidate_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "case_id": "stale-claim-release-guard", + "finding_id": "missing-test-null-owner", + "reason": "partial", + "run_number": 5 + } + ], + "attempts": 35, + "baseline_eligible": true, + "configuration": { + "cases": 7, + "corpus_version": "0.1-s1-populated", + "executor": null, + "grader_version": "1.1", + "regrade_note": "No executor process ran in this pass. Every non-grading attempt field is carried over unchanged from the source attempts file; only `grade` was recomputed, from the retained raw artifact, using the grader version above.", + "regraded": true, + "regraded_from_artifact_dir": "review-suite/evals/artifacts/s1-correctness-orchestrator/2c671fd-0.1-s1-populated", + "regraded_from_attempts": "review-suite/evals/artifacts/s1-correctness-orchestrator/2c671fd-0.1-s1-populated.attempts.jsonl", + "regraded_grader_version": "1.1", + "runs_per_case": 5, + "stratum": { + "grading_is_signal": true, + "ground_truth": "human-review", + "id": "s1-correctness-orchestrator", + "purpose": "Correctness and verification cases for the orchestrator target, whose payload therefore carries its three required lens skills. Every case carries the recorded source disposition as its first adjudication and an executable oracle - a runnable module independent of the reviewer's model - as its second. SCORED: both owner-gated inputs are satisfied (the $15 total cost ceiling and complete independent adjudication across all three strata), and every expectation stays `calibrated: false` permanently, per the owner-settled three-way grading method - a scored case is never calibrated on its own prose, and a grader miss surfaces as `referred_root_cause_ids` rather than a silent reviewer-miss.", + "scored": true + }, + "suite_commit": "2c671fd75f4bc11b3137f8d615764ac6c4e21851", + "target_skill": "review-code-change", + "target_skill_dependencies": [ + "review-solution-simplicity", + "review-correctness", + "review-code-simplicity" + ] + }, + "failures": { + "blocked_rate": 0.0, + "malformed_output_rate": 0.02857142857142857, + "output_too_large_rate": 0.0, + "protocol_mismatch_rate": 0.0, + "runtime_failure_rate": 0.0, + "spawn_failure_rate": 0.0, + "timeout_rate": 0.0 + }, + "graded_attempts": 34, + "grader_version": "1.1", + "latency": { + "count": 35, + "max_seconds": 116.61037687491626, + "mean_seconds": 49.03866441659629, + "min_seconds": 22.06988333305344, + "p50_seconds": 45.6546071250923 + }, + "per_case": [ + { + "attempts": 5, + "case_id": "dependency-hint-parser-coverage", + "ever_referred_relevant_root_cause_ids": [], + "ever_referred_root_cause_ids": [], + "expected_root_cause_ids": [], + "false_alarm_attempts": 0, + "false_clean_attempts": 0, + "false_positive_attempts": 0, + "finding_stability": 1.0, + "graded_attempts": 5, + "intersection_root_cause_ids": [], + "mean_combined_recall": null, + "mean_recall": null, + "stability_denominator": 5, + "statuses": { + "review_result": 5 + }, + "union_root_cause_ids": [], + "verdict_stability": 1.0 + }, + { + "attempts": 5, + "case_id": "dependency-strictness-propagation", + "ever_referred_relevant_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "ever_referred_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "expected_root_cause_ids": [ + "rc.sibling-call-site-keeps-permissive-mode" + ], + "false_alarm_attempts": 0, + "false_clean_attempts": 0, + "false_positive_attempts": 0, + "finding_stability": 1.0, + "graded_attempts": 5, + "intersection_root_cause_ids": [], + "mean_combined_recall": 1.0, + "mean_recall": 0.0, + "stability_denominator": 5, + "statuses": { + "review_result": 5 + }, + "union_root_cause_ids": [], + "verdict_stability": 1.0 + }, + { + "attempts": 5, + "case_id": "optional-tool-probe", + "ever_referred_relevant_root_cause_ids": [ + "rc.probe-checks-status-not-absence" + ], + "ever_referred_root_cause_ids": [ + "rc.probe-checks-status-not-absence" + ], + "expected_root_cause_ids": [ + "rc.probe-checks-status-not-absence" + ], + "false_alarm_attempts": 0, + "false_clean_attempts": 0, + "false_positive_attempts": 0, + "finding_stability": 0.8, + "graded_attempts": 5, + "intersection_root_cause_ids": [], + "mean_combined_recall": 1.0, + "mean_recall": 0.8, + "stability_denominator": 5, + "statuses": { + "review_result": 5 + }, + "union_root_cause_ids": [ + "rc.probe-checks-status-not-absence" + ], + "verdict_stability": 1.0 + }, + { + "attempts": 5, + "case_id": "post-bootstrap-module-load", + "ever_referred_relevant_root_cause_ids": [], + "ever_referred_root_cause_ids": [], + "expected_root_cause_ids": [], + "false_alarm_attempts": 0, + "false_clean_attempts": 0, + "false_positive_attempts": 0, + "finding_stability": 1.0, + "graded_attempts": 5, + "intersection_root_cause_ids": [], + "mean_combined_recall": null, + "mean_recall": null, + "stability_denominator": 5, + "statuses": { + "review_result": 5 + }, + "union_root_cause_ids": [], + "verdict_stability": 1.0 + }, + { + "attempts": 5, + "case_id": "process-isolation-assertion", + "ever_referred_relevant_root_cause_ids": [], + "ever_referred_root_cause_ids": [], + "expected_root_cause_ids": [], + "false_alarm_attempts": 0, + "false_clean_attempts": 0, + "false_positive_attempts": 0, + "finding_stability": 1.0, + "graded_attempts": 5, + "intersection_root_cause_ids": [], + "mean_combined_recall": null, + "mean_recall": null, + "stability_denominator": 5, + "statuses": { + "review_result": 5 + }, + "union_root_cause_ids": [], + "verdict_stability": 1.0 + }, + { + "attempts": 5, + "case_id": "session-continuation-summary", + "ever_referred_relevant_root_cause_ids": [], + "ever_referred_root_cause_ids": [], + "expected_root_cause_ids": [], + "false_alarm_attempts": 0, + "false_clean_attempts": 0, + "false_positive_attempts": 0, + "finding_stability": 1.0, + "graded_attempts": 4, + "intersection_root_cause_ids": [], + "mean_combined_recall": null, + "mean_recall": null, + "stability_denominator": 4, + "statuses": { + "malformed_output": 1, + "review_result": 4 + }, + "union_root_cause_ids": [], + "verdict_stability": 1.0 + }, + { + "attempts": 5, + "case_id": "stale-claim-release-guard", + "ever_referred_relevant_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "ever_referred_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "expected_root_cause_ids": [ + "rc.guard-skipped-when-snapshot-owner-absent" + ], + "false_alarm_attempts": 0, + "false_clean_attempts": 0, + "false_positive_attempts": 0, + "finding_stability": 1.0, + "graded_attempts": 5, + "intersection_root_cause_ids": [], + "mean_combined_recall": 1.0, + "mean_recall": 0.0, + "stability_denominator": 5, + "statuses": { + "review_result": 5 + }, + "union_root_cause_ids": [], + "verdict_stability": 1.0 + } + ], + "protocol_version": "1.0", + "quality": { + "false_alarm_denominator": 19, + "false_alarm_rate": 0.0, + "false_clean_denominator": 15, + "false_clean_rate": 0.0, + "false_positive_denominator": 34, + "false_positive_rate": 0.0, + "material_finding_recall": 0.26666666666666666, + "recall_attempts": 15, + "referred_denominator": 34, + "referred_rate": 0.3235294117647059, + "unique_finding_contribution": [ + { + "case_id": "optional-tool-probe", + "only_some_attempts": [ + "rc.probe-checks-status-not-absence" + ] + } + ] + }, + "report_version": "1.0", + "simulation": false, + "stability": { + "mean_finding_stability": 0.9714285714285714, + "mean_verdict_stability": 1.0, + "per_case_finding_stability": { + "dependency-hint-parser-coverage": 1.0, + "dependency-strictness-propagation": 1.0, + "optional-tool-probe": 0.8, + "post-bootstrap-module-load": 1.0, + "process-isolation-assertion": 1.0, + "session-continuation-summary": 1.0, + "stale-claim-release-guard": 1.0 + }, + "per_case_stability_denominator": { + "dependency-hint-parser-coverage": 5, + "dependency-strictness-propagation": 5, + "optional-tool-probe": 5, + "post-bootstrap-module-load": 5, + "process-isolation-assertion": 5, + "session-continuation-summary": 4, + "stale-claim-release-guard": 5 + }, + "per_case_verdict_stability": { + "dependency-hint-parser-coverage": 1.0, + "dependency-strictness-propagation": 1.0, + "optional-tool-probe": 1.0, + "post-bootstrap-module-load": 1.0, + "process-isolation-assertion": 1.0, + "session-continuation-summary": 1.0, + "stale-claim-release-guard": 1.0 + }, + "stability_denominator": 34 + }, + "usage": { + "available": true, + "reporting_attempts_cost_usd": 35, + "reporting_attempts_input_tokens": 35, + "reporting_attempts_output_tokens": 35, + "total_cost_usd": 3.559636249999999, + "total_input_tokens": 1275175.0, + "total_output_tokens": 75593.0 + } +} diff --git a/review-suite/evals/v2/s1-rescore-pre-post-comparison.md b/review-suite/evals/v2/s1-rescore-pre-post-comparison.md new file mode 100644 index 0000000..81390e6 --- /dev/null +++ b/review-suite/evals/v2/s1-rescore-pre-post-comparison.md @@ -0,0 +1,171 @@ +# `s1-correctness-orchestrator`: pre-#53 vs. post-#53, under the fixed grader + +This is the plain, apples-to-apples comparison the repository owner asked for: +whether `dependency-strictness-propagation` and `stale-claim-release-guard` - +the two cases the frozen v1 baseline recorded as confident (zero-ambiguity) +misses, and the two cases #53 was built to fix + +- were a genuine reviewer capability gap before #53, and whether #53 closed it. + It measures facts; it does not decide #54's fate, #52/#53's disposition, or + anything else. See `GRADER-1.1-COMPARABILITY.md` in this directory for why a + grader fix was necessary before this comparison meant anything, and + `s1-rescore-frozen-configuration.json` for the originally frozen re-score + configuration this comparison extends. + +## The four numbers + +Both cases have exactly one material root cause each, so `mean_recall` is the +fraction of 5 attempts that named it in an accepted formulation, and +`mean_combined_recall` is the fraction of 5 attempts that either did that or had +the surface concretely and relevantly named in a referred finding (the +preregistered v2 scoring gate's alternative path, relevance-guarded per #53's +issue body). + +| Case | Era | Runs | `mean_recall` | `mean_combined_recall` | Gate (`>=0.6` recall OR `>=0.8` combined) | +| ----------------------------------- | --------------------------------------- | ---------- | ------------- | ---------------------- | ----------------------------------------- | +| `dependency-strictness-propagation` | pre-#53 (commit `8e4fdbd`, #52's merge) | 5/5 graded | 0.0 | **1.0** | **PASS** (combined path) | +| `dependency-strictness-propagation` | post-#53 (commit `2c671fd`) | 5/5 graded | 0.0 | **1.0** | **PASS** (combined path) | +| `stale-claim-release-guard` | pre-#53 (commit `8e4fdbd`, #52's merge) | 5/5 graded | 0.0 | **1.0** | **PASS** (combined path) | +| `stale-claim-release-guard` | post-#53 (commit `2c671fd`) | 5/5 graded | 0.0 | **1.0** | **PASS** (combined path) | + +Every one of these 20 attempts (10 pre-#53 + 10 post-#53) individually scored +`combined_recall: 1.0` - this is not an average smoothing over a mixed result; +the fixed grader recognizes the correct root cause in all 10 attempts of both +eras. `mean_recall` stays `0.0` in both eras because both cases are +deliberately, permanently `calibrated: false` (their `equivalent_formulations` +were authored without ever being checked against real reviewer prose, by corpus +design - see `CALIBRATION.md`), so a real reviewer's paraphrase essentially +never contains one of the five pre-written phrases verbatim. That is a property +of the corpus's calibration state, not of reviewer behavior, and applies +identically to both eras. + +## Non-regression floor (post-#53, 35-attempt full-stratum run) + +| Requirement | v1 (frozen) | v2, this run (fixed grader) | Holds? | +| ------------------------------------------------------------------------------------------------------------------ | ----------------- | --------------------------- | ------------------- | +| `dependency-strictness-propagation` / `stale-claim-release-guard` `verdict_stability`, `finding_stability` >= 1.0 | 1.0 / 1.0 | 1.0 / 1.0 (both cases) | Yes | +| `dependency-hint-parser-coverage`, `post-bootstrap-module-load`, `session-continuation-summary`: 0 false positives | 0 each | 0 each | Yes | +| `process-isolation-assertion` false-alarm rate \<= 2/5 | 2/5 | 0/5 | Yes (improved) | +| `optional-tool-probe` (open question, not gated) | `mean_recall` 0.2 | `mean_recall` 0.8 | Not gated; recorded | + +One evaluation failure occurred in the 35-attempt run +(`session-continuation-summary` run 4, `malformed_output`: the reviewer's own +`consumer_impact_evidence[1].disposition` failed validator cross-checks) - +excluded from grading per protocol, not counted toward any floor, consistent +with the "never retried, never silently padded" rule both v1 and this run share. + +## What this means + +**The pre-#53 reviewer already found both defects, every time, before #53 +existed.** Reading the raw pre-#53 attempts directly (not just the grade) +confirms this: run 1 of `dependency-strictness-propagation` names +`dependency_finalized`/`integration_proven` and the exact consequence +(`reconcile_closed` reopens the record, the scheduler path does not); run 1 of +`stale-claim-release-guard` names the `owner=None` scan-to-apply race and the +exact guard-skip mechanism. All 5 attempts of each case, both pre- and post-#53, +are this concrete and this consistent - checked individually, not sampled. + +**The frozen v1 baseline's "confident miss" on these two cases was a grader +artifact, not a demonstrated reviewer capability gap.** v1 scored these two +cases with `GRADER_VERSION` `1.0`, which read only structured `location` fields +for a surface match; a reviewer that names the exact symbol only in prose (which +both eras of reviewer did, on every attempt) was scored `unexpected` (false +positive) and the root cause counted as a genuine miss, identically to what this +measurement's own post-#53 run showed before the grader was fixed. v1's raw +attempts are not retained anywhere on this machine or in this repository's +history, so this cannot be proven attempt-by-attempt for v1 the way it was just +proven for pre-#53 and post-#53 here - but the mechanism is the same code, the +same two deliberately-uncalibrated expectations, and the same "confident, zero- +referral" shape v1 reported, on a reviewer now directly shown to answer these +exact two cases correctly and consistently in the era immediately preceding v1's +own measurement window. + +**#53's two added passes (consumer/impact-traversal, verification-sufficiency), +while real and reasonable engineering, are not shown by this evidence to have +closed a demonstrated recall or stability gap on these two specific cases.** The +pre-#53 reviewer already populated `consumer_impact_evidence` for the traversal +case in the sampled raw attempt, and already stated the correct verification gap +in prose for the guard case, without being explicitly instructed to run either +pass by name. + +## Methodology notes and honest caveats + +- **The grader fix happened mid-measurement, after some scored v2 output was + already visible.** This is a real deviation from strict freeze-before-scoring + discipline in letter, and is disclosed rather than smoothed over. It differs + from the tuning `gate-manifest.json` forbids ("frozen the moment any scored + output is examined... requires a new manifest version") in kind, not just in + degree: the defect was identified by reading raw prose directly and finding it + plainly, concretely correct despite being graded a miss - not by adjusting a + number to get a preferred result - and the fix was constrained by an existing, + unrelated calibration boundary (`probe.partial-claim-wrong-surface`) that had + to keep passing, not by this comparison's own two target cases. + `GRADER-1.1-COMPARABILITY.md` documents the change and the new-stratum rule + this creates. +- **v1's own raw attempts cannot be re-graded** - they are not retained anywhere + reachable from this repository or this machine, confirmed by direct search. + The claim above about v1's baseline is therefore a well-evidenced inference + from an unchanged mechanism and an unchanged expectation file, not a re-graded + fact the way pre-#53 and post-#53 are. +- **`mean_recall` staying `0.0` in both eras is a corpus calibration property, + not a reviewer weakness** - see `CALIBRATION.md` and each case's + `expectation.json` (`"calibrated": false`, permanently, by owner-settled + design). It is reported here for completeness, not as a quality signal to act + on. +- **Sample size is small** (5 runs/case/era, 20 attempts total for the two + target cases): a real capability difference smaller than this sample can + resolve is not ruled out. What is ruled out, at this sample size, is the + specific "zero-for-five, zero-referral, confident" pattern v1 reported - + neither era reproduces that pattern once the grader can see what the reviewer + actually wrote. + +## Spend + +| Run | Attempts | Cost (USD) | +| ------------------------------------------ | -------- | ------------ | +| Post-#53, full 7-case stratum, 5 runs/case | 35 | 3.559636 | +| Pre-#53, 2-case subset corpus, 5 runs/case | 10 | 0.822182 | +| **Total** | **45** | **4.381818** | + +Against the $12.00 ceiling (per #53's issue body and this task's own hard +ceiling): **$4.38 spent, $7.62 headroom remaining, ceiling never approached.** +Re-grading both the 35 post-#53 and 10 pre-#53 attempts with the fixed grader +(`review-suite/scripts/evals/regrade.py`) cost nothing further - no executor +process ran for either re-grade. + +## Recommendation (not a decision) + +This is a recommendation for the repository owner to weigh, not a decision this +measurement task is authorized to make: + +- **On #54:** the evidence #54's provisional status cites (via #59's decision + record) - two confident, zero-ambiguity misses - does not survive this + measurement. Both cases resolve correctly under both the pre- and post-#53 + reviewer once the grader can see what was actually written. This measurement + finds no remaining recall or stability failure on these two cases that would + justify unblocking #54's independent-discovery architecture; if anything, it + removes the specific evidence #54's own text says is required before + proceeding. #54's own "narrow or close this ticket" fallback reads as the + better-supported path on this evidence, but that determination belongs to the + repository owner. +- **On #52/#53's original justification (separate from #54):** #59's decision + record grounded #52's `consumer_impact_evidence` schema and #53's two required + passes in these same two "confident miss" cases. That grounding is now shown + to rest on a grader artifact rather than a real capability gap, for these two + specific cases. This does not mean #52/#53's actual changes are wrong or + harmful - `consumer_impact_evidence` and `verification_sufficiency_evidence` + are reasonable, real, structurally sound additions to the review contract + regardless of whether they closed a measured gap - but their evidentiary + justification, as recorded, should likely be revisited or reworded by whoever + owns `DECISION-RECORD.md`. This measurement does not edit that file; it only + flags the discrepancy for the owner who does. +- **On the grader itself:** the location-only surface-matching blind spot this + fix closes is not proven to be limited to these two cases. Any other stratum + or case whose expectation's `surface` field is a symbol name (rather than, + say, a file path a reviewer would naturally type into `location`) may carry + the same undetected blind spot. Re-auditing `s2-solution-simplicity-lens` and + `s3-code-simplicity-lens`'s expectations for this pattern, and considering + whether v1's baseline itself warrants a disclosed re-score under `1.1` (a + large, separate undertaking, explicitly out of scope here since it would mean + touching or superseding `baseline/v1/`), are both reasonable next steps for + the owner to weigh - not decided or begun here. From cb14d92351d3bd30f0547e41f89234e52a92ca38 Mon Sep 17 00:00:00 2001 From: Scott Haug Date: Tue, 28 Jul 2026 23:10:03 -0700 Subject: [PATCH 5/5] refactor(review-suite): reuse _signal_match instead of duplicating it ## Summary - `surface_named_in_prose` now delegates to the existing `_signal_match` (called with a single-item `[expected.get("surface", "")]` formulations list) instead of reimplementing the same normalize-then-substring check a second time. ## Why - Found in review-code-change's code-simplicity pass on this branch: `surface_named_in_prose`'s body was behaviorally identical to `_signal_match([surface], text)` in every case checked (the real dependency_finalized/_release matches, the render_rollback_guidance wrong-surface calibration case, and both empty/missing surface fields), including the same falsy-formulation guard that makes an empty `surface` never match. Two separately-named implementations of the same rule is one more concept than a maintainer needs to hold, and a future change to the matching rule would otherwise have to be made in both places. - Purely internal: no observable behavior changes. `just test` (252 tests) passes unchanged before and after. Co-Authored-By: Claude Sonnet 5 --- review-suite/scripts/evals/grader.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/review-suite/scripts/evals/grader.py b/review-suite/scripts/evals/grader.py index 45d4784..0d61b1f 100644 --- a/review-suite/scripts/evals/grader.py +++ b/review-suite/scripts/evals/grader.py @@ -137,9 +137,14 @@ def surface_named_in_prose(expected: dict[str, Any], finding: dict[str, Any]) -> phrase to appear keeps that boundary intact (a lone "guidance" is not "render rollback guidance") while still catching a real reviewer that plainly names the exact symbol. + + Implemented as `_signal_match` on a single-item formulations list rather + than a second, separately-written normalize/substring check: it is the + exact same rule (including the same falsy-formulation guard, so an + empty or missing `surface` field correctly never matches), and reusing it + keeps only one place that rule can drift. """ - surface = normalize(expected.get("surface", "")) - return bool(surface) and surface in finding_text(finding) + return _signal_match([expected.get("surface", "")], finding_text(finding)) def match_signals(