diff --git a/actions/setup/js/close_issue.cjs b/actions/setup/js/close_issue.cjs index 435b2ae70a5..016b331ceca 100644 --- a/actions/setup/js/close_issue.cjs +++ b/actions/setup/js/close_issue.cjs @@ -13,6 +13,7 @@ const { loadTemporaryIdMapFromResolved, resolveRepoIssueTarget } = require("./te const { getErrorMessage } = require("./error_helpers.cjs"); const { normalizeIssueIntentMetadata } = require("./issue_intents.cjs"); const { resolveTarget } = require("./safe_output_helpers.cjs"); +const { withRetry, RATE_LIMIT_RETRY_CONFIG } = require("./error_recovery.cjs"); /** * Parse a `duplicate_of` value into { owner, repo, issueNumber }. @@ -67,22 +68,31 @@ function parseDuplicateOf(value, defaultOwner, defaultRepo) { * This creates a native "marked this as a duplicate of #X" timeline event. * * @param {any} github - Authenticated GitHub client (must support graphql) - * @param {string} duplicateNodeId - GraphQL node ID of the issue being marked as a duplicate + * @param {string|undefined} duplicateNodeId - GraphQL node ID of the issue being marked as a duplicate * @param {string} canonicalOwner - Owner of the canonical issue's repo * @param {string} canonicalRepo - Repo of the canonical issue * @param {number} canonicalNumber - Issue number that is the canonical original * @returns {Promise} */ -async function markIssueAsDuplicate(github, duplicateNodeId, canonicalOwner, canonicalRepo, canonicalNumber) { +async function markIssueAsDuplicate(github, duplicateNodeId, canonicalOwner, canonicalRepo, canonicalNumber, canonicalIssueData) { if (!duplicateNodeId) { throw new Error(`${ERR_NOT_FOUND}: node_id missing for duplicate issue`); } - const { data: canonicalData } = await github.rest.issues.get({ - owner: canonicalOwner, - repo: canonicalRepo, - issue_number: canonicalNumber, - }); + let canonicalData = canonicalIssueData; + if (!canonicalData) { + const response = await withRetry( + () => + github.rest.issues.get({ + owner: canonicalOwner, + repo: canonicalRepo, + issue_number: canonicalNumber, + }), + RATE_LIMIT_RETRY_CONFIG, + `get canonical issue ${canonicalOwner}/${canonicalRepo}#${canonicalNumber}` + ); + canonicalData = response.data; + } const canonicalNodeId = canonicalData.node_id; if (!canonicalNodeId) { @@ -108,6 +118,23 @@ async function markIssueAsDuplicate(github, duplicateNodeId, canonicalOwner, can ); } +function resolveEligibleCanonicalIssue(parsed, owner, repo, issueNumber, defaultTargetRepo, allowedRepos) { + if (!parsed) { + return { canonicalIssue: undefined, reason: "duplicate_of value could not be parsed" }; + } + + const canonicalRepoSlug = `${parsed.owner}/${parsed.repo}`; + if (!validateRepo(canonicalRepoSlug, defaultTargetRepo, allowedRepos).valid) { + return { canonicalIssue: undefined, reason: `canonical repo "${canonicalRepoSlug}" is not in the allowed-repos list` }; + } + + if (parsed.owner.toLowerCase() === owner.toLowerCase() && parsed.repo.toLowerCase() === repo.toLowerCase() && parsed.issueNumber === issueNumber) { + return { canonicalIssue: undefined, reason: `issue #${issueNumber} cannot be a duplicate of itself` }; + } + + return { canonicalIssue: parsed, reason: undefined }; +} + /** * Get issue details using REST API * @param {any} github - GitHub REST API instance @@ -157,9 +184,9 @@ async function addIssueComment(github, owner, repo, issueNumber, message) { * @param {string} repo - Repository name * @param {number} issueNumber - Issue number * @param {string} stateReason - The reason for closing: "COMPLETED", "NOT_PLANNED", or "DUPLICATE" - * @returns {Promise<{number: number, html_url: string, title: string, node_id: string}>} Issue details + * @returns {Promise<{issue: {number: number, html_url: string, title: string, node_id?: string}, canonicalIssueData?: {id?: number, node_id?: string}}>} Close result and fetched canonical issue details */ -async function closeIssue(github, owner, repo, issueNumber, stateReason, intentMetadata, useIssueIntent) { +async function closeIssue(github, owner, repo, issueNumber, stateReason, intentMetadata, useIssueIntent, canonicalIssue) { const baseParams = { owner, repo, @@ -169,7 +196,30 @@ async function closeIssue(github, owner, repo, issueNumber, stateReason, intentM }; const hasIntentMetadata = Boolean(intentMetadata && Object.keys(intentMetadata).length > 0); + let canonicalIssueData; if (useIssueIntent && hasIntentMetadata) { + let duplicateIssueId; + if (baseParams.state_reason === "duplicate" && !canonicalIssue) { + throw new Error(`${ERR_VALIDATION}: duplicate close intent requires a valid, allowed, non-self duplicate_of reference`); + } + if (canonicalIssue) { + const { data } = await withRetry( + () => + github.rest.issues.get({ + owner: canonicalIssue.owner, + repo: canonicalIssue.repo, + issue_number: canonicalIssue.issueNumber, + }), + RATE_LIMIT_RETRY_CONFIG, + `get canonical issue ${canonicalIssue.owner}/${canonicalIssue.repo}#${canonicalIssue.issueNumber}` + ); + canonicalIssueData = data; + duplicateIssueId = data.id; + if (!duplicateIssueId) { + throw new Error(`${ERR_NOT_FOUND}: id missing for canonical issue ${canonicalIssue.owner}/${canonicalIssue.repo}#${canonicalIssue.issueNumber}`); + } + } + try { core.debug("Attempting issue-intent close request"); const { data: issue } = await github.request("PATCH /repos/{owner}/{repo}/issues/{issue_number}", { @@ -178,9 +228,10 @@ async function closeIssue(github, owner, repo, issueNumber, stateReason, intentM issue_number: issueNumber, state: { value: "closed", ...intentMetadata }, state_reason: baseParams.state_reason, + ...(duplicateIssueId !== undefined ? { duplicate_issue_id: duplicateIssueId } : {}), }); core.debug("Issue-intent close request succeeded"); - return issue; + return { issue, canonicalIssueData }; } catch (error) { const errorRecord = error && typeof error === "object" ? /** @type {Record} */ error : undefined; const response = errorRecord?.response; @@ -196,7 +247,7 @@ async function closeIssue(github, owner, repo, issueNumber, stateReason, intentM const { data: issue } = await github.rest.issues.update(baseParams); - return issue; + return { issue, canonicalIssueData: undefined }; } /** @@ -351,46 +402,35 @@ async function main(config = {}) { const intentMetadata = issueIntentEnabled ? normalizeIssueIntentMetadata(item) : {}; core.info(`Closing issue #${entityNumber} with state_reason=${stateReason}`); - const closePromise = closeIssue(github, owner, repo, entityNumber, stateReason, intentMetadata, issueIntentEnabled); + const stateReasonUpper = stateReason.toUpperCase(); + const parsed = stateReasonUpper === "DUPLICATE" ? parseDuplicateOf(item.duplicate_of, owner, repo) : null; + const canonicalIssueResolution = resolveEligibleCanonicalIssue(parsed, owner, repo, entityNumber, defaultTargetRepo, allowedRepos); + const canonicalIssue = canonicalIssueResolution.canonicalIssue; + const closePromise = closeIssue(github, owner, repo, entityNumber, stateReason, intentMetadata, issueIntentEnabled, canonicalIssue); // When duplicate_of is provided and state_reason is DUPLICATE, create the native duplicate relationship - const stateReasonUpper = stateReason.toUpperCase(); if (item.duplicate_of !== undefined && item.duplicate_of !== null && stateReasonUpper !== "DUPLICATE") { core.warning(`duplicate_of is set but state_reason is ${stateReason} (not DUPLICATE); native duplicate marking will be skipped`); } if (item.duplicate_of !== undefined && item.duplicate_of !== null && stateReasonUpper === "DUPLICATE") { - const parsed = parseDuplicateOf(item.duplicate_of, owner, repo); - if (parsed) { - // Validate canonical repo against the same allowedRepos policy - const canonicalRepoSlug = `${parsed.owner}/${parsed.repo}`; - const repoValidation = validateRepo(canonicalRepoSlug, defaultTargetRepo, allowedRepos); - if (!repoValidation.valid) { - core.warning(`Skipping native duplicate marking: canonical repo "${canonicalRepoSlug}" is not in the allowed-repos list`); - return closePromise; - } - - // Guard against marking an issue as a duplicate of itself - if (parsed.owner === owner && parsed.repo === repo && parsed.issueNumber === entityNumber) { - core.warning(`Skipping native duplicate marking: issue #${entityNumber} cannot be a duplicate of itself`); - return closePromise; - } - - core.info(`Marking issue #${entityNumber} as duplicate of ${parsed.owner}/${parsed.repo}#${parsed.issueNumber}`); - return closePromise.then(async closedEntity => { - try { - await markIssueAsDuplicate(github, closedEntity.node_id, parsed.owner, parsed.repo, parsed.issueNumber); - core.info(`✓ Marked issue #${entityNumber} as duplicate of ${parsed.owner}/${parsed.repo}#${parsed.issueNumber}`); - } catch (dupError) { - core.warning(`Failed to mark native duplicate relationship for #${entityNumber}: ${getErrorMessage(dupError)}`); - } - return closedEntity; - }); + if (!canonicalIssue) { + core.warning(`Skipping native duplicate marking: ${canonicalIssueResolution.reason}`); } else { - core.warning(`duplicate_of value "${item.duplicate_of}" could not be parsed; skipping native duplicate marking`); + core.info(`Marking issue #${entityNumber} as duplicate of ${canonicalIssue.owner}/${canonicalIssue.repo}#${canonicalIssue.issueNumber}`); } } - return closePromise; + return closePromise.then(async ({ issue: closedEntity, canonicalIssueData }) => { + if (canonicalIssue) { + try { + await markIssueAsDuplicate(github, closedEntity.node_id, canonicalIssue.owner, canonicalIssue.repo, canonicalIssue.issueNumber, canonicalIssueData); + core.info(`✓ Marked issue #${entityNumber} as duplicate of ${canonicalIssue.owner}/${canonicalIssue.repo}#${canonicalIssue.issueNumber}`); + } catch (dupError) { + core.warning(`Failed to mark native duplicate relationship for #${entityNumber}: ${getErrorMessage(dupError)}`); + } + } + return closedEntity; + }); }, continueOnCommentError: false, diff --git a/actions/setup/js/close_issue.test.cjs b/actions/setup/js/close_issue.test.cjs index 6a77ee9bb19..c920aa15356 100644 --- a/actions/setup/js/close_issue.test.cjs +++ b/actions/setup/js/close_issue.test.cjs @@ -1189,6 +1189,107 @@ describe("close_issue", () => { }); describe("duplicate_of native marking", () => { + it.each([50, "50", "#50", "test-owner/test-repo#50", "https://github.com/test-owner/test-repo/issues/50", "other-org/other-repo#50"])("should resolve %s to a database ID before sending a duplicate close intent", async duplicateOf => { + const handler = await main({ allowed_repos: ["other-org/other-repo"] }); + const getCalls = []; + const requestCalls = []; + const graphqlCalls = []; + mockGithub.rest.issues.get = async params => { + getCalls.push(params); + return { data: { id: 100000 + params.issue_number, node_id: `node_${params.issue_number}`, number: params.issue_number, title: "Test Issue", labels: [], state: "open" } }; + }; + mockGithub.request = async (route, params) => { + expect(getCalls).toHaveLength(2); + requestCalls.push({ route, params }); + return { data: { number: params.issue_number, node_id: `node_${params.issue_number}`, title: "Test Issue", state: "open" } }; + }; + mockGithub.graphql = async (mutation, variables) => graphqlCalls.push(variables); + + const result = await handler({ issue_number: 100, body: "Duplicate", state_reason: "duplicate", duplicate_of: duplicateOf, suggest: true, rationale: "Same defect", confidence: "high" }, {}); + + expect(result.success).toBe(true); + expect(getCalls[1]).toEqual({ + owner: String(duplicateOf).startsWith("other-org/") ? "other-org" : "test-owner", + repo: String(duplicateOf).startsWith("other-org/") ? "other-repo" : "test-repo", + issue_number: 50, + }); + expect(requestCalls).toEqual([ + { + route: "PATCH /repos/{owner}/{repo}/issues/{issue_number}", + params: { + owner: "test-owner", + repo: "test-repo", + issue_number: 100, + state: { value: "closed", suggest: true, rationale: "Same defect", confidence: "HIGH" }, + state_reason: "duplicate", + duplicate_issue_id: 100050, + }, + }, + ]); + expect(getCalls).toHaveLength(2); + expect(graphqlCalls).toHaveLength(1); + expect(graphqlCalls[0].canonicalId).toBe("node_50"); + }); + + it.each([403, 404, "missing ID"])("should fail closed when canonical issue lookup fails with %s", async failure => { + const handler = await main(); + const mutations = []; + mockGithub.rest.issues.get = async ({ issue_number }) => { + if (issue_number === 50 && typeof failure === "number") { + throw Object.assign(new Error("Canonical issue unavailable"), { status: failure }); + } + return { data: { number: issue_number, title: "Test Issue", labels: [], state: "open" } }; + }; + mockGithub.request = async () => mutations.push("intent"); + mockGithub.rest.issues.update = async () => mutations.push("legacy close"); + mockGithub.graphql = async () => mutations.push("duplicate marking"); + + const result = await handler({ issue_number: 100, body: "Duplicate", state_reason: "duplicate", duplicate_of: 50, suggest: true }, {}); + + expect(result.success).toBe(false); + expect(result.error).toContain(failure === "missing ID" ? "id missing for canonical issue" : "Canonical issue unavailable"); + expect(mutations).toEqual([]); + }); + + it.each([ + { fields: { duplicate_of: "invalid" }, rejected: true }, + { fields: { duplicate_of: 100 }, rejected: true }, + { fields: { duplicate_of: "Test-Owner/Test-Repo#100" }, rejected: true }, + { fields: { duplicate_of: "other-org/other-repo#50" }, rejected: true }, + { fields: { duplicate_of: 50, state_reason: "completed" }, rejected: false }, + ])("should handle an ineligible duplicate reference safely: %j", async ({ fields, rejected }) => { + const handler = await main(); + const getCalls = []; + const requestCalls = []; + const updateCalls = []; + mockGithub.rest.issues.get = async params => { + getCalls.push(params); + return { data: { number: params.issue_number, title: "Test Issue", labels: [], state: "open" } }; + }; + mockGithub.request = async (route, params) => { + requestCalls.push(params); + return { data: { number: params.issue_number, title: "Test Issue" } }; + }; + mockGithub.rest.issues.update = async params => { + updateCalls.push(params); + return { data: { number: params.issue_number, title: "Test Issue" } }; + }; + + const result = await handler({ issue_number: 100, body: "Close", state_reason: "duplicate", suggest: true, ...fields }, {}); + + expect(getCalls).toHaveLength(1); + if (rejected) { + expect(result.success).toBe(false); + expect(result.error).toContain("duplicate close intent requires a valid"); + expect(requestCalls).toHaveLength(0); + expect(updateCalls).toHaveLength(0); + } else { + expect(result.success).toBe(true); + expect(requestCalls).toHaveLength(1); + expect(requestCalls[0].duplicate_issue_id).toBeUndefined(); + } + }); + it("should call markAsDuplicate mutation when duplicate_of and state_reason DUPLICATE are set", async () => { const handler = await main({ max: 10, state_reason: "DUPLICATE" }); const graphqlCalls = []; @@ -1520,6 +1621,7 @@ describe("close_issue", () => { mockGithub.rest.issues.get = async ({ owner, repo, issue_number }) => ({ data: { + id: 100000 + issue_number, number: issue_number, title: "Test Issue", labels: [], @@ -1563,6 +1665,7 @@ describe("close_issue", () => { expect(requestCalls).toHaveLength(1); expect(requestCalls[0].route).toBe("PATCH /repos/{owner}/{repo}/issues/{issue_number}"); expect(requestCalls[0].params.state).toMatchObject({ value: "closed", suggest: true, rationale: "Same CSV defect.", confidence: "HIGH" }); + expect(requestCalls[0].params.duplicate_issue_id).toBe(100050); // Verify the native duplicate GraphQL mutation was also executed expect(graphqlCalls.length).toBeGreaterThan(0);