From 7b31238989233ffc38b30e75a9fcf2cd3ea2e6f8 Mon Sep 17 00:00:00 2001 From: bitfathers94 <237535319+bitfathers94@users.noreply.github.com> Date: Fri, 31 Jul 2026 07:57:55 +0000 Subject: [PATCH] fix(scripts): make BRANDING_DRIFT_PATHSPECS scan files at depth 0 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. --- .github/workflows/ci.yml | 4 +- scripts/branding-drift-baseline.json | 3 + scripts/check-branding-drift.ts | 20 +-- test/unit/check-branding-drift-script.test.ts | 119 ++++++++++++++++-- 4 files changed, 127 insertions(+), 19 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 52c2b79ba1..ba346d75a6 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -441,8 +441,8 @@ jobs: # rebrand -- see the script's own header comment (#6786 is the concrete incident this was written # for). Scoped broadly (src/** plus every workspace package's bin/lib/src/scripts dirs), so it's # gated broadly to match; same local-only-until-now gap as the drift checks above. Also gated on - # `discoveryIndex`: BRANDING_DRIFT_PATHSPECS includes "packages/*/src/**/*.ts", which matches - # packages/discovery-index/src/**. + # `discoveryIndex`: BRANDING_DRIFT_PATHSPECS includes "packages/*/src/*.ts", which matches + # packages/discovery-index/src/*.ts (git pathspec `*` matches `/`, so this covers every depth). - name: Branding drift check 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' }} run: npm run branding-drift:check diff --git a/scripts/branding-drift-baseline.json b/scripts/branding-drift-baseline.json index cdc06f68d7..40d6158d24 100644 --- a/scripts/branding-drift-baseline.json +++ b/scripts/branding-drift-baseline.json @@ -4,10 +4,12 @@ "apps/loopover-ui/src/routes/app.index.tsx": 1, "apps/loopover-ui/src/routes/app.runs.tsx": 1, "apps/loopover-ui/src/routes/app.workbench.tsx": 1, + "packages/loopover-contract/src/cli-config.ts": 2, "packages/loopover-engine/src/signals/engine.ts": 2, "packages/loopover-mcp/bin/loopover-mcp.ts": 2, "src/api/routes.ts": 2, "src/db/repositories.ts": 1, + "src/env.d.ts": 3, "src/github/app.ts": 11, "src/github/backfill.ts": 7, "src/github/commands.ts": 2, @@ -30,6 +32,7 @@ "src/selfhost/health.ts": 3, "src/selfhost/monitored-work.ts": 1, "src/selfhost/orb-collector.ts": 1, + "src/server.ts": 3, "src/services/ai-review.ts": 4, "src/services/ai-slop.ts": 1, "src/services/ai-summaries.ts": 1, diff --git a/scripts/check-branding-drift.ts b/scripts/check-branding-drift.ts index 4e49e1015c..7083eb983f 100644 --- a/scripts/check-branding-drift.ts +++ b/scripts/check-branding-drift.ts @@ -28,17 +28,17 @@ export const BASELINE_RELATIVE_PATH = "scripts/branding-drift-baseline.json"; // top-level src/** scope; docs/README/CHANGELOG/schema/terraform/css and every test dir are deliberately // excluded (see header comment). export const BRANDING_DRIFT_PATHSPECS = [ - "src/**/*.ts", - "src/**/*.tsx", + "src/*.ts", + "src/*.tsx", "packages/*/bin/**", - "packages/*/lib/**/*.js", - "packages/*/lib/**/*.ts", - "packages/*/src/**/*.ts", - "packages/*/src/**/*.tsx", - "packages/*/scripts/**/*.mjs", - "apps/*/src/**/*.ts", - "apps/*/src/**/*.tsx", - "apps/*/scripts/**/*.mjs", + "packages/*/lib/*.js", + "packages/*/lib/*.ts", + "packages/*/src/*.ts", + "packages/*/src/*.tsx", + "packages/*/scripts/*.mjs", + "apps/*/src/*.ts", + "apps/*/src/*.tsx", + "apps/*/scripts/*.mjs", ":(exclude)**/*.test.ts", ":(exclude)**/*.test.tsx", ":(exclude)packages/*/test/**", diff --git a/test/unit/check-branding-drift-script.test.ts b/test/unit/check-branding-drift-script.test.ts index 72ba7c39f6..970ec4046e 100644 --- a/test/unit/check-branding-drift-script.test.ts +++ b/test/unit/check-branding-drift-script.test.ts @@ -1,6 +1,23 @@ import { execFileSync } from "node:child_process"; import { describe, expect, it } from "vitest"; -import { diffBrandingBaseline, scanBrandingHits } from "../../scripts/check-branding-drift"; +import { BRANDING_DRIFT_PATHSPECS, diffBrandingBaseline, scanBrandingHits } from "../../scripts/check-branding-drift"; + +// Mirrors git's own pathspec matching for patterns with no `:(glob)` magic: fnmatch(3) with FNM_PATHNAME +// OFF, so `*` (and therefore `**`, which collapses to the same thing under plain fnmatch) matches `/` too. +// This is deliberately NOT a shortcut like `path.startsWith(root)` -- it has to reproduce the exact +// depth-blind-or-not behavior `git grep` applies, so this test fails against the pre-fix `**/`-segmented list. +function globToRegExp(pattern: string): RegExp { + const body = pattern.replace(/[.+^${}()|[\]\\]/g, "\\$&").replace(/\*+/g, ".*").replace(/\?/g, "."); + return new RegExp(`^${body}$`); +} + +function matchesPathspecs(pathspecs: readonly string[], file: string): boolean { + const includes = pathspecs.filter((spec) => !spec.startsWith(":(exclude)")); + const excludes = pathspecs.filter((spec) => spec.startsWith(":(exclude)")).map((spec) => spec.slice(":(exclude)".length)); + const included = includes.some((spec) => globToRegExp(spec).test(file)); + const excluded = excludes.some((spec) => globToRegExp(spec).test(file)); + return included && !excluded; +} describe("scanBrandingHits", () => { it("parses git grep -c output into a { file: count } map", () => { @@ -33,7 +50,7 @@ describe("scanBrandingHits", () => { scanBrandingHits({ root: "/fake", exec }); expect(capturedArgs[0]).toBe("grep"); - expect(capturedArgs).toContain("src/**/*.ts"); + expect(capturedArgs).toContain("src/*.ts"); expect(capturedArgs).toContain(":(exclude)**/*.test.ts"); }); @@ -45,12 +62,12 @@ describe("scanBrandingHits", () => { }; scanBrandingHits({ root: "/fake", exec }); - expect(capturedArgs).toContain("apps/*/src/**/*.ts"); - expect(capturedArgs).toContain("apps/*/src/**/*.tsx"); - expect(capturedArgs).toContain("apps/*/scripts/**/*.mjs"); + expect(capturedArgs).toContain("apps/*/src/*.ts"); + expect(capturedArgs).toContain("apps/*/src/*.tsx"); + expect(capturedArgs).toContain("apps/*/scripts/*.mjs"); }); - it("scans packages/*/src/**/*.tsx, so ui-kit design-system components are covered like apps/* .tsx are", () => { + it("scans packages/*/src/*.tsx, so ui-kit design-system components are covered like apps/* .tsx are", () => { let capturedArgs: string[] = []; const exec = (_root: string, args: string[]) => { capturedArgs = args; @@ -58,7 +75,7 @@ describe("scanBrandingHits", () => { }; scanBrandingHits({ root: "/fake", exec }); - expect(capturedArgs).toContain("packages/*/src/**/*.tsx"); + expect(capturedArgs).toContain("packages/*/src/*.tsx"); }); it("includes a packages/*/src/*.tsx hit in the scanned set (a ui-kit component now in scope)", () => { @@ -135,6 +152,94 @@ describe("diffBrandingBaseline", () => { }); }); +describe("BRANDING_DRIFT_PATHSPECS depth coverage (regression for #10045)", () => { + // Each row is a root that used to be written `/**/*.`. A depth-0 file (directly inside the + // root) and a nested file must both match; a file outside the root must not. Against the pre-fix `**/` + // form, `depthZero` fails to match (the exact bug this issue is about) while `nested` and `outside` still + // pass -- so this table only turns fully green once every affected pathspec drops its `**/` segment. + const CASES = [ + { root: "src/*.ts", depthZero: "src/index.ts", nested: "src/api/routes.ts", outside: "docs/index.ts" }, + { root: "src/*.tsx", depthZero: "src/App.tsx", nested: "src/components/App.tsx", outside: "docs/App.tsx" }, + { + root: "packages/*/src/*.ts", + depthZero: "packages/discovery-index/src/app.ts", + nested: "packages/discovery-index/src/ingest/app.ts", + outside: "packages/discovery-index/test/app.ts", + }, + { + root: "packages/*/src/*.tsx", + depthZero: "packages/loopover-ui-kit/src/card.tsx", + nested: "packages/loopover-ui-kit/src/components/card.tsx", + outside: "packages/loopover-ui-kit/test/card.tsx", + }, + { + // outside deliberately avoids bin/ -- packages/*/bin/** matches every file under bin/ regardless + // of extension, so a bin/ path would pass for the wrong reason. + root: "packages/*/lib/*.js", + depthZero: "packages/loopover-mcp/lib/tools.js", + nested: "packages/loopover-mcp/lib/resources/tools.js", + outside: "packages/loopover-mcp/test/tools.js", + }, + { + root: "packages/*/lib/*.ts", + depthZero: "packages/loopover-mcp/lib/tools.ts", + nested: "packages/loopover-mcp/lib/resources/tools.ts", + outside: "packages/loopover-mcp/test/tools.ts", + }, + { + root: "packages/*/scripts/*.mjs", + depthZero: "packages/loopover-engine/scripts/build.mjs", + nested: "packages/loopover-engine/scripts/codegen/build.mjs", + outside: "packages/loopover-engine/test/build.mjs", + }, + { + root: "apps/*/src/*.ts", + depthZero: "apps/loopover-ui/src/main.ts", + nested: "apps/loopover-ui/src/lib/main.ts", + outside: "apps/loopover-ui/scripts/main.ts", + }, + { + root: "apps/*/src/*.tsx", + depthZero: "apps/loopover-ui/src/main.tsx", + nested: "apps/loopover-ui/src/routes/main.tsx", + outside: "apps/loopover-ui/scripts/main.tsx", + }, + { + root: "apps/*/scripts/*.mjs", + depthZero: "apps/loopover-ui/scripts/build.mjs", + nested: "apps/loopover-ui/scripts/codegen/build.mjs", + outside: "apps/loopover-ui/src/build.mjs", + }, + ] as const; + + it.each(CASES)("$root matches a depth-0 file, still matches a nested file, and rejects an outside file", ({ root, depthZero, nested, outside }) => { + expect(BRANDING_DRIFT_PATHSPECS).toContain(root); + expect(matchesPathspecs(BRANDING_DRIFT_PATHSPECS, depthZero)).toBe(true); + expect(matchesPathspecs(BRANDING_DRIFT_PATHSPECS, nested)).toBe(true); + expect(matchesPathspecs(BRANDING_DRIFT_PATHSPECS, outside)).toBe(false); + }); + + it("packages/*/bin/** is untouched -- a bare ** tail already matches every depth, including depth 0", () => { + expect(BRANDING_DRIFT_PATHSPECS).toContain("packages/*/bin/**"); + expect(matchesPathspecs(BRANDING_DRIFT_PATHSPECS, "packages/loopover-mcp/bin/cli.js")).toBe(true); + expect(matchesPathspecs(BRANDING_DRIFT_PATHSPECS, "packages/loopover-mcp/bin/nested/cli.js")).toBe(true); + }); + + it("still excludes test files at every depth, including depth 0", () => { + expect(matchesPathspecs(BRANDING_DRIFT_PATHSPECS, "src/index.test.ts")).toBe(false); + expect(matchesPathspecs(BRANDING_DRIFT_PATHSPECS, "packages/loopover-mcp/test/tools.ts")).toBe(false); + }); + + it("contains no **/ path segment except packages/*/bin/** and the three untouched :(exclude) entries", () => { + const allowedDoubleStarEntries = ["packages/*/bin/**", ":(exclude)**/*.test.ts", ":(exclude)**/*.test.tsx", ":(exclude)packages/*/test/**"]; + for (const spec of BRANDING_DRIFT_PATHSPECS) { + if (spec.includes("**")) { + expect(allowedDoubleStarEntries).toContain(spec); + } + } + }); +}); + describe("check-branding-drift script (real repo state)", () => { // Most important test in this file: proves the checked-in baseline actually matches the real repo right // now. If this fails, real drift landed (or a cleanup did) without regenerating the baseline -- either way,