ci: run commit-check on every PR, and check the title as it will land - #531
ci: run commit-check on every PR, and check the title as it will land#531shenxianpeng merged 2 commits into
Conversation
Sorry, something went wrong.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting. Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR adds a dedicated pull-request commit-check workflow, removes commit-check execution from the main build, and updates README examples with rule IDs, documentation URLs, expanded results, and revised guidance. ChangesCommit check CI
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubPullRequest
participant CommitCheckWorkflow
participant CommitCheckAction
GitHubPullRequest->>CommitCheckWorkflow: trigger on selected pull-request events
CommitCheckWorkflow->>CommitCheckAction: run commit and pull-request checks
Possibly related PRs
Suggested labels: enhancement, documentation Suggested reviewers: sahilsaiyed-oss 🚥 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.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 24-26: Add persist-credentials: false to the with configuration of the actions/checkout step in the commit-check workflow, while retaining the existing full-history fetch-depth setting. - Line 30: Update the workflow step that installs Nox to use the approved pinned version from the development dependency configuration, or install the locked package with its hash pins instead of resolving the latest release.
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: ce09c2f1-0e46-428c-a791-931803f006f1
📥 CommitsReviewing files that changed from the base of the PR and between e2edc16 and d52a191.
📒 Files selected for processing (3)- .github/workflows/commit-check.yml
- .github/workflows/main.yml
- noxfile.py
- .github/workflows/main.yml
Sorry, something went wrong.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #531 +/- ##
=======================================
Coverage 97.97% 97.97%
=======================================
Files 12 12
Lines 1281 1281
=======================================
Hits 1255 1255
Misses 26 26 ☔ View full report in Codecov by Harness.
|
Sorry, something went wrong.
|
❌ The last analysis has failed. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1).github/workflows/commit-check.yml (1)🤖 Prompt for all review comments with AI agents31-33: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Pin the interpreter for this required check.
python-version: '3.x' installs the latest stable Python 3 release, so this required PR gate can start failing after an interpreter release. Pin the Python version explicitly unless the job is intended to test the rolling latest Python 3.
🤖 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 @.github/workflows/commit-check.yml around lines 31 - 33, Update the Python setup step using actions/setup-python to replace the rolling python-version value '3.x' with an explicit supported Python version, keeping the required check deterministic.
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: - Line 38: Pin the bootstrap pip installation in the workflow command to an exact approved version, or install it through hash-locked constraints before installing the development dependencies; replace the unbounded upgrade in the pip bootstrap step while preserving the existing binary-only installation behavior. --- Nitpick comments: In @.github/workflows/commit-check.yml: - Around line 31-33: Update the Python setup step using actions/setup-python to replace the rolling python-version value '3.x' with an explicit supported Python version, keeping the required check deterministic.
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: 354b977e-a734-4cf9-868a-6fb560ec8b41
📥 CommitsReviewing files that changed from the base of the PR and between d52a191 and a607bb4.
📒 Files selected for processing (3)- .github/workflows/commit-check.yml
- commit_check/engine.py
- tests/engine_test.py
Sorry, something went wrong.
Merging this PR will regress 1 benchmark⚡ 2 improved benchmarks Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent. Comparing claude/refresh-sample-output-602anc (93bf2ef) with main (7315edf)2 Footnotes
|
Sorry, something went wrong.
Commit Check✅ All 6 checks passed Show all 6 checksCommit message
✔ PR title (ci: run commit-check on every PR, and check the title as ...)
✔ Commit 1/2 (ci: run commit-check on every PR, and check the title as ...)
✔ Commit 2/2 (ci: bump commit-check-action to v2.13.1, which clears CC202)
Branch
✔ Branch (claude/refresh-sample-output-602anc)
Author
✔ Author name (Xianpeng Shen)
✔ Author email (xianpeng.shen@gmail.com)
commit-check 2.13.4 · Rules reference |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)tests/workflow_test.py (1)🤖 Prompt for all review comments with AI agents10-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Include .yaml files in WORKFLOWS.
WORKFLOWS only collects *.yml files. A .yaml workflow will bypass test_every_workflow_parses, despite the test scope claiming every workflow file.
Proposed fix🤖 Prompt for AI Agents-WORKFLOWS = sorted((REPO_ROOT / ".github" / "workflows").glob("*.yml")) +WORKFLOWS = sorted( + path + for pattern in ("*.yml", "*.yaml") + for path in (REPO_ROOT / ".github" / "workflows").glob(pattern) +)Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/workflow_test.py` at line 10, Update the WORKFLOWS collection in tests/workflow_test.py to include both .yml and .yaml files from .github/workflows, ensuring test_every_workflow_parses covers every supported workflow extension while preserving the existing sorted ordering.
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 `@tests/engine_test.py`: - Around line 1085-1094: Update test_merge_base_validator_valid so ValidationRule(check="merge_base") includes a target regex that causes MergeBaseValidator.validate to execute its merge-base lookup. Configure or mock the target branch resolution as needed, then assert that the patched commit_check.engine.git_merge_base is called with the expected arguments while preserving the valid PASS assertion. --- Nitpick comments: In `@tests/workflow_test.py`: - Line 10: Update the WORKFLOWS collection in tests/workflow_test.py to include both .yml and .yaml files from .github/workflows, ensuring test_every_workflow_parses covers every supported workflow extension while preserving the existing sorted ordering.
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: b4a178c3-5641-4205-b33e-44122c0b7c75
📥 CommitsReviewing files that changed from the base of the PR and between a607bb4 and 46bc539.
📒 Files selected for processing (4)- .github/workflows/commit-check.yml
- commit_check/engine.py
- tests/engine_test.py
- tests/workflow_test.py
Sorry, something went wrong.
#530 passed review with a 75-character title and failed commit-check on main at 82, against a limit of 80 — the squash merge appended " (#530)" and pushed the subject over. Nothing had run on the pull request itself: main.yml's paths filter skipped the check because #530 touched only assets/, and the nox session checked HEAD, never the title a squash merge would actually commit. So: a dedicated workflow running commit-check-action on pull requests only, and the push-to-main check removed instead of taught to cope. Pull requests only, deliberately. A push to main carries work that already passed on its pull request, re-checked against a subject the author never wrote and cannot shorten. Dropping that removes the failure mode instead of predicting it. No paths filter — a subject, a branch name or an author address is wrong regardless of which files changed. The `edited` trigger re-checks when a title changes, because the title is what a squash merge commits; pr-title on the action checks it directly. The action rather than the nox session is also this repository checking itself with the thing users actually run. Known: CC202 fails on pull-request checkouts until the merge-base fix in commit-check ships and the action picks it up — the released engine cannot resolve a target that exists only as origin/main. That fix is a separate pull request; this one stays CI-only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn
The pin was v2.13.0, whose requirements.txt installs commit-check 2.13.1. That engine cannot resolve a rebase target existing only as origin/main in a pull request checkout, so it reported "not rebased" for a branch that was -- the false CC202 this workflow has carried since its first run. v2.13.1 of the action installs commit-check 2.13.4, which carries the merge-base fix from #532. Confirmed by reading requirements.txt at the tag rather than assuming the action version tracks the engine version: v2.13.0 (124de73) -> commit-check==2.13.1 v2.13.1 (562a184) -> commit-check==2.13.4 The branch was already rebased before this change -- git merge-base --is-ancestor origin/main HEAD returned true while CC202 still failed -- so the failure was the engine, not the branch, and rebasing again could never have fixed it. Two things ride along on the newer engine. Skipped checks now report as skipped rather than as passes (#537), so a run bypassed by ignore_authors says so instead of showing green ticks over nothing. And the imperative whitelist goes from 396 verbs to 529, retiring a class of false CC003 -- 2.13.1 rejected "treat", which #527 had added three releases earlier. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn
|
Sorry, something went wrong.
What
Two workflow files, plus the action pin that made this PR green.
Why
#530 passed review with a 75-character title and failed commit-check on main at 82/80 — the squash merge appended (#530) and pushed the subject over the limit. Nothing had run on the PR itself:
Checking only pull requests removes the failure mode instead of predicting it: the title is checked once, where it can still be edited, and edited re-checks on retitle. This is also the project checking itself with the thing users actually run.
CC202 — resolved
This PR carried a false CC202 merge-base failure from its first run. The engine the pinned action installed could not resolve a rebase target that exists only as origin/main in a PR checkout, so it reported "not rebased" for a branch that was.
That it was the engine and not the branch is now established rather than argued: the branch was rebased onto main while CC202 still failed, with git merge-base --is-ancestor origin/main HEAD returning true throughout.
The fix was the pin. The action's version does not track the engine's, which is the part worth reading requirements.txt for rather than inferring:
A patch bump on the action jumps the engine three releases. commit-check is now green, and the check's own footer confirms which engine ran (commit-check 2.13.4).
Two behaviours ride along on the newer engine:
Worth knowing before merging: edited doubles the CI cost
Measured on this PR's own run history, duplicate runs land on every SHA:
edited fires on description edits, not just title edits, and CodeRabbit edits the description on every review to insert its summary — so each review costs an extra full run. edited is still needed (a retitle must re-check pr-title), but GitHub offers no title-only filter. Gating the job would keep the retitle re-check and drop the rest:
Not applied here — flagging it as a follow-up decision for the reviewer.
🤖 Generated with Claude Code
https://claude.ai/code/session_01U9zFxq8V4qxG4aMzJhGBFn