From ea07ac7863d47db5a04a5518cab617d20f45d26e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Salavisa?= Date: Mon, 3 Aug 2026 21:53:00 +0200 Subject: [PATCH] fix(i18n): detect duplicate allowlist entries, correct the collapse advice (#2221) Co-authored-by: Kandev Agent --- apps/web/eslint.i18n.options.mjs | 17 +++- apps/web/scripts/check-guard-allowlist.mjs | 26 +++++ apps/web/scripts/lib/guard-allowlist.mjs | 31 ++++++ apps/web/scripts/lib/guard-allowlist.test.ts | 99 ++++++++++++++++++++ docs/i18n.md | 24 ++++- 5 files changed, 191 insertions(+), 6 deletions(-) create mode 100644 apps/web/scripts/lib/guard-allowlist.mjs create mode 100644 apps/web/scripts/lib/guard-allowlist.test.ts diff --git a/apps/web/eslint.i18n.options.mjs b/apps/web/eslint.i18n.options.mjs index 1ff2f75a7c..00f3318b6e 100644 --- a/apps/web/eslint.i18n.options.mjs +++ b/apps/web/eslint.i18n.options.mjs @@ -161,8 +161,21 @@ export const noLiteralStringOptions = { * and there is no treadmill. * * Entries may be single files or directory globs — use a file glob while a - * directory is partially migrated, and collapse to `dir/**` once it is done. - * The list only shrinks by mistake; adding to it is the whole point. + * directory is partially migrated. Once it is done, LEAVE those entries alone: + * **adding a path is the only safe edit to this list.** + * + * Do not collapse several file globs into one `dir/**`. That fails + * `check-guard-allowlist.mjs`, which flags every entry that disappears while its + * path still exists — globs included, resolved with `fs.globSync`. The check + * cannot tell a tidy-up from quietly dropping protection, and refusing to guess + * is the whole point of the ratchet: the two edits produce an identical diff to + * the array, and only one of them is harmless. A broader glob added *alongside* + * the existing entries passes; swapping them for it never does. + * + * Nor may a path be listed twice. An exact duplicate protects nothing new and + * presents an earlier migration's work as yours; the same check rejects it. + * (Entries a broader glob already covers are a different thing and are kept on + * purpose — see the storage globs below.) * * A clean lint is NOT proof a path is fully migrated — the rule only sees plain * literals in JSX. Template literals, `confirm()`/`alert()` arguments, and copy diff --git a/apps/web/scripts/check-guard-allowlist.mjs b/apps/web/scripts/check-guard-allowlist.mjs index 947139f2d6..97fb445f8c 100644 --- a/apps/web/scripts/check-guard-allowlist.mjs +++ b/apps/web/scripts/check-guard-allowlist.mjs @@ -11,6 +11,11 @@ * Removing an entry is legitimate exactly once: when a file is deleted or renamed. * Both are detectable, so the check only complains when the path still exists. * + * The list also has to stay honest about what it claims, so a second check + * rejects a path listed twice. A duplicate costs nothing at runtime, so before + * this check existed it slipped through every gate the repo had while quietly + * presenting an earlier migration's work as the current PR's. + * * Usage: node scripts/check-guard-allowlist.mjs [--base ] */ import fs from "node:fs"; @@ -18,6 +23,7 @@ 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"; const OPTIONS_PATH = "apps/web/eslint.i18n.options.mjs"; @@ -92,6 +98,26 @@ if (removed.length > 0) { process.exit(1); } +/** + * Nothing above catches a path listed twice: a removal check only ever looks at + * what left. A duplicate changes no behaviour either, so `i18next/no-literal-string`, + * `i18n:check` and the new-code half of the ratchet are all indifferent to one — + * which is exactly why it needs its own check here. See `duplicateEntries` for + * why this is deliberately exact-match and not a subsumption check. + */ +const duplicates = duplicateEntries(after); + +if (duplicates.length > 0) { + console.error( + `\n✖ ${duplicates.length} duplicate entr(ies) in i18nGuardFiles:\n\n` + + duplicates.map((entry) => ` ${entry}`).join("\n") + + `\n\n Each of those is already listed elsewhere in the array, so the second` + + `\n copy protects nothing new — it only reads as coverage this change added.` + + `\n Delete the copy you just wrote; the existing entry already covers it.\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 new file mode 100644 index 0000000000..646f0d05b8 --- /dev/null +++ b/apps/web/scripts/lib/guard-allowlist.mjs @@ -0,0 +1,31 @@ +/** + * Shape checks on the i18n guard allowlist itself. + * + * Split out of `check-guard-allowlist.mjs` so they can be tested directly: that + * script resolves this repository's own git history and options module from + * fixed paths, so it cannot be pointed at a fixture. + */ + +/** + * Entries listed more than once, each named once however often it repeats. + * + * Nothing else looks for one, because a duplicate changes no behaviour: ESLint is + * indifferent to a repeated pattern, and the removal check in + * `check-guard-allowlist.mjs` only ever inspects entries that LEFT the array. So + * when #2214 added two, lint, `i18n:check`, the ratchet and the unit suite all + * passed and it was caught by eye. What a duplicate damages is the record: a + * second copy of a path an earlier migration already listed reads as the new + * PR's own coverage, which is the misinformation the comments around the list + * exist to prevent. + * + * EXACT duplicates only, deliberately. The list also carries entries a broader + * glob already covers — `app/settings/system/storage/**` sits inside + * `app/settings/system/**`, `system-page-shell.tsx` inside + * `components/settings/system/*` — and #2202 kept those on purpose, as the + * record of which PR migrated which half of a group. That redundancy is + * intentional and must keep passing. Do not turn this into a subsumption check; + * it would fight a decision the list makes knowingly. + */ +export function duplicateEntries(entries) { + return [...new Set(entries.filter((entry, index) => entries.indexOf(entry) !== index))]; +} diff --git a/apps/web/scripts/lib/guard-allowlist.test.ts b/apps/web/scripts/lib/guard-allowlist.test.ts new file mode 100644 index 0000000000..b580b9ac3f --- /dev/null +++ b/apps/web/scripts/lib/guard-allowlist.test.ts @@ -0,0 +1,99 @@ +/** + * Duplicate detection for `i18nGuardFiles`, and the invariant it protects. + * + * The bug this pins down (#2214): the sidebar migration appended two paths that + * earlier migrations had already listed. A duplicate changes no behaviour — the + * ESLint config just passes the same pattern twice — so it cleared `pnpm lint`, + * `i18n:check`, the ratchet, `check-guard-allowlist.mjs` itself and 8,218 unit + * tests. It was caught by eye. What it costs is the record: the array is read as + * "which PR migrated what", and a second copy silently books someone else's work + * as the new PR's coverage. + * + * Following the pattern in `git-base.test.ts`, the "fixed" assertions are paired + * with one showing the pre-existing removal check still cannot see a duplicate — + * 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 { i18nGuardFiles } from "../../eslint.i18n.options.mjs"; +import { duplicateEntries } from "./guard-allowlist.mjs"; + +/** What `check-guard-allowlist.mjs` did before this helper existed. */ +function removedEntries(before: string[], after: string[]) { + const afterSet = new Set(after); + return before.filter((entry) => !afterSet.has(entry)); +} + +describe("duplicateEntries", () => { + it("reports nothing for a list with no repeats", () => { + expect(duplicateEntries(["a.tsx", "b.tsx", "components/c/**"])).toEqual([]); + }); + + it("names an entry listed twice", () => { + const entries = ["a.tsx", "b.tsx", "a.tsx"]; + + expect(duplicateEntries(entries)).toEqual(["a.tsx"]); + }); + + it("names an entry once however often it repeats", () => { + expect(duplicateEntries(["a.tsx", "a.tsx", "a.tsx", "a.tsx"])).toEqual(["a.tsx"]); + }); + + it("names every distinct repeat — #2214 added two, not one", () => { + const entries = [ + "components/app-sidebar/sections/settings/general-group.tsx", + "components/app-sidebar/sections/settings/agents-group.tsx", + "components/app-status-bar/**/*.{ts,tsx}", + "components/app-sidebar/sections/settings/general-group.tsx", + "components/app-sidebar/sections/settings/agents-group.tsx", + ]; + + expect(duplicateEntries(entries)).toEqual([ + "components/app-sidebar/sections/settings/general-group.tsx", + "components/app-sidebar/sections/settings/agents-group.tsx", + ]); + }); + + /** + * The check is exact-match ON PURPOSE. `main` carries entries that a broader + * glob already covers, and #2202's comment keeps them deliberately: they are + * the historical record of which PR migrated which half of the System group. + * Widening this into a subsumption check would fail the list as it stands. + */ + it("does not flag an entry a broader glob already covers", () => { + const entries = [ + "app/settings/system/**/*.{ts,tsx}", + "app/settings/system/storage/**/*.{ts,tsx}", + "components/settings/system/*.{ts,tsx}", + "components/settings/system/system-page-shell.tsx", + ]; + + expect(duplicateEntries(entries)).toEqual([]); + }); +}); + +describe("the removal check cannot stand in for it", () => { + it("reproduces the gap: adding a duplicate removes nothing", () => { + const before = ["a.tsx", "b.tsx"]; + const after = ["a.tsx", "b.tsx", "a.tsx"]; + + expect(removedEntries(before, after)).toEqual([]); + expect(duplicateEntries(after)).toEqual(["a.tsx"]); + }); +}); + +describe("the live allowlist", () => { + it("lists no path twice", () => { + expect(duplicateEntries(i18nGuardFiles)).toEqual([]); + }); + + /** + * Guards the scenario above against drift: if these two ever stop coexisting, + * the "exact duplicates only" test is no longer pinning a real decision. + */ + it("still carries the deliberate storage redundancy the check must tolerate", () => { + expect(i18nGuardFiles).toContain("app/settings/system/**/*.{ts,tsx}"); + expect(i18nGuardFiles).toContain("app/settings/system/storage/**/*.{ts,tsx}"); + }); +}); diff --git a/docs/i18n.md b/docs/i18n.md index 02ee2aedc8..6fbe643a8c 100644 --- a/docs/i18n.md +++ b/docs/i18n.md @@ -214,10 +214,26 @@ Scoping inverts it. The guard can only ever tighten: is no treadmill. - Every migration PR moves one path from the second group to the first. -Entries are globs, so use a file glob while a directory is partly migrated and -collapse to `dir/**` once it is done. **Never remove an entry** to make a build -pass — that silently un-protects migrated copy. `check-guard-allowlist.mjs` -enforces this: it fails if an entry disappears while its path still exists. +Entries are globs, so use a file glob while a directory is partly migrated. When +it is done, leave those entries where they are: **adding a path is the only safe +edit to this list.** + +`check-guard-allowlist.mjs` fails if an entry disappears while its path still +exists — globs included, resolved with `fs.globSync`. So **never remove an entry** +to make a build pass, and equally, never tidy several file globs into one +`dir/**`. Both produce the same diff to the array, the check cannot tell them +apart, and refusing to guess is the point: one of them silently un-protects +migrated copy. A broader glob added *alongside* the existing entries passes; +swapping them for it does not. + +The same check rejects a path listed **twice**. An exact duplicate changes no +behaviour, so nothing else looks for one — the eslint rule is indifferent to a +repeated pattern, and the removal check above only inspects entries that *left* +the array. That is how #2214 landed two past every gate the repo then had. What a +duplicate costs is the record: it claims an earlier migration's coverage as the +new PR's. Entries that a broader glob already covers are deliberate and stay — +they record which PR migrated which half of a group — so this is an exact-match +check, not a redundancy check. ### …but new code is guarded everywhere