fix: tui default trusted settings should respect workspace write config #3341

dylan-hurd-oai · 2025-09-09T00:42:38Z

Summary

When using the trusted state during tui startup, we created a new WorkspaceWrite policy without checking the config.toml for a sandbox_workspace_write field. This would result in us setting the sandbox_mode as workspace-write, but ignoring the field if the user had set sandbox_workspace_write without also setting sandbox_mode in the config.toml. This PR adds support for respecting sandbox_workspace_write setting in config.toml in the trusted directory flow, and adds tests to cover this case.

Testing

Added unit tests

codex-rs/tui/src/lib.rs

bolinfest · 2025-09-24T22:06:33Z

This would result in us ignoring the field if the user had set sandbox_workspace_write without also setting sandbox_mode.

To be clear, the presence of sandbox_workspace_write in config.toml does not imply that sandbox_mode = "workspace-write" should be set.

What it really means is: "If sandbox_mode = "workspace-write" is set, then use this sandbox_workspace_write policy."

dylan-hurd-oai · 2025-09-24T22:07:43Z

Agree - updated the PR description to be clearer!

codex-rs/tui/src/lib.rs

bolinfest

Putting back on your queue for an answer to the codex exec scenario.

dylan-hurd-oai · 2025-09-25T20:31:04Z

@bolinfest I refactored this to take a couple changes into account:

We now resolve this logic in core/config.rs. I think we originally had a different abstraction in mind, but this approach feels simpler.
I think you're right that exec should take project-specific trust into account. If you trust the directory, we can give exec more permissions. Since exec always overrides to AskForApproval::Never, we now resolve approval_policy and sandbox_policy separately.
Also updated the tui onboarding flow so that we correctly handle the trust mutation. Rather than directly mutating the config object, we simply reload it. I think there's some risk to this approach e.g. with simultaneous sessions, but it feels mostly reasonable. Open to pushback here!
I've added some additional tests to cover these updates in behavior.

bolinfest

This is definitely a big improvement, but I'm concerned about abstraction leakage!

bolinfest · 2025-09-30T21:11:33Z

codex-rs/tui/src/lib.rs

 async fn run_ratatui_app(
    cli: Cli,
-    config: Config,
+    mut config: Config,


Since it doesn't appear that we actually need to mutate the Config, I would drop the mut here because it's misleading (see below).

codex-rs/tui/src/lib.rs

dylan-hurd-oai · 2025-10-14T18:56:43Z

codex-rs/core/src/config.rs


+    /// True if the user passed in an override or set a value in config.toml
+    /// for either of approval_policy or sandbox_mode.
+    pub did_user_set_custom_approval_policy_or_sandbox_mode: bool,


@bolinfest curious if you have a suggestion for a better approach here!

bolinfest

OK, I have a bunch of thoughts, but none of these are blocking...

bolinfest · 2025-10-16T01:20:46Z

codex-rs/core/src/config.rs

-    pub fn is_cwd_trusted(&self, resolved_cwd: &Path) -> bool {
+    /// Resolves the cwd to an existing project, or returns None if ConfigToml
+    /// does not contain a project corresponding to cwd or a git repo for cwd
+    pub fn get_active_project(&self, resolved_cwd: &Path) -> Option<ProjectConfig> {


I see that today, this is defined as:

pub projects: Option<HashMap<String, ProjectConfig>>,

This may be out of the scope of this PR, but I feel like we should consider changing this to:

#[serde(default)] pub projects: HashMap<PathBuf, ProjectConfig>,

because I don't love the to_string_lossy().to_string() stuff we're doing here.

I'm also not completely convinced that resolve_root_git_project_for_trust() is the right thing to do first. That is, I see the motivation behind the check to see if resolved_cwd is part of a Git worktree and to check its presence in self.projects.

But for an arbitrary path, I would say the more general solution would be to recursively check the parent of PathBuf for presence in projects until you have verified the filesystem root does not have an entry in projects. Only once that has failed would I do the worktree check because I think an explicit listing should take priority.

[nit] All of which starts to get a bit more complicated, so I would consider moving all of this method to in a pure function that takes &HashMap<PathBuf, ProjectConfig> and &Path as parameters.

Also, the first thing we are doing in this function right now is:

let projects = self.projects.clone().unwrap_or_default();

But if self.projects is None, then we can just return None right away and there is no need to prematurely .clone() the HashMap. We can defer a clone() of ProjectConfig until we find a matching entry in the HashMap.

bolinfest · 2025-10-16T01:26:26Z

codex-rs/core/src/config.rs


-        let sandbox_policy = cfg.derive_sandbox_policy(sandbox_mode);
+        let resolved_cwd = {
+            use std::env;


I would just inline this use.

bolinfest · 2025-10-16T01:30:04Z

codex-rs/core/src/config.rs

+                    tracing::info!("cwd not set, using current dir");
+                    env::current_dir()?
+                }
+                Some(p) if p.is_absolute() => p,


I think we may want to introduce the use of https://crates.io/crates/path-absolutize.

Note that, perhaps surprisingly, this is true:

assert!(Path::new("/foo/../bar").is_absolute());

absolute is not the same as normalized (or canonicalized)

https://crates.io/crates/path-absolutize also normalizes (but does not canonicalize, which I think is OK because sometimes people use symlinks and don't want them canonicalized, though that can be confusing at times...)

bolinfest · 2025-10-16T01:40:30Z

codex-rs/core/src/config.rs

+            .is_some()
+            || config_profile.approval_policy.is_some()
+            || cfg.approval_policy.is_some()
+            // TODO: policy.sandbox_mode is not implemented


Is this this issue #3034

It is! @jif-oai also started doing some related filling in of profile in #4948 - I think we should just close this gap. Also I can update this comment with the github issue and reference profile here

bolinfest · 2025-10-16T01:44:35Z

codex-rs/core/src/config.rs

+exclude_tmpdir_env_var = true
+exclude_slash_tmp = true
+
+[projects."/tmp/test"]


Hmm, would this test break on Windows if I had us change from String to PathBuf as described? I guess we can try it in a follow-up and see...

bolinfest · 2025-10-16T01:47:28Z

codex-rs/tui/src/lib.rs

+    } else if config.active_project.is_trusted() {
+        // if the current cwd project is already trusted, Config derives the policy.


consider...

Suggested change

} else if config.active_project.is_trusted() {

// if the current cwd project is already trusted, Config derives the policy.

} else {

!config.active_project.is_trusted()

dylan-hurd-oai requested a review from nornagon-openai September 9, 2025 00:42

dylan-hurd-oai changed the title ~~Dh tui trust sandbox policy~~ fix: tui default trusted settings should respect workspace write config Sep 9, 2025

dylan-hurd-oai marked this pull request as ready for review September 9, 2025 05:34

nornagon-openai reviewed Sep 9, 2025

View reviewed changes

codex-rs/tui/src/lib.rs Outdated Show resolved Hide resolved

codex-rs/tui/src/lib.rs Outdated Show resolved Hide resolved

dylan-hurd-oai force-pushed the dh--tui-trust-sandbox-policy branch from 8f02dd7 to 0b7dca9 Compare September 23, 2025 00:01

dylan-hurd-oai mentioned this pull request Sep 23, 2025

Python UV fails in Codex #1457

Open

dylan-hurd-oai requested a review from bolinfest September 23, 2025 17:11

dylan-hurd-oai force-pushed the dh--tui-trust-sandbox-policy branch from 0b7dca9 to c14acc3 Compare September 23, 2025 17:12

dylan-hurd-oai requested review from easong-openai and fouad-openai September 24, 2025 04:08

bolinfest reviewed Sep 24, 2025

View reviewed changes

codex-rs/tui/src/lib.rs Outdated Show resolved Hide resolved

bolinfest requested changes Sep 25, 2025

View reviewed changes

dylan-hurd-oai force-pushed the dh--tui-trust-sandbox-policy branch 3 times, most recently from 70e9c46 to eada566 Compare September 25, 2025 07:24

dylan-hurd-oai requested a review from bolinfest September 25, 2025 20:31

dylan-hurd-oai force-pushed the dh--tui-trust-sandbox-policy branch from 5a7cf01 to 82c95a0 Compare September 29, 2025 18:54

bolinfest requested changes Sep 30, 2025

View reviewed changes

dylan-hurd-oai force-pushed the dh--tui-trust-sandbox-policy branch 3 times, most recently from cced6ee to 4fa9266 Compare October 8, 2025 18:04

dylan-hurd-oai force-pushed the dh--tui-trust-sandbox-policy branch 2 times, most recently from 6f2e7b9 to d056330 Compare October 14, 2025 18:21

dylan-hurd-oai requested a review from bolinfest October 14, 2025 18:55

dylan-hurd-oai commented Oct 14, 2025

View reviewed changes

bolinfest approved these changes Oct 16, 2025

View reviewed changes

fix: tui default settings should respect sandbox_policy

c73e51d

dylan-hurd-oai added 12 commits October 15, 2025 23:12

unit tests

d127156

update tests

d231b5a

clean up comments

0cfb16f

cleanup tests

af43ca5

simplify

4783200

add config test

f05a8ee

lock rebase

5f3a2fb

cleanup project usage

78e99f7

deal with rebase fun

a45500f

just fix

139788f

fix debug_assertions

8f20ab9

rebase and review

354fdd6

dylan-hurd-oai force-pushed the dh--tui-trust-sandbox-policy branch from d056330 to 354fdd6 Compare October 16, 2025 06:20

one more rebase

def5b8e

dylan-hurd-oai merged commit 4b01f0f into main Oct 16, 2025
20 checks passed

dylan-hurd-oai deleted the dh--tui-trust-sandbox-policy branch October 16, 2025 18:23

github-actions bot locked and limited conversation to collaborators Oct 16, 2025

		} else if config.active_project.is_trusted() {
		// if the current cwd project is already trusted, Config derives the policy.

fix: tui default trusted settings should respect workspace write config #3341

fix: tui default trusted settings should respect workspace write config #3341

Uh oh!

Conversation

dylan-hurd-oai commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Uh oh!

Uh oh!

Uh oh!

bolinfest commented Sep 24, 2025

Uh oh!

dylan-hurd-oai commented Sep 24, 2025

Uh oh!

Uh oh!

bolinfest left a comment

Choose a reason for hiding this comment

Uh oh!

dylan-hurd-oai commented Sep 25, 2025

Uh oh!

bolinfest left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bolinfest left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dylan-hurd-oai commented Sep 9, 2025 •

edited

Loading