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

[Copilot]

Safe-output processing was failing when update_pull_request attempted update_branch and GitHub rejected workflow-file updates due to missing workflows permission. This caused the safe-outputs job to fail instead of continuing with remaining actionable updates.

• Failure classification adjustment

  • Updates update_pull_request branch-update error handling to classify workflow-permission rejections as non-fatal for the update_branch step.
  • Preserves warning visibility while allowing the handler flow to continue (e.g., title/body/state updates and downstream safe outputs).

• Scoped matching to avoid over-catching

  • Detects this class of failure only when permission wording and update-branch context are present, rather than broadly suppressing unrelated API errors.

• Regression coverage

  • Adds a focused unit case asserting that a workflow-permission rejection during update_branch does not fail the overall update_pull_request operation when other updates are present.

const hasWorkflowsPermissionPhrase = /without\s+`?workflows`?\s+permission/i.test(message);
const hasWorkflowMutationRefusal = message.includes("refusing to allow a github app to create or update workflow");
const hasWorkflowsPermissionError =
  hasWorkflowsPermissionPhrase &&
  (hasWorkflowMutationRefusal || message.includes("update pull request"));

[Copilot]

Pull request overview

Treats GitHub's "refusing to allow a GitHub App to create or update workflow ... without workflows permission" rejection from pulls.updateBranch as non-fatal so that the rest of an update_pull_request safe output (title/body/state) can still be applied.

Changes:

• Extends isNonFatalUpdateBranchError to also match workflow-permission rejection messages, gated by a refusal/update-pull-request context to avoid over-matching.
• Adds a unit test verifying that a workflows-permission rejection during updateBranch still allows the title update to succeed and emits the non-fatal warning.

Show a summary per file

File	Description
actions/setup/js/update_pull_request.cjs	Adds workflow-permission phrase detection to isNonFatalUpdateBranchError.
actions/setup/js/update_pull_request.test.cjs	Adds regression test asserting title update proceeds when updateBranch is blocked by workflows permission.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comments suppressed due to low confidence (2)

actions/setup/js/update_pull_request.cjs:48

• The comment block above (lines 38-41) enumerates the benign 422 messages handled by this function but was not updated to mention the new workflow-permission rejection case. Readers will be misled about which conditions are treated as non-fatal. Consider adding a third bullet describing the "refusing to allow a GitHub App to create or update workflow ... without workflows permission" case.

  const hasWorkflowsPermissionPhrase = /without\s+`?workflows`?\s+permission/i.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"));
  return message.includes("there are no new commits on the base branch") || message.includes("merge conflict between base and head") || hasWorkflowsPermissionError;

actions/setup/js/update_pull_request.cjs:47

• The fallback condition message.includes("update pull request") is very broad and will match the retry context label passed to withRetry ("update pull request #N branch from base"), which gets appended to many error messages by the retry helper. Combined with the workflows-permission phrase regex this is probably acceptable, but it means almost any 422 containing "without workflows permission" anywhere will be swallowed for this code path, not just the workflow-mutation refusal. Consider tightening this to a more specific marker (e.g. requiring hasWorkflowMutationRefusal) so unrelated workflow-permission errors aren't silently downgraded to warnings.

  const hasWorkflowsPermissionError = hasWorkflowsPermissionPhrase && (hasWorkflowMutationRefusal || message.includes("update pull request"));

• Files reviewed: 2/2 changed files
• Comments generated: 1

[pelikhan]

@copilot review all comments

• lint js
• lint go

[github-actions]

Skills-Based Review 🧠

Applied /diagnose and /tdd — this is a targeted bug fix with a regression test, exactly where these skills add value.

Key Themes

• Minor implementation nit (/diagnose): The regex i flag is redundant on an already-lowercased string (line 43). Harmless but worth cleaning up.
• Partial test coverage (/tdd): The regression test covers the hasWorkflowMutationRefusal path through isNonFatalUpdateBranchError. The second matching branch (message.includes("update pull request")) and the guard's negative case (phrase without context = still fatal) are not tested.

Positive Highlights

• ✅ The conjunction guard (hasWorkflowsPermissionPhrase && (...)) is well-reasoned and directly addresses the over-catching concern called out in the description.
• ✅ A regression test was added in the same PR — good /diagnose discipline.
• ✅ Clear, descriptive variable names make the matching logic easy to audit.

Verdict

Fix is sound. The suggestions are non-blocking — the main failure mode is covered. The secondary branch test and negative guard test would meaningfully harden the scoped-matching promise made in the PR description.

🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 5.5M

[github-actions]

[/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);

[github-actions]

[/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.

[github-actions]

[/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.

[github-actions]

🧪 Test Quality Sentinel Report

Test Quality Score: 80/100

✅ Excellent

Metric	Value
New/modified tests analyzed	1
✅ Design tests (behavioral contracts)	1 (100%)
⚠️ Implementation tests (low value)	0 (0%)
Tests with error/edge cases	1 (100%)
Duplicate test clusters	0
Test inflation detected	Yes (20 test lines vs 6 production lines, ratio 3.3:1)
🚨 Coding-guideline violations	None

---

Test Classification Details

Test	File	Classification	Issues Detected
should continue title/body updates when updateBranch is blocked by workflows permission	actions/setup/js/update_pull_request.test.cjs	✅ Design	Minor: test inflation (3.3:1 ratio), but content is justified

---

Flagged Tests — Requires Review

No tests require mandatory review. One minor observation is noted below.

i️ Test inflation (informational, not blocking)

The test file added 20 lines while the production file added 6 lines (ratio ≈ 3.3:1, threshold 2:1). However, the 6 production lines introduce a non-trivial regex-based error detection path that justifies thorough behavioral verification. The test covers the full observable contract: updateBranch is attempted, a workflows-permission error is absorbed non-fatally, title/body updates still proceed (pulls.update called), result.success is true, and a warning is emitted. This is appropriate coverage for the change.

---

Language Support

Tests analyzed:

• 🟨 JavaScript (*.test.cjs): 1 test (vitest)

---

Verdict

✅ Check passed. 0% of new tests are implementation tests (threshold: 30%). The single new test is a well-structured behavioral contract test that covers the error path introduced by this PR — specifically, that a workflows permission refusal from updateBranch is treated as non-fatal while title/body updates continue.

📖 Understanding Test Classifications

Design Tests (High Value) verify what the system does:

• Assert on observable outputs, return values, or state changes
• Cover error paths and boundary conditions
• Would catch a behavioral regression if deleted
• Remain valid even after internal refactoring

Implementation Tests (Low Value) verify how the system does it:

• Assert on internal function calls (mocking internals)
• Only test the happy path with typical inputs
• Break during legitimate refactoring even when behavior is correct
• Give false assurance: they pass even when the system is wrong

Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators.

🧪 Test quality analysis by Test Quality Sentinel · ● 8.1M · ◷

[github-actions]

✅ Test Quality Sentinel: 80/100. Test quality is acceptable — 0% of new tests are implementation tests (threshold: 30%). The new test verifies the behavioral contract of the non-fatal workflows permission path end-to-end.

[pelikhan]

@copilot review all comments

[Copilot]

@copilot review all comments

• lint js
• lint go

Addressed in 2b8eb85.

• Updated update_pull_request non-fatal classification so workflow-permission rejections are handled when returned as 403 during updateBranch.
• Updated regression test to use a realistic error object with status = 403.
• Ran JS lint: make lint-cjs ✅
• Ran Go lint: make golint ❌ (pre-existing gofmt issue in pkg/workflow/gh_cli_permissions.go, unrelated to this change).