fix: always use cache_by_config for skills cache.#15017
fix: always use cache_by_config for skills cache.#15017
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: 1b0149b686
ℹ️ 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".
| roots.retain(|root| root.scope != SkillScope::System); | ||
| } | ||
| roots.extend( | ||
| let cache_key = config_skills_cache_key(&resolved.roots, &resolved.config_layer_stack); |
There was a problem hiding this comment.
skills_for_cwd_with_extra_user_roots builds cache_key from resolved.roots before appending extra_user_roots, then stores the loaded result under that key. A force-reload call with extra roots can therefore poison the config cache so later skills_for_cwd(..., false) or skills_for_config(...) (without extras) returns unexpected extra skills until cache clear/reload.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
That's how it was designed today. We rely on the cache to register extra_user_roots
jif-oai
left a comment
jif-oai
left a comment
There was a problem hiding this comment.
For my change, this is good. But we need to change some ordering then. What we have right now gives something like
- Call
skills_for_cwd_with_extra_user_roots(/repo, true, [/tmp/extra]) - Cache stores result containing extra-skill
- Later call skills_for_cwd(/repo, false)
- It reuses that same config key and incorrectly returns extra-skill
3 participants
skills/list with extra_user_roots writes results into cached_by_cwd. But after #14806 introduced cached_by_config, session injection started reading from cached_by_config instead. As a result, extra_user_roots is no longer respected.
This PR deprecates cached_by_cwd and moves everything to cached_by_config.