core: move workspace roots onto environments - #31655
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76ec0b291f
ℹ️ 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".
| .map(PathUri::to_abs_path) | ||
| .collect::<std::io::Result<Vec<_>>>() | ||
| }) | ||
| .transpose() | ||
| .ok() |
There was a problem hiding this comment.
Keep foreign environment roots out of host path conversion
For cross-OS remote environments, such as a Linux app-server selecting a Windows exec environment with runtimeWorkspaceRoots: ["C:\\repo"], PathUri::to_abs_path rejects the Windows URI because it is not host-native, and the error is discarded before falling back to the legacy host roots. The turn context and model-visible filesystem permissions are then built for the host fallback instead of the selected environment-native roots, despite the API accepting those roots.
AGENTS.md reference: AGENTS.md:L317-L322
Useful? React with 👍 / 👎.
| let legacy_fallback_workspace_roots = workspace_roots.unwrap_or(snapshot.workspace_roots); | ||
| let environment_selections = environment_selections.unwrap_or_else(|| { | ||
| self.thread_manager.default_environment_selections( | ||
| &legacy_fallback_cwd, | ||
| &legacy_fallback_workspace_roots, | ||
| ) |
There was a problem hiding this comment.
Retarget implicit workspace roots on cwd-only turns
When turn/start changes only cwd, this reuses the previous snapshot roots unchanged, so a normal thread whose roots were implicitly [old_cwd] creates the next default environment with cwd = new_cwd but workspace_roots = [old_cwd]. Profiles that grant :workspace_roots therefore keep permitting the old directory and not the new working directory, regressing the prior cwd-retargeting behavior for app-server turns that move between workspaces.
AGENTS.md reference: AGENTS.md:L102-L110
Useful? React with 👍 / 👎.
| .unwrap_or_else(|| PathUri::from_abs_path(&turn_ctx.cwd)); | ||
| let workspace_roots = turn_ctx.config.effective_workspace_roots(); | ||
| let workspace_roots = tool.workspace_roots(req); | ||
| let permissions = workspace_roots |
| windows_sandbox_private_desktop: self.windows_sandbox_private_desktop, | ||
| }) | ||
| .map_err(CodexErr::from)?; | ||
| let workspace_roots = self |
| .and_then(|turn_environment| turn_environment.cwd().to_abs_path().ok()) | ||
| .unwrap_or_else(|| session_configuration.cwd().clone()); | ||
| let per_turn_config = Self::build_per_turn_config(&session_configuration, cwd.clone()); | ||
| let workspace_roots = primary_turn_environment |
aa1b8e8 to
78b99eb
Compare
78b99eb to
2917cdb
Compare
|
@codex review this |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e215b3e109
ℹ️ 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".
| let environment_selections = environment_selections.unwrap_or_else(|| { | ||
| let workspace_roots = workspace_roots.unwrap_or(snapshot.workspace_roots); | ||
| self.thread_manager | ||
| .default_environment_selections(&legacy_fallback_cwd, &workspace_roots) |
There was a problem hiding this comment.
Rebind implicit roots on cwd-only updates
When a thread started with the default workspace root later receives only a cwd update, this path reuses snapshot.workspace_roots from the previous cwd instead of rebinding the implicit root to legacy_fallback_cwd. The next turn then runs in the new cwd while :workspace_roots still points at the old directory, so workspace-write commands or patches in the new cwd are denied unless the client also sends runtimeWorkspaceRoots (which thread/settings/update cannot do).
AGENTS.md reference: AGENTS.md:L102-L110
Useful? React with 👍 / 👎.
abce272 to
2020f57
Compare
|
@codex review this |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2020f5755e
ℹ️ 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".
| let sandbox = | ||
| turn.file_system_sandbox_context(/*additional_permissions*/ None, &turn_environment); |
There was a problem hiding this comment.
Preserve command cwd for intercepted patches
When apply_patch is intercepted from shell/exec_command with a workdir different from the selected environment's cwd, this builds the filesystem sandbox with turn_environment.cwd() instead of the cwd argument that the patch paths are resolved against. Relative filesystem grants/globs are therefore evaluated from the wrong directory, so a patch under the command workdir can be denied (or the wrong sibling path can be authorized) even though the actual command sandbox would use the command cwd.
Useful? React with 👍 / 👎.
| cwd: environments | ||
| .as_ref() | ||
| .map(|environments| environments.legacy_fallback_cwd.to_path_buf()), | ||
| workspace_roots: Some( | ||
| runtime_workspace_roots | ||
| .clone() | ||
| .unwrap_or_else(|| snapshot.workspace_roots.clone()), | ||
| ), | ||
| default_permissions: Some(permissions), |
There was a problem hiding this comment.
Preserve environment roots when resolving profiles
When a turn/start request combines permissions with runtimeWorkspaceRoots (or environments[].runtimeWorkspaceRoots) and selects a configured profile that uses :workspace_roots, this config reload only passes the fallback cwd into ConfigOverrides; the new environment-owned roots are not supplied while the profile is materialized. That makes the selected profile grant the old/current cwd instead of the roots requested for the turn, so the sandbox can deny intended writes or leave the wrong root writable for this combined app-server API request.
AGENTS.md reference: AGENTS.md:L102-L110
Useful? React with 👍 / 👎.
| per_turn_config.features.enabled(Feature::FastMode), | ||
| &model_info, | ||
| ); | ||
| let permission_profile = per_turn_config.permissions.effective_permission_profile(); |
There was a problem hiding this comment.
Use updated session permissions for each turn
When a turn applies a sandboxPolicy/permission update, SessionConfiguration::apply updates permission_profile_state, but this line now derives the TurnContext permissions from the cloned original Config instead of the updated session configuration. For example, a turn/start that downgrades a workspace-write thread to read-only can still run with the startup workspace-write profile, while an upgrade can be ignored in the opposite direction, so app-server permission overrides no longer reliably affect the actual tool sandbox for that turn.
AGENTS.md reference: AGENTS.md:L102-L110
Useful? React with 👍 / 👎.
| let legacy_fallback_cwd = match cwd { | ||
| Some(cwd) => cwd, | ||
| None => thread.config_snapshot().await.cwd().clone(), | ||
| }; |
There was a problem hiding this comment.
Keep local environment cwd as fallback
If a turn/start supplies explicit environments containing the local environment with a new cwd but omits the top-level cwd, this now keeps the thread's previous fallback cwd instead of using the local selection's cwd as before. The tools run in the selected environment, but thread/settings/updated and any later environment-less fallback continue to report/use the stale cwd, which is a behavior regression for clients that drive cwd through environments[].cwd.
AGENTS.md reference: AGENTS.md:L102-L110
Useful? React with 👍 / 👎.
|
@codex review this |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8fbc6719a7
ℹ️ 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".
| let workspace_roots = environment | ||
| .runtime_workspace_roots | ||
| .map(|roots| { | ||
| let mut resolved_roots = Vec::new(); | ||
| for root in roots { |
There was a problem hiding this comment.
Bound environment workspace roots before accepting them
When a v2 client supplies environments, each runtimeWorkspaceRoots list is accepted and deduped with no count or byte/token limit before it becomes the active environment's workspace roots and is rendered into the model-visible environment context. A large list can therefore inject unbounded context and churn the prompt cache; reject or truncate the per-environment roots at a hard cap before storing them.
AGENTS.md reference: AGENTS.md:L97-L100
Useful? React with 👍 / 👎.
| let legacy_fallback_cwd = match cwd { | ||
| Some(cwd) => cwd, | ||
| None => thread.config_snapshot().await.cwd().clone(), |
There was a problem hiding this comment.
Preserve selected local cwd when cwd is omitted
When turn/start supplies explicit environments without the legacy top-level cwd, this now leaves legacy_fallback_cwd at the previous thread cwd. Before this change the selected local environment's cwd became the fallback, so the same request updated app-server-visible settings and same-turn permission-profile resolution to the selected cwd; now a client switching local cwd via environments only gets model/tools on the new cwd while thread/settings/permission config still use the old one. Keep deriving the fallback from the local selected environment when available.
AGENTS.md reference: AGENTS.md:L102-L110
Useful? React with 👍 / 👎.
| pub(super) fn permission_profile(&self) -> PermissionProfile { | ||
| self.permission_profile_state | ||
| .permission_profile() | ||
| .clone() | ||
| .materialize_project_roots_with_workspace_roots(&self.workspace_roots) | ||
| self.permission_profile_state.permission_profile().clone() |
There was a problem hiding this comment.
Keep startup permission_profile materialized
When a thread uses a workspace/named profile whose filesystem entries contain :workspace_roots, SessionConfiguredEvent still reads this accessor during startup, but it now returns the raw symbolic profile instead of the profile materialized with the session's selected roots. For runtime workspace roots that are not just cwd, the persisted/evented permission_profile no longer contains the concrete allow roots and has no companion workspace-roots field, so consumers of the session_configured event or rollout history see the wrong effective command permissions.
AGENTS.md reference: AGENTS.md:L102-L110
Useful? React with 👍 / 👎.
| .workspace_roots() | ||
| .iter() | ||
| .map(PathUri::to_abs_path) | ||
| .collect::<std::io::Result<Vec<_>>>() | ||
| .ok() | ||
| }) | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
Preserve foreign workspace roots in turn context
When the selected primary environment uses paths from a different OS than the host, such as a Windows exec-server selected from Linux, each PathUri::to_abs_path call returns an error; the .ok().unwrap_or_default() path then drops the entire workspace root list. The remote sandbox still receives these roots as PathUris elsewhere, but the per-turn config, model-visible filesystem context, and persisted turn context lose them, so cross-OS turns report incorrect workspace permissions to the model.
AGENTS.md reference: AGENTS.md:L319-L322
Useful? React with 👍 / 👎.
| let sandbox = | ||
| turn.file_system_sandbox_context(/*additional_permissions*/ None, &turn_environment); |
There was a problem hiding this comment.
Use the patch cwd when verifying intercepted patches
When a shell/exec command invokes apply_patch from a subdirectory or explicit workdir, this now builds the verification sandbox with the environment cwd instead of the cwd argument that maybe_parse_apply_patch_verified uses to resolve the patch paths. For permission profiles that contain cwd-relative filesystem entries, the reads used to verify update/delete hunks are checked against the wrong base directory, so patches can be incorrectly denied or allowed depending on the environment root rather than the command's actual patch cwd.
Useful? React with 👍 / 👎.
|
@codex review this |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31fbc7a399
ℹ️ 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".
| .permission_profile() | ||
| .clone() | ||
| .materialize_project_roots_with_workspace_roots(&self.workspace_roots) | ||
| self.permission_profile_state.permission_profile().clone() |
There was a problem hiding this comment.
Keep configured permissions materialized
When the thread uses a workspace-based profile (for example :workspace or any profile containing :workspace_roots), this accessor now returns the raw symbolic profile. Existing callers such as the persisted SessionConfiguredEvent still use SessionConfiguration::permission_profile() for the protocol field documented as effective command permissions, while actual turns use per_turn_config.permissions.effective_permission_profile(). That makes the event/rollout report :workspace_roots instead of the concrete roots applied to commands, so consumers can reconstruct or display the wrong sandbox; either keep this accessor materialized or switch those external-event call sites to materialized_permission_profile().
AGENTS.md reference: AGENTS.md:L102-L110
Useful? React with 👍 / 👎.
| if let Some(environment_selections) = environment_selections { | ||
| let legacy_fallback_cwd = match cwd { | ||
| Some(cwd) => cwd, | ||
| None => thread.config_snapshot().await.cwd().clone(), |
There was a problem hiding this comment.
Retarget the fallback cwd for explicit local environments
When turn/start supplies an explicit local environment with a new cwd but omits the top-level cwd, this keeps legacy_fallback_cwd on the previous thread cwd. For threads still using the legacy sandboxPolicy: workspace-write path, SessionConfiguration::apply therefore sees no cwd change and does not rederive the absolute workspace-write permission profile, so the selected environment runs in the new directory while the sandbox remains bound to the old root (denying writes in the new workspace and potentially leaving the old root writable). The previous code used the local environment selection’s cwd for this fallback.
AGENTS.md reference: AGENTS.md:L102-L110
Useful? React with 👍 / 👎.
| ) -> Self { | ||
| let native_workspace_roots = workspace_roots | ||
| .iter() | ||
| .filter_map(|root| root.to_abs_path().ok()) |
There was a problem hiding this comment.
Preserve foreign roots when rendering permissions
When the selected environment uses paths that are not native to the app-server host (for example a Windows exec-server selected from Linux), to_abs_path() fails and this filter_map drops every workspace root before materializing the permission profile. The same roots are still rendered in <workspace_roots>, but the <permission_profile> loses the concrete :workspace_roots write/deny entries that the exec-server will enforce, so the model receives an inaccurate filesystem context for those remote environments.
AGENTS.md reference: AGENTS.md:L91-L100
Useful? React with 👍 / 👎.
|
@codex review this |
|
@codex review this |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6507c02a25
ℹ️ 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".
| let permissions = workspace_roots | ||
| .iter() | ||
| .map(PathUri::to_abs_path) | ||
| .collect::<std::io::Result<Vec<_>>>() | ||
| .map(|workspace_roots| { |
There was a problem hiding this comment.
Use target roots for sandbox override decisions
When a tool targets a non-primary environment, this now materializes permissions from that target environment's workspace roots, but the approval/sandbox override logic just above still uses turn_ctx.file_system_sandbox_policy(), which was built from the primary environment roots. If the primary environment has empty or different roots and the target environment has workspace-root deny-read carveouts, an escalated command can be allowed to bypass the sandbox based on the primary policy, dropping the target environment's denied-read restrictions. Recompute the filesystem policy from the same workspace_roots before calling sandbox_override_for_first_attempt and related sandbox-permission checks.
Useful? React with 👍 / 👎.
| let sandbox = | ||
| turn.file_system_sandbox_context(/*additional_permissions*/ None, &turn_environment); |
There was a problem hiding this comment.
Use command cwd for intercepted apply_patch sandbox
When apply_patch is intercepted from a shell/unified-exec command whose cwd differs from the selected environment's root, this now builds the filesystem sandbox with turn_environment.cwd() instead of the cwd passed to maybe_parse_apply_patch_verified. Any relative filesystem policy entries or cwd-scoped checks are then evaluated against the wrong directory, so patches run from subdirectories can be incorrectly denied or granted compared with the command's actual working directory. Pass the intercepted patch cwd into the sandbox context, as this path did before the change.
Useful? React with 👍 / 👎.
|
@codex review this |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Depends on #31892.
Why
Workspace roots describe the filesystem available in a selected execution environment, but core stored them as separate thread-global session state. That split allowed the selected
cwdand workspace roots to diverge and made remote executor sandbox context depend on app-server-host paths.What changed
runtimeWorkspaceRootsto app-server environment selectionscwdruntimeWorkspaceRootsonly when app-server synthesizes default environmentsPathUrivalues through core and into filesystem/process sandbox contexts:workspace_rootsfor the selected environmentExplicit empty environment roots remain empty. When explicit environments and top-level roots are both supplied, the explicit environments determine the roots.
Validation
just test -p codex-app-server-protocolcwddefaultingcodex-core,codex-app-server, andcodex-app-server-protocol