Handle deleted PR head refs non-fatally - #50051
Conversation
|
Hey
The issue itself is well-defined with clear scope and a working reference implementation already in the codebase. Once you have changes to show, the PR should be ready for review.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes #50019 by treating deleted PR head references as non-fatal during branch updates.
Changes:
- Classifies “head ref does not exist” independently of HTTP status.
- Adds regression coverage for a non-422 response.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/update_pull_request.cjs |
Updates non-fatal error classification. |
actions/setup/js/update_pull_request.test.cjs |
Verifies non-fatal handling with status 404. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
PR Triage: #50051
Small, focused fix (11+/8-) with all 12 CI checks passing. Prevents PR Sous Chef from failing entire batches on deleted head refs. Candidate for auto-merge.
|
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the implementation label and has 0 new lines of code in business logic directories. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: Request changes — the status guard was removed too broadly for the head ref does not exist match.
💡 Themes
- The fix correctly addresses the reported bug (stale head ref reported with unexpected status), but the implementation matches the message substring with no status constraint at all, rather than a bounded set like
404 | 422 | undefined. This risks silently swallowing genuinely fatal errors (e.g. 500s, network/proxy errors) that happen to contain this substring. - Test coverage only adds a 404 case; there's no test guarding against unrelated/unexpected statuses being misclassified as non-fatal.
- Everything else in the diff (comment updates, other status-gated conditions) looks accurate and consistent with the stated intent.
🔎 Code quality review by PR Code Quality Reviewer · auto · 14.5 AIC · ⌖ 4.57 AIC · ⊞ 7.9K
Comment /review to run again
Comments that could not be inline-anchored
actions/setup/js/update_pull_request.cjs:29
Matching head ref does not exist unconditionally on status (not just 404/422) means any non-fatal-status branch-update failure (500, 401, network-wrapped errors, etc.) whose message happens to contain this substring is now silently swallowed as non-fatal.
<details><summary>💡 Details</summary>
Before this change, the early-return guarded on status !== 422 (with 403 handled separately), so only 422/undefined/403 statuses could reach the final return. Now the guard is `status !== 422 && !me…
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — one actionable finding on the error-classification logic.
📋 Key Themes & Highlights
Key Themes
- Status-undefined gap: The new
message.includes("head ref does not exist")guard in thereturnexpression is not restricted to errors with a known HTTP status. A network/proxy error with nostatus(i.e.status === undefined) that contains this substring in its message would be treated as non-fatal, which may not be intended.
Positive Highlights
- ✅ Clean, minimal change — only the necessary lines are modified
- ✅ Test case is correctly flipped from fatal → non-fatal with the right expectation added
- ✅ Updated comments accurately reflect the new intent
- ✅ The early-return guard at line 55 correctly threads the
head ref does not existcase through to the finalreturn, avoiding double-evaluation
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 28.8 AIC · ⌖ 12.1 AIC · ⊞ 7.1K
Comment /matt to run again
| // the if-block above. | ||
| return ( | ||
| (status === 422 && (message.includes("there are no new commits on the base branch") || message.includes("merge conflict between base and head") || message.includes("head ref does not exist"))) || | ||
| message.includes("head ref does not exist") || |
There was a problem hiding this comment.
[/diagnosing-bugs] The message.includes("head ref does not exist") check here is not gated by any status — it matches even when status === undefined (network/proxy errors with no HTTP status), not just when the API returns a non-422 status. The PR description intends to handle the status-mismatch case, not the no-status case.
💡 Suggested guard
Consider:
return (
(status !== undefined && message.includes("head ref does not exist")) ||
(status === 422 && (message.includes("there are no new commits on the base branch") || message.includes("merge conflict between base and head"))) ||
((hasWorkflowsPermissionError || hasWorkflowsScopeRequired) && status === undefined)
);This keeps the status === undefined arm exclusive to workflows-permission errors as originally designed.
@copilot please address this.
There was a problem hiding this comment.
The change is correct and well-targeted. Extracting "head ref does not exist" from the status === 422 gate means deleted/force-pushed head branches are treated as non-fatal regardless of how an API proxy wraps the HTTP status. The accompanying test is updated to assert the new non-fatal path. No security, correctness, or reliability concerns.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 18.1 AIC · ⌖ 9.18 AIC · ⊞ 5.4K
🧪 Test Quality Sentinel Report✅ Test Quality Score: N/A — Minimal Test Modification
📊 Metrics (1 test)
🧪 Test AnalysisModified Test:
Context of Related Tests:
Quality Signals:
Verdict
Quality Signal: This change demonstrates good testing discipline — modifying test expectations to match intentional behavior changes, rather than adding redundant new tests. The existing test suite provides 3 complementary scenarios covering different status codes and continuation behavior.
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: Minimal Modification — Passed.
1 JavaScript test was correctly updated to reflect the new design invariant: deleted PR head refs are treated as non-fatal regardless of HTTP status code (previously only 422 was non-fatal).
Metrics:
- Modified tests: 1 (design-focused)
- Implementation-only tests: 0
- Edge-case coverage: Good (422 and 404 status codes tested)
- Test inflation: No (0.375:1 ratio)
- Violations: None
The test now correctly verifies that the behavior is non-fatal for any HTTP status when "head ref does not exist" is detected, complementing the existing 422-status tests.
|
@copilot sous-chef triage: Unresolved review threads remain:
Please address the unresolved review feedback, refresh the branch if needed, then run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot keep working |
Current head is
Addressed in |
|
🎉 This pull request is included in a new release. Release: |
PR Sous Chef could fail an entire batch when
update_pull_requestencountered a deleted or force-pushed head branch. The existing stale-target handling depended on a422status, which is not consistently preserved through API/proxy error wrapping.Safe-output error classification
head ref does not existbranch-update error as non-fatal regardless of reported HTTP status.Regression coverage
422status produces a non-fatal skip.