Skip to content

feat(cli): reach a server through a Unix socket and refuse non-server schemes - #988

Merged
ragnorc merged 3 commits into
ModernRelay:mainfrom
ragnorc:claude/omnigraph-issue-979-2858f0
Oct 14, 2026
Merged

ragnorc merged 3 commits into
ModernRelay:mainfrom
ragnorc:claude/omnigraph-issue-979-2858f0

Conversation

@ragnorc

@ragnorc ragnorc commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

What & why

Sandboxed agents often cannot open a TCP connection to a loopback server, but a sandbox can allow one Unix-socket path. --server unix:///absolute/path.sock (or a server url: of that form in ~/.omnigraph/config.yaml) now carries the CLI's HTTP/1.1 requests over that socket, for example to a local gateway in front of omnigraph-server. A --server value or configured url: with any scheme other than http, https or unix now fails as an invalid server address instead of being resolved as graph storage ([storage_root_invalid] … unsupported storage URI scheme).

Backing issue / RFC

Checklist

  • Change is focused (one logical change)
  • Tests added/updated for behavior changes
  • Public docs updated if user-facing surface changed
  • Reviewed against docs/dev/invariants.md: CLI transport only; no engine, storage or server change

Design

  • graph_http::ServerTransport::parse is the one parser of a server address. resolve_server_flag calls it for every literal --server, configured url:, profile and alias server, so a server scope can no longer resolve to an embedded client. GraphHttpClient builds its transport from the same parser, and helpers::is_remote_uri uses the same scheme list (now case-insensitive, as URL schemes are).
  • A unix:// root's whole path names the socket, so a request's HTTP target is whatever its composed URL carries after the root. GraphHttpClient::request, now fallible, is the one place that maps unix://…/graphs/g/… to http://localhost/graphs/g/…. It refuses a URL outside the root (another socket path, or a .. graph id) before sending anything. URL composition (remote_url), user-visible URIs, keyed-token matching and write-target echoes keep the unix:// spelling.
  • The transport is reqwest 0.12's ClientBuilder::unix_socket (already a dependency). Proxies and DNS do not apply, and requests carry Host: localhost, which the server does not read. Discovery, the contract header, the bearer token, no redirects and no retries are unchanged, because every client of a root is built by the same GraphHttpClient::build.
  • Socket paths are percent-decoded (new Unix-only direct dependency percent-encoding, already in the lockfile through url), so ~/Library/Application Support/… works written raw or as %20. A host, query, fragment, a path naming a directory (…/, …/..) and a NUL byte are refused. Non-Unix platforms refuse unix://.
  • GraphClient::resolve_registry drops its own scheme check, which resolve_server_flag now guarantees.

Local verification

  • cargo test --locked -p omnigraph-cli (with RUST_MIN_STACK=16777216): 145 unit tests passed (6 ignored), 296 integration tests passed (8 ignored)
  • cargo clippy -p omnigraph-cli --all-targets --locked -- -D warnings -W clippy::dbg_macro: clean
  • cargo fmt --all --check, typos: clean
  • python3 scripts/check-docs.py, bash scripts/check-agents-md.sh, python3 scripts/release_notes.py preview --working-tree: pass
  • python3 scripts/check-dependency-sources.py, cargo deny --locked check: pass
  • python3 scripts/check-fix-regression.py against this body: ok: issue #979 has a matching regression addition
  • Before/after: the 0.13.0 release CLI fails query --server unix:///tmp/x.sock --graph g and --server ftp://h with [storage_root_invalid]. This branch attempts the socket (server discovery connection failed (Unix socket); … this data request was not sent) and refuses ftp:// as an invalid server address.
  • Not run: the canonical workspace test graph with the failpoint features, and other packages' suites (no other crate changes).

Tests

  • helpers::tests::server_flag_resolves_unix_sockets_and_refuses_other_schemes_issue_979: the accepted and refused address table.
  • client::tests::graph_http_over_a_unix_socket_sends_root_relative_targets_issue_979: over a real socket, discovery is HEAD /healthz with no bearer. The data request's target is /graphs/knowledge/branches/etl%2Frun%201?branch=a+b, with Host: localhost, the bearer and the contract header. URLs outside the root are refused unsent, and a removed socket fails discovery as connection failed (Unix socket), not dispatched.
  • system_remote::remote_unix_socket_serves_query_mutate_load_export_and_blob_get_issue_979: the real CLI and server through tests/support/unix_gateway.rs, a byte relay whose socket sits under a directory with a space. Query and export output over the socket equal the same reads over TCP, the Blob bytes are exact, mutate and load land, and every command used the socket.
  • cli_data::server_scope_refuses_a_non_server_scheme_issue_979: query, mutate, load, export and blob get (with and without --graph), graphs list and cluster status refuse ftp://, s3://, file:// and a configured url: ftp://… as an invalid server address, never as storage. --store unix://… is refused like an http:// store.

Notes for reviewers

  • The server still listens on TCP only. The new "Unix-socket access" section in docs/user/deployment.md shows a socat UNIX-LISTEN:…,fork TCP:127.0.0.1:8080 relay. socat was not available locally, so that example command is untested.
  • The issue describes load as a "streamed body". The CLI sends the NDJSON as one raw request body (the server streams its intake), and the journey test covers that path as it exists.
  • Two pages were at the docs checker's 350-line cap, so the reference.md and server.md additions fit into existing lines.
  • Docs: docs/user/cli/index.md (new section), docs/user/cli/reference.md, docs/user/operations/server.md, docs/user/deployment.md, docs/dev/testing.md, and two changelog.d notes.

… schemes

`--server unix:///absolute/path.sock`, or a configured server `url:` of that
form, carries the CLI's HTTP/1.1 requests over a Unix-domain socket, so a
sandboxed client allowed one socket path can reach a local gateway in front of
omnigraph-server. Discovery, the contract header, the bearer token and the
no-redirect and no-retry rules are the same as over TCP.

`ServerTransport::parse` is the one parser of a server address. It runs where
every literal `--server`, configured url, profile and alias server resolves,
so a server scope with any scheme other than http, https or unix now fails as
an invalid server address instead of falling through to storage resolution.
A unix root's whole path names the socket: `GraphHttpClient::request` sends
what a composed URL carries after the root to http://localhost and refuses a
URL outside the root before sending anything.
@ragnorc
ragnorc requested a review from aaltshuler as a code owner October 13, 2026 22:40

@ragnorc ragnorc left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommendation: fix the platform-dependent address test before merge. I found one P2 test defect, described inline. I found no other blocking defect in the Unix runtime path.

This PR lets a sandboxed CLI reach OmniGraph through an allowed Unix-socket path. A local relay forwards HTTP to the existing TCP server. The CLI keeps the socket address for configuration and token lookup, then maps each request to a root-relative HTTP target. It also rejects storage and unknown schemes in --server, so an explicit server scope cannot become embedded storage access.

--server must never select embedded storage. The fix enforces that rule at server-address resolution. resolve_server_flag validates literal and configured roots. The shared classifier then covers reads, writes, registry commands, aliases, and cluster administration. GraphHttpClient retains discovery, contract checks, bearer handling, no redirects, and no retries. Blob delivery also retains the selected socket when it rebuilds its client.

The tradeoff is a new local relay process and socket-permission setup. Discovery still precedes each data request; this is an access feature, with no measured speed claim. The internal transport enum, normalized root, and decoded socket path separate the filesystem address from the HTTP target. They stay behind one request boundary. This removes the server-to-storage fallthrough and a duplicate registry scheme check. The new unix:// address contract needs platform and path-encoding coverage. It adds no durable state or storage format. Reusing reqwest and one shared test relay keeps the maintenance cost contained. Five more changes of this kind should extend this boundary and its tests, while still sharing credential and contract logic.

I checked reqwest 0.12.28's ClientBuilder::unix_socket and connector implementation: the fixed socket bypasses DNS and proxies. This agrees with the versioned upstream API. I also checked OmniGraph's storage classifier and the pinned Lance 11.0.0 object-store registry. Unix here is an HTTP transport, with storage still owned by the server. No private Lance API or future Lance feature is needed. The design fits the open profile work (#675), managed-client direction (#897), and request/outcome work (#981); these branches were inspected for direction, not tested as a combined merge.

The CLI Rust tests are the correct owner. GQT cannot choose CLI schemes, operator configuration, Unix sockets, or wire headers. The existing integration owners cover the user-visible contract without a second graph-test setup.

Local before/after proof on macOS:

  • Before: d8d2e3a8ddaa614acb6aa8fc1371f81c5bcf2897, with only the PR's two integration regressions and shared fixture carried back. All four test/fixture files matched the reviewed head byte for byte. 2 tests ran; 2 failed (exit 101). The scheme test got server discovery transport failed for ftp:// without a graph. The socket journey started its server and relay, then got storage_root_invalid / unsupported unix scheme on its first query. These were the predicted behavioral failures.
  • After: exact head 15586e2678e69378dec8fd58ca7d1b2351a972b3. 2 tests ran; 2 passed (exit 0), both in the clean baseline and again after restoring the checkout. Query/export matched TCP, Blob bytes matched, and mutation/load rows appeared in the export.

Both runs used this command from the checkout root:

env RUSTUP_TOOLCHAIN=1.97.1 \
  CARGO_TARGET_DIR=/tmp/review-988/target \
  RUSTFLAGS='--cfg tokio_unstable --cfg tokio_unstable' \
  RUST_MIN_STACK=16777216 RUSTC_WRAPPER= \
  CARGO_BIN_EXE_omnigraph-server=/tmp/review-988/target/debug/omnigraph-server \
  cargo test --locked -p omnigraph-cli -p omnigraph-server \
  --test integration -j3 issue_979 -- --nocapture

I prebuilt with cargo build --locked -p omnigraph-cli -p omnigraph-server --bin omnigraph-server --test integration -j3 under the same toolchain/flags. This avoided the fixture's initial 30-second server-build timeout. Selecting both packages kept build features aligned; the server test target matched zero cases and is not counted as proof.

Other local checks: the two new unit tests passed. The head-built unit binary, run with graph_http --nocapture, passed six cases; its normally ignored five-second timeout case also passed when run by its full name with --exact --ignored --nocapture. cargo fmt --all --check, git diff --check, bash scripts/check-agents-md.sh, /tmp/review-916-docs/bin/python scripts/check-docs.py, and python3 scripts/check-dependency-sources.py passed. All temporary test edits are restored, and the checkout is clean. Nothing was pushed.

GitHub CI is green. Its workspace log confirms all four new tests passed. That job checked out synthetic merge 4ed8800144a24214486afcf151edd1140b48b65c (this head into ff80af2fbca81ceb396faae8f1d58931442e6c01); it is merge evidence. The local runs above provide the exact-head evidence.

The Windows finding follows directly from the unconditional test and the cfg(not(unix)) refusal; I did not execute a Windows binary. I did not test the documented socat command or a real agent sandbox allowlist. The socket journey uses the PR's byte relay and the real server. No performance benchmark was run. No optional improvements are requested.

Comment thread crates/omnigraph-cli/src/helpers.rs Outdated
ragnorc and others added 2 commits October 13, 2026 22:18
`unix_transport` refuses every `unix://` address on a non-Unix platform, so
the address test's Unix acceptance and malformed-path cases failed wherever
the CLI builds without Unix sockets (the Windows release target). The scheme
classification and the refusal of storage schemes stay unconditional; the
socket path cases run under `cfg(unix)`, and other platforms assert the
platform refusal instead.
@ragnorc
ragnorc enabled auto-merge October 14, 2026 05:32
@ragnorc
ragnorc added this pull request to the merge queue Oct 14, 2026
Merged via the queue into ModernRelay:main with commit 1b514b4 Oct 14, 2026
33 checks passed
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.

CLI: Unix-socket transport for --server, and refuse unknown server schemes

1 participant