From f445f11674a033764389c2c141546363ac198444 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Mon, 27 Jul 2026 12:16:09 +1000 Subject: [PATCH 1/3] feat(pikchr): inline grammar in diagram sub-session prompt, drop parent warnings, double timeout MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Diagram sub-session improvements for generate_pikchr: - Inline the full bundled Pikchr grammar text into the specialist's prompt instead of referencing the grammar file by path — the sub-session has no repo access, so a path was a dead reference. When the bundled grammar is missing or unreadable, the prompt falls back to naming the public grammar URL. Remote blox note sessions are unaffected and keep referencing the uploaded grammar file. - Stop surfacing accepted-render layout warnings to the parent session: the warnings text content and the renderWarnings structured field are gone from the tool result, and the now-unused warnings plumbing is removed from GenOutcome and PreviewOutcome. - Double the generate_pikchr wall-clock timeout from 10 to 20 minutes. Co-Authored-By: Claude Fable 5 Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/pikchr_mcp.rs | 117 ++++-------------- .../staged/src-tauri/src/pikchr_subsession.rs | 87 ++++++------- apps/staged/src-tauri/src/session_commands.rs | 11 ++ 3 files changed, 74 insertions(+), 141 deletions(-) diff --git a/apps/staged/src-tauri/src/pikchr_mcp.rs b/apps/staged/src-tauri/src/pikchr_mcp.rs index 41d4c915..be049377 100644 --- a/apps/staged/src-tauri/src/pikchr_mcp.rs +++ b/apps/staged/src-tauri/src/pikchr_mcp.rs @@ -59,7 +59,7 @@ use crate::store::{AcpMessageMetadata, CompletionReason, Session, SessionStatus, /// Wall-clock cap for one `generate_pikchr` call. Each call spins a provider /// subprocess and runs several turns; the cap keeps a stuck sub-agent from /// running indefinitely. Enforced by cancelling the sub-session's token. -const GENERATE_PIKCHR_TIMEOUT: Duration = Duration::from_secs(600); +const GENERATE_PIKCHR_TIMEOUT: Duration = Duration::from_secs(1200); /// Cap the rasterized PNG so a runaway diagram can't allocate a huge pixmap. const MAX_RENDER_DIMENSION: u32 = 4096; @@ -655,17 +655,13 @@ fn rasterize_tree_to_png(tree: &usvg::Tree, scale: f32) -> Option> { // ============================================================================= /// Outcome of rendering a candidate diagram: the PNG (if rasterization -/// succeeded), a text summary of dimensions and layout warnings, whether the -/// source failed to render at all, and the warning report on its own when -/// renderable geometry still overlaps or extends beyond the diagram bounds. +/// succeeded), a text summary of dimensions and layout warnings, and whether +/// the source failed to render at all. Layout warnings live inside `summary` +/// for the specialist to weigh; they are not carried past acceptance. pub(crate) struct PreviewOutcome { pub(crate) png: Option>, pub(crate) summary: String, pub(crate) is_error: bool, - /// The layout-warning portion of `summary`, when any warnings were - /// detected — carried separately so an accepted render's warnings can be - /// surfaced to the calling agent on their own. - pub(crate) warnings: Option, } /// Render + analyze a candidate Pikchr source. @@ -677,7 +673,6 @@ pub(crate) fn run_preview(source: &str, scale: f32) -> PreviewOutcome { png: None, summary: "Pikchr source is empty — nothing to render.".to_string(), is_error: true, - warnings: None, }; } let rendered = match render_pikchr_svg(source) { @@ -687,7 +682,6 @@ pub(crate) fn run_preview(source: &str, scale: f32) -> PreviewOutcome { png: None, summary: format!("Pikchr could not render this diagram:\n{}", err.trim()), is_error: true, - warnings: None, }; } }; @@ -705,7 +699,6 @@ the rendered SVG could not be parsed.)", rendered.width, rendered.height ), is_error: false, - warnings: None, }; }; @@ -722,7 +715,6 @@ the rendered SVG could not be parsed.)", png, summary, is_error: false, - warnings, } } @@ -757,7 +749,7 @@ struct PikchrToolsHandler { /// child-session announcements are written into it so the UI can link to /// the diagram session while the specialist is still running. parent_session_id: String, - /// Handle used to resolve the bundled Pikchr grammar reference for the + /// Handle used to load the bundled Pikchr grammar text inlined into the /// sub-agent's prompt. app_handle: tauri::AppHandle, /// Shared app store used to persist the child diagram session. @@ -796,9 +788,8 @@ An internal Pikchr specialist writes the diagram, then renders and visually revi session, iterating until it is satisfied. Prefer this over hand-writing Pikchr. Pass a fine-grained \ `description` (boxes, arrows, labels, layout, relationships). To revise an existing diagram, also \ pass its current source as `previous_pikchr` so it is edited rather than redrawn. Returns the \ -validated Pikchr source (drop it into a ```pikchr fenced code block), a filesystem path to a \ -rendered PNG preview you may open as an optional final check, and any layout warnings the \ -specialist deliberately accepted." +validated Pikchr source (drop it into a ```pikchr fenced code block) and a filesystem path to a \ +rendered PNG preview you may open as an optional final check." )] async fn generate_pikchr( &self, @@ -811,10 +802,13 @@ specialist deliberately accepted." let inner_session_id = session.id.clone(); announce_pikchr_child_session(&self.store, &self.parent_session_id, &inner_session_id); let store = Arc::clone(&self.store); - // The sub-session always runs locally, so resolve a local grammar path - // (workspace_name = None). - let grammar_reference = - crate::session_commands::resolve_pikchr_grammar_reference(&self.app_handle, None); + // The full grammar text is inlined into the sub-agent's prompt rather + // than referenced by file path (the sub-session has no repo access). + // Remote note sessions keep referencing an uploaded grammar file — + // that path is resolved separately in `session_commands`. `None` + // (bundled grammar missing or unreadable) makes the prompt fall back + // to naming the public grammar URL. + let grammar = crate::session_commands::bundled_pikchr_grammar_text(&self.app_handle); // Cancellation token owned by *this* future (the parent MCP request). // The worker gets a clone; the parent keeps the token alive through a @@ -918,7 +912,7 @@ accepted a render, so the diagram run was cancelled.", &driver, worker_store, &worker_session_id, - &grammar_reference, + grammar.as_deref(), &p.description, p.previous_pikchr.as_deref(), &slot, @@ -959,7 +953,6 @@ accepted a render, so the diagram run was cancelled.", &inner_session_id, preview_image_path.as_deref(), &outcome.source, - outcome.warnings.as_deref(), )) } } @@ -1036,7 +1029,6 @@ fn build_generate_pikchr_result( inner_session_id: &str, preview_image_path: Option<&str>, source: &str, - warnings: Option<&str>, ) -> CallToolResult { let mut content = Vec::new(); if let Some(path) = preview_image_path { @@ -1044,13 +1036,6 @@ fn build_generate_pikchr_result( "Rendered preview image path: {path}" ))); } - // The specialist saw these warnings in its render results and accepted - // anyway, so they're deliberate — surface them rather than swallow them. - if let Some(warnings) = warnings { - content.push(Content::text(format!( - "The specialist accepted this render despite layout warnings:\n{warnings}" - ))); - } content.push(Content::text(source.to_string())); let mut structured = serde_json::Map::new(); @@ -1064,12 +1049,6 @@ fn build_generate_pikchr_result( serde_json::Value::String(path.to_string()), ); } - if let Some(warnings) = warnings { - structured.insert( - "renderWarnings".to_string(), - serde_json::Value::String(warnings.to_string()), - ); - } structured.insert( "source".to_string(), serde_json::Value::String(source.to_string()), @@ -1098,8 +1077,8 @@ impl ServerHandler for PikchrToolsHandler { /// string is tolerated — the server still starts and `generate_pikchr` then /// fails per-call rather than failing session startup). `parent_session_id` is /// the session this server is attached to; child diagram sessions are -/// announced into its transcript as they start. `app_handle` resolves the -/// bundled Pikchr grammar reference for the sub-agent. `registry` holds each +/// announced into its transcript as they start. `app_handle` loads the +/// bundled Pikchr grammar text for the sub-agent. `registry` holds each /// child diagram session while it runs so a user Stop reaches its worker. pub async fn start_pikchr_mcp_server( provider_id: String, @@ -1223,7 +1202,6 @@ now would accept that earlier version, not this source." self.slot.store(GenOutcome { source: p.pikchr, png: preview.png, - warnings: preview.warnings, }); Ok(CallToolResult::success(content)) @@ -1410,7 +1388,7 @@ arrow from COLL.e to SNOW.w"#; &driver, Arc::clone(&store), &session.id, - "/tmp/grammar.md", + Some("test grammar body"), "a friendly box", None, &slot, @@ -1472,7 +1450,6 @@ arrow from COLL.e to SNOW.w"#; "child-session-1", Some("/tmp/staged-pikchr-preview.png"), "box \"Clean\" fit", - None, ); let texts: Vec = result @@ -1498,45 +1475,6 @@ arrow from COLL.e to SNOW.w"#; "/tmp/staged-pikchr-preview.png" ); assert_eq!(structured["source"], "box \"Clean\" fit"); - assert!( - structured.get("renderWarnings").is_none(), - "no warnings field for a clean render" - ); - } - - #[test] - fn generate_pikchr_result_surfaces_accepted_warnings() { - let result = build_generate_pikchr_result( - "child-session-1", - None, - "box \"Busy\" fit", - Some("⚠ 1 overlapping pair(s) detected"), - ); - - let texts: Vec = result - .content - .iter() - .filter_map(|content| content.as_text().map(|text| text.text.clone())) - .collect(); - assert_eq!( - texts, - vec![ - "The specialist accepted this render despite layout warnings:\n\ -⚠ 1 overlapping pair(s) detected" - .to_string(), - "box \"Busy\" fit".to_string(), - ] - ); - - let structured = result - .structured_content - .as_ref() - .expect("structured content"); - assert_eq!( - structured["renderWarnings"], - "⚠ 1 overlapping pair(s) detected" - ); - assert_eq!(structured["source"], "box \"Busy\" fit"); } /// Render `source` and parse it into a usvg tree the way `run_preview` does, @@ -1689,7 +1627,7 @@ arrow from COLL.e to SNOW.w"#; fn valid_source_produces_png_and_dimensions() { let outcome = run_preview("box \"hello\"", DEFAULT_SCALE); assert!(!outcome.is_error); - assert!(outcome.warnings.is_none()); + assert!(outcome.summary.contains("No layout issues detected")); assert!(outcome.png.is_some(), "expected a PNG for valid source"); assert!(!outcome.png.unwrap().is_empty()); assert!(outcome.summary.contains("px")); @@ -1699,13 +1637,8 @@ arrow from COLL.e to SNOW.w"#; fn overlapping_source_reports_warnings() { let outcome = run_preview(OVERLAPPING_SOURCE, DEFAULT_SCALE); assert!(!outcome.is_error); + assert!(outcome.summary.contains('⚠')); assert!(outcome.summary.contains("overlapping pair")); - let warnings = outcome.warnings.expect("overlaps produce warnings"); - assert!(warnings.contains("overlapping pair")); - assert!( - outcome.summary.contains(&warnings), - "the summary embeds the warning report" - ); } #[test] @@ -1715,9 +1648,8 @@ arrow from COLL.e to SNOW.w"#; // the diagram edges on every host. let outcome = run_preview("margin = -0.2in\nbox \"Out\"", DEFAULT_SCALE); assert!(!outcome.is_error); + assert!(outcome.summary.contains('⚠')); assert!(outcome.summary.contains("beyond the diagram bounds")); - let warnings = outcome.warnings.expect("out-of-bounds produces warnings"); - assert!(warnings.contains("beyond the diagram bounds")); } #[test] @@ -1740,7 +1672,6 @@ arrow from COLL.e to SNOW.w"#; fn malformed_source_reports_error() { let outcome = run_preview("box \"unterminated", DEFAULT_SCALE); assert!(outcome.is_error); - assert!(outcome.warnings.is_none()); assert!(outcome.png.is_none()); assert!(outcome.summary.to_lowercase().contains("pikchr")); } @@ -1749,7 +1680,6 @@ arrow from COLL.e to SNOW.w"#; fn empty_source_reports_error() { let outcome = run_preview(" \n ", DEFAULT_SCALE); assert!(outcome.is_error); - assert!(outcome.warnings.is_none()); assert!(outcome.png.is_none()); } @@ -1879,7 +1809,6 @@ arrow from COLL.e to SNOW.w"#; let stored = slot.take().expect("slot holds the render"); assert_eq!(stored.source, "box \"hello\""); assert!(stored.png.is_some()); - assert!(stored.warnings.is_none()); } #[tokio::test] @@ -1924,7 +1853,7 @@ arrow from COLL.e to SNOW.w"#; } #[tokio::test] - async fn render_tool_stores_warnings_for_overlapping_source() { + async fn render_tool_stores_overlapping_render_despite_warnings() { let slot = Arc::new(LastRenderSlot::new()); let handler = PikchrPreviewHandler::new(DEFAULT_SCALE, Arc::clone(&slot)); @@ -1937,7 +1866,5 @@ arrow from COLL.e to SNOW.w"#; let stored = slot.take().expect("slot holds the flagged render"); assert_eq!(stored.source, OVERLAPPING_SOURCE); - let warnings = stored.warnings.expect("warnings recorded on the render"); - assert!(warnings.contains("overlapping pair")); } } diff --git a/apps/staged/src-tauri/src/pikchr_subsession.rs b/apps/staged/src-tauri/src/pikchr_subsession.rs index 2b648f74..2fb55618 100644 --- a/apps/staged/src-tauri/src/pikchr_subsession.rs +++ b/apps/staged/src-tauri/src/pikchr_subsession.rs @@ -22,6 +22,7 @@ use std::sync::{Arc, Mutex}; use tokio_util::sync::CancellationToken; use crate::agent::{AgentDriver, MessageWriter}; +use crate::session_commands::PIKCHR_GRAMMAR_URL; use crate::store::{CompletionReason, MessageRole, SessionStatus, Store}; /// Total sub-agent turns before giving up. The specialist iterates on the @@ -85,11 +86,6 @@ pub(crate) struct GenOutcome { pub(crate) source: String, /// Rendered PNG preview, if rasterization succeeded. pub(crate) png: Option>, - /// Layout warnings on the accepted render (overlaps, out-of-bounds - /// elements). The specialist saw these in the tool result and accepted - /// anyway, so they're deliberate — surfaced to the caller rather than - /// swallowed. - pub(crate) warnings: Option, } /// The last *successful* render produced through the specialist's @@ -128,7 +124,7 @@ pub(crate) async fn generate_pikchr_source( driver: &D, store: Arc, session_id: &str, - grammar_reference: &str, + grammar: Option<&str>, description: &str, previous_pikchr: Option<&str>, slot: &LastRenderSlot, @@ -139,7 +135,7 @@ pub(crate) async fn generate_pikchr_source( driver, Arc::clone(&store), session_id, - grammar_reference, + grammar, description, previous_pikchr, slot, @@ -228,17 +224,17 @@ async fn generate_pikchr_source_inner( driver: &D, store: Arc, session_id: &str, - grammar_reference: &str, + grammar: Option<&str>, description: &str, previous_pikchr: Option<&str>, slot: &LastRenderSlot, cancel_token: &CancellationToken, ) -> Result { - // The sub-agent needs no repo access; the grammar path is absolute. + // The sub-agent needs no repo access; the grammar is inlined in the prompt. let working_dir = std::env::temp_dir(); let store_dyn: Arc = store.clone(); - let mut prompt = initial_prompt(grammar_reference, description, previous_pikchr); + let mut prompt = initial_prompt(grammar, description, previous_pikchr); let mut agent_session_id: Option = None; for _ in 0..MAX_ATTEMPTS { @@ -355,10 +351,25 @@ fn reply_accepts_last_render(reply: &str) -> bool { // Prompt templates // ============================================================================= -fn initial_prompt(reference: &str, description: &str, previous_pikchr: Option<&str>) -> String { +/// `grammar` is the full grammar text to inline into the message — the +/// sub-session has no repo access, so a file path would be a dead reference. +/// `None` (bundled grammar missing or unreadable) falls back to naming the +/// public grammar URL instead. +fn initial_prompt( + grammar: Option<&str>, + description: &str, + previous_pikchr: Option<&str>, +) -> String { + let grammar_line = match grammar { + Some(_) => { + "Consult the full Pikchr grammar reference included below for exact syntax.".to_string() + } + None => format!( + "The Pikchr grammar reference is at {PIKCHR_GRAMMAR_URL}; consult it for exact syntax." + ), + }; let mut prompt = format!( - "You are a Pikchr diagram specialist. The Pikchr grammar reference is at `{reference}`; \ -consult it for exact syntax.\n\ + "You are a Pikchr diagram specialist. {grammar_line}\n\ \n\ Workflow: draft the diagram source, then call the `render_pikchr` tool with it (no code fences). \ Inspect the returned image and layout analysis, then revise and render again until the diagram \ @@ -367,6 +378,12 @@ matches the request. When you are satisfied, accept the render: your whole messa `{ACCEPT_SENTINEL}` as its own line. The accepted diagram is your last successful render — the \ rest of your reply text is ignored." ); + if let Some(grammar) = grammar { + prompt.push_str(&format!( + "\n\nPikchr grammar reference:\n\n{}", + grammar.trim_end() + )); + } if let Some(previous) = previous_pikchr { prompt.push_str(&format!( "\n\nHere is the current diagram to modify:\n```pikchr\n{previous}\n```\n\ @@ -423,7 +440,6 @@ mod tests { GenOutcome { source: source.to_string(), png: Some(vec![1, 2, 3]), - warnings: None, } } @@ -537,7 +553,7 @@ mod tests { driver, Arc::clone(store), session_id, - "/tmp/grammar.md", + Some("test grammar body"), "a friendly box", None, slot, @@ -568,7 +584,6 @@ mod tests { assert_eq!(outcome.source, CLEAN_SOURCE); assert!(outcome.png.is_some()); - assert!(outcome.warnings.is_none()); assert_eq!(*driver.calls.lock().unwrap(), 1); assert!(slot.is_empty(), "acceptance takes the slot"); @@ -681,33 +696,6 @@ mod tests { assert_eq!(*driver.calls.lock().unwrap(), 2); } - #[tokio::test] - async fn accepted_render_keeps_its_warnings() { - let slot = Arc::new(LastRenderSlot::new()); - let mut accepted = render(CLEAN_SOURCE); - accepted.warnings = Some("⚠ 1 overlapping pair(s) detected".to_string()); - let driver = FakeDriver::new( - Arc::clone(&slot), - vec![turn(Some(accepted), ACCEPT_SENTINEL)], - ); - let (store, session_id) = child_session(); - - let outcome = run_generation( - &driver, - &store, - &session_id, - &slot, - &CancellationToken::new(), - ) - .await - .expect("warnings don't block acceptance"); - - assert_eq!( - outcome.warnings.as_deref(), - Some("⚠ 1 overlapping pair(s) detected") - ); - } - #[tokio::test] async fn reprompts_when_accepting_before_any_render() { let slot = Arc::new(LastRenderSlot::new()); @@ -974,8 +962,8 @@ mod tests { #[test] fn initial_prompt_embeds_previous_source_when_revising() { - let prompt = initial_prompt("/tmp/grammar.md", "add a box", Some("box \"old\"")); - assert!(prompt.contains("/tmp/grammar.md")); + let prompt = initial_prompt(Some("GRAMMAR BODY"), "add a box", Some("box \"old\"")); + assert!(prompt.contains("Pikchr grammar reference:\n\nGRAMMAR BODY")); assert!(prompt.contains("render_pikchr")); assert!(prompt.contains(ACCEPT_SENTINEL)); assert!(prompt.contains("current diagram to modify")); @@ -985,10 +973,17 @@ mod tests { #[test] fn initial_prompt_omits_revision_block_for_fresh_diagram() { - let prompt = initial_prompt("/tmp/grammar.md", "a fresh box", None); + let prompt = initial_prompt(Some("GRAMMAR BODY"), "a fresh box", None); assert!(!prompt.contains("current diagram to modify")); assert!(prompt.contains("render_pikchr")); assert!(prompt.contains(ACCEPT_SENTINEL)); assert!(prompt.contains("a fresh box")); } + + #[test] + fn initial_prompt_falls_back_to_grammar_url_without_bundled_grammar() { + let prompt = initial_prompt(None, "a fresh box", None); + assert!(prompt.contains(PIKCHR_GRAMMAR_URL)); + assert!(!prompt.contains("Pikchr grammar reference:\n")); + } } diff --git a/apps/staged/src-tauri/src/session_commands.rs b/apps/staged/src-tauri/src/session_commands.rs index 811a0c1f..dfdcb05b 100644 --- a/apps/staged/src-tauri/src/session_commands.rs +++ b/apps/staged/src-tauri/src/session_commands.rs @@ -132,6 +132,17 @@ fn bundled_pikchr_grammar_bytes(app_handle: &tauri::AppHandle) -> Option } } +/// Full text of the bundled Pikchr grammar, for prompts that inline the +/// grammar rather than referencing it by path (the local `generate_pikchr` +/// sub-session). Remote note sessions keep referencing an uploaded grammar +/// file instead — see [`resolve_pikchr_grammar_reference`]. `None` when the +/// bundled resource is missing or unreadable; callers fall back to pointing +/// at [`PIKCHR_GRAMMAR_URL`]. +pub(crate) fn bundled_pikchr_grammar_text(app_handle: &tauri::AppHandle) -> Option { + bundled_pikchr_grammar_bytes(app_handle) + .map(|bytes| String::from_utf8_lossy(&bytes).into_owned()) +} + fn generated_pikchr_grammar_remote_path() -> String { format!( "{PIKCHR_GRAMMAR_REMOTE_PATH_PREFIX}{}{PIKCHR_GRAMMAR_REMOTE_PATH_SUFFIX}", From 28c65618430b34a31adcec5841996546e8d3ea1c Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Mon, 27 Jul 2026 14:43:26 +1000 Subject: [PATCH 2/3] feat(pikchr): send MCP progress keep-alives during generate_pikchr MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A generate_pikchr call was one silent MCP request for the entire specialist run, and Claude Code aborts any MCP tool call that produces no response or progress notification for 300 s — well under a long diagram run. The abort never reached the server-side worker, which kept going, accepted a render, and completed the child session with nobody left waiting: every specialist run past ~5 minutes failed in the parent while the diagram session finished cleanly. Claude Code's idle timer explicitly resets on progress, so the handler now takes its rmcp RequestContext and, while awaiting the worker, ticks a progress notification back to the caller every 30 s, reporting elapsed seconds (monotonic, no total) with a short liveness message. The keep-alive loop runs under select! against the worker's oneshot, so it stops the moment the worker reports or the call future is dropped. When the caller sends no progress token, the loop pends forever instead — the MCP spec only allows progress against a caller-provided token — and the call behaves exactly as before. Co-Authored-By: Claude Fable 5 Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/pikchr_mcp.rs | 92 +++++++++++++++++++++++-- 1 file changed, 88 insertions(+), 4 deletions(-) diff --git a/apps/staged/src-tauri/src/pikchr_mcp.rs b/apps/staged/src-tauri/src/pikchr_mcp.rs index be049377..47236c63 100644 --- a/apps/staged/src-tauri/src/pikchr_mcp.rs +++ b/apps/staged/src-tauri/src/pikchr_mcp.rs @@ -17,6 +17,10 @@ //! The specialist accepts by ending its turn with the //! [`crate::pikchr_subsession::ACCEPT_SENTINEL`] token, and the host returns //! the slot's contents — so unvalidated source can never reach the caller. +//! While the specialist runs, `generate_pikchr` ticks MCP progress +//! notifications back to its caller (when the request carries a progress +//! token) so client-side idle timers don't abort a call whose run outlasts +//! them. //! //! Fidelity: rendering goes through the `pikchr` crate, which bundles the same //! official `pikchr.c` that the frontend's `pikchr-js` compiles to WASM. The @@ -45,11 +49,15 @@ use acp_client::{McpServer, McpServerHttp}; use axum::Router; use base64::Engine as _; use rmcp::handler::server::{router::tool::ToolRouter, wrapper::Parameters}; -use rmcp::model::{CallToolResult, Content, ServerCapabilities, ServerInfo}; +use rmcp::model::{ + CallToolResult, Content, ProgressNotificationParam, ProgressToken, ServerCapabilities, + ServerInfo, +}; +use rmcp::service::RequestContext; use rmcp::transport::streamable_http_server::{ session::local::LocalSessionManager, StreamableHttpServerConfig, StreamableHttpService, }; -use rmcp::{schemars, tool, tool_handler, tool_router, ErrorData, ServerHandler}; +use rmcp::{schemars, tool, tool_handler, tool_router, ErrorData, Peer, RoleServer, ServerHandler}; use crate::agent::AcpDriver; use crate::pikchr_subsession::{CancelReason, GenOutcome, LastRenderSlot, ACCEPT_SENTINEL}; @@ -61,6 +69,15 @@ use crate::store::{AcpMessageMetadata, CompletionReason, Session, SessionStatus, /// running indefinitely. Enforced by cancelling the sub-session's token. const GENERATE_PIKCHR_TIMEOUT: Duration = Duration::from_secs(1200); +/// Interval between MCP progress keep-alives sent to the caller while +/// `generate_pikchr` waits on its specialist run. Without them the whole run +/// is one silent request, and MCP clients cut those off long before +/// [`GENERATE_PIKCHR_TIMEOUT`]: Claude Code aborts any tool call that produces +/// no response or progress notification for 300 s, orphaning a worker that +/// then finishes into the void. 30 s keeps a generous margin under that (and +/// any comparable client-side idle timer) at negligible cost. +const PROGRESS_KEEPALIVE_INTERVAL: Duration = Duration::from_secs(30); + /// Cap the rasterized PNG so a runaway diagram can't allocate a huge pixmap. const MAX_RENDER_DIMENSION: u32 = 4096; /// Default rasterization scale — 2× keeps labels legible. @@ -794,6 +811,7 @@ rendered PNG preview you may open as an optional final check." async fn generate_pikchr( &self, Parameters(p): Parameters, + ctx: RequestContext, ) -> Result { let scale = p.scale.unwrap_or(DEFAULT_SCALE); let provider_id = self.provider_id.clone(); @@ -924,8 +942,17 @@ accepted a render, so the diagram run was cancelled.", let _ = tx.send(result); }); - let outcome = rx - .await + // Await the worker while ticking progress keep-alives back to the + // caller so its idle timer doesn't sever a long run. The keep-alive + // loop never completes; the select ends when the worker reports (or + // this future is dropped, which also stops the keep-alives). + let received = tokio::select! { + received = rx => received, + _ = send_progress_keepalives(&ctx.peer, ctx.meta.get_progress_token()) => { + unreachable!("the progress keep-alive loop never completes") + } + }; + let outcome = received .map_err(|e| { let message = format!("generate_pikchr worker dropped: {e}"); mark_pikchr_child_session_error(&store, &inner_session_id, &message); @@ -957,6 +984,49 @@ accepted a render, so the diagram run was cancelled.", } } +/// Build the progress keep-alive notification sent `elapsed_secs` into a +/// `generate_pikchr` run. Progress reports elapsed seconds with no total: +/// monotonically increasing, as the spec asks of an unbounded operation. +fn progress_keepalive( + progress_token: ProgressToken, + elapsed_secs: u64, +) -> ProgressNotificationParam { + ProgressNotificationParam { + progress_token, + progress: elapsed_secs as f64, + total: None, + message: Some(format!( + "Diagram specialist still working ({elapsed_secs}s elapsed)." + )), + } +} + +/// Tick an MCP progress notification to the caller every +/// [`PROGRESS_KEEPALIVE_INTERVAL`] for as long as this future is polled. +/// Never completes — run it under `select!` against the awaited work so it +/// stops when the work does. Progress notifications may only reference a +/// token the caller provided, so when the request carries none this pends +/// forever (rather than returning, which the caller treats as unreachable) +/// and the call proceeds without keep-alives. +async fn send_progress_keepalives(peer: &Peer, progress_token: Option) { + let Some(progress_token) = progress_token else { + return std::future::pending().await; + }; + let started = tokio::time::Instant::now(); + loop { + tokio::time::sleep(PROGRESS_KEEPALIVE_INTERVAL).await; + let elapsed_secs = started.elapsed().as_secs(); + if let Err(e) = peer + .notify_progress(progress_keepalive(progress_token.clone(), elapsed_secs)) + .await + { + // A failed keep-alive usually means the caller is gone; the + // worker result (or this future being dropped) settles the call. + log::debug!("[pikchr_mcp] failed to send generate_pikchr progress keep-alive: {e}"); + } + } +} + /// Cancel reason recorded when the user stops the child diagram session from /// its own UI, distinguishing a deliberate stop from caller abandonment on /// both the cancelled session row and the parent tool error. @@ -1314,6 +1384,20 @@ arrow from COLL.e to SNOW.w"#; assert_eq!(persisted.provider.as_deref(), Some("fake-agent")); } + #[test] + fn progress_keepalive_reports_elapsed_seconds_with_no_total() { + let token = ProgressToken(rmcp::model::NumberOrString::Number(7)); + + let notification = progress_keepalive(token.clone(), 90); + + assert_eq!(notification.progress_token, token); + // Elapsed seconds as the progress value keeps successive keep-alives + // monotonically increasing, and an unbounded run reports no total. + assert_eq!(notification.progress, 90.0); + assert_eq!(notification.total, None); + assert!(notification.message.expect("message").contains("90s")); + } + #[tokio::test] async fn forward_user_cancel_records_reason_and_arms_worker_token() { let user_cancel = CancellationToken::new(); From 03e1d6a6706882f3202a47375b007d65311d9586 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Mon, 27 Jul 2026 15:54:44 +1000 Subject: [PATCH 3/3] feat(pikchr): collapse inlined grammar into an expandable context card MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since the Pikchr grammar was inlined into the diagram sub-session's first user message, opening a diagram session in the UI showed ~10.5 KB of grammar text as a wall in the first prompt bubble. Give the grammar the same treatment as the existing injected-context blocks (action, branch-history, launch-context): the backend now wraps it in a tag, and the session UI renders it as a collapsed, expandable card above the prompt text. - pikchr_subsession.rs: initial_prompt wraps the appended grammar in … instead of the plain "Pikchr grammar reference:" heading, and grammar_line points the specialist at the tagged block. The URL fallback (bundled grammar missing) is unchanged. Prompt tests updated. - sessionModalHelpers.ts: the tag whitelist previously lived in three hand-maintained regexes across two files; export a single XML_BLOCK_TAGS list (now including pikchr-grammar) and derive the block/open-tag patterns from it, so previews, hints, and titles keep stripping the new block. - SessionChatPane.svelte: derive parseContentSegments' tagPattern from XML_BLOCK_TAGS and map the new tag to a "Pikchr grammar" card with the BookOpen icon. Co-Authored-By: Claude Fable 5 Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/pikchr_subsession.rs | 9 +++++---- .../lib/features/sessions/SessionChatPane.svelte | 11 +++++++---- .../lib/features/sessions/sessionModalHelpers.ts | 13 ++++++++++--- 3 files changed, 22 insertions(+), 11 deletions(-) diff --git a/apps/staged/src-tauri/src/pikchr_subsession.rs b/apps/staged/src-tauri/src/pikchr_subsession.rs index 2fb55618..ca02180b 100644 --- a/apps/staged/src-tauri/src/pikchr_subsession.rs +++ b/apps/staged/src-tauri/src/pikchr_subsession.rs @@ -362,7 +362,8 @@ fn initial_prompt( ) -> String { let grammar_line = match grammar { Some(_) => { - "Consult the full Pikchr grammar reference included below for exact syntax.".to_string() + "Consult the full Pikchr grammar reference included in the `` block below for exact syntax." + .to_string() } None => format!( "The Pikchr grammar reference is at {PIKCHR_GRAMMAR_URL}; consult it for exact syntax." @@ -380,7 +381,7 @@ rest of your reply text is ignored." ); if let Some(grammar) = grammar { prompt.push_str(&format!( - "\n\nPikchr grammar reference:\n\n{}", + "\n\n\n{}\n", grammar.trim_end() )); } @@ -963,7 +964,7 @@ mod tests { #[test] fn initial_prompt_embeds_previous_source_when_revising() { let prompt = initial_prompt(Some("GRAMMAR BODY"), "add a box", Some("box \"old\"")); - assert!(prompt.contains("Pikchr grammar reference:\n\nGRAMMAR BODY")); + assert!(prompt.contains("\nGRAMMAR BODY\n")); assert!(prompt.contains("render_pikchr")); assert!(prompt.contains(ACCEPT_SENTINEL)); assert!(prompt.contains("current diagram to modify")); @@ -984,6 +985,6 @@ mod tests { fn initial_prompt_falls_back_to_grammar_url_without_bundled_grammar() { let prompt = initial_prompt(None, "a fresh box", None); assert!(prompt.contains(PIKCHR_GRAMMAR_URL)); - assert!(!prompt.contains("Pikchr grammar reference:\n")); + assert!(!prompt.contains("")); } } diff --git a/apps/staged/src/lib/features/sessions/SessionChatPane.svelte b/apps/staged/src/lib/features/sessions/SessionChatPane.svelte index 9fd2b4ae..71c8585a 100644 --- a/apps/staged/src/lib/features/sessions/SessionChatPane.svelte +++ b/apps/staged/src/lib/features/sessions/SessionChatPane.svelte @@ -44,6 +44,7 @@ import ChevronDown from '@lucide/svelte/icons/chevron-down'; import Zap from '@lucide/svelte/icons/zap'; import GitBranch from '@lucide/svelte/icons/git-branch'; + import BookOpen from '@lucide/svelte/icons/book-open'; import FileText from '@lucide/svelte/icons/file-text'; import ExternalLink from '@lucide/svelte/icons/external-link'; import ImagePlus from '@lucide/svelte/icons/image-plus'; @@ -103,7 +104,7 @@ isMaybeTextFile, insertFilePathsAtCursor, } from '../branches/branchCardHelpers'; - import { hasXmlBlocks, sessionEndMessage } from './sessionModalHelpers'; + import { hasXmlBlocks, sessionEndMessage, XML_BLOCK_TAGS } from './sessionModalHelpers'; import { buildAcpTranscriptGroups, diffsFromAcpContent, @@ -1112,7 +1113,7 @@ const segments: ContentSegment[] = []; let remaining = content; - const tagPattern = /<(action|branch-history|launch-context)>([\s\S]*?)<\/\1>/g; + const tagPattern = new RegExp(`<(${XML_BLOCK_TAGS.join('|')})>([\\s\\S]*?)`, 'g'); let lastIndex = 0; let match: RegExpExecArray | null; @@ -1129,8 +1130,10 @@ ? 'Action instructions' : tag === 'branch-history' ? 'Branch history' - : 'Launch context'; - const icon = tag === 'action' ? Zap : GitBranch; + : tag === 'pikchr-grammar' + ? 'Pikchr grammar' + : 'Launch context'; + const icon = tag === 'action' ? Zap : tag === 'pikchr-grammar' ? BookOpen : GitBranch; segments.push({ type: 'xml-block', tag, label, content: match[2].trim(), icon }); lastIndex = match.index + match[0].length; diff --git a/apps/staged/src/lib/features/sessions/sessionModalHelpers.ts b/apps/staged/src/lib/features/sessions/sessionModalHelpers.ts index 1a94d645..51a13af7 100644 --- a/apps/staged/src/lib/features/sessions/sessionModalHelpers.ts +++ b/apps/staged/src/lib/features/sessions/sessionModalHelpers.ts @@ -25,14 +25,21 @@ export function sessionEndMessage(current: Pick): s return 'This session can be resumed.'; } +/** Tags wrapping injected context blocks in prompts/messages (rendered as collapsed cards). */ +export const XML_BLOCK_TAGS = ['action', 'branch-history', 'launch-context', 'pikchr-grammar']; + +const TAG_ALTERNATION = XML_BLOCK_TAGS.join('|'); + /** Pattern matching XML-tagged context blocks embedded in prompts/messages. */ -const XML_BLOCK_PATTERN = /<(action|branch-history|launch-context)>[\s\S]*?<\/\1>/g; +const XML_BLOCK_PATTERN = new RegExp(`<(${TAG_ALTERNATION})>[\\s\\S]*?`, 'g'); + +const XML_OPEN_TAG_PATTERN = new RegExp(`<(${TAG_ALTERNATION})>`); export function hasXmlBlocks(content: string): boolean { - return /<(action|branch-history|launch-context)>/.test(content); + return XML_OPEN_TAG_PATTERN.test(content); } -/** Strip XML-tagged context blocks (action, branch-history, launch-context) from display text. */ +/** Strip XML-tagged context blocks (XML_BLOCK_TAGS) from display text. */ export function stripXmlTags(text: string): string { return text.replace(XML_BLOCK_PATTERN, '').trim(); }