fix(cli): unify remote URL builder, fix branch delete //branches 404#230
Conversation
![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: 15cf4bf7ce
ℹ️ 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".
| let graph = SystemGraph::loaded(); | ||
| graph.write_file( | ||
| "graph.policy.yaml", | ||
| "version: 1\ngroups:\n ops: [\"act-op\"]\nprotected_branches: [main]\nrules:\n - id: allow-branch-ops\n allow:\n actors: { group: ops }\n actions: [branch_create, branch_delete]\n branch_scope: any\n - id: allow-read\n allow:\n actors: { group: ops }\n actions: [read]\n branch_scope: any\n", |
There was a problem hiding this comment.
This new policy fixture grants branch_create/branch_delete with branch_scope, but policy validation rejects branch_scope for those actions: uses_branch_scope only allows read/export/change, while branch create/delete are target-branch-scoped (crates/omnigraph-policy/src/lib.rs:92-100, enforced at :344-352). When this test starts the server with this per-graph policy, the policy load fails before the regression path can run; change this rule to target_branch_scope: any.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid catch (branch_create/branch_delete are target-branch-scoped, so branch_scope fails validation). Resolved by removing the spawned-server e2e fixture entirely — that file is now unchanged vs main. The CLI URL composition (the actual defect) is covered by remote_url unit tests instead; the server route is already covered by the boot_settings DELETE /graphs/{id}/branches/{branch} oneshot.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } | ||
|
|
||
| /// Regression: `branch delete` over a multi-graph `--server`/`--graph` target | ||
| /// must compose `…/graphs/{id}/branches/{name}` with no empty `//` segment. | ||
| /// The pre-fix CLI emitted `…/graphs/local//branches/<name>` and 404ed, while | ||
| /// list/create (which carry the branch in the JSON body, not the path) worked. | ||
| /// This exercises the real CLI→HTTP→multi-graph route, the boundary the | ||
| /// `remote_url` unit tests cannot reach. Branch names with `/` are sent as one | ||
| /// percent-encoded segment, so this also covers the slash-in-name case. | ||
| #[test] | ||
| fn local_cli_branch_delete_over_server_graph_target_composes_clean_url() { | ||
| let graph = SystemGraph::loaded(); | ||
| graph.write_file( | ||
| "graph.policy.yaml", | ||
| "version: 1\ngroups:\n ops: [\"act-op\"]\nprotected_branches: [main]\nrules:\n - id: allow-branch-ops\n allow:\n actors: { group: ops }\n actions: [branch_create, branch_delete]\n branch_scope: any\n - id: allow-read\n allow:\n actors: { group: ops }\n actions: [read]\n branch_scope: any\n", | ||
| ); | ||
| let config = graph.write_config( | ||
| "omnigraph-server.yaml", | ||
| &format!( | ||
| "graphs:\n local:\n uri: {}\n policy:\n file: ./graph.policy.yaml\n", | ||
| yaml_string(&graph.path().to_string_lossy()) | ||
| ), | ||
| ); | ||
| let server = spawn_server_with_config_env( | ||
| &config, | ||
| &[( | ||
| "OMNIGRAPH_SERVER_BEARER_TOKENS_JSON", | ||
| r#"{"act-op":"srv-tok"}"#, | ||
| )], | ||
| ); | ||
|
|
||
| let operator_home = tempfile::tempdir().unwrap(); | ||
| fs::write( | ||
| operator_home.path().join("config.yaml"), | ||
| format!("servers:\n dev:\n url: {}\n", server.base_url), | ||
| ) | ||
| .unwrap(); | ||
| fs::write( | ||
| operator_home.path().join("credentials"), | ||
| "[dev]\ntoken = srv-tok\n", | ||
| ) | ||
| .unwrap(); | ||
| #[cfg(unix)] | ||
| { | ||
| use std::os::unix::fs::PermissionsExt; | ||
| fs::set_permissions( | ||
| operator_home.path().join("credentials"), | ||
| fs::Permissions::from_mode(0o600), | ||
| ) | ||
| .unwrap(); | ||
| } | ||
|
|
||
| // A branch name with a slash, sent as one `%2F`-encoded path segment. | ||
| let branch = "etl/zendesk/run-1"; | ||
|
|
||
| let create = cli() | ||
| .env("OMNIGRAPH_HOME", operator_home.path()) | ||
| .arg("branch") | ||
| .arg("create") | ||
| .arg("--server") | ||
| .arg("dev") | ||
| .arg("--graph") | ||
| .arg("local") | ||
| .arg("--from") | ||
| .arg("main") | ||
| .arg(branch) | ||
| .output() | ||
| .unwrap(); | ||
| assert!( | ||
| create.status.success(), | ||
| "branch create over --server/--graph: {create:?}" | ||
| ); | ||
|
|
||
| // The regression assertion: pre-fix this 404ed on `…/graphs/local//branches/…`. | ||
| let delete = cli() | ||
| .env("OMNIGRAPH_HOME", operator_home.path()) | ||
| .arg("branch") | ||
| .arg("delete") | ||
| .arg("--server") | ||
| .arg("dev") | ||
| .arg("--graph") | ||
| .arg("local") | ||
| .arg(branch) | ||
| .output() | ||
| .unwrap(); | ||
| assert!( | ||
| delete.status.success(), | ||
| "branch delete over --server/--graph must succeed (no `//branches` 404): {delete:?}" | ||
| ); | ||
| } |
There was a problem hiding this comment.
AGENTS.md rule 12 requires that a bug-fix regression test land in a commit immediately before the fix commit, so the red→green pair is visible in git log and a reviewer can check out only the test commit to independently reproduce the failure. The git history for this PR has a single commit (15cf4bf) that bundles both the unit tests in helpers.rs and the e2e test here together with the remote_url rewrite. That means there's no standalone "failing test" commit to check out — the guarantee the rule was designed to preserve is absent. Please split into (1) a test commit that compiles but fails, then (2) the fix commit.
Context Used: AGENTS.md (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Addressed. The PR is now two commits: a5d3304 adds a regression test that fails red against the old remote_branch_url (left: …/graphs/p9-os//branches/tmpbranch), and e888089 is the correct-by-design fix (unified remote_url builder) that turns it green. The red commit is check-out-able and reproduces the 404 independently, per AGENTS.md rule 12.
ragnorc
force-pushed
the
ragnorc/delete-branch-endpoint
branch
from
68ac6f8 to
cd1862b
Compare
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"
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.
ragnorc
force-pushed
the
ragnorc/delete-branch-endpoint
branch
from
cd1862b to
e888089
Compare
1 participant
What & why
omnigraph branch deleteover an HTTP--server/--graphtarget composed its URL with a manually appended trailing slash, so the path-segment join emitted an empty//segment (/graphs/{id}//branches/{name}) that missed the route and returned 404; the server endpoint itself was fine. This replaces the two divergent URL helpers with one structuredremote_url(base, segments, query)builder that every remote call routes through, so trailing-slash normalization and per-segment / per-value percent-encoding live in one place and the bug class (stray//, un-encoded dynamic components) is closed by construction.Backing issue / RFC
branch deleteover --server/--graph builds double-slash URL → 404 #232Checklist
Notes for reviewers
The fix also closes two previously latent siblings of the same class: a commit id interpolated raw into the path (
get_commit) and a branch name interpolated raw into the query string (list_commits/snapshot) are now percent-encoded.Coverage is boundary-matched: the defect is CLI URL composition (a pure function), so it is covered by 6
remote_urlunit tests — multi-graph/single-graph/trailing-slash bases asserting no//, slash-in-name path encoding, and the latent commit-id and query-value cases. The server route is already covered by theboot_settingsDELETE /graphs/{id}/branches/{branch}oneshot, and branch-delete logic by the embeddedlocal_cli_branch_delete_enforces_engine_layer_policytest, so no new spawned-server e2e was added.Note
Low Risk
CLI-only HTTP URL composition with no server or storage changes; behavior is narrowed to correct routing and encoding, backed by targeted unit tests.
Overview
Fixes remote
branch delete(and similar) 404s when the CLI targets a multi-graph server base like…/graphs/{id}: URL assembly no longer produces an empty//path segment beforebranches/{name}.The CLI now has one
remote_url(base, segments, query)builder (viareqwest::Urlwithpop_if_empty+ segment extend) instead of separate string-concat and branch-specific helpers. All remote HTTP call sites inGraphClient(and operator alias query POSTs) go through it, with?propagation on URL build failure.Dynamic pieces are encoded by construction: branch/commit/query names as path segments (e.g. slashes in branch names), and branch names in query strings for snapshot/commits, instead of raw
format!interpolation.Adds six unit tests on
remote_urlfor no double-slash joins, trailing-slash bases, and path/query encoding.Reviewed by Cursor Bugbot for commit e888089. Bugbot is set up for automated code reviews on this repo. Configure here.
Greptile Summary
This PR fixes a 404 on
branch deleteover an HTTP server target by replacing two divergent URL-building helpers with a singleremote_url(base, segments, query)builder that routes every remote call through one code path. The double-slash bug was caused byremote_branch_urlappending a literal/before callingUrl::path_segments_mut().extend(…), which left a trailing empty segment that produced…/graphs/{id}//branches/{name}.pop_if_empty()on the path segments beforeextend(segments), eliminating the stray empty segment by construction rather than by per-callsite string hygiene.get_commit) and branch names interpolated raw into query strings (list_commits,snapshot) are now percent-encoded automatically via the structured segment and query-pair API.remote_urlverify multi-graph/single-graph bases, trailing-slash tolerance, and slash/special-char encoding for both path segments and query values.Confidence Score: 5/5
Safe to merge — the change is a pure URL construction refactor with no behavioral change except the fix itself; both helpers were pub(crate) so there is no external API surface to break.
The bug fix is mechanically correct: pop_if_empty() removes the trailing empty path segment before extend appends the new ones, which is precisely what was causing the double-slash 404. All 16 call-sites in client.rs migrate cleanly to the new signature, and the six unit tests directly pin the fixed invariant plus the latent encoding cases. No cross-crate API surface is affected.
No files require special attention.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["remote_url(base, segments, query)"] --> B["trim_end_matches('/') on base"] B --> C["Url::parse(trimmed_base)"] C -->|"cannot-be-a-base URL"| ERR["Err: invalid remote base url"] C -->|"valid URL"| D["path_segments_mut()"] D --> E["pop_if_empty()\n(removes trailing empty segment,\ne.g. from http://host/ or …/graphs/id/)"] E --> F["extend(segments)\n(each segment is percent-encoded,\ne.g. etl/zendesk → etl%2Fzendesk)"] F --> G{"query.is_empty()?"} G -->|"No"| H["query_pairs_mut()\n.append_pair(key, value)\n(both key & value percent-encoded)"] G -->|"Yes"| I["Ok(url.to_string())"] H --> IReviews (4): Last reviewed commit: "fix(cli): unify remote URL builder, clos..." | Re-trigger Greptile