diff --git a/.fork/customizations.yaml b/.fork/customizations.yaml index e319ae085ab3..00bff7d2de21 100644 --- a/.fork/customizations.yaml +++ b/.fork/customizations.yaml @@ -564,3 +564,73 @@ - apps/web/package.json verify: - apps/web/src/__fork_guards__/geistTypography.test.ts + +- id: fork-lint-cleanliness + intent: > + Fork-owned code carries zero lint warnings, and CI fails on the first one. + Nothing in this repository otherwise gates on a warning: vp check lints + everything but exits 0, and no --max-warnings is set anywhere, so warnings + in fork-authored files accumulate in silence. Nine dead imports survived + three pull requests that way — removing them was never the difficulty, + noticing was. The guard apparatus exists to catch "the fork drifted and + nothing noticed", and this is that failure one level up, in the one place + the apparatus did not look. + The gate is scoped to fork-owned paths on purpose rather than being a + repo-wide --max-warnings count. A repo-wide count ratchets against + upstream: the first sync that lands an upstream warning turns the build red + for code this fork does not own and cannot fix, and the only available + response is to raise the number, which is how a ratchet stops meaning + anything. Scoped, upstream can add as many warnings as it likes and this + stays green because it never looks there. The twelve upstream warnings + present when this landed are deliberately untouched. + Scope has three parts, filtered to .ts/.tsx/.mjs because files: also lists + images, CSS, YAML and shell. First, the fork-owned directories, which now + include .fork and apps/web/fork — the fork's own tooling and override + machinery, both missed in the first cut. Second, every lintable files: + entry in this manifest. Third, and least obvious, an explicit list of + upstream-path files the fork has edited enough to own their lint output. + That third part is the one that matters. The fork's largest authored + surface is hunks inside files at upstream paths, and a file-level scope + cannot express "the fork owns these lines but not this file". The nine dead + imports that motivated this gate were in SidebarV2.tsx, which appears in + this manifest only under watch: — a key the selector does not read — so a + directories-only scope would have printed "no warnings" while all nine were + live. Review of the implementing PR caught exactly that. + Adoption is not free: an upstream-authored warning in an adopted file turns + the build red, which is the ratchet this entry otherwise argues against. It + is accepted only for files the fork already maintains hunks in and would + have to act on anyway. ThreadTerminalDrawer.tsx is deliberately excluded + despite carrying fork fences, because its one warning is an unused + eslint-disable on upstream's line, under upstream's rule config, surfaced + by upstream's own lint flag — no fork change can clear it. Fenced hunks in + any other upstream file remain uncovered until adopted by name; that gap is + real and stated rather than papered over. + The gate compares oxlint's number_of_files against the count it passed in, + which is what keeps it honest: an explicitly-passed path matching + lint.ignorePatterns is skipped silently, so without that comparison a fork + path landing in an ignore list would produce a green run that inspected + nothing. It also passes --report-unused-disable-directives, matching the + repo's own lint script, so the gate is never weaker than the lint it claims + to enforce. + The guard test deliberately does not run the lint. It asserts the path + selection and the CI wiring, because selection is the half that fails + quietly — a gate pointed at the wrong paths still exits 0 — while the lint + itself is a multi-second subprocess that belongs in CI rather than in a + unit suite that finishes 1600 tests in ten seconds. Selection is checked by + walking the tree independently and demanding the selection match, not by + spot-checks: the scope list is hand-maintained, and three fork-owned + surfaces were missing from it on day one. + tier: 1 + files: + - .fork/lint-owned.mjs + shadows: [] + watch: + # Carries the fenced step that runs the gate. + - .github/workflows/ci.yml + # lint.ignorePatterns lives here. An upstream addition that happens to + # match a fork-owned path would silently narrow the gate; the file-count + # check turns that into a failure, and this entry makes the change visible + # to the drift detector in the first place. + - vite.config.ts + verify: + - apps/web/src/__fork_guards__/forkLintCleanliness.test.ts diff --git a/.fork/lint-owned.mjs b/.fork/lint-owned.mjs new file mode 100644 index 000000000000..64e980d703ab --- /dev/null +++ b/.fork/lint-owned.mjs @@ -0,0 +1,242 @@ +#!/usr/bin/env node +/** + * Fork-owned lint gate — see `.fork/notes/FORK-LINT-GUARD-HANDOFF.md`. + * + * Usage: node .fork/lint-owned.mjs + * + * Lints every file the fork owns and fails on a single warning. Upstream's own + * warnings are untouched: this never looks outside fork-owned paths, so an + * upstream commit that adds a warning can never turn this red for code the + * fork cannot fix. That is the whole reason it is scoped rather than a + * repo-wide `--max-warnings`, which would ratchet against upstream and train + * everyone to raise the number. + * + * Dependency-free by design, like detect-drift.mjs: it runs in a bare Actions + * runner with no install step beyond what CI already does for `vp`. + */ + +import * as NodeChildProcess from "node:child_process"; +import * as NodeFS from "node:fs"; +import * as NodePath from "node:path"; +import * as NodeURL from "node:url"; + +import { parseCustomizations } from "./detect-drift.mjs"; + +const FORK_DIR = NodePath.dirname(NodeURL.fileURLToPath(import.meta.url)); +const REPO_ROOT = NodePath.resolve(FORK_DIR, ".."); + +/** + * Directories the fork owns outright. Everything under them is fork-authored, + * so all of it is in scope regardless of whether the manifest names each file. + */ +export const FORK_OWNED_DIRECTORIES = [ + ".fork", + "apps/web/fork", + "apps/web/src/custom", + "apps/web/src/overrides", + "apps/web/src/__fork_guards__", +]; + +/** + * Upstream-path files the fork has edited enough to own their lint output. + * + * The fork's largest authored surface is not the directories above — it is + * hunks inside files at upstream paths, and a file-level scope cannot say "the + * fork owns these lines but not this file". The nine dead imports that + * motivated this gate lived here, in SidebarV2.tsx, in unfenced lines the + * sidebar extraction stranded. A gate that skipped this list would have + * printed "no warnings" while all nine were live. + * + * Adoption means accepting that an upstream-authored warning in one of these + * turns the build red. That is the ratchet risk §3 of the handoff argues + * against for the repo at large, and it is acceptable here only because the + * fork already maintains hunks in each of these and would have to act anyway. + * + * ThreadTerminalDrawer.tsx is deliberately absent despite carrying fork + * fences: its one warning is an unused eslint-disable that is upstream's line, + * under upstream's rule config, surfaced by upstream's own lint flag. Adopting + * it would mean going red for something no fork change can fix. + */ +export const FORK_ADOPTED_FILES = [ + "apps/web/src/components/AppSidebarLayout.tsx", + "apps/web/src/components/SidebarV2.tsx", + "apps/web/src/components/sidebar/SidebarChrome.tsx", +]; + +/** + * Only these are lintable. `files:` also lists .md, .png, .ico, .css, .yml and + * .sh. `.mjs` is included so the fork's own tooling — this file and + * detect-drift.mjs — is held to the standard it enforces on everything else; + * the repo-wide lint does cover them, and it caught a namespace-node-imports + * violation in this very script that an earlier `.ts`-only filter missed. + */ +const LINTABLE = new Set([".ts", ".tsx", ".mjs"]); + +const isLintable = (path) => LINTABLE.has(NodePath.extname(path)); + +/** + * oxlint prefixes its JSON with a human line ("No files found to lint…") when a + * path is skipped, which is exactly the case this script most needs to read. + * Parse from the first brace rather than assuming the payload starts at byte 0. + */ +const parseReport = (stdout) => { + const start = stdout.indexOf("{"); + if (start === -1) { + return undefined; + } + try { + return JSON.parse(stdout.slice(start)); + } catch { + return undefined; + } +}; + +const walk = (absoluteDir) => { + const out = []; + if (!NodeFS.existsSync(absoluteDir)) { + // Not a silent empty list. A fork-owned directory that has moved or been + // deleted must not quietly shrink the gate's scope — that is precisely the + // "nothing noticed" failure this exists to prevent, relocated into the + // gate's own configuration. + throw new Error(`fork-owned directory is missing: ${absoluteDir}`); + } + for (const entry of NodeFS.readdirSync(absoluteDir, { withFileTypes: true })) { + const absolute = NodePath.join(absoluteDir, entry.name); + if (entry.isDirectory()) { + out.push(...walk(absolute)); + } else if (entry.isFile() && isLintable(entry.name)) { + out.push(absolute); + } + } + return out; +}; + +/** + * Every fork-owned lintable file, repo-relative and deduplicated. + * + * Most `files:` entries already live under a fork-owned directory, so a naive + * concatenation hands oxlint the same path repeatedly. Measured: dropping the + * dedup takes 29 paths to 39, and oxlint counts each occurrence rather than + * collapsing them — so the file-count check below still agrees and the gate + * still passes. The cost is duplicated work and a file count that overstates + * what was actually covered, not a false green. + */ +export const collectForkOwnedFiles = (manifestText, repoRoot = REPO_ROOT) => { + const fromDirectories = FORK_OWNED_DIRECTORIES.flatMap((directory) => + walk(NodePath.join(repoRoot, directory)), + ); + const fromManifest = parseCustomizations(manifestText) + .flatMap((entry) => entry.files) + .filter(isLintable) + .map((relative) => NodePath.join(repoRoot, relative)) + .filter((absolute) => NodeFS.existsSync(absolute)); + + const fromAdopted = FORK_ADOPTED_FILES.map((relative) => { + const absolute = NodePath.join(repoRoot, relative); + if (!NodeFS.existsSync(absolute)) { + throw new Error(`adopted file is missing: ${relative}`); + } + return absolute; + }); + + const relative = [...fromDirectories, ...fromManifest, ...fromAdopted].map((absolute) => + NodePath.relative(repoRoot, absolute), + ); + return [...new Set(relative)].sort(); +}; + +function main() { + const manifestText = NodeFS.readFileSync(NodePath.join(FORK_DIR, "customizations.yaml"), "utf8"); + const files = collectForkOwnedFiles(manifestText); + + if (files.length === 0) { + console.error("fork-lint: no fork-owned files found — the path list is wrong, not clean."); + process.exit(2); + } + + const result = NodeChildProcess.spawnSync( + "vp", + // --report-unused-disable-directives matches the repo's own `lint` script. + // Without it the gate is strictly weaker than the lint it claims to + // enforce: a stale suppression in fork-owned code would pass here and be + // reported by `vp run lint`. + ["lint", ...files, "--report-unused-disable-directives", "--format", "json"], + { + cwd: REPO_ROOT, + encoding: "utf8", + }, + ); + + if (result.error) { + console.error(`fork-lint: could not run vp lint — ${result.error.message}`); + process.exit(2); + } + + const report = parseReport(result.stdout); + if (report === undefined) { + console.error("fork-lint: could not parse oxlint JSON. Raw output follows.\n"); + console.error(result.stdout || result.stderr); + process.exit(2); + } + + // The check that keeps this honest. An explicitly-passed path that matches + // `lint.ignorePatterns` is silently skipped — verified: oxlint reports + // number_of_files 0 for one. Skip every path and it exits non-zero on its + // own, but skip *some* and the rest lint clean and this would pass while + // looking at less than it claims. Compare counts so that cannot happen. + if (report.number_of_files !== files.length) { + console.error( + `fork-lint: expected to lint ${files.length} fork-owned files, oxlint reported ` + + `${report.number_of_files}. Some path was skipped — most likely it now matches ` + + `lint.ignorePatterns in vite.config.ts. Skipped paths:\n`, + ); + for (const file of files) { + const single = NodeChildProcess.spawnSync("vp", ["lint", file, "--format", "json"], { + cwd: REPO_ROOT, + encoding: "utf8", + }); + const singleReport = parseReport(single.stdout); + if (singleReport === undefined || singleReport.number_of_files === 0) { + console.error(` - ${file}`); + } + } + process.exit(1); + } + + const diagnostics = report.diagnostics ?? []; + + // The verdict comes from the parsed report, which is precise about what and + // where. But a non-zero exit with nothing to show for it means oxlint failed + // in a way this script cannot see, and treating that as "no warnings" is the + // exact shape that produces a false green. No such case has been reproduced; + // this is a backstop, not a fix for an observed bug. + if (result.status !== 0 && diagnostics.length === 0) { + console.error( + `fork-lint: vp lint exited ${result.status} but reported no diagnostics. ` + + `Refusing to call that clean. Raw output follows.\n`, + ); + console.error(result.stdout || result.stderr); + process.exit(2); + } + + if (diagnostics.length > 0) { + console.error( + `fork-lint: ${diagnostics.length} warning(s) in fork-owned code. The fork owns these ` + + `files, so there is no upstream to wait for — fix them.\n`, + ); + for (const diagnostic of diagnostics) { + const span = diagnostic.labels?.[0]?.span; + const where = span + ? `${diagnostic.filename}:${span.line}:${span.column}` + : diagnostic.filename; + console.error(` ${where} ${diagnostic.code} ${diagnostic.message}`); + } + process.exit(1); + } + + console.log(`fork-lint: ${files.length} fork-owned files, no warnings.`); +} + +if (process.argv[1] && import.meta.url === NodeURL.pathToFileURL(process.argv[1]).href) { + main(); +} diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d7f133cee396..d9f9a5a49a65 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -33,6 +33,16 @@ jobs: - name: Check run: vp check + # fork:begin fork-lint-cleanliness — see .fork/customizations.yaml#fork-lint-cleanliness + # `vp check` lints the whole repo but exits 0 on warnings, and no + # --max-warnings is set anywhere, so fork-authored dead code accumulates + # silently — it took three PRs to remove nine dead imports because + # noticing was the hard part. This fails on a single warning in + # fork-owned code only, so upstream's warnings can never turn it red. + - name: Lint fork-owned code + run: node .fork/lint-owned.mjs + # fork:end fork-lint-cleanliness + - name: Typecheck run: vpr typecheck diff --git a/apps/web/src/__fork_guards__/forkLintCleanliness.test.ts b/apps/web/src/__fork_guards__/forkLintCleanliness.test.ts new file mode 100644 index 000000000000..d10838d91e45 --- /dev/null +++ b/apps/web/src/__fork_guards__/forkLintCleanliness.test.ts @@ -0,0 +1,171 @@ +// @effect-diagnostics nodeBuiltinImport:off +/** + * Fork guard — see `.fork/customizations.yaml#fork-lint-cleanliness`. + * + * The gate itself runs in CI, not here: linting 37 files is a multi-second + * subprocess and this suite finishes 1600+ tests in about ten seconds. What + * this guard covers is the part that fails *quietly* — the path selection. A + * gate pointed at the wrong paths still exits 0, so "green" would mean + * "inspected nothing" rather than "found nothing". + * + * The scope is a hand-maintained list, and nothing reconciles it against the + * tree. Review of #19 found three fork-owned surfaces missing from it on day + * one — including the file that produced the very imports the gate was built + * for. So the reconciliation tests below walk the tree independently and + * demand the selection match, rather than trusting the list. + */ + +import * as NodeFS from "node:fs"; +import * as NodePath from "node:path"; +import * as NodeURL from "node:url"; +import { describe, expect, it } from "vite-plus/test"; + +// Namespace import kept on one line: @ts-expect-error applies to the line that +// follows it, and a multi-line named import puts the diagnostic on the `from` +// clause instead, leaving the directive itself unused. +// @ts-expect-error — plain .mjs module without type declarations, same as detect-drift.mjs beside it. +import * as LintOwned from "../../../../.fork/lint-owned.mjs"; + +const selectForkOwnedFiles = (manifest: string, root: string): readonly string[] => + LintOwned.collectForkOwnedFiles(manifest, root) as readonly string[]; + +const ownedDirectories = LintOwned.FORK_OWNED_DIRECTORIES as readonly string[]; +const adoptedFiles = LintOwned.FORK_ADOPTED_FILES as readonly string[]; + +const LINTABLE = new Set([".ts", ".tsx", ".mjs"]); + +/** + * Deliberately a second implementation rather than a call into the gate's own + * `walk`. Comparing the selector against itself would prove nothing; this + * exists so a directory dropped from the scope list is caught by something + * that did not read that list. + */ +const walkLintable = (absoluteDir: string): readonly string[] => { + const out: string[] = []; + for (const entry of NodeFS.readdirSync(absoluteDir, { withFileTypes: true })) { + const absolute = NodePath.join(absoluteDir, entry.name); + if (entry.isDirectory()) { + out.push(...walkLintable(absolute)); + } else if (entry.isFile() && LINTABLE.has(NodePath.extname(entry.name))) { + out.push(absolute); + } + } + return out; +}; + +const repoRoot = NodePath.resolve( + NodeURL.fileURLToPath(new URL(".", import.meta.url)), + "../../../..", +); + +const manifestText = NodeFS.readFileSync( + NodePath.join(repoRoot, ".fork/customizations.yaml"), + "utf8", +); + +describe("fork guard: fork-lint-cleanliness", () => { + it("keeps every declared fork-owned directory on disk", () => { + // The gate throws on a missing directory rather than silently narrowing + // its scope, but nothing else pins these paths: they are not manifest + // entries, so customizationsManifest's "paths exist in the tree" check + // does not reach them. `overrides/` in particular held only a README, so + // an assertion that merely looked for selected files under it would have + // stayed true after the directory was deleted. + for (const directory of ownedDirectories) { + expect(NodeFS.existsSync(NodePath.join(repoRoot, directory))).toBe(true); + } + }); + + it("selects every lintable file under every fork-owned directory", () => { + const files = new Set(selectForkOwnedFiles(manifestText, repoRoot)); + // Reconciliation, not spot-checking. Walks the tree independently and + // demands the selection contain everything found, so dropping a directory + // from the scope list fails here instead of silently shrinking coverage. + for (const directory of ownedDirectories) { + for (const absolute of walkLintable(NodePath.join(repoRoot, directory))) { + expect(files).toContain(NodePath.relative(repoRoot, absolute)); + } + } + }); + + it("covers the fork hunks that live at upstream paths", () => { + const files = selectForkOwnedFiles(manifestText, repoRoot); + // The gate's whole reason for existing. The nine dead imports it was built + // for were in SidebarV2.tsx — an upstream path, named in the manifest only + // under watch:, which the selector does not read. Scoped to directories + // alone this gate would have printed "no warnings" while all nine were + // live, which is what review of #19 caught. + expect(files).toContain("apps/web/src/components/SidebarV2.tsx"); + for (const adopted of adoptedFiles) { + expect(files).toContain(adopted); + } + }); + + it("leaves upstream-manufactured warnings out of scope", () => { + const files = selectForkOwnedFiles(manifestText, repoRoot); + // ThreadTerminalDrawer.tsx carries fork fences but its only warning is an + // unused eslint-disable on upstream's line, under upstream's rule config, + // surfaced by upstream's own lint flag. Adopting it would mean going red + // for something no fork change can fix — the ratchet this gate is scoped + // to avoid. Pinned so adoption stays a deliberate act. + expect(files).not.toContain("apps/web/src/components/ThreadTerminalDrawer.tsx"); + }); + + it("picks up fork-owned files the directories do not cover", () => { + const files = selectForkOwnedFiles(manifestText, repoRoot); + // A manifest `files:` entry outside the fork-owned directories. If the + // selector ever collapsed to "just walk the directories", this is lost. + expect(files).toContain("apps/desktop/src/app/DesktopClerkForkSkip.test.ts"); + }); + + it("lints only what can be linted", () => { + const files = selectForkOwnedFiles(manifestText, repoRoot); + // `files:` also lists .md, .png, .ico, .css, .yml and .sh. Handing oxlint a + // PNG is not a hypothetical — nine such entries are in the manifest today. + for (const file of files) { + expect(LINTABLE.has(NodePath.extname(file))).toBe(true); + } + }); + + it("covers the fork's own tooling", () => { + const files = selectForkOwnedFiles(manifestText, repoRoot); + // The gate holds itself to the standard it enforces. It did not at first: + // a .ts-only filter meant the repo-wide lint caught a rule violation in + // this script that the gate itself had passed over. And the comment + // claiming detect-drift.mjs was covered was false until review checked it, + // so both are pinned rather than one standing in for the pair. + expect(files).toContain(".fork/lint-owned.mjs"); + expect(files).toContain(".fork/detect-drift.mjs"); + }); + + it("never hands the same path over twice", () => { + const files = selectForkOwnedFiles(manifestText, repoRoot); + // Most `files:` entries live under a fork-owned directory, so duplicates + // are the default failure. They do not produce a false green — oxlint + // counts each occurrence, so the gate's file-count check still agrees — + // but they duplicate work and inflate the reported coverage, which is + // worth failing on precisely because nothing downstream would notice. + expect(new Set(files).size).toBe(files.length); + }); + + it("runs the gate as an unconditional step of the check job", () => { + const ci = NodeFS.readFileSync(NodePath.join(repoRoot, ".github/workflows/ci.yml"), "utf8"); + expect(ci).toContain("fork:begin fork-lint-cleanliness"); + + // Anchored to position, not presence. `toContain("node .fork/…")` stayed + // true if the step were commented out, given `if: false`, or moved to a + // job that does not run on pull_request — the same unfalsifiable shape as + // the CLAUDE.md guard whose `.trim()` made it unable to fail. Slice the + // check job out and assert the step lives inside it. + const checkJob = /\n {2}check:\n([\s\S]*?)(?=\n {2}\w[\w-]*:\n)/u.exec(ci); + expect(checkJob).not.toBeNull(); + const body = checkJob?.[1] ?? ""; + expect(body).toMatch(/- name: Lint fork-owned code\n\s+run: node \.fork\/lint-owned\.mjs\n/u); + + // And unconditional: a step carrying `if:` can be switched off without + // this file changing. + const step = /- name: Lint fork-owned code\n((?:\s+\w[\w-]*:.*\n)+)/u.exec(body); + expect(step).not.toBeNull(); + expect(step?.[1] ?? "").not.toMatch(/^\s+if:/mu); + }); +});