diff --git a/.github/scripts/classify.mjs b/.github/scripts/classify.mjs index a44c8ca..ae1fc60 100644 --- a/.github/scripts/classify.mjs +++ b/.github/scripts/classify.mjs @@ -36,6 +36,10 @@ const PRIORITY = [ // Classes we test patterns against. `standard` is not in this list — it's // the fallback for any file that matches NO pattern in any class. const PATTERN_CLASSES = ['blocked', 'sensitive', 'safe_test', 'safe_deps', 'safe_config', 'trivial']; +// Classes matched case-insensitively. See the invariant on classify() below: +// folding case may only ever ADD gating, never remove it — which is exactly +// why the safe/trivial classes are absent here. +const NOCASE_CLASSES = new Set(['blocked', 'sensitive']); function fail(msg) { process.stderr.write(`classify.mjs: ${msg}\n`); @@ -83,6 +87,32 @@ for (const cls of [...PATTERN_CLASSES, 'always_review']) { } } +// Glob negation is incompatible with the case-fold applied to the gating +// classes, and breaks its one invariant. '!' inverts the match, so folding +// case REMOVES gating rather than adding it: minimatch('FOO', '!foo') is true +// (gated) but false under {nocase:true} (ungated) — a downgrade. Segment +// extglobs have the same shape: 'src/!(*.md)' matches 'src/A.MD' today and +// stops matching once case is folded. Fail closed rather than quietly violate +// the invariant classify() documents. Zero of the 45 repos carrying a +// risk-paths.yml use negation in a gating class (fleet audit 2026-07-14), so +// — as with the bracket guard above — strictness costs nothing today and +// stops the footgun from ever being introduced. (Codex round-2 P2 on the +// change that introduced the fold.) +for (const cls of NOCASE_CLASSES) { + for (const p of rules[cls] || []) { + if (typeof p === 'string' && (p.trimStart().startsWith('!') || p.includes('!('))) { + fail( + `${RULES_PATH}: pattern '${p}' (under '${cls}:') uses glob negation — ` + + `'${cls}' is matched case-insensitively so that a lowercase pattern still ` + + `catches real-world case variants, and negation inverts that: folding case ` + + `REMOVES gating instead of adding it (minimatch('FOO','!foo') is true, but ` + + `false with nocase). Express the rule positively — list the paths you want ` + + `gated rather than the ones you don't. Context: wxa-jake-ai#877.` + ); + } + } +} + const changedFiles = readFileSync(0, 'utf8') .split('\n') .map((s) => s.trim()) @@ -95,11 +125,53 @@ if (changedFiles.length === 0) { process.exit(0); } +// Case-folding is applied to the GATING classes only. The invariant: +// +// folding case may only ever ADD gating, never remove it. +// +// Why fold at all: minimatch defaults to case-SENSITIVE, so a lowercase +// pattern silently misses real-world case variants. '**/secrets*' matched +// 'docs/secrets.md' but NOT 'docs/SECRETS.md', so wxa-jake-ai's production +// secrets ROTATION RUNBOOK fell through to 'docs/**' and classified +// risk:trivial — auto-merge-eligible (wxa-jake-ai#875 had to be held as a +// draft to dodge it; fixed repo-side in wxa-jake-ai#877). The same latent gap +// exists for 'Dockerfile' (a committed 'dockerfile'/'DOCKERFILE') and the +// '.env' family, in every repo in the fleet. +// +// Why NOT fold the safe/trivial classes: doing so is a fail-OPEN. A path that +// matches nothing today gets the deliberately-strict 'standard' fallback; +// folding case can hand it to an auto-merge-eligible class instead. With +// `safe_test: ['tests/**']`, a PR adding 'Tests/release.py' would classify +// safe_test rather than standard — and on GitHub's case-sensitive filesystem +// that is a genuinely DISTINCT path, not the same file recased, so a +// lowercase pattern has no business claiming it. Class precedence cannot +// prevent this: it only breaks ties when a blocked/sensitive pattern also +// matches, and here none does. Folding the safe classes has no upside either +// — its only effect is to make them more lenient, which is precisely the +// direction we don't want. (Caught by codex pre-review on this change; the +// fleet audit below could not have found it, since it scanned files that +// already exist and this vector is about files a future PR introduces.) +// +// So the asymmetry is the point, not an oversight: blocked/sensitive can only +// grow, safe/trivial can only shrink-or-stay. selftest/test_classify_nocase.sh +// pins both halves. +// +// Fleet audit before shipping, 2026-07-14 — every blob in all 45 repos +// carrying a risk-paths.yml (18,604 files) classified twice, fold off vs on: +// ZERO downgrades, exactly 2 upgrades, both real secrets docs (wxa-jake-ai +// 'docs/SECRETS.md', inbox_superpilot 'docs/SECRETS_ROTATION.md'). Both are +// blocked:-class hits, so both still land under this narrower fold. +// +// NOTE: this does NOT fix .github/CODEOWNERS, which GitHub matches itself and +// also case-sensitively ("CODEOWNERS paths are case sensitive, because GitHub +// uses a case sensitive file system"). A repo relying on a lowercase glob to +// own an uppercase path still needs an exact-case CODEOWNERS line. function classify(file) { for (const cls of PATTERN_CLASSES) { const patterns = rules[cls] || []; for (const p of patterns) { - if (minimatch(file, p, { dot: true, matchBase: false })) return cls; + const opts = { dot: true, matchBase: false, nocase: NOCASE_CLASSES.has(cls) }; + if (minimatch(file, p, opts)) return cls; } } return 'standard'; diff --git a/selftest/README.md b/selftest/README.md index b7141c3..700686e 100644 --- a/selftest/README.md +++ b/selftest/README.md @@ -21,6 +21,15 @@ arbitrary helper scripts; that's a different kind of repo. - `test_automerge_risk_patterns.sh` / `test_bb_automerge_risk_patterns.sh` — risk-tier regex behavior, driven by the shared corpus in `risk_patterns_corpus.txt`. +- `test_classify_nocase.sh` — `classify.mjs` case-folds pattern matching for + `blocked`/`sensitive` **only**. minimatch defaults to case-sensitive, so a + lowercase `**/secrets*` missed `docs/SECRETS.md` and a production secrets + rotation runbook classified `risk:trivial` (wxa-jake-ai#875 / #877). Pins + the fix _and_ the asymmetry that makes it safe — folding may only ever add + gating, never remove it — so folding the safe/trivial classes is rejected: + it would demote an unmatched path from the strict `standard` fallback into + an auto-merge-eligible class (a PR adding `Tests/release.py` under + `safe_test: ['tests/**']`). - `test_pr_files_listing.sh` — no reusable may fetch changed files via `gh pr diff` (HTTP 406 past 20k diff lines); pins the paginated files-API idiom instead. diff --git a/selftest/test_classify_nocase.sh b/selftest/test_classify_nocase.sh new file mode 100644 index 0000000..64af67c --- /dev/null +++ b/selftest/test_classify_nocase.sh @@ -0,0 +1,185 @@ +#!/usr/bin/env bash +# Guards classify.mjs's case-insensitive path matching. +# +# minimatch defaults to case-SENSITIVE, so a lowercase-authored pattern +# silently misses real-world case variants. '**/secrets*' in wxa-jake-ai's +# `blocked:` matched 'docs/secrets.md' but NOT 'docs/SECRETS.md' — so the +# production secrets ROTATION RUNBOOK, whose shell snippets are pasted +# against prod hosts, fell through to 'docs/**' and classified risk:trivial +# ("zero runtime impact"), auto-merge-eligible. wxa-jake-ai#875 had to be +# opened as a draft to dodge it; the repo-side exact-case gate is +# wxa-jake-ai#877. classify.mjs now matches with {nocase:true}; this test +# bakes that into version control. +# +# Fleet evidence gathered before shipping (2026-07-14): every blob in all 45 +# repos carrying a risk-paths.yml (18,604 files) was classified twice, nocase +# off vs on. ZERO downgrades fleet-wide; exactly 2 upgrades, both real secrets +# docs (wxa-jake-ai 'docs/SECRETS.md', inbox_superpilot +# 'docs/SECRETS_ROTATION.md'). +# +# Run from the repo root: +# bash selftest/test_classify_nocase.sh +set -euo pipefail + +tmp=$(mktemp -d) +trap 'rm -rf "$tmp"' EXIT + +# classify.mjs resolves its deps from its own location, so install them next +# to a copy in the temp dir — same versions pr-classify.yml pins. +cp .github/scripts/classify.mjs "$tmp/classify.mjs" +(cd "$tmp" && npm install --no-save --silent yaml@2 minimatch@10 >/dev/null 2>&1) + +mkdir -p "$tmp/repo/.github" +failed=0 + +# Assert `expected` is the class for `file` under the rules already written +# to $tmp/repo/.github/risk-paths.yml. +expect() { + local file="$1" expected="$2" desc="$3" got + got=$(cd "$tmp/repo" && echo "$file" | node "$tmp/classify.mjs") + if [ "$got" = "$expected" ]; then + echo "✓ $desc" + else + echo "✗ $desc — expected '$expected' for '$file', got '$got'" + failed=1 + fi +} + +# The real wxa-jake-ai shape that produced the bug: a lowercase 'secrets' glob +# in blocked:, and a docs/** + **/*.md catch-all in trivial:. Before nocase, +# 'docs/SECRETS.md' skipped blocked and landed in trivial. +cat > "$tmp/repo/.github/risk-paths.yml" <<'YAML' +blocked: + - '**/secrets*' + - 'Dockerfile' + - '**/.env' +sensitive: + - 'src/lib/server/auth.ts' +trivial: + - '**/*.md' + - 'docs/**' +YAML + +# 1. THE BUG. This is the whole point of the file. +expect "docs/SECRETS.md" blocked "uppercase SECRETS.md is blocked by a lowercase 'secrets' glob" + +# 2. The pre-existing lowercase behavior must not regress. +expect "docs/secrets.md" blocked "lowercase secrets.md still blocked" + +# 3. PRECEDENCE — the property that makes nocase fail-SAFE rather than just +# fail-safe-in-practice. 'docs/SECRETS.md' matches BOTH the blocked glob +# (only under case-folding) and the trivial 'docs/**' glob. PATTERN_CLASSES +# is ordered most-gated-first, so blocked must win. If someone ever +# reorders that list, case-folding starts handing files to trivial and this +# test fires. +expect "docs/SECRETS.md" blocked "blocked wins over a same-file trivial match (ordering intact)" + +# 4. Mixed case, not just upper — real repos have 'Secrets.yaml'. +expect "config/Secrets.yaml" blocked "mixed-case Secrets.yaml is blocked" + +# 5. Dockerfile case variants — latent in 44/45 fleet repos (no live hit yet). +expect "DOCKERFILE" blocked "DOCKERFILE is blocked by the 'Dockerfile' pattern" + +# 6. nocase must not swallow genuinely unrelated paths into blocked. +expect "docs/PLAN.md" trivial "an ordinary doc is still trivial" +expect "src/lib/server/auth.ts" sensitive "a sensitive path is unaffected" +expect "src/lib/server/thing.ts" standard "an unmatched path still falls back to standard" + +# 7. NO FAIL-OPEN: case-folding must never move a file OUT of blocked or +# sensitive. Construct the adversarial case directly — a path whose ONLY +# exact-case match is blocked, plus a trivial glob that would swallow it if +# class ordering or matching ever inverted. +cat > "$tmp/repo/.github/risk-paths.yml" <<'YAML' +blocked: + - 'infra/PROD_KEYS.md' +trivial: + - '**/*.md' +YAML +expect "infra/PROD_KEYS.md" blocked "an exact-case blocked entry is not downgraded by a trivial glob" +expect "infra/prod_keys.md" blocked "...and its lowercase variant is caught too, not left standard/trivial" + +# 8. The '.env' family keeps working (enumerated, not globbed, in most repos). +cat > "$tmp/repo/.github/risk-paths.yml" <<'YAML' +blocked: + - '**/.env' +trivial: + - '**/*.md' +YAML +expect ".env" blocked ".env still blocked" +expect "app/.ENV" blocked "uppercase .ENV is blocked too" + +# 9. THE ASYMMETRY — case-folding is applied to blocked/sensitive ONLY, and +# that is deliberate. Folding the safe/trivial classes is a fail-OPEN: a +# path matching nothing gets the strict 'standard' fallback, and folding +# would hand it to an auto-merge-eligible class instead. On GitHub's +# case-sensitive filesystem 'Tests/release.py' is a DISTINCT path from +# 'tests/release.py', so a lowercase 'tests/**' must not claim it. +# +# Class precedence does NOT protect against this — it only breaks ties when +# a blocked/sensitive pattern also matches, and here none does. These cases +# are about files a PR ADDS, which no audit of existing files can catch. +# (Raised as P1 by codex pre-review on the change that introduced folding.) +cat > "$tmp/repo/.github/risk-paths.yml" <<'YAML' +blocked: + - '**/secrets*' +safe_test: + - 'tests/**' +safe_config: + - '.vscode/**' +trivial: + - 'docs/**' +YAML +expect "tests/release.py" safe_test "exact-case safe_test path still matches" +expect "Tests/release.py" standard "case-variant of a safe_test glob stays standard, NOT safe_test" +expect "docs/notes.md" trivial "exact-case trivial path still matches" +expect "DOCS/deploy-prod.sh" standard "case-variant of a trivial glob stays standard, NOT trivial" +expect ".VSCode/tasks.json" standard "case-variant of a safe_config glob stays standard" +# ...while the gating classes DO still fold, in the same rules file. +expect "docs/SECRETS.md" blocked "blocked still folds case even as safe classes do not" + +# 10. NEGATION FAILS CLOSED in a folded class. '!' inverts the match, so +# case-folding would REMOVE gating rather than add it — the one thing the +# fold must never do. Verified directly: minimatch('FOO','!foo') is true, +# but false under {nocase:true}. Same shape for segment extglobs. +# Mirrors the bracket guard's fail-closed stance: no repo in the fleet +# uses negation in a gating class, so strictness costs nothing. +# (Codex round-2 P2.) +expect_fail_closed() { + local desc="$1" needle="$2" out rc + set +e + out=$(cd "$tmp/repo" && echo "README.md" | node "$tmp/classify.mjs" 2>&1) + rc=$? + set -e + if [ "$rc" -ne 0 ] && echo "$out" | grep -q "$needle"; then + echo "✓ $desc" + else + echo "✗ $desc — expected nonzero exit + '$needle'; got rc=$rc:" + echo "$out" | sed 's/^/ /' + failed=1 + fi +} + +cat > "$tmp/repo/.github/risk-paths.yml" <<'YAML' +blocked: + - '!foo' +YAML +expect_fail_closed "leading-'!' negation under blocked: fails closed" "negation" + +cat > "$tmp/repo/.github/risk-paths.yml" <<'YAML' +blocked: [] +sensitive: + - 'src/!(*.md)' +YAML +expect_fail_closed "segment extglob negation under sensitive: fails closed" "negation" + +# ...but negation in a NON-folded class is not this guard's business: those +# classes are still matched case-sensitively, so the invariant can't break +# there. Guarding them too would be scope creep beyond the fold. +cat > "$tmp/repo/.github/risk-paths.yml" <<'YAML' +blocked: [] +trivial: + - '!src/**' +YAML +expect "docs/x.md" trivial "negation in a non-folded class is left alone" + +exit "$failed" diff --git a/selftest/test_workflow_guards.py b/selftest/test_workflow_guards.py index 183ae39..a3a9d91 100644 --- a/selftest/test_workflow_guards.py +++ b/selftest/test_workflow_guards.py @@ -22,6 +22,7 @@ [ "selftest/test_automerge_risk_patterns.sh", "selftest/test_classify_bracket_guard.sh", + "selftest/test_classify_nocase.sh", "selftest/test_pr_files_listing.sh", "selftest/test_prettier_symlink_filter.sh", ],