Skip to content

Fix add-reviewer cross-repo targeting and Copilot bot review requests #35326

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
May 28, 2026
Merged

Fix add-reviewer cross-repo targeting and Copilot bot review requests #35326

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
01a0545
Initial plan
Copilot May 27, 2026
bb35995
Fix add-reviewer target-repo and Copilot bot GraphQL path
Copilot May 27, 2026
6c02e6c
Resolve Copilot bot node ID at runtime for GHES compatibility
Copilot May 28, 2026
9e5a567
Apply style fixes from code review
Copilot May 28, 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
95 changes: 85 additions & 10 deletions actions/setup/js/add_reviewer.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
*/

/**
* @typedef {{ reviewers?: Array<string|null|undefined|false>, team_reviewers?: Array<string|null|undefined|false>, pull_request_number?: number|string }} AddReviewerMessage
* @typedef {{ reviewers?: Array<string|null|undefined|false>, team_reviewers?: Array<string|null|undefined|false>, pull_request_number?: number|string, repo?: string }} AddReviewerMessage
*/

/** @type {string} Safe output type handled by this module */
Expand All @@ -22,7 +22,8 @@ const { logStagedPreviewInfo } = require("./staged_preview.cjs");
const { isStagedMode, checkRequiredFilter } = require("./safe_output_helpers.cjs");
const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs");
const { attachExecutionState, extractReviewStateFromData, fetchPullRequestReviewState } = require("./safe_output_execution_metadata.cjs");
const { COPILOT_REVIEWER_BOT } = require("./constants.cjs");
const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs");
const { COPILOT_REVIEWER_BOT, COPILOT_REVIEWER_BOT_ID } = require("./constants.cjs");

/**
* Main handler factory for add_reviewer
Expand All @@ -33,6 +34,7 @@ async function main(config = {}) {
const allowedReviewers = config.allowed ?? [];
const allowedTeamReviewers = config.allowed_team_reviewers ?? [];
const maxCount = config.max ?? 10;
const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config);
const githubClient = await createAuthenticatedGitHubClient(config);
const isStaged = isStagedMode(config);

Expand All @@ -42,13 +44,45 @@ async function main(config = {}) {
if (requiredTitlePrefix) core.info(`Required title prefix: ${requiredTitlePrefix}`);

core.info(`Add reviewer configuration: max=${maxCount}`);
core.info(`Default target repo: ${defaultTargetRepo}`);
if (allowedRepos.size > 0) {
core.info(`Allowed repos: ${Array.from(allowedRepos).join(", ")}`);
}
if (allowedReviewers.length > 0) {
core.info(`Allowed reviewers: ${allowedReviewers.join(", ")}`);
}
if (allowedTeamReviewers.length > 0) {
core.info(`Allowed team reviewers: ${allowedTeamReviewers.join(", ")}`);
}

/** @type {string|null} Copilot reviewer bot node ID, resolved once and cached per handler instance */
let copilotBotNodeIdCache = null;

/**
* Resolves the Copilot reviewer bot's GraphQL node ID for the current GitHub instance.
* Uses the REST users API so the result is correct on GitHub.com and GHES alike.
* Caches the resolved ID for the lifetime of this handler to avoid redundant requests.
* Falls back to the built-in GitHub.com constant when the API call fails.
* @returns {Promise<string>} GraphQL node ID for the Copilot reviewer bot
*/
async function resolveCopilotBotNodeId() {
if (copilotBotNodeIdCache !== null) {
return copilotBotNodeIdCache;
}
try {
const response = await githubClient.rest.users.getByUsername({ username: COPILOT_REVIEWER_BOT });
const nodeId = response?.data?.node_id;
if (nodeId) {
copilotBotNodeIdCache = nodeId;
return nodeId;
}
} catch (err) {
core.warning(`Could not resolve Copilot reviewer bot node ID at runtime (${getErrorMessage(err)}); using built-in fallback`);
}
copilotBotNodeIdCache = COPILOT_REVIEWER_BOT_ID;
return copilotBotNodeIdCache;
}

let processedCount = 0;

/**
Expand Down Expand Up @@ -83,8 +117,17 @@ async function main(config = {}) {
};
}

const repoParts = { owner: context.repo.owner, repo: context.repo.repo };
const itemRepo = `${repoParts.owner}/${repoParts.repo}`;
const repoResult = resolveAndValidateRepo(message, defaultTargetRepo, allowedRepos, "pull request reviewer");
if (!repoResult.success) {
core.warning(`Skipping add_reviewer: ${repoResult.error}`);
return {
success: false,
error: repoResult.error,
};
}
const { repo: itemRepo, repoParts } = repoResult;
core.info(`Target repository: ${itemRepo}`);

const filterResult = await checkRequiredFilter(githubClient, repoParts, prNumber, requiredLabels, requiredTitlePrefix, HANDLER_TYPE);
if (filterResult) return filterResult;

Expand Down Expand Up @@ -138,8 +181,8 @@ async function main(config = {}) {
if (otherReviewers.length > 0 || uniqueTeamReviewers.length > 0) {
/** @type {{ owner: string, repo: string, pull_number: number, reviewers: string[], team_reviewers?: string[] }} */
const reviewerRequest = {
owner: context.repo.owner,
repo: context.repo.repo,
owner: repoParts.owner,
repo: repoParts.repo,
pull_number: prNumber,
reviewers: otherReviewers,
};
Expand All @@ -154,11 +197,43 @@ async function main(config = {}) {
// Add copilot reviewer separately if requested
if (hasCopilot) {
try {
const response = await githubClient.rest.pulls.requestReviewers({
owner: context.repo.owner,
repo: context.repo.repo,
const pullRequestQuery = `
query($owner: String!, $repo: String!, $number: Int!) {
repository(owner: $owner, name: $repo) {
pullRequest(number: $number) {
id
}
}
}
`;
const pullRequestResponse = await githubClient.graphql(pullRequestQuery, {
owner: repoParts.owner,
repo: repoParts.repo,

@github-actions github-actions Bot May 28, 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.

[/tdd] The PR node ID lookup failure path (pullRequestId is null/undefined) is not covered by a test — only the mutation rejection is tested.

💡 Suggested test 
it("should handle missing PR node ID gracefully", async () => {
  mockGithub.graphql.mockResolvedValueOnce({ repository: { pullRequest: null } });
  const message = { type: "add_reviewer", reviewers: ["copilot"] };
  const result = await handler(message, {});
  expect(result.success).toBe(true); // or false depending on intended behaviour
  expect(mockGithub.graphql).toHaveBeenCalledTimes(1); // mutation never called
});

Right now, a null PR node ID throws synchronously inside the try/catch, so it is handled — but there is no test pinning this behaviour, leaving it vulnerable to silent regression.

number: prNumber,
});
const pullRequestId = pullRequestResponse?.repository?.pullRequest?.id;
if (!pullRequestId) {
throw new Error(`Could not resolve pull request node ID for ${repoParts.owner}/${repoParts.repo}#${prNumber}`);
}

const requestReviewsMutation = `
mutation($pullRequestId: ID!, $botIds: [ID!]!) {
requestReviews(input: { pullRequestId: $pullRequestId, botIds: $botIds, union: true }) {
pullRequest {
id
}
}
}
`;

@github-actions github-actions Bot May 28, 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 requestReviews mutation result is silently discarded — there is no confirmation that Copilot was actually assigned as a reviewer.

💡 Details and suggested fix

After await githubClient.graphql(requestReviewsMutation, ...), the code immediately calls pulls.get to refresh latestPullRequest but never inspects the mutation response to verify the reviewer was added. GitHub's GraphQL API returns the mutated pullRequest object; if the bot ID is wrong, Copilot is not enabled on the repo, or the PR is in a state that disallows it, the mutation succeeds with an empty reviewer list and the caller logs "Successfully added copilot as reviewer" regardless.

Suggested fix: inspect the mutation response:

const mutationResult = await githubClient.graphql(requestReviewsMutation, {
  pullRequestId,
  botIds: [COPILOT_REVIEWER_BOT_ID],
});
// Validate the reviewer actually appears in the response, or at minimum check for errors
  throw new Error(`requestReviews mutation returned unexpected response for PR #${prNumber}`);
}

await githubClient.graphql(requestReviewsMutation, {
pullRequestId,
botIds: [await resolveCopilotBotNodeId()],
});

const response = await githubClient.rest.pulls.get({
owner: repoParts.owner,
repo: repoParts.repo,
pull_number: prNumber,
reviewers: [COPILOT_REVIEWER_BOT],
});
latestPullRequest = response?.data || latestPullRequest;

@github-actions github-actions Bot May 28, 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.

When Copilot is the only requested reviewer and the mutation fails, the catch block logs a warning but the function still returns success: true with reviewersAdded containing "copilot".

💡 Details and suggested fix

The outer catch around the Copilot path calls core.warning and falls through, so the final return includes copilot in reviewersAdded despite the failure. For callers that requested only Copilot review and no other reviewers, the net result is zero reviewers added but a success response.

The existing test "should handle copilot reviewer failure gracefully" asserts result.success === true — which confirms the current intent — but this is likely wrong behavior if Copilot was the only reviewer requested.

Suggested fix: track whether the Copilot assignment actually succeeded and exclude it from reviewersAdded (and potentially flip success) on failure:

let copilotAdded = false;
try {
  // ... graphql calls ...
  copilotAdded = true;
} catch (err) {
  core.warning(`Failed to add copilot reviewer: ${err.message}`);
}
if (copilotAdded) reviewersAdded.push("copilot");

core.info(`Successfully added copilot as reviewer to PR #${prNumber}`);
Expand Down
90 changes: 68 additions & 22 deletions actions/setup/js/add_reviewer.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const mockCore = {
};

const mockGithub = {
graphql: vi.fn().mockResolvedValue({}),
rest: {
pulls: {
get: vi.fn().mockResolvedValue({
Expand All @@ -27,6 +28,9 @@ const mockGithub = {
listReviews: vi.fn().mockResolvedValue({ data: [{ id: 1, user: { login: "reviewer-a" }, state: "COMMENTED" }] }),
requestReviewers: vi.fn().mockResolvedValue({}),
},
users: {
getByUsername: vi.fn().mockResolvedValue({ data: { node_id: "BOT_kgDOCnlnWA" } }),
},
},
};

Expand Down Expand Up @@ -122,6 +126,8 @@ describe("add_reviewer (Handler Factory Architecture)", () => {
});

it("should add copilot reviewer separately", async () => {
mockGithub.graphql.mockResolvedValueOnce({ repository: { pullRequest: { id: "PR_NODE_ID" } } }).mockResolvedValueOnce({ requestReviews: { pullRequest: { id: "PR_NODE_ID" } } });

const message = {
type: "add_reviewer",
reviewers: ["user1", "copilot"],
Expand All @@ -131,23 +137,52 @@ describe("add_reviewer (Handler Factory Architecture)", () => {

expect(result.success).toBe(true);
expect(result.reviewersAdded).toEqual(["user1", "copilot"]);
// Should be called twice - once for regular reviewers, once for copilot
expect(mockGithub.rest.pulls.requestReviewers).toHaveBeenCalledTimes(2);
expect(mockGithub.rest.pulls.requestReviewers).toHaveBeenCalledTimes(1);
expect(mockGithub.rest.pulls.requestReviewers).toHaveBeenCalledWith({
owner: "testowner",
repo: "testrepo",
pull_number: 123,
reviewers: ["user1"],
});
expect(mockGithub.rest.pulls.requestReviewers).toHaveBeenCalledWith({
owner: "testowner",
repo: "testrepo",
pull_number: 123,
reviewers: ["copilot-pull-request-reviewer[bot]"],
});
expect(mockGithub.rest.users.getByUsername).toHaveBeenCalledWith({ username: "copilot-pull-request-reviewer[bot]" });
expect(mockGithub.graphql).toHaveBeenNthCalledWith(1, expect.stringContaining("pullRequest(number: $number)"), expect.objectContaining({ owner: "testowner", repo: "testrepo", number: 123 }));
expect(mockGithub.graphql).toHaveBeenNthCalledWith(2, expect.stringContaining("requestReviews(input"), expect.objectContaining({ pullRequestId: "PR_NODE_ID", botIds: ["BOT_kgDOCnlnWA"] }));
});

it("should fall back to built-in node ID when users API fails", async () => {
mockGithub.rest.users.getByUsername.mockRejectedValueOnce(new Error("Not Found"));
mockGithub.graphql.mockResolvedValueOnce({ repository: { pullRequest: { id: "PR_NODE_ID" } } }).mockResolvedValueOnce({ requestReviews: { pullRequest: { id: "PR_NODE_ID" } } });

const { main } = require("./add_reviewer.cjs");
const fallbackHandler = await main({ max: 10, allowed: ["copilot"] });

const result = await fallbackHandler({ type: "add_reviewer", reviewers: ["copilot"] }, {});

expect(result.success).toBe(true);
expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Could not resolve Copilot reviewer bot node ID"));
expect(mockGithub.graphql).toHaveBeenCalledWith(expect.stringContaining("requestReviews(input"), expect.objectContaining({ botIds: ["BOT_kgDOCnlnWA"] }));
});

it("should cache the resolved bot node ID across calls", async () => {
mockGithub.graphql
.mockResolvedValueOnce({ repository: { pullRequest: { id: "PR_NODE_ID_1" } } })
.mockResolvedValueOnce({ requestReviews: { pullRequest: { id: "PR_NODE_ID_1" } } })
.mockResolvedValueOnce({ repository: { pullRequest: { id: "PR_NODE_ID_2" } } })
.mockResolvedValueOnce({ requestReviews: { pullRequest: { id: "PR_NODE_ID_2" } } });

const { main } = require("./add_reviewer.cjs");
const cachingHandler = await main({ max: 10, allowed: ["copilot"] });

await cachingHandler({ type: "add_reviewer", reviewers: ["copilot"] }, {});
await cachingHandler({ type: "add_reviewer", reviewers: ["copilot"] }, {});

// users.getByUsername should only be called once despite two copilot reviewer requests
expect(mockGithub.rest.users.getByUsername).toHaveBeenCalledTimes(1);
});

it("should keep team reviewers with non-copilot reviewers when copilot is requested", async () => {
mockGithub.graphql.mockResolvedValueOnce({ repository: { pullRequest: { id: "PR_NODE_ID" } } }).mockResolvedValueOnce({ requestReviews: { pullRequest: { id: "PR_NODE_ID" } } });

const message = {
type: "add_reviewer",
reviewers: ["user1", "copilot"],
Expand All @@ -166,12 +201,7 @@ describe("add_reviewer (Handler Factory Architecture)", () => {
reviewers: ["user1"],
team_reviewers: ["platform-team"],
});
expect(mockGithub.rest.pulls.requestReviewers).toHaveBeenNthCalledWith(2, {
owner: "testowner",
repo: "testrepo",
pull_number: 123,
reviewers: ["copilot-pull-request-reviewer[bot]"],
});
expect(mockGithub.graphql).toHaveBeenCalledTimes(2);
});

it("should filter by allowed reviewers", async () => {
Expand Down Expand Up @@ -390,9 +420,8 @@ describe("add_reviewer (Handler Factory Architecture)", () => {
});

it("should handle copilot reviewer failure gracefully", async () => {
mockGithub.rest.pulls.requestReviewers
.mockResolvedValueOnce({}) // regular reviewers succeed
.mockRejectedValueOnce(new Error("Copilot not available")); // copilot fails
mockGithub.rest.pulls.requestReviewers.mockResolvedValueOnce({});
mockGithub.graphql.mockResolvedValueOnce({ repository: { pullRequest: { id: "PR_NODE_ID" } } }).mockRejectedValueOnce(new Error("Copilot not available"));

const message = {
type: "add_reviewer",
Expand Down Expand Up @@ -422,6 +451,8 @@ describe("add_reviewer (Handler Factory Architecture)", () => {
});

it("should add only copilot when copilot is the only reviewer", async () => {
mockGithub.graphql.mockResolvedValueOnce({ repository: { pullRequest: { id: "PR_NODE_ID" } } }).mockResolvedValueOnce({ requestReviews: { pullRequest: { id: "PR_NODE_ID" } } });

const message = {
type: "add_reviewer",
reviewers: ["copilot"],
Expand All @@ -431,13 +462,28 @@ describe("add_reviewer (Handler Factory Architecture)", () => {

expect(result.success).toBe(true);
expect(result.reviewersAdded).toEqual(["copilot"]);
// Only called once for copilot, not for other reviewers
expect(mockGithub.rest.pulls.requestReviewers).toHaveBeenCalledTimes(1);
expect(mockGithub.rest.pulls.requestReviewers).not.toHaveBeenCalled();
expect(mockGithub.graphql).toHaveBeenCalledTimes(2);
});

it("should honor target-repo for cross-repository reviewer requests", async () => {
const { main } = require("./add_reviewer.cjs");
const crossRepoHandler = await main({ max: 10, allowed: ["user1"], "target-repo": "microsoft/vscode", allowed_repos: ["microsoft/vscode"] });

const message = {
type: "add_reviewer",
reviewers: ["user1"],
};

const result = await crossRepoHandler(message, {});

expect(result.success).toBe(true);
expect(result.repo).toBe("microsoft/vscode");
expect(mockGithub.rest.pulls.requestReviewers).toHaveBeenCalledWith({
owner: "testowner",
repo: "testrepo",
owner: "microsoft",
repo: "vscode",
pull_number: 123,
reviewers: ["copilot-pull-request-reviewer[bot]"],
reviewers: ["user1"],
});
});

Expand Down
10 changes: 10 additions & 0 deletions actions/setup/js/constants.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,15 @@ const TMP_GH_AW_PATH = "/tmp/gh-aw";
*/
const COPILOT_REVIEWER_BOT = "copilot-pull-request-reviewer[bot]";

/**
* GitHub.com GraphQL node ID for the Copilot pull request reviewer bot.
* Used as a fallback when the node ID cannot be resolved at runtime (e.g. network error).
* For GHES and other GitHub instances the node ID differs; prefer runtime resolution via
* the REST users API ({@link https://docs.github.com/en/rest/users/users#get-a-user}).
* @type {string}
*/
const COPILOT_REVIEWER_BOT_ID = "BOT_kgDOCnlnWA";
Comment on lines +34 to +41

@github-actions github-actions Bot May 28, 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.

Hardcoded bot node ID with no provenance will silently break on GHES or if GitHub reassigns this bot account.

💡 Details and suggested fix

COPILOT_REVIEWER_BOT_ID = "BOT_kgDOCnlnWA" is a global-node-ID string with zero documentation of where it came from, how to re-derive it, or whether it varies across GitHub.com vs GHES instances.

If GitHub ever rotates the bot's node ID, or if this constant differs per deployment, the requestReviews mutation will silently accept botIds containing an unrecognized ID and add no reviewer — the caller sees success: true with no feedback.

Minimum fix: add a comment with the derivation command and a stability note, e.g.:

// Derived via: gh api graphql -f query='{ user(login: copilot-pull-request-reviewer[bot]) { id } }'
// This is a stable global ID on GitHub.com; GHES installations may differ.
// Re-derive if the Copilot reviewer bot is replaced or renamed.
const COPILOT_REVIEWER_BOT_ID = "BOT_kgDOCnlnWA";

Better fix: resolve the ID at runtime via a GraphQL user lookup so GHES and future bot renames are handled automatically.


// ---------------------------------------------------------------------------
// Documentation URLs
// ---------------------------------------------------------------------------
Expand Down Expand Up @@ -125,6 +134,7 @@ module.exports = {
AGENT_OUTPUT_FILENAME,
TMP_GH_AW_PATH,
COPILOT_REVIEWER_BOT,
COPILOT_REVIEWER_BOT_ID,
FAQ_CREATE_PR_PERMISSIONS_URL,
MAX_LABELS,
MAX_ASSIGNEES,
Expand Down
6 changes: 6 additions & 0 deletions actions/setup/js/constants.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ const {
AGENT_OUTPUT_FILENAME,
TMP_GH_AW_PATH,
COPILOT_REVIEWER_BOT,
COPILOT_REVIEWER_BOT_ID,
FAQ_CREATE_PR_PERMISSIONS_URL,
MAX_LABELS,
MAX_ASSIGNEES,
Expand Down Expand Up @@ -55,6 +56,10 @@ describe("constants", () => {
it("should export COPILOT_REVIEWER_BOT", () => {
expect(COPILOT_REVIEWER_BOT).toBe("copilot-pull-request-reviewer[bot]");
});

it("should export COPILOT_REVIEWER_BOT_ID", () => {
expect(COPILOT_REVIEWER_BOT_ID).toBe("BOT_kgDOCnlnWA");
});
});

describe("documentation URLs", () => {
Expand Down Expand Up @@ -85,6 +90,7 @@ describe("constants", () => {
"AGENT_OUTPUT_FILENAME",
"TMP_GH_AW_PATH",
"COPILOT_REVIEWER_BOT",
"COPILOT_REVIEWER_BOT_ID",
"FAQ_CREATE_PR_PERMISSIONS_URL",
"MAX_LABELS",
"MAX_ASSIGNEES",
Expand Down
Loading