diff --git a/crates/op-acp/src/client.rs b/crates/op-acp/src/client.rs index 1be503d4d..538bbaabe 100644 --- a/crates/op-acp/src/client.rs +++ b/crates/op-acp/src/client.rs @@ -93,7 +93,11 @@ impl AcpConnection { // Connection closed: fail every in-flight request now so // callers get `Closed` immediately instead of stalling // until the request timeout. - let waiters: Vec<_> = pending.lock().unwrap().drain().collect(); + let waiters: Vec<_> = pending + .lock() + .unwrap_or_else(|p| p.into_inner()) + .drain() + .collect(); for (_, waiter) in waiters { let _ = waiter.send(Err(AcpError::Closed)); } @@ -316,7 +320,11 @@ async fn connect_remote(config: &AcpAgentConfig) -> Result = pending.lock().unwrap().drain().collect(); + let waiters: Vec<_> = pending + .lock() + .unwrap_or_else(|p| p.into_inner()) + .drain() + .collect(); for (_, waiter) in waiters { let _ = waiter.send(Err(AcpError::Closed)); } diff --git a/crates/op-acp/src/jsonrpc.rs b/crates/op-acp/src/jsonrpc.rs index d3bdba105..c9fe2f790 100644 --- a/crates/op-acp/src/jsonrpc.rs +++ b/crates/op-acp/src/jsonrpc.rs @@ -63,12 +63,18 @@ impl JsonRpcEngine { ) -> Result { let id = self.next_id.fetch_add(1, Ordering::Relaxed); let (tx, rx) = oneshot::channel(); - self.pending.lock().unwrap().insert(id, tx); + self.pending + .lock() + .unwrap_or_else(|p| p.into_inner()) + .insert(id, tx); let req = JsonRpcRequest::new(id, method, params); let frame = serde_json::to_value(&req).map_err(|e| AcpError::Protocol(e.to_string()))?; if self.out_tx.send(frame).is_err() { - self.pending.lock().unwrap().remove(&id); + self.pending + .lock() + .unwrap_or_else(|p| p.into_inner()) + .remove(&id); return Err(AcpError::Closed); } @@ -77,7 +83,10 @@ impl JsonRpcEngine { // The reader task dropped the sender — connection died. Ok(Err(_)) => Err(AcpError::Closed), Err(_) => { - self.pending.lock().unwrap().remove(&id); + self.pending + .lock() + .unwrap_or_else(|p| p.into_inner()) + .remove(&id); Err(AcpError::Transport(format!("request '{method}' timed out"))) } } @@ -117,7 +126,11 @@ pub fn dispatch_inbound( ) { match classify_inbound(&value) { Inbound::Response { id, result, error } => { - if let Some(tx) = pending.lock().unwrap().remove(&id) { + if let Some(tx) = pending + .lock() + .unwrap_or_else(|p| p.into_inner()) + .remove(&id) + { let resolved = match error { Some(e) => Err(AcpError::Rpc { code: e.code, diff --git a/crates/op-host-desktop/src/chat_session_launch.rs b/crates/op-host-desktop/src/chat_session_launch.rs index 0be7ca262..85fd29181 100644 --- a/crates/op-host-desktop/src/chat_session_launch.rs +++ b/crates/op-host-desktop/src/chat_session_launch.rs @@ -334,7 +334,7 @@ fn launch_direct_modify_turn( *current_design = None; super::finalize_design_session_if_needed(host, current_chat, "teardown-backstop"); *current_chat = Some(ChatSession::from_channels(chat_rx, Some(tool_rx))); - thread::Builder::new() + let spawned = thread::Builder::new() .name("op-chat-modify".into()) .spawn(move || { op_host_services::chat_intent::run_modify_turn( @@ -344,8 +344,14 @@ fn launch_direct_modify_turn( &executor, target_frame_ids, ); - }) - .expect("spawn op-chat-modify thread"); + }); + if let Err(err) = spawned { + // Worker never started — un-park the session and fall through + // to the honest-error path instead of crashing the UI thread. + eprintln!("openpencil-desktop: spawn op-chat-modify thread failed: {err}"); + *current_chat = None; + return false; + } true } @@ -465,12 +471,21 @@ fn launch_cli_standard_turn( abort: design_abort, model, }; - thread::Builder::new() + let spawned = thread::Builder::new() .name("op-chat-intent".into()) .spawn(move || { op_host_services::chat_intent::run_cli_turn(plan, chat_tx, executor, delta_tx, cmd_tx); - }) - .expect("spawn op-chat-intent thread"); + }); + if let Err(err) = spawned { + // Worker never started — un-park both sessions, end the epoch + // begun above, and fall to the honest-error path instead of + // crashing the UI thread. + eprintln!("openpencil-desktop: spawn op-chat-intent thread failed: {err}"); + *current_chat = None; + *current_design = None; + op_editor_core::agent_indicators::end_if_epoch(indicator_epoch); + return false; + } true } diff --git a/crates/op-host-desktop/src/figma_import_session.rs b/crates/op-host-desktop/src/figma_import_session.rs index da8a35d24..5f84c48c3 100644 --- a/crates/op-host-desktop/src/figma_import_session.rs +++ b/crates/op-host-desktop/src/figma_import_session.rs @@ -179,7 +179,8 @@ pub(crate) fn spawn_approved( let path_for_thread = path.clone(); let worker_cancellation = cancellation.clone(); - thread::Builder::new() + let worker_tx = tx.clone(); + let spawned = thread::Builder::new() .name("op-figma-import".into()) .spawn(move || { let Some(_permit) = worker_cancellation.worker_permit() else { @@ -189,10 +190,15 @@ pub(crate) fn spawn_approved( // Recv side may be gone if the user closed the app // mid-parse; tolerate the SendError silently. if !worker_cancellation.is_cancelled() { - let _ = tx.send(result); + let _ = worker_tx.send(result); } - }) - .expect("spawn op-figma-import worker"); + }); + if let Err(err) = spawned { + // Deliver the failure through the normal pump path (error + // dialog + overlay-flag clear) instead of crashing the app. + eprintln!("[import-figma] failed to spawn worker: {err}"); + let _ = tx.send(Err(format!("import worker failed to start: {err}"))); + } FigmaImportSession { path, @@ -288,7 +294,8 @@ fn conversion_receiver( output_mode: ImportOutputMode, ) -> Receiver { let (tx, rx) = mpsc::channel(); - thread::Builder::new() + let worker_tx = tx.clone(); + let spawned = thread::Builder::new() .name("op-figma-convert".into()) .spawn(move || { let Some(_permit) = cancellation.worker_permit() else { @@ -298,10 +305,15 @@ fn conversion_receiver( persist_import_next_to_source(prepared, &source_path, output_mode, &cancellation) }); if !cancellation.is_cancelled() { - let _ = tx.send(result); + let _ = worker_tx.send(result); } - }) - .expect("spawn op-figma-convert worker"); + }); + if let Err(err) = spawned { + // Deliver the failure through the normal pump path (error + // dialog + overlay-flag clear) instead of crashing the app. + eprintln!("[import-figma] failed to spawn convert worker: {err}"); + let _ = tx.send(Err(format!("convert worker failed to start: {err}"))); + } rx } diff --git a/crates/op-host-desktop/src/git_host.rs b/crates/op-host-desktop/src/git_host.rs index 8a3c16199..2b08f6689 100644 --- a/crates/op-host-desktop/src/git_host.rs +++ b/crates/op-host-desktop/src/git_host.rs @@ -913,12 +913,7 @@ impl DesktopApp { self.git_clone_job = None; return false; } - let Some(result) = self - .git_clone_job - .as_mut() - .expect("clone job present (checked above)") - .poll() - else { + let Some(result) = self.git_clone_job.as_mut().and_then(|job| job.poll()) else { return false; }; self.git_clone_job = None; diff --git a/crates/op-host-desktop/src/html_import_session.rs b/crates/op-host-desktop/src/html_import_session.rs index 82d0b62f5..247652f1e 100644 --- a/crates/op-host-desktop/src/html_import_session.rs +++ b/crates/op-host-desktop/src/html_import_session.rs @@ -42,13 +42,19 @@ pub fn spawn(host: &mut WidgetHostNative, path: PathBuf) -> HtmlImportSession { ui.figma_import_in_progress = true; let path_for_thread = path.clone(); - thread::Builder::new() + let worker_tx = tx.clone(); + let spawned = thread::Builder::new() .name("op-html-import".into()) .spawn(move || { let result = parse_path(&path_for_thread); - let _ = tx.send(result); - }) - .expect("spawn op-html-import worker"); + let _ = worker_tx.send(result); + }); + if let Err(err) = spawned { + // Deliver the failure through the normal pump path (error + // dialog + overlay-flag clear) instead of crashing the app. + eprintln!("[import-html] failed to spawn worker: {err}"); + let _ = tx.send(Err(format!("import worker failed to start: {err}"))); + } HtmlImportSession { path, rx } } diff --git a/crates/op-host-desktop/src/image_decode_host.rs b/crates/op-host-desktop/src/image_decode_host.rs index d0a778a77..ccfa7efba 100644 --- a/crates/op-host-desktop/src/image_decode_host.rs +++ b/crates/op-host-desktop/src/image_decode_host.rs @@ -48,19 +48,26 @@ impl ImageDecodeHost { let (result_tx, result_rx) = mpsc::channel::(); let job_rx = Arc::new(Mutex::new(job_rx)); let wake_proxy = Arc::new(Mutex::new(None::>)); - let workers = (0..WORKER_COUNT) - .map(|index| { + let workers: Vec> = (0..WORKER_COUNT) + .filter_map(|index| { let job_rx = Arc::clone(&job_rx); let result_tx = result_tx.clone(); let wake_proxy = Arc::clone(&wake_proxy); std::thread::Builder::new() .name(format!("op-image-decode-{index}")) .spawn(move || decode_worker(job_rx, result_tx, wake_proxy)) - .expect("spawn image decode worker") + .map_err(|err| { + eprintln!("openpencil-desktop: spawn image decode worker failed: {err}"); + }) + .ok() }) .collect(); + // With no worker to drain the queue, disable submission entirely so + // `pump` retires pending decodes instead of parking them forever + // (images stay at thumbnail quality — degraded, not stuck). + let job_tx = (!workers.is_empty()).then_some(job_tx); Self { - job_tx: Some(job_tx), + job_tx, result_rx, workers, in_flight: 0, diff --git a/crates/op-host-desktop/src/image_decode_stats.rs b/crates/op-host-desktop/src/image_decode_stats.rs index fe1212c0d..ae8960687 100644 --- a/crates/op-host-desktop/src/image_decode_stats.rs +++ b/crates/op-host-desktop/src/image_decode_stats.rs @@ -109,6 +109,10 @@ impl ImageDecodeStats { let reporter = std::thread::Builder::new() .name("op-image-decode-stats".into()) .spawn(move || report_loop(reporter_shared, stop_rx)) + .map_err(|err| { + // Opt-in telemetry only — log and run without stats. + eprintln!("openpencil-desktop: spawn image decode stats reporter failed: {err}"); + }) .ok()?; Some(Self { tally: InstallTally::default(), diff --git a/crates/op-host-desktop/src/save_session.rs b/crates/op-host-desktop/src/save_session.rs index 1884d1c22..3973bacc4 100644 --- a/crates/op-host-desktop/src/save_session.rs +++ b/crates/op-host-desktop/src/save_session.rs @@ -229,7 +229,7 @@ impl SaveSession { Err("background save worker stopped before reporting a result".to_string()) } }; - Some(self.finish_running(result)) + self.finish_running(result) } /// Block only for close/reload confirmation. Ordinary Save never calls @@ -239,7 +239,7 @@ impl SaveSession { let result = self.running.as_ref()?.rx.recv().unwrap_or_else(|_| { Err("background save worker stopped before reporting a result".to_string()) }); - Some(self.finish_running(result)) + self.finish_running(result) } fn matches_pending( @@ -300,7 +300,7 @@ impl SaveSession { let worker_path = path.clone(); let worker_snapshot = Arc::clone(&snapshot); let (tx, rx) = mpsc::channel(); - std::thread::Builder::new() + let spawned = std::thread::Builder::new() .name("op-document-save".to_string()) .spawn(move || { let started = Instant::now(); @@ -334,8 +334,13 @@ impl SaveSession { if let Some(proxy) = wake { let _ = proxy.send_event(DesktopEvent::SaveReady); } - }) - .expect("spawn document save worker"); + }); + if let Err(err) = spawned { + // The worker never started, so `tx` is gone and the rx below is + // disconnected — `poll`/`wait_next` surface that as a failed + // completion instead of the UI thread crashing mid-save. + eprintln!("[save] failed to spawn save worker: {err}"); + } self.running = Some(RunningSave { snapshot, path, @@ -347,8 +352,13 @@ impl SaveSession { }); } - fn finish_running(&mut self, result: Result<(), String>) -> SaveCompletion { - let job = self.running.take().expect("running save exists"); + fn finish_running(&mut self, result: Result<(), String>) -> Option { + let Some(job) = self.running.take() else { + // Both callers check `running` before calling; an ordering bug + // here must not panic mid-save. Drop the stray result instead. + eprintln!("[save] finish_running called with no running job; result dropped"); + return None; + }; let completion = SaveCompletion { path: job.path, set_current_path: job.set_current_path, @@ -360,7 +370,7 @@ impl SaveSession { if let Some(next) = self.queued.take() { self.start(next); } - completion + Some(completion) } } diff --git a/crates/op-host-desktop/src/sub_agent_session.rs b/crates/op-host-desktop/src/sub_agent_session.rs index 3e0a5adc3..fff8f2130 100644 --- a/crates/op-host-desktop/src/sub_agent_session.rs +++ b/crates/op-host-desktop/src/sub_agent_session.rs @@ -111,14 +111,17 @@ pub(crate) fn stash_pending_spawn(specs: Vec, nested: bool) -> bool { if nested { return false; } - *PENDING_SPAWN.lock().unwrap() = Some(specs); + *PENDING_SPAWN.lock().unwrap_or_else(|p| p.into_inner()) = Some(specs); true } /// Take the stashed specs, if any. Called by `app_handler` each frame /// after the parent pump. pub(crate) fn take_pending_spawn() -> Option> { - PENDING_SPAWN.lock().unwrap().take() + PENDING_SPAWN + .lock() + .unwrap_or_else(|p| p.into_inner()) + .take() } // --------------------------------------------------------------------------- @@ -266,7 +269,10 @@ pub(crate) fn abort_all(subs: &mut Vec, active: &mut usize) { // A spawn request can be stashed by the parent one frame before the host // launches it. Stop/New Chat must cancel that queued batch too, otherwise // it starts after the user has already ended the turn. - PENDING_SPAWN.lock().unwrap().take(); + PENDING_SPAWN + .lock() + .unwrap_or_else(|p| p.into_inner()) + .take(); SUB_AGENT_ACTIVE.store(false, Ordering::SeqCst); subs.clear(); *active = 0; diff --git a/crates/op-host-native/src/widget_host.rs b/crates/op-host-native/src/widget_host.rs index bf7989183..70cfffdd0 100644 --- a/crates/op-host-native/src/widget_host.rs +++ b/crates/op-host-native/src/widget_host.rs @@ -1419,14 +1419,29 @@ impl WidgetHostNative { // clipboard paste decode) is dropped instead of applied here. self.document_epoch = self.document_epoch.wrapping_add(1); let old_scene = std::mem::take(&mut self.layout_scene); - std::thread::Builder::new() + // Take-once slot so the drop work runs exactly once: on the worker + // when the spawn succeeds, inline (blocking the UI thread briefly) + // when thread creation fails under FD/memory pressure. + let work: Box = Box::new(move || { + drop(old_state); + drop(old_scene); + after_drop(); + }); + let work = std::sync::Arc::new(std::sync::Mutex::new(Some(work))); + let worker_slot = std::sync::Arc::clone(&work); + let spawned = std::thread::Builder::new() .name("op-import-drop".into()) .spawn(move || { - drop(old_state); - drop(old_scene); - after_drop(); - }) - .expect("spawn op-import-drop worker"); + if let Some(f) = worker_slot.lock().unwrap_or_else(|p| p.into_inner()).take() { + f(); + } + }); + if let Err(err) = spawned { + eprintln!("[widget-host] failed to spawn op-import-drop worker: {err}"); + if let Some(f) = work.lock().unwrap_or_else(|p| p.into_inner()).take() { + f(); + } + } // The imported document restarts at revision 0 / page 0, so its // LayerPanel row-model-cache key aliases the replaced document's. diff --git a/crates/op-host-native/src/widget_host/variables_panel_press.rs b/crates/op-host-native/src/widget_host/variables_panel_press.rs index 91d7c94c0..44db806f1 100644 --- a/crates/op-host-native/src/widget_host/variables_panel_press.rs +++ b/crates/op-host-native/src/widget_host/variables_panel_press.rs @@ -533,11 +533,15 @@ impl WidgetHostNative { } fn unique_numbered(base: &str, exists: impl Fn(&str) -> bool) -> String { - for idx in 1.. { + // Bounded scan — 10k collisions on one base never happens in practice, + // but a pathological document must not spin the UI thread forever. + for idx in 1u32..=10_000 { let candidate = format!("{base}-{idx}"); if !exists(&candidate) { return candidate; } } - unreachable!() + // Deterministic fallback past the bound; a duplicate name is cosmetic + // while an unbounded loop would hang the editor. + format!("{base}-10001") } diff --git a/crates/op-host-services/src/chat_intent.rs b/crates/op-host-services/src/chat_intent.rs index fd6aab485..27f25fbc0 100644 --- a/crates/op-host-services/src/chat_intent.rs +++ b/crates/op-host-services/src/chat_intent.rs @@ -245,7 +245,7 @@ fn classify_intent_llm_with_timeout( // after a timeout makes its sends fail and the drain unwind. let iter = provider.send(req); let (tx, rx) = mpsc::channel::(); - std::thread::Builder::new() + let spawned = std::thread::Builder::new() .name("op-chat-classify".into()) .spawn(move || { for delta in iter { @@ -253,8 +253,12 @@ fn classify_intent_llm_with_timeout( return; } } - }) - .expect("spawn op-chat-classify thread"); + }); + if let Err(err) = spawned { + // TS: any classification failure falls back to { intent: 'new' }. + eprintln!("[chat-intent] failed to spawn classify drain thread: {err}"); + return DesignIntent::New; + } let deadline = Instant::now() + timeout; let mut out = String::new(); diff --git a/crates/op-host-services/src/chat_runtime.rs b/crates/op-host-services/src/chat_runtime.rs index 1ae99d017..6801897c0 100644 --- a/crates/op-host-services/src/chat_runtime.rs +++ b/crates/op-host-services/src/chat_runtime.rs @@ -22,6 +22,7 @@ use std::sync::atomic::{AtomicBool, Ordering}; use std::sync::{Arc, OnceLock}; +use std::task::{Context, Poll, Wake, Waker}; use std::time::Duration; use agent::abort::AbortController; @@ -278,6 +279,13 @@ impl Iterator for BlockingRecvIter { if self.cancel.is_none() { return self.rx.blocking_recv(); } + // Timed blocking bridge: tokio's mpsc has no blocking recv with a + // timeout, so poll the receiver with a waker that unparks this + // thread. A message (or channel close) ends the park immediately; + // the 20 ms cap keeps the cancel flag observed even when the + // provider is silent. + let waker = Waker::from(Arc::new(ThreadUnparker(std::thread::current()))); + let mut cx = Context::from_waker(&waker); loop { if self .cancel @@ -289,17 +297,24 @@ impl Iterator for BlockingRecvIter { } return None; } - match self.rx.try_recv() { - Ok(value) => return Some(value), - Err(mpsc::error::TryRecvError::Disconnected) => return None, - Err(mpsc::error::TryRecvError::Empty) => { - std::thread::sleep(Duration::from_millis(20)); - } + match self.rx.poll_recv(&mut cx) { + Poll::Ready(Some(value)) => return Some(value), + Poll::Ready(None) => return None, + Poll::Pending => std::thread::park_timeout(Duration::from_millis(20)), } } } } +/// Wakes [`BlockingRecvIter::next`]'s park as soon as a message lands. +struct ThreadUnparker(std::thread::Thread); + +impl Wake for ThreadUnparker { + fn wake(self: Arc) { + self.0.unpark(); + } +} + impl Drop for BlockingRecvIter { fn drop(&mut self) { if let Some(task) = self.task.take() { diff --git a/crates/op-host-services/src/design_session.rs b/crates/op-host-services/src/design_session.rs index cd27f13b8..ea426c75f 100644 --- a/crates/op-host-services/src/design_session.rs +++ b/crates/op-host-services/src/design_session.rs @@ -53,7 +53,8 @@ pub fn start( let abort = AbortFlag::new(); let worker_abort = abort.clone(); - thread::Builder::new() + let failure_tx = delta_tx.clone(); + if let Err(err) = thread::Builder::new() .name("op-design-turn".into()) .spawn(move || { run_design_worker( @@ -67,7 +68,17 @@ pub fn start( vision_provider, ) }) - .expect("spawn op-design-turn thread"); + { + // Thread creation can fail under FD/memory pressure; surface a failed + // turn instead of panicking the UI thread. + eprintln!("[design-session] failed to spawn op-design-turn thread: {err}"); + op_editor_core::agent_indicators::end_if_epoch(indicator_epoch); + let _ = failure_tx.send(DesignDelta::Done(Err( + op_orchestrator::OrchestratorError::Internal(format!( + "failed to spawn design worker thread: {err}" + )), + ))); + } DesignSession::from_channels_with_epoch_and_abort(delta_rx, cmd_rx, indicator_epoch, abort) } @@ -172,7 +183,8 @@ pub fn start_subtask_retry( let abort = AbortFlag::new(); let worker_abort = abort.clone(); - thread::Builder::new() + let failure_tx = delta_tx.clone(); + if let Err(err) = thread::Builder::new() .name("op-subtask-retry".into()) .spawn(move || { run_subtask_retry_worker( @@ -186,7 +198,16 @@ pub fn start_subtask_retry( worker_abort, ) }) - .expect("spawn op-subtask-retry thread"); + { + // Same degrade-instead-of-panic path as `start` above. + eprintln!("[design-session] failed to spawn op-subtask-retry thread: {err}"); + op_editor_core::agent_indicators::end_if_epoch(indicator_epoch); + let _ = failure_tx.send(DesignDelta::Done(Err( + op_orchestrator::OrchestratorError::Internal(format!( + "failed to spawn subtask-retry thread: {err}" + )), + ))); + } DesignSession::from_channels_with_epoch_and_abort(delta_rx, cmd_rx, indicator_epoch, abort) } diff --git a/crates/op-host-web/src/widget_host/variables_panel_press.rs b/crates/op-host-web/src/widget_host/variables_panel_press.rs index 807f829e2..80944dd0f 100644 --- a/crates/op-host-web/src/widget_host/variables_panel_press.rs +++ b/crates/op-host-web/src/widget_host/variables_panel_press.rs @@ -530,11 +530,15 @@ impl WidgetHost { } pub(in crate::widget_host) fn unique_numbered(base: &str, exists: impl Fn(&str) -> bool) -> String { - for idx in 1.. { + // Bounded scan — 10k collisions on one base never happens in practice, + // but a pathological document must not spin the UI thread forever. + for idx in 1u32..=10_000 { let candidate = format!("{base}-{idx}"); if !exists(&candidate) { return candidate; } } - unreachable!() + // Deterministic fallback past the bound; a duplicate name is cosmetic + // while an unbounded loop would hang the editor. + format!("{base}-10001") } diff --git a/crates/op-mcp/src/batch_design.rs b/crates/op-mcp/src/batch_design.rs index 752a8a7b1..b723365f8 100644 --- a/crates/op-mcp/src/batch_design.rs +++ b/crates/op-mcp/src/batch_design.rs @@ -134,7 +134,12 @@ pub(crate) fn expand_script_arg( Ok(BatchInputKind::Operations | BatchInputKind::NodesJson) => return None, Err((code, message)) => return Some(Err(ToolOutcome::Err(code, message))), } - let script = args.get("script").expect("selected script exists"); + let Some(script) = args.get("script") else { + return Some(Err(ToolOutcome::Err( + ToolErrorCode::MissingArgument, + "script argument is missing".into(), + ))); + }; #[cfg(feature = "script")] { let program = match crate::script_runner::run_script_to_program(script) { @@ -172,7 +177,12 @@ pub(crate) fn dispatch_batch_design( }; let page_id = optional_page_id(args); if input == BatchInputKind::Operations { - let operations = args.get("operations").expect("selected operations exists"); + let Some(operations) = args.get("operations") else { + return ToolOutcome::Err( + ToolErrorCode::MissingArgument, + "operations argument is missing".into(), + ); + }; if let Some(phase) = phase.filter(|_| is_direct_image_operation(operations)) { return ToolOutcome::Err( ToolErrorCode::InvalidArgument, @@ -223,9 +233,12 @@ pub(crate) fn dispatch_batch_design( Err(e) => ToolOutcome::Err(ToolErrorCode::InvalidArgument, e), }; } - let raw = args - .get("nodes_json") - .expect("selected nodes_json exists after script expansion"); + let Some(raw) = args.get("nodes_json") else { + return ToolOutcome::Err( + ToolErrorCode::MissingArgument, + "nodes_json argument is missing".into(), + ); + }; match parse_batch_items(raw) { Ok(items) if items.is_empty() => ToolOutcome::Err( ToolErrorCode::InvalidArgument, diff --git a/crates/op-mcp/src/batch_design_result.rs b/crates/op-mcp/src/batch_design_result.rs index fe90db768..2a4c67c65 100644 --- a/crates/op-mcp/src/batch_design_result.rs +++ b/crates/op-mcp/src/batch_design_result.rs @@ -26,7 +26,7 @@ use super::batch_design::{ use super::batch_direct_ops::is_direct_image_operation; use super::batch_page::optional_page_id; use super::read_nodes::{page_nodes_snapshots, PageNodes}; -use super::{EditorCommand, McpTool, ToolOutcome}; +use super::{EditorCommand, McpTool, ToolErrorCode, ToolOutcome}; /// `batch_design` — insert a node forest in one shot. Holds a document snapshot /// (page node sets + the doc-wide id state) so the operations path can report @@ -68,7 +68,12 @@ impl McpTool for BatchDesign { if input == BatchInputKind::NodesJson { return dispatch_batch_design(args, None); } - let operations = args.get("operations").expect("selected operations exists"); + let Some(operations) = args.get("operations") else { + return ToolOutcome::Err( + ToolErrorCode::MissingArgument, + "operations argument is missing".into(), + ); + }; match parse_operations(operations) { Ok(ParsedOperations::Insert { parent_id, diff --git a/crates/op-mcp/src/codegen_plan_store_assembly.rs b/crates/op-mcp/src/codegen_plan_store_assembly.rs index d661aa12f..1b43d17ca 100644 --- a/crates/op-mcp/src/codegen_plan_store_assembly.rs +++ b/crates/op-mcp/src/codegen_plan_store_assembly.rs @@ -61,8 +61,9 @@ pub(crate) fn assemble_plan(plan_id: &str, _framework: &str) -> Result Ok(Some(node)), + None => Err(ToolOutcome::Err( + ToolErrorCode::Internal, + "data node normalization produced no node".into(), + )), + } } fn should_preserve_ts_data_node(obj: &serde_json::Map) -> bool { diff --git a/crates/op-mcp/src/write_tools.rs b/crates/op-mcp/src/write_tools.rs index 1bb2b8a9b..00d0fcac7 100644 --- a/crates/op-mcp/src/write_tools.rs +++ b/crates/op-mcp/src/write_tools.rs @@ -324,15 +324,18 @@ impl McpTool for UpdateNode { Ok(v) => v, Err(e) => return e, }; - let x = parse_opt_i32(&patch_args, "x"); - let y = parse_opt_i32(&patch_args, "y"); - let width = parse_opt_i32(&patch_args, "width"); - let height = parse_opt_i32(&patch_args, "height"); - for (lab, v) in [("x", &x), ("y", &y), ("width", &width), ("height", &height)] { - if let Err(e) = v { - return ToolOutcome::Err(ToolErrorCode::InvalidArgument, format!("{lab}: {e}")); + // Parse each dimension, rejecting the first invalid one; collecting + // the validated values here keeps the bindings below unwrap-free. + let mut dims = [None, None, None, None]; + for (slot, key) in dims.iter_mut().zip(["x", "y", "width", "height"]) { + match parse_opt_i32(&patch_args, key) { + Ok(v) => *slot = v, + Err(e) => { + return ToolOutcome::Err(ToolErrorCode::InvalidArgument, format!("{key}: {e}")) + } } } + let [x, y, width, height] = dims; let name = patch_args.get("name").cloned(); let fill_hex = match patch_args.get("fill_hex") { None => None, @@ -344,10 +347,6 @@ impl McpTool for UpdateNode { } Some(s) => Some(s.clone()), }; - let x = x.unwrap(); - let y = y.unwrap(); - let width = width.unwrap(); - let height = height.unwrap(); let page_id = args .get("pageId") .or_else(|| args.get("page_id"))