From 47e1a31c44253cd711640407cc58c12ed29051c3 Mon Sep 17 00:00:00 2001 From: ghost <49853598+JSONbored@users.noreply.github.com> Date: Thu, 25 Jun 2026 23:50:29 -0700 Subject: [PATCH] fix(signals): preserve segment boundaries for globstar paths --- src/signals/focus-manifest.ts | 31 +++++++++++++++++++++++++++---- test/unit/focus-manifest.test.ts | 25 ++++++++++++++++++++++++- 2 files changed, 51 insertions(+), 5 deletions(-) diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 46912a37e9..7046fd9208 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -193,6 +193,7 @@ export type FocusManifestGuidance = { const MAX_LIST_ITEMS = 200; const MAX_ITEM_LENGTH = 300; +const MAX_GLOBSTAR_SLASH_ALTERNATIVES = 128; export const MAX_FOCUS_MANIFEST_BYTES = 64 * 1024; const EMPTY_GATE_CONFIG: FocusManifestGateConfig = { @@ -900,14 +901,36 @@ function linearGlobMatcher(pattern: string): (path: string) => boolean { * Compiling once lets a caller test many paths against one pattern without recompiling per path — see * {@link matchedPatterns}. An empty/blank pattern never matches. */ +function expandGlobstarSlash(pattern: string): string[] { + const alternatives = [""]; + for (let idx = 0; idx < pattern.length; ) { + if (pattern.startsWith("**/", idx)) { + const count = alternatives.length; + const canKeepRootAlternatives = count * 2 <= MAX_GLOBSTAR_SLASH_ALTERNATIVES; + for (let altIdx = count - 1; altIdx >= 0; altIdx -= 1) { + const prefix = alternatives[altIdx]!; + alternatives[altIdx] = `${prefix}*/`; + if (canKeepRootAlternatives) alternatives.push(prefix); + } + idx += 3; + continue; + } + for (let altIdx = 0; altIdx < alternatives.length; altIdx += 1) alternatives[altIdx] += pattern[idx]!; + idx += 1; + } + return alternatives; +} + function compileManifestPathMatcher(pattern: string): (normalizedPath: string) => boolean { const normalizedPattern = normalizePathForMatch(pattern); if (!normalizedPattern) return () => false; if (normalizedPattern.includes("*")) { - // A double-star-then-slash run collapses the mandatory separator into the wildcard so the glob matches - // zero-depth/root too (e.g. a leading double-star glob matches a root-level file). Then run the linear matcher. - const globbed = normalizedPattern.replace(/\*\*\//g, "*"); - return linearGlobMatcher(globbed); + // `**/` means zero or more whole path segments. Keep the slash in the non-root alternative so + // basename globs (e.g. `**/safe.ts`) do not degrade into suffix globs that match `unsafe.ts`. + const matchers = expandGlobstarSlash(normalizedPattern).map((globbed) => + globbed.includes("*") ? linearGlobMatcher(globbed) : (normalizedPath: string) => normalizedPath === globbed, + ); + return (normalizedPath) => matchers.some((matcher) => matcher(normalizedPath)); } const dirPattern = normalizedPattern.endsWith("/") ? normalizedPattern : `${normalizedPattern}/`; return (normalizedPath) => normalizedPath === normalizedPattern || normalizedPath.startsWith(dirPattern); diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 6cd102ab9e..2dd612e02e 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -194,6 +194,15 @@ describe("matchesManifestPath", () => { expect(matchesManifestPath("a/b/c.ts", "**/*.ts")).toBe(true); }); + it("keeps **/ on path-segment boundaries instead of broad suffix matching (#review-audit)", () => { + expect(matchesManifestPath("safe.ts", "**/safe.ts")).toBe(true); + expect(matchesManifestPath("dir/safe.ts", "**/safe.ts")).toBe(true); + expect(matchesManifestPath("unsafe.ts", "**/safe.ts")).toBe(false); + expect(matchesManifestPath("src/safe.ts", "src/**/safe.ts")).toBe(true); + expect(matchesManifestPath("src/dir/safe.ts", "src/**/safe.ts")).toBe(true); + expect(matchesManifestPath("src/unsafe.ts", "src/**/safe.ts")).toBe(false); + }); + it("multi-wildcard matching is correct (ordered substrings, suffix cannot overlap)", () => { expect(matchesManifestPath("xayybzzc", "*a*b*c")).toBe(true); expect(matchesManifestPath("aXbXc", "a*b*c")).toBe(true); @@ -212,6 +221,15 @@ describe("matchesManifestPath", () => { expect(result).toBe(false); expect(elapsed).toBeLessThan(100); // the old per-star regex did not return within 30s on this input }); + + it("bounds repeated **/ expansion while retaining linear matching (#review-audit)", () => { + const globstarRun = "**/".repeat(20) + "safe.ts"; + const start = performance.now(); + const result = matchesManifestPath("a/b/c/safe.ts", globstarRun); + const elapsed = performance.now() - start; + expect(result).toBe(false); + expect(elapsed).toBeLessThan(100); + }); }); // Regression tests for the three compileManifestPathMatcher branches: exact, @@ -1192,11 +1210,16 @@ describe("review.exclude_paths (#review-exclude-paths)", () => { it("excludeReviewPaths filters matching files; empty globs return the same array (byte-identical)", () => { const files = [{ path: "src/a.ts" }, { path: "pnpm-lock.yaml" }, { path: "dist/bundle.js" }]; - // `*` collapses to `.*` (crosses slashes), so `*.yaml` matches a top-level lockfile; `dist/**` matches under dist/. + // `*` crosses slashes, so `*.yaml` matches a top-level lockfile; `dist/**` matches under dist/. expect(excludeReviewPaths(files, ["*.yaml", "dist/**"])).toEqual([{ path: "src/a.ts" }]); expect(excludeReviewPaths(files, ["docs/**"])).toEqual(files); // no match → unchanged expect(excludeReviewPaths(files, [])).toBe(files); // empty → same reference (no-op) }); + + it("does not exclude attacker-named suffix collisions for **/ basename globs (#review-audit)", () => { + const files = [{ path: "unsafe.ts" }, { path: "dir/safe.ts" }, { path: "feature.ts" }]; + expect(excludeReviewPaths(files, ["**/safe.ts"])).toEqual([{ path: "unsafe.ts" }, { path: "feature.ts" }]); + }); }); describe("review.pre_merge_checks (#review-pre-merge-checks)", () => {