fix(i18n): detect duplicate allowlist entries, correct the collapse advice (#2221)

Co-authored-by: Kandev Agent <agent@kandev.dev>

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

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 <ref-or-sha>]
  */
 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)` +

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))];
+}

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}");
+  });
+});

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