diff --git a/crates/op-cli/src/mcp_http_cli.rs b/crates/op-cli/src/mcp_http_cli.rs index 03245c7be..258ce5cb8 100644 --- a/crates/op-cli/src/mcp_http_cli.rs +++ b/crates/op-cli/src/mcp_http_cli.rs @@ -210,10 +210,21 @@ fn ping_result(port: u16) -> Option { if !(200..300).contains(&status) { return None; } - let value = serde_json::from_str::(&body).ok()?; + ping_reply_identity(&body) +} + +/// Extract the OpenPencil identity object from a raw `ping` reply, or +/// `None` for a non-OpenPencil responder. Servers ≥ 0.8.3 nest the +/// identity under `result._meta` (a spec-compliant ping result is empty +/// apart from `_meta` — strict clients reject top-level extras, issue +/// #199); older servers reported it at the `result` top level, so fall +/// back there to keep discovering a still-running pre-0.8.3 editor. +fn ping_reply_identity(body: &str) -> Option { + let value = serde_json::from_str::(body).ok()?; let result = value.get("result")?; - if result.get("server").and_then(Value::as_str) == Some(MCP_SERVER_NAME) { - Some(result.clone()) + let identity = result.get("_meta").unwrap_or(result); + if identity.get("server").and_then(Value::as_str) == Some(MCP_SERVER_NAME) { + Some(identity.clone()) } else { None } @@ -326,4 +337,29 @@ mod tests { )); assert!(!is_web_canvas_health("not json")); } + + #[test] + fn ping_identity_reads_meta_and_falls_back_to_legacy_top_level() { + // ≥ 0.8.3 servers nest the identity under `result._meta` (spec-empty + // ping result, issue #199). + let meta = ping_reply_identity( + r#"{"jsonrpc":"2.0","id":0,"result":{"_meta":{"server":"openpencil-mcp","mode":"live","token":"t-1"}}}"#, + ) + .expect("meta identity accepted"); + assert_eq!(meta.get("token").and_then(Value::as_str), Some("t-1")); + // A still-running pre-0.8.3 editor reports identity at the result top + // level — discovery must keep working against it. + let legacy = ping_reply_identity( + r#"{"jsonrpc":"2.0","id":0,"result":{"server":"openpencil-mcp","mode":"headless","token":"t-2"}}"#, + ) + .expect("legacy identity accepted"); + assert_eq!(legacy.get("token").and_then(Value::as_str), Some("t-2")); + // A foreign server (spec-compliant empty result, or another + // product's marker) is never treated as OpenPencil. + assert!(ping_reply_identity(r#"{"jsonrpc":"2.0","id":0,"result":{}}"#).is_none()); + assert!(ping_reply_identity( + r#"{"jsonrpc":"2.0","id":0,"result":{"_meta":{"server":"someone-else"}}}"# + ) + .is_none()); + } } diff --git a/crates/op-host-services/src/mcp_live/ui_requests.rs b/crates/op-host-services/src/mcp_live/ui_requests.rs index 1710c7142..e438b9326 100644 --- a/crates/op-host-services/src/mcp_live/ui_requests.rs +++ b/crates/op-host-services/src/mcp_live/ui_requests.rs @@ -157,10 +157,14 @@ pub(super) fn make_live_token() -> String { /// Live `ping` reply: OpenPencil identity + `mode:"live"` + the instance /// token, so `op` can distinguish this live canvas server from the -/// headless file server and confirm it owns the discovery file. +/// headless file server and confirm it owns the discovery file. The +/// identity rides in `_meta` — the MCP spec's extension point — because +/// strict clients validate a ping result as exactly `{ _meta? }` and +/// reject top-level extras (Gemini CLI, issue #199); see +/// `mcp_serve::ping_response`. pub(super) fn live_ping_response(id_raw: &str, token: &str) -> String { format!( - r#"{{"jsonrpc":"2.0","id":{id_raw},"result":{{"server":"{}","mode":"live","token":"{}"}}}}"#, + r#"{{"jsonrpc":"2.0","id":{id_raw},"result":{{"_meta":{{"server":"{}","mode":"live","token":"{}"}}}}}}"#, crate::mcp_serve::MCP_SERVER_NAME, crate::mcp_serve::json_escape(token) ) diff --git a/crates/op-host-services/src/mcp_live_tests.rs b/crates/op-host-services/src/mcp_live_tests.rs index 2621488bf..eb35b2bae 100644 --- a/crates/op-host-services/src/mcp_live_tests.rs +++ b/crates/op-host-services/src/mcp_live_tests.rs @@ -13,6 +13,34 @@ fn live_ping_response_carries_identity_mode_and_token() { assert!(resp.contains(r#""id":3"#), "{resp}"); } +#[test] +fn live_ping_result_is_spec_empty_apart_from_meta() { + // The live server is the one third-party MCP clients attach to, so it + // is the path that regressed in issue #199: a ping result must contain + // NOTHING but `_meta` or strict validators (Gemini CLI's + // `EmptyResultSchema.strict()`) mark the server disconnected. The + // identity trio rides inside `_meta`. + let resp = live_ping_response("3", "abc-123"); + let value: serde_json::Value = serde_json::from_str(&resp).expect("valid JSON"); + let result = value + .get("result") + .expect("has result") + .as_object() + .unwrap(); + assert_eq!( + result.keys().collect::>(), + vec!["_meta"], + "live ping result must contain only _meta: {resp}" + ); + let meta = &result["_meta"]; + assert_eq!( + meta.get("server").and_then(|v| v.as_str()), + Some(crate::mcp_serve::MCP_SERVER_NAME) + ); + assert_eq!(meta.get("mode").and_then(|v| v.as_str()), Some("live")); + assert_eq!(meta.get("token").and_then(|v| v.as_str()), Some("abc-123")); +} + #[test] fn make_live_token_is_nonempty_and_structured() { let token = make_live_token(); diff --git a/crates/op-host-services/src/mcp_serve/wire.rs b/crates/op-host-services/src/mcp_serve/wire.rs index 92a54928a..7a81c3617 100644 --- a/crates/op-host-services/src/mcp_serve/wire.rs +++ b/crates/op-host-services/src/mcp_serve/wire.rs @@ -46,13 +46,21 @@ pub fn ping_response(id_raw: &str, token: Option<&str>) -> String { // confirm the pid in its manager file owns this port. The token is // passed in (sourced from the env at the call site) so this stays a // pure formatter — testable without mutating process-global env. + // + // The identity fields MUST live under `_meta`, never at the `result` + // top level: the MCP spec says a ping result is empty, and strict + // clients validate it as exactly `{ _meta? }` (Gemini CLI's + // `EmptyResultSchema.strict()` rejected the old top-level shape with + // "Unrecognized keys: server, mode, token" — issue #199). `_meta` is + // the spec's sanctioned extension point, so the CLI handshake keeps + // working without breaking third-party compliance checks. match token { Some(token) => format!( - r#"{{"jsonrpc":"2.0","id":{id_raw},"result":{{"server":"{MCP_SERVER_NAME}","mode":"headless","token":"{token}"}}}}"# + r#"{{"jsonrpc":"2.0","id":{id_raw},"result":{{"_meta":{{"server":"{MCP_SERVER_NAME}","mode":"headless","token":"{token}"}}}}}}"# ), None => { format!( - r#"{{"jsonrpc":"2.0","id":{id_raw},"result":{{"server":"{MCP_SERVER_NAME}"}}}}"# + r#"{{"jsonrpc":"2.0","id":{id_raw},"result":{{"_meta":{{"server":"{MCP_SERVER_NAME}"}}}}}}"# ) } } diff --git a/crates/op-host-services/src/mcp_serve/wire_tests.rs b/crates/op-host-services/src/mcp_serve/wire_tests.rs index 8b9622b7a..bb51e7282 100644 --- a/crates/op-host-services/src/mcp_serve/wire_tests.rs +++ b/crates/op-host-services/src/mcp_serve/wire_tests.rs @@ -24,6 +24,31 @@ fn ping_response_embeds_headless_token_when_present() { assert!(resp.contains(r#""server":"openpencil-mcp""#), "{resp}"); } +#[test] +fn ping_response_result_is_spec_empty_apart_from_meta() { + // The MCP spec says a ping result is empty; strict clients (Gemini + // CLI's `EmptyResultSchema.strict()`, issue #199) reject any top-level + // key other than `_meta`. The OpenPencil identity must therefore ride + // INSIDE `_meta` — a top-level `server`/`mode`/`token` is a regression. + for resp in [ping_response("1", None), ping_response("2", Some("tok-9"))] { + let value: serde_json::Value = serde_json::from_str(&resp).expect("valid JSON"); + let result = value + .get("result") + .expect("has result") + .as_object() + .unwrap(); + assert_eq!( + result.keys().collect::>(), + vec!["_meta"], + "ping result must contain only _meta: {resp}" + ); + assert_eq!( + result["_meta"].get("server").and_then(|v| v.as_str()), + Some(MCP_SERVER_NAME) + ); + } +} + #[test] fn sanitize_token_rejects_unsafe_or_empty() { // Safe tokens (the CLI emits pid-nanos hex) pass through.