[SREP-4485]add pre-commit hooks#245
Open
charlesgong wants to merge 4 commits intoopenshift:masterfrom
Open
Conversation
Adds .pre-commit-config.yaml with Tier 1 common hooks mirroring ci/prow/lint: - File hygiene (merge conflicts, trailing whitespace, EOF) - YAML syntax validation on deploy/ - Secrets detection (gitleaks) - Static analysis (golangci-lint v2.0.2, same version and config as CI) - Compile check (go build) - Dependency drift guard (go mod tidy) - RBAC wildcard permission check - Unit tests (pkg/, api/, controllers/ — no envtest dependency) Also adds .claude/commands/pre-commit.md defining the /pre-commit agent with golden rule compliance: 2-retry limit, security escalation, structured output. All hooks validated passing on ocm-agent-operator master. Golden rules: SREP-4450 Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (6)
WalkthroughAdds a new agent command guide for running and retrying Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Agent as Agent (runner)
participant Git as Git (index/worktree)
participant PreCommit as pre-commit CLI
participant Hooks as Hook processes
participant FS as Filesystem
participant Human as Human (escalation)
Agent->>Git: determine target files (staged/all/specific hook)
Agent->>PreCommit: run `pre-commit run` (captures stdout/stderr)
PreCommit->>Hooks: invoke hooks (concurrent)
Hooks->>PreCommit: return results (Passed/Auto-fixed/Failed + metadata)
PreCommit->>Agent: deliver full output
Agent->>FS: stage auto-fixed files (if any)
Agent->>PreCommit: re-run on staged contents (loop, up to 3 attempts)
Hooks->>Agent: if security-hook failed or attempts exhausted -> fail
Agent->>Human: escalate with `PRE-COMMIT SUMMARY` and full error output
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: charlesgong The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
.claude/commands/pre-commit.md (1)
40-43: Keep rerun scope consistent with the selected run mode.Line 42 always reruns staged-only. If Step 2 was
--all-files, this narrows coverage mid-flow.Suggested fix
-2. Re-run the hooks on staged files +2. Re-run hooks using the same mode chosen in Step 2 + - if `--all-files`, run `pre-commit run --all-files` + - otherwise run `pre-commit run` (staged files)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/commands/pre-commit.md around lines 40 - 43, The instructions always rerun hooks on staged files (the line "Re-run the hooks on staged files") which forces a staged-only rerun even when the initial run used --all-files; change this flow to preserve and re-apply the original run mode—detect whether the initial run was --all-files or staged-only and, after auto-fixes, re-run hooks using the same mode (e.g., re-run with --all-files when the original run used --all-files, otherwise re-run staged-only), and update the step text to reflect the chosen mode rather than hardcoding "staged files".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/commands/pre-commit.md:
- Around line 77-85: The fenced code block containing the "PRE-COMMIT SUMMARY"
header is missing a language specifier (causing markdownlint MD040); update the
triple-backtick fence in .claude/commands/pre-commit.md for the block starting
with "PRE-COMMIT SUMMARY" to include a language token such as text (e.g., change
``` to ```text) so the block is explicitly marked and the linter warning is
resolved.
In @.pre-commit-config.yaml:
- Around line 119-124: The go-mod-tidy pre-commit hook (id: go-mod-tidy)
currently only triggers when go.mod or go.sum are staged due to the files:
go\.(mod|sum)$ filter; update the hook to always run by removing or replacing
that files filter with always_run: true so that the entry (the bash command in
entry) executes for commits that only change .go files and catches dependency
drift; keep existing settings like language and pass_filenames: false intact.
- Around line 132-136: The rbac-wildcard-check hook (id: rbac-wildcard-check)
currently uses a line-by-line grep and misses YAML list wildcards when `verbs:`
or `resources:` and `- '*'` are on separate lines; change the hook's entry to
perform a multiline-aware search (e.g., use grep -Pz or pcregrep with a PCRE
that matches a `verbs` or `resources` key followed by a list item `*`, or use yq
to parse deploy/*.yaml and check for "*" entries) so it reliably detects `- '*'`
under either `verbs` or `resources` across lines. Ensure the updated entry still
confines checks to deploy/*.ya?ml files and returns non-zero when any wildcard
is found.
---
Nitpick comments:
In @.claude/commands/pre-commit.md:
- Around line 40-43: The instructions always rerun hooks on staged files (the
line "Re-run the hooks on staged files") which forces a staged-only rerun even
when the initial run used --all-files; change this flow to preserve and re-apply
the original run mode—detect whether the initial run was --all-files or
staged-only and, after auto-fixes, re-run hooks using the same mode (e.g.,
re-run with --all-files when the original run used --all-files, otherwise re-run
staged-only), and update the step text to reflect the chosen mode rather than
hardcoding "staged files".
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 75914646-4f59-417d-8c80-4e0d43937120
📒 Files selected for processing (2)
.claude/commands/pre-commit.md.pre-commit-config.yaml
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #245 +/- ##
=======================================
Coverage 66.25% 66.25%
=======================================
Files 23 23
Lines 1544 1544
=======================================
Hits 1023 1023
Misses 445 445
Partials 76 76
🚀 New features to boost your workflow:
|
- Add language tag to fenced code block in /pre-commit command (MD040) - Expand go-mod-tidy trigger to .go files to catch dependency drift from new imports (not just when go.mod/go.sum are directly staged) - Fix RBAC wildcard detection: replace grep with Python context-aware check that handles multi-line YAML format (- '*' under verbs/resources) The original grep only matched inline format ["*"] and missed 6 real violations present in deploy/*.yaml Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.pre-commit-config.yaml:
- Around line 136-137: The RBAC wildcard detector one-liner uses top-level glob
and only detects multiline lines exactly equal to '*' within 5 prior lines, so
it misses inline forms like verbs: ["*"], gaps with comments/blank lines, and
nested deploy paths; update the one-liner logic to (1) use recursive glob (e.g.,
glob(..., recursive=True) with a pattern like 'deploy/**/*.y*ml') to match
nested files consistently with the files regex, (2) expand the parser in the
violations list comprehension to detect both inline list/value forms (e.g.,
lines containing 'verbs:' or 'resources:' followed on the same line by '*' or
'["*"]' or "['*']") and multiline forms while allowing a few intervening
blank/comment lines between the key and the wildcard value, and (3) increase the
lookback window (currently max(0,i-5)) as needed to tolerate small gaps; keep
using the same printing/sys.exit behavior but ensure the glob and detection
logic in the one-liner (the violations comprehension and glob.glob call) are
updated accordingly.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e5b10372-337c-4530-b4d9-cb46a72d5c76
📒 Files selected for processing (2)
.claude/commands/pre-commit.md.pre-commit-config.yaml
✅ Files skipped from review due to trivial changes (1)
- .claude/commands/pre-commit.md
Comment on lines
+136
to
+137
| entry: "python3 -c \"import sys,glob;violations=[(f,n,l.rstrip()) for f in glob.glob('deploy/*.yaml')+glob.glob('deploy/*.yml') for lines in [list(enumerate(open(f),1))] for i,(n,l) in enumerate(lines) if l.strip().lstrip('- ').strip(chr(39)+chr(34))=='*' and any(lines[j][1].strip() in ('verbs:','resources:') for j in range(max(0,i-5),i))];[print(v[0]+'|'+str(v[1])+'|'+v[2]) for v in violations];sys.exit(int(bool(violations)))\"" | ||
| files: ^deploy/.*\.ya?ml$ |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
def current_detect(text):
lines = list(enumerate(text.splitlines(True), 1))
out = []
for i, (n, l) in enumerate(lines):
if l.strip().lstrip('- ').strip(chr(39)+chr(34)) == '*' and any(
lines[j][1].strip() in ('verbs:', 'resources:') for j in range(max(0, i-5), i)
):
out.append((n, l.rstrip()))
return out
cases = {
"inline_verbs": "verbs: [\"*\"]\n",
"multiline_gap_bypass": "verbs:\n `#1`\n `#2`\n `#3`\n `#4`\n `#5`\n `#6`\n - \"*\"\n",
"simple_multiline": "resources:\n - \"*\"\n",
}
for name, content in cases.items():
print(name, "=>", current_detect(content))
PY
echo
echo "YAML files currently under deploy/:"
find deploy -type f \( -name '*.yaml' -o -name '*.yml' \) | sortRepository: openshift/ocm-agent-operator
Length of output: 923
🏁 Script executed:
#!/bin/bash
# Check for wildcard patterns in all deploy YAML files
echo "=== Checking for verbs:/resources: with wildcards in deploy/ ==="
rg -A 3 -B 1 "verbs:|resources:" deploy/ --type yaml | head -100
echo
echo "=== Checking for inline wildcard syntax ==="
rg '\[.*"\*".*\]|\["*\*"*\]' deploy/ --type yaml
echo
echo "=== File count comparison ==="
echo "Top-level deploy files (*.yaml/*.yml):"
find deploy -maxdepth 1 -type f \( -name '*.yaml' -o -name '*.yml' \) | wc -l
echo "All deploy files including nested:"
find deploy -type f \( -name '*.yaml' -o -name '*.yml' \) | wc -l
echo
echo "=== Files in deploy/crds (nested) ==="
ls -la deploy/crds/ 2>/dev/null | head -20Repository: openshift/ocm-agent-operator
Length of output: 6045
RBAC wildcard detector has bypasses: misses inline syntax and can be evaded with comment gaps.
At Line 136, detection only matches lines that normalize to * in multiline form and only if verbs:/resources: appears in the prior 5 lines. This misses inline forms like verbs: ["*"] and can be bypassed entirely by inserting comments/blank lines between the key and wildcard value. Additionally, the detector uses glob.glob('deploy/*.yaml')+glob.glob('deploy/*.yml') (top-level only) while Line 137 regex ^deploy/.*\.ya?ml$ includes nested paths, creating inconsistent scope that could miss future RBAC files.
Suggested hardening diff
- entry: "python3 -c \"import sys,glob;violations=[(f,n,l.rstrip()) for f in glob.glob('deploy/*.yaml')+glob.glob('deploy/*.yml') for lines in [list(enumerate(open(f),1))] for i,(n,l) in enumerate(lines) if l.strip().lstrip('- ').strip(chr(39)+chr(34))=='*' and any(lines[j][1].strip() in ('verbs:','resources:') for j in range(max(0,i-5),i))];[print(v[0]+'|'+str(v[1])+'|'+v[2]) for v in violations];sys.exit(int(bool(violations)))\""
+ entry: >
+ bash -c 'python3 - <<'"'"'PY'"'"'
+ import glob, re, sys
+ violations = []
+ files = glob.glob("deploy/**/*.yaml", recursive=True) + glob.glob("deploy/**/*.yml", recursive=True)
+ for f in files:
+ with open(f, encoding="utf-8") as fh:
+ ctx = None # (key, indent)
+ for n, raw in enumerate(fh, 1):
+ line = raw.split("#", 1)[0].rstrip()
+ if not line.strip():
+ continue
+ m = re.match(r"^(\s*)(verbs|resources):\s*(\[[^\]]*\])?\s*$", line)
+ if m:
+ indent = len(m.group(1))
+ inline = m.group(3) or ""
+ ctx = (m.group(2), indent)
+ if re.search(r"(^|[,\[])\s*[\"'"'"']?\*[\"'"'"']?\s*([,\]]|$)", inline):
+ violations.append((f, n, raw.rstrip()))
+ continue
+ if ctx:
+ indent = len(raw) - len(raw.lstrip(" "))
+ if indent <= ctx[1] and not raw.lstrip().startswith("-"):
+ ctx = None
+ elif re.match(r"^\s*-\s*[\"'"'"']?\*[\"'"'"']?\s*$", line):
+ violations.append((f, n, raw.rstrip()))
+ for f, n, l in violations:
+ print(f"{f}|{n}|{l}")
+ sys.exit(1 if violations else 0)
+ PY'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.pre-commit-config.yaml around lines 136 - 137, The RBAC wildcard detector
one-liner uses top-level glob and only detects multiline lines exactly equal to
'*' within 5 prior lines, so it misses inline forms like verbs: ["*"], gaps with
comments/blank lines, and nested deploy paths; update the one-liner logic to (1)
use recursive glob (e.g., glob(..., recursive=True) with a pattern like
'deploy/**/*.y*ml') to match nested files consistently with the files regex, (2)
expand the parser in the violations list comprehension to detect both inline
list/value forms (e.g., lines containing 'verbs:' or 'resources:' followed on
the same line by '*' or '["*"]' or "['*']") and multiline forms while allowing a
few intervening blank/comment lines between the key and the wildcard value, and
(3) increase the lookback window (currently max(0,i-5)) as needed to tolerate
small gaps; keep using the same printing/sys.exit behavior but ensure the glob
and detection logic in the one-liner (the violations comprehension and glob.glob
call) are updated accordingly.
Re-run make op-generate to sync generated files with current controller-gen version. Required for generate-check to pass. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Bumps ubi9/ubi-minimal from 9.7-1776104705 to 9.7-1776833838. --- updated-dependencies: - dependency-name: ubi9/ubi-minimal dependency-version: 9.7-1776833838 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <support@github.com>
Contributor
|
@charlesgong: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds .pre-commit-config.yaml with Tier 1 common hooks mirroring ci/prow/lint:
Also adds .claude/commands/pre-commit.md defining the /pre-commit agent with golden rule compliance: 2-retry limit, security escalation, structured output.
All hooks validated passing on ocm-agent-operator master. Golden rules: SREP-4450
What type of PR is this?
(bug/feature/cleanup/documentation/test/refactor)
What this PR does / why we need it?
Which Jira/Github issue(s) this PR fixes?
Fixes #
Special notes for your reviewer:
Pre-checks (if applicable):
make generatecommand locally to validate code changesSummary by CodeRabbit