diff --git a/docs/interactive-capabilities/technical/tauri-command-map.json b/docs/interactive-capabilities/technical/tauri-command-map.json index 8808e8a736..4f37e195ad 100644 --- a/docs/interactive-capabilities/technical/tauri-command-map.json +++ b/docs/interactive-capabilities/technical/tauri-command-map.json @@ -329,7 +329,7 @@ "visibility": "documented", "rustPath": "add_skill", "sourceFile": "src/apps/desktop/src/api/skill_api.rs", - "signature": "fn add_skill( _state: State<'_, AppState>, source_path: String, level: String, workspace_path: Option, source_key: Option, target_name: Option, ) -> Result", + "signature": "fn add_skill( _state: State<'_, AppState>, source_path: String, level: String, workspace_path: Option, source_key: Option, target_name: Option, expected_source_fingerprint: Option, ) -> Result", "remoteWorkspacePolicy": "LegacyUnaudited" }, { @@ -10200,7 +10200,7 @@ "visibility": "documented", "rustPath": "validate_skill_path", "sourceFile": "src/apps/desktop/src/api/skill_api.rs", - "signature": "fn validate_skill_path(path: String) -> Result", + "signature": "fn validate_skill_path( path: String, source_key: Option, workspace_path: Option, ) -> Result", "remoteWorkspacePolicy": "LegacyUnaudited" }, { diff --git a/scripts/core-boundaries/rules/source/public-api-rules.mjs b/scripts/core-boundaries/rules/source/public-api-rules.mjs index 74d7184a83..94468e18b2 100644 --- a/scripts/core-boundaries/rules/source/public-api-rules.mjs +++ b/scripts/core-boundaries/rules/source/public-api-rules.mjs @@ -311,6 +311,20 @@ export const opencodeAdapterPublicApiEntries = [ 'OpenCodeConfiguredSkillRoot', 'openbitfun-core external source composition root and OpenCode configured Skill fixtures', ), + ...['OpenCodeSkillRootDiagnostic', 'OpenCodeSkillRootReport'].map((symbol) => ({ + symbol, + owner: 'opencode-adapter configured Skill root source owner', + consumer: 'openbitfun-core external_sources::opencode_configured_skill_roots_with_provider', + verification: + 'opencode_static_source_contracts::opencode_skill_roots diagnostics fixtures and core-boundary public API checks', + p0: 'runtime-free configured local Skill root discovery with bounded diagnostics', + contractSlice: contractSlices.opencodeAdapterBoundary, + wireImpact: false, + rationale: + 'Product Assembly projects valid roots and redacted discovery failures into its neutral Skill scan report without exposing OpenCode configuration syntax', + exit: + 'remove only with the configured Skill root provider or an equivalent reviewed diagnostic projection', + })), opencodeAdapterEntry( 'OpenCodeSkillRootProvider', 'openbitfun-core external source composition root and OpenCode configured Skill fixtures', diff --git a/scripts/core-boundaries/self-test.mjs b/scripts/core-boundaries/self-test.mjs index 5a53a60241..1cd582f860 100644 --- a/scripts/core-boundaries/self-test.mjs +++ b/scripts/core-boundaries/self-test.mjs @@ -1638,7 +1638,7 @@ export function runManifestParserSelfTest({ ).map((entry) => entry.symbol); if ( opencodeAdapterPublicApiSymbols.join(',') !== - 'load_opencode_package_adapter,load_opencode_config_snapshot,OpenCodeConfigSnapshot,OpenCodeConfigSnapshotError,project_plugin_config,project_plugin_tool_ref,OpenCodePluginConfigProjectionError,OpenCodeCommandProvider,OpenCodeCommandProviderOptions,OpenCodeConfiguredSkillRoot,OpenCodeSkillRootProvider,OpenCodeSkillRootProviderOptions,OpenCodeToolProvider,OpenCodeToolProviderOptions,OpenCodeSubagentProvider,OpenCodeSubagentProviderOptions,OpenCodeMcpProvider,OpenCodeMcpProviderOptions,OpenCodeHookProvider,OpenCodeHookProviderOptions,OpenCodeWorkspaceReferenceProvider,OpenCodeWorkspaceReferenceProviderOptions,load_opencode_user_instructions,OpenCodeInstructionSourceOptions' + 'load_opencode_package_adapter,load_opencode_config_snapshot,OpenCodeConfigSnapshot,OpenCodeConfigSnapshotError,project_plugin_config,project_plugin_tool_ref,OpenCodePluginConfigProjectionError,OpenCodeCommandProvider,OpenCodeCommandProviderOptions,OpenCodeConfiguredSkillRoot,OpenCodeSkillRootDiagnostic,OpenCodeSkillRootReport,OpenCodeSkillRootProvider,OpenCodeSkillRootProviderOptions,OpenCodeToolProvider,OpenCodeToolProviderOptions,OpenCodeSubagentProvider,OpenCodeSubagentProviderOptions,OpenCodeMcpProvider,OpenCodeMcpProviderOptions,OpenCodeHookProvider,OpenCodeHookProviderOptions,OpenCodeWorkspaceReferenceProvider,OpenCodeWorkspaceReferenceProviderOptions,load_opencode_user_instructions,OpenCodeInstructionSourceOptions' ) { throw new Error( 'OpenCode adapter public API budget must stay limited to the reviewed package factory, managed Plugin Host Config projection, and capability-specific command, configured Skill root, tool, subagent, MCP, static Hook, workspace Reference, and user Instruction providers', diff --git a/src/apps/desktop/src/api/skill_api.rs b/src/apps/desktop/src/api/skill_api.rs index 0cffcabef0..5ae283f1b2 100644 --- a/src/apps/desktop/src/api/skill_api.rs +++ b/src/apps/desktop/src/api/skill_api.rs @@ -22,6 +22,9 @@ use openbitfun_core::agentic::tools::implementations::skills::mode_overrides::{ save_project_mode_skills_document_local, set_disabled_mode_skills_in_document, set_global_user_skill_disabled, set_mode_skill_disabled_in_document, set_user_mode_skill_state, }; +use openbitfun_core::agentic::tools::implementations::skills::registry::imports::{ + self as skill_imports, SkillImportPreview, +}; use openbitfun_core::agentic::tools::implementations::skills::{ resolver::resolve_skill_default_enabled_for_mode, ModeSkillInfo, SkillData, SkillInfo, SkillLocation, SkillRegistry, SkillScanReport, @@ -86,6 +89,12 @@ fn ensure_skill_can_be_deleted(skill: &SkillInfo) -> Result<(), String> { #[derive(Debug, Clone, serde::Serialize, serde::Deserialize)] pub struct SkillValidationResult { + #[serde( + default, + rename = "importPreview", + skip_serializing_if = "Option::is_none" + )] + pub import_preview: Option, pub valid: bool, pub name: Option, pub description: Option, @@ -574,7 +583,7 @@ pub async fn get_skill_configs( }; object.insert( "importOperationsVersion".into(), - serde_json::json!(if supported { 2 } else { 0 }), + serde_json::json!(if supported { 3 } else { 0 }), ); } Ok(response) @@ -885,14 +894,58 @@ pub async fn reset_mode_skill_selection( )) } +async fn resolve_external_skill_import_source( + source_path: &str, + source_key: &str, + workspace: Option<&Path>, +) -> Result { + if let Some(root) = workspace { + if is_remote_path(&root.to_string_lossy()).await { + return Err("External Skill import into remote workspaces is not supported".into()); + } + } + let source = SkillRegistry::global() + .find_skill_by_key_for_workspace(source_key, workspace) + .await + .ok_or("skill_import_stale: External Skill source changed; refresh before importing")?; + if tokio::fs::canonicalize(&source.path) + .await + .map_err(|error| error.to_string())? + != tokio::fs::canonicalize(source_path) + .await + .map_err(|error| error.to_string())? + { + return Err("External Skill path does not match the selected source".into()); + } + Ok(source) +} + #[tauri::command] -pub async fn validate_skill_path(path: String) -> Result { +pub async fn validate_skill_path( + path: String, + source_key: Option, + workspace_path: Option, +) -> Result { use std::path::Path; + if let Some(source_key) = source_key { + let workspace = workspace_root_from_input(workspace_path.as_deref()); + let source = + resolve_external_skill_import_source(&path, &source_key, workspace.as_deref()).await?; + let preview = skill_imports::preview_import(source).await?; + return Ok(SkillValidationResult { + valid: true, + name: Some(preview.name.clone()), + description: Some(preview.description.clone()), + error: None, + import_preview: Some(preview), + }); + } let skill_path = Path::new(&path); if !skill_path.exists() { return Ok(SkillValidationResult { + import_preview: None, valid: false, name: None, description: None, @@ -902,6 +955,7 @@ pub async fn validate_skill_path(path: String) -> Result Result Result { match SkillData::from_markdown(path.clone(), &content, SkillLocation::User, false) { Ok(data) => Ok(SkillValidationResult { + import_preview: None, valid: true, name: Some(data.name), description: Some(data.description), error: None, }), Err(e) => Ok(SkillValidationResult { + import_preview: None, valid: false, name: None, description: None, @@ -937,6 +994,7 @@ pub async fn validate_skill_path(path: String) -> Result Ok(SkillValidationResult { + import_preview: None, valid: false, name: None, description: None, @@ -953,30 +1011,16 @@ pub async fn add_skill( workspace_path: Option, source_key: Option, target_name: Option, + expected_source_fingerprint: Option, ) -> Result { if let Some(source_key) = source_key { if !matches!(level.as_str(), "user" | "project") { return Err("Invalid Skill target scope".into()); } let workspace = workspace_root_from_input(workspace_path.as_deref()); - if let Some(root) = &workspace { - if is_remote_path(&root.to_string_lossy()).await { - return Err("External Skill import into remote workspaces is not supported".into()); - } - } - let source = SkillRegistry::global() - .find_skill_by_key_for_workspace(&source_key, workspace.as_deref()) - .await - .ok_or("External Skill source changed; refresh before importing")?; - if tokio::fs::canonicalize(&source.path) - .await - .map_err(|error| error.to_string())? - != tokio::fs::canonicalize(&source_path) - .await - .map_err(|error| error.to_string())? - { - return Err("External Skill path does not match the selected source".into()); - } + let source = + resolve_external_skill_import_source(&source_path, &source_key, workspace.as_deref()) + .await?; let paths = get_path_manager_arc(); let target = if level == "project" { paths @@ -985,8 +1029,11 @@ pub async fn add_skill( } else { paths.user_skills_dir() }; - openbitfun_core::agentic::tools::implementations::skills::registry::imports::import_copy_as( - source, target, target_name, + skill_imports::import_copy_as_reviewed( + source, + target, + target_name, + expected_source_fingerprint, ) .await?; SkillRegistry::global() @@ -994,10 +1041,10 @@ pub async fn add_skill( .await; return Ok("External Skill imported successfully".into()); } - if target_name.is_some() { - return Err("Renaming an imported Skill requires its source identity".into()); + if target_name.is_some() || expected_source_fingerprint.is_some() { + return Err("Reviewed or renamed Skill imports require their source identity".into()); } - let validation = validate_skill_path(source_path.clone()).await?; + let validation = validate_skill_path(source_path.clone(), None, None).await?; if !validation.valid { return Err(validation.error.unwrap_or("Invalid skill path".to_string())); } @@ -1152,7 +1199,7 @@ pub async fn delete_skill( let skill_path = std::path::PathBuf::from(&skill_info.path); if let Some(expected) = expected_import_id { - openbitfun_core::agentic::tools::implementations::skills::registry::imports::remove_imported_copy(&skill_path, &expected).await?; + skill_imports::remove_imported_copy(&skill_path, &expected).await?; } else if skill_path.exists() { if let Err(e) = tokio::fs::remove_dir_all(&skill_path).await { return Err(format!("Failed to delete skill folder: {}", e)); @@ -1177,6 +1224,35 @@ mod tests { use std::future; use tokio::time::Duration; + #[test] + fn skill_validation_reads_and_round_trips_legacy_payloads() { + let old = serde_json::json!({ "valid": true, "name": "demo", "description": "existing", "error": null }); + let parsed: super::SkillValidationResult = serde_json::from_value(old.clone()).unwrap(); + assert!(parsed.import_preview.is_none()); + assert_eq!(serde_json::to_value(parsed).unwrap(), old); + let new = serde_json::json!({ "valid": true, "name": "demo", "description": "existing", "error": null, + "importPreview": { "fingerprint": "reviewed", "fileCount": 2, "name": "demo", "description": "existing" } }); + let parsed: super::SkillValidationResult = serde_json::from_value(new.clone()).unwrap(); + assert_eq!(serde_json::to_value(parsed).unwrap(), new); + } + + #[tokio::test] + async fn legacy_skill_path_validation_remains_available_without_source_identity() { + let temp = tempfile::tempdir().unwrap(); + std::fs::write( + temp.path().join("SKILL.md"), + "---\nname: demo\ndescription: Existing\n---\nBody", + ) + .unwrap(); + let result = + super::validate_skill_path(temp.path().to_string_lossy().into_owned(), None, None) + .await + .unwrap(); + assert!(result.valid); + assert!(result.import_preview.is_none()); + assert_eq!(result.name.as_deref(), Some("demo")); + } + #[test] fn skill_scan_response_preserves_legacy_arrays_and_opt_in_diagnostics() { let report = super::SkillScanReport { diff --git a/src/crates/adapters/claude-code-adapter/src/agent_source.rs b/src/crates/adapters/claude-code-adapter/src/agent_source.rs index 7af07db0e7..d2e4a0e443 100644 --- a/src/crates/adapters/claude-code-adapter/src/agent_source.rs +++ b/src/crates/adapters/claude-code-adapter/src/agent_source.rs @@ -56,9 +56,9 @@ pub struct ClaudeCodeSubagentProviderOptions { impl ClaudeCodeSubagentProviderOptions { pub fn from_environment() -> Self { Self { - user_claude_dir: dirs::home_dir() - .unwrap_or_else(|| PathBuf::from(".")) - .join(".claude"), + user_claude_dir: crate::ClaudeCodeInstructionSourceOptions::from_environment() + .config_dir + .unwrap_or_default(), project_root_override: None, project_config_enabled: true, } @@ -140,6 +140,13 @@ impl ExternalSubagentSourceProvider for ClaudeCodeSubagentProvider { &self, input: &ExternalSubagentDiscoveryInput, ) -> Result { + if !self.options.user_claude_dir.is_absolute() { + return Err(ExternalSourceProviderError::new( + "claude.config_root_invalid", + "Claude Code configuration directory must be absolute", + false, + )); + } if input .context .workspace_root diff --git a/src/crates/adapters/claude-code-adapter/src/command_source.rs b/src/crates/adapters/claude-code-adapter/src/command_source.rs index 26712ecd06..a3a78e9466 100644 --- a/src/crates/adapters/claude-code-adapter/src/command_source.rs +++ b/src/crates/adapters/claude-code-adapter/src/command_source.rs @@ -39,9 +39,9 @@ pub struct ClaudeCodeCommandProviderOptions { impl ClaudeCodeCommandProviderOptions { pub fn from_environment() -> Self { Self { - user_claude_dir: dirs::home_dir() - .unwrap_or_else(|| PathBuf::from(".")) - .join(".claude"), + user_claude_dir: crate::ClaudeCodeInstructionSourceOptions::from_environment() + .config_dir + .unwrap_or_default(), project_root_override: None, project_config_enabled: true, } @@ -113,6 +113,13 @@ impl PromptCommandSourceProvider for ClaudeCodeCommandProvider { &self, context: &ExternalSourceContext, ) -> Result { + if !self.options.user_claude_dir.is_absolute() { + return Err(ExternalSourceProviderError::new( + "claude.config_root_invalid", + "Claude Code configuration directory must be absolute", + false, + )); + } if context .workspace_root .as_ref() @@ -345,7 +352,9 @@ impl PromptCommandSourceProvider for ClaudeCodeCommandProvider { fn watch_roots(&self, context: &ExternalSourceContext) -> Vec { let mut roots = BTreeMap::new(); - roots.insert(self.options.user_claude_dir.clone(), true); + if self.options.user_claude_dir.is_absolute() { + roots.insert(self.options.user_claude_dir.clone(), true); + } if self.options.project_config_enabled { if let Some(workspace) = &context.workspace_root { let project_root = self.project_root(workspace); diff --git a/src/crates/adapters/claude-code-adapter/src/hook_source.rs b/src/crates/adapters/claude-code-adapter/src/hook_source.rs index a77d9747ef..8e9f8b538a 100644 --- a/src/crates/adapters/claude-code-adapter/src/hook_source.rs +++ b/src/crates/adapters/claude-code-adapter/src/hook_source.rs @@ -60,9 +60,10 @@ pub struct ClaudeCodeHookProviderOptions { impl ClaudeCodeHookProviderOptions { pub fn from_environment() -> Self { - let user_settings_file = dirs::home_dir() - .unwrap_or_else(|| PathBuf::from(".")) - .join(".claude/settings.json"); + let user_settings_file = crate::ClaudeCodeInstructionSourceOptions::from_environment() + .config_dir + .unwrap_or_default() + .join("settings.json"); Self { user_settings_file, project_root_override: None, @@ -158,6 +159,13 @@ impl ExternalHookSourceProvider for ClaudeCodeHookProvider { &self, context: &ExternalSourceContext, ) -> Result { + if !self.options.user_settings_file.is_absolute() { + return Err(ExternalSourceProviderError::new( + "claude.config_root_invalid", + "Claude Code configuration directory must be absolute", + false, + )); + } if context .workspace_root .as_ref() @@ -253,6 +261,12 @@ impl ExternalHookSourceProvider for ClaudeCodeHookProvider { requested_source: &SourceKey, expected_catalog_content_version: &str, ) -> Result { + if !self.options.user_settings_file.is_absolute() { + return Err(import_error( + "claude.config_root_invalid", + "Claude Code configuration directory must be absolute", + )); + } if requested_source.provider_id.as_str() != PROVIDER_ID { return Err(import_error( "claude.hook.import_provider_mismatch", diff --git a/src/crates/adapters/claude-code-adapter/tests/claude_code_source_contracts/command_source.rs b/src/crates/adapters/claude-code-adapter/tests/claude_code_source_contracts/command_source.rs index 7276aec5fb..d542efb4d2 100644 --- a/src/crates/adapters/claude-code-adapter/tests/claude_code_source_contracts/command_source.rs +++ b/src/crates/adapters/claude-code-adapter/tests/claude_code_source_contracts/command_source.rs @@ -573,3 +573,100 @@ fn watch_roots_are_bounded_to_user_and_project_claude_directories() { .iter() .any(|root| root.path == fixture.workspace.join(".claude") && root.recursive)); } + +#[test] +fn configured_claude_directory_is_shared_by_providers_and_watch_roots() { + use openbitfun_claude_code_adapter::{ + ClaudeCodeHookProvider, ClaudeCodeHookProviderOptions, ClaudeCodeInstructionSourceOptions, + ClaudeCodeSubagentProvider, ClaudeCodeSubagentProviderOptions, + }; + use openbitfun_product_domains::external_hook_catalog::ExternalHookSourceProvider; + use openbitfun_product_domains::external_subagents::{ + ExternalSubagentDiscoveryInput, ExternalSubagentSourceProvider, + }; + struct Restore(Option); + impl Drop for Restore { + fn drop(&mut self) { + match &self.0 { + Some(value) => std::env::set_var("CLAUDE_CONFIG_DIR", value), + None => std::env::remove_var("CLAUDE_CONFIG_DIR"), + } + } + } + // Other fixtures inject explicit options; only this test changes this variable. + let _restore = Restore(std::env::var_os("CLAUDE_CONFIG_DIR")); + let temp = tempfile::tempdir().unwrap(); + let custom = temp.path().join("custom-claude"); + std::fs::create_dir_all(custom.join("commands")).unwrap(); + std::fs::create_dir_all(custom.join("agents")).unwrap(); + std::fs::write(custom.join("commands/review.md"), "Review this change.").unwrap(); + std::fs::write( + custom.join("agents/reviewer.md"), + "---\nname: reviewer\ndescription: Review changes\n---\nReview this change.", + ) + .unwrap(); + std::fs::write( + custom.join("settings.json"), + r#"{"hooks":{"PreToolUse":[{"hooks":[{"type":"command","command":"echo check"}]}]}}"#, + ) + .unwrap(); + std::env::set_var("CLAUDE_CONFIG_DIR", &custom); + assert_eq!( + ClaudeCodeInstructionSourceOptions::from_environment().config_dir, + Some(custom.clone()) + ); + let commands = + ClaudeCodeCommandProvider::new(ClaudeCodeCommandProviderOptions::from_environment()); + let agents = + ClaudeCodeSubagentProvider::new(ClaudeCodeSubagentProviderOptions::from_environment()); + let hooks = ClaudeCodeHookProvider::new(ClaudeCodeHookProviderOptions::from_environment()); + let context = openbitfun_product_domains::external_sources::ExternalSourceContext { + workspace_root: None, + execution_domain_id: openbitfun_product_domains::external_sources::ExecutionDomainId::new( + "local-user", + ) + .unwrap(), + }; + assert_eq!(commands.discover(&context).unwrap().commands.len(), 1); + assert_eq!( + agents + .discover(&ExternalSubagentDiscoveryInput { + context: context.clone(), + suppressed_sources: Default::default() + }) + .unwrap() + .definitions + .len(), + 1 + ); + assert_eq!(hooks.discover(&context).unwrap().sources.len(), 1); + assert!(commands + .watch_roots(&context) + .iter() + .any(|root| root.path == custom)); + assert!(agents + .watch_roots(&context) + .iter() + .any(|root| root.path == custom)); + for invalid in ["", "relative", "~/claude"] { + std::env::set_var("CLAUDE_CONFIG_DIR", invalid); + assert!(ClaudeCodeInstructionSourceOptions::from_environment() + .config_dir + .is_none()); + assert!(ClaudeCodeCommandProvider::default() + .discover(&context) + .is_err()); + assert!(ClaudeCodeSubagentProvider::default() + .discover(&ExternalSubagentDiscoveryInput { + context: context.clone(), + suppressed_sources: Default::default() + }) + .is_err()); + assert!(ClaudeCodeHookProvider::default() + .discover(&context) + .is_err()); + assert!(ClaudeCodeCommandProvider::default() + .watch_roots(&context) + .is_empty()); + } +} diff --git a/src/crates/adapters/opencode-adapter/src/lib.rs b/src/crates/adapters/opencode-adapter/src/lib.rs index fc557f9fb6..22ac569145 100644 --- a/src/crates/adapters/opencode-adapter/src/lib.rs +++ b/src/crates/adapters/opencode-adapter/src/lib.rs @@ -31,7 +31,8 @@ pub use reference_source::{ OpenCodeWorkspaceReferenceProvider, OpenCodeWorkspaceReferenceProviderOptions, }; pub use skill_source::{ - OpenCodeConfiguredSkillRoot, OpenCodeSkillRootProvider, OpenCodeSkillRootProviderOptions, + OpenCodeConfiguredSkillRoot, OpenCodeSkillRootDiagnostic, OpenCodeSkillRootProvider, + OpenCodeSkillRootProviderOptions, OpenCodeSkillRootReport, }; pub use source_adapter::{ load_opencode_config_snapshot, load_opencode_package_adapter, OpenCodeConfigSnapshot, diff --git a/src/crates/adapters/opencode-adapter/src/skill_source.rs b/src/crates/adapters/opencode-adapter/src/skill_source.rs index 93a5d24532..cf8acee16c 100644 --- a/src/crates/adapters/opencode-adapter/src/skill_source.rs +++ b/src/crates/adapters/opencode-adapter/src/skill_source.rs @@ -40,6 +40,34 @@ pub struct OpenCodeConfiguredSkillRoot { pub precedence: usize, } +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct OpenCodeSkillRootDiagnostic { + pub location: String, + pub message: String, +} + +#[derive(Debug, Default)] +pub struct OpenCodeSkillRootReport { + pub roots: Vec, + pub diagnostics: Vec, +} + +impl OpenCodeSkillRootReport { + fn diagnose(&mut self, location: String, message: &str) { + if self.diagnostics.len() < MAX_CONFIGURED_SKILL_ROOTS { + self.diagnostics.push(OpenCodeSkillRootDiagnostic { + location, + message: message.into(), + }); + } else if self.diagnostics.len() == MAX_CONFIGURED_SKILL_ROOTS { + self.diagnostics.push(OpenCodeSkillRootDiagnostic { + location: "OpenCode skills".into(), + message: "Additional configured skill root diagnostics were omitted".into(), + }); + } + } +} + pub struct OpenCodeSkillRootProvider { config: OpenCodeLocalConfigOptions, home_dir: Option, @@ -54,6 +82,27 @@ impl OpenCodeSkillRootProvider { } pub fn discover(&self, workspace_root: Option<&Path>) -> Vec { + self.discover_with_diagnostics(workspace_root).roots + } + + pub fn discover_with_diagnostics( + &self, + workspace_root: Option<&Path>, + ) -> OpenCodeSkillRootReport { + let mut report = OpenCodeSkillRootReport::default(); + if let Some(config) = &self.config.explicit_config_file { + let resolved = if config.is_absolute() { + Some(config.clone()) + } else { + workspace_root.map(|root| root.join(config)) + }; + if resolved.as_ref().is_none_or(|path| !path.is_file()) { + report.diagnose( + "OPENCODE_CONFIG".into(), + "Explicit configuration file is missing, unreadable, or cannot be resolved", + ); + } + } let canonical_workspace = workspace_root .map(find_project_root) .and_then(|path| dunce::canonicalize(path).ok()); @@ -68,18 +117,33 @@ impl OpenCodeSkillRootProvider { let LocalSourcePlanItem::Config(document) = item else { continue; }; - let Some(paths) = read_local_skill_paths(&document) else { - continue; + let paths = match read_local_skill_paths(&document, &mut report) { + Ok(paths) => paths, + Err(message) => { + report.diagnose(document.location(), message); + continue; + } }; for value in paths { let current_precedence = precedence; precedence = precedence.saturating_add(1); - configured_paths.push((value, document.scope, current_precedence)); + configured_paths.push(( + value, + document.scope, + current_precedence, + document.location(), + )); } } + if configured_paths.len() > MAX_CONFIGURED_SKILL_ROOTS { + report.diagnose( + "OpenCode skills".into(), + "Configured skill root limit exceeded; only the latest 64 entries are inspected", + ); + } let mut contributions = Vec::new(); - for (value, source_scope, current_precedence) in configured_paths + for (value, source_scope, current_precedence, location) in configured_paths .into_iter() .rev() .take(MAX_CONFIGURED_SKILL_ROOTS) @@ -90,12 +154,21 @@ impl OpenCodeSkillRootProvider { let Some(path) = resolve_configured_path(&value, workspace_root, self.home_dir.as_deref()) else { + report.diagnose( + location, + "Configured skill path is invalid or requires a workspace or home directory", + ); continue; }; let Ok(path) = dunce::canonicalize(path) else { + report.diagnose( + location, + "Configured skill directory is missing or unreadable", + ); continue; }; if !path.is_dir() { + report.diagnose(location, "Configured skill path is not a directory"); continue; } let workspace_scoped = canonical_workspace @@ -112,7 +185,13 @@ impl OpenCodeSkillRootProvider { } ExternalSourceScope::UserGlobal if workspace_scoped => ExternalSourceScope::Project, ExternalSourceScope::UserGlobal if home_scoped => ExternalSourceScope::UserGlobal, - _ => continue, + _ => { + report.diagnose( + location, + "Configured skill directory is outside the allowed source boundary", + ); + continue; + } }; contributions.push(OpenCodeConfiguredSkillRoot { path, @@ -122,10 +201,11 @@ impl OpenCodeSkillRootProvider { } let mut seen = BTreeSet::new(); - contributions + report.roots = contributions .into_iter() .filter(|root| seen.insert(root.path.clone())) - .collect() + .collect(); + report } } @@ -135,22 +215,55 @@ impl Default for OpenCodeSkillRootProvider { } } -fn read_local_skill_paths(document: &LocalConfigDocument) -> Option> { - let content = match document.read_bounded(MAX_CONFIG_FILE_BYTES).ok()? { +fn read_local_skill_paths( + document: &LocalConfigDocument, + report: &mut OpenCodeSkillRootReport, +) -> Result, &'static str> { + let content = match document + .read_bounded(MAX_CONFIG_FILE_BYTES) + .map_err(|_| "Cannot read OpenCode configuration")? + { BoundedTextRead::Content(content) => content, - BoundedTextRead::TooLarge | BoundedTextRead::InvalidUtf8 => return None, + BoundedTextRead::TooLarge => return Err("OpenCode configuration exceeds the size limit"), + BoundedTextRead::InvalidUtf8 => return Err("OpenCode configuration is not UTF-8"), }; - let document = serde_json::from_str::(&strip_jsonc(&content)).ok()?; - match document.get("skills")? { - Value::Object(skills) => strict_string_array(skills.get("paths")) - .map(|paths| local_only_paths(paths.into_iter())), - Value::Array(skills) => skills - .iter() - .map(Value::as_str) - .collect::>>() - .map(|paths| local_only_paths(paths.into_iter())), + let value = serde_json::from_str::(&strip_jsonc(&content)) + .map_err(|_| "OpenCode configuration is not valid JSON/JSONC")?; + if !value.is_object() { + return Err("OpenCode configuration must be an object"); + } + let paths = match value.get("skills") { + None => return Ok(Vec::new()), + Some(Value::Object(skills)) => { + if skills + .get("urls") + .is_some_and(|urls| urls.as_array().is_none_or(|urls| !urls.is_empty())) + { + report.diagnose( + document.location(), + "Remote skill URLs are not supported by static discovery", + ); + } + strict_string_array(skills.get("paths")) + } + Some(Value::Array(skills)) => skills.iter().map(Value::as_str).collect(), _ => None, } + .ok_or("Configured skill paths must be an array of strings")?; + Ok(paths + .into_iter() + .filter_map(|path| { + if is_remote_url(path.trim()) { + report.diagnose( + document.location(), + "Remote skill URLs are not supported by static discovery", + ); + None + } else { + Some(path.to_string()) + } + }) + .collect()) } fn strict_string_array(value: Option<&Value>) -> Option> { @@ -160,13 +273,6 @@ fn strict_string_array(value: Option<&Value>) -> Option> { value.as_array()?.iter().map(Value::as_str).collect() } -fn local_only_paths<'a>(paths: impl Iterator) -> Vec { - paths - .filter(|value| !is_remote_url(value)) - .map(str::to_string) - .collect() -} - fn is_remote_url(value: &str) -> bool { url::Url::parse(value).is_ok_and(|url| matches!(url.scheme(), "http" | "https")) } diff --git a/src/crates/adapters/opencode-adapter/tests/opencode_static_source_contracts/opencode_skill_roots.rs b/src/crates/adapters/opencode-adapter/tests/opencode_static_source_contracts/opencode_skill_roots.rs index 94d2201d0e..c8d447f957 100644 --- a/src/crates/adapters/opencode-adapter/tests/opencode_static_source_contracts/opencode_skill_roots.rs +++ b/src/crates/adapters/opencode-adapter/tests/opencode_static_source_contracts/opencode_skill_roots.rs @@ -231,3 +231,96 @@ fn inline_config_paths_are_applied_last_and_stay_workspace_scoped() { assert_eq!(roots[1].path, dunce::canonicalize(inline_root).unwrap()); assert_eq!(roots[1].scope, ExternalSourceScope::Project); } + +#[test] +fn diagnostics_explain_bad_sources_without_hiding_valid_roots_or_secrets() { + let fixture = Fixture::new(); + fs::create_dir_all(fixture.project.join("valid")).unwrap(); + write( + fixture.user_config.join("opencode.json"), + r#"{"secret":"do-not-display",broken}"#, + ); + write( + fixture.project.join("opencode.json"), + r#"{"skills":["../../valid", "../../missing", "https://user:secret@example.test/skills", "../../opencode.json", "../../../home"]}"#, + ); + let report = fixture + .provider() + .discover_with_diagnostics(Some(&fixture.opened_directory)); + assert_eq!(report.roots.len(), 1); + assert_eq!(report.diagnostics.len(), 5); + let messages = format!("{:?}", report.diagnostics); + for expected in [ + "JSON/JSONC", + "missing or unreadable", + "not a directory", + "outside", + "URLs", + ] { + assert!(messages.contains(expected), "{messages}"); + } + assert!(!messages.contains("secret")); + assert!(!messages.contains("example.test")); + write(fixture.user_config.join("opencode.json"), "{}"); + write( + fixture.project.join("opencode.json"), + r#"{"skills":["../../valid"]}"#, + ); + let refreshed = fixture + .provider() + .discover_with_diagnostics(Some(&fixture.opened_directory)); + assert_eq!(refreshed.roots.len(), 1); + assert!(refreshed.diagnostics.is_empty()); +} + +#[test] +fn diagnostics_are_bounded_and_absent_default_configuration_is_quiet() { + let fixture = Fixture::new(); + assert!(fixture + .provider() + .discover_with_diagnostics(None) + .diagnostics + .is_empty()); + write( + fixture.user_config.join("opencode.json"), + &serde_json::json!({"skills": vec!["https://secret@example.test"; 200]}).to_string(), + ); + let report = fixture.provider().discover_with_diagnostics(None); + assert!(report.roots.is_empty()); + assert_eq!(report.diagnostics.len(), 65); + assert!(report + .diagnostics + .last() + .unwrap() + .message + .contains("omitted")); +} + +#[test] +fn unreadable_configuration_and_missing_explicit_file_are_reported() { + let fixture = Fixture::new(); + fs::write(fixture.user_config.join("opencode.json"), [255, 254]).unwrap(); + let mut options = OpenCodeSkillRootProviderOptions { + config: OpenCodeCommandProviderOptions { + explicit_config_file: Some(fixture.home.join("missing.json")), + user_config_dir: fixture.user_config.clone(), + legacy_user_config_dir: None, + explicit_config_dir: None, + inline_config_content: None, + project_config_enabled: true, + }, + home_dir: Some(fixture.home.clone()), + }; + let report = OpenCodeSkillRootProvider::new(options.clone()).discover_with_diagnostics(None); + assert_eq!(report.diagnostics.len(), 2); + assert!(report + .diagnostics + .iter() + .any(|entry| entry.message.contains("UTF-8"))); + options.config.inline_config_content = Some(" ".repeat(1024 * 1024 + 1)); + let report = OpenCodeSkillRootProvider::new(options).discover_with_diagnostics(None); + assert!(report + .diagnostics + .iter() + .any(|entry| entry.message.contains("size limit"))); +} diff --git a/src/crates/assembly/core/src/agentic/tools/implementations/skills/registry.rs b/src/crates/assembly/core/src/agentic/tools/implementations/skills/registry.rs index 0deaae4abd..3e28c1cd98 100644 --- a/src/crates/assembly/core/src/agentic/tools/implementations/skills/registry.rs +++ b/src/crates/assembly/core/src/agentic/tools/implementations/skills/registry.rs @@ -264,6 +264,81 @@ mod local_skill_scan_tests { use std::fs; use std::path::Path; + #[tokio::test] + async fn codex_home_override_discovers_skills_without_changing_persisted_keys() { + let temp = tempfile::tempdir().unwrap(); + let home = temp.path().join("home"); + let custom = temp.path().join("custom-codex"); + write_skill(&custom.join("skills/shared-review")); + let spec = super::USER_HOME_SKILL_ROOTS + .iter() + .find(|root| root.slot == "home.codex") + .unwrap(); + let resolved = SkillRegistry::user_skill_root_path_with_environment(spec, &home, |name| { + (name == "CODEX_HOME").then(|| custom.to_string_lossy().into_owned()) + }); + assert_eq!(resolved, custom.join("skills")); + let mut root = test_root(resolved); + root.slot = spec.slot; + root.source_id = spec.source_id; + let scan = SkillRegistry::scan_skills_in_dir(&root).await; + assert!(scan.diagnostics.is_empty()); + assert_eq!(scan.candidates.len(), 1); + assert_eq!( + scan.candidates[0].info.key, + "user::home.codex::shared-review" + ); + assert_eq!( + SkillRegistry::user_skill_root_path_with_environment(spec, &home, |_| None), + home.join(".codex/skills") + ); + assert_eq!( + SkillRegistry::user_skill_root_path_with_environment(spec, &home, |_| Some( + String::new() + )), + home.join(".codex/skills") + ); + assert_eq!( + SkillRegistry::user_skill_root_path_with_environment(spec, &home, |_| Some( + "~/custom".into() + )), + home.join("custom/skills") + ); + } + + #[tokio::test] + async fn claude_config_root_keeps_source_identity_and_rejects_relative_roots() { + let temp = tempfile::tempdir().unwrap(); + let custom = temp.path().join("custom-claude"); + write_skill(&custom.join("skills/shared-review")); + let spec = super::USER_HOME_SKILL_ROOTS + .iter() + .find(|root| root.slot == "home.claude") + .unwrap(); + let path = + SkillRegistry::user_skill_root_path_with_environment(spec, temp.path(), |name| { + (name == "CLAUDE_CONFIG_DIR").then(|| custom.to_string_lossy().into_owned()) + }); + let mut root = test_root(path); + root.slot = spec.slot; + root.source_id = spec.source_id; + let scan = SkillRegistry::scan_skills_in_dir(&root).await; + assert_eq!( + scan.candidates[0].info.key, + "user::home.claude::shared-review" + ); + for invalid in ["", "relative", "~/claude"] { + root.path = + SkillRegistry::user_skill_root_path_with_environment(spec, temp.path(), |_| { + Some(invalid.into()) + }); + let scan = SkillRegistry::scan_skills_in_dir(&root).await; + assert!(scan.candidates.is_empty()); + assert_eq!(scan.diagnostics.len(), 1); + assert!(!scan.cacheable); + } + } + fn write_skill(path: &Path) { fs::create_dir_all(path).expect("skill directory"); fs::write( @@ -762,13 +837,31 @@ impl SkillRegistry { spec: &openbitfun_agent_runtime::skills::SkillRootSpec, home: &Path, ) -> PathBuf { + Self::user_skill_root_path_with_environment(spec, home, |name| std::env::var(name).ok()) + } + + fn user_skill_root_path_with_environment( + spec: &openbitfun_agent_runtime::skills::SkillRootSpec, + home: &Path, + environment: impl Fn(&str) -> Option, + ) -> PathBuf { + // Claude config roots, like its Instruction provider, must be absolute. + // Keep invalid explicit input relative so discovery reports it instead + // of silently reading the default home or expanding a different root. + if spec.slot == "home.claude" { + return environment("CLAUDE_CONFIG_DIR") + .map(PathBuf::from) + .unwrap_or_else(|| home.join(spec.parent)) + .join(spec.subdir); + } let variable = match spec.slot { + "home.codex" => Some("CODEX_HOME"), "home.dsh" => Some("DSH_HOME"), "home.pi" => Some("PI_CODING_AGENT_DIR"), _ => None, }; let root = variable - .and_then(|name| std::env::var(name).ok()) + .and_then(environment) .filter(|value| !value.trim().is_empty()); let root = root .map(|value| { @@ -885,8 +978,10 @@ impl SkillRegistry { let mut roots = Vec::new(); let home_dir = dirs::home_dir(); if let Some(home) = home_dir.as_deref() { - roots.extend(USER_HOME_SKILL_ROOTS.iter().map(|spec| { - LocalSkillWatchRoot::recursive(Self::user_skill_root_path(spec, home)) + roots.extend(USER_HOME_SKILL_ROOTS.iter().filter_map(|spec| { + let path = Self::user_skill_root_path(spec, home); + (spec.slot != "home.claude" || path.is_absolute()) + .then(|| LocalSkillWatchRoot::recursive(path)) })); } @@ -1005,7 +1100,14 @@ impl SkillRegistry { // from user config may become project-scoped for the current workspace. // Discover and scan them once per request so scope and the 64-root cap // are applied to one coherent OpenCode configuration snapshot. - let roots = opencode_configured_skill_roots(workspace_root); + let (roots, root_diagnostics) = opencode_configured_skill_roots(workspace_root); + diagnostics.extend(root_diagnostics.into_iter().map(|(path, message)| { + SkillScanDiagnostic { + path, + source_id: "opencode".to_string(), + message, + } + })); let configured_scan = Self::scan_configured_opencode_candidates_with_diagnostics(roots).await; diagnostics.extend(configured_scan.diagnostics); diff --git a/src/crates/assembly/core/src/agentic/tools/implementations/skills/registry/discovery.rs b/src/crates/assembly/core/src/agentic/tools/implementations/skills/registry/discovery.rs index 35e5d1aed5..b27247cb4a 100644 --- a/src/crates/assembly/core/src/agentic/tools/implementations/skills/registry/discovery.rs +++ b/src/crates/assembly/core/src/agentic/tools/implementations/skills/registry/discovery.rs @@ -369,6 +369,17 @@ impl SkillRegistry { } pub(super) async fn scan_skills_in_dir(entry: &SkillRootEntry) -> LocalSkillScan { + if entry.slot == "home.claude" && !entry.path.is_absolute() { + return LocalSkillScan { + candidates: Vec::new(), + diagnostics: vec![diagnostic( + "$CLAUDE_CONFIG_DIR", + entry.source_id, + "Claude Code configuration directory must be absolute", + )], + cacheable: false, + }; + } let mut scan = LocalSkillScan { candidates: Vec::new(), diagnostics: Vec::new(), diff --git a/src/crates/assembly/core/src/agentic/tools/implementations/skills/registry/imports.rs b/src/crates/assembly/core/src/agentic/tools/implementations/skills/registry/imports.rs index 8d7b117dbf..38ab93ae08 100644 --- a/src/crates/assembly/core/src/agentic/tools/implementations/skills/registry/imports.rs +++ b/src/crates/assembly/core/src/agentic/tools/implementations/skills/registry/imports.rs @@ -99,31 +99,117 @@ fn package_files(root: &Path) -> Result, String> { Ok(files) } -pub fn package_fingerprint(root: &Path) -> Result { +#[derive(Debug, Clone, serde::Serialize, serde::Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct SkillImportPreview { + pub fingerprint: String, + pub file_count: usize, + pub name: String, + pub description: String, +} + +fn source_files(source: &SkillInfo, root: &Path) -> Result, String> { + if let Some(entry) = source + .entry_file + .as_deref() + .filter(|entry| *entry != "SKILL.md") + { + if entry.is_empty() || entry == "." || entry == ".." || entry.contains(['/', '\\', '\0']) { + return Err("Invalid Skill entry file".into()); + } + let path = root.join(entry); + let metadata = std::fs::symlink_metadata(&path).map_err(|error| error.to_string())?; + if is_symlink_or_reparse(&metadata) || !metadata.is_file() { + return Err("Skill entry is not a standalone file".into()); + } + Ok(vec![(path, "SKILL.md".into())]) + } else { + relative_package_files(root) + } +} + +fn relative_package_files(root: &Path) -> Result, String> { + package_files(root)? + .into_iter() + .map(|path| { + let relative = path + .strip_prefix(root) + .map_err(|error| error.to_string())? + .to_path_buf(); + Ok((path, relative)) + }) + .collect() +} + +fn fingerprint_files( + files: &[(PathBuf, PathBuf)], + capture_entry: bool, +) -> Result<(String, Vec), String> { + use std::io::Read; let mut digest = Sha256::new(); - for path in package_files(root)? { - let relative = path.strip_prefix(root).map_err(|error| error.to_string())?; + let mut entry = Vec::new(); + for (path, relative) in files { let relative = relative.to_string_lossy().replace('\\', "/"); digest.update((relative.len() as u64).to_le_bytes()); digest.update(relative.as_bytes()); - let mut file = std::fs::File::open(&path).map_err(|error| error.to_string())?; - digest.update( - file.metadata() - .map_err(|error| error.to_string())? - .len() - .to_le_bytes(), - ); + let mut file = std::fs::File::open(path).map_err(|error| error.to_string())?; + let size = file.metadata().map_err(|error| error.to_string())?.len(); + digest.update(size.to_le_bytes()); + let mut read_size = 0u64; let mut buffer = [0u8; 64 * 1024]; loop { - use std::io::Read; let count = file.read(&mut buffer).map_err(|error| error.to_string())?; if count == 0 { break; } + read_size += count as u64; digest.update(&buffer[..count]); + if capture_entry && relative == "SKILL.md" { + if entry.len() + count > 1024 * 1024 { + return Err("Skill entry exceeds the preview size limit".into()); + } + entry.extend_from_slice(&buffer[..count]); + } } + if read_size != size { + return Err("Skill package changed while being read; refresh the preview".into()); + } + } + Ok((format!("{:x}", digest.finalize()), entry)) +} + +pub fn package_fingerprint(root: &Path) -> Result { + fingerprint_files(&relative_package_files(root)?, false).map(|(fingerprint, _)| fingerprint) +} + +/// Read only the files that will be copied, including the normalized flat Pi entry. +/// Display metadata and the digest come from the same entry bytes. +pub async fn preview_import(source: SkillInfo) -> Result { + if source.is_builtin || source.source_id == OPENBITFUN_SKILL_SOURCE_ID { + return Err("Expected an external Skill source".into()); } - Ok(format!("{:x}", digest.finalize())) + tokio::task::spawn_blocking(move || { + let root = std::fs::canonicalize(&source.path).map_err(|error| error.to_string())?; + let files = source_files(&source, &root)?; + let (fingerprint, entry) = fingerprint_files(&files, true)?; + let content = String::from_utf8(entry).map_err(|_| "Skill entry is not UTF-8")?; + let data = SkillRegistry::parse_skill_markdown( + source.parser_path(), + &content, + source.level, + false, + &source.source_slot, + ) + .map_err(|error| error.to_string())?; + Ok(SkillImportPreview { + fingerprint, + file_count: files.len(), + name: data.name, + description: data.description, + }) + }) + .await + .map_err(|error| error.to_string())? } /// Idempotently imports one discovered external package. Existing distinct user @@ -141,6 +227,17 @@ pub async fn import_copy_as( source: SkillInfo, target_root: PathBuf, target_name: Option, +) -> Result { + import_copy_as_reviewed(source, target_root, target_name, None).await +} + +/// Compare the reviewed digest against the staged bytes before publishing or adopting a copy. +/// Existing callers may omit the digest; new hosts advertise reviewed import support separately. +pub async fn import_copy_as_reviewed( + source: SkillInfo, + target_root: PathBuf, + target_name: Option, + expected_fingerprint: Option, ) -> Result { if let Some(name) = &target_name { let reserved = name @@ -217,32 +314,16 @@ pub async fn import_copy_as( let staging = staging_root.join(&import_id); disk::create_dir(&staging).map_err(|error| error.to_string())?; let prepared = (|| { - if let Some(entry) = source - .entry_file - .as_deref() - .filter(|entry| *entry != "SKILL.md") - { - if entry.contains(['/', '\\']) { - return Err("Invalid Skill entry file".into()); + for (file, relative) in source_files(&source, &source_root)? { + let destination = staging.join(relative); + if let Some(parent) = destination.parent() { + disk::create_dir_all(parent).map_err(|error| error.to_string())?; } - if is_symlink_or_reparse( - &disk::symlink_metadata(source_root.join(entry)) - .map_err(|error| error.to_string())?, - ) { - return Err("Skill entry is linked; import a standalone copy".into()); - } - disk::copy(source_root.join(entry), staging.join("SKILL.md")) - .map_err(|error| error.to_string())?; - } else { - for file in package_files(&source_root)? { - let destination = staging.join( - file.strip_prefix(&source_root) - .map_err(|error| error.to_string())?, - ); - if let Some(parent) = destination.parent() { - disk::create_dir_all(parent).map_err(|error| error.to_string())?; - } - disk::copy(file, destination).map_err(|error| error.to_string())?; + disk::copy(file, destination).map_err(|error| error.to_string())?; + } + if let Some(expected) = &expected_fingerprint { + if expected != &package_fingerprint(&staging)? { + return Err("skill_import_stale: Skill package changed; review it again before importing".into()); } } let mut markdown = disk::read_to_string(staging.join("SKILL.md")) @@ -395,6 +476,102 @@ mod tests { .info } + #[tokio::test] + async fn reviewed_import_rejects_entry_asset_and_file_set_changes_without_publishing() { + for mutation in ["entry", "asset", "added", "removed", "renamed"] { + let temp = tempfile::tempdir().unwrap(); + let source = source(temp.path()).await; + let preview = preview_import(source.clone()).await.unwrap(); + assert_eq!(preview.name, "demo"); + assert_eq!(preview.description, "Imported workflow"); + assert_eq!(preview.file_count, 2); + // Golden digest from the existing v1 package fingerprint format. + assert_eq!( + preview.fingerprint, + "dc337461d9f3e60efe4482bb17667dc1572c73c336a55a976ef0b98e2785bb6a" + ); + let package = Path::new(&source.path); + match mutation { + "entry" => fs::write( + package.join("SKILL.md"), + "---\ndescription: Changed\n---\nChanged", + ) + .await + .unwrap(), + "asset" => fs::write(package.join("scripts/tool.py"), "changed asset") + .await + .unwrap(), + "added" => fs::write(package.join("extra.txt"), "new dependency") + .await + .unwrap(), + "removed" => fs::remove_file(package.join("scripts/tool.py")) + .await + .unwrap(), + _ => fs::rename( + package.join("scripts/tool.py"), + package.join("scripts/renamed.py"), + ) + .await + .unwrap(), + } + let target = temp.path().join("native"); + let error = import_copy_as_reviewed( + source, + target.clone(), + Some("reviewed-alias".into()), + Some(preview.fingerprint), + ) + .await + .unwrap_err(); + assert!(error.starts_with("skill_import_stale:"), "{error}"); + assert!(!target.join("reviewed-alias").exists()); + assert_eq!( + std::fs::read_dir(temp.path().join("skill-import-staging")) + .unwrap() + .count(), + 0 + ); + } + } + + #[tokio::test] + async fn reviewed_alias_is_idempotent_and_never_replaces_user_edits() { + let temp = tempfile::tempdir().unwrap(); + let source = source(temp.path()).await; + let preview = preview_import(source.clone()).await.unwrap(); + let target = temp.path().join("native"); + let first = import_copy_as_reviewed( + source.clone(), + target.clone(), + Some("alias".into()), + Some(preview.fingerprint.clone()), + ) + .await + .unwrap(); + fs::write(target.join("alias/scripts/tool.py"), "user edit") + .await + .unwrap(); + let repeated = import_copy_as_reviewed( + source, + target.clone(), + Some("alias".into()), + Some(preview.fingerprint), + ) + .await + .unwrap(); + assert_eq!(first, repeated); + assert!(fs::read_to_string(target.join("alias/SKILL.md")) + .await + .unwrap() + .contains("name: alias")); + assert_eq!( + fs::read_to_string(target.join("alias/scripts/tool.py")) + .await + .unwrap(), + "user edit" + ); + } + #[tokio::test] async fn imported_package_preserves_dialect_assets_identity_and_runtime_selection() { let temp = tempfile::tempdir().unwrap(); @@ -523,7 +700,31 @@ mod tests { .unwrap(); assert_eq!(loaded.name, "demo"); let target = temp.path().join("native"); - import_copy(source, target.clone()).await.unwrap(); + let preview = preview_import(source.clone()).await.unwrap(); + assert_eq!(preview.file_count, 1); + fs::write(Path::new(&source.path).join("other.md"), "unrelated change") + .await + .unwrap(); + import_copy_as_reviewed( + source.clone(), + target.clone(), + None, + Some(preview.fingerprint.clone()), + ) + .await + .unwrap(); + fs::write(Path::new(&source.path).join("demo.md"), "changed entry") + .await + .unwrap(); + assert!(import_copy_as_reviewed( + source, + target.clone(), + Some("other-alias".into()), + Some(preview.fingerprint) + ) + .await + .unwrap_err() + .starts_with("skill_import_stale:")); assert!(target.join("demo/SKILL.md").is_file()); assert!(!target.join("demo/other.md").exists()); } diff --git a/src/crates/assembly/core/src/external_sources.rs b/src/crates/assembly/core/src/external_sources.rs index 67293d6326..33abb45036 100644 --- a/src/crates/assembly/core/src/external_sources.rs +++ b/src/crates/assembly/core/src/external_sources.rs @@ -548,7 +548,10 @@ pub(crate) struct LocalConfiguredSkillRootContribution { pub(crate) fn opencode_configured_skill_roots( workspace_root: Option<&Path>, -) -> Vec { +) -> ( + Vec, + Vec<(String, String)>, +) { opencode_configured_skill_roots_with_provider( workspace_root, &OpenCodeSkillRootProvider::default(), @@ -558,16 +561,28 @@ pub(crate) fn opencode_configured_skill_roots( fn opencode_configured_skill_roots_with_provider( workspace_root: Option<&Path>, provider: &OpenCodeSkillRootProvider, -) -> Vec { - provider - .discover(workspace_root) +) -> ( + Vec, + Vec<(String, String)>, +) { + let report = provider.discover_with_diagnostics(workspace_root); + let roots = report + .roots .into_iter() .map(|root| LocalConfiguredSkillRootContribution { path: root.path, scope: root.scope, precedence: root.precedence, }) - .collect() + .collect(); + ( + roots, + report + .diagnostics + .into_iter() + .map(|diagnostic| (diagnostic.location, diagnostic.message)) + .collect(), + ) } async fn finalize_prompt_command_expansion( @@ -8984,7 +8999,9 @@ mod tests { home_dir: Some(home), }); - let roots = opencode_configured_skill_roots_with_provider(Some(&project), &provider); + let (roots, diagnostics) = + opencode_configured_skill_roots_with_provider(Some(&project), &provider); + assert!(diagnostics.is_empty()); assert_eq!(roots.len(), 1); assert_eq!( diff --git a/src/web-ui/src/app/scenes/ecosystem-compatibility/ExternalAgentContent.test.tsx b/src/web-ui/src/app/scenes/ecosystem-compatibility/ExternalAgentContent.test.tsx index e86f54b1b6..e608f743a8 100644 --- a/src/web-ui/src/app/scenes/ecosystem-compatibility/ExternalAgentContent.test.tsx +++ b/src/web-ui/src/app/scenes/ecosystem-compatibility/ExternalAgentContent.test.tsx @@ -2,14 +2,20 @@ import React, { act } from 'react'; import { createRoot, type Root } from 'react-dom/client'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { globalEventBus } from '@/infrastructure/event-bus'; +import { MCP_CONFIG_CHANGED } from '@/infrastructure/mcp/configEvents'; import type { ExternalSourceCatalogSnapshot } from '@/infrastructure/api/service-api/ExternalSourcesAPI'; import { buildEcosystemProductRuntimes, type EcosystemProductId } from './ecosystemCompatibilityModel'; const mocks = vi.hoisted(() => ({ + openScene: vi.fn(), openDestination: vi.fn(), openNativeSkills: vi.fn(), deleteSkill: vi.fn(), loadMcp: vi.fn(), saveMcp: vi.fn(), mutateHook: vi.fn(), getSkills: vi.fn(), validateSkill: vi.fn(), addSkill: vi.fn(), getHooks: vi.fn(), getHookCatalog: vi.fn(), planHook: vi.fn(), applyHook: vi.fn(), planMcp: vi.fn(), applyMcp: vi.fn(), refresh: vi.fn(), workspacePath: '/project', remote: false, peer: false, skillImportVersion: 0, })); +vi.mock('@/app/stores/sceneStore', () => ({ useSceneStore: { getState: () => ({ openScene: mocks.openScene }) } })); +vi.mock('@/app/scenes/settings/settingsStore', () => ({ useSettingsStore: { getState: () => ({ openDestination: mocks.openDestination }) } })); +vi.mock('@/app/scenes/skills/skillsSceneStore', () => ({ useSkillsSceneStore: { getState: () => ({ openNativeSkills: mocks.openNativeSkills }) } })); vi.mock('@/infrastructure/i18n', () => ({ useI18n: () => ({ t: (key: string) => key, formatNumber: String }) })); vi.mock('@/infrastructure/contexts/WorkspaceContext', () => ({ useCurrentWorkspace: () => ({ workspacePath: mocks.workspacePath, workspace: { workspaceKind: mocks.remote ? 'remote' : 'normal' }, @@ -125,6 +131,41 @@ describe('external agent content and explicit import boundary', () => { if (trigger.getAttribute('aria-expanded') !== 'true') await act(async () => trigger.click()); } + it('keeps copy management on imported rows without category management links', async () => { + data.plan.items[0].disposition = 'already_imported'; + await render(); + expect(container.textContent).not.toContain('content.manageNative'); + await click('content.manageCopy', 'mcp'); + expect(mocks.openDestination).toHaveBeenCalledWith({ pageId: 'tools.mcp' }); + expect(mocks.openScene).toHaveBeenCalledWith('settings'); + }); + + it.each(['single', 'batch'])('binds the %s Skill import to its reviewed package and reports stale content', async (mode) => { + mocks.skillImportVersion = 3; + mocks.validateSkill.mockResolvedValue({ valid: true, importPreview: { + fingerprint: 'reviewed-package', fileCount: 2, name: 'fresh-name', description: 'Fresh description', + } }); + mocks.addSkill.mockRejectedValue(new Error('skill_import_stale: package changed')); + await render(); + if (mode === 'single') await click('content.prepareImport', 'skill'); + else await click('content.importAll'); + expect(mocks.validateSkill).toHaveBeenCalledWith(data.skills[0].path, { sourceKey: data.skills[0].key, workspacePath: '/project' }); + expect(container.querySelector('[role="dialog"]')?.textContent).toContain('content.reviewedPackage'); + expect(container.querySelector('[role="dialog"]')?.textContent).toContain('fresh-name'); + expect(mocks.addSkill).not.toHaveBeenCalled(); + await click('content.confirm'); + expect(mocks.addSkill).toHaveBeenCalledWith(expect.objectContaining({ expectedSourceFingerprint: 'reviewed-package', sourceKey: data.skills[0].key })); + expect(mocks.validateSkill).toHaveBeenCalledTimes(1); + expect(container.querySelector('[role="dialog"]')?.textContent).toContain(mode === 'single' ? 'content.skillStale' : 'content.batchState.stale'); + }); + + it('does not silently downgrade when a host advertising reviewed imports omits the preview', async () => { + mocks.skillImportVersion = 3; + await render(); await click('content.prepareImport', 'skill'); + expect(container.textContent).toContain('content.previewFailed'); + expect(mocks.addSkill).not.toHaveBeenCalled(); + }); + it('reviews a full agent batch once and copies only that agent after confirmation', async () => { mocks.skillImportVersion = 1; await render(); @@ -195,6 +236,15 @@ describe('external agent content and explicit import boundary', () => { }); it('requires a second explicit confirmation and sends only the selected MCP candidate', async () => { + mocks.applyMcp.mockImplementationOnce(async () => { + mocks.planMcp.mockResolvedValue({ + ...data.plan, + items: data.plan.items.map((item) => item.candidateId === 'codex' + ? { ...item, disposition: 'already_imported' } + : item), + }); + return { outcome: { status: 'applied' } }; + }); await render(); await click('content.prepareImport', 'mcp'); expect(mocks.applyMcp).not.toHaveBeenCalled(); await click('content.confirm'); @@ -214,6 +264,69 @@ describe('external agent content and explicit import boundary', () => { expect(mocks.applyMcp).toHaveBeenCalledTimes(1); }); + it.each([ + ['single', 'notification'], ['single', 'refresh'], ['batch', 'notification'], ['batch', 'refresh'], + ])('reconciles a %s MCP import deleted in native management through %s', async (mode, trigger) => { + mocks.skillImportVersion = 3; + mocks.planMcp.mockImplementation(async () => structuredClone(data.plan)); + mocks.applyMcp.mockImplementation(async () => { + data.plan.items[0].disposition = 'already_imported'; + return { outcome: { status: 'applied' } }; + }); + await render(); + if (mode === 'single') await click('content.prepareImport', 'mcp'); + else await click('content.importAll'); + await click('content.confirm'); + if (mode === 'batch') await click('content.close'); + await expand('mcp'); + const row = () => container.querySelector('[data-import-kind="mcp"]')!; + expect(row().getAttribute('data-import-state')).toBe('imported'); + const calls = mocks.planMcp.mock.calls.length; + const generation = data.snapshot.generation; + data.plan.items[0].disposition = 'eligible'; + if (trigger === 'notification') { + await act(async () => globalEventBus.emit(MCP_CONFIG_CHANGED, { surfaceId: 'local' })); + } else { + const refresh = container.querySelector('button[aria-label="content.refresh"]')!; + await act(async () => refresh.click()); + } + expect(data.snapshot.generation).toBe(generation); + expect(mocks.planMcp.mock.calls.length).toBeGreaterThan(calls); + expect(row().getAttribute('data-import-state')).toBe('ready'); + expect(row().textContent).toContain('content.prepareImport'); + expect(row().textContent).not.toContain('content.undo'); + expect(row().textContent).not.toContain('content.manageCopy'); + expect(mocks.saveMcp).not.toHaveBeenCalled(); + }); + + it('ignores changes on another host and old MCP plan responses after deletion', async () => { + data.plan.items[0].disposition = 'already_imported'; + mocks.planMcp.mockImplementation(async () => structuredClone(data.plan)); + await render(); await expand('mcp'); + const calls = mocks.planMcp.mock.calls.length; + await act(async () => globalEventBus.emit(MCP_CONFIG_CHANGED, { surfaceId: 'another-device' })); + expect(mocks.planMcp).toHaveBeenCalledTimes(calls); + const stale = structuredClone(data.plan); + let finish!: (plan: typeof data.plan) => void; + mocks.planMcp.mockImplementationOnce(() => new Promise(resolve => { finish = resolve; })); + await act(async () => globalEventBus.emit(MCP_CONFIG_CHANGED, { surfaceId: 'local' })); + data.plan.items[0].disposition = 'eligible'; + await act(async () => globalEventBus.emit(MCP_CONFIG_CHANGED, { surfaceId: 'local' })); + await act(async () => finish(stale)); + expect(container.querySelector('[data-import-kind="mcp"]')?.getAttribute('data-import-state')).toBe('ready'); + }); + + it('does not offer another import when the native MCP read-back fails after saving', async () => { + await render(); await click('content.prepareImport', 'mcp'); + mocks.planMcp.mockRejectedValue(new Error('Native configuration unavailable')); + await click('content.confirm'); + const row = container.querySelector('[data-import-kind="mcp"]')!; + expect(mocks.applyMcp).toHaveBeenCalledOnce(); + expect(row.getAttribute('data-import-state')).toBe('unavailable'); + expect(row.textContent).not.toContain('content.prepareImport'); + expect(row.textContent).not.toContain('content.undo'); + }); + it('copies a selected Skill only after validation, target review and confirmation', async () => { await render(); await click('content.prepareImport', 'skill'); expect(mocks.validateSkill).toHaveBeenCalledWith('/codex/skills/demo'); expect(mocks.addSkill).not.toHaveBeenCalled(); @@ -382,9 +495,12 @@ describe('external agent content and explicit import boundary', () => { it.each(['remote', 'peer'] as const)('keeps %s source previews but gates unsupported imports without a local fallback', async (surface) => { mocks[surface] = true; + mocks.skillImportVersion = 3; await render(); await expand('skill'); expect(container.textContent).toContain('codex-Skill'); expect(container.textContent).not.toContain('content.prepareImport'); + expect(container.textContent).not.toContain('content.manageNative'); + expect(mocks.validateSkill).not.toHaveBeenCalled(); expect(mocks.planMcp).not.toHaveBeenCalled(); expect(mocks.getHooks).not.toHaveBeenCalled(); expect(mocks.getHookCatalog).toHaveBeenCalledWith('/project', false); expect(mocks.addSkill).not.toHaveBeenCalled(); expect(mocks.applyMcp).not.toHaveBeenCalled(); diff --git a/src/web-ui/src/app/scenes/ecosystem-compatibility/ExternalAgentContent.tsx b/src/web-ui/src/app/scenes/ecosystem-compatibility/ExternalAgentContent.tsx index 6b52911d29..bdb6f04aad 100644 --- a/src/web-ui/src/app/scenes/ecosystem-compatibility/ExternalAgentContent.tsx +++ b/src/web-ui/src/app/scenes/ecosystem-compatibility/ExternalAgentContent.tsx @@ -6,16 +6,22 @@ import { presentEcosystemContent } from './ecosystemContentPresentation'; import { suggestSkillImportName, importErrorMessage } from './ecosystemSkillImport'; import { applyEcosystemBatchUndo, type BatchUndoEntry, type BatchUndoResult } from './ecosystemBatchUndo'; import { Bot, CircleUserRound, Package, PawPrint, Server, Webhook, Wrench } from 'lucide-react'; +import { useSceneStore } from '@/app/stores/sceneStore'; +import { useSettingsStore } from '@/app/scenes/settings/settingsStore'; +import { useSkillsSceneStore } from '@/app/scenes/skills/skillsSceneStore'; import { useI18n } from '@/infrastructure/i18n'; import { useNotification } from '@/shared/notification-system'; import { useCurrentWorkspace } from '@/infrastructure/contexts/WorkspaceContext'; import { usePeerDeviceModeOptional } from '@/infrastructure/peer-device/peerDeviceContextState'; +import { globalEventBus } from '@/infrastructure/event-bus'; +import { MCP_CONFIG_CHANGED, type MCPConfigChanged } from '@/infrastructure/mcp/configEvents'; +import { getActiveSurfaceScope } from '@/infrastructure/peer-device/deviceSurface'; import { isTauriRuntime } from '@/infrastructure/runtime'; import { WorkspaceKind } from '@/shared/types'; import { configAPI } from '@/infrastructure/api/service-api/ConfigAPI'; import { externalSourcesAPI, type ExternalMcpImportPlanV1, type ExternalSourceCatalogSnapshot } from '@/infrastructure/api/service-api/ExternalSourcesAPI'; import { externalHooksAPI, type ExternalHookImportPlan, type ExternalHookImportSnapshot, type ExternalHookSource } from '@/infrastructure/api/service-api/ExternalHooksAPI'; -import type { SkillInfo, SkillLevel, SkillScanDiagnostic } from '@/infrastructure/config/types'; +import type { SkillInfo, SkillLevel, SkillScanDiagnostic, SkillImportPreview } from '@/infrastructure/config/types'; import { getSkillSourceId, isOpenBitFunManagedSkill } from '@/infrastructure/config/skillSourcePresentation'; import { buildEcosystemImportItems, catalogDiscoveryState, type EcosystemImportItem, type EcosystemImportItemKind, type EcosystemProductRuntime } from './ecosystemCompatibilityModel'; import { applyImportUndo, matchesSkillReceipt, prepareHookUndo, prepareMcpUndo, readSkillImportReceipt, rememberSkillImport, type ImportUndoReview } from './ecosystemImportUndo'; @@ -42,7 +48,7 @@ const CONTENT_ICONS: Record = { type Review = | { kind: 'mcp'; item: ContentItem; plan: ExternalMcpImportPlanV1 } - | { kind: 'skill'; item: ContentItem; skill: SkillInfo; level: SkillLevel; targetName?: string } + | { kind: 'skill'; item: ContentItem; skill: SkillInfo; level: SkillLevel; targetName?: string; preview?: SkillImportPreview } | { kind: 'hook'; item: ContentItem; plan: ExternalHookImportPlan }; interface Props { @@ -62,6 +68,17 @@ export default function ExternalAgentContent({ runtime, snapshot, catalogFailed, const peer = usePeerDeviceModeOptional(); const localImportSupported = isTauriRuntime() && !peer?.peerMode.active && workspace?.workspaceKind !== WorkspaceKind.Remote; + const openNativeManagement = (kind: EcosystemImportItemKind) => { + if (!localImportSupported) return; + if (kind === 'skill') { + useSkillsSceneStore.getState().openNativeSkills(); + useSceneStore.getState().openScene('skills'); + } else if (kind === 'mcp' || kind === 'hook') { + useSettingsStore.getState().openDestination(kind === 'mcp' + ? { pageId: 'tools.mcp' } : { pageId: 'tools.automation', viewId: 'hooks' }); + useSceneStore.getState().openScene('settings'); + } + }; const [skills, setSkills] = useState([]); const [skillImportVersion, setSkillImportVersion] = useState(0); const [skillDiagnostics, setSkillDiagnostics] = useState([]); @@ -175,6 +192,14 @@ export default function ExternalAgentContent({ runtime, snapshot, catalogFailed, return () => { mcpPlanSequence.current += 1; }; }, [refreshMcpPlan, snapshot?.generation]); + useEffect(() => { + if (!localImportSupported || !hasMcp) return; + const scope = getActiveSurfaceScope(); + return globalEventBus.on(MCP_CONFIG_CHANGED, ({ surfaceId }) => { + if (scope.isCurrent() && surfaceId === scope.surfaceId) void refreshMcpPlan(); + }); + }, [hasMcp, localImportSupported, refreshMcpPlan]); + const importedHook = (item: ContentItem) => hooks?.imports.some((entry) => ( entry.source.key.providerId === item.hookSource?.key.providerId && entry.source.key.sourceId === item.hookSource?.key.sourceId @@ -204,7 +229,8 @@ export default function ExternalAgentContent({ runtime, snapshot, catalogFailed, } else discoveryState = catalogDiscoveryState(snapshot, runtime.spec.ecosystemId, item.kind); return presentEcosystemContent({ item, discoveryState, catalogFailed, localImportSupported, - imported: completed.has(item.id) || !!importedHook(item) || !!importedSkill(item), + // MCP copy existence comes from the current native import plan, not a past success. + imported: (item.kind !== 'mcp' && completed.has(item.id)) || !!importedHook(item) || !!importedSkill(item), skillImportSupported: skillImportVersion >= 1 || !item.skill?.entryFile || item.skill.entryFile === 'SKILL.md', hookImportSupported: !!item.hookSource && ['claude-code', 'codex'].includes(item.hookSource.ecosystemId), planLoading, @@ -224,6 +250,12 @@ export default function ExternalAgentContent({ runtime, snapshot, catalogFailed, return item.description || t('content.externalOnly'); }; + async function previewSkill(skill: SkillInfo): Promise { + const result = await configAPI.validateSkillPath(skill.path, { sourceKey: skill.key, workspacePath: workspacePath || undefined }); + if (!result.valid || !result.importPreview?.fingerprint) throw new Error('Skill import preview is unavailable'); + return result.importPreview; + } + async function prepareBatch(group?: EcosystemImportItemKind, selectedOnly = false) { if (busy || !localImportSupported) return; setBusy(true); @@ -238,11 +270,14 @@ export default function ExternalAgentContent({ runtime, snapshot, catalogFailed, if (!alive.current) return; if (!presentation(item).canImport) continue; if (item.skill && skillImportVersion >= 1) { - const level = item.skill.level === 'project' && workspacePath ? 'project' : 'user'; + const preview = skillImportVersion >= 3 ? await previewSkill(item.skill).catch(() => null) : undefined; + if (preview === null) continue; + const skill = preview ? { ...item.skill, name: preview.name, description: preview.description } : item.skill; + const level = skill.level === 'project' && workspacePath ? 'project' : 'user'; const reserved = entries.filter((entry) => entry.kind === 'skill' && entry.level === level) .flatMap((entry) => entry.kind === 'skill' ? [entry.targetName ?? entry.skill.name, entry.targetName ?? entry.skill.dirName] : []); - const targetName = skillImportVersion >= 2 ? suggestSkillImportName(item.skill, level, skills, reserved) : undefined; - entries.push({ id: item.id, name: item.name, kind: 'skill', skill: item.skill, level, targetName }); + const targetName = skillImportVersion >= 2 ? suggestSkillImportName(skill, level, skills, reserved) : undefined; + entries.push({ id: item.id, name: skill.name, kind: 'skill', skill, level, targetName, ...(preview ? { preview } : {}) }); } else if (item.kind === 'mcp' && item.candidateId && freshMcpPlan?.items.some((candidate) => candidate.candidateId === item.candidateId && ['eligible', 'automatic_rename'].includes(candidate.disposition))) { @@ -272,9 +307,12 @@ export default function ExternalAgentContent({ runtime, snapshot, catalogFailed, await applyEcosystemBatch(batch, workspacePath || undefined, (result) => { if (!alive.current) return; setBatchResults((current) => [...(current ?? []), result]); - if (result.status === 'imported') setCompleted((current) => new Set([...current, result.id])); + if (result.status === 'imported' && batch.find((entry) => entry.id === result.id)?.kind !== 'mcp') { + setCompleted((current) => new Set([...current, result.id])); + } }); if (alive.current) { + void refreshMcpPlan(); void loadSupplemental(true); void onRefresh().catch(() => { if (alive.current) setNotice(t('content.refreshAfterImportFailed')); }); } @@ -289,13 +327,15 @@ export default function ExternalAgentContent({ runtime, snapshot, catalogFailed, setBusy(true); try { if (item.skill) { - const validation = skillImportVersion >= 1 && item.skill.entryFile && item.skill.entryFile !== 'SKILL.md' + const preview = skillImportVersion >= 3 ? await previewSkill(item.skill) : undefined; + const validation = preview || (skillImportVersion >= 1 && item.skill.entryFile && item.skill.entryFile !== 'SKILL.md') ? { valid: true } : await configAPI.validateSkillPath(item.skill.path); if (!alive.current || sequence !== reviewSequence.current) return; if (!validation.valid) { setNotice(t('content.validationFailed')); return; } - const level = item.skill.level === 'project' && workspacePath ? 'project' : 'user'; - setReview({ kind: 'skill', item, skill: item.skill, level, - targetName: skillImportVersion >= 2 ? suggestSkillImportName(item.skill, level, skills) : undefined }); + const skill = preview ? { ...item.skill, name: preview.name, description: preview.description } : item.skill; + const level = skill.level === 'project' && workspacePath ? 'project' : 'user'; + setReview({ kind: 'skill', item: { ...item, name: skill.name, description: skill.description }, skill, level, preview, + targetName: skillImportVersion >= 2 ? suggestSkillImportName(skill, level, skills) : undefined }); } else if (item.hookSource) { const next = await externalHooksAPI.planImport(workspacePath || undefined, item.hookSource.key); if (!alive.current || sequence !== reviewSequence.current) return; @@ -322,7 +362,8 @@ export default function ExternalAgentContent({ runtime, snapshot, catalogFailed, await configAPI.addSkill({ sourcePath: captured.skill.path, level: captured.level, workspacePath: workspacePath || undefined, ...(skillImportVersion >= 2 && captured.targetName ? { targetName: captured.targetName } : {}), - ...(skillImportVersion >= 1 ? { sourceKey: captured.skill.key } : {}) }); + ...(skillImportVersion >= 1 ? { sourceKey: captured.skill.key } : {}), + ...(captured.preview ? { expectedSourceFingerprint: captured.preview.fingerprint } : {}) }); // The copy has committed. A failed read-back must not invite a duplicate import. const report = await configAPI.getSkillScanReport({ workspacePath: workspacePath || undefined, forceRefresh: true }).catch(() => null); if (!alive.current) return; @@ -355,7 +396,8 @@ export default function ExternalAgentContent({ runtime, snapshot, catalogFailed, setHooks(result.outcome.snapshot); } if (!alive.current) return; - setCompleted((current) => new Set([...current, captured.item.id])); + if (captured.kind === 'mcp') void refreshMcpPlan(); + else setCompleted((current) => new Set([...current, captured.item.id])); setReview(null); setDetail(null); notification.success(t('content.importSuccess', { name: captured.item.name }), { duration: 3200 }); @@ -364,7 +406,7 @@ export default function ExternalAgentContent({ runtime, snapshot, catalogFailed, } catch (error) { const reason = importErrorMessage(error); const conflict = reason.includes('Skill target already exists with different content') || reason.includes('Skill target belongs to a different import'); - if (alive.current) setNotice(`${t('content.importFailed')} ${conflict ? t('content.skillNameConflict') : reason}`); + if (alive.current) setNotice(reason.includes('skill_import_stale:') ? t('content.skillStale') : `${t('content.importFailed')} ${conflict ? t('content.skillNameConflict') : reason}`); } finally { if (alive.current) setBusy(false); } @@ -485,7 +527,7 @@ export default function ExternalAgentContent({ runtime, snapshot, catalogFailed,
{localImportSupported ? <> : null} - } aria-label={t('content.refresh')} title={t('content.refresh')} disabled={busy || loading} onClick={() => { setNotice(null); void loadSupplemental(true); void onRefresh().catch(() => { if (alive.current) setNotice(t('content.refreshAfterImportFailed')); }); }} /> + } aria-label={t('content.refresh')} title={t('content.refresh')} disabled={busy || loading} onClick={() => { setNotice(null); void refreshMcpPlan(); void loadSupplemental(true); void onRefresh().catch(() => { if (alive.current) setNotice(t('content.refreshAfterImportFailed')); }); }} />
{notice && !review && !undo && !batch && !batchUndo ?

{notice}

: null} @@ -556,6 +598,7 @@ export default function ExternalAgentContent({ runtime, snapshot, catalogFailed, {item.discovered ? : null} {importable ? : null} + {state === 'imported' && localImportSupported && ['skill', 'mcp', 'hook'].includes(item.kind) ? : null} {state === 'imported' && localImportSupported ? : null} {state === 'imported' && item.skill && skillImportVersion >= 1 && !importedSkill(item)?.importId ? : null} @@ -611,6 +654,7 @@ export default function ExternalAgentContent({ runtime, snapshot, catalogFailed, {entry.kind === 'skill' && entry.targetName ?

{t('content.renameNotice', { name: entry.targetName })}

: null} {entry.kind === 'skill' ?

{entry.skill.path}

: entry.kind === 'hook' ?

{entry.plan.source.locationHint}

: null} {entry.kind === 'hook' ? <>

{t('content.hookWarning')}

{entry.plan.handlers.map((handler) =>

{handler.event}{handler.matcher ? ` · ${handler.matcher}` : ''}

{handler.command}
{handler.commandWindows ? 
{handler.commandWindows}
: null}{handler.dependencies.map((dependency) =>

{dependency.kind === 'managed' ? dependency.relativePath : dependency.location}

)}
)}{entry.plan.skipped.map((entry) =>

{t('content.skipped', { reason: entry.reasonCode, count: formatNumber(entry.count) })}

)} : null} + {entry.kind === 'skill' && entry.preview ?

{t('content.reviewedPackage', { count: formatNumber(entry.preview.fileCount) })}

: null} {entry.kind === 'mcp' ?

{t('content.mcpTarget', { name: entry.plan.items.find((item) => item.candidateId === entry.candidateId)?.proposedNativeId ?? entry.name })}

: null} } ; @@ -650,6 +694,7 @@ export default function ExternalAgentContent({ runtime, snapshot, catalogFailed,

{t('content.copyWarning')}

{review.kind === 'mcp' ?

{t('content.mcpTarget', { name: reviewMcp?.proposedNativeId ?? review.item.name })}

: null} {review.kind === 'skill' ? <> + {review.preview ?

{t('content.reviewedPackage', { count: formatNumber(review.preview.fileCount) })}

: null}
{ setNotice(null); setReview({ ...review, targetName: event.target.value }); }} />
: null} {review.targetName ?

{t('content.renameNotice', { name: review.targetName })}

: null} diff --git a/src/web-ui/src/app/scenes/ecosystem-compatibility/ecosystemBatchImport.test.ts b/src/web-ui/src/app/scenes/ecosystem-compatibility/ecosystemBatchImport.test.ts index ae4c35c7df..8f6e45ace1 100644 --- a/src/web-ui/src/app/scenes/ecosystem-compatibility/ecosystemBatchImport.test.ts +++ b/src/web-ui/src/app/scenes/ecosystem-compatibility/ecosystemBatchImport.test.ts @@ -42,3 +42,16 @@ it('refreshes a Hook target revision but refuses executable content changed sinc expect(mocks.hook.mock.calls[0][1].planFingerprint).toBe('new-target-revision'); expect(results.map(({ status }) => status)).toEqual(['imported', 'stale']); }); + +it('retains each reviewed Skill digest and continues after a stale package', async () => { + mocks.add.mockRejectedValueOnce(new Error('skill_import_stale: changed')).mockResolvedValueOnce('ok'); + const entries: BatchImportEntry[] = ['changed', 'unchanged'].map((id) => ({ + id, name: id, kind: 'skill', skill: { key: id, path: `/source/${id}` } as SkillInfo, + level: 'user', targetName: `${id}-alias`, preview: { fingerprint: `digest-${id}`, fileCount: 2, name: id, description: '' }, + })); + const results: BatchImportResult[] = []; + await applyEcosystemBatch(entries, undefined, (result) => results.push(result)); + expect(results.map(({ status }) => status)).toEqual(['stale', 'imported']); + expect(mocks.add.mock.calls.map(([request]) => request.expectedSourceFingerprint)).toEqual(['digest-changed', 'digest-unchanged']); + expect(mocks.add.mock.calls[1][0].targetName).toBe('unchanged-alias'); +}); diff --git a/src/web-ui/src/app/scenes/ecosystem-compatibility/ecosystemBatchImport.ts b/src/web-ui/src/app/scenes/ecosystem-compatibility/ecosystemBatchImport.ts index 3e98a0ea0a..ad3f85245b 100644 --- a/src/web-ui/src/app/scenes/ecosystem-compatibility/ecosystemBatchImport.ts +++ b/src/web-ui/src/app/scenes/ecosystem-compatibility/ecosystemBatchImport.ts @@ -1,12 +1,12 @@ import { configAPI } from '@/infrastructure/api/service-api/ConfigAPI'; import { externalSourcesAPI, type ExternalMcpImportPlanV1 } from '@/infrastructure/api/service-api/ExternalSourcesAPI'; import { externalHooksAPI, type ExternalHookImportPlan } from '@/infrastructure/api/service-api/ExternalHooksAPI'; -import type { SkillInfo, SkillLevel } from '@/infrastructure/config/types'; +import type { SkillInfo, SkillLevel, SkillImportPreview } from '@/infrastructure/config/types'; import { importErrorMessage } from './ecosystemSkillImport'; import { getActiveSurfaceScope, isLocalSurface } from '@/infrastructure/peer-device/deviceSurface'; export type BatchImportEntry = { id: string; name: string } & ( - | { kind: 'skill'; skill: SkillInfo; level: SkillLevel; targetName?: string } + | { kind: 'skill'; skill: SkillInfo; level: SkillLevel; targetName?: string; preview?: SkillImportPreview } | { kind: 'mcp'; candidateId: string; plan: ExternalMcpImportPlanV1 } | { kind: 'hook'; plan: ExternalHookImportPlan } ); @@ -40,7 +40,8 @@ export async function applyEcosystemBatch( scope.assertCurrent('apply reviewed external import'); if (entry.kind === 'skill') { await configAPI.addSkill({ sourcePath: entry.skill.path, sourceKey: entry.skill.key, - level: entry.level, workspacePath, ...(entry.targetName ? { targetName: entry.targetName } : {}) }); + level: entry.level, workspacePath, ...(entry.targetName ? { targetName: entry.targetName } : {}), + ...(entry.preview ? { expectedSourceFingerprint: entry.preview.fingerprint } : {}) }); status = 'imported'; } else { // Earlier imports change the target revision. Refresh it while requiring the @@ -56,7 +57,10 @@ export async function applyEcosystemBatch( status = response.outcome.kind === 'stale' ? 'stale' : 'imported'; } } - } catch (cause) { error = importErrorMessage(cause); } + } catch (cause) { + error = importErrorMessage(cause); + if (error.includes('skill_import_stale:')) status = 'stale'; + } onResult({ id: entry.id, name: entry.name, status, ...(error ? { error } : {}) }); } } diff --git a/src/web-ui/src/app/scenes/skills/SkillsScene.tsx b/src/web-ui/src/app/scenes/skills/SkillsScene.tsx index 6471c0e6d0..97e8aa65a9 100644 --- a/src/web-ui/src/app/scenes/skills/SkillsScene.tsx +++ b/src/web-ui/src/app/scenes/skills/SkillsScene.tsx @@ -118,6 +118,7 @@ const SkillsScene: React.FC = () => { const remoteConnectionActive = peerDevice?.peerMode.active === true; const desktopConfigAvailable = isTauriRuntime() && !remoteConnectionActive; const { + nativeNavigationRequest, searchDraft, marketQuery, installedView, @@ -140,6 +141,13 @@ const SkillsScene: React.FC = () => { | null >(null); + useEffect(() => { + if (!nativeNavigationRequest) return; + setActiveTab('installed'); + setInstalledSearch(''); + setSelectedDetail(null); + }, [nativeNavigationRequest]); + const installed = useInstalledSkills({ searchQuery: installedSearch, activeFilter: installedView === 'groups' ? 'all' : installedView, diff --git a/src/web-ui/src/app/scenes/skills/skillsSceneStore.ts b/src/web-ui/src/app/scenes/skills/skillsSceneStore.ts index a5ef57ad2d..cc4bdad669 100644 --- a/src/web-ui/src/app/scenes/skills/skillsSceneStore.ts +++ b/src/web-ui/src/app/scenes/skills/skillsSceneStore.ts @@ -4,6 +4,8 @@ export type InstalledFilter = 'all' | 'builtin' | 'user' | 'project' | `source:$ export type SkillsView = InstalledFilter | 'groups'; interface SkillsSceneState { + nativeNavigationRequest: number; + openNativeSkills: () => void; searchDraft: string; marketQuery: string; installedView: SkillsView; @@ -18,6 +20,8 @@ interface SkillsSceneState { } export const useSkillsSceneStore = create((set) => ({ + nativeNavigationRequest: 0, + openNativeSkills: () => set((state) => ({ nativeNavigationRequest: state.nativeNavigationRequest + 1, installedView: 'all', hideDuplicates: false })), searchDraft: '', marketQuery: '', installedView: 'all', diff --git a/src/web-ui/src/infrastructure/api/service-api/ConfigAPI.test.ts b/src/web-ui/src/infrastructure/api/service-api/ConfigAPI.test.ts index 77cd57403b..eba300683c 100644 --- a/src/web-ui/src/infrastructure/api/service-api/ConfigAPI.test.ts +++ b/src/web-ui/src/infrastructure/api/service-api/ConfigAPI.test.ts @@ -191,3 +191,24 @@ describe('skill scan response compatibility', () => { await expect(new ConfigAPI().getSkillScanReport()).rejects.toThrow(); }); }); + +describe('reviewed Skill import wire compatibility', () => { + it('preserves legacy validation and sends the reviewed digest only when supplied', async () => { + invokeMock.mockReset(); + invokeMock.mockResolvedValue({ valid: true }); + const config = new ConfigAPI(); + await config.validateSkillPath('/source'); + expect(invokeMock).toHaveBeenLastCalledWith('validate_skill_path', { path: '/source' }); + await config.validateSkillPath('/source', { sourceKey: 'external-key', workspacePath: '/workspace' }); + expect(invokeMock).toHaveBeenLastCalledWith('validate_skill_path', { path: '/source', sourceKey: 'external-key', workspacePath: '/workspace' }); + await config.addSkill({ sourcePath: '/source', level: 'user', sourceKey: 'external-key', expectedSourceFingerprint: 'reviewed', targetName: 'alias' }); + expect(invokeMock).toHaveBeenLastCalledWith('add_skill', { sourcePath: '/source', level: 'user', workspacePath: undefined, sourceKey: 'external-key', expectedSourceFingerprint: 'reviewed', targetName: 'alias' }); + await config.addSkill({ sourcePath: '/source', level: 'user' }); + expect(invokeMock).toHaveBeenLastCalledWith('add_skill', { sourcePath: '/source', level: 'user', workspacePath: undefined }); + }); + + it('retains the advertised reviewed-import version', async () => { + invokeMock.mockResolvedValueOnce({ skills: [], diagnostics: [], importOperationsVersion: 3 }); + expect((await new ConfigAPI().getSkillScanReport()).importOperationsVersion).toBe(3); + }); +}); diff --git a/src/web-ui/src/infrastructure/api/service-api/ConfigAPI.ts b/src/web-ui/src/infrastructure/api/service-api/ConfigAPI.ts index 0ae0479832..89518942c5 100644 --- a/src/web-ui/src/infrastructure/api/service-api/ConfigAPI.ts +++ b/src/web-ui/src/infrastructure/api/service-api/ConfigAPI.ts @@ -68,6 +68,7 @@ export interface ResetModeSkillSelectionParams { } export interface AddSkillParams { + expectedSourceFingerprint?: string; targetName?: string; sourceKey?: string; sourcePath: string; @@ -496,9 +497,9 @@ export class ConfigAPI { } - async validateSkillPath(path: string): Promise { + async validateSkillPath(path: string, source?: { sourceKey: string; workspacePath?: string }): Promise { try { - return await api.invoke('validate_skill_path', { path }); + return await api.invoke('validate_skill_path', { path, ...source }); } catch (error) { throw createTauriCommandError('validate_skill_path', error, { path }); } @@ -506,6 +507,7 @@ export class ConfigAPI { async addSkill({ + expectedSourceFingerprint, targetName, sourceKey, sourcePath, @@ -513,7 +515,7 @@ export class ConfigAPI { workspacePath, }: AddSkillParams): Promise { try { - return await api.invoke('add_skill', { sourcePath, level, workspacePath, ...(sourceKey ? { sourceKey } : {}), ...(targetName ? { targetName } : {}) }); + return await api.invoke('add_skill', { sourcePath, level, workspacePath, ...(sourceKey ? { sourceKey } : {}), ...(targetName ? { targetName } : {}), ...(expectedSourceFingerprint !== undefined ? { expectedSourceFingerprint } : {}) }); } catch (error) { throw createTauriCommandError('add_skill', error, { sourcePath, level, workspacePath }); } diff --git a/src/web-ui/src/infrastructure/api/service-api/MCPAPI.test.ts b/src/web-ui/src/infrastructure/api/service-api/MCPAPI.test.ts index af8dba8a2a..f2f77ab1d1 100644 --- a/src/web-ui/src/infrastructure/api/service-api/MCPAPI.test.ts +++ b/src/web-ui/src/infrastructure/api/service-api/MCPAPI.test.ts @@ -55,6 +55,29 @@ describe('MCP JSON save acknowledgement', () => { } finally { unsubscribe(); } }); + it('publishes native MCP deletions only after acknowledged writes on the same host', async () => { + const changed = vi.fn(); + const unsubscribe = globalEventBus.on(MCP_CONFIG_CHANGED, changed); + try { + let finish!: () => void; + invokeMock.mockImplementationOnce(() => new Promise(resolve => { finish = resolve; })); + const deleting = MCPAPI.deleteServer({ serverId: 'imported-copy' }); + expect(changed).not.toHaveBeenCalled(); + finish(); + await deleting; + expect(invokeMock).toHaveBeenCalledWith('delete_mcp_server', { request: { serverId: 'imported-copy' } }); + expect(changed).toHaveBeenCalledExactlyOnceWith({ surfaceId: 'local' }); + changed.mockClear(); + invokeMock.mockRejectedValueOnce(new Error('Delete failed')); + await expect(MCPAPI.deleteServer({ serverId: 'kept-copy' })).rejects.toThrow('Delete failed'); + expect(changed).not.toHaveBeenCalled(); + invokeMock.mockResolvedValueOnce(undefined); + scopeMock.assertCurrent.mockImplementationOnce(() => { throw new Error('Surface changed'); }); + await expect(MCPAPI.deleteServer({ serverId: 'previous-device-copy' })).rejects.toThrow('Surface changed'); + expect(changed).not.toHaveBeenCalled(); + } finally { unsubscribe(); } + }); + it('accepts the existing void success response', async () => { invokeMock.mockResolvedValue(undefined); await expect(MCPAPI.saveMCPJsonConfig('{}', 'revision')).resolves.toEqual({ runtimeApplied: true }); diff --git a/src/web-ui/src/infrastructure/api/service-api/MCPAPI.ts b/src/web-ui/src/infrastructure/api/service-api/MCPAPI.ts index ed00d794ce..31e19f00f7 100644 --- a/src/web-ui/src/infrastructure/api/service-api/MCPAPI.ts +++ b/src/web-ui/src/infrastructure/api/service-api/MCPAPI.ts @@ -444,7 +444,9 @@ export class MCPAPI { } static async deleteServer(request: DeleteMCPServerRequest): Promise { - return api.invoke('delete_mcp_server', { request }); + const scope = getActiveSurfaceScope(); + await api.invoke('delete_mcp_server', { request }); + notifyMcpConfigChanged(scope); } static async startRemoteOAuth( diff --git a/src/web-ui/src/infrastructure/config/types/index.ts b/src/web-ui/src/infrastructure/config/types/index.ts index 9f8e553751..e20b40413e 100644 --- a/src/web-ui/src/infrastructure/config/types/index.ts +++ b/src/web-ui/src/infrastructure/config/types/index.ts @@ -490,7 +490,15 @@ export interface SkillMarketDownloadResult { output: string; } +export interface SkillImportPreview { + fingerprint: string; + fileCount: number; + name: string; + description: string; +} + export interface SkillValidationResult { + importPreview?: SkillImportPreview; valid: boolean; name?: string; description?: string; diff --git a/src/web-ui/src/locales/en-US/scenes/ecosystem-compatibility.json b/src/web-ui/src/locales/en-US/scenes/ecosystem-compatibility.json index 41a7ffea3c..109302d988 100644 --- a/src/web-ui/src/locales/en-US/scenes/ecosystem-compatibility.json +++ b/src/web-ui/src/locales/en-US/scenes/ecosystem-compatibility.json @@ -181,6 +181,9 @@ "manageAction": "Discovery settings" }, "content": { + "skillStale": "The Skill package changed. Cancel this confirmation and prepare a new preview before importing.", + "reviewedPackage": "Reviewed package: {{count}} files. If the source changes, review it again before importing.", + "manageCopy": "Manage copy", "skillNameConflict": "A Skill with this name already has different content or provenance. Change the import name above and retry; the existing copy will not be overwritten.", "batchUndoWarning": "Remove these {{count}} OpenBitFun copies, including edits made to them. External sources are preserved and can be imported again.", "batchUndoSummary": "{{removed}} removed · {{pending}} runtime updates pending · {{failed}} failed", diff --git a/src/web-ui/src/locales/zh-CN/scenes/ecosystem-compatibility.json b/src/web-ui/src/locales/zh-CN/scenes/ecosystem-compatibility.json index 5effa62801..0e756d1ea0 100644 --- a/src/web-ui/src/locales/zh-CN/scenes/ecosystem-compatibility.json +++ b/src/web-ui/src/locales/zh-CN/scenes/ecosystem-compatibility.json @@ -181,6 +181,9 @@ "manageAction": "管理发现设置" }, "content": { + "skillStale": "Skill 包内容已变化。请取消本次确认，重新预览后再导入。", + "reviewedPackage": "已核对包内容，共 {{count}} 个文件。来源变化后需要重新预览才能导入。", + "manageCopy": "管理副本", "skillNameConflict": "目标已有同名 Skill，且内容或来源不同。请修改上方的导入名称后重试；已有副本不会被覆盖。", "batchUndoWarning": "将移除以下 {{count}} 个 OpenBitFun 副本，包括对副本所做的修改。外部来源保留，之后可以重新导入。", "batchUndoSummary": "已撤销 {{removed}} 项 · 运行时待同步 {{pending}} 项 · 失败 {{failed}} 项", diff --git a/src/web-ui/src/locales/zh-TW/scenes/ecosystem-compatibility.json b/src/web-ui/src/locales/zh-TW/scenes/ecosystem-compatibility.json index 6bf27e5aec..25b36e9759 100644 --- a/src/web-ui/src/locales/zh-TW/scenes/ecosystem-compatibility.json +++ b/src/web-ui/src/locales/zh-TW/scenes/ecosystem-compatibility.json @@ -181,6 +181,9 @@ "manageAction": "管理探索設定" }, "content": { + "skillStale": "Skill 套件內容已變更。請取消本次確認，重新預覽後再匯入。", + "reviewedPackage": "已核對套件內容，共 {{count}} 個檔案。來源變更後需要重新預覽才能匯入。", + "manageCopy": "管理副本", "skillNameConflict": "目標已有同名 Skill，且內容或來源不同。請修改上方的匯入名稱後重試；既有副本不會被覆寫。", "batchUndoWarning": "將移除以下 {{count}} 個 OpenBitFun 副本，包括對副本所做的修改。外部來源保留，之後可以重新匯入。", "batchUndoSummary": "已復原 {{removed}} 項 · 執行階段待同步 {{pending}} 項 · 失敗 {{failed}} 項",