From c4a9fe68c049f5e66c8dab6e6397134717370179 Mon Sep 17 00:00:00 2001 From: Christoph Blecker Date: Fri, 3 Jul 2026 11:30:16 -0700 Subject: [PATCH 1/2] refactor(pr-review-toolkit): simplify overlap sections and restructure report Collapse possiblePlusOnes/partialOverlaps into a single relatedToExisting section, simplify overlap and recommendation enums to binary (overlaps/already_covered), remove actionPlan and coverageSummary from the board schema, restructure the SKILL.md report layout with per-finding recommendation rationale, replace the 9-command keyword system with contextual AskUserQuestion options, and add thread reply posting for overlap findings via add_reply_to_pull_request_comment. Assisted-by: Claude:claude-opus-4-6 --- pr-review-toolkit/.claude-plugin/plugin.json | 2 +- pr-review-toolkit/README.md | 8 +- pr-review-toolkit/skills/review-pr/SKILL.md | 205 ++++++++++++------ .../skills/review-pr/review-pr.js | 163 +++----------- 4 files changed, 176 insertions(+), 202 deletions(-) diff --git a/pr-review-toolkit/.claude-plugin/plugin.json b/pr-review-toolkit/.claude-plugin/plugin.json index baa71d2..dffb2bf 100644 --- a/pr-review-toolkit/.claude-plugin/plugin.json +++ b/pr-review-toolkit/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "pr-review-toolkit", - "version": "1.7.0", + "version": "1.8.0", "description": "Comprehensive PR review board using shared workflow context", "author": { "name": "cblecker", diff --git a/pr-review-toolkit/README.md b/pr-review-toolkit/README.md index 9ca36dd..d7a8a49 100644 --- a/pr-review-toolkit/README.md +++ b/pr-review-toolkit/README.md @@ -106,8 +106,7 @@ The workflow returns a review board grouped by outcome: - `recommendedToPost` — high-signal findings that look postable by a human reviewer and are not already covered -- `possiblePlusOnes` — existing threads where an endorsement may help -- `partialOverlaps` — findings that add useful detail beyond an existing thread +- `relatedToExisting` — findings that overlap or endorse existing review threads - `discussionOnly` — useful reviewer notes that should not be posted yet - `alreadyCovered` — findings fully covered by existing human or bot review threads @@ -115,8 +114,7 @@ The workflow returns a review board grouped by outcome: Each finding preserves the specialist's claim, evidence, reasoning, suggested fix, confidence, source lens, and existing-review overlap rationale. The board -also includes positive observations, an action plan, coverage notes, PR metadata, -and review metadata. +also includes positive observations, PR metadata, and review metadata. ## Interaction And Posting @@ -212,7 +210,7 @@ tools, local-git runs record manifest/full-diff provenance in `reviewMeta.sources`, and MCP fallback runs record the fallback reason and recovery counts. Also verify no generated parsing scripts are used, existing review context affects recommendations, the review board is understandable, the -action plan is concise, drafts remain editable, and posting requires explicit +drafts remain editable, and posting requires explicit approval. ## Prerequisites diff --git a/pr-review-toolkit/skills/review-pr/SKILL.md b/pr-review-toolkit/skills/review-pr/SKILL.md index 30c80bc..c5e74fa 100644 --- a/pr-review-toolkit/skills/review-pr/SKILL.md +++ b/pr-review-toolkit/skills/review-pr/SKILL.md @@ -15,6 +15,7 @@ allowed-tools: - mcp__plugin_github_github__pull_request_read - mcp__plugin_github_github__pull_request_review_write - mcp__plugin_github_github__add_comment_to_pending_review + - mcp__plugin_github_github__add_reply_to_pull_request_comment --- # PR Review: $pr-url @@ -219,74 +220,99 @@ The workflow owns PR metadata collection (via MCP), reviewer selection, specialist analysis, and review-board synthesis. It also fetches review threads (always from MCP). -The workflow returns grouped findings, positive observations, an action plan, -coverage summary, PR metadata, summary, and review metadata. +The workflow returns grouped findings, positive observations, PR metadata, +summary, and review metadata. ## Present Review Board -Present the review board before drafting or posting anything. Keep it concise, -but do not hide important groups. - -Use this order: - -1. Review heading: `owner/repo#number` and PR title when available. -2. Coverage summary: - - `coverageSummary.scope` - - `coverageSummary.largePrNotes`, if present - - selected reviewers and file/thread counts from `reviewMeta` -3. Action plan: - - critical - - important - - suggestions - - recommended next action -4. Findings grouped by outcome: - - recommended to post - - possible plus-ones - - partial overlaps - - worth discussing, not posting - - already covered - - discarded or weak findings, summarized if long -5. Positive observations. - -For each finding shown in detail, include: - -- stable id -- location -- lens -- title -- confidence +Present the review board before drafting or posting anything. Use this order: + +### 1. Heading + +Format: `owner/repo#number — PR title` + +Below the heading, include a one-line summary with section counts derived from +section array lengths: + +```text +N findings recommended, M overlap existing threads, P discussion-worthy. +Reviewers: code-reviewer, pr-test-analyzer, silent-failure-hunter. +``` + +The reviewer list comes from `reviewMeta.selectedReviewers` which stores full +agent names. + +### 2. Recommended to post (full detail) + +For each finding, include: + +- stable id, location, lens, title, confidence - claim - evidence - why it matters - suggested fix or next step -- existing review overlap rationale when present +- recommendation rationale: one sentence explaining why this finding is + recommended for posting, synthesized from severity, confidence, and overlap + status + +### 3. Related to existing threads (full detail) + +For each finding, include the same fields as recommended findings, plus: + +- existing review overlap rationale +- recommendation rationale explaining why this is related to an existing thread + rather than standalone + +### 4. Discussion-worthy (full detail) + +For each finding, include the same fields as recommended findings, with the +recommendation rationale explaining why this is not recommended to post. + +### 5. Already covered (one-liner per finding) + +One line per finding: `id — title (covered by thread on path:line)`. + +### 6. Discarded (one-liner per finding) + +One line per finding: `id — title (reason)`. + +### 7. Positive observations + +List positive observations when present. ## Ask What To Do Next -After presenting the board, ask with `AskUserQuestion`: +After presenting the board, propose a recommended action based on board state +using `AskUserQuestion` with contextual options. -```text -What should we do next? You can reply with commands like "draft recommended", -"draft F1 F3", "plus-one F2", "explain F4", "challenge F5", "show covered", -"skip F6", "post selected", or "cancel". -``` +### When recommended findings exist -Use a free-text response, not option buttons. Interpret natural language -flexibly, but preserve the review board ids as the stable selection handles. +Write a brief assessment of the recommended findings and any notable overlaps, +then offer options: -Support these actions: +1. "Draft recommended findings" (first option — the recommended action) +2. "Draft all including overlap endorsements" +3. "I want to adjust the selection" +4. "Cancel" -- `draft recommended`: draft all `recommendedToPost` findings. -- `draft F1 F3`: draft selected findings. -- `plus-one F2`: draft a concise endorsement for an overlap finding. -- `skip F4`: mark a finding as intentionally omitted in the conversation. -- `explain F5`: explain the evidence, uncertainty, and tradeoffs. -- `challenge F6`: reassess the finding using the board evidence and state any - uncertainty plainly. -- `show covered`: show `alreadyCovered` and relevant overlap rationale. -- `cancel`: stop without drafting or posting. -- `post selected`: only continue if there is already an approved preview; - otherwise draft and preview first. +### When only overlap or discussion findings exist + +1. "Endorse overlap findings" +2. "Skip posting" +3. "I want to discuss specific findings" +4. "Cancel" + +### When nothing is postable + +1. "Leave an approving review" +2. "I spotted something" +3. "Done" + +### Exploratory actions + +There are no dedicated commands. The user can type anything via the Other field, +such as "Tell me more about F3" or "I disagree with F1". Interpret natural +language flexibly, respond accordingly, and loop back to present updated options. ## Draft Selected Comments @@ -299,38 +325,64 @@ Draft comments only in the conversation. Drafts should: - avoid duplicating comments already covered elsewhere - distinguish blocking concerns from optional suggestions -For possible plus-ones and partial overlaps, make the overlap explicit. Draft a -plus-one only when the finding's `existingReviewOverlap` indicates that an -endorsement or additional detail is useful. +### Drafting for overlap findings (relatedToExisting) + +For findings in `relatedToExisting`, draft as thread replies rather than +standalone comments. The draft should: + +- acknowledge the original comment and add the new perspective +- avoid restating the concern from scratch +- read naturally as a reply in the existing conversation + +### Line comments vs review body Prefer line comments for findings with a concrete changed-file location. Put findings without a valid line location in the review body. +### Review event + Choose the proposed review event from the selected findings: - `REQUEST_CHANGES` only when at least one selected finding is a serious correctness or blocking concern. -- `COMMENT` for non-blocking feedback, suggestions, plus-ones, or discussion. +- `COMMENT` for non-blocking feedback, suggestions, endorsements, or discussion. +- `APPROVE` when the user selected "Leave an approving review" from the + nothing-postable menu and no findings are being posted. ## Preview And Confirm -Before posting, show an exact preview: +Before posting, show an exact preview. + +For each finding being posted as a new line comment, show: + +- finding id, path, line, and body + +For each overlap finding being posted as a thread reply, show: -- each line comment with finding id, path, line, and body -- review body text for non-line findings -- proposed review event: `COMMENT` or `REQUEST_CHANGES` -- any selected findings intentionally omitted from posting +- finding id, "Reply to thread on path:line", and body -Ask for explicit final approval with `AskUserQuestion`. Accept approval only -when the user clearly confirms posting the preview, such as "post this", -"approved", or "submit". If the user requests edits or removals, update the -preview and ask for approval again. +For review body text (non-line findings), show the review body. + +Show the proposed review event: `COMMENT`, `REQUEST_CHANGES`, or `APPROVE`. + +After the preview, ask for explicit approval with `AskUserQuestion`: + +1. "Post this review" +2. "Edit the draft" +3. "Add or remove findings" +4. "Cancel" + +Accept approval only when the user selects "Post this review" or clearly +confirms posting. If the user requests edits or removals, update the preview and +ask for approval again. ## Post Approved Review Use GitHub write tools only in this final approved step. -If the approved preview has line comments: +### Posting new line comments + +If the approved preview has new line comments: 1. Create a pending review with `mcp__plugin_github_github__pull_request_review_write`. @@ -340,9 +392,26 @@ If the approved preview has line comments: `mcp__plugin_github_github__pull_request_review_write` using the approved event and review body. +### Posting thread replies for overlap findings + +For findings with `existingReviewOverlap.status === 'overlaps'` and a valid +numeric `existingReviewOverlap.commentId`, post as a reply to the existing +thread using `mcp__plugin_github_github__add_reply_to_pull_request_comment` +with the `commentId` and `pullNumber`. + +If the `commentId` is missing or invalid, fall back to posting as a new line +comment via the pending review flow. + +Thread replies are posted independently of the pending review flow — they do +not need to be part of the pending review submission. + +### Review body only + If the approved preview has only review-body text, submit the review body with `mcp__plugin_github_github__pull_request_review_write` using the approved event. +### Invalid locations + If a line comment cannot be added because the location is invalid for the PR diff, move that text into the review body, show the revised preview, and ask for approval again before posting. diff --git a/pr-review-toolkit/skills/review-pr/review-pr.js b/pr-review-toolkit/skills/review-pr/review-pr.js index da16b0d..690173d 100644 --- a/pr-review-toolkit/skills/review-pr/review-pr.js +++ b/pr-review-toolkit/skills/review-pr/review-pr.js @@ -116,7 +116,7 @@ const FINDING_SCHEMA = { properties: { recommendation: { type: 'string', - enum: ['recommended_to_post', 'possible_plus_one', 'partial_overlap', 'discussion_only', 'already_covered', 'discard'] + enum: ['recommended_to_post', 'overlaps', 'discussion_only', 'already_covered', 'discard'] }, rationale: { type: 'string' }, existingThreadIds: { @@ -151,6 +151,7 @@ const THREAD_SCHEMA = { type: 'object', properties: { id: { type: 'string' }, + commentId: { type: 'number' }, path: { type: 'string' }, line: { type: 'number' }, author: { type: 'string' }, @@ -200,9 +201,10 @@ const BOARD_ITEM_SCHEMA = { properties: { status: { type: 'string', - enum: ['none', 'possible_plus_one', 'partial_overlap', 'already_covered'] + enum: ['none', 'overlaps', 'already_covered'] }, threadId: { type: 'string' }, + commentId: { type: 'number' }, rationale: { type: 'string' } }, required: ['status', 'rationale'] @@ -219,11 +221,7 @@ const REVIEW_BOARD_SCHEMA = { type: 'array', items: BOARD_ITEM_SCHEMA }, - possiblePlusOnes: { - type: 'array', - items: BOARD_ITEM_SCHEMA - }, - partialOverlaps: { + relatedToExisting: { type: 'array', items: BOARD_ITEM_SCHEMA }, @@ -242,39 +240,9 @@ const REVIEW_BOARD_SCHEMA = { positiveObservations: { type: 'array', items: { type: 'string' } - }, - actionPlan: { - type: 'object', - properties: { - critical: { - type: 'array', - items: { type: 'string' } - }, - important: { - type: 'array', - items: { type: 'string' } - }, - suggestions: { - type: 'array', - items: { type: 'string' } - }, - recommendedNextAction: { type: 'string' } - }, - required: ['critical', 'important', 'suggestions', 'recommendedNextAction'] - }, - coverageSummary: { - type: 'object', - properties: { - scope: { type: 'string' }, - largePrNotes: { - type: 'array', - items: { type: 'string' } - } - }, - required: ['scope', 'largePrNotes'] } }, - required: ['recommendedToPost', 'possiblePlusOnes', 'partialOverlaps', 'discussionOnly', 'alreadyCovered', 'discarded', 'positiveObservations', 'actionPlan', 'coverageSummary'] + required: ['recommendedToPost', 'relatedToExisting', 'discussionOnly', 'alreadyCovered', 'discarded', 'positiveObservations'] } let config = {} @@ -682,7 +650,7 @@ Think deeply about each type's role in the larger system. Sometimes a simpler ty Map each finding to severity (critical/important/suggestion) and confidence (0-100). Only report findings with confidence >= 50.` } -const STANDARDIZATION_SUFFIX = `Return only high-signal candidate findings. For each finding, provide a concise title, a concrete claim, structured evidence, specialist reasoning, why it matters, and a specific suggested fix when applicable. Include a postability recommendation for human review only: recommended_to_post, possible_plus_one, partial_overlap, discussion_only, already_covered, or discard. Preserve concrete evidence from patches, files, and existing threads; do not collapse reasoning into generic summaries. Use a neutral technical voice and do not reference yourself, your role, or your review methodology.` +const STANDARDIZATION_SUFFIX = `Return only high-signal candidate findings. For each finding, provide a concise title, a concrete claim, structured evidence, specialist reasoning, why it matters, and a specific suggested fix when applicable. Include a postability recommendation for human review only: recommended_to_post, overlaps, discussion_only, already_covered, or discard. Preserve concrete evidence from patches, files, and existing threads; do not collapse reasoning into generic summaries. Use a neutral technical voice and do not reference yourself, your role, or your review methodology.` const FILE_PAGE_SIZE = 10 const FILE_SINGLE_PAGE_RETRIES = 2 @@ -721,7 +689,7 @@ const REVIEWERS = { } } -const BOARD_SECTIONS = ['recommendedToPost', 'possiblePlusOnes', 'partialOverlaps', 'discussionOnly', 'alreadyCovered', 'discarded'] +const BOARD_SECTIONS = ['recommendedToPost', 'relatedToExisting', 'discussionOnly', 'alreadyCovered', 'discarded'] function asNumber(value, fallback) { const parsed = Number(value) @@ -799,17 +767,16 @@ function overlapFromFinding(finding) { const postability = finding.postability || {} const recommendation = postability.recommendation || '' const threadIds = Array.isArray(postability.existingThreadIds) ? postability.existingThreadIds : [] - const status = recommendation === 'possible_plus_one' - ? 'possible_plus_one' - : recommendation === 'partial_overlap' - ? 'partial_overlap' - : recommendation === 'already_covered' - ? 'already_covered' - : 'none' + const status = recommendation === 'overlaps' + ? 'overlaps' + : recommendation === 'already_covered' + ? 'already_covered' + : 'none' return { status: status, threadId: threadIds[0] || '', + commentId: undefined, rationale: postability.rationale || 'No existing review overlap was classified.' } } @@ -900,7 +867,7 @@ function bestSeverity(left, right) { } function bestOverlap(left, right) { - const order = { none: 0, possible_plus_one: 1, partial_overlap: 2, already_covered: 3 } + const order = { none: 0, overlaps: 1, already_covered: 2 } const leftStatus = (left && left.status) || 'none' const rightStatus = (right && right.status) || 'none' const selected = (order[rightStatus] > order[leftStatus]) ? right : left @@ -909,6 +876,7 @@ function bestOverlap(left, right) { return { status: selected.status || 'none', threadId: selected.threadId || '', + commentId: selected.commentId || undefined, rationale: combineText(left && left.rationale, right && right.rationale) } } @@ -932,7 +900,15 @@ function mergeBoardItem(base, next) { function inferThreadOverlap(item, threads) { const existing = item.existingReviewOverlap || {} - if (existing.status && existing.status !== 'none') return existing + if (existing.status && existing.status !== 'none') { + if (!existing.commentId && existing.threadId) { + const matched = (threads || []).find(t => t && t.id === existing.threadId) + if (matched && matched.commentId) { + return Object.assign({}, existing, { commentId: matched.commentId }) + } + } + return existing + } const location = item.location || {} const itemText = combinedItemText(item) @@ -955,19 +931,19 @@ function inferThreadOverlap(item, threads) { return { status: 'none', threadId: existing.threadId || '', + commentId: existing.commentId || undefined, rationale: existing.rationale || 'No existing review overlap was classified.' } } const status = best.thread.isResolved && best.overlap >= 0.25 ? 'already_covered' - : best.sameLine && best.overlap >= 0.2 - ? 'possible_plus_one' - : 'partial_overlap' + : 'overlaps' return { status: status, threadId: best.thread.id || '', + commentId: best.thread.commentId || undefined, rationale: 'Inferred overlap with an existing review thread on ' + location.path + (best.thread.line != null ? ':' + best.thread.line : '') + '.' } } @@ -976,8 +952,7 @@ function routeSection(item, preferredSection, recommendation) { const overlap = item.existingReviewOverlap || {} if (preferredSection === 'discarded' || recommendation === 'discard') return 'discarded' if (overlap.status === 'already_covered' || recommendation === 'already_covered') return 'alreadyCovered' - if (overlap.status === 'partial_overlap' || recommendation === 'partial_overlap') return 'partialOverlaps' - if (overlap.status === 'possible_plus_one' || recommendation === 'possible_plus_one') return 'possiblePlusOnes' + if (overlap.status === 'overlaps' || recommendation === 'overlaps') return 'relatedToExisting' if (recommendation === 'discussion_only') return 'discussionOnly' if (recommendation === 'recommended_to_post') return 'recommendedToPost' if (BOARD_SECTIONS.indexOf(preferredSection) !== -1) return preferredSection @@ -991,9 +966,8 @@ function bestRecommendation(left, right) { discard: 0, already_covered: 1, discussion_only: 2, - partial_overlap: 3, - possible_plus_one: 4, - recommended_to_post: 5 + overlaps: 3, + recommended_to_post: 4 } const leftScore = Object.prototype.hasOwnProperty.call(order, left) ? order[left] : -1 const rightScore = Object.prototype.hasOwnProperty.call(order, right) ? order[right] : -1 @@ -1046,22 +1020,6 @@ function normalizeBoardSections(board, prContext) { }) } -function actionPlanForBoard(board) { - const actionable = board.recommendedToPost.concat(board.possiblePlusOnes, board.partialOverlaps, board.discussionOnly) - return { - critical: actionable.filter(item => item.severity === 'critical').map(item => item.id + ': ' + item.title), - important: actionable.filter(item => item.severity === 'important').map(item => item.id + ': ' + item.title), - suggestions: actionable.filter(item => item.severity === 'suggestion').map(item => item.id + ': ' + item.title), - recommendedNextAction: board.recommendedToPost.length > 0 - ? 'review recommended postable findings' - : (board.possiblePlusOnes.length + board.partialOverlaps.length) > 0 - ? 'review overlap findings before posting' - : board.discussionOnly.length > 0 - ? 'review discussion-only findings' - : 'no postable findings identified' - } -} - function uniq(values) { const seen = {} const out = [] @@ -1455,22 +1413,11 @@ function boardItemFromFinding(finding, index) { function fallbackBoard(findings, positives, prContext) { const board = { recommendedToPost: [], - possiblePlusOnes: [], - partialOverlaps: [], + relatedToExisting: [], discussionOnly: [], alreadyCovered: [], discarded: [], - positiveObservations: positives, - actionPlan: { - critical: [], - important: [], - suggestions: [], - recommendedNextAction: 'review board' - }, - coverageSummary: { - scope: coverageScope(prContext), - largePrNotes: largePrNotes(prContext) - } + positiveObservations: positives } const entries = findings.map((finding, index) => { @@ -1486,42 +1433,9 @@ function fallbackBoard(findings, positives, prContext) { board[routeSection(entry.item, '', entry.recommendation)].push(entry.item) }) BOARD_SECTIONS.forEach(section => sortFindings(board[section])) - board.actionPlan = actionPlanForBoard(board) return board } -function coverageScope(prContext) { - const summary = prContext.summary || {} - const sources = prContext.sources || {} - const categories = summary.categories || {} - const categoryText = Object.keys(categories).sort().map(key => key + ': ' + categories[key]).join(', ') - const sourceNote = sources.manifestSource === 'local-git' - ? ' from local git merge ref' - : ' across ' + (prContext.filePageCount || 0) + ' page(s) via GitHub API' - return 'Collected ' + (summary.collectedFileCount || 0) + ' changed file(s)' + sourceNote - + ' with ' + (summary.existingThreadCount || 0) + ' existing review thread(s). Categories: ' - + (categoryText || 'none') + '.' -} - -function largePrNotes(prContext) { - const summary = prContext.summary || {} - const sources = prContext.sources || {} - const notes = [] - if (summary.scale === 'large' || summary.scale === 'very_large') { - if (sources.fullDiffIncluded) { - notes.push('Large PR: reviewers received the complete manifest and full merge diff.') - } else if (sources.manifestSource === 'local-git') { - notes.push('Large PR: reviewers received the complete manifest. Full diff exceeded size cap; specialists used manifest plus Read/Grep on the merged checkout.') - } else { - notes.push('Large PR: reviewers received the complete manifest via GitHub API. Full diff was not preloaded; specialists used focused GitHub patch reads when needed.') - } - } - const lowSignal = (summary.categories && ((summary.categories.vendor || 0) + (summary.categories.generated || 0) + (summary.categories.lockfile || 0))) || 0 - if (lowSignal > 0) notes.push(lowSignal + ' vendor/generated/lockfile file(s) were kept visible in the manifest but deprioritized for detailed review.') - if (sources.fallbackReason) notes.push('Fell back to MCP for diff collection: ' + sources.fallbackReason) - return notes -} - function finalizeBoard(board, findings, positives, prContext) { const finalBoard = board || fallbackBoard(findings, positives, prContext) BOARD_SECTIONS.forEach(section => { @@ -1551,13 +1465,6 @@ function finalizeBoard(board, findings, positives, prContext) { }) }) if (!Array.isArray(finalBoard.positiveObservations)) finalBoard.positiveObservations = positives - finalBoard.actionPlan = actionPlanForBoard(finalBoard) - if (!finalBoard.coverageSummary) { - finalBoard.coverageSummary = { - scope: coverageScope(prContext), - largePrNotes: largePrNotes(prContext) - } - } // These fields intentionally extend the synthesizer schema; the skill // presents them as part of the final review board contract. @@ -1651,7 +1558,7 @@ if (localGitManifest && localGitManifest.length > 0) { if (pr.changedFiles === 0) pr.changedFiles = files.length log('Fetching review threads') -const threadPrompt = `Use GitHub read tools only. Fetch all review comment threads via pull_request_read method get_review_comments for ${pr.owner}/${pr.repo} PR #${pr.number}. Paginate if needed. Return compact thread records only: id (thread node id when available), path, line, author login of the first comment, body of the first comment, isResolved, and replies with author/body. Do not call any GitHub write tools.` +const threadPrompt = `Use GitHub read tools only. Fetch all review comment threads via pull_request_read method get_review_comments for ${pr.owner}/${pr.repo} PR #${pr.number}. Paginate if needed. Return compact thread records only: id (thread node id when available), commentId (the numeric comment ID from discussion_r anchors, as a number), path, line, author login of the first comment, body of the first comment, isResolved, and replies with author/body. Do not call any GitHub write tools.` const threadData = await agent(threadPrompt, { label: 'collect-review-threads', @@ -1734,7 +1641,7 @@ const synthesisInput = { positiveObservations: allPositive } -const synthPrompt = `You are synthesizing a human-centered PR review board from specialist candidate findings.\n\nDo not call tools. Use only the JSON input below.\n\n${JSON.stringify(synthesisInput)}\n\nBuild a review board grouped by outcome:\n- recommendedToPost: high-signal findings that look postable by a human reviewer and are not already covered by existing review threads.\n- possiblePlusOnes: findings where an existing thread already raises the issue but an endorsement may help.\n- partialOverlaps: findings that add useful information beyond an existing thread.\n- discussionOnly: useful reviewer notes that should not be posted as comments yet.\n- alreadyCovered: findings fully covered by existing human or bot review threads.\n- discarded: weak, low-confidence, duplicate, or not-actionable findings.\n\nSynthesis rules:\n1. Merge duplicate specialist findings by logical concern before assigning a section. Same concern means the same bug, risk, missing test, comment problem, or type-design issue, even when titles differ.\n2. Preserve specialist evidence and reasoning in the existing board fields, especially evidence, whyItMatters, suggestedFix, and existingReviewOverlap.rationale. When merging duplicates, combine non-redundant evidence rather than dropping it.\n3. Classify against existing review threads by logical concern, not just file proximity. Use existingReviewOverlap.status values none, possible_plus_one, partial_overlap, or already_covered.\n4. Use each specialist's postability recommendation as an input, not a command. Do not invent posting or drafting behavior.\n5. Include positive observations when useful. The action plan should be easy to scan: critical issues first, important issues next, optional suggestions last, and one recommended next action. The coverage summary must be honest about large-PR scope and low-signal areas.` +const synthPrompt = `You are synthesizing a human-centered PR review board from specialist candidate findings.\n\nDo not call tools. Use only the JSON input below.\n\n${JSON.stringify(synthesisInput)}\n\nBuild a review board grouped by outcome:\n- recommendedToPost: high-signal findings that look postable by a human reviewer and are not already covered by existing review threads.\n- relatedToExisting: findings that overlap with an existing review thread — either as an endorsement or with additional detail beyond what the thread covers.\n- discussionOnly: useful reviewer notes that should not be posted as comments yet.\n- alreadyCovered: findings fully covered by existing human or bot review threads.\n- discarded: weak, low-confidence, duplicate, or not-actionable findings.\n\nSynthesis rules:\n1. Merge duplicate specialist findings by logical concern before assigning a section. Same concern means the same bug, risk, missing test, comment problem, or type-design issue, even when titles differ.\n2. Preserve specialist evidence and reasoning in the existing board fields, especially evidence, whyItMatters, suggestedFix, and existingReviewOverlap.rationale. When merging duplicates, combine non-redundant evidence rather than dropping it.\n3. Classify against existing review threads by logical concern, not just file proximity. Use existingReviewOverlap.status values none, overlaps, or already_covered.\n4. Use each specialist's postability recommendation as an input, not a command. Do not invent posting or drafting behavior.\n5. Include positive observations when useful.` const synthesized = await agent(synthPrompt, { label: 'synthesize-review-board', From dc16454a9b82b633fb7c43ab18d6150d84512307 Mon Sep 17 00:00:00 2001 From: Christoph Blecker Date: Fri, 3 Jul 2026 12:11:16 -0700 Subject: [PATCH 2/2] refactor(pr-review-toolkit): trim SKILL.md to resolve token budget warning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tighten prose across all sections to bring estimated token count under the 3,000-token skillsaw warn threshold (~3,379 → ~2,450). Preserves all behavioral instructions including ad-hoc script prohibition and allowed-tools constraints. Assisted-by: Claude:claude-opus-4-6 --- pr-review-toolkit/skills/review-pr/SKILL.md | 189 +++++------------- .../skills/review-pr/review-pr.js | 4 +- 2 files changed, 50 insertions(+), 143 deletions(-) diff --git a/pr-review-toolkit/skills/review-pr/SKILL.md b/pr-review-toolkit/skills/review-pr/SKILL.md index c5e74fa..92d34c0 100644 --- a/pr-review-toolkit/skills/review-pr/SKILL.md +++ b/pr-review-toolkit/skills/review-pr/SKILL.md @@ -28,21 +28,13 @@ allowed-tools: ## Constraints -Use only the tools listed in `allowed-tools`. Do not generate ad-hoc scripts to -process GitHub data. Workflow return values and MCP responses are structured -JSON; read them directly. +Use only `allowed-tools`. Do not generate ad-hoc processing scripts. Workflow +return values and MCP responses are structured JSON; read them directly. Bash is +limited to the git patterns in `allowed-tools` for preflight and diff steps +below. -Bash access is intentionally limited to the `git fetch`, `git checkout`, -`git rev-parse *`, and `git diff *` patterns listed in `allowed-tools`. Use them -only for the exact local-git preflight and diff commands below. The three git -environment checks above were injected during skill preprocessing and do not -require Bash tool calls. +The workflow and its agents are read-only — they must not call GitHub write tools. -The bundled workflow and workflow-spawned agents are analysis-only. They must -use GitHub read tools only and must not draft pending reviews, add comments, -submit reviews, resolve threads, or call GitHub write tools. - -The skill conversation may draft comment text after the user selects findings. GitHub write tools may be used only after an exact preview and explicit final posting approval from the user. @@ -56,84 +48,54 @@ https://github.com/{owner}/{repo}/pull/{number} ## Fetch PR Metadata -Before local git preflight, fetch PR metadata to obtain the head SHA and base -repository information. Call `mcp__plugin_github_github__pull_request_read` with -method `get` for the parsed owner, repo, and pull number. - -Extract and record: +Call `pull_request_read` with method `get`. Extract and record: - `headSha`: the current head commit SHA of the PR - `changedFiles`: the number of changed files - base repository clone URL (typically `https://github.com/{owner}/{repo}`) -These values are needed to verify the local git checkout. - ## Local Git Preflight -Attempt to set up a verified local git checkout of the PR merge result. If any -check fails, record the failure reason, skip to the Launch Analysis Workflow -section, and pass no `localGitManifest` or `fullDiff` in the workflow args. The -workflow will fall back to MCP-based file collection. +Set up a verified local checkout of the PR merge result. If any check below +fails, record the reason as `fallbackReason`, skip remaining preflight, and +launch the workflow without `localGitManifest` or `fullDiff`. ### Check injected git environment Read the three values from the Git Environment section above. -1. If repository root is `__NOT_A_GIT_REPO__`, record fallback reason "not a - git repository" and skip to workflow launch. -2. If worktree state is `__WORKTREE_DIRTY__`, record fallback reason "worktree - has uncommitted changes" and skip to workflow launch. -3. If origin URL is `__NO_ORIGIN_REMOTE__`, record fallback reason "no origin - remote" and skip to workflow launch. -4. Compare the origin URL to the PR base repository URL. The origin URL may use - HTTPS (`https://github.com/{owner}/{repo}.git` or - `https://github.com/{owner}/{repo}`) or SSH - (`git@github.com:{owner}/{repo}.git`). Normalize both to `{owner}/{repo}` - for comparison (case-insensitive). If they do not match, record fallback - reason "origin does not match PR base repository" and skip to workflow - launch. +1. `__NOT_A_GIT_REPO__` → "not a git repository" +2. `__WORKTREE_DIRTY__` → "worktree has uncommitted changes" +3. `__NO_ORIGIN_REMOTE__` → "no origin remote" +4. Normalize origin URL and PR base URL to `{owner}/{repo}` (strip protocol, + `.git` suffix; case-insensitive). Mismatch → "origin does not match PR base + repository" ### Fetch merge ref -Run this exact command, substituting the PR number: - ```bash git fetch origin refs/pull/{number}/merge ``` -If this fails, the merge ref may not exist (e.g., the PR has merge conflicts or -is closed). Record fallback reason "merge ref fetch failed" and skip to workflow -launch. +Failure → "merge ref fetch failed". ### Checkout merge commit -Run this exact command: - ```bash git checkout --detach FETCH_HEAD ``` -This checks out the merge result as a detached HEAD. Do NOT auto-restore the -original ref afterward. +Do NOT auto-restore the original ref afterward. ### Verify merge parents -Run this exact command (all three refs in one call): - ```bash git rev-parse HEAD HEAD^1 HEAD^2 ``` -This outputs three lines: - -- first line = `mergeCommit` (the merge commit SHA) -- second line = `baseSha` (the base branch parent) -- third line = `headSha` (the PR head parent) - -Verify that the third line matches the `headSha` from the PR metadata fetched -earlier. If they do not match, record fallback reason "HEAD^2 does not match PR -headSha" and skip to workflow launch. Do NOT trust local diff data when the -merge parents do not match. +Output: line 1 = `mergeCommit`, line 2 = `baseSha`, line 3 = `headSha` (PR +head). If line 3 does not match the PR metadata `headSha`, record "HEAD^2 does +not match PR headSha". Do not trust local diff data on mismatch. ## Build Local Git Manifest @@ -141,87 +103,49 @@ If all preflight checks passed, build the file manifest from local git. ### Collect file statuses -Run this exact command: - ```bash git diff --name-status -z HEAD^1 HEAD ``` -Parse the NUL-delimited output into `{path, status}` entries. Map `A`, `M`, `D`, -`R*`, and `C*` to `added`, `modified`, `deleted`, `renamed`, and `copied`. For -renames and copies, use the destination path. +Parse NUL-delimited output into `{path, status}` entries. Map `A`, `M`, `D`, +`R*`, `C*` to `added`, `modified`, `deleted`, `renamed`, `copied`. For renames +and copies, use the destination path. ### Collect line counts -Run this exact command: - ```bash git diff --numstat -z HEAD^1 HEAD ``` -Parse the NUL-delimited numstat output and merge it with the status list. Normal -files have additions, deletions, and path. Rename/copy entries include source -and destination paths; use the destination path as the merge key. Binary files -show `-` for additions and deletions; store both as 0. Each manifest entry must -have this shape: - -```json -{ - "path": "pkg/auth/session.go", - "status": "modified", - "additions": 42, - "deletions": 12 -} -``` +Parse NUL-delimited numstat and merge with the status list. For renames/copies +use destination path as key. Binary files show `-` for additions/deletions; +store as 0. Each entry has shape: `{path, status, additions, deletions}`. The resulting array is the `localGitManifest`. ## Collect Full Diff (Optional) -If the local git manifest was built successfully, attempt to collect the full -merge diff. - -Run this exact command: +If the manifest was built, collect the full merge diff: ```bash git diff --no-ext-diff --no-textconv HEAD^1 HEAD ``` -If the output is 200,000 characters or fewer, store it as the `fullDiff` string -to pass to the workflow. If the output exceeds 200,000 characters, do not pass -`fullDiff`. +Store as `fullDiff` if 200,000 characters or fewer; otherwise omit. ## Launch Analysis Workflow Invoke the Workflow tool with: - `scriptPath`: `${CLAUDE_SKILL_DIR}/review-pr.js` -- `args`: - - `owner` - - `repo` - - `pullNumber` - - `localGitManifest` (array of manifest entries, or omit if preflight failed) - - `fullDiff` (string, or omit if not collected or preflight failed) - - `sources` (object with preflight metadata; the workflow derives - `manifestSource` and `patchSource` from the presence of `localGitManifest` - and `fullDiff`): - - `mergeCommit`: the merge commit SHA (or empty string) - - `baseSha`: the base parent SHA (or empty string) - - `headSha`: the head parent SHA (or empty string) - - `fullDiffIncluded`: true if fullDiff is being passed - - `fallbackReason`: reason preflight failed (or empty string) - -If preflight failed, omit `localGitManifest` and `fullDiff`, but still pass -`sources` with empty `mergeCommit`, `baseSha`, and `headSha`, -`fullDiffIncluded: false`, and the preserved `fallbackReason` so the workflow -records why local git was not used. - -The workflow owns PR metadata collection (via MCP), reviewer selection, -specialist analysis, and review-board synthesis. It also fetches review threads -(always from MCP). - -The workflow returns grouped findings, positive observations, PR metadata, -summary, and review metadata. +- `args`: `owner`, `repo`, `pullNumber`, `localGitManifest` (omit if preflight + failed), `fullDiff` (omit if not collected), and `sources`: + - `mergeCommit`, `baseSha`, `headSha` (empty strings if preflight failed) + - `fullDiffIncluded`: whether fullDiff is included + - `fallbackReason`: reason preflight failed (or empty string) + +The workflow collects PR metadata via MCP, runs specialist analysis, fetches +review threads, and returns grouped findings with review metadata. ## Present Review Board @@ -257,16 +181,11 @@ For each finding, include: ### 3. Related to existing threads (full detail) -For each finding, include the same fields as recommended findings, plus: - -- existing review overlap rationale -- recommendation rationale explaining why this is related to an existing thread - rather than standalone +Same fields as recommended, plus existing review overlap rationale. ### 4. Discussion-worthy (full detail) -For each finding, include the same fields as recommended findings, with the -recommendation rationale explaining why this is not recommended to post. +Same fields as recommended; rationale explains why not recommended to post. ### 5. Already covered (one-liner per finding) @@ -308,11 +227,8 @@ then offer options: 2. "I spotted something" 3. "Done" -### Exploratory actions - -There are no dedicated commands. The user can type anything via the Other field, -such as "Tell me more about F3" or "I disagree with F1". Interpret natural -language flexibly, respond accordingly, and loop back to present updated options. +The user may type free-form text via Other (e.g., "Tell me more about F3"). +Respond accordingly and loop back to updated options. ## Draft Selected Comments @@ -325,14 +241,10 @@ Draft comments only in the conversation. Drafts should: - avoid duplicating comments already covered elsewhere - distinguish blocking concerns from optional suggestions -### Drafting for overlap findings (relatedToExisting) - -For findings in `relatedToExisting`, draft as thread replies rather than -standalone comments. The draft should: +### Overlap findings -- acknowledge the original comment and add the new perspective -- avoid restating the concern from scratch -- read naturally as a reply in the existing conversation +Draft `relatedToExisting` findings as thread replies: acknowledge the original +comment, add the new perspective, and avoid restating the concern. ### Line comments vs review body @@ -394,16 +306,11 @@ If the approved preview has new line comments: ### Posting thread replies for overlap findings -For findings with `existingReviewOverlap.status === 'overlaps'` and a valid -numeric `existingReviewOverlap.commentId`, post as a reply to the existing -thread using `mcp__plugin_github_github__add_reply_to_pull_request_comment` -with the `commentId` and `pullNumber`. - -If the `commentId` is missing or invalid, fall back to posting as a new line -comment via the pending review flow. - -Thread replies are posted independently of the pending review flow — they do -not need to be part of the pending review submission. +Post overlapping findings as replies using +`add_reply_to_pull_request_comment` with the numeric `commentId` and +`pullNumber`. If `commentId` is missing or invalid, post as a new line comment +via the pending review flow instead. Thread replies are independent of the +pending review submission. ### Review body only diff --git a/pr-review-toolkit/skills/review-pr/review-pr.js b/pr-review-toolkit/skills/review-pr/review-pr.js index 690173d..10353b8 100644 --- a/pr-review-toolkit/skills/review-pr/review-pr.js +++ b/pr-review-toolkit/skills/review-pr/review-pr.js @@ -876,7 +876,7 @@ function bestOverlap(left, right) { return { status: selected.status || 'none', threadId: selected.threadId || '', - commentId: selected.commentId || undefined, + commentId: selected.commentId || (right && right.commentId) || (left && left.commentId) || undefined, rationale: combineText(left && left.rationale, right && right.rationale) } } @@ -907,7 +907,7 @@ function inferThreadOverlap(item, threads) { return Object.assign({}, existing, { commentId: matched.commentId }) } } - return existing + if (existing.threadId || existing.commentId) return existing } const location = item.location || {}