diff --git a/crates/op-cli/src/app_control_cli.rs b/crates/op-cli/src/app_control_cli.rs index 410700bbf..d7560a129 100644 --- a/crates/op-cli/src/app_control_cli.rs +++ b/crates/op-cli/src/app_control_cli.rs @@ -1,3 +1,4 @@ +use crate::cli_error::CliError; use op_process_io::{spawn_null, wait_for_child_or, wait_until_false, WaitOutcome}; use serde_json::json; use std::env; @@ -47,7 +48,7 @@ pub(crate) fn run_start( headless: bool, web: bool, host: Option<&str>, -) -> Result { +) -> Result { if web { return run_start_web(port, document_path, host); } @@ -96,7 +97,7 @@ fn editor_will_open(path: &str) -> bool { /// Launch the visible editor with `--live-mcp ` and wait for it to /// publish the discovery port file. No `$TMPDIR` manager files: the /// editor owns `~/.openpencil/.op-mcp-port` and removes it on exit. -fn run_start_live(port: u16, document_path: Option<&str>) -> Result { +fn run_start_live(port: u16, document_path: Option<&str>) -> Result { let binary = find_desktop_binary()?; let mut command = Command::new(&binary); command.arg("--live-mcp").arg(port.to_string()); @@ -106,7 +107,7 @@ fn run_start_live(port: u16, document_path: Option<&str>) -> Result) -> Result { return Ok(start_json(live_pid, live_port, opened.map(Path::new))); } WaitOutcome::Exited(status) => { - return Err(format!( + return Err(CliError::Daemon(format!( "OpenPencil editor exited before serving the live MCP server: {status}" - )); + ))); } WaitOutcome::TimedOut => {} } @@ -133,10 +134,10 @@ fn run_start_live(port: u16, document_path: Option<&str>) -> Result, host: Option<&str>, -) -> Result { +) -> Result { // Reuse only an already-running WEB daemon (token-verified ping + // `mode:"web-canvas"` health). A plain `--mcp-http` server answers the // ping too but serves no editor, so reusing it would hand the user a @@ -160,10 +161,10 @@ fn run_start_web( open_in_browser(&url); return Ok(start_web_json(existing.pid, existing.port, None, host)); } - return Err(format!( + return Err(CliError::Daemon(format!( "a non-web MCP server (pid {}) already owns port {}; run `op stop` first", existing.pid, existing.port - )); + ))); } let binary = find_desktop_binary()?; @@ -188,7 +189,7 @@ fn run_start_web( } command.env(op_config_store::env_vars::MCP_TOKEN, &token); let mut child = spawn_null(&mut command) - .map_err(|e| format!("spawn {} --serve-web: {e}", binary.display()))?; + .map_err(|e| CliError::Daemon(format!("spawn {} --serve-web: {e}", binary.display())))?; let pid = child.id(); write_manager_files(pid, port, &token)?; @@ -199,7 +200,7 @@ fn run_start_web( match wait_for_child_or(&mut child, 50, Duration::from_millis(100), || { crate::mcp_http_cli::mcp_ping_headless(port, &token).then_some(()) }) - .map_err(|e| format!("wait for {} --serve-web: {e}", binary.display()))? + .map_err(|e| CliError::Daemon(format!("wait for {} --serve-web: {e}", binary.display())))? { WaitOutcome::Ready(()) => { let url = format!("http://127.0.0.1:{port}"); @@ -208,15 +209,15 @@ fn run_start_web( } WaitOutcome::Exited(status) => { remove_manager_files(); - return Err(format!( + return Err(CliError::Daemon(format!( "OpenPencil web daemon exited before accepting connections: {status}" - )); + ))); } WaitOutcome::TimedOut => {} } - Err(format!( + Err(CliError::Daemon(format!( "OpenPencil web daemon did not respond on 127.0.0.1:{port} within 5s" - )) + ))) } /// Best-effort default-browser launch, per OS: `open` (macOS), @@ -283,7 +284,7 @@ fn start_web_json(pid: u32, port: u16, document_path: Option<&Path>, host: Optio /// Legacy windowless mode: spawn `--mcp-http` against a `.op` file and /// track it via the `$TMPDIR` pid/port manager files. -fn run_start_headless(port: u16, document_path: Option<&str>) -> Result { +fn run_start_headless(port: u16, document_path: Option<&str>) -> Result { let document = match document_path { Some(path) => PathBuf::from(path), None => default_document_path()?, @@ -302,7 +303,7 @@ fn run_start_headless(port: u16, document_path: Option<&str>) -> Result) -> Result { return Ok(start_json(pid, port, Some(&document))); } WaitOutcome::Exited(status) => { remove_manager_files(); - return Err(format!( + return Err(CliError::Daemon(format!( "OpenPencil MCP server exited before accepting connections: {status}" - )); + ))); } WaitOutcome::TimedOut => {} } - Err(format!( + Err(CliError::Daemon(format!( "OpenPencil MCP server did not respond on 127.0.0.1:{port} within 3s" - )) + ))) } -pub(crate) fn run_stop() -> Result { +pub(crate) fn run_stop() -> Result { // `op stop` asks the server to quit ITSELF via a token-authed // `openpencil/shutdown`. This NEVER signals a pid, so there is no // recycled-pid / wrong-process race anywhere — and the live editor @@ -461,17 +462,22 @@ pub(crate) fn discover_running_port() -> Option { reachable_headless_server().map(|info| info.port) } -pub(crate) fn ensure_document_file(path: &Path) -> Result<(), String> { +pub(crate) fn ensure_document_file(path: &Path) -> Result<(), CliError> { if path.exists() { if path.is_file() { return Ok(()); } - return Err(format!("{} exists but is not a file", path.display())); + return Err(CliError::Io(format!( + "{} exists but is not a file", + path.display() + ))); } if let Some(parent) = path.parent() { - fs::create_dir_all(parent).map_err(|e| format!("create {}: {e}", parent.display()))?; + fs::create_dir_all(parent) + .map_err(|e| CliError::Io(format!("create {}: {e}", parent.display())))?; } - fs::write(path, MINIMAL_DOCUMENT).map_err(|e| format!("write {}: {e}", path.display())) + fs::write(path, MINIMAL_DOCUMENT) + .map_err(|e| CliError::Io(format!("write {}: {e}", path.display()))) } /// Verify `path` holds a document the headless MCP server can actually load, @@ -485,26 +491,27 @@ pub(crate) fn ensure_document_file(path: &Path) -> Result<(), String> { /// bare "connection refused" — with no hint that the file is the cause. The /// check never mutates the file, so a corrupt-but-valuable document is /// preserved rather than silently replaced. -fn preflight_document(path: &Path) -> Result<(), String> { - let bytes = fs::read(path).map_err(|e| format!("read {}: {e}", path.display()))?; +fn preflight_document(path: &Path) -> Result<(), CliError> { + let bytes = + fs::read(path).map_err(|e| CliError::Io(format!("read {}: {e}", path.display())))?; // A `.op` document is UTF-8 JSON; a real `.fig` / `.pen` is a binary ZIP // archive (invalid UTF-8). Reject the binary case here as a wrong file type // rather than letting the server's `read_to_string` fail with an IO-flavored // error that reads like the file is unreadable. let src = std::str::from_utf8(&bytes).map_err(|_| { - format!( + CliError::Document(format!( "{} is not a valid OpenPencil document: not UTF-8 text \ (looks like a binary archive). Only .op (JSON) documents open \ directly; legacy .pen / .fig files must be imported.", path.display() - ) + )) })?; op_pen_loader::load_canonical(src).map(|_| ()).map_err(|e| { - format!( + CliError::Document(format!( "{} is not a valid OpenPencil document: {e}\n\ Only .op (JSON) documents open directly; legacy .pen / .fig files must be imported.", path.display() - ) + )) }) } @@ -544,13 +551,14 @@ fn running_mcp_from_pid_file() -> Option { Some(RunningMcp { pid, port, token }) } -fn write_manager_files(pid: u32, port: u16, token: &str) -> Result<(), String> { +fn write_manager_files(pid: u32, port: u16, token: &str) -> Result<(), CliError> { let (pid_file, port_file, token_file) = manager_files(); fs::write(&pid_file, pid.to_string()) - .map_err(|e| format!("write {}: {e}", pid_file.display()))?; + .map_err(|e| CliError::Io(format!("write {}: {e}", pid_file.display())))?; fs::write(&port_file, port.to_string()) - .map_err(|e| format!("write {}: {e}", port_file.display()))?; - fs::write(&token_file, token).map_err(|e| format!("write {}: {e}", token_file.display())) + .map_err(|e| CliError::Io(format!("write {}: {e}", port_file.display())))?; + fs::write(&token_file, token) + .map_err(|e| CliError::Io(format!("write {}: {e}", token_file.display()))) } fn remove_manager_files() { @@ -581,29 +589,29 @@ fn make_token() -> String { format!("{pid:x}-{nanos:x}") } -fn default_document_path() -> Result { - let store = op_config_store::ConfigStore::user().map_err(|e| e.to_string())?; - default_document_path_in(&store).map_err(|e| e.to_string()) +fn default_document_path() -> Result { + let store = op_config_store::ConfigStore::user().map_err(|e| CliError::Io(e.to_string()))?; + default_document_path_in(&store).map_err(|e| CliError::Io(e.to_string())) } fn default_document_path_in(store: &op_config_store::ConfigStore) -> std::io::Result { store.path(op_config_store::well_known::CLI_SESSION) } -fn home_dir() -> Result { +fn home_dir() -> Result { op_config_store::home_dir() - .map_err(|_| "home directory not available; pass --file ".to_string()) + .map_err(|_| CliError::Io("home directory not available; pass --file ".into())) } -fn find_desktop_binary() -> Result { +fn find_desktop_binary() -> Result { if let Some(path) = env::var_os("OPENPENCIL_DESKTOP_BIN").map(PathBuf::from) { if path.is_file() { return Ok(path); } - return Err(format!( + return Err(CliError::Daemon(format!( "OPENPENCIL_DESKTOP_BIN points to a missing file: {}", path.display() - )); + ))); } for path in desktop_binary_candidates() { @@ -611,7 +619,10 @@ fn find_desktop_binary() -> Result { return Ok(path); } } - Err("OpenPencil desktop binary not found; set OPENPENCIL_DESKTOP_BIN or build openpencil-desktop".into()) + Err(CliError::Daemon( + "OpenPencil desktop binary not found; set OPENPENCIL_DESKTOP_BIN or build openpencil-desktop" + .into(), + )) } fn desktop_binary_candidates() -> Vec { diff --git a/crates/op-cli/src/app_control_cli_tests.rs b/crates/op-cli/src/app_control_cli_tests.rs index e71fb4ecd..9d7f428c9 100644 --- a/crates/op-cli/src/app_control_cli_tests.rs +++ b/crates/op-cli/src/app_control_cli_tests.rs @@ -17,7 +17,9 @@ fn preflight_rejects_malformed_documents_and_accepts_a_valid_one() { // (which would exit(1) before binding → opaque "connection refused"). let archive = dir.join("archive.op"); fs::write(&archive, b"PK\x03\x04\xff\xfe\x00\x01binary\x80\x81").expect("write archive"); - let err = preflight_document(&archive).expect_err("binary archive must be rejected"); + let err = preflight_document(&archive) + .expect_err("binary archive must be rejected") + .to_string(); assert!( err.contains("is not a valid OpenPencil document"), "unexpected error text: {err}" @@ -27,7 +29,9 @@ fn preflight_rejects_malformed_documents_and_accepts_a_valid_one() { // JSON parse-error path (distinct from the invalid-UTF-8 path above). let garbage = dir.join("garbage.op"); fs::write(&garbage, b"this is not a .op document").expect("write garbage"); - let err = preflight_document(&garbage).expect_err("non-document text must be rejected"); + let err = preflight_document(&garbage) + .expect_err("non-document text must be rejected") + .to_string(); assert!( err.contains("is not a valid OpenPencil document"), "unexpected error text: {err}" diff --git a/crates/op-cli/src/cli_conversion.rs b/crates/op-cli/src/cli_conversion.rs index 8f86965d1..62125f335 100644 --- a/crates/op-cli/src/cli_conversion.rs +++ b/crates/op-cli/src/cli_conversion.rs @@ -3,11 +3,12 @@ use std::fs; use super::{flag_value, pair, tool_call, Command, Flags}; +use crate::cli_error::CliError; pub(crate) fn map_design_conversion( positionals: &[String], flags: &Flags, -) -> Result { +) -> Result { match positionals[0].as_str() { "design:upsert-vars" => map_upsert_variables(flags), "design:upsert-component" => map_upsert_component(flags), @@ -18,7 +19,7 @@ pub(crate) fn map_design_conversion( } } -fn map_upsert_variables(flags: &Flags) -> Result { +fn map_upsert_variables(flags: &Flags) -> Result { let key = required_flag( flags, "key", @@ -33,7 +34,7 @@ fn map_upsert_variables(flags: &Flags) -> Result { tool_call("upsert_variables", args) } -fn map_upsert_component(flags: &Flags) -> Result { +fn map_upsert_component(flags: &Flags) -> Result { let key = required_flag( flags, "key", @@ -57,7 +58,7 @@ fn map_upsert_component(flags: &Flags) -> Result { tool_call("upsert_component", args) } -fn map_upsert_screen(flags: &Flags) -> Result { +fn map_upsert_screen(flags: &Flags) -> Result { let key = required_flag( flags, "key", @@ -72,18 +73,20 @@ fn map_upsert_screen(flags: &Flags) -> Result { tool_call("upsert_screen", args) } -fn map_status(flags: &Flags) -> Result { +fn map_status(flags: &Flags) -> Result { let mut args = Vec::new(); if let Some(kind) = flag_value(flags, "kind") { if !matches!(kind.as_str(), "token" | "component" | "screen") { - return Err("--kind must be token, component, or screen".into()); + return Err(CliError::usage( + "--kind must be token, component, or screen", + )); } args.push(pair("kind", kind)); } tool_call("conversion_status", args) } -fn map_lint(flags: &Flags) -> Result { +fn map_lint(flags: &Flags) -> Result { let mut args = Vec::new(); if let Some(node_id) = flag_value(flags, "node") { args.push(pair("nodeId", node_id)); @@ -91,15 +94,15 @@ fn map_lint(flags: &Flags) -> Result { tool_call("lint_document", args) } -fn required_flag(flags: &Flags, name: &str, usage: &str) -> Result { - flag_value(flags, name).ok_or_else(|| usage.to_string()) +fn required_flag(flags: &Flags, name: &str, usage: &str) -> Result { + flag_value(flags, name).ok_or_else(|| CliError::usage(usage)) } -fn read_payload_file(flags: &Flags, usage: &str) -> Result { +fn read_payload_file(flags: &Flags, usage: &str) -> Result { let path = required_flag(flags, "file", usage)?; fs::read_to_string(&path) .map(|contents| contents.trim().to_string()) - .map_err(|e| format!("cannot read --file {path:?}: {e}")) + .map_err(|e| CliError::Io(format!("cannot read --file {path:?}: {e}"))) } fn push_source_fields(args: &mut Vec<(String, String)>, flags: &Flags) { diff --git a/crates/op-cli/src/cli_error.rs b/crates/op-cli/src/cli_error.rs new file mode 100644 index 000000000..a1060ef0d --- /dev/null +++ b/crates/op-cli/src/cli_error.rs @@ -0,0 +1,71 @@ +//! The one error type every `op` subcommand fails with. +//! +//! Style mirrors `op_orchestrator::OrchestratorError`: a plain enum plus a +//! hand-written `Display` — no `thiserror`, no extra dependency. The +//! variants name the REAL failure kinds the CLI can hit (bad arguments, a +//! malformed payload, filesystem trouble, a daemon that will not come up, a +//! transport failure, a tool that reported an error, a document that will not +//! load). Each variant's payload is the exact user-facing sentence, so +//! `Display` is transparent: `main` still prints `op: {e}` byte-for-byte as +//! before and the message-shape tests keep passing. +//! +//! The classification is what's new and load-bearing — a caller (or a future +//! exit-code table) can now branch on the KIND instead of grepping the text. + +use crate::skill_install_error::SkillInstallError; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum CliError { + /// Missing, malformed, or mutually exclusive command-line arguments. + /// The payload is the usage line or validation sentence shown to the user. + Usage(String), + /// A JSON payload (argument, file, or server reply) failed to parse, + /// failed to serialize, or had the wrong shape. + Payload(String), + /// A filesystem read / write / create failed. + Io(String), + /// Locating, spawning, or waiting for the OpenPencil desktop / daemon + /// process failed, or a conflicting server already owns the port. + Daemon(String), + /// Talking to a running MCP server over HTTP failed (connect, timeout, + /// or a non-2xx status). + Transport(String), + /// The remote MCP tool ran and reported its own failure. The payload is + /// the tool's verbatim text — never reworded, so agents parsing `op` + /// output see exactly what the server said. + Tool(String), + /// A document could not be read, validated, converted, or imported. + Document(String), + /// Installing or uninstalling the bundled agent skill failed. + Skill(SkillInstallError), +} + +impl std::fmt::Display for CliError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + CliError::Usage(m) + | CliError::Payload(m) + | CliError::Io(m) + | CliError::Daemon(m) + | CliError::Transport(m) + | CliError::Tool(m) + | CliError::Document(m) => f.write_str(m), + CliError::Skill(e) => write!(f, "{e}"), + } + } +} + +impl std::error::Error for CliError {} + +impl From for CliError { + fn from(error: SkillInstallError) -> Self { + CliError::Skill(error) + } +} + +impl CliError { + /// Shorthand for the very common `--flag`-shaped usage complaint. + pub(crate) fn usage(message: impl Into) -> Self { + CliError::Usage(message.into()) + } +} diff --git a/crates/op-cli/src/cli_export_tests.rs b/crates/op-cli/src/cli_export_tests.rs index e3003dbc9..1d25a5483 100644 --- a/crates/op-cli/src/cli_export_tests.rs +++ b/crates/op-cli/src/cli_export_tests.rs @@ -98,7 +98,10 @@ fn export_rejects_conflicting_target_and_format_flags() { "--output", "/tmp/node.png", ])); - assert!(target.unwrap_err().contains("--item and --selection")); + assert!(target + .unwrap_err() + .to_string() + .contains("--item and --selection")); let format = parse_args(&args(&[ "export", @@ -109,7 +112,10 @@ fn export_rejects_conflicting_target_and_format_flags() { "--formats", "jpeg", ])); - assert!(format.unwrap_err().contains("--format and --formats")); + assert!(format + .unwrap_err() + .to_string() + .contains("--format and --formats")); } #[test] diff --git a/crates/op-cli/src/codegen_cli.rs b/crates/op-cli/src/codegen_cli.rs index b88301e80..9e2b82a44 100644 --- a/crates/op-cli/src/codegen_cli.rs +++ b/crates/op-cli/src/codegen_cli.rs @@ -4,8 +4,9 @@ use super::{ flag_value, json_escape, pair, required_pos, resolve_arg, resolve_file_path_arg, tool_call, Command, Flags, }; +use crate::cli_error::CliError; -pub(super) fn map_codegen(positionals: &[String], flags: &Flags) -> Result { +pub(super) fn map_codegen(positionals: &[String], flags: &Flags) -> Result { match positionals[0].as_str() { "codegen:plan" => map_codegen_plan(positionals, flags), "codegen:submit" => map_codegen_submit(positionals, flags), @@ -15,7 +16,7 @@ pub(super) fn map_codegen(positionals: &[String], flags: &Flags) -> Result Result { +fn map_codegen_plan(positionals: &[String], flags: &Flags) -> Result { let raw = resolve_codegen_payload(positionals, 1, "Usage: op codegen:plan ")?; let plan = compact_json_object(&raw, "plan-json")?; @@ -31,7 +32,7 @@ fn map_codegen_plan(positionals: &[String], flags: &Flags) -> Result Result { +fn map_codegen_submit(positionals: &[String], flags: &Flags) -> Result { let plan_id = required_pos( positionals, 1, @@ -53,7 +54,7 @@ fn map_codegen_submit(positionals: &[String], flags: &Flags) -> Result Result { +fn map_codegen_assemble(positionals: &[String], flags: &Flags) -> Result { let plan_id = required_pos( positionals, 1, @@ -66,7 +67,7 @@ fn map_codegen_assemble(positionals: &[String], flags: &Flags) -> Result Result { +fn map_codegen_clean(positionals: &[String]) -> Result { let plan_id = required_pos(positionals, 1, "Usage: op codegen:clean ")?; tool_call("codegen_clean", vec![pair("planId", plan_id)]) } @@ -75,28 +76,29 @@ fn resolve_codegen_payload( positionals: &[String], index: usize, usage: &str, -) -> Result { +) -> Result { let arg = positionals .get(index) .map(String::as_str) - .ok_or_else(|| usage.to_string())?; + .ok_or_else(|| CliError::usage(usage))?; resolve_arg(Some(arg)) } -fn compact_json_object(raw: &str, label: &str) -> Result { +fn compact_json_object(raw: &str, label: &str) -> Result { let value: Value = serde_json::from_str(raw.trim()) - .map_err(|e| format!("invalid {label} JSON payload: {e}"))?; + .map_err(|e| CliError::Payload(format!("invalid {label} JSON payload: {e}")))?; if !value.is_object() { - return Err(format!("{label} must be a JSON object")); + return Err(CliError::Payload(format!("{label} must be a JSON object"))); } - serde_json::to_string(&value).map_err(|e| format!("cannot serialize {label}: {e}")) + serde_json::to_string(&value) + .map_err(|e| CliError::Payload(format!("cannot serialize {label}: {e}"))) } fn json_string_field(key: &str, value: &str) -> String { format!(r#""{}":"{}""#, json_escape(key), json_escape(value)) } -fn raw_json_tool_call(tool: &str, fields: Vec) -> Result { +fn raw_json_tool_call(tool: &str, fields: Vec) -> Result { Ok(Command::ToolCallJson { tool: tool.to_string(), args_json: format!("{{{}}}", fields.join(",")), diff --git a/crates/op-cli/src/command_helpers.rs b/crates/op-cli/src/command_helpers.rs index 3d77eb8b6..1ca171584 100644 --- a/crates/op-cli/src/command_helpers.rs +++ b/crates/op-cli/src/command_helpers.rs @@ -1,3 +1,4 @@ +use crate::cli_error::CliError; use crate::path_args::resolve_file_path_arg; use crate::{Command, Flags}; @@ -15,13 +16,13 @@ pub(crate) fn pair(key: impl Into, value: impl Into) -> (String, (key.into(), value.into()) } -pub(crate) fn tool_call_with_file(tool: &str, flags: &Flags) -> Result { +pub(crate) fn tool_call_with_file(tool: &str, flags: &Flags) -> Result { let mut pairs = Vec::new(); push_file_path(&mut pairs, flags); tool_call(tool, pairs) } -pub(crate) fn tool_call(tool: &str, args: Vec<(String, String)>) -> Result { +pub(crate) fn tool_call(tool: &str, args: Vec<(String, String)>) -> Result { Ok(Command::ToolCall { tool: tool.to_string(), args, diff --git a/crates/op-cli/src/export_cli.rs b/crates/op-cli/src/export_cli.rs index 5ddd4b17f..b892fe37e 100644 --- a/crates/op-cli/src/export_cli.rs +++ b/crates/op-cli/src/export_cli.rs @@ -3,33 +3,41 @@ use std::path::Path; use base64::Engine as _; use serde_json::Value; +use crate::cli_error::CliError; use crate::command_helpers::flag_value; use crate::mcp_http_cli::{post, tool_call_body}; use crate::{Command, Flags}; -pub(crate) fn map_export(flags: &Flags) -> Result { +pub(crate) fn map_export(flags: &Flags) -> Result { let item_id = flag_value(flags, "item"); let selection = flags.contains_key("selection"); if item_id.is_some() && selection { - return Err("--item and --selection cannot be used together".into()); + return Err(CliError::usage( + "--item and --selection cannot be used together", + )); } let format_flag = flag_value(flags, "format"); let formats_flag = flag_value(flags, "formats"); if let (Some(format), Some(formats)) = (&format_flag, &formats_flag) { if format != formats { - return Err("--format and --formats cannot specify different values".into()); + return Err(CliError::usage( + "--format and --formats cannot specify different values", + )); } } let format = format_flag.or(formats_flag).unwrap_or_else(|| "png".into()); if !matches!(format.as_str(), "png" | "jpeg" | "jpg" | "webp" | "pdf") { - return Err(format!("unsupported export format {format:?}")); + return Err(CliError::Usage(format!( + "unsupported export format {format:?}" + ))); } - let output = flag_value(flags, "output").ok_or("--output is required")?; + let output = + flag_value(flags, "output").ok_or_else(|| CliError::usage("--output is required"))?; let scale = flag_value(flags, "scale"); if let Some(value) = &scale { value .parse::() - .map_err(|_| format!("--scale must be a number, got {value:?}"))?; + .map_err(|_| CliError::Usage(format!("--scale must be a number, got {value:?}")))?; } Ok(Command::Export { item_id, @@ -46,7 +54,7 @@ pub(crate) fn run_export( output: &str, format: &str, scale: Option<&str>, -) -> Result { +) -> Result { let mut arguments = serde_json::Map::new(); if let Some(item_id) = item_id { arguments.insert("itemId".into(), Value::String(item_id.into())); @@ -55,7 +63,7 @@ pub(crate) fn run_export( if let Some(scale) = scale { let scale = scale .parse::() - .map_err(|_| format!("--scale must be a number, got {scale:?}"))?; + .map_err(|_| CliError::Usage(format!("--scale must be a number, got {scale:?}")))?; arguments.insert("scale".into(), Value::from(scale)); } let response = post( @@ -65,18 +73,25 @@ pub(crate) fn run_export( write_export_response(&response, Path::new(output)) } -pub(crate) fn write_export_response(response: &str, output: &Path) -> Result { - let value: Value = serde_json::from_str(response) - .map_err(|error| format!("export_item returned invalid JSON: {error}"))?; +pub(crate) fn write_export_response(response: &str, output: &Path) -> Result { + let value: Value = serde_json::from_str(response).map_err(|error| { + CliError::Payload(format!("export_item returned invalid JSON: {error}")) + })?; let encoded = value .get("bytes_base64") .and_then(Value::as_str) - .ok_or("export_item response is missing bytes_base64")?; + .ok_or_else(|| CliError::Payload("export_item response is missing bytes_base64".into()))?; let bytes = base64::engine::general_purpose::STANDARD .decode(encoded) - .map_err(|error| format!("export_item returned invalid Base64: {error}"))?; - std::fs::write(output, bytes) - .map_err(|error| format!("cannot write export to {}: {error}", output.display()))?; + .map_err(|error| { + CliError::Payload(format!("export_item returned invalid Base64: {error}")) + })?; + std::fs::write(output, bytes).map_err(|error| { + CliError::Io(format!( + "cannot write export to {}: {error}", + output.display() + )) + })?; Ok(serde_json::json!({ "output": output.to_string_lossy(), diff --git a/crates/op-cli/src/figma_cli.rs b/crates/op-cli/src/figma_cli.rs index d8078e58b..21f24b9dc 100644 --- a/crates/op-cli/src/figma_cli.rs +++ b/crates/op-cli/src/figma_cli.rs @@ -3,8 +3,9 @@ use std::path::Path; use serde_json::json; use super::{flag_value, required_pos, Command, Flags}; +use crate::cli_error::CliError; -pub(super) fn map_import_figma(positionals: &[String], flags: &Flags) -> Result { +pub(super) fn map_import_figma(positionals: &[String], flags: &Flags) -> Result { let fig_path = required_pos( positionals, 1, @@ -21,22 +22,23 @@ pub(super) fn figma_default_out_path(fig_path: &str) -> String { .unwrap_or_else(|| fig_path.to_string()) } -pub(super) fn run_import_figma(fig_path: &str, out_path: &str) -> Result { - let bytes = std::fs::read(fig_path).map_err(|e| format!("read {fig_path:?}: {e}"))?; +pub(super) fn run_import_figma(fig_path: &str, out_path: &str) -> Result { + let bytes = + std::fs::read(fig_path).map_err(|e| CliError::Io(format!("read {fig_path:?}: {e}")))?; let file_name = Path::new(fig_path) .file_stem() .and_then(|s| s.to_str()) .unwrap_or("Figma Import"); let import = op_figma::parse_fig_binary(&bytes, file_name, op_figma::FigLayoutMode::OpenPencil) - .map_err(|e| format!("import {fig_path:?}: {e}"))?; + .map_err(|e| CliError::Document(format!("import {fig_path:?}: {e}")))?; // Dedup shared image payloads into the `images` table — an // image-heavy `.fig` references the same bitmap from many fills, // and the inline form writes one full copy per reference. let mut value = serde_json::to_value(&import.document) - .map_err(|e| format!("serialize {out_path:?}: {e}"))?; + .map_err(|e| CliError::Payload(format!("serialize {out_path:?}: {e}")))?; jian_ops_schema::image_table::externalize_images(&mut value); let raw = value.to_string(); - std::fs::write(out_path, raw).map_err(|e| format!("write {out_path:?}: {e}"))?; + std::fs::write(out_path, raw).map_err(|e| CliError::Io(format!("write {out_path:?}: {e}")))?; let page_count = import.document.pages.as_ref().map_or(1, Vec::len); let node_count = import .document diff --git a/crates/op-cli/src/html_cli.rs b/crates/op-cli/src/html_cli.rs index c34ba0723..3356e673c 100644 --- a/crates/op-cli/src/html_cli.rs +++ b/crates/op-cli/src/html_cli.rs @@ -4,11 +4,12 @@ use jian_ops_schema::node::PenNode; use serde_json::json; use super::{flag_value, pair, push_file_path, required_pos, tool_call, Command, Flags}; +use crate::cli_error::CliError; /// The virtual origin `op-html` rebases project-local resources onto. const LOCAL_RESOURCE_ORIGIN: &str = op_html::VIRTUAL_PROJECT_ORIGIN; -pub(super) fn map_import_svg(positionals: &[String], flags: &Flags) -> Result { +pub(super) fn map_import_svg(positionals: &[String], flags: &Flags) -> Result { let path = required_pos( positionals, 1, @@ -29,7 +30,7 @@ pub(super) fn map_import_svg(positionals: &[String], flags: &Flags) -> Result Result { +pub(super) fn map_import_html(positionals: &[String], flags: &Flags) -> Result { let source = required_pos( positionals, 1, @@ -38,7 +39,9 @@ pub(super) fn map_import_html(positionals: &[String], flags: &Flags) -> Result Result Result { +) -> Result { let json_path = required_pos( positionals, 1, @@ -103,9 +106,9 @@ pub(super) fn map_import_snapshot( tool_call("import_web_snapshot", pairs) } -pub(super) fn run_import_html(html_path: &str, out_path: &str) -> Result { - let source_bytes = - std::fs::read(html_path).map_err(|error| format!("read {html_path:?}: {error}"))?; +pub(super) fn run_import_html(html_path: &str, out_path: &str) -> Result { + let source_bytes = std::fs::read(html_path) + .map_err(|error| CliError::Io(format!("read {html_path:?}: {error}")))?; let source = op_html::html_encoding::decode_html_bytes(&source_bytes); let source_path = Path::new(html_path); let resource_dir = source_path.parent().unwrap_or_else(|| Path::new(".")); @@ -130,12 +133,14 @@ pub(super) fn run_import_html(html_path: &str, out_path: &str) -> Result Result Result { +pub(super) fn run_import_snapshot(json_path: &str, out_path: &str) -> Result { let source = std::fs::read_to_string(json_path) - .map_err(|error| format!("read {json_path:?}: {error}"))?; + .map_err(|error| CliError::Io(format!("read {json_path:?}: {error}")))?; let imported = op_html::import_snapshot_document(&source, &op_html::HtmlImportOptions::default()); if imported.document.children.is_empty() { @@ -156,12 +161,14 @@ pub(super) fn run_import_snapshot(json_path: &str, out_path: &str) -> Result Result { +fn run(args: &[String]) -> Result { let Parsed { port, port_explicit, @@ -203,7 +207,7 @@ type Flags = BTreeMap>; /// Parse command-line args. `--port`, `--pretty`, `--help`, and /// `--version` are global; the rest are left for command aliases or /// low-level MCP tool arguments. -fn parse_args(args: &[String]) -> Result { +fn parse_args(args: &[String]) -> Result { let mut port = DEFAULT_PORT; let mut port_explicit = false; let mut pretty = false; @@ -237,16 +241,16 @@ fn parse_args(args: &[String]) -> Result { let raw_port = match inline_value { Some(v) => v, None => { - let next = args - .get(i + 1) - .ok_or("--port needs a value (e.g. --port 3100)")?; + let next = args.get(i + 1).ok_or_else(|| { + CliError::usage("--port needs a value (e.g. --port 3100)") + })?; i += 1; next.clone() } }; - port = raw_port - .parse::() - .map_err(|_| format!("--port must be a u16, got {raw_port:?}"))?; + port = raw_port.parse::().map_err(|_| { + CliError::Usage(format!("--port must be a u16, got {raw_port:?}")) + })?; port_explicit = true; } "pretty" => pretty = true, @@ -291,7 +295,7 @@ fn parse_args(args: &[String]) -> Result { }) } -fn command_from_positionals(positionals: &[String], flags: &Flags) -> Result { +fn command_from_positionals(positionals: &[String], flags: &Flags) -> Result { match positionals[0].as_str() { "help" | "-h" | "--help" => Ok(Command::Help), "version" => Ok(Command::Version), @@ -301,13 +305,15 @@ fn command_from_positionals(positionals: &[String], flags: &Flags) -> Result Result Result { +fn map_get(flags: &Flags) -> Result { let mut pairs = Vec::new(); if let Some(id) = flag_value(flags, "id") { pairs.push(pair("nodeIds", format!(r#"["{}"]"#, json_escape(&id)))); @@ -415,7 +421,7 @@ fn map_get(flags: &Flags) -> Result { tool_call("batch_get", pairs) } -fn map_selection(flags: &Flags) -> Result { +fn map_selection(flags: &Flags) -> Result { let mut pairs = Vec::new(); if let Some(depth) = flag_value(flags, "depth") { pairs.push(pair("readDepth", depth)); @@ -424,7 +430,7 @@ fn map_selection(flags: &Flags) -> Result { tool_call("get_selection", pairs) } -fn map_insert(positionals: &[String], flags: &Flags) -> Result { +fn map_insert(positionals: &[String], flags: &Flags) -> Result { let raw = resolve_arg(positionals.get(1).map(String::as_str))?; let mut pairs = data_pairs(&raw)?; if let Some(parent) = flag_value(flags, "parent") { @@ -440,7 +446,7 @@ fn map_insert(positionals: &[String], flags: &Flags) -> Result tool_call("insert_node", pairs) } -fn map_update(positionals: &[String], flags: &Flags) -> Result { +fn map_update(positionals: &[String], flags: &Flags) -> Result { let node_id = required_pos(positionals, 1, "Usage: op update ")?; let raw = resolve_arg(positionals.get(2).map(String::as_str))?; let mut pairs = vec![pair("nodeId", node_id)]; @@ -455,7 +461,7 @@ fn map_update(positionals: &[String], flags: &Flags) -> Result tool_call("update_node", pairs) } -fn map_replace(positionals: &[String], flags: &Flags) -> Result { +fn map_replace(positionals: &[String], flags: &Flags) -> Result { let node_id = required_pos(positionals, 1, "Usage: op replace ")?; let raw = resolve_arg(positionals.get(2).map(String::as_str))?; let mut pairs = vec![pair("nodeId", node_id)]; @@ -470,7 +476,7 @@ fn map_replace(positionals: &[String], flags: &Flags) -> Result tool_call("replace_node", pairs) } -fn map_delete(positionals: &[String], flags: &Flags) -> Result { +fn map_delete(positionals: &[String], flags: &Flags) -> Result { let id = required_pos(positionals, 1, "Usage: op delete ")?; let mut pairs = vec![pair("nodeId", id)]; if let Some(page) = flag_value(flags, "page") { @@ -480,7 +486,7 @@ fn map_delete(positionals: &[String], flags: &Flags) -> Result tool_call("delete_node", pairs) } -fn map_read_nodes(positionals: &[String], flags: &Flags) -> Result { +fn map_read_nodes(positionals: &[String], flags: &Flags) -> Result { let mut pairs = Vec::new(); if let Some(ids) = positionals.get(1) { pairs.push(pair("nodeIds", ids.clone())); @@ -498,7 +504,7 @@ fn map_read_nodes(positionals: &[String], flags: &Flags) -> Result Result { +fn map_reparent(tool: &str, positionals: &[String], flags: &Flags) -> Result { let id = required_pos( positionals, 1, @@ -528,7 +534,7 @@ fn map_design_like( payload: Option<&String>, flags: &Flags, default_post_process: bool, -) -> Result { +) -> Result { let raw = resolve_arg(payload.map(String::as_str))?; let trimmed = raw.trim(); let script_requested = flags.contains_key("script") @@ -556,7 +562,7 @@ fn map_design_like( tool_call(tool, pairs) } -fn map_design_content(positionals: &[String], flags: &Flags) -> Result { +fn map_design_content(positionals: &[String], flags: &Flags) -> Result { let section_id = required_pos( positionals, 1, @@ -564,21 +570,25 @@ fn map_design_content(positionals: &[String], flags: &Flags) -> Result() - .map_err(|_| format!("--canvas-width must be an integer, got {canvas_width:?}"))?; + let width = canvas_width.parse::().map_err(|_| { + CliError::Usage(format!( + "--canvas-width must be an integer, got {canvas_width:?}" + )) + })?; args.insert("canvasWidth".into(), Value::Number(width.into())); } if let Some(page) = flag_value(flags, "page") { @@ -588,28 +598,32 @@ fn map_design_content(positionals: &[String], flags: &Flags) -> Result Result { +fn map_design_skeleton(positionals: &[String], flags: &Flags) -> Result { let raw = resolve_arg(positionals.get(1).map(String::as_str))?; let payload: Value = serde_json::from_str(raw.trim()) - .map_err(|e| format!("invalid design:skeleton JSON payload: {e}"))?; - let root_frame = payload - .get("rootFrame") - .ok_or("design:skeleton JSON payload must contain rootFrame")?; + .map_err(|e| CliError::Payload(format!("invalid design:skeleton JSON payload: {e}")))?; + let root_frame = payload.get("rootFrame").ok_or_else(|| { + CliError::Payload("design:skeleton JSON payload must contain rootFrame".into()) + })?; if !root_frame.is_object() { - return Err("design:skeleton rootFrame must be an object".into()); + return Err(CliError::Payload( + "design:skeleton rootFrame must be an object".into(), + )); } - let sections = payload - .get("sections") - .ok_or("design:skeleton JSON payload must contain sections")?; + let sections = payload.get("sections").ok_or_else(|| { + CliError::Payload("design:skeleton JSON payload must contain sections".into()) + })?; if !sections.is_array() { - return Err("design:skeleton sections must be an array".into()); + return Err(CliError::Payload( + "design:skeleton sections must be an array".into(), + )); } let mut args = serde_json::Map::new(); @@ -617,14 +631,18 @@ fn map_design_skeleton(positionals: &[String], flags: &Flags) -> Result() - .map_err(|_| format!("--canvas-width must be an integer, got {canvas_width:?}"))?; + let width = canvas_width.parse::().map_err(|_| { + CliError::Usage(format!( + "--canvas-width must be an integer, got {canvas_width:?}" + )) + })?; args.insert("canvasWidth".into(), Value::Number(width.into())); } if let Some(page) = flag_value(flags, "page") { @@ -634,15 +652,16 @@ fn map_design_skeleton(positionals: &[String], flags: &Flags) -> Result Result { - let root_id = flag_value(flags, "root-id").ok_or("Usage: op design:refine --root-id ")?; +fn map_design_refine(flags: &Flags) -> Result { + let root_id = flag_value(flags, "root-id") + .ok_or_else(|| CliError::usage("Usage: op design:refine --root-id "))?; let mut pairs = vec![pair("rootId", root_id)]; if let Some(canvas_width) = flag_value(flags, "canvas-width") { pairs.push(pair("canvasWidth", canvas_width)); @@ -654,7 +673,7 @@ fn map_design_refine(flags: &Flags) -> Result { tool_call("design_refine", pairs) } -fn map_layout(flags: &Flags) -> Result { +fn map_layout(flags: &Flags) -> Result { let mut pairs = Vec::new(); if let Some(parent) = flag_value(flags, "parent") { pairs.push(pair("parentId", parent)); @@ -669,7 +688,7 @@ fn map_layout(flags: &Flags) -> Result { tool_call("snapshot_layout", pairs) } -fn map_find_space(flags: &Flags) -> Result { +fn map_find_space(flags: &Flags) -> Result { let direction = flag_value(flags, "direction").unwrap_or_else(|| "right".into()); let width = flag_value(flags, "width").unwrap_or_else(|| "400".into()); let height = flag_value(flags, "height").unwrap_or_else(|| "300".into()); @@ -694,14 +713,16 @@ fn map_find_space(flags: &Flags) -> Result { tool_call("find_empty_space", pairs) } -fn generic_tool_call(tool: &str, rest: &[String], flags: &Flags) -> Result { +fn generic_tool_call(tool: &str, rest: &[String], flags: &Flags) -> Result { let mut pairs = Vec::new(); for kv in rest { let (k, v) = kv .split_once('=') - .ok_or_else(|| format!("argument must be key=value, got {kv:?}"))?; + .ok_or_else(|| CliError::Usage(format!("argument must be key=value, got {kv:?}")))?; if k.is_empty() { - return Err(format!("argument has an empty key: {kv:?}")); + return Err(CliError::Usage(format!( + "argument has an empty key: {kv:?}" + ))); } pairs.push(pair(k, v)); } @@ -719,29 +740,29 @@ fn is_global_compat_flag(key: &str) -> bool { ["file", "page", "post-process", "canvas-width", "depth"].contains(&key) } -fn parse_json_object(raw: &str) -> Result { - let value: Value = - serde_json::from_str(raw.trim()).map_err(|e| format!("invalid JSON payload: {e}"))?; +fn parse_json_object(raw: &str) -> Result { + let value: Value = serde_json::from_str(raw.trim()) + .map_err(|e| CliError::Payload(format!("invalid JSON payload: {e}")))?; if !value.is_object() { - return Err("JSON payload must be an object".into()); + return Err(CliError::Payload("JSON payload must be an object".into())); } Ok(value) } -fn data_pairs(raw: &str) -> Result, String> { +fn data_pairs(raw: &str) -> Result, CliError> { parse_json_object(raw)?; Ok(vec![pair("data", raw.trim())]) } -fn resolve_arg(arg: Option<&str>) -> Result { +fn resolve_arg(arg: Option<&str>) -> Result { match arg { Some("-") => { let mut input = String::new(); io::stdin() .read_to_string(&mut input) - .map_err(|e| format!("stdin read failed: {e}"))?; + .map_err(|e| CliError::Io(format!("stdin read failed: {e}")))?; if input.trim().is_empty() { - Err("No data received from stdin".into()) + Err(CliError::usage("No data received from stdin")) } else { Ok(input.trim().to_string()) } @@ -750,18 +771,20 @@ fn resolve_arg(arg: Option<&str>) -> Result { let path = &path[1..]; fs::read_to_string(path) .map(|s| s.trim().to_string()) - .map_err(|e| format!("cannot read file {path:?}: {e}")) + .map_err(|e| CliError::Io(format!("cannot read file {path:?}: {e}"))) } Some(value) => Ok(value.to_string()), - None => Err("No data provided. Pass as argument, @filepath, or '-' for stdin".into()), + None => Err(CliError::usage( + "No data provided. Pass as argument, @filepath, or '-' for stdin", + )), } } -fn required_pos(positionals: &[String], index: usize, usage: &str) -> Result { +fn required_pos(positionals: &[String], index: usize, usage: &str) -> Result { positionals .get(index) .cloned() - .ok_or_else(|| usage.to_string()) + .ok_or_else(|| CliError::usage(usage)) } #[cfg(test)] diff --git a/crates/op-cli/src/mcp_http_cli.rs b/crates/op-cli/src/mcp_http_cli.rs index 887f62f54..d91a95271 100644 --- a/crates/op-cli/src/mcp_http_cli.rs +++ b/crates/op-cli/src/mcp_http_cli.rs @@ -1,6 +1,7 @@ use serde_json::Value; use std::time::{SystemTime, UNIX_EPOCH}; +use crate::cli_error::CliError; use op_rpc_transport::{JsonRpcRequest, TcpJsonRpc, PING_TIMEOUT, POST_TIMEOUT, SHUTDOWN_TIMEOUT}; /// OpenPencil MCP identity marker reported in the `ping` reply's `result` @@ -99,10 +100,18 @@ pub(crate) fn http_request(body: &str) -> String { /// return `(http_status, body)`. `http_status` is 0 when no recognizable /// status line was returned. The deadlines stop a stale port whose /// service accepts but never replies from hanging the CLI indefinitely. -fn post_raw(port: u16, body: &str, timeout: std::time::Duration) -> Result<(u16, String), String> { +fn post_raw( + port: u16, + body: &str, + timeout: std::time::Duration, +) -> Result<(u16, String), CliError> { + // `op-rpc-transport` is a shared crate outside this conversion's scope and + // still reports failures as `String`; adapt it here so the stringly-typed + // error never escapes into the CLI's own call graph. TcpJsonRpc::local_mcp(port) .post_raw(body, timeout) .map(|reply| (reply.status, reply.body)) + .map_err(CliError::Transport) } /// POST `body` to the HTTP MCP server on `127.0.0.1:port` and return the @@ -111,12 +120,12 @@ fn post_raw(port: u16, body: &str, timeout: std::time::Duration) -> Result<(u16, /// `tools/call` content envelope is unwrapped to the raw tool result, and an /// `isError` tool result (or a JSON-RPC transport error) is surfaced as an /// error — so `op` exits non-zero on a failed tool, like the TS CLI. -pub(crate) fn post(port: u16, body: &str) -> Result { +pub(crate) fn post(port: u16, body: &str) -> Result { let (status, reply) = post_raw(port, body, POST_TIMEOUT)?; if !(200..300).contains(&status) { - return Err(format!( + return Err(CliError::Transport(format!( "MCP server on 127.0.0.1:{port} returned HTTP {status}: {reply}" - )); + ))); } unwrap_mcp_reply(&reply) } @@ -127,7 +136,7 @@ pub(crate) fn post(port: u16, body: &str) -> Result { /// an `isError:true` result → `Err(text)`. /// - a JSON-RPC transport `error` → `Err(message)`. /// - anything else (e.g. a `tools/list` reply) → the raw reply unchanged. -fn unwrap_mcp_reply(reply: &str) -> Result { +fn unwrap_mcp_reply(reply: &str) -> Result { let Ok(value) = serde_json::from_str::(reply) else { return Ok(reply.to_string()); }; @@ -136,7 +145,7 @@ fn unwrap_mcp_reply(reply: &str) -> Result { .and_then(|err| err.get("message")) .and_then(Value::as_str) { - return Err(message.to_string()); + return Err(CliError::Tool(message.to_string())); } let Some(result) = value.get("result") else { return Ok(reply.to_string()); @@ -148,7 +157,7 @@ fn unwrap_mcp_reply(reply: &str) -> Result { .collect::>() .join("\n"); if result.get("isError").and_then(Value::as_bool) == Some(true) { - return Err(text); + return Err(CliError::Tool(text)); } return Ok(text); } @@ -161,10 +170,11 @@ fn http_get_raw( port: u16, path: &str, timeout: std::time::Duration, -) -> Result<(u16, String), String> { +) -> Result<(u16, String), CliError> { TcpJsonRpc::local_mcp(port) .get_raw(path, timeout) .map(|reply| (reply.status, reply.body)) + .map_err(CliError::Transport) } /// True when `127.0.0.1:port` is the `--serve-web` web-canvas daemon — @@ -279,13 +289,16 @@ mod tests { #[test] fn unwrap_surfaces_iserror_result_as_err() { let reply = r#"{"jsonrpc":"2.0","id":1,"result":{"content":[{"type":"text","text":"Error: boom"}],"isError":true}}"#; - assert_eq!(unwrap_mcp_reply(reply), Err("Error: boom".to_string())); + assert_eq!( + unwrap_mcp_reply(reply).unwrap_err().to_string(), + "Error: boom" + ); } #[test] fn unwrap_surfaces_transport_error_as_err() { let reply = r#"{"jsonrpc":"2.0","id":1,"error":{"code":-32602,"message":"bad args"}}"#; - assert_eq!(unwrap_mcp_reply(reply), Err("bad args".to_string())); + assert_eq!(unwrap_mcp_reply(reply).unwrap_err().to_string(), "bad args"); } #[test] diff --git a/crates/op-cli/src/page_theme_cli.rs b/crates/op-cli/src/page_theme_cli.rs index 5220ece6d..c84e445b2 100644 --- a/crates/op-cli/src/page_theme_cli.rs +++ b/crates/op-cli/src/page_theme_cli.rs @@ -2,14 +2,14 @@ //! mappers — split out of `main.rs` per the 800-line-cap convention. Pure //! arg → `Command` mapping; the shared helpers stay in `main.rs`. +use crate::cli_error::CliError; use crate::{flag_value, pair, push_file_path, required_pos, resolve_arg, tool_call}; use crate::{Command, Flags}; -pub(crate) fn map_page(positionals: &[String], flags: &Flags) -> Result { - let sub = positionals - .get(1) - .map(String::as_str) - .ok_or("Usage: op page list|add|remove|rename|reorder|duplicate ...")?; +pub(crate) fn map_page(positionals: &[String], flags: &Flags) -> Result { + let sub = positionals.get(1).map(String::as_str).ok_or_else(|| { + CliError::usage("Usage: op page list|add|remove|rename|reorder|duplicate ...") + })?; match sub { "list" => { let mut pairs = Vec::new(); @@ -54,11 +54,11 @@ pub(crate) fn map_page(positionals: &[String], flags: &Flags) -> Result Err(format!("unknown page subcommand {sub:?}")), + _ => Err(CliError::Usage(format!("unknown page subcommand {sub:?}"))), } } -pub(crate) fn map_vars_set(positionals: &[String], flags: &Flags) -> Result { +pub(crate) fn map_vars_set(positionals: &[String], flags: &Flags) -> Result { let raw = resolve_arg(positionals.get(1).map(String::as_str))?; let mut pairs = vec![pair("variables", raw)]; if flags.contains_key("replace") { @@ -68,7 +68,7 @@ pub(crate) fn map_vars_set(positionals: &[String], flags: &Flags) -> Result Result { +pub(crate) fn map_themes_set(positionals: &[String], flags: &Flags) -> Result { let raw = resolve_arg(positionals.get(1).map(String::as_str))?; let mut pairs = vec![pair("themes", raw)]; if flags.contains_key("replace") { @@ -78,7 +78,7 @@ pub(crate) fn map_themes_set(positionals: &[String], flags: &Flags) -> Result Result { +pub(crate) fn map_theme_save(positionals: &[String], flags: &Flags) -> Result { let preset_path = required_pos(positionals, 1, "Usage: op theme:save ")?; let mut pairs = vec![pair("presetPath", preset_path)]; if let Some(name) = flag_value(flags, "name") { @@ -88,14 +88,14 @@ pub(crate) fn map_theme_save(positionals: &[String], flags: &Flags) -> Result Result { +pub(crate) fn map_theme_load(positionals: &[String], flags: &Flags) -> Result { let preset_path = required_pos(positionals, 1, "Usage: op theme:load ")?; let mut pairs = vec![pair("presetPath", preset_path)]; push_file_path(&mut pairs, flags); tool_call("load_theme_preset", pairs) } -pub(crate) fn map_theme_list(positionals: &[String]) -> Result { +pub(crate) fn map_theme_list(positionals: &[String]) -> Result { let directory = required_pos(positionals, 1, "Usage: op theme:list ")?; tool_call("list_theme_presets", vec![pair("directory", directory)]) } diff --git a/crates/op-cli/src/skill_export_cli.rs b/crates/op-cli/src/skill_export_cli.rs index 496cb07ae..1e402c35e 100644 --- a/crates/op-cli/src/skill_export_cli.rs +++ b/crates/op-cli/src/skill_export_cli.rs @@ -5,14 +5,17 @@ use std::path::Path; use serde_json::json; +use crate::cli_error::CliError; + const DEFAULT_SKILL_OUT_DIR: &str = ".claude/skills"; -pub(crate) fn run_export(name: &str, out_dir: Option<&str>) -> Result { - let skill = - op_ai_skills::get_skill_by_name(name).ok_or_else(|| format!("unknown skill {name:?}"))?; +pub(crate) fn run_export(name: &str, out_dir: Option<&str>) -> Result { + let skill = op_ai_skills::get_skill_by_name(name) + .ok_or_else(|| CliError::Usage(format!("unknown skill {name:?}")))?; let root = out_dir.unwrap_or(DEFAULT_SKILL_OUT_DIR); let dir = Path::new(root).join(&skill.meta.name); - fs::create_dir_all(&dir).map_err(|e| format!("cannot create {}: {e}", dir.display()))?; + fs::create_dir_all(&dir) + .map_err(|e| CliError::Io(format!("cannot create {}: {e}", dir.display())))?; let path = dir.join("SKILL.md"); let contents = format!( @@ -21,7 +24,8 @@ pub(crate) fn run_export(name: &str, out_dir: Option<&str>) -> Result = std::result::Result; + const BUNDLE_JSON: &str = include_str!("../assets/skill-bundle.json"); -const VERSION_SENTINEL: &str = "__OPENPENCIL_VERSION__"; +pub(crate) const VERSION_SENTINEL: &str = "__OPENPENCIL_VERSION__"; // Top-level bundle version plus four embedded plugin/package manifest versions. const EXPECTED_VERSION_SENTINEL_COUNT: usize = 5; const REPO: &str = "zseven-w/openpencil-skill"; @@ -25,15 +29,13 @@ enum Target { } impl Target { - fn parse(raw: &str) -> Result { + fn parse(raw: &str) -> Result { match raw.to_ascii_lowercase().as_str() { "claude" | "claude-code" | "claudecode" => Ok(Target::Claude), "codex" => Ok(Target::Codex), "cursor" => Ok(Target::Cursor), "opencode" | "open-code" => Ok(Target::OpenCode), - _ => Err(format!( - "unknown target {raw:?}; available: claude, codex, cursor, opencode" - )), + _ => Err(SkillInstallError::UnknownTarget(raw.to_string())), } } @@ -47,22 +49,22 @@ impl Target { } } -pub(crate) fn run_install(target: Option<&str>) -> Result { +pub(crate) fn run_install(target: Option<&str>) -> Result { run_for_home(Action::Install, target, &home_dir()?) } -pub(crate) fn run_uninstall(target: Option<&str>) -> Result { +pub(crate) fn run_uninstall(target: Option<&str>) -> Result { run_for_home(Action::Uninstall, target, &home_dir()?) } #[cfg(test)] -pub(crate) fn install_target_at_home(target: &str, home: &Path) -> Result<(), String> { +pub(crate) fn install_target_at_home(target: &str, home: &Path) -> Result<()> { let bundle = load_bundle()?; install_target(Target::parse(target)?, home, &bundle) } #[cfg(test)] -pub(crate) fn uninstall_target_at_home(target: &str, home: &Path) -> Result<(), String> { +pub(crate) fn uninstall_target_at_home(target: &str, home: &Path) -> Result<()> { uninstall_target(Target::parse(target)?, home) } @@ -72,7 +74,7 @@ enum Action { Uninstall, } -fn run_for_home(action: Action, target: Option<&str>, home: &Path) -> Result { +fn run_for_home(action: Action, target: Option<&str>, home: &Path) -> Result { let targets = resolve_targets(target, action, home)?; let bundle = load_bundle()?; let mut results = Vec::new(); @@ -83,7 +85,9 @@ fn run_for_home(action: Action, target: Option<&str>, home: &Path) -> Result json!({ "target": target.key(), "ok": true }), - Err(error) => json!({ "target": target.key(), "ok": false, "error": error }), + Err(error) => { + json!({ "target": target.key(), "ok": false, "error": error.to_string() }) + } }); } Ok(json!({ @@ -96,20 +100,13 @@ fn run_for_home(action: Action, target: Option<&str>, home: &Path) -> Result, - action: Action, - home: &Path, -) -> Result, String> { +fn resolve_targets(target: Option<&str>, action: Action, home: &Path) -> Result> { if let Some(target) = target { return Ok(vec![Target::parse(target)?]); } let detected = detect_targets(home); if detected.is_empty() && matches!(action, Action::Install) { - return Err( - "no supported AI coding agents detected; pass --target claude|codex|cursor|opencode" - .into(), - ); + return Err(SkillInstallError::NoTargetsDetected); } Ok(detected) } @@ -141,7 +138,7 @@ fn command_exists(name: &str) -> bool { }) } -fn install_target(target: Target, home: &Path, bundle: &SkillBundle) -> Result<(), String> { +fn install_target(target: Target, home: &Path, bundle: &SkillBundle) -> Result<()> { match target { Target::Claude => install_claude(home, bundle), Target::Codex => install_codex(home, bundle), @@ -150,7 +147,7 @@ fn install_target(target: Target, home: &Path, bundle: &SkillBundle) -> Result<( } } -fn uninstall_target(target: Target, home: &Path) -> Result<(), String> { +fn uninstall_target(target: Target, home: &Path) -> Result<()> { match target { Target::Claude => uninstall_claude(home), Target::Codex => uninstall_codex(home), @@ -159,7 +156,7 @@ fn uninstall_target(target: Target, home: &Path) -> Result<(), String> { } } -fn install_claude(home: &Path, bundle: &SkillBundle) -> Result<(), String> { +fn install_claude(home: &Path, bundle: &SkillBundle) -> Result<()> { let cache_dir = home .join(".claude/plugins/cache") .join(SKILL_NAME) @@ -197,7 +194,7 @@ fn install_claude(home: &Path, bundle: &SkillBundle) -> Result<(), String> { write_json_object(&marketplace_path, &marketplaces) } -fn uninstall_claude(home: &Path) -> Result<(), String> { +fn uninstall_claude(home: &Path) -> Result<()> { remove_path(&home.join(".claude/plugins/cache").join(SKILL_NAME))?; let registry_path = home.join(".claude/plugins/installed_plugins.json"); if registry_path.exists() { @@ -210,12 +207,13 @@ fn uninstall_claude(home: &Path) -> Result<(), String> { Ok(()) } -fn install_codex(home: &Path, bundle: &SkillBundle) -> Result<(), String> { +fn install_codex(home: &Path, bundle: &SkillBundle) -> Result<()> { let clone_dir = home.join(".codex").join(SKILL_NAME); write_bundle_to(&clone_dir, bundle)?; let skills_dir = home.join(".agents/skills"); - fs::create_dir_all(&skills_dir).map_err(|e| format!("create {}: {e}", skills_dir.display()))?; + fs::create_dir_all(&skills_dir) + .map_err(|e| SkillInstallError::fs(FsAction::Create, &skills_dir, e))?; let link_path = skills_dir.join(SKILL_NAME); let link_target = clone_dir.join("skills"); if fs::symlink_metadata(&link_path).is_err() { @@ -224,12 +222,12 @@ fn install_codex(home: &Path, bundle: &SkillBundle) -> Result<(), String> { Ok(()) } -fn uninstall_codex(home: &Path) -> Result<(), String> { +fn uninstall_codex(home: &Path) -> Result<()> { remove_path(&home.join(".agents/skills").join(SKILL_NAME))?; remove_path(&home.join(".codex").join(SKILL_NAME)) } -fn install_opencode(home: &Path, bundle: &SkillBundle) -> Result<(), String> { +fn install_opencode(home: &Path, bundle: &SkillBundle) -> Result<()> { // opencode discovers skills by scanning its config directory for // `{skill,skills}/**/SKILL.md` (packages/opencode/src/skill/index.ts). // A `plugin` array entry does NOT work for skills: the npm/git package @@ -240,7 +238,8 @@ fn install_opencode(home: &Path, bundle: &SkillBundle) -> Result<(), String> { write_bundle_to(&bundle_dir, bundle)?; let skills_dir = home.join(".config/opencode/skills"); - fs::create_dir_all(&skills_dir).map_err(|e| format!("create {}: {e}", skills_dir.display()))?; + fs::create_dir_all(&skills_dir) + .map_err(|e| SkillInstallError::fs(FsAction::Create, &skills_dir, e))?; let link_path = skills_dir.join(SKILL_NAME); let link_target = bundle_dir.join("skills"); // The discovery entry is owned by this installer: recreate it on every @@ -253,7 +252,7 @@ fn install_opencode(home: &Path, bundle: &SkillBundle) -> Result<(), String> { prune_opencode_plugin_entry(home) } -fn uninstall_opencode(home: &Path) -> Result<(), String> { +fn uninstall_opencode(home: &Path) -> Result<()> { remove_path(&home.join(".config/opencode/skills").join(SKILL_NAME))?; remove_path(&home.join(".config/opencode").join(SKILL_NAME))?; prune_opencode_plugin_entry(home) @@ -261,7 +260,7 @@ fn uninstall_opencode(home: &Path) -> Result<(), String> { /// Remove the legacy `openpencil-skill@git+…` plugin entry (older installers /// wrote it; opencode installs the package but never loads anything from it). -fn prune_opencode_plugin_entry(home: &Path) -> Result<(), String> { +fn prune_opencode_plugin_entry(home: &Path) -> Result<()> { let config_path = home.join(".config/opencode/opencode.json"); if !config_path.exists() { return Ok(()); @@ -272,73 +271,75 @@ fn prune_opencode_plugin_entry(home: &Path) -> Result<(), String> { write_json_object(&config_path, &config) } -fn render_bundle_template(template: &str, version: &str) -> Result { +fn render_bundle_template( + template: &str, + version: &str, +) -> std::result::Result { let sentinel_count = template.matches(VERSION_SENTINEL).count(); if sentinel_count != EXPECTED_VERSION_SENTINEL_COUNT { - return Err(format!( - "embedded skill bundle template expected {EXPECTED_VERSION_SENTINEL_COUNT} version \ - sentinels {VERSION_SENTINEL:?}, found {sentinel_count}" - )); + return Err(BundleError::SentinelCount { + expected: EXPECTED_VERSION_SENTINEL_COUNT, + found: sentinel_count, + }); } let rendered = template.replace(VERSION_SENTINEL, version); if rendered.contains(VERSION_SENTINEL) { - return Err( - "embedded skill bundle still contains the version sentinel after rendering".into(), - ); + return Err(BundleError::SentinelRemains); } Ok(rendered) } -fn load_bundle() -> Result { +fn load_bundle() -> Result { let rendered = render_bundle_template(BUNDLE_JSON, env!("CARGO_PKG_VERSION"))?; let value: Value = - serde_json::from_str(&rendered).map_err(|e| format!("parse skill bundle: {e}"))?; + serde_json::from_str(&rendered).map_err(|e| BundleError::Parse(e.to_string()))?; let version = value .get("version") .and_then(Value::as_str) - .ok_or("skill bundle missing version")? + .ok_or(BundleError::MissingField("version"))? .to_string(); let files_obj = value .get("files") .and_then(Value::as_object) - .ok_or("skill bundle missing files")?; + .ok_or(BundleError::MissingField("files"))?; if files_obj.is_empty() { - return Err("embedded skill bundle is empty".into()); + return Err(BundleError::Empty.into()); } let mut files = Vec::new(); for (path, content) in files_obj { let content = content .as_str() - .ok_or_else(|| format!("bundle file {path:?} is not a string"))?; + .ok_or_else(|| BundleError::FileNotString(path.clone()))?; files.push((path.clone(), content.to_string())); } Ok(SkillBundle { version, files }) } -fn write_bundle_to(dest: &Path, bundle: &SkillBundle) -> Result<(), String> { - fs::create_dir_all(dest).map_err(|e| format!("create {}: {e}", dest.display()))?; +fn write_bundle_to(dest: &Path, bundle: &SkillBundle) -> Result<()> { + fs::create_dir_all(dest).map_err(|e| SkillInstallError::fs(FsAction::Create, dest, e))?; for (relative, content) in &bundle.files { let path = dest.join(relative); if let Some(parent) = path.parent() { - fs::create_dir_all(parent).map_err(|e| format!("create {}: {e}", parent.display()))?; + fs::create_dir_all(parent) + .map_err(|e| SkillInstallError::fs(FsAction::Create, parent, e))?; } - fs::write(&path, content).map_err(|e| format!("write {}: {e}", path.display()))?; + fs::write(&path, content).map_err(|e| SkillInstallError::fs(FsAction::Write, &path, e))?; } Ok(()) } -fn link_or_copy_dir(target: &Path, link_path: &Path) -> Result<(), String> { +fn link_or_copy_dir(target: &Path, link_path: &Path) -> Result<()> { #[cfg(unix)] { std::os::unix::fs::symlink(target, link_path) .or_else(|_| copy_dir_recursive(target, link_path)) - .map_err(|e| format!("link {} -> {}: {e}", link_path.display(), target.display())) + .map_err(|e| SkillInstallError::link(link_path, target, e)) } #[cfg(windows)] { std::os::windows::fs::symlink_dir(target, link_path) .or_else(|_| copy_dir_recursive(target, link_path)) - .map_err(|e| format!("link {} -> {}: {e}", link_path.display(), target.display())) + .map_err(|e| SkillInstallError::link(link_path, target, e)) } } @@ -357,7 +358,7 @@ fn copy_dir_recursive(src: &Path, dest: &Path) -> std::io::Result<()> { Ok(()) } -fn remove_path(path: &Path) -> Result<(), String> { +fn remove_path(path: &Path) -> Result<()> { // Only a missing entry is "nothing to remove"; any other metadata error // (permissions, I/O) must propagate — treating it as absence would let a // later create step fail with a misleading error, or silently keep a @@ -365,62 +366,64 @@ fn remove_path(path: &Path) -> Result<(), String> { let metadata = match fs::symlink_metadata(path) { Ok(metadata) => metadata, Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(()), - Err(e) => return Err(format!("inspect {}: {e}", path.display())), + Err(e) => return Err(SkillInstallError::fs(FsAction::Inspect, path, e)), }; if metadata.file_type().is_symlink() { remove_symlink_path(path) } else if metadata.is_file() { - fs::remove_file(path).map_err(|e| format!("remove {}: {e}", path.display())) + fs::remove_file(path).map_err(|e| SkillInstallError::fs(FsAction::Remove, path, e)) } else { - fs::remove_dir_all(path).map_err(|e| format!("remove {}: {e}", path.display())) + fs::remove_dir_all(path).map_err(|e| SkillInstallError::fs(FsAction::Remove, path, e)) } } #[cfg(windows)] -fn remove_symlink_path(path: &Path) -> Result<(), String> { +fn remove_symlink_path(path: &Path) -> Result<()> { // Windows removes directory symlinks via remove_dir and file symlinks via // remove_file. `is_dir()` follows the target, so a DANGLING directory // symlink reports false — try both forms instead of classifying. fs::remove_dir(path) .or_else(|_| fs::remove_file(path)) - .map_err(|e| format!("remove {}: {e}", path.display())) + .map_err(|e| SkillInstallError::fs(FsAction::Remove, path, e)) } #[cfg(not(windows))] -fn remove_symlink_path(path: &Path) -> Result<(), String> { - fs::remove_file(path).map_err(|e| format!("remove {}: {e}", path.display())) +fn remove_symlink_path(path: &Path) -> Result<()> { + fs::remove_file(path).map_err(|e| SkillInstallError::fs(FsAction::Remove, path, e)) } -fn read_json_object(path: &Path) -> Result, String> { +fn read_json_object(path: &Path) -> Result> { let text = match fs::read_to_string(path) { Ok(text) => text, Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(Map::new()), - Err(e) => return Err(format!("read {}: {e}", path.display())), + Err(e) => return Err(SkillInstallError::fs(FsAction::Read, path, e)), }; if text.trim().is_empty() { return Ok(Map::new()); } let value: Value = - serde_json::from_str(&text).map_err(|e| format!("parse {}: {e}", path.display()))?; + serde_json::from_str(&text).map_err(|e| SkillInstallError::fs(FsAction::Parse, path, e))?; value .as_object() .cloned() - .ok_or_else(|| format!("{} must contain a JSON object", path.display())) + .ok_or_else(|| SkillInstallError::NotAJsonObject(path.to_path_buf())) } -fn write_json_object(path: &Path, root: &Map) -> Result<(), String> { +fn write_json_object(path: &Path, root: &Map) -> Result<()> { if let Some(parent) = path.parent() { - fs::create_dir_all(parent).map_err(|e| format!("create {}: {e}", parent.display()))?; + fs::create_dir_all(parent) + .map_err(|e| SkillInstallError::fs(FsAction::Create, parent, e))?; } let text = serde_json::to_string_pretty(root) - .map_err(|e| format!("serialize {}: {e}", path.display()))?; - fs::write(path, format!("{text}\n")).map_err(|e| format!("write {}: {e}", path.display())) + .map_err(|e| SkillInstallError::fs(FsAction::Serialize, path, e))?; + fs::write(path, format!("{text}\n")) + .map_err(|e| SkillInstallError::fs(FsAction::Write, path, e)) } fn object_entry<'a>( root: &'a mut Map, key: &str, -) -> Result<&'a mut Map, String> { +) -> Result<&'a mut Map> { let entry = root .entry(key.to_string()) .or_insert_with(|| Value::Object(Map::new())); @@ -429,7 +432,7 @@ fn object_entry<'a>( } entry .as_object_mut() - .ok_or_else(|| format!("{key} is not an object")) + .ok_or_else(|| SkillInstallError::NotAnObject(key.to_string())) } fn array_entry<'a>(root: &'a mut Map, key: &str) -> &'a mut Vec { @@ -442,11 +445,11 @@ fn array_entry<'a>(root: &'a mut Map, key: &str) -> &'a mut Vec Result { +fn home_dir() -> Result { env::var_os("HOME") .or_else(|| env::var_os("USERPROFILE")) .map(PathBuf::from) - .ok_or_else(|| "home directory not available".to_string()) + .ok_or(SkillInstallError::HomeUnavailable) } fn timestamp_string() -> String { @@ -522,7 +525,8 @@ mod tests { #[test] fn bundle_renderer_rejects_missing_version_sentinels() { let error = render_bundle_template("{}", env!("CARGO_PKG_VERSION")) - .expect_err("template without version sentinels should fail"); + .expect_err("template without version sentinels should fail") + .to_string(); let expected = format!("expected {EXPECTED_VERSION_SENTINEL_COUNT}"); assert!(error.contains(&expected), "unexpected error: {error}"); @@ -533,7 +537,8 @@ mod tests { fn bundle_renderer_rejects_partially_templated_versions() { let partial_template = BUNDLE_JSON.replacen(VERSION_SENTINEL, env!("CARGO_PKG_VERSION"), 1); let error = render_bundle_template(&partial_template, env!("CARGO_PKG_VERSION")) - .expect_err("partially rendered template should fail"); + .expect_err("partially rendered template should fail") + .to_string(); let expected = format!("expected {EXPECTED_VERSION_SENTINEL_COUNT}"); let found = format!("found {}", EXPECTED_VERSION_SENTINEL_COUNT - 1); @@ -544,7 +549,8 @@ mod tests { #[test] fn gemini_cli_is_not_an_install_target() { let error = super::Target::parse("gemini-cli") - .expect_err("retired Gemini CLI integration must stay unavailable"); + .expect_err("retired Gemini CLI integration must stay unavailable") + .to_string(); assert!( error.contains("unknown target"), diff --git a/crates/op-cli/src/skill_install_error.rs b/crates/op-cli/src/skill_install_error.rs new file mode 100644 index 000000000..a8f3ad8c7 --- /dev/null +++ b/crates/op-cli/src/skill_install_error.rs @@ -0,0 +1,172 @@ +//! Typed failures for `op install` / `op uninstall` (see `skill_install_cli`). +//! +//! Same hand-rolled style as `op_orchestrator::OrchestratorError` — a plain +//! enum with a `Display` impl, no `thiserror`. Every variant reproduces the +//! exact sentence the stringly-typed version produced, so the JSON `error` +//! field each per-target result carries is unchanged. + +use std::fmt; +use std::path::PathBuf; + +/// The filesystem verb that failed. Doubles as the message prefix, which is +/// why the messages read `create : ` etc. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum FsAction { + Create, + Write, + Read, + Remove, + Inspect, + Parse, + Serialize, +} + +impl FsAction { + fn label(self) -> &'static str { + match self { + FsAction::Create => "create", + FsAction::Write => "write", + FsAction::Read => "read", + FsAction::Remove => "remove", + FsAction::Inspect => "inspect", + FsAction::Parse => "parse", + FsAction::Serialize => "serialize", + } + } +} + +/// Something is wrong with the skill bundle compiled into the binary — a +/// build-time packaging fault, never a user mistake. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum BundleError { + /// The template did not carry the expected number of version sentinels. + SentinelCount { expected: usize, found: usize }, + /// A sentinel survived rendering (so a shipped file would advertise the + /// placeholder as its version). + SentinelRemains, + /// The rendered bundle is not valid JSON. + Parse(String), + /// A required top-level field (`version` / `files`) is absent. + MissingField(&'static str), + /// The bundle carries no files at all. + Empty, + /// A bundle file entry is not a JSON string. + FileNotString(String), +} + +impl fmt::Display for BundleError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + BundleError::SentinelCount { expected, found } => write!( + f, + "embedded skill bundle template expected {expected} version sentinels {:?}, \ + found {found}", + crate::skill_install_cli::VERSION_SENTINEL + ), + BundleError::SentinelRemains => f.write_str( + "embedded skill bundle still contains the version sentinel after rendering", + ), + BundleError::Parse(e) => write!(f, "parse skill bundle: {e}"), + BundleError::MissingField(field) => write!(f, "skill bundle missing {field}"), + BundleError::Empty => f.write_str("embedded skill bundle is empty"), + BundleError::FileNotString(path) => write!(f, "bundle file {path:?} is not a string"), + } + } +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum SkillInstallError { + /// `--target ` named an agent this build does not support. + UnknownTarget(String), + /// Nothing to install into and no `--target` to disambiguate. + NoTargetsDetected, + /// Neither `$HOME` nor `%USERPROFILE%` is set. + HomeUnavailable, + /// A plain filesystem / serde operation against `path` failed. + Fs { + action: FsAction, + path: PathBuf, + detail: String, + }, + /// Neither symlinking nor copying could create the discovery entry. + Link { + link: PathBuf, + target: PathBuf, + detail: String, + }, + /// An agent config file exists but its root is not a JSON object. + NotAJsonObject(PathBuf), + /// A config key that must hold an object holds something else. + NotAnObject(String), + /// The bundle compiled into this binary is unusable. + Bundle(BundleError), +} + +impl SkillInstallError { + pub(crate) fn fs( + action: FsAction, + path: impl Into, + detail: impl fmt::Display, + ) -> Self { + SkillInstallError::Fs { + action, + path: path.into(), + detail: detail.to_string(), + } + } + + pub(crate) fn link( + link: impl Into, + target: impl Into, + detail: impl fmt::Display, + ) -> Self { + SkillInstallError::Link { + link: link.into(), + target: target.into(), + detail: detail.to_string(), + } + } +} + +impl fmt::Display for SkillInstallError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + SkillInstallError::UnknownTarget(raw) => write!( + f, + "unknown target {raw:?}; available: claude, codex, cursor, opencode" + ), + SkillInstallError::NoTargetsDetected => f.write_str( + "no supported AI coding agents detected; pass --target claude|codex|cursor|opencode", + ), + SkillInstallError::HomeUnavailable => f.write_str("home directory not available"), + SkillInstallError::Fs { + action, + path, + detail, + } => write!(f, "{} {}: {detail}", action.label(), path.display()), + SkillInstallError::Link { + link, + target, + detail, + } => write!( + f, + "link {} -> {}: {detail}", + link.display(), + target.display() + ), + SkillInstallError::NotAJsonObject(path) => { + write!(f, "{} must contain a JSON object", path.display()) + } + SkillInstallError::NotAnObject(key) => write!(f, "{key} is not an object"), + SkillInstallError::Bundle(e) => write!(f, "{e}"), + } + } +} + +impl std::error::Error for SkillInstallError {} + +impl From for SkillInstallError { + fn from(error: BundleError) -> Self { + SkillInstallError::Bundle(error) + } +} diff --git a/crates/op-host-services/src/lib.rs b/crates/op-host-services/src/lib.rs index 0dc680f70..996231e19 100644 --- a/crates/op-host-services/src/lib.rs +++ b/crates/op-host-services/src/lib.rs @@ -75,6 +75,7 @@ pub mod settings_io; pub mod validation_providers; pub(crate) mod web_auth; pub mod web_canvas_server; +mod web_canvas_server_error; pub mod web_chat_standard; pub mod web_credential_policy; pub mod web_credentials; diff --git a/crates/op-host-services/src/web_canvas_server.rs b/crates/op-host-services/src/web_canvas_server.rs index 04f56c4f0..9170750c1 100644 --- a/crates/op-host-services/src/web_canvas_server.rs +++ b/crates/op-host-services/src/web_canvas_server.rs @@ -28,8 +28,12 @@ use base64::Engine as _; use op_editor_core::agent_settings::{AcpAgentConnectOutcome, ProviderConnectOutcome}; use op_editor_core::{AgentSettings, EditorState}; +use crate::web_canvas_server_error::WebCanvasError; use crate::web_credential_policy::WebCredentialPersistence; +/// Every fallible step of this module fails with [`WebCanvasError`]. +type Result = std::result::Result; + /// Slow/stalled-peer bound — bodies can be large (whole documents with embedded /// images), so a connection that opens and dribbles must not pin a thread. const IO_TIMEOUT: Duration = Duration::from_secs(30); @@ -188,9 +192,10 @@ impl WebCanvasState { /// Clear the transient web-sync document back to the same starter document a /// fresh browser shell paints before the daemon applies updates. - pub(crate) fn reset_document(&mut self) -> Result { + pub(crate) fn reset_document(&mut self) -> Result { if let Some(path) = self.current_path.clone() { - let mut next = crate::mcp_serve::load_editor_state(&path)?; + let mut next = + crate::mcp_serve::load_editor_state(&path).map_err(WebCanvasError::Document)?; preserve_web_canvas_preferences(&self.editor, &mut next); set_file_name_display(&mut next, &path); self.editor = next; @@ -209,7 +214,7 @@ impl WebCanvasState { /// retryable, and the document/version are left exactly as /// `reset_document`'s own `?`-early-return already guarantees (nothing /// touched before the fallible load succeeds). - pub(crate) fn reset_document_guarded(&mut self) -> Result { + pub(crate) fn reset_document_guarded(&mut self) -> Result { if self.reset_consumed { return Ok(ResetOutcome { skipped: true }); } @@ -233,8 +238,9 @@ impl WebCanvasState { &mut self, body: &str, base_version_override: Option, - ) -> Result { - let request = crate::mcp_serve::parse_document_sync_request(body)?; + ) -> Result { + let request = crate::mcp_serve::parse_document_sync_request(body) + .map_err(WebCanvasError::BadRequest)?; let base_version = base_version_override.or(request.base_version); if let Some(expected) = base_version { if expected != self.version { @@ -258,8 +264,8 @@ impl WebCanvasState { } // Load via the same proven path as desktop file-open. A load failure // is a client fault → 400, like the TS validation 400s. - let loaded = - op_pen_loader::load_canonical(request.document_json).map_err(|e| e.to_string())?; + let loaded = op_pen_loader::load_canonical(request.document_json) + .map_err(|e| WebCanvasError::Document(e.to_string()))?; for w in &loaded.warnings { eprintln!("openpencil-desktop --serve-web: schema warning: {w:?}"); } @@ -306,7 +312,9 @@ fn persist_api_settings( save_credentials: F, ) -> WebReply where - F: FnOnce(&EditorState) -> Result<(), String>, + // `settings_io::save_checked` is outside this conversion's scope and still + // reports `String`; only its success/failure is consulted here. + F: FnOnce(&EditorState) -> std::result::Result<(), String>, { if method == "POST" && path == "/api/settings/credentials" && reply.status == "200 OK" { if save_credentials(&state.editor).is_err() { @@ -396,9 +404,9 @@ pub fn handle_web_canvas_request( }) .to_string(), }, - Err(message) => WebReply { - status: "400 Bad Request", - body: crate::mcp_serve::rest_error_body(&message), + Err(error) => WebReply { + status: error.http_status(), + body: crate::mcp_serve::rest_error_body(&error.to_string()), }, }, ("GET", "/api/mcp/version") => WebReply { @@ -424,7 +432,7 @@ pub fn handle_web_canvas_request( body: crate::mcp_serve::document_sync_ok(state.version), }, Err(e) => WebReply { - status: "400 Bad Request", + status: e.http_status(), body: crate::mcp_serve::rest_error_body(&format!("sync reset failed: {e}")), }, }, @@ -539,13 +547,13 @@ fn export_raster_download(body: &str, state: &WebCanvasState) -> WebReply { .to_string(), }, Err(e) => WebReply { - status: "400 Bad Request", + status: e.http_status(), body: crate::mcp_serve::rest_error_body(&format!("export raster failed: {e}")), }, } } -fn build_raster_download(body: &str, fallback: &EditorState) -> Result { +fn build_raster_download(body: &str, fallback: &EditorState) -> Result { let parsed = parse_export_body(body)?; let editor = export_editor_from_value(parsed.as_ref(), fallback)?; let (format, file_name, mime, ext) = raster_format_from_export_body(parsed.as_ref())?; @@ -564,9 +572,9 @@ fn build_raster_download(body: &str, fallback: &EditorState) -> Result Result, -) -> Result< - ( - crate::export::RasterFormat, - &'static str, - &'static str, - &'static str, - ), - String, -> { +) -> Result<( + crate::export::RasterFormat, + &'static str, + &'static str, + &'static str, +)> { let format = body .and_then(|body| body.get("format")) .and_then(|format| format.as_str()) @@ -609,7 +614,9 @@ fn raster_format_from_export_body( "image/webp", "webp", )), - other => Err(format!("unsupported raster format: {other}")), + other => Err(WebCanvasError::BadRequest(format!( + "unsupported raster format: {other}" + ))), } } @@ -643,23 +650,23 @@ fn export_pdf_download(body: &str, state: &WebCanvasState) -> WebReply { .to_string(), }, Err(e) => WebReply { - status: "400 Bad Request", + status: e.http_status(), body: crate::mcp_serve::rest_error_body(&format!("export PDF failed: {e}")), }, } } -fn build_pdf_download(body: &str, fallback: &EditorState) -> Result, String> { +fn build_pdf_download(body: &str, fallback: &EditorState) -> Result> { let editor = export_editor_from_body(body, fallback)?; let scene = op_pen_loader::editor_state_to_layout_scene(&editor); let tmp = tmp_export_path("pdf"); - crate::export_pdf::export_pdf(&scene, &tmp)?; - let bytes = std::fs::read(&tmp).map_err(|e| e.to_string())?; + crate::export_pdf::export_pdf(&scene, &tmp).map_err(WebCanvasError::Export)?; + let bytes = std::fs::read(&tmp).map_err(|e| WebCanvasError::Io(e.to_string()))?; let _ = std::fs::remove_file(&tmp); Ok(bytes) } -fn export_editor_from_body(body: &str, fallback: &EditorState) -> Result { +fn export_editor_from_body(body: &str, fallback: &EditorState) -> Result { let parsed = parse_export_body(body)?; export_editor_from_value(parsed.as_ref(), fallback) } @@ -667,16 +674,19 @@ fn export_editor_from_body(body: &str, fallback: &EditorState) -> Result, fallback: &EditorState, -) -> Result { +) -> Result { let Some(doc) = body.and_then(|body| body.get("document")) else { return Ok(fallback.clone()); }; if !doc.is_object() { - return Err("document must be an object".into()); + return Err(WebCanvasError::BadRequest( + "document must be an object".into(), + )); } - let src = serde_json::to_string(doc).map_err(|e| e.to_string())?; + let src = serde_json::to_string(doc).map_err(|e| WebCanvasError::BadRequest(e.to_string()))?; let editor_meta = op_pen_loader::extract_editor_meta(&src); - let loaded = op_pen_loader::load_canonical(&src).map_err(|e| e.to_string())?; + let loaded = + op_pen_loader::load_canonical(&src).map_err(|e| WebCanvasError::Document(e.to_string()))?; let mut editor = EditorState::from_document(loaded.value); if let Some(meta) = editor_meta { op_pen_loader::apply_editor_meta(&mut editor, meta); @@ -698,13 +708,13 @@ fn export_editor_from_value( Ok(editor) } -fn parse_export_body(body: &str) -> Result, String> { +fn parse_export_body(body: &str) -> Result> { if body.trim().is_empty() { return Ok(None); } serde_json::from_str(body) .map(Some) - .map_err(|e| format!("parse request body: {e}")) + .map_err(|e| WebCanvasError::BadRequest(format!("parse request body: {e}"))) } fn tmp_export_path(ext: &str) -> PathBuf { @@ -741,7 +751,7 @@ fn save_current_file(body: &str, state: &mut WebCanvasState) -> WebReply { } } Err(e) => WebReply { - status: "400 Bad Request", + status: e.http_status(), body: crate::mcp_serve::rest_error_body(&format!("save failed: {e}")), }, } @@ -751,7 +761,7 @@ fn save_editor_from_body( body: &str, previous: &EditorState, path: &std::path::Path, -) -> Result { +) -> Result { let (doc, active_page_index, editor_meta) = document_and_active_page_from_body(body)?; let mut next = previous.clone(); next.replace_document(doc); @@ -769,30 +779,30 @@ fn save_editor_from_body( next.ui.active_page_index = index.min(page_count - 1); } set_file_name_display(&mut next, path); - crate::doc_io::save_to_path(&next, path)?; + crate::doc_io::save_to_path(&next, path).map_err(WebCanvasError::Io)?; Ok(next) } fn document_and_active_page_from_body( body: &str, -) -> Result< - ( - jian_ops_schema::PenDocument, - Option, - Option, - ), - String, -> { - let parsed = - crate::mcp_serve::parse_borrowed_document_envelope(body).map_err(|e| e.to_string())?; +) -> Result<( + jian_ops_schema::PenDocument, + Option, + Option, +)> { + let parsed = crate::mcp_serve::parse_borrowed_document_envelope(body) + .map_err(|e| WebCanvasError::BadRequest(e.to_string()))?; let Some(doc_json) = parsed.document_json else { - return Err("missing document".into()); + return Err(WebCanvasError::BadRequest("missing document".into())); }; if !doc_json.trim_start().starts_with('{') { - return Err("document must be an object".into()); + return Err(WebCanvasError::BadRequest( + "document must be an object".into(), + )); } let editor_meta = op_pen_loader::extract_editor_meta(doc_json); - let loaded = op_pen_loader::load_canonical(doc_json).map_err(|e| e.to_string())?; + let loaded = op_pen_loader::load_canonical(doc_json) + .map_err(|e| WebCanvasError::Document(e.to_string()))?; for w in &loaded.warnings { eprintln!("openpencil-desktop --serve-web: schema warning: {w:?}"); } @@ -1269,22 +1279,33 @@ pub struct ServeWebOptions { /// The host defaults to loopback; `--host 0.0.0.0` is the LAN/Docker opt-in /// (no TLS — deploy behind a proxy for anything beyond a trusted network). pub fn parse_serve_web_args>( - mut args: I, -) -> Result { + args: I, +) -> std::result::Result { + // Public entry point consumed by `cli_modes.rs` and the host binaries, + // which are outside this conversion's scope — keep the `String` contract + // and adapt the typed error here rather than rippling outward. + parse_serve_web_args_typed(args).map_err(|e| e.to_string()) +} + +fn parse_serve_web_args_typed>(mut args: I) -> Result { let Some(first) = args.next() else { - return Err("missing arg".into()); + return Err(WebCanvasError::Config("missing arg".into())); }; if first.starts_with("--") { return parse_serve_web_args_managed(first, args); } let Ok(port) = first.parse::() else { - return Err(format!(" must be a u16, got {first:?}")); + return Err(WebCanvasError::Config(format!( + " must be a u16, got {first:?}" + ))); }; let mut path: Option = None; let mut host = "127.0.0.1".to_string(); while let Some(arg) = args.next() { if arg == "--host" { - host = args.next().ok_or("--host needs a value (e.g. 0.0.0.0)")?; + host = args.next().ok_or_else(|| { + WebCanvasError::Config("--host needs a value (e.g. 0.0.0.0)".into()) + })?; } else if let Some(value) = arg.strip_prefix("--host=") { host = value.to_string(); } else if path.is_none() { @@ -1292,11 +1313,11 @@ pub fn parse_serve_web_args>( // from the same starter document the web shell paints locally. path = Some(PathBuf::from(arg)); } else { - return Err(format!("unexpected arg {arg:?}")); + return Err(WebCanvasError::Config(format!("unexpected arg {arg:?}"))); } } if host.is_empty() { - return Err("--host must not be empty".into()); + return Err(WebCanvasError::Config("--host must not be empty".into())); } Ok(ServeWebOptions { port, @@ -1315,7 +1336,7 @@ pub fn parse_serve_web_args>( fn parse_serve_web_args_managed>( first_flag: String, mut args: I, -) -> Result { +) -> Result { let mut managed = false; let mut port: Option = None; let mut path: Option = None; @@ -1326,30 +1347,36 @@ fn parse_serve_web_args_managed>( match arg.as_str() { "--managed" => managed = true, "--port" => { - let value = args.next().ok_or("--port needs a value")?; - port = Some( - value - .parse::() - .map_err(|_| format!("--port must be a u16, got {value:?}"))?, - ); + let value = args + .next() + .ok_or_else(|| WebCanvasError::Config("--port needs a value".into()))?; + port = Some(value.parse::().map_err(|_| { + WebCanvasError::Config(format!("--port must be a u16, got {value:?}")) + })?); } "--file" => { - path = Some(PathBuf::from(args.next().ok_or("--file needs a value")?)); + path = Some(PathBuf::from(args.next().ok_or_else(|| { + WebCanvasError::Config("--file needs a value".into()) + })?)); } "--host" => { - host = args.next().ok_or("--host needs a value (e.g. 0.0.0.0)")?; + host = args.next().ok_or_else(|| { + WebCanvasError::Config("--host needs a value (e.g. 0.0.0.0)".into()) + })?; } "--allow-origin" => { - allow_origins.push(args.next().ok_or("--allow-origin needs a value")?); + allow_origins.push(args.next().ok_or_else(|| { + WebCanvasError::Config("--allow-origin needs a value".into()) + })?); } - other => return Err(format!("unexpected arg {other:?}")), + other => return Err(WebCanvasError::Config(format!("unexpected arg {other:?}"))), } } let Some(port) = port else { - return Err("missing --port ".into()); + return Err(WebCanvasError::Config("missing --port ".into())); }; if host.is_empty() { - return Err("--host must not be empty".into()); + return Err(WebCanvasError::Config("--host must not be empty".into())); } Ok(ServeWebOptions { port, @@ -1397,10 +1424,11 @@ fn random_token() -> String { fn startup_editor_from_base_for_web_canvas( base: EditorState, path: Option, -) -> Result { +) -> Result { match path { Some(p) => { - let mut next = crate::mcp_serve::load_editor_state(&p)?; + let mut next = + crate::mcp_serve::load_editor_state(&p).map_err(WebCanvasError::Document)?; preserve_web_canvas_preferences(&base, &mut next); set_file_name_display(&mut next, &p); next.editor_ui.touch_recent_file( @@ -1420,24 +1448,31 @@ fn startup_editor_for_web_canvas_with_loader( path: Option, _policy: WebCredentialPersistence, checked_load: Checked, -) -> Result +) -> Result where - Checked: FnOnce(&mut EditorState) -> Result<(), String>, + // `settings_io::load_checked` is outside this conversion's scope and + // still reports `String`; keep its shape and adapt at the call. + Checked: FnOnce(&mut EditorState) -> std::result::Result<(), String>, { let mut base = EditorState::starter(); - checked_load(&mut base)?; + checked_load(&mut base).map_err(WebCanvasError::Config)?; startup_editor_from_base_for_web_canvas(base, path) } fn startup_editor_for_web_canvas_with_policy( path: Option, policy: WebCredentialPersistence, -) -> Result { +) -> Result { startup_editor_for_web_canvas_with_loader(path, policy, crate::settings_io::load_checked) } -pub fn startup_editor_for_web_canvas(path: Option) -> Result { +/// Public entry point (host binaries) — keeps the `String` contract and +/// adapts the typed error at the boundary. +pub fn startup_editor_for_web_canvas( + path: Option, +) -> std::result::Result { startup_editor_for_web_canvas_with_policy(path, crate::web_credential_policy::from_env()) + .map_err(|e| e.to_string()) } /// Run the web-canvas daemon per `options` (host/port default `127.0.0.1`), @@ -1457,7 +1492,12 @@ pub fn startup_editor_for_web_canvas(path: Option) -> Result Result<(), String> { +pub fn run_web_canvas(options: ServeWebOptions) -> std::result::Result<(), String> { + // Public entry point (`cli_modes.rs`) — `String` contract preserved. + run_web_canvas_typed(options).map_err(|e| e.to_string()) +} + +fn run_web_canvas_typed(options: ServeWebOptions) -> Result<()> { let ServeWebOptions { port, path, @@ -1479,9 +1519,11 @@ pub fn run_web_canvas(options: ServeWebOptions) -> Result<(), String> { // to the daemon owner, not to whoever can reach the port. let loopback_bind = matches!(host.as_str(), "127.0.0.1" | "localhost" | "::1"); crate::web_auth::init(&mut editor, managed || loopback_bind); - let listener = - TcpListener::bind((host.as_str(), port)).map_err(|e| format!("bind {host}:{port}: {e}"))?; - let local_addr = listener.local_addr().map_err(|e| e.to_string())?; + let listener = TcpListener::bind((host.as_str(), port)) + .map_err(|e| WebCanvasError::Config(format!("bind {host}:{port}: {e}")))?; + let local_addr = listener + .local_addr() + .map_err(|e| WebCanvasError::Config(e.to_string()))?; let bound = local_addr.port(); eprintln!("openpencil-desktop --serve-web: listening on {host}:{bound}"); match crate::web_static::resolve_bundle_dir() { @@ -1521,17 +1563,36 @@ pub fn run_web_canvas(options: ServeWebOptions) -> Result<(), String> { let _ = out.flush(); drop(out); let shutdown_stdin = Arc::clone(&shutdown); - std::thread::spawn(move || { - let mut sink = [0u8; 64]; - let mut stdin = std::io::stdin(); - while matches!(stdin.read(&mut sink), Ok(n) if n > 0) {} - shutdown_stdin.store(true, Ordering::Release); - // Wake the (possibly blocked) accept loop — reconnect to the - // bound address exactly (works for IPv6 / custom --host, unlike - // the loopback-only wake used by the token-authed shutdown - // path below). - let _ = std::net::TcpStream::connect(local_addr); - }); + // Detached on purpose — there is NO portable way to cancel a thread + // parked in a blocking `Stdin::read`. A channel or flag can only be + // observed between reads, and putting fd 0 into non-blocking mode + // would need platform `fcntl`/`SetNamedPipeHandleState` calls (a new + // dependency or unsafe per-OS code) AND would change what "EOF" means + // for the parent-death lease, which is this thread's whole purpose. + // So the exit path is: (a) the parent closes stdin — the loop ends and + // raises `shutdown` itself, or (b) some other path raised `shutdown` + // first, in which case the checks below make this thread a no-op and + // the process exit reaps it. The flag check per iteration is what + // makes (b) prompt rather than "whenever the parent next writes". + let _ = std::thread::Builder::new() + .name("op-serve-web-stdin".into()) + .spawn(move || { + let mut sink = [0u8; 64]; + let mut stdin = std::io::stdin(); + while !shutdown_stdin.load(Ordering::Acquire) + && matches!(stdin.read(&mut sink), Ok(n) if n > 0) + {} + // Only raise + wake when nobody else already shut the daemon + // down; a redundant wake connect against an already-closed + // listener is harmless but pointlessly noisy. + if !shutdown_stdin.swap(true, Ordering::AcqRel) { + // Wake the (possibly blocked) accept loop — reconnect to + // the bound address exactly (works for IPv6 / custom + // --host, unlike the loopback-only wake used by the + // token-authed shutdown path below). + let _ = std::net::TcpStream::connect(local_addr); + } + }); } // Stash the managed token + allow-origins on the shared state. `serve_one` // reads them via `RequestAuth` gate (token) and `cors_origin_for` (CORS @@ -1596,16 +1657,20 @@ fn enforce_credential_persistence_policy( editor: &mut EditorState, policy: WebCredentialPersistence, save: F, -) -> Result<(), String> +) -> Result<()> where - F: FnOnce(&EditorState) -> Result<(), String>, + // `settings_io::save_checked`'s `String` shape is preserved (unowned); + // only the outcome is retyped. + F: FnOnce(&EditorState) -> std::result::Result<(), String>, { if !policy.server_persistence() && crate::web_credentials::remove_browser_owned_credentials(editor) { save(editor).map_err(|_| { - "failed to remove browser-owned credentials while server persistence is disabled" - .to_string() + WebCanvasError::Config( + "failed to remove browser-owned credentials while server persistence is disabled" + .into(), + ) })?; } Ok(()) @@ -1678,8 +1743,8 @@ fn serve_one( stream: &mut S, state: &Mutex, hub: &SseHub, -) -> Result { - let req = crate::mcp_serve::read_http_request(stream)?; +) -> Result { + let req = crate::mcp_serve::read_http_request(stream).map_err(WebCanvasError::Transport)?; let (auth, allow_origins) = { let guard = state.lock().unwrap_or_else(|p| p.into_inner()); let auth = RequestAuth { @@ -1701,6 +1766,7 @@ fn serve_one( "", cors_origin, ) + .map_err(WebCanvasError::Transport) .map(|()| false); } if is_sensitive_browser_post(&req) && !credential_request_origin_allowed(&req) { @@ -1710,6 +1776,7 @@ fn serve_one( &crate::mcp_serve::rest_error_body("cross-origin sensitive request is forbidden"), cors_origin, ) + .map_err(WebCanvasError::Transport) .map(|()| false); } // Sensitive JSON routes refuse CORS "simple request" content types @@ -1724,6 +1791,7 @@ fn serve_one( ), cors_origin, ) + .map_err(WebCanvasError::Transport) .map(|()| false); } // Static serving: the host page (`/`) and the wasm-bindgen bundle @@ -1734,6 +1802,7 @@ fn serve_one( crate::web_static::handle_static_request(&req.path, bundle_dir.as_deref()) { return crate::web_static::write_static_response(stream, &reply, cors_origin) + .map_err(WebCanvasError::Transport) .map(|()| false); } } @@ -1746,6 +1815,7 @@ fn serve_one( body: crate::web_auth::LOADING_PAGE_HTML.as_bytes().to_vec(), }; return crate::web_static::write_static_response(stream, &reply, cors_origin) + .map_err(WebCanvasError::Transport) .map(|()| false); } // Managed-mode token gate: everything below this point is a privileged @@ -1760,6 +1830,7 @@ fn serve_one( r#"{"ok":false,"error":"unauthorized"}"#, cors_origin, ) + .map_err(WebCanvasError::Transport) .map(|()| false); } // Device-login begin: waits (per-connection thread, off the state @@ -1774,6 +1845,7 @@ fn serve_one( &reply.body, cors_origin, ) + .map_err(WebCanvasError::Transport) .map(|()| false); } // SSE live-update stream: the browser shell subscribes and re-syncs whenever @@ -1795,7 +1867,7 @@ fn serve_one( if req.method == "POST" && req.path == "/api/ai/stream" { let Some(ai_req) = crate::ai_proxy::parse_ai_stream_body(&req.body) else { return crate::ai_proxy::write_sse_error(stream, "invalid request body", cors_origin) - .map_err(|e| format!("ai stream error: {e}")) + .map_err(|e| WebCanvasError::Transport(format!("ai stream error: {e}"))) .map(|()| false); }; let provider = { @@ -1814,17 +1886,17 @@ fn serve_one( "no model configured", cors_origin, ) - .map_err(|e| format!("ai stream error: {e}")) + .map_err(|e| WebCanvasError::Transport(format!("ai stream error: {e}"))) .map(|()| false); } Err(message) => { return crate::ai_proxy::write_sse_error(stream, &message, cors_origin) - .map_err(|e| format!("ai stream error: {e}")) + .map_err(|e| WebCanvasError::Transport(format!("ai stream error: {e}"))) .map(|()| false); } }; return crate::ai_proxy::stream_ai_response(stream, ai_req, provider.as_ref(), cors_origin) - .map_err(|e| format!("ai stream: {e}")) + .map_err(|e| WebCanvasError::Transport(format!("ai stream: {e}"))) .map(|()| false); } // Standard web chat/design turn: same external-CLI routing shape as @@ -1834,7 +1906,7 @@ fn serve_one( let Some(standard_req) = crate::web_chat_standard::parse_standard_turn_body(&req.body) else { return crate::ai_proxy::write_sse_error(stream, "invalid request body", cors_origin) - .map_err(|e| format!("ai standard error: {e}")) + .map_err(|e| WebCanvasError::Transport(format!("ai standard error: {e}"))) .map(|()| false); }; return crate::web_chat_standard::stream_standard_turn( @@ -1844,7 +1916,7 @@ fn serve_one( hub, cors_origin, ) - .map_err(|e| format!("ai standard: {e}")) + .map_err(|e| WebCanvasError::Transport(format!("ai standard: {e}"))) .map(|()| false); } // Image panel Search popover (desktop `image_panel_host` parity). Long @@ -1891,6 +1963,7 @@ fn serve_one( &body, cors_origin, ) + .map_err(WebCanvasError::Transport) .map(|()| false); } // Image panel Generate popover (desktop `image_generate_host` parity). @@ -1927,6 +2000,7 @@ fn serve_one( &body, cors_origin, ) + .map_err(WebCanvasError::Transport) .map(|()| false); } // Offline `.fig` -> `.op` convert for the VS Code plugin: it can't parse @@ -1949,6 +2023,7 @@ fn serve_one( &body, cors_origin, ) + .map_err(WebCanvasError::Transport) .map(|()| false); } // All `/api/mcp/*` REST paths go to the REST handler — including ones this @@ -1988,6 +2063,7 @@ fn serve_one( &reply.body, cors_origin, ) + .map_err(WebCanvasError::Transport) .map(|()| false); } // JSON-RPC tool dispatch is served ONLY as a POST to `/` or `/mcp`. An @@ -2001,6 +2077,7 @@ fn serve_one( r#"{"ok":false,"error":"Not found. Use /, /pkg/*, /api/mcp/document, /api/mcp/sync-reset, /api/mcp/server, /api/mcp/events, /api/file/save, /api/export/raster, /api/export/pdf, or /mcp."}"#, cors_origin, ) + .map_err(WebCanvasError::Transport) .map(|()| false); } if req.method != "POST" { @@ -2010,6 +2087,7 @@ fn serve_one( r#"{"ok":false,"error":"Method not allowed. POST a JSON-RPC message to /mcp."}"#, cors_origin, ) + .map_err(WebCanvasError::Transport) .map(|()| false); } // Token-authed graceful shutdown (`op stop`): same contract as the @@ -2025,7 +2103,8 @@ fn serve_one( "200 OK", &crate::mcp_serve::shutdown_ok_response(&id), cors_origin, - )?; + ) + .map_err(WebCanvasError::Transport)?; return Ok(true); } // `debug_screenshot` for `--serve-web`: the browser shell mirrors this @@ -2051,6 +2130,7 @@ fn serve_one( &response, cors_origin, ) + .map_err(WebCanvasError::Transport) .map(|()| false); } // JSON-RPC `/mcp` dispatch against the in-memory document. A mutating apply @@ -2075,7 +2155,8 @@ fn serve_one( applied_any |= ok; ok }, - )? + ) + .map_err(WebCanvasError::BadRequest)? .unwrap_or_default(); if applied_any { guard.version += 1; @@ -2093,6 +2174,7 @@ fn serve_one( "200 OK" }; crate::mcp_serve::write_mcp_http_response_with_origin(stream, status, &response, cors_origin) + .map_err(WebCanvasError::Transport) .map(|()| false) } @@ -2218,7 +2300,7 @@ fn serve_sse( rx: Receiver, current_version: u64, cors_origin: Option<&str>, -) -> Result<(), String> { +) -> Result<()> { let cors_line = cors_origin .map(|origin| format!("Access-Control-Allow-Origin: {origin}\r\n")) .unwrap_or_default(); @@ -2231,7 +2313,7 @@ fn serve_sse( ); stream .write_all(headers.as_bytes()) - .map_err(|e| format!("sse headers: {e}"))?; + .map_err(|e| WebCanvasError::Transport(format!("sse headers: {e}")))?; write_sse_event(stream, current_version)?; loop { match rx.recv_timeout(SSE_HEARTBEAT) { @@ -2250,8 +2332,10 @@ fn serve_sse( // write here is how we notice it disconnected. stream .write_all(b": ping\n\n") - .map_err(|e| format!("sse heartbeat: {e}"))?; - stream.flush().map_err(|e| format!("sse flush: {e}"))?; + .map_err(|e| WebCanvasError::Transport(format!("sse heartbeat: {e}")))?; + stream + .flush() + .map_err(|e| WebCanvasError::Transport(format!("sse flush: {e}")))?; } Err(RecvTimeoutError::Disconnected) => return Ok(()), } @@ -2259,12 +2343,14 @@ fn serve_sse( } /// Format + write one SSE `data:` event carrying the document version. -fn write_sse_event(stream: &mut S, version: u64) -> Result<(), String> { +fn write_sse_event(stream: &mut S, version: u64) -> Result<()> { let event = format!("data: {{\"version\":{version}}}\n\n"); stream .write_all(event.as_bytes()) - .map_err(|e| format!("sse write: {e}"))?; - stream.flush().map_err(|e| format!("sse flush: {e}")) + .map_err(|e| WebCanvasError::Transport(format!("sse write: {e}")))?; + stream + .flush() + .map_err(|e| WebCanvasError::Transport(format!("sse flush: {e}"))) } #[cfg(test)] diff --git a/crates/op-host-services/src/web_canvas_server_error.rs b/crates/op-host-services/src/web_canvas_server_error.rs new file mode 100644 index 000000000..fa1f12f35 --- /dev/null +++ b/crates/op-host-services/src/web_canvas_server_error.rs @@ -0,0 +1,79 @@ +//! Typed failures for the `--serve-web` web-canvas daemon +//! (`web_canvas_server.rs`). +//! +//! Style follows `op_orchestrator::OrchestratorError`: a plain enum plus a +//! hand-written `Display`, no `thiserror` and no new dependency. `Display` is +//! transparent — each variant carries the exact sentence the route already +//! embedded in its JSON error body, so the wire bytes are unchanged. +//! +//! What the enum adds is a route-independent classification plus +//! [`WebCanvasError::http_status`], which turns "which status does this +//! failure answer with" from a per-call-site literal into one table. +//! +//! Several dependencies of this module (`mcp_serve`, `doc_io`, `export`, +//! `settings_io`, `op_pen_loader`) still report `String`; they are outside +//! this conversion's scope, so their errors are adapted into a variant at the +//! call site instead of rippling the change into them. The daemon's three +//! public entry points (`parse_serve_web_args`, `startup_editor_for_web_canvas`, +//! `run_web_canvas`) likewise keep their `Result<_, String>` signatures — they +//! are consumed by `cli_modes.rs` and the host binaries — and convert at the +//! boundary. + +use std::fmt; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum WebCanvasError { + /// The request body is malformed, has the wrong shape, or names an + /// unsupported parameter — a client fault. + BadRequest(String), + /// A document failed to load or validate through the canonical loader. + Document(String), + /// The raster / PDF export pipeline failed. + Export(String), + /// A filesystem operation on a request-scoped file (the save target, an + /// export temp file, the backing document) failed. + Io(String), + /// Start-up configuration failed: bad `--serve-web` argv, an unusable + /// settings file, or a socket that would not bind. Never becomes an HTTP + /// response — it aborts daemon start-up. + Config(String), + /// A connection-level read/write failed (request parse, response write, + /// SSE stream). Never becomes an HTTP response — the socket is already + /// unusable; the accept loop just logs it. + Transport(String), +} + +impl WebCanvasError { + /// The HTTP status a route answers with when it surfaces this failure. + /// + /// The four request-scoped kinds are all client faults — the daemon's + /// in-memory document authority is healthy; what failed is the request's + /// payload or the file it named — so they answer `400`, matching the + /// pre-conversion behaviour of every route in this module exactly. + /// `Config` / `Transport` never reach a response; they report the generic + /// `500` so a future caller that does surface one is not silently wrong. + pub(crate) fn http_status(&self) -> &'static str { + match self { + WebCanvasError::BadRequest(_) + | WebCanvasError::Document(_) + | WebCanvasError::Export(_) + | WebCanvasError::Io(_) => "400 Bad Request", + WebCanvasError::Config(_) | WebCanvasError::Transport(_) => "500 Internal Server Error", + } + } +} + +impl fmt::Display for WebCanvasError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + WebCanvasError::BadRequest(m) + | WebCanvasError::Document(m) + | WebCanvasError::Export(m) + | WebCanvasError::Io(m) + | WebCanvasError::Config(m) + | WebCanvasError::Transport(m) => f.write_str(m), + } + } +} + +impl std::error::Error for WebCanvasError {} diff --git a/crates/op-host-services/src/web_canvas_server_tests.rs b/crates/op-host-services/src/web_canvas_server_tests.rs index e6578bc8c..5abe72720 100644 --- a/crates/op-host-services/src/web_canvas_server_tests.rs +++ b/crates/op-host-services/src/web_canvas_server_tests.rs @@ -230,7 +230,8 @@ fn browser_only_startup_propagates_credential_scrub_save_failure() { crate::web_credential_policy::WebCredentialPersistence::BrowserOnly, |_| Err("simulated disk failure".into()), ) - .expect_err("startup must fail when scrubbed settings cannot be saved"); + .expect_err("startup must fail when scrubbed settings cannot be saved") + .to_string(); assert_eq!( error, @@ -478,7 +479,9 @@ fn every_web_persistence_policy_propagates_strict_settings_load_failures() { Err("invalid existing settings".into()) }); - let error = result.expect_err("all web policies must fail closed on invalid settings"); + let error = result + .expect_err("all web policies must fail closed on invalid settings") + .to_string(); assert_eq!(error, "invalid existing settings"); assert_eq!(checked_calls.get(), 1, "policy={policy:?}"); } diff --git a/crates/op-mcp/src/batch_program.rs b/crates/op-mcp/src/batch_program.rs index bdcb917e9..c83d2aad2 100644 --- a/crates/op-mcp/src/batch_program.rs +++ b/crates/op-mcp/src/batch_program.rs @@ -67,8 +67,12 @@ use super::batch_design::{ }; use super::batch_direct_ops::{split_top_level_args, update_command_from_value}; use super::batch_page::optional_page_id; +use super::batch_program_error::ProgramError; use super::{EditorCommand, ToolOutcome}; +/// Every fallible step of the executor fails with [`ProgramError`]. +type Result = std::result::Result; + /// Run a mixed multi-op DSL program against the document snapshot and /// return the TS `handleBatchDesign` envelope: /// `{ results, nodeCount, postProcessed?, errors? }`. @@ -120,7 +124,7 @@ pub(crate) fn run_batch_design_program( for (line_index, line) in lines.into_iter().enumerate() { ctx.current_line = line_index; if let Err(error) = execute_line(&line, &mut ctx) { - errors.push(json!({ "line": line_preview(&line), "error": error })); + errors.push(json!({ "line": line_preview(&line), "error": error.to_string() })); } } @@ -189,9 +193,9 @@ struct ProgramCtx { impl ProgramCtx { /// Emit `cmd` AND apply it to the sim. The sim apply is the line's /// final validation gate — the host will run the same code. - fn emit(&mut self, cmd: EditorCommand, failure: &str) -> Result<(), String> { + fn emit(&mut self, cmd: EditorCommand, failure: &str) -> Result<()> { if !self.sim.apply(cmd.clone()) { - return Err(failure.to_string()); + return Err(ProgramError::ApplyRejected(failure.to_string())); } self.commands.push(cmd); Ok(()) @@ -218,14 +222,14 @@ impl ProgramCtx { /// Assign fresh sim-allocator ids to `nodes` (in place); returns /// the (authored → final) map. Authored ids are recorded into the /// alias table so slash paths keep resolving TS-style. - fn remap(&mut self, nodes: &mut [PenNode]) -> Result, String> { + fn remap(&mut self, nodes: &mut [PenNode]) -> Result> { let mut seed = self .sim .next_node_id_seed() - .ok_or("node id space exhausted")?; + .ok_or(ProgramError::IdSpaceExhausted)?; let mut taken = self.sim.collect_node_ids(); let map = remap_subtree_ids_mapping(nodes, &mut seed, &mut taken) - .ok_or("node id space exhausted")?; + .ok_or(ProgramError::IdSpaceExhausted)?; for (old, new) in &map { if !old.starts_with("__op_tmp_") { self.alias.entry(old.clone()).or_insert_with(|| new.clone()); @@ -236,7 +240,7 @@ impl ProgramCtx { } /// TS `executeLine` — one DSL operation. -fn execute_line(line: &str, ctx: &mut ProgramCtx) -> Result<(), String> { +fn execute_line(line: &str, ctx: &mut ProgramCtx) -> Result<()> { // TS line grammar (dotAll `s` flag — pretty-printed JSON bodies // carry literal newlines inside the arg list): // binding=OP(args) for I/C/K/R/M/G @@ -272,7 +276,7 @@ fn execute_line(line: &str, ctx: &mut ProgramCtx) -> Result<(), String> { _ => unreachable!(), }; } - Err(format!("Cannot parse operation: {line}")) + Err(ProgramError::UnparsableLine(line.to_string())) } /// Return append-G line indexes that are robustly sized by later U() calls in @@ -368,7 +372,7 @@ fn positive_json_number(value: &Value) -> bool { .is_some_and(|number| number.is_finite() && number > 0.0) } -fn execute_assign(op: &str, binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<(), String> { +fn execute_assign(op: &str, binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<()> { match op { "I" => execute_insert(binding, args, ctx), "C" => execute_copy(binding, args, ctx), @@ -385,8 +389,9 @@ fn execute_assign(op: &str, binding: &str, args: &str, ctx: &mut ProgramCtx) -> } /// `binding=I(parent, data)` — insert a (possibly nested) node. -fn execute_insert(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<(), String> { - let comma = find_top_level_char(args, ',').ok_or("Insert requires parent and node data")?; +fn execute_insert(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<()> { + let comma = find_top_level_char(args, ',') + .ok_or_else(|| ProgramError::Syntax("Insert requires parent and node data".into()))?; let parent_raw = args[..comma].trim(); let parent = resolve_parent_ref(parent_raw, &ctx.bindings); let mut node = parse_node_json(&args[comma + 1..], ctx.post_process)?; @@ -419,7 +424,7 @@ fn execute_insert(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<(), let root_id = map .first() .map(|(_, new)| new.clone()) - .ok_or("Insert produced no node")?; + .ok_or(ProgramError::ProducedNoNode("Insert"))?; for (cmd, failure) in pre_commands { ctx.emit(cmd, failure)?; } @@ -461,8 +466,9 @@ fn execute_insert(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<(), } /// `binding=C(sourceId, parent[, overrides])` — clone with fresh ids. -fn execute_copy(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<(), String> { - let first = find_top_level_char(args, ',').ok_or("Copy requires sourceId, parent, and data")?; +fn execute_copy(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<()> { + let first = find_top_level_char(args, ',') + .ok_or_else(|| ProgramError::Syntax("Copy requires sourceId, parent, and data".into()))?; let source_raw = args[..first].trim(); let rest = args[first + 1..].trim(); let (parent_raw, data_str) = match find_top_level_char(rest, ',') { @@ -473,15 +479,19 @@ fn execute_copy(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<(), S let Some(source) = op_editor_core::walkers::find_node(ctx.sim.active_children(), &NodeId::new(&source_id)) else { - return Err(format!("Copy source not found: {source_id}")); + return Err(ProgramError::NotFound(format!( + "Copy source not found: {source_id}" + ))); }; - let mut cloned_value = - serde_json::to_value(source).map_err(|e| format!("Copy source unserializable: {e}"))?; + let mut cloned_value = serde_json::to_value(source) + .map_err(|e| ProgramError::InvalidNode(format!("Copy source unserializable: {e}")))?; // TS `Object.assign(cloned, data)` minus `id` (never overridden) // and `descendants` (a TS no-op — see module docs). let overrides = parse_json_arg(data_str)?; let Some(overrides) = overrides.as_object() else { - return Err("C() overrides JSON must be an object".into()); + return Err(ProgramError::Syntax( + "C() overrides JSON must be an object".into(), + )); }; if let Some(obj) = cloned_value.as_object_mut() { for (key, value) in overrides { @@ -492,8 +502,9 @@ fn execute_copy(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<(), S } } normalize_node_shape(&mut cloned_value); - let mut node: PenNode = serde_json::from_value(cloned_value) - .map_err(|e| format!("C() overrides produce an invalid node: {e}"))?; + let mut node: PenNode = serde_json::from_value(cloned_value).map_err(|e| { + ProgramError::InvalidNode(format!("C() overrides produce an invalid node: {e}")) + })?; if ctx.post_process { let _ = op_editor_core::command_refine::refine_subtree(&mut node); } @@ -508,7 +519,7 @@ fn execute_copy(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<(), S let clone_id = map .first() .map(|(_, new)| new.clone()) - .ok_or("Copy produced no node")?; + .ok_or(ProgramError::ProducedNoNode("Copy"))?; let parent = resolve_parent_ref(parent_raw, &ctx.bindings); ctx.emit( EditorCommand::InsertAuthoredSubtree { @@ -532,10 +543,12 @@ fn execute_copy(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<(), S /// `shadcn-` (for example `shadcn/btn-primary` → /// `shadcn-ui` / `shadcn-btn-primary`). Exact `/` /// pairs are also accepted for imported/future kits. -fn execute_kit_instantiate(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<(), String> { +fn execute_kit_instantiate(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<()> { let parts = split_top_level_args(args); if !(2..=3).contains(&parts.len()) { - return Err("K() requires kitComponentId, parent, and optional overrides".into()); + return Err(ProgramError::Syntax( + "K() requires kitComponentId, parent, and optional overrides".into(), + )); } let kit_component_id = parse_string_arg(parts[0].trim(), "K() kitComponentId")?; let (kit_id, component_id) = resolve_kit_component_id(&kit_component_id, &ctx.sim)?; @@ -550,7 +563,9 @@ fn execute_kit_instantiate(binding: &str, args: &str, ctx: &mut ProgramCtx) -> R Some(raw) => { let value = parse_json_arg(raw)?; if !value.is_object() { - return Err("K() overrides JSON must be an object".into()); + return Err(ProgramError::Syntax( + "K() overrides JSON must be an object".into(), + )); } Some(value.to_string()) } @@ -571,7 +586,9 @@ fn execute_kit_instantiate(binding: &str, args: &str, ctx: &mut ProgramCtx) -> R if !node_id.is_real() || op_editor_core::walkers::find_node(ctx.sim.active_children(), &node_id).is_none() { - return Err("K() did not produce a selected node".into()); + return Err(ProgramError::Rejected( + "K() did not produce a selected node".into(), + )); } ctx.bind(binding, node_id.as_str()); Ok(()) @@ -579,12 +596,15 @@ fn execute_kit_instantiate(binding: &str, args: &str, ctx: &mut ProgramCtx) -> R /// `binding=R(path, data)` — replace the node at `path` with a fresh /// node built from `data` (same slot, fresh id, children dropped). -fn execute_replace(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<(), String> { - let comma = find_top_level_char(args, ',').ok_or("Replace requires path and node data")?; +fn execute_replace(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<()> { + let comma = find_top_level_char(args, ',') + .ok_or_else(|| ProgramError::Syntax("Replace requires path and node data".into()))?; let path_raw = args[..comma].trim(); let path = resolve_path_expr(path_raw, &ctx.bindings); let Some(old) = find_node_by_path(ctx.sim.active_children(), &path, &ctx.alias) else { - return Err(format!("Replace target not found: {path}")); + return Err(ProgramError::NotFound(format!( + "Replace target not found: {path}" + ))); }; let old_id = old.id_str().to_string(); let mut node = parse_node_json(&args[comma + 1..], ctx.post_process)?; @@ -603,7 +623,7 @@ fn execute_replace(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<() let new_id = map .first() .map(|(_, new)| new.clone()) - .ok_or("Replace produced no node")?; + .ok_or(ProgramError::ProducedNoNode("Replace"))?; ctx.emit( EditorCommand::ReplaceSubtree { node_id: NodeId::new(&old_id), @@ -638,10 +658,10 @@ fn execute_replace(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<() /// hatch allows a new sibling only under a horizontal/vertical flow parent. /// No fetcher at this layer — `src` stays empty (browser-caller parity); /// the host's own image pipeline enriches later. -fn execute_image(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<(), String> { +fn execute_image(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<()> { let parts = split_top_level_args(args); if !matches!(parts.len(), 3 | 4) { - return Err(format!("Invalid G() syntax: {args}")); + return Err(ProgramError::Syntax(format!("Invalid G() syntax: {args}"))); } let parent_raw = parts[0].trim(); let parent = if matches!(parent_raw, "null" | "undefined" | "0" | "\"\"" | "\"0\"") { @@ -650,21 +670,23 @@ fn execute_image(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<(), resolve_path_expr(parent_raw, &ctx.bindings) }; let mode = serde_json::from_str::(parts[1].trim()) - .map_err(|_| format!("Invalid G() syntax: {args}"))?; + .map_err(|_| ProgramError::Syntax(format!("Invalid G() syntax: {args}")))?; if !matches!(mode.as_str(), "search" | "generate") { - return Err(format!("G() mode must be search or generate: {mode}")); + return Err(ProgramError::Rejected(format!( + "G() mode must be search or generate: {mode}" + ))); } let prompt = serde_json::from_str::(parts[2].trim()) - .map_err(|_| format!("Invalid G() syntax: {args}"))?; + .map_err(|_| ProgramError::Syntax(format!("Invalid G() syntax: {args}")))?; let placement = match parts.get(3) { None => "slot".to_string(), Some(raw) => serde_json::from_str::(raw.trim()) - .map_err(|_| format!("Invalid G() syntax: {args}"))?, + .map_err(|_| ProgramError::Syntax(format!("Invalid G() syntax: {args}")))?, }; if !matches!(placement.as_str(), "slot" | "append") { - return Err(format!( + return Err(ProgramError::Rejected(format!( "G() placement must be \"slot\" or \"append\", got {placement:?}" - )); + ))); } let name: String = prompt.chars().take(40).collect(); let mut value = json!({ @@ -682,18 +704,21 @@ fn execute_image(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<(), value["imageSearchQuery"] = json!(prompt); } if parent.trim().is_empty() || parent.trim() == "0" { - return Err(format!( + return Err(ProgramError::Rejected(format!( "G() placement {placement:?} requires an explicit frame/rectangle target id; create the target first instead of using null" - )); + ))); } - let target = find_node_by_path(ctx.sim.active_children(), &parent, &ctx.alias) - .ok_or_else(|| format!("G() parent not found or not a container: {parent}"))?; + let target = + find_node_by_path(ctx.sim.active_children(), &parent, &ctx.alias).ok_or_else(|| { + ProgramError::NotFound(format!("G() parent not found or not a container: {parent}")) + })?; // `parent` may be a slash path or an authored id that `find_node_by_path` // translated through `ctx.alias`. The emitted insert must target the live // resolved node id, never the caller's path/alias spelling. let target_id = target.id_str().to_string(); - let container = node_container(target) - .ok_or_else(|| format!("G() parent not found or not a container: {parent}"))?; + let container = node_container(target).ok_or_else(|| { + ProgramError::NotFound(format!("G() parent not found or not a container: {parent}")) + })?; // Placement is an explicit structural contract. Slot-fill accepts only an // EMPTY target; append accepts only an explicitly-authored flow parent. // Never recover intent from names, dimensions, child kinds, or position. @@ -706,29 +731,29 @@ fn execute_image(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<(), .map(PenNode::id_str) .collect::>(); if !child_ids.is_empty() { - return Err(format!( + return Err(ProgramError::Rejected(format!( "G() slot target {} must be empty, but it has children [{}]. Pass the exact empty frame/rectangle slot id; use \"append\" only for an intentional child of an explicit horizontal/vertical flow parent", target.id_str(), child_ids.join(", ") - )); + ))); } } "append" => { if explicit_flow_layout(container).is_none() { - return Err(format!( + return Err(ProgramError::Rejected(format!( "G() append target {} must declare layout \"horizontal\" or \"vertical\"; got {}. Append means a new flow sibling and is never an absolute overlay", target.id_str(), layout_label(container) - )); + ))); } if !ctx .explicitly_sized_append_lines .contains(&ctx.current_line) { - return Err( + return Err(ProgramError::Rejected( "G() append requires a result binding followed in the same batch by U(binding, {\"width\": , \"height\": }); refusing an unsized flow child with default fill_container width and height" .into(), - ); + )); } } _ => unreachable!("placement validated above"), @@ -741,14 +766,14 @@ fn execute_image(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<(), let width = target.width_px().or_else(|| resolved.map(|size| size.0)); let height = target.height_px().or_else(|| resolved.map(|size| size.1)); let (Some(width), Some(height)) = (width, height) else { - return Err(format!( + return Err(ProgramError::Rejected(format!( "G() target {target_id} uses layout none, so it needs declared width and height that resolve above zero before an image can fill it" - )); + ))); }; if width <= 0.0 || height <= 0.0 { - return Err(format!( + return Err(ProgramError::Rejected(format!( "G() target {target_id} uses layout none, so it needs declared width and height that resolve above zero before an image can fill it" - )); + ))); } value["x"] = json!(0); value["y"] = json!(0); @@ -761,14 +786,14 @@ fn execute_image(binding: &str, args: &str, ctx: &mut ProgramCtx) -> Result<(), value["width"] = json!("fill_container"); value["height"] = json!("fill_container"); } - let node: PenNode = - serde_json::from_value(value).map_err(|e| format!("invalid G() image node: {e}"))?; + let node: PenNode = serde_json::from_value(value) + .map_err(|e| ProgramError::InvalidNode(format!("invalid G() image node: {e}")))?; let mut nodes = vec![node]; let map = ctx.remap(&mut nodes)?; let image_id = map .first() .map(|(_, new)| new.clone()) - .ok_or("G() produced no node")?; + .ok_or(ProgramError::ProducedNoNode("G()"))?; ctx.emit( EditorCommand::InsertAuthoredSubtree { nodes, @@ -829,23 +854,29 @@ fn resolved_node_size(state: &EditorState, node_id: &str) -> Option<(f64, f64)> /// `U(path, data)` — shallow-patch the node at `path`. No result entry /// (TS call-form ops don't push results). -fn execute_update(args: &str, ctx: &mut ProgramCtx) -> Result<(), String> { - let comma = find_top_level_char(args, ',').ok_or("Update requires path and update data")?; +fn execute_update(args: &str, ctx: &mut ProgramCtx) -> Result<()> { + let comma = find_top_level_char(args, ',') + .ok_or_else(|| ProgramError::Syntax("Update requires path and update data".into()))?; let path = resolve_path_expr(args[..comma].trim(), &ctx.bindings); let mut value = parse_json_arg(&args[comma + 1..])?; normalize_node_shape(&mut value); let Some(target) = find_node_by_path(ctx.sim.active_children(), &path, &ctx.alias) else { - return Err(format!("Update target not found: {path}")); + return Err(ProgramError::NotFound(format!( + "Update target not found: {path}" + ))); }; let node_id = NodeId::new(target.id_str()); - let cmd = update_command_from_value(node_id, &value)?; + // `batch_direct_ops` is shared with the non-program write paths and is + // outside this conversion's scope; adapt its `String` at the boundary + // rather than rippling the change into it. + let cmd = update_command_from_value(node_id, &value).map_err(ProgramError::Rejected)?; let cmd = with_page_id(cmd, ctx.page_id.clone()); ctx.emit(cmd, &format!("Update failed for: {path}")) } /// `D(ref)` — delete. TS `removeNodeFromTree` silently no-ops on an /// unknown id: no error, no result. -fn execute_delete(args: &str, ctx: &mut ProgramCtx) -> Result<(), String> { +fn execute_delete(args: &str, ctx: &mut ProgramCtx) -> Result<()> { let raw = strip_outer_quotes(args.trim()); let node_id = lookup_id(&resolve_ref(&raw, &ctx.bindings), &ctx.alias); if op_editor_core::walkers::find_node(ctx.sim.active_children(), &NodeId::new(&node_id)) @@ -864,16 +895,20 @@ fn execute_delete(args: &str, ctx: &mut ProgramCtx) -> Result<(), String> { /// `M(nodeId, parent[, index])` (call or bound form) — reparent. /// Returns the moved node's id so the bound form can record it. -fn execute_move(args: &str, ctx: &mut ProgramCtx) -> Result { +fn execute_move(args: &str, ctx: &mut ProgramCtx) -> Result { let parts = split_top_level_args(args); if parts.len() < 2 { - return Err("Move requires nodeId and parent".into()); + return Err(ProgramError::Syntax( + "Move requires nodeId and parent".into(), + )); } let node_id = lookup_id(&resolve_ref(parts[0].trim(), &ctx.bindings), &ctx.alias); if op_editor_core::walkers::find_node(ctx.sim.active_children(), &NodeId::new(&node_id)) .is_none() { - return Err(format!("Move target not found: {node_id}")); + return Err(ProgramError::NotFound(format!( + "Move target not found: {node_id}" + ))); } let parent_raw = parts[1].trim(); let parent = resolve_parent_ref(parent_raw, &ctx.bindings); @@ -882,7 +917,11 @@ fn execute_move(args: &str, ctx: &mut ProgramCtx) -> Result { Some(raw) => Some( strip_outer_quotes(raw.trim()) .parse::() - .map_err(|_| format!("M() index must be a non-negative integer, got {raw:?}"))?, + .map_err(|_| { + ProgramError::Syntax(format!( + "M() index must be a non-negative integer, got {raw:?}" + )) + })?, ), }; ctx.emit( @@ -963,16 +1002,16 @@ fn find_node_with_parent<'a>( /// Parse + normalize an I()/R() node body into a `PenNode` with /// authored ids filled in (the caller remaps them to final ids). -fn parse_node_json(raw: &str, post_process: bool) -> Result { +fn parse_node_json(raw: &str, post_process: bool) -> Result { let mut value = parse_json_arg(raw)?; if !value.is_object() { - return Err("node data must be a JSON object".into()); + return Err(ProgramError::Json("node data must be a JSON object".into())); } normalize_node_shape(&mut value); let mut tmp = 1usize; ensure_node_ids(&mut value, &mut tmp); - let mut node: PenNode = - serde_json::from_value(value).map_err(|e| format!("invalid PenNode payload: {e}"))?; + let mut node: PenNode = serde_json::from_value(value) + .map_err(|e| ProgramError::InvalidNode(format!("invalid PenNode payload: {e}")))?; if post_process { // TS postProcess hooks (emoji strip, unique ids, layout-child // position sanitize, screen-bounds clamp) — the deterministic @@ -985,7 +1024,7 @@ fn parse_node_json(raw: &str, post_process: bool) -> Result { /// TS `parseJsonArg` — strict JSON first, then the lenient agent-typo /// pipeline: quote unquoted keys, single→double quote delimiters, /// strip empty-key artifacts and trailing commas. -fn parse_json_arg(raw: &str) -> Result { +fn parse_json_arg(raw: &str) -> Result { let trimmed = raw.trim(); if let Ok(value) = serde_json::from_str::(trimmed) { return Ok(value); @@ -1039,24 +1078,24 @@ fn parse_json_arg(raw: &str) -> Result { .map_err(|e| { let snippet: String = raw.chars().take(300).collect(); let ellipsis = if raw.chars().count() > 300 { "..." } else { "" }; - format!("Failed to parse JSON ({e}): {snippet}{ellipsis}") + ProgramError::Json(format!("Failed to parse JSON ({e}): {snippet}{ellipsis}")) }) } -fn parse_string_arg(raw: &str, label: &str) -> Result { +fn parse_string_arg(raw: &str, label: &str) -> Result { let value = parse_json_arg(raw)?; value .as_str() .map(str::to_string) - .ok_or_else(|| format!("{label} must be a JSON string")) + .ok_or_else(|| ProgramError::Syntax(format!("{label} must be a JSON string"))) } -fn resolve_kit_component_id(raw: &str, state: &EditorState) -> Result<(String, String), String> { +fn resolve_kit_component_id(raw: &str, state: &EditorState) -> Result<(String, String)> { let Some((kit_part, component_part)) = raw.split_once('/') else { - return Err( + return Err(ProgramError::Syntax( "K() kitComponentId must be starter/, shadcn/, or /" .into(), - ); + )); }; let kit_id = match kit_part { "starter" => "openpencil-starter".to_string(), @@ -1069,17 +1108,19 @@ fn resolve_kit_component_id(raw: &str, state: &EditorState) -> Result<(String, S component_part.to_string() }; let Some(kit) = state.ui_kits.iter().find(|kit| kit.id == kit_id) else { - return Err(format!("K() kit not found: {kit_part}")); + return Err(ProgramError::NotFound(format!( + "K() kit not found: {kit_part}" + ))); }; if !kit .components .iter() .any(|component| component.id == component_id) { - return Err(format!( + return Err(ProgramError::NotFound(format!( "K() component not found: {raw} (resolved to {}/{})", kit.id, component_id - )); + ))); } Ok((kit.id.clone(), component_id)) } diff --git a/crates/op-mcp/src/batch_program_error.rs b/crates/op-mcp/src/batch_program_error.rs new file mode 100644 index 000000000..84cd0796d --- /dev/null +++ b/crates/op-mcp/src/batch_program_error.rs @@ -0,0 +1,63 @@ +//! Typed per-line failures for the `batch_design` DSL program executor +//! (`batch_program.rs`). +//! +//! Style follows `op_orchestrator::OrchestratorError`: a plain enum plus a +//! hand-written `Display`, no `thiserror` and no new dependency. Each +//! variant's `Display` reproduces the exact sentence the stringly-typed +//! executor produced, because those sentences ship verbatim to the model in +//! the envelope's `errors[]` array (and several tests assert on them). +//! +//! What the enum buys over `String` is the CLASSIFICATION: the executor — +//! and anything downstream, e.g. a future retry ladder in `program_gen.rs` — +//! can now tell a grammar mistake from a missing node from an id-space +//! exhaustion without pattern-matching prose. + +use std::fmt; + +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum ProgramError { + /// The line matched none of the DSL operation grammars. + UnparsableLine(String), + /// An operation's argument list is malformed, has the wrong arity, or a + /// scalar argument has the wrong JSON type. + Syntax(String), + /// A JSON argument could not be parsed even after the lenient + /// agent-typo repair pipeline, or parsed to the wrong JSON kind. + Json(String), + /// A body parsed as JSON but is not a valid `PenNode`. + InvalidNode(String), + /// A referenced node, path, kit, or component does not exist. + NotFound(String), + /// The operation parsed and resolved, but a structural / semantic rule + /// of the design protocol refuses it (placement contracts, sizing + /// requirements, layout preconditions). + Rejected(String), + /// The simulated apply refused the command — the host would refuse it + /// too, so the line cannot ship. + ApplyRejected(String), + /// An operation that must yield a node yielded none. The payload is the + /// operation label as it appears in the message (`Insert`, `Copy`, + /// `Replace`, `G()`). + ProducedNoNode(&'static str), + /// The document's node id space is exhausted, so no fresh id can be + /// minted for the remapped subtree. + IdSpaceExhausted, +} + +impl fmt::Display for ProgramError { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + ProgramError::UnparsableLine(line) => write!(f, "Cannot parse operation: {line}"), + ProgramError::Syntax(m) + | ProgramError::Json(m) + | ProgramError::InvalidNode(m) + | ProgramError::NotFound(m) + | ProgramError::Rejected(m) + | ProgramError::ApplyRejected(m) => f.write_str(m), + ProgramError::ProducedNoNode(op) => write!(f, "{op} produced no node"), + ProgramError::IdSpaceExhausted => f.write_str("node id space exhausted"), + } + } +} + +impl std::error::Error for ProgramError {}