mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
app-server: preserve target-native environment cwd (#28146)
## Why app-server may run on a different OS from the selected exec-server environment. Parsing that environment’s cwd with the Codex host’s path rules prevents thread startup. ## What Carry environment cwd values as `LegacyAppPathString` at the app-server boundary and `PathUri` internally. Existing tool-call schemas and relative-path behavior stay host-native; remaining local-only consumers convert explicitly and leave follow-up TODOs. The Wine integration test verifies app-server can start a thread and complete an ordinary turn with a Windows environment cwd from Linux. ## Validation - `bazel test //codex-rs/core/tests/remote_env_windows:smoke-test --test_output=errors` - focused app-server environment-selection and protocol schema tests - scoped Clippy for `codex-core` and `codex-app-server-protocol`
This commit is contained in:
@@ -541,6 +541,37 @@ fn resolve_request_cwd(cwd: Option<PathBuf>) -> Result<Option<AbsolutePathBuf>,
|
||||
.transpose()
|
||||
}
|
||||
|
||||
fn resolve_turn_environment_selections(
|
||||
thread_manager: &ThreadManager,
|
||||
environments: Option<Vec<TurnEnvironmentParams>>,
|
||||
) -> Result<Option<Vec<TurnEnvironmentSelection>>, JSONRPCErrorError> {
|
||||
let Some(environments) = environments else {
|
||||
return Ok(None);
|
||||
};
|
||||
let mut selections = Vec::with_capacity(environments.len());
|
||||
for environment in environments {
|
||||
let environment_id = environment.environment_id;
|
||||
let cwd = environment
|
||||
.cwd
|
||||
.infer_absolute_path_convention()
|
||||
.and_then(|convention| environment.cwd.to_path_uri(convention).ok())
|
||||
.ok_or_else(|| {
|
||||
invalid_request(format!(
|
||||
"invalid cwd for environment `{environment_id}`: path `{}` does not use absolute POSIX or Windows path syntax",
|
||||
environment.cwd
|
||||
))
|
||||
})?;
|
||||
selections.push(TurnEnvironmentSelection {
|
||||
environment_id,
|
||||
cwd,
|
||||
});
|
||||
}
|
||||
thread_manager
|
||||
.validate_environment_selections(&selections)
|
||||
.map_err(environment_selection_error)?;
|
||||
Ok(Some(selections))
|
||||
}
|
||||
|
||||
fn resolve_runtime_workspace_roots(workspace_roots: Vec<AbsolutePathBuf>) -> Vec<AbsolutePathBuf> {
|
||||
let mut resolved_roots = Vec::new();
|
||||
for root in workspace_roots {
|
||||
|
||||
@@ -4,7 +4,6 @@ use codex_app_server_protocol::SelectedCapabilityRoot;
|
||||
use codex_extension_api::ExtensionDataInit;
|
||||
use codex_protocol::models::BUILT_IN_PERMISSION_PROFILE_DANGER_FULL_ACCESS;
|
||||
use codex_protocol::models::BUILT_IN_PERMISSION_PROFILE_WORKSPACE;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
|
||||
const THREAD_LIST_DEFAULT_LIMIT: usize = 25;
|
||||
const THREAD_LIST_MAX_LIMIT: usize = 100;
|
||||
@@ -906,7 +905,8 @@ impl ThreadRequestProcessor {
|
||||
"`permissions` cannot be combined with `sandbox`",
|
||||
));
|
||||
}
|
||||
let environment_selections = self.parse_environment_selections(environments)?;
|
||||
let environment_selections =
|
||||
resolve_turn_environment_selections(self.thread_manager.as_ref(), environments)?;
|
||||
let runtime_workspace_roots = runtime_workspace_roots.map(resolve_runtime_workspace_roots);
|
||||
let mut typesafe_overrides = self.build_thread_config_overrides(
|
||||
model,
|
||||
@@ -1311,27 +1311,6 @@ impl ThreadRequestProcessor {
|
||||
}
|
||||
}
|
||||
|
||||
fn parse_environment_selections(
|
||||
&self,
|
||||
environments: Option<Vec<TurnEnvironmentParams>>,
|
||||
) -> Result<Option<Vec<TurnEnvironmentSelection>>, JSONRPCErrorError> {
|
||||
let environment_selections = environments.map(|environments| {
|
||||
environments
|
||||
.into_iter()
|
||||
.map(|environment| TurnEnvironmentSelection {
|
||||
environment_id: environment.environment_id,
|
||||
cwd: PathUri::from_abs_path(&environment.cwd),
|
||||
})
|
||||
.collect::<Vec<_>>()
|
||||
});
|
||||
if let Some(environment_selections) = environment_selections.as_ref() {
|
||||
self.thread_manager
|
||||
.validate_environment_selections(environment_selections)
|
||||
.map_err(environment_selection_error)?;
|
||||
}
|
||||
Ok(environment_selections)
|
||||
}
|
||||
|
||||
async fn thread_archive_inner(
|
||||
&self,
|
||||
params: ThreadArchiveParams,
|
||||
|
||||
@@ -4,7 +4,6 @@ use codex_protocol::protocol::AdditionalContextKind as CoreAdditionalContextKind
|
||||
use codex_protocol::protocol::MultiAgentVersion;
|
||||
use codex_protocol::protocol::SessionSource;
|
||||
use codex_protocol::protocol::SubAgentSource;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
|
||||
const DIRECT_INPUT_TO_MULTI_AGENT_V2_SUBAGENT_ERROR: &str =
|
||||
"direct app-server input is not allowed for multi-agent v2 sub-agents";
|
||||
@@ -341,27 +340,6 @@ impl TurnRequestProcessor {
|
||||
Ok((review_request, hint))
|
||||
}
|
||||
|
||||
fn parse_environment_selections(
|
||||
&self,
|
||||
environments: Option<Vec<TurnEnvironmentParams>>,
|
||||
) -> Result<Option<Vec<TurnEnvironmentSelection>>, JSONRPCErrorError> {
|
||||
let environment_selections = environments.map(|environments| {
|
||||
environments
|
||||
.into_iter()
|
||||
.map(|environment| TurnEnvironmentSelection {
|
||||
environment_id: environment.environment_id,
|
||||
cwd: PathUri::from_abs_path(&environment.cwd),
|
||||
})
|
||||
.collect::<Vec<_>>()
|
||||
});
|
||||
if let Some(environment_selections) = environment_selections.as_ref() {
|
||||
self.thread_manager
|
||||
.validate_environment_selections(environment_selections)
|
||||
.map_err(environment_selection_error)?;
|
||||
}
|
||||
Ok(environment_selections)
|
||||
}
|
||||
|
||||
async fn request_trace_context(
|
||||
&self,
|
||||
request_id: &ConnectionRequestId,
|
||||
@@ -433,7 +411,8 @@ impl TurnRequestProcessor {
|
||||
self.track_error_response(&request_id, error, /*error_type*/ None);
|
||||
})?;
|
||||
|
||||
let environment_selections = self.parse_environment_selections(params.environments)?;
|
||||
let environment_selections =
|
||||
resolve_turn_environment_selections(self.thread_manager.as_ref(), params.environments)?;
|
||||
|
||||
// Map v2 input items to core input items.
|
||||
let mapped_items: Vec<CoreInputItem> = params
|
||||
|
||||
@@ -293,7 +293,10 @@ async fn thread_start_rejects_unknown_environment_as_invalid_request() -> Result
|
||||
.send_thread_start_request(ThreadStartParams {
|
||||
environments: Some(vec![TurnEnvironmentParams {
|
||||
environment_id: "missing".to_string(),
|
||||
cwd: codex_home.path().to_path_buf().try_into()?,
|
||||
cwd: codex_utils_absolute_path::AbsolutePathBuf::try_from(
|
||||
codex_home.path().to_path_buf(),
|
||||
)?
|
||||
.into(),
|
||||
}]),
|
||||
..Default::default()
|
||||
})
|
||||
@@ -312,6 +315,40 @@ async fn thread_start_rejects_unknown_environment_as_invalid_request() -> Result
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_start_rejects_relative_environment_cwd_as_invalid_request() -> Result<()> {
|
||||
let server = create_mock_responses_server_repeating_assistant("Done").await;
|
||||
let codex_home = TempDir::new()?;
|
||||
create_config_toml_without_approval_policy(codex_home.path(), &server.uri())?;
|
||||
|
||||
let mut mcp = TestAppServer::new(codex_home.path()).await?;
|
||||
timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??;
|
||||
|
||||
let request_id = mcp
|
||||
.send_thread_start_request(ThreadStartParams {
|
||||
environments: Some(vec![TurnEnvironmentParams {
|
||||
environment_id: "local".to_string(),
|
||||
cwd: serde_json::from_value(json!("relative"))?,
|
||||
}]),
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
let error: JSONRPCError = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_error_message(RequestId::Integer(request_id)),
|
||||
)
|
||||
.await??;
|
||||
|
||||
assert_eq!(error.id, RequestId::Integer(request_id));
|
||||
assert_eq!(error.error.code, INVALID_REQUEST_ERROR_CODE);
|
||||
assert_eq!(
|
||||
error.error.message,
|
||||
"invalid cwd for environment `local`: path `relative` does not use absolute POSIX or Windows path syntax"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn thread_start_response_includes_loaded_instruction_sources() -> Result<()> {
|
||||
let server = create_mock_responses_server_repeating_assistant("Done").await;
|
||||
|
||||
@@ -76,6 +76,7 @@ use codex_protocol::models::BUILT_IN_PERMISSION_PROFILE_DANGER_FULL_ACCESS;
|
||||
use codex_protocol::models::ImageDetail;
|
||||
use codex_protocol::openai_models::ReasoningEffort;
|
||||
use codex_protocol::user_input::MAX_USER_INPUT_TEXT_CHARS;
|
||||
use codex_utils_absolute_path::test_support::PathExt;
|
||||
use core_test_support::responses;
|
||||
use core_test_support::skip_if_no_network;
|
||||
use pretty_assertions::assert_eq;
|
||||
@@ -1326,7 +1327,10 @@ async fn turn_start_rejects_unknown_environment_before_starting_turn() -> Result
|
||||
}],
|
||||
environments: Some(vec![TurnEnvironmentParams {
|
||||
environment_id: "missing".to_string(),
|
||||
cwd: codex_home.path().to_path_buf().try_into()?,
|
||||
cwd: codex_utils_absolute_path::AbsolutePathBuf::try_from(
|
||||
codex_home.path().to_path_buf(),
|
||||
)?
|
||||
.into(),
|
||||
}]),
|
||||
..Default::default()
|
||||
})
|
||||
@@ -2669,7 +2673,7 @@ async fn run_environment_selection_case(
|
||||
.send_thread_start_request(ThreadStartParams {
|
||||
model: Some("mock-model".to_string()),
|
||||
cwd: Some(workspace.to_string_lossy().into_owned()),
|
||||
environments: environment_params(case.sticky, workspace)?,
|
||||
environments: environment_params(case.sticky, workspace),
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
@@ -2688,7 +2692,7 @@ async fn run_environment_selection_case(
|
||||
text: format!("run {}", case.name),
|
||||
text_elements: Vec::new(),
|
||||
}],
|
||||
environments: environment_params(case.turn, workspace)?,
|
||||
environments: environment_params(case.turn, workspace),
|
||||
cwd: Some(workspace.to_path_buf()),
|
||||
model: Some("mock-model".to_string()),
|
||||
..Default::default()
|
||||
@@ -2735,21 +2739,15 @@ async fn run_environment_selection_case(
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn environment_params(
|
||||
ids: Option<&[&str]>,
|
||||
cwd: &Path,
|
||||
) -> Result<Option<Vec<TurnEnvironmentParams>>> {
|
||||
fn environment_params(ids: Option<&[&str]>, cwd: &Path) -> Option<Vec<TurnEnvironmentParams>> {
|
||||
ids.map(|ids| {
|
||||
ids.iter()
|
||||
.map(|id| {
|
||||
Ok(TurnEnvironmentParams {
|
||||
environment_id: (*id).to_string(),
|
||||
cwd: cwd.to_path_buf().try_into()?,
|
||||
})
|
||||
.map(|id| TurnEnvironmentParams {
|
||||
environment_id: (*id).to_string(),
|
||||
cwd: cwd.abs().into(),
|
||||
})
|
||||
.collect()
|
||||
})
|
||||
.transpose()
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
|
||||
Reference in New Issue
Block a user