diff --git a/src/crates/assembly/core/AGENTS.md b/src/crates/assembly/core/AGENTS.md index d8a895e583..9d3d45130c 100644 --- a/src/crates/assembly/core/AGENTS.md +++ b/src/crates/assembly/core/AGENTS.md @@ -232,6 +232,12 @@ cargo test -p openbitfun-core --no-default-features --features agent-runtime,git cargo test -p openbitfun-core --no-default-features --features agent-runtime,remote-workspace,git --lib service::snapshot:: ``` +Skill discovery, installation provenance, and local/remote registry regressions: + +```bash +cargo test --locked -p openbitfun-core --no-default-features --features agent-runtime,git --lib agentic::tools::implementations::skills:: +``` + Detached Dispatch controller, target query compatibility, and managed-baseline checks: ```bash 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 0a3c5deb58..b673c38b96 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 @@ -41,7 +41,7 @@ use openbitfun_services_core::bounded_fs::{read_bounded_text, BoundedTextRead}; use openbitfun_services_core::workspace_text::read_workspace_relative_text_bounded; #[cfg(feature = "external-sources")] use sha2::{Digest, Sha256}; -use std::collections::HashSet; +use std::collections::{HashMap, HashSet}; use std::path::{Path, PathBuf}; use std::sync::OnceLock; use tokio::fs; @@ -93,6 +93,7 @@ mod implicit_invocation_policy_tests { source_slot: "codex".to_string(), source_id: "codex".to_string(), source_label: "Codex".to_string(), + installation_source: None, dir_name: name.to_string(), is_builtin: false, group_key: None, @@ -186,6 +187,49 @@ async fn local_source_path_is_cacheable(path: &Path) -> bool { } } +// The skills installer records repository provenance separately from SKILL.md. +// Read existing lock versions without rewriting them; a missing origin must +// never turn a name-only match into an installed marketplace package. +fn parse_skill_installation_sources(content: &str) -> HashMap { + let lock: serde_json::Value = match serde_json::from_str(content) { + Ok(lock) => lock, + Err(error) => { + warn!("Ignoring invalid skill installation provenance: {}", error); + return HashMap::new(); + } + }; + lock.get("skills") + .and_then(serde_json::Value::as_object) + .into_iter() + .flatten() + .filter_map(|(name, entry)| { + if entry.get("sourceType").and_then(serde_json::Value::as_str) != Some("github") { + return None; + } + let source = entry.get("source")?.as_str()?.trim(); + (!source.is_empty()).then(|| (name.clone(), source.to_string())) + }) + .collect() +} + +fn skill_installation_lock_path(entry: &SkillRootEntry) -> Option { + match (entry.level, entry.slot) { + (SkillLocation::Project, "agents") => { + Some(entry.path.parent()?.parent()?.join("skills-lock.json")) + } + (SkillLocation::User, "home.agents") => { + let default_path = entry.path.parent()?.join(".skill-lock.json"); + Some( + std::env::var_os("XDG_STATE_HOME") + .filter(|path| !path.is_empty()) + .map(|path| PathBuf::from(path).join("skills/.skill-lock.json")) + .unwrap_or(default_path), + ) + } + _ => None, + } +} + #[cfg(test)] mod local_skill_scan_tests { use super::{SkillLocation, SkillRegistry, SkillRootEntry}; @@ -213,6 +257,66 @@ mod local_skill_scan_tests { } } + #[tokio::test] + async fn installation_source_uses_only_the_matching_project_lock_and_existing_skills() { + let temp = tempfile::tempdir().unwrap(); + let skills_path = temp.path().join(".agents/skills"); + write_skill(&skills_path.join("shared-review")); + fs::write(temp.path().join("skills-lock.json"), r#"{ + "version": 1, "skills": { + "shared-review": {"source":"first/skills", "sourceType":"github", "computedHash":"old"}, + "deleted": {"source":"other/skills", "sourceType":"github"} + } + }"#).unwrap(); + let mut entry = test_root(&skills_path); + entry.level = SkillLocation::Project; + entry.slot = "agents"; + let scanned = SkillRegistry::scan_skills_in_dir(&entry).await; + assert_eq!(scanned.len(), 1); + assert_eq!( + scanned[0].info.installation_source.as_deref(), + Some("first/skills") + ); + + entry.slot = "claude"; + assert!(SkillRegistry::scan_skills_in_dir(&entry).await[0] + .info + .installation_source + .is_none()); + entry.slot = "agents"; + fs::write( + temp.path().join("skills-lock.json"), + "invalid existing user data", + ) + .unwrap(); + let scanned = SkillRegistry::scan_skills_in_dir(&entry).await; + assert_eq!(scanned.len(), 1); + assert!(scanned[0].info.installation_source.is_none()); + assert_eq!( + fs::read_to_string(temp.path().join("skills-lock.json")).unwrap(), + "invalid existing user data" + ); + } + + #[test] + fn installation_source_accepts_existing_global_lock_versions_without_name_fallback() { + for version in [1, 2, 3] { + let content = format!( + r#"{{"version":{version},"skills":{{ + "eli5":{{"source":"first/skills","sourceType":"github","unknown":true}}, + "local":{{"source":"first/skills","sourceType":"local"}}, + "missing":{{"sourceType":"github"}} + }}}}"# + ); + let sources = super::parse_skill_installation_sources(&content); + assert_eq!(sources.len(), 1); + assert_eq!( + sources.get("eli5").map(String::as_str), + Some("first/skills") + ); + } + } + #[cfg(unix)] fn create_dir_symlink(target: &Path, link: &Path) -> bool { std::os::unix::fs::symlink(target, link).is_ok() @@ -794,6 +898,25 @@ impl SkillRegistry { } }; let mut cacheable = root_cacheable; + let installation_sources = if let Some(lock_path) = skill_installation_lock_path(entry) { + cacheable &= local_source_path_is_cacheable(&lock_path).await; + match fs::read_to_string(&lock_path).await { + Ok(content) => parse_skill_installation_sources(&content), + Err(error) => { + if error.kind() != std::io::ErrorKind::NotFound { + warn!( + "Failed to read skill installation provenance {}: {}", + lock_path.display(), + error + ); + cacheable = false; + } + HashMap::new() + } + } + } else { + HashMap::new() + }; loop { let item = match read_dir.next_entry().await { @@ -866,7 +989,7 @@ impl SkillRegistry { SkillLocation::User => USER_SKILL_KEY_PREFIX, SkillLocation::Project => PROJECT_SKILL_KEY_PREFIX, }; - skills.push(SkillCandidate::from_data( + let mut candidate = SkillCandidate::from_data( skill_data, entry.slot, entry.source_id, @@ -874,7 +997,10 @@ impl SkillRegistry { key_prefix, entry.priority, entry.is_builtin, - )); + ); + candidate.info.installation_source = + installation_sources.get(&candidate.info.name).cloned(); + skills.push(candidate); } Err(error) => { error!("Failed to parse SKILL.md in {}: {}", path.display(), error); @@ -1279,6 +1405,24 @@ impl SkillRegistry { .collect::>() .await; + let installation_sources = if roots + .iter() + .any(|(entry, items)| entry.slot == "agents" && !items.is_empty()) + { + match fs.read_file_text(&format!("{root}/skills-lock.json")).await { + Ok(content) => parse_skill_installation_sources(&content), + Err(error) => { + debug!( + "Remote skill installation provenance unavailable for {}: {}", + root, error + ); + HashMap::new() + } + } + } else { + HashMap::new() + }; + let directories = roots.iter().flat_map(|(entry, items)| { items .iter() @@ -1286,43 +1430,51 @@ impl SkillRegistry { .map(move |item| (entry, item)) }); let skill_scans = directories - .map(|(entry, item)| async move { - let dir_name = normalize_remote_skill_dir_name(&item.path)?; - let skill_md_path = format!("{}/SKILL.md", item.path.trim_end_matches('/')); - if !fs.is_file(&skill_md_path).await.unwrap_or(false) { - return None; - } - let content = match fs.read_file_text(&skill_md_path).await { - Ok(content) => content, - Err(error) => { - debug!("Failed to read {}: {}", skill_md_path, error); + .map(|(entry, item)| { + let installation_sources = &installation_sources; + async move { + let dir_name = normalize_remote_skill_dir_name(&item.path)?; + let skill_md_path = format!("{}/SKILL.md", item.path.trim_end_matches('/')); + if !fs.is_file(&skill_md_path).await.unwrap_or(false) { return None; } - }; - let mut skill_data = match Self::parse_skill_markdown( - item.path.clone(), - &content, - SkillLocation::Project, - false, - entry.slot, - ) { - Ok(data) => data, - Err(error) => { - error!("Failed to parse SKILL.md in {}: {}", item.path, error); - return None; + let content = match fs.read_file_text(&skill_md_path).await { + Ok(content) => content, + Err(error) => { + debug!("Failed to read {}: {}", skill_md_path, error); + return None; + } + }; + let mut skill_data = match Self::parse_skill_markdown( + item.path.clone(), + &content, + SkillLocation::Project, + false, + entry.slot, + ) { + Ok(data) => data, + Err(error) => { + error!("Failed to parse SKILL.md in {}: {}", item.path, error); + return None; + } + }; + Self::apply_remote_openai_policy(&mut skill_data, fs, &item.path).await; + skill_data.dir_name = dir_name; + let mut candidate = SkillCandidate::from_data( + skill_data, + entry.slot, + entry.source_id, + entry.source_label, + PROJECT_SKILL_KEY_PREFIX, + entry.priority, + false, + ); + if entry.slot == "agents" { + candidate.info.installation_source = + installation_sources.get(&candidate.info.name).cloned(); } - }; - Self::apply_remote_openai_policy(&mut skill_data, fs, &item.path).await; - skill_data.dir_name = dir_name; - Some(SkillCandidate::from_data( - skill_data, - entry.slot, - entry.source_id, - entry.source_label, - PROJECT_SKILL_KEY_PREFIX, - entry.priority, - false, - )) + Some(candidate) + } }) .collect::>(); stream::iter(skill_scans) @@ -2247,6 +2399,7 @@ mod remote_scan_tests { active: AtomicUsize, peak: AtomicUsize, calls: AtomicUsize, + installation_lock: Option, } impl DelayedFs { @@ -2266,6 +2419,13 @@ mod remote_scan_tests { } async fn read_file_text(&self, path: &str) -> anyhow::Result { self.round_trip().await; + if path.ends_with("skills-lock.json") { + assert_eq!(path, "/remote/project/skills-lock.json"); + return self + .installation_lock + .clone() + .ok_or_else(|| anyhow::anyhow!("missing lock")); + } if path.ends_with("openai.yaml") { return Ok("policy:\n allow_implicit_invocation: false\n".into()); } @@ -2286,7 +2446,9 @@ mod remote_scan_tests { } async fn is_dir(&self, path: &str) -> anyhow::Result { self.round_trip().await; - Ok(path.contains("/.openbitfun/") || path.contains("/.codex/")) + Ok(path.contains("/.openbitfun/") + || path.contains("/.codex/") + || (self.installation_lock.is_some() && path.contains("/.agents/"))) } async fn read_dir(&self, path: &str) -> anyhow::Result> { self.round_trip().await; @@ -2303,6 +2465,40 @@ mod remote_scan_tests { } } + #[tokio::test] + async fn remote_scan_reads_provenance_from_the_remote_project_only() { + let mut fs = DelayedFs { + installation_lock: Some( + r#"{"version":1,"skills":{ + "skill-00":{"source":"remote/skills","sourceType":"github"}, + "deleted":{"source":"missing/skills","sourceType":"github"} + }}"# + .into(), + ), + ..Default::default() + }; + let skills = SkillRegistry::scan_remote_project_skills(&fs, "/remote/project/").await; + assert_eq!(skills.len(), 36); + let installed = skills + .iter() + .filter(|skill| skill.info.installation_source.is_some()) + .collect::>(); + assert_eq!(installed.len(), 1); + assert_eq!(installed[0].info.source_slot, "agents"); + assert_eq!(installed[0].info.name, "skill-00"); + assert_eq!( + installed[0].info.installation_source.as_deref(), + Some("remote/skills") + ); + + fs.installation_lock = Some("invalid remote lock".into()); + let skills = SkillRegistry::scan_remote_project_skills(&fs, "/remote/project/").await; + assert_eq!(skills.len(), 36); + assert!(skills + .iter() + .all(|skill| skill.info.installation_source.is_none())); + } + #[tokio::test] async fn remote_scan_preserves_order_and_policy_with_bounded_io() { let fs = DelayedFs::default(); diff --git a/src/crates/assembly/core/src/agentic/tools/implementations/skills/resolver.rs b/src/crates/assembly/core/src/agentic/tools/implementations/skills/resolver.rs index 645fb13bac..3210418b7b 100644 --- a/src/crates/assembly/core/src/agentic/tools/implementations/skills/resolver.rs +++ b/src/crates/assembly/core/src/agentic/tools/implementations/skills/resolver.rs @@ -26,6 +26,7 @@ mod tests { source_slot: "openbitfun-system".to_string(), source_id: "openbitfun".to_string(), source_label: "OpenBitFun".to_string(), + installation_source: None, dir_name: dir_name.to_string(), is_builtin: true, group_key: None, @@ -47,6 +48,7 @@ mod tests { source_slot: "openbitfun".to_string(), source_id: "openbitfun".to_string(), source_label: "OpenBitFun".to_string(), + installation_source: None, dir_name: dir_name.to_string(), is_builtin: false, group_key: None, diff --git a/src/crates/execution/agent-runtime/src/skills/selection.rs b/src/crates/execution/agent-runtime/src/skills/selection.rs index 3d775d838c..fe83d86083 100644 --- a/src/crates/execution/agent-runtime/src/skills/selection.rs +++ b/src/crates/execution/agent-runtime/src/skills/selection.rs @@ -40,6 +40,7 @@ impl SkillCandidate { source_slot: data.source_slot, source_id: source_id.to_string(), source_label: source_label.to_string(), + installation_source: None, dir_name: data.dir_name, is_builtin, group_key, @@ -332,6 +333,7 @@ mod tests { source_slot: String::new(), source_id: String::new(), source_label: String::new(), + installation_source: None, dir_name: name.to_string(), is_builtin: false, group_key: None, diff --git a/src/crates/execution/agent-runtime/src/skills/types.rs b/src/crates/execution/agent-runtime/src/skills/types.rs index 963e08ad98..3782242eae 100644 --- a/src/crates/execution/agent-runtime/src/skills/types.rs +++ b/src/crates/execution/agent-runtime/src/skills/types.rs @@ -55,6 +55,9 @@ pub struct SkillInfo { /// Stable product name supplied by the source definition. #[serde(default)] pub source_label: String, + /// Repository recorded by the installer, distinct from the discovery ecosystem. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub installation_source: Option, pub dir_name: String, #[serde(default)] pub is_builtin: bool, diff --git a/src/crates/execution/agent-runtime/tests/agent_definition_contracts/skill_contracts.rs b/src/crates/execution/agent-runtime/tests/agent_definition_contracts/skill_contracts.rs index 756717e0a2..884f8dd467 100644 --- a/src/crates/execution/agent-runtime/tests/agent_definition_contracts/skill_contracts.rs +++ b/src/crates/execution/agent-runtime/tests/agent_definition_contracts/skill_contracts.rs @@ -24,6 +24,7 @@ fn builtin_skill(dir_name: &str) -> SkillInfo { source_slot: "openbitfun-system".to_string(), source_id: "openbitfun".to_string(), source_label: "OpenBitFun".to_string(), + installation_source: None, dir_name: dir_name.to_string(), is_builtin: true, group_key: builtin_skill_group_key(dir_name).map(str::to_string), @@ -45,6 +46,7 @@ fn custom_user_skill(dir_name: &str) -> SkillInfo { source_slot: "openbitfun".to_string(), source_id: "openbitfun".to_string(), source_label: "OpenBitFun".to_string(), + installation_source: None, dir_name: dir_name.to_string(), is_builtin: false, group_key: None, @@ -56,6 +58,20 @@ fn custom_user_skill(dir_name: &str) -> SkillInfo { } } +#[test] +fn skill_installation_source_is_optional_for_legacy_payloads_and_round_trips() { + let original = custom_user_skill("eli5"); + let legacy = serde_json::to_value(&original).unwrap(); + assert!(legacy.get("installationSource").is_none()); + let mut decoded: SkillInfo = serde_json::from_value(legacy.clone()).unwrap(); + assert!(decoded.installation_source.is_none()); + assert_eq!(serde_json::to_value(&decoded).unwrap(), legacy); + decoded.installation_source = Some("first/skills".into()); + let current: SkillInfo = + serde_json::from_value(serde_json::to_value(decoded).unwrap()).unwrap(); + assert_eq!(current.installation_source.as_deref(), Some("first/skills")); +} + #[test] fn skill_source_dialect_is_derived_from_the_stable_source_slot() { let markdown = "---\ndescription: Directory fallback.\n---\n\nBody.\n"; @@ -267,6 +283,7 @@ fn project_skill(dir_name: &str) -> SkillInfo { source_slot: "openbitfun".to_string(), source_id: "openbitfun".to_string(), source_label: "OpenBitFun".to_string(), + installation_source: None, dir_name: dir_name.to_string(), is_builtin: false, group_key: None, diff --git a/src/web-ui/src/app/scenes/skills/SkillsScene.tsx b/src/web-ui/src/app/scenes/skills/SkillsScene.tsx index 7f31b89a92..0b04c08915 100644 --- a/src/web-ui/src/app/scenes/skills/SkillsScene.tsx +++ b/src/web-ui/src/app/scenes/skills/SkillsScene.tsx @@ -24,6 +24,7 @@ import { useI18n } from '@/infrastructure/i18n/hooks/useI18n'; import { GalleryDetailModal, GalleryPageHeader } from '@/app/components'; import type { SkillInfo, SkillLevel, SkillMarketItem } from '@/infrastructure/config/types'; +import { installedSkillMarketIds, isSkillMarketItemInstalled } from '@/infrastructure/config/skillMarketInstallation'; import { buildSkillCoverageSourceMap, canDeleteSkill, @@ -132,8 +133,8 @@ const SkillsScene: React.FC = () => { enabled: desktopConfigAvailable, }); - const installedSkillNames = useMemo( - () => new Set(installed.skills.map((skill) => skill.name)), + const installedMarketIds = useMemo( + () => installedSkillMarketIds(installed.skills), [installed.skills], ); const coverageSourceBySkillKey = useMemo( @@ -167,7 +168,7 @@ const SkillsScene: React.FC = () => { const market = useSkillMarket({ searchQuery: marketQuery, - installedSkillNames, + installedMarketIds, pageSize: 15, enabled: desktopConfigAvailable, onInstalledChanged: async () => { @@ -689,7 +690,7 @@ const SkillsScene: React.FC = () => {
{market.marketSkills.map((skill, index) => { - const isInstalled = installedSkillNames.has(skill.name); + const isInstalled = isSkillMarketItemInstalled(skill, installedMarketIds); const isDownloading = market.downloadingPackage === skill.installId; return ( { : t('list.item.project')} - ) : selectedMarketSkill && installedSkillNames.has(selectedMarketSkill.name) ? ( + ) : selectedMarketSkill && isSkillMarketItemInstalled(selectedMarketSkill, installedMarketIds) ? ( }> {t('market.item.installed')} @@ -847,7 +848,7 @@ const SkillsScene: React.FC = () => { ) : selectedMarketSkill ? ( <> - {installedSkillNames.has(selectedMarketSkill.name) ? ( + {isSkillMarketItemInstalled(selectedMarketSkill, installedMarketIds) ? ( diff --git a/src/web-ui/src/app/scenes/skills/hooks/useSkillMarket.test.tsx b/src/web-ui/src/app/scenes/skills/hooks/useSkillMarket.test.tsx index 1d33e338e2..084610d3d3 100644 --- a/src/web-ui/src/app/scenes/skills/hooks/useSkillMarket.test.tsx +++ b/src/web-ui/src/app/scenes/skills/hooks/useSkillMarket.test.tsx @@ -39,10 +39,10 @@ vi.mock('@/shared/notification-system', () => ({ let currentMarket: ReturnType | null = null; -function Harness({ enabled }: { enabled: boolean }) { +function Harness({ enabled, installedMarketIds = new Set() }: { enabled: boolean; installedMarketIds?: Set }) { const market = useSkillMarket({ searchQuery: '', - installedSkillNames: new Set(), + installedMarketIds, enabled, onInstalledChanged: installedChangedMock, }); @@ -73,6 +73,28 @@ describe('useSkillMarket', () => { container.remove(); }); + it('prioritizes only the installed repository when market skills share a name', async () => { + const first = { + id: 'first/skills/eli5', name: 'eli5', source: 'first/skills', + installId: 'first/skills@eli5', installs: 1, description: '', url: '', + }; + const second = { + ...first, id: 'second/skills/eli5', source: 'second/skills', + installId: 'second/skills@eli5', installs: 100, + }; + listSkillMarketMock.mockResolvedValue([second, first]); + await act(async () => { + root.render(); + }); + expect(currentMarket?.marketSkills.map(skill => skill.id)).toEqual([first.id, second.id]); + + await act(async () => { + root.render(); + }); + expect(currentMarket?.marketSkills.map(skill => skill.id)).toEqual([second.id, first.id]); + expect(listSkillMarketMock).toHaveBeenCalledTimes(1); + }); + it('does not query the skill market outside the desktop app', async () => { await act(async () => { root.render(); diff --git a/src/web-ui/src/app/scenes/skills/hooks/useSkillMarket.ts b/src/web-ui/src/app/scenes/skills/hooks/useSkillMarket.ts index e7b7dcfe24..a39aa597b8 100644 --- a/src/web-ui/src/app/scenes/skills/hooks/useSkillMarket.ts +++ b/src/web-ui/src/app/scenes/skills/hooks/useSkillMarket.ts @@ -1,6 +1,7 @@ import { useCallback, useEffect, useLayoutEffect, useMemo, useRef, useState } from 'react'; import { useTranslation } from 'react-i18next'; import { configAPI } from '@/infrastructure/api'; +import { isSkillMarketItemInstalled } from '@/infrastructure/config/skillMarketInstallation'; import type { SkillLevel, SkillMarketItem } from '@/infrastructure/config/types'; import { useWorkspaceManagerSync } from '@/infrastructure/hooks/useWorkspaceManagerSync'; import { useNotification } from '@/shared/notification-system'; @@ -13,7 +14,7 @@ const MAX_TOTAL_SKILLS = 500; interface UseSkillMarketOptions { searchQuery: string; - installedSkillNames: Set; + installedMarketIds: Set; onInstalledChanged?: () => Promise | void; pageSize?: number; enabled?: boolean; @@ -21,7 +22,7 @@ interface UseSkillMarketOptions { export function useSkillMarket({ searchQuery, - installedSkillNames, + installedMarketIds, onInstalledChanged, pageSize = DEFAULT_PAGE_SIZE, enabled = true, @@ -119,7 +120,7 @@ export function useSkillMarket({ const entries = marketSkills.map((skill, index) => ({ skill, index, - installed: installedSkillNames.has(skill.name), + installed: isSkillMarketItemInstalled(skill, installedMarketIds), })); entries.sort((a, b) => { @@ -134,7 +135,7 @@ export function useSkillMarket({ }); return entries.map((entry) => entry.skill); - }, [installedSkillNames, marketSkills]); + }, [installedMarketIds, marketSkills]); const loadedPages = Math.ceil(displayMarketSkills.length / pageSize); const totalPages = hasMore ? loadedPages + 1 : Math.max(1, loadedPages); diff --git a/src/web-ui/src/infrastructure/config/components/SkillsConfig.tsx b/src/web-ui/src/infrastructure/config/components/SkillsConfig.tsx index f8448245af..d0b464edd4 100644 --- a/src/web-ui/src/infrastructure/config/components/SkillsConfig.tsx +++ b/src/web-ui/src/infrastructure/config/components/SkillsConfig.tsx @@ -5,6 +5,7 @@ import { useTranslation } from 'react-i18next'; import { FolderOpen, TrendingUp } from 'lucide-react'; import { useI18n } from '@/infrastructure/i18n/hooks/useI18n'; +import { installedSkillMarketIds, isSkillMarketItemInstalled } from '@/infrastructure/config/skillMarketInstallation'; import { ConfigPageHeader, ConfigPageLayout, ConfigPageContent, ConfigPageSection, ConfigCollectionItem } from './common'; import { useCurrentWorkspace } from '@/infrastructure/contexts/WorkspaceContext'; @@ -428,7 +429,7 @@ const SkillsConfig: React.FC = () => {
{displayMarketSkills.map((skill) => { const isDownloading = downloadingPackage === skill.installId; - const isInstalled = installedSkillNames.has(skill.name); + const isInstalled = isSkillMarketItemInstalled(skill, installedMarketIds); const sourceLabel = formatMarketSource(skill.source); const projectTooltipText = !hasWorkspace ? t('messages.noWorkspace') @@ -571,8 +572,8 @@ const SkillsConfig: React.FC = () => { ); - const installedSkillNames = useMemo( - () => new Set(skills.map((skill) => skill.name)), + const installedMarketIds = useMemo( + () => installedSkillMarketIds(skills), [skills] ); @@ -600,7 +601,7 @@ const SkillsConfig: React.FC = () => { const entries = marketSkills.map((skill, index) => ({ skill, index, - installed: installedSkillNames.has(skill.name), + installed: isSkillMarketItemInstalled(skill, installedMarketIds), })); entries.sort((a, b) => { @@ -617,7 +618,7 @@ const SkillsConfig: React.FC = () => { }); return entries.map((entry) => entry.skill); - }, [marketSkills, installedSkillNames]); + }, [marketSkills, installedMarketIds]); const handleMarketSearch = useCallback(() => { loadMarketSkills(marketKeyword); diff --git a/src/web-ui/src/infrastructure/config/skillMarketInstallation.test.ts b/src/web-ui/src/infrastructure/config/skillMarketInstallation.test.ts new file mode 100644 index 0000000000..989541406c --- /dev/null +++ b/src/web-ui/src/infrastructure/config/skillMarketInstallation.test.ts @@ -0,0 +1,31 @@ +import { describe, expect, it } from 'vitest'; +import type { SkillInfo, SkillMarketItem } from './types'; +import { installedSkillMarketIds, isSkillMarketItemInstalled } from './skillMarketInstallation'; + +const installed = (installationSource?: string): SkillInfo => ({ name: 'eli5', installationSource } as SkillInfo); +const market = (source: string, name = 'eli5'): SkillMarketItem => ({ source, name, installId: `${source}@${name}` } as SkillMarketItem); + +describe('skill marketplace installation identity', () => { + it('marks only the installed repository when several packages share a name', () => { + const ids = installedSkillMarketIds([installed('first/skills')]); + expect(['first/skills', 'second/skills', 'third/skills'].map(source => isSkillMarketItemInstalled(market(source), ids))) + .toEqual([true, false, false]); + expect(isSkillMarketItemInstalled(market('first/skills', 'another'), ids)).toBe(false); + }); + it('does not infer provenance for legacy, builtin or manually copied names', () => { + expect(isSkillMarketItemInstalled(market('first/skills'), installedSkillMarketIds([installed()]))).toBe(false); + }); + it('matches repository URL aliases but preserves the package identity', () => { + const ids = installedSkillMarketIds([installed('https://github.com/First/Skills.git/')]); + expect(isSkillMarketItemInstalled(market('first/skills'), ids)).toBe(true); + expect(isSkillMarketItemInstalled({ ...market('first/skills'), name: 'Display label' }, ids)).toBe(true); + }); + it('follows refreshed installs after replacement, removal or workspace changes', () => { + const first = market('first/skills'); + const second = market('second/skills'); + const ids = installedSkillMarketIds([installed('second/skills')]); + expect(isSkillMarketItemInstalled(first, ids)).toBe(false); + expect(isSkillMarketItemInstalled(second, ids)).toBe(true); + expect(isSkillMarketItemInstalled(second, installedSkillMarketIds([]))).toBe(false); + }); +}); diff --git a/src/web-ui/src/infrastructure/config/skillMarketInstallation.ts b/src/web-ui/src/infrastructure/config/skillMarketInstallation.ts new file mode 100644 index 0000000000..313ec915b0 --- /dev/null +++ b/src/web-ui/src/infrastructure/config/skillMarketInstallation.ts @@ -0,0 +1,28 @@ +import type { SkillInfo, SkillMarketItem } from './types'; + +function repositoryKey(source: string): string { + return source.trim() + .replace(/^https?:\/\/github\.com\//i, '') + .replace(/\/+$/, '') + .replace(/\.git$/i, '') + .toLowerCase(); +} + +function installationKey(source: string, name: string): string { + return `${repositoryKey(source)}@${name.trim()}`; +} + +/** A display name alone cannot establish marketplace provenance. */ +export function installedSkillMarketIds(skills: readonly SkillInfo[]): Set { + return new Set(skills.flatMap(skill => skill.installationSource?.trim() + ? [installationKey(skill.installationSource, skill.name)] + : [])); +} + +export function isSkillMarketItemInstalled(skill: SkillMarketItem, installedIds: ReadonlySet): boolean { + const separator = skill.installId.lastIndexOf('@'); + if (separator > 0) { + return installedIds.has(installationKey(skill.installId.slice(0, separator), skill.installId.slice(separator + 1))); + } + return !!skill.source.trim() && installedIds.has(installationKey(skill.source, skill.name)); +} diff --git a/src/web-ui/src/infrastructure/config/types/index.ts b/src/web-ui/src/infrastructure/config/types/index.ts index 3ca304cbc3..7a5718529c 100644 --- a/src/web-ui/src/infrastructure/config/types/index.ts +++ b/src/web-ui/src/infrastructure/config/types/index.ts @@ -408,6 +408,8 @@ export interface SkillInfo { sourceId?: string; /** Stable product name supplied by the skill source definition. */ sourceLabel?: string; + /** Repository recorded by the installer; absent for legacy or untracked skills. */ + installationSource?: string | null; dirName: string; isBuiltin: boolean; groupKey?: string | null;