-
Notifications
You must be signed in to change notification settings - Fork 389
Handle update_pull_request update-branch soft 422s as non-fatal in safe outputs
#32246
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.
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
Changes from all commits
0792a7d
15f0268
82b2e4e
4e4eb22
c286967
9c7cde4
36c4914
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -847,4 +847,36 @@ describe("update_pull_request.cjs - update_branch behavior", () => { | |
| expect(mockGithub.rest.pulls.updateBranch).toHaveBeenCalledTimes(1); | ||
| expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Failed to update pull request #100 branch from base")); | ||
| }); | ||
|
|
||
| it("should treat no-new-commits updateBranch response as a non-fatal no-op", async () => { | ||
| mockGithub.rest.pulls.updateBranch.mockRejectedValueOnce(new Error("There are no new commits on the base branch.")); | ||
|
|
||
| const handler = await updatePRModule.main({ update_branch: true }); | ||
| const result = await handler({ pull_request_number: 100 }); | ||
|
|
||
| expect(result.success).toBe(true); | ||
| expect(mockGithub.rest.pulls.updateBranch).toHaveBeenCalledTimes(1); | ||
| expect(mockGithub.rest.pulls.update).not.toHaveBeenCalled(); | ||
| expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("branch from base (non-fatal)")); | ||
| }); | ||
|
|
||
| it("should continue title/body updates when updateBranch reports merge conflict", async () => { | ||
| mockGithub.rest.pulls.updateBranch.mockRejectedValueOnce(new Error("merge conflict between base and head")); | ||
|
|
||
| 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)")); | ||
| }); | ||
| }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The two new tests cover the two known-benign 422 messages, but there's no test verifying that a different 422 message remains fatal. That boundary condition is the riskiest part of the classifier — without it, a future edit could accidentally widen the non-fatal set. Consider adding: it("should re-throw a 422 with an unrecognised message as fatal", async () => {
const err = Object.assign(new Error("Unprocessable Entity: unknown reason"), { status: 422 });
mockGithub.rest.pulls.updateBranch.mockRejectedValueOnce(err);
const handler = await updatePRModule.main({ update_branch: true });
const result = await handler({ pull_request_number: 100 });
expect(result.success).toBe(false);
expect(mockCore.warning).not.toHaveBeenCalledWith(expect.stringContaining("(non-fatal)"));
}); |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/tdd] The classifier allows errors without a
statusproperty (plainErrorobjects) to bypass the early-return guard and still match on message alone. In production the GitHub API always provides an HTTP status, so this is unlikely in practice.A brief comment would prevent future readers from "fixing" it inadvertently: