From a5d3304425b021518b491959d234d31370d36aa7 Mon Sep 17 00:00:00 2001 From: Ragnor Comerford Date: Sun, 14 Jun 2026 20:30:16 +0200 Subject: [PATCH 1/2] test(cli): reproduce branch-delete //branches 404 (failing) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Regression test for the `branch delete` 404 over a multi-graph `--server`/`--graph` target: the composed URL must be `/branches/` 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" --- crates/omnigraph-cli/src/helpers.rs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/crates/omnigraph-cli/src/helpers.rs b/crates/omnigraph-cli/src/helpers.rs index 7e1ca15e..39bc1f8f 100644 --- a/crates/omnigraph-cli/src/helpers.rs +++ b/crates/omnigraph-cli/src/helpers.rs @@ -1059,3 +1059,27 @@ pub(crate) fn rewrite_deprecated_argv(args: Vec) -> Vec { } args } + +#[cfg(test)] +mod tests { + use super::*; + + // Regression for the `branch delete` 404: over a multi-graph + // `--server`/`--graph` target the composed URL must be exactly + // `/branches/` with no empty `//` segment. An empty segment + // misses the `/graphs/{graph_id}/branches/{branch}` route and 404s. + // + // This FAILS against the current `remote_branch_url`, which parses the base + // with a manually appended trailing slash and so emits + // `…/graphs/p9-os//branches/tmpbranch`. The fix (a unified, structured URL + // builder) turns it green. + #[test] + fn remote_branch_url_multi_graph_base_has_no_double_slash() { + let url = remote_branch_url("http://host/graphs/p9-os", "tmpbranch").unwrap(); + assert_eq!(url, "http://host/graphs/p9-os/branches/tmpbranch"); + assert!( + !url.contains("//branches"), + "double slash before branches: {url}" + ); + } +} From e88808918a46b709e0f982851f6ba50d0f11da63 Mon Sep 17 00:00:00 2001 From: Ragnor Comerford Date: Sun, 14 Jun 2026 20:30:55 +0200 Subject: [PATCH 2/2] 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 | 34 ++++----- crates/omnigraph-cli/src/helpers.rs | 108 +++++++++++++++++++++++----- 2 files changed, 106 insertions(+), 36 deletions(-) diff --git a/crates/omnigraph-cli/src/client.rs b/crates/omnigraph-cli/src/client.rs index d9e77264..4faaa11a 100644 --- a/crates/omnigraph-cli/src/client.rs +++ b/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::( 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::( 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(), ) diff --git a/crates/omnigraph-cli/src/helpers.rs b/crates/omnigraph-cli/src/helpers.rs index 39bc1f8f..6a381b13 100644 --- a/crates/omnigraph-cli/src/helpers.rs +++ b/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 { - 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 { + 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(), ) @@ -1064,22 +1090,66 @@ pub(crate) fn rewrite_deprecated_argv(args: Vec) -> Vec { mod tests { use super::*; - // Regression for the `branch delete` 404: over a multi-graph - // `--server`/`--graph` target the composed URL must be exactly - // `/branches/` with no empty `//` segment. An empty segment - // misses the `/graphs/{graph_id}/branches/{branch}` route and 404s. - // - // This FAILS against the current `remote_branch_url`, which parses the base - // with a manually appended trailing slash and so emits - // `…/graphs/p9-os//branches/tmpbranch`. The fix (a unified, structured URL - // builder) turns it green. + // `branch delete` interpolates the branch into the URL path. The composed + // path must be exactly `/branches/` with no empty `//` + // segment — an empty segment misses the + // `/graphs/{graph_id}/branches/{branch}` route and 404s. #[test] - fn remote_branch_url_multi_graph_base_has_no_double_slash() { - let url = remote_branch_url("http://host/graphs/p9-os", "tmpbranch").unwrap(); + 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" + ); + } }