fix(i18n): reject guard allowlist entries that match no file (#2275)
Co-authored-by: Kandev Agent <agent@kandev.dev>
This commit is contained in:
João Salavisa
@@ -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 <ref-or-sha>]
|
||||
*/
|
||||
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)` +
|
||||
|
||||
@@ -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}`));
|
||||
}
|
||||
|
||||
@@ -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([]);
|
||||
});
|
||||
});
|
||||
|
||||
+16
-5
@@ -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
|
||||
|
||||
|
||||
Reference in New Issue
Block a user