From e689c6c8d4f81bd53e11b169d887a6f676abc57a Mon Sep 17 00:00:00 2001 From: Jonathan Zhang Date: Sat, 4 Jul 2026 22:18:07 -0700 Subject: [PATCH] feat(classify): fail closed on bracket patterns in risk-paths.yml MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A '[' in a minimatch pattern is a character class, never a literal bracket — so a SvelteKit-style 'src/routes/[id]/...' entry silently matches nothing, and the CODEOWNERS mirror dies too (GitHub ignores bracket lines). wxa-jake-ai's stream gate was a no-op for six weeks this way (wxa-jake-ai#783). classify.mjs now rejects any bracket in blocked/sensitive/safe_*/trivial AND always_review (consumed by the permissive-by-design codex-gate.mjs, so this is its only fail-closed pass) with an actionable rewrite hint. New selftest bakes in the guard, the '*' rewrite equivalence, and the intentional char-class ban; wired into test_workflow_guards.py so CI enforces it. Co-Authored-By: Claude Fable 5 --- .github/scripts/classify.mjs | 34 +++++++ selftest/test_classify_bracket_guard.sh | 114 ++++++++++++++++++++++++ selftest/test_workflow_guards.py | 1 + 3 files changed, 149 insertions(+) create mode 100644 selftest/test_classify_bracket_guard.sh diff --git a/.github/scripts/classify.mjs b/.github/scripts/classify.mjs index 50cdd24..a44c8ca 100644 --- a/.github/scripts/classify.mjs +++ b/.github/scripts/classify.mjs @@ -49,6 +49,40 @@ try { fail(`failed to read ${RULES_PATH}: ${e.message}`); } +// A '[' in a pattern is a minimatch character class, never a literal +// bracket — a SvelteKit-style 'src/routes/[id]/+page.ts' entry silently +// matches nothing, and GitHub CODEOWNERS drops bracket lines entirely, so +// the gate the author thinks exists doesn't. That exact failure shipped +// once: wxa-jake-ai's stream gate was a no-op for six weeks +// (wxa-jake-ai#783). ALL brackets are banned, including intentional +// character classes like '*.[jt]s': '[id]' is syntactically a valid char +// class too, so no check can tell intent apart, and a heuristic that +// guesses wrong recreates the silent dead gate. Enumerate instead +// ('*.js' + '*.ts') — the 2026-07-04 fleet audit found zero real +// char-class uses, so strictness costs nothing. Fail closed. +// +// 'always_review' is validated here too even though this script never +// matches against it: codex-gate.mjs consumes it with the same minimatch +// semantics but is deliberately permissive on config errors, so this +// fail-closed pass is the only place a dead always_review entry gets +// caught before it silently skips a required Codex review. +for (const cls of [...PATTERN_CLASSES, 'always_review']) { + for (const p of rules[cls] || []) { + if (typeof p === 'string' && (p.includes('[') || p.includes(']'))) { + fail( + `${RULES_PATH}: pattern '${p}' (under '${cls}:') contains a bracket — ` + + `minimatch reads '[...]' as a character class, so a literal path like a SvelteKit ` + + `'[id]' segment can never match. Replace the bracket segment with '*' ` + + `(e.g. 'src/routes/api/chat/*/stream/+server.ts') or use a parent '**' glob, ` + + `and mirror the same fix in .github/CODEOWNERS (GitHub ignores bracket lines there). ` + + `If you meant a real character class like '*.[jt]s', enumerate it instead ` + + `('*.js' + '*.ts') — brackets are banned outright because intent is ambiguous. ` + + `Context: wxa-jake-ai#783.` + ); + } + } +} + const changedFiles = readFileSync(0, 'utf8') .split('\n') .map((s) => s.trim()) diff --git a/selftest/test_classify_bracket_guard.sh b/selftest/test_classify_bracket_guard.sh new file mode 100644 index 0000000..54036e8 --- /dev/null +++ b/selftest/test_classify_bracket_guard.sh @@ -0,0 +1,114 @@ +#!/usr/bin/env bash +# Guards classify.mjs's bracket-pattern validation. +# +# minimatch treats '[...]' as a character class, so a literal SvelteKit-style +# entry like 'src/routes/api/chat/[id]/stream/+server.ts' in risk-paths.yml +# can never match its own file — the gate is a silent no-op. GitHub +# CODEOWNERS additionally ignores any line containing brackets, so the +# lockstep CODEOWNERS mirror dies the same way. This exact failure shipped +# in wxa-jake-ai (stream gate dead 2026-05-24 → 2026-07-04, fixed in +# wxa-jake-ai#783; fleet audited same day — no other repo affected). +# classify.mjs now fails closed on bracket patterns; this test bakes that +# behavior AND the recommended '*' rewrite into version control. +# +# Run from the repo root: +# bash selftest/test_classify_bracket_guard.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 + +# 1. A bracket pattern must fail closed with an actionable message. +cat > "$tmp/repo/.github/risk-paths.yml" <<'YAML' +blocked: [] +sensitive: + - 'src/routes/api/chat/[id]/stream/+server.ts' +YAML +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 "character class"; then + echo "✓ bracket pattern fails closed with an actionable message" +else + echo "✗ expected nonzero exit + 'character class' in message; got rc=$rc:" + echo "$out" | sed 's/^/ /' + failed=1 +fi + +# 2. The recommended '*' rewrite actually matches the real bracketed path +# (this is the wxa-jake-ai#783 fix, verified against the classifier's own +# minimatch options). +cat > "$tmp/repo/.github/risk-paths.yml" <<'YAML' +blocked: [] +sensitive: + - 'src/routes/api/chat/*/stream/+server.ts' +YAML +cls=$(cd "$tmp/repo" && echo "src/routes/api/chat/[id]/stream/+server.ts" | node "$tmp/classify.mjs") +if [ "$cls" = "sensitive" ]; then + echo "✓ '*' rewrite classifies the literal [id] path as sensitive" +else + echo "✗ expected 'sensitive' for the [id] path under the '*' rewrite, got '$cls'" + failed=1 +fi + +# 3. A clean rules file still classifies normally (no false positive). +cls=$(cd "$tmp/repo" && echo "README.md" | node "$tmp/classify.mjs") +if [ "$cls" = "standard" ]; then + echo "✓ clean rules file classifies normally" +else + echo "✗ expected 'standard' for README.md, got '$cls'" + failed=1 +fi + +# 4. Intentional character classes are ALSO rejected — this is a decision, +# not an oversight. '[id]' is syntactically a valid char class, so intent +# can't be told apart; authors must enumerate ('*.js' + '*.ts'). The +# message must say so. +cat > "$tmp/repo/.github/risk-paths.yml" <<'YAML' +blocked: [] +safe_test: + - 'tests/**/*.[jt]s' +YAML +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 "enumerate"; then + echo "✓ intentional char class rejected with the enumeration hint" +else + echo "✗ expected nonzero exit + 'enumerate' in message for char-class pattern; got rc=$rc:" + echo "$out" | sed 's/^/ /' + failed=1 +fi + +# 5. 'always_review' is validated too: classify.mjs never matches against it, +# but codex-gate.mjs consumes it with the same minimatch semantics and is +# permissive on config errors — this guard is the only fail-closed pass +# over that list. +cat > "$tmp/repo/.github/risk-paths.yml" <<'YAML' +blocked: [] +always_review: + - 'src/routes/[id]/+page.ts' +YAML +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 "always_review"; then + echo "✓ bracket pattern under always_review fails closed" +else + echo "✗ expected nonzero exit + 'always_review' in message; got rc=$rc:" + echo "$out" | sed 's/^/ /' + failed=1 +fi + +exit "$failed" diff --git a/selftest/test_workflow_guards.py b/selftest/test_workflow_guards.py index f23c7dd..183ae39 100644 --- a/selftest/test_workflow_guards.py +++ b/selftest/test_workflow_guards.py @@ -21,6 +21,7 @@ "script", [ "selftest/test_automerge_risk_patterns.sh", + "selftest/test_classify_bracket_guard.sh", "selftest/test_pr_files_listing.sh", "selftest/test_prettier_symlink_filter.sh", ],