| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
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@shenxianpeng, you've reached your PR review limit, so we couldn't start this review. 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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR. 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: 1332379e-b0ed-4a21-8222-c05c763452b0 📥 CommitsReviewing files that changed from the base of the PR and between b95acfd and 2ca7070. 📒 Files selected for processing (1)
WalkthroughThe 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: chore 🚥 Pre-merge checks | ✅ 5 ✅ Passed checks (5 passed)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
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 agentsVerify 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.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 911ae1e9-8196-4cba-bd70-9e949a95653d
📥 CommitsReviewing files that changed from the base of the PR and between a90e8c8 and c2e8aaf.
📒 Files selected for processing (1)
Sorry, something went wrong.
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. @@ 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.
|
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agentsVerify 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.
Fix all unresolved CodeRabbit comments on this PR:
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f98a1e4d-03df-49cf-8a2c-20247c445dc0
📥 CommitsReviewing files that changed from the base of the PR and between c2e8aaf and b95acfd.
📒 Files selected for processing (2)
Sorry, something went wrong.
| 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 AgentsVerify 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.
Sorry, something went wrong.
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.
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.
|
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Reverts the engine half of #531: commit-check.yml goes back to the CLI, installed from the checkout. The file itself stays — the reasoning in #531 about running on pull requests only, with no paths: filter, was right and is kept verbatim.
Why
The self-test could not see the change under test. commit-check-action installs a released commit-check, so every pull request here was checked by the version before it. Two demonstrations from this week:
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 shipped 2.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 --message would have made this worse, not better. On a pull_request checkout HEAD is 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 when HEAD is 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 the run: block. A title is attacker-controlled text and inlining one is a script injection. Checked: no ${{ }} appears inside any run:.
permissions drops from pull-requests: write to contents: read, since nothing writes back any more.
What is lost
job-summary and pr-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:
The 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