diff --git a/AGENTS.md b/AGENTS.md index 981738e..380b262 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -15,7 +15,7 @@ action set and always prefixed with a fixed disclaimer. Two transports: stdio ## Architecture -- **`src/github.ts`**: `GitHubReviewClient` class. The ONLY file that touches GitHub APIs. Uses GraphQL for resolved review-thread context and mutations, and REST for review summaries and general PR comments. Review-thread comments (GraphQL `reactionGroups`) and conversation comments (REST `reactions`) expose a normalized `reactions` map (content -> count, e.g. `THUMBS_DOWN`) so agents can see downvotes. Thread `isResolved`/`resolvedBy` are also returned. Safety boundary: comment/thread mutations exclude the `event` field, a post-creation tripwire verifies `PENDING` state (its error message tells the agent to stop and alert the human, since many MCP clients do not surface tool errors), and write operations are limited to the authenticated user's review. Submission is gated: `submitReview` (the only place `submitPullRequestReview` is called) refuses any action not in the `allowSubmit` set passed to the constructor, and always prefixes the review body with the fixed `submitBody` (an optional caller `additionalBody` is appended below it, never replacing it). Thread resolution is gated too: `resolveReviewThread` runs only when `allowResolve` is set AND the thread's first comment is authored by the authenticated user (it refuses others' threads), so a bot can tidy up its own now-fixed findings but not close anyone else's conversations. PR scoping is enforced here too: when the client is constructed with a `scope` (owner/repo/PR), every public method calls `assertInScope` and refuses input targeting a different PR/repo, and `resolveReviewThread` verifies the thread's PR matches the scope. This is the authoritative boundary. The server-side schema change is convenience on top of it. +- **`src/github.ts`**: `GitHubReviewClient` class. The ONLY file that touches GitHub APIs. Uses GraphQL for resolved review-thread context and mutations, and REST for review summaries and general PR comments. Review-thread comments (GraphQL `reactionGroups`) and conversation comments (REST `reactions`) expose a normalized `reactions` map (content -> count, e.g. `THUMBS_DOWN`) so agents can see downvotes. Thread `isResolved`/`resolvedBy` are also returned. Review comments, review summaries and conversation comments all carry their GitHub permalink as `url`, so an agent can link an earlier discussion (a thread's permalink is its first comment's `url`). The field is named `url` everywhere even though it comes from GraphQL `url` for review comments and REST `html_url` for the REST-sourced ones, since the output shape should be more consistent than the GitHub API is. A pending review comment already carries its final permalink, which starts resolving when the review is submitted, so an agent can cross-link its own findings while drafting. The `ReviewComment.url` doc comment and the pending tool descriptions say so, and the integration suite asserts that a comment's permalink survives submission unchanged. Safety boundary: comment/thread mutations exclude the `event` field, a post-creation tripwire verifies `PENDING` state (its error message tells the agent to stop and alert the human, since many MCP clients do not surface tool errors), and write operations are limited to the authenticated user's review. Submission is gated: `submitReview` (the only place `submitPullRequestReview` is called) refuses any action not in the `allowSubmit` set passed to the constructor, and always prefixes the review body with the fixed `submitBody` (an optional caller `additionalBody` is appended below it, never replacing it). Thread resolution is gated too: `resolveReviewThread` runs only when `allowResolve` is set AND the thread's first comment is authored by the authenticated user (it refuses others' threads), so a bot can tidy up its own now-fixed findings but not close anyone else's conversations. PR scoping is enforced here too: when the client is constructed with a `scope` (owner/repo/PR), every public method calls `assertInScope` and refuses input targeting a different PR/repo, and `resolveReviewThread` verifies the thread's PR matches the scope. This is the authoritative boundary. The server-side schema change is convenience on top of it. - **`src/server.ts`**: `createMcpServer(client)` factory. Registers `get_pr_review_context`, `list_pending_review`, `add_review_comments`, `modify_review_comment`, and `delete_pending_review` with Zod schemas. Additionally registers `submit` **only when** `client.allowedSubmitActions` is non-empty (its `action` enum is restricted to that set), and `resolve_review_thread` **only when** `client.resolveEnabled` (started with `--allow-resolve`). When `client.scopedPullRequest` is set, the PR tools omit their `owner`/`repo`/`pull_number` arguments and act on the scoped PR implicitly. Shared by both transports. - **`src/stdio.ts`**: Stdio transport entry point. Connects the MCP server to stdin/stdout for IDE-managed lifetime (Theia, VS Code). - **`src/http.ts`**: HTTP transport. A plain `node:http` server exposing stateless Streamable HTTP at `/mcp` (POST only, GET/DELETE return 405, other paths 404). No web framework: the MCP transport parses the request body and enforces the Host header itself. Receives `{ port, host }` from the entry point and binds that address (default `127.0.0.1`). Validates the Host header of incoming requests (DNS rebinding protection): loopback aliases only for a loopback bind, plus the bind address and the container-runtime host names (`host.docker.internal`, `host.containers.internal`) for a non-loopback bind. diff --git a/README.md b/README.md index c0f668f..448d524 100644 --- a/README.md +++ b/README.md @@ -62,8 +62,8 @@ npm link # creates a global symlink to the binary | Tool | Availability | Description | | ----------------------- | ----------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `get_pr_review_context` | always | Get PR author/message, submitted review summaries, inline review threads with resolved state and reactions, and general PR comments | -| `list_pending_review` | always | List the authenticated user's pending draft review, including all current pending review comments | +| `get_pr_review_context` | always | Get PR author/message, submitted review summaries, inline review threads with resolved state, reactions and permalinks, and general PR comments | +| `list_pending_review` | always | List the authenticated user's pending draft review, including all current pending review comments and their permalinks | | `add_review_comments` | always | Add one or more comments to the authenticated user's pending review, creating the pending review if needed | | `modify_review_comment` | always | Update or delete one comment from the authenticated user's pending review | | `delete_pending_review` | always | Delete the authenticated user's pending review and all its comments | diff --git a/src/github.ts b/src/github.ts index 7f58d15..3da6626 100644 --- a/src/github.ts +++ b/src/github.ts @@ -153,7 +153,8 @@ export interface ReviewSummary { body: string; submittedAt: string | null; commitId: string | null; - htmlUrl: string | null; + /** Permalink to the submitted review. */ + url: string | null; authorAssociation: string; } @@ -165,6 +166,15 @@ export interface ReviewComment { line: number | null; createdAt: string; updatedAt: string; + /** + * Permalink to the comment. GraphQL types this as non-null, but it stays + * nullable here so a response that omits it degrades to null instead of + * failing the whole call. A comment in a pending review already carries its + * final permalink, which starts resolving once the review is submitted. Since + * all comments of a review go live at the same moment, one pending comment can + * link another. + */ + url: string | null; /** Reaction content -> count, only for counts > 0 (e.g. THUMBS_DOWN). */ reactions?: Record; pullRequestReview?: { @@ -200,7 +210,8 @@ export interface PullRequestConversationComment { body: string; createdAt: string; updatedAt: string; - htmlUrl: string; + /** Permalink to the comment. */ + url: string; authorAssociation: string; /** Reaction content -> count, only for counts > 0 (e.g. THUMBS_DOWN). */ reactions?: Record; @@ -404,6 +415,7 @@ export class GitHubReviewClient { pageInfo { hasNextPage endCursor } nodes { id + url body path line @@ -520,6 +532,7 @@ export class GitHubReviewClient { pageInfo { hasNextPage endCursor } nodes { id + url body path line @@ -585,7 +598,7 @@ export class GitHubReviewClient { body: review.body ?? "", submittedAt: review.submitted_at ?? null, commitId: review.commit_id ?? null, - htmlUrl: review.html_url ?? null, + url: review.html_url ?? null, authorAssociation: review.author_association, }); } @@ -622,7 +635,7 @@ export class GitHubReviewClient { body: comment.body ?? "", createdAt: comment.created_at, updatedAt: comment.updated_at, - htmlUrl: comment.html_url, + url: comment.html_url, authorAssociation: comment.author_association, reactions: this.mapRestReactions((comment as any).reactions), }); @@ -731,6 +744,7 @@ export class GitHubReviewClient { pageInfo { hasNextPage endCursor } nodes { id + url body path line @@ -865,6 +879,7 @@ export class GitHubReviewClient { comments(first: 1) { nodes { id + url body path line @@ -963,6 +978,7 @@ export class GitHubReviewClient { updatePullRequestReviewComment(input: $input) { pullRequestReviewComment { id + url body path line @@ -1014,6 +1030,7 @@ export class GitHubReviewClient { } pullRequestReviewComment { id + url body path line @@ -1282,6 +1299,7 @@ export class GitHubReviewClient { body: comment.body ?? "", path: comment.path ?? "", line: comment.line ?? null, + url: comment.url ?? null, createdAt: comment.createdAt ?? "", updatedAt: comment.updatedAt ?? "", reactions: this.mapReactionGroups(comment), diff --git a/src/server.ts b/src/server.ts index dad484f..de90c36 100644 --- a/src/server.ts +++ b/src/server.ts @@ -93,6 +93,12 @@ export function createMcpServer(client: GitHubReviewClient): McpServer { const scopeNote = scope ? ` This server is pinned to ${scope.owner}/${scope.repo}#${scope.pullNumber}. The PR is implicit (no owner/repo/pull_number arguments).` : ""; + // A pending comment's permalink is already its final one, so cross-linking + // findings inside a draft review works. Every tool that hands back pending + // comments says so, since the link does not resolve while drafting and an + // agent would otherwise assume it is broken. + const pendingPermalinkNote = + " Each pending comment already carries its final `url` permalink. It starts resolving once the review is submitted, and because all comments of a review go live together, one pending comment may link another."; const target = (args: Record): PullRequestInput => scope ?? { owner: args.owner as string, @@ -103,7 +109,8 @@ export function createMcpServer(client: GitHubReviewClient): McpServer { // -- get_pr_review_context ------------------------------------------------- server.tool( "get_pr_review_context", - "Get the pull request author/message plus all submitted PR discussion context: review summaries, inline review threads with resolved state, and general PR comments." + + "Get the pull request author/message plus all submitted PR discussion context: review summaries, inline review threads with resolved state and reactions, and general PR comments. " + + "Every review, review comment and PR comment carries its GitHub permalink as `url`, so you can link an earlier discussion when you refer to one (a thread's permalink is its first comment's `url`)." + scopeNote, prFields, async (args) => { @@ -121,6 +128,7 @@ export function createMcpServer(client: GitHubReviewClient): McpServer { server.tool( "list_pending_review", "List the authenticated user's current pending (draft) review on a pull request, including all current pending review comments. Returns null if there is no pending review." + + pendingPermalinkNote + scopeNote, prFields, async (args) => { @@ -138,6 +146,7 @@ export function createMcpServer(client: GitHubReviewClient): McpServer { server.tool( "add_review_comments", "Add one or more comments to the authenticated user's pending review. Creates the pending review if it does not already exist. The review is NOT submitted." + + pendingPermalinkNote + scopeNote, { ...prFields, ...AddReviewCommentsFields }, async (args) => { @@ -159,6 +168,7 @@ export function createMcpServer(client: GitHubReviewClient): McpServer { server.tool( "modify_review_comment", "Update or delete one comment from the authenticated user's pending review. This cannot modify submitted review comments." + + pendingPermalinkNote + scopeNote, { ...prFields, ...ModifyReviewCommentFields }, async (args) => { diff --git a/test/github.test.ts b/test/github.test.ts index 246511d..cf90003 100644 --- a/test/github.test.ts +++ b/test/github.test.ts @@ -151,6 +151,160 @@ describe("submitReview safety gate", () => { }); }); +describe("permalinks", () => { + function contextGql(commentNodes: unknown[]) { + return gqlBySubstring({ + "reviewThreads(first: 100": { + repository: { + pullRequest: { + id: "PR_NODE", + number: 1, + title: "t", + body: "b", + author: { login: "alice" }, + url: "https://github.com/octo/hello/pull/1", + state: "OPEN", + isDraft: false, + createdAt: "2026-01-01T00:00:00Z", + updatedAt: "2026-01-02T00:00:00Z", + baseRefName: "main", + headRefName: "feature", + reviewThreads: { + pageInfo: { hasNextPage: false, endCursor: null }, + nodes: [ + { + id: "T_1", + path: "a.ts", + line: 3, + diffSide: "RIGHT", + subjectType: "LINE", + isResolved: false, + isOutdated: false, + isCollapsed: false, + comments: { + totalCount: commentNodes.length, + pageInfo: { hasNextPage: false, endCursor: null }, + nodes: commentNodes, + }, + }, + ], + }, + }, + }, + }, + }); + } + + const commentNode = (id: string, url: string, reviewState = "COMMENTED") => ({ + id, + url, + body: `body of ${id}`, + path: "a.ts", + line: 3, + createdAt: "2026-01-01T00:00:00Z", + updatedAt: "2026-01-01T00:00:00Z", + author: { login: "bot" }, + pullRequestReview: { + id: "REV_1", + databaseId: 10, + state: reviewState, + author: { login: "bot" }, + }, + }); + const submitted = (id: string, url: string) => commentNode(id, url); + const draft = (id: string, url: string) => commentNode(id, url, "PENDING"); + + const DISCUSSION_URL = "https://github.com/octo/hello/pull/1#discussion_r1"; + + it("exposes each thread comment's permalink", async () => { + const { client } = makeClient( + {}, + { + gql: contextGql([ + submitted("C_1", DISCUSSION_URL), + submitted("C_2", "https://github.com/octo/hello/pull/1#discussion_r2"), + ]), + }, + ); + + const context = await client.getPullRequestReviewContext(PR); + expect(context.threads).toHaveLength(1); + expect(context.threads[0].comments.map((comment) => comment.url)).toEqual([ + DISCUSSION_URL, + "https://github.com/octo/hello/pull/1#discussion_r2", + ]); + }); + + // The pending path is the one an agent uses to quote its own drafts, and the + // `?? null` fallback in the mapper would let a dropped `url` selection + // degrade silently, so it gets its own assertion. + it("exposes each pending review comment's permalink", async () => { + const gql = gqlBySubstring({ + "node(id: $reviewId)": { + node: { + id: "REV_1", + databaseId: 10, + state: "PENDING", + body: "", + createdAt: "2026-01-01T00:00:00Z", + author: { login: "bot" }, + comments: { + totalCount: 1, + pageInfo: { hasNextPage: false, endCursor: null }, + nodes: [draft("C_1", DISCUSSION_URL)], + }, + }, + }, + }); + const { client } = makeClient( + {}, + { gql, octokit: makeOctokitStub("bot", [PENDING_REVIEW_REST]) }, + ); + + const review = await client.listPendingReview(PR); + expect(review?.comments.map((comment) => comment.url)).toEqual([DISCUSSION_URL]); + }); + + // The REST-sourced fields spell the permalink `html_url`, but the output shape + // normalizes every one of them to `url`. + it("exposes review summary and conversation comment permalinks as url", async () => { + const REVIEW_URL = "https://github.com/octo/hello/pull/1#pullrequestreview-10"; + const PR_COMMENT_URL = "https://github.com/octo/hello/pull/1#issuecomment-20"; + const octokit = makeOctokitStub("bot", [ + { + state: "COMMENTED", + node_id: "REV_1", + id: 10, + user: { login: "alice" }, + body: "looks good", + submitted_at: "2026-01-01T00:00:00Z", + commit_id: "abc123", + html_url: REVIEW_URL, + author_association: "MEMBER", + }, + ]); + octokit.issues.listComments.mockResolvedValue({ + data: [ + { + node_id: "IC_1", + id: 20, + user: { login: "alice" }, + body: "ping", + created_at: "2026-01-01T00:00:00Z", + updated_at: "2026-01-01T00:00:00Z", + html_url: PR_COMMENT_URL, + author_association: "MEMBER", + }, + ], + }); + const { client } = makeClient({}, { gql: contextGql([]), octokit }); + + const context = await client.getPullRequestReviewContext(PR); + expect(context.reviews.map((review) => review.url)).toEqual([REVIEW_URL]); + expect(context.prComments.map((comment) => comment.url)).toEqual([PR_COMMENT_URL]); + }); +}); + describe("deletePendingReview", () => { it("reports an explicit deletion flag, since GitHub returns the deleted node still as PENDING", async () => { const gql = gqlBySubstring({ diff --git a/test/integration/review-guard.itest.ts b/test/integration/review-guard.itest.ts index a933bd4..3032328 100644 --- a/test/integration/review-guard.itest.ts +++ b/test/integration/review-guard.itest.ts @@ -196,6 +196,8 @@ let scopedTools: Client; let draftComments: ReviewComment[]; let ownThreadId: string; +/** Permalink the finding carried while it was still a draft. */ +let draftFindingUrl: string | null; function target(): { owner: string; repo: string; pull_number: number } { return { owner: config.owner, repo: config.repo, pull_number: pullNumber }; @@ -396,6 +398,13 @@ describe("ReviewGuard against a real repository", () => { const { data: seenByAuthor } = await authorOctokit.rest.pulls.listReviews(target()); expect(seenByAuthor.filter((review) => review.user?.login === reviewerLogin)).toHaveLength(0); + // The draft is invisible at comment level too, not just as a review. + const { data: commentsSeenByAuthor } = + await authorOctokit.rest.pulls.listReviewComments(target()); + expect(commentsSeenByAuthor.filter((comment) => comment.body === DRAFT_LINE_BODY)).toHaveLength( + 0, + ); + const context = await callTool( pendingTools, "get_pr_review_context", @@ -412,6 +421,12 @@ describe("ReviewGuard against a real repository", () => { expect(review.comments.map((comment) => comment.body).sort()).toEqual( [DRAFT_FILE_BODY, DRAFT_LINE_BODY].sort(), ); + // A draft comment already carries a permalink, so an agent can cross-link + // its own findings while drafting. That the link survives submission is + // asserted further down. + for (const comment of review.comments) { + expect(comment.url).toContain(`/pull/${pullNumber}#discussion_r`); + } }); it("modify_review_comment updates a draft comment", async () => { @@ -460,10 +475,13 @@ describe("ReviewGuard against a real repository", () => { }); it("submit posts the review with the fixed disclaimer prefix", async () => { - await callTool(submitTools, "add_review_comments", { + const draft = await callTool(submitTools, "add_review_comments", { ...target(), comments: [{ path: filePath, body: FINDING_BODY, line: 3 }], }); + draftFindingUrl = draft.addedComments[0].url; + expect(draftFindingUrl).toContain(`/pull/${pullNumber}#discussion_r`); + const submitted = await callTool<{ id: string; state: string }>(submitTools, "submit", { ...target(), action: "comment", @@ -510,6 +528,10 @@ describe("ReviewGuard against a real repository", () => { expect(thread!.comments[0].author).toBe(reviewerLogin); expect(thread!.isResolved).toBe(false); expect(thread!.comments[0].reactions?.THUMBS_DOWN).toBe(1); + expect(thread!.comments[0].url).toContain(`/pull/${pullNumber}#discussion_r`); + // The permalink handed out while the comment was a draft is the one it + // keeps once submitted, which is what makes citing a pending comment safe. + expect(thread!.comments[0].url).toBe(draftFindingUrl); return thread!.id; }); });