diff --git a/.github/scripts/pr-hygiene.cjs b/.github/scripts/pr-hygiene.cjs new file mode 100644 index 000000000..1509f77d2 --- /dev/null +++ b/.github/scripts/pr-hygiene.cjs @@ -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)); +} + +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") + ) { + 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))) { + 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, +}; diff --git a/.github/scripts/pr-hygiene.test.cjs b/.github/scripts/pr-hygiene.test.cjs new file mode 100644 index 000000000..482a05181 --- /dev/null +++ b/.github/scripts/pr-hygiene.test.cjs @@ -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", () => { + 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, []); + }); +}); diff --git a/.github/workflows/pr-hygiene.yml b/.github/workflows/pr-hygiene.yml new file mode 100644 index 000000000..d360a1085 --- /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 000000000..884117fd8 --- /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.