mirror of
https://github.com/storytold/craft-anything.git
synced 2026-11-05 20:47:45 +01:00
Approve Craft Anything's tools for Codex and explain a refused call instead of showing null (#553)
Fixes #540. Codex's codex exec can't ask for approval, so setup and the builder approve rebuild_start (which asks the person itself) on Codex's side, keeping the person's other settings in the entry; a refused call explains the fix instead of showing null.
This commit is contained in:
@@ -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::<Vec<_>>().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;
|
||||
|
||||
|
||||
@@ -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::<Value>(&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::<Value>(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() };
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -139,6 +139,11 @@ pub enum Shape {
|
||||
Enabled,
|
||||
/// `{command, args, startup_timeout_sec: 30}`
|
||||
WithStartupTimeout,
|
||||
/// `{command, args, startup_timeout_sec: 30, tools: {<tool>: {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<String, serde_json::Value> =
|
||||
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<Vec<String>> {
|
||||
match entry.get("command")? {
|
||||
@@ -259,7 +283,7 @@ pub fn clients(host: &Host) -> Vec<Client> {
|
||||
|
||||
// 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);
|
||||
}
|
||||
|
||||
+143
-10
@@ -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<String, String> {
|
||||
match client.format {
|
||||
@@ -113,7 +119,40 @@ fn json_to_toml(v: &Value) -> Option<toml_edit::Value> {
|
||||
})
|
||||
}
|
||||
|
||||
/// `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<String, Value>) -> 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<String, String> {
|
||||
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<String, String
|
||||
doc.insert(table_name, toml_edit::Item::Table(t));
|
||||
}
|
||||
let servers = doc.get_mut(table_name).and_then(|t| t.as_table_mut()).ok_or_else(|| format!("`{table_name}` exists but is not a table"))?;
|
||||
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());
|
||||
// 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");
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
|
||||
+5
-1
@@ -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 <id>`), 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
|
||||
|
||||
+2
-2
@@ -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.
|
||||
|
||||
@@ -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.<tool>]` 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).
|
||||
Reference in New Issue
Block a user