From cc2b80717c906dd2733c5ebe7607fe8d9b212e15 Mon Sep 17 00:00:00 2001 From: Alec Wenzowski Date: Fri, 21 Aug 2026 22:20:03 +0000 Subject: [PATCH] ci(gate): hold `.coderabbit.yaml` to the three keys it was landed for MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CLOUD-847 measured what each key buys and landed the file; nothing then held it to those readings, so a rule shipped without a mechanism — the shape non-negotiable rule 2 names, and a punt on this session's own landed work. The three keys are what the lifecycle rests on. `request_changes_workflow` is what makes findings reach `reviewDecision` at all; `auto_review.drafts` is what puts a review in the phase that costs nothing (every job in ci.yml is `if: draft == false`); `tools.gitleaks.enabled` is the only secret scanning a draft gets, for that same reason. The failure this refuses is silent in the worst direction: flipping `drafts` back is a one-line diff whose symptom is reviews quietly not happening, which looks exactly like nobody having pushed — and the gate that consumes the config would take the blame for a review the config stopped producing. Absent is not passing for the first two, since a key nobody wrote and a key someone deleted leave the same default in force. `gitleaks` is the inverse: its default is enabled, so only an explicit `false` is a violation. The gitleaks arm is scoped to its own block because `enabled:` appears once per tool, and reading it unscoped would answer about `clippy` — which is deliberately false, and would have made the gate refuse a compliant file. Closes CLOUD-860 --- hk.pkl | 16 +++ mise-tasks/coderabbit-config-check | 139 +++++++++++++++++++++++++ tests/coderabbit-config-check.bats | 156 +++++++++++++++++++++++++++++ 3 files changed, 311 insertions(+) create mode 100755 mise-tasks/coderabbit-config-check create mode 100644 tests/coderabbit-config-check.bats diff --git a/hk.pkl b/hk.pkl index 2c30935d8..1fc38f7d2 100644 --- a/hk.pkl +++ b/hk.pkl @@ -173,6 +173,22 @@ local gate = new Mapping { ["license-table-check"] { check = "mise run license-table-check" } + // The mechanism CLOUD-847 shipped without (CLOUD-860). That row landed + // `.coderabbit.yaml` after measuring what each key buys — a formal + // CHANGES_REQUESTED review so a verdict exists at all, review on drafts so it + // exists in the phase that costs nothing, and `gitleaks` kept because + // `mise run ci` is `if: draft == false` and a draft would otherwise have no + // secret scanning. Nothing then held the file to those readings, and the + // failure is silent in the worst direction: reviews simply stop, which looks + // exactly like nobody having pushed. + // + // Globbed on the one file it reads, unlike its glob-less neighbours above: a + // commit touching nothing else cannot change the answer, so the whole-index + // scan they need would only cost time here. + ["coderabbit-config-check"] { + glob = List(".coderabbit.yaml") + check = "mise run coderabbit-config-check" + } // The two-tier gate ships with its mechanism (non-negotiable 2). The steps // below marked `profiles = List("slow")` are skipped at pre-commit and run by // `check`, and the dangerous direction is not the obvious one: a tier that diff --git a/mise-tasks/coderabbit-config-check b/mise-tasks/coderabbit-config-check new file mode 100755 index 000000000..5eeb9f7af --- /dev/null +++ b/mise-tasks/coderabbit-config-check @@ -0,0 +1,139 @@ +#!/usr/bin/env bash +#MISE description="Gate: .coderabbit.yaml still carries the keys the review lifecycle depends on (pointer-only)" +# +# CLOUD-860, and the missing half of CLOUD-847. That row landed `.coderabbit.yaml` +# and measured every key in it; nothing then held the file to those readings, so +# a rule shipped without a mechanism — the shape non-negotiable rule 2 names. +# +# THE THREE KEYS ARE NOT A STYLE PREFERENCE, they are what the lifecycle rests on: +# +# request_changes_workflow findings arrive as a formal CHANGES_REQUESTED review, +# so `reviewDecision` carries an answer. Off, the bot +# only COMMENTS and the decision stays null. +# auto_review.drafts the draft phase is the free phase (every job in +# ci.yml is `if: draft == false`). Off, nothing reviews +# it, and the review can only arrive after the ready — +# which is the whole defect CLOUD-847 measured. +# tools.gitleaks.enabled the ONLY secret scanning a draft gets, for the same +# reason: `mise run ci` does not run on drafts. The +# other linters are deliberately off because our gates +# already run them; this one is deliberately kept. +# +# WHY THE FAILURE IS WORTH A GATE RATHER THAN A COMMENT. Flipping `drafts` back is +# a one-line diff, and its symptom is silence: reviews stop happening, which looks +# exactly like nobody having pushed. The gate that consumes the config would then +# refuse every PR for want of a review the config quietly stopped producing, and +# the visible failure would be the gate rather than the cause. +# +# ABSENT IS NOT PASSING for the first two, and that asymmetry is the point: a key +# nobody wrote and a key someone deleted are the same file, and both leave the +# default in force — which is the value this gate exists to refuse. `gitleaks` is +# the inverse, because its default IS enabled: only an explicit `false` is a +# violation there, so absence passes. +# +# Pointer-only per non-negotiable rule 4: `path:line key=value` and a count, never +# a byte of the file. A config can carry instructions and paths, and a gate that +# echoed them would put them in every CI log. +# +# Exit 0 every required key holds / 2 a key is missing or flipped. No fail-open +# arm, unlike a gate that reads GitHub: the input is a tracked file in this +# checkout, so "could not look" means the file is gone, which is itself the +# violation this refuses. +# +# The mutation drops the empty-file guard, so a file with no keys reports zero +# violations — the vacuous pass this gate is shaped to avoid, and only the +# empty-fixture case can catch it. +#MUTANT empty-file-is-a-pass|s/if \[ "\$keys" -eq 0 \]/if false/|a file with no keys must not read as compliant +set -euo pipefail + +CFG="${1:-.coderabbit.yaml}" + +if [ ! -f "$CFG" ]; then + echo "::error:: coderabbit-config-check: $CFG is absent — the review lifecycle has no configuration to rest on" >&2 + exit 2 +fi + +violations=0 +report() { # $1 = line (0 when the key is absent), $2 = pointer + echo "$CFG:$1 $2" >&2 + violations=$((violations + 1)) +} + +# A key line at any indentation, reported with its line number. The file is ours +# and its shape is reviewed; this reads the key rather than the tree, which is +# what keeps the gate to one awk pass and no YAML dependency. +key_line() { # $1 = key name -> "linevalue", empty when absent + awk -v key="$1" ' + $0 ~ "^[[:space:]]*#" { next } + { + pattern = "^[[:space:]]*" key ":[[:space:]]*" + if ($0 ~ pattern) { + value = $0 + sub(pattern, "", value) + sub("[[:space:]]*#.*$", "", value) + sub("[[:space:]]+$", "", value) + print NR "\t" value + exit + } + } + ' "$CFG" +} + +# `enabled:` appears once per tool, so this one is scoped: find the tool, then read +# the first `enabled:` inside its block. Reading it unscoped would answer about +# whichever tool happened to come first in the file. +tool_enabled() { # $1 = tool name -> "linevalue", empty when the tool is absent + awk -v tool="$1" ' + $0 ~ "^[[:space:]]*#" { next } + !seen && $0 ~ ("^[[:space:]]*" tool ":[[:space:]]*$") { seen = 1; next } + seen && $0 ~ "^[[:space:]]*enabled:[[:space:]]*" { + value = $0 + sub("^[[:space:]]*enabled:[[:space:]]*", "", value) + sub("[[:space:]]*#.*$", "", value) + sub("[[:space:]]+$", "", value) + print NR "\t" value + exit + } + seen && $0 ~ "^[[:space:]]*[A-Za-z0-9_-]+:[[:space:]]*$" { exit } + ' "$CFG" +} + +# The vacuity guard, and the reason it is first: every check below is an assertion +# ABOUT a key, so a file carrying none of them satisfies all of them by having +# nothing to judge. Counting real keys turns "no violations found" back into a +# statement about a file that was actually read. +keys=$(grep -cE '^[[:space:]]*[A-Za-z0-9_-]+:' "$CFG" || true) +if [ "$keys" -eq 0 ]; then + echo "::error:: coderabbit-config-check: $CFG carries no keys at all, so every assertion below would pass vacuously" >&2 + exit 2 +fi + +require_true() { # $1 = key name + local hit line value + hit=$(key_line "$1") + if [ -z "$hit" ]; then + report 0 "$1 absent (default is in force)" + return + fi + line=${hit%% *} + value=${hit#* } + [ "$value" = "true" ] || report "$line" "$1=$value (want true)" +} + +require_true "request_changes_workflow" +require_true "drafts" + +# The inverse arm: gitleaks defaults to enabled, so absence is compliant and only +# an explicit `false` is the violation. +hit=$(tool_enabled "gitleaks") +if [ -n "$hit" ]; then + line=${hit%% *} + value=${hit#* } + [ "$value" != "false" ] || report "$line" "tools.gitleaks.enabled=false (drafts would have no secret scanning)" +fi + +if [ "$violations" -ne 0 ]; then + echo "::error:: coderabbit-config-check: $violations violation(s) in $CFG — see CLOUD-847 for what each key was measured to do" >&2 + exit 2 +fi +echo "coderabbit-config-check: $CFG holds the three keys the review lifecycle depends on" diff --git a/tests/coderabbit-config-check.bats b/tests/coderabbit-config-check.bats new file mode 100644 index 000000000..2177c4e9d --- /dev/null +++ b/tests/coderabbit-config-check.bats @@ -0,0 +1,156 @@ +#!/usr/bin/env bats +# subject: mise-tasks/coderabbit-config-check +# The mechanism CLOUD-847 shipped without (CLOUD-860). +# +# That row landed `.coderabbit.yaml` and measured what each key buys; nothing then +# held the file to those readings. These cases pin both directions per CLOUD-418 — +# each of the three keys refused when flipped, the real file passing — plus the +# vacuity case a key-absence check needs, where a file carrying no keys satisfies +# every per-key assertion by having none to judge. + +setup() { + CHECK="$BATS_TEST_DIRNAME/../mise-tasks/coderabbit-config-check" + cd "$BATS_TEST_DIRNAME/.." || return 1 +} + +# The same shape as the real file, so a fixture exercises the parser rather than a +# simplified stand-in — including the sibling tools whose `enabled:` keys are what +# make the gitleaks arm need scoping. +write_config() { + # $1 = destination, $2 = request_changes_workflow, $3 = drafts, $4 = gitleaks enabled + cat >"$1" <"$BATS_TEST_TMPDIR/c.yaml" <<'EOF' +reviews: + request_changes_workflow: true + auto_review: + drafts: true +EOF + run "$CHECK" "$BATS_TEST_TMPDIR/c.yaml" + [ "$status" -eq 0 ] +} + +@test "a key deleted rather than flipped fails: absence leaves the default in force" { + # The two ways a key stops holding are the same file to CodeRabbit, so they + # must be the same verdict here. + cat >"$BATS_TEST_TMPDIR/c.yaml" <<'EOF' +reviews: + auto_review: + drafts: true +EOF + run "$CHECK" "$BATS_TEST_TMPDIR/c.yaml" + [ "$status" -eq 2 ] + [[ "$output" == *"request_changes_workflow absent"* ]] +} + +@test "a comment-only file is a failure, not a vacuous pass" { + # The case the whole gate is shaped around: every check is an assertion ABOUT + # a key, so a file with none satisfies all of them by having nothing to judge. + printf '# every key deleted\n# but the file still exists\n' >"$BATS_TEST_TMPDIR/c.yaml" + run "$CHECK" "$BATS_TEST_TMPDIR/c.yaml" + [ "$status" -eq 2 ] + [[ "$output" == *"no keys at all"* ]] +} + +@test "a commented-out key does not satisfy the assertion" { + # `# drafts: true` is the shape of a key someone disabled while leaving a note. + cat >"$BATS_TEST_TMPDIR/c.yaml" <<'EOF' +reviews: + request_changes_workflow: true + auto_review: + # drafts: true + auto_pause_after_reviewed_commits: 50 +EOF + run "$CHECK" "$BATS_TEST_TMPDIR/c.yaml" + [ "$status" -eq 2 ] + [[ "$output" == *"drafts absent"* ]] +} + +@test "an absent file fails rather than passing for want of anything to read" { + run "$CHECK" "$BATS_TEST_TMPDIR/does-not-exist.yaml" + [ "$status" -eq 2 ] + [[ "$output" == *"absent"* ]] +} + +@test "output is pointer-only: it names keys and lines, never the file's contents" { + # Non-negotiable rule 4. A config carries paths and instructions, and a gate + # that echoed them would put them in every CI log. + write_config "$BATS_TEST_TMPDIR/c.yaml" false true true + run "$CHECK" "$BATS_TEST_TMPDIR/c.yaml" + [[ "$output" != *"auto_pause_after_reviewed_commits"* ]] + [[ "$output" != *"high_level_summary"* ]] +}