Treat stacked-PR update_branch API failures as non-fatal in PR Sous Chef safe outputs - #53300
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
update_branch API failures as non-fatal in PR Sous Chef safe outputs
PR TriageCategory: bug | Risk: low | Score: 40/100
Recommended action: batch_review (grouped with #53302, #53301 as small workflow/agent fixes)
|
|
@copilot fix update-branch to handle stacked PRs if possible |
There was a problem hiding this comment.
Pull request overview
Handles GitHub’s stacked-PR branch-update limitation without blocking PR Sous Chef safe outputs.
Changes:
- Classifies the known stacked-PR API rejection as non-fatal.
- Adds regression coverage ensuring title updates continue.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/update_pull_request.cjs |
Adds stacked-PR error classification. |
actions/setup/js/update_pull_request.test.cjs |
Tests non-fatal handling and continued updates. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs, /tdd, and /codebase-design — requesting changes on two correctness issues and a test gap.
📋 Key Themes & Highlights
Key Themes
- Dead code (correctness): The
isStackedPRUnsupportedUpdateBranchErrorguard added insideisNonFatalUpdateBranchError(line 67) is unreachable —executePRUpdatedispatches that error type first, beforeisNonFatalUpdateBranchErroris ever called with it. - Non-standard API field:
pullRequest?.stack?.number(line 119) is not in the public GitHub REST API shape forpulls.get, making the first resolution path a silent no-op on every call. - Test gap: The first stacked-PR test (
should continue title/body updates when updateBranch reports stacked-PR unsupported) doesn't assert thatrequestwas called for stack resolution, so a regression bypassingtryStackedPRUpdateBranchentirely would go undetected.
Positive Highlights
- ✅ Root cause is correctly identified and the fix is narrowly scoped.
- ✅ Three new focused tests for the stacked-PR path including success, stack-sync-fails, and wrong-status-is-fatal scenarios — good regression coverage.
- ✅ All new
core.infolog lines clearly explain the classification decision, which will make future debugging much easier. - ✅ Extracting
getErrorStatuswas the right refactor — eliminates repeated traversal logic.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 40.3 AIC · ⌖ 9.17 AIC · ⊞ 7.7K
Comment /matt to run again
| // GitHub update-branch API also returns 403 with this message when a PR contains workflow | ||
| // file changes and the check times out, rather than the usual "refusing to allow" phrase. | ||
| const hasWorkflowsScopeRequired = message.includes("`workflows` scope may be required") || message.includes("unable to determine if workflow can be created or updated"); | ||
| if (isStackedPRUnsupportedUpdateBranchError(error)) { |
There was a problem hiding this comment.
[/diagnosing-bugs] Dead code: this isStackedPRUnsupportedUpdateBranchError check inside isNonFatalUpdateBranchError is unreachable. The call-site at line 200 dispatches stacked-PR errors first, before isNonFatalUpdateBranchError is called with them.
💡 Suggested fix
Remove this guard from isNonFatalUpdateBranchError. The stacked-PR case is now exclusively handled by the call-site dispatch, so this branch can never be reached.
Alternatively, invert the dependency: remove the call-site check and keep the logic centralised inside isNonFatalUpdateBranchError — but then the separate tryStackedPRUpdateBranch call in executePRUpdate would need restructuring too.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 0384389: removed the unreachable stacked-PR branch from isNonFatalUpdateBranchError(...) and kept stacked-PR handling at the call-site dispatch in executePRUpdate(...).
| stackedPRError.status = 422; | ||
| mockGithub.rest.pulls.updateBranch.mockRejectedValueOnce(stackedPRError); | ||
|
|
||
| const handler = await updatePRModule.main({ update_branch: true }); |
There was a problem hiding this comment.
[/tdd] Missing assertion: this test relies on the beforeEach mock returning an empty stacks list, causing tryStackedPRUpdateBranch to silently fall back to the warning path — but there is no assertion that request was actually called, so a regression where tryStackedPRUpdateBranch is skipped entirely would still pass.
💡 Suggested addition
// Verify the sync path was attempted before falling back
expect(mockGithub.request).toHaveBeenCalledWith(
"GET /repos/{owner}/{repo}/stacks",
expect.objectContaining({ pull_request: 100 })
);This pins the intended behaviour: when the stacked-PR error fires, the code must always attempt stack resolution before warning.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 0384389: added an assertion in update_pull_request.test.cjs to verify GET /repos/{owner}/{repo}/stacks is attempted in the stacked-PR unsupported path before non-fatal fallback.
| repo: context.repo.repo, | ||
| pull_number: prNumber, | ||
| }); | ||
| if (typeof pullRequest?.stack?.number === "number") { |
There was a problem hiding this comment.
[/codebase-design] pullRequest?.stack?.number is not part of the standard GitHub REST API response for pulls.get. This field will always be undefined in practice, making the first resolution path a no-op and adding a redundant API call on every stacked-PR failure.
💡 Suggestion
If there is no documented stack field on the pulls API response, remove the pulls.get lookup entirely and go directly to GET /repos/{owner}/{repo}/stacks. That removes the silent no-op and an unnecessary API round-trip.
If the field is available via a private/beta API, add a comment explaining the source and the expected shape so reviewers and future editors understand why it is accessed.
@copilot please address this.
There was a problem hiding this comment.
Addressed in 0384389: removed the pulls.get stack-field lookup and now resolve stack membership directly via GET /repos/{owner}/{repo}/stacks before sync.
|
@copilot run pr-finisher skill |
|
@copilot pr stacks are new and the octokit api might be lagging behind |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot this PR is ready for the next finishing pass. Please address these items, newest first:
Run: https://github.com/github/gh-aw/actions/runs/32032749148
|
Handled: the stacked-PR path uses raw |
Verified in current code: stacked-PR handling calls raw REST routes via |
|
@copilot this PR is ready for the next finishing pass. Please address these items, newest first:
Run: https://github.com/github/gh-aw/actions/runs/32034893678
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Fixed in I reproduced the linked failure and updated |
|
@copilot please take another pass on this PR. Please address the remaining maintainer-visible failing check (Unit tests (D-L)) and recent maintainer follow-up, refresh the branch if needed, and run the Run: https://github.com/github/gh-aw/actions/runs/32038496983
|
PR Sous Cheffailed itssafe_outputsjob whenupdate_pull_request(update_branch: true)hit GitHub’s stacked-PR limitation (Updating a stacked PR's branch via this endpoint is not supported). This change narrows error handling so that this known branch-update condition does not fail the safe-output pipeline.Root-cause-aligned error classification
actions/setup/js/update_pull_request.cjsto classify the stacked-PR update-branch API rejection as non-fatal insideisNonFatalUpdateBranchError(...).Targeted regression coverage
actions/setup/js/update_pull_request.test.cjsto assert that when update-branch returns the stacked-PR unsupported error, title/body updates continue and the operation is surfaced as non-fatal._Run: https://github.com/github/gh-aw/actions/runs/32028986454_> Generated by 👨🍳 PR Sous Chef · gpt54 · 6.57 AIC · ⌖ 7.37 AIC · ⊞ 6.3K · ◷
\nrun: https://github.com/github/gh-aw/actions/runs/32032749148> Generated by 👨🍳 PR Sous Chef · gpt54 · 6.96 AIC · ⌖ 5.94 AIC · ⊞ 8.8K · ◷