shifathosn
/
photocraft
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

automation: say why a command is disabled in engine.commands (#3740) 

engine.commands, command.list and the MCP command_list tool now give each
row a disabledReason (null when enabled, else the failed precondition) and
needsDocument (true when the command is off because no document is open),
so an agent can tell "open a document first" from "not runnable". enabled
keeps its meaning. Both listings share one row builder in the engine.

Fixes #3664

Co-authored-by: Fabio Lussi <198260595+itrycodinganditsbad@users.noreply.github.com>
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Fabio Lussi 2026-10-12 13:13:57 +02:00 committed by GitHub
parent 6751a46003
commit 3dd5fc1cdc
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 54 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

16
crates/automation/src/headless.rs
View File

@ -317,21 +317,7 @@ impl Headless {
}
pub fn command_list(&self) -> Value {
Value::Array(
command_specs()
.iter()
.map(|c| {
json!({
"id": c.id,
"label": c.label,
"menu": c.menu,
"shortcut": c.shortcut,
"params": c.params,
"enabled": self.session.is_enabled(c.id),
})
})
.collect(),
)
Value::Array(command_specs().iter().map(|c| photocraft_engine::commands::list_entry(&self.session, c, self.session.disabled_reason(c.id))).collect())
}
pub fn command_run(&mut self, id: &str, params: Value) -> Result<Value, AutomationError> {

4
crates/automation/src/server.rs
View File

@ -792,7 +792,9 @@ impl PhotocraftMcp {
}
}
#[tool(description = "List engine commands: id, label, menu path, shortcut, parameter doc, enabled now.")]
#[tool(description = "List engine commands: id, label, menu path, shortcut, parameter doc, enabled now. \
`disabledReason` says why a command is off (null when enabled); `needsDocument` is true when it is off because no \
document is open: create or open one first (doc_new, doc_open).")]
async fn command_list(&self, Parameters(p): Parameters<ListParams>) -> Result<CallToolResult, McpError> {
let all = if let Some(r) = self.headless_op(|h| Ok(h.command_list())).await {
r

6
crates/automation/tests/mcp.rs
View File

@ -170,6 +170,12 @@ async fn command_list_filters() {
let enabled = json_of(&call(&client, "command_list", json!({"enabled_only": true})).await);
assert!(enabled.as_array().unwrap().iter().any(|c| c["id"] == "file.new"));
assert!(!enabled.as_array().unwrap().iter().any(|c| c["id"] == "layer.new.layer"), "needs a document");
// The full list says why a document command is off with no document open (#3664).
let layer = all.as_array().unwrap().iter().find(|c| c["id"] == "layer.new.layer").unwrap();
assert_eq!((layer["enabled"].clone(), layer["needsDocument"].clone()), (json!(false), json!(true)), "{layer}");
assert!(layer["disabledReason"].as_str().is_some_and(|r| r.starts_with("no document")), "{layer}");
let new = all.as_array().unwrap().iter().find(|c| c["id"] == "file.new").unwrap();
assert_eq!((new["needsDocument"].clone(), new["disabledReason"].clone()), (json!(false), Value::Null), "{new}");
client.cancel().await.unwrap();
}

26
crates/engine/src/commands.rs
View File

@ -136,6 +136,19 @@ pub fn command_specs() -> &'static [CommandSpec] {
&registry().specs
}
/// One `command.list` / `engine.commands` row for `c`, given why it can't run now (`None` = it can).
/// `disabledReason` is that message; `needsDocument` is true when the reason is that no document is
/// open (#3664), so an agent can tell "open or create a document first" from a precondition that a
/// document alone won't meet (a selection, an empty clipboard).
pub fn list_entry(s: &Session, c: &CommandSpec, reason: Option<String>) -> Value {
let needs_document =
s.active().is_none() && reason.as_deref().is_some_and(|r| ["no document", "no active document", "no open document"].iter().any(|p| r.starts_with(p)));
json!({
"id": c.id, "label": c.label, "menu": c.menu, "shortcut": c.shortcut, "params": c.params,
"enabled": reason.is_none(), "disabledReason": reason, "needsDocument": needs_document,
})
}
pub fn find(id: &str) -> Option<&'static CommandSpec> {
let registry = registry();
registry.by_id.get(id).and_then(|&index| registry.specs.get(index))
@ -1014,18 +1027,7 @@ fn build() -> Vec<CommandSpec> {
shortcut: None,
params: "{}",
enabled: always,
run: |s, _| {
Ok(Value::Array(
command_specs()
.iter()
.map(|c| {
json!({
"id": c.id, "label": c.label, "menu": c.menu, "shortcut": c.shortcut, "params": c.params, "enabled": (c.enabled)(s).is_ok(),
})
})
.collect(),
))
},
run: |s, _| Ok(Value::Array(command_specs().iter().map(|c| list_entry(s, c, (c.enabled)(s).err())).collect())),
journal: false,
},
CommandSpec {

27
crates/engine/src/tests.rs
View File

@ -767,6 +767,33 @@ fn command_list_reports_enablement() {
assert_eq!(find("layer.new.layer")["enabled"], false);
}
/// With no document open, a document command says so instead of only `enabled: false` (#3664).
#[test]
fn command_list_tells_no_document_from_other_preconditions() {
let mut s = Session::new();
let list = s.execute("command.list", json!({})).unwrap();
let find = |list: &Value, id: &str| list.as_array().unwrap().iter().find(|c| c["id"] == id).unwrap().clone();
for id in ["paint.stroke", "layer.new.layer", "filter.blur.gaussianBlur"] {
let c = find(&list, id);
assert_eq!((c["enabled"].clone(), c["needsDocument"].clone()), (json!(false), json!(true)), "{id}: {c}");
assert!(c["disabledReason"].as_str().is_some_and(|r| r.starts_with("no document")), "{id}: {c}");
}
let new = find(&list, "file.new");
assert_eq!((new["enabled"].clone(), new["needsDocument"].clone(), new["disabledReason"].clone()), (json!(true), json!(false), Value::Null));
// A precondition that isn't the missing document keeps its own reason.
let paste = find(&list, "file.newFromClipboard");
assert_eq!((paste["enabled"].clone(), paste["needsDocument"].clone()), (json!(false), json!(false)));
assert_eq!(paste["disabledReason"], "the clipboard is empty");
// With a document, the same commands run; a missing selection is not a missing document.
let mut s = session_with_doc();
let list = s.execute("command.list", json!({})).unwrap();
assert_eq!(find(&list, "paint.stroke")["needsDocument"], false);
assert_eq!(find(&list, "paint.stroke")["disabledReason"], Value::Null);
let deselect = find(&list, "select.deselect");
assert_eq!((deselect["enabled"].clone(), deselect["needsDocument"].clone()), (json!(false), json!(false)));
assert_eq!(deselect["disabledReason"], "no selection");
}
#[test]
fn translate_moves_pixels_and_respects_locks() {
let mut s = session_with_doc();

6
docs/control-protocol.md
View File

@ -90,7 +90,7 @@ Change the UI language without restarting with `prefs.set`:
are documented in [UI localisation](localization.md). `prefs.get` and the existing preference
store expose and persist the same setting; scripts keep using canonical command IDs.
`engine.execute` runs any command by id. `engine.commands` (or the engine command `command.list`) lists them all, with labels, menu paths, shortcuts, a parameter description, and whether each is currently enabled. Examples:
`engine.execute` runs any command by id. `engine.commands` (or the engine command `command.list`) lists them all, with labels, menu paths, shortcuts, a parameter description, and whether each is currently enabled. Each row carries `enabled` (bool), `disabledReason` (`null` when enabled, otherwise the precondition that failed, such as `"no document open"`, `"no selection"` or `"the clipboard is empty"`) and `needsDocument` (true when the command is off because no document is open, so the first step is to create or open one; a command may still have its own precondition after that, such as an active text layer. False when enabled or off for another reason). With no document open, nearly every document command reports `enabled: false, needsDocument: true`; plan around `needsDocument` rather than reading `enabled` alone (#3664). Examples:
| Command | Params |
|---|---|
@ -263,7 +263,7 @@ How each MCP tool maps onto control methods in bridge mode:
|---|---|
| `command_run {id, params, wait?}` | `engine.execute {command: id, params, wait}` |
| `jobs_list` / `jobs_cancel {job?}` | `jobs.list` / `jobs.cancel {job?}` |
| `command_list {filter?, enabled_only?}` | `engine.commands` (filtered by the MCP server) |
| `command_list {filter?, enabled_only?}` | `engine.commands` (filtered by the MCP server; rows keep `disabledReason` and `needsDocument`) |
| `doc_new {…}` | `engine.execute {command: "file.new", params}` |
| `doc_inspect` | `engine.execute {command: "document.inspect"}` |
| `doc_open {path}` | `app.open {path}` |
@ -321,7 +321,7 @@ no MCP framing, no app start-up per command. Configure its file access with the
|---|---|
| `engine.execute` | `{command, params?, wait?}`: any engine command (`wait: false` starts a long one as a background job: `{job, pending}`) |
| `jobs.list` / `jobs.cancel` | `{}` / `{job?}`: background jobs; cancel one or all. Every request (and MCP tool call) first applies the jobs that finished, so `doc.save`, `doc.inspect`, `doc.render` and `session.list` include a finished job's result without polling `jobs.list` first |
| `engine.commands` | `{filter?}`: registry with params docs and enablement |
| `engine.commands` | `{filter?}`: registry with params docs and enablement (`enabled`, `disabledReason`, `needsDocument`) |
| `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 |