feat: add workdir to unified_exec (#6466)
This commit is contained in:
@@ -1,3 +1,5 @@
|
|||||||
|
use std::path::PathBuf;
|
||||||
|
|
||||||
use async_trait::async_trait;
|
use async_trait::async_trait;
|
||||||
use serde::Deserialize;
|
use serde::Deserialize;
|
||||||
|
|
||||||
@@ -24,6 +26,8 @@ pub struct UnifiedExecHandler;
|
|||||||
#[derive(Debug, Deserialize)]
|
#[derive(Debug, Deserialize)]
|
||||||
struct ExecCommandArgs {
|
struct ExecCommandArgs {
|
||||||
cmd: String,
|
cmd: String,
|
||||||
|
#[serde(default)]
|
||||||
|
workdir: Option<String>,
|
||||||
#[serde(default = "default_shell")]
|
#[serde(default = "default_shell")]
|
||||||
shell: String,
|
shell: String,
|
||||||
#[serde(default = "default_login")]
|
#[serde(default = "default_login")]
|
||||||
@@ -96,6 +100,12 @@ impl ToolHandler for UnifiedExecHandler {
|
|||||||
"failed to parse exec_command arguments: {err:?}"
|
"failed to parse exec_command arguments: {err:?}"
|
||||||
))
|
))
|
||||||
})?;
|
})?;
|
||||||
|
let workdir = args
|
||||||
|
.workdir
|
||||||
|
.as_deref()
|
||||||
|
.filter(|value| !value.is_empty())
|
||||||
|
.map(PathBuf::from);
|
||||||
|
let cwd = workdir.clone().unwrap_or_else(|| context.turn.cwd.clone());
|
||||||
|
|
||||||
let event_ctx = ToolEventCtx::new(
|
let event_ctx = ToolEventCtx::new(
|
||||||
context.session.as_ref(),
|
context.session.as_ref(),
|
||||||
@@ -103,8 +113,7 @@ impl ToolHandler for UnifiedExecHandler {
|
|||||||
&context.call_id,
|
&context.call_id,
|
||||||
None,
|
None,
|
||||||
);
|
);
|
||||||
let emitter =
|
let emitter = ToolEmitter::unified_exec(args.cmd.clone(), cwd.clone(), true);
|
||||||
ToolEmitter::unified_exec(args.cmd.clone(), context.turn.cwd.clone(), true);
|
|
||||||
emitter.emit(event_ctx, ToolEventStage::Begin).await;
|
emitter.emit(event_ctx, ToolEventStage::Begin).await;
|
||||||
|
|
||||||
manager
|
manager
|
||||||
@@ -115,6 +124,7 @@ impl ToolHandler for UnifiedExecHandler {
|
|||||||
login: args.login,
|
login: args.login,
|
||||||
yield_time_ms: args.yield_time_ms,
|
yield_time_ms: args.yield_time_ms,
|
||||||
max_output_tokens: args.max_output_tokens,
|
max_output_tokens: args.max_output_tokens,
|
||||||
|
workdir,
|
||||||
},
|
},
|
||||||
&context,
|
&context,
|
||||||
)
|
)
|
||||||
|
|||||||
@@ -138,6 +138,15 @@ fn create_exec_command_tool() -> ToolSpec {
|
|||||||
description: Some("Shell command to execute.".to_string()),
|
description: Some("Shell command to execute.".to_string()),
|
||||||
},
|
},
|
||||||
);
|
);
|
||||||
|
properties.insert(
|
||||||
|
"workdir".to_string(),
|
||||||
|
JsonSchema::String {
|
||||||
|
description: Some(
|
||||||
|
"Optional working directory to run the command in; defaults to the turn cwd."
|
||||||
|
.to_string(),
|
||||||
|
),
|
||||||
|
},
|
||||||
|
);
|
||||||
properties.insert(
|
properties.insert(
|
||||||
"shell".to_string(),
|
"shell".to_string(),
|
||||||
JsonSchema::String {
|
JsonSchema::String {
|
||||||
|
|||||||
@@ -70,6 +70,7 @@ pub(crate) struct ExecCommandRequest<'a> {
|
|||||||
pub login: bool,
|
pub login: bool,
|
||||||
pub yield_time_ms: Option<u64>,
|
pub yield_time_ms: Option<u64>,
|
||||||
pub max_output_tokens: Option<usize>,
|
pub max_output_tokens: Option<usize>,
|
||||||
|
pub workdir: Option<PathBuf>,
|
||||||
}
|
}
|
||||||
|
|
||||||
#[derive(Debug)]
|
#[derive(Debug)]
|
||||||
@@ -199,6 +200,7 @@ mod tests {
|
|||||||
login: true,
|
login: true,
|
||||||
yield_time_ms,
|
yield_time_ms,
|
||||||
max_output_tokens: None,
|
max_output_tokens: None,
|
||||||
|
workdir: None,
|
||||||
},
|
},
|
||||||
&context,
|
&context,
|
||||||
)
|
)
|
||||||
|
|||||||
@@ -1,3 +1,4 @@
|
|||||||
|
use std::path::PathBuf;
|
||||||
use std::sync::Arc;
|
use std::sync::Arc;
|
||||||
|
|
||||||
use tokio::sync::Notify;
|
use tokio::sync::Notify;
|
||||||
@@ -38,6 +39,10 @@ impl UnifiedExecSessionManager {
|
|||||||
request: ExecCommandRequest<'_>,
|
request: ExecCommandRequest<'_>,
|
||||||
context: &UnifiedExecContext,
|
context: &UnifiedExecContext,
|
||||||
) -> Result<UnifiedExecResponse, UnifiedExecError> {
|
) -> Result<UnifiedExecResponse, UnifiedExecError> {
|
||||||
|
let cwd = request
|
||||||
|
.workdir
|
||||||
|
.clone()
|
||||||
|
.unwrap_or_else(|| context.turn.cwd.clone());
|
||||||
let shell_flag = if request.login { "-lc" } else { "-c" };
|
let shell_flag = if request.login { "-lc" } else { "-c" };
|
||||||
let command = vec![
|
let command = vec![
|
||||||
request.shell.to_string(),
|
request.shell.to_string(),
|
||||||
@@ -45,7 +50,9 @@ impl UnifiedExecSessionManager {
|
|||||||
request.command.to_string(),
|
request.command.to_string(),
|
||||||
];
|
];
|
||||||
|
|
||||||
let session = self.open_session_with_sandbox(command, context).await?;
|
let session = self
|
||||||
|
.open_session_with_sandbox(command, cwd.clone(), context)
|
||||||
|
.await?;
|
||||||
|
|
||||||
let max_tokens = resolve_max_tokens(request.max_output_tokens);
|
let max_tokens = resolve_max_tokens(request.max_output_tokens);
|
||||||
let yield_time_ms =
|
let yield_time_ms =
|
||||||
@@ -66,7 +73,7 @@ impl UnifiedExecSessionManager {
|
|||||||
None
|
None
|
||||||
} else {
|
} else {
|
||||||
Some(
|
Some(
|
||||||
self.store_session(session, context, request.command, start)
|
self.store_session(session, context, request.command, cwd.clone(), start)
|
||||||
.await,
|
.await,
|
||||||
)
|
)
|
||||||
};
|
};
|
||||||
@@ -87,6 +94,7 @@ impl UnifiedExecSessionManager {
|
|||||||
Self::emit_exec_end_from_context(
|
Self::emit_exec_end_from_context(
|
||||||
context,
|
context,
|
||||||
request.command.to_string(),
|
request.command.to_string(),
|
||||||
|
cwd,
|
||||||
response.output.clone(),
|
response.output.clone(),
|
||||||
exit,
|
exit,
|
||||||
response.wall_time,
|
response.wall_time,
|
||||||
@@ -211,6 +219,7 @@ impl UnifiedExecSessionManager {
|
|||||||
session: UnifiedExecSession,
|
session: UnifiedExecSession,
|
||||||
context: &UnifiedExecContext,
|
context: &UnifiedExecContext,
|
||||||
command: &str,
|
command: &str,
|
||||||
|
cwd: PathBuf,
|
||||||
started_at: Instant,
|
started_at: Instant,
|
||||||
) -> i32 {
|
) -> i32 {
|
||||||
let session_id = self
|
let session_id = self
|
||||||
@@ -222,7 +231,7 @@ impl UnifiedExecSessionManager {
|
|||||||
turn_ref: Arc::clone(&context.turn),
|
turn_ref: Arc::clone(&context.turn),
|
||||||
call_id: context.call_id.clone(),
|
call_id: context.call_id.clone(),
|
||||||
command: command.to_string(),
|
command: command.to_string(),
|
||||||
cwd: context.turn.cwd.clone(),
|
cwd,
|
||||||
started_at,
|
started_at,
|
||||||
};
|
};
|
||||||
self.sessions.lock().await.insert(session_id, entry);
|
self.sessions.lock().await.insert(session_id, entry);
|
||||||
@@ -258,6 +267,7 @@ impl UnifiedExecSessionManager {
|
|||||||
async fn emit_exec_end_from_context(
|
async fn emit_exec_end_from_context(
|
||||||
context: &UnifiedExecContext,
|
context: &UnifiedExecContext,
|
||||||
command: String,
|
command: String,
|
||||||
|
cwd: PathBuf,
|
||||||
aggregated_output: String,
|
aggregated_output: String,
|
||||||
exit_code: i32,
|
exit_code: i32,
|
||||||
duration: Duration,
|
duration: Duration,
|
||||||
@@ -276,7 +286,7 @@ impl UnifiedExecSessionManager {
|
|||||||
&context.call_id,
|
&context.call_id,
|
||||||
None,
|
None,
|
||||||
);
|
);
|
||||||
let emitter = ToolEmitter::unified_exec(command, context.turn.cwd.clone(), true);
|
let emitter = ToolEmitter::unified_exec(command, cwd, true);
|
||||||
emitter
|
emitter
|
||||||
.emit(event_ctx, ToolEventStage::Success(output))
|
.emit(event_ctx, ToolEventStage::Success(output))
|
||||||
.await;
|
.await;
|
||||||
@@ -300,13 +310,14 @@ impl UnifiedExecSessionManager {
|
|||||||
pub(super) async fn open_session_with_sandbox(
|
pub(super) async fn open_session_with_sandbox(
|
||||||
&self,
|
&self,
|
||||||
command: Vec<String>,
|
command: Vec<String>,
|
||||||
|
cwd: PathBuf,
|
||||||
context: &UnifiedExecContext,
|
context: &UnifiedExecContext,
|
||||||
) -> Result<UnifiedExecSession, UnifiedExecError> {
|
) -> Result<UnifiedExecSession, UnifiedExecError> {
|
||||||
let mut orchestrator = ToolOrchestrator::new();
|
let mut orchestrator = ToolOrchestrator::new();
|
||||||
let mut runtime = UnifiedExecRuntime::new(self);
|
let mut runtime = UnifiedExecRuntime::new(self);
|
||||||
let req = UnifiedExecToolRequest::new(
|
let req = UnifiedExecToolRequest::new(
|
||||||
command,
|
command,
|
||||||
context.turn.cwd.clone(),
|
cwd,
|
||||||
create_env(&context.turn.shell_environment_policy),
|
create_env(&context.turn.shell_environment_policy),
|
||||||
);
|
);
|
||||||
let tool_ctx = ToolCtx {
|
let tool_ctx = ToolCtx {
|
||||||
|
|||||||
@@ -223,6 +223,90 @@ async fn unified_exec_emits_exec_command_begin_event() -> Result<()> {
|
|||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||||
|
async fn unified_exec_respects_workdir_override() -> Result<()> {
|
||||||
|
skip_if_no_network!(Ok(()));
|
||||||
|
skip_if_sandbox!(Ok(()));
|
||||||
|
|
||||||
|
let server = start_mock_server().await;
|
||||||
|
|
||||||
|
let mut builder = test_codex().with_config(|config| {
|
||||||
|
config.use_experimental_unified_exec_tool = true;
|
||||||
|
config.features.enable(Feature::UnifiedExec);
|
||||||
|
});
|
||||||
|
let TestCodex {
|
||||||
|
codex,
|
||||||
|
cwd,
|
||||||
|
session_configured,
|
||||||
|
..
|
||||||
|
} = builder.build(&server).await?;
|
||||||
|
|
||||||
|
let workdir = cwd.path().join("uexec_workdir_test");
|
||||||
|
std::fs::create_dir_all(&workdir)?;
|
||||||
|
|
||||||
|
let call_id = "uexec-workdir";
|
||||||
|
let args = json!({
|
||||||
|
"cmd": "pwd",
|
||||||
|
"yield_time_ms": 250,
|
||||||
|
"workdir": workdir.to_string_lossy().to_string(),
|
||||||
|
});
|
||||||
|
|
||||||
|
let responses = vec![
|
||||||
|
sse(vec![
|
||||||
|
ev_response_created("resp-1"),
|
||||||
|
ev_function_call(call_id, "exec_command", &serde_json::to_string(&args)?),
|
||||||
|
ev_completed("resp-1"),
|
||||||
|
]),
|
||||||
|
sse(vec![
|
||||||
|
ev_response_created("resp-2"),
|
||||||
|
ev_assistant_message("msg-1", "finished"),
|
||||||
|
ev_completed("resp-2"),
|
||||||
|
]),
|
||||||
|
];
|
||||||
|
mount_sse_sequence(&server, responses).await;
|
||||||
|
|
||||||
|
let session_model = session_configured.model.clone();
|
||||||
|
|
||||||
|
codex
|
||||||
|
.submit(Op::UserTurn {
|
||||||
|
items: vec![UserInput::Text {
|
||||||
|
text: "run workdir test".into(),
|
||||||
|
}],
|
||||||
|
final_output_json_schema: None,
|
||||||
|
cwd: cwd.path().to_path_buf(),
|
||||||
|
approval_policy: AskForApproval::Never,
|
||||||
|
sandbox_policy: SandboxPolicy::DangerFullAccess,
|
||||||
|
model: session_model,
|
||||||
|
effort: None,
|
||||||
|
summary: ReasoningSummary::Auto,
|
||||||
|
})
|
||||||
|
.await?;
|
||||||
|
|
||||||
|
wait_for_event(&codex, |event| matches!(event, EventMsg::TaskComplete(_))).await;
|
||||||
|
|
||||||
|
let requests = server.received_requests().await.expect("recorded requests");
|
||||||
|
assert!(!requests.is_empty(), "expected at least one POST request");
|
||||||
|
|
||||||
|
let bodies = requests
|
||||||
|
.iter()
|
||||||
|
.map(|req| req.body_json::<Value>().expect("request json"))
|
||||||
|
.collect::<Vec<_>>();
|
||||||
|
|
||||||
|
let outputs = collect_tool_outputs(&bodies)?;
|
||||||
|
let output = outputs
|
||||||
|
.get(call_id)
|
||||||
|
.expect("missing exec_command workdir output");
|
||||||
|
let output_text = output.output.trim();
|
||||||
|
let output_canonical = std::fs::canonicalize(output_text)?;
|
||||||
|
let expected_canonical = std::fs::canonicalize(&workdir)?;
|
||||||
|
assert_eq!(
|
||||||
|
output_canonical, expected_canonical,
|
||||||
|
"pwd should reflect the requested workdir override"
|
||||||
|
);
|
||||||
|
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
|
|
||||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||||
async fn unified_exec_emits_exec_command_end_event() -> Result<()> {
|
async fn unified_exec_emits_exec_command_end_event() -> Result<()> {
|
||||||
skip_if_no_network!(Ok(()));
|
skip_if_no_network!(Ok(()));
|
||||||
|
|||||||
Reference in New Issue
Block a user