Merge pull request #37 from redhat-developer/feat/pr-review-consolidation

feat: add code review pipeline to rhdh-pr-review skill (v0.3.1)

Author: schultzp2020

.claude-plugin/marketplace.json

@@ -5,14 +5,14 @@
   },
   "metadata": {
     "description": "Orchestrator skill for RHDH plugin development - onboard, update, and maintain plugins in the Extensions Catalog",
-    "version": "0.3.0"
+    "version": "0.3.1"
   },
   "plugins": [
     {
       "name": "rhdh",
       "source": "./",
       "description": "Skills for RHDH plugin lifecycle management",
-      "version": "0.3.0",
+      "version": "0.3.1",
       "strict": true
     }
   ]

.claude-plugin/plugin.json

@@ -1,7 +1,7 @@
 {
   "name": "rhdh",
   "description": "All-in-one toolkit for Red Hat Developer Hub (RHDH). Covers plugin development, overlay management, environment setup, version compatibility, CI/CD, and RHDH ecosystem navigation.",
-  "version": "0.3.0",
+  "version": "0.3.1",
   "author": {
     "name": "RHDH Store Manager"
   },

README.md

@@ -61,7 +61,7 @@ Track work across the four RHDH Jira projects.
 
 ### PR Review
 
-- **[rhdh-pr-review](./skills/rhdh-pr-review/SKILL.md)** — Test PR changes on a live RHDH cluster. Swaps CI images, verifies code changes, and reports findings.
+- **[rhdh-pr-review](./skills/rhdh-pr-review/SKILL.md)** — PR code review with inline comments (GitHub, GitLab planned) and live cluster testing for rhdh-operator PRs. Layered architecture: fetch → analyze → post.
 
 ### Test Plan
 

pyproject.toml

@@ -1,6 +1,6 @@
 [project]
 name = "rhdh-skill"
-version = "0.3.0"
+version = "0.3.1"
 description = "Claude Code skill for RHDH plugin development"
 readme = "README.md"
 license = "Apache-2.0"

skills/rhdh-pr-review/SKILL.md

@@ -1,10 +1,12 @@
 ---
 name: rhdh-pr-review
-description: Test PR changes on a live RHDH cluster. Fetches CI-built images from PR comments, checks cluster status (deploying if needed), deploys the full PR operator bundle or manifests (not just image swap), actively verifies the code changes by exercising affected code paths on the cluster, and closes with findings including best-practice and security assessment. Use when asked to review an rhdh-operator PR, test PR changes on a cluster, deploy PR images for testing, or deploy PR bundle. Also use when user mentions "operator PR", "review PR", or "test this PR on my cluster". Currently supports rhdh-operator PRs.
+description: >
+  Review pull requests: code-level analysis with inline comments, and live cluster testing for rhdh-operator PRs. Supports GitHub (GitLab planned). Use when asked to review a PR, review code, post review comments, test PR changes on a cluster, deploy PR images for testing, or do a full PR review. Also use when given a PR URL or number and asked for feedback, or when user mentions "review this PR", "PR review", "code review", or "test this PR on my cluster".
 ---
 
 <cli_setup>
-This skill uses the orchestrator CLI for activity tracking. **Set up first:**
+
+For cluster testing workflows, set up the orchestrator CLI:
 
 ```bash
 RHDH=../rhdh/scripts/rhdh
@@ -14,24 +16,20 @@ RHDH=../rhdh/scripts/rhdh
 
 <essential_principles>
 
-<principle name="ensure_cluster">
-Always verify the user has a running RHDH cluster with `oc` access before deploying PR bundles.
-If no cluster or no RHDH instance, provision one using `redhat-developer/rhdh-test-instance` — see `rhdh-repos.md` for details on that repo's capabilities.
-Don't just tell the user to set things up — do it.
+<principle name="layered_architecture">
+Reviews follow a three-layer pipeline: **fetch** (forge-specific) → **analyze** (agnostic) → **post** (forge-specific). Each layer produces a structured artifact for the next. The analyze layer never calls forge-specific CLIs.
+</principle>
+
+<principle name="verify_findings">
+Reviewers will produce false positives. Verify every finding against actual code at HEAD before including it. Drop findings that reference non-existent code, duplicate existing comments, misread the code, or conflict with codebase conventions.
 </principle>
 
-<principle name="use_pr_comment_images">
-Extract image URLs from PR comments posted by CI — never construct image URLs manually.
-The tag format includes PR number + commit SHA, which only CI knows.
-See `references/operator-pr-images.md` for extraction commands.
+<principle name="user_confirms_before_posting">
+Present the full review draft — summary, inline comments with file:line, and event type — to the user before posting. Proceed only after confirmation.
 </principle>
 
 <principle name="deploy_full_bundle">
-Deploy the full PR bundle/manifests, not just the operator binary image.
-PR changes to CRDs, RBAC, default config, or bundle metadata are baked into the OLM bundle or install.yaml manifests — a binary-only image swap misses them and the operator may fail to reconcile.
-For OLM-managed installs, replace the CatalogSource with the PR's `operator-catalog` image so OLM reinstalls the full bundle (CRDs, RBAC, CSV, deployment).
-For non-OLM installs, fetch and apply the PR branch's `install.yaml` (CRDs, RBAC, ConfigMaps, Deployment) with the CI-built operator image substituted in.
-Both paths preserve existing Backstage CRs, config, and Keycloak state — only the operator-side resources are replaced.
+For cluster testing: deploy the full PR bundle/manifests, not just the operator binary image. PR changes to CRDs, RBAC, default config, or bundle metadata are baked into the OLM bundle or install.yaml — a binary-only image swap misses them.
 </principle>
 
 </essential_principles>
@@ -40,35 +38,96 @@ Both paths preserve existing Backstage CRs, config, and Keycloak state — only
 
 ## What would you like to do?
 
-### PR Review Tasks
+### Code Review
+
+1. **Review PR code** — Analyze a PR diff, generate findings, and post inline comments
+2. **Review PR code (analysis only)** — Analyze without posting (e.g., to review locally or post later)
 
-*For testing PR changes on a live RHDH cluster*
+### Cluster Testing (rhdh-operator PRs)
 
-1. **Review rhdh-operator PR** — Deploy PR operator bundle on cluster and get review checklist
+3. **Test PR on cluster** — Deploy PR operator bundle on a live RHDH cluster and verify changes
+
+### Combined
+
+4. **Full review** — Code review + post to GitHub + cluster testing
 
 **Wait for response before proceeding.**
 
 </intake>
 
 <routing>
 
-### PR Review Routes
-
 | Response | Workflow |
 |----------|----------|
-| 1, "operator", "rhdh-operator", a PR number, "review" | Route to `workflows/review-operator-pr.md` |
+| 1, "review", "review PR", "code review", a PR URL or number | `workflows/fetch-github.md` → `workflows/review-code.md` → `workflows/post-to-github.md` |
+| 2, "analyze", "analysis only", "review locally" | `workflows/fetch-github.md` → `workflows/review-code.md` (stop after findings) |
+| 3, "test", "cluster", "deploy", "operator PR", "test on cluster" | `workflows/fetch-github.md` → `workflows/review-operator-pr.md` |
+| 4, "full", "full review", "both" | `workflows/fetch-github.md` → `workflows/review-code.md` → `workflows/post-to-github.md` → `workflows/review-operator-pr.md` |
 
-**To route:** Read `workflows/review-operator-pr.md` and follow its process.
+### Routing rules
+
+1. **PR URL or number with no other context**: default to route 1 (code review + post).
+2. **rhdh-operator PR detected** (repo is `redhat-developer/rhdh-operator`): suggest route 4 (full review) but let the user choose.
+3. **"review" without "post" or "cluster"**: route 1.
+4. **All routes start with fetch.** The fetch workflow produces a context artifact consumed by all downstream workflows.
+
+### Forge detection
+
+Currently GitHub only. Detect from URL pattern or `gh` CLI availability.
+
+When GitLab support is added: `fetch-gitlab.md` and `post-to-gitlab.md` will slot into the same pipeline. The analyze workflow (`review-code.md`) is forge-agnostic and needs no changes.
 
 </routing>
 
+<artifact_contracts>
+
+## Context artifact (fetch → analyze / cluster test)
+
+Produced by the fetch workflow, consumed by all downstream workflows:
+
+```
+context artifact
+├── forge: "github"
+├── repo: "owner/repo"
+├── pr_number: 123
+├── head_sha: "abc123..."
+├── base_ref, head_ref, title, body, author, state, url
+├── labels: [...]
+├── files: [{path, additions, deletions}, ...]
+├── total_additions, total_deletions
+├── diff: "full unified diff"
+├── linked_issues: [{number, title, body, labels, state}, ...]
+├── jira_keys: ["RHIDP-1234", ...]
+├── existing_comments: [{user, path, line, body, created_at}, ...]
+├── existing_reviews: [{user, state, body}, ...]
+└── ci_status: "pass" | "fail" | "pending" | "unknown"
+```
+
+## Findings artifact (analyze → post)
+
+Produced by the analysis workflow, consumed by the posting workflow:
+
+```
+findings artifact
+├── pr: {repo, number, head_sha}
+├── summary: "top-level review text"
+├── event: "COMMENT" | "APPROVE" | "REQUEST_CHANGES"
+└── findings[]
+    ├── path, line, start_line
+    ├── type: "suggestion" | "question" | "observation"
+    └── body: "comment text"
+```
+
+</artifact_contracts>
+
 <reference_index>
 
-| Reference | Purpose | Path |
-|-----------|---------|------|
-| operator-pr-images | CI image extraction and validation | `references/operator-pr-images.md` |
-| github-reference | gh CLI patterns, PR queries | `../rhdh/references/github-reference.md` (if unavailable, use standard `gh` CLI patterns) |
-| rhdh-repos | RHDH ecosystem repository map | `../rhdh/references/rhdh-repos.md` (if unavailable, see CONTEXT.md for repo list) |
+| Reference | Purpose | Load when... | Path |
+|-----------|---------|--------------|------|
+| review-perspectives | Review perspective examples and signal hints | Running `review-code.md` | `references/review-perspectives.md` |
+| operator-pr-images | CI image extraction and validation | Running `review-operator-pr.md` | `references/operator-pr-images.md` |
+| github-reference | gh CLI patterns, PR queries | Running any GitHub workflow | `../rhdh/references/github-reference.md` (if available) |
+| rhdh-repos | RHDH ecosystem repository map | Cluster testing | `../rhdh/references/rhdh-repos.md` (if available) |
 
 </reference_index>
 
@@ -82,6 +141,21 @@ Both paths preserve existing Backstage CRs, config, and Keycloak state — only
 
 <success_criteria>
 
+### Code review
+
+- [ ] PR context fetched (metadata, diff, linked issues, existing comments)
+- [ ] Review perspectives chosen based on PR content
+- [ ] Findings verified against actual code at HEAD
+- [ ] False positives dropped with reasoning shown
+- [ ] Review draft presented to user with event type choice
+- [ ] Review posted to forge (if posting route selected)
+
+### Cluster testing
+
 See `workflows/review-operator-pr.md` `<success_criteria>` for the full checklist.
 
+### Full review
+
+All code review criteria + all cluster testing criteria.
+
 </success_criteria>

skills/rhdh-pr-review/references/review-perspectives.md

@@ -0,0 +1,41 @@
+# Code Review Perspectives
+
+Examples of review perspectives and the signals that suggest them. These are starting points, not a fixed roster — choose perspectives that fit the PR's actual content. Invent new ones when the PR calls for it (e.g., devil's advocate, agile coach, skill expert, domain specialist).
+
+## Common perspectives
+
+| Perspective | Focus | Prompt guidance |
+|-------------|-------|-----------------|
+| **Correctness** | Logic bugs, edge cases, error handling, off-by-ones, null/undefined paths | "Find bugs that would reach production. Ignore style." |
+| **Security** | Injection vectors, auth/authz gaps, secrets exposure, input validation | "Flag vulnerabilities with severity ratings." |
+| **Requirements** | Coverage of linked issues, test adequacy, scope gaps | "Check every linked requirement. Note what's addressed, tested, and missing." |
+| **Architecture** | Module boundaries, coupling, abstraction levels, extensibility | "Evaluate structural impact. Is this change in the right place?" |
+| **Performance** | Hot paths, query patterns, algorithmic complexity, caching | "Flag measurable performance risks." |
+| **Compatibility** | Public API surface, breaking changes, deprecations | "Determine if changed symbols are public-facing before flagging." |
+
+## Signals that suggest a perspective
+
+Use these as hints, not rules. A PR may need perspectives not listed here, or may not need ones that signal-match.
+
+| Signal | Suggests | Example |
+|--------|----------|---------|
+| Changes span 2+ modules/packages | Architecture | `src/api/` + `src/worker/` |
+| New files created | Architecture | New module, new component |
+| Changed paths match DB/query patterns | Performance | `**/model*`, `**/migration*`, `**/schema*` |
+| Keywords in title/body | Performance | `optimization`, `latency`, `cache`, `slow` |
+| Changed paths match API surface | Compatibility | `**/api/**`, `**/proto/**`, `**/openapi*` |
+| Package version changes | Compatibility | `package.json`, `pyproject.toml` version bumps |
+| Labels | Varies | `refactor` → Architecture, `breaking` → Compatibility |
+| Linked issues exist | Requirements | Any `Fixes #N` or Jira key in the body |
+
+## Choosing perspectives
+
+Read the PR's diff, metadata, and linked issues. Create perspectives based on what matters most for this specific change — the examples above are a starting point, not a menu to pick from.
+
+## Reviewer coordination
+
+When using multiple reviewers:
+
+- Each gets the diff, linked requirements, and their focus area
+- Instruct them to read source at HEAD, not just the diff
+- Encourage cross-checking — reviewers should challenge overlapping or contradictory findings