docs, feat(SREP-4460, SREP-4926: Add Standardized Claude hooks, skill, agents. Update standardised docs)#422
Open
devppratik wants to merge 6 commits into
Open
Conversation
Contributor
|
[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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR establishes comprehensive Claude Code automation infrastructure for the aws-vpce-operator repository, adding five specialized agents (lint, test, security, docs, ci), pre-commit hook integration via prek with gitleaks secret scanning, a Prow CI failure analysis skill with artifact fetching and pattern detection, and developer guides covering contribution, development, and testing workflows. ChangesClaude Code Automation Infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 11❌ Failed checks (11 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
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: 14
🧹 Nitpick comments (1)
hack/ci.sh (1)
9-9: ⚡ Quick winMake CI config path independent of current working directory.
Using a CWD-relative
--configcan break when this script is called from outside repo root. Resolve the repo root from the script path and pass an absolute config path.Proposed fix
#!/usr/bin/env bash set -euo pipefail +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +REPO_ROOT="$(cd "${SCRIPT_DIR}/.." && pwd)" + if ! command -v prek &>/dev/null; then echo "Error: prek is not installed. Install with: uv tool install prek" >&2 exit 1 fi -prek run --config hack/prek.ci.toml --all-files +prek run --config "${REPO_ROOT}/hack/prek.ci.toml" --all-files🤖 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/ci.sh` at line 9, The CI invocation currently uses a CWD-relative config ("--config hack/prek.ci.toml"); make it path-independent by resolving the repo root from the script location and passing an absolute config path to prek. Update the invocation that runs "prek run" to first compute the repo root (e.g., from "${BASH_SOURCE[0]}" or "$0") and join it with "hack/prek.ci.toml", then call "prek run --config <absolute-path-to-hack/prek.ci.toml> --all-files" so the config is found regardless of the current working directory.
🤖 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/ci-agent.md:
- Line 10: Replace the incorrect project name "OCM Agent Operator" with the
correct "aws-vpce-operator" throughout the .claude/agents/ci-agent.md file
(including the occurrence noted around line 177); update any headings, metadata,
and in-document references so the file consistently identifies the project as
aws-vpce-operator and verify there are no remaining instances of the old name.
- Around line 24-30: Update the Tekton pipeline filenames listed (e.g., replace
occurrences of `ocm-agent-operator-pull-request.yaml`,
`ocm-agent-operator-push.yaml`, `ocm-agent-operator-e2e-pull-request.yaml`,
`ocm-agent-operator-e2e-push.yaml`, `ocm-agent-operator-pko-push.yaml`, and
`ocm-agent-operator-pko-pull-request.yaml`) to the correct repo-specific names
using the `aws-vpce-operator-*` prefix so the docs point to existing manifests
(e.g., `aws-vpce-operator-pull-request.yaml`, `aws-vpce-operator-push.yaml`,
`aws-vpce-operator-e2e-pull-request.yaml`, `aws-vpce-operator-e2e-push.yaml`,
`aws-vpce-operator-pko-push.yaml`, `aws-vpce-operator-pko-pull-request.yaml`),
and make the same replacements for the other referenced occurrences (lines noted
in the comment).
- Around line 31-39: The CI detection currently greps .tekton/*.yaml for literal
strings like "golangci-lint", "gitleaks", "go test", and "go build", which
breaks when Tekton tasks are referenced via pipelineRef; update the detection
logic to resolve pipelineRef entries instead of raw string grep: when a manifest
contains a pipelineRef, read its name and search the repo for the corresponding
pipeline definition (e.g., pipelines/<pipeline-name>/pipeline.yaml or any YAML
with metadata.name == <pipelineRef.name>), then inspect that pipeline's
taskRefs/steps for golangci-lint/gitleaks/go test/go build; ensure fallback
behavior still scans inline pipeline specs in .tekton/*.yaml and treat
unresolved pipelineRefs as unknown rather than false positives.
- Around line 55-60: The parity check commands that run grep "rev:"
.pre-commit-config.yaml | grep golangci-lint and the gitleaks variant fail
because the repo name is not on the same line as the rev field; update the check
to first find the hook block for "golangci-lint" (and "gitleaks") and then
extract the subsequent rev value (e.g., locate the line containing the hook
repo/name and read the following indented rev: line or use a YAML-aware lookup),
replacing the current two-stage grep pipelines in .claude/agents/ci-agent.md so
the command reliably prints the hook's rev for both golangci-lint and gitleaks.
In @.claude/agents/docs-agent.md:
- Line 10: Update the intro text that currently says "OCM Agent Operator" to use
the correct repository name `aws-vpce-operator`; locate the string "OCM Agent
Operator" in docs-agent.md (intro section) and replace it with
`aws-vpce-operator`, and scan the file for any other occurrences of "OCM Agent
Operator" or similar variants to keep naming consistent across the document.
- Around line 188-198: The fenced example block in .claude/agents/docs-agent.md
is missing a language tag causing MD040; update the opening fence from ``` to
```text (or another appropriate tag like ```md) for the block that contains
"Updated: DEVELOPMENT.md ... Validated: ..." so the markdown linter recognizes
the code block and leave the closing ``` unchanged.
In @.claude/agents/lint-agent.md:
- Line 10: Update the agent brief text that currently says "OCM Agent Operator"
to use the correct repository/operator name "aws-vpce-operator"; locate the
phrase "OCM Agent Operator" in the agent brief content (the agent
description/title) and replace it with "aws-vpce-operator" so the prompt/context
matches this repo and avoids drift.
In @.claude/agents/security-agent.md:
- Line 10: Update the introductory sentence that currently reads "OCM Agent
Operator" to the correct repository/operator name used in this PR; locate the
phrase "OCM Agent Operator" in the Security Agent intro (around the opening
paragraph in .claude/agents/security-agent.md) and replace it with the actual
repo/operator name used by this PR so the guidance matches the codebase.
In @.claude/agents/test-agent.md:
- Line 10: Replace the incorrect repository name "OCM Agent Operator" in the
Test Agent intro with the current repository/operator name "aws-vpce-operator";
update the string occurrence in the test-agent.md intro so the sentence reads
with "aws-vpce-operator" for consistency across docs.
In @.claude/hooks/README.md:
- Around line 182-183: Update the README description for the stop hook so it
matches the actual hook script: change the text that currently states "prek run
--all-files" to "prek run --config hack/prek.ci.toml" (referencing the stop hook
command in the hooks README and the stop hook script invocation) so
documentation and implementation are consistent.
- Line 3: The README currently mentions "OCM Agent Operator" which is
inconsistent with this repository's name; update the phrasing in the README (the
string "OCM Agent Operator" on the mentioned line) to the correct repository
name "aws-vpce-operator" so contributor docs consistently reference this
project; ensure any other occurrences of "OCM Agent Operator" in the README are
also replaced to avoid copy/paste confusion.
In @.claude/skills/README.md:
- Around line 66-72: The fenced code block in .claude/skills/ README.md (the
directory tree example) lacks a language identifier which triggers markdownlint
MD040; update the opening triple-backtick to include a language tag (e.g.,
```text) for the tree diagram so the block becomes fenced with a language
identifier and satisfies linting, keeping the block content unchanged.
In @.gitleaks.toml:
- Around line 25-32: The allowlist in the paths array currently contains a
global pattern '''.*_test\.go''' which excludes all Go test files from gitleaks;
remove that entry and limit exemptions to specific fixture patterns (e.g., keep
'''test/fixtures/.*''' and any other explicit fixture/deploy patterns you
intentionally want to allow) so only known fixture paths are excluded; update
the paths array to drop the '''.*_test\.go''' symbol and ensure remaining
entries (like '''test/fixtures/.*''', '''test/deploy/.*''') are the only
test-related exemptions.
In `@TESTING.md`:
- Around line 266-275: Remove the contradictory guidance by keeping the YAML
pre-commit snippet as an example/future option and retaining the authoritative
instruction to run tests manually with `make go-test`; specifically, modify the
section containing the `- id: go-test` snippet and the surrounding sentences so
the YAML block is clearly labeled "example (optional / future)" or "not enabled
in current config", and ensure the lone authoritative statement left is "Run
manually before pushing: `make go-test`" so contributors see only one source of
truth.
---
Nitpick comments:
In `@hack/ci.sh`:
- Line 9: The CI invocation currently uses a CWD-relative config ("--config
hack/prek.ci.toml"); make it path-independent by resolving the repo root from
the script location and passing an absolute config path to prek. Update the
invocation that runs "prek run" to first compute the repo root (e.g., from
"${BASH_SOURCE[0]}" or "$0") and join it with "hack/prek.ci.toml", then call
"prek run --config <absolute-path-to-hack/prek.ci.toml> --all-files" so the
config is found regardless of the current working directory.
🪄 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: a04a178b-9caa-4d59-b033-cd34acd01279
📒 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
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #422 +/- ##
=======================================
Coverage 40.69% 40.69%
=======================================
Files 32 32
Lines 2150 2150
=======================================
Hits 875 875
Misses 1171 1171
Partials 104 104 🚀 New features to boost your workflow:
|
devppratik
force-pushed
the
add-claude-and-docs
branch
2 times, most recently
from
17b785f to
917afb6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
.claude/agents/docs-agent.md (1)
188-198:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language tag to the fenced output block.
This block still lacks a fence language and can fail markdown lint checks (MD040).
Proposed fix
-``` +```text Updated: DEVELOPMENT.md - Added section on new make target: go-bench - Fixed typo in test commands - Updated Go version requirement: 1.22.7 -> 1.24.0 @@ - All command examples tested - Links checked</details> <details> <summary>🤖 Prompt for AI Agents</summary>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/agents/docs-agent.md around lines 188 - 198, The fenced output block
in .claude/agents/docs-agent.md is missing a language tag which triggers MD040;
update the opening fence fromtotext (or another appropriate tag likewith a language identifier, keeping the content unchanged and leaving the closing ``` as-is..claude/hooks/README.md (2)
3-3:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRepository name is inconsistent in docs.
Line 3 still says “OCM Agent Operator”; this should be
aws-vpce-operatorto avoid copy/paste confusion in contributor-facing 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/hooks/README.md at line 3, Update the repository name in the hooks README by replacing the incorrect string "OCM Agent Operator" with the correct repository name "aws-vpce-operator" in the README.md content (look for the sentence beginning "Security and validation hooks for ...") so contributor-facing docs reflect the correct project and avoid copy/paste confusion.
182-183:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStop-hook command description conflicts with the rest of this README.
Line 182 says the stop hook runs
prek run --all-files, but Line 72 documentsprek run --config hack/prek.ci.toml. Please make these consistent with actual hook behavior.🤖 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 around lines 182 - 183, README contains conflicting descriptions for the stop hook: one place says the stop hook runs "prek run --all-files" while another documents "prek run --config hack/prek.ci.toml"; update the stop-hook description to match the actual hook implementation by replacing the incorrect command with the real one (reference the "stop hook" entry and the exact command string "prek run --all-files" or "prek run --config hack/prek.ci.toml" as appropriate) so both the stop hook section and the earlier command listing use the same command string and wording.
🤖 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/README.md:
- Line 3: Update the intro sentence in the README titled line "Specialized
agents for this operator development workflows." to read "Specialized agents for
this operator’s development workflows." so the grammar and possessive are
corrected; locate the sentence in .claude/agents/README.md and replace the
existing phrase with the revised wording.
In @.claude/hooks/cleanup.sh:
- Around line 3-4: Replace the incorrect repository name in the header comment
at the top of .claude/hooks/cleanup.sh: change the phrase "OCM Agent Operator"
to "aws-vpce-operator" so the file header accurately reflects this repository.
---
Duplicate comments:
In @.claude/agents/docs-agent.md:
- Around line 188-198: The fenced output block in .claude/agents/docs-agent.md
is missing a language tag which triggers MD040; update the opening fence from
``` to ```text (or another appropriate tag like ```diff if you prefer diff
semantics) so the block becomes a fenced code block with a language identifier,
keeping the content unchanged and leaving the closing ``` as-is.
In @.claude/hooks/README.md:
- Line 3: Update the repository name in the hooks README by replacing the
incorrect string "OCM Agent Operator" with the correct repository name
"aws-vpce-operator" in the README.md content (look for the sentence beginning
"Security and validation hooks for ...") so contributor-facing docs reflect the
correct project and avoid copy/paste confusion.
- Around line 182-183: README contains conflicting descriptions for the stop
hook: one place says the stop hook runs "prek run --all-files" while another
documents "prek run --config hack/prek.ci.toml"; update the stop-hook
description to match the actual hook implementation by replacing the incorrect
command with the real one (reference the "stop hook" entry and the exact command
string "prek run --all-files" or "prek run --config hack/prek.ci.toml" as
appropriate) so both the stop hook section and the earlier command listing use
the same command string and wording.
🪄 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: 3f91ded8-0e69-486e-ace5-5c89ee898c50
📒 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
- hack/ci.sh
- .claude/agents/security-agent.md
- DEVELOPMENT.md
- .claude/agents/test-agent.md
- hack/prek.ci.toml
- prek.toml
- .claude/agents/lint-agent.md
- CONTRIBUTING.md
🚧 Files skipped from review as they are similar to previous changes (6)
- TESTING.md
- .claude/agents/ci-agent.md
- .claude/hooks/stop-prek-validation.sh
- .claude/settings.json
- .gitleaks.toml
- .claude/hooks/pre-edit.sh
devppratik
force-pushed
the
add-claude-and-docs
branch
2 times, most recently
from
59cb98e to
2e629e2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.claude/agents/docs-agent.md (1)
44-45: ⚡ Quick winBroaden markdown validation commands beyond repo-root
*.md.Current examples only scan top-level markdown files, so nested docs can be skipped during validation.
Based on learnings: Run pre-commit run --all-files before submitting a PR.Proposed command updates
- grep '```bash' -A 10 *.md | grep '^make\|^go\|^ginkgo' + find . -name "*.md" -not -path "./vendor/*" -not -path "./.git/*" -print0 \ + | xargs -0 grep -nE '```bash|^(make|go|ginkgo)\b' - grep -E '```$' *.md # Code blocks without language - grep -E '\[.*\]\(\./' *.md # Relative links to check + find . -name "*.md" -not -path "./vendor/*" -not -path "./.git/*" -print0 \ + | xargs -0 grep -nE '```$' # Code blocks without language + find . -name "*.md" -not -path "./vendor/*" -not -path "./.git/*" -print0 \ + | xargs -0 grep -nE '\[.*\]\(\./' # Relative links to check - grep '```bash' *.md | grep 'make ' | sed 's/.*make \([a-z-]*\).*/\1/' | sort -u + find . -name "*.md" -not -path "./vendor/*" -not -path "./.git/*" -print0 \ + | xargs -0 grep -hE '```bash|make ' \ + | sed -n 's/.*make \([a-zA-Z0-9._-]*\).*/\1/p' \ + | sort -uAlso applies to: 59-60, 176-183
🤖 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/agents/docs-agent.md around lines 44 - 45, Replace the repo-root-only markdown grep invocations (e.g., the commands matching "grep '```bash' -A 10 *.md", "grep -E '```$' *.md", "grep -E '\\[.*\\]\\(\\.\\/ ' *.md", and "grep '```bash' *.md | grep 'make '") with recursive find+xargs pipelines that search all *.md under the tree while excluding ./vendor and .git (so nested docs aren't skipped); update the three affected blocks (the grep '```bash' block, the code-block/relative-link checks, and the make-target extraction pipeline) accordingly and apply the same changes at the other occurrences noted (lines around 59-60 and 176-183), and ensure the docs recommend running "pre-commit run --all-files" before submitting a PR.
🤖 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/skills/prow-ci/SKILL.md:
- Around line 113-116: Replace the explicit pre-commit invocation with the
repo-standard wrapper command: change the line containing "pre-commit run
--all-files" to use "prek" so the validation flow is consistent with this PR;
update the SKILL.md section that currently shows the commands (the lines with
"make go-check" and "pre-commit run --all-files") to instead show "make
go-check" followed by "prek".
---
Nitpick comments:
In @.claude/agents/docs-agent.md:
- Around line 44-45: Replace the repo-root-only markdown grep invocations (e.g.,
the commands matching "grep '```bash' -A 10 *.md", "grep -E '```$' *.md", "grep
-E '\\[.*\\]\\(\\.\\/ ' *.md", and "grep '```bash' *.md | grep 'make '") with
recursive find+xargs pipelines that search all *.md under the tree while
excluding ./vendor and .git (so nested docs aren't skipped); update the three
affected blocks (the grep '```bash' block, the code-block/relative-link checks,
and the make-target extraction pipeline) accordingly and apply the same changes
at the other occurrences noted (lines around 59-60 and 176-183), and ensure the
docs recommend running "pre-commit run --all-files" before submitting a PR.
🪄 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: 1b1c2e67-4748-4e3e-8ea8-0b0ee579a1a0
📒 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 (8)
- .claude/hooks/cleanup.sh
- .prek-version
- DEVELOPMENT.md
- .claude/agents/test-agent.md
- .claude/agents/lint-agent.md
- CONTRIBUTING.md
- TESTING.md
- .claude/agents/ci-agent.md
🚧 Files skipped from review as they are similar to previous changes (7)
- hack/ci.sh
- .claude/settings.json
- .claude/agents/security-agent.md
- .claude/hooks/pre-edit.sh
- hack/prek.ci.toml
- prek.toml
- .gitleaks.toml
devppratik
force-pushed
the
add-claude-and-docs
branch
from
70292d8 to
d7b3241
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
.claude/agents/README.md (1)
3-3:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix grammar in the intro sentence.
The possessive form is missing. Change "this operator development workflows" to "this operator's development workflows".
✏️ Suggested fix
-Specialized agents for this operator development workflows. +Specialized agents for this operator's development workflows.🤖 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/agents/README.md at line 3, The intro sentence in .claude/agents/README.md has incorrect grammar; update the phrase "this operator development workflows" to the possessive form "this operator's development workflows" so the sentence reads correctly and conveys ownership..claude/agents/docs-agent.md (1)
188-198:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd language tag to fenced code block.
The code block is missing a language identifier, which violates markdown linting rules (MD040).
🔧 Suggested fix
-``` +```text Updated: DEVELOPMENT.md - Added section on new make target: go-bench - Fixed typo in test commands - Updated Go version requirement: 1.22.7 -> 1.24.0 Validated: - All make targets exist and work - All command examples tested - Links checked</details> <details> <summary>🤖 Prompt for AI Agents</summary>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/agents/docs-agent.md around lines 188 - 198, The fenced code block
using triple backticks around the changelog snippet is missing a language tag
which triggers MD040; update the opening fence fromtotext (or another
appropriate language like markdown) so the block reads with a language
identifier and keep the closing ``` as-is; locate the fence surrounding the
lines starting with "Updated: DEVELOPMENT.md" and change the opener to include
the language tag.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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/stop-prek-validation.sh:
- Line 30: Guard the cd so a failed directory change aborts validation: after
the cd "$REPO_ROOT" in stop-prek-validation.sh check its exit status and if it
fails emit the same styled error message used elsewhere in this script and exit
non‑zero (fail closed) so validation cannot run in the wrong directory; ensure
you follow the existing error formatting/pattern when reporting the failure.- Around line 53-80: The prek invocation currently stored in PREK_OUTPUT runs
without file args and thus only checks the staged set; replace the PREK_OUTPUT
assignment that calls "prek run --config hack/prek.ci.toml 2>&1" with a call
that includes explicit file coverage (e.g. "prek run --config hack/prek.ci.toml
--all-files 2>&1") so unstaged/untracked edits are validated, keeping the same
PREK_OUTPUT variable and error capture semantics.In @.claude/skills/prow-ci/fetch_prow_artifacts.py:
- Around line 96-118: In fetch_junit_xml, remove the unused gcs_path variable
and fix how GCS object URIs are constructed: either change the gcloud list to
return full URIs (use --uri or --format=value(uri()) so junit_files already
contain gs://...), or keep the current name output and join each listed object
to gcs_base_path to build a proper "gs:///" before calling
download_from_gcs; ensure download_from_gcs is passed a single valid gs:// URI
rather than prepending "gs://" to an already-absolute value.In @.claude/skills/prow-ci/SKILL.md:
- Line 7: The document title currently reads "Rbac Permissions Operator" but the
skill is for aws-vpce-operator; update the top-level heading text (the Markdown
header that currently contains "Rbac Permissions Operator") to the correct
operator name "aws-vpce-operator" so the file title matches the documented
skill.- Around line 76-80: The README contains a hardcoded absolute developer path in
the example cd command; change it to a repo-relative path (or a git-root
relative reference) so others can run it. Update the example that references
".claude/skills/prow-ci" and the invocation of fetch_prow_artifacts.py to use a
relative cd (e.g., cd .claude/skills/prow-ci) or document using the repository
root (git rev-parse --show-toplevel) instead of "/Users/ppanda/…", and ensure
the example still shows running python3 fetch_prow_artifacts.py ""
-o .work/prow-artifacts.- Line 69: The fenced code block that contains the example URL in SKILL.md is
missing a language identifier which triggers MD040; update the fenced block that
currently wraps the URL (the example-URL fence) to include a language tag such
as "text" (i.e., changetotext) so the block is explicitly marked and
the linter warning is resolved.
Duplicate comments:
In @.claude/agents/docs-agent.md:
- Around line 188-198: The fenced code block using triple backticks around the
changelog snippet is missing a language tag which triggers MD040; update the
opening fence fromtotext (or another appropriate language like
markdown) so the block reads with a language identifier and keep the closing ```
as-is; locate the fence surrounding the lines starting with "Updated:
DEVELOPMENT.md" and change the opener to include the language tag.In @.claude/agents/README.md:
- Line 3: The intro sentence in .claude/agents/README.md has incorrect grammar;
update the phrase "this operator development workflows" to the possessive form
"this operator's development workflows" so the sentence reads correctly and
conveys ownership.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Repository: openshift/coderabbit/.coderabbit.yaml **Review profile**: CHILL **Plan**: Enterprise **Run ID**: `b81f78f8-7119-409e-897f-fa6144c133a6` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 59cb98ea6ff58ac3e54c6f9165a97d27b718bbbf and 70292d83c5e61b6e06a30c0ced40e8b6862354dd. </details> <details> <summary>📒 Files selected for processing (24)</summary> * `.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` * `.claude/skills/prow-ci/analyze_failure.py` * `.claude/skills/prow-ci/fetch_prow_artifacts.py` * `.gitignore` * `.gitleaks.toml` * `.prek-version` * `CONTRIBUTING.md` * `DEVELOPMENT.md` * `TESTING.md` * `hack/ci.sh` * `hack/prek.ci.toml` * `prek.toml` </details> <details> <summary>✅ Files skipped from review due to trivial changes (10)</summary> * .prek-version * .gitignore * .claude/agents/lint-agent.md * .claude/settings.json * DEVELOPMENT.md * .claude/agents/security-agent.md * prek.toml * CONTRIBUTING.md * .claude/agents/test-agent.md * .claude/agents/ci-agent.md </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (5)</summary> * .claude/hooks/cleanup.sh * hack/ci.sh * .gitleaks.toml * TESTING.md * hack/prek.ci.toml </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
devppratik
force-pushed
the
add-claude-and-docs
branch
from
d7b3241 to
37ee90f
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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:
- Around line 182-183: The README entry under the "On every turn" stop-hook is
out of sync with the actual stop hook invocation; update the docs or the hook so
they match: either change the README's documented command for the stop-hook from
the old `prek run --all-files` text to the actual invocation `prek run --config
hack/prek.ci.toml`, or modify the stop-hook implementation to invoke `prek run
--all-files` instead; ensure the "On every turn" README line and the stop hook
implementation use the exact same command string.
In @.claude/skills/prow-ci/analyze_failure.py:
- Around line 22-24: The current loop skips the root testsuite and double-counts
nested testcases; change the iteration to first include root when root.tag ==
'testsuite' and then add all descendant suites, e.g. build a suites list
starting with root if it's a testsuite and extend it with
root.findall('.//testsuite'), then for each testsuite (variable name testsuite)
use testsuite.findall('testcase') (direct children) instead of './/testcase' to
avoid double-counting nested suites; keep suite_name and testcase handling the
same.
- Around line 11-18: The JUnit XML parser in parse_junit_xml currently imports
xml.etree.ElementTree (ET) — replace it with a hardened parser from defusedxml
to mitigate XXE risks: change the import to use defusedxml.ElementTree (or
defusedxml.ElementTree as ET) and ensure parse_junit_xml continues calling
ET.parse(xml_file) and subsequent ET APIs unchanged; also add defusedxml to the
project dependencies/manifests so the import resolves.
In @.claude/skills/prow-ci/fetch_prow_artifacts.py:
- Around line 122-139: main() currently prints failure messages but still
returns 0 even when fetch_prowjob_json or fetch_build_log fail; update main() to
track success and return a non-zero exit code when required artifacts are
missing by checking the prowjob variable and the boolean result of
fetch_build_log (use prowjob and fetch_build_log(parsed['gcs_base_path'],
output_dir)), set an error flag or cumulative success boolean, and return e.g. 1
if any required download failed so automation sees the failure.
🪄 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: 9fffc63d-c0f7-40a5-81a0-b85de30bf95b
📒 Files selected for processing (24)
.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.claude/skills/prow-ci/analyze_failure.py.claude/skills/prow-ci/fetch_prow_artifacts.py.gitignore.gitleaks.toml.prek-versionCONTRIBUTING.mdDEVELOPMENT.mdTESTING.mdhack/ci.shhack/prek.ci.tomlprek.toml
✅ Files skipped from review due to trivial changes (8)
- .gitignore
- CONTRIBUTING.md
- .prek-version
- TESTING.md
- .claude/agents/lint-agent.md
- .claude/agents/security-agent.md
- .claude/agents/test-agent.md
- .claude/agents/ci-agent.md
🚧 Files skipped from review as they are similar to previous changes (7)
- .claude/hooks/cleanup.sh
- hack/ci.sh
- DEVELOPMENT.md
- .claude/settings.json
- prek.toml
- .claude/hooks/pre-edit.sh
- hack/prek.ci.toml
devppratik
force-pushed
the
add-claude-and-docs
branch
2 times, most recently
from
0f791ef to
c634cbe
Compare
devppratik
force-pushed
the
add-claude-and-docs
branch
from
c634cbe to
dd05bc0
Compare
- Update settings.json with prek permissions and comprehensive deny rules - Update hooks (stop-prek-validation.sh, pre-edit.sh, README.md) - Update agents with repo-specific references - Update skills with repo-specific prow-ci examples - Ensure all references are operator-specific Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
Author
|
/label tide/merge-method-squash |
openshift-ci
Bot
added
the
tide/merge-method-squash
- Make pre-edit.sh canonicalization portable across GNU/BSD/macOS - Add python fallback for path normalization - Works with non-existent files - Fix stop-prek-validation.sh to explicitly validate changed files - Prevents validation scope ambiguity - Validates staged + unstaged + untracked files - Complete prow-ci SKILL.md title with operator name Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (4)
.claude/skills/prow-ci/analyze_failure.py (2)
23-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid duplicate counting of nested testcases.
testsuite.findall('.//testcase')over root+nested suites can count the same testcase multiple times. Iterate direct testcase children per suite.Suggested fix
- for testsuite in suites: + for testsuite in suites: suite_name = testsuite.get('name', 'unknown') - for testcase in testsuite.findall('.//testcase'): + for testcase in testsuite.findall('testcase'): test_name = testcase.get('name', 'unknown') classname = testcase.get('classname', '')🤖 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/prow-ci/analyze_failure.py around lines 23 - 29, The current loop over suites uses testsuite.findall('.//testcase'), which causes nested testcases to be counted multiple times; update the iteration to only gather direct testcase children by replacing testsuite.findall('.//testcase') with testsuite.findall('testcase') so each testsuite processes its immediate testcase elements once (references: the variables/functions suites, testsuite, and testcase in analyze_failure.py).
11-12:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse a hardened XML parser for untrusted JUnit artifacts.
The script parses external XML from Prow artifacts with
xml.etree.ElementTree; switch todefusedxmlto reduce XML parser attack surface.Suggested fix
-import xml.etree.ElementTree as ET +from defusedxml import ElementTree as ET🤖 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/prow-ci/analyze_failure.py around lines 11 - 12, Replace the use of the vulnerable xml.etree.ElementTree parser with the hardened defusedxml parser: change the import of xml.etree.ElementTree to import defusedxml.ElementTree (aliasing it as ET) so all existing uses (e.g., any calls to ET.parse, ET.fromstring, ET.ElementTree) in analyze_failure.py now use the defusedxml implementation; no other logic changes are needed beyond updating the import..claude/skills/prow-ci/fetch_prow_artifacts.py (1)
126-143:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReturn non-zero when required artifact downloads fail.
The script currently reports failures but still exits 0, which masks broken fetches in automation flows.
Suggested fix
- # Fetch prowjob.json + success = True + + # Fetch prowjob.json print("Fetching prowjob.json...") prowjob = fetch_prowjob_json(parsed['gcs_base_path'], output_dir) if prowjob: print("✓ prowjob.json downloaded") else: print("✗ Could not fetch prowjob.json") + success = False # Fetch build-log.txt print("Fetching build-log.txt...") if fetch_build_log(parsed['gcs_base_path'], output_dir): print("✓ build-log.txt downloaded") else: print("✗ Could not fetch build-log.txt") + success = False print(f"\nArtifacts saved to: {output_dir}") - return 0 + return 0 if success else 1🤖 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/prow-ci/fetch_prow_artifacts.py around lines 126 - 143, The exit code always returns 0 even when artifact downloads fail; update the final block that calls fetch_prowjob_json and fetch_build_log so the function returns a non-zero exit code when either download fails. Specifically, after calling fetch_prowjob_json(parsed['gcs_base_path'], output_dir) and fetch_build_log(parsed['gcs_base_path'], output_dir), propagate failure by returning a non-zero integer (e.g., return 1) if prowjob is falsy or if fetch_build_log returned False; otherwise return 0. Ensure you reference the existing symbols fetch_prowjob_json, fetch_build_log and the existing return 0..gitleaks.toml (1)
25-32:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove global
_test.goexemption from gitleaks allowlist.Line 28 excludes all Go tests from scanning, which is too broad and weakens secret detection coverage.
Suggested fix
paths = [ '''test/fixtures/.*''', '''test/deploy/.*''', - '''.*_test\.go''', '''boilerplate/.*''', '''vendor/.*''', '''zz_generated\..*\.go''', ]Based on learnings: Never commit API keys, tokens, passwords, AWS credentials, kubeconfig files, private keys, certificates,
.envfiles with secrets, or debug statements printing sensitive data.🤖 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, The allowlist entry '''.*_test\.go''' inside the paths array is too broad and exempts all Go test files from gitleaks scanning; remove that pattern (or replace it with narrow, explicit test-directory patterns) from the paths list so Go test files are scanned again, keeping the other allowed patterns (e.g., '''test/fixtures/.*''', '''test/deploy/.*''') intact.
🤖 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.
Duplicate comments:
In @.claude/skills/prow-ci/analyze_failure.py:
- Around line 23-29: The current loop over suites uses
testsuite.findall('.//testcase'), which causes nested testcases to be counted
multiple times; update the iteration to only gather direct testcase children by
replacing testsuite.findall('.//testcase') with testsuite.findall('testcase') so
each testsuite processes its immediate testcase elements once (references: the
variables/functions suites, testsuite, and testcase in analyze_failure.py).
- Around line 11-12: Replace the use of the vulnerable xml.etree.ElementTree
parser with the hardened defusedxml parser: change the import of
xml.etree.ElementTree to import defusedxml.ElementTree (aliasing it as ET) so
all existing uses (e.g., any calls to ET.parse, ET.fromstring, ET.ElementTree)
in analyze_failure.py now use the defusedxml implementation; no other logic
changes are needed beyond updating the import.
In @.claude/skills/prow-ci/fetch_prow_artifacts.py:
- Around line 126-143: The exit code always returns 0 even when artifact
downloads fail; update the final block that calls fetch_prowjob_json and
fetch_build_log so the function returns a non-zero exit code when either
download fails. Specifically, after calling
fetch_prowjob_json(parsed['gcs_base_path'], output_dir) and
fetch_build_log(parsed['gcs_base_path'], output_dir), propagate failure by
returning a non-zero integer (e.g., return 1) if prowjob is falsy or if
fetch_build_log returned False; otherwise return 0. Ensure you reference the
existing symbols fetch_prowjob_json, fetch_build_log and the existing return 0.
In @.gitleaks.toml:
- Around line 25-32: The allowlist entry '''.*_test\.go''' inside the paths
array is too broad and exempts all Go test files from gitleaks scanning; remove
that pattern (or replace it with narrow, explicit test-directory patterns) from
the paths list so Go test files are scanned again, keeping the other allowed
patterns (e.g., '''test/fixtures/.*''', '''test/deploy/.*''') intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 6ebb6e58-7af1-489d-a6e4-1995ec5ca987
📒 Files selected for processing (23)
.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/pre-edit.sh.claude/hooks/stop-prek-validation.sh.claude/settings.json.claude/skills/README.md.claude/skills/prow-ci/SKILL.md.claude/skills/prow-ci/analyze_failure.py.claude/skills/prow-ci/fetch_prow_artifacts.py.gitignore.gitleaks.toml.prek-versionCONTRIBUTING.mdDEVELOPMENT.mdTESTING.mdhack/ci.shhack/prek.ci.tomlprek.toml
✅ Files skipped from review due to trivial changes (13)
- .gitignore
- .prek-version
- .claude/skills/README.md
- TESTING.md
- DEVELOPMENT.md
- prek.toml
- .claude/agents/security-agent.md
- .claude/agents/README.md
- .claude/agents/test-agent.md
- .claude/agents/docs-agent.md
- .claude/agents/ci-agent.md
- .claude/hooks/README.md
- .claude/agents/lint-agent.md
- Copy latest analyze_failure.py and fetch_prow_artifacts.py from AAO - Fix gitleaks: narrow test file allowlist to testdata dirs - Remove broad .*_test.go pattern that disabled scanning for all test files
devppratik
commented
Jun 3, 2026
Changes: - Added SessionStart hook to .claude/settings.json that runs session-start-prek-setup.sh - Copied session-start-prek-setup.sh hook script to .claude/hooks/ - Updated docs-agent.md to include .claude/settings.json in sync triggers The SessionStart hook ensures prek is installed and configured when Claude Code starts a session, improving developer experience. Related: CodeRabbit review feedback on hook configuration documentation
Contributor
Author
|
/test validate |
Contributor
|
@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. |
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
3 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.
What type of PR is this?
docs/feature
What this PR does / why we need it?
References
Summary by CodeRabbit
Documentation
New Features
Configuration