Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11750256e8
ℹ️ 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".
3eace94 to
ea13af2
Compare
014c589 to
0e2d80b
Compare
| @@ -203,14 +210,9 @@ pub struct ThreadStartResponse { | |||
| /// Reviewer currently used for approval requests on this thread. | |||
| pub approvals_reviewer: ApprovalsReviewer, | |||
| /// Legacy sandbox policy retained for compatibility. Experimental clients | |||
There was a problem hiding this comment.
[P1] This drops the exact active permissionProfile from thread start/resume/fork responses and leaves remote clients with only the legacy sandbox projection plus profile identity metadata. The remote TUI then rebuilds its local PermissionProfile from SandboxPolicy (app_server_session.rs), but that projection is lossy for profiles that do not round-trip through to_legacy_sandbox_policy and instead hit the compatibility workspace-write fallback. In that case the server keeps the exact profile while the TUI syncs a coarser approximation into local session/config state, so /status and any later UI logic reading config.permissions.permission_profile() can be wrong. Can we keep returning the effective permissionProfile here, or replace it with an equally expressive canonical field? Immutability does not require dropping observability.
There was a problem hiding this comment.
Addressed in d824faf: v2 thread start/resume/fork responses now return the effective permissionProfile again, and the remote TUI now prefers that exact response profile instead of rebuilding from the lossy legacy sandbox projection. Added coverage for the remote preference path as well.
4c0a41a to
bb9aa31
Compare
448ea1b to
d824faf
Compare
f7e604e to
f6eacf7
Compare
…permission profile modifications as an overlay for writable roots. Existing app-server threads no longer accept arbitrary PermissionProfile or SandboxPolicy replacements; permissions requests select a server-known profile id and apply the resolved server-owned profile together with active profile metadata. Workspace roots can be updated independently, and SandboxPolicy::WorkspaceWrite no longer stores its own writable_roots.
Why
Codex is moving permission state from the legacy
SandboxPolicyabstraction toPermissionProfile. For app-server threads, the effectivePermissionProfilevalue should be durable thread state: app-server clients should not be able to submit arbitrary replacement permission profiles through resume, fork, or turn APIs.The pieces that still need to be mutable are separate from that client-supplied value. A client can select a named profile by id, and the server resolves that id against local config before updating the thread's active profile name and effective server-owned profile value. A client can also update the thread's workspace roots independently. Keeping those concerns separate removes profile overlays such as
ActivePermissionProfileModificationand preventsSandboxPolicy::WorkspaceWritefrom carrying a second copy of workspace roots.What Changed
App-server API behavior
permissionsselection is now just a profile id string at the v2 boundary; unknown ids return a JSON-RPC error before a turn starts.PermissionProfileunless the client selects a valid server-known profile id, in which case the server applies the resolved profile value together with the active profile name.workspaceRootscan be updated independently fromcwd;cwdis only a workspace root when it appears explicitly in the root list.permissionProfilefor exact remote observability, plusactivePermissionProfileidentity metadata and the legacysandboxprojection for compatibility.codex exec resumenow omitsworkspaceRootsunless the invocation explicitly uses--add-dir, so ordinary exec resumes preserve the roots stored with the rollout instead of replacing them with the current process config.Thread state, rollout compatibility, and memories
workspace_rootsare persisted with thread/session state and replayed through turn context, rollout reconstruction, and thread-store metadata.SandboxPolicy::WorkspaceWrite.writable_rootsare migrated on deserialize so their extra writable roots are not lost afterSandboxPolicystops carrying that field.workspace_rootscontinue to win over any legacy sandbox-derived roots.workspace_rootslist and still works when clients pass explicitworkspaceRoots.CODEX_HOME/memoriesfrom user-facing workspace-root summaries, so enabling memories does not make/statusreport the memories store as the active workspace.Sandbox, status, and model-facing summaries
SandboxPolicy::WorkspaceWriteno longer ownswritable_roots; sandbox setup receives workspace roots from current thread/session state instead.PermissionProfilesummary path with explicit workspace roots, so/statusand related CLI/TUI surfaces show roots added with--add-dir.cwdis inherently writable.summarize_sandbox_policypath and TUI permission compatibility shim were removed where they are no longer needed.Cleanup and generated API artifacts
ActivePermissionProfileModification,PermissionProfileModificationParams, andPermissionProfileSelectionParamsfrom protocol models and generated TypeScript exports.$HOME/.agents/skillsso the expected omitted-skill count remains deterministic.Test Strategy
The main regression risks are not whether the workspace compiles, but whether permission/root state changes only through the intended API surfaces and whether old persisted state keeps replaying correctly. I focused the targeted tests around those invariants:
codex-rs/app-server-protocol/src/protocol/v2/tests.rs::turn_start_permissions_uses_profile_id_string_shapeverifiespermissionsis a bare profile id string.sandbox_policy_rejects_legacy_workspace_write_writable_roots_fieldverifies removedworkspaceWrite.writableRootspayloads are rejected instead of silently ignored.codex-rs/app-server/tests/suite/v2/turn_start.rs::turn_start_rejects_unknown_permission_selection_before_starting_turncovers unknown profile ids.turn_start_updates_cwd_without_replacing_workspace_roots_v2covers the important split between changingcwd, preservingworkspaceRoots, and selecting:workspace.codex-rs/app-server/tests/suite/v2/thread_resume.rs::thread_resume_running_applies_workspace_roots_and_active_profile_namecovers live resume updates to roots and active profile name.codex-rs/core/src/session/tests.rs::session_configuration_apply_preserves_legacy_workspace_roots_on_cwd_updateandsession_configuration_apply_preserves_active_permission_profile_on_legacy_cwd_updateprotect the turn-context behavior that app-server ultimately relies on.codex-rs/app-server/src/request_processors/thread_processor_tests.rs::persisted_thread_permission_state_uses_latest_turn_active_profileandpersisted_thread_permission_state_preserves_empty_workspace_roots_from_event_roundtripcover reconstruction of durable profile/root state from stored history.codex-rs/core/src/config/config_tests.rs::workspace_write_always_includes_memories_root_onceverifies the internal memories root remains writable.codex-rs/utils/sandbox-summary/src/sandbox_summary.rs::workspace_write_summary_hides_internal_writable_rootsandcodex-rs/tui/src/status/tests.rs::status_permissions_workspace_profile_hides_internal_memories_rootverify that root does not leak into/statusor sandbox summaries.codex-rs/utils/sandbox-summary/src/sandbox_summary.rs::workspace_write_summary_includes_workspace_roots_and_network_access,codex-rs/tui/src/status/tests.rs::status_permissions_named_workspace_profile_shows_workspace_roots, andcodex-rs/core/src/context/permissions_instructions_tests.rs::renders_sandbox_mode_textcover the user/model text surfaces affected by removing roots fromSandboxPolicy.codex exec resume:codex-rs/exec/src/lib_tests.rs::thread_resume_params_include_workspace_roots_only_when_explicitverifies ordinary exec resumes preserve rollout roots, while explicit--add-dirresumes still send replacement roots.session_configured_from_thread_response_uses_workspace_roots_from_responseandsession_configured_from_thread_response_preserves_empty_workspace_rootsverify exec trusts the server response exactly, including explicit empty roots.session_configured_from_thread_response_prefers_exact_permission_profileandsession_configured_from_thread_resume_response_prefers_exact_permission_profileverify exec keeps the exact v2permissionProfileinstead of reconstructing a lossy legacysandboxprojection.