From f9a29a9184b11f2848cdc88ded1aff19d3d866f7 Mon Sep 17 00:00:00 2001 From: Sydney Gomes <67939367+aerodeval@users.noreply.github.com> Date: Thu, 12 Nov 2026 22:57:41 +0530 Subject: [PATCH 1/2] fix: ignore associated pull requests of other repositories (#1329) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Problem When a release is cut from a branch that is not the default branch (e.g. releases from `main` while `develop` is the default), the `success` step crashes and posts no comment at all: ``` HttpError: Not Found GET /repos/frappe/helpdesk/pulls/66/commits ``` Root cause: the GraphQL field `associatedPullRequests` returns every pull request that contains the commit, including pull requests opened in **other repositories**. For commits on the default branch GitHub only returns the merged PR, but for commits on other branches it also returns open PRs from forks (for example a fork's `pull[bot]` sync PR `fork-owner/fork#66`). The plugin then calls `GET /repos///pulls/66/commits` on the released repository, where `#66` is an unrelated issue, gets a 404, and the `pFilter` over all associated PRs rejects. Verified with the GraphQL API on frappe/helpdesk commit `c295efa8e`: `associatedPullRequests` returns `#66` (repository `jango-blockchained/desk`, a fork) next to `#3882`, `#3897` and `#3931` (repository `frappe/helpdesk`). This happens on every release while such a fork PR stays open. Same bug as reported in #1032 and #1092. #1292 proposed the same approach but was closed by its author before review; this PR adds the regression test and the pagination fix below. ## Fix - Request `repository { nameWithOwner }` for each associated PR and drop PRs whose repository is not the released one. The comparison uses the `owner/repo` the plugin already resolves via `GET /repos/{owner}/{repo}`, so renamed repositories keep working. - The filter runs before `uniqBy(…, "number")`, so a foreign PR can no longer shadow a same-numbered PR of this repository. - While touching the loop: the pagination branch for commits with more than 100 associated PRs referenced an undefined variable (`response.commit.oid`) and threw a `ReferenceError`. The per-commit pagination now lives in a small `loadAllAssociatedPRs` helper (`while (pageInfo.hasNextPage)`) that passes the commit's `oid`, which also removes the manual `cursor`/`hasNextPage` flags and the duplicated push site. The existing "multipaged associatedPRs" test never reached that branch because its second `postOnce` on the same URL used `overwriteRoutes: true` and replaced the first mock; it now matches on the query name and asserts the `sha` and `cursor` variables. - The PR field selection was copied in all three queries; it is now one `prFields` fragment, so `repository` cannot be present on page 1 but missing on page 2. The two `flatten()` calls on already-flat arrays are gone with it. Note: the pagination path is live for the first time. On a heavily forked repository a non-default-branch commit can carry hundreds of fork PRs, which now means a few sequential 100-node pages per commit instead of a crash. ## Edge cases considered - PRs opened from a fork **into** this repository have `repository` = this repository (only `headRepository` is the fork), so contributor PRs are still commented on. Filtering on `headRepository` would be wrong. - Same-repository PRs targeting non-default branches (e.g. backport PRs) are kept, as before. - Releasing from a fork (the #1032 scenario): the upstream repository's PRs are now dropped, as they should be. - Renamed or transferred repositories: both sides of the comparison come from GitHub's canonical name. - Issues referenced in commit or PR bodies are unaffected; that path already filters on the `owner/repo` slug. ## Tests - New test: a fork PR `#66` is returned next to PR `#1` of this repository; only `#1` is looked up and commented on (no mock exists for `#66`, so any request for it fails the test). - "multipaged associatedPRs" test now exercises the second GraphQL query and the four PRs returned across both pages. Co-authored-by: Olabode Lawal-Shittabey --- lib/success.js | 132 ++++++++++------------ test/success.test.js | 254 ++++++++++++++++++++++++++++++++++++------- 2 files changed, 272 insertions(+), 114 deletions(-) diff --git a/lib/success.js b/lib/success.js index 48246f29..98f7ae0c 100644 --- a/lib/success.js +++ b/lib/success.js @@ -1,4 +1,4 @@ -import { isNil, uniqBy, template, flatten, isEmpty, merge } from "lodash-es"; +import { isNil, uniqBy, template, isEmpty, merge } from "lodash-es"; import pFilter from "p-filter"; import AggregateError from "aggregate-error"; import issueParser from "issue-parser"; @@ -75,47 +75,29 @@ export default async function success(pluginConfig, context, { Octokit }) { const releaseInfos = releases.filter((release) => Boolean(release.name)); const shas = commits.map(({ hash }) => hash); + // For commits that are not on the default branch, `associatedPullRequests` + // also returns pull requests of other repositories (e.g. forks syncing from + // this one). Their numbers refer to the other repository, so skip them. + const isFromThisRepository = ({ repository }) => + !repository || repository.nameWithOwner === `${owner}/${repo}`; + // Get associatedPRs const associatedPRs = await inChunks(shas, 100, async (chunk) => { - const responsePRs = []; const { repository } = await octokit.graphql( buildAssociatedPRsQuery(chunk), { owner, repo }, ); - const responseAssociatedPRs = Object.values(repository).map( - (item) => item.associatedPullRequests, + const nodes = await Promise.all( + Object.values(repository).map((commit) => + getAssociatedPRsOfCommit(octokit, owner, repo, commit), + ), + ); + return buildIssuesOrPRsFromResponseNode( + nodes.flat().filter(isFromThisRepository), ); - for (const { nodes, pageInfo } of responseAssociatedPRs) { - if (nodes.length === 0) continue; - - responsePRs.push(...buildIssuesOrPRsFromResponseNode(nodes)); - if (pageInfo.hasNextPage) { - let cursor = pageInfo.endCursor; - let hasNextPage = true; - while (hasNextPage) { - const { repository } = await octokit.graphql( - loadSingleCommitAssociatedPRs, - { owner, repo, sha: response.commit.oid, cursor }, - ); - const { associatedPullRequests } = repository.commit; - responsePRs.push( - ...buildIssuesOrPRsFromResponseNode( - associatedPullRequests.nodes, - "PR", - ), - ); - if (associatedPullRequests.pageInfo.hasNextPage) { - cursor = associatedPullRequests.pageInfo.endCursor; - } else { - hasNextPage = false; - } - } - } - } - return responsePRs; }); - const uniqueAssociatedPRs = uniqBy(flatten(associatedPRs), "number"); + const uniqueAssociatedPRs = uniqBy(associatedPRs, "number"); const prs = await pFilter(uniqueAssociatedPRs, async ({ number }) => { const commits = await octokit.paginate( @@ -169,7 +151,7 @@ export default async function success(pluginConfig, context, { Octokit }) { let issues = []; if (!isEmpty(parsedIssues)) { - const uniqueParsedIssues = uniqBy(flatten(parsedIssues), "number"); + const uniqueParsedIssues = uniqBy(parsedIssues, "number"); // Get relatedIssues (or relatedPRs i.e. Issues/PRs that are closed by an associatedPR) issues = await inChunks(uniqueParsedIssues, 100, async (chunk) => { @@ -354,6 +336,24 @@ async function inChunks(items, chunkSize, callback) { return results.flat(); } +/** + * Returns the `associatedPullRequests` nodes of a commit from the `getAssociatedPRs` + * response, loading the remaining pages for commits that have more than 100 of them. + */ +async function getAssociatedPRsOfCommit(octokit, owner, repo, commit) { + let page = commit.associatedPullRequests; + const nodes = [...page.nodes]; + while (page.pageInfo.hasNextPage) { + const { repository } = await octokit.graphql( + loadSingleCommitAssociatedPRs, + { owner, repo, sha: commit.oid, cursor: page.pageInfo.endCursor }, + ); + page = repository.commit.associatedPullRequests; + nodes.push(...page.nodes); + } + return nodes; +} + /** * Fields common accross PRs and Issue */ @@ -408,6 +408,28 @@ const baseFields = ` locked `; +/** + * Fields of a PR, on top of the `baseFields` + */ +const prFields = ` + ${baseFields} + repository { + nameWithOwner + } + mergeable + changedFiles + mergedAt + isDraft + mergedBy { + login + avatarUrl + url + } + commits { + totalCount + } +`; + /** * Builds GraphQL query for fetching PRs/Commits related Issues to a list of commit hash (sha) * @param {Array} numbers @@ -424,19 +446,7 @@ function buildRelatedIssuesQuery(numbers) { ${baseFields} } ... on PullRequest { - ${baseFields} - mergeable - changedFiles - mergedAt - isDraft - mergedBy { - login - avatarUrl - url - } - commits { - totalCount - } + ${prFields} } }`; }) @@ -466,19 +476,7 @@ function buildAssociatedPRsQuery(shas) { hasNextPage } nodes { - ${baseFields} - mergeable - changedFiles - mergedAt - isDraft - mergedBy { - login - avatarUrl - url - } - commits { - totalCount - } + ${prFields} } } } @@ -505,19 +503,7 @@ const loadSingleCommitAssociatedPRs = `#graphql hasNextPage } nodes { - ${baseFields} - mergeable - changedFiles - mergedAt - isDraft - mergedBy { - login - avatarUrl - url - } - commits { - totalCount - } + ${prFields} } } } diff --git a/test/success.test.js b/test/success.test.js index e88e1b70..040a6721 100644 --- a/test/success.test.js +++ b/test/success.test.js @@ -341,44 +341,57 @@ test("Add comment and labels to PRs associated with release commits and issues ( .getOnce(`https://api.github.local/repos/${owner}/${repo}`, { full_name: `${owner}/${repo}`, }) - .postOnce("https://api.github.local/graphql", { - data: { - repository: { - commit123: { - oid: "123", - associatedPullRequests: { - pageInfo: { - endCursor: "YE", - hasNextPage: true, + .postOnce( + (url, { body }) => + url === "https://api.github.local/graphql" && + JSON.parse(body).query.includes("query getAssociatedPRs("), + { + data: { + repository: { + commit123: { + oid: "123", + associatedPullRequests: { + pageInfo: { + endCursor: "YE", + hasNextPage: true, + }, + nodes: [prs[0]], }, - nodes: [prs[0]], }, - }, - commit456: { - oid: "456", - associatedPullRequests: { - pageInfo: { - endCursor: "NI", - hasNextPage: false, + commit456: { + oid: "456", + associatedPullRequests: { + pageInfo: { + endCursor: "NI", + hasNextPage: false, + }, + nodes: [prs[1]], }, - nodes: [prs[1]], }, - }, - commit789: { - oid: "789", - associatedPullRequests: { - pageInfo: { - endCursor: "NI", - hasNextPage: false, + commit789: { + oid: "789", + associatedPullRequests: { + pageInfo: { + endCursor: "NI", + hasNextPage: false, + }, + nodes: [prs[2]], }, - nodes: [prs[2]], }, }, }, }, - }) + ) .postOnce( - "https://api.github.local/graphql", + (url, { body }) => { + const { query, variables } = JSON.parse(body); + return ( + url === "https://api.github.local/graphql" && + query.includes("query getCommitAssociatedPRs(") && + variables.sha === "123" && + variables.cursor === "YE" + ); + }, { data: { repository: { @@ -394,9 +407,6 @@ test("Add comment and labels to PRs associated with release commits and issues ( }, }, }, - { - overwriteRoutes: true, - }, ) .postOnce( (url, { body }) => @@ -483,10 +493,49 @@ test("Add comment and labels to PRs associated with release commits and issues ( }, }, ) + .getOnce( + `https://api.github.local/repos/${owner}/${repo}/pulls/1/commits`, + [{ sha: commits[0].hash }], + ) + .getOnce( + `https://api.github.local/repos/${owner}/${repo}/pulls/2/commits`, + [{ sha: commits[1].hash }], + ) + .getOnce( + `https://api.github.local/repos/${owner}/${repo}/pulls/5/commits`, + [{ sha: commits[2].hash }], + ) .getOnce( `https://api.github.local/repos/${owner}/${repo}/pulls/6/commits`, [{ sha: commits[0].hash }], ) + .postOnce( + `https://api.github.local/repos/${owner}/${repo}/issues/1/comments`, + { html_url: "https://github.com/successcomment-1" }, + ) + .postOnce( + `https://api.github.local/repos/${owner}/${repo}/issues/1/labels`, + {}, + { body: ["released"] }, + ) + .postOnce( + `https://api.github.local/repos/${owner}/${repo}/issues/2/comments`, + { html_url: "https://github.com/successcomment-2" }, + ) + .postOnce( + `https://api.github.local/repos/${owner}/${repo}/issues/2/labels`, + {}, + { body: ["released"] }, + ) + .postOnce( + `https://api.github.local/repos/${owner}/${repo}/issues/5/comments`, + { html_url: "https://github.com/successcomment-5" }, + ) + .postOnce( + `https://api.github.local/repos/${owner}/${repo}/issues/5/labels`, + {}, + { body: ["released"] }, + ) .postOnce( `https://api.github.local/repos/${owner}/${repo}/issues/3/comments`, { html_url: "https://github.com/successcomment-3" }, @@ -567,16 +616,22 @@ test("Add comment and labels to PRs associated with release commits and issues ( t.true( t.context.log.calledWith("Added labels %O to issue #%d", ["released"], 4), ); - t.true( - t.context.log.calledWith( - "Added comment to PR #%d: %s", - 6, - "https://github.com/successcomment-6", - ), - ); - t.true( - t.context.log.calledWith("Added labels %O to PR #%d", ["released"], 6), - ); + for (const number of [1, 2, 5, 6]) { + t.true( + t.context.log.calledWith( + "Added comment to PR #%d: %s", + number, + `https://github.com/successcomment-${number}`, + ), + ); + t.true( + t.context.log.calledWith( + "Added labels %O to PR #%d", + ["released"], + number, + ), + ); + } t.true(fetch.done()); }); @@ -1306,6 +1361,123 @@ test("Do not add comment and labels for unrelated PR returned by search (compare t.true(fetch.done()); }); +test("Do not add comment and labels to associated PRs of other repositories", async (t) => { + const owner = "test_user"; + const repo = "test_repo"; + const env = { GITHUB_TOKEN: "github_token" }; + const failTitle = "The automated release is failing 🚨"; + const pluginConfig = { failTitle }; + const prs = [ + { + number: 1, + __typename: "PullRequest", + state: "closed", + repository: { nameWithOwner: `${owner}/${repo}` }, + }, + // A fork's PR that contains the release commit. Its number belongs to the + // fork, so no request must be made for it in this repository + { + number: 66, + __typename: "PullRequest", + state: "open", + repository: { nameWithOwner: "other_user/fork" }, + }, + ]; + const options = { + branch: "master", + repositoryUrl: `https://github.com/${owner}/${repo}.git`, + }; + const commits = [{ hash: "123", message: "Commit 1 message" }]; + const nextRelease = { version: "1.0.0" }; + const releases = [ + { name: "GitHub release", url: "https://github.com/release" }, + ]; + + const fetch = fetchMock + .sandbox() + .getOnce(`https://api.github.local/repos/${owner}/${repo}`, { + full_name: `${owner}/${repo}`, + }) + .postOnce( + (url, { body }) => + url === "https://api.github.local/graphql" && + JSON.parse(body).query.includes("query getAssociatedPRs("), + { + data: { + repository: { + commit123: { + oid: "123", + associatedPullRequests: { + pageInfo: { + endCursor: "NI", + hasNextPage: false, + }, + nodes: prs, + }, + }, + }, + }, + }, + ) + .getOnce( + `https://api.github.local/repos/${owner}/${repo}/pulls/1/commits`, + [{ sha: commits[0].hash }], + ) + .postOnce( + `https://api.github.local/repos/${owner}/${repo}/issues/1/comments`, + { html_url: "https://github.com/successcomment-1" }, + ) + .postOnce( + `https://api.github.local/repos/${owner}/${repo}/issues/1/labels`, + {}, + { body: ["released"] }, + ) + .postOnce( + (url, { body }) => { + t.is(url, "https://api.github.local/graphql"); + t.regex(JSON.parse(body).query, /query getSRIssues\(/); + return true; + }, + { + data: { + repository: { + issues: { nodes: [] }, + }, + }, + }, + ); + + await success( + pluginConfig, + { + env, + options, + commits, + nextRelease, + releases, + logger: t.context.logger, + }, + { + Octokit: TestOctokit.defaults((options) => ({ + ...options, + request: { ...options.request, fetch }, + })), + }, + ); + + t.true( + t.context.log.calledWith( + "Added comment to PR #%d: %s", + 1, + "https://github.com/successcomment-1", + ), + ); + t.true( + t.context.log.calledWith("Added labels %O to PR #%d", ["released"], 1), + ); + t.true(fetch.done()); +}); + test("Do not add comment and labels if no PR is associated with release commits", async (t) => { const owner = "test_user"; const repo = "test_repo"; From 8dc43ade45201dd019e4a739ee95cf5c3c996826 Mon Sep 17 00:00:00 2001 From: Phil Sturgeon <67381+philsturgeon@users.noreply.github.com> Date: Thu, 12 Nov 2026 17:35:20 +0000 Subject: [PATCH 2/2] fix(success): ignore related issues/PRs that do not exist (#1326) ## Problem If a commit message references an issue number that doesn't exist in the repository (e.g. `closes #3068` copied from another repo, which is common in forks and monorepo migrations), the `getRelatedIssues` GraphQL query returns partial `data` plus a `NOT_FOUND` error. `@octokit/graphql` throws on any `errors`, so the whole `success` step fails: ``` Error: Could not resolve to an issue or pull request with the number of 3068. type: 'NOT_FOUND', path: [ 'repository', 'issue3068' ] ``` This is inconsistent with the existing handling of missing issues when commenting (404 is logged and ignored, see "Ignore missing and forbidden issues/PRs"). ## Change - Catch the error when resolving related issues. If every error is `NOT_FOUND` and partial data is present, log the missing numbers and continue with the issues that resolved (null entries are filtered out). - Any other error is rethrown unchanged. - Added a test; it fails without the fix. `npm run lint:prettier` and `ava` (266 tests) pass. Co-authored-by: Olabode Lawal-Shittabey --- lib/success.js | 29 ++++++--- test/success.test.js | 136 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 158 insertions(+), 7 deletions(-) diff --git a/lib/success.js b/lib/success.js index 98f7ae0c..68abe094 100644 --- a/lib/success.js +++ b/lib/success.js @@ -155,13 +155,28 @@ export default async function success(pluginConfig, context, { Octokit }) { // Get relatedIssues (or relatedPRs i.e. Issues/PRs that are closed by an associatedPR) issues = await inChunks(uniqueParsedIssues, 100, async (chunk) => { - const { repository } = await octokit.graphql( - buildRelatedIssuesQuery(chunk.map((issue) => issue.number)), - { owner, repo }, - ); - const responseRelatedIssues = Object.values(repository).map( - (issue) => issue, - ); + let repository; + try { + ({ repository } = await octokit.graphql( + buildRelatedIssuesQuery(chunk.map((issue) => issue.number)), + { owner, repo }, + )); + } catch (error) { + // Referenced numbers that do not exist (e.g. "closes #123" copied from + // another repository) are reported as NOT_FOUND alongside partial data. + if ( + !error.data?.repository || + !error.errors?.every((e) => e.type === "NOT_FOUND") + ) { + throw error; + } + logger.log( + "Ignoring issues/PRs that do not exist: %s", + error.errors.map((e) => e.path?.[1]).join(", "), + ); + repository = error.data.repository; + } + const responseRelatedIssues = Object.values(repository).filter(Boolean); return buildIssuesOrPRsFromResponseNode(responseRelatedIssues); }); diff --git a/test/success.test.js b/test/success.test.js index 040a6721..a7b570f6 100644 --- a/test/success.test.js +++ b/test/success.test.js @@ -2099,6 +2099,142 @@ test("Ignore missing and forbidden issues/PRs", async (t) => { t.true(fetch.done()); }); +test("Ignore issues/PRs that do not exist when resolving related issues", async (t) => { + const owner = "test_user"; + const repo = "test_repo"; + const env = { GITHUB_TOKEN: "github_token" }; + const pluginConfig = {}; + const options = { + branch: "master", + repositoryUrl: `https://github.com/${owner}/${repo}.git`, + }; + const commits = [ + { hash: "123", message: "Commit 1 message\n\n Fix #4, #99" }, + ]; + const nextRelease = { version: "1.0.0" }; + const releases = [ + { name: "GitHub release", url: "https://github.com/release" }, + ]; + + const fetch = fetchMock + .sandbox() + .getOnce(`https://api.github.local/repos/${owner}/${repo}`, { + full_name: `${owner}/${repo}`, + clone_url: `https://api.github.local/${owner}/${repo}.git`, + }) + .postOnce( + (url, { body }) => + url === "https://api.github.local/graphql" && + JSON.parse(body).query.includes("query getAssociatedPRs("), + { + data: { + repository: { + commit123: { + oid: "123", + associatedPullRequests: { + pageInfo: { endCursor: "NI", hasNextPage: false }, + nodes: [], + }, + }, + }, + }, + }, + ) + .postOnce( + (url, { body }) => + url === "https://api.github.local/graphql" && + JSON.parse(body).query.includes("query getRelatedIssues("), + { + data: { + repository: { + issue4: { + __typename: "Issue", + id: "I_kw", + title: "issue title", + body: "", + url: "https://github.com/owner/repo/issues/4", + number: 4, + createdAt: "2024-07-13T09:58:09Z", + updatedAt: "2024-08-26T16:19:59Z", + closedAt: "2024-07-13T09:58:51Z", + comments: { totalCount: 0 }, + state: "CLOSED", + author: { + login: "user", + url: "author_url", + avatarUrl: "author_avatar_url", + __typename: "User", + }, + authorAssociation: "OWNER", + activeLockReason: null, + labels: { nodes: [] }, + milestone: null, + locked: false, + }, + issue99: null, + }, + }, + errors: [ + { + type: "NOT_FOUND", + path: ["repository", "issue99"], + locations: [{ line: 4, column: 9 }], + message: + "Could not resolve to an issue or pull request with the number of 99.", + }, + ], + }, + ) + .postOnce( + `https://api.github.local/repos/${owner}/${repo}/issues/4/comments`, + { html_url: "https://github.com/successcomment-4" }, + ) + .postOnce( + `https://api.github.local/repos/${owner}/${repo}/issues/4/labels`, + {}, + { body: ["released"] }, + ) + .postOnce( + (url, { body }) => + url === "https://api.github.local/graphql" && + JSON.parse(body).query.includes("query getSRIssues("), + { data: { repository: { issues: { nodes: [] } } } }, + ); + + await success( + pluginConfig, + { + env, + options, + commits, + nextRelease, + releases, + logger: t.context.logger, + }, + { + Octokit: TestOctokit.defaults((options) => ({ + ...options, + request: { ...options.request, fetch }, + })), + }, + ); + + t.true( + t.context.log.calledWith( + "Ignoring issues/PRs that do not exist: %s", + "issue99", + ), + ); + t.true( + t.context.log.calledWith( + "Added comment to issue #%d: %s", + 4, + "https://github.com/successcomment-4", + ), + ); + t.true(fetch.done()); +}); + test("Add custom comment and labels", async (t) => { const owner = "test_user"; const repo = "test_repo";