Add --delete-branch to branch merge#354
Conversation
POST /branches/merge now takes an optional delete_branch field that deletes the source branch after the merge lands. The deletion is composed in the handler under its own branch_delete authorization (mirroring the standalone delete route), so a merge permission never implies a delete permission. Because the merge is durable by the time the deletion runs, every deletion failure -- policy denial, dependent descendant branch, operational error -- is reported via the new branch_deleted / branch_delete_error response fields instead of failing the request. Deletion runs on every successful outcome, including already_up_to_date (the already-merged cleanup case). Both wire fields are additive (serde default / skip-if-none), so older clients and requests are unaffected. The CLI request/response literals gain the new fields with no behavior change; the flag itself lands in the next commit.
The flag composes merge-then-delete at each arm's own boundary: the embedded arm runs the two engine calls exactly like the server handler, and the remote arm sends the delete_branch request field. A deletion refusal keeps exit code 0 (the merge landed) and prints a stderr warning alongside the branch_deleted / branch_delete_error payload fields; stdout JSON stays clean. If a pre-delete_branch server ignores the request field, the missing branch_deleted response field is detected and surfaced as a warning rather than silently leaving the branch behind. The parity matrix gains a --delete-branch row so the two composition sites cannot drift, and cli_data covers both the deletion and the non-fatal dependent-branch refusal.
Merged branches are not cleaned up automatically and every live branch caps how far cleanup can GC the main versions it inherited, so the branching docs now state the intended lifecycle (create, write, merge, delete) and describe --delete-branch / delete_branch on the merge surfaces, including the non-fatal refusal semantics and the separate branch_delete policy check.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| #[serde(default)] | ||
| pub delete_branch: bool, |
There was a problem hiding this comment.
delete_branch changes the public merge request, but the checked-in openapi.json still lacks delete_branch, branch_deleted, and branch_delete_error. Generated clients will not expose the new merge-and-delete path or its failure state, and the server OpenAPI drift check can fail until the spec is regenerated with these fields.
Context Used: AGENTS.md (source)
There was a problem hiding this comment.
Valid — the drift check runs strict on PRs (the auto-commit step is post-merge-only), so the spec had to be regenerated locally. Fixed in b9c40ab (regenerated openapi.json with delete_branch, branch_deleted, branch_delete_error); a8c2ecb also corrects the stale testing.md claim that CI auto-commits the regeneration on same-repo PRs.
There was a problem hiding this comment.
Valid — the drift check runs strict on PRs (the auto-commit step is post-merge-only), so the spec had to be regenerated locally. Fixed in b9c40ab (regenerated openapi.json with delete_branch, branch_deleted, branch_delete_error); a8c2ecb also corrects the stale testing.md claim that CI auto-commits the regeneration on same-repo PRs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3645314ad0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| handle | ||
| .engine | ||
| .branch_delete_as(source, actor_id) |
There was a problem hiding this comment.
With delete_branch: true, this starts a fresh delete after branch_merge_as has returned, so the source branch is no longer pinned to the head that was just merged. If another request writes to source in that gap (or was already queued behind the merge and wins the next branch gate), branch_delete_as will delete that newer unmerged head because branch deletion only checks descendants/dependencies, not that the source still matches the merged head. Please carry a source-head token through the delete or report branch_deleted: false when the source head changed; the embedded client has the same two-call race.
Useful? React with 👍 / 👎.
| #[serde(default)] | ||
| pub delete_branch: bool, |
There was a problem hiding this comment.
This new request field (and the new BranchMergeOutput deletion fields) are live in the utoipa types, but the checked-in root openapi.json was not regenerated: its BranchMergeRequest still only exposes source/target, and BranchMergeOutput lacks branch_deleted/branch_delete_error. SDKs generated from the committed spec cannot discover or use delete_branch, and the openapi_spec_is_up_to_date drift test will fail for this API change.
Useful? React with 👍 / 👎.
The auto-commit step lives in the heavy test job, which is post-merge only, so the drift check runs strict on pull requests; regenerate locally and commit the result. The stale claim that CI auto-commits the regeneration on same-repository PRs is what let this branch push an outdated openapi.json in the first place.
1 participant
What & why
Merged source branches are never cleaned up automatically: they linger in
branch listand pin the main versions they inherited againstcleanupGC. This adds opt-in merge-then-delete composition —delete_branchonPOST /branches/mergeand--delete-branchonomnigraph branch merge— so the blessed lifecycle (create → write → merge → delete) is one step for both HTTP consumers and the CLI.Backing issue / RFC
Maintainer change (per the maintainer process noted above).
Checklist
branch_deleteauthorization at each boundary, so a merge permission never implies a delete permissionNotes for reviewers
branch_merge; the parity matrix gains a--delete-branchrow that pins the two composition sites against drift.already_up_to_date(the "already merged, clean me up" case), and every deletion failure — Cedar denial, dependent descendant branch, operational error — is non-fatal by design: the merge is durable by then, so it is reported viabranch_deleted: false+branch_delete_error(CLI: exit 0 + stderr warning). The CLI also warns when an older server silently ignores the unknown request field.openapi.jsonregeneration is left to CI's auto-commit on this PR (local regen was still queued behind a cold build); the TypeScript SDK's vendored spec copy should be re-synced after merge.branch listand acleanupretention-pinning report.Note
Medium Risk
Touches branch merge API and destructive branch_delete composition with policy boundaries; behavior is well-tested and merge success is isolated from delete failures, but operators must understand merge can succeed while delete does not.
Overview
Adds opt-in merge-then-delete so merged feature branches can be removed in one step instead of lingering in
branch listand pinning cleanup GC.API & CLI:
BranchMergeRequestgainsdelete_branch(default false);BranchMergeOutputreportsbranch_deletedandbranch_delete_error. The CLI exposesomnigraph branch merge --delete-branchand threads the flag through remote and embedded clients.Composition (not in the engine): After a successful merge (including
already_up_to_date), the server handler and the CLI embedded path optionally callbranch_deleteunder a separatebranch_deletepolicy check. Merge permission never implies delete permission. Any delete refusal or failure stays non-fatal—HTTP still returns 200 with the merge outcome; the CLI exits 0 and prints stderr warnings (including when an older server omits the new response fields).Tests & docs: Coverage for successful delete, dependent-branch refusal, and policy denial on delete; parity matrix row for embedded vs remote. User docs and
openapi.jsonupdated for the new contract.Reviewed by Cursor Bugbot for commit a8c2ecb. Bugbot is set up for automated code reviews on this repo. Configure here.
Greptile Summary
This PR adds opt-in source-branch deletion after a successful branch merge. The main changes are:
delete_branchon the merge request and deletion status fields on the response.--delete-branchsupport in the CLI for remote and embedded graphs.Confidence Score: 5/5
This looks safe to merge.
Important Files Changed
Reviews (2): Last reviewed commit: "Correct the OpenAPI drift check descript..." | Re-trigger Greptile
Context used: