From 5eb9948af47afb390c80ceca19247aa45e720bbe Mon Sep 17 00:00:00 2001 From: Alex <47092487+ooodummy@users.noreply.github.com> Date: Fri, 16 Oct 2026 13:04:49 -0400 Subject: [PATCH 01/14] Stash --- plugins/semctx/README.md | 13 ++ src/mcp/mod.rs | 7 + src/mcp/presentation.rs | 157 ++++++++++++++++ src/mcp/tools.rs | 27 ++- src/query/mod.rs | 17 +- src/query/render.rs | 375 +++++++++++++++++++++++++++++++-------- tests/cli_workflows.rs | 31 ++-- tests/mcp_metadata.rs | 46 ++++- 8 files changed, 577 insertions(+), 96 deletions(-) create mode 100644 src/mcp/presentation.rs diff --git a/plugins/semctx/README.md b/plugins/semctx/README.md index 95dbe3f..562fcd4 100644 --- a/plugins/semctx/README.md +++ b/plugins/semctx/README.md @@ -28,6 +28,19 @@ repository content searches, so they map to `Grep` like `grep` does. A search of OMP internal URLs (`omp://`, `local://`, `skill://`) or web URLs is not repository discovery, so the adapter does not send it to `semctl hook`. + +Tool presentation comes from the shared MCP server. Search and symbol results +start with a match count and source location. Each hit separates metadata from +the fenced source snippet. Local stale-file warnings remain next to the +location. Tool names and argument schemas stay stable. Display titles are +available to clients that support MCP titles. Failed operations set `isError` +and retain their explanation. + +OMP uses its built-in MCP tool cards. Keep `mcp.renderMarkdownResults` enabled +to render the headings and source fences. OMP controls the card title, colors, +and collapsed preview length. Use its expand control to see more output. +The lifecycle adapter does not register duplicate tools or replace result text. + Do not manually copy shared skills; when a host requires a projected format, generate it from the canonical skill and verify parity in CI. diff --git a/src/mcp/mod.rs b/src/mcp/mod.rs index d5e9205..331d494 100644 --- a/src/mcp/mod.rs +++ b/src/mcp/mod.rs @@ -36,6 +36,7 @@ use crate::engine::{CheckoutKey, CoordinatorStatus, Engine, EngineSettings, Trig use crate::query; use crate::session::SessionContext; +mod presentation; pub(crate) mod readiness; mod tool_types; mod tools; @@ -781,6 +782,12 @@ impl McpServer { } else { ToolAnnotations::new().read_only(true).open_world(false) }); + if let Some(title) = presentation::tool_title(&tool.name) { + tool.title = Some(title.into()); + if let Some(annotations) = &mut tool.annotations { + annotations.title = Some(title.into()); + } + } tool } } diff --git a/src/mcp/presentation.rs b/src/mcp/presentation.rs new file mode 100644 index 0000000..3a86ba1 --- /dev/null +++ b/src/mcp/presentation.rs @@ -0,0 +1,157 @@ +//! Host display titles and status for the existing MCP tool results. + +use rmcp::model::CallToolResult; + +/// Display titles do not change the stable tool names used by clients. +pub(super) fn tool_title(name: &str) -> Option<&'static str> { + Some(match name { + "search_codebase" => "Search code", + "find_definition" => "Find definitions", + "find_references" => "Find references", + "who_calls" => "Find callers", + "implementations_of" => "Find implementations", + "call_path" => "Trace a call path", + "reaches" => "Trace outgoing data flow", + "flows_into" => "Trace incoming data flow", + "flows_between" => "Trace data flow between symbols", + "trace" => "Trace a symbol", + "grep" => "Search text", + "file_outline" => "Show file outline", + "expand_chunk" => "Expand source context", + "symbol_at_position" => "Find symbol at position", + "batch_lookup" => "Look up symbols", + "file_tree" => "Show file tree", + "list_files" => "List indexed files", + "list_projects" => "List projects", + "imports" => "Show imports", + "symbol_edges" => "Show symbol connections", + "external_links" => "Show external references", + "list_domains" => "List search domains", + "index_codebase" => "Index a codebase", + "sync_status" => "Show index status", + "list_codebases" => "List codebases", + "current_context" => "Show codebase context", + "read_source" => "Read indexed source", + "search_symbols" => "Search symbols", + "type_hierarchy" => "Show type hierarchy", + "call_graph" => "Show call graph", + "cycles" => "Find call cycles", + "unused" => "Find unused symbols", + "duplicates" => "Find duplicate code", + "rename_symbol" => "Rename a symbol", + "safe_delete_symbol" => "Delete a symbol", + "replace_symbol_body" => "Replace a symbol body", + "insert_before_symbol" => "Insert before a symbol", + "insert_after_symbol" => "Insert after a symbol", + "undo_edit" => "Undo a symbol edit", + _ => return None, + }) +} + +/// Keep the text contract intact while exposing failures to host tool cards. +/// Query failures use the same discriminator as the CLI. MCP-only failures +/// start with the called tool name, except for the initial-index status. +pub(super) fn mark_failure(name: &str, result: &mut CallToolResult) { + let Some(text) = result.content.first().and_then(|content| content.as_text()) else { + return; + }; + let first_line = text.text.lines().next().unwrap_or_default(); + let tool_failure = first_line.strip_prefix(name).is_some_and(|rest| { + [ + " unavailable — ", + " planning failed:", + " result render failed:", + " refused:", + " failed for ", + ] + .iter() + .any(|prefix| rest.starts_with(prefix)) + }); + let index_failure = name == "index_codebase" + && (first_line == "initial indexing failed" + || first_line.starts_with("initial indexing failed for ")); + if crate::query::is_failure(&text.text) || tool_failure || index_failure { + result.is_error = Some(true); + } +} + +#[cfg(test)] +mod tests { + use rmcp::model::{CallToolResult, Content}; + + use super::mark_failure; + + #[test] + fn failures_keep_their_text_and_set_the_host_error_status() { + for (name, text) in [ + ("search_codebase", "search failed: server unreachable"), + ( + "read_source", + "read_source failed: request either bytes or lines, not both", + ), + ( + "find_definition", + "find_definition unavailable — not logged in", + ), + ( + "rename_symbol", + "rename_symbol planning failed: ambiguous symbol", + ), + ("rename_symbol", "rename_symbol refused: source changed"), + ( + "undo_edit", + "undo_edit result render failed: invalid result", + ), + ( + "index_codebase", + "index_codebase failed for C:\\repo: server unreachable", + ), + ( + "index_codebase", + "initial indexing failed\nerror: embedding failed", + ), + ( + "index_codebase", + "initial indexing failed for C:\\repo: embedding failed", + ), + ] { + let content = vec![Content::text(text)]; + let mut result = CallToolResult::success(content.clone()); + mark_failure(name, &mut result); + assert_eq!(result.is_error, Some(true), "{text}"); + assert_eq!(result.content, content); + } + } + + #[test] + fn empty_results_source_text_and_freshness_warnings_remain_successful() { + for (name, text) in [ + ("search_codebase", "no results"), + ( + "search_codebase", + "**1 match**\n\nsearch failed: is source text", + ), + ( + "sync_status", + "last sync run job-1: failed\n error: embedding failed", + ), + ( + "search_codebase", + "src/lib.rs:1\n\n(index freshness: the last sync FAILED)", + ), + ("index_codebase", "initial indexing complete\npath C:\\repo"), + ("read_source", "rename_symbol refused: this is source text"), + ] { + let mut result = CallToolResult::success(vec![Content::text(text)]); + mark_failure(name, &mut result); + assert_eq!(result.is_error, Some(false), "{text}"); + } + } + + #[test] + fn existing_error_status_is_preserved_without_text() { + let mut result = CallToolResult::error(Vec::new()); + mark_failure("read_source", &mut result); + assert_eq!(result.is_error, Some(true)); + } +} diff --git a/src/mcp/tools.rs b/src/mcp/tools.rs index 65cd482..9bb5fcb 100644 --- a/src/mcp/tools.rs +++ b/src/mcp/tools.rs @@ -5,8 +5,11 @@ use std::path::PathBuf; use anyhow::Result; use rmcp::{ ErrorData, RoleServer, ServerHandler, - handler::server::{router::tool::ToolRouter, wrapper::Parameters}, - model::{ListToolsResult, ServerCapabilities, ServerInfo, Tool}, + handler::server::{router::tool::ToolRouter, tool::ToolCallContext, wrapper::Parameters}, + model::{ + CallToolRequestParams, CallToolResult, Implementation, ListToolsResult, ServerCapabilities, + ServerInfo, Tool, + }, service::RequestContext, tool, tool_handler, tool_router, }; @@ -726,8 +729,8 @@ impl McpServer { impl ServerHandler for McpServer { // Defining these here makes `#[tool_handler]` skip its generated // versions (it checks `has_method`), so the Markdown descriptions - // reach the host. `call_tool` is still generated — it ignores - // descriptions, so the default delegation is correct. + // reach the host. Calls retain the shared query text and report failures + // through MCP so hosts can display the correct tool status. // Not `async`: there is nothing to await, and the trait accepts any // `Future`. Written as async it is a future that never yields, which newer // clippy calls out rather than letting it read as if it might. @@ -754,11 +757,27 @@ impl ServerHandler for McpServer { self.tool_router.get(name).cloned().map(Self::with_doc) } + async fn call_tool( + &self, + request: CallToolRequestParams, + context: RequestContext, + ) -> Result { + let name = request.name.clone(); + let call = ToolCallContext::new(self, request, context); + let mut result = self.tool_router.call(call).await?; + super::presentation::mark_failure(&name, &mut result); + Ok(result) + } + fn get_info(&self) -> ServerInfo { // ServerInfo is #[non_exhaustive] — start from default, then set // the fields we care about. let mut info = ServerInfo::default(); info.capabilities = ServerCapabilities::builder().enable_tools().build(); + info.server_info = Implementation::new("semctx", env!("CARGO_PKG_VERSION")) + .with_title("semctx code intelligence") + .with_description("Search, navigate, and edit indexed code with semctl.") + .with_website_url(env!("CARGO_PKG_REPOSITORY")); // Hosts can prepend server instructions to every tool description. // The session hook delivers server.md once per context segment instead. info diff --git a/src/query/mod.rs b/src/query/mod.rs index 14fee1c..592b63a 100644 --- a/src/query/mod.rs +++ b/src/query/mod.rs @@ -42,19 +42,24 @@ use render::{ /// the same failure. Every hard renderer failure starts its first line with the /// stable ` failed:` form; successful output never does. pub fn cli_result(output: String) -> anyhow::Result { + if is_failure(&output) { + anyhow::bail!(output); + } + Ok(output) +} + +/// Identify the shared query failure prefix without treating source text or +/// freshness warnings later in a result as a failed operation. +pub(crate) fn is_failure(output: &str) -> bool { let first_line = output.lines().next().unwrap_or_default(); - let hard_failure = first_line + first_line .split_once(" failed:") .is_some_and(|(operation, _)| { !operation.is_empty() && operation .bytes() .all(|byte| byte.is_ascii_lowercase() || byte == b'_') - }); - if hard_failure { - anyhow::bail!(output); - } - Ok(output) + }) } /// Map the user's `--prefer` value to the server's `SearchPreference` enum name diff --git a/src/query/render.rs b/src/query/render.rs index 929432d..71dce16 100644 --- a/src/query/render.rs +++ b/src/query/render.rs @@ -3,6 +3,7 @@ //! model. No `Client`, no `async`, no network — just formatting, so this //! layer is cheap to unit-test in isolation. +use std::borrow::Cow; use std::collections::HashSet; use std::fmt::Write; use std::path::Path; @@ -220,9 +221,8 @@ pub(super) fn render_tree(nodes: &[api::FileTreeNode], depth: usize, out: &mut S } } -/// Render a hit list as readable text. Each hit is a location line plus -/// a few lines of snippet — enough for the model to decide whether to -/// expand, without flooding the context. +/// Render a hit list as Markdown. The count and first location lead the output. +/// Each hit has a heading, metadata, and a fenced snippet. /// /// `show_score` gates the relevance score: it's meaningful for `search` /// (ranked by hybrid similarity) but a constant 0.000 for the symbol-graph @@ -267,7 +267,11 @@ pub(super) fn render_hits_inner( let weak_below = show_score .then(|| hits.iter().map(|h| h.score).fold(f64::MIN, f64::max) * WEAK_HIT_FRACTION); - let mut out = String::new(); + let mut out = format!( + "**{} {}**\n\n", + hits.len(), + if hits.len() == 1 { "match" } else { "matches" } + ); let mut separated = false; for h in hits { let root = context.root_for(h); @@ -275,73 +279,122 @@ pub(super) fn render_hits_inner( && !separated && h.score < floor { - out.push_str("--- weaker matches (below "); - write!( - out, - "{:.0}% of top score) ---\n\n", - WEAK_HIT_FRACTION * 100.0 - ) - .expect("writing to a String cannot fail"); + out.push_str("### Weaker matches (below "); + write!(out, "{:.0}% of top score)\n\n", WEAK_HIT_FRACTION * 100.0) + .expect("writing to a String cannot fail"); separated = true; } - let lang = h - .language - .as_deref() - .map(|l| format!(" {l}")) - .unwrap_or_default(); - let score = if show_score { - format!(" (score {:.3})", h.score) - } else { - String::new() - }; - let sym = hit_symbol(h); - // Kind (container/function/block) lets the reader tell a type/def chunk - // from a free block at a glance. Omitted when the server didn't send one. - let kind = if h.kind.is_empty() { + let marker = write_marker(h); + let classification = if marker.is_empty() { String::new() } else { - format!(" ({})", h.kind) - }; - let stale_mark = if root.is_some() && h.path.as_deref().is_some_and(|p| stale.contains(p)) { - " ⚠ stale (edited since indexed)" - } else { - "" - }; - let codebase = if show_score { - h.codebase_id - .as_deref() - .map(|id| format!(" [codebase {id}]")) - .unwrap_or_default() - } else { - String::new() + format!(" {}", markdown_code(marker.trim())) }; writeln!( out, - "[{}{lang}]{codebase} {} {sym}{kind}{}{score}{stale_mark}", - h.domain_id, - hit_location(h, root), - write_marker(h) + "### {} — {}{classification}", + markdown_code(&hit_location(h, root)), + markdown_code(&hit_symbol(h)) ) .expect("writing to a String cannot fail"); - // Skip leading blank lines so the declaration/signature leads the snippet - // rather than whitespace. `--expand` shows the whole body; otherwise the - // first few lines, enough to judge relevance without flooding context. - let take = if full_body { usize::MAX } else { 4 }; - for line in h - .snippet - .lines() - .skip_while(|l| l.trim().is_empty()) - .take(take) - { - out.push_str(" "); - out.push_str(line); - out.push('\n'); + if root.is_some() && h.path.as_deref().is_some_and(|p| stale.contains(p)) { + // Put this warning before metadata so collapsed host views show it. + out.push_str("⚠ stale (edited since indexed)\n"); + } + render_hit_metadata(&mut out, h, show_score); + // The preview starts at the first nonblank line. Expanded bodies retain + // all source bytes, including leading blank lines and indentation. + let snippet = if full_body { + Cow::Borrowed(h.snippet.as_str()) + } else { + Cow::Owned( + h.snippet + .lines() + .skip_while(|line| line.trim().is_empty()) + .take(4) + .collect::>() + .join("\n"), + ) + }; + if !snippet.is_empty() { + let fence = "`".repeat(longest_backtick_run(&snippet).saturating_add(1).max(3)); + let language = fence_language(h.language.as_deref()); + write!(out, "\n{fence}{language}\n{snippet}").expect("writing to a String cannot fail"); + if !snippet.ends_with('\n') { + out.push('\n'); + } + writeln!(out, "{fence}").expect("writing to a String cannot fail"); } out.push('\n'); } out } +fn render_hit_metadata(out: &mut String, hit: &api::SearchHit, show_score: bool) { + write!(out, "Domain: {}", markdown_code(&hit.domain_id)) + .expect("writing to a String cannot fail"); + if let Some(language) = &hit.language { + write!(out, " · Language: {}", markdown_code(language)) + .expect("writing to a String cannot fail"); + } + if !hit.kind.is_empty() { + write!(out, " · Kind: {}", markdown_code(&hit.kind)) + .expect("writing to a String cannot fail"); + } + if show_score { + write!(out, " · Score: {:.3}", hit.score).expect("writing to a String cannot fail"); + } + if let Some(codebase) = &hit.codebase_id { + write!(out, " · Codebase: {}", markdown_code(codebase)) + .expect("writing to a String cannot fail"); + } + out.push('\n'); +} + +/// Keep untrusted values on one line and outside Markdown syntax. +fn markdown_code(value: &str) -> String { + if value.is_empty() { + return "(empty)".into(); + } + let mut text = String::new(); + for character in value.chars() { + if character.is_control() { + text.extend(character.escape_default()); + } else { + text.push(character); + } + } + let fence = "`".repeat(longest_backtick_run(&text).saturating_add(1)); + // Padding separates delimiter backticks and preserves surrounding spaces. + // CommonMark removes one padding space at each end of a code span. + let needs_padding = text.starts_with('`') + || text.ends_with('`') + || (text.starts_with(' ') && text.ends_with(' ') && !text.trim().is_empty()); + let padding = if needs_padding { " " } else { "" }; + format!("{fence}{padding}{text}{padding}{fence}") +} + +fn longest_backtick_run(value: &str) -> usize { + value + .split(|character| character != '`') + .map(str::len) + .max() + .unwrap_or(0) +} + +/// A fence language must be a single token with no Markdown delimiters. +fn fence_language(language: Option<&str>) -> String { + language + .filter(|value| { + value.chars().all(|character| { + character.is_ascii_alphanumeric() + || matches!(character, '_' | '-' | '+' | '#' | '.') + }) + }) + .map(str::to_ascii_lowercase) + .unwrap_or_default() +} + /// A hit scoring below this fraction of the top hit's score is sorted under the /// "weaker matches" separator. Relative (not absolute) because raw hybrid /// scores aren't calibrated across queries. @@ -450,8 +503,14 @@ mod tests { &stale, false, ); - let dirty = out.lines().find(|l| l.contains("dirty.rs")).unwrap(); - let clean = out.lines().find(|l| l.contains("clean.rs")).unwrap(); + let dirty = out + .split("\n### ") + .find(|hit| hit.contains("dirty.rs")) + .unwrap(); + let clean = out + .split("\n### ") + .find(|hit| hit.contains("clean.rs")) + .unwrap(); assert!(dirty.contains("⚠ stale"), "edited file flagged"); assert!(!clean.contains("⚠ stale"), "untouched file not flagged"); @@ -475,18 +534,17 @@ mod tests { &stale, false, ); - let lines: Vec<_> = output - .lines() - .filter(|line| line.starts_with('[')) - .collect(); - assert!(lines[0].contains("[codebase a]")); - assert!(lines[0].contains(&root.join("src/lib.rs").display().to_string())); - assert!(lines[0].contains("stale")); - assert!(lines[1].contains("[codebase b] src/lib.rs")); - assert!(!lines[1].contains("checkout-a")); - assert!(!lines[1].contains("stale")); - assert!(!lines[2].contains("checkout-a")); - assert!(!lines[2].contains("stale")); + let sections: Vec<_> = output.split("\n### ").skip(1).collect(); + assert!(sections[0].contains("Codebase: `a`")); + assert!(sections[0].contains(&root.join("src/lib.rs").display().to_string())); + assert!(sections[0].contains("stale")); + assert!(sections[1].starts_with("`src/lib.rs:1-2`")); + assert!(sections[1].contains("Codebase: `b`")); + assert!(!sections[1].contains("checkout-a")); + assert!(!sections[1].contains("stale")); + assert!(!sections[2].contains("checkout-a")); + assert!(!sections[2].contains("stale")); + assert!(!sections[2].contains("Codebase:")); } #[test] @@ -507,6 +565,9 @@ mod tests { let mut write = node("dispatch", "run.rs"); write.enclosing_symbol = Some("reset".into()); write.is_write = Some(true); + write.reference_namespace = Some("Value".into()); + write.reference_kind = Some("assignment".into()); + write.qualified_symbol = Some("crate::dispatch".into()); let out = render_hits_inner( &[read, write], @@ -519,9 +580,179 @@ mod tests { let lines: Vec<&str> = out.lines().filter(|l| l.contains("run.rs")).collect(); assert!(lines[0].contains("dispatch in run_until"), "{out}"); - assert!(!lines[0].contains("(write)"), "a read isn't marked: {out}"); + assert!(lines[0].contains("(read)"), "{out}"); assert!(lines[1].contains("dispatch in reset"), "{out}"); - assert!(lines[1].contains("(write)"), "{out}"); + assert!( + lines[1].contains("(write; value; assignment; -> crate::dispatch)"), + "{out}" + ); + assert!( + !out.contains("Score:"), + "graph hits have no relevance score: {out}" + ); + } + + #[test] + fn render_count_location_and_stale_warning_lead_the_output() { + let hits = [node("first", "dirty.rs"), node("second", "clean.rs")]; + let output = render_hits_inner( + &hits, + "none", + HitContext::local(Some(std::path::Path::new("/repo"))), + true, + &HashSet::from(["dirty.rs".to_string()]), + false, + ); + let lines: Vec<_> = output.lines().collect(); + assert_eq!(lines[0], "**2 matches**"); + assert_eq!(lines[1], ""); + assert!(lines[2].starts_with("### `"), "{output}"); + assert!(lines[2].contains("dirty.rs:1-2` — `first`"), "{output}"); + assert_eq!(lines[3], "⚠ stale (edited since indexed)"); + assert_eq!( + lines[4], + "Domain: `code` · Language: `rust` · Kind: `function` · Score: 0.000" + ); + } + + #[test] + fn render_fences_preserve_snippet_markdown_and_preview_line_selection() { + let mut hit = node("snippet", "source.rs"); + hit.snippet = + "\n \nfn example() {\n ```rust\n ~~~\n **text** \n omitted\n}" + .into(); + let output = render_hits_inner( + &[hit], + "none", + HitContext::local(None), + false, + &HashSet::new(), + false, + ); + assert!(output.starts_with("**1 match**\n"), "{output}"); + assert!( + output.contains( + "\n````rust\nfn example() {\n ```rust\n ~~~\n **text** \n````\n" + ), + "{output}" + ); + assert!(!output.contains("omitted"), "{output}"); + } + + #[test] + fn render_expanded_body_preserves_all_source_bytes() { + let mut hit = node("expanded", "source.rs"); + let source = "\r\n fn example() {\r\n one\r\n two\r\n three\r\n four\r\n `````\r\n }\r\n"; + hit.snippet = source.into(); + let output = render_hits_inner( + &[hit], + "none", + HitContext::local(None), + false, + &HashSet::new(), + true, + ); + assert!( + output.contains(&format!("\n``````rust\n{source}``````\n")), + "{output}" + ); + } + + #[test] + fn render_untrusted_values_cannot_open_markdown_blocks() { + let mut hit = node("`symbol`\n