ci: check this pull request with the commit-check in it - #541
Conversation
The workflow ran commit-check-action, which installs a released commit-check -- so every pull request was checked by the version before it. #540 had its own title rejected by the bug it was fixing, and the same thing happened tonight in the .github repository. A self-test that cannot see the change under test is not a self-test, and the action's version does not track the engine's, so a fix needed two releases and a bump before it could be reproduced here. Install from the checkout instead. The cost is job-summary and pr-comments, which are the action's rendering rather than the engine's behaviour and are covered by the action's own repository. Enumerating the commits is not optional. On a pull_request checkout HEAD is the synthetic merge commit, which the engine skips, so a bare 'commit-check --message' reports a pass having read nothing. Verified on a scratch repository holding one plainly bad commit: the bare form exits 0, this workflow exits 1. HEAD^1..HEAD^2 gives the real commits, and a guard fails loudly if HEAD is not a merge so the failure can never be silence. The pull request title reaches the CLI through the environment. Titles are attacker-controlled, and interpolating one into a run: block is a script injection.
|
Warning Review limit reached
Next review available in: 48 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe workflow removes pull request write access and replaces the pinned action with local installation and explicit validation. The Nox session checks pull request commits, metadata, branch, author, and merge structure, then reports combined failures. ChangesPull request validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
SonarCloud's unpinned-install rule failed the quality gate on a bare 'pip install .'. main.yml and publish-package.yml both carry --only-binary :all: for exactly this, added in #479; this step was the one that did not. Verified it still installs a local path: the resulting version reads 2.13.4.post1.dev4, which is the checkout rather than the release.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/workflows/commit-check.yml:
- Around line 82-85: Update the author-validation step around commit-check so it
validates every commit in the pull request rather than only the synthetic merge
commit at HEAD. Iterate over the HEAD^1..HEAD^2 commits and provide each
commit’s author to commit-check, or use an explicit revision input while
preserving the existing branch and author-name/email checks.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 911ae1e9-8196-4cba-bd70-9e949a95653d
📒 Files selected for processing (1)
.github/workflows/commit-check.yml
With stdin left open, commit-check waits to read a message even when only --branch and --author-name were asked for. Reproduced locally: the process runs until killed, which in CI is a step that hangs rather than fails. Closing stdin also decides what the author checks look at, so the comment says which: the author of HEAD, matching what the action reports today.
I added --only-binary :all: because main.yml and publish-package.yml carry it and #479 mentions SonarCloud. It does not satisfy this rule -- the alert stayed -- and --upgrade pip added a second flagged line, so the change took the count from one to two. Back to one plain line. The remaining alert needs a decision rather than another flag: main.yml has the identical install and passes only because it is not new code, so locking would mean a hash-pinned requirements file or reviving the stale uv.lock.
The session was already there and already wrong: it ran a bare 'commit-check --message', which on a pull_request checkout inspects the merge commit, which the engine skips. Leaving it that way while putting a correct copy in YAML would have left two implementations, with the broken one being the one a contributor reaches for locally. So the logic lives in noxfile.py and the workflow is one line. The same command now reproduces a CI failure on a laptop, which was half the reason for moving off the action. The session adapts rather than assuming CI: HEAD^1..HEAD^2 when the checkout is a merge commit, HEAD otherwise, and the title only when PR_TITLE is set. It still refuses to pass silently -- missing HEAD^2 during a pull_request event is an error, not a fallback. Verified locally: enumerates 2 of 2 commits on a merge ref and 0 on a plain one, exits 1 on a bad title having still run the branch and author checks, exits 0 on a good one, and no longer hangs on stdin.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #541 +/- ##
=======================================
Coverage 98.00% 98.00%
=======================================
Files 12 12
Lines 1300 1300
=======================================
Hits 1274 1274
Misses 26 26 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@noxfile.py`:
- Line 62: Update the commit-log parsing function to preserve every
NUL-delimited record, including empty and whitespace-only messages, instead of
filtering with message.strip(). Return a distinct None sentinel when the log
does not represent a merge commit, and update the caller around the HEAD
validation flow to test that sentinel explicitly rather than relying on list
truthiness.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f98a1e4d-03df-49cf-8a2c-20247c445dc0
📒 Files selected for processing (2)
.github/workflows/commit-check.ymlnoxfile.py
| text=True, | ||
| check=True, | ||
| ).stdout | ||
| return [message for message in log.split("\0") if message.strip()] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve empty commit messages.
At Line 62, message.strip() removes empty and whitespace-only commit messages. Git permits these messages. For such a pull request, Lines 93-99 report that HEAD is not a merge commit, and the commit is not validated.
Use a distinct non-merge sentinel, such as None. Preserve each NUL-delimited log record, including empty records. Update the caller to test the sentinel instead of list truthiness.
🤖 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 `@noxfile.py` at line 62, Update the commit-log parsing function to preserve
every NUL-delimited record, including empty and whitespace-only messages,
instead of filtering with message.strip(). Return a distinct None sentinel when
the log does not represent a merge commit, and update the caller around the HEAD
validation flow to test that sentinel explicitly rather than relying on list
truthiness.
Reverts b95acfd, which moved the pull request checks into the nox session. The session is a developer command -- run commit-check on your working copy -- and folding CI's shape into it made the simple thing complicated for no gain. The justification was wrong too. I claimed the same command would reproduce a CI failure locally, then had to branch on whether HEAD^2 exists, because locally there is no merge commit and no PR title. A command that behaves differently in the two places does not reproduce one from the other. It also introduced a bug the shell never had: filtering on .strip() dropped empty messages, which this repository rejects via allow_empty_commits = false, so such a commit would have been reported as 'HEAD is not a merge commit' instead of as the thing it is. The shell keeps them: printf 'a\0\0b\0' through 'read -r -d' yields a, empty, b. noxfile.py is now byte-identical to main.
fb82347 to
0d37026
Compare
main is linear and every subject ends in (#N): this repository squashes, so the commits on a branch never reach it. The title becomes the subject. Checking each commit was protecting history that does not exist, and cost thirty lines of shell to do it. What is left is two commands. The bare 'commit-check --message' still cannot be one of them -- it reads HEAD, the synthetic merge commit, which the engine skips and would pass having read nothing -- so the title goes in through stdin instead, which sidesteps HEAD entirely and needs no enumeration. Contributors lose CI feedback on intermediate commit messages. The pre-commit hook is where that belongs anyway: it arrives while the message is being written rather than a round trip later. Verified on a merge ref: bad title 1, good title 0, good title with a bad branch name 1.
|

Reverts the engine half of #531:
commit-check.ymlgoes back to the CLI, installed from the checkout. The file itself stays — the reasoning in #531 about running on pull requests only, with nopaths:filter, was right and is kept verbatim.Why
The self-test could not see the change under test.
commit-check-actioninstalls a releasedcommit-check, so every pull request here was checked by the version before it. Two demonstrations from this week:cut, and advised changing a past tense that had not been written.The version lag is two hops, not one. The action's version does not track the engine's: action v2.13.0 shipped
commit-check==2.13.1, and v2.13.1 shipped2.13.4. Reproducing an engine bug here required an engine release, an action release, and a bump — by which point the bug is a week old.The part that is not optional
A bare
commit-check --messagewould have made this worse, not better. On apull_requestcheckoutHEADis the synthetic merge commit, and the engine skips merge commits — so the check reports a pass having read nothing.Verified on a scratch repository containing exactly one plainly bad commit (
updated the parser):So the workflow enumerates
HEAD^1..HEAD^2— base tip to branch tip — and pipes each message in, which is what the action does internally (main.py:243). A guard fails loudly whenHEADis not a merge commit, so this can never degrade back into silence.Security
The pull request title reaches the CLI through
env:, never interpolated into therun:block. A title is attacker-controlled text and inlining one is a script injection. Checked: no${{ }}appears inside anyrun:.permissionsdrops frompull-requests: writetocontents: read, since nothing writes back any more.What is lost
job-summaryandpr-comments. Those are the action's rendering, not the engine's behaviour, and they are covered by tests in the action's own repository. Failures still appear in the step log, grouped per scope. My view is that a pretty comment from a stale engine is worth less than a plain log from the right one — but it is a real trade, and easy to reverse if you disagree.Verified
Six scenarios on scratch repositories built with a real merge ref (
HEAD^1/HEAD^2), not simulated:HEADnot a merge commitThe last one confirms the NUL-delimited read keeps bodies intact and that only subjects are judged.
🤖 Generated with Claude Code
https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn
Generated by Claude Code
Summary by CodeRabbit