Use ConfigLayerStack for skills discovery. #8497
Conversation
xl-openai
force-pushed
the
xl/skills
branch
4 times, most recently
from
c31928f to
fc55aa5
Compare
| outcome | ||
| .skills | ||
| .sort_by(|a, b| a.name.cmp(&b.name).then_with(|| a.path.cmp(&b.path))); | ||
| fn scope_rank(scope: SkillScope) -> u8 { |
There was a problem hiding this comment.
If this internal only, it's fine, but if you want some breathing room, consider at least i16?
There was a problem hiding this comment.
The set of scopes should be small and relatively stable.
| outcome | ||
| } | ||
| outcome.skills.sort_by(|a, b| { | ||
| scope_rank(a.scope) |
There was a problem hiding this comment.
Consider defining PartialOrd or Ord on SkillMetadata?
There was a problem hiding this comment.
This ordering is only used locally in the loader to make the output deterministic, so keeping the scope_rank + sort_by here is the simplest and most straightforward approach.
codex-rs/core/src/skills/loader.rs
Outdated
|
|
||
| let system_folder = tmp.path().join("etc/codex"); | ||
| let user_folder = tmp.path().join("home/codex"); | ||
| fs::create_dir_all(&system_folder).unwrap(); |
There was a problem hiding this comment.
up to you, but you can change the return type to anyhow::Result<()> and then you can use ? instead of unwrap() throughout, but have to add Ok(() at the end.
codex-rs/core/src/skills/loader.rs
Outdated
| async fn deduplicates_by_name_preferring_nearest_project_codex_dir() { | ||
| let codex_home = tempfile::tempdir().expect("tempdir"); | ||
| let repo_dir = tempfile::tempdir().expect("tempdir"); | ||
| let status = Command::new("git") |
There was a problem hiding this comment.
Note you can use a marker file or just write .git with the contents "gitdir: fake" or something.
codex-rs/core/src/skills/loader.rs
Outdated
| assert!( | ||
| outcome.errors.is_empty(), | ||
| "unexpected errors: {:?}", | ||
| outcome.errors | ||
| ); | ||
| assert_eq!(outcome.skills.len(), 1); | ||
| assert_eq!(outcome.skills[0].name, "dupe-skill"); | ||
| assert_eq!(outcome.skills[0].description, "from nested"); | ||
| assert_eq!(outcome.skills[0].scope, SkillScope::Repo); |
There was a problem hiding this comment.
Prefer:
assert_eq!(
vec!(
SkillMetadata {
// ...
}
),
outcome.skills,
)test failures are far more informative when you do it this way.
Use ConfigLayerStack to get all folders while loading skills.