feat(fork): fail CI on a lint warning in fork-owned code - #19
Conversation
Nothing in this repository gated on a lint warning. `vp check` lints everything and exits 0, and no --max-warnings was set anywhere, so warnings in fork-authored files accumulated 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 was that failure one level up, in the one place the apparatus did not look. Scoped to fork-owned paths deliberately, not a repo-wide --max-warnings count. A repo-wide count ratchets against upstream: the first sync landing an upstream warning turns the build red for code the fork cannot fix, and the only response is to raise the number, which is how a ratchet stops meaning anything. Scoped, the twelve upstream warnings present today stay untouched and can grow without ever turning this red. Scope is the three fork-owned directories plus every lintable files: entry in the manifest, deduplicated and filtered by extension. .mjs is included so the fork's own tooling is held to the standard it enforces: a .ts-only filter let the repo-wide lint catch a namespace-node-imports violation in lint-owned.mjs that the gate itself had passed over. The gate compares oxlint's number_of_files against the count handed in, which is what keeps it honest. An explicitly-passed path matching lint.ignorePatterns is skipped silently — measured, not assumed: oxlint reports number_of_files 0 for one. Skipping every path exits non-zero on its own, but skipping some would leave the rest linting clean and the run passing while covering less than it claimed. The guard test does not run the lint. It asserts path selection and the CI wiring, because selection is the half that fails quietly — a gate aimed at the wrong paths still exits 0 — while linting 30 files is a multi-second subprocess that does not belong in a suite finishing 1650 tests in ten seconds. Every failure mode was proven by causing it rather than asserted: a planted unused import fails with its location, a fork path added to lint.ignorePatterns fails and names the ten files it would have skipped, deleting the CI step fails the wiring assertion, and removing the dedup fails its own. Transcripts in the pull request.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
github-actions
Bot
added
vouch:trusted
![cursor[bot]](https://avatars.github.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thermo-nuclear review: request changes.
The fork placement, scoped gate, and number_of_files honesty check are the right shape — walker + collector earn their keep for out-of-tree manifest files and silent ignorePatterns skips. What does not is a second lint-result interpreter on top of --max-warnings 0, an N+1 skip probe on the failure path, and a cast-only test wrapper. Collapse to one control plane and delete the rest.
Also: manifest intent still says filtered to .ts/.tsx while the gate includes .mjs, and the script header points at .fork/notes/FORK-LINT-GUARD-HANDOFF.md, which is not in the tree.
Sent by Cursor Automation: Thermo-nuclear PR review
| const result = NodeChildProcess.spawnSync( | ||
| "vp", | ||
| ["lint", ...files, "--format", "json", "--max-warnings", "0"], | ||
| { | ||
| 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 ?? []; | ||
| 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); | ||
| } |
There was a problem hiding this comment.
i think there's a code-judo move here that makes this much simpler. can we reframe this so these branches disappear?
you pass --max-warnings 0, then never read result.status, and re-derive failure from diagnostics.length plus a hand-rolled location formatter. that is two control planes for one contract — the flag is dead for control flow, and a non-zero exit with empty/missing diagnostics (config blow-up, odd oxlint failure mode) greens while claiming clean.
pick one plane:
- trust the flag — after the
number_of_fileshonesty check,process.exit(result.status ?? 1)(print stdout/stderr or re-run without--format jsonfor humans). delete the diagnostics loop. - trust the JSON — drop
--max-warnings 0and keep the diagnostics path, but then also fail onresult.status !== 0so spawn/tool failures cannot slip past an empty report.
same cut for the skip diagnosis above: N sequential spawnSync calls to rediscover what the count mismatch already proved. print the two counts, point at lint.ignorePatterns, exit. the walker + collector + honesty compare stay — those are load-bearing. the second interpreter and the N+1 probe are not.
| const selectForkOwnedFiles = (manifest: string, root: string): readonly string[] => | ||
| collectForkOwnedFiles(manifest, root) as readonly string[]; |
There was a problem hiding this comment.
this abstraction seems unnecessary. can we just keep the direct flow?
selectForkOwnedFiles exists only to cast the untyped .mjs return. sibling customizationsManifest.test.ts casts once at the import/use site (as ManifestEntry[]) and calls through — no identity wrapper. either cast at each call, or once into a local (const collect = collectForkOwnedFiles as ...), and delete the pass-through.
| Scope is the three fork-owned directories plus every lintable files: entry | ||
| in this manifest, deduplicated — most of those entries already sit under | ||
| one of the directories — and filtered to .ts/.tsx, because files: also | ||
| lists images, CSS, YAML and shell. The gate compares oxlint's |
There was a problem hiding this comment.
intent says "filtered to .ts/.tsx" but lint-owned.mjs includes .mjs (and the guard asserts that). the script header also points at .fork/notes/FORK-LINT-GUARD-HANDOFF.md, which is not in the tree. fix the intent to match the gate, and either land the note or point at #fork-lint-cleanliness like every other guard.
ReviewThe engineering discipline here is real — the §7.3 investigation into But I think the central claim doesn't hold, and it's worth being blunt about it before this lands. 1. The gate would not have caught the bug it was built forThe PR opens with: "Closes the gap that review identified: three PRs touched the same nine dead imports." All nine of those imports were in That path is not in the gate's scope. It isn't under any of the three And against the tree today: So run this gate on the pre-#16 tree and it prints I want to be clear that the §3 scoping decision is correct — I'm not arguing for a repo-wide That doesn't sink the PR, but it does mean the summary line is wrong. Either extend scope to an explicit list of fenced files whose warnings the fork accepts ownership of, or say plainly in the manifest intent that fenced hunks in upstream files remain uncovered and the original failure mode is still open. What shouldn't ship is a manifest entry asserting the gap is closed when the specific nine imports that motivated it would still slip through. 2.
|
Review of #19 established that the gate would not have caught the bug it was built for. All nine dead imports were in apps/web/src/components/SidebarV2.tsx, which sits under no fork-owned directory and appears in the manifest only under watch: — a key the selector never reads. Run the first version against the pre-#16 tree and it prints "no warnings" while all nine are live. Verified, and now verified in the other direction too: reinstating one of those imports fails the gate at SidebarV2.tsx:154. The fork's largest authored surface is hunks inside files at upstream paths, and a file-level scope cannot say "the fork owns these lines but not this file". Add an explicit adopted-files list for upstream paths the fork has edited enough to own their lint: SidebarV2.tsx, SidebarChrome.tsx, AppSidebarLayout.tsx. Adoption is not free — an upstream warning in one turns the build red — so ThreadTerminalDrawer.tsx stays out despite carrying fences, because its one warning is upstream's line under upstream's rule config surfaced by upstream's own flag, and no fork change can clear it. Fenced hunks elsewhere remain uncovered; the manifest now says so instead of claiming the gap is closed. Two more fork-owned surfaces were missing on day one: apps/web/fork (the override machinery — the fork's own code by any reading) and .fork itself, which meant the comment claiming detect-drift.mjs was covered was false. Both added, both pinned by tests rather than by comments. The scope is hand-maintained and nothing reconciled it against the tree, which is the same "drifted and nothing noticed" failure this gate exists to prevent, one level up in its own configuration. The guard now walks the tree with a second implementation and demands the selection match, so dropping a directory fails in something that did not read the list. Also from review: the CI assertion was toContain, which stayed true with the step commented out, given if: false, or moved to another job — the same unfalsifiable shape as the CLAUDE.md guard in #18. Anchored to the check job and asserted unconditional; both evasions now fail it. result.status was discarded, so a non-zero exit with no diagnostics would have read as clean; it is a backstop now, no such case reproduced. walk() returned [] for a missing directory and now throws. --report-unused-disable-directives added to match the repo's own lint script, so the gate is never weaker than the lint it enforces. The manifest said .ts/.tsx after .mjs had been added — the register of record had already drifted from the code it registers. The gate caught one warning in this very commit: prefer-set-has in the new guard test, in code written to enforce exactly that standard.
Addressed — findings 1-8You were right on the central claim, and it was the one that mattered. Verified every finding before acting. 1. The gate would not have caught the nine imports — confirmed, fixedOnly under Added an adopted-files list for upstream paths the fork has edited enough to own their lint:
The manifest now states plainly that fenced hunks in non-adopted upstream files remain uncovered, rather than asserting the gap is closed. 2 & 3.
|
| Check | Result |
|---|---|
| Gate | 37 files, no warnings, exit 0 |
apps/web tests |
189 files, 1655 tests, pass |
| Guard tests (9, was 6) | pass |
typecheck |
exit 0 |
lint |
exit 0 — 0 errors, 12 warnings (unchanged) |
fmt --check |
clean |
Pushed f1a0d36d3.
Generated by Claude Code
1 participant
Implements the guard specified in #17. Closes the gap that review identified: three PRs touched the same nine dead imports because nothing fails on a lint warning —
vp checklints everything and exits 0, and no--max-warningswas set anywhere.What it does
.fork/lint-owned.mjslints every fork-owned file and fails on the first warning. Wired as a fenced step in CI'scheckjob, registered asfork-lint-cleanlinessin the manifest, with a guard test.Scoped to fork-owned paths, not repo-wide — the decision §3 of the handoff argues for. A repo-wide
--max-warnings 12ratchets against upstream: the first sync landing an upstream warning goes red for code the fork can't fix, and the only response is raising the number. The 12 upstream warnings today stay untouched and can grow freely.§8 — I took a fourth option
The handoff offered three (guard test shells out / CI-only / text-assert the wiring) and recommended the third. I split the concern instead:
The recommended option pins wiring but never tests selection, which is the part most likely to silently rot. This tests the risky logic cheaply and leaves the expensive check where expensive checks already live.
§7 unknowns — resolved
§7.3 (
lint.ignorePatternsinteraction) was the important one, and the hazard is real. Measured:An explicitly-passed ignored path is silently skipped. Skip all paths and oxlint exits non-zero on its own — but skip some and the rest lint clean and the gate passes while covering less than it claims.
So the gate compares oxlint's
number_of_filesagainst the count it handed in. That check is what makes this trustworthy rather than decorative.§7.2 (dedup / overlap): measured — dropping the dedup takes 29 paths to 39, and oxlint counts each occurrence rather than collapsing. So duplicates don't produce a false green, they just duplicate work and overstate coverage. I'd written the opposite in a code comment before measuring; corrected.
Proof it fails — caused, not asserted
Per §5 step 6.
Clean:
1. Unused import planted in a fork-owned file:
2. Fork path added to
lint.ignorePatterns:3. CI step deleted →
keeps the gate wired into CIfails.4. Dedup removed →
never hands the same path over twicefails.All four reverted; final state clean.
One thing the gate caught in itself
The first version filtered to
.ts/.tsx, so.fork/lint-owned.mjswasn't in its own scope — and the repo-wide lint then flagged at3code(namespace-node-imports)violation in it that the gate had passed over. Added.mjsso the fork's own tooling is held to the standard it enforces, with a test pinning it.Verification
node .fork/lint-owned.mjsapps/webtestsvite.config.ts+ci.ymlfork-lint-cleanlinesstypechecklintfmt --checkNot verified
This has never run in an Actions runner — §7.1, still open until this PR's own CI proves it. That's the one remaining unknown, and this PR is the experiment.
🤖 Generated with Claude Code