diff --git a/app/src/components/skills/SkillsExplorerTab.tsx b/app/src/components/skills/SkillsExplorerTab.tsx index 082610bd2..bae852206 100644 --- a/app/src/components/skills/SkillsExplorerTab.tsx +++ b/app/src/components/skills/SkillsExplorerTab.tsx @@ -559,7 +559,9 @@ export default function SkillsExplorerTab({ onToast }: SkillsExplorerTabProps) { setSkillsLoading(true); setSkillsError(null); try { - const result = await workflowsApi.listWorkflows(); + // Include `skills/`-root installs (registry installs land there) so they + // appear in the Installed tab and flip the catalog Install button. + const result = await workflowsApi.listWorkflows({ includeSkills: true }); log('fetchSkills: count=%d', result.length); setSkills(result); } catch (err) { @@ -697,7 +699,11 @@ export default function SkillsExplorerTab({ onToast }: SkillsExplorerTabProps) { setInstallingId(entry.id); try { const result = await skillRegistryApi.install(entry.id); - void fetchSkills(); + // Await the refetch so `installedKeys` is fresh before the button + // re-renders — otherwise it briefly flips back to "Install" between + // clearing the installing state and the list updating. `fetchSkills` + // swallows its own errors, so this never throws into the catch below. + await fetchSkills(); onToast?.({ type: 'success', title: t('skills.install.installComplete'), diff --git a/app/src/components/skills/WorkflowRunnerBody.tsx b/app/src/components/skills/WorkflowRunnerBody.tsx index 5b84a14ca..63757c3a1 100644 --- a/app/src/components/skills/WorkflowRunnerBody.tsx +++ b/app/src/components/skills/WorkflowRunnerBody.tsx @@ -460,7 +460,8 @@ export const WorkflowRunnerBody = ({ headerText, className }: SkillsRunnerBodyPr setSkillsLoading(true); setSkillsError(null); workflowsApi - .listWorkflows() + // Include `skills/`-root installs so registry-installed skills are runnable here. + .listWorkflows({ includeSkills: true }) .then(list => { if (cancelled) return; // Hide the codegraph-smoke skill — internal smoke-test only. @@ -1583,7 +1584,7 @@ export const WorkflowRunnerBody = ({ headerText, className }: SkillsRunnerBodyPr onCreated={() => { setEditOpen(false); void workflowsApi - .listWorkflows() + .listWorkflows({ includeSkills: true }) .then(list => setSkills(list.filter(s => s.id !== 'codegraph-smoke'))) .catch(() => {}); void workflowsApi diff --git a/app/src/services/api/__tests__/workflowsApi.test.ts b/app/src/services/api/__tests__/workflowsApi.test.ts index f8a7badd5..9fd6fea3c 100644 --- a/app/src/services/api/__tests__/workflowsApi.test.ts +++ b/app/src/services/api/__tests__/workflowsApi.test.ts @@ -281,6 +281,25 @@ describe('workflowsApi.listWorkflows', () => { expect(result[0].platforms).toEqual([]); expect(result[1].sourceFormat).toBe('legacy'); }); + + it('omits params by default (automations-only view)', async () => { + const { callCoreRpc } = await import('../../coreRpcClient'); + vi.mocked(callCoreRpc).mockResolvedValueOnce({ workflows: [] }); + await workflowsApi.listWorkflows(); + const call = vi.mocked(callCoreRpc).mock.calls[0][0]; + expect(call.method).toBe('openhuman.workflows_list'); + expect(call.params).toBeUndefined(); + }); + + it('passes include_skills when includeSkills is set', async () => { + const { callCoreRpc } = await import('../../coreRpcClient'); + vi.mocked(callCoreRpc).mockResolvedValueOnce({ workflows: [] }); + await workflowsApi.listWorkflows({ includeSkills: true }); + expect(callCoreRpc).toHaveBeenCalledWith({ + method: 'openhuman.workflows_list', + params: { include_skills: true }, + }); + }); }); describe('workflowsApi.readWorkflowResource', () => { diff --git a/app/src/services/api/skillRegistryApi.test.ts b/app/src/services/api/skillRegistryApi.test.ts index 090f9e72f..9091537ec 100644 --- a/app/src/services/api/skillRegistryApi.test.ts +++ b/app/src/services/api/skillRegistryApi.test.ts @@ -62,6 +62,7 @@ describe('skillRegistryApi', () => { expect(mockCallCoreRpc).toHaveBeenCalledWith({ method: 'openhuman.skill_registry_search', params: { query: 'demo' }, + timeoutMs: 120_000, }); expect(result[0].id).toBe('demo'); }); @@ -74,6 +75,7 @@ describe('skillRegistryApi', () => { expect(mockCallCoreRpc).toHaveBeenCalledWith({ method: 'openhuman.skill_registry_search', params: { query: 'q', source: 'ClawHub', category: 'devops' }, + timeoutMs: 120_000, }); }); @@ -90,7 +92,10 @@ describe('skillRegistryApi', () => { const result = await skillRegistryApi.sources(); - expect(mockCallCoreRpc).toHaveBeenCalledWith({ method: 'openhuman.skill_registry_sources' }); + expect(mockCallCoreRpc).toHaveBeenCalledWith({ + method: 'openhuman.skill_registry_sources', + timeoutMs: 120_000, + }); expect(result).toEqual(['built-in', 'ClawHub']); }); @@ -107,7 +112,10 @@ describe('skillRegistryApi', () => { const result = await skillRegistryApi.categories(); - expect(mockCallCoreRpc).toHaveBeenCalledWith({ method: 'openhuman.skill_registry_categories' }); + expect(mockCallCoreRpc).toHaveBeenCalledWith({ + method: 'openhuman.skill_registry_categories', + timeoutMs: 120_000, + }); expect(result).toEqual(['productivity', 'devops']); }); @@ -140,6 +148,7 @@ describe('skillRegistryApi', () => { expect(mockCallCoreRpc).toHaveBeenCalledWith({ method: 'openhuman.skill_registry_browse', params: { force_refresh: true }, + timeoutMs: 120_000, }); }); @@ -151,6 +160,7 @@ describe('skillRegistryApi', () => { expect(mockCallCoreRpc).toHaveBeenCalledWith({ method: 'openhuman.skill_registry_browse', params: { force_refresh: false }, + timeoutMs: 120_000, }); }); }); diff --git a/app/src/services/api/skillRegistryApi.ts b/app/src/services/api/skillRegistryApi.ts index 915a238a5..26b5ca569 100644 --- a/app/src/services/api/skillRegistryApi.ts +++ b/app/src/services/api/skillRegistryApi.ts @@ -4,6 +4,18 @@ import { callCoreRpc } from '../coreRpcClient'; const log = debug('skillRegistryApi'); +/** + * Catalog reads (`browse`/`search`/`sources`/`categories`) all funnel through + * the backend's single-flight `browse_catalog`, whose COLD fetch downloads the + * full ~90k-entry registry and can take ~80s. That comfortably exceeds the + * default 30s `CORE_RPC_TIMEOUT_MS`, so a first load (or post-TTL revalidate) + * would spuriously time out. Give these the longer per-call timeout the RPC + * client supports for exactly such "slow-but-alive" calls; warm-cache reads + * still return in milliseconds, so this only raises the ceiling for the rare + * cold path. 120s = ~80s cold download + margin (well under the 10min clamp). + */ +const CATALOG_RPC_TIMEOUT_MS = 120_000; + export interface CatalogEntry { id: string; name: string; @@ -72,7 +84,11 @@ export const skillRegistryApi = { log('browse: forceRefresh=%s', forceRefresh); const response = await callCoreRpc< Envelope<{ entries: CatalogEntry[] }> | { entries: CatalogEntry[] } - >({ method: 'openhuman.skill_registry_browse', params: { force_refresh: forceRefresh } }); + >({ + method: 'openhuman.skill_registry_browse', + params: { force_refresh: forceRefresh }, + timeoutMs: CATALOG_RPC_TIMEOUT_MS, + }); const result = unwrap(response); log('browse: count=%d', result.entries.length); return result.entries; @@ -85,6 +101,7 @@ export const skillRegistryApi = { >({ method: 'openhuman.skill_registry_search', params: { query, ...(source ? { source } : {}), ...(category ? { category } : {}) }, + timeoutMs: CATALOG_RPC_TIMEOUT_MS, }); const result = unwrap(response); log('search: count=%d', result.entries.length); @@ -95,6 +112,7 @@ export const skillRegistryApi = { log('sources: request'); const response = await callCoreRpc | { sources: string[] }>({ method: 'openhuman.skill_registry_sources', + timeoutMs: CATALOG_RPC_TIMEOUT_MS, }); const result = unwrap(response); log('sources: count=%d', result.sources.length); @@ -105,7 +123,7 @@ export const skillRegistryApi = { log('categories: request'); const response = await callCoreRpc< Envelope<{ categories: string[] }> | { categories: string[] } - >({ method: 'openhuman.skill_registry_categories' }); + >({ method: 'openhuman.skill_registry_categories', timeoutMs: CATALOG_RPC_TIMEOUT_MS }); const result = unwrap(response); log('categories: count=%d', result.categories.length); return result.categories; diff --git a/app/src/services/api/workflowsApi.ts b/app/src/services/api/workflowsApi.ts index 3ad392a81..371457795 100644 --- a/app/src/services/api/workflowsApi.ts +++ b/app/src/services/api/workflowsApi.ts @@ -243,12 +243,29 @@ function normalizeWorkflowSummary(raw: RawWorkflowSummary): WorkflowSummary { }; } +/** Options for {@link workflowsApi.listWorkflows}. */ +export interface ListWorkflowsOptions { + /** + * When `true`, also include capability skills under the `skills/` roots + * (registry installs land there), not just `workflows/`-root automations. + */ + includeSkills?: boolean; +} + export const workflowsApi = { - /** Enumerate SKILL.md / legacy skills visible in the active workspace. */ - listWorkflows: async (): Promise => { - log('listWorkflows: request'); + /** + * Enumerate SKILL.md / legacy skills visible in the active workspace. + * + * By default returns only `workflows/`-root automations (the Automations UI + * view). Pass `{ includeSkills: true }` to also include capability skills + * under the `skills/` roots — the Skills Explorer uses this so + * registry-installed skills show up in its Installed tab. + */ + listWorkflows: async (opts?: ListWorkflowsOptions): Promise => { + log('listWorkflows: request includeSkills=%s', opts?.includeSkills ?? false); const response = await callCoreRpc | WorkflowsListResult>({ method: 'openhuman.workflows_list', + params: opts?.includeSkills ? { include_skills: true } : undefined, }); const result = unwrapEnvelope(response); const workflows = (result?.workflows ?? []).map(normalizeWorkflowSummary); diff --git a/src/openhuman/workflows/ops_discover.rs b/src/openhuman/workflows/ops_discover.rs index 4ccdebe38..3d616d6a2 100644 --- a/src/openhuman/workflows/ops_discover.rs +++ b/src/openhuman/workflows/ops_discover.rs @@ -556,3 +556,60 @@ fn resolve_workflow_for_resource( (None, None) => Err(format!("skill '{skill_id}' not found")), } } + +#[cfg(test)] +mod include_skills_tests { + use super::*; + + /// Write a minimal ``-named bundle under `root/slug/`. + fn seed_bundle(root: &Path, slug: &str, file: &str) { + let dir = root.join(slug); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write( + dir.join(file), + format!("---\nname: {slug}\ndescription: {slug} desc\n---\n\n{slug} body\n"), + ) + .unwrap(); + } + + /// `discover_automations` lists only `workflows/`-root automations, while + /// `discover_workflows` additionally surfaces `skills/`-root installs. This + /// is exactly the branch `handle_workflows_list` selects on `include_skills` + /// so the Skills Explorer's Installed tab can show registry installs (#3954). + #[test] + fn automations_excludes_skill_roots_but_full_discover_includes_them() { + let home = tempfile::TempDir::new().unwrap(); + let home_path = home.path(); + // A registry-style install lands under `~/.openhuman/skills/`. + seed_bundle( + &home_path.join(".openhuman").join("skills"), + "installed-skill", + "SKILL.md", + ); + // A "New workflow" automation lands under `~/.openhuman/workflows/`. + seed_bundle( + &home_path.join(".openhuman").join("workflows"), + "my-automation", + "WORKFLOW.md", + ); + + // Automations-only view (the default `workflows_list` path) hides the skill. + let automations = discover_automations(Some(home_path), None, false); + let auto_names: Vec<&str> = automations.iter().map(|w| w.name.as_str()).collect(); + assert_eq!( + auto_names, + vec!["my-automation"], + "discover_automations must exclude `skills/`-root installs" + ); + + // Full view (`include_skills=true`) surfaces both. + let full = discover_workflows(Some(home_path), None, false); + let mut full_names: Vec<&str> = full.iter().map(|w| w.name.as_str()).collect(); + full_names.sort_unstable(); + assert_eq!( + full_names, + vec!["installed-skill", "my-automation"], + "discover_workflows must include `skills/`-root installs" + ); + } +} diff --git a/src/openhuman/workflows/registry.rs b/src/openhuman/workflows/registry.rs index 984588aca..91dac43a3 100644 --- a/src/openhuman/workflows/registry.rs +++ b/src/openhuman/workflows/registry.rs @@ -189,7 +189,21 @@ pub fn load_workflows(workspace_dir: &Path) -> Vec { let Some(dir) = skill_md.parent() else { continue; }; - if let Some(def) = load_workflow_definition(dir, &wf.name, &wf.description) { + // Build the runnable id from the on-disk slug (`dir_name`) so it matches + // the `WorkflowSummary.id` shown in lists, the id the orchestrator prompt + // tells the agent to run, and the slug uninstall resolves against — all + // of which key on `dir_name`. A SKILL.md-only install whose frontmatter + // `name` differs from its install slug (e.g. `name: My Cool Workflow` in + // `my-cool-workflow/`) would otherwise build `definition.id` from the + // name and be unresolvable by `workflows_describe` / `workflows_run` + // ("unknown skill"). Falls back to `name` for legacy `Workflow` values + // that predate `dir_name`. (#3987 codex review.) + let slug = if wf.dir_name.is_empty() { + wf.name.as_str() + } else { + wf.dir_name.as_str() + }; + if let Some(def) = load_workflow_definition(dir, slug, &wf.description) { workflows.push(def); } } @@ -341,6 +355,33 @@ mod tests { } } + #[test] + fn skill_md_only_install_resolves_by_dir_slug_not_frontmatter_name() { + // Regression (#3987 codex review): a SKILL.md-only install whose + // frontmatter `name` differs from its install slug must resolve via the + // dir slug — the id surfaced in the list summary / orchestrator prompt / + // uninstall — not the frontmatter name. Before the fix, `definition.id` + // was built from `wf.name` ("My Cool Workflow"), so `get_workflow` + // (keyed on the slug) returned None → "unknown skill". + let tmp = tempfile::TempDir::new().unwrap(); + let dir = tmp.path().join("skills").join("my-cool-workflow"); + std::fs::create_dir_all(&dir).unwrap(); + std::fs::write( + dir.join("SKILL.md"), + "---\nname: My Cool Workflow\ndescription: does cool things\n---\n\n# Body\n", + ) + .unwrap(); + + let resolved = get_workflow(tmp.path(), "my-cool-workflow") + .expect("SKILL.md-only install must resolve by its dir slug"); + assert_eq!(resolved.definition.id, "my-cool-workflow"); + // And NOT by the frontmatter name. + assert!( + get_workflow(tmp.path(), "My Cool Workflow").is_none(), + "frontmatter name must not be the runnable id" + ); + } + #[test] fn prune_removes_legacy_bundled_only() { let tmp = tempfile::TempDir::new().unwrap(); diff --git a/src/openhuman/workflows/schemas/controller_schemas.rs b/src/openhuman/workflows/schemas/controller_schemas.rs index 9f2ccf83d..85e882726 100644 --- a/src/openhuman/workflows/schemas/controller_schemas.rs +++ b/src/openhuman/workflows/schemas/controller_schemas.rs @@ -86,7 +86,12 @@ pub fn workflows_schemas(function: &str) -> ControllerSchema { namespace: "workflows", function: "list", description: "List SKILL.md and legacy skills discovered in the user home and workspace.", - inputs: vec![], + inputs: vec![FieldSchema { + name: "include_skills", + ty: TypeSchema::Bool, + comment: "When true, also include capability skills under the `skills/` roots (where registry installs land), not just `workflows/`-root automations. Defaults to false (automations-only view).", + required: false, + }], outputs: vec![FieldSchema { name: "skills", ty: TypeSchema::Array(Box::new(TypeSchema::Ref("WorkflowSummary"))), diff --git a/src/openhuman/workflows/schemas/handlers.rs b/src/openhuman/workflows/schemas/handlers.rs index 9da8473e6..afcfa9832 100644 --- a/src/openhuman/workflows/schemas/handlers.rs +++ b/src/openhuman/workflows/schemas/handlers.rs @@ -12,8 +12,9 @@ use serde_json::{Map, Value}; use crate::core::all::ControllerFuture; use crate::openhuman::skill_runtime::spawn_workflow_run_background; use crate::openhuman::workflows::ops::{ - create_workflow, discover_automations, install_workflow_from_url, is_workspace_trusted, - read_workflow_resource, uninstall_workflow, CreateWorkflowParams, UninstallWorkflowParams, + create_workflow, discover_automations, discover_workflows, install_workflow_from_url, + is_workspace_trusted, read_workflow_resource, uninstall_workflow, CreateWorkflowParams, + UninstallWorkflowParams, }; use crate::openhuman::workflows::{registry, run_log}; use crate::rpc::RpcOutcome; @@ -30,22 +31,31 @@ use super::wire_types::{ pub(super) fn handle_workflows_list(params: Map) -> ControllerFuture { Box::pin(async move { - let _ = deserialize_params::(params)?; - tracing::debug!("[workflows][rpc] list automations"); + let params = deserialize_params::(params)?; + let include_skills = params.include_skills; + tracing::debug!(include_skills, "[workflows][rpc] list automations"); let workspace = resolve_workspace_dir().await; let trusted = is_workspace_trusted(&workspace); let home = dirs::home_dir(); - // Automations list shows only `workflows/`-root task templates — not the - // capability skills under `skills/` roots, which the agent harness still - // loads via `discover_workflows` / `load_workflow_metadata`. - let automations = discover_automations(home.as_deref(), Some(workspace.as_path()), trusted); + // Default: automations-only (`workflows/` roots) so capability skills + // don't masquerade as task templates in the Automations UI. The Skills + // Explorer passes `include_skills=true` to also surface `skills/`-root + // installs (registry installs land there) in its Installed tab. Either + // way the agent harness loads both via `discover_workflows` / + // `load_workflow_metadata`. + let listed = if include_skills { + discover_workflows(home.as_deref(), Some(workspace.as_path()), trusted) + } else { + discover_automations(home.as_deref(), Some(workspace.as_path()), trusted) + }; tracing::debug!( - count = automations.len(), + count = listed.len(), + include_skills, workspace = %workspace.display(), trusted, "[workflows][rpc] list result" ); - let summaries = automations.into_iter().map(WorkflowSummary::from).collect(); + let summaries = listed.into_iter().map(WorkflowSummary::from).collect(); to_json(RpcOutcome::new( WorkflowsListResult { workflows: summaries, diff --git a/src/openhuman/workflows/schemas/wire_types.rs b/src/openhuman/workflows/schemas/wire_types.rs index 1c6f20bec..5cc868ef2 100644 --- a/src/openhuman/workflows/schemas/wire_types.rs +++ b/src/openhuman/workflows/schemas/wire_types.rs @@ -15,8 +15,14 @@ use crate::openhuman::workflows::ops::{ #[derive(Debug, Deserialize, Default)] pub(super) struct WorkflowsListParams { - // No params today. Kept as an empty struct so future filters (scope, - // search, etc.) can slot in without breaking older clients. + /// When `true`, also include capability skills (under the `skills/` roots) + /// in the listing — not just `workflows/`-root automations. The Skills + /// Explorer passes this so registry-installed skills (which land under + /// `~/.openhuman/skills/`) appear in its Installed tab and flip the catalog + /// Install button to Installed. Omitted (defaults `false`) by the + /// Automations UI, which keeps the automations-only view. + #[serde(default)] + pub(super) include_skills: bool, } #[derive(Debug, Deserialize)]