Merge f783157c27 into sapling-pr-archive-bolinfest

This commit is contained in:
Michael Bolin
2025-08-06 21:21:41 -07:00
committed by GitHub
9 changed files with 150 additions and 77 deletions

View File

@@ -7,22 +7,25 @@ pub fn summarize_sandbox_policy(sandbox_policy: &SandboxPolicy) -> String {
SandboxPolicy::WorkspaceWrite {
writable_roots,
network_access,
include_default_writable_roots,
exclude_tmpdir_env_var,
exclude_slash_tmp,
} => {
let mut summary = "workspace-write".to_string();
if !writable_roots.is_empty() {
summary.push_str(&format!(
" [{}]",
writable_roots
.iter()
.map(|p| p.to_string_lossy())
.collect::<Vec<_>>()
.join(", ")
));
let mut writable_entries = Vec::<String>::new();
if !*exclude_slash_tmp {
writable_entries.push("/tmp".to_string());
}
if !*include_default_writable_roots {
summary.push_str(" (exact writable roots)");
if !*exclude_tmpdir_env_var {
writable_entries.push("$TMPDIR".to_string());
}
writable_entries.extend(
writable_roots
.iter()
.map(|p| p.to_string_lossy().to_string()),
);
summary.push_str(&format!(" [{}]", writable_entries.join(", ")));
if *network_access {
summary.push_str(" (network access enabled)");
}

View File

@@ -364,7 +364,8 @@ impl ConfigToml {
Some(s) => SandboxPolicy::WorkspaceWrite {
writable_roots: s.writable_roots.clone(),
network_access: s.network_access,
include_default_writable_roots: true,
exclude_tmpdir_env_var: false,
exclude_slash_tmp: false,
},
None => SandboxPolicy::new_workspace_write_policy(),
},
@@ -756,7 +757,8 @@ writable_roots = [
SandboxPolicy::WorkspaceWrite {
writable_roots: vec![PathBuf::from("/tmp")],
network_access: false,
include_default_writable_roots: true,
exclude_tmpdir_env_var: false,
exclude_slash_tmp: false,
},
sandbox_workspace_write_cfg.derive_sandbox_policy(sandbox_mode_override)
);

View File

@@ -185,11 +185,16 @@ pub enum SandboxPolicy {
#[serde(default)]
network_access: bool,
/// When set to `true`, will include defaults like the current working
/// directory and TMPDIR (on macOS). When `false`, only `writable_roots`
/// are used. (Mainly used for testing.)
#[serde(default = "default_true")]
include_default_writable_roots: bool,
/// When set to `true`, will NOT include the per-user `TMPDIR`
/// environment variable among the default writable roots. Defaults to
/// `false`.
#[serde(default)]
exclude_tmpdir_env_var: bool,
/// When set to `true`, will NOT include the `/tmp` among the default
/// writable roots on UNIX. Defaults to `false`.
#[serde(default)]
exclude_slash_tmp: bool,
},
}
@@ -203,10 +208,6 @@ pub struct WritableRoot {
pub read_only_subpaths: Vec<PathBuf>,
}
fn default_true() -> bool {
true
}
impl FromStr for SandboxPolicy {
type Err = serde_json::Error;
@@ -228,7 +229,8 @@ impl SandboxPolicy {
SandboxPolicy::WorkspaceWrite {
writable_roots: vec![],
network_access: false,
include_default_writable_roots: true,
exclude_tmpdir_env_var: false,
exclude_slash_tmp: false,
}
}
@@ -263,27 +265,40 @@ impl SandboxPolicy {
SandboxPolicy::ReadOnly => Vec::new(),
SandboxPolicy::WorkspaceWrite {
writable_roots,
include_default_writable_roots,
..
exclude_tmpdir_env_var,
exclude_slash_tmp,
network_access: _,
} => {
// Start from explicitly configured writable roots.
let mut roots: Vec<PathBuf> = writable_roots.clone();
// Optionally include defaults (cwd and TMPDIR on macOS).
if *include_default_writable_roots {
roots.push(cwd.to_path_buf());
// Always include defaults: cwd, /tmp (if present on Unix), and
// on macOS, the per-user TMPDIR unless explicitly excluded.
roots.push(cwd.to_path_buf());
// Also include the per-user tmp dir on macOS.
// Note this is added dynamically rather than storing it in
// `writable_roots` because `writable_roots` contains only static
// values deserialized from the config file.
if cfg!(target_os = "macos") {
if let Some(tmpdir) = std::env::var_os("TMPDIR") {
roots.push(PathBuf::from(tmpdir));
}
// Include /tmp on Unix unless explicitly excluded.
if cfg!(unix) && !exclude_slash_tmp {
let slash_tmp = PathBuf::from("/tmp");
if slash_tmp.is_dir() {
roots.push(slash_tmp);
}
}
// Include $TMPDIR unless explicitly excluded. On macOS, TMPDIR
// is per-user on macOS, so writes to TMPDIR should not be
// readable by other users on the system.
//
// By comparison, TMPDIR is not guaranteed to be defined on
// Linux or Windows, but supporting it here gives users a way
// to provide the model with their own temporary directory
// without having to hardcode it in the config.
if !*exclude_tmpdir_env_var
&& let Some(tmpdir) = std::env::var_os("TMPDIR")
&& !tmpdir.is_empty()
{
roots.push(PathBuf::from(tmpdir));
}
// For each root, compute subpaths that should remain read-only.
roots
.into_iter()

View File

@@ -145,12 +145,13 @@ mod tests {
root_without_git_canon,
} = populate_tmpdir(tmp.path());
// Build a policy that only includes the two test roots as writable and
// does not automatically include defaults like cwd or TMPDIR.
// Build a policy that includes the two test roots as writable and also
// includes default writable roots (cwd, /tmp, and possibly TMPDIR).
let policy = SandboxPolicy::WorkspaceWrite {
writable_roots: vec![root_with_git.clone(), root_without_git.clone()],
network_access: false,
include_default_writable_roots: false,
exclude_tmpdir_env_var: false,
exclude_slash_tmp: false,
};
let args = create_seatbelt_command_args(
@@ -159,22 +160,50 @@ mod tests {
tmp.path(),
);
// Compute dynamic expected bits for defaults.
let cwd_canon = tmp
.path()
.canonicalize()
.expect("canonicalize tmp cwd")
.to_string_lossy()
.to_string();
let slash_tmp_canon = PathBuf::from("/tmp")
.canonicalize()
.expect("canonicalize /tmp")
.to_string_lossy()
.to_string();
let tmpdir_env_var = if cfg!(target_os = "macos") {
std::env::var("TMPDIR")
.ok()
.map(PathBuf::from)
.and_then(|p| p.canonicalize().ok())
.map(|p| p.to_string_lossy().to_string())
} else {
None
};
let tmpdir_policy_entry = if tmpdir_env_var.is_some() {
" (subpath (param \"WRITABLE_ROOT_4\"))"
} else {
""
};
// Build the expected policy text using a raw string for readability.
// Note that the policy includes:
// - the base policy,
// - read-only access to the filesystem,
// - write access to WRITABLE_ROOT_0 (but not its .git) and WRITABLE_ROOT_1.
// - write access to WRITABLE_ROOT_0 (but not its .git), WRITABLE_ROOT_1,
// WRITABLE_ROOT_2 (cwd), WRITABLE_ROOT_3 (/tmp), and optionally WRITABLE_ROOT_4 (TMPDIR).
let expected_policy = format!(
r#"{MACOS_SEATBELT_BASE_POLICY}
; 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"))) ) (subpath (param "WRITABLE_ROOT_1"))
(require-all (subpath (param "WRITABLE_ROOT_0")) (require-not (subpath (param "WRITABLE_ROOT_0_RO_0"))) ) (subpath (param "WRITABLE_ROOT_1")) (subpath (param "WRITABLE_ROOT_2")) (subpath (param "WRITABLE_ROOT_3")){tmpdir_policy_entry}
)
"#,
);
let expected_args = vec![
let mut expected_args = vec![
"-p".to_string(),
expected_policy,
format!(
@@ -189,12 +218,21 @@ mod tests {
"-DWRITABLE_ROOT_1={}",
root_without_git_canon.to_string_lossy()
),
format!("-DWRITABLE_ROOT_2={cwd_canon}"),
format!("-DWRITABLE_ROOT_3={slash_tmp_canon}"),
];
if let Some(p) = tmpdir_env_var {
expected_args.push(format!("-DWRITABLE_ROOT_4={p}"));
}
expected_args.extend(vec![
"--".to_string(),
"/bin/echo".to_string(),
"hello".to_string(),
];
]);
assert_eq!(args, expected_args);
assert_eq!(expected_args, args);
}
#[test]
@@ -215,7 +253,8 @@ mod tests {
let policy = SandboxPolicy::WorkspaceWrite {
writable_roots: vec![],
network_access: false,
include_default_writable_roots: true,
exclude_tmpdir_env_var: false,
exclude_slash_tmp: false,
};
let args = create_seatbelt_command_args(
@@ -234,7 +273,7 @@ mod tests {
None
};
let tempdir_policy_entry = if tmpdir_env_var.is_some() {
" (subpath (param \"WRITABLE_ROOT_1\"))"
" (subpath (param \"WRITABLE_ROOT_2\"))"
} else {
""
};
@@ -249,7 +288,7 @@ mod tests {
; 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"))) ){tempdir_policy_entry}
(require-all (subpath (param "WRITABLE_ROOT_0")) (require-not (subpath (param "WRITABLE_ROOT_0_RO_0"))) ) (subpath (param "WRITABLE_ROOT_1")){tempdir_policy_entry}
)
"#,
);
@@ -265,10 +304,17 @@ mod tests {
"-DWRITABLE_ROOT_0_RO_0={}",
root_with_git_git_canon.to_string_lossy()
),
format!(
"-DWRITABLE_ROOT_1={}",
PathBuf::from("/tmp")
.canonicalize()
.expect("canonicalize /tmp")
.to_string_lossy()
),
];
if let Some(p) = tmpdir_env_var {
expected_args.push(format!("-DWRITABLE_ROOT_1={p}"));
expected_args.push(format!("-DWRITABLE_ROOT_2={p}"));
}
expected_args.extend(vec![
@@ -277,7 +323,7 @@ mod tests {
"hello".to_string(),
]);
assert_eq!(args, expected_args);
assert_eq!(expected_args, args);
}
struct PopulatedTmp {

View File

@@ -76,7 +76,8 @@ async fn if_parent_of_repo_is_writable_then_dot_git_folder_is_writable() {
let policy = SandboxPolicy::WorkspaceWrite {
writable_roots: vec![test_scenario.repo_parent.clone()],
network_access: false,
include_default_writable_roots: false,
exclude_tmpdir_env_var: true,
exclude_slash_tmp: true,
};
test_scenario
@@ -101,7 +102,8 @@ async fn if_git_repo_is_writable_root_then_dot_git_folder_is_read_only() {
let policy = SandboxPolicy::WorkspaceWrite {
writable_roots: vec![test_scenario.repo_root.clone()],
network_access: false,
include_default_writable_roots: false,
exclude_tmpdir_env_var: true,
exclude_slash_tmp: true,
};
test_scenario

View File

@@ -51,7 +51,7 @@ async fn run_cmd(cmd: &[&str], writable_roots: &[PathBuf], timeout_ms: u64) {
let sandbox_policy = SandboxPolicy::WorkspaceWrite {
writable_roots: writable_roots.to_vec(),
network_access: false,
include_default_writable_roots: true,
exclude_tmpdir_env_var: false,
};
let sandbox_program = env!("CARGO_BIN_EXE_codex-linux-sandbox");
let codex_linux_sandbox_exe = Some(PathBuf::from(sandbox_program));

View File

@@ -453,6 +453,8 @@ impl ChatComposer {
new_text.push_str(&text[end_idx..]);
self.textarea.set_text(&new_text);
let new_cursor = start_idx.saturating_add(path.len()).saturating_add(1);
self.textarea.set_cursor(new_cursor);
}
/// Handle key event when no popup is visible.

View File

@@ -485,7 +485,7 @@ impl ChatWidget<'_> {
EventMsg::ExecCommandEnd(ExecCommandEndEvent {
call_id,
exit_code,
duration,
duration: _,
stdout,
stderr,
}) => {
@@ -498,7 +498,6 @@ impl ChatWidget<'_> {
exit_code,
stdout,
stderr,
duration,
},
));
}

View File

@@ -40,7 +40,6 @@ pub(crate) struct CommandOutput {
pub(crate) exit_code: i32,
pub(crate) stdout: String,
pub(crate) stderr: String,
pub(crate) duration: Duration,
}
pub(crate) enum PatchEventType {
@@ -124,7 +123,7 @@ pub(crate) enum HistoryCell {
PatchApplyResult { view: TextBlock },
}
const TOOL_CALL_MAX_LINES: usize = 5;
const TOOL_CALL_MAX_LINES: usize = 3;
impl HistoryCell {
/// Return a cloned, plain representation of the cell's lines suitable for
@@ -234,8 +233,11 @@ impl HistoryCell {
let command_escaped = strip_bash_lc_and_escape(&command);
let lines: Vec<Line<'static>> = vec![
Line::from(vec!["command".magenta(), " running...".dim()]),
Line::from(format!("$ {command_escaped}")),
Line::from(vec![
"".cyan(),
"Running command ".magenta(),
command_escaped.into(),
]),
Line::from(""),
];
@@ -249,34 +251,36 @@ impl HistoryCell {
exit_code,
stdout,
stderr,
duration,
} = output;
let mut lines: Vec<Line<'static>> = Vec::new();
// Title depends on whether we have output yet.
let title_line = Line::from(vec![
"command".magenta(),
format!(
" (code: {}, duration: {})",
exit_code,
format_duration(duration)
)
.dim(),
]);
lines.push(title_line);
let command_escaped = strip_bash_lc_and_escape(&command);
lines.push(Line::from(vec![
"⚡Ran command ".magenta(),
command_escaped.into(),
]));
let src = if exit_code == 0 { stdout } else { stderr };
let cmdline = strip_bash_lc_and_escape(&command);
lines.push(Line::from(format!("$ {cmdline}")));
let mut lines_iter = src.lines();
for raw in lines_iter.by_ref().take(TOOL_CALL_MAX_LINES) {
lines.push(ansi_escape_line(raw).dim());
for (idx, raw) in lines_iter.by_ref().take(TOOL_CALL_MAX_LINES).enumerate() {
let mut line = ansi_escape_line(raw);
let prefix = if idx == 0 { "" } else { " " };
line.spans.insert(0, prefix.into());
line.spans.iter_mut().for_each(|span| {
span.style = span.style.add_modifier(Modifier::DIM);
});
lines.push(line);
}
let remaining = lines_iter.count();
if remaining > 0 {
lines.push(Line::from(format!("... {remaining} additional lines")).dim());
let mut more = Line::from(format!("... +{remaining} lines"));
// Continuation/ellipsis is treated as a subsequent line for prefixing
more.spans.insert(0, " ".into());
more.spans.iter_mut().for_each(|span| {
span.style = span.style.add_modifier(Modifier::DIM);
});
lines.push(more);
}
lines.push(Line::from(""));