Skip to content

Handle update_pull_request.update_branch workflow-permission failures as non-fatal #32900

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 10 commits into from
May 17, 2026
Merged

Handle update_pull_request.update_branch workflow-permission failures as non-fatal #32900

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
10efb47
Handle update_pull_request branch permission errors as non-fatal
Copilot May 17, 2026
2e83d62
Treat update_branch workflow-permission errors as non-fatal
Copilot May 17, 2026
6623ed4
Refine workflows permission error matching for update_branch
Copilot May 17, 2026
8de97af
Tighten update_branch workflows-permission error detection
Copilot May 17, 2026
a9c0780
Narrow workflow permission regex for update_branch failures
Copilot May 17, 2026
ea50650
Simplify update_branch workflows-permission failure detection
Copilot May 17, 2026
59386ed
Document non-fatal workflows permission matching intent
Copilot May 17, 2026
c148a58
Merge branch 'main' into copilot/handle-failure-safe-outputs
pelikhan May 17, 2026
a973720
Merge branch 'main' into copilot/handle-failure-safe-outputs
pelikhan May 17, 2026
2b8eb85
Handle workflow-permission 403 as non-fatal in update_branch
Copilot May 17, 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
19 changes: 15 additions & 4 deletions actions/setup/js/update_pull_request.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,27 @@ function isNonFatalUpdateBranchError(error) {
status = candidateStatus;
}
}
if (status !== undefined && status !== 422) {
return false;
const message = getErrorMessage(error).toLowerCase();
const hasWorkflowsPermissionPhrase = /without\s+`?workflows`?\s+permission/i.test(message);

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

[/diagnose] The i flag on this regex is redundant — message was already lowercased on the line above (getErrorMessage(error).toLowerCase()), so the flag has no effect. It is harmless but misleading; remove it for clarity:

const hasWorkflowsPermissionPhrase = /without\s+`?workflows`?\s+permission/.test(message);

const hasWorkflowMutationRefusal = message.includes("refusing to allow a github app to create or update workflow");
// Require both permission wording and update-branch context to avoid treating unrelated
// "workflows permission" errors as non-fatal for pull request branch updates.
const hasWorkflowsPermissionError = hasWorkflowsPermissionPhrase && (hasWorkflowMutationRefusal || message.includes("update pull request"));

@github-actions github-actions Bot May 17, 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 message.includes("update pull request") branch has no dedicated test. The regression test added in this PR exercises hasWorkflowMutationRefusal (the "refusing to allow a GitHub App..." path), but not the fallback path where only "update pull request" appears in the message. Consider adding a second test case — e.g. an error like "without workflows permission for update pull request" — to ensure both branches are covered and the conjunction logic is verified independently.


if (status !== undefined) {
if (status === 403 && hasWorkflowsPermissionError) {
return true;
}
if (status !== 422) {
return false;
}
}

// GitHub update-branch API can return these 422 messages for benign conditions:
// - already up to date ("There are no new commits on the base branch")
// - cannot auto-update due to conflict ("merge conflict between base and head")
// These should not fail safe output processing.
const message = getErrorMessage(error).toLowerCase();
return message.includes("there are no new commits on the base branch") || message.includes("merge conflict between base and head");
return message.includes("there are no new commits on the base branch") || message.includes("merge conflict between base and head") || hasWorkflowsPermissionError;
}

/**
Expand Down
22 changes: 22 additions & 0 deletions actions/setup/js/update_pull_request.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -879,4 +879,26 @@ describe("update_pull_request.cjs - update_branch behavior", () => {
});
expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("branch from base (non-fatal)"));
});

it("should continue title/body updates when updateBranch gets workflows-permission 403", async () => {
const permissionError = new Error("refusing to allow a GitHub App to create or update workflow `.github/workflows/test.lock.yml` without `workflows` permission");
permissionError.status = 403;
mockGithub.rest.pulls.updateBranch.mockRejectedValueOnce(permissionError);

const handler = await updatePRModule.main({ update_branch: true });
const result = await handler({
pull_request_number: 100,
title: "Updated PR",
});

expect(result.success).toBe(true);
expect(mockGithub.rest.pulls.updateBranch).toHaveBeenCalledTimes(1);
expect(mockGithub.rest.pulls.update).toHaveBeenCalledWith({
owner: "testowner",
repo: "testrepo",
pull_number: 100,
title: "Updated PR",
});
expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("branch from base (non-fatal)"));
});
});

@github-actions github-actions Bot May 17, 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 new test verifies the passing case but there's no negative/guard test confirming that a workflows-permission phrase alone (without the mutation-refusal or "update pull request" context) is still treated as fatal. This matters because the PR description explicitly calls out "scoped matching to avoid over-catching". A test like:

it("should propagate error when workflows permission phrase appears without update-branch context", async () => {
  mockGithub.rest.pulls.updateBranch.mockRejectedValueOnce(
    new Error("missing `workflows` permission")
  );
  const handler = await updatePRModule.main({ update_branch: true });
  const result = await handler({ pull_request_number: 100, title: "PR" });
  expect(result.success).toBe(false);
});

would lock down the guard condition and prevent accidental over-broadening in future edits.

Loading