diff --git a/actions/setup/js/resolve_pr_review_thread.cjs b/actions/setup/js/resolve_pr_review_thread.cjs index ae508ee53c6..b629aec598c 100644 --- a/actions/setup/js/resolve_pr_review_thread.cjs +++ b/actions/setup/js/resolve_pr_review_thread.cjs @@ -24,8 +24,9 @@ const HANDLER_TYPE = "resolve_pull_request_review_thread"; * @param {string} threadId - Review thread node ID (e.g., 'PRRT_kwDOABCD...') * @returns {Promise< * | {status: "missing"} - * | {status: "thread", prNumber: number, repoNameWithOwner: string|null, isResolved: boolean} + * | {status: "thread", threadId: string, prNumber: number, repoNameWithOwner: string|null, isResolved: boolean} * | {status: "invalid_type", nodeType: string} + * | {status: "comment_without_thread"} * >} Thread lookup result */ async function getThreadPullRequestInfo(github, threadId) { @@ -34,6 +35,7 @@ async function getThreadPullRequestInfo(github, threadId) { node(id: $threadId) { __typename ... on PullRequestReviewThread { + id isResolved pullRequest { number @@ -42,6 +44,18 @@ async function getThreadPullRequestInfo(github, threadId) { } } } + ... on PullRequestReviewComment { + pullRequest { + number + repository { + name + nameWithOwner + owner { + login + } + } + } + } } } `; @@ -52,6 +66,9 @@ async function getThreadPullRequestInfo(github, threadId) { if (!threadNode) { return { status: "missing" }; } + if (threadNode.__typename === "PullRequestReviewComment") { + return findThreadInfoForReviewComment(github, threadId, threadNode); + } if (threadNode.__typename !== "PullRequestReviewThread") { return { status: "invalid_type", @@ -62,12 +79,130 @@ async function getThreadPullRequestInfo(github, threadId) { return { status: "thread", + threadId: threadNode.id, prNumber: pullRequest.number, repoNameWithOwner: pullRequest.repository?.nameWithOwner ?? null, isResolved: threadNode?.isResolved === true, }; } +/** + * Resolve a PullRequestReviewComment node ID to its containing review thread. + * @param {any} github - GitHub GraphQL instance + * @param {string} commentId - Pull request review comment node ID (e.g., 'PRRC_kwDOABCD...') + * @param {any} commentNode - PullRequestReviewComment node returned by the initial lookup + * @returns {Promise< + * | {status: "thread", threadId: string, prNumber: number, repoNameWithOwner: string|null, isResolved: boolean} + * | {status: "comment_without_thread"} + * >} + */ +async function findThreadInfoForReviewComment(github, commentId, commentNode) { + const pullRequest = commentNode.pullRequest; + const repository = pullRequest?.repository; + const prNumber = pullRequest?.number; + const repoOwner = repository?.owner?.login; + const repoName = repository?.name; + const repoNameWithOwner = repository?.nameWithOwner ?? (repoOwner && repoName ? `${repoOwner}/${repoName}` : null); + + if (!prNumber || !repoOwner || !repoName) { + return { status: "comment_without_thread" }; + } + + const threadListQuery = /* GraphQL */ ` + query ($owner: String!, $repo: String!, $number: Int!, $cursor: String) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $number) { + reviewThreads(first: 100, after: $cursor) { + pageInfo { + hasNextPage + endCursor + } + nodes { + id + isResolved + comments(first: 100) { + pageInfo { + hasNextPage + endCursor + } + nodes { + id + } + } + } + } + } + } + } + `; + + const threadCommentsQuery = /* GraphQL */ ` + query ($threadId: ID!, $cursor: String) { + node(id: $threadId) { + ... on PullRequestReviewThread { + comments(first: 100, after: $cursor) { + pageInfo { + hasNextPage + endCursor + } + nodes { + id + } + } + } + } + } + `; + + let threadCursor = null; + do { + const result = await github.graphql(threadListQuery, { + owner: repoOwner, + repo: repoName, + number: prNumber, + cursor: threadCursor, + }); + const reviewThreads = result?.repository?.pullRequest?.reviewThreads; + + for (const thread of reviewThreads?.nodes || []) { + // Check first page of comments for this thread + if ((thread.comments?.nodes || []).some(comment => comment?.id === commentId)) { + return { + status: "thread", + threadId: thread.id, + prNumber, + repoNameWithOwner, + isResolved: thread.isResolved === true, + }; + } + + // If there are more comment pages, paginate within this thread + let commentCursor = thread.comments?.pageInfo?.hasNextPage ? thread.comments.pageInfo.endCursor : null; + while (commentCursor) { + const commentResult = await github.graphql(threadCommentsQuery, { + threadId: thread.id, + cursor: commentCursor, + }); + const commentsPage = commentResult?.node?.comments; + if ((commentsPage?.nodes || []).some(comment => comment?.id === commentId)) { + return { + status: "thread", + threadId: thread.id, + prNumber, + repoNameWithOwner, + isResolved: thread.isResolved === true, + }; + } + commentCursor = commentsPage?.pageInfo?.hasNextPage ? commentsPage.pageInfo.endCursor : null; + } + } + + threadCursor = reviewThreads?.pageInfo?.hasNextPage ? reviewThreads.pageInfo.endCursor : null; + } while (threadCursor); + + return { status: "comment_without_thread" }; +} + /** * Resolve a pull request review thread using the GraphQL API. * @param {any} github - GitHub GraphQL instance @@ -199,6 +334,13 @@ async function main(config = {}) { }; } + if (threadInfo.status === "comment_without_thread") { + return { + success: false, + error: `Could not find a PullRequestReviewThread containing review comment ${threadId}`, + }; + } + if (threadInfo.status !== "thread") { return { success: false, @@ -206,7 +348,10 @@ async function main(config = {}) { }; } - const { prNumber: threadPRNumber, repoNameWithOwner: threadRepo, isResolved } = threadInfo; + const { threadId: resolvedThreadId, prNumber: threadPRNumber, repoNameWithOwner: threadRepo, isResolved } = threadInfo; + if (resolvedThreadId !== threadId) { + core.info(`Resolved review comment ${threadId} to review thread ${resolvedThreadId}`); + } // When the user explicitly configured target-repo or allowed-repos, validate the thread's // repository using validateTargetRepo (supports wildcards like "*", "org/*"). @@ -214,15 +359,15 @@ async function main(config = {}) { if (hasExplicitTargetConfig) { // Cross-repo mode: validate thread repo against configured repos (fail closed if missing) if (!threadRepo) { - core.warning(`Could not determine repository for thread ${threadId}`); + core.warning(`Could not determine repository for thread ${resolvedThreadId}`); return { success: false, - error: `Could not determine the repository for thread ${threadId}`, + error: `Could not determine the repository for thread ${resolvedThreadId}`, }; } const repoValidation = validateTargetRepo(threadRepo, defaultTargetRepo, allowedRepos); if (!repoValidation.valid) { - core.warning(`Thread ${threadId} belongs to repo ${threadRepo}, which is not in the allowed repos`); + core.warning(`Thread ${resolvedThreadId} belongs to repo ${threadRepo}, which is not in the allowed repos`); return { success: false, error: repoValidation.error, @@ -239,7 +384,7 @@ async function main(config = {}) { }; } if (threadPRNumber !== triggeringPRNumber) { - core.warning(`Thread ${threadId} belongs to PR #${threadPRNumber}, not triggering PR #${triggeringPRNumber}`); + core.warning(`Thread ${resolvedThreadId} belongs to PR #${threadPRNumber}, not triggering PR #${triggeringPRNumber}`); return { success: false, error: `Thread belongs to PR #${threadPRNumber}, but only threads on the triggering PR #${triggeringPRNumber} can be resolved`, @@ -256,7 +401,7 @@ async function main(config = {}) { }; } if (threadPRNumber !== targetPRNumber) { - core.warning(`Thread ${threadId} belongs to PR #${threadPRNumber}, not target PR #${targetPRNumber}`); + core.warning(`Thread ${resolvedThreadId} belongs to PR #${threadPRNumber}, not target PR #${targetPRNumber}`); return { success: false, error: `Thread belongs to PR #${threadPRNumber}, but target is PR #${targetPRNumber}`, @@ -268,16 +413,16 @@ async function main(config = {}) { // Default (legacy) mode: always validate thread repo against defaultTargetRepo to stay // least-privilege, even when there is no triggering PR (e.g. schedule/workflow_dispatch). if (!threadRepo) { - core.warning(`Unable to determine repository for review thread ${threadId}; refusing to resolve in legacy mode`); + core.warning(`Unable to determine repository for review thread ${resolvedThreadId}; refusing to resolve in legacy mode`); return { success: false, - error: `Unable to determine repository for review thread ${threadId}`, + error: `Unable to determine repository for review thread ${resolvedThreadId}`, }; } const legacyRepoValidation = validateTargetRepo(threadRepo, defaultTargetRepo, allowedRepos); if (!legacyRepoValidation.valid) { - core.warning(`Thread ${threadId} repository ${threadRepo} is not allowed in legacy mode`); + core.warning(`Thread ${resolvedThreadId} repository ${threadRepo} is not allowed in legacy mode`); return { success: false, error: legacyRepoValidation.error || `Repository ${threadRepo} is not allowed for this handler`, @@ -288,9 +433,9 @@ async function main(config = {}) { if (!triggeringPRNumber) { // No triggering PR (e.g. schedule/workflow_dispatch trigger), but the thread has been // resolved to a specific allowed repository via the API — allow the resolution to proceed - core.info(`No triggering PR context; resolving thread ${threadId} via explicit thread_id (PR #${threadPRNumber} in ${threadRepo})`); + core.info(`No triggering PR context; resolving thread ${resolvedThreadId} via explicit thread_id (PR #${threadPRNumber} in ${threadRepo})`); } else if (threadPRNumber !== triggeringPRNumber) { - core.warning(`Thread ${threadId} belongs to PR #${threadPRNumber}, not triggering PR #${triggeringPRNumber}`); + core.warning(`Thread ${resolvedThreadId} belongs to PR #${threadPRNumber}, not triggering PR #${triggeringPRNumber}`); return { success: false, error: `Thread belongs to PR #${threadPRNumber}, but only threads on the triggering PR #${triggeringPRNumber} can be resolved`, @@ -298,7 +443,7 @@ async function main(config = {}) { } } - core.info(`Resolving review thread: ${threadId} (PR #${threadPRNumber}${threadRepo ? " in " + threadRepo : ""})`); + core.info(`Resolving review thread: ${resolvedThreadId} (PR #${threadPRNumber}${threadRepo ? " in " + threadRepo : ""})`); // Apply required-labels/required-title-prefix filter const [threadOwner, threadRepoName] = (threadRepo || `${context.repo.owner}/${context.repo.repo}`).split("/"); @@ -307,10 +452,10 @@ async function main(config = {}) { if (filterResult) return filterResult; if (isResolved) { - core.info(`Review thread ${threadId} is already resolved; skipping`); + core.info(`Review thread ${resolvedThreadId} is already resolved; skipping`); return { success: true, - thread_id: threadId, + thread_id: resolvedThreadId, is_resolved: true, skipped: true, }; @@ -318,12 +463,12 @@ async function main(config = {}) { // If in staged mode, preview without executing if (isStaged) { - logStagedPreviewInfo(`Would resolve review thread ${threadId}`); + logStagedPreviewInfo(`Would resolve review thread ${resolvedThreadId}`); return { success: true, staged: true, previewInfo: { - thread_id: threadId, + thread_id: resolvedThreadId, pr_number: threadPRNumber, }, }; @@ -331,11 +476,11 @@ async function main(config = {}) { let resolveResult; try { - resolveResult = await resolveReviewThreadAPI(githubClient, threadId); + resolveResult = await resolveReviewThreadAPI(githubClient, resolvedThreadId); } catch (error) { if (isIntegrationAccessError(error)) { const warningMessage = - `Skipping resolve_pull_request_review_thread for ${threadId}: configuration mismatch ` + + `Skipping resolve_pull_request_review_thread for ${resolvedThreadId}: configuration mismatch ` + `(GitHub integration token cannot resolve this review thread: Resource not accessible by integration). ` + `Use safe-outputs.resolve-pull-request-review-thread.github-token with a token that can resolve review threads.`; core.warning(warningMessage); @@ -349,17 +494,17 @@ async function main(config = {}) { } if (resolveResult.isResolved) { - core.info(`Successfully resolved review thread: ${threadId}`); + core.info(`Successfully resolved review thread: ${resolvedThreadId}`); return { success: true, - thread_id: threadId, + thread_id: resolvedThreadId, is_resolved: true, }; } else { - core.error(`Failed to resolve review thread: ${threadId}`); + core.error(`Failed to resolve review thread: ${resolvedThreadId}`); return { success: false, - error: `Failed to resolve review thread: ${threadId}`, + error: `Failed to resolve review thread: ${resolvedThreadId}`, }; } } catch (error) { diff --git a/actions/setup/js/resolve_pr_review_thread.test.cjs b/actions/setup/js/resolve_pr_review_thread.test.cjs index 78b4d560f29..4cd8294603a 100644 --- a/actions/setup/js/resolve_pr_review_thread.test.cjs +++ b/actions/setup/js/resolve_pr_review_thread.test.cjs @@ -39,14 +39,14 @@ global.context = mockContext; * @param {number} lookupPRNumber - PR number returned by the thread lookup query * @param {string} [lookupRepo] - Repository nameWithOwner returned by the lookup query (default: "test-owner/test-repo") */ -function mockGraphqlForThread(lookupPRNumber, lookupRepo = "test-owner/test-repo") { +function mockGraphqlForThread(lookupPRNumber, lookupRepo = "test-owner/test-repo", lookupThreadId = "PRRT_kwDOABCD123456") { mockGraphql.mockImplementation(query => { if (query.includes("resolveReviewThread")) { // Mutation return Promise.resolve({ resolveReviewThread: { thread: { - id: "PRRT_kwDOABCD123456", + id: lookupThreadId, isResolved: true, }, }, @@ -56,6 +56,7 @@ function mockGraphqlForThread(lookupPRNumber, lookupRepo = "test-owner/test-repo return Promise.resolve({ node: { __typename: "PullRequestReviewThread", + id: lookupThreadId, isResolved: false, pullRequest: { number: lookupPRNumber, @@ -153,10 +154,196 @@ describe("resolve_pr_review_thread", () => { expect(mockGraphql).toHaveBeenCalledTimes(1); }); - it("should reject non-thread node IDs instead of treating them as resolved", async () => { + it("should resolve a review comment node ID by finding its parent thread", async () => { + mockGraphql.mockImplementation(query => { + if (query.includes("resolveReviewThread")) { + return Promise.resolve({ + resolveReviewThread: { + thread: { + id: "PRRT_kwDOThread123", + isResolved: true, + }, + }, + }); + } + if (query.includes("reviewThreads")) { + return Promise.resolve({ + repository: { + pullRequest: { + reviewThreads: { + pageInfo: { hasNextPage: false, endCursor: null }, + nodes: [ + { + id: "PRRT_kwDOThread123", + isResolved: false, + comments: { + pageInfo: { hasNextPage: false, endCursor: null }, + nodes: [{ id: "PRRC_kwDOComment123" }], + }, + }, + ], + }, + }, + }, + }); + } + return Promise.resolve({ + node: { + __typename: "PullRequestReviewComment", + pullRequest: { + number: 42, + repository: { + name: "test-repo", + nameWithOwner: "test-owner/test-repo", + owner: { login: "test-owner" }, + }, + }, + }, + }); + }); + + const { main } = require("./resolve_pr_review_thread.cjs"); + const freshHandler = await main({ max: 10 }); + + const result = await freshHandler({ + type: "resolve_pull_request_review_thread", + thread_id: "PRRC_kwDOComment123", + }); + + expect(result.success).toBe(true); + expect(result.thread_id).toBe("PRRT_kwDOThread123"); + expect(result.is_resolved).toBe(true); + expect(mockGraphql).toHaveBeenCalledTimes(3); + expect(mockGraphql).toHaveBeenCalledWith(expect.stringContaining("resolveReviewThread"), expect.objectContaining({ threadId: "PRRT_kwDOThread123" })); + }); + + it("should return comment_without_thread when the PRRC_ ID is not found in any review thread", async () => { + mockGraphql.mockImplementation(query => { + if (query.includes("reviewThreads")) { + return Promise.resolve({ + repository: { + pullRequest: { + reviewThreads: { + pageInfo: { hasNextPage: false, endCursor: null }, + nodes: [ + { + id: "PRRT_kwDOThread999", + isResolved: false, + comments: { + pageInfo: { hasNextPage: false, endCursor: null }, + nodes: [{ id: "PRRC_kwDOOtherComment" }], + }, + }, + ], + }, + }, + }, + }); + } + return Promise.resolve({ + node: { + __typename: "PullRequestReviewComment", + pullRequest: { + number: 42, + repository: { + name: "test-repo", + nameWithOwner: "test-owner/test-repo", + owner: { login: "test-owner" }, + }, + }, + }, + }); + }); + + const { main } = require("./resolve_pr_review_thread.cjs"); + const freshHandler = await main({ max: 10 }); + + const result = await freshHandler({ + type: "resolve_pull_request_review_thread", + thread_id: "PRRC_kwDOComment123", + }); + + expect(result.success).toBe(false); + expect(result.error).toContain("PRRC_kwDOComment123"); + }); + + it("should resolve a review comment found on a later comments page within a thread", async () => { + mockGraphql.mockImplementation(query => { + if (query.includes("resolveReviewThread")) { + return Promise.resolve({ + resolveReviewThread: { + thread: { + id: "PRRT_kwDOThread123", + isResolved: true, + }, + }, + }); + } + if (query.includes("threadId") && query.includes("comments(first: 100, after:")) { + // Second comments page for the thread — contains the matching comment + return Promise.resolve({ + node: { + comments: { + pageInfo: { hasNextPage: false, endCursor: null }, + nodes: [{ id: "PRRC_kwDOComment123" }], + }, + }, + }); + } + if (query.includes("reviewThreads")) { + return Promise.resolve({ + repository: { + pullRequest: { + reviewThreads: { + pageInfo: { hasNextPage: false, endCursor: null }, + nodes: [ + { + id: "PRRT_kwDOThread123", + isResolved: false, + comments: { + pageInfo: { hasNextPage: true, endCursor: "cursor-page2" }, + nodes: [{ id: "PRRC_kwDOOtherComment" }], + }, + }, + ], + }, + }, + }, + }); + } + return Promise.resolve({ + node: { + __typename: "PullRequestReviewComment", + pullRequest: { + number: 42, + repository: { + name: "test-repo", + nameWithOwner: "test-owner/test-repo", + owner: { login: "test-owner" }, + }, + }, + }, + }); + }); + + const { main } = require("./resolve_pr_review_thread.cjs"); + const freshHandler = await main({ max: 10 }); + + const result = await freshHandler({ + type: "resolve_pull_request_review_thread", + thread_id: "PRRC_kwDOComment123", + }); + + expect(result.success).toBe(true); + expect(result.thread_id).toBe("PRRT_kwDOThread123"); + expect(result.is_resolved).toBe(true); + expect(mockGraphql).toHaveBeenCalledWith(expect.stringContaining("resolveReviewThread"), expect.objectContaining({ threadId: "PRRT_kwDOThread123" })); + }); + + it("should still reject non-thread and non-comment node IDs", async () => { mockGraphql.mockResolvedValue({ node: { - __typename: "PullRequestReviewComment", + __typename: "Issue", }, }); @@ -165,12 +352,12 @@ describe("resolve_pr_review_thread", () => { const result = await freshHandler({ type: "resolve_pull_request_review_thread", - thread_id: "PRRC_kwDOComment123", + thread_id: "I_kwDOIssue123", }); expect(result.success).toBe(false); expect(result.error).toContain("PullRequestReviewThread"); - expect(result.error).toContain("PullRequestReviewComment"); + expect(result.error).toContain("Issue"); }); it("should still reject already resolved threads from a different PR", async () => { @@ -259,7 +446,7 @@ describe("resolve_pr_review_thread", () => { // Simulate a schedule-triggered workflow (no pull_request in payload) global.context.payload = {}; - mockGraphqlForThread(77); + mockGraphqlForThread(77, "test-owner/test-repo", "PRRT_kwDOSchedule77"); const { main } = require("./resolve_pr_review_thread.cjs"); const freshHandler = await main({ max: 10 }); @@ -521,7 +708,7 @@ describe("resolve_pr_review_thread - cross-repo support", () => { }); it("should allow resolving a thread in target-repo when configured", async () => { - mockGraphqlForThread(10, "other-owner/other-repo"); + mockGraphqlForThread(10, "other-owner/other-repo", "PRRT_kwDOCrossRepo"); const { main } = require("./resolve_pr_review_thread.cjs"); const freshHandler = await main({