From 47e682900d82e50d8ee83d6fe72b06defe12a961 Mon Sep 17 00:00:00 2001 From: Serina Mcfall Date: Fri, 21 Aug 2026 11:39:55 +1200 Subject: [PATCH 1/5] feat(launchpad): escalate-only enforcement and total-refutation status (#118 STEP 6) Extend run_adjudication.py's adjudicate() with the three STEP 6 behaviours: a judge's return dict may now carry a severity re-rating, guarded so an out-of-ladder value it produces is refused (UNPROVEN at reported_severity, never published) rather than copied through; a genuine downgrade is recorded into adjudication.downgrades at the moment it is applied, never by a later sweep; and total_refutation now surfaces in the stages manifest's own status ("total_refutation", not "complete") rather than only in adjudication's own boolean. Also adds a belt-and-braces finding_id set-equality check inside adjudicate() itself, raising before the document is ever printed. Verified against STEP 6's own done-when: a REFUTED-everything judge leaves findings/findings_count unchanged and flips the stage status; the same judge against zero findings stays "complete"; a judge returning "Info" over a legally in-ladder reported_severity is refused with a reason and still passes verdicts.validate; a bare review.SEVERITY_ORDER[...] subscript succeeds on every finding in every output; and a Blocker-to-Low downgrade is named in adjudication.downgrades with from/to/reason. Signed-off-by: Serina Mcfall --- launchpad/review-agent/run_adjudication.py | 296 +++++++++++++++--- .../review-agent/test_run_adjudication.py | 278 +++++++++++++++- 2 files changed, 527 insertions(+), 47 deletions(-) diff --git a/launchpad/review-agent/run_adjudication.py b/launchpad/review-agent/run_adjudication.py index dcff95b1844..e3a58fc5af3 100644 --- a/launchpad/review-agent/run_adjudication.py +++ b/launchpad/review-agent/run_adjudication.py @@ -1,5 +1,7 @@ -"""The adjudication stage's CLI. Implements launchpad-26/buzz#118 STEP 3 and -STEP 4 (the nonce check and the `stages` manifest -- see that section below). +"""The adjudication stage's CLI. Implements launchpad-26/buzz#118 STEP 3, +STEP 4 (the nonce check and the `stages` manifest) and STEP 6 (severity +re-rating, the out-of-ladder guard, downgrade recording and the +total-refutation status) -- see those sections below. Reads one #117 **merged document** on stdin, adjudicates every finding with an **injected judge callable** -- defaulting to a stub that returns ``UNPROVEN`` @@ -53,11 +55,60 @@ model" out of scope here, per #117's own framing and #118's issue: this module never names one, and neither flag lets a caller supply one. -Severity re-rating, the escalate-only guard, downgrade recording and dedupe -are STEP 6/7's job, layered on top of this module later. This step leaves -every finding's ``severity`` exactly equal to its ``reported_severity`` -- -the honest behaviour for a judge (the stub) that never rates anything -- and -``duplicate_of`` always null. +Dedupe is STEP 7's job, layered on top of this module later -- +``duplicate_of`` is always null here. Severity re-rating and the +escalate-only guard are STEP 6, below. + +**STEP 6 -- severity re-rating and the out-of-ladder guard.** A judge's +return dict MAY now also carry a ``severity`` key and, when re-rating, a +``severity_reason``. ``_run_judge_safely`` forwards both from a *usable* +result only -- a judge whose output already failed closed to ``UNPROVEN`` +(a crash, a missing/illegal ``verdict``, empty ``verdict_evidence``) never +gets to re-rate severity too; failing closed means failing closed on both. + +``_apply_severity_rerating`` (below) is the guard, run once per finding: + +* No ``severity`` key, or one equal to the finding's own + ``reported_severity``: unchanged from STEP 3/4 -- ``severity == + reported_severity``, ``severity_reason`` stays ``None``. +* A **legal** (in ``review.SEVERITY_ORDER``) severity that differs: it + becomes the finding's ``severity``, with a reason -- the judge's own if it + gave one, else a generated default, since ``verdicts.validate`` requires a + reason whenever ``severity != reported_severity`` regardless of direction. + If it is a genuine **fall** (worse ``SEVERITY_ORDER`` index than + ``reported_severity``), it is appended to ``adjudication.downgrades`` -- + ``{finding_id, from, to, reason}`` -- right there, at the moment the + re-rating is applied, never by a later sweep over the finished document (a + sweep is a second place the two could disagree). An **upgrade** (more + severe) is not a downgrade and is never added to that list. +* An **illegal** (out-of-ladder) severity: refused, not published. This is + the one place this stage can still *create* an out-of-ladder value -- the + input arriving illegal is already caught upstream by STEP 3's + ``findings.validate`` call, before any judge runs -- so this is defence in + depth over this stage's own re-rating, not a repeat of that guard. The + finding's ``verdict`` becomes ``UNPROVEN`` regardless of what the judge + said for ``verdict``, ``severity`` falls back to ``reported_severity`` + (guaranteed legal at this point by STEP 3's input validation) or, purely + as a second layer should that guarantee ever be bypassed, to ``"Blocker"`` + when even ``reported_severity`` is not in the ladder, and + ``severity_reason`` names the refusal. Never added to ``downgrades``: + nothing legally fell -- the value was refused, not accepted-then-compared. + +**Total refutation now reaches the ``stages`` status, not only +``adjudication.total_refutation``.** When every finding is ``REFUTED`` and +at least one finding was received, the ``adjudication`` stage entry's own +``status`` is ``"total_refutation"`` -- checked ahead of the "every finding +has a verdict" condition below, so it wins whenever it applies. A document +with zero findings is never total refutation (the existing ``findings_in > +0`` condition already excludes it), so it keeps reporting ``"complete"``. + +**Nothing is removed, reasserted inside this module.** ``adjudicate()`` +compares the ``finding_id`` set of ``output_document`` against +``input_document`` immediately before returning and raises +``FindingSetIntegrityError`` if they differ -- belt-and-braces, since this +function does not drop or invent one by construction, but a stage that can +print a lossy document and rely on a downstream ``verdicts.validate`` call +to catch it has already lost the document once. **STEP 4 -- the nonce check and the `stages` manifest.** Two more things ``adjudicate()`` does, on top of STEP 3's pass-through/anchor/validation-order @@ -124,13 +175,18 @@ from typing import Callable import findings +import review import verdicts #: The judge protocol: ``judge(finding, input_document) -> dict`` with at -#: least ``{"verdict": ..., "verdict_evidence": ...}``. Anything else -- -#: a raised exception, a missing/illegal ``verdict``, empty -#: ``verdict_evidence`` -- is treated as unusable output and fails closed to -#: UNPROVEN, per ADJUDICATION.md's own default. +#: least ``{"verdict": ..., "verdict_evidence": ...}``, and MAY also carry +#: ``severity`` (STEP 6's re-rating) plus, when re-rating, ``severity_reason`` +#: -- omitted, or equal to the finding's own ``reported_severity``, means no +#: re-rating at all. Anything else -- a raised exception, a missing/illegal +#: ``verdict``, empty ``verdict_evidence`` -- is treated as unusable output +#: and fails closed to UNPROVEN, per ADJUDICATION.md's own default, and no +#: severity re-rating is attempted from output this module could not use in +#: the first place. Judge = Callable[[dict, dict], dict] @@ -171,6 +227,19 @@ class AlreadyAdjudicatedError(ValueError): """ +class FindingSetIntegrityError(RuntimeError): + """Raised by ``adjudicate()`` if the ``finding_id`` set of the document it + is about to return would differ from the set it was given -- STEP 6's + "nothing is removed" reassertion. By construction this function never + drops or invents a finding_id, so this is belt-and-braces: a stage that + can print a lossy document and rely on a downstream ``verdicts.validate`` + call to catch it has already lost the document once. Deliberately a bare + ``RuntimeError`` subclass rather than a caller-input error like the three + above -- this names a bug in this module, not a defect in the document it + was handed. + """ + + def _report_marker_nonce(report: dict) -> str | None: """Extract the nonce embedded in one report's ``completion_marker``, or ``None`` when the marker is missing, non-string, or does not parse as @@ -343,6 +412,14 @@ def _run_judge_safely(judge: Judge, finding: dict, input_document: dict) -> dict empty ``verdict_evidence``. ADJUDICATION.md's own words: "An adjudicator that cannot reach the location, cannot parse the finding, times out, or returns unusable output yields UNPROVEN with a reason." + + Returns a dict carrying at least ``verdict``/``verdict_evidence``, and -- + only when the judge's own output was usable -- ``severity``/ + ``severity_reason`` when the judge's return dict carried them (STEP 6's + re-rating; see ``_apply_severity_rerating``). A judge whose output failed + closed never gets to re-rate severity too: the two failure keys below + never include a ``severity`` key, on purpose, so failing closed means + failing closed on both. """ try: result = judge(finding, input_document) @@ -368,7 +445,109 @@ def _run_judge_safely(judge: Judge, finding: dict, input_document: dict) -> dict "ADJUDICATION.md's default." ), } - return {"verdict": verdict, "verdict_evidence": evidence} + + safe_result = {"verdict": verdict, "verdict_evidence": evidence} + if "severity" in result: + safe_result["severity"] = result["severity"] + if "severity_reason" in result: + safe_result["severity_reason"] = result["severity_reason"] + return safe_result + + +def _apply_severity_rerating( + finding_id: object, + reported_severity: str, + verdict: str, + proposed_severity: object, + proposed_reason: object, + downgrades: list[dict], +) -> tuple[str, str, str | None]: + """STEP 6's severity re-rating guard, applied to one finding. Returns the + ``(verdict, severity, severity_reason)`` that should actually be + emitted -- ``verdict`` is returned rather than assumed unchanged because + the illegal-severity branch overrides it. + + ``proposed_severity``/``proposed_reason`` are exactly what + ``_run_judge_safely`` forwarded from the judge's own return dict -- + ``None`` when the judge did not re-rate, or when its output already + failed closed (in which case no re-rating is attempted at all). + + Mutates ``downgrades`` in place by appending an entry at the moment a + genuine fall is applied -- never by a later sweep over the finished + document, per ADJUDICATION.md's "record it here, not by a later sweep" + reasoning: a sweep is a second place the two could disagree. + """ + if proposed_severity is None or proposed_severity == reported_severity: + # No re-rating: unchanged from STEP 3/4's behaviour. + return verdict, reported_severity, None + + if proposed_severity not in review.SEVERITY_ORDER: + # ILLEGAL re-rating: refused, not published. This is the one place + # this stage can still *create* an out-of-ladder value -- an input + # finding arriving illegal is already caught upstream by STEP 3's + # findings.validate call, before any judge runs at all -- so this is + # defence in depth over this stage's OWN re-rating, not a repeat of + # that upstream guard. + fallback_severity = ( + reported_severity if reported_severity in review.SEVERITY_ORDER else "Blocker" + ) + reason = ( + f"judge returned an out-of-ladder severity {proposed_severity!r} for finding " + f"{finding_id!r}; refused, falling back to the reported severity " + f"{fallback_severity!r}" + ) + # Not appended to `downgrades`: nothing legally fell -- the value was + # refused, not accepted and then compared. + return "UNPROVEN", fallback_severity, reason + + # LEGAL re-rating that differs from reported_severity. verdicts.validate + # requires severity_reason whenever severity != reported_severity + # regardless of direction, so a reason is generated even for an upgrade, + # which is not itself a downgrade. + reason = proposed_reason if proposed_reason else ( + f"judge re-rated severity from {reported_severity!r} to {proposed_severity!r} " + "with no reason given" + ) + if review.SEVERITY_ORDER[proposed_severity] > review.SEVERITY_ORDER[reported_severity]: + downgrades.append( + { + "finding_id": finding_id, + "from": reported_severity, + "to": proposed_severity, + "reason": reason, + } + ) + return verdict, proposed_severity, reason + + +def _collect_finding_ids(document: dict) -> set[str]: + """The set of every ``finding_id`` present across ``document``'s + ``reports[].findings``. + + A small, local walk -- not a call into ``verdicts._finding_ids`` -- on + purpose: this function backs STEP 6's "nothing is removed" reassertion + inside ``adjudicate()`` itself, and a bug shared between the producer and + its own belt-and-braces check would prove nothing. Every container is + type-checked before being treated as its expected shape, same discipline + ``verdicts.py`` and ``findings.py`` both already use for a document that + might be malformed. + """ + ids: set[str] = set() + reports = document.get("reports") + if not isinstance(reports, list): + return ids + for report in reports: + if not isinstance(report, dict): + continue + findings_list = report.get("findings") + if not isinstance(findings_list, list): + continue + for finding in findings_list: + if isinstance(finding, dict): + fid = finding.get("finding_id") + if isinstance(fid, str): + ids.add(fid) + return ids def adjudicate(input_document: dict, judge: Judge) -> dict: @@ -392,10 +571,17 @@ def adjudicate(input_document: dict, judge: Judge) -> dict: Pass-through fields (``pr``, ``merge_base_sha``, ``head_sha``, ``containment``) are never touched: the output starts as a ``copy.deepcopy`` of the input, and only a finding dict's own six new keys - are ever written. Severity re-rating, the escalate-only guard, downgrade - recording and dedupe are later steps' job -- every finding's ``severity`` - here is left exactly equal to its ``reported_severity``, and - ``duplicate_of`` is always null. + are ever written. Dedupe is a later step's job -- ``duplicate_of`` is + always null here. Severity re-rating and the out-of-ladder guard are + STEP 6 (see the module docstring's STEP 6 section and + ``_apply_severity_rerating``); a judge that never re-rates leaves every + finding's ``severity`` exactly equal to its ``reported_severity``, same + as STEP 3/4. + + Before returning, asserts (raising ``FindingSetIntegrityError`` on + failure) that ``output_document``'s ``finding_id`` set equals + ``input_document``'s -- STEP 6's "nothing is removed" reassertion, run + here rather than left to a downstream ``verdicts.validate`` call. """ _check_not_already_adjudicated(input_document) @@ -431,21 +617,28 @@ def adjudicate(input_document: dict, judge: Judge) -> dict: verdict_counts = {"CONFIRMED": 0, "REFUTED": 0, "UNPROVEN": 0} findings_in = 0 + downgrades: list[dict] = [] for report in output_document.get("reports", []): for finding in report.get("findings", []): findings_in += 1 result = _run_judge_safely(judge, finding, input_document) reported_severity = finding["severity"] - finding["verdict"] = result["verdict"] + verdict, severity, severity_reason = _apply_severity_rerating( + finding_id=finding.get("finding_id"), + reported_severity=reported_severity, + verdict=result["verdict"], + proposed_severity=result.get("severity"), + proposed_reason=result.get("severity_reason"), + downgrades=downgrades, + ) + finding["verdict"] = verdict finding["verdict_evidence"] = result["verdict_evidence"] finding["reported_severity"] = reported_severity - # No re-rating in this stage: `severity` (#117's own field, already - # present on `finding`) is left exactly as reported. STEP 6 adds - # the guard that lets a judge's re-rating land here safely. - finding["severity_reason"] = None + finding["severity"] = severity + finding["severity_reason"] = severity_reason finding["duplicate_of"] = None - verdict_counts[result["verdict"]] += 1 + verdict_counts[verdict] += 1 # Nothing is dropped or invented at this stage, so the two counts are the # same number by construction -- kept as two separate values (rather than @@ -460,7 +653,7 @@ def adjudicate(input_document: dict, judge: Judge) -> dict: findings_in=findings_in, findings_out=findings_out, duplicate_groups=[], - downgrades=[], + downgrades=downgrades, total_refutation=total_refutation, notes=[], completion_marker=f"BUZZ-ADJUDICATION-COMPLETE:{nonce}", @@ -473,34 +666,43 @@ def adjudicate(input_document: dict, judge: Judge) -> dict: input_stages_raw = input_document.get("stages") input_stages = copy.deepcopy(input_stages_raw) if isinstance(input_stages_raw, list) else [] - # `status` is "complete" only when every finding received a verdict AND - # the nonce was established. The nonce condition is always True here -- - # `_verify_nonce` above would have raised otherwise -- named explicitly - # anyway so the AND reads as the real, multi-condition guarantee - # ADJUDICATION.md states rather than a constant. `every_finding_has_ - # verdict` is read back off `output_document` itself (not tracked as a - # separate counter through the loop above) so it is a check ON the - # produced data rather than a second bookkeeping path that could drift - # from it. STEP 6's total-refutation flag is a third condition this stage - # does not build yet -- its absence must not make `stage_complete` wrongly - # unconditional, which is why it is named as its own boolean rather than - # inlined into one `and` chain that silently drops it. + # `status` is "complete" only when every finding received a verdict, the + # nonce was established, AND `total_refutation` is false. The nonce + # condition is always True here -- `_verify_nonce` above would have + # raised otherwise -- named explicitly anyway so the AND reads as the + # real, multi-condition guarantee ADJUDICATION.md states rather than a + # constant. `every_finding_has_verdict` is read back off + # `output_document` itself (not tracked as a separate counter through the + # loop above) so it is a check ON the produced data rather than a second + # bookkeeping path that could drift from it. nonce_established = True every_finding_has_verdict = all( finding.get("verdict") in verdicts.VERDICTS for report in output_document.get("reports", []) for finding in report.get("findings", []) ) - stage_complete = every_finding_has_verdict and nonce_established - - if stage_complete: + stage_complete = every_finding_has_verdict and nonce_established and not total_refutation + + # `total_refutation` is checked FIRST and wins whenever it applies -- + # ADJUDICATION.md § The `stages` entry names "total_refutation" as one of + # the specific reasons `status` carries when it is not "complete", and + # the zero-findings case never reaches here with `total_refutation` true + # (the `findings_in > 0` condition above already excludes it), so a + # document with no findings still falls through to "complete" below. + if total_refutation: + stage_status = "total_refutation" + stage_reason = ( + "every finding was REFUTED; see adjudication.total_refutation and " + "adjudication.verdict_counts" + ) + elif stage_complete: stage_status, stage_reason = "complete", None else: # Unreachable today: `_run_judge_safely` always returns a legal # verdict, so `every_finding_has_verdict` is always True by the time # this runs, and a False `nonce_established` would already have - # raised above. Kept as a real branch, not asserted away, so STEP 6 - # can add its own condition here without restructuring this function. + # raised above. Kept as a real branch, not asserted away, same + # discipline as `nonce_established` above. stage_status = "incomplete" stage_reason = "not every finding received a verdict" @@ -509,6 +711,20 @@ def adjudicate(input_document: dict, judge: Judge) -> dict: {"name": "adjudication", "status": stage_status, "reason": stage_reason}, ] + # STEP 6's "nothing is removed" reassertion: this function does not drop + # or invent a finding_id by construction, but a stage that can print a + # lossy document and rely on a downstream `verdicts.validate` call to + # catch it has already lost the document once. Checked here, inside the + # runner itself, immediately before the document it guards is returned. + input_ids = _collect_finding_ids(input_document) + output_ids = _collect_finding_ids(output_document) + if input_ids != output_ids: + raise FindingSetIntegrityError( + "adjudicate() would drop or invent a finding_id -- input and output finding_id " + f"sets differ: dropped={sorted(input_ids - output_ids)}, " + f"invented={sorted(output_ids - input_ids)}" + ) + return output_document diff --git a/launchpad/review-agent/test_run_adjudication.py b/launchpad/review-agent/test_run_adjudication.py index 35006892ae9..6f2b08f1a6a 100644 --- a/launchpad/review-agent/test_run_adjudication.py +++ b/launchpad/review-agent/test_run_adjudication.py @@ -24,11 +24,22 @@ reports for any fixture that also happens to fail #117's own contract, which is every reachable fixture today. +Also exercises STEP 6's own done-when: a judge that REFUTEs every finding +(membership/length/`findings_count` unchanged, `total_refutation` true, the +`adjudication` stage status not "complete") and the same judge against zero +findings (`total_refutation` false, status "complete"); a judge returning +the out-of-ladder severity "Info" over a legally in-ladder +`reported_severity` (UNPROVEN at the reported severity, with a reason, and +the document still passes `verdicts.validate`) -- the sibling case, a +finding ARRIVING with an illegal `reported_severity`, stays +`IllegalInputSeverityTests`' job above, not repeated here; a bare +`review.SEVERITY_ORDER[f["severity"]]` subscript over every finding in +every output; and a judge that downgrades a Blocker to Low +(`adjudication.downgrades` names it with from/to/reason). See +`SeverityRerateTests` and `TotalRefutationStatusTests` below. + Deliberately NOT exercised here (later steps' territory, per the plan): -the escalate-only guard and downgrade recording for a judge that actually -re-rates severity (STEP 6), and dedupe (STEP 7). Every fixture below either -omits a re-rating entirely or only ever asserts that this stage's own -severity pass-through (``severity == reported_severity``, always) holds. +dedupe (STEP 7). This file is scoped to `run_adjudication.py` alone and is deliberately not wired into `run_controls.py`'s CONTROLS list -- that is STEP 10's control @@ -52,6 +63,7 @@ import contain import findings +import review import run_adjudication import verdicts @@ -358,9 +370,11 @@ def _empty_evidence_judge(finding, document): class NoRerateInThisStepTests(unittest.TestCase): - """This step performs no re-rating at all (STEP 6's job): every finding's - `severity` equals its `reported_severity`, even when the injected judge - returns a verdict -- the judge protocol here carries no severity field. + """The stub judge never re-rates: every finding's `severity` equals its + `reported_severity`, even though `stub_judge` returns a verdict -- + it simply never includes a `severity` key in its return dict, which + STEP 6's guard (see `SeverityRerateTests` below) treats identically to a + `severity` equal to `reported_severity`: no re-rating at all. """ def test_severity_always_equals_reported_severity(self): @@ -700,5 +714,255 @@ def test_happy_path_stage_status_is_complete_so_119_would_not_banner_it(self): self.assertIsNone(adjudication_stage["reason"]) +def _make_judge(verdict="CONFIRMED", **overrides): + """A judge returning ``verdict`` (default CONFIRMED) plus whatever keys + ``overrides`` supplies -- used throughout STEP 6's tests to inject a + judge that re-rates, refuses, or refutes without hand-writing a callable + per test. + """ + + def _judge(finding: dict, document: dict) -> dict: + result = {"verdict": verdict, "verdict_evidence": "judge examined it directly"} + result.update(overrides) + return result + + return _judge + + +class SeverityRerateTests(unittest.TestCase): + """STEP 6's severity re-rating guard: legal re-ratings (both directions), + illegal ones (refused), and the no-op case, all against `adjudicate()` + directly. + """ + + def test_no_severity_key_is_unchanged_from_step_3_4(self): + finding = make_raw_finding(severity="High") + input_doc = make_document(reports=[make_report(findings_list=[finding])]) + + output_doc = run_adjudication.adjudicate(input_doc, _make_judge()) + + adjudicated = output_doc["reports"][0]["findings"][0] + self.assertEqual(adjudicated["severity"], "High") + self.assertEqual(adjudicated["reported_severity"], "High") + self.assertIsNone(adjudicated["severity_reason"]) + self.assertEqual(output_doc["adjudication"]["downgrades"], []) + + def test_severity_equal_to_reported_is_treated_as_no_rerating(self): + finding = make_raw_finding(severity="High") + input_doc = make_document(reports=[make_report(findings_list=[finding])]) + + output_doc = run_adjudication.adjudicate(input_doc, _make_judge(severity="High")) + + adjudicated = output_doc["reports"][0]["findings"][0] + self.assertEqual(adjudicated["severity"], "High") + self.assertIsNone(adjudicated["severity_reason"]) + self.assertEqual(output_doc["adjudication"]["downgrades"], []) + + def test_legal_downgrade_blocker_to_low_is_recorded_with_reason(self): + finding = make_raw_finding(severity="Blocker") + input_doc = make_document(reports=[make_report(findings_list=[finding])]) + fid = finding["finding_id"] + + output_doc = run_adjudication.adjudicate( + input_doc, + _make_judge(severity="Low", severity_reason="on inspection this is cosmetic"), + ) + + adjudicated = output_doc["reports"][0]["findings"][0] + self.assertEqual(adjudicated["reported_severity"], "Blocker") + self.assertEqual(adjudicated["severity"], "Low") + self.assertEqual(adjudicated["severity_reason"], "on inspection this is cosmetic") + self.assertEqual( + output_doc["adjudication"]["downgrades"], + [ + { + "finding_id": fid, + "from": "Blocker", + "to": "Low", + "reason": "on inspection this is cosmetic", + } + ], + ) + self.assertEqual(verdicts.validate(input_doc, output_doc), []) + + def test_downgrade_with_no_judge_reason_gets_a_generated_default(self): + finding = make_raw_finding(severity="Blocker") + input_doc = make_document(reports=[make_report(findings_list=[finding])]) + + output_doc = run_adjudication.adjudicate(input_doc, _make_judge(severity="Low")) + + adjudicated = output_doc["reports"][0]["findings"][0] + self.assertTrue(adjudicated["severity_reason"]) + self.assertEqual(len(output_doc["adjudication"]["downgrades"]), 1) + self.assertEqual(output_doc["adjudication"]["downgrades"][0]["reason"], adjudicated["severity_reason"]) + self.assertEqual(verdicts.validate(input_doc, output_doc), []) + + def test_legal_upgrade_is_not_a_downgrade_but_still_needs_a_reason(self): + finding = make_raw_finding(severity="Low") + input_doc = make_document(reports=[make_report(findings_list=[finding])]) + + output_doc = run_adjudication.adjudicate( + input_doc, _make_judge(severity="Blocker", severity_reason="worse than reported") + ) + + adjudicated = output_doc["reports"][0]["findings"][0] + self.assertEqual(adjudicated["reported_severity"], "Low") + self.assertEqual(adjudicated["severity"], "Blocker") + self.assertEqual(adjudicated["severity_reason"], "worse than reported") + self.assertEqual(output_doc["adjudication"]["downgrades"], []) + self.assertEqual(verdicts.validate(input_doc, output_doc), []) + + def test_illegal_severity_over_legal_reported_severity_is_unproven_at_reported(self): + # The scenario STEP 6's own done-when names precisely: a judge + # re-rates a LEGALLY in-ladder reported_severity to an out-of-ladder + # value. STEP 3's input validation does not catch this -- the input + # was legal -- only this stage's own re-rating guard does. + finding = make_raw_finding(severity="Medium") + input_doc = make_document(reports=[make_report(findings_list=[finding])]) + + output_doc = run_adjudication.adjudicate( + input_doc, _make_judge(verdict="CONFIRMED", severity="Info") + ) + + adjudicated = output_doc["reports"][0]["findings"][0] + self.assertEqual(adjudicated["verdict"], "UNPROVEN") + self.assertEqual(adjudicated["reported_severity"], "Medium") + self.assertEqual(adjudicated["severity"], "Medium") + self.assertTrue(adjudicated["severity_reason"]) + self.assertIn("Info", adjudicated["severity_reason"]) + self.assertEqual(output_doc["adjudication"]["downgrades"], []) + self.assertEqual(verdicts.validate(input_doc, output_doc), []) + # The positive form, used bare on purpose: #119's own `.get(sev, 9)` + # default would silently mask an out-of-ladder emission here. + for report in output_doc["reports"]: + for f in report["findings"]: + review.SEVERITY_ORDER[f["severity"]] + + def test_illegal_severity_is_never_added_to_downgrades(self): + finding = make_raw_finding(severity="High") + input_doc = make_document(reports=[make_report(findings_list=[finding])]) + + output_doc = run_adjudication.adjudicate(input_doc, _make_judge(severity="Info")) + + self.assertEqual(output_doc["adjudication"]["downgrades"], []) + + +class BareSeverityOrderSubscriptTests(unittest.TestCase): + """The positive form of the out-of-ladder guard, run over every finding + in a document containing every re-rating shape at once: a bare + `review.SEVERITY_ORDER[f["severity"]]` subscript must succeed for all of + them. Bare on purpose -- #119 defends itself with `.get(severity, 9)`, + and a control borrowing that default would pass on exactly the output + this stage must not emit. + """ + + def test_bare_subscript_succeeds_for_every_finding_across_every_rerating_shape(self): + no_rerate = make_raw_finding(dimension="a", severity="Medium") + downgraded = make_raw_finding(dimension="b", severity="Blocker") + upgraded = make_raw_finding(dimension="c", severity="Low") + refused = make_raw_finding(dimension="d", severity="High") + report = make_report( + dimension="mixed", + findings_list=[no_rerate, downgraded, upgraded, refused], + ) + input_doc = make_document(reports=[report]) + + def judge(finding: dict, document: dict) -> dict: + by_id = { + no_rerate["finding_id"]: {}, + downgraded["finding_id"]: {"severity": "Low"}, + upgraded["finding_id"]: {"severity": "Blocker"}, + refused["finding_id"]: {"severity": "Info"}, + } + extra = by_id[finding["finding_id"]] + result = {"verdict": "CONFIRMED", "verdict_evidence": "checked"} + result.update(extra) + if "severity" in extra and extra["severity"] in review.SEVERITY_ORDER: + result["severity_reason"] = "re-rated for this test" + return result + + output_doc = run_adjudication.adjudicate(input_doc, judge) + + for r in output_doc["reports"]: + for f in r["findings"]: + review.SEVERITY_ORDER[f["severity"]] # must not raise + + self.assertEqual(verdicts.validate(input_doc, output_doc), []) + self.assertEqual(len(output_doc["adjudication"]["downgrades"]), 1) + self.assertEqual(output_doc["adjudication"]["downgrades"][0]["finding_id"], downgraded["finding_id"]) + + +class TotalRefutationStatusTests(unittest.TestCase): + """STEP 6's total-refutation status: the `stages` adjudication entry + reports `"total_refutation"` (never "complete") when every finding is + REFUTED, and the zero-findings case is never flagged. + """ + + def test_every_finding_refuted_is_flagged_and_stage_status_is_not_complete(self): + findings_list = [ + make_raw_finding(dimension="a"), + make_raw_finding(dimension="b"), + ] + report = make_report(dimension="mixed", findings_list=findings_list) + input_doc = make_document(reports=[report]) + + output_doc = run_adjudication.adjudicate(input_doc, _make_judge(verdict="REFUTED")) + + self.assertEqual( + [f["finding_id"] for f in output_doc["reports"][0]["findings"]], + [f["finding_id"] for f in findings_list], + ) + self.assertEqual(output_doc["reports"][0]["findings_count"], 2) + self.assertTrue(output_doc["adjudication"]["total_refutation"]) + adjudication_stage = next( + entry for entry in output_doc["stages"] if entry["name"] == "adjudication" + ) + self.assertNotEqual(adjudication_stage["status"], "complete") + self.assertEqual(adjudication_stage["status"], "total_refutation") + self.assertTrue(adjudication_stage["reason"]) + self.assertEqual(verdicts.validate(input_doc, output_doc), []) + + def test_zero_findings_is_not_total_refutation_and_stage_is_complete(self): + report = make_report(dimension="clean", findings_list=[]) + input_doc = make_document(reports=[report]) + + output_doc = run_adjudication.adjudicate(input_doc, _make_judge(verdict="REFUTED")) + + self.assertFalse(output_doc["adjudication"]["total_refutation"]) + adjudication_stage = next( + entry for entry in output_doc["stages"] if entry["name"] == "adjudication" + ) + self.assertEqual(adjudication_stage["status"], "complete") + self.assertEqual(verdicts.validate(input_doc, output_doc), []) + + +class NothingRemovedAssertionTests(unittest.TestCase): + """STEP 6's "nothing is removed" reassertion inside `adjudicate()` + itself -- proven here by monkeypatching `_collect_finding_ids` to lie + about the output set, since the function does not otherwise ever drop or + invent a finding_id by construction. This is deliberately a whitebox + test of a belt-and-braces check that has no other way to fail. + """ + + def test_a_finding_id_mismatch_raises_before_returning(self): + input_doc = make_document() + real_collect = run_adjudication._collect_finding_ids + calls = {"n": 0} + + def lying_collect(document: dict) -> set[str]: + calls["n"] += 1 + ids = real_collect(document) + # Lie only on the SECOND call (the output-document call) so the + # input-side call still reflects the truth, matching what a real + # drop/invent defect would look like. + if calls["n"] == 2: + return ids | {"invented-id-not-really-present"} + return ids + + with mock.patch.object(run_adjudication, "_collect_finding_ids", lying_collect): + with self.assertRaises(run_adjudication.FindingSetIntegrityError): + run_adjudication.adjudicate(input_doc, run_adjudication.stub_judge) + + if __name__ == "__main__": unittest.main() From e79da04fbb309042975e2487cf6044202db56fba Mon Sep 17 00:00:00 2001 From: Serina Mcfall Date: Fri, 21 Aug 2026 11:52:22 +1200 Subject: [PATCH 2/5] fix(launchpad): guard against an unhashable judge-returned severity (#118 STEP 6) review-code found that _run_judge_safely forwarded a judge's severity value with no type check, and _apply_severity_rerating's `proposed_severity not in review.SEVERITY_ORDER` raises TypeError on an unhashable value (a list or dict) instead of failing closed to UNPROVEN -- reachable today through make_replay_judge (a malformed --replay recording), confirmed by reproducing the crash before fixing it. Fixed by only forwarding severity/severity_reason when severity is a str, matching the type discipline verdict/verdict_evidence already get in the same function. A non-string severity is now treated as no re-rating at all rather than crashing the whole run. Signed-off-by: Serina Mcfall --- launchpad/review-agent/run_adjudication.py | 13 ++++++++++--- .../review-agent/test_run_adjudication.py | 19 +++++++++++++++++++ 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/launchpad/review-agent/run_adjudication.py b/launchpad/review-agent/run_adjudication.py index e3a58fc5af3..8341d813785 100644 --- a/launchpad/review-agent/run_adjudication.py +++ b/launchpad/review-agent/run_adjudication.py @@ -447,10 +447,17 @@ def _run_judge_safely(judge: Judge, finding: dict, input_document: dict) -> dict } safe_result = {"verdict": verdict, "verdict_evidence": evidence} - if "severity" in result: + # A judge-supplied `severity` must be a string before it is ever compared + # against `review.SEVERITY_ORDER` (a dict) with `in` -- an unhashable + # value (a list, a dict) raises TypeError there rather than failing + # closed, exactly the crash-instead-of-UNPROVEN outcome this function + # exists to prevent. Same type discipline `verdict`/`evidence` already + # get above; a non-string severity is treated as no re-rating at all, + # never forwarded to _apply_severity_rerating. + if isinstance(result.get("severity"), str): safe_result["severity"] = result["severity"] - if "severity_reason" in result: - safe_result["severity_reason"] = result["severity_reason"] + if "severity_reason" in result: + safe_result["severity_reason"] = result["severity_reason"] return safe_result diff --git a/launchpad/review-agent/test_run_adjudication.py b/launchpad/review-agent/test_run_adjudication.py index 6f2b08f1a6a..b0a0e556dd3 100644 --- a/launchpad/review-agent/test_run_adjudication.py +++ b/launchpad/review-agent/test_run_adjudication.py @@ -846,6 +846,25 @@ def test_illegal_severity_is_never_added_to_downgrades(self): self.assertEqual(output_doc["adjudication"]["downgrades"], []) + def test_unhashable_severity_fails_closed_instead_of_crashing(self): + # A judge (or a malformed --replay recording) returning a severity + # that isn't even a string -- a list or dict -- must fail closed the + # same as any other unusable output, never raise TypeError from + # `proposed_severity not in review.SEVERITY_ORDER`'s `in` check. + finding = make_raw_finding(severity="High") + input_doc = make_document(reports=[make_report(findings_list=[finding])]) + + for bad_severity in (["Blocker"], {"value": "Blocker"}): + with self.subTest(bad_severity=bad_severity): + output_doc = run_adjudication.adjudicate( + input_doc, _make_judge(severity=bad_severity) + ) + adjudicated = output_doc["reports"][0]["findings"][0] + self.assertEqual(adjudicated["severity"], "High") + self.assertEqual(adjudicated["reported_severity"], "High") + self.assertEqual(output_doc["adjudication"]["downgrades"], []) + self.assertEqual(verdicts.validate(input_doc, output_doc), []) + class BareSeverityOrderSubscriptTests(unittest.TestCase): """The positive form of the out-of-ladder guard, run over every finding From b5e934904d8b6e5e56c9ab7d16db4edc793d7851 Mon Sep 17 00:00:00 2001 From: Serina Mcfall Date: Fri, 21 Aug 2026 12:26:25 +1200 Subject: [PATCH 3/5] feat(launchpad): dedupe via an injectable second judge (#118 STEP 7) Groups findings that describe the same defect in different words, possibly from different review dimensions (finding_id differs by construction since dimension is a hash input), into adjudication.duplicate_groups with a deterministic survivor and a duplicate_of back-reference on every non-survivor -- discoverable from either the finding or the top-level block. The Judge protocol (judge(finding, input_document) -> dict) is called once per finding, independently, so it structurally cannot see across findings to detect a duplicate. Rather than reshape that existing, already-tested protocol, dedupe gets its own separate injectable callable, dedupe_judge, called once after every finding is adjudicated over the full list -- mirroring how the primary judge already defaults to stub_judge to prove the harness before a real model exists. stub_dedupe_judge finds no duplicates by default: never merging incorrectly is safer than merging wrongly. Survivor selection (highest severity, then CONFIRMED > UNPROVEN > REFUTED, then lowest finding_id) is deterministic code, independent of whichever mechanism decides who is a duplicate of whom. verdicts.validate already rejects a duplicate_of naming an absent id or itself (STEP 2); confirmed here with tests, not reimplemented. Signed-off-by: Serina Mcfall --- launchpad/review-agent/run_adjudication.py | 217 +++++++++++++++- .../review-agent/test_run_adjudication.py | 244 +++++++++++++++++- 2 files changed, 450 insertions(+), 11 deletions(-) diff --git a/launchpad/review-agent/run_adjudication.py b/launchpad/review-agent/run_adjudication.py index 8341d813785..3d0f394b224 100644 --- a/launchpad/review-agent/run_adjudication.py +++ b/launchpad/review-agent/run_adjudication.py @@ -55,9 +55,8 @@ model" out of scope here, per #117's own framing and #118's issue: this module never names one, and neither flag lets a caller supply one. -Dedupe is STEP 7's job, layered on top of this module later -- -``duplicate_of`` is always null here. Severity re-rating and the -escalate-only guard are STEP 6, below. +Severity re-rating and the escalate-only guard are STEP 6, below. Dedupe is +STEP 7, further below. **STEP 6 -- severity re-rating and the out-of-ladder guard.** A judge's return dict MAY now also carry a ``severity`` key and, when re-rating, a @@ -163,6 +162,52 @@ the nonce was established. STEP 6's total-refutation flag does not exist yet -- when it lands it becomes a third condition ANDed into ``adjudicate()``'s ``stage_complete`` computation below, not a rewrite of it. + +**STEP 7 -- dedupe, and the mechanism ADJUDICATION.md deliberately leaves +open.** ADJUDICATION.md § Dedupe states the *outcome* contract -- a group is +``{survivor, duplicates: [finding_id]}`` in ``adjudication.duplicate_groups``, +every duplicate also carries ``duplicate_of``, a duplicate is still emitted +with its own verdict, and the survivor is chosen deterministically (highest +severity, then CONFIRMED before UNPROVEN before REFUTED, then lowest +``finding_id``) -- but it does not, and structurally cannot, say *how* two +findings are recognised as describing the same defect. ``Judge`` is +``judge(finding, input_document) -> dict``, called once per finding, +independently; it has no visibility into any other finding, so it cannot +detect a cross-finding duplicate by construction, and a cross-dimension +duplicate has a different ``finding_id`` by construction too (``dimension`` +is one of ``finding_id``'s hash inputs -- see FINDINGS.md). + +This module's answer is a **second, separate injectable callable**, +``dedupe_judge: DedupeJudge`` (default ``stub_dedupe_judge``, below) -- +mirroring how ``judge: Judge`` already defaults to ``stub_judge`` to prove the +harness end to end before a single prompt is written. It is called **once**, +after every finding has already been adjudicated (so it sees each finding's +final ``verdict``/``severity``, not the raw #117 input), and returns which +sets of ``finding_id`` s -- if any -- describe the same defect; it does +*not* choose the survivor itself. Survivor selection is ADJUDICATION.md's +own deterministic rule, applied here in ``_build_duplicate_groups`` / +``_survivor_sort_key``, and is the same code path regardless of which +dedupe mechanism decided the grouping -- the two are independent axes on +purpose, the same reason ``judge``'s per-finding verdict and STEP 6's +severity re-rating are independent fields on one return dict rather than one +combined decision. + +The alternative considered and set aside: folding dedupe into a single +richer ``Judge`` call/return shape. That would need ``Judge`` itself to see +every finding at once (a signature change reaching STEPs 3/4/6's existing +call sites and tests) to decide something that is, conceptually, a +completely separate question from "is this one finding true". A second +callable keeps ``Judge`` exactly as STEPs 3/4/6 already built it, at the +cost of one more injection point -- the smaller, less disruptive change, +and the one this module takes. + +``stub_dedupe_judge`` finds **no duplicates**, by design, not merely because +nothing else exists yet: never merging incorrectly is safer than merging +findings that turn out not to share a defect, the identical fail-safe +direction ADJUDICATION.md already states for the per-finding default +(``UNPROVEN``, never ``REFUTED``). A ``dedupe_judge`` that crashes or returns +something unusable fails closed to the same "no duplicates" answer +(``_run_dedupe_safely``), never to a partial or guessed grouping. """ from __future__ import annotations @@ -189,6 +234,22 @@ #: the first place. Judge = Callable[[dict, dict], dict] +#: STEP 7's dedupe protocol: ``dedupe_judge(adjudicated_findings, +#: input_document) -> list[list[str]]``, called ONCE after every finding has +#: already been adjudicated (each finding dict in ``adjudicated_findings`` +#: already carries its own ``verdict``/``severity``/etc.), never once per +#: finding like ``Judge`` above. Returns zero or more groups, each a list of +#: two-or-more ``finding_id`` strings describing the same defect -- the +#: dedupe judge decides WHICH findings are duplicates of each other; it does +#: not choose the survivor, which is ADJUDICATION.md's own deterministic rule +#: (see ``_build_duplicate_groups``/``_survivor_sort_key`` below), applied +#: identically regardless of which dedupe judge produced the grouping. A +#: raised exception, a non-list return, a group naming fewer than two real +#: finding_ids, or a finding_id already claimed by an earlier group is +#: handled defensively by ``_run_dedupe_safely``/``_build_duplicate_groups`` +#: -- never a crash, and never a silently wrong merge. +DedupeJudge = Callable[[list, dict], list] + class InputValidationError(ValueError): """Raised by ``adjudicate()`` when the input document fails #117's own @@ -367,6 +428,22 @@ def stub_judge(finding: dict, document: dict) -> dict: } +def stub_dedupe_judge(adjudicated_findings: list, input_document: dict) -> list: + """The default dedupe judge (STEP 7). Finds **no duplicates** -- not + merely because no real dedupe mechanism exists yet, but because it is + the conservative, fail-safe default: never merging two findings is + always safer than merging findings that turn out not to share a defect, + the same fail-closed direction ADJUDICATION.md already states for the + per-finding verdict default (``UNPROVEN``, never ``REFUTED``). + + Exists to prove STEP 7's harness -- ``adjudicate()``'s dedupe wiring, + survivor selection, and ``duplicate_groups``/``duplicate_of`` emission -- + end to end before a single cross-finding dedupe mechanism is built, the + same reason ``stub_judge`` exists for the per-finding verdict. + """ + return [] + + def make_replay_judge(replay_dir: Path) -> Judge: """Build a judge that replays recorded judge outputs from ``replay_dir`` (STEP 9's future recordings) instead of calling a live model. @@ -527,6 +604,98 @@ def _apply_severity_rerating( return verdict, proposed_severity, reason +#: Tie-break rank for ADJUDICATION.md § Dedupe's survivor rule: "CONFIRMED +#: before UNPROVEN before REFUTED". Lower is better, same convention as +#: ``review.SEVERITY_ORDER`` (Blocker=0 is the most severe). +_VERDICT_SURVIVOR_RANK = {"CONFIRMED": 0, "UNPROVEN": 1, "REFUTED": 2} + + +def _survivor_sort_key(finding: dict) -> tuple: + """ADJUDICATION.md § Dedupe's survivor rule, as a sort key: highest + adjudicated severity, then CONFIRMED before UNPROVEN before REFUTED, + then lowest ``finding_id`` -- the minimum of this key over a group is + the survivor. Deliberately independent of *how* the group was formed: + this is the same computation regardless of which ``dedupe_judge`` + decided two findings are duplicates. + + A finding whose ``severity``/``verdict`` is somehow not a legal ladder + value sorts last on that axis (worse than every legal value) rather than + raising -- defensive in the same style as this module's other guards, + though ``adjudicate()`` only ever calls this after STEP 6's re-rating + guard has already guaranteed both fields are legal. + """ + severity_rank = review.SEVERITY_ORDER.get(finding.get("severity"), len(review.SEVERITY_ORDER)) + verdict_rank = _VERDICT_SURVIVOR_RANK.get(finding.get("verdict"), len(_VERDICT_SURVIVOR_RANK)) + finding_id = finding.get("finding_id") if isinstance(finding.get("finding_id"), str) else "" + return (severity_rank, verdict_rank, finding_id) + + +def _run_dedupe_safely( + dedupe_judge: DedupeJudge, adjudicated_findings: list, input_document: dict +) -> list: + """Call ``dedupe_judge`` once and fail closed to "no duplicates" (``[]``) + on anything unusable -- a raised exception or a non-list return. Mirrors + ``_run_judge_safely``'s "the judge's own crash or garbage output must not + abort or corrupt the run" discipline, applied to the second, dedupe-only + injection point: a dedupe judge that misbehaves must never silently + merge findings it did not actually decide were duplicates. + """ + try: + raw_groups = dedupe_judge(adjudicated_findings, input_document) + except Exception: # noqa: BLE001 -- a dedupe judge's own crash must fail + # closed to no duplicates, exactly like a per-finding judge's crash + # fails closed to UNPROVEN in `_run_judge_safely` above. + return [] + return raw_groups if isinstance(raw_groups, list) else [] + + +def _build_duplicate_groups(raw_groups: list, findings_by_id: dict) -> list[dict]: + """Turn ``dedupe_judge``'s raw ``list[list[finding_id]]`` into + ADJUDICATION.md's ``{survivor, duplicates: [finding_id]}`` shape, with + the survivor chosen by ``_survivor_sort_key``. + + Defensive against a dedupe judge returning something it should not, + the same discipline this module already applies to a misbehaving + per-finding ``Judge``: + * a non-list group, or a ``finding_id`` that is not a string, is + dropped from that group rather than raising; + * a ``finding_id`` naming a finding not present in ``findings_by_id`` + is dropped -- a dedupe judge can only group findings that were + actually adjudicated; + * a group left with fewer than two distinct real finding_ids after the + above is not a group at all, and is dropped entirely; + * a ``finding_id`` already claimed by an earlier group is dropped from + every later group, so one duplicate can never point at two + survivors -- first group wins, applied in the order + ``dedupe_judge`` returned them. + Never raises: a dedupe judge cannot break ``adjudicate()`` by returning + a malformed grouping, it can only fail to have its grouping honoured. + """ + groups: list[dict] = [] + claimed: set[str] = set() + for raw_group in raw_groups: + if not isinstance(raw_group, list): + continue + candidate_ids: list[str] = [] + for fid in raw_group: + if ( + isinstance(fid, str) + and fid in findings_by_id + and fid not in claimed + and fid not in candidate_ids + ): + candidate_ids.append(fid) + if len(candidate_ids) < 2: + continue + survivor = min((findings_by_id[fid] for fid in candidate_ids), key=_survivor_sort_key)[ + "finding_id" + ] + duplicates = sorted(fid for fid in candidate_ids if fid != survivor) + groups.append({"survivor": survivor, "duplicates": duplicates}) + claimed.update(candidate_ids) + return groups + + def _collect_finding_ids(document: dict) -> set[str]: """The set of every ``finding_id`` present across ``document``'s ``reports[].findings``. @@ -557,9 +726,13 @@ def _collect_finding_ids(document: dict) -> set[str]: return ids -def adjudicate(input_document: dict, judge: Judge) -> dict: - """Adjudicate every finding in ``input_document`` with ``judge`` and - return the adjudicated output document. Never mutates ``input_document``. +def adjudicate( + input_document: dict, judge: Judge, dedupe_judge: DedupeJudge = stub_dedupe_judge +) -> dict: + """Adjudicate every finding in ``input_document`` with ``judge``, group + duplicates with ``dedupe_judge`` (STEP 7; defaults to ``stub_dedupe_judge``, + which finds none), and return the adjudicated output document. Never + mutates ``input_document``. Raises ``AlreadyAdjudicatedError`` when ``input_document["stages"]`` already carries an ``adjudication`` entry (a re-run), and @@ -578,13 +751,21 @@ def adjudicate(input_document: dict, judge: Judge) -> dict: Pass-through fields (``pr``, ``merge_base_sha``, ``head_sha``, ``containment``) are never touched: the output starts as a ``copy.deepcopy`` of the input, and only a finding dict's own six new keys - are ever written. Dedupe is a later step's job -- ``duplicate_of`` is - always null here. Severity re-rating and the out-of-ladder guard are + are ever written. Severity re-rating and the out-of-ladder guard are STEP 6 (see the module docstring's STEP 6 section and ``_apply_severity_rerating``); a judge that never re-rates leaves every finding's ``severity`` exactly equal to its ``reported_severity``, same as STEP 3/4. + Dedupe (STEP 7) runs once, after every finding already has its final + ``verdict``/``severity`` -- ``dedupe_judge`` is called exactly once with + the full list of adjudicated findings, never once per finding, and its + grouping is turned into ``adjudication.duplicate_groups`` plus each + duplicate's own ``duplicate_of`` by ``_build_duplicate_groups`` (see the + module docstring's STEP 7 section). A duplicate is never removed from + ``reports[].findings`` -- it keeps its own verdict and is still counted + in ``findings_out``. + Before returning, asserts (raising ``FindingSetIntegrityError`` on failure) that ``output_document``'s ``finding_id`` set equals ``input_document``'s -- STEP 6's "nothing is removed" reassertion, run @@ -654,12 +835,30 @@ def adjudicate(input_document: dict, judge: Judge) -> dict: findings_out = findings_in total_refutation = findings_in > 0 and verdict_counts["REFUTED"] == findings_in + # STEP 7 -- dedupe. Runs once, after every finding above already carries + # its final verdict/severity, and never removes or re-counts anything: + # `findings_out` (computed above) is unaffected by grouping. + output_findings_by_id: dict[str, dict] = {} + adjudicated_findings: list[dict] = [] + for report in output_document.get("reports", []): + for finding in report.get("findings", []): + adjudicated_findings.append(finding) + fid = finding.get("finding_id") + if isinstance(fid, str): + output_findings_by_id[fid] = finding + + raw_dedupe_groups = _run_dedupe_safely(dedupe_judge, adjudicated_findings, input_document) + duplicate_groups = _build_duplicate_groups(raw_dedupe_groups, output_findings_by_id) + for group in duplicate_groups: + for dup_id in group["duplicates"]: + output_findings_by_id[dup_id]["duplicate_of"] = group["survivor"] + output_document["adjudication"] = verdicts.Adjudication( schema_version=1, verdict_counts=verdict_counts, findings_in=findings_in, findings_out=findings_out, - duplicate_groups=[], + duplicate_groups=duplicate_groups, downgrades=downgrades, total_refutation=total_refutation, notes=[], diff --git a/launchpad/review-agent/test_run_adjudication.py b/launchpad/review-agent/test_run_adjudication.py index b0a0e556dd3..c3456863fe9 100644 --- a/launchpad/review-agent/test_run_adjudication.py +++ b/launchpad/review-agent/test_run_adjudication.py @@ -38,8 +38,18 @@ (`adjudication.downgrades` names it with from/to/reason). See `SeverityRerateTests` and `TotalRefutationStatusTests` below. -Deliberately NOT exercised here (later steps' territory, per the plan): -dedupe (STEP 7). +Also exercises STEP 7's own done-when: given two findings from two +dimensions describing one planted defect (a ``dedupe_judge`` injected to +report them as duplicates of each other), both are present in the output, +both carry a verdict, exactly one carries `duplicate_of` naming the other, +and `duplicate_groups` carries one group naming both; the survivor is the +same across two runs of the same input, asserted by byte-comparing the two +outputs; a finding whose `duplicate_of` names an id absent from the document, +and one naming itself, are each rejected by `verdicts.validate` (that +validator already exists from STEP 2 -- confirmed here, not reimplemented); +and a run that dedupes nothing (the default `stub_dedupe_judge`) emits an +EMPTY `duplicate_groups` array rather than omitting the key. See +`DedupeTests` below. This file is scoped to `run_adjudication.py` alone and is deliberately not wired into `run_controls.py`'s CONTROLS list -- that is STEP 10's control @@ -983,5 +993,235 @@ def lying_collect(document: dict) -> set[str]: run_adjudication.adjudicate(input_doc, run_adjudication.stub_judge) +def _pairing_dedupe_judge(fid_a: str, fid_b: str): + """A ``dedupe_judge`` that always reports exactly one group: ``fid_a`` + and ``fid_b`` are the same defect. Used throughout ``DedupeTests`` in + place of a real cross-finding dedupe mechanism -- STEP 7's own harness + is what is under test, not any particular mechanism (see + ``run_adjudication``'s module docstring, STEP 7 section, for why the + mechanism is a separate, injectable callable at all). + """ + + def _dedupe(adjudicated_findings: list, document: dict) -> list: + return [[fid_a, fid_b]] + + return _dedupe + + +class DedupeTests(unittest.TestCase): + """STEP 7's own done-when, in full: two findings from two dimensions + describing one planted defect, both emitted with their own verdict and + exactly one carrying `duplicate_of`; `duplicate_groups` naming both; + survivor determinism proven by byte-comparing two runs; `verdicts. + validate` already rejecting a `duplicate_of` naming an absent id or + itself (STEP 2's validator, confirmed here rather than reimplemented); + and the empty-not-missing `duplicate_groups` key on a run that dedupes + nothing. + """ + + def _two_dimension_document(self, severity="High") -> tuple[dict, str, str]: + """Two findings, two different dimensions, describing one planted + defect in different words -- different `finding_id`s by + construction, since `dimension` is one of `finding_id`'s hash + inputs. Returns ``(input_doc, finding_id_a, finding_id_b)``. + """ + finding_a = make_raw_finding( + dimension="secrets-and-access", + defect="hardcoded credential in connection string", + severity=severity, + ) + finding_b = make_raw_finding( + dimension="access-control", + defect="database password embedded directly in source", + severity=severity, + ) + input_doc = make_document( + reports=[ + make_report(dimension="secrets-and-access", findings_list=[finding_a]), + make_report(dimension="access-control", findings_list=[finding_b]), + ] + ) + return input_doc, finding_a["finding_id"], finding_b["finding_id"] + + def test_both_findings_present_with_their_own_verdict_and_grouped(self): + input_doc, fid_a, fid_b = self._two_dimension_document() + + output_doc = run_adjudication.adjudicate( + input_doc, _make_judge(verdict="CONFIRMED"), dedupe_judge=_pairing_dedupe_judge(fid_a, fid_b) + ) + + all_findings = [f for r in output_doc["reports"] for f in r["findings"]] + self.assertEqual({f["finding_id"] for f in all_findings}, {fid_a, fid_b}) + for f in all_findings: + self.assertEqual(f["verdict"], "CONFIRMED") + + # Equal severity and verdict on both sides -- the tiebreaker is the + # lowest finding_id, per ADJUDICATION.md § Dedupe. + survivor, duplicate = sorted([fid_a, fid_b]) + by_id = {f["finding_id"]: f for f in all_findings} + self.assertIsNone(by_id[survivor]["duplicate_of"]) + self.assertEqual(by_id[duplicate]["duplicate_of"], survivor) + + self.assertEqual( + output_doc["adjudication"]["duplicate_groups"], + [{"survivor": survivor, "duplicates": [duplicate]}], + ) + self.assertEqual(output_doc["adjudication"]["findings_out"], 2) + self.assertEqual(verdicts.validate(input_doc, output_doc), []) + self.assertEqual(findings.validate(output_doc), []) + + def test_dedupe_judge_sees_the_adjudicated_findings_not_the_raw_ones(self): + # The dedupe judge is called ONCE, after every finding already has + # its final verdict/severity -- never once per finding like `Judge`. + input_doc, fid_a, fid_b = self._two_dimension_document() + seen: list[list[dict]] = [] + + def _recording_dedupe(adjudicated_findings, document): + seen.append(adjudicated_findings) + return [] + + run_adjudication.adjudicate( + input_doc, _make_judge(verdict="CONFIRMED"), dedupe_judge=_recording_dedupe + ) + + self.assertEqual(len(seen), 1, "dedupe_judge must be called exactly once") + (adjudicated_findings,) = seen + self.assertEqual(len(adjudicated_findings), 2) + for f in adjudicated_findings: + self.assertIn("verdict", f) + self.assertIn("severity", f) + + def test_survivor_prefers_highest_severity(self): + finding_a = make_raw_finding(dimension="a", defect="one defect", severity="Medium") + finding_b = make_raw_finding(dimension="b", defect="same defect worded differently", severity="Blocker") + input_doc = make_document( + reports=[ + make_report(dimension="a", findings_list=[finding_a]), + make_report(dimension="b", findings_list=[finding_b]), + ] + ) + fid_a, fid_b = finding_a["finding_id"], finding_b["finding_id"] + + output_doc = run_adjudication.adjudicate( + input_doc, _make_judge(verdict="CONFIRMED"), dedupe_judge=_pairing_dedupe_judge(fid_a, fid_b) + ) + + [group] = output_doc["adjudication"]["duplicate_groups"] + self.assertEqual(group["survivor"], fid_b, "the Blocker finding must survive over the Medium one") + self.assertEqual(verdicts.validate(input_doc, output_doc), []) + + def test_survivor_prefers_confirmed_over_unproven_over_refuted(self): + finding_a = make_raw_finding(dimension="a", defect="one defect", severity="High") + finding_b = make_raw_finding(dimension="b", defect="same defect worded differently", severity="High") + input_doc = make_document( + reports=[ + make_report(dimension="a", findings_list=[finding_a]), + make_report(dimension="b", findings_list=[finding_b]), + ] + ) + fid_a, fid_b = finding_a["finding_id"], finding_b["finding_id"] + + def judge(finding: dict, document: dict) -> dict: + verdict = "UNPROVEN" if finding["finding_id"] == fid_a else "CONFIRMED" + return {"verdict": verdict, "verdict_evidence": "checked independently"} + + output_doc = run_adjudication.adjudicate( + input_doc, judge, dedupe_judge=_pairing_dedupe_judge(fid_a, fid_b) + ) + + [group] = output_doc["adjudication"]["duplicate_groups"] + self.assertEqual(group["survivor"], fid_b, "CONFIRMED must survive over UNPROVEN at equal severity") + self.assertEqual(verdicts.validate(input_doc, output_doc), []) + + def test_survivor_is_the_same_across_two_runs_byte_for_byte(self): + input_doc, fid_a, fid_b = self._two_dimension_document() + + output_1 = run_adjudication.adjudicate( + input_doc, _make_judge(verdict="CONFIRMED"), dedupe_judge=_pairing_dedupe_judge(fid_a, fid_b) + ) + output_2 = run_adjudication.adjudicate( + input_doc, _make_judge(verdict="CONFIRMED"), dedupe_judge=_pairing_dedupe_judge(fid_a, fid_b) + ) + + self.assertEqual( + json.dumps(output_1, sort_keys=True), + json.dumps(output_2, sort_keys=True), + "two runs of the same input must agree on the same survivor, byte for byte", + ) + + def test_dedupe_judge_raising_fails_closed_to_no_duplicates(self): + input_doc, fid_a, fid_b = self._two_dimension_document() + + def _raising_dedupe(adjudicated_findings, document): + raise RuntimeError("boom") + + output_doc = run_adjudication.adjudicate( + input_doc, _make_judge(verdict="CONFIRMED"), dedupe_judge=_raising_dedupe + ) + + self.assertEqual(output_doc["adjudication"]["duplicate_groups"], []) + for f in [f for r in output_doc["reports"] for f in r["findings"]]: + self.assertIsNone(f["duplicate_of"]) + self.assertEqual(verdicts.validate(input_doc, output_doc), []) + + def test_dedupe_judge_returning_garbage_is_dropped_not_raised(self): + input_doc, fid_a, fid_b = self._two_dimension_document() + + def _garbage_dedupe(adjudicated_findings, document): + return [ + "not-a-list", # a group that is not a list at all + [fid_a], # too few real ids to be a group + [fid_a, "unknown-finding-id-not-in-document"], # references an absent id + 123, # not even a list-shaped entry + ] + + output_doc = run_adjudication.adjudicate( + input_doc, _make_judge(verdict="CONFIRMED"), dedupe_judge=_garbage_dedupe + ) + + self.assertEqual(output_doc["adjudication"]["duplicate_groups"], []) + for f in [f for r in output_doc["reports"] for f in r["findings"]]: + self.assertIsNone(f["duplicate_of"]) + self.assertEqual(verdicts.validate(input_doc, output_doc), []) + + def test_default_dedupe_judge_finds_no_duplicates_and_key_is_present_not_missing(self): + input_doc = make_document() + + output_doc = run_adjudication.adjudicate(input_doc, run_adjudication.stub_judge) + + self.assertIn("duplicate_groups", output_doc["adjudication"]) + self.assertEqual(output_doc["adjudication"]["duplicate_groups"], []) + self.assertEqual(verdicts.validate(input_doc, output_doc), []) + + def test_stub_dedupe_judge_directly_returns_no_groups(self): + finding = make_raw_finding() + self.assertEqual(run_adjudication.stub_dedupe_judge([finding], {}), []) + + def test_duplicate_of_naming_an_absent_id_is_rejected_by_validate(self): + # STEP 2's validator, confirmed here rather than reimplemented. + finding = make_raw_finding() + input_doc = make_document(reports=[make_report(findings_list=[finding])]) + output_doc = run_adjudication.adjudicate(input_doc, run_adjudication.stub_judge) + + output_doc["reports"][0]["findings"][0]["duplicate_of"] = "not-a-real-finding-id" + + violations = verdicts.validate(input_doc, output_doc) + self.assertTrue( + any("is not a finding_id present in the document" in v for v in violations), violations + ) + + def test_duplicate_of_naming_itself_is_rejected_by_validate(self): + # STEP 2's validator, confirmed here rather than reimplemented. + finding = make_raw_finding() + input_doc = make_document(reports=[make_report(findings_list=[finding])]) + output_doc = run_adjudication.adjudicate(input_doc, run_adjudication.stub_judge) + + fid = output_doc["reports"][0]["findings"][0]["finding_id"] + output_doc["reports"][0]["findings"][0]["duplicate_of"] = fid + + violations = verdicts.validate(input_doc, output_doc) + self.assertTrue(any("names itself" in v for v in violations), violations) + + if __name__ == "__main__": unittest.main() From 965c2325b93fa3abb0fb5646f4814798480d5b97 Mon Sep 17 00:00:00 2001 From: Serina Mcfall Date: Fri, 21 Aug 2026 16:57:10 +1200 Subject: [PATCH 4/5] Merge STEP 6 into STEP 7, carrying the stages-shape fix (#118) Brings #264's `StagesShapeError` / `_input_stages` and #261/#263's `verdicts.is_nonempty_str` up to the chain tip. Both of this PR's findings were homed on earlier branches -- the `stages` Blocker on #264 and the `notes` drift on #263 -- so this branch is cleared by propagation rather than by any change of its own, which is what the adjudication asked for. Clean merge, no conflicts. 230 tests across launchpad/review-agent. `notes` remains empty and is now documented as deferred at STEP 6/7, with the unresolved tension against `adjudicator.md` (#265) stated in the code. #265 should not merge ahead of that decision. Refs #118 Signed-off-by: Serina Mcfall From c73a9ba733c8623a5d839033ca423e4a67b681bb Mon Sep 17 00:00:00 2001 From: Serina Mcfall Date: Sat, 22 Aug 2026 09:26:50 +1200 Subject: [PATCH 5/5] fix(launchpad): guard the effective severity, not only a re-rating (#118 STEP 6) benmitchell11's independent pass on #266/#267 found that _apply_severity_rerating's no-re-rating branch returned reported_severity without ever checking it, so an out-of-ladder value the judge agreed with (or said nothing about) was published untouched: _apply_severity_rerating("x", "Info", "CONFIRMED", "Info", None, []) -> ('CONFIRMED', 'Info', None) Reproduced before fixing. This is a plan-conformance gap, not merely defence in depth: STEP 6's own done-when names this exact case -- "a guard watching only re-ratings never sees a finding that ARRIVED at 'Info' and was agreed with, and copies it into `severity` untouched" -- and ADJUDICATION.md promises the guarantee holds on the EFFECTIVE severity, the re-rating where there is one and reported_severity where there is not. Fixed by checking reported_severity inside that branch, ahead of the return that used to copy it: verdict becomes UNPROVEN, severity falls back to Blocker (not something smaller -- this stage may not decide an unrateable finding is minor), severity_reason names the refusal, and nothing is added to downgrades since no legal value fell. Unreachable through main() today (STEP 3's findings.validate refuses an out-of-ladder input severity before any judge runs) and kept as a real branch regardless: adjudicate() is importable, and STEP 10's control suite is planned to feed this function malformed values directly. Proved the new tests can fail: removing the guard on a scratch copy fails exactly the two new sub-cases while the legal-path control still passes. 71 tests OK, run_controls 13/13, all seven suites OK. Signed-off-by: Serina Mcfall --- launchpad/review-agent/run_adjudication.py | 29 ++++++++++++++ .../review-agent/test_run_adjudication.py | 40 +++++++++++++++++++ 2 files changed, 69 insertions(+) diff --git a/launchpad/review-agent/run_adjudication.py b/launchpad/review-agent/run_adjudication.py index ba30836c686..96d606e3272 100644 --- a/launchpad/review-agent/run_adjudication.py +++ b/launchpad/review-agent/run_adjudication.py @@ -572,6 +572,35 @@ def _apply_severity_rerating( reasoning: a sweep is a second place the two could disagree. """ if proposed_severity is None or proposed_severity == reported_severity: + # THE GUARD FIRES ON THE EFFECTIVE SEVERITY -- the value that will + # actually be emitted -- not only on a re-rating that differs from + # `reported_severity`. ADJUDICATION.md is explicit that both must be + # checked: "a finding arriving with an out-of-ladder + # `reported_severity` that the judge happens to agree with is never + # re-rated at all, so a guard watching only re-ratings never fires and + # the bad value is copied into `severity` untouched." This branch -- + # no re-rating, or a re-rating that agrees -- IS that path, so the + # check belongs here, ahead of the return that used to copy it. + if reported_severity not in review.SEVERITY_ORDER: + # There is no legal re-rating to refuse and no safe value to fall + # back to: the severity ARRIVED illegal and the judge either + # agreed with it or proposed nothing. `Blocker` rather than + # anything smaller, because this stage may not silently decide + # that an unrateable finding is a minor one. + # + # Unreachable through `main()` today -- STEP 3's + # `findings.validate` refuses an out-of-ladder input severity + # before any judge runs -- and kept as a real branch regardless: + # `adjudicate()` is importable by anything, and STEP 10's control + # suite is planned to feed this function malformed values + # directly. Defence in depth that the contract already promises + # is not the same as dead code. + reason = ( + f"finding {finding_id!r} carries an out-of-ladder reported severity " + f"{reported_severity!r} and the judge proposed no legal re-rating; " + "refused, falling back to 'Blocker'" + ) + return "UNPROVEN", "Blocker", reason # No re-rating: unchanged from STEP 3/4's behaviour. return verdict, reported_severity, None diff --git a/launchpad/review-agent/test_run_adjudication.py b/launchpad/review-agent/test_run_adjudication.py index ae9746cad2c..cee5b001ade 100644 --- a/launchpad/review-agent/test_run_adjudication.py +++ b/launchpad/review-agent/test_run_adjudication.py @@ -1123,6 +1123,46 @@ def test_unhashable_severity_fails_closed_instead_of_crashing(self): self.assertEqual(output_doc["adjudication"]["downgrades"], []) self.assertEqual(verdicts.validate(input_doc, output_doc), []) + def test_out_of_ladder_reported_severity_is_refused_even_when_agreed_with(self): + # STEP 6's own done-when: "a guard watching only re-ratings never sees + # a finding that ARRIVED at 'Info' and was agreed with, and copies it + # into `severity` untouched." Asserted against + # `_apply_severity_rerating` directly, because `main()` cannot reach + # this shape -- STEP 3's findings.validate refuses an out-of-ladder + # input severity before any judge runs -- so a document-level fixture + # would test STEP 3's gate instead of this guard. + # + # Both sub-cases produce the same effective severity, which is the + # point: agreement and silence are the same thing to this branch. + for proposed in ("Info", None): + with self.subTest(proposed=proposed): + downgrades: list[dict] = [] + verdict, severity, reason = run_adjudication._apply_severity_rerating( + "fid", "Info", "CONFIRMED", proposed, None, downgrades + ) + self.assertEqual(verdict, "UNPROVEN") + # Blocker, not something smaller: this stage may not decide an + # unrateable finding is a minor one. + self.assertEqual(severity, "Blocker") + self.assertIn("Info", reason) + self.assertTrue(reason) + # Nothing legally fell -- the value was refused, not compared. + self.assertEqual(downgrades, []) + + def test_legal_reported_severity_is_untouched_when_agreed_with(self): + # The control for the guard above: a LEGAL reported severity the judge + # agrees with (or says nothing about) must still pass through + # unchanged, with no reason and no verdict override. Without this, the + # guard above could pass by refusing everything. + for proposed in ("High", None): + with self.subTest(proposed=proposed): + downgrades: list[dict] = [] + verdict, severity, reason = run_adjudication._apply_severity_rerating( + "fid", "High", "CONFIRMED", proposed, None, downgrades + ) + self.assertEqual((verdict, severity, reason), ("CONFIRMED", "High", None)) + self.assertEqual(downgrades, []) + class BareSeverityOrderSubscriptTests(unittest.TestCase): """The positive form of the out-of-ladder guard, run over every finding