From 1bde3488577457279f1a8fe818417867b9fee88d Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Sun, 2 Aug 2026 20:14:53 +0200 Subject: [PATCH 1/6] chore: add deterministic PR hygiene gate --- .github/scripts/pr-hygiene.cjs | 112 ++++++++++++++++ .github/scripts/pr-hygiene.test.cjs | 80 ++++++++++++ .github/workflows/pr-hygiene.yml | 121 ++++++++++++++++++ .../specs/2026-08-02-pr-hygiene-design.md | 18 +++ 4 files changed, 331 insertions(+) create mode 100644 .github/scripts/pr-hygiene.cjs create mode 100644 .github/scripts/pr-hygiene.test.cjs create mode 100644 .github/workflows/pr-hygiene.yml create mode 100644 docs/superpowers/specs/2026-08-02-pr-hygiene-design.md diff --git a/.github/scripts/pr-hygiene.cjs b/.github/scripts/pr-hygiene.cjs new file mode 100644 index 0000000000..2a8fd7da77 --- /dev/null +++ b/.github/scripts/pr-hygiene.cjs @@ -0,0 +1,112 @@ +"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 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 behaviorChanged = filenames.some(isBehaviorPath); + const testsChanged = filenames.some(isTestPath); + + if ( + behaviorChanged && + !testsChanged && + !labelSet.has("test-exception-approved") + ) { + failures.push({ code: "missing_regression_test" }); + } + + const generated = filenames.filter(isGeneratedPath); + 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); + } + if (hasEmptyCatch(lines)) 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, + isBehaviorPath, + isGeneratedPath, + isTestPath, +}; diff --git a/.github/scripts/pr-hygiene.test.cjs b/.github/scripts/pr-hygiene.test.cjs new file mode 100644 index 0000000000..e435542c9c --- /dev/null +++ b/.github/scripts/pr-hygiene.test.cjs @@ -0,0 +1,80 @@ +"use strict"; + +const { describe, it } = require("node:test"); +const assert = require("node:assert/strict"); +const { addedLines, assessHygiene, hasEmptyCatch } = 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); + }); +}); + +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("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("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 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 0000000000..8895fac219 --- /dev/null +++ b/.github/workflows/pr-hygiene.yml @@ -0,0 +1,121 @@ +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. +permissions: + contents: read + issues: write + pull-requests: write + +concurrency: + group: pr-hygiene-${{ github.event.pull_request.number }} + cancel-in-progress: true + +jobs: + hygiene: + runs-on: ubuntu-latest + 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)); + 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..33a6566488 --- /dev/null +++ b/docs/superpowers/specs/2026-08-02-pr-hygiene-design.md @@ -0,0 +1,18 @@ +# 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. From 735ca1a2b47a60234a37546f8ac225cbe5004736 Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Mon, 3 Aug 2026 04:23:22 +0200 Subject: [PATCH 2/6] fix(ci): classify renamed files on both sides in hygiene gate, scope job permissions --- .github/scripts/pr-hygiene.cjs | 12 +++++++++--- .github/scripts/pr-hygiene.test.cjs | 21 +++++++++++++++++++++ .github/workflows/pr-hygiene.yml | 12 ++++++++---- 3 files changed, 38 insertions(+), 7 deletions(-) diff --git a/.github/scripts/pr-hygiene.cjs b/.github/scripts/pr-hygiene.cjs index 2a8fd7da77..2d24a381dc 100644 --- a/.github/scripts/pr-hygiene.cjs +++ b/.github/scripts/pr-hygiene.cjs @@ -42,8 +42,14 @@ function assessHygiene({ files = [], labels = [] }) { const labelSet = new Set(labels); const failures = []; const filenames = files.map((file) => file.filename); - const behaviorChanged = filenames.some(isBehaviorPath); - const testsChanged = filenames.some(isTestPath); + // 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); + const testsChanged = allPaths.some(isTestPath); if ( behaviorChanged && @@ -53,7 +59,7 @@ function assessHygiene({ files = [], labels = [] }) { failures.push({ code: "missing_regression_test" }); } - const generated = filenames.filter(isGeneratedPath); + const generated = allPaths.filter(isGeneratedPath); if ( generated.length > 0 && !labelSet.has("generated-change-approved") diff --git a/.github/scripts/pr-hygiene.test.cjs b/.github/scripts/pr-hygiene.test.cjs index e435542c9c..755eff734e 100644 --- a/.github/scripts/pr-hygiene.test.cjs +++ b/.github/scripts/pr-hygiene.test.cjs @@ -32,6 +32,27 @@ describe("assessHygiene", () => { }), []); }); + 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();" }, diff --git a/.github/workflows/pr-hygiene.yml b/.github/workflows/pr-hygiene.yml index 8895fac219..3db1e8a46d 100644 --- a/.github/workflows/pr-hygiene.yml +++ b/.github/workflows/pr-hygiene.yml @@ -6,10 +6,8 @@ on: # Trusted default-branch script only. Patches are read through the GitHub API; # PR-head code is never checked out or executed. -permissions: - contents: read - issues: write - pull-requests: write +# Least privilege: no default permissions; the hygiene job grants only what it needs. +permissions: {} concurrency: group: pr-hygiene-${{ github.event.pull_request.number }} @@ -18,6 +16,12 @@ concurrency: 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 From ff320969bfda90624908c7e001bcda32459d1df2 Mon Sep 17 00:00:00 2001 From: Wibias <37517432+Wibias@users.noreply.github.com> Date: Mon, 3 Aug 2026 04:35:19 +0200 Subject: [PATCH 3/6] fix(ci): revoke hygiene exceptions on new commits, catch emptied catches, allow removals --- .github/scripts/pr-hygiene.cjs | 41 +++++++++++++++++-- .github/scripts/pr-hygiene.test.cjs | 37 ++++++++++++++++- .github/workflows/pr-hygiene.yml | 18 ++++++++ .../specs/2026-08-02-pr-hygiene-design.md | 2 + 4 files changed, 94 insertions(+), 4 deletions(-) diff --git a/.github/scripts/pr-hygiene.cjs b/.github/scripts/pr-hygiene.cjs index 2d24a381dc..1509f77d25 100644 --- a/.github/scripts/pr-hygiene.cjs +++ b/.github/scripts/pr-hygiene.cjs @@ -21,6 +21,28 @@ function addedLines(patch) { .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)); } @@ -42,6 +64,11 @@ 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) => @@ -49,7 +76,10 @@ function assessHygiene({ files = [], labels = [] }) { ); const allPaths = [...new Set([...filenames, ...previousFilenames])]; const behaviorChanged = allPaths.some(isBehaviorPath); - const testsChanged = allPaths.some(isTestPath); + // Deleted tests add no coverage and must not satisfy the regression gate. + const testsChanged = allPaths.some( + (path) => isTestPath(path) && !removedFilenames.has(path), + ); if ( behaviorChanged && @@ -59,7 +89,9 @@ function assessHygiene({ files = [], labels = [] }) { failures.push({ code: "missing_regression_test" }); } - const generated = allPaths.filter(isGeneratedPath); + const generated = allPaths.filter( + (path) => isGeneratedPath(path) && !removedFilenames.has(path), + ); if ( generated.length > 0 && !labelSet.has("generated-change-approved") @@ -86,7 +118,8 @@ function assessHygiene({ files = [], labels = [] }) { if (lines.some((line) => FOCUSED_TEST_PATTERN.test(line))) { focusedTests.push(file.filename); } - if (hasEmptyCatch(lines)) emptyCatches.push(file.filename); + const catchLines = hasDeletions(file.patch) ? resultLines(file.patch) : lines; + if (hasEmptyCatch(catchLines)) emptyCatches.push(file.filename); } if ( @@ -112,7 +145,9 @@ 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 index 755eff734e..482a05181f 100644 --- a/.github/scripts/pr-hygiene.test.cjs +++ b/.github/scripts/pr-hygiene.test.cjs @@ -2,7 +2,7 @@ const { describe, it } = require("node:test"); const assert = require("node:assert/strict"); -const { addedLines, assessHygiene, hasEmptyCatch } = require("./pr-hygiene.cjs"); +const { addedLines, assessHygiene, hasEmptyCatch, resultLines } = require("./pr-hygiene.cjs"); describe("patch parsing", () => { it("returns added content without diff headers", () => { @@ -13,6 +13,13 @@ describe("patch parsing", () => { 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", () => { @@ -74,6 +81,20 @@ describe("assessHygiene", () => { 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" }, @@ -82,6 +103,20 @@ describe("assessHygiene", () => { 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: [ diff --git a/.github/workflows/pr-hygiene.yml b/.github/workflows/pr-hygiene.yml index 3db1e8a46d..d360a10856 100644 --- a/.github/workflows/pr-hygiene.yml +++ b/.github/workflows/pr-hygiene.yml @@ -71,6 +71,24 @@ jobs: 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) { diff --git a/docs/superpowers/specs/2026-08-02-pr-hygiene-design.md b/docs/superpowers/specs/2026-08-02-pr-hygiene-design.md index 33a6566488..884117fd80 100644 --- a/docs/superpowers/specs/2026-08-02-pr-hygiene-design.md +++ b/docs/superpowers/specs/2026-08-02-pr-hygiene-design.md @@ -16,3 +16,5 @@ Blocking checks: 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. From 4d381c6bd37579a3a1365be2217526bd3becd05e Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 3 Aug 2026 11:51:41 +0900 Subject: [PATCH 4/6] ci(hygiene): do not demand a regression test for a comment-only source change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The gate asks for a test whenever a path under a behavior prefix appears in the diff. A PR that only rewrites a comment inside `src/` changed no behavior, and this repository asks for dense explanatory comments in exactly those files — so the common case of sharpening one would fail the gate, and the only way out would be a maintainer applying `test-exception-approved`. That is the worst outcome available: it teaches contributors to request the label instead of writing tests, which weakens the gate everywhere it actually matters. A file whose patch contains only comment or blank lines, on both the added and removed sides, no longer counts as a behavior change. Deliberately narrow: one non-comment line anywhere in that file's patch makes it behavior again, so a code change cannot hide behind a comment. Block-comment continuations are recognized only in the leading-asterisk form; anything more clever reads as code and keeps the requirement. Verified against the two shapes I was actually worried about before writing this: `test.skipIf(...)` and `describe.skipIf(...)` already pass the focused-test rule (it matches `.only(`/`.skip(` only), and `catch { /* ... */ }` already passes the empty-catch rule — across the whole tree that rule flags six files, all of them minified. Neither needed a change. --- .github/scripts/pr-hygiene.cjs | 49 ++++++++++++++++++++++++++++- .github/scripts/pr-hygiene.test.cjs | 24 ++++++++++++++ 2 files changed, 72 insertions(+), 1 deletion(-) diff --git a/.github/scripts/pr-hygiene.cjs b/.github/scripts/pr-hygiene.cjs index 1509f77d25..a9d62b167f 100644 --- a/.github/scripts/pr-hygiene.cjs +++ b/.github/scripts/pr-hygiene.cjs @@ -55,6 +55,39 @@ 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); @@ -75,7 +108,21 @@ function assessHygiene({ files = [], labels = [] }) { file.previous_filename ? [file.previous_filename] : [], ); const allPaths = [...new Set([...filenames, ...previousFilenames])]; - const behaviorChanged = allPaths.some(isBehaviorPath); + // 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), diff --git a/.github/scripts/pr-hygiene.test.cjs b/.github/scripts/pr-hygiene.test.cjs index 482a05181f..625e208f13 100644 --- a/.github/scripts/pr-hygiene.test.cjs +++ b/.github/scripts/pr-hygiene.test.cjs @@ -39,6 +39,30 @@ describe("assessHygiene", () => { }), []); }); + 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: "" }, From e03c129e74c354cf0289acd09a365e52cd868d72 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 3 Aug 2026 11:56:58 +0900 Subject: [PATCH 5/6] ci(hygiene): run the hygiene gate's own tests in CI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `pr-hygiene.test.cjs` shipped with the gate but nothing executed it. Its workflow only evaluates pull requests, and Cross-platform CI's `paths:` filter does not match `.github/scripts/**`, so a change that broke the gate's logic would have merged with the suite green — the exact "skipped is not passed" shape this repository already guards elsewhere. This repo already has the right home for it: `issue-quality-tests.yml` runs the policy-script tests and triggers on their own paths. The hygiene script, its test, and its workflow join that list on both the pull-request and push triggers, and the test runs beside the other six. --- .github/workflows/issue-quality-tests.yml | 7 +++++++ 1 file changed, 7 insertions(+) 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 From cc5c06c7349fd868af94c70f78f2f2a308f41743 Mon Sep 17 00:00:00 2001 From: bitkyc08-arch Date: Mon, 3 Aug 2026 12:03:07 +0900 Subject: [PATCH 6/6] ci(hygiene): fold the CodeRabbit review round MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two real false positives, both reproduced before fixing. Empty-catch scanning concatenated every hunk in a file's patch before looking for `catch {}`. Hunks are disjoint windows onto the file, so a hunk ending at `} catch (e) {` followed by one starting at `}` reads as an empty catch that exists nowhere in the file. Scanning is per hunk now; a catch emptied within one window is still caught. The orphan-lockfile check tested `bun.lock` unconditionally while the generated-output and regression-test checks beside it both exclude removals. Deleting `bun.lock` adds no dependency, so it no longer fails. A lockfile that MOVED still does — both sides of a rename count, which the old check also missed. Reported by CodeRabbit on #918. Both driven red by restoring the old behavior. --- .github/scripts/pr-hygiene.cjs | 57 ++++++++++++++++++++++------- .github/scripts/pr-hygiene.test.cjs | 47 +++++++++++++++++++++++- 2 files changed, 90 insertions(+), 14 deletions(-) diff --git a/.github/scripts/pr-hygiene.cjs b/.github/scripts/pr-hygiene.cjs index a9d62b167f..a910be674c 100644 --- a/.github/scripts/pr-hygiene.cjs +++ b/.github/scripts/pr-hygiene.cjs @@ -31,16 +31,36 @@ function hasDeletions(patch) { // 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) { +// +// 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 []; - return patch - .split("\n") - .filter( - (line) => - (line.startsWith("+") && !line.startsWith("+++")) || - line.startsWith(" "), - ) - .map((line) => line.slice(1)); + 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) { @@ -146,9 +166,14 @@ function assessHygiene({ files = [], labels = [] }) { 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 ( - filenames.includes("bun.lock") && - !filenames.includes("package.json") && + allPaths.includes("bun.lock") && + !removedFilenames.has("bun.lock") && + !allPaths.includes("package.json") && !labelSet.has("dependency-change-approved") ) { failures.push({ code: "orphan_lockfile" }); @@ -165,8 +190,13 @@ function assessHygiene({ files = [], labels = [] }) { 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); + // 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 ( @@ -197,4 +227,5 @@ module.exports = { isGeneratedPath, isTestPath, resultLines, + resultLinesByHunk, }; diff --git a/.github/scripts/pr-hygiene.test.cjs b/.github/scripts/pr-hygiene.test.cjs index 625e208f13..16f5f79a37 100644 --- a/.github/scripts/pr-hygiene.test.cjs +++ b/.github/scripts/pr-hygiene.test.cjs @@ -2,7 +2,13 @@ const { describe, it } = require("node:test"); const assert = require("node:assert/strict"); -const { addedLines, assessHygiene, hasEmptyCatch, resultLines } = require("./pr-hygiene.cjs"); +const { + addedLines, + assessHygiene, + hasEmptyCatch, + resultLines, + resultLinesByHunk, +} = require("./pr-hygiene.cjs"); describe("patch parsing", () => { it("returns added content without diff headers", () => { @@ -39,6 +45,45 @@ describe("assessHygiene", () => { }), []); }); + 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