OKD-399: Generate REVIEW.md and coderabibit.yaml for Agentic docs - #666
OKD-399: Generate REVIEW.md and coderabibit.yaml for Agentic docs#666jatinsu wants to merge 2 commits into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jatinsu: This pull request references OKD-399 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. 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jatinsu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
WalkthroughThe ChangesAgentic Docs review artifacts
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@plugins/agentic-docs/skills/component-docs/scripts/validate.sh`:
- Around line 390-400: Update the REVIEW.md validation flow and validate_links
so link checking never modifies files: return validation failures only, and move
any link-removal or repair behavior behind an explicit user-selected fix mode.
Preserve the existing validation reporting while ensuring normal validation
cannot alter REVIEW.md.
- Around line 292-304: Update the skip-path validation loop around the skip_path
extraction to resolve each glob against REPO_PATH rather than deriving and
checking only its first path component. Normalize the glob safely, test whether
it matches at least one repository path, and emit the existing warning only when
no matches are found; preserve vendor and go.* exclusions and verbose success
reporting.
- Around line 331-338: Update the validation logic around the REVIEW.md and
CLAUDE.md checks to parse .coderabbit.yaml as YAML and read
knowledge_base.code_guidelines.filePatterns instead of grepping raw text.
Perform both presence checks against the parsed filePatterns values so template
comments cannot produce false results, preserving the existing success and
warning messages.
In `@plugins/agentic-docs/skills/component-docs/SKILL.md`:
- Around line 251-263: Add the `text` language identifier to the fenced code
block containing the mcp__chai-bot__ask_persona example, preserving its
illustrative MCP input content unchanged.
- Around line 267-305: Synchronize lockfile exclusions between REVIEW.md and
CodeRabbit: in plugins/agentic-docs/skills/component-docs/SKILL.md lines
267-305, require Phase 9.5 to translate go.sum and go.mod skip rules into
corresponding CodeRabbit path filters unless they are removed from REVIEW.md; in
plugins/agentic-docs/skills/component-docs/templates/REVIEW-template.md lines
48-65, either remove those lockfiles from “Do not report” or require their
matching filters. Keep organization-default exclusions unchanged.
In
`@plugins/agentic-docs/skills/component-docs/templates/coderabbit-template.yaml`:
- Around line 28-41: Update the reviews configuration in
coderabbit-template.yaml so path_filters and path_instructions are explicitly
empty sequences ([]), removing the placeholder-only structure. Preserve the
documented field types and validate the resulting configuration against the
schema.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1c9c93a8-8328-47f9-b2bd-bc2e14190656
📒 Files selected for processing (7)
.claude-plugin/marketplace.jsondocs/index.htmlplugins/agentic-docs/.claude-plugin/plugin.jsonplugins/agentic-docs/skills/component-docs/SKILL.mdplugins/agentic-docs/skills/component-docs/scripts/validate.shplugins/agentic-docs/skills/component-docs/templates/REVIEW-template.mdplugins/agentic-docs/skills/component-docs/templates/coderabbit-template.yaml
| while IFS= read -r skip_path; do | ||
| clean_path=$(echo "$skip_path" | sed 's/[`*]//g' | xargs) | ||
| if [ -n "$clean_path" ] && [[ "$clean_path" != vendor* ]] && [[ "$clean_path" != go.* ]]; then | ||
| base_dir=$(echo "$clean_path" | cut -d'/' -f1) | ||
| if [ -d "$REPO_PATH/$base_dir" ] || [ -f "$REPO_PATH/$base_dir" ]; then | ||
| if [ "${VERBOSE:-false}" = "true" ]; then | ||
| echo " ✅ Skip path base exists: $base_dir" | ||
| fi | ||
| else | ||
| echo " ⚠️ Skip path base not found: $base_dir (from $skip_path)" | ||
| fi | ||
| fi | ||
| done < <(grep -oP '`[^`]+\*\*[^`]*`' "$REPO_PATH/REVIEW.md" 2>/dev/null || true) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Match each skip glob instead of checking its first path component.
For **/clientset/**, Line 293 produces /clientset/. Line 295 then produces an empty base_dir, and Line 296 checks $REPO_PATH/. The validator therefore accepts a missing clientset directory. Resolve each glob against the repository and warn only when it has no matches.
🧰 Tools
🪛 Shellcheck (0.11.0)
[info] 304-304: Expressions don't expand in single quotes, use double quotes for that.
(SC2016)
🤖 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 `@plugins/agentic-docs/skills/component-docs/scripts/validate.sh` around lines
292 - 304, Update the skip-path validation loop around the skip_path extraction
to resolve each glob against REPO_PATH rather than deriving and checking only
its first path component. Normalize the glob safely, test whether it matches at
least one repository path, and emit the existing warning only when no matches
are found; preserve vendor and go.* exclusions and verbose success reporting.
| if grep -q "REVIEW.md" "$REPO_PATH/.coderabbit.yaml"; then | ||
| echo " ✅ filePatterns includes REVIEW.md" | ||
| else | ||
| echo " ⚠️ filePatterns missing REVIEW.md" | ||
| fi | ||
| if grep -q "CLAUDE.md" "$REPO_PATH/.coderabbit.yaml" 2>/dev/null; then | ||
| echo " ⚠️ filePatterns includes CLAUDE.md (auto-detected, remove)" | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file="plugins/agentic-docs/skills/component-docs/scripts/validate.sh"
printf '%s\n' '--- target file ---'
sed -n '300,350p' "$file"
printf '%s\n' '--- related filePatterns and template references ---'
rg -n -C 3 'filePatterns|REVIEW\.md|CLAUDE\.md|knowledge_base' \
plugins/agentic-docs/skills/component-docs
printf '%s\n' '--- candidate YAML parsers and validation dependencies ---'
rg -n 'yq|python|yaml|ruby|filePatterns' \
plugins/agentic-docs/skills/component-docs/scripts \
plugins/agentic-docs/skills/component-docs \
2>/dev/null | head -200Repository: openshift-eng/ai-helpers
Length of output: 29683
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
import yaml
path = Path("plugins/agentic-docs/skills/component-docs/templates/coderabbit-template.yaml")
text = path.read_text()
data = yaml.safe_load(text)
patterns = data["knowledge_base"]["code_guidelines"]["filePatterns"]
print("parsed filePatterns:", patterns)
for needle in ("REVIEW.md", "CLAUDE.md"):
print(f"grep {needle!r} on template:", bool(re.search(re.escape(needle), text)))
# Model the validator's false-positive cases without writing files.
cases = {
"missing REVIEW.md, template guidance comment retained":
"# knowledge_base.filePatterns must include REVIEW.md\n"
"knowledge_base:\n code_guidelines:\n filePatterns:\n - AGENTS.md\n",
"CLAUDE.md absent from list, prohibition comment retained":
"knowledge_base:\n code_guidelines:\n"
" # NEVER add CLAUDE.md here\n"
" filePatterns:\n - REVIEW.md\n - AGENTS.md\n",
}
for name, case in cases.items():
parsed = yaml.safe_load(case)
values = parsed["knowledge_base"]["code_guidelines"]["filePatterns"]
print(name)
print(" parsed values:", values)
for needle in ("REVIEW.md", "CLAUDE.md"):
print(f" grep {needle!r}:", needle in case)
print(f" parsed-list membership {needle!r}:", needle in values)
PYRepository: openshift-eng/ai-helpers
Length of output: 279
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- available YAML parser ---'
if command -v ruby >/dev/null 2>&1; then
ruby -e 'require "yaml"; puts "ruby psych available"'
else
echo "ruby unavailable"
fi
if command -v yq >/dev/null 2>&1; then
echo "yq available"
else
echo "yq unavailable"
fi
ruby <<'RB'
require "yaml"
path = "plugins/agentic-docs/skills/component-docs/templates/coderabbit-template.yaml"
text = File.read(path)
data = YAML.safe_load(text, permitted_classes: [], aliases: false)
patterns = data.fetch("knowledge_base").fetch("code_guidelines").fetch("filePatterns")
puts "parsed filePatterns: #{patterns.inspect}"
["REVIEW.md", "CLAUDE.md"].each do |needle|
puts "grep #{needle.inspect} on template: #{text.include?(needle)}"
end
cases = {
"missing REVIEW.md, guidance comment retained" =>
"# filePatterns must include REVIEW.md\n" \
"knowledge_base:\n code_guidelines:\n filePatterns:\n - AGENTS.md\n",
"CLAUDE.md absent from list, prohibition comment retained" =>
"knowledge_base:\n code_guidelines:\n" \
" # NEVER add CLAUDE.md here\n" \
" filePatterns:\n - REVIEW.md\n - AGENTS.md\n"
}
cases.each do |name, case_text|
parsed = YAML.safe_load(case_text, permitted_classes: [], aliases: false)
values = parsed.fetch("knowledge_base").fetch("code_guidelines").fetch("filePatterns")
puts name
puts " parsed values: #{values.inspect}"
["REVIEW.md", "CLAUDE.md"].each do |needle|
puts " grep #{needle.inspect}: #{case_text.include?(needle)}"
puts " parsed-list membership #{needle.inspect}: #{values.include?(needle)}"
end
end
RBRepository: openshift-eng/ai-helpers
Length of output: 799
Read knowledge_base.code_guidelines.filePatterns from parsed YAML.
grep matches comments. The template comment contains both REVIEW.md and CLAUDE.md, which causes false validation results.
🤖 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 `@plugins/agentic-docs/skills/component-docs/scripts/validate.sh` around lines
331 - 338, Update the validation logic around the REVIEW.md and CLAUDE.md checks
to parse .coderabbit.yaml as YAML and read
knowledge_base.code_guidelines.filePatterns instead of grepping raw text.
Perform both presence checks against the parsed filePatterns values so template
comments cannot produce false results, preserving the existing success and
warning messages.
| # Check links in REVIEW.md | ||
| if [ -f "$REPO_PATH/REVIEW.md" ]; then | ||
| echo "📄 Checking REVIEW.md:" | ||
| echo " 🔗 External links:" | ||
| if ! validate_links "$REPO_PATH/REVIEW.md"; then | ||
| LINK_VALIDATION_FAILED=true | ||
| fi | ||
| echo " 🔗 Internal links:" | ||
| if ! validate_internal_links "$REPO_PATH/REVIEW.md"; then | ||
| LINK_VALIDATION_FAILED=true | ||
| fi |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep validation read-only.
This new call reaches validate_links, which removes links from the file on transient HTTP failures and non-200 responses. Running validation can therefore delete valid REVIEW.md links without user approval. Return failures from validation only. Put any repair operation behind an explicit user-selected fix mode.
🤖 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 `@plugins/agentic-docs/skills/component-docs/scripts/validate.sh` around lines
390 - 400, Update the REVIEW.md validation flow and validate_links so link
checking never modifies files: return validation failures only, and move any
link-removal or repair behavior behind an explicit user-selected fix mode.
Preserve the existing validation reporting while ensuring normal validation
cannot alter REVIEW.md.
Source: Linters/SAST tools
| ``` | ||
| mcp__chai-bot__ask_persona: | ||
| "I'm generating REVIEW.md for {component} (github.com/openshift/{component}). | ||
| I extracted these enforceable review rules from openshift/enhancements dev-guide. | ||
| Are these still current? Have any been superseded, relaxed, or tightened? | ||
|
|
||
| 1. [Rule 1 from Step 2] | ||
| 2. [Rule 2 from Step 2] | ||
| ... | ||
| (list top 5-8 most critical rules for the detected repo type) | ||
|
|
||
| For each rule: confirm current, superseded (by what), or unknown." | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language identifier to the fenced block.
markdownlint reports MD040 for this block. Use text if the block is illustrative MCP input.
🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 251-251: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 SkillSpector (2.4.4)
[warning] 440: [EA2] Autonomous Decision Making: Skill enables autonomous high-impact decisions without human-in-the-loop verification. Critical operations (destructive commands, financial transactions, data deletion) should require explicit user confirmation.
Remediation: Add human-in-the-loop confirmation for destructive, irreversible, or high-impact operations. Never auto-execute commands that modify files, send data, or alter system state.
(Excessive Agency (EA2))
🤖 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 `@plugins/agentic-docs/skills/component-docs/SKILL.md` around lines 251 - 263,
Add the `text` language identifier to the fenced code block containing the
mcp__chai-bot__ask_persona example, preserving its illustrative MCP input
content unchanged.
Source: Linters/SAST tools
| **Step 4 — Collect skip patterns** from Phase 5 discoveries: | ||
| - [ ] Generated code inventory (zz_generated*, clientset, informers, listers, bindata, protobuf, payload-manifests) | ||
| - [ ] Vendored dependencies (vendor/**) | ||
| - [ ] CI-enforced checks (from Phase 5 CI enforcement discovery) | ||
| - [ ] Lockfiles (go.sum, go.mod) | ||
| - [ ] Generated dashboards/assets if present | ||
|
|
||
| **Step 5 — Extract path-specific rules** from Phase 5 discoveries: | ||
| - [ ] Framework split table (which controllers use which apply method) | ||
| - [ ] Anti-patterns per package/directory | ||
| - [ ] Naming conventions per area | ||
| - [ ] Test conventions (Jira annotations, JUnit output, scoping) | ||
|
|
||
| **Step 6 — Calibrate severity** by repo type: | ||
|
|
||
| | Repo Type | Must-Fix Categories | | ||
| |-----------|-------------------| | ||
| | **Operator** | Incorrect reconciliation logic, unscoped queries crossing tenant boundaries, resource leaks, upgrade/downgrade safety violations, breaking changes to GA openshift.io APIs, security vulnerabilities, `Available=False` or `Degraded=True` during normal upgrade, premature version bump in ClusterOperator status, tolerating `node.kubernetes.io/unschedulable` | | ||
| | **Library** | API convention violations (bool fields, annotation-based APIs, missing validation markers, pointer misuse in CRDs), breaking changes to stable APIs, functions added to openshift/api | | ||
| | **CLI** | Breaking changes to CLI behavior, security vulnerabilities, incorrect error codes | | ||
|
|
||
| Style and naming issues are minor at most for all repo types. | ||
|
|
||
| **Step 7 — Generate REVIEW.md**: | ||
| - [ ] Use `templates/REVIEW-template.md` for structure | ||
| - [ ] Fill each section from Steps 2-6, stripping template comments from output | ||
| - [ ] Use tool-agnostic severity language ("must fix before merge" / "worth fixing, not blocking" / "suggestion only") | ||
| - [ ] Use glob patterns for skip rules, not prose descriptions | ||
| - [ ] Cite the dev-guide source for each "Always check" rule (parenthetical at end of line) | ||
| - [ ] Include "Verification bar" section — require file:line citations for every comment | ||
| - [ ] Include "Re-review" section — suppress new nits on unchanged code during re-reviews | ||
| - [ ] Validate line count: target 60-80 lines, soft cap 100 | ||
| - [ ] **Do NOT** copy CLAUDE.md content — different purposes | ||
|
|
||
| **Step 8 — Generate/merge .coderabbit.yaml**: | ||
| - [ ] Use `templates/coderabbit-template.yaml` for structure — always set `inheritance: true` (inherits org-wide config from `openshift/coderabbit` which already excludes `vendor/**`, `zz_generated*`, `node_modules/**`) | ||
| - [ ] Only add repo-specific exclusions to `path_filters` — skip patterns already covered by org config (vendor, zz_generated, boilerplate) | ||
| - [ ] Translate "Path-specific rules" subsections to `path_instructions` entries | ||
| - [ ] Set `knowledge_base.filePatterns` to `["REVIEW.md", "AGENTS.md"]` — **NEVER add CLAUDE.md** (auto-detected separately) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Keep lockfile skip rules synchronized.
Phase 9.5 requires go.sum and go.mod in REVIEW.md “Do not report” rules. It only translates repository-specific exclusions to .coderabbit.yaml, and its listed organization defaults do not include lockfiles. Generated tools can therefore apply different review scope.
plugins/agentic-docs/skills/component-docs/SKILL.md#L267-L305: translate every REVIEW.md skip rule that is not an explicit organization default, including lockfile rules.plugins/agentic-docs/skills/component-docs/templates/REVIEW-template.md#L48-L65: remove lockfiles from “Do not report” if they must remain reviewable, or require their corresponding CodeRabbit path filters.
🧰 Tools
🪛 SkillSpector (2.4.4)
[warning] 440: [EA2] Autonomous Decision Making: Skill enables autonomous high-impact decisions without human-in-the-loop verification. Critical operations (destructive commands, financial transactions, data deletion) should require explicit user confirmation.
Remediation: Add human-in-the-loop confirmation for destructive, irreversible, or high-impact operations. Never auto-execute commands that modify files, send data, or alter system state.
(Excessive Agency (EA2))
📍 Affects 2 files
plugins/agentic-docs/skills/component-docs/SKILL.md#L267-L305(this comment)plugins/agentic-docs/skills/component-docs/templates/REVIEW-template.md#L48-L65
🤖 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 `@plugins/agentic-docs/skills/component-docs/SKILL.md` around lines 267 - 305,
Synchronize lockfile exclusions between REVIEW.md and CodeRabbit: in
plugins/agentic-docs/skills/component-docs/SKILL.md lines 267-305, require Phase
9.5 to translate go.sum and go.mod skip rules into corresponding CodeRabbit path
filters unless they are removed from REVIEW.md; in
plugins/agentic-docs/skills/component-docs/templates/REVIEW-template.md lines
48-65, either remove those lockfiles from “Do not report” or require their
matching filters. Keep organization-default exclusions unchanged.
| path_filters: | ||
| # Phase 9.5: Translate "Do not report" globs to negated path_filters. | ||
| # Org config already excludes vendor/** and zz_generated* — only add | ||
| # repo-specific exclusions here. | ||
| # Example entries (replace with actual Phase 5 discoveries): | ||
| # | ||
| # - "!assets/**" | ||
| # - "!**/bindata.go" | ||
| # - "!payload-manifests/**" | ||
|
|
||
| path_instructions: | ||
| # Phase 9.5: Translate "Path-specific rules" subsections to path_instructions. | ||
| # Each entry maps a glob path to review instructions. | ||
| # Example entries (replace with actual Phase 5 discoveries): |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Does the CodeRabbit configuration schema v2 accept null values for reviews.path_filtersandreviews.path_instructions, or must these fields be YAML arrays?
💡 Result:
The CodeRabbit configuration schema v2 requires reviews.path_filters and reviews.path_instructions to be YAML arrays [1][2]. According to the official CodeRabbit configuration reference and schema definitions: - reviews.path_filters is defined as an array of string [1][3]. - reviews.path_instructions is defined as an array of object [1][3]. These fields are not intended to accept null values; they are typed explicitly as arrays and have default values of an empty array [] [1][2]. Providing null instead of an array would violate the schema's type constraints and will result in a validation error [3].
Citations:
- 1: https://docs.coderabbit.ai/reference/configuration
- 2: https://coderabbit.mintlify.app/reference/configuration
- 3: https://catalog.lintel.tools/schemas/schemastore/coderabbit/
Use empty sequences for both fields. reviews.path_filters must be an array of strings, and reviews.path_instructions must be an array of objects. Set both fields to [] when Phase 9.5 produces no entries, then validate the generated configuration against the schema.
🤖 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
`@plugins/agentic-docs/skills/component-docs/templates/coderabbit-template.yaml`
around lines 28 - 41, Update the reviews configuration in
coderabbit-template.yaml so path_filters and path_instructions are explicitly
empty sequences ([]), removing the placeholder-only structure. Preserve the
documented field types and validate the resulting configuration against the
schema.
2 participants
This PR adds the ability to create a review.md and coderabbit.yaml file. Example files
Summary by CodeRabbit
New Features
Validation
Chores