Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Copy link
Copy Markdown
Collaborator

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Testing

  • cargo fmt --all
  • cargo fmt --all -- --check
  • git diff --check
  • ⚠️ just fmt (warning: environment lacked dotslash, 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-nextest was 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_v8 archive 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 →

Copy link
Copy Markdown
Author

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?

Copy link
Copy Markdown
Contributor

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_reload on snapshot_for_cwd only affects the skills/list response path, not the turn/model-context path.

So the short answer is: snapshot_for_cwd(force_reload = true) refreshes the list result, but without this explicit clear_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.rs

View task →

skills_service.clear_cache();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Clear plugin snapshots during forced skill reload

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 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Clear plugin skill snapshots on forced reload

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 👍 / 👎.

}
let fs = self
.thread_manager
.environment_manager()
Expand Down
96 changes: 96 additions & 0 deletions codex-rs/app-server/tests/suite/v2/skills_list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Use the auto environment for this thread test

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 👍 / 👎.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Use auto environment selection in this turn test

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 👍 / 👎.

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()?;
Expand Down
Loading