-
Notifications
You must be signed in to change notification settings - Fork 670
ci: add the deterministic PR hygiene gate (extracted from #903) #918
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
1bde348
735ca1a
ff32096
4d381c6
e03c129
cc5c06c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,231 @@ | ||
| "use strict"; | ||
|
|
||
| const GENERATED_PREFIXES = [ | ||
| "gui/dist/", | ||
| "dist/", | ||
| "coverage/", | ||
| ".next/", | ||
| "node_modules/", | ||
| ]; | ||
| const BEHAVIOR_PREFIXES = ["src/", "gui/src/"]; | ||
| const TEST_PREFIXES = ["tests/"]; | ||
| const TEST_FILE_PATTERN = /(?:^|\/)(?:__tests__\/.*|[^/]+\.(?:test|spec)\.[^.]+)$/; | ||
| const SUPPRESSION_PATTERN = /(?:@ts-ignore|@ts-nocheck|eslint-disable|biome-ignore|prettier-ignore)/; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The suppression pattern omits Useful? React with 👍 / 👎. |
||
| const FOCUSED_TEST_PATTERN = /\b(?:describe|it|test)\.(?:only|skip)\s*\(/; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The pattern detects Useful? React with 👍 / 👎. |
||
|
|
||
| function addedLines(patch) { | ||
| if (typeof patch !== "string") return []; | ||
| return patch | ||
| .split("\n") | ||
| .filter((line) => line.startsWith("+") && !line.startsWith("+++")) | ||
| .map((line) => line.slice(1)); | ||
| } | ||
|
|
||
| function hasDeletions(patch) { | ||
| if (typeof patch !== "string") return false; | ||
| return patch | ||
| .split("\n") | ||
| .some((line) => line.startsWith("-") && !line.startsWith("---")); | ||
| } | ||
|
|
||
| // Lines that survive in the result of a hunk: additions plus context. Used for | ||
| // empty-catch detection when the hunk also deletes lines, so deleting a catch | ||
| // body cannot bypass the check. | ||
| // | ||
| // Returned per hunk, never as one flat list. Hunks are disjoint windows onto the | ||
| // file, so concatenating them puts unrelated lines next to each other: a hunk | ||
| // ending at `} catch (e) {` followed by one starting at `}` reads as an empty | ||
| // catch that does not exist anywhere in the file. | ||
| function resultLinesByHunk(patch) { | ||
| if (typeof patch !== "string") return []; | ||
| const hunks = []; | ||
| let current = null; | ||
| for (const line of patch.split("\n")) { | ||
| if (line.startsWith("@@")) { | ||
| current = []; | ||
| hunks.push(current); | ||
| continue; | ||
| } | ||
| if (current === null) { | ||
| // A patch without a hunk header (some API shapes omit it) is one window. | ||
| current = []; | ||
| hunks.push(current); | ||
| } | ||
| if ((line.startsWith("+") && !line.startsWith("+++")) || line.startsWith(" ")) { | ||
| current.push(line.slice(1)); | ||
| } | ||
| } | ||
| return hunks; | ||
| } | ||
|
|
||
| // Flat form, kept for callers that only need the surviving text of a patch. | ||
| function resultLines(patch) { | ||
| return resultLinesByHunk(patch).flat(); | ||
| } | ||
|
|
||
| function isGeneratedPath(path) { | ||
| return GENERATED_PREFIXES.some((prefix) => path.startsWith(prefix)); | ||
| } | ||
|
Comment on lines
+46
to
+48
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Most generated prefixes are checked only at the repository root because Useful? React with 👍 / 👎. |
||
|
|
||
| function isBehaviorPath(path) { | ||
| return BEHAVIOR_PREFIXES.some((prefix) => path.startsWith(prefix)); | ||
| } | ||
|
|
||
| function isTestPath(path) { | ||
| return TEST_PREFIXES.some((prefix) => path.startsWith(prefix)) || TEST_FILE_PATTERN.test(path); | ||
| } | ||
|
|
||
| // A hunk whose surviving and removed lines are all comments or blank changed no | ||
| // behavior, so it cannot owe a regression test. This matters because the review | ||
| // standard here asks for dense explanatory comments in the source: a PR that | ||
| // only sharpens a comment about WHY something fails closed would otherwise be | ||
| // told to add a test for a change it did not make, and the only escape would be | ||
| // a maintainer label — which trains contributors to ask for the label instead of | ||
| // writing tests, weakening the gate everywhere it actually matters. | ||
| // | ||
| // Deliberately narrow: a single non-comment line anywhere in the file's patch | ||
| // makes the whole file count as behavior again. Block-comment CONTINUATION | ||
| // lines are recognized only in the common leading-asterisk form; anything more | ||
| // clever than that reads as code and keeps the requirement. | ||
| function isCommentOnlyChange(patch) { | ||
| if (typeof patch !== "string") return false; | ||
| const changed = patch | ||
| .split("\n") | ||
| .filter( | ||
| (line) => | ||
| (line.startsWith("+") && !line.startsWith("+++")) || | ||
| (line.startsWith("-") && !line.startsWith("---")), | ||
| ) | ||
| .map((line) => line.slice(1).trim()); | ||
| if (changed.length === 0) return false; | ||
| return changed.every( | ||
| (line) => | ||
| line === "" || | ||
| line.startsWith("//") || | ||
| line.startsWith("/*") || | ||
| line.startsWith("*") || | ||
| line.startsWith("#"), | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Treating every trimmed line beginning with AGENTS.md reference: AGENTS.md:L199-L201 Useful? React with 👍 / 👎. |
||
| ); | ||
| } | ||
|
|
||
| function hasEmptyCatch(lines) { | ||
| const text = lines.join("\n"); | ||
| return /catch\s*(?:\([^)]*\))?\s*\{\s*\}/m.test(text); | ||
| } | ||
|
|
||
| function assessHygiene({ files = [], labels = [] }) { | ||
| const labelSet = new Set(labels); | ||
| const failures = []; | ||
| const filenames = files.map((file) => file.filename); | ||
| const removedFilenames = new Set( | ||
| files | ||
| .filter((file) => file.status === "removed") | ||
| .map((file) => file.filename), | ||
| ); | ||
| // Renames are classified on both sides: moving a behavior or generated file | ||
| // to a documentation path must not bypass the hygiene gates. | ||
| const previousFilenames = files.flatMap((file) => | ||
| file.previous_filename ? [file.previous_filename] : [], | ||
| ); | ||
| const allPaths = [...new Set([...filenames, ...previousFilenames])]; | ||
| // A file whose patch is entirely comments changed no behavior. Renamed-from | ||
| // paths carry no patch of their own, so they are judged by the file that | ||
| // carries them. | ||
| const commentOnlyPaths = new Set( | ||
| files | ||
| .filter((file) => isCommentOnlyChange(file.patch)) | ||
| .flatMap((file) => | ||
| file.previous_filename | ||
| ? [file.filename, file.previous_filename] | ||
| : [file.filename], | ||
| ), | ||
| ); | ||
| const behaviorChanged = allPaths.some( | ||
| (path) => isBehaviorPath(path) && !commentOnlyPaths.has(path), | ||
| ); | ||
| // Deleted tests add no coverage and must not satisfy the regression gate. | ||
| const testsChanged = allPaths.some( | ||
| (path) => isTestPath(path) && !removedFilenames.has(path), | ||
| ); | ||
|
Comment on lines
+127
to
+129
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
AGENTS.md reference: AGENTS.md:L199-L201 Useful? React with 👍 / 👎. |
||
|
|
||
| if ( | ||
| behaviorChanged && | ||
| !testsChanged && | ||
| !labelSet.has("test-exception-approved") | ||
| ) { | ||
| failures.push({ code: "missing_regression_test" }); | ||
| } | ||
|
|
||
| const generated = allPaths.filter( | ||
| (path) => isGeneratedPath(path) && !removedFilenames.has(path), | ||
| ); | ||
| if ( | ||
| generated.length > 0 && | ||
| !labelSet.has("generated-change-approved") | ||
| ) { | ||
| failures.push({ code: "generated_output", paths: generated }); | ||
| } | ||
|
|
||
| // A lockfile that MOVED with no manifest beside it is still orphaned, so both | ||
| // sides of a rename count. A lockfile that was DELETED is not: dropping | ||
| // `bun.lock` adds no dependency, which is why the generated-output and | ||
| // regression-test checks above exclude removals the same way. | ||
| if ( | ||
| allPaths.includes("bun.lock") && | ||
| !removedFilenames.has("bun.lock") && | ||
| !allPaths.includes("package.json") && | ||
| !labelSet.has("dependency-change-approved") | ||
|
Comment on lines
+149
to
+152
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This exact-name check covers only the repository-root lockfile. The repository also contains independent Useful? React with 👍 / 👎. |
||
| ) { | ||
| failures.push({ code: "orphan_lockfile" }); | ||
| } | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| const suppressions = []; | ||
| const focusedTests = []; | ||
| const emptyCatches = []; | ||
| for (const file of files) { | ||
| const lines = addedLines(file.patch); | ||
| if (lines.some((line) => SUPPRESSION_PATTERN.test(line))) { | ||
| suppressions.push(file.filename); | ||
| } | ||
| if (lines.some((line) => FOCUSED_TEST_PATTERN.test(line))) { | ||
| focusedTests.push(file.filename); | ||
| } | ||
| // Scan hunk by hunk: an empty catch has to be empty within one window. | ||
| const catchWindows = hasDeletions(file.patch) | ||
| ? resultLinesByHunk(file.patch) | ||
| : [lines]; | ||
| if (catchWindows.some((window) => hasEmptyCatch(window))) { | ||
| emptyCatches.push(file.filename); | ||
| } | ||
| } | ||
|
|
||
| if ( | ||
| suppressions.length > 0 && | ||
| !labelSet.has("suppression-approved") | ||
| ) { | ||
| failures.push({ code: "new_suppression", paths: suppressions }); | ||
| } | ||
| if ( | ||
| focusedTests.length > 0 && | ||
| !labelSet.has("test-exception-approved") | ||
| ) { | ||
| failures.push({ code: "focused_or_skipped_test", paths: focusedTests }); | ||
| } | ||
| if (emptyCatches.length > 0) { | ||
| failures.push({ code: "empty_catch", paths: emptyCatches }); | ||
| } | ||
|
|
||
| return failures; | ||
| } | ||
|
|
||
| module.exports = { | ||
| addedLines, | ||
| assessHygiene, | ||
| hasEmptyCatch, | ||
| hasDeletions, | ||
| isBehaviorPath, | ||
| isGeneratedPath, | ||
| isTestPath, | ||
| resultLines, | ||
| resultLinesByHunk, | ||
| }; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bin/ocx.mjsis the user-facing npm launcher and contains substantial update, service, and process-launch behavior, but it is outside both behavior prefixes. A PR can therefore change production launcher behavior without any test change or exception label. Include the runtime launcher path in behavior classification.Useful? React with 👍 / 👎.