Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions ROADMAP.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
22 changes: 16 additions & 6 deletions docs/DESIGN.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
37 changes: 33 additions & 4 deletions docs/REFERENCE.md
Original file line number Diff line number Diff line change
Expand Up @@ -4270,7 +4270,36 @@ 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] [--timeout SECONDS] -- ./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. 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).
33 changes: 24 additions & 9 deletions policy/upheld.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand All @@ -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."
12 changes: 10 additions & 2 deletions review-controls.json
Original file line number Diff line number Diff line change
@@ -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.",
Expand All @@ -14,15 +24,13 @@
"economy-of-mechanism",
"end-to-end-principle",
"enforcement-needs-a-trigger",
"fail-fast",
"follow-published-standards",
"goodharts-law",
"graceful-degradation",
"high-cohesion-low-coupling",
"hyrums-law",
"information-hiding",
"informed-consent",
"least-astonishment",
"least-common-mechanism",
"least-power",
"make-illegal-states-unrepresentable",
Expand Down
164 changes: 161 additions & 3 deletions scripts/review.py
Original file line number Diff line number Diff line change
Expand Up @@ -34,17 +34,30 @@
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

#: 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.
Expand Down Expand Up @@ -235,9 +248,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.
Expand Down Expand Up @@ -296,3 +309,148 @@ 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],
timeout: int = DEFAULT_TIMEOUT,
) -> 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,
encoding="utf-8",
timeout=timeout,
check=False,
)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
except OSError as error:
raise ReviewerFailed(f"could not run {reviewer[0]!r}: {error}") from error
Comment thread
coderabbitai[bot] marked this conversation as resolved.
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(
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"
Loading
Loading