From c8e91c11f087254ae0be52853d9de912dbd40ea0 Mon Sep 17 00:00:00 2001 From: Ragnor Comerford Date: Wed, 17 Jun 2026 16:04:05 +0200 Subject: [PATCH] feat(mcp): per-query @mcp(...) annotation + per-param @description + @instruction folding MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wire the `.gq` authoring surface that controls how a stored query is projected as an MCP tool. All of it rides in the query source (content-addressed, re-parsed at boot), so there is no cluster.yaml / catalog / serving-snapshot plumbing — and it is orthogonal to Cedar `invoke_query` (presentation, not authorization). - Per-parameter `@description("…")` (leading the variable) → carried on `Param.description`, mapped through `param_descriptor`, and emitted on the outer JSON-Schema property by `param_json_schema`, so it shows up in both the MCP tool input schema and the `GET /queries` catalog. - Query `@mcp(expose: , tool_name: "")` → parsed into `QueryDecl.mcp`; `StoredQuery::is_exposed()` / `effective_tool_name()` resolve from it. `expose: false` hides a query from the agent surface (`tools/list`, `stored_query_list`, run-by-name) while keeping it HTTP/service-callable. - `@instruction` is folded into the MCP tool description (after `@description`), so the agent-facing how/when-to-use guidance reaches `tools/list`. - Removes the now-dead `RegistrySpec.{expose, tool_name}` fields (server + CLI); `settings.rs` no longer hardcodes `expose: true`. Test helpers express exposure by injecting `@mcp(expose: false)` into the source (the real path). openapi.json regenerated: `ParamDescriptor` gains an optional `description`. Tests: compiler parser (param @description, @mcp parse + duplicate rejection), api-types schema_equivalence (description on the outer property), server mcp (folded description + param docs + @mcp tool rename, list==call). Full workspace gate green. --- crates/omnigraph-api-types/src/lib.rs | 15 +- .../tests/schema_equivalence.rs | 36 ++++ crates/omnigraph-cli/src/helpers.rs | 6 +- crates/omnigraph-compiler/src/query/ast.rs | 16 ++ crates/omnigraph-compiler/src/query/parser.rs | 160 +++++++++++++----- .../src/query/parser_tests.rs | 58 +++++++ .../omnigraph-compiler/src/query/query.pest | 12 +- crates/omnigraph-server/src/handlers.rs | 3 +- crates/omnigraph-server/src/mcp.rs | 10 +- crates/omnigraph-server/src/queries.rs | 89 +++++----- crates/omnigraph-server/src/settings.rs | 10 +- crates/omnigraph-server/tests/mcp.rs | 61 ++++++- crates/omnigraph-server/tests/support/mod.rs | 20 ++- openapi.json | 7 + 14 files changed, 396 insertions(+), 107 deletions(-) diff --git a/crates/omnigraph-api-types/src/lib.rs b/crates/omnigraph-api-types/src/lib.rs index b26e46c..5733f21 100644 --- a/crates/omnigraph-api-types/src/lib.rs +++ b/crates/omnigraph-api-types/src/lib.rs @@ -383,6 +383,10 @@ pub struct ParamDescriptor { pub vector_dim: Option, /// `false` → the caller must supply it; `true` → optional. pub nullable: bool, + /// Per-parameter documentation from a leading `@description("…")`, surfaced + /// into the JSON-Schema property `description` (MCP tool input + catalog). + #[serde(skip_serializing_if = "Option::is_none")] + pub description: Option, } /// One entry in the stored-query catalog (`GET /queries`). @@ -432,6 +436,7 @@ pub fn param_descriptor(param: &Param) -> ParamDescriptor { item_kind: Some(scalar_kind(pt.scalar)), vector_dim: None, nullable: param.nullable, + description: param.description.clone(), }, Some(pt) => { let (kind, vector_dim) = match pt.scalar { @@ -444,6 +449,7 @@ pub fn param_descriptor(param: &Param) -> ParamDescriptor { item_kind: None, vector_dim, nullable: param.nullable, + description: param.description.clone(), } } // Unreachable for a parsed query (every declared param type is @@ -455,6 +461,7 @@ pub fn param_descriptor(param: &Param) -> ParamDescriptor { item_kind: None, vector_dim: None, nullable: param.nullable, + description: param.description.clone(), }, } } @@ -514,11 +521,17 @@ pub fn param_json_schema(p: &ParamDescriptor) -> Value { // The coercer accepts explicit `null` for a nullable param (and its // omission); a strict client would reject `null` against the bare scalar. // Allow null at the schema level for nullable params. - if p.nullable { + let mut schema = if p.nullable { json!({ "anyOf": [ base, { "type": "null" } ] }) } else { base + }; + // Put the description on the OUTER property object (a sibling of `anyOf` + // for nullable params, never nested inside it), so clients read it directly. + if let Some(description) = &p.description { + schema["description"] = json!(description); } + schema } diff --git a/crates/omnigraph-api-types/tests/schema_equivalence.rs b/crates/omnigraph-api-types/tests/schema_equivalence.rs index b65a013..14e4f91 100644 --- a/crates/omnigraph-api-types/tests/schema_equivalence.rs +++ b/crates/omnigraph-api-types/tests/schema_equivalence.rs @@ -63,6 +63,7 @@ fn descriptor(type_name: &str, nullable: bool) -> omnigraph_api_types::ParamDesc name: "p".to_string(), type_name: type_name.to_string(), nullable, + description: None, }) } @@ -138,6 +139,41 @@ fn nullable_rule_matches_the_parent_coercer() { } } +#[test] +fn param_description_lands_on_the_outer_property() { + let doc = "the user's slug"; + let with_doc = |type_name: &str, nullable: bool| { + param_json_schema(¶m_descriptor(&Param { + name: "p".to_string(), + type_name: type_name.to_string(), + nullable, + description: Some(doc.to_string()), + })) + }; + + // Non-nullable: description is a sibling of the type. + let scalar = with_doc("String", false); + assert_eq!(scalar["description"], json!(doc)); + assert_eq!(scalar["type"], json!("string")); + + // Nullable: description sits on the OUTER object next to `anyOf`, never + // inside it (a consumer reading `anyOf[i].description` must not find it). + let nullable = with_doc("I32", true); + assert_eq!(nullable["description"], json!(doc)); + assert!(nullable.get("anyOf").is_some(), "nullable schema keeps anyOf: {nullable}"); + for branch in nullable["anyOf"].as_array().unwrap() { + assert!(branch.get("description").is_none(), "description leaked into anyOf branch: {branch}"); + } + // Carries on a composite (list) too, and the value still validates. + let list = with_doc("[String]", false); + assert_eq!(list["description"], json!(doc)); + assert!(schema_accepts(&list, &json!(["a", "b"]))); + + // Absent description → no `description` key (wire shape unchanged). + assert!(descriptor("String", false).description.is_none()); + assert!(param_json_schema(&descriptor("String", false)).get("description").is_none()); +} + #[test] fn vector_dim_bounds_are_present_or_omitted() { let with_dim = param_json_schema(&descriptor("Vector(4)", false)); diff --git a/crates/omnigraph-cli/src/helpers.rs b/crates/omnigraph-cli/src/helpers.rs index 971ca30..0bd2cca 100644 --- a/crates/omnigraph-cli/src/helpers.rs +++ b/crates/omnigraph-cli/src/helpers.rs @@ -780,8 +780,6 @@ fn registry_from_serving_queries( .map(|q| omnigraph_server::queries::RegistrySpec { name: q.name.clone(), source: q.source.clone(), - expose: false, - tool_name: None, }) .collect(); QueryRegistry::from_specs(specs).map_err(|errors| { @@ -890,8 +888,8 @@ pub(crate) async fn execute_queries_list( .iter() .map(|q| QueriesListItem { name: q.name.clone(), - mcp_expose: q.expose, - tool_name: q.tool_name.clone(), + mcp_expose: q.is_exposed(), + tool_name: q.decl.mcp.tool_name.clone(), mutation: q.is_mutation(), params: q .decl diff --git a/crates/omnigraph-compiler/src/query/ast.rs b/crates/omnigraph-compiler/src/query/ast.rs index f0bbdb5..53b2611 100644 --- a/crates/omnigraph-compiler/src/query/ast.rs +++ b/crates/omnigraph-compiler/src/query/ast.rs @@ -10,6 +10,10 @@ pub struct QueryDecl { pub name: String, pub description: Option, pub instruction: Option, + /// MCP-presentation controls from the `@mcp(...)` annotation (tool name + + /// visibility on the agent tool surface). Distinct from `description` / + /// `instruction`, which are general docs consumed by both REST and MCP. + pub mcp: McpQueryMeta, pub params: Vec, pub match_clause: Vec, pub return_clause: Vec, @@ -18,11 +22,23 @@ pub struct QueryDecl { pub mutations: Vec, } +/// Parsed `@mcp(...)` annotation. Both fields default to `None`: `expose` +/// absent ⇒ exposed (the historical default); `tool_name` absent ⇒ the query +/// name. Presentation only — never an authorization control. +#[derive(Debug, Clone, Default)] +pub struct McpQueryMeta { + pub expose: Option, + pub tool_name: Option, +} + #[derive(Debug, Clone)] pub struct Param { pub name: String, pub type_name: String, pub nullable: bool, + /// Optional per-parameter documentation from a leading `@description("…")`, + /// surfaced into tool input-schema property descriptions. + pub description: Option, } #[derive(Debug, Clone)] diff --git a/crates/omnigraph-compiler/src/query/parser.rs b/crates/omnigraph-compiler/src/query/parser.rs index 4ba8476..29066c0 100644 --- a/crates/omnigraph-compiler/src/query/parser.rs +++ b/crates/omnigraph-compiler/src/query/parser.rs @@ -50,6 +50,8 @@ fn parse_query_decl(pair: pest::iterators::Pair) -> Result { let mut description = None; let mut instruction = None; + let mut mcp = McpQueryMeta::default(); + let mut mcp_seen = false; let mut params = Vec::new(); let mut match_clause = Vec::new(); let mut return_clause = Vec::new(); @@ -66,33 +68,34 @@ fn parse_query_decl(pair: pest::iterators::Pair) -> Result { } } } - Rule::query_annotation => { - let (annotation_name, value) = parse_query_annotation(item)?; - match annotation_name { - "description" => { - if description.replace(value).is_some() { - return Err(NanoError::Parse(format!( - "query `{}` cannot include duplicate @description annotations", - name - ))); - } - } - "instruction" => { - if instruction.replace(value).is_some() { - return Err(NanoError::Parse(format!( - "query `{}` cannot include duplicate @instruction annotations", - name - ))); - } - } - other => { + Rule::query_annotation => match parse_query_annotation(item)? { + ParsedAnnotation::Description(value) => { + if description.replace(value).is_some() { return Err(NanoError::Parse(format!( - "unsupported query annotation: @{}", - other + "query `{}` cannot include duplicate @description annotations", + name ))); } } - } + ParsedAnnotation::Instruction(value) => { + if instruction.replace(value).is_some() { + return Err(NanoError::Parse(format!( + "query `{}` cannot include duplicate @instruction annotations", + name + ))); + } + } + ParsedAnnotation::Mcp(value) => { + if mcp_seen { + return Err(NanoError::Parse(format!( + "query `{}` cannot include duplicate @mcp annotations", + name + ))); + } + mcp_seen = true; + mcp = value; + } + }, Rule::query_body => { let body = item .into_inner() @@ -157,6 +160,7 @@ fn parse_query_decl(pair: pest::iterators::Pair) -> Result { name, description, instruction, + mcp, params, match_clause, return_clause, @@ -166,32 +170,36 @@ fn parse_query_decl(pair: pest::iterators::Pair) -> Result { }) } -fn parse_query_annotation(pair: pest::iterators::Pair) -> Result<(&'static str, String)> { +enum ParsedAnnotation { + Description(String), + Instruction(String), + Mcp(McpQueryMeta), +} + +/// Extract the single string-literal argument from an `@name("…")`-shaped +/// annotation pair (`description_annotation` / `instruction_annotation`). +fn annotation_string(pair: pest::iterators::Pair, what: &str) -> Result { + pair.into_inner() + .next() + .ok_or_else(|| NanoError::Parse(format!("{what} requires a string literal"))) + .map(|value| parse_string_lit(value.as_str()))? +} + +fn parse_query_annotation(pair: pest::iterators::Pair) -> Result { let inner = pair .into_inner() .next() .ok_or_else(|| NanoError::Parse("query annotation cannot be empty".to_string()))?; match inner.as_rule() { - Rule::description_annotation => { - let value = inner - .into_inner() - .next() - .ok_or_else(|| { - NanoError::Parse("@description requires a string literal".to_string()) - }) - .map(|value| parse_string_lit(value.as_str()))??; - Ok(("description", value)) - } - Rule::instruction_annotation => { - let value = inner - .into_inner() - .next() - .ok_or_else(|| { - NanoError::Parse("@instruction requires a string literal".to_string()) - }) - .map(|value| parse_string_lit(value.as_str()))??; - Ok(("instruction", value)) - } + Rule::description_annotation => Ok(ParsedAnnotation::Description(annotation_string( + inner, + "@description", + )?)), + Rule::instruction_annotation => Ok(ParsedAnnotation::Instruction(annotation_string( + inner, + "@instruction", + )?)), + Rule::mcp_annotation => Ok(ParsedAnnotation::Mcp(parse_mcp_annotation(inner)?)), other => Err(NanoError::Parse(format!( "unexpected query annotation rule: {:?}", other @@ -199,9 +207,70 @@ fn parse_query_annotation(pair: pest::iterators::Pair) -> Result<(&'static } } +/// Parse `@mcp(expose: , tool_name: "")` into [`McpQueryMeta`]. +/// Both keys are optional; a repeated key is a loud error. +fn parse_mcp_annotation(pair: pest::iterators::Pair) -> Result { + let mut meta = McpQueryMeta::default(); + for arg in pair.into_inner() { + let kv = arg + .into_inner() + .next() + .ok_or_else(|| NanoError::Parse("@mcp argument cannot be empty".to_string()))?; + match kv.as_rule() { + Rule::mcp_expose_arg => { + let value = kv + .into_inner() + .next() + .ok_or_else(|| { + NanoError::Parse("@mcp expose requires a boolean".to_string()) + })? + .as_str() + == "true"; + if meta.expose.replace(value).is_some() { + return Err(NanoError::Parse( + "@mcp cannot include duplicate `expose` arguments".to_string(), + )); + } + } + Rule::mcp_tool_name_arg => { + let value = kv + .into_inner() + .next() + .ok_or_else(|| { + NanoError::Parse("@mcp tool_name requires a string literal".to_string()) + }) + .map(|value| parse_string_lit(value.as_str()))??; + if meta.tool_name.replace(value).is_some() { + return Err(NanoError::Parse( + "@mcp cannot include duplicate `tool_name` arguments".to_string(), + )); + } + } + other => { + return Err(NanoError::Parse(format!( + "unexpected @mcp argument rule: {:?}", + other + ))); + } + } + } + Ok(meta) +} + fn parse_param(pair: pest::iterators::Pair) -> Result { let mut inner = pair.into_inner(); - let var = inner.next().unwrap().as_str(); + let mut next = inner + .next() + .ok_or_else(|| NanoError::Parse("parameter is missing a variable".to_string()))?; + // Optional leading `@description("…")` documents the parameter. + let mut description = None; + if next.as_rule() == Rule::description_annotation { + description = Some(annotation_string(next, "@description")?); + next = inner + .next() + .ok_or_else(|| NanoError::Parse("parameter is missing a variable".to_string()))?; + } + let var = next.as_str(); let name = var.strip_prefix('$').unwrap_or(var).to_string(); let type_ref = inner.next().unwrap(); let nullable = type_ref.as_str().trim_end().ends_with('?'); @@ -237,6 +306,7 @@ fn parse_param(pair: pest::iterators::Pair) -> Result { name, type_name: base, nullable, + description, }) } diff --git a/crates/omnigraph-compiler/src/query/parser_tests.rs b/crates/omnigraph-compiler/src/query/parser_tests.rs index 5267a36..24f3851 100644 --- a/crates/omnigraph-compiler/src/query/parser_tests.rs +++ b/crates/omnigraph-compiler/src/query/parser_tests.rs @@ -62,6 +62,64 @@ return { $p.name } assert!(err.to_string().contains("duplicate @description")); } +#[test] +fn test_parse_param_description() { + let input = r#" +query find(@description("the user's slug") $slug: String, $limit: I32?) { +match { + $u: User { slug: $slug } +} +return { $u.name } +} +"#; + let qf = parse_query(input).unwrap(); + let q = &qf.queries[0]; + assert_eq!(q.params.len(), 2); + // Annotated param keeps its name/type and gains the doc. + assert_eq!(q.params[0].name, "slug"); + assert_eq!(q.params[0].type_name, "String"); + assert!(!q.params[0].nullable); + assert_eq!(q.params[0].description.as_deref(), Some("the user's slug")); + // Un-annotated param: no description, nullable preserved (annotation slot + // sits before the variable, so it composes with the trailing `?`). + assert_eq!(q.params[1].name, "limit"); + assert!(q.params[1].nullable); + assert_eq!(q.params[1].description, None); +} + +#[test] +fn test_parse_mcp_annotation() { + // Either argument order parses; expose + tool_name both captured. + for input in [ + r#"query q() @mcp(tool_name: "lookup", expose: false) { match { $p: Person } return { $p.name } }"#, + r#"query q() @mcp(expose: false, tool_name: "lookup") { match { $p: Person } return { $p.name } }"#, + ] { + let qf = parse_query(input).unwrap(); + let q = &qf.queries[0]; + assert_eq!(q.mcp.tool_name.as_deref(), Some("lookup"), "input: {input}"); + assert_eq!(q.mcp.expose, Some(false), "input: {input}"); + } + + // Absent @mcp ⇒ both None (exposed-by-default, name as tool name). + let bare = parse_query(r#"query q() { match { $p: Person } return { $p.name } }"#).unwrap(); + assert_eq!(bare.queries[0].mcp.tool_name, None); + assert_eq!(bare.queries[0].mcp.expose, None); +} + +#[test] +fn test_duplicate_mcp_annotation_is_rejected() { + let dup_block = r#"query q() @mcp(expose: true) @mcp(expose: false) { match { $p: Person } return { $p.name } }"#; + assert!( + parse_query(dup_block).unwrap_err().to_string().contains("duplicate @mcp"), + "two @mcp annotations must be rejected" + ); + let dup_key = r#"query q() @mcp(expose: true, expose: false) { match { $p: Person } return { $p.name } }"#; + assert!( + parse_query(dup_key).unwrap_err().to_string().contains("duplicate `expose`"), + "a repeated @mcp key must be rejected" + ); +} + #[test] fn test_parse_no_params() { let input = r#" diff --git a/crates/omnigraph-compiler/src/query/query.pest b/crates/omnigraph-compiler/src/query/query.pest index 353c226..d7c2eaa 100644 --- a/crates/omnigraph-compiler/src/query/query.pest +++ b/crates/omnigraph-compiler/src/query/query.pest @@ -12,9 +12,14 @@ query_decl = { ~ query_body ~ "}" } -query_annotation = { description_annotation | instruction_annotation } +query_annotation = { description_annotation | instruction_annotation | mcp_annotation } description_annotation = { "@description" ~ "(" ~ string_lit ~ ")" } instruction_annotation = { "@instruction" ~ "(" ~ string_lit ~ ")" } +// MCP-presentation controls (the agent tool surface only): tool name + visibility. +mcp_annotation = { "@mcp" ~ "(" ~ mcp_arg ~ ("," ~ mcp_arg)* ~ ","? ~ ")" } +mcp_arg = { mcp_expose_arg | mcp_tool_name_arg } +mcp_expose_arg = { "expose" ~ ":" ~ bool_lit } +mcp_tool_name_arg = { "tool_name" ~ ":" ~ string_lit } query_body = { read_query_body | mutation_body } mutation_body = { mutation_stmt+ } @@ -33,7 +38,10 @@ mutation_assignment = { ident ~ ":" ~ match_value ~ ","? } mutation_predicate = { ident ~ comp_op ~ match_value } param_list = { param ~ ("," ~ param)* } -param = { variable ~ ":" ~ type_ref } +// A leading `@description("…")` documents the parameter (surfaced in tool input +// schemas). Leading position avoids PEG ambiguity with `type_ref`'s trailing `?` +// and the `,` separator. +param = { description_annotation? ~ variable ~ ":" ~ type_ref } type_ref = { (list_type | base_type | vector_type) ~ "?"? } list_type = { "[" ~ base_type ~ "]" } diff --git a/crates/omnigraph-server/src/handlers.rs b/crates/omnigraph-server/src/handlers.rs index c2c0263..59e09fe 100644 --- a/crates/omnigraph-server/src/handlers.rs +++ b/crates/omnigraph-server/src/handlers.rs @@ -1087,8 +1087,7 @@ pub(crate) async fn server_list_queries( )?; let queries = match handle.queries.as_ref() { Some(registry) => registry - .iter() - .filter(|q| q.expose) + .exposed() .map(api::query_catalog_entry) .collect(), None => Vec::new(), diff --git a/crates/omnigraph-server/src/mcp.rs b/crates/omnigraph-server/src/mcp.rs index 1e94357..4af8828 100644 --- a/crates/omnigraph-server/src/mcp.rs +++ b/crates/omnigraph-server/src/mcp.rs @@ -535,11 +535,19 @@ fn stored_query_input_schema(stored: &StoredQuery) -> Value { } fn stored_query_tool(stored: &StoredQuery) -> Tool { - let description = stored + // The MCP tool description folds `@description` and `@instruction` (the + // agent-facing "how to use" guidance) into the one description slot MCP + // tools have. Instruction-only queries still surface their instruction + // (appended to the fallback base). + let mut description = stored .decl .description .clone() .unwrap_or_else(|| format!("Stored query '{}'.", stored.name)); + if let Some(instruction) = &stored.decl.instruction { + description.push_str("\n\n"); + description.push_str(instruction); + } let annotations = if stored.is_mutation() { write_annotations(true) } else { diff --git a/crates/omnigraph-server/src/queries.rs b/crates/omnigraph-server/src/queries.rs index 085800d..842b4e1 100644 --- a/crates/omnigraph-server/src/queries.rs +++ b/crates/omnigraph-server/src/queries.rs @@ -31,15 +31,8 @@ pub struct StoredQuery { pub name: String, /// Full `.gq` source text the query was selected from. pub source: Arc, - /// Parsed declaration (params, mutations, description, …). + /// Parsed declaration (params, mutations, description, `@mcp(...)`, …). pub decl: QueryDecl, - /// Whether this query is listed in the MCP tool catalog (`GET /queries`). - /// Default `true` (the manifest entry is the opt-in); `expose: false` - /// keeps it HTTP/service-callable but hidden from the agent tool list. - /// Catalog membership only — not an authorization gate. - pub expose: bool, - /// Optional MCP tool-name override; defaults to `name`. - pub tool_name: Option, } impl StoredQuery { @@ -49,13 +42,22 @@ impl StoredQuery { !self.decl.mutations.is_empty() } - /// The MCP tool name this query is catalogued under: the explicit - /// `tool_name` override, else the query `name`. The catalog key — - /// enforced unique across exposed queries at load. Server-side - /// consumers (the uniqueness check, the future catalog projection) read - /// this; the CLI `queries list` resolves the same rule on its own DTO. + /// Whether this query is listed on the MCP tool surface (`GET /queries` and + /// the `/mcp` tool catalog). From the source `@mcp(expose: …)` annotation; + /// absent ⇒ `true`. An unexposed query stays HTTP/service-callable by name — + /// this is **presentation only, not an authorization gate** (Cedar + /// `invoke_query` is the authority for who may call it). + pub fn is_exposed(&self) -> bool { + self.decl.mcp.expose.unwrap_or(true) + } + + /// The MCP tool name this query is catalogued under: the source + /// `@mcp(tool_name: …)` override, else the query `name`. The catalog key — + /// enforced unique across exposed queries at load. Server-side consumers + /// (the uniqueness check, the catalog/MCP projection) read this; the CLI + /// `queries list` resolves the same rule on its own DTO. pub fn effective_tool_name(&self) -> &str { - self.tool_name.as_deref().unwrap_or(&self.name) + self.decl.mcp.tool_name.as_deref().unwrap_or(&self.name) } } @@ -67,13 +69,13 @@ pub struct QueryRegistry { /// In-memory registry spec: a query's name + already-read `.gq` source. The /// input to [`QueryRegistry::from_specs`] — built by the server's cluster boot -/// and by the CLI's `queries` tooling from a cluster serving snapshot. +/// and by the CLI's `queries` tooling from a cluster serving snapshot. MCP +/// presentation (`expose` / `tool_name`) is carried in the source `@mcp(...)` +/// annotation, not here — see [`StoredQuery::is_exposed`] / `effective_tool_name`. #[derive(Debug, Clone)] pub struct RegistrySpec { pub name: String, pub source: String, - pub expose: bool, - pub tool_name: Option, } /// A single registry load failure. Collected (not fail-fast) so a bad @@ -120,8 +122,6 @@ impl QueryRegistry { name: spec.name, source: Arc::from(spec.source), decl, - expose: spec.expose, - tool_name: spec.tool_name, }, ); } @@ -159,7 +159,7 @@ impl QueryRegistry { for builtin in crate::mcp::BUILTIN_TOOL_NAMES { claimed.insert(builtin, BUILTIN_OWNER); } - for query in by_name.values().filter(|q| q.expose) { + for query in by_name.values().filter(|q| q.is_exposed()) { let tool = query.effective_tool_name(); if let Some(winner) = claimed.insert(tool, &query.name) { let message = if winner == BUILTIN_OWNER { @@ -182,11 +182,11 @@ impl QueryRegistry { } } - /// Resolve by symbol name, **ignoring `expose`**. The raw catalog accessor - /// for HTTP/service callers (`expose:false` queries are deliberately - /// HTTP-callable; see [`StoredQuery::expose`]). The MCP backend must NOT use - /// this — it resolves through [`Self::exposed_by_name`] so the agent surface - /// can never reach a query hidden from the tool list. + /// Resolve by symbol name, **ignoring exposure**. The raw catalog accessor + /// for HTTP/service callers (`@mcp(expose: false)` queries are deliberately + /// HTTP-callable; see [`StoredQuery::is_exposed`]). The MCP backend must NOT + /// use this — it resolves through [`Self::exposed_by_name`] so the agent + /// surface can never reach a query hidden from the tool list. pub fn lookup(&self, name: &str) -> Option<&StoredQuery> { self.by_name.get(name) } @@ -201,14 +201,14 @@ impl QueryRegistry { /// `stored_query_list` tool, and per-query tool dispatch all funnel through /// it, so they cannot drift on which queries an agent may see or run. pub fn exposed(&self) -> impl Iterator { - self.by_name.values().filter(|q| q.expose) + self.by_name.values().filter(|q| q.is_exposed()) } /// Resolve by symbol name, **exposed-only** — the MCP `stored_query_run` /// resolver. An unexposed query is unreachable by name through this path /// even to a caller that knows the name (the agent surface honors `expose`). pub fn exposed_by_name(&self, name: &str) -> Option<&StoredQuery> { - self.by_name.get(name).filter(|q| q.expose) + self.by_name.get(name).filter(|q| q.is_exposed()) } pub fn is_empty(&self) -> bool { @@ -284,7 +284,7 @@ pub fn check(registry: &QueryRegistry, catalog: &Catalog) -> CheckReport { message: err.to_string(), }); } - if query.expose { + if query.is_exposed() { for param in &query.decl.params { // Resolve to the structured type via the compiler's own // resolver rather than string-matching `Vector(` — one @@ -332,21 +332,34 @@ pub fn format_check_breakages(label: &str, report: &CheckReport) -> String { mod tests { use super::*; - fn spec(name: &str, source: &str, expose: bool) -> RegistrySpec { - RegistrySpec { - name: name.to_string(), - source: source.to_string(), - expose, - tool_name: None, + /// Inject an `@mcp()` annotation between the param-list `)` and the + /// body `{` (the first `{` in a query source is always the body open). + /// MCP presentation now lives in the source, so the test helpers express + /// expose/tool_name by rewriting the `.gq` rather than via dead spec fields. + fn inject_mcp(source: &str, args: &str) -> String { + match source.find('{') { + Some(i) => format!("{}@mcp({}) {}", &source[..i], args, &source[i..]), + None => source.to_string(), } } + fn spec(name: &str, source: &str, expose: bool) -> RegistrySpec { + let source = if expose { + source.to_string() + } else { + inject_mcp(source, "expose: false") + }; + RegistrySpec { name: name.to_string(), source } + } + fn spec_tool(name: &str, source: &str, expose: bool, tool_name: &str) -> RegistrySpec { + let mut args = format!("tool_name: {tool_name:?}"); + if !expose { + args.push_str(", expose: false"); + } RegistrySpec { name: name.to_string(), - source: source.to_string(), - expose, - tool_name: Some(tool_name.to_string()), + source: inject_mcp(source, &args), } } @@ -360,7 +373,7 @@ mod tests { .unwrap(); let q = reg.lookup("find_user").unwrap(); assert_eq!(q.name, "find_user"); - assert!(q.expose); + assert!(q.is_exposed()); assert_eq!(q.decl.params.len(), 1); assert!(!q.is_mutation()); // No override → the effective tool name is the query name. diff --git a/crates/omnigraph-server/src/settings.rs b/crates/omnigraph-server/src/settings.rs index bb6febd..7c0e620 100644 --- a/crates/omnigraph-server/src/settings.rs +++ b/crates/omnigraph-server/src/settings.rs @@ -77,11 +77,9 @@ pub(crate) async fn load_cluster_settings( .map(|query| queries::RegistrySpec { name: query.name.clone(), source: query.source.clone(), - // The §D5 bridge: the cluster registry has no expose flag - // (exposure becomes a policy decision in Phase 6) — cluster - // mode lists every stored query. - expose: true, - tool_name: None, + // MCP presentation (expose / tool_name) rides in the `.gq` + // source `@mcp(...)` annotation, re-parsed here; the registry + // spec carries only identity + source. }) .collect(); let registry = QueryRegistry::from_specs(specs).map_err(|errors| { @@ -392,8 +390,6 @@ mod tests { let spec = |name: &str, source: &str| RegistrySpec { name: name.to_string(), source: source.to_string(), - expose: false, - tool_name: None, }; // Empty registry → nothing attached, no error. diff --git a/crates/omnigraph-server/tests/mcp.rs b/crates/omnigraph-server/tests/mcp.rs index 960f813..eb597d6 100644 --- a/crates/omnigraph-server/tests/mcp.rs +++ b/crates/omnigraph-server/tests/mcp.rs @@ -518,6 +518,65 @@ async fn write_tool_listed_when_only_unprotected_writes_allowed() { ); } +#[tokio::test] +async fn stored_query_tool_folds_docs_and_honors_mcp_annotation() { + // A query carrying @description + @instruction + a per-param @description + + // @mcp(tool_name: …) projects as ONE tool whose name is the override, whose + // description folds the instruction in, and whose input schema documents the + // param — and it is callable under the override name (list == call). + const SRC: &str = r#"query find_person(@description("the person's exact name") $name: String) +@description("Find a person by name.") +@instruction("Use only for an exact name; for fuzzy matches use search.") +@mcp(tool_name: "lookup_person") +{ match { $p: Person { name: $name } } return { $p.age } }"#; + + let (_t, app) = app_with_stored_queries( + &[("find_person", SRC, true)], + &[("act-invoke", "tok")], + INVOKE_POLICY_YAML, + ) + .await; + + let (_s, list) = + json_response(&app, mcp_request(Some("tok"), rpc(1, "tools/list", json!({})))).await; + let tools = list["result"]["tools"].as_array().unwrap(); + let tool = tools + .iter() + .find(|t| t["name"] == json!("lookup_person")) + .unwrap_or_else(|| panic!("lookup_person not listed: {:?}", tool_names(&list))); + // The @mcp tool name replaces the query name on the surface. + assert!( + !tool_names(&list).contains(&"find_person".to_string()), + "query name must not double as a tool: {:?}", + tool_names(&list) + ); + // Description folds @description then @instruction. + let desc = tool["description"].as_str().unwrap(); + assert!(desc.contains("Find a person by name."), "description: {desc}"); + assert!(desc.contains("Use only for an exact name"), "instruction folded in: {desc}"); + // The parameter carries its @description in the tool input schema. + let param_desc = + tool["inputSchema"]["properties"]["params"]["properties"]["name"]["description"].as_str(); + assert_eq!( + param_desc, + Some("the person's exact name"), + "param doc in input schema: {}", + tool["inputSchema"] + ); + + // Callable under the override name (list and call agree). + let (status, v) = json_response( + &app, + mcp_request( + Some("tok"), + rpc(2, "tools/call", json!({ "name": "lookup_person", "arguments": { "params": { "name": "Nobody" } } })), + ), + ) + .await; + assert_eq!(status, StatusCode::OK); + assert_ne!(v["result"]["isError"], json!(true), "renamed tool not callable: {v}"); +} + #[tokio::test] async fn per_query_mode_does_not_expose_meta_tools() { // Below the auto threshold the projection is per-query, so the discovery + @@ -607,8 +666,6 @@ fn stored_query_shadowing_a_builtin_is_a_load_error() { let result = QueryRegistry::from_specs(vec![RegistrySpec { name: "graph_query".to_string(), source: "query graph_query() { match { $p: Person } return { $p.name } }".to_string(), - expose: true, - tool_name: None, }]); let errors = result.expect_err("expected a collision error"); assert!( diff --git a/crates/omnigraph-server/tests/support/mod.rs b/crates/omnigraph-server/tests/support/mod.rs index 157c58e..a481e87 100644 --- a/crates/omnigraph-server/tests/support/mod.rs +++ b/crates/omnigraph-server/tests/support/mod.rs @@ -145,14 +145,24 @@ pub fn graph_path(root: &Path) -> PathBuf { } pub fn stored_query_registry(specs: &[(&str, &str, bool)]) -> QueryRegistry { + // MCP `expose` now lives in the `.gq` source `@mcp(...)` annotation. The + // `(name, source, expose)` tuple stays for ergonomics: when `expose` is + // false, inject `@mcp(expose: false)` between the param-list `)` and the + // body `{` (the first `{` in a query source is always the body open), so + // the real parse path is exercised. QueryRegistry::from_specs( specs .iter() - .map(|(name, source, expose)| RegistrySpec { - name: name.to_string(), - source: source.to_string(), - expose: *expose, - tool_name: None, + .map(|(name, source, expose)| { + let source = if *expose { + source.to_string() + } else { + match source.find('{') { + Some(i) => format!("{}@mcp(expose: false) {}", &source[..i], &source[i..]), + None => source.to_string(), + } + }; + RegistrySpec { name: name.to_string(), source } }) .collect(), ) diff --git a/openapi.json b/openapi.json index fb76fae..b3c1be9 100644 --- a/openapi.json +++ b/openapi.json @@ -2198,6 +2198,13 @@ "nullable" ], "properties": { + "description": { + "type": [ + "string", + "null" + ], + "description": "Per-parameter documentation from a leading `@description(\"…\")`, surfaced\ninto the JSON-Schema property `description` (MCP tool input + catalog)." + }, "item_kind": { "oneOf": [ {