docs, feat(SREP-4460, SREP-4926: Add Standardized Claude hooks, skill, agents. Update standardised docs)#483
Open
devppratik wants to merge 6 commits into
Open
Conversation
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. |
|
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:
WalkthroughAdds Claude agents documentation and per-agent specs, hook scripts and README, Prek and gitleaks configs, Claude settings, prow-ci skills and CLI tools, CI helper scripts, and contributor/developer/testing guides. ChangesClaude Agent Framework & Specifications
Validation Hooks & Configuration
Skills Framework & Prow CI
Developer & Testing Guides
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (1)
.claude/agents/ci-agent.md (1)
113-127: ⚡ Quick winMake missing-check detection less ambiguous.
The current grep-based scan can misreport coverage of required checks (generic string matching in YAML). Prefer matching task/step identifiers used in pipeline definitions to reduce false parity signals.
Suggested doc patch
-REQUIRED_CHECKS=( - "golangci-lint" - "gitleaks" - "go test" - "go build" - "rbac-wildcard-check" -) +REQUIRED_CHECKS=( + "golangci-lint" + "gitleaks" + "go-test" + "go-build" + "rbac-wildcard-check" +) for check in "${REQUIRED_CHECKS[@]}"; do - if ! grep -q "$check" .tekton/*.yaml; then + if ! grep -Eq "name:\s*${check}\b|taskRef:\s*name:\s*${check}\b" .tekton/*.yaml; then echo "WARNING: $check not found in CI" fi done🤖 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/ci-agent.md around lines 113 - 127, The REQUIRED_CHECKS grep loop is too fuzzy and can false-positive on generic substrings; update the check logic to match pipeline task/step identifiers instead of plain text by parsing .tekton/*.yaml and comparing explicit keys (e.g., "task:", "taskRef.name", "name:" for steps) against the REQUIRED_CHECKS entries; replace the simple grep call in the for loop with a YAML-aware check (using yq or a small awk/python parser) that extracts taskRef.name/task names and step names from .tekton/*.yaml and fails/warns only if the exact identifier is missing, keeping the REQUIRED_CHECKS array and loop structure but changing the matching to exact identifier comparison.
🤖 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:
- Line 188: The fenced code block at the example in .claude/agents/docs-agent.md
is unlabeled (triple backticks only), which triggers markdownlint MD040; update
that fenced block by adding a language tag (e.g., use "text") after the opening
backticks so the block reads ```text and keep the existing contents unchanged to
satisfy the linter.
In @.claude/agents/test-agent.md:
- Around line 31-39: The CHANGED_FILES extraction and PACKAGES handling are
brittle when no .go files changed or when the git diff base is different; update
the CHANGED_FILES command to compare against a real base (use git diff
--name-only "$(git merge-base HEAD ${GIT_BASE_REF:-origin/main})" or use an
injected GIT_BASE_REF) and make the pipeline resilient by appending "|| true" so
grep/xargs don't fail; then guard PACKAGES by checking if CHANGED_FILES is empty
before computing PACKAGES (or use xargs -r) and skip the for-loop when PACKAGES
is empty to avoid running "go test" with an empty package list (adjust the
variables CHANGED_FILES, PACKAGES and the for pkg in $PACKAGES loop
accordingly).
In @.claude/hooks/README.md:
- Around line 182-183: Update the README entry that currently says “On every
turn: Stop hook runs `prek run --all-files`” to reflect the hook's real
behavior: note that the stop hook skips execution when there are no changes
(default mode) and, when it runs for CI-style validation, it invokes prek with
the CI config (i.e., `prek --config hack/prek.ci.toml`); reference the exact
phrase “On every turn” and the command `prek run --all-files` in your edit so
reviewers can verify the change.
- Around line 332-335: The README documents the pinned Prek version as `v0.3.9`
but the actual `.prek-version` file is `v0.4.1`; update the example in the
README (the text showing `cat .prek-version # v0.3.9`) to reflect `v0.4.1` so
the documented `.prek-version` value matches the real file.
- Line 3: The README header contains a copy/paste reference to “OCM Agent
Operator development”; update that phrase in .claude/hooks/README.md to
reference the correct repository name openshift/cloud-ingress-operator (e.g.,
replace “OCM Agent Operator development” with “openshift/cloud-ingress-operator
development” or equivalent wording) so the repository name and description are
consistent.
In @.claude/settings.json:
- Around line 22-25: The entries "Bash(grep *)", "Bash(find *)", "Bash(ls *)",
and "Bash(cat *)" in the settings allow list are too broad and permit silent
discovery/reading of arbitrary files; update the settings.json allow/ask
configuration by removing these four broad "Bash(...)" entries from the allow
list and either (a) add them to the ask list so the agent must prompt before
running them, or (b) replace each with a scoped pattern (e.g., "Bash(grep *.md)"
or "Bash(cat /safe/path/*)" or explicit extensions) that limits file
paths/extensions to non-sensitive locations, ensuring the names "Bash(grep *)",
"Bash(find *)", "Bash(ls *)", and "Bash(cat *)" are the symbols you change.
- Around line 37-41: Update the deny patterns in the settings so they match
dangerous argument permutations instead of only the exact strings; replace exact
entries like "Bash(git commit --no-verify)", "Bash(git push --force origin
master/main)", and "Bash(rm -rf /)" with globbed variants that catch flag/target
permutations (e.g., patterns that include * around flags and targets such as
variants matching "--no-verify" anywhere in the git commit args, "--force" or
"--force-with-lease" anywhere in git push args, and "rm -rf" with any target
like * or .), and also add a more general "rm -r*" style pattern to cover
recursive removal variants so unsafe invocations cannot bypass the deny list.
In @.claude/skills/README.md:
- Around line 66-72: The fenced code block in .claude/skills/README.md lacks a
language identifier (MD040); edit the README.md code fence starting before the
tree diagram and add a language token (e.g., use "text" or "bash" immediately
after the opening ``` of the block) so the opening fence reads ```text (or
```bash) to satisfy markdownlint and preserve the existing block content and
formatting.
In @.gitleaks.toml:
- Around line 25-32: Remove the overly-broad test-file allowlist entry from the
.gitleaks.toml paths array (the pattern ".*_test\.go") so gitleaks will no
longer ignore secrets in repository-wide test files; instead retain only the
narrow fixture/deploy-specific patterns (e.g., '''test/fixtures/.*''',
'''test/deploy/.*''') and any other targeted exceptions already present in the
paths array, then run gitleaks to validate no unintended matches remain.
In `@DEVELOPMENT.md`:
- Around line 149-153: Update the DEVELOPMENT.md bullet list so it no longer
claims local pre-commit hooks mirror CI by including go-test; specifically
change or remove the "go-test ↔ Unit test job" line and add a brief note that
go-test is not currently installed in pre-commit and refer readers to TESTING.md
for the current local test instructions; ensure the entry mentions "go-test"
explicitly so contributors searching for that symbol see the accurate status.
In `@TESTING.md`:
- Around line 266-274: Remove the contradictory pre-commit note about `go-test`:
delete the YAML snippet that implies `go-test` runs automatically and replace
the paragraph to state that `go-test` is NOT part of the current pre-commit
config (too slow) and must be run manually; update the instructions to show the
command `make go-test` (formatted as a shell command) and remove any lingering
references that suggest `id: go-test` is enabled in pre-commit so only the
manual-run guidance remains.
---
Nitpick comments:
In @.claude/agents/ci-agent.md:
- Around line 113-127: The REQUIRED_CHECKS grep loop is too fuzzy and can
false-positive on generic substrings; update the check logic to match pipeline
task/step identifiers instead of plain text by parsing .tekton/*.yaml and
comparing explicit keys (e.g., "task:", "taskRef.name", "name:" for steps)
against the REQUIRED_CHECKS entries; replace the simple grep call in the for
loop with a YAML-aware check (using yq or a small awk/python parser) that
extracts taskRef.name/task names and step names from .tekton/*.yaml and
fails/warns only if the exact identifier is missing, keeping the REQUIRED_CHECKS
array and loop structure but changing the matching to exact identifier
comparison.
🪄 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: 62e6b38b-ad46-46ac-91d9-9f0631009a91
📒 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 @@
## master #483 +/- ##
=======================================
Coverage 41.57% 41.57%
=======================================
Files 27 27
Lines 2665 2665
=======================================
Hits 1108 1108
Misses 1478 1478
Partials 79 79 🚀 New features to boost your workflow:
|
devppratik
force-pushed
the
add-claude-and-docs
branch
3 times, most recently
from
2fc801d to
60dd6d9
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.claude/hooks/README.md (1)
182-183:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix conflicting stop-hook behavior docs (Line 182).
This section contradicts Line 72. It should describe conditional execution and CI config usage, not
prek run --all-files.Suggested doc fix
-- **On every turn**: Stop hook runs `prek run --all-files` +- **On stop (when changes exist, default mode)**: Stop hook runs `prek run --config hack/prek.ci.toml` on changed files +- **On every stop (strict mode)**: set `CLAUDE_LINT_ON_STOP=true` to always run validation🤖 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, Update the README entry that currently states "Stop hook runs `prek run --all-files`" to reflect the intended conditional behavior: describe that the Stop hook runs checks only for changed files or when explicitly triggered and that full-suite runs are delegated to CI via the repository's CI config; replace the literal `prek run --all-files` mention with guidance on the Stop hook's conditional execution, how to trigger full runs, and reference the CI config used for full runs (e.g., CI pipeline or jobs) and the Pre-commit hook behavior so readers understand when `prek` is run locally vs in CI.
🤖 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/hooks/README.md:
- Around line 182-183: Update the README entry that currently states "Stop hook
runs `prek run --all-files`" to reflect the intended conditional behavior:
describe that the Stop hook runs checks only for changed files or when
explicitly triggered and that full-suite runs are delegated to CI via the
repository's CI config; replace the literal `prek run --all-files` mention with
guidance on the Stop hook's conditional execution, how to trigger full runs, and
reference the CI config used for full runs (e.g., CI pipeline or jobs) and the
Pre-commit hook behavior so readers understand when `prek` is run locally vs in
CI.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7598ed71-93f7-47ba-a2ca-9f065a721532
📒 Files selected for processing (21)
.claude/agents/README.md.claude/agents/ci-agent.md.claude/agents/docs-agent.md.claude/agents/lint-agent.md.claude/agents/security-agent.md.claude/agents/test-agent.md.claude/hooks/README.md.claude/hooks/cleanup.sh.claude/hooks/pre-edit.sh.claude/hooks/stop-prek-validation.sh.claude/settings.json.claude/skills/README.md.claude/skills/prow-ci/SKILL.md.gitleaks.toml.prek-versionCONTRIBUTING.mdDEVELOPMENT.mdTESTING.mdhack/ci.shhack/prek.ci.tomlprek.toml
✅ Files skipped from review due to trivial changes (9)
- .prek-version
- .claude/hooks/cleanup.sh
- prek.toml
- .claude/agents/lint-agent.md
- .claude/agents/security-agent.md
- TESTING.md
- .claude/agents/ci-agent.md
- CONTRIBUTING.md
- DEVELOPMENT.md
🚧 Files skipped from review as they are similar to previous changes (7)
- .claude/settings.json
- hack/ci.sh
- .gitleaks.toml
- .claude/agents/test-agent.md
- .claude/hooks/pre-edit.sh
- hack/prek.ci.toml
- .claude/hooks/stop-prek-validation.sh
devppratik
force-pushed
the
add-claude-and-docs
branch
2 times, most recently
from
c039cf5 to
fa8e3e8
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
.claude/agents/docs-agent.md (1)
188-188:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language tag to the fenced code block.
The example output block at line 188 should have a language tag (e.g.,
text) to satisfy markdownlint MD040.📝 Proposed fix
-``` +```text Updated: DEVELOPMENT.md - Added section on new make target: go-bench🤖 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 at line 188, Add a language tag to the fenced code block shown in the example output in .claude/agents/docs-agent.md by changing the opening fence from ``` to a tagged fence (for example ```text) so markdownlint MD040 is satisfied; locate the example output fenced block (the triple-backtick block near the sample "Updated: DEVELOPMENT.md" lines) and update the opening fence to include the language token (e.g., text)..claude/hooks/README.md (1)
182-182:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate automatic validation description to match actual hook behavior.
Line 182 states "On every turn: Stop hook runs
prek run --all-files", but the stop hook documentation (lines 54-76) describes:
- Default mode: only runs when there are uncommitted changes
- Uses
--config hack/prek.ci.toml(not the default config)- Runs on changed files (not
--all-files)This inconsistency was flagged in a previous review and remains unaddressed.
📝 Proposed fix
### Automatic Validation Prek runs automatically: -- **On every turn**: Stop hook runs `prek run --all-files` +- **On stop (when changes detected)**: Stop hook runs `prek run --config hack/prek.ci.toml` (default mode) +- **On stop (strict mode)**: Stop hook runs `prek run --config hack/prek.ci.toml --all-files` (if CLAUDE_LINT_ON_STOP=true) - **On commit**: Pre-commit hook runs relevant checks🤖 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 182, Update the one-line summary that currently reads "On every turn: Stop hook runs `prek run --all-files`" to accurately reflect the Stop hook behavior: it does not run on every turn, it runs by default only when there are uncommitted changes, invokes prek with the CI config (`--config hack/prek.ci.toml`), and runs against changed files (not `--all-files`); replace the incorrect `--all-files` claim and “on every turn” wording with a concise sentence stating the default mode, the use of `--config hack/prek.ci.toml`, and that it targets changed files..claude/skills/README.md (1)
66-66:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFenced code block still missing a language identifier (MD040).
The directory-structure fence at Line 66 has no language token despite being marked addressed previously; the current code still shows a bare
```.Suggested patch
-``` +```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 at line 66, The fenced code block showing the
directory structure is missing a language identifier (MD040); update the opening
fence for that block (the directory-structure fence in .claude/skills/README.md)
from a bareto a fenced block with a language token such astext so the
linter recognizes it; ensure the opening fence for the directory listing (the
one around the .claude/skills/ tree) is changed to include the language token
and the closing fence remains unchanged.</details> </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (1)</summary><blockquote> <details> <summary>.claude/skills/prow-ci/SKILL.md (1)</summary><blockquote> `69-71`: _💤 Low value_ **Consider adding a language tag to the fenced code block.** The example URL block could benefit from a language tag for consistency with other code blocks in the document, though this is a minor style improvement. <details> <summary>♻️ Optional improvement</summary> ```diff -``` +```text https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/openshift_cloud-ingress-operator/328/pull-ci-openshift-cloud-ingress-operator-master-lint/2059308810190721024 ``` ``` </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 69 - 71, The fenced code block
containing the Prow CI URL should include a language tag for consistency; change
the opening fence fromtotext so the block readstext followed by the URL and closing, ensuring it matches other code blocks in the document.</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: The script runs cd "$REPO_ROOT" without checking for failure, so if
changing directory fails the rest of stop-prek-validation.sh (including invoking
prek) can run in the wrong directory; update the cd invocation in
stop-prek-validation.sh to detect and handle errors (for example by exiting or
returning) when cd "$REPO_ROOT" fails — ensure the error is logged or reported
and the script aborts instead of continuing.In @.claude/skills/prow-ci/analyze_failure.py:
- Line 18: Replace use of xml.etree.ElementTree parsing with the defusedxml-safe
equivalent: change the import that creates the ET alias to import
defusedxml.ElementTree as ET and replace the call ET.parse(xml_file) (and any
other ET parsing/iterating usages) so parsing of untrusted CI artifact XML uses
defusedxml; also add defusedxml to the project dependencies (requirements.txt or
pyproject.toml) if it’s not already present.In @.claude/skills/prow-ci/SKILL.md:
- Line 77: Replace the hardcoded absolute path string
"/Users/ppanda/rh-projects/ROSA-730/cloud-ingress-operator/" found in the
SKILL.md content (the cd command line) with a relative path or a placeholder
variable (e.g., "./" or "$REPO_ROOT") so the instruction is portable; update the
cd /Users/ppanda/.../cloud-ingress-operator/.claude/skills/prow-ciline to use
the relative path or placeholder and ensure surrounding documentation/examples
reference the placeholder consistently.In @.gitignore:
- Line 99: Restore the original
.DS_Storeignore entry and add a separate
entry for the.work/directory instead of replacing.DS_Store; specifically,
update the .gitignore line that currently reads.DS_Store.work/to include two
lines:.DS_Storeand.work/so macOS metadata and the project.work/
directory are each ignored independently.
Duplicate comments:
In @.claude/agents/docs-agent.md:
- Line 188: Add a language tag to the fenced code block shown in the example
output in .claude/agents/docs-agent.md by changing the opening fence fromto a tagged fence (for exampletext) so markdownlint MD040 is satisfied; locate
the example output fenced block (the triple-backtick block near the sample
"Updated: DEVELOPMENT.md" lines) and update the opening fence to include the
language token (e.g., text).In @.claude/hooks/README.md:
- Line 182: Update the one-line summary that currently reads "On every turn:
Stop hook runsprek run --all-files" to accurately reflect the Stop hook
behavior: it does not run on every turn, it runs by default only when there are
uncommitted changes, invokes prek with the CI config (--config hack/prek.ci.toml), and runs against changed files (not--all-files); replace
the incorrect--all-filesclaim and “on every turn” wording with a concise
sentence stating the default mode, the use of--config hack/prek.ci.toml, and
that it targets changed files.In @.claude/skills/README.md:
- Line 66: The fenced code block showing the directory structure is missing a
language identifier (MD040); update the opening fence for that block (the
directory-structure fence in .claude/skills/README.md) from a bareto a fenced block with a language token such astext so the linter recognizes it;
ensure the opening fence for the directory listing (the one around the
.claude/skills/ tree) is changed to include the language token and the closing
fence remains unchanged.
Nitpick comments:
In @.claude/skills/prow-ci/SKILL.md:
- Around line 69-71: The fenced code block containing the Prow CI URL should
include a language tag for consistency; change the opening fence fromtotext so the block readstext followed by the URL and closing, ensuring
it matches other code blocks in the document.</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**: `264b4268-e936-4bfc-be69-abe4d40552eb` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 60dd6d9934e9e54217c5ff786ef0c23a7f009f78 and c039cf55f546669da73d5b5bd6afcfccb6ddfccc. </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 (8)</summary> * .prek-version * prek.toml * DEVELOPMENT.md * .claude/agents/lint-agent.md * .claude/agents/security-agent.md * .claude/agents/ci-agent.md * .claude/agents/test-agent.md * CONTRIBUTING.md </details> <details> <summary>🚧 Files skipped from review as they are similar to previous changes (6)</summary> * .claude/hooks/cleanup.sh * hack/ci.sh * .claude/hooks/pre-edit.sh * TESTING.md * .claude/settings.json * .gitleaks.toml </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
devppratik
force-pushed
the
add-claude-and-docs
branch
from
fa8e3e8 to
9495d6e
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
.claude/agents/docs-agent.md (1)
188-188:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd a language tag to the fenced code block.
The fenced code block at line 188 is missing a language specifier, which triggers markdownlint MD040.
📝 Proposed fix
-``` +```text Updated: DEVELOPMENT.md🤖 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 at line 188, The fenced code block starting with the triple backticks in docs-agent.md is missing a language tag; update that backtick fence to include a language specifier (e.g., change ``` to ```text) so the block is "```text" and the snippet (Updated: DEVELOPMENT.md) follows the fence to satisfy markdownlint MD040..claude/skills/prow-ci/analyze_failure.py (1)
11-18:⚠️ Potential issue | 🟠 MajorUse a hardened XML parser for downloaded JUnit artifacts.
This is still parsing external CI artifacts with
xml.etree.ElementTree, so the earlierdefusedxmlconcern remains unresolved.#!/bin/bash set -euo pipefail echo "Current XML parser usage:" rg -n 'xml\.etree\.ElementTree|ET\.parse\(' .claude/skills/prow-ci/analyze_failure.py echo echo "Check whether defusedxml is already declared anywhere obvious:" fd -HI 'pyproject.toml' . -x rg -n 'defusedxml' {} || true fd -HI 'requirements*.txt' . -x rg -n 'defusedxml' {} || true fd -HI 'setup.py' . -x rg -n 'defusedxml' {} || true🤖 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 - 18, The parse_junit_xml function currently uses xml.etree.ElementTree (ET.parse) to parse untrusted CI artifacts; replace that with a hardened parser from defusedxml by changing the import to use defusedxml.ElementTree (e.g., from defusedxml import ElementTree as ET) and update the parse call in parse_junit_xml to ET.parse(xml_file); also add defusedxml to project dependencies (pyproject.toml/requirements) if not present and run tests to ensure no other code relies on the old xml.etree import name.
🤖 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: The README intro contains a grammatical error: change the phrase "this
operator development workflows" to either "operator development workflows" or
"this operator's development workflows" for correct possessive form; update the
sentence in the first paragraph of .claude/agents/README.md to use one of those
two options so the introduction reads grammatically correct.
In @.claude/hooks/stop-prek-validation.sh:
- Around line 53-80: The hook can skip validating relevant files because `prek
run` without flags only checks staged/indexed files while the pre-stop logic
also allows untracked/unstaged changes; update the logic around FORCE_LINT and
PREK_OUTPUT/`prek run` so the set prek validates matches the files that
triggered the hook: detect whether changes are staged (use git diff --cached) vs
unstaged/untracked (use git ls-files --others --exclude-standard or git diff
--name-only) and then either invoke `prek run` with an explicit file list for
those paths or call `prek run --all-files` when intent is to cover
untracked/unstaged changes; ensure the file-list construction and the `prek run
--config hack/prek.ci.toml` invocation are changed (the PREK_OUTPUT assignment)
so prek actually runs against the same files the hook examined.
In @.claude/skills/prow-ci/analyze_failure.py:
- Around line 21-24: The code uses root.findall('.//testsuite') which skips the
root element when the XML root itself is a <testsuite>, so single-suite JUnit
files are ignored; update the logic in analyze_failure.py to include the root
when root.tag == 'testsuite' (e.g., build the list of suites as suites =
root.findall('.//testsuite'); if root.tag == 'testsuite': suites.insert(0,
root)) and then iterate over that suites list (using the existing suite_name,
testsuite and testcase handling) so single-suite files are processed correctly.
In @.claude/skills/prow-ci/fetch_prow_artifacts.py:
- Around line 78-93: The fetcher currently only downloads prowjob.json and
build-log.txt (see fetch_prowjob_json and fetch_build_log) but
analyze_failure.py expects artifacts/junit*.xml; modify the fetcher to also
locate and download JUnit XML artifacts from the job's artifacts directory (e.g.
gcs_base_path + "/artifacts/" pattern) by listing/matching objects and calling
download_from_gcs for each matching junit*.xml file (or, if listing isn't
available, attempt to download a small set of expected names like
artifacts/junit.xml, artifacts/junit1.xml, artifacts/junit_*.xml); ensure files
are saved under output_dir with their original names so analyze_failure.py can
find artifacts/junit*.xml, or alternatively remove/adjust references to JUnit in
downstream docs/analysis if you choose not to fetch them.
- Around line 31-57: The parsed bucket_path can include artifact suffixes (like
/build-log.txt) so normalize it to the build root by truncating bucket_path
immediately after the matched build_id: after you compute build_id from
build_match, compute end = bucket_path.find(build_id) + len(build_id) and set
bucket_path = bucket_path[:end].rstrip('/') (this preserves the job segment and
build_id but removes any artifact paths), then recompute gcs_base_path =
f"gs://test-platform-results/{bucket_path}" (keep build_match, build_id,
job_match, job_name variable usage intact).
In @.claude/skills/prow-ci/SKILL.md:
- Around line 7-9: The SKILL.md still contains copied CI metadata for
"rbac-permissions-operator"; replace every occurrence of that repo-specific
string and all referenced Prow URLs, job names, ci-operator config names, Tekton
pipeline names, and the Codecov secret with the actual values for this
repository ("openshift/cloud-ingress-operator") so links, job identifiers, and
secret names point to this repo; update example commands, GCS bucket paths, and
any pipeline or job YAML snippets to use the cloud-ingress-operator project
identifiers (Prow job IDs, ci-operator config name, Tekton task/pipeline names,
and Codecov token name) and ensure any URLs and example commands reflect the
correct repo and CI resources throughout the document (including the other
affected sections mentioned).
---
Duplicate comments:
In @.claude/agents/docs-agent.md:
- Line 188: The fenced code block starting with the triple backticks in
docs-agent.md is missing a language tag; update that backtick fence to include a
language specifier (e.g., change ``` to ```text) so the block is "```text" and
the snippet (Updated: DEVELOPMENT.md) follows the fence to satisfy markdownlint
MD040.
In @.claude/skills/prow-ci/analyze_failure.py:
- Around line 11-18: The parse_junit_xml function currently uses
xml.etree.ElementTree (ET.parse) to parse untrusted CI artifacts; replace that
with a hardened parser from defusedxml by changing the import to use
defusedxml.ElementTree (e.g., from defusedxml import ElementTree as ET) and
update the parse call in parse_junit_xml to ET.parse(xml_file); also add
defusedxml to project dependencies (pyproject.toml/requirements) if not present
and run tests to ensure no other code relies on the old xml.etree import name.
🪄 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: a1e83c95-d6d2-4db6-b82c-c7d8e3a983b1
📒 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 (11)
- .gitignore
- .claude/hooks/cleanup.sh
- .prek-version
- hack/prek.ci.toml
- .claude/agents/lint-agent.md
- .claude/agents/test-agent.md
- CONTRIBUTING.md
- DEVELOPMENT.md
- .claude/agents/ci-agent.md
- TESTING.md
- .claude/agents/security-agent.md
🚧 Files skipped from review as they are similar to previous changes (5)
- hack/ci.sh
- .claude/settings.json
- prek.toml
- .claude/hooks/pre-edit.sh
- .gitleaks.toml
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.claude/skills/prow-ci/SKILL.md (1)
80-80:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake the working-directory step portable and unambiguous.
cd $PWD/.claude/skills/prow-cionly works when$PWDis already the repo root. Use a repo-root-relative command (or explicitly derive repo root) to avoid broken copy/paste flows.Suggested doc fix
-cd $PWD/.claude/skills/prow-ci +cd .claude/skills/prow-ci🤖 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 80, Replace the brittle "cd $PWD/.claude/skills/prow-ci" usage with a repo-root-relative change-dir: compute the repository root (e.g. with git rev-parse --show-toplevel or use the CI-provided workspace env like GITHUB_WORKSPACE) and then cd into the .claude/skills/prow-ci directory from that root; update the line containing the literal string "cd $PWD/.claude/skills/prow-ci" to use the derived repo-root path so the step works 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/skills/prow-ci/SKILL.md:
- Line 244: The text in SKILL.md uses incorrect capitalization "Github" in the
skip rules line; update the mention of `.github/` to use the official "GitHub"
casing so the line reads: "Skip rules: Changes to `.tekton/`, `.github/`, `.md`
files, `OWNERS`, `LICENSE` don't trigger most jobs" with "GitHub" capitalized
where referenced (locate the skip rules sentence in SKILL.md and replace
"Github" with "GitHub").
---
Duplicate comments:
In @.claude/skills/prow-ci/SKILL.md:
- Line 80: Replace the brittle "cd $PWD/.claude/skills/prow-ci" usage with a
repo-root-relative change-dir: compute the repository root (e.g. with git
rev-parse --show-toplevel or use the CI-provided workspace env like
GITHUB_WORKSPACE) and then cd into the .claude/skills/prow-ci directory from
that root; update the line containing the literal string "cd
$PWD/.claude/skills/prow-ci" to use the derived repo-root path so the step works
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: c72aa3e3-9525-4b19-80ff-fe77325ed67a
📒 Files selected for processing (1)
.claude/skills/prow-ci/SKILL.md
devppratik
force-pushed
the
add-claude-and-docs
branch
2 times, most recently
from
0831a64 to
14b312a
Compare
devppratik
force-pushed
the
add-claude-and-docs
branch
from
14b312a to
6b513cd
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
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
Member
|
/test validate |
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
Documentation
Chores
New Features