diff --git a/crates/ai/src/cli_agents/adapters.rs b/crates/ai/src/cli_agents/adapters.rs index 7a7df833..c3f03650 100644 --- a/crates/ai/src/cli_agents/adapters.rs +++ b/crates/ai/src/cli_agents/adapters.rs @@ -535,6 +535,12 @@ impl AgentAdapter for Codex { v.extend(["-c".into(), format!("mcp_servers.{MCP_NAME}.command={}", toml_string(&cx.mcp.command))]); let args_toml = format!("[{}]", cx.mcp.args.iter().map(|s| toml_string(s)).collect::>().join(", ")); v.extend(["-c".into(), format!("mcp_servers.{MCP_NAME}.args={args_toml}")]); + // `codex exec` can't ask, so without this it refuses a tool not marked read-only, such as + // rebuild_start, before our server sees the call (#540). Only tools that ask the person + // themselves are approved; every other tool keeps the person's own Codex policy. + for tool in craft_anything_engine::AGENT_APP_PREAPPROVED_TOOLS { + v.extend(["-c".into(), format!("mcp_servers.{MCP_NAME}.tools.{tool}.approval_mode=\"approve\"")]); + } if let Some(m) = &cx.options.cli_model { v.extend(["-m".into(), m.clone()]); } @@ -577,7 +583,9 @@ impl AgentAdapter for Codex { out.push(p.started(&id, &name, item.get("arguments").cloned().unwrap_or(Value::Null))); } let failed = item.get("status").and_then(Value::as_str) == Some("failed") || item.get("error").is_some_and(|e| !e.is_null()); - let text = item.get("result").or_else(|| item.get("error")).map(|r| r.to_string()).unwrap_or_default(); + let tool = item.get("tool").and_then(Value::as_str).unwrap_or_default(); + let ours = item.get("server").and_then(Value::as_str) == Some(MCP_NAME); + let text = codex_tool_text(&item, tool, ours); out.push(p.finished(&id, !failed, &text)); } ("item.started", "command_execution") => { @@ -600,6 +608,30 @@ impl AgentAdapter for Codex { } } +/// What a finished Codex tool call says. A failed call carries `"result": null` beside its +/// `error`, which showed as "null" (#540), so null fields are skipped and an error's message is +/// used as written. Codex's refusal to run one of our tools it can't ask about gets the way out. +fn codex_tool_text(item: &Value, tool: &str, ours: bool) -> String { + let field = |k: &str| item.get(k).filter(|v| !v.is_null()); + let error = field("error").map(|e| e.get("message").and_then(Value::as_str).map_or_else(|| e.to_string(), str::to_string)); + let text = match (field("result"), error) { + (_, Some(e)) => e, + (Some(r), None) => r.to_string(), + (None, None) => String::new(), + }; + // Another MCP server's refusal isn't fixed by our setup: it keeps Codex's own words. + if ours && text.contains(CODEX_NEEDS_APPROVAL) { + // The feed keeps 200 characters of a result, so the fix comes first and fits. + return format!( + "Codex blocked {tool}: it can't ask for approval here. Fix: run `craft-anything-cli setup --client codex --yes`, then retry. See docs/codex.md." + ); + } + text +} + +/// Codex's wording when a tool needs approval and its policy is `never` (#540). +const CODEX_NEEDS_APPROVAL: &str = "requires approval, but approval policy is never"; + /// Gemini CLI (`--output-format stream-json`). Flags are checked against `gemini --help`. pub struct GeminiCli; diff --git a/crates/ai/src/cli_agents/mod.rs b/crates/ai/src/cli_agents/mod.rs index b1d9f81a..f5876e00 100644 --- a/crates/ai/src/cli_agents/mod.rs +++ b/crates/ai/src/cli_agents/mod.rs @@ -905,6 +905,43 @@ mod tests { assert!(pos("--mcp-config") > pos("--resume")); } + /// #540: only tools that ask the person themselves are approved on Codex's side. + #[test] + fn every_tool_approved_for_codex_is_registered_and_asks_for_consent() { + let e = craft_anything_engine::Engine::shared(craft_anything_engine::testing::session(), crate::rebuild::tools::specs()); + for id in craft_anything_engine::AGENT_APP_PREAPPROVED_TOOLS { + let spec = e.registry().get(id).unwrap_or_else(|| panic!("{id} isn't registered")); + assert!(spec.effects.needs_consent(), "{id} must ask for consent itself"); + } + } + + /// #540: a refused Codex tool call came back as "null"; it says why and how to fix it. + #[test] + fn a_codex_tool_refused_for_approval_explains_the_way_out() { + let mut p = StreamParser::new(adapter_for("codex")); + let line = r#"{"type":"item.completed","item":{"id":"i1","type":"mcp_tool_call","server":"craft-anything","tool":"rebuild_start","status":"failed","result":null,"error":{"message":"tool rebuild_start requires approval, but approval policy is never"}}}"#; + let v: Value = serde_json::from_str(line).unwrap(); + let events = adapter_for("codex").parse(&mut p, &v); + let text = format!("{events:?}"); + assert!(text.contains("setup --client codex --yes"), "{text}"); + // Another server's refusal keeps Codex's words: our setup can't fix it. + let mut p = StreamParser::new(adapter_for("codex")); + let theirs = line.replace("craft-anything", "other-server"); + let text = format!("{:?}", adapter_for("codex").parse(&mut p, &serde_json::from_str::(&theirs).unwrap())); + assert!(text.contains("approval policy is never") && !text.contains("setup --client codex"), "{text}"); + let mut p = StreamParser::new(adapter_for("codex")); + let v: Value = serde_json::from_str(line).unwrap(); + let events = adapter_for("codex").parse(&mut p, &v); + let text = format!("{events:?}"); + assert!(!text.contains("\"null\""), "{text}"); + // Any other failure shows its own message, not the JSON around it. + let mut p = StreamParser::new(adapter_for("codex")); + let other = + r#"{"type":"item.completed","item":{"id":"i2","type":"mcp_tool_call","tool":"x","status":"failed","result":null,"error":{"message":"boom"}}}"#; + let text = format!("{:?}", adapter_for("codex").parse(&mut p, &serde_json::from_str::(other).unwrap())); + assert!(text.contains("boom") && !text.contains("null"), "{text}"); + } + #[test] fn codex_and_gemini_args() { let a = build_args(adapter_for("codex").as_ref(), &BackendOptions::default(), "q", "P:", &mcp(), None, Access::Analyse); @@ -919,6 +956,10 @@ mod tests { assert!(a.contains(&"mcp_servers.craft-anything.args=[\"mcp\", \"--project\", \"/p/x.cany\"]".to_string())); // #528: the message goes in on stdin; `-` tells `codex exec` to read it there. assert_eq!(a.last().unwrap(), "-"); + // #540: `codex exec` can't ask, so the tools that ask for themselves are approved in the + // same -c list, and nothing wider. + assert!(a.contains(&"mcp_servers.craft-anything.tools.rebuild_start.approval_mode=\"approve\"".to_string())); + assert!(!a.iter().any(|x| x.contains("default_tools_approval_mode"))); let g = build_args(adapter_for("gemini-cli").as_ref(), &BackendOptions::default(), "q", "", &mcp(), None, Access::Analyse); assert!(g.windows(2).any(|w| w[0] == "--output-format" && w[1] == "stream-json")); let o = BackendOptions { args: vec!["run".into()], ..Default::default() }; diff --git a/crates/engine/src/lib.rs b/crates/engine/src/lib.rs index e84faa61..f408f9f7 100644 --- a/crates/engine/src/lib.rs +++ b/crates/engine/src/lib.rs @@ -6,6 +6,12 @@ #![deny(clippy::unwrap_used, clippy::expect_used, clippy::panic, clippy::unimplemented, clippy::todo, clippy::unreachable)] pub mod clean_room; + +/// Tools an agent app may be told to approve on its own side (Codex's `approval_mode`): each one +/// asks the person itself (`Effects::needs_consent`), so approving it there skips nothing. +/// Everything else keeps the app's own policy; a test in the AI crate checks each is registered +/// and asks for consent. +pub const AGENT_APP_PREAPPROVED_TOOLS: &[&str] = &["rebuild_start"]; pub mod feed; pub mod inbox; pub mod journal; diff --git a/crates/setup/src/clients.rs b/crates/setup/src/clients.rs index aa991a83..33e9525b 100644 --- a/crates/setup/src/clients.rs +++ b/crates/setup/src/clients.rs @@ -139,6 +139,11 @@ pub enum Shape { Enabled, /// `{command, args, startup_timeout_sec: 30}` WithStartupTimeout, + /// `{command, args, startup_timeout_sec: 30, tools: {: {approval_mode: "approve"}}}`: + /// `codex exec` can't ask, so it refuses a tool not marked read-only unless the entry approves + /// it (#540). Only the tools that ask the person themselves are approved + /// ([`craft_anything_engine::AGENT_APP_PREAPPROVED_TOOLS`]); other settings in the entry are kept. + Codex, } /// One supported client, resolved for a host. @@ -199,10 +204,29 @@ impl Client { Shape::TransportStdio => json!({"transport": "stdio", "enabled": true, "command": program, "args": args}), Shape::Enabled => json!({"command": program, "args": args, "enabled": true}), Shape::WithStartupTimeout => json!({"command": program, "args": args, "startup_timeout_sec": 30}), + Shape::Codex => { + let tools: serde_json::Map = + craft_anything_engine::AGENT_APP_PREAPPROVED_TOOLS.iter().map(|t| ((*t).to_string(), json!({"approval_mode": "approve"}))).collect(); + json!({"command": program, "args": args, "startup_timeout_sec": 30, "tools": tools}) + } } } } +impl Client { + /// Whether `existing` already is what `desired` asks for. Codex's entry may carry the person's + /// own settings beside ours, so it only has to contain ours; every other client's must match. + pub fn entry_is_current(&self, existing: &serde_json::Value, desired: &serde_json::Value) -> bool { + fn contains(have: &serde_json::Value, want: &serde_json::Value) -> bool { + match (have, want) { + (serde_json::Value::Object(h), serde_json::Value::Object(w)) => w.iter().all(|(k, v)| h.get(k).is_some_and(|x| contains(x, v))), + _ => have == want, + } + } + if self.shape == Shape::Codex { contains(existing, desired) } else { existing == desired } + } +} + /// The command line of an existing entry (program + args), whatever its shape. pub fn entry_command(entry: &serde_json::Value) -> Option> { match entry.get("command")? { @@ -259,7 +283,7 @@ pub fn clients(host: &Host) -> Vec { // Codex let codex = host.var("CODEX_HOME").unwrap_or_else(|| h.join(".codex")); - add("codex", "Codex", codex.join("config.toml"), vec![codex.clone()], Format::Toml, vec!["mcp_servers"], Shape::WithStartupTimeout, None); + add("codex", "Codex", codex.join("config.toml"), vec![codex.clone()], Format::Toml, vec!["mcp_servers"], Shape::Codex, None); // Cursor add("cursor", "Cursor", h.join(".cursor").join("mcp.json"), vec![h.join(".cursor")], Format::Jsonc, vec!["mcpServers"], Shape::Plain, None); @@ -507,6 +531,8 @@ mod tests { assert_eq!(oc, json!({"type": "local", "command": ["/b/craft-anything-cli", "mcp"], "enabled": true})); let codex = cs.iter().find(|c| c.id == "codex").unwrap().desired_entry(&cmd); assert_eq!(codex["startup_timeout_sec"], 30); + assert_eq!(codex["tools"]["rebuild_start"]["approval_mode"], "approve"); + assert!(codex.get("default_tools_approval_mode").is_none(), "only the tools that ask for themselves"); for c in &cs { assert!(is_owned_entry(&c.desired_entry(&cmd)), "{}", c.id); } diff --git a/crates/setup/src/config.rs b/crates/setup/src/config.rs index da1d459d..65524703 100644 --- a/crates/setup/src/config.rs +++ b/crates/setup/src/config.rs @@ -1,6 +1,6 @@ //! Reading and editing one client's config text, per format. Pure text in, text out. -use crate::clients::{Client, Format, SERVER_NAME}; +use crate::clients::{Client, Format, SERVER_NAME, Shape, is_owned_entry}; use crate::yaml; use craft_anything_formats::jsonc; use serde_json::Value; @@ -48,6 +48,12 @@ pub fn is_disabled(client: &Client, text: &str) -> bool { parse(client, text).ok().and_then(|v| v.get("disabled_mcp_servers").and_then(|d| d.as_array()).map(|a| a.iter().any(|x| x == SERVER_NAME))).unwrap_or(false) } +/// True when the person turned our entry off themselves (Codex's `enabled = false`). Setup leaves +/// that as it is; the doctor says so. +pub fn entry_turned_off(client: &Client, text: &str) -> bool { + client.shape == Shape::Codex && read_entry(client, text).ok().flatten().is_some_and(|e| e.get("enabled") == Some(&Value::Bool(false))) +} + /// Text with our entry set to `entry`. pub fn set_entry(client: &Client, text: &str, entry: &Value) -> Result { match client.format { @@ -113,7 +119,40 @@ fn json_to_toml(v: &Value) -> Option { }) } +/// `want` set into `target` where it stands: a setting that already has the value is left alone, a +/// different one is replaced keeping its comment, and a table of ours is merged into the person's +/// (regular or inline) instead of replacing it, so their other settings and comments stay. +fn merge_in_place(target: &mut dyn toml_edit::TableLike, want: &serde_json::Map) -> Result<(), String> { + for (k, v) in want { + if let Value::Object(inner) = v + && let Some(child) = target.get_mut(k).and_then(|i| i.as_table_like_mut()) + { + merge_in_place(child, inner)?; + continue; + } + let mut new = json_to_toml(v).ok_or_else(|| format!("cannot write {k} as TOML"))?; + if let Some(have) = target.get(k).and_then(|i| i.as_value()) { + if have.to_string().trim() == new.to_string().trim() { + continue; + } + *new.decor_mut() = have.decor().clone(); + } + // Through the entry, so the key stays as written (a comment on the line above it is part of + // the key); `insert` would put in a fresh key. + match target.entry(k) { + toml_edit::Entry::Occupied(mut entry) => { + entry.insert(toml_edit::Item::Value(new)); + } + toml_edit::Entry::Vacant(entry) => { + entry.insert(toml_edit::Item::Value(new)); + } + } + } + Ok(()) +} + fn toml_set(client: &Client, text: &str, entry: &Value) -> Result { + let old = read_entry(client, text)?; let mut doc: toml_edit::DocumentMut = text.parse().map_err(|e: toml_edit::TomlError| e.to_string())?; let table_name = client.table.first().copied().unwrap_or("mcp_servers"); let Value::Object(fields) = entry else { return Err("entry must be a table".into()) }; @@ -128,16 +167,24 @@ fn toml_set(client: &Client, text: &str, entry: &Value) -> Result { - // Keep the existing table's position and decor; replace its keys. - existing.clear(); - for (k, v) in ours.iter() { - existing.insert(k, v.clone()); + // Only our own entry is merged into (a foreign one, such as an HTTP `url` Codex would refuse + // beside a `command`, is replaced, as the plan says it will be). The person's other settings + // and comments stay, whether the entry is a table or written inline. + let merge = client.shape == Shape::Codex && old.as_ref().is_some_and(is_owned_entry); + if merge && let Some(existing) = servers.get_mut(SERVER_NAME).and_then(|i| i.as_table_like_mut()) { + merge_in_place(existing, fields)?; + } else { + match servers.get_mut(SERVER_NAME).and_then(|i| i.as_table_mut()) { + Some(existing) => { + // Keep the existing table's position and decor; replace its keys. + existing.clear(); + for (k, v) in ours.iter() { + existing.insert(k, v.clone()); + } + } + None => { + servers.insert(SERVER_NAME, toml_edit::Item::Table(ours)); } - } - None => { - servers.insert(SERVER_NAME, toml_edit::Item::Table(ours)); } } if client.id == "grok_build" @@ -175,6 +222,92 @@ mod tests { assert!(parse(&c, "a = [").is_err()); } + /// #540: moving an older Codex entry to the approving one keeps what the person added to it. + #[test] + fn a_codex_entry_keeps_the_persons_own_settings_when_it_gains_the_approval() { + let c = client("codex"); + let text = "[mcp_servers.craft-anything]\ncommand = \"/old/craft-anything-cli\"\nargs = [\"mcp\"]\nenabled_tools = [\"core_a\"] # mine\ntools = { other = { approval_mode = \"prompt\" } }\n\n[mcp_servers.craft-anything.env]\nK = \"v\"\n"; + let e = c.desired_entry(&["/b/craft-anything-cli".into(), "mcp".into()]); + assert!(!c.entry_is_current(&read_entry(&c, text).unwrap().unwrap(), &e)); + let new = set_entry(&c, text, &e).unwrap(); + let got = read_entry(&c, &new).unwrap().unwrap(); + assert_eq!(got["command"], "/b/craft-anything-cli"); + assert_eq!(got["enabled_tools"][0], "core_a"); + assert_eq!(got["env"]["K"], "v"); + assert_eq!(got["tools"]["other"]["approval_mode"], "prompt", "their other approvals stay"); + assert_eq!(got["tools"]["rebuild_start"]["approval_mode"], "approve"); + assert!(got.get("default_tools_approval_mode").is_none()); + assert!(new.contains("# mine")); + assert!(c.entry_is_current(&got, &e), "their extra settings don't make it out of date"); + assert_eq!(set_entry(&c, &new, &e).unwrap(), new, "a second run changes nothing"); + } + + /// #540: a foreign entry (here an HTTP one) is replaced whole, not merged into, and an entry the + /// person turned off is reported as such. + #[test] + fn a_foreign_codex_entry_is_replaced_and_a_disabled_one_is_noticed() { + let c = client("codex"); + let e = c.desired_entry(&["/b/craft-anything-cli".into(), "mcp".into()]); + let foreign = "[mcp_servers.craft-anything]\nurl = \"https://example.test/mcp\"\n"; + let got = read_entry(&c, &set_entry(&c, foreign, &e).unwrap()).unwrap().unwrap(); + assert!(got.get("url").is_none(), "{got}"); + assert_eq!(got, e); + let off = set_entry(&c, "", &e).unwrap().replace("startup_timeout_sec", "enabled = false\nstartup_timeout_sec"); + assert!(entry_turned_off(&c, &off) && !entry_turned_off(&c, &set_entry(&c, "", &e).unwrap())); + assert!(c.entry_is_current(&read_entry(&c, &off).unwrap().unwrap(), &e), "setup keeps their choice"); + } + + /// #540: our own entry written inline keeps the person's settings too. + #[test] + fn an_inline_codex_entry_keeps_the_persons_own_settings() { + let c = client("codex"); + let e = c.desired_entry(&["/b/craft-anything-cli".into(), "mcp".into()]); + let text = "[mcp_servers]\ncraft-anything = { command = \"/old/craft-anything-cli\", args = [\"mcp\"], enabled = false, disabled_tools = [\"x\"] }\n"; + let new = set_entry(&c, text, &e).unwrap(); + let got = read_entry(&c, &new).unwrap().unwrap(); + assert_eq!(got["command"], "/b/craft-anything-cli"); + assert_eq!(got["enabled"], false); + assert_eq!(got["disabled_tools"][0], "x"); + assert_eq!(got["tools"]["rebuild_start"]["approval_mode"], "approve"); + assert_eq!(set_entry(&c, &new, &e).unwrap(), new, "a second run changes nothing"); + } + + /// #540: comments on an inline entry, in its fields and in a `tools` table stay. + #[test] + fn comments_on_our_codex_entry_and_its_tools_table_stay() { + let c = client("codex"); + let e = c.desired_entry(&["/b/craft-anything-cli".into(), "mcp".into()]); + let inline = "[mcp_servers]\ncraft-anything = { command = \"/old/craft-anything-cli\", args = [\"mcp\"], enabled = false } # mine\n"; + let new = set_entry(&c, inline, &e).unwrap(); + assert!(new.contains("# mine"), "{new}"); + let got = read_entry(&c, &new).unwrap().unwrap(); + assert_eq!((got["enabled"].clone(), got["command"].clone()), (Value::Bool(false), Value::from("/b/craft-anything-cli"))); + assert_eq!(got["tools"]["rebuild_start"]["approval_mode"], "approve"); + assert_eq!(set_entry(&c, &new, &e).unwrap(), new); + + let table = "[mcp_servers.craft-anything]\ncommand = \"/old/craft-anything-cli\" # the program\nargs = [\"mcp\"]\n\n# about tools\n[mcp_servers.craft-anything.tools]\n# keep me\nother = { approval_mode = \"prompt\" } # theirs\n"; + let new = set_entry(&c, table, &e).unwrap(); + for note in ["# the program", "# about tools", "# keep me", "# theirs"] { + assert!(new.contains(note), "{note} lost:\n{new}"); + } + let got = read_entry(&c, &new).unwrap().unwrap(); + assert_eq!(got["tools"]["other"]["approval_mode"], "prompt"); + assert_eq!(got["tools"]["rebuild_start"]["approval_mode"], "approve"); + assert_eq!(set_entry(&c, &new, &e).unwrap(), new, "a second run changes nothing"); + } + + /// #540: a comment on the line above a key stays when that key's value changes. + #[test] + fn a_comment_above_a_key_stays_when_its_value_changes() { + let c = client("codex"); + let e = c.desired_entry(&["/b/craft-anything-cli".into(), "mcp".into()]); + let text = "[mcp_servers.craft-anything]\n# the program\ncommand = \"/old/craft-anything-cli\"\nargs = [\"mcp\"]\n# how long to wait\nstartup_timeout_sec = 10\n"; + let new = set_entry(&c, text, &e).unwrap(); + assert!(new.contains("# the program\ncommand = \"/b/craft-anything-cli\""), "{new}"); + assert!(new.contains("# how long to wait\nstartup_timeout_sec = 30"), "{new}"); + assert_eq!(set_entry(&c, &new, &e).unwrap(), new, "a second run changes nothing"); + } + #[test] fn grok_enables_disabled_server() { let c = client("grok_build"); diff --git a/crates/setup/src/doctor.rs b/crates/setup/src/doctor.rs index f3be766f..90a26ec6 100644 --- a/crates/setup/src/doctor.rs +++ b/crates/setup/src/doctor.rs @@ -129,9 +129,12 @@ pub fn doctor(host: &Host, req: &DoctorRequest) -> DoctorReport { let program_ok = entry_command(&e).and_then(|v| v.into_iter().next()).is_some_and(|p| Path::new(&p).is_file()); if config::is_disabled(c, &text) { Check::new(name, CheckStatus::Warn, format!("{} lists Craft Anything as disabled.", c.display)).fix(setup_cmd) + } else if config::entry_turned_off(c, &text) { + Check::new(name, CheckStatus::Warn, format!("{} has Craft Anything turned off (enabled = false).", c.display)) + .fix(format!("Set enabled = true (or remove it) under [mcp_servers.craft-anything] in {}.", c.config.display())) } else if !program_ok { Check::new(name, CheckStatus::Fail, format!("{} points to a craft-anything-cli that no longer exists.", c.display)).fix(setup_cmd) - } else if desired.as_ref() == Some(&e) { + } else if desired.as_ref().is_some_and(|d| c.entry_is_current(&e, d)) { Check::new(name, CheckStatus::Ok, format!("{} is connected.", c.display)) } else { Check::new(name, CheckStatus::Warn, format!("{} is connected to a different copy or shape of the command.", c.display)).fix(setup_cmd) diff --git a/crates/setup/src/plan.rs b/crates/setup/src/plan.rs index d3dd89fd..8f0b84f3 100644 --- a/crates/setup/src/plan.rs +++ b/crates/setup/src/plan.rs @@ -159,7 +159,7 @@ pub fn inspect_client(c: &Client, command: &[String]) -> Action { return a; } }; - if existing.as_ref() == Some(&desired) && !config::is_disabled(c, &base) { + if existing.as_ref().is_some_and(|e| c.entry_is_current(e, &desired)) && !config::is_disabled(c, &base) { a.old_text = text; return a; } diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index a0c27d31..7212e1e3 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -316,8 +316,12 @@ Owner: open · Status: in progress - [ ] Aider has no adapter, so the plain one hands it the prompt as a file name, and it can open a browser sign-in nobody agreed to (#72). Give it an adapter, or stop offering it - [ ] Codex (#499): in a chat the tool server has no active target, tools turned on with - `enable_toolset` can't be called, and `rebuild_start` is refused by the approval policy. The + `enable_toolset` can't be called, The reporter attached a patch for chat continuity; look at that first +- [x] Codex (#540): `rebuild_start` was refused by `codex exec`'s approval policy and showed as + `null`; setup and the builder now approve our tools for Codex (we still ask before running + code) and a refusal explains the fix (`docs/codex.md`) +- [ ] Codex (#540): run `codex exec` on a real install and check `rebuild_start` gets through - [ ] Codex: resume support (`codex exec resume `), verified on a real install - [ ] Every adapter but Claude Code's still takes the message in argv (`prompt_on_stdin`), so a long one can pass Windows' command-line limit as Claude Code's did (#402); hand each its message on diff --git a/docs/STATUS.md b/docs/STATUS.md index a5ae02cc..4353a8b5 100644 --- a/docs/STATUS.md +++ b/docs/STATUS.md @@ -154,8 +154,8 @@ The full workspace gates (`cargo xtask ci`) passed in [hosted CI for #541](https with its writing tools, shell and code execution switched off, and Maki refuses anything else that would ask (an MCP server's tools, in subagents too) unless the person allowed it in Maki's own permissions. It hasn't been run as a `maki -p` builder here. -- Claude Code is fully wired as a builder; Codex works through its own flags but is unverified on - a real install and doesn't resume; other agent programs get plain text in and out. API-key +- Claude Code is fully wired as a builder; Codex works through its own flags (our tools are pre-approved for + `codex exec`, see `docs/codex.md`) but is unverified on a real install and doesn't resume; other agent programs get plain text in and out. API-key backends (Anthropic, OpenRouter, OpenAI, local; Kimi K3 through OpenRouter is offered by Detect; a backend chosen as the Extractor takes a Deep-mode game's own files out into `app/Original/` after the study) build with our own loop and the Build tools, trim and summarise long conversations and keep them in `transcript.json`; no live API build has been run yet. diff --git a/docs/codex.md b/docs/codex.md new file mode 100644 index 00000000..25480f76 --- /dev/null +++ b/docs/codex.md @@ -0,0 +1,63 @@ +# Using Codex with Craft Anything + +Craft Anything reaches Codex in two ways, and both have to get past Codex's own tool approval. + +## Why a tool can be refused + +`codex exec` is non-interactive: it has no one to ask, so its approval policy is `never`. A tool +that an MCP server doesn't mark read-only (such as `rebuild_start`, which runs a coding agent) is +refused before our server sees the call, with `requires approval, but approval policy is never`. +Codex's own settings decide this; Craft Anything's consent is a second, separate check. + +## What we set + +Only tools that ask the person themselves are approved on Codex's side +(`AGENT_APP_PREAPPROVED_TOOLS` in the engine, today just `rebuild_start`; a test checks each one +asks for consent). Every other tool keeps whatever approval policy you set in Codex. + +- **Connecting Codex to Craft Anything** (`craft-anything-cli setup --client codex --yes`, or the + Connect screen) writes the entry in `~/.codex/config.toml` (`$CODEX_HOME/config.toml` if set): + + ```toml + [mcp_servers.craft-anything] + command = "…/craft-anything-cli" + args = ["mcp"] + startup_timeout_sec = 30 + tools = { rebuild_start = { approval_mode = "approve" } } + ``` + + Your other entries and comments stay as they are, the old file is kept as + `config.toml.craft-anything.backup`, and settings you added to our entry (`env`, + `enabled_tools`, approvals for other tools) are kept when setup updates it. Setting + `approval_mode` for `rebuild_start` yourself is overwritten by the next setup run. + `craft-anything-cli doctor --client codex` reports an entry without the approval as "a different + copy or shape of the command"; running setup again fixes it. +- **Building with Codex from the app** starts `codex exec` with the same setting on its `-c` + flags, so nothing in your config is needed. + +## Who still asks + +Approving `rebuild_start` in Codex doesn't remove Craft Anything's own consent: it runs a coding +agent, so our server asks before it runs. Who can answer depends on how the server was started: + +- **Builds started from the app** run the server with the project's inbox, so the person answers + in the app (Allow / Don't allow). +- **The entry from `setup`** starts `craft-anything-cli mcp` on its own, with no app attached, so + there is no approval card: the call returns `consent_required` and nothing runs. To let Codex + start builds this way, add `"--allow-run"` to the entry's `args` (every tool that runs code is + then treated as allowed, so only do it for a Codex you trust), or start the build from the app. + +In Clean room mode the tools that touch the original stay refused either way. + +## If it still fails + +A refusal for one of our tools now says what was blocked and how to fix it, instead of showing +`null`; another MCP server's refusal keeps Codex's own words, because our setup can't change +that server's policy. To approve more of our tools by hand, add +`[mcp_servers.craft-anything.tools.]` with `approval_mode = "approve"` and start the chat +again. Approving all of them (`default_tools_approval_mode = "approve"`) also approves tools with +no consent check of their own, such as `project_inbox_answer`, which can answer a pending approval, +so we don't do it. + +Not yet verified on a real Codex install (this was written from Codex's published configuration +keys, tested with stand-in output).