Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions codex-rs/core/src/tools/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ pub struct ExecCommandToolOutput {
pub process_id: Option<i32>,
pub exit_code: Option<i32>,
pub original_token_count: Option<usize>,
pub session_command: Option<Vec<String>>,
pub hook_command: Option<String>,
}

impl ToolOutput for ExecCommandToolOutput {
Expand All @@ -394,7 +394,7 @@ impl ToolOutput for ExecCommandToolOutput {
}

fn post_tool_use_response(&self, _call_id: &str, _payload: &ToolPayload) -> Option<JsonValue> {
if self.process_id.is_some() || self.session_command.is_none() {
if self.process_id.is_some() || self.hook_command.is_none() {
return None;
}

Expand Down
2 changes: 1 addition & 1 deletion codex-rs/core/src/tools/context_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@ fn exec_command_tool_output_formats_truncated_response() {
process_id: None,
exit_code: Some(0),
original_token_count: Some(10),
session_command: None,
hook_command: None,
}
.to_response_item("call-42", &payload);

Expand Down
3 changes: 2 additions & 1 deletion codex-rs/core/src/tools/handlers/apply_patch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -324,11 +324,12 @@ impl ToolHandler for ApplyPatchHandler {
&self,
call_id: &str,
payload: &ToolPayload,
result: &dyn ToolOutput,
result: &Self::Output,
) -> Option<PostToolUsePayload> {
let tool_response = result.post_tool_use_response(call_id, payload)?;
Some(PostToolUsePayload {
tool_name: HookToolName::apply_patch(),
tool_use_id: call_id.to_string(),
command: apply_patch_payload_command(payload)?,
tool_response,
})
Expand Down
1 change: 1 addition & 0 deletions codex-rs/core/src/tools/handlers/apply_patch_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ fn post_tool_use_payload_uses_patch_input_and_tool_output() {
handler.post_tool_use_payload("call-apply-patch", &payload, &output),
Some(PostToolUsePayload {
tool_name: HookToolName::apply_patch(),
tool_use_id: "call-apply-patch".to_string(),
command: patch.to_string(),
tool_response: json!("Success. Updated files."),
})
Expand Down
6 changes: 4 additions & 2 deletions codex-rs/core/src/tools/handlers/shell.rs
Original file line number Diff line number Diff line change
Expand Up @@ -216,11 +216,12 @@ impl ToolHandler for ShellHandler {
&self,
call_id: &str,
payload: &ToolPayload,
result: &dyn ToolOutput,
result: &Self::Output,
) -> Option<PostToolUsePayload> {
let tool_response = result.post_tool_use_response(call_id, payload)?;
Some(PostToolUsePayload {
tool_name: HookToolName::bash(),
tool_use_id: call_id.to_string(),
command: shell_payload_command(payload)?,
tool_response,
})
Expand Down Expand Up @@ -328,11 +329,12 @@ impl ToolHandler for ShellCommandHandler {
&self,
call_id: &str,
payload: &ToolPayload,
result: &dyn ToolOutput,
result: &Self::Output,
) -> Option<PostToolUsePayload> {
let tool_response = result.post_tool_use_response(call_id, payload)?;
Some(PostToolUsePayload {
tool_name: HookToolName::bash(),
tool_use_id: call_id.to_string(),
command: shell_command_payload_command(payload)?,
tool_response,
})
Expand Down
1 change: 1 addition & 0 deletions codex-rs/core/src/tools/handlers/shell_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,7 @@ fn build_post_tool_use_payload_uses_tool_output_wire_value() {
handler.post_tool_use_payload("call-42", &payload, &output),
Some(crate::tools::registry::PostToolUsePayload {
tool_name: HookToolName::bash(),
tool_use_id: "call-42".to_string(),
command: "printf shell command".to_string(),
tool_response: json!("shell output"),
})
Expand Down
30 changes: 16 additions & 14 deletions codex-rs/core/src/tools/handlers/unified_exec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -147,21 +147,23 @@ impl ToolHandler for UnifiedExecHandler {
&self,
call_id: &str,
payload: &ToolPayload,
result: &dyn ToolOutput,
result: &Self::Output,
) -> Option<PostToolUsePayload> {
let ToolPayload::Function { arguments } = payload else {
let ToolPayload::Function { .. } = payload else {
return None;
};

let args = parse_arguments::<ExecCommandArgs>(arguments).ok()?;
if args.tty {
return None;
}

let tool_response = result.post_tool_use_response(call_id, payload)?;
let command = result.hook_command.clone()?;
let tool_use_id = if result.event_call_id.is_empty() {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[question] when would this be empty?

and isn't it the same as call_id?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good ask - I think you're right that this should never happen. It’s only there as a defensive fallback because ExecCommandToolOutput has one synthetic constructor path (intercept_apply_patch) that doesn’t participate in the normal unified-exec lifecycle and leaves event_call_id empty. That path also never produces a PostToolUse payload, but I'll leave it in just in case

call_id.to_string()
} else {
result.event_call_id.clone()
};
let tool_response = result.post_tool_use_response(&tool_use_id, payload)?;
Some(PostToolUsePayload {
tool_name: HookToolName::bash(),
command: args.cmd,
tool_use_id,
command,
tool_response,
})
}
Expand Down Expand Up @@ -200,11 +202,12 @@ impl ToolHandler for UnifiedExecHandler {
"exec_command" => {
let cwd = resolve_workdir_base_path(&arguments, &context.turn.cwd)?;
let args: ExecCommandArgs = parse_arguments_with_base_path(&arguments, &cwd)?;
let hook_command = args.cmd.clone();
let workdir = context.turn.resolve_path(args.workdir.clone());
maybe_emit_implicit_skill_invocation(
session.as_ref(),
context.turn.as_ref(),
&args.cmd,
&hook_command,
&workdir,
)
.await;
Expand Down Expand Up @@ -313,17 +316,16 @@ impl ToolHandler for UnifiedExecHandler {
process_id: None,
exit_code: None,
original_token_count: None,
session_command: None,
hook_command: None,
});
}

emit_unified_exec_tty_metric(&turn.session_telemetry, tty);
let session_command = command.clone();
match manager
.exec_command(
ExecCommandRequest {
command,
hook_command: args.cmd,
hook_command: hook_command.clone(),
process_id,
yield_time_ms,
max_output_tokens,
Expand Down Expand Up @@ -357,7 +359,7 @@ impl ToolHandler for UnifiedExecHandler {
process_id: None,
exit_code: Some(output.exit_code),
original_token_count: Some(original_token_count),
session_command: Some(session_command),
hook_command: Some(hook_command),
}
}
Err(err) => {
Expand Down
116 changes: 97 additions & 19 deletions codex-rs/core/src/tools/handlers/unified_exec_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -252,55 +252,53 @@ fn exec_command_post_tool_use_payload_uses_output_for_noninteractive_one_shot_co
arguments: serde_json::json!({ "cmd": "echo three", "tty": false }).to_string(),
};
let output = ExecCommandToolOutput {
event_call_id: "event-43".to_string(),
event_call_id: "call-43".to_string(),
chunk_id: "chunk-1".to_string(),
wall_time: std::time::Duration::from_millis(498),
raw_output: b"three".to_vec(),
max_output_tokens: None,
process_id: None,
exit_code: Some(0),
original_token_count: None,
session_command: Some(vec![
"/bin/zsh".to_string(),
"-lc".to_string(),
"echo three".to_string(),
]),
hook_command: Some("echo three".to_string()),
};

assert_eq!(
UnifiedExecHandler.post_tool_use_payload("call-43", &payload, &output),
Some(crate::tools::registry::PostToolUsePayload {
tool_name: HookToolName::bash(),
tool_use_id: "call-43".to_string(),
command: "echo three".to_string(),
tool_response: serde_json::json!("three"),
})
);
}

#[test]
fn exec_command_post_tool_use_payload_skips_interactive_exec() {
fn exec_command_post_tool_use_payload_uses_output_for_interactive_completion() {
let payload = ToolPayload::Function {
arguments: serde_json::json!({ "cmd": "echo three", "tty": true }).to_string(),
};
let output = ExecCommandToolOutput {
event_call_id: "event-44".to_string(),
event_call_id: "call-44".to_string(),
chunk_id: "chunk-1".to_string(),
wall_time: std::time::Duration::from_millis(498),
raw_output: b"three".to_vec(),
max_output_tokens: None,
process_id: None,
exit_code: Some(0),
original_token_count: None,
session_command: Some(vec![
"/bin/zsh".to_string(),
"-lc".to_string(),
"echo three".to_string(),
]),
hook_command: Some("echo three".to_string()),
};

assert_eq!(
UnifiedExecHandler.post_tool_use_payload("call-44", &payload, &output),
None
Some(crate::tools::registry::PostToolUsePayload {
tool_name: HookToolName::bash(),
tool_use_id: "call-44".to_string(),
command: "echo three".to_string(),
tool_response: serde_json::json!("three"),
})
);
}

Expand All @@ -318,15 +316,95 @@ fn exec_command_post_tool_use_payload_skips_running_sessions() {
process_id: Some(45),
exit_code: None,
original_token_count: None,
session_command: Some(vec![
"/bin/zsh".to_string(),
"-lc".to_string(),
"echo three".to_string(),
]),
hook_command: Some("echo three".to_string()),
};

assert_eq!(
UnifiedExecHandler.post_tool_use_payload("call-45", &payload, &output),
None
);
}

#[test]
fn write_stdin_post_tool_use_payload_uses_original_exec_call_id_and_command_on_completion() {
let payload = ToolPayload::Function {
arguments: serde_json::json!({
"session_id": 45,
"chars": "",
})
.to_string(),
};
let output = ExecCommandToolOutput {
event_call_id: "exec-call-45".to_string(),
chunk_id: "chunk-2".to_string(),
wall_time: std::time::Duration::from_millis(498),
raw_output: b"finished\n".to_vec(),
max_output_tokens: None,
process_id: None,
exit_code: Some(0),
original_token_count: None,
hook_command: Some("sleep 1; echo finished".to_string()),
};

assert_eq!(
UnifiedExecHandler.post_tool_use_payload("write-stdin-call", &payload, &output),
Some(crate::tools::registry::PostToolUsePayload {
tool_name: HookToolName::bash(),
tool_use_id: "exec-call-45".to_string(),
command: "sleep 1; echo finished".to_string(),
tool_response: serde_json::json!("finished\n"),
})
);
}

#[test]
fn write_stdin_post_tool_use_payload_keeps_parallel_session_metadata_separate() {
let payload = ToolPayload::Function {
arguments: serde_json::json!({ "session_id": 45, "chars": "" }).to_string(),
};
let output_a = ExecCommandToolOutput {
event_call_id: "exec-call-a".to_string(),
chunk_id: "chunk-a".to_string(),
wall_time: std::time::Duration::from_millis(498),
raw_output: b"alpha\n".to_vec(),
max_output_tokens: None,
process_id: None,
exit_code: Some(0),
original_token_count: None,
hook_command: Some("sleep 2; echo alpha".to_string()),
};
let output_b = ExecCommandToolOutput {
event_call_id: "exec-call-b".to_string(),
chunk_id: "chunk-b".to_string(),
wall_time: std::time::Duration::from_millis(498),
raw_output: b"beta\n".to_vec(),
max_output_tokens: None,
process_id: None,
exit_code: Some(0),
original_token_count: None,
hook_command: Some("sleep 1; echo beta".to_string()),
};

let payloads = [
UnifiedExecHandler.post_tool_use_payload("write-call-b", &payload, &output_b),
UnifiedExecHandler.post_tool_use_payload("write-call-a", &payload, &output_a),
];

assert_eq!(
payloads,
[
Some(crate::tools::registry::PostToolUsePayload {
tool_name: HookToolName::bash(),
tool_use_id: "exec-call-b".to_string(),
command: "sleep 1; echo beta".to_string(),
tool_response: serde_json::json!("beta\n"),
}),
Some(crate::tools::registry::PostToolUsePayload {
tool_name: HookToolName::bash(),
tool_use_id: "exec-call-a".to_string(),
command: "sleep 2; echo alpha".to_string(),
tool_response: serde_json::json!("alpha\n"),
}),
]
);
}
1 change: 1 addition & 0 deletions codex-rs/core/src/tools/parallel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ impl ToolCallRuntime {
result: Box::new(AbortedToolOutput {
message: Self::abort_message(call, secs),
}),
post_tool_use_payload: None,
}
}

Expand Down
Loading
Loading