OCPEDGE-2577: Add code-quality plugin with commit convention hooks#75
OCPEDGE-2577: Add code-quality plugin with commit convention hooks#75jeff-roche wants to merge 4 commits intoopenshift-eng:mainfrom
Conversation
Adds PreToolUse hook for conventional commit message validation, PostToolUse hook for AI attribution trailer checking, and SessionStart hook for Coderabbit config validation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jeff-roche The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds a new code-quality plugin with manifest and marketplace entry, registers PreToolUse/PostToolUse/SessionStart hooks, and introduces three Bash hook scripts to validate commit subjects, check AI attribution trailers, and verify Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 4
🧹 Nitpick comments (1)
plugins/code-quality/scripts/check-coderabbit-config.sh (1)
34-43: Tighten YAML key checks to avoid comment/substring false positives.Current
greppatterns can pass when keys appear only in comments or unrelated values.Suggested refactor
-if ! grep -q 'auto_review' "$CONFIG"; then +if ! grep -qE '^[[:space:]]*auto_review:' "$CONFIG"; then MISSING+=("auto_review") fi -if ! grep -qE 'path_filters|path_instructions' "$CONFIG"; then +if ! grep -qE '^[[:space:]]*(path_filters|path_instructions):' "$CONFIG"; then MISSING+=("path_filters or path_instructions") fi -if ! grep -q 'instructions' "$CONFIG"; then +if ! grep -qE '^[[:space:]]*instructions:' "$CONFIG"; then MISSING+=("instructions") fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/code-quality/scripts/check-coderabbit-config.sh` around lines 34 - 43, The current grep checks on CONFIG use substring matches and can be fooled by comments; update the three grep invocations to match YAML keys explicitly by looking for lines that start with optional whitespace followed by the key name and a colon (e.g., use patterns like '^[[:space:]]*auto_review[[:space:]]*:'), and for the combined check match either 'path_filters' or 'path_instructions' with the same anchored pattern; keep using the CONFIG variable and append to the MISSING array exactly as before when a key is not found.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@plugins/code-quality/README.md`:
- Around line 36-38: The fenced code block containing the command "/plugin
marketplace add openshift-eng/edge-tooling code-quality" is missing a language
identifier and triggers markdownlint MD040; update that fenced block in
plugins/code-quality/README.md to include a language tag (e.g., "bash") after
the opening backticks so the block reads as a bash code fence to satisfy the
linter.
In `@plugins/code-quality/scripts/check-attribution.sh`:
- Line 1: Change the script shebang from the current /bin/bash to the
repository-standard /usr/bin/bash per CONTRIBUTING.md; update the top of
plugins/code-quality/scripts/check-attribution.sh so the first line uses
#!/usr/bin/bash to ensure the correct shell is invoked.
In `@plugins/code-quality/scripts/check-coderabbit-config.sh`:
- Line 1: Replace the non-standard shebang in the script
check-coderabbit-config.sh: change the first line from #!/bin/bash to the
repository-standard interpreter path #!/usr/bin/bash so the script uses the
required shell as specified in CONTRIBUTING.md.
In `@plugins/code-quality/scripts/check-commit-message.sh`:
- Line 1: Update the shebang in the check-commit-message.sh script: replace the
current first line shebang (#!/bin/bash) with the repository standard shebang
(#!/usr/bin/bash) so it conforms to CONTRIBUTING.md; modify only the top-of-file
shebang in check-commit-message.sh and keep the rest of the script (including
set -euo pipefail and quoting) unchanged, preserving the file's executable
permission.
---
Nitpick comments:
In `@plugins/code-quality/scripts/check-coderabbit-config.sh`:
- Around line 34-43: The current grep checks on CONFIG use substring matches and
can be fooled by comments; update the three grep invocations to match YAML keys
explicitly by looking for lines that start with optional whitespace followed by
the key name and a colon (e.g., use patterns like
'^[[:space:]]*auto_review[[:space:]]*:'), and for the combined check match
either 'path_filters' or 'path_instructions' with the same anchored pattern;
keep using the CONFIG variable and append to the MISSING array exactly as before
when a key is not found.
🪄 Autofix (Beta)
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d031845d-20c3-4bdd-824f-c8e3c4ead8f2
📒 Files selected for processing (6)
plugins/code-quality/.claude-plugin/plugin.jsonplugins/code-quality/README.mdplugins/code-quality/hooks/hooks.jsonplugins/code-quality/scripts/check-attribution.shplugins/code-quality/scripts/check-coderabbit-config.shplugins/code-quality/scripts/check-commit-message.sh
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add bash language tag to fenced code block in README (MD040) - Use #!/usr/bin/bash shebang across all scripts - Anchor grep patterns to YAML key positions in coderabbit config check Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
/assign |
brandisher
left a comment
brandisher
left a comment
There was a problem hiding this comment.
I think I get the goal of this PR but the implementation needs refining if I've understood it correctly.
First thing I see is that the plugin is called code-quality but is geared more towards standardizing parts of the git workflow like commit message correctness. For that goal, ai-helpers actually has a good git base already that we should take inspiration from.
Second thing, the use case isn't a good fit for a plugin. If someone doesn't install it, they don't get the hooks. I think the idea is good, it just needs a different implementation approach. Is there any reason these can't be landed in .claude/hooks so just by virture of starting CC in the repo we're doing all the necessary checks?
Third thing, I'm not sure I understand the coderabbit check - that's distinctly separate from the git aspect and once the file exists in the repo, I'm not sure why we'd want to check for it on repeat. That's kind of a set it and forget it thing. What am I missing?
|
Adding a hold while the comment gets addressed. /hold |
openshift-ci
Bot
added
the
do-not-merge/hold
…ew process (#77) * docs: add architectural guidelines from PR #74 and #75 reviews Codify recurring review feedback into CONTRIBUTING.md so contributors can self-evaluate before submitting: naming accuracy, hooks vs plugins for mandatory checks, lifecycle placement, reuse, and test coverage for validation logic. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * docs: remove ai-helpers references from CONTRIBUTING.md Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Agreed, I meant to throw a hold on it so we could discuss approaches like this as a team but forgot to do the wip/hold
I actually disagree, I think it's a good usecase for a repo hook and a plugin and here's my reasoning:
That being said, I'm not married to any of these checks and happy to change them up
I actually think the code rabbit check is a better fit for #73 because it was meant to be part of the org-standardization toolkit (which is what #73 covers, will comment there about potentially moving it to a different repo (ai-helpers?)) |
brandisher
left a comment
brandisher
left a comment
There was a problem hiding this comment.
Okay I put some more specific comments to narrow the discussion a bit. The spirit of the PR is clear - standardize conventions. But, we need to be clear about what this PR is doing in practice.
- Trys to enforce that all commits have an attribution for AI.
- Trys to enforce that .coderabbit.yaml exists.
- Trys to enforce a commit message style.
We want to maintain quality within this repo which fits a git/claude hook perfectly.
We agree that we want to maintain quality. What this PR is doing is enforcing an opinionated style. See the individual comments for specifics.
It's also based around generic industry best practices so it can be applied to any repo we work on which fits it as a good plugin so that it can be cross-repo ✔️
I hear the drive, but generic practices lead to generic results. We want to generalize where it makes sense and is impactful, not just where its possible. I left a few specific comments around this as well - the tldr is that these things are generally done well enough that they're not worth enforcing. If we are going to try to enforce them, they need to be done at the right level:
- git related checks need to be
.gitresident so they're deterministic and work regardless of development model. The Claude hook you have kind of fits the case where you're creating commits with CC so it only needs to be checked there, but its repo-local so for our multi-repo workstreams this would get missed anyway. - Checking for optional specific files has little impact. In this particular case, we'd actually be fine with no
.coderabbit.yamlin our repo - it just got set up so we could start evolving it.
| "version": "1.0.0" | ||
| }, | ||
| { | ||
| "name": "code-quality", |
There was a problem hiding this comment.
This plugin is trying to enforce an opinionated style as opposed to code quality which is something objectively measurable. This needs to be renamed to reflect that.
| "name": "code-quality", | ||
| "description": "Code convention enforcement — commit messages, attribution, and Coderabbit configuration", |
There was a problem hiding this comment.
Same here about the name update.
| "type": "command", | ||
| "command": "${CLAUDE_PLUGIN_ROOT}/scripts/check-commit-message.sh", | ||
| "timeout": 2, | ||
| "statusMessage": "Validating commit message..." |
There was a problem hiding this comment.
Commit message checking needs to be deterministic to be valuable. The only way to do this as deterministically as possible is to have a pre-commit hook at the git level.
The Claude hooks also don't work in a multi-repo development model which is table stakes for many workstreams.
| @@ -0,0 +1,65 @@ | |||
| #!/usr/bin/bash | |||
There was a problem hiding this comment.
This should be dropped. This isn't worth the tokens to enforce. Especially in the case of openshift and openshift-eng there's a default configuration enabled globally so even without this file you still get coderabbit configuration.
| #!/usr/bin/bash | ||
| # PreToolUse(Bash) hook — validates conventional commits format on git commit commands | ||
| set -euo pipefail | ||
|
|
||
| if ! command -v jq &>/dev/null; then | ||
| exit 0 | ||
| fi | ||
|
|
||
| INPUT=$(cat) | ||
| COMMAND=$(echo "$INPUT" | jq -r '.toolInput.command // empty') | ||
|
|
||
| # Fast path: not a commit command | ||
| if [[ "$COMMAND" != *"git commit"* ]]; then | ||
| exit 0 | ||
| fi | ||
|
|
||
| # Extract commit message subject line | ||
| SUBJECT="" | ||
|
|
||
| # Heredoc pattern: git commit -m "$(cat <<'EOF' ... EOF )" | ||
| if [[ "$COMMAND" =~ cat\ \<\<[\']?EOF ]]; then | ||
| # Extract first non-empty line after the heredoc delimiter | ||
| SUBJECT=$(echo "$COMMAND" | sed -n '/<<.*EOF/,/^EOF/{/<<.*EOF/d;/^EOF/d;/^[[:space:]]*$/d;p;}' | head -1 | sed 's/^[[:space:]]*//') | ||
| # Standard -m pattern with double or single quotes | ||
| elif [[ "$COMMAND" =~ git\ commit.*-m\ \" ]]; then | ||
| SUBJECT=$(echo "$COMMAND" | sed -n 's/.*git commit[^"]*-m "\([^"]*\)".*/\1/p' | head -1) | ||
| elif [[ "$COMMAND" =~ git\ commit.*-m\ \' ]]; then | ||
| SUBJECT=$(echo "$COMMAND" | sed -n "s/.*git commit[^']*-m '\\([^']*\\)'.*/\\1/p" | head -1) | ||
| fi | ||
|
|
||
| # If we couldn't extract a message, fail open | ||
| if [ -z "$SUBJECT" ]; then | ||
| exit 0 | ||
| fi | ||
|
|
||
| # For multi-line messages, take only the first line (subject) | ||
| SUBJECT=$(echo "$SUBJECT" | head -1) | ||
|
|
||
| # Validate against conventional commits pattern | ||
| if echo "$SUBJECT" | grep -qE '^(feat|fix|docs|refactor|test|chore|build|ci|perf|style)(\(.+\))?: .+'; then | ||
| exit 0 | ||
| fi | ||
|
|
||
| # Invalid format | ||
| cat >&2 <<'MSG' | ||
| Commit message does not follow conventional commits format. | ||
| Expected: type(scope): description | ||
| Types: feat, fix, docs, refactor, test, chore, build, ci, perf, style | ||
| Examples: | ||
| feat(api): add health endpoint | ||
| fix(deploy): correct subnet CIDR validation | ||
| docs: update contributing guide | ||
| MSG | ||
| exit 1 |
There was a problem hiding this comment.
Commit message enforcement needs to happen at the git level to be deterministic. This hook would get skipped in a multi-repo setup.
I'd also say that this style is informal and not something we'd want to enforce firmly. The CONTRIBUTING.md has some words around this style so as long as CC does it best effort, that's all we need.
| #!/usr/bin/bash | ||
| # PostToolUse(Bash) hook — warns if AI attribution trailers are missing after commits | ||
| set -euo pipefail | ||
|
|
||
| if ! command -v jq &>/dev/null; then | ||
| exit 0 | ||
| fi | ||
|
|
||
| INPUT=$(cat) | ||
| COMMAND=$(echo "$INPUT" | jq -r '.toolInput.command // empty') | ||
|
|
||
| # Fast path: not a commit command | ||
| if [[ "$COMMAND" != *"git commit"* ]]; then | ||
| exit 0 | ||
| fi | ||
|
|
||
| # Get the last commit message | ||
| COMMIT_MSG=$(git log -1 --format=%B 2>/dev/null || true) | ||
|
|
||
| if [ -z "$COMMIT_MSG" ]; then | ||
| exit 0 | ||
| fi | ||
|
|
||
| # Check for attribution trailers (case-insensitive) | ||
| if echo "$COMMIT_MSG" | grep -qi 'Co-Authored-By:'; then | ||
| exit 0 | ||
| fi | ||
| if echo "$COMMIT_MSG" | grep -qi 'Assisted-by:'; then | ||
| exit 0 | ||
| fi | ||
| if echo "$COMMIT_MSG" | grep -qi 'Generated-by:'; then | ||
| exit 0 | ||
| fi | ||
|
|
||
| # No attribution found — emit advisory context | ||
| cat <<'EOF' | ||
| { | ||
| "hookSpecificOutput": { | ||
| "hookEventName": "PostToolUse", | ||
| "additionalContext": "AI ATTRIBUTION MISSING: The last commit has no AI attribution trailer. Consider amending with one of:\n Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>\n Assisted-by: Claude Code\n Generated-by: Claude Code" | ||
| } | ||
| } | ||
| EOF | ||
|
|
||
| exit 0 |
There was a problem hiding this comment.
Enforcing at this level is bound to create more noise than value. This is an opinionated style and it creates noise where commits are not AI generated which is still a valid case.
To be frank, the guideline around this is clear so I get the motivation but the impact is very low to the point that I wouldn't want to have enforcement of this.
There was a problem hiding this comment.
Completely fair! I think none of this is 100% required from the perspective of our repo. This one is an easy one to cut
|
I'm going to close this one, I don't think any of this is 100% required and if we want to revisit down the road, it's easy enough to recreate any of this. |
2 participants
Summary
code-qualityClaude Code plugin with automatic hooks for code conventionstype(scope): description)Test plan
echo '{"toolInput":{"command":"git commit -m \"bad\""}}' | bash plugins/code-quality/scripts/check-commit-message.shexits 1 with guidanceecho '{"toolInput":{"command":"git commit -m \"feat(api): add endpoint\""}}' | bash plugins/code-quality/scripts/check-commit-message.shexits 0echo '{"cwd":"/tmp"}' | bash plugins/code-quality/scripts/check-coderabbit-config.shoutputs missing config warning🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation