From 8341959e5ece4a2ddf6bd49d4611515c1ac35f28 Mon Sep 17 00:00:00 2001 From: Bruno-Alumn <171815746+Bruno-Alumn@users.noreply.github.com> Date: Thu, 22 Oct 2026 16:22:43 +0200 Subject: [PATCH 01/23] MCP: render and screenshot PNGs go through a private temp folder (#2573) The render_photo, render_preview and screenshot tools had the backend write their PNG to lightcraft-mcp---.png right in the temp dir. On Linux and FreeBSD, where that is the shared /tmp, other users could read the rendered photos and screenshots, and a name someone else created first made the render fail. Each image now goes in a new lightcraft-mcp-- folder, made 0700 on Unix and never reused when the name already exists, and the folder is removed with anything left in it once the image is read or the call fails. On Windows, and on macOS with its usual per-user TMPDIR, the temp dir was already private; there the PNG just sits one folder deeper. --- crates/mcp/src/tests.rs | 63 ++++++++++++++++++++++++++++++++++++++++ crates/mcp/src/tools.rs | 64 +++++++++++++++++++++++++++++++++++------ 2 files changed, 118 insertions(+), 9 deletions(-) diff --git a/crates/mcp/src/tests.rs b/crates/mcp/src/tests.rs index 59e7ba3eb..7cc61a318 100644 --- a/crates/mcp/src/tests.rs +++ b/crates/mcp/src/tests.rs @@ -539,3 +539,66 @@ fn the_clipboard_tool_cuts_copies_and_pastes_in_the_app() { assert!(!r.is_error, "{r:?}"); assert_eq!(b.0, vec![("ui.clipboard".to_string(), json!({"action": "paste", "text": "travel"}))]); } + +/// A running app that writes the PNG `ui.render` / `ui.screenshot` ask for. It notes each path and +/// what its folder looked like then; with `dies_mid_write` it leaves a half-written temp file and +/// fails instead, like an app that quits part-way through. +struct PngWriter { + seen: Vec<(std::path::PathBuf, Option)>, + dies_mid_write: bool, +} + +impl Backend for PngWriter { + fn call(&mut self, method: &str, params: Value) -> Result { + assert!(matches!(method, "ui.render" | "ui.screenshot"), "{method}"); + let path = std::path::PathBuf::from(params["path"].as_str().unwrap()); + self.seen.push((path.clone(), path.parent().and_then(|d| std::fs::symlink_metadata(d).ok()))); + if self.dies_mid_write { + std::fs::write(path.with_file_name(".lightcraft-mcp-test.tmp"), b"half a png").unwrap(); + return Err("connection closed".into()); + } + crate::write_image(&path, &lightcraft_raster::Rgba8::new(1, 1), 92)?; + Ok(json!({"width": 1, "height": 1})) + } + fn has_ui(&self) -> bool { + true + } + fn describe(&self) -> String { + "png writer".into() + } +} + +/// Renders and screenshots reach the agent through a PNG the app writes. In a shared /tmp other +/// users could read it or take its predictable name first and make the render fail, so it goes in +/// a new folder of its own, private on Unix, that is gone afterwards even when the write fails +/// (#2573). +#[test] +fn render_and_screenshot_pngs_go_through_a_private_folder() { + let mut b = PngWriter { seen: vec![], dies_mid_write: false }; + for (tool, args) in [("render_photo", json!({"id": 1})), ("render_preview", json!({"id": 1})), ("screenshot", json!({}))] { + let r = call_tool(&mut b, tool, &args); + assert!(!r.is_error, "{tool}: {r:?}"); + assert_eq!(r.image().map(|(_, mime)| mime), Some("image/png"), "{tool}: {r:?}"); + } + b.dies_mid_write = true; + assert!(call_tool(&mut b, "screenshot", &json!({})).is_error); + assert_eq!(b.seen.len(), 4); + let temp = std::env::temp_dir(); + let mut folders = vec![]; + for (file, folder) in &b.seen { + let dir = file.parent().unwrap(); + let meta = folder.as_ref().unwrap(); + assert!(meta.is_dir() && !meta.file_type().is_symlink(), "{}", dir.display()); + assert_ne!(dir, temp, "{} is right in the shared temp dir", file.display()); + assert_eq!(dir.parent(), Some(temp.as_path()), "{}", file.display()); + assert!(dir.file_name().unwrap().to_string_lossy().starts_with("lightcraft-mcp-"), "{}", dir.display()); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + assert_eq!(meta.permissions().mode() & 0o777, 0o700, "{}", dir.display()); + } + assert!(!dir.exists(), "{} was left behind", dir.display()); + assert!(!folders.contains(&dir), "{} was used twice", dir.display()); + folders.push(dir); + } +} diff --git a/crates/mcp/src/tools.rs b/crates/mcp/src/tools.rs index 8f782bb9d..11c25f087 100644 --- a/crates/mcp/src/tools.rs +++ b/crates/mcp/src/tools.rs @@ -2,6 +2,7 @@ //! per command in the engine's command registry (`cmd_`), all forwarded to a //! [`Backend`] as control-channel methods. +use std::hash::{BuildHasher, Hasher, RandomState}; use std::sync::atomic::{AtomicU64, Ordering}; use serde_json::{Map, Value, json}; @@ -437,9 +438,48 @@ fn activate(b: &mut dyn Backend, args: &Value) -> Result<(), String> { Ok(()) } -fn temp_path(tag: &str) -> std::path::PathBuf { - static N: AtomicU64 = AtomicU64::new(0); - std::env::temp_dir().join(format!("lightcraft-mcp-{}-{}-{tag}.png", std::process::id(), N.fetch_add(1, Ordering::Relaxed))) +/// Where the backend writes a render or screenshot for us to read back: `.png` in a new +/// `lightcraft-mcp--` folder in the temp dir, private to this user on Unix (0700). +/// Straight in a shared /tmp, other users could read the PNG, and a name they took first made the +/// render fail (#2573). Dropping this removes the folder and whatever is in it. +struct TempPng { + dir: std::path::PathBuf, + file: std::path::PathBuf, +} + +impl TempPng { + fn new(tag: &str) -> Result { + static N: AtomicU64 = AtomicU64::new(0); + let temp = std::env::temp_dir(); + for _ in 0..100 { + let mut h = RandomState::new().build_hasher(); + h.write_u64(N.fetch_add(1, Ordering::Relaxed)); + let dir = temp.join(format!("lightcraft-mcp-{}-{:016x}", std::process::id(), h.finish())); + #[cfg(unix)] + let builder = { + use std::os::unix::fs::DirBuilderExt; + let mut b = std::fs::DirBuilder::new(); + b.mode(0o700); + b + }; + #[cfg(not(unix))] + let builder = std::fs::DirBuilder::new(); + // Never recursive: a name someone else made first fails and is not used. + match builder.create(&dir) { + Ok(()) => return Ok(Self { file: dir.join(format!("{tag}.png")), dir }), + Err(e) if e.kind() == std::io::ErrorKind::AlreadyExists => continue, + Err(e) => return Err(format!("cannot make a private temporary folder in {}: {e}", temp.display())), + } + } + Err(format!("cannot make a private temporary folder in {}: every name tried is taken", temp.display())) + } +} + +impl Drop for TempPng { + fn drop(&mut self) { + // Also removes the temp file a backend that died mid-write leaves next to the PNG. + let _ = std::fs::remove_dir_all(&self.dir); + } } /// Read a PNG the backend wrote, optionally downscale / re-encode as JPEG, and wrap it as image @@ -512,26 +552,32 @@ fn render_photo(b: &mut dyn Backend, args: &Value) -> ToolResult { }, }; let Some(id) = id else { return ToolResult::error("no active photo: import or select one, or pass `id`") }; - let file = temp_path("render"); - let path = file.to_string_lossy().to_string(); + let tmp = match TempPng::new("render") { + Ok(t) => t, + Err(e) => return ToolResult::error(e), + }; + let path = tmp.file.to_string_lossy().to_string(); let meta = match b.call("ui.render", json!({"id": id, "size": size, "path": path})) { Ok(m) => m, Err(e) => return ToolResult::error(e), }; let mut meta = meta; meta["id"] = json!(id); - image_result(&file, None, args.get("format").and_then(Value::as_str).unwrap_or("png"), args.get("path").and_then(Value::as_str), meta) + image_result(&tmp.file, None, args.get("format").and_then(Value::as_str).unwrap_or("png"), args.get("path").and_then(Value::as_str), meta) } fn screenshot(b: &mut dyn Backend, args: &Value) -> ToolResult { if let Err(e) = check_save_path(b, args) { return ToolResult::error(e); } - let file = temp_path("screenshot"); - let path = file.to_string_lossy().to_string(); + let tmp = match TempPng::new("screenshot") { + Ok(t) => t, + Err(e) => return ToolResult::error(e), + }; + let path = tmp.file.to_string_lossy().to_string(); match b.call("ui.screenshot", json!({"path": path})) { Ok(meta) => image_result( - &file, + &tmp.file, Some(args.get("maxSize").and_then(Value::as_u64).unwrap_or(1600).clamp(64, 8192) as u32), args.get("format").and_then(Value::as_str).unwrap_or("png"), args.get("path").and_then(Value::as_str), From 2b341bbdee6fa128320d8e7918360b9348dd12f9 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 21 Oct 2026 23:56:44 -0700 Subject: [PATCH 02/23] Control port: close gracefully so the peer reads the last reply (Windows reset) After rejecting a line the server closed its socket at once. A line the client sent after the junk then arrived at a closed socket, which answers with a reset, and on Windows the reset discards the replies the client has not read yet: the client saw os error 10053 / 10054 instead of the error explaining why. This is the Windows x64 failure of tcp_connection_is_closed_after_junk in #2377's CI. serve_stream now shuts down the write side (FIN), reads and drops what is left (at most 4 MiB, 500 ms), then closes. close_gracefully is taken from #1970 by LochLogic, split out so it can land ahead of the per-run token work; #1991 did not include it. The test sends its last line 100 ms after the junk and reads after another 100 ms, so it fails on every run without the fix on Windows (10053) instead of depending on timing. Co-Authored-By: LochLogic <252105752+LochLogic@users.noreply.github.com> Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01K93HTyCiwYvbRGaAkjPvXn --- apps/lightcraft/src/control_server.rs | 38 ++++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 4 deletions(-) diff --git a/apps/lightcraft/src/control_server.rs b/apps/lightcraft/src/control_server.rs index c7244c703..7ba1072a6 100644 --- a/apps/lightcraft/src/control_server.rs +++ b/apps/lightcraft/src/control_server.rs @@ -87,7 +87,7 @@ fn serve(stream: TcpStream, tx: &Sender, ctx: &egui::Context, id log::warn!("control connection closed: could not set idle read timeout: {e}"); return; } - serve_lines(&stream, &stream, |method, params| { + serve_stream(stream, |method, params| { let (req, rrx) = ControlRequest::new(method, params); let state = Arc::clone(&req.state); tx.send(req).ok()?; @@ -96,6 +96,31 @@ fn serve(stream: TcpStream, tx: &Sender, ctx: &egui::Context, id }); } +/// [`serve_lines`] on a socket, then close it so the peer reads every reply. +fn serve_stream(stream: TcpStream, handle: impl FnMut(String, Value) -> Option) { + serve_lines(&stream, &stream, handle); + close_gracefully(&stream); +} + +/// Close a connection the server is done with without destroying its last reply. A socket closed +/// while the peer's later lines are unread, or arrive after it, answers them with a reset, and a +/// reset can discard the replies the peer has not read yet (on Windows it does): the client sees +/// "connection reset" instead of why. Send FIN, then read and drop what is left (bounded in bytes +/// and time) before closing. From #1970 (LochLogic). +fn close_gracefully(stream: &TcpStream) { + let _ = stream.shutdown(std::net::Shutdown::Write); + let _ = stream.set_read_timeout(Some(Duration::from_millis(500))); + let mut sink = [0u8; 8192]; + let mut left = MAX_LINE; + let mut reader = stream; + while left > 0 { + match reader.read(&mut sink) { + Ok(0) | Err(_) => break, + Ok(n) => left = left.saturating_sub(n), + } + } +} + /// Wait for a request's reply. While the app has not picked the request up and `frames` (its /// frame counter) stands still for `no_frames`, the request is abandoned (it will not run late) /// and the reply says why; a request the app is running gets the whole `timeout`. @@ -495,12 +520,17 @@ mod tests { let addr = listener.local_addr().unwrap(); let server = std::thread::spawn(move || { let (s, _) = listener.accept().unwrap(); - let read = s.try_clone().unwrap(); - serve_lines(read, s, |_, _| Some(json!({"ok": true}))); + serve_stream(s, |_, _| Some(json!({"ok": true}))); }); let mut c = TcpStream::connect(addr).unwrap(); c.set_read_timeout(Some(Duration::from_secs(10))).unwrap(); - c.write_all(b"{\"id\":7,\"method\":\"x\"}\nhello\n{\"method\":\"y\"}\n").unwrap(); + c.write_all(b"{\"id\":7,\"method\":\"x\"}\nhello\n").unwrap(); + // A client that keeps sending after the junk: its next line reaches the server after the + // rejection. A socket closed by then answers that line with a reset, and on Windows the reset + // discarded the replies the client had not read yet (os error 10054). + std::thread::sleep(Duration::from_millis(100)); + c.write_all(b"{\"method\":\"y\"}\n").unwrap(); + std::thread::sleep(Duration::from_millis(100)); let mut text = String::new(); c.read_to_string(&mut text).unwrap(); // returns at EOF: the server closed the connection server.join().unwrap(); From e99fbea7f96404eb956088f2ed3e41d3613c80f6 Mon Sep 17 00:00:00 2001 From: insideTheFlux Date: Fri, 23 Oct 2026 11:59:47 -0700 Subject: [PATCH 03/23] Control port: bound the close drain in total; the reset test waits for the rejection close_gracefully read with a 500 ms timeout per read but no overall limit, so a local client sending one byte every few hundred milliseconds kept the drain (and its connection slot) going until it had sent MAX_LINE. Each read now waits at most the time left of DRAIN_TIME (1 s). New test a_closing_connection_is_not_held_open_by_a_trickling_peer: without the limit it drains for 10 s. tcp_connection_is_closed_after_junk no longer sleeps before its second write: it reads the reply and the rejection first, so the server has rejected the junk before the next line is sent. It still pauses before the last read so that a reset can arrive. Without close_gracefully it fails with os error 10053 on Windows. From CodeRabbit's review of #2723. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01PtjHGt5gnCHXgLt4NbGorf --- apps/lightcraft/src/control_server.rs | 62 +++++++++++++++++++++------ 1 file changed, 50 insertions(+), 12 deletions(-) diff --git a/apps/lightcraft/src/control_server.rs b/apps/lightcraft/src/control_server.rs index 7ba1072a6..275341183 100644 --- a/apps/lightcraft/src/control_server.rs +++ b/apps/lightcraft/src/control_server.rs @@ -96,6 +96,9 @@ fn serve(stream: TcpStream, tx: &Sender, ctx: &egui::Context, id }); } +/// The longest a closing connection is drained, however slowly its peer keeps sending. +const DRAIN_TIME: Duration = Duration::from_secs(1); + /// [`serve_lines`] on a socket, then close it so the peer reads every reply. fn serve_stream(stream: TcpStream, handle: impl FnMut(String, Value) -> Option) { serve_lines(&stream, &stream, handle); @@ -105,15 +108,21 @@ fn serve_stream(stream: TcpStream, handle: impl FnMut(String, Value) -> Option 0 { + // a peer that keeps trickling bytes must not hold the connection slot past DRAIN_TIME + let wait = DRAIN_TIME.saturating_sub(start.elapsed()).min(Duration::from_millis(500)); + if wait.is_zero() || stream.set_read_timeout(Some(wait)).is_err() { + break; + } match reader.read(&mut sink) { Ok(0) | Err(_) => break, Ok(n) => left = left.saturating_sub(n), @@ -525,18 +534,47 @@ mod tests { let mut c = TcpStream::connect(addr).unwrap(); c.set_read_timeout(Some(Duration::from_secs(10))).unwrap(); c.write_all(b"{\"id\":7,\"method\":\"x\"}\nhello\n").unwrap(); + let mut replies = BufReader::new(c.try_clone().unwrap()); + let (mut first, mut second) = (String::new(), String::new()); + replies.read_line(&mut first).unwrap(); + replies.read_line(&mut second).unwrap(); + assert!(first.contains("\"id\":7"), "{first}"); + assert!(second.contains("closing the connection"), "{second}"); // A client that keeps sending after the junk: its next line reaches the server after the - // rejection. A socket closed by then answers that line with a reset, and on Windows the reset - // discarded the replies the client had not read yet (os error 10054). - std::thread::sleep(Duration::from_millis(100)); + // rejection. A socket closed by then answers that line with a reset, which the client reads + // as "connection reset" (os error 10053 / 10054 on Windows) instead of the end of the + // replies. The pause lets a reset arrive before the read. c.write_all(b"{\"method\":\"y\"}\n").unwrap(); std::thread::sleep(Duration::from_millis(100)); - let mut text = String::new(); - c.read_to_string(&mut text).unwrap(); // returns at EOF: the server closed the connection + let mut rest = String::new(); + let read = replies.read_to_string(&mut rest); server.join().unwrap(); - let lines: Vec<&str> = text.lines().collect(); - assert_eq!(lines.len(), 2, "{text}"); - assert!(lines[0].contains("\"id\":7")); - assert!(lines[1].contains("closing the connection")); + assert!(read.is_ok(), "the close must be clean, not a reset: {read:?}"); + assert_eq!(rest, "", "nothing follows the rejection"); + } + + #[test] + fn a_closing_connection_is_not_held_open_by_a_trickling_peer() { + let listener = TcpListener::bind(("127.0.0.1", 0)).unwrap(); + let addr = listener.local_addr().unwrap(); + let done = Arc::new(std::sync::atomic::AtomicBool::new(false)); + let server_done = Arc::clone(&done); + let server = std::thread::spawn(move || { + let (s, _) = listener.accept().unwrap(); + let t0 = std::time::Instant::now(); + close_gracefully(&s); + server_done.store(true, Ordering::SeqCst); + t0.elapsed() + }); + let mut c = TcpStream::connect(addr).unwrap(); + // one byte every 100 ms: each read returns well within its 500 ms timeout + for _ in 0..100 { + if done.load(Ordering::SeqCst) || c.write_all(b"x").is_err() { + break; + } + std::thread::sleep(Duration::from_millis(100)); + } + let drained = server.join().unwrap(); + assert!(drained < DRAIN_TIME + Duration::from_secs(1), "drained for {drained:?}"); } } From 286154a17cfc0dac1b2bbec4e4a89c251f923493 Mon Sep 17 00:00:00 2001 From: NetterSilver <133535268+Silvertree2010@users.noreply.github.com> Date: Sun, 25 Oct 2026 17:16:44 +0000 Subject: [PATCH 04/23] Accept a UTF-8 BOM at the start of lightcraft-cli --script files Fixes #2937 Co-Authored-By: Claude Haiku 5.5 Co-Authored-By: Claude Sonnet 5.5 --- apps/lightcraft-cli/src/main.rs | 4 ++++ apps/lightcraft-cli/tests/it/cli.rs | 14 ++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/apps/lightcraft-cli/src/main.rs b/apps/lightcraft-cli/src/main.rs index a27e97beb..b2d3804b3 100644 --- a/apps/lightcraft-cli/src/main.rs +++ b/apps/lightcraft-cli/src/main.rs @@ -557,6 +557,8 @@ fn run(args: &[String]) -> Result<(), String> { } else { std::fs::read_to_string(path).map_err(|e| format!("{path}: {e}"))? }; + // Windows PowerShell 5.1 writes a UTF-8 BOM; it is not part of the first JSON line. + let text = text.strip_prefix('\u{feff}').unwrap_or(&text); for (n, line) in text.lines().enumerate().filter(|(_, l)| !l.trim().is_empty() && !l.trim_start().starts_with('#')) { let v: Value = serde_json::from_str(line).map_err(|e| format!("{path}:{}: {e}", n + 1))?; // `{"command": id, "params": …}` or a control request `{"method": m, "params": …}` @@ -851,6 +853,8 @@ fn snapshot(args: &[String]) -> Result<(), String> { let mut failed = 0usize; if let Some(script) = &script { let text = std::fs::read_to_string(script).map_err(|e| format!("{script}: {e}"))?; + // Windows PowerShell 5.1 writes a UTF-8 BOM; it is not part of the first JSON line. + let text = text.strip_prefix('\u{feff}').unwrap_or(&text); let mut out = std::io::stdout().lock(); for (n, line) in text.lines().enumerate() { let line = line.trim(); diff --git a/apps/lightcraft-cli/tests/it/cli.rs b/apps/lightcraft-cli/tests/it/cli.rs index 1c8910d9d..04a74560c 100644 --- a/apps/lightcraft-cli/tests/it/cli.rs +++ b/apps/lightcraft-cli/tests/it/cli.rs @@ -506,6 +506,20 @@ fn run_cli(args: &[&str], stdin: Option<&str>) -> (bool, Vec, String) { (o.status.success(), lines, String::from_utf8_lossy(&o.stderr).to_string()) } +/// Windows PowerShell 5.1 writes a UTF-8 BOM; `--script` (file and stdin) must accept it. +#[test] +fn run_script_accepts_a_utf8_bom() { + let text = "\u{feff}{\"command\": \"library.info\", \"params\": {}}\n"; + let script = tmp("bom-steps.jsonl"); + std::fs::write(&script, text).unwrap(); + let (ok, lines, err) = run_cli(&["--demo", "--script", script.to_str().unwrap()], None); + assert!(ok, "file: {err}"); + assert_eq!(lines.len(), 1, "{lines:?}"); + let (ok, lines, err) = run_cli(&["--demo", "--script", "-"], Some(text)); + assert!(ok, "stdin: {err}"); + assert_eq!(lines.len(), 1, "{lines:?}"); +} + #[test] fn run_subcommand_chains_commands_and_persists_a_library() { let input = tmp("run-in.png"); From 20854262305114829efa0bdf80cceca2053adbaf Mon Sep 17 00:00:00 2001 From: Andrew Frahn Date: Mon, 26 Oct 2026 11:13:34 +1100 Subject: [PATCH 05/23] CLI-HOSTILE: render --set refuses inf / NaN instead of dropping the value `inf`, `NaN` and `1e999` parse as f64 but serialize to JSON null, so the control was silently skipped and the photo rendered without the requested edit. They are now an error. Hostile-input check added to the render subcommand test. Co-Authored-By: Claude Opus 5.5 --- apps/lightcraft-cli/src/main.rs | 4 +++- apps/lightcraft-cli/tests/it/cli.rs | 6 ++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/apps/lightcraft-cli/src/main.rs b/apps/lightcraft-cli/src/main.rs index c1f9d258c..ec9ba14d1 100644 --- a/apps/lightcraft-cli/src/main.rs +++ b/apps/lightcraft-cli/src/main.rs @@ -683,7 +683,9 @@ fn render(args: &[String]) -> Result<(), String> { "--set" => { let kv = take_value(args, &mut i, "--set")?; let (k, v) = kv.split_once('=').ok_or_else(|| format!("--set expects control=value, got `{kv}`"))?; - let v: f64 = v.trim().parse().map_err(|_| format!("--set {k}: `{v}` is not a number"))?; + // `inf` / `NaN` parse as f64 but become JSON null, which the control would silently ignore. + let v: f64 = + v.trim().parse().ok().filter(|n: &f64| n.is_finite()).ok_or_else(|| format!("--set {k}: `{v}` is not a finite number"))?; values.insert(k.trim().to_string(), json!(v)); } "--settings" => settings = Some(read_settings(take_value(args, &mut i, "--settings")?)?), diff --git a/apps/lightcraft-cli/tests/it/cli.rs b/apps/lightcraft-cli/tests/it/cli.rs index 1c8910d9d..9faa0a14d 100644 --- a/apps/lightcraft-cli/tests/it/cli.rs +++ b/apps/lightcraft-cli/tests/it/cli.rs @@ -179,6 +179,12 @@ fn render_subcommand() { let o = Command::new(BIN).args(["render", input.to_str().unwrap(), "-o", out.to_str().unwrap(), "--set", "nope=1"]).output().unwrap(); assert!(!o.status.success()); assert!(String::from_utf8_lossy(&o.stderr).contains("unknown control")); + // Values that parse as f64 but are not finite fail cleanly too. + for bad in ["light.exposure=inf", "light.exposure=-inf", "light.exposure=NaN", "light.exposure=1e999"] { + let o = Command::new(BIN).args(["render", input.to_str().unwrap(), "-o", out.to_str().unwrap(), "--set", bad]).output().unwrap(); + assert!(!o.status.success(), "{bad} must be refused"); + assert!(String::from_utf8_lossy(&o.stderr).contains("not a finite number"), "{bad}"); + } } /// Issue #1730: `render --settings` took a `.lcpreset` from `preset.export`, ignored its wrapper From 0a76419c4ed4869a1a1d786dd648973e7b9c81b1 Mon Sep 17 00:00:00 2001 From: Chad Elliott Date: Mon, 26 Oct 2026 11:08:24 -0500 Subject: [PATCH 06/23] fix(mcp): #3069 resend a request that crossed the app's idle-close notice A request sent just as the app expires an idle control connection crosses the app's id-less idle-close notice: the client's pre-send check saw an open connection, wrote the request, and read the notice as its reply, so `--connect` and the MCP server failed a command the app never read. The app reads nothing after its idle timeout, so such a request never ran. The app now marks the notice `"closed": "idle"`, and `Remote::call` sends a request whose reply is that notice once more on a new connection (it also matches the notice's exact text, for apps from before the marker). At most one resend per call, whatever the reason; a reply with an id or any other error is returned as before. `idle_close_reconnects_without_replaying_a_command` now forces both orders (notice before the next request, and a request crossing it) instead of racing the notice; with main's client it fails every run. Co-Authored-By: Claude Opus 5.5 --- apps/lightcraft/src/control_server.rs | 46 ++++- crates/mcp/src/backend.rs | 231 ++++++++++++++++++++------ docs/control-protocol.md | 20 ++- docs/mcp.md | 4 +- 4 files changed, 231 insertions(+), 70 deletions(-) diff --git a/apps/lightcraft/src/control_server.rs b/apps/lightcraft/src/control_server.rs index 95aa10896..adc972312 100644 --- a/apps/lightcraft/src/control_server.rs +++ b/apps/lightcraft/src/control_server.rs @@ -170,15 +170,40 @@ fn parse_line(bytes: &[u8]) -> Line { } } +/// The error of the notice sent when an idle connection is closed. Clients match the reply's +/// `"closed": "idle"` marker; `crates/mcp/src/backend.rs` also matches this text, for apps from +/// before the marker, so keep the two in step. +const IDLE_CLOSE_ERROR: &str = "control connection idle read timed out; closing the connection"; + +/// Why the server closes a connection; its error reply says so. +#[derive(Debug, PartialEq)] +enum Closing { + /// Nothing complete arrived within the idle timeout. Nothing more is read, so a request that + /// crosses the notice is never run: the reply carries `"closed": "idle"` to tell clients it is + /// safe to send that request again on a new connection. + Idle, + /// Junk, an overlong line or a failed read. + Error(String), +} + +impl Closing { + fn reply(&self) -> Value { + match self { + Self::Idle => json!({"ok": false, "error": IDLE_CLOSE_ERROR, "closed": "idle"}), + Self::Error(e) => json!({"ok": false, "error": e}), + } + } +} + /// Read one `\n`-terminated line of at most `max` bytes into `buf` (newline stripped). /// `Ok(false)` at end of stream; `Err` when the line is too long or the read fails. -fn read_bounded_line(reader: &mut impl BufRead, buf: &mut Vec, max: usize) -> Result { +fn read_bounded_line(reader: &mut impl BufRead, buf: &mut Vec, max: usize) -> Result { buf.clear(); // `take` bounds the bytes buffered for one line: a peer can't make us allocate more than `max`. let limit = u64::try_from(max).unwrap_or(u64::MAX).saturating_add(1); let n = reader.take(limit).read_until(b'\n', buf).map_err(|e| match e.kind() { - std::io::ErrorKind::WouldBlock | std::io::ErrorKind::TimedOut => "control connection idle read timed out; closing the connection".to_string(), - _ => e.to_string(), + std::io::ErrorKind::WouldBlock | std::io::ErrorKind::TimedOut => Closing::Idle, + _ => Closing::Error(e.to_string()), })?; if n == 0 { return Ok(false); @@ -191,7 +216,7 @@ fn read_bounded_line(reader: &mut impl BufRead, buf: &mut Vec, max: usize) - return Ok(true); } if buf.len() > max { - return Err(format!("request line longer than {max} bytes; closing the connection")); + return Err(Closing::Error(format!("request line longer than {max} bytes; closing the connection"))); } // Last line without a newline, then end of stream. Ok(true) @@ -204,12 +229,12 @@ fn serve_lines(read: R, mut out: W, mut handle: impl FnMut(St let mut reader = BufReader::new(read); let mut buf = Vec::new(); loop { - let reject = match read_bounded_line(&mut reader, &mut buf, MAX_LINE) { + let closing = match read_bounded_line(&mut reader, &mut buf, MAX_LINE) { Ok(false) => return, - Err(e) => e, + Err(closing) => closing, Ok(true) => match parse_line(&buf) { Line::Blank => continue, - Line::Reject(e) => e, + Line::Reject(e) => Closing::Error(e), Line::Request { id, method, params } => { let Some(mut r) = handle(method, params) else { return }; if let Some(o) = r.as_object_mut() { @@ -222,7 +247,7 @@ fn serve_lines(read: R, mut out: W, mut handle: impl FnMut(St } }, }; - let _ = writeln!(out, "{}", json!({"ok": false, "error": reject})).and_then(|()| out.flush()); + let _ = writeln!(out, "{}", closing.reply()).and_then(|()| out.flush()); return; } } @@ -287,7 +312,9 @@ mod tests { assert!(connection.requests.try_recv().is_err(), "incomplete input never runs"); let reply: Value = serde_json::from_str(&reply).unwrap(); assert_eq!(reply["ok"], false); - assert!(reply["error"].as_str().unwrap().contains("idle read timed out")); + assert_eq!(reply["error"], IDLE_CLOSE_ERROR); + assert_eq!(reply["closed"], "idle", "clients may resend a request that crossed this notice"); + assert!(reply.get("id").is_none()); // A subsequent connection is admitted through the same accounting path. let mut next = Connection::new(&open, Duration::from_secs(1), |c| c.shutdown(std::net::Shutdown::Write).unwrap()); @@ -413,6 +440,7 @@ mod tests { assert_eq!(r.len(), 1); assert_eq!(r[0]["ok"], false); assert!(r[0]["error"].as_str().unwrap().contains("closing the connection")); + assert!(r[0].get("closed").is_none(), "only an idle close invites a resend: {}", r[0]); } #[test] diff --git a/crates/mcp/src/backend.rs b/crates/mcp/src/backend.rs index 6a2c2efb9..a621f4037 100644 --- a/crates/mcp/src/backend.rs +++ b/crates/mcp/src/backend.rs @@ -136,39 +136,58 @@ fn peer_closed(stream: &TcpStream) -> bool { stream.set_nonblocking(false).is_err() || !open } +/// The app's idle-close notice (`apps/lightcraft/src/control_server.rs`), matched when a reply lacks +/// the `closed` marker (an app from before it). Keep the two in step. +const IDLE_CLOSE_ERROR: &str = "control connection idle read timed out; closing the connection"; + +/// True for the app's idle-close notice: an id-less error the app writes when it closes a connection +/// on which it read nothing for its idle timeout. Read in reply to a request, it means the app closed +/// the connection without reading the request (the request crossed the notice), so nothing ran. +fn is_idle_close(reply: &Value) -> bool { + reply.get("id").is_none() + && reply.get("ok").and_then(Value::as_bool) == Some(false) + && (reply.get("closed").and_then(Value::as_str) == Some("idle") || reply.get("error").and_then(Value::as_str) == Some(IDLE_CLOSE_ERROR)) +} + impl Backend for Remote { fn call(&mut self, method: &str, params: Value) -> Result { let id = self.next_id; self.next_id += 1; let line = json!({"id": id, "method": method, "params": params}).to_string(); - // Retry once with a fresh connection (the app may have restarted), but only when the request - // never reached the app. After a full write the command may have run, so resending it could - // apply a mutation twice (toggle, undo, ...): report the unknown outcome instead. - let reply = match self.roundtrip(&line) { - Ok(r) => r, - Err((_, false)) => { - self.conn = None; - self.roundtrip(&line).map_err(|(e, sent)| { + // Send again once, on a fresh connection, but only when the request never reached the app: it + // could not be written (the app restarted), or the app's idle-close notice crossed it (the app + // stopped reading before it arrived). After a full write anything else may mean the command + // ran, so resending it could apply a mutation twice (toggle, undo, ...): report the unknown + // outcome instead. One resend at most, whatever the reason, so this can't loop. + let mut resent = false; + let v = loop { + let reply = match self.roundtrip(&line) { + Ok(r) => r, + Err((e, sent)) => { self.conn = None; if sent { - self.unknown_outcome(method, &e) - } else { - format!("LightCraft app at {} is not reachable: {e} (start it with `lightcraft --control PORT`)", self.addr) + return Err(self.unknown_outcome(method, &e)); } - })? - } - Err((e, true)) => { + if resent { + return Err(format!("LightCraft app at {} is not reachable: {e} (start it with `lightcraft --control PORT`)", self.addr)); + } + resent = true; + continue; + } + }; + let v = parse_reply(&reply, id).map_err(|e| { self.conn = None; - return Err(self.unknown_outcome(method, &e)); + self.unknown_outcome(method, &e) + })?; + if v.get("id").is_none() { + self.conn = None; + if !resent && is_idle_close(&v) { + resent = true; + continue; + } } + break v; }; - let v = parse_reply(&reply, id).map_err(|e| { - self.conn = None; - self.unknown_outcome(method, &e) - })?; - if v.get("id").is_none() { - self.conn = None; - } if v.get("ok").and_then(Value::as_bool) == Some(true) { Ok(v.get("result").cloned().unwrap_or(Value::Null)) } else { @@ -275,6 +294,11 @@ mod tests { ("{\"id\":1,\"ok\":true,\"result\":null}\r\n", Ok(Value::Null)), ("{\"id\":1,\"ok\":false,\"error\":\"unknown command\"}\n", Err("unknown command".to_string())), ("{\"ok\":false,\"error\":\"too many control connections\"}\n", Err("too many control connections".to_string())), + // The idle-close text in reply to the request (it has the id) is the command's own error. + ( + "{\"id\":1,\"ok\":false,\"error\":\"control connection idle read timed out; closing the connection\"}\n", + Err(IDLE_CLOSE_ERROR.to_string()), + ), ] { with_reply(reply.as_bytes(), |remote, result| { assert_eq!(result, expected); @@ -380,51 +404,148 @@ mod tests { assert_eq!(seen.load(Ordering::SeqCst), 2); } + /// Accept the next connection within 5 s, as a blocking stream with a 3 s read timeout. + fn accept(listener: &TcpListener) -> (BufReader, TcpStream) { + let deadline = std::time::Instant::now() + Duration::from_secs(5); + let stream = loop { + match listener.accept() { + Ok((stream, _)) => break stream, + Err(e) if e.kind() == std::io::ErrorKind::WouldBlock && std::time::Instant::now() < deadline => { + std::thread::sleep(Duration::from_millis(5)); + } + Err(e) => panic!("accept: {e}"), + } + }; + // BSD kernels (macOS, FreeBSD) let an accepted socket inherit the listener's non-blocking flag. + stream.set_nonblocking(false).unwrap(); + stream.set_read_timeout(Some(Duration::from_secs(3))).unwrap(); + (BufReader::new(stream.try_clone().unwrap()), stream) + } + + fn read_request(reader: &mut BufReader) -> Value { + let mut line = String::new(); + reader.read_line(&mut line).unwrap(); + serde_json::from_str(&line).unwrap() + } + + /// The app's idle-close notice, as `apps/lightcraft/src/control_server.rs` writes it. + fn idle_notice() -> Value { + json!({"ok": false, "error": IDLE_CLOSE_ERROR, "closed": "idle"}) + } + + /// Issue #3069. The app closes an idle connection with an id-less notice. Either the notice + /// arrives before the next request (the client sees the close and reconnects before sending), + /// or the request crosses it (sent just as the app stopped reading): the notice then comes + /// back as the request's reply although the app never ran it, so the client sends it once more + /// on a new connection. Both orders are forced here, so the test does not depend on timing. #[test] fn idle_close_reconnects_without_replaying_a_command() { + let _serial = crate::serial_sockets_and_children(); let listener = TcpListener::bind("127.0.0.1:0").unwrap(); listener.set_nonblocking(true).unwrap(); let addr = listener.local_addr().unwrap().to_string(); let (closed, expired) = std::sync::mpsc::channel(); let server = std::thread::spawn(move || { - let mut ids = Vec::new(); - for _ in 0..2 { - let deadline = std::time::Instant::now() + Duration::from_secs(5); - let stream = loop { - match listener.accept() { - Ok((stream, _)) => break stream, - Err(e) if e.kind() == std::io::ErrorKind::WouldBlock && std::time::Instant::now() < deadline => { - std::thread::sleep(Duration::from_millis(5)); - } - Err(e) => panic!("accept: {e}"), - } - }; - stream.set_read_timeout(Some(Duration::from_secs(3))).unwrap(); - let mut reader = BufReader::new(stream.try_clone().unwrap()); - let mut writer = stream; - let mut line = String::new(); - reader.read_line(&mut line).unwrap(); - let request: Value = serde_json::from_str(&line).unwrap(); - ids.push(request["id"].as_u64().unwrap()); - writeln!(writer, "{}", json!({"id": request["id"], "ok": true, "result": ids.len()})).unwrap(); - writer.set_read_timeout(Some(Duration::from_millis(50))).unwrap(); - line.clear(); - let error = reader.read_line(&mut line).unwrap_err(); - assert!(matches!(error.kind(), std::io::ErrorKind::WouldBlock | std::io::ErrorKind::TimedOut)); - assert!(line.is_empty(), "no command is resent while idle"); - writeln!(writer, "{}", json!({"ok": false, "error": "control connection idle read timed out; closing the connection"})).unwrap(); - drop(reader); - drop(writer); - closed.send(()).unwrap(); - } - ids + let mut ran = Vec::new(); + let mut run = |request: &Value, writer: &mut TcpStream| { + ran.push(request["id"].as_u64().unwrap()); + writeln!(writer, "{}", json!({"id": request["id"], "ok": true, "result": ran.len()})).unwrap(); + }; + + // 1. Idle with nothing sent: the notice (in an older app's form, without `closed`) and + // the close reach the client before its next request. + let (mut reader, mut writer) = accept(&listener); + run(&read_request(&mut reader), &mut writer); + writer.set_read_timeout(Some(Duration::from_millis(50))).unwrap(); + let mut line = String::new(); + let error = reader.read_line(&mut line).unwrap_err(); + assert!(matches!(error.kind(), std::io::ErrorKind::WouldBlock | std::io::ErrorKind::TimedOut)); + assert!(line.is_empty(), "no command is resent while idle"); + writeln!(writer, "{}", json!({"ok": false, "error": IDLE_CLOSE_ERROR})).unwrap(); + drop((reader, writer)); + closed.send(()).unwrap(); + + // 2. The client reconnected before sending. Its next request crosses the notice: the + // app's read timed out just before the request arrived, so it is not run. (Reading + // it here only shows that it was sent on this connection and keeps the close clean.) + let (mut reader, mut writer) = accept(&listener); + run(&read_request(&mut reader), &mut writer); + let crossed = read_request(&mut reader); + writeln!(writer, "{}", idle_notice()).unwrap(); + drop((reader, writer)); + + // 3. The crossed request, sent once more on a new connection, and nothing after it. + let (mut reader, mut writer) = accept(&listener); + let resent = read_request(&mut reader); + assert_eq!(resent, crossed, "the same request is sent again"); + run(&resent, &mut writer); + line.clear(); + assert_eq!(reader.read_line(&mut line).unwrap(), 0, "nothing else arrives: {line}"); + ran }); let mut remote = Remote::lazy(&addr); assert_eq!(remote.call("engine.execute", json!({"command": "edit.undo"})).unwrap(), json!(1)); expired.recv_timeout(Duration::from_secs(5)).unwrap(); + let deadline = std::time::Instant::now() + Duration::from_secs(5); + while !remote.conn.as_ref().is_some_and(|(_, writer)| peer_closed(writer)) { + assert!(std::time::Instant::now() < deadline, "the idle close never reached the client"); + std::thread::sleep(Duration::from_millis(1)); + } assert_eq!(remote.call("engine.execute", json!({"command": "edit.undo"})).unwrap(), json!(2)); - expired.recv_timeout(Duration::from_secs(5)).unwrap(); - assert_eq!(server.join().unwrap(), [1, 2], "each command ran once on a separate connection"); + assert_eq!(remote.call("engine.execute", json!({"command": "edit.undo"})).unwrap(), json!(3)); + drop(remote); + assert_eq!(server.join().unwrap(), [1, 2, 3], "each command ran once"); + } + + /// The resend after an idle close happens once: a second notice is the call's error. + #[test] + fn idle_close_is_resent_only_once() { + let _serial = crate::serial_sockets_and_children(); + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + listener.set_nonblocking(true).unwrap(); + let addr = listener.local_addr().unwrap().to_string(); + let server = std::thread::spawn(move || { + for _ in 0..2 { + let (mut reader, mut writer) = accept(&listener); + assert_eq!(read_request(&mut reader)["id"], 1); + writeln!(writer, "{}", idle_notice()).unwrap(); + } + listener + }); + let mut remote = Remote::lazy(&addr); + assert_eq!(remote.call("engine.execute", json!({"command": "edit.undo"})), Err(IDLE_CLOSE_ERROR.to_string())); + assert!(remote.conn.is_none(), "the closed connection is not reused"); + drop(remote); + let listener = server.join().unwrap(); + assert_eq!(listener.accept().unwrap_err().kind(), std::io::ErrorKind::WouldBlock, "a third connection was opened"); + } + + #[test] + fn only_the_idle_close_notice_counts_as_undelivered() { + let idle = [ + json!({"ok": false, "error": IDLE_CLOSE_ERROR, "closed": "idle"}), + // an app from before the `closed` marker + json!({"ok": false, "error": IDLE_CLOSE_ERROR}), + // the marker decides when present, whatever the wording + json!({"ok": false, "error": "idle, closing", "closed": "idle"}), + ]; + for reply in idle { + assert!(is_idle_close(&reply), "{reply}"); + } + let delivered = [ + // a reply to the request itself + json!({"id": 1, "ok": false, "error": IDLE_CLOSE_ERROR}), + json!({"id": 1, "ok": false, "error": IDLE_CLOSE_ERROR, "closed": "idle"}), + // other refusals and closes + json!({"ok": false, "error": "too many control connections"}), + json!({"ok": false, "error": "bad JSON; closing the connection"}), + json!({"ok": false, "error": "bad JSON; closing the connection", "closed": "junk"}), + json!({"ok": false, "error": format!("{IDLE_CLOSE_ERROR}.")}), + json!({"ok": true, "result": IDLE_CLOSE_ERROR, "closed": "idle"}), + ]; + for reply in delivered { + assert!(!is_idle_close(&reply), "{reply}"); + } } #[test] diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 36294c787..38e92190b 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -21,15 +21,25 @@ headless snapshots or `lightcraft-cli mcp` without `--connect`. A request that h Connections close after five minutes without incoming bytes while the server is waiting for input, including in the middle of an unfinished request line. This allows pauses between -automation commands while releasing abandoned connection slots. An idle read timeout gets an -`ok: false` error saying the connection is closing; incomplete input is never dispatched. +automation commands while releasing abandoned connection slots. An idle read timeout gets one +reply without an `id`, then the connection closes; incomplete input is never dispatched: + +```text +← {"ok": false, "error": "control connection idle read timed out; closing the connection", "closed": "idle"} +``` + This is a socket read inactivity limit, not a total connection lifetime: a peer that keeps sending bytes can keep it open. Waiting for an already-dispatched command's reply does not count as idle reading and retains the request timeout above. -Reconnect before sending another request after an idle close. MCP connect mode detects closed -connections before sending and reconnects; if a connection is lost after sending a request, -its outcome remains unknown and the client must not blindly replay it. +Reconnect before sending another request after an idle close. A request sent just as the +connection expires crosses the notice: the client reads the notice where it expects the reply. +The server reads nothing after its idle timeout, so that request never ran, and `"closed": "idle"` +(only on this notice) says it is safe to send it again, once, on a new connection. Apps from +before the marker send the same notice without it; match its exact `error` text for them. MCP +connect mode does both: it detects closed connections before sending and reconnects, and +resends a request that crossed the idle notice once. Any other lost connection after sending a +request leaves its outcome unknown, and the client must not blindly replay it. **Only requests are read.** Every line must be a JSON object with a string `method` (`id` and `params` are optional; blank lines are skipped). Anything else — text that isn't JSON, a JSON array or number, an object without diff --git a/docs/mcp.md b/docs/mcp.md index 4800c37d8..8dd6f0bcd 100644 --- a/docs/mcp.md +++ b/docs/mcp.md @@ -22,7 +22,9 @@ it open, `--library` on the same folder fails with "This library is already open `--demo` starts the headless session with the procedurally generated demo library; `--compact` lists only the helper tools (see below). In connect mode the server starts even when the app is not running yet and connects on the first call (and reconnects if the app restarts). -A request is sent again only when it never reached the app. If the connection drops after a request +A request is sent again (once) only when it never reached the app: it could not be sent, or the app's +idle-close notice came back in its place (the app closed the idle connection without reading it; see +[control-protocol.md](control-protocol.md)). If the connection drops after a request was sent but before its reply, the tool fails with "the outcome is unknown" and nothing is resent, so a command such as undo can't run twice: check the app's state before retrying. Replies must be newline-terminated, at most 64 MiB before the newline (including encoded previews/screenshots), From 05ff4ca06da6f005141e7fa31f5b8f10c5d78955 Mon Sep 17 00:00:00 2001 From: Dominic Anaeto Date: Mon, 26 Oct 2026 22:59:08 -0400 Subject: [PATCH 07/23] LC-001: stop commands after failed imports --- apps/lightcraft-cli/src/main.rs | 80 +++++++++++++-- apps/lightcraft-cli/tests/it/cli.rs | 147 ++++++++++++++++++++++++++++ 2 files changed, 219 insertions(+), 8 deletions(-) diff --git a/apps/lightcraft-cli/src/main.rs b/apps/lightcraft-cli/src/main.rs index c1f9d258c..bff4f326a 100644 --- a/apps/lightcraft-cli/src/main.rs +++ b/apps/lightcraft-cli/src/main.rs @@ -594,14 +594,7 @@ fn run(args: &[String]) -> Result<(), String> { None => Headless::default(), }; if !imports.is_empty() { - let paths = expand_paths(&imports); - let r = h.session.execute("library.import", &json!({"paths": paths})).map_err(|e| e.to_string())?; - // what follows acts on the imported photos (already-known files are reported as duplicates) - let mut ids: Vec = r["imported"].as_array().cloned().unwrap_or_default(); - ids.extend(r["duplicates"].as_array().into_iter().flatten().filter_map(|d| d.get("existing").filter(|v| v.is_u64()).cloned())); - if let Some(first) = ids.first().cloned() { - h.session.execute("library.select", &json!({"ids": ids, "active": first})).map_err(|e| e.to_string())?; - } + import_and_select(&mut h.session, &imports)?; } Box::new(h) } @@ -626,6 +619,32 @@ fn run(args: &[String]) -> Result<(), String> { if failed > 0 { Err(format!("{failed} command(s) failed")) } else { Ok(()) } } +fn import_and_select(session: &mut Session, imports: &[String]) -> Result<(), String> { + let selection = session.selection.clone(); + let paths = expand_paths(imports); + let result = (|| { + let r = session.execute("library.import", &json!({"paths": paths})).map_err(|e| format!("import failed: {e}; no commands were run"))?; + let ids = import_targets(&r)?; + let first = ids.first().cloned().ok_or("import found no photos to select; no commands were run")?; + session.execute("library.select", &json!({"ids": ids, "active": first})).map_err(|e| e.to_string())?; + Ok::<(), String>(()) + })(); + if result.is_err() { + session.selection = selection; + } + result +} + +fn import_targets(report: &Value) -> Result, String> { + if let Some(failed) = report["failed"].as_array().filter(|failed| !failed.is_empty()) { + return Err(format!("import failed for {} item(s): {}; no commands were run", failed.len(), json!(failed))); + } + let mut ids: Vec = report["imported"].as_array().cloned().unwrap_or_default(); + ids.extend(report["duplicates"].as_array().into_iter().flatten().filter_map(|d| d.get("existing").filter(|v| v.is_u64()).cloned())); + ids.extend(report["restored"].as_array().into_iter().flatten().filter(|id| id.is_u64()).cloned()); + if ids.is_empty() { Err("import found no photos to select; no commands were run".into()) } else { Ok(ids) } +} + fn take_value<'a>(args: &'a [String], i: &mut usize, flag: &str) -> Result<&'a str, String> { *i += 1; args.get(*i).map(String::as_str).ok_or_else(|| format!("{flag} needs a value")) @@ -1028,3 +1047,48 @@ fn denoise_serve(args: &[String]) -> Result<(), String> { fn denoise_serve(_: &[String]) -> Result<(), String> { Err("denoise-serve: this build has no denoise support (build with the `denoise` feature)".into()) } + +#[cfg(test)] +mod import_target_tests { + use super::{import_and_select, import_targets, json}; + + #[test] + fn empty_reports_and_duplicates_without_ids_have_no_target() { + assert!(import_targets(&json!({})).is_err()); + assert!(import_targets(&json!({"imported": [], "duplicates": [{"existing": null}]})).is_err()); + } + + #[test] + fn restored_only_and_mixed_reports_keep_target_order() { + assert_eq!(import_targets(&json!({"restored": [9]})).unwrap(), vec![json!(9)]); + assert_eq!( + import_targets(&json!({"imported": [1], "duplicates": [{"existing": null}, {"existing": 2}], "restored": [3]})).unwrap(), + vec![json!(1), json!(2), json!(3)] + ); + } + + #[test] + fn failed_items_reject_even_when_targets_exist() { + let error = import_targets(&json!({"imported": [1], "failed": [["broken.png", "invalid"]]})).unwrap_err(); + assert!(error.contains("1") && error.contains("broken.png") && error.contains("no commands were run"), "{error}"); + } + + #[test] + fn failed_partial_import_restores_empty_selection() { + let dir = std::env::temp_dir().join(format!("lightcraft-import-selection-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let input = dir.join("valid.png"); + let broken = dir.join("broken.png"); + let img = lightcraft_raster::Rgba8::from_fn(2, 2, |_, _| [20, 40, 60, 255]); + let png = lightcraft_codecs::encode_png(&lightcraft_codecs::EncodeImage::rgba8(&img), &Default::default()).unwrap(); + std::fs::write(&input, png).unwrap(); + std::fs::write(&broken, b"not a PNG").unwrap(); + let mut headless = lightcraft_mcp::Headless::default(); + let session = &mut headless.session; + let selection = session.selection.clone(); + assert!(selection.ids.is_empty() && selection.active.is_none()); + assert!(import_and_select(session, &[input.to_string_lossy().into_owned(), broken.to_string_lossy().into_owned()]).is_err()); + assert_eq!(session.selection, selection); + let _ = std::fs::remove_dir_all(dir); + } +} diff --git a/apps/lightcraft-cli/tests/it/cli.rs b/apps/lightcraft-cli/tests/it/cli.rs index 8a11f5c9a..52c28ade4 100644 --- a/apps/lightcraft-cli/tests/it/cli.rs +++ b/apps/lightcraft-cli/tests/it/cli.rs @@ -513,6 +513,153 @@ fn run_cli(args: &[&str], stdin: Option<&str>) -> (bool, Vec, String) { (o.status.success(), lines, String::from_utf8_lossy(&o.stderr).to_string()) } +#[test] +fn run_failed_import_never_edits_existing_selection() { + let dir = tmp("failed-import"); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let lib = dir.join("library"); + let missing = dir.join("missing.png"); + let empty = dir.join("empty"); + std::fs::create_dir(&empty).unwrap(); + assert!(!missing.exists()); + let lib = lib.to_str().unwrap(); + let (ok, lines, err) = run_cli(&["--demo", "--library", lib, "library.state"], None); + assert!(ok, "{err}"); + let id = lines[0]["result"]["selection"]["active"].as_u64().unwrap(); + let (ok, _, err) = run_cli(&["--library", lib, "photo.rate", &format!("ids=[{id}]"), "rating=0"], None); + assert!(ok, "{err}"); + for path in [&missing, &empty] { + let (ok, lines, err) = run_cli(&["--library", lib, "--import", path.to_str().unwrap(), "photo.rate", "rating=5"], None); + assert!(!ok, "import unexpectedly succeeded for {}: {lines:?}", path.display()); + assert!(lines.is_empty(), "commands ran: {lines:?}"); + assert!(err.contains("import") && err.contains("no commands were run"), "{err}"); + if path == &missing { + assert!(err.contains("failed") && err.contains("missing.png"), "{err}"); + } + let (ok, lines, err) = run_cli(&["--library", lib, "photo.inspect", &format!("id={id}")], None); + assert!(ok, "{err}"); + assert_eq!(lines[0]["result"]["rating"], 0); + } +} + +#[test] +fn run_partial_import_stops_commands_and_import_targets_remain_usable() { + let dir = tmp("partial-import"); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let input = dir.join("valid.png"); + let broken = dir.join("broken.png"); + gradient_png(&input); + std::fs::write(&broken, b"not a PNG").unwrap(); + let mixed_lib = dir.join("mixed"); + let mixed_lib = mixed_lib.to_str().unwrap(); + let input = input.to_str().unwrap(); + let (ok, lines, err) = run_cli(&["--demo", "--library", mixed_lib, "catalog.query"], None); + assert!(ok, "{err}"); + let photos = lines[0]["result"]["photos"].as_array().unwrap(); + let a = photos[0]["id"].as_u64().unwrap(); + let b = photos[1]["id"].as_u64().unwrap(); + let (ok, lines, err) = run_cli( + &[ + "--library", + mixed_lib, + "library.select", + &format!("ids=[{a},{b}]"), + &format!("active={a}"), + "photo.rate", + &format!("ids=[{a}]"), + "rating=0", + "library.state", + ], + None, + ); + assert!(ok, "{err}"); + let selection = lines[2]["result"]["selection"].clone(); + assert_eq!(selection["ids"], json!([a, b])); + assert_eq!(selection["active"], a); + let (ok, lines, err) = + run_cli(&["--library", mixed_lib, "--import", input, "--import", broken.to_str().unwrap(), "photo.rate", "rating=5"], None); + let (query_ok, after, query_err) = + run_cli(&["--library", mixed_lib, "library.state", "photo.inspect", &format!("id={a}"), "catalog.query"], None); + assert!(query_ok, "{query_err}"); + assert_eq!(after[0]["result"]["selection"], selection, "selection changed after partial import"); + assert!(!ok, "partial import unexpectedly succeeded: {lines:?}"); + assert!(lines.is_empty() && err.contains("failed") && err.contains("no commands were run"), "{err}: {lines:?}"); + assert_eq!(after[1]["result"]["rating"], 0); + assert!(after[2]["result"]["photos"].as_array().unwrap().iter().any(|p| p["fileName"] == "valid.png")); + + let lib = dir.join("duplicates"); + let lib = lib.to_str().unwrap(); + let (ok, lines, err) = run_cli(&["--demo", "--library", lib, "library.state"], None); + assert!(ok, "{err}"); + let previous = lines[0]["result"]["selection"]["active"].as_u64().unwrap(); + let (ok, lines, err) = run_cli(&["--library", lib, "--import", input, "photo.rate", "rating=4", "library.state"], None); + assert!(ok, "{err}"); + let id = lines[1]["result"]["selection"]["active"].as_u64().unwrap(); + assert_ne!(id, previous); + let (ok, _, err) = run_cli(&["--library", lib, "library.select", &format!("ids=[{previous}]"), "photo.rate", "rating=0"], None); + assert!(ok, "{err}"); + let (ok, _, err) = run_cli(&["--library", lib, "--import", input, "photo.rate", "rating=3"], None); + assert!(ok, "{err}"); + let (ok, lines, err) = run_cli(&["--library", lib, "photo.inspect", &format!("id={id}"), "photo.inspect", &format!("id={previous}")], None); + assert!(ok, "{err}"); + assert_eq!(lines[0]["result"]["rating"], 3); + assert_eq!(lines[1]["result"]["rating"], 0); +} + +#[test] +fn run_restored_import_is_selected() { + let dir = tmp("restored-import"); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let input = dir.join("valid.png"); + gradient_png(&input); + let lib = dir.join("library"); + let (lib, input) = (lib.to_str().unwrap(), input.to_str().unwrap()); + let (ok, lines, err) = run_cli(&["--library", lib, "--import", input, "library.state"], None); + assert!(ok, "{err}"); + let id = lines[0]["result"]["selection"]["active"].as_u64().unwrap(); + let (ok, _, err) = run_cli(&["--library", lib, "photo.delete"], None); + assert!(ok, "{err}"); + let (ok, _, err) = run_cli(&["--library", lib, "--import", input, "photo.rate", "rating=4"], None); + assert!(ok, "{err}"); + let (ok, lines, err) = run_cli(&["--library", lib, "catalog.query"], None); + assert!(ok, "{err}"); + assert_eq!(lines[0]["result"]["photos"].as_array().unwrap().len(), 1); + assert_eq!(lines[0]["result"]["photos"][0]["id"], id); + assert_eq!(lines[0]["result"]["photos"][0]["rating"], 4); +} + +#[test] +fn run_mixed_new_and_restored_import_selects_both() { + let dir = tmp("mixed-restored-import"); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let restored = dir.join("restored.png"); + let new = dir.join("new.png"); + gradient_png(&restored); + let image = lightcraft_raster::Rgba8::from_fn(120, 80, |x, y| [(x * 2) as u8, (y * 3) as u8, 200, 255]); + let png = lightcraft_codecs::encode_png(&lightcraft_codecs::EncodeImage::rgba8(&image), &Default::default()).unwrap(); + std::fs::write(&new, png).unwrap(); + let lib = dir.join("library"); + let (lib, restored, new) = (lib.to_str().unwrap(), restored.to_str().unwrap(), new.to_str().unwrap()); + let (ok, lines, err) = run_cli(&["--library", lib, "--import", restored, "library.state"], None); + assert!(ok, "{err}"); + let restored_id = lines[0]["result"]["selection"]["active"].as_u64().unwrap(); + let (ok, _, err) = run_cli(&["--library", lib, "photo.delete"], None); + assert!(ok, "{err}"); + let (ok, lines, err) = run_cli(&["--library", lib, "--import", new, "--import", restored, "photo.rate", "rating=4", "library.state"], None); + assert!(ok, "{err}"); + assert_eq!(lines[1]["result"]["selection"]["ids"].as_array().unwrap().len(), 2); + let (ok, lines, err) = run_cli(&["--library", lib, "catalog.query"], None); + assert!(ok, "{err}"); + let photos = lines[0]["result"]["photos"].as_array().unwrap(); + assert_eq!(photos.len(), 2); + assert!(photos.iter().any(|p| p["id"] == restored_id && p["rating"] == 4)); + assert!(photos.iter().any(|p| p["fileName"] == "new.png" && p["id"] != restored_id && p["rating"] == 4)); +} + #[test] fn run_subcommand_chains_commands_and_persists_a_library() { let input = tmp("run-in.png"); From b576c72d0b83fd8296b7e7d1f384539c7078ca1f Mon Sep 17 00:00:00 2001 From: Dominic Anaeto Date: Mon, 26 Oct 2026 22:59:20 -0400 Subject: [PATCH 08/23] LC-002: reject invalid script parameters --- apps/lightcraft-cli/src/main.rs | 6 +++++- apps/lightcraft-cli/tests/it/cli.rs | 26 ++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/apps/lightcraft-cli/src/main.rs b/apps/lightcraft-cli/src/main.rs index c1f9d258c..84ca49d21 100644 --- a/apps/lightcraft-cli/src/main.rs +++ b/apps/lightcraft-cli/src/main.rs @@ -565,7 +565,11 @@ fn run(args: &[String]) -> Result<(), String> { .or(v.get("method")) .and_then(Value::as_str) .ok_or_else(|| format!("{path}:{}: needs `command` or `method`", n + 1))?; - let params = v.get("params").cloned().filter(|p| p.is_object()).unwrap_or_else(|| json!({})); + let params = match v.get("params") { + None | Some(Value::Null) => json!({}), + Some(Value::Object(params)) => Value::Object(params.clone()), + Some(_) => return Err(format!("{path}:{}: `params` must be a JSON object", n + 1)), + }; steps.push(Step { id: id.to_string(), params }); } } diff --git a/apps/lightcraft-cli/tests/it/cli.rs b/apps/lightcraft-cli/tests/it/cli.rs index 8a11f5c9a..8622e7c18 100644 --- a/apps/lightcraft-cli/tests/it/cli.rs +++ b/apps/lightcraft-cli/tests/it/cli.rs @@ -513,6 +513,32 @@ fn run_cli(args: &[&str], stdin: Option<&str>) -> (bool, Vec, String) { (o.status.success(), lines, String::from_utf8_lossy(&o.stderr).to_string()) } +#[test] +fn run_script_params_validate_before_any_commands() { + let path = tmp("run-invalid-params.jsonl"); + let path_str = path.to_str().unwrap(); + for params in [json!([]), json!("invalid"), json!(42), json!(true)] { + let script = format!("{}\n{}\n", json!({"command": "library.info"}), json!({"command": "develop.reset", "params": params})); + std::fs::write(&path, &script).unwrap(); + for (source, stdin) in [("-", Some(script.as_str())), (path_str, None)] { + let (ok, lines, err) = run_cli(&["--script", source], stdin); + assert!(!ok, "{source}: {params}: {lines:?}"); + assert!(lines.is_empty(), "{source}: {params}: {lines:?}"); + assert!(err.contains(&format!("{source}:2")), "{err}"); + assert!(err.contains("`params` must be a JSON object"), "{err}"); + } + } + for script in + ["{\"command\":\"library.info\"}\n", "{\"command\":\"library.info\",\"params\":null}\n", "{\"command\":\"library.info\",\"params\":{}}\n"] + { + let (ok, lines, err) = run_cli(&["--script", "-"], Some(script)); + assert!(ok, "{script}: {err}"); + assert_eq!(lines.len(), 1, "{script}: {lines:?}"); + assert_eq!(lines[0]["ok"], true, "{script}: {lines:?}"); + } + std::fs::remove_file(path).unwrap(); +} + #[test] fn run_subcommand_chains_commands_and_persists_a_library() { let input = tmp("run-in.png"); From 939055897fff9729a71c8e0883fe90e3c5fbb7cf Mon Sep 17 00:00:00 2001 From: Dominic Anaeto Date: Mon, 26 Oct 2026 22:59:25 -0400 Subject: [PATCH 09/23] LC-003: reject non-UTF-8 CLI arguments cleanly --- apps/lightcraft-cli/src/main.rs | 11 ++++++++++- apps/lightcraft-cli/tests/it/cli.rs | 18 ++++++++++++++++++ 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/apps/lightcraft-cli/src/main.rs b/apps/lightcraft-cli/src/main.rs index c1f9d258c..a650ef748 100644 --- a/apps/lightcraft-cli/src/main.rs +++ b/apps/lightcraft-cli/src/main.rs @@ -155,7 +155,16 @@ fn main() -> ExitCode { // Warnings (a GPU render redone on the CPU, an unknown backend name) on stderr; LIGHTCRAFT_LOG // or RUST_LOG picks another level (#168). lightcraft_engine::logging::install("lightcraft-cli"); - let args: Vec = std::env::args().skip(1).collect(); + let mut args: Vec = Vec::new(); + for (index, arg) in std::env::args_os().skip(1).enumerate() { + match arg.into_string() { + Ok(arg) => args.push(arg), + Err(_) => { + eprintln!("lightcraft-cli: argument {} is not valid UTF-8; use a UTF-8 encoded argument", index + 1); + return ExitCode::FAILURE; + } + } + } let r = match args.first().map(String::as_str) { Some("run") => run(&args[1..]), Some("mcp") => mcp(&args[1..]), diff --git a/apps/lightcraft-cli/tests/it/cli.rs b/apps/lightcraft-cli/tests/it/cli.rs index 8a11f5c9a..cb898005c 100644 --- a/apps/lightcraft-cli/tests/it/cli.rs +++ b/apps/lightcraft-cli/tests/it/cli.rs @@ -162,6 +162,24 @@ fn mcp_over_stdio_sets_exposure_and_renders() { assert!(b > a + 10.0, "{a} → {b}"); } +#[cfg(unix)] +#[test] +fn non_utf8_argument_fails_without_a_panic() { + use std::ffi::OsString; + use std::os::unix::ffi::OsStringExt; + + let out = tmp("non-utf8-output.png"); + let _ = std::fs::remove_file(&out); + let output = Command::new(binary()).arg("render").arg(OsString::from_vec(b"bad-\xff.png".to_vec())).arg("-o").arg(&out).output().unwrap(); + let stderr = String::from_utf8_lossy(&output.stderr); + assert_eq!(output.status.code(), Some(1), "{stderr}"); + assert!(stderr.contains("argument 2"), "{stderr}"); + assert!(stderr.contains("UTF-8"), "{stderr}"); + assert!(!stderr.contains("panicked"), "{stderr}"); + assert!(!out.exists()); + assert!(Command::new(binary()).arg("--version").status().unwrap().success()); +} + #[test] fn render_subcommand() { let input = tmp("r-in.png"); From a55218d221f57a2ad61077f0e68bafc007431480 Mon Sep 17 00:00:00 2001 From: Dominic Anaeto Date: Tue, 27 Oct 2026 11:12:10 -0400 Subject: [PATCH 10/23] CLI: select each imported photo once --- apps/lightcraft-cli/src/main.rs | 8 +++++++ apps/lightcraft-cli/tests/it/cli.rs | 37 +++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+) diff --git a/apps/lightcraft-cli/src/main.rs b/apps/lightcraft-cli/src/main.rs index bff4f326a..36e435e81 100644 --- a/apps/lightcraft-cli/src/main.rs +++ b/apps/lightcraft-cli/src/main.rs @@ -642,6 +642,8 @@ fn import_targets(report: &Value) -> Result, String> { let mut ids: Vec = report["imported"].as_array().cloned().unwrap_or_default(); ids.extend(report["duplicates"].as_array().into_iter().flatten().filter_map(|d| d.get("existing").filter(|v| v.is_u64()).cloned())); ids.extend(report["restored"].as_array().into_iter().flatten().filter(|id| id.is_u64()).cloned()); + let mut seen = std::collections::HashSet::new(); + ids.retain(|id| id.as_u64().is_none_or(|id| seen.insert(id))); if ids.is_empty() { Err("import found no photos to select; no commands were run".into()) } else { Ok(ids) } } @@ -1067,6 +1069,12 @@ mod import_target_tests { ); } + #[test] + fn repeated_targets_are_selected_once_in_first_occurrence_order() { + let report = json!({"imported": [5, 7], "duplicates": [{"existing": 5}, {"existing": 8}, {"existing": 8}, {"existing": 7}, {"existing": null}], "restored": [8, 9, 5]}); + assert_eq!(import_targets(&report).unwrap(), vec![json!(5), json!(7), json!(8), json!(9)]); + } + #[test] fn failed_items_reject_even_when_targets_exist() { let error = import_targets(&json!({"imported": [1], "failed": [["broken.png", "invalid"]]})).unwrap_err(); diff --git a/apps/lightcraft-cli/tests/it/cli.rs b/apps/lightcraft-cli/tests/it/cli.rs index 52c28ade4..ffa78b541 100644 --- a/apps/lightcraft-cli/tests/it/cli.rs +++ b/apps/lightcraft-cli/tests/it/cli.rs @@ -631,6 +631,43 @@ fn run_restored_import_is_selected() { assert_eq!(lines[0]["result"]["photos"][0]["rating"], 4); } +#[test] +fn run_same_batch_duplicates_selects_photo_once() { + let dir = tmp("same-batch-duplicates"); + let _ = std::fs::remove_dir_all(&dir); + std::fs::create_dir_all(&dir).unwrap(); + let first = dir.join("first.png"); + let second = dir.join("second.png"); + gradient_png(&first); + std::fs::copy(&first, &second).unwrap(); + let library = dir.join("library"); + let (ok, lines, err) = run_cli( + &[ + "--library", + library.to_str().unwrap(), + "--import", + first.to_str().unwrap(), + "--import", + second.to_str().unwrap(), + "photo.rate", + "rating=4", + "library.state", + "catalog.query", + ], + None, + ); + assert!(ok, "{err}"); + let selection = &lines[1]["result"]["selection"]; + let ids = selection["ids"].as_array().unwrap(); + assert_eq!(ids.len(), 1, "same photo selected more than once: {selection}"); + assert_eq!(selection["active"], ids[0]); + let photos = lines[2]["result"]["photos"].as_array().unwrap(); + assert_eq!(photos.len(), 1); + assert_eq!(photos[0]["id"], ids[0]); + assert_eq!(photos[0]["rating"], 4); + let _ = std::fs::remove_dir_all(dir); +} + #[test] fn run_mixed_new_and_restored_import_selects_both() { let dir = tmp("mixed-restored-import"); From d99fb7bc0d95e848a87c458f89157dd580048edc Mon Sep 17 00:00:00 2001 From: Nathaniel Ledford Date: Thu, 22 Oct 2026 19:10:10 -0400 Subject: [PATCH 11/23] LRC-LIB-MISSING: library.missing checks the files off the UI thread MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the control channel (an agent, `mcp --connect`) the command ran in Session::execute on the UI thread and stat-ed every photo's file, which on a large library on a network share froze the window for minutes. The app now checks the files on a worker thread under a "Missing Photos" row of the activity stack (counted, with ✕), answers the request with the same [{id, path}] when the check ends, lets a second request share the running check, and hands an answer nobody was waiting for to the next request. The command itself asks the session's existence check, the one the availability cache uses. Co-Authored-By: Claude Sonnet 5.5 --- crates/engine/src/cmd/missing.rs | 46 +++++- crates/engine/src/tests/import.rs | 49 ++++++ crates/ui-egui/src/app.rs | 1 + crates/ui-egui/src/app/control.rs | 1 + crates/ui-egui/src/control.rs | 20 ++- crates/ui-egui/src/headless.rs | 16 +- crates/ui-egui/src/lib.rs | 4 + crates/ui-egui/src/missing_sweep.rs | 96 ++++++++++++ crates/ui-egui/src/tests.rs | 1 + crates/ui-egui/src/tests/missing_sweep.rs | 173 ++++++++++++++++++++++ docs/parity-checklist.md | 5 +- docs/whats-new.md | 8 + 12 files changed, 409 insertions(+), 11 deletions(-) create mode 100644 crates/ui-egui/src/missing_sweep.rs create mode 100644 crates/ui-egui/src/tests/missing_sweep.rs diff --git a/crates/engine/src/cmd/missing.rs b/crates/engine/src/cmd/missing.rs index c06997eef..09e2131ef 100644 --- a/crates/engine/src/cmd/missing.rs +++ b/crates/engine/src/cmd/missing.rs @@ -41,7 +41,47 @@ pub fn missing(s: &Session) -> Vec<(PhotoId, String)> { if cfg!(target_arch = "wasm32") { return Vec::new(); } - missing_with(&s.catalog, |f| Path::new(f).exists()) + let exists = s.media.availability.probe(); + missing_with(&s.catalog, |f| exists(f)) +} + +/// Every in-scope photo with its file ([`checked_path`]), without touching the disk: what a +/// sweep ([`sweep`]) goes through, so a caller can take the list on its own thread and check +/// the files on another. +pub fn checked(cat: &Catalog) -> Vec<(PhotoId, String)> { + cat.photos().filter_map(|p| checked_path(p).map(|f| (p.id, f.to_string()))).collect() +} + +/// How many files [`sweep`] checks between two reports of its progress. +const PROGRESS_EVERY: usize = 64; + +/// The files among `files` that `exists` doesn't find, in order: [`missing`] for a caller that +/// checks on a thread of its own. `stop` is asked before every check (`None`: it said yes, and +/// the rest was not looked at); `progress` is told how many files are done every few dozen. +pub fn sweep( + files: Vec<(PhotoId, String)>, + exists: impl Fn(&str) -> bool, + stop: impl Fn() -> bool, + progress: impl Fn(usize), +) -> Option> { + let mut gone = Vec::new(); + for (done, (id, file)) in files.into_iter().enumerate() { + if stop() { + return None; + } + if done % PROGRESS_EVERY == 0 { + progress(done); + } + if !exists(&file) { + gone.push((id, file)); + } + } + Some(gone) +} + +/// `library.missing`'s answer: `[{id, path}]`. +pub fn missing_json(gone: Vec<(PhotoId, String)>) -> Value { + Value::Array(gone.into_iter().map(|(id, path)| json!({"id": id.0, "path": path})).collect()) } /// Whether photo `id` is in scope and its file is gone (the Missing Photos view checks only @@ -418,8 +458,8 @@ fn find_missing(s: &mut Session, p: &Value) -> Result { pub fn specs() -> Vec { vec![ - cmd!(query "library.missing", "Missing Photos", [], None, "{} → [{id, path}] — photos whose original file isn't where the library expects it", always, |s, _| { - Ok(Value::Array(missing(s).into_iter().map(|(id, path)| json!({"id": id.0, "path": path})).collect())) + cmd!(query "library.missing", "Missing Photos", [], None, "{} → [{id, path}] — photos whose original file isn't where the library expects it (every file is looked at: on a network share that takes minutes, so the desktop app, asked over its control channel, checks on a worker thread, with a row in the activity stack that can stop it, and answers when it is done)", always, |s, _| { + Ok(missing_json(missing(s))) }), cmd!( "photo.relink", diff --git a/crates/engine/src/tests/import.rs b/crates/engine/src/tests/import.rs index 6e5585a82..89b2dcafa 100644 --- a/crates/engine/src/tests/import.rs +++ b/crates/engine/src/tests/import.rs @@ -1255,6 +1255,55 @@ fn missing_photos_skip_local_browse_records() { assert_eq!(s.visible_cloned().len(), 4, "the view lists what the count counts"); } +/// `library.missing` asks the session's existence check (the one the availability cache and +/// the tests replace), and a sweep on another thread answers what the command does: the same +/// files in the same order, progress on the way, and a stop that leaves the rest unchecked. +#[test] +fn missing_sweep_answers_what_the_command_does_and_can_stop() { + use lightcraft_catalog::{Op, Photo, PhotoId, Source}; + use std::sync::atomic::{AtomicUsize, Ordering}; + let mut s = Session::new(); + let mut ops = Vec::new(); + for i in 0..200u64 { + let path = format!("/share/IMG_{i:04}.jpg"); + let p = Photo::new(PhotoId(i + 1), Source::File { path }, &format!("IMG_{i:04}.jpg"), "JPEG", 60, 40, "2026-01-01T00:00:00"); + ops.push(Op::AddPhoto { photo: Box::new(p) }); + } + s.commit("Add", Op::Batch { ops }).unwrap(); + // the first hundred files are gone + s.media.availability.set_probe(std::sync::Arc::new(|f| !f.contains("_00"))); + let asked = s.execute("library.missing", &json!({})).unwrap(); + assert_eq!(asked.as_array().map(Vec::len), Some(100), "the command used the session's check: {asked}"); + + let files = crate::cmd::missing::checked(&s.catalog); + assert_eq!(files.len(), 200); + let reports = AtomicUsize::new(0); + let probe = s.media.availability.probe(); + let swept = crate::cmd::missing::sweep( + files.clone(), + |f| probe(f), + || false, + |_| { + reports.fetch_add(1, Ordering::Relaxed); + }, + ); + assert_eq!(crate::cmd::missing::missing_json(swept.unwrap()), asked); + assert!(reports.load(Ordering::Relaxed) >= 2, "progress is reported on the way"); + + // stopped after 10 files: nothing is answered, and the rest is not looked at + let looked = AtomicUsize::new(0); + let stopped = crate::cmd::missing::sweep( + files, + |_| { + looked.fetch_add(1, Ordering::Relaxed); + true + }, + || looked.load(Ordering::Relaxed) >= 10, + |_| {}, + ); + assert_eq!((stopped, looked.load(Ordering::Relaxed)), (None, 10)); +} + /// `library.browse` follows `library.source`'s rule for the active photo (issue #1447): a library /// photo the folder doesn't show gives way to the folder's first photo; one it shows stays. #[test] diff --git a/crates/ui-egui/src/app.rs b/crates/ui-egui/src/app.rs index 98f8458c9..0937f531d 100644 --- a/crates/ui-egui/src/app.rs +++ b/crates/ui-egui/src/app.rs @@ -65,6 +65,7 @@ impl LightcraftApp { drop_pending: vec![], copied: None, pending_picks: vec![], + missing: Default::default(), synthetic_mods: egui::Modifiers::NONE, synthetic_mods_release: false, host_mods: egui::Modifiers::NONE, diff --git a/crates/ui-egui/src/app/control.rs b/crates/ui-egui/src/app/control.rs index 00b66e4b0..42bdea5c0 100644 --- a/crates/ui-egui/src/app/control.rs +++ b/crates/ui-egui/src/app/control.rs @@ -18,6 +18,7 @@ impl LightcraftApp { control::Outcome::Done(v) => { let _ = reply.send(v); } + control::Outcome::Later => {} control::Outcome::Screenshot { target, headless } => { if let Err(error) = self.admit_shot(ctx, target, headless, reply.clone()) { let _ = reply.send(serde_json::json!({"ok": false, "error": error})); diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index caacbc590..8e79820db 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -59,6 +59,8 @@ impl ControlRequest { pub enum Outcome { Done(Value), + /// Answered later, by whoever holds the request's `reply` (a task that is still running). + Later, /// A capture was admitted: `target` pins the destination identity until the write. Screenshot { target: Option, @@ -72,11 +74,19 @@ pub enum Outcome { }, } +/// A reply that says it worked: `result`. +pub(crate) fn success(v: Value) -> Value { + json!({"ok": true, "result": v}) +} +/// A reply that says it failed: `error`. +pub(crate) fn failure(e: impl std::fmt::Display) -> Value { + json!({"ok": false, "error": e.to_string()}) +} fn ok(v: Value) -> Outcome { - Outcome::Done(json!({"ok": true, "result": v})) + Outcome::Done(success(v)) } fn err(e: impl std::fmt::Display) -> Outcome { - Outcome::Done(json!({"ok": false, "error": e.to_string()})) + Outcome::Done(failure(e)) } fn wrap(r: Result) -> Outcome { match r { @@ -329,6 +339,12 @@ pub fn handle(app: &mut LightcraftApp, ctx: &egui::Context, req: &ControlRequest "engine.execute" | "ui.menu.invoke" | "command" => { let Some(id) = s("command").or(s("id")) else { return err("missing `command`") }; let params = p.get("params").cloned().filter(|v| !v.is_null()).unwrap_or(json!({})); + if id == "library.missing" { + // looks at every photo's file: on a worker thread, answered when it is done + let started = crate::missing_sweep::ask(app, req.reply.clone()); + ctx.request_repaint(); + return started; + } let r = app.run(id, params); ctx.request_repaint(); wrap(r) diff --git a/crates/ui-egui/src/headless.rs b/crates/ui-egui/src/headless.rs index ab49563cb..d9a90d546 100644 --- a/crates/ui-egui/src/headless.rs +++ b/crates/ui-egui/src/headless.rs @@ -466,13 +466,21 @@ impl Headless { } } + /// Queue a control-protocol request without running a frame: the answer comes on the returned + /// channel once a frame ([`Self::step`]) has taken the request up, and the work it starts is done. + pub(crate) fn send(&mut self, method: &str, params: Value) -> Result, Value> { + let (req, rx) = ControlRequest::new(method, params); + self.control.send(req).map_err(|_| json!({"ok": false, "error": "control channel closed"}))?; + Ok(rx) + } + /// Send a control-protocol request (see [`crate::control`]) and run frames until it is /// answered, then until its input has been consumed. Returns `{"ok": …, "result"|"error": …}`. pub fn request(&mut self, method: &str, params: Value, timeout: Duration) -> Value { - let (req, rx) = ControlRequest::new(method, params); - if self.control.send(req).is_err() { - return json!({"ok": false, "error": "control channel closed"}); - } + let rx = match self.send(method, params) { + Ok(rx) => rx, + Err(e) => return e, + }; let t0 = Instant::now(); let reply = loop { self.step(); diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 9db7035c1..c8158b8fe 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -22,6 +22,7 @@ pub mod icons; pub mod lightroom_import; pub mod links; pub mod menus; +mod missing_sweep; mod model_setup; pub mod panels; pub mod pick; @@ -225,6 +226,9 @@ pub struct LightcraftApp { pub copied: Option, /// Native file dialogs up for commands (`pick`): each command runs again when its closes. pub(crate) pending_picks: Vec, + /// `library.missing` requests from the control channel and the check that answers them on a + /// worker thread ([`missing_sweep`]). + pub(crate) missing: missing_sweep::State, /// Modifiers announced for synthetic input (held from a button down to its release). This is /// only the app's synthetic ownership; the real host state is tracked separately in /// `host_mods`, so a canceled synthetic gesture never clears a physically held key. diff --git a/crates/ui-egui/src/missing_sweep.rs b/crates/ui-egui/src/missing_sweep.rs new file mode 100644 index 000000000..dd743b8ef --- /dev/null +++ b/crates/ui-egui/src/missing_sweep.rs @@ -0,0 +1,96 @@ +//! `library.missing` in the app. The engine's command looks at every photo's file on the calling +//! thread, which here is the UI's: on a share with hundreds of thousands of photos that froze the +//! window for minutes. A request from the control channel (and so an agent, `mcp --connect`) goes +//! the way of the person's own long jobs instead: the files are checked on a worker thread, under +//! a row in the activity stack that counts them and can stop the check, and the request is +//! answered with the same `[{id, path}]` once the check ends. A second request meanwhile joins the +//! check that is running and gets its answer, rather than starting another walk of the share. +//! +//! A check on a big share can outlast what a caller waits for (the control server gives a request +//! 60 s). An answer that nobody was waiting for any more is kept for a few minutes and given to +//! the next request, so that asking again after a timeout is not another walk of the share that +//! times out again. + +use std::sync::Arc; +use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::mpsc::Sender; +use std::time::Duration; + +use web_time::Instant; + +use lightcraft_catalog::PhotoId; +use lightcraft_engine::cmd::missing; +use serde_json::Value; + +use crate::LightcraftApp; +use crate::control::{self, Outcome}; + +/// The activity stack's label (the command's name, already translated). +pub const LABEL: &str = "Missing Photos"; + +/// The activity row's kind. +const KIND: &str = "missing"; + +/// How long an answer nobody was waiting for is kept. +const KEEP: Duration = Duration::from_secs(10 * 60); + +/// The requests waiting for a check, and the answer kept for the next one. +#[derive(Default)] +pub struct State { + /// Each request with the library ([`lightcraft_engine::Session::library_generation`]) it asked about. + waiting: Vec<(u64, Sender)>, + /// A finished check's answer that no request received: its library and when it was kept. + kept: Option<(u64, Instant, Value)>, +} + +/// Answer `library.missing` from the control channel: give the answer kept from a check nobody +/// waited for, or start the check, or join the one running for this library. The answer goes to +/// `reply` when the check ends. +pub fn ask(app: &mut LightcraftApp, reply: Sender) -> Outcome { + let library = app.session.library_generation(); + if let Some((kept_for, at, answer)) = app.missing.kept.take() + && kept_for == library + && at.elapsed() < KEEP + { + return Outcome::Done(answer); + } + app.missing.waiting.push((library, reply)); + if app.tasks.is_running(LABEL) && app.missing.waiting.iter().filter(|(l, _)| *l == library).count() > 1 { + return Outcome::Later; + } + let files = missing::checked(&app.session.catalog); + let total = files.len() as u64; + let exists = app.session.media.availability.probe(); + let cancel = Arc::new(AtomicBool::new(false)); + let flag = cancel.clone(); + let work = move |task: lightcraft_engine::activity::TaskHandle| { + task.progress(0, total); + // (a panic is this check's failure, answered to the callers: they would wait for it in vain) + lightcraft_engine::guard::catch("Missing Photos", || { + missing::sweep(files, |f| exists(f), || flag.load(Ordering::Relaxed), |done| task.progress(done as u64, total)) + }) + }; + let done = move |app: &mut LightcraftApp, _ctx: &egui::Context, checked: Result>, String>| { + let (answer, complete) = match checked { + _ if app.session.library_generation() != library => { + (control::failure("Missing Photos: another library was opened before the check ended"), false) + } + Err(e) => (control::failure(e), false), + Ok(None) => (control::failure("Missing Photos: the check was stopped"), false), + Ok(Some(gone)) => (control::success(missing::missing_json(gone)), true), + }; + let (theirs, others): (Vec<_>, Vec<_>) = std::mem::take(&mut app.missing.waiting).into_iter().partition(|(l, _)| *l == library); + app.missing.waiting = others; + // (a request whose caller gave up has no one to send to) + let received = theirs.into_iter().fold(false, |received, (_, waiting)| waiting.send(answer.clone()).is_ok() || received); + if complete && !received { + app.missing.kept = Some((library, Instant::now(), answer)); + } + }; + if let Err(e) = crate::tasks::spawn_tracked(app, LABEL, KIND, cancel, work, done) { + for (_, waiting) in std::mem::take(&mut app.missing.waiting) { + let _ = waiting.send(control::failure(&e)); + } + } + Outcome::Later +} diff --git a/crates/ui-egui/src/tests.rs b/crates/ui-egui/src/tests.rs index 3a102a29a..f9af57df2 100644 --- a/crates/ui-egui/src/tests.rs +++ b/crates/ui-egui/src/tests.rs @@ -50,6 +50,7 @@ mod masking; mod masking_layout; mod menubar; mod merge_preview; +mod missing_sweep; mod model_setup; mod offline; mod open_lightroom; diff --git a/crates/ui-egui/src/tests/missing_sweep.rs b/crates/ui-egui/src/tests/missing_sweep.rs new file mode 100644 index 000000000..eea6fc0a5 --- /dev/null +++ b/crates/ui-egui/src/tests/missing_sweep.rs @@ -0,0 +1,173 @@ +//! `library.missing` from the control channel (an agent, `mcp --connect`) looks at every photo's +//! file, which on a share with hundreds of thousands of photos took minutes on the UI thread and +//! froze the window: the files are checked on a worker thread under a row of the activity stack. + +use std::sync::Arc; +use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; +use std::sync::mpsc::Receiver; +use std::time::{Duration, Instant}; + +use lightcraft_catalog::{Op, Photo, PhotoId, Source}; +use serde_json::{Value, json}; + +use crate::headless::Headless; +use crate::{LightcraftApp, Services}; + +const T: Duration = Duration::from_secs(60); +const PHOTOS: u64 = 40; + +/// Photos `IMG_0000` … on a share; the first ten are gone. The existence check waits for `open` +/// (a share that answers slowly), at most `guard`, and counts its calls, and those made on the UI thread. +struct Share { + open: Arc, + on_ui: Arc, + calls: Arc, +} + +fn app_on_a_slow_share(guard: Duration) -> (Headless, Share) { + let mut app = LightcraftApp::new(lightcraft_engine::Session::new(), Services { png: None, ..Default::default() }); + for i in 0..PHOTOS { + let path = format!("/share/IMG_{i:04}.jpg"); + let p = Photo::new(PhotoId(i + 1), Source::File { path }, &format!("IMG_{i:04}.jpg"), "JPEG", 60, 40, "2026-01-01T00:00:00"); + app.session.catalog.apply(Op::AddPhoto { photo: Box::new(p) }).unwrap(); + } + let share = Share { open: Arc::new(AtomicBool::new(false)), on_ui: Arc::new(AtomicUsize::new(0)), calls: Arc::new(AtomicUsize::new(0)) }; + let (ui, open, on_ui, calls) = (std::thread::current().id(), share.open.clone(), share.on_ui.clone(), share.calls.clone()); + let deadline = Instant::now() + guard; + app.session.media.availability.set_probe(Arc::new(move |f| { + calls.fetch_add(1, Ordering::SeqCst); + if std::thread::current().id() == ui { + on_ui.fetch_add(1, Ordering::SeqCst); + } + // (a hang guard: the check goes on after it, so a regression fails the test instead of hanging it) + while !open.load(Ordering::SeqCst) && Instant::now() < deadline { + std::thread::sleep(Duration::from_millis(1)); + } + !f.contains("_000") + })); + (Headless::new(app, [1200.0, 800.0], 1.0), share) +} + +fn ask(h: &mut Headless) -> Receiver { + h.send("engine.execute", json!({"command": "library.missing"})).unwrap() +} + +/// Frames until `rx` has an answer. +fn answer(h: &mut Headless, rx: &Receiver) -> Value { + let t0 = Instant::now(); + loop { + h.step(); + if let Ok(v) = rx.try_recv() { + return v; + } + assert!(t0.elapsed() < T, "never answered"); + std::thread::sleep(Duration::from_millis(1)); + } +} + +fn missing_rows(h: &Headless) -> Vec { + h.app.session.activity.list().into_iter().filter(|r| r.kind == "missing").collect() +} + +#[test] +fn library_missing_is_checked_off_the_ui_thread_and_answered_when_done() { + let (mut h, share) = app_on_a_slow_share(Duration::from_secs(3)); + let t0 = Instant::now(); + let rx = ask(&mut h); + for _ in 0..3 { + h.step(); + } + // the share is still answering nothing: the window took its frames all the same + assert!(t0.elapsed() < Duration::from_secs(2), "the frames waited for the files: {:?}", t0.elapsed()); + assert!(rx.try_recv().is_err(), "no answer before the files are checked"); + let rows = missing_rows(&h); + assert_eq!(rows.len(), 1, "{rows:?}"); + assert!(rows[0].cancellable, "the row can stop the check"); + assert_eq!(rows[0].label, crate::missing_sweep::LABEL); + + share.open.store(true, Ordering::SeqCst); + let reply = answer(&mut h, &rx); + assert_eq!(share.on_ui.load(Ordering::SeqCst), 0, "a file was looked at on the UI thread"); + assert_eq!(reply["ok"], true, "{reply}"); + let ids: Vec = reply["result"].as_array().unwrap().iter().map(|r| r["id"].as_u64().unwrap()).collect(); + assert_eq!(ids, (1..=10).collect::>(), "the same [{{id, path}}] the command gives"); + assert_eq!(reply["result"][0]["path"], "/share/IMG_0000.jpg"); + assert!(h.step_until(T, |h| h.app.tasks.is_empty())); + assert!(missing_rows(&h).is_empty(), "the row goes with the check"); +} + +/// A request that comes while a check runs gets that check's answer: the share isn't walked twice. +#[test] +fn a_second_request_joins_the_check_that_is_running() { + let (mut h, share) = app_on_a_slow_share(Duration::from_secs(3)); + let (first, second) = (ask(&mut h), ask(&mut h)); + for _ in 0..3 { + h.step(); + } + assert_eq!(missing_rows(&h).len(), 1, "one check for both"); + share.open.store(true, Ordering::SeqCst); + let (a, b) = (answer(&mut h, &first), answer(&mut h, &second)); + assert_eq!(a["ok"], true, "{a}"); + assert_eq!(a, b); + assert_eq!(share.on_ui.load(Ordering::SeqCst), 0); +} + +/// ✕ on the row (or `activity.cancel`) stops the check; the request is told so. +#[test] +fn stopping_the_check_answers_the_request_with_an_error() { + let (mut h, share) = app_on_a_slow_share(Duration::from_secs(3)); + let rx = ask(&mut h); + for _ in 0..3 { + h.step(); + } + let id = missing_rows(&h)[0].id; + let r = h.request("engine.execute", json!({"command": "activity.cancel", "params": {"id": id}}), T); + assert_eq!(r["ok"], true, "{r}"); + share.open.store(true, Ordering::SeqCst); + let reply = answer(&mut h, &rx); + assert_eq!(reply["ok"], false, "{reply}"); + assert!(reply["error"].as_str().is_some_and(|e| e.contains("stopped")), "{reply}"); + assert!(h.step_until(T, |h| h.app.tasks.is_empty())); + assert!(missing_rows(&h).is_empty()); +} + +/// The control server gives a request 60 s, a check on a big share takes longer: the caller asks +/// again after its timeout and is given the answer the check ended with, not another walk. +#[test] +fn an_answer_nobody_waited_for_is_given_to_the_next_request() { + let (mut h, share) = app_on_a_slow_share(Duration::from_secs(3)); + let gave_up = ask(&mut h); + for _ in 0..3 { + h.step(); + } + drop(gave_up); + share.open.store(true, Ordering::SeqCst); + assert!(h.step_until(T, |h| h.app.tasks.is_empty()), "the check ends without a caller"); + assert_eq!(share.calls.load(Ordering::SeqCst), PHOTOS as usize); + + let again = ask(&mut h); + let reply = answer(&mut h, &again); + assert_eq!(reply["ok"], true, "{reply}"); + assert_eq!(reply["result"].as_array().map(Vec::len), Some(10), "{reply}"); + assert_eq!(share.calls.load(Ordering::SeqCst), PHOTOS as usize, "no second walk of the share"); + // given once: the next request checks again + let third = ask(&mut h); + assert_eq!(answer(&mut h, &third)["ok"], true); + assert_eq!(share.calls.load(Ordering::SeqCst), 2 * PHOTOS as usize); +} + +/// A check that panics is a failure told to the caller, not a request left waiting for its timeout. +#[test] +fn a_check_that_panics_is_answered_with_an_error() { + let (mut h, _share) = app_on_a_slow_share(Duration::from_secs(3)); + h.app.session.media.availability.set_probe(Arc::new(|_| panic!("a file system that panics"))); + let rx = ask(&mut h); + let reply = answer(&mut h, &rx); + assert_eq!(reply["ok"], false, "{reply}"); + assert!(reply["error"].as_str().is_some_and(|e| e.contains("failed unexpectedly")), "{reply}"); + assert!(h.step_until(T, |h| h.app.tasks.is_empty())); + // and the next request is checked afresh + h.app.session.media.availability.set_probe(Arc::new(|_| true)); + let rx = ask(&mut h); + assert_eq!(answer(&mut h, &rx)["result"], json!([])); +} diff --git a/docs/parity-checklist.md b/docs/parity-checklist.md index 90cfc248f..551214c54 100644 --- a/docs/parity-checklist.md +++ b/docs/parity-checklist.md @@ -1,6 +1,6 @@ # Lightroom parity checklist -> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-25 · **Change:** minor (LR-BEHAV-TITLEBAR: a double-click follows the system setting, and a panel over the bar keeps the window still; KEYC-METADATA, LRC-LIB-KEYWORDS: ⌥0 / ⌥⇧0 go to the next / previous keyword set; LRC-DEV-HISTOGRAM-DRAG: the histogram's five tonal zones — hover for the value, drag to adjust the Basic control the press locked, double-click to reset; LR-VIEW-HISTOGRAM note updated; MENU-BAR: on macOS a menu item that yields its key to a focused text field or to shortcut recording really lets it go; LR-MERGE-HDR / LR-MERGE-PANO: Lanczos-3 warps; LR-IMP-LOCAL: browsing from the sidebar records the folder; LR-VIEW-ZOOM: only a full-size drag within 20 ms clears a slow-drag verdict; LR-IMP-ADD-DIALOG: hierarchical import album destination reconciled with main, #2362; KEY-SLIDER-NUDGE: vertical wheel nudges need a click or drag in the row; leaving it disarms them; LR-EDIT-OPTICS-PROFILE: Fujifilm nine-knot lens tables read at the plain radius; LR-VIEW-ZOOM: only a full-size drag within 20 ms clears a slow-drag verdict; LRC-LIB-LIBFOLDERS, LR-BEHAV-BG-TASKS: Synchronize Folder's scan shows its folder listing and stops it on ✕; LRC-LIB-LIBFOLDERS: Synchronize Folder says a folder is up to date; LR-LIB-STACK: filmstrip stack badge click; LRC-LIB-REFVIEW / CATALOG-SWITCH: Escape preserves edits; catalog-local pins; deleted and malformed references refused; LRC-LIB-COMPARE: the Info panel labels the active photo while culling, #2398; KEYC-RATING: `\` shows / hides the Filter Bar in the grids; LR-VIEW-PHOTOGRID: the grid reads only the photos an import batch or an edit touched, not the whole list; earlier: LRC-PRINT-PAGE: Print Page Options; LR-META-XMP: fewer false unimported-XMP warnings; KEYC-METADATA: ⌘K / Ctrl+K Add Keywords; LRC-DEV-HISTOGRAM-DRAG: the histogram's five tonal zones — hover for the value, drag to adjust the Basic control the press locked, double-click to reset; LR-VIEW-HISTOGRAM note updated; MENU-BAR: on macOS a menu item that yields its key to a focused text field or to shortcut recording really lets it go; LR-MERGE-HDR / LR-MERGE-PANO: Lanczos-3 warps; LR-IMP-LOCAL: browsing from the sidebar records the folder; LR-VIEW-ZOOM: only a full-size drag within 20 ms clears a slow-drag verdict; LR-IMP-ADD-DIALOG: hierarchical import album destination reconciled with main, #2362; KEY-SLIDER-NUDGE: vertical wheel nudges need a click or drag in the row; leaving it disarms them; LR-EDIT-OPTICS-PROFILE: Fujifilm nine-knot lens tables read at the plain radius; LR-VIEW-ZOOM: only a full-size drag within 20 ms clears a slow-drag verdict; LRC-LIB-LIBFOLDERS, LR-BEHAV-BG-TASKS: Synchronize Folder's scan shows its folder listing and stops it on ✕; LRC-LIB-LIBFOLDERS: Synchronize Folder says a folder is up to date; LR-LIB-STACK: filmstrip stack badge click; LRC-LIB-REFVIEW / CATALOG-SWITCH: Escape preserves edits; catalog-local pins; deleted and malformed references refused; LRC-LIB-COMPARE: the Info panel labels the active photo while culling, #2398; KEYC-RATING: `\` shows / hides the Filter Bar in the grids; LR-VIEW-PHOTOGRID: the grid reads only the photos an import batch or an edit touched, not the whole list; earlier: LRC-PRINT-PAGE: Print Page Options; LR-META-XMP: fewer false unimported-XMP warnings) · **Target:** Adobe Lightroom Classic 15.5 / Lightroom 9.6 +> **Last reviewed:** 2026-10-10 · **Last updated:** 2026-10-25 · **Change:** minor (LRC-LIB-MISSING: `library.missing` from the control channel checks the files on a worker thread instead of the UI thread; LR-BEHAV-TITLEBAR: a double-click follows the system setting, and a panel over the bar keeps the window still; KEYC-METADATA, LRC-LIB-KEYWORDS: ⌥0 / ⌥⇧0 go to the next / previous keyword set; LRC-DEV-HISTOGRAM-DRAG: the histogram's five tonal zones — hover for the value, drag to adjust the Basic control the press locked, double-click to reset; LR-VIEW-HISTOGRAM note updated; MENU-BAR: on macOS a menu item that yields its key to a focused text field or to shortcut recording really lets it go; LR-MERGE-HDR / LR-MERGE-PANO: Lanczos-3 warps; LR-IMP-LOCAL: browsing from the sidebar records the folder; LR-VIEW-ZOOM: only a full-size drag within 20 ms clears a slow-drag verdict; LR-IMP-ADD-DIALOG: hierarchical import album destination reconciled with main, #2362; KEY-SLIDER-NUDGE: vertical wheel nudges need a click or drag in the row; leaving it disarms them; LR-EDIT-OPTICS-PROFILE: Fujifilm nine-knot lens tables read at the plain radius; LR-VIEW-ZOOM: only a full-size drag within 20 ms clears a slow-drag verdict; LRC-LIB-LIBFOLDERS, LR-BEHAV-BG-TASKS: Synchronize Folder's scan shows its folder listing and stops it on ✕; LRC-LIB-LIBFOLDERS: Synchronize Folder says a folder is up to date; LR-LIB-STACK: filmstrip stack badge click; LRC-LIB-REFVIEW / CATALOG-SWITCH: Escape preserves edits; catalog-local pins; deleted and malformed references refused; LRC-LIB-COMPARE: the Info panel labels the active photo while culling, #2398; KEYC-RATING: `\` shows / hides the Filter Bar in the grids; LR-VIEW-PHOTOGRID: the grid reads only the photos an import batch or an edit touched, not the whole list; earlier: LRC-PRINT-PAGE: Print Page Options; LR-META-XMP: fewer false unimported-XMP warnings; KEYC-METADATA: ⌘K / Ctrl+K Add Keywords; LRC-DEV-HISTOGRAM-DRAG: the histogram's five tonal zones — hover for the value, drag to adjust the Basic control the press locked, double-click to reset; LR-VIEW-HISTOGRAM note updated; MENU-BAR: on macOS a menu item that yields its key to a focused text field or to shortcut recording really lets it go; LR-MERGE-HDR / LR-MERGE-PANO: Lanczos-3 warps; LR-IMP-LOCAL: browsing from the sidebar records the folder; LR-VIEW-ZOOM: only a full-size drag within 20 ms clears a slow-drag verdict; LR-IMP-ADD-DIALOG: hierarchical import album destination reconciled with main, #2362; KEY-SLIDER-NUDGE: vertical wheel nudges need a click or drag in the row; leaving it disarms them; LR-EDIT-OPTICS-PROFILE: Fujifilm nine-knot lens tables read at the plain radius; LR-VIEW-ZOOM: only a full-size drag within 20 ms clears a slow-drag verdict; LRC-LIB-LIBFOLDERS, LR-BEHAV-BG-TASKS: Synchronize Folder's scan shows its folder listing and stops it on ✕; LRC-LIB-LIBFOLDERS: Synchronize Folder says a folder is up to date; LR-LIB-STACK: filmstrip stack badge click; LRC-LIB-REFVIEW / CATALOG-SWITCH: Escape preserves edits; catalog-local pins; deleted and malformed references refused; LRC-LIB-COMPARE: the Info panel labels the active photo while culling, #2398; KEYC-RATING: `\` shows / hides the Filter Bar in the grids; LR-VIEW-PHOTOGRID: the grid reads only the photos an import batch or an edit touched, not the whole list; earlier: LRC-PRINT-PAGE: Print Page Options; LR-META-XMP: fewer false unimported-XMP warnings) · **Target:** Adobe Lightroom Classic 15.5 / Lightroom 9.6 The measured, row-by-row checklist: one row per Lightroom feature, menu item and shortcut, what we have and what is missing. It answers *does the feature exist?* (feature breadth). Whether it is good @@ -717,7 +717,7 @@ From `08-lightroom-classic-extras.md` (Classic-only features) and part 2 of `06- | LRC-LIB-PEOPLE | People view | P2 | 🟡 | `cmd:view.people`, `cmd:view.person`, `cmd:faces.person`, `cmd:faces.unnamed`, `cmd:faces.nameFaces`, `crates/ui-egui/src/panels/people.rs`, `crates/ui-egui/src/panels/person.rs`, `crates/ui-egui/src/panels/unnamed.rs`, `crates/engine/src/face_tiles.rs` | Named People cards (face close-up, the photo count on the picture, name) for the photos the active filters let through, with the filter chips shown above them, sized by the thumbnail slider. Below them the **Unnamed faces**: every face nobody has named as a cropped picture, look-alikes next to each other once recognition has looked at them, a suggested name on a face the named ones recognise; click or Shift-click to select, type a name, Enter names the whole group in one undo step. A click on a person opens their page: only cropped faces, then **More** (unnamed faces that look like them, best first; click to confirm one, × to hide for the session, or ↑ / hovered-or-focused A key to confirm all currently shown non-hidden suggestions at or above that face's score, including ties, in one undo step). Every face is shown by the detector's box where the scan has one, so faces are equally close. Until recognition is set up a line at the top offers the next step (Settings ▸ Faces, or Turn on), as does the loupe's name box. Face pictures of unedited raws are cut from the largest embedded preview (planned read: header and JPEG only, never the sensor data) when it holds at least ¾ of the tile's pixels across the face, else developed as before; all are kept in the thumbnail disk cache at 16 px size steps, so a later launch shows them without reading the originals (#1440, `docs/faces.md`). No clusters with separators between groups, no persistent reject, no merge of two people | LRC-LIB-COMMENTS | Comments panel | P2 | ⬜ | | | | LRC-LIB-VISUALSEARCH | Find similar photos | P2 | ✅ | `cmd:library.findSimilar`, `crates/pipeline/src/cull.rs` (`signature`) | photo menu / Photo ▸ Find Similar Photos: look-alike photos (composition and tones), most similar first, as a filter (`only`) | -| LRC-LIB-MISSING | Missing files & relink | P1 | ✅ | `cmd:library.missing`, `cmd:photo.relink`, `cmd:library.findMissing`, `cmd:file.findMissing`, `cmd:photo.locate`, `crates/engine/src/cmd/missing.rs` (`checked_path`), `crates/ui-egui/src/panels/left.rs` (`locate_missing_item`) | File → Find Missing Photos… (anywhere in a folder: same name, size and content hash; renamed files by size + content hash; same name and size alone only when unambiguous — look-alikes are skipped and reported; searched on a worker thread, one undo step), photo menu → Locate Missing File…; a folder or disk row's Locate Missing Files… chooses its new location and scopes the same recursive background finder to that folder and its subfolders. Unreadable files show "!" in the grid and a reason in the loupe; undo never moves files. Missing Photos in the sidebar (counted on a worker thread; the view, the Info panel, the grid's context menu and thumbnails read file availability from a cache a worker thread fills, so an offline NAS never stalls a frame); it covers library photos only — Local browse records are never checked, for the count, the view, `library.missing` and Find Missing alike | +| LRC-LIB-MISSING | Missing files & relink | P1 | ✅ | `cmd:library.missing`, `cmd:photo.relink`, `cmd:library.findMissing`, `cmd:file.findMissing`, `cmd:photo.locate`, `crates/engine/src/cmd/missing.rs` (`checked_path`), `crates/ui-egui/src/missing_sweep.rs`, `crates/ui-egui/src/panels/left.rs` (`locate_missing_item`) | File → Find Missing Photos… (anywhere in a folder: same name, size and content hash; renamed files by size + content hash; same name and size alone only when unambiguous — look-alikes are skipped and reported; searched on a worker thread, one undo step), photo menu → Locate Missing File…; a folder or disk row's Locate Missing Files… chooses its new location and scopes the same recursive background finder to that folder and its subfolders. Unreadable files show "!" in the grid and a reason in the loupe; undo never moves files. Missing Photos in the sidebar (counted on a worker thread; the view, the Info panel, the grid's context menu and thumbnails read file availability from a cache a worker thread fills, so an offline NAS never stalls a frame); it covers library photos only — Local browse records are never checked, for the count, the view, `library.missing` and Find Missing alike; `library.missing` from the control channel or `mcp --connect` checks the files on a worker thread (a "Missing Photos" row in the activity stack, ✕ stops it) and answers when it ends, so a large library on a network share never stalls the window; the same request meanwhile shares the running check | | LRC-LIB-CONVERT | Convert to DNG | P2 | ✅ | `cmd:photo.convertToDng`, `crates/engine/src/cmd/convert.rs` | Photo ▸ Convert to DNG: lossless DNG next to the raw with the settings embedded, photo (and its virtual copies) relinked; originals kept; undoable; the DNG is decoded and compared with the raw data, written to a temp file, synced, read back and only then given a free name (never replacing a file) — a failed write leaves no DNG and the photo on its raw (issue #106) | | LRC-LIB-PREVIEWS | Build / discard previews | P1 | ✅ | `cmd:library.buildPreviews`, `cmd:library.previewProgress`, `cmd:library.cancelPreviews`, `cmd:library.clearPreviews`, `crates/engine/src/cmd/previews.rs`, `crates/preview/src/lib.rs` | File ▸ Previews: build standard-sized (Settings → Performance size) or 1:1 previews of the selected / visible photos in the background (a row in the activity stack, ✕ to stop), discard the cache; disk thumbnail + view cache; no smart previews (offline proxies) | | LRC-LIB-SLIDESHOW-IMPROMPTU | Impromptu slideshow | P2 | ✅ | `cmd:view.slideshow` | see LR-VIEW-SLIDESHOW | @@ -959,3 +959,4 @@ Latin stays upright. Advanced Japanese composition remains open. Regression cove | 2026-10-23 | patch | LR-BEHAV-ACCESS: the name field of New Album, New Smart Album, Rename and the other one-field dialogs takes the keyboard focus once when the dialog opens instead of every frame, so Tab and Shift+Tab reach the check box, Cancel and OK; status unchanged (🟡) | | 2026-10-22 | minor | LR-CROP-AUTO: Auto straighten turns by at most 10° and only on at least 3 agreeing lines, and says when it finds nothing to level by; measured on 68 photos straightened by hand and 300 left as shot, unwanted turns of more than 1° fell from 79 to 19 of the 300. Downgraded ✅ → 🟡: it still leaves most tilted photos alone | | 2026-10-23 | patch | LRC-LIB-FILTERBAR: showing an album (`library.source`) while the filter holds an album drops the filter's album, as showing a library folder already dropped its folder filter; before, the old album stayed in the filter unseen (the filter bar has no album chip) and narrowed the next source such as All Photos; the row's note says so; status unchanged (✅) | +| 2026-10-22 | patch | LRC-LIB-MISSING: `library.missing` from the control channel (an agent, `mcp --connect`) checks the photos' files on a worker thread under a "Missing Photos" row in the activity stack (counts the files, ✕ stops it) and is answered when the check ends, instead of freezing the window while the UI thread looked at every file (minutes on a large library on a network share); status unchanged (✅) | diff --git a/docs/whats-new.md b/docs/whats-new.md index c6c259200..100f1e741 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -50,6 +50,14 @@ stay available in All Metadata; PSD edit copies keep their capture date. Original exports with Remove Location Info enabled are refused when a PSD or PSB embeds GPS, since LightCraft cannot strip metadata from a byte-for-byte original copy. +### Asking for the missing photos no longer freezes the window +- `library.missing` (the control channel and `mcp --connect`) looked at every photo's file on the thread that draws + the window, so on a library of hundreds of thousands of photos on a network share the app stopped answering for + minutes. The files are now checked in the background: a "Missing Photos" row in the activity stack counts them and + its ✕ stops the check, and the request is answered with the same list when the check ends. A second request meanwhile + shares the running check. A caller that stops waiting (the control server gives a request 60 s) gets the answer of + that check when it asks again within ten minutes, instead of another walk of the share. + ### Choose where a Lightroom catalog goes - File ▸ Import Lightroom Catalog… now asks first: merge the catalog's photos, collections and edits into the library open now, or create a new library for them, named after the catalog and placed next to the open library From d3e0c090bfcbd0e2acf94229425296c8b27b6c92 Mon Sep 17 00:00:00 2001 From: Nathaniel Ledford Date: Thu, 22 Oct 2026 20:28:14 -0400 Subject: [PATCH 12/23] LRC-LIB-MISSING: honour a stop during the last check; reject a check of an earlier catalog; fail only the library's own requests --- crates/engine/src/cmd/missing.rs | 15 ++++- crates/engine/src/tests/import.rs | 21 +++++++ crates/ui-egui/src/missing_sweep.rs | 55 +++++++++++++----- crates/ui-egui/src/tests/missing_sweep.rs | 71 +++++++++++++++++++++++ docs/whats-new.md | 4 +- 5 files changed, 149 insertions(+), 17 deletions(-) diff --git a/crates/engine/src/cmd/missing.rs b/crates/engine/src/cmd/missing.rs index 09e2131ef..3c1bd7799 100644 --- a/crates/engine/src/cmd/missing.rs +++ b/crates/engine/src/cmd/missing.rs @@ -52,6 +52,18 @@ pub fn checked(cat: &Catalog) -> Vec<(PhotoId, String)> { cat.photos().filter_map(|p| checked_path(p).map(|f| (p.id, f.to_string()))).collect() } +/// What [`checked`] would list, as one number, without building the list: the photos in scope and +/// the file each one points at, whatever their order. A caller that checked on its own thread +/// compares it before and after to know the answer is still of the catalog it is given to. +pub fn fingerprint(cat: &Catalog) -> u64 { + use std::hash::{DefaultHasher, Hash, Hasher}; + cat.photos().filter_map(|p| checked_path(p).map(|f| (p.id, f))).fold(0u64, |sum, entry| { + let mut h = DefaultHasher::new(); + entry.hash(&mut h); + sum.wrapping_add(h.finish()) + }) +} + /// How many files [`sweep`] checks between two reports of its progress. const PROGRESS_EVERY: usize = 64; @@ -76,7 +88,8 @@ pub fn sweep( gone.push((id, file)); } } - Some(gone) + // (a stop that came during the last check is honoured too) + (!stop()).then_some(gone) } /// `library.missing`'s answer: `[{id, path}]`. diff --git a/crates/engine/src/tests/import.rs b/crates/engine/src/tests/import.rs index 89b2dcafa..c8e2ce2da 100644 --- a/crates/engine/src/tests/import.rs +++ b/crates/engine/src/tests/import.rs @@ -1304,6 +1304,27 @@ fn missing_sweep_answers_what_the_command_does_and_can_stop() { assert_eq!((stopped, looked.load(Ordering::Relaxed)), (None, 10)); } +/// A stop that arrives while the last file is being checked is honoured: no answer. +#[test] +fn missing_sweep_honours_a_stop_during_the_last_check() { + use lightcraft_catalog::PhotoId; + use std::sync::atomic::{AtomicBool, Ordering}; + let stop = AtomicBool::new(false); + let files = vec![(PhotoId(1), "/a".to_string()), (PhotoId(2), "/b".to_string())]; + let swept = crate::cmd::missing::sweep( + files, + |f| { + if f == "/b" { + stop.store(true, Ordering::Relaxed); + } + false + }, + || stop.load(Ordering::Relaxed), + |_| {}, + ); + assert_eq!(swept, None); +} + /// `library.browse` follows `library.source`'s rule for the active photo (issue #1447): a library /// photo the folder doesn't show gives way to the folder's first photo; one it shows stays. #[test] diff --git a/crates/ui-egui/src/missing_sweep.rs b/crates/ui-egui/src/missing_sweep.rs index dd743b8ef..6ac45d1e2 100644 --- a/crates/ui-egui/src/missing_sweep.rs +++ b/crates/ui-egui/src/missing_sweep.rs @@ -38,9 +38,33 @@ const KEEP: Duration = Duration::from_secs(10 * 60); #[derive(Default)] pub struct State { /// Each request with the library ([`lightcraft_engine::Session::library_generation`]) it asked about. - waiting: Vec<(u64, Sender)>, - /// A finished check's answer that no request received: its library and when it was kept. - kept: Option<(u64, Instant, Value)>, + pub(crate) waiting: Vec<(u64, Sender)>, + /// A finished check's answer that no request received. + kept: Option, +} + +/// An answer nobody received: its library, the photos it was of ([`missing::fingerprint`]), when it was kept. +struct Kept { + library: u64, + snapshot: u64, + at: Instant, + answer: Value, +} + +impl State { + /// Send `answer` to every request waiting on `library`; `true` if any caller was still there. + /// (a request whose caller gave up has no one to send to) + fn answer(&mut self, library: u64, answer: &Value) -> bool { + let (theirs, others): (Vec<_>, Vec<_>) = std::mem::take(&mut self.waiting).into_iter().partition(|(l, _)| *l == library); + self.waiting = others; + theirs.into_iter().fold(false, |received, (_, waiting)| waiting.send(answer.clone()).is_ok() || received) + } + + /// A check for `library` could not start: fail its requests, and only those (another library's + /// check may be running for the others). + pub(crate) fn fail(&mut self, library: u64, error: &str) { + self.answer(library, &control::failure(error)); + } } /// Answer `library.missing` from the control channel: give the answer kept from a check nobody @@ -48,17 +72,19 @@ pub struct State { /// `reply` when the check ends. pub fn ask(app: &mut LightcraftApp, reply: Sender) -> Outcome { let library = app.session.library_generation(); - if let Some((kept_for, at, answer)) = app.missing.kept.take() - && kept_for == library - && at.elapsed() < KEEP + if let Some(kept) = app.missing.kept.take() + && kept.library == library + && kept.at.elapsed() < KEEP + && kept.snapshot == missing::fingerprint(&app.session.catalog) { - return Outcome::Done(answer); + return Outcome::Done(kept.answer); } app.missing.waiting.push((library, reply)); if app.tasks.is_running(LABEL) && app.missing.waiting.iter().filter(|(l, _)| *l == library).count() > 1 { return Outcome::Later; } let files = missing::checked(&app.session.catalog); + let snapshot = missing::fingerprint(&app.session.catalog); let total = files.len() as u64; let exists = app.session.media.availability.probe(); let cancel = Arc::new(AtomicBool::new(false)); @@ -75,22 +101,21 @@ pub fn ask(app: &mut LightcraftApp, reply: Sender) -> Outcome { _ if app.session.library_generation() != library => { (control::failure("Missing Photos: another library was opened before the check ended"), false) } + // (photos added, removed or relinked while the files were checked: this is not the catalog's answer) + Ok(_) if missing::fingerprint(&app.session.catalog) != snapshot => { + (control::failure("Missing Photos: the photos changed while the files were checked, ask again"), false) + } Err(e) => (control::failure(e), false), Ok(None) => (control::failure("Missing Photos: the check was stopped"), false), Ok(Some(gone)) => (control::success(missing::missing_json(gone)), true), }; - let (theirs, others): (Vec<_>, Vec<_>) = std::mem::take(&mut app.missing.waiting).into_iter().partition(|(l, _)| *l == library); - app.missing.waiting = others; - // (a request whose caller gave up has no one to send to) - let received = theirs.into_iter().fold(false, |received, (_, waiting)| waiting.send(answer.clone()).is_ok() || received); + let received = app.missing.answer(library, &answer); if complete && !received { - app.missing.kept = Some((library, Instant::now(), answer)); + app.missing.kept = Some(Kept { library, snapshot, at: Instant::now(), answer }); } }; if let Err(e) = crate::tasks::spawn_tracked(app, LABEL, KIND, cancel, work, done) { - for (_, waiting) in std::mem::take(&mut app.missing.waiting) { - let _ = waiting.send(control::failure(&e)); - } + app.missing.fail(library, &e); } Outcome::Later } diff --git a/crates/ui-egui/src/tests/missing_sweep.rs b/crates/ui-egui/src/tests/missing_sweep.rs index eea6fc0a5..4cde807f0 100644 --- a/crates/ui-egui/src/tests/missing_sweep.rs +++ b/crates/ui-egui/src/tests/missing_sweep.rs @@ -171,3 +171,74 @@ fn a_check_that_panics_is_answered_with_an_error() { let rx = ask(&mut h); assert_eq!(answer(&mut h, &rx)["result"], json!([])); } + +/// Photos added while the files are being checked were not in that check: its answer is not given as +/// the library's current one, and is not kept for a later request either. +#[test] +fn a_check_of_an_earlier_catalog_is_not_answered_as_current() { + let (mut h, share) = app_on_a_slow_share(Duration::from_secs(3)); + let rx = ask(&mut h); + for _ in 0..3 { + h.step(); + } + let p = + Photo::new(PhotoId(PHOTOS + 1), Source::File { path: "/share/IMG_9999.jpg".into() }, "IMG_9999.jpg", "JPEG", 60, 40, "2026-01-01T00:00:00"); + h.app.session.catalog.apply(Op::AddPhoto { photo: Box::new(p) }).unwrap(); + share.open.store(true, Ordering::SeqCst); + let reply = answer(&mut h, &rx); + assert_eq!(reply["ok"], false, "{reply}"); + assert!(reply["error"].as_str().is_some_and(|e| e.contains("changed")), "{reply}"); + assert!(h.step_until(T, |h| h.app.tasks.is_empty())); + let calls = share.calls.load(Ordering::SeqCst); + let again = ask(&mut h); + let reply = answer(&mut h, &again); + assert_eq!(reply["ok"], true, "{reply}"); + assert!(share.calls.load(Ordering::SeqCst) > calls, "checked afresh, not given the stale answer"); +} + +/// An edit that leaves the photos' files alone (a rating) does not spoil the check. +#[test] +fn an_edit_that_leaves_the_files_alone_does_not_spoil_the_check() { + let (mut h, share) = app_on_a_slow_share(Duration::from_secs(3)); + let rx = ask(&mut h); + for _ in 0..3 { + h.step(); + } + h.app.session.catalog.apply(Op::SetRating { id: PhotoId(1), rating: 3 }).unwrap(); + share.open.store(true, Ordering::SeqCst); + assert_eq!(answer(&mut h, &rx)["ok"], true); +} + +/// A check that can't start for one library fails only the requests for that library. +#[test] +fn a_failed_start_fails_only_that_librarys_requests() { + let mut state = crate::missing_sweep::State::default(); + let (a_tx, a_rx) = std::sync::mpsc::channel(); + let (b_tx, b_rx) = std::sync::mpsc::channel(); + state.waiting.push((1, a_tx)); + state.waiting.push((2, b_tx)); + state.fail(2, "Missing Photos: could not start"); + assert!(b_rx.try_recv().is_ok_and(|v| v["ok"] == false)); + assert!(a_rx.try_recv().is_err(), "the other library's request keeps waiting"); + assert_eq!(state.waiting.len(), 1); +} + +/// An answer kept for a request that never came is not given once the photos have changed. +#[test] +fn a_kept_answer_is_not_given_after_the_photos_changed() { + let (mut h, share) = app_on_a_slow_share(Duration::from_secs(3)); + drop(ask(&mut h)); + for _ in 0..3 { + h.step(); + } + share.open.store(true, Ordering::SeqCst); + assert!(h.step_until(T, |h| h.app.tasks.is_empty())); + let p = + Photo::new(PhotoId(PHOTOS + 1), Source::File { path: "/share/IMG_0000x.jpg".into() }, "IMG_0000x.jpg", "JPEG", 60, 40, "2026-01-01T00:00:00"); + h.app.session.catalog.apply(Op::AddPhoto { photo: Box::new(p) }).unwrap(); + let calls = share.calls.load(Ordering::SeqCst); + let again = ask(&mut h); + let reply = answer(&mut h, &again); + assert_eq!(reply["result"].as_array().map(Vec::len), Some(11), "{reply}"); + assert!(share.calls.load(Ordering::SeqCst) > calls); +} diff --git a/docs/whats-new.md b/docs/whats-new.md index 100f1e741..86d1c190b 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -56,7 +56,9 @@ minutes. The files are now checked in the background: a "Missing Photos" row in the activity stack counts them and its ✕ stops the check, and the request is answered with the same list when the check ends. A second request meanwhile shares the running check. A caller that stops waiting (the control server gives a request 60 s) gets the answer of - that check when it asks again within ten minutes, instead of another walk of the share. + that check when it asks again within ten minutes, instead of another walk of the share. If photos are added, removed + or relinked while the files are being checked, that answer is not given as current: the request is told to ask + again. ### Choose where a Lightroom catalog goes - File ▸ Import Lightroom Catalog… now asks first: merge the catalog's photos, collections and edits into the From 7e9a810ff4e927f28f88ca8d4af7f7780dd04578 Mon Sep 17 00:00:00 2001 From: Nathaniel Ledford Date: Thu, 22 Oct 2026 20:43:58 -0400 Subject: [PATCH 13/23] LRC-LIB-MISSING: answer a check over changing photos with what still holds instead of refusing it --- crates/engine/src/cmd/missing.rs | 27 ++++++++-- crates/ui-egui/src/missing_sweep.rs | 11 ++-- crates/ui-egui/src/tests/missing_sweep.rs | 61 +++++++++++++++++++---- docs/whats-new.md | 7 +-- 4 files changed, 82 insertions(+), 24 deletions(-) diff --git a/crates/engine/src/cmd/missing.rs b/crates/engine/src/cmd/missing.rs index 3c1bd7799..e5a17ab5d 100644 --- a/crates/engine/src/cmd/missing.rs +++ b/crates/engine/src/cmd/missing.rs @@ -52,18 +52,35 @@ pub fn checked(cat: &Catalog) -> Vec<(PhotoId, String)> { cat.photos().filter_map(|p| checked_path(p).map(|f| (p.id, f.to_string()))).collect() } -/// What [`checked`] would list, as one number, without building the list: the photos in scope and -/// the file each one points at, whatever their order. A caller that checked on its own thread -/// compares it before and after to know the answer is still of the catalog it is given to. -pub fn fingerprint(cat: &Catalog) -> u64 { +fn fingerprint_of<'a>(entries: impl Iterator) -> u64 { use std::hash::{DefaultHasher, Hash, Hasher}; - cat.photos().filter_map(|p| checked_path(p).map(|f| (p.id, f))).fold(0u64, |sum, entry| { + entries.fold(0u64, |sum, entry| { let mut h = DefaultHasher::new(); entry.hash(&mut h); sum.wrapping_add(h.finish()) }) } +/// What [`checked`] lists, as one number (the photos in scope and the file each one points at, +/// whatever their order), without building the list. A caller that took `checked` and checked its +/// files on another thread compares it with [`fingerprint_of_checked`] to know whether the photos +/// changed meanwhile. +pub fn fingerprint(cat: &Catalog) -> u64 { + fingerprint_of(cat.photos().filter_map(|p| checked_path(p).map(|f| (p.id, f)))) +} + +/// [`fingerprint`] of the list [`checked`] gave. +pub fn fingerprint_of_checked(files: &[(PhotoId, String)]) -> u64 { + fingerprint_of(files.iter().map(|(id, f)| (*id, f.as_str()))) +} + +/// What of `gone` (a sweep's answer, of the catalog as it was) still holds in `cat` as it is now: the +/// photos still in scope that still point at the file that was not found. A photo removed, moved to +/// Recently Deleted or relinked since is not reported as missing; one added since was not checked. +pub fn still_missing(cat: &Catalog, gone: Vec<(PhotoId, String)>) -> Vec<(PhotoId, String)> { + gone.into_iter().filter(|(id, file)| cat.photo(*id).and_then(|p| checked_path(p)) == Some(file.as_str())).collect() +} + /// How many files [`sweep`] checks between two reports of its progress. const PROGRESS_EVERY: usize = 64; diff --git a/crates/ui-egui/src/missing_sweep.rs b/crates/ui-egui/src/missing_sweep.rs index 6ac45d1e2..1afd0a280 100644 --- a/crates/ui-egui/src/missing_sweep.rs +++ b/crates/ui-egui/src/missing_sweep.rs @@ -84,7 +84,7 @@ pub fn ask(app: &mut LightcraftApp, reply: Sender) -> Outcome { return Outcome::Later; } let files = missing::checked(&app.session.catalog); - let snapshot = missing::fingerprint(&app.session.catalog); + let snapshot = missing::fingerprint_of_checked(&files); let total = files.len() as u64; let exists = app.session.media.availability.probe(); let cancel = Arc::new(AtomicBool::new(false)); @@ -101,13 +101,12 @@ pub fn ask(app: &mut LightcraftApp, reply: Sender) -> Outcome { _ if app.session.library_generation() != library => { (control::failure("Missing Photos: another library was opened before the check ended"), false) } - // (photos added, removed or relinked while the files were checked: this is not the catalog's answer) - Ok(_) if missing::fingerprint(&app.session.catalog) != snapshot => { - (control::failure("Missing Photos: the photos changed while the files were checked, ask again"), false) - } Err(e) => (control::failure(e), false), Ok(None) => (control::failure("Missing Photos: the check was stopped"), false), - Ok(Some(gone)) => (control::success(missing::missing_json(gone)), true), + Ok(Some(gone)) if missing::fingerprint(&app.session.catalog) == snapshot => (control::success(missing::missing_json(gone)), true), + // (photos changed while the files were checked, as an import does: the answer is what of the + // check still holds, and is not kept, for the next request must see the photos as they are then) + Ok(Some(gone)) => (control::success(missing::missing_json(missing::still_missing(&app.session.catalog, gone))), false), }; let received = app.missing.answer(library, &answer); if complete && !received { diff --git a/crates/ui-egui/src/tests/missing_sweep.rs b/crates/ui-egui/src/tests/missing_sweep.rs index 4cde807f0..62b15a9d1 100644 --- a/crates/ui-egui/src/tests/missing_sweep.rs +++ b/crates/ui-egui/src/tests/missing_sweep.rs @@ -172,28 +172,69 @@ fn a_check_that_panics_is_answered_with_an_error() { assert_eq!(answer(&mut h, &rx)["result"], json!([])); } -/// Photos added while the files are being checked were not in that check: its answer is not given as -/// the library's current one, and is not kept for a later request either. +fn new_photo(id: u64, path: &str) -> Box { + Box::new(Photo::new(PhotoId(id), Source::File { path: path.into() }, "x.jpg", "JPEG", 60, 40, "2026-01-01T00:00:00")) +} + +/// Photos change while the files are being checked (auto-import adds some every few seconds, a share check +/// takes minutes): the answer is the check's, kept to the photos that are still where it looked, so asking +/// again never has to be the answer. A photo that was missing and has been removed, moved to Recently +/// Deleted or relinked since is not reported; one added since was not checked and is not either. #[test] -fn a_check_of_an_earlier_catalog_is_not_answered_as_current() { +fn a_check_over_changing_photos_answers_what_still_holds() { let (mut h, share) = app_on_a_slow_share(Duration::from_secs(3)); let rx = ask(&mut h); for _ in 0..3 { h.step(); } - let p = - Photo::new(PhotoId(PHOTOS + 1), Source::File { path: "/share/IMG_9999.jpg".into() }, "IMG_9999.jpg", "JPEG", 60, 40, "2026-01-01T00:00:00"); - h.app.session.catalog.apply(Op::AddPhoto { photo: Box::new(p) }).unwrap(); + let cat = &mut h.app.session.catalog; + cat.apply(Op::RemovePhoto { id: PhotoId(1) }).unwrap(); + cat.apply(Op::SetDeleted { id: PhotoId(2), deleted: true }).unwrap(); + let relinked = + Op::Relink { id: PhotoId(3), file_name: "y.jpg".into(), source: Source::File { path: "/share/IMG_9998.jpg".into() }, format: None }; + cat.apply(relinked).unwrap(); + cat.apply(Op::AddPhoto { photo: new_photo(PHOTOS + 1, "/share/IMG_0000x.jpg") }).unwrap(); share.open.store(true, Ordering::SeqCst); let reply = answer(&mut h, &rx); - assert_eq!(reply["ok"], false, "{reply}"); - assert!(reply["error"].as_str().is_some_and(|e| e.contains("changed")), "{reply}"); + assert_eq!(reply["ok"], true, "{reply}"); + let ids: Vec = reply["result"].as_array().unwrap().iter().map(|r| r["id"].as_u64().unwrap()).collect(); + assert_eq!(ids, (4..=10).collect::>(), "{reply}"); +} + +/// A stop is a stop, whatever the photos did meanwhile. +#[test] +fn a_stopped_check_says_stopped_even_if_the_photos_changed() { + let (mut h, share) = app_on_a_slow_share(Duration::from_secs(3)); + let rx = ask(&mut h); + for _ in 0..3 { + h.step(); + } + h.app.session.catalog.apply(Op::AddPhoto { photo: new_photo(PHOTOS + 1, "/share/IMG_9999.jpg") }).unwrap(); + let id = missing_rows(&h)[0].id; + let r = h.request("engine.execute", json!({"command": "activity.cancel", "params": {"id": id}}), T); + assert_eq!(r["ok"], true, "{r}"); + share.open.store(true, Ordering::SeqCst); + let reply = answer(&mut h, &rx); + assert!(reply["error"].as_str().is_some_and(|e| e.contains("stopped")), "{reply}"); +} + +/// A check nobody waited for, over photos that changed while it ran, is not kept: the next request +/// checks afresh (and then sees the photo that came meanwhile). +#[test] +fn a_check_over_changed_photos_is_not_kept_for_a_later_request() { + let (mut h, share) = app_on_a_slow_share(Duration::from_secs(3)); + drop(ask(&mut h)); + for _ in 0..3 { + h.step(); + } + h.app.session.catalog.apply(Op::AddPhoto { photo: new_photo(PHOTOS + 1, "/share/IMG_0000x.jpg") }).unwrap(); + share.open.store(true, Ordering::SeqCst); assert!(h.step_until(T, |h| h.app.tasks.is_empty())); let calls = share.calls.load(Ordering::SeqCst); let again = ask(&mut h); let reply = answer(&mut h, &again); - assert_eq!(reply["ok"], true, "{reply}"); - assert!(share.calls.load(Ordering::SeqCst) > calls, "checked afresh, not given the stale answer"); + assert_eq!(reply["result"].as_array().map(Vec::len), Some(11), "{reply}"); + assert!(share.calls.load(Ordering::SeqCst) > calls, "checked afresh"); } /// An edit that leaves the photos' files alone (a rating) does not spoil the check. diff --git a/docs/whats-new.md b/docs/whats-new.md index 86d1c190b..279a8585b 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -56,9 +56,10 @@ minutes. The files are now checked in the background: a "Missing Photos" row in the activity stack counts them and its ✕ stops the check, and the request is answered with the same list when the check ends. A second request meanwhile shares the running check. A caller that stops waiting (the control server gives a request 60 s) gets the answer of - that check when it asks again within ten minutes, instead of another walk of the share. If photos are added, removed - or relinked while the files are being checked, that answer is not given as current: the request is told to ask - again. + that check when it asks again within ten minutes, instead of another walk of the share, as long as the photos have + not changed since; otherwise the files are checked again. If photos change while the files are being checked (an + import adds some, others are removed, moved to Recently Deleted or relinked), the answer leaves out the photos that + no longer apply, a photo added meanwhile is not part of that check, and the answer is not kept for a later request. ### Choose where a Lightroom catalog goes - File ▸ Import Lightroom Catalog… now asks first: merge the catalog's photos, collections and edits into the From 3d357617c4a6fa8c42ab6372e02c59a476d2bf33 Mon Sep 17 00:00:00 2001 From: Dominic Anaeto Date: Tue, 27 Oct 2026 18:05:24 -0400 Subject: [PATCH 14/23] CLI: cover invalid Windows arguments --- apps/lightcraft-cli/src/main.rs | 2 +- apps/lightcraft-cli/tests/it/cli.rs | 21 ++++++++++++++++++++- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/apps/lightcraft-cli/src/main.rs b/apps/lightcraft-cli/src/main.rs index a650ef748..70a9771b0 100644 --- a/apps/lightcraft-cli/src/main.rs +++ b/apps/lightcraft-cli/src/main.rs @@ -160,7 +160,7 @@ fn main() -> ExitCode { match arg.into_string() { Ok(arg) => args.push(arg), Err(_) => { - eprintln!("lightcraft-cli: argument {} is not valid UTF-8; use a UTF-8 encoded argument", index + 1); + eprintln!("lightcraft-cli: argument {} is not valid Unicode", index + 1); return ExitCode::FAILURE; } } diff --git a/apps/lightcraft-cli/tests/it/cli.rs b/apps/lightcraft-cli/tests/it/cli.rs index cb898005c..fef1e7331 100644 --- a/apps/lightcraft-cli/tests/it/cli.rs +++ b/apps/lightcraft-cli/tests/it/cli.rs @@ -174,7 +174,26 @@ fn non_utf8_argument_fails_without_a_panic() { let stderr = String::from_utf8_lossy(&output.stderr); assert_eq!(output.status.code(), Some(1), "{stderr}"); assert!(stderr.contains("argument 2"), "{stderr}"); - assert!(stderr.contains("UTF-8"), "{stderr}"); + assert!(stderr.contains("is not valid Unicode"), "{stderr}"); + assert!(!stderr.contains("panicked"), "{stderr}"); + assert!(!out.exists()); + assert!(Command::new(binary()).arg("--version").status().unwrap().success()); +} + +#[cfg(windows)] +#[test] +fn non_unicode_argument_fails_without_a_panic_on_windows() { + use std::ffi::OsString; + use std::os::windows::ffi::OsStringExt; + + let out = tmp("non-unicode-output.png"); + let _ = std::fs::remove_file(&out); + let name: Vec = "bad-".encode_utf16().chain([0xD800]).chain(".png".encode_utf16()).collect(); + let output = Command::new(binary()).arg("render").arg(OsString::from_wide(&name)).arg("-o").arg(&out).output().unwrap(); + let stderr = String::from_utf8_lossy(&output.stderr); + assert_eq!(output.status.code(), Some(1), "{stderr}"); + assert!(stderr.contains("argument 2"), "{stderr}"); + assert!(stderr.contains("is not valid Unicode"), "{stderr}"); assert!(!stderr.contains("panicked"), "{stderr}"); assert!(!out.exists()); assert!(Command::new(binary()).arg("--version").status().unwrap().success()); From a412dc1dbddc1c604c4dae7bb5d05182e2bf5acf Mon Sep 17 00:00:00 2001 From: Dominic Anaeto Date: Tue, 27 Oct 2026 18:25:33 -0400 Subject: [PATCH 15/23] LC-003: run CLI tests on Windows --- .github/workflows/ci.yml | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a309a9fb3..53b96e5aa 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -30,3 +30,14 @@ jobs: pkg-config libxkbcommon-dev libwayland-dev libx11-dev libxcb1-dev libxcursor-dev libxrandr-dev libxi-dev libfontconfig1-dev libfreetype-dev mesa-vulkan-drivers libgl1-mesa-dev libdbus-1-dev + + windows-cli: + uses: storytold/craft-common-actions/.github/workflows/gate.yml@main + with: + os: '["windows-latest"]' + command: cargo test --locked -p lightcraft-cli + craft-fonts-ref: b076417b5880bd4154bd22f00695b1d28c6bea83 + timeout-minutes: 90 + tools: "" + targets: "" + packages: "" From 647da76d20ca3e4fa494f5feea88b359bdc3d255 Mon Sep 17 00:00:00 2001 From: Dominic Anaeto Date: Wed, 28 Oct 2026 01:32:16 -0400 Subject: [PATCH 16/23] LC-003: keep Windows CI changes out of the fix --- .github/workflows/ci.yml | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 53b96e5aa..a309a9fb3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -30,14 +30,3 @@ jobs: pkg-config libxkbcommon-dev libwayland-dev libx11-dev libxcb1-dev libxcursor-dev libxrandr-dev libxi-dev libfontconfig1-dev libfreetype-dev mesa-vulkan-drivers libgl1-mesa-dev libdbus-1-dev - - windows-cli: - uses: storytold/craft-common-actions/.github/workflows/gate.yml@main - with: - os: '["windows-latest"]' - command: cargo test --locked -p lightcraft-cli - craft-fonts-ref: b076417b5880bd4154bd22f00695b1d28c6bea83 - timeout-minutes: 90 - tools: "" - targets: "" - packages: "" From f87e38efb5170d9388a5557c7a31592888d8a6f5 Mon Sep 17 00:00:00 2001 From: Brain Wang Date: Thu, 29 Oct 2026 10:08:26 +0800 Subject: [PATCH 17/23] Issue #3293: README no longer lists an MCP mask helper; masks go through run_command Co-Authored-By: Claude Sonnet 5.5 --- README.md | 3 ++- README.zh-hans.md | 2 +- README.zh-hant.md | 2 +- 3 files changed, 4 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 6c556e46f..3784da0ab 100644 --- a/README.md +++ b/README.md @@ -259,7 +259,8 @@ lightcraft --control 7980 ~/Pictures/trip - **Command registry.** `engine.commands` lists the available commands; `develop.controls` lists every slider's range, default and current value. - **MCP server.** `lightcraft-cli mcp` exposes the command registry to Claude (or any MCP client), alongside - helpers for import, query, develop, mask, render (returned as an image) and export. It runs headless, or attached + helpers for import, query, develop, render (returned as an image) and export. Masks have no helper of their own: + create and edit them through `run_command` (e.g. `mask.add`). It runs headless, or attached to the running app with screenshots, clicks and gestures. See [docs/mcp.md](docs/mcp.md). ```sh diff --git a/README.zh-hans.md b/README.zh-hans.md index fa782ba10..d00a97cad 100644 --- a/README.zh-hans.md +++ b/README.zh-hans.md @@ -241,7 +241,7 @@ lightcraft --control 7980 ~/Pictures/trip - **命令注册表。** `engine.commands` 列出可用命令；`develop.controls` 列出每个滑块的范围、 默认值和当前值。 - **MCP 服务器。** `lightcraft-cli mcp` 把命令注册表开放给 Claude（或任何 MCP 客户端）， - 同时提供导入、查询、显影、蒙版、渲染（以图像返回）和导出的辅助工具。它既能无界面运行，也能附着 + 同时提供导入、查询、显影、渲染（以图像返回）和导出的辅助工具。蒙版没有专门的辅助工具，请通过 `run_command` 使用（例如 `mask.add`）。它既能无界面运行，也能附着 到正在运行的应用上，支持截图、点击和手势。见 [docs/mcp.md](docs/mcp.md)。 ```sh diff --git a/README.zh-hant.md b/README.zh-hant.md index 07ac908d2..2927b217f 100644 --- a/README.zh-hant.md +++ b/README.zh-hant.md @@ -240,7 +240,7 @@ lightcraft --control 7980 ~/Pictures/trip - **指令註冊表。** `engine.commands` 列出可用的指令；`develop.controls` 列出每個滑桿的範圍、 預設值與目前值。 - **MCP 伺服器。** `lightcraft-cli mcp` 把指令註冊表開放給 Claude（或任何 MCP 用戶端）， - 同時提供匯入、查詢、顯影、遮色片、算繪（以影像回傳）與匯出的輔助工具。它可以無介面執行，也可以附掛 + 同時提供匯入、查詢、顯影、算繪（以影像回傳）與匯出的輔助工具。遮色片沒有專屬的輔助工具，請透過 `run_command` 使用（例如 `mask.add`）。它可以無介面執行，也可以附掛 到正在執行的應用程式上，支援截圖、點擊與手勢。詳見 [docs/mcp.md](docs/mcp.md)。 ```sh From bdf535e954f60e89a66a169cb075b291f7e39b34 Mon Sep 17 00:00:00 2001 From: NetterSilver <133535268+Silvertree2010@users.noreply.github.com> Date: Thu, 29 Oct 2026 12:42:30 +0000 Subject: [PATCH 18/23] Reject malformed base64 in MCP and segmask decoders Found while testing crates/mcp/src/base64.rs:20 (base64_decode) Co-Authored-By: Claude Haiku 5.5 Co-Authored-By: Claude Sonnet 5.5 --- crates/develop/src/segmask.rs | 14 +++++++++++++- crates/mcp/src/base64.rs | 18 +++++++++++++++++- 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/crates/develop/src/segmask.rs b/crates/develop/src/segmask.rs index 172d53fce..de0aede09 100644 --- a/crates/develop/src/segmask.rs +++ b/crates/develop/src/segmask.rs @@ -104,9 +104,14 @@ fn base64_encode(bytes: &[u8]) -> String { fn base64_decode(s: &str) -> Option> { let mut out = Vec::with_capacity(s.len() / 4 * 3); let (mut acc, mut bits) = (0u32, 0u32); + let mut padded = false; for c in s.bytes().filter(|c| !c.is_ascii_whitespace()) { if c == b'=' { - break; + padded = true; + continue; + } + if padded { + return None; } let v = B64.iter().position(|b| *b == c)? as u32; acc = (acc << 6) | v; @@ -117,6 +122,9 @@ fn base64_decode(s: &str) -> Option> { acc &= (1 << bits) - 1; } } + if bits == 6 { + return None; + } Some(out) } @@ -133,6 +141,10 @@ mod tests { assert_eq!(base64_encode(b"Man"), "TWFu"); assert_eq!(base64_encode(b"Ma"), "TWE="); assert!(base64_decode("TW!u").is_none()); + assert!(base64_decode("Z").is_none()); + assert!(base64_decode("Zm9vY").is_none()); + assert!(base64_decode("Zg==!!!not-base64").is_none()); + assert_eq!(base64_decode("Zg==\n").unwrap(), b"f"); } #[test] diff --git a/crates/mcp/src/base64.rs b/crates/mcp/src/base64.rs index 9c02ac327..12ccf45bb 100644 --- a/crates/mcp/src/base64.rs +++ b/crates/mcp/src/base64.rs @@ -20,9 +20,14 @@ pub fn base64_encode(bytes: &[u8]) -> String { pub fn base64_decode(text: &str) -> Option> { let mut out = Vec::with_capacity(text.len() / 4 * 3); let (mut acc, mut bits) = (0u32, 0u32); + let mut padded = false; for c in text.bytes().filter(|c| !c.is_ascii_whitespace()) { if c == b'=' { - break; + padded = true; + continue; + } + if padded { + return None; } let v = ALPHABET.iter().position(|&a| a == c)? as u32; acc = (acc << 6) | v; @@ -33,6 +38,9 @@ pub fn base64_decode(text: &str) -> Option> { acc &= (1 << bits) - 1; } } + if bits == 6 { + return None; + } Some(out) } @@ -57,4 +65,12 @@ mod tests { } assert!(base64_decode("not base64!").is_none()); } + + #[test] + fn rejects_malformed() { + assert!(base64_decode("Z").is_none()); + assert!(base64_decode("Zm9vY").is_none()); + assert!(base64_decode("Zg==!!!not-base64").is_none()); + assert_eq!(base64_decode("Zg==\n").unwrap(), b"f"); + } } From 54193e7cb8659ce03b74b0fb956e70764b958e60 Mon Sep 17 00:00:00 2001 From: Chad Elliott Date: Mon, 26 Oct 2026 15:18:46 -0500 Subject: [PATCH 19/23] test(cli): #3070 refused-connection tests use a free port, not 18437 The `--connect` failure-hint tests connected to 127.0.0.1:18437 and assumed nothing listened there, but agents give their apps' control servers ports in 18000-19999. With an app on 18437 the tests sent it `ui.inspect` and failed. They now bind 127.0.0.1:0, drop the listener and use that port, and check the `--control ` hint against it. Co-Authored-By: Claude Opus 5.5 --- apps/lightcraft-cli/tests/it/cli.rs | 31 +++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/apps/lightcraft-cli/tests/it/cli.rs b/apps/lightcraft-cli/tests/it/cli.rs index 8a11f5c9a..e1c9a7b64 100644 --- a/apps/lightcraft-cli/tests/it/cli.rs +++ b/apps/lightcraft-cli/tests/it/cli.rs @@ -502,6 +502,18 @@ fn snapshot_starts_without_a_gpu() { } } +/// A loopback port nothing listens on, for tests that need `--connect` to be refused (#3070). +/// A fixed port is not safe: agents give their apps' control servers ports in 18000–19999 (and +/// 7980 is the default), so a test could reach someone's running app and drive it. The OS hands +/// out a free port for `:0`; the listener is dropped so connecting to it is refused. Another +/// process could bind the port between the drop and the connect. That window is a few +/// milliseconds, and `:0` draws from the OS's ephemeral range (from 32768 on Linux, 49152 on +/// macOS and Windows), where nobody puts a control server (7980, 18000–19999): the standard +/// trade-off for a test that needs a refused connection. +fn unused_port() -> u16 { + std::net::TcpListener::bind("127.0.0.1:0").unwrap().local_addr().unwrap().port() +} + fn run_cli(args: &[&str], stdin: Option<&str>) -> (bool, Vec, String) { let mut child = Command::new(binary()).arg("run").args(args).stdin(Stdio::piped()).stdout(Stdio::piped()).stderr(Stdio::piped()).spawn().unwrap(); if let Some(text) = stdin { @@ -614,9 +626,10 @@ fn a_library_open_in_another_process_is_refused() { /// ADDR actually names — following the old fixed "7980" left the retry connecting elsewhere. #[test] fn connect_failure_hint_names_the_attempted_port() { - let (ok, _lines, err) = run_cli(&["--connect", "127.0.0.1:18437", "ui.inspect"], None); + let port = unused_port(); + let (ok, _lines, err) = run_cli(&["--connect", &format!("127.0.0.1:{port}"), "ui.inspect"], None); assert!(!ok); - assert!(err.contains("--control 18437"), "the hint must name the port it tried: {err}"); + assert!(err.contains(&format!("--control {port}")), "the hint must name the port it tried: {err}"); assert!(!err.contains("--control 7980"), "{err}"); } @@ -626,20 +639,22 @@ fn connect_failure_hint_names_the_attempted_port() { /// works; non-numeric, non-`ADDR`-shaped tokens (e.g. a real command id) keep falling through. #[test] fn connect_bare_port_is_treated_as_loopback_port() { + let port = unused_port(); + let hint = format!("--control {port}"); // Bare port: the failure hint names that port. - let (ok, _lines, err) = run_cli(&["--connect", "18437", "ui.inspect"], None); + let (ok, _lines, err) = run_cli(&["--connect", &port.to_string(), "ui.inspect"], None); assert!(!ok); - assert!(err.contains("--control 18437"), "bare port must reach the hint: {err}"); + assert!(err.contains(&hint), "bare port must reach the hint: {err}"); assert!(!err.contains("--control 7980"), "{err}"); // `:` still works (issue #167 case). - let (ok, _lines, err) = run_cli(&["--connect", "127.0.0.1:18437", "ui.inspect"], None); + let (ok, _lines, err) = run_cli(&["--connect", &format!("127.0.0.1:{port}"), "ui.inspect"], None); assert!(!ok); - assert!(err.contains("--control 18437"), "{err}"); + assert!(err.contains(&hint), "{err}"); // `--connect=` already worked (the `a.starts_with("--connect=")` arm); spot-check // the hint matches its port too. - let (ok, _lines, err) = run_cli(&["--connect=18437", "ui.inspect"], None); + let (ok, _lines, err) = run_cli(&[&format!("--connect={port}"), "ui.inspect"], None); assert!(!ok); - assert!(err.contains("--control 18437"), "{err}"); + assert!(err.contains(&hint), "{err}"); } /// The CLI logs warnings on stderr (#168): with `RUST_LOG=warn` an unknown GPU backend name is From e407ed0e3ddee3f27574dfe468aa32da6f6feea4 Mon Sep 17 00:00:00 2001 From: Chad Elliott Date: Thu, 29 Oct 2026 23:59:11 -0500 Subject: [PATCH 20/23] test(cli): #3070 the tests that release an ephemeral port before use take turns unused_port() lets its port go before the CLI connects, and the denoise-serve test does the same before starting its server: with several test threads the server could start on a port a refused-connection test had picked, which then reached it instead of being refused. The three tests now hold one lock for their whole run. Co-Authored-By: Claude Opus 5.5 --- apps/lightcraft-cli/tests/it/cli.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/apps/lightcraft-cli/tests/it/cli.rs b/apps/lightcraft-cli/tests/it/cli.rs index e1c9a7b64..03711a318 100644 --- a/apps/lightcraft-cli/tests/it/cli.rs +++ b/apps/lightcraft-cli/tests/it/cli.rs @@ -510,10 +510,21 @@ fn snapshot_starts_without_a_gpu() { /// milliseconds, and `:0` draws from the OS's ephemeral range (from 32768 on Linux, 49152 on /// macOS and Windows), where nobody puts a control server (7980, 18000–19999): the standard /// trade-off for a test that needs a refused connection. +/// +/// Within this binary the window is closed: the tests that take an ephemeral port and let it go +/// hold [`ephemeral_ports`], so `denoise-serve` can't start on a port a refused-connection test +/// picked while it runs. fn unused_port() -> u16 { std::net::TcpListener::bind("127.0.0.1:0").unwrap().local_addr().unwrap().port() } +/// Held by each test that picks an ephemeral port and releases it before use, for the whole test. +fn ephemeral_ports() -> std::sync::MutexGuard<'static, ()> { + static PORTS: std::sync::Mutex<()> = std::sync::Mutex::new(()); + // a test that failed while holding it doesn't fail the others + PORTS.lock().unwrap_or_else(std::sync::PoisonError::into_inner) +} + fn run_cli(args: &[&str], stdin: Option<&str>) -> (bool, Vec, String) { let mut child = Command::new(binary()).arg("run").args(args).stdin(Stdio::piped()).stdout(Stdio::piped()).stderr(Stdio::piped()).spawn().unwrap(); if let Some(text) = stdin { @@ -626,6 +637,7 @@ fn a_library_open_in_another_process_is_refused() { /// ADDR actually names — following the old fixed "7980" left the retry connecting elsewhere. #[test] fn connect_failure_hint_names_the_attempted_port() { + let _ports = ephemeral_ports(); let port = unused_port(); let (ok, _lines, err) = run_cli(&["--connect", &format!("127.0.0.1:{port}"), "ui.inspect"], None); assert!(!ok); @@ -639,6 +651,7 @@ fn connect_failure_hint_names_the_attempted_port() { /// works; non-numeric, non-`ADDR`-shaped tokens (e.g. a real command id) keep falling through. #[test] fn connect_bare_port_is_treated_as_loopback_port() { + let _ports = ephemeral_ports(); let port = unused_port(); let hint = format!("--control {port}"); // Bare port: the failure hint names that port. @@ -800,7 +813,8 @@ fn denoise_serve_runs_tiles_for_another_computer() { Command::new(binary()).args(["denoise-serve", "--models", models.to_str().unwrap(), "--token-env", "LC_TEST_NO_SUCH_VAR"]).output().unwrap(); assert!(!o.status.success() && String::from_utf8_lossy(&o.stderr).contains("LC_TEST_NO_SUCH_VAR")); - let port = std::net::TcpListener::bind("127.0.0.1:0").unwrap().local_addr().unwrap().port(); + let _ports = ephemeral_ports(); + let port = unused_port(); let addr = format!("127.0.0.1:{port}"); let token = "cli-test-token-0123456789"; let mut child = Command::new(binary()) From 7283694154c8766f1e0c2240918e15fcf6240b9e Mon Sep 17 00:00:00 2001 From: Bruno-Alumn <171815746+Bruno-Alumn@users.noreply.github.com> Date: Fri, 30 Oct 2026 19:41:24 +0100 Subject: [PATCH 21/23] Control: pointer and scroll gestures refuse numbers past any screen instead of queueing NaN positions --- crates/ui-egui/src/control.rs | 24 +++++++++++++++ crates/ui-egui/src/tests/lifecycle_037.rs | 37 ++++++++++++++++++++++- docs/control-protocol.md | 2 +- docs/whats-new.md | 7 +++++ 4 files changed, 68 insertions(+), 2 deletions(-) diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index caacbc590..25bc00812 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -600,6 +600,29 @@ pub fn export_active(app: &mut LightcraftApp, p: &Value) -> Result Result<(), String> { + fn check(method: &str, key: &str, v: &Value) -> Result<(), String> { + match v { + Value::Number(n) if !matches!(key, "count" | "steps" | "factor") => match n.as_f64() { + Some(x) if x.abs() <= MAX_GESTURE_NUMBER => Ok(()), + _ => Err(format!("{method}: `{key}` must be a number between -1000000 and 1000000")), + }, + Value::Array(a) => a.iter().try_for_each(|v| check(method, key, v)), + Value::Object(o) => o.iter().try_for_each(|(k, v)| check(method, k, v)), + _ => Ok(()), + } + } + check(method, "", p) +} + /// Resolve one queued synthetic-input control request against the **current** layout. /// /// Called by the app only after a completed UI pass newer than admission (plan §4.5). Widget rects @@ -607,6 +630,7 @@ pub fn export_active(app: &mut LightcraftApp, p: &Value) -> Result Result { + check_gesture_numbers(method, p)?; let s = |k: &str| p.get(k).and_then(Value::as_str); let f = |k: &str| p.get(k).and_then(Value::as_f64); match method { diff --git a/crates/ui-egui/src/tests/lifecycle_037.rs b/crates/ui-egui/src/tests/lifecycle_037.rs index bb6f9d860..eb24e9b7a 100644 --- a/crates/ui-egui/src/tests/lifecycle_037.rs +++ b/crates/ui-egui/src/tests/lifecycle_037.rs @@ -564,7 +564,7 @@ fn one_synthetic_batch_until_a_completed_pass_and_drops_wait_for_the_ui_phase() #[test] fn first_headless_frame_runs_logic_and_uses_its_simulated_clock() { - let mut h = Headless::new(app(), [800.0, 600.0], 1.0); + let mut h = Headless::new(self::app(), [800.0, 600.0], 1.0); assert_eq!(h.time(), 0.0); h.step(); assert!(h.app.styled, "the first frame at t=0 runs logic and installs fonts"); @@ -903,6 +903,41 @@ fn ui_scroll_carries_validated_unit_phase_and_source() { assert!(app.synthetic.is_empty()); } +/// A coordinate or delta too large for an `f32` became an infinite or NaN pointer position or +/// scroll delta: egui panicked on the NaN in debug builds ("Comparing NaN vectors"), and a release +/// build handed it to every widget. Such a gesture is refused and queues nothing. +#[test] +fn gestures_refuse_numbers_that_are_not_screen_positions() { + let (mut app, ctx) = (app(), egui::Context::default()); + app.widgets.push(("finite:test".into(), egui::Rect::from_min_size(egui::Pos2::ZERO, egui::vec2(20.0, 20.0)))); + app.image_rect = Some(egui::Rect::from_min_size(egui::Pos2::ZERO, egui::vec2(100.0, 100.0))); + let refused = [ + ("ui.click", serde_json::json!({"x": 1e308})), + ("ui.click", serde_json::json!({"y": -1e308})), + ("ui.move", serde_json::json!({"x": 1e300, "y": 1.0})), + ("ui.drag", serde_json::json!({"x": 1.0, "toY": 1e308})), + ("ui.dragWidget", serde_json::json!({"id": "finite:test", "dx": -1e308})), + ("ui.scroll", serde_json::json!({"dy": 1e308})), + ("ui.pointer", serde_json::json!({"events": [{"kind": "down", "x": 0.5}, {"kind": "move", "x": 1e308}]})), + ]; + for (method, p) in refused { + let e = crate::control::gesture_events(&mut app, &ctx, method, &p).err().unwrap_or_else(|| panic!("{method} {p} was accepted")); + assert!(e.starts_with(method) && e.contains("between"), "{e}"); + assert!(app.synthetic.is_empty(), "{method} {p} queued events"); + } + // ordinary values still work, and count / steps are still clamped rather than refused + crate::control::gesture_events(&mut app, &ctx, "ui.click", &serde_json::json!({"x": 10.0, "y": -5.0, "count": u64::MAX})).unwrap(); + crate::control::gesture_events(&mut app, &ctx, "ui.drag", &serde_json::json!({"x": 1.0, "toX": 999_999.0, "steps": 1e12})).unwrap(); + assert!(!app.synthetic.is_empty()); + + // through the whole control channel, as an agent sends it: an error, not a panic + let mut h = Headless::new(self::app(), [800.0, 600.0], 1.0); + h.step(); + let r = h.request("ui.click", serde_json::json!({"x": 1e308}), Duration::from_secs(5)); + assert_eq!(r["ok"], false, "{r}"); + h.step(); +} + #[test] fn synthetic_pointer_requests_bound_clicks_and_drag_steps() { let (mut app, ctx) = (app(), egui::Context::default()); diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 36294c787..52f32beaa 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -82,7 +82,7 @@ filmstrip is visible. | `ui.inspect` | — | UI state, window, `loupe` (`source`, `pending`, `failed` = why its render failed, else `null`, and `region: {full, window, pending}` when a zoomed view also renders the window on screen at no more than 100 % — `full` is the frame it is cut from, `window` its `[x, y, w, h]` — and `regionBefore` for the Before side of a Before/After view), canvas/image rects, `scroll: {grid, filmstrip}` (scroll offsets in points, `null` until drawn), active photo, selection, perf (`frameMs` = layout, `logicMs` = per-frame logic before it, `updateMs` = both, `maxUpdateMs`, `fps`, render queue …, `gpu` = adapter in use, `gpuReason` = why renders don't use the GPU, `gpuFallback` = latest render redone on the CPU and why — see `docs/gpu-pipeline.md`), status, memory (bytes per cache, see `library.memory`; plus stage caches — `budgetBytes`, `trimmed`, `sharedSourceBytes` (the one device copy of a big original) — and textures), `export: {running: {total, done, current} \| null, last}`, `notices` (warnings waiting to be shown, e.g. a damaged settings file; OK = `button:noticeOk`), `quitPrompt` (why quitting was stopped: tasks that would be cut short are running — `button:quitAnyway` cancels them and quits, `button:quitCancel` keeps working — or unsaved changes: `button:quitRetry` / `button:quitAnyway` / `button:quitCancel`), `import: {done, total, imported, cancelled} \| null` (an import runs on a worker thread), `tasks` (other background work: `Find Missing Photos`, `Auto Import`), `activity` (every long-running task in flight as the activity stack in the top-left corner shows it, same shape as `activity.list`; a row appears once its task is 0.5 s old, ids `activity:row:`, ✕ = `activity:cancel:`, `activity:more` expands past 3 rows), `fileDialogs` (commands waiting on a native file dialog, which runs off the UI thread; they run again with the answer when it closes), `copied` (the text the UI last put on the clipboard, `null` until something is copied) | | `ui.widgets` | `{filter?}` | On-screen widgets `{id, rect: [x, y, w, h]}` (screen points) | | `ui.clickWidget` / `ui.dragWidget` / `ui.hoverWidget` | `{id, count?, button?, fx?, fy?}` (`button: "right"` right-clicks; `count` is capped at 16) / `{id, toX?, toY?, dx?, dy?, steps?}` (steps are capped at 1000) / `{id, fx?, fy?}` | Real egui input on a widget (hover: the pointer rests on it, e.g. for preset/profile previews). `fx`/`fy` are fractions of the registered rect and may lie outside `[0,1]`; the resulting point must still be inside egui's clipped interaction rect, and a non-finite fraction or resolved point is rejected before any input is queued. Geometry comes from the latest completed layout; if the widget moves before the first event or before a press, or its latest clipped rect no longer contains the point, the request fails with an error instead of clicking another widget or retargeting. A target below an ordinary modal dialog may succeed as a no-op or a backdrop click, because egui routes that input to the dialog; a target covered by a non-modal overlay fails. The blocking library-problem/launch-list window is explicit: its dim takes the modal layer, but a background target fails instead of reporting a no-op success, and only that window's own controls (`button:library…`) pass. Its creation frame is shielded and redrawn with empty input like a prompt's, so background text/pointer input cannot act while the new modal areas settle. A quit prompt or settings notice is a real egui modal scope with a foreground backdrop. Only its own buttons (`button:quit…`, `button:noticeOk`) accept a `ui.clickWidget`; background targets fail, and background keystrokes, text/clipboard input, wheel/zoom input and AccessKit actions are dropped before panels see them. The prompt itself takes Tab/Shift+Tab, Enter/Space on the focused button, and Escape/Enter for the safe default Cancel; AccessKit Focus/Click works when targeted at a prompt button. Its first creation frame (including a Tasks→Unsaved or notice→next-notice transition) is shielded and redrawn with empty input, so a press/release in that frame, or a held press from before it, cannot act underneath it; a synthetic drag already in progress is canceled. Once settled, raw `ui.click`/`ui.drag`/`ui.pointer` and backdrop clicks are still delivered to the prompt backdrop as no-ops, and the prompt's own button clicks work normally. The area/layer check cannot see a different widget covering the point in the same layer. A `ui.clickWidget` whose widget disappears before its release also fails, after canceling the pointer press without a click or drag (the same rules apply when a click spans several host frames) | -| `ui.move` / `ui.click` / `ui.drag` | `{x, y, count?, button?}` / `{x, y, toX, toY, steps?}` | Raw pointer input, screen points (the same click and step caps apply) | +| `ui.move` / `ui.click` / `ui.drag` | `{x, y, count?, button?}` / `{x, y, toX, toY, steps?}` | Raw pointer input, screen points (the same click and step caps apply). In every pointer and scroll gesture a number other than `count` and `steps` must lie between −1,000,000 and 1,000,000, or the request is refused and queues nothing | | `ui.pointer` | `{events: [{kind: down\|drag\|up, x, y}], alt?, shift?, cmd?}` | Gesture in normalized image coordinates (Detail view) | | `ui.key` | `{key, cmd?, shift?, alt?, ctrl?}` | Key press; while a prompt is up only its Tab/Shift+Tab, Enter, Space and Escape are delivered, every other key is dropped | | `ui.text` | `{text}` | Text input | diff --git a/docs/whats-new.md b/docs/whats-new.md index c6c259200..efafd7215 100644 --- a/docs/whats-new.md +++ b/docs/whats-new.md @@ -1,5 +1,12 @@ # What's new in LightCraft +## 2026-10-30 + +### The control channel refuses pointer positions that aren't on any screen +- `ui.click`, `ui.move`, `ui.drag`, `ui.dragWidget`, `ui.pointer` and `ui.scroll` with a coordinate or delta far past + any screen (such as `1e308`) used to queue an infinite or NaN position. A debug build panicked on it and a release + build handed it to every widget. Numbers outside ±1,000,000 are now an error, and nothing is queued. + ## 2026-10-24 ### Relative-date filters and smart albums follow the clock From 8d256db7a0526b65594a8f4682d3a3383dbdfe1f Mon Sep 17 00:00:00 2001 From: Bruno-Alumn <171815746+Bruno-Alumn@users.noreply.github.com> Date: Fri, 30 Oct 2026 20:14:15 +0100 Subject: [PATCH 22/23] Gesture numbers are checked when the request is admitted, before it waits for a layout --- crates/ui-egui/src/app/control.rs | 2 ++ crates/ui-egui/src/control.rs | 5 +++-- crates/ui-egui/src/tests/lifecycle_037.rs | 7 +++++++ 3 files changed, 12 insertions(+), 2 deletions(-) diff --git a/crates/ui-egui/src/app/control.rs b/crates/ui-egui/src/app/control.rs index 525c5419c..2f608971b 100644 --- a/crates/ui-egui/src/app/control.rs +++ b/crates/ui-egui/src/app/control.rs @@ -53,6 +53,8 @@ impl LightcraftApp { if self.pending_gestures.len() >= MAX_PENDING_GESTURES { return Err(format!("{}: {MAX_PENDING_GESTURES} control gestures are already pending", method)); } + // refused at once, without waiting for a layout or taking a pending slot + crate::control::check_gesture_numbers(&method, ¶ms)?; let now = self.tick_now_ms(); self.pending_gestures.push(PendingGesture { method, diff --git a/crates/ui-egui/src/control.rs b/crates/ui-egui/src/control.rs index 25bc00812..4b704069c 100644 --- a/crates/ui-egui/src/control.rs +++ b/crates/ui-egui/src/control.rs @@ -607,8 +607,9 @@ const MAX_GESTURE_NUMBER: f64 = 1e6; /// ±[`MAX_GESTURE_NUMBER`]. A larger one turns into an infinite or NaN `f32` position or scroll /// delta: egui asserts on NaN positions, and a release build would hand them to every widget. /// `count` and `steps` are clamped where they are read instead, and `ui.zoom`'s `factor` (a -/// multiplier, not a position) is checked and clamped by its own branch. -fn check_gesture_numbers(method: &str, p: &Value) -> Result<(), String> { +/// multiplier, not a position) is checked and clamped by its own branch. Checked when a request +/// is admitted and again here, for callers that resolve a gesture directly. +pub(crate) fn check_gesture_numbers(method: &str, p: &Value) -> Result<(), String> { fn check(method: &str, key: &str, v: &Value) -> Result<(), String> { match v { Value::Number(n) if !matches!(key, "count" | "steps" | "factor") => match n.as_f64() { diff --git a/crates/ui-egui/src/tests/lifecycle_037.rs b/crates/ui-egui/src/tests/lifecycle_037.rs index eb24e9b7a..d994b9300 100644 --- a/crates/ui-egui/src/tests/lifecycle_037.rs +++ b/crates/ui-egui/src/tests/lifecycle_037.rs @@ -930,6 +930,13 @@ fn gestures_refuse_numbers_that_are_not_screen_positions() { crate::control::gesture_events(&mut app, &ctx, "ui.drag", &serde_json::json!({"x": 1.0, "toX": 999_999.0, "steps": 1e12})).unwrap(); assert!(!app.synthetic.is_empty()); + // refused when the request is admitted: no pending slot, no wait for a layout + let (tx, rx) = reply_channel(); + let e = app.admit_gesture(&ctx, "ui.click".into(), serde_json::json!({"x": 1e308}), tx).expect_err("refused at admission"); + assert!(e.contains("between"), "{e}"); + assert!(app.pending_gestures.is_empty()); + drop(rx); + // through the whole control channel, as an agent sends it: an error, not a panic let mut h = Headless::new(self::app(), [800.0, 600.0], 1.0); h.step(); From 057ef1fd27a545cf3d09e108c680ab1687d613c9 Mon Sep 17 00:00:00 2001 From: aiongg Date: Sat, 31 Oct 2026 02:17:56 +0000 Subject: [PATCH 23/23] Integration: #3006 use binary() in the non-finite --set test --- apps/lightcraft-cli/tests/it/cli.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/lightcraft-cli/tests/it/cli.rs b/apps/lightcraft-cli/tests/it/cli.rs index d74fe267a..ceebce2d3 100644 --- a/apps/lightcraft-cli/tests/it/cli.rs +++ b/apps/lightcraft-cli/tests/it/cli.rs @@ -223,7 +223,7 @@ fn render_subcommand() { assert!(String::from_utf8_lossy(&o.stderr).contains("unknown control")); // Values that parse as f64 but are not finite fail cleanly too. for bad in ["light.exposure=inf", "light.exposure=-inf", "light.exposure=NaN", "light.exposure=1e999"] { - let o = Command::new(BIN).args(["render", input.to_str().unwrap(), "-o", out.to_str().unwrap(), "--set", bad]).output().unwrap(); + let o = Command::new(binary()).args(["render", input.to_str().unwrap(), "-o", out.to_str().unwrap(), "--set", bad]).output().unwrap(); assert!(!o.status.success(), "{bad} must be refused"); assert!(String::from_utf8_lossy(&o.stderr).contains("not a finite number"), "{bad}"); }