From 86422ca14ef98dfec5a02bcc3b541d0344617cc7 Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Thu, 23 Jul 2026 15:26:42 -0700 Subject: [PATCH 1/4] feat(pikchr): configurable agent, model, and effort for diagram sub-sessions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The generate_pikchr diagram sub-session always inherited the invoking session's agent and ran at that agent's default model/effort (its driver.run() was handed no config options). Let the user pin diagram generation to a specific agent — and that agent's model and effort — independent of the session that triggered the tool, via a new "Diagram generation" control in General settings. - acp_config: add DiagramSubsessionConfig and read_diagram_subsession_config(), reading the `diagram-subsession-config` preference key (mirroring how branch-prefix is read straight from preferences.json). Model/effort are applied only alongside a configured provider, since their value ids are provider-specific. - pikchr_mcp / pikchr_subsession: resolve the effective provider and thread the model/effort selections into every driver.run() turn (including resumed turns); fall back to the invoking session's agent at its defaults when unset, reproducing the prior behaviour. - frontend: persist the override through the preferences store and add an agent/model/effort picker to the General settings panel, rendered through the same AcpConfigPickerShell/Section chrome as the chat pickers (one trigger button with up to three columns) but with its own discovery and persistence, so the shared session pickers stay untouched. Co-Authored-By: Claude Fable 5 Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/acp_config.rs | 137 +++++++++ apps/staged/src-tauri/src/pikchr_mcp.rs | 14 +- .../staged/src-tauri/src/pikchr_subsession.rs | 70 ++++- .../settings/DiagramConfigSetting.svelte | 289 ++++++++++++++++++ .../settings/GeneralSettingsPanel.svelte | 5 + .../features/settings/preferences.svelte.ts | 56 ++++ 6 files changed, 567 insertions(+), 4 deletions(-) create mode 100644 apps/staged/src/lib/features/settings/DiagramConfigSetting.svelte diff --git a/apps/staged/src-tauri/src/acp_config.rs b/apps/staged/src-tauri/src/acp_config.rs index cb3efb8f..e0be6fb2 100644 --- a/apps/staged/src-tauri/src/acp_config.rs +++ b/apps/staged/src-tauri/src/acp_config.rs @@ -86,6 +86,75 @@ fn selected_config_option( } } +/// Preferences-store key holding the diagram sub-session override (General +/// settings → Diagram generation). Written by the frontend preferences store +/// and read directly from `preferences.json` here, mirroring `branch-prefix`. +const DIAGRAM_SUBSESSION_CONFIG_KEY: &str = "diagram-subsession-config"; + +/// The agent/model/effort the `generate_pikchr` diagram sub-session runs under, +/// distinct from the session that invoked the tool. Every field is optional: an +/// unset (or empty) provider falls the sub-session back to the invoking +/// session's agent, and unset model/effort fall back to that agent's defaults — +/// so an all-unset config reproduces the pre-setting behaviour. Model/effort +/// value ids are meaningful only relative to the provider they were chosen for, +/// so they are applied only when a provider is configured (see +/// [`Self::config_options`]). +#[derive(Debug, Clone, Default, PartialEq, Eq, Deserialize)] +#[serde(rename_all = "camelCase")] +pub(crate) struct DiagramSubsessionConfig { + #[serde(default)] + pub(crate) provider: Option, + #[serde(default)] + pub(crate) model: Option, + #[serde(default)] + pub(crate) effort: Option, +} + +impl DiagramSubsessionConfig { + /// The configured provider id to run the sub-session under, if any. Blank + /// (whitespace-only) values read as unset. + pub(crate) fn provider_id(&self) -> Option<&str> { + self.provider + .as_deref() + .map(str::trim) + .filter(|provider| !provider.is_empty()) + } + + /// The model/effort selections as ACP config options to apply per turn. + /// + /// Returns empty when no provider is configured: the stored model/effort + /// value ids belong to the configured provider, so applying them against a + /// different (inherited) agent would be meaningless. + pub(crate) fn config_options(&self) -> Vec { + if self.provider_id().is_none() { + return Vec::new(); + } + selected_acp_config_options(Some(&AcpConfigSelection { + model: self.model.clone(), + effort: self.effort.clone(), + })) + } +} + +/// Read the diagram sub-session override from the preferences store. Any +/// failure — missing file, unparseable JSON, absent or malformed key — yields +/// the default (all-unset) config, so the sub-session simply inherits the +/// invoking session's agent at its default model/effort. +pub(crate) fn read_diagram_subsession_config() -> DiagramSubsessionConfig { + let Some(path) = crate::preferences_store_path_buf() else { + return DiagramSubsessionConfig::default(); + }; + let Ok(contents) = std::fs::read_to_string(&path) else { + return DiagramSubsessionConfig::default(); + }; + let Ok(json) = serde_json::from_str::(&contents) else { + return DiagramSubsessionConfig::default(); + }; + json.get(DIAGRAM_SUBSESSION_CONFIG_KEY) + .and_then(|value| serde_json::from_value(value.clone()).ok()) + .unwrap_or_default() +} + fn normalize_selector_for_category( config_options: &[SessionConfigOption], category: &SessionConfigOptionCategory, @@ -345,4 +414,72 @@ mod tests { assert_eq!(selected[1].config_id, "reasoning"); assert_eq!(selected[1].value_id, "high"); } + + #[test] + fn diagram_config_deserializes_provider_model_and_effort() { + let config: DiagramSubsessionConfig = serde_json::from_value(serde_json::json!({ + "provider": "claude", + "model": { "configId": "model", "valueId": "opus", "label": "Opus" }, + "effort": { "configId": "reasoning", "valueId": "high", "label": "High" }, + })) + .expect("valid diagram config"); + + assert_eq!(config.provider_id(), Some("claude")); + + let options = config.config_options(); + assert_eq!(options.len(), 2); + assert_eq!(options[0].category, SessionConfigOptionCategory::Model); + assert_eq!(options[0].value_id, "opus"); + assert_eq!( + options[1].category, + SessionConfigOptionCategory::ThoughtLevel + ); + assert_eq!(options[1].value_id, "high"); + } + + #[test] + fn diagram_config_treats_blank_provider_as_unset() { + let config = DiagramSubsessionConfig { + provider: Some(" ".to_string()), + model: Some(AcpConfigValueSelection { + config_id: "model".to_string(), + value_id: "opus".to_string(), + label: None, + }), + effort: None, + }; + + assert_eq!(config.provider_id(), None); + } + + #[test] + fn diagram_config_without_provider_applies_no_config_options() { + // Model/effort value ids are provider-specific; with no configured + // provider the sub-session inherits the invoking agent and must not + // apply overrides meant for a different one. + let config = DiagramSubsessionConfig { + provider: None, + model: Some(AcpConfigValueSelection { + config_id: "model".to_string(), + value_id: "opus".to_string(), + label: None, + }), + effort: Some(AcpConfigValueSelection { + config_id: "reasoning".to_string(), + value_id: "high".to_string(), + label: None, + }), + }; + + assert!(config.config_options().is_empty()); + } + + #[test] + fn diagram_config_defaults_when_missing() { + let config: DiagramSubsessionConfig = + serde_json::from_value(serde_json::json!({})).expect("empty object is valid"); + assert_eq!(config, DiagramSubsessionConfig::default()); + assert_eq!(config.provider_id(), None); + assert!(config.config_options().is_empty()); + } } diff --git a/apps/staged/src-tauri/src/pikchr_mcp.rs b/apps/staged/src-tauri/src/pikchr_mcp.rs index 47236c63..712e9c12 100644 --- a/apps/staged/src-tauri/src/pikchr_mcp.rs +++ b/apps/staged/src-tauri/src/pikchr_mcp.rs @@ -814,7 +814,17 @@ rendered PNG preview you may open as an optional final check." ctx: RequestContext, ) -> Result { let scale = p.scale.unwrap_or(DEFAULT_SCALE); - let provider_id = self.provider_id.clone(); + // Resolve the agent/model/effort the diagram sub-session runs under. The + // user can point it at a specific agent — and that agent's model and + // effort — via General settings → Diagram generation, distinct from the + // session that invoked this tool. When unset, fall back to the invoking + // session's agent (the field this handler was built with) at its + // default model/effort, reproducing the pre-setting behaviour. + let diagram_config = crate::acp_config::read_diagram_subsession_config(); + let (provider_id, config_options) = match diagram_config.provider_id() { + Some(configured) => (configured.to_string(), diagram_config.config_options()), + None => (self.provider_id.clone(), Vec::new()), + }; let session = create_pikchr_child_session(&self.store, &provider_id) .map_err(|e| ErrorData::internal_error(e, None))?; let inner_session_id = session.id.clone(); @@ -933,6 +943,7 @@ accepted a render, so the diagram run was cancelled.", grammar.as_deref(), &p.description, p.previous_pikchr.as_deref(), + &config_options, &slot, &worker_cancel, &worker_cancel_reason, @@ -1475,6 +1486,7 @@ arrow from COLL.e to SNOW.w"#; Some("test grammar body"), "a friendly box", None, + &[], &slot, &worker_cancel, &reason, diff --git a/apps/staged/src-tauri/src/pikchr_subsession.rs b/apps/staged/src-tauri/src/pikchr_subsession.rs index ca02180b..bf032e7d 100644 --- a/apps/staged/src-tauri/src/pikchr_subsession.rs +++ b/apps/staged/src-tauri/src/pikchr_subsession.rs @@ -127,6 +127,7 @@ pub(crate) async fn generate_pikchr_source( grammar: Option<&str>, description: &str, previous_pikchr: Option<&str>, + config_options: &[acp_client::AcpSessionConfigOptionSelection], slot: &LastRenderSlot, cancel_token: &CancellationToken, cancel_reason: &CancelReason, @@ -138,6 +139,7 @@ pub(crate) async fn generate_pikchr_source( grammar, description, previous_pikchr, + config_options, slot, cancel_token, ) @@ -227,6 +229,7 @@ async fn generate_pikchr_source_inner( grammar: Option<&str>, description: &str, previous_pikchr: Option<&str>, + config_options: &[acp_client::AcpSessionConfigOptionSelection], slot: &LastRenderSlot, cancel_token: &CancellationToken, ) -> Result { @@ -263,7 +266,7 @@ async fn generate_pikchr_source_inner( &writer_dyn, cancel_token, agent_session_id.as_deref(), - &[], + config_options, ) .await; writer_dyn.finalize().await; @@ -446,13 +449,15 @@ mod tests { /// Scripted driver that replays canned turns (writing the shared slot the /// way the `render_pikchr` tool would) and records the `agent_session_id` - /// it was handed each turn (to assert resumption). + /// and `config_options` it was handed each turn (to assert resumption and + /// that the diagram model/effort selections reach the driver). struct FakeDriver { slot: Arc, turns: Mutex>, calls: Mutex, seen_session_ids: Mutex>>, prompts: Mutex>, + seen_config_options: Mutex>>, } impl FakeDriver { @@ -463,6 +468,7 @@ mod tests { calls: Mutex::new(0), seen_session_ids: Mutex::new(Vec::new()), prompts: Mutex::new(Vec::new()), + seen_config_options: Mutex::new(Vec::new()), } } } @@ -479,7 +485,7 @@ mod tests { writer: &Arc, _cancel_token: &CancellationToken, agent_session_id: Option<&str>, - _config_options: &[acp_client::AcpSessionConfigOptionSelection], + config_options: &[acp_client::AcpSessionConfigOptionSelection], ) -> Result { *self.calls.lock().unwrap() += 1; self.seen_session_ids @@ -487,6 +493,10 @@ mod tests { .unwrap() .push(agent_session_id.map(str::to_string)); self.prompts.lock().unwrap().push(prompt.to_string()); + self.seen_config_options + .lock() + .unwrap() + .push(config_options.to_vec()); // Mimic a new-session turn: register an agent session id so the // loop resumes it next time. @@ -557,6 +567,7 @@ mod tests { Some("test grammar body"), "a friendly box", None, + &[], slot, cancel, cancel_reason, @@ -613,6 +624,59 @@ mod tests { assert_eq!(messages[1].content, ACCEPT_SENTINEL); } + #[tokio::test] + async fn forwards_config_options_to_the_driver_each_turn() { + use acp_client::AcpSessionConfigOptionSelection; + use agent_client_protocol::schema::v1::SessionConfigOptionCategory; + + let slot = Arc::new(LastRenderSlot::new()); + // Two turns: a protocol miss (no sentinel) then acceptance, so the loop + // resumes and the selections must accompany the resumed turn too. + let driver = FakeDriver::new( + Arc::clone(&slot), + vec![ + turn(Some(render(CLEAN_SOURCE)), "still working on it"), + turn(Some(render(CLEAN_SOURCE)), ACCEPT_SENTINEL), + ], + ); + let (store, session_id) = child_session(); + + let config_options = vec![ + AcpSessionConfigOptionSelection { + category: SessionConfigOptionCategory::Model, + config_id: "model".to_string(), + value_id: "opus".to_string(), + }, + AcpSessionConfigOptionSelection { + category: SessionConfigOptionCategory::ThoughtLevel, + config_id: "reasoning".to_string(), + value_id: "high".to_string(), + }, + ]; + + generate_pikchr_source( + &driver, + Arc::clone(&store), + &session_id, + "/tmp/grammar.md", + "a friendly box", + None, + &config_options, + &slot, + &CancellationToken::new(), + &CancelReason::new(), + ) + .await + .expect("should accept the rendered diagram"); + + let seen = driver.seen_config_options.lock().unwrap(); + assert_eq!(seen.len(), 2, "one entry per driver turn"); + assert!( + seen.iter().all(|options| *options == config_options), + "every turn (including the resumed one) receives the diagram selections" + ); + } + #[test] fn sentinel_must_be_the_final_line() { for reply in [ diff --git a/apps/staged/src/lib/features/settings/DiagramConfigSetting.svelte b/apps/staged/src/lib/features/settings/DiagramConfigSetting.svelte new file mode 100644 index 00000000..601bfc6b --- /dev/null +++ b/apps/staged/src/lib/features/settings/DiagramConfigSetting.svelte @@ -0,0 +1,289 @@ + + + +
+ Diagram generation + + +
+ Agent + + + Same as session + + {#each providers as provider (provider.id)} + + + + {provider.label} + + + {/each} + +
+ + {#if selectedProvider} + {#if modelSelector} +
+ +
+ {/if} + + {#if effortSelector} +
+ +
+ {/if} + + {#if configLoading && (!modelSelector || loadingEffortOptions)} +
+ + + + Loading options… + + +
+ {/if} + {/if} + + {#snippet footer()} + {#if selectedProvider && configError && !modelSelector && !effortSelector} + + Using agent defaults + + {/if} + {/snippet} + + +

+ + {#if !selectedProvider} + Diagrams are drawn by a sub-agent using the same agent as the session that requested them. + Pick an agent to always draw diagrams with a specific agent, model, and effort instead. + {:else if configError && !modelSelector && !effortSelector} + Diagrams will use {providerTriggerLabel} at its default model and effort. + {:else} + Diagrams are drawn by {providerTriggerLabel}{modelTriggerLabel + ? `, model ${modelTriggerLabel}` + : ''}{effortTriggerLabel ? `, ${effortTriggerLabel} effort` : ''}. + {/if} +

+
+ + diff --git a/apps/staged/src/lib/features/settings/GeneralSettingsPanel.svelte b/apps/staged/src/lib/features/settings/GeneralSettingsPanel.svelte index c5be1cc7..b2d082c3 100644 --- a/apps/staged/src/lib/features/settings/GeneralSettingsPanel.svelte +++ b/apps/staged/src/lib/features/settings/GeneralSettingsPanel.svelte @@ -9,6 +9,7 @@ import * as ToggleGroup from '$lib/components/ui/toggle-group'; import { Input } from '$lib/components/ui/input'; import { Label } from '$lib/components/ui/label'; + import DiagramConfigSetting from './DiagramConfigSetting.svelte'; import { preferences, getAvailableSyntaxThemes, @@ -188,6 +189,10 @@

+
+ +
+
, + /** + * Agent/model/effort override for the diagram-generation sub-session, or + * `null` to inherit the invoking session's agent. Read by the backend from + * the store file. + */ + diagramSubsessionConfig: null as DiagramSubsessionConfig | null, /** Whether auto code reviews are triggered after commits */ autoReviewMode: 'after-changes' as AutoReviewMode, /** @@ -297,6 +324,18 @@ export async function initPreferences(): Promise { preferences.acpConfigPrefs = savedConfigPrefs; } + // Load the diagram sub-session agent/model/effort override + const savedDiagramConfig = await getStoreValue( + DIAGRAM_SUBSESSION_CONFIG_STORE_KEY + ); + if ( + savedDiagramConfig && + typeof savedDiagramConfig === 'object' && + !Array.isArray(savedDiagramConfig) + ) { + preferences.diagramSubsessionConfig = savedDiagramConfig; + } + // Load auto-review mode const savedAutoReview = await getStoreValue(AUTO_REVIEW_STORE_KEY); if (savedAutoReview === 'never' || savedAutoReview === 'after-changes') { @@ -442,6 +481,23 @@ export function getAcpConfigPref(providerId: string): AcpConfigPref | null { return preferences.acpConfigPrefs[providerId] ?? null; } +// ============================================================================= +// Diagram Sub-session Config Actions +// ============================================================================= + +/** + * Persist the diagram sub-session agent/model/effort override. + * + * `null` clears the override so diagram generation inherits the invoking + * session's agent (and its default model/effort). Dropping empty configs to + * `null` keeps the store tidy and the backend on its inherit path. + */ +export function setDiagramSubsessionConfig(config: DiagramSubsessionConfig | null): void { + const normalized = config && config.provider ? config : null; + preferences.diagramSubsessionConfig = normalized; + setStoreValue(DIAGRAM_SUBSESSION_CONFIG_STORE_KEY, normalized); +} + /** * Return the most recently used agent that is present in `available`. * From 1037829d0b47e4b642220ecc5ec6a540542b4a0e Mon Sep 17 00:00:00 2001 From: Matt Toohey Date: Mon, 27 Jul 2026 16:59:33 +1000 Subject: [PATCH 2/4] fix(pikchr): fall back to the invoking agent when the diagram override goes stale MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The diagram-generation override stored in `diagram-subsession-config` can drift out from under the preference: an agent update can drop the pinned model/effort value id, or the pinned agent can be uninstalled or lack the HTTP MCP support the render tool needs. Previously any of those failed the `generate_pikchr` call hard — and since the stale preference was re-read on every call and never recovered from, every subsequent diagram generation failed too, while the settings UI silently reconciled the stale id to a valid-looking default. Backend — degrade to the no-override behaviour instead of erroring: - pikchr_subsession: generate_pikchr_source now takes an optional DiagramFallback (the invoking session's agent at its default model/effort). When the override run fails with an error attributable to the override — a stale/missing config selection per acp-client's is_config_selection_unavailable_error, or the required-MCP-transport check — the run retries once on the fallback driver with no config options, and the child session row is moved to the fallback provider so the transcript names the agent that actually drew the diagram. Unrelated failures and cancellations still surface as before. - pikchr_mcp: override runs carry the invoking session's agent as that fallback; an override provider that no longer resolves at all falls back before the run even starts. The stored preference is deliberately left intact so a later agent update can revive it. Settings — show the stale state instead of masking it: - DiagramConfigSetting: detect a stored provider missing from discovery and stored model/effort ids the live options no longer include. The trigger keeps the stale stored label and gains a danger border, and the field description switches to an error explaining that diagrams fall back to the requesting session's own agent until the selection is updated. Tests cover the stale-selection fallback (fallback runs at defaults in a fresh agent session, session row reassigned), that unrelated failures do not reroute, and the override-unavailable error matcher. Co-Authored-By: Claude Fable 5 Signed-off-by: Matt Toohey --- apps/staged/src-tauri/src/pikchr_mcp.rs | 112 +++++++-- .../staged/src-tauri/src/pikchr_subsession.rs | 234 +++++++++++++++++- .../settings/DiagramConfigSetting.svelte | 74 +++++- 3 files changed, 386 insertions(+), 34 deletions(-) diff --git a/apps/staged/src-tauri/src/pikchr_mcp.rs b/apps/staged/src-tauri/src/pikchr_mcp.rs index 712e9c12..48fbe98f 100644 --- a/apps/staged/src-tauri/src/pikchr_mcp.rs +++ b/apps/staged/src-tauri/src/pikchr_mcp.rs @@ -820,10 +820,23 @@ rendered PNG preview you may open as an optional final check." // session that invoked this tool. When unset, fall back to the invoking // session's agent (the field this handler was built with) at its // default model/effort, reproducing the pre-setting behaviour. + // + // An override run additionally carries the invoking session's agent as + // a fallback (`fallback_provider_id`): the stored preference can drift + // stale between runs — agent uninstalled, model/effort id dropped by an + // agent update, agent without the HTTP MCP support the render tool + // needs — and a stale override degrades to the no-override behaviour + // instead of failing every call until the setting is fixed. The stored + // preference stays untouched; the settings UI surfaces its stale state. let diagram_config = crate::acp_config::read_diagram_subsession_config(); - let (provider_id, config_options) = match diagram_config.provider_id() { - Some(configured) => (configured.to_string(), diagram_config.config_options()), - None => (self.provider_id.clone(), Vec::new()), + let (provider_id, config_options, fallback_provider_id) = match diagram_config.provider_id() + { + Some(configured) => ( + configured.to_string(), + diagram_config.config_options(), + Some(self.provider_id.clone()), + ), + None => (self.provider_id.clone(), Vec::new(), None), }; let session = create_pikchr_child_session(&self.store, &provider_id) .map_err(|e| ErrorData::internal_error(e, None))?; @@ -896,22 +909,87 @@ rendered PNG preview you may open as an optional final check." return Err(e); } }; + let preview_mcp_server = || { + vec![McpServer::Http(McpServerHttp::new( + "pikchr-preview", + format!("http://127.0.0.1:{preview_port}/mcp"), + ))] + }; // Providers without HTTP MCP support fail the required- - // transport check and the call errors — acceptable, since the - // parent session already requires an MCP-capable provider to - // have `generate_pikchr` at all. + // transport check inside the run. For a no-override run that + // errors the call — acceptable, since the parent session + // already requires an MCP-capable provider to have + // `generate_pikchr` at all. An override run instead falls back + // to that same invoking agent (below), which the same argument + // makes a safe harbour. + let mut config_options = config_options; + let mut fallback_provider_id = fallback_provider_id; let driver = match AcpDriver::new(&provider_id) { - Ok(driver) => { - driver.with_mcp_servers(vec![McpServer::Http(McpServerHttp::new( - "pikchr-preview", - format!("http://127.0.0.1:{preview_port}/mcp"), - ))]) - } - Err(e) => { - mark_pikchr_child_session_error(&worker_store, &worker_session_id, &e); - return Err(e); - } + Ok(driver) => driver, + // The configured diagram agent no longer resolves (e.g. + // uninstalled since it was chosen): fall back to the + // invoking session's agent up front rather than after a + // doomed run. + Err(e) => match fallback_provider_id.take() { + Some(parent_provider) => match AcpDriver::new(&parent_provider) { + Ok(driver) => { + log::warn!( + "[pikchr_mcp] configured diagram agent unavailable ({e}); \ +falling back to the invoking session's agent" + ); + if let Err(store_error) = worker_store + .set_session_provider(&worker_session_id, &parent_provider) + { + log::warn!( + "[pikchr_mcp] failed to move Pikchr session \ +{worker_session_id} to the fallback provider: {store_error}" + ); + } + config_options = Vec::new(); + driver + } + Err(parent_error) => { + mark_pikchr_child_session_error( + &worker_store, + &worker_session_id, + &parent_error, + ); + return Err(parent_error); + } + }, + None => { + mark_pikchr_child_session_error(&worker_store, &worker_session_id, &e); + return Err(e); + } + }, }; + let driver = driver.with_mcp_servers(preview_mcp_server()); + // The fallback driver for override failures that only surface + // inside the run (stale model/effort selection, unsupported + // MCP transport) — see generate_pikchr_source. An invoking + // agent that itself doesn't resolve simply leaves those + // failures as the hard errors they were. + let fallback_driver = fallback_provider_id.as_deref().and_then(|parent| { + match AcpDriver::new(parent) { + Ok(driver) => Some(driver.with_mcp_servers(preview_mcp_server())), + Err(e) => { + log::warn!( + "[pikchr_mcp] invoking session's agent unavailable as the \ +diagram fallback: {e}" + ); + None + } + } + }); + let fallback = fallback_driver + .as_ref() + .zip(fallback_provider_id.as_deref()) + .map( + |(driver, provider_id)| crate::pikchr_subsession::DiagramFallback { + driver, + provider_id, + }, + ); // Enforce the wall-clock cap by cancelling the sub-session's // token; the driver shuts its subprocess down gracefully. The // parent's `DropGuard` cancels this same token if the MCP @@ -944,6 +1022,7 @@ accepted a render, so the diagram run was cancelled.", &p.description, p.previous_pikchr.as_deref(), &config_options, + fallback, &slot, &worker_cancel, &worker_cancel_reason, @@ -1487,6 +1566,7 @@ arrow from COLL.e to SNOW.w"#; "a friendly box", None, &[], + None, &slot, &worker_cancel, &reason, diff --git a/apps/staged/src-tauri/src/pikchr_subsession.rs b/apps/staged/src-tauri/src/pikchr_subsession.rs index bf032e7d..7886295c 100644 --- a/apps/staged/src-tauri/src/pikchr_subsession.rs +++ b/apps/staged/src-tauri/src/pikchr_subsession.rs @@ -115,6 +115,32 @@ impl LastRenderSlot { } } +/// Fallback identity for a run pinned to the configured diagram override: the +/// invoking session's agent at its default model/effort — exactly what the run +/// would have used with no override. Consulted only when the override itself +/// can't run (see [`is_diagram_override_unavailable_error`]). +pub(crate) struct DiagramFallback<'a, D: AgentDriver + ?Sized> { + pub(crate) driver: &'a D, + /// Provider id written back onto the child session row when the fallback + /// takes over, so the transcript names the agent that actually drew the + /// diagram. + pub(crate) provider_id: &'a str, +} + +/// Whether a generation failure is attributable to the configured diagram +/// override rather than to the request itself: the pinned model/effort +/// selection no longer resolves on the agent, or the pinned agent doesn't +/// support the HTTP MCP transport the `render_pikchr` tool server requires. +/// The transport fragment mirrors the message produced by the acp-client +/// driver's required-transport check — the same producer/matcher string +/// coupling `is_config_selection_unavailable_error` itself relies on; if the +/// producer is reworded this degrades to today's hard error, never to a +/// spurious fallback. +fn is_diagram_override_unavailable_error(error: &str) -> bool { + acp_client::is_config_selection_unavailable_error(error) + || error.contains("does not support required MCP transports") +} + /// Drive the sub-agent to produce validated Pikchr for `description`. /// /// Generic over [`AgentDriver`] so it can be unit-tested with a fake driver @@ -128,11 +154,12 @@ pub(crate) async fn generate_pikchr_source( description: &str, previous_pikchr: Option<&str>, config_options: &[acp_client::AcpSessionConfigOptionSelection], + fallback: Option>, slot: &LastRenderSlot, cancel_token: &CancellationToken, cancel_reason: &CancelReason, ) -> Result { - let result = generate_pikchr_source_inner( + let mut result = generate_pikchr_source_inner( driver, Arc::clone(&store), session_id, @@ -145,6 +172,42 @@ pub(crate) async fn generate_pikchr_source( ) .await; + // The configured diagram override can drift stale between runs: an agent + // update can drop the pinned model/effort id, or the pinned agent may not + // support the transport the render tool needs. Those failures are + // properties of the override, not of this request — retry once on the + // invoking session's agent at its defaults instead of failing the tool. + // The stored preference is deliberately left intact (the settings UI + // surfaces its stale state) so a later agent update can revive it. + if let Some(fallback) = fallback { + if let Err(GenerationError::Failed(message)) = &result { + if is_diagram_override_unavailable_error(message) { + log::warn!( + "[pikchr_subsession] configured diagram agent can't run ({message}); \ +retrying with the invoking session's agent" + ); + if let Err(e) = store.set_session_provider(session_id, fallback.provider_id) { + log::warn!( + "[pikchr_subsession] failed to move Pikchr session {session_id} to the \ +fallback provider: {e}" + ); + } + result = generate_pikchr_source_inner( + fallback.driver, + Arc::clone(&store), + session_id, + grammar_reference, + description, + previous_pikchr, + &[], + slot, + cancel_token, + ) + .await; + } + } + } + // A token cancellation is externally imposed — the wall-clock timeout or // the caller abandoning the MCP request — so resolve the recorded reason // once and tell the same story on the child session's status row and in @@ -427,16 +490,28 @@ mod tests { const CLEAN_SOURCE: &str = r#"box "Clean" fit"#; /// One scripted specialist turn: optionally store a render into the slot - /// (as a real `render_pikchr` call would mid-turn), then reply with `reply`. + /// (as a real `render_pikchr` call would mid-turn), then reply with `reply` + /// — or fail the whole turn with `error` (as a run whose session setup + /// failed would, e.g. on a stale config selection). struct FakeTurn { store_render: Option, reply: String, + error: Option, } fn turn(store_render: Option, reply: &str) -> FakeTurn { FakeTurn { store_render, reply: reply.to_string(), + error: None, + } + } + + fn failing(error: &str) -> FakeTurn { + FakeTurn { + store_render: None, + reply: String::new(), + error: Some(error.to_string()), } } @@ -498,14 +573,6 @@ mod tests { .unwrap() .push(config_options.to_vec()); - // Mimic a new-session turn: register an agent session id so the - // loop resumes it next time. - if agent_session_id.is_none() { - store - .set_agent_session_id(session_id, "fake-agent-session") - .unwrap(); - } - let turn = { let mut turns = self.turns.lock().unwrap(); if turns.is_empty() { @@ -514,6 +581,20 @@ mod tests { turns.remove(0) } }; + // A failing turn dies during session setup: no agent session comes + // up and nothing is written. + if let Some(error) = turn.error { + return Err(error); + } + + // Mimic a new-session turn: register an agent session id so the + // loop resumes it next time. + if agent_session_id.is_none() { + store + .set_agent_session_id(session_id, "fake-agent-session") + .unwrap(); + } + if let Some(outcome) = turn.store_render { self.slot.store(outcome); } @@ -568,6 +649,7 @@ mod tests { "a friendly box", None, &[], + None, slot, cancel, cancel_reason, @@ -662,6 +744,7 @@ mod tests { "a friendly box", None, &config_options, + None, &slot, &CancellationToken::new(), &CancelReason::new(), @@ -677,6 +760,137 @@ mod tests { ); } + #[tokio::test] + async fn stale_override_falls_back_to_the_invoking_agent_at_defaults() { + use acp_client::AcpSessionConfigOptionSelection; + use agent_client_protocol::schema::v1::SessionConfigOptionCategory; + + let slot = Arc::new(LastRenderSlot::new()); + // The override run dies applying its pinned model — the message shape + // acp-client produces when a stored selection no longer resolves. + let override_driver = FakeDriver::new( + Arc::clone(&slot), + vec![failing( + "Selected ACP model value 'opus-legacy' is no longer available \ +for config option 'model'", + )], + ); + let fallback_driver = FakeDriver::new( + Arc::clone(&slot), + vec![turn(Some(render(CLEAN_SOURCE)), ACCEPT_SENTINEL)], + ); + let (store, session_id) = child_session(); + + let config_options = vec![AcpSessionConfigOptionSelection { + category: SessionConfigOptionCategory::Model, + config_id: "model".to_string(), + value_id: "opus-legacy".to_string(), + }]; + + let outcome = generate_pikchr_source( + &override_driver, + Arc::clone(&store), + &session_id, + "/tmp/grammar.md", + "a friendly box", + None, + &config_options, + Some(DiagramFallback { + driver: &fallback_driver, + provider_id: "parent-agent", + }), + &slot, + &CancellationToken::new(), + &CancelReason::new(), + ) + .await + .expect("the fallback run should accept the rendered diagram"); + + assert_eq!(outcome.source, CLEAN_SOURCE); + assert_eq!(*override_driver.calls.lock().unwrap(), 1); + assert_eq!(*fallback_driver.calls.lock().unwrap(), 1); + // The fallback runs the invoking agent at its defaults — the stale + // selections belong to the override agent and must not follow along. + assert!(fallback_driver.seen_config_options.lock().unwrap()[0].is_empty()); + // ...in a fresh agent session, not a resume of the failed attempt. + assert_eq!(fallback_driver.seen_session_ids.lock().unwrap()[0], None); + + let session = store + .get_session(&session_id) + .expect("load session") + .expect("session exists"); + assert_eq!(session.status, SessionStatus::Completed); + assert_eq!( + session.provider.as_deref(), + Some("parent-agent"), + "the session row names the agent that actually drew the diagram" + ); + } + + #[tokio::test] + async fn unrelated_failures_do_not_trigger_the_fallback() { + let slot = Arc::new(LastRenderSlot::new()); + let override_driver = + FakeDriver::new(Arc::clone(&slot), vec![failing("agent subprocess crashed")]); + let fallback_driver = FakeDriver::new(Arc::clone(&slot), vec![]); + let (store, session_id) = child_session(); + + let result = generate_pikchr_source( + &override_driver, + Arc::clone(&store), + &session_id, + "/tmp/grammar.md", + "a friendly box", + None, + &[], + Some(DiagramFallback { + driver: &fallback_driver, + provider_id: "parent-agent", + }), + &slot, + &CancellationToken::new(), + &CancelReason::new(), + ) + .await; + + assert_eq!(result.err().as_deref(), Some("agent subprocess crashed")); + assert_eq!( + *fallback_driver.calls.lock().unwrap(), + 0, + "a failure not attributable to the override must surface, not reroute" + ); + + let session = store + .get_session(&session_id) + .expect("load session") + .expect("session exists"); + assert_eq!(session.status, SessionStatus::Error); + assert_eq!(session.provider.as_deref(), Some("fake-agent")); + } + + #[test] + fn override_unavailable_matcher_covers_stale_selections_and_transport() { + // Stale/missing config selections (the acp-client matcher's cases). + assert!(is_diagram_override_unavailable_error( + "Selected ACP model value 'opus-legacy' is no longer available for config option 'model'" + )); + assert!(is_diagram_override_unavailable_error( + "Agent did not return ACP config options needed to apply selected model before prompting" + )); + // The pinned agent can't host the render tool's HTTP MCP server. + assert!(is_diagram_override_unavailable_error( + "Agent does not support required MCP transports (required: http=true, sse=false; \ +agent: http=false, sse=false). Select a provider that supports MCP over HTTP/SSE." + )); + // Anything else is a real failure and must not reroute. + assert!(!is_diagram_override_unavailable_error( + "agent subprocess crashed" + )); + assert!(!is_diagram_override_unavailable_error( + "The Pikchr specialist did not accept a rendered diagram." + )); + } + #[test] fn sentinel_must_be_the_final_line() { for reply in [ diff --git a/apps/staged/src/lib/features/settings/DiagramConfigSetting.svelte b/apps/staged/src/lib/features/settings/DiagramConfigSetting.svelte index 601bfc6b..1825185b 100644 --- a/apps/staged/src/lib/features/settings/DiagramConfigSetting.svelte +++ b/apps/staged/src/lib/features/settings/DiagramConfigSetting.svelte @@ -14,6 +14,7 @@ -->