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
67 changes: 42 additions & 25 deletions actions/setup/js/create_pull_request.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ const { checkFileProtection } = require("./manifest_file_helpers.cjs");
const { renderTemplateFromFile, buildProtectedFileList, encodePathSegments, getPromptPath } = require("./messages_core.cjs");
const { COPILOT_REVIEWER_BOT, FAQ_CREATE_PR_PERMISSIONS_URL, MAX_ASSIGNEES } = require("./constants.cjs");
const { isStagedMode } = require("./safe_output_helpers.cjs");
const { withRetry, isTransientError } = require("./error_recovery.cjs");
const { withRetry, isTransientError, RATE_LIMIT_RETRY_CONFIG } = require("./error_recovery.cjs");
const { tryEnforceArrayLimit } = require("./limit_enforcement_helpers.cjs");
const { findAgent, getIssueDetails, assignAgentToIssue } = require("./assign_agent_helpers.cjs");
const { globPatternToRegex } = require("./glob_pattern_helpers.cjs");
Expand Down Expand Up @@ -195,9 +195,11 @@ function sanitizeFallbackAssignees(assignees) {
}

/**
* Creates a fallback GitHub issue, retrying without assignees if the API rejects them.
* Creates a fallback GitHub issue, retrying on rate-limit and other transient errors
* (with exponential back-off) and retrying without assignees if the API rejects them.
* This ensures fallback issue creation remains reliable even if an assignee username
* is invalid or the repository does not have that collaborator.
* is invalid, the repository does not have that collaborator, or the installation token
* quota is temporarily exhausted.
* @param {object} githubClient - Authenticated GitHub client
* @param {{owner: string, repo: string}} repoParts - Repository owner and name
* @param {string} title - Issue title
Expand All @@ -216,19 +218,29 @@ async function createFallbackIssue(githubClient, repoParts, title, body, labels,
...(assignees && assignees.length > 0 && { assignees }),
};

try {
return await githubClient.rest.issues.create(payload);
} catch (error) {
const status = typeof error === "object" && error !== null && "status" in error ? error.status : undefined;
const message = getErrorMessage(error).toLowerCase();
const isAssigneeError = status === 422 && (message.includes("assignee") || message.includes("assignees") || message.includes("unprocessable"));
if (isAssigneeError && assignees && assignees.length > 0) {
core.warning(`Fallback issue creation failed due to assignee error, retrying without assignees: ${getErrorMessage(error)}`);
const { assignees: _removed, ...payloadWithoutAssignees } = payload;
return await githubClient.rest.issues.create(payloadWithoutAssignees);
}
throw error;
}
return withRetry(
async () => {
try {
return await githubClient.rest.issues.create(payload);
} catch (error) {
const status = typeof error === "object" && error !== null && "status" in error ? error.status : undefined;
const message = getErrorMessage(error).toLowerCase();
const isAssigneeError = status === 422 && (message.includes("assignee") || message.includes("assignees") || message.includes("unprocessable"));
if (isAssigneeError && payload.assignees && payload.assignees.length > 0) {
const removedAssignees = payload.assignees.join(", ");
core.warning(`Fallback issue creation failed due to assignee error, retrying without assignees: ${getErrorMessage(error)}`);
// Mutate payload in-place so that any subsequent withRetry attempts also
// omit assignees and do not re-trigger the same 422 path.
delete payload.assignees;
payload.body = `${payload.body}\n\n> [!NOTE]\n> Assignees (${removedAssignees}) could not be set on this issue due to an API error.`;
return await githubClient.rest.issues.create(payload);
}
throw error;
}
},
RATE_LIMIT_RETRY_CONFIG,
`create fallback issue in ${repoParts.owner}/${repoParts.repo}`
);
}

/**
Expand Down Expand Up @@ -1743,15 +1755,20 @@ ${patchPreview}`;

// Try to create the pull request, with fallback to issue creation
try {
const { data: pullRequest } = await githubClient.rest.pulls.create({
owner: repoParts.owner,
repo: repoParts.repo,
title: title,
body: body,
head: branchName,
base: baseBranch,
draft: draft,
});
const { data: pullRequest } = await withRetry(
() =>
githubClient.rest.pulls.create({
owner: repoParts.owner,
repo: repoParts.repo,
title: title,
body: body,
head: branchName,
base: baseBranch,
draft: draft,
}),
RATE_LIMIT_RETRY_CONFIG,
`create pull request in ${repoParts.owner}/${repoParts.repo}`
);

core.info(`Created pull request #${pullRequest.number}: ${pullRequest.html_url}`);

Expand Down
203 changes: 203 additions & 0 deletions actions/setup/js/create_pull_request.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -2114,3 +2114,206 @@ describe("create_pull_request - threat detection caution", () => {
expect((between.match(/\n/g) || []).length).toBeGreaterThanOrEqual(2);
});
});

describe("create_pull_request - rate-limit retry", () => {
let originalEnv;
let tempDir;

/**
* Creates a mock GitHub API rate-limit error object (HTTP 403 with x-ratelimit-remaining: 0)
* that matches what octokit returns when the installation token quota is exhausted.
* @param {string} [message]
* @returns {Error}
*/
function createRateLimitError(message = "API rate limit exceeded") {
return Object.assign(new Error(message), {
status: 403,
response: { headers: { "x-ratelimit-remaining": "0" }, status: 403 },
});
}

beforeEach(() => {
originalEnv = { ...process.env };
process.env.GH_AW_WORKFLOW_ID = "test-workflow";
process.env.GITHUB_REPOSITORY = "test-owner/test-repo";
process.env.GITHUB_BASE_REF = "main";
tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "create-pr-rate-limit-test-"));

global.core = {
info: vi.fn(),
warning: vi.fn(),
error: vi.fn(),
debug: vi.fn(),
setFailed: vi.fn(),
setOutput: vi.fn(),
startGroup: vi.fn(),
endGroup: vi.fn(),
summary: {
addRaw: vi.fn().mockReturnThis(),
write: vi.fn().mockResolvedValue(undefined),
},
};

global.github = {
rest: {
pulls: {
create: vi.fn().mockResolvedValue({ data: { number: 42, html_url: "https://github.com/test/pull/42" } }),
requestReviewers: vi.fn().mockResolvedValue({}),
},
repos: {
get: vi.fn().mockResolvedValue({ data: { default_branch: "main" } }),
},
issues: {
create: vi.fn().mockResolvedValue({ data: { number: 99, html_url: "https://github.com/test/issues/99" } }),
addLabels: vi.fn().mockResolvedValue({}),
},
},
graphql: vi.fn(),
};

global.context = {
eventName: "issues",
repo: { owner: "test-owner", repo: "test-repo" },
payload: {},
runId: "12345",
};

global.exec = {
exec: vi.fn().mockResolvedValue(0),
getExecOutput: vi.fn().mockImplementation(async (program, args) => {
if (program === "git" && args[0] === "rev-list") {
return { exitCode: 0, stdout: "1", stderr: "" };
}
return { exitCode: 0, stdout: "main", stderr: "" };
}),
};

delete require.cache[require.resolve("./create_pull_request.cjs")];
});

afterEach(() => {
for (const key of Object.keys(process.env)) {
if (!(key in originalEnv)) {
delete process.env[key];
}
}
Object.assign(process.env, originalEnv);

if (tempDir && fs.existsSync(tempDir)) {
fs.rmSync(tempDir, { recursive: true, force: true });
}

delete global.core;
delete global.github;
delete global.context;
delete global.exec;
vi.clearAllMocks();
});

it("should retry PR creation on rate limit error and succeed", async () => {
vi.useFakeTimers();
try {
global.github.rest.pulls.create.mockRejectedValueOnce(createRateLimitError()).mockResolvedValue({ data: { number: 42, html_url: "https://github.com/test/pull/42" } });

const { main } = require("./create_pull_request.cjs");
const handler = await main({ allow_empty: true });

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

await vi.runAllTimersAsync();

const result = await resultPromise;

expect(result.success).toBe(true);
expect(result.pull_request_number).toBe(42);
// 1 initial (rate-limited) + 1 retry (succeeds) = 2 calls total
expect(global.github.rest.pulls.create).toHaveBeenCalledTimes(2);
expect(global.core.warning).toHaveBeenCalledWith(expect.stringContaining("create pull request"));
} finally {
vi.useRealTimers();
}
});

it("should fall back to issue when PR creation fails after all rate-limit retries", async () => {
vi.useFakeTimers();
try {
global.github.rest.pulls.create.mockRejectedValue(createRateLimitError());
global.github.rest.issues.create.mockResolvedValue({ data: { number: 99, html_url: "https://github.com/test/issues/99" } });

const { main } = require("./create_pull_request.cjs");
const handler = await main({ allow_empty: true });

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

await vi.runAllTimersAsync();

const result = await resultPromise;

// Should fall back to issue creation after PR retries are exhausted
expect(result.success).toBe(true);
expect(result.fallback_used).toBe(true);
expect(result.issue_number).toBe(99);
// 1 initial + 5 retries = 6 total PR creation attempts (RATE_LIMIT_RETRY_CONFIG.maxRetries = 5)
expect(global.github.rest.pulls.create).toHaveBeenCalledTimes(6);

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] This assertion hardcodes 6 (1 initial + 5 retries), coupling the test to RATE_LIMIT_RETRY_CONFIG.maxRetries. If the retry config changes, this test fails with a cryptic number mismatch.

Consider importing the constant:

const { RATE_LIMIT_RETRY_CONFIG } = require('./error_recovery.cjs');
// ...
expect(global.github.rest.pulls.create).toHaveBeenCalledTimes(RATE_LIMIT_RETRY_CONFIG.maxRetries + 1);

This makes the test self-documenting and resilient to config tuning.

expect(global.github.rest.issues.create).toHaveBeenCalled();
} finally {
vi.useRealTimers();
}
});

it("should retry fallback issue creation on rate limit error and succeed", async () => {
vi.useFakeTimers();
try {
// PR creation fails with a non-rate-limit error to trigger fallback immediately
global.github.rest.pulls.create.mockRejectedValue(new Error("Some PR creation error"));
// Fallback issue creation first fails with rate limit, then succeeds
global.github.rest.issues.create.mockRejectedValueOnce(createRateLimitError()).mockResolvedValue({ data: { number: 99, html_url: "https://github.com/test/issues/99" } });

const { main } = require("./create_pull_request.cjs");
const handler = await main({ allow_empty: true });

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

await vi.runAllTimersAsync();

const result = await resultPromise;

expect(result.success).toBe(true);
expect(result.fallback_used).toBe(true);
expect(result.issue_number).toBe(99);
// Fallback issue: 1 rate-limited attempt + 1 successful retry = 2 calls
expect(global.github.rest.issues.create).toHaveBeenCalledTimes(2);
expect(global.core.warning).toHaveBeenCalledWith(expect.stringContaining("create fallback issue"));
} finally {
vi.useRealTimers();
}
});

it("should append a note to the fallback issue body when assignees are removed due to 422 error", async () => {
// PR creation fails with a non-rate-limit error to trigger fallback immediately
global.github.rest.pulls.create.mockRejectedValue(new Error("Some PR creation error"));

const assigneeError = Object.assign(new Error("Validation Failed: assignees are invalid"), {
status: 422,
response: { status: 422 },
});
// First call fails with assignee 422, second succeeds
global.github.rest.issues.create.mockRejectedValueOnce(assigneeError).mockResolvedValue({ data: { number: 77, html_url: "https://github.com/test/issues/77" } });

const { main } = require("./create_pull_request.cjs");
const handler = await main({ allow_empty: true, assignees: ["user1", "user2"] });

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

expect(result.success).toBe(true);
expect(result.fallback_used).toBe(true);
expect(result.issue_number).toBe(77);
expect(global.github.rest.issues.create).toHaveBeenCalledTimes(2);
// Second call (without assignees) should have a note in the body
const secondCall = global.github.rest.issues.create.mock.calls[1][0];
expect(secondCall.assignees).toBeUndefined();
expect(secondCall.body).toContain("user1");
expect(secondCall.body).toContain("user2");
expect(secondCall.body).toContain("could not be set");
});
});
20 changes: 13 additions & 7 deletions actions/setup/js/push_to_pull_request_branch.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const { isStagedMode } = require("./safe_output_helpers.cjs");
const { pushSignedCommits } = require("./push_signed_commits.cjs");
const { updateActivationCommentWithCommit, updateActivationComment } = require("./update_activation_comment.cjs");
const { getErrorMessage } = require("./error_helpers.cjs");
const { withRetry, RATE_LIMIT_RETRY_CONFIG } = require("./error_recovery.cjs");
const { normalizeBranchName } = require("./normalize_branch_name.cjs");
const { pushExtraEmptyCommit } = require("./extra_empty_commit.cjs");
const { detectForkPR, checkBranchPushable } = require("./pr_helpers.cjs");
Expand Down Expand Up @@ -471,13 +472,18 @@ async function main(config = {}) {
});

try {
const { data: issue } = await githubClient.rest.issues.create({
owner: repoParts.owner,
repo: repoParts.repo,
title: issueTitle,
body: issueBody,
labels: ["agentic-workflows"],
});
const { data: issue } = await withRetry(

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 manifest-protection issue creation path now uses withRetry, but there are no new tests covering this code path's rate-limit behaviour. The only rate-limit tests are in create_pull_request.test.cjs.

A test for push_to_pull_request_branch.cjs that simulates a rate-limit error on the manifest-protection issues.create call would confirm this retry path works end-to-end and protect against regressions if the retry wiring is ever accidentally removed.

() =>
githubClient.rest.issues.create({
owner: repoParts.owner,
repo: repoParts.repo,
title: issueTitle,
body: issueBody,
labels: ["agentic-workflows"],
}),
RATE_LIMIT_RETRY_CONFIG,
`create manifest-protection review issue in ${repoParts.owner}/${repoParts.repo}`
);
core.info(`Created manifest-protection review issue #${issue.number}: ${issue.html_url}`);
await updateActivationComment(github, context, core, issue.html_url, issue.number, "issue");
return {
Expand Down