fix(skills): partition /skills output so user-created skills stay visible
The /skills menu renders one history cell containing every discoverable skill with full description. With the v0.8.34 bundled-skill set (11 first-party skills), a typical 40-row terminal viewport no longer fits the message, and the top of the list scrolls off — including any workspace-level user skill at .agents/skills/, which the registry returns first thanks to precedence ordering. The qa_pty `skills_menu_shows_local_and_global_skills` test exercises exactly this case: a workspace skill plus a sealed-home global skill plus the 11 bundled skills come to 13 entries, the viewport cuts off above /global-alpha, and the workspace skill is invisible. Discovery was healthy — confirmed by the new unit test `discover_finds_both_workspace_and_global_skills`. Render the menu in two sections so user-created skills stay prominent: - `Your skills (N):` lists every user-installed skill with its full description. - `Built-in skills (M):` either lists every bundled skill with its description (when there are no user skills to surface) or compacts them into a comma-separated name list with a "run /skills <name> for details" hint (when user skills are present). Filtered views (`/skills <prefix>`) keep the old flat list — the user has already narrowed the catalog and section headers would be noise. Expose `skills::is_bundled_skill_name` so the renderer can partition without a side-channel marker. Replace one unit test that asserted the old between-entry blank-line spacing with one that asserts the section structure.
This commit is contained in:
Hunter Bown
@@ -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<String> =
|
||||
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 <name> for details on a built-in)\n");
|
||||
}
|
||||
}
|
||||
let _ = writeln!(output, " /{} - {}", skill.name, skill.description);
|
||||
}
|
||||
|
||||
let _ = write!(
|
||||
output,
|
||||
"\nUse /skill <name> 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]
|
||||
|
||||
@@ -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:?}",
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -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).
|
||||
|
||||
Reference in New Issue
Block a user