mirror of
https://github.com/ModernRelay/omnigraph.git
synced 2026-06-12 01:45:14 +02:00
Refuse duplicate MCP tool names across exposed stored queries
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.
This commit is contained in:
Ragnor Comerford
parent
8a0df1d305
commit
36c3a16dba
2 changed files with 140 additions and 1 deletions
|
|
@ -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}"
|
||||
);
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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 =
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue