diff --git a/batten.toml b/batten.toml index c90f2a4f4..8551fcecb 100644 --- a/batten.toml +++ b/batten.toml @@ -491,17 +491,13 @@ spent by then.""" # 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. +# WHAT THIS FACT BINDS, and it is the head rather than the branch. The record is +# filed under the subject `key = "head"` names below, so a commit pushed after a +# clear answer leaves the record under the previous head and the next `gh pr +# ready` is refused. That was NOT true when this row landed — `sourced_path` keyed +# on the fact's name alone, so the column was accepted and unread and the gate +# bound once per branch — and the limitation comment that stood here is deleted in +# the change that removed it (CLOUD-859). [[fact]] name = "review-answered" returns = "json-array" @@ -518,10 +514,11 @@ command = "gh api graphql -F id=$(gh pr view --json id --jq .id) -f query='query # 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. +# `key = "head"` and it DECIDES (CLOUD-859): an agent-sourced record is resolved +# from its own file rather than from the receipt store, and that file is named for +# the subject this column resolves to. `head` is the honest answer for a fact +# about a specific head — a review is of one set of bytes — and `branch` is what a +# fact about the WORK takes, which is why `claim-needs-receipt` below takes it. [[rule]] id = "ready-needs-an-answered-review" kind = "receipt" diff --git a/bench/suites/RESULTS.md b/bench/suites/RESULTS.md index ff77db8ca..97548bd58 100644 --- a/bench/suites/RESULTS.md +++ b/bench/suites/RESULTS.md @@ -6,167 +6,168 @@ 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: 159 -- serial total: 1382.6s +- suites: 160 +- serial total: 1088.8s | seconds | share | suite | | ---: | ---: | --- | -| 158.4 | 11.5% | `tests/derived-check.bats` | -| 153.1 | 11.1% | `tests/land-lock.bats` | -| 105.9 | 7.7% | `tests/session-start.bats` | -| 102.6 | 7.4% | `tests/ci-wait.bats` | -| 95.7 | 6.9% | `tests/land.bats` | -| 77.0 | 5.6% | `tests/hooks-wiring-check.bats` | -| 50.1 | 3.6% | `tests/ci-local-parity.bats` | -| 38.5 | 2.8% | `tests/prebuilt-lint.bats` | -| 36.1 | 2.6% | `tests/helpers.bats` | -| 34.6 | 2.5% | `tests/main-watch.bats` | -| 28.2 | 2.0% | `tests/commit-convention.bats` | -| 24.7 | 1.8% | `tests/sbom-check.bats` | -| 24.4 | 1.8% | `tests/hook-latency-drift.bats` | -| 22.8 | 1.6% | `tests/graph-check.bats` | -| 21.4 | 1.6% | `tests/claim-check.bats` | -| 19.7 | 1.4% | `tests/config-deprecations.bats` | -| 17.6 | 1.3% | `tests/token-bench.bats` | -| 17.5 | 1.3% | `tests/board-diff-overlap.bats` | -| 16.8 | 1.2% | `tests/board-write-record.bats` | -| 15.3 | 1.1% | `tests/config-lint.bats` | -| 11.8 | 0.9% | `tests/run-shape-guard.bats` | -| 9.5 | 0.7% | `tests/filed-here-check.bats` | -| 9.5 | 0.7% | `tests/ready-lint.bats` | -| 9.5 | 0.7% | `tests/board-sweep.bats` | -| 9.0 | 0.6% | `tests/target-race.bats` | -| 8.9 | 0.6% | `tests/renovate-config-validator.bats` | -| 8.8 | 0.6% | `tests/replay.bats` | -| 8.6 | 0.6% | `tests/stop-guard.bats` | -| 8.5 | 0.6% | `tests/released.bats` | -| 8.3 | 0.6% | `tests/mutant.bats` | -| 7.6 | 0.6% | `tests/ready-guard.bats` | -| 7.0 | 0.5% | `tests/reference-check.bats` | -| 6.8 | 0.5% | `tests/lock-complete.bats` | -| 6.5 | 0.5% | `tests/release-tracking-check.bats` | -| 6.4 | 0.5% | `tests/sbom.bats` | -| 6.2 | 0.5% | `tests/mcp-allow-check.bats` | -| 6.2 | 0.4% | `tests/step-receipt.bats` | -| 5.8 | 0.4% | `tests/task-registry.bats` | -| 5.4 | 0.4% | `tests/in-progress-drain.bats` | -| 5.2 | 0.4% | `tests/schema-check.bats` | -| 5.0 | 0.4% | `tests/release-assets-check.bats` | -| 4.7 | 0.3% | `tests/hk-selection.bats` | -| 4.6 | 0.3% | `tests/singleton.bats` | -| 4.6 | 0.3% | `tests/ready-cites-check.bats` | -| 4.5 | 0.3% | `tests/land-divergence.bats` | -| 4.4 | 0.3% | `tests/ntia-check.bats` | -| 4.2 | 0.3% | `tests/target-ensure.bats` | -| 4.1 | 0.3% | `tests/rules-drift.bats` | -| 3.8 | 0.3% | `tests/suite-select.bats` | -| 3.7 | 0.3% | `tests/doctor-race.bats` | -| 3.6 | 0.3% | `tests/semver.bats` | -| 3.6 | 0.3% | `tests/pre-commit-staging.bats` | -| 3.4 | 0.2% | `tests/landed-check.bats` | -| 3.1 | 0.2% | `tests/with-lock.bats` | -| 3.0 | 0.2% | `tests/closing-key-check.bats` | -| 2.8 | 0.2% | `tests/unlanded-check.bats` | -| 2.7 | 0.2% | `tests/claim-race-check.bats` | -| 2.7 | 0.2% | `tests/tree-clean.bats` | -| 2.6 | 0.2% | `tests/review-answered.bats` | -| 2.5 | 0.2% | `tests/skill-check.bats` | -| 2.5 | 0.2% | `tests/target-prune.bats` | -| 2.4 | 0.2% | `tests/finding-sink-check.bats` | -| 2.4 | 0.2% | `tests/timeout-drift.bats` | -| 2.3 | 0.2% | `tests/spec-ref-check.bats` | -| 2.3 | 0.2% | `tests/ci-tools-check.bats` | -| 2.2 | 0.2% | `tests/signing-posture.bats` | -| 2.2 | 0.2% | `tests/bot-issue.bats` | +| 143.4 | 13.2% | `tests/land-lock.bats` | +| 122.0 | 11.2% | `tests/derived-check.bats` | +| 102.4 | 9.4% | `tests/ci-wait.bats` | +| 92.5 | 8.5% | `tests/land.bats` | +| 59.8 | 5.5% | `tests/hooks-wiring-check.bats` | +| 43.5 | 4.0% | `tests/ci-local-parity.bats` | +| 34.8 | 3.2% | `tests/main-watch.bats` | +| 26.2 | 2.4% | `tests/helpers.bats` | +| 24.3 | 2.2% | `tests/hook-latency-drift.bats` | +| 22.3 | 2.0% | `tests/sbom-check.bats` | +| 20.8 | 1.9% | `tests/graph-check.bats` | +| 19.2 | 1.8% | `tests/claim-check.bats` | +| 19.0 | 1.7% | `tests/prebuilt-lint.bats` | +| 15.8 | 1.5% | `tests/board-diff-overlap.bats` | +| 15.1 | 1.4% | `tests/board-write-record.bats` | +| 12.0 | 1.1% | `tests/config-lint.bats` | +| 10.4 | 1.0% | `tests/token-bench.bats` | +| 9.7 | 0.9% | `tests/run-shape-guard.bats` | +| 9.5 | 0.9% | `tests/ready-lint.bats` | +| 9.4 | 0.9% | `tests/target-race.bats` | +| 9.3 | 0.9% | `tests/released.bats` | +| 8.8 | 0.8% | `tests/filed-here-check.bats` | +| 8.5 | 0.8% | `tests/board-sweep.bats` | +| 8.2 | 0.8% | `tests/stop-guard.bats` | +| 7.6 | 0.7% | `tests/renovate-config-validator.bats` | +| 7.5 | 0.7% | `tests/release-tracking-check.bats` | +| 7.1 | 0.7% | `tests/mutant.bats` | +| 7.1 | 0.7% | `tests/ready-guard.bats` | +| 7.0 | 0.6% | `tests/step-receipt.bats` | +| 7.0 | 0.6% | `tests/mcp-allow-check.bats` | +| 6.7 | 0.6% | `tests/session-start.bats` | +| 6.2 | 0.6% | `tests/lock-complete.bats` | +| 6.2 | 0.6% | `tests/replay.bats` | +| 5.5 | 0.5% | `tests/sbom.bats` | +| 5.4 | 0.5% | `tests/release-assets-check.bats` | +| 5.3 | 0.5% | `tests/singleton.bats` | +| 5.3 | 0.5% | `tests/in-progress-drain.bats` | +| 5.2 | 0.5% | `tests/semver.bats` | +| 4.7 | 0.4% | `tests/land-divergence.bats` | +| 4.5 | 0.4% | `tests/schema-check.bats` | +| 4.5 | 0.4% | `tests/pre-commit-staging.bats` | +| 4.4 | 0.4% | `tests/task-registry.bats` | +| 4.3 | 0.4% | `tests/ready-cites-check.bats` | +| 4.1 | 0.4% | `tests/ntia-check.bats` | +| 3.8 | 0.3% | `tests/doctor-race.bats` | +| 3.6 | 0.3% | `tests/target-ensure.bats` | +| 3.6 | 0.3% | `tests/reference-check.bats` | +| 3.4 | 0.3% | `tests/hk-selection.bats` | +| 3.2 | 0.3% | `tests/with-lock.bats` | +| 3.0 | 0.3% | `tests/suite-select.bats` | +| 2.8 | 0.3% | `tests/skill-check.bats` | +| 2.7 | 0.2% | `tests/landed-check.bats` | +| 2.7 | 0.2% | `tests/rules-drift.bats` | +| 2.5 | 0.2% | `tests/config-deprecations.bats` | +| 2.5 | 0.2% | `tests/closing-key-check.bats` | +| 2.4 | 0.2% | `tests/spec-ref-check.bats` | +| 2.3 | 0.2% | `tests/signing-posture.bats` | +| 2.2 | 0.2% | `tests/claim-race-check.bats` | | 2.2 | 0.2% | `tests/alive.bats` | -| 2.1 | 0.1% | `tests/claimed-keys.bats` | -| 2.0 | 0.1% | `tests/ci-slow-needed.bats` | -| 1.9 | 0.1% | `tests/run-shape.bats` | -| 1.8 | 0.1% | `tests/reclaim-census.bats` | -| 1.7 | 0.1% | `tests/ci-lease-precondition.bats` | -| 1.7 | 0.1% | `tests/awk-regex-check.bats` | -| 1.7 | 0.1% | `tests/ready-lint-deferral.bats` | -| 1.7 | 0.1% | `tests/verify.bats` | -| 1.7 | 0.1% | `tests/memories-check.bats` | -| 1.5 | 0.1% | `tests/mutant-census.bats` | -| 1.5 | 0.1% | `tests/spawn-census.bats` | -| 1.4 | 0.1% | `tests/perf-record.bats` | +| 2.2 | 0.2% | `tests/unlanded-check.bats` | +| 2.1 | 0.2% | `tests/finding-sink-check.bats` | +| 2.1 | 0.2% | `tests/bot-issue.bats` | +| 2.1 | 0.2% | `tests/ci-tools-check.bats` | +| 1.9 | 0.2% | `tests/ci-slow-needed.bats` | +| 1.9 | 0.2% | `tests/reclaim-census.bats` | +| 1.9 | 0.2% | `tests/verify.bats` | +| 1.8 | 0.2% | `tests/review-answered.bats` | +| 1.8 | 0.2% | `tests/claimed-keys.bats` | +| 1.8 | 0.2% | `tests/ci-lease-precondition.bats` | +| 1.7 | 0.2% | `tests/tree-clean.bats` | +| 1.7 | 0.2% | `tests/land-divergence-assert.bats` | +| 1.7 | 0.2% | `tests/awk-regex-check.bats` | +| 1.6 | 0.1% | `tests/spawn-census.bats` | +| 1.6 | 0.1% | `tests/install-check.bats` | +| 1.6 | 0.1% | `tests/mutant-census.bats` | +| 1.6 | 0.1% | `tests/ready-lint-deferral.bats` | +| 1.5 | 0.1% | `tests/memories-check.bats` | +| 1.5 | 0.1% | `tests/run-shape.bats` | | 1.4 | 0.1% | `tests/deferral-check.bats` | -| 1.3 | 0.1% | `tests/land-divergence-assert.bats` | | 1.3 | 0.1% | `tests/done-check.bats` | -| 1.3 | 0.1% | `tests/install-check.bats` | -| 1.3 | 0.1% | `tests/linear-check.bats` | -| 1.3 | 0.1% | `tests/nonverdict-scan.bats` | +| 1.3 | 0.1% | `tests/suite-bench-check.bats` | +| 1.3 | 0.1% | `tests/perf-record.bats` | +| 1.3 | 0.1% | `tests/release-backfill.bats` | +| 1.2 | 0.1% | `tests/linear-check.bats` | | 1.2 | 0.1% | `tests/attestation-check.bats` | -| 1.2 | 0.1% | `tests/checks-green.bats` | -| 1.2 | 0.1% | `tests/perf-assert.bats` | -| 1.1 | 0.1% | `tests/release-backfill.bats` | -| 1.1 | 0.1% | `tests/gh-guard.bats` | -| 1.1 | 0.1% | `tests/transcript-corpus-check.bats` | -| 1.1 | 0.1% | `tests/done-pr-check.bats` | -| 1.1 | 0.1% | `tests/lint-deno.bats` | -| 1.0 | 0.1% | `tests/prose-only-check.bats` | -| 1.0 | 0.1% | `tests/sbom-binary.bats` | -| 1.0 | 0.1% | `tests/module-map-check.bats` | -| 1.0 | 0.1% | `tests/doctor.bats` | -| 1.0 | 0.1% | `tests/verified.bats` | -| 1.0 | 0.1% | `tests/render-cli.bats` | -| 0.9 | 0.1% | `tests/pr-unsubscribed.bats` | -| 0.9 | 0.1% | `tests/evaluator-closure-check.bats` | -| 0.9 | 0.1% | `tests/hook-matcher-check.bats` | -| 0.9 | 0.1% | `tests/suite-bench-check.bats` | +| 1.2 | 0.1% | `tests/nonverdict-scan.bats` | +| 1.1 | 0.1% | `tests/target-prune.bats` | +| 1.1 | 0.1% | `tests/perf-assert.bats` | +| 1.0 | 0.1% | `tests/pr-unsubscribed.bats` | +| 1.0 | 0.1% | `tests/checks-green.bats` | +| 1.0 | 0.1% | `tests/gh-guard.bats` | +| 0.9 | 0.1% | `tests/prose-only-check.bats` | +| 0.9 | 0.1% | `tests/lint-deno.bats` | +| 0.9 | 0.1% | `tests/module-map-check.bats` | +| 0.9 | 0.1% | `tests/render-cli.bats` | +| 0.9 | 0.1% | `tests/timeout-drift.bats` | +| 0.9 | 0.1% | `tests/doctor.bats` | +| 0.9 | 0.1% | `tests/done-pr-check.bats` | +| 0.8 | 0.1% | `tests/install.bats` | | 0.8 | 0.1% | `tests/stop-posture-check.bats` | -| 0.8 | 0.1% | `tests/perf-compare.bats` | +| 0.8 | 0.1% | `tests/sbom-binary.bats` | | 0.8 | 0.1% | `tests/duplicate-close-check.bats` | -| 0.8 | 0.1% | `tests/mcp-attach-check.bats` | -| 0.8 | 0.1% | `tests/timeout-check.bats` | -| 0.8 | 0.1% | `tests/mcp-timeout-budget.bats` | -| 0.8 | 0.1% | `tests/merged-pr-keys.bats` | -| 0.8 | 0.1% | `tests/hook-profile-check.bats` | -| 0.8 | 0.1% | `tests/lint-rego.bats` | -| 0.8 | 0.1% | `tests/install.bats` | +| 0.8 | 0.1% | `tests/verified.bats` | +| 0.8 | 0.1% | `tests/fact-record-keying.bats` | | 0.8 | 0.1% | `tests/checksums.bats` | -| 0.7 | 0.1% | `tests/hook-pin-check.bats` | -| 0.7 | 0.1% | `tests/publish-credential-check.bats` | -| 0.7 | 0.1% | `tests/run-shape-guard-quoting.bats` | -| 0.7 | 0.0% | `tests/branch-age-check.bats` | -| 0.7 | 0.0% | `tests/macos-link-check.bats` | -| 0.6 | 0.0% | `tests/board-payloads.bats` | -| 0.6 | 0.0% | `tests/abandon-matrix.bats` | -| 0.6 | 0.0% | `tests/sonar-gate.bats` | -| 0.6 | 0.0% | `tests/pipefail-grep-check.bats` | -| 0.6 | 0.0% | `tests/land-lock-check.bats` | -| 0.6 | 0.0% | `tests/msrv-pin-agreement.bats` | -| 0.5 | 0.0% | `tests/report-only-check.bats` | -| 0.5 | 0.0% | `tests/digest-major-agreement.bats` | -| 0.5 | 0.0% | `tests/batten-glob-check.bats` | +| 0.8 | 0.1% | `tests/sonar-gate.bats` | +| 0.7 | 0.1% | `tests/perf-compare.bats` | +| 0.7 | 0.1% | `tests/merged-pr-keys.bats` | +| 0.7 | 0.1% | `tests/hook-matcher-check.bats` | +| 0.7 | 0.1% | `tests/mcp-timeout-budget.bats` | +| 0.7 | 0.1% | `tests/evaluator-closure-check.bats` | +| 0.7 | 0.1% | `tests/lint-rego.bats` | +| 0.7 | 0.1% | `tests/mcp-attach-check.bats` | +| 0.6 | 0.1% | `tests/publish-credential-check.bats` | +| 0.6 | 0.1% | `tests/hook-profile-check.bats` | +| 0.6 | 0.1% | `tests/pipefail-grep-check.bats` | +| 0.6 | 0.1% | `tests/msrv-pin-agreement.bats` | +| 0.6 | 0.1% | `tests/hook-pin-check.bats` | +| 0.6 | 0.1% | `tests/board-payloads.bats` | +| 0.6 | 0.1% | `tests/abandon-matrix.bats` | | 0.5 | 0.0% | `tests/connector-allow-guard.bats` | -| 0.5 | 0.0% | `tests/pkl-check.bats` | -| 0.5 | 0.0% | `tests/token-bench-check.bats` | +| 0.5 | 0.0% | `tests/branch-age-check.bats` | +| 0.5 | 0.0% | `tests/macos-link-check.bats` | +| 0.5 | 0.0% | `tests/digest-major-agreement.bats` | +| 0.5 | 0.0% | `tests/transcript-corpus-check.bats` | +| 0.5 | 0.0% | `tests/land-lock-check.bats` | +| 0.5 | 0.0% | `tests/run-shape-guard-quoting.bats` | | 0.5 | 0.0% | `tests/serena-mcp.bats` | -| 0.5 | 0.0% | `tests/cap-drift.bats` | -| 0.4 | 0.0% | `tests/privileged-lane.bats` | -| 0.4 | 0.0% | `tests/license-table-check.bats` | -| 0.4 | 0.0% | `tests/commit-attribution.bats` | +| 0.4 | 0.0% | `tests/timeout-check.bats` | +| 0.4 | 0.0% | `tests/batten-glob-check.bats` | | 0.4 | 0.0% | `tests/release-due.bats` | -| 0.4 | 0.0% | `tests/test-bats-parallel.bats` | -| 0.4 | 0.0% | `tests/ci-drift.bats` | | 0.4 | 0.0% | `tests/nonverdict-assert.bats` | +| 0.4 | 0.0% | `tests/pkl-check.bats` | | 0.4 | 0.0% | `tests/no-doctests.bats` | +| 0.4 | 0.0% | `tests/cap-drift.bats` | +| 0.4 | 0.0% | `tests/license-table-check.bats` | +| 0.4 | 0.0% | `tests/mise-pin-agreement.bats` | +| 0.4 | 0.0% | `tests/report-only-check.bats` | +| 0.4 | 0.0% | `tests/ci-drift.bats` | | 0.4 | 0.0% | `tests/container-preflight.bats` | -| 0.4 | 0.0% | `tests/coderabbit-config-check.bats` | -| 0.3 | 0.0% | `tests/mise-pin-agreement.bats` | +| 0.3 | 0.0% | `tests/privileged-lane.bats` | +| 0.3 | 0.0% | `tests/commit-attribution.bats` | | 0.3 | 0.0% | `tests/connector-allow-resolve.bats` | -| 0.3 | 0.0% | `tests/rust-paths-check.bats` | -| 0.3 | 0.0% | `tests/task-fail-closed.bats` | -| 0.3 | 0.0% | `tests/mise-action-floor.bats` | +| 0.3 | 0.0% | `tests/commit-convention.bats` | +| 0.3 | 0.0% | `tests/coderabbit-config-check.bats` | | 0.3 | 0.0% | `tests/git-hook.bats` | -| 0.2 | 0.0% | `tests/perf-gate.bats` | +| 0.2 | 0.0% | `tests/rust-paths-check.bats` | | 0.2 | 0.0% | `tests/remedy-payload-source.bats` | +| 0.2 | 0.0% | `tests/mise-action-floor.bats` | +| 0.2 | 0.0% | `tests/perf-gate.bats` | +| 0.2 | 0.0% | `tests/token-bench-check.bats` | +| 0.2 | 0.0% | `tests/task-fail-closed.bats` | | 0.2 | 0.0% | `tests/dist.bats` | +| 0.2 | 0.0% | `tests/test-bats-parallel.bats` | +| 0.1 | 0.0% | `tests/evaluator-io-check.bats` | | 0.1 | 0.0% | `tests/egress-check.bats` | | 0.1 | 0.0% | `tests/perf-pair.bats` | -| 0.1 | 0.0% | `tests/evaluator-io-check.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.0 | 0.0% | `tests/zizmor-split.bats` | diff --git a/crates/batten/src/config.rs b/crates/batten/src/config.rs index 3ff3fbc42..302d6a406 100644 --- a/crates/batten/src/config.rs +++ b/crates/batten/src/config.rs @@ -947,6 +947,10 @@ fn parse_ungated(text: &str, source: &str) -> Result { // waiver carries an expiry" true of the resolved config rather than aspirational. crate::waiver::validate(&config.waivers)?; crate::facts::validate(&config.facts)?; + // The cross-table half (CLOUD-859), which needs both lists and so cannot live + // in either one's own validator: a `named` receipt row over an agent-sourced + // check is a gate no record can satisfy. + crate::facts::validate_keying(&config.facts, &config.rules)?; crate::mint::validate(&config.mints)?; // `[budget]` is a table rather than a list, so the census below (which scans // `Vec` fields) does not reach it — but the failure it guards against is diff --git a/crates/batten/src/facts.rs b/crates/batten/src/facts.rs index 1b08b450b..74a41c284 100644 --- a/crates/batten/src/facts.rs +++ b/crates/batten/src/facts.rs @@ -1729,15 +1729,37 @@ impl Sourced { /// Where one agent-sourced fact's record lives, beside the other receipts. /// -/// Keyed on the FACT's own natural key rather than on a branch or a SHA: a -/// claimed-key answer is a statement about one issue row at one moment, and -/// keying it to a branch would make the same answer unavailable to the next -/// branch that needs it and stale-by-construction on this one. +/// **Keyed on the SUBJECT the declaring row's `key` resolves to, never on the +/// name alone** (CLOUD-859). The name-only form shipped with this channel and its +/// own doc argued for it: a `claimed-key` answer is a statement about one issue +/// row at one moment, so keying it to a branch would make the same answer +/// unavailable to the next branch that needs it. That reasoning is right for a +/// fact about an *issue* and wrong for a fact about a *head*, and hard-coding it +/// made the difference inexpressible — a `receipt` row could declare +/// `key = "head"`, `rules::validate` would hold it to one keying per check, and +/// the record would file under the name regardless. Measured: a record minted +/// clear on one head still satisfied its check on the next commit, so the gate +/// bound once per branch. +/// +/// So the reading that doc defends is now spelled `key = "branch"` rather than +/// built in, and both readings are expressible. The subject is HEAD's SHA under +/// [`crate::rules::ReceiptKey::Head`], the branch name under +/// [`crate::rules::ReceiptKey::Branch`], and the call's own value under +/// [`crate::rules::ReceiptKey::Named`] — the same three subjects +/// `receipt::verdicts` already resolves for the receipt store, resolved once at +/// the boundary because `adjudicate` may not look. +/// +/// Both components are `/`-substituted: a branch name legitimately carries +/// separators and a filename may not. The spelling matches +/// `receipt::branch_receipt_name`'s for the same reason it does there — two +/// spellings of one filename are two things to drift. #[must_use] -pub fn sourced_path(git_dir: &std::path::Path, name: &str) -> std::path::PathBuf { - git_dir - .join("batten-receipts") - .join(format!("fact.{}", name.replace('/', "-"))) +pub fn sourced_path(git_dir: &std::path::Path, name: &str, subject: &str) -> std::path::PathBuf { + git_dir.join("batten-receipts").join(format!( + "fact.{}.{}", + name.replace('/', "-"), + subject.replace('/', "-") + )) } /// What a recorded answer means, given the command the gate asked for. @@ -2270,3 +2292,48 @@ pub fn validate(facts: &[Declared]) -> anyhow::Result<()> { } Ok(()) } + +/// Refuse a keying an agent-sourced record cannot be filed under (CLOUD-859). +/// +/// `key` became load-bearing on this path when the record started honouring it, +/// and exactly one of the three values is unreachable here. **The two halves run +/// on different envelopes**: the record is WRITTEN on the post-tool event of the +/// fact's own command — a shell call carrying a command line and nothing else — +/// and READ on the mediated call the receipt row selects. A `head` or `branch` +/// subject is a fact about the checkout and resolves identically at both moments; +/// a `named` subject is projected out of the reading call's own arguments, which +/// the writing call does not have. +/// +/// So a `named` agent-sourced check would deny with a `Fix::Run`, the agent would +/// run the command it names, and no record would be filed — a gate nobody can +/// satisfy by doing what it asks. That is the failure this whole row exists to +/// end, so it is refused at load rather than shipped as a column that reads as +/// configured and files nothing. +/// +/// # Errors +/// +/// Returns a [`crate::error::UsageError`] (→ exit `1`) naming the fact and the +/// row. Pointer-only: two ids, never a command. +pub fn validate_keying(facts: &[Declared], rules: &[crate::rules::Rule]) -> anyhow::Result<()> { + for rule in rules { + if rule.kind != crate::rules::RuleKind::Receipt + || rule.receipt_key() != crate::rules::ReceiptKey::Named + { + continue; + } + for check in rule.checks.iter().flatten() { + if facts.iter().any(|fact| &fact.name == check) { + return Err(crate::error::UsageError::raise(format!( + "rule {}: `key = \"named\"` over the agent-sourced fact `{check}` — a named \ + subject is projected from the call this row SELECTS, and the record is \ + written on the post-tool event of the fact's own command, which carries no \ + such subject. No record could ever be filed, so the check would deny \ + forever and running the command it names would not satisfy it. Key it \ + `head` or `branch`.", + rule.id + ))); + } + } + } + Ok(()) +} diff --git a/crates/batten/src/hook.rs b/crates/batten/src/hook.rs index 5b4644b11..2b859fbf0 100644 --- a/crates/batten/src/hook.rs +++ b/crates/batten/src/hook.rs @@ -2641,6 +2641,32 @@ impl Policy { self.facts.iter().find(|fact| fact.name == check) } + /// The keying declared for `check` by the rows that require it (CLOUD-859). + /// + /// **Policy-wide, deliberately, where [`Policy::required_checks_for`] is + /// scoped to the call.** The record is WRITTEN on the post-tool event of the + /// declared command — `gh api graphql …` — and READ on the mediated call the + /// receipt row selects — `gh pr ready`. Those are different envelopes, so a + /// call-scoped lookup finds nothing at the moment the record is filed, and + /// the two halves would file and look under different subjects. + /// + /// The first match is the answer because `rules::validate` refuses one check + /// required under two keys: the keying is unambiguous by construction rather + /// than by picking a winner here. + #[must_use] + pub fn receipt_key_for_check(&self, check: &str) -> Option { + self.shapes + .iter() + .filter(|rule| rule.kind == RuleKind::Receipt) + .find(|rule| { + rule.checks + .iter() + .flatten() + .any(|required| required == check) + }) + .map(Rule::receipt_key) + } + /// The receipt names this **command** needs proved, deduplicated. /// /// Scoped to the command, not to the policy (CLOUD-460). The earlier form diff --git a/crates/batten/src/lib.rs b/crates/batten/src/lib.rs index 735549c70..528e2d15a 100644 --- a/crates/batten/src/lib.rs +++ b/crates/batten/src/lib.rs @@ -2143,8 +2143,8 @@ fn run_hook_field( fn receipt_facts( policy: &hook::Policy, envelope: &hook::Envelope, - required: &std::collections::BTreeMap, sourced: &[(&String, &rules::ReceiptKey)], + store: Option<&receipt::SourcedStore>, receipted: &std::collections::BTreeMap, max_ages: &std::collections::BTreeMap, judgeable: bool, @@ -2154,7 +2154,13 @@ fn receipt_facts( // check selected this call, or the write lands somewhere policy does not // judge. `CouldNotLook` is further down, where the receipt store itself // could not be read. Both allow, and they are not the same fact. - if required.is_empty() || !judgeable { + // + // `sourced` and `receipted` are the partition of the required set, so their + // both being empty IS "no required check selected this call" — read from the + // two halves rather than from a third parameter carrying the union, which + // clippy's argument ceiling is right to refuse and which was a second + // spelling of one fact besides. + if (sourced.is_empty() && receipted.is_empty()) || !judgeable { return facts::Look::IsNot; } let mut verdicts = if receipted.is_empty() { @@ -2181,16 +2187,42 @@ fn receipt_facts( // the boundary just read. `Look::Is` is the only answer that satisfies a // check; never-ran and command-mismatch both arrive as `Missing`, which // is the deny that carries the `Fix::Run` asking for the command. - if let Some(verdicts) = verdicts.as_mut() { + // + // A store the boundary could not build takes the WHOLE call to + // could-not-look (CLOUD-859), rather than leaving these checks out of the + // map. Leaving them out is not a softer answer: `receipt_rules` reads an + // absent verdict as `Missing` — deliberately, since a boundary that answered + // for fewer checks than a row requires has not proved the precondition — so + // an omission is the strictest answer available, and it would make a + // checkout with no resolvable HEAD refuse every `gh pr ready` for a property + // of the environment. This is `receipt::verdicts`'s own posture, where an + // unresolvable branch takes the call to could-not-look for the same reason. + if !sourced.is_empty() && store.is_none() { + return facts::Look::CouldNotLook; + } + if let (Some(verdicts), Some(store)) = (verdicts.as_mut(), store) { for (check, _) in sourced { let Some(declared) = policy.agent_fact(check) else { continue; }; - let record = receipt::sourced_record(check); + let record = store.record(check); let verdict = match facts::sourced(record.as_ref(), &declared.command) { facts::Look::Is(_) => receipt::Validity::Valid, facts::Look::IsNot | facts::Look::CouldNotLook => receipt::Validity::Missing, }; + // THE AGE IS READ LAST AND ONLY OVER A VALID RECORD, exactly as + // `receipt::verdicts` reads it (CLOUD-988): a record already Missing + // has a more specific answer and a different remedy — *run it*, + // where this one says *run it again* — and a repository declaring no + // bound pays no `stat`. + let verdict = match (verdict, max_ages.get(*check)) { + (receipt::Validity::Valid, Some(&max_age)) + if store.expired(check, max_age, std::time::SystemTime::now()) => + { + receipt::Validity::Expired + } + (verdict, _) => verdict, + }; verdicts.insert((*check).clone(), verdict); } } @@ -2409,10 +2441,22 @@ fn run_hook( .into_iter() .map(|(check, key)| (check.clone(), *key)) .collect(); + // Where each agent-sourced record lives on THIS call, resolved once for both + // readers (CLOUD-859). A record is filed under the subject its receipt row's + // `key` names, so the boundary resolves that subject here — `adjudicate` may + // not look, and resolving it per reader would let the two disagree. + let sourced_store = + receipt::sourced_store(&sourced, policy.named_receipt_subject(&envelope).as_deref()); let receipts: hook::ReceiptFacts = receipt_facts( - &policy, &envelope, &required, &sourced, &receipted, &max_ages, judgeable, + &policy, + &envelope, + &sourced, + sourced_store.as_ref(), + &receipted, + &max_ages, + judgeable, ); - let agent_sourced = agent_records(&sourced); + let agent_sourced = agent_records(&sourced, sourced_store.as_ref()); // The key evidence (CLOUD-446), resolved on the same terms and for the same // reason: two git queries a pure `adjudicate` cannot make, spent only when a // `requires_key` row has already selected this command. A repository @@ -2659,13 +2703,21 @@ fn dispatch_handlers( /// Same narrowing as every other fact on this path: `checks` is empty unless a /// required check is agent-sourced, so a repository declaring none pays nothing /// and the answer is `None` rather than an empty map. -fn agent_records(checks: &[(&String, &rules::ReceiptKey)]) -> hook::AgentFacts { +fn agent_records( + checks: &[(&String, &rules::ReceiptKey)], + store: Option<&receipt::SourcedStore>, +) -> hook::AgentFacts { if checks.is_empty() { return None; } + // A store that could not be built answers `None` for the same reason an + // empty `checks` does: there is nothing this boundary looked at, so the + // policy input carries no records rather than an empty map asserting there + // are none. + let store = store?; let mut records = std::collections::BTreeMap::new(); for (check, _) in checks { - if let Some(record) = receipt::sourced_record(check) { + if let Some(record) = store.record(check) { records.insert((*check).clone(), record); } } @@ -3218,7 +3270,21 @@ fn record_agent_fact(overrides: &Overrides, envelope: &hook::Envelope) { ), rows, }; - let _ = receipt::record_sourced(&declared.name, &record); + // FILED UNDER THE KEY THE DECLARING ROW STATES (CLOUD-859), resolved + // policy-wide because this envelope is the fact's own command and not the + // call the receipt row selects — a call-scoped lookup finds nothing here. + // A fact no receipt row requires is unreadable by anything, so recording it + // would leave a file nobody consults; that is silent like every other + // failure on this path. + let Some(key) = policy.receipt_key_for_check(&declared.name) else { + return; + }; + let _ = receipt::record_sourced( + &declared.name, + key, + policy.named_receipt_subject(envelope).as_deref(), + &record, + ); } /// Everything the post-tool event records, in the order it must happen. diff --git a/crates/batten/src/receipt.rs b/crates/batten/src/receipt.rs index cc81637e5..8f77e9725 100644 --- a/crates/batten/src/receipt.rs +++ b/crates/batten/src/receipt.rs @@ -469,17 +469,130 @@ fn receipt_path(repo_root: &str, check: &str) -> Result { /// guard has, and the one CLOUD-312 §5 preserves end to end. It is deliberately /// distinct from `Some(Missing)`, which is a real verdict about a real /// repository and denies. -/// Read one agent-sourced fact's record, if it exists and parses (CLOUD-776). +/// Where each agent-sourced check's record lives on this call, resolved once +/// (CLOUD-859). +/// +/// **One resolution for every check, rather than one per check.** Both readers — +/// the receipt verdict and the policy input's record set — ask about the same +/// checks on the same call, and each of them used to call [`git::git_dir`] +/// per check. The subjects a declared key resolves to are facts about the +/// checkout, so they are resolved here, at the boundary, exactly as +/// [`verdicts`] resolves them for the receipt store. +pub(crate) struct SourcedStore { + /// The absolute git dir — per-worktree by construction, as [`RepoFacts`]. + git_dir: std::path::PathBuf, + /// The subject each check's record files under, by check name. + subjects: BTreeMap, +} + +impl SourcedStore { + /// Where `check`'s record lives, or `None` if no subject resolved for it. + fn path(&self, check: &str) -> Option { + let subject = self.subjects.get(check)?; + Some(crate::facts::sourced_path(&self.git_dir, check, subject)) + } + + /// Read one agent-sourced fact's record, if it exists and parses + /// (CLOUD-776). + /// + /// The I/O half, at the boundary — the deciding half is + /// [`crate::facts::sourced`] and is pure. Unreadable and unparseable both + /// answer `None`, which that function turns into + /// [`crate::facts::Look::CouldNotLook`]: fail closed to *we do not know*, + /// never to a fact. A record filed under a DIFFERENT subject is simply + /// absent here, and lands on that same arm — which is the whole of + /// CLOUD-859's fix, since the existing three-valued contract already carries + /// it and no new verdict was needed. + pub(crate) fn record(&self, check: &str) -> Option { + crate::facts::Sourced::parse(&std::fs::read_to_string(self.path(check)?).ok()?) + } + + /// Whether `check`'s record is older than the declaring row's bound + /// (CLOUD-988). + /// + /// Discarded on this path until CLOUD-859: `max_ages` reached + /// `receipt_facts` and the agent-sourced loop never read it, so neither the + /// head nor the clock bounded the evidence. Reads the file's mtime rather + /// than the record's own `seen_at`, because [`older_than`] is already the + /// one spelling of *how old is a receipt* and a second one is a second thing + /// to drift. + pub(crate) fn expired(&self, check: &str, max_age: u64, now: std::time::SystemTime) -> bool { + self.path(check) + .is_some_and(|path| older_than(&path, max_age, now)) + } +} + +/// Resolve the subject every agent-sourced check on this call files under. /// -/// The I/O half, at the boundary — the deciding half is [`crate::facts::sourced`] -/// and is pure. Unreadable and unparseable both answer `None`, which that -/// function turns into [`crate::facts::Look::CouldNotLook`]: fail closed to *we -/// do not know*, never to a fact. -#[must_use] -pub fn sourced_record(name: &str) -> Option { +/// `None` is **could not look** and takes the whole agent-sourced arm with it — +/// the fail-open direction [`verdicts`] documents, and for its reason: a gate +/// that cannot see the repository must not become a gate that denies everything. +/// An empty `checks` answers an empty store having done no git work at all, +/// which is the narrowing every other fact on this path applies. +pub(crate) fn sourced_store( + checks: &[(&String, &ReceiptKey)], + named: Option<&str>, +) -> Option { + if checks.is_empty() { + return Some(SourcedStore { + git_dir: std::path::PathBuf::new(), + subjects: BTreeMap::new(), + }); + } let git_dir = git::git_dir(Path::new(".")).ok()?; - let path = crate::facts::sourced_path(&git_dir, name); - crate::facts::Sourced::parse(&std::fs::read_to_string(path).ok()?) + // Resolved once, and only where a row asked — a head-keyed caller must not + // pay a branch lookup for a question it never asks. `current_branch` rather + // than `branch_facts`: that one also counts this branch's own commits, which + // `branch_validity` needs and a record filed by name does not. + let head = if checks.iter().any(|(_, key)| **key == ReceiptKey::Head) { + Some(git::head_commit(Path::new(".")).ok()?) + } else { + None + }; + let branch = if checks.iter().any(|(_, key)| **key == ReceiptKey::Branch) { + Some(git::current_branch(Path::new(".")).ok()??) + } else { + None + }; + // `named` over an agent-sourced check is REFUSED AT LOAD + // (`facts::validate_keying`), so this is reachable only from a policy + // assembled in-process. Resolved anyway rather than left to a wildcard arm + // below: `_ =>` would silently absorb a fourth keying, which is the shape + // `facts.rs`'s own no-wildcard scan exists to refuse. + let named = if checks.iter().any(|(_, key)| **key == ReceiptKey::Named) { + Some(named.filter(|value| safe_subject(value))?.to_owned()) + } else { + None + }; + let mut subjects = BTreeMap::new(); + for (check, key) in checks { + let subject = match key { + ReceiptKey::Head => head.clone()?, + ReceiptKey::Branch => branch.clone()?, + ReceiptKey::Named => named.clone()?, + }; + subjects.insert((*check).clone(), subject); + } + Some(SourcedStore { git_dir, subjects }) +} + +/// The subject ONE agent-sourced fact's record is written under (CLOUD-859). +/// +/// The write half of [`sourced_store`], and separate from it because the two +/// sides ask on different calls: the read resolves subjects for the checks a +/// mediated call requires, and the write resolves one for the fact whose command +/// just ran. That difference is exactly why +/// [`crate::facts::validate_keying`] refuses `named` for an agent-sourced +/// check — this envelope carries no subject to project — so the third arm is +/// unreachable through a loaded config and is written out rather than wildcarded. +fn sourced_subject(key: ReceiptKey, named: Option<&str>) -> Option { + match key { + ReceiptKey::Head => git::head_commit(Path::new(".")).ok(), + ReceiptKey::Branch => git::current_branch(Path::new(".")).ok()?, + ReceiptKey::Named => named + .filter(|value| safe_subject(value)) + .map(ToOwned::to_owned), + } } /// Write one agent-sourced fact's record (CLOUD-776). @@ -488,18 +601,35 @@ pub fn sourced_record(name: &str) -> Option { /// buffer it was derived from (rule 4): a command's stdout can hold anything, so /// nothing under the state root may reproduce it. /// +/// **Filed under the key the declaring row states** (CLOUD-859), resolved here +/// because `adjudicate` may not look and the reader resolves the same subject +/// the same way. `rules::validate` refuses one check required under two keys, so +/// there is exactly one subject to file under and the two halves cannot disagree +/// about where the record is. +/// /// # Errors /// -/// Propagates a failure to locate the git dir or to write the record. A caller on -/// the mediated path treats that as *could not record* and allows — a hook that -/// cannot write a fact must not become the reason work stops. -pub fn record_sourced(name: &str, record: &crate::facts::Sourced) -> Result<()> { +/// Propagates a failure to locate the git dir, to resolve the key's subject, or +/// to write the record. A caller on the mediated path treats that as *could not +/// record* and allows — a hook that cannot write a fact must not become the +/// reason work stops. +pub fn record_sourced( + name: &str, + key: ReceiptKey, + named: Option<&str>, + record: &crate::facts::Sourced, +) -> Result<()> { let git_dir = git::git_dir(Path::new(".")).map_err(|_| { UsageError::raise( "not a git repository, so there is nowhere an agent-sourced fact could be recorded", ) })?; - let path = crate::facts::sourced_path(&git_dir, name); + let subject = sourced_subject(key, named).ok_or_else(|| { + UsageError::raise( + "the declared key's subject does not resolve in this checkout, so there is no subject to file an agent-sourced record under", + ) + })?; + let path = crate::facts::sourced_path(&git_dir, name, &subject); if let Some(parent) = path.parent() { std::fs::create_dir_all(parent)?; } diff --git a/crates/batten/tests/agent_facts.rs b/crates/batten/tests/agent_facts.rs index 7388108f7..34abccfc7 100644 --- a/crates/batten/tests/agent_facts.rs +++ b/crates/batten/tests/agent_facts.rs @@ -356,15 +356,29 @@ fn both_measured_buffer_shapes_are_read() { } #[test] -fn the_record_is_keyed_on_the_fact_not_on_a_branch_or_a_sha() { - // CLOUD-776 decision 2: a claimed-key answer is a statement about one row at - // one moment. Keying it to a branch would make the same answer unavailable to - // the next branch that needs it, and stale-by-construction on this one. - let path = facts::sourced_path(std::path::Path::new("/repo/.git"), "claimed-key"); - assert!(path.ends_with("batten-receipts/fact.claimed-key")); - // A name that would escape the directory cannot. - let nested = facts::sourced_path(std::path::Path::new("/repo/.git"), "a/b"); - assert!(nested.ends_with("batten-receipts/fact.a-b")); +fn the_record_is_keyed_on_the_fact_and_on_its_rows_declared_subject() { + // CLOUD-776 decision 2 held that a `claimed-key` answer is a statement about + // one row at one moment, so keying it to a branch would make the same answer + // unavailable to the next branch that needs it. That reading is now spelled + // `key = "branch"` on the declaring row rather than built in (CLOUD-859), and + // the subject is the other component of the filename. + let git = std::path::Path::new("/repo/.git"); + assert!( + facts::sourced_path(git, "claimed-key", "CLOUD-776") + .ends_with("batten-receipts/fact.claimed-key.CLOUD-776") + ); + // TWO KEYINGS, TWO FILES, which is the whole of what "the key is read" means + // here: a record minted under one subject is simply absent under another, and + // `facts::sourced` already turns absence into could-not-look. + assert_ne!( + facts::sourced_path(git, "review-answered", "0f1e2d3"), + facts::sourced_path(git, "review-answered", "claude/some-branch") + ); + // Neither component may escape the directory: a fact name may carry a `/` and + // a branch name routinely does. + assert!( + facts::sourced_path(git, "a/b", "claude/c").ends_with("batten-receipts/fact.a-b.claude-c") + ); } #[test] diff --git a/tests/fact-record-keying.bats b/tests/fact-record-keying.bats new file mode 100644 index 000000000..55bfa777e --- /dev/null +++ b/tests/fact-record-keying.bats @@ -0,0 +1,265 @@ +#!/usr/bin/env bats +# subject: crates/batten/src/facts.rs +# +# The subject header is READ, not decoration: `bats-tests-not-deleted` resolves it +# to decide whether a suite's subject is still alive, and every other suite names +# exactly one bare path. This one named two and a parenthetical, and the ratchet +# refused it at this line. The other half of the subject is +# `crates/batten/src/receipt.rs`, which is said here in prose rather than in the +# header for that reason. +# +# An agent-sourced record is filed under the subject its receipt row's `key` +# names (CLOUD-859). Until this suite existed the record was keyed on the fact's +# NAME alone, so `key` was accepted at load and unread at decision: one clear +# answer authorised every later head on the branch, and the read-the-review gate +# that shipped in #705 bound once per branch rather than once per head. +# +# WHY THIS TIER AND NOT A UNIT TEST, stated because the temptation is real and +# `sourced_path` is a pure function two lines long. A unit test over it asserts +# that a filename contains a subject somebody passed in; it cannot show that the +# BOUNDARY resolves that subject and hands it to both halves. The defect was +# never in the filename — it was that nothing computed a subject at all. So every +# case here goes through the two real hook calls a session makes: a `PostToolUse` +# envelope carrying the declared command, which is what mints the record, and a +# `PreToolUse` `gh pr ready`, which is what reads it. Nothing writes a receipt by +# hand and nothing inspects a path to decide a case. +# +# THE ANTI-VACUITY TWIN IS THE POINT OF THE SUITE, not a courtesy: head-keying +# everything would satisfy the first case and break `claim` repo-wide, because a +# claim attests to a decision about an ISSUE and every commit on the branch +# continues to serve it. "a branch-keyed record survives a new commit" is the +# case that has to stay green. + +setup() { + load helpers + + # `batten_binary` rather than the release-first chain five suites carried: + # `test:bats` builds DEBUG, so a leftover release binary shadowed it and a + # suite reported on a build older than the code under test. This suite is + # where that was measured — see the helper's own header. + BIN=$(batten_binary "$BATS_TEST_DIRNAME/..") || skip "no batten binary to drive" + + # A CONSTANT string, which is what the channel requires: `Declared.command` is + # compared byte-for-byte against what the agent ran, and that comparison is the + # forgery control. The engine never executes it — it compares the command and + # counts the result — so these cases can drive shapes a live `gh` could not be + # made to produce on demand. + COMMAND="gh pr view --json reviewThreads --jq '[.reviewThreads[] | select(.isResolved | not)]'" + REPO="$BATS_TEST_TMPDIR/repo" +} + +# Build the fixture repository with ONE `[[fact]]` and one `receipt` row keyed as +# asked. The keying is the only thing that varies between cases, which is what +# makes a difference in verdict attributable to it. +fixture() { # fixture [max_age] + rm -rf "$REPO" + mkdir -p "$REPO" + { + echo "version = 1" + echo + echo "[[fact]]" + echo 'name = "keyed"' + echo 'returns = "json-array"' + printf 'command = "%s"\n' "$COMMAND" + echo + echo "[[rule]]" + echo 'id = "ready-needs-the-fact"' + echo 'kind = "receipt"' + echo 'scope = "mediated_call"' + echo 'severity = "deny"' + echo 'pattern = "gh pr ready"' + echo 'checks = ["keyed"]' + printf 'key = "%s"\n' "$1" + if [ -n "${2:-}" ]; then printf 'max_age = %s\n' "$2"; fi + echo 'reason = "run the declared command"' + } >"$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" + commit "the first commit" +} + +commit() { # commit + (cd "$REPO" && + GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null \ + git -c user.email=fixture@example.invalid -c user.name=fixture \ + commit -q --allow-empty -m "$1") +} + +# 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-keying","cwd":"/repo","tool_name":"Bash","tool_input":{"command":sys.argv[1]},"tool_response":{"stdout":sys.argv[2],"stderr":""}}))' "$COMMAND" "${1:-[]}") + (cd "$REPO" && printf '%s' "$envelope" | "$BIN" hook --harness claude-code) +} + +# Read it: the call the receipt row judges. +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 999") + (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 record filenames in the store, one per line. +# +# A glob rather than `find -printf`: that flag is GNU-only, and macOS `find` +# rejects it — so on a Mac this would print nothing and every non-empty assertion +# below would fail on the tool rather than on the gate. That is the class +# `tests/helpers.bash` exists for (CLOUD-282), and CI cannot catch it because CI +# is ubuntu. A glob is already sorted, so nothing else is needed. +records() { + local file names="" + for file in "$REPO"/.git/batten-receipts/fact.*; do + [[ -e "$file" ]] || continue + names="${names}${names:+$'\n'}$(basename "$file")" + done + printf '%s\n' "$names" +} + +# Backdate every record in the store by `$1` seconds. +# +# `python3` rather than `touch -d '2 hours ago'`: BSD `touch` reads `-d` as an +# ISO timestamp and rejects a relative expression outright, and these suites +# already depend on python3 for their envelopes. +age_records() { # age_records + python3 - "$REPO/.git/batten-receipts" "$1" <<'AGE' +import glob, os, sys, time + +store, seconds = sys.argv[1], int(sys.argv[2]) +when = time.time() - seconds +for path in glob.glob(os.path.join(store, "fact.*")): + os.utime(path, (when, when)) +AGE +} + +# --- the defect, and the case that shows it able to fail --------------------- + +@test "a head-keyed record cleared on one commit does not satisfy the check on the next" { + # THE MEASURED DEFECT. Before this change the record filed under the fact's + # name, so the second `ready` here was ALLOWED — an agent ran the command, got a + # clear answer, pushed a fix nobody had reviewed, and readied it. + fixture head + run record '[]' + [ "$status" -eq 0 ] + run ready + allowed "$output" + + commit "a fix nobody has looked at" + run ready + denied "$output" + # The remedy is the DECLARED command, unchanged: a record under a new head is + # simply missing, so no new verdict and no new message were needed. + [[ "$output" == *"gh pr view --json reviewThreads"* ]] +} + +# --- the anti-vacuity twin, which is what stops the fix being "key by head" --- + +@test "ANTI-VACUITY: a branch-keyed record still satisfies the check after a new commit" { + # The case that has to stay green. `claim-needs-receipt` is keyed by branch + # precisely because a claim attests to a decision about an issue that every + # commit on the branch continues to serve — CLOUD-516's incident read the other + # way round. A fix that head-keyed every record would pass the case above and + # make `claim` demand a re-claim per commit, which is the false-positive rate + # that gets a guard bypassed. + fixture branch + run record '[]' + [ "$status" -eq 0 ] + commit "one more commit on the same claim" + run ready + allowed "$output" +} + +@test "a branch-keyed record does not follow the checkout onto another branch" { + # And the twin's own twin: `branch` must be a real subject rather than a way of + # spelling "never expires". A record minted on one branch is absent on the next, + # which is the same could-not-look the missing-record arm already carries. + fixture branch + run record '[]' + [ "$status" -eq 0 ] + run ready + allowed "$output" + + (cd "$REPO" && git checkout -q -b claude/somewhere-else) + run ready + denied "$output" +} + +# --- the key is read at all -------------------------------------------------- + +@test "head and branch keyings file the record under different names" { + # The cheapest statement of "the column is load-bearing": two fixtures differing + # only in `key` put the record in two different places. Read off the store + # rather than asserted about a path, so this fails if the boundary stops + # resolving a subject even though `sourced_path` still accepts one. + fixture head + run record '[]' + [ "$status" -eq 0 ] + local head_named + head_named=$(records) + [ -n "$head_named" ] + + fixture branch + run record '[]' + [ "$status" -eq 0 ] + local branch_named + branch_named=$(records) + [ -n "$branch_named" ] + + [ "$head_named" != "$branch_named" ] + # The branch-keyed one names the branch; the head-keyed one does not. + [[ "$branch_named" == *"main"* ]] + [[ "$head_named" != *"main"* ]] +} + +# --- the clock, discarded on this path until now ------------------------------ + +@test "max_age bounds an agent-sourced record, and an unaged one still passes" { + # CLOUD-988's column reached `receipt_facts` and the agent-sourced loop never + # read it, so neither the head nor the clock bounded the evidence. Both halves + # here, because a bound that refused everything would pass the first assertion + # alone. + fixture head 3600 + run record '[]' + [ "$status" -eq 0 ] + run ready + allowed "$output" + + # Aged by the fixture rather than by waiting: the property under test is that + # the bound is READ, and a suite that slept an hour would assert the same thing + # and cost an hour. + age_records 7200 + run ready + denied "$output" +} + +# --- what a keying nobody can file under does -------------------------------- + +@test "a named keying over an agent-sourced fact is refused at LOAD, not at decision" { + # The two halves run on different envelopes: the record is written on the + # post-tool event of the fact's own command, and a `named` subject is projected + # out of the call the row selects. So a `named` agent-sourced check would deny + # forever and running the command it names would not satisfy it — a gate nobody + # can clear, which is the failure this whole row exists to end. Refused where it + # can still be fixed rather than shipped as a column that files nothing. + fixture head + python3 - "$REPO/batten.toml" <<'PATCH' +import sys +path = sys.argv[1] +text = open(path).read().replace('key = "head"', 'key = "named"\nkey_from = "input-id"') +open(path, "w").write(text) +PATCH + run ready + # Exit 1 and a usage error, never exit 2: this is config the operator wrote + # being refused, not a verdict about the call. + [ "$status" -eq 1 ] + [[ "$output" == *"key = \"named\""* ]] + [[ "$output" == *"keyed"* ]] +} diff --git a/tests/helpers.bash b/tests/helpers.bash index d92032a59..7d2ff73cc 100644 --- a/tests/helpers.bash +++ b/tests/helpers.bash @@ -95,3 +95,55 @@ run_timeout() { fi return "$rc" } + +# The batten binary a suite drives, chosen by MTIME rather than by build profile +# (CLOUD-859). +# +# Five suites carried the same chain — `$BATTEN_BIN`, then release, then debug, +# first hit wins — and release-first is a measured false green. `test:bats` +# builds the DEBUG binary; a release binary left over from an earlier session +# shadows it, so a suite reports on a build that predates the change it exists to +# catch. Measured on this very change: `tests/review-answered.bats` passed all +# twelve cases against a release binary nine hours older than the code under +# test, and `tests/fact-record-keying.bats` only failed loudly because it asserts +# behaviour the stale build does not have. +# +# Newest-wins is right in every case release-first was right in and in this one +# too: with only one build present it picks that one, and CI — which has no +# release binary at all — is unaffected. `$BATTEN_BIN` still outranks both, +# because an explicit choice is not a guess. +# +# Prints nothing and returns 1 when there is no binary to drive; a caller skips +# on that rather than aborting setup, since a suite that cannot find a build has +# not learned anything about the gate. +batten_binary() { + local root=$1 candidate newest="" + if [[ -n "${BATTEN_BIN:-}" ]] && [[ -x "${BATTEN_BIN}" ]]; then + _batten_binary_abs "$BATTEN_BIN" + return 0 + fi + for candidate in "$root/target/release/batten" "$root/target/debug/batten"; do + [[ -x "$candidate" ]] || continue + if [[ -z "$newest" ]] || [[ "$candidate" -nt "$newest" ]]; then + newest=$candidate + fi + done + [[ -n "$newest" ]] || newest="$(command -v batten || true)" + [[ -n "$newest" ]] || return 1 + _batten_binary_abs "$newest" +} + +# Absolute form of a binary path, on EVERY branch above (CLOUD-859). +# +# `command -v batten` returns whatever `PATH` held, which can be relative, and +# `$BATTEN_BIN` is a caller's string with no shape guaranteed. Several suites +# `cd` into a throwaway repository before executing what this returned, so a +# relative path resolves against the wrong directory and the suite fails on the +# lookup rather than on the gate. `tests/replay.bats` had canonicalised by hand +# for exactly this reason; doing it here is what lets that copy go. +_batten_binary_abs() { # _batten_binary_abs + local dir base + dir=$(cd "$(dirname -- "$1")" && pwd) || return 1 + base=$(basename -- "$1") + printf '%s/%s\n' "$dir" "$base" +} diff --git a/tests/privileged-lane.bats b/tests/privileged-lane.bats index 64677a64a..a1c930479 100644 --- a/tests/privileged-lane.bats +++ b/tests/privileged-lane.bats @@ -32,17 +32,10 @@ setup() { # measured there: there is no release build when `test:bats` runs in CI, and a # shorter chain aborts setup before the skip can fire — turning "no binary # here" into a wall of red over a gate that was never exercised. - 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" + # `batten_binary` rather than a release-first chain: `test:bats` builds DEBUG, + # so a leftover release binary shadowed it and this suite would report on a + # build older than the code under test (CLOUD-859). + BIN=$(batten_binary "$BATS_TEST_DIRNAME/..") || skip "no batten binary to drive" MODULE="$BATS_TEST_DIRNAME/../policy/privileged-lane.rego" REPO="$BATS_TEST_TMPDIR/repo" diff --git a/tests/replay.bats b/tests/replay.bats index a9b9cd850..2007d74dc 100644 --- a/tests/replay.bats +++ b/tests/replay.bats @@ -25,17 +25,13 @@ setup() { # The compiled binary, resolved the way `run-shape.bats` resolves it and for # the reason recorded there: there is no release build when `test:bats` runs # in CI, and a shorter chain aborts setup before a skip can fire. - 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="$(cd "$(dirname "$candidate")" && pwd)/$(basename "$candidate")" - break - done - [ -n "$BIN" ] || BIN="$(command -v batten || true)" - [ -n "$BIN" ] || skip "no batten binary to drive" + # `batten_binary` rather than a release-first chain: `test:bats` builds DEBUG, + # so a leftover release binary shadowed it and this suite would report on a + # build older than the code under test (CLOUD-859). + # Absolute already — `batten_binary` canonicalises on every branch, which is + # why the hand-rolled `cd`/`pwd` this suite carried is gone rather than kept + # beside it. + BIN=$(batten_binary "$BATS_TEST_DIRNAME/..") || skip "no batten binary to drive" BATS_RUNNER="$BATS_TEST_DIRNAME/bats/bin/bats" [ -x "$BATS_RUNNER" ] || skip "no bats runner to hand the base tree" diff --git a/tests/review-answered.bats b/tests/review-answered.bats index 3eaec3a83..14103eb7d 100644 --- a/tests/review-answered.bats +++ b/tests/review-answered.bats @@ -24,20 +24,12 @@ 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" + # `batten_binary`, which keeps the skip this chain existed for — there is no + # release build when `test:bats` runs in CI, and a shorter chain took every + # case with it — and drops the release-FIRST ordering, which was a measured + # false green: these twelve cases passed against a release binary nine hours + # older than the change they would otherwise have caught (CLOUD-859). + BIN=$(batten_binary "$BATS_TEST_DIRNAME/..") || skip "no batten binary to drive" MODULE="$BATS_TEST_DIRNAME/../policy/review-answered.rego" REPO="$BATS_TEST_TMPDIR/repo" @@ -99,6 +91,16 @@ READER # 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" + # AND A COMMIT, which this fixture did not have and now needs (CLOUD-859). The + # record is filed under the subject the row's `key = "head"` names, so a + # repository whose HEAD does not resolve has no subject — the boundary answers + # could-not-look and every case below would be ALLOWED, which is the fail-open + # posture working and not the thing these cases are about. `tests/fact-record-keying.bats` + # is where the keying itself is asserted. + (cd "$REPO" && + GIT_CONFIG_GLOBAL=/dev/null GIT_CONFIG_SYSTEM=/dev/null \ + git -c user.email=fixture@example.invalid -c user.name=fixture \ + commit -q --allow-empty -m "the head this gate judges") } # Mint the record the way a session does: a PostToolUse envelope carrying the diff --git a/tests/run-shape.bats b/tests/run-shape.bats index 0ff83f99c..33faac439 100644 --- a/tests/run-shape.bats +++ b/tests/run-shape.bats @@ -28,17 +28,10 @@ setup() { # else. The debug arm is what keeps these controls running in CI rather than # skipping there, which for a suite whose whole job is proving the gate # decides would be coverage evaporating exactly where it counts. - 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" + # `batten_binary` rather than a release-first chain: `test:bats` builds DEBUG, + # so a leftover release binary shadowed it and this suite would report on a + # build older than the code under test (CLOUD-859). + BIN=$(batten_binary "$BATS_TEST_DIRNAME/..") || skip "no batten binary to drive" MODULE="$BATS_TEST_DIRNAME/../policy/run-shape.rego" REPO="$BATS_TEST_TMPDIR/repo" diff --git a/tests/stop-guard.bats b/tests/stop-guard.bats index 80f74c220..55a4f09d9 100644 --- a/tests/stop-guard.bats +++ b/tests/stop-guard.bats @@ -6,6 +6,8 @@ # in-session proof that the entry exists at all. setup() { + # For `batten_binary`, which `unlanded_stub` delegates to (CLOUD-859). + load helpers GUARD="$BATS_TEST_DIRNAME/../mise-tasks/stop-guard.sh" SETTINGS="$BATS_TEST_DIRNAME/../.claude/settings.json" cd "$BATS_TEST_DIRNAME/.." || return 1 @@ -381,8 +383,11 @@ unlanded_stub() { # `state list` silently empties the payload every rule above reads — and the # suite then measures the reader being broken rather than the rule being # ordered. Cost one debugging round; stated here so it costs nobody another. - local real="$BATS_TEST_DIRNAME/../target/release/batten" - [ -x "$real" ] || real="$BATS_TEST_DIRNAME/../target/debug/batten" + # `batten_binary` rather than release-first: `test:bats` builds DEBUG, so a + # leftover release binary shadowed it and the stub would delegate to a build + # older than the code under test (CLOUD-859). + local real + real=$(batten_binary "$BATS_TEST_DIRNAME/..") || skip "no batten binary to delegate to" { echo '#!/usr/bin/env bash' echo 'if [ "$1" = "state" ] && [ "$2" = "list" ]; then'