fix(cli): unify remote URL builder, fix branch delete //branches 404 (#230)

* test(cli): reproduce branch-delete //branches 404 (failing)

Regression test for the `branch delete` 404 over a multi-graph
`--server`/`--graph` target: the composed URL must be `<base>/branches/<name>`
with no empty `//` segment. Fails against the current `remote_branch_url`,
which appends a trailing slash before extending path segments and so emits
`…/graphs/p9-os//branches/tmpbranch`. The next commit fixes it.

  left:  "http://host/graphs/p9-os//branches/tmpbranch"
  right: "http://host/graphs/p9-os/branches/tmpbranch"

* fix(cli): unify remote URL builder, close the //branches 404 class

Correct-by-design fix for the failing test in the previous commit. The bug was
not specific to `branch delete`: URL assembly was scattered across a
string-concat `remote_url`, a url-crate `remote_branch_url`, and several
`format!` interpolations that left dynamic path/query components un-encoded
(commit id in the path, branch in the query string). `branch delete` was the
instance that surfaced because it is the only verb that puts a dynamic value in
the path.

Replace both helpers with one `remote_url(base, segments, query)` that every
remote call routes through. Callers pass structured segments and query pairs,
so trailing-slash normalization (pop_if_empty) and per-segment / per-value
percent-encoding live in one place. A stray `//` or an un-encoded dynamic
component is no longer representable, closing the whole class rather than the
reported instance.

Migrates the previous commit's failing test to the new builder and adds the
single-graph, trailing-slash, slash-in-name, commit-id-path, and query-value
cases (the last two cover the previously latent siblings). All 16 callsites
migrated; `remote_branch_url` removed.

crates/omnigraph-cli/src/client.rs

@@ -40,7 +40,7 @@ use serde_json::Value;
 use crate::cli::CliLoadMode;
 use crate::helpers::{
     ResolvedCliGraph, apply_bearer_token, apply_server_flag, build_http_client, is_remote_uri,
-    legacy_change_request_body, open_local_db_with_policy, query_params_from_json, remote_branch_url,
+    legacy_change_request_body, open_local_db_with_policy, query_params_from_json,
     remote_json, remote_url, resolve_cli_actor, resolve_cli_graph, resolve_remote_bearer_token,
     select_named_query,
 };
@@ -173,7 +173,7 @@ impl GraphClient {
                 remote_json(
                     http,
                     Method::GET,
-                    remote_url(base_url, "/branches"),
+                    remote_url(base_url, &["branches"], &[])?,
                     None,
                     token.as_deref(),
                 )
@@ -198,7 +198,7 @@ impl GraphClient {
                 remote_json(
                     http,
                     Method::GET,
-                    format!("{}?branch={}", remote_url(base_url, "/snapshot"), branch),
+                    remote_url(base_url, &["snapshot"], &[("branch", branch)])?,
                     None,
                     token.as_deref(),
                 )
@@ -222,7 +222,7 @@ impl GraphClient {
                 remote_json(
                     http,
                     Method::GET,
-                    remote_url(base_url, "/schema"),
+                    remote_url(base_url, &["schema"], &[])?,
                     None,
                     token.as_deref(),
                 )
@@ -245,8 +245,8 @@ impl GraphClient {
                 token,
             } => {
                 let url = match branch {
-                    Some(branch) => format!("{}?branch={}", remote_url(base_url, "/commits"), branch),
-                    None => remote_url(base_url, "/commits"),
+                    Some(branch) => remote_url(base_url, &["commits"], &[("branch", branch)])?,
+                    None => remote_url(base_url, &["commits"], &[])?,
                 };
                 remote_json(http, Method::GET, url, None, token.as_deref()).await
             }
@@ -273,7 +273,7 @@ impl GraphClient {
                 remote_json(
                     http,
                     Method::GET,
-                    remote_url(base_url, &format!("/commits/{commit_id}")),
+                    remote_url(base_url, &["commits", commit_id], &[])?,
                     None,
                     token.as_deref(),
                 )
@@ -310,7 +310,7 @@ impl GraphClient {
                 let output = remote_json::<IngestOutput>(
                     http,
                     Method::POST,
-                    remote_url(base_url, "/load"),
+                    remote_url(base_url, &["load"], &[])?,
                     Some(serde_json::to_value(IngestRequest {
                         branch: Some(branch.to_string()),
                         from: from.map(ToOwned::to_owned),
@@ -354,7 +354,7 @@ impl GraphClient {
                 remote_json(
                     http,
                     Method::POST,
-                    remote_url(base_url, "/ingest"),
+                    remote_url(base_url, &["ingest"], &[])?,
                     Some(serde_json::to_value(IngestRequest {
                         branch: Some(branch.to_string()),
                         from: Some(from.to_string()),
@@ -393,7 +393,7 @@ impl GraphClient {
                 remote_json(
                     http,
                     Method::POST,
-                    remote_url(base_url, "/change"),
+                    remote_url(base_url, &["change"], &[])?,
                     Some(legacy_change_request_body(
                         query_source,
                         query_name,
@@ -446,7 +446,7 @@ impl GraphClient {
                 remote_json(
                     http,
                     Method::POST,
-                    remote_url(base_url, "/read"),
+                    remote_url(base_url, &["read"], &[])?,
                     Some(serde_json::to_value(ReadRequest {
                         query_source: query_source.to_string(),
                         query_name: query_name.map(ToOwned::to_owned),
@@ -484,7 +484,7 @@ impl GraphClient {
                 remote_json(
                     http,
                     Method::POST,
-                    remote_url(base_url, "/branches"),
+                    remote_url(base_url, &["branches"], &[])?,
                     Some(serde_json::to_value(BranchCreateRequest {
                         from: Some(from.to_string()),
                         name: name.to_string(),
@@ -518,7 +518,7 @@ impl GraphClient {
                 remote_json(
                     http,
                     Method::DELETE,
-                    remote_branch_url(base_url, name)?,
+                    remote_url(base_url, &["branches", name], &[])?,
                     None,
                     token.as_deref(),
                 )
@@ -547,7 +547,7 @@ impl GraphClient {
                 remote_json(
                     http,
                     Method::POST,
-                    remote_url(base_url, "/branches/merge"),
+                    remote_url(base_url, &["branches", "merge"], &[])?,
                     Some(serde_json::to_value(BranchMergeRequest {
                         source: source.to_string(),
                         target: Some(into.to_string()),
@@ -598,7 +598,7 @@ impl GraphClient {
                 remote_json::<SchemaApplyOutput>(
                     http,
                     Method::POST,
-                    remote_url(base_url, "/schema/apply"),
+                    remote_url(base_url, &["schema", "apply"], &[])?,
                     Some(serde_json::to_value(SchemaApplyRequest {
                         schema_source: schema_source.to_string(),
                         allow_data_loss,
@@ -642,7 +642,7 @@ impl GraphClient {
                 token,
             } => {
                 let request = apply_bearer_token(
-                    http.request(Method::POST, remote_url(base_url, "/export")),
+                    http.request(Method::POST, remote_url(base_url, &["export"], &[])?),
                     token.as_deref(),
                 )
                 .json(&ExportRequest {
@@ -690,7 +690,7 @@ impl GraphClient {
                 remote_json(
                     http,
                     Method::GET,
-                    remote_url(base_url, "/graphs"),
+                    remote_url(base_url, &["graphs"], &[])?,
                     None,
                     token.as_deref(),
                 )

crates/omnigraph-cli/src/helpers.rs

@@ -16,15 +16,41 @@ pub(crate) fn is_remote_uri(uri: &str) -> bool {
     uri.starts_with("http://") || uri.starts_with("https://")
 }
 
-pub(crate) fn remote_url(base: &str, path: &str) -> String {
-    format!("{}{}", base.trim_end_matches('/'), path)
-}
-
-pub(crate) fn remote_branch_url(base: &str, branch: &str) -> Result<String> {
-    let mut url = reqwest::Url::parse(&format!("{}/", base.trim_end_matches('/')))?;
+/// THE one way the CLI composes a remote request URL. Every remote call
+/// routes through here so URL assembly has a single mechanism instead of
+/// per-callsite string interpolation.
+///
+/// - `base` is the resolved server root (single-graph) or `…/graphs/{id}`
+///   (multi-graph).
+/// - `segments` are appended as individual percent-encoded path segments, so
+///   a dynamic component (branch name, commit id, query name) is always one
+///   safe segment — e.g. a branch `etl/zendesk/run-1` becomes `%2F`-escaped.
+/// - `query` pairs are percent-encoded values.
+///
+/// Trailing-slash normalization happens exactly once via `pop_if_empty`:
+/// `Url::parse` normalizes a path-less base (`http://host`) to a single empty
+/// trailing segment, and a `…/graphs/{id}/` base keeps its own. `extend`
+/// appends *after* the last segment, so without dropping a trailing empty one
+/// the join emits `…/graphs/{id}//branches/{name}` — the empty `//` segment
+/// misses the route and 404s. Because callers pass structured segments rather
+/// than a pre-joined string, neither a stray `//` nor an un-encoded dynamic
+/// component is representable here.
+pub(crate) fn remote_url(
+    base: &str,
+    segments: &[&str],
+    query: &[(&str, &str)],
+) -> Result<String> {
+    let mut url = reqwest::Url::parse(base.trim_end_matches('/'))?;
     url.path_segments_mut()
         .map_err(|_| color_eyre::eyre::eyre!("invalid remote base url"))?
-        .extend(["branches", branch]);
+        .pop_if_empty()
+        .extend(segments);
+    if !query.is_empty() {
+        let mut pairs = url.query_pairs_mut();
+        for (key, value) in query {
+            pairs.append_pair(key, value);
+        }
+    }
     Ok(url.to_string())
 }
 
@@ -340,7 +366,7 @@ pub(crate) async fn execute_operator_alias(
     remote_json(
         client,
         Method::POST,
-        remote_url(&uri, &format!("/queries/{}", alias.query)),
+        remote_url(&uri, &["queries", &alias.query], &[])?,
         body,
         bearer_token.as_deref(),
     )
@@ -1059,3 +1085,71 @@ pub(crate) fn rewrite_deprecated_argv(args: Vec<OsString>) -> Vec<OsString> {
     }
     args
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    // `branch delete` interpolates the branch into the URL path. The composed
+    // path must be exactly `<base-path>/branches/<name>` with no empty `//`
+    // segment — an empty segment misses the
+    // `/graphs/{graph_id}/branches/{branch}` route and 404s.
+    #[test]
+    fn remote_url_multi_graph_base_has_no_double_slash() {
+        let url = remote_url("http://host/graphs/p9-os", &["branches", "tmpbranch"], &[]).unwrap();
+        assert_eq!(url, "http://host/graphs/p9-os/branches/tmpbranch");
+        assert!(
+            !url.contains("//branches"),
+            "double slash before branches: {url}"
+        );
+    }
+
+    #[test]
+    fn remote_url_single_graph_base_has_no_double_slash() {
+        let url = remote_url("http://host", &["branches", "tmpbranch"], &[]).unwrap();
+        assert_eq!(url, "http://host/branches/tmpbranch");
+    }
+
+    #[test]
+    fn remote_url_tolerates_trailing_slash_on_base() {
+        let url = remote_url("http://host/graphs/p9-os/", &["branches", "tmpbranch"], &[]).unwrap();
+        assert_eq!(url, "http://host/graphs/p9-os/branches/tmpbranch");
+    }
+
+    #[test]
+    fn remote_url_encodes_slashes_in_path_segment() {
+        let url = remote_url(
+            "http://host/graphs/p9-os",
+            &["branches", "etl/zendesk/run-1"],
+            &[],
+        )
+        .unwrap();
+        assert_eq!(
+            url,
+            "http://host/graphs/p9-os/branches/etl%2Fzendesk%2Frun-1"
+        );
+    }
+
+    // Sibling cases the unified builder closes by construction: a dynamic
+    // commit id in the path, and a branch name carried as a query value, are
+    // both percent-encoded instead of interpolated raw.
+    #[test]
+    fn remote_url_encodes_dynamic_path_segment_for_commits() {
+        let url = remote_url("http://host/graphs/p9-os", &["commits", "a/b c"], &[]).unwrap();
+        assert_eq!(url, "http://host/graphs/p9-os/commits/a%2Fb%20c");
+    }
+
+    #[test]
+    fn remote_url_encodes_query_values() {
+        let url = remote_url(
+            "http://host/graphs/p9-os",
+            &["snapshot"],
+            &[("branch", "feature&x=1")],
+        )
+        .unwrap();
+        assert_eq!(
+            url,
+            "http://host/graphs/p9-os/snapshot?branch=feature%26x%3D1"
+        );
+    }
+}