diff --git a/apps/web/scripts/check-guard-allowlist.mjs b/apps/web/scripts/check-guard-allowlist.mjs index 97fb445f8c..6a471358a3 100644 --- a/apps/web/scripts/check-guard-allowlist.mjs +++ b/apps/web/scripts/check-guard-allowlist.mjs @@ -16,6 +16,11 @@ * this check existed it slipped through every gate the repo had while quietly * presenting an earlier migration's work as the current PR's. * + * A third check rejects an entry that matches no file. Both of the above compare + * two STATES of the array and so cannot see an entry that was dead the day it + * was written — which is what a dynamic route produces, since `[id]` is a glob + * character class. That entry guards nothing while every check reports green. + * * Usage: node scripts/check-guard-allowlist.mjs [--base ] */ import fs from "node:fs"; @@ -23,10 +28,13 @@ import os from "node:os"; import path from "node:path"; import { git, REPO_ROOT, resolveBase, WEB_DIR } from "./lib/git-base.mjs"; -import { duplicateEntries } from "./lib/guard-allowlist.mjs"; +import { duplicateEntries, filesForEntry, unmatchedEntries } from "./lib/guard-allowlist.mjs"; const OPTIONS_PATH = "apps/web/eslint.i18n.options.mjs"; +/** Shared by the removal and unmatched checks so they cannot disagree. */ +const filesFor = (entry) => filesForEntry(entry, { cwd: WEB_DIR, fsImpl: fs }); + const resolved = resolveBase(); if (resolved.skip) { console.log(`⚠ guard allowlist check skipped — ${resolved.skip}`); @@ -79,13 +87,7 @@ const afterSet = new Set(after); * block the legitimate flow of migrating `components/foo/**`, listing it, then * deleting `foo/` entirely. */ -const removed = before.filter((entry) => { - if (afterSet.has(entry)) return false; - if (/[*?[\]{}]/.test(entry)) { - return fs.globSync(entry, { cwd: WEB_DIR }).length > 0; - } - return fs.existsSync(path.join(WEB_DIR, entry)); -}); +const removed = before.filter((entry) => !afterSet.has(entry) && filesFor(entry).length > 0); if (removed.length > 0) { console.error( @@ -118,6 +120,31 @@ if (duplicates.length > 0) { process.exit(1); } +/** + * And nothing above catches an entry that never matched anything: both checks + * compare two states of the array, so an entry that was born dead looks exactly + * like a healthy one. See `unmatchedEntries` for how a dynamic route produces + * that silently. + * + * Uses the same `filesFor` resolver as the removal check above, so the two + * cannot disagree about what "this entry has files" means. + */ +const unmatched = unmatchedEntries(after, filesFor); + +if (unmatched.length > 0) { + console.error( + `\n✖ ${unmatched.length} entr(ies) in i18nGuardFiles match no file:\n\n` + + unmatched.map((entry) => ` ${entry}`).join("\n") + + `\n\n Each is listed but guards nothing, so migrated copy there can drift` + + `\n back with every check reporting green. A dynamic route is the usual` + + `\n cause: glob brackets are a character class, so \`[id]\` matches the` + + `\n letters i and d, never the directory — write \`[[]id[]]\`. If the path` + + `\n was genuinely deleted, drop the entry instead.` + + `\n See docs/i18n.md ("An entry can be born dead").\n`, + ); + process.exit(1); +} + const added = after.filter((entry) => !new Set(before).has(entry)); console.log( `✓ guard allowlist intact — ${after.length} entr(ies)` + diff --git a/apps/web/scripts/lib/guard-allowlist.mjs b/apps/web/scripts/lib/guard-allowlist.mjs index 646f0d05b8..b1800b9370 100644 --- a/apps/web/scripts/lib/guard-allowlist.mjs +++ b/apps/web/scripts/lib/guard-allowlist.mjs @@ -29,3 +29,68 @@ export function duplicateEntries(entries) { return [...new Set(entries.filter((entry, index) => entries.indexOf(entry) !== index))]; } + +/** + * Entries that match no file at all — listed, but guarding nothing. + * + * Both other checks ask what happened to an entry BETWEEN two states: the + * removal check inspects entries that left the array, and `duplicateEntries` + * compares the array with itself. Neither asks the prior question — does this + * entry match anything? An entry that matched nothing on the day it was added + * never leaves, is not a duplicate, and passes `pnpm lint` trivially because the + * rule simply has no files to apply it to. The list grows, the ratchet reports + * "N added", and the path is unguarded. It is the allowlist's own version of a + * pass it had not earned: green everywhere, protecting nothing. + * + * A dynamic route is how you hit it, because Next-style directories are named in + * glob syntax and `[id]` is a CHARACTER CLASS matching a single `i` or `d`: + * + * app/settings/workspace/[id]/automations/**\/*.tsx -> 0 files + * app/settings/workspace/[[]id[]]/automations/**\/*.tsx -> 4 files + * + * #2247 shipped the unescaped form and review caught it; escaping it turned a + * hardcoded literal in that route from 0 lint errors into 1. Three older entries + * had the same defect. This makes the class impossible rather than the instances + * fixed. + * + * `resolve` is injected so this is testable against a fixture: the calling + * script resolves this repository's own paths from fixed locations and cannot be + * pointed elsewhere. + */ +export function unmatchedEntries(entries, resolve) { + return entries.filter((entry) => resolve(entry).length === 0); +} + +/** An entry is a glob only if it carries glob metacharacters. */ +const GLOB_METACHARACTERS = /[*?[\]{}]/; + +function isFile(fsImpl, fullPath) { + try { + return fsImpl.statSync(fullPath).isFile(); + } catch { + // Missing, or a broken symlink — either way it selects nothing. + return false; + } +} + +/** + * The files an allowlist entry currently matches, rooted at `cwd`. + * + * Exported so the removal check, the unmatched check and the tests share ONE + * definition of "this entry has files" — they used to state it separately, and + * two copies of a rule about glob semantics is how they drift apart. `fsImpl` is + * a seam for tests; it takes `node:fs` in the script. + * + * **Directories do not count, and that is the whole subtlety.** ESLint flat + * config matches `files` patterns against FILE paths, so an entry of + * `components/foo` selects nothing — it does not stand for `components/foo/**`. + * `existsSync` would happily confirm the directory exists and report the entry as + * live, which would let exactly the born-dead entry this module exists to catch + * through under a different spelling. Both branches are therefore filtered by + * `isFile`: `globSync` returns directories too (`components/automations/*` + * matches `.../trigger-configs`), so it is not only the literal case. + */ +export function filesForEntry(entry, { cwd, fsImpl }) { + const candidates = GLOB_METACHARACTERS.test(entry) ? fsImpl.globSync(entry, { cwd }) : [entry]; + return candidates.filter((candidate) => isFile(fsImpl, `${cwd}/${candidate}`)); +} diff --git a/apps/web/scripts/lib/guard-allowlist.test.ts b/apps/web/scripts/lib/guard-allowlist.test.ts index b580b9ac3f..f2c114550d 100644 --- a/apps/web/scripts/lib/guard-allowlist.test.ts +++ b/apps/web/scripts/lib/guard-allowlist.test.ts @@ -14,10 +14,15 @@ * a test that only exercised the new helper would not explain why it has to be a * separate check. */ -import { describe, expect, it } from "vitest"; +import * as nodeFs from "node:fs"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import path from "node:path"; + +import { afterAll, beforeAll, describe, expect, it } from "vitest"; import { i18nGuardFiles } from "../../eslint.i18n.options.mjs"; -import { duplicateEntries } from "./guard-allowlist.mjs"; +import { duplicateEntries, filesForEntry, unmatchedEntries } from "./guard-allowlist.mjs"; /** What `check-guard-allowlist.mjs` did before this helper existed. */ function removedEntries(before: string[], after: string[]) { @@ -97,3 +102,137 @@ describe("the live allowlist", () => { expect(i18nGuardFiles).toContain("app/settings/system/storage/**/*.{ts,tsx}"); }); }); + +/** + * The born-dead entry (#2247). A glob for the `[id]` automations route was + * appended by a migration and matched NOTHING: glob brackets are a CHARACTER CLASS, so `[id]` + * means "one `i` or one `d`" and never the literal directory. The route was + * allowlisted and unguarded at the same time, and every check reported green — + * it never left the array so the removal check ignored it, it was not repeated + * so `duplicateEntries` ignored it, and `pnpm lint` passed because the rule had + * no files to apply. Confirmed then by putting a hardcoded literal in that + * route: 0 lint errors before escaping, 1 after. + * + * These run the REAL `fs.globSync` against a fixture tree rather than a + * re-implementation of glob semantics — the bracket behaviour is the whole point, + * so emulating it would only test the emulation. **Do not simplify this back to a + * hand-rolled matcher.** The first draft did exactly that, and a malformed + * character class in it (`/[*?[]]{}]/` — a lost backslash) made EVERY entry look + * unmatched, so the live-allowlist test below reported a problem that did not + * exist. A false positive that reads as a find is the failure mode this whole + * area keeps producing; a fixture plus the real matcher cannot have it. + * + * As with `duplicateEntries` above, the helper is paired with a test showing both + * existing checks are blind to it, so the reason it must be separate stays on the + * record rather than becoming folklore. + */ +describe("unmatchedEntries", () => { + let root: string; + + /** + * The very resolver `check-guard-allowlist.mjs` uses, pointed at the fixture — + * not a re-implementation. Glob bracket semantics are the thing under test, so + * emulating them would only test the emulation. + */ + const resolve = (entry: string) => filesForEntry(entry, { cwd: root, fsImpl: nodeFs }); + + beforeAll(() => { + root = mkdtempSync(path.join(tmpdir(), "kandev-guard-allowlist-")); + for (const file of [ + "app/settings/workspace/[id]/automations/page.tsx", + "app/settings/workspace/[id]/automations/new/page.tsx", + "components/automations/trigger-card.tsx", + ]) { + const full = path.join(root, file); + mkdirSync(path.dirname(full), { recursive: true }); + writeFileSync(full, ""); + } + }); + + afterAll(() => rmSync(root, { recursive: true, force: true })); + + it("reports nothing when every entry matches", () => { + expect(unmatchedEntries(["components/automations/trigger-card.tsx"], resolve)).toEqual([]); + expect(unmatchedEntries(["components/automations/*.tsx"], resolve)).toEqual([]); + }); + + it("names an unescaped dynamic-route entry, which matches nothing", () => { + const entry = "app/settings/workspace/[id]/automations/**/*.tsx"; + + expect(resolve(entry)).toEqual([]); + expect(unmatchedEntries([entry], resolve)).toEqual([entry]); + }); + + it("accepts the escaped form of the very same route", () => { + const entry = "app/settings/workspace/[[]id[]]/automations/**/*.tsx"; + + expect(resolve(entry)).toHaveLength(2); + expect(unmatchedEntries([entry], resolve)).toEqual([]); + }); + + /** + * A bare directory is the same bug wearing different clothes: `existsSync` + * confirms it, but ESLint flat config matches `files` against FILE paths, so + * `components/automations` selects nothing — it does not stand for + * `components/automations/**`. Caught in review on this very PR. + */ + it("does not count a bare directory as matched", () => { + expect(resolve("components/automations")).toEqual([]); + expect(unmatchedEntries(["components/automations"], resolve)).toEqual([ + "components/automations", + ]); + }); + + it("does not count directories a glob happens to match", () => { + // `*` matches the nested directory as well as the file; only the file counts. + expect(resolve("app/settings/workspace/[[]id[]]/automations/*")).toEqual([ + "app/settings/workspace/[id]/automations/page.tsx", + ]); + }); + + it("names an entry whose path is gone", () => { + expect(unmatchedEntries(["components/gone/page.tsx"], resolve)).toEqual([ + "components/gone/page.tsx", + ]); + }); + + it("names every dead entry, not only the first", () => { + const dead = ["components/gone/a.tsx", "components/gone/b.tsx"]; + const entries = [...dead, "components/automations/trigger-card.tsx"]; + + expect(unmatchedEntries(entries, resolve)).toEqual(dead); + }); +}); + +describe("neither existing check can stand in for it", () => { + it("reproduces the gap: a born-dead entry never leaves the array and is not a duplicate", () => { + const before = ["a.tsx"]; + const after = ["a.tsx", "app/settings/workspace/[id]/automations/**/*.tsx"]; + + // A resolver that is realistic rather than blanket: "a.tsx" exists, the + // unescaped glob matches nothing. A `() => []` stub would report BOTH and + // read as if the check flags live entries too. + const resolve = (entry: string) => (entry.includes("[id]") ? [] : [entry]); + + // Both existing checks are clean: nothing left, nothing repeated. + expect(removedEntries(before, after)).toEqual([]); + expect(duplicateEntries(after)).toEqual([]); + // Only the new check sees it, and it names just the dead entry. + expect(unmatchedEntries(after, resolve)).toEqual([ + "app/settings/workspace/[id]/automations/**/*.tsx", + ]); + }); +}); + +describe("the live allowlist", () => { + /** + * If this fails, some entry is listed but guards nothing. Run + * `node scripts/check-guard-allowlist.mjs` — it names the entry and the fix. + */ + it("has no entry matching zero files", () => { + const webDir = path.resolve(import.meta.dirname, "..", ".."); + const live = (entry: string) => filesForEntry(entry, { cwd: webDir, fsImpl: nodeFs }); + + expect(unmatchedEntries(i18nGuardFiles, live)).toEqual([]); + }); +}); diff --git a/docs/i18n.md b/docs/i18n.md index e8f2f2b13c..14a47b0414 100644 --- a/docs/i18n.md +++ b/docs/i18n.md @@ -261,11 +261,22 @@ node -e 'console.log(require("fs").globSync("app/settings/workspace/[[]id[]]/aut ``` The Automations migration (#2247) shipped the unescaped form, and review caught -it. So when an entry contains `[`, escape it as `[[]…[]]` — and prove the entry -is live rather than trusting the glob result, the same way you would prove any -other check earned its pass: put a hardcoded literal in one of the files the -entry is supposed to cover and confirm `pnpm lint` reports -`i18next/no-literal-string`, then remove it. A silent 0 errors is the tell. +it. So when an entry contains `[`, escape it as `[[]…[]]`. + +`check-guard-allowlist.mjs` now rejects an entry that matches no file, so this +particular class cannot recur — the three checks it runs are deliberately +separate because each is blind to the others' failure: + +| check | sees | +|---|---| +| removal | an entry that **left** the array while its path still exists | +| duplicate | an entry listed **twice** | +| unmatched | an entry that **never matched anything**, including on the day it was added | + +The first two compare two *states* of the array, which is why neither can see a +born-dead entry. If you add a check here, add the test that shows the existing +ones miss what it catches — `scripts/lib/guard-allowlist.test.ts` pins that for +all three. ### …but new code is guarded everywhere