-
Notifications
You must be signed in to change notification settings - Fork 13.9k
[skills] clear all caches on forced reload #29974
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -493,6 +493,9 @@ impl CatalogRequestProcessor { | |
| .await; | ||
| let skills_service = self.thread_manager.skills_service(); | ||
| let plugins_manager = self.thread_manager.plugins_manager(); | ||
| if force_reload { | ||
| skills_service.clear_cache(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When plugins are enabled, clearing only AGENTS.md reference: AGENTS.md:L102-L106 Useful? React with 👍 / 👎. |
||
| } | ||
| let fs = self | ||
| .thread_manager | ||
| .environment_manager() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,10 @@ use codex_app_server_protocol::SkillsExtraRootsSetResponse; | |
| use codex_app_server_protocol::SkillsListParams; | ||
| use codex_app_server_protocol::SkillsListResponse; | ||
| use codex_app_server_protocol::ThreadStartParams; | ||
| use codex_app_server_protocol::ThreadStartResponse; | ||
| use codex_app_server_protocol::TurnStartParams; | ||
| use codex_app_server_protocol::TurnStartResponse; | ||
| use codex_app_server_protocol::UserInput as V2UserInput; | ||
| use codex_config::types::AuthCredentialsStoreMode; | ||
| use codex_exec_server::CODEX_EXEC_SERVER_URL_ENV_VAR; | ||
| use codex_utils_absolute_path::AbsolutePathBuf; | ||
|
|
@@ -699,6 +703,98 @@ async fn skills_list_uses_cached_result_until_force_reload() -> Result<()> { | |
| Ok(()) | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn skills_list_force_reload_clears_turn_skills_cache() -> Result<()> { | ||
| let server = create_mock_responses_server_repeating_assistant("Done").await; | ||
| let codex_home = TempDir::new()?; | ||
| let cwd = TempDir::new()?; | ||
| write_mock_responses_config_toml_with_chatgpt_base_url( | ||
| codex_home.path(), | ||
| &server.uri(), | ||
| &server.uri(), | ||
| )?; | ||
|
|
||
| let mut mcp = TestAppServer::new(codex_home.path()).await?; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In remote app/exec integration runs, starting the app server this way and then using a host AGENTS.md reference: AGENTS.md:L252-L258 Useful? React with 👍 / 👎.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In foreign app/exec test runs, AGENTS.md reference: AGENTS.md:L256-L258 Useful? React with 👍 / 👎. |
||
| timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??; | ||
|
|
||
| let thread_request_id = mcp | ||
| .send_thread_start_request(ThreadStartParams { | ||
| cwd: Some(cwd.path().to_string_lossy().into_owned()), | ||
| ..Default::default() | ||
| }) | ||
| .await?; | ||
| let thread_response: JSONRPCResponse = timeout( | ||
| DEFAULT_TIMEOUT, | ||
| mcp.read_stream_until_response_message(RequestId::Integer(thread_request_id)), | ||
| ) | ||
| .await??; | ||
| let ThreadStartResponse { thread, .. } = to_response(thread_response)?; | ||
|
|
||
| let skill_dir = codex_home.path().join("skills/late-extra-skill"); | ||
| std::fs::create_dir_all(&skill_dir)?; | ||
| std::fs::write( | ||
| skill_dir.join("SKILL.md"), | ||
| "---\nname: late-extra-skill\ndescription: late skill\n---\n\n# Body\n", | ||
| )?; | ||
|
|
||
| let skills_request_id = mcp | ||
| .send_skills_list_request(SkillsListParams { | ||
| cwds: vec![cwd.path().to_path_buf()], | ||
| force_reload: true, | ||
| }) | ||
| .await?; | ||
| let _: JSONRPCResponse = timeout( | ||
| DEFAULT_TIMEOUT, | ||
| mcp.read_stream_until_response_message(RequestId::Integer(skills_request_id)), | ||
| ) | ||
| .await??; | ||
|
|
||
| let turn_request_id = mcp | ||
| .send_turn_start_request(TurnStartParams { | ||
| thread_id: thread.id, | ||
| input: vec![V2UserInput::Text { | ||
| text: "Use the updated skill".to_string(), | ||
| text_elements: Vec::new(), | ||
| }], | ||
| ..Default::default() | ||
| }) | ||
| .await?; | ||
| let turn_response: JSONRPCResponse = timeout( | ||
| DEFAULT_TIMEOUT, | ||
| mcp.read_stream_until_response_message(RequestId::Integer(turn_request_id)), | ||
| ) | ||
| .await??; | ||
| let _: TurnStartResponse = to_response(turn_response)?; | ||
| timeout( | ||
| DEFAULT_TIMEOUT, | ||
| mcp.read_stream_until_notification_message("turn/completed"), | ||
| ) | ||
| .await??; | ||
|
|
||
| let requests = server | ||
| .received_requests() | ||
| .await | ||
| .context("failed to fetch received requests")?; | ||
| let request_body = requests | ||
| .last() | ||
| .context("expected a model request")? | ||
| .body_json::<serde_json::Value>()?; | ||
| let developer_messages = request_body["input"] | ||
| .as_array() | ||
| .context("expected model input")? | ||
| .iter() | ||
| .filter(|item| item["role"] == "developer") | ||
| .map(serde_json::Value::to_string) | ||
| .collect::<Vec<_>>() | ||
| .join("\n"); | ||
| assert!( | ||
| developer_messages.contains("late-extra-skill"), | ||
| "expected updated skill in developer messages: {developer_messages}" | ||
| ); | ||
| assert!(developer_messages.contains("late skill")); | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn skills_extra_roots_set_updates_process_runtime_roots() -> Result<()> { | ||
| let codex_home = TempDir::new()?; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codex Doesn't snapshot_for_cwd handle force_reload and reset the cache already?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary
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-L500TestAppServer::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-L746skills/listwithforce_reload: true, and verifies the next turn sees the updated plugin skill snapshot. codex-rs/app-server/tests/suite/v2/skills_list.rsL801-L891Testing
cargo fmt --allcargo fmt --all -- --checkgit diff --checkjust 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.
View task →
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codex why do we need this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need this change because
force_reloadonsnapshot_for_cwdonly affects theskills/listresponse path, not the turn/model-context path.skills/listeventually callssnapshot_for_cwd(&skills_input, force_reload, fs), soforce_reload: truebypasses the cwd cache for that list request. codex-rs/app-server/src/request_processors/catalog_processor.rsL545-L547snapshot_for_cwddoes not clear all skills caches; it only skips readingcache_by_cwdwhenforce_reloadis true, rebuilds the snapshot, and writes back to the cwd cache when applicable. codex-rs/core-skills/src/service.rsL169-L206snapshot_for_config, notsnapshot_for_cwd. codex-rs/core/src/session/turn_context.rsL720-L738snapshot_for_configreturns an existingcache_by_configentry if one is present, so a thread that already warmed that cache before a skill was added would keep sending the stale skill set to the model. codex-rs/core-skills/src/service.rsL126-L147skills_service.clear_cache()onforce_reloadis what clears bothcache_by_cwdandcache_by_config, making the forced reload invalidate the cache used by the next turn as well as the list response. codex-rs/app-server/src/request_processors/catalog_processor.rsL494-L498 codex-rs/core-skills/src/service.rsL223-L244skills/listwithforce_reload: true, then start a turn and assert the model developer messages include the new skill. codex-rs/app-server/tests/suite/v2/skills_list.rsL706-L795So the short answer is:
snapshot_for_cwd(force_reload = true)refreshes the list result, but without this explicitclear_cache()the next turn can still use a stale config-keyed skills snapshot.Commands used
pwd && find .. -name AGENTS.md -print && git status --short && nl -ba codex-rs/app-server/src/request_processors/catalog_processor.rs | sed -n '470,510p' && rg -n "snapshot_for_cwd|snapshot_for_config|clear_cache|plugins_for_config|PluginSkillSnapshots" codex-rs -g'*.rs'nl -ba codex-rs/core-skills/src/service.rs | sed -n '120,270p'; nl -ba codex-rs/core-plugins/src/manager.rs | sed -n '360,590p'; nl -ba codex-rs/core/src/session/turn_context.rs | sed -n '700,750p'; nl -ba codex-rs/app-server/src/request_processors/catalog_processor.rs | sed -n '510,555p'git log --oneline -5 --decorate && git rev-parse HEAD && nl -ba codex-rs/app-server/tests/suite/v2/skills_list.rs | sed -n '650,760p'nl -ba codex-rs/app-server/tests/suite/v2/skills_list.rs | sed -n '760,830p'; rg -n "late-extra-skill|Use the updated skill|skills_list_force_reload_clears_turn_skills_cache" codex-rs/app-server/tests/suite/v2/skills_list.rsView task →