Skip to content

Include canonical issue IDs in duplicate close intents #73078

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 4 commits into from
Oct 30, 2026
Merged

Include canonical issue IDs in duplicate close intents #73078

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5e15bc4
Initial plan
Copilot Oct 30, 2026
f5f5766
Fix canonical duplicate issue IDs in close-issue intents
Copilot Oct 30, 2026
e4e5036
Harden duplicate close intent references
Copilot Oct 30, 2026
6ba8dc4
Retry canonical issue lookups
Copilot Oct 30, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions actions/setup/js/close_issue.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ async function addIssueComment(github, owner, repo, issueNumber, message) {
* @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
*/
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,
Expand All @@ -170,6 +170,19 @@ async function closeIssue(github, owner, repo, issueNumber, stateReason, intentM

const hasIntentMetadata = Boolean(intentMetadata && Object.keys(intentMetadata).length > 0);
if (useIssueIntent && hasIntentMetadata) {
let duplicateIssueId;
if (canonicalIssue) {
const { data } = await github.rest.issues.get({

@pelikhan pelikhan Oct 30, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot use withRetry

Copilot AI Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrapped this canonical lookup in withRetry using the rate-limit retry configuration. Commit: 6ba8dc4e.

owner: canonicalIssue.owner,
repo: canonicalIssue.repo,
issue_number: canonicalIssue.issueNumber,
});

@github-actions github-actions Bot Oct 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/codebase-design] Fetching the canonical issue here (for its REST id) duplicates the GET that markIssueAsDuplicate makes a few lines later for the same issue's node_id, costing an extra API round trip (and an extra rate-limit hit) on every duplicate-close path.

💡 Suggested consolidation

Consider having closeIssue return (or accept a shared fetcher/cache for) the canonical issue data, or restructure so the single github.rest.issues.get call feeding duplicate_issue_id also supplies node_id to markIssueAsDuplicate, avoiding two separate fetches of the same resource per close.

@copilot please address this.

Copilot AI Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The canonical REST response used for duplicate_issue_id is now reused for the native GraphQL relationship, avoiding a second GET; the test asserts this. Commit e4e5036.

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}", {
Expand All @@ -178,6 +191,7 @@ 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;
Expand Down Expand Up @@ -351,15 +365,16 @@ 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 canonicalIssue = parsed && validateRepo(`${parsed.owner}/${parsed.repo}`, defaultTargetRepo, allowedRepos).valid && !(parsed.owner === owner && parsed.repo === repo && parsed.issueNumber === entityNumber) ? parsed : undefined;

Copilot AI Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the self-reference guard to compare owner and repository case-insensitively, with a mixed-case regression test in close_issue.test.cjs. Commit e4e5036.

@github-actions github-actions Bot Oct 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still sends a duplicate close intent without the required duplicate_issue_id whenever duplicate_of is invalid, disallowed, or self-referential, so those cases continue to fail with HTTP 422 instead of closing successfully or failing before mutation.

💡 Reject or explicitly downgrade these cases

canonicalIssue becomes undefined for these inputs, but closeIssue does not know whether that means no reference was supplied or an ineligible reference was supplied. It therefore emits the same state_reason: duplicate intent payload without an ID. Please validate an ineligible/self reference before calling closeIssue (or deliberately choose the legacy close path), and add coverage for the resulting behavior rather than asserting the known-invalid PATCH shape.

Copilot AI Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate intents now reject invalid, disallowed, and self-referential targets before any mutation; tests cover those paths. Commit e4e5036.

@github-actions github-actions Bot Oct 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The self-reference guard is case-sensitive even though GitHub repository names are case-insensitive, so with a wildcard target policy a reference such as Owner/Repo#100 can resolve to the issue being closed and be sent as its own duplicate_issue_id.

💡 Normalize repository names before the self check

Compare parsed.owner and parsed.repo to owner and repo after lowercasing (or use a canonical repository identity), and add a mixed-case self-reference regression test. The same guard is duplicated in the native-marking branch below, so both paths need the normalized comparison.

Copilot AI Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shared eligibility check now rejects mixed-case self-references for both intent payloads and native duplicate marking; added regression coverage. Commit e4e5036.

@github-actions github-actions Bot Oct 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/codebase-design] The eligibility check for canonicalIssue (parsed + repo-allowed + not-self-reference) is re-derived independently in the if (item.duplicate_of ...) block below (lines ~379-391). Duplicated validation logic in two places risks silent divergence if one path is updated without the other.

💡 Suggested refactor

Extract a single helper, e.g. resolveEligibleCanonicalIssue(parsed, owner, repo, entityNumber, defaultTargetRepo, allowedRepos), returning { eligible, canonicalIssue, reason }. Use its result both to decide canonicalIssue for closeIssue(...) and to drive the warnings/skip logic in the native-duplicate-marking block, so there's one source of truth for "is this a legitimate, allowed, non-self canonical reference".

@copilot please address this.

Copilot AI Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added resolveEligibleCanonicalIssue as the single source for parsing/allowlist/self-reference eligibility used by both intent and native-marking paths. Commit e4e5036.

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}`;
Expand Down
83 changes: 83 additions & 0 deletions actions/setup/js/close_issue.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1189,6 +1189,87 @@ 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 = [];
mockGithub.rest.issues.get = async params => {
getCalls.push(params);
return { data: { id: 100000 + 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, title: "Test Issue", state: "open" } };
};

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,
},
},
]);
});

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([{ duplicate_of: "invalid" }, { duplicate_of: 100 }, { duplicate_of: "other-org/other-repo#50" }, { duplicate_of: 50, state_reason: "completed" }])(
"should not resolve a canonical ID for an ineligible duplicate reference: %j",
async fields => {
const handler = await main();
const getCalls = [];
const requestCalls = [];
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" } };
};

const result = await handler({ issue_number: 100, body: "Close", state_reason: "duplicate", suggest: true, ...fields }, {});

expect(result.success).toBe(true);
expect(getCalls).toHaveLength(1);
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 = [];
Expand Down Expand Up @@ -1520,6 +1601,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: [],
Expand Down Expand Up @@ -1563,6 +1645,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);
Expand Down
Loading