-
Notifications
You must be signed in to change notification settings - Fork 687
stack 4/5: add deterministic anti-slop CI #903
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
0cbaf4b
3ae7ad2
9f4b7e8
7a6982d
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,153 @@ | ||
| "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)/; | ||
| const FOCUSED_TEST_PATTERN = /\b(?:describe|it|test)\.(?:only|skip)\s*\(/; | ||
|
|
||
| function addedLines(patch) { | ||
| if (typeof patch !== "string") return []; | ||
| return patch | ||
| .split("\n") | ||
| .filter((line) => line.startsWith("+") && !line.startsWith("+++")) | ||
| .map((line) => line.slice(1)); | ||
|
Comment on lines
+20
to
+21
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.
Because Useful? React with 👍 / 👎.
Collaborator
Author
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. [shipping-github] Fixed in |
||
| } | ||
|
|
||
| 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. | ||
| function resultLines(patch) { | ||
| if (typeof patch !== "string") return []; | ||
| return patch | ||
| .split("\n") | ||
| .filter( | ||
| (line) => | ||
| (line.startsWith("+") && !line.startsWith("+++")) || | ||
| line.startsWith(" "), | ||
| ) | ||
| .map((line) => line.slice(1)); | ||
| } | ||
|
|
||
| function isGeneratedPath(path) { | ||
| return GENERATED_PREFIXES.some((prefix) => path.startsWith(prefix)); | ||
| } | ||
|
|
||
| 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); | ||
| } | ||
|
|
||
| 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])]; | ||
| const behaviorChanged = allPaths.some(isBehaviorPath); | ||
| // Deleted tests add no coverage and must not satisfy the regression gate. | ||
| const testsChanged = allPaths.some( | ||
| (path) => isTestPath(path) && !removedFilenames.has(path), | ||
| ); | ||
|
|
||
| if ( | ||
| behaviorChanged && | ||
| !testsChanged && | ||
| !labelSet.has("test-exception-approved") | ||
|
Comment on lines
+85
to
+87
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.
Once AGENTS.md reference: .github/AGENTS.md:L15-L17 Useful? React with 👍 / 👎.
Collaborator
Author
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. [shipping-github] Fixed in |
||
| ) { | ||
| 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 }); | ||
| } | ||
|
|
||
| if ( | ||
| filenames.includes("bun.lock") && | ||
| !filenames.includes("package.json") && | ||
| !labelSet.has("dependency-change-approved") | ||
| ) { | ||
| failures.push({ code: "orphan_lockfile" }); | ||
| } | ||
|
|
||
| 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))) { | ||
|
Comment on lines
+114
to
+118
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.
These checks search every added line as raw text, so string literals, regex definitions, and documentation are treated as executable suppressions or focused tests. Feeding this commit's own patches to Useful? React with 👍 / 👎.
Collaborator
Author
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. [shipping-github] Declined with rationale: raw-text scanning is the deliberate design (documented in the design record), and the four exception labels are the escape hatch for legitimate fixtures/prose that must mention these tokens. A source parser would be disproportionate for a gate whose failure mode is a maintainer reviewing the flagged line. |
||
| focusedTests.push(file.filename); | ||
| } | ||
| const catchLines = hasDeletions(file.patch) ? resultLines(file.patch) : lines; | ||
| if (hasEmptyCatch(catchLines)) 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, | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,136 @@ | ||
| "use strict"; | ||
|
|
||
| const { describe, it } = require("node:test"); | ||
| const assert = require("node:assert/strict"); | ||
| const { addedLines, assessHygiene, hasEmptyCatch, resultLines } = require("./pr-hygiene.cjs"); | ||
|
|
||
| describe("patch parsing", () => { | ||
| it("returns added content without diff headers", () => { | ||
| assert.deepEqual(addedLines("+++ b/a.ts\n+const x = 1;\n-old"), ["const x = 1;"]); | ||
| }); | ||
|
|
||
| it("detects empty catch blocks across added lines", () => { | ||
| assert.equal(hasEmptyCatch(["try { work(); } catch (error) {", "}"]), true); | ||
| assert.equal(hasEmptyCatch(["catch (error) {", "report(error);", "}"]), false); | ||
| }); | ||
|
|
||
| it("keeps hunk context and added lines for result scanning", () => { | ||
| assert.deepEqual( | ||
| resultLines(" catch (e) {\n- report(e);\n }"), | ||
| ["catch (e) {", "}"], | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| describe("assessHygiene", () => { | ||
|
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 new test suite is not invoked by any GitHub workflow: Useful? React with 👍 / 👎.
Collaborator
Author
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. |
||
| it("requires regression coverage for behavior changes", () => { | ||
| const failures = assessHygiene({ files: [{ filename: "src/router.ts", patch: "+change" }] }); | ||
| assert.equal(failures[0].code, "missing_regression_test"); | ||
| }); | ||
|
|
||
| it("accepts behavior changes with tests or approved exception", () => { | ||
| assert.deepEqual(assessHygiene({ files: [ | ||
| { filename: "src/router.ts", patch: "+change" }, | ||
| { filename: "tests/router.test.ts", patch: "+test" }, | ||
| ] }), []); | ||
| assert.deepEqual(assessHygiene({ | ||
| files: [{ filename: "src/router.ts", patch: "+change" }], | ||
| labels: ["test-exception-approved"], | ||
| }), []); | ||
| }); | ||
|
|
||
| it("classifies renamed behavior files on both sides", () => { | ||
| const failures = assessHygiene({ files: [ | ||
| { filename: "docs/moved.md", previous_filename: "src/router.ts", patch: "" }, | ||
| ] }); | ||
| assert.equal(failures[0].code, "missing_regression_test"); | ||
| }); | ||
|
|
||
| it("accepts a renamed behavior file when tests are included", () => { | ||
| assert.deepEqual(assessHygiene({ files: [ | ||
| { filename: "docs/moved.md", previous_filename: "src/router.ts", patch: "" }, | ||
| { filename: "tests/moved.test.ts", patch: "+test" }, | ||
| ] }), []); | ||
| }); | ||
|
|
||
| it("classifies renamed generated files on both sides", () => { | ||
| const failures = assessHygiene({ files: [ | ||
| { filename: "docs/notes.md", previous_filename: "gui/dist/index.js", patch: "" }, | ||
| ] }); | ||
| assert.equal(failures[0].code, "generated_output"); | ||
| }); | ||
|
|
||
| it("blocks added suppressions", () => { | ||
| const failures = assessHygiene({ files: [ | ||
| { filename: "tests/a.test.ts", patch: "+// @ts-ignore\n+value();" }, | ||
| ] }); | ||
| assert.equal(failures[0].code, "new_suppression"); | ||
| }); | ||
|
|
||
| it("blocks focused or skipped tests", () => { | ||
| const failures = assessHygiene({ files: [ | ||
| { filename: "tests/a.test.ts", patch: "+test.only(\"x\", () => {});" }, | ||
| ] }); | ||
| assert.equal(failures[0].code, "focused_or_skipped_test"); | ||
| }); | ||
|
|
||
| it("blocks empty catches", () => { | ||
| const failures = assessHygiene({ files: [ | ||
| { filename: "tests/a.test.ts", patch: "+try {} catch (error) {}" }, | ||
| ] }); | ||
| assert.equal(failures[0].code, "empty_catch"); | ||
| }); | ||
|
|
||
| it("detects a catch emptied by deletion", () => { | ||
| const failures = assessHygiene({ files: [ | ||
| { filename: "docs/example.ts", patch: " catch (e) {\n- report(e);\n }" }, | ||
| ] }); | ||
| assert.equal(failures[0].code, "empty_catch"); | ||
| }); | ||
|
|
||
| it("does not flag a nonempty catch in a hunk with unrelated deletions", () => { | ||
| const failures = assessHygiene({ files: [ | ||
| { filename: "docs/example.ts", patch: " catch (e) {\n report(e);\n- old();\n }" }, | ||
| ] }); | ||
| assert.deepEqual(failures, []); | ||
| }); | ||
|
|
||
| it("blocks generated output and orphan lockfile churn", () => { | ||
| const failures = assessHygiene({ files: [ | ||
| { filename: "gui/dist/index.js", patch: "+built" }, | ||
| { filename: "bun.lock", patch: "+package" }, | ||
| ] }); | ||
| assert.deepEqual(failures.map((failure) => failure.code), ["generated_output", "orphan_lockfile"]); | ||
| }); | ||
|
|
||
| it("allows removal of generated output", () => { | ||
| assert.deepEqual(assessHygiene({ files: [ | ||
| { filename: "gui/dist/index.js", status: "removed", patch: "-built" }, | ||
| ] }), []); | ||
| }); | ||
|
|
||
| it("does not count deleted tests as regression coverage", () => { | ||
| const failures = assessHygiene({ files: [ | ||
| { filename: "src/router.ts", patch: "+change" }, | ||
| { filename: "tests/old.test.ts", status: "removed", patch: "-test" }, | ||
| ] }); | ||
| assert.equal(failures[0].code, "missing_regression_test"); | ||
| }); | ||
|
|
||
| it("allows maintainer-approved narrow exceptions", () => { | ||
| const failures = assessHygiene({ | ||
| files: [ | ||
| { filename: "src/router.ts", patch: "+// eslint-disable-next-line\n+run();" }, | ||
| { filename: "gui/dist/index.js", patch: "+built" }, | ||
| { filename: "bun.lock", patch: "+package" }, | ||
| ], | ||
| labels: [ | ||
| "test-exception-approved", | ||
| "suppression-approved", | ||
| "generated-change-approved", | ||
| "dependency-change-approved", | ||
| ], | ||
| }); | ||
| assert.deepEqual(failures, []); | ||
| }); | ||
| }); | ||
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.
The pull-files API does not guarantee a
patchstring for every file, notably for binary or oversized/truncated diffs. Returning an empty line set in that case silently bypasses suppression, focused-test, and empty-catch checks for the affected file. Since this workflow treats untrusted PR metadata as an enforcement boundary, it should retrieve complete content or fail explicitly when an applicable text file has no inspectable patch.AGENTS.md reference: .github/AGENTS.md:L15-L17
Useful? React with 👍 / 👎.
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.
[shipping-github] Declined as a residual: binary files have no executable suppressions to scan, and truncated patches are an inherent GitHub API limit already noted in the review. Text-file content fallback would add a content fetch per file for marginal coverage; the exception labels cover the remaining judgment cases.