fix: treat workflows-scope 403 on branch update as non-fatal; add token fallback for add_comment - #48900
Conversation
…allback - Extend isNonFatalUpdateBranchError in update_pull_request.cjs to treat 403 errors with "workflows scope may be required" or "unable to determine if workflow can be created or updated" as non-fatal. This matches the GitHub API error returned when a PR contains workflow file changes and the token lacks workflows scope, preventing a spurious update_pull_request failure. - Add test coverage for the new non-fatal 403 pattern. - In pr-sous-chef.md (and recompiled lock.yml), update add-comment github-token to use AWI_MAINTENANCE_TOKEN || GH_AW_GITHUB_TOKEN || GITHUB_TOKEN, providing a fallback when AWI_MAINTENANCE_TOKEN is not configured. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
workflows-scope 403 on branch update as non-fatal; add token fallback for add_comment
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #48900 does not have the 'implementation' label and has 0 new lines of code in business logic directories (threshold: 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. Caution agentic threat detected DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
There was a problem hiding this comment.
Pull request overview
Fixes safe-output failures during pull request branch updates and comment authentication.
Changes:
- Treats workflow-scope update-branch 403s as non-fatal.
- Adds token fallback for
add_commentand regression coverage. - Refreshes compiled workflows, including an unrelated Daily Graft model change.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/update_pull_request.cjs |
Recognizes the additional non-fatal 403 message. |
actions/setup/js/update_pull_request.test.cjs |
Tests continued PR updates after the 403. |
.github/workflows/pr-sous-chef.md |
Adds the comment-token fallback chain. |
.github/workflows/pr-sous-chef.lock.yml |
Recompiles the PR Sous Chef workflow. |
.github/workflows/daily-graft-intelligence.lock.yml |
Changes default model selection to auto. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Medium
| GH_AW_INFO_ENGINE_ID: "copilot" | ||
| GH_AW_INFO_ENGINE_NAME: "GitHub Copilot CLI" | ||
| GH_AW_INFO_MODEL: ${{ vars.GH_AW_MODEL_AGENT_COPILOT || vars.GH_AW_DEFAULT_MODEL_COPILOT || 'claude-sonnet-5' }} | ||
| GH_AW_INFO_MODEL: ${{ vars.GH_AW_MODEL_AGENT_COPILOT || vars.GH_AW_DEFAULT_MODEL_COPILOT || 'auto' }} |
| it("should continue title/body updates when updateBranch gets workflows-scope-required 403", async () => { | ||
| const scopeError = new Error("Unable to determine if workflow can be created or updated due to timeout; `workflows` scope may be required. - https://docs.github.com/rest/pulls/pulls#update-a-pull-request-branch"); | ||
| scopeError.status = 403; | ||
| // The message contains "timeout" which makes isTransientError return true, so withRetry | ||
| // retries once (maxRetries: 1). Both attempts must fail to reach the non-fatal catch path. | ||
| 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", | ||
| pull_number: 100, | ||
| title: "Updated PR", | ||
| }); | ||
| expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("branch from base (non-fatal)")); | ||
| }); |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The changes look correct and well-tested.
update_pull_request.cjs: The new hasWorkflowsScopeRequired check correctly extends isNonFatalUpdateBranchError to cover the workflows-scope 403 timeout variant. The string matching uses message which is already lowercased (line 35), and all new literals are lowercase — no case-matching issue. The new path is covered by a dedicated test case.
Token fallback: The AWI_MAINTENANCE_TOKEN || GH_AW_GITHUB_TOKEN || GITHUB_TOKEN cascade in the lock.yml and workflow source follows the established fallback pattern and ensures add_comment is not blocked by a missing maintenance token.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 24.9 AIC · ⌖ 5.87 AIC · ⊞ 5.1K
|
Caution agentic threat detected DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report🔶 Test Quality Score: 50/100 — Needs improvement
📊 Metrics (1 test)
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — commenting on two targeted issues; no blocking defects.
📋 Key Themes & Highlights
Key Themes
- Status-guard gap (
update_pull_request.cjsline 58): the fallthroughreturnnow includeshasWorkflowsScopeRequiredwithout restricting tostatus === undefined, which could silently swallow future errors with unexpected HTTP statuses. Addressed in inline comment. - Test fragility (
update_pull_request.test.cjs): thetoHaveBeenCalledTimes(2)assertion is implicitly coupled tomaxRetries: 1with no pointer to the source of truth; a retry-count change could cause a misleading test failure. The secondhasWorkflowsScopeRequiredphrase is also untested. Both addressed in inline comments.
Positive Highlights
- ✅ Root cause properly diagnosed — two distinct failure modes, each fixed independently
- ✅
mockRejectedValueusage (persistent) is well-documented in both the PR description and test comment — that's valuable for future readers - ✅ Token fallback chain in
pr-sous-chef.mdis correctly ordered: scoped token → broader token → built-in token - ✅ Lock files recompiled and consistent with the source change
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 44.2 AIC · ⌖ 5.26 AIC · ⊞ 6.8K
Comment /matt to run again
| // - cannot auto-update due to conflict ("merge conflict between base and head") | ||
| // These should not fail safe output processing. | ||
| return message.includes("there are no new commits on the base branch") || message.includes("merge conflict between base and head") || hasWorkflowsPermissionError; | ||
| return message.includes("there are no new commits on the base branch") || message.includes("merge conflict between base and head") || hasWorkflowsPermissionError || hasWorkflowsScopeRequired; |
There was a problem hiding this comment.
[/diagnosing-bugs] The fallthrough return at line 58 includes hasWorkflowsScopeRequired without a status guard. If GitHub ever returns a non-403/non-422 status with one of these message fragments, a real error could be silently swallowed.
💡 Suggested guard
Consider restricting the fallthrough path to only fire when status === undefined:
return (
message.includes("there are no new commits on the base branch") ||
message.includes("merge conflict between base and head") ||
(hasWorkflowsPermissionError && status === undefined) ||
(hasWorkflowsScopeRequired && status === undefined)
);The if (status !== undefined) block above already handles the 403 case explicitly, so the fallthrough should only apply to errors without a numeric status.
@copilot please address this.
| scopeError.status = 403; | ||
| // The message contains "timeout" which makes isTransientError return true, so withRetry | ||
| // retries once (maxRetries: 1). Both attempts must fail to reach the non-fatal catch path. | ||
| mockGithub.rest.pulls.updateBranch.mockRejectedValue(scopeError); |
There was a problem hiding this comment.
[/tdd] The test comment explains why mockRejectedValue (persistent) is used instead of mockRejectedValueOnce, but this implicit coupling to retry count is fragile — if maxRetries changes, the assertion toHaveBeenCalledTimes(2) will silently pass or fail for the wrong reason.
💡 Suggestion
Consider making the retry count explicit in the test by checking the warning call count or asserting that the final result still succeeds regardless of retry count. At minimum, add a comment linking to where maxRetries is defined so a future change there prompts updating this test:
// updateBranch is retried once (maxRetries: 1, see executePRUpdate in update_pull_request.cjs)
// Update this assertion if maxRetries changes.
expect(mockGithub.rest.pulls.updateBranch).toHaveBeenCalledTimes(2);@copilot please address this.
| expect(mockGithub.rest.pulls.updateBranch).toHaveBeenCalledTimes(2); | ||
| expect(mockGithub.rest.pulls.update).toHaveBeenCalledWith({ | ||
| owner: "testowner", | ||
| repo: "testrepo", |
There was a problem hiding this comment.
[/tdd] The test covers only the first message variant (workflows scope may be required). A second variant (unable to determine if workflow can be created or updated) in hasWorkflowsScopeRequired is untested — a typo or drift in that string would go undetected.
💡 Suggested additional test
Add 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.
There was a problem hiding this comment.
Verdict: Approve-level, no blocking issues
Small, well-scoped fix (5 files, 40 lines). The 403 non-fatal error handling extension and token fallback are correct and adequately tested.
Review notes
isNonFatalUpdateBranchError: new substring checks for the workflows-scope-required 403 message are correctly scoped and consistent with existing patterns.- Test correctly uses
mockRejectedValue(persistent) instead ofmockRejectedValueOnce, with a clear comment explaining the retry interaction withisTransientError. add-commentgithub-token fallback (AWI_MAINTENANCE_TOKEN || GH_AW_GITHUB_TOKEN || GITHUB_TOKEN) is reasonable; the PR description already documents the expired-secret caveat.- The
daily-graft-intelligence.lock.ymlmodel default change (claude-sonnet-5→auto) is an unrelated recompile side-effect from base branch drift, not part of the manual change set.
No correctness, concurrency, or security-adjacent issues found in the changed lines.
🔎 Code quality review by PR Code Quality Reviewer · aut00 · 43.2 AIC · ⌖ 4.52 AIC · ⊞ 7.5K
Comment /review to run again
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
Two failures observed in the
pr-sous-chefworkflow run (job/90592217977):update_pull_requestmessages were failing entirely when the branch-update step hit a 403 from a PR containing workflow files.add_commentwas returning 401 becauseAWI_MAINTENANCE_TOKENhad no fallback when unset.update_pull_request— new non-fatal 403 patternGitHub's update-branch API returns a distinct 403 message when it times out validating workflow file changes:
isNonFatalUpdateBranchErroronly handled the"refusing to allow a GitHub App … without \workflows` permission"variant. This new message was re-thrown as fatal, aborting the rest of theupdate_pull_request` message (body append, manifest logging, etc.).Extended the guard to also match:
`workflows` scope may be requiredunable to determine if workflow can be created or updatedThe branch-update failure is now demoted to a warning and processing continues. Added a test; note the mock uses
mockRejectedValue(persistent) rather thanmockRejectedValueOncebecause"timeout"in the message makesisTransientErrorreturntrue, triggering one retry.add_comment— token fallbackUpdated
pr-sous-chef.mdsafe-output config:This prevents a hard failure when
AWI_MAINTENANCE_TOKENis not set. Note: if the secret is set but expired (non-empty), the||fallback won't activate — the token itself needs to be rotated in that case.Lock file recompiled.