diff --git a/src-tauri/src/commands/mcp.rs b/src-tauri/src/commands/mcp.rs index 3e705f3e81..f750937a71 100644 --- a/src-tauri/src/commands/mcp.rs +++ b/src-tauri/src/commands/mcp.rs @@ -80,6 +80,30 @@ pub struct LocalMcpServer { pub apps: Vec, } +/// One agent whose config the scan could not read. +/// +/// Carries the reader's own message so the user can go fix the file — the +/// alternative (swallowing it) would leave that agent's servers missing from +/// the list with nothing to explain why. +#[derive(Debug, Clone, Serialize)] +pub struct LocalMcpSourceWarning { + pub app: McpAppType, + pub message: String, +} + +/// A local scan: every server codeg could read, plus a warning per source it +/// could not. +/// +/// Deliberately not a bare `Vec` with a fail-fast error. These +/// config files are owned by other agents and by the user, so any one of them +/// can be half-written at any moment; letting a single unreadable file abort +/// the scan hid EVERY agent's servers behind one error banner (issue #632). +#[derive(Debug, Clone, Serialize)] +pub struct LocalMcpScan { + pub servers: Vec, + pub warnings: Vec, +} + #[derive(Debug, Clone, Serialize)] pub struct McpMarketplaceProvider { pub id: String, @@ -153,7 +177,7 @@ pub struct McpMarketplaceServerDetail { } #[cfg_attr(feature = "tauri-runtime", tauri::command)] -pub async fn mcp_scan_local() -> Result, AppCommandError> { +pub async fn mcp_scan_local() -> LocalMcpScan { scan_local_servers() } @@ -376,7 +400,7 @@ pub async fn mcp_install_from_marketplace( upsert_server_for_app(app, &server_id, &canonical_spec)?; } - find_local_server(&server_id)?.ok_or_else(|| { + find_local_server(&server_id).ok_or_else(|| { mcp_configuration_invalid(format!( "installed server '{server_id}', but failed to load it from local configuration" )) @@ -432,6 +456,7 @@ pub async fn mcp_upsert_local_server( // Nothing below is reversible, and the walk REMOVES the server from every // non-target agent, so a target whose config cannot take it has to be // caught before the first write rather than halfway through. + require_complete_scan(&scan_local_servers())?; with_upsert_preflight(&target_set, || { for app in all_apps { if target_set.contains(&app) { @@ -443,7 +468,7 @@ pub async fn mcp_upsert_local_server( Ok(()) })?; - find_local_server(&server_id)?.ok_or_else(|| { + find_local_server(&server_id).ok_or_else(|| { mcp_configuration_invalid(format!( "saved local MCP server '{server_id}', but failed to reload it" )) @@ -456,7 +481,12 @@ pub async fn mcp_set_server_apps( apps: Vec, ) -> Result, AppCommandError> { let target_apps = normalize_apps(apps); - let current = find_local_server(&server_id)? + let scan = scan_local_servers(); + require_complete_scan(&scan)?; + let current = scan + .servers + .into_iter() + .find(|item| item.id == server_id) .ok_or_else(|| mcp_not_found(format!("local MCP server not found: {server_id}")))?; // Preflight-exclude apps whose config can't host this transport (e.g. Codex + @@ -492,7 +522,7 @@ pub async fn mcp_set_server_apps( Ok(()) })?; - find_local_server(&server_id) + Ok(find_local_server(&server_id)) } #[cfg_attr(feature = "tauri-runtime", tauri::command)] @@ -744,12 +774,56 @@ fn cline_config_path() -> PathBuf { .join("cline_mcp_settings.json") } +/// Read a file that is absent for most users, distinguishing "nobody has +/// configured this agent" from "this agent's config exists and codeg could not +/// read it". +/// +/// The absence test is the read itself, not `Path::exists()`: `exists()` +/// answers `false` for ANY failed stat — a permission wall on a parent +/// directory, a symlink loop — so it would report a file codeg simply could not +/// open as an empty config. `scan_local_servers` would then leave that agent +/// out with no warning, and `require_complete_scan` would wave through a +/// reassignment that strips the server from the agents it COULD read and then +/// fails on the write to this one. +fn read_config_to_string(path: &Path) -> Result, AppCommandError> { + match fs::read_to_string(path) { + Ok(raw) => Ok(Some(raw)), + Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(None), + Err(err) => { + // `AppCommandError::io` flattens the kind to "Permission denied" / + // "I/O operation failed" and keeps only the OS text, neither of + // which names the file. The scan warning built from this is the + // user's only pointer to what to go fix, so put the path in. + let detail = format!("{}: {err}", path.display()); + Err(AppCommandError::io(err).with_detail(detail)) + } + } +} + fn read_json_file(path: &Path) -> Result { - if !path.exists() { + let Some(raw) = read_config_to_string(path)? else { + return Ok(json!({})); + }; + // A 0-byte (or whitespace-only) config means "nothing configured", exactly + // like an absent one: several of these agents touch the file into existence + // before they ever write a server into it. serde has no such notion and + // reports `EOF while parsing a value at line 1 column 0`, which used to be + // raised as a hard configuration error (issue #632). The Hermes writer + // already applies this rule to its own config; it belongs here for every + // JSON-backed source. + // + // The writers share this reader, so an agent that truncates its config + // before rewriting it can be caught mid-write and have codeg start from + // `{}` — and unlike the ordinary stale read this subsystem already lives + // with (nothing locks these files), that one loses settings even when the + // agent's rewrite changed nothing. It is accepted deliberately: the window + // is one non-atomic rewrite wide, while REFUSING empty files would leave a + // user whose config is PERSISTENTLY 0 bytes — the reported case — unable to + // assign a server to that agent at all, and codeg cannot tell the two + // apart from a single read. + if raw.trim().is_empty() { return Ok(json!({})); } - - let raw = fs::read_to_string(path).map_err(AppCommandError::io)?; serde_json::from_str::(&raw) .map_err(|e| mcp_configuration_invalid(format!("invalid JSON at {}: {e}", path.display()))) } @@ -769,11 +843,9 @@ fn write_json_file(path: &Path, value: &Value) -> Result<(), AppCommandError> { fn read_codex_root_toml() -> Result { let path = codex_config_toml_path(); - if !path.exists() { + let Some(raw) = read_config_to_string(&path)? else { return Ok(toml::Value::Table(toml::map::Map::new())); - } - - let raw = fs::read_to_string(&path).map_err(AppCommandError::io)?; + }; let parsed = raw.parse::().map_err(|e| { mcp_configuration_invalid(format!("invalid TOML at {}: {e}", path.display())) })?; @@ -2865,142 +2937,167 @@ fn remove_antigravity_server_at(path: &Path, id: &str) -> Result Result, AppCommandError> { - let mut merged: BTreeMap)> = BTreeMap::new(); - - for (id, spec) in read_claude_servers()? { - let entry = merged - .entry(id) - .or_insert_with(|| (spec.clone(), BTreeSet::new())); - entry.1.insert(McpAppType::ClaudeCode); - } +type LocalMcpReadResult = Result, AppCommandError>; +type LocalMcpReadFn = fn() -> LocalMcpReadResult; - for (id, spec) in read_codex_servers()? { - let entry = merged - .entry(id) - .or_insert_with(|| (spec.clone(), BTreeSet::new())); - entry.1.insert(McpAppType::Codex); - } +#[derive(Clone, Copy)] +struct LocalMcpReader { + source: &'static str, + app: McpAppType, + read: LocalMcpReadFn, +} - for (id, spec) in read_opencode_servers()? { - let entry = merged - .entry(id) - .or_insert_with(|| (spec.clone(), BTreeSet::new())); - entry.1.insert(McpAppType::OpenCode); +impl LocalMcpReader { + const fn new(source: &'static str, app: McpAppType, read: LocalMcpReadFn) -> Self { + Self { source, app, read } } +} - for (id, spec) in read_gemini_servers()? { - let entry = merged - .entry(id) - .or_insert_with(|| (spec.clone(), BTreeSet::new())); - entry.1.insert(McpAppType::Gemini); - } +fn local_mcp_readers() -> [LocalMcpReader; 14] { + [ + LocalMcpReader::new("Claude Code", McpAppType::ClaudeCode, read_claude_servers), + LocalMcpReader::new("Codex", McpAppType::Codex, read_codex_servers), + LocalMcpReader::new("OpenCode", McpAppType::OpenCode, read_opencode_servers), + LocalMcpReader::new("Gemini", McpAppType::Gemini, read_gemini_servers), + LocalMcpReader::new("OpenClaw", McpAppType::OpenClaw, read_openclaw_servers), + LocalMcpReader::new("Cline", McpAppType::Cline, read_cline_servers), + LocalMcpReader::new("Hermes", McpAppType::Hermes, read_hermes_servers), + LocalMcpReader::new("CodeBuddy", McpAppType::CodeBuddy, read_codebuddy_servers), + LocalMcpReader::new("Kimi Code", McpAppType::KimiCode, read_kimi_code_servers), + LocalMcpReader::new("Grok", McpAppType::Grok, read_grok_servers), + LocalMcpReader::new("Cursor", McpAppType::Cursor, read_cursor_servers), + LocalMcpReader::new("DeepSeek", McpAppType::DeepSeek, read_deepseek_servers), + LocalMcpReader::new( + "Antigravity", + McpAppType::Antigravity, + read_antigravity_servers, + ), + LocalMcpReader::new("Qoder", McpAppType::Qoder, read_qoder_servers), + ] +} +/// Merge every agent's MCP config into one list, degrading a source that cannot +/// be read into a warning instead of failing the whole scan. +/// +/// These files belong to the other agents and to the user, so any of them can be +/// empty, half-written or hand-edited into something codeg cannot parse. A +/// fail-fast `?` here meant one such file hid every OTHER agent's servers too +/// (issue #632: an empty `~/.gemini/config/mcp_config.json` emptied the entire +/// local MCP list). The broken source drops out; the rest of the scan stands. +/// +/// Reading degrades. WRITING does not: callers that mutate must first clear the +/// scan through [`require_complete_scan`]. +fn scan_local_servers_from_readers(readers: &[LocalMcpReader]) -> LocalMcpScan { + let mut merged: BTreeMap)> = BTreeMap::new(); + let mut warnings: Vec = Vec::new(); // OpenClaw is the one agent that shares a key with Kimi (`auth`), so keep // what its own config declares: below, that is what tells Kimi's pass an // OpenClaw setting from an echo codeg once wrote into some other agent's // file — and it has to be the VALUE, since the agent that wins the merge may // carry neither. See `KIMI_SHARED_KEYS`. let mut openclaw_declares: BTreeMap> = BTreeMap::new(); - for (id, spec) in read_openclaw_servers()? { - let declared: Map = KIMI_SHARED_KEYS - .iter() - .filter_map(|key| { - spec.get(*key) - .map(|value| ((*key).to_string(), value.clone())) - }) - .collect(); - if !declared.is_empty() { - openclaw_declares.insert(id.clone(), declared); - } - let entry = merged - .entry(id) - .or_insert_with(|| (spec.clone(), BTreeSet::new())); - entry.1.insert(McpAppType::OpenClaw); - } - - for (id, spec) in read_cline_servers()? { - let entry = merged - .entry(id) - .or_insert_with(|| (spec.clone(), BTreeSet::new())); - entry.1.insert(McpAppType::Cline); - } - - for (id, spec) in read_hermes_servers()? { - let entry = merged - .entry(id) - .or_insert_with(|| (spec.clone(), BTreeSet::new())); - entry.1.insert(McpAppType::Hermes); - } - - for (id, spec) in read_codebuddy_servers()? { - let entry = merged - .entry(id) - .or_insert_with(|| (spec.clone(), BTreeSet::new())); - entry.1.insert(McpAppType::CodeBuddy); - } - - for (id, spec) in read_kimi_code_servers()? { - let owner_values = openclaw_declares.remove(&id).unwrap_or_default(); - let entry = merged - .entry(id) - .or_insert_with(|| (spec.clone(), BTreeSet::new())); - // Kimi models fields no other agent does, and this merge is - // first-writer-wins — so when an earlier agent already claimed the id, - // fold Kimi's own fields back in or they never reach the editor (and the - // next save writes them off disk). See `merge_kimi_extension_fields`. - merge_kimi_extension_fields(&mut entry.0, &spec, &owner_values); - entry.1.insert(McpAppType::KimiCode); - } - for (id, spec) in read_grok_servers()? { - let entry = merged - .entry(id) - .or_insert_with(|| (spec.clone(), BTreeSet::new())); - entry.1.insert(McpAppType::Grok); - } + for reader in readers { + let servers = match (reader.read)() { + Ok(servers) => servers, + Err(err) => { + tracing::warn!( + source = reader.source, + app = ?reader.app, + error = ?err, + "[MCP] failed to scan local MCP source; skipping" + ); + // A parse failure's own message names the offending file; an I/O + // failure's does not (`AppCommandError::io` flattens those to + // "Permission denied" and parks the specifics in `detail`), so + // carry both or the warning cannot be acted on. + warnings.push(LocalMcpSourceWarning { + app: reader.app, + message: match err.detail { + Some(detail) => format!("{}: {detail}", err.message), + None => err.message, + }, + }); + continue; + } + }; - for (id, spec) in read_cursor_servers()? { - let entry = merged - .entry(id) - .or_insert_with(|| (spec.clone(), BTreeSet::new())); - entry.1.insert(McpAppType::Cursor); - } + for (id, spec) in servers { + if reader.app == McpAppType::OpenClaw { + let declared: Map = KIMI_SHARED_KEYS + .iter() + .filter_map(|key| { + spec.get(*key) + .map(|value| ((*key).to_string(), value.clone())) + }) + .collect(); + if !declared.is_empty() { + openclaw_declares.insert(id.clone(), declared); + } + } - for (id, spec) in read_deepseek_servers()? { - let entry = merged - .entry(id) - .or_insert_with(|| (spec.clone(), BTreeSet::new())); - entry.1.insert(McpAppType::DeepSeek); + let entry = merged + .entry(id.clone()) + .or_insert_with(|| (spec.clone(), BTreeSet::new())); + if reader.app == McpAppType::KimiCode { + // Kimi models fields no other agent does, and this merge is + // first-writer-wins — so when an earlier agent already claimed the id, + // fold Kimi's own fields back in or they never reach the editor (and the + // next save writes them off disk). See `merge_kimi_extension_fields`. + let owner_values = openclaw_declares.remove(&id).unwrap_or_default(); + merge_kimi_extension_fields(&mut entry.0, &spec, &owner_values); + } + entry.1.insert(reader.app); + } } - for (id, spec) in read_antigravity_servers()? { - let entry = merged - .entry(id) - .or_insert_with(|| (spec.clone(), BTreeSet::new())); - entry.1.insert(McpAppType::Antigravity); + LocalMcpScan { + servers: merged + .into_iter() + .map(|(id, (spec, apps))| LocalMcpServer { + id, + spec, + apps: apps.into_iter().collect(), + }) + .collect(), + warnings, } +} - for (id, spec) in read_qoder_servers()? { - let entry = merged - .entry(id) - .or_insert_with(|| (spec.clone(), BTreeSet::new())); - entry.1.insert(McpAppType::Qoder); - } +fn scan_local_servers() -> LocalMcpScan { + scan_local_servers_from_readers(&local_mcp_readers()) +} - Ok(merged +fn find_local_server(server_id: &str) -> Option { + scan_local_servers() + .servers .into_iter() - .map(|(id, (spec, apps))| LocalMcpServer { - id, - spec, - apps: apps.into_iter().collect(), - }) - .collect()) + .find(|item| item.id == server_id) } -fn find_local_server(server_id: &str) -> Result, AppCommandError> { - let servers = scan_local_servers()?; - Ok(servers.into_iter().find(|item| item.id == server_id)) +/// Refuse a reassignment that would act on an INCOMPLETE picture of who holds +/// the server. +/// +/// `mcp_upsert_local_server` and `mcp_set_server_apps` both mean "these agents, +/// and no others" — every agent absent from the list they are handed gets the +/// server REMOVED. That list is seeded from a scan, so an agent whose config +/// could not be read is absent for that reason alone, and acting on it would +/// strip the server from an agent the user never unchecked (and then, in +/// `mcp_set_server_apps`, report it as held by nobody). +/// +/// So: READING degrades past an unreadable source (issue #632 — one bad file +/// must not hide every agent's servers), WRITING does not. Blocking the save +/// costs the user one "fix this file" round trip; guessing costs them an +/// assignment they never asked to lose. +fn require_complete_scan(scan: &LocalMcpScan) -> Result<(), AppCommandError> { + let Some(warning) = scan.warnings.first() else { + return Ok(()); + }; + Err(mcp_configuration_invalid(format!( + "cannot change MCP server assignments while {:?}'s configuration cannot be read \ + ({}). Fix or remove that file, then try again.", + warning.app, warning.message + ))) } fn upsert_server_for_app(app: McpAppType, id: &str, spec: &Value) -> Result<(), AppCommandError> { @@ -3412,10 +3509,9 @@ fn grok_config_toml_path() -> PathBuf { } fn read_grok_root_toml_at(path: &Path) -> Result { - if !path.exists() { + let Some(raw) = read_config_to_string(path)? else { return Ok(toml::Value::Table(toml::map::Map::new())); - } - let raw = fs::read_to_string(path).map_err(AppCommandError::io)?; + }; let parsed = raw.parse::().map_err(|e| { mcp_configuration_invalid(format!("invalid TOML at {}: {e}", path.display())) })?; @@ -3965,21 +4061,31 @@ fn canonical_to_hermes_entry(spec: &Value) -> Result Result, AppCommandError> { - let path = crate::commands::acp::hermes_config_yaml_path(); - let Ok(raw) = fs::read_to_string(&path) else { + read_hermes_servers_at(&crate::commands::acp::hermes_config_yaml_path()) +} + +fn read_hermes_servers_at(path: &Path) -> Result, AppCommandError> { + let Some(raw) = read_config_to_string(path)? else { return Ok(BTreeMap::new()); }; - let root: serde_yaml::Value = match serde_yaml::from_str(&raw) { - Ok(value) => value, - Err(err) => { - tracing::warn!("[MCP] skip Hermes mcp_servers: invalid config.yaml: {err}"); - return Ok(BTreeMap::new()); - } - }; + if raw.trim().is_empty() { + return Ok(BTreeMap::new()); + } + let root: serde_yaml::Value = serde_yaml::from_str(&raw).map_err(|err| { + mcp_configuration_invalid(format!("invalid YAML at {}: {err}", path.display())) + })?; let mut out = BTreeMap::new(); let Some(servers) = root @@ -5867,6 +5973,162 @@ fn resolve_smithery_install_spec_with_selection( mod tests { use super::*; + fn test_claude_servers() -> Result, AppCommandError> { + Ok(BTreeMap::from([ + ( + "claude-only".to_string(), + json!({"type": "stdio", "command": "claude-only"}), + ), + ( + "shared".to_string(), + json!({"type": "stdio", "command": "claude-wins"}), + ), + ])) + } + + fn test_gemini_servers() -> Result, AppCommandError> { + Ok(BTreeMap::from([ + ( + "gemini-only".to_string(), + json!({"type": "stdio", "command": "gemini-only"}), + ), + ( + "shared".to_string(), + json!({"type": "stdio", "command": "gemini-loses"}), + ), + ])) + } + + fn test_broken_antigravity_servers() -> Result, AppCommandError> { + Err(mcp_configuration_invalid("broken Antigravity fixture")) + } + + fn test_openclaw_servers() -> Result, AppCommandError> { + Ok(BTreeMap::from([( + "shared-remote".to_string(), + json!({ + "type": "http", + "url": "https://example.test/mcp", + "auth": "oauth" + }), + )])) + } + + fn test_kimi_servers() -> Result, AppCommandError> { + Ok(BTreeMap::from([( + "shared-remote".to_string(), + json!({ + "type": "http", + "url": "https://example.test/mcp", + "bearerTokenEnvVar": "MCP_TOKEN" + }), + )])) + } + + #[test] + fn best_effort_scan_keeps_valid_sources_around_a_failed_source() { + let readers = [ + LocalMcpReader::new("Claude Code", McpAppType::ClaudeCode, test_claude_servers), + LocalMcpReader::new( + "Antigravity", + McpAppType::Antigravity, + test_broken_antigravity_servers, + ), + LocalMcpReader::new("Gemini", McpAppType::Gemini, test_gemini_servers), + ]; + + let scan = scan_local_servers_from_readers(&readers); + + assert_eq!( + scan.servers + .iter() + .map(|server| server.id.as_str()) + .collect::>(), + ["claude-only", "gemini-only", "shared"] + ); + let shared = scan + .servers + .iter() + .find(|server| server.id == "shared") + .expect("shared server"); + assert_eq!(shared.spec["command"], "claude-wins"); + assert_eq!(shared.apps, [McpAppType::ClaudeCode, McpAppType::Gemini]); + + // Dropping the source silently would leave the user staring at an + // Antigravity column that is simply blank, with nothing to act on. + assert_eq!( + scan.warnings + .iter() + .map(|warning| warning.app) + .collect::>(), + [McpAppType::Antigravity] + ); + } + + #[test] + fn a_failed_source_is_refused_by_the_write_guard() { + let readers = [ + LocalMcpReader::new("Claude Code", McpAppType::ClaudeCode, test_claude_servers), + LocalMcpReader::new( + "Antigravity", + McpAppType::Antigravity, + test_broken_antigravity_servers, + ), + LocalMcpReader::new("Gemini", McpAppType::Gemini, test_gemini_servers), + ]; + + // The list survives the broken source; a reassignment computed from that + // same list must not, or Antigravity is dropped from the "keep it here" + // set purely because codeg could not read it. + let err = require_complete_scan(&scan_local_servers_from_readers(&readers)) + .expect_err("writers must stay fail-closed on a degraded scan"); + assert!( + err.message.contains("Antigravity"), + "the refusal has to name the file to go fix: {}", + err.message + ); + } + + #[test] + fn empty_antigravity_file_reads_as_no_servers() { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("mcp_config.json"); + std::fs::write(&path, "").expect("seed empty config"); + + // Issue #632 verbatim: Antigravity touches this file into existence + // before it ever writes a server, and a 0-byte config means "nothing + // configured", not "corrupt". Treating it as a reader error deadlocks + // the user — every writer starts by reading the file it is about to + // change, so nothing can ever put content into it again. + assert_eq!( + read_antigravity_servers_at(&path).expect("empty config is not an error"), + BTreeMap::new() + ); + } + + #[test] + fn successful_scan_preserves_openclaw_and_kimi_merge_rules() { + let readers = [ + LocalMcpReader::new("OpenClaw", McpAppType::OpenClaw, test_openclaw_servers), + LocalMcpReader::new("Kimi Code", McpAppType::KimiCode, test_kimi_servers), + ]; + + let scan = scan_local_servers_from_readers(&readers); + + assert!( + scan.warnings.is_empty(), + "readable sources must not warn: {:?}", + scan.warnings + ); + let servers = scan.servers; + assert_eq!(servers.len(), 1); + let shared = &servers[0]; + assert_eq!(shared.id, "shared-remote"); + assert_eq!(shared.spec["auth"], "oauth"); + assert_eq!(shared.spec["bearerTokenEnvVar"], "MCP_TOKEN"); + assert_eq!(shared.apps, [McpAppType::OpenClaw, McpAppType::KimiCode]); + } + #[test] fn normalize_mcp_type_canonical_pass_through() { assert_eq!(normalize_mcp_type("stdio"), Some("stdio")); @@ -6293,6 +6555,229 @@ mod tests { assert!(root["mcpServers"].get("b").is_some()); } + #[test] + fn empty_json_config_reads_as_no_servers() { + // Issue #632: a 0-byte `~/.gemini/config/mcp_config.json` made serde + // report `EOF while parsing a value at line 1 column 0`, which the + // reader raised as a hard configuration error. An empty file is + // "nothing configured", exactly like an absent one. + let dir = tempfile::tempdir().expect("tempdir"); + for (name, body) in [("empty.json", ""), ("blank.json", " \r\n\t ")] { + let path = dir.path().join(name); + std::fs::write(&path, body).expect("seed file"); + + assert_eq!(read_json_file(&path).expect(name), json!({})); + assert!(read_antigravity_servers_at(&path).expect(name).is_empty()); + assert!(read_cursor_servers_at(&path).expect(name).is_empty()); + assert!(read_kimi_code_servers_at(&path).expect(name).is_empty()); + assert!(read_qoder_servers_at(&path).expect(name).is_empty()); + assert!(read_deepseek_servers_at(&path).expect(name).is_empty()); + + // Writing into one still works — there is nothing in an empty file + // to preserve, so the save must not refuse either. + upsert_antigravity_server_at(&path, "ctx7", &json!({ "command": "npx" })) + .expect("upsert into an empty config"); + assert!(read_antigravity_servers_at(&path) + .expect("read back") + .contains_key("ctx7")); + } + + // A file with actual content that is not JSON is still an error: codeg + // must not overwrite something the user wrote and it failed to read. + let broken = dir.path().join("broken.json"); + std::fs::write(&broken, "{ not json").expect("seed broken"); + assert!(read_json_file(&broken).is_err()); + } + + #[test] + fn a_config_that_cannot_be_stat_ed_is_an_error_not_an_empty_one() { + // Absence is decided by the read, not by `Path::exists()`: `exists()` + // answers `false` for any failed stat, so a config codeg cannot open + // would be reported as "this agent has none" — silently, with no + // warning for `require_complete_scan` to refuse a reassignment on. + let dir = tempfile::tempdir().expect("tempdir"); + + let absent = dir.path().join("absent.json"); + assert!(read_config_to_string(&absent) + .expect("absent is Ok") + .is_none()); + assert_eq!(read_json_file(&absent).expect("absent"), json!({})); + + // A symlink loop: present enough to matter, unreadable, and `exists()` + // reports it as absent. (Only the construction is unix-specific; the + // rule it pins is not.) + #[cfg(unix)] + { + let loop_a = dir.path().join("loop-a.json"); + let loop_b = dir.path().join("loop-b.json"); + std::os::unix::fs::symlink(&loop_b, &loop_a).expect("link a->b"); + std::os::unix::fs::symlink(&loop_a, &loop_b).expect("link b->a"); + assert!(!loop_a.exists(), "exists() cannot tell this from absent"); + + let err = read_config_to_string(&loop_a).expect_err("unreadable"); + // The kind alone ("Permission denied", "I/O operation failed") + // names no file, so the path has to survive into the detail or the + // scan warning tells the user nothing they can act on. + assert!( + err.detail + .as_deref() + .is_some_and(|d| d.contains(&loop_a.display().to_string())), + "{err:?}" + ); + + assert!(read_json_file(&loop_a).is_err(), "must not read as {{}}"); + assert!(read_hermes_servers_at(&loop_a).is_err()); + assert!(read_grok_root_toml_at(&loop_a).is_err()); + } + } + + fn test_one_readable_server() -> Result, AppCommandError> { + Ok(BTreeMap::from([( + "ctx7".to_string(), + json!({ "command": "npx" }), + )])) + } + + fn test_parse_failure() -> Result, AppCommandError> { + Err(mcp_configuration_invalid( + "invalid JSON at /x/mcp_config.json: boom", + )) + } + + fn test_io_failure() -> Result, AppCommandError> { + Err(AppCommandError::io(std::io::Error::new( + std::io::ErrorKind::PermissionDenied, + "permission denied (os error 13)", + ))) + } + + fn test_late_server() -> Result, AppCommandError> { + Ok(BTreeMap::from([( + "later".to_string(), + json!({ "command": "later-bin" }), + )])) + } + + #[test] + fn one_unreadable_source_warns_instead_of_emptying_the_scan() { + // Issue #632: `scan_local_servers` read every agent behind a fail-fast + // `?`, so a single broken config hid EVERY agent's servers behind one + // "load failed" banner. A bad source must degrade to a warning. + let scan = scan_local_servers_from_readers(&[ + LocalMcpReader::new("Claude Code", McpAppType::ClaudeCode, test_one_readable_server), + LocalMcpReader::new("Antigravity", McpAppType::Antigravity, test_parse_failure), + LocalMcpReader::new("Cursor", McpAppType::Cursor, test_io_failure), + LocalMcpReader::new("Codex", McpAppType::Codex, test_late_server), + ]); + + // Readable sources on BOTH sides of the failures still land. + assert_eq!( + scan.servers + .iter() + .map(|server| server.id.as_str()) + .collect::>(), + ["ctx7", "later"] + ); + assert_eq!( + scan.warnings + .iter() + .map(|warning| warning.app) + .collect::>(), + [McpAppType::Antigravity, McpAppType::Cursor] + ); + + // The reader's own message names the offending file, so the user can go + // fix it rather than just seeing that agent's servers go missing. + assert!( + scan.warnings[0].message.contains("/x/mcp_config.json"), + "{:?}", + scan.warnings[0].message + ); + + // An I/O failure carries its specifics in `detail` rather than in + // `message`, so the warning has to fold both in or it reads as a bare + // "Permission denied" with nothing to act on. + assert!( + scan.warnings[1].message.contains("os error 13"), + "{:?}", + scan.warnings[1].message + ); + } + + #[test] + fn hermes_reports_an_unreadable_config_instead_of_swallowing_it() { + // Hermes used to answer `Ok(empty)` for a config.yaml it could not + // parse, back when an `Err` would have aborted the whole scan. Now that + // a failure only warns, swallowing it would leave Hermes missing from a + // scan that still reports itself complete — and `require_complete_scan` + // would wave through a reassignment that strips the server from the + // agents that DID load and then fails on the Hermes write. + let dir = tempfile::tempdir().expect("tempdir"); + + // Absent and empty stay "no Hermes servers": most machines have none. + let missing = dir.path().join("nope.yaml"); + assert!(read_hermes_servers_at(&missing).expect("absent").is_empty()); + let empty = dir.path().join("empty.yaml"); + std::fs::write(&empty, " \n").expect("seed empty"); + assert!(read_hermes_servers_at(&empty).expect("empty").is_empty()); + + // Unparseable is an error that names the file. + let broken = dir.path().join("config.yaml"); + std::fs::write(&broken, "mcp_servers: [unclosed\n").expect("seed broken"); + let err = read_hermes_servers_at(&broken).expect_err("invalid YAML"); + assert!( + err.message.contains(&broken.display().to_string()), + "{:?}", + err.message + ); + + // A valid document still reads. + std::fs::write( + &broken, + "model:\n provider: openai\nmcp_servers:\n ctx7:\n command: npx\n", + ) + .expect("seed valid"); + let servers = read_hermes_servers_at(&broken).expect("valid"); + assert!(servers.contains_key("ctx7"), "{servers:?}"); + } + + #[test] + fn the_write_guard_accepts_a_clean_scan_and_refuses_a_degraded_one() { + // Reading past an unreadable source is the whole point of #632. WRITING + // past one is not: the app list a save is handed comes from the scan, + // and every agent missing from it gets the server removed — so an agent + // that is missing only because its file could not be read would be + // stripped without the user ever unchecking it. + // + // This covers the guard itself. That the two write commands actually + // CALL it before their first write is asserted by + // `a_failed_source_is_refused_by_the_write_guard` plus the call sites. + let clean = LocalMcpScan { + servers: vec![LocalMcpServer { + id: "ctx7".to_string(), + spec: json!({ "command": "npx" }), + apps: vec![McpAppType::ClaudeCode], + }], + warnings: Vec::new(), + }; + require_complete_scan(&clean).expect("a complete scan may be written from"); + + let degraded = LocalMcpScan { + warnings: vec![LocalMcpSourceWarning { + app: McpAppType::Antigravity, + message: "invalid JSON at /x/mcp_config.json: boom".to_string(), + }], + ..clean + }; + let err = require_complete_scan(°raded).expect_err("a degraded scan may not"); + // The refusal has to name the file, or the user cannot clear the block. + assert!( + err.message.contains("Antigravity") && err.message.contains("/x/mcp_config.json"), + "{:?}", + err.message + ); + } + #[test] fn qoder_settings_json_round_trips_and_preserves_foreign_keys() { // `~/.qoder/settings.json` is QODER'S file, not codeg's: the CLI owns diff --git a/src-tauri/src/web/handlers/mcp.rs b/src-tauri/src/web/handlers/mcp.rs index 7b8c2cba02..bc3438aef3 100644 --- a/src-tauri/src/web/handlers/mcp.rs +++ b/src-tauri/src/web/handlers/mcp.rs @@ -5,7 +5,7 @@ use serde_json::Value; use crate::app_error::AppCommandError; use crate::commands::mcp as mcp_commands; use crate::commands::mcp::{ - LocalMcpServer, McpAppType, McpMarketplaceItem, McpMarketplaceProvider, + LocalMcpScan, LocalMcpServer, McpAppType, McpMarketplaceItem, McpMarketplaceProvider, McpMarketplaceServerDetail, }; @@ -66,9 +66,8 @@ pub struct RemoveServerParams { // Handlers // --------------------------------------------------------------------------- -pub async fn mcp_scan_local() -> Result>, AppCommandError> { - let result = mcp_commands::mcp_scan_local().await?; - Ok(Json(result)) +pub async fn mcp_scan_local() -> Json { + Json(mcp_commands::mcp_scan_local().await) } pub async fn mcp_list_marketplaces() -> Result>, AppCommandError> { diff --git a/src/components/settings/mcp-settings.tsx b/src/components/settings/mcp-settings.tsx index f8c61d2b70..06053ed615 100644 --- a/src/components/settings/mcp-settings.tsx +++ b/src/components/settings/mcp-settings.tsx @@ -54,6 +54,7 @@ import { normalizeMcpType } from "@/lib/mcp-types" import { cn } from "@/lib/utils" import type { LocalMcpServer, + LocalMcpSourceWarning, McpAppType, McpMarketplaceItem, McpMarketplaceInstallOption, @@ -104,6 +105,21 @@ const APP_OPTIONS: { value: McpAppType; label: string }[] = [ { value: "antigravity", label: "Google Antigravity" }, ] +// The backend SCANS one more agent than it lets you assign to: OpenClaw is read +// back so existing entries survive, but is not an assignable target (see the +// note in APP_OPTIONS). A scan warning can still name it, so it needs a label. +const SCAN_ONLY_APP_LABELS: Partial> = { + open_claw: "OpenClaw", +} + +function appLabel(app: McpAppType): string { + return ( + APP_OPTIONS.find((option) => option.value === app)?.label ?? + SCAN_ONLY_APP_LABELS[app] ?? + app + ) +} + function isObject(value: unknown): value is Record { return Boolean(value) && typeof value === "object" && !Array.isArray(value) } @@ -338,6 +354,9 @@ export function McpSettings() { const [selection, setSelection] = useState(null) const [installedServers, setInstalledServers] = useState([]) + const [sourceWarnings, setSourceWarnings] = useState( + [] + ) const [localFilter, setLocalFilter] = useState("") const [providers, setProviders] = useState([]) @@ -409,6 +428,13 @@ export function McpSettings() { [localSpecText] ) + // A scan that could not read every agent is fine to LIST from but not to + // reassign from: the app checkboxes it seeds drive removals, so an agent + // missing only because its file was unreadable would be stripped. The + // backend refuses such a save; the UI blocks composing one, which also stops + // the draft outliving the repair (fix the file, hit Refresh, then edit). + const scanDegraded = sourceWarnings.length > 0 + const filteredLocalServers = useMemo(() => { const q = localFilter.trim().toLowerCase() if (!q) return installedServers @@ -420,9 +446,10 @@ export function McpSettings() { }, [installedServers, localFilter, mcpT]) const refreshLocalServers = useCallback(async () => { - const servers = await mcpScanLocal() - setInstalledServers(servers) - return servers + const scan = await mcpScanLocal() + setInstalledServers(scan.servers) + setSourceWarnings(scan.warnings) + return scan.servers }, []) const loadInitial = useCallback(async () => { @@ -430,18 +457,19 @@ export function McpSettings() { setLoadingError(null) try { - const [servers, marketProviders] = await Promise.all([ + const [scan, marketProviders] = await Promise.all([ mcpScanLocal(), mcpListMarketplaces(), ]) - setInstalledServers(servers) + setInstalledServers(scan.servers) + setSourceWarnings(scan.warnings) setProviders(marketProviders) setSelectedProvider( (current) => current || marketProviders[0]?.id || "official_registry" ) - if (servers[0]) { - setSelection({ kind: "local", id: servers[0].id }) + if (scan.servers[0]) { + setSelection({ kind: "local", id: scan.servers[0].id }) } } catch (err) { const message = toLocalizedErrorMessage(err, mcpT) @@ -1082,6 +1110,21 @@ export function McpSettings() { ) : null} + {/* One agent's config being unreadable hides only that agent's + servers — the rest of the list below is still real, so this + is a warning beside it rather than an error instead of it. */} + {sourceWarnings.map((warning) => ( +
+ {t("local.sourceUnreadable", { + app: appLabel(warning.app), + message: warning.message, + })} +
+ ))} +
{filteredLocalServers.length === 0 ? (
@@ -1411,6 +1454,15 @@ export function McpSettings() {
) : null} + {/* Creating writes through the same command, which refuses while + any agent's config is unreadable — an id that already exists + in the unread one would be assigned away from it. */} + {scanDegraded ? ( +
+ {t("local.saveBlockedByUnreadableSource")} +
+ ) : null} +
) : null} + {/* The checkboxes above were seeded from a scan that could not + read every agent, so an agent that holds this server may be + showing as unchecked — and saving means "remove it from every + unchecked agent". The backend refuses such a save too; this + keeps the user from composing one whose stale draft would + still be accepted once they repair the file out of band. */} + {scanDegraded ? ( +
+ {t("local.saveBlockedByUnreadableSource")} +
+ ) : null} +