stack 4/5: add deterministic anti-slop CI - #903
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Wibias
added
the
chore
Stack navigation
Review and merge bottom-up. Each PR targets the preceding stack branch, so its Files changed view contains only that layer. |
[shipping-github] Verdict: approve-commentPR: Semantic propagation
UsefulnessHigh: deterministic, auditable anti-slop rules with narrow maintainer-approved exceptions and no AI-origin guessing. The Bugs / correctness
Security
Spec / standards
Reviews
Base / CI
GateNone: draft cleared, PR is ready for review. Simplification (approved S1 - withdrawn)
Bottom lineSubstantively clean and approve-worthy after the fix batch: the rename bypass is closed, permissions are job-scoped, and the branch is current with its parent. Merge still requires maintainer approval (requested) and the synthetic fork test. When #905 is reviewed it will need the same base update - it is still based on the old |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0cbaf4baab
ℹ️ 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; every mutation uses the issues API for labels and comments. Granting pull-requests: write therefore gives this privileged pull_request_target job unnecessary PR-mutation capability. Set this scope to read while retaining issues: write.
AGENTS.md reference: .github/AGENTS.md:L12-L17
Useful? React with 👍 / 👎.
| 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))) { |
There was a problem hiding this comment.
These checks search every added line as raw text, so string literals, regex definitions, and documentation are treated as executable suppressions or focused tests. Feeding this commit's own patches to assessHygiene reports new_suppression, focused_or_skipped_test, and empty_catch because .github/scripts/pr-hygiene.test.cjs contains those constructs as fixture strings. Restrict checks to applicable source syntax or otherwise distinguish literal fixture/prose text.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[shipping-github] Declined with rationale: raw-text scanning is the deliberate design (documented in the design record), and the four exception labels are the escape hatch for legitimate fixtures/prose that must mention these tokens. A source parser would be disproportionate for a gate whose failure mode is a maintainer reviewing the flagged line.
| .filter((line) => line.startsWith("+") && !line.startsWith("+++")) | ||
| .map((line) => line.slice(1)); |
There was a problem hiding this comment.
Because addedLines discards every deletion and context line, deleting the final statement from an existing catch leaves an empty catch in the PR result but supplies no added braces for hasEmptyCatch, so the supposedly non-bypassable check passes. Conversely, changing only the catch signature and closing brace can make an unchanged nonempty body look empty. Inspect the resulting file or preserve enough hunk context to determine the actual catch body.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[shipping-github] Fixed in 7a6982d0: when a hunk deletes lines, the empty-catch scan now includes hunk context (added + context lines) instead of additions only, so deleting a catch body leaves the empty structure visible and is rejected. A nonempty body in a deletion hunk is not flagged. Unit-tested in both directions.
| const FOCUSED_TEST_PATTERN = /\b(?:describe|it|test)\.(?:only|skip)\s*\(/; | ||
|
|
||
| function addedLines(patch) { | ||
| if (typeof patch !== "string") return []; |
There was a problem hiding this comment.
The pull-files API does not guarantee a patch string for every file, notably for binary or oversized/truncated diffs. Returning an empty line set in that case silently bypasses suppression, focused-test, and empty-catch checks for the affected file. Since this workflow treats untrusted PR metadata as an enforcement boundary, it should retrieve complete content or fail explicitly when an applicable text file has no inspectable patch.
AGENTS.md reference: .github/AGENTS.md:L15-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[shipping-github] Declined as a residual: binary files have no executable suppressions to scan, and truncated patches are an inherent GitHub API limit already noted in the review. Text-file content fallback would add a content fetch per file for marginal coverage; the exception labels cover the remaining judgment cases.
| failures.push({ code: "missing_regression_test" }); | ||
| } | ||
|
|
||
| const generated = filenames.filter(isGeneratedPath); |
There was a problem hiding this comment.
This classifies files solely by path, so a PR deleting dist/, gui/dist/, coverage/, or another forbidden generated directory is rejected as generated_output and told to remove output it is already removing. Filter out files whose API status is removed before reporting committed generated output.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[shipping-github] Fixed in 7a6982d0: files with status: "removed" are excluded from the generated-output check, so a PR deleting dist/ or gui/dist/ output is no longer told to remove output it is already removing. Unit-tested.
| const filenames = files.map((file) => file.filename); | ||
| const behaviorChanged = filenames.some(isBehaviorPath); |
There was a problem hiding this comment.
For renamed files, GitHub reports the destination in filename and the source in previous_filename. A PR that moves a runtime file from src/ or gui/src/ elsewhere therefore removes behavior from those trees while behaviorChanged remains false, bypassing the regression-test requirement. Include previous_filename when classifying renamed files.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[shipping-github] Already fixed in 9f4b7e8b: behavior/test/generated classification now includes previous_filename, so moving a runtime file out of src/ or gui/src/ still counts as a behavior change and requires regression coverage. Regression tests cover renames into docs paths.
| behaviorChanged && | ||
| !testsChanged && | ||
| !labelSet.has("test-exception-approved") |
There was a problem hiding this comment.
Once test-exception-approved is applied, later synchronize events retain it and this validator continues bypassing both the regression-test and skipped-test checks for the new head. A contributor can therefore obtain approval for one narrow exception, push additional unreviewed violations, and still receive a passing hygiene result. Bind approvals to the reviewed head SHA or clear exception labels whenever the PR head changes.
AGENTS.md reference: .github/AGENTS.md:L15-L17
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[shipping-github] Fixed in 7a6982d0: on synchronize events the workflow revokes all four exception labels before assessment, so approvals are head-specific and a contributor cannot push unreviewed violations under a stale exception. Harness-verified (synchronize revokes + blocks; labeled keeps + passes).
| }); | ||
| }); | ||
|
|
||
| describe("assessHygiene", () => { |
There was a problem hiding this comment.
This new test suite is not invoked by any GitHub workflow: .github/workflows/issue-quality-tests.yml neither includes the hygiene files in its path filters nor runs this test, and Cross-platform CI does not trigger for these new paths either. Consequently the privileged gate can regress while repository CI remains green. Add the hygiene script, test, and workflow paths to the automation-test workflow and execute node --test .github/scripts/pr-hygiene.test.cjs there.
Useful? React with 👍 / 👎.
| 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`.", |
There was a problem hiding this comment.
This mandatory gate introduces contributor-visible rejection rules and four approval labels, but the commit updates only an internal design note; docs-site/src/content/docs/contributing.md still describes the existing CI and PR requirements without any of these rules or the exception process. Contributors will learn the contract only after a bot failure, so add the hygiene requirements to the public contributing documentation and keep localized guidance consistent.
AGENTS.md reference: AGENTS.md:L200-L201
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
| const behaviorChanged = filenames.some(isBehaviorPath); | ||
| const testsChanged = filenames.some(isTestPath); |
There was a problem hiding this comment.
testsChanged only checks the filename, so a runtime PR that deletes an unrelated test—or renames one away—satisfies the regression-test gate despite adding no coverage at all. Use the file status and resulting test paths to exclude removals, at minimum, before treating a test-file change as evidence of focused regression coverage.
AGENTS.md reference: AGENTS.md:L197-L199
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[shipping-github] Fixed in 7a6982d0: test files with status: "removed" no longer count toward testsChanged, so a runtime PR that only deletes an unrelated test is still rejected as missing_regression_test. Unit-tested.
…hes, allow removals
|
This is the one in the stack I want, and I would like to take it first rather than last. Reason: every check in Two things before it lands:
Context for the ordering: I measured the lane before deciding anything. Windows was the last job to finish in 23 of 23 recent runs (median 17m41s vs ubuntu 5m58s), so #899 goes first as the actual bottleneck fix. This lands right behind it. |
|
Extracted this onto Two of the three concerns I raised above were wrong, and I checked before touching anything rather than asking you to:
The one real false positive: Your call on how to finish it: I can merge #918, or close it and you land the same thing from a branch you re-cut onto Separately on the stack: #905's head is not a descendant of this branch (merge base |
ci: add the deterministic PR hygiene gate (extracted from #903)
|
Landed as #918 — your three commits, authorship intact, merged to On top of them: a comment-only source change no longer owes a regression test (the label escape would have taught contributors to ask for the label instead of writing tests), the gate's own The two concerns I raised earlier were wrong and I checked before changing anything: Thanks — this was the one in the stack that reduces friction rather than adding it, which is why it went first. |
2 participants
Stack
4/5 — deterministic anti-slop CI
Base:
agent/pr-trust-lane(#902)Next: review lifecycle, ownership, closure policy, and repository-settings rollout
Summary
bun.lockchurn withoutpackage.jsonVerification
node --test .github/scripts/pr-hygiene.test.cjs— 9 passed, 0 failedWhy
These checks target concrete, auditable defects instead of attempting unreliable AI-origin detection.