Skip to content

ci: check this pull request with the commit-check in it - #541

Merged
shenxianpeng merged 7 commits into
mainfrom
claude/self-check-from-source-602anc
Aug 9, 2026
Merged

ci: check this pull request with the commit-check in it#541
shenxianpeng merged 7 commits into
mainfrom
claude/self-check-from-source-602anc

Conversation

@shenxianpeng

@shenxianpeng shenxianpeng commented Aug 9, 2026

Copy link
Copy Markdown
Member

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):

  naive 'commit-check --message' : exit=0   ← green, validated nothing
  this workflow                  : exit=1   ← caught it

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:

Scenario Expected Got
two good commits, good title 0 0
second commit in past tense 1 1
commits fine, title in past tense 1 1
HEAD not a merge commit error, 1 error, 1
naive form on the same bad repo 0 (the trap)
multi-line message whose body says "updated"/"fixed" 0 0

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

  • Chores
    • Updated pull request validation checks to run with reduced permissions and safer title handling.
    • Improved validation of pull request commits, titles, branches, authors, and merge structure.
    • Added clearer reporting when multiple validation checks fail.
    • Validation now detects unavailable commit information instead of incorrectly reporting success.
    • Standardized the validation environment and execution process for more consistent results.

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.
@shenxianpeng
shenxianpeng requested a review from a team as a code owner August 9, 2026 05:49
@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 1332379e-b0ed-4a21-8222-c05c763452b0

📥 Commits

Reviewing files that changed from the base of the PR and between b95acfd and 2ca7070.

📒 Files selected for processing (1)
  • .github/workflows/commit-check.yml
📝 Walkthrough

Walkthrough

The 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.

Changes

Pull request validation

Layer / File(s) Summary
Workflow validation flow
.github/workflows/commit-check.yml
The workflow uses read-only pull request permissions, installs the checkout with pinned Python setup, and invokes nox -s commit-check with PR_TITLE.
Commit enumeration and checks
noxfile.py
The Nox session enumerates pull request commits from synthetic merge commits, validates commit messages and metadata, runs branch and author checks, aggregates failures, and rejects unavailable commit enumeration in pull request CI.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

Suggested labels: chore

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: running commit-check for this pull request in CI.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/self-check-from-source-602anc

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Comment thread .github/workflows/commit-check.yml Fixed
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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between a90e8c8 and c2e8aaf.

📒 Files selected for processing (1)
  • .github/workflows/commit-check.yml

Comment thread .github/workflows/commit-check.yml Outdated
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.
Comment thread .github/workflows/commit-check.yml Fixed
Comment thread .github/workflows/commit-check.yml Fixed
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.
Comment thread .github/workflows/commit-check.yml Fixed
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

codecov Bot commented Aug 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.00%. Comparing base (a90e8c8) to head (b95acfd).

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between c2e8aaf and b95acfd.

📒 Files selected for processing (2)
  • .github/workflows/commit-check.yml
  • noxfile.py

Comment thread noxfile.py Outdated
text=True,
check=True,
).stdout
return [message for message in log.split("\0") if message.strip()]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Comment thread .github/workflows/commit-check.yml Fixed
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.
@shenxianpeng
shenxianpeng force-pushed the claude/self-check-from-source-602anc branch from fb82347 to 0d37026 Compare August 9, 2026 15:21
Comment thread .github/workflows/commit-check.yml Dismissed
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.
@sonarqubecloud

sonarqubecloud Bot commented Aug 9, 2026

Copy link
Copy Markdown

Quality Gate Passed Quality Gate passed

Issues
0 New issues
1 Accepted issue

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@shenxianpeng
shenxianpeng merged commit f8418e2 into main Aug 9, 2026
9 checks passed
@shenxianpeng
shenxianpeng deleted the claude/self-check-from-source-602anc branch August 9, 2026 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants