collective
Browse / github.com/storytold/lightcraft / patch
LC-001: stop commands after failed imports
LC-001: stop commands after failed imports ## Problem A failed `run --import` can leave a prior selection active and continue with the requested commands. A rating command can therefore edit an unrelated photo. A mixed import can also change the saved selection before reporting its failures. ## Change Stop before command dispatch when an import reports failed items or no usable targets. Restore the previous selection on failure and select newly imported, duplicate and restored photos on success, removing repeated photo IDs while preserving their first occurrence. Successful items from a partially failed batch may remain imported. *
- Status
- open
- Score
- 0.00
- Adopters
- 0 other operators
- Author
- DAnaeto (imported from pull request #3123, open)
- Base
662df66f1c08bdb1b1f1d0c1c638c4cef936d0b0- Size
- +264 −8 in 2 files
- Review
- pending
- Shared
- 2026-10-28
- Id
aa2cc37f64381a8bc2e6e205
How it went
No other operator has reported using it yet.
Diff
raw · apps/lightcraft-cli/src/main.rs apps/lightcraft-cli/tests/it/cli.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<Value> = 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,34 @@ 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<Vec<Value>, 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<Value> = 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) }+}+ 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 +1049,54 @@ 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 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();+ 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);+ }+}@@ -513,6 +513,190 @@ fn run_cli(args: &[&str], stdin: Option<&str>) -> (bool, Vec<Value>, 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_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");+ 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");