Skip to content

config: gate legacy writable roots from profiles#20456

Open
bolinfest wants to merge 1 commit intopr20455from
pr20456
Open

config: gate legacy writable roots from profiles#20456
bolinfest wants to merge 1 commit intopr20455from
pr20456

Conversation

@bolinfest
Copy link
Copy Markdown
Collaborator

@bolinfest bolinfest commented Apr 30, 2026

Why

The config loader still had a small profile-mode detour through SandboxPolicy: it projected the active PermissionProfile into a legacy sandbox policy only to ask whether the result looked like WorkspaceWrite before applying legacy writable_roots and memory-root additions.

That keeps legacy policy semantics in the middle of config loading even though the canonical runtime model is now PermissionProfile. This PR makes that decision from the canonical profile and its runtime filesystem policy instead.

What Changed

  • Added a small helper that says legacy additional writable roots may extend only managed profiles that already grant write access to the active workspace.
  • Replaced two temporary compatibility_sandbox_policy_for_permission_profile(...) projections in Config::load_config_with_layer_stack with that helper.
  • Kept disabled, external, read-only, and full-disk profiles from silently growing legacy writable roots.
  • Left the existing Config::legacy_sandbox_policy() compatibility projection in place for call sites that still need to speak the old API.

Verification

  • cargo check -p codex-core --tests
  • cargo test -p codex-core workspace_profile
  • just fix -p codex-core

Stack created with Sapling. Best reviewed with ReviewStack.

@bolinfest bolinfest requested a review from a team as a code owner April 30, 2026 16:32
@bolinfest bolinfest changed the base branch from main to pr20455 April 30, 2026 16:32
This was referenced Apr 30, 2026
This was referenced Apr 30, 2026
@chatgpt-codex-connector
Copy link
Copy Markdown
Contributor

💡 Codex Review

// Current app-server responses include `permissionProfile`. If it is absent,
// keep the TUI on its configured profile rather than rebuilding a lossy
// profile from the legacy `sandbox` compatibility field.
config.permissions.permission_profile()

P1 Badge Preserve remote sandbox fallback when permissionProfile is missing

When permissionProfile is absent in thread start/resume/fork responses, this now always falls back to the local config profile, but remote mode still derives future turn overrides from this stored profile. In version-skew or non-experimental responses where only legacy sandbox is populated, resuming a remote read-only thread can be silently rewritten to the local default (for example workspace-write) on the next turn. The previous remote fallback reconstructed permissions from sandbox, so dropping that path can change effective remote sandboxing and unexpectedly broaden write access.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

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.

1 participant