Handle "Don't Trust" directory selection in onboarding (#4941)
Fixes #4940 Fixes #4892 When selecting "No, ask me to approve edits and commands" during onboarding, the code wasn't applying the correct approval policy, causing Codex to block all write operations instead of requesting approval. This PR fixes the issue by persisting the "DontTrust" decision in config.toml as `trust_level = "untrusted"` and handling it in the sandbox and approval policy logic, so Codex correctly asks for approval before making changes. ## Before (bug) <img width="709" height="500" alt="bef" src="https://github.com/user-attachments/assets/5aced26d-d810-4754-879a-89d9e4e0073b" /> ## After (fixed) <img width="713" height="359" alt="aft" src="https://github.com/user-attachments/assets/9887bbcb-a9a5-4e54-8e76-9125a782226b" /> --------- Co-authored-by: Eric Traut <etraut@openai.com>
This commit is contained in:
parent
018a2d2e50
commit
89ecc00b79
5 changed files with 173 additions and 35 deletions
|
|
@ -3,6 +3,7 @@ use crate::config::types::McpServerConfig;
|
|||
use crate::config::types::Notice;
|
||||
use anyhow::Context;
|
||||
use codex_protocol::config_types::ReasoningEffort;
|
||||
use codex_protocol::config_types::TrustLevel;
|
||||
use codex_utils_tokenizer::warm_model_cache;
|
||||
use std::collections::BTreeMap;
|
||||
use std::path::Path;
|
||||
|
|
@ -34,9 +35,9 @@ pub enum ConfigEdit {
|
|||
SetNoticeHideModelMigrationPrompt(String, bool),
|
||||
/// Replace the entire `[mcp_servers]` table.
|
||||
ReplaceMcpServers(BTreeMap<String, McpServerConfig>),
|
||||
/// Set trust_level = "trusted" under `[projects."<path>"]`,
|
||||
/// Set trust_level under `[projects."<path>"]`,
|
||||
/// migrating inline tables to explicit tables.
|
||||
SetProjectTrusted(PathBuf),
|
||||
SetProjectTrustLevel { path: PathBuf, level: TrustLevel },
|
||||
/// Set the value stored at the exact dotted path.
|
||||
SetPath {
|
||||
segments: Vec<String>,
|
||||
|
|
@ -274,10 +275,14 @@ impl ConfigDocument {
|
|||
ConfigEdit::ReplaceMcpServers(servers) => Ok(self.replace_mcp_servers(servers)),
|
||||
ConfigEdit::SetPath { segments, value } => Ok(self.insert(segments, value.clone())),
|
||||
ConfigEdit::ClearPath { segments } => Ok(self.clear_owned(segments)),
|
||||
ConfigEdit::SetProjectTrusted(project_path) => {
|
||||
ConfigEdit::SetProjectTrustLevel { path, level } => {
|
||||
// Delegate to the existing, tested logic in config.rs to
|
||||
// ensure tables are explicit and migration is preserved.
|
||||
crate::config::set_project_trusted_inner(&mut self.doc, project_path.as_path())?;
|
||||
crate::config::set_project_trust_level_inner(
|
||||
&mut self.doc,
|
||||
path.as_path(),
|
||||
*level,
|
||||
)?;
|
||||
Ok(true)
|
||||
}
|
||||
}
|
||||
|
|
@ -533,9 +538,15 @@ impl ConfigEditsBuilder {
|
|||
self
|
||||
}
|
||||
|
||||
pub fn set_project_trusted<P: Into<PathBuf>>(mut self, project_path: P) -> Self {
|
||||
self.edits
|
||||
.push(ConfigEdit::SetProjectTrusted(project_path.into()));
|
||||
pub fn set_project_trust_level<P: Into<PathBuf>>(
|
||||
mut self,
|
||||
project_path: P,
|
||||
trust_level: TrustLevel,
|
||||
) -> Self {
|
||||
self.edits.push(ConfigEdit::SetProjectTrustLevel {
|
||||
path: project_path.into(),
|
||||
level: trust_level,
|
||||
});
|
||||
self
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -38,6 +38,7 @@ use codex_protocol::config_types::ForcedLoginMethod;
|
|||
use codex_protocol::config_types::ReasoningEffort;
|
||||
use codex_protocol::config_types::ReasoningSummary;
|
||||
use codex_protocol::config_types::SandboxMode;
|
||||
use codex_protocol::config_types::TrustLevel;
|
||||
use codex_protocol::config_types::Verbosity;
|
||||
use codex_rmcp_client::OAuthCredentialsStoreMode;
|
||||
use dirs::home_dir;
|
||||
|
|
@ -382,15 +383,16 @@ fn ensure_no_inline_bearer_tokens(value: &TomlValue) -> std::io::Result<()> {
|
|||
Ok(())
|
||||
}
|
||||
|
||||
pub(crate) fn set_project_trusted_inner(
|
||||
pub(crate) fn set_project_trust_level_inner(
|
||||
doc: &mut DocumentMut,
|
||||
project_path: &Path,
|
||||
trust_level: TrustLevel,
|
||||
) -> anyhow::Result<()> {
|
||||
// Ensure we render a human-friendly structure:
|
||||
//
|
||||
// [projects]
|
||||
// [projects."/path/to/project"]
|
||||
// trust_level = "trusted"
|
||||
// trust_level = "trusted" or "untrusted"
|
||||
//
|
||||
// rather than inline tables like:
|
||||
//
|
||||
|
|
@ -446,17 +448,21 @@ pub(crate) fn set_project_trusted_inner(
|
|||
return Err(anyhow::anyhow!("project table missing for {project_key}"));
|
||||
};
|
||||
proj_tbl.set_implicit(false);
|
||||
proj_tbl["trust_level"] = toml_edit::value("trusted");
|
||||
proj_tbl["trust_level"] = toml_edit::value(trust_level.to_string());
|
||||
Ok(())
|
||||
}
|
||||
|
||||
/// Patch `CODEX_HOME/config.toml` project state.
|
||||
/// Patch `CODEX_HOME/config.toml` project state to set trust level.
|
||||
/// Use with caution.
|
||||
pub fn set_project_trusted(codex_home: &Path, project_path: &Path) -> anyhow::Result<()> {
|
||||
pub fn set_project_trust_level(
|
||||
codex_home: &Path,
|
||||
project_path: &Path,
|
||||
trust_level: TrustLevel,
|
||||
) -> anyhow::Result<()> {
|
||||
use crate::config::edit::ConfigEditsBuilder;
|
||||
|
||||
ConfigEditsBuilder::new(codex_home)
|
||||
.set_project_trusted(project_path)
|
||||
.set_project_trust_level(project_path, trust_level)
|
||||
.apply_blocking()
|
||||
}
|
||||
|
||||
|
|
@ -686,15 +692,16 @@ impl From<ConfigToml> for UserSavedConfig {
|
|||
|
||||
#[derive(Deserialize, Debug, Clone, PartialEq, Eq)]
|
||||
pub struct ProjectConfig {
|
||||
pub trust_level: Option<String>,
|
||||
pub trust_level: Option<TrustLevel>,
|
||||
}
|
||||
|
||||
impl ProjectConfig {
|
||||
pub fn is_trusted(&self) -> bool {
|
||||
match &self.trust_level {
|
||||
Some(trust_level) => trust_level == "trusted",
|
||||
None => false,
|
||||
}
|
||||
matches!(self.trust_level, Some(TrustLevel::Trusted))
|
||||
}
|
||||
|
||||
pub fn is_untrusted(&self) -> bool {
|
||||
matches!(self.trust_level, Some(TrustLevel::Untrusted))
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -735,9 +742,9 @@ impl ConfigToml {
|
|||
.or(profile_sandbox_mode)
|
||||
.or(self.sandbox_mode)
|
||||
.or_else(|| {
|
||||
// if no sandbox_mode is set, but user has marked directory as trusted, use WorkspaceWrite
|
||||
// if no sandbox_mode is set, but user has marked directory as trusted or untrusted, use WorkspaceWrite
|
||||
self.get_active_project(resolved_cwd).and_then(|p| {
|
||||
if p.is_trusted() {
|
||||
if p.is_trusted() || p.is_untrusted() {
|
||||
Some(SandboxMode::WorkspaceWrite)
|
||||
} else {
|
||||
None
|
||||
|
|
@ -958,6 +965,9 @@ impl Config {
|
|||
if active_project.is_trusted() {
|
||||
// If no explicit approval policy is set, but we trust cwd, default to OnRequest
|
||||
AskForApproval::OnRequest
|
||||
} else if active_project.is_untrusted() {
|
||||
// If project is explicitly marked untrusted, require approval for non-safe commands
|
||||
AskForApproval::UnlessTrusted
|
||||
} else {
|
||||
AskForApproval::default()
|
||||
}
|
||||
|
|
@ -3164,7 +3174,7 @@ model_verbosity = "high"
|
|||
let project_dir = Path::new("/some/path");
|
||||
let mut doc = DocumentMut::new();
|
||||
|
||||
set_project_trusted_inner(&mut doc, project_dir)?;
|
||||
set_project_trust_level_inner(&mut doc, project_dir, TrustLevel::Trusted)?;
|
||||
|
||||
let contents = doc.to_string();
|
||||
|
||||
|
|
@ -3204,7 +3214,7 @@ trust_level = "trusted"
|
|||
let mut doc = initial.parse::<DocumentMut>()?;
|
||||
|
||||
// Run the function; it should convert to explicit tables and set trusted
|
||||
set_project_trusted_inner(&mut doc, project_dir)?;
|
||||
set_project_trust_level_inner(&mut doc, project_dir, TrustLevel::Trusted)?;
|
||||
|
||||
let contents = doc.to_string();
|
||||
|
||||
|
|
@ -3231,7 +3241,7 @@ model = "foo""#;
|
|||
|
||||
// Approve a new directory
|
||||
let new_project = Path::new("/Users/mbolin/code/codex2");
|
||||
set_project_trusted_inner(&mut doc, new_project)?;
|
||||
set_project_trust_level_inner(&mut doc, new_project, TrustLevel::Trusted)?;
|
||||
|
||||
let contents = doc.to_string();
|
||||
|
||||
|
|
@ -3254,6 +3264,87 @@ trust_level = "trusted"
|
|||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_untrusted_project_gets_workspace_write_sandbox() -> anyhow::Result<()> {
|
||||
let config_with_untrusted = r#"
|
||||
[projects."/tmp/test"]
|
||||
trust_level = "untrusted"
|
||||
"#;
|
||||
|
||||
let cfg = toml::from_str::<ConfigToml>(config_with_untrusted)
|
||||
.expect("TOML deserialization should succeed");
|
||||
|
||||
let resolution = cfg.derive_sandbox_policy(None, None, &PathBuf::from("/tmp/test"));
|
||||
|
||||
// Verify that untrusted projects get WorkspaceWrite (or ReadOnly on Windows due to downgrade)
|
||||
if cfg!(target_os = "windows") {
|
||||
assert!(
|
||||
matches!(resolution.policy, SandboxPolicy::ReadOnly),
|
||||
"Expected ReadOnly on Windows, got {:?}",
|
||||
resolution.policy
|
||||
);
|
||||
} else {
|
||||
assert!(
|
||||
matches!(resolution.policy, SandboxPolicy::WorkspaceWrite { .. }),
|
||||
"Expected WorkspaceWrite for untrusted project, got {:?}",
|
||||
resolution.policy
|
||||
);
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_untrusted_project_gets_unless_trusted_approval_policy() -> std::io::Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
let test_project_dir = TempDir::new()?;
|
||||
let test_path = test_project_dir.path();
|
||||
|
||||
let mut projects = std::collections::HashMap::new();
|
||||
projects.insert(
|
||||
test_path.to_string_lossy().to_string(),
|
||||
ProjectConfig {
|
||||
trust_level: Some(TrustLevel::Untrusted),
|
||||
},
|
||||
);
|
||||
|
||||
let cfg = ConfigToml {
|
||||
projects: Some(projects),
|
||||
..Default::default()
|
||||
};
|
||||
|
||||
let config = Config::load_from_base_config_with_overrides(
|
||||
cfg,
|
||||
ConfigOverrides {
|
||||
cwd: Some(test_path.to_path_buf()),
|
||||
..Default::default()
|
||||
},
|
||||
codex_home.path().to_path_buf(),
|
||||
)?;
|
||||
|
||||
// Verify that untrusted projects get UnlessTrusted approval policy
|
||||
assert_eq!(
|
||||
config.approval_policy,
|
||||
AskForApproval::UnlessTrusted,
|
||||
"Expected UnlessTrusted approval policy for untrusted project"
|
||||
);
|
||||
|
||||
// Verify that untrusted projects still get WorkspaceWrite sandbox (or ReadOnly on Windows)
|
||||
if cfg!(target_os = "windows") {
|
||||
assert!(
|
||||
matches!(config.sandbox_policy, SandboxPolicy::ReadOnly),
|
||||
"Expected ReadOnly on Windows"
|
||||
);
|
||||
} else {
|
||||
assert!(
|
||||
matches!(config.sandbox_policy, SandboxPolicy::WorkspaceWrite { .. }),
|
||||
"Expected WorkspaceWrite sandbox for untrusted project"
|
||||
);
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
|
|
|
|||
|
|
@ -98,3 +98,13 @@ pub enum ForcedLoginMethod {
|
|||
Chatgpt,
|
||||
Api,
|
||||
}
|
||||
|
||||
/// Represents the trust level for a project directory.
|
||||
/// This determines the approval policy and sandbox mode applied.
|
||||
#[derive(Debug, Serialize, Deserialize, Clone, Copy, PartialEq, Eq, Display, JsonSchema, TS)]
|
||||
#[serde(rename_all = "lowercase")]
|
||||
#[strum(serialize_all = "lowercase")]
|
||||
pub enum TrustLevel {
|
||||
Trusted,
|
||||
Untrusted,
|
||||
}
|
||||
|
|
|
|||
|
|
@ -82,7 +82,6 @@ mod wrapping;
|
|||
#[cfg(test)]
|
||||
pub mod test_backend;
|
||||
|
||||
use crate::onboarding::TrustDirectorySelection;
|
||||
use crate::onboarding::WSL_INSTRUCTIONS;
|
||||
use crate::onboarding::onboarding_screen::OnboardingScreenArgs;
|
||||
use crate::onboarding::onboarding_screen::run_onboarding_app;
|
||||
|
|
@ -378,13 +377,8 @@ async fn run_ratatui_app(
|
|||
update_action: None,
|
||||
});
|
||||
}
|
||||
// if the user acknowledged windows or made an explicit decision ato trust the directory, reload the config accordingly
|
||||
if should_show_windows_wsl_screen
|
||||
|| onboarding_result
|
||||
.directory_trust_decision
|
||||
.map(|d| d == TrustDirectorySelection::Trust)
|
||||
.unwrap_or(false)
|
||||
{
|
||||
// if the user acknowledged windows or made any trust decision, reload the config accordingly
|
||||
if should_show_windows_wsl_screen || onboarding_result.directory_trust_decision.is_some() {
|
||||
load_config_or_exit(cli_kv_overrides, overrides).await
|
||||
} else {
|
||||
initial_config
|
||||
|
|
@ -540,8 +534,8 @@ fn should_show_trust_screen(config: &Config) -> bool {
|
|||
// Respect explicit approval/sandbox overrides made by the user.
|
||||
return false;
|
||||
}
|
||||
// otherwise, skip iff the active project is trusted
|
||||
!config.active_project.is_trusted()
|
||||
// otherwise, show only if no trust decision has been made
|
||||
config.active_project.trust_level.is_none()
|
||||
}
|
||||
|
||||
fn should_show_onboarding(
|
||||
|
|
@ -635,4 +629,25 @@ mod tests {
|
|||
}
|
||||
Ok(())
|
||||
}
|
||||
#[test]
|
||||
fn untrusted_project_skips_trust_prompt() -> std::io::Result<()> {
|
||||
use codex_protocol::config_types::TrustLevel;
|
||||
let temp_dir = TempDir::new()?;
|
||||
let mut config = Config::load_from_base_config_with_overrides(
|
||||
ConfigToml::default(),
|
||||
ConfigOverrides::default(),
|
||||
temp_dir.path().to_path_buf(),
|
||||
)?;
|
||||
config.did_user_set_custom_approval_policy_or_sandbox_mode = false;
|
||||
config.active_project = ProjectConfig {
|
||||
trust_level: Some(TrustLevel::Untrusted),
|
||||
};
|
||||
|
||||
let should_show = should_show_trust_screen(&config);
|
||||
assert!(
|
||||
!should_show,
|
||||
"Trust prompt should not be shown for projects explicitly marked as untrusted"
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,7 +1,8 @@
|
|||
use std::path::PathBuf;
|
||||
|
||||
use codex_core::config::set_project_trusted;
|
||||
use codex_core::config::set_project_trust_level;
|
||||
use codex_core::git_info::resolve_root_git_project_for_trust;
|
||||
use codex_protocol::config_types::TrustLevel;
|
||||
use crossterm::event::KeyCode;
|
||||
use crossterm::event::KeyEvent;
|
||||
use crossterm::event::KeyEventKind;
|
||||
|
|
@ -153,7 +154,7 @@ impl TrustDirectoryWidget {
|
|||
fn handle_trust(&mut self) {
|
||||
let target =
|
||||
resolve_root_git_project_for_trust(&self.cwd).unwrap_or_else(|| self.cwd.clone());
|
||||
if let Err(e) = set_project_trusted(&self.codex_home, &target) {
|
||||
if let Err(e) = set_project_trust_level(&self.codex_home, &target, TrustLevel::Trusted) {
|
||||
tracing::error!("Failed to set project trusted: {e:?}");
|
||||
self.error = Some(format!("Failed to set trust for {}: {e}", target.display()));
|
||||
}
|
||||
|
|
@ -163,6 +164,16 @@ impl TrustDirectoryWidget {
|
|||
|
||||
fn handle_dont_trust(&mut self) {
|
||||
self.highlighted = TrustDirectorySelection::DontTrust;
|
||||
let target =
|
||||
resolve_root_git_project_for_trust(&self.cwd).unwrap_or_else(|| self.cwd.clone());
|
||||
if let Err(e) = set_project_trust_level(&self.codex_home, &target, TrustLevel::Untrusted) {
|
||||
tracing::error!("Failed to set project untrusted: {e:?}");
|
||||
self.error = Some(format!(
|
||||
"Failed to set untrusted for {}: {e}",
|
||||
target.display()
|
||||
));
|
||||
}
|
||||
|
||||
self.selection = Some(TrustDirectorySelection::DontTrust);
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue