ci: add the deterministic PR hygiene gate (extracted from #903) - #918
Conversation
…hes, allow removals
…e change
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.
|
Warning Review limit reached
Next review available in: 22 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds a patch-based hygiene assessor, a test suite, and a trusted ChangesPR hygiene enforcement
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PR as Pull request event
participant Workflow as pr-hygiene.yml
participant GitHub as GitHub API
participant Assessor as pr-hygiene.cjs
PR->>Workflow: Trigger hygiene check
Workflow->>GitHub: Retrieve changed files and labels
Workflow->>Assessor: Evaluate trusted patch data
Assessor-->>Workflow: Return hygiene result
Workflow->>GitHub: Update labels and bot comment
Workflow-->>PR: Pass or fail workflow
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
`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.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/scripts/pr-hygiene.cjs:
- Around line 34-44: Update resultLines and the hasEmptyCatch text assembly to
preserve diff-hunk boundaries: split on /^@@ .* @@/ and insert a sentinel
between hunks that cannot be consumed by the empty-catch regex’s whitespace
matching. Ensure catch blocks from separate hunks cannot become adjacent during
scanning, including the joined text path used by hasDeletions.
- Around line 149-155: Update the orphan_lockfile condition in the PR hygiene
checks to ignore bun.lock when it appears only in removedFilenames, and use
allPaths rather than filenames when checking for package.json so renames are
recognized. Preserve the existing dependency-change-approved exemption and
failure code.
In @.github/workflows/pr-hygiene.yml:
- Around line 54-67: Replace the per-label getLabel calls in ensureLabel and the
Object.keys(labelDefinitions) loop with one issues.listLabelsForRepo request to
fetch existing labels, then diff the returned names locally and call createLabel
only for missing definitions. Preserve the existing 422 handling for concurrent
label creation and propagate other errors.
- Around line 126-143: Sanitize untrusted filenames in the `failures.map`
formatting before embedding them in the bot comment: neutralize or remove
backticks from each `p` value before the inline-code interpolation, while
preserving the existing path list and failure message structure.
- Around line 3-5: Update the pr-hygiene workflow’s label-revocation logic in
assessHygiene to run for reopened pull requests as well as synchronize events,
ensuring close→push→reopen cycles revoke stale exception labels before hygiene
assessment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5c202c28-60b3-49fb-834c-e632854d43fc
📒 Files selected for processing (4)
.github/scripts/pr-hygiene.cjs.github/scripts/pr-hygiene.test.cjs.github/workflows/pr-hygiene.ymldocs/superpowers/specs/2026-08-02-pr-hygiene-design.md
| on: | ||
| pull_request_target: | ||
| types: [opened, reopened, synchronize, labeled, unlabeled] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does GitHub Actions pull_request_target fire a synchronize event for commits pushed to a pull request branch while the pull request is closed, and does reopening later re-trigger synchronize or only reopened?
💡 Result:
The pull_request_target event does not trigger for a synchronize activity when a pull request is closed [1]. The synchronize event type is specifically associated with updates to an active (open) pull request's source branch [2][3]. When a pull request is closed, it is no longer in a state that triggers the synchronize activity [1]. Consequently, pushing new commits to the pull request branch while the pull request is closed will not fire a pull_request_target workflow with the synchronize activity [1]. Regarding your second question, reopening a closed pull request triggers a reopened activity type, not a synchronize activity [1]. If you require workflows to run upon the reopening of a pull request, you must explicitly include the reopened activity type in your workflow configuration: on: pull_request_target: types: [opened, synchronize, reopened] If a pull request is reopened, the pull_request_target event will fire with the reopened action, but it will not retrospectively trigger synchronize events for any commits pushed while the pull request was closed [1]. To ensure that workflows process the latest state after a pull request is reopened, you would need to trigger a job based on the reopened event [1].
Citations:
- 1: https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows
- 2: https://runs-on.com/github-actions/pull-request-vs-pull-request-target/
- 3: Add in definition of terms like "synchronize" for Github Action Activity types github/docs#2257
🏁 Script executed:
#!/bin/bash
set -eu
file=".github/workflows/pr-hygiene.yml"
printf '%s\n' "=== workflow lines 1-120 ==="
cat -n "$file" | sed -n '1,120p'
printf '%s\n' "=== related exception references ==="
rg -n -C 3 'test-exception-approved|assessHygiene|exception|synchronize|reopened' .github MAINTAINERS.md 2>/dev/null || trueRepository: lidge-jun/opencodex
Length of output: 34223
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
text = Path(".github/workflows/pr-hygiene.yml").read_text()
trigger_line = "types: [opened, reopened, synchronize, labeled, unlabeled]"
condition = 'if (context.payload.action === "synchronize")'
assert trigger_line in text
assert condition in text
for action in ("synchronize", "reopened"):
revokes = action == "synchronize"
print(f"{action}: exception labels revoked = {revokes}")
print("reopened is configured as a trigger but is not included in the revocation predicate.")
PYRepository: lidge-jun/opencodex
Length of output: 330
Revoke exception labels when a pull request is reopened
GitHub does not emit synchronize for commits pushed while a pull request is closed. Reopening emits only reopened. Because .github/workflows/pr-hygiene.yml:77-91 revokes labels only for synchronize, a close→push→reopen cycle preserves stale approvals, and assessHygiene accepts new violations. Include reopened in the revocation condition or compare the approved head SHA with pr.head.sha.
🧰 Tools
🪛 zizmor (1.28.0)
[error] 3-9: use of fundamentally insecure workflow trigger (dangerous-triggers): pull_request_target is almost always used insecurely
(dangerous-triggers)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/pr-hygiene.yml around lines 3 - 5, Update the pr-hygiene
workflow’s label-revocation logic in assessHygiene to run for reopened pull
requests as well as synchronize events, ensuring close→push→reopen cycles revoke
stale exception labels before hygiene assessment.
Source: Path instructions
| 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); |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
ensureLabel issues a GET (and a conditional POST) per label on every triggering event.
ensureLabel (Lines 54-66) is called once per entry in labelDefinitions (Line 67), so every opened, reopened, synchronize, labeled, and unlabeled event makes 5 getLabel calls even though these 5 labels almost never change after the first run. This is a minor, steady API-call tax rather than a functional problem, given GitHub's generous per-installation rate limits, but it's easy to cut: fetch the repo's label list once (github.rest.issues.listLabelsForRepo) and diff locally, only calling createLabel for the ones actually missing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/pr-hygiene.yml around lines 54 - 67, Replace the per-label
getLabel calls in ensureLabel and the Object.keys(labelDefinitions) loop with
one issues.listLabelsForRepo request to fetch existing labels, then diff the
returned names locally and call createLabel only for missing definitions.
Preserve the existing 422 handling for concurrent label creation and propagate
other errors.
| 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(", ")}`); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Untrusted PR filenames are embedded into the bot's own comment body without escaping backticks.
failure.paths (Line 135) comes directly from github.rest.pulls.listFiles filenames, which a PR author fully controls, including on Linux where backtick (`) is a legal filename character. Line 136 wraps each path in single backticks: `${p}`. A filename containing a backtick terminates the inline code span early, letting the remainder of the string render as arbitrary Markdown inside the github-actions[bot] comment (e.g., injecting a fake "approved" note, a phishing link, or content that looks like a maintainer instruction). Since this bot comment carries implicit trust (it drives the blockedLabel state maintainers rely on), this is a credible comment-spoofing vector introduced by a hostile PR.
Sanitize or strip backticks from p before interpolation:
🛡️ Proposed fix: neutralize backticks in embedded filenames
const lines = failures.map((failure) => {
const paths = failure.paths?.length
- ? ` Paths: ${failure.paths.map((p) => `\`${p}\``).join(", ")}.`
+ ? ` Paths: ${failure.paths.map((p) => `\`${p.replace(/`/g, "'")}\``).join(", ")}.`
: "";
return `- **${failure.code}** — ${explanations[failure.code]}${paths}`;
});As per path instructions for .github/**, workflow changes require explicit security review; this is exactly the class of finding (untrusted input flowing into a trust-bearing artifact) that review should catch.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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(", ")}`); | |
| 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.replace(/`/g, "'")}\``).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(", ")}`); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/pr-hygiene.yml around lines 126 - 143, Sanitize untrusted
filenames in the `failures.map` formatting before embedding them in the bot
comment: neutralize or remove backticks from each `p` value before the
inline-code interpolation, while preserving the existing path list and failure
message structure.
Source: Path instructions
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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d381c6bd3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| permissions: | ||
| contents: read | ||
| issues: write | ||
| pull-requests: write |
There was a problem hiding this comment.
The workflow only calls pulls.get and pulls.listFiles; all label and comment mutations use the separately granted issues: write permission. Granting pull-requests: write therefore gives this pull_request_target job unnecessary authority to modify pull requests, contrary to the repository's least-privilege requirement. Set this permission to read.
AGENTS.md reference: .github/AGENTS.md:L12-L12
Useful? React with 👍 / 👎.
| await github.rest.issues.removeLabel({ | ||
| owner, repo, issue_number: pull_number, name, | ||
| }); | ||
| labels.delete(name); |
There was a problem hiding this comment.
When a PR has multiple exception labels, each removeLabel emits an unlabeled event for this same workflow. Because the concurrency group uses cancel-in-progress: true, the first removal can cancel the synchronize run before the remaining labels are cleared; the replacement unlabeled run skips this revocation block and can assess the new head with a stale approval. Structure revocation so these self-generated events cannot interrupt it, or make every run validate approvals against the current head.
AGENTS.md reference: .github/AGENTS.md:L15-L17
Useful? React with 👍 / 👎.
| line.startsWith("//") || | ||
| line.startsWith("/*") || | ||
| line.startsWith("*") || | ||
| line.startsWith("#"), |
There was a problem hiding this comment.
Treating every trimmed line beginning with # as a comment misclassifies TypeScript private fields and methods. For example, changing only #verifier = ""—a pattern already used under src/oauth/—makes both changed lines comment-only, so the behavior change passes without a regression test. Restrict hash comments to applicable file types or remove this case for src/ and gui/src/.
AGENTS.md reference: AGENTS.md:L199-L201
Useful? React with 👍 / 👎.
| const BEHAVIOR_PREFIXES = ["src/", "gui/src/"]; | ||
| const TEST_PREFIXES = ["tests/"]; | ||
| const TEST_FILE_PATTERN = /(?:^|\/)(?:__tests__\/.*|[^/]+\.(?:test|spec)\.[^.]+)$/; | ||
| const SUPPRESSION_PATTERN = /(?:@ts-ignore|@ts-nocheck|eslint-disable|biome-ignore|prettier-ignore)/; |
There was a problem hiding this comment.
The suppression pattern omits @ts-expect-error, even though it suppresses TypeScript diagnostics and is already used in this repository. A PR can therefore introduce new @ts-expect-error directives without producing new_suppression or requiring suppression-approved; include this directive in the pattern and its regression cases.
Useful? React with 👍 / 👎.
| 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*\(/; |
There was a problem hiding this comment.
The pattern detects .only( and .skip( but not Bun's test.todo( or it.todo( forms. Consequently, a contributor can add a disabled placeholder test while the advertised focused_or_skipped_test check reports success. Add the todo form while retaining the intentional allowance for conditional platform skips such as skipIf.
Useful? React with 👍 / 👎.
| ".next/", | ||
| "node_modules/", | ||
| ]; | ||
| const BEHAVIOR_PREFIXES = ["src/", "gui/src/"]; |
There was a problem hiding this comment.
bin/ocx.mjs is the user-facing npm launcher and contains substantial update, service, and process-launch behavior, but it is outside both behavior prefixes. A PR can therefore change production launcher behavior without any test change or exception label. Include the runtime launcher path in behavior classification.
Useful? React with 👍 / 👎.
| const testsChanged = allPaths.some( | ||
| (path) => isTestPath(path) && !removedFilenames.has(path), | ||
| ); |
There was a problem hiding this comment.
testsChanged checks both sides of renames and accepts any extension matched by the broad test-name pattern. A source change can therefore pass by renaming tests/x.test.ts to a non-test location, or by adding a non-runnable file such as docs/note.test.md; neither adds regression coverage. Determine coverage from surviving current paths and restrict it to locations and extensions exercised by the repository's test commands.
AGENTS.md reference: AGENTS.md:L199-L201
Useful? React with 👍 / 👎.
| 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, | ||
| }); |
There was a problem hiding this comment.
GitHub's pull-request files endpoint returns at most 3,000 files, but pagination is used without comparing the result to the already-fetched pr.changed_files. For a larger PR, files beyond that cap are silently absent from every hygiene check, allowing a behavior or generated-output violation outside the returned subset to pass. Reject incomplete results or use another complete diff source when files.length !== pr.changed_files.
Useful? React with 👍 / 👎.
| function isGeneratedPath(path) { | ||
| return GENERATED_PREFIXES.some((prefix) => path.startsWith(prefix)); | ||
| } |
There was a problem hiding this comment.
Most generated prefixes are checked only at the repository root because startsWith("dist/"), startsWith("coverage/"), and startsWith("node_modules/") do not match nested paths. Generated content such as docs-site/dist/, gui/coverage/, or docs-site/node_modules/ therefore bypasses generated_output without an approval label. Match these names as path segments or enumerate every package's generated directories.
Useful? React with 👍 / 👎.
| pull_request_target: | ||
| types: [opened, reopened, synchronize, labeled, unlabeled] |
There was a problem hiding this comment.
The file-list verdict depends on the PR base, but the trigger omits the edited activity. When a stacked child is retargeted after its parent lands, GitHub emits pull_request_target: edited; if the head SHA is unchanged, this workflow does not run, leaving the old green result and any exception labels in place even though the effective diff changed. Add edited and reassess the fresh PR state.
AGENTS.md reference: AGENTS.md:L158-L161
Useful? React with 👍 / 👎.
Summary
Extraction of @Wibias's deterministic PR hygiene gate (#903) onto
dev, withauthorship preserved on all three original commits, plus one tuning commit.
#903 sits fourth in a five-PR stack (
#900 → #901 → #902 → #903 → #905), but itsfiles are self-contained —
.github/scripts/pr-hygiene.cjs, its test, itsworkflow, and its spec touch nothing the other four add. The rest of that stack
is held on questions that have nothing to do with this gate (see the review
comments on #900, #901, #902, #905), and it should not wait behind them.
Why this one first
Every check here is deterministic: a test-change requirement for behavior paths,
new suppression comments, focused/skipped tests, empty catches, generated output,
and orphaned lockfiles. A contributor who trips one can read the failure and know
exactly what to change. That is the property I was unwilling to trade away in
#901, where CodeRabbit is configured to request changes on "technically credible"
reasons and a readiness reconciler then auto-drafts the PR.
The tuning commit
The gate asked for a regression test whenever a path under a behavior prefix
appeared in the diff. This repository asks for dense explanatory comments in
exactly those files, so a PR that only sharpens one would fail — and the only
escape was a maintainer applying
test-exception-approved. That trainscontributors to ask for the label instead of writing tests, which weakens the
gate everywhere it actually matters.
A file whose patch contains only comment or blank lines, added and removed sides
both, no longer counts as a behavior change. One non-comment line anywhere in
that file's patch makes it behavior again, so code cannot hide behind a comment.
Two other worries I raised in review turned out to be unfounded, and I checked
before changing anything:
test.skipIf(...)flagged as a focused test.only(/.skip(onlycatch { /* best-effort */ }flagged as emptyVerification
node --test .github/scripts/pr-hygiene.test.cjs: 19 pass, 0 fail (17 fromstack 4/5: add deterministic anti-slop CI #903, 2 added here)
src-without-test still fails, comment-only passes, comment-plus-one-code-line
still fails
Attribution
Commits 1–3 are @Wibias's, unmodified, from
agent/pr-hygiene-gate. #903 can beclosed once this lands, or kept open if he would rather land it himself from the
restacked branch — his call, and I have said so on the PR.
Summary by CodeRabbit
New Features
Documentation