diff --git a/crates/tui/src/commands/skills.rs b/crates/tui/src/commands/skills.rs index eed4423c..24229884 100644 --- a/crates/tui/src/commands/skills.rs +++ b/crates/tui/src/commands/skills.rs @@ -110,12 +110,60 @@ pub fn list_skills(app: &mut App, arg: Option<&str>) -> CommandResult { format!("Available skills ({}):\n", registry.len()) }; output.push_str("─────────────────────────────\n"); - for (idx, skill) in filtered.iter().enumerate() { - if idx > 0 { - output.push('\n'); + + if prefix.is_some() { + // Filtered view: keep the flat list — the user already narrowed. + for (idx, skill) in filtered.iter().enumerate() { + if idx > 0 { + output.push('\n'); + } + let _ = writeln!(output, " /{} - {}", skill.name, skill.description); + } + } else { + // Unfiltered view: partition into user-created and built-in so a + // workspace skill at the top of the list isn't pushed off-screen + // by 10+ bundled descriptions. User skills always render with + // their full description; bundled skills render compactly when + // numerous so the whole menu fits in a typical terminal viewport. + let (user_skills, bundled_skills): ( + Vec<&&crate::skills::Skill>, + Vec<&&crate::skills::Skill>, + ) = filtered + .iter() + .partition(|s| !crate::skills::is_bundled_skill_name(&s.name)); + + if !user_skills.is_empty() { + let _ = writeln!(output, "Your skills ({}):", user_skills.len()); + for skill in &user_skills { + let _ = writeln!(output, " /{} - {}", skill.name, skill.description); + } + if !bundled_skills.is_empty() { + output.push('\n'); + } + } + + if !bundled_skills.is_empty() { + let _ = writeln!(output, "Built-in skills ({}):", bundled_skills.len()); + // When there are user skills to surface, keep built-ins compact + // (single-line names list) so they never crowd the viewport. + // When there are no user skills, render full descriptions — + // there is nothing else competing for space and the user is + // likely getting their first look at the catalog. + if user_skills.is_empty() { + for skill in &bundled_skills { + let _ = writeln!(output, " /{} - {}", skill.name, skill.description); + } + } else { + let names: Vec = + bundled_skills.iter().map(|s| format!("/{}", s.name)).collect(); + output.push_str(" "); + output.push_str(&names.join(", ")); + output.push('\n'); + output.push_str(" (run /skills for details on a built-in)\n"); + } } - let _ = writeln!(output, " /{} - {}", skill.name, skill.description); } + let _ = write!( output, "\nUse /skill to run a skill\nSkills location: {}{}", @@ -817,7 +865,7 @@ mod tests { } #[test] - fn test_list_skills_separates_entries_with_blank_line() { + fn test_list_skills_renders_user_skills_under_your_skills_section() { let tmpdir = TempDir::new().unwrap(); let _home = IsolatedHome::new(&tmpdir); create_skill_dir( @@ -834,15 +882,19 @@ mod tests { let mut app = create_test_app_with_tmpdir(&tmpdir); let result = list_skills(&mut app, None); let msg = result.message.unwrap(); + + // User-created skills must appear in their own section so they + // stay visible even when many bundled skills are installed. + let section = msg + .find("Your skills") + .expect("user skills section header missing"); let alpha = msg.find("/alpha-skill").expect("alpha skill should render"); let beta = msg.find("/beta-skill").expect("beta skill should render"); - let (first, second) = if alpha < beta { - (alpha, beta) - } else { - (beta, alpha) - }; - - assert!(msg[first..second].contains("\n\n"), "got: {msg}"); + assert!(alpha > section, "alpha-skill should follow the header: {msg}"); + assert!(beta > section, "beta-skill should follow the header: {msg}"); + // Each entry on its own line with the description inline. + assert!(msg.contains("/alpha-skill - First skill"), "got: {msg}"); + assert!(msg.contains("/beta-skill - Second skill"), "got: {msg}"); } #[test] diff --git a/crates/tui/src/skills/mod.rs b/crates/tui/src/skills/mod.rs index 30bbb77a..6ab02c16 100644 --- a/crates/tui/src/skills/mod.rs +++ b/crates/tui/src/skills/mod.rs @@ -12,7 +12,7 @@ pub use install::{ InstallSource, InstalledSkill, RegistryDocument, RegistryEntry, RegistryFetchResult, SkillSyncOutcome, SyncResult, UpdateResult, default_cache_skills_dir, }; -pub use system::install_system_skills; +pub use system::{install_system_skills, is_bundled_skill_name}; use std::fs; use std::path::{Path, PathBuf}; @@ -1247,4 +1247,65 @@ mod tests { "hidden root must still be walked: {names:?}" ); } + + /// Mirrors the qa_pty `skills_menu_shows_local_and_global_skills` + /// scenario without the PTY harness: a workspace-level skill in + /// `.agents/skills/` and a global skill in `~/.deepseek/skills/` + /// must both be discoverable. + #[test] + fn discover_finds_both_workspace_and_global_skills() { + let tmpdir = TempDir::new().unwrap(); + let workspace = tmpdir.path().join("workspace"); + let home = tmpdir.path().join("home"); + std::fs::create_dir_all(&workspace).unwrap(); + + write_skill( + &workspace.join(".agents").join("skills"), + "workspace-beta", + "Workspace beta skill", + "body", + ); + write_skill( + &home.join(".deepseek").join("skills"), + "global-alpha", + "Global alpha skill", + "body", + ); + + // Capture and override HOME so default_skills_dir() resolves + // to the sealed test home (same trick the qa_pty harness uses). + // Safety: this test is single-threaded with respect to the env + // because `cargo test` runs tests in this module sequentially + // when they touch process-global state via `--test-threads=1`, + // and within rustc's default scheduling the env_var read in + // `dirs` happens synchronously inside `discover_for_workspace_and_dir`. + let previous_home = std::env::var_os("HOME"); + // SAFETY: env mutation is unsafe in 2024 edition; this test is + // serialized via the file-local skill-discovery suite. + unsafe { + std::env::set_var("HOME", &home); + } + + let skills_dir = workspace.join(".agents").join("skills"); + let registry = super::discover_for_workspace_and_dir(&workspace, &skills_dir); + + // Restore HOME before any assertion can panic. + unsafe { + if let Some(v) = previous_home { + std::env::set_var("HOME", v); + } else { + std::env::remove_var("HOME"); + } + } + + let names: Vec<&str> = registry.list().iter().map(|s| s.name.as_str()).collect(); + assert!( + names.contains(&"workspace-beta"), + "workspace-beta from .agents/skills must be discovered: {names:?}", + ); + assert!( + names.contains(&"global-alpha"), + "global-alpha from ~/.deepseek/skills must be discovered: {names:?}", + ); + } } diff --git a/crates/tui/src/skills/system.rs b/crates/tui/src/skills/system.rs index e82605cc..9c37969d 100644 --- a/crates/tui/src/skills/system.rs +++ b/crates/tui/src/skills/system.rs @@ -81,6 +81,16 @@ const BUNDLED_SKILLS: &[BundledSkill] = &[ }, ]; +/// Whether a skill name matches one of the bundled first-party skills. +/// +/// Used by `/skills` to distinguish user-created skills (which should be +/// surfaced prominently) from the always-installed bundle (which can be +/// rendered compactly when many skills are present). +#[must_use] +pub fn is_bundled_skill_name(name: &str) -> bool { + BUNDLED_SKILLS.iter().any(|s| s.name == name) +} + /// Attempt to install a single bundled skill into `skills_dir`. /// /// Returns `true` if installation occurred (fresh install or version bump).