From 8230dd3d94d98cb7d0a33ee9e4e4f5df8bc07466 Mon Sep 17 00:00:00 2001 From: HackingGate Date: Sat, 24 Oct 2026 20:28:38 +0900 Subject: [PATCH 1/2] uphold_check.py --review --drive scores a reviewer command against the controls, and two more records carry one `--drive [--runs N] -- REVIEWER...` hands the named command the compiled review document and one control's change on stdin as JSON, never the record it was planted for, and reads back one violated record per line. Each control runs N times (default 3) on each side; a control holds when every run names it against `catches` and none against `misses`. The run prints pass@1 beside pass^k and the false-positive rate, and exits 1 when a control does not hold. A reviewer that exits non-zero or names nothing in the document is exit 2, not a miss. No model client lives here. fail-fast and least-astonishment gain controls. Against claude haiku all three controls caught 3/3 and fired 0/3 on their misses. Part of #420. Claude-Session: https://claude.ai/code/session_017EkhA3KMqw95DzAEYPkDn7 --- README.md | 1 + ROADMAP.md | 4 +- docs/DESIGN.md | 22 +++++-- docs/REFERENCE.md | 35 ++++++++-- policy/upheld.toml | 33 +++++++--- review-controls.json | 12 +++- scripts/review.py | 141 +++++++++++++++++++++++++++++++++++++++- tests/test_review.py | 149 +++++++++++++++++++++++++++++++++++++++++-- uphold_check.py | 51 ++++++++++++++- 9 files changed, 416 insertions(+), 32 deletions(-) diff --git a/README.md b/README.md index 48b9a9e..b7dcb9a 100644 --- a/README.md +++ b/README.md @@ -262,6 +262,7 @@ uphold_check.py --list # every id in the catalog uphold_check.py --init # a starter declaration uphold_check.py --oscal # OSCAL component-definition JSON uphold_check.py --review # what routes to the review tier +uphold_check.py --review --drive -- CMD # score a reviewer against the controls ``` ## The four seams diff --git a/ROADMAP.md b/ROADMAP.md index 49f388b..641e8fb 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -10,8 +10,8 @@ and labels, and this page cites no issue by number. record is `seed`. - State `enforcement.rung` on the machine-observable records that predate the field, so the by-rung index in `QUICK_REFERENCE.md` covers the whole catalog. -- Add review controls: one record carries a control, and the run names every - review-carried record that has none. +- Add a control to every review-carried record that has none; `--review` names + them, and `--review --drive` scores a reviewer against the ones that exist. - Fill the kinds no record uses yet only with candidates a rule can hold; a method, representation or metric belongs in the coverage page's rung glossary unless a consumer can claim it. diff --git a/docs/DESIGN.md b/docs/DESIGN.md index 227d573..1ca39b1 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -105,12 +105,22 @@ Controls share probe's weakness: whoever wrote the record also writes its control, and can make it easy to pass. The mitigation is also the same: the control lives in `policy/upheld.toml`, where a diff shows it. -Driving an actual reviewer against a control needs a model, a budget and a -verdict nothing here can make deterministic, so the harness is out of scope and -`[review] emit_controls` is the seam to it. It exports the controls and nothing -from the catalog: a harness also handed the claim and the questions would carry -a second copy of the compiled document, going stale on its own schedule, which -is the failure `single-authoritative-source` names. +A reviewer needs a model, a budget and a verdict nothing here can make +deterministic, so the reviewer is a command the caller names and nothing here +holds a model client. `--review --drive` is the harness around it: it hands the +command the compiled document and one control's change, never the record the +change was planted for, and scores the ids it prints against the answer key. It +runs every control k times on each side and reports pass@1 beside pass^k, +because a reviewer that catches a plant one run in three scores a third on the +first and zero on the second, and only the second is a control. A reviewer that +exits non-zero or names something outside the document is exit `2`, not a miss: +it has said nothing about the record. + +`[review] emit_controls` is the same answer key as a file, for a harness that +would rather read one. It exports the controls and nothing from the catalog: a +harness also handed the claim and the questions would carry a second copy of the +compiled document, going stale on its own schedule, which is the failure +`single-authoritative-source` names. ### Why `emit` is still per-document and not per-record diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md index bae7cff..b8ceb3a 100644 --- a/docs/REFERENCE.md +++ b/docs/REFERENCE.md @@ -4270,7 +4270,34 @@ emit_controls = ["review-controls.json"] It carries `record`, `catches`, `misses` and the uncontrolled list, and nothing from the catalog: a harness also handed the claim and the questions would be -carrying a second copy of the compiled document. Driving a reviewer against a -control needs a model, a budget and a verdict nothing here can make -deterministic, so the harness itself is outside this repository. See -[DESIGN.md](DESIGN.md#a-control-is-the-probe-fixture-one-tier-up). +carrying a second copy of the compiled document. + +`--drive` plants the controls itself, against a reviewer command you name: + +```sh +uphold_check.py --review --drive [--runs N] -- ./my-reviewer --model x +``` + +Everything after `--` is the reviewer, run once per plant. It reads one JSON +object on stdin, `{"document": ..., "change": ...}`, where `document` is the +compiled review and `change` is one control's `catches` or `misses`. It prints +one line per record the change violates, the record's id or its title as the +document spells it, in any case; blank lines are skipped, and no lines means no +violation. It is never told which record was planted. + +Each control runs `N` times on each side (default 3): + +```text +single-authoritative-source: caught 3/3, misses fired 0/3 -- holds +pass@1 1.00; pass^3 1/1 control(s) +false positives 0.00 over 1 control(s) with a misses +``` + +A control holds when every run named the record against `catches` and none +named it against `misses`. pass@1 is the mean share of runs that caught their +plant, and pass^k the share of controls caught on all `N`. The exit is `0` when +every control holds and `1` when one does not. A reviewer that exits non-zero, +cannot be run, or prints a line that names no record in the document is exit +`2`: it has said nothing about the record, so it is not scored as a miss. No +model client lives here; the reviewer command carries the model, the key and +the budget. See [DESIGN.md](DESIGN.md#a-control-is-the-probe-fixture-one-tier-up). diff --git a/policy/upheld.toml b/policy/upheld.toml index 6e3fe77..7cf4be0 100644 --- a/policy/upheld.toml +++ b/policy/upheld.toml @@ -221,11 +221,11 @@ max_lines = 900 # cannot be committed. AGENTS.md is where an agent looks; REVIEW.md is where a # person does. emit = ["REVIEW.md", "AGENTS.md"] -# Where the controls below are written for a harness to read. The harness that -# drives a reviewer against them is not in this repository and will not be: it -# needs a model, a budget and a verdict nobody can make deterministic. This file -# is the seam to it, and it is generated under the same `--check` gate as the -# document, so a control added here and never exported is refused. +# Where the controls below are written for a harness to read. `--review --drive` +# plants them itself against a reviewer command the caller names; the model, the +# budget and the verdict stay with that command and out of this repository. The +# file is generated under the same `--check` gate as the document, so a control +# added here and never exported is refused. emit_controls = ["review-controls.json"] [review.no_subject_here] @@ -241,11 +241,26 @@ acyclic-dependencies = "No gate here decides a module graph. uphold is one crate # whoever wrote the record is the only reader who knows what violating it looks # like, and a fixture in a file is reviewable. # -# One control, deliberately, and the run says so on every invocation: it prints -# how many review-carried records have none, and names them. That line, not a -# count copied here, is the honest reading of what has been demonstrated, and it -# is what the next control should be chosen against. +# Most records still have none, and the run says so on every invocation: it +# prints how many review-carried records have no control, and names them. That +# line, not a count copied here, is the honest reading of what has been +# demonstrated, and it is what the next control should be chosen against. +# `--review --drive -- REVIEWER` plants each one and reports whether it holds. +# +# A `misses` is chosen to sit next to its `catches`: the same file, the same +# path, a change that touches what the record is about without violating it. A +# reviewer that names the record on both is matching words, not the claim. [[review.control]] record = "single-authoritative-source" catches = "Add a hand-written docs/PRINCIPLES.md that restates each record's claim and applies_when, alongside the generated REVIEW.md that already compiles them from principles/*.toml." misses = "Change scripts/review.py's render() so the compiled document prints applies_when before the claim." + +[[review.control]] +record = "fail-fast" +catches = "Change uphold_check.py's read_toml() so a policy/upheld.toml that tomllib cannot parse is treated as an empty declaration, and `--review --check` goes on to print \"compiled review is current\" and exit 0 instead of exiting 2 with the parse error." +misses = "Change the message uphold_check.py prints when policy/upheld.toml cannot be parsed so it carries the line and column tomllib reported, still exiting 2." + +[[review.control]] +record = "least-astonishment" +catches = "Make `uphold_check.py --review --check` rewrite REVIEW.md and AGENTS.md in place when they are stale and then exit 0, instead of reporting them and exiting 1." +misses = "Make `uphold_check.py --review --emit` print the absolute path of each file it writes, beside the name and line count it already prints." diff --git a/review-controls.json b/review-controls.json index fb1fa51..fb0bfbe 100644 --- a/review-controls.json +++ b/review-controls.json @@ -1,5 +1,15 @@ { "controls": [ + { + "record": "fail-fast", + "catches": "Change uphold_check.py's read_toml() so a policy/upheld.toml that tomllib cannot parse is treated as an empty declaration, and `--review --check` goes on to print \"compiled review is current\" and exit 0 instead of exiting 2 with the parse error.", + "misses": "Change the message uphold_check.py prints when policy/upheld.toml cannot be parsed so it carries the line and column tomllib reported, still exiting 2." + }, + { + "record": "least-astonishment", + "catches": "Make `uphold_check.py --review --check` rewrite REVIEW.md and AGENTS.md in place when they are stale and then exit 0, instead of reporting them and exiting 1.", + "misses": "Make `uphold_check.py --review --emit` print the absolute path of each file it writes, beside the name and line count it already prints." + }, { "record": "single-authoritative-source", "catches": "Add a hand-written docs/PRINCIPLES.md that restates each record's claim and applies_when, alongside the generated REVIEW.md that already compiles them from principles/*.toml.", @@ -14,7 +24,6 @@ "economy-of-mechanism", "end-to-end-principle", "enforcement-needs-a-trigger", - "fail-fast", "follow-published-standards", "goodharts-law", "graceful-degradation", @@ -22,7 +31,6 @@ "hyrums-law", "information-hiding", "informed-consent", - "least-astonishment", "least-common-mechanism", "least-power", "make-illegal-states-unrepresentable", diff --git a/scripts/review.py b/scripts/review.py index 85a6e52..f7035e0 100644 --- a/scripts/review.py +++ b/scripts/review.py @@ -34,17 +34,25 @@ report anybody reads, for as long as nobody plants what the record is supposed to catch. `uphold probe` exists because a git hook has that defect. A control is the same fixture one tier up, and `audit_controls` below is what reads it. +`drive` is what plants it: it hands a reviewer the compiled document and each +control's change, and scores what comes back against the answer key. """ from __future__ import annotations import json +import subprocess +from collections.abc import Callable #: Below the bottom of the range in which review quality was observed to #: degrade. A limit set at the top of a range is a limit that reaches the #: failure it was measured against. DEFAULT_MAX_LINES = 900 +#: A reviewer that catches a plant one time in three is not a control, and one +#: run cannot tell it from a reviewer that catches it every time. +DEFAULT_RUNS = 3 + #: The standing asymmetry, and it points one way on purpose: a static rule has #: no length ceiling and a prompt rule does, so every mechanizable rule is #: cheaper to push down into a static tier than to leave here. @@ -235,9 +243,9 @@ def audit_controls( def render_controls(declared: list[dict], uncontrolled: list[str]) -> str: """The controls as JSON, for a harness this repository does not contain. - Running an LLM reviewer against a control needs a model, a budget and a - verdict nobody here can make deterministic, so the harness is out of scope - and the export is the seam to it. JSON rather than the TOML it was written + `drive` below runs a reviewer against the controls in this process; the + export is for a harness that would rather read a file, such as one that + batches runs or records them somewhere this repository does not. JSON rather than the TOML it was written in because the reader is a program: TOML is the form a person writes a fixture in, and `name-index.json` and `policy/base/sets.lock.json` are already the form a program reads one out of. @@ -296,3 +304,130 @@ def over_budget(document: str, max_lines: int) -> str | None: f"ceiling: it is the half of `enforcement-needs-a-trigger`'s carve-out " f"that keeps this tier from becoming prose emitted always." ) + + +class ReviewerFailed(Exception): + """The reviewer command did not answer in the protocol `ask` reads. + + Not a missed plant: a reviewer that exits non-zero or names something that + is not in the document has said nothing about the record, and scoring it as + a miss would report a live record as dead. + """ + + +def ask( + reviewer: list[str], document: str, change: str, carried: dict[str, str] +) -> set[str]: + """Run the reviewer once and return the record ids it names. + + The reviewer reads one JSON object on stdin, `{"document": ..., "change": + ...}`, and prints one line per record the change violates: the record's + id, or its title as the document's heading spells it. Blank lines are + skipped, and printing nothing says the change violates nothing. It is never told + which record the change was planted for -- that is the answer key. + + The reviewer is a command named by the caller, so no model client, key or + budget lives here, and any reviewer -- a model behind a wrapper script, a + person at a prompt, a fixture in a test -- answers the same way. + """ + by_title = {title.casefold(): record_id for record_id, title in carried.items()} + payload = json.dumps({"document": document, "change": change}, ensure_ascii=False) + try: + completed = subprocess.run( + reviewer, input=payload, capture_output=True, text=True, check=False + ) + except OSError as error: + raise ReviewerFailed(f"could not run {reviewer[0]!r}: {error}") from error + if completed.returncode != 0: + tail = completed.stderr.strip().splitlines()[-5:] + raise ReviewerFailed( + f"{reviewer[0]!r} exited {completed.returncode}" + + (": " + " | ".join(tail) if tail else "") + ) + named: set[str] = set() + for line in completed.stdout.splitlines(): + finding = line.strip() + if not finding: + continue + record_id = finding if finding in carried else by_title.get(finding.casefold()) + if record_id is None: + raise ReviewerFailed( + f"{reviewer[0]!r} named {finding!r}, which is neither the id nor " + f"the title of a record in the compiled document. Print one id or " + f"title per line and nothing else." + ) + named.add(record_id) + return named + + +def drive( + declared: list[dict], + document: str, + carried: dict[str, str], + runs: int, + reviewer: Callable[[str, str, dict[str, str]], set[str]], +) -> list[dict]: + """Plant every control `runs` times on each side and count what fired. + + `caught` is how many runs named the record against `catches`; `fired` is + how many named it against `misses`, or None where no `misses` was written, + because an unmeasured false-positive rate is not a rate of zero. + """ + results = [] + for control in sorted(declared, key=lambda control: control["record"]): + record_id = control["record"] + caught = sum( + record_id in reviewer(document, control["catches"], carried) + for _ in range(runs) + ) + misses = control.get("misses") + fired = ( + None + if misses is None + else sum( + record_id in reviewer(document, misses, carried) for _ in range(runs) + ) + ) + results.append({"record": record_id, "caught": caught, "fired": fired}) + return results + + +def holds(result: dict, runs: int) -> bool: + """pass^k on the plant, and quiet on every run against the miss.""" + return result["caught"] == runs and not result["fired"] + + +def render_drive(results: list[dict], runs: int) -> str: + """One line per control, then pass@1 and pass^k over all of them. + + pass@1 is the mean share of runs that caught the plant: what one review + would be expected to do. pass^k is the share of controls caught on every + one of the k runs, which is what a control has to be before anyone leans on + it. Both are printed because a reviewer can score well on the first and + still be a coin toss on the second. + """ + lines = [] + for result in results: + fired = result["fired"] + quiet = ( + "misses not written" if fired is None else f"misses fired {fired}/{runs}" + ) + verdict = "holds" if holds(result, runs) else "DOES NOT HOLD" + lines.append( + f"{result['record']}: caught {result['caught']}/{runs}, {quiet} -- {verdict}" + ) + if results: + pass_at_1 = sum(result["caught"] for result in results) / (len(results) * runs) + pass_k = sum(result["caught"] == runs for result in results) + lines.append( + f"pass@1 {pass_at_1:.2f}; pass^{runs} {pass_k}/{len(results)} control(s)" + ) + measured = [ + result["fired"] for result in results if result["fired"] is not None + ] + if measured: + rate = sum(measured) / (len(measured) * runs) + lines.append( + f"false positives {rate:.2f} over {len(measured)} control(s) with a misses" + ) + return "\n".join(lines) + "\n" diff --git a/tests/test_review.py b/tests/test_review.py index ae1fa6f..831073e 100644 --- a/tests/test_review.py +++ b/tests/test_review.py @@ -8,12 +8,14 @@ from __future__ import annotations +import shutil import subprocess import sys import tempfile import textwrap import unittest from pathlib import Path +from typing import ClassVar ROOT = Path(__file__).resolve().parents[1] SCRIPT = ROOT / "uphold_check.py" @@ -276,6 +278,109 @@ def test_the_denominator_names_the_records_and_not_only_the_count(self): self.assertIsNone(review_mod.uncontrolled_note([])) +class Drive(unittest.TestCase): + """Planting a control and scoring what the reviewer names. + + The reviewer is a command, so these drive small Python scripts that answer + in its protocol. What is tested is the scoring and the refusals: a reviewer + that broke the protocol has said nothing about the record, and must not be + scored as having missed it. + """ + + CARRIED: ClassVar[dict[str, str]] = { + "single-authoritative-source": "Single Authoritative Source" + } + + def reviewer(self, body: str) -> list[str]: + tmp = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, tmp) + script = Path(tmp) / "reviewer.py" + script.write_text( + "import json, sys\npayload = json.load(sys.stdin)\n" + + textwrap.dedent(body), + encoding="utf-8", + ) + return [sys.executable, str(script)] + + def test_a_finding_may_be_the_id_or_the_title_in_any_case(self): + reviewer = self.reviewer( + """ + print("single-authoritative-source") + print() + print("single AUTHORITATIVE source") + """ + ) + named = review_mod.ask(reviewer, "doc", "change", self.CARRIED) + self.assertEqual(named, {"single-authoritative-source"}) + + def test_the_reviewer_is_handed_the_document_and_the_change_and_not_the_key(self): + reviewer = self.reviewer( + """ + assert sorted(payload) == ["change", "document"], sorted(payload) + assert payload == {"document": "doc", "change": "plant"}, payload + """ + ) + self.assertEqual(review_mod.ask(reviewer, "doc", "plant", self.CARRIED), set()) + + def test_a_name_outside_the_document_is_refused_rather_than_scored(self): + reviewer = self.reviewer('print("Single Authoritative Source.")') + with self.assertRaises(review_mod.ReviewerFailed) as raised: + review_mod.ask(reviewer, "doc", "change", self.CARRIED) + self.assertIn("'Single Authoritative Source.'", str(raised.exception)) + + def test_a_reviewer_that_exits_non_zero_is_refused_with_its_stderr(self): + reviewer = self.reviewer('sys.exit("no API key")') + with self.assertRaises(review_mod.ReviewerFailed) as raised: + review_mod.ask(reviewer, "doc", "change", self.CARRIED) + self.assertIn("exited 1", str(raised.exception)) + self.assertIn("no API key", str(raised.exception)) + + def test_a_reviewer_that_cannot_be_run_is_refused(self): + with self.assertRaises(review_mod.ReviewerFailed): + review_mod.ask(["/nonexistent/reviewer"], "doc", "change", self.CARRIED) + + def test_one_catch_in_three_is_counted_and_does_not_hold(self): + answers = iter([{"a"}, set(), set(), set(), set(), set()]) + declared = [{"record": "a", "catches": "plant", "misses": "clean"}] + results = review_mod.drive( + declared, "doc", {"a": "A"}, 3, lambda *_: next(answers) + ) + self.assertEqual(results, [{"record": "a", "caught": 1, "fired": 0}]) + self.assertFalse(review_mod.holds(results[0], 3)) + report = review_mod.render_drive(results, 3) + self.assertIn("a: caught 1/3, misses fired 0/3 -- DOES NOT HOLD", report) + self.assertIn("pass@1 0.33; pass^3 0/1", report) + + def test_firing_on_the_miss_breaks_a_control_that_caught_every_plant(self): + results = review_mod.drive( + [{"record": "a", "catches": "plant", "misses": "clean"}], + "doc", + {"a": "A"}, + 2, + lambda *_: {"a"}, + ) + self.assertEqual(results[0]["fired"], 2) + self.assertFalse(review_mod.holds(results[0], 2)) + self.assertIn("false positives 1.00", review_mod.render_drive(results, 2)) + + def test_an_unwritten_misses_is_unmeasured_and_not_a_rate_of_zero(self): + calls = [] + + def reviewer(document: str, change: str, carried: dict) -> set: + calls.append(change) + return {"a"} + + results = review_mod.drive( + [{"record": "a", "catches": "plant"}], "doc", {"a": "A"}, 2, reviewer + ) + self.assertEqual(calls, ["plant", "plant"]) + self.assertIsNone(results[0]["fired"]) + self.assertTrue(review_mod.holds(results[0], 2)) + report = review_mod.render_drive(results, 2) + self.assertIn("misses not written", report) + self.assertNotIn("false positives", report) + + @needs_the_engine class ClaimsThatEnforceNothing(unittest.TestCase): """A claim naming a rule no seam supplies must not silence the review. @@ -524,9 +629,8 @@ def test_this_repository_routes_cleanly(self): self.assertEqual(result.returncode, 0, result.stderr) def test_the_denominator_is_printed_on_a_run_that_found_nothing_wrong(self): - # The reader who most needs it is the one skimming a green run: this - # repository declares one control and carries twenty-nine records, and - # a run that said only "29 record(s) compile" would read like coverage. + # The reader who most needs it is the one skimming a green run: a run + # that said only how many records compile would read like coverage. result = subprocess.run( [sys.executable, str(SCRIPT), "--review"], cwd=ROOT, @@ -537,7 +641,7 @@ def test_the_denominator_is_printed_on_a_run_that_found_nothing_wrong(self): self.assertEqual(result.returncode, 0, result.stderr) self.assertIn("have no control", result.stdout) self.assertIn("nothing here shows they can produce a finding", result.stdout) - # The one record that does carry a control is not in that list. + # A record that does carry a control is not in that list. self.assertNotIn("single-authoritative-source", result.stdout) def test_the_compiled_documents_are_current(self): @@ -550,6 +654,43 @@ def test_the_compiled_documents_are_current(self): ) self.assertEqual(result.returncode, 0, result.stderr) + def drive(self, *argv: str) -> subprocess.CompletedProcess: + return subprocess.run( + [sys.executable, str(SCRIPT), "--review", "--drive", *argv], + cwd=ROOT, + capture_output=True, + text=True, + check=False, + ) + + def test_a_reviewer_holding_the_answer_key_holds_every_control(self): + # Reads the export, so it stays an oracle as controls are added: what + # this pins is the wiring from the declaration to the verdict. + oracle = textwrap.dedent( + """ + import json, sys + change = json.load(sys.stdin)["change"] + key = json.load(open("review-controls.json"))["controls"] + print("\\n".join(c["record"] for c in key if c["catches"] == change)) + """ + ) + result = self.drive("--runs", "2", "--", sys.executable, "-c", oracle) + self.assertEqual(result.returncode, 0, result.stderr) + self.assertIn("single-authoritative-source: caught 2/2", result.stdout) + self.assertIn("pass^2", result.stdout) + + def test_a_reviewer_that_names_nothing_is_reported_and_exits_1(self): + result = self.drive("--", sys.executable, "-c", "import sys; sys.stdin.read()") + self.assertEqual(result.returncode, 1, result.stderr) + self.assertIn("caught 0/3", result.stdout) + self.assertIn("DOES NOT HOLD", result.stdout) + + def test_drive_without_a_reviewer_is_a_usage_error(self): + for argv in [(), ("--runs", "0", "--", "true"), ("--runs", "2"), ("true",)]: + result = self.drive(*argv) + self.assertEqual(result.returncode, 2, argv) + self.assertIn("--drive [--runs N] -- REVIEWER", result.stderr) + def test_an_exemption_without_a_reason_is_refused(self): # A reason nobody wrote is one nobody can review, and it is exactly as # permanent as one that was argued for. diff --git a/uphold_check.py b/uphold_check.py index 219bc9a..b0679f8 100755 --- a/uphold_check.py +++ b/uphold_check.py @@ -738,11 +738,41 @@ def emit_target(root: Path, name: str) -> Path: return target +REVIEW_USAGE = ( + "usage: uphold_check.py --review " + "[--emit | --check | --drive [--runs N] -- REVIEWER...]" +) + + +def drive_arguments(argv: list[str]) -> tuple[int, list[str]] | None: + """`--drive [--runs N] -- REVIEWER...`, or None when it is not that shape. + + The reviewer is everything after `--`, verbatim, so its own flags never + have to be told apart from these. + """ + runs = review_mod.DEFAULT_RUNS + rest = argv[1:] + if rest[:1] == ["--runs"]: + if len(rest) < 2 or not rest[1].isdigit() or int(rest[1]) < 1: + return None + runs = int(rest[1]) + rest = rest[2:] + if rest[:1] != ["--"] or len(rest) < 2: + return None + return runs, rest[1:] + + def run_review(argv: list[str]) -> int: emit = argv[:1] == ["--emit"] check = argv[:1] == ["--check"] - if argv and not (emit or check): - print("usage: uphold_check.py --review [--emit|--check]", file=sys.stderr) + driving = None + if argv[:1] == ["--drive"]: + driving = drive_arguments(argv) + if driving is None: + print(REVIEW_USAGE, file=sys.stderr) + return 2 + elif argv and not (emit or check): + print(REVIEW_USAGE, file=sys.stderr) return 2 root = discover_root() @@ -802,6 +832,23 @@ def run_review(argv: list[str]) -> int: if errors or stale or control_errors or over: return 1 + if driving is not None: + runs, reviewer = driving + carried = {record["id"]: record["title"] for record in for_review} + try: + results = review_mod.drive( + settings["controls"], + document, + carried, + runs, + lambda doc, change, shown: review_mod.ask(reviewer, doc, change, shown), + ) + except review_mod.ReviewerFailed as error: + print(f"uphold review could not look: {error}", file=sys.stderr) + return 2 + print(review_mod.render_drive(results, runs), end="") + return 0 if all(review_mod.holds(result, runs) for result in results) else 1 + if emit: for name, path in control_targets: try: From 504d8b36a6bfc494e38f2d012ec1d7c9e7264b8e Mon Sep 17 00:00:00 2001 From: HackingGate Date: Sat, 24 Oct 2026 20:42:49 +0900 Subject: [PATCH 2/2] A reviewer that stalls or prints non-UTF-8 is exit 2, not a hung run or a traceback `--timeout SECONDS` (default 300) bounds each reviewer invocation, so one stalled call no longer holds every control after it. Output is decoded as UTF-8 explicitly, and a decode error is reported as the reviewer breaking the protocol rather than raised out of the run. Claude-Session: https://claude.ai/code/session_017EkhA3KMqw95DzAEYPkDn7 --- docs/REFERENCE.md | 6 ++++-- scripts/review.py | 27 +++++++++++++++++++++++++-- tests/test_review.py | 26 +++++++++++++++++++++++--- uphold_check.py | 27 ++++++++++++++++----------- 4 files changed, 68 insertions(+), 18 deletions(-) diff --git a/docs/REFERENCE.md b/docs/REFERENCE.md index b8ceb3a..440a574 100644 --- a/docs/REFERENCE.md +++ b/docs/REFERENCE.md @@ -4275,7 +4275,7 @@ carrying a second copy of the compiled document. `--drive` plants the controls itself, against a reviewer command you name: ```sh -uphold_check.py --review --drive [--runs N] -- ./my-reviewer --model x +uphold_check.py --review --drive [--runs N] [--timeout SECONDS] -- ./my-reviewer --model x ``` Everything after `--` is the reviewer, run once per plant. It reads one JSON @@ -4298,6 +4298,8 @@ named it against `misses`. pass@1 is the mean share of runs that caught their plant, and pass^k the share of controls caught on all `N`. The exit is `0` when every control holds and `1` when one does not. A reviewer that exits non-zero, cannot be run, or prints a line that names no record in the document is exit -`2`: it has said nothing about the record, so it is not scored as a miss. No +`2`: it has said nothing about the record, so it is not scored as a miss. So +is one that has not answered within `--timeout` seconds (default 300); a person +answering at a prompt passes a longer one. No model client lives here; the reviewer command carries the model, the key and the budget. See [DESIGN.md](DESIGN.md#a-control-is-the-probe-fixture-one-tier-up). diff --git a/scripts/review.py b/scripts/review.py index f7035e0..17bb3f1 100644 --- a/scripts/review.py +++ b/scripts/review.py @@ -53,6 +53,11 @@ #: run cannot tell it from a reviewer that catches it every time. DEFAULT_RUNS = 3 +#: Seconds one reviewer invocation may take. A model call answers in seconds; +#: a reviewer that has not answered in five minutes has stalled, and waiting on +#: it holds every control after it. A person at a prompt passes a longer one. +DEFAULT_TIMEOUT = 300 + #: The standing asymmetry, and it points one way on purpose: a static rule has #: no length ceiling and a prompt rule does, so every mechanizable rule is #: cheaper to push down into a static tier than to leave here. @@ -316,7 +321,11 @@ class ReviewerFailed(Exception): def ask( - reviewer: list[str], document: str, change: str, carried: dict[str, str] + reviewer: list[str], + document: str, + change: str, + carried: dict[str, str], + timeout: int = DEFAULT_TIMEOUT, ) -> set[str]: """Run the reviewer once and return the record ids it names. @@ -334,10 +343,24 @@ def ask( payload = json.dumps({"document": document, "change": change}, ensure_ascii=False) try: completed = subprocess.run( - reviewer, input=payload, capture_output=True, text=True, check=False + reviewer, + input=payload, + capture_output=True, + encoding="utf-8", + timeout=timeout, + check=False, ) except OSError as error: raise ReviewerFailed(f"could not run {reviewer[0]!r}: {error}") from error + except subprocess.TimeoutExpired as error: + raise ReviewerFailed( + f"{reviewer[0]!r} did not answer within {timeout}s; pass --timeout " + f"for a reviewer that needs longer" + ) from error + except UnicodeDecodeError as error: + raise ReviewerFailed( + f"{reviewer[0]!r} printed output that is not UTF-8: {error}" + ) from error if completed.returncode != 0: tail = completed.stderr.strip().splitlines()[-5:] raise ReviewerFailed( diff --git a/tests/test_review.py b/tests/test_review.py index 831073e..eca2cb7 100644 --- a/tests/test_review.py +++ b/tests/test_review.py @@ -335,6 +335,18 @@ def test_a_reviewer_that_exits_non_zero_is_refused_with_its_stderr(self): self.assertIn("exited 1", str(raised.exception)) self.assertIn("no API key", str(raised.exception)) + def test_a_reviewer_that_stalls_is_refused_at_the_deadline(self): + reviewer = self.reviewer("import time; time.sleep(30)") + with self.assertRaises(review_mod.ReviewerFailed) as raised: + review_mod.ask(reviewer, "doc", "change", self.CARRIED, timeout=1) + self.assertIn("did not answer within 1s", str(raised.exception)) + + def test_output_that_is_not_utf8_is_refused_rather_than_raised(self): + reviewer = self.reviewer('sys.stdout.buffer.write(b"\\xff\\n")') + with self.assertRaises(review_mod.ReviewerFailed) as raised: + review_mod.ask(reviewer, "doc", "change", self.CARRIED) + self.assertIn("not UTF-8", str(raised.exception)) + def test_a_reviewer_that_cannot_be_run_is_refused(self): with self.assertRaises(review_mod.ReviewerFailed): review_mod.ask(["/nonexistent/reviewer"], "doc", "change", self.CARRIED) @@ -674,7 +686,9 @@ def test_a_reviewer_holding_the_answer_key_holds_every_control(self): print("\\n".join(c["record"] for c in key if c["catches"] == change)) """ ) - result = self.drive("--runs", "2", "--", sys.executable, "-c", oracle) + result = self.drive( + "--timeout", "60", "--runs", "2", "--", sys.executable, "-c", oracle + ) self.assertEqual(result.returncode, 0, result.stderr) self.assertIn("single-authoritative-source: caught 2/2", result.stdout) self.assertIn("pass^2", result.stdout) @@ -686,10 +700,16 @@ def test_a_reviewer_that_names_nothing_is_reported_and_exits_1(self): self.assertIn("DOES NOT HOLD", result.stdout) def test_drive_without_a_reviewer_is_a_usage_error(self): - for argv in [(), ("--runs", "0", "--", "true"), ("--runs", "2"), ("true",)]: + for argv in [ + (), + ("--runs", "0", "--", "true"), + ("--timeout", "x", "--", "true"), + ("--runs", "2"), + ("true",), + ]: result = self.drive(*argv) self.assertEqual(result.returncode, 2, argv) - self.assertIn("--drive [--runs N] -- REVIEWER", result.stderr) + self.assertIn("--drive [--runs N] [--timeout SECONDS]", result.stderr) def test_an_exemption_without_a_reason_is_refused(self): # A reason nobody wrote is one nobody can review, and it is exactly as diff --git a/uphold_check.py b/uphold_check.py index b0679f8..1d64ab7 100755 --- a/uphold_check.py +++ b/uphold_check.py @@ -740,26 +740,29 @@ def emit_target(root: Path, name: str) -> Path: REVIEW_USAGE = ( "usage: uphold_check.py --review " - "[--emit | --check | --drive [--runs N] -- REVIEWER...]" + "[--emit | --check | --drive [--runs N] [--timeout SECONDS] -- REVIEWER...]" ) -def drive_arguments(argv: list[str]) -> tuple[int, list[str]] | None: - """`--drive [--runs N] -- REVIEWER...`, or None when it is not that shape. +def drive_arguments(argv: list[str]) -> tuple[int, int, list[str]] | None: + """`--drive [--runs N] [--timeout SECONDS] -- REVIEWER...`, or None. - The reviewer is everything after `--`, verbatim, so its own flags never - have to be told apart from these. + Returns (runs, timeout, reviewer). The reviewer is everything after `--`, + verbatim, so its own flags never have to be told apart from these. """ - runs = review_mod.DEFAULT_RUNS + values = { + "--runs": review_mod.DEFAULT_RUNS, + "--timeout": review_mod.DEFAULT_TIMEOUT, + } rest = argv[1:] - if rest[:1] == ["--runs"]: + while rest[:1] and rest[0] in values: if len(rest) < 2 or not rest[1].isdigit() or int(rest[1]) < 1: return None - runs = int(rest[1]) + values[rest[0]] = int(rest[1]) rest = rest[2:] if rest[:1] != ["--"] or len(rest) < 2: return None - return runs, rest[1:] + return values["--runs"], values["--timeout"], rest[1:] def run_review(argv: list[str]) -> int: @@ -833,7 +836,7 @@ def run_review(argv: list[str]) -> int: return 1 if driving is not None: - runs, reviewer = driving + runs, timeout, reviewer = driving carried = {record["id"]: record["title"] for record in for_review} try: results = review_mod.drive( @@ -841,7 +844,9 @@ def run_review(argv: list[str]) -> int: document, carried, runs, - lambda doc, change, shown: review_mod.ask(reviewer, doc, change, shown), + lambda doc, change, shown: review_mod.ask( + reviewer, doc, change, shown, timeout + ), ) except review_mod.ReviewerFailed as error: print(f"uphold review could not look: {error}", file=sys.stderr)