From 5c11b51236730cbeeec2619c728db51955c34900 Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Tue, 25 Aug 2026 18:44:53 +0000 Subject: [PATCH 1/3] feat(rules): refuse a ready whose review is unread or unanswered MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `land` readies a PR without ever asking whether anyone reviewed it. Three gates run before the ready — `deferral-check`, `filed-here-check`, `closing-key-check` — and none is about code review. The ready is what starts the matrix, so an unreviewed head, or one carrying findings nobody answered, spends CI exactly as readily as a reviewed one. Replayed over the 100 most recently merged PRs the predicate fires on 89: 21 carrying no review from anyone but their author, 68 carrying unresolved threads, 163 open threads across the fired set. This is the fact channel's FIRST consumer. `[[fact]]` has had none since CLOUD-776 landed it. ## The count is the predicate, and that is a constraint rather than a shortcut `facts::Sourced` stores `{command, seen_at, rows}` and no byte of the buffer (non-negotiable rule 4, structural rather than careful), so `reviews` and `reviewThreads` never reach Rego and the predicate CLOUD-859 §2 specifies is unwritable. What reaches the module is how many rows the declared command's stdout carried. So the selection lives in the command's `--jq` projection — one element per blocking condition, each unresolved thread's id plus the PR author's login when nothing but the author reviewed — and `rows == 0` is exactly "reviewed and addressed". Which is also why the finding names the count and not the thread ids: they are not in the engine, and a `msg` claiming them would assert a payload this channel refuses to carry. ## Three rows, and the coupling between them is not obvious `node(id:)` rather than `repository(owner:,name:)` because `Declared.command` is compared byte-for-byte — that equality IS the forgery control — so the string must be a constant. `gh pr view --json id` derives the node id from the current branch, which keeps the query constant AND per-PR, and keeps the owner and repo name out of it. The projection to an array is semantic rather than a parsing workaround: post-CLOUD-992 a bare object normalises to one row whatever the query found, so without the brackets the count means nothing. `ready-needs-an-answered-review` is a `receipt` row, and it is also what makes the fact reach Rego at all: `required_checks_for` reads receipt rows and `agent_records` populates the agent-sourced key only for the checks they name, so without it the module reads null on every call. It owns the never-ran and command-mismatch refusals because only it is handed the declaration and can print the command. The module decides the COUNT and stays silent about absence, so one call never draws two refusals. ## What the anchor is actually for, measured rather than assumed The module narrows on `startswith` rather than `contains`, and the first draft of this change claimed that was what stopped it judging PROSE — a commit message naming `gh pr ready`, which this repository writes constantly. Probed against the real binary with only the receipt row declared, that claim is false: the engine's `pattern` matcher is ANCHORED, so it does not select `git commit -m "... gh pr ready ..."`, no record is projected for it, and the module decides nothing about it either way. So the anchor is defence in depth and is labelled as such: it keeps this module's correctness from depending on another row's pattern. Widen that `pattern` and a `contains` here would start judging commit messages. It accepts one false negative — a compound `cd x && gh pr ready` is not judged here, though the receipt row still selects it — and CLOUD-199 is why that direction is right: a guard with false positives gets bypassed, and then it guards nothing. ## Shown able to fail, and one declared mutation was deleted for being unprovable `tests/review-answered.bats` is the tier that proves the ENGINE builds what the predicate reads: every case makes two real hook calls, a `PostToolUse` that mints the record and a `PreToolUse` that reads it, and nothing writes a receipt by hand. It reads the declared command out of `batten.toml` by NAME rather than retyping it, so a rewording there fails here instead of silently ending the coupling. `count-not-decided` kills exactly the two cases the module decides and leaves the receipt row's green; `redraft-judged` kills exactly the re-draft case. A third row aimed at the anchor SURVIVED twice — first named at a case whose command carries no `ready` at all, so the cheap conjunct excluded it, and then correctly named and still invisible, because the engine never hands the module a prose command with a record beside it. That second survival is a true statement about the module rather than a bad filter, so the row is deleted instead of retuned: a declared mutation that cannot be caught is a coverage claim that is not true. The anchor's discriminating case stays in the load-time tier, where `with input as` can put a record beside prose. Live, both directions, on PR #702 rather than a fixture: two unresolved threads read as two rows mid-`land`, and an empty array once both were answered. Not in this change, and the row is emphatic: the published check-run and the `protect-main` required-context edit never land together, because `bypass_actors: []` means a wrongly-failing required context cannot be merged past. Refs: CLOUD-859 --- batten.toml | 89 ++++++++++++ bench/suites/RESULTS.md | 279 ++++++++++++++++++------------------ mise.toml | 2 +- policy/review-answered.rego | 205 ++++++++++++++++++++++++++ tests/review-answered.bats | 250 ++++++++++++++++++++++++++++++++ 5 files changed, 685 insertions(+), 140 deletions(-) create mode 100644 policy/review-answered.rego create mode 100644 tests/review-answered.bats diff --git a/batten.toml b/batten.toml index 07bd4b3e2..2c6970e7a 100644 --- a/batten.toml +++ b/batten.toml @@ -430,6 +430,95 @@ retry — or just `mise run land`, which drives the whole loop. A red run on a \ freshly-readied PR means that step was skipped, and the CI minutes are already \ spent by then.""" +# CLOUD-859. The read-the-review gate, and this repository's FIRST `[[fact]]` +# row — the agent-sourced channel has had no consumer since CLOUD-776 landed it. +# +# THE COMMAND IS THE PREDICATE'S SELECTION, not merely its source. `Sourced` +# stores `{command, seen_at, rows}` and no byte of the buffer (non-negotiable +# rule 4), so a module can ask how many rows came back and nothing else. The +# `--jq` projection therefore emits ONE ELEMENT PER BLOCKING CONDITION — each +# unresolved review thread's id, plus the PR author's login when nothing but the +# author has reviewed — which is what makes `rows == 0` mean "reviewed and +# addressed" rather than "nobody looked". +# +# WHY IT PROJECTS TO AN ARRAY, on grounds that outlived the parsing reason. +# CLOUD-992 taught `rows_in` to read a text buffer, so a bare object no longer +# fails to parse — it normalises to `Is(1)`, always one row whatever the query +# found. The `[...]` is semantic now: without it the count means nothing. Do not +# "simplify" it away on the grounds that the parse exists. +# +# WHY `node(id:)` RATHER THAN `repository(owner:,name:)`. `Declared.command` is +# compared byte-for-byte against what the agent ran — that byte-equality IS the +# forgery control — so the string is a CONSTANT and cannot be parameterised per +# pull request. `gh pr view --json id` derives the PR's node id from the current +# branch, so the whole query is constant and still per-PR. It also keeps the +# owner and repository name out of the string, which `repository(owner:...)` +# would have hard-coded. +# +# WHY NOT `gh pr view` ALONE: checked against `gh` 2.97.0, its field list carries +# `reviews`, `latestReviews` and `reviewDecision` and has NO `reviewThreads`. +# Threads are the half this decides on, so `gh pr view` cannot answer it. +# +# WHY NOT A `mise run` PRODUCER, which is shorter and would have been nicer: +# measured, and it is unusable. `capture::decode_response` concatenates `stdout` +# then `stderr`, so mise's `[task] $ …` prefix lands in the buffer either way, +# the text stops parsing as JSON, and the count is one opaque row forever — a +# gate refusing every `gh pr ready` in the repository and unsatisfiable by the +# command its own deny prints. Probed side by side: the bare command recorded +# `rows 3` where `mise run envelope-probe` over the identical body recorded +# `rows 1`. The residue is broader than mise and is CLOUD-993's inventory: any +# declared command routed through something that annotates its own output is +# disqualified by the same mechanism. +# +# `returns = "json-array"` is what keeps that from being silent (CLOUD-993). A +# command that stops emitting an array — an auth error, a changed default format, +# a wrapper that annotates — records NOTHING and denies, instead of recording a +# plausible `rows 1` that no `rows == 0` predicate can ever clear. +[[fact]] +name = "review-answered" +returns = "json-array" +command = "gh api graphql -F id=$(gh pr view --json id --jq .id) -f query='query($id:ID!){node(id:$id){... on PullRequest{author{login} reviews(last:100){nodes{author{login}}} reviewThreads(last:100){nodes{id isResolved}}}}}' --jq '.data.node as $p | [($p.reviewThreads.nodes[] | select(.isResolved == false) | .id), (if ([$p.reviews.nodes[] | select(.author.login != $p.author.login)] | length) == 0 then $p.author.login else empty end)]'" + +# The DID-YOU-LOOK half. A `receipt` row is also what makes the fact reach Rego +# at all: `required_checks_for` reads the receipt rows, and `agent_records` +# populates `input.facts["agent-sourced"]` only for the checks they name — so +# without this row the policy module below reads `null` on every call and decides +# nothing. That coupling is not obvious and is why the two rows ship together. +# +# It carries the never-ran and command-mismatch refusals, and only it can: the +# boundary hands it the declaration, so its deny is built from the declared +# command rather than from prose. The module decides the COUNT and stays silent +# about absence, so one call never draws two refusals. +# +# `key = "head"` because an agent-sourced record is resolved from its own file +# rather than from the receipt store, so the keying is unread on this path — the +# column is required by `rules::validate` and `head` is the honest answer for a +# fact about a specific head. +[[rule]] +id = "ready-needs-an-answered-review" +kind = "receipt" +scope = "mediated_call" +severity = "deny" +pattern = "gh pr ready" +checks = ["review-answered"] +key = "head" +reason = """ +Read the review before buying a matrix. Run the declared command above — it \ +prints one line per blocking condition and nothing when the head is clear — \ +then retry. If no review exists yet, force one with `@coderabbitai full review` \ +(~3 minutes measured, and free: the draft phase runs no CI), answer what it \ +says, and run the command again. Replayed over the last 100 merges this fires \ +on 89, so the usual outcome is that there is something to read.""" + +# The DID-YOU-ANSWER-IT half. The module's own header carries the measurement, +# the count reformulation, and why the thread ids cannot be in the message. +[[rule]] +id = "review-answered" +kind = "policy" +scope = "mediated_call" +module = "policy/review-answered.rego" +severity = "deny" + # `claim-guard`, as config (CLOUD-444) — the other half of the pair # `ready-needs-receipts` starts, and the one the receipt kind could not express # until this change. diff --git a/bench/suites/RESULTS.md b/bench/suites/RESULTS.md index cfd822a93..d71c8e9b4 100644 --- a/bench/suites/RESULTS.md +++ b/bench/suites/RESULTS.md @@ -6,168 +6,169 @@ runner measured it; the suite runs `--no-parallelize-within-files`, so a file's number is its own serial cost and is what an author adding a case to it pays. -- suites: 160 -- serial total: 1382.4s +- suites: 161 +- serial total: 1498.2s | seconds | share | suite | | ---: | ---: | --- | -| 144.4 | 10.4% | `tests/land-lock.bats` | -| 140.1 | 10.1% | `tests/session-start.bats` | -| 118.3 | 8.6% | `tests/derived-check.bats` | -| 102.4 | 7.4% | `tests/ci-wait.bats` | -| 79.9 | 5.8% | `tests/land.bats` | -| 60.3 | 4.4% | `tests/hooks-wiring-check.bats` | -| 54.9 | 4.0% | `tests/commit-convention.bats` | -| 52.0 | 3.8% | `tests/ci-local-parity.bats` | -| 51.4 | 3.7% | `tests/prebuilt-lint.bats` | -| 39.0 | 2.8% | `tests/config-deprecations.bats` | -| 34.7 | 2.5% | `tests/main-watch.bats` | -| 32.6 | 2.4% | `tests/sbom-check.bats` | -| 26.8 | 1.9% | `tests/config-lint.bats` | -| 26.1 | 1.9% | `tests/helpers.bats` | -| 24.3 | 1.8% | `tests/hook-latency-drift.bats` | -| 23.7 | 1.7% | `tests/claim-check.bats` | -| 18.4 | 1.3% | `tests/graph-check.bats` | -| 17.1 | 1.2% | `tests/token-bench.bats` | -| 14.9 | 1.1% | `tests/reference-check.bats` | -| 14.3 | 1.0% | `tests/board-diff-overlap.bats` | -| 13.3 | 1.0% | `tests/board-write-record.bats` | -| 12.3 | 0.9% | `tests/stop-guard.bats` | -| 11.0 | 0.8% | `tests/ready-guard.bats` | -| 10.4 | 0.8% | `tests/run-shape-guard.bats` | -| 9.0 | 0.6% | `tests/filed-here-check.bats` | -| 8.9 | 0.6% | `tests/ready-lint.bats` | -| 8.9 | 0.6% | `tests/step-receipt.bats` | -| 8.4 | 0.6% | `tests/target-race.bats` | -| 7.0 | 0.5% | `tests/ready-cites-check.bats` | -| 6.9 | 0.5% | `tests/released.bats` | -| 6.9 | 0.5% | `tests/board-sweep.bats` | -| 6.7 | 0.5% | `tests/mutant.bats` | -| 6.4 | 0.5% | `tests/lock-complete.bats` | -| 6.3 | 0.5% | `tests/renovate-config-validator.bats` | -| 6.1 | 0.4% | `tests/sbom.bats` | -| 5.8 | 0.4% | `tests/singleton.bats` | -| 5.5 | 0.4% | `tests/replay.bats` | -| 5.5 | 0.4% | `tests/release-tracking-check.bats` | -| 5.3 | 0.4% | `tests/mcp-allow-check.bats` | -| 5.2 | 0.4% | `tests/task-registry.bats` | +| 141.9 | 9.5% | `tests/land-lock.bats` | +| 128.3 | 8.6% | `tests/derived-check.bats` | +| 118.0 | 7.9% | `tests/step-receipt.bats` | +| 116.1 | 7.7% | `tests/stop-guard.bats` | +| 102.3 | 6.8% | `tests/spec-ref-check.bats` | +| 102.3 | 6.8% | `tests/ci-wait.bats` | +| 95.5 | 6.4% | `tests/semver.bats` | +| 81.0 | 5.4% | `tests/land.bats` | +| 63.0 | 4.2% | `tests/hooks-wiring-check.bats` | +| 40.6 | 2.7% | `tests/ci-local-parity.bats` | +| 34.5 | 2.3% | `tests/main-watch.bats` | +| 28.3 | 1.9% | `tests/suite-bench-check.bats` | +| 27.9 | 1.9% | `tests/graph-check.bats` | +| 24.3 | 1.6% | `tests/hook-latency-drift.bats` | +| 18.3 | 1.2% | `tests/sbom-check.bats` | +| 18.1 | 1.2% | `tests/prebuilt-lint.bats` | +| 17.0 | 1.1% | `tests/claim-check.bats` | +| 16.2 | 1.1% | `tests/helpers.bats` | +| 14.5 | 1.0% | `tests/board-diff-overlap.bats` | +| 14.2 | 0.9% | `tests/board-write-record.bats` | +| 13.6 | 0.9% | `tests/filed-here-check.bats` | +| 11.5 | 0.8% | `tests/config-lint.bats` | +| 9.9 | 0.7% | `tests/suite-select.bats` | +| 9.7 | 0.6% | `tests/ready-lint.bats` | +| 8.5 | 0.6% | `tests/replay.bats` | +| 8.5 | 0.6% | `tests/run-shape-guard.bats` | +| 8.0 | 0.5% | `tests/ready-guard.bats` | +| 7.5 | 0.5% | `tests/lock-complete.bats` | +| 7.4 | 0.5% | `tests/target-race.bats` | +| 7.4 | 0.5% | `tests/token-bench.bats` | +| 7.3 | 0.5% | `tests/released.bats` | +| 7.1 | 0.5% | `tests/board-sweep.bats` | +| 7.1 | 0.5% | `tests/mutant.bats` | +| 6.3 | 0.4% | `tests/renovate-config-validator.bats` | +| 6.0 | 0.4% | `tests/release-tracking-check.bats` | +| 5.5 | 0.4% | `tests/release-assets-check.bats` | +| 5.4 | 0.4% | `tests/mcp-allow-check.bats` | +| 5.3 | 0.4% | `tests/session-start.bats` | +| 5.0 | 0.3% | `tests/sbom.bats` | +| 5.0 | 0.3% | `tests/singleton.bats` | | 4.8 | 0.3% | `tests/in-progress-drain.bats` | -| 4.4 | 0.3% | `tests/schema-check.bats` | -| 4.1 | 0.3% | `tests/skill-check.bats` | -| 4.0 | 0.3% | `tests/pre-commit-staging.bats` | -| 3.9 | 0.3% | `tests/release-assets-check.bats` | -| 3.8 | 0.3% | `tests/target-ensure.bats` | -| 3.7 | 0.3% | `tests/signing-posture.bats` | -| 3.7 | 0.3% | `tests/doctor-race.bats` | -| 3.6 | 0.3% | `tests/semver.bats` | -| 3.5 | 0.3% | `tests/suite-select.bats` | -| 3.5 | 0.2% | `tests/ntia-check.bats` | -| 3.4 | 0.2% | `tests/with-lock.bats` | -| 3.4 | 0.2% | `tests/spec-ref-check.bats` | -| 3.4 | 0.2% | `tests/hk-selection.bats` | -| 3.3 | 0.2% | `tests/land-divergence.bats` | -| 3.3 | 0.2% | `tests/ready-lint-deferral.bats` | -| 3.2 | 0.2% | `tests/unlanded-check.bats` | -| 2.8 | 0.2% | `tests/tree-clean.bats` | -| 2.6 | 0.2% | `tests/verify.bats` | -| 2.4 | 0.2% | `tests/reclaim-census.bats` | -| 2.3 | 0.2% | `tests/run-shape.bats` | -| 2.3 | 0.2% | `tests/fanout-guard.bats` | -| 2.2 | 0.2% | `tests/landed-check.bats` | -| 2.2 | 0.2% | `tests/rules-drift.bats` | -| 2.1 | 0.2% | `tests/spawn-census.bats` | -| 2.0 | 0.1% | `tests/closing-key-check.bats` | -| 2.0 | 0.1% | `tests/claim-race-check.bats` | -| 1.9 | 0.1% | `tests/finding-sink-check.bats` | -| 1.9 | 0.1% | `tests/target-prune.bats` | -| 1.7 | 0.1% | `tests/bot-issue.bats` | -| 1.6 | 0.1% | `tests/ci-tools-check.bats` | -| 1.6 | 0.1% | `tests/claimed-keys.bats` | -| 1.5 | 0.1% | `tests/ci-slow-needed.bats` | -| 1.5 | 0.1% | `tests/prose-only-check.bats` | -| 1.5 | 0.1% | `tests/timeout-drift.bats` | -| 1.3 | 0.1% | `tests/ci-lease-precondition.bats` | -| 1.3 | 0.1% | `tests/memories-check.bats` | -| 1.3 | 0.1% | `tests/stop-posture-check.bats` | -| 1.2 | 0.1% | `tests/mutant-census.bats` | -| 1.2 | 0.1% | `tests/verified.bats` | -| 1.2 | 0.1% | `tests/awk-regex-check.bats` | -| 1.2 | 0.1% | `tests/publish-credential-check.bats` | -| 1.2 | 0.1% | `tests/suite-bench-check.bats` | -| 1.1 | 0.1% | `tests/alive.bats` | -| 1.1 | 0.1% | `tests/install-check.bats` | -| 1.1 | 0.1% | `tests/land-divergence-assert.bats` | -| 1.1 | 0.1% | `tests/perf-record.bats` | -| 1.1 | 0.1% | `tests/deferral-check.bats` | -| 1.1 | 0.1% | `tests/done-check.bats` | -| 1.0 | 0.1% | `tests/privileged-lane.bats` | -| 1.0 | 0.1% | `tests/nonverdict-scan.bats` | +| 4.1 | 0.3% | `tests/task-registry.bats` | +| 4.0 | 0.3% | `tests/schema-check.bats` | +| 3.8 | 0.3% | `tests/pre-commit-staging.bats` | +| 3.8 | 0.3% | `tests/reference-check.bats` | +| 3.8 | 0.3% | `tests/ntia-check.bats` | +| 3.8 | 0.3% | `tests/ready-cites-check.bats` | +| 3.6 | 0.2% | `tests/doctor-race.bats` | +| 3.6 | 0.2% | `tests/land-divergence.bats` | +| 3.6 | 0.2% | `tests/tree-clean.bats` | +| 3.6 | 0.2% | `tests/unlanded-check.bats` | +| 3.5 | 0.2% | `tests/fanout-guard.bats` | +| 3.4 | 0.2% | `tests/finding-sink-check.bats` | +| 3.2 | 0.2% | `tests/hk-selection.bats` | +| 2.8 | 0.2% | `tests/with-lock.bats` | +| 2.6 | 0.2% | `tests/target-ensure.bats` | +| 2.5 | 0.2% | `tests/landed-check.bats` | +| 2.1 | 0.1% | `tests/claim-race-check.bats` | +| 2.1 | 0.1% | `tests/config-deprecations.bats` | +| 2.1 | 0.1% | `tests/closing-key-check.bats` | +| 2.1 | 0.1% | `tests/rules-drift.bats` | +| 2.0 | 0.1% | `tests/done-check.bats` | +| 1.9 | 0.1% | `tests/ready-lint-deferral.bats` | +| 1.8 | 0.1% | `tests/alive.bats` | +| 1.8 | 0.1% | `tests/skill-check.bats` | +| 1.7 | 0.1% | `tests/ci-slow-needed.bats` | +| 1.7 | 0.1% | `tests/reclaim-census.bats` | +| 1.7 | 0.1% | `tests/signing-posture.bats` | +| 1.7 | 0.1% | `tests/memories-check.bats` | +| 1.7 | 0.1% | `tests/ci-tools-check.bats` | +| 1.6 | 0.1% | `tests/bot-issue.bats` | +| 1.5 | 0.1% | `tests/done-pr-check.bats` | +| 1.5 | 0.1% | `tests/claimed-keys.bats` | +| 1.5 | 0.1% | `tests/doctor.bats` | +| 1.5 | 0.1% | `tests/ci-lease-precondition.bats` | +| 1.4 | 0.1% | `tests/gh-guard.bats` | +| 1.4 | 0.1% | `tests/mutant-census.bats` | +| 1.3 | 0.1% | `tests/perf-record.bats` | +| 1.3 | 0.1% | `tests/awk-regex-check.bats` | +| 1.2 | 0.1% | `tests/duplicate-close-check.bats` | +| 1.2 | 0.1% | `tests/install-check.bats` | +| 1.2 | 0.1% | `tests/review-answered.bats` | +| 1.2 | 0.1% | `tests/verify.bats` | +| 1.2 | 0.1% | `tests/release-backfill.bats` | +| 1.2 | 0.1% | `tests/land-divergence-assert.bats` | +| 1.2 | 0.1% | `tests/deferral-check.bats` | +| 1.2 | 0.1% | `tests/run-shape.bats` | +| 1.2 | 0.1% | `tests/perf-assert.bats` | +| 1.1 | 0.1% | `tests/module-map-check.bats` | +| 1.0 | 0.1% | `tests/attestation-check.bats` | | 1.0 | 0.1% | `tests/linear-check.bats` | -| 0.9 | 0.1% | `tests/sonar-gate.bats` | -| 0.9 | 0.1% | `tests/attestation-check.bats` | -| 0.9 | 0.1% | `tests/release-backfill.bats` | -| 0.9 | 0.1% | `tests/sbom-binary.bats` | -| 0.9 | 0.1% | `tests/transcript-corpus-check.bats` | -| 0.8 | 0.1% | `tests/gh-guard.bats` | -| 0.8 | 0.1% | `tests/done-pr-check.bats` | -| 0.8 | 0.1% | `tests/perf-assert.bats` | -| 0.8 | 0.1% | `tests/module-map-check.bats` | -| 0.8 | 0.1% | `tests/render-cli.bats` | -| 0.8 | 0.1% | `tests/doctor.bats` | +| 1.0 | 0.1% | `tests/spawn-census.bats` | +| 1.0 | 0.1% | `tests/hook-profile-check.bats` | +| 1.0 | 0.1% | `tests/nonverdict-scan.bats` | +| 0.9 | 0.1% | `tests/target-prune.bats` | +| 0.9 | 0.1% | `tests/evaluator-closure-check.bats` | +| 0.9 | 0.1% | `tests/checks-green.bats` | +| 0.9 | 0.1% | `tests/pr-unsubscribed.bats` | | 0.8 | 0.1% | `tests/lint-deno.bats` | -| 0.8 | 0.1% | `tests/checks-green.bats` | -| 0.8 | 0.1% | `tests/pr-unsubscribed.bats` | -| 0.7 | 0.1% | `tests/hook-pin-check.bats` | -| 0.7 | 0.1% | `tests/timeout-check.bats` | -| 0.7 | 0.1% | `tests/duplicate-close-check.bats` | -| 0.7 | 0.0% | `tests/hook-matcher-check.bats` | -| 0.7 | 0.0% | `tests/mcp-timeout-budget.bats` | +| 0.8 | 0.1% | `tests/install.bats` | +| 0.8 | 0.1% | `tests/sbom-binary.bats` | +| 0.8 | 0.1% | `tests/hook-pin-check.bats` | +| 0.8 | 0.1% | `tests/timeout-drift.bats` | +| 0.7 | 0.0% | `tests/prose-only-check.bats` | +| 0.7 | 0.0% | `tests/render-cli.bats` | +| 0.7 | 0.0% | `tests/perf-compare.bats` | | 0.7 | 0.0% | `tests/merged-pr-keys.bats` | -| 0.6 | 0.0% | `tests/mcp-attach-check.bats` | -| 0.6 | 0.0% | `tests/perf-compare.bats` | -| 0.6 | 0.0% | `tests/install.bats` | -| 0.6 | 0.0% | `tests/macos-link-check.bats` | -| 0.6 | 0.0% | `tests/hook-profile-check.bats` | -| 0.6 | 0.0% | `tests/evaluator-closure-check.bats` | +| 0.7 | 0.0% | `tests/lint-rego.bats` | +| 0.6 | 0.0% | `tests/hook-matcher-check.bats` | +| 0.6 | 0.0% | `tests/mcp-timeout-budget.bats` | | 0.6 | 0.0% | `tests/checksums.bats` | +| 0.6 | 0.0% | `tests/mcp-attach-check.bats` | | 0.6 | 0.0% | `tests/connector-verb-guard.bats` | -| 0.5 | 0.0% | `tests/lint-rego.bats` | +| 0.5 | 0.0% | `tests/publish-credential-check.bats` | +| 0.5 | 0.0% | `tests/macos-link-check.bats` | +| 0.5 | 0.0% | `tests/pipefail-grep-check.bats` | +| 0.5 | 0.0% | `tests/branch-age-check.bats` | +| 0.5 | 0.0% | `tests/verified.bats` | | 0.5 | 0.0% | `tests/board-payloads.bats` | +| 0.5 | 0.0% | `tests/abandon-matrix.bats` | | 0.5 | 0.0% | `tests/msrv-pin-agreement.bats` | -| 0.4 | 0.0% | `tests/abandon-matrix.bats` | +| 0.4 | 0.0% | `tests/git-hook.bats` | | 0.4 | 0.0% | `tests/digest-major-agreement.bats` | -| 0.4 | 0.0% | `tests/pipefail-grep-check.bats` | | 0.4 | 0.0% | `tests/connector-allow-guard.bats` | -| 0.4 | 0.0% | `tests/run-shape-guard-quoting.bats` | -| 0.4 | 0.0% | `tests/serena-mcp.bats` | -| 0.4 | 0.0% | `tests/branch-age-check.bats` | | 0.4 | 0.0% | `tests/land-lock-check.bats` | -| 0.4 | 0.0% | `tests/task-fail-closed.bats` | | 0.4 | 0.0% | `tests/pkl-check.bats` | -| 0.4 | 0.0% | `tests/nonverdict-assert.bats` | -| 0.3 | 0.0% | `tests/token-bench-check.bats` | -| 0.3 | 0.0% | `tests/commit-attribution.bats` | -| 0.3 | 0.0% | `tests/no-doctests.bats` | -| 0.3 | 0.0% | `tests/report-only-check.bats` | -| 0.3 | 0.0% | `tests/batten-glob-check.bats` | -| 0.3 | 0.0% | `tests/release-due.bats` | +| 0.4 | 0.0% | `tests/stop-posture-check.bats` | +| 0.4 | 0.0% | `tests/release-due.bats` | +| 0.4 | 0.0% | `tests/timeout-check.bats` | +| 0.4 | 0.0% | `tests/report-only-check.bats` | +| 0.4 | 0.0% | `tests/sonar-gate.bats` | +| 0.4 | 0.0% | `tests/no-doctests.bats` | +| 0.4 | 0.0% | `tests/transcript-corpus-check.bats` | +| 0.4 | 0.0% | `tests/serena-mcp.bats` | +| 0.4 | 0.0% | `tests/run-shape-guard-quoting.bats` | | 0.3 | 0.0% | `tests/cap-drift.bats` | | 0.3 | 0.0% | `tests/connector-allow-resolve.bats` | -| 0.3 | 0.0% | `tests/ci-drift.bats` | +| 0.3 | 0.0% | `tests/nonverdict-assert.bats` | +| 0.3 | 0.0% | `tests/commit-attribution.bats` | | 0.3 | 0.0% | `tests/license-table-check.bats` | +| 0.3 | 0.0% | `tests/privileged-lane.bats` | +| 0.3 | 0.0% | `tests/batten-glob-check.bats` | | 0.3 | 0.0% | `tests/container-preflight.bats` | -| 0.3 | 0.0% | `tests/git-hook.bats` | +| 0.3 | 0.0% | `tests/commit-convention.bats` | | 0.3 | 0.0% | `tests/mise-pin-agreement.bats` | -| 0.3 | 0.0% | `tests/rust-paths-check.bats` | +| 0.3 | 0.0% | `tests/ci-drift.bats` | | 0.3 | 0.0% | `tests/coderabbit-config-check.bats` | -| 0.2 | 0.0% | `tests/test-bats-parallel.bats` | | 0.2 | 0.0% | `tests/mise-action-floor.bats` | +| 0.2 | 0.0% | `tests/egress-check.bats` | +| 0.2 | 0.0% | `tests/rust-paths-check.bats` | | 0.2 | 0.0% | `tests/perf-gate.bats` | +| 0.2 | 0.0% | `tests/evaluator-io-check.bats` | +| 0.2 | 0.0% | `tests/token-bench-check.bats` | | 0.2 | 0.0% | `tests/remedy-payload-source.bats` | -| 0.2 | 0.0% | `tests/dist.bats` | -| 0.1 | 0.0% | `tests/evaluator-io-check.bats` | -| 0.1 | 0.0% | `tests/egress-check.bats` | +| 0.2 | 0.0% | `tests/test-bats-parallel.bats` | +| 0.1 | 0.0% | `tests/task-fail-closed.bats` | +| 0.1 | 0.0% | `tests/dist.bats` | | 0.1 | 0.0% | `tests/perf-pair.bats` | -| 0.1 | 0.0% | `tests/zizmor-split.bats` | -| 0.1 | 0.0% | `tests/darwin-link.bats` | | 0.1 | 0.0% | `tests/cross-check.bats` | +| 0.1 | 0.0% | `tests/darwin-link.bats` | +| 0.0 | 0.0% | `tests/zizmor-split.bats` | diff --git a/mise.toml b/mise.toml index b573c050a..f01786421 100644 --- a/mise.toml +++ b/mise.toml @@ -398,7 +398,7 @@ CI_FANIN_WORKFLOW = ".github/workflows/ci.yml" # which is a property of the world and belongs on a clock (`lock-complete`). REGORUS_OPA_COMPLIANCE = "1.2.0" REGORUS_OPA_COMPLIANCE_FOR = "0.11" -MUTANT_GATES = "alive,ci-slow-needed,bot-issue,land,land-lock,ci-lease-precondition,board-diff-overlap,reclaim-census,connector-allow-resolve,serena-mcp,target-prune,claimed-keys,released,in-progress-drain,merged-pr-keys,board-payloads,attestation-check,awk-regex-check,batten-glob-check,board-sweep,board-write-record,branch-age-check,cap-drift,checks-green,ci-drift,ci-local-parity,ci-tools-check,claim-check,claim-race-check,closing-key-check,prose-only-check,coderabbit-config-check,config-deprecations,config-lint,connector-allow-guard,connector-verb-guard,container-preflight,darwin-link,deferral-check,derived-check,digest-major-agreement,doctor,done-check,done-pr-check,duplicate-close-check,evaluator-closure-check,evaluator-io-check,fanout-guard,filed-here-check,finding-sink-check,gh-guard,graph-check,hook-matcher-check,hook-pin-check,hook-profile-check,hooks-wiring-check,install-check,land-divergence-assert,land-lock-check,landed-check,license-table-check,linear-check,lock-complete,macos-link-check,mcp-allow-check,mcp-attach-check,mcp-timeout-budget,memories-check,mise-action-floor,mise-pin-agreement,module-map-check,msrv-pin-agreement,mutant,mutant-census,no-doctests,nonverdict-assert,ntia-check,perf-assert,perf-compare,perf-gate,pipefail-grep-check,privileged-lane,pr-unsubscribed,publish-credential-check,ready-cites-check,ready-guard,ready-lint,reference-check,release-assets-check,release-due,release-tracking-check,renovate-config-validator,report-only-check,rules-drift,run-shape,run-shape-guard,rust-paths-check,sbom,sbom-check,schema-check,semver,signing-posture,skill-check,sonar-gate,spec-ref-check,stop-guard,stop-posture-check,suite-bench-check,timeout-check,token-bench-check,transcript-corpus-check,tree-clean,unlanded-check,verified" +MUTANT_GATES = "alive,ci-slow-needed,bot-issue,land,land-lock,ci-lease-precondition,board-diff-overlap,reclaim-census,connector-allow-resolve,serena-mcp,target-prune,claimed-keys,released,in-progress-drain,merged-pr-keys,board-payloads,attestation-check,awk-regex-check,batten-glob-check,board-sweep,board-write-record,branch-age-check,cap-drift,checks-green,ci-drift,ci-local-parity,ci-tools-check,claim-check,claim-race-check,closing-key-check,prose-only-check,coderabbit-config-check,config-deprecations,config-lint,connector-allow-guard,connector-verb-guard,container-preflight,darwin-link,deferral-check,derived-check,digest-major-agreement,doctor,done-check,done-pr-check,duplicate-close-check,evaluator-closure-check,evaluator-io-check,fanout-guard,filed-here-check,finding-sink-check,gh-guard,graph-check,hook-matcher-check,hook-pin-check,hook-profile-check,hooks-wiring-check,install-check,land-divergence-assert,land-lock-check,landed-check,license-table-check,linear-check,lock-complete,macos-link-check,mcp-allow-check,mcp-attach-check,mcp-timeout-budget,memories-check,mise-action-floor,mise-pin-agreement,module-map-check,msrv-pin-agreement,mutant,mutant-census,no-doctests,nonverdict-assert,ntia-check,perf-assert,perf-compare,perf-gate,pipefail-grep-check,privileged-lane,pr-unsubscribed,publish-credential-check,ready-cites-check,ready-guard,ready-lint,reference-check,release-assets-check,release-due,release-tracking-check,renovate-config-validator,report-only-check,review-answered,rules-drift,run-shape,run-shape-guard,rust-paths-check,sbom,sbom-check,schema-check,semver,signing-posture,skill-check,sonar-gate,spec-ref-check,stop-guard,stop-posture-check,suite-bench-check,timeout-check,token-bench-check,transcript-corpus-check,tree-clean,unlanded-check,verified" # --- GitHub reachability behind an egress proxy (Claude Code web sandbox etc.) --- # mise resolves every tool's release through GitHub's *API* host, api.github.com. diff --git a/policy/review-answered.rego b/policy/review-answered.rego new file mode 100644 index 000000000..7f60ebf5c --- /dev/null +++ b/policy/review-answered.rego @@ -0,0 +1,205 @@ +# The read-the-review gate (CLOUD-859), as a COUNT predicate over the one +# agent-sourced fact this repository declares. +# +# WHY IT IS WORTH REFUSING, measured rather than argued. Replayed over the 100 +# most recently merged pull requests (2026-08-19 to 2026-08-22) this predicate +# fires on 89 — 21 carrying no review from anyone but their own author, 68 +# carrying unresolved threads, 163 open threads across the fired set. Readying is +# the event that starts CI, and nothing in `land`'s pre-ready sequence asks about +# review, so an unreviewed head — or one carrying findings nobody answered — +# spends the matrix exactly as readily as a reviewed one. The worst instance is +# #617: reviewed at 15:39:44, merged twelve minutes later carrying three threads +# that are open to this day. Not an unreviewed merge, an unanswered one. +# +# THE COUNT IS THE PREDICATE, and that is a constraint rather than a shortcut. +# `facts::Sourced` stores `{command, seen_at, rows}` and no byte of the buffer — +# non-negotiable rule 4, structural rather than careful — so `reviews`, +# `latestReviews` and `reviewThreads` never reach Rego and a predicate over them +# is unwritable here. What reaches this module is HOW MANY rows the declared +# command's stdout carried. So the selection lives in the declared command's own +# `--jq` projection, one element per blocking condition, and `rows == 0` is +# exactly "reviewed and addressed". CLOUD-859's §2 specifies a predicate over the +# payload; it is not implementable on this channel, and the row records the +# reformulation rather than this file re-arguing it. +# +# WHICH IS ALSO WHY THE THREAD IDS ARE NOT IN THE MESSAGE. §5 asks for them. They +# are not in the engine, so the finding names the count and points at the command +# that produces the ids. A `msg` claiming to name them would be prose asserting a +# payload this channel refuses to carry, which is worse than the honest count. +# +# THE ABSENT RECORD IS NOT THIS MODULE'S, and the split is deliberate. +# `ready-needs-an-answered-review` is a `receipt` row over the same fact: a +# never-ran record, and one whose command does not match the declaration +# byte-for-byte, are `Validity::Missing` there — the deny that carries the +# `Fix::Run` asking for the command. Deciding it here as well would be two rows +# refusing one call with two messages, and only the receipt row's can name the +# command, because only it is handed the declaration. +# +# `--undo` IS NOT A READY. `land` re-drafts a PR on a red run, and that is the +# one thing that stops the next push buying another matrix. A predicate anchored +# on `gh pr ready` alone refuses the re-draft, which would leave the tap open on +# exactly the head this gate is trying to keep out of CI. +# METADATA +# description: | +# Bound to the mediated-call surface: this module is `scope = "mediated_call"`, +# so it reads `{call, facts}` and NOT the tree document. Binding it to the tree +# schema would type check it against a shape the engine never hands it, which is +# CLOUD-845's defect introduced on purpose rather than caught. +# THE BRACKETS ARE NOT STYLE: the schema file carries a hyphen, so the dotted +# form is a parse error reported as `invalid schema reference`. +# schemas: +# - input: schema["policy-call.schema"] +package batten.review_answered + +import rego.v1 + +rules contains "review-unanswered" + +# THE MUTATIONS, and each corrupts a conjunct no other conjunct already excludes +# — the discrimination `.claude/rules/policy-modules.md` records a survivor for, +# and the first draft of this block earned that warning twice. Field 3 is a bats +# `--filter` and must match a real case name: three descriptions that matched +# nothing all reported `case-already-red`, which is indistinguishable from a +# mutation nobody caught. `@` delimits each sed script because the rows are +# `|`-separated. +# +# TWO ROWS, NOT THREE, AND THE MISSING ONE IS THE INTERESTING PART. A +# `prose-judged` row corrupting the `startswith` anchor below was declared and +# SURVIVED, twice over: first as `command-not-anchored`, which named a case whose +# command carries no `ready` at all so the cheap conjunct excluded it, and then +# as a properly-named case that still could not see the change. The second +# survival is not a bad filter — it is a true statement about this module, and +# measured directly: the engine's `pattern` matcher is ANCHORED, so +# `ready-needs-an-answered-review` does not select +# `git commit -m "... gh pr ready ..."`, `agent_records` therefore projects no +# record for it, and `input.facts["agent-sourced"]` is null whatever this +# module's own narrowing says. No bats case can discriminate a conjunct the +# engine never gives an input reaching. So the row is deleted rather than +# retuned: a declared mutation that cannot be caught is a claim about coverage +# that is not true, which is what `mutant`'s own anti-vacuity terms exist to +# refuse. The anchor's discriminating case lives in the load-time tier below, +# where `with input as` CAN put a record beside a prose command. +#MUTANT count-not-decided|s@\trecord.rows > 0@\trecord.rows > 99999@|a head carrying unresolved threads is refused +#MUTANT redraft-judged|s@\tnot contains(input.call.command, "--undo")@\ttrue@|a re-draft is not a ready + +violation contains { + "rule": "review-unanswered", + "msg": sprintf( + "readying this PR would buy a CI matrix on a head carrying %d blocking review condition(s): an unresolved review thread, or no review from anyone but the author. Answer them first — resolve each thread, or force a review with `@coderabbitai full review`, which returns in ~3 minutes and costs no CI because the draft phase is the free phase — then re-run the declared command and retry. Replayed over the last 100 merges this fires on 89, so the common case is that there is something here to read. The thread ids are deliberately absent: the fact channel stores a COUNT and no byte of the buffer (non-negotiable rule 4), so the command that produced this number is also the only thing that can name them", + [record.rows], + ), +} if { + readying + record := input.facts["agent-sourced"]["review-answered"] + record.rows > 0 +} + +# The cheap term first, for `run-shape.rego`'s measured reason: everything else +# here is computed only if a `ready` appears in the command at all. +# +# `startswith` RATHER THAN `contains`, and it is DEFENCE IN DEPTH rather than the +# thing that keeps prose out. The distinction is measured, and the first version +# of this comment got it wrong in the flattering direction. +# +# The prose hazard is real in kind: this repository writes `gh pr ready` down +# constantly — in commit messages, in issue bodies, in this file — and +# `run-shape.rego`'s header records exactly this failure for its own predicate, +# answering it with a ~50-line heredoc-and-quote scrub. Copying that scrub here +# would be one concept in two spellings. +# +# But it is NOT reachable here, and the reason is one layer up. Probed directly +# against the real binary with only `ready-needs-an-answered-review` declared: +# +# gh pr ready 702 -> deny +# git commit -m "run gh pr ready once ... answered" -> allow +# +# The engine's `pattern` matcher is anchored rather than a substring, so that +# receipt row does not select the commit, `agent_records` projects no record for +# it, and `input.facts["agent-sourced"]` is null — this module decides nothing +# about prose whatever it says. So `startswith` buys one thing only: it keeps the +# module's correctness from DEPENDING on another row's pattern. Widen that +# `pattern` and a `contains` here would start judging commit messages; this does +# not. Cheap, and honest about being a second line rather than the first. +# +# THE FALSE NEGATIVE IT ACCEPTS, stated rather than absorbed: a compound +# `cd /repo && gh pr ready 702` is not judged HERE. The receipt row still selects +# it, so the did-you-look half holds and only the count half is missed. CLOUD-199 +# is why that direction is the right one: a guard with false positives gets +# bypassed, and then it guards nothing at all. +readying if { + contains(input.call.command, "ready") + startswith(trim_space(input.call.command), "gh pr ready") + not contains(input.call.command, "--undo") +} + +# --------------------------------------------------------------------------- +# The predicate's own tests (CLOUD-835). LOAD-TIME tier only: what proves this +# gate decides is `tests/review-answered.bats`, which drives the compiled binary +# over a real envelope AND a really-recorded fact, because a `with input as` case +# fabricates the very shape the engine may be unable to produce (CLOUD-845). +# --------------------------------------------------------------------------- + +# `rows` values, not fixtures: each is a real reading from the row's own replay. +# 4 is #623's open-thread count, 3 is #617's, 7 is #613's. +test_a_head_with_open_threads_is_refused if { + some v in violation with input as { + "call": {"command": "gh pr ready 623"}, + "facts": {"agent-sourced": {"review-answered": {"rows": 4}}}, + } + v.rule == "review-unanswered" +} + +test_a_head_with_every_thread_answered_is_left_alone if { + count(violation) == 0 with input as { + "call": {"command": "gh pr ready 620"}, + "facts": {"agent-sourced": {"review-answered": {"rows": 0}}}, + } +} + +# THE VACUITY TWIN, and the case the whole projection is shaped around. #618 +# carries no threads and no review. A predicate that counted only threads would +# read that as zero and pass it as "all addressed"; the declared command emits the +# PR author's login when nothing but the author reviewed, so the honest reading is +# one row and a refusal. +test_zero_threads_and_no_review_reads_as_unreviewed if { + some v in violation with input as { + "call": {"command": "gh pr ready 618"}, + "facts": {"agent-sourced": {"review-answered": {"rows": 1}}}, + } + v.rule == "review-unanswered" +} + +# `land` re-drafts on a red run, and that is what closes the CI tap. Refusing it +# would leave the tap open on the very head this gate is keeping out of CI. +test_a_redraft_is_not_a_ready if { + count(violation) == 0 with input as { + "call": {"command": "gh pr ready 623 --undo"}, + "facts": {"agent-sourced": {"review-answered": {"rows": 4}}}, + } +} + +test_another_gh_command_is_not_judged if { + count(violation) == 0 with input as { + "call": {"command": "gh pr view 623 --json reviewDecision"}, + "facts": {"agent-sourced": {"review-answered": {"rows": 4}}}, + } +} + +# THE PROSE CASE, and it is the one that discriminates the anchor. A `contains` +# over the raw string refuses this, and the message is a commit message rather +# than a call — the failure `run-shape.rego` records for its own predicate. +test_a_commit_message_naming_the_command_is_prose_not_a_ready if { + count(violation) == 0 with input as { + "call": {"command": "git commit -m \"run gh pr ready once the review is answered\""}, + "facts": {"agent-sourced": {"review-answered": {"rows": 4}}}, + } +} + +# An absent record is the receipt row's refusal, not this one's — so this module +# must be silent about it rather than adding a second message. +test_an_absent_record_is_the_receipt_rows_refusal if { + count(violation) == 0 with input as { + "call": {"command": "gh pr ready 623"}, + "facts": {"agent-sourced": null}, + } +} diff --git a/tests/review-answered.bats b/tests/review-answered.bats new file mode 100644 index 000000000..7c26d5fe7 --- /dev/null +++ b/tests/review-answered.bats @@ -0,0 +1,250 @@ +#!/usr/bin/env bats +# subject: policy/review-answered.rego +# +# The second test tier for the read-the-review gate (CLOUD-859), and the one that +# is not optional. The module's own `test_` rules pin the predicate; they cannot +# prove the ENGINE builds the input it reads, because a `with input as` case +# fabricates the very shape the engine may be unable to produce — the defect class +# `.claude/rules/policy-modules.md` records twice, both instances found by adding +# this tier rather than by reading. +# +# So every case here goes through TWO real hook calls in the order a session makes +# them: a `PostToolUse` envelope carrying the declared command and a buffer, which +# is what mints the record, and then a `PreToolUse` `gh pr ready`, which is what +# reads it. Nothing writes a receipt by hand. That is what makes this the tier +# that would have caught `record_agent_fact` calling `rows_in` where the row +# declared `json-array` — a live instance, in the same change. +# +# The fixture declares the REAL command string, byte for byte from `batten.toml`, +# so a rewording of it there fails here rather than silently ending the coupling. +# The engine never executes it — it compares `envelope.command` to the +# declaration and counts `envelope.result` — which is exactly what lets these +# cases cover shapes a live `gh` call could not be made to produce on demand. + +setup() { + load helpers + + # The same resolution chain `tests/run-shape.bats` uses, and for its measured + # reason: there is no release build when `test:bats` runs in CI, and a shorter + # chain took every case with it. + BIN="" + for candidate in \ + "${BATTEN_BIN:-}" \ + "$BATS_TEST_DIRNAME/../target/release/batten" \ + "$BATS_TEST_DIRNAME/../target/debug/batten"; do + [ -n "$candidate" ] && [ -x "$candidate" ] || continue + BIN="$candidate" + break + done + [ -n "$BIN" ] || BIN="$(command -v batten || true)" + [ -n "$BIN" ] || skip "no batten binary to drive" + + MODULE="$BATS_TEST_DIRNAME/../policy/review-answered.rego" + REPO="$BATS_TEST_TMPDIR/repo" + mkdir -p "$REPO/policy" + cp "$MODULE" "$REPO/policy/review-answered.rego" + + # THE DECLARED COMMAND, read out of this repository's own `batten.toml` rather + # than retyped. Byte-equality is the forgery control, so a copy here that + # drifted would leave every case passing over a string the real gate does not + # use — the coupling this suite exists to hold. + # + # ONE READER, not two. The first draft tried `batten config show --json` and + # fell back to a text read; the fallback had to cover the same ground anyway, + # so the first stage was a second way to get the same string and therefore a + # second way to get it wrong. It reads the row BY NAME rather than taking the + # first `[[fact]]` block, so adding a second fact row above this one cannot + # silently repoint every case in this file at the wrong command. + DECLARED=$( + python3 - "$BATS_TEST_DIRNAME/../batten.toml" review-answered <<'READER' +import re, sys + +text, want = open(sys.argv[1]).read(), sys.argv[2] +for block in text.split("[[fact]]")[1:]: + name = re.search(r'^name = "(.*)"$', block, re.M) + command = re.search(r'^command = "(.*)"$', block, re.M) + if name and command and name.group(1) == want: + print(command.group(1)) + break +READER + ) + [ -n "$DECLARED" ] || skip "no [[fact]] row named review-answered in batten.toml" + + { + echo "version = 1" + echo + echo "[[fact]]" + echo 'name = "review-answered"' + echo 'returns = "json-array"' + printf 'command = "%s"\n' "$DECLARED" + echo + echo "[[rule]]" + echo 'id = "ready-needs-an-answered-review"' + echo 'kind = "receipt"' + echo 'scope = "mediated_call"' + echo 'severity = "deny"' + echo 'pattern = "gh pr ready"' + echo 'checks = ["review-answered"]' + echo 'key = "head"' + echo 'reason = "run the declared command"' + echo + echo "[[rule]]" + echo 'id = "review-answered"' + echo 'kind = "policy"' + echo 'scope = "mediated_call"' + echo 'module = "policy/review-answered.rego"' + echo 'severity = "deny"' + } >"$REPO/batten.toml" + + # No global or system config: a contributor's own git settings must not be + # able to change a verdict here (CLOUD-282). + GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null git init -q -b main "$REPO" +} + +# Mint the record the way a session does: a PostToolUse envelope carrying the +# declared command and the buffer the host handed back. +record() { # record + local envelope + envelope=$(python3 -c 'import json,sys; print(json.dumps({"hook_event_name":"PostToolUse","session_id":"sess-review","cwd":"/repo","tool_name":"Bash","tool_input":{"command":sys.argv[1]},"tool_response":{"stdout":sys.argv[2],"stderr":""}}))' "$DECLARED" "$1") + (cd "$REPO" && printf '%s' "$envelope" | "$BIN" hook --harness claude-code) +} + +# Read it: the call the gate exists to judge. +ready() { # ready [] + local envelope + envelope=$(python3 -c 'import json,sys; print(json.dumps({"hook_event_name":"PreToolUse","tool_name":"Bash","tool_input":{"command":sys.argv[1]}}))' "gh pr ready 702${1:+ $1}") + (cd "$REPO" && printf '%s' "$envelope" | "$BIN" hook --harness claude-code) +} + +# BOTH HELPERS ASSERT THE EXIT STATUS, for `tests/run-shape.bats`' measured +# reason: `batten hook` prints nothing on an allow and exits 0 either way, so a +# substring check over an empty string is true — including the empty output of a +# binary that died before it judged anything. +denied() { [ "$status" -eq 0 ] && [[ "$1" == *'"permissionDecision":"deny"'* ]]; } +allowed() { [ "$status" -eq 0 ] && [[ "$1" != *'"deny"'* ]]; } + +# --- the two refusals, and which row owns each ------------------------------ + +@test "a ready with no record at all is refused, and the receipt row names the command" { + # The did-you-look half. The deny is built from the DECLARED command rather + # than from prose, which is the property that makes the remedy runnable. + run ready + denied "$output" + [[ "$output" == *"gh api graphql"* ]] + [[ "$output" == *"reviewThreads"* ]] +} + +@test "THE MEASURED SHAPE: a head carrying unresolved threads is refused, naming the count" { + # #623's four open threads, as the projection emits them: one element per + # thread id. + run record '["PRRT_a","PRRT_b","PRRT_c","PRRT_d"]' + [ "$status" -eq 0 ] + run ready + denied "$output" + [[ "$output" == *"review-unanswered"* ]] + [[ "$output" == *"4 blocking"* ]] + # Pointer-only (non-negotiable rule 4): the ids are not in the engine, so a + # message naming one would be a payload this channel refuses to carry. + [[ "$output" != *"PRRT_a"* ]] +} + +@test "a head whose threads are all answered is allowed" { + # THE LOAD-BEARING HALF. A predicate that only ever denied would satisfy every + # case above and gate nothing (CLOUD-418). `[]` is the genuine zero: the + # command looked and found none. + run record '[]' + [ "$status" -eq 0 ] + run ready + allowed "$output" +} + +# --- the vacuity cases the row enumerates ----------------------------------- + +@test "VACUITY: zero threads and no review reads as unreviewed, not as all-addressed" { + # #618 carries no threads and no review. The projection emits the PR author's + # login when nothing but the author reviewed, so the honest count is one — and + # a thread-only predicate would have read this as zero and passed it. + run record '["wenzowski"]' + [ "$status" -eq 0 ] + run ready + denied "$output" + [[ "$output" == *"1 blocking"* ]] +} + +@test "VACUITY: a buffer that is not the declared shape records nothing rather than one row" { + # `returns = "json-array"` (CLOUD-993). A `gh` that printed an auth error, or a + # wrapper that annotated its own output, must not become `rows 1` — which would + # be a refusal nobody can clear — nor `rows 0`, which would be a pass over a + # command that never answered. It records NOTHING, so the receipt row's + # did-you-look refusal stands. + run record 'gh: could not determine the current repository' + [ "$status" -eq 0 ] + run ready + denied "$output" + [[ "$output" == *"gh api graphql"* ]] +} + +@test "VACUITY: an empty buffer is not zero rows" { + # A command that printed nothing is could-not-look, not "there are none". + # Recording a zero here would turn silence into a pass. + run record '' + [ "$status" -eq 0 ] + run ready + denied "$output" +} + +@test "a buffer from a command nobody asked for never becomes the record" { + # The forgery control, over this fact: the agent chooses WHICH command runs and + # does not author what it prints, so byte-equality against the declaration is + # what stands between the two. + local envelope + envelope=$(python3 -c 'import json; print(json.dumps({"hook_event_name":"PostToolUse","session_id":"s","cwd":"/repo","tool_name":"Bash","tool_input":{"command":"echo []"},"tool_response":{"stdout":"[]","stderr":""}}))') + run bash -c "cd '$REPO' && printf '%s' '$envelope' | '$BIN' hook --harness claude-code" + [ "$status" -eq 0 ] + run ready + denied "$output" +} + +# --- what must NOT be refused ---------------------------------------------- + +@test "a re-draft is not a ready, even on a head carrying findings" { + # `land` re-drafts on a red run, and that is the one thing that stops the next + # push buying another matrix (CLOUD-240). Refusing it would leave the tap open + # on exactly the head this gate is keeping out of CI. + run record '["PRRT_a","PRRT_b"]' + [ "$status" -eq 0 ] + run ready '--undo' + allowed "$output" +} + +@test "a commit message naming the command is prose, not a ready" { + # THE ANCHOR'S DISCRIMINATING CASE. This repository writes `gh pr ready` down + # constantly — in commit messages, in issue bodies, in the module itself — so a + # `contains` over the raw command refuses its own documentation, which is the + # hazard `run-shape.rego`'s header records for the identical predicate. + # + # Over the binary rather than only in the module's own `test_` rules, because + # what is at risk is the engine handing the whole command string through: a + # `with input as` case fabricates that string and cannot show it arrives raw. + run record '["PRRT_a","PRRT_b"]' + [ "$status" -eq 0 ] + + local envelope + envelope=$(python3 -c 'import json,sys; print(json.dumps({"hook_event_name":"PreToolUse","tool_name":"Bash","tool_input":{"command":sys.argv[1]}}))' \ + 'git commit -m "run gh pr ready once the review is answered"') + run bash -c "cd '$REPO' && printf '%s' \"\$1\" | '$BIN' hook --harness claude-code" _ "$envelope" + allowed "$output" +} + +@test "reading the review is never refused, so the remedy is reachable" { + # A gate whose own remedy it blocks is unsatisfiable. Both `gh pr view` and the + # declared `gh api graphql` must pass on a head with findings recorded. + run record '["PRRT_a"]' + [ "$status" -eq 0 ] + local envelope + for c in 'gh pr view 702 --json reviewDecision' "$DECLARED"; do + envelope=$(python3 -c 'import json,sys; print(json.dumps({"hook_event_name":"PreToolUse","tool_name":"Bash","tool_input":{"command":sys.argv[1]}}))' "$c") + run bash -c "cd '$REPO' && printf '%s' \"\$1\" | '$BIN' hook --harness claude-code" _ "$envelope" + allowed "$output" + done +} From 228ba64cc2b98d1caf6503b7be8dbacabf55a75f Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Tue, 25 Aug 2026 19:07:51 +0000 Subject: [PATCH 2/3] fix(rules): an unread review page is a blocking condition, not a zero MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeRabbit's draft-phase review of this PR returned two Major findings in the gate's core. This fixes the first and records the second, which is real and larger than a config change. ## An unread page was a false green Both connections were `last:100`, and GitHub caps a page at 100. A PR with more review threads than that would have the surplus fall outside the query, so an unresolved thread out there left `rows == 0` and allowed the ready — a false green in the one direction this gate exists to prevent. Both connections now carry `pageInfo{hasNextPage}` and the projection emits an extra element when either is true, so "I could not see all of it" refuses instead of passing. Chosen over paginating deliberately: `Declared.command` is byte-compared, so it is one string with no loop in it, and `--paginate` emits one JSON document per page — which stops being a single array and lands back in the could-not-look arm. Counting a truncation as blocking reaches the safe verdict with no iteration at all. `true` rather than a quoted marker keeps the whole command free of double quotes, which is what lets it stay a plain TOML string with no escaping — one fewer way to retype a CLOUD-990-shaped remedy wrong. The discriminating pair is the new bats case beside "all answered": both are a head with zero unresolved threads, and only the truncated one refuses. Without the `pageInfo` clauses they are the same buffer. Verified live against this PR: the command returns the two real open threads and no truncation marker. ## The second finding is real, measured, and NOT fixed here The reviewer says `key = "head"` does not bind the evidence to a head. It does not, and probing the real binary confirms the consequence rather than inferring it: a record minted clear on one commit still allowed `gh pr ready` on the next with no new record. So the gate binds once per BRANCH, not once per head. It is not fixable in config. `facts::sourced_path` keys an agent-sourced record on the fact's name alone, and its own doc argues for that: a `claimed-key` answer is about an issue row and is deliberately shared across branches. So the fix is to make the record honour the key the row already declares — which changes the channel for every fact and contradicts a documented decision. That is CLOUD-1064, filed with the measurement, the tension it has to resolve, and an anti-vacuity twin (a `branch`-keyed fact must still survive a new commit, or `claim` starts demanding a re-claim per commit). Recorded beside the fact in `batten.toml` rather than left for a reader to find, because a gate that over-claims is worse than one with a stated limit: binding once per branch is strictly better than today's nothing and weaker than CLOUD-859's Acceptance, and both halves of that belong in the file. Refs: CLOUD-859, CLOUD-1064 --- batten.toml | 30 +++++++++++++++++++++++++++++- policy/review-answered.rego | 16 ++++++++++++++++ tests/review-answered.bats | 18 ++++++++++++++++++ 3 files changed, 63 insertions(+), 1 deletion(-) diff --git a/batten.toml b/batten.toml index 2c6970e7a..496ee4e1e 100644 --- a/batten.toml +++ b/batten.toml @@ -474,10 +474,38 @@ spent by then.""" # command that stops emitting an array — an auth error, a changed default format, # a wrapper that annotates — records NOTHING and denies, instead of recording a # plausible `rows 1` that no `rows == 0` predicate can ever clear. +# +# AN UNREAD PAGE IS A BLOCKING CONDITION, NOT A ZERO. GitHub caps a connection +# page at 100, so a PR with more than 100 review threads would have the surplus +# fall outside the query — and an unresolved thread out there would leave +# `rows == 0`, which is a FALSE GREEN in the one direction this gate exists to +# prevent. Both connections therefore carry `pageInfo{hasNextPage}` and the +# projection emits an extra element when either is true, so "I could not see all +# of it" refuses instead of passing. +# +# Chosen over paginating, deliberately: `Declared.command` is byte-compared, so +# it is one string with no loop in it, and `--paginate` emits one JSON document +# per page — which stops being a single array and lands back in the +# could-not-look arm. Counting a truncation as blocking gets the safe verdict +# with no iteration at all. `true` rather than a quoted marker keeps the whole +# command free of double quotes, which is what lets it be a plain TOML string +# with no escaping — one fewer way to retype a CLOUD-990-shaped remedy wrong. +# +# WHAT THIS FACT DOES NOT BIND, stated because the gate must not over-claim: +# **the record is not tied to the head it was taken on.** `facts::sourced_path` +# keys an agent-sourced record by the fact's NAME only, and its own doc says why +# — a `claimed-key` answer is about an issue row and is deliberately shared +# across branches. So `key = "head"` below is accepted and unread on this path, +# and it is measured rather than inferred: a record minted clear on one head +# still allowed `gh pr ready` on the next commit with no new record. The gate +# therefore binds once per branch rather than once per head, which is strictly +# better than today's nothing and weaker than the row's Acceptance. Fixing it +# changes the channel for every fact and contradicts a documented decision, so +# it is CLOUD-1064's rather than smuggled in here. [[fact]] name = "review-answered" returns = "json-array" -command = "gh api graphql -F id=$(gh pr view --json id --jq .id) -f query='query($id:ID!){node(id:$id){... on PullRequest{author{login} reviews(last:100){nodes{author{login}}} reviewThreads(last:100){nodes{id isResolved}}}}}' --jq '.data.node as $p | [($p.reviewThreads.nodes[] | select(.isResolved == false) | .id), (if ([$p.reviews.nodes[] | select(.author.login != $p.author.login)] | length) == 0 then $p.author.login else empty end)]'" +command = "gh api graphql -F id=$(gh pr view --json id --jq .id) -f query='query($id:ID!){node(id:$id){... on PullRequest{author{login} reviews(first:100){pageInfo{hasNextPage} nodes{author{login}}} reviewThreads(first:100){pageInfo{hasNextPage} nodes{id isResolved}}}}}' --jq '.data.node as $p | [($p.reviewThreads.nodes[] | select(.isResolved == false) | .id), (if ([$p.reviews.nodes[] | select(.author.login != $p.author.login)] | length) == 0 then $p.author.login else empty end), (if $p.reviewThreads.pageInfo.hasNextPage then true else empty end), (if $p.reviews.pageInfo.hasNextPage then true else empty end)]'" # The DID-YOU-LOOK half. A `receipt` row is also what makes the fact reach Rego # at all: `required_checks_for` reads the receipt rows, and `agent_records` diff --git a/policy/review-answered.rego b/policy/review-answered.rego index 7f60ebf5c..fe89b819e 100644 --- a/policy/review-answered.rego +++ b/policy/review-answered.rego @@ -185,6 +185,22 @@ test_another_gh_command_is_not_judged if { } } +# AN UNREAD PAGE COUNTS. GitHub caps a connection page at 100, so the declared +# command emits an extra element when either connection reports `hasNextPage` — +# "I could not see all of it" has to refuse rather than pass, because an +# unresolved thread beyond the page would otherwise leave `rows == 0`, which is a +# false green in the one direction this gate exists to prevent. Nothing about that +# is visible to the module, which sees only a count; this case pins that a +# truncation-inflated count still refuses, so a future reader cannot "simplify" +# the projection's last two clauses away as noise. +test_a_truncated_page_still_refuses_because_it_is_counted if { + some v in violation with input as { + "call": {"command": "gh pr ready 705"}, + "facts": {"agent-sourced": {"review-answered": {"rows": 1}}}, + } + v.rule == "review-unanswered" +} + # THE PROSE CASE, and it is the one that discriminates the anchor. A `contains` # over the raw string refuses this, and the message is a commit message rather # than a call — the failure `run-shape.rego` records for its own predicate. diff --git a/tests/review-answered.bats b/tests/review-answered.bats index 7c26d5fe7..f2b31488a 100644 --- a/tests/review-answered.bats +++ b/tests/review-answered.bats @@ -217,6 +217,24 @@ allowed() { [ "$status" -eq 0 ] && [[ "$1" != *'"deny"'* ]]; } allowed "$output" } +@test "VACUITY: a page the command could not read refuses rather than passing" { + # GitHub caps a connection page at 100, so a PR with more threads than that + # would have the surplus fall outside the query — and an unresolved thread out + # there would leave `rows == 0`, a FALSE GREEN in the one direction this gate + # exists to prevent. The projection emits an extra element per truncated + # connection, so the buffer a clear-but-truncated head produces is `[true]` + # rather than `[]`. + # + # THE DISCRIMINATING PAIR is this case beside "all answered": both are a head + # with zero unresolved threads, and only the truncated one refuses. Without the + # `pageInfo` clauses they would be the same buffer. + run record '[true]' + [ "$status" -eq 0 ] + run ready + denied "$output" + [[ "$output" == *"1 blocking"* ]] +} + @test "a commit message naming the command is prose, not a ready" { # THE ANCHOR'S DISCRIMINATING CASE. This repository writes `gh pr ready` down # constantly — in commit messages, in issue bodies, in the module itself — so a From 033484e78dba6e7084c72de68ed92a2220220123 Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Tue, 25 Aug 2026 19:14:49 +0000 Subject: [PATCH 3/3] fix(rules): drop the anchor that opened a bypass, and judge a compound ready MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The second draft-phase review pass returned two more Major findings. One is mine and is a live bypass; the other is pre-existing and filed. A third was found by this gate's sibling refusing the commit that documented the first two. ## The anchor was wrong twice over, and both halves are measured `readying` narrowed with an anchored `startswith`, added last round to keep PROSE out — a commit message naming the readying command, which this repository writes constantly. Probing the real binary refutes both sides of that reasoning: It bought almost nothing. A `-m` quoted message is allowed even under `contains`, because a quoted span is scrubbed before the pattern matcher sees it: the receipt row does not select that commit, no record is projected, and the module decides nothing about it. It opened a bypass. A compound `cd /repo && …` DOES select the receipt row, so an existing record satisfies the did-you-look half — and the anchor then made this module silent, allowing the call with two unresolved threads recorded. The previous comment called that "only the count half is missed", which was wrong: the count half IS the gate. So the anchor traded a hazard the engine mostly handles for a real hole. Back to `contains`, with the measurement in the module instead of the reasoning. The module's prose test rule goes with it, and its absence is now stated: a fabricated input can hand this module a record beside a commit message, which the engine never does, so asserting over that pins a property of the fixture — and the anchor it justified is what opened the bypass. The real behaviour stays asserted end to end, over the binary, where the engine's selection decides. `compound-not-judged` replaces the mutation that could not be caught. It took three attempts: the first named a case the cheap conjunct excluded, the second was named correctly and survived anyway because no case can discriminate a conjunct the engine never feeds. Reinstating the anchor is a mutation a case CAN see, because the anchor is the defect. ## Two findings that are not this branch's, both filed The undo flag is refused by the receipt row before the module's exemption can apply. Confirmed against main's own config rather than a fixture: the landed row, unrelated to this branch, refuses it too. That is CLOUD-237's fix lost when CLOUD-312 ported the bash guard into config, and a pattern cannot express "this shape except that flag" because the regex crate has no lookahead. Latent rather than loud, because the re-draft runs inside the landing task and never reaches the hook. CLOUD-1065. And the residue on the prose measurement above: a heredoc body is NOT scrubbed, so a commit whose message names the readying command IS selected. Measured the hard way — the landed receipt row refused the commit that wrote this very message, printing a remedy about verify receipts that had nothing to do with what the caller was doing. The `-m` probe alone read as "prose is excluded" and was wrong. CLOUD-1066, against the kind rather than either row, because the fix is in what a pattern is matched against. Refs: CLOUD-859, CLOUD-1065, CLOUD-1066 --- policy/review-answered.rego | 120 +++++++++++++++++++++--------------- tests/review-answered.bats | 20 ++++++ 2 files changed, 92 insertions(+), 48 deletions(-) diff --git a/policy/review-answered.rego b/policy/review-answered.rego index fe89b819e..5ab13c40a 100644 --- a/policy/review-answered.rego +++ b/policy/review-answered.rego @@ -63,22 +63,21 @@ rules contains "review-unanswered" # mutation nobody caught. `@` delimits each sed script because the rows are # `|`-separated. # -# TWO ROWS, NOT THREE, AND THE MISSING ONE IS THE INTERESTING PART. A -# `prose-judged` row corrupting the `startswith` anchor below was declared and -# SURVIVED, twice over: first as `command-not-anchored`, which named a case whose -# command carries no `ready` at all so the cheap conjunct excluded it, and then -# as a properly-named case that still could not see the change. The second -# survival is not a bad filter — it is a true statement about this module, and -# measured directly: the engine's `pattern` matcher is ANCHORED, so -# `ready-needs-an-answered-review` does not select -# `git commit -m "... gh pr ready ..."`, `agent_records` therefore projects no -# record for it, and `input.facts["agent-sourced"]` is null whatever this -# module's own narrowing says. No bats case can discriminate a conjunct the -# engine never gives an input reaching. So the row is deleted rather than -# retuned: a declared mutation that cannot be caught is a claim about coverage -# that is not true, which is what `mutant`'s own anti-vacuity terms exist to -# refuse. The anchor's discriminating case lives in the load-time tier below, -# where `with input as` CAN put a record beside a prose command. +# THE THIRD ROW TOOK THREE ATTEMPTS AND THE FIRST TWO ARE THE LESSON. It began as +# `command-not-anchored`, naming a case whose command carries no `ready` at all, +# so the cheap conjunct excluded the input and the mutation could not be seen. It +# was renamed to `prose-judged` — correctly named, and it SURVIVED anyway, because +# the engine never hands this module a prose command with a record beside it, so +# no bats case can discriminate that conjunct at all. Both survivals were true +# statements about the module rather than bad filters. +# +# What finally made it discriminating was deleting the conjunct. `compound-not-judged` +# reinstates the anchor those rows were defending, and the anchor is exactly what +# opened the bypass below — so the mutation now corrupts something a case CAN see: +# `cd /repo && gh pr ready` goes from refused to allowed. A mutation that cannot +# be caught is a coverage claim that is not true; a mutation that reintroduces a +# measured defect is the opposite. +#MUTANT compound-not-judged|s@\tcontains(input.call.command, "gh pr ready")@\tstartswith(trim_space(input.call.command), "gh pr ready")@|a compound command is still a ready #MUTANT count-not-decided|s@\trecord.rows > 0@\trecord.rows > 99999@|a head carrying unresolved threads is refused #MUTANT redraft-judged|s@\tnot contains(input.call.command, "--undo")@\ttrue@|a re-draft is not a ready @@ -97,38 +96,51 @@ violation contains { # The cheap term first, for `run-shape.rego`'s measured reason: everything else # here is computed only if a `ready` appears in the command at all. # -# `startswith` RATHER THAN `contains`, and it is DEFENCE IN DEPTH rather than the -# thing that keeps prose out. The distinction is measured, and the first version -# of this comment got it wrong in the flattering direction. +# `contains` RATHER THAN AN ANCHOR, and this reversed twice before it was right. +# +# The draft before this one used `startswith(trim_space(...))`, on the reasoning +# that an anchor keeps PROSE out — a commit message naming `gh pr ready`, which +# this repository writes constantly, and which `run-shape.rego`'s header records +# as a real failure for its own predicate. That anchor was wrong twice over, both +# measured against the real binary rather than argued: # -# The prose hazard is real in kind: this repository writes `gh pr ready` down -# constantly — in commit messages, in issue bodies, in this file — and -# `run-shape.rego`'s header records exactly this failure for its own predicate, -# answering it with a ~50-line heredoc-and-quote scrub. Copying that scrub here -# would be one concept in two spellings. +# 1. **It bought almost nothing, and the "almost" is measured too.** With only +# `ready-needs-an-answered-review` declared, +# `git commit -m "run gh pr ready once ... answered"` is ALLOWED even under +# `contains`: a quoted span is scrubbed before the engine's `pattern` matcher +# sees it, so the receipt row does not select that commit, no record is +# projected, and this module decides nothing about it. # -# But it is NOT reachable here, and the reason is one layer up. Probed directly -# against the real binary with only `ready-needs-an-answered-review` declared: +# A HEREDOC IS NOT SCRUBBED, and that is the residue. `git commit -F - < deny -# git commit -m "run gh pr ready once ... answered" -> allow +# 2. **It opened a BYPASS.** `cd /repo && gh pr ready 702` does select the +# receipt row, so an existing record satisfies the did-you-look half — and the +# anchor then made this module silent, so the call was ALLOWED with two +# unresolved threads recorded. Measured exactly that. The earlier comment here +# called it "only the count half is missed", which was wrong: the count half +# IS the gate, so missing it is the whole gate. # -# The engine's `pattern` matcher is anchored rather than a substring, so that -# receipt row does not select the commit, `agent_records` projects no record for -# it, and `input.facts["agent-sourced"]` is null — this module decides nothing -# about prose whatever it says. So `startswith` buys one thing only: it keeps the -# module's correctness from DEPENDING on another row's pattern. Widen that -# `pattern` and a `contains` here would start judging commit messages; this does -# not. Cheap, and honest about being a second line rather than the first. +# So the anchor traded a hazard the engine already handles for a live hole. A +# substring over the command has no such hole, and the prose it would judge never +# arrives. CLOUD-199's lesson still applies and points the other way here: the +# false positive this was defending against does not exist, so paying a false +# negative for it was pure loss. # -# THE FALSE NEGATIVE IT ACCEPTS, stated rather than absorbed: a compound -# `cd /repo && gh pr ready 702` is not judged HERE. The receipt row still selects -# it, so the did-you-look half holds and only the count half is missed. CLOUD-199 -# is why that direction is the right one: a guard with false positives gets -# bypassed, and then it guards nothing at all. +# The scrub `run-shape.rego` uses is the answer if prose ever DOES reach this +# module — which would mean the receipt row's `pattern` had been widened. That is +# a change somebody makes deliberately, and it is where the ~50 lines earn their +# place; until then copying them is one concept in two spellings. readying if { contains(input.call.command, "ready") - startswith(trim_space(input.call.command), "gh pr ready") + contains(input.call.command, "gh pr ready") not contains(input.call.command, "--undo") } @@ -201,16 +213,28 @@ test_a_truncated_page_still_refuses_because_it_is_counted if { v.rule == "review-unanswered" } -# THE PROSE CASE, and it is the one that discriminates the anchor. A `contains` -# over the raw string refuses this, and the message is a commit message rather -# than a call — the failure `run-shape.rego` records for its own predicate. -test_a_commit_message_naming_the_command_is_prose_not_a_ready if { - count(violation) == 0 with input as { - "call": {"command": "git commit -m \"run gh pr ready once the review is answered\""}, - "facts": {"agent-sourced": {"review-answered": {"rows": 4}}}, +# A COMPOUND COMMAND IS STILL A READY, and this is the case that was missing when +# the anchor was here. `cd /repo && gh pr ready 702` selects the receipt row, so +# an existing record satisfies the did-you-look half — and under an anchor this +# module went silent, allowing the call with unresolved threads recorded. The +# count half IS the gate, so a spelling that skips it skips everything. +test_a_compound_command_is_still_a_ready if { + some v in violation with input as { + "call": {"command": "cd /repo && gh pr ready 702"}, + "facts": {"agent-sourced": {"review-answered": {"rows": 2}}}, } + v.rule == "review-unanswered" } +# NO PROSE CASE LIVES HERE, deliberately, and its absence is the honest reading. +# A `with input as` case can hand this module a record beside a commit message, +# which the ENGINE never does — the receipt row's anchored `pattern` does not +# select a commit, so no record is projected for one. Asserting prose is allowed +# here would therefore pin a property of a fabricated input rather than of the +# system, and the anchor it justified turned out to open a bypass. The real +# behaviour is asserted end to end in `tests/review-answered.bats`, over the +# binary, where the engine's own selection is what decides. + # An absent record is the receipt row's refusal, not this one's — so this module # must be silent about it rather than adding a second message. test_an_absent_record_is_the_receipt_rows_refusal if { diff --git a/tests/review-answered.bats b/tests/review-answered.bats index f2b31488a..3eaec3a83 100644 --- a/tests/review-answered.bats +++ b/tests/review-answered.bats @@ -235,6 +235,26 @@ allowed() { [ "$status" -eq 0 ] && [[ "$1" != *'"deny"'* ]]; } [[ "$output" == *"1 blocking"* ]] } +@test "THE BYPASS: a compound command is still a ready" { + # The case an earlier draft did not have, and the reason it did not: this module + # anchored on `startswith`, so `cd /repo && gh pr ready 702` went unjudged. The + # receipt row DOES select it, so an existing record satisfies the did-you-look + # half — and with the count half silent the call was allowed carrying two + # unresolved threads. Measured exactly that before the anchor came out. + # + # End to end rather than only in the module's `test_` rules, because what was + # wrong was the interaction between two rows: the receipt row's selection and + # this module's narrowing disagreeing about the same command. + run record '["PRRT_a","PRRT_b"]' + [ "$status" -eq 0 ] + + local envelope + envelope=$(python3 -c 'import json,sys; print(json.dumps({"hook_event_name":"PreToolUse","tool_name":"Bash","tool_input":{"command":sys.argv[1]}}))' 'cd /repo && gh pr ready 702') + run bash -c "cd '$REPO' && printf '%s' \"\$1\" | '$BIN' hook --harness claude-code" _ "$envelope" + denied "$output" + [[ "$output" == *"2 blocking"* ]] +} + @test "a commit message naming the command is prose, not a ready" { # THE ANCHOR'S DISCRIMINATING CASE. This repository writes `gh pr ready` down # constantly — in commit messages, in issue bodies, in the module itself — so a