mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Add remote --cd forwarding for app-server sessions (#16700)
Addresses #16124 Problem: `codex --remote --cd <path>` canonicalized the path locally and then omitted it from remote thread lifecycle requests, so remote-only working directories failed or were ignored. Solution: Keep remote startup on the local cwd, forward explicit `--cd` values verbatim to `thread/start`, `thread/resume`, and `thread/fork`, and cover the behavior with `codex-tui` tests. Testing: I manually tested `--remote --cd` with both absolute and relative paths and validated correct behavior. --- Update based on code review feedback: Problem: Remote `--cd` was forwarded to `thread/resume` and `thread/fork`, but not to `thread/list` lookups, so `--resume --last` and picker flows could select a session from the wrong cwd; relative cwd filters also failed against stored absolute paths. Solution: Apply explicit remote `--cd` to `thread/list` lookups for `--last` and picker flows, normalize relative cwd filters on the app-server before exact matching, and document/test the behavior.
This commit is contained in:
committed by
GitHub
Unverified
parent
a71fc47cf8
commit
0ab8eda375
@@ -104,6 +104,7 @@ pub(crate) struct AppServerBootstrap {
|
||||
pub(crate) struct AppServerSession {
|
||||
client: AppServerClient,
|
||||
next_request_id: i64,
|
||||
remote_cwd_override: Option<PathBuf>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq)]
|
||||
@@ -150,9 +151,19 @@ impl AppServerSession {
|
||||
Self {
|
||||
client,
|
||||
next_request_id: 1,
|
||||
remote_cwd_override: None,
|
||||
}
|
||||
}
|
||||
|
||||
pub(crate) fn with_remote_cwd_override(mut self, remote_cwd_override: Option<PathBuf>) -> Self {
|
||||
self.remote_cwd_override = remote_cwd_override;
|
||||
self
|
||||
}
|
||||
|
||||
pub(crate) fn remote_cwd_override(&self) -> Option<&std::path::Path> {
|
||||
self.remote_cwd_override.as_deref()
|
||||
}
|
||||
|
||||
pub(crate) fn is_remote(&self) -> bool {
|
||||
matches!(self.client, AppServerClient::Remote(_))
|
||||
}
|
||||
@@ -290,7 +301,11 @@ impl AppServerSession {
|
||||
.client
|
||||
.request_typed(ClientRequest::ThreadStart {
|
||||
request_id,
|
||||
params: thread_start_params_from_config(config, self.thread_params_mode()),
|
||||
params: thread_start_params_from_config(
|
||||
config,
|
||||
self.thread_params_mode(),
|
||||
self.remote_cwd_override.as_deref(),
|
||||
),
|
||||
})
|
||||
.await
|
||||
.wrap_err("thread/start failed during TUI bootstrap")?;
|
||||
@@ -311,6 +326,7 @@ impl AppServerSession {
|
||||
config.clone(),
|
||||
thread_id,
|
||||
self.thread_params_mode(),
|
||||
self.remote_cwd_override.as_deref(),
|
||||
),
|
||||
})
|
||||
.await
|
||||
@@ -332,6 +348,7 @@ impl AppServerSession {
|
||||
config.clone(),
|
||||
thread_id,
|
||||
self.thread_params_mode(),
|
||||
self.remote_cwd_override.as_deref(),
|
||||
),
|
||||
})
|
||||
.await
|
||||
@@ -839,11 +856,12 @@ fn sandbox_mode_from_policy(
|
||||
fn thread_start_params_from_config(
|
||||
config: &Config,
|
||||
thread_params_mode: ThreadParamsMode,
|
||||
remote_cwd_override: Option<&std::path::Path>,
|
||||
) -> ThreadStartParams {
|
||||
ThreadStartParams {
|
||||
model: config.model.clone(),
|
||||
model_provider: thread_params_mode.model_provider_from_config(config),
|
||||
cwd: thread_cwd_from_config(config, thread_params_mode),
|
||||
cwd: thread_cwd_from_config(config, thread_params_mode, remote_cwd_override),
|
||||
approval_policy: Some(config.permissions.approval_policy.value().into()),
|
||||
approvals_reviewer: approvals_reviewer_override_from_config(config),
|
||||
sandbox: sandbox_mode_from_policy(config.permissions.sandbox_policy.get().clone()),
|
||||
@@ -858,12 +876,13 @@ fn thread_resume_params_from_config(
|
||||
config: Config,
|
||||
thread_id: ThreadId,
|
||||
thread_params_mode: ThreadParamsMode,
|
||||
remote_cwd_override: Option<&std::path::Path>,
|
||||
) -> ThreadResumeParams {
|
||||
ThreadResumeParams {
|
||||
thread_id: thread_id.to_string(),
|
||||
model: config.model.clone(),
|
||||
model_provider: thread_params_mode.model_provider_from_config(&config),
|
||||
cwd: thread_cwd_from_config(&config, thread_params_mode),
|
||||
cwd: thread_cwd_from_config(&config, thread_params_mode, remote_cwd_override),
|
||||
approval_policy: Some(config.permissions.approval_policy.value().into()),
|
||||
approvals_reviewer: approvals_reviewer_override_from_config(&config),
|
||||
sandbox: sandbox_mode_from_policy(config.permissions.sandbox_policy.get().clone()),
|
||||
@@ -877,12 +896,13 @@ fn thread_fork_params_from_config(
|
||||
config: Config,
|
||||
thread_id: ThreadId,
|
||||
thread_params_mode: ThreadParamsMode,
|
||||
remote_cwd_override: Option<&std::path::Path>,
|
||||
) -> ThreadForkParams {
|
||||
ThreadForkParams {
|
||||
thread_id: thread_id.to_string(),
|
||||
model: config.model.clone(),
|
||||
model_provider: thread_params_mode.model_provider_from_config(&config),
|
||||
cwd: thread_cwd_from_config(&config, thread_params_mode),
|
||||
cwd: thread_cwd_from_config(&config, thread_params_mode, remote_cwd_override),
|
||||
approval_policy: Some(config.permissions.approval_policy.value().into()),
|
||||
approvals_reviewer: approvals_reviewer_override_from_config(&config),
|
||||
sandbox: sandbox_mode_from_policy(config.permissions.sandbox_policy.get().clone()),
|
||||
@@ -893,10 +913,16 @@ fn thread_fork_params_from_config(
|
||||
}
|
||||
}
|
||||
|
||||
fn thread_cwd_from_config(config: &Config, thread_params_mode: ThreadParamsMode) -> Option<String> {
|
||||
fn thread_cwd_from_config(
|
||||
config: &Config,
|
||||
thread_params_mode: ThreadParamsMode,
|
||||
remote_cwd_override: Option<&std::path::Path>,
|
||||
) -> Option<String> {
|
||||
match thread_params_mode {
|
||||
ThreadParamsMode::Embedded => Some(config.cwd.to_string_lossy().to_string()),
|
||||
ThreadParamsMode::Remote => None,
|
||||
ThreadParamsMode::Remote => {
|
||||
remote_cwd_override.map(|cwd| cwd.to_string_lossy().to_string())
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1143,22 +1169,39 @@ mod tests {
|
||||
let temp_dir = tempfile::tempdir().expect("tempdir");
|
||||
let config = build_config(&temp_dir).await;
|
||||
|
||||
let params = thread_start_params_from_config(&config, ThreadParamsMode::Embedded);
|
||||
let params = thread_start_params_from_config(
|
||||
&config,
|
||||
ThreadParamsMode::Embedded,
|
||||
/*remote_cwd_override*/ None,
|
||||
);
|
||||
|
||||
assert_eq!(params.cwd, Some(config.cwd.to_string_lossy().to_string()));
|
||||
assert_eq!(params.model_provider, Some(config.model_provider_id));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_lifecycle_params_omit_local_overrides_for_remote_sessions() {
|
||||
async fn thread_lifecycle_params_omit_cwd_without_remote_override_for_remote_sessions() {
|
||||
let temp_dir = tempfile::tempdir().expect("tempdir");
|
||||
let config = build_config(&temp_dir).await;
|
||||
let thread_id = ThreadId::new();
|
||||
|
||||
let start = thread_start_params_from_config(&config, ThreadParamsMode::Remote);
|
||||
let resume =
|
||||
thread_resume_params_from_config(config.clone(), thread_id, ThreadParamsMode::Remote);
|
||||
let fork = thread_fork_params_from_config(config, thread_id, ThreadParamsMode::Remote);
|
||||
let start = thread_start_params_from_config(
|
||||
&config,
|
||||
ThreadParamsMode::Remote,
|
||||
/*remote_cwd_override*/ None,
|
||||
);
|
||||
let resume = thread_resume_params_from_config(
|
||||
config.clone(),
|
||||
thread_id,
|
||||
ThreadParamsMode::Remote,
|
||||
/*remote_cwd_override*/ None,
|
||||
);
|
||||
let fork = thread_fork_params_from_config(
|
||||
config,
|
||||
thread_id,
|
||||
ThreadParamsMode::Remote,
|
||||
/*remote_cwd_override*/ None,
|
||||
);
|
||||
|
||||
assert_eq!(start.cwd, None);
|
||||
assert_eq!(resume.cwd, None);
|
||||
@@ -1168,6 +1211,39 @@ mod tests {
|
||||
assert_eq!(fork.model_provider, None);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_lifecycle_params_forward_explicit_remote_cwd_override_for_remote_sessions() {
|
||||
let temp_dir = tempfile::tempdir().expect("tempdir");
|
||||
let config = build_config(&temp_dir).await;
|
||||
let thread_id = ThreadId::new();
|
||||
let remote_cwd = PathBuf::from("repo/on/server");
|
||||
|
||||
let start = thread_start_params_from_config(
|
||||
&config,
|
||||
ThreadParamsMode::Remote,
|
||||
Some(remote_cwd.as_path()),
|
||||
);
|
||||
let resume = thread_resume_params_from_config(
|
||||
config.clone(),
|
||||
thread_id,
|
||||
ThreadParamsMode::Remote,
|
||||
Some(remote_cwd.as_path()),
|
||||
);
|
||||
let fork = thread_fork_params_from_config(
|
||||
config,
|
||||
thread_id,
|
||||
ThreadParamsMode::Remote,
|
||||
Some(remote_cwd.as_path()),
|
||||
);
|
||||
|
||||
assert_eq!(start.cwd.as_deref(), Some("repo/on/server"));
|
||||
assert_eq!(resume.cwd.as_deref(), Some("repo/on/server"));
|
||||
assert_eq!(fork.cwd.as_deref(), Some("repo/on/server"));
|
||||
assert_eq!(start.model_provider, None);
|
||||
assert_eq!(resume.model_provider, None);
|
||||
assert_eq!(fork.model_provider, None);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn resume_response_restores_turns_from_thread_items() {
|
||||
let temp_dir = tempfile::tempdir().expect("tempdir");
|
||||
|
||||
Reference in New Issue
Block a user