mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
feat(core) RequestRule (#9489)
## Summary Instead of trying to derive the prefix_rule for a command mechanically, let's let the model decide for us. ## Testing - [x] tested locally
This commit is contained in:
@@ -6,6 +6,7 @@ use std::sync::Arc;
|
||||
use crate::codex::TurnContext;
|
||||
use crate::exec::ExecParams;
|
||||
use crate::exec_env::create_env;
|
||||
use crate::exec_policy::ExecApprovalRequest;
|
||||
use crate::function_tool::FunctionCallError;
|
||||
use crate::is_safe_command::is_known_safe_command;
|
||||
use crate::protocol::ExecCommandSource;
|
||||
@@ -28,16 +29,27 @@ pub struct ShellHandler;
|
||||
|
||||
pub struct ShellCommandHandler;
|
||||
|
||||
struct RunExecLikeArgs {
|
||||
tool_name: String,
|
||||
exec_params: ExecParams,
|
||||
prefix_rule: Option<Vec<String>>,
|
||||
session: Arc<crate::codex::Session>,
|
||||
turn: Arc<TurnContext>,
|
||||
tracker: crate::tools::context::SharedTurnDiffTracker,
|
||||
call_id: String,
|
||||
freeform: bool,
|
||||
}
|
||||
|
||||
impl ShellHandler {
|
||||
fn to_exec_params(params: ShellToolCallParams, turn_context: &TurnContext) -> ExecParams {
|
||||
fn to_exec_params(params: &ShellToolCallParams, turn_context: &TurnContext) -> ExecParams {
|
||||
ExecParams {
|
||||
command: params.command,
|
||||
command: params.command.clone(),
|
||||
cwd: turn_context.resolve_path(params.workdir.clone()),
|
||||
expiration: params.timeout_ms.into(),
|
||||
env: create_env(&turn_context.shell_environment_policy),
|
||||
sandbox_permissions: params.sandbox_permissions.unwrap_or_default(),
|
||||
windows_sandbox_level: turn_context.windows_sandbox_level,
|
||||
justification: params.justification,
|
||||
justification: params.justification.clone(),
|
||||
arg0: None,
|
||||
}
|
||||
}
|
||||
@@ -50,7 +62,7 @@ impl ShellCommandHandler {
|
||||
}
|
||||
|
||||
fn to_exec_params(
|
||||
params: ShellCommandToolCallParams,
|
||||
params: &ShellCommandToolCallParams,
|
||||
session: &crate::codex::Session,
|
||||
turn_context: &TurnContext,
|
||||
) -> ExecParams {
|
||||
@@ -64,7 +76,7 @@ impl ShellCommandHandler {
|
||||
env: create_env(&turn_context.shell_environment_policy),
|
||||
sandbox_permissions: params.sandbox_permissions.unwrap_or_default(),
|
||||
windows_sandbox_level: turn_context.windows_sandbox_level,
|
||||
justification: params.justification,
|
||||
justification: params.justification.clone(),
|
||||
arg0: None,
|
||||
}
|
||||
}
|
||||
@@ -108,29 +120,32 @@ impl ToolHandler for ShellHandler {
|
||||
match payload {
|
||||
ToolPayload::Function { arguments } => {
|
||||
let params: ShellToolCallParams = parse_arguments(&arguments)?;
|
||||
let exec_params = Self::to_exec_params(params, turn.as_ref());
|
||||
Self::run_exec_like(
|
||||
tool_name.as_str(),
|
||||
let prefix_rule = params.prefix_rule.clone();
|
||||
let exec_params = Self::to_exec_params(¶ms, turn.as_ref());
|
||||
Self::run_exec_like(RunExecLikeArgs {
|
||||
tool_name: tool_name.clone(),
|
||||
exec_params,
|
||||
prefix_rule,
|
||||
session,
|
||||
turn,
|
||||
tracker,
|
||||
call_id,
|
||||
false,
|
||||
)
|
||||
freeform: false,
|
||||
})
|
||||
.await
|
||||
}
|
||||
ToolPayload::LocalShell { params } => {
|
||||
let exec_params = Self::to_exec_params(params, turn.as_ref());
|
||||
Self::run_exec_like(
|
||||
tool_name.as_str(),
|
||||
let exec_params = Self::to_exec_params(¶ms, turn.as_ref());
|
||||
Self::run_exec_like(RunExecLikeArgs {
|
||||
tool_name: tool_name.clone(),
|
||||
exec_params,
|
||||
prefix_rule: None,
|
||||
session,
|
||||
turn,
|
||||
tracker,
|
||||
call_id,
|
||||
false,
|
||||
)
|
||||
freeform: false,
|
||||
})
|
||||
.await
|
||||
}
|
||||
_ => Err(FunctionCallError::RespondToModel(format!(
|
||||
@@ -181,30 +196,43 @@ impl ToolHandler for ShellCommandHandler {
|
||||
};
|
||||
|
||||
let params: ShellCommandToolCallParams = parse_arguments(&arguments)?;
|
||||
let exec_params = Self::to_exec_params(params, session.as_ref(), turn.as_ref());
|
||||
ShellHandler::run_exec_like(
|
||||
tool_name.as_str(),
|
||||
let prefix_rule = params.prefix_rule.clone();
|
||||
let exec_params = Self::to_exec_params(¶ms, session.as_ref(), turn.as_ref());
|
||||
ShellHandler::run_exec_like(RunExecLikeArgs {
|
||||
tool_name,
|
||||
exec_params,
|
||||
prefix_rule,
|
||||
session,
|
||||
turn,
|
||||
tracker,
|
||||
call_id,
|
||||
true,
|
||||
)
|
||||
freeform: true,
|
||||
})
|
||||
.await
|
||||
}
|
||||
}
|
||||
|
||||
impl ShellHandler {
|
||||
async fn run_exec_like(
|
||||
tool_name: &str,
|
||||
exec_params: ExecParams,
|
||||
session: Arc<crate::codex::Session>,
|
||||
turn: Arc<TurnContext>,
|
||||
tracker: crate::tools::context::SharedTurnDiffTracker,
|
||||
call_id: String,
|
||||
freeform: bool,
|
||||
) -> Result<ToolOutput, FunctionCallError> {
|
||||
async fn run_exec_like(args: RunExecLikeArgs) -> Result<ToolOutput, FunctionCallError> {
|
||||
let RunExecLikeArgs {
|
||||
tool_name,
|
||||
exec_params,
|
||||
prefix_rule,
|
||||
session,
|
||||
turn,
|
||||
tracker,
|
||||
call_id,
|
||||
freeform,
|
||||
} = args;
|
||||
|
||||
let features = session.features();
|
||||
let request_rule_enabled = features.enabled(crate::features::Feature::RequestRule);
|
||||
let prefix_rule = if request_rule_enabled {
|
||||
prefix_rule
|
||||
} else {
|
||||
None
|
||||
};
|
||||
|
||||
// Approval policy guard for explicit escalation in non-OnRequest modes.
|
||||
if exec_params
|
||||
.sandbox_permissions
|
||||
@@ -214,9 +242,9 @@ impl ShellHandler {
|
||||
codex_protocol::protocol::AskForApproval::OnRequest
|
||||
)
|
||||
{
|
||||
let approval_policy = turn.approval_policy;
|
||||
return Err(FunctionCallError::RespondToModel(format!(
|
||||
"approval policy is {policy:?}; reject command — you should not ask for escalated permissions if the approval policy is {policy:?}",
|
||||
policy = turn.approval_policy
|
||||
"approval policy is {approval_policy:?}; reject command — you should not ask for escalated permissions if the approval policy is {approval_policy:?}"
|
||||
)));
|
||||
}
|
||||
|
||||
@@ -229,7 +257,7 @@ impl ShellHandler {
|
||||
turn.as_ref(),
|
||||
Some(&tracker),
|
||||
&call_id,
|
||||
tool_name,
|
||||
tool_name.as_str(),
|
||||
)
|
||||
.await?
|
||||
{
|
||||
@@ -246,17 +274,17 @@ impl ShellHandler {
|
||||
let event_ctx = ToolEventCtx::new(session.as_ref(), turn.as_ref(), &call_id, None);
|
||||
emitter.begin(event_ctx).await;
|
||||
|
||||
let features = session.features();
|
||||
let exec_approval_requirement = session
|
||||
.services
|
||||
.exec_policy
|
||||
.create_exec_approval_requirement_for_command(
|
||||
&features,
|
||||
&exec_params.command,
|
||||
turn.approval_policy,
|
||||
&turn.sandbox_policy,
|
||||
exec_params.sandbox_permissions,
|
||||
)
|
||||
.create_exec_approval_requirement_for_command(ExecApprovalRequest {
|
||||
features: &features,
|
||||
command: &exec_params.command,
|
||||
approval_policy: turn.approval_policy,
|
||||
sandbox_policy: &turn.sandbox_policy,
|
||||
sandbox_permissions: exec_params.sandbox_permissions,
|
||||
prefix_rule,
|
||||
})
|
||||
.await;
|
||||
|
||||
let req = ShellRequest {
|
||||
@@ -274,7 +302,7 @@ impl ShellHandler {
|
||||
session: session.as_ref(),
|
||||
turn: turn.as_ref(),
|
||||
call_id: call_id.clone(),
|
||||
tool_name: tool_name.to_string(),
|
||||
tool_name,
|
||||
};
|
||||
let out = orchestrator
|
||||
.run(&mut runtime, &req, &tool_ctx, &turn, turn.approval_policy)
|
||||
@@ -377,10 +405,11 @@ mod tests {
|
||||
login,
|
||||
timeout_ms,
|
||||
sandbox_permissions: Some(sandbox_permissions),
|
||||
prefix_rule: None,
|
||||
justification: justification.clone(),
|
||||
};
|
||||
|
||||
let exec_params = ShellCommandHandler::to_exec_params(params, &session, &turn_context);
|
||||
let exec_params = ShellCommandHandler::to_exec_params(¶ms, &session, &turn_context);
|
||||
|
||||
// ExecParams cannot derive Eq due to the CancellationToken field, so we manually compare the fields.
|
||||
assert_eq!(exec_params.command, expected_command);
|
||||
|
||||
@@ -43,6 +43,8 @@ struct ExecCommandArgs {
|
||||
sandbox_permissions: SandboxPermissions,
|
||||
#[serde(default)]
|
||||
justification: Option<String>,
|
||||
#[serde(default)]
|
||||
prefix_rule: Option<Vec<String>>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Deserialize)]
|
||||
@@ -135,19 +137,28 @@ impl ToolHandler for UnifiedExecHandler {
|
||||
max_output_tokens,
|
||||
sandbox_permissions,
|
||||
justification,
|
||||
prefix_rule,
|
||||
..
|
||||
} = args;
|
||||
|
||||
let features = session.features();
|
||||
let request_rule_enabled = features.enabled(crate::features::Feature::RequestRule);
|
||||
let prefix_rule = if request_rule_enabled {
|
||||
prefix_rule
|
||||
} else {
|
||||
None
|
||||
};
|
||||
|
||||
if sandbox_permissions.requires_escalated_permissions()
|
||||
&& !matches!(
|
||||
context.turn.approval_policy,
|
||||
codex_protocol::protocol::AskForApproval::OnRequest
|
||||
)
|
||||
{
|
||||
let approval_policy = context.turn.approval_policy;
|
||||
manager.release_process_id(&process_id).await;
|
||||
return Err(FunctionCallError::RespondToModel(format!(
|
||||
"approval policy is {policy:?}; reject command — you cannot ask for escalated permissions if the approval policy is {policy:?}",
|
||||
policy = context.turn.approval_policy
|
||||
"approval policy is {approval_policy:?}; reject command — you cannot ask for escalated permissions if the approval policy is {approval_policy:?}"
|
||||
)));
|
||||
}
|
||||
|
||||
@@ -183,6 +194,7 @@ impl ToolHandler for UnifiedExecHandler {
|
||||
tty,
|
||||
sandbox_permissions,
|
||||
justification,
|
||||
prefix_rule,
|
||||
},
|
||||
&context,
|
||||
)
|
||||
|
||||
@@ -114,6 +114,7 @@ impl ToolRouter {
|
||||
workdir: exec.working_directory,
|
||||
timeout_ms: exec.timeout_ms,
|
||||
sandbox_permissions: Some(SandboxPermissions::UseDefault),
|
||||
prefix_rule: None,
|
||||
justification: None,
|
||||
};
|
||||
Ok(Some(ToolCall {
|
||||
|
||||
@@ -30,6 +30,7 @@ pub(crate) struct ToolsConfig {
|
||||
pub web_search_mode: Option<WebSearchMode>,
|
||||
pub collab_tools: bool,
|
||||
pub collaboration_modes_tools: bool,
|
||||
pub request_rule_enabled: bool,
|
||||
pub experimental_supported_tools: Vec<String>,
|
||||
}
|
||||
|
||||
@@ -49,6 +50,7 @@ impl ToolsConfig {
|
||||
let include_apply_patch_tool = features.enabled(Feature::ApplyPatchFreeform);
|
||||
let include_collab_tools = features.enabled(Feature::Collab);
|
||||
let include_collaboration_modes_tools = features.enabled(Feature::CollaborationModes);
|
||||
let request_rule_enabled = features.enabled(Feature::RequestRule);
|
||||
|
||||
let shell_type = if !features.enabled(Feature::ShellTool) {
|
||||
ConfigShellToolType::Disabled
|
||||
@@ -81,6 +83,7 @@ impl ToolsConfig {
|
||||
web_search_mode: *web_search_mode,
|
||||
collab_tools: include_collab_tools,
|
||||
collaboration_modes_tools: include_collaboration_modes_tools,
|
||||
request_rule_enabled,
|
||||
experimental_supported_tools: model_info.experimental_supported_tools.clone(),
|
||||
}
|
||||
}
|
||||
@@ -142,8 +145,50 @@ impl From<JsonSchema> for AdditionalProperties {
|
||||
}
|
||||
}
|
||||
|
||||
fn create_exec_command_tool() -> ToolSpec {
|
||||
let properties = BTreeMap::from([
|
||||
fn create_approval_parameters(include_prefix_rule: bool) -> BTreeMap<String, JsonSchema> {
|
||||
let mut properties = BTreeMap::from([
|
||||
(
|
||||
"sandbox_permissions".to_string(),
|
||||
JsonSchema::String {
|
||||
description: Some(
|
||||
"Sandbox permissions for the command. Set to \"require_escalated\" to request running without sandbox restrictions; defaults to \"use_default\"."
|
||||
.to_string(),
|
||||
),
|
||||
},
|
||||
),
|
||||
(
|
||||
"justification".to_string(),
|
||||
JsonSchema::String {
|
||||
description: Some(
|
||||
r#"Only set if sandbox_permissions is \"require_escalated\".
|
||||
Request approval from the user to run this command outside the sandbox.
|
||||
Phrased as a simple question that summarizes the purpose of the
|
||||
command as it relates to the task at hand - e.g. 'Do you want to
|
||||
fetch and pull the latest version of this git branch?'"#
|
||||
.to_string(),
|
||||
),
|
||||
},
|
||||
),
|
||||
]);
|
||||
|
||||
if include_prefix_rule {
|
||||
properties.insert(
|
||||
"prefix_rule".to_string(),
|
||||
JsonSchema::Array {
|
||||
items: Box::new(JsonSchema::String { description: None }),
|
||||
description: Some(
|
||||
r#"Only specify when sandbox_permissions is `require_escalated`.
|
||||
Suggest a prefix command pattern that will allow you to fulfill similar requests from the user in the future.
|
||||
Should be a short but reasonable prefix, e.g. [\"git\", \"pull\"] or [\"uv\", \"run\"] or [\"pytest\"]."#.to_string(),
|
||||
),
|
||||
});
|
||||
}
|
||||
|
||||
properties
|
||||
}
|
||||
|
||||
fn create_exec_command_tool(include_prefix_rule: bool) -> ToolSpec {
|
||||
let mut properties = BTreeMap::from([
|
||||
(
|
||||
"cmd".to_string(),
|
||||
JsonSchema::String {
|
||||
@@ -199,25 +244,8 @@ fn create_exec_command_tool() -> ToolSpec {
|
||||
),
|
||||
},
|
||||
),
|
||||
(
|
||||
"sandbox_permissions".to_string(),
|
||||
JsonSchema::String {
|
||||
description: Some(
|
||||
"Sandbox permissions for the command. Set to \"require_escalated\" to request running without sandbox restrictions; defaults to \"use_default\"."
|
||||
.to_string(),
|
||||
),
|
||||
},
|
||||
),
|
||||
(
|
||||
"justification".to_string(),
|
||||
JsonSchema::String {
|
||||
description: Some(
|
||||
"Only set if sandbox_permissions is \"require_escalated\". 1-sentence explanation of why we want to run this command."
|
||||
.to_string(),
|
||||
),
|
||||
},
|
||||
),
|
||||
]);
|
||||
properties.extend(create_approval_parameters(include_prefix_rule));
|
||||
|
||||
ToolSpec::Function(ResponsesApiTool {
|
||||
name: "exec_command".to_string(),
|
||||
@@ -280,8 +308,8 @@ fn create_write_stdin_tool() -> ToolSpec {
|
||||
})
|
||||
}
|
||||
|
||||
fn create_shell_tool() -> ToolSpec {
|
||||
let properties = BTreeMap::from([
|
||||
fn create_shell_tool(include_prefix_rule: bool) -> ToolSpec {
|
||||
let mut properties = BTreeMap::from([
|
||||
(
|
||||
"command".to_string(),
|
||||
JsonSchema::Array {
|
||||
@@ -301,19 +329,8 @@ fn create_shell_tool() -> ToolSpec {
|
||||
description: Some("The timeout for the command in milliseconds".to_string()),
|
||||
},
|
||||
),
|
||||
(
|
||||
"sandbox_permissions".to_string(),
|
||||
JsonSchema::String {
|
||||
description: Some("Sandbox permissions for the command. Set to \"require_escalated\" to request running without sandbox restrictions; defaults to \"use_default\".".to_string()),
|
||||
},
|
||||
),
|
||||
(
|
||||
"justification".to_string(),
|
||||
JsonSchema::String {
|
||||
description: Some("Only set if sandbox_permissions is \"require_escalated\". 1-sentence explanation of why we want to run this command.".to_string()),
|
||||
},
|
||||
),
|
||||
]);
|
||||
properties.extend(create_approval_parameters(include_prefix_rule));
|
||||
|
||||
let description = if cfg!(windows) {
|
||||
r#"Runs a Powershell command (Windows) and returns its output. Arguments to `shell` will be passed to CreateProcessW(). Most commands should be prefixed with ["powershell.exe", "-Command"].
|
||||
@@ -344,8 +361,8 @@ Examples of valid command strings:
|
||||
})
|
||||
}
|
||||
|
||||
fn create_shell_command_tool() -> ToolSpec {
|
||||
let properties = BTreeMap::from([
|
||||
fn create_shell_command_tool(include_prefix_rule: bool) -> ToolSpec {
|
||||
let mut properties = BTreeMap::from([
|
||||
(
|
||||
"command".to_string(),
|
||||
JsonSchema::String {
|
||||
@@ -375,19 +392,8 @@ fn create_shell_command_tool() -> ToolSpec {
|
||||
description: Some("The timeout for the command in milliseconds".to_string()),
|
||||
},
|
||||
),
|
||||
(
|
||||
"sandbox_permissions".to_string(),
|
||||
JsonSchema::String {
|
||||
description: Some("Sandbox permissions for the command. Set to \"require_escalated\" to request running without sandbox restrictions; defaults to \"use_default\".".to_string()),
|
||||
},
|
||||
),
|
||||
(
|
||||
"justification".to_string(),
|
||||
JsonSchema::String {
|
||||
description: Some("Only set if sandbox_permissions is \"require_escalated\". 1-sentence explanation of why we want to run this command.".to_string()),
|
||||
},
|
||||
),
|
||||
]);
|
||||
properties.extend(create_approval_parameters(include_prefix_rule));
|
||||
|
||||
let description = if cfg!(windows) {
|
||||
r#"Runs a Powershell command (Windows) and returns its output.
|
||||
@@ -1292,13 +1298,13 @@ pub(crate) fn build_specs(
|
||||
|
||||
match &config.shell_type {
|
||||
ConfigShellToolType::Default => {
|
||||
builder.push_spec(create_shell_tool());
|
||||
builder.push_spec(create_shell_tool(config.request_rule_enabled));
|
||||
}
|
||||
ConfigShellToolType::Local => {
|
||||
builder.push_spec(ToolSpec::LocalShell {});
|
||||
}
|
||||
ConfigShellToolType::UnifiedExec => {
|
||||
builder.push_spec(create_exec_command_tool());
|
||||
builder.push_spec(create_exec_command_tool(config.request_rule_enabled));
|
||||
builder.push_spec(create_write_stdin_tool());
|
||||
builder.register_handler("exec_command", unified_exec_handler.clone());
|
||||
builder.register_handler("write_stdin", unified_exec_handler);
|
||||
@@ -1307,7 +1313,7 @@ pub(crate) fn build_specs(
|
||||
// Do nothing.
|
||||
}
|
||||
ConfigShellToolType::ShellCommand => {
|
||||
builder.push_spec(create_shell_command_tool());
|
||||
builder.push_spec(create_shell_command_tool(config.request_rule_enabled));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1579,7 +1585,7 @@ mod tests {
|
||||
// Build expected from the same helpers used by the builder.
|
||||
let mut expected: BTreeMap<String, ToolSpec> = BTreeMap::from([]);
|
||||
for spec in [
|
||||
create_exec_command_tool(),
|
||||
create_exec_command_tool(false),
|
||||
create_write_stdin_tool(),
|
||||
create_list_mcp_resources_tool(),
|
||||
create_list_mcp_resource_templates_tool(),
|
||||
@@ -2413,7 +2419,7 @@ mod tests {
|
||||
|
||||
#[test]
|
||||
fn test_shell_tool() {
|
||||
let tool = super::create_shell_tool();
|
||||
let tool = super::create_shell_tool(false);
|
||||
let ToolSpec::Function(ResponsesApiTool {
|
||||
description, name, ..
|
||||
}) = &tool
|
||||
@@ -2443,7 +2449,7 @@ Examples of valid command strings:
|
||||
|
||||
#[test]
|
||||
fn test_shell_command_tool() {
|
||||
let tool = super::create_shell_command_tool();
|
||||
let tool = super::create_shell_command_tool(false);
|
||||
let ToolSpec::Function(ResponsesApiTool {
|
||||
description, name, ..
|
||||
}) = &tool
|
||||
|
||||
Reference in New Issue
Block a user