From 0a33f98cc8a3e9304c55a0f2cecdce32aa89cd99 Mon Sep 17 00:00:00 2001 From: shin-core <153108882+shin-core@users.noreply.github.com> Date: Fri, 31 Jul 2026 17:14:03 +0900 Subject: [PATCH] fix(engine): count fnmatch label-pattern wildcard groups per raw star, not by path-glob rules labelPatternToRegExp reused change-guardrail's path-glob wildcard-group counter to guard against catastrophic backtracking, but that counter treats a ** pair as ONE group (the path compiler collapses ** into a single .*). The fnmatch compiler here has no ** concept and emits one .* per *, so the count and the compiled regex disagreed for any ** pattern: *a**b counted 2 but compiled 3 .* groups and was wrongly accepted, admitting a pattern this compiler builds into a catastrophic- backtracking RegExp on an adversarial near-miss label. Count one group per raw * (no ** pairing; ? and [..] are not counted) and compare against the shared MAX_GLOB_WILDCARD_GROUPS, now exported from change-guardrail rather than redeclared. An over-complex registry key degrades to the existing LABEL_PATTERN_NEVER_MATCHES and is still cached. The path-glob counter and every path consumer keep their **-is-one-group semantics unchanged. Closes #9994 --- .../src/scoring/label-match.ts | 31 ++++++++++++----- .../src/signals/change-guardrail.ts | 5 ++- .../loopover-engine/test/label-match.test.ts | 34 +++++++++++++++++++ test/unit/scoring.test.ts | 33 ++++++++++++++++-- 4 files changed, 92 insertions(+), 11 deletions(-) create mode 100644 packages/loopover-engine/test/label-match.test.ts diff --git a/packages/loopover-engine/src/scoring/label-match.ts b/packages/loopover-engine/src/scoring/label-match.ts index 1237563cbb..fe18c2350a 100644 --- a/packages/loopover-engine/src/scoring/label-match.ts +++ b/packages/loopover-engine/src/scoring/label-match.ts @@ -1,4 +1,4 @@ -import { hasUnsafeWildcardCount } from "../signals/change-guardrail.js"; +import { MAX_GLOB_WILDCARD_GROUPS } from "../signals/change-guardrail.js"; export function labelMatchesPattern(label: string, pattern: string): boolean { return labelPatternToRegExp(pattern.toLowerCase()).test(label.toLowerCase()); @@ -42,6 +42,19 @@ const LABEL_PATTERN_NEVER_MATCHES = /^(?!)$/; // change-guardrail.ts (there `*` stops at `/` and `?` is literal): labels are flat strings, so `*` matches any // run, `?` any single character, and `[seq]`/`[!seq]` a character class. Literal keys are unaffected — for a // pattern with no glob metacharacter the RegExp is an exact match, so existing configs score identically. + +/** Count the backtracking-capable wildcard GROUPS this fnmatch compiler will emit: one per raw `*` (each + * compiles to a `.*` below), with NO `**`-is-one-group rule — unlike change-guardrail's path-glob counter, + * this compiler has no globstar concept, so `**` is two `.*` groups, not one (#9994). `?` is not counted (it + * compiles to a single `.`, which cannot backtrack ambiguously), and neither are `[…]` classes. */ +function fnmatchWildcardGroups(pattern: string): number { + let count = 0; + for (let i = 0; i < pattern.length; i += 1) { + if (pattern.charAt(i) === "*") count += 1; + } + return count; +} + function labelPatternToRegExp(pattern: string): RegExp { const cached = labelPatternRegExpCache.get(pattern); if (cached !== undefined) { @@ -51,13 +64,15 @@ function labelPatternToRegExp(pattern: string): RegExp { labelPatternRegExpCache.set(pattern, cached); return cached; } - // Reuses change-guardrail.ts's wildcard-GROUP counting (a `*` here matches the same "any run of chars" - // semantics as that glob compiler's `*`, so the same catastrophic-backtracking risk and the same empirically- - // safe threshold apply) — an over-complex registry-sourced label_multipliers key degrades to a safe never-match - // instead of hanging RegExp.test() on an adversarial near-miss label (#2456). Reachable via the public - // score-preview API, the MCP tool, and the per-PR label-audit signal, so one bad registry entry could otherwise - // hang scoring for every PR on that repo. - if (hasUnsafeWildcardCount(pattern)) { + // Reject an over-complex registry-sourced label_multipliers key so it degrades to a safe never-match instead + // of hanging RegExp.test() on an adversarial near-miss label (#2456). Reachable via the public score-preview + // API, the MCP tool, and the per-PR label-audit signal, so one bad registry entry could otherwise hang + // scoring for every PR on that repo. Counting is fnmatch-specific, NOT change-guardrail's path-glob count: + // this compiler emits one `.*` per `*` with no `**` pairing (see below), so `**` is TWO backtracking groups + // here, not the single `.*` the path compiler collapses it into (#9994) — counting via that predicate would + // undercount `**` and admit a glob this compiler builds into a catastrophic-backtracking RegExp. The threshold + // itself is the shared MAX_GLOB_WILDCARD_GROUPS, so the two surfaces stay on one empirically-safe boundary. + if (fnmatchWildcardGroups(pattern) > MAX_GLOB_WILDCARD_GROUPS) { setLabelPatternRegExpCacheEntry(pattern, LABEL_PATTERN_NEVER_MATCHES); return LABEL_PATTERN_NEVER_MATCHES; } diff --git a/packages/loopover-engine/src/signals/change-guardrail.ts b/packages/loopover-engine/src/signals/change-guardrail.ts index e6a14969fd..1f79acd933 100644 --- a/packages/loopover-engine/src/signals/change-guardrail.ts +++ b/packages/loopover-engine/src/signals/change-guardrail.ts @@ -34,7 +34,10 @@ export function canonicalize(value: string): string { // protected automatically rather than needing to separately remember the risk. The boundary is set at the // highest GROUP count proven safe by the benchmark above (2) — a boundary that itself sits inside the // empirically dangerous range would defeat the point of a cap. -const MAX_GLOB_WILDCARD_GROUPS = 2; +// Exported (#9994) so label-match.ts's fnmatch compiler applies the SAME empirically-safe threshold rather +// than redeclaring its own literal — a second, independently-chosen cap is exactly the drift the +// hasUnsafeWildcardCount export below warns about. +export const MAX_GLOB_WILDCARD_GROUPS = 2; /** Count `*` GROUPS in `glob` — a `**` pair is ONE group (it compiles to a single `.*`, see globToRegExp), not * two. Mirrors globToRegExp's own tokenization exactly (including consuming a `**`'s trailing `/`) so the count diff --git a/packages/loopover-engine/test/label-match.test.ts b/packages/loopover-engine/test/label-match.test.ts new file mode 100644 index 0000000000..a7e13c4698 --- /dev/null +++ b/packages/loopover-engine/test/label-match.test.ts @@ -0,0 +1,34 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; + +import { + clearLabelPatternRegExpCacheForTest, + labelMatchesPattern, + labelPatternRegExpCacheKeysForTest, +} from "../dist/scoring/label-match.js"; + +// #9994: the fnmatch compiler emits one `.*` per `*` and has no `**` concept, so `*a**b` compiles to THREE +// `.*` groups. The old guard reused change-guardrail's path-glob counter, which scores a `**` pair as ONE +// group, undercounting `**` and admitting a pattern this compiler builds into a catastrophic-backtracking +// RegExp. Counting is now fnmatch-specific (one per raw `*`), so `**`-containing patterns over the cap are +// rejected — they fail SAFE toward no-multiplier (never match). +test("#9994: a pattern whose COMPILED groups exceed the cap via `**` is rejected (never matches)", () => { + clearLabelPatternRegExpCacheForTest(); + assert.equal(labelMatchesPattern("anything", "*a**b"), false); // 3 stars → 3 groups → rejected + assert.equal(labelMatchesPattern("x/y", "**/**"), false); // 4 stars → 4 groups → rejected +}); + +test("#9994: the preserved 2-group and non-`*` cases still match exactly", () => { + assert.equal(labelMatchesPattern("type:bug-fix", "type:*"), true); + assert.equal(labelMatchesPattern("priority:1", "priority:?"), true); // `?` is not a counted group + assert.equal(labelMatchesPattern("a-b-c", "a*b*c"), true); // 2 stars, at the cap + assert.equal(labelMatchesPattern("kind:bug", "kind:[bc]ug"), true); // classes are not counted groups +}); + +test("#9994: a rejected over-complex pattern is still cached (repeated read served from cache)", () => { + clearLabelPatternRegExpCacheForTest(); + assert.equal(labelMatchesPattern("anything", "*a**b"), false); + assert.ok(labelPatternRegExpCacheKeysForTest().includes("*a**b")); + assert.equal(labelMatchesPattern("something-else", "*a**b"), false); // cache-hit arm, still false + assert.equal(labelPatternRegExpCacheKeysForTest().filter((k) => k === "*a**b").length, 1); +}); diff --git a/test/unit/scoring.test.ts b/test/unit/scoring.test.ts index c13475d69d..a61be9df40 100644 --- a/test/unit/scoring.test.ts +++ b/test/unit/scoring.test.ts @@ -960,9 +960,12 @@ NOVELTY_BONUS_SCALAR = 3 // Regex metacharacters in a literal key stay literal: `.` matches only a dot, not any char. expect(labelMultiplierFor({ "v1.0": 1.1 }, ["v1.0"])).toBe(1.1); expect(labelMultiplierFor({ "v1.0": 1.1 }, ["v1x0"])).toBe(1); - // `**/` counts as one wildcard group and matches across path-like label segments. + // #9994: this fnmatch compiler counts one wildcard group per raw `*` (it has no `**`-is-one-group rule — + // that is the PATH compiler's semantics), so `**/bug` and `**bug` are 2 groups (at the cap → still + // compile and match), but `public/**/*.json` is THREE (`**` + `*`) and is now rejected as over-complex, + // failing SAFE toward no multiplier — where the old path-glob count wrongly scored it as 2 and matched it. expect(labelMultiplierFor({ "**/bug": 1.45 }, ["feature/bug"])).toBe(1.45); - expect(labelMultiplierFor({ "public/**/*.json": 1.2 }, ["public/release/config.json"])).toBe(1.2); + expect(labelMultiplierFor({ "public/**/*.json": 1.2 }, ["public/release/config.json"])).toBe(1); expect(labelMultiplierFor({ "**bug": 1.35 }, ["feature-bug"])).toBe(1.35); // When several patterns match, the highest multiplier wins (mirrors upstream `max(...)`). expect(labelMultiplierFor({ "kind/*": 1.1, "*/bug": 1.6 }, ["kind/bug"])).toBe(1.6); @@ -2150,4 +2153,30 @@ describe("label pattern matcher memoization (#2106)", () => { expect(labelMatchesPattern("type-bug-fix", "type-*-*")).toBe(true); expect(labelMatchesPattern("type-bug", "type-*-*")).toBe(false); }); + + it("#9994: a `**`-containing pattern is counted by its COMPILED groups (one per raw *), not the path-glob `**`-is-one rule, so it is rejected", () => { + // The fnmatch compiler emits one `.*` per `*` and has no `**` concept, so `*a**b` compiles to THREE `.*` + // groups. change-guardrail's path counter scored it as 2 (a `**` pair = one group) and wrongly ACCEPTED it, + // admitting a pattern this compiler builds into a catastrophic-backtracking RegExp. All three fail SAFE + // toward no-multiplier (never matches). + clearLabelPatternRegExpCacheForTest(); + expect(labelMatchesPattern("anything", "*a**b")).toBe(false); // 3 stars → 3 compiled groups → rejected + expect(labelMatchesPattern("x/y", "**/**")).toBe(false); // 4 stars → 4 compiled groups → rejected + expect(labelMatchesPattern("abc", "a**b**c")).toBe(false); // 4 stars → rejected + + // The preserved 2-group cases and non-`*` metacharacters still compile and match exactly as before. + expect(labelMatchesPattern("type:bug-fix", "type:*")).toBe(true); + expect(labelMatchesPattern("priority:1", "priority:?")).toBe(true); // `?` is not a counted group + expect(labelMatchesPattern("a-b-c", "a*b*c")).toBe(true); + expect(labelMatchesPattern("kind:bug", "kind:[bc]ug")).toBe(true); // classes are not counted groups + }); + + it("#9994: a rejected over-complex pattern is still cached, so a repeated read is served from the cache", () => { + clearLabelPatternRegExpCacheForTest(); + expect(labelMatchesPattern("anything", "*a**b")).toBe(false); + expect(labelPatternRegExpCacheKeysForTest()).toContain("*a**b"); + // Second read of the same over-complex pattern is served from the cache (cache-hit arm), still false. + expect(labelMatchesPattern("something-else", "*a**b")).toBe(false); + expect(labelPatternRegExpCacheKeysForTest().filter((k) => k === "*a**b")).toHaveLength(1); + }); });