From 36c3a16dbaa02538de3d6ad32693fb9a6d66ca4b Mon Sep 17 00:00:00 2001 From: Ragnor Comerford Date: Sat, 30 May 2026 22:05:18 +0200 Subject: [PATCH] Refuse duplicate MCP tool names across exposed stored queries MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The effective MCP tool name (explicit tool_name, else the query name) is a second identity namespace beside the registry key, but nothing enforced it unique — two exposed queries could claim one catalog key, and each consumer re-derived the name ad hoc. Add StoredQuery::effective_tool_name() as the one definition, and a load-time uniqueness pass in from_specs over exposed queries: a collision is a collected LoadError naming the loser and the winner. Scoped to exposed queries (unexposed have no MCP tool); deterministic over the BTreeMap so the first-declared wins and the error order is stable. New (rare) refusal: a config with colliding exposed tool names now fails `omnigraph queries validate` offline and refuses server boot, the same posture as a malformed registry. Release-note-worthy. Test-first: duplicate_exposed_tool_name_is_a_load_error (red before the pass, green after) + a CLI offline test; the unexposed sibling pins the exposed-only scope; effective_tool_name asserts folded into the load test. --- crates/omnigraph-cli/tests/cli.rs | 58 +++++++++++++++++- crates/omnigraph-server/src/queries.rs | 83 ++++++++++++++++++++++++++ 2 files changed, 140 insertions(+), 1 deletion(-) diff --git a/crates/omnigraph-cli/tests/cli.rs b/crates/omnigraph-cli/tests/cli.rs index 1e61fe5..2d08965 100644 --- a/crates/omnigraph-cli/tests/cli.rs +++ b/crates/omnigraph-cli/tests/cli.rs @@ -2425,9 +2425,24 @@ fn queries_list_prints_registered_query() { "find_person.gq", "query find_person($name: String) { match { $p: Person { name: $name } } return { $p.age } }", ); + // Exposed with an explicit tool name so the list shows the MCP suffix. let config = graph.write_config( "omnigraph.yaml", - &queries_test_config(&graph.path().to_string_lossy(), "find_person", "find_person.gq"), + &format!( + concat!( + "graphs:\n", + " local:\n", + " uri: '{}'\n", + " queries:\n", + " find_person:\n", + " file: ./find_person.gq\n", + " mcp: {{ expose: true, tool_name: lookup_person }}\n", + "cli:\n", + " graph: local\n", + "policy: {{}}\n", + ), + graph.path().to_string_lossy().replace('\'', "''") + ), ); let output = output_success(cli().arg("queries").arg("list").arg("--config").arg(&config)); let stdout = stdout_string(&output); @@ -2436,4 +2451,45 @@ fn queries_list_prints_registered_query() { stdout.contains("$name: String"), "list should show typed params; stdout:\n{stdout}" ); + assert!( + stdout.contains("[mcp: lookup_person]"), + "list should show the MCP tool name for exposed queries; stdout:\n{stdout}" + ); +} + +#[test] +fn queries_validate_exits_nonzero_on_duplicate_tool_name() { + // Two exposed queries claiming one MCP tool name is a load-time + // collision — `queries validate` must fail (offline, before the engine + // opens) and name both queries plus the contested tool. + let graph = SystemGraph::loaded(); + graph.write_query("a.gq", "query a() { match { $p: Person } return { $p.name } }"); + graph.write_query("b.gq", "query b() { match { $p: Person } return { $p.name } }"); + let config = graph.write_config( + "omnigraph.yaml", + &format!( + concat!( + "graphs:\n", + " local:\n", + " uri: '{}'\n", + " queries:\n", + " a:\n", + " file: ./a.gq\n", + " mcp: {{ expose: true, tool_name: dup }}\n", + " b:\n", + " file: ./b.gq\n", + " mcp: {{ expose: true, tool_name: dup }}\n", + "cli:\n", + " graph: local\n", + "policy: {{}}\n", + ), + graph.path().to_string_lossy().replace('\'', "''") + ), + ); + let output = output_failure(cli().arg("queries").arg("validate").arg("--config").arg(&config)); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("dup") && stderr.contains("'a'") && stderr.contains("'b'"), + "duplicate tool name should be reported naming both queries; stderr:\n{stderr}" + ); } diff --git a/crates/omnigraph-server/src/queries.rs b/crates/omnigraph-server/src/queries.rs index 075a104..868866b 100644 --- a/crates/omnigraph-server/src/queries.rs +++ b/crates/omnigraph-server/src/queries.rs @@ -50,6 +50,15 @@ impl StoredQuery { pub fn is_mutation(&self) -> bool { !self.decl.mutations.is_empty() } + + /// The MCP tool name this query is catalogued under: the explicit + /// `tool_name` override, else the query `name`. This is the catalog + /// key — enforced unique across exposed queries at load — and the one + /// definition every consumer (uniqueness check, catalog projection, + /// CLI display) reads, so the resolution can't drift between them. + pub fn effective_tool_name(&self) -> &str { + self.tool_name.as_deref().unwrap_or(&self.name) + } } /// A loaded, identity-checked stored-query registry for one graph. @@ -129,6 +138,27 @@ impl QueryRegistry { } } + // Exposed queries are catalogued under their effective tool name; + // two claiming one name is an MCP-namespace collision. Refuse it at + // load (collected, not fail-fast), naming the loser and the winner. + // Iterating the `BTreeMap` keeps "first declared wins" and the error + // order deterministic. Scoped to a block so these borrows of + // `by_name` end before it is moved into `Self`. + { + let mut claimed: BTreeMap<&str, &str> = BTreeMap::new(); + for query in by_name.values().filter(|q| q.expose) { + let tool = query.effective_tool_name(); + if let Some(winner) = claimed.insert(tool, &query.name) { + errors.push(LoadError { + query: Some(query.name.clone()), + message: format!( + "MCP tool name '{tool}' already claimed by exposed query '{winner}'" + ), + }); + } + } + } + if errors.is_empty() { Ok(Self { by_name }) } else { @@ -286,6 +316,15 @@ mod tests { } } + fn spec_tool(name: &str, source: &str, expose: bool, tool_name: &str) -> RegistrySpec { + RegistrySpec { + name: name.to_string(), + source: source.to_string(), + expose, + tool_name: Some(tool_name.to_string()), + } + } + #[test] fn key_equal_symbol_loads() { let reg = QueryRegistry::from_specs(vec![spec( @@ -299,6 +338,21 @@ mod tests { assert!(q.expose); assert_eq!(q.decl.params.len(), 1); assert!(!q.is_mutation()); + // No override → the effective tool name is the query name. + assert_eq!(q.effective_tool_name(), "find_user"); + + // An explicit override is what the catalog keys on. + let with_tool = QueryRegistry::from_specs(vec![spec_tool( + "find_user", + "query find_user($id: String) { match { $u: User } return { $u.name } }", + true, + "lookup_user", + )]) + .unwrap(); + assert_eq!( + with_tool.lookup("find_user").unwrap().effective_tool_name(), + "lookup_user" + ); } #[test] @@ -326,6 +380,35 @@ mod tests { assert!(reg.lookup("a").is_none(), "only the selected symbol is registered"); } + #[test] + fn duplicate_exposed_tool_name_is_a_load_error() { + // Two MCP-exposed queries claiming one tool name is an ambiguity in + // the catalog key space — refused at load, naming both queries and + // the contested tool. + let errors = QueryRegistry::from_specs(vec![ + spec_tool("a", "query a() { match { $u: User } return { $u.name } }", true, "dup"), + spec_tool("b", "query b() { match { $u: User } return { $u.name } }", true, "dup"), + ]) + .unwrap_err(); + assert_eq!(errors.len(), 1); + let msg = errors[0].to_string(); + assert!(msg.contains("'dup'"), "names the contested tool: {msg}"); + assert!(msg.contains("'a'"), "names the winning query: {msg}"); + assert!(msg.contains("'b'"), "names the losing query: {msg}"); + } + + #[test] + fn duplicate_tool_name_among_unexposed_is_allowed() { + // Unexposed queries have no MCP tool, so a shared effective tool + // name is inert — must not error (pins the exposed-only scope). + let reg = QueryRegistry::from_specs(vec![ + spec_tool("a", "query a() { match { $u: User } return { $u.name } }", false, "dup"), + spec_tool("b", "query b() { match { $u: User } return { $u.name } }", false, "dup"), + ]) + .unwrap(); + assert_eq!(reg.len(), 2); + } + #[test] fn parse_error_surfaces_per_entry() { let errors =