Support symlink for skills discovery. #8801
Conversation
|
Please make sure that this also works with the IDE extension. We should try to keep the behavior the same across surfaces. |
Allegedly |
|
Oh, I may have misunderstood what that constant is... |
| const MAX_NAME_LEN: usize = 64; | ||
| const MAX_DESCRIPTION_LEN: usize = 1024; | ||
| const MAX_SHORT_DESCRIPTION_LEN: usize = MAX_DESCRIPTION_LEN; | ||
| const MAX_SCAN_DEPTH: usize = 6; |
There was a problem hiding this comment.
Please add a docstring here because I think "depth" means two things: directory depth and number of symlink traversals?
codex-rs/core/src/skills/loader.rs
Outdated
| }; | ||
|
|
||
| if metadata.is_dir() { | ||
| let Ok(resolved_dir) = normalize_path(&path) else { |
There was a problem hiding this comment.
Path normalization and canonicalization are different. I see we do:
use dunce::canonicalize as normalize_path;
why not import it as canonicalize_path instead?
|
@codex review again |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Skills discovery now follows symlink entries for SkillScope::User ($CODEX_HOME/skills), SkillScope::Admin (e.g. /etc/codex/skills), and SKillScope::Repo
Added cycle protection: directories are canonicalized and tracked in a visited set to prevent infinite traversal from circular links.
Added per-root traversal limits to avoid accidentally scanning huge trees:
For now, symlink stat failures and traversal truncation are logged rather than surfaced as UI “invalid SKILL.md” warnings.