Skip to content

fix(server): delete a branch named like a static branch route - #1091

Merged
ragnorc merged 3 commits into
ModernRelay:mainfrom
ragnorc:fix/branch-names-beside-static-routes
Oct 20, 2026
Merged

ragnorc merged 3 commits into
ModernRelay:mainfrom
ragnorc:fix/branch-names-beside-static-routes

Conversation

@ragnorc

@ragnorc ragnorc commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

Why

merge, diff and merge-base are valid branch names. The router matches the static routes /branches/merge, /branches/diff and /branches/merge-base before /branches/{branch}. So DELETE /graphs/{graph_id}/branches/merge reached the POST-only merge route and answered 405, and such a branch could not be deleted over HTTP. An API audit found this, and the HTTP API conventions draft (#1089) records the rule that keeps it from recurring: no static segment beside a parameter.

What

Each of the three static routes now also answers DELETE, which deletes the branch it spells. It goes through server_branch_delete_named, the body DELETE /branches/{branch} already runs: the same policy check, admission and engine call. The spec is unchanged, since the operation is the documented DELETE /branches/{branch}.

Tests

  • data_routes::remote_branch_delete_flow_works now creates and deletes feature, merge, diff and merge-base. Against the old router it fails on the 405, whose body is not JSON. With the fix it passes.
  • The openapi:: tests pass, and clippy passes with -D warnings.

Backing issue / RFC

Branch names `merge`, `diff` and `merge-base` are valid, but the router
matches the static routes `/branches/merge`, `/branches/diff` and
`/branches/merge-base` before `/branches/{branch}`, so deleting such a
branch over HTTP answered 405 and the branch could not be removed. Each
static route now also answers DELETE by deleting the branch it spells,
through the same body as `DELETE /branches/{branch}`. The delete flow
test creates and deletes `merge`, `diff` and `merge-base` beside
`feature`; it fails with the 405 against the old router.
@ragnorc
ragnorc requested a review from aaltshuler as a code owner October 19, 2026 20:16
@cursor

cursor Bot commented Oct 19, 2026

Copy link
Copy Markdown

Recommendation: request changes. Review of head 4a50c1890ba378923573b709edb0224df6f329fe.

Why

DELETE /graphs/{graph_id}/branches/{branch} is the documented way to delete a branch. The router also registers static routes at /branches/merge, /branches/diff, and /branches/merge-base. matchit 0.8.4 selects the static route for that exact segment. Axum 0.8 then answers 405 for every method that static route does not serve. It does not continue to /branches/{branch}.

merge, diff, and merge-base are valid branch names. Create accepts them. DELETE /graphs/{graph_id}/branches/merge therefore cannot delete that branch. The response is 405, Allow: POST, and an empty body. diff and merge-base answer 405 with Allow: GET, HEAD.

What this change does

Each static route now also serves DELETE. The closure calls server_branch_delete_named with a literal name. That function runs branch_delete_body, the same policy check, admission, and branch_delete_as as DELETE /branches/{branch}.

data_routes::remote_branch_delete_flow_works creates and deletes feature, merge, diff, and merge-base, then expects the list to be main only.

Affected class

The broken contract is one rule. Every live branch name that create accepts can be removed with DELETE /graphs/{id}/branches/{name}.

Only three requests break that rule today. The raw path segment is exactly merge, diff, or merge-base, and the method is DELETE.

These paths are a different case:

  • %6D%65%72%67%65 is a different raw segment. matchit binds {branch}. Axum decodes it to merge. On the old router this DELETE answers 404 branch 'merge' not found. The CLI does not send this form. remote_url leaves unreserved characters unchanged.
  • A name that contains /, such as merge/child, travels as one segment merge%2Fchild. The old router decodes it and looks up merge/child. An unencoded slash is two segments and matches no route. That is the existing one-segment rule. The CLI encodes /.
  • No route exists under /branches/{branch}/....
  • GET /graphs/discovery is one static path. A graph id discovery is a valid id. /graphs/discovery/snapshot falls through to {graph_id}. No current graph operation uses the exact path /graphs/discovery.
  • /graphs/og_catalog is a static tree with a catch-all. It would hide a user graph of that id. Admission refuses ids with the prefix og_. Reservation is the right close for that system scope.
  • /commits/{commit_id} has child segments publish and changes. It has no static sibling. Commit ids have the form hb1.<block>.<slot>.<nonce>. GET /commits/publish binds {commit_id}.
  • /queries/{name} has the child if-graph-commit. A query named if-graph-commit is a valid id. POST /queries/if-graph-commit binds {name}.
  • /cluster/deployments/{id} has no static sibling. /cluster/plan is a different path. An id plan binds {id}.
  • This server has no /actors or /tokens collection.
  • MCP branches_delete calls server_branch_delete in process. GQ branch delete on POST /mutate calls branch_delete_body. Both already delete these names. A .gqt case runs the engine, so it cannot show this 405.

Create refuses an empty name, surrounding whitespace, a control character, main, an incarnation-shaped suffix, and a live path-prefix clash. Lance check_valid_branch then checks the native ref {logical}.{ULID}. It refuses a leading or trailing /, //, .., \, a segment outside alphanumeric plus . - _, and a name that ends in .lock. The Lance 11 branch page states the same rules. merge, diff, and merge-base pass both checks. Reserving them at create would change the branch-name contract on the engine, the CLI, GQ, and MCP in order to hide an HTTP route clash. #1089 plans to move these actions off the collection, so the reserved words would then be obsolete.

Blocking

crates/omnigraph-server/src/lib.rs lines 2765-2788.

The three DELETE closures make today's delete succeed. They leave two routes on one URL. The static route still wins. The next method on {branch}, or the next static sibling, answers 405 again unless a closure is copied. The deleted name is also a string literal beside the path, so the URL and the branch can drift.

Register one route, /branches/{branch}. Dispatch inside it:

  1. POST of the name merge runs the merge body.
  2. GET of the name diff runs the branch-diff body. GET also serves HEAD, which is the current Allow: GET, HEAD.
  3. GET of the name merge-base runs the merge-base body.
  4. DELETE of any name deletes that branch, using the captured segment.
  5. Every other method and name answers 405, as those paths do now.

Keep the current URLs. Keep branch_delete_body as the one delete body. Do not reserve the three names. Do not move the paths in this pull request. That move is a contract break, and #1089 already places it in a later batch.

Tradeoff and liability

This pull request removes a real hole. A remote branch delete uses DELETE /branches/{name} (client.rs, and #1090 keeps that shape). Those three deletes start to work, with the same policy and engine call as any other branch.

It adds a copy. Five more static actions under /branches produce five more DELETE closures, and each new method on {branch} must be added to every sibling. The clash remains until the path move.

One dispatcher is still a special case. It lives in one match, at the router that owns the clash, and the deleted name comes from the path. The path move in #1089 removes the special case. That is the right long-term change. It is out of scope for a fix that must keep current clients on these URLs.

Spec and docs

OpenAPI and the branch guide need no change for this bug. cluster_deleteBranch is already DELETE /branches/{branch}. utoipa reads the handler attributes, not the axum method list. openapi::openapi_spec_is_up_to_date passes on this head. A second DELETE on the static path items would publish the workaround that #1089 removes. The changelog note is the right user-visible record.

A gateway that selects the concrete path /branches/merge still sees POST only. The server accepts DELETE on that URL after this change. The path move closes that gap.

Sibling pull requests

#1089 records this 405 as a separate bug and schedules the path move later. This pull request matches that sequence. The closure pattern should not become the way the next clash is fixed.

#1076 renames the path extractor to ApiPath on server_branch_delete and turns 405 into a JSON body. Expect a conflict in handlers.rs. #1016 adds from_commit on BranchCreateRequest. The struct literal in the delete test must name that field when both land. #1074, #1075, and #1077 add layers in build_app. #1012 changes the branch list. None of them change this precedence.

Validation

Reviewed head 4a50c1890ba378923573b709edb0224df6f329fe. Base 9e2425d26a03b1b5628030a359ee51f096d3ca1c. Isolated checkout. CARGO_TARGET_DIR=/workspace/target.

GQT cannot express this contract. The owner is omnigraph-server data_routes.

On the head:

cargo test -p omnigraph-server --test integration remote_branch_delete -- --test-threads=1 --nocapture

Result: remote_branch_delete_flow_works passed. A temporary test that deletes merge/child through merge%2Fchild, and merge through %6D%65%72%67%65, also passed.

On the head, exact names:

  • data_routes::remote_branch_list_create_merge_flow_works passed. POST /branches/merge still merges.
  • data_routes::diff_routes_review_a_branch_against_its_merge_base passed. GET diff and merge-base still answer.
  • openapi::openapi_spec_is_up_to_date passed.

Before, the same delete tests with lib.rs and handlers.rs restored from 9e2425d2:

remote_branch_delete_flow_works failed in json_response (support/mod.rs:513) with EOF while parsing a value. That is the empty 405 body. The temporary encoded-name test passed on that old router. Those spellings already reached {branch}.

A status probe with no branch created, old router:

  • DELETE /branches/feature answers 404 branch 'feature' not found.
  • DELETE /branches/merge answers 405, Allow: POST, empty body.
  • DELETE /branches/diff answers 405, Allow: GET, HEAD, empty body.
  • DELETE /branches/merge-base answers 405, Allow: GET, HEAD, empty body.
  • DELETE /branches/%6D%65%72%67%65 answers 404 branch 'merge' not found.
  • DELETE /branches/merge%2Fchild answers 404 branch 'merge/child' not found.

The same probe on the head: the three literal names answer 404 branch '<name>' not found. The encoded paths stay 404.

A separate matchit 0.8.4 probe confirmed the path choices for discovery, og_catalog, commits, queries, and deployments. That probe is not the server.

Limitations: I did not run the full server suite, Clippy, or a live CLI process. Temporary tests were removed. Nothing was pushed.

Comment thread crates/omnigraph-server/src/lib.rs
The previous fix gave each static branch action a DELETE closure, which
left two routes on one URL: the next method on `/branches/{branch}` or
the next static sibling would answer 405 again unless a closure were
copied, and each deleted name was a literal beside its path. One route
now owns `/branches/{branch}`: POST of `merge` runs the merge, GET of
`diff` or `merge-base` runs that read (HEAD follows GET), DELETE of any
captured name deletes that branch through the one delete body, and every
other method and name answers 405 with an `Allow` header naming what that
name serves. URLs, the spec and `branch_delete_body` are unchanged.

ragnorc commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

The blocking finding on 4a50c189 is resolved. One route now owns /branches/{branch}. I recommend approval. Two notes below are optional.

Why

A branch may be named merge, diff, or merge-base. Lance 11 allows those names, and OmniGraph allows them. DELETE /graphs/{graph_id}/branches/<name> answered 405. Axum matched the static route of that name before /branches/{branch}, so the branch stayed.

What this commit does

5c2dc29c removes /branches/merge, /branches/diff, and /branches/merge-base. One route, /branches/{branch}, dispatches by name and method:

  • POST of merge runs server_branch_merge.
  • GET of diff runs server_branch_diff. GET of merge-base runs server_merge_base. HEAD follows GET, so Axum still strips the body after the read.
  • DELETE of the captured name runs server_branch_delete, which calls branch_delete_body.
  • A GET, HEAD, or POST that the name does not serve answers 405. branch_method_not_allowed sets Allow to the methods that name serves: DELETE, plus POST for merge, or GET, HEAD for diff and merge-base.

The URLs, the OpenAPI paths, and branch_delete_body stay as they are. The three DELETE closures and the literal names beside the path are gone. The names are not reserved at create.

Optional

  1. crates/omnigraph-server/src/lib.rs, the /branches/{branch} route. PUT, PATCH, OPTIONS, TRACE, and CONNECT never enter branch_method_not_allowed. Axum rejects them on the method router and sets the route-level Allow value POST,GET,HEAD,DELETE. That list names methods the name does not serve. Before this change, PUT /branches/feature answered Allow: DELETE. It now answers Allow: POST,GET,HEAD,DELETE. GET, HEAD, POST, and DELETE already get the per-name header. Add a fallback on this route and set the same per-name Allow there. Axum keeps an Allow header that the fallback sets (axum 0.8.9, allow_header_with_fallback_that_sets_allow in method_routing.rs). Drive the fallback and the two match arms from BRANCH_ACTIONS, so the next action cannot update one and leave the other behind.

  2. changelog.d/branch-delete-beside-static-routes.fixed.md. The changelog still describes the closure design this commit removed: "each of those routes now deletes the branch it spells." The first sentence is the user outcome, and it is true. Replace the second sentence. The comment on remote_branch_delete_flow_works still says the router matches the static route first. That comment is now false.

Tradeoff and liability

The violated contract is the HTTP interface: DELETE /branches/{name} must delete a valid branch name. The router owns that contract. This fix is at that layer. It does not reserve the three names. check_valid_branch in Lance 11.0.0 allows a non-empty name whose segments are alphanumeric plus ., -, and _, with / between segments. It refuses main, a leading or trailing /, //, .., \, and a name that ends in .lock. OmniGraph then refuses an empty name, surrounding whitespace, a control character, and an incarnation-shaped suffix. merge, diff, and merge-base pass both checks. A reservation at create would change the branch-name contract on the engine, the CLI, GQ, and MCP in order to hide a route clash. That is the wrong layer. Draft #1089 already plans to move these actions off the collection, so reserved words would then be obsolete.

This removes the precedence hole and the three copied closures. The next action is one match arm, not a new static sibling. Five more actions stay in one function. The remaining special case is the name match, and the path move in #1089 removes it. Keeping the current URLs is the smaller break. A path move in this pull request would break current clients.

The extra work on this path is one string match. That cost is not material. Correctness of the URL is the goal. Simplicity of a later path move stays available. This change does not add a second source of truth for branch identity, policy, or publication.

Other collections

I read the route table in build_app. The branch collection was the only place where a valid user identifier shared a URL with a static sibling and hid an operation.

  • /queries/{name} has the child if-graph-commit. The child is not a static sibling. POST /queries/if-graph-commit binds {name}.
  • /commits/{commit_id} has the children publish and changes. There is no static sibling. A commit id has the form hb1.<block>.<slot>.<nonce>.
  • /cluster/deployments/{id} has no static sibling. /cluster/plan is a different path.
  • GET /graphs/discovery is one static path. No graph operation uses the exact path /graphs/{graph_id}. /graphs/discovery/snapshot binds the id.
  • /graphs/og_catalog is a static catalogue tree. Admission refuses a graph id with the prefix og_ (reserved_graph_id in cluster deployment). Reservation is the right close for that system scope.
  • This server has no /actors or /tokens collection.
  • No route exists under /branches/{branch}/.... An unencoded / is two segments and answers 404. merge%2Fchild is one segment. Axum decodes it to merge/child.

Callers, policy, and the spec

DELETE reaches branch_delete_body: the branch_delete check, admission, and branch_delete_as. POST of merge reaches server_branch_merge, including its admission and policy checks. GET reaches server_branch_diff or server_merge_base, and those call authorize_branch_reads. The 405 helper does not touch storage. Bearer auth and the contract check still run in the route layers, before the handler.

The CLI remote delete is Method::DELETE and remote_url(base, ["branches", name]) in client.rs. remote_url encodes / and leaves unreserved characters unchanged. #1090 keeps that delete URL and still posts the merge to /branches/merge. MCP branches_delete calls server_branch_delete in process. GQ branch delete calls branch_delete_body through POST /mutate (handlers/dispatch.rs). Those two paths already deleted these names.

OpenAPI still describes the operations clients call: POST .../branches/merge, GET .../branches/diff, GET .../branches/merge-base, and DELETE .../branches/{branch}. The handler attributes did not change, and openapi::openapi_spec_is_up_to_date passed. A gateway that selects the concrete path /branches/merge still sees POST only. The server accepts DELETE on that URL. The path move in #1089 closes that gap. Do not add a second DELETE to those static path items in this pull request.

One spelling changed with the single route. The handler sees the decoded segment. POST /branches/%6D%65%72%67%65 now runs the merge. Before, that request hit {branch} and answered 405 with Allow: DELETE. GET of the encoded spelling of diff now runs the diff. DELETE of the encoded spelling already deleted the branch on the old router. The CLI does not send that spelling. The two URI spellings now name one action, which matches the decoded path.

Sibling pull requests

#1089 says this 405 is a separate bug, and it forbids a static segment beside a parameter. This router no longer has that pair under /branches. The path move stays in the later contract batch.

#1076 adds method_not_allowed_fallback, which returns an ApiError body, and it edits handlers.rs. Expect a merge conflict. After both land, a method Axum rejects can take the JSON 405, while a GET or POST that this handler rejects stays an empty body unless this route uses that fallback. #1074 adds layers in build_app and does not change this precedence. #1075 and #1077 edit the end of build_app. #1012 does not edit these routes. #1016 adds from_commit on BranchCreateRequest. The struct literal in the delete test must name that field when both land.

Validation

Reviewed head 5c2dc29c1d6576bab9fcc2ab9460575827f36733. Base 9e2425d26a03b1b5628030a359ee51f096d3ca1c. Isolated worktree. CARGO_TARGET_DIR=/workspace/target.

GQT cannot express this contract. A .gqt case runs the engine. branch delete in GQ enters POST /mutate and branch_delete_body. It does not build the Axum router, so it cannot show this 405. The owner is omnigraph-server data_routes. The pull request extends remote_branch_delete_flow_works.

Before, that test, with lib.rs and handlers.rs restored from 9e2425d2 and the test file from 5c2dc29c:

cargo test -p omnigraph-server --test integration --locked data_routes::remote_branch_delete_flow_works -- --exact --test-threads=1 --nocapture

Result: failed. Panic at crates/omnigraph-server/tests/support/mod.rs:513, EOF while parsing a value. The 405 body is empty, so json_response cannot parse it.

A temporary probe on that same router, then removed:

  • DELETE /branches/feature answered 200.
  • DELETE /branches/merge answered 405, Allow: POST, body length 0. Create had answered 200.
  • DELETE /branches/diff answered 405, Allow: GET,HEAD, body length 0.
  • DELETE /branches/merge-base answered 405, Allow: GET,HEAD, body length 0.
  • DELETE /branches/merge%2Fchild, after create of merge/child, answered 200.
  • DELETE /branches/merge/child answered 404.
  • PUT /branches/feature answered 405, Allow: DELETE.
  • POST /branches/%6D%65%72%67%65 answered 405, Allow: DELETE.
  • GET /branches/%64%69%66%66 answered 405, Allow: DELETE.
  • DELETE /branches/%6D%65%72%67%65 answered 200 and deleted the branch merge that the literal DELETE had left in place.

After, exact 5c2dc29c, the same delete command passed.

These also passed on that head:

  • data_routes::remote_branch_list_create_merge_flow_works
  • data_routes::diff_routes_review_a_branch_against_its_merge_base
  • openapi::openapi_spec_is_up_to_date
  • auth_policy::signed_data_requires_cedar_and_rejects_forgery_on_every_protected_route
  • auth_policy::signed_merge_requires_separate_policy_permission_to_delete_source
  • cargo clippy -p omnigraph-server --locked -- -D warnings -W clippy::dbg_macro (exit 0, without --all-targets)

A temporary matrix on the head, then removed, also passed. It deleted feature, merge, diff, and merge-base. It deleted merge/child through merge%2Fchild. An unencoded slash and /branches/feature/extra answered 404. POST of %6D%65%72%67%65 merged. GET and HEAD of encoded diff, and GET and HEAD of merge-base with source=feature, answered 200, and the HEAD body was empty. HEAD /branches/feature answered 405, Allow: DELETE. HEAD /branches/merge answered 405, Allow: DELETE, POST. POST /branches/merge-base answered 405, Allow: DELETE, GET, HEAD. PUT, PATCH, and OPTIONS answered 405, Allow: POST,GET,HEAD,DELETE.

Limitations: I did not run the full server suite, a live CLI process, or DST. Temporary tests were removed. Nothing was pushed.

Brings in branch creation at a commit; the delete flow test's create
requests name the new `from_commit` field.

The branch route also takes the review's optional notes. Each action's
name, method and handler now live in one `BranchAction` enum. The route
keeps DELETE and sends every other method to one fallback, which runs
the action the name and method select (HEAD runs a GET action) or
answers 405 with that name's `Allow`, so PUT and PATCH name the right
methods too. The release note and the test comment describe this route
instead of the closures it replaced.
@ragnorc

ragnorc commented Oct 20, 2026

Copy link
Copy Markdown
Contributor Author

Both optional notes addressed in c7f780c, which also merges main; the delete-flow test's create requests now name from_commit.

  1. Fallback and one table. BranchAction is an enum holding each action's name, method and handler; its exhaustive run match is the only dispatch. /branches/{branch} keeps DELETE and sends every other method to one fallback. The fallback runs the action the name and method select (HEAD runs a GET action) or answers 405 with that name's Allow. The 405 cases now cover PUT /branches/feature (DELETE) and PATCH /branches/merge (DELETE, POST).
  2. Release note and test comment. Both describe the one route now, not the closures.

data_routes, openapi, auth_policy, clippy and check-docs pass.

@ragnorc
ragnorc enabled auto-merge October 20, 2026 04:12
@ragnorc
ragnorc added this pull request to the merge queue Oct 20, 2026
Merged via the queue into ModernRelay:main with commit 85a4613 Oct 20, 2026
32 checks passed
@cursor

cursor Bot commented Oct 20, 2026

Copy link
Copy Markdown

Recommendation: fix the Allow header on the action responses in a follow-up. The delete fix can stay. This pull request merged during the review, at head c7f780c28fd5f16f0560fb389a4e9f38d07aeed4 (squash 85a46138). The branch route on that squash is the same code. One blocking defect remains. The two earlier optional notes are resolved.

Why

A branch may be named merge, diff, or merge-base. Lance 11 allows those names, and OmniGraph allows them. DELETE /graphs/{graph_id}/branches/<name> must delete that branch. The previous static routes answered 405 instead.

What c7f780c2 changes

c7f780c2 is a merge of main b8e82abf into 5c2dc29c. The merge brings branch creation at a commit (from_commit on POST /branches). The author's own edits are the route and the note:

  • BranchAction holds each action's name, method, and handler. run is one exhaustive match and is the only dispatch for the three actions.
  • /branches/{branch} keeps DELETE and sends every other method to server_branch_action.
  • The fallback runs the action the name and method select. HEAD runs a GET action. Any other pair answers 405 with that name's Allow.
  • The release note and the comment on remote_branch_delete_flow_works describe this one route.
  • The delete-flow creates name from_commit: None, so they compile with the create request from #1016.

DELETE still calls branch_delete_body. Create stays on POST /branches. The two routes do not share a path.

Earlier optional notes

  1. Resolved. PUT /branches/feature answers 405 with Allow: DELETE. PATCH /branches/merge answers 405 with Allow: DELETE, POST. OPTIONS, TRACE, and CONNECT do the same for each name class: an ordinary name allows DELETE; merge allows DELETE, POST; diff and merge-base allow DELETE, GET, HEAD. Axum 0.8.9 keeps an Allow header the fallback sets (RouteFuture::set_allow_header writes the route value only when the response has none).

  2. Resolved. changelog.d/branch-delete-beside-static-routes.fixed.md and the test comment describe the one route. python3 scripts/check-docs.py reported Documentation OK (345 Markdown files checked).

Blocking

server_branch_action in crates/omnigraph-server/src/handlers.rs (around lines 3108-3116 at this head).

The method router registers only DELETE, so its route Allow value is DELETE. Every fallback response that does not set Allow receives that value. A 405 sets its own Allow, and that value wins. A response from action.run does not set Allow, so Axum writes DELETE on it.

Measured on this head, then removed:

  • GET /branches/diff?source=main&target=main answers 200, body length 249, Allow: DELETE.
  • HEAD of that URL answers 200, empty body, Allow: DELETE.
  • HEAD /branches/merge-base?source=main&target=main answers 200, empty body, Allow: DELETE.
  • GET /branches/diff with no source answers 400, Allow: DELETE.
  • POST /branches/merge of main into main with dry_run: true answers 400, Allow: DELETE, body branch_merge requires distinct source and target branches.

The same three calls at 5c2dc29c returned the same status, the same body, and no Allow header. HEAD was already an empty body with no Content-Length. Those facts did not change. The new header did.

Allow: DELETE on a successful diff says the resource serves only DELETE. The request that just succeeded was GET. The same false list is on the merge 400, whose method was POST and whose body is a request error. HEAD of diff and merge-base is a served method, and it carries the same false list.

Set the name's Allow on the response from action.run before returning it, with the same text branch_method_not_allowed builds (DELETE, or DELETE, POST, or DELETE, GET, HEAD). Axum then leaves that header in place. A 200, a 400, and a 405 for one name then name the same methods. Add those two assertions to remote_branch_delete_flow_works: a successful GET of diff and the distinct-branch 400 from POST of merge must not carry Allow: DELETE.

Checked claims

HEAD of diff and merge-base returns an empty body. Axum strips it in the top-level RouteFuture because the request method is HEAD. The handler still runs the read, as it did when GET was registered.

OPTIONS is not a special 200. It reaches the fallback and answers 405 with the name's Allow. TRACE and CONNECT do the same. Bodies are empty.

Bearer auth and the contract check are route layers outside the handler. auth_policy passed, including merge, diff, and delete decisions (policy_blocks_non_admin_merge_to_main_and_allows_admin, policy_branch_review_needs_read_on_the_target, signed_merge_requires_separate_policy_permission_to_delete_source). The fallback calls server_branch_merge, server_branch_diff, and server_merge_base, so those checks still run.

OpenAPI is unchanged against main and still describes POST .../branches/merge, GET .../branches/diff, GET .../branches/merge-base, and DELETE .../branches/{branch}. openapi passed (96 tests). A gateway that reads the static path item still sees POST only on /branches/merge. The server also accepts DELETE there. Draft #1089 moves these actions off the collection. Leave the static path items as they are in this change.

#1016 adds from_commit on POST /branches. That path is not /branches/{branch}. Every BranchCreateRequest literal in the server tests names the field. The delete test uses from_commit: None with from: Some("main"), which starts the branch at the source head.

Tradeoff and liability

The contract is the HTTP interface. DELETE /branches/{name} deletes a branch whose name Lance and OmniGraph accept. The router owns that contract. One route and one BranchAction enum are the right layer. check_valid_branch in Lance 11.0.0 allows a non-empty name of alphanumeric segments plus ., -, and _. It refuses main, a leading or trailing /, //, .., \, and a name that ends in .lock. OmniGraph also refuses an empty name, surrounding whitespace, a control character, and an incarnation-shaped suffix. merge, diff, and merge-base pass both checks. A reservation at create would change the branch-name contract on the engine, the CLI, GQ, and MCP in order to hide a route clash.

The enum removes the copied closures. The next action is one variant. Five more actions stay in that enum. The name match remains until #1089 moves the paths. The extra work is one string compare.

The new liability is the route-level Allow value. It is DELETE because that is the only method registered on the router. Axum attaches it to every fallback response that omits the header. That makes a successful read advertise the wrong methods. Setting the name's list on the action response keeps one source of truth and removes the false header.

Other open work

#1089 says this 405 was a separate bug and forbids a static segment beside a parameter. This router no longer has that pair under /branches. The path move stays in the later contract batch.

#1076 adds Router::method_not_allowed_fallback. Axum's default_fallback replaces only a default fallback, so this custom fallback stays. After both are on main, a method this route rejects remains an empty 405 with the name's Allow, while other routes gain the JSON 405 body. #1074, #1075, #1077, #1078, #1081, #1083, #1084, and #1096 edit lib.rs or handlers.rs and can conflict in text. #1096 changes an error field inside server_branch_merge; the fallback still calls that function. #1090 still posts the merge to /branches/merge and deletes /branches/{name}. #1017 and #1093 do not change this dispatch.

Validation

Reviewed head c7f780c28fd5f16f0560fb389a4e9f38d07aeed4. Isolated worktrees. Temporary probes were removed. Nothing was pushed.

GQT cannot show this contract. A .gqt case runs the engine. branch delete enters POST /mutate and branch_delete_body. It does not build the Axum router. The owner is omnigraph-server data_routes.

Before, at 5c2dc29c, with the head's two new rows added to remote_branch_delete_flow_works and then removed:

CARGO_TARGET_DIR=/tmp/target-1091-before cargo test -p omnigraph-server --test integration --locked data_routes::remote_branch_delete_flow_works -- --exact --test-threads=1 --nocapture

Failed. PUT feature returned Allow: POST,GET,HEAD,DELETE. The assertion required DELETE. The test stopped on that row, so PATCH was not reached. That header is the route list from .post().get().delete().

After, at c7f780c2, the same test in the committed form passed, including PUT /branches/feature (DELETE) and PATCH /branches/merge (DELETE, POST).

Also passed on that head:

  • data_routes:: — 93 passed, 0 failed
  • openapi:: — 96 passed, 0 failed
  • auth_policy:: — 41 passed, 0 failed
  • cargo clippy -p omnigraph-server --all-targets --locked -- -D warnings -W clippy::dbg_macro — exit 0
  • python3 scripts/check-docs.py — Documentation OK (345 Markdown files)

Limitations: I did not run the full workspace suite, a live CLI process, or DST. The pull request text and the squash body still start from the removed closure design. The changelog in the tree describes the one route.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant