docs, feat(SREP-4460, SREP-4926: Add Standardized Claude hooks, skill, agents. Update standardised docs)#270
Conversation
|
Warning Review limit reached
More reviews will be available in 9 minutes and 20 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (21)
WalkthroughThis PR establishes a comprehensive Claude Code developer experience framework for the OCM Agent Operator repository. It introduces agents for automated validation (lint, test, security, docs, CI), integrates them with pre-commit hooks via prek, configures gitleaks for secret detection, and provides complete developer documentation on contributions, development, and testing workflows. ChangesClaude Developer Experience & Validation Framework
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels: 🚥 Pre-merge checks | ✅ 13 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (13 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: devppratik 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 |
devppratik
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@devppratik: This pull request references SREP-4460 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. This pull request references SREP-4926 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
![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 (2)
hack/prek.ci.toml (1)
30-63: ⚡ Quick winAdd a unit-test hook to this CI prek profile.
This config currently gates lint/build/tidy/RBAC only; it should also run unit tests in this pipeline path.
Proposed change
[[repos]] repo = "local" hooks = [ + # Unit tests (exclude integration tests under ./int/) + { + id = "go-test", + name = "go unit tests", + language = "system", + entry = "bash -c 'make test'", + files = '(\.go$|go\.(mod|sum)$)', + pass_filenames = false + }, + # Go build check { id = "go-build",As per coding guidelines,
!(int)/**/*_test.go: Unit tests should exclude integration tests in./int/and can be run withmake test.🤖 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 `@hack/prek.ci.toml` around lines 30 - 63, Add a new local hook entry named "unit-tests" to the existing [[repos]] block so the prek profile runs unit tests; define id "unit-tests", name "unit tests", language "system", pass_filenames = false and set entry to run the project Make target (e.g. "bash -c 'T=$(command -v timeout || command -v gtimeout || echo); ${T:+$T 120s} make test'") so tests are run under the CI timeout and rely on the Makefile to run only unit tests (excluding ./int/ per the guideline); place this hook alongside the existing "go-build" / "go-mod-tidy" hooks in the diff..claude/skills/README.md (1)
66-72: ⚡ Quick winAdd language specifier to the directory structure code fence.
The code fence showing the directory structure is missing a language specifier, which static analysis tools flag.
📝 Proposed fix
-**Directory structure**: -``` -.claude/skills/ -├── README.md -└── skillname/ - ├── SKILL.md # Required: skill definition - └── reference/ # Optional: supporting docs -``` +**Directory structure**: +```text +.claude/skills/ +├── README.md +└── skillname/ + ├── SKILL.md # Required: skill definition + └── reference/ # Optional: supporting docs +```🤖 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 @.claude/skills/README.md around lines 66 - 72, Update the README code block for the .claude/skills directory to include a language specifier (e.g., "text") so linters stop flagging it; edit the README.md section that renders the directory tree for .claude/skills and change the opening triple-backtick fence to ```text (and optionally add a short "Directory structure" label), leaving the directory contents (README.md, skillname/, SKILL.md, reference/) unchanged.
🤖 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 @.claude/hooks/README.md:
- Line 73: Update the README text that currently reads "Uses CI-compatible
config (skips network-dependent hooks like rh-pre-commit, gitleaks)" to reflect
the actual rationale: mention that gitleaks is skipped because it "may not be
available in all CI environments" (matching the rationale in hack/prek.ci.toml)
rather than calling it "network-dependent"; locate the string in
.claude/hooks/README.md and replace the wording so the README and
hack/prek.ci.toml rationale are consistent.
In @.claude/hooks/stop-prek-validation.sh:
- Around line 39-49: The validation exit logic uses FORCE_LINT to decide whether
to run but the subsequent prek invocation only validates staged files by
default, allowing unstaged edits to skip checks; update the script where prek is
invoked (the stop-time validation call in .claude/hooks/stop-prek-validation.sh)
to explicitly include all changed files (e.g., call prek run --all-files) or
compute and pass the staged+unstaged file list to prek so the scope of files
validated matches the trigger condition that checks git diff-index and git
ls-files; keep the FORCE_LINT gating and only alter the prek invocation to
ensure unstaged/untracked edits are covered.
In @.gitleaks.toml:
- Around line 25-32: Remove the global allowlist entry that matches all test
files by deleting the ".*_test\.go" pattern from the allowlist.paths in
.gitleaks.toml; instead, keep or add only narrowly scoped exemptions (e.g., the
existing '''test/fixtures/.*''', '''test/deploy/.*''', or other explicit
fixture/example paths) so findings in arbitrary *_test.go files are no longer
ignored. Locate the allowlist.paths block in .gitleaks.toml and remove the
'''.*_test\.go''' line, then run gitleaks to verify only intended paths remain
exempt.
In `@TESTING.md`:
- Around line 86-105: Unit Tests section is missing an explicit exclusion for
the integration test directory; update the "Unit Tests" paragraph under the Unit
Tests header to add a short sentence clarifying that the integration tests in
the int directory are excluded from unit test guidance (and either link to the
existing doc describing test targets or add a parenthetical note), explicitly
stating that the normal CI/dev target (make test) does not run tests in int and
that integration tests are executed separately via the integration test target
(for example: go test ./int/).
---
Nitpick comments:
In @.claude/skills/README.md:
- Around line 66-72: Update the README code block for the .claude/skills
directory to include a language specifier (e.g., "text") so linters stop
flagging it; edit the README.md section that renders the directory tree for
.claude/skills and change the opening triple-backtick fence to ```text (and
optionally add a short "Directory structure" label), leaving the directory
contents (README.md, skillname/, SKILL.md, reference/) unchanged.
In `@hack/prek.ci.toml`:
- Around line 30-63: Add a new local hook entry named "unit-tests" to the
existing [[repos]] block so the prek profile runs unit tests; define id
"unit-tests", name "unit tests", language "system", pass_filenames = false and
set entry to run the project Make target (e.g. "bash -c 'T=$(command -v timeout
|| command -v gtimeout || echo); ${T:+$T 120s} make test'") so tests are run
under the CI timeout and rely on the Makefile to run only unit tests (excluding
./int/ per the guideline); place this hook alongside the existing "go-build" /
"go-mod-tidy" hooks in the diff.
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a74f616f-43fa-4c73-96d0-acdc2f91f0f9
📒 Files selected for processing (21)
.claude/agents/README.md.claude/agents/ci-agent.md.claude/agents/docs-agent.md.claude/agents/lint-agent.md.claude/agents/security-agent.md.claude/agents/test-agent.md.claude/hooks/README.md.claude/hooks/cleanup.sh.claude/hooks/pre-edit.sh.claude/hooks/stop-prek-validation.sh.claude/settings.json.claude/skills/README.md.claude/skills/prow-ci/SKILL.md.gitleaks.toml.prek-versionCONTRIBUTING.mdDEVELOPMENT.mdTESTING.mdhack/ci.shhack/prek.ci.tomlprek.toml
| # Determine if validation should run: | ||
| # 1. If CLAUDE_LINT_ON_STOP=true → always run (user opt-in for strict mode) | ||
| # 2. Otherwise, only run if there are uncommitted changes (about to commit) | ||
| FORCE_LINT="${CLAUDE_LINT_ON_STOP:-false}" | ||
|
|
||
| if [[ "$FORCE_LINT" != "true" ]]; then | ||
| # Check for uncommitted changes (staged, unstaged, or untracked) | ||
| if git diff-index --quiet HEAD -- 2>/dev/null && [[ -z "$(git ls-files --others --exclude-standard)" ]]; then | ||
| # No changes and not forced - skip validation | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For the prekCLI, what files doesprek runvalidate by default when no filenames or--all-files flag is provided?
💡 Result:
When the prek CLI's "prek run" command is executed without specifying individual files or using the "--all-files" flag, it defaults to validating only the files that are currently staged in Git [1]. This behavior is similar to the default operation of "pre-commit run" [2][1].
Citations:
Fix stop-time validation coverage for unstaged edits
prek run validates only staged files by default when no filenames/--all-files are provided. In .claude/hooks/stop-prek-validation.sh, the trigger exits only when there are any uncommitted changes (including unstaged/untracked), but the validation command at the referenced prek run line does not appear to scope to unstaged files—so unstaged edits can bypass stop-time validation. Add an explicit scope (e.g., --all-files or a staged/unstaged-aware invocation) so the files checked match the trigger condition.
🤖 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 @.claude/hooks/stop-prek-validation.sh around lines 39 - 49, The validation
exit logic uses FORCE_LINT to decide whether to run but the subsequent prek
invocation only validates staged files by default, allowing unstaged edits to
skip checks; update the script where prek is invoked (the stop-time validation
call in .claude/hooks/stop-prek-validation.sh) to explicitly include all changed
files (e.g., call prek run --all-files) or compute and pass the staged+unstaged
file list to prek so the scope of files validated matches the trigger condition
that checks git diff-index and git ls-files; keep the FORCE_LINT gating and only
alter the prek invocation to ensure unstaged/untracked edits are covered.
| paths = [ | ||
| '''test/fixtures/.*''', | ||
| '''test/deploy/.*''', | ||
| '''.*_test\.go''', | ||
| '''boilerplate/.*''', | ||
| '''vendor/.*''', | ||
| '''zz_generated\..*\.go''', | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In gitleaks v8 TOML config, does [allowlist].pathswith pattern.*_test.goexclude all files ending in_test.go from detection?
💡 Result:
Yes—if the pattern you set under [allowlist].paths is .*_test\.go, it will exclude any file path that matches that regex, i.e., any path ending in _test.go (and possibly more, depending on the rest of the path). In gitleaks, allowlist.paths entries are compiled as regular expressions and a finding is ignored when the allowlist path regex matches the finding’s path value. The implementation combines the configured path regexes and uses MatchString(path) to decide whether a path is allowed to be ignored [1]. Also, gitleaks’ docs/examples show that allowlist matching uses the paths field as one of the match criteria for whether a finding should be ignored [2]. So with paths = ['''.*_test\.go'''], the regex .*_test\.go will match a path like .../foo_test.go, which effectively excludes _test.go files from detection for any findings whose path matches the allowlist criteria [1].
Citations:
- 1: https://github.com/gitleaks/gitleaks/blob/master/config/allowlist.go
- 2: https://github.com/gitleaks/gitleaks
Remove global allowlist for *_test.go from .gitleaks.toml
allowlist.paths includes .*_test\.go, which regex-matches finding path values and causes gitleaks to ignore any findings in files ending with _test.go, creating a security blind spot. Remove that entry and scope any exemptions to specific fixture/example paths instead.
🤖 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 @.gitleaks.toml around lines 25 - 32, Remove the global allowlist entry that
matches all test files by deleting the ".*_test\.go" pattern from the
allowlist.paths in .gitleaks.toml; instead, keep or add only narrowly scoped
exemptions (e.g., the existing '''test/fixtures/.*''', '''test/deploy/.*''', or
other explicit fixture/example paths) so findings in arbitrary *_test.go files
are no longer ignored. Locate the allowlist.paths block in .gitleaks.toml and
remove the '''.*_test\.go''' line, then run gitleaks to verify only intended
paths remain exempt.
| ## Test Organization | ||
|
|
||
| ### Unit Tests | ||
| - Test individual functions and methods | ||
| - Mock external dependencies (Kubernetes client, HTTP calls) | ||
| - Fast execution (<1s per package) | ||
| - Located alongside source code | ||
|
|
||
| ### Controller Tests | ||
| - Test reconciliation logic | ||
| - Use envtest for simulated Kubernetes API | ||
| - Test custom resource lifecycle | ||
| - Located in `controllers/*/` | ||
|
|
||
| ### E2E Tests | ||
| - Full operator deployment | ||
| - Real cluster interaction | ||
| - Located in `test/e2e/` | ||
| - Run in CI via Tekton | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if ./int/ directory exists and contains tests
echo "=== Checking for ./int/ directory ==="
if [ -d "./int" ]; then
echo "Directory ./int/ exists"
fd -t f -e go . ./int
else
echo "Directory ./int/ does not exist"
fi
echo ""
echo "=== Checking for references to int tests in documentation ==="
rg -i 'int.*test|integration.*test' -g '*.md'Repository: openshift/managed-node-metadata-operator
Length of output: 2164
Document ./int/ exclusion for Unit Tests in TESTING.md (lines 86-105).
./int/ exists and contains integration tests, but TESTING.md’s Unit Tests section doesn’t mention excluding ./int/ from the unit-test guidance. Add an explicit note (or cross-link to the existing doc that states make test excludes ./int/ and go test ./int/ runs integration tests).
🤖 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 `@TESTING.md` around lines 86 - 105, Unit Tests section is missing an explicit
exclusion for the integration test directory; update the "Unit Tests" paragraph
under the Unit Tests header to add a short sentence clarifying that the
integration tests in the int directory are excluded from unit test guidance (and
either link to the existing doc describing test targets or add a parenthetical
note), explicitly stating that the normal CI/dev target (make test) does not run
tests in int and that integration tests are executed separately via the
integration test target (for example: go test ./int/).
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #270 +/- ##
=======================================
Coverage 58.60% 58.60%
=======================================
Files 5 5
Lines 244 244
=======================================
Hits 143 143
Misses 87 87
Partials 14 14 🚀 New features to boost your workflow:
|
devppratik
force-pushed
the
add-claude-and-docs
branch
2 times, most recently
from
6f3b031 to
a34cb2e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
.claude/hooks/README.md (1)
73-73:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify gitleaks exclusion rationale.
This line describes gitleaks as "network-dependent," which is inconsistent with the actual rationale stated on line 149: "may not be available in all CI environments." Gitleaks runs locally without network access.
📝 Suggested fix
-Uses CI-compatible config (skips network-dependent hooks like rh-pre-commit, gitleaks) +Uses CI-compatible config (skips hooks requiring internal network access or that may not be available: rh-pre-commit, gitleaks)🤖 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 @.claude/hooks/README.md at line 73, Update the README line that currently reads "Uses CI-compatible config (skips network-dependent hooks like rh-pre-commit, gitleaks)" to accurately reflect the reason for skipping gitleaks: change the phrase to indicate gitleaks "may not be available in all CI environments" (or similar) rather than calling it "network-dependent"; locate and edit the sentence containing "rh-pre-commit, gitleaks" so it matches the rationale used elsewhere (e.g., the line referencing availability in CI environments).
🤖 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 @.claude/agents/docs-agent.md:
- Around line 185-198: The code block under the "## Output Format" section (the
example starting with "Updated: DEVELOPMENT.md") is missing a language tag;
update the fenced code block to include the required language identifier (use
`bash` per the file's style rule) so the block reads with a language tag (change
the opening fence from ``` to ```bash) to comply with the styling rule
referenced earlier.
In @.claude/hooks/README.md:
- Line 182: Update the README.md entry that currently says the stop hook runs
"prek run --all-files" to match the actual behavior implemented in
stop-prek-validation.sh: document that the hook runs "prek run --config
hack/prek.ci.toml" and that it validates only changed files (for speed) rather
than all files. Mention the exact command and the changed-files-only behavior so
the documentation matches the script.
In @.claude/skills/README.md:
- Around line 66-72: The fenced code block showing the directory tree in
.claude/skills/ README.md lacks a language specifier; update the opening fence
from ``` to a language-tagged fence such as ```text (or ```bash) so the snippet
(the block containing ".claude/skills/ ├── README.md └── skillname/") includes a
language specifier for proper Markdown rendering and consistency.
---
Duplicate comments:
In @.claude/hooks/README.md:
- Line 73: Update the README line that currently reads "Uses CI-compatible
config (skips network-dependent hooks like rh-pre-commit, gitleaks)" to
accurately reflect the reason for skipping gitleaks: change the phrase to
indicate gitleaks "may not be available in all CI environments" (or similar)
rather than calling it "network-dependent"; locate and edit the sentence
containing "rh-pre-commit, gitleaks" so it matches the rationale used elsewhere
(e.g., the line referencing availability in CI environments).
🪄 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 YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f35d25d3-0fea-4b4a-9829-b07d968c8acd
📒 Files selected for processing (21)
.claude/agents/README.md.claude/agents/ci-agent.md.claude/agents/docs-agent.md.claude/agents/lint-agent.md.claude/agents/security-agent.md.claude/agents/test-agent.md.claude/hooks/README.md.claude/hooks/cleanup.sh.claude/hooks/pre-edit.sh.claude/hooks/stop-prek-validation.sh.claude/settings.json.claude/skills/README.md.claude/skills/prow-ci/SKILL.md.gitleaks.toml.prek-versionCONTRIBUTING.mdDEVELOPMENT.mdTESTING.mdhack/ci.shhack/prek.ci.tomlprek.toml
✅ Files skipped from review due to trivial changes (9)
- .prek-version
- .claude/hooks/cleanup.sh
- DEVELOPMENT.md
- .claude/agents/lint-agent.md
- CONTRIBUTING.md
- .claude/agents/ci-agent.md
- TESTING.md
- .claude/agents/security-agent.md
- hack/prek.ci.toml
🚧 Files skipped from review as they are similar to previous changes (6)
- hack/ci.sh
- .gitleaks.toml
- .claude/settings.json
- .claude/agents/test-agent.md
- .claude/hooks/stop-prek-validation.sh
- prek.toml
| ``` | ||
| .claude/skills/ | ||
| ├── README.md | ||
| └── skillname/ | ||
| ├── SKILL.md # Required: skill definition | ||
| └── reference/ # Optional: supporting docs | ||
| ``` |
There was a problem hiding this comment.
Add language specifier to fenced code block.
The fenced code block showing directory structure should have a language specifier for consistency with markdown best practices.
📝 Proposed fix
**Directory structure**:
-```
+```text
.claude/skills/
├── README.md
└── skillname/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` | |
| .claude/skills/ | |
| ├── README.md | |
| └── skillname/ | |
| ├── SKILL.md # Required: skill definition | |
| └── reference/ # Optional: supporting docs | |
| ``` | |
| **Directory structure**: |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 66-66: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 @.claude/skills/README.md around lines 66 - 72, The fenced code block showing
the directory tree in .claude/skills/ README.md lacks a language specifier;
update the opening fence from ``` to a language-tagged fence such as ```text (or
```bash) so the snippet (the block containing ".claude/skills/ ├── README.md └──
skillname/") includes a language specifier for proper Markdown rendering and
consistency.
a34cb2e to
05e47b8
Compare
- Add .claude/ directory with agents, hooks, and skills - Add prek validation framework (prek.toml, hack/prek.ci.toml) - Add gitleaks secret scanning (.gitleaks.toml) - Add CONTRIBUTING.md, DEVELOPMENT.md, TESTING.md - Add CLAUDE.md (if not already present) - Add stop hook for automatic validation Based on ocm-agent-operator PR openshift#257 (SREP-4410, SREP-4411) Brings the repo up to Agentic SDLC contribution standard
devppratik
force-pushed
the
add-claude-and-docs
branch
from
05e47b8 to
9ce23bb
Compare
|
@devppratik: all tests passed! 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. |
3 participants
What type of PR is this?
docs/feature
What this PR does / why we need it?
Changes
New Infrastructure:
Validation:
Documentation:
References
Summary by CodeRabbit
Documentation
Security & Validation
Chores