docs, feat(SREP-4460, SREP-4926: Add Standardized Claude hooks, skill, agents. Update standardised docs)#483
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. |
|
Warning Review limit reached
More reviews will be available in 9 minutes and 51 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (21)
WalkthroughThis PR adds comprehensive Claude Code integration documentation and validation infrastructure for the Cloud Ingress operator repository. It establishes reusable agent specifications for automated linting, testing, security scanning, docs maintenance, and CI validation; implements validation hooks to prevent dangerous edits and enforce pre-commit checks; configures gitleaks secret scanning; and provides developer guides for contributing, developing, and testing. ChangesClaude Agent Framework & Specifications
Validation Hooks & Configuration
Skills Framework & Developer Documentation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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
| ## Output Format | ||
|
|
||
| When updating docs, report: | ||
| ``` |
There was a problem hiding this comment.
Add a language tag to the fenced example block.
Line 188 uses an unlabeled fenced block, which triggers markdownlint (MD040).
Suggested doc patch
-```
+```text
Updated: DEVELOPMENT.md
- Added section on new make target: go-bench
- Fixed typo in test commands
- Updated Go version requirement: 1.22.7 -> 1.24.0
@@
- Links checked</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 188-188: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 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.
| CHANGED_FILES=$(git diff --name-only HEAD | grep "\.go$") | ||
|
|
||
| # Extract packages | ||
| PACKAGES=$(echo "$CHANGED_FILES" | xargs -n1 dirname | sort -u | tr '\n' ' ') | ||
|
|
||
| # Run targeted tests | ||
| for pkg in $PACKAGES; do | ||
| go test -v ./$pkg/... | ||
| done |
There was a problem hiding this comment.
Harden changed-package detection to avoid empty-input and diff-base failures.
Line 31 uses a diff form that often won’t represent intended PR deltas, and Line 34 can fail when no .go files changed. This makes the agent workflow brittle in normal use.
Suggested doc patch
-CHANGED_FILES=$(git diff --name-only HEAD | grep "\.go$")
+CHANGED_FILES=$(git diff --name-only --cached | grep "\.go$" || true)
-# Extract packages
-PACKAGES=$(echo "$CHANGED_FILES" | xargs -n1 dirname | sort -u | tr '\n' ' ')
+# Extract packages (safe when empty)
+PACKAGES=$(printf "%s\n" "$CHANGED_FILES" | sed '/^$/d' | xargs -r -n1 dirname | sort -u)
# Run targeted tests
-for pkg in $PACKAGES; do
- go test -v ./$pkg/...
+for pkg in $PACKAGES; do
+ go test -v "./$pkg/..."
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/test-agent.md around lines 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).
| - **On every turn**: Stop hook runs `prek run --all-files` | ||
| - **On commit**: Pre-commit hook runs relevant checks |
There was a problem hiding this comment.
Automatic validation behavior is documented incorrectly.
Line 182 states “On every turn: Stop hook runs prek run --all-files”, but the hook currently skips when there are no changes (default mode) and runs with --config hack/prek.ci.toml. Please update this section to match the script behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/hooks/README.md around lines 182 - 183, 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.
| "Bash(git commit --no-verify)", | ||
| "Bash(git push --force origin master)", | ||
| "Bash(git push --force origin main)", | ||
| "Bash(rm -rf /)", | ||
| "Bash(chmod 777 *)" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Claude Code .claude/settings.jsonpermissions, how areBash(...)deny patterns matched (exact vs wildcard), and what is the recommended way to block dangerous flag variants like--no-verify, --force, and rm -rf across argument permutations?
💡 Result:
Answer (for 2026-05-29) 1) How Bash(...) deny patterns are matched (exact vs wildcard) - Permission rules use either: a) Exact matches: Bash(<command>) matches the exact command string. b) Wildcard matches: * inside the parentheses matches a sequence of characters (a glob-style pattern) and can appear anywhere in the pattern, not just at the end (example: Bash(* install)). [1][2] c) Legacy prefix-style: :<*>-style “prefix matching” exists for backwards compatibility (documented as a separate syntax), while the newer * glob syntax is the general approach. [2][1] - Rule precedence: deny rules are evaluated first, and the first matching rule wins (“deny -> ask -> allow”). This means a deny match will block even if an allow rule also matches. [3][1] - Tool/context behavior: rules can be scoped to a tool call (e.g., Bash(rm *)), and a bare tool name like Bash removes the tool from Claude’s context entirely. [3] Important limitations / security caveats (relevant to how reliable wildcard matching is in practice) - Several open issues report that Bash pattern matching/denying can fail to trigger for syntactic variants such as: - Commands spanning multiple lines / heredocs (wildcards may not be enforced consistently). [4] - Variable-expansion syntax such as ${VAR} can bypass glob substring deny patterns like Bash(*phi*). [5] - Heredoc/redirection syntax can bypass deny patterns that otherwise should match. [6] - There are also reports of internal matcher/path-scoped wildcard behavior that can differ from what users expect in certain constructed examples. [7] So, while the documentation describes glob wildcard semantics, real-world enforcement has known bypass cases. 2) Recommended way to block dangerous flag variants like --no-verify, --force, and rm -rf across argument permutations Given both the documented semantics and the reported bypasses, the safest recommended configuration pattern is: - Deny the Bash tool broadly, then explicitly allow only the safe subset of commands you want. - The docs confirm deny precedence and matching behavior. [3][1] - Community guidance also recommends using Bash(*) with a focused deny list for the dangerous commands/arguments, rather than a fragmented allow list (because it reduces gaps during “argument permutation” drift). [8] - Use multiple deny patterns that cover common renderings/permutations for each dangerous category, because there is no single documented “argument token” operator—matching is against the overall command string with glob wildcards. [1][3] Concrete deny patterns to use (baseline) A) Block rm destructive forms - Deny plain rm with wildcard coverage of options/targets: - Bash(rm *) - Bash(rm -rf *) - Bash(rm -r *) - Bash(rm -rf ~*) - Bash(rm -rf /*) These are aligned with the general rule that wildcard * can match arbitrary character sequences. [1][3] B) Block dangerous “force” / “no-verify” flags - Add deny patterns that match the presence of the flag text anywhere in the command: - Bash(* --no-verify *) - Bash(*--no-verify*) - Bash(* --force *) - Bash(*--force*) - (Optionally) Bash(* -f *) only if you are sure you won’t block safe uses of -f for your allowed command set; -f is extremely overloaded. This follows the documented use of * wildcard anywhere in the pattern. [1][2] C) Block rm variants even when preceded by other tokens - Deny patterns that include leading command chains/prefixes are hard to do perfectly, but you can defensively deny rm anywhere by using * rm ... *-style patterns: - Bash(* rm * -rf *) - Bash(* rm * --no-preserve-root *) (if your workflow might encounter it) However: don’t rely on these patterns alone against bypass cases - Multiple reported bugs show that certain syntactic transformations can cause deny patterns to not match (notably heredocs/redirection patterns and ${VAR} variable expansion). [4][5][6] Therefore, for strong enforcement across permutations, prefer a policy that does not require correct matching of every syntactic variant: - Use defaultMode: "dontAsk" or otherwise restrict who can change permissions, but more importantly: - Use an allowlist that is as small and tool-specific as possible, and keep Bash(*) allowed only if you can tolerate the remaining known bypasses; otherwise: - Consider avoiding Bash entirely for sensitive operations by steering those operations into Read/Write/Edit tool calls where possible. The docs describe how denial at any scope prevents allowance at other scopes, which is the core of deny-first security—but the known matcher bugs show Bash deny isn’t perfectly robust. [1][3] If you want a practical “starter” permissions.json Example (deny-by-default Bash, allow only safe Bash commands): { "permissions": { "defaultMode": "dontAsk", "allow": [ "Read()", "Write()", "Edit(*)", "Bash(npm *)", "Bash(git *)", "Bash(ls *)", "Bash(mkdir )", "Bash(cd )", "Bash(pwd )" ], "deny": [ "Bash(rm )", "Bash(rm -rf )", "Bash(rm -rf /)", "Bash( --no-verify )", "Bash(--no-verify)", "Bash( --force )", "Bash(--force)" ] } } This uses deny precedence and wildcard matching semantics as documented. [3][1] Recommendation summary - Matching semantics: Bash(<exact>) is exact; * wildcard patterns are glob-like and can appear anywhere; deny rules win first. [1][2][3] - Recommended blocking strategy: prefer deny-first or deny-by-default for Bash, and cover dangerous flags with multiple glob variants (with and without surrounding spaces). [1][8] - Critical caveat: there are known bypass cases (heredocs/multiline, ${VAR} expansions, redirection/heredoc syntax) where deny patterns may fail, so treat Bash deny as “best effort,” and minimize Bash usage for high-risk operations. [5][4][6] If you tell me whether your intent is (a) “allow most Bash but block only these” or (b) “block Bash except a safe allowlist,” I can produce a tighter ruleset tailored to your workflow.
Citations:
- 1: https://code.claude.com/docs/en/settings
- 2: [DOCS] Documentation Contradiction: Bash Permission Matching Logic (Prefix vs Wildcard) anthropics/claude-code#18961
- 3: https://code.claude.com/docs/en/permissions
- 4: Bash permission wildcards don't match multiline/heredoc commands anthropics/claude-code#25441
- 5: [BUG] Bash deny pattern bypass via ${VAR} curly-brace variable expansion anthropics/claude-code#23924
- 6: Deny list patterns don't match commands with heredoc syntax anthropics/claude-code#23314
- 7: Path-scoped Bash deny rules silently fail to match (e.g.
Bash(grep * /etc/*)does not blockgrep root /etc/passwd) anthropics/claude-code#56816 - 8: https://rajiv.com/blog/2026/03/31/stop-asking-me-configuring-claude-code-permissions-for-uninterrupted-flow/
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".claude/settings.json"
if [ ! -f "$FILE" ]; then
echo "Missing $FILE"
exit 1
fi
nl -ba "$FILE" | sed -n '1,120p'Repository: openshift/cloud-ingress-operator
Length of output: 119
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE=".claude/settings.json"
if [ ! -f "$FILE" ]; then
echo "Missing $FILE"
exit 1
fi
cat -n "$FILE" | sed -n '1,120p'Repository: openshift/cloud-ingress-operator
Length of output: 1767
Harden deny patterns to block unsafe Bash argument permutations
The deny entries in .claude/settings.json only match the exact command strings shown (e.g., Bash(git commit --no-verify), Bash(git push --force origin main), Bash(rm -rf /)), so variants like git commit -m "x" --no-verify, git push --force-with-lease ..., and rm -rf . can bypass.
Expand the deny patterns to use * globs for the dangerous flags/targets, e.g.:
Bash(git commit * --no-verify *)andBash(git commit *--no-verify*)Bash(git push * --force* *)(optionally scoped toorigin master/main)- Replace
Bash(rm -rf /)with broader forms likeBash(rm -rf *)(and/orBash(rm -r *))
🤖 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/settings.json around lines 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.
| paths = [ | ||
| '''test/fixtures/.*''', | ||
| '''test/deploy/.*''', | ||
| '''.*_test\.go''', | ||
| '''boilerplate/.*''', | ||
| '''vendor/.*''', | ||
| '''zz_generated\..*\.go''', | ||
| ] |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In gitleaks config, does a regex listed under [allowlist].paths fully skip scanning of matching files (for all rules)?
💡 Result:
Yes—when you use an allowlist entry with a regex under [allowlist].paths (or [[allowlists]].paths in newer configs), gitleaks will ignore findings whose file path matches that regex. This suppression applies to the rule(s) targeted by that allowlist: if the allowlist is global (no targetRules/targetRules matches), it suppresses matching findings for all rules; if targetRules is set, it suppresses findings only for those specified rules (not necessarily by fully skipping scanning per rule). Evidence: the configuration docs say that allowlists can match on commits/regexes/paths and that “A finding will be ignored if ANY [[allowlists]] matches” and that “the same logic applies for regexes and paths” [1]. The default config template also describes allowlists as instructing what is “allowed, i.e. not a secret,” consistent with suppressing findings based on paths [2]. Additionally, the issues/trace examples show that a path pattern in an allowlist results in the corresponding finding being ignored for the targeted rule(s), rather than just disabling one regex check [3]. The configuration package defines Allowlist.Paths as “path regular expressions that are allowed to be ignored” [4]. Answering your exact wording (“fully skip scanning of matching files (for all rules)”): based on the docs phrasing, it’s best to interpret “skip scanning” as “skip reporting/flagging findings” for matching paths (i.e., findings are ignored). Whether gitleaks still reads/analyzes those files internally isn’t clearly specified in the cited docs, but the externally observable behavior is that findings for those paths are ignored for the applicable rules [1][4][2].
Citations:
- 1: https://github.com/gitleaks/gitleaks
- 2: https://github.com/gitleaks/gitleaks/blob/4fb43823/cmd/generate/config/rules/config.tmpl
- 3: targetRules field in [[allowLists]] is ignored gitleaks/gitleaks#1919
- 4: https://pkg.go.dev/github.com/grachovskaia/gitleaks/config
Tighten .gitleaks.toml allowlist: remove the repo-wide .*_test\.go suppression
- The global
[allowlist].pathsentry.*_test\.gomakes gitleaks ignore findings for matching*_test.gofiles (for applicable/global allowlist matching), which can hide real secrets leaked in tests. - Keep only fixture/deploy-specific exceptions (or narrower patterns) instead.
🔧 Proposed tightening
paths = [
'''test/fixtures/.*''',
'''test/deploy/.*''',
- '''.*_test\.go''',
'''boilerplate/.*''',
'''vendor/.*''',
'''zz_generated\..*\.go''',
]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.gitleaks.toml around lines 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.
| Tests run automatically in pre-commit when Go files change: | ||
| ```yaml | ||
| - id: go-test | ||
| entry: make go-test | ||
| files: '\.go$' | ||
| ``` | ||
|
|
||
| This is NOT in current pre-commit config (too slow for pre-commit). | ||
| Run manually before pushing: `make go-test` |
There was a problem hiding this comment.
Resolve contradictory pre-commit guidance.
Lines 266-271 state go-test runs automatically in pre-commit, but Line 273 says it is not in the current config. Keep only one truth path here.
Suggested patch
-Tests run automatically in pre-commit when Go files change:
-```yaml
-- id: go-test
- entry: make go-test
- files: '\.go$'
-```
-
-This is NOT in current pre-commit config (too slow for pre-commit).
-Run manually before pushing: `make go-test`
+`go-test` is **not** part of the current pre-commit config (too slow for pre-commit).
+Run it manually before pushing:
+
+```bash
+make go-test
+```🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@TESTING.md` around lines 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.
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
- Add .claude/ directory with agents, hooks, and skills - Add prek validation framework (prek.toml, hack/prek.ci.toml) - Add gitleaks secret scanning (.gitleaks.toml) - Add CONTRIBUTING.md, DEVELOPMENT.md, TESTING.md - Add CLAUDE.md (if not already present) - Add stop hook for automatic validation Based on ocm-agent-operator PR openshift#257 (SREP-4410, SREP-4411) Brings the repo up to Agentic SDLC contribution standard
devppratik
force-pushed
the
add-claude-and-docs
branch
from
60dd6d9 to
a540956
Compare
|
@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. |
2 participants
What type of PR is this?
docs/feature
What this PR does / why we need it?
References
Summary by CodeRabbit
Documentation
Chores