Fail loudly when preserve-branch-name collides with existing remote branch

[Copilot]

safe-outputs.create-pull-request.preserve-branch-name: true was silently bypassed whenever the agent-supplied branch already existed on the remote: the collision handler unconditionally appended an 8-char hex salt, defeating the flag's purpose and producing a growing pile of disconnected PRs for long-running branches.

Root cause: three identical remoteBranchExists blocks in actions/setup/js/create_pull_request.cjs (signed-commits, patch-apply, empty-commit paths) did not consult preserveBranchName.

Changes

• actions/setup/js/create_pull_request.cjs: extract remote-branch-collision handling into a shared handleRemoteBranchCollision(branchName, preserveBranchName) helper used by all three push paths (signed-commits, patch-apply, allow-empty). When preserveBranchName is true and the remote branch already exists, the helper throws an actionable error naming the branch and pointing at the three remediation paths (delete the remote branch, rename, or disable the flag). Otherwise it preserves the existing behavior of renaming the local branch with a random hex suffix. The surrounding try/catch propagates this as a push_failed result, optionally honoring fallback-as-issue.

• Allow-empty push-failure consistency: the catch (pushError) in the allow-empty empty-branch push path now returns error_type: "push_failed", matching the signed-commits and patch-apply paths so callers see a consistent error shape across all three push paths.

• docs/src/content/docs/reference/safe-outputs-pull-requests.md: document the fail-loud collision behavior alongside the existing preserve-branch-name description.

• actions/setup/js/create_pull_request.test.cjs: two regression tests — one asserting the new fail-loud path emits the expected error and does not warn about appending a suffix; one confirming the default (flag off) path still appends a random suffix.

Notes for reviewers

• Chose option 2 from the issue (fail loudly) over option 1 (push onto existing branch). Pushing onto an existing remote branch from a local branch created off base would require either a force-push (history loss) or a fetch-and-rebase flow — both materially larger than the surgical fix and arguably their own feature (closer to push-to-pull-request-branch).

[github-actions]

🧪 Test Quality Sentinel Report

Test Quality Score: 90/100

✅ Excellent

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	⚠️ Yes (57 test lines / 21 production lines ≈ 2.7:1)
🚨 Coding-guideline violations	None

---

Test Classification Details

Test	File	Classification	Issues Detected
should fail loudly when preserve-branch-name is true and remote branch already exists	actions/setup/js/create_pull_request.test.cjs	✅ Design	None — verifies error path, correct error message, and absence of silent bypass
should append random suffix when preserve-branch-name is false and remote branch already exists	actions/setup/js/create_pull_request.test.cjs	✅ Design	Minor: renameCalled flag checks internal side-effect; main behavioral assertions are present

---

Flagged Tests — Requires Review

No tests require mandatory review. One minor note below.

i️ should append random suffix when preserve-branch-name is false and remote branch already exists

Classification: Design test (minor implementation detail noted)
Note: The renameCalled = true assertion verifies that git branch -m was called internally, which is an implementation detail. However, the test also asserts result.success === true and the presence of the warning message — observable behavioral outputs — so this does not lower the classification.
Suggested improvement (optional): Replace or supplement renameCalled with an assertion on the final branch name present in the result or a commit log check, to make the test more resilient to internal refactoring.

---

Test Inflation Note

The test file added 57 lines against 21 production lines (≈ 2.7:1 ratio), triggering the inflation threshold (> 2:1). In this case the extra lines are justified: each test requires async mock setup for global.exec.getExecOutput, require() cache invalidation, and multiple behavioral assertions. No penalty concern in practice, but flagged for transparency.

---

Language Support

Tests analyzed:

• 🟨 JavaScript (*.test.cjs): 2 tests (vitest)
• 🐹 Go (*_test.go): 0 tests

---

Verdict

✅ Check passed. 0% of new tests are implementation tests (threshold: 30%). Both new tests verify observable behavioral contracts: one confirms the system fails loudly (with an actionable error message) when preserve-branch-name conflicts with an existing remote branch, and the other confirms the pre-existing random-suffix fallback continues to work when preserve is disabled. Mocking targets (global.exec) are external I/O — legitimate.

---

📖 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.

References: §24697555931

🧪 Test quality analysis by Test Quality Sentinel · ● 397.2K · ◷

[github-actions]

✅ Test Quality Sentinel: 90/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). Both new JavaScript tests verify behavioral contracts with full error/edge case coverage.

[Copilot]

Pull request overview

This PR ensures safe-outputs.create-pull-request.preserve-branch-name: true is no longer silently bypassed when the agent-supplied branch already exists on the remote, by failing loudly with an actionable error instead of auto-renaming the branch.

Changes:

• Update all three remote-branch collision handlers to throw an explicit error when preserve-branch-name is enabled.
• Document the new fail-loud collision behavior for preserve-branch-name.
• Add regression tests covering fail-loud (flag on) and suffix-append (flag off) behavior.

Show a summary per file

File	Description
actions/setup/js/create_pull_request.cjs	Throws an explicit error on remote-branch collisions when preserve-branch-name is enabled; otherwise retains suffix-append behavior.
actions/setup/js/create_pull_request.test.cjs	Adds tests validating the new fail-loud path and the existing default collision-avoidance behavior.
docs/src/content/docs/reference/safe-outputs-pull-requests.md	Documents that collisions now fail explicitly when preserve-branch-name: true.

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: 2

[Copilot]

The actionable collision error message is duplicated verbatim in three separate remote-branch collision blocks (bundle push, patch push, and allow-empty push). To reduce drift risk and make future edits safer, consider extracting this message into a small helper (e.g., makePreserveBranchNameCollisionError(branchName)) or a shared constant.

[Copilot]

In the allow-empty (empty patch) push path, this new throw ends up in the catch (pushError) that returns { success: false, error } without error_type: "push_failed" and without honoring fallback-as-issue (unlike the bundle and patch paths). If callers rely on error_type/fallback consistency (and per the PR description), consider mapping this failure to the same push_failed structure and/or using the same fallback-as-issue behavior here.

[pelikhan]

@copilot review all comments

[Copilot]

@copilot review all comments

Addressed both reviewer comments in 65bfa28:

1. Extracted the three duplicated collision blocks into a shared handleRemoteBranchCollision(branchName, preserveBranchName) helper near the top of the file. The error message now lives in a single place.
2. The allow-empty push-failure catch now returns error_type: "push_failed" to be consistent with the signed-commits and patch-apply paths.

All 63 tests in create_pull_request.test.cjs still pass.