Conversation
This was referenced Apr 27, 2026
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b5373d9b2
ℹ️ 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".
Comment on lines
+82
to
+86
| session.sandbox_policy = self | ||
| .config | ||
| .permissions | ||
| .legacy_sandbox_policy(thread.cwd.as_path()); | ||
| session.permission_profile = self.legacy_permission_profile_for_cwd(thread.cwd.as_path()); |
Contributor
There was a problem hiding this comment.
Overwriting session.sandbox_policy/session.permission_profile from self.config.permissions here can regress permissions when thread/read is used as a fallback after thread/resume fails: self.config may still hold startup defaults while the active session has already been updated from server state (for example, via enqueue_primary_thread_session calling chat_widget.handle_thread_session without syncing self.config). In that case, this code downgrades the replayed thread to stale defaults (often read-only), causing incorrect approval/permission behavior for that thread.
Useful? React with 👍 / 👎.
Collaborator
Author
There was a problem hiding this comment.
Addressed in c543c8d681. thread/read fallback now derives from the active chat widget permission settings instead of stale self.config, and rebuilds the fallback profile for the target thread cwd. Covered by thread_read_fallback_uses_active_permission_settings.
bolinfest
force-pushed
the
pr19772
branch
2 times, most recently
from
8335dc5 to
bbb9acc
Compare
bolinfest
force-pushed
the
pr19773
branch
2 times, most recently
from
19b0792 to
3e7f657
Compare
bolinfest
force-pushed
the
pr19773
branch
2 times, most recently
from
8e23a09 to
dbfac94
Compare
Bojun-Vvibe
added a commit
to Bojun-Vvibe/oss-contributions
that referenced
this pull request
Apr 27, 2026
- openai/codex#19773 permissions: require profiles in TUI thread state (merge-after-nits) - openai/codex#19755 Add Responses stream lifecycle diagnostics (merge-after-nits) - openai/codex#19753 Terminate stdio MCP servers on shutdown (merge-as-is) - BerriAI/litellm#26581 fix(semantic-filter): run for anthropic_messages call type (merge-as-is)
bolinfest
force-pushed
the
pr19773
branch
2 times, most recently
from
0519638 to
86504e2
Compare
bolinfest
force-pushed
the
pr19772
branch
2 times, most recently
from
b1c33f6 to
eec6237
Compare
bolinfest
force-pushed
the
pr19773
branch
2 times, most recently
from
536ab2d to
ef36313
Compare
bolinfest
force-pushed
the
pr19772
branch
2 times, most recently
from
bd9c490 to
87b6e80
Compare
bolinfest
force-pushed
the
pr19772
branch
2 times, most recently
from
5357a4a to
d95b637
Compare
bolinfest
force-pushed
the
pr19773
branch
2 times, most recently
from
030cb28 to
5b19acc
Compare
bolinfest
force-pushed
the
pr19772
branch
2 times, most recently
from
e103c1c to
d7ebb4c
Compare
bolinfest
added a commit
that referenced
this pull request
Apr 27, 2026
## Why This continues the permissions migration by making legacy config default resolution produce the canonical `PermissionProfile` first. The legacy `SandboxPolicy` projection should stay available at compatibility boundaries, but config loading should not create a legacy policy just to immediately convert it back into a profile. Specifically, when `default_permissions` is not specified in `config.toml`, instead of creating a `SandboxPolicy` in `codex-rs/core/src/config/mod.rs` and then trying to derive a `PermissionProfile` from it, we use `derive_permission_profile()` to create a more faithful `PermissionProfile` using the values of `ConfigToml` directly. This also keeps the existing behavior of `sandbox_workspace_write` and extra writable roots after #19841 replaced `:cwd` with `:project_roots`. Legacy workspace-write defaults are represented as symbolic `:project_roots` write access plus symbolic project-root metadata carveouts. Extra absolute writable roots are still added directly and continue to get concrete metadata protections for paths that exist under those roots. The platform sandboxes differ when a symbolic project-root subpath does not exist yet. * **Seatbelt** can encode literal/subpath exclusions directly, so macOS emits project-root metadata subpath policies even if `.git`, `.agents`, or `.codex` do not exist. * **bwrap** has to materialize bind-mount targets. Binding `/dev/null` to a missing `.git` can create a host-visible placeholder that changes Git repo discovery. Binding missing `.agents` would not affect Git discovery, but it would still create a host-visible project metadata placeholder from an automatic compatibility carveout. Linux therefore skips only missing automatic `.git` and `.agents` read-only metadata masks; missing `.codex` remains protected so first-time project config creation goes through the protected-path approval flow. User-authored `read` and `none` subpath rules keep normal bwrap behavior, and `none` can still mask the first missing component to prevent creation under writable roots. ## What Changed - Adds profile-native helpers for legacy workspace-write semantics, including `PermissionProfile::workspace_write_with()`, `FileSystemSandboxPolicy::workspace_write()`, and `FileSystemSandboxPolicy::with_additional_legacy_workspace_writable_roots()`. - Makes `FileSystemSandboxPolicy::workspace_write()` the single legacy workspace-write constructor so both `from_legacy_sandbox_policy()` and `From<&SandboxPolicy>` include the project-root metadata carveouts. - Removes the no-carveout `legacy_workspace_write_base_policy()` path and the `prune_read_entries_under_writable_roots()` cleanup that was only needed by that split construction. - Adds `ConfigToml::derive_permission_profile()` for legacy sandbox-mode fallback resolution; named `default_permissions` profiles continue through the permissions profile pipeline instead of being reconstructed from `sandbox_mode`. - Updates `Config::load()` to start from the derived profile, validate that it still has a legacy compatibility projection, and apply additional writable roots directly to managed workspace-write filesystem policies. - Updates Linux bwrap argument construction so missing automatic `.git`/`.agents` symbolic project-root read-only carveouts are skipped before emitting bind args; missing `.codex`, user-authored `read`/`none` subpath rules, and existing missing writable-root behavior are preserved. - Adds coverage that legacy workspace-write config produces symbolic project-root metadata carveouts, extra legacy workspace writable roots still protect existing metadata paths such as `.git`, and bwrap skips missing `.git`/`.agents` project-root carveouts while preserving missing `.codex` and user-authored missing subpath rules. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/19772). * #19776 * #19775 * #19774 * #19773 * __->__ #19772
bolinfest
force-pushed
the
pr19773
branch
2 times, most recently
from
fcfcacf to
5ffd215
Compare
viyatb-oai
approved these changes
Apr 28, 2026
This was referenced Apr 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
ThreadSessionStateis the TUI's cached view of an app-server session. To makePermissionProfilethe canonical runtime permissions model, cached thread sessions need to always have a profile instead of treating the profile as an optional supplement to a legacysandboxresponse field.The main compatibility concern is older app-server v2 lifecycle responses that only include
sandboxand omitpermissionProfile:thread/start->ThreadStartResponse.sandboxthread/resume->ThreadResumeResponse.sandboxthread/fork->ThreadForkResponse.sandboxThose responses must still hydrate correctly when the TUI is pointed at an older app-server. This PR converts the legacy
sandboxvalue into aPermissionProfileimmediately at response ingestion time, using the responsecwd, so cached sessions do not carry an optional profile that can later reinterpret cwd-bound grants against a different thread cwd.This fallback is intentionally boundary compatibility. The follow-up PRs in this stack continue the cleanup by making
SessionConfiguredEventprofile-only, deriving sandbox projections from snapshots only when an API still needs them, and then removingsandbox_policyfromThreadSessionState.What Changed
ThreadSessionState.permission_profilerequired.sandboxvalues into aPermissionProfileat ingestion time using the response cwd.thread/readhydration does not reuse a primary session profile that may be anchored to a different cwd; it uses the active widget permission settings for the read thread fallback instead of reusing cached primary-session permissions.Verification
cargo test -p codex-tui thread_read --libcargo test -p codex-tui permission_settings_sync_preserves_active_profile_only_rules --libcargo test -p codex-tui resume_response_restores_turns_from_thread_items --libcargo test -p codex-tui thread_session_state::tests --libStack created with Sapling. Best reviewed with ReviewStack.