From 9671d20fc82ea269d49ecc70b29a8630b29c1a32 Mon Sep 17 00:00:00 2001 From: Eric Traut Date: Wed, 14 Oct 2026 17:22:42 +0000 Subject: [PATCH] Scope TUI skill refreshes and workspace commands to explicit directories (#54050) ## Why A queued skill refresh can outlive the workspace it was requested for when the working directory changes. ## What changed - Refresh skills for the current working directory when the skills management popup closes. - Ignore queued `ListSkills` commands whose directories do not include the current working directory. - Require an explicit working directory when constructing `WorkspaceCommand` and always pass it to app-server command execution. Update Git and GitHub metadata probes to use the constructor. ## Testing Extend regression tests to verify that closing skill management refreshes the configured working directory and that stale skill refreshes after a directory change do not issue a `skills/list` request. GitOrigin-RevId: 47ad1d95930840b3f6f0e20d7d8fe8d025e789e6 --- .../app/tests/session_lifecycle_requests.rs | 10 ++++++++++ codex-rs/tui/src/app/thread_routing.rs | 7 +++++++ codex-rs/tui/src/app_event_sender.rs | 8 -------- .../tui/src/bottom_pane/skills_toggle_view.rs | 2 -- codex-rs/tui/src/branch_summary.rs | 9 ++------- codex-rs/tui/src/chatwidget/skills.rs | 1 + .../src/chatwidget/tests/history_replay.rs | 9 ++++++++- codex-rs/tui/src/get_git_diff.rs | 11 ++++------- codex-rs/tui/src/workspace_command.rs | 19 ++++++++----------- 9 files changed, 40 insertions(+), 36 deletions(-) diff --git a/codex-rs/tui/src/app/tests/session_lifecycle_requests.rs b/codex-rs/tui/src/app/tests/session_lifecycle_requests.rs index ed67f0bf9d..5f5d4615ad 100644 --- a/codex-rs/tui/src/app/tests/session_lifecycle_requests.rs +++ b/codex-rs/tui/src/app/tests/session_lifecycle_requests.rs @@ -4589,6 +4589,16 @@ async fn changing_directory_preserves_project_trust_permissions_history_and_hook let source = codex_utils_path_uri::PathUri::from_abs_path(&agents.abs()); assert!(configured.instruction_source_paths.contains(&source)); assert_eq!(configured.approvals_reviewer, ApprovalsReviewer::AutoReview); + let earlier_skills_requests = recorded_params(&requests, "skills/list"); + app.submit_active_thread_op( + &mut server, + AppCommand::list_skills(vec![current.clone()], /*force_reload*/ true), + ) + .await?; + assert_eq!( + recorded_params(&requests, "skills/list"), + earlier_skills_requests + ); let (cwd, result) = (current.clone(), Err("stale skills".into())); let skills = AppEvent::SkillsListLoaded { cwd, result }; let (cwd, plugins) = (current.clone(), Some(vec![])); diff --git a/codex-rs/tui/src/app/thread_routing.rs b/codex-rs/tui/src/app/thread_routing.rs index f73bca912a..95591b4933 100644 --- a/codex-rs/tui/src/app/thread_routing.rs +++ b/codex-rs/tui/src/app/thread_routing.rs @@ -924,6 +924,13 @@ impl App { Ok(true) } AppCommand::ListSkills { cwds, force_reload } => { + // A queued refresh can outlive the workspace it was requested for. + if !cwds + .iter() + .any(|cwd| cwd == self.chat_widget.config_ref().cwd.as_path()) + { + return Ok(true); + } self.handle_skills_list_result( app_server .skills_list(codex_app_server_protocol::SkillsListParams { diff --git a/codex-rs/tui/src/app_event_sender.rs b/codex-rs/tui/src/app_event_sender.rs index dac6b8cbe8..0057b90ad0 100644 --- a/codex-rs/tui/src/app_event_sender.rs +++ b/codex-rs/tui/src/app_event_sender.rs @@ -3,7 +3,6 @@ //! This wraps the raw channel so call sites can submit typed `AppCommand`s //! without duplicating event construction or session logging behavior. -use std::path::PathBuf; use std::sync::Arc; use std::sync::atomic::AtomicBool; use std::sync::atomic::Ordering; @@ -86,13 +85,6 @@ impl AppEventSender { self.send(AppEvent::CodexOp(AppCommand::review(target))); } - pub(crate) fn list_skills(&self, cwds: Vec, force_reload: bool) { - self.send(AppEvent::CodexOp(AppCommand::list_skills( - cwds, - force_reload, - ))); - } - pub(crate) fn user_input_answer( &self, thread_id: ThreadId, diff --git a/codex-rs/tui/src/bottom_pane/skills_toggle_view.rs b/codex-rs/tui/src/bottom_pane/skills_toggle_view.rs index 3124fd25f3..9bd9084614 100644 --- a/codex-rs/tui/src/bottom_pane/skills_toggle_view.rs +++ b/codex-rs/tui/src/bottom_pane/skills_toggle_view.rs @@ -224,8 +224,6 @@ impl SkillsToggleView { } self.complete = true; self.app_event_tx.send(AppEvent::ManageSkillsClosed); - self.app_event_tx - .list_skills(Vec::new(), /*force_reload*/ true); } fn rows_height(&self, rows: &[GenericDisplayRow]) -> u16 { diff --git a/codex-rs/tui/src/branch_summary.rs b/codex-rs/tui/src/branch_summary.rs index 6329e43422..a14a25f104 100644 --- a/codex-rs/tui/src/branch_summary.rs +++ b/codex-rs/tui/src/branch_summary.rs @@ -480,11 +480,7 @@ async fn run_git_command( argv.push(codex_git_utils::SAFE_BARE_REPOSITORY_CONFIG.to_string()); argv.extend(args.iter().map(|arg| (*arg).to_string())); runner - .run( - WorkspaceCommand::new(argv) - .cwd(cwd.to_path_buf()) - .env("GIT_OPTIONAL_LOCKS", "0"), - ) + .run(WorkspaceCommand::new(argv, cwd.to_path_buf()).env("GIT_OPTIONAL_LOCKS", "0")) .await } @@ -502,8 +498,7 @@ async fn run_gh_command( argv.extend(args.iter().map(|arg| (*arg).to_string())); runner .run( - WorkspaceCommand::new(argv) - .cwd(cwd.to_path_buf()) + WorkspaceCommand::new(argv, cwd.to_path_buf()) .env("GH_PROMPT_DISABLED", "1") .env("GIT_TERMINAL_PROMPT", "0"), ) diff --git a/codex-rs/tui/src/chatwidget/skills.rs b/codex-rs/tui/src/chatwidget/skills.rs index 56bb6071f9..f505fdde38 100644 --- a/codex-rs/tui/src/chatwidget/skills.rs +++ b/codex-rs/tui/src/chatwidget/skills.rs @@ -101,6 +101,7 @@ impl ChatWidget { } pub(crate) fn handle_manage_skills_closed(&mut self) { + self.refresh_skills_for_current_cwd(/*force_reload*/ true); let Some(initial_state) = self.skills_initial_state.take() else { return; }; diff --git a/codex-rs/tui/src/chatwidget/tests/history_replay.rs b/codex-rs/tui/src/chatwidget/tests/history_replay.rs index 26388ec042..9655f96ff6 100644 --- a/codex-rs/tui/src/chatwidget/tests/history_replay.rs +++ b/codex-rs/tui/src/chatwidget/tests/history_replay.rs @@ -597,7 +597,7 @@ async fn replayed_user_message_preserves_remote_image_urls() { #[tokio::test] async fn session_configured_syncs_widget_config_permissions_and_cwd() { - let (mut chat, _rx, _ops) = make_chatwidget_manual(/*model_override*/ None).await; + let (mut chat, _rx, mut ops) = make_chatwidget_manual(/*model_override*/ None).await; chat.config .permissions @@ -675,6 +675,13 @@ async fn session_configured_syncs_widget_config_permissions_and_cwd() { expected_app_server_permission_profile ); assert_eq!(&chat.config_ref().cwd, &expected_cwd); + while ops.try_recv().is_ok() {} + chat.handle_manage_skills_closed(); + assert!(matches!( + ops.try_recv(), + Ok(AppCommand::ListSkills { cwds, force_reload: true }) + if cwds == vec![expected_cwd.to_path_buf()] + )); let updated_profile = PermissionProfile::workspace_write(); chat.set_permission_profile_from_session_snapshot(PermissionProfileSnapshot::legacy( diff --git a/codex-rs/tui/src/get_git_diff.rs b/codex-rs/tui/src/get_git_diff.rs index b3893cf493..11812b3984 100644 --- a/codex-rs/tui/src/get_git_diff.rs +++ b/codex-rs/tui/src/get_git_diff.rs @@ -36,7 +36,7 @@ impl FsmonitorProbeRunner for WorkspaceFsmonitorProbeRunner<'_> { let argv = ["git", "-c", codex_git_utils::SAFE_BARE_REPOSITORY_CONFIG] .into_iter() .chain(args.iter().copied()); - let command = WorkspaceCommand::new(argv).cwd(self.cwd.to_path_buf()); + let command = WorkspaceCommand::new(argv, self.cwd.to_path_buf()); match self.runner.run(command).await { Ok(output) if output.success() => Some(output.stdout.into_bytes()), _ => None, @@ -242,8 +242,7 @@ async fn run_git_command( ] .into_iter() .chain(args.iter().copied()); - let mut command = WorkspaceCommand::new(argv) - .cwd(cwd.to_path_buf()) + let mut command = WorkspaceCommand::new(argv, cwd.to_path_buf()) .timeout(DIFF_COMMAND_TIMEOUT) .disable_output_cap(); if !config_overrides.is_empty() { @@ -830,7 +829,7 @@ mod tests { fn assert_command_metadata(commands: &[WorkspaceCommand], cwd: &Path) { for command in commands { - assert_eq!(command.cwd.as_deref(), Some(cwd)); + assert_eq!(command.cwd.as_path(), cwd); if matches!( command.argv.get(3).map(String::as_str), Some("config" | "version") @@ -911,9 +910,7 @@ mod tests { > { Box::pin(async move { let mut process = ProcessCommand::new(&command.argv[0]); - process - .args(&command.argv[1..]) - .current_dir(command.cwd.expect("test command cwd")); + process.args(&command.argv[1..]).current_dir(command.cwd); for (key, value) in command.env { match value { Some(value) => { diff --git a/codex-rs/tui/src/workspace_command.rs b/codex-rs/tui/src/workspace_command.rs index 6d3c030603..5d0014a388 100644 --- a/codex-rs/tui/src/workspace_command.rs +++ b/codex-rs/tui/src/workspace_command.rs @@ -37,8 +37,8 @@ pub(crate) type WorkspaceCommandRunner = Arc; pub(crate) struct WorkspaceCommand { /// Program and arguments to execute without shell interpolation. pub(crate) argv: Vec, - /// Working directory for the command, if different from app-server's session cwd. - pub(crate) cwd: Option, + /// Explicit working directory on the host that executes the command. + pub(crate) cwd: PathBuf, /// Environment overrides where `None` removes a variable. pub(crate) env: HashMap>, /// Maximum wall-clock duration before app-server cancels the command. @@ -49,22 +49,19 @@ pub(crate) struct WorkspaceCommand { impl WorkspaceCommand { /// Creates a workspace command with conservative defaults for metadata probes. - pub(crate) fn new(argv: impl IntoIterator>) -> Self { + pub(crate) fn new( + argv: impl IntoIterator>, + cwd: impl Into, + ) -> Self { Self { argv: argv.into_iter().map(Into::into).collect(), - cwd: None, + cwd: cwd.into(), env: HashMap::new(), timeout: Duration::from_secs(/*secs*/ 5), disable_output_cap: false, } } - /// Sets the command working directory. - pub(crate) fn cwd(mut self, cwd: impl Into) -> Self { - self.cwd = Some(cwd.into()); - self - } - /// Adds or replaces one environment variable override. pub(crate) fn env(mut self, key: impl Into, value: impl Into) -> Self { self.env.insert(key.into(), Some(value.into())); @@ -192,7 +189,7 @@ impl WorkspaceCommandExecutor for AppServerWorkspaceCommandRunner { disable_output_cap: command.disable_output_cap, disable_timeout: false, timeout_ms: Some(timeout_ms), - cwd: command.cwd, + cwd: Some(command.cwd), env, size: None, sandbox_policy: None,