From dc4c4bf755c4b5d7cadf2507bd52ddf5f6fba385 Mon Sep 17 00:00:00 2001 From: Serina Mcfall Date: Fri, 21 Aug 2026 10:31:55 +1200 Subject: [PATCH 1/3] feat(launchpad): nonce check and stages manifest (#118 STEP 4) Extends run_adjudication.py's adjudicate() with two things it did not previously do: - _verify_nonce() checks the top-level `nonce` against every report's own completion marker and raises NonceVerificationError naming exactly one of three refusals, in fixed order: "absent provenance" (no top-level nonce, or a report's marker does not parse), "mixed document" (reports disagree with each other -- wins over the third case when both apply), and "mismatched envelope" (reports agree with each other but not the top-level key). Runs after #117's own findings.validate, never before, and never invents or accepts a caller-supplied nonce. - The output now carries a top-level `stages` array: every entry present on input plus exactly one new {name: "adjudication", status, reason} entry. AlreadyAdjudicatedError refuses a document whose `stages` already carries an `adjudication` entry, rather than silently overwriting it. `status` is "complete" only when every finding has a verdict and the nonce was established -- STEP 6's total-refutation flag isn't built yet, so it is not one of the two live conditions today, but the computation is structured so that flag slots in later without a rewrite. Because findings.validate() already checks every report's marker nonce against the top-level key, every fixture that exercises the three nonce refusals through main() end-to-end is *also* caught there first, with its own generic (and, between "mixed" and "mismatched", indistinguishable) message -- so the three refusals' own distinct reasons are proven directly against _verify_nonce with hand-built documents, not observable through the CLI today. Both are tested: the dedicated check is real defence in depth per ADJUDICATION.md's own stated reasoning, and main()'s "exits non-zero, prints no document" contract holds either way. Signed-off-by: Serina Mcfall --- launchpad/review-agent/run_adjudication.py | 227 +++++++++++++++- .../review-agent/test_run_adjudication.py | 254 +++++++++++++++++- 2 files changed, 472 insertions(+), 9 deletions(-) diff --git a/launchpad/review-agent/run_adjudication.py b/launchpad/review-agent/run_adjudication.py index 81a5f3a2f9f..ef3266dd59d 100644 --- a/launchpad/review-agent/run_adjudication.py +++ b/launchpad/review-agent/run_adjudication.py @@ -1,4 +1,5 @@ -"""The adjudication stage's CLI. Implements launchpad-26/buzz#118 STEP 3. +"""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). Reads one #117 **merged document** on stdin, adjudicates every finding with an **injected judge callable** -- defaulting to a stub that returns ``UNPROVEN`` @@ -57,6 +58,51 @@ 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. + +**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 +guarantees above, both implemented in this module because STEP 3 and STEP 4 +are two facets of one CLI: + +The top-level ``nonce`` is checked and passed through, **never generated**. +#117's own ``findings.validate`` -- which ``adjudicate()`` already runs first +-- rejects a document whose marker disagrees with the top-level key, so in +practice a document that clears that gate today already agrees everywhere. +This module checks it again anyway, for ADJUDICATION.md's own reason: this +stage is agnostic about its producer and may not inherit a guarantee it did +not watch being made. ``_verify_nonce`` below is that check, kept as its own +function precisely so it is testable directly against hand-built documents -- +the same reason ``verdicts.validate`` is tested against hand-built documents +rather than only against this module's own output -- since #117's validator +being this thorough today means the three refusals below are not otherwise +reachable through ``main()`` with a document that still satisfies #117's own +contract. + +Three refusals, checked in this fixed order because one document can satisfy +more than one at once: + 1. ``"absent provenance"`` -- no top-level ``nonce``, or no report carries a + parseable completion marker. Checked first because the other two need a + value to compare against. + 2. ``"mixed document"`` -- the reports disagree with EACH OTHER. Checked + second, and wins over 3 when a document exhibits both: a mixed document + is the larger fact and a header mismatch is its consequence. + 3. ``"mismatched envelope"`` -- the reports agree with each other but not + with the top-level key. +This module never picks a winner among disagreeing nonces and never accepts +a caller-supplied one -- ``_verify_nonce`` reads only ``document`` itself. + +The ``stages`` manifest. #117 emits no top-level ``stages`` array; it is the +manifest #119 reads for stages -- #116's pre-flight, this one -- that produce +no envelope of their own. ``adjudicate()`` copies through every entry already +present on input and appends exactly one new ``{name: "adjudication", status, +reason}`` entry. An input already carrying an ``adjudication`` entry is a +re-run against an already-adjudicated document, and is refused outright +(``AlreadyAdjudicatedError``) rather than silently overwritten. + +``status`` is ``"complete"`` only when every finding received a verdict and +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. """ from __future__ import annotations @@ -91,6 +137,124 @@ def __init__(self, violations: list[str]): super().__init__("input document fails findings.validate: " + "; ".join(violations)) +class NonceVerificationError(ValueError): + """Raised by ``_verify_nonce`` when the document's provenance cannot be + established -- one of the three refusals ADJUDICATION.md § The + ``adjudication`` block names. ``reason`` is one of ``"absent provenance"``, + ``"mixed document"`` or ``"mismatched envelope"``; ``detail`` is the + human-readable specifics. Kept as two separate attributes (rather than one + formatted string) so a caller -- ``main`` below, or a future control -- + can assert on the *category* without parsing prose. + """ + + def __init__(self, reason: str, detail: str): + self.reason = reason + self.detail = detail + super().__init__(f"{reason}: {detail}") + + +class AlreadyAdjudicatedError(ValueError): + """Raised when ``input_document["stages"]`` already carries an + ``"adjudication"`` entry. That shape means this exact document has + already been through this stage once -- a re-run -- and ADJUDICATION.md + § The ``stages`` entry requires refusing it outright rather than + silently overwriting the earlier result. + """ + + +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 + ``BUZZ-DIMENSION-COMPLETE:{dimension}:{nonce}`` -- the exact format + ``findings.py``'s own ``_validate_report`` parses, matched here rather + than re-invented, since #117 is this format's one producer. + """ + marker = report.get("completion_marker") + if not isinstance(marker, str): + return None + parts = marker.split(":", 2) + if len(parts) != 3 or parts[0] != "BUZZ-DIMENSION-COMPLETE": + return None + return parts[2] + + +def _verify_nonce(document: dict) -> str: + """Verify the document's top-level ``nonce`` against every report's own + completion marker and return it. Raises ``NonceVerificationError`` -- + naming exactly one of the three refusals, in the fixed order + ADJUDICATION.md states -- when it cannot be established. Never invents a + nonce and never accepts one from anywhere but ``document`` itself. + + Reads ``document`` directly rather than trusting ``findings.validate`` + already ran (see the module docstring's STEP 4 section): a stage + agnostic about its producer verifies this itself. + """ + top_nonce = document.get("nonce") + reports_raw = document.get("reports") + reports = reports_raw if isinstance(reports_raw, list) else [] + report_nonces = [ + _report_marker_nonce(report) if isinstance(report, dict) else None for report in reports + ] + + # Refusal 1: ABSENT PROVENANCE. No top-level nonce, or at least one + # report's marker does not parse -- "must equal the nonce embedded in + # EVERY report's completion marker" cannot be checked for a report whose + # marker cannot even be read, so one unparseable report is enough to + # withhold provenance for the whole document, not just that report. + # Checked first: refusals 2 and 3 both need a value to compare against. + if not top_nonce: + raise NonceVerificationError("absent provenance", "no top-level `nonce` is present") + if not report_nonces or any(n is None for n in report_nonces): + raise NonceVerificationError( + "absent provenance", + "at least one report carries no parseable completion marker to compare against", + ) + + # Refusal 2: MIXED DOCUMENT. The reports disagree with each other. Wins + # over refusal 3 even when every report also disagrees with the + # top-level key: a mixed document is the larger fact, and the header + # mismatch that also follows from it is that fact's consequence, not a + # second, independent finding. + distinct_report_nonces = set(report_nonces) + if len(distinct_report_nonces) > 1: + raise NonceVerificationError( + "mixed document", + "reports carry different nonces in their completion markers: " + f"{sorted(distinct_report_nonces)}", + ) + + # Refusal 3: MISMATCHED ENVELOPE. The reports agree with each other but + # not with the top-level key -- one run's reports under another run's + # header. + (agreed_nonce,) = distinct_report_nonces + if agreed_nonce != top_nonce: + raise NonceVerificationError( + "mismatched envelope", + f"every report's completion marker carries nonce {agreed_nonce!r}, which does " + f"not match the top-level nonce {top_nonce!r}", + ) + + return top_nonce + + +def _check_not_already_adjudicated(document: dict) -> None: + """Raise ``AlreadyAdjudicatedError`` when ``document["stages"]`` already + carries an entry named ``"adjudication"``. Run before ``_verify_nonce`` + in ``adjudicate()`` -- a re-run is a structural defect in the request + itself, independent of whether this particular re-run's nonce happens to + check out. + """ + stages = document.get("stages") + if not isinstance(stages, list): + return + for entry in stages: + if isinstance(entry, dict) and entry.get("name") == "adjudication": + raise AlreadyAdjudicatedError( + "input document's `stages` array already carries an `adjudication` entry -- " + "refusing to re-run adjudication over an already-adjudicated document" + ) + + def _location_description(finding: dict) -> str: """Describe where a finding is anchored, branching on ``anchor`` FIRST -- never assuming ``file``/``line`` exist. Anchor ``"pr"`` is a normal, valid @@ -207,6 +371,14 @@ def adjudicate(input_document: dict, judge: Judge) -> dict: a finding whose ``severity`` already arrived illegal is refused here, wholesale, rather than reaching a per-finding fallback with no good answer. + Raises ``AlreadyAdjudicatedError`` when ``input_document["stages"]`` + already carries an ``adjudication`` entry (a re-run), and + ``NonceVerificationError`` when the top-level ``nonce`` cannot be + established against every report's completion marker (STEP 4; see the + module docstring). Both checks run after #117's own ``findings.validate`` + and before any finding is adjudicated -- input validation stays the first + gate, unchanged from STEP 3. + 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 @@ -219,8 +391,10 @@ def adjudicate(input_document: dict, judge: Judge) -> dict: if violations: raise InputValidationError(violations) + _check_not_already_adjudicated(input_document) + nonce = _verify_nonce(input_document) + output_document = copy.deepcopy(input_document) - nonce = output_document.get("nonce") verdict_counts = {"CONFIRMED": 0, "REFUTED": 0, "UNPROVEN": 0} findings_in = 0 @@ -259,6 +433,49 @@ def adjudicate(input_document: dict, judge: Judge) -> dict: completion_marker=f"BUZZ-ADJUDICATION-COMPLETE:{nonce}", ).as_dict() + # The `stages` manifest (STEP 4). Every entry already on input, in order, + # plus exactly one new `adjudication` entry -- `_check_not_already_ + # adjudicated` above already guarantees none of the input entries is + # itself named `adjudication`. + 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. + 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_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. + stage_status = "incomplete" + stage_reason = "not every finding received a verdict" + + output_document["stages"] = [ + *input_stages, + {"name": "adjudication", "status": stage_status, "reason": stage_reason}, + ] + return output_document @@ -326,6 +543,12 @@ def main(argv: list[str] | None = None) -> int: for violation in exc.violations: print(f"run_adjudication: {violation}", file=sys.stderr) return 1 + except AlreadyAdjudicatedError as exc: + print(f"run_adjudication: {exc}", file=sys.stderr) + return 1 + except NonceVerificationError as exc: + print(f"run_adjudication: {exc.reason}: {exc.detail}", file=sys.stderr) + return 1 print(json.dumps(output_document)) return 0 diff --git a/launchpad/review-agent/test_run_adjudication.py b/launchpad/review-agent/test_run_adjudication.py index 4d331dc2287..9ec5854fbf6 100644 --- a/launchpad/review-agent/test_run_adjudication.py +++ b/launchpad/review-agent/test_run_adjudication.py @@ -1,5 +1,5 @@ #!/usr/bin/env python3 -"""Controls for run_adjudication.py -- issue #118 STEP 3's CLI. +"""Controls for run_adjudication.py -- issue #118 STEP 3 and STEP 4's CLI. Exercises every behaviour named in STEP 3's own done-when list in launchpad/plans/2026-08-13-issue-118-adjudication.md: byte-identical @@ -10,13 +10,25 @@ adjudicated (the injected judge's own call count proves the refusal happens first), and no ``gh`` subprocess or HTTP client invoked during a stub run. +Also exercises STEP 4's own done-when: the nonce check (three refusals -- +``"absent provenance"``, ``"mixed document"``, ``"mismatched envelope"`` -- +in that fixed order) and the ``stages`` manifest. The three refusals are +proven two ways: directly against ``_verify_nonce`` with hand-built +documents (``NonceVerificationDirectTests`` below), which is the only way to +observe their distinct reasons at all, and end to end through ``main`` with +realistic fixtures (``NonceVerificationEndToEndTests``), where every one of +them is ALSO already caught by #117's own ``findings.validate`` -- which +``adjudicate`` runs first -- with its own, less specific, message. Both are +tested because both are true: the dedicated check is real defence in depth, +per ADJUDICATION.md's own reasoning, and it does not change what ``main`` +reports for any fixture that also happens to fail #117's own contract, which +is every reachable fixture today. + Deliberately NOT exercised here (later steps' territory, per the plan): -the nonce three-way disagreement diagnosis and the ``stages`` manifest -(STEP 4), 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. +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. 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 @@ -411,5 +423,233 @@ def test_real_process_illegal_severity_exits_nonzero_no_stdout(self): self.assertEqual(proc.stdout, "") +class NonceVerificationDirectTests(unittest.TestCase): + """Direct tests of ``_verify_nonce``, bypassing ``findings.validate`` + entirely. This is the only way to observe the three refusals' distinct + reasons and their fixed precedence: any document exhibiting one of them + also already fails #117's own ``findings.validate`` -- which + ``adjudicate`` runs first -- so ``main``/``adjudicate`` end to end never + reaches ``_verify_nonce`` with a genuinely disagreeing document today + (see ``NonceVerificationEndToEndTests`` below for that half). + """ + + def test_no_top_level_nonce_is_absent_provenance(self): + doc = make_document(reports=[make_report(nonce=NONCE)], nonce=NONCE) + del doc["nonce"] + with self.assertRaises(run_adjudication.NonceVerificationError) as ctx: + run_adjudication._verify_nonce(doc) + self.assertEqual(ctx.exception.reason, "absent provenance") + + def test_report_with_no_parseable_marker_is_absent_provenance(self): + report = make_report(nonce=NONCE) + del report["completion_marker"] + doc = make_document(reports=[report], nonce=NONCE) + with self.assertRaises(run_adjudication.NonceVerificationError) as ctx: + run_adjudication._verify_nonce(doc) + self.assertEqual(ctx.exception.reason, "absent provenance") + + def test_one_unparseable_marker_among_otherwise_agreeing_reports_is_absent_provenance(self): + # "must equal EVERY report's completion marker" cannot be checked for + # a report whose marker cannot be read -- one bad report withholds + # provenance for the whole document, it does not just drop out of + # the comparison. + good = make_report(dimension="a", nonce=NONCE) + unreadable = make_report(dimension="b", nonce=NONCE) + del unreadable["completion_marker"] + doc = make_document(reports=[good, unreadable], nonce=NONCE) + with self.assertRaises(run_adjudication.NonceVerificationError) as ctx: + run_adjudication._verify_nonce(doc) + self.assertEqual(ctx.exception.reason, "absent provenance") + + def test_reports_disagreeing_with_each_other_is_mixed_document(self): + doc = make_document( + reports=[make_report(dimension="a", nonce="N1"), make_report(dimension="b", nonce="N2")], + nonce=NONCE, + ) + with self.assertRaises(run_adjudication.NonceVerificationError) as ctx: + run_adjudication._verify_nonce(doc) + self.assertEqual(ctx.exception.reason, "mixed document") + + def test_reports_agreeing_but_not_with_top_level_is_mismatched_envelope(self): + doc = make_document( + reports=[make_report(dimension="a", nonce="N1"), make_report(dimension="b", nonce="N1")], + nonce=NONCE, # top-level differs from both reports' shared "N1" + ) + with self.assertRaises(run_adjudication.NonceVerificationError) as ctx: + run_adjudication._verify_nonce(doc) + self.assertEqual(ctx.exception.reason, "mismatched envelope") + + def test_mixed_document_wins_over_mismatched_envelope_when_both_apply(self): + # Every report disagrees with the top-level key AND with each other: + # satisfies both "mixed document" and "mismatched envelope" at once. + # ADJUDICATION.md states the mixed document wins. + doc = make_document( + reports=[make_report(dimension="a", nonce="N1"), make_report(dimension="b", nonce="N2")], + nonce="N3", + ) + with self.assertRaises(run_adjudication.NonceVerificationError) as ctx: + run_adjudication._verify_nonce(doc) + self.assertEqual(ctx.exception.reason, "mixed document") + + def test_matching_nonce_is_returned_unchanged_and_never_invented(self): + doc = make_document(reports=[make_report(nonce=NONCE)], nonce=NONCE) + self.assertEqual(run_adjudication._verify_nonce(doc), NONCE) + + +class NonceVerificationEndToEndTests(unittest.TestCase): + """The same three refusals, but through `main` with realistic fixtures. + Every one of these ALSO already fails #117's own `findings.validate` + (run first, per STEP 3), so what is actually asserted here is `main`'s + contract -- exits non-zero, prints no document at all -- not that the + stderr text is `_verify_nonce`'s own. See `NonceVerificationDirectTests` + above for the distinct-reason proof. + """ + + def _run_main_with_document(self, document: dict) -> tuple[int, str, str]: + stdout, stderr = io.StringIO(), io.StringIO() + with mock.patch.object(sys, "stdin", io.StringIO(json.dumps(document))), \ + contextlib.redirect_stdout(stdout), contextlib.redirect_stderr(stderr): + exit_code = run_adjudication.main([]) + return exit_code, stdout.getvalue(), stderr.getvalue() + + def test_two_reports_with_different_nonces_exits_nonzero_no_document(self): + doc = make_document( + reports=[make_report(dimension="a", nonce="N1"), make_report(dimension="b", nonce="N2")], + nonce=NONCE, + ) + exit_code, stdout, stderr = self._run_main_with_document(doc) + self.assertNotEqual(exit_code, 0) + self.assertEqual(stdout, "") + self.assertTrue(stderr) + + def test_reports_agreeing_but_not_top_level_exits_nonzero_no_document(self): + doc = make_document( + reports=[make_report(dimension="a", nonce="N1"), make_report(dimension="b", nonce="N1")], + nonce=NONCE, + ) + exit_code, stdout, stderr = self._run_main_with_document(doc) + self.assertNotEqual(exit_code, 0) + self.assertEqual(stdout, "") + self.assertTrue(stderr) + + def test_no_top_level_nonce_exits_nonzero_and_invents_nothing(self): + doc = make_document(reports=[make_report(nonce=NONCE)], nonce=NONCE) + del doc["nonce"] + exit_code, stdout, stderr = self._run_main_with_document(doc) + self.assertNotEqual(exit_code, 0) + self.assertEqual(stdout, "") # nothing printed means no nonce was invented + + def test_report_with_no_marker_exits_nonzero_and_never_reports_complete(self): + report = make_report(nonce=NONCE) + del report["completion_marker"] + doc = make_document(reports=[report], nonce=NONCE) + exit_code, stdout, stderr = self._run_main_with_document(doc) + self.assertNotEqual(exit_code, 0) + # No document at all is printed, so no stage status is ever emitted -- + # "complete" in particular is never among them. + self.assertEqual(stdout, "") + + +class StagesManifestTests(unittest.TestCase): + """The top-level `stages` array STEP 4 adds: every entry already on + input, in order, plus exactly one new `adjudication` entry. + """ + + def test_happy_path_output_nonce_unchanged_and_marker_is_last_key(self): + reports = [make_report(dimension=d, nonce=NONCE) for d in ("a", "b", "c")] + input_doc = make_document(reports=reports, nonce=NONCE) + + output_doc = run_adjudication.adjudicate(input_doc, run_adjudication.stub_judge) + + self.assertEqual(output_doc["nonce"], NONCE) + adjudication = output_doc["adjudication"] + keys = list(adjudication.keys()) + self.assertEqual(keys[-1], "completion_marker") + self.assertEqual(adjudication["completion_marker"], f"BUZZ-ADJUDICATION-COMPLETE:{NONCE}") + + def test_output_stages_carries_input_entries_plus_one_new_adjudication_entry(self): + input_doc = make_document() + input_doc["stages"] = [{"name": "preflight", "status": "complete", "reason": None}] + + output_doc = run_adjudication.adjudicate(input_doc, run_adjudication.stub_judge) + + self.assertEqual( + output_doc["stages"], + [ + {"name": "preflight", "status": "complete", "reason": None}, + {"name": "adjudication", "status": "complete", "reason": None}, + ], + ) + + def test_output_stages_is_just_the_new_entry_when_input_has_none(self): + input_doc = make_document() + self.assertNotIn("stages", input_doc) + + output_doc = run_adjudication.adjudicate(input_doc, run_adjudication.stub_judge) + + self.assertEqual( + output_doc["stages"], + [{"name": "adjudication", "status": "complete", "reason": None}], + ) + + def test_input_stages_list_is_not_mutated(self): + input_doc = make_document() + input_stages = [{"name": "preflight", "status": "complete", "reason": None}] + input_doc["stages"] = input_stages + + run_adjudication.adjudicate(input_doc, run_adjudication.stub_judge) + + self.assertEqual(input_stages, [{"name": "preflight", "status": "complete", "reason": None}]) + + +class AlreadyAdjudicatedTests(unittest.TestCase): + """An input already carrying an `adjudication` entry in `stages` is a + re-run against an already-adjudicated document -- refused outright, + never silently overwritten. + """ + + def test_adjudicate_raises_and_never_calls_the_judge(self): + input_doc = make_document() + input_doc["stages"] = [{"name": "adjudication", "status": "complete", "reason": None}] + judge = CountingJudge() + + with self.assertRaises(run_adjudication.AlreadyAdjudicatedError): + run_adjudication.adjudicate(input_doc, judge) + + self.assertEqual(judge.call_count, 0, judge.calls) + + def test_main_exits_nonzero_and_prints_no_document(self): + input_doc = make_document() + input_doc["stages"] = [{"name": "adjudication", "status": "complete", "reason": None}] + stdout, stderr = io.StringIO(), io.StringIO() + with mock.patch.object(sys, "stdin", io.StringIO(json.dumps(input_doc))), \ + contextlib.redirect_stdout(stdout), contextlib.redirect_stderr(stderr): + exit_code = run_adjudication.main([]) + self.assertNotEqual(exit_code, 0) + self.assertEqual(stdout.getvalue(), "") + self.assertTrue(stderr.getvalue()) + + +class PublishIncompleteRuleTests(unittest.TestCase): + """#119's own rule -- "any status other than 'complete' is incomplete + and banners the whole review" -- run here as an assertion against the + output, since #119's own code does not exist to run against (STEP 4's + own done-when names this explicitly). + """ + + def test_happy_path_stage_status_is_complete_so_119_would_not_banner_it(self): + input_doc = make_document() + + output_doc = run_adjudication.adjudicate(input_doc, run_adjudication.stub_judge) + + adjudication_stage = next( + entry for entry in output_doc["stages"] if entry["name"] == "adjudication" + ) + # #119's stated rule, applied directly: only "complete" reads as + # complete: anything else -- any other string -- banners the review. + self.assertEqual(adjudication_stage["status"], "complete") + self.assertIsNone(adjudication_stage["reason"]) + + if __name__ == "__main__": unittest.main() From 36348cc0dc278b4cc2c79b76f9199bbbb27972c8 Mon Sep 17 00:00:00 2001 From: Serina Mcfall Date: Fri, 21 Aug 2026 10:39:51 +1200 Subject: [PATCH 2/3] fix(launchpad): run nonce verification before findings.validate (#118 STEP 4) adjudicate() ran #117's findings.validate first, which independently rejects a document whose report markers disagree with the top-level nonce -- but with one generic per-report message that does not distinguish "reports disagree with each other" (mixed document) from "reports agree with each other but not the top-level key" (mismatched envelope). Since every nonce problem findings.validate can see is a problem _verify_nonce can also see, findings.validate always won the race, so the three distinct refusals STEP 4's plan requires to be observable end-to-end were provably unreachable through main() -- only testable by calling _verify_nonce directly. Fixed by running _check_not_already_adjudicated and _verify_nonce before findings.validate. findings.validate still runs before any finding reaches the judge loop -- STEP 3's actual guarantee -- just second now instead of first. Strengthened NonceVerificationEndToEndTests to assert the specific reason text in main()'s stderr (mixed document / mismatched envelope / absent provenance), not just exit code and empty stdout, so a regression that reverts the ordering fails a test again. Signed-off-by: Serina Mcfall --- launchpad/review-agent/run_adjudication.py | 53 ++++++++++--------- .../review-agent/test_run_adjudication.py | 38 +++++++------ 2 files changed, 51 insertions(+), 40 deletions(-) diff --git a/launchpad/review-agent/run_adjudication.py b/launchpad/review-agent/run_adjudication.py index ef3266dd59d..5d952db5051 100644 --- a/launchpad/review-agent/run_adjudication.py +++ b/launchpad/review-agent/run_adjudication.py @@ -65,18 +65,20 @@ are two facets of one CLI: The top-level ``nonce`` is checked and passed through, **never generated**. -#117's own ``findings.validate`` -- which ``adjudicate()`` already runs first --- rejects a document whose marker disagrees with the top-level key, so in -practice a document that clears that gate today already agrees everywhere. -This module checks it again anyway, for ADJUDICATION.md's own reason: this -stage is agnostic about its producer and may not inherit a guarantee it did -not watch being made. ``_verify_nonce`` below is that check, kept as its own -function precisely so it is testable directly against hand-built documents -- -the same reason ``verdicts.validate`` is tested against hand-built documents -rather than only against this module's own output -- since #117's validator -being this thorough today means the three refusals below are not otherwise -reachable through ``main()`` with a document that still satisfies #117's own -contract. +``_verify_nonce`` runs BEFORE #117's own ``findings.validate`` -- deliberately +reordered from STEP 3's original sequence, and this is why: ``findings. +validate`` independently rejects a document whose marker disagrees with the +top-level key too, but it does so with one generic per-report message, +identical whether the reports disagree with EACH OTHER or merely with the +top-level key. Running it first would mean ``_verify_nonce``'s three distinct +refusals below could never actually surface through ``main()`` -- every +document that would trigger one of them already fails ``findings.validate`` +first, so the operator would only ever see the generic message and never the +category. Checking the nonce first makes the three refusals genuinely +observable end to end, which is what ADJUDICATION.md's "own reason, distinct +from the first" requirement means in practice. ``findings.validate`` still +runs -- second now, but still before a single finding reaches the judge loop, +which is STEP 3's actual guarantee, not "first" in an absolute sense. Three refusals, checked in this fixed order because one document can satisfy more than one at once: @@ -186,8 +188,9 @@ def _verify_nonce(document: dict) -> str: nonce and never accepts one from anywhere but ``document`` itself. Reads ``document`` directly rather than trusting ``findings.validate`` - already ran (see the module docstring's STEP 4 section): a stage - agnostic about its producer verifies this itself. + to have run first -- it runs BEFORE ``findings.validate`` in + ``adjudicate()`` (see the module docstring's STEP 4 section for why): a + stage agnostic about its producer verifies this itself. """ top_nonce = document.get("nonce") reports_raw = document.get("reports") @@ -365,19 +368,19 @@ 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``. + Raises ``AlreadyAdjudicatedError`` when ``input_document["stages"]`` + already carries an ``adjudication`` entry (a re-run), and + ``NonceVerificationError`` when the top-level ``nonce`` cannot be + established against every report's completion marker (STEP 4; see the + module docstring for why these run BEFORE ``findings.validate`` now). + Raises ``InputValidationError`` -- adjudicating nothing, calling ``judge`` zero times -- when ``input_document`` fails #117's own ``findings.validate``. This is the boundary STEP 1/STEP 2 call load-bearing: a finding whose ``severity`` already arrived illegal is refused here, wholesale, rather than reaching a per-finding fallback with no good answer. - - Raises ``AlreadyAdjudicatedError`` when ``input_document["stages"]`` - already carries an ``adjudication`` entry (a re-run), and - ``NonceVerificationError`` when the top-level ``nonce`` cannot be - established against every report's completion marker (STEP 4; see the - module docstring). Both checks run after #117's own ``findings.validate`` - and before any finding is adjudicated -- input validation stays the first - gate, unchanged from STEP 3. + Checked after the two STEP 4 gates above, but still before any finding + reaches the judge loop -- STEP 3's actual guarantee. Pass-through fields (``pr``, ``merge_base_sha``, ``head_sha``, ``containment``) are never touched: the output starts as a @@ -387,13 +390,13 @@ def adjudicate(input_document: dict, judge: Judge) -> dict: here is left exactly equal to its ``reported_severity``, and ``duplicate_of`` is always null. """ + _check_not_already_adjudicated(input_document) + nonce = _verify_nonce(input_document) + violations = findings.validate(input_document) if violations: raise InputValidationError(violations) - _check_not_already_adjudicated(input_document) - nonce = _verify_nonce(input_document) - output_document = copy.deepcopy(input_document) verdict_counts = {"CONFIRMED": 0, "REFUTED": 0, "UNPROVEN": 0} diff --git a/launchpad/review-agent/test_run_adjudication.py b/launchpad/review-agent/test_run_adjudication.py index 9ec5854fbf6..da2c74e6558 100644 --- a/launchpad/review-agent/test_run_adjudication.py +++ b/launchpad/review-agent/test_run_adjudication.py @@ -424,13 +424,12 @@ def test_real_process_illegal_severity_exits_nonzero_no_stdout(self): class NonceVerificationDirectTests(unittest.TestCase): - """Direct tests of ``_verify_nonce``, bypassing ``findings.validate`` - entirely. This is the only way to observe the three refusals' distinct - reasons and their fixed precedence: any document exhibiting one of them - also already fails #117's own ``findings.validate`` -- which - ``adjudicate`` runs first -- so ``main``/``adjudicate`` end to end never - reaches ``_verify_nonce`` with a genuinely disagreeing document today - (see ``NonceVerificationEndToEndTests`` below for that half). + """Direct, unit-level tests of ``_verify_nonce`` in isolation -- the + three refusals' distinct reasons and their fixed precedence, without the + rest of ``adjudicate`` around them. ``NonceVerificationEndToEndTests`` + below proves the same three reasons surface through the real CLI, now + that ``_verify_nonce`` runs before ``findings.validate`` in + ``adjudicate`` (see the module docstring's STEP 4 section). """ def test_no_top_level_nonce_is_absent_provenance(self): @@ -497,12 +496,18 @@ def test_matching_nonce_is_returned_unchanged_and_never_invented(self): class NonceVerificationEndToEndTests(unittest.TestCase): - """The same three refusals, but through `main` with realistic fixtures. - Every one of these ALSO already fails #117's own `findings.validate` - (run first, per STEP 3), so what is actually asserted here is `main`'s - contract -- exits non-zero, prints no document at all -- not that the - stderr text is `_verify_nonce`'s own. See `NonceVerificationDirectTests` - above for the distinct-reason proof. + """The same three refusals, through `main` with realistic fixtures -- + proving the DISTINCT reason each one names is actually observable end to + end, not just from calling `_verify_nonce` directly. + + This is only true because `adjudicate()` runs `_verify_nonce` BEFORE + #117's own `findings.validate`. `findings.validate` independently rejects + the same documents, but with one generic per-report message that does not + distinguish "reports disagree with each other" from "reports agree with + each other but not the top-level key" -- see the module docstring's STEP 4 + section. Checking the ordering here, not just the exit code, is the whole + point of this class: a regression that reverts the ordering would still + pass a test that only asserts `stdout == ""`. """ def _run_main_with_document(self, document: dict) -> tuple[int, str, str]: @@ -520,7 +525,7 @@ def test_two_reports_with_different_nonces_exits_nonzero_no_document(self): exit_code, stdout, stderr = self._run_main_with_document(doc) self.assertNotEqual(exit_code, 0) self.assertEqual(stdout, "") - self.assertTrue(stderr) + self.assertIn("mixed document", stderr, stderr) def test_reports_agreeing_but_not_top_level_exits_nonzero_no_document(self): doc = make_document( @@ -530,7 +535,8 @@ def test_reports_agreeing_but_not_top_level_exits_nonzero_no_document(self): exit_code, stdout, stderr = self._run_main_with_document(doc) self.assertNotEqual(exit_code, 0) self.assertEqual(stdout, "") - self.assertTrue(stderr) + self.assertIn("mismatched envelope", stderr, stderr) + self.assertNotIn("mixed document", stderr, stderr) def test_no_top_level_nonce_exits_nonzero_and_invents_nothing(self): doc = make_document(reports=[make_report(nonce=NONCE)], nonce=NONCE) @@ -538,6 +544,7 @@ def test_no_top_level_nonce_exits_nonzero_and_invents_nothing(self): exit_code, stdout, stderr = self._run_main_with_document(doc) self.assertNotEqual(exit_code, 0) self.assertEqual(stdout, "") # nothing printed means no nonce was invented + self.assertIn("absent provenance", stderr, stderr) def test_report_with_no_marker_exits_nonzero_and_never_reports_complete(self): report = make_report(nonce=NONCE) @@ -548,6 +555,7 @@ def test_report_with_no_marker_exits_nonzero_and_never_reports_complete(self): # No document at all is printed, so no stage status is ever emitted -- # "complete" in particular is never among them. self.assertEqual(stdout, "") + self.assertIn("absent provenance", stderr, stderr) class StagesManifestTests(unittest.TestCase): From 2be90c62986ffc3d44f8e6fa102918cca4d12588 Mon Sep 17 00:00:00 2001 From: Serina Mcfall Date: Fri, 21 Aug 2026 10:50:28 +1200 Subject: [PATCH 3/3] fix(launchpad): don't let a malformed reports shape read as absent provenance (#118 STEP 4) review-code found that checking _verify_nonce before findings.validate (the previous fix in this branch) has a side effect: a document whose reports key is missing, non-list, or empty has nothing for nonce verification to compare against, so _verify_nonce calls it "absent provenance" -- technically true, but it buries findings.validate's more specific message for exactly that shape defect ("missing required key 'reports'", "must not be empty", "expected an array"), pointing a maintainer at the wrong subsystem. Fixed by checking reports is a present, non-empty list before running _verify_nonce at all; when it isn't, defer straight to findings.validate, which already names the real problem. The _verify_nonce call in that branch is unreachable in practice (a malformed reports always fails findings.validate on one of those three grounds) but kept as a real call rather than asserted away, matching this module's existing "real branch, not assumed" discipline for stage_complete's nonce_established condition. Signed-off-by: Serina Mcfall --- launchpad/review-agent/run_adjudication.py | 38 +++++++++++++++-- .../review-agent/test_run_adjudication.py | 41 +++++++++++++++++++ 2 files changed, 75 insertions(+), 4 deletions(-) diff --git a/launchpad/review-agent/run_adjudication.py b/launchpad/review-agent/run_adjudication.py index 5d952db5051..dcff95b1844 100644 --- a/launchpad/review-agent/run_adjudication.py +++ b/launchpad/review-agent/run_adjudication.py @@ -80,6 +80,13 @@ runs -- second now, but still before a single finding reaches the judge loop, which is STEP 3's actual guarantee, not "first" in an absolute sense. +One exception: a document whose ``reports`` key is missing, non-list, or +empty defers straight to ``findings.validate`` instead of ``_verify_nonce`` +-- there is nothing for nonce verification to compare against, and calling +that "absent provenance" would bury ``findings.validate``'s more specific, +more useful message for exactly that shape defect. See ``adjudicate``'s own +body for the precise condition. + Three refusals, checked in this fixed order because one document can satisfy more than one at once: 1. ``"absent provenance"`` -- no top-level ``nonce``, or no report carries a @@ -391,11 +398,34 @@ def adjudicate(input_document: dict, judge: Judge) -> dict: ``duplicate_of`` is always null. """ _check_not_already_adjudicated(input_document) - nonce = _verify_nonce(input_document) - violations = findings.validate(input_document) - if violations: - raise InputValidationError(violations) + # `_verify_nonce`'s job is provenance, not `reports`'s basic shape. A + # document whose `reports` key is missing, non-list, or empty has nothing + # for nonce verification to compare against -- `_verify_nonce` would call + # that "absent provenance", which is technically true but masks the more + # specific, more useful message findings.validate already gives for + # exactly this ("missing required key 'reports'", "must not be empty", + # "expected an array"). So a document this malformed defers straight to + # findings.validate instead of being told the wrong subsystem is broken. + reports_raw = input_document.get("reports") + reports_present_and_nonempty = isinstance(reports_raw, list) and len(reports_raw) > 0 + + if reports_present_and_nonempty: + nonce = _verify_nonce(input_document) + violations = findings.validate(input_document) + if violations: + raise InputValidationError(violations) + else: + violations = findings.validate(input_document) + if violations: + raise InputValidationError(violations) + # Unreachable in practice: a missing, non-list, or empty `reports` + # always fails findings.validate above, on one of the three grounds + # named in this branch's comment. Kept as a real call, not asserted + # away, the same "real branch" discipline `stage_complete`'s + # nonce_established condition already uses below for STEP 6's + # not-yet-built flag. + nonce = _verify_nonce(input_document) output_document = copy.deepcopy(input_document) diff --git a/launchpad/review-agent/test_run_adjudication.py b/launchpad/review-agent/test_run_adjudication.py index da2c74e6558..35006892ae9 100644 --- a/launchpad/review-agent/test_run_adjudication.py +++ b/launchpad/review-agent/test_run_adjudication.py @@ -558,6 +558,47 @@ def test_report_with_no_marker_exits_nonzero_and_never_reports_complete(self): self.assertIn("absent provenance", stderr, stderr) +class MalformedReportsDefersToFindingsValidateTests(unittest.TestCase): + """A `reports`-shape defect (missing, non-list, empty) is not a nonce + problem -- `_verify_nonce` would call it "absent provenance", which + buries `findings.validate`'s more specific, more useful message. These + three shapes must all defer to that message instead. + """ + + def _run_main_with_document(self, document: dict) -> tuple[int, str, str]: + stdout, stderr = io.StringIO(), io.StringIO() + with mock.patch.object(sys, "stdin", io.StringIO(json.dumps(document))), \ + contextlib.redirect_stdout(stdout), contextlib.redirect_stderr(stderr): + exit_code = run_adjudication.main([]) + return exit_code, stdout.getvalue(), stderr.getvalue() + + def test_missing_reports_key_names_the_missing_key_not_provenance(self): + doc = make_document(reports=[make_report(nonce=NONCE)], nonce=NONCE) + del doc["reports"] + exit_code, stdout, stderr = self._run_main_with_document(doc) + self.assertNotEqual(exit_code, 0) + self.assertEqual(stdout, "") + self.assertIn("missing required key 'reports'", stderr, stderr) + self.assertNotIn("absent provenance", stderr, stderr) + + def test_empty_reports_array_names_the_empty_array_not_provenance(self): + doc = make_document(reports=[], nonce=NONCE) + exit_code, stdout, stderr = self._run_main_with_document(doc) + self.assertNotEqual(exit_code, 0) + self.assertEqual(stdout, "") + self.assertIn("must not be empty", stderr, stderr) + self.assertNotIn("absent provenance", stderr, stderr) + + def test_non_list_reports_names_the_wrong_type_not_provenance(self): + doc = make_document(reports=[make_report(nonce=NONCE)], nonce=NONCE) + doc["reports"] = "not-a-list" + exit_code, stdout, stderr = self._run_main_with_document(doc) + self.assertNotEqual(exit_code, 0) + self.assertEqual(stdout, "") + self.assertIn("expected an array", stderr, stderr) + self.assertNotIn("absent provenance", stderr, stderr) + + class StagesManifestTests(unittest.TestCase): """The top-level `stages` array STEP 4 adds: every entry already on input, in order, plus exactly one new `adjudication` entry.