diff --git a/.github/scripts/pr-hygiene.cjs b/.github/scripts/pr-hygiene.cjs new file mode 100644 index 0000000000..a910be674c --- /dev/null +++ b/.github/scripts/pr-hygiene.cjs @@ -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)/; +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)); +} + +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)); +} + +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("#"), + ); +} + +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), + ); + + 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") + ) { + 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))) { + 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, +}; diff --git a/.github/scripts/pr-hygiene.test.cjs b/.github/scripts/pr-hygiene.test.cjs new file mode 100644 index 0000000000..16f5f79a37 --- /dev/null +++ b/.github/scripts/pr-hygiene.test.cjs @@ -0,0 +1,205 @@ +"use strict"; + +const { describe, it } = require("node:test"); +const assert = require("node:assert/strict"); +const { + addedLines, + assessHygiene, + hasEmptyCatch, + resultLines, + resultLinesByHunk, +} = 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", () => { + 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("does not read an empty catch across a hunk boundary", () => { + // Hunks are disjoint windows onto the file. 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 exists nowhere in the file. + const crossHunk = [ + "@@ -10,2 +10,3 @@", + "+ const a = 1;", + " } catch (e) {", + "@@ -90,2 +90,3 @@", + " }", + "+ const b = 2;", + ].join("\n"); + assert.equal(resultLinesByHunk(crossHunk).some((w) => hasEmptyCatch(w)), false); + + // A catch emptied within one window is still caught. + const realEmpty = ["@@ -10,3 +10,3 @@", "- report(e);", " } catch (e) {", " }"].join("\n"); + assert.equal(resultLinesByHunk(realEmpty).some((w) => hasEmptyCatch(w)), true); + }); + + it("treats a deleted lockfile as no dependency change", () => { + // Removing bun.lock adds no dependency. The generated-output and + // regression-test checks already exclude removals; this one did not. + assert.deepEqual( + assessHygiene({ files: [{ filename: "bun.lock", status: "removed", patch: "@@\n-x" }] }), + [], + ); + // A modified or MOVED lockfile with no manifest beside it is still orphaned. + assert.equal( + assessHygiene({ files: [{ filename: "bun.lock", status: "modified", patch: "@@\n+x" }] })[0].code, + "orphan_lockfile", + ); + assert.equal( + assessHygiene({ files: [ + { filename: "lock/bun.lock", previous_filename: "bun.lock", status: "renamed", patch: "@@\n+x" }, + ] })[0].code, + "orphan_lockfile", + ); + }); + + it("does not demand a test for a comment-only source change", () => { + // This repository asks for dense explanatory comments in source. A PR that + // only sharpens one changed no behavior, and forcing it through the label + // escape would teach contributors to request the label instead of writing + // tests — weakening the gate exactly where it matters. + assert.deepEqual(assessHygiene({ files: [ + { filename: "src/router.ts", patch: "@@\n+// clarify why this fails closed\n-// old wording" }, + ] }), []); + assert.deepEqual(assessHygiene({ files: [ + { filename: "src/router.ts", patch: "@@\n+/**\n+ * why this is bounded\n+ */" }, + ] }), []); + }); + + it("still demands a test when a comment change carries any code", () => { + for (const patch of [ + "@@\n+// note\n+const y = 2;", + "@@\n+// looks harmless\n+runUntrusted(payload);", + "@@\n-const y = 2;\n+// removed the line", + ]) { + const failures = assessHygiene({ files: [{ filename: "src/router.ts", patch }] }); + assert.equal(failures[0].code, "missing_regression_test", patch); + } + }); + + 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, []); + }); +}); diff --git a/.github/workflows/issue-quality-tests.yml b/.github/workflows/issue-quality-tests.yml index 8c6d0da143..0d36e0b2af 100644 --- a/.github/workflows/issue-quality-tests.yml +++ b/.github/workflows/issue-quality-tests.yml @@ -11,6 +11,8 @@ on: - ".github/scripts/pr-labeler.cjs" - ".github/scripts/pr-labeler.test.cjs" - ".github/scripts/enforce-pr-target.test.cjs" + - ".github/scripts/pr-hygiene.cjs" + - ".github/scripts/pr-hygiene.test.cjs" - ".github/scripts/issue-translation.cjs" - ".github/scripts/issue-translation.test.cjs" - ".github/scripts/issue-triage.cjs" @@ -22,6 +24,7 @@ on: - ".github/workflows/pr-labeler.yml" - ".github/workflows/issue-triage.yml" - ".github/workflows/issue-quality-tests.yml" + - ".github/workflows/pr-hygiene.yml" push: paths: - ".github/ISSUE_TEMPLATE/**" @@ -32,6 +35,8 @@ on: - ".github/scripts/pr-labeler.cjs" - ".github/scripts/pr-labeler.test.cjs" - ".github/scripts/enforce-pr-target.test.cjs" + - ".github/scripts/pr-hygiene.cjs" + - ".github/scripts/pr-hygiene.test.cjs" - ".github/scripts/issue-translation.cjs" - ".github/scripts/issue-translation.test.cjs" - ".github/scripts/issue-triage.cjs" @@ -43,6 +48,7 @@ on: - ".github/workflows/pr-labeler.yml" - ".github/workflows/issue-triage.yml" - ".github/workflows/issue-quality-tests.yml" + - ".github/workflows/pr-hygiene.yml" permissions: contents: read @@ -63,6 +69,7 @@ jobs: node --test .github/scripts/pr-quality.test.cjs node --test .github/scripts/pr-labeler.test.cjs node --test .github/scripts/enforce-pr-target.test.cjs + node --test .github/scripts/pr-hygiene.test.cjs node --test .github/scripts/issue-translation.test.cjs node --test .github/scripts/issue-triage.test.cjs node --test .github/scripts/parse-issue-translation-response.test.cjs diff --git a/.github/workflows/pr-hygiene.yml b/.github/workflows/pr-hygiene.yml new file mode 100644 index 0000000000..d360a10856 --- /dev/null +++ b/.github/workflows/pr-hygiene.yml @@ -0,0 +1,143 @@ +name: PR hygiene + +on: + pull_request_target: + types: [opened, reopened, synchronize, labeled, unlabeled] + +# Trusted default-branch script only. Patches are read through the GitHub API; +# PR-head code is never checked out or executed. +# Least privilege: no default permissions; the hygiene job grants only what it needs. +permissions: {} + +concurrency: + group: pr-hygiene-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + hygiene: + runs-on: ubuntu-latest + # contents: read for the trusted script checkout; issues/pull-requests write + # maintain the blocked label and one bot comment. + permissions: + contents: read + issues: write + pull-requests: write + steps: + - name: Checkout trusted hygiene script + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + with: + ref: ${{ github.event.repository.default_branch }} + persist-credentials: false + sparse-checkout: .github/scripts + + - name: Enforce deterministic PR hygiene + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 + with: + script: | + const path = require("node:path"); + const { assessHygiene } = require( + path.join(process.cwd(), ".github", "scripts", "pr-hygiene.cjs"), + ); + + const { owner, repo } = context.repo; + const pull_number = context.payload.pull_request.number; + const marker = ""; + const blockedLabel = "intake: hygiene-blocked"; + const labelDefinitions = { + [blockedLabel]: ["b60205", "Deterministic PR hygiene checks failed"], + "test-exception-approved": ["5319e7", "Maintainer approved a non-automated regression-test exception"], + "suppression-approved": ["5319e7", "Maintainer approved a new type or lint suppression"], + "generated-change-approved": ["5319e7", "Maintainer approved committed generated output"], + "dependency-change-approved": ["5319e7", "Maintainer approved exceptional dependency or lockfile handling"], + }; + + async function ensureLabel(name) { + try { + await github.rest.issues.getLabel({ owner, repo, name }); + } catch (error) { + if (error.status !== 404) throw error; + const [color, description] = labelDefinitions[name]; + try { + await github.rest.issues.createLabel({ owner, repo, name, color, description }); + } catch (createError) { + if (createError.status !== 422) throw createError; + } + } + } + for (const name of Object.keys(labelDefinitions)) await ensureLabel(name); + + const { data: pr } = await github.rest.pulls.get({ owner, repo, pull_number }); + const files = await github.paginate(github.rest.pulls.listFiles, { + owner, repo, pull_number, per_page: 100, + }); + const labels = new Set(pr.labels.map((label) => label.name)); + // Exception approvals are head-specific: a new commit invalidates + // them, so a contributor cannot obtain one narrow exception and + // then push unreviewed violations under the same label. + if (context.payload.action === "synchronize") { + for (const name of [ + "test-exception-approved", + "suppression-approved", + "generated-change-approved", + "dependency-change-approved", + ]) { + if (labels.has(name)) { + await github.rest.issues.removeLabel({ + owner, repo, issue_number: pull_number, name, + }); + labels.delete(name); + } + } + } + const failures = assessHygiene({ files, labels: [...labels] }); + + async function setBlocked(blocked) { + if (blocked && !labels.has(blockedLabel)) { + await github.rest.issues.addLabels({ + owner, repo, issue_number: pull_number, labels: [blockedLabel], + }); + } else if (!blocked && labels.has(blockedLabel)) { + await github.rest.issues.removeLabel({ + owner, repo, issue_number: pull_number, name: blockedLabel, + }); + } + } + + async function upsert(body) { + const comments = await github.paginate(github.rest.issues.listComments, { + owner, repo, issue_number: pull_number, per_page: 100, + }); + const existing = comments.find( + (comment) => comment.user?.login === "github-actions[bot]" && comment.body?.includes(marker), + ); + if (existing) { + await github.rest.issues.updateComment({ owner, repo, comment_id: existing.id, body }); + } else { + await github.rest.issues.createComment({ owner, repo, issue_number: pull_number, body }); + } + } + + if (failures.length === 0) { + await setBlocked(false); + await upsert(`${marker}\n\n✅ **Deterministic PR hygiene checks passed.**`); + return; + } + + const explanations = { + missing_regression_test: "Behavior changed under `src/` or `gui/src/` without a test change. Add focused coverage or obtain `test-exception-approved`.", + generated_output: "Generated build output is committed. Remove it or obtain `generated-change-approved`.", + orphan_lockfile: "`bun.lock` changed without `package.json`. Revert accidental churn or obtain `dependency-change-approved`.", + new_suppression: "A new TypeScript, lint, formatter, or similar suppression was added. Fix the underlying issue or obtain `suppression-approved`.", + focused_or_skipped_test: "A focused or skipped test was added. Restore the complete suite or obtain `test-exception-approved`.", + empty_catch: "An empty catch block was added. Handle, report, or deliberately propagate the error.", + }; + const lines = failures.map((failure) => { + const paths = failure.paths?.length + ? ` Paths: ${failure.paths.map((p) => `\`${p}\``).join(", ")}.` + : ""; + return `- **${failure.code}** — ${explanations[failure.code]}${paths}`; + }); + + await setBlocked(true); + await upsert([marker, "", "⚠️ **Deterministic hygiene checks failed.**", "", ...lines].join("\n")); + core.setFailed(`PR hygiene failed: ${failures.map((f) => f.code).join(", ")}`); diff --git a/docs/superpowers/specs/2026-08-02-pr-hygiene-design.md b/docs/superpowers/specs/2026-08-02-pr-hygiene-design.md new file mode 100644 index 0000000000..884117fd80 --- /dev/null +++ b/docs/superpowers/specs/2026-08-02-pr-hygiene-design.md @@ -0,0 +1,20 @@ +# Deterministic anti-slop CI — Design + +**Stack:** 4/5, based on `agent/pr-trust-lane` + +This layer rejects concrete defect patterns rather than guessing whether code was AI-generated. + +Blocking checks: + +- runtime or dashboard behavior changed without a test change; +- newly added TypeScript/lint/formatter suppressions; +- newly focused or skipped tests; +- empty catch blocks; +- committed generated build output; +- `bun.lock` churn without `package.json`. + +Narrow exception labels exist for cases that genuinely need maintainer judgment. Empty catches have no bypass because swallowing errors without behavior is not an acceptable implementation choice. + +The workflow reads PR patches through GitHub APIs using trusted default-branch code and never executes the PR head. + +Empty-catch detection scans hunk context as well as additions when a hunk deletes lines, so removing a catch body cannot bypass the rule. Removed generated files and removed test files are excluded from the generated-output and regression-coverage checks respectively. Exception labels are head-specific: a `synchronize` event revokes them so approvals cannot cover unreviewed new commits.