Skip to content

feat(mcp)!: one query tool, and every tool listed under policy - #1207

Merged
ragnorc merged 3 commits into
ModernRelay:mainfrom
ragnorc:mcp-tools-listed-by-policy
Oct 30, 2026
Merged

ragnorc merged 3 commits into
ModernRelay:mainfrom
ragnorc:mcp-tools-listed-by-policy

Conversation

@ragnorc

@ragnorc ragnorc commented Oct 30, 2026 •

Copy link
Copy Markdown
Contributor

Amends RFC 0003 (decision log, 2026-10-29). /mcp hid the ad-hoc read tool and every write tool unless the server started with --mcp-allow-adhoc or --mcp-allow-writes. Each of those tools already runs under its HTTP route's policy action, so the flags closed only the MCP door to access the same credential held over POST /query and /mutate. They protected nothing, and an agent connected to a default server saw allowed tools as missing.

Both flags and their OMNIGRAPH_MCP_ALLOW_* variables are gone, and every tool is listed. The catalog loses its per-tool gate: whether a tool writes (its write contract, deadline and result disposition) now follows from the effect it declares, the same field that sets its read-only annotation, so the two cannot disagree. The server instructions become one constant. omnigraph mcp --write is unchanged: it remains the local agent's consent before the bridge relays a write.

Reads were also split across two tools whose names pointed the wrong way: query ran a stored read and read ran GQ source, while mutate already took either. query now takes query (source) or name (stored), with params, branch and snapshot, and read is gone: one tool per effect, the same arguments meaning the same thing on both, as the CLI's query does. read was listed only behind the removed opt-in, so few clients call it; a stored read now also accepts snapshot, which its route already took.

The server's tool-list test now compares the full listing, with annotations and required arguments, against the shared operation inventory, and the write test covers annotations, the conditional mutate and policy refusals on a default server. The bridge test checks that a server lists every write tool and that only --write admits one. A default-deny test pins a token with no policy: every tool listed, query returns rows, mutate refused 403 with the manifest version unchanged.

For review: this is breaking for any deployment that passes either flag, which now fails at argument parsing, and it widens what an agent can do over MCP to whatever its credential's policy already allowed over HTTP. Operators who relied on the flags to keep agents read-only must narrow the agents' policy instead; the guide and changelog say so. Narrowing agents without touching humans' grants, and granting stored queries without general read, are follow-up policy work, not in this change. Locally, cli_mcp::managed_mcp_renews_control_helper_without_changing_identity and cli_mcp::mcp_renews_expired_helper_credentials_before_dispatch failed with credential_expired against their stand-in endpoint; neither reaches the server, and this branch changes only a doc comment in the CLI, so I read it as their 8-second expiry window on a loaded machine.

`/mcp` hid the ad-hoc `read` tool and the write tools behind the
`--mcp-allow-adhoc` and `--mcp-allow-writes` boot flags. Each tool already
runs under its HTTP route's policy action, so the flags restricted only the
MCP door of access the same credential held over HTTP, and made allowed
tools look missing.

Remove both flags and their environment variables. Every tool is listed;
whether a tool writes now follows from its declared effect, and the server
instructions are one constant. `omnigraph mcp --write` stays as the local
agent's consent.
@ragnorc
ragnorc requested a review from aaltshuler as a code owner October 30, 2026 01:03

ragnorc commented Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

Revise before merge. The authorization path holds. The current operator contract does not.

Why. MCP had a second door. --mcp-allow-adhoc and --mcp-allow-writes hid tools that the same credential could already call over HTTP. That list could drift away from policy.

What. This change removes those flags and OMNIGRAPH_MCP_ALLOW_ADHOC / OMNIGRAPH_MCP_ALLOW_WRITES. /mcp lists every tool. Each tool's write contract comes from its declared effect. The HTTP handler's policy action still authorizes the call.

How. ToolSpec::writes() is true for every effect other than Read. Additive (branches_create) and Destructive keep the write deadline, the write-contract text, outcome_unknown, and readOnlyHint: false. That set matches the old write gate for this catalog. Each ToolCall arm still calls the HTTP handler. omnigraph mcp --write stays a local consent gate on the bridge.

Impact. A token that can change a graph over HTTP can now change it over MCP with no server flag. A token with no applied policy can now run ad-hoc read over MCP. Write tools appear in the list and still return 403 in that mode. A server started with either old flag exits 2. A server that only set the old environment variables starts and ignores them. Direct /mcp clients have no --write switch.

Blocking

  1. The accepted RFC still tells operators to pass a flag that this binary rejects. docs/rfcs/2026-10-16-graph-access-serving-and-cross-process-writes.md is status: accepted. RFC 0003 is status: draft. An accepted RFC stays authoritative until an accepted replacement retires it (docs/rfcs/README.md). Two current sentences still require --mcp-allow-writes:

    • "Write tools need two consents. The existing server is the ceiling: without --mcp-allow-writes it lists no write tools" (around line 614).
    • The self-hosted table row "Agents over MCP" (around line 683).
      I ran omnigraph-server --mcp-allow-writes and --mcp-allow-adhoc from this commit. Both exit 2 with unexpected argument. Rewrite those sentences in this pull request and name them in that RFC's decision log. The gap table at lines 294-295 records the same flag as part of the change that RFC closed. Mark those two cells as history in the same log entry, so a reader does not treat them as the current ceiling.
  2. RFC 0003's body still describes the removed gates. The 2026-10-29 decision log names the settings-table rows, the defence-in-depth sentence, the catalog phase labels, and one rejected alternative. It does not name these sentences, and the body still states them:

    • Evidence, line 445: a tools/list snapshot per setting combination, and "absence of disabled tools".
    • Evidence, line 453: "write tools are absent by default".
    • Evidence, lines 456-457: "a stored mutation runs only with writes enabled".
    • Design, line 317: "the setting that enables it".
      A post-merge amendment rewrites the body and the decision log names each superseded sentence (docs/rfcs/README.md). The rollout note for shipped feat(server): /mcp for every credential source, read parity, resources and opt-in writes #994 can stay as history. These four sentences are the current test and design contract. Rewrite them in this pull request.

Optional

  1. The old environment variables are ignored, and the user guide does not say so. OMNIGRAPH_MCP_ALLOW_WRITES=1 and OMNIGRAPH_MCP_ALLOW_ADHOC=1 with --help exit 0. Help names only --mcp-allowed-host and OMNIGRAPH_MCP_ALLOWED_HOSTS. The breaking note's "either flag" sentence is true for the two CLI flags. A deployment that used only the variables to keep writes off will start and list every tool. Add one sentence to docs/user/operations/mcp.md: the old flags make startup fail, leftover variables are ignored, and a token with no applied policy can still only read. Listed write calls return 403. This build also lists read, which default settings used to hide.

  2. Please add a default-deny regression in crates/omnigraph-server/tests/integration/mcp.rs. The existing write tests use a permit-all policy or a Cedar bundle. They do not cover tokens with no policy, which is the path this change newly lists. GQT cannot express a tool list or a policy envelope. A temporary test, mcp::mcp_default_deny_lists_read_and_refuses_mutate_without_a_policy, failed at the merge base and passed at this head. It asserts that default MCP settings list read and mutate, that read returns rows, that mutate is isError with status 403 and a body containing default-deny, and that the manifest version does not move. I removed the test after the runs. It is not on the branch.

  3. Two tool descriptions still say that a stored mutation runs "when write tools are enabled" (queries and query in crates/omnigraph-mcp/src/catalog.rs). Agents read those sentences. Say that mutate runs the stored mutation and that policy can still refuse it. Two comments still name the removed gates: crates/omnigraph-api-types/src/operations.rs ("MCP boot gates keep their existing owners") and crates/omnigraph-server/src/lib.rs ("which opt-in tools /mcp lists and admits").

  4. Draft docs/rfcs/2026-09-26-self-contained-server-testing.md still starts via mcp with --mcp-allow-adhoc (around line 212). Implementation is not-started. One sentence avoids a future startup failure.

Tradeoff and liability

One policy door is the right long-term contract. Invariant 10 puts trust at the HTTP boundary and enforcement in the engine. The flags hid tools from MCP-only clients. They did not stop the holder of the token from calling HTTP. This pull request removes that guardrail on purpose.

Effect is now the one MCP read/write annotation. Policy actions stay in the HTTP handlers, which already decide the HTTP call. The next tools should add actions there. They should not add boot flags.

The bridge consent is local. Without --write, the bridge lists and admits only tools the server marks readOnlyHint: true, and it refuses another call before the relay (-32602, and the message names --write). The server still authorizes the relayed call. A direct /mcp client has no second switch. A shared person-and-agent token that grants change can now mutate over MCP. Narrow that credential, or split it. Open pull requests #1197, #1149, and #951 are the later narrowing. This change lands the wider list first. That order stays safe when the accepted operator text matches the binary.

This removes a shadow allow-list that could drift from Cedar. It adds exposure until operators narrow credentials, and it adds a duty to keep the accepted RFC and the user guide as the one current contract. The deleted Gate enum is a real simplification. The documentation duty remains.

Write tools still share the 16 MCP slots and the read observer. RFC 0003 already records the new reason: the refusal is explicit and retryable (capacity_exceeded). I did not remeasure that lane.

These paths match the HTTP contract, from code inspection:

  • /mcp is mounted only when the server has a credential source (build_app). Open mode has no MCP.
  • health has no policy action, same as GET /healthz. access evaluates each action and needs no graph action, same as GET /access.
  • Best-practice resources are static skill text. They do not read the graph.
  • An unknown tool stays invalid_params.
  • explain load csv on read still needs change and publishes nothing. read stays readOnlyHint: true, so the bridge relays that call without --write. Policy still applies.
  • Signed-data credentials with no policy are denied for every action in authorize. I read that branch. I did not execute it.
  • The v0.14.0 release notes still name the flags. Leave those snapshots in place. The new breaking note is the correction.

Validation

Head 52171696578fc08147ed824be5345766ce500e5b. Merge base with origin/main: 17557824d2788953d4049fd9c19b1430bff46f60. The pull request is one commit, so checking out its 13 files onto that base reproduces the head tree.

Local, merge base, temporary test (removed afterward):

cargo test -p omnigraph-server --test integration mcp::mcp_default_deny_lists_read_and_refuses_mutate_without_a_policy -- --exact --nocapture

Failed in 0.05s. The assertion was read is not listed under default MCP settings. The list was health, graphs, schema, queries, query, snapshot, access, branches_list, branches_diff, branches_merge_base, commits_list, commits_get, commits_changes, diff, changes_poll, blobs_stat. mutate was not reached. That is the predicted reason: default settings hide read and the write tools.

Local, head sources plus the same temporary test:

RUST_MIN_STACK=16777216 cargo test -p omnigraph-server --test integration -- --exact --test-threads=1 --nocapture mcp::mcp_default_deny_lists_read_and_refuses_mutate_without_a_policy mcp::mcp_tool_list_describes_every_tool mcp::mcp_write_tools_are_conditional_and_policy_gated

Passed. 3 passed, 0 failed, 0.28s. A first run without RUST_MIN_STACK aborted with a stack overflow in the new test before the assertions. CI sets RUST_MIN_STACK=16777216 (.github/workflows/ci.yml, docs/dev/testing.md). That abort is the local stack limit.

Also local, on the head sources:

  • cargo test -p omnigraph-mcp --lib tests::every_tool_parses_its_required_arguments_and_writes_are_annotated -- --exact — 1 passed.
  • RUST_MIN_STACK=16777216 cargo test -p omnigraph-cli --test integration cli_mcp::mcp_lists_and_admits_write_tools_only_with_the_agents_consent -- --exact — 1 passed, 0.77s.
  • omnigraph-server --mcp-allow-writes and --mcp-allow-adhoc — exit 2, unexpected argument.
  • OMNIGRAPH_MCP_ALLOW_WRITES=1 OMNIGRAPH_MCP_ALLOW_ADHOC=1 omnigraph-server --help — exit 0. Help does not name those variables.

CI on this commit, separate from the local runs: the pull request checks are green, including Test Workspace, Lint (clippy), GQ Logic Tests, and CI Gate. Devin Review is a skip ("trial expired and no credits remaining"). This head has no GitHub reviews and no review comments.

Limitations

GQT cannot express this behavior, and this pull request adds no GQT case. I did not run the full workspace test graph locally. CI on this commit did. I did not remeasure MCP slot capacity under write load. No storage, version, index, or query-execution surface changes, so I did not open the pinned Lance 13.0.0 sources (docs/dev/lance.md). The temporary test is not on the branch.

Inline comments that belong on the diff:

  • docs/rfcs/0003-mcp-server-surface.md:516 — This list names the settings-table rows, the defence-in-depth sentence, the catalog phase labels, and one rejected alternative. It does not name the Evidence bullets that still require a per-setting tool list, absent write tools, and a stored mutation only when writes are enabled (lines 445-457). It also leaves "the setting that enables it" in Design (line 317). Rewrite those sentences in this pull request and add them here. The body has to stay current.
  • docs/user/operations/mcp.md:183 — Add the migration next to this contract. The old flags make omnigraph-server exit 2. OMNIGRAPH_MCP_ALLOW_ADHOC and OMNIGRAPH_MCP_ALLOW_WRITES are ignored, so a deployment that set only those variables starts and lists every tool. A token with no applied policy can still only read. Listed write calls return 403. This build also lists read, which default settings used to hide.
  • changelog.d/mcp-tools-listed-by-policy.breaking.md:4 — "Either flag" is the two CLI flags. Both exit 2 on this commit. The environment variables in the previous sentence do not. A process with OMNIGRAPH_MCP_ALLOW_WRITES=1 still starts, and --help does not name the variable. Say here that leftover variables are ignored. The user guide needs the same sentence.

…ult deny

Rewrite the accepted graph-access RFC's two sentences that still required
`--mcp-allow-writes`, and RFC 0003's Design and Evidence sentences that
described per-setting tool lists, naming each in the decision logs. The user
guide, the breaking note and RFC 0003 say that leftover OMNIGRAPH_MCP_ALLOW_*
variables are ignored and that a token with no applied policy still only
reads. Tool descriptions and comments stop naming the removed gates.

Add a regression for a static token with no policy: every tool is listed,
`read` answers, and `mutate` is refused 403 by default deny with the manifest
version unchanged.
@ragnorc

ragnorc commented Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

Addressed in e9f522c.

Blocking:

  1. Graph-access RFC (accepted): the two-consents sentence and the "Agents over MCP" row no longer require --mcp-allow-writes; the applied policy is the ceiling. A 2026-10-30 decision-log entry quotes both superseded sentences and marks the two gap-table cells as history.
  2. RFC 0003: Design's "the setting that enables it" and the three Evidence bullets (per-setting tools/list, write tools absent by default, stored mutation only with writes enabled) are rewritten and named in the 2026-10-29 entry.

Optional:

  1. The user guide, the breaking note and RFC 0003's compatibility section say the old flags stop startup, leftover OMNIGRAPH_MCP_ALLOW_* variables are ignored, and a token with no applied policy still only reads, with write calls refused 403.
  2. Added mcp::mcp_default_deny_lists_read_and_refuses_mutate_without_a_policy, matching your temporary test: read and mutate listed, read returns rows, mutate is isError with status 403 and default-deny, manifest version unchanged. It passes at this head; I did not rerun it at the merge base.
  3. The queries and query descriptions no longer say "when write tools are enabled", and the comments in omnigraph-api-types operations.rs and the server's lib.rs no longer name the removed gates.
  4. The self-contained server testing RFC no longer starts via mcp with --mcp-allow-adhoc.

Locally: server mcp:: 12 passed, omnigraph-mcp 7 passed; docs check, fmt and clippy (1.97.1) clean.

`query` ran a stored read and `read` ran GQ source, while `mutate` already
took either source or a stored name. Make `query` take `query` (source) or
`name` (stored), plus `params`, `branch` and `snapshot`, and remove `read`.
One tool per effect, the same arguments meaning the same thing on both, as
the CLI's `query` does. A stored read now also accepts `snapshot`, which its
route already took.
@ragnorc ragnorc changed the title feat(mcp)!: list every tool and let policy decide each call feat(mcp)!: one query tool, and every tool listed under policy Oct 30, 2026

ragnorc commented Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

Revise before merge. The query fold holds. RFC 0003's compatibility section still states the old tool contract.

Earlier findings

The two blockers at 52171696578fc08147ed824be5345766ce500e5b are resolved in e9f522ce.

  1. The accepted graph-access RFC no longer requires --mcp-allow-writes. The two-consents paragraph and the "Agents over MCP" row name the applied policy as the ceiling. The 2026-10-30 log quotes both superseded sentences and marks the two gap-table cells as history.
  2. RFC 0003 Design and Evidence no longer require per-setting tool lists, absent write tools, or a stored mutation only when writes are enabled. The 2026-10-29 log names those sentences.

The four optional items are resolved. The user guide, the breaking note, and RFC 0003 say the old flags stop startup and that leftover OMNIGRAPH_MCP_ALLOW_* variables are ignored. mcp::mcp_default_deny_lists_read_and_refuses_mutate_without_a_policy is on the branch. The queries and query descriptions, and the comments in operations.rs and the server lib.rs, no longer name the removed gates. The self-contained server testing draft runs via mcp through query with source.

Why

query ran a stored read and read ran GQ source. mutate already takes source or a stored name. One tool per effect matches the CLI.

What holds

query with source calls server_query (POST /graphs/{id}/query). query with only name calls server_invoke_query with expect_mutation: false (POST /graphs/{id}/queries/{name}). A call with neither argument returns 400. Source plus name stays on the ad-hoc path: name selects a declared query in that source. A stored mutation name cannot run through query.

The tool is Effect::Read. readOnlyHint stays true. The write contract, the write deadline, and outcome_unknown stay off this tool. A cancelled query is a cancelled read. explain load csv still requires change and publishes nothing, which is the same rule the old read tool had. The bridge admits the tool without --write. The server still authorizes the call.

Policy matches the HTTP routes. Source uses read, and change for explain load csv. A stored name uses invoke_query, then the inner read gate. An actor with read and without invoke_query gets rows from source and 404 from a stored name (stored query not found). Alice, who holds invoke_query, gets rows from that same name. Default deny permits read and refuses every other per-graph action, so source answers and a stored name is that 404. mutate on a no-policy static token is 403 and the manifest version does not move. This does not give a no-policy token a path that HTTP refuses.

read is removed. A client that called read calls query with the same arguments (query, name, params, branch, snapshot). A client that called query with only name still runs the stored read. A stored read can now pass snapshot, which the invoke route already accepted. The breaking note states the rename.

Blocking

  1. RFC 0003 Compatibility still says "Existing tool names, arguments and result shapes are unchanged" (lines 396-397). The fold removes read and changes query: query and snapshot are new arguments, and name is no longer required. The 2026-10-30 log does not name that sentence. Rewrite it in this pull request and name it in that log entry. State the migration there: callers of read send the same arguments to query, and a stored query that passes only name stays valid.

Optional

  1. The user guide says a no-policy token "still only reads" and that "query answers". Source answers under read. A stored name is invoke_query and returns 404 until a policy grants it. Say that split in the same paragraph, and say that clients of read call query. Add one assertion in crates/omnigraph-server/tests/integration/mcp.rs: an actor with read and without invoke_query gets rows from source and 404 from name. I ran that temporary test at this head. It is not on the branch.
  2. skills/omnigraph/references/stored-queries.md still says MCP query runs a stored read. It also runs GQ source. Agents read this file.
  3. The RFC catalog cell for source says the Cedar action is read. explain load csv on that path requires change and publishes nothing. The user guide already says so. Add the exception to the cell.

Tradeoff and liability

One tool per effect removes a second name for the same read. Authorization stays in the HTTP handlers (server_query, invoke_stored_query, authorize). The new rule is argument precedence, which mutate already uses: source selects POST /query, and a name alone selects invoke. That is the right owner. A new selector type would duplicate those handlers.

The cost is one sentence copied into the catalog, the user guide, RFC 0003, the operation inventory, and the skill. This pull request updated four of those and left the skill and the compatibility bullet. Close that drift in this pull request. Five more tools of this shape stay cheap if each one calls the existing handler and the compatibility section records the break once.

Open drafts #1197, #1149, and #951 can later grant read and invoke_query separately. This fold keeps those as two actions on one tool. The open optimize pull requests do not share this surface.

Validation

Head 5fecb93dfa1a81e7c82cbddb61c5d202016108c0. Merge base with origin/main: 17557824d2788953d4049fd9c19b1430bff46f60.

GQT cannot express a tool list or a policy envelope. The corpus has no via mcp step. The owner is crates/omnigraph-server/tests/integration/mcp.rs.

Local, merge base, temporary test (removed afterward):

RUST_MIN_STACK=16777216 cargo test -p omnigraph-server --test integration mcp::mcp_query_source_answers_without_a_read_tool -- --exact --nocapture --test-threads=1

Failed in 0.08s. query with GQ source returned isError: true, status 400, invalid MCP tool arguments, disposition effects: none. The stored-only query tool rejects the source field. The mutation assertion was not reached.

Local, head, the same temporary test plus the branch tests (removed afterward):

CARGO_TARGET_DIR reused the merge-base build. RUST_MIN_STACK=16777216 cargo test -p omnigraph-server --test integration -- --exact --test-threads=1 --nocapture with:

  • mcp::mcp_query_source_answers_without_a_read_tool
  • mcp::mcp_schema_and_adhoc_read_reuse_their_routes_and_policy
  • mcp::mcp_default_deny_lists_read_and_refuses_mutate_without_a_policy
  • mcp::mcp_tool_list_describes_every_tool
  • mcp::oidc_mcp_reuses_discovery_cedar_and_stored_read_handlers_without_mutations

Passed. 5 passed, 0 failed, 0.34s.

A second temporary test at head, mcp::mcp_query_source_uses_read_and_name_uses_invoke_query (removed afterward), passed in 0.09s. read_only gets rows from source and 404 from name: people. alice, who holds invoke_query, gets rows from that name.

cargo test -p omnigraph-mcp --lib tests::every_tool_parses_its_required_arguments_and_writes_are_annotated -- --exact — 1 passed.

CI on this commit, separate from the local runs: run 41030172646 is green, including Test Workspace, Lint (clippy), GQ Logic Tests, and CI Gate. Devin Review skipped (trial expired, no credits). This head has no GitHub reviews and no inline review comments.

Limitations

I did not run the full workspace test graph locally. CI on this commit did. I did not remeasure the 16 MCP slots under write load. No storage, version, index, or query-execution surface changes, so I did not open the pinned Lance 13.0.0 sources. I did not execute a signed-data token with no policy. authorize still denies every action for that credential, including read. The temporary tests are not on the branch.

Inline comments that belong on the diff:

  • docs/rfcs/0003-mcp-server-surface.md:409 — This bullet does not retire the sentence above it: "Existing tool names, arguments and result shapes are unchanged" (lines 396-397). The fold removes read and changes query: query and snapshot are new, and name is no longer required. Rewrite that sentence in this pull request. The 2026-10-30 decision log must name it. State the migration: callers of read send the same arguments to query. A stored query that passes only name stays valid.
  • docs/rfcs/0003-mcp-server-surface.md:180 — Optional. The source action in this cell is read. explain load csv on server_query requires change and publishes nothing. The user guide states that exception. Add it here so the catalog and the guide agree.
  • docs/user/operations/mcp.md:188 — Optional. "query answers" is true for GQ source under read. A stored name is invoke_query and returns 404 (stored query not found) when the credential has no policy, or has read without invoke_query. Say that split in this paragraph. Also say that clients which called read now call query with the same arguments. The breaking note has the rename. This paragraph does not.
  • skills/omnigraph/references/stored-queries.md:85 — Optional. After the fold, query also runs GQ source. This sentence still describes only the stored read. Agents read this file. Say that source selects POST /query under read, and that a name alone selects the stored read under invoke_query.

@ragnorc
ragnorc added this pull request to the merge queue Oct 30, 2026
Merged via the queue into ModernRelay:main with commit 1a3e7ed Oct 30, 2026
34 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.

1 participant