Skip to content

Commit d4db354

Browse files
ericjutacodex
andcommitted
fix: stabilize upstream pick tests
Keep shell and unified exec hook payloads aligned with the upstream Bash hook contract while preserving the local tool_use_id behavior. Avoid races in exec-server late-output tests by waiting for actual retained output after process exit. Co-authored-by: Codex <noreply@openai.com>
1 parent fbc0ae5 commit d4db354

7 files changed

Lines changed: 47 additions & 27 deletions

File tree

codex-rs/core/src/tools/handlers/shell.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,7 @@ impl ToolHandler for ShellHandler {
206206

207207
fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option<PreToolUsePayload> {
208208
shell_payload_command(&invocation.payload).map(|command| PreToolUsePayload {
209-
tool_name: invocation.tool_name.display(),
209+
tool_name: "Bash".to_string(),
210210
command,
211211
agentmemory_input: None,
212212
agentmemory_capability: None,
@@ -221,7 +221,7 @@ impl ToolHandler for ShellHandler {
221221
let tool_response =
222222
result.post_tool_use_response(&invocation.call_id, &invocation.payload)?;
223223
Some(PostToolUsePayload {
224-
tool_name: invocation.tool_name.display(),
224+
tool_name: "Bash".to_string(),
225225
tool_use_id: invocation.call_id.clone(),
226226
command: shell_payload_command(&invocation.payload)?,
227227
tool_response,
@@ -321,7 +321,7 @@ impl ToolHandler for ShellCommandHandler {
321321

322322
fn pre_tool_use_payload(&self, invocation: &ToolInvocation) -> Option<PreToolUsePayload> {
323323
shell_command_payload_command(&invocation.payload).map(|command| PreToolUsePayload {
324-
tool_name: invocation.tool_name.display(),
324+
tool_name: "Bash".to_string(),
325325
command,
326326
agentmemory_input: None,
327327
agentmemory_capability: None,
@@ -336,7 +336,7 @@ impl ToolHandler for ShellCommandHandler {
336336
let tool_response =
337337
result.post_tool_use_response(&invocation.call_id, &invocation.payload)?;
338338
Some(PostToolUsePayload {
339-
tool_name: invocation.tool_name.display(),
339+
tool_name: "Bash".to_string(),
340340
tool_use_id: invocation.call_id.clone(),
341341
command: shell_command_payload_command(&invocation.payload)?,
342342
tool_response,

codex-rs/core/src/tools/handlers/shell_tests.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -231,7 +231,7 @@ async fn shell_pre_tool_use_payload_uses_joined_command() {
231231
payload,
232232
}),
233233
Some(crate::tools::registry::PreToolUsePayload {
234-
tool_name: "shell".to_string(),
234+
tool_name: "Bash".to_string(),
235235
command: "bash -lc 'printf hi'".to_string(),
236236
agentmemory_input: None,
237237
agentmemory_capability: None,
@@ -259,7 +259,7 @@ async fn shell_command_pre_tool_use_payload_uses_raw_command() {
259259
payload,
260260
}),
261261
Some(crate::tools::registry::PreToolUsePayload {
262-
tool_name: "shell_command".to_string(),
262+
tool_name: "Bash".to_string(),
263263
command: "printf shell command".to_string(),
264264
agentmemory_input: None,
265265
agentmemory_capability: None,
@@ -294,7 +294,7 @@ async fn build_post_tool_use_payload_uses_tool_output_wire_value() {
294294
assert_eq!(
295295
handler.post_tool_use_payload(&invocation, &output),
296296
Some(crate::tools::registry::PostToolUsePayload {
297-
tool_name: "shell_command".to_string(),
297+
tool_name: "Bash".to_string(),
298298
tool_use_id: "call-42".to_string(),
299299
command: "printf shell command".to_string(),
300300
tool_response: json!("shell output"),

codex-rs/core/src/tools/handlers/unified_exec.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -146,7 +146,7 @@ impl ToolHandler for UnifiedExecHandler {
146146
parse_arguments::<ExecCommandArgs>(arguments)
147147
.ok()
148148
.map(|args| PreToolUsePayload {
149-
tool_name: invocation.tool_name.display(),
149+
tool_name: "Bash".to_string(),
150150
command: args.cmd,
151151
agentmemory_input: None,
152152
agentmemory_capability: None,
@@ -170,7 +170,7 @@ impl ToolHandler for UnifiedExecHandler {
170170
};
171171
let tool_response = result.post_tool_use_response(&tool_use_id, &invocation.payload)?;
172172
Some(PostToolUsePayload {
173-
tool_name: invocation.tool_name.display(),
173+
tool_name: "Bash".to_string(),
174174
tool_use_id,
175175
command,
176176
tool_response,

codex-rs/core/src/tools/handlers/unified_exec_tests.rs

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -216,7 +216,7 @@ async fn exec_command_pre_tool_use_payload_uses_raw_command() {
216216
payload,
217217
}),
218218
Some(crate::tools::registry::PreToolUsePayload {
219-
tool_name: "exec_command".to_string(),
219+
tool_name: "Bash".to_string(),
220220
command: "printf exec command".to_string(),
221221
agentmemory_input: None,
222222
agentmemory_capability: None,
@@ -275,7 +275,7 @@ async fn exec_command_post_tool_use_payload_uses_output_for_noninteractive_one_s
275275
assert_eq!(
276276
UnifiedExecHandler.post_tool_use_payload(&invocation, &output),
277277
Some(crate::tools::registry::PostToolUsePayload {
278-
tool_name: "exec_command".to_string(),
278+
tool_name: "Bash".to_string(),
279279
tool_use_id: "call-43".to_string(),
280280
command: "echo three".to_string(),
281281
tool_response: serde_json::json!("three"),
@@ -313,7 +313,7 @@ async fn exec_command_post_tool_use_payload_uses_output_for_interactive_completi
313313
assert_eq!(
314314
UnifiedExecHandler.post_tool_use_payload(&invocation, &output),
315315
Some(crate::tools::registry::PostToolUsePayload {
316-
tool_name: "exec_command".to_string(),
316+
tool_name: "Bash".to_string(),
317317
tool_use_id: "call-44".to_string(),
318318
command: "echo three".to_string(),
319319
tool_response: serde_json::json!("three"),
@@ -388,7 +388,7 @@ async fn write_stdin_post_tool_use_payload_uses_original_exec_call_id_and_comman
388388
assert_eq!(
389389
UnifiedExecHandler.post_tool_use_payload(&invocation, &output),
390390
Some(crate::tools::registry::PostToolUsePayload {
391-
tool_name: "exec_command".to_string(),
391+
tool_name: "Bash".to_string(),
392392
tool_use_id: "exec-call-45".to_string(),
393393
command: "sleep 1; echo finished".to_string(),
394394
tool_response: serde_json::json!("finished\n"),
@@ -452,13 +452,13 @@ async fn write_stdin_post_tool_use_payload_keeps_parallel_session_metadata_separ
452452
payloads,
453453
[
454454
Some(crate::tools::registry::PostToolUsePayload {
455-
tool_name: "exec_command".to_string(),
455+
tool_name: "Bash".to_string(),
456456
tool_use_id: "exec-call-b".to_string(),
457457
command: "sleep 1; echo beta".to_string(),
458458
tool_response: serde_json::json!("beta\n"),
459459
}),
460460
Some(crate::tools::registry::PostToolUsePayload {
461-
tool_name: "exec_command".to_string(),
461+
tool_name: "Bash".to_string(),
462462
tool_use_id: "exec-call-a".to_string(),
463463
command: "sleep 2; echo alpha".to_string(),
464464
tool_response: serde_json::json!("alpha\n"),

codex-rs/core/src/tools/registry.rs

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -180,7 +180,7 @@ fn pre_tool_use_hooks_enabled(payload: &PreToolUsePayload) -> bool {
180180
payload.agentmemory_capability.is_some()
181181
|| matches!(
182182
payload.tool_name.as_str(),
183-
"shell" | "shell_command" | "local_shell" | "exec_command"
183+
"shell" | "shell_command" | "local_shell" | "exec_command" | "Bash"
184184
)
185185
}
186186

@@ -191,6 +191,7 @@ fn post_tool_use_hooks_enabled(payload: &PostToolUsePayload) -> bool {
191191
| "shell_command"
192192
| "local_shell"
193193
| "exec_command"
194+
| "Bash"
194195
| "Edit"
195196
| "Write"
196197
| "Read"

codex-rs/exec-server/src/local_process.rs

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -784,7 +784,7 @@ mod tests {
784784
.expect("send late stdout");
785785

786786
let late_response =
787-
read_process_until_change(&backend, &process.process_id, /*after_seq*/ Some(1)).await;
787+
read_process_until_chunk(&backend, &process.process_id, /*after_seq*/ Some(1)).await;
788788
assert_eq!(
789789
late_response.chunks,
790790
vec![ProcessOutputChunk {
@@ -952,6 +952,32 @@ mod tests {
952952
.expect("process read")
953953
}
954954

955+
async fn read_process_until_chunk(
956+
backend: &LocalProcess,
957+
process_id: &ProcessId,
958+
after_seq: Option<u64>,
959+
) -> ReadResponse {
960+
timeout(Duration::from_secs(1), async {
961+
loop {
962+
let response = backend
963+
.exec_read(ReadParams {
964+
process_id: process_id.clone(),
965+
after_seq,
966+
max_bytes: None,
967+
wait_ms: Some(50),
968+
})
969+
.await
970+
.expect("process read");
971+
if !response.chunks.is_empty() {
972+
break response;
973+
}
974+
tokio::time::sleep(Duration::from_millis(5)).await;
975+
}
976+
})
977+
.await
978+
.expect("process chunk read should finish")
979+
}
980+
955981
async fn read_process_until_closed(
956982
backend: &LocalProcess,
957983
process_id: &ProcessId,

codex-rs/exec-server/tests/exec_process.rs

Lines changed: 3 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -371,25 +371,18 @@ async fn assert_exec_process_retains_output_after_exit_until_streams_close(
371371
.next_seq
372372
.checked_sub(1)
373373
.context("exit response should advance next_seq")?;
374+
let mut wake_rx = process.subscribe_wake();
374375
std::fs::write(&release_path, b"go")?;
375376

376-
let late_response = timeout(
377-
Duration::from_secs(2),
378-
process.read(
379-
/*after_seq*/ Some(exit_seq),
380-
/*max_bytes*/ None,
381-
/*wait_ms*/ Some(2_000),
382-
),
383-
)
384-
.await??;
377+
let late_response =
378+
read_process_until_change(Arc::clone(&process), &mut wake_rx, Some(exit_seq)).await?;
385379
let mut late_output = String::new();
386380
for chunk in late_response.chunks {
387381
assert_eq!(chunk.stream, ExecOutputStream::Stdout);
388382
late_output.push_str(&String::from_utf8_lossy(&chunk.chunk.into_inner()));
389383
}
390384
assert_eq!(late_output, "late output after exit\n");
391385

392-
let wake_rx = process.subscribe_wake();
393386
let actual = collect_process_output_from_reads(process, wake_rx).await?;
394387
assert_eq!(
395388
actual,

0 commit comments

Comments
 (0)