ci: run commit-check on every PR, and check the title as it will land - #531
Conversation
|
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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe 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: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 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.
🪄 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: ce09c2f1-0e46-428c-a791-931803f006f1
📒 Files selected for processing (3)
.github/workflows/commit-check.yml.github/workflows/main.ymlnoxfile.py
💤 Files with no reviewable changes (1)
- .github/workflows/main.yml
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ 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. 🚀 New features to boost your workflow:
|
|
❌ The last analysis has failed. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/commit-check.yml (1)
31-33: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPin 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 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 @.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.
🤖 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:
- 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.
🪄 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: 354b977e-a734-4cf9-868a-6fb560ec8b41
📒 Files selected for processing (3)
.github/workflows/commit-check.ymlcommit_check/engine.pytests/engine_test.py
Merging this PR will regress 1 benchmark
Warning Please fix the performance issues or acknowledge them on CodSpeed. Performance Changes
Tip Investigate this regression by commenting Comparing 
Footnotes
|
Commit Check✅ All 6 checks passed Show all 6 checkscommit-check 2.13.4 · Rules reference |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/workflow_test.py (1)
10-10: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude
.yamlfiles inWORKFLOWS.
WORKFLOWSonly collects*.ymlfiles. A.yamlworkflow will bypasstest_every_workflow_parses, despite the test scope claiming every workflow file.Proposed fix
-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) +)🤖 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 `@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.
🤖 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 `@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.
🪄 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: b4a178c3-5641-4205-b33e-44122c0b7c75
📒 Files selected for processing (4)
.github/workflows/commit-check.ymlcommit_check/engine.pytests/engine_test.pytests/workflow_test.py
93bf2ef to
e481ae8
Compare
02bd378 to
5f22f7c
Compare
#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
5f22f7c to
86722d9
Compare
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
|
2 participants
What
Two workflow files, plus the action pin that made this PR green.
.github/workflows/commit-check.yml: runs commit-check-action on pull requests only (opened, synchronize, reopened, edited), withpr-title: trueso the title is checked as the subject a squash merge will actually commit. Nopaths:filter. Action SHA-pinned like every other action in this repo.Run commit-checkstep frommain.yml: the push-to-main check is gone entirely.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:main.yml'spaths:filter skipped the check — docs: refresh the demo recording, and fix a tape that recorded a false pass #530 touched onlyassets/.HEAD, never the title a squash merge would commit.Checking only pull requests removes the failure mode instead of predicting it: the title is checked once, where it can still be edited, and
editedre-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/mainin 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
mainwhile CC202 still failed, withgit merge-base --is-ancestor origin/main HEADreturning true throughout.The fix was the pin. The action's version does not track the engine's, which is the part worth reading
requirements.txtfor rather than inferring:124de73commit-check==2.13.1— the buggy engine562a184commit-check==2.13.4— carries #532's fixA patch bump on the action jumps the engine three releases.
commit-checkis 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:
ignore_authorssays so instead of showing green ticks over nothing validated.treat, which chore: expand imperative verb whitelist and improve validation logic #527 had added three releases earlier.Worth knowing before merging:
editeddoubles the CI costMeasured on this PR's own run history, duplicate runs land on every SHA:
commit-checkruns93bf2efe481ae802bd378editedfires 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.editedis still needed (a retitle must re-checkpr-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