Skip to content

Commit d7f1173

Browse files
authored
fix(scripts): make BRANDING_DRIFT_PATHSPECS scan files at depth 0 (#10126)
git pathspecs without :(glob) magic use fnmatch without FNM_PATHNAME, so a single * already matches /. The **/ segments in BRANDING_DRIFT_PATHSPECS were redundant and silently required at least one extra path separator, so files sitting directly inside a scanned root (src/index.ts, every file under packages/discovery-index/src/, packages/loopover-mcp/lib/) were never grepped. Co-authored-by: bitfathers94 <237535319+bitfathers94@users.noreply.github.com>
1 parent f8fe08d commit d7f1173

4 files changed

Lines changed: 127 additions & 19 deletions

File tree

.github/workflows/ci.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -441,8 +441,8 @@ jobs:
441441
# rebrand -- see the script's own header comment (#6786 is the concrete incident this was written
442442
# for). Scoped broadly (src/** plus every workspace package's bin/lib/src/scripts dirs), so it's
443443
# gated broadly to match; same local-only-until-now gap as the drift checks above. Also gated on
444-
# `discoveryIndex`: BRANDING_DRIFT_PATHSPECS includes "packages/*/src/**/*.ts", which matches
445-
# packages/discovery-index/src/**.
444+
# `discoveryIndex`: BRANDING_DRIFT_PATHSPECS includes "packages/*/src/*.ts", which matches
445+
# packages/discovery-index/src/*.ts (git pathspec `*` matches `/`, so this covers every depth).
446446
- name: Branding drift check
447447
if: ${{ github.event_name == 'push' || needs.changes.outputs.backend == 'true' || needs.changes.outputs.mcp == 'true' || needs.changes.outputs.engine == 'true' || needs.changes.outputs.miner == 'true' || needs.changes.outputs.ui == 'true' || needs.changes.outputs.discoveryIndex == 'true' }}
448448
run: npm run branding-drift:check

scripts/branding-drift-baseline.json

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,12 @@
44
"apps/loopover-ui/src/routes/app.index.tsx": 1,
55
"apps/loopover-ui/src/routes/app.runs.tsx": 1,
66
"apps/loopover-ui/src/routes/app.workbench.tsx": 1,
7+
"packages/loopover-contract/src/cli-config.ts": 2,
78
"packages/loopover-engine/src/signals/engine.ts": 2,
89
"packages/loopover-mcp/bin/loopover-mcp.ts": 2,
910
"src/api/routes.ts": 2,
1011
"src/db/repositories.ts": 1,
12+
"src/env.d.ts": 3,
1113
"src/github/app.ts": 11,
1214
"src/github/backfill.ts": 7,
1315
"src/github/commands.ts": 2,
@@ -30,6 +32,7 @@
3032
"src/selfhost/health.ts": 3,
3133
"src/selfhost/monitored-work.ts": 1,
3234
"src/selfhost/orb-collector.ts": 1,
35+
"src/server.ts": 3,
3336
"src/services/ai-review.ts": 4,
3437
"src/services/ai-slop.ts": 1,
3538
"src/services/ai-summaries.ts": 1,

scripts/check-branding-drift.ts

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -28,17 +28,17 @@ export const BASELINE_RELATIVE_PATH = "scripts/branding-drift-baseline.json";
2828
// top-level src/** scope; docs/README/CHANGELOG/schema/terraform/css and every test dir are deliberately
2929
// excluded (see header comment).
3030
export const BRANDING_DRIFT_PATHSPECS = [
31-
"src/**/*.ts",
32-
"src/**/*.tsx",
31+
"src/*.ts",
32+
"src/*.tsx",
3333
"packages/*/bin/**",
34-
"packages/*/lib/**/*.js",
35-
"packages/*/lib/**/*.ts",
36-
"packages/*/src/**/*.ts",
37-
"packages/*/src/**/*.tsx",
38-
"packages/*/scripts/**/*.mjs",
39-
"apps/*/src/**/*.ts",
40-
"apps/*/src/**/*.tsx",
41-
"apps/*/scripts/**/*.mjs",
34+
"packages/*/lib/*.js",
35+
"packages/*/lib/*.ts",
36+
"packages/*/src/*.ts",
37+
"packages/*/src/*.tsx",
38+
"packages/*/scripts/*.mjs",
39+
"apps/*/src/*.ts",
40+
"apps/*/src/*.tsx",
41+
"apps/*/scripts/*.mjs",
4242
":(exclude)**/*.test.ts",
4343
":(exclude)**/*.test.tsx",
4444
":(exclude)packages/*/test/**",

test/unit/check-branding-drift-script.test.ts

Lines changed: 112 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,23 @@
11
import { execFileSync } from "node:child_process";
22
import { describe, expect, it } from "vitest";
3-
import { diffBrandingBaseline, scanBrandingHits } from "../../scripts/check-branding-drift";
3+
import { BRANDING_DRIFT_PATHSPECS, diffBrandingBaseline, scanBrandingHits } from "../../scripts/check-branding-drift";
4+
5+
// Mirrors git's own pathspec matching for patterns with no `:(glob)` magic: fnmatch(3) with FNM_PATHNAME
6+
// OFF, so `*` (and therefore `**`, which collapses to the same thing under plain fnmatch) matches `/` too.
7+
// This is deliberately NOT a shortcut like `path.startsWith(root)` -- it has to reproduce the exact
8+
// depth-blind-or-not behavior `git grep` applies, so this test fails against the pre-fix `**/`-segmented list.
9+
function globToRegExp(pattern: string): RegExp {
10+
const body = pattern.replace(/[.+^${}()|[\]\\]/g, "\\$&").replace(/\*+/g, ".*").replace(/\?/g, ".");
11+
return new RegExp(`^${body}$`);
12+
}
13+
14+
function matchesPathspecs(pathspecs: readonly string[], file: string): boolean {
15+
const includes = pathspecs.filter((spec) => !spec.startsWith(":(exclude)"));
16+
const excludes = pathspecs.filter((spec) => spec.startsWith(":(exclude)")).map((spec) => spec.slice(":(exclude)".length));
17+
const included = includes.some((spec) => globToRegExp(spec).test(file));
18+
const excluded = excludes.some((spec) => globToRegExp(spec).test(file));
19+
return included && !excluded;
20+
}
421

522
describe("scanBrandingHits", () => {
623
it("parses git grep -c output into a { file: count } map", () => {
@@ -33,7 +50,7 @@ describe("scanBrandingHits", () => {
3350
scanBrandingHits({ root: "/fake", exec });
3451

3552
expect(capturedArgs[0]).toBe("grep");
36-
expect(capturedArgs).toContain("src/**/*.ts");
53+
expect(capturedArgs).toContain("src/*.ts");
3754
expect(capturedArgs).toContain(":(exclude)**/*.test.ts");
3855
});
3956

@@ -45,20 +62,20 @@ describe("scanBrandingHits", () => {
4562
};
4663
scanBrandingHits({ root: "/fake", exec });
4764

48-
expect(capturedArgs).toContain("apps/*/src/**/*.ts");
49-
expect(capturedArgs).toContain("apps/*/src/**/*.tsx");
50-
expect(capturedArgs).toContain("apps/*/scripts/**/*.mjs");
65+
expect(capturedArgs).toContain("apps/*/src/*.ts");
66+
expect(capturedArgs).toContain("apps/*/src/*.tsx");
67+
expect(capturedArgs).toContain("apps/*/scripts/*.mjs");
5168
});
5269

53-
it("scans packages/*/src/**/*.tsx, so ui-kit design-system components are covered like apps/* .tsx are", () => {
70+
it("scans packages/*/src/*.tsx, so ui-kit design-system components are covered like apps/* .tsx are", () => {
5471
let capturedArgs: string[] = [];
5572
const exec = (_root: string, args: string[]) => {
5673
capturedArgs = args;
5774
return "";
5875
};
5976
scanBrandingHits({ root: "/fake", exec });
6077

61-
expect(capturedArgs).toContain("packages/*/src/**/*.tsx");
78+
expect(capturedArgs).toContain("packages/*/src/*.tsx");
6279
});
6380

6481
it("includes a packages/*/src/*.tsx hit in the scanned set (a ui-kit component now in scope)", () => {
@@ -135,6 +152,94 @@ describe("diffBrandingBaseline", () => {
135152
});
136153
});
137154

155+
describe("BRANDING_DRIFT_PATHSPECS depth coverage (regression for #10045)", () => {
156+
// Each row is a root that used to be written `<root>/**/*.<ext>`. A depth-0 file (directly inside the
157+
// root) and a nested file must both match; a file outside the root must not. Against the pre-fix `**/`
158+
// form, `depthZero` fails to match (the exact bug this issue is about) while `nested` and `outside` still
159+
// pass -- so this table only turns fully green once every affected pathspec drops its `**/` segment.
160+
const CASES = [
161+
{ root: "src/*.ts", depthZero: "src/index.ts", nested: "src/api/routes.ts", outside: "docs/index.ts" },
162+
{ root: "src/*.tsx", depthZero: "src/App.tsx", nested: "src/components/App.tsx", outside: "docs/App.tsx" },
163+
{
164+
root: "packages/*/src/*.ts",
165+
depthZero: "packages/discovery-index/src/app.ts",
166+
nested: "packages/discovery-index/src/ingest/app.ts",
167+
outside: "packages/discovery-index/test/app.ts",
168+
},
169+
{
170+
root: "packages/*/src/*.tsx",
171+
depthZero: "packages/loopover-ui-kit/src/card.tsx",
172+
nested: "packages/loopover-ui-kit/src/components/card.tsx",
173+
outside: "packages/loopover-ui-kit/test/card.tsx",
174+
},
175+
{
176+
// outside deliberately avoids bin/ -- packages/*/bin/** matches every file under bin/ regardless
177+
// of extension, so a bin/ path would pass for the wrong reason.
178+
root: "packages/*/lib/*.js",
179+
depthZero: "packages/loopover-mcp/lib/tools.js",
180+
nested: "packages/loopover-mcp/lib/resources/tools.js",
181+
outside: "packages/loopover-mcp/test/tools.js",
182+
},
183+
{
184+
root: "packages/*/lib/*.ts",
185+
depthZero: "packages/loopover-mcp/lib/tools.ts",
186+
nested: "packages/loopover-mcp/lib/resources/tools.ts",
187+
outside: "packages/loopover-mcp/test/tools.ts",
188+
},
189+
{
190+
root: "packages/*/scripts/*.mjs",
191+
depthZero: "packages/loopover-engine/scripts/build.mjs",
192+
nested: "packages/loopover-engine/scripts/codegen/build.mjs",
193+
outside: "packages/loopover-engine/test/build.mjs",
194+
},
195+
{
196+
root: "apps/*/src/*.ts",
197+
depthZero: "apps/loopover-ui/src/main.ts",
198+
nested: "apps/loopover-ui/src/lib/main.ts",
199+
outside: "apps/loopover-ui/scripts/main.ts",
200+
},
201+
{
202+
root: "apps/*/src/*.tsx",
203+
depthZero: "apps/loopover-ui/src/main.tsx",
204+
nested: "apps/loopover-ui/src/routes/main.tsx",
205+
outside: "apps/loopover-ui/scripts/main.tsx",
206+
},
207+
{
208+
root: "apps/*/scripts/*.mjs",
209+
depthZero: "apps/loopover-ui/scripts/build.mjs",
210+
nested: "apps/loopover-ui/scripts/codegen/build.mjs",
211+
outside: "apps/loopover-ui/src/build.mjs",
212+
},
213+
] as const;
214+
215+
it.each(CASES)("$root matches a depth-0 file, still matches a nested file, and rejects an outside file", ({ root, depthZero, nested, outside }) => {
216+
expect(BRANDING_DRIFT_PATHSPECS).toContain(root);
217+
expect(matchesPathspecs(BRANDING_DRIFT_PATHSPECS, depthZero)).toBe(true);
218+
expect(matchesPathspecs(BRANDING_DRIFT_PATHSPECS, nested)).toBe(true);
219+
expect(matchesPathspecs(BRANDING_DRIFT_PATHSPECS, outside)).toBe(false);
220+
});
221+
222+
it("packages/*/bin/** is untouched -- a bare ** tail already matches every depth, including depth 0", () => {
223+
expect(BRANDING_DRIFT_PATHSPECS).toContain("packages/*/bin/**");
224+
expect(matchesPathspecs(BRANDING_DRIFT_PATHSPECS, "packages/loopover-mcp/bin/cli.js")).toBe(true);
225+
expect(matchesPathspecs(BRANDING_DRIFT_PATHSPECS, "packages/loopover-mcp/bin/nested/cli.js")).toBe(true);
226+
});
227+
228+
it("still excludes test files at every depth, including depth 0", () => {
229+
expect(matchesPathspecs(BRANDING_DRIFT_PATHSPECS, "src/index.test.ts")).toBe(false);
230+
expect(matchesPathspecs(BRANDING_DRIFT_PATHSPECS, "packages/loopover-mcp/test/tools.ts")).toBe(false);
231+
});
232+
233+
it("contains no **/ path segment except packages/*/bin/** and the three untouched :(exclude) entries", () => {
234+
const allowedDoubleStarEntries = ["packages/*/bin/**", ":(exclude)**/*.test.ts", ":(exclude)**/*.test.tsx", ":(exclude)packages/*/test/**"];
235+
for (const spec of BRANDING_DRIFT_PATHSPECS) {
236+
if (spec.includes("**")) {
237+
expect(allowedDoubleStarEntries).toContain(spec);
238+
}
239+
}
240+
});
241+
});
242+
138243
describe("check-branding-drift script (real repo state)", () => {
139244
// Most important test in this file: proves the checked-in baseline actually matches the real repo right
140245
// now. If this fails, real drift landed (or a cleanup did) without regenerating the baseline -- either way,

0 commit comments

Comments
 (0)