[aaltshuler]

What & why

Extract MCP protocol handling from the server's large MCP module into omnigraph-mcp. The new crate owns the SDK, settings, tool/resource catalog, typed arguments, bounded envelopes and response deadlines. The server mounts it at the existing /mcp endpoint and retains authentication, admission, handler dispatch and execution ownership.

Preserve the current unified error contract through the extraction: the host projects both protocol refusals and handler failures through ApiError. Write cancellation, timeout and undeliverable results retain their reconciliation disposition; resources retain structured error data. The protocol crate has no second error-code registry, engine dependency, graph opener or listener.

Backing issue / RFC

Maintainer-requested internal refactor. Related to #991 and RFC 0003; this preserves the shipped MCP surface and does not close the broader issue.

Checklist

• One coherent crate extraction, integrated with current main and its handler/error APIs
• Existing protocol tests moved; error projection, result bounds and execution ownership covered
• Current developer and user documentation updated
• Architectural invariants preserved

Local verification

• cargo test --offline --locked -p omnigraph-mcp -p omnigraph-server --lib — 6 MCP and 213 server tests passed; two standalone subprocess helpers are ignored and exercised by their parent tests.
• RUST_MIN_STACK=16777216 cargo test --offline --locked -p omnigraph-server --test integration -- mcp:: openapi:: — 106 passed; OpenAPI unchanged relative to current main.
• RUST_MIN_STACK=16777216 cargo test --offline --locked -p omnigraph-server --test boot_settings owned_shutdown::disconnected_write_and_shutdown_share_ownership_v2_finish -- --exact — passed.
• cargo clippy --offline --locked -p omnigraph-mcp -p omnigraph-server --all-targets --features omnigraph-server/aws -- -D warnings -W clippy::dbg_macro — passed.
• Formatting, documentation, AGENTS map, dependency-source and workflow-pin checks, merge-group and CI-cell self-tests, changed-file spelling and diff whitespace checks — passed.

The HTTP fixture initially overflowed the default debug-test stack; it passes with the same 16 MiB setting used by repository CI. Full workspace and GQT suites were not run locally; fresh CI owns those broader checks.

Notes for reviewers

Host::start_tool and Host::start_resource register server-owned execution synchronously before returning an observation future. Cancellation, timeout or a dropped observation cannot release the MCP slot, ingress lease or deferred graph capture while execution continues. Write ingress reservation still precedes argument decoding.

The old cancellation test failed when checked against the current canonical error shape before the fix. Server tests now pin the real error registry and write dispositions; protocol tests verify opaque error forwarding, resource data parity and both result-size limits without duplicating the registry. Existing HTTP tests retain stale-write and partial-load assertions.

This is an internal extraction with no intended public behavior change, so no release-note fragment is needed.

[ragnorc]

Summary

This pull request moves the MCP protocol out of omnigraph-server into omnigraph-mcp. The server still mounts /mcp on its one listener. The new crate owns the SDK session, the tool and resource catalog, argument decoding, the result byte bound, and the wait deadline. The server still owns authentication, OAuth metadata, admission, handler dispatch, and execution lifetime.

That split matches the base 7fd9cc90. The catalog text, the argument structs, and the handler calls are the same. start_tool and start_resource register owned work before they return the observation future. A dropped or timed-out observation keeps the MCP slot, the ingress lease, and the graph capture until that work finishes.

The branch does not merge with current main. GitHub reports mergeable: false and mergeable_state: dirty. The only content conflict is crates/omnigraph-server/src/mcp.rs, in nine regions. Those regions come from #1076 (d111c403), which replaced the MCP error body. This branch copies the previous body into the new crate and locks it with tests. A resolution that keeps those helpers reverts the shipped error contract.

Recommendation: request changes. Do not merge until the protocol crate renders the current error body and the server adapter compiles against the current handlers.

Blocking

The new crate freezes the error body that main already replaced

#1076, recorded in changelog.d/mcp-error-body.breaking.md, makes every MCP refusal the body an HTTP route answers:

{"status": <HTTP status, or absent>, "error": {"error", "code", "disposition", ...}}

A write that may have happened uses error.code outcome_unknown and disposition {"effects": "unknown", "action": "reconcile"}. The top-level flag outcome_unknown: true is gone. Refusals that only MCP makes use the registry codes unauthorized, too_many_requests, cancelled, deadline_exceeded, and result_too_large. A resource error carries that same body in data.

On this head a cancelled write is {"code":"cancelled","message":"...","outcome_unknown":true}.

The protocol crate is the right owner of the envelope. It is the wrong owner of a second code table. ErrorOutput lives in omnigraph-api-types, and that crate depends on the engine. This crate stays free of the engine, so it cannot call ApiError itself. The server already owns ApiError. Extend Host so a protocol refusal (cancel, deadline, capacity, missing credential, result too large, and whether the call is a write) is rendered there. The crate only places the rendered body in the tool result or the resource data. Copying the new code strings into failure and outcome_unknown would fix the bytes and leave two tables.

On current main, ApiError has no status field. host_error reads error.status, so that function does not compile on main. Use status(). into_output() already carries code and disposition.

The server wrapper around bounded_result maps every Err to ApiError::internal. A result over 1 MiB must stay result_too_large. For a write, main sets effects: unknown, because the write ran and the receipt did not fit. fit_result needs the write flag for that case.

The handlers on main take ApiJson and ApiPath. server_query, server_mutate, server_load, server_branch_create, and server_branch_merge take ApiJson. server_invoke_query, the commit paths, and server_branch_delete take ApiPath. This adapter still passes Json and Ok(Json(...)).

error_schema in catalog.rs still describes the old fields. Main's schema requires error, and that object requires code and disposition.

The branch copy of docs/user/operations/mcp.md still says "outcome_unknown": true. A merge with main keeps main's paragraph (code: outcome_unknown, effects: unknown) and this branch's intro sentence. Keep that paragraph.

Update tests::a_write_whose_wait_ends_first_reports_an_unknown_outcome to the assertions that pass on main. After the rebase, crates/omnigraph-server/tests/integration/mcp.rs on main must pass as it stands. It already checks error.disposition for a stale conditional write and for a load that publishes its fork first. Add no second fixture.

Optional

Publish an explicit list, not pub use arguments::*

publish-crates.yml publishes omnigraph-mcp, because omnigraph-server is a published crate and cannot depend on publish = false. pub use arguments::* makes every argument struct and the exhaustive ToolCall enum public. The server uses ToolCall, ToolRequest, change_query, Host, HostError, McpSettings, and REQUEST_BYTES. Export those names. Each new tool is then a deliberate public change. McpSettings::resolve returns color_eyre::Result. That signature was already public on the server. It is now public on this crate as well.

Pull request 1118

#1118 relays an agent stdio MCP session to this server. The two branches share no Rust files. docs/user/operations/mcp.md and docs/dev/testing.md merge cleanly between the two heads from 7fd9cc90. The bridge does not link omnigraph_mcp or rmcp. A test in that branch fails if commands/mcp names those crates. The crate move is compatible with the bridge.

The bridge reads the #1076 body: error.code and error.disposition.effects (crates/omnigraph-cli/src/commands/mcp/wire.rs, refusal_data, and cli_mcp.rs on bd54c9da). Landing this branch with the old envelope makes those answers false. Port the body here. The bridge can stay a client of the wire.

Tradeoff and liability

One protocol owner, with trust and execution left on the server, is a smaller long-term surface than a 2,000-line server module. The Host rule is the right seam: the crate opens no graph and no listener, and owned work outlives the HTTP observation.

This branch also publishes that crate and a public Host. That cost is acceptable while the server is the only implementor. It grows if the next change adds a second code table, a second deadline, or a second admission path inside the crate. Five extractions of this shape stay cheap only when each new crate renders a contract that already has one owner.

The net line reduction is real. It is not evidence that the error contract survived. Correctness of the body a client reads comes first. Settings are copied when the router is built. Boot settings do not change after build_app today. A later replacement of AppState.mcp would not reach /mcp.

Validation

Reviewed head: 9395620c2a2583314e351cd6654e50bb92acd0e9.
Current main: 235cd3b9a18aa4b89da57c796e41f504d7ef10b4.
Merge base: 7fd9cc90.
The earlier main tip 13ecdc8b is an ancestor of current main. mcp.rs on main last changed in d111c403 (#1076), which is in both tips.

git merge-tree origin/main 9395620c exits 1. The conflict is only crates/omnigraph-server/src/mcp.rs. Cargo.lock and docs/user/operations/mcp.md auto-merge.

GitHub, read again immediately before this review: state open, head 9395620c, mergeable: false, mergeable_state: dirty. No reviews and no review comments.

CI on this head (run 39514095652): Test Workspace, Lint, Format, GQ Logic Tests, and the storage jobs passed. That run is against the pull request base. It does not include #1076. Devin Review skipped because the trial has no credits.

Local commands:

• On this head, in a detached worktree: cargo test --locked --offline -p omnigraph-mcp --lib -- --test-threads=8. Result: 6 passed, 0 failed.
• Temporary probe, restored, not pushed. tests::a_write_whose_wait_ends_first_reports_an_unknown_outcome was changed to require main's body: no top-level outcome_unknown, error.code = outcome_unknown, disposition = {"effects":"unknown","action":"reconcile"}. Command: cargo test --locked --offline -p omnigraph-mcp --lib tests::a_write_whose_wait_ends_first_reports_an_unknown_outcome -- --exact --nocapture. Result: FAILED. The cancelled write body was {"code":"cancelled","message":"...","outcome_unknown":true}. The file was restored. The original test then passed (1 passed). The worktree is clean.
• On e466a127, which has the same mcp.rs test as origin/main: cargo test --locked --offline -p omnigraph-server --lib mcp::tests::a_write_whose_wait_ends_first_reports_an_unknown_outcome -- --exact --nocapture. Result: 1 passed. That test requires error.code outcome_unknown, effects: unknown, and the absence of the top-level flag.

GQT cannot express this contract. The observable is an MCP tool result, not query rows, column types, or a GQ error. The owners are the protocol unit test and crates/omnigraph-server/tests/integration/mcp.rs.

I did not re-run the server integration suite or the full workspace graph on a merged tree. CI on this head ran them against the old base. Those runs do not prove the merge.

This change does not touch Lance. No storage fence moves.

What to keep

Keep the crate boundary. Keep authentication, OAuth metadata, admission, and spawn_read on the server. Keep argument decoding inside the owned task, after the write-ingress reservation. Keep one listener.

Inline notes

crates/omnigraph-mcp/src/lib.rs:167 — A cancelled or timed-out write takes this arm and returns {code, message, outcome_unknown: true} from outcome_unknown. Current main (#1076, d111c403) returns {status, error} where error.code is outcome_unknown and error.disposition is {"effects":"unknown","action":"reconcile"}. The same function still emits unauthenticated, internal_error, capacity_exceeded, cancelled, and deadline_exceeded as a top-level {code, message}. Main uses the registry codes unauthorized (status 401), too_many_requests (status 429), cancelled, and deadline_exceeded, and a resource error puts that body in data.

This crate should place a body the server already rendered. It should not own a second code table. omnigraph-api-types depends on the engine, so this crate cannot call ApiError. Add a Host method that renders a protocol refusal (kind, and whether the call is a write) through ApiError, including after_owned_write for a write whose wait ended. tool_result and resource_error then use that body only.

error_schema in catalog.rs still lists the old fields. Main requires error with code and disposition. Update tests::a_write_whose_wait_ends_first_reports_an_unknown_outcome to the assertions that pass on main. Copying the new strings into failure leaves two tables.

crates/omnigraph-mcp/src/result.rs:28 — A value over 1 MiB returns Ok(failure("result_too_large", ...)). fit_result below does the same for the SDK text copy, and it has no write flag. On current main both cases are errors. For a write, the body is result_too_large with effects: unknown, because the write already ran and the receipt did not fit.

The server wrapper maps every Err from this function to ApiError::internal (crates/omnigraph-server/src/mcp.rs). A size failure must stay result_too_large, and a write must still reach the owned-write disposition. Return the rendered host body, as with the other protocol refusals. Do not collapse it to internal.

crates/omnigraph-server/src/mcp.rs:369 — This call does not compile against current main. server_query takes ApiJson<QueryRequest>, not Result<Json<_>, _>. server_mutate, server_load, server_branch_create, and server_branch_merge take ApiJson. server_invoke_query, the commit paths, and server_branch_delete take ApiPath. Main's mcp.rs already shows the call shape.

host_error reads error.status. On main ApiError has no status field. Use status(), and pass into_output() through. That output already carries code and disposition. The nine conflict regions in this file are all from #1076. Keep this adapter, and fit it to those handlers and that error type.

crates/omnigraph-mcp/src/lib.rs:14 (optional) — pub use arguments::* publishes every argument struct and the exhaustive ToolCall enum. publish-crates.yml publishes this crate, because the server crate is published and cannot depend on publish = false. The server imports ToolCall, ToolRequest, change_query, Host, HostError, McpSettings, and REQUEST_BYTES. Export that list. A new tool is then a deliberate public change, not an accident of a glob. McpSettings::resolve also returns color_eyre::Result, which becomes part of this crate's public signature.

[ragnorc]

Heads-up on an overlap with #1122 (server-wide WWW-Authenticate, closes #1020), which also edits crates/omnigraph-server/src/mcp.rs.

• fix(server): send WWW-Authenticate on every 401 #1122 takes the 401 challenge out of /mcp. One layer outside every router in build_app (crate::challenge::answer) writes WWW-Authenticate on every 401, from the reason authentication records on the error (Unauthenticated::{NoCredential, Refused}). resource_challenge is gone; /mcp keeps only a no_store layer for Cache-Control: no-store.
• Whichever of the two merges second: wrap the extracted omnigraph_mcp::router in no_store instead of resource_challenge, and keep that outer layer the only writer of WWW-Authenticate. Otherwise /mcp has two writers, and the old one reads Authorization a second time: it treated bearer <token> as presented while authentication refused it as missing.
• mcp::advertised_metadata_url becomes pub(crate), because crate::challenge builds the resource_metadata URL from it once at boot. It needs to stay reachable from the server crate.
• ApiError::unauthorized now takes the reason. The tool-level refusal for a missing actor passes Unauthenticated::NoCredential, which adds nothing to the result body, so the MCP crate's results don't change.