claude: add slash command skill to lint commits in a PR#30162
claude: add slash command skill to lint commits in a PR#30162dotnwat wants to merge 1 commit intoredpanda-data:devfrom
Conversation
Add a /lint-pr slash command that checks pull request commits against project standards. It can be run with no arguments to lint the current branch against upstream/dev, or with a PR number to lint a remote PR. Checks implemented: - M1: subject line length (72 chars, 90 with long identifiers) - M2: body line wrapping (72 chars, exempt code blocks/URLs) - M3: blank line after subject - M4: subject format (area[/detail]: lowercase description) - M5: commit message adequately explains the change - M6: bug fix commits explain bug, fix, and rationale - M7: commit atomicity (one logical change per commit) - M8: commit message accuracy (message matches the diff) Signed-off-by: Noah Watkins <noahwatkins@gmail.com>
dotnwat
changed the title
There was a problem hiding this comment.
Pull request overview
Adds a new Claude skill (/lint-pr) that lints pull request commits against the repo’s commit message standards and (for some checks) commit diffs, supporting both local-branch and remote-PR workflows.
Changes:
- Introduces
.claude/skills/lint-pr/SKILL.mddefining the/lint-prskill and its execution steps. - Specifies commit message checks M1–M8, including formatting, adequacy, bug-fix explanation, atomicity, and message/diff consistency.
- Defines an expected output/report format for per-commit results and summary.
| - **Pass** if there is no body, or if line 2 is blank | ||
| - **Warn** if line 2 is non-blank and the message has more than one line |
There was a problem hiding this comment.
This section states there must be “exactly one blank line” between subject and body, but the PASS criteria only checks that line 2 is blank. That would incorrectly PASS messages with multiple blank lines before the body. Either relax the requirement to “at least one blank line” or update the check logic to WARN when there is more than one blank line separating subject and body.
| - **Pass** if there is no body, or if line 2 is blank | |
| - **Warn** if line 2 is non-blank and the message has more than one line | |
| - **Pass** if there is no body, or if line 2 is blank and line 3 is the | |
| first non-blank body line (that is, there is exactly one blank line | |
| between subject and body) | |
| - **Warn** if line 2 is non-blank and the message has more than one line | |
| - **Warn** if there is more than one consecutive blank line between the | |
| subject and the first non-blank body line |
| git log --format='%H %s' upstream/dev..HEAD | ||
| ``` |
There was a problem hiding this comment.
Local branch mode hard-codes upstream/dev..HEAD, but earlier you say {owner}/{repo} should fall back to origin when upstream is missing. If a repo doesn’t have an upstream remote (or doesn’t have upstream/dev fetched), this git log will fail. Consider deriving the base ref the same way here (prefer upstream/dev, else origin/dev, else dev), and emitting a clear error if none exist.
| **Diffstat:** | ||
|
|
||
| - Local branch mode: `git show --stat --format='' <sha>` | ||
| - Remote PR mode: `gh api repos/{owner}/{repo}/commits/<sha> --jq '.stats.total as $t | .files | map(.filename + " | " + (.changes | tostring) + " (+\(.additions)/-\(.deletions))") | .[] ' ` and `gh api repos/{owner}/{repo}/commits/<sha> --jq '.stats | "\(.total) lines changed (+\(.additions)/-\(.deletions))"'` |
There was a problem hiding this comment.
The remote diffstat command defines (.stats.total as $t | ...) but $t is never used, and the inline code span includes a trailing space before the closing backtick. This makes copy/paste error-prone and slightly confusing. Remove the unused $t binding and trim trailing whitespace in the command string.
| - Remote PR mode: `gh api repos/{owner}/{repo}/commits/<sha> --jq '.stats.total as $t | .files | map(.filename + " | " + (.changes | tostring) + " (+\(.additions)/-\(.deletions))") | .[] ' ` and `gh api repos/{owner}/{repo}/commits/<sha> --jq '.stats | "\(.total) lines changed (+\(.additions)/-\(.deletions))"'` | |
| - Remote PR mode: `gh api repos/{owner}/{repo}/commits/<sha> --jq '.files | map(.filename + " | " + (.changes | tostring) + " (+\(.additions)/-\(.deletions))") | .[]'` and `gh api repos/{owner}/{repo}/commits/<sha> --jq '.stats | "\(.total) lines changed (+\(.additions)/-\(.deletions))"'` |
| - Remote PR mode: `gh api repos/{owner}/{repo}/commits/<sha> --jq '.files[] | "\(.filename) \(.status)\n\(.patch)"'` | ||
|
|
There was a problem hiding this comment.
Remote PR mode says M7 “always requires the full diff”, but the suggested gh api .../commits/<sha> --jq ... .patch only provides GitHub’s truncated per-file patch (and .patch may be null for binaries/large diffs). That means M7 (and M8, which also needs the full diff) can be based on incomplete data. Consider fetching the real diff via gh api .../commits/<sha> -H 'Accept: application/vnd.github.v3.diff' (or by fetching the PR branch locally and using git show) and handle missing/binary diffs explicitly.
| - Remote PR mode: `gh api repos/{owner}/{repo}/commits/<sha> --jq '.files[] | "\(.filename) \(.status)\n\(.patch)"'` | |
| - Remote PR mode: `gh api repos/{owner}/{repo}/commits/<sha> -H 'Accept: application/vnd.github.v3.diff'` | |
| In remote PR mode, do not use `.files[].patch` from the JSON commit API | |
| as the source for M7 or M8: GitHub may truncate that per-file patch data | |
| for large diffs, and it may be absent for binary files. If the diff | |
| output indicates binary content or otherwise lacks textual hunks for some | |
| files, handle that explicitly in the lint result instead of silently | |
| treating the commit as fully checked. |
Add a /lint-pr slash command that checks pull request commits against project standards. It can be run with no arguments to lint the current branch against upstream/dev, or with a PR number to lint a remote PR.
Checks implemented:
I spot checked this with backtesting on old PR reviews.
Backports Required
Release Notes