Skip to content

[codex-exec] Enforce add-dir for macOS sandbox#20658

Closed
evawong-oai wants to merge 7 commits into
mainfrom
codex/bugb15632-macos-add-dir-exec
Closed

[codex-exec] Enforce add-dir for macOS sandbox#20658
evawong-oai wants to merge 7 commits into
mainfrom
codex/bugb15632-macos-add-dir-exec

Conversation

@evawong-oai
Copy link
Copy Markdown
Contributor

@evawong-oai evawong-oai commented May 1, 2026

Why

This PR is a user migration fix for the legacy exec sandbox path during the PermissionProfile migration.

The failing path was:

codex exec
  --sandbox workspace-write
  --add-dir <extra>
  -C <workspace>

Then the model shell tool ran from the added root and tried:

printf ok > normal.txt

macOS Seatbelt returned operation not permitted.

Legacy workspace write added roots are dropped only when codex exec starts or resumes an app server thread without an active permission profile.

That no active profile state is intentional for this legacy path. The local Config has the resolved effective permissions, including the added root, but there is no named profile that can be reselected without losing legacy root details. Before this PR, codex exec sent only sandbox = workspace-write into thread/start and thread/resume. App server rebuilt workspace write permissions from that lossy shape, so the added root was dropped.

Change Details

  1. Add projectRoots request fields to thread/start and thread/resume.
  2. Store project_roots on Config as the final cwd plus the original --add-dir values resolved against that cwd.
  3. Have codex exec forward config.project_roots when starting or resuming the app server thread.
  4. Have app server translate those concrete roots into the existing additional_writable_roots override before config builds the thread permissions.
  5. Keep the legacy no active profile path on the legacy sandbox selection. This PR does not synthesize a built in permission profile for that path.

Validation

Current pushed head is 351d4995ef671c4ef1169bac7c2dc21b097c94b8.

Current head validation:

  1. just fmt
  2. cargo check -p codex-thread-manager-sample
  3. cargo test -p codex-core add_dir_override_extends_workspace_writable_roots -- --nocapture
  4. cargo test -p codex-exec thread_lifecycle_params_include_legacy_sandbox_when_no_active_profile -- --nocapture
  5. git diff --check

Earlier validation on this PR before the Config ownership cleanup:

  1. cargo test -p codex-app-server-protocol -- --nocapture
  2. cargo test -p codex-app-server --no-run

AWS macOS VM i-01c31233406f6b2c4, macOS 26.4.1 build 25E253, validated current head 351d4995ef671c4ef1169bac7c2dc21b097c94b8:

  1. Direct Seatbelt metadata tier: SUMMARY preflight_pass=6 pass=55 fail=0 skip=3 unsupported=0 invalid=0 root=/Users/ec2-user/codex-bugb15632-validation-20260503/macos-seatbelt-cases.jv6Xeg head=351d4995ef671c4ef1169bac7c2dc21b097c94b8 backend=macos-seatbelt
  2. just c exec Seatbelt E2E tier: SUMMARY preflight_pass=5 pass=8 fail=0 skip=0 invalid=0 root=/Users/ec2-user/codex-bugb15632-validation-20260503/just-c-exec-e2e-cases.MeIPIc head=351d4995ef671c4ef1169bac7c2dc21b097c94b8 backend=just-c-exec-macos-seatbelt

@evawong-oai evawong-oai force-pushed the codex/bugb15632-macos-add-dir-exec branch 4 times, most recently from 6003ea4 to a74de26 Compare May 4, 2026 15:10
@bolinfest bolinfest requested review from bolinfest and viyatb-oai May 4, 2026 17:15
@evawong-oai evawong-oai force-pushed the codex/bugb15632-macos-add-dir-exec branch from a74de26 to f227f2b Compare May 4, 2026 22:25
@evawong-oai evawong-oai changed the title [codex-exec] Preserve add-dir roots for macOS exec sandbox [codex-exec] Enforce add-dir for macOS sandbox May 4, 2026
@evawong-oai evawong-oai marked this pull request as ready for review May 4, 2026 23:34
Comment thread codex-rs/exec/src/lib.rs Outdated
.map(|profile| HashMap::from([("profile".to_string(), Value::String(profile.clone()))]))
let mut overrides = HashMap::new();
if let Some(profile) = config.active_profile.as_ref() {
overrides.insert("profile".to_string(), Value::String(profile.clone()));
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, we are going to be changing how profiles work in #17141.

Comment thread codex-rs/exec/src/lib.rs Outdated
Comment on lines +1055 to +1063
overrides.insert(
"sandbox_workspace_write".to_string(),
json!({
"writable_roots": writable_roots,
"network_access": network_access,
"exclude_tmpdir_env_var": exclude_tmpdir_env_var,
"exclude_slash_tmp": exclude_slash_tmp,
}),
);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like it would be ideal if, instead of propagating the use of the legacy sandbox_workspace_write, we injected the appropriate equivalent to something using the [permissions] shape here.

Or, perhaps more appropriately, addressed this at the ConfigBuilder level.

Note config_request_overrides_from_config() was historically created to support a --profile flag, which as I noted, we are trying to redo in a very different way. As such, I would prefer to avoid expanding the role of this function.

/// with `sandbox` or named `permissions`.
#[experimental("thread/start.permissionProfile")]
#[ts(optional = nullable)]
pub permission_profile: Option<PermissionProfile>,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I noted in a comment on #15977, I do not think we should be exposing this via the app server API: we should only expose profile names. Clients should not be able to specify arbitrary profile values.

@evawong-oai evawong-oai force-pushed the codex/bugb15632-macos-add-dir-exec branch 5 times, most recently from de137e2 to 302633f Compare May 5, 2026 05:48
@evawong-oai evawong-oai requested a review from a team as a code owner May 5, 2026 05:48
@evawong-oai evawong-oai force-pushed the codex/bugb15632-macos-add-dir-exec branch from 302633f to 13656c6 Compare May 5, 2026 05:57
@evawong-oai evawong-oai force-pushed the codex/bugb15632-macos-add-dir-exec branch from 13656c6 to d9024bc Compare May 5, 2026 15:43
@bolinfest
Copy link
Copy Markdown
Collaborator

I think #22610 made it possible to solve this in a cleaner way by supporting workspace_roots for profiles and runtime_workspace_roots for threads.

@evawong-oai
Copy link
Copy Markdown
Contributor Author

Closing this because latest main now covers the macOS validation slice this PR was intended to fix.

Validated on the AWS macOS VM against main at 249d50a. The Seatbelt harness passed with preflight_pass=5 pass=8 fail=0 skip=0 invalid=0. The key cases passed: normal write in added root, protected .git in added root, and protected metadata read but not write.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants