Skip to content

feat(classify): fail closed on bracket patterns in risk-paths.yml#107

Merged
topcoder1 merged 1 commit into
mainfrom
fix/classify-bracket-guard
Jul 5, 2026
Merged

feat(classify): fail closed on bracket patterns in risk-paths.yml#107
topcoder1 merged 1 commit into
mainfrom
fix/classify-bracket-guard

Conversation

@topcoder1

Copy link
Copy Markdown
Owner

Why

wxa-jake-ai#783 (2026-07-04) uncovered a whole bug class: a [ in a risk-paths pattern is a minimatch character class, never a literal bracket, so a SvelteKit-style entry like src/routes/api/chat/[id]/stream/+server.ts silently matches nothing — and GitHub CODEOWNERS drops any line containing brackets, so the lockstep mirror dies the same way. That repo's stream gate was a silent no-op from 2026-05-24 to 2026-07-04.

Fleet audit (2026-07-04, same day)

All 43 repos using the pr-classify pipeline were scanned (.github/risk-paths.yml + CODEOWNERS, every non-comment line containing [):

  • 1 real hit: wxa-jake-ai — already fixed by the open wxa-jake-ai#783 (manual-merge pending)
  • 6 repos matched only on sensitive: [] — benign empty YAML flow arrays, not patterns
  • Installer templates (dotclaude + ~/.claude/templates/ci-workflows/) — clean
  • No other repo has any bracket pattern, in always_review: lists included

So this guard can merge without breaking any caller except wxa-jake-ai (see merge order).

What

classify.mjs now fails closed on any pattern containing [ or ] across blocked/sensitive/safe_*/trivial and always_review — the latter because codex-gate.mjs consumes it with the same minimatch semantics but is deliberately permissive on config errors, so this pass is the only fail-closed check that list gets before a dead entry silently skips a required Codex review. The error message gives the concrete rewrite (* for a dynamic segment, parent ** glob) and tells the author to mirror the fix into CODEOWNERS.

Intentional strictness: legitimate character classes like *.[jt]s are banned too, with an "enumerate as *.js + *.ts" hint. [id] is syntactically a valid char class, so no check can tell intent apart — a heuristic that guesses wrong recreates the silent dead gate. The fleet audit found zero real char-class uses, so strictness costs nothing today.

New selftest/test_classify_bracket_guard.sh (wired into test_workflow_guards.py, so uv run pytest enforces it on this repo's PRs) bakes in:

  1. bracket pattern → exit 1 with actionable message
  2. the recommended * rewrite classifies the literal [id] path as sensitive under the classifier's exact minimatch options (the #783 fix, verified)
  3. clean rules file classifies normally (no false positive)
  4. intentional char class rejected with the enumeration hint
  5. bracket under always_review: fails closed

⚠️ Merge order — fleet-live on merge

pr-classify.yml fetches classify.mjs from this repo's main at run time, so the guard is live fleet-wide the moment this merges. wxa-jake-ai's main still has the bracketed entry until wxa-jake-ai#783 merges — merge #783 first, or every wxa-jake-ai PR's classify / Classify PR Risk required check goes red (loud, not dangerous, but blocks that repo).

Validation

  • bash selftest/test_classify_bracket_guard.sh — 5/5 ✓
  • uv run pytest -q — 7 passed (new selftest runs via the wrapper)
  • ruff check + ruff format --check — clean

Auto-merge rationale: manual-merge by definition — PRs to ci-workflows are always manual, and this touches the classifier itself (blocked-class fleet-wide surface).

Codex rounds: 4 — R1 char-class false-positive concern → addressed via explicit enumeration hint + selftest case 4; R2 always_review coverage gap → guard extended + selftest case 5; R3, R4 clean.

🤖 Generated with Claude Code

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 <noreply@anthropic.com>
@github-actions github-actions Bot added the risk:sensitive Risk class: sensitive label Jul 5, 2026
@github-actions

github-actions Bot commented Jul 5, 2026

Copy link
Copy Markdown

Codex review

no regressions found
no regressions found

@github-actions

github-actions Bot commented Jul 5, 2026

Copy link
Copy Markdown

Coverage Floor — mode: enforce

metric value
measured 100.0%
floor (current) 99.0%
target 100.0%
last bumped 2026-05-12

@claude

claude Bot commented Jul 5, 2026

Copy link
Copy Markdown

No issues found. Bracket guard logic is correct, always_review coverage is justified, and the 5-case selftest covers the new branches.

@topcoder1 topcoder1 merged commit 29ac283 into main Jul 5, 2026
13 checks passed
@topcoder1 topcoder1 deleted the fix/classify-bracket-guard branch July 5, 2026 17:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk:sensitive Risk class: sensitive

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant