Support SYSTEM skills. #8220
Support SYSTEM skills. #8220
Conversation
xl-openai
commented
Dec 18, 2025
xl-openai
commented
- Remove PUBLIC skills and introduce SYSTEM skills embedded in the binary and installed into $CODEX_HOME/skills/.system at startup.
- Skills are now always enabled (feature flag removed).
- Update skills/list to accept forceReload and plumb it through (not used by clients yet).
| fs::create_dir_all(&skill_dir).unwrap(); | ||
| fs::write(skill_dir.join("SKILL.md"), "not yaml").unwrap(); | ||
| }); | ||
| let mut builder = test_codex().with_pre_build_hook(|home| { |
There was a problem hiding this comment.
Can we make a helper and have an integration test for system skills? Maybe we can also test the priorities (system, user, repo)
There was a problem hiding this comment.
There is a list_skills_includes_system_cache_entries one :)
codex-rs/core/src/skills/system.rs
Outdated
| use std::path::Path; | ||
| use std::path::PathBuf; | ||
| use std::time::SystemTime; | ||
| use std::time::UNIX_EPOCH; |
There was a problem hiding this comment.
let's use AbsolutePathBuf
codex-rs/core/src/skills/system.rs
Outdated
| Ok(()) | ||
| } | ||
|
|
||
| fn write_embedded_dir(dir: &Dir<'_>, dest: &Path) -> Result<(), SystemSkillsError> { |
There was a problem hiding this comment.
let's have docstrings for the helper functions here. a bit hard to digest
codex-rs/core/src/skills/system.rs
Outdated
| } | ||
|
|
||
| write_embedded_dir(&SYSTEM_SKILLS_DIR, &staged_system)?; | ||
| atomic_swap_dir(&staged_system, &dest_system, &skills_root_dir)?; |
There was a problem hiding this comment.
do we need that for writing few .md files?
| impl SkillsManager { | ||
| pub fn new(codex_home: PathBuf) -> Self { | ||
| let (skills_update_tx, _skills_update_rx) = broadcast::channel(1); | ||
| if let Err(err) = install_system_skills(&codex_home) { |
There was a problem hiding this comment.
not blocking: we can leave a marker of the cli version or skills hash so we don't repeat this on startup every time. but I think it's not needed now it's few files.
| @@ -0,0 +1,95 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
this file and the above file both have the same name but the content is different though.
xl-openai
force-pushed
the
xl/skills
branch
2 times, most recently
from
081956f to
16b6725
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let mut items: Vec<(String, Option<u64>)> = SYSTEM_SKILLS_DIR | ||
| .entries() | ||
| .iter() | ||
| .map(|entry| match entry { | ||
| include_dir::DirEntry::Dir(dir) => (dir.path().to_string_lossy().to_string(), None), |
There was a problem hiding this comment.
The fingerprint used to decide whether to reinstall embedded system skills only iterates SYSTEM_SKILLS_DIR.entries() and hashes directory entries as (path, None). The embedded skills live under subdirectories (e.g., plan/ and skill-creator/), so changes to their SKILL.md contents won’t affect the fingerprint. That means an upgraded binary with updated system skill content will keep the stale on-disk cache because the marker still matches, and install_system_skills will early-return. Consider hashing files recursively (e.g., using Dir::files() or recursing like write_embedded_dir) so any content changes update the marker and trigger a refresh.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
It is OK for now.
