From e87618c0e73269055f84d2c69882be7687735b9c Mon Sep 17 00:00:00 2001 From: Adam Creeger Date: Tue, 26 May 2026 11:58:45 -0400 Subject: [PATCH] feat(mcp): add get_reviews tool for PR review submissions Adds a new `get_reviews` MCP tool that fetches PR review submissions (the body text and verdict submitted via the "Submit review" flow). This is distinct from inline review comments (get_review_comments) and PR comments (get_pr). The pr-prompt now cross-references review submissions against inline comments to identify truly unresolved feedback, preventing false reports when findings have already been addressed via a review submission. Also normalizes `pullRequestReviewId` from number to string for provider-agnostic ID handling, adds NaN guard on PR number parsing, and instructs agents to reply inline to specific comment threads rather than posting standalone reviews. --- src/lib/VersionControlProvider.ts | 13 ++ .../bitbucket/BitBucketVCSProvider.ts | 7 +- src/mcp/CLAUDE.md | 2 +- src/mcp/GitHubIssueManagementProvider.test.ts | 123 +++++++++++++++++- src/mcp/GitHubIssueManagementProvider.ts | 55 +++++++- src/mcp/issue-management-server.ts | 99 +++++++++++++- src/mcp/types.ts | 22 +++- templates/prompts/pr-prompt.txt | 29 ++++- 8 files changed, 340 insertions(+), 10 deletions(-) diff --git a/src/lib/VersionControlProvider.ts b/src/lib/VersionControlProvider.ts index 5021b08d..adfdcfd3 100644 --- a/src/lib/VersionControlProvider.ts +++ b/src/lib/VersionControlProvider.ts @@ -20,6 +20,18 @@ export interface ExistingPR { url: string } +/** + * Represents a review submission on a pull request (the body text submitted via "Submit review") + */ +export interface ReviewSubmission { + id: string + body: string + state: string // APPROVED, CHANGES_REQUESTED, COMMENTED, DISMISSED, PENDING (GitHub); similar for other providers + author: { id: string; displayName: string } | null + submittedAt: string + htmlUrl: string +} + /** * Represents an inline review comment on a pull request */ @@ -76,6 +88,7 @@ export interface VersionControlProvider { createPRComment(prNumber: number, body: string, cwd?: string): Promise<{ id: string; url: string }> updatePRComment?(prNumber: number, commentId: string, body: string, cwd?: string): Promise<{ id: string; url: string }> getReviewComments?(prNumber: number, cwd?: string): Promise + getReviewSubmissions?(prNumber: number, cwd?: string): Promise createReviewComment?(prNumber: number, path: string, line: number, body: string, cwd?: string): Promise<{ id: string; url: string }> // Remote and repository detection diff --git a/src/lib/providers/bitbucket/BitBucketVCSProvider.ts b/src/lib/providers/bitbucket/BitBucketVCSProvider.ts index 04ae7d44..00902e72 100644 --- a/src/lib/providers/bitbucket/BitBucketVCSProvider.ts +++ b/src/lib/providers/bitbucket/BitBucketVCSProvider.ts @@ -1,7 +1,7 @@ // BitBucketVCSProvider - Implements VersionControlProvider for BitBucket // Provides PR/VCS operations via BitBucket REST API -import type { VersionControlProvider, ExistingPR, PRCreationResult, ReviewComment } from '../../VersionControlProvider.js' +import type { VersionControlProvider, ExistingPR, PRCreationResult, ReviewComment, ReviewSubmission } from '../../VersionControlProvider.js' import type { PullRequest } from '../../../types/index.js' import { BitBucketApiClient, type BitBucketConfig, type BitBucketPullRequest } from './BitBucketApiClient.js' import type { IloomSettings } from '../../SettingsManager.js' @@ -244,6 +244,11 @@ export class BitBucketVCSProvider implements VersionControlProvider { return inlineComments } + // BitBucket has no "Submit review" flow — approvals are a simple toggle with no body text. + async getReviewSubmissions(_prNumber: number, _cwd?: string): Promise { + return [] + } + /** * Create an inline review comment on a specific file and line in a PR */ diff --git a/src/mcp/CLAUDE.md b/src/mcp/CLAUDE.md index 40320c2b..d3689dea 100644 --- a/src/mcp/CLAUDE.md +++ b/src/mcp/CLAUDE.md @@ -10,7 +10,7 @@ This directory contains MCP (Model Context Protocol) servers that expose tools t Exposes issue/PR operations to agents. Provider-agnostic — routes to GitHub, Linear, or Jira based on `ISSUE_PROVIDER` env var. -**Tools (27):** `get_issue`, `get_pr`, `get_review_comments`, `get_comment`, `create_comment`, `update_comment`, `create_issue`, `create_child_issue`, `create_dependency`, `get_dependencies`, `remove_dependency`, `get_child_issues`, `close_issue`, `reopen_issue`, `edit_issue`, and more. +**Tools (28):** `get_issue`, `get_pr`, `get_review_comments`, `get_reviews`, `get_comment`, `create_comment`, `update_comment`, `create_issue`, `create_child_issue`, `create_dependency`, `get_dependencies`, `remove_dependency`, `get_child_issues`, `close_issue`, `reopen_issue`, `edit_issue`, and more. **Used by:** All execution contexts (regular, swarm child, swarm orchestrator). diff --git a/src/mcp/GitHubIssueManagementProvider.test.ts b/src/mcp/GitHubIssueManagementProvider.test.ts index 7b44e226..6c9920bd 100644 --- a/src/mcp/GitHubIssueManagementProvider.test.ts +++ b/src/mcp/GitHubIssueManagementProvider.test.ts @@ -836,7 +836,7 @@ describe('GitHubIssueManagementProvider', () => { createdAt: '2025-01-01T00:00:00Z', updatedAt: '2025-01-01T01:00:00Z', inReplyToId: null, - pullRequestReviewId: 5000, + pullRequestReviewId: '5000', }) expect(result[1].id).toBe('1002') expect(result[1].path).toBe('src/bar.ts') @@ -876,7 +876,7 @@ describe('GitHubIssueManagementProvider', () => { expect(result).toHaveLength(1) expect(result[0].id).toBe('2001') - expect(result[0].pullRequestReviewId).toBe(100) + expect(result[0].pullRequestReviewId).toBe('100') }) it('handles empty review comments', async () => { @@ -962,6 +962,125 @@ describe('GitHubIssueManagementProvider', () => { }) }) + describe('getReviewSubmissions', () => { + it('returns review submissions with state, body, and author', async () => { + const mockResponse = [ + { + id: 9001, + body: 'LGTM — dismissing the CodeRabbit finding as a false positive', + state: 'APPROVED', + user: { login: 'reviewer1' }, + submitted_at: '2025-01-01T00:00:00Z', + html_url: 'https://github.com/owner/repo/pull/42#pullrequestreview-9001', + }, + { + id: 9002, + body: 'Please fix the null check issue', + state: 'CHANGES_REQUESTED', + user: { login: 'reviewer2' }, + submitted_at: '2025-01-02T00:00:00Z', + html_url: 'https://github.com/owner/repo/pull/42#pullrequestreview-9002', + }, + ] + + vi.mocked(executeGhCommand).mockResolvedValueOnce(mockResponse) + + const result = await provider.getReviewSubmissions({ number: '42' }) + + expect(result).toHaveLength(2) + expect(result[0]).toEqual({ + id: '9001', + body: 'LGTM — dismissing the CodeRabbit finding as a false positive', + state: 'APPROVED', + author: { id: 'reviewer1', displayName: 'reviewer1', login: 'reviewer1' }, + submittedAt: '2025-01-01T00:00:00Z', + htmlUrl: 'https://github.com/owner/repo/pull/42#pullrequestreview-9001', + }) + expect(result[1].state).toBe('CHANGES_REQUESTED') + }) + + it('handles reviews with empty body', async () => { + const mockResponse = [ + { + id: 9003, + body: '', + state: 'APPROVED', + user: { login: 'reviewer' }, + submitted_at: '2025-01-01T00:00:00Z', + html_url: 'https://github.com/owner/repo/pull/42#pullrequestreview-9003', + }, + ] + + vi.mocked(executeGhCommand).mockResolvedValueOnce(mockResponse) + + const result = await provider.getReviewSubmissions({ number: '42' }) + + expect(result).toHaveLength(1) + expect(result[0].body).toBe('') + }) + + it('handles empty reviews list', async () => { + vi.mocked(executeGhCommand).mockResolvedValueOnce([]) + + const result = await provider.getReviewSubmissions({ number: '42' }) + + expect(result).toEqual([]) + }) + + it('passes repo to API path when provided', async () => { + vi.mocked(executeGhCommand).mockResolvedValueOnce([]) + + await provider.getReviewSubmissions({ number: '42', repo: 'other-owner/other-repo' }) + + expect(executeGhCommand).toHaveBeenCalledWith([ + 'api', + 'repos/other-owner/other-repo/pulls/42/reviews', + '--paginate', + '--jq', + expect.any(String), + ]) + }) + + it('uses :owner/:repo placeholder when repo is not provided', async () => { + vi.mocked(executeGhCommand).mockResolvedValueOnce([]) + + await provider.getReviewSubmissions({ number: '42' }) + + expect(executeGhCommand).toHaveBeenCalledWith([ + 'api', + 'repos/:owner/:repo/pulls/42/reviews', + '--paginate', + '--jq', + expect.any(String), + ]) + }) + + it('throws error for non-numeric PR number', async () => { + await expect(provider.getReviewSubmissions({ number: 'not-a-number' })).rejects.toThrow( + 'Invalid GitHub PR number: not-a-number. GitHub PR IDs must be numeric.' + ) + }) + + it('handles reviews with null user', async () => { + const mockResponse = [ + { + id: 9004, + body: 'Automated review', + state: 'COMMENTED', + user: null, + submitted_at: '2025-01-01T00:00:00Z', + html_url: 'https://github.com/owner/repo/pull/42#pullrequestreview-9004', + }, + ] + + vi.mocked(executeGhCommand).mockResolvedValueOnce(mockResponse) + + const result = await provider.getReviewSubmissions({ number: '42' }) + + expect(result[0].author).toBeNull() + }) + }) + describe('createDependency', () => { it('should create dependency between two issues', async () => { vi.mocked(getIssueDatabaseId).mockResolvedValueOnce(123456789) diff --git a/src/mcp/GitHubIssueManagementProvider.ts b/src/mcp/GitHubIssueManagementProvider.ts index b6923617..e801970d 100644 --- a/src/mcp/GitHubIssueManagementProvider.ts +++ b/src/mcp/GitHubIssueManagementProvider.ts @@ -9,6 +9,7 @@ import type { GetIssueInput, GetPRInput, GetReviewCommentsInput, + GetReviewsInput, GetCommentInput, CreateCommentInput, UpdateCommentInput, @@ -25,6 +26,7 @@ import type { IssueResult, PRResult, ReviewCommentResult, + ReviewSubmissionResult, CommentDetailResult, CommentResult, DependenciesResult, @@ -433,7 +435,58 @@ export class GitHubIssueManagementProvider implements IssueManagementProvider { createdAt: comment.created_at, updatedAt: comment.updated_at ?? null, inReplyToId: comment.in_reply_to_id ? String(comment.in_reply_to_id) : null, - pullRequestReviewId: comment.pull_request_review_id, + pullRequestReviewId: comment.pull_request_review_id != null ? String(comment.pull_request_review_id) : null, + }) + } + + return results + } + + /** + * Fetch PR review submissions (the body text submitted via "Submit review") + * Uses gh api with --paginate to handle PRs with many reviews + */ + async getReviewSubmissions(input: GetReviewsInput): Promise { + const { number, repo } = input + + const prNumber = parseInt(number, 10) + if (isNaN(prNumber)) { + throw new Error(`Invalid GitHub PR number: ${number}. GitHub PR IDs must be numeric.`) + } + + interface GitHubReview { + id: number + body: string + state: string + user: GitHubAuthor | null + submitted_at: string + html_url: string + } + + const apiPath = repo + ? `repos/${repo}/pulls/${prNumber}/reviews` + : `repos/:owner/:repo/pulls/${prNumber}/reviews` + + const args = [ + 'api', + apiPath, + '--paginate', + '--jq', + '[.[] | {id: .id, body: .body, state: .state, user: .user, submitted_at: .submitted_at, html_url: .html_url}]', + ] + + const raw = await executeGhCommand(args) + + const results: ReviewSubmissionResult[] = [] + for (const review of raw) { + const processedBody = review.body ? await processMarkdownImages(review.body, 'github') : '' + results.push({ + id: String(review.id), + body: processedBody, + state: review.state, + author: normalizeAuthor(review.user), + submittedAt: review.submitted_at, + htmlUrl: review.html_url, }) } diff --git a/src/mcp/issue-management-server.ts b/src/mcp/issue-management-server.ts index dfeec960..00054057 100644 --- a/src/mcp/issue-management-server.ts +++ b/src/mcp/issue-management-server.ts @@ -32,6 +32,7 @@ import type { ReopenIssueInput, EditIssueInput, GetReviewCommentsInput, + GetReviewsInput, } from './types.js' // Module-level settings loaded at startup @@ -360,7 +361,7 @@ server.registerTool( createdAt: z.string().describe('Comment creation timestamp'), updatedAt: z.string().nullable().describe('Comment last updated timestamp'), inReplyToId: z.string().nullable().describe('ID of the comment this replies to'), - pullRequestReviewId: z.number().nullable().describe('The review this comment belongs to'), + pullRequestReviewId: z.string().nullable().describe('The review submission ID this comment belongs to (matches get_reviews id)'), }) ).describe('Inline review comments on the PR'), }, @@ -427,6 +428,102 @@ server.registerTool( } ) +// Register get_reviews tool (review submissions — the body text submitted via "Submit review") +server.registerTool( + 'get_reviews', + { + title: 'Get PR Review Submissions', + description: + 'Fetch review submissions on a pull request (the body text and verdict submitted via the "Submit review" flow). ' + + 'Returns reviews with state (APPROVED, CHANGES_REQUESTED, COMMENTED, DISMISSED), body text, author, and timestamp. ' + + 'These are distinct from inline review comments (use get_review_comments for those). ' + + 'Uses the configured VCS provider when available, falls back to GitHub.', + inputSchema: { + number: z.string().describe('The PR number'), + repo: z + .string() + .optional() + .describe( + 'Optional repository in "owner/repo" format or full GitHub URL. ' + + 'When not provided, uses the current repository.' + ), + }, + outputSchema: { + reviews: z.array( + z.object({ + id: z.string().describe('Review submission ID'), + body: z.string().describe('Review body text'), + state: z.string().describe('Review state: APPROVED, CHANGES_REQUESTED, COMMENTED, DISMISSED, PENDING'), + author: flexibleAuthorSchema.nullable().describe('Review author'), + submittedAt: z.string().describe('Review submission timestamp'), + htmlUrl: z.string().describe('URL to the review on the provider'), + }) + ).describe('Review submissions on the PR'), + }, + }, + async ({ number, repo }: GetReviewsInput) => { + console.error(`Fetching review submissions for PR ${number}${repo ? ` from ${repo}` : ''}`) + + const prNumber = parseInt(number, 10) + if (isNaN(prNumber)) { + throw new Error(`Invalid PR number: ${number}. PR numbers must be numeric.`) + } + + try { + if (vcsProvider?.getReviewSubmissions) { + if (repo) { + console.error(`VCS provider path does not support 'repo' override parameter, using configured repository`) + } + const reviewSubmissions = await vcsProvider.getReviewSubmissions(prNumber) + + const reviews = reviewSubmissions.map(r => ({ + id: r.id, + body: r.body, + state: r.state, + author: r.author ? { id: r.author.id, displayName: r.author.displayName } : null, + submittedAt: r.submittedAt, + htmlUrl: r.htmlUrl, + })) + + console.error(`Review submissions fetched successfully: ${reviews.length} reviews`) + + const result = { reviews } + return { + content: [ + { + type: 'text' as const, + text: JSON.stringify(result), + }, + ], + structuredContent: result as unknown as { [x: string]: unknown }, + } + } + + // Fall back to GitHub provider + const provider = new GitHubIssueManagementProvider() + const reviews = await provider.getReviewSubmissions({ number, repo }) + + console.error(`Review submissions fetched successfully: ${reviews.length} reviews`) + + const result = { reviews } + return { + content: [ + { + type: 'text' as const, + text: JSON.stringify(result), + }, + ], + structuredContent: result as unknown as { [x: string]: unknown }, + } + } catch (error) { + const errorMessage = + error instanceof Error ? error.message : 'Unknown error' + console.error(`Failed to fetch review submissions: ${errorMessage}`) + throw new Error(`Failed to fetch review submissions: ${errorMessage}`) + } + } +) + // Register get_comment tool server.registerTool( 'get_comment', diff --git a/src/mcp/types.ts b/src/mcp/types.ts index 80a54c2c..0755c623 100644 --- a/src/mcp/types.ts +++ b/src/mcp/types.ts @@ -45,6 +45,26 @@ export interface GetReviewCommentsInput { repo?: string | undefined // Optional repository in "owner/repo" format or full GitHub URL } +/** + * Input schema for getting PR review submissions (the body text submitted via "Submit review") + */ +export interface GetReviewsInput { + number: string // PR number + repo?: string | undefined // Optional repository in "owner/repo" format or full GitHub URL +} + +/** + * Output schema for a single PR review submission + */ +export interface ReviewSubmissionResult { + id: string + body: string + state: string // APPROVED, CHANGES_REQUESTED, COMMENTED, DISMISSED, PENDING + author: FlexibleAuthor | null + submittedAt: string + htmlUrl: string +} + /** * Output schema for a single PR review comment (inline code comment) */ @@ -58,7 +78,7 @@ export interface ReviewCommentResult { createdAt: string updatedAt: string | null inReplyToId: string | null // If this is a reply to another review comment - pullRequestReviewId: number | null // The review this comment belongs to + pullRequestReviewId: string | null // The review this comment belongs to } /** diff --git a/templates/prompts/pr-prompt.txt b/templates/prompts/pr-prompt.txt index e3065e6e..b84ab670 100644 --- a/templates/prompts/pr-prompt.txt +++ b/templates/prompts/pr-prompt.txt @@ -61,6 +61,7 @@ Use these Recap MCP tools: - **Read PR details** using `mcp__issue_management__get_pr` with `{ number: "{{PR_NUMBER}}", includeComments: true }` - **Read inline code review comments** using `mcp__issue_management__get_review_comments` with `{ number: "{{PR_NUMBER}}" }` +- **Read review submissions** (verdicts and body text from "Submit review") using `mcp__issue_management__get_reviews` with `{ number: "{{PR_NUMBER}}" }` - **Write comments** to PR #{{PR_NUMBER}} using `type: "pr"` When calling `mcp__issue_management__create_comment`: @@ -103,8 +104,18 @@ To read inline code review comments (comments on specific files and lines), use: This returns: - Inline comments on specific files and lines (path, line number, diff side) - Review comment threads and replies (inReplyToId) +- Each comment's `pullRequestReviewId` links it to a review submission - Optionally filter by review ID: `{ number: "{{PR_NUMBER}}", reviewId: "REVIEW_ID" }` +To read review submissions (verdicts and body text from the "Submit review" flow), use: +`mcp__issue_management__get_reviews` with `{ number: "{{PR_NUMBER}}" }` + +This returns: +- Review state (APPROVED, CHANGES_REQUESTED, COMMENTED, DISMISSED) +- Review body text (often contains rationale for approving/dismissing findings) +- Author and timestamp +- Each review's `id` matches the `pullRequestReviewId` on inline review comments + Also check if the branch name contains an issue number (like issue-123) and if so, read that issue for context: `mcp__issue_management__get_issue` with `{ number: "{{ISSUE_NUMBER}}", includeComments: true }` @@ -121,7 +132,18 @@ When working with libraries, frameworks, or APIs that you need documentation for ## Addressing PR Review Feedback -After reading the PR, check for unresolved inline review comments using `mcp__issue_management__get_review_comments`. If there are unresolved review threads: +After reading the PR, gather all feedback sources: +1. Fetch inline review comments: `mcp__issue_management__get_review_comments` with `{ number: "{{PR_NUMBER}}" }` +2. Fetch review submissions: `mcp__issue_management__get_reviews` with `{ number: "{{PR_NUMBER}}" }` + +**Cross-reference to identify truly unresolved feedback:** +- Each inline review comment has a `pullRequestReviewId` that links it to a review submission +- A review submission's `id` matches the `pullRequestReviewId` on its inline comments +- Check review submissions for responses that address or dismiss inline findings (e.g., a reviewer submitting a COMMENT or APPROVED review with body text explaining why a finding was dismissed or resolved) +- Also check inline comment reply threads (`inReplyToId`) for direct replies addressing findings +- Only treat feedback as **unresolved** if no subsequent review submission or inline reply addresses it + +If there are unresolved review threads: {{#if ONE_SHOT_MODE}} Evaluate unresolved review comments using the same severity framework as the code review agent, then address automatically where appropriate: @@ -133,7 +155,7 @@ Evaluate unresolved review comments using the same severity framework as the cod 2. **Summarize**: Post a PR comment summarizing all feedback, grouped by what was addressed vs deferred. For deferred items, explain why. 3. **Address must-fix items and low-effort nice-to-haves**: For each: a. Make the code change - b. Reply to the review thread explaining what was changed: + b. Reply to the specific inline comment thread explaining what was changed: ```bash gh api \ --method POST \ @@ -146,6 +168,7 @@ Evaluate unresolved review comments using the same severity framework as the cod } EOF ``` + Always reply to the specific inline comment thread (using `in_reply_to`) rather than posting a standalone review. This keeps responses threaded under the finding they address. 4. **Commit and push** changes after addressing comments 5. Proceed to code review (see "Code Review" section below) {{else}} @@ -161,7 +184,7 @@ Present a summary of unresolved review feedback to the user: - "Skip for now" — move on to other work 3. **For each comment being addressed**: a. Make the code change - b. Reply to the review thread explaining the fix (using `gh api` as shown above) + b. Reply to the specific inline comment thread explaining the fix (using `gh api` with `in_reply_to` as shown above). Always reply inline rather than posting a standalone review. 4. After addressing comments, offer to run code review {{/if}}