[skills] clear all caches on forced reload#29974
Conversation
|
I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
![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: 335a21340d
ℹ️ 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 skills_service = self.thread_manager.skills_service(); | ||
| let plugins_manager = self.thread_manager.plugins_manager(); | ||
| if force_reload { | ||
| skills_service.clear_cache(); |
There was a problem hiding this comment.
forceReload now drops only SkillsService caches. For plugin-provided skills, a thread that has already warmed plugins_manager.plugins_for_config() keeps parsed PluginSkillSnapshots in the plugin manager cache; after a plugin skill is edited, skills/list can rescan it via the uncached layer-stack path, but the next turn rebuilds its skills snapshot from those stale plugin snapshots, leaving the model context with the old plugin skill despite the forced reload. Clear the plugin manager cache here as well before clearing the skills cache.
Useful? React with 👍 / 👎.
| &server.uri(), | ||
| )?; | ||
|
|
||
| let mut mcp = TestAppServer::new(codex_home.path()).await?; |
There was a problem hiding this comment.
In remote app/exec integration runs, starting the app server this way and then using a host TempDir as thread/start.cwd leaves the test bound to the app host instead of the executor fixture, so the new turn-based regression can fail or miss the foreign-OS path. Use TestAppServer::new_with_auto_env() together with send_thread_start_request_with_auto_env() for this test.
AGENTS.md reference: AGENTS.md:L252-L258
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 335a21340d
ℹ️ 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".
| &server.uri(), | ||
| )?; | ||
|
|
||
| let mut mcp = TestAppServer::new(codex_home.path()).await?; |
There was a problem hiding this comment.
In foreign app/exec test runs, TestAppServer::new can inherit a remote executor while the test starts the thread with a host TempDir cwd, so the turn may resolve its primary environment against a path that the executor cannot see before this regression actually verifies cache invalidation. Use the auto-env helper and corresponding thread-start helper (or otherwise select the target-native test cwd) for this app-server integration test.
AGENTS.md reference: AGENTS.md:L256-L258
Useful? React with 👍 / 👎.
| let skills_service = self.thread_manager.skills_service(); | ||
| let plugins_manager = self.thread_manager.plugins_manager(); | ||
| if force_reload { | ||
| skills_service.clear_cache(); |
There was a problem hiding this comment.
When plugins are enabled, clearing only SkillsService still leaves PluginsManager's loaded plugin cache and its PluginSkillSnapshots intact. If a thread has already warmed plugins and then a plugin skill is added or edited, skills/list with forceReload can display the fresh disk scan, but the next turn's snapshot_for_config can be rebuilt from stale plugin snapshots, so the model still sees the old plugin skill set; clear the plugin cache along with the skills cache for this forced app-server API reload.
AGENTS.md reference: AGENTS.md:L102-L106
Useful? React with 👍 / 👎.
| .await; | ||
| let skills_service = self.thread_manager.skills_service(); | ||
| let plugins_manager = self.thread_manager.plugins_manager(); | ||
| if force_reload { |
There was a problem hiding this comment.
@codex Doesn't snapshot_for_cwd handle force_reload and reset the cache already?
There was a problem hiding this comment.
Summary
- Added plugin-manager cache invalidation for
skills/listforceReload, with an inline note explaining thatsnapshot_for_cwdonly bypasses the cwd cache while turns also depend on config-keyed skills and cached plugin skill snapshots. codex-rs/app-server/src/request_processors/catalog_processor.rsL494-L500 - Updated the existing forced-reload turn regression to use
TestAppServer::new_with_auto_env()andsend_thread_start_request_with_auto_env(), avoiding host-only cwd assumptions in remote app/exec runs. codex-rs/app-server/tests/suite/v2/skills_list.rsL713-L746 - Added a plugin-skill regression that edits a cached plugin skill, calls
skills/listwithforce_reload: true, and verifies the next turn sees the updated plugin skill snapshot. codex-rs/app-server/tests/suite/v2/skills_list.rsL801-L891 - Refactored the local curated plugin skill fixture helper so tests can write different plugin skill descriptions. codex-rs/app-server/tests/suite/v2/skills_list.rsL164-L185
Testing
- ✅
cargo fmt --all - ✅
cargo fmt --all -- --check - ✅
git diff --check ⚠️ just fmt(warning: environment lackeddotslash, and Python formatter dependency downloads were blocked by proxy/network 403)⚠️ just test -p codex-app-server --test all skills_list_force_reload_clears -- --nocapture(warning:cargo-nextestwas not installed)⚠️ cargo install cargo-nextest --locked(warning: crates.io access was blocked by proxy/network 403)⚠️ cargo check -p codex-app-server --test all(warning:rusty_v8archive download from GitHub was blocked by proxy/network 403)
Committed changes on the current branch and opened the follow-up PR: [skills] clear plugin snapshots on forced reload.
2 participants
Summary
skills/listis called withforceReload, so turn-facing config-keyed snapshots are invalidated along with cwd snapshots.Validation
cargo fmt --checkcargo test -p codex-app-server --test all skills_list_force_reload_clears_turn_skills_cache -- --nocaptureThe repository-prescribed
justwrapper was unavailable in this environment, so the equivalent Cargo commands were run directly.