SREP-4410: Claude Agent, Hooks and Skills Setup MVP#243
SREP-4410: Claude Agent, Hooks and Skills Setup MVP#243devppratik wants to merge 2 commits intoopenshift:masterfrom
Conversation
|
@devppratik: This pull request references SREP-4410 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 epic 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. |
openshift-ci-robot
added
the
jira/valid-reference
|
[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 |
WalkthroughIntroduces Claude Code integration infrastructure including configuration files, agent definitions for comprehensive code review (security, linting, testing, error handling), automation hooks for pre-commit validation and user prompts, custom skills for full reviews and CRD updates, and corresponding documentation. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #243 +/- ##
=======================================
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:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
♻️ Duplicate comments (1)
.claude/agents/error-handling-reviewer.md (1)
8-12:⚠️ Potential issue | 🟡 MinorSame "main" vs
mastermismatch as insecurity-reviewer.md.Prose says "vs the main branch" but the diff command uses
master. Align the text with the actual base branch to avoid confusing the model about which ref to diff against.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/agents/error-handling-reviewer.md around lines 8 - 12, The README text in error-handling-reviewer.md mismatches the git ref: it says "vs the main branch" while the diff command uses `master`; update the prose to consistently reference `master` (or alternatively change the diff command to `main`) so both the sentence and the diff command match; edit the string "vs the main branch" (and any other occurrences) to match the chosen base branch, ensuring consistency with the existing `git diff master...HEAD` usage in this file.
🧹 Nitpick comments (2)
.claude/agents/error-handling-reviewer.md (1)
94-94: Specify a language on the output template fence (markdownlint MD040).Adding
markdownafter the opening backticks silences the lint warning and is consistent with intent.✏️ Proposed fix
-``` +```markdown ## Error Handling Review Report🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/agents/error-handling-reviewer.md at line 94, The code block that begins immediately before "## Error Handling Review Report" is missing a language specifier which triggers markdownlint MD040; update the opening fence for that output template fence to add the language tag "markdown" (i.e., change the triple backticks that precede the "## Error Handling Review Report" header to ```markdown) so the linter is satisfied and the intent is explicit..claude/hooks/before-commit (1)
14-14: UnusedLINT_EXIT/TEST_EXITcaptures.
LINT_EXIT=$?(line 14) andTEST_EXIT=$?(line 30) are assigned but never referenced before the explicitexit 1. Either drop them or surface the value in the error payload for easier debugging.🧹 Proposed cleanup
-LINT_OUTPUT=$(make lint 2>&1) || { - LINT_EXIT=$? +LINT_OUTPUT=$(make lint 2>&1) || {-TEST_OUTPUT=$(make go-test 2>&1) || { - TEST_EXIT=$? +TEST_OUTPUT=$(make go-test 2>&1) || {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/hooks/before-commit at line 14, The hook currently assigns LINT_EXIT and TEST_EXIT but never uses them; either remove those unused assignments or surface the values when failing so failures are debuggable. Update the before-commit hook: when capturing exit codes (LINT_EXIT and TEST_EXIT) either delete those variables and directly use the previous command's $? in your failure handling, or include the captured values in the error output and use them as the exit code (e.g., echo a clear message containing LINT_EXIT/TEST_EXIT and then exit with that numeric value). Ensure you update the failure branches that currently call exit 1 to reference the captured variable (LINT_EXIT or TEST_EXIT) so the real failure code is propagated.
🤖 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/agents/lint-reviewer.md:
- Around line 84-86: The fenced code block containing
"boilerplate/openshift/golang-osd-operator/golangci.yml" is missing a language
tag which triggers MD040; add an explicit language identifier (e.g., ```yaml or
```text) on the opening fence for that snippet so the markdown linter recognizes
the block language.
In @.claude/agents/security-reviewer.md:
- Around line 8-12: The README text and the example command are inconsistent:
the prose says "vs the main branch" but the command uses `git diff
master...HEAD`; update the wording or command so they match. Either change the
prose phrase "vs the main branch" to "vs the master branch" to match the
existing command, or replace the hardcoded `master` in the `git diff
master...HEAD` command with a parameter/variable (e.g., `${BASE_BRANCH}`) and
show how to set it (or detect default branch) so the instruction works across
repos. Ensure both the descriptive text ("vs the main branch") and the command
(`git diff ...`) use the same branch identifier.
In @.claude/agents/test-reviewer.md:
- Around line 87-103: Add a fenced code language tag to the output template's
code block that starts with the triple backticks (the block containing "## Test
Review Report" etc.) so it satisfies MD040; update the opening fence (``` ) to
include a language identifier (for example ```md or ```markdown or ```txt) to
mark the block's language and prevent lint failures.
- Around line 8-10: The branch baseline strings are inconsistent—one occurrence
uses "main" and another uses "master...HEAD"; pick a single canonical baseline
(e.g., "main") and make both tokens consistent in the file by replacing
"master...HEAD" with "main...HEAD" (or vice versa if you prefer "master"), and
update any adjacent instructions/comments that reference the baseline so the
review scope is unambiguous.
In @.claude/hooks/before-commit:
- Around line 13-42: The JSON currently built in the lint and test failure
blocks is invalid when outputs contain newlines/control-chars because the sed
pipeline only escapes backslashes and quotes; replace the sed-based escaping
with proper JSON encoding via jq: capture the raw tail output into
LINT_OUTPUT/TEST_OUTPUT as before, set ERROR_SUMMARY using the tail result (keep
the variable names), then build the JSON payload with jq (for example use jq -n
--arg body "$ERROR_SUMMARY" '[{role:"user", content: "❌ Linting failed. Recent
errors:\n```\n"+$body+"\n```\n\nRun '\''make lint'\'' to see full output."}]')
for the lint branch and similarly for the test branch (use "❌ Tests failed..."
and run 'make go-test'); also remove or use the unused LINT_EXIT/TEST_EXIT
variables or delete those assignments.
In @.claude/hooks/user-prompt-submit:
- Line 9: The hook uses plain git diff --name-only which misses staged and
untracked files; update the condition that currently references git diff
--name-only so it diffs against HEAD and also includes untracked files (e.g.,
combine git diff --name-only HEAD with git ls-files --others --exclude-standard
or union both outputs) and then pipe that combined list to grep -qE
'\.(go|yaml|yml)$' so staged and new files trigger the reminder; modify the if
line that contains git diff --name-only to use this combined approach.
In @.claude/README.md:
- Around line 7-29: The fenced code block that renders the directory tree (the
triple-backtick block containing ".claude/ ├── README.md ..." ) is missing a
language identifier and triggers MD040; update the opening fence from ``` to
```text (or ```bash) so the block has a language tag, e.g. replace the existing
``` with ```text to satisfy the linter while keeping the tree formatting intact.
- Around line 139-153: The docs list permissions with wildcard-style entries
like `Bash:make *` but the actual `.claude/settings.json` uses explicit
function-like syntax (e.g., `Bash(make lint)` and `Bash(git diff *)`); update
the README section (the lines showing `Read:*, Glob:*, Grep:*, Bash:make *,
Bash:go *, Bash:git *, Bash:kubectl get/describe/explain, Bash:gh *`) to match
the real `settings.json` format and include concrete examples and a short
example snippet showing how to add a permission such as `Bash(make lint)` and
`Bash(git diff *)` under the "permissions.allow" array so users won’t produce
invalid configs.
In @.claude/settings.json:
- Around line 23-24: The auto-allow entries "Bash(make install)" and "Bash(make
uninstall)" in .claude/settings.json permit destructive cluster changes; remove
these two strings from the allow list (or move them to a separate, explicitly
confirmed actions list) so that executing make install/uninstall requires
interactive human approval rather than being pre-authorized by Claude.
- Around line 46-52: The deny list in .claude/settings.json currently has
Read(./.env) and Read(./.env.*) which only match at repo root; update the deny
array to also block nested .env files by adding recursive variants such as
Read(./**/.env) and Read(./**/.env.*) (or similar glob patterns) alongside the
existing Read(./.env) and Read(./.env.*) entries so subdirectory files like
config/.env are covered.
In @.claude/skills/full-review/SKILL.md:
- Around line 15-24: The skill text "Launch all review agents in parallel"
conflicts with the agents' frontmatter which sets isolation: worktree for the
agents security-reviewer, lint-reviewer, test-reviewer, and
error-handling-reviewer; pick one fix: either remove isolation: worktree from
those four agent definitions (security-reviewer, lint-reviewer, test-reviewer,
error-handling-reviewer) if you intend true parallel read-only runs without
worktrees, or update the skill wording to require per-agent worktree isolation
(rephrase the parallel step to spawn agents respecting each agent's isolation:
worktree); ensure the chosen approach is applied consistently across the skill
text and the four agent frontmatters so behavior is unambiguous.
In `@CLAUDE.md`:
- Line 108: Docs mismatch: update the CLAUDE.md entry for the "before-commit"
hook to match the actual hook command; change the text that currently says
"Automatically runs `make test` and `make lint`" to state it runs `make go-test`
(since `.claude/hooks/before-commit` invokes `make go-test`) so readers
searching for the exact make target find the real command.
In `@docs/CLAUDE_CODE_GUIDE.md`:
- Around line 91-96: The documentation lists the test command as `make test` but
the repository hooks/skills expect `make go-test`; update all occurrences (e.g.,
the section heading "What it runs" and the listed bullet `make test` and the
other mentions around lines referenced) to `make go-test` (or add a note that
`make test` is an alias if you prefer), and ensure the examples and any guidance
consistently use `make go-test` so copy-pasted commands match configured hooks
like the go-test skill.
- Around line 226-250: The example in the "/full-review" output (under the "##
Full Review Report" block attributed to "Claude") incorrectly states "Launches
all 5 review agents" and includes agent categories not present in the configured
set; update this example to match the actual configured 4-reviewer set by
removing or renaming the extra agent category lines (e.g., drop or combine the
Performance/Concurrency sections as appropriate), adjust the summary counts ("3
high, 3 medium") and the estimated fix time to reflect the corrected categories,
and ensure the header and inline text no longer reference "5 review agents" so
the example is consistent with the real configuration.
- Around line 61-63: Several fenced code blocks in CLAUDE_CODE_GUIDE.md lack
language identifiers and trigger MD040; update each unlabeled triple-backtick
block (e.g., the block containing "/full-review" and the other blocks noted in
the review) by adding the appropriate language tag (bash, text, or markdown)
based on the snippet content so the linter recognizes the language and MD040 is
satisfied; scan the document for other unlabeled ```...``` blocks (including the
ranges called out in the review) and add consistent language tags to all of
them.
---
Duplicate comments:
In @.claude/agents/error-handling-reviewer.md:
- Around line 8-12: The README text in error-handling-reviewer.md mismatches the
git ref: it says "vs the main branch" while the diff command uses `master`;
update the prose to consistently reference `master` (or alternatively change the
diff command to `main`) so both the sentence and the diff command match; edit
the string "vs the main branch" (and any other occurrences) to match the chosen
base branch, ensuring consistency with the existing `git diff master...HEAD`
usage in this file.
---
Nitpick comments:
In @.claude/agents/error-handling-reviewer.md:
- Line 94: The code block that begins immediately before "## Error Handling
Review Report" is missing a language specifier which triggers markdownlint
MD040; update the opening fence for that output template fence to add the
language tag "markdown" (i.e., change the triple backticks that precede the "##
Error Handling Review Report" header to ```markdown) so the linter is satisfied
and the intent is explicit.
In @.claude/hooks/before-commit:
- Line 14: The hook currently assigns LINT_EXIT and TEST_EXIT but never uses
them; either remove those unused assignments or surface the values when failing
so failures are debuggable. Update the before-commit hook: when capturing exit
codes (LINT_EXIT and TEST_EXIT) either delete those variables and directly use
the previous command's $? in your failure handling, or include the captured
values in the error output and use them as the exit code (e.g., echo a clear
message containing LINT_EXIT/TEST_EXIT and then exit with that numeric value).
Ensure you update the failure branches that currently call exit 1 to reference
the captured variable (LINT_EXIT or TEST_EXIT) so the real failure code is
propagated.
🪄 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: 6f32ec71-2b49-4347-bbcd-0df2266ee115
📒 Files selected for processing (13)
.claude/README.md.claude/agents/error-handling-reviewer.md.claude/agents/lint-reviewer.md.claude/agents/security-reviewer.md.claude/agents/test-reviewer.md.claude/hooks/before-commit.claude/hooks/user-prompt-submit.claude/settings.json.claude/skills/full-review/SKILL.md.claude/skills/run-tests/SKILL.md.claude/skills/update-crds/SKILL.mdCLAUDE.mddocs/CLAUDE_CODE_GUIDE.md
| ``` | ||
| boilerplate/openshift/golang-osd-operator/golangci.yml | ||
| ``` |
There was a problem hiding this comment.
Use a language tag for the fenced config snippet.
The code block at Line 84 should specify a language (e.g., yaml or text) to satisfy MD040.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 84-84: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/agents/lint-reviewer.md around lines 84 - 86, The fenced code block
containing "boilerplate/openshift/golang-osd-operator/golangci.yml" is missing a
language tag which triggers MD040; add an explicit language identifier (e.g.,
```yaml or ```text) on the opening fence for that snippet so the markdown linter
recognizes the block language.
| Review all changes on the current branch vs the main branch for security issues. | ||
|
|
||
| IMPORTANT: Only review files and lines that appear in the diff (`git diff master...HEAD`). | ||
| You may read surrounding context in those files to understand the change, but do NOT report | ||
| findings on files or code that are not part of the changeset. |
There was a problem hiding this comment.
Branch name inconsistency ("main" vs master).
Line 8 says "vs the main branch" while the command on line 10 uses master. This repo's default branch is master, so either align the prose with the command (recommended) or parameterize the base branch so the agent works across repos with different defaults.
📝 Proposed fix
-Review all changes on the current branch vs the main branch for security issues.
+Review all changes on the current branch vs the `master` branch for security issues.📝 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.
| Review all changes on the current branch vs the main branch for security issues. | |
| IMPORTANT: Only review files and lines that appear in the diff (`git diff master...HEAD`). | |
| You may read surrounding context in those files to understand the change, but do NOT report | |
| findings on files or code that are not part of the changeset. | |
| Review all changes on the current branch vs the `master` branch for security issues. | |
| IMPORTANT: Only review files and lines that appear in the diff (`git diff master...HEAD`). | |
| You may read surrounding context in those files to understand the change, but do NOT report | |
| findings on files or code that are not part of the changeset. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/agents/security-reviewer.md around lines 8 - 12, The README text and
the example command are inconsistent: the prose says "vs the main branch" but
the command uses `git diff master...HEAD`; update the wording or command so they
match. Either change the prose phrase "vs the main branch" to "vs the master
branch" to match the existing command, or replace the hardcoded `master` in the
`git diff master...HEAD` command with a parameter/variable (e.g.,
`${BASE_BRANCH}`) and show how to set it (or detect default branch) so the
instruction works across repos. Ensure both the descriptive text ("vs the main
branch") and the command (`git diff ...`) use the same branch identifier.
| Review all changes on the current branch vs the main branch for test quality. | ||
|
|
||
| IMPORTANT: Only review files and lines that appear in the diff (`git diff master...HEAD`). |
There was a problem hiding this comment.
Branch baseline is inconsistent (main vs master).
Line 8 says main, while Line 10 uses master...HEAD. Pick one canonical baseline to avoid drift in review scope.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/agents/test-reviewer.md around lines 8 - 10, The branch baseline
strings are inconsistent—one occurrence uses "main" and another uses
"master...HEAD"; pick a single canonical baseline (e.g., "main") and make both
tokens consistent in the file by replacing "master...HEAD" with "main...HEAD"
(or vice versa if you prefer "master"), and update any adjacent
instructions/comments that reference the baseline so the review scope is
unambiguous.
| ``` | ||
| ## Test Review Report | ||
|
|
||
| ### Missing Coverage (High Priority) | ||
| [Critical untested code paths] | ||
|
|
||
| ### Test Quality Issues | ||
| [Brittle tests, incorrect mocking, isolation problems] | ||
|
|
||
| ### Best Practice Improvements | ||
| [Naming, organization, Go testing conventions] | ||
|
|
||
| ### Summary | ||
| - Coverage gaps: X high, Y medium | ||
| - Quality issues: Z | ||
| - Total recommendations: N | ||
| ``` |
There was a problem hiding this comment.
Add a fenced code language for the output template.
The block at Line 87 is missing a language tag and will fail MD040 checks.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/agents/test-reviewer.md around lines 87 - 103, Add a fenced code
language tag to the output template's code block that starts with the triple
backticks (the block containing "## Test Review Report" etc.) so it satisfies
MD040; update the opening fence (``` ) to include a language identifier (for
example ```md or ```markdown or ```txt) to mark the block's language and prevent
lint failures.
| LINT_OUTPUT=$(make lint 2>&1) || { | ||
| LINT_EXIT=$? | ||
| # Show last 50 lines which typically contain the most relevant errors | ||
| ERROR_SUMMARY=$(echo "$LINT_OUTPUT" | tail -50 | sed 's/\\/\\\\/g; s/"/\\"/g') | ||
| cat <<EOF | ||
| [ | ||
| { | ||
| "role": "user", | ||
| "content": "❌ Linting failed. Recent errors:\n\`\`\`\n${ERROR_SUMMARY}\n\`\`\`\n\nRun 'make lint' to see full output." | ||
| } | ||
| ] | ||
| EOF | ||
| exit 1 | ||
| } | ||
|
|
||
| # Run tests | ||
| TEST_OUTPUT=$(make go-test 2>&1) || { | ||
| TEST_EXIT=$? | ||
| # Show last 100 lines to capture multiple failures and stack traces | ||
| ERROR_SUMMARY=$(echo "$TEST_OUTPUT" | tail -100 | sed 's/\\/\\\\/g; s/"/\\"/g') | ||
| cat <<EOF | ||
| [ | ||
| { | ||
| "role": "user", | ||
| "content": "❌ Tests failed. Recent output:\n\`\`\`\n${ERROR_SUMMARY}\n\`\`\`\n\nRun 'make go-test' to see full output." | ||
| } | ||
| ] | ||
| EOF | ||
| exit 1 | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Simulate the current escaping against multi-line input and validate the JSON with jq.
ERROR_SUMMARY=$(printf 'first line\nsecond "line" with \\backslash\nthird\n' | sed 's/\\/\\\\/g; s/"/\\"/g')
cat <<EOF | jq .
[
{
"role": "user",
"content": "❌ Linting failed. Recent errors:\n\`\`\`\n${ERROR_SUMMARY}\n\`\`\`\n\nRun 'make lint' to see full output."
}
]
EOFRepository: openshift/ocm-agent-operator
Length of output: 184
🏁 Script executed:
#!/bin/bash
# Check if the file exists and read the relevant lines
if [ -f ".claude/hooks/before-commit" ]; then
echo "=== File found, reading lines 13-42 ==="
sed -n '13,42p' .claude/hooks/before-commit
echo ""
echo "=== Full file context ==="
cat -n .claude/hooks/before-commit
else
echo "File not found: .claude/hooks/before-commit"
# Try to locate it
find . -name "before-commit" -type f 2>/dev/null | head -20
fiRepository: openshift/ocm-agent-operator
Length of output: 2511
JSON output is invalid whenever lint or test output spans multiple lines.
Lines 16 and 32 use sed 's/\\/\\\\/g; s/"/\\"/g' which escapes only backslashes and double quotes. The captured multi-line output from tail still contains literal newlines (and possibly tabs/control characters) that are inserted into the "content" string via the heredoc. Per RFC 8259, JSON strings cannot contain unescaped control characters, so strict JSON parsers will reject the payload—meaning the consumer (Claude Code hook runtime) discards the message and the user never sees the failure reason.
Proposed fix using jq for JSON-safe encoding
# Run linting on changed files
LINT_OUTPUT=$(make lint 2>&1) || {
- LINT_EXIT=$?
- # Show last 50 lines which typically contain the most relevant errors
- ERROR_SUMMARY=$(echo "$LINT_OUTPUT" | tail -50 | sed 's/\\/\\\\/g; s/"/\\"/g')
- cat <<EOF
-[
- {
- "role": "user",
- "content": "❌ Linting failed. Recent errors:\n\`\`\`\n${ERROR_SUMMARY}\n\`\`\`\n\nRun 'make lint' to see full output."
- }
-]
-EOF
+ ERROR_SUMMARY=$(printf '%s\n' "$LINT_OUTPUT" | tail -50)
+ jq -n --arg body "$ERROR_SUMMARY" '
+ [{role:"user",
+ content:("❌ Linting failed. Recent errors:\n```\n" + $body + "\n```\n\nRun '\''make lint'\'' to see full output.")}]
+ '
exit 1
}Apply the same pattern to the test branch (lines 29–42). Alternatively, extend the sed pipeline to escape control characters: sed 's/\\/\\\\/g; s/"/\\"/g; s/$/\\n/g' (though this approach is less robust than jq).
Minor: LINT_EXIT and TEST_EXIT (lines 14, 30) are captured but never used.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/hooks/before-commit around lines 13 - 42, The JSON currently built
in the lint and test failure blocks is invalid when outputs contain
newlines/control-chars because the sed pipeline only escapes backslashes and
quotes; replace the sed-based escaping with proper JSON encoding via jq: capture
the raw tail output into LINT_OUTPUT/TEST_OUTPUT as before, set ERROR_SUMMARY
using the tail result (keep the variable names), then build the JSON payload
with jq (for example use jq -n --arg body "$ERROR_SUMMARY" '[{role:"user",
content: "❌ Linting failed. Recent errors:\n```\n"+$body+"\n```\n\nRun '\''make
lint'\'' to see full output."}]') for the lint branch and similarly for the test
branch (use "❌ Tests failed..." and run 'make go-test'); also remove or use the
unused LINT_EXIT/TEST_EXIT variables or delete those assignments.
| 2. **Launch all review agents in parallel** | ||
|
|
||
| Spawn each of the following agents in the background without worktree isolation. These agents are read-only reviewers and do not need isolated worktrees. IMPORTANT: Include the diff output in each agent's prompt and instruct them to ONLY review files and lines that appear in the diff — they must not explore or report on files outside the changeset: | ||
|
|
||
| - @security-reviewer - OWASP Top 10, RBAC, secrets management, input validation | ||
| - @lint-reviewer - Code quality, style, golangci-lint findings | ||
| - @test-reviewer - Test coverage gaps, test quality, Ginkgo/Gomega patterns | ||
| - @error-handling-reviewer - Error wrapping, logging, failure modes | ||
|
|
||
| Additional focus areas if specified: $ARGUMENTS |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Claude Code agent frontmatter isolation worktree override from skill
💡 Result:
In Claude Code (Anthropic's agentic coding tool), "frontmatter isolation worktree override from skill" refers to configuring Git worktree isolation for agents and subagents via YAML frontmatter in agent definition files (.claude/agents/*.md), with known bugs where frontmatter isolation: worktree is ignored in certain invocation methods, requiring explicit overrides, sometimes via skills or tool parameters. Key concepts: - isolation: worktree in agent frontmatter creates a temporary Git worktree for the agent/subagent, providing file isolation for parallel work without conflicts. Worktrees auto-clean if no changes. - Works reliably for subagents spawned via Agent tool. - Bug: Ignored when invoking main agent via claude --agent (runs in main worktree) or plugin agents (edits main tree). - Workaround: Pass isolation: "worktree" explicitly in Agent tool calls; use --worktree CLI flag; invoke via parent session Agent tool. - Skills (/skill-name) are Markdown files in .claude/skills/ with frontmatter; they can reference or trigger worktree usage but do not directly override agent isolation (skills load shared across worktrees). Custom skills like "using-git-worktrees" automate worktree creation/management. Example agent frontmatter: --- name: issue-handler description: Handle GitHub issues in isolated worktree isolation: worktree skills: [handle-issue] --- Official docs confirm isolation field for subagents/agents. Open GitHub issues (#50357, #34775 as of Apr 2026) track frontmatter bugs; no fix confirmed in results.
Citations:
- 1: https://www.stepcodex.com/en/issue/isolation-worktree-frontmatter-not-respected-when
- 2: Plugin agent frontmatter
isolation: worktreenot applied at runtime anthropics/claude-code#34775 - 3: isolation: worktree frontmatter not respected when agent invoked via claude --agent anthropics/claude-code#50357
- 4: [DOCS] Subagents documentation missing
isolation: "worktree"configuration anthropics/claude-code#27023 - 5: https://code.claude.com/docs/en/common-workflows
- 6: https://github.com/philoserf/claude-code-setup/blob/main/references/frontmatter-requirements.md
- 7: https://claudelint.com/api/schemas/agents
🏁 Script executed:
# Find the skill file and read the relevant lines
find . -name "SKILL.md" -path "*full-review*" | head -20Repository: openshift/ocm-agent-operator
Length of output: 110
🏁 Script executed:
# Find the agent files referenced in the review
find . -path "*/.claude/agents/*" -name "*.md" | head -30Repository: openshift/ocm-agent-operator
Length of output: 222
🏁 Script executed:
cat -n ./.claude/skills/full-review/SKILL.md | sed -n '1,30p'Repository: openshift/ocm-agent-operator
Length of output: 1735
🏁 Script executed:
# Check frontmatter of each agent file
for agent in ./.claude/agents/{security-reviewer,lint-reviewer,test-reviewer,error-handling-reviewer}.md; do
echo "=== $agent ==="
head -20 "$agent"
echo ""
doneRepository: openshift/ocm-agent-operator
Length of output: 3269
Skill contradicts the agent frontmatter on worktree isolation.
This skill instructs parallel launches "without worktree isolation" (line 17), but every referenced agent (security-reviewer, lint-reviewer, test-reviewer, error-handling-reviewer) declares isolation: worktree in its frontmatter. Either:
- Remove
isolation: worktreefrom the four agent definitions (if read-only parallel runs without isolation are the intent), or - Reword this skill to respect each agent's declared isolation.
Leaving both as-is creates ambiguity in expected behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/full-review/SKILL.md around lines 15 - 24, The skill text
"Launch all review agents in parallel" conflicts with the agents' frontmatter
which sets isolation: worktree for the agents security-reviewer, lint-reviewer,
test-reviewer, and error-handling-reviewer; pick one fix: either remove
isolation: worktree from those four agent definitions (security-reviewer,
lint-reviewer, test-reviewer, error-handling-reviewer) if you intend true
parallel read-only runs without worktrees, or update the skill wording to
require per-agent worktree isolation (rephrase the parallel step to spawn agents
respecting each agent's isolation: worktree); ensure the chosen approach is
applied consistently across the skill text and the four agent frontmatters so
behavior is unambiguous.
| ### Automated Hooks | ||
|
|
||
| - **user-prompt-submit** - Reminds to run tests after code changes | ||
| - **before-commit** - Automatically runs `make lint` and `make test` before commits |
There was a problem hiding this comment.
Doc drift: hook runs make go-test, not make test.
.claude/hooks/before-commit invokes make go-test. While make test delegates to go-test in boilerplate standard.mk, having the docs match the actual command avoids confusion when readers grep for the exact target.
📝 Proposed fix
-- **before-commit** - Automatically runs `make lint` and `make test` before commits
+- **before-commit** - Automatically runs `make lint` and `make go-test` before commits📝 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.
| - **before-commit** - Automatically runs `make lint` and `make test` before commits | |
| - **before-commit** - Automatically runs `make lint` and `make go-test` before commits |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLAUDE.md` at line 108, Docs mismatch: update the CLAUDE.md entry for the
"before-commit" hook to match the actual hook command; change the text that
currently says "Automatically runs `make test` and `make lint`" to state it runs
`make go-test` (since `.claude/hooks/before-commit` invokes `make go-test`) so
readers searching for the exact make target find the real command.
| ``` | ||
| /full-review | ||
| ``` |
There was a problem hiding this comment.
Add language tags to unlabeled fenced blocks.
Multiple fenced blocks are missing language identifiers and will keep failing MD040. Please label them (bash, text, or markdown) based on content.
Also applies to: 85-89, 104-106, 145-147, 151-153, 174-178, 195-211, 223-251, 253-261, 265-279, 283-295, 299-313, 357-359, 362-364, 379-381, 384-386
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 61-61: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/CLAUDE_CODE_GUIDE.md` around lines 61 - 63, Several fenced code blocks
in CLAUDE_CODE_GUIDE.md lack language identifiers and trigger MD040; update each
unlabeled triple-backtick block (e.g., the block containing "/full-review" and
the other blocks noted in the review) by adding the appropriate language tag
(bash, text, or markdown) based on the snippet content so the linter recognizes
the language and MD040 is satisfied; scan the document for other unlabeled
```...``` blocks (including the ranges called out in the review) and add
consistent language tags to all of them.
| **What it runs:** | ||
| - `make lint` - golangci-lint checks | ||
| - `make test` - Unit tests with coverage | ||
| - `go test -race` - Race condition detection | ||
| - `make go-build` - Build verification | ||
|
|
There was a problem hiding this comment.
Test command names are inconsistent with configured hooks/skills.
These lines document make test, but the configured hook/skill behavior uses make go-test. Please align guide text/examples with actual commands to prevent broken copy-paste workflows.
Proposed doc fix
- - `make test` - Unit tests with coverage
+ - `make go-test` - Unit tests
- • make test - Run unit tests
+ • make go-test - Run unit tests
- 2. `make test` - Tests must pass
+ 2. `make go-test` - Tests must passAlso applies to: 186-188, 175-178
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/CLAUDE_CODE_GUIDE.md` around lines 91 - 96, The documentation lists the
test command as `make test` but the repository hooks/skills expect `make
go-test`; update all occurrences (e.g., the section heading "What it runs" and
the listed bullet `make test` and the other mentions around lines referenced) to
`make go-test` (or add a note that `make test` is an alias if you prefer), and
ensure the examples and any guidance consistently use `make go-test` so
copy-pasted commands match configured hooks like the go-test skill.
| Claude: [Launches all 5 review agents in parallel] | ||
|
|
||
| ## Full Review Report | ||
|
|
||
| ### Security Review | ||
| ✅ No critical issues found | ||
|
|
||
| ### Performance Review | ||
| 🔴 1 high severity: Inefficient API calls in reconcile loop | ||
| [Details and fix suggestions] | ||
|
|
||
| ### Test Review | ||
| 🔴 2 high severity: Missing test coverage for error paths | ||
| [Details and suggested tests] | ||
|
|
||
| ### Error Handling Review | ||
| ⚠️ 3 medium: Missing error wrapping | ||
| [Details] | ||
|
|
||
| ### Concurrency Review | ||
| ✅ No issues found | ||
|
|
||
| --- | ||
| Summary: 3 high, 3 medium issues found | ||
| Estimated fix time: 1-2 hours |
There was a problem hiding this comment.
/full-review output example references non-existent agent categories.
The example claims “all 5 review agents” and includes Performance/Concurrency sections, but the configured agent set is 4 reviewers. This creates incorrect expectations for users.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/CLAUDE_CODE_GUIDE.md` around lines 226 - 250, The example in the
"/full-review" output (under the "## Full Review Report" block attributed to
"Claude") incorrectly states "Launches all 5 review agents" and includes agent
categories not present in the configured set; update this example to match the
actual configured 4-reviewer set by removing or renaming the extra agent
category lines (e.g., drop or combine the Performance/Concurrency sections as
appropriate), adjust the summary counts ("3 high, 3 medium") and the estimated
fix time to reflect the corrected categories, and ensure the header and inline
text no longer reference "5 review agents" so the example is consistent with the
real configuration.
|
@devppratik: 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. |
| - Mock interfaces for testing | ||
|
|
||
| Always run `make` after modifying API types to regenerate required code. No newline at end of file | ||
| Always run `make` after modifying API types to regenerate required code. |
There was a problem hiding this comment.
You could use a stop hook here so you can reduce context bloat. See https://github.com/openshift-online/rosa-claude-plugins/blob/main/.claude/hooks/stop-prek-validation.sh for an example.
|
|
||
| ## Claude Code Integration | ||
|
|
||
| This repository is configured with Claude Code for AI-assisted development. |
There was a problem hiding this comment.
I don't think this is necessary to tell Claude.
|
|
||
| ### Quick Start Skills | ||
|
|
||
| Skills are invoked with the `/` prefix: |
There was a problem hiding this comment.
This reads like it's for a human but is in CLAUDE.md
| - **`/run-tests`** - Runs linting, unit tests, and race detection | ||
| - **`/update-crds`** - Regenerates CRD manifests and deepcopy code after API changes | ||
|
|
||
| ### Review Agents |
There was a problem hiding this comment.
Should these be agent-based stop hooks? https://code.claude.com/docs/en/hooks#prompt-and-agent-hook-fields
|
|
||
| ### Comprehensive Guide | ||
|
|
||
| See [docs/CLAUDE_CODE_GUIDE.md](docs/CLAUDE_CODE_GUIDE.md) for detailed documentation on skills, agents, workflows, and best practices. |
There was a problem hiding this comment.
This is mentioned in the Resources section again.
| - [OSD Operator Development Guide](https://github.com/openshift/ops-sop/blob/master/operators/README.md) | ||
| - [Boilerplate Documentation](https://github.com/openshift/boilerplate) | ||
| - [Operator SDK Documentation](https://sdk.operatorframework.io/) | ||
| - [SREP-4410: Claude Integration Epic](https://issues.redhat.com/browse/SREP-4410) No newline at end of file |
There was a problem hiding this comment.
The git commit for this should have this reference so I don't think we need it in here.
There was a problem hiding this comment.
There's a hooks directory with scripts in it, but neither of those scripts are configured in here.
| @@ -0,0 +1,42 @@ | |||
| --- | |||
| name: full-review | |||
| description: Run all review agents in parallel for comprehensive code quality analysis | |||
There was a problem hiding this comment.
Why not run the coderabbit cli? These seems like configurations that should be in coderabbit and can be run in a pre-push hook locally.
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
4 participants
What type of PR is this?
feature
What this PR does / why we need it?
This PR adds .claude folder with Skills, Agents and Hooks for starting with Agentic SDLC for OAO
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
Documentation
Chores