diff --git a/actions/setup/js/resolve_pr_review_thread.cjs b/actions/setup/js/resolve_pr_review_thread.cjs index 870eed6370..f4013c7612 100644 --- a/actions/setup/js/resolve_pr_review_thread.cjs +++ b/actions/setup/js/resolve_pr_review_thread.cjs @@ -9,6 +9,7 @@ const { getErrorMessage } = require("./error_helpers.cjs"); const { getPRNumber } = require("./update_context_helpers.cjs"); const { logStagedPreviewInfo } = require("./staged_preview.cjs"); const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs"); +const { resolveTargetRepoConfig, validateTargetRepo } = require("./repo_helpers.cjs"); /** * Type constant for handler identification @@ -16,19 +17,22 @@ const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs"); const HANDLER_TYPE = "resolve_pull_request_review_thread"; /** - * Look up a review thread's parent PR number via the GraphQL API. - * Used to validate the thread belongs to the triggering PR before resolving. + * Look up a review thread's parent PR number and repository via the GraphQL API. + * Used to validate the thread before resolving. * @param {any} github - GitHub GraphQL instance * @param {string} threadId - Review thread node ID (e.g., 'PRRT_kwDOABCD...') - * @returns {Promise} The PR number the thread belongs to, or null if not found + * @returns {Promise<{prNumber: number, repoNameWithOwner: string|null}|null>} The PR number and repo, or null if not found */ -async function getThreadPullRequestNumber(github, threadId) { +async function getThreadPullRequestInfo(github, threadId) { const query = /* GraphQL */ ` query ($threadId: ID!) { node(id: $threadId) { ... on PullRequestReviewThread { pullRequest { number + repository { + nameWithOwner + } } } } @@ -37,7 +41,15 @@ async function getThreadPullRequestNumber(github, threadId) { const result = await github.graphql(query, { threadId }); - return result?.node?.pullRequest?.number ?? null; + const pullRequest = result?.node?.pullRequest; + if (!pullRequest) { + return null; + } + + return { + prNumber: pullRequest.number, + repoNameWithOwner: pullRequest.repository?.nameWithOwner ?? null, + }; } /** @@ -70,14 +82,21 @@ async function resolveReviewThreadAPI(github, threadId) { * Main handler factory for resolve_pull_request_review_thread * Returns a message handler function that processes individual resolve messages. * - * Resolution is scoped to the triggering PR only — the handler validates that each - * thread belongs to the triggering pull request before resolving it. This prevents - * agents from resolving threads on unrelated PRs. + * By default, resolution is scoped to the triggering PR only. When target-repo or + * allowed-repos are specified, cross-repository thread resolution is supported. * @type {HandlerFactoryFunction} */ async function main(config = {}) { // Extract configuration const maxCount = config.max || 10; + const resolveTarget = config.target || "triggering"; + const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config); + + // Whether the user explicitly configured cross-repo targeting. + // defaultTargetRepo always has a value (falls back to context.repo), so we check + // the raw config keys to distinguish user-configured from default. + const hasExplicitTargetConfig = !!(config["target-repo"] || config.allowed_repos?.length > 0); + const authClient = await createAuthenticatedGitHubClient(config); // Determine the triggering PR number from context @@ -86,7 +105,11 @@ async function main(config = {}) { // Check if we're in staged mode const isStaged = process.env.GH_AW_SAFE_OUTPUTS_STAGED === "true"; - core.info(`Resolve PR review thread configuration: max=${maxCount}, triggeringPR=${triggeringPRNumber || "none"}`); + core.info(`Resolve PR review thread configuration: max=${maxCount}, target=${resolveTarget}, triggeringPR=${triggeringPRNumber || "none"}`); + core.info(`Default target repo: ${defaultTargetRepo}`); + if (allowedRepos.size > 0) { + core.info(`Allowed repos: ${Array.from(allowedRepos).join(", ")}`); + } // Track how many items we've processed for max limit let processedCount = 0; @@ -122,18 +145,9 @@ async function main(config = {}) { }; } - // Validate triggering PR context - if (!triggeringPRNumber) { - core.warning("Cannot resolve review thread: not running in a pull request context"); - return { - success: false, - error: "Cannot resolve review threads outside of a pull request context", - }; - } - - // Look up the thread to validate it belongs to the triggering PR - const threadPRNumber = await getThreadPullRequestNumber(authClient, threadId); - if (threadPRNumber === null) { + // Look up the thread's PR number and repository + const threadInfo = await getThreadPullRequestInfo(authClient, threadId); + if (threadInfo === null) { core.warning(`Review thread not found or not a PullRequestReviewThread: ${threadId}`); return { success: false, @@ -141,15 +155,84 @@ async function main(config = {}) { }; } - if (threadPRNumber !== triggeringPRNumber) { - core.warning(`Thread ${threadId} 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`, - }; + const { prNumber: threadPRNumber, repoNameWithOwner: threadRepo } = threadInfo; + + // When the user explicitly configured target-repo or allowed-repos, validate the thread's + // repository using validateTargetRepo (supports wildcards like "*", "org/*"). + // Otherwise, fall back to the legacy behavior of scoping to the triggering PR only. + 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}`); + return { + success: false, + error: `Could not determine the repository for thread ${threadId}`, + }; + } + const repoValidation = validateTargetRepo(threadRepo, defaultTargetRepo, allowedRepos); + if (!repoValidation.valid) { + core.warning(`Thread ${threadId} belongs to repo ${threadRepo}, which is not in the allowed repos`); + return { + success: false, + error: repoValidation.error, + }; + } + + // Determine target PR number based on target config + if (resolveTarget === "triggering") { + if (!triggeringPRNumber) { + core.warning("Cannot resolve review thread: not running in a pull request context"); + return { + success: false, + error: "Cannot resolve review threads outside of a pull request context", + }; + } + if (threadPRNumber !== triggeringPRNumber) { + core.warning(`Thread ${threadId} 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`, + }; + } + } else if (resolveTarget !== "*") { + // Explicit PR number target + const targetPRNumber = parseInt(resolveTarget, 10); + if (Number.isNaN(targetPRNumber) || targetPRNumber <= 0) { + core.warning(`Invalid target PR number: '${resolveTarget}'`); + return { + success: false, + error: `Invalid target: '${resolveTarget}' - must be 'triggering', '*', or a positive integer`, + }; + } + if (threadPRNumber !== targetPRNumber) { + core.warning(`Thread ${threadId} belongs to PR #${threadPRNumber}, not target PR #${targetPRNumber}`); + return { + success: false, + error: `Thread belongs to PR #${threadPRNumber}, but target is PR #${targetPRNumber}`, + }; + } + } + // resolveTarget === "*": any PR in allowed repos — no further PR number check needed + } else { + // Default (legacy) mode: scope to triggering PR only + if (!triggeringPRNumber) { + core.warning("Cannot resolve review thread: not running in a pull request context"); + return { + success: false, + error: "Cannot resolve review threads outside of a pull request context", + }; + } + + if (threadPRNumber !== triggeringPRNumber) { + core.warning(`Thread ${threadId} 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`, + }; + } } - core.info(`Resolving review thread: ${threadId} (PR #${triggeringPRNumber})`); + core.info(`Resolving review thread: ${threadId} (PR #${threadPRNumber}${threadRepo ? " in " + threadRepo : ""})`); // If in staged mode, preview without executing if (isStaged) { @@ -159,7 +242,7 @@ async function main(config = {}) { staged: true, previewInfo: { thread_id: threadId, - pr_number: triggeringPRNumber, + pr_number: threadPRNumber, }, }; } diff --git a/actions/setup/js/resolve_pr_review_thread.test.cjs b/actions/setup/js/resolve_pr_review_thread.test.cjs index 1a4051be7b..69d5af7ef9 100644 --- a/actions/setup/js/resolve_pr_review_thread.test.cjs +++ b/actions/setup/js/resolve_pr_review_thread.test.cjs @@ -37,8 +37,9 @@ global.context = mockContext; /** * Helper to set up mockGraphql to handle both the lookup query and the resolve mutation. * @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) { +function mockGraphqlForThread(lookupPRNumber, lookupRepo = "test-owner/test-repo") { mockGraphql.mockImplementation(query => { if (query.includes("resolveReviewThread")) { // Mutation @@ -54,7 +55,10 @@ function mockGraphqlForThread(lookupPRNumber) { // Lookup query return Promise.resolve({ node: { - pullRequest: { number: lookupPRNumber }, + pullRequest: { + number: lookupPRNumber, + repository: { nameWithOwner: lookupRepo }, + }, }, }); }); @@ -318,6 +322,243 @@ describe("resolve_pr_review_thread", () => { }); }); +describe("resolve_pr_review_thread - cross-repo support", () => { + const originalPayload = mockContext.payload; + + beforeEach(() => { + vi.resetModules(); + vi.clearAllMocks(); + }); + + afterEach(() => { + global.context.payload = originalPayload; + }); + + it("should allow resolving a thread in target-repo when configured", async () => { + mockGraphqlForThread(10, "other-owner/other-repo"); + + const { main } = require("./resolve_pr_review_thread.cjs"); + const freshHandler = await main({ + max: 10, + "target-repo": "other-owner/other-repo", + target: "*", + }); + + const message = { + type: "resolve_pull_request_review_thread", + thread_id: "PRRT_kwDOCrossRepo", + }; + + const result = await freshHandler(message, {}); + + expect(result.success).toBe(true); + expect(result.thread_id).toBe("PRRT_kwDOCrossRepo"); + expect(result.is_resolved).toBe(true); + }); + + it("should reject a thread whose repo is not in allowed-repos", async () => { + mockGraphqlForThread(10, "other-owner/other-repo"); + + const { main } = require("./resolve_pr_review_thread.cjs"); + const freshHandler = await main({ + max: 10, + "target-repo": "allowed-owner/allowed-repo", + target: "*", + }); + + const message = { + type: "resolve_pull_request_review_thread", + thread_id: "PRRT_kwDOCrossRepo", + }; + + const result = await freshHandler(message, {}); + + expect(result.success).toBe(false); + expect(result.error).toContain("other-owner/other-repo"); + expect(result.error).toContain("allowed"); + }); + + it("should allow cross-repo thread in allowed_repos list", async () => { + mockGraphqlForThread(10, "extra-owner/extra-repo"); + + const { main } = require("./resolve_pr_review_thread.cjs"); + const freshHandler = await main({ + max: 10, + "target-repo": "allowed-owner/allowed-repo", + allowed_repos: ["extra-owner/extra-repo"], + target: "*", + }); + + const message = { + type: "resolve_pull_request_review_thread", + thread_id: "PRRT_kwDOCrossRepo", + }; + + const result = await freshHandler(message, {}); + + expect(result.success).toBe(true); + }); + + it("should reject thread not on target PR when target is an explicit PR number", async () => { + mockGraphqlForThread(99, "other-owner/other-repo"); + + const { main } = require("./resolve_pr_review_thread.cjs"); + const freshHandler = await main({ + max: 10, + "target-repo": "other-owner/other-repo", + target: "55", + }); + + const message = { + type: "resolve_pull_request_review_thread", + thread_id: "PRRT_kwDOCrossRepo", + }; + + const result = await freshHandler(message, {}); + + expect(result.success).toBe(false); + expect(result.error).toContain("PR #99"); + expect(result.error).toContain("PR #55"); + }); + + it("should allow thread on correct explicit target PR", async () => { + mockGraphqlForThread(55, "other-owner/other-repo"); + + const { main } = require("./resolve_pr_review_thread.cjs"); + const freshHandler = await main({ + max: 10, + "target-repo": "other-owner/other-repo", + target: "55", + }); + + const message = { + type: "resolve_pull_request_review_thread", + thread_id: "PRRT_kwDOCrossRepo", + }; + + const result = await freshHandler(message, {}); + + expect(result.success).toBe(true); + }); + + it("should require triggering PR context when target=triggering with cross-repo config", async () => { + global.context.payload = { + repository: { html_url: "https://github.com/test-owner/test-repo" }, + }; + mockGraphqlForThread(10, "other-owner/other-repo"); + + const { main } = require("./resolve_pr_review_thread.cjs"); + const freshHandler = await main({ + max: 10, + "target-repo": "other-owner/other-repo", + target: "triggering", + }); + + const message = { + type: "resolve_pull_request_review_thread", + thread_id: "PRRT_kwDOCrossRepo", + }; + + const result = await freshHandler(message, {}); + + expect(result.success).toBe(false); + expect(result.error).toContain("pull request context"); + }); + + it("should allow resolving when allowed_repos uses '*' wildcard", async () => { + mockGraphqlForThread(10, "any-owner/any-repo"); + + const { main } = require("./resolve_pr_review_thread.cjs"); + const freshHandler = await main({ + max: 10, + "target-repo": "default-owner/default-repo", + allowed_repos: ["*"], + target: "*", + }); + + const message = { + type: "resolve_pull_request_review_thread", + thread_id: "PRRT_kwDOWildcard", + }; + + const result = await freshHandler(message, {}); + + expect(result.success).toBe(true); + }); + + it("should allow resolving when allowed_repos uses org/* pattern", async () => { + mockGraphqlForThread(10, "other-owner/specific-repo"); + + const { main } = require("./resolve_pr_review_thread.cjs"); + const freshHandler = await main({ + max: 10, + "target-repo": "default-owner/default-repo", + allowed_repos: ["other-owner/*"], + target: "*", + }); + + const message = { + type: "resolve_pull_request_review_thread", + thread_id: "PRRT_kwDOOrgWildcard", + }; + + const result = await freshHandler(message, {}); + + expect(result.success).toBe(true); + }); + + it("should reject resolving when org/* pattern does not match", async () => { + mockGraphqlForThread(10, "wrong-owner/specific-repo"); + + const { main } = require("./resolve_pr_review_thread.cjs"); + const freshHandler = await main({ + max: 10, + "target-repo": "default-owner/default-repo", + allowed_repos: ["other-owner/*"], + target: "*", + }); + + const message = { + type: "resolve_pull_request_review_thread", + thread_id: "PRRT_kwDOOrgWildcard", + }; + + const result = await freshHandler(message, {}); + + expect(result.success).toBe(false); + expect(result.error).toContain("not in the allowed-repos list"); + }); + + it("should fail closed when threadRepo is null in cross-repo mode", async () => { + // Simulate GraphQL returning a thread with no repository info + mockGraphql.mockImplementation(query => { + if (query.includes("resolveReviewThread")) { + return Promise.resolve({ resolveReviewThread: { thread: { id: "PRRT_x", isResolved: true } } }); + } + return Promise.resolve({ + node: { pullRequest: { number: 10, repository: null } }, + }); + }); + + const { main } = require("./resolve_pr_review_thread.cjs"); + const freshHandler = await main({ + max: 10, + "target-repo": "other-owner/other-repo", + target: "*", + }); + + const message = { + type: "resolve_pull_request_review_thread", + thread_id: "PRRT_kwDONoRepo", + }; + + const result = await freshHandler(message, {}); + + expect(result.success).toBe(false); + expect(result.error).toContain("Could not determine"); + }); +}); + describe("getPRNumber (shared helper in update_context_helpers)", () => { it("should return pull_request.number for pull_request events", () => { const { getPRNumber } = require("./update_context_helpers.cjs"); diff --git a/pkg/workflow/compiler_safe_outputs_config.go b/pkg/workflow/compiler_safe_outputs_config.go index 2d4299898e..c1c0282d3f 100644 --- a/pkg/workflow/compiler_safe_outputs_config.go +++ b/pkg/workflow/compiler_safe_outputs_config.go @@ -440,6 +440,9 @@ var handlerRegistry = map[string]handlerBuilder{ c := cfg.ResolvePullRequestReviewThread return newHandlerConfigBuilder(). AddTemplatableInt("max", c.Max). + AddIfNotEmpty("target", c.Target). + AddIfNotEmpty("target-repo", c.TargetRepoSlug). + AddStringSlice("allowed_repos", c.AllowedRepos). AddIfNotEmpty("github-token", c.GitHubToken). Build() }, diff --git a/pkg/workflow/resolve_pr_review_thread.go b/pkg/workflow/resolve_pr_review_thread.go index 7b5a6dd8ca..3ef25a19ef 100644 --- a/pkg/workflow/resolve_pr_review_thread.go +++ b/pkg/workflow/resolve_pr_review_thread.go @@ -7,10 +7,11 @@ import ( var resolvePRReviewThreadLog = logger.New("workflow:resolve_pr_review_thread") // ResolvePullRequestReviewThreadConfig holds configuration for resolving PR review threads. -// Resolution is scoped to the triggering PR only — the JavaScript handler validates -// that each thread belongs to the triggering pull request before resolving it. +// By default, resolution is scoped to the triggering PR only. When target, target-repo, +// or allowed-repos are specified, cross-repository thread resolution is supported. type ResolvePullRequestReviewThreadConfig struct { - BaseSafeOutputConfig `yaml:",inline"` + BaseSafeOutputConfig `yaml:",inline"` + SafeOutputTargetConfig `yaml:",inline"` } // parseResolvePullRequestReviewThreadConfig handles resolve-pull-request-review-thread configuration @@ -25,7 +26,11 @@ func (c *Compiler) parseResolvePullRequestReviewThreadConfig(outputMap map[strin // Parse common base fields with default max of 10 c.parseBaseSafeOutputConfig(configMap, &config.BaseSafeOutputConfig, 10) - resolvePRReviewThreadLog.Printf("Parsed resolve-pull-request-review-thread config: max=%d", config.Max) + // Parse target config (target, target-repo, allowed-repos) + targetConfig, _ := ParseTargetConfig(configMap) + config.SafeOutputTargetConfig = targetConfig + + resolvePRReviewThreadLog.Printf("Parsed resolve-pull-request-review-thread config: max=%d, target_repo=%s", templatableIntValue(config.Max), config.TargetRepoSlug) } else { // If configData is nil or not a map, still set the default max config.Max = defaultIntStr(10)