Conversation
…ve-branch-name is true Agent-Logs-Url: https://github.com/github/gh-aw/sessions/42a2353b-dfa9-404d-a067-2b28b74fa140 Co-authored-by: mrjf <180956+mrjf@users.noreply.github.com>
…nch-name is enabled Agent-Logs-Url: https://github.com/github/gh-aw/sessions/42a2353b-dfa9-404d-a067-2b28b74fa140 Co-authored-by: mrjf <180956+mrjf@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adjusts create_pull_request behavior so preserve-branch-name: true can reuse an existing remote branch instead of failing, enabling long-lived reusable branch workflows.
Changes:
- Update remote-branch collision handling to delete the existing remote ref (when preserving branch name) and proceed with recreating the branch on push.
- Update unit tests to assert reuse behavior and cover deleteRef failure behavior.
- Update documentation and add a patch changeset describing the new reuse semantics.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/create_pull_request.cjs | Reworks remote-branch collision logic to reuse existing remote branch under preserve-branch-name via deleteRef + recreate on push. |
| actions/setup/js/create_pull_request.test.cjs | Updates assertions for reuse behavior; adds coverage for deleteRef failure path; extends GitHub REST mock. |
| docs/src/content/docs/reference/safe-outputs-pull-requests.md | Documents the new reuse behavior for preserve-branch-name when the remote branch already exists. |
| .changeset/patch-preserve-branch-name-reuse-existing-branch.md | Adds a patch changeset entry describing the behavior change. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
actions/setup/js/create_pull_request.test.cjs:1765
- This test name says it "should fall back to issue", but the test config sets
fallback_as_issue: falseand assertserror_type === "push_failed"(no fallback). Rename the test to match the behavior being verified (e.g., that deleteRef failures surface as push_failed when reuse is attempted).
it("should fall back to issue when deleteRef fails for preserve-branch-name reuse", async () => {
global.exec = {
exec: vi.fn().mockResolvedValue(0),
getExecOutput: vi.fn().mockImplementation((cmd, args) => {
const cmdStr = typeof cmd === "string" ? cmd : `${cmd} ${(args || []).join(" ")}`;
if (cmdStr.includes("ls-remote --heads origin")) {
return Promise.resolve({ exitCode: 0, stdout: "abc123\trefs/heads/preserve-me\n", stderr: "" });
}
return Promise.resolve({ exitCode: 0, stdout: "", stderr: "" });
}),
};
// Simulate deleteRef failing with a non-recoverable error
global.github.rest.git.deleteRef = vi.fn().mockRejectedValue(Object.assign(new Error("Forbidden"), { status: 403 }));
const { main } = require("./create_pull_request.cjs");
const handler = await main({ preserve_branch_name: true, fallback_as_issue: false });
- Files reviewed: 4/4 changed files
- Comments generated: 2
| `branch name, or disable preserve-branch-name to allow a random suffix to be appended.` | ||
| ); | ||
| // Reuse the existing branch by deleting the remote ref so the subsequent | ||
| // push recreates it from the local HEAD (force-push semantic). This is the |
There was a problem hiding this comment.
The phrase "force-push semantic" is ungrammatical here; this is describing overall behavior, so "force-push semantics" (plural) is clearer.
| // push recreates it from the local HEAD (force-push semantic). This is the | |
| // push recreates it from the local HEAD (force-push semantics). This is the |
| The `preserve-branch-name` field, when set to `true`, omits the random hex salt suffix that is normally appended to the agent-specified branch name. This is useful when the target repository enforces branch naming conventions such as Jira keys in uppercase (e.g., `bugfix/BR-329-red` instead of `bugfix/br-329-red-cde2a954`). Invalid characters are always replaced for security, and casing is always preserved regardless of this setting. Defaults to `false`. | ||
|
|
||
| When `preserve-branch-name: true` and the agent-supplied branch name already exists on the remote, the workflow fails with an explicit error rather than silently appending a random suffix. To resolve, delete the existing remote branch, choose a different branch name, or disable `preserve-branch-name` to allow collision-avoidance via a random suffix. | ||
| When `preserve-branch-name: true` and the agent-supplied branch name already exists on the remote, the workflow reuses the existing branch by force-deleting the stale remote ref and recreating it from the agent's local HEAD (force-push semantic). This is the intended behavior for long-lived reusable branches whose previous PR was merged. The handler does not silently rename the branch in this case. |
There was a problem hiding this comment.
The phrase "force-push semantic" reads like a grammatical error; this describes behavior/meaning, so "force-push semantics" (plural) is clearer and consistent with common usage. Consider updating this wording here and anywhere else it appears in nearby docs/comments.
| When `preserve-branch-name: true` and the agent-supplied branch name already exists on the remote, the workflow reuses the existing branch by force-deleting the stale remote ref and recreating it from the agent's local HEAD (force-push semantic). This is the intended behavior for long-lived reusable branches whose previous PR was merged. The handler does not silently rename the branch in this case. | |
| When `preserve-branch-name: true` and the agent-supplied branch name already exists on the remote, the workflow reuses the existing branch by force-deleting the stale remote ref and recreating it from the agent's local HEAD (force-push semantics). This is the intended behavior for long-lived reusable branches whose previous PR was merged. The handler does not silently rename the branch in this case. |
🧪 Test Quality Sentinel ReportTest Quality Score: 88/100✅ Excellent test quality
Test Classification DetailsView Per-Test Classification
Minor Observations (Non-Blocking)
Language Support
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: §25120480700
|
|
@copilot review all comments |
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/73c62235-cc79-479b-b145-e8529c7c8ee0 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot add a boolean field |
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. |
… branch Agent-Logs-Url: https://github.com/github/gh-aw/sessions/6e0849fa-2983-466f-a75a-b9ebf64dfabb Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Added
Surfaced via Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot rename remove-create-ref into recreate-ref |
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/2bfd1f6a-b277-46a4-bc57-1a2801884bae Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Renamed Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot do a deep review, use w3c-spec-writer to update spec |
…tputs spec Agent-Logs-Url: https://github.com/github/gh-aw/sessions/86fca68e-f8dc-4220-9001-fb2c1de35a52 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Did a deep review and updated the W3C-style spec at
Note: I don't have the Code review found no other issues — naming is consistent across cjs/Go/schema/tests, the cjs handler correctly threads |
With
preserve-branch-name: true,create_pull_requestpreviously aborted (and fell back to opening an issue) whenever the target branch already existed on the remote. This breaks workflows like autoloop that intentionally maintain one long-lived branch per program: after a PR merges, the branch persists, so the next iteration'screate_pull_requestcall always collides.This PR introduces a new opt-in
recreate-refboolean field that gates the force-delete-and-recreate behavior. When enabled together withpreserve-branch-name: true, the handler force-deletes the stale remote ref and lets the subsequent push recreate it from the agent's local HEAD (force-push semantics). Whenrecreate-refis omitted orfalse(the default), an existing remote branch underpreserve-branch-name: trueresults in a fallback (e.g. opening an issue) rather than overwriting the remote ref.Changes
actions/setup/js/create_pull_request.cjs—handleRemoteBranchCollisionnow accepts arecreateRefoption. Whenpreserve-branch-name: trueand the remote branch exists, it force-deletes the stale ref viagithubClient.rest.git.deleteRefonly ifrecreateRefis true, letting the subsequentpushSignedCommitsrecreate the branch from the local HEAD's parent OID throughcreateCommitOnBranch(force-push semantics without needing git credentials at the agent step). WhenrecreateRefis false, it throws so the caller falls back. Concurrent-deletion 422s are swallowed; other failures surface aspush_failed. The three call sites now thread{ recreateRef, githubClient, owner, repo }into the helper.RecreateRef boolfield toCreatePullRequestsConfig, surfaced asrecreate_refin the handler config JSON, and added therecreate-refboolean property topkg/parser/schemas/main_workflow_schema.json.recreate_ref: true(assertsdeleteRefcalled withheads/<branch>, no local rename,success: true). Added a new test for the fallback path whenpreserve-branch-name: trueandrecreate_refis omitted (assertspush_failed, nodeleteRefcall). Kept coverage for the deleteRef-failure path. Shared mock gainsrest.git.deleteRef.safe-outputs-pull-requests.mdupdated to describe the opt-inrecreate-refbehavior; changeset updated accordingly.safe-outputs-specification.md(W3C-style spec) updated to documentpreserve-branch-nameandrecreate-refin thecreate_pull_requestsection, including new operational semantics for branch name normalization and remote branch collision handling, an updated configuration parameters list, and a behavior matrix.Behavior matrix
preserve-branch-namerecreate-reftruetruetruetruetruefalse(default)truefalse(default)falsefalse