-
Notifications
You must be signed in to change notification settings - Fork 478
fix: treat workflows-scope 403 on branch update as non-fatal; add token fallback for add_comment
#48900
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
fix: treat workflows-scope 403 on branch update as non-fatal; add token fallback for add_comment
#48900
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -941,4 +941,56 @@ 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-scope-required 403 (scope phrase variant)", async () => { | ||
| // Message matches only the "`workflows` scope may be required" branch of hasWorkflowsScopeRequired. | ||
| const scopeError = new Error("Validation failed; `workflows` scope may be required due to timeout in check."); | ||
| scopeError.status = 403; | ||
| // The message contains "timeout" which makes isTransientError return true, so withRetry | ||
| // retries once (maxRetries: 1, see executePRUpdate). Both attempts must fail to reach the | ||
| // non-fatal catch path. Update this assertion if maxRetries changes. | ||
| mockGithub.rest.pulls.updateBranch.mockRejectedValue(scopeError); | ||
|
|
||
| const handler = await updatePRModule.main({ update_branch: true }); | ||
| const result = await handler({ | ||
| pull_request_number: 100, | ||
| title: "Updated PR", | ||
| }); | ||
|
|
||
| expect(result.success).toBe(true); | ||
| // Called twice: initial attempt + 1 retry (maxRetries: 1 in executePRUpdate) | ||
| expect(mockGithub.rest.pulls.updateBranch).toHaveBeenCalledTimes(2); | ||
| expect(mockGithub.rest.pulls.update).toHaveBeenCalledWith({ | ||
| owner: "testowner", | ||
| repo: "testrepo", | ||
|
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 test covers only the first message variant ( 💡 Suggested additional testAdd a parallel test case for the second phrase: it('should treat unable-to-determine 403 as non-fatal', async () => {
const err = new Error('Unable to determine if workflow can be created or updated; contact support');
err.status = 403;
mockGithub.rest.pulls.updateBranch.mockRejectedValue(err);
const handler = await updatePRModule.main({ update_branch: true });
const result = await handler({ pull_request_number: 100, title: 'PR' });
expect(result.success).toBe(true);
expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining('branch from base (non-fatal)'));
});@copilot please address this. |
||
| pull_number: 100, | ||
| title: "Updated PR", | ||
| }); | ||
| expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("branch from base (non-fatal)")); | ||
| }); | ||
|
|
||
| it("should continue title/body updates when updateBranch gets unable-to-determine-workflow 403 (unable-to-determine variant)", async () => { | ||
| // Message matches only the "unable to determine if workflow can be created or updated" branch | ||
| // of hasWorkflowsScopeRequired, independently of the scope-phrase variant above. | ||
| const unableToDetermineError = new Error("Unable to determine if workflow can be created or updated; contact support."); | ||
| unableToDetermineError.status = 403; | ||
| // No "timeout" in the message, so isTransientError returns false — no retry, called once. | ||
| mockGithub.rest.pulls.updateBranch.mockRejectedValueOnce(unableToDetermineError); | ||
|
|
||
| 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)")); | ||
| }); | ||
| }); | ||
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 test comment explains why
mockRejectedValue(persistent) is used instead ofmockRejectedValueOnce, but this implicit coupling to retry count is fragile — ifmaxRetrieschanges, the assertiontoHaveBeenCalledTimes(2)will silently pass or fail for the wrong reason.💡 Suggestion
Consider making the retry count explicit in the test by checking the
warningcall count or asserting that the final result still succeeds regardless of retry count. At minimum, add a comment linking to wheremaxRetriesis defined so a future change there prompts updating this test:@copilot please address this.