docs, feat(SREP-4460, SREP-4926: Add Standardized Claude hooks, skill, agents. Update standardised docs)#750
Open
devppratik wants to merge 6 commits into
Open
Conversation
|
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 a comprehensive Claude Code integration infrastructure for the addon-operator repository. It defines agent specifications for automated lint, test, security, and documentation workflows; configures pre-commit validation with gitleaks secret scanning; implements a Prow CI failure analysis skill with artifact fetching and log analysis; sets up lifecycle hooks for safe edits and stop-time validation; and provides development guides covering setup, testing patterns, and contribution workflows. ChangesClaude Code Integration & Development Automation
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5 | ❌ 10❌ Failed checks (10 inconclusive)
✅ Passed checks (5 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 |
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 |
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: 9
🧹 Nitpick comments (3)
.claude/agents/test-agent.md (1)
29-40: ⚡ Quick winClarify the git diff scope for test selection.
The test selection logic uses
git diff --name-only HEAD, which shows unstaged changes in the working directory. For a PR workflow, you likely want changed files in the branch, such asgit diff --name-only origin/main...HEADorgit diff --name-only --cachedfor staged changes.Please verify this matches the intended test selection strategy.
💡 Suggested alternative for PR-based test selection
# Changed Go files -CHANGED_FILES=$(git diff --name-only HEAD | grep "\.go$") +CHANGED_FILES=$(git diff --name-only origin/main...HEAD | grep "\.go$")🤖 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/test-agent.md around lines 29 - 40, The git diff scope currently uses CHANGED_FILES=$(git diff --name-only HEAD) which picks up unstaged working-tree changes; update the command that sets CHANGED_FILES to use the intended PR/branch scope (e.g., compare branch against main with git diff --name-only origin/main...HEAD for PR-based changed files, or use --cached if you want staged changes) so the PACKAGES and subsequent for loop that runs go test -v ./$pkg/... only run for the correct set of changed Go files; replace the CHANGED_FILES assignment accordingly and keep the downstream PACKAGES and for pkg loop unchanged..claude/agents/docs-agent.md (1)
186-198: ⚡ Quick winAdd language tag to code block in output format example.
The docs-agent instructs others to always specify language tags for code blocks (line 98: "Use
bashfor code blocks, notshorshell"), but the output format example at lines 188-198 uses a fenced code block without a language specifier.📝 Proposed fix
## Output Format When updating docs, report: -``` +```text Updated: DEVELOPMENT.md - Added section on new make target: go-bench - Fixed typo in test commands🤖 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 186 - 198, Update the output format example's fenced code block that shows the docs update (the block containing "Updated: DEVELOPMENT.md ...") to include the language tag recommended by the docs-agent ("Use `bash` for code blocks, not `sh` or `shell`"); replace the current opening fence with one that specifies bash (e.g., change the fence token before "Updated: DEVELOPMENT.md" from ``` to ```bash) so the example follows the stated guideline..claude/skills/prow-ci/SKILL.md (1)
7-7: 💤 Low valueConsider using full repository name in title.
The title "Prow CI Access for Addon" uses the abbreviated "Addon". Consider "Prow CI Access for addon-operator" for clarity and consistency with line 3 in the frontmatter.
📝 Suggested improvement
-# Prow CI Access for Addon +# Prow CI Access for addon-operator🤖 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/SKILL.md at line 7, Change the top-level title string "Prow CI Access for Addon" to use the full repository name to match the frontmatter (e.g., replace with "Prow CI Access for addon-operator"); update only the header text that currently reads "Prow CI Access for Addon" so it consistently references the full repo name used elsewhere.
🤖 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:
- Around line 23-29: The listed Tekton YAMLs in the ci-agent documentation are
incorrect: replace the non-existent ocm-agent-operator-* entries
(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, ocm-agent-operator-pko-pull-request.yaml) with
the actual pipeline filenames used in the repo and remove the e2e entries;
update ci-agent.md to reference addon-operator-pull-request.yaml,
addon-operator-push.yaml, addon-operator-pko-pull-request.yaml, and
addon-operator-pko-push.yaml and adjust any validation or lookup logic that
expects the old six-file set to instead check for these four files.
In @.claude/hooks/README.md:
- Around line 181-183: The README claim that the stop hook runs "prek run
--all-files" is incorrect; update the documentation to reflect the actual stop
hook command used by the script (prek run --config hack/prek.ci.toml). Edit the
section describing the stop hook (referencing "stop hook" and the current README
lines mentioning "prek run --all-files") so it states the actual command "prek
run --config hack/prek.ci.toml" and briefly note that it uses the
hack/prek.ci.toml config rather than running all files.
In @.claude/settings.json:
- Around line 44-55: The repo only wires a Stop hook; to enforce pre-edit
blocking add a hooks.PreToolUse command hook in .claude/settings.json that
targets the file-edit tools (Edit/Write/MultiEdit) and invokes
.claude/hooks/pre-edit.sh; update the hook invocation so the target file path is
passed correctly (either by wrapping the command to forward the tool input as
the first arg or by changing pre-edit.sh to read the tool input from stdin), and
ensure the wrapper or script extracts the file path and exits non‑zero on
failure (reference hooks.PreToolUse, .claude/hooks/pre-edit.sh, and the
Edit/Write/MultiEdit tools).
In @.claude/skills/prow-ci/SKILL.md:
- Around line 69-83: Update .claude/skills/prow-ci/SKILL.md to correct the
Tekton pipeline name: replace the non-existent reference to
addon-operator-e2e-pull-request with the actual pipeline filename(s) found in
.tekton/ (e.g., addon-operator-pull-request, addon-operator-pko-pull-request,
addon-operator-push) or add a brief note pointing to where the E2E pipeline is
defined; also leave the Prow job list as-is but keep the “configured in
openshift/release” qualifier so readers know those jobs aren’t in this repo.
- Around line 84-128: The SKILL.md entry references a non-existent make target
"e2e-binary-build"; update the document to either use the actual E2E binary
build make target that exists in the repo (replace "make e2e-binary-build" with
the correct target name) or add a corresponding make rule named
"e2e-binary-build" to the repo; look for the other existing targets referenced
in the file ("go-test", "go-check", "validate", "coverage", "docker-build") to
match style and naming when choosing the correct replacement or when adding the
new rule.
In @.claude/skills/README.md:
- Line 66: Update the fenced code block in .claude/skills/README.md that shows
the directory structure to include a language specifier (e.g., change the
opening "```" to "```text") so the block for the directory tree renders and
lints correctly; locate the directory structure snippet in README.md and modify
its opening fence accordingly.
In @.gitleaks.toml:
- Around line 25-32: The gitleaks config is too permissive because the paths
array contains the pattern '.*_test\.go' which excludes all Go test files;
change the 'paths' entry by removing or narrowing that pattern and instead
whitelist only explicit test directories or file-glob anchors (e.g., limit to
specific directories like test/fixtures or test/deploy or use anchored regexes
that target only those paths) so that only intended fixture/deploy test files
are exempted; update the 'paths' array entry that currently contains
'.*_test\.go' accordingly.
In `@hack/ci.sh`:
- Line 9: The command "prek run --config hack/prek.ci.toml --all-files" relies
on the current working directory; change it to compute the repository root from
the script location and pass an absolute config path instead. In the hack/ci.sh
script, determine REPO_ROOT (e.g., from the script dirname/parent) and replace
the literal "hack/prek.ci.toml" in the "prek run --config hack/prek.ci.toml
--all-files" invocation with "$REPO_ROOT/hack/prek.ci.toml" (or equivalent
absolute path variable) so the config is resolved independent of cwd.
In `@TESTING.md`:
- Around line 264-274: Update the "Pre-commit Integration" section in TESTING.md
to remove the contradictory example and make the status explicit: start with a
sentence that tests are NOT run by pre-commit hooks (too slow), then instruct
readers to run `make go-test` manually before pushing, add a brief rationale
that the full suite takes 2+ minutes and pre-commit is reserved for fast checks,
and optionally note how to enable it locally by adding the shown go-test hook
snippet to .pre-commit-config.yaml; edit the "Pre-commit Integration" header and
body so the YAML example is presented only as an optional local enablement and
the main guidance clearly states tests are not run automatically.
---
Nitpick comments:
In @.claude/agents/docs-agent.md:
- Around line 186-198: Update the output format example's fenced code block that
shows the docs update (the block containing "Updated: DEVELOPMENT.md ...") to
include the language tag recommended by the docs-agent ("Use `bash` for code
blocks, not `sh` or `shell`"); replace the current opening fence with one that
specifies bash (e.g., change the fence token before "Updated: DEVELOPMENT.md"
from ``` to ```bash) so the example follows the stated guideline.
In @.claude/agents/test-agent.md:
- Around line 29-40: The git diff scope currently uses CHANGED_FILES=$(git diff
--name-only HEAD) which picks up unstaged working-tree changes; update the
command that sets CHANGED_FILES to use the intended PR/branch scope (e.g.,
compare branch against main with git diff --name-only origin/main...HEAD for
PR-based changed files, or use --cached if you want staged changes) so the
PACKAGES and subsequent for loop that runs go test -v ./$pkg/... only run for
the correct set of changed Go files; replace the CHANGED_FILES assignment
accordingly and keep the downstream PACKAGES and for pkg loop unchanged.
In @.claude/skills/prow-ci/SKILL.md:
- Line 7: Change the top-level title string "Prow CI Access for Addon" to use
the full repository name to match the frontmatter (e.g., replace with "Prow CI
Access for addon-operator"); update only the header text that currently reads
"Prow CI Access for Addon" so it consistently references the full repo name used
elsewhere.
🪄 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: a508032a-ab23-4583-940b-dec6cc5b1649
📒 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 #750 +/- ##
=======================================
Coverage 59.24% 59.24%
=======================================
Files 62 62
Lines 4125 4125
=======================================
Hits 2444 2444
Misses 1532 1532
Partials 149 149 🚀 New features to boost your workflow:
|
devppratik
force-pushed
the
add-claude-and-docs
branch
4 times, most recently
from
8a28197 to
6202233
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
.claude/skills/prow-ci/analyze_failure.py (1)
51-53: ⚡ Quick winAvoid blanket exception handling in XML parse path.
except Exceptionwill also swallow programming errors and can hide real failures. Catch parse/IO exceptions explicitly and let unexpected failures surface.Suggested patch
- except Exception as e: + except (ET.ParseError, OSError, ValueError) as e: print(f"Warning: Could not parse {xml_file}: {e}", file=sys.stderr) return []🤖 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 51 - 53, Replace the broad "except Exception as e" in the XML parse/IO try block with explicit exception handling for XML parsing and I/O errors (e.g., xml.etree.ElementTree.ParseError and OSError) so only expected failures return an empty list and other errors bubble up; keep the existing stderr warning using xml_file and the error variable but change the except to "except (ET.ParseError, OSError) as e:" (or the appropriate ParseError symbol imported) and remove the blanket catch so programming errors are not swallowed.
🤖 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 188-198: The fenced code block shown (the "Updated:
DEVELOPMENT.md" commit notes under the Output Format section) is missing a
language tag and triggers markdownlint MD040; update that fenced block by adding
a language tag (e.g., use ```text) to the opening fence so it becomes ```text,
ensuring the block content remains unchanged and the lint rule is satisfied.
In @.claude/hooks/stop-prek-validation.sh:
- Line 30: In stop-prek-validation.sh the bare cd "$REPO_ROOT" can fail silently
and cause validations to run in the wrong directory; change it to explicitly
guard the cd (e.g., test the exit status) and abort on failure: perform the cd
using the REPO_ROOT variable and if it fails, print an error to stderr and exit
with a non-zero status so subsequent validation steps do not run in the wrong
directory.
In @.claude/skills/prow-ci/SKILL.md:
- Around line 69-71: The fenced code block that currently contains the URL (```
... https://prow.ci.openshift.org/view/gs/test-platform-results/... ```) needs a
language tag to satisfy markdownlint MD040; update the opening fence to include
a language such as text (e.g. change ``` to ```text) so the example URL fence is
explicitly tagged and markdownlint no longer flags MD040.
- Around line 77-80: Replace the hard-coded absolute cd command with a
repo-relative invocation so the example is portable: instead of changing to
/Users/ppanda/... use a repo-relative path to the prow-ci directory (for example
change to .claude/skills/prow-ci) or run the script directly from the repo root
by invoking fetch_prow_artifacts.py in .claude/skills/prow-ci; update the
example command that references fetch_prow_artifacts.py and "<prow-job-url>"
accordingly.
In `@TESTING.md`:
- Around line 131-147: Remove the stray extra opening fenced code block so the
controller test example renders correctly: keep a single ```go fence surrounding
the It("should reconcile resource", func() { ... }) example (which includes
k8sClient.Create, reconciler.Reconcile, k8sClient.Get and the Expect on
resource.Status.Conditions) and delete the additional erroneous ``` that appears
before the snippet.
---
Nitpick comments:
In @.claude/skills/prow-ci/analyze_failure.py:
- Around line 51-53: Replace the broad "except Exception as e" in the XML
parse/IO try block with explicit exception handling for XML parsing and I/O
errors (e.g., xml.etree.ElementTree.ParseError and OSError) so only expected
failures return an empty list and other errors bubble up; keep the existing
stderr warning using xml_file and the error variable but change the except to
"except (ET.ParseError, OSError) as e:" (or the appropriate ParseError symbol
imported) and remove the blanket catch so programming errors are not swallowed.
🪄 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: 0d31b831-89ec-45b3-969c-73e680b5ff73
📒 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/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.gitleaks.toml.prek-versionCONTRIBUTING.mdDEVELOPMENT.mdTESTING.mdhack/ci.shhack/prek.ci.tomlprek.toml
✅ Files skipped from review due to trivial changes (10)
- .prek-version
- DEVELOPMENT.md
- prek.toml
- .claude/hooks/cleanup.sh
- hack/prek.ci.toml
- CONTRIBUTING.md
- .claude/agents/lint-agent.md
- .claude/agents/ci-agent.md
- .claude/agents/security-agent.md
- .claude/agents/test-agent.md
🚧 Files skipped from review as they are similar to previous changes (4)
- .gitleaks.toml
- .claude/settings.json
- hack/ci.sh
- .claude/hooks/pre-edit.sh
devppratik
force-pushed
the
add-claude-and-docs
branch
from
3829466 to
0684413
Compare
devppratik
force-pushed
the
add-claude-and-docs
branch
from
0684413 to
56b345b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (4)
TESTING.md (2)
267-275:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winResolve contradictory pre-commit guidance (Lines 267-275).
This section currently says tests run automatically, then says they do not. Keep one clear path: tests are not in current pre-commit config; run
make go-testmanually before push.🤖 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 267 - 275, The TESTING.md fragment currently contradicts itself by showing a pre-commit YAML snippet for "go-test" while also stating it's not in the current pre-commit config; remove the YAML snippet and update the wording so it clearly states that Go tests are not run automatically by pre-commit and must be run manually using "make go-test" before pushing (referencing the "go-test" id and the "make go-test" command to locate the lines to edit).
131-147:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix malformed fenced code block in controller example (Lines 131-147).
There’s a duplicate opening fence and an extra closing fence; rendering/linting breaks.
Suggested patch
```go -```go It("should reconcile resource", func() { @@ })-```
</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
@TESTING.mdaround lines 131 - 147, The fenced Go code block in the
controller example is malformed (duplicate opening fence and an extra closing
fence) which breaks rendering; fix by replacing the section starting at the
duplicated "go" with a single properly delimited code fence so the snippet containing It("should reconcile resource", the k8sClient.Create/ k8sClient.Get calls, reconciler.Reconcile(ctx, req) and the Expect(resource.Status.Conditions) assertion is wrapped only once withgo at the top and ``` at the end.</details> </blockquote></details> <details> <summary>.claude/skills/README.md (1)</summary><blockquote> `66-72`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Add a language specifier to the directory-structure code fence (Line 66).** Use a typed fence (`text`) so markdownlint MD040 passes consistently. <details> <summary>Suggested patch</summary> ```diff -``` +```text .claude/skills/ ├── README.md └── skillname/ ├── SKILL.md # Required: skill definition └── reference/ # Optional: supporting docs ``` ``` </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/skills/README.md around lines 66 - 72, Update the fenced code block
that contains the directory tree in README.md (.claude/skills/README.md) to use
a language specifier by changing the opening fence fromtotext so the
block starting with ".claude/skills/" is a typed fence and satisfies
markdownlint MD040.</details> </blockquote></details> <details> <summary>.claude/skills/prow-ci/SKILL.md (1)</summary><blockquote> `72-74`: _⚠️ Potential issue_ | _🟡 Minor_ | _⚡ Quick win_ **Tag the URL example fence with a language (Line 72).** Use `text` on the opening fence to satisfy markdownlint MD040. <details> <summary>Suggested patch</summary> ```diff -``` +```text https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_addon_operator/<PR_NUMBER>/pull-ci-openshift-addon-operator-master-lint/<BUILD_ID> ``` ``` </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/skills/prow-ci/SKILL.md around lines 72 - 74, The fenced code block
containing the Prow CI URL example (the triple-backtick block that wraps
"https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_addon_operator/<PR_NUMBER>/pull-ci-openshift-addon-operator-master-lint/<BUILD_ID>")
needs an explicit language tag to satisfy markdownlint MD040; change the opening
fence fromtotext so the block is marked as text.</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:
- Around line 26-38: The "not in a git repository" branch currently invokes jq
("jq -n") before ensuring jq is installed, so if jq is missing the hook will
fail to emit structured JSON; fix by either moving the jq existence check
(command -v jq) to run before any use of jq (so REPO_ROOT check happens after
verifying jq), or replace the jq invocation in the REPO_ROOT-empty branch with a
static JSON heredoc that does not call jq (emit
{"decision":"block","reason":"Not in a git repository. Cannot run prek
validation."} directly). Update the code paths referring to REPO_ROOT and the jq
command accordingly.In @.claude/skills/prow-ci/analyze_failure.py:
- Around line 51-53: The current broad except Exception around parsing xml_file
should be narrowed: catch xml.etree.ElementTree.ParseError to handle malformed
XML and OSError/FileNotFoundError for filesystem issues, log distinct messages
for each (including xml_file and the error), return [] for expected parse/fs
errors, and re-raise any other unexpected exceptions; update the except block
that references xml_file and the parsing call in analyze_failure.py accordingly.- Around line 18-19: The parse_junit_xml function currently uses
xml.etree.ElementTree (ET.parse / ET.getroot) to read junit*.xml; replace ET
with defusedxml.ElementTree to harden XML parsing (update imports and calls in
parse_junit_xml to use defusedxml.ElementTree.parse/getroot). Also replace the
overly broad "except Exception" in parse_junit_xml (around the block handling
parse failures) with specific exceptions (e.g.,
defusedxml.ElementTree.ParseError and I/O exceptions like
FileNotFoundError/OSError) and log/propagate the caught error; finally ensure
defusedxml is added to project dependencies if not present.In @.claude/skills/prow-ci/fetch_prow_artifacts.py:
- Around line 63-75: The gcloud invocation using subprocess.run in the block
that builds cmd and calls subprocess.run(...) should be hardened: add a timeout
argument (e.g., timeout=60 or a configurable constant) to prevent hangs, and add
handling for FileNotFoundError and subprocess.TimeoutExpired in addition to
subprocess.CalledProcessError; catch FileNotFoundError to print a clear stderr
message that gcloud is not installed and return False, catch TimeoutExpired to
print a timeout-specific warning (including cmd and elapsed timeout) and return
False, and keep the existing CalledProcessError branch but include
e.stderr.decode() as before; update the try/except around the subprocess.run
call (the variables to edit include cmd and the subprocess.run invocation) and
ensure all error messages write to sys.stderr and the function returns False on
failure.
Duplicate comments:
In @.claude/skills/prow-ci/SKILL.md:
- Around line 72-74: The fenced code block containing the Prow CI URL example
(the triple-backtick block that wraps
"https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_addon_operator/<PR_NUMBER>/pull-ci-openshift-addon-operator-master-lint/<BUILD_ID>")
needs an explicit language tag to satisfy markdownlint MD040; change the opening
fence fromtotext so the block is marked as text.In @.claude/skills/README.md:
- Around line 66-72: Update the fenced code block that contains the directory
tree in README.md (.claude/skills/README.md) to use a language specifier by
changing the opening fence fromtotext so the block starting with
".claude/skills/" is a typed fence and satisfies markdownlint MD040.In
@TESTING.md:
- Around line 267-275: The TESTING.md fragment currently contradicts itself by
showing a pre-commit YAML snippet for "go-test" while also stating it's not in
the current pre-commit config; remove the YAML snippet and update the wording so
it clearly states that Go tests are not run automatically by pre-commit and must
be run manually using "make go-test" before pushing (referencing the "go-test"
id and the "make go-test" command to locate the lines to edit).- Around line 131-147: The fenced Go code block in the controller example is
malformed (duplicate opening fence and an extra closing fence) which breaks
rendering; fix by replacing the section starting at the duplicated "go" with a single properly delimited code fence so the snippet containing It("should reconcile resource", the k8sClient.Create/ k8sClient.Get calls, reconciler.Reconcile(ctx, req) and the Expect(resource.Status.Conditions) assertion is wrapped only once withgo at the top and ``` at the end.</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**: `0f3bcfca-2406-4349-a301-341b3b7f0ea7` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 3829466a70e74daca1c3bbf974105ae204013ba8 and 39f726921d9bce4c4d5677a660be06b7e3aff1cc. </details> <details> <summary>📒 Files selected for processing (23)</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` * `.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 (9)</summary> * .prek-version * .claude/hooks/cleanup.sh * hack/ci.sh * DEVELOPMENT.md * CONTRIBUTING.md * .claude/agents/lint-agent.md * .claude/agents/ci-agent.md * .claude/agents/test-agent.md * .claude/agents/security-agent.md </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (5)</summary> * .claude/settings.json * hack/prek.ci.toml * prek.toml * .gitleaks.toml * .claude/hooks/pre-edit.sh </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
devppratik
force-pushed
the
add-claude-and-docs
branch
2 times, most recently
from
ec5832c to
5720fed
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.claude/skills/prow-ci/SKILL.md (1)
229-231:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-verify
make e2e-binary-buildbefore documenting it as reproducible locally.Line 230 reintroduces the same target that was previously flagged as missing; if still undefined, this command will fail and mislead CI triage.
#!/bin/bash set -euo pipefail echo "Checking for e2e-binary-build make rule definitions..." files=$(fd -t f 'Makefile*' -d 10 --hidden .; fd -t f '*.mk' -d 10 --hidden .) || true files=$(printf "%s\n" "$files" | sed '/^$/d' | sort -u) if [ -z "$files" ]; then echo "No Makefile/.mk files found." exit 0 fi rg -n --no-heading -S '^[[:space:]]*e2e-binary-build[[:space:]]*:' $files || { echo "No e2e-binary-build target definition found." exit 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/SKILL.md around lines 229 - 231, The documented make target "e2e-binary-build" may not exist; re-verify and either add or correct the target before claiming it is reproducible. Search the repository's Makefile(s) and *.mk files for a rule named "e2e-binary-build" (or any similarly named e2e build target) and if missing, add a proper make rule or update the SKILL.md entry in .claude/skills/prow-ci/SKILL.md to the correct target name or note that it is not locally reproducible; ensure the docs reflect the actual target name and usage so running "make e2e-binary-build" will not fail.
🤖 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/SKILL.md:
- Around line 229-231: The documented make target "e2e-binary-build" may not
exist; re-verify and either add or correct the target before claiming it is
reproducible. Search the repository's Makefile(s) and *.mk files for a rule
named "e2e-binary-build" (or any similarly named e2e build target) and if
missing, add a proper make rule or update the SKILL.md entry in
.claude/skills/prow-ci/SKILL.md to the correct target name or note that it is
not locally reproducible; ensure the docs reflect the actual target name and
usage so running "make e2e-binary-build" will not fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f206db96-9531-4b0e-98f0-174e283e0889
📒 Files selected for processing (7)
.claude/hooks/README.md.claude/hooks/pre-edit.sh.claude/hooks/stop-prek-validation.sh.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
🚧 Files skipped from review as they are similar to previous changes (6)
- .claude/skills/README.md
- .claude/hooks/pre-edit.sh
- .claude/hooks/stop-prek-validation.sh
- .claude/hooks/README.md
- .claude/skills/prow-ci/fetch_prow_artifacts.py
- .claude/skills/prow-ci/analyze_failure.py
devppratik
force-pushed
the
add-claude-and-docs
branch
from
5720fed to
72f7f5f
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.
|
- 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
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
.claude/skills/prow-ci/fetch_prow_artifacts.py (1)
63-75:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden
gcloudexecution path to avoid hangs/crashes.This external call can still hang indefinitely and still crashes if
gcloudis missing fromPATH.Suggested patch
def download_from_gcs(gcs_path, local_path): """Download a file from GCS using gcloud storage cp.""" try: os.makedirs(os.path.dirname(local_path), exist_ok=True) cmd = [ 'gcloud', 'storage', 'cp', gcs_path, local_path, '--no-user-output-enabled' ] - subprocess.run(cmd, check=True, capture_output=True) + subprocess.run(cmd, check=True, capture_output=True, timeout=120) return True + except FileNotFoundError: + print("Error: gcloud CLI not found in PATH", file=sys.stderr) + return False + except subprocess.TimeoutExpired: + print(f"Warning: Timed out downloading {gcs_path}", file=sys.stderr) + return False except subprocess.CalledProcessError as e: print(f"Warning: Could not download {gcs_path}: {e.stderr.decode()}", file=sys.stderr) return False🤖 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 63 - 75, The gcloud subprocess call (the cmd list and subprocess.run invocation) can hang or fail if gcloud is missing; update the try/except around subprocess.run to: call subprocess.run(cmd, check=True, capture_output=True, timeout=120) (or a sane timeout), catch subprocess.TimeoutExpired and FileNotFoundError in addition to subprocess.CalledProcessError, and in each except branch print a clear stderr/detail message (use e.stderr.decode() when present or str(e) otherwise) and return False; keep the os.makedirs call and existing logic but ensure missing gcloud and timed-out runs are handled instead of hanging/crashing.
🤖 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/pre-edit.sh:
- Around line 47-49: The CANONICAL assignment uses python3 -c / python -c with
direct interpolation of $FILE and $REPO_ROOT which allows code injection if
those paths contain quotes; change both branches to call python with a fixed
script string that reads values from sys.argv (e.g., pass $FILE and $REPO_ROOT
as separate arguments) instead of embedding them in the -c source, update the
CANONICAL assignment lines to invoke python/python3 with argv-based parsing (use
os.path.normpath and os.path.relpath inside the script using sys.argv[1],
sys.argv[2]) and ensure any fallback echo remains unchanged.
In @.claude/skills/prow-ci/analyze_failure.py:
- Around line 14-63: Add unit tests covering the new CLI parsing and
log-analysis code paths (including analyze_build_log) that assert correct
behavior on (1) successful/expected inputs, (2) malformed inputs (bad URLs,
truncated logs), and (3) missing required artifacts (absent log file);
specifically write tests that call analyze_build_log and the CLI parsing
functions to verify the returned analysis contains expected pattern counts,
clipped lists (errors <=10, warnings <=5), and that missing files return None or
a clear error; include cases for multi-category lines (ensure a single line can
contribute to multiple lists/patterns), encoding/replace behavior, and negative
tests that validate graceful handling of malformed prowjob/log payloads. Ensure
tests cover both happy-path parsing and the three failure modes mentioned so
future changes to analyze_build_log and the CLI parsing logic are protected from
regressions.
- Around line 42-63: Wrap the open(log_file, ...) and subsequent read loop in a
try/except that catches OSError (and optionally IOError/UnicodeError) and
handles it by setting a read error on the analysis dict (e.g.,
analysis['read_error'] = str(e)) and returning analysis so main() can follow its
controlled exit path; locate the block that iterates the file (the for line in
f: loop and the analysis dict with keys
'patterns','errors','failures','warnings') and add the try/except around it (or
raise a specific LogReadError if your main() expects an exception) so file read
failures are guarded and reported instead of raising a traceback.
In @.claude/skills/prow-ci/fetch_prow_artifacts.py:
- Around line 84-89: The current read of prowjob.json only catches
json.JSONDecodeError so an OSError from open(local_path, 'r') will crash; update
the error handling around opening and loading the file (the block using
open(local_path, 'r') and json.load) to also catch OSError (or a broader
IOError/OSError) and treat it as non-fatal: log/print a concise error to stderr
including local_path and the exception, then return None just like for JSON
decode errors. Ensure you reference the same local_path/json.load block so the
file-read failures and parse failures are both handled the same way.
---
Duplicate comments:
In @.claude/skills/prow-ci/fetch_prow_artifacts.py:
- Around line 63-75: The gcloud subprocess call (the cmd list and subprocess.run
invocation) can hang or fail if gcloud is missing; update the try/except around
subprocess.run to: call subprocess.run(cmd, check=True, capture_output=True,
timeout=120) (or a sane timeout), catch subprocess.TimeoutExpired and
FileNotFoundError in addition to subprocess.CalledProcessError, and in each
except branch print a clear stderr/detail message (use e.stderr.decode() when
present or str(e) otherwise) and return False; keep the os.makedirs call and
existing logic but ensure missing gcloud and timed-out runs are handled instead
of hanging/crashing.
🪄 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: c4b672d8-90a9-4375-a390-814abdd1383b
📒 Files selected for processing (13)
.claude/agents/README.md.claude/agents/ci-agent.md.claude/agents/docs-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
✅ Files skipped from review due to trivial changes (6)
- .gitignore
- .claude/skills/README.md
- .claude/agents/ci-agent.md
- .claude/skills/prow-ci/SKILL.md
- .claude/agents/README.md
- .claude/agents/docs-agent.md
Comment on lines
+42
to
+63
| with open(log_file, 'r', encoding='utf-8', errors='replace') as f: | ||
| for line in f: | ||
| line_stripped = line.strip() | ||
|
|
||
| # Count pattern matches | ||
| for pattern_name, pattern_regex in patterns.items(): | ||
| if pattern_regex.search(line): | ||
| analysis['patterns'][pattern_name] += 1 | ||
|
|
||
| # Extract error lines (limit collection to avoid memory issues) | ||
| # Use independent if statements to allow capturing multiple categories per line | ||
| if len(analysis['errors']) < 10 and re.search(r'\bERROR\b', line, re.IGNORECASE): | ||
| analysis['errors'].append(line_stripped) | ||
| if len(analysis['failures']) < 10 and re.search(r'\bFAIL(ED)?\b', line, re.IGNORECASE): | ||
| analysis['failures'].append(line_stripped) | ||
| if len(analysis['warnings']) < 5 and re.search(r'\bWARNING\b', line, re.IGNORECASE): | ||
| analysis['warnings'].append(line_stripped) | ||
|
|
||
| # Remove patterns with zero occurrences | ||
| analysis['patterns'] = {k: v for k, v in analysis['patterns'].items() if v > 0} | ||
|
|
||
| return analysis |
There was a problem hiding this comment.
Guard build-log.txt read errors and fail cleanly.
open(log_file, ...) can raise OSError (permissions/corruption), which currently causes a traceback instead of the controlled exit path in main().
Suggested patch
- with open(log_file, 'r', encoding='utf-8', errors='replace') as f:
- for line in f:
- line_stripped = line.strip()
+ try:
+ with open(log_file, 'r', encoding='utf-8', errors='replace') as f:
+ for line in f:
+ line_stripped = line.strip()
- # Count pattern matches
- for pattern_name, pattern_regex in patterns.items():
- if pattern_regex.search(line):
- analysis['patterns'][pattern_name] += 1
+ # Count pattern matches
+ for pattern_name, pattern_regex in patterns.items():
+ if pattern_regex.search(line):
+ analysis['patterns'][pattern_name] += 1
- # Extract error lines (limit collection to avoid memory issues)
- # Use independent if statements to allow capturing multiple categories per line
- if len(analysis['errors']) < 10 and re.search(r'\bERROR\b', line, re.IGNORECASE):
- analysis['errors'].append(line_stripped)
- if len(analysis['failures']) < 10 and re.search(r'\bFAIL(ED)?\b', line, re.IGNORECASE):
- analysis['failures'].append(line_stripped)
- if len(analysis['warnings']) < 5 and re.search(r'\bWARNING\b', line, re.IGNORECASE):
- analysis['warnings'].append(line_stripped)
+ # Extract error lines (limit collection to avoid memory issues)
+ # Use independent if statements to allow capturing multiple categories per line
+ if len(analysis['errors']) < 10 and re.search(r'\bERROR\b', line, re.IGNORECASE):
+ analysis['errors'].append(line_stripped)
+ if len(analysis['failures']) < 10 and re.search(r'\bFAIL(ED)?\b', line, re.IGNORECASE):
+ analysis['failures'].append(line_stripped)
+ if len(analysis['warnings']) < 5 and re.search(r'\bWARNING\b', line, re.IGNORECASE):
+ analysis['warnings'].append(line_stripped)
+ except OSError as e:
+ print(f"Error: Could not read build log {log_file}: {e}", file=sys.stderr)
+ return NoneAlso applies to: 185-189
🤖 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 42 - 63, Wrap the
open(log_file, ...) and subsequent read loop in a try/except that catches
OSError (and optionally IOError/UnicodeError) and handles it by setting a read
error on the analysis dict (e.g., analysis['read_error'] = str(e)) and returning
analysis so main() can follow its controlled exit path; locate the block that
iterates the file (the for line in f: loop and the analysis dict with keys
'patterns','errors','failures','warnings') and add the try/except around it (or
raise a specific LogReadError if your main() expects an exception) so file read
failures are guarded and reported instead of raising a traceback.
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
|
@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. |
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
New Features
Documentation
Chores