Add public skills + improve repo skill discovery and error UX #8098
Conversation
xl-openai
commented
Dec 16, 2025
xl-openai
commented
- Adds SkillScope::Public end-to-end (core + protocol) and loads skills from the public cache directory
- Improves repo skill discovery by searching upward for the nearest .codex/skills within a git repo
- Deduplicates skills by name with deterministic ordering to avoid duplicates across sources
- Fixes garbled “Skill errors” overlay rendering by preventing pending history lines from being injected during the modal
- Updates the project docs “Skills” intro wording to avoid hardcoded paths
codex-rs/core/src/skills/public.rs
Outdated
| Ok(()) | ||
| } | ||
|
|
||
| fn clone_repo_blocking(repo_url: &str, checkout_dir: &Path) -> anyhow::Result<()> { |
There was a problem hiding this comment.
We try not to use anyhow inside core and use specific enums for error types.
| .map(|entry| (entry.skills.clone(), entry.errors.clone())) | ||
| .unwrap_or_default(); | ||
|
|
||
| assert!( |
There was a problem hiding this comment.
Can we add tests for the new public skills feature?
codex-rs/core/src/skills/public.rs
Outdated
| } | ||
|
|
||
| let staged_skills = tmp_dir.join(SKILLS_DIR_NAME); | ||
| copy_dir_recursive(&src_skills, &staged_skills)?; |
There was a problem hiding this comment.
why are we making an intermediate copy and not moving the directory from the temp checkout directly to the target location?
codex-rs/core/src/skills/manager.rs
Outdated
| }); | ||
| } else { | ||
| let (sender, receiver) = mpsc::channel(); | ||
| std::thread::spawn(move || { |
There was a problem hiding this comment.
why are we spawning if we will block on it anyways? how about converting skills_for_cwd to async and just awaiting?
There was a problem hiding this comment.
Now I make refresh_public_skills_blocking unblocking and rely on event to re-sync later.
xl-openai
force-pushed
the
xl/skills
branch
2 times, most recently
from
22047ff to
895bce6
Compare
|
@codex review again |
![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".
codex-rs/core/src/skills/public.rs
Outdated
| // TODO: Temporary test repo. Revert to `https://github.com/openai/skills.git`. | ||
| const PUBLIC_SKILLS_REPO_URL: &str = "https://github.com/xl-openai/test.git"; |
There was a problem hiding this comment.
The public skills refresher is hardcoded to clone a temporary test repo, so in normal runs users will fetch the wrong skill set (or nothing if that repo disappears). This means public skill discovery will silently diverge from the intended production source until the constant is corrected or made configurable.
Useful? React with 👍 / 👎.
codex-rs/core/src/skills/manager.rs
Outdated
| if outcome.updated() | ||
| && let Ok(mut listeners) = skills_update_listeners.lock() | ||
| { | ||
| listeners.retain(|tx| { | ||
| tx.try_send(Event { |
There was a problem hiding this comment.
The refresh thread only notifies listeners present at the moment it finishes, and attempted_public_refresh prevents any retry. If the refresh completes before any listener registers (e.g., early startup), the cache remains populated with pre-refresh skills and later ListSkills calls will keep returning stale results unless a manual force_reload is sent. Consider recording a pending update or invalidating the cache when refresh completes so late listeners can trigger a reload.
Useful? React with 👍 / 👎.
xl-openai
force-pushed
the
xl/skills
branch
2 times, most recently
from
bfb03a7 to
4bd7739
Compare
codex-rs/core/tests/suite/skills.rs
Outdated
| let skill_dir = home | ||
| .join("skills") | ||
| .join(".public") | ||
| .join("skills") |
There was a problem hiding this comment.
Why skills twice? Namespacing public skills inside a .public folder makes sense. We can just put the skills inside that.
codex-rs/core/src/skills/manager.rs
Outdated
| codex_home, | ||
| cache_by_cwd: RwLock::new(HashMap::new()), | ||
| attempted_public_refresh: AtomicBool::new(false), | ||
| skills_update_listeners: Arc::new(Mutex::new(Vec::new())), |
There was a problem hiding this comment.
This vector of listeners feels complicated. We would also lost the operation id. Cannot we have the tokio spawn in core and use send_event?
@pakrym-oai what do you think?
