fix: control port needs a per-launch token; MCP tool annotations tell the whole truth (#205) (#901)

solidcraft --control PORT writes a random per-launch token to a user-only file (<data>/control/PORT.token; 0700/0600 on Unix, icacls on Windows; SOLIDCRAFT_CONTROL_TOKEN overrides) and refuses any connection whose first request lacks it. mcp --connect, the demo and packaging smoke clients send it. MCP execute/batch/open/save/export/screenshot are annotated destructive and open-world, new_design destructive. Docs: control-protocol.md, mcp.md, AGENTS.md, pending release note in releasing.md.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
bherbruckandClaude Opus 5.5 authored and GitHub committed 2026-10-13 18:51:19 -04:00
1 parent d9237d004e
commit fe3717f2e4
19 files changed
+465 -53

No files matched your search

+17 -2
View File
@@ -31,11 +31,22 @@ pub struct Remote {
addr: String,
conn: Option<(BufReader<TcpStream>, TcpStream)>,
next_id: u64,
/// The app's control token, sent with every request (docs/control-protocol.md).
token: Option<String>,
}
impl Remote {
/// Connect to the app at `addr`, with its control token found the usual way
/// (`SOLIDCRAFT_CONTROL_TOKEN`, else the app's token file for that port).
pub fn connect(addr: &str) -> std::io::Result<Self> {
let mut r = Self { addr: addr.to_string(), conn: None, next_id: 1 };
Self::connect_with_token(addr, None)
}
/// [`Remote::connect`] with the token given (`--token`); `None` finds it the usual way.
pub fn connect_with_token(addr: &str, token: Option<&str>) -> std::io::Result<Self> {
let port = addr.rsplit(':').next().and_then(|p| p.parse::<u16>().ok()).unwrap_or(0);
let token = solidcraft_engine::control_auth::client_token(port, token);
let mut r = Self { addr: addr.to_string(), conn: None, next_id: 1, token };
r.reconnect()?;
Ok(r)
}
@@ -95,7 +106,11 @@ impl Backend for Remote {
if let (Some(scope), Some(o)) = (solidcraft_engine::io::vfs::current_scope(), params.as_object_mut()) {
o.insert("allow_paths".into(), json!(scope.roots().iter().map(|r| r.to_string_lossy()).collect::<Vec<_>>()));
}
let line = json!({"id": id, "method": method, "params": params}).to_string();
let mut msg = json!({"id": id, "method": method, "params": params});
if let Some(t) = &self.token {
msg["token"] = json!(t);
}
let line = msg.to_string();
let reply = match self.roundtrip(&line) {
Ok(r) => r,
// One retry on a fresh connection (the app may have restarted).
+30 -2
View File
@@ -544,13 +544,17 @@ fn a_bridged_request_carries_the_agents_folders() {
let mut line = String::new();
BufReader::new(stream.try_clone().unwrap()).read_line(&mut line).unwrap();
let req: Value = serde_json::from_str(&line).unwrap();
writeln!(&stream, "{}", json!({"id": req["id"], "ok": true, "result": req["params"]})).unwrap();
let mut result = req["params"].clone();
result["sent_token"] = req["token"].clone();
writeln!(&stream, "{}", json!({"id": req["id"], "ok": true, "result": result})).unwrap();
});
let mut b = crate::Remote::connect(&addr).unwrap();
// The app's control token goes with the request too (#205).
let mut b = crate::Remote::connect_with_token(&addr, Some("0123456789abcdef0123456789abcdef")).unwrap();
let dir = std::fs::canonicalize(std::env::temp_dir()).unwrap();
let got = scoped(Some(PathScope::new([dir.clone()])), || crate::Backend::call(&mut b, "engine.execute", json!({"command": "file.new"}))).unwrap();
assert_eq!(got["allow_paths"], json!([dir.to_string_lossy()]), "{got}");
assert_eq!(got["command"], "file.new");
assert_eq!(got["sent_token"], "0123456789abcdef0123456789abcdef");
echo.join().unwrap();
}
@@ -862,3 +866,27 @@ fn instructions_name_only_real_tools() {
assert!(tools.iter().any(|t| t == name), "the instructions name `{name}`, which is not a tool");
}
}
/// Tool annotations never understate what a tool may do (#205): `screenshot` writes a file when
/// given a `path`, and `execute`, `batch`, `save`, `export` and `open` can overwrite or delete
/// files, so none of them is read-only and all are destructive and reach the file system.
#[test]
fn tool_annotations_tell_the_whole_truth() {
let tools = crate::tool_definitions();
let hints = |name: &str| {
let t = tools.iter().find(|t| t["name"] == name).unwrap_or_else(|| panic!("no tool {name}"));
let a = &t["annotations"];
(a["readOnlyHint"].as_bool().unwrap(), a["destructiveHint"].as_bool().unwrap(), a["openWorldHint"].as_bool().unwrap())
};
for name in ["execute", "batch", "save", "export", "open", "screenshot"] {
assert_eq!(hints(name), (false, true, true), "{name}");
}
for name in ["list_commands", "inspect_design", "measure", "body_topology", "crash_reports"] {
assert_eq!(hints(name), (true, false, false), "{name}");
}
assert_eq!(hints("new_design"), (false, true, false));
for name in ["set_parameter", "undo", "redo"] {
assert_eq!(hints(name), (false, false, false), "{name}");
}
assert!(tools.iter().all(|t| t["annotations"]["readOnlyHint"].is_boolean()), "every tool is annotated");
}
+37 -30
View File
@@ -23,13 +23,33 @@ fn obj(props: Value, required: &[&str]) -> Value {
o
}
fn tool(name: &str, title: &str, desc: &str, schema: Value, read_only: bool, destructive: bool) -> Value {
/// What a tool may do, for the client's approval prompts (MCP tool annotations). Clients that
/// auto-approve on these hints must not be told less than the truth (#205).
#[derive(Clone, Copy)]
enum Effect {
/// Only looks.
ReadOnly,
/// Changes the design in memory, undoably.
Edits,
/// Replaces the design in memory (unsaved changes are lost).
Replaces,
/// May read, write, overwrite or delete files on disk (any command, a file path).
Files,
}
fn tool(name: &str, title: &str, desc: &str, schema: Value, effect: Effect) -> Value {
let (read_only, destructive, open_world) = match effect {
Effect::ReadOnly => (true, false, false),
Effect::Edits => (false, false, false),
Effect::Replaces => (false, true, false),
Effect::Files => (false, true, true),
};
json!({
"name": name,
"title": title,
"description": desc,
"inputSchema": schema,
"annotations": {"title": title, "readOnlyHint": read_only, "destructiveHint": destructive, "idempotentHint": read_only, "openWorldHint": false},
"annotations": {"title": title, "readOnlyHint": read_only, "destructiveHint": destructive, "idempotentHint": read_only, "openWorldHint": open_world},
})
}
@@ -49,8 +69,7 @@ pub fn tool_definitions() -> Vec<Value> {
substring `filter` over id, label, tab and panel (not the parameter docs, so a command id \
finds that command rather than every command whose docs mention it).",
obj(json!({"filter": string("Substring to match, e.g. \"sketch\", \"fillet\", \"pattern\"")}), &[]),
true,
false,
Effect::ReadOnly,
),
tool(
"execute",
@@ -73,8 +92,7 @@ pub fn tool_definitions() -> Vec<Value> {
}),
&["command"],
),
false,
false,
Effect::Files,
),
tool(
"batch",
@@ -95,8 +113,7 @@ pub fn tool_definitions() -> Vec<Value> {
}),
&["commands"],
),
false,
false,
Effect::Files,
),
tool(
"inspect_design",
@@ -112,8 +129,7 @@ pub fn tool_definitions() -> Vec<Value> {
}),
&[],
),
true,
false,
Effect::ReadOnly,
),
tool(
"measure",
@@ -121,8 +137,7 @@ pub fn tool_definitions() -> Vec<Value> {
"Volume (mm³), surface area (mm²), centre of mass, bounding box and face/edge/vertex counts of one body or all \
bodies, with totals.",
obj(json!({"body": string("Body name (default: all bodies)")}), &[]),
true,
false,
Effect::ReadOnly,
),
tool(
"body_topology",
@@ -130,8 +145,7 @@ pub fn tool_definitions() -> Vec<Value> {
"The edges (index, midpoint, length, endpoints) and faces (index, area, centroid, plane normal) of a body. Use \
an edge midpoint as the [x,y,z] point that picks the edge for fillet and chamfer.",
obj(json!({"body": string("Body name, e.g. \"Body1\"")}), &["body"]),
true,
false,
Effect::ReadOnly,
),
tool(
"set_parameter",
@@ -148,8 +162,7 @@ pub fn tool_definitions() -> Vec<Value> {
}),
&["name", "value"],
),
false,
false,
Effect::Edits,
),
tool(
"screenshot",
@@ -171,8 +184,7 @@ pub fn tool_definitions() -> Vec<Value> {
}),
&[],
),
true,
false,
Effect::Files,
),
tool(
"export",
@@ -187,8 +199,7 @@ pub fn tool_definitions() -> Vec<Value> {
}),
&["path"],
),
false,
false,
Effect::Files,
),
tool(
"crash_reports",
@@ -205,18 +216,16 @@ pub fn tool_definitions() -> Vec<Value> {
}),
&[],
),
true,
false,
Effect::ReadOnly,
),
tool("undo", "Undo", "Undo the last change to the design.", obj(json!({}), &[]), false, false),
tool("redo", "Redo", "Redo the last undone change.", obj(json!({}), &[]), false, false),
tool("undo", "Undo", "Undo the last change to the design.", obj(json!({}), &[]), Effect::Edits),
tool("redo", "Redo", "Redo the last undone change.", obj(json!({}), &[]), Effect::Edits),
tool(
"new_design",
"New design",
"Start a new, empty design (unsaved changes to the current one are discarded).",
obj(json!({"name": string("Design name (default Untitled)")}), &[]),
false,
true,
Effect::Replaces,
),
tool(
"open",
@@ -225,8 +234,7 @@ pub fn tool_definitions() -> Vec<Value> {
Import base feature) or a 3MF/STL mesh (.3mf/.stl: mesh bodies). Replaces the current design; insert into it \
instead with the file.insert_step (STEP) or file.insert_mesh (3MF/STL) commands.",
obj(json!({"path": string("Path to a .solidcraft, .step/.stp, .3mf or .stl file")}), &["path"]),
false,
true,
Effect::Files,
),
tool(
"save",
@@ -236,8 +244,7 @@ pub fn tool_definitions() -> Vec<Value> {
json!({"path": string("Path to save to (default: the current file)"), "format": {"type": "string", "enum": ["container", "json"], "description": "Design encoding (default: JSON for .json, container otherwise)"}}),
&[],
),
false,
false,
Effect::Files,
),
]
}