docs, feat(SREP-4460, SREP-4926: Add Standardized Claude hooks, skill, agents. Update standardised docs)#437
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. |
Contributor
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR adds a Claude Agents framework (lint/test/security/ci/docs), Prek-based hooks and edit-guards, Prow CI fetch/analyze skills and scripts, Prek/CI configurations, gitleaks rules, Claude command permissions, and comprehensive developer documentation (CONTRIBUTING, DEVELOPMENT, TESTING). ChangesClaude Agents & Automation Framework
Pre-commit Hook Infrastructure & File Guards
Pre-commit Validation Configurations
Claude Code Configuration
Prow CI Analysis Skill
Developer Documentation
Security & Miscellaneous Configuration
🎯 4 (Complex) | ⏱️ ~60 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 inconclusive)
✅ Passed checks (13 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #437 +/- ##
=======================================
Coverage 72.44% 72.44%
=======================================
Files 11 11
Lines 704 704
=======================================
Hits 510 510
Misses 173 173
Partials 21 21 🚀 New features to boost your workflow:
|
Member
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
devppratik
force-pushed
the
add-claude-and-docs
branch
5 times, most recently
from
9c8534a to
d010c85
Compare
devppratik
force-pushed
the
add-claude-and-docs
branch
2 times, most recently
from
dd433bd to
6f9f342
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (1)
.claude/hooks/stop-prek-validation.sh (1)
21-21: 💤 Low valueConsider adding
-eflag to strict mode for fail-fast behavior.Line 21 sets
set -uo pipefailbut omits the-eflag. This means command failures won't immediately exit the script. While the current implementation handles errors explicitly (jq checks, exit code capture for prek), adding-ewould provide fail-fast protection for any future additions.🔒 Proposed fix
-set -uo pipefail +set -euo pipefailReview the script to ensure all intentional non-zero exits are handled with
|| trueor explicit conditionals.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.claude/hooks/stop-prek-validation.sh at line 21, The script currently enables strict mode with "set -uo pipefail" but omits "-e", so add "-e" to make it "set -euo pipefail" to enable fail-fast behavior; after adding this, audit the stop-prek-validation.sh script for any commands that intentionally may return non-zero and make them explicit (e.g., append "|| true" or wrap in conditionals) — pay special attention to the existing jq checks and the prek exit-code handling to ensure those still behave as intended.
🤖 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 52-60: The parity validation commands currently grep
.pre-commit-config.yaml but the new source of truth is prek.toml; update the two
grep invocations (the "Check pre-commit uses same golangci-lint version as CI"
and "Check gitleaks version" commands referenced around the parity validation
block and the duplicate at lines 148-149) to point at prek.toml instead of
.pre-commit-config.yaml so the CI/local comparison validates prek.toml as
intended.
- Around line 23-30: The placeholder pipeline names "this repository-*.yaml" in
.claude/agents/ci-agent.md should be replaced with the real Tekton pipeline
filenames; update the list, the `tkn` example, and the failure report to use:
splunk-forwarder-operator-pull-request.yaml,
splunk-forwarder-operator-push.yaml,
splunk-forwarder-operator-e2e-pull-request.yaml,
splunk-forwarder-operator-e2e-push.yaml,
splunk-forwarder-operator-pko-push.yaml, and
splunk-forwarder-operator-pko-pull-request.yaml so every occurrence of "this
repository-..." is swapped to the corresponding splunk-forwarder-operator-...
filename (search for the text "this repository-" to locate all places, including
the pipeline list, the tkn example, and the failure report).
In @.claude/agents/docs-agent.md:
- Around line 31-37: Replace all references to docs/development.md with the
repo-root DEVELOPMENT.md so the doc trigger paths are consistent: update the
list entries that currently mention "New dependencies: Update
`docs/development.md`" (and the similar occurrences at lines 90-93) to point to
"DEVELOPMENT.md" instead, and verify other entries referring to development docs
(e.g., in the "Build process" and "Make targets" bullets) use the root
DEVELOPMENT.md consistently while leaving other filenames (CLAUDE.md,
docs/design.md, TESTING.md, CONTRIBUTING.md) unchanged.
- Around line 174-183: The current pipeline using "grep '```bash' *.md | grep
'make '" never captures make invocations because it only matches fence lines;
replace that pipeline with a pipeline that scans all markdown files for fenced
bash blocks and extracts lines starting with "make " (e.g., use find + xargs +
awk to enter fenced ```bash blocks and print lines beginning with make), then
pipe the results through the existing sed/sort logic to extract and unique the
target names; update the section that currently runs grep '```bash' *.md | grep
'make ' | sed ... to use this fenced-block-aware extractor so documented make
targets are detected.
In @.claude/agents/lint-agent.md:
- Around line 21-25: Replace the invalid use of the "go fmt -l ." command with
the correct "gofmt -l ." wherever it appears (the occurrences that currently
read "go fmt -l .", including the Validation Flow and the duplicate block later)
so the documentation invokes the tool that accepts the -l flag; search for the
literal string "go fmt -l ." and change it to "gofmt -l ." to fix the
lint-agent.md commands.
In @.claude/agents/test-agent.md:
- Around line 29-39: The targeted-test flow fails when CHANGED_FILES is empty
because xargs dirname runs on no input; add a guard after computing
CHANGED_FILES (or before computing PACKAGES) to detect an empty value and
skip/exit successfully (e.g., check [ -z "$CHANGED_FILES" ] and echo/exit 0) so
PACKAGES and the for pkg in $PACKAGES loop are not executed on docs-only
changes; update the snippet around CHANGED_FILES, PACKAGES, and the for loop to
implement this guard.
In @.claude/hooks/README.md:
- Line 334: The README entry showing a pinned Prek version is out of sync:
update the documentation string that currently shows "v0.3.9" (the line with
"cat .prek-version") to match the actual .prek-version content "v0.4.1" so the
README and the .prek-version file are consistent; modify the README.md text to
display v0.4.1 (or alternatively update .prek-version if the doc is
authoritative) and commit the change.
- Line 182: Update the README to reflect the actual smart-triggering behavior:
clarify that by default the stop hook (see stop-prek-validation.sh) only runs
`prek run --all-files` when there are uncommitted changes, and that the "runs on
every turn" behavior only occurs in strict mode when CLAUDE_LINT_ON_STOP=true;
reference the stop-prek-validation.sh script and the CLAUDE_LINT_ON_STOP env var
so readers can locate the implementation and the strict vs default behavior.
In @.claude/skills/prow-ci/analyze_failure.py:
- Around line 22-24: The loop currently uses root.findall('.//testsuite') which
omits the root when the document root is a <testsuite>; change the iteration to
include the root node when root.tag == 'testsuite' (e.g. build a suites list
like: include root if root.tag == 'testsuite' otherwise extend with
root.findall('.//testsuite')) and then iterate over that suites list to find
testcase elements (still using testcase = testsuite.findall('.//testcase') or
.findall('testcase') as appropriate) so top-level <testsuite> documents are
processed.
- Around line 11-18: The parse_junit_xml function currently uses
xml.etree.ElementTree.parse on untrusted CI artifacts; either harden it by
switching to a secure parser from defusedxml (e.g.,
defusedxml.ElementTree.parse) and add a runtime dependency check/clear error if
defusedxml is missing, or guard/remove the JUnit path so it never tries to parse
absent/untrusted files (e.g., skip when fetch_prow_artifacts does not download
junit files). Update parse_junit_xml and any callers to use
defusedxml.ElementTree (or early-return/log when junit*.xml not present) and
ensure the package requirement is listed so defusedxml is available at runtime.
In @.claude/skills/prow-ci/fetch_prow_artifacts.py:
- Around line 78-93: The fetcher currently only pulls prowjob.json and
build-log.txt (in fetch_prowjob_json and fetch_build_log) so analyze_failure.py
never sees artifacts/**/junit*.xml; update the fetch step to also download the
artifacts tree (or at minimum all files matching artifacts/**/junit*.xml) from
GCS under gcs_base_path (e.g., implement a new
download_artifacts_or_download_junit_matching function invoked from the same
place as fetch_prowjob_json/fetch_build_log) so the JUnit XMLs are present for
analyze_failure.py, or alternatively remove the JUnit-specific claims from the
skill if you prefer to not fetch artifacts. Ensure the change references the
same gcs_base_path/output_dir convention and uses the existing download_from_gcs
or a recursive GCS listing/downloader to retrieve artifacts/**/junit*.xml.
- Around line 130-139: The script's main() always returns 0 even if
fetch_build_log(parsed['gcs_base_path'], output_dir) fails; change the control
flow in main() so that when fetch_build_log(...) returns False you return a
non-zero exit code (e.g., return 1 or call sys.exit(1)) instead of continuing to
print success and returning 0; update the logic around the fetch_build_log call
and the final return so callers receive a failure status when the build-log.txt
download fails.
- Around line 61-75: The download_from_gcs function should also catch
FileNotFoundError (in addition to subprocess.CalledProcessError) and return
False with a clear stderr warning so the script won't crash if gcloud is
missing; update the download_from_gcs function to catch FileNotFoundError and
print a similar warning message to stderr. Also update main() to treat failures
of essential artifact downloads (specifically the prowjob.json and build-log.txt
download attempts) as fatal by returning a non-zero exit code when those
downloads fail instead of always returning 0; adjust the logic that calls
download_from_gcs for those two artifacts so main exits with non-zero on
failure. Finally, add a short note (or comment) near the analyze_failure.py
reference or SKILL.md mention to clarify that JUnit XMLs are optional and won't
be reported unless downloaded.
In `@CONTRIBUTING.md`:
- Around line 7-8: Update the Go version requirement in CONTRIBUTING.md so it
matches the repository's go.mod declaration: change the current "Go 1.22.7+"
text to reflect "go 1.25.9" (or "Go 1.25.9+") as declared in go.mod; ensure the
install step text ("Setup: Install Go ...") and any other references in
CONTRIBUTING.md are updated to the same version string to keep docs consistent
with go.mod.
In `@hack/prek.ci.toml`:
- Around line 55-62: The CI hook "rbac-wildcard-check" currently runs the
Makefile target rbac-wildcard-check which always exits 0, so wildcard RBAC
findings do not block CI; update the rbac-wildcard-check Makefile target to
detect violations and exit with a non-zero status when any are found (so the
hook fails) and ensure the hook in hack/prek.ci.toml still references the same
entry; alternatively, if you want warn-only behavior, update
.claude/hooks/README.md to change the documented "Action: BLOCK commit" to
reflect that the check is warn-only—pick one approach and implement it
consistently (Makefile target name: rbac-wildcard-check; docs file:
.claude/hooks/README.md).
In `@TESTING.md`:
- Around line 132-148: The markdown has duplicated code fences around the test
block which breaks rendering; fix by keeping a single opening fence ("```go")
before the It("should reconcile resource", func() { ... }) block and a single
closing fence ("```") after the closing parenthesis, removing the
extra/duplicate opening and closing backticks so the test snippet (the It(...)
test that creates resource, calls reconciler.Reconcile, and asserts
Status.Conditions) is enclosed in exactly one proper Go code fence.
---
Nitpick comments:
In @.claude/hooks/stop-prek-validation.sh:
- Line 21: The script currently enables strict mode with "set -uo pipefail" but
omits "-e", so add "-e" to make it "set -euo pipefail" to enable fail-fast
behavior; after adding this, audit the stop-prek-validation.sh script for any
commands that intentionally may return non-zero and make them explicit (e.g.,
append "|| true" or wrap in conditionals) — pay special attention to the
existing jq checks and the prek exit-code handling to ensure those still behave
as intended.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: bde6f7ee-d59b-48a4-ba96-a91b03f67e41
📒 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
devppratik
force-pushed
the
add-claude-and-docs
branch
from
6f9f342 to
c618f25
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.claude/skills/prow-ci/SKILL.md (2)
80-80: ⚡ Quick winSimplify the directory change command.
The command
cd $PWD/.claude/skills/prow-ciis unnecessarily complex. Since$PWDexpands to the current directory's absolute path, this could be confusing. Use a simpler relative path instead.📝 Proposed fix
-cd $PWD/.claude/skills/prow-ci +cd .claude/skills/prow-ciOr clarify the starting directory assumption:
+# From repository root: -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, The cd invocation uses an unnecessary $PWD prefix; replace the command `cd $PWD/.claude/skills/prow-ci` with a simple relative change into the skills directory (e.g., `cd .claude/skills/prow-ci`) or add a brief note clarifying the expected starting directory before using the absolute path, updating the line where the shell command appears so the intent is clear.
78-83: ⚡ Quick winConsider documenting Python requirements.
The documentation references Python scripts (
fetch_prow_artifacts.py,analyze_failure.py) but doesn't specify Python version requirements or dependencies. Users may encounter import errors if dependencies aren't installed.📋 Suggested addition
Add a Python requirements note in the Prerequisites section (after line 20):
**Python Requirements**: - Python 3.7 or later - Dependencies: Install with `pip install -r .claude/skills/prow-ci/requirements.txt` (if requirements file exists)Or add inline before first script usage (around line 78):
### Step 2: Fetch and Analyze **Prerequisites**: Ensure Python 3.7+ is installed and any required packages are available. Run the fetch script first:🤖 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 78 - 83, Update the SKILL.md doc to document Python requirements: add a brief "Python Requirements" note in the Prerequisites section (or immediately before the first script usage around the fetch example) that specifies Python 3.7+ and instructs users to install dependencies via pip (e.g. "pip install -r .claude/skills/prow-ci/requirements.txt") if a requirements file exists; mention the two scripts by name (fetch_prow_artifacts.py and analyze_failure.py) so readers know which tools need the environment.
🤖 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.
Nitpick comments:
In @.claude/skills/prow-ci/SKILL.md:
- Line 80: The cd invocation uses an unnecessary $PWD prefix; replace the
command `cd $PWD/.claude/skills/prow-ci` with a simple relative change into the
skills directory (e.g., `cd .claude/skills/prow-ci`) or add a brief note
clarifying the expected starting directory before using the absolute path,
updating the line where the shell command appears so the intent is clear.
- Around line 78-83: Update the SKILL.md doc to document Python requirements:
add a brief "Python Requirements" note in the Prerequisites section (or
immediately before the first script usage around the fetch example) that
specifies Python 3.7+ and instructs users to install dependencies via pip (e.g.
"pip install -r .claude/skills/prow-ci/requirements.txt") if a requirements file
exists; mention the two scripts by name (fetch_prow_artifacts.py and
analyze_failure.py) so readers know which tools need the environment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9ef3833f-f58b-4f81-8bd7-d87e948aadcd
📒 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 due to trivial changes (2)
- .claude/skills/README.md
- .claude/hooks/README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- .claude/hooks/pre-edit.sh
- .claude/hooks/stop-prek-validation.sh
- .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
c618f25 to
df970bc
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
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 7: The top-level Markdown heading "# Prow CI Analysis for " is incomplete
and renders broken; update that heading in SKILL.md to include the missing
subject (e.g., "# Prow CI Analysis for <project/name>" or a generic complete
title like "# Prow CI Analysis") so the page title is a full phrase and renders
correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 35ddff82-6136-4c93-8f73-335f0fed8ab0
📒 Files selected for processing (11)
.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
✅ Files skipped from review due to trivial changes (3)
- .claude/hooks/README.md
- .claude/agents/README.md
- .claude/agents/ci-agent.md
- 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 |
Contributor
✅ 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
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.claude/skills/prow-ci/fetch_prow_artifacts.py (1)
61-75:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCatch
FileNotFoundErrorwhen gcloud CLI is missing.When
gcloudis not installed,subprocess.run()raisesFileNotFoundError, which crashes the script with an unclear traceback instead of a helpful error message.🛠️ Proposed fix
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) return True + except FileNotFoundError: + print("Error: gcloud CLI not found. Install Google Cloud SDK first.", 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 61 - 75, download_from_gcs currently only catches subprocess.CalledProcessError so a missing gcloud executable (which raises FileNotFoundError) will crash; update download_from_gcs to also catch FileNotFoundError and print a clear stderr message (including the gcs_path and instruction that gcloud CLI is not found) then return False, keeping existing behavior for CalledProcessError in the except block or via a combined except for (subprocess.CalledProcessError, FileNotFoundError) to ensure the function never raises when gcloud is missing.
🤖 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 33-57: The python fallback currently embeds $FILE and $REPO_ROOT
directly into the -c string which allows command injection; change the
python3/python invocations to pass the paths as safe shell arguments (e.g., use
sys.argv) instead of interpolating them into the code string so that CANONICAL
is computed from python using argv[1] and argv[2]; update the two places that
call python3 -c and python -c to call python with a short script that reads
sys.argv (or use a here-doc) and ensure FILE and REPO_ROOT are passed as quoted
positional parameters to avoid shell interpretation, keeping the rest of the
surrounding CANONICAL assignment and checks intact.
In @.claude/hooks/stop-prek-validation.sh:
- Around line 79-88: Replace whitespace-splitting file handling with
null-delimited output and null-safe xargs: produce CHANGED_FILES using git diff
--name-only -z and git ls-files --others --exclude-standard -z (instead of plain
git commands), and when invoking prek use xargs -0 (or pipe the null-delimited
CHANGED_FILES via printf '%s\0' into xargs -0) so the PREK_OUTPUT assignment
that currently does echo "$CHANGED_FILES" | xargs prek run --config
hack/prek.ci.toml --files 2>&1 becomes null-safe; keep the same empty-check
logic around CHANGED_FILES/PREK_OUTPUT but ensure you handle the null-delimited
form correctly when testing for empty.
---
Outside diff comments:
In @.claude/skills/prow-ci/fetch_prow_artifacts.py:
- Around line 61-75: download_from_gcs currently only catches
subprocess.CalledProcessError so a missing gcloud executable (which raises
FileNotFoundError) will crash; update download_from_gcs to also catch
FileNotFoundError and print a clear stderr message (including the gcs_path and
instruction that gcloud CLI is not found) then return False, keeping existing
behavior for CalledProcessError in the except block or via a combined except for
(subprocess.CalledProcessError, FileNotFoundError) to ensure the function never
raises when gcloud is missing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3c96e500-db36-4185-94f2-a335ed68665d
📒 Files selected for processing (6)
.claude/hooks/pre-edit.sh.claude/hooks/stop-prek-validation.sh.claude/skills/prow-ci/SKILL.md.claude/skills/prow-ci/analyze_failure.py.claude/skills/prow-ci/fetch_prow_artifacts.py.gitleaks.toml
✅ Files skipped from review due to trivial changes (1)
- .claude/skills/prow-ci/SKILL.md
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
5 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