From f4ba85ffa29c20d830a52acfcaa999b4232108a1 Mon Sep 17 00:00:00 2001 From: Bowen Wang Date: Mon, 23 Feb 2026 06:51:43 -0800 Subject: [PATCH] fix: fall back to build-from-source when extension download fails (#312) * fix: fall back to build-from-source when extension download fails Extension manifests hardcode GitHub release URLs for WASM artifacts, but these artifacts are not yet published to any release. This causes all WASM extension installs to fail with HTTP 404. Add a fallback_source field to RegistryEntry so that when the primary WasmDownload source fails (e.g., 404), the installer automatically falls back to WasmBuildable (build from source). The manifest conversion now populates this fallback whenever a download URL is set. Fixes nearai/ironclaw#298 Co-Authored-By: Claude Opus 4.6 * Address Copilot/Gemini review feedback - Skip fallback for AlreadyInstalled errors (Gemini) - Include both primary and fallback errors in combined message (Copilot) - Fix comment to match broader behavior (any error, not just download) (Copilot) Co-Authored-By: Claude Opus 4.6 * Address serrrfirat review feedback - Forward AlreadyInstalled from fallback directly instead of wrapping in ExtensionError::Other (defensive, prevents misleading error message) Co-Authored-By: Claude Opus 4.6 * Add unit tests for fallback install logic Extract fallback_decision() and combine_install_errors() from install_from_entry() to enable direct unit testing without requiring a full ExtensionManager setup. Tests cover: - Primary success returns directly (no fallback attempted) - AlreadyInstalled short-circuits (no fallback attempted) - Download failure with fallback available triggers fallback - Error without fallback source returns primary error - Both-fail produces combined error with both messages - AlreadyInstalled from fallback is forwarded directly Co-Authored-By: Claude Opus 4.6 --------- Co-authored-by: Claude Opus 4.6 Co-authored-by: firat.sertgoz --- src/extensions/discovery.rs | 2 + src/extensions/manager.rs | 173 ++++++++++++++++++++++++++++++++++-- src/extensions/mod.rs | 3 + src/extensions/registry.rs | 20 +++++ src/registry/manifest.rs | 155 ++++++++++++++++++++++++++++---- 5 files changed, 330 insertions(+), 23 deletions(-) diff --git a/src/extensions/discovery.rs b/src/extensions/discovery.rs index 51123dc3..04cb366b 100644 --- a/src/extensions/discovery.rs +++ b/src/extensions/discovery.rs @@ -104,6 +104,7 @@ impl OnlineDiscovery { source: ExtensionSource::McpUrl { url: url.to_string(), }, + fallback_source: None, auth_hint: AuthHint::Dcr, }) } else { @@ -178,6 +179,7 @@ impl OnlineDiscovery { .unwrap_or_else(|| format!("MCP server from GitHub: {}", item.full_name)), keywords: item.topics, source: ExtensionSource::Discovered { url }, + fallback_source: None, auth_hint: AuthHint::Dcr, }) }) diff --git a/src/extensions/manager.rs b/src/extensions/manager.rs index 884b9706..52814340 100644 --- a/src/extensions/manager.rs +++ b/src/extensions/manager.rs @@ -545,10 +545,41 @@ impl ExtensionManager { async fn install_from_entry( &self, entry: &RegistryEntry, + ) -> Result { + let primary_result = self.try_install_from_source(entry, &entry.source).await; + match fallback_decision(&primary_result, &entry.fallback_source) { + FallbackDecision::Return => primary_result, + FallbackDecision::TryFallback => { + let primary_err = primary_result.unwrap_err(); + let fallback = entry.fallback_source.as_ref().unwrap(); + tracing::info!( + extension = %entry.name, + primary_error = %primary_err, + "Primary install failed, trying fallback source" + ); + self.try_install_from_source(entry, fallback) + .await + .map_err(|fallback_err| { + tracing::error!( + extension = %entry.name, + fallback_error = %fallback_err, + "Fallback install also failed" + ); + combine_install_errors(&primary_err, fallback_err) + }) + } + } + } + + /// Attempt to install an extension using a specific source. + async fn try_install_from_source( + &self, + entry: &RegistryEntry, + source: &ExtensionSource, ) -> Result { match entry.kind { ExtensionKind::McpServer => { - let url = match &entry.source { + let url = match source { ExtensionSource::McpUrl { url } => url.clone(), ExtensionSource::Discovered { url } => url.clone(), _ => { @@ -559,7 +590,7 @@ impl ExtensionManager { }; self.install_mcp_from_url(&entry.name, &url).await } - ExtensionKind::WasmTool => match &entry.source { + ExtensionKind::WasmTool => match source { ExtensionSource::WasmDownload { wasm_url, capabilities_url, @@ -586,10 +617,10 @@ impl ExtensionManager { .await } _ => Err(ExtensionError::InstallFailed( - "WASM tool entry has no download URL".to_string(), + "WASM tool entry has no download URL or build info".to_string(), )), }, - ExtensionKind::WasmChannel => match &entry.source { + ExtensionKind::WasmChannel => match source { ExtensionSource::WasmDownload { wasm_url, capabilities_url, @@ -616,7 +647,7 @@ impl ExtensionManager { .await } _ => Err(ExtensionError::InstallFailed( - "WASM channel entry has no download URL".to_string(), + "WASM channel entry has no download URL or build info".to_string(), )), }, } @@ -2228,10 +2259,56 @@ fn infer_kind_from_url(url: &str) -> ExtensionKind { } } +/// Decision from `fallback_decision`: should we try the fallback source or +/// return the primary result as-is? +enum FallbackDecision { + /// Return the primary result directly (success or non-retriable error). + Return, + /// Primary failed with a retriable error and a fallback source is available. + TryFallback, +} + +/// Decide whether to attempt a fallback install based on the primary result +/// and the availability of a fallback source. +fn fallback_decision( + primary_result: &Result, + fallback_source: &Option>, +) -> FallbackDecision { + match (primary_result, fallback_source) { + // Success — no fallback needed + (Ok(_), _) => FallbackDecision::Return, + // AlreadyInstalled — don't try building from source + (Err(ExtensionError::AlreadyInstalled(_)), _) => FallbackDecision::Return, + // Failed with a fallback available — try it + (Err(_), Some(_)) => FallbackDecision::TryFallback, + // Failed with no fallback — return the error + (Err(_), None) => FallbackDecision::Return, + } +} + +/// Combine primary and fallback errors into a single error. +/// +/// Preserves `AlreadyInstalled` from the fallback directly; otherwise wraps +/// both error messages into `ExtensionError::Other`. +fn combine_install_errors( + primary_err: &ExtensionError, + fallback_err: ExtensionError, +) -> ExtensionError { + if matches!(fallback_err, ExtensionError::AlreadyInstalled(_)) { + return fallback_err; + } + ExtensionError::Other(format!( + "Primary install failed: {}; fallback install also failed: {}", + primary_err, fallback_err + )) +} + #[cfg(test)] mod tests { - use crate::extensions::ExtensionKind; - use crate::extensions::manager::infer_kind_from_url; + use crate::extensions::manager::{ + FallbackDecision, combine_install_errors, fallback_decision, infer_kind_from_url, + }; + use crate::extensions::{ExtensionError, ExtensionKind, ExtensionSource, InstallResult}; #[test] fn test_infer_kind_from_url() { @@ -2252,4 +2329,86 @@ mod tests { ExtensionKind::McpServer ); } + + // ---- fallback install logic tests ---- + + fn make_ok_result() -> Result { + Ok(InstallResult { + name: "test".to_string(), + kind: ExtensionKind::WasmTool, + message: "Installed".to_string(), + }) + } + + fn make_fallback_source() -> Option> { + Some(Box::new(ExtensionSource::WasmBuildable { + repo_url: "tools-src/test".to_string(), + build_dir: Some("tools-src/test".to_string()), + crate_name: Some("test-tool".to_string()), + })) + } + + #[test] + fn test_fallback_decision_success_returns_directly() { + let result = make_ok_result(); + let fallback = make_fallback_source(); + assert!(matches!( + fallback_decision(&result, &fallback), + FallbackDecision::Return + )); + } + + #[test] + fn test_fallback_decision_already_installed_skips_fallback() { + let result: Result = + Err(ExtensionError::AlreadyInstalled("test".to_string())); + let fallback = make_fallback_source(); + assert!(matches!( + fallback_decision(&result, &fallback), + FallbackDecision::Return + )); + } + + #[test] + fn test_fallback_decision_download_failed_triggers_fallback() { + let result: Result = + Err(ExtensionError::DownloadFailed("404 Not Found".to_string())); + let fallback = make_fallback_source(); + assert!(matches!( + fallback_decision(&result, &fallback), + FallbackDecision::TryFallback + )); + } + + #[test] + fn test_fallback_decision_error_without_fallback_returns() { + let result: Result = + Err(ExtensionError::DownloadFailed("404 Not Found".to_string())); + let fallback = None; + assert!(matches!( + fallback_decision(&result, &fallback), + FallbackDecision::Return + )); + } + + #[test] + fn test_combine_errors_includes_both_messages() { + let primary = ExtensionError::DownloadFailed("404 Not Found".to_string()); + let fallback = ExtensionError::InstallFailed("cargo not found".to_string()); + let combined = combine_install_errors(&primary, fallback); + let msg = combined.to_string(); + assert!(msg.contains("404 Not Found"), "missing primary: {msg}"); + assert!(msg.contains("cargo not found"), "missing fallback: {msg}"); + } + + #[test] + fn test_combine_errors_forwards_already_installed_from_fallback() { + let primary = ExtensionError::DownloadFailed("404".to_string()); + let fallback = ExtensionError::AlreadyInstalled("test".to_string()); + let combined = combine_install_errors(&primary, fallback); + assert!( + matches!(combined, ExtensionError::AlreadyInstalled(ref name) if name == "test"), + "Expected AlreadyInstalled, got: {combined:?}" + ); + } } diff --git a/src/extensions/mod.rs b/src/extensions/mod.rs index d9b36291..cb45ed02 100644 --- a/src/extensions/mod.rs +++ b/src/extensions/mod.rs @@ -64,6 +64,9 @@ pub struct RegistryEntry { pub keywords: Vec, /// Where to get this extension. pub source: ExtensionSource, + /// Fallback source when the primary source fails (e.g., download 404 → build from source). + #[serde(default, skip_serializing_if = "Option::is_none")] + pub fallback_source: Option>, /// How authentication works. pub auth_hint: AuthHint, } diff --git a/src/extensions/registry.rs b/src/extensions/registry.rs index 5f925427..95b96bd1 100644 --- a/src/extensions/registry.rs +++ b/src/extensions/registry.rs @@ -208,6 +208,7 @@ fn builtin_entries() -> Vec { source: ExtensionSource::McpUrl { url: "https://mcp.notion.com/mcp".to_string(), }, + fallback_source: None, auth_hint: AuthHint::Dcr, }, RegistryEntry { @@ -227,6 +228,7 @@ fn builtin_entries() -> Vec { source: ExtensionSource::McpUrl { url: "https://mcp.linear.app".to_string(), }, + fallback_source: None, auth_hint: AuthHint::Dcr, }, RegistryEntry { @@ -245,6 +247,7 @@ fn builtin_entries() -> Vec { source: ExtensionSource::McpUrl { url: "https://mcp.google.com/calendar".to_string(), }, + fallback_source: None, auth_hint: AuthHint::Dcr, }, RegistryEntry { @@ -263,6 +266,7 @@ fn builtin_entries() -> Vec { source: ExtensionSource::McpUrl { url: "https://mcp.google.com/drive".to_string(), }, + fallback_source: None, auth_hint: AuthHint::Dcr, }, RegistryEntry { @@ -282,6 +286,7 @@ fn builtin_entries() -> Vec { source: ExtensionSource::McpUrl { url: "https://mcp.github.com".to_string(), }, + fallback_source: None, auth_hint: AuthHint::Dcr, }, RegistryEntry { @@ -301,6 +306,7 @@ fn builtin_entries() -> Vec { source: ExtensionSource::McpUrl { url: "https://mcp.slack.com".to_string(), }, + fallback_source: None, auth_hint: AuthHint::Dcr, }, RegistryEntry { @@ -320,6 +326,7 @@ fn builtin_entries() -> Vec { source: ExtensionSource::McpUrl { url: "https://mcp.sentry.dev/sse".to_string(), }, + fallback_source: None, auth_hint: AuthHint::Dcr, }, RegistryEntry { @@ -339,6 +346,7 @@ fn builtin_entries() -> Vec { source: ExtensionSource::McpUrl { url: "https://mcp.stripe.com".to_string(), }, + fallback_source: None, auth_hint: AuthHint::Dcr, }, RegistryEntry { @@ -358,6 +366,7 @@ fn builtin_entries() -> Vec { source: ExtensionSource::McpUrl { url: "https://mcp.cloudflare.com/sse".to_string(), }, + fallback_source: None, auth_hint: AuthHint::Dcr, }, RegistryEntry { @@ -375,6 +384,7 @@ fn builtin_entries() -> Vec { source: ExtensionSource::McpUrl { url: "https://mcp.asana.com".to_string(), }, + fallback_source: None, auth_hint: AuthHint::Dcr, }, RegistryEntry { @@ -393,6 +403,7 @@ fn builtin_entries() -> Vec { source: ExtensionSource::McpUrl { url: "https://mcp.intercom.com".to_string(), }, + fallback_source: None, auth_hint: AuthHint::Dcr, }, // WASM channels (telegram, slack, discord, whatsapp) come from the embedded @@ -417,6 +428,7 @@ mod tests { source: ExtensionSource::McpUrl { url: "https://example.com".to_string(), }, + fallback_source: None, auth_hint: AuthHint::Dcr, }; @@ -439,6 +451,7 @@ mod tests { source: ExtensionSource::McpUrl { url: "https://example.com".to_string(), }, + fallback_source: None, auth_hint: AuthHint::Dcr, }; @@ -461,6 +474,7 @@ mod tests { source: ExtensionSource::McpUrl { url: "https://example.com".to_string(), }, + fallback_source: None, auth_hint: AuthHint::Dcr, }; @@ -483,6 +497,7 @@ mod tests { source: ExtensionSource::McpUrl { url: "https://example.com".to_string(), }, + fallback_source: None, auth_hint: AuthHint::Dcr, }; @@ -546,6 +561,7 @@ mod tests { source: ExtensionSource::McpUrl { url: "https://custom.example.com".to_string(), }, + fallback_source: None, auth_hint: AuthHint::Dcr, }; @@ -571,6 +587,7 @@ mod tests { source: ExtensionSource::McpUrl { url: "https://example.com".to_string(), }, + fallback_source: None, auth_hint: AuthHint::None, }; @@ -595,6 +612,7 @@ mod tests { build_dir: Some("channels-src/telegram".to_string()), crate_name: Some("telegram-channel".to_string()), }, + fallback_source: None, auth_hint: AuthHint::CapabilitiesAuth, }, // This shares a name with the builtin slack-mcp but has a different kind, so both should appear @@ -609,6 +627,7 @@ mod tests { build_dir: Some("tools-src/slack".to_string()), crate_name: Some("slack-tool".to_string()), }, + fallback_source: None, auth_hint: AuthHint::CapabilitiesAuth, }, ]; @@ -644,6 +663,7 @@ mod tests { source: ExtensionSource::McpUrl { url: "https://other.slack.com".to_string(), }, + fallback_source: None, auth_hint: AuthHint::Dcr, }]; diff --git a/src/registry/manifest.rs b/src/registry/manifest.rs index d5b3fedf..de84aa31 100644 --- a/src/registry/manifest.rs +++ b/src/registry/manifest.rs @@ -154,26 +154,29 @@ impl ExtensionManifest { /// Convert this manifest into a [`RegistryEntry`] for use with the in-chat /// extension discovery system. pub fn to_registry_entry(&self) -> RegistryEntry { - // Prefer pre-built artifact download when a URL is available - let source = if let Some(artifact) = self.artifacts.get("wasm32-wasip2") { + let buildable = ExtensionSource::WasmBuildable { + repo_url: self.source.dir.clone(), + build_dir: Some(self.source.dir.clone()), + crate_name: Some(self.source.crate_name.clone()), + }; + + // Prefer pre-built artifact download when a URL is available, + // with build-from-source as fallback in case the download fails (e.g., 404). + let (source, fallback_source) = if let Some(artifact) = self.artifacts.get("wasm32-wasip2") + { if let Some(ref url) = artifact.url { - ExtensionSource::WasmDownload { - wasm_url: url.clone(), - capabilities_url: artifact.capabilities_url.clone(), - } + ( + ExtensionSource::WasmDownload { + wasm_url: url.clone(), + capabilities_url: artifact.capabilities_url.clone(), + }, + Some(Box::new(buildable)), + ) } else { - ExtensionSource::WasmBuildable { - repo_url: self.source.dir.clone(), - build_dir: Some(self.source.dir.clone()), - crate_name: Some(self.source.crate_name.clone()), - } + (buildable, None) } } else { - ExtensionSource::WasmBuildable { - repo_url: self.source.dir.clone(), - build_dir: Some(self.source.dir.clone()), - crate_name: Some(self.source.crate_name.clone()), - } + (buildable, None) }; let auth_hint = match self.auth_summary.as_ref().and_then(|a| a.method.as_deref()) { @@ -190,6 +193,7 @@ impl ExtensionManifest { description: self.description.clone(), keywords: self.keywords.clone(), source, + fallback_source, auth_hint, } } @@ -292,4 +296,123 @@ mod tests { assert_eq!(ManifestKind::Tool.to_string(), "tool"); assert_eq!(ManifestKind::Channel.to_string(), "channel"); } + + /// When a manifest has a download URL in artifacts, to_registry_entry() + /// should set WasmDownload as primary source and WasmBuildable as fallback. + #[test] + fn test_manifest_with_download_url_has_buildable_fallback() { + let json = r#"{ + "name": "gmail", + "display_name": "Gmail", + "kind": "tool", + "version": "0.1.0", + "description": "Gmail tool", + "keywords": ["email"], + "source": { + "dir": "tools-src/gmail", + "capabilities": "gmail-tool.capabilities.json", + "crate_name": "gmail-tool" + }, + "artifacts": { + "wasm32-wasip2": { + "url": "https://github.com/nearai/ironclaw/releases/latest/download/gmail-wasm32-wasip2.tar.gz", + "sha256": null + } + }, + "tags": ["default"] + }"#; + + let manifest: ExtensionManifest = serde_json::from_str(json).expect("parse manifest"); + let entry = manifest.to_registry_entry(); + + // Primary source should be WasmDownload + assert!( + matches!(&entry.source, ExtensionSource::WasmDownload { .. }), + "Primary source should be WasmDownload, got {:?}", + entry.source + ); + + // Fallback should be WasmBuildable with the source dir info + let fallback = entry + .fallback_source + .as_ref() + .expect("Should have fallback_source when download URL is set"); + match fallback.as_ref() { + ExtensionSource::WasmBuildable { + build_dir, + crate_name, + .. + } => { + assert_eq!(build_dir.as_deref(), Some("tools-src/gmail")); + assert_eq!(crate_name.as_deref(), Some("gmail-tool")); + } + other => panic!("Fallback should be WasmBuildable, got {:?}", other), + } + } + + /// When a manifest has null URL in artifacts, the primary source should be + /// WasmBuildable with no fallback. + #[test] + fn test_manifest_with_null_url_no_fallback() { + let json = r#"{ + "name": "slack", + "display_name": "Slack", + "kind": "tool", + "version": "0.1.0", + "description": "Slack tool", + "keywords": [], + "source": { + "dir": "tools-src/slack", + "capabilities": "slack-tool.capabilities.json", + "crate_name": "slack-tool" + }, + "artifacts": { + "wasm32-wasip2": { "url": null, "sha256": null } + }, + "tags": [] + }"#; + + let manifest: ExtensionManifest = serde_json::from_str(json).expect("parse manifest"); + let entry = manifest.to_registry_entry(); + + assert!( + matches!(&entry.source, ExtensionSource::WasmBuildable { .. }), + "Should use WasmBuildable when URL is null" + ); + assert!( + entry.fallback_source.is_none(), + "Should have no fallback when already using WasmBuildable" + ); + } + + /// When a manifest has no artifacts section, should use WasmBuildable with no fallback. + #[test] + fn test_manifest_no_artifacts_no_fallback() { + let json = r#"{ + "name": "custom", + "display_name": "Custom", + "kind": "tool", + "version": "0.1.0", + "description": "Custom tool", + "keywords": [], + "source": { + "dir": "tools-src/custom", + "capabilities": "custom.capabilities.json", + "crate_name": "custom-tool" + }, + "tags": [] + }"#; + + let manifest: ExtensionManifest = serde_json::from_str(json).expect("parse manifest"); + let entry = manifest.to_registry_entry(); + + assert!( + matches!(&entry.source, ExtensionSource::WasmBuildable { .. }), + "Should use WasmBuildable when no artifacts" + ); + assert!( + entry.fallback_source.is_none(), + "Should have no fallback when already using WasmBuildable" + ); + } }