Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f68960302a
ℹ️ 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".
bb76782 to
b9f3284
Compare
e8ce411 to
dfbc24a
Compare
| ), | ||
| )?; | ||
| } else { | ||
| if cwd_changed |
There was a problem hiding this comment.
This rebind path no longer runs for ordinary workspace-write profiles. current_file_system_sandbox_policy above is already materialized against workspace_roots, so the symbolic ProjectRoots entry checked here is gone by the time we reach this condition. As a result, turn/start calls that only change cwd keep write access anchored to the previous roots. Suggest preserving the old cwd-only behavior for implicit/default-seeded roots, or making the rebind decision from the unmaterialized profile so explicit roots can stay sticky without breaking existing callers.
There was a problem hiding this comment.
Addressed in 0f90ce4: the rebind decision now checks the unmaterialized PermissionProfile filesystem policy for the symbolic project-roots write entry, before materialization removes it. I also added workspace_roots_explicit tracking so cwd-only updates rebind only the implicit [cwd] root; explicit or persisted roots remain stable unless workspaceRoots is updated. Covered by session_configuration_apply_rebinds_implicit_workspace_root_on_cwd_update and session_configuration_apply_preserves_explicit_workspace_roots_on_cwd_update.
| /// entries for this thread. | ||
| #[experimental("thread/start.workspaceRoots")] | ||
| #[serde(default)] | ||
| pub workspace_roots: Vec<AbsolutePathBuf>, |
There was a problem hiding this comment.
Adding workspaceRoots here while removing the full effective permission profile makes this response lossy. sandbox + workspaceRoots + activePermissionProfile cannot represent richer filesystem state such as deny-read overlays or split read/write policies, and downstream code already has to reconstruct from the legacy sandbox projection. Suggest keeping an exact effective-permissions field in these lifecycle responses, even if activePermissionProfile remains the preferred high-level selector.
There was a problem hiding this comment.
Addressed in 0f90ce4: lifecycle responses now include a read-only exact permissionProfile, materialized with the thread workspaceRoots, in addition to activePermissionProfile, workspaceRoots, and the legacy sandbox projection. Exec/TUI clients prefer that exact field when present and keep the legacy sandbox fallback for older servers. The request APIs still only allow selecting by id or updating workspaceRoots, so clients cannot replace the thread PermissionProfile value through app-server calls.
e6b6753 to
d11aa33
Compare
5e82c0d to
f65d5b5
Compare
Why
PermissionProfileis becoming the source of truth for a thread's effective permissions, butworkspace-writeroots were still split across multiple representations.SandboxPolicy::WorkspaceWritecarried its ownwritable_roots, rollout data did not treat workspace roots as first-class thread state, and status / compatibility code had to reconstruct the current roots indirectly.That split makes resume, fork, memories, and app-server lifecycle responses harder to keep consistent. This PR moves workspace roots onto thread state so the current thread owns that data and legacy sandbox projections become derived compatibility output.
What Changed
workspace_rootsalongside thread/session state in rollout and thread-store metadata, and carry them through resume/fork reconstruction.SandboxPolicy::WorkspaceWrite; the policy now carries only sandbox flags, and project roots are materialized from the current thread'sworkspace_roots.cwd, so cwd-only updates can rebind the implicit default root while preserving explicit/persisted roots unlessworkspaceRootsis updated.ActivePermissionProfileModificationand tighteningPermissionsaccess around the canonicalPermissionProfile.workspaceRoots,activePermissionProfile, the legacysandboxprojection, and a read-only exactpermissionProfileso clients can recover non-lossy effective permissions; app-server request APIs still cannot replace that profile value.PermissionProfile + workspace_roots, including hiding internal writable roots such as Codex memories from/status.Compatibility and Migration
SandboxPolicy::WorkspaceWrite.writable_rootsis migrated into threadworkspace_rootswhen loaded.workspace_rootscontinue to fall back tocwdwhere the legacy format implied that behavior, including oldSessionMetalines.workspace_rootsstill round trips as empty; this matters for read-only / full-access cases where workspace roots are intentionally irrelevant.SessionMetaaliases such asagent_typecontinue to deserialize while the custom workspace-roots migration runs.permissionProfileuse that exact read-only field; older clients can continue reconstructing from the legacysandboxprojection.Verification
The targeted regression coverage here focused on the persistence, migration, and response-projection paths most likely to silently break stored threads or memories:
turn_context_item_migrates_legacy_workspace_write_writable_rootsturn_context_item_migrates_missing_workspace_roots_to_cwdsession_meta_migrates_missing_workspace_roots_to_cwdsession_meta_preserves_explicit_empty_workspace_rootssession_meta_preserves_legacy_agent_type_aliasdeserialize_legacy_session_configured_event_migrates_workspace_write_writable_rootsworkspace_write_summary_hides_internal_writable_rootssession_configuration_apply_rebinds_implicit_workspace_root_on_cwd_updatesession_configuration_apply_preserves_explicit_workspace_roots_on_cwd_updatethread_resume_preserves_persisted_workspace_roots_when_request_omits_themthread_fork_preserves_persisted_workspace_roots_when_request_omits_themcodex-execsession_configured_from_thread_*_uses_workspace_roots_for_workspace_sandboxcodex-execsession_configured_from_thread_response_prefers_permission_profile_fieldcodex-rollout/codex-thread-storeresume and metadata tests, including empty-root round tripsStack created with Sapling. Best reviewed with ReviewStack.