openai/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-10-20 17:38:45 +00:00
Code Issues Packages Projects Releases Wiki Activity

app-server: address environment cwd review feedback

Browse Source
This commit is contained in:
Adam Perry
2026-06-16 16:13:43 +00:00
parent f1a3c44427
commit b92ee76fd3
7 changed files with 150 additions and 53 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/app-server-protocol/src/protocol/v2/tests.rs
+7 -24
View File
@@ -3723,8 +3723,14 @@ fn thread_settings_update_params_preserve_field_level_experimental_gates() {
#[test]
fn turn_start_params_round_trip_environments() {
// Use a path foreign to the test host so this exercises syntax preservation instead of the
// host-native conversion performed by test_absolute_path().
#[cfg(windows)]
let raw_cwd = "/workspace";
#[cfg(not(windows))]
let raw_cwd = r"C:\workspace";
let cwd: ApiPathString =
serde_json::from_value(json!(r"C:\workspace")).expect("API path should deserialize");
serde_json::from_value(json!(raw_cwd)).expect("API path should deserialize");
let params: TurnStartParams = serde_json::from_value(json!({
"threadId": "thread_123",
"input": [],
@@ -3806,29 +3812,6 @@ fn turn_start_params_treat_null_or_omitted_environments_as_default() {
);
}
#[test]
fn turn_start_params_preserve_relative_environment_cwd_for_boundary_validation() {
let params = serde_json::from_value::<TurnStartParams>(json!({
"threadId": "thread_123",
"input": [],
"environments": [
{
"environmentId": "local",
"cwd": "relative"
}
],
}))
.expect("native cwd should deserialize without applying host rules");
assert_eq!(
params.environments,
Some(vec![TurnEnvironmentParams {
environment_id: "local".to_string(),
cwd: serde_json::from_value(json!("relative")).expect("API path should deserialize"),
}])
);
}
#[test]
fn realtime_append_text_defaults_role_to_user() {
let params = serde_json::from_value::<ThreadRealtimeAppendTextParams>(json!({
codex-rs/app-server/README.md
+1 -1
View File
@@ -709,7 +709,7 @@ Turns attach user input (text or images) to a thread and trigger Codex generatio
- `{"type":"image","url":"https://…png"}`
- `{"type":"localImage","path":"/tmp/screenshot.png"}`
You can optionally specify config overrides on the new turn. If specified, these settings become the default for subsequent turns on the same thread. `outputSchema` applies only to the current turn. Experimental `environments` is turn-scoped: omit it to inherit the thread's sticky environments, pass `[]` to run the turn with no environments, or pass explicit environment ids to override the sticky selection for this turn only. Each environment `cwd` must be absolute and use that environment's native path convention, so a Windows environment uses a value such as `C:\\workspace` even when app-server runs on Linux.
You can optionally specify config overrides on the new turn. If specified, these settings become the default for subsequent turns on the same thread. `outputSchema` applies only to the current turn. Experimental `environments` is turn-scoped: omit it to inherit the thread's sticky environments, pass `[]` to run the turn with no environments, or pass explicit environment objects to override the sticky selection for this turn only. A `cwd` is allowed on each environment object.
`approvalsReviewer` accepts:
codex-rs/app-server/src/request_processors.rs
-1
View File
@@ -554,7 +554,6 @@ async fn resolve_turn_environment_selections(
.into_iter()
.map(|environment| (environment.environment_id, environment.cwd)),
)
.await
.map(Some)
.map_err(environment_selection_error)
}
codex-rs/app-server/tests/suite/v2/thread_start.rs
+36
View File
@@ -317,6 +317,42 @@ 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_raw_request(
"thread/start",
Some(json!({
"environments": [{
"environmentId": "local",
"cwd": "relative",
}],
})),
)
.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;
codex-rs/core/src/thread_manager.rs
+2 -15
View File
@@ -474,27 +474,13 @@ impl ThreadManager {
}
/// Resolves environment-native cwd strings without applying the Codex host's path rules.
pub async fn resolve_native_environment_selections(
pub fn resolve_native_environment_selections(
&self,
environments: impl IntoIterator<Item = (String, ApiPathString)>,
) -> CodexResult<Vec<TurnEnvironmentSelection>> {
let environments = environments.into_iter();
let mut environment_ids = HashSet::new();
let mut selections = Vec::with_capacity(environments.size_hint().0);
for (environment_id, cwd) in environments {
if !environment_ids.insert(environment_id.clone()) {
return Err(CodexErr::InvalidRequest(format!(
"duplicate turn environment id `{environment_id}`"
)));
}
self.state
.environment_manager
.get_environment(&environment_id)
.ok_or_else(|| {
CodexErr::InvalidRequest(format!(
"unknown turn environment id `{environment_id}`"
))
})?;
let convention = cwd.infer_absolute_path_convention().ok_or_else(|| {
CodexErr::InvalidRequest(format!(
"invalid cwd for environment `{environment_id}`: path `{cwd}` does not use absolute POSIX or Windows path syntax"
@@ -510,6 +496,7 @@ impl ThreadManager {
cwd,
});
}
self.validate_environment_selections(&selections)?;
Ok(selections)
}
codex-rs/core/src/thread_manager_tests.rs
-3
View File
@@ -373,7 +373,6 @@ async fn native_environment_selections_preserve_selected_path_syntax() {
ApiPathString::from_abs_path(&config.cwd, PathConvention::native())
.expect("render native cwd"),
)])
.await
.expect("resolve local selection");
assert_eq!(
@@ -406,7 +405,6 @@ async fn native_environment_selections_reject_duplicate_ids() {
("local".to_string(), native_cwd.clone()),
("local".to_string(), native_cwd),
])
.await
.expect_err("duplicate ids must fail");
assert_eq!(error.to_string(), "duplicate turn environment id `local`");
@@ -417,7 +415,6 @@ async fn native_environment_selections_reject_duplicate_ids() {
serde_json::from_value(serde_json::json!("relative"))
.expect("API path should deserialize"),
)])
.await
.expect_err("relative cwd must fail at the environment boundary");
assert_eq!(
error.to_string(),
codex-rs/core/tests/remote_env_windows/remote_env_windows_test.rs
+104 -9
View File
@@ -3,8 +3,15 @@
use anyhow::Context;
use anyhow::Result;
use app_test_support::TestAppServer;
use codex_app_server_protocol::JSONRPCError;
use app_test_support::create_mock_responses_server_sequence;
use app_test_support::to_response;
use app_test_support::write_mock_responses_config_toml;
use codex_app_server_protocol::CommandExecutionStatus;
use codex_app_server_protocol::ItemCompletedNotification;
use codex_app_server_protocol::JSONRPCMessage;
use codex_app_server_protocol::RequestId;
use codex_app_server_protocol::ThreadItem;
use codex_app_server_protocol::ThreadStartResponse;
use codex_exec_server::REMOTE_ENVIRONMENT_ID;
use codex_exec_server::CODEX_EXEC_SERVER_URL_ENV_VAR;
use codex_features::Feature;
@@ -29,6 +36,7 @@ use core_test_support::wait_for_event;
use codex_utils_path_uri::PathUri;
use pretty_assertions::assert_eq;
use serde_json::json;
use std::collections::BTreeMap;
use tempfile::TempDir;
use tokio::time::timeout;
use wine_exec_server_test_support::WineExecServer;
@@ -166,6 +174,36 @@ async fn app_server_starts_thread_with_windows_environment_native_cwd() -> Resul
WineExecServer
.scope(|exec_server_url| async move {
let codex_home = TempDir::new()?;
let responses = vec![
sse(vec![
ev_response_created("resp-1"),
ev_function_call(
CALL_ID,
"exec_command",
&serde_json::to_string(&json!({
"cmd": COMMAND,
"login": false,
"yield_time_ms": 5_000,
}))?,
),
ev_completed("resp-1"),
]),
sse(vec![
ev_response_created("resp-2"),
ev_assistant_message("msg-1", "done"),
ev_completed("resp-2"),
]),
];
let server = create_mock_responses_server_sequence(responses).await;
write_mock_responses_config_toml(
codex_home.path(),
&server.uri(),
&BTreeMap::from([(Feature::UnifiedExec, true)]),
100_000,
/*requires_openai_auth*/ None,
"mock",
"compact",
)?;
let mut app_server = TestAppServer::new_with_env(
codex_home.path(),
&[(
@@ -187,18 +225,75 @@ async fn app_server_starts_thread_with_windows_environment_native_cwd() -> Resul
})),
)
.await?;
let error: JSONRPCError = timeout(
let request_id = RequestId::Integer(request_id);
let message = timeout(APP_SERVER_READ_TIMEOUT, async {
loop {
let message = app_server.read_next_message().await?;
match &message {
JSONRPCMessage::Response(response) if response.id == request_id => {
return Ok::<JSONRPCMessage, anyhow::Error>(message);
}
JSONRPCMessage::Error(error) if error.id == request_id => {
return Ok(message);
}
_ => {}
}
}
})
.await??;
let JSONRPCMessage::Response(response) = message else {
panic!("thread/start should succeed, got {message:?}");
};
let response: ThreadStartResponse = to_response(response)?;
assert!(!response.thread.id.is_empty());
let turn_request_id = app_server
.send_raw_request(
"turn/start",
Some(json!({
"threadId": response.thread.id,
"input": [{"type": "text", "text": "run the Windows smoke command"}],
"approvalPolicy": "never",
"sandboxPolicy": {"type": "dangerFullAccess"},
})),
)
.await?;
timeout(
APP_SERVER_READ_TIMEOUT,
app_server.read_stream_until_error_message(RequestId::Integer(request_id)),
app_server.read_stream_until_response_message(RequestId::Integer(turn_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 request: AbsolutePathBuf deserialized without a base path"
);
let completed = timeout(APP_SERVER_READ_TIMEOUT, async {
loop {
let notification = app_server
.read_stream_until_notification_message("item/completed")
.await?;
let completed: ItemCompletedNotification = serde_json::from_value(
notification.params.context("item/completed params")?,
)?;
if matches!(completed.item, ThreadItem::CommandExecution { .. }) {
return Ok::<ThreadItem, anyhow::Error>(completed.item);
}
}
})
.await??;
let ThreadItem::CommandExecution {
id,
status,
exit_code,
..
} = completed
else {
unreachable!("loop returns only command execution items");
};
assert_eq!(id, CALL_ID);
assert_eq!((status, exit_code), (CommandExecutionStatus::Completed, Some(0)));
timeout(
APP_SERVER_READ_TIMEOUT,
app_server.read_stream_until_notification_message("turn/completed"),
)
.await??;
Ok(())
})