feat(SREP-4410): Update Claude configuration from MUO PR #641#288
feat(SREP-4410): Update Claude configuration from MUO PR #641#288devppratik wants to merge 1 commit into
Conversation
Apply additional improvements from managed-upgrade-operator PR #641 that were merged after the initial ocm-agent-operator PR openshift#257. New files: - .claude/hooks/session-start-prek-setup.sh: Auto-setup prek on session start - .claude/skills/prow-ci/analyze_failure.py: Automated CI failure analysis - .claude/skills/prow-ci/fetch_prow_artifacts.py: Fetch Prow artifacts from GCS Updated files: - .claude/hooks/pre-edit.sh: Enhanced path validation and security checks - .claude/hooks/stop-prek-validation.sh: Improved change detection logic - .claude/settings.json: Added SessionStart hook and enhanced git safety - .claude/skills/prow-ci/SKILL.md: Enhanced with artifact analysis workflow - .claude/hooks/README.md: Updated hook behavior documentation - .gitleaks.toml: Enhanced test file patterns and custom rules - .gitignore: Added .work/ for prow-ci skill artifacts All repo-specific references updated for ocm-agent-operator. Source: openshift/managed-upgrade-operator#641 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
WalkthroughThis PR enhances Claude Code's developer experience by automating prek hook setup, refining path validation in file edits, implementing conditional changed-file-only validation, and introducing a new Prow CI failure analysis skill with artifact fetching and pattern-based log analysis. ChangesClaude Code Infrastructure and Prow CI Skill
🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
|
[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 #288 +/- ##
=======================================
Coverage 66.04% 66.04%
=======================================
Files 23 23
Lines 1546 1546
=======================================
Hits 1021 1021
Misses 447 447
Partials 78 78 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 53-56: The traversal check currently rejects any path containing
the substring ".." (using CANONICAL) and wrongly blocks valid filenames; change
the condition to detect ".." only as a path segment (e.g. leading, trailing, or
between slashes). Replace the boolean test that references CANONICAL with a
pattern or regex match that looks for '(^|/)..(/|$)' so it only flags true
directory-traversal segments, and keep the same echo using FILE and exit 1 on
match.
- Around line 45-50: The CANONICAL assignment is unsafe because it injects $FILE
into an inline Python string and only uses normpath (which doesn't resolve
symlinks); change the two python invocations so they pass "$FILE" and
"$REPO_ROOT" as command-line arguments (not embedded in the -c string) and have
the Python snippet call os.path.realpath on both inputs then os.path.relpath on
the realpath values (use try/except and write to stdout or return empty on
error) before assigning to CANONICAL; update both the python3 and python
fallback invocations (the lines creating CANONICAL) to use this safer
argument-passing + realpath-based resolution.
In @.claude/hooks/stop-prek-validation.sh:
- Around line 23-29: The script calls jq (the command `jq -n '{"decision":
"block", "reason": "Not in a git repository. Cannot run prek validation."}'`)
before verifying jq exists, causing a shell error if jq is missing; change the
logic to check for jq availability (e.g., `command -v jq` or `type jq`) before
any `jq` invocation and if jq is not found produce the same hook JSON using a
plain shell fallback (echo the JSON string) or emit a deterministic JSON error
message; update the branch that uses REPO_ROOT and replace the direct `jq -n
...` call with a guarded call that uses jq when available and the fallback when
not.
- Around line 80-87: The script currently builds CHANGED_FILES and pipes it
through xargs which splits on whitespace and breaks filenames with
spaces/newlines; change the collection to NUL-separated (use git diff
--name-only -z and git ls-files --others --exclude-standard -z) and invoke prek
with xargs -0 (or use printf '%s' "$CHANGED_FILES" | xargs -0 ...) so filenames
are passed safely; update the CHANGED_FILES assignment and the PREK_OUTPUT
branch that calls xargs, keeping the existing fallback path (prek run
--all-files) for the empty-case logic and preserving the PREK_OUTPUT capture.
In @.claude/skills/prow-ci/analyze_failure.py:
- Around line 200-203: The current write to args.output in analyze_failure.py
can raise FileNotFoundError when parent directories don't exist; before opening
the file in the block that references args.output, ensure the parent directory
exists by computing parent = os.path.dirname(args.output) and calling
os.makedirs(parent, exist_ok=True) when parent is non-empty, then proceed to
open and write; update the block that contains the with open(args.output, 'w')
as f: ... logic to perform this directory creation step first.
In @.claude/skills/prow-ci/fetch_prow_artifacts.py:
- Around line 63-75: The try/except around subprocess.run that downloads from
GCS currently only catches subprocess.CalledProcessError, so a missing gcloud
binary raises FileNotFoundError and crashes; update the block around
subprocess.run (the download routine that builds cmd and calls subprocess.run)
to also catch FileNotFoundError (or add a separate except FileNotFoundError) and
handle it by printing a clear warning like "gcloud not found" (including the
gcs_path context) to stderr and returning False so the function fails
gracefully.
🪄 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: 09f7b4ac-35d0-4e55-8cfa-4a92b97aa2b6
📒 Files selected for processing (12)
.claude/agents/README.md.claude/hooks/README.md.claude/hooks/pre-edit.sh.claude/hooks/session-start-prek-setup.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
| if [[ -z "$CANONICAL" ]]; then | ||
| if command -v python3 >/dev/null 2>&1; then | ||
| CANONICAL=$(python3 -c "import os.path; print(os.path.relpath(os.path.normpath('$FILE'), '$REPO_ROOT'))" 2>/dev/null || echo "") | ||
| elif command -v python >/dev/null 2>&1; then | ||
| CANONICAL=$(python -c "import os.path; print(os.path.relpath(os.path.normpath('$FILE'), '$REPO_ROOT'))" 2>/dev/null || echo "") | ||
| fi |
There was a problem hiding this comment.
Harden Python fallback to avoid path-based code injection and symlink escape.
Line 47 and Line 49 embed $FILE directly into Python code. A crafted filename containing quotes can break out of the string and execute arbitrary Python. Also, normpath alone doesn’t resolve symlinks, so out-of-repo symlink targets can be misclassified.
Suggested fix
- if command -v python3 >/dev/null 2>&1; then
- CANONICAL=$(python3 -c "import os.path; print(os.path.relpath(os.path.normpath('$FILE'), '$REPO_ROOT'))" 2>/dev/null || echo "")
- elif command -v python >/dev/null 2>&1; then
- CANONICAL=$(python -c "import os.path; print(os.path.relpath(os.path.normpath('$FILE'), '$REPO_ROOT'))" 2>/dev/null || echo "")
+ if command -v python3 >/dev/null 2>&1; then
+ CANONICAL=$(python3 - "$REPO_ROOT" "$FILE" <<'PY' 2>/dev/null || echo ""
+import os, sys
+repo_root = os.path.realpath(sys.argv[1])
+raw = sys.argv[2]
+target = raw if os.path.isabs(raw) else os.path.join(repo_root, raw)
+target = os.path.realpath(target)
+print(os.path.relpath(target, repo_root))
+PY
+)
+ elif command -v python >/dev/null 2>&1; then
+ CANONICAL=$(python - "$REPO_ROOT" "$FILE" <<'PY' 2>/dev/null || echo ""
+import os, sys
+repo_root = os.path.realpath(sys.argv[1])
+raw = sys.argv[2]
+target = raw if os.path.isabs(raw) else os.path.join(repo_root, raw)
+target = os.path.realpath(target)
+print(os.path.relpath(target, repo_root))
+PY
+)
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [[ -z "$CANONICAL" ]]; then | |
| if command -v python3 >/dev/null 2>&1; then | |
| CANONICAL=$(python3 -c "import os.path; print(os.path.relpath(os.path.normpath('$FILE'), '$REPO_ROOT'))" 2>/dev/null || echo "") | |
| elif command -v python >/dev/null 2>&1; then | |
| CANONICAL=$(python -c "import os.path; print(os.path.relpath(os.path.normpath('$FILE'), '$REPO_ROOT'))" 2>/dev/null || echo "") | |
| fi | |
| if [[ -z "$CANONICAL" ]]; then | |
| if command -v python3 >/dev/null 2>&1; then | |
| CANONICAL=$(python3 - "$REPO_ROOT" "$FILE" <<'PY' 2>/dev/null || echo "" | |
| import os, sys | |
| repo_root = os.path.realpath(sys.argv[1]) | |
| raw = sys.argv[2] | |
| target = raw if os.path.isabs(raw) else os.path.join(repo_root, raw) | |
| target = os.path.realpath(target) | |
| print(os.path.relpath(target, repo_root)) | |
| PY | |
| ) | |
| elif command -v python >/dev/null 2>&1; then | |
| CANONICAL=$(python - "$REPO_ROOT" "$FILE" <<'PY' 2>/dev/null || echo "" | |
| import os, sys | |
| repo_root = os.path.realpath(sys.argv[1]) | |
| raw = sys.argv[2] | |
| target = raw if os.path.isabs(raw) else os.path.join(repo_root, raw) | |
| target = os.path.realpath(target) | |
| print(os.path.relpath(target, repo_root)) | |
| PY | |
| ) | |
| fi |
🤖 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/pre-edit.sh around lines 45 - 50, The CANONICAL assignment is
unsafe because it injects $FILE into an inline Python string and only uses
normpath (which doesn't resolve symlinks); change the two python invocations so
they pass "$FILE" and "$REPO_ROOT" as command-line arguments (not embedded in
the -c string) and have the Python snippet call os.path.realpath on both inputs
then os.path.relpath on the realpath values (use try/except and write to stdout
or return empty on error) before assigning to CANONICAL; update both the python3
and python fallback invocations (the lines creating CANONICAL) to use this safer
argument-passing + realpath-based resolution.
| if [[ -z "$CANONICAL" ]] || [[ "$CANONICAL" == *".."* ]]; then | ||
| echo "❌ ERROR: Invalid file path (contains traversal): $FILE" | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Traversal check is too broad and blocks legitimate filenames.
Line 53 rejects any path containing .. as a substring, including valid names like docs/v1..notes.md. Match traversal as a segment instead.
Suggested fix
-if [[ -z "$CANONICAL" ]] || [[ "$CANONICAL" == *".."* ]]; then
+if [[ -z "$CANONICAL" ]] || [[ "$CANONICAL" =~ (^|/)\.\.(/|$) ]]; then📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [[ -z "$CANONICAL" ]] || [[ "$CANONICAL" == *".."* ]]; then | |
| echo "❌ ERROR: Invalid file path (contains traversal): $FILE" | |
| exit 1 | |
| fi | |
| if [[ -z "$CANONICAL" ]] || [[ "$CANONICAL" =~ (^|/)\.\.(/|$) ]]; then | |
| echo "❌ ERROR: Invalid file path (contains traversal): $FILE" | |
| exit 1 | |
| fi |
🤖 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/pre-edit.sh around lines 53 - 56, The traversal check
currently rejects any path containing the substring ".." (using CANONICAL) and
wrongly blocks valid filenames; change the condition to detect ".." only as a
path segment (e.g. leading, trailing, or between slashes). Replace the boolean
test that references CANONICAL with a pattern or regex match that looks for
'(^|/)..(/|$)' so it only flags true directory-traversal segments, and keep the
same echo using FILE and exit 1 on match.
| # Ensure we're running from the git repository root | ||
| # This handles cases where Claude Code's CWD is in a subdirectory (e.g., .claude/skills/) | ||
| REPO_ROOT=$(git rev-parse --show-toplevel 2>/dev/null) | ||
| if [[ -z "$REPO_ROOT" ]]; then | ||
| jq -n '{"decision": "block", "reason": "Not in a git repository. Cannot run prek validation."}' | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
Do not call jq before verifying it exists.
Line 27 invokes jq before the dependency check at Line 32. In environments without jq, this branch emits a shell error instead of deterministic hook JSON.
Suggested fix
if [[ -z "$REPO_ROOT" ]]; then
- jq -n '{"decision": "block", "reason": "Not in a git repository. Cannot run prek validation."}'
+ if command -v jq >/dev/null 2>&1; then
+ jq -n '{"decision": "block", "reason": "Not in a git repository. Cannot run prek validation."}'
+ else
+ cat <<'EOF'
+{"decision":"block","reason":"Not in a git repository. Cannot run prek validation."}
+EOF
+ fi
exit 0
fi🤖 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 around lines 23 - 29, The script calls
jq (the command `jq -n '{"decision": "block", "reason": "Not in a git
repository. Cannot run prek validation."}'`) before verifying jq exists, causing
a shell error if jq is missing; change the logic to check for jq availability
(e.g., `command -v jq` or `type jq`) before any `jq` invocation and if jq is not
found produce the same hook JSON using a plain shell fallback (echo the JSON
string) or emit a deterministic JSON error message; update the branch that uses
REPO_ROOT and replace the direct `jq -n ...` call with a guarded call that uses
jq when available and the fallback when not.
| CHANGED_FILES=$(git diff --name-only --diff-filter=d HEAD; git ls-files --others --exclude-standard) | ||
| if [[ -z "$CHANGED_FILES" ]]; then | ||
| # No files changed, but we're here because git status showed changes | ||
| # Fall back to --all-files to catch any edge cases | ||
| PREK_OUTPUT=$(prek run --all-files --config hack/prek.ci.toml 2>&1) | ||
| else | ||
| # Pass changed files explicitly to prek | ||
| PREK_OUTPUT=$(echo "$CHANGED_FILES" | xargs prek run --config hack/prek.ci.toml --files 2>&1) |
There was a problem hiding this comment.
Use NUL-safe file collection instead of xargs splitting.
Line 87 splits CHANGED_FILES on whitespace, so filenames with spaces/newlines are passed incorrectly to prek, which can skip intended files or fail unexpectedly.
Suggested fix
-CHANGED_FILES=$(git diff --name-only --diff-filter=d HEAD; git ls-files --others --exclude-standard)
-if [[ -z "$CHANGED_FILES" ]]; then
+mapfile -d '' -t CHANGED_FILES < <(
+ {
+ git diff -z --name-only --diff-filter=d HEAD 2>/dev/null || true
+ git ls-files -z --others --exclude-standard
+ }
+)
+if (( ${`#CHANGED_FILES`[@]} == 0 )); then
# No files changed, but we're here because git status showed changes
# Fall back to --all-files to catch any edge cases
PREK_OUTPUT=$(prek run --all-files --config hack/prek.ci.toml 2>&1)
else
# Pass changed files explicitly to prek
- PREK_OUTPUT=$(echo "$CHANGED_FILES" | xargs prek run --config hack/prek.ci.toml --files 2>&1)
+ PREK_OUTPUT=$(prek run --config hack/prek.ci.toml --files "${CHANGED_FILES[@]}" 2>&1)
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CHANGED_FILES=$(git diff --name-only --diff-filter=d HEAD; git ls-files --others --exclude-standard) | |
| if [[ -z "$CHANGED_FILES" ]]; then | |
| # No files changed, but we're here because git status showed changes | |
| # Fall back to --all-files to catch any edge cases | |
| PREK_OUTPUT=$(prek run --all-files --config hack/prek.ci.toml 2>&1) | |
| else | |
| # Pass changed files explicitly to prek | |
| PREK_OUTPUT=$(echo "$CHANGED_FILES" | xargs prek run --config hack/prek.ci.toml --files 2>&1) | |
| mapfile -d '' -t CHANGED_FILES < <( | |
| { | |
| git diff -z --name-only --diff-filter=d HEAD 2>/dev/null || true | |
| git ls-files -z --others --exclude-standard | |
| } | |
| ) | |
| if (( ${`#CHANGED_FILES`[@]} == 0 )); then | |
| # No files changed, but we're here because git status showed changes | |
| # Fall back to --all-files to catch any edge cases | |
| PREK_OUTPUT=$(prek run --all-files --config hack/prek.ci.toml 2>&1) | |
| else | |
| # Pass changed files explicitly to prek | |
| PREK_OUTPUT=$(prek run --config hack/prek.ci.toml --files "${CHANGED_FILES[@]}" 2>&1) | |
| fi |
🤖 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 around lines 80 - 87, The script
currently builds CHANGED_FILES and pipes it through xargs which splits on
whitespace and breaks filenames with spaces/newlines; change the collection to
NUL-separated (use git diff --name-only -z and git ls-files --others
--exclude-standard -z) and invoke prek with xargs -0 (or use printf '%s'
"$CHANGED_FILES" | xargs -0 ...) so filenames are passed safely; update the
CHANGED_FILES assignment and the PREK_OUTPUT branch that calls xargs, keeping
the existing fallback path (prek run --all-files) for the empty-case logic and
preserving the PREK_OUTPUT capture.
| if args.output: | ||
| with open(args.output, 'w') as f: | ||
| f.write(output) | ||
| print(f"Analysis saved to: {args.output}") |
There was a problem hiding this comment.
Make output file writing resilient for nested paths.
On Line 201, writing directly to args.output will fail with FileNotFoundError when parent directories don’t exist, causing an avoidable traceback.
Suggested fix
# Write output
if args.output:
- with open(args.output, 'w') as f:
- f.write(output)
- print(f"Analysis saved to: {args.output}")
+ try:
+ output_path = Path(args.output)
+ output_path.parent.mkdir(parents=True, exist_ok=True)
+ with open(output_path, 'w', encoding='utf-8') as f:
+ f.write(output)
+ print(f"Analysis saved to: {output_path}")
+ except OSError as e:
+ print(f"Error: Could not write output file {args.output}: {e}", file=sys.stderr)
+ return 1
else:
print(output)🤖 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 200 - 203, The
current write to args.output in analyze_failure.py can raise FileNotFoundError
when parent directories don't exist; before opening the file in the block that
references args.output, ensure the parent directory exists by computing parent =
os.path.dirname(args.output) and calling os.makedirs(parent, exist_ok=True) when
parent is non-empty, then proceed to open and write; update the block that
contains the with open(args.output, 'w') as f: ... logic to perform this
directory creation step first.
| 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 subprocess.CalledProcessError as e: | ||
| print(f"Warning: Could not download {gcs_path}: {e.stderr.decode()}", file=sys.stderr) | ||
| return False |
There was a problem hiding this comment.
Handle missing gcloud binary explicitly.
On Line 71, subprocess.run(...) can raise FileNotFoundError when gcloud is not installed, which currently escapes and crashes with a traceback instead of returning a controlled failure.
Suggested 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 and ensure `gcloud` is on 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🧰 Tools
🪛 Ruff (0.15.15)
[error] 71-71: subprocess call: check for execution of untrusted input
(S603)
🤖 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
try/except around subprocess.run that downloads from GCS currently only catches
subprocess.CalledProcessError, so a missing gcloud binary raises
FileNotFoundError and crashes; update the block around subprocess.run (the
download routine that builds cmd and calls subprocess.run) to also catch
FileNotFoundError (or add a separate except FileNotFoundError) and handle it by
printing a clear warning like "gcloud not found" (including the gcs_path
context) to stderr and returning False so the function fails gracefully.
|
/test ocm-agent-operator-on-pull-request |
|
/test validate |
|
@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. |
What type of PR is this?
feature/docs
What this PR does / why we need it?
Updates Claude Code configuration with improvements based on feedback for other operators to make it upto standard
Which Jira/Github issue(s) this PR fixes?
SREP-4410
Special notes for your reviewer:
All repo-specific references updated for ocm-agent-operator.
Pre-checks:
make generatecommand locally to validate code changesSummary by CodeRabbit
Release Notes
New Features
Improvements
Documentation