mirror of
https://github.com/storytold/photocraft.git
synced 2026-10-24 09:57:08 -06:00
Address CodeRabbit review: bridge-mode doc_replace_smart_object refuses index (#8150)
The bridge forwards Replace Contents to the app, which runs it on its active document, so a given `index` was silently dropped and could replace another document's contents. It is now refused before anything reaches the app, as bridge previews and saves already do; documented in the schema and control-protocol.md. A fake-app test checks the indexed call never arrives. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5.5
parent
4604cfc4bb
commit
b54ab8ff30
@@ -76,7 +76,8 @@ pub struct PlaceParams {
|
||||
pub struct ReplaceSmartObjectParams {
|
||||
/// Path beneath the configured automation read root: relative (forward slashes) or absolute.
|
||||
pub path: String,
|
||||
/// Document index from `session_list` (default: the active document).
|
||||
/// Headless mode only: document index from `session_list` (default: the active document).
|
||||
/// Bridge mode rejects `index`.
|
||||
#[serde(default)]
|
||||
pub index: Option<usize>,
|
||||
/// The Smart Object layer: an id or a name (default: the active layer).
|
||||
@@ -668,6 +669,11 @@ impl PhotocraftMcp {
|
||||
if let Some(r) = self.headless_op(move |h| h.replace_smart_object(index, &path, &opts)).await {
|
||||
return to_result(r);
|
||||
}
|
||||
// The app's command runs on its active document: an `index` it cannot honour is refused
|
||||
// rather than dropped, or another document's contents would be replaced.
|
||||
if index.is_some() {
|
||||
return Ok(fail("`index` is only supported in headless mode; in bridge mode select the document in the app first"));
|
||||
}
|
||||
self.run_command("layer.smartObjects.replaceContents".into(), bridge_params, true).await
|
||||
}
|
||||
|
||||
|
||||
@@ -1074,6 +1074,26 @@ async fn bridge_previews_reject_document_indices() {
|
||||
assert_eq!(schema.get("additionalProperties"), Some(&json!(false)));
|
||||
}
|
||||
|
||||
/// Bridge mode runs Replace Contents on the app's active document, so an `index` is refused
|
||||
/// before anything reaches the app rather than dropped (it would replace another document's
|
||||
/// contents); without one the command is forwarded as before.
|
||||
#[tokio::test(flavor = "multi_thread")]
|
||||
async fn bridge_replace_smart_object_rejects_a_document_index() {
|
||||
let (addr, app) = fake_app().await;
|
||||
let client = connect(PhotocraftMcp::bridge(&addr, CONTROL_TOKEN).unwrap()).await;
|
||||
let refused = call(&client, "doc_replace_smart_object", json!({"path": "new.png", "index": 1})).await;
|
||||
assert_eq!(refused.is_error, Some(true), "{}", text(&refused));
|
||||
assert!(text(&refused).contains("index"), "{}", text(&refused));
|
||||
let forwarded = call(&client, "doc_replace_smart_object", json!({"path": "new.png", "layer": 7})).await;
|
||||
assert_ne!(forwarded.is_error, Some(true), "{}", text(&forwarded));
|
||||
client.cancel().await.unwrap();
|
||||
let seen = tokio::time::timeout(std::time::Duration::from_secs(5), app).await.unwrap().unwrap();
|
||||
let executed: Vec<&Value> = seen.iter().filter(|request| request["method"] == "engine.execute").collect();
|
||||
assert_eq!(executed.len(), 1, "the indexed call must not reach the app: {seen:?}");
|
||||
assert_eq!(executed[0]["params"]["command"], "layer.smartObjects.replaceContents");
|
||||
assert_eq!(executed[0]["params"]["params"], json!({"path": "new.png", "layer": 7}));
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread")]
|
||||
async fn headless_previews_preserve_document_indices() {
|
||||
let client = connect(PhotocraftMcp::headless()).await;
|
||||
|
||||
@@ -335,7 +335,7 @@ How each MCP tool maps onto control methods in bridge mode:
|
||||
| `doc_inspect` | `engine.execute {command: "document.inspect"}` |
|
||||
| `doc_open {path}` | `app.open {path}` |
|
||||
| `doc_place {path, …}` | `engine.execute {command: "file.placeEmbedded", params}` |
|
||||
| `doc_replace_smart_object {path, layer?, …}` | `engine.execute {command: "layer.smartObjects.replaceContents", params}`, which the desktop refuses (the command reads an ambient path, and no control method reads it through the automation root yet; `doc_place` is refused the same way). Use it headless or in the workspace server (`doc.replaceSmartObject`), where `path` resolves beneath the read root |
|
||||
| `doc_replace_smart_object {path, layer?, …}` | `engine.execute {command: "layer.smartObjects.replaceContents", params}`, which the desktop refuses (the command reads an ambient path, and no control method reads it through the automation root yet; `doc_place` is refused the same way). An `index` is refused in bridge mode instead of being dropped, since the command runs on the app's active document. Use it headless or in the workspace server (`doc.replaceSmartObject`), where `path` resolves beneath the read root |
|
||||
| `doc_export_layers {dir, …}` | `engine.execute {command: "file.export.layersToFiles", params}` |
|
||||
| `doc_export_data_sets {dir, …}` | `engine.execute {command: "file.export.dataSetsAsFiles", params}` |
|
||||
| `doc_import_data_sets {path, …}` | `engine.execute {command: "file.import.variableDataSets", params}` |
|
||||
|
||||
Reference in New Issue
Block a user