Skip to content

fix(skills): union configured skills_dir with workspace skills instead of or_else fallback #2737

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
h3c-hexin wants to merge 2 commits into from
+43 −45
Closed

fix(skills): union configured skills_dir with workspace skills instead of or_else fallback #2737

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
38f6040
fix(skills): union configured skills_dir with workspace skills instea…
Jun 4, 2026
f93be56
fix(skills): remove dead render_available_skills_context after union …
Jun 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions crates/tui/src/prompts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1068,13 +1068,15 @@ pub fn system_prompt_for_mode_with_context_skills_session_and_approval(
// skills directory (`.agents/skills`, `skills`,
// `.opencode/skills`, `.claude/skills`, `.cursor/skills`) plus global
// `~/.agents/skills` / `~/.deepseek/skills` so skills installed for any
// AI-tool convention show up in the catalogue. The legacy
// single-`skills_dir` path is
// honoured as a fallback for callers that don't supply a
// workspace-aware view; it falls through to the same merged
// registry when available.
let skills_block = crate::skills::render_available_skills_context_for_workspace(workspace)
.or_else(|| skills_dir.and_then(crate::skills::render_available_skills_context));
// AI-tool convention show up in the catalogue.
// When an explicit `skills_dir` is configured, union it with the
// workspace view rather than treating it as an `or_else` fallback: the
// workspace view almost always returns Some (any cross-tool skill folder),
// which would otherwise shadow the configured `skills_dir` entirely.
let skills_block = skills_dir.map_or_else(
|| crate::skills::render_available_skills_context_for_workspace(workspace),
|dir| crate::skills::render_available_skills_context_for_workspace_and_dir(workspace, dir),
);
if let Some(block) = skills_block {
full_prompt = format!("{full_prompt}\n\n{block}");
}
Expand Down
72 changes: 34 additions & 38 deletions crates/tui/src/skills/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -619,19 +619,25 @@ pub fn render_available_skills_context_for_workspace(workspace: &Path) -> Option
render_skills_block(&registry)
}

/// Codex's progressive-disclosure contract: the model sees skill names,
/// descriptions, and paths up front, then opens the specific `SKILL.md` only
/// when a skill is relevant.
///
/// Single-directory variant — use
/// [`render_available_skills_context_for_workspace`] when scanning
/// a workspace for cross-tool skill folders (#432).
/// Union variant: merge skills discovered in the `workspace` (cross-tool skill
/// folders) **and** an explicitly-configured `skills_dir`. Use this when an
/// embedder injects a dedicated `skills_dir` that must stay visible even when
/// the workspace already contains skills of its own — a plain `or_else` between
/// the workspace view and the dir view shadows the configured `skills_dir`
/// whenever `discover_in_workspace` returns any skill, so embedder-provided
/// skills silently disappear.
#[must_use]
pub fn render_available_skills_context(skills_dir: &Path) -> Option<String> {
let registry = SkillRegistry::discover(skills_dir);
pub fn render_available_skills_context_for_workspace_and_dir(
workspace: &Path,
skills_dir: &Path,
) -> Option<String> {
let registry = discover_for_workspace_and_dir(workspace, skills_dir);
render_skills_block(&registry)
}

/// Codex's progressive-disclosure contract: the model sees skill names,
/// descriptions, and paths up front, then opens the specific `SKILL.md` only
/// when a skill is relevant.
fn render_skills_block(registry: &SkillRegistry) -> Option<String> {
if registry.is_empty() {
return None;
Expand Down Expand Up @@ -760,6 +766,14 @@ mod tests {
std::fs::write(skill_dir.join("SKILL.md"), skill_content).unwrap();
}

/// Single-directory render used by the rendering-behaviour tests below.
/// Mirrors the retired `render_available_skills_context` entry point —
/// prompts.rs now goes through the workspace/union variants, which share
/// this same discover + render path.
fn render_dir_context(skills_dir: &std::path::Path) -> Option<String> {
super::render_skills_block(&super::SkillRegistry::discover(skills_dir))
}

#[test]
fn render_available_skills_context_lists_paths_and_usage() {
let tmpdir = TempDir::new().unwrap();
Expand All @@ -769,9 +783,7 @@ mod tests {
"---\nname: test-skill\ndescription: A test skill\n---\nDo something special",
);

let rendered =
crate::skills::render_available_skills_context(&tmpdir.path().join("skills"))
.expect("skill context");
let rendered = render_dir_context(&tmpdir.path().join("skills")).expect("skill context");

let expected_path = tmpdir
.path()
Expand Down Expand Up @@ -804,9 +816,7 @@ mod tests {
"---\nname: friendly-name\ndescription: drift case\n---\nbody",
);

let rendered =
crate::skills::render_available_skills_context(&tmpdir.path().join("skills"))
.expect("skill context");
let rendered = render_dir_context(&tmpdir.path().join("skills")).expect("skill context");

let real_path = tmpdir
.path()
Expand Down Expand Up @@ -838,10 +848,10 @@ mod tests {
let tmpdir = TempDir::new().unwrap();
let empty = tmpdir.path().join("skills");
std::fs::create_dir_all(&empty).unwrap();
assert!(crate::skills::render_available_skills_context(&empty).is_none());
assert!(render_dir_context(&empty).is_none());

let missing = tmpdir.path().join("does-not-exist");
assert!(crate::skills::render_available_skills_context(&missing).is_none());
assert!(render_dir_context(&missing).is_none());
}

#[test]
Expand All @@ -851,9 +861,7 @@ mod tests {
let body = format!("---\nname: bigdesc\ndescription: {long_desc}\n---\nbody");
create_skill_dir(&tmpdir, "bigdesc", &body);

let rendered =
crate::skills::render_available_skills_context(&tmpdir.path().join("skills"))
.expect("skill context");
let rendered = render_dir_context(&tmpdir.path().join("skills")).expect("skill context");

let max = super::MAX_SKILL_DESCRIPTION_CHARS;
assert!(rendered.contains('…'), "expected truncation marker");
Expand All @@ -872,9 +880,7 @@ mod tests {
"---\nname: spaced-skill\ndescription: alpha \t beta gamma\n---\nbody",
);

let rendered =
crate::skills::render_available_skills_context(&tmpdir.path().join("skills"))
.expect("skill context");
let rendered = render_dir_context(&tmpdir.path().join("skills")).expect("skill context");

let line = rendered
.lines()
Expand All @@ -892,9 +898,7 @@ mod tests {
create_skill_dir(&tmpdir, &format!("skill-{i:03}"), &body);
}

let rendered =
crate::skills::render_available_skills_context(&tmpdir.path().join("skills"))
.expect("skill context");
let rendered = render_dir_context(&tmpdir.path().join("skills")).expect("skill context");

assert!(
rendered.contains("additional skills omitted from this prompt budget"),
Expand Down Expand Up @@ -1435,9 +1439,7 @@ mod tests {
"folded-skill",
"---\nname: folded-skill\ndescription: >\n line one chinese\n line two chinese\n---\nbody",
);
let rendered =
crate::skills::render_available_skills_context(&tmpdir.path().join("skills"))
.expect("skill context");
let rendered = render_dir_context(&tmpdir.path().join("skills")).expect("skill context");
assert!(
rendered.contains("line one chinese line two chinese"),
"folded block scalar should join lines with space, got:\n{rendered}"
Expand All @@ -1454,9 +1456,7 @@ mod tests {
"literal-skill",
"---\nname: literal-skill\ndescription: |\n line one\n line two\n---\nbody",
);
let rendered =
crate::skills::render_available_skills_context(&tmpdir.path().join("skills"))
.expect("skill context");
let rendered = render_dir_context(&tmpdir.path().join("skills")).expect("skill context");
// `truncate_for_prompt` collapses whitespace, so the newlines
// become spaces. The key assertion is that the content is
// captured (not just `|`).
Expand All @@ -1476,9 +1476,7 @@ mod tests {
"strip-skill",
"---\nname: strip-skill\ndescription: >-\n alpha\n beta\n\n---\nbody",
);
let rendered =
crate::skills::render_available_skills_context(&tmpdir.path().join("skills"))
.expect("skill context");
let rendered = render_dir_context(&tmpdir.path().join("skills")).expect("skill context");
assert!(
rendered.contains("alpha beta"),
"strip-chomped folded block should join lines, got:\n{rendered}"
Expand All @@ -1495,9 +1493,7 @@ mod tests {
"plain-skill",
"---\nname: plain-skill\ndescription: A simple description\n---\nbody",
);
let rendered =
crate::skills::render_available_skills_context(&tmpdir.path().join("skills"))
.expect("skill context");
let rendered = render_dir_context(&tmpdir.path().join("skills")).expect("skill context");
assert!(
rendered.contains("- plain-skill: A simple description"),
"single-line description should still work, got:\n{rendered}"
Expand Down
Loading