Merge 7a90ed65ad into sapling-pr-archive-bolinfest

This commit is contained in:
Michael Bolin
2026-02-02 12:51:16 -08:00
committed by GitHub
14 changed files with 532 additions and 32 deletions

1
codex-rs/Cargo.lock generated
View File

@@ -1897,6 +1897,7 @@ dependencies = [
"codex-otel",
"codex-protocol",
"dirs",
"log",
"owo-colors",
"pretty_assertions",
"serde",

View File

@@ -13,6 +13,10 @@ struct Args {
/// Optional path to the Prettier executable to format generated TypeScript files.
#[arg(short = 'p', long = "prettier", value_name = "PRETTIER_BIN")]
prettier: Option<PathBuf>,
/// Include experimental API methods and fields in generated fixtures.
#[arg(long = "experimental")]
experimental: bool,
}
fn main() -> Result<()> {
@@ -22,11 +26,17 @@ fn main() -> Result<()> {
.schema_root
.unwrap_or_else(|| PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("schema"));
codex_app_server_protocol::write_schema_fixtures(&schema_root, args.prettier.as_deref())
.with_context(|| {
format!(
"failed to regenerate schema fixtures under {}",
schema_root.display()
)
})
codex_app_server_protocol::write_schema_fixtures_with_options(
&schema_root,
args.prettier.as_deref(),
codex_app_server_protocol::SchemaFixtureOptions {
experimental_api: args.experimental,
},
)
.with_context(|| {
format!(
"failed to regenerate schema fixtures under {}",
schema_root.display()
)
})
}

View File

@@ -16,5 +16,7 @@ pub use protocol::common::*;
pub use protocol::thread_history::*;
pub use protocol::v1::*;
pub use protocol::v2::*;
pub use schema_fixtures::SchemaFixtureOptions;
pub use schema_fixtures::read_schema_fixture_tree;
pub use schema_fixtures::write_schema_fixtures;
pub use schema_fixtures::write_schema_fixtures_with_options;

View File

@@ -7,6 +7,11 @@ use std::collections::BTreeMap;
use std::path::Path;
use std::path::PathBuf;
#[derive(Clone, Copy, Debug, Default)]
pub struct SchemaFixtureOptions {
pub experimental_api: bool,
}
pub fn read_schema_fixture_tree(schema_root: &Path) -> Result<BTreeMap<PathBuf, Vec<u8>>> {
let typescript_root = schema_root.join("typescript");
let json_root = schema_root.join("json");
@@ -27,14 +32,30 @@ pub fn read_schema_fixture_tree(schema_root: &Path) -> Result<BTreeMap<PathBuf,
/// This is intended to be used by tooling (e.g., `just write-app-server-schema`).
/// It deletes any previously generated files so stale artifacts are removed.
pub fn write_schema_fixtures(schema_root: &Path, prettier: Option<&Path>) -> Result<()> {
write_schema_fixtures_with_options(schema_root, prettier, SchemaFixtureOptions::default())
}
/// Regenerates schema fixtures with configurable options.
pub fn write_schema_fixtures_with_options(
schema_root: &Path,
prettier: Option<&Path>,
options: SchemaFixtureOptions,
) -> Result<()> {
let typescript_out_dir = schema_root.join("typescript");
let json_out_dir = schema_root.join("json");
ensure_empty_dir(&typescript_out_dir)?;
ensure_empty_dir(&json_out_dir)?;
crate::generate_ts(&typescript_out_dir, prettier)?;
crate::generate_json(&json_out_dir)?;
crate::generate_ts_with_options(
&typescript_out_dir,
prettier,
crate::GenerateTsOptions {
experimental_api: options.experimental_api,
..crate::GenerateTsOptions::default()
},
)?;
crate::generate_json_with_experimental(&json_out_dir, options.experimental_api)?;
Ok(())
}

View File

@@ -788,6 +788,8 @@ At runtime, clients must send `initialize` with `capabilities.experimentalApi =
```bash
just write-app-server-schema
# Include experimental API fields/methods in fixtures.
just write-app-server-schema --experimental
```
5. Verify the protocol crate:

View File

@@ -27,12 +27,100 @@ pub fn command_might_be_dangerous(command: &[String]) -> bool {
false
}
fn is_git_global_option_with_value(arg: &str) -> bool {
matches!(
arg,
"-C" | "-c"
| "--config-env"
| "--exec-path"
| "--git-dir"
| "--namespace"
| "--super-prefix"
| "--work-tree"
)
}
fn is_git_global_option_with_inline_value(arg: &str) -> bool {
matches!(
arg,
s if s.starts_with("--config-env=")
|| s.starts_with("--exec-path=")
|| s.starts_with("--git-dir=")
|| s.starts_with("--namespace=")
|| s.starts_with("--super-prefix=")
|| s.starts_with("--work-tree=")
) || ((arg.starts_with("-C") || arg.starts_with("-c")) && arg.len() > 2)
}
/// Find the first matching git subcommand, skipping known global options that
/// may appear before it (e.g., `-C`, `-c`, `--git-dir`).
///
/// Shared with `is_safe_command` to avoid git-global-option bypasses.
pub(crate) fn find_git_subcommand<'a>(
command: &'a [String],
subcommands: &[&str],
) -> Option<(usize, &'a str)> {
let cmd0 = command.first().map(String::as_str)?;
if !cmd0.ends_with("git") {
return None;
}
let mut skip_next = false;
for (idx, arg) in command.iter().enumerate().skip(1) {
if skip_next {
skip_next = false;
continue;
}
let arg = arg.as_str();
if is_git_global_option_with_inline_value(arg) {
continue;
}
if is_git_global_option_with_value(arg) {
skip_next = true;
continue;
}
if arg == "--" || arg.starts_with('-') {
continue;
}
if subcommands.contains(&arg) {
return Some((idx, arg));
}
// In git, the first non-option token is the subcommand. If it isn't
// one of the subcommands we're looking for, we must stop scanning to
// avoid misclassifying later positional args (e.g., branch names).
return None;
}
None
}
fn is_dangerous_to_call_with_exec(command: &[String]) -> bool {
let cmd0 = command.first().map(String::as_str);
match cmd0 {
Some(cmd) if cmd.ends_with("git") || cmd.ends_with("/git") => {
matches!(command.get(1).map(String::as_str), Some("reset" | "rm"))
Some(cmd) if cmd.ends_with("git") => {
let Some((subcommand_idx, subcommand)) =
find_git_subcommand(command, &["reset", "rm", "branch", "push", "clean"])
else {
return false;
};
match subcommand {
"reset" | "rm" => true,
"branch" => git_branch_is_delete(&command[subcommand_idx + 1..]),
"push" => git_push_is_dangerous(&command[subcommand_idx + 1..]),
"clean" => git_clean_is_force(&command[subcommand_idx + 1..]),
other => {
debug_assert!(false, "unexpected git subcommand from matcher: {other}");
false
}
}
}
Some("rm") => matches!(command.get(1).map(String::as_str), Some("-f" | "-rf")),
@@ -45,6 +133,48 @@ fn is_dangerous_to_call_with_exec(command: &[String]) -> bool {
}
}
fn git_branch_is_delete(branch_args: &[String]) -> bool {
// Git allows stacking short flags (for example, `-dv` or `-vd`). Treat any
// short-flag group containing `d`/`D` as a delete flag.
branch_args.iter().map(String::as_str).any(|arg| {
matches!(arg, "-d" | "-D" | "--delete")
|| arg.starts_with("--delete=")
|| short_flag_group_contains(arg, 'd')
|| short_flag_group_contains(arg, 'D')
})
}
fn short_flag_group_contains(arg: &str, target: char) -> bool {
arg.starts_with('-') && !arg.starts_with("--") && arg.chars().skip(1).any(|c| c == target)
}
fn git_push_is_dangerous(push_args: &[String]) -> bool {
push_args.iter().map(String::as_str).any(|arg| {
matches!(
arg,
"--force" | "--force-with-lease" | "--force-if-includes" | "--delete" | "-f" | "-d"
) || arg.starts_with("--force-with-lease=")
|| arg.starts_with("--force-if-includes=")
|| arg.starts_with("--delete=")
|| short_flag_group_contains(arg, 'f')
|| short_flag_group_contains(arg, 'd')
|| git_push_refspec_is_dangerous(arg)
})
}
fn git_push_refspec_is_dangerous(arg: &str) -> bool {
// `+<refspec>` forces updates and `:<dst>` deletes remote refs.
(arg.starts_with('+') || arg.starts_with(':')) && arg.len() > 1
}
fn git_clean_is_force(clean_args: &[String]) -> bool {
clean_args.iter().map(String::as_str).any(|arg| {
matches!(arg, "--force" | "-f")
|| arg.starts_with("--force=")
|| short_flag_group_contains(arg, 'f')
})
}
#[cfg(test)]
mod tests {
use super::*;
@@ -63,7 +193,7 @@ mod tests {
assert!(command_might_be_dangerous(&vec_str(&[
"bash",
"-lc",
"git reset --hard"
"git reset --hard",
])));
}
@@ -72,7 +202,7 @@ mod tests {
assert!(command_might_be_dangerous(&vec_str(&[
"zsh",
"-lc",
"git reset --hard"
"git reset --hard",
])));
}
@@ -86,14 +216,14 @@ mod tests {
assert!(!command_might_be_dangerous(&vec_str(&[
"bash",
"-lc",
"git status"
"git status",
])));
}
#[test]
fn sudo_git_reset_is_dangerous() {
assert!(command_might_be_dangerous(&vec_str(&[
"sudo", "git", "reset", "--hard"
"sudo", "git", "reset", "--hard",
])));
}
@@ -102,7 +232,141 @@ mod tests {
assert!(command_might_be_dangerous(&vec_str(&[
"/usr/bin/git",
"reset",
"--hard"
"--hard",
])));
}
#[test]
fn git_branch_delete_is_dangerous() {
assert!(command_might_be_dangerous(&vec_str(&[
"git", "branch", "-d", "feature",
])));
assert!(command_might_be_dangerous(&vec_str(&[
"git", "branch", "-D", "feature",
])));
assert!(command_might_be_dangerous(&vec_str(&[
"bash",
"-lc",
"git branch --delete feature",
])));
}
#[test]
fn git_branch_delete_with_stacked_short_flags_is_dangerous() {
assert!(command_might_be_dangerous(&vec_str(&[
"git", "branch", "-dv", "feature",
])));
assert!(command_might_be_dangerous(&vec_str(&[
"git", "branch", "-vd", "feature",
])));
assert!(command_might_be_dangerous(&vec_str(&[
"git", "branch", "-vD", "feature",
])));
assert!(command_might_be_dangerous(&vec_str(&[
"git", "branch", "-Dvv", "feature",
])));
}
#[test]
fn git_branch_delete_with_global_options_is_dangerous() {
assert!(command_might_be_dangerous(&vec_str(&[
"git", "-C", ".", "branch", "-d", "feature",
])));
assert!(command_might_be_dangerous(&vec_str(&[
"git",
"-c",
"color.ui=false",
"branch",
"-D",
"feature",
])));
assert!(command_might_be_dangerous(&vec_str(&[
"bash",
"-lc",
"git -C . branch -d feature",
])));
}
#[test]
fn git_checkout_reset_is_not_dangerous() {
// The first non-option token is "checkout", so later positional args
// like branch names must not be treated as subcommands.
assert!(!command_might_be_dangerous(&vec_str(&[
"git", "checkout", "reset",
])));
}
#[test]
fn git_push_force_is_dangerous() {
assert!(command_might_be_dangerous(&vec_str(&[
"git", "push", "--force", "origin", "main",
])));
assert!(command_might_be_dangerous(&vec_str(&[
"git", "push", "-f", "origin", "main",
])));
assert!(command_might_be_dangerous(&vec_str(&[
"git",
"-C",
".",
"push",
"--force-with-lease",
"origin",
"main",
])));
}
#[test]
fn git_push_plus_refspec_is_dangerous() {
assert!(command_might_be_dangerous(&vec_str(&[
"git", "push", "origin", "+main",
])));
assert!(command_might_be_dangerous(&vec_str(&[
"git",
"push",
"origin",
"+refs/heads/main:refs/heads/main",
])));
}
#[test]
fn git_push_delete_flag_is_dangerous() {
assert!(command_might_be_dangerous(&vec_str(&[
"git", "push", "--delete", "origin", "feature",
])));
assert!(command_might_be_dangerous(&vec_str(&[
"git", "push", "-d", "origin", "feature",
])));
}
#[test]
fn git_push_delete_refspec_is_dangerous() {
assert!(command_might_be_dangerous(&vec_str(&[
"git", "push", "origin", ":feature",
])));
assert!(command_might_be_dangerous(&vec_str(&[
"bash",
"-lc",
"git push origin :feature",
])));
}
#[test]
fn git_push_without_force_is_not_dangerous() {
assert!(!command_might_be_dangerous(&vec_str(&[
"git", "push", "origin", "main",
])));
}
#[test]
fn git_clean_force_is_dangerous_even_when_f_is_not_first_flag() {
assert!(command_might_be_dangerous(&vec_str(&[
"git", "clean", "-fdx",
])));
assert!(command_might_be_dangerous(&vec_str(&[
"git", "clean", "-xdf",
])));
assert!(command_might_be_dangerous(&vec_str(&[
"git", "clean", "--force",
])));
}

View File

@@ -1,4 +1,8 @@
use crate::bash::parse_shell_lc_plain_commands;
// Find the first matching git subcommand, skipping known global options that
// may appear before it (e.g., `-C`, `-c`, `--git-dir`).
// Implemented in `is_dangerous_command` and shared here.
use crate::command_safety::is_dangerous_command::find_git_subcommand;
use crate::command_safety::windows_safe_commands::is_safe_command_windows;
pub fn is_known_safe_command(command: &[String]) -> bool {
@@ -131,13 +135,36 @@ fn is_safe_to_call_with_exec(command: &[String]) -> bool {
}
// Git
Some("git") => matches!(
command.get(1).map(String::as_str),
Some("branch" | "status" | "log" | "diff" | "show")
),
Some("git") => {
// Global config overrides like `-c core.pager=...` can force git
// to execute arbitrary external commands. With no sandboxing, we
// should always prompt in those cases.
if git_has_config_override_global_option(command) {
return false;
}
// Rust
Some("cargo") if command.get(1).map(String::as_str) == Some("check") => true,
let Some((subcommand_idx, subcommand)) =
find_git_subcommand(command, &["status", "log", "diff", "show", "branch"])
else {
return false;
};
let subcommand_args = &command[subcommand_idx + 1..];
match subcommand {
"status" | "log" | "diff" | "show" => {
git_subcommand_args_are_read_only(subcommand_args)
}
"branch" => {
git_subcommand_args_are_read_only(subcommand_args)
&& git_branch_is_read_only(subcommand_args)
}
other => {
debug_assert!(false, "unexpected git subcommand from matcher: {other}");
false
}
}
}
// Special-case `sed -n {N|M,N}p`
Some("sed")
@@ -155,6 +182,60 @@ fn is_safe_to_call_with_exec(command: &[String]) -> bool {
}
}
// Treat `git branch` as safe only when the arguments clearly indicate
// a read-only query, not a branch mutation (create/rename/delete).
fn git_branch_is_read_only(branch_args: &[String]) -> bool {
if branch_args.is_empty() {
// `git branch` with no additional args lists branches.
return true;
}
let mut saw_read_only_flag = false;
for arg in branch_args.iter().map(String::as_str) {
match arg {
"--list" | "-l" | "--show-current" | "-a" | "--all" | "-r" | "--remotes" | "-v"
| "-vv" | "--verbose" => {
saw_read_only_flag = true;
}
_ if arg.starts_with("--format=") => {
saw_read_only_flag = true;
}
_ => {
// Any other flag or positional argument may create, rename, or delete branches.
return false;
}
}
}
saw_read_only_flag
}
fn git_has_config_override_global_option(command: &[String]) -> bool {
command.iter().map(String::as_str).any(|arg| {
matches!(arg, "-c" | "--config-env")
|| (arg.starts_with("-c") && arg.len() > 2)
|| arg.starts_with("--config-env=")
})
}
fn git_subcommand_args_are_read_only(args: &[String]) -> bool {
// Flags that can write to disk or execute external tools should never be
// auto-approved on an unsandboxed machine.
const UNSAFE_GIT_FLAGS: &[&str] = &[
"--output",
"--ext-diff",
"--textconv",
"--exec",
"--paginate",
];
!args.iter().map(String::as_str).any(|arg| {
UNSAFE_GIT_FLAGS.contains(&arg)
|| arg.starts_with("--output=")
|| arg.starts_with("--exec=")
})
}
// (bash parsing helpers implemented in crate::bash)
/* ----------------------------------------------------------
@@ -207,6 +288,12 @@ mod tests {
fn known_safe_examples() {
assert!(is_safe_to_call_with_exec(&vec_str(&["ls"])));
assert!(is_safe_to_call_with_exec(&vec_str(&["git", "status"])));
assert!(is_safe_to_call_with_exec(&vec_str(&["git", "branch"])));
assert!(is_safe_to_call_with_exec(&vec_str(&[
"git",
"branch",
"--show-current"
])));
assert!(is_safe_to_call_with_exec(&vec_str(&["base64"])));
assert!(is_safe_to_call_with_exec(&vec_str(&[
"sed", "-n", "1,5p", "file.txt"
@@ -231,6 +318,86 @@ mod tests {
}
}
#[test]
fn git_branch_mutating_flags_are_not_safe() {
assert!(!is_known_safe_command(&vec_str(&[
"git", "branch", "-d", "feature"
])));
assert!(!is_known_safe_command(&vec_str(&[
"git",
"branch",
"new-branch"
])));
}
#[test]
fn git_branch_global_options_respect_safety_rules() {
use pretty_assertions::assert_eq;
assert_eq!(
is_known_safe_command(&vec_str(&["git", "-C", ".", "branch", "--show-current"])),
true
);
assert_eq!(
is_known_safe_command(&vec_str(&["git", "-C", ".", "branch", "-d", "feature"])),
false
);
assert_eq!(
is_known_safe_command(&vec_str(&["bash", "-lc", "git -C . branch -d feature",])),
false
);
}
#[test]
fn git_first_positional_is_the_subcommand() {
// In git, the first non-option token is the subcommand. Later positional
// args (like branch names) must not be treated as subcommands.
assert!(!is_known_safe_command(&vec_str(&[
"git", "checkout", "status",
])));
}
#[test]
fn git_output_and_config_override_flags_are_not_safe() {
assert!(!is_known_safe_command(&vec_str(&[
"git",
"log",
"--output=/tmp/git-log-out-test",
"-n",
"1",
])));
assert!(!is_known_safe_command(&vec_str(&[
"git",
"diff",
"--output",
"/tmp/git-diff-out-test",
])));
assert!(!is_known_safe_command(&vec_str(&[
"git",
"show",
"--output=/tmp/git-show-out-test",
"HEAD",
])));
assert!(!is_known_safe_command(&vec_str(&[
"git",
"-c",
"core.pager=cat",
"log",
"-n",
"1",
])));
assert!(!is_known_safe_command(&vec_str(&[
"git",
"-ccore.pager=cat",
"status",
])));
}
#[test]
fn cargo_check_is_not_safe() {
assert!(!is_known_safe_command(&vec_str(&["cargo", "check"])));
}
#[test]
fn zsh_lc_safe_command_sequence() {
assert!(is_known_safe_command(&vec_str(&["zsh", "-lc", "ls"])));

View File

@@ -1280,6 +1280,30 @@ prefix_rule(
);
}
#[tokio::test]
async fn dangerous_git_push_requires_approval_in_danger_full_access() {
let command = vec_str(&["git", "push", "origin", "+main"]);
let manager = ExecPolicyManager::default();
let requirement = manager
.create_exec_approval_requirement_for_command(ExecApprovalRequest {
features: &Features::with_defaults(),
command: &command,
approval_policy: AskForApproval::OnRequest,
sandbox_policy: &SandboxPolicy::DangerFullAccess,
sandbox_permissions: SandboxPermissions::UseDefault,
prefix_rule: None,
})
.await;
assert_eq!(
requirement,
ExecApprovalRequirement::NeedsApproval {
reason: None,
proposed_execpolicy_amendment: Some(ExecPolicyAmendment::new(command)),
}
);
}
fn vec_str(items: &[&str]) -> Vec<String> {
items.iter().map(std::string::ToString::to_string).collect()
}

View File

@@ -11,6 +11,7 @@ clap = { workspace = true, features = ["derive", "env"] }
codex-otel = { workspace = true }
codex-protocol = { workspace = true }
dirs = { workspace = true }
log = { workspace = true }
owo-colors = { workspace = true }
serde = { workspace = true, features = ["derive"] }
serde_json = { workspace = true }

View File

@@ -17,6 +17,8 @@ use chrono::Utc;
use codex_otel::OtelManager;
use codex_protocol::ThreadId;
use codex_protocol::protocol::RolloutItem;
use log::LevelFilter;
use sqlx::ConnectOptions;
use sqlx::QueryBuilder;
use sqlx::Row;
use sqlx::Sqlite;
@@ -511,7 +513,8 @@ async fn open_sqlite(path: &Path) -> anyhow::Result<SqlitePool> {
.create_if_missing(true)
.journal_mode(SqliteJournalMode::Wal)
.synchronous(SqliteSynchronous::Normal)
.busy_timeout(Duration::from_secs(5));
.busy_timeout(Duration::from_secs(5))
.log_statements(LevelFilter::Off);
let pool = SqlitePoolOptions::new()
.max_connections(5)
.connect_with(options)

View File

@@ -175,11 +175,16 @@ impl BottomPaneView for ExperimentalFeaturesView {
..
} => self.move_down(),
KeyEvent {
code: KeyCode::Enter,
code: KeyCode::Char(' '),
modifiers: KeyModifiers::NONE,
..
} => self.toggle_selected(),
KeyEvent {
code: KeyCode::Enter,
modifiers: KeyModifiers::NONE,
..
}
| KeyEvent {
code: KeyCode::Esc, ..
} => {
self.on_ctrl_c();
@@ -287,9 +292,9 @@ impl Renderable for ExperimentalFeaturesView {
fn experimental_popup_hint_line() -> Line<'static> {
Line::from(vec![
"Press ".into(),
key_hint::plain(KeyCode::Char(' ')).into(),
" to select or ".into(),
key_hint::plain(KeyCode::Enter).into(),
" to toggle or ".into(),
key_hint::plain(KeyCode::Esc).into(),
" to save for next conversation".into(),
])
}

View File

@@ -8,4 +8,4 @@ expression: popup
[ ] Ghost snapshots Capture undo snapshots each turn.
[x] Shell tool Allow the model to run shell commands.
Press enter to toggle or esc to save for next conversation
Press space to select or enter to save for next conversation

View File

@@ -3036,14 +3036,14 @@ async fn experimental_features_toggle_saves_on_exit() {
);
chat.bottom_pane.show_view(Box::new(view));
chat.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE));
chat.handle_key_event(KeyEvent::new(KeyCode::Char(' '), KeyModifiers::NONE));
assert!(
rx.try_recv().is_err(),
"expected no updates until exiting the popup"
"expected no updates until saving the popup"
);
chat.handle_key_event(KeyEvent::new(KeyCode::Esc, KeyModifiers::NONE));
chat.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE));
let mut updates = None;
while let Ok(event) = rx.try_recv() {

View File

@@ -69,8 +69,8 @@ write-config-schema:
cargo run -p codex-core --bin codex-write-config-schema
# Regenerate vendored app-server protocol schema artifacts.
write-app-server-schema:
cargo run -p codex-app-server-protocol --bin write_schema_fixtures
write-app-server-schema *args:
cargo run -p codex-app-server-protocol --bin write_schema_fixtures -- "$@"
# Tail logs from the state SQLite database
log *args: