chore(core) rm Feature::RequestRule (#11866)
## Summary This feature is now reasonably stable, let's remove it so we can simplify our upcoming iterations here. ## Testing - [x] Existing tests pass
This commit is contained in:
parent
5b421bba34
commit
19afbc35c1
10 changed files with 35 additions and 105 deletions
|
|
@ -477,7 +477,6 @@ fn assert_permissions_message(item: &ResponseItem) {
|
|||
&SandboxPolicy::DangerFullAccess,
|
||||
AskForApproval::Never,
|
||||
&Policy::empty(),
|
||||
false,
|
||||
&PathBuf::from("/tmp"),
|
||||
)
|
||||
.into_text();
|
||||
|
|
|
|||
|
|
@ -2004,7 +2004,6 @@ impl Session {
|
|||
&next.sandbox_policy,
|
||||
next.approval_policy,
|
||||
self.services.exec_policy.current().as_ref(),
|
||||
self.features.enabled(Feature::RequestRule),
|
||||
&next.cwd,
|
||||
)
|
||||
.into(),
|
||||
|
|
@ -2624,7 +2623,6 @@ impl Session {
|
|||
&turn_context.sandbox_policy,
|
||||
turn_context.approval_policy,
|
||||
self.services.exec_policy.current().as_ref(),
|
||||
self.features.enabled(Feature::RequestRule),
|
||||
&turn_context.cwd,
|
||||
)
|
||||
.into(),
|
||||
|
|
|
|||
|
|
@ -686,8 +686,6 @@ mod tests {
|
|||
use crate::config_loader::ConfigLayerStack;
|
||||
use crate::config_loader::ConfigRequirements;
|
||||
use crate::config_loader::ConfigRequirementsToml;
|
||||
use crate::features::Feature;
|
||||
use crate::features::Features;
|
||||
use codex_app_server_protocol::ConfigLayerSource;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
|
|
@ -1281,8 +1279,6 @@ prefix_rule(
|
|||
"cargo-insta".to_string(),
|
||||
];
|
||||
let manager = ExecPolicyManager::default();
|
||||
let mut features = Features::with_defaults();
|
||||
features.enable(Feature::RequestRule);
|
||||
|
||||
let requirement = manager
|
||||
.create_exec_approval_requirement_for_command(ExecApprovalRequest {
|
||||
|
|
|
|||
|
|
@ -511,8 +511,8 @@ pub const FEATURES: &[FeatureSpec] = &[
|
|||
FeatureSpec {
|
||||
id: Feature::RequestRule,
|
||||
key: "request_rule",
|
||||
stage: Stage::Stable,
|
||||
default_enabled: true,
|
||||
stage: Stage::Removed,
|
||||
default_enabled: false,
|
||||
},
|
||||
FeatureSpec {
|
||||
id: Feature::WindowsSandbox,
|
||||
|
|
|
|||
|
|
@ -243,14 +243,6 @@ impl ShellHandler {
|
|||
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
|
||||
};
|
||||
|
||||
let mut exec_params = exec_params;
|
||||
let dependency_env = session.dependency_env().await;
|
||||
if !dependency_env.is_empty() {
|
||||
|
|
|
|||
|
|
@ -142,14 +142,6 @@ impl ToolHandler for UnifiedExecHandler {
|
|||
..
|
||||
} = 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,
|
||||
|
|
|
|||
|
|
@ -41,7 +41,6 @@ pub(crate) struct ToolsConfig {
|
|||
pub js_repl_tools_only: bool,
|
||||
pub collab_tools: bool,
|
||||
pub collaboration_modes_tools: bool,
|
||||
pub request_rule_enabled: bool,
|
||||
pub experimental_supported_tools: Vec<String>,
|
||||
}
|
||||
|
||||
|
|
@ -64,7 +63,6 @@ impl ToolsConfig {
|
|||
include_js_repl && features.enabled(Feature::JsReplToolsOnly);
|
||||
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 include_search_tool = features.enabled(Feature::Apps);
|
||||
|
||||
let shell_type = if !features.enabled(Feature::ShellTool) {
|
||||
|
|
@ -101,7 +99,6 @@ impl ToolsConfig {
|
|||
js_repl_tools_only: include_js_repl_tools_only,
|
||||
collab_tools: include_collab_tools,
|
||||
collaboration_modes_tools: include_collaboration_modes_tools,
|
||||
request_rule_enabled,
|
||||
experimental_supported_tools: model_info.experimental_supported_tools.clone(),
|
||||
}
|
||||
}
|
||||
|
|
@ -174,7 +171,7 @@ impl From<JsonSchema> for AdditionalProperties {
|
|||
}
|
||||
}
|
||||
|
||||
fn create_approval_parameters(include_prefix_rule: bool) -> BTreeMap<String, JsonSchema> {
|
||||
fn create_approval_parameters() -> BTreeMap<String, JsonSchema> {
|
||||
let mut properties = BTreeMap::from([
|
||||
(
|
||||
"sandbox_permissions".to_string(),
|
||||
|
|
@ -200,23 +197,22 @@ fn create_approval_parameters(include_prefix_rule: bool) -> BTreeMap<String, Jso
|
|||
),
|
||||
]);
|
||||
|
||||
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`.
|
||||
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 {
|
||||
fn create_exec_command_tool() -> ToolSpec {
|
||||
let mut properties = BTreeMap::from([
|
||||
(
|
||||
"cmd".to_string(),
|
||||
|
|
@ -274,7 +270,7 @@ fn create_exec_command_tool(include_prefix_rule: bool) -> ToolSpec {
|
|||
},
|
||||
),
|
||||
]);
|
||||
properties.extend(create_approval_parameters(include_prefix_rule));
|
||||
properties.extend(create_approval_parameters());
|
||||
|
||||
ToolSpec::Function(ResponsesApiTool {
|
||||
name: "exec_command".to_string(),
|
||||
|
|
@ -337,7 +333,7 @@ fn create_write_stdin_tool() -> ToolSpec {
|
|||
})
|
||||
}
|
||||
|
||||
fn create_shell_tool(include_prefix_rule: bool) -> ToolSpec {
|
||||
fn create_shell_tool() -> ToolSpec {
|
||||
let mut properties = BTreeMap::from([
|
||||
(
|
||||
"command".to_string(),
|
||||
|
|
@ -359,7 +355,7 @@ fn create_shell_tool(include_prefix_rule: bool) -> ToolSpec {
|
|||
},
|
||||
),
|
||||
]);
|
||||
properties.extend(create_approval_parameters(include_prefix_rule));
|
||||
properties.extend(create_approval_parameters());
|
||||
|
||||
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"].
|
||||
|
|
@ -390,7 +386,7 @@ Examples of valid command strings:
|
|||
})
|
||||
}
|
||||
|
||||
fn create_shell_command_tool(include_prefix_rule: bool) -> ToolSpec {
|
||||
fn create_shell_command_tool() -> ToolSpec {
|
||||
let mut properties = BTreeMap::from([
|
||||
(
|
||||
"command".to_string(),
|
||||
|
|
@ -422,7 +418,7 @@ fn create_shell_command_tool(include_prefix_rule: bool) -> ToolSpec {
|
|||
},
|
||||
),
|
||||
]);
|
||||
properties.extend(create_approval_parameters(include_prefix_rule));
|
||||
properties.extend(create_approval_parameters());
|
||||
|
||||
let description = if cfg!(windows) {
|
||||
r#"Runs a Powershell command (Windows) and returns its output.
|
||||
|
|
@ -1442,19 +1438,13 @@ pub(crate) fn build_specs(
|
|||
|
||||
match &config.shell_type {
|
||||
ConfigShellToolType::Default => {
|
||||
builder.push_spec_with_parallel_support(
|
||||
create_shell_tool(config.request_rule_enabled),
|
||||
true,
|
||||
);
|
||||
builder.push_spec_with_parallel_support(create_shell_tool(), true);
|
||||
}
|
||||
ConfigShellToolType::Local => {
|
||||
builder.push_spec_with_parallel_support(ToolSpec::LocalShell {}, true);
|
||||
}
|
||||
ConfigShellToolType::UnifiedExec => {
|
||||
builder.push_spec_with_parallel_support(
|
||||
create_exec_command_tool(config.request_rule_enabled),
|
||||
true,
|
||||
);
|
||||
builder.push_spec_with_parallel_support(create_exec_command_tool(), true);
|
||||
builder.push_spec(create_write_stdin_tool());
|
||||
builder.register_handler("exec_command", unified_exec_handler.clone());
|
||||
builder.register_handler("write_stdin", unified_exec_handler);
|
||||
|
|
@ -1463,10 +1453,7 @@ pub(crate) fn build_specs(
|
|||
// Do nothing.
|
||||
}
|
||||
ConfigShellToolType::ShellCommand => {
|
||||
builder.push_spec_with_parallel_support(
|
||||
create_shell_command_tool(config.request_rule_enabled),
|
||||
true,
|
||||
);
|
||||
builder.push_spec_with_parallel_support(create_shell_command_tool(), true);
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -1832,7 +1819,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(true),
|
||||
create_exec_command_tool(),
|
||||
create_write_stdin_tool(),
|
||||
PLAN_TOOL.clone(),
|
||||
create_request_user_input_tool(),
|
||||
|
|
@ -2815,7 +2802,7 @@ mod tests {
|
|||
|
||||
#[test]
|
||||
fn test_shell_tool() {
|
||||
let tool = super::create_shell_tool(true);
|
||||
let tool = super::create_shell_tool();
|
||||
let ToolSpec::Function(ResponsesApiTool {
|
||||
description, name, ..
|
||||
}) = &tool
|
||||
|
|
@ -2845,7 +2832,7 @@ Examples of valid command strings:
|
|||
|
||||
#[test]
|
||||
fn test_shell_command_tool() {
|
||||
let tool = super::create_shell_command_tool(true);
|
||||
let tool = super::create_shell_command_tool();
|
||||
let ToolSpec::Function(ResponsesApiTool {
|
||||
description, name, ..
|
||||
}) = &tool
|
||||
|
|
|
|||
|
|
@ -488,7 +488,6 @@ async fn permissions_message_includes_writable_roots() -> Result<()> {
|
|||
&sandbox_policy,
|
||||
AskForApproval::OnRequest,
|
||||
&Policy::empty(),
|
||||
true,
|
||||
test.config.cwd.as_path(),
|
||||
)
|
||||
.into_text();
|
||||
|
|
|
|||
|
|
@ -225,8 +225,6 @@ const APPROVAL_POLICY_UNLESS_TRUSTED: &str =
|
|||
include_str!("prompts/permissions/approval_policy/unless_trusted.md");
|
||||
const APPROVAL_POLICY_ON_FAILURE: &str =
|
||||
include_str!("prompts/permissions/approval_policy/on_failure.md");
|
||||
const APPROVAL_POLICY_ON_REQUEST: &str =
|
||||
include_str!("prompts/permissions/approval_policy/on_request.md");
|
||||
const APPROVAL_POLICY_ON_REQUEST_RULE: &str =
|
||||
include_str!("prompts/permissions/approval_policy/on_request_rule.md");
|
||||
|
||||
|
|
@ -241,29 +239,20 @@ impl DeveloperInstructions {
|
|||
Self { text: text.into() }
|
||||
}
|
||||
|
||||
pub fn from(
|
||||
approval_policy: AskForApproval,
|
||||
exec_policy: &Policy,
|
||||
request_rule_enabled: bool,
|
||||
) -> DeveloperInstructions {
|
||||
pub fn from(approval_policy: AskForApproval, exec_policy: &Policy) -> DeveloperInstructions {
|
||||
let text = match approval_policy {
|
||||
AskForApproval::Never => APPROVAL_POLICY_NEVER.to_string(),
|
||||
AskForApproval::UnlessTrusted => APPROVAL_POLICY_UNLESS_TRUSTED.to_string(),
|
||||
AskForApproval::OnFailure => APPROVAL_POLICY_ON_FAILURE.to_string(),
|
||||
AskForApproval::OnRequest => {
|
||||
if !request_rule_enabled {
|
||||
APPROVAL_POLICY_ON_REQUEST.to_string()
|
||||
} else {
|
||||
let command_prefixes =
|
||||
format_allow_prefixes(exec_policy.get_allowed_prefixes());
|
||||
match command_prefixes {
|
||||
Some(prefixes) => {
|
||||
format!(
|
||||
"{APPROVAL_POLICY_ON_REQUEST_RULE}\n## Approved command prefixes\nThe following prefix rules have already been approved: {prefixes}"
|
||||
)
|
||||
}
|
||||
None => APPROVAL_POLICY_ON_REQUEST_RULE.to_string(),
|
||||
let command_prefixes = format_allow_prefixes(exec_policy.get_allowed_prefixes());
|
||||
match command_prefixes {
|
||||
Some(prefixes) => {
|
||||
format!(
|
||||
"{APPROVAL_POLICY_ON_REQUEST_RULE}\n## Approved command prefixes\nThe following prefix rules have already been approved: {prefixes}"
|
||||
)
|
||||
}
|
||||
None => APPROVAL_POLICY_ON_REQUEST_RULE.to_string(),
|
||||
}
|
||||
}
|
||||
};
|
||||
|
|
@ -301,7 +290,6 @@ impl DeveloperInstructions {
|
|||
sandbox_policy: &SandboxPolicy,
|
||||
approval_policy: AskForApproval,
|
||||
exec_policy: &Policy,
|
||||
request_rule_enabled: bool,
|
||||
cwd: &Path,
|
||||
) -> Self {
|
||||
let network_access = if sandbox_policy.has_full_network_access() {
|
||||
|
|
@ -325,7 +313,6 @@ impl DeveloperInstructions {
|
|||
network_access,
|
||||
approval_policy,
|
||||
exec_policy,
|
||||
request_rule_enabled,
|
||||
writable_roots,
|
||||
)
|
||||
}
|
||||
|
|
@ -349,7 +336,6 @@ impl DeveloperInstructions {
|
|||
network_access: NetworkAccess,
|
||||
approval_policy: AskForApproval,
|
||||
exec_policy: &Policy,
|
||||
request_rule_enabled: bool,
|
||||
writable_roots: Option<Vec<WritableRoot>>,
|
||||
) -> Self {
|
||||
let start_tag = DeveloperInstructions::new("<permissions instructions>");
|
||||
|
|
@ -359,11 +345,7 @@ impl DeveloperInstructions {
|
|||
sandbox_mode,
|
||||
network_access,
|
||||
))
|
||||
.concat(DeveloperInstructions::from(
|
||||
approval_policy,
|
||||
exec_policy,
|
||||
request_rule_enabled,
|
||||
))
|
||||
.concat(DeveloperInstructions::from(approval_policy, exec_policy))
|
||||
.concat(DeveloperInstructions::from_writable_roots(writable_roots))
|
||||
.concat(end_tag)
|
||||
}
|
||||
|
|
@ -1207,7 +1189,6 @@ mod tests {
|
|||
NetworkAccess::Enabled,
|
||||
AskForApproval::OnRequest,
|
||||
&Policy::empty(),
|
||||
false,
|
||||
None,
|
||||
);
|
||||
|
||||
|
|
@ -1217,7 +1198,7 @@ mod tests {
|
|||
"expected network access to be enabled in message"
|
||||
);
|
||||
assert!(
|
||||
text.contains("`approval_policy` is `on-request`"),
|
||||
text.contains("How to request escalation"),
|
||||
"expected approval guidance to be included"
|
||||
);
|
||||
}
|
||||
|
|
@ -1236,7 +1217,6 @@ mod tests {
|
|||
&policy,
|
||||
AskForApproval::UnlessTrusted,
|
||||
&Policy::empty(),
|
||||
false,
|
||||
&PathBuf::from("/tmp"),
|
||||
);
|
||||
let text = instructions.into_text();
|
||||
|
|
@ -1245,7 +1225,7 @@ mod tests {
|
|||
}
|
||||
|
||||
#[test]
|
||||
fn includes_request_rule_instructions_when_enabled() {
|
||||
fn includes_request_rule_instructions_for_on_request() {
|
||||
let mut exec_policy = Policy::empty();
|
||||
exec_policy
|
||||
.add_prefix_rule(
|
||||
|
|
@ -1258,7 +1238,6 @@ mod tests {
|
|||
NetworkAccess::Enabled,
|
||||
AskForApproval::OnRequest,
|
||||
&exec_policy,
|
||||
true,
|
||||
None,
|
||||
);
|
||||
|
||||
|
|
|
|||
|
|
@ -1,12 +0,0 @@
|
|||
Approvals are your mechanism to get user consent to run shell commands without the sandbox. `approval_policy` is `on-request`: Commands will be run in the sandbox by default, and you can specify in your tool call if you want to escalate a command to run without sandboxing. If the completing the task requires escalated permissions, Do not let these settings or the sandbox deter you from attempting to accomplish the user's task.
|
||||
|
||||
Here are scenarios where you'll need to request approval:
|
||||
- You need to run a command that writes to a directory that requires it (e.g. running tests that write to /var)
|
||||
- You need to run a GUI app (e.g., open/xdg-open/osascript) to open browsers or files.
|
||||
- You are running sandboxed and need to run a command that requires network access (e.g. installing packages)
|
||||
- If you run a command that is important to solving the user's query, but it fails because of sandboxing, rerun the command with approval. ALWAYS proceed to use the `sandbox_permissions` and `justification` parameters - do not message the user before requesting approval for the command.
|
||||
- You are about to take a potentially destructive action such as an `rm` or `git reset` that the user did not explicitly ask for.
|
||||
|
||||
When requesting approval to execute a command that will require escalated privileges:
|
||||
- Provide the `sandbox_permissions` parameter with the value `"require_escalated"`
|
||||
- Include a short, 1 sentence explanation for why you need escalated permissions in the justification parameter
|
||||
Loading…
Add table
Reference in a new issue