From ccee419d8687dd9133c35a5952fb7db65ff27110 Mon Sep 17 00:00:00 2001 From: Zhang <56248212+YonganZhang@users.noreply.github.com> Date: Tue, 23 Jun 2026 06:12:21 +0800 Subject: [PATCH] fix(skills): detect installed built-in catalog entries (#3656) Co-authored-by: Steven Enamakel --- app/.prettierignore | 1 + app/scripts/e2e-web-build.sh | 10 ++- .../components/skills/SkillsExplorerTab.tsx | 63 +++++++++++++++++-- .../__tests__/SkillsExplorerTab.test.tsx | 63 +++++++++++++++++-- scripts/test-rust-e2e.sh | 13 ++-- src/openhuman/workflows/ops.rs | 4 +- src/openhuman/workflows/ops_install.rs | 50 ++++++++++++--- src/openhuman/workflows/ops_tests.rs | 59 +++++++++++++++++ tests/skill_registry_e2e.rs | 29 +++++---- 9 files changed, 253 insertions(+), 39 deletions(-) diff --git a/app/.prettierignore b/app/.prettierignore index cded8a49f..fa5f5cc7b 100644 --- a/app/.prettierignore +++ b/app/.prettierignore @@ -1,5 +1,6 @@ node_modules dist +dist-web coverage app src-tauri diff --git a/app/scripts/e2e-web-build.sh b/app/scripts/e2e-web-build.sh index 51e4346d3..776c9f0ee 100755 --- a/app/scripts/e2e-web-build.sh +++ b/app/scripts/e2e-web-build.sh @@ -6,7 +6,13 @@ APP_DIR="$(cd "$(dirname "$0")/.." && pwd)" REPO_ROOT="$(cd "$APP_DIR/.." && pwd)" cd "$APP_DIR" -RUST_HOST_TRIPLE="${RUST_HOST_TRIPLE:-$(rustc -vV | awk '/^host: / { print $2 }')}" +RUSTC_BIN="$(command -v rustc)" +CARGO_BIN="${CARGO_BIN:-$(dirname "$RUSTC_BIN")/cargo}" +if [ ! -x "$CARGO_BIN" ]; then + CARGO_BIN="$(command -v cargo)" +fi + +RUST_HOST_TRIPLE="${RUST_HOST_TRIPLE:-$("$RUSTC_BIN" -vV | awk '/^host: / { print $2 }')}" E2E_WEB_CORE_TARGET_DIR="${E2E_WEB_CORE_TARGET_DIR:-$REPO_ROOT/target/e2e-web-${RUST_HOST_TRIPLE}}" export VITE_BACKEND_URL="http://127.0.0.1:${E2E_MOCK_PORT:-18473}" @@ -24,4 +30,4 @@ fi echo "Building web E2E bundle with backend ${VITE_BACKEND_URL}" pnpm run build:web echo "Building standalone openhuman-core for web E2E into ${E2E_WEB_CORE_TARGET_DIR}..." -CARGO_TARGET_DIR="$E2E_WEB_CORE_TARGET_DIR" "$REPO_ROOT/scripts/ci-cancel-aware.sh" cargo build --manifest-path "$REPO_ROOT/Cargo.toml" --bin openhuman-core +CARGO_TARGET_DIR="$E2E_WEB_CORE_TARGET_DIR" bash "$REPO_ROOT/scripts/ci-cancel-aware.sh" "$CARGO_BIN" build --manifest-path "$REPO_ROOT/Cargo.toml" --bin openhuman-core diff --git a/app/src/components/skills/SkillsExplorerTab.tsx b/app/src/components/skills/SkillsExplorerTab.tsx index 69116689d..082610bd2 100644 --- a/app/src/components/skills/SkillsExplorerTab.tsx +++ b/app/src/components/skills/SkillsExplorerTab.tsx @@ -16,6 +16,58 @@ const log = debug('skills:explorer-tab'); const CATALOG_PAGE_SIZE = 60; const SEARCH_DEBOUNCE_MS = 300; +function slugifyInstallKey(value: string | null | undefined): string | null { + const raw = value?.trim(); + if (!raw) return null; + + let out = ''; + let lastDash = false; + for (const ch of raw) { + if (/[a-z0-9]/i.test(ch)) { + out += ch.toLowerCase(); + lastDash = false; + } else if (!lastDash && out.length > 0) { + out += '-'; + lastDash = true; + } + } + return out.replace(/-+$/, '') || null; +} + +function lastPathSegment(value: string | null | undefined): string | null { + const raw = value?.trim(); + if (!raw) return null; + const parts = raw.split(/[/:#?]+/).filter(Boolean); + return parts.at(-1) ?? null; +} + +function parentPathSegment(value: string | null | undefined): string | null { + const raw = value?.trim(); + if (!raw) return null; + const parts = raw.split(/[\\/]+/).filter(Boolean); + return parts.length >= 2 ? parts.at(-2) ?? null : null; +} + +function catalogInstallKeys(entry: CatalogEntry): string[] { + return [ + slugifyInstallKey(entry.id), + slugifyInstallKey(lastPathSegment(entry.id)), + slugifyInstallKey(parentPathSegment(entry.docs_path)), + slugifyInstallKey(parentPathSegment(entry.download_url)), + ].filter((key): key is string => Boolean(key)); +} + +function workflowInstallKeys(skill: WorkflowSummary): string[] { + return [ + slugifyInstallKey(skill.id), + slugifyInstallKey(parentPathSegment(skill.location)), + ].filter((key): key is string => Boolean(key)); +} + +function isCatalogEntryInstalled(entry: CatalogEntry, installedKeys: Set): boolean { + return catalogInstallKeys(entry).some(key => installedKeys.has(key)); +} + function SourceBadge({ source }: { source: string }) { const SOURCE_COLORS: Record = { 'built-in': @@ -577,7 +629,10 @@ export default function SkillsExplorerTab({ onToast }: SkillsExplorerTabProps) { } }, [view, debouncedQuery, activeSourceFilter, fetchCatalog]); - const installedIds = useMemo(() => new Set(skills.map(s => s.id)), [skills]); + const installedKeys = useMemo( + () => new Set(skills.flatMap(skill => workflowInstallKeys(skill))), + [skills] + ); const filteredSkills = useMemo(() => { const q = searchQuery.toLowerCase().trim(); @@ -902,7 +957,7 @@ export default function SkillsExplorerTab({ onToast }: SkillsExplorerTabProps) { setDetailEntry(entry)} onInstall={() => void handleRegistryInstall(entry)} @@ -941,13 +996,13 @@ export default function SkillsExplorerTab({ onToast }: SkillsExplorerTabProps) { { setDetailEntry(null); setDetailSkill(null); }} onInstall={ - detailEntry && !installedIds.has(detailEntry.id) + detailEntry && !isCatalogEntryInstalled(detailEntry, installedKeys) ? () => { void handleRegistryInstall(detailEntry); setDetailEntry(null); diff --git a/app/src/components/skills/__tests__/SkillsExplorerTab.test.tsx b/app/src/components/skills/__tests__/SkillsExplorerTab.test.tsx index e2110b431..d9cf432ed 100644 --- a/app/src/components/skills/__tests__/SkillsExplorerTab.test.tsx +++ b/app/src/components/skills/__tests__/SkillsExplorerTab.test.tsx @@ -1,4 +1,4 @@ -import { act, fireEvent, render, screen, waitFor } from '@testing-library/react'; +import { act, fireEvent, render, screen, waitFor, within } from '@testing-library/react'; import { beforeEach, describe, expect, it, vi } from 'vitest'; import type { CatalogEntry } from '../../../services/api/skillRegistryApi'; @@ -307,16 +307,69 @@ describe('SkillsExplorerTab', () => { const { skillRegistryApi } = await import( '../../../services/api/skillRegistryApi' ); - const installedSkill = { ...MOCK_SKILL, id: 'registry-skill-1' }; + const catalogEntry = { + ...MOCK_CATALOG_ENTRY, + id: 'built-in/apple-notes', + name: 'Apple Notes', + docs_path: 'skills/apple-notes/SKILL.md', + }; + const installedSkill = { + ...MOCK_SKILL, + id: 'apple-notes', + name: 'Apple Notes', + location: '/Users/test/.openhuman/skills/apple-notes/SKILL.md', + }; vi.mocked(workflowsApi.listWorkflows).mockResolvedValue([installedSkill]); - vi.mocked(skillRegistryApi.browse).mockResolvedValue([MOCK_CATALOG_ENTRY]); + vi.mocked(skillRegistryApi.browse).mockResolvedValue([catalogEntry]); render(); await waitFor(() => { - expect(screen.getByText('Registry Skill')).toBeInTheDocument(); + expect(screen.getByText('Apple Notes')).toBeInTheDocument(); }); - expect(screen.getByTestId('registry-tile-registry-skill-1')).toBeInTheDocument(); + const tile = screen.getByTestId('registry-tile-built-in/apple-notes'); + expect(within(tile).getByText('Installed')).toBeInTheDocument(); + expect( + within(tile).queryByTestId('registry-install-built-in/apple-notes') + ).not.toBeInTheDocument(); + + await act(async () => { + fireEvent.click(tile); + }); + + await waitFor(() => { + expect(screen.getAllByText('Apple Notes').length).toBeGreaterThan(1); + }); + expect(screen.queryByRole('button', { name: 'Install' })).not.toBeInTheDocument(); + }); + + it('does not mark catalog entries installed by display name alone', async () => { + const { workflowsApi } = await import('../../../services/api/workflowsApi'); + const { skillRegistryApi } = await import( + '../../../services/api/skillRegistryApi' + ); + const catalogEntry = { + ...MOCK_CATALOG_ENTRY, + id: 'built-in/apple-notes', + name: 'Apple Notes', + docs_path: 'skills/apple-notes/SKILL.md', + }; + const unrelatedInstalledSkill = { + ...MOCK_SKILL, + id: 'apple-notes-copy', + name: 'Apple Notes', + location: '/Users/test/.openhuman/skills/apple-notes-copy/SKILL.md', + }; + vi.mocked(workflowsApi.listWorkflows).mockResolvedValue([unrelatedInstalledSkill]); + vi.mocked(skillRegistryApi.browse).mockResolvedValue([catalogEntry]); + + render(); + + const tile = await screen.findByTestId('registry-tile-built-in/apple-notes'); + expect(within(tile).queryByText('Installed')).not.toBeInTheDocument(); + expect( + within(tile).getByTestId('registry-install-built-in/apple-notes') + ).toBeInTheDocument(); }); it('has an install from URL button', async () => { diff --git a/scripts/test-rust-e2e.sh b/scripts/test-rust-e2e.sh index b086796f4..0b6590ccc 100755 --- a/scripts/test-rust-e2e.sh +++ b/scripts/test-rust-e2e.sh @@ -128,14 +128,19 @@ export VITE_BACKEND_URL="$MOCK_API_URL" cd "$REPO_ROOT" source "$HOME/.cargo/env" 2>/dev/null || true +RUSTC_BIN="$(command -v rustc)" +CARGO_BIN="${CARGO_BIN:-$(dirname "$RUSTC_BIN")/cargo}" +if [ ! -x "$CARGO_BIN" ]; then + CARGO_BIN="$(command -v cargo)" +fi echo "[rust-e2e] Running ${#SUITES[@]} suite(s) serially." for suite in "${SUITES[@]}"; do if [ "${#EXTRA_ARGS[@]}" -gt 0 ]; then - echo "[rust-e2e] cargo test --manifest-path Cargo.toml --test $suite -- ${EXTRA_ARGS[*]}" - "$SCRIPT_DIR/ci-cancel-aware.sh" cargo test --manifest-path Cargo.toml --test "$suite" -- "${EXTRA_ARGS[@]}" + echo "[rust-e2e] $CARGO_BIN test --manifest-path Cargo.toml --test $suite -- ${EXTRA_ARGS[*]}" + bash "$SCRIPT_DIR/ci-cancel-aware.sh" "$CARGO_BIN" test --manifest-path Cargo.toml --test "$suite" -- "${EXTRA_ARGS[@]}" else - echo "[rust-e2e] cargo test --manifest-path Cargo.toml --test $suite" - "$SCRIPT_DIR/ci-cancel-aware.sh" cargo test --manifest-path Cargo.toml --test "$suite" + echo "[rust-e2e] $CARGO_BIN test --manifest-path Cargo.toml --test $suite" + bash "$SCRIPT_DIR/ci-cancel-aware.sh" "$CARGO_BIN" test --manifest-path Cargo.toml --test "$suite" fi done diff --git a/src/openhuman/workflows/ops.rs b/src/openhuman/workflows/ops.rs index ec4601ab6..db3116557 100644 --- a/src/openhuman/workflows/ops.rs +++ b/src/openhuman/workflows/ops.rs @@ -50,7 +50,9 @@ pub(crate) use super::ops_create::{create_workflow_inner, slugify_workflow_name} #[cfg(test)] pub(crate) use super::ops_discover::discover_workflows_inner; #[cfg(test)] -pub(crate) use super::ops_install::{derive_install_slug, normalize_install_url}; +pub(crate) use super::ops_install::{ + derive_install_slug, install_workflow_from_url_with_home, normalize_install_url, +}; #[cfg(test)] pub(crate) use super::ops_types::{ MAX_NAME_LEN, RESOURCE_DIRS, SKILL_MD, TRUST_MARKER, WORKFLOW_MD, WORKFLOW_TOML, diff --git a/src/openhuman/workflows/ops_install.rs b/src/openhuman/workflows/ops_install.rs index 666d0470d..746880c3c 100644 --- a/src/openhuman/workflows/ops_install.rs +++ b/src/openhuman/workflows/ops_install.rs @@ -104,8 +104,10 @@ pub struct InstallWorkflowFromUrlOutcome { /// * Frontmatter is validated — `name` and `description` are required per /// the agentskills.io spec. /// * The slug is derived from `metadata.id` when present, otherwise the -/// sanitized `name` field. Collision with an existing directory is fatal -/// (no silent overwrite). +/// sanitized `name` field. If the target directory already contains a +/// `SKILL.md`, the install is treated as an idempotent success and reports +/// that the skill is already installed. Other directory collisions remain +/// fatal, and existing files are never silently overwritten. /// * Write is atomic: `SKILL.md.tmp` in the target dir, then `rename` on /// success. /// @@ -115,6 +117,15 @@ pub struct InstallWorkflowFromUrlOutcome { pub async fn install_workflow_from_url( workspace_dir: &Path, params: InstallWorkflowFromUrlParams, +) -> Result { + let home = dirs::home_dir(); + install_workflow_from_url_with_home(workspace_dir, params, home.as_deref()).await +} + +pub(crate) async fn install_workflow_from_url_with_home( + workspace_dir: &Path, + params: InstallWorkflowFromUrlParams, + home: Option<&Path>, ) -> Result { let raw_url = params.url.trim().to_string(); validate_install_url(&raw_url)?; @@ -148,10 +159,9 @@ pub async fn install_workflow_from_url( "[skills] install_workflow_from_url: entry" ); - let home = dirs::home_dir(); let trusted_before = is_workspace_trusted(workspace_dir); let before: std::collections::HashSet = - discover_workflows_inner(home.as_deref(), Some(workspace_dir), trusted_before) + discover_workflows_inner(home, Some(workspace_dir), trusted_before) .into_iter() .map(|s| s.name) .collect(); @@ -256,16 +266,36 @@ pub async fn install_workflow_from_url( // a `/.openhuman/trust` marker and would render the install invisible to the // skills list until the user opts the workspace into trust. let skills_root = home - .as_deref() .ok_or_else(|| "write failed: unable to resolve home directory".to_string())? .join(".openhuman") .join("skills"); let target_dir = skills_root.join(&slug); if target_dir.exists() { - return Err(format!( - "skill already installed as {slug:?} at {}", - target_dir.display() - )); + let target_file = target_dir.join(SKILL_MD); + if !target_file.is_file() { + return Err(format!( + "skill install target already exists but has no {SKILL_MD}: {}", + target_dir.display() + )); + } + + tracing::info!( + raw_url = %redacted_raw_url, + fetch_url = %redacted_fetch_url, + slug = %slug, + target = %target_file.display(), + "[skills] install_workflow_from_url: already installed" + ); + + return Ok(InstallWorkflowFromUrlOutcome { + url: raw_url, + stdout: format!( + "Skill {slug:?} is already installed at {}", + target_file.display() + ), + stderr: parse_warnings.join("\n"), + new_skills: Vec::new(), + }); } std::fs::create_dir_all(&target_dir).map_err(|e| { @@ -319,7 +349,7 @@ pub async fn install_workflow_from_url( } let trusted_after = is_workspace_trusted(workspace_dir); - let after = discover_workflows_inner(home.as_deref(), Some(workspace_dir), trusted_after); + let after = discover_workflows_inner(home, Some(workspace_dir), trusted_after); let new_skills: Vec = after .into_iter() .map(|s| s.name) diff --git a/src/openhuman/workflows/ops_tests.rs b/src/openhuman/workflows/ops_tests.rs index b0e1dc228..24e5a4017 100644 --- a/src/openhuman/workflows/ops_tests.rs +++ b/src/openhuman/workflows/ops_tests.rs @@ -7,6 +7,30 @@ fn write(path: &Path, content: &str) { std::fs::write(path, content).unwrap(); } +struct EnvVarGuard { + key: &'static str, + previous: Option, +} + +impl EnvVarGuard { + fn set(key: &'static str, value: &str) -> Self { + let previous = std::env::var_os(key); + unsafe { std::env::set_var(key, value) }; + Self { key, previous } + } +} + +impl Drop for EnvVarGuard { + fn drop(&mut self) { + unsafe { + match &self.previous { + Some(value) => std::env::set_var(self.key, value), + None => std::env::remove_var(self.key), + } + } + } +} + /// Workspace-only variant of [`load_workflow_metadata`] used by tests that care only /// about project-scope semantics. The production [`load_workflow_metadata`] now /// consults `dirs::home_dir()`; in unit tests that would non-deterministically @@ -1185,6 +1209,41 @@ fn parse_skill_md_str_bad_yaml_returns_empty_frontmatter_with_warning() { ); } +#[tokio::test] +async fn install_workflow_from_url_is_idempotent_when_skill_already_exists() { + use wiremock::matchers::{method, path}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + let _guard = EnvVarGuard::set("OPENHUMAN_SKILL_INSTALL_ALLOW_LOCAL_HTTP", "1"); + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/SKILL.md")) + .respond_with(ResponseTemplate::new(200).set_body_string( + "---\nname: apple-notes\ndescription: Apple Notes access\n---\n\n# Apple Notes\n", + )) + .mount(&server) + .await; + + let workspace = tempfile::tempdir().unwrap(); + let home = tempfile::tempdir().unwrap(); + let params = InstallWorkflowFromUrlParams { + url: format!("{}/SKILL.md", server.uri()), + timeout_secs: Some(5), + }; + + let first = + install_workflow_from_url_with_home(workspace.path(), params.clone(), Some(home.path())) + .await + .unwrap(); + assert_eq!(first.new_skills, vec!["apple-notes"]); + + let second = install_workflow_from_url_with_home(workspace.path(), params, Some(home.path())) + .await + .unwrap(); + assert!(second.new_skills.is_empty(), "{second:?}"); + assert!(second.stdout.contains("already installed"), "{second:?}"); +} + /// Happy path: install a SKILL.md under a synthetic user home, verify /// discovery sees it, uninstall, verify discovery no longer sees it and /// the on-disk dir is gone. diff --git a/tests/skill_registry_e2e.rs b/tests/skill_registry_e2e.rs index 041174ee6..37d38c8e6 100644 --- a/tests/skill_registry_e2e.rs +++ b/tests/skill_registry_e2e.rs @@ -208,11 +208,6 @@ fn assert_no_jsonrpc_error<'a>(v: &'a Value, context: &str) -> &'a Value { .unwrap_or_else(|| panic!("{context}: missing `result` field: {v}")) } -fn assert_jsonrpc_error<'a>(v: &'a Value, context: &str) -> &'a Value { - v.get("error") - .unwrap_or_else(|| panic!("{context}: expected JSON-RPC error, got: {v}")) -} - // ── Test ─────────────────────────────────────────────────────────────────── /// End-to-end coverage for the `openhuman.skill_registry_*` endpoints. @@ -223,7 +218,7 @@ fn assert_jsonrpc_error<'a>(v: &'a Value, context: &str) -> &'a Value { /// 3. `search` — queries for "git" and expects at least one match. /// 4. `schemas` — exposes CLI/RPC schemas for prod smoke scripts. /// 5. `install` — happy-path install of a skill. -/// 6. `install` — duplicate-rejection: same install must return an error. +/// 6. `install` — duplicate install returns idempotent success with no new skills. /// 7. `uninstall` — removes the installed skill. #[tokio::test] async fn skill_registry_e2e_sources_browse_search_install() { @@ -503,7 +498,7 @@ encrypt = false skill_file.display() ); - // ── Step 6: install (duplicate rejection) ───────────────────────────── + // ── Step 6: install (duplicate no-op success) ──────────────────────── let dup_resp = post_json_rpc( &rpc_base, @@ -512,14 +507,22 @@ encrypt = false json!({ "entry_id": entry_id }), ) .await; - let dup_error = assert_jsonrpc_error(&dup_resp, "skill_registry_install (duplicate)"); - let dup_message = dup_error - .get("message") + let dup_result = assert_no_jsonrpc_error(&dup_resp, "skill_registry_install (duplicate)"); + let dup_stdout = dup_result + .get("stdout") .and_then(Value::as_str) - .unwrap_or_default(); + .expect("duplicate install result must contain `stdout`"); assert!( - dup_message.contains("already installed"), - "duplicate install error should mention 'already installed', got: {dup_message}" + dup_stdout.contains("already installed"), + "duplicate install stdout should mention 'already installed', got: {dup_stdout}" + ); + let dup_new_skills = dup_result + .get("new_skills") + .and_then(Value::as_array) + .expect("duplicate install result must contain `new_skills` array"); + assert!( + dup_new_skills.is_empty(), + "duplicate install should report no new skills, got: {dup_new_skills:?}" ); // ── Step 7: uninstall ─────────────────────────────────────────────────