From 35cb32be202f8aa961b877889c4db7c2d6ae1f63 Mon Sep 17 00:00:00 2001 From: JGF <44821497+bommerts@users.noreply.github.com> Date: Fri, 23 Oct 2026 17:41:19 -0400 Subject: [PATCH] fix(export): report skipped layers in doc_export_layers (#8491) (#8713) * fix(export): report skipped layers across automation surfaces * test(export): use the MCP visible_only parameter spelling * test(export): restore old adapter for hosted regression proof * Revert "test(export): restore old adapter for hosted regression proof" This reverts commit 397a5ffa2a583561068667af843f373d9905f9a7. * docs(export): qualify workspace-only response guarantees --- crates/automation/src/headless.rs | 10 ++++- crates/automation/src/rpc.rs | 53 +++++++++++++++++++++++ crates/automation/src/server.rs | 2 +- crates/automation/tests/it/mcp.rs | 37 ++++++++++++++++ crates/engine/src/file_cmds.rs | 57 +++++++++++++++++++------ crates/engine/src/file_cmds/tests.rs | 63 ++++++++++++++++++++++++++++ docs/control-protocol.md | 2 +- docs/development.md | 10 +++++ 8 files changed, 218 insertions(+), 16 deletions(-) diff --git a/crates/automation/src/headless.rs b/crates/automation/src/headless.rs index bed78c0f65..1430fb978d 100644 --- a/crates/automation/src/headless.rs +++ b/crates/automation/src/headless.rs @@ -171,10 +171,16 @@ impl Headless { _ => return Err(AutomationError::BadRequest("automation filesystem access is not granted: write authority is absent".into())), }; let mut outputs = workspace.export_sequence(dir)?; - let written = file_cmds::export_layers_with(&this.session, params, |name, bytes| { + let report = file_cmds::export_layers_report_with(&this.session, params, |name, bytes| { outputs.write(name, bytes).map_err(|e| photocraft_engine::EngineError::Other(e.to_string())) }); - exported(dir, written, &mut outputs) + let (written, skipped) = match report { + Ok(report) => (Ok(report.files), report.skipped), + Err(error) => (Err(error), Vec::new()), + }; + let mut result = exported(dir, written, &mut outputs)?; + result["skipped"] = json!(skipped); + Ok(result) }) } diff --git a/crates/automation/src/rpc.rs b/crates/automation/src/rpc.rs index a0923a6744..7d52291f42 100644 --- a/crates/automation/src/rpc.rs +++ b/crates/automation/src/rpc.rs @@ -724,6 +724,59 @@ mod tests { std::fs::remove_dir_all(base).unwrap(); } + #[test] + fn doc_export_layers_reports_skips_in_workspace_and_trusted_sessions() { + for trusted in [false, true] { + let base = std::env::temp_dir().join(format!("pc-export-skips-{}-{trusted}", std::process::id())); + let _ = std::fs::remove_dir_all(&base); + std::fs::create_dir_all(&base).unwrap(); + let mut h = + if trusted { Headless::trusted_local() } else { Headless::with_workspace(crate::AuthorizedWorkspace::new(Some(&base), Some(&base)).unwrap()) }; + h.handle("doc.new", json!({"width": 4, "height": 4, "background": "white", "name": "card"})).unwrap(); + let adjustment = h.handle("engine.execute", json!({"command": "layer.newAdjustmentLayer.levels"})).unwrap()["layer"].clone(); + h.handle("engine.execute", json!({"command": "layer.setProps", "params": {"name": "Levels"}})).unwrap(); + let hidden = h.handle("engine.execute", json!({"command": "layer.new.layer", "params": {"name": "Hidden"}})).unwrap()["layer"].clone(); + h.handle("engine.execute", json!({"command": "layer.setProps", "params": {"visible": false}})).unwrap(); + // Exercise the actual JSON-lines serve envelope, not only direct dispatch. + let h = Mutex::new(h); + let out_dir = base.join("out").to_string_lossy().into_owned(); + let request = json!({"id": 8491, "method": "doc.exportLayers", "params": {"dir": out_dir}}).to_string() + "\n"; + let mut output = Vec::new(); + serve_lines(&h, request.as_bytes(), &mut output).unwrap(); + let reply: Value = serde_json::from_slice(&output).unwrap(); + assert_eq!(reply["id"], 8491); + assert_eq!(reply["ok"], true, "{reply}"); + assert_eq!(reply["result"]["files"], json!([format!("{out_dir}/card_0001_Background.png")])); + if !trusted { + assert_eq!(reply["result"]["count"], 1); + } + assert_eq!( + reply["result"]["skipped"], + json!([ + {"layer": hidden, "name": "Hidden", "reason": "hidden by visibleOnly"}, + {"layer": adjustment, "name": "Levels", "reason": "adjustment layer has no pixels of its own"} + ]) + ); + assert_eq!(std::fs::read_dir(base.join("out")).unwrap().count(), 1); + let mut h = h.into_inner().unwrap(); + let all_dir = base.join("all").to_string_lossy().into_owned(); + let all = h.export_layers(None, &all_dir, &json!({"visibleOnly": false})).unwrap(); + assert_eq!(all["files"].as_array().unwrap().len(), 2); + assert_eq!(all["skipped"].as_array().unwrap().len(), 1); + // Workspace no-overwrite errors still fail and remove only newly created files. + if !trusted { + let collision = base.join("collision"); + std::fs::create_dir_all(&collision).unwrap(); + std::fs::write(collision.join("card_0002_Background.png"), b"keep").unwrap(); + assert!(h.export_layers(None, &collision.to_string_lossy(), &json!({"visibleOnly": false})).is_err()); + assert_eq!(std::fs::read_dir(&collision).unwrap().count(), 1); + assert_eq!(std::fs::read(collision.join("card_0002_Background.png")).unwrap(), b"keep"); + } + drop(h); + std::fs::remove_dir_all(base).unwrap(); + } + } + #[test] fn doc_export_layers_writes_only_beneath_the_write_root() { let base = std::env::temp_dir().join(format!("pc-doc-export-layers-{}", std::process::id())); diff --git a/crates/automation/src/server.rs b/crates/automation/src/server.rs index 56d85509a6..1174c5f84f 100644 --- a/crates/automation/src/server.rs +++ b/crates/automation/src/server.rs @@ -645,7 +645,7 @@ impl PhotocraftMcp { } #[tool( - description = "Export each exported layer of the document as its own file beneath the write root (the capability-scoped Layers to Files). A file that already exists is an error, not an overwrite." + description = "Export top-level layers as files (groups merged). Returns files and skipped: [{layer, name, reason}] for hidden layers (visible_only) and adjustments with no pixels of their own. Workspace exports resolve beneath the write root, also return count of written files, and reject existing files instead of overwriting them. Trusted-local/bridge exports use the engine command's file access policy." )] async fn doc_export_layers(&self, Parameters(p): Parameters) -> Result { let mut opts = serde_json::Map::new(); diff --git a/crates/automation/tests/it/mcp.rs b/crates/automation/tests/it/mcp.rs index 88d62f6595..cd1e9b84d1 100644 --- a/crates/automation/tests/it/mcp.rs +++ b/crates/automation/tests/it/mcp.rs @@ -768,6 +768,43 @@ async fn doc_exports_and_data_set_import_are_capability_scoped() { cleanup(&base); } +/// Skipped-layer details survive the MCP tool result, including the all-skipped case. +#[tokio::test(flavor = "multi_thread")] +async fn doc_export_layers_reports_skipped_layers() { + let base = tmp("skipped-layers"); + let client = connect(headless_in(&base)).await; + json_of(&call(&client, "doc_new", json!({"width": 4, "height": 4, "background": "white", "name": "card"})).await); + let background = json_of(&call(&client, "doc_inspect", json!({})).await)["layers"][0]["id"].clone(); + let adjustment = json_of(&call(&client, "command_run", json!({"id": "layer.newAdjustmentLayer.levels"})).await)["layer"].clone(); + json_of(&call(&client, "command_run", json!({"id": "layer.setProps", "params": {"name": "Levels/α"}})).await); + let hidden = json_of(&call(&client, "command_run", json!({"id": "layer.new.layer", "params": {"name": "Hidden"}})).await)["layer"].clone(); + json_of(&call(&client, "command_run", json!({"id": "layer.setProps", "params": {"visible": false}})).await); + let result = json_of(&call(&client, "doc_export_layers", json!({"dir": "layers"})).await); + assert_eq!(result["count"], 1); + assert_eq!(result["files"], json!(["layers/card_0001_Background.png"])); + assert_eq!( + result["skipped"], + json!([ + {"layer": hidden, "name": "Hidden", "reason": "hidden by visibleOnly"}, + {"layer": adjustment, "name": "Levels/α", "reason": "adjustment layer has no pixels of its own"} + ]) + ); + let all = json_of(&call(&client, "doc_export_layers", json!({"dir": "all", "visible_only": false})).await); + assert_eq!(all["count"], 2); + assert_eq!(all["skipped"].as_array().unwrap().len(), 1); + assert_eq!(std::fs::read_dir(base.join("layers")).unwrap().count(), 1); + assert_eq!(std::fs::read_dir(base.join("all")).unwrap().count(), 2); + // Every top-level layer is still accounted for when none can be exported. + json_of(&call(&client, "command_run", json!({"id": "layer.setProps", "params": {"layer": background, "visible": false}})).await); + let none = json_of(&call(&client, "doc_export_layers", json!({"dir": "none"})).await); + assert_eq!(none["files"], json!([])); + assert_eq!(none["count"], 0); + assert_eq!(none["skipped"].as_array().unwrap().len(), 3); + assert_eq!(std::fs::read_dir(base.join("none")).unwrap().count(), 0); + client.cancel().await.unwrap(); + cleanup(&base); +} + /// The template workflow stays in memory, including saving nested contents back to the parent. #[tokio::test(flavor = "multi_thread")] async fn embedded_smart_objects_can_be_edited_saved_and_unpacked_without_file_authority() { diff --git a/crates/engine/src/file_cmds.rs b/crates/engine/src/file_cmds.rs index b5878fa3db..58dc42c951 100644 --- a/crates/engine/src/file_cmds.rs +++ b/crates/engine/src/file_cmds.rs @@ -1668,16 +1668,41 @@ fn flatten_all_masks(s: &mut Session) -> Result { fn layers_to_files(s: &mut Session, p: &Value) -> Result { let cmd = "file.export.layersToFiles"; let dir = str_param(p, "dir", cmd).or_else(|_| str_param(p, "output", cmd))?.to_string(); - let names = export_layers_with(s, p, |name, bytes| write_file(&join(&dir, name), bytes))?; - let files: Vec = names.iter().map(|name| join(&dir, name)).collect(); - Ok(json!({"files": files})) + let report = export_layers_report_with(s, p, |name, bytes| write_file(&join(&dir, name), bytes))?; + let files: Vec = report.files.iter().map(|name| join(&dir, name)).collect(); + Ok(json!({"files": files, "skipped": report.skipped})) } /// The bodies of `file.export.layersToFiles`, streamed to `write` as a single-component filename /// plus its bytes: one file per exported layer (top to bottom, like Photoshop's numbering), /// encoded but never touched as a path. Automation writes exactly these bytes beneath its write /// root (#3216), so the two paths cannot drift. Returns the filenames written, in order. -pub fn export_layers_with(s: &Session, p: &Value, mut write: impl FnMut(&str, &[u8]) -> Result<()>) -> Result> { +pub fn export_layers_with(s: &Session, p: &Value, write: impl FnMut(&str, &[u8]) -> Result<()>) -> Result> { + export_layers_report_with(s, p, write).map(|report| report.files) +} + +/// A top-level layer omitted from a successful Layers to Files export. +#[derive(Debug, serde::Serialize)] +pub struct SkippedExportLayer { + /// Session layer id (names need not be unique). + pub layer: u64, + /// Original name, not the sanitized filename component. + pub name: String, + /// Why the layer produced no file. + pub reason: &'static str, +} + +/// Filenames written and layers skipped, each in top-to-bottom order. +#[derive(Debug, Default)] +pub struct LayerExportReport { + pub files: Vec, + pub skipped: Vec, +} + +/// The same export as [`export_layers_with`], with an explanation for every omitted top-level +/// layer. Groups export merged, so their children are not separate exports or skips. Encoding +/// and write failures remain errors, never successful skips. Existing numbering is preserved. +pub fn export_layers_report_with(s: &Session, p: &Value, mut write: impl FnMut(&str, &[u8]) -> Result<()>) -> Result { let cmd = "file.export.layersToFiles"; let format = p.get("format").and_then(Value::as_str).unwrap_or("png").trim_start_matches('.').to_ascii_lowercase(); let visible_only = p.get("visibleOnly").and_then(Value::as_bool).unwrap_or(true); @@ -1688,24 +1713,32 @@ pub fn export_layers_with(s: &Session, p: &Value, mut write: impl FnMut(&str, &[ let d = s.active().ok_or(EngineError::NoDocument)?; let prefix = p.get("prefix").and_then(Value::as_str).map(str::to_string).unwrap_or_else(|| stem(&d.doc.name)); let doc = d.doc.clone(); - let mut written = Vec::new(); - // Top-level layers (groups export merged), top to bottom like Photoshop's numbering. - let layers: Vec<&Layer> = doc.layers.iter().rev().filter(|l| l.visible || !visible_only).collect(); - for (i, l) in layers.iter().enumerate() { + let mut report = LayerExportReport::default(); + // Hidden layers never consumed a filename index; adjustments that pass the visibility + // filter did. Keep those filenames stable while reporting both kinds of omission. + let mut i = 0usize; + for l in doc.layers.iter().rev() { + if !l.visible && visible_only { + report.skipped.push(SkippedExportLayer { layer: l.id.0, name: l.name.clone(), reason: "hidden by visibleOnly" }); + continue; + } + let index = i; + i += 1; if matches!(l.content, LayerContent::Adjustment(_)) { + report.skipped.push(SkippedExportLayer { layer: l.id.0, name: l.name.clone(), reason: "adjustment layer has no pixels of its own" }); continue; } let mut one = (*doc).clone(); - let mut only = (*l).clone(); + let mut only = l.clone(); only.visible = true; only.clipped = false; one.layers = vec![only]; - let name = format!("{}_{:04}_{}.{format}", sanitize(&prefix), i, sanitize(&l.name)); + let name = format!("{}_{:04}_{}.{format}", sanitize(&prefix), index, sanitize(&l.name)); let (bytes, _warnings) = encode(&one, &name, save)?; write(&name, &bytes)?; - written.push(name); + report.files.push(name); } - Ok(written) + Ok(report) } /// Adjustment layers that can be represented as an RGB-only LUT, in document stack order. diff --git a/crates/engine/src/file_cmds/tests.rs b/crates/engine/src/file_cmds/tests.rs index f034a62656..6dc73d1c54 100644 --- a/crates/engine/src/file_cmds/tests.rs +++ b/crates/engine/src/file_cmds/tests.rs @@ -1080,6 +1080,69 @@ fn flatten_all_layer_effects_keeps_a_pattern_overlay() { } } +#[test] +fn layers_to_files_reports_skipped_layers_without_changing_files() { + for depth in [8, 16, 32] { + let dir = tmp(&format!("skipped-layers-{depth}")); + let mut s = session(4, 4, depth); + let adjustment = s.execute("layer.newAdjustmentLayer.levels", json!({})).unwrap()["layer"].clone(); + s.execute("layer.setProps", json!({"name": "Levels/α"})).unwrap(); + let hidden = s.execute("layer.new.layer", json!({"name": "Levels/α"})).unwrap()["layer"].clone(); + s.execute("layer.setProps", json!({"visible": false})).unwrap(); + let before = s.active().unwrap().doc.clone(); + let result = s.execute("file.export.layersToFiles", json!({"dir": dir, "prefix": "doc"})).unwrap(); + assert_eq!( + result["skipped"], + json!([ + {"layer": hidden, "name": "Levels/α", "reason": "hidden by visibleOnly"}, + {"layer": adjustment, "name": "Levels/α", "reason": "adjustment layer has no pixels of its own"} + ]) + ); + assert_eq!(result["files"], json!([join(&dir, "doc_0001_Background.png")])); + assert_eq!(std::fs::read_dir(&dir).unwrap().count(), 1); + assert_eq!(s.active().unwrap().doc.layers.len(), before.layers.len()); + assert!(!s.active().unwrap().doc.layer(LayerId(hidden.as_u64().unwrap())).unwrap().visible); + let all = s.execute("file.export.layersToFiles", json!({"dir": dir, "prefix": "all", "visibleOnly": false})).unwrap(); + assert_eq!( + all["skipped"], + json!([ + {"layer": adjustment, "name": "Levels/α", "reason": "adjustment layer has no pixels of its own"} + ]) + ); + assert_eq!(all["files"], json!([join(&dir, "all_0000_Levels_α.png"), join(&dir, "all_0002_Background.png")])); + } +} + +#[test] +fn layers_to_files_reports_groups_as_exports_and_failures_as_errors() { + let mut s = session(4, 4, 8); + s.execute("layer.newAdjustmentLayer.levels", json!({})).unwrap(); + s.execute("layer.new.groupFromLayers", json!({"name": "Pack"})).unwrap(); + let params = json!({"prefix": "doc"}); + let mut names = Vec::new(); + let report = export_layers_report_with(&s, ¶ms, |name, bytes| { + assert!(!bytes.is_empty()); + names.push(name.to_string()); + Ok(()) + }) + .unwrap(); + assert_eq!(names, vec!["doc_0000_Pack.png", "doc_0001_Background.png"]); + assert_eq!(report.files, names); + assert!(report.skipped.is_empty(), "a group's adjustment is not an omitted top-level layer"); + assert_eq!(export_layers_with(&s, ¶ms, |_, _| Ok(())).unwrap(), names, "legacy Rust helper remains compatible"); + assert!(export_layers_report_with(&s, ¶ms, |_, _| Err(EngineError::Other("disk full".into()))).unwrap_err().to_string().contains("disk full")); + assert!(export_layers_report_with(&s, &json!({"format": "not-a-format"}), |_, _| panic!("must not write")).is_err()); + assert!(export_layers_report_with(&Session::new(), ¶ms, |_, _| panic!("must not write")).is_err()); + s.edit("Empty", |doc, _| { + doc.layers.clear(); + Ok(()) + }) + .unwrap(); + let empty = export_layers_report_with(&s, ¶ms, |_, _| panic!("must not write")).unwrap(); + assert!(empty.files.is_empty()); + assert!(empty.skipped.is_empty()); +} + #[test] fn layers_to_files_writes_each_visible_layer() { let dir = tmp("layers"); diff --git a/docs/control-protocol.md b/docs/control-protocol.md index 9e60af2136..07d3a09183 100644 --- a/docs/control-protocol.md +++ b/docs/control-protocol.md @@ -378,7 +378,7 @@ no MCP framing, no app start-up per command. Configure its file access with the | `session.list` | open documents and the active index | | `doc.open` / `doc.new` | `{path}` / `file.new` params | | `doc.place` | `{path, scale? (percent), fit? (true), center? ([x, y]), index?}`: places a file as a Smart Object layer above the active one and returns `{layer, scale, bounds}`. `path` resolves beneath the read root, so this is the capability-scoped `file.placeEmbedded` — that command itself still refuses an ambient path | -| `doc.exportLayers` | `{dir, format? (png), prefix?, visibleOnly? (true), quality?, index?}` → `{files, count}`: one file per exported layer, named like `file.export.layersToFiles` names them. `dir` resolves beneath the write root, so the ambient command stays refused. The bytes come from the command's own core, so both paths write the same files; the write capability refuses to overwrite an existing file | +| `doc.exportLayers` | `{dir, format? (png), prefix?, visibleOnly? (true), quality?, index?}` → `{files, skipped}` (plus `count` for workspace exports): one file per exported top-level layer, named like `file.export.layersToFiles` names them. `count` is present only for workspace exports and counts written files only. `skipped` is always an array (empty when nothing is omitted), in top-to-bottom order, of `{layer, name, reason}`: the session layer id, original unsanitized name, and `"hidden by visibleOnly"` or `"adjustment layer has no pixels of its own"`. Visibility takes precedence for hidden adjustments; `visibleOnly:false` exports hidden non-adjustment layers. Groups export merged; their children are not separate exports or skips. Existing filenames/numbering are unchanged. `dir` resolves beneath the write root, so the ambient command stays refused. The bytes come from the command's own core, so both paths write the same files; the write capability refuses to overwrite an existing file. Encode/write failures are errors, not skips | | `doc.exportDataSets` | `{dir, format? (png), dataSets? (all), naming? ("{name}\|{index}\|{document}"), index?}` → `{files, count}`: the capability-scoped `file.export.dataSetsAsFiles`. The batch is planned first and colliding names are refused before any file is written, as in the command | | `doc.importDataSets` | `{path, delimiter? (,), index?}` → `{imported}`: the capability-scoped `file.import.variableDataSets`; the CSV is read beneath the read root | | `doc.save` | `{path?, format?, quality?, index?, tiffLayers?, tgaBits?, ddsCompression?, ddsMipmaps?}` (`.pcraft` native, else export by extension; set `tiffLayers: true` to preserve TIFF layers rather than exporting a flattened TIFF; `tgaBits` 24 or 32 sets a TGA's bits per pixel, and `ddsCompression`/`ddsMipmaps` a DDS texture's compression and mip chain, as `app.save` does). Without `path` only a PSD, PSB or `.pcraft` document is written back to its own file, in its own format; anything else is an error and the file is left unchanged. A successful layered save (PSD, PSB or `.pcraft`) records the current revision as saved and makes the file the document's path, so `session.list` reports `dirty: false`; a flat export is a copy and leaves the document dirty | diff --git a/docs/development.md b/docs/development.md index b4cacd0a61..40b044fdd5 100644 --- a/docs/development.md +++ b/docs/development.md @@ -261,6 +261,16 @@ Tools: - `jobs_list` (running background jobs with progress, then recently finished ones with their result or error), `jobs_cancel` (one job by id, or every running job) - bridge only: `ui_inspect`, `ui_screenshot`, `ui_pointer`, `ui_menu_invoke`, `ui_set`, `control_call` +`doc_export_layers` in workspace headless mode returns `{files, count, skipped}`. `count` is +only the number of files written. Each skipped top-level layer is reported as +`{layer, name, reason}` (session id, original name, explanation); `skipped` is `[]` when every +layer exports. Hidden layers are reported when `visible_only` is true (the default; `visibleOnly` in the engine/serve params), and +standalone adjustments are reported because they have no pixels of their own. Groups still +export merged, so their children are not separate skips. Filename numbering is unchanged; +encoding or write failures remain errors. The engine command and trusted-local exports add +the same `skipped` array to their existing `{files}` response. See the +[`doc.exportLayers` protocol entry](control-protocol.md#headless-server) for details. + Claude Code (`.mcp.json` in the repo root, or `claude mcp add`): ```json