Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
32 changes: 24 additions & 8 deletions actions/setup/js/add_reviewer.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
*/

/**
* @typedef {{ 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 }} AddReviewerMessage
*/

/** @type {string} Safe output type handled by this module */
Expand All @@ -30,6 +30,7 @@ const { COPILOT_REVIEWER_BOT } = require("./constants.cjs");
*/
async function main(config = {}) {
const allowedReviewers = config.allowed ?? [];
const allowedTeamReviewers = config.allowed_team_reviewers ?? [];
const maxCount = config.max ?? 10;
const githubClient = await createAuthenticatedGitHubClient(config);
const isStaged = isStagedMode(config);
Expand All @@ -38,6 +39,9 @@ async function main(config = {}) {
if (allowedReviewers.length > 0) {
core.info(`Allowed reviewers: ${allowedReviewers.join(", ")}`);
}
if (allowedTeamReviewers.length > 0) {
core.info(`Allowed team reviewers: ${allowedTeamReviewers.join(", ")}`);
}

let processedCount = 0;

Expand Down Expand Up @@ -68,22 +72,27 @@ async function main(config = {}) {
}

const requestedReviewers = message.reviewers ?? [];
const requestedTeamReviewers = message.team_reviewers ?? [];
core.info(`Requested reviewers: ${JSON.stringify(requestedReviewers)}`);
core.info(`Requested team reviewers: ${JSON.stringify(requestedTeamReviewers)}`);

// Use shared helper to filter, sanitize, dedupe, and limit
// Use shared helper to filter, sanitize, dedupe, and limit across both reviewer types
const uniqueReviewers = processItems(requestedReviewers, allowedReviewers, maxCount);
const remainingReviewerSlots = Math.max(0, maxCount - uniqueReviewers.length);
const uniqueTeamReviewers = processItems(requestedTeamReviewers, allowedTeamReviewers, remainingReviewerSlots);

if (uniqueReviewers.length === 0) {
if (uniqueReviewers.length === 0 && uniqueTeamReviewers.length === 0) {
core.info("No reviewers to add");
return {
success: true,
prNumber,
reviewersAdded: [],
teamReviewersAdded: [],
message: "No valid reviewers found",
};
}

core.info(`Adding ${uniqueReviewers.length} reviewers to PR #${prNumber}: ${JSON.stringify(uniqueReviewers)}`);
core.info(`Adding reviewers to PR #${prNumber}: reviewers=${JSON.stringify(uniqueReviewers)}, team_reviewers=${JSON.stringify(uniqueTeamReviewers)}`);

// If in staged mode, preview without executing
if (isStaged) {
Expand All @@ -94,6 +103,7 @@ async function main(config = {}) {
previewInfo: {
number: prNumber,
reviewers: uniqueReviewers,
team_reviewers: uniqueTeamReviewers,
},
};
}
Expand All @@ -104,14 +114,19 @@ async function main(config = {}) {
const otherReviewers = uniqueReviewers.filter(r => r !== "copilot");

// Add non-copilot reviewers first
if (otherReviewers.length > 0) {
await githubClient.rest.pulls.requestReviewers({
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,
pull_number: prNumber,
reviewers: otherReviewers,
});
core.info(`Successfully added ${otherReviewers.length} reviewer(s) to PR #${prNumber}`);
};
if (uniqueTeamReviewers.length > 0) {
reviewerRequest.team_reviewers = uniqueTeamReviewers;
}
await githubClient.rest.pulls.requestReviewers(reviewerRequest);
core.info(`Successfully added reviewers to PR #${prNumber}: reviewers=${JSON.stringify(otherReviewers)}, team_reviewers=${JSON.stringify(uniqueTeamReviewers)}`);
}

// Add copilot reviewer separately if requested
Expand All @@ -134,6 +149,7 @@ async function main(config = {}) {
success: true,
prNumber,
reviewersAdded: uniqueReviewers,
teamReviewersAdded: uniqueTeamReviewers,
};
} catch (error) {
const errorMessage = getErrorMessage(error);
Expand Down
91 changes: 91 additions & 0 deletions actions/setup/js/add_reviewer.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,25 @@ describe("add_reviewer (Handler Factory Architecture)", () => {
});
});

it("should add team reviewers to PR", async () => {
const message = {
type: "add_reviewer",
team_reviewers: ["platform-team", "security-team"],
};

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

expect(result.success).toBe(true);
expect(result.teamReviewersAdded).toEqual(["platform-team", "security-team"]);
expect(mockGithub.rest.pulls.requestReviewers).toHaveBeenCalledWith({
owner: "testowner",
repo: "testrepo",
pull_number: 123,
reviewers: [],
team_reviewers: ["platform-team", "security-team"],
});
});

it("should add copilot reviewer separately", async () => {
const message = {
type: "add_reviewer",
Expand All @@ -110,6 +129,33 @@ describe("add_reviewer (Handler Factory Architecture)", () => {
});
});

it("should keep team reviewers with non-copilot reviewers when copilot is requested", async () => {
const message = {
type: "add_reviewer",
reviewers: ["user1", "copilot"],
team_reviewers: ["platform-team"],
};

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

expect(result.success).toBe(true);
expect(result.reviewersAdded).toEqual(["user1", "copilot"]);
expect(result.teamReviewersAdded).toEqual(["platform-team"]);
expect(mockGithub.rest.pulls.requestReviewers).toHaveBeenNthCalledWith(1, {
owner: "testowner",
repo: "testrepo",
pull_number: 123,
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]"],
});
});

it("should filter by allowed reviewers", async () => {
const message = {
type: "add_reviewer",
Expand All @@ -128,6 +174,28 @@ describe("add_reviewer (Handler Factory Architecture)", () => {
});
});

it("should filter by allowed team reviewers", async () => {
const { main } = require("./add_reviewer.cjs");
const restrictedHandler = await main({ max: 10, allowed_team_reviewers: ["platform-team"] });

const message = {
type: "add_reviewer",
team_reviewers: ["platform-team", "security-team"],
};

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

expect(result.success).toBe(true);
expect(result.teamReviewersAdded).toEqual(["platform-team"]);
expect(mockGithub.rest.pulls.requestReviewers).toHaveBeenCalledWith({
owner: "testowner",
repo: "testrepo",
pull_number: 123,
reviewers: [],
team_reviewers: ["platform-team"],
});
});

it("should enforce max count limit", async () => {
const { main } = require("./add_reviewer.cjs");
const limitedHandler = await main({ max: 1, allowed: ["user1", "user2"] });
Expand Down Expand Up @@ -431,6 +499,29 @@ describe("add_reviewer (Handler Factory Architecture)", () => {
});
});

it("should enforce maxCount across reviewers and team_reviewers combined", async () => {
const { main } = require("./add_reviewer.cjs");
const tightHandler = await main({ max: 3, allowed: [], allowed_team_reviewers: [] });

const message = {
type: "add_reviewer",
reviewers: ["user1", "user2", "user3"],
team_reviewers: ["platform-team", "security-team"],
};

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

expect(result.success).toBe(true);
expect(result.reviewersAdded).toEqual(["user1", "user2", "user3"]);
expect(result.teamReviewersAdded).toEqual([]);
expect(mockGithub.rest.pulls.requestReviewers).toHaveBeenCalledWith({
owner: "testowner",
repo: "testrepo",
pull_number: 123,
reviewers: ["user1", "user2", "user3"],
});
});

it("should count staged calls toward max processedCount", async () => {
const originalEnv = process.env.GH_AW_SAFE_OUTPUTS_STAGED;
process.env.GH_AW_SAFE_OUTPUTS_STAGED = "true";
Expand Down
21 changes: 15 additions & 6 deletions actions/setup/js/create_pull_request.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,7 @@ async function main(config = {}) {
const titlePrefix = config.title_prefix || "";
const envLabels = config.labels ? (Array.isArray(config.labels) ? config.labels : config.labels.split(",")).map(label => String(label).trim()).filter(label => label) : [];
const configReviewers = config.reviewers ? (Array.isArray(config.reviewers) ? config.reviewers : config.reviewers.split(",")).map(r => String(r).trim()).filter(r => r) : [];
const configTeamReviewers = config.team_reviewers ? (Array.isArray(config.team_reviewers) ? config.team_reviewers : config.team_reviewers.split(",")).map(r => String(r).trim()).filter(r => r) : [];
const rawAssignees = config.assignees ? (Array.isArray(config.assignees) ? config.assignees : config.assignees.split(",")).map(a => String(a).trim()).filter(a => a) : [];
const hasCopilotInAssignees = rawAssignees.some(a => a.toLowerCase() === "copilot");
const configAssignees = sanitizeFallbackAssignees(rawAssignees);
Expand Down Expand Up @@ -356,6 +357,9 @@ async function main(config = {}) {
if (configReviewers.length > 0) {
core.info(`Configured reviewers: ${configReviewers.join(", ")}`);
}
if (configTeamReviewers.length > 0) {
core.info(`Configured team reviewers: ${configTeamReviewers.join(", ")}`);
}
if (configAssignees && configAssignees.length > 0) {
core.info(`Configured assignees (for fallback issues): ${configAssignees.join(", ")}`);
}
Expand Down Expand Up @@ -1448,20 +1452,25 @@ ${patchPreview}`;
}

// Add configured reviewers if specified
if (configReviewers.length > 0) {
if (configReviewers.length > 0 || configTeamReviewers.length > 0) {
const hasCopilot = configReviewers.includes("copilot");
const otherReviewers = configReviewers.filter(r => r !== "copilot");

if (otherReviewers.length > 0) {
core.info(`Requesting ${otherReviewers.length} reviewer(s) for pull request #${pullRequest.number}: ${JSON.stringify(otherReviewers)}`);
if (otherReviewers.length > 0 || configTeamReviewers.length > 0) {
core.info(`Requesting reviewers for pull request #${pullRequest.number}: reviewers=${JSON.stringify(otherReviewers)}, team_reviewers=${JSON.stringify(configTeamReviewers)}`);
try {
await githubClient.rest.pulls.requestReviewers({
/** @type {{ owner: string, repo: string, pull_number: number, reviewers: string[], team_reviewers?: string[] }} */
const reviewerRequest = {
owner: repoParts.owner,
repo: repoParts.repo,
pull_number: pullRequest.number,
reviewers: otherReviewers,
});
core.info(`Requested reviewers for pull request #${pullRequest.number}: ${JSON.stringify(otherReviewers)}`);
};
if (configTeamReviewers.length > 0) {
reviewerRequest.team_reviewers = configTeamReviewers;
}
await githubClient.rest.pulls.requestReviewers(reviewerRequest);
core.info(`Requested reviewers for pull request #${pullRequest.number}: reviewers=${JSON.stringify(otherReviewers)}, team_reviewers=${JSON.stringify(configTeamReviewers)}`);
} catch (reviewerError) {
core.warning(`Failed to request reviewers for PR #${pullRequest.number}: ${reviewerError instanceof Error ? reviewerError.message : String(reviewerError)}`);
}
Expand Down
30 changes: 30 additions & 0 deletions actions/setup/js/create_pull_request.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -999,6 +999,24 @@ describe("create_pull_request - configured reviewers", () => {
);
});

it("should request configured team reviewers after creating the PR", async () => {
const { main } = require("./create_pull_request.cjs");
const handler = await main({ team_reviewers: ["platform-team"], allow_empty: true });

const result = await handler({ title: "Test PR", body: "Test body" }, {});

expect(result.success).toBe(true);
expect(global.github.rest.pulls.requestReviewers).toHaveBeenCalledWith(
expect.objectContaining({
owner: "test-owner",
repo: "test-repo",
pull_number: 42,
reviewers: [],
team_reviewers: ["platform-team"],
})
);
});

it("should handle copilot reviewer separately from regular reviewers", async () => {
const { main } = require("./create_pull_request.cjs");
const handler = await main({ reviewers: ["user1", "copilot"], allow_empty: true });
Expand All @@ -1012,6 +1030,18 @@ describe("create_pull_request - configured reviewers", () => {
expect(global.github.rest.pulls.requestReviewers).toHaveBeenCalledWith(expect.objectContaining({ reviewers: ["copilot-pull-request-reviewer[bot]"] }));
});

it("should keep configured team reviewers with non-copilot reviewers when copilot is configured", async () => {
const { main } = require("./create_pull_request.cjs");
const handler = await main({ reviewers: ["user1", "copilot"], team_reviewers: ["platform-team"], allow_empty: true });

const result = await handler({ title: "Test PR", body: "Test body" }, {});

expect(result.success).toBe(true);
expect(global.github.rest.pulls.requestReviewers).toHaveBeenCalledTimes(2);
expect(global.github.rest.pulls.requestReviewers).toHaveBeenNthCalledWith(1, expect.objectContaining({ reviewers: ["user1"], team_reviewers: ["platform-team"] }));
expect(global.github.rest.pulls.requestReviewers).toHaveBeenNthCalledWith(2, expect.objectContaining({ reviewers: ["copilot-pull-request-reviewer[bot]"] }));
});

it("should not call requestReviewers when no reviewers are configured", async () => {
const { main } = require("./create_pull_request.cjs");
const handler = await main({ allow_empty: true });
Expand Down
2 changes: 2 additions & 0 deletions actions/setup/js/types/safe-outputs-config.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ interface CreatePullRequestConfig extends SafeOutputConfig {
"title-prefix"?: string;
labels?: string[];
reviewers?: string | string[];
"team-reviewers"?: string | string[];
assignees?: string | string[];
draft?: boolean;
"if-no-changes"?: string;
Expand Down Expand Up @@ -161,6 +162,7 @@ interface AddLabelsConfig extends SafeOutputConfig {
*/
interface AddReviewerConfig extends SafeOutputConfig {
reviewers?: string[];
"team-reviewers"?: string[];
target?: string;
}

Expand Down
Loading