agent/codex
2
0
Fork 0
mirror of https://github.com/pchuan98/codex.git synced 2026-07-01 00:31:56 +08:00
Code Issues Packages Projects Releases Wiki Activity

[codex] Remove unused legacy shell tools (#22246)

Browse Source 
## Why

Recent session history showed no active use of the raw `shell`,
`local_shell`, or `container.exec` execution surfaces. Keeping those
handlers/specs wired into core leaves duplicate shell execution paths
alongside the supported `shell_command` and unified exec tools.

## What changed

- Removed the raw `shell` handler/spec and its `ShellToolCallParams`
protocol helper.
- Removed the legacy `local_shell` and `container.exec` handler/spec
plumbing while preserving persisted-history compatibility for old
response items.
- Normalized model/config `default` and `local` shell selections to
`shell_command`.
- Pruned tests that exercised removed raw-shell/local-shell/apply-patch
variants and kept coverage on `shell_command`, unified exec, and
freeform `apply_patch`.

## Verification

- `git diff --check`
- `cargo test -p codex-protocol`
- `cargo test -p codex-tools`
- `cargo test -p codex-core tools::handlers::shell`
- `cargo test -p codex-core tools::spec`
- `cargo test -p codex-core tools::router`
- `cargo test -p codex-core
active_call_preserves_triggering_command_context`
- `cargo test -p codex-core guardian_tests`
- `cargo test -p codex-core --test all shell_serialization`
- `cargo test -p codex-core --test all apply_patch_cli`
- `cargo test -p codex-core --test all shell_command_`
- `cargo test -p codex-core --test all local_shell`
- `cargo test -p codex-core --test all otel::`
- `cargo test -p codex-core --test all hooks::`
- `just fix -p codex-core`
- `just fix -p codex-tools`
This commit is contained in:
pakrym-oai
2026-05-13 09:43:25 -07:00
committed by GitHub
Unverified
parent 7c7b4861d8
commit 83decfa300
47 changed files with 205 additions and 1981 deletions
Download Patch File Download Diff File Expand all files Collapse all files
codex-rs/core/src/session/tests.rs
+20 -43
View File
@@ -4,7 +4,6 @@ use crate::config::ConfigBuilder;
use crate::config::test_config;
use crate::context::ContextualUserFragment;
use crate::context::TurnAborted;
use crate::exec::ExecCapturePolicy;
use crate::function_tool::FunctionCallError;
use crate::shell::default_user_shell;
use crate::skills::SkillRenderSideEffects;
@@ -69,7 +68,7 @@ use crate::tools::context::ToolInvocation;
use crate::tools::context::ToolPayload;
use crate::tools::handlers::CreateGoalHandler;
use crate::tools::handlers::ExecCommandHandler;
use crate::tools::handlers::ShellHandler;
use crate::tools::handlers::ShellCommandHandler;
use crate::tools::handlers::UpdateGoalHandler;
use crate::tools::registry::ToolExecutor;
use crate::tools::router::ToolCallSource;
@@ -8317,7 +8316,7 @@ async fn budget_limited_accounting_steers_active_turn_without_aborting() -> anyh
sess.goal_runtime_apply(GoalRuntimeEvent::ToolCompleted {
turn_context: tc.as_ref(),
tool_name: "shell",
tool_name: "shell_command",
})
.await?;
@@ -8553,7 +8552,7 @@ async fn external_active_goal_set_marks_current_turn_for_accounting() -> anyhow:
.await;
sess.goal_runtime_apply(GoalRuntimeEvent::ToolCompleted {
turn_context: tc.as_ref(),
tool_name: "shell",
tool_name: "shell_command",
})
.await?;
@@ -8984,7 +8983,7 @@ async fn fatal_tool_error_stops_turn_and_reports_error() {
id: None,
status: None,
call_id: "call-1".to_string(),
name: "shell".to_string(),
name: "shell_command".to_string(),
input: "{}".to_string(),
};
@@ -9007,7 +9006,10 @@ async fn fatal_tool_error_stops_turn_and_reports_error() {
match err {
FunctionCallError::Fatal(message) => {
assert_eq!(message, "tool shell invoked with incompatible payload");
assert_eq!(
message,
"tool shell_command invoked with incompatible payload"
);
}
other => panic!("expected FunctionCallError::Fatal, got {other:?}"),
}
@@ -9353,13 +9355,12 @@ async fn update_goal_tool_marks_goal_complete() {
#[tokio::test]
async fn rejects_escalated_permissions_when_policy_not_on_request() {
use crate::exec::ExecParams;
use crate::exec_policy::ExecApprovalRequest;
use crate::sandboxing::SandboxPermissions;
use crate::tools::sandboxing::ExecApprovalRequirement;
use crate::turn_diff_tracker::TurnDiffTracker;
use codex_protocol::protocol::AskForApproval;
use std::collections::HashMap;
use codex_tools::ShellCommandBackendConfig;
let (session, mut turn_context_raw) = make_session_and_context().await;
// Ensure policy is NOT OnRequest so the early rejection path triggers
@@ -9370,43 +9371,16 @@ async fn rejects_escalated_permissions_when_policy_not_on_request() {
let session = Arc::new(session);
let mut turn_context = Arc::new(turn_context_raw);
let command_script = "echo hi";
let timeout_ms = 1000;
let sandbox_permissions = SandboxPermissions::RequireEscalated;
let params = ExecParams {
command: if cfg!(windows) {
vec![
"cmd.exe".to_string(),
"/C".to_string(),
"echo hi".to_string(),
]
} else {
vec![
"/bin/sh".to_string(),
"-c".to_string(),
"echo hi".to_string(),
]
},
cwd: turn_context.cwd.clone(),
expiration: timeout_ms.into(),
capture_policy: ExecCapturePolicy::ShellTool,
env: HashMap::new(),
network: None,
sandbox_permissions,
windows_sandbox_level: turn_context.windows_sandbox_level,
windows_sandbox_private_desktop: turn_context
.config
.permissions
.windows_sandbox_private_desktop,
justification: Some("test".to_string()),
arg0: None,
};
let turn_diff_tracker = Arc::new(tokio::sync::Mutex::new(TurnDiffTracker::new()));
let tool_name = "shell";
let tool_name = "shell_command";
let call_id = "test-call".to_string();
let handler = ShellHandler::default();
let handler = ShellCommandHandler::from(ShellCommandBackendConfig::Classic);
let resp = handler
.handle(ToolInvocation {
session: Arc::clone(&session),
@@ -9418,11 +9392,11 @@ async fn rejects_escalated_permissions_when_policy_not_on_request() {
source: crate::tools::context::ToolCallSource::Direct,
payload: ToolPayload::Function {
arguments: serde_json::json!({
"command": params.command.clone(),
"command": command_script,
"workdir": Some(turn_context.cwd.to_string_lossy().to_string()),
"timeout_ms": params.expiration.timeout_ms(),
"sandbox_permissions": params.sandbox_permissions,
"justification": params.justification.clone(),
"timeout_ms": timeout_ms,
"sandbox_permissions": sandbox_permissions,
"justification": Some("test"),
})
.to_string(),
},
@@ -9448,11 +9422,14 @@ async fn rejects_escalated_permissions_when_policy_not_on_request() {
turn_context_mut.permission_profile = PermissionProfile::Disabled;
let file_system_sandbox_policy = turn_context.file_system_sandbox_policy();
let command = session
.user_shell()
.derive_exec_args(command_script, turn_context.tools_config.allow_login_shell);
let exec_approval_requirement = session
.services
.exec_policy
.create_exec_approval_requirement_for_command(ExecApprovalRequest {
command: &params.command,
command: &command,
approval_policy: turn_context.approval_policy.value(),
permission_profile: turn_context.permission_profile(),
file_system_sandbox_policy: &file_system_sandbox_policy,
codex-rs/core/src/session/tests/guardian_tests.rs
+27 -104
View File
@@ -1,8 +1,6 @@
use super::*;
use crate::compact::InitialContextInjection;
use crate::environment_selection::ResolvedTurnEnvironments;
use crate::exec::ExecCapturePolicy;
use crate::exec::ExecParams;
use crate::exec_policy::ExecPolicyManager;
use crate::guardian::GUARDIAN_REVIEWER_NAME;
use crate::sandboxing::SandboxPermissions;
@@ -43,8 +41,6 @@ use core_test_support::responses::sse;
use core_test_support::responses::sse_response;
use core_test_support::responses::start_mock_server;
use pretty_assertions::assert_eq;
use serde::Deserialize;
use std::collections::HashMap;
use std::fs;
use std::sync::Arc;
use std::time::Duration;
@@ -238,7 +234,7 @@ async fn request_permissions_guardian_review_stops_when_cancelled() {
}
#[tokio::test]
async fn guardian_allows_shell_additional_permissions_requests_past_policy_validation() {
async fn guardian_allows_shell_command_additional_permissions_requests_past_policy_validation() {
let server = start_mock_server().await;
let _request_log = mount_sse_once(
&server,
@@ -292,38 +288,9 @@ async fn guardian_allows_shell_additional_permissions_requests_past_policy_valid
let turn_context = Arc::new(turn_context_raw);
let expiration_ms: u64 = if cfg!(windows) { 2_500 } else { 1_000 };
let params = ExecParams {
command: if cfg!(windows) {
vec![
"cmd.exe".to_string(),
"/Q".to_string(),
"/D".to_string(),
"/C".to_string(),
"echo hi".to_string(),
]
} else {
vec![
"/bin/sh".to_string(),
"-c".to_string(),
"echo hi".to_string(),
]
},
cwd: turn_context.cwd.clone(),
expiration: expiration_ms.into(),
capture_policy: ExecCapturePolicy::ShellTool,
env: HashMap::new(),
network: None,
sandbox_permissions: SandboxPermissions::WithAdditionalPermissions,
windows_sandbox_level: turn_context.windows_sandbox_level,
windows_sandbox_private_desktop: turn_context
.config
.permissions
.windows_sandbox_private_desktop,
justification: Some("test".to_string()),
arg0: None,
};
let handler = ShellHandler::default();
let handler = crate::tools::handlers::ShellCommandHandler::from(
codex_tools::ShellCommandBackendConfig::Classic,
);
let resp = handler
.handle(ToolInvocation {
session: Arc::clone(&session),
@@ -331,21 +298,22 @@ async fn guardian_allows_shell_additional_permissions_requests_past_policy_valid
cancellation_token: CancellationToken::new(),
tracker: Arc::new(tokio::sync::Mutex::new(TurnDiffTracker::new())),
call_id: "test-call".to_string(),
tool_name: codex_tools::ToolName::plain("shell"),
tool_name: codex_tools::ToolName::plain("shell_command"),
source: crate::tools::context::ToolCallSource::Direct,
payload: ToolPayload::Function {
arguments: serde_json::json!({
"command": params.command.clone(),
"command": "echo hi",
"login": false,
"workdir": Some(turn_context.cwd.to_string_lossy().to_string()),
"timeout_ms": params.expiration.timeout_ms(),
"sandbox_permissions": params.sandbox_permissions,
"timeout_ms": expiration_ms,
"sandbox_permissions": SandboxPermissions::WithAdditionalPermissions,
"additional_permissions": PermissionProfile {
network: Some(NetworkPermissions {
enabled: Some(true),
}),
file_system: None,
},
"justification": params.justification.clone(),
"justification": Some("test"),
})
.to_string(),
},
@@ -353,27 +321,11 @@ async fn guardian_allows_shell_additional_permissions_requests_past_policy_valid
.await;
let output = expect_text_output(&resp.expect("expected Ok result"));
#[derive(Deserialize, PartialEq, Eq, Debug)]
struct ResponseExecMetadata {
exit_code: i32,
}
#[derive(Deserialize)]
struct ResponseExecOutput {
output: String,
metadata: ResponseExecMetadata,
}
let exec_output: ResponseExecOutput =
serde_json::from_str(&output).expect("valid exec output json");
assert_eq!(exec_output.metadata, ResponseExecMetadata { exit_code: 0 });
assert!(exec_output.output.contains("hi"));
assert!(output.contains("hi"));
}
#[tokio::test]
async fn strict_auto_review_turn_grant_forces_guardian_for_shell_policy_skip() {
async fn strict_auto_review_turn_grant_forces_guardian_for_shell_command_policy_skip() {
let server = start_mock_server().await;
let guardian_request_log = mount_sse_once(
&server,
@@ -437,34 +389,22 @@ async fn strict_auto_review_turn_grant_forces_guardian_for_shell_policy_skip() {
let session = Arc::new(session);
let turn_context = Arc::new(turn_context_raw);
let handler = ShellHandler::default();
let command = if cfg!(windows) {
vec![
"cmd.exe".to_string(),
"/Q".to_string(),
"/D".to_string(),
"/C".to_string(),
"echo hi".to_string(),
]
} else {
vec![
"/bin/sh".to_string(),
"-c".to_string(),
"echo hi".to_string(),
]
};
let handler = crate::tools::handlers::ShellCommandHandler::from(
codex_tools::ShellCommandBackendConfig::Classic,
);
let resp = handler
.handle(ToolInvocation {
session: Arc::clone(&session),
turn: Arc::clone(&turn_context),
cancellation_token: CancellationToken::new(),
tracker: Arc::new(tokio::sync::Mutex::new(TurnDiffTracker::new())),
call_id: "strict-shell-call".to_string(),
tool_name: codex_tools::ToolName::plain("shell"),
call_id: "strict-shell-command-call".to_string(),
tool_name: codex_tools::ToolName::plain("shell_command"),
source: ToolCallSource::Direct,
payload: ToolPayload::Function {
arguments: serde_json::json!({
"command": command,
"command": "echo hi",
"login": false,
"workdir": Some(turn_context.cwd.to_string_lossy().to_string()),
"timeout_ms": 1_000_u64,
})
@@ -593,7 +533,7 @@ async fn process_compacted_history_preserves_separate_guardian_developer_message
clippy::await_holding_invalid_type,
reason = "test mutates active turn state directly to seed granted permissions"
)]
async fn shell_handler_allows_sticky_turn_permissions_without_inline_request_permissions_feature() {
async fn shell_command_allows_sticky_turn_permissions_without_inline_request_permissions_feature() {
let (mut session, turn_context_raw) = make_session_and_context().await;
session
.features
@@ -615,7 +555,9 @@ async fn shell_handler_allows_sticky_turn_permissions_without_inline_request_per
let session = Arc::new(session);
let turn_context = Arc::new(turn_context_raw);
let handler = ShellHandler::default();
let handler = crate::tools::handlers::ShellCommandHandler::from(
codex_tools::ShellCommandBackendConfig::Classic,
);
let resp = handler
.handle(ToolInvocation {
session: Arc::clone(&session),
@@ -623,15 +565,12 @@ async fn shell_handler_allows_sticky_turn_permissions_without_inline_request_per
cancellation_token: CancellationToken::new(),
tracker: Arc::new(tokio::sync::Mutex::new(TurnDiffTracker::new())),
call_id: "sticky-turn-grant".to_string(),
tool_name: codex_tools::ToolName::plain("shell"),
tool_name: codex_tools::ToolName::plain("shell_command"),
source: crate::tools::context::ToolCallSource::Direct,
payload: ToolPayload::Function {
arguments: serde_json::json!({
"command": [
"/bin/sh",
"-c",
"echo hi",
],
"command": "echo hi",
"login": false,
"timeout_ms": 1_000_u64,
"workdir": Some(turn_context.cwd.to_string_lossy().to_string()),
})
@@ -643,23 +582,7 @@ async fn shell_handler_allows_sticky_turn_permissions_without_inline_request_per
match resp {
Ok(output) => {
let output = expect_text_output(&output);
#[derive(Deserialize, PartialEq, Eq, Debug)]
struct ResponseExecMetadata {
exit_code: i32,
}
#[derive(Deserialize)]
struct ResponseExecOutput {
output: String,
metadata: ResponseExecMetadata,
}
let exec_output: ResponseExecOutput =
serde_json::from_str(&output).expect("valid exec output json");
assert_eq!(exec_output.metadata, ResponseExecMetadata { exit_code: 0 });
assert!(exec_output.output.contains("hi"));
assert!(output.contains("hi"));
}
Err(FunctionCallError::RespondToModel(output)) => {
assert!(