diff --git a/docs/gates.md b/docs/gates.md index ee8443d3..72d8be23 100644 --- a/docs/gates.md +++ b/docs/gates.md @@ -14,7 +14,11 @@ name its invariant. Whatever is left narrowed is listed here, and not an oversight; a narrowing missing from here is a test failure. How the test matches: each narrowing must appear in a table row below as its `code-quoted` id, in a -row whose **Scope** cell names the scope exactly as the test reports it. +row whose **Scope** cell names the scope exactly as the test reports it. For ESLint it reads each +scope's effective config through a sample file AND every config block on its own: a block that +targets files no sample reaches fails as an `unsampled block` (add a sample), a rule a block or src +sets weaker than src counts, so does a `no-restricted-*` list that leaves out an entry src has, and +so does an option that lets something through (`allow…`, `ignore…`, `except…`) (E32 review). ## ESLint (frontend) @@ -34,6 +38,11 @@ them itself (`no-undef`, `no-redeclare`, `no-dupe-keys`, …), nor the formattin | src | `react-hooks/unsupported-syntax` — warn | Preset level; fails under `--max-warnings 0`. | 0 | | src | `react-refresh/only-export-components` — warn | Fails under `--max-warnings 0`; `allowConstantExport` lets a module export constants beside its component. | 0 | | src | `@typescript-eslint/no-unused-expressions` — narrowed | `allowShortCircuit` + `allowTernary`: `cond && f()` / `a ? f() : g()` side-effect statements are an idiom here. | — | +| src | `@typescript-eslint/no-floating-promises` — narrowed | `allowForKnownSafeCalls`: React Query's `invalidateQueries`, `refetchQueries`, `cancelQueries`, `resetQueries`, `prefetchQuery`, `prefetchInfiniteQuery` never reject unless asked to (`throwOnError`), so a bare call is safe by name. | 6 names | +| e2e | `@typescript-eslint/no-floating-promises` — narrowed | The same `allowForKnownSafeCalls` list (the typed rules are one object for both scopes). | 6 names | +| lib/http.ts | `no-restricted-globals` — narrowed | A network owner (`NETWORK_OWNERS` in eslint.config.js): the one module that may call `fetch` (`send`, which `req` and the public pages go through). Every other network global stays restricted there. | 1 global | +| lib/api/core.ts | `no-restricted-globals` — narrowed | A network owner: `uploadReq` is the one `XMLHttpRequest` (upload progress). | 1 global | +| lib/messagesSocket.ts | `no-restricted-globals` — narrowed | A network owner: the chat socket is the one `WebSocket`. | 1 global | | src tests | `no-restricted-globals`, `no-restricted-properties` — off | A test stands a double in for the network (`vi.stubGlobal("fetch", …)`) and may reach it directly. | — | | src tests | `local/literal-a11y-text`, `local/button-primitive`, `local/input-primitive`, `local/surface-role`, `local/glyph-link` — off | A test renders throwaway markup (a raw button as a focus target, an untranslated label) that is not product UI. | — | | FocusSafeButton.tsx | `local/focus-safe-disabled` — off | It IS the focus-safe primitive the rule points everyone else to. | — | @@ -51,10 +60,14 @@ none real in the sample; the curated typed pair `no-floating-promises` + `no-dep | Scope | Narrowed | Why | Measured | | --- | --- | --- | --- | +| config files | `eslint.config.js — not type-checked` | Linted (the `js configs` row) and executed by every lint run, but in no tsc project. Under the node project's `strict` + `checkJs` it reports 43 errors: 42 implicit-`any` parameters in the local rules' visitors (JSX nodes, which ESLint's own `Rule` typings do not model) and one inferred `string` severity. None is a defect; typing them means moving the local rules onto typescript-eslint's `TSESTree` types — a refactor of its own, not done in E32. | 43 (E32 review) | | all three projects | `skipLibCheck` | The `.d.ts` of installed packages are not type-checked (ours are). Standard for an app: a package's own types are its maintainers' gate, and checking them costs minutes and reports errors we cannot fix. | — | Every project has `strict`, `noUncheckedIndexedAccess`, `noUnusedLocals`, `noUnusedParameters` and -`noFallthroughCasesInSwitch` on; the test fails if one goes off. +`noFallthroughCasesInSwitch` on; the test fails if one goes off, if a `strict` sub-flag +(`strictNullChecks`, `noImplicitAny`, …) is set false beside it, if a project gains an `extends`, +`exclude` or `files`, or if its `include` changes — and a config file at the frontend root that no +project checks needs a row. ## Ruff (backend + scripts) diff --git a/frontend/src/lib/gates.contract.test.ts b/frontend/src/lib/gates.contract.test.ts index 32444e82..84ea2a11 100644 --- a/frontend/src/lib/gates.contract.test.ts +++ b/frontend/src/lib/gates.contract.test.ts @@ -1,5 +1,5 @@ -import { readFileSync } from "node:fs"; -import { fileURLToPath } from "node:url"; +import { readdirSync, readFileSync } from "node:fs"; +import { fileURLToPath, pathToFileURL } from "node:url"; import { ESLint, type Linter } from "eslint"; import prettierConfig from "eslint-config-prettier"; import ts from "typescript"; @@ -68,8 +68,16 @@ const TSC_OWNED = new Set( ); const family = (rule: string) => (rule.includes("/") ? `${rule.split("/")[0]}/*` : rule); +// Each scope is checked through a sample file's EFFECTIVE config (what a rule family not applied there +// means), and every config block that targets files is checked on its own (below) — so a block no +// sample reaches fails rather than passing unseen (E32 review: a narrowing scoped to any other file +// changed none of the sampled configs and needed no row). const SCOPES: Record = { src: "src/components/Feed.tsx", + "src/lib": "src/lib/format.ts", + "lib/http.ts": "src/lib/http.ts", + "lib/api/core.ts": "src/lib/api/core.ts", + "lib/messagesSocket.ts": "src/lib/messagesSocket.ts", "src tests": "src/components/Feed.ytsort.dom.test.tsx", "FocusSafeButton.tsx": "src/components/ui/FocusSafeButton.tsx", "AnchoredPopover.tsx": "src/components/ui/AnchoredPopover.tsx", @@ -79,6 +87,55 @@ const SCOPES: Record = { "js configs": "eslint.config.js", }; +/** A flat-config `files` glob as a regex — the shapes this config uses: `**`, `*`, `{a,b}`. */ +function globRegex(glob: string): RegExp { + let out = ""; + for (let i = 0; i < glob.length; i++) { + const c = glob[i]!; + if (glob.startsWith("**/", i)) { + out += "(?:.*/)?"; + i += 2; + } else if (c === "*") out += "[^/]*"; + else if (c === "{") { + const end = glob.indexOf("}", i); + out += `(?:${glob + .slice(i + 1, end) + .split(",") + .join("|")})`; + i = end; + } else out += /[.+?^$()|[\]\\]/.test(c) ? `\\${c}` : c; + } + return new RegExp(`^${out}$`); +} + +type Block = Linter.Config & { files?: (string | string[])[]; ignores?: string[] }; +const blocks = async (): Promise => + ((await import(pathToFileURL(`${FRONTEND}eslint.config.js`).href)) as { default: Block[] }) + .default; + +// A rule option whose key says it lets something through (`allowShortCircuit`, +// `allowForKnownSafeCalls`, `ignoreRestSiblings`, `exceptAfterSingleLine`) narrows the rule, and a +// `no-restricted-*` list re-declared for a scope narrows it by every entry it leaves out. +const RELAXING_KEY = /^(allow|ignore|except)/; +const relaxes = (options: unknown[]): boolean => + options.some( + (o) => + o !== null && + typeof o === "object" && + Object.entries(o).some( + ([k, v]) => RELAXING_KEY.test(k) && v !== false && !(Array.isArray(v) && v.length === 0), + ), + ); +function restrictedEntries(entry: Linter.RuleEntry | undefined): Set { + const opts: unknown[] = Array.isArray(entry) ? entry.slice(1) : []; + const flat = opts.flatMap((o) => { + if (o === null || typeof o !== "object" || Array.isArray(o)) return [o]; + const { paths = [], patterns = [] } = o as { paths?: unknown[]; patterns?: unknown[] }; + return "paths" in o || "patterns" in o ? [...paths, ...patterns] : [o]; + }); + return new Set(flat.map((e) => JSON.stringify(e))); +} + async function eslintNarrowings(): Promise<[string, string][]> { const eslint = new ESLint({ cwd: FRONTEND }); const rules: Record = {}; @@ -91,30 +148,48 @@ async function eslintNarrowings(): Promise<[string, string][]> { // a core rule off because its typescript-eslint twin replaces it is reported as the twin (!rule.includes("/") && `@typescript-eslint/${rule}` in all); const found: [string, string][] = []; - // src: everything at off or warn - for (const [rule, v] of Object.entries(src)) - if (severity(v) < 2 && !ignorable(rule, src)) found.push(["src", rule]); - // every other scope: what is WEAKER there than in src (a rule family not applied at all is one id) + // src: everything at off or warn, and every rule whose options let something through + for (const [rule, v] of Object.entries(src)) { + if (ignorable(rule, src)) continue; + if (severity(v) < 2 || (Array.isArray(v) && relaxes(v.slice(1)))) found.push(["src", rule]); + } + // every other scope: what is WEAKER there than in src (a rule family not applied at all is one + // id). A src rule at warn counts too: switched off for a scope, it is narrowed there. for (const scope of Object.keys(SCOPES).filter((s) => s !== "src")) { const ids = new Set(); for (const [rule, v] of Object.entries(src)) { - if (ignorable(rule, src) || severity(v) < 2) continue; + if (ignorable(rule, src) || severity(v) === 0) continue; const there = severity(rules[scope]![rule]); - if (there < 2 && !(there === 1 && severity(v) === 1)) - ids.add(there < 0 ? family(rule) : rule); + if (there < severity(v)) ids.add(there < 0 ? family(rule) : rule); } for (const id of ids) found.push([scope, id]); } - for (const ignored of eslintIgnores()) found.push(["everywhere", ignored]); + // every config block: a global `ignores` is a narrowing everywhere; one that targets files must + // reach a sample, and what it re-declares is held to src + for (const block of await blocks()) { + if (!block.files) { + if (Object.keys(block).join() === "ignores") + for (const ignored of block.ignores ?? []) found.push(["everywhere", ignored]); + continue; + } + const globs = block.files.flat().map(globRegex); + const scope = Object.keys(SCOPES).find((s) => globs.some((g) => g.test(SCOPES[s]!))); + if (!scope) { + found.push(["unsampled block", block.files.flat().join(", ")]); + continue; + } + if (block.ignores?.length) found.push([scope, `ignores: ${block.ignores.join(", ")}`]); + for (const [rule, v] of Object.entries(block.rules ?? {})) { + if (ignorable(rule, src)) continue; + if (rule.includes("no-restricted-")) { + const own = restrictedEntries(v); + if ([...restrictedEntries(src[rule])].some((e) => !own.has(e))) found.push([scope, rule]); + } else if (Array.isArray(v) && relaxes(v.slice(1))) found.push([scope, rule]); + } + } return found; } -/** The global `ignores` of eslint.config.js (the first, file-less config object). */ -function eslintIgnores(): string[] { - const m = /\{\s*ignores:\s*\[([^\]]*)\]\s*\}/.exec(read("frontend/eslint.config.js")); - return m ? [...m[1]!.matchAll(/"([^"]+)"/g)].map((x) => x[1]!) : []; -} - describe("the gate register (docs/gates.md)", () => { it("lists every ESLint rule narrowed, by scope", async () => { expect(missingFrom(await eslintNarrowings())).toEqual([]); @@ -142,60 +217,146 @@ describe("the gate register (docs/gates.md)", () => { const opts = jsonc(path).compilerOptions as Record; expect(REQUIRED_ON.filter((flag) => opts[flag] !== true)).toEqual([]); }); + // `strict: true` is a bundle: a sub-flag set false beside it, an `extends` this test cannot see + // through, or a narrowed `include` / an `exclude` weakens the project while the five flags above + // still read true (E32 review). + const STRICT_FAMILY = [ + "noImplicitAny", + "strictNullChecks", + "strictFunctionTypes", + "strictBindCallApply", + "strictPropertyInitialization", + "noImplicitThis", + "alwaysStrict", + "useUnknownInCatchVariables", + "strictBuiltinIteratorReturn", + ]; + const INCLUDE: Record = { + "frontend/tsconfig.json": ["src"], + "frontend/tsconfig.node.json": [ + "vite.config.ts", + "vitest.config.ts", + "reactPlugin.ts", + "devOnlyGuard.ts", + "playwright.config.ts", + "postcss.config.js", + ], + "frontend/e2e/tsconfig.json": [".", "../playwright.config.ts"], + }; it("lists every weakening compiler flag", () => { const found: [string, string][] = []; for (const path of PROJECTS) { - const opts = jsonc(path).compilerOptions as Record; + const cfg = jsonc(path); + const opts = cfg.compilerOptions as Record; if (opts.skipLibCheck === true) found.push(["all three projects", "skipLibCheck"]); + for (const flag of STRICT_FAMILY) if (opts[flag] === false) found.push([path, flag]); + for (const key of ["extends", "exclude", "files"]) if (key in cfg) found.push([path, key]); + expect(cfg.include, path).toEqual(INCLUDE[path]); } expect(missingFrom(found)).toEqual([]); }); + it("type-checks every config file at the frontend root, or lists it", () => { + const checked = new Set(INCLUDE["frontend/tsconfig.node.json"]); + const roots = readdirSync(FRONTEND).filter((f) => /[.](ts|js|mjs|cjs)$/.test(f)); + const found = roots + .filter((f) => !checked.has(f)) + .map((f): [string, string] => ["config files", `${f} — not type-checked`]); + expect(missingFrom(found)).toEqual([]); + }); // --- Ruff ----------------------------------------------------------------------------------------- - it("lists every ruff ignore and immutable call", () => { + // Ruff, pyright, pytest and knip are read as KEY whitelists: a key this test does not know (an + // `extend-exclude`, a `lint.extend-ignore`, a narrowing `select`, a pyright `exclude`, a pytest + // `-m "not slow"`, a knip `rules`) fails until someone decides whether it narrows (E32 review — + // only `ignore` and the immutable calls used to be read). + const tomlKeys = (toml: string) => + toml + .split("\n") + .map((l) => /^\s*([A-Za-z0-9_.-]+)\s*=/.exec(l)?.[1]) + .filter((k): k is string => k !== undefined); + it("lists every ruff ignore and immutable call, and knows every key", () => { const toml = read("backend/ruff.toml"); const list = (key: string) => { const m = new RegExp(`^${key}\\s*=\\s*\\[([^\\]]*)\\]`, "m").exec(toml); return m ? [...m[1]!.matchAll(/"([^"]+)"/g)].map((x) => x[1]!) : []; }; + expect(tomlKeys(toml).sort()).toEqual( + ["extend-immutable-calls", "extend-select", "ignore", "line-length", "target-version"].sort(), + ); + // the selection on top of ruff's defaults (E, F) — dropping one narrows the lane + expect(list("extend-select")).toEqual([ + "LOG", + "G", + "T20", + "TRY400", + "TRY401", + "S110", + "S112", + "BLE001", + "RUF100", + "PLW1510", + "B", + ]); const found: [string, string][] = [ ...list("ignore").map((code): [string, string] => ["backend + scripts", code]), ...list("extend-immutable-calls").map((c): [string, string] => ["backend + scripts", c]), ]; - expect(toml).not.toMatch(/per-file-ignores/); // none today — a new one needs a row and this line expect(missingFrom(found)).toEqual([]); }); // --- basedpyright --------------------------------------------------------------------------------- - it("lists the type-check mode, the baseline and every rule override", () => { + const PYRIGHT_KEYS = [ + "include", + "extraPaths", + "typeCheckingMode", + "pythonVersion", + "pythonPlatform", + ]; + it("lists the type-check mode, the baseline and every rule override, and knows every key", () => { const cfg = jsonc("backend/pyrightconfig.json"); const found: [string, string][] = []; if (cfg.typeCheckingMode !== "all" && cfg.typeCheckingMode !== "strict") found.push(["backend/app + alembic", "typeCheckingMode"]); found.push(["backend/app + alembic", "baseline.json"]); + const weak = [false, "none", "warning", "information", "hint"]; for (const [key, value] of Object.entries(cfg)) - if (key.startsWith("report") && (value === false || value === "none" || value === "warning")) + if (key.startsWith("report") && weak.includes(value as string | boolean)) found.push(["backend/app + alembic", key]); + expect( + Object.keys(cfg).filter((k) => !k.startsWith("report") && !PYRIGHT_KEYS.includes(k)), + ).toEqual([]); expect(cfg.include).toEqual(["app", "alembic"]); // tests/ out — its row says why expect(missingFrom(found)).toEqual([]); }); // --- pytest --------------------------------------------------------------------------------------- - it("keeps pytest strict: no deselection, no warning filters, --strict-markers", () => { + it("keeps pytest strict: the whole tests/ tree, no selection, no warning filters, --strict-markers", () => { const ini = read("backend/pytest.ini"); - expect(ini).toMatch(/--strict-markers/); - expect(ini).not.toMatch(/--deselect|--ignore|-p no:|filterwarnings|\s-k\s/); + const keys = ini + .split("\n") + .filter((l) => !l.trim().startsWith("#")) + .map((l) => /^([a-z_]+)\s*=\s*(.*)$/.exec(l)) + .filter((m) => m !== null); + expect(Object.fromEntries(keys.map((m) => [m[1], m[2]!.trim()]))).toEqual({ + testpaths: "tests", + pythonpath: ".", + addopts: "-q -rs --strict-markers", + }); }); // --- knip ----------------------------------------------------------------------------------------- - it("lists every knip production exclusion and ignore", () => { + it("lists every knip production exclusion and every other key, and runs both modes", () => { const cfg = jsonc("frontend/knip.jsonc") as { project?: string[] } & Record; const found: [string, string][] = (cfg.project ?? []) .filter((p) => p.startsWith("!")) .map((p): [string, string] => ["--production", p]); - for (const key of Object.keys(cfg).filter((k) => k.startsWith("ignore"))) + for (const key of Object.keys(cfg).filter( + (k) => !["$schema", "project", "playwright"].includes(k), + )) found.push(["knip", key]); expect(missingFrom(found)).toEqual([]); + const pkg = JSON.parse(read("frontend/package.json")) as { scripts: Record }; + expect(pkg.scripts.knip).toBe("knip && knip --production"); }); // --- accepted findings ----------------------------------------------------------------------------