fix: rename branch when recreate-ref deleteRef is blocked by branch protection rules#33163
Conversation
…by branch protection rules
When `recreate_ref: true` and the remote `chaos/*` branch exists, the handler
calls `DELETE /git/refs/heads/{branch}`. If a repository ruleset blocks
deletion (HTTP 422 "Repository rule violations found / Cannot delete this
branch"), the previous code threw an error causing a fallback-to-issue.
Now, that specific 422 variant is caught and the handler falls back to the
existing rename-with-random-suffix strategy instead, so a PR is still created.
All other 422 variants (e.g. "Reference does not exist") and non-422 errors
retain their original behaviour.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a protected-branch fallback for recreate_ref collisions so PR creation can continue when deleting an existing remote ref is blocked by repository rules.
Changes:
- Handles 422 branch deletion rule violations by renaming the local branch with a random suffix instead of failing.
- Adds a test for the
chaos/*protected branch deletion scenario.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/create_pull_request.cjs |
Adds delete-blocked detection and falls through to existing branch renaming behavior. |
actions/setup/js/create_pull_request.test.cjs |
Adds coverage for protected-branch deletion failure fallback. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
| ref: "heads/chaos/preserve-me", | ||
| }); | ||
| // Should have fallen back to rename with suffix | ||
| expect(renameCalled).toBe(true); |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose and /tdd — this is a targeted bug fix with a new regression test.
Key Themes
- Regex precision (
/diagnose): The||condition on line 662 includes/Repository rule violations/iwhich is broader than the actual error case and could silently absorb unrelated 422s. The more specific/Cannot delete this branch/iclause alone is sufficient. - Test completeness (
/tdd): The new test is well-structured and covers the happy path through the fallback. One missing assertion: the final branch name returned inresult.branchis never checked, leaving the rename format unspecified.
Positive Highlights
- ✅ Clean, minimal code change —
deleteBlockedflag is easy to follow and doesn't alter any existing paths - ✅ Both GitHub error message variants (
Cannot delete this branchandRepository rule violations) are covered defensively - ✅ Test is realistic and covers the full observable behaviour (deleteRef called, warning emitted, rename executed,
success: true) - ✅ PR description is excellent — the before/after comment in the body makes the intent immediately clear
Verdict
Overall a solid, well-scoped fix. Two minor suggestions above — neither is blocking, but the regex tightening in particular is worth considering to avoid future surprises.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 3.2M
| // treat that as success and continue. | ||
| if (status === 422 && /Reference does not exist/i.test(message)) { | ||
| core.info(`Remote branch ${branchName} was already deleted concurrently; continuing`); | ||
| } else if (status === 422 && (/Cannot delete this branch/i.test(message) || /Repository rule violations/i.test(message))) { |
There was a problem hiding this comment.
[/diagnose] The /Repository rule violations/i branch of the || is quite broad — it will silently swallow any 422 that mentions those words, even violations unrelated to branch deletion (e.g. push rulesets, required status checks on a different flow). The concrete GitHub error message for a blocked delete always includes "Cannot delete this branch", so that first clause is sufficient and more precise. Consider removing the second clause or tightening it to something like /Repository rule violations.*Cannot delete/is.
// Safer: only match the specific phrase that appears when delete is blocked
} else if (status === 422 && /Cannot delete this branch/i.test(message)) {| ref: "heads/chaos/preserve-me", | ||
| }); | ||
| // Should have fallen back to rename with suffix | ||
| expect(renameCalled).toBe(true); |
There was a problem hiding this comment.
[/tdd] The test verifies renameCalled and the warning messages, but never asserts on the shape of the renamed branch. The result object (from handler(...)) carries the final branch name — asserting it matches the expected pattern would complete the specification:
expect(result.branch).toMatch(/^chaos\/preserve-me-[0-9a-f]{8}$/);Without this, a refactor that changes the suffix format (e.g. longer hex, different separator) would still pass the test.
🧪 Test Quality Sentinel ReportTest Quality Score: 90/100✅ Excellent
Test Classification Details
AnalysisThis test covers the new error-handling branch introduced in Behavioral assertions verified:
All assertions target observable behavior or user-facing outputs. The mock of Test inflation note: 45 test lines were added for 13 production lines (≈3.5:1 ratio, exceeding the 2:1 guideline). This is flagged but not surprising — the test requires significant setup mocking (exec, getExecOutput, github.rest.git.deleteRef) to simulate the protected branch scenario. The complexity is justified. Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. References: §26061303760
|
|
@copilot review all comments |
…eleteRef is blocked Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in the latest commit. The test now:
A regression that renames locally but still pushes/creates the PR against the original branch would now fail assertions 3–5. |
chaos/*branches are protected by a ruleset that blocks deletion. Whenrecreate_ref: trueand the remote branch already exists, the handler callsDELETE /git/refs/heads/{branch}— which returns HTTP 422 "Repository rule violations found / Cannot delete this branch" — causing a hard failure and fallback-to-issue instead of creating a PR.Changes
create_pull_request.cjs—handleRemoteBranchCollision: add a second 422 catch arm matchingCannot delete this branch/Repository rule violations. Instead of throwing, setsdeleteBlocked = trueand falls through to the existing rename-with-random-suffix path, so the PR is still created under{branch}-{hex}.create_pull_request.test.cjs: new test covering thechaos/*ruleset violation scenario — verifiesdeleteRefis attempted, a protection-rules warning is emitted, the branch is renamed, and the result issuccess: true.All other error paths are unchanged:
422 "Reference does not exist"(concurrent delete) is still treated as success; any other status still throws.