feat: extend skills/list to support additional roots.#10835
feat: extend skills/list to support additional roots.#10835
Conversation
![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.
Reviewed commit: 1076fdb2f5
ℹ️ 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".
| pub async fn skills_for_cwd(&self, cwd: &Path, force_reload: bool) -> SkillLoadOutcome { | ||
| if !force_reload && let Some(entry) = self.cached_entry_for_cwd(cwd) { | ||
| return entry.outcome; | ||
| } |
There was a problem hiding this comment.
Because skills_for_cwd returns whatever cached entry exists for the cwd without checking that it was computed with no extra roots, any earlier call to skills_for_cwd_with_extra_user_roots will “poison” the cache. In that case a subsequent skills_for_cwd(cwd, false) (used elsewhere for normal skill listing) will unexpectedly include skills from those extra user roots, even though no extra roots were requested. This makes the extra-root behavior persist beyond the request that opted in and can surface unintended skills to other callers.
Useful? React with 👍 / 👎.
xl-openai
force-pushed
the
xl/skill-list
branch
2 times, most recently
from
a15ed4f to
9dc0be5
Compare
xl-openai
force-pushed
the
xl/skill-list
branch
from
9dc0be5 to
8dc0fa5
Compare
owenlin0
left a comment
owenlin0
left a comment
There was a problem hiding this comment.
pre-approving with small comments
|
|
||
| /// Optional per-cwd extra roots to scan as user-scoped skills. | ||
| #[serde(default, skip_serializing_if = "Vec::is_empty")] | ||
| pub per_cwd_extra_user_roots: Vec<SkillsListExtraRootsForCwd>, |
There was a problem hiding this comment.
nit: let's do this instead for consistency (should be covered by AGENTS.md now)
#[ts(optional = nullable)]
pub per_cwd_extra_user_roots: Option<Vec<SkillsListExtraRootsForCwd>>,
There was a problem hiding this comment.
my goal is to keep updating AGENTS.md so codex just applies consistent conventions by default🤞
| #[ts(export_to = "v2/")] | ||
| pub struct SkillsListExtraRootsForCwd { | ||
| pub cwd: PathBuf, | ||
| #[serde(default, skip_serializing_if = "Vec::is_empty")] |
2 participants
Add an optional perCwdExtraUserRoots