Handle update_pull_request update-branch soft 422s as non-fatal in safe outputs

[Copilot]

Bug Fix

PR Sous Chef was intermittently failing in safe_outputs because update_pull_request treated benign pulls.updateBranch responses as hard failures. Specifically, “no new commits on base” and “merge conflict between base and head” were counted as failed safe outputs, causing avoidable workflow failures.

What was the bug?

• update_pull_request with update_branch: true threw on all updateBranch failures.
• Two expected 422 outcomes (already up-to-date, merge conflict) were misclassified as fatal.
• Result: safe_outputs exited non-zero despite successful agent/detection execution and no actionable defect.

How did you fix it?

• - Handler error classification (actions/setup/js/update_pull_request.cjs)

  • Added isNonFatalUpdateBranchError(error) classifier.
  • Treats only known soft conditions as non-fatal:
    • There are no new commits on the base branch
    • merge conflict between base and head
  • Keeps hard errors fatal (including non-422 statuses).

• - Behavior change in update flow

  • For classified soft conditions:
    • emit warning (non-fatal)
    • do not throw
    • continue processing remaining PR update operations (e.g., title/body updates).

• - Tests (actions/setup/js/update_pull_request.test.cjs)

  • Added coverage for:
    • no-new-commits => successful no-op
    • merge-conflict => warning + subsequent PR update still proceeds
  • Preserved existing hard-failure expectations.

• - Docs (docs/src/content/docs/reference/safe-outputs-pull-requests.md)

  • Documented update-branch best-effort semantics for these two soft responses.

Testing

function isNonFatalUpdateBranchError(error) {
  const hasStatus = typeof error === "object" && error !== null && "status" in error;
  if (hasStatus && error.status !== 422) return false;

  const message = getErrorMessage(error).toLowerCase();
  return (
    message.includes("there are no new commits on the base branch") ||
    message.includes("merge conflict between base and head")
  );
}

[Copilot]

Pull request overview

Fixes intermittent safe_outputs failures by treating specific benign pulls.updateBranch 422 responses as non-fatal during update_pull_request processing.

Changes:

• Add a classifier to detect “already up-to-date” and “merge conflict” updateBranch errors and continue processing without throwing.
• Add tests covering the two non-fatal updateBranch scenarios (no-op and “continue with title/body update”).
• Document best-effort update-branch semantics for these soft failure cases.

Show a summary per file

File	Description
docs/src/content/docs/reference/safe-outputs-pull-requests.md	Documents that two specific updateBranch outcomes are treated as best-effort (warning + continue).
actions/setup/js/update_pull_request.test.cjs	Adds test coverage for the two newly non-fatal updateBranch error paths.
actions/setup/js/update_pull_request.cjs	Introduces isNonFatalUpdateBranchError and updates updateBranch error handling to warn+continue for soft 422s.

Copilot's findings

Tip

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

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

[github-actions]

Skills-Based Review 🧠

Applied /diagnose + /tdd — this is a targeted bug fix with new regression tests.

Key Themes

• Root cause addressed correctly: the fix introduces a dedicated classifier isNonFatalUpdateBranchError rather than burying the logic inline, which is exactly the right seam for future changes.
• Regression test coverage is good: both known-benign messages are exercised, including the "continues processing" scenario (title update after merge-conflict soft-fail). One missing edge case noted inline.
• Classifier boundary: the implicit pass-through for errors without a status field is correct behaviour but should carry a comment to prevent accidental "cleanup" later.

Positive Highlights

• ✅ Clean extraction of isNonFatalUpdateBranchError as a named, testable predicate
• ✅ Warning message clearly distinguishes (non-fatal) from fatal errors — good observability
• ✅ Documentation updated in the same PR, including the exact GitHub messages users may see
• ✅ PR description includes a post-mortem — clear root cause and fix rationale

Verdict

Approving — the two inline comments are suggestions only (a missing boundary test and a clarifying comment). Neither blocks correctness.

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

[github-actions]

[/tdd] The classifier allows errors without a status property (plain Error objects) 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:

// Errors without a `status` field (e.g. plain Error objects from tests or
// unexpected throws) also reach the message check. If the message matches a
// known benign condition they are treated as non-fatal.
const message = getErrorMessage(error).toLowerCase();

[github-actions]

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

[github-actions]

🧪 Test Quality Sentinel Report

Test Quality Score: 100/100

✅ Excellent test quality

Metric	Value
New/modified tests analyzed	2
✅ Design tests (behavioral contracts)	2 (100%)
⚠️ Implementation tests (low value)	0 (0%)
Tests with error/edge cases	2 (100%)
Duplicate test clusters	0
Test inflation detected	No (32 test lines / 27 production lines ≈ 1.2:1)
🚨 Coding-guideline violations	None

---

Test Classification Details

Test	File	Classification	Issues Detected
should treat no-new-commits updateBranch response as a non-fatal no-op	actions/setup/js/update_pull_request.test.cjs	✅ Design	None
should continue title/body updates when updateBranch reports merge conflict	actions/setup/js/update_pull_request.test.cjs	✅ Design	None

---

Test Analysis

Both new tests verify behavioral contracts of the soft-422 / non-fatal error handling introduced in this PR:

Test 1 — should treat no-new-commits updateBranch response as a non-fatal no-op
Simulates the GitHub API rejecting updateBranch with a "no new commits" message. Asserts result.success === true (non-fatal), that no further pulls.update call is made (true no-op), and that a warning is emitted. This directly validates the core behavioral promise of the PR.

Test 2 — should continue title/body updates when updateBranch reports merge conflict
Simulates a merge-conflict rejection from updateBranch. Asserts result.success === true, that pulls.update is still called with the correct payload (execution continues), and that a warning is issued. This verifies the equally important invariant: a soft 422 on the branch-update step must not abort the title/body update step.

Mocking targets (mockGithub.rest.pulls.*, mockCore) are all external I/O — the GitHub API client and the Actions core logger — which is the legitimate and accepted mocking pattern for this codebase.

---

Language Support

Tests analyzed:

• 🟨 JavaScript (*.test.cjs): 2 tests (vitest)

No Go test files were modified in this PR.

---

Verdict

✅ Check passed. 0% of new tests are implementation tests (threshold: 30%). Both tests enforce observable behavioral contracts of the new non-fatal 422 handling.

📖 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 · ● 5.3M · ◷

[pelikhan]

@copilot review all comments and reviews

[Copilot]

@copilot review all comments and reviews

Reviewed all comments/reviews and applied the requested follow-up in 36c4914 (with the core typing fix in 9c7cde4). isNonFatalUpdateBranchError now extracts status through an explicit guarded cast for checkJs compatibility, and targeted typecheck + update_pull_request tests pass.