Skip to content

session: convert legacy sandbox updates before settings apply - #20426

Closed
bolinfest wants to merge 1 commit into
pr20424from
pr20426
Closed

session: convert legacy sandbox updates before settings apply#20426
bolinfest wants to merge 1 commit into
pr20424from
pr20426

Conversation

@bolinfest

@bolinfest bolinfest commented Apr 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Summary

  • converts legacy SandboxPolicy turn overrides into PermissionProfile before building SessionSettingsUpdate
  • removes sandbox_policy from SessionSettingsUpdate, keeping the session update/apply path profile-based
  • preserves legacy behavior by clearing the active named profile when a legacy sandbox override was the source of the profile

Review Notes

  • Protocol Op payloads and app-server request handling still accept legacy sandboxPolicy; this PR only moves the conversion boundary earlier.
  • The conversion uses the same cwd normalization as session updates and preserves existing deny-read restrictions when rebuilding the runtime filesystem policy.

Verification

  • cd codex-rs && just fmt
  • cd codex-rs && cargo test -p codex-core session_configuration_apply_permission_profile_preserves_existing_deny_read_entries
  • cd codex-rs && cargo test -p codex-core override_turn_context
  • cd codex-rs && 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 13:45
@bolinfest
bolinfest changed the base branch from main to pr20424 April 30, 2026 13:45
This was referenced Apr 30, 2026
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
This was referenced Apr 30, 2026
Closed
Closed
Closed
Closed
Closed
Closed
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Apr 30, 2026

Copy link
Copy Markdown
Contributor

💡 Codex Review

codex/codex-rs/tui/src/app_server_session.rs

Lines 1414 to 1417 in 2bc3c95

// 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 Fall back to response sandbox when permissionProfile missing

When permissionProfile is omitted in thread/start|resume|fork responses (which is still accepted for backward compatibility), this code now uses the local config profile instead of the server-provided legacy sandbox field. In remote mode that can desynchronize the TUI from the thread’s actual permissions, so subsequent turns may send the wrong sandbox overrides and surface incorrect permission state. Please preserve the previous remote fallback behavior by deriving permissions from the response sandbox when permissionProfile is absent.

ℹ️ 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".

This was referenced Apr 30, 2026
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Closed
@github-actions

github-actions Bot commented May 15, 2026

Copy link
Copy Markdown
Contributor

Closing this pull request because it has had no updates for more than 14 days. If you plan to continue working on it, feel free to reopen or open a new PR.

@github-actions github-actions Bot closed this May 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@bolinfest