From 7935a602b035024704dbb7e8dfc2643c7179c3af Mon Sep 17 00:00:00 2001 From: Bryan Cox Date: Thu, 26 Mar 2026 16:06:59 -0400 Subject: [PATCH 1/8] feat(code-review): add classify-review-comment skill Add a skill for classifying GitHub PR review comments by severity and topic. Supports three input modes: single comment text, GitHub comment URL, or full PR URL with batch classification. Severity categories: nitpick, suggestion, required_change, question Topic categories: style, logic_bug, test_gap, api_design, documentation, ci, bot_instruction, approval, process Includes noise filtering guidance and real-world examples from openshift/hypershift PRs. Co-Authored-By: Claude Opus 4.6 --- docs/data.json | 7 +- .../code-review/.claude-plugin/plugin.json | 2 +- .../skills/classify-review-comment/SKILL.md | 201 ++++++++++++++++++ 3 files changed, 208 insertions(+), 2 deletions(-) create mode 100644 plugins/code-review/skills/classify-review-comment/SKILL.md diff --git a/docs/data.json b/docs/data.json index 64f7eb573..830ef664b 100644 --- a/docs/data.json +++ b/docs/data.json @@ -1579,6 +1579,11 @@ "hooks": [], "name": "code-review", "skills": [ + { + "description": "Classify GitHub PR review comments by severity and topic. Use when the user wants to categorize, analyze, or understand patterns in code review feedback \u2014 whether for a single comment, a comment URL, or an entire pull request. Triggers on requests like 'classify this comment', 'categorize PR feedback', 'what kind of review comments does this PR have', or 'break down comments by severity'.", + "id": "classify-review-comment", + "name": "classify-review-comment" + }, { "description": "Language-specific review guidance for Go code including idiomatic patterns, test conventions, and build commands", "id": "lang-go", @@ -1590,7 +1595,7 @@ "name": "HyperShift Project Profile" } ], - "version": "0.0.5" + "version": "0.0.6" }, { "commands": [ diff --git a/plugins/code-review/.claude-plugin/plugin.json b/plugins/code-review/.claude-plugin/plugin.json index 005929da6..278630fe2 100644 --- a/plugins/code-review/.claude-plugin/plugin.json +++ b/plugins/code-review/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "code-review", "description": "Automated code quality review with language-aware analysis for pre-commit verification", - "version": "0.0.5", + "version": "0.0.6", "author": { "name": "github.com/openshift-eng" } diff --git a/plugins/code-review/skills/classify-review-comment/SKILL.md b/plugins/code-review/skills/classify-review-comment/SKILL.md new file mode 100644 index 000000000..7b580f9dc --- /dev/null +++ b/plugins/code-review/skills/classify-review-comment/SKILL.md @@ -0,0 +1,201 @@ +--- +name: "classify-review-comment" +description: "Classify GitHub PR review comments by severity and topic. Use when the user wants to categorize, analyze, or understand patterns in code review feedback — whether for a single comment, a comment URL, or an entire pull request. Triggers on requests like 'classify this comment', 'categorize PR feedback', 'what kind of review comments does this PR have', or 'break down comments by severity'." +--- + +# Classify Review Comments + +Classify GitHub pull request review comments into severity and topic categories. Works with a single comment (text), a GitHub comment URL, or an entire PR (classifies all comments). + +This enables tracking review feedback patterns: what kinds of issues reviewers catch, how severe they are, and where AI-generated code needs the most improvement. + +## Input Modes + +### 1. Single Comment (text) + +Classify a comment provided directly as text. + +**Input:** The raw comment body. +**Output:** A single classification object. + +### 2. Comment URL + +Fetch a specific comment by its GitHub URL and classify it. + +**URL formats supported:** +- `https://github.com/{owner}/{repo}/pull/{number}#issuecomment-{id}` +- `https://github.com/{owner}/{repo}/pull/{number}#discussion_r{id}` + +**Fetch with:** +```bash +# Issue comment +gh api repos/{owner}/{repo}/issues/comments/{id} --jq '{author: .user.login, body: .body}' + +# Review comment (discussion) +gh api repos/{owner}/{repo}/pulls/comments/{id} --jq '{author: .user.login, body: .body}' +``` + +### 3. Full PR + +Fetch all comments on a PR, filter out noise, and classify each one. + +**URL format:** `https://github.com/{owner}/{repo}/pull/{number}` + +**Fetch with:** +```bash +# Issue-level conversation comments +gh api repos/{owner}/{repo}/issues/{number}/comments --paginate --jq '.[] | {id: .id, author: .user.login, body: .body}' + +# Inline review comments +gh api repos/{owner}/{repo}/pulls/{number}/comments --paginate --jq '.[] | {id: .id, author: .user.login, body: .body}' +``` + +**Before classifying, filter out noise comments** (these carry no review signal): +- Pure slash commands: body starts with `/` followed by a command word (e.g., `/lgtm`, `/test e2e-aws`, `/approve`, `/retest`, `/cc`) +- CI bot notifications: authors like `openshift-ci-robot`, `openshift-ci[bot]`, `cwbotbot`, or any `*[bot]` author except `coderabbitai[bot]` +- CodeRabbit noise: "No actionable comments were generated", "Skipped: comment is from another GitHub bot" +- Auto-CC commands: `/auto-cc` + +**Do classify** comments from: +- Human reviewers (all comments, including those directing bots) +- `coderabbitai[bot]` (substantive review comments and walkthrough summaries) +- `hypershift-jira-solve-ci[bot]` responding to review feedback (these show how the AI addressed the review) + +## Severity Categories + +Severity captures how urgent or blocking the feedback is. + +| Severity | Description | Signal | +|----------|-------------|--------| +| `nitpick` | Cosmetic or stylistic preference — take it or leave it | "nit:", "minor:", optional wording | +| `suggestion` | Worth considering but not blocking merge | "consider", "might want to", "could" | +| `required_change` | Must be addressed before merge | "this will break", "bug", "needs to be fixed" | +| `question` | Reviewer is asking for clarification | "why", "what does", "can you explain" | +| `unclassified` | Meta-comments, acknowledgments, or doesn't fit above | "looks good", "thanks", process comments | + +## Topic Categories + +Topic captures what area of concern the comment addresses. + +| Topic | Description | Examples | +|-------|-------------|---------| +| `style` | Code formatting, naming, organization, idioms | "rename `cnt` to `count`", "move vars to `var ()` block" | +| `logic_bug` | Incorrect logic, potential bugs, edge cases, panics | "this will panic if nil", "off-by-one", "case mismatch" | +| `test_gap` | Missing tests, test quality, coverage issues | "add a test for empty input", "test doesn't validate X" | +| `api_design` | API surface, interfaces, abstractions, contracts | "why is this exported?", "consider a different signature" | +| `documentation` | Missing docs, comments, READMEs, godoc | "missing godoc", "add a comment explaining why" | +| `ci` | CI triggers, test results, overrides, retests | "/test e2e-aws", "e2e failed on Teardown", CI status | +| `bot_instruction` | Directing a bot/AI to take action | "fix the unit tests", "rebase the PR", "push the changes" | +| `approval` | Approvals, LGTMs, sign-offs, acknowledgments | "/lgtm", "no changes requested", "LGTM" | +| `process` | Jira refs, duplicates, PR status, meta-discussion | "dup of #7727", Jira validation notices | +| `unclassified` | Doesn't fit other categories | Catch-all | + +## Classification Approach + +For each comment, determine severity and topic by reading the comment body and considering: + +1. **The language used** — imperative ("fix this") vs suggestive ("consider") vs interrogative ("why?") +2. **The content focus** — what aspect of the code is being discussed? +3. **The author context** — a bot responding to feedback vs a human reviewing code +4. **Slash commands mixed with text** — if a comment has substantive text before a slash command (e.g., "good analysis\n/override ci/prow/e2e"), classify based on the substantive text, not the slash command + +When a comment could fit multiple topics, pick the primary one — what is the reviewer's main concern? + +## Output Format + +### Single comment +```json +{ + "severity": "nitpick", + "topic": "style", + "rationale": "Brief one-line explanation of why this classification was chosen" +} +``` + +### Full PR +```json +{ + "pr": "https://github.com/openshift/hypershift/pull/7620", + "total_comments": 15, + "classified": 5, + "filtered_noise": 10, + "comments": [ + { + "id": 2871360513, + "author": "jparrill", + "body_preview": "small nit: I would move the vars...", + "severity": "nitpick", + "topic": "style", + "rationale": "Reviewer suggests moving variable declarations for consistency" + } + ], + "summary": { + "by_severity": {"nitpick": 1, "suggestion": 1, "required_change": 2, "question": 1}, + "by_topic": {"style": 1, "api_design": 1, "logic_bug": 2, "bot_instruction": 1} + } +} +``` + +## Real-World Examples + +These are from actual PRs in openshift/hypershift: + +**Comment:** "small nit: I would move the vars `key`, `log` and `cloudName` to `var (` section just to be consistent." +```json +{"severity": "nitpick", "topic": "style", "rationale": "Reviewer suggests grouping variables for consistency — cosmetic, not functional"} +``` + +**Comment:** "Why not use NewARMClientOptions here for the clientOptions?" +```json +{"severity": "question", "topic": "api_design", "rationale": "Reviewer asks about API choice for client options construction"} +``` + +**Comment:** "This will panic if `items` is nil — needs a nil check before the loop" +```json +{"severity": "required_change", "topic": "logic_bug", "rationale": "Nil pointer dereference would cause runtime panic"} +``` + +**Comment:** "failing during `hypershift install`\n```\nClusterRoleBinding is invalid: roleRef.kind: Unsupported value\n```" +```json +{"severity": "required_change", "topic": "logic_bug", "rationale": "Installation fails due to missing required roleRef fields"} +``` + +**Comment:** "hypershift-jira-solve-ci - the unit test job is failing and needs fixed" +```json +{"severity": "required_change", "topic": "bot_instruction", "rationale": "Human directing AI bot to fix failing unit tests"} +``` + +**Comment:** "hypershift-jira-solve-ci - rebase the PR to fix the konflux issues" +```json +{"severity": "suggestion", "topic": "bot_instruction", "rationale": "Human directing AI bot to rebase — a process action, not a code issue"} +``` + +**Comment:** "`e2e-aws-4-21` failed on `Teardown` but due to uncleaned cloud resources, not VPC endpoint blocking the finalizer\n/override ci/prow/e2e-aws-4-21" +```json +{"severity": "suggestion", "topic": "ci", "rationale": "Reviewer explains CI failure root cause and overrides — substantive analysis before the slash command"} +``` + +**Comment:** "Oh no that's ok. I missed that part. No changes requested." +```json +{"severity": "unclassified", "topic": "approval", "rationale": "Reviewer withdrawing their earlier question — acknowledgment"} +``` + +**Comment:** "dup of https://github.com/openshift/hypershift/pull/7727" +```json +{"severity": "unclassified", "topic": "process", "rationale": "Marking PR as duplicate of another — process meta-comment"} +``` + +**Comment:** "The root cause of the CI failure in this PR has been identified. The fix in `rejectVpcEndpointConnections` doesn't work because of a **case mismatch** between AWS API responses and SDK v2 enum constants." +```json +{"severity": "required_change", "topic": "logic_bug", "rationale": "Detailed root cause analysis identifying a case mismatch bug"} +``` + +**CodeRabbit walkthrough summary** (long HTML comment starting with ``): +```json +{"severity": "unclassified", "topic": "documentation", "rationale": "Automated PR summary providing overview of changes — informational, not actionable feedback"} +``` + +**CodeRabbit issue flagged** (starting with `_Potential issue_ | _Critical_`): +```json +{"severity": "required_change", "topic": "logic_bug", "rationale": "CodeRabbit identified a critical code issue requiring attention"} +``` From 839011e3ff276add67a42f0e187381b8f9dc46f1 Mon Sep 17 00:00:00 2001 From: Bryan Cox Date: Wed, 1 Apr 2026 12:40:48 -0400 Subject: [PATCH 2/8] Add labels.json for deterministic comment classification Move severity and topic label definitions from SKILL.md prose into a structured labels.json file. The skill now instructs the AI to read labels.json first and select only from the defined values, making classification more deterministic and consistent. Also removes bot_instruction topic, updates noise filters for CodeRabbit walkthroughs/review-skipped, and bumps version to 0.0.7. Co-Authored-By: Claude Opus 4.6 --- .claude-plugin/marketplace.json | 2 +- .../code-review/.claude-plugin/plugin.json | 2 +- .../skills/classify-review-comment/SKILL.md | 85 ++++++++----------- .../classify-review-comment/labels.json | 76 +++++++++++++++++ 4 files changed, 114 insertions(+), 51 deletions(-) create mode 100644 plugins/code-review/skills/classify-review-comment/labels.json diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index f13b6dc4c..888c4c091 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -194,7 +194,7 @@ "name": "code-review", "source": "./plugins/code-review", "description": "Automated code quality review with language-aware analysis for pre-commit verification", - "version": "0.0.5" + "version": "0.0.7" }, { "name": "golang", diff --git a/plugins/code-review/.claude-plugin/plugin.json b/plugins/code-review/.claude-plugin/plugin.json index 278630fe2..465b490cc 100644 --- a/plugins/code-review/.claude-plugin/plugin.json +++ b/plugins/code-review/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "code-review", "description": "Automated code quality review with language-aware analysis for pre-commit verification", - "version": "0.0.6", + "version": "0.0.7", "author": { "name": "github.com/openshift-eng" } diff --git a/plugins/code-review/skills/classify-review-comment/SKILL.md b/plugins/code-review/skills/classify-review-comment/SKILL.md index 7b580f9dc..c0952c1cd 100644 --- a/plugins/code-review/skills/classify-review-comment/SKILL.md +++ b/plugins/code-review/skills/classify-review-comment/SKILL.md @@ -9,6 +9,18 @@ Classify GitHub pull request review comments into severity and topic categories. This enables tracking review feedback patterns: what kinds of issues reviewers catch, how severe they are, and where AI-generated code needs the most improvement. +## Labels + +**Read the labels file before classifying any comments:** + +``` +labels.json (in the same directory as this skill) +``` + +The labels file defines the **exact** set of valid values for `severity` and `topic`. You MUST select from these values — do not invent new labels. Each label includes a description and signal words or examples to guide your selection. + +**Classification rule:** For each comment, find the single best-matching `severity` and single best-matching `topic` from the labels file. Match based on the label's description, signals/examples, and the comment content. Use `unclassified` only when no other label fits. + ## Input Modes ### 1. Single Comment (text) @@ -53,61 +65,36 @@ gh api repos/{owner}/{repo}/pulls/{number}/comments --paginate --jq '.[] | {id: **Before classifying, filter out noise comments** (these carry no review signal): - Pure slash commands: body starts with `/` followed by a command word (e.g., `/lgtm`, `/test e2e-aws`, `/approve`, `/retest`, `/cc`) - CI bot notifications: authors like `openshift-ci-robot`, `openshift-ci[bot]`, `cwbotbot`, or any `*[bot]` author except `coderabbitai[bot]` -- CodeRabbit noise: "No actionable comments were generated", "Skipped: comment is from another GitHub bot" +- CodeRabbit noise: "No actionable comments were generated", "Skipped: comment is from another GitHub bot", walkthrough summaries (``), review-skipped notices (`skip review by coderabbit.ai`) - Auto-CC commands: `/auto-cc` **Do classify** comments from: - Human reviewers (all comments, including those directing bots) -- `coderabbitai[bot]` (substantive review comments and walkthrough summaries) +- `coderabbitai[bot]` (substantive review comments — code issues, suggestions, questions) - `hypershift-jira-solve-ci[bot]` responding to review feedback (these show how the AI addressed the review) -## Severity Categories - -Severity captures how urgent or blocking the feedback is. - -| Severity | Description | Signal | -|----------|-------------|--------| -| `nitpick` | Cosmetic or stylistic preference — take it or leave it | "nit:", "minor:", optional wording | -| `suggestion` | Worth considering but not blocking merge | "consider", "might want to", "could" | -| `required_change` | Must be addressed before merge | "this will break", "bug", "needs to be fixed" | -| `question` | Reviewer is asking for clarification | "why", "what does", "can you explain" | -| `unclassified` | Meta-comments, acknowledgments, or doesn't fit above | "looks good", "thanks", process comments | - -## Topic Categories - -Topic captures what area of concern the comment addresses. - -| Topic | Description | Examples | -|-------|-------------|---------| -| `style` | Code formatting, naming, organization, idioms | "rename `cnt` to `count`", "move vars to `var ()` block" | -| `logic_bug` | Incorrect logic, potential bugs, edge cases, panics | "this will panic if nil", "off-by-one", "case mismatch" | -| `test_gap` | Missing tests, test quality, coverage issues | "add a test for empty input", "test doesn't validate X" | -| `api_design` | API surface, interfaces, abstractions, contracts | "why is this exported?", "consider a different signature" | -| `documentation` | Missing docs, comments, READMEs, godoc | "missing godoc", "add a comment explaining why" | -| `ci` | CI triggers, test results, overrides, retests | "/test e2e-aws", "e2e failed on Teardown", CI status | -| `bot_instruction` | Directing a bot/AI to take action | "fix the unit tests", "rebase the PR", "push the changes" | -| `approval` | Approvals, LGTMs, sign-offs, acknowledgments | "/lgtm", "no changes requested", "LGTM" | -| `process` | Jira refs, duplicates, PR status, meta-discussion | "dup of #7727", Jira validation notices | -| `unclassified` | Doesn't fit other categories | Catch-all | - ## Classification Approach -For each comment, determine severity and topic by reading the comment body and considering: +For each comment: -1. **The language used** — imperative ("fix this") vs suggestive ("consider") vs interrogative ("why?") -2. **The content focus** — what aspect of the code is being discussed? -3. **The author context** — a bot responding to feedback vs a human reviewing code -4. **Slash commands mixed with text** — if a comment has substantive text before a slash command (e.g., "good analysis\n/override ci/prow/e2e"), classify based on the substantive text, not the slash command +1. **Read labels.json** to load the valid severity and topic values +2. **Scan the comment body** for signal words and patterns that match label descriptions +3. **Select exactly one severity** — match the comment's urgency/tone to the severity descriptions and signals +4. **Select exactly one topic** — match the comment's subject matter to the topic descriptions and examples +5. **When ambiguous**, prefer the more specific label over `unclassified` +6. **When a comment spans multiple topics**, pick the primary one — what is the reviewer's main concern? -When a comment could fit multiple topics, pick the primary one — what is the reviewer's main concern? +Additional classification rules: +- **Slash commands mixed with text** — if a comment has substantive text before a slash command (e.g., "good analysis\n/override ci/prow/e2e"), classify based on the substantive text, not the slash command +- **Comments directed at bots/AI** — classify by the underlying problem, not the fact that the recipient is a bot. "Fix the unit tests" is about test failures (`test_gap`), "rebase the PR" is a CI/process issue (`ci`), "push the changes" is a process failure (`process`). The topic should answer "what went wrong?" not "who is being told to fix it?" ## Output Format ### Single comment ```json { - "severity": "nitpick", - "topic": "style", + "severity": "", + "topic": "", "rationale": "Brief one-line explanation of why this classification was chosen" } ``` @@ -124,14 +111,14 @@ When a comment could fit multiple topics, pick the primary one — what is the r "id": 2871360513, "author": "jparrill", "body_preview": "small nit: I would move the vars...", - "severity": "nitpick", - "topic": "style", + "severity": "", + "topic": "", "rationale": "Reviewer suggests moving variable declarations for consistency" } ], "summary": { "by_severity": {"nitpick": 1, "suggestion": 1, "required_change": 2, "question": 1}, - "by_topic": {"style": 1, "api_design": 1, "logic_bug": 2, "bot_instruction": 1} + "by_topic": {"style": 1, "api_design": 1, "logic_bug": 2, "test_gap": 1} } } ``` @@ -162,12 +149,17 @@ These are from actual PRs in openshift/hypershift: **Comment:** "hypershift-jira-solve-ci - the unit test job is failing and needs fixed" ```json -{"severity": "required_change", "topic": "bot_instruction", "rationale": "Human directing AI bot to fix failing unit tests"} +{"severity": "required_change", "topic": "test_gap", "rationale": "Unit tests are failing — the bot made code changes without ensuring tests pass"} ``` **Comment:** "hypershift-jira-solve-ci - rebase the PR to fix the konflux issues" ```json -{"severity": "suggestion", "topic": "bot_instruction", "rationale": "Human directing AI bot to rebase — a process action, not a code issue"} +{"severity": "suggestion", "topic": "ci", "rationale": "PR needs rebasing to resolve CI pipeline issues — classify by the problem (CI), not the recipient (bot)"} +``` + +**Comment:** "hypershift-jira-solve-ci - this still needs fixed since the code did not get pushed" +```json +{"severity": "required_change", "topic": "process", "rationale": "Code changes were not committed/pushed — a process failure, not a code issue"} ``` **Comment:** "`e2e-aws-4-21` failed on `Teardown` but due to uncleaned cloud resources, not VPC endpoint blocking the finalizer\n/override ci/prow/e2e-aws-4-21" @@ -190,11 +182,6 @@ These are from actual PRs in openshift/hypershift: {"severity": "required_change", "topic": "logic_bug", "rationale": "Detailed root cause analysis identifying a case mismatch bug"} ``` -**CodeRabbit walkthrough summary** (long HTML comment starting with ``): -```json -{"severity": "unclassified", "topic": "documentation", "rationale": "Automated PR summary providing overview of changes — informational, not actionable feedback"} -``` - **CodeRabbit issue flagged** (starting with `_Potential issue_ | _Critical_`): ```json {"severity": "required_change", "topic": "logic_bug", "rationale": "CodeRabbit identified a critical code issue requiring attention"} diff --git a/plugins/code-review/skills/classify-review-comment/labels.json b/plugins/code-review/skills/classify-review-comment/labels.json new file mode 100644 index 000000000..b1e55edf6 --- /dev/null +++ b/plugins/code-review/skills/classify-review-comment/labels.json @@ -0,0 +1,76 @@ +{ + "severity": [ + { + "value": "nitpick", + "description": "Cosmetic or stylistic preference — take it or leave it", + "signals": ["nit:", "minor:", "optional", "could also", "personal preference"] + }, + { + "value": "suggestion", + "description": "Worth considering but not blocking merge", + "signals": ["consider", "might want to", "could", "what about", "have you thought about"] + }, + { + "value": "required_change", + "description": "Must be addressed before merge — blocking", + "signals": ["this will break", "bug", "needs to be fixed", "must", "fails", "panic", "crash"] + }, + { + "value": "question", + "description": "Reviewer is asking for clarification or understanding", + "signals": ["why", "what does", "can you explain", "how does", "is this intentional"] + }, + { + "value": "unclassified", + "description": "Meta-comments, acknowledgments, or does not fit above categories", + "signals": ["looks good", "thanks", "LGTM", "acknowledged"] + } + ], + "topic": [ + { + "value": "style", + "description": "Code formatting, naming, organization, idioms", + "examples": ["rename variable", "move to var block", "use consistent casing", "reorder imports"] + }, + { + "value": "logic_bug", + "description": "Incorrect logic, potential bugs, edge cases, nil panics, race conditions", + "examples": ["nil pointer dereference", "off-by-one", "case mismatch", "missing error check"] + }, + { + "value": "test_gap", + "description": "Missing tests, test quality, coverage issues, failing tests", + "examples": ["add a test for empty input", "unit tests failing", "test doesn't validate X"] + }, + { + "value": "api_design", + "description": "API surface, interfaces, abstractions, contracts, exported types", + "examples": ["why is this exported?", "consider a different signature", "interface too broad"] + }, + { + "value": "documentation", + "description": "Missing docs, comments, READMEs, godoc", + "examples": ["missing godoc", "add a comment explaining why", "update README"] + }, + { + "value": "ci", + "description": "CI triggers, test results, overrides, retests, pipeline issues", + "examples": ["/test e2e-aws", "e2e failed on Teardown", "rebase to fix CI", "konflux issues"] + }, + { + "value": "approval", + "description": "Approvals, LGTMs, sign-offs, acknowledgments", + "examples": ["/lgtm", "no changes requested", "LGTM", "looks good to me"] + }, + { + "value": "process", + "description": "Jira refs, duplicates, PR status, meta-discussion, code not pushed", + "examples": ["dup of #7727", "Jira validation", "code not pushed", "close this PR"] + }, + { + "value": "unclassified", + "description": "Does not fit other categories — catch-all", + "examples": [] + } + ] +} From 7637ac14df5f7982a4fe21ef98d902d6ed4b4cb9 Mon Sep 17 00:00:00 2001 From: Bryan Cox Date: Wed, 1 Apr 2026 13:00:59 -0400 Subject: [PATCH 3/8] fix(code-review): use allowed_bots from labels.json for deterministic bot filtering Address PR review comments: replace hardcoded bot names with references to the allowed_bots and noise_patterns lists in labels.json. Add text language tag to fenced code block. Co-Authored-By: Claude Opus 4.6 --- .../skills/classify-review-comment/SKILL.md | 9 ++++----- .../skills/classify-review-comment/labels.json | 12 ++++++++++++ 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/plugins/code-review/skills/classify-review-comment/SKILL.md b/plugins/code-review/skills/classify-review-comment/SKILL.md index c0952c1cd..0f68b25b8 100644 --- a/plugins/code-review/skills/classify-review-comment/SKILL.md +++ b/plugins/code-review/skills/classify-review-comment/SKILL.md @@ -13,7 +13,7 @@ This enables tracking review feedback patterns: what kinds of issues reviewers c **Read the labels file before classifying any comments:** -``` +```text labels.json (in the same directory as this skill) ``` @@ -64,14 +64,13 @@ gh api repos/{owner}/{repo}/pulls/{number}/comments --paginate --jq '.[] | {id: **Before classifying, filter out noise comments** (these carry no review signal): - Pure slash commands: body starts with `/` followed by a command word (e.g., `/lgtm`, `/test e2e-aws`, `/approve`, `/retest`, `/cc`) -- CI bot notifications: authors like `openshift-ci-robot`, `openshift-ci[bot]`, `cwbotbot`, or any `*[bot]` author except `coderabbitai[bot]` -- CodeRabbit noise: "No actionable comments were generated", "Skipped: comment is from another GitHub bot", walkthrough summaries (``), review-skipped notices (`skip review by coderabbit.ai`) +- CI bot notifications: authors like `openshift-ci-robot`, `openshift-ci[bot]`, `cwbotbot`, or any `*[bot]` author **not listed** in the `allowed_bots` section of labels.json +- Comments matching any pattern in the `noise_patterns` section of labels.json - Auto-CC commands: `/auto-cc` **Do classify** comments from: - Human reviewers (all comments, including those directing bots) -- `coderabbitai[bot]` (substantive review comments — code issues, suggestions, questions) -- `hypershift-jira-solve-ci[bot]` responding to review feedback (these show how the AI addressed the review) +- Any bot listed in `allowed_bots` in labels.json (classify their substantive review comments — code issues, suggestions, questions) ## Classification Approach diff --git a/plugins/code-review/skills/classify-review-comment/labels.json b/plugins/code-review/skills/classify-review-comment/labels.json index b1e55edf6..084ef8cb0 100644 --- a/plugins/code-review/skills/classify-review-comment/labels.json +++ b/plugins/code-review/skills/classify-review-comment/labels.json @@ -1,4 +1,16 @@ { + "allowed_bots": [ + { + "author": "coderabbitai[bot]", + "description": "CodeRabbit AI reviewer — classify substantive review comments (code issues, suggestions, questions)" + } + ], + "noise_patterns": [ + "No actionable comments were generated", + "Skipped: comment is from another GitHub bot", + "", + "skip review by coderabbit.ai" + ], "severity": [ { "value": "nitpick", From 80b21c9f5408e6f947cde215d81305736582e237 Mon Sep 17 00:00:00 2001 From: Bryan Cox Date: Wed, 1 Apr 2026 13:06:55 -0400 Subject: [PATCH 4/8] refactor(code-review): rename labels.json to config.json MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit File now contains allowed_bots, noise_patterns, and classification labels — config.json better describes its broader scope. Co-Authored-By: Claude Opus 4.6 --- .../skills/classify-review-comment/SKILL.md | 18 +++++++++--------- .../{labels.json => config.json} | 0 2 files changed, 9 insertions(+), 9 deletions(-) rename plugins/code-review/skills/classify-review-comment/{labels.json => config.json} (100%) diff --git a/plugins/code-review/skills/classify-review-comment/SKILL.md b/plugins/code-review/skills/classify-review-comment/SKILL.md index 0f68b25b8..c5d00ceaa 100644 --- a/plugins/code-review/skills/classify-review-comment/SKILL.md +++ b/plugins/code-review/skills/classify-review-comment/SKILL.md @@ -14,7 +14,7 @@ This enables tracking review feedback patterns: what kinds of issues reviewers c **Read the labels file before classifying any comments:** ```text -labels.json (in the same directory as this skill) +config.json (in the same directory as this skill) ``` The labels file defines the **exact** set of valid values for `severity` and `topic`. You MUST select from these values — do not invent new labels. Each label includes a description and signal words or examples to guide your selection. @@ -64,19 +64,19 @@ gh api repos/{owner}/{repo}/pulls/{number}/comments --paginate --jq '.[] | {id: **Before classifying, filter out noise comments** (these carry no review signal): - Pure slash commands: body starts with `/` followed by a command word (e.g., `/lgtm`, `/test e2e-aws`, `/approve`, `/retest`, `/cc`) -- CI bot notifications: authors like `openshift-ci-robot`, `openshift-ci[bot]`, `cwbotbot`, or any `*[bot]` author **not listed** in the `allowed_bots` section of labels.json -- Comments matching any pattern in the `noise_patterns` section of labels.json +- CI bot notifications: authors like `openshift-ci-robot`, `openshift-ci[bot]`, `cwbotbot`, or any `*[bot]` author **not listed** in the `allowed_bots` section of config.json +- Comments matching any pattern in the `noise_patterns` section of config.json - Auto-CC commands: `/auto-cc` **Do classify** comments from: - Human reviewers (all comments, including those directing bots) -- Any bot listed in `allowed_bots` in labels.json (classify their substantive review comments — code issues, suggestions, questions) +- Any bot listed in `allowed_bots` in config.json (classify their substantive review comments — code issues, suggestions, questions) ## Classification Approach For each comment: -1. **Read labels.json** to load the valid severity and topic values +1. **Read config.json** to load the valid severity and topic values 2. **Scan the comment body** for signal words and patterns that match label descriptions 3. **Select exactly one severity** — match the comment's urgency/tone to the severity descriptions and signals 4. **Select exactly one topic** — match the comment's subject matter to the topic descriptions and examples @@ -92,8 +92,8 @@ Additional classification rules: ### Single comment ```json { - "severity": "", - "topic": "", + "severity": "", + "topic": "", "rationale": "Brief one-line explanation of why this classification was chosen" } ``` @@ -110,8 +110,8 @@ Additional classification rules: "id": 2871360513, "author": "jparrill", "body_preview": "small nit: I would move the vars...", - "severity": "", - "topic": "", + "severity": "", + "topic": "", "rationale": "Reviewer suggests moving variable declarations for consistency" } ], diff --git a/plugins/code-review/skills/classify-review-comment/labels.json b/plugins/code-review/skills/classify-review-comment/config.json similarity index 100% rename from plugins/code-review/skills/classify-review-comment/labels.json rename to plugins/code-review/skills/classify-review-comment/config.json From 264b8e0059d068c4cac49bd39c8c46dab5ccb7c3 Mon Sep 17 00:00:00 2001 From: Bryan Cox Date: Wed, 1 Apr 2026 13:10:15 -0400 Subject: [PATCH 5/8] chore: sync docs/data.json after rebase Co-Authored-By: Claude Opus 4.6 --- docs/data.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/data.json b/docs/data.json index 830ef664b..30da160d1 100644 --- a/docs/data.json +++ b/docs/data.json @@ -1595,7 +1595,7 @@ "name": "HyperShift Project Profile" } ], - "version": "0.0.6" + "version": "0.0.7" }, { "commands": [ From 52697f0852283ee14e39371f96a4f30c9e53dc07 Mon Sep 17 00:00:00 2001 From: Bryan Cox Date: Wed, 1 Apr 2026 13:18:37 -0400 Subject: [PATCH 6/8] fix(code-review): add review-body comment support Add #pullrequestreview-{id} URL format and /pulls/reviews API endpoint for fetching review summaries (approvals, rejections, general comments with state). Co-Authored-By: Claude Opus 4.6 --- .../code-review/skills/classify-review-comment/SKILL.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/plugins/code-review/skills/classify-review-comment/SKILL.md b/plugins/code-review/skills/classify-review-comment/SKILL.md index c5d00ceaa..036d65e15 100644 --- a/plugins/code-review/skills/classify-review-comment/SKILL.md +++ b/plugins/code-review/skills/classify-review-comment/SKILL.md @@ -37,6 +37,7 @@ Fetch a specific comment by its GitHub URL and classify it. **URL formats supported:** - `https://github.com/{owner}/{repo}/pull/{number}#issuecomment-{id}` - `https://github.com/{owner}/{repo}/pull/{number}#discussion_r{id}` +- `https://github.com/{owner}/{repo}/pull/{number}#pullrequestreview-{id}` **Fetch with:** ```bash @@ -45,6 +46,9 @@ gh api repos/{owner}/{repo}/issues/comments/{id} --jq '{author: .user.login, bod # Review comment (discussion) gh api repos/{owner}/{repo}/pulls/comments/{id} --jq '{author: .user.login, body: .body}' + +# Review body comment +gh api repos/{owner}/{repo}/pulls/{number}/reviews/{id} --jq '{author: .user.login, body: .body}' ``` ### 3. Full PR @@ -60,6 +64,9 @@ gh api repos/{owner}/{repo}/issues/{number}/comments --paginate --jq '.[] | {id: # Inline review comments gh api repos/{owner}/{repo}/pulls/{number}/comments --paginate --jq '.[] | {id: .id, author: .user.login, body: .body}' + +# Review body comments (approvals, rejections, general review summaries) +gh api repos/{owner}/{repo}/pulls/{number}/reviews --paginate --jq '.[] | select(.body != null and .body != "") | {id: .id, author: .user.login, body: .body}' ``` **Before classifying, filter out noise comments** (these carry no review signal): From 0d339a7fbe5a2bc940b820cbdcfb71b3d2264bee Mon Sep 17 00:00:00 2001 From: Bryan Cox Date: Mon, 6 Apr 2026 08:23:10 -0400 Subject: [PATCH 7/8] feat(code-review): add confidence scoring rubric to classify-review-comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a weighted signal rubric (0.00–1.00) for classification confidence, modeled after the payload analysis scoring approach. Signals include signal word match, unambiguous category, example pattern match, context reinforcement, and single viable label. Confidence thresholds gate workflow routing: >= 0.95 auto-apply, 0.80–0.94 human review, < 0.80 manual classification. Co-Authored-By: Claude Opus 4.6 --- .../skills/classify-review-comment/SKILL.md | 53 +++++++++++++------ 1 file changed, 38 insertions(+), 15 deletions(-) diff --git a/plugins/code-review/skills/classify-review-comment/SKILL.md b/plugins/code-review/skills/classify-review-comment/SKILL.md index 036d65e15..4a7a20601 100644 --- a/plugins/code-review/skills/classify-review-comment/SKILL.md +++ b/plugins/code-review/skills/classify-review-comment/SKILL.md @@ -87,8 +87,28 @@ For each comment: 2. **Scan the comment body** for signal words and patterns that match label descriptions 3. **Select exactly one severity** — match the comment's urgency/tone to the severity descriptions and signals 4. **Select exactly one topic** — match the comment's subject matter to the topic descriptions and examples -5. **When ambiguous**, prefer the more specific label over `unclassified` -6. **When a comment spans multiple topics**, pick the primary one — what is the reviewer's main concern? +5. **Score confidence** using the rubric below +6. **When ambiguous**, prefer the more specific label over `unclassified` +7. **When a comment spans multiple topics**, pick the primary one — what is the reviewer's main concern? + +## Confidence Scoring Rubric + +Each classification includes a `confidence` score (0.00–1.00) indicating how certain the classification is. Accumulate the score from these signals: + +| Signal | Weight | Criteria | +|--------|--------|----------| +| Signal word match | +0.25 | Comment contains signal words/phrases from config.json for the chosen severity or topic label | +| Unambiguous category | +0.25 | Comment clearly fits one severity and one topic with no viable alternatives | +| Example pattern match | +0.25 | Comment closely matches a real-world example in this skill or in config.json | +| Context reinforcement | +0.15 | Multiple independent indicators point to the same classification (e.g., tone + keywords + structure all agree) | +| Single viable label | +0.10 | No other severity or topic label is a reasonable alternative | + +Maximum score is 1.00. When multiple signals apply, sum them and cap at 1.00. + +**Interpretation:** +- **>= 0.95** — High confidence: classification can be auto-applied for low-risk labels +- **0.80–0.94** — Moderate confidence: human confirmation recommended +- **< 0.80** — Low confidence: manual classification required Additional classification rules: - **Slash commands mixed with text** — if a comment has substantive text before a slash command (e.g., "good analysis\n/override ci/prow/e2e"), classify based on the substantive text, not the slash command @@ -101,6 +121,7 @@ Additional classification rules: { "severity": "", "topic": "", + "confidence": 0.95, "rationale": "Brief one-line explanation of why this classification was chosen" } ``` @@ -119,12 +140,14 @@ Additional classification rules: "body_preview": "small nit: I would move the vars...", "severity": "", "topic": "", + "confidence": 0.95, "rationale": "Reviewer suggests moving variable declarations for consistency" } ], "summary": { "by_severity": {"nitpick": 1, "suggestion": 1, "required_change": 2, "question": 1}, - "by_topic": {"style": 1, "api_design": 1, "logic_bug": 2, "test_gap": 1} + "by_topic": {"style": 1, "api_design": 1, "logic_bug": 2, "test_gap": 1}, + "by_confidence": {"high": 3, "moderate": 1, "low": 1} } } ``` @@ -135,60 +158,60 @@ These are from actual PRs in openshift/hypershift: **Comment:** "small nit: I would move the vars `key`, `log` and `cloudName` to `var (` section just to be consistent." ```json -{"severity": "nitpick", "topic": "style", "rationale": "Reviewer suggests grouping variables for consistency — cosmetic, not functional"} +{"severity": "nitpick", "topic": "style", "confidence": 1.00, "rationale": "Reviewer suggests grouping variables for consistency — cosmetic, not functional"} ``` **Comment:** "Why not use NewARMClientOptions here for the clientOptions?" ```json -{"severity": "question", "topic": "api_design", "rationale": "Reviewer asks about API choice for client options construction"} +{"severity": "question", "topic": "api_design", "confidence": 0.90, "rationale": "Reviewer asks about API choice for client options construction"} ``` **Comment:** "This will panic if `items` is nil — needs a nil check before the loop" ```json -{"severity": "required_change", "topic": "logic_bug", "rationale": "Nil pointer dereference would cause runtime panic"} +{"severity": "required_change", "topic": "logic_bug", "confidence": 1.00, "rationale": "Nil pointer dereference would cause runtime panic"} ``` **Comment:** "failing during `hypershift install`\n```\nClusterRoleBinding is invalid: roleRef.kind: Unsupported value\n```" ```json -{"severity": "required_change", "topic": "logic_bug", "rationale": "Installation fails due to missing required roleRef fields"} +{"severity": "required_change", "topic": "logic_bug", "confidence": 0.90, "rationale": "Installation fails due to missing required roleRef fields"} ``` **Comment:** "hypershift-jira-solve-ci - the unit test job is failing and needs fixed" ```json -{"severity": "required_change", "topic": "test_gap", "rationale": "Unit tests are failing — the bot made code changes without ensuring tests pass"} +{"severity": "required_change", "topic": "test_gap", "confidence": 0.90, "rationale": "Unit tests are failing — the bot made code changes without ensuring tests pass"} ``` **Comment:** "hypershift-jira-solve-ci - rebase the PR to fix the konflux issues" ```json -{"severity": "suggestion", "topic": "ci", "rationale": "PR needs rebasing to resolve CI pipeline issues — classify by the problem (CI), not the recipient (bot)"} +{"severity": "suggestion", "topic": "ci", "confidence": 0.85, "rationale": "PR needs rebasing to resolve CI pipeline issues — classify by the problem (CI), not the recipient (bot)"} ``` **Comment:** "hypershift-jira-solve-ci - this still needs fixed since the code did not get pushed" ```json -{"severity": "required_change", "topic": "process", "rationale": "Code changes were not committed/pushed — a process failure, not a code issue"} +{"severity": "required_change", "topic": "process", "confidence": 0.85, "rationale": "Code changes were not committed/pushed — a process failure, not a code issue"} ``` **Comment:** "`e2e-aws-4-21` failed on `Teardown` but due to uncleaned cloud resources, not VPC endpoint blocking the finalizer\n/override ci/prow/e2e-aws-4-21" ```json -{"severity": "suggestion", "topic": "ci", "rationale": "Reviewer explains CI failure root cause and overrides — substantive analysis before the slash command"} +{"severity": "suggestion", "topic": "ci", "confidence": 0.75, "rationale": "Reviewer explains CI failure root cause and overrides — substantive analysis before the slash command"} ``` **Comment:** "Oh no that's ok. I missed that part. No changes requested." ```json -{"severity": "unclassified", "topic": "approval", "rationale": "Reviewer withdrawing their earlier question — acknowledgment"} +{"severity": "unclassified", "topic": "approval", "confidence": 0.70, "rationale": "Reviewer withdrawing their earlier question — acknowledgment"} ``` **Comment:** "dup of https://github.com/openshift/hypershift/pull/7727" ```json -{"severity": "unclassified", "topic": "process", "rationale": "Marking PR as duplicate of another — process meta-comment"} +{"severity": "unclassified", "topic": "process", "confidence": 0.90, "rationale": "Marking PR as duplicate of another — process meta-comment"} ``` **Comment:** "The root cause of the CI failure in this PR has been identified. The fix in `rejectVpcEndpointConnections` doesn't work because of a **case mismatch** between AWS API responses and SDK v2 enum constants." ```json -{"severity": "required_change", "topic": "logic_bug", "rationale": "Detailed root cause analysis identifying a case mismatch bug"} +{"severity": "required_change", "topic": "logic_bug", "confidence": 1.00, "rationale": "Detailed root cause analysis identifying a case mismatch bug"} ``` **CodeRabbit issue flagged** (starting with `_Potential issue_ | _Critical_`): ```json -{"severity": "required_change", "topic": "logic_bug", "rationale": "CodeRabbit identified a critical code issue requiring attention"} +{"severity": "required_change", "topic": "logic_bug", "confidence": 0.85, "rationale": "CodeRabbit identified a critical code issue requiring attention"} ``` From be9524c23f574fe4bf569cd3ad23e6dd53e45bb4 Mon Sep 17 00:00:00 2001 From: Bryan Cox Date: Mon, 6 Apr 2026 10:48:55 -0400 Subject: [PATCH 8/8] feat(classify-review-comment): add architecture_design and security topics Add two new topic categories per review feedback: - architecture_design: system architecture, component boundaries, patterns - security: vulnerabilities, auth issues, input validation, secrets Co-Authored-By: Claude Opus 4.6 --- .../skills/classify-review-comment/SKILL.md | 10 ++++++++++ .../skills/classify-review-comment/config.json | 10 ++++++++++ 2 files changed, 20 insertions(+) diff --git a/plugins/code-review/skills/classify-review-comment/SKILL.md b/plugins/code-review/skills/classify-review-comment/SKILL.md index 4a7a20601..771c69c55 100644 --- a/plugins/code-review/skills/classify-review-comment/SKILL.md +++ b/plugins/code-review/skills/classify-review-comment/SKILL.md @@ -211,6 +211,16 @@ These are from actual PRs in openshift/hypershift: {"severity": "required_change", "topic": "logic_bug", "confidence": 1.00, "rationale": "Detailed root cause analysis identifying a case mismatch bug"} ``` +**Comment:** "This controller is doing too much — the reconciler should delegate VPC cleanup to a separate controller instead of inlining it" +```json +{"severity": "suggestion", "topic": "architecture_design", "confidence": 0.90, "rationale": "Reviewer identifies a separation-of-concerns issue at the controller level"} +``` + +**Comment:** "The service account token is being logged in plain text here — this needs to be redacted" +```json +{"severity": "required_change", "topic": "security", "confidence": 1.00, "rationale": "Sensitive credentials exposed in log output — security vulnerability"} +``` + **CodeRabbit issue flagged** (starting with `_Potential issue_ | _Critical_`): ```json {"severity": "required_change", "topic": "logic_bug", "confidence": 0.85, "rationale": "CodeRabbit identified a critical code issue requiring attention"} diff --git a/plugins/code-review/skills/classify-review-comment/config.json b/plugins/code-review/skills/classify-review-comment/config.json index 084ef8cb0..760b5db03 100644 --- a/plugins/code-review/skills/classify-review-comment/config.json +++ b/plugins/code-review/skills/classify-review-comment/config.json @@ -59,6 +59,16 @@ "description": "API surface, interfaces, abstractions, contracts, exported types", "examples": ["why is this exported?", "consider a different signature", "interface too broad"] }, + { + "value": "architecture_design", + "description": "System architecture, component boundaries, patterns, layering, coupling", + "examples": ["this belongs in the controller", "separate concerns", "wrong abstraction layer", "circular dependency"] + }, + { + "value": "security", + "description": "Security vulnerabilities, auth issues, input validation, secrets handling", + "examples": ["SQL injection", "unsanitized input", "hardcoded secret", "missing RBAC check", "privilege escalation"] + }, { "value": "documentation", "description": "Missing docs, comments, READMEs, godoc",