mirror of
https://github.com/openai/codex.git
synced 2026-03-08 07:33:21 +00:00
Compare commits
6 Commits
app-server
...
dev/flaky-
| Author | SHA1 | Date | |
|---|---|---|---|
|
|
e0dbee9c61 | ||
|
|
46b8d127cf | ||
|
|
07a30da3fb | ||
|
|
a4a9536fd7 | ||
|
|
590cfa6176 | ||
|
|
bf5c2f48a5 |
@@ -3,6 +3,7 @@ use crate::spawn::SpawnChildRequest;
|
||||
use crate::spawn::StdioPolicy;
|
||||
use crate::spawn::spawn_child_async;
|
||||
use codex_network_proxy::NetworkProxy;
|
||||
use codex_protocol::permissions::FileSystemSandboxPolicy;
|
||||
use codex_protocol::permissions::NetworkSandboxPolicy;
|
||||
use std::collections::HashMap;
|
||||
use std::path::Path;
|
||||
@@ -14,9 +15,9 @@ use tokio::process::Child;
|
||||
/// isolation plus seccomp for network restrictions.
|
||||
///
|
||||
/// Unlike macOS Seatbelt where we directly embed the policy text, the Linux
|
||||
/// helper accepts a list of `--sandbox-permission`/`-s` flags mirroring the
|
||||
/// public CLI. We convert the internal [`SandboxPolicy`] representation into
|
||||
/// the equivalent CLI options.
|
||||
/// helper is a separate executable. We pass the legacy [`SandboxPolicy`] plus
|
||||
/// split filesystem/network policies as JSON so the helper can migrate
|
||||
/// incrementally without breaking older call sites.
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
pub async fn spawn_command_under_linux_sandbox<P>(
|
||||
codex_linux_sandbox_exe: P,
|
||||
@@ -32,9 +33,13 @@ pub async fn spawn_command_under_linux_sandbox<P>(
|
||||
where
|
||||
P: AsRef<Path>,
|
||||
{
|
||||
let args = create_linux_sandbox_command_args(
|
||||
let file_system_sandbox_policy = FileSystemSandboxPolicy::from(sandbox_policy);
|
||||
let network_sandbox_policy = NetworkSandboxPolicy::from(sandbox_policy);
|
||||
let args = create_linux_sandbox_command_args_for_policies(
|
||||
command,
|
||||
sandbox_policy,
|
||||
&file_system_sandbox_policy,
|
||||
network_sandbox_policy,
|
||||
sandbox_policy_cwd,
|
||||
use_bwrap_sandbox,
|
||||
allow_network_for_proxy(false),
|
||||
@@ -45,7 +50,7 @@ where
|
||||
args,
|
||||
arg0,
|
||||
cwd: command_cwd,
|
||||
network_sandbox_policy: NetworkSandboxPolicy::from(sandbox_policy),
|
||||
network_sandbox_policy,
|
||||
network,
|
||||
stdio_policy,
|
||||
env,
|
||||
@@ -60,32 +65,43 @@ pub(crate) fn allow_network_for_proxy(enforce_managed_network: bool) -> bool {
|
||||
enforce_managed_network
|
||||
}
|
||||
|
||||
/// Converts the sandbox policy into the CLI invocation for `codex-linux-sandbox`.
|
||||
/// Converts the sandbox policies into the CLI invocation for
|
||||
/// `codex-linux-sandbox`.
|
||||
///
|
||||
/// The helper performs the actual sandboxing (bubblewrap + seccomp) after
|
||||
/// parsing these arguments. See `docs/linux_sandbox.md` for the Linux semantics.
|
||||
pub(crate) fn create_linux_sandbox_command_args(
|
||||
/// parsing these arguments. Policy JSON flags are emitted before helper feature
|
||||
/// flags so the argv order matches the helper's CLI shape. See
|
||||
/// `docs/linux_sandbox.md` for the Linux semantics.
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
pub(crate) fn create_linux_sandbox_command_args_for_policies(
|
||||
command: Vec<String>,
|
||||
sandbox_policy: &SandboxPolicy,
|
||||
file_system_sandbox_policy: &FileSystemSandboxPolicy,
|
||||
network_sandbox_policy: NetworkSandboxPolicy,
|
||||
sandbox_policy_cwd: &Path,
|
||||
use_bwrap_sandbox: bool,
|
||||
allow_network_for_proxy: bool,
|
||||
) -> Vec<String> {
|
||||
#[expect(clippy::expect_used)]
|
||||
let sandbox_policy_json = serde_json::to_string(sandbox_policy)
|
||||
.unwrap_or_else(|err| panic!("failed to serialize sandbox policy: {err}"));
|
||||
let file_system_policy_json = serde_json::to_string(file_system_sandbox_policy)
|
||||
.unwrap_or_else(|err| panic!("failed to serialize filesystem sandbox policy: {err}"));
|
||||
let network_policy_json = serde_json::to_string(&network_sandbox_policy)
|
||||
.unwrap_or_else(|err| panic!("failed to serialize network sandbox policy: {err}"));
|
||||
let sandbox_policy_cwd = sandbox_policy_cwd
|
||||
.to_str()
|
||||
.expect("cwd must be valid UTF-8")
|
||||
.unwrap_or_else(|| panic!("cwd must be valid UTF-8"))
|
||||
.to_string();
|
||||
|
||||
#[expect(clippy::expect_used)]
|
||||
let sandbox_policy_json =
|
||||
serde_json::to_string(sandbox_policy).expect("Failed to serialize SandboxPolicy to JSON");
|
||||
|
||||
let mut linux_cmd: Vec<String> = vec![
|
||||
"--sandbox-policy-cwd".to_string(),
|
||||
sandbox_policy_cwd,
|
||||
"--sandbox-policy".to_string(),
|
||||
sandbox_policy_json,
|
||||
"--file-system-sandbox-policy".to_string(),
|
||||
file_system_policy_json,
|
||||
"--network-sandbox-policy".to_string(),
|
||||
network_policy_json,
|
||||
];
|
||||
if use_bwrap_sandbox {
|
||||
linux_cmd.push("--use-bwrap-sandbox".to_string());
|
||||
@@ -93,6 +109,32 @@ pub(crate) fn create_linux_sandbox_command_args(
|
||||
if allow_network_for_proxy {
|
||||
linux_cmd.push("--allow-network-for-proxy".to_string());
|
||||
}
|
||||
linux_cmd.push("--".to_string());
|
||||
linux_cmd.extend(command);
|
||||
linux_cmd
|
||||
}
|
||||
|
||||
/// Converts the sandbox cwd and execution options into the CLI invocation for
|
||||
/// `codex-linux-sandbox`.
|
||||
#[cfg(test)]
|
||||
pub(crate) fn create_linux_sandbox_command_args(
|
||||
command: Vec<String>,
|
||||
sandbox_policy_cwd: &Path,
|
||||
use_bwrap_sandbox: bool,
|
||||
allow_network_for_proxy: bool,
|
||||
) -> Vec<String> {
|
||||
let sandbox_policy_cwd = sandbox_policy_cwd
|
||||
.to_str()
|
||||
.unwrap_or_else(|| panic!("cwd must be valid UTF-8"))
|
||||
.to_string();
|
||||
|
||||
let mut linux_cmd: Vec<String> = vec!["--sandbox-policy-cwd".to_string(), sandbox_policy_cwd];
|
||||
if use_bwrap_sandbox {
|
||||
linux_cmd.push("--use-bwrap-sandbox".to_string());
|
||||
}
|
||||
if allow_network_for_proxy {
|
||||
linux_cmd.push("--allow-network-for-proxy".to_string());
|
||||
}
|
||||
|
||||
// Separator so that command arguments starting with `-` are not parsed as
|
||||
// options of the helper itself.
|
||||
@@ -113,16 +155,14 @@ mod tests {
|
||||
fn bwrap_flags_are_feature_gated() {
|
||||
let command = vec!["/bin/true".to_string()];
|
||||
let cwd = Path::new("/tmp");
|
||||
let policy = SandboxPolicy::new_read_only_policy();
|
||||
|
||||
let with_bwrap =
|
||||
create_linux_sandbox_command_args(command.clone(), &policy, cwd, true, false);
|
||||
let with_bwrap = create_linux_sandbox_command_args(command.clone(), cwd, true, false);
|
||||
assert_eq!(
|
||||
with_bwrap.contains(&"--use-bwrap-sandbox".to_string()),
|
||||
true
|
||||
);
|
||||
|
||||
let without_bwrap = create_linux_sandbox_command_args(command, &policy, cwd, false, false);
|
||||
let without_bwrap = create_linux_sandbox_command_args(command, cwd, false, false);
|
||||
assert_eq!(
|
||||
without_bwrap.contains(&"--use-bwrap-sandbox".to_string()),
|
||||
false
|
||||
@@ -133,15 +173,46 @@ mod tests {
|
||||
fn proxy_flag_is_included_when_requested() {
|
||||
let command = vec!["/bin/true".to_string()];
|
||||
let cwd = Path::new("/tmp");
|
||||
let policy = SandboxPolicy::new_read_only_policy();
|
||||
|
||||
let args = create_linux_sandbox_command_args(command, &policy, cwd, true, true);
|
||||
let args = create_linux_sandbox_command_args(command, cwd, true, true);
|
||||
assert_eq!(
|
||||
args.contains(&"--allow-network-for-proxy".to_string()),
|
||||
true
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn split_policy_flags_are_included() {
|
||||
let command = vec!["/bin/true".to_string()];
|
||||
let cwd = Path::new("/tmp");
|
||||
let sandbox_policy = SandboxPolicy::new_read_only_policy();
|
||||
let file_system_sandbox_policy = FileSystemSandboxPolicy::from(&sandbox_policy);
|
||||
let network_sandbox_policy = NetworkSandboxPolicy::from(&sandbox_policy);
|
||||
|
||||
let args = create_linux_sandbox_command_args_for_policies(
|
||||
command,
|
||||
&sandbox_policy,
|
||||
&file_system_sandbox_policy,
|
||||
network_sandbox_policy,
|
||||
cwd,
|
||||
true,
|
||||
false,
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
args.windows(2).any(|window| {
|
||||
window[0] == "--file-system-sandbox-policy" && !window[1].is_empty()
|
||||
}),
|
||||
true
|
||||
);
|
||||
assert_eq!(
|
||||
args.windows(2)
|
||||
.any(|window| window[0] == "--network-sandbox-policy"
|
||||
&& window[1] == "\"restricted\""),
|
||||
true
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn proxy_network_requires_managed_requirements() {
|
||||
assert_eq!(allow_network_for_proxy(false), false);
|
||||
|
||||
@@ -55,6 +55,7 @@ pub use mcp_connection_manager::SandboxState;
|
||||
pub use text_encoding::bytes_to_string_smart;
|
||||
mod mcp_tool_call;
|
||||
mod memories;
|
||||
pub mod mention_syntax;
|
||||
mod mentions;
|
||||
mod message_history;
|
||||
mod model_provider_info;
|
||||
|
||||
@@ -13,6 +13,8 @@ use crate::analytics_client::InvocationType;
|
||||
use crate::analytics_client::build_track_events_context;
|
||||
use crate::codex::Session;
|
||||
use crate::codex::TurnContext;
|
||||
use crate::config::edit::ConfigEdit;
|
||||
use crate::config::edit::ConfigEditsBuilder;
|
||||
use crate::config::types::AppToolApproval;
|
||||
use crate::connectors;
|
||||
use crate::features::Feature;
|
||||
@@ -42,7 +44,9 @@ use codex_rmcp_client::ElicitationAction;
|
||||
use codex_rmcp_client::ElicitationResponse;
|
||||
use rmcp::model::ToolAnnotations;
|
||||
use serde::Serialize;
|
||||
use std::path::Path;
|
||||
use std::sync::Arc;
|
||||
use toml_edit::value;
|
||||
|
||||
/// Handles the specified tool call dispatches the appropriate
|
||||
/// `McpToolCallBegin` and `McpToolCallEnd` events to the `Session`.
|
||||
@@ -127,7 +131,9 @@ pub(crate) async fn handle_mcp_tool_call(
|
||||
.await
|
||||
{
|
||||
let result = match decision {
|
||||
McpToolApprovalDecision::Accept | McpToolApprovalDecision::AcceptAndRemember => {
|
||||
McpToolApprovalDecision::Accept
|
||||
| McpToolApprovalDecision::AcceptForSession
|
||||
| McpToolApprovalDecision::AcceptAndRemember => {
|
||||
let tool_call_begin_event = EventMsg::McpToolCallBegin(McpToolCallBeginEvent {
|
||||
call_id: call_id.clone(),
|
||||
invocation: invocation.clone(),
|
||||
@@ -352,6 +358,7 @@ async fn maybe_track_codex_app_used(
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
enum McpToolApprovalDecision {
|
||||
Accept,
|
||||
AcceptForSession,
|
||||
AcceptAndRemember,
|
||||
Decline,
|
||||
Cancel,
|
||||
@@ -366,15 +373,22 @@ struct McpToolApprovalMetadata {
|
||||
tool_description: Option<String>,
|
||||
}
|
||||
|
||||
#[derive(Clone, Copy)]
|
||||
struct McpToolApprovalPromptOptions {
|
||||
allow_session_remember: bool,
|
||||
allow_persistent_approval: bool,
|
||||
}
|
||||
|
||||
const MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX: &str = "mcp_tool_call_approval";
|
||||
const MCP_TOOL_APPROVAL_ACCEPT: &str = "Approve Once";
|
||||
const MCP_TOOL_APPROVAL_ACCEPT_AND_REMEMBER: &str = "Approve this Session";
|
||||
const MCP_TOOL_APPROVAL_DECLINE: &str = "Deny";
|
||||
const MCP_TOOL_APPROVAL_ACCEPT_FOR_SESSION: &str = "Approve this session";
|
||||
const MCP_TOOL_APPROVAL_ACCEPT_AND_REMEMBER: &str = "Always allow";
|
||||
const MCP_TOOL_APPROVAL_CANCEL: &str = "Cancel";
|
||||
const MCP_TOOL_APPROVAL_KIND_KEY: &str = "codex_approval_kind";
|
||||
const MCP_TOOL_APPROVAL_KIND_MCP_TOOL_CALL: &str = "mcp_tool_call";
|
||||
const MCP_TOOL_APPROVAL_PERSIST_KEY: &str = "persist";
|
||||
const MCP_TOOL_APPROVAL_PERSIST_SESSION: &str = "session";
|
||||
const MCP_TOOL_APPROVAL_PERSIST_ALWAYS: &str = "always";
|
||||
const MCP_TOOL_APPROVAL_SOURCE_KEY: &str = "source";
|
||||
const MCP_TOOL_APPROVAL_SOURCE_CONNECTOR: &str = "connector";
|
||||
const MCP_TOOL_APPROVAL_CONNECTOR_ID_KEY: &str = "connector_id";
|
||||
@@ -384,13 +398,25 @@ const MCP_TOOL_APPROVAL_TOOL_TITLE_KEY: &str = "tool_title";
|
||||
const MCP_TOOL_APPROVAL_TOOL_DESCRIPTION_KEY: &str = "tool_description";
|
||||
const MCP_TOOL_APPROVAL_TOOL_PARAMS_KEY: &str = "tool_params";
|
||||
|
||||
#[derive(Debug, Serialize)]
|
||||
#[derive(Clone, Debug, PartialEq, Eq, Serialize)]
|
||||
struct McpToolApprovalKey {
|
||||
server: String,
|
||||
connector_id: Option<String>,
|
||||
tool_name: String,
|
||||
}
|
||||
|
||||
fn mcp_tool_approval_prompt_options(
|
||||
session_approval_key: Option<&McpToolApprovalKey>,
|
||||
persistent_approval_key: Option<&McpToolApprovalKey>,
|
||||
tool_call_mcp_elicitation_enabled: bool,
|
||||
) -> McpToolApprovalPromptOptions {
|
||||
McpToolApprovalPromptOptions {
|
||||
allow_session_remember: session_approval_key.is_some(),
|
||||
allow_persistent_approval: tool_call_mcp_elicitation_enabled
|
||||
&& persistent_approval_key.is_some(),
|
||||
}
|
||||
}
|
||||
|
||||
async fn maybe_request_mcp_tool_approval(
|
||||
sess: &Arc<Session>,
|
||||
turn_context: &Arc<TurnContext>,
|
||||
@@ -412,25 +438,18 @@ async fn maybe_request_mcp_tool_approval(
|
||||
}
|
||||
}
|
||||
|
||||
let approval_key = if approval_mode == AppToolApproval::Auto {
|
||||
let connector_id = metadata.and_then(|metadata| metadata.connector_id.clone());
|
||||
if invocation.server == CODEX_APPS_MCP_SERVER_NAME && connector_id.is_none() {
|
||||
None
|
||||
} else {
|
||||
Some(McpToolApprovalKey {
|
||||
server: invocation.server.clone(),
|
||||
connector_id,
|
||||
tool_name: invocation.tool.clone(),
|
||||
})
|
||||
}
|
||||
} else {
|
||||
None
|
||||
};
|
||||
if let Some(key) = approval_key.as_ref()
|
||||
let session_approval_key = session_mcp_tool_approval_key(invocation, metadata, approval_mode);
|
||||
let persistent_approval_key =
|
||||
persistent_mcp_tool_approval_key(invocation, metadata, approval_mode);
|
||||
if let Some(key) = session_approval_key.as_ref()
|
||||
&& mcp_tool_approval_is_remembered(sess, key).await
|
||||
{
|
||||
return Some(McpToolApprovalDecision::Accept);
|
||||
}
|
||||
let tool_call_mcp_elicitation_enabled = turn_context
|
||||
.config
|
||||
.features
|
||||
.enabled(Feature::ToolCallMcpElicitation);
|
||||
|
||||
if routes_approval_to_guardian(turn_context) {
|
||||
let decision = review_approval_request(
|
||||
@@ -440,9 +459,23 @@ async fn maybe_request_mcp_tool_approval(
|
||||
None,
|
||||
)
|
||||
.await;
|
||||
return Some(mcp_tool_approval_decision_from_guardian(decision));
|
||||
let decision = mcp_tool_approval_decision_from_guardian(decision);
|
||||
apply_mcp_tool_approval_decision(
|
||||
sess,
|
||||
turn_context,
|
||||
decision,
|
||||
session_approval_key,
|
||||
persistent_approval_key,
|
||||
)
|
||||
.await;
|
||||
return Some(decision);
|
||||
}
|
||||
|
||||
let prompt_options = mcp_tool_approval_prompt_options(
|
||||
session_approval_key.as_ref(),
|
||||
persistent_approval_key.as_ref(),
|
||||
tool_call_mcp_elicitation_enabled,
|
||||
);
|
||||
let question_id = format!("{MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX}_{call_id}");
|
||||
let question = build_mcp_tool_approval_question(
|
||||
question_id.clone(),
|
||||
@@ -451,13 +484,9 @@ async fn maybe_request_mcp_tool_approval(
|
||||
metadata.and_then(|metadata| metadata.tool_title.as_deref()),
|
||||
metadata.and_then(|metadata| metadata.connector_name.as_deref()),
|
||||
annotations,
|
||||
approval_key.is_some(),
|
||||
prompt_options,
|
||||
);
|
||||
if turn_context
|
||||
.config
|
||||
.features
|
||||
.enabled(Feature::ToolCallMcpElicitation)
|
||||
{
|
||||
if tool_call_mcp_elicitation_enabled {
|
||||
let request_id = rmcp::model::RequestId::String(
|
||||
format!("{MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX}_{call_id}").into(),
|
||||
);
|
||||
@@ -468,7 +497,7 @@ async fn maybe_request_mcp_tool_approval(
|
||||
metadata,
|
||||
invocation.arguments.as_ref(),
|
||||
question.clone(),
|
||||
approval_key.is_some(),
|
||||
prompt_options,
|
||||
);
|
||||
let decision = parse_mcp_tool_approval_elicitation_response(
|
||||
sess.request_mcp_server_elicitation(turn_context.as_ref(), request_id, params)
|
||||
@@ -476,11 +505,14 @@ async fn maybe_request_mcp_tool_approval(
|
||||
&question_id,
|
||||
);
|
||||
let decision = normalize_approval_decision_for_mode(decision, approval_mode);
|
||||
if matches!(decision, McpToolApprovalDecision::AcceptAndRemember)
|
||||
&& let Some(key) = approval_key
|
||||
{
|
||||
remember_mcp_tool_approval(sess, key).await;
|
||||
}
|
||||
apply_mcp_tool_approval_decision(
|
||||
sess,
|
||||
turn_context,
|
||||
decision,
|
||||
session_approval_key,
|
||||
persistent_approval_key,
|
||||
)
|
||||
.await;
|
||||
return Some(decision);
|
||||
}
|
||||
|
||||
@@ -494,14 +526,51 @@ async fn maybe_request_mcp_tool_approval(
|
||||
parse_mcp_tool_approval_response(response, &question_id),
|
||||
approval_mode,
|
||||
);
|
||||
if matches!(decision, McpToolApprovalDecision::AcceptAndRemember)
|
||||
&& let Some(key) = approval_key
|
||||
{
|
||||
remember_mcp_tool_approval(sess, key).await;
|
||||
}
|
||||
apply_mcp_tool_approval_decision(
|
||||
sess,
|
||||
turn_context,
|
||||
decision,
|
||||
session_approval_key,
|
||||
persistent_approval_key,
|
||||
)
|
||||
.await;
|
||||
Some(decision)
|
||||
}
|
||||
|
||||
fn session_mcp_tool_approval_key(
|
||||
invocation: &McpInvocation,
|
||||
metadata: Option<&McpToolApprovalMetadata>,
|
||||
approval_mode: AppToolApproval,
|
||||
) -> Option<McpToolApprovalKey> {
|
||||
if approval_mode != AppToolApproval::Auto {
|
||||
return None;
|
||||
}
|
||||
|
||||
let connector_id = metadata.and_then(|metadata| metadata.connector_id.clone());
|
||||
if invocation.server == CODEX_APPS_MCP_SERVER_NAME && connector_id.is_none() {
|
||||
return None;
|
||||
}
|
||||
|
||||
Some(McpToolApprovalKey {
|
||||
server: invocation.server.clone(),
|
||||
connector_id,
|
||||
tool_name: invocation.tool.clone(),
|
||||
})
|
||||
}
|
||||
|
||||
fn persistent_mcp_tool_approval_key(
|
||||
invocation: &McpInvocation,
|
||||
metadata: Option<&McpToolApprovalMetadata>,
|
||||
approval_mode: AppToolApproval,
|
||||
) -> Option<McpToolApprovalKey> {
|
||||
if invocation.server != CODEX_APPS_MCP_SERVER_NAME {
|
||||
return None;
|
||||
}
|
||||
|
||||
session_mcp_tool_approval_key(invocation, metadata, approval_mode)
|
||||
.filter(|key| key.connector_id.is_some())
|
||||
}
|
||||
|
||||
fn build_guardian_mcp_tool_review_request(
|
||||
invocation: &McpInvocation,
|
||||
metadata: Option<&McpToolApprovalMetadata>,
|
||||
@@ -594,8 +663,8 @@ fn mcp_tool_approval_decision_from_guardian(decision: ReviewDecision) -> McpTool
|
||||
match decision {
|
||||
ReviewDecision::Approved
|
||||
| ReviewDecision::ApprovedExecpolicyAmendment { .. }
|
||||
| ReviewDecision::ApprovedForSession
|
||||
| ReviewDecision::NetworkPolicyAmendment { .. } => McpToolApprovalDecision::Accept,
|
||||
ReviewDecision::ApprovedForSession => McpToolApprovalDecision::AcceptForSession,
|
||||
ReviewDecision::Denied | ReviewDecision::Abort => McpToolApprovalDecision::Decline,
|
||||
}
|
||||
}
|
||||
@@ -691,7 +760,7 @@ fn build_mcp_tool_approval_question(
|
||||
tool_title: Option<&str>,
|
||||
connector_name: Option<&str>,
|
||||
annotations: Option<&ToolAnnotations>,
|
||||
allow_remember_option: bool,
|
||||
prompt_options: McpToolApprovalPromptOptions,
|
||||
) -> RequestUserInputQuestion {
|
||||
let destructive =
|
||||
annotations.and_then(|annotations| annotations.destructive_hint) == Some(true);
|
||||
@@ -721,22 +790,22 @@ fn build_mcp_tool_approval_question(
|
||||
label: MCP_TOOL_APPROVAL_ACCEPT.to_string(),
|
||||
description: "Run the tool and continue.".to_string(),
|
||||
}];
|
||||
if allow_remember_option {
|
||||
if prompt_options.allow_session_remember {
|
||||
options.push(RequestUserInputQuestionOption {
|
||||
label: MCP_TOOL_APPROVAL_ACCEPT_AND_REMEMBER.to_string(),
|
||||
label: MCP_TOOL_APPROVAL_ACCEPT_FOR_SESSION.to_string(),
|
||||
description: "Run the tool and remember this choice for this session.".to_string(),
|
||||
});
|
||||
}
|
||||
options.extend([
|
||||
RequestUserInputQuestionOption {
|
||||
label: MCP_TOOL_APPROVAL_DECLINE.to_string(),
|
||||
description: "Decline this tool call and continue.".to_string(),
|
||||
},
|
||||
RequestUserInputQuestionOption {
|
||||
label: MCP_TOOL_APPROVAL_CANCEL.to_string(),
|
||||
description: "Cancel this tool call".to_string(),
|
||||
},
|
||||
]);
|
||||
if prompt_options.allow_persistent_approval {
|
||||
options.push(RequestUserInputQuestionOption {
|
||||
label: MCP_TOOL_APPROVAL_ACCEPT_AND_REMEMBER.to_string(),
|
||||
description: "Run the tool and remember this choice for future tool calls.".to_string(),
|
||||
});
|
||||
}
|
||||
options.push(RequestUserInputQuestionOption {
|
||||
label: MCP_TOOL_APPROVAL_CANCEL.to_string(),
|
||||
description: "Cancel this tool call".to_string(),
|
||||
});
|
||||
|
||||
RequestUserInputQuestion {
|
||||
id: question_id,
|
||||
@@ -755,7 +824,7 @@ fn build_mcp_tool_approval_elicitation_request(
|
||||
metadata: Option<&McpToolApprovalMetadata>,
|
||||
tool_params: Option<&serde_json::Value>,
|
||||
question: RequestUserInputQuestion,
|
||||
allow_session_persist: bool,
|
||||
prompt_options: McpToolApprovalPromptOptions,
|
||||
) -> McpServerElicitationRequestParams {
|
||||
let message = if question.header.trim().is_empty() {
|
||||
question.question
|
||||
@@ -774,7 +843,7 @@ fn build_mcp_tool_approval_elicitation_request(
|
||||
server,
|
||||
metadata,
|
||||
tool_params,
|
||||
allow_session_persist,
|
||||
prompt_options,
|
||||
),
|
||||
message,
|
||||
requested_schema: McpElicitationSchema {
|
||||
@@ -791,18 +860,39 @@ fn build_mcp_tool_approval_elicitation_meta(
|
||||
server: &str,
|
||||
metadata: Option<&McpToolApprovalMetadata>,
|
||||
tool_params: Option<&serde_json::Value>,
|
||||
allow_session_persist: bool,
|
||||
prompt_options: McpToolApprovalPromptOptions,
|
||||
) -> Option<serde_json::Value> {
|
||||
let mut meta = serde_json::Map::new();
|
||||
meta.insert(
|
||||
MCP_TOOL_APPROVAL_KIND_KEY.to_string(),
|
||||
serde_json::Value::String(MCP_TOOL_APPROVAL_KIND_MCP_TOOL_CALL.to_string()),
|
||||
);
|
||||
if allow_session_persist {
|
||||
meta.insert(
|
||||
MCP_TOOL_APPROVAL_PERSIST_KEY.to_string(),
|
||||
serde_json::Value::String(MCP_TOOL_APPROVAL_PERSIST_SESSION.to_string()),
|
||||
);
|
||||
match (
|
||||
prompt_options.allow_session_remember,
|
||||
prompt_options.allow_persistent_approval,
|
||||
) {
|
||||
(true, true) => {
|
||||
meta.insert(
|
||||
MCP_TOOL_APPROVAL_PERSIST_KEY.to_string(),
|
||||
serde_json::json!([
|
||||
MCP_TOOL_APPROVAL_PERSIST_SESSION,
|
||||
MCP_TOOL_APPROVAL_PERSIST_ALWAYS,
|
||||
]),
|
||||
);
|
||||
}
|
||||
(true, false) => {
|
||||
meta.insert(
|
||||
MCP_TOOL_APPROVAL_PERSIST_KEY.to_string(),
|
||||
serde_json::Value::String(MCP_TOOL_APPROVAL_PERSIST_SESSION.to_string()),
|
||||
);
|
||||
}
|
||||
(false, true) => {
|
||||
meta.insert(
|
||||
MCP_TOOL_APPROVAL_PERSIST_KEY.to_string(),
|
||||
serde_json::Value::String(MCP_TOOL_APPROVAL_PERSIST_ALWAYS.to_string()),
|
||||
);
|
||||
}
|
||||
(false, false) => {}
|
||||
}
|
||||
if let Some(metadata) = metadata {
|
||||
if let Some(tool_title) = metadata.tool_title.as_ref() {
|
||||
@@ -864,15 +954,20 @@ fn parse_mcp_tool_approval_elicitation_response(
|
||||
};
|
||||
match response.action {
|
||||
ElicitationAction::Accept => {
|
||||
if response
|
||||
match response
|
||||
.meta
|
||||
.as_ref()
|
||||
.and_then(serde_json::Value::as_object)
|
||||
.and_then(|meta| meta.get(MCP_TOOL_APPROVAL_PERSIST_KEY))
|
||||
.and_then(serde_json::Value::as_str)
|
||||
== Some(MCP_TOOL_APPROVAL_PERSIST_SESSION)
|
||||
{
|
||||
return McpToolApprovalDecision::AcceptAndRemember;
|
||||
Some(MCP_TOOL_APPROVAL_PERSIST_SESSION) => {
|
||||
return McpToolApprovalDecision::AcceptForSession;
|
||||
}
|
||||
Some(MCP_TOOL_APPROVAL_PERSIST_ALWAYS) => {
|
||||
return McpToolApprovalDecision::AcceptAndRemember;
|
||||
}
|
||||
_ => {}
|
||||
}
|
||||
|
||||
match parse_mcp_tool_approval_response(
|
||||
@@ -930,6 +1025,11 @@ fn parse_mcp_tool_approval_response(
|
||||
return McpToolApprovalDecision::Cancel;
|
||||
};
|
||||
if answers
|
||||
.iter()
|
||||
.any(|answer| answer == MCP_TOOL_APPROVAL_ACCEPT_FOR_SESSION)
|
||||
{
|
||||
McpToolApprovalDecision::AcceptForSession
|
||||
} else if answers
|
||||
.iter()
|
||||
.any(|answer| answer == MCP_TOOL_APPROVAL_ACCEPT_AND_REMEMBER)
|
||||
{
|
||||
@@ -939,13 +1039,8 @@ fn parse_mcp_tool_approval_response(
|
||||
.any(|answer| answer == MCP_TOOL_APPROVAL_ACCEPT)
|
||||
{
|
||||
McpToolApprovalDecision::Accept
|
||||
} else if answers
|
||||
.iter()
|
||||
.any(|answer| answer == MCP_TOOL_APPROVAL_CANCEL)
|
||||
{
|
||||
McpToolApprovalDecision::Cancel
|
||||
} else {
|
||||
McpToolApprovalDecision::Decline
|
||||
McpToolApprovalDecision::Cancel
|
||||
}
|
||||
}
|
||||
|
||||
@@ -954,7 +1049,10 @@ fn normalize_approval_decision_for_mode(
|
||||
approval_mode: AppToolApproval,
|
||||
) -> McpToolApprovalDecision {
|
||||
if approval_mode == AppToolApproval::Prompt
|
||||
&& decision == McpToolApprovalDecision::AcceptAndRemember
|
||||
&& matches!(
|
||||
decision,
|
||||
McpToolApprovalDecision::AcceptForSession | McpToolApprovalDecision::AcceptAndRemember
|
||||
)
|
||||
{
|
||||
McpToolApprovalDecision::Accept
|
||||
} else {
|
||||
@@ -972,6 +1070,81 @@ async fn remember_mcp_tool_approval(sess: &Session, key: McpToolApprovalKey) {
|
||||
store.put(key, ReviewDecision::ApprovedForSession);
|
||||
}
|
||||
|
||||
async fn apply_mcp_tool_approval_decision(
|
||||
sess: &Session,
|
||||
turn_context: &TurnContext,
|
||||
decision: McpToolApprovalDecision,
|
||||
session_approval_key: Option<McpToolApprovalKey>,
|
||||
persistent_approval_key: Option<McpToolApprovalKey>,
|
||||
) {
|
||||
match decision {
|
||||
McpToolApprovalDecision::AcceptForSession => {
|
||||
if let Some(key) = session_approval_key {
|
||||
remember_mcp_tool_approval(sess, key).await;
|
||||
}
|
||||
}
|
||||
McpToolApprovalDecision::AcceptAndRemember => {
|
||||
if let Some(key) = persistent_approval_key {
|
||||
maybe_persist_mcp_tool_approval(sess, turn_context, key).await;
|
||||
} else if let Some(key) = session_approval_key {
|
||||
remember_mcp_tool_approval(sess, key).await;
|
||||
}
|
||||
}
|
||||
McpToolApprovalDecision::Accept
|
||||
| McpToolApprovalDecision::Decline
|
||||
| McpToolApprovalDecision::Cancel => {}
|
||||
}
|
||||
}
|
||||
|
||||
async fn maybe_persist_mcp_tool_approval(
|
||||
sess: &Session,
|
||||
turn_context: &TurnContext,
|
||||
key: McpToolApprovalKey,
|
||||
) {
|
||||
let Some(connector_id) = key.connector_id.clone() else {
|
||||
remember_mcp_tool_approval(sess, key).await;
|
||||
return;
|
||||
};
|
||||
let tool_name = key.tool_name.clone();
|
||||
|
||||
if let Err(err) =
|
||||
persist_codex_app_tool_approval(&turn_context.config.codex_home, &connector_id, &tool_name)
|
||||
.await
|
||||
{
|
||||
error!(
|
||||
error = %err,
|
||||
connector_id,
|
||||
tool_name,
|
||||
"failed to persist codex app tool approval"
|
||||
);
|
||||
remember_mcp_tool_approval(sess, key).await;
|
||||
return;
|
||||
}
|
||||
|
||||
sess.reload_user_config_layer().await;
|
||||
remember_mcp_tool_approval(sess, key).await;
|
||||
}
|
||||
|
||||
async fn persist_codex_app_tool_approval(
|
||||
codex_home: &Path,
|
||||
connector_id: &str,
|
||||
tool_name: &str,
|
||||
) -> anyhow::Result<()> {
|
||||
ConfigEditsBuilder::new(codex_home)
|
||||
.with_edits([ConfigEdit::SetPath {
|
||||
segments: vec![
|
||||
"apps".to_string(),
|
||||
connector_id.to_string(),
|
||||
"tools".to_string(),
|
||||
tool_name.to_string(),
|
||||
"approval_mode".to_string(),
|
||||
],
|
||||
value: value("approve"),
|
||||
}])
|
||||
.apply()
|
||||
.await
|
||||
}
|
||||
|
||||
fn requires_mcp_tool_approval(annotations: &ToolAnnotations) -> bool {
|
||||
if annotations.destructive_hint == Some(true) {
|
||||
return true;
|
||||
@@ -1006,7 +1179,17 @@ async fn notify_mcp_tool_call_skip(
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::codex::make_session_and_context;
|
||||
use crate::config::ConfigToml;
|
||||
use crate::config::types::AppConfig;
|
||||
use crate::config::types::AppToolConfig;
|
||||
use crate::config::types::AppToolsConfig;
|
||||
use crate::config::types::AppsConfigToml;
|
||||
use codex_config::CONFIG_TOML_FILE;
|
||||
use pretty_assertions::assert_eq;
|
||||
use serde::Deserialize;
|
||||
use std::collections::HashMap;
|
||||
use tempfile::tempdir;
|
||||
|
||||
fn annotations(
|
||||
read_only: Option<bool>,
|
||||
@@ -1039,6 +1222,16 @@ mod tests {
|
||||
}
|
||||
}
|
||||
|
||||
fn prompt_options(
|
||||
allow_session_remember: bool,
|
||||
allow_persistent_approval: bool,
|
||||
) -> McpToolApprovalPromptOptions {
|
||||
McpToolApprovalPromptOptions {
|
||||
allow_session_remember,
|
||||
allow_persistent_approval,
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn approval_required_when_read_only_false_and_destructive() {
|
||||
let annotations = annotations(Some(false), Some(true), None);
|
||||
@@ -1058,7 +1251,14 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn prompt_mode_does_not_allow_session_remember() {
|
||||
fn prompt_mode_does_not_allow_persistent_remember() {
|
||||
assert_eq!(
|
||||
normalize_approval_decision_for_mode(
|
||||
McpToolApprovalDecision::AcceptForSession,
|
||||
AppToolApproval::Prompt,
|
||||
),
|
||||
McpToolApprovalDecision::Accept
|
||||
);
|
||||
assert_eq!(
|
||||
normalize_approval_decision_for_mode(
|
||||
McpToolApprovalDecision::AcceptAndRemember,
|
||||
@@ -1077,7 +1277,7 @@ mod tests {
|
||||
Some("Run Action"),
|
||||
None,
|
||||
Some(&annotations(Some(false), Some(true), None)),
|
||||
true,
|
||||
prompt_options(false, false),
|
||||
);
|
||||
|
||||
assert_eq!(question.header, "Approve app tool call?");
|
||||
@@ -1086,7 +1286,7 @@ mod tests {
|
||||
"The custom_server MCP server wants to run the tool \"Run Action\", which may modify or delete data. Allow this action?"
|
||||
);
|
||||
assert!(
|
||||
question
|
||||
!question
|
||||
.options
|
||||
.expect("options")
|
||||
.into_iter()
|
||||
@@ -1104,7 +1304,7 @@ mod tests {
|
||||
Some("Run Action"),
|
||||
None,
|
||||
Some(&annotations(Some(false), Some(true), None)),
|
||||
true,
|
||||
prompt_options(true, true),
|
||||
);
|
||||
|
||||
assert!(
|
||||
@@ -1114,6 +1314,149 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn trusted_codex_apps_tool_question_offers_always_allow() {
|
||||
let question = build_mcp_tool_approval_question(
|
||||
"q".to_string(),
|
||||
CODEX_APPS_MCP_SERVER_NAME,
|
||||
"run_action",
|
||||
Some("Run Action"),
|
||||
Some("Calendar"),
|
||||
Some(&annotations(Some(false), Some(true), None)),
|
||||
prompt_options(true, true),
|
||||
);
|
||||
let options = question.options.expect("options");
|
||||
|
||||
assert!(options.iter().any(|option| {
|
||||
option.label == MCP_TOOL_APPROVAL_ACCEPT_FOR_SESSION
|
||||
&& option.description == "Run the tool and remember this choice for this session."
|
||||
}));
|
||||
assert!(options.iter().any(|option| {
|
||||
option.label == MCP_TOOL_APPROVAL_ACCEPT_AND_REMEMBER
|
||||
&& option.description
|
||||
== "Run the tool and remember this choice for future tool calls."
|
||||
}));
|
||||
assert_eq!(
|
||||
options
|
||||
.into_iter()
|
||||
.map(|option| option.label)
|
||||
.collect::<Vec<_>>(),
|
||||
vec![
|
||||
MCP_TOOL_APPROVAL_ACCEPT.to_string(),
|
||||
MCP_TOOL_APPROVAL_ACCEPT_FOR_SESSION.to_string(),
|
||||
MCP_TOOL_APPROVAL_ACCEPT_AND_REMEMBER.to_string(),
|
||||
MCP_TOOL_APPROVAL_CANCEL.to_string(),
|
||||
]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn codex_apps_tool_question_without_elicitation_omits_always_allow() {
|
||||
let session_key = McpToolApprovalKey {
|
||||
server: CODEX_APPS_MCP_SERVER_NAME.to_string(),
|
||||
connector_id: Some("calendar".to_string()),
|
||||
tool_name: "run_action".to_string(),
|
||||
};
|
||||
let persistent_key = session_key.clone();
|
||||
let question = build_mcp_tool_approval_question(
|
||||
"q".to_string(),
|
||||
CODEX_APPS_MCP_SERVER_NAME,
|
||||
"run_action",
|
||||
Some("Run Action"),
|
||||
Some("Calendar"),
|
||||
Some(&annotations(Some(false), Some(true), None)),
|
||||
mcp_tool_approval_prompt_options(Some(&session_key), Some(&persistent_key), false),
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
question
|
||||
.options
|
||||
.expect("options")
|
||||
.into_iter()
|
||||
.map(|option| option.label)
|
||||
.collect::<Vec<_>>(),
|
||||
vec![
|
||||
MCP_TOOL_APPROVAL_ACCEPT.to_string(),
|
||||
MCP_TOOL_APPROVAL_ACCEPT_FOR_SESSION.to_string(),
|
||||
MCP_TOOL_APPROVAL_CANCEL.to_string(),
|
||||
]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn custom_mcp_tool_question_offers_session_remember_without_always_allow() {
|
||||
let question = build_mcp_tool_approval_question(
|
||||
"q".to_string(),
|
||||
"custom_server",
|
||||
"run_action",
|
||||
Some("Run Action"),
|
||||
None,
|
||||
Some(&annotations(Some(false), Some(true), None)),
|
||||
prompt_options(true, false),
|
||||
);
|
||||
|
||||
assert_eq!(
|
||||
question
|
||||
.options
|
||||
.expect("options")
|
||||
.into_iter()
|
||||
.map(|option| option.label)
|
||||
.collect::<Vec<_>>(),
|
||||
vec![
|
||||
MCP_TOOL_APPROVAL_ACCEPT.to_string(),
|
||||
MCP_TOOL_APPROVAL_ACCEPT_FOR_SESSION.to_string(),
|
||||
MCP_TOOL_APPROVAL_CANCEL.to_string(),
|
||||
]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn custom_servers_keep_session_remember_without_persistent_approval() {
|
||||
let invocation = McpInvocation {
|
||||
server: "custom_server".to_string(),
|
||||
tool: "run_action".to_string(),
|
||||
arguments: None,
|
||||
};
|
||||
let expected = McpToolApprovalKey {
|
||||
server: "custom_server".to_string(),
|
||||
connector_id: None,
|
||||
tool_name: "run_action".to_string(),
|
||||
};
|
||||
|
||||
assert_eq!(
|
||||
session_mcp_tool_approval_key(&invocation, None, AppToolApproval::Auto),
|
||||
Some(expected)
|
||||
);
|
||||
assert_eq!(
|
||||
persistent_mcp_tool_approval_key(&invocation, None, AppToolApproval::Auto),
|
||||
None
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn codex_apps_connectors_support_persistent_approval() {
|
||||
let invocation = McpInvocation {
|
||||
server: CODEX_APPS_MCP_SERVER_NAME.to_string(),
|
||||
tool: "calendar/list_events".to_string(),
|
||||
arguments: None,
|
||||
};
|
||||
let metadata = approval_metadata(Some("calendar"), Some("Calendar"), None, None, None);
|
||||
let expected = McpToolApprovalKey {
|
||||
server: CODEX_APPS_MCP_SERVER_NAME.to_string(),
|
||||
connector_id: Some("calendar".to_string()),
|
||||
tool_name: "calendar/list_events".to_string(),
|
||||
};
|
||||
|
||||
assert_eq!(
|
||||
session_mcp_tool_approval_key(&invocation, Some(&metadata), AppToolApproval::Auto),
|
||||
Some(expected.clone())
|
||||
);
|
||||
assert_eq!(
|
||||
persistent_mcp_tool_approval_key(&invocation, Some(&metadata), AppToolApproval::Auto),
|
||||
Some(expected)
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn sanitize_mcp_tool_result_for_model_rewrites_image_content() {
|
||||
let result = Ok(CallToolResult {
|
||||
@@ -1194,7 +1537,12 @@ mod tests {
|
||||
#[test]
|
||||
fn approval_elicitation_meta_marks_tool_approvals() {
|
||||
assert_eq!(
|
||||
build_mcp_tool_approval_elicitation_meta("custom_server", None, None, false),
|
||||
build_mcp_tool_approval_elicitation_meta(
|
||||
"custom_server",
|
||||
None,
|
||||
None,
|
||||
prompt_options(false, false),
|
||||
),
|
||||
Some(serde_json::json!({
|
||||
MCP_TOOL_APPROVAL_KIND_KEY: MCP_TOOL_APPROVAL_KIND_MCP_TOOL_CALL,
|
||||
}))
|
||||
@@ -1202,7 +1550,7 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn approval_elicitation_meta_keeps_session_persist_behavior() {
|
||||
fn approval_elicitation_meta_keeps_session_persist_behavior_for_custom_servers() {
|
||||
assert_eq!(
|
||||
build_mcp_tool_approval_elicitation_meta(
|
||||
"custom_server",
|
||||
@@ -1214,7 +1562,7 @@ mod tests {
|
||||
Some("Runs the selected action."),
|
||||
)),
|
||||
Some(&serde_json::json!({"id": 1})),
|
||||
true,
|
||||
prompt_options(true, false),
|
||||
),
|
||||
Some(serde_json::json!({
|
||||
MCP_TOOL_APPROVAL_KIND_KEY: MCP_TOOL_APPROVAL_KIND_MCP_TOOL_CALL,
|
||||
@@ -1335,7 +1683,7 @@ mod tests {
|
||||
Some(&serde_json::json!({
|
||||
"calendar_id": "primary",
|
||||
})),
|
||||
false,
|
||||
prompt_options(false, false),
|
||||
),
|
||||
Some(serde_json::json!({
|
||||
MCP_TOOL_APPROVAL_KIND_KEY: MCP_TOOL_APPROVAL_KIND_MCP_TOOL_CALL,
|
||||
@@ -1353,7 +1701,7 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn approval_elicitation_meta_merges_session_persist_with_connector_source() {
|
||||
fn approval_elicitation_meta_merges_session_and_always_persist_with_connector_source() {
|
||||
assert_eq!(
|
||||
build_mcp_tool_approval_elicitation_meta(
|
||||
CODEX_APPS_MCP_SERVER_NAME,
|
||||
@@ -1367,11 +1715,14 @@ mod tests {
|
||||
Some(&serde_json::json!({
|
||||
"calendar_id": "primary",
|
||||
})),
|
||||
true,
|
||||
prompt_options(true, true),
|
||||
),
|
||||
Some(serde_json::json!({
|
||||
MCP_TOOL_APPROVAL_KIND_KEY: MCP_TOOL_APPROVAL_KIND_MCP_TOOL_CALL,
|
||||
MCP_TOOL_APPROVAL_PERSIST_KEY: MCP_TOOL_APPROVAL_PERSIST_SESSION,
|
||||
MCP_TOOL_APPROVAL_PERSIST_KEY: [
|
||||
MCP_TOOL_APPROVAL_PERSIST_SESSION,
|
||||
MCP_TOOL_APPROVAL_PERSIST_ALWAYS,
|
||||
],
|
||||
MCP_TOOL_APPROVAL_SOURCE_KEY: MCP_TOOL_APPROVAL_SOURCE_CONNECTOR,
|
||||
MCP_TOOL_APPROVAL_CONNECTOR_ID_KEY: "calendar",
|
||||
MCP_TOOL_APPROVAL_CONNECTOR_NAME_KEY: "Calendar",
|
||||
@@ -1401,6 +1752,22 @@ mod tests {
|
||||
assert_eq!(response, McpToolApprovalDecision::Decline);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn accepted_elicitation_response_uses_always_persist_meta() {
|
||||
let response = parse_mcp_tool_approval_elicitation_response(
|
||||
Some(ElicitationResponse {
|
||||
action: ElicitationAction::Accept,
|
||||
content: None,
|
||||
meta: Some(serde_json::json!({
|
||||
MCP_TOOL_APPROVAL_PERSIST_KEY: MCP_TOOL_APPROVAL_PERSIST_ALWAYS,
|
||||
})),
|
||||
}),
|
||||
"approval",
|
||||
);
|
||||
|
||||
assert_eq!(response, McpToolApprovalDecision::AcceptAndRemember);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn accepted_elicitation_response_uses_session_persist_meta() {
|
||||
let response = parse_mcp_tool_approval_elicitation_response(
|
||||
@@ -1414,7 +1781,7 @@ mod tests {
|
||||
"approval",
|
||||
);
|
||||
|
||||
assert_eq!(response, McpToolApprovalDecision::AcceptAndRemember);
|
||||
assert_eq!(response, McpToolApprovalDecision::AcceptForSession);
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -1430,4 +1797,83 @@ mod tests {
|
||||
|
||||
assert_eq!(response, McpToolApprovalDecision::Accept);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn persist_codex_app_tool_approval_writes_tool_override() {
|
||||
let tmp = tempdir().expect("tempdir");
|
||||
|
||||
persist_codex_app_tool_approval(tmp.path(), "calendar", "calendar/list_events")
|
||||
.await
|
||||
.expect("persist approval");
|
||||
|
||||
let contents =
|
||||
std::fs::read_to_string(tmp.path().join(CONFIG_TOML_FILE)).expect("read config");
|
||||
let parsed: ConfigToml = toml::from_str(&contents).expect("parse config");
|
||||
|
||||
assert_eq!(
|
||||
parsed.apps,
|
||||
Some(AppsConfigToml {
|
||||
default: None,
|
||||
apps: HashMap::from([(
|
||||
"calendar".to_string(),
|
||||
AppConfig {
|
||||
enabled: true,
|
||||
destructive_enabled: None,
|
||||
open_world_enabled: None,
|
||||
default_tools_approval_mode: None,
|
||||
default_tools_enabled: None,
|
||||
tools: Some(AppToolsConfig {
|
||||
tools: HashMap::from([(
|
||||
"calendar/list_events".to_string(),
|
||||
AppToolConfig {
|
||||
enabled: None,
|
||||
approval_mode: Some(AppToolApproval::Approve),
|
||||
},
|
||||
)]),
|
||||
}),
|
||||
},
|
||||
)]),
|
||||
})
|
||||
);
|
||||
assert!(contents.contains("[apps.calendar.tools.\"calendar/list_events\"]"));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn maybe_persist_mcp_tool_approval_reloads_session_config() {
|
||||
let (session, turn_context) = make_session_and_context().await;
|
||||
let codex_home = session.codex_home().await;
|
||||
std::fs::create_dir_all(&codex_home).expect("create codex home");
|
||||
let key = McpToolApprovalKey {
|
||||
server: CODEX_APPS_MCP_SERVER_NAME.to_string(),
|
||||
connector_id: Some("calendar".to_string()),
|
||||
tool_name: "calendar/list_events".to_string(),
|
||||
};
|
||||
|
||||
maybe_persist_mcp_tool_approval(&session, &turn_context, key.clone()).await;
|
||||
|
||||
let config = session.get_config().await;
|
||||
let apps_toml = config
|
||||
.config_layer_stack
|
||||
.effective_config()
|
||||
.as_table()
|
||||
.and_then(|table| table.get("apps"))
|
||||
.cloned()
|
||||
.expect("apps table");
|
||||
let apps = AppsConfigToml::deserialize(apps_toml).expect("deserialize apps config");
|
||||
let tool = apps
|
||||
.apps
|
||||
.get("calendar")
|
||||
.and_then(|app| app.tools.as_ref())
|
||||
.and_then(|tools| tools.tools.get("calendar/list_events"))
|
||||
.expect("calendar/list_events tool config exists");
|
||||
|
||||
assert_eq!(
|
||||
tool,
|
||||
&AppToolConfig {
|
||||
enabled: None,
|
||||
approval_mode: Some(AppToolApproval::Approve),
|
||||
}
|
||||
);
|
||||
assert_eq!(mcp_tool_approval_is_remembered(&session, &key).await, true);
|
||||
}
|
||||
}
|
||||
|
||||
4
codex-rs/core/src/mention_syntax.rs
Normal file
4
codex-rs/core/src/mention_syntax.rs
Normal file
@@ -0,0 +1,4 @@
|
||||
// Default plaintext sigil for tools.
|
||||
pub const TOOL_MENTION_SIGIL: char = '$';
|
||||
// Plugins use `@` in linked plaintext outside TUI.
|
||||
pub const PLUGIN_TEXT_MENTION_SIGIL: char = '@';
|
||||
@@ -5,11 +5,13 @@ use std::path::PathBuf;
|
||||
use codex_protocol::user_input::UserInput;
|
||||
|
||||
use crate::connectors;
|
||||
use crate::mention_syntax::PLUGIN_TEXT_MENTION_SIGIL;
|
||||
use crate::mention_syntax::TOOL_MENTION_SIGIL;
|
||||
use crate::plugins::PluginCapabilitySummary;
|
||||
use crate::skills::SkillMetadata;
|
||||
use crate::skills::injection::ToolMentionKind;
|
||||
use crate::skills::injection::app_id_from_path;
|
||||
use crate::skills::injection::extract_tool_mentions;
|
||||
use crate::skills::injection::extract_tool_mentions_with_sigil;
|
||||
use crate::skills::injection::plugin_config_name_from_path;
|
||||
use crate::skills::injection::tool_kind_for_path;
|
||||
|
||||
@@ -19,10 +21,17 @@ pub(crate) struct CollectedToolMentions {
|
||||
}
|
||||
|
||||
pub(crate) fn collect_tool_mentions_from_messages(messages: &[String]) -> CollectedToolMentions {
|
||||
collect_tool_mentions_from_messages_with_sigil(messages, TOOL_MENTION_SIGIL)
|
||||
}
|
||||
|
||||
fn collect_tool_mentions_from_messages_with_sigil(
|
||||
messages: &[String],
|
||||
sigil: char,
|
||||
) -> CollectedToolMentions {
|
||||
let mut plain_names = HashSet::new();
|
||||
let mut paths = HashSet::new();
|
||||
for message in messages {
|
||||
let mentions = extract_tool_mentions(message);
|
||||
let mentions = extract_tool_mentions_with_sigil(message, sigil);
|
||||
plain_names.extend(mentions.plain_names().map(str::to_string));
|
||||
paths.extend(mentions.paths().map(str::to_string));
|
||||
}
|
||||
@@ -50,7 +59,7 @@ pub(crate) fn collect_explicit_app_ids(input: &[UserInput]) -> HashSet<String> {
|
||||
.collect()
|
||||
}
|
||||
|
||||
/// Collect explicit structured `plugin://...` mentions.
|
||||
/// Collect explicit structured or linked `plugin://...` mentions.
|
||||
pub(crate) fn collect_explicit_plugin_mentions(
|
||||
input: &[UserInput],
|
||||
plugins: &[PluginCapabilitySummary],
|
||||
@@ -73,7 +82,11 @@ pub(crate) fn collect_explicit_plugin_mentions(
|
||||
UserInput::Mention { path, .. } => Some(path.clone()),
|
||||
_ => None,
|
||||
})
|
||||
.chain(collect_tool_mentions_from_messages(&messages).paths)
|
||||
.chain(
|
||||
// Plugin plaintext links use `@`, not the default `$` tool sigil.
|
||||
collect_tool_mentions_from_messages_with_sigil(&messages, PLUGIN_TEXT_MENTION_SIGIL)
|
||||
.paths,
|
||||
)
|
||||
.filter(|path| tool_kind_for_path(path.as_str()) == ToolMentionKind::Plugin)
|
||||
.filter_map(|path| plugin_config_name_from_path(path.as_str()).map(str::to_string))
|
||||
.collect();
|
||||
@@ -222,7 +235,7 @@ mod tests {
|
||||
];
|
||||
|
||||
let mentioned = collect_explicit_plugin_mentions(
|
||||
&[text_input("use [$sample](plugin://sample@test)")],
|
||||
&[text_input("use [@sample](plugin://sample@test)")],
|
||||
&plugins,
|
||||
);
|
||||
|
||||
@@ -238,7 +251,7 @@ mod tests {
|
||||
|
||||
let mentioned = collect_explicit_plugin_mentions(
|
||||
&[
|
||||
text_input("use [$sample](plugin://sample@test)"),
|
||||
text_input("use [@sample](plugin://sample@test)"),
|
||||
UserInput::Mention {
|
||||
name: "sample".to_string(),
|
||||
path: "plugin://sample@test".to_string(),
|
||||
@@ -263,4 +276,16 @@ mod tests {
|
||||
|
||||
assert_eq!(mentioned, Vec::<PluginCapabilitySummary>::new());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn collect_explicit_plugin_mentions_ignores_dollar_linked_plugin_mentions() {
|
||||
let plugins = vec![plugin("sample@test", "sample")];
|
||||
|
||||
let mentioned = collect_explicit_plugin_mentions(
|
||||
&[text_input("use [$sample](plugin://sample@test)")],
|
||||
&plugins,
|
||||
);
|
||||
|
||||
assert_eq!(mentioned, Vec::<PluginCapabilitySummary>::new());
|
||||
}
|
||||
}
|
||||
|
||||
@@ -14,12 +14,12 @@ use crate::exec::SandboxType;
|
||||
use crate::exec::StdoutStream;
|
||||
use crate::exec::execute_exec_request;
|
||||
use crate::landlock::allow_network_for_proxy;
|
||||
use crate::landlock::create_linux_sandbox_command_args;
|
||||
use crate::landlock::create_linux_sandbox_command_args_for_policies;
|
||||
use crate::protocol::SandboxPolicy;
|
||||
#[cfg(target_os = "macos")]
|
||||
use crate::seatbelt::MACOS_PATH_TO_SEATBELT_EXECUTABLE;
|
||||
#[cfg(target_os = "macos")]
|
||||
use crate::seatbelt::create_seatbelt_command_args_with_extensions;
|
||||
use crate::seatbelt::create_seatbelt_command_args_for_policies_with_extensions;
|
||||
#[cfg(target_os = "macos")]
|
||||
use crate::spawn::CODEX_SANDBOX_ENV_VAR;
|
||||
use crate::spawn::CODEX_SANDBOX_NETWORK_DISABLED_ENV_VAR;
|
||||
@@ -35,7 +35,6 @@ use codex_protocol::permissions::FileSystemPath;
|
||||
use codex_protocol::permissions::FileSystemSandboxEntry;
|
||||
use codex_protocol::permissions::FileSystemSandboxKind;
|
||||
use codex_protocol::permissions::FileSystemSandboxPolicy;
|
||||
use codex_protocol::permissions::FileSystemSpecialPath;
|
||||
use codex_protocol::permissions::NetworkSandboxPolicy;
|
||||
use codex_protocol::protocol::NetworkAccess;
|
||||
use codex_protocol::protocol::ReadOnlyAccess;
|
||||
@@ -215,7 +214,6 @@ fn additional_permission_roots(
|
||||
)
|
||||
}
|
||||
|
||||
#[cfg_attr(not(test), allow(dead_code))]
|
||||
fn merge_file_system_policy_with_additional_permissions(
|
||||
file_system_policy: &FileSystemSandboxPolicy,
|
||||
extra_reads: Vec<AbsolutePathBuf>,
|
||||
@@ -369,14 +367,7 @@ pub(crate) fn should_require_platform_sandbox(
|
||||
}
|
||||
|
||||
match file_system_policy.kind {
|
||||
FileSystemSandboxKind::Restricted => !file_system_policy.entries.iter().any(|entry| {
|
||||
entry.access == FileSystemAccessMode::Write
|
||||
&& matches!(
|
||||
&entry.path,
|
||||
FileSystemPath::Special { value }
|
||||
if matches!(value, FileSystemSpecialPath::Root)
|
||||
)
|
||||
}),
|
||||
FileSystemSandboxKind::Restricted => !file_system_policy.has_full_disk_write_access(),
|
||||
FileSystemSandboxKind::Unrestricted | FileSystemSandboxKind::ExternalSandbox => false,
|
||||
}
|
||||
}
|
||||
@@ -496,9 +487,10 @@ impl SandboxManager {
|
||||
SandboxType::MacosSeatbelt => {
|
||||
let mut seatbelt_env = HashMap::new();
|
||||
seatbelt_env.insert(CODEX_SANDBOX_ENV_VAR.to_string(), "seatbelt".to_string());
|
||||
let mut args = create_seatbelt_command_args_with_extensions(
|
||||
let mut args = create_seatbelt_command_args_for_policies_with_extensions(
|
||||
command.clone(),
|
||||
&effective_policy,
|
||||
&effective_file_system_policy,
|
||||
effective_network_policy,
|
||||
sandbox_policy_cwd,
|
||||
enforce_managed_network,
|
||||
network,
|
||||
@@ -515,9 +507,11 @@ impl SandboxManager {
|
||||
let exe = codex_linux_sandbox_exe
|
||||
.ok_or(SandboxTransformError::MissingLinuxSandboxExecutable)?;
|
||||
let allow_proxy_network = allow_network_for_proxy(enforce_managed_network);
|
||||
let mut args = create_linux_sandbox_command_args(
|
||||
let mut args = create_linux_sandbox_command_args_for_policies(
|
||||
command.clone(),
|
||||
&effective_policy,
|
||||
&effective_file_system_policy,
|
||||
effective_network_policy,
|
||||
sandbox_policy_cwd,
|
||||
use_linux_sandbox_bwrap,
|
||||
allow_proxy_network,
|
||||
|
||||
@@ -22,6 +22,7 @@ use crate::spawn::CODEX_SANDBOX_ENV_VAR;
|
||||
use crate::spawn::SpawnChildRequest;
|
||||
use crate::spawn::StdioPolicy;
|
||||
use crate::spawn::spawn_child_async;
|
||||
use codex_protocol::permissions::FileSystemSandboxPolicy;
|
||||
use codex_protocol::permissions::NetworkSandboxPolicy;
|
||||
|
||||
const MACOS_SEATBELT_BASE_POLICY: &str = include_str!("seatbelt_base_policy.sbpl");
|
||||
@@ -260,10 +261,23 @@ fn unix_socket_policy(proxy: &ProxyPolicyInputs) -> String {
|
||||
policy
|
||||
}
|
||||
|
||||
#[cfg_attr(not(test), allow(dead_code))]
|
||||
fn dynamic_network_policy(
|
||||
sandbox_policy: &SandboxPolicy,
|
||||
enforce_managed_network: bool,
|
||||
proxy: &ProxyPolicyInputs,
|
||||
) -> String {
|
||||
dynamic_network_policy_for_network(
|
||||
NetworkSandboxPolicy::from(sandbox_policy),
|
||||
enforce_managed_network,
|
||||
proxy,
|
||||
)
|
||||
}
|
||||
|
||||
fn dynamic_network_policy_for_network(
|
||||
network_policy: NetworkSandboxPolicy,
|
||||
enforce_managed_network: bool,
|
||||
proxy: &ProxyPolicyInputs,
|
||||
) -> String {
|
||||
let should_use_restricted_network_policy =
|
||||
!proxy.ports.is_empty() || proxy.has_proxy_config || enforce_managed_network;
|
||||
@@ -288,7 +302,19 @@ fn dynamic_network_policy(
|
||||
return format!("{policy}{MACOS_SEATBELT_NETWORK_POLICY}");
|
||||
}
|
||||
|
||||
if sandbox_policy.has_full_network_access() {
|
||||
if proxy.has_proxy_config {
|
||||
// Proxy configuration is present but we could not infer any valid loopback endpoints.
|
||||
// Fail closed to avoid silently widening network access in proxy-enforced sessions.
|
||||
return String::new();
|
||||
}
|
||||
|
||||
if enforce_managed_network {
|
||||
// Managed network requirements are active but no usable proxy endpoints
|
||||
// are available. Fail closed for network access.
|
||||
return String::new();
|
||||
}
|
||||
|
||||
if network_policy.is_enabled() {
|
||||
// No proxy env is configured: retain the existing full-network behavior.
|
||||
format!(
|
||||
"(allow network-outbound)\n(allow network-inbound)\n{MACOS_SEATBELT_NETWORK_POLICY}"
|
||||
@@ -305,9 +331,10 @@ pub(crate) fn create_seatbelt_command_args(
|
||||
enforce_managed_network: bool,
|
||||
network: Option<&NetworkProxy>,
|
||||
) -> Vec<String> {
|
||||
create_seatbelt_command_args_with_extensions(
|
||||
create_seatbelt_command_args_for_policies_with_extensions(
|
||||
command,
|
||||
sandbox_policy,
|
||||
&FileSystemSandboxPolicy::from(sandbox_policy),
|
||||
NetworkSandboxPolicy::from(sandbox_policy),
|
||||
sandbox_policy_cwd,
|
||||
enforce_managed_network,
|
||||
network,
|
||||
@@ -315,6 +342,64 @@ pub(crate) fn create_seatbelt_command_args(
|
||||
)
|
||||
}
|
||||
|
||||
fn root_absolute_path() -> AbsolutePathBuf {
|
||||
match AbsolutePathBuf::from_absolute_path(Path::new("/")) {
|
||||
Ok(path) => path,
|
||||
Err(err) => panic!("root path must be absolute: {err}"),
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone)]
|
||||
struct SeatbeltAccessRoot {
|
||||
root: AbsolutePathBuf,
|
||||
excluded_subpaths: Vec<AbsolutePathBuf>,
|
||||
}
|
||||
|
||||
fn build_seatbelt_access_policy(
|
||||
action: &str,
|
||||
param_prefix: &str,
|
||||
roots: Vec<SeatbeltAccessRoot>,
|
||||
) -> (String, Vec<(String, PathBuf)>) {
|
||||
let mut policy_components = Vec::new();
|
||||
let mut params = Vec::new();
|
||||
|
||||
for (index, access_root) in roots.into_iter().enumerate() {
|
||||
let root =
|
||||
normalize_path_for_sandbox(access_root.root.as_path()).unwrap_or(access_root.root);
|
||||
let root_param = format!("{param_prefix}_{index}");
|
||||
params.push((root_param.clone(), root.into_path_buf()));
|
||||
|
||||
if access_root.excluded_subpaths.is_empty() {
|
||||
policy_components.push(format!("(subpath (param \"{root_param}\"))"));
|
||||
continue;
|
||||
}
|
||||
|
||||
let mut require_parts = vec![format!("(subpath (param \"{root_param}\"))")];
|
||||
for (excluded_index, excluded_subpath) in
|
||||
access_root.excluded_subpaths.into_iter().enumerate()
|
||||
{
|
||||
let excluded_subpath =
|
||||
normalize_path_for_sandbox(excluded_subpath.as_path()).unwrap_or(excluded_subpath);
|
||||
let excluded_param = format!("{param_prefix}_{index}_RO_{excluded_index}");
|
||||
params.push((excluded_param.clone(), excluded_subpath.into_path_buf()));
|
||||
require_parts.push(format!(
|
||||
"(require-not (subpath (param \"{excluded_param}\")))"
|
||||
));
|
||||
}
|
||||
policy_components.push(format!("(require-all {} )", require_parts.join(" ")));
|
||||
}
|
||||
|
||||
if policy_components.is_empty() {
|
||||
(String::new(), Vec::new())
|
||||
} else {
|
||||
(
|
||||
format!("(allow {action}\n{}\n)", policy_components.join(" ")),
|
||||
params,
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg_attr(not(test), allow(dead_code))]
|
||||
pub(crate) fn create_seatbelt_command_args_with_extensions(
|
||||
command: Vec<String>,
|
||||
sandbox_policy: &SandboxPolicy,
|
||||
@@ -323,101 +408,112 @@ pub(crate) fn create_seatbelt_command_args_with_extensions(
|
||||
network: Option<&NetworkProxy>,
|
||||
extensions: Option<&MacOsSeatbeltProfileExtensions>,
|
||||
) -> Vec<String> {
|
||||
let (file_write_policy, file_write_dir_params) = {
|
||||
if sandbox_policy.has_full_disk_write_access() {
|
||||
// Allegedly, this is more permissive than `(allow file-write*)`.
|
||||
(
|
||||
r#"(allow file-write* (regex #"^/"))"#.to_string(),
|
||||
Vec::new(),
|
||||
)
|
||||
} else {
|
||||
let writable_roots = sandbox_policy.get_writable_roots_with_cwd(sandbox_policy_cwd);
|
||||
create_seatbelt_command_args_for_policies_with_extensions(
|
||||
command,
|
||||
&FileSystemSandboxPolicy::from(sandbox_policy),
|
||||
NetworkSandboxPolicy::from(sandbox_policy),
|
||||
sandbox_policy_cwd,
|
||||
enforce_managed_network,
|
||||
network,
|
||||
extensions,
|
||||
)
|
||||
}
|
||||
|
||||
let mut writable_folder_policies: Vec<String> = Vec::new();
|
||||
let mut file_write_params = Vec::new();
|
||||
|
||||
for (index, wr) in writable_roots.iter().enumerate() {
|
||||
// Canonicalize to avoid mismatches like /var vs /private/var on macOS.
|
||||
let canonical_root = wr
|
||||
.root
|
||||
.as_path()
|
||||
.canonicalize()
|
||||
.unwrap_or_else(|_| wr.root.to_path_buf());
|
||||
let root_param = format!("WRITABLE_ROOT_{index}");
|
||||
file_write_params.push((root_param.clone(), canonical_root));
|
||||
|
||||
if wr.read_only_subpaths.is_empty() {
|
||||
writable_folder_policies.push(format!("(subpath (param \"{root_param}\"))"));
|
||||
} else {
|
||||
// Add parameters for each read-only subpath and generate
|
||||
// the `(require-not ...)` clauses.
|
||||
let mut require_parts: Vec<String> = Vec::new();
|
||||
require_parts.push(format!("(subpath (param \"{root_param}\"))"));
|
||||
for (subpath_index, ro) in wr.read_only_subpaths.iter().enumerate() {
|
||||
let canonical_ro = ro
|
||||
.as_path()
|
||||
.canonicalize()
|
||||
.unwrap_or_else(|_| ro.to_path_buf());
|
||||
let ro_param = format!("WRITABLE_ROOT_{index}_RO_{subpath_index}");
|
||||
require_parts
|
||||
.push(format!("(require-not (subpath (param \"{ro_param}\")))"));
|
||||
file_write_params.push((ro_param, canonical_ro));
|
||||
}
|
||||
let policy_component = format!("(require-all {} )", require_parts.join(" "));
|
||||
writable_folder_policies.push(policy_component);
|
||||
}
|
||||
}
|
||||
|
||||
if writable_folder_policies.is_empty() {
|
||||
("".to_string(), Vec::new())
|
||||
pub(crate) fn create_seatbelt_command_args_for_policies_with_extensions(
|
||||
command: Vec<String>,
|
||||
file_system_sandbox_policy: &FileSystemSandboxPolicy,
|
||||
network_sandbox_policy: NetworkSandboxPolicy,
|
||||
sandbox_policy_cwd: &Path,
|
||||
enforce_managed_network: bool,
|
||||
network: Option<&NetworkProxy>,
|
||||
extensions: Option<&MacOsSeatbeltProfileExtensions>,
|
||||
) -> Vec<String> {
|
||||
let unreadable_roots =
|
||||
file_system_sandbox_policy.get_unreadable_roots_with_cwd(sandbox_policy_cwd);
|
||||
let (file_write_policy, file_write_dir_params) =
|
||||
if file_system_sandbox_policy.has_full_disk_write_access() {
|
||||
if unreadable_roots.is_empty() {
|
||||
// Allegedly, this is more permissive than `(allow file-write*)`.
|
||||
(
|
||||
r#"(allow file-write* (regex #"^/"))"#.to_string(),
|
||||
Vec::new(),
|
||||
)
|
||||
} else {
|
||||
let file_write_policy = format!(
|
||||
"(allow file-write*\n{}\n)",
|
||||
writable_folder_policies.join(" ")
|
||||
);
|
||||
(file_write_policy, file_write_params)
|
||||
build_seatbelt_access_policy(
|
||||
"file-write*",
|
||||
"WRITABLE_ROOT",
|
||||
vec![SeatbeltAccessRoot {
|
||||
root: root_absolute_path(),
|
||||
excluded_subpaths: unreadable_roots.clone(),
|
||||
}],
|
||||
)
|
||||
}
|
||||
}
|
||||
};
|
||||
|
||||
let (file_read_policy, file_read_dir_params) = if sandbox_policy.has_full_disk_read_access() {
|
||||
(
|
||||
"; allow read-only file operations\n(allow file-read*)".to_string(),
|
||||
Vec::new(),
|
||||
)
|
||||
} else {
|
||||
let mut readable_roots_policies: Vec<String> = Vec::new();
|
||||
let mut file_read_params = Vec::new();
|
||||
for (index, root) in sandbox_policy
|
||||
.get_readable_roots_with_cwd(sandbox_policy_cwd)
|
||||
.into_iter()
|
||||
.enumerate()
|
||||
{
|
||||
// Canonicalize to avoid mismatches like /var vs /private/var on macOS.
|
||||
let canonical_root = root
|
||||
.as_path()
|
||||
.canonicalize()
|
||||
.unwrap_or_else(|_| root.to_path_buf());
|
||||
let root_param = format!("READABLE_ROOT_{index}");
|
||||
file_read_params.push((root_param.clone(), canonical_root));
|
||||
readable_roots_policies.push(format!("(subpath (param \"{root_param}\"))"));
|
||||
}
|
||||
|
||||
if readable_roots_policies.is_empty() {
|
||||
("".to_string(), Vec::new())
|
||||
} else {
|
||||
(
|
||||
format!(
|
||||
"; allow read-only file operations\n(allow file-read*\n{}\n)",
|
||||
readable_roots_policies.join(" ")
|
||||
),
|
||||
file_read_params,
|
||||
build_seatbelt_access_policy(
|
||||
"file-write*",
|
||||
"WRITABLE_ROOT",
|
||||
file_system_sandbox_policy
|
||||
.get_writable_roots_with_cwd(sandbox_policy_cwd)
|
||||
.into_iter()
|
||||
.map(|root| SeatbeltAccessRoot {
|
||||
root: root.root,
|
||||
excluded_subpaths: root.read_only_subpaths,
|
||||
})
|
||||
.collect(),
|
||||
)
|
||||
}
|
||||
};
|
||||
};
|
||||
|
||||
let (file_read_policy, file_read_dir_params) =
|
||||
if file_system_sandbox_policy.has_full_disk_read_access() {
|
||||
if unreadable_roots.is_empty() {
|
||||
(
|
||||
"; allow read-only file operations\n(allow file-read*)".to_string(),
|
||||
Vec::new(),
|
||||
)
|
||||
} else {
|
||||
let (policy, params) = build_seatbelt_access_policy(
|
||||
"file-read*",
|
||||
"READABLE_ROOT",
|
||||
vec![SeatbeltAccessRoot {
|
||||
root: root_absolute_path(),
|
||||
excluded_subpaths: unreadable_roots,
|
||||
}],
|
||||
);
|
||||
(
|
||||
format!("; allow read-only file operations\n{policy}"),
|
||||
params,
|
||||
)
|
||||
}
|
||||
} else {
|
||||
let (policy, params) = build_seatbelt_access_policy(
|
||||
"file-read*",
|
||||
"READABLE_ROOT",
|
||||
file_system_sandbox_policy
|
||||
.get_readable_roots_with_cwd(sandbox_policy_cwd)
|
||||
.into_iter()
|
||||
.map(|root| SeatbeltAccessRoot {
|
||||
excluded_subpaths: unreadable_roots
|
||||
.iter()
|
||||
.filter(|path| path.as_path().starts_with(root.as_path()))
|
||||
.cloned()
|
||||
.collect(),
|
||||
root,
|
||||
})
|
||||
.collect(),
|
||||
);
|
||||
if policy.is_empty() {
|
||||
(String::new(), params)
|
||||
} else {
|
||||
(
|
||||
format!("; allow read-only file operations\n{policy}"),
|
||||
params,
|
||||
)
|
||||
}
|
||||
};
|
||||
|
||||
let proxy = proxy_policy_inputs(network);
|
||||
let network_policy = dynamic_network_policy(sandbox_policy, enforce_managed_network, &proxy);
|
||||
let network_policy =
|
||||
dynamic_network_policy_for_network(network_sandbox_policy, enforce_managed_network, &proxy);
|
||||
let seatbelt_extensions = extensions.map_or_else(
|
||||
|| {
|
||||
// Backward-compatibility default when no extension profile is provided.
|
||||
@@ -426,7 +522,7 @@ pub(crate) fn create_seatbelt_command_args_with_extensions(
|
||||
build_seatbelt_extensions,
|
||||
);
|
||||
|
||||
let include_platform_defaults = sandbox_policy.include_platform_defaults();
|
||||
let include_platform_defaults = file_system_sandbox_policy.include_platform_defaults();
|
||||
let mut policy_sections = vec![
|
||||
MACOS_SEATBELT_BASE_POLICY.to_string(),
|
||||
file_read_policy,
|
||||
@@ -493,6 +589,7 @@ mod tests {
|
||||
use super::ProxyPolicyInputs;
|
||||
use super::UnixDomainSocketPolicy;
|
||||
use super::create_seatbelt_command_args;
|
||||
use super::create_seatbelt_command_args_for_policies_with_extensions;
|
||||
use super::create_seatbelt_command_args_with_extensions;
|
||||
use super::dynamic_network_policy;
|
||||
use super::macos_dir_params;
|
||||
@@ -504,6 +601,11 @@ mod tests {
|
||||
use crate::seatbelt_permissions::MacOsAutomationPermission;
|
||||
use crate::seatbelt_permissions::MacOsPreferencesPermission;
|
||||
use crate::seatbelt_permissions::MacOsSeatbeltProfileExtensions;
|
||||
use codex_protocol::permissions::FileSystemAccessMode;
|
||||
use codex_protocol::permissions::FileSystemPath;
|
||||
use codex_protocol::permissions::FileSystemSandboxEntry;
|
||||
use codex_protocol::permissions::FileSystemSandboxPolicy;
|
||||
use codex_protocol::permissions::NetworkSandboxPolicy;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::fs;
|
||||
@@ -526,6 +628,15 @@ mod tests {
|
||||
AbsolutePathBuf::from_absolute_path(Path::new(path)).expect("absolute path")
|
||||
}
|
||||
|
||||
fn seatbelt_policy_arg(args: &[String]) -> &str {
|
||||
let policy_index = args
|
||||
.iter()
|
||||
.position(|arg| arg == "-p")
|
||||
.expect("seatbelt args should include -p");
|
||||
args.get(policy_index + 1)
|
||||
.expect("seatbelt args should include policy text")
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn base_policy_allows_node_cpu_sysctls() {
|
||||
assert!(
|
||||
@@ -573,6 +684,95 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn explicit_unreadable_paths_are_excluded_from_full_disk_read_and_write_access() {
|
||||
let unreadable = absolute_path("/tmp/codex-unreadable");
|
||||
let file_system_policy = FileSystemSandboxPolicy::restricted(vec![
|
||||
FileSystemSandboxEntry {
|
||||
path: FileSystemPath::Special {
|
||||
value: crate::protocol::FileSystemSpecialPath::Root,
|
||||
},
|
||||
access: FileSystemAccessMode::Write,
|
||||
},
|
||||
FileSystemSandboxEntry {
|
||||
path: FileSystemPath::Path { path: unreadable },
|
||||
access: FileSystemAccessMode::None,
|
||||
},
|
||||
]);
|
||||
|
||||
let args = create_seatbelt_command_args_for_policies_with_extensions(
|
||||
vec!["/bin/true".to_string()],
|
||||
&file_system_policy,
|
||||
NetworkSandboxPolicy::Restricted,
|
||||
Path::new("/"),
|
||||
false,
|
||||
None,
|
||||
None,
|
||||
);
|
||||
|
||||
let policy = seatbelt_policy_arg(&args);
|
||||
assert!(
|
||||
policy.contains("(require-not (subpath (param \"READABLE_ROOT_0_RO_0\")))"),
|
||||
"expected read carveout in policy:\n{policy}"
|
||||
);
|
||||
assert!(
|
||||
policy.contains("(require-not (subpath (param \"WRITABLE_ROOT_0_RO_0\")))"),
|
||||
"expected write carveout in policy:\n{policy}"
|
||||
);
|
||||
assert!(
|
||||
args.iter()
|
||||
.any(|arg| arg == "-DREADABLE_ROOT_0_RO_0=/tmp/codex-unreadable"),
|
||||
"expected read carveout parameter in args: {args:#?}"
|
||||
);
|
||||
assert!(
|
||||
args.iter()
|
||||
.any(|arg| arg == "-DWRITABLE_ROOT_0_RO_0=/tmp/codex-unreadable"),
|
||||
"expected write carveout parameter in args: {args:#?}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn explicit_unreadable_paths_are_excluded_from_readable_roots() {
|
||||
let root = absolute_path("/tmp/codex-readable");
|
||||
let unreadable = absolute_path("/tmp/codex-readable/private");
|
||||
let file_system_policy = FileSystemSandboxPolicy::restricted(vec![
|
||||
FileSystemSandboxEntry {
|
||||
path: FileSystemPath::Path { path: root },
|
||||
access: FileSystemAccessMode::Read,
|
||||
},
|
||||
FileSystemSandboxEntry {
|
||||
path: FileSystemPath::Path { path: unreadable },
|
||||
access: FileSystemAccessMode::None,
|
||||
},
|
||||
]);
|
||||
|
||||
let args = create_seatbelt_command_args_for_policies_with_extensions(
|
||||
vec!["/bin/true".to_string()],
|
||||
&file_system_policy,
|
||||
NetworkSandboxPolicy::Restricted,
|
||||
Path::new("/"),
|
||||
false,
|
||||
None,
|
||||
None,
|
||||
);
|
||||
|
||||
let policy = seatbelt_policy_arg(&args);
|
||||
assert!(
|
||||
policy.contains("(require-not (subpath (param \"READABLE_ROOT_0_RO_0\")))"),
|
||||
"expected read carveout in policy:\n{policy}"
|
||||
);
|
||||
assert!(
|
||||
args.iter()
|
||||
.any(|arg| arg == "-DREADABLE_ROOT_0=/tmp/codex-readable"),
|
||||
"expected readable root parameter in args: {args:#?}"
|
||||
);
|
||||
assert!(
|
||||
args.iter()
|
||||
.any(|arg| arg == "-DREADABLE_ROOT_0_RO_0=/tmp/codex-readable/private"),
|
||||
"expected read carveout parameter in args: {args:#?}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn seatbelt_args_include_macos_permission_extensions() {
|
||||
let cwd = std::env::temp_dir();
|
||||
@@ -991,7 +1191,7 @@ sys.exit(0 if allowed else 13)
|
||||
; allow read-only file operations
|
||||
(allow file-read*)
|
||||
(allow file-write*
|
||||
(require-all (subpath (param "WRITABLE_ROOT_0")) (require-not (subpath (param "WRITABLE_ROOT_0_RO_0"))) (require-not (subpath (param "WRITABLE_ROOT_0_RO_1"))) ) (subpath (param "WRITABLE_ROOT_1")) (subpath (param "WRITABLE_ROOT_2"))
|
||||
(subpath (param "WRITABLE_ROOT_0")) (require-all (subpath (param "WRITABLE_ROOT_1")) (require-not (subpath (param "WRITABLE_ROOT_1_RO_0"))) (require-not (subpath (param "WRITABLE_ROOT_1_RO_1"))) ) (subpath (param "WRITABLE_ROOT_2"))
|
||||
)
|
||||
|
||||
; macOS permission profile extensions
|
||||
@@ -1004,43 +1204,51 @@ sys.exit(0 if allowed else 13)
|
||||
"#,
|
||||
);
|
||||
|
||||
let mut expected_args = vec![
|
||||
"-p".to_string(),
|
||||
expected_policy,
|
||||
assert_eq!(seatbelt_policy_arg(&args), expected_policy);
|
||||
|
||||
let expected_definitions = [
|
||||
format!(
|
||||
"-DWRITABLE_ROOT_0={}",
|
||||
vulnerable_root_canonical.to_string_lossy()
|
||||
),
|
||||
format!(
|
||||
"-DWRITABLE_ROOT_0_RO_0={}",
|
||||
dot_git_canonical.to_string_lossy()
|
||||
),
|
||||
format!(
|
||||
"-DWRITABLE_ROOT_0_RO_1={}",
|
||||
dot_codex_canonical.to_string_lossy()
|
||||
),
|
||||
format!(
|
||||
"-DWRITABLE_ROOT_1={}",
|
||||
empty_root_canonical.to_string_lossy()
|
||||
),
|
||||
format!(
|
||||
"-DWRITABLE_ROOT_2={}",
|
||||
cwd.canonicalize()
|
||||
.expect("canonicalize cwd")
|
||||
.to_string_lossy()
|
||||
),
|
||||
format!(
|
||||
"-DWRITABLE_ROOT_1={}",
|
||||
vulnerable_root_canonical.to_string_lossy()
|
||||
),
|
||||
format!(
|
||||
"-DWRITABLE_ROOT_1_RO_0={}",
|
||||
dot_git_canonical.to_string_lossy()
|
||||
),
|
||||
format!(
|
||||
"-DWRITABLE_ROOT_1_RO_1={}",
|
||||
dot_codex_canonical.to_string_lossy()
|
||||
),
|
||||
format!(
|
||||
"-DWRITABLE_ROOT_2={}",
|
||||
empty_root_canonical.to_string_lossy()
|
||||
),
|
||||
];
|
||||
for expected_definition in expected_definitions {
|
||||
assert!(
|
||||
args.contains(&expected_definition),
|
||||
"expected definition arg `{expected_definition}` in {args:#?}"
|
||||
);
|
||||
}
|
||||
for (key, value) in macos_dir_params() {
|
||||
let expected_definition = format!("-D{key}={}", value.to_string_lossy());
|
||||
assert!(
|
||||
args.contains(&expected_definition),
|
||||
"expected definition arg `{expected_definition}` in {args:#?}"
|
||||
);
|
||||
}
|
||||
|
||||
expected_args.extend(
|
||||
macos_dir_params()
|
||||
.into_iter()
|
||||
.map(|(key, value)| format!("-D{key}={value}", value = value.to_string_lossy())),
|
||||
);
|
||||
|
||||
expected_args.push("--".to_string());
|
||||
expected_args.extend(shell_command);
|
||||
|
||||
assert_eq!(expected_args, args);
|
||||
let command_index = args
|
||||
.iter()
|
||||
.position(|arg| arg == "--")
|
||||
.expect("seatbelt args should include command separator");
|
||||
assert_eq!(args[command_index + 1..], shell_command);
|
||||
|
||||
// Verify that .codex/config.toml cannot be modified under the generated
|
||||
// Seatbelt policy.
|
||||
|
||||
@@ -7,6 +7,7 @@ use crate::analytics_client::InvocationType;
|
||||
use crate::analytics_client::SkillInvocation;
|
||||
use crate::analytics_client::TrackEventsContext;
|
||||
use crate::instructions::SkillInstructions;
|
||||
use crate::mention_syntax::TOOL_MENTION_SIGIL;
|
||||
use crate::mentions::build_skill_name_counts;
|
||||
use crate::skills::SkillMetadata;
|
||||
use codex_otel::SessionTelemetry;
|
||||
@@ -232,10 +233,10 @@ pub(crate) fn normalize_skill_path(path: &str) -> &str {
|
||||
/// resource path is present, it is captured for exact path matching while also tracking
|
||||
/// the name for fallback matching.
|
||||
pub(crate) fn extract_tool_mentions(text: &str) -> ToolMentions<'_> {
|
||||
extract_tool_mentions_with_sigil(text, '$')
|
||||
extract_tool_mentions_with_sigil(text, TOOL_MENTION_SIGIL)
|
||||
}
|
||||
|
||||
fn extract_tool_mentions_with_sigil(text: &str, sigil: char) -> ToolMentions<'_> {
|
||||
pub(crate) fn extract_tool_mentions_with_sigil(text: &str, sigil: char) -> ToolMentions<'_> {
|
||||
let text_bytes = text.as_bytes();
|
||||
let mut mentioned_names: HashSet<&str> = HashSet::new();
|
||||
let mut mentioned_paths: HashSet<&str> = HashSet::new();
|
||||
|
||||
@@ -1750,6 +1750,16 @@ mod tests {
|
||||
use std::path::Path;
|
||||
use tempfile::tempdir;
|
||||
|
||||
fn set_danger_full_access(turn: &mut crate::codex::TurnContext) {
|
||||
turn.sandbox_policy
|
||||
.set(SandboxPolicy::DangerFullAccess)
|
||||
.expect("test setup should allow updating sandbox policy");
|
||||
turn.file_system_sandbox_policy =
|
||||
crate::protocol::FileSystemSandboxPolicy::from(turn.sandbox_policy.get());
|
||||
turn.network_sandbox_policy =
|
||||
crate::protocol::NetworkSandboxPolicy::from(turn.sandbox_policy.get());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn node_version_parses_v_prefix_and_suffix() {
|
||||
let version = NodeVersion::parse("v25.1.0-nightly.2024").unwrap();
|
||||
@@ -2467,9 +2477,7 @@ mod tests {
|
||||
turn.approval_policy
|
||||
.set(AskForApproval::Never)
|
||||
.expect("test setup should allow updating approval policy");
|
||||
turn.sandbox_policy
|
||||
.set(SandboxPolicy::DangerFullAccess)
|
||||
.expect("test setup should allow updating sandbox policy");
|
||||
set_danger_full_access(&mut turn);
|
||||
|
||||
let session = Arc::new(session);
|
||||
let turn = Arc::new(turn);
|
||||
@@ -2521,9 +2529,7 @@ console.log("cell-complete");
|
||||
turn.approval_policy
|
||||
.set(AskForApproval::Never)
|
||||
.expect("test setup should allow updating approval policy");
|
||||
turn.sandbox_policy
|
||||
.set(SandboxPolicy::DangerFullAccess)
|
||||
.expect("test setup should allow updating sandbox policy");
|
||||
set_danger_full_access(&mut turn);
|
||||
|
||||
let session = Arc::new(session);
|
||||
let turn = Arc::new(turn);
|
||||
@@ -2579,9 +2585,7 @@ console.log(out.type);
|
||||
turn.approval_policy
|
||||
.set(AskForApproval::Never)
|
||||
.expect("test setup should allow updating approval policy");
|
||||
turn.sandbox_policy
|
||||
.set(SandboxPolicy::DangerFullAccess)
|
||||
.expect("test setup should allow updating sandbox policy");
|
||||
set_danger_full_access(&mut turn);
|
||||
|
||||
let session = Arc::new(session);
|
||||
let turn = Arc::new(turn);
|
||||
|
||||
@@ -208,14 +208,17 @@ async fn with_additional_permissions_requires_approval_under_on_request() -> Res
|
||||
});
|
||||
let test = builder.build(&server).await?;
|
||||
|
||||
let requested_write = test.workspace_path("requested-but-unused.txt");
|
||||
let requested_dir = test.workspace_path("requested-dir");
|
||||
fs::create_dir_all(&requested_dir)?;
|
||||
let requested_dir_canonical = requested_dir.canonicalize()?;
|
||||
let requested_write = requested_dir.join("requested-but-unused.txt");
|
||||
let _ = fs::remove_file(&requested_write);
|
||||
let call_id = "request_permissions_skip_approval";
|
||||
let command = "touch requested-but-unused.txt";
|
||||
let command = "touch requested-dir/requested-but-unused.txt";
|
||||
let requested_permissions = PermissionProfile {
|
||||
file_system: Some(FileSystemPermissions {
|
||||
read: Some(vec![]),
|
||||
write: Some(vec![absolute_path(&requested_write)]),
|
||||
write: Some(vec![absolute_path(&requested_dir_canonical)]),
|
||||
}),
|
||||
..Default::default()
|
||||
};
|
||||
@@ -292,6 +295,7 @@ async fn relative_additional_permissions_resolve_against_tool_workdir() -> Resul
|
||||
|
||||
let nested_dir = test.workspace_path("nested");
|
||||
fs::create_dir_all(&nested_dir)?;
|
||||
let nested_dir_canonical = nested_dir.canonicalize()?;
|
||||
let requested_write = nested_dir.join("relative-write.txt");
|
||||
let _ = fs::remove_file(&requested_write);
|
||||
|
||||
@@ -300,7 +304,7 @@ async fn relative_additional_permissions_resolve_against_tool_workdir() -> Resul
|
||||
let expected_permissions = PermissionProfile {
|
||||
file_system: Some(FileSystemPermissions {
|
||||
read: None,
|
||||
write: Some(vec![absolute_path(&requested_write)]),
|
||||
write: Some(vec![absolute_path(&nested_dir_canonical)]),
|
||||
}),
|
||||
..Default::default()
|
||||
};
|
||||
@@ -310,7 +314,7 @@ async fn relative_additional_permissions_resolve_against_tool_workdir() -> Resul
|
||||
Some("nested"),
|
||||
json!({
|
||||
"file_system": {
|
||||
"write": ["./relative-write.txt"],
|
||||
"write": ["."],
|
||||
},
|
||||
}),
|
||||
)?;
|
||||
@@ -366,7 +370,8 @@ async fn relative_additional_permissions_resolve_against_tool_workdir() -> Resul
|
||||
|
||||
#[tokio::test(flavor = "current_thread")]
|
||||
#[cfg(target_os = "macos")]
|
||||
async fn read_only_with_additional_permissions_widens_to_unrequested_cwd_write() -> Result<()> {
|
||||
async fn read_only_with_additional_permissions_does_not_widen_to_unrequested_cwd_write()
|
||||
-> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
skip_if_sandbox!(Ok(()));
|
||||
|
||||
@@ -440,16 +445,18 @@ async fn read_only_with_additional_permissions_widens_to_unrequested_cwd_write()
|
||||
|
||||
let result = parse_result(&results.single_request().function_call_output(call_id));
|
||||
assert!(
|
||||
result.exit_code.is_none() || result.exit_code == Some(0),
|
||||
"unexpected exit code/output: {:?} {}",
|
||||
result.exit_code != Some(0),
|
||||
"unrequested cwd write should stay denied: {:?} {}",
|
||||
result.exit_code,
|
||||
result.stdout
|
||||
);
|
||||
assert!(result.stdout.contains("cwd-widened"));
|
||||
assert_eq!(fs::read_to_string(&unrequested_write)?, "cwd-widened");
|
||||
assert!(
|
||||
!requested_write.exists(),
|
||||
"only the unrequested cwd path should have been written"
|
||||
"requested path should remain untouched when the command targets an unrequested file"
|
||||
);
|
||||
assert!(
|
||||
!unrequested_write.exists(),
|
||||
"unrequested cwd write should remain blocked"
|
||||
);
|
||||
|
||||
let _ = fs::remove_file(unrequested_write);
|
||||
@@ -459,7 +466,8 @@ async fn read_only_with_additional_permissions_widens_to_unrequested_cwd_write()
|
||||
|
||||
#[tokio::test(flavor = "current_thread")]
|
||||
#[cfg(target_os = "macos")]
|
||||
async fn read_only_with_additional_permissions_widens_to_unrequested_tmp_write() -> Result<()> {
|
||||
async fn read_only_with_additional_permissions_does_not_widen_to_unrequested_tmp_write()
|
||||
-> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
skip_if_sandbox!(Ok(()));
|
||||
|
||||
@@ -534,16 +542,18 @@ async fn read_only_with_additional_permissions_widens_to_unrequested_tmp_write()
|
||||
|
||||
let result = parse_result(&results.single_request().function_call_output(call_id));
|
||||
assert!(
|
||||
result.exit_code.is_none() || result.exit_code == Some(0),
|
||||
"unexpected exit code/output: {:?} {}",
|
||||
result.exit_code != Some(0),
|
||||
"unrequested tmp write should stay denied: {:?} {}",
|
||||
result.exit_code,
|
||||
result.stdout
|
||||
);
|
||||
assert!(result.stdout.contains("tmp-widened"));
|
||||
assert_eq!(fs::read_to_string(&tmp_write)?, "tmp-widened");
|
||||
assert!(
|
||||
!requested_write.exists(),
|
||||
"only the unrequested tmp path should have been written"
|
||||
"requested path should remain untouched when the command targets an unrequested file"
|
||||
);
|
||||
assert!(
|
||||
!tmp_write.exists(),
|
||||
"unrequested tmp write should remain blocked"
|
||||
);
|
||||
|
||||
let _ = fs::remove_file(tmp_write);
|
||||
|
||||
@@ -8,6 +8,7 @@ use std::path::Path;
|
||||
use codex_core::error::CodexErr;
|
||||
use codex_core::error::Result;
|
||||
use codex_core::error::SandboxErr;
|
||||
use codex_protocol::protocol::NetworkSandboxPolicy;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
|
||||
@@ -40,13 +41,14 @@ use seccompiler::apply_filter;
|
||||
/// Filesystem restrictions are intentionally handled by bubblewrap.
|
||||
pub(crate) fn apply_sandbox_policy_to_current_thread(
|
||||
sandbox_policy: &SandboxPolicy,
|
||||
network_sandbox_policy: NetworkSandboxPolicy,
|
||||
cwd: &Path,
|
||||
apply_landlock_fs: bool,
|
||||
allow_network_for_proxy: bool,
|
||||
proxy_routed_network: bool,
|
||||
) -> Result<()> {
|
||||
let network_seccomp_mode = network_seccomp_mode(
|
||||
sandbox_policy,
|
||||
network_sandbox_policy,
|
||||
allow_network_for_proxy,
|
||||
proxy_routed_network,
|
||||
);
|
||||
@@ -91,20 +93,20 @@ enum NetworkSeccompMode {
|
||||
}
|
||||
|
||||
fn should_install_network_seccomp(
|
||||
sandbox_policy: &SandboxPolicy,
|
||||
network_sandbox_policy: NetworkSandboxPolicy,
|
||||
allow_network_for_proxy: bool,
|
||||
) -> bool {
|
||||
// Managed-network sessions should remain fail-closed even for policies that
|
||||
// would normally grant full network access (for example, DangerFullAccess).
|
||||
!sandbox_policy.has_full_network_access() || allow_network_for_proxy
|
||||
!network_sandbox_policy.is_enabled() || allow_network_for_proxy
|
||||
}
|
||||
|
||||
fn network_seccomp_mode(
|
||||
sandbox_policy: &SandboxPolicy,
|
||||
network_sandbox_policy: NetworkSandboxPolicy,
|
||||
allow_network_for_proxy: bool,
|
||||
proxy_routed_network: bool,
|
||||
) -> Option<NetworkSeccompMode> {
|
||||
if !should_install_network_seccomp(sandbox_policy, allow_network_for_proxy) {
|
||||
if !should_install_network_seccomp(network_sandbox_policy, allow_network_for_proxy) {
|
||||
None
|
||||
} else if proxy_routed_network {
|
||||
Some(NetworkSeccompMode::ProxyRouted)
|
||||
@@ -266,13 +268,13 @@ mod tests {
|
||||
use super::NetworkSeccompMode;
|
||||
use super::network_seccomp_mode;
|
||||
use super::should_install_network_seccomp;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
use codex_protocol::protocol::NetworkSandboxPolicy;
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
#[test]
|
||||
fn managed_network_enforces_seccomp_even_for_full_network_policy() {
|
||||
assert_eq!(
|
||||
should_install_network_seccomp(&SandboxPolicy::DangerFullAccess, true),
|
||||
should_install_network_seccomp(NetworkSandboxPolicy::Enabled, true),
|
||||
true
|
||||
);
|
||||
}
|
||||
@@ -280,7 +282,7 @@ mod tests {
|
||||
#[test]
|
||||
fn full_network_policy_without_managed_network_skips_seccomp() {
|
||||
assert_eq!(
|
||||
should_install_network_seccomp(&SandboxPolicy::DangerFullAccess, false),
|
||||
should_install_network_seccomp(NetworkSandboxPolicy::Enabled, false),
|
||||
false
|
||||
);
|
||||
}
|
||||
@@ -288,11 +290,11 @@ mod tests {
|
||||
#[test]
|
||||
fn restricted_network_policy_always_installs_seccomp() {
|
||||
assert!(should_install_network_seccomp(
|
||||
&SandboxPolicy::new_read_only_policy(),
|
||||
NetworkSandboxPolicy::Restricted,
|
||||
false
|
||||
));
|
||||
assert!(should_install_network_seccomp(
|
||||
&SandboxPolicy::new_read_only_policy(),
|
||||
NetworkSandboxPolicy::Restricted,
|
||||
true
|
||||
));
|
||||
}
|
||||
@@ -300,7 +302,7 @@ mod tests {
|
||||
#[test]
|
||||
fn managed_proxy_routes_use_proxy_routed_seccomp_mode() {
|
||||
assert_eq!(
|
||||
network_seccomp_mode(&SandboxPolicy::DangerFullAccess, true, true),
|
||||
network_seccomp_mode(NetworkSandboxPolicy::Enabled, true, true),
|
||||
Some(NetworkSeccompMode::ProxyRouted)
|
||||
);
|
||||
}
|
||||
@@ -308,7 +310,7 @@ mod tests {
|
||||
#[test]
|
||||
fn restricted_network_without_proxy_routing_uses_restricted_mode() {
|
||||
assert_eq!(
|
||||
network_seccomp_mode(&SandboxPolicy::new_read_only_policy(), false, false),
|
||||
network_seccomp_mode(NetworkSandboxPolicy::Restricted, false, false),
|
||||
Some(NetworkSeccompMode::Restricted)
|
||||
);
|
||||
}
|
||||
@@ -316,7 +318,7 @@ mod tests {
|
||||
#[test]
|
||||
fn full_network_without_managed_proxy_skips_network_seccomp_mode() {
|
||||
assert_eq!(
|
||||
network_seccomp_mode(&SandboxPolicy::DangerFullAccess, false, false),
|
||||
network_seccomp_mode(NetworkSandboxPolicy::Enabled, false, false),
|
||||
None
|
||||
);
|
||||
}
|
||||
|
||||
@@ -14,6 +14,9 @@ use crate::proxy_routing::activate_proxy_routes_in_netns;
|
||||
use crate::proxy_routing::prepare_host_proxy_route_spec;
|
||||
use crate::vendored_bwrap::exec_vendored_bwrap;
|
||||
use crate::vendored_bwrap::run_vendored_bwrap_main;
|
||||
use codex_protocol::protocol::FileSystemSandboxPolicy;
|
||||
use codex_protocol::protocol::NetworkSandboxPolicy;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
|
||||
#[derive(Debug, Parser)]
|
||||
/// CLI surface for the Linux sandbox helper.
|
||||
@@ -26,8 +29,18 @@ pub struct LandlockCommand {
|
||||
#[arg(long = "sandbox-policy-cwd")]
|
||||
pub sandbox_policy_cwd: PathBuf,
|
||||
|
||||
#[arg(long = "sandbox-policy")]
|
||||
pub sandbox_policy: codex_protocol::protocol::SandboxPolicy,
|
||||
/// Legacy compatibility policy.
|
||||
///
|
||||
/// Newer callers pass split filesystem/network policies as well so the
|
||||
/// helper can migrate incrementally without breaking older invocations.
|
||||
#[arg(long = "sandbox-policy", hide = true)]
|
||||
pub sandbox_policy: Option<SandboxPolicy>,
|
||||
|
||||
#[arg(long = "file-system-sandbox-policy", hide = true)]
|
||||
pub file_system_sandbox_policy: Option<FileSystemSandboxPolicy>,
|
||||
|
||||
#[arg(long = "network-sandbox-policy", hide = true)]
|
||||
pub network_sandbox_policy: Option<NetworkSandboxPolicy>,
|
||||
|
||||
/// Opt-in: use the bubblewrap-based Linux sandbox pipeline.
|
||||
///
|
||||
@@ -77,6 +90,8 @@ pub fn run_main() -> ! {
|
||||
let LandlockCommand {
|
||||
sandbox_policy_cwd,
|
||||
sandbox_policy,
|
||||
file_system_sandbox_policy,
|
||||
network_sandbox_policy,
|
||||
use_bwrap_sandbox,
|
||||
apply_seccomp_then_exec,
|
||||
allow_network_for_proxy,
|
||||
@@ -89,6 +104,16 @@ pub fn run_main() -> ! {
|
||||
panic!("No command specified to execute.");
|
||||
}
|
||||
ensure_inner_stage_mode_is_valid(apply_seccomp_then_exec, use_bwrap_sandbox);
|
||||
let EffectiveSandboxPolicies {
|
||||
sandbox_policy,
|
||||
file_system_sandbox_policy,
|
||||
network_sandbox_policy,
|
||||
} = resolve_sandbox_policies(
|
||||
sandbox_policy_cwd.as_path(),
|
||||
sandbox_policy,
|
||||
file_system_sandbox_policy,
|
||||
network_sandbox_policy,
|
||||
);
|
||||
|
||||
// Inner stage: apply seccomp/no_new_privs after bubblewrap has already
|
||||
// established the filesystem view.
|
||||
@@ -104,6 +129,7 @@ pub fn run_main() -> ! {
|
||||
let proxy_routing_active = allow_network_for_proxy;
|
||||
if let Err(e) = apply_sandbox_policy_to_current_thread(
|
||||
&sandbox_policy,
|
||||
network_sandbox_policy,
|
||||
&sandbox_policy_cwd,
|
||||
false,
|
||||
allow_network_for_proxy,
|
||||
@@ -114,9 +140,10 @@ pub fn run_main() -> ! {
|
||||
exec_or_panic(command);
|
||||
}
|
||||
|
||||
if sandbox_policy.has_full_disk_write_access() && !allow_network_for_proxy {
|
||||
if file_system_sandbox_policy.has_full_disk_write_access() && !allow_network_for_proxy {
|
||||
if let Err(e) = apply_sandbox_policy_to_current_thread(
|
||||
&sandbox_policy,
|
||||
network_sandbox_policy,
|
||||
&sandbox_policy_cwd,
|
||||
false,
|
||||
allow_network_for_proxy,
|
||||
@@ -139,17 +166,20 @@ pub fn run_main() -> ! {
|
||||
} else {
|
||||
None
|
||||
};
|
||||
let inner = build_inner_seccomp_command(
|
||||
&sandbox_policy_cwd,
|
||||
&sandbox_policy,
|
||||
let inner = build_inner_seccomp_command(InnerSeccompCommandArgs {
|
||||
sandbox_policy_cwd: &sandbox_policy_cwd,
|
||||
sandbox_policy: &sandbox_policy,
|
||||
file_system_sandbox_policy: &file_system_sandbox_policy,
|
||||
network_sandbox_policy,
|
||||
use_bwrap_sandbox,
|
||||
allow_network_for_proxy,
|
||||
proxy_route_spec,
|
||||
command,
|
||||
);
|
||||
});
|
||||
run_bwrap_with_proc_fallback(
|
||||
&sandbox_policy_cwd,
|
||||
&sandbox_policy,
|
||||
network_sandbox_policy,
|
||||
inner,
|
||||
!no_proc,
|
||||
allow_network_for_proxy,
|
||||
@@ -159,6 +189,7 @@ pub fn run_main() -> ! {
|
||||
// Legacy path: Landlock enforcement only, when bwrap sandboxing is not enabled.
|
||||
if let Err(e) = apply_sandbox_policy_to_current_thread(
|
||||
&sandbox_policy,
|
||||
network_sandbox_policy,
|
||||
&sandbox_policy_cwd,
|
||||
true,
|
||||
allow_network_for_proxy,
|
||||
@@ -169,6 +200,59 @@ pub fn run_main() -> ! {
|
||||
exec_or_panic(command);
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone)]
|
||||
struct EffectiveSandboxPolicies {
|
||||
sandbox_policy: SandboxPolicy,
|
||||
file_system_sandbox_policy: FileSystemSandboxPolicy,
|
||||
network_sandbox_policy: NetworkSandboxPolicy,
|
||||
}
|
||||
|
||||
fn resolve_sandbox_policies(
|
||||
sandbox_policy_cwd: &Path,
|
||||
sandbox_policy: Option<SandboxPolicy>,
|
||||
file_system_sandbox_policy: Option<FileSystemSandboxPolicy>,
|
||||
network_sandbox_policy: Option<NetworkSandboxPolicy>,
|
||||
) -> EffectiveSandboxPolicies {
|
||||
// Accept either a fully legacy policy, a fully split policy pair, or all
|
||||
// three views together. Reject partial split-policy input so the helper
|
||||
// never runs with mismatched filesystem/network state.
|
||||
let split_policies = match (file_system_sandbox_policy, network_sandbox_policy) {
|
||||
(Some(file_system_sandbox_policy), Some(network_sandbox_policy)) => {
|
||||
Some((file_system_sandbox_policy, network_sandbox_policy))
|
||||
}
|
||||
(None, None) => None,
|
||||
_ => panic!("file-system and network sandbox policies must be provided together"),
|
||||
};
|
||||
|
||||
match (sandbox_policy, split_policies) {
|
||||
(Some(sandbox_policy), Some((file_system_sandbox_policy, network_sandbox_policy))) => {
|
||||
EffectiveSandboxPolicies {
|
||||
sandbox_policy,
|
||||
file_system_sandbox_policy,
|
||||
network_sandbox_policy,
|
||||
}
|
||||
}
|
||||
(Some(sandbox_policy), None) => EffectiveSandboxPolicies {
|
||||
file_system_sandbox_policy: FileSystemSandboxPolicy::from(&sandbox_policy),
|
||||
network_sandbox_policy: NetworkSandboxPolicy::from(&sandbox_policy),
|
||||
sandbox_policy,
|
||||
},
|
||||
(None, Some((file_system_sandbox_policy, network_sandbox_policy))) => {
|
||||
let sandbox_policy = file_system_sandbox_policy
|
||||
.to_legacy_sandbox_policy(network_sandbox_policy, sandbox_policy_cwd)
|
||||
.unwrap_or_else(|err| {
|
||||
panic!("failed to derive legacy sandbox policy from split policies: {err}")
|
||||
});
|
||||
EffectiveSandboxPolicies {
|
||||
sandbox_policy,
|
||||
file_system_sandbox_policy,
|
||||
network_sandbox_policy,
|
||||
}
|
||||
}
|
||||
(None, None) => panic!("missing sandbox policy configuration"),
|
||||
}
|
||||
}
|
||||
|
||||
fn ensure_inner_stage_mode_is_valid(apply_seccomp_then_exec: bool, use_bwrap_sandbox: bool) {
|
||||
if apply_seccomp_then_exec && !use_bwrap_sandbox {
|
||||
panic!("--apply-seccomp-then-exec requires --use-bwrap-sandbox");
|
||||
@@ -177,12 +261,13 @@ fn ensure_inner_stage_mode_is_valid(apply_seccomp_then_exec: bool, use_bwrap_san
|
||||
|
||||
fn run_bwrap_with_proc_fallback(
|
||||
sandbox_policy_cwd: &Path,
|
||||
sandbox_policy: &codex_protocol::protocol::SandboxPolicy,
|
||||
sandbox_policy: &SandboxPolicy,
|
||||
network_sandbox_policy: NetworkSandboxPolicy,
|
||||
inner: Vec<String>,
|
||||
mount_proc: bool,
|
||||
allow_network_for_proxy: bool,
|
||||
) -> ! {
|
||||
let network_mode = bwrap_network_mode(sandbox_policy, allow_network_for_proxy);
|
||||
let network_mode = bwrap_network_mode(network_sandbox_policy, allow_network_for_proxy);
|
||||
let mut mount_proc = mount_proc;
|
||||
|
||||
if mount_proc && !preflight_proc_mount_support(sandbox_policy_cwd, sandbox_policy, network_mode)
|
||||
@@ -200,12 +285,12 @@ fn run_bwrap_with_proc_fallback(
|
||||
}
|
||||
|
||||
fn bwrap_network_mode(
|
||||
sandbox_policy: &codex_protocol::protocol::SandboxPolicy,
|
||||
network_sandbox_policy: NetworkSandboxPolicy,
|
||||
allow_network_for_proxy: bool,
|
||||
) -> BwrapNetworkMode {
|
||||
if allow_network_for_proxy {
|
||||
BwrapNetworkMode::ProxyOnly
|
||||
} else if sandbox_policy.has_full_network_access() {
|
||||
} else if network_sandbox_policy.is_enabled() {
|
||||
BwrapNetworkMode::FullAccess
|
||||
} else {
|
||||
BwrapNetworkMode::Isolated
|
||||
@@ -214,7 +299,7 @@ fn bwrap_network_mode(
|
||||
|
||||
fn build_bwrap_argv(
|
||||
inner: Vec<String>,
|
||||
sandbox_policy: &codex_protocol::protocol::SandboxPolicy,
|
||||
sandbox_policy: &SandboxPolicy,
|
||||
sandbox_policy_cwd: &Path,
|
||||
options: BwrapOptions,
|
||||
) -> Vec<String> {
|
||||
@@ -237,7 +322,7 @@ fn build_bwrap_argv(
|
||||
|
||||
fn preflight_proc_mount_support(
|
||||
sandbox_policy_cwd: &Path,
|
||||
sandbox_policy: &codex_protocol::protocol::SandboxPolicy,
|
||||
sandbox_policy: &SandboxPolicy,
|
||||
network_mode: BwrapNetworkMode,
|
||||
) -> bool {
|
||||
let preflight_argv =
|
||||
@@ -248,7 +333,7 @@ fn preflight_proc_mount_support(
|
||||
|
||||
fn build_preflight_bwrap_argv(
|
||||
sandbox_policy_cwd: &Path,
|
||||
sandbox_policy: &codex_protocol::protocol::SandboxPolicy,
|
||||
sandbox_policy: &SandboxPolicy,
|
||||
network_mode: BwrapNetworkMode,
|
||||
) -> Vec<String> {
|
||||
let preflight_command = vec![resolve_true_command()];
|
||||
@@ -358,15 +443,29 @@ fn is_proc_mount_failure(stderr: &str) -> bool {
|
||||
|| stderr.contains("Permission denied"))
|
||||
}
|
||||
|
||||
/// Build the inner command that applies seccomp after bubblewrap.
|
||||
fn build_inner_seccomp_command(
|
||||
sandbox_policy_cwd: &Path,
|
||||
sandbox_policy: &codex_protocol::protocol::SandboxPolicy,
|
||||
struct InnerSeccompCommandArgs<'a> {
|
||||
sandbox_policy_cwd: &'a Path,
|
||||
sandbox_policy: &'a SandboxPolicy,
|
||||
file_system_sandbox_policy: &'a FileSystemSandboxPolicy,
|
||||
network_sandbox_policy: NetworkSandboxPolicy,
|
||||
use_bwrap_sandbox: bool,
|
||||
allow_network_for_proxy: bool,
|
||||
proxy_route_spec: Option<String>,
|
||||
command: Vec<String>,
|
||||
) -> Vec<String> {
|
||||
}
|
||||
|
||||
/// Build the inner command that applies seccomp after bubblewrap.
|
||||
fn build_inner_seccomp_command(args: InnerSeccompCommandArgs<'_>) -> Vec<String> {
|
||||
let InnerSeccompCommandArgs {
|
||||
sandbox_policy_cwd,
|
||||
sandbox_policy,
|
||||
file_system_sandbox_policy,
|
||||
network_sandbox_policy,
|
||||
use_bwrap_sandbox,
|
||||
allow_network_for_proxy,
|
||||
proxy_route_spec,
|
||||
command,
|
||||
} = args;
|
||||
let current_exe = match std::env::current_exe() {
|
||||
Ok(path) => path,
|
||||
Err(err) => panic!("failed to resolve current executable path: {err}"),
|
||||
@@ -375,6 +474,14 @@ fn build_inner_seccomp_command(
|
||||
Ok(json) => json,
|
||||
Err(err) => panic!("failed to serialize sandbox policy: {err}"),
|
||||
};
|
||||
let file_system_policy_json = match serde_json::to_string(file_system_sandbox_policy) {
|
||||
Ok(json) => json,
|
||||
Err(err) => panic!("failed to serialize filesystem sandbox policy: {err}"),
|
||||
};
|
||||
let network_policy_json = match serde_json::to_string(&network_sandbox_policy) {
|
||||
Ok(json) => json,
|
||||
Err(err) => panic!("failed to serialize network sandbox policy: {err}"),
|
||||
};
|
||||
|
||||
let mut inner = vec![
|
||||
current_exe.to_string_lossy().to_string(),
|
||||
@@ -382,6 +489,10 @@ fn build_inner_seccomp_command(
|
||||
sandbox_policy_cwd.to_string_lossy().to_string(),
|
||||
"--sandbox-policy".to_string(),
|
||||
policy_json,
|
||||
"--file-system-sandbox-policy".to_string(),
|
||||
file_system_policy_json,
|
||||
"--network-sandbox-policy".to_string(),
|
||||
network_policy_json,
|
||||
];
|
||||
if use_bwrap_sandbox {
|
||||
inner.push("--use-bwrap-sandbox".to_string());
|
||||
|
||||
@@ -1,7 +1,13 @@
|
||||
#[cfg(test)]
|
||||
use super::*;
|
||||
#[cfg(test)]
|
||||
use codex_protocol::protocol::FileSystemSandboxPolicy;
|
||||
#[cfg(test)]
|
||||
use codex_protocol::protocol::NetworkSandboxPolicy;
|
||||
#[cfg(test)]
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
#[cfg(test)]
|
||||
use pretty_assertions::assert_eq;
|
||||
|
||||
#[test]
|
||||
fn detects_proc_mount_invalid_argument_failure() {
|
||||
@@ -91,42 +97,66 @@ fn inserts_unshare_net_when_proxy_only_network_mode_requested() {
|
||||
|
||||
#[test]
|
||||
fn proxy_only_mode_takes_precedence_over_full_network_policy() {
|
||||
let mode = bwrap_network_mode(&SandboxPolicy::DangerFullAccess, true);
|
||||
let mode = bwrap_network_mode(NetworkSandboxPolicy::Enabled, true);
|
||||
assert_eq!(mode, BwrapNetworkMode::ProxyOnly);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn managed_proxy_preflight_argv_is_wrapped_for_full_access_policy() {
|
||||
let mode = bwrap_network_mode(&SandboxPolicy::DangerFullAccess, true);
|
||||
let mode = bwrap_network_mode(NetworkSandboxPolicy::Enabled, true);
|
||||
let argv = build_preflight_bwrap_argv(Path::new("/"), &SandboxPolicy::DangerFullAccess, mode);
|
||||
assert!(argv.iter().any(|arg| arg == "--"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn managed_proxy_inner_command_includes_route_spec() {
|
||||
let args = build_inner_seccomp_command(
|
||||
Path::new("/tmp"),
|
||||
&SandboxPolicy::new_read_only_policy(),
|
||||
true,
|
||||
true,
|
||||
Some("{\"routes\":[]}".to_string()),
|
||||
vec!["/bin/true".to_string()],
|
||||
);
|
||||
let sandbox_policy = SandboxPolicy::new_read_only_policy();
|
||||
let args = build_inner_seccomp_command(InnerSeccompCommandArgs {
|
||||
sandbox_policy_cwd: Path::new("/tmp"),
|
||||
sandbox_policy: &sandbox_policy,
|
||||
file_system_sandbox_policy: &FileSystemSandboxPolicy::from(&sandbox_policy),
|
||||
network_sandbox_policy: NetworkSandboxPolicy::Restricted,
|
||||
use_bwrap_sandbox: true,
|
||||
allow_network_for_proxy: true,
|
||||
proxy_route_spec: Some("{\"routes\":[]}".to_string()),
|
||||
command: vec!["/bin/true".to_string()],
|
||||
});
|
||||
|
||||
assert!(args.iter().any(|arg| arg == "--proxy-route-spec"));
|
||||
assert!(args.iter().any(|arg| arg == "{\"routes\":[]}"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn inner_command_includes_split_policy_flags() {
|
||||
let sandbox_policy = SandboxPolicy::new_read_only_policy();
|
||||
let args = build_inner_seccomp_command(InnerSeccompCommandArgs {
|
||||
sandbox_policy_cwd: Path::new("/tmp"),
|
||||
sandbox_policy: &sandbox_policy,
|
||||
file_system_sandbox_policy: &FileSystemSandboxPolicy::from(&sandbox_policy),
|
||||
network_sandbox_policy: NetworkSandboxPolicy::Restricted,
|
||||
use_bwrap_sandbox: true,
|
||||
allow_network_for_proxy: false,
|
||||
proxy_route_spec: None,
|
||||
command: vec!["/bin/true".to_string()],
|
||||
});
|
||||
|
||||
assert!(args.iter().any(|arg| arg == "--file-system-sandbox-policy"));
|
||||
assert!(args.iter().any(|arg| arg == "--network-sandbox-policy"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn non_managed_inner_command_omits_route_spec() {
|
||||
let args = build_inner_seccomp_command(
|
||||
Path::new("/tmp"),
|
||||
&SandboxPolicy::new_read_only_policy(),
|
||||
true,
|
||||
false,
|
||||
None,
|
||||
vec!["/bin/true".to_string()],
|
||||
);
|
||||
let sandbox_policy = SandboxPolicy::new_read_only_policy();
|
||||
let args = build_inner_seccomp_command(InnerSeccompCommandArgs {
|
||||
sandbox_policy_cwd: Path::new("/tmp"),
|
||||
sandbox_policy: &sandbox_policy,
|
||||
file_system_sandbox_policy: &FileSystemSandboxPolicy::from(&sandbox_policy),
|
||||
network_sandbox_policy: NetworkSandboxPolicy::Restricted,
|
||||
use_bwrap_sandbox: true,
|
||||
allow_network_for_proxy: false,
|
||||
proxy_route_spec: None,
|
||||
command: vec!["/bin/true".to_string()],
|
||||
});
|
||||
|
||||
assert!(!args.iter().any(|arg| arg == "--proxy-route-spec"));
|
||||
}
|
||||
@@ -134,15 +164,71 @@ fn non_managed_inner_command_omits_route_spec() {
|
||||
#[test]
|
||||
fn managed_proxy_inner_command_requires_route_spec() {
|
||||
let result = std::panic::catch_unwind(|| {
|
||||
build_inner_seccomp_command(
|
||||
let sandbox_policy = SandboxPolicy::new_read_only_policy();
|
||||
build_inner_seccomp_command(InnerSeccompCommandArgs {
|
||||
sandbox_policy_cwd: Path::new("/tmp"),
|
||||
sandbox_policy: &sandbox_policy,
|
||||
file_system_sandbox_policy: &FileSystemSandboxPolicy::from(&sandbox_policy),
|
||||
network_sandbox_policy: NetworkSandboxPolicy::Restricted,
|
||||
use_bwrap_sandbox: true,
|
||||
allow_network_for_proxy: true,
|
||||
proxy_route_spec: None,
|
||||
command: vec!["/bin/true".to_string()],
|
||||
})
|
||||
});
|
||||
assert!(result.is_err());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn resolve_sandbox_policies_derives_split_policies_from_legacy_policy() {
|
||||
let sandbox_policy = SandboxPolicy::new_read_only_policy();
|
||||
|
||||
let resolved =
|
||||
resolve_sandbox_policies(Path::new("/tmp"), Some(sandbox_policy.clone()), None, None);
|
||||
|
||||
assert_eq!(resolved.sandbox_policy, sandbox_policy);
|
||||
assert_eq!(
|
||||
resolved.file_system_sandbox_policy,
|
||||
FileSystemSandboxPolicy::from(&sandbox_policy)
|
||||
);
|
||||
assert_eq!(
|
||||
resolved.network_sandbox_policy,
|
||||
NetworkSandboxPolicy::from(&sandbox_policy)
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn resolve_sandbox_policies_derives_legacy_policy_from_split_policies() {
|
||||
let sandbox_policy = SandboxPolicy::new_read_only_policy();
|
||||
let file_system_sandbox_policy = FileSystemSandboxPolicy::from(&sandbox_policy);
|
||||
let network_sandbox_policy = NetworkSandboxPolicy::from(&sandbox_policy);
|
||||
|
||||
let resolved = resolve_sandbox_policies(
|
||||
Path::new("/tmp"),
|
||||
None,
|
||||
Some(file_system_sandbox_policy.clone()),
|
||||
Some(network_sandbox_policy),
|
||||
);
|
||||
|
||||
assert_eq!(resolved.sandbox_policy, sandbox_policy);
|
||||
assert_eq!(
|
||||
resolved.file_system_sandbox_policy,
|
||||
file_system_sandbox_policy
|
||||
);
|
||||
assert_eq!(resolved.network_sandbox_policy, network_sandbox_policy);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn resolve_sandbox_policies_rejects_partial_split_policies() {
|
||||
let result = std::panic::catch_unwind(|| {
|
||||
resolve_sandbox_policies(
|
||||
Path::new("/tmp"),
|
||||
&SandboxPolicy::new_read_only_policy(),
|
||||
true,
|
||||
true,
|
||||
Some(SandboxPolicy::new_read_only_policy()),
|
||||
Some(FileSystemSandboxPolicy::default()),
|
||||
None,
|
||||
vec!["/bin/true".to_string()],
|
||||
)
|
||||
});
|
||||
|
||||
assert!(result.is_err());
|
||||
}
|
||||
|
||||
|
||||
@@ -727,6 +727,22 @@ impl FromStr for SandboxPolicy {
|
||||
}
|
||||
}
|
||||
|
||||
impl FromStr for FileSystemSandboxPolicy {
|
||||
type Err = serde_json::Error;
|
||||
|
||||
fn from_str(s: &str) -> Result<Self, Self::Err> {
|
||||
serde_json::from_str(s)
|
||||
}
|
||||
}
|
||||
|
||||
impl FromStr for NetworkSandboxPolicy {
|
||||
type Err = serde_json::Error;
|
||||
|
||||
fn from_str(s: &str) -> Result<Self, Self::Err> {
|
||||
serde_json::from_str(s)
|
||||
}
|
||||
}
|
||||
|
||||
impl SandboxPolicy {
|
||||
/// Returns a policy with read-only disk access and no network.
|
||||
pub fn new_read_only_policy() -> Self {
|
||||
@@ -3177,6 +3193,7 @@ mod tests {
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use pretty_assertions::assert_eq;
|
||||
use serde_json::json;
|
||||
use std::path::PathBuf;
|
||||
use tempfile::NamedTempFile;
|
||||
use tempfile::TempDir;
|
||||
|
||||
|
||||
@@ -768,12 +768,47 @@ impl App {
|
||||
}
|
||||
|
||||
async fn refresh_in_memory_config_from_disk(&mut self) -> Result<()> {
|
||||
let mut config = self.rebuild_config_for_cwd(self.config.cwd.clone()).await?;
|
||||
let mut config = self
|
||||
.rebuild_config_for_cwd(self.chat_widget.config_ref().cwd.clone())
|
||||
.await?;
|
||||
self.apply_runtime_policy_overrides(&mut config);
|
||||
self.config = config;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn refresh_in_memory_config_from_disk_best_effort(&mut self, action: &str) {
|
||||
if let Err(err) = self.refresh_in_memory_config_from_disk().await {
|
||||
tracing::warn!(
|
||||
error = %err,
|
||||
action,
|
||||
"failed to refresh config before thread transition; continuing with current in-memory config"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
async fn rebuild_config_for_resume_or_fallback(
|
||||
&mut self,
|
||||
current_cwd: &Path,
|
||||
resume_cwd: PathBuf,
|
||||
) -> Result<Config> {
|
||||
match self.rebuild_config_for_cwd(resume_cwd.clone()).await {
|
||||
Ok(config) => Ok(config),
|
||||
Err(err) => {
|
||||
if crate::cwds_differ(current_cwd, &resume_cwd) {
|
||||
Err(err)
|
||||
} else {
|
||||
let resume_cwd_display = resume_cwd.display().to_string();
|
||||
tracing::warn!(
|
||||
error = %err,
|
||||
cwd = %resume_cwd_display,
|
||||
"failed to rebuild config for same-cwd resume; using current in-memory config"
|
||||
);
|
||||
Ok(self.config.clone())
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn apply_runtime_policy_overrides(&mut self, config: &mut Config) {
|
||||
if let Some(policy) = self.runtime_approval_policy_override.as_ref()
|
||||
&& let Err(err) = config.permissions.approval_policy.set(*policy)
|
||||
@@ -1482,6 +1517,8 @@ impl App {
|
||||
async fn start_fresh_session_with_summary_hint(&mut self, tui: &mut tui::Tui) {
|
||||
// Start a fresh in-memory session while preserving resumability via persisted rollout
|
||||
// history.
|
||||
self.refresh_in_memory_config_from_disk_best_effort("starting a new thread")
|
||||
.await;
|
||||
let model = self.chat_widget.current_model().to_string();
|
||||
let config = self.fresh_session_config();
|
||||
let summary = session_summary(
|
||||
@@ -2078,19 +2115,17 @@ impl App {
|
||||
return Ok(AppRunControl::Exit(ExitReason::UserRequested));
|
||||
}
|
||||
};
|
||||
let mut resume_config = if crate::cwds_differ(¤t_cwd, &resume_cwd) {
|
||||
match self.rebuild_config_for_cwd(resume_cwd).await {
|
||||
Ok(cfg) => cfg,
|
||||
Err(err) => {
|
||||
self.chat_widget.add_error_message(format!(
|
||||
"Failed to rebuild configuration for resume: {err}"
|
||||
));
|
||||
return Ok(AppRunControl::Continue);
|
||||
}
|
||||
let mut resume_config = match self
|
||||
.rebuild_config_for_resume_or_fallback(¤t_cwd, resume_cwd)
|
||||
.await
|
||||
{
|
||||
Ok(cfg) => cfg,
|
||||
Err(err) => {
|
||||
self.chat_widget.add_error_message(format!(
|
||||
"Failed to rebuild configuration for resume: {err}"
|
||||
));
|
||||
return Ok(AppRunControl::Continue);
|
||||
}
|
||||
} else {
|
||||
// No rebuild needed: current_cwd comes from self.config.cwd.
|
||||
self.config.clone()
|
||||
};
|
||||
self.apply_runtime_policy_overrides(&mut resume_config);
|
||||
let summary = session_summary(
|
||||
@@ -2165,6 +2200,8 @@ impl App {
|
||||
self.chat_widget
|
||||
.add_plain_history_lines(vec!["/fork".magenta().into()]);
|
||||
if let Some(path) = self.chat_widget.rollout_path() {
|
||||
self.refresh_in_memory_config_from_disk_best_effort("forking the thread")
|
||||
.await;
|
||||
// Fresh threads expose a precomputed path, but the file is
|
||||
// materialized lazily on first user message.
|
||||
if path.exists() {
|
||||
@@ -5907,6 +5944,95 @@ mod tests {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn refresh_in_memory_config_from_disk_best_effort_keeps_current_config_on_error()
|
||||
-> Result<()> {
|
||||
let mut app = make_test_app().await;
|
||||
let codex_home = tempdir()?;
|
||||
app.config.codex_home = codex_home.path().to_path_buf();
|
||||
std::fs::write(codex_home.path().join("config.toml"), "[broken")?;
|
||||
let original_config = app.config.clone();
|
||||
|
||||
app.refresh_in_memory_config_from_disk_best_effort("starting a new thread")
|
||||
.await;
|
||||
|
||||
assert_eq!(app.config, original_config);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn refresh_in_memory_config_from_disk_uses_active_chat_widget_cwd() -> Result<()> {
|
||||
let mut app = make_test_app().await;
|
||||
let original_cwd = app.config.cwd.clone();
|
||||
let next_cwd_tmp = tempdir()?;
|
||||
let next_cwd = next_cwd_tmp.path().to_path_buf();
|
||||
|
||||
app.chat_widget.handle_codex_event(Event {
|
||||
id: String::new(),
|
||||
msg: EventMsg::SessionConfigured(SessionConfiguredEvent {
|
||||
session_id: ThreadId::new(),
|
||||
forked_from_id: None,
|
||||
thread_name: None,
|
||||
model: "gpt-test".to_string(),
|
||||
model_provider_id: "test-provider".to_string(),
|
||||
service_tier: None,
|
||||
approval_policy: AskForApproval::Never,
|
||||
sandbox_policy: SandboxPolicy::new_read_only_policy(),
|
||||
cwd: next_cwd.clone(),
|
||||
reasoning_effort: None,
|
||||
history_log_id: 0,
|
||||
history_entry_count: 0,
|
||||
initial_messages: None,
|
||||
network_proxy: None,
|
||||
rollout_path: Some(PathBuf::new()),
|
||||
}),
|
||||
});
|
||||
|
||||
assert_eq!(app.chat_widget.config_ref().cwd, next_cwd);
|
||||
assert_eq!(app.config.cwd, original_cwd);
|
||||
|
||||
app.refresh_in_memory_config_from_disk().await?;
|
||||
|
||||
assert_eq!(app.config.cwd, app.chat_widget.config_ref().cwd);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn rebuild_config_for_resume_or_fallback_uses_current_config_on_same_cwd_error()
|
||||
-> Result<()> {
|
||||
let mut app = make_test_app().await;
|
||||
let codex_home = tempdir()?;
|
||||
app.config.codex_home = codex_home.path().to_path_buf();
|
||||
std::fs::write(codex_home.path().join("config.toml"), "[broken")?;
|
||||
let current_config = app.config.clone();
|
||||
let current_cwd = current_config.cwd.clone();
|
||||
|
||||
let resume_config = app
|
||||
.rebuild_config_for_resume_or_fallback(¤t_cwd, current_cwd.clone())
|
||||
.await?;
|
||||
|
||||
assert_eq!(resume_config, current_config);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn rebuild_config_for_resume_or_fallback_errors_when_cwd_changes() -> Result<()> {
|
||||
let mut app = make_test_app().await;
|
||||
let codex_home = tempdir()?;
|
||||
app.config.codex_home = codex_home.path().to_path_buf();
|
||||
std::fs::write(codex_home.path().join("config.toml"), "[broken")?;
|
||||
let current_cwd = app.config.cwd.clone();
|
||||
let next_cwd_tmp = tempdir()?;
|
||||
let next_cwd = next_cwd_tmp.path().to_path_buf();
|
||||
|
||||
let result = app
|
||||
.rebuild_config_for_resume_or_fallback(¤t_cwd, next_cwd)
|
||||
.await;
|
||||
|
||||
assert!(result.is_err());
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn sync_tui_theme_selection_updates_chat_widget_config_copy() {
|
||||
let mut app = make_test_app().await;
|
||||
|
||||
@@ -49,12 +49,14 @@ const MIN_OVERLAY_HEIGHT: u16 = 8;
|
||||
const APPROVAL_FIELD_ID: &str = "__approval";
|
||||
const APPROVAL_ACCEPT_ONCE_VALUE: &str = "accept";
|
||||
const APPROVAL_ACCEPT_SESSION_VALUE: &str = "accept_session";
|
||||
const APPROVAL_ACCEPT_ALWAYS_VALUE: &str = "accept_always";
|
||||
const APPROVAL_DECLINE_VALUE: &str = "decline";
|
||||
const APPROVAL_CANCEL_VALUE: &str = "cancel";
|
||||
const APPROVAL_META_KIND_KEY: &str = "codex_approval_kind";
|
||||
const APPROVAL_META_KIND_MCP_TOOL_CALL: &str = "mcp_tool_call";
|
||||
const APPROVAL_PERSIST_KEY: &str = "persist";
|
||||
const APPROVAL_PERSIST_SESSION_VALUE: &str = "session";
|
||||
const APPROVAL_PERSIST_ALWAYS_VALUE: &str = "always";
|
||||
|
||||
#[derive(Clone, PartialEq, Default)]
|
||||
struct ComposerDraft {
|
||||
@@ -181,29 +183,49 @@ impl McpServerElicitationFormRequest {
|
||||
.and_then(Value::as_object)
|
||||
.is_some_and(serde_json::Map::is_empty)
|
||||
});
|
||||
let is_tool_approval_action =
|
||||
is_tool_approval && (requested_schema.is_null() || is_empty_object_schema);
|
||||
|
||||
let (response_mode, fields) =
|
||||
if requested_schema.is_null() || (is_tool_approval && is_empty_object_schema) {
|
||||
let mut options = vec![McpServerElicitationOption {
|
||||
label: "Approve Once".to_string(),
|
||||
description: Some("Run the tool and continue.".to_string()),
|
||||
value: Value::String(APPROVAL_ACCEPT_ONCE_VALUE.to_string()),
|
||||
}];
|
||||
if meta
|
||||
.as_ref()
|
||||
.and_then(Value::as_object)
|
||||
.and_then(|meta| meta.get(APPROVAL_PERSIST_KEY))
|
||||
.and_then(Value::as_str)
|
||||
== Some(APPROVAL_PERSIST_SESSION_VALUE)
|
||||
{
|
||||
options.push(McpServerElicitationOption {
|
||||
label: "Approve this Session".to_string(),
|
||||
description: Some(
|
||||
"Run the tool and remember this choice for this session.".to_string(),
|
||||
),
|
||||
value: Value::String(APPROVAL_ACCEPT_SESSION_VALUE.to_string()),
|
||||
});
|
||||
}
|
||||
let (response_mode, fields) = if requested_schema.is_null()
|
||||
|| (is_tool_approval && is_empty_object_schema)
|
||||
{
|
||||
let mut options = vec![McpServerElicitationOption {
|
||||
label: "Approve Once".to_string(),
|
||||
description: Some("Run the tool and continue.".to_string()),
|
||||
value: Value::String(APPROVAL_ACCEPT_ONCE_VALUE.to_string()),
|
||||
}];
|
||||
if is_tool_approval_action
|
||||
&& tool_approval_supports_persist_mode(
|
||||
meta.as_ref(),
|
||||
APPROVAL_PERSIST_SESSION_VALUE,
|
||||
)
|
||||
{
|
||||
options.push(McpServerElicitationOption {
|
||||
label: "Approve this session".to_string(),
|
||||
description: Some(
|
||||
"Run the tool and remember this choice for this session.".to_string(),
|
||||
),
|
||||
value: Value::String(APPROVAL_ACCEPT_SESSION_VALUE.to_string()),
|
||||
});
|
||||
}
|
||||
if is_tool_approval_action
|
||||
&& tool_approval_supports_persist_mode(meta.as_ref(), APPROVAL_PERSIST_ALWAYS_VALUE)
|
||||
{
|
||||
options.push(McpServerElicitationOption {
|
||||
label: "Always allow".to_string(),
|
||||
description: Some(
|
||||
"Run the tool and remember this choice for future tool calls.".to_string(),
|
||||
),
|
||||
value: Value::String(APPROVAL_ACCEPT_ALWAYS_VALUE.to_string()),
|
||||
});
|
||||
}
|
||||
if is_tool_approval_action {
|
||||
options.push(McpServerElicitationOption {
|
||||
label: "Cancel".to_string(),
|
||||
description: Some("Cancel this tool call".to_string()),
|
||||
value: Value::String(APPROVAL_CANCEL_VALUE.to_string()),
|
||||
});
|
||||
} else {
|
||||
options.extend([
|
||||
McpServerElicitationOption {
|
||||
label: "Deny".to_string(),
|
||||
@@ -216,25 +238,26 @@ impl McpServerElicitationFormRequest {
|
||||
value: Value::String(APPROVAL_CANCEL_VALUE.to_string()),
|
||||
},
|
||||
]);
|
||||
(
|
||||
McpServerElicitationResponseMode::ApprovalAction,
|
||||
vec![McpServerElicitationField {
|
||||
id: APPROVAL_FIELD_ID.to_string(),
|
||||
label: String::new(),
|
||||
prompt: String::new(),
|
||||
required: true,
|
||||
input: McpServerElicitationFieldInput::Select {
|
||||
options,
|
||||
default_idx: Some(0),
|
||||
},
|
||||
}],
|
||||
)
|
||||
} else {
|
||||
(
|
||||
McpServerElicitationResponseMode::FormContent,
|
||||
parse_fields_from_schema(&requested_schema)?,
|
||||
)
|
||||
};
|
||||
}
|
||||
(
|
||||
McpServerElicitationResponseMode::ApprovalAction,
|
||||
vec![McpServerElicitationField {
|
||||
id: APPROVAL_FIELD_ID.to_string(),
|
||||
label: String::new(),
|
||||
prompt: String::new(),
|
||||
required: true,
|
||||
input: McpServerElicitationFieldInput::Select {
|
||||
options,
|
||||
default_idx: Some(0),
|
||||
},
|
||||
}],
|
||||
)
|
||||
} else {
|
||||
(
|
||||
McpServerElicitationResponseMode::FormContent,
|
||||
parse_fields_from_schema(&requested_schema)?,
|
||||
)
|
||||
};
|
||||
|
||||
Some(Self {
|
||||
thread_id,
|
||||
@@ -247,6 +270,24 @@ impl McpServerElicitationFormRequest {
|
||||
}
|
||||
}
|
||||
|
||||
fn tool_approval_supports_persist_mode(meta: Option<&Value>, expected_mode: &str) -> bool {
|
||||
let Some(persist) = meta
|
||||
.and_then(Value::as_object)
|
||||
.and_then(|meta| meta.get(APPROVAL_PERSIST_KEY))
|
||||
else {
|
||||
return false;
|
||||
};
|
||||
|
||||
match persist {
|
||||
Value::String(value) => value == expected_mode,
|
||||
Value::Array(values) => values
|
||||
.iter()
|
||||
.filter_map(Value::as_str)
|
||||
.any(|value| value == expected_mode),
|
||||
_ => false,
|
||||
}
|
||||
}
|
||||
|
||||
fn parse_fields_from_schema(requested_schema: &Value) -> Option<Vec<McpServerElicitationField>> {
|
||||
let schema = requested_schema.as_object()?;
|
||||
if schema.get("type").and_then(Value::as_str) != Some("object") {
|
||||
@@ -856,6 +897,12 @@ impl McpServerElicitationOverlay {
|
||||
APPROVAL_PERSIST_KEY: APPROVAL_PERSIST_SESSION_VALUE,
|
||||
})),
|
||||
),
|
||||
Some(APPROVAL_ACCEPT_ALWAYS_VALUE) => (
|
||||
ElicitationAction::Accept,
|
||||
Some(serde_json::json!({
|
||||
APPROVAL_PERSIST_KEY: APPROVAL_PERSIST_ALWAYS_VALUE,
|
||||
})),
|
||||
),
|
||||
Some(APPROVAL_DECLINE_VALUE) => (ElicitationAction::Decline, None),
|
||||
Some(APPROVAL_CANCEL_VALUE) => (ElicitationAction::Cancel, None),
|
||||
_ => (ElicitationAction::Cancel, None),
|
||||
@@ -1414,15 +1461,20 @@ mod tests {
|
||||
})
|
||||
}
|
||||
|
||||
fn tool_approval_meta(include_session_persist: bool) -> Option<Value> {
|
||||
fn tool_approval_meta(persist_modes: &[&str]) -> Option<Value> {
|
||||
let mut meta = serde_json::Map::from_iter([(
|
||||
APPROVAL_META_KIND_KEY.to_string(),
|
||||
Value::String(APPROVAL_META_KIND_MCP_TOOL_CALL.to_string()),
|
||||
)]);
|
||||
if include_session_persist {
|
||||
if !persist_modes.is_empty() {
|
||||
meta.insert(
|
||||
APPROVAL_PERSIST_KEY.to_string(),
|
||||
Value::String(APPROVAL_PERSIST_SESSION_VALUE.to_string()),
|
||||
Value::Array(
|
||||
persist_modes
|
||||
.iter()
|
||||
.map(|mode| Value::String((*mode).to_string()))
|
||||
.collect(),
|
||||
),
|
||||
);
|
||||
}
|
||||
Some(Value::Object(meta))
|
||||
@@ -1581,7 +1633,7 @@ mod tests {
|
||||
form_request(
|
||||
"Allow this request?",
|
||||
empty_object_schema(),
|
||||
tool_approval_meta(false),
|
||||
tool_approval_meta(&[]),
|
||||
),
|
||||
)
|
||||
.expect("expected approval fallback");
|
||||
@@ -1606,13 +1658,6 @@ mod tests {
|
||||
description: Some("Run the tool and continue.".to_string()),
|
||||
value: Value::String(APPROVAL_ACCEPT_ONCE_VALUE.to_string()),
|
||||
},
|
||||
McpServerElicitationOption {
|
||||
label: "Deny".to_string(),
|
||||
description: Some(
|
||||
"Decline this tool call and continue.".to_string(),
|
||||
),
|
||||
value: Value::String(APPROVAL_DECLINE_VALUE.to_string()),
|
||||
},
|
||||
McpServerElicitationOption {
|
||||
label: "Cancel".to_string(),
|
||||
description: Some("Cancel this tool call".to_string()),
|
||||
@@ -1696,7 +1741,10 @@ mod tests {
|
||||
form_request(
|
||||
"Allow this request?",
|
||||
empty_object_schema(),
|
||||
tool_approval_meta(true),
|
||||
tool_approval_meta(&[
|
||||
APPROVAL_PERSIST_SESSION_VALUE,
|
||||
APPROVAL_PERSIST_ALWAYS_VALUE,
|
||||
]),
|
||||
),
|
||||
)
|
||||
.expect("expected approval fallback");
|
||||
@@ -1731,6 +1779,53 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn empty_tool_approval_schema_always_allow_sets_persist_meta() {
|
||||
let (tx, mut rx) = test_sender();
|
||||
let thread_id = ThreadId::default();
|
||||
let request = McpServerElicitationFormRequest::from_event(
|
||||
thread_id,
|
||||
form_request(
|
||||
"Allow this request?",
|
||||
empty_object_schema(),
|
||||
tool_approval_meta(&[
|
||||
APPROVAL_PERSIST_SESSION_VALUE,
|
||||
APPROVAL_PERSIST_ALWAYS_VALUE,
|
||||
]),
|
||||
),
|
||||
)
|
||||
.expect("expected approval fallback");
|
||||
let mut overlay = McpServerElicitationOverlay::new(request, tx, true, false, false);
|
||||
|
||||
if let Some(answer) = overlay.current_answer_mut() {
|
||||
answer.selection.selected_idx = Some(2);
|
||||
}
|
||||
overlay.select_current_option(true);
|
||||
overlay.submit_answers();
|
||||
|
||||
let event = rx.try_recv().expect("expected resolution");
|
||||
let AppEvent::SubmitThreadOp {
|
||||
thread_id: resolved_thread_id,
|
||||
op,
|
||||
} = event
|
||||
else {
|
||||
panic!("expected SubmitThreadOp");
|
||||
};
|
||||
assert_eq!(resolved_thread_id, thread_id);
|
||||
assert_eq!(
|
||||
op,
|
||||
Op::ResolveElicitation {
|
||||
server_name: "server-1".to_string(),
|
||||
request_id: McpRequestId::String("request-1".to_string()),
|
||||
decision: ElicitationAction::Accept,
|
||||
content: None,
|
||||
meta: Some(serde_json::json!({
|
||||
APPROVAL_PERSIST_KEY: APPROVAL_PERSIST_ALWAYS_VALUE,
|
||||
})),
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn ctrl_c_cancels_elicitation() {
|
||||
let (tx, mut rx) = test_sender();
|
||||
@@ -1886,7 +1981,7 @@ mod tests {
|
||||
form_request(
|
||||
"Allow this request?",
|
||||
empty_object_schema(),
|
||||
tool_approval_meta(false),
|
||||
tool_approval_meta(&[]),
|
||||
),
|
||||
)
|
||||
.expect("expected approval fallback");
|
||||
@@ -1899,14 +1994,17 @@ mod tests {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn approval_form_tool_approval_with_session_persist_snapshot() {
|
||||
fn approval_form_tool_approval_with_persist_options_snapshot() {
|
||||
let (tx, _rx) = test_sender();
|
||||
let request = McpServerElicitationFormRequest::from_event(
|
||||
ThreadId::default(),
|
||||
form_request(
|
||||
"Allow this request?",
|
||||
empty_object_schema(),
|
||||
tool_approval_meta(true),
|
||||
tool_approval_meta(&[
|
||||
APPROVAL_PERSIST_SESSION_VALUE,
|
||||
APPROVAL_PERSIST_ALWAYS_VALUE,
|
||||
]),
|
||||
),
|
||||
)
|
||||
.expect("expected approval fallback");
|
||||
|
||||
@@ -6,8 +6,8 @@ expression: "render_snapshot(&overlay, Rect::new(0, 0, 120, 16))"
|
||||
Field 1/1
|
||||
Allow this request?
|
||||
› 1. Approve Once Run the tool and continue.
|
||||
2. Approve this Session Run the tool and remember this choice for this session.
|
||||
3. Deny Decline this tool call and continue.
|
||||
2. Approve this session Run the tool and remember this choice for this session.
|
||||
3. Always allow Run the tool and remember this choice for future tool calls.
|
||||
4. Cancel Cancel this tool call
|
||||
|
||||
|
||||
|
||||
@@ -6,8 +6,8 @@ expression: "render_snapshot(&overlay, Rect::new(0, 0, 120, 16))"
|
||||
Field 1/1
|
||||
Allow this request?
|
||||
› 1. Approve Once Run the tool and continue.
|
||||
2. Deny Decline this tool call and continue.
|
||||
3. Cancel Cancel this tool call
|
||||
2. Cancel Cancel this tool call
|
||||
|
||||
|
||||
|
||||
|
||||
|
||||
@@ -14,6 +14,7 @@ use crate::skills_helpers::skill_description;
|
||||
use crate::skills_helpers::skill_display_name;
|
||||
use codex_chatgpt::connectors::AppInfo;
|
||||
use codex_core::connectors::connector_mention_slug;
|
||||
use codex_core::mention_syntax::TOOL_MENTION_SIGIL;
|
||||
use codex_core::skills::model::SkillDependencies;
|
||||
use codex_core::skills::model::SkillInterface;
|
||||
use codex_core::skills::model::SkillMetadata;
|
||||
@@ -296,8 +297,6 @@ pub(crate) struct ToolMentions {
|
||||
linked_paths: HashMap<String, String>,
|
||||
}
|
||||
|
||||
const TOOL_MENTION_SIGIL: char = '$';
|
||||
|
||||
fn extract_tool_mentions_from_text(text: &str) -> ToolMentions {
|
||||
extract_tool_mentions_from_text_with_sigil(text, TOOL_MENTION_SIGIL)
|
||||
}
|
||||
|
||||
@@ -1,14 +1,15 @@
|
||||
use std::collections::HashMap;
|
||||
use std::collections::VecDeque;
|
||||
|
||||
use codex_core::mention_syntax::PLUGIN_TEXT_MENTION_SIGIL;
|
||||
use codex_core::mention_syntax::TOOL_MENTION_SIGIL;
|
||||
|
||||
#[derive(Clone, Debug, PartialEq, Eq)]
|
||||
pub(crate) struct LinkedMention {
|
||||
pub(crate) mention: String,
|
||||
pub(crate) path: String,
|
||||
}
|
||||
|
||||
const TOOL_MENTION_SIGIL: char = '$';
|
||||
|
||||
#[derive(Clone, Debug, PartialEq, Eq)]
|
||||
pub(crate) struct DecodedHistoryText {
|
||||
pub(crate) text: String,
|
||||
@@ -77,10 +78,7 @@ pub(crate) fn decode_history_mentions(text: &str) -> DecodedHistoryText {
|
||||
|
||||
while index < bytes.len() {
|
||||
if bytes[index] == b'['
|
||||
&& let Some((name, path, end_index)) =
|
||||
parse_linked_tool_mention(text, bytes, index, TOOL_MENTION_SIGIL)
|
||||
&& !is_common_env_var(name)
|
||||
&& is_tool_path(path)
|
||||
&& let Some((name, path, end_index)) = parse_history_linked_mention(text, bytes, index)
|
||||
{
|
||||
out.push(TOOL_MENTION_SIGIL);
|
||||
out.push_str(name);
|
||||
@@ -105,6 +103,31 @@ pub(crate) fn decode_history_mentions(text: &str) -> DecodedHistoryText {
|
||||
}
|
||||
}
|
||||
|
||||
fn parse_history_linked_mention<'a>(
|
||||
text: &'a str,
|
||||
text_bytes: &[u8],
|
||||
start: usize,
|
||||
) -> Option<(&'a str, &'a str, usize)> {
|
||||
// TUI writes `$name`, but may read plugin `[@name](plugin://...)` links from other clients.
|
||||
if let Some(mention @ (name, path, _)) =
|
||||
parse_linked_tool_mention(text, text_bytes, start, TOOL_MENTION_SIGIL)
|
||||
&& !is_common_env_var(name)
|
||||
&& is_tool_path(path)
|
||||
{
|
||||
return Some(mention);
|
||||
}
|
||||
|
||||
if let Some(mention @ (name, path, _)) =
|
||||
parse_linked_tool_mention(text, text_bytes, start, PLUGIN_TEXT_MENTION_SIGIL)
|
||||
&& !is_common_env_var(name)
|
||||
&& path.starts_with("plugin://")
|
||||
{
|
||||
return Some(mention);
|
||||
}
|
||||
|
||||
None
|
||||
}
|
||||
|
||||
fn parse_linked_tool_mention<'a>(
|
||||
text: &'a str,
|
||||
text_bytes: &[u8],
|
||||
@@ -225,6 +248,35 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn decode_history_mentions_restores_plugin_links_with_at_sigil() {
|
||||
let decoded = decode_history_mentions(
|
||||
"Use [@sample](plugin://sample@test) and [$figma](app://figma-1).",
|
||||
);
|
||||
assert_eq!(decoded.text, "Use $sample and $figma.");
|
||||
assert_eq!(
|
||||
decoded.mentions,
|
||||
vec![
|
||||
LinkedMention {
|
||||
mention: "sample".to_string(),
|
||||
path: "plugin://sample@test".to_string(),
|
||||
},
|
||||
LinkedMention {
|
||||
mention: "figma".to_string(),
|
||||
path: "app://figma-1".to_string(),
|
||||
},
|
||||
]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn decode_history_mentions_ignores_at_sigil_for_non_plugin_paths() {
|
||||
let decoded = decode_history_mentions("Use [@figma](app://figma-1).");
|
||||
|
||||
assert_eq!(decoded.text, "Use [@figma](app://figma-1).");
|
||||
assert_eq!(decoded.mentions, Vec::<LinkedMention>::new());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn encode_history_mentions_links_bound_mentions_in_order() {
|
||||
let text = "$figma then $sample then $figma then $other";
|
||||
|
||||
@@ -130,52 +130,36 @@ async fn collect_output_until_exit(
|
||||
}
|
||||
|
||||
async fn wait_for_python_repl_ready(
|
||||
writer: &tokio::sync::mpsc::Sender<Vec<u8>>,
|
||||
output_rx: &mut tokio::sync::broadcast::Receiver<Vec<u8>>,
|
||||
timeout_ms: u64,
|
||||
newline: &str,
|
||||
ready_marker: &str,
|
||||
) -> anyhow::Result<Vec<u8>> {
|
||||
let mut collected = Vec::new();
|
||||
let marker = "__codex_pty_ready__";
|
||||
let deadline = tokio::time::Instant::now() + tokio::time::Duration::from_millis(timeout_ms);
|
||||
let probe_window = tokio::time::Duration::from_millis(if cfg!(windows) { 750 } else { 250 });
|
||||
|
||||
while tokio::time::Instant::now() < deadline {
|
||||
writer
|
||||
.send(format!("print('{marker}'){newline}").into_bytes())
|
||||
.await?;
|
||||
|
||||
let probe_deadline = tokio::time::Instant::now() + probe_window;
|
||||
loop {
|
||||
let now = tokio::time::Instant::now();
|
||||
if now >= deadline || now >= probe_deadline {
|
||||
break;
|
||||
}
|
||||
let remaining = std::cmp::min(
|
||||
deadline.saturating_duration_since(now),
|
||||
probe_deadline.saturating_duration_since(now),
|
||||
);
|
||||
match tokio::time::timeout(remaining, output_rx.recv()).await {
|
||||
Ok(Ok(chunk)) => {
|
||||
collected.extend_from_slice(&chunk);
|
||||
if String::from_utf8_lossy(&collected).contains(marker) {
|
||||
return Ok(collected);
|
||||
}
|
||||
let now = tokio::time::Instant::now();
|
||||
let remaining = deadline.saturating_duration_since(now);
|
||||
match tokio::time::timeout(remaining, output_rx.recv()).await {
|
||||
Ok(Ok(chunk)) => {
|
||||
collected.extend_from_slice(&chunk);
|
||||
if String::from_utf8_lossy(&collected).contains(ready_marker) {
|
||||
return Ok(collected);
|
||||
}
|
||||
Ok(Err(tokio::sync::broadcast::error::RecvError::Lagged(_))) => continue,
|
||||
Ok(Err(tokio::sync::broadcast::error::RecvError::Closed)) => {
|
||||
anyhow::bail!(
|
||||
"PTY output closed while waiting for Python REPL readiness: {:?}",
|
||||
String::from_utf8_lossy(&collected)
|
||||
);
|
||||
}
|
||||
Err(_) => break,
|
||||
}
|
||||
Ok(Err(tokio::sync::broadcast::error::RecvError::Lagged(_))) => continue,
|
||||
Ok(Err(tokio::sync::broadcast::error::RecvError::Closed)) => {
|
||||
anyhow::bail!(
|
||||
"PTY output closed while waiting for Python REPL readiness: {:?}",
|
||||
String::from_utf8_lossy(&collected)
|
||||
);
|
||||
}
|
||||
Err(_) => break,
|
||||
}
|
||||
}
|
||||
|
||||
anyhow::bail!(
|
||||
"timed out waiting for Python REPL readiness in PTY: {:?}",
|
||||
"timed out waiting for Python REPL readiness marker {ready_marker:?} in PTY: {:?}",
|
||||
String::from_utf8_lossy(&collected)
|
||||
);
|
||||
}
|
||||
@@ -254,10 +238,17 @@ async fn pty_python_repl_emits_output_and_exits() -> anyhow::Result<()> {
|
||||
return Ok(());
|
||||
};
|
||||
|
||||
let ready_marker = "__codex_pty_ready__";
|
||||
let args = vec![
|
||||
"-i".to_string(),
|
||||
"-q".to_string(),
|
||||
"-c".to_string(),
|
||||
format!("print('{ready_marker}')"),
|
||||
];
|
||||
let env_map: HashMap<String, String> = std::env::vars().collect();
|
||||
let spawned = spawn_pty_process(
|
||||
&python,
|
||||
&[],
|
||||
&args,
|
||||
Path::new("."),
|
||||
&env_map,
|
||||
&None,
|
||||
@@ -269,7 +260,7 @@ async fn pty_python_repl_emits_output_and_exits() -> anyhow::Result<()> {
|
||||
let newline = if cfg!(windows) { "\r\n" } else { "\n" };
|
||||
let startup_timeout_ms = if cfg!(windows) { 10_000 } else { 5_000 };
|
||||
let mut output =
|
||||
wait_for_python_repl_ready(&writer, &mut output_rx, startup_timeout_ms, newline).await?;
|
||||
wait_for_python_repl_ready(&mut output_rx, startup_timeout_ms, ready_marker).await?;
|
||||
writer
|
||||
.send(format!("print('hello from pty'){newline}").into_bytes())
|
||||
.await?;
|
||||
|
||||
Reference in New Issue
Block a user