Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c86d16a254
ℹ️ 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".
| prune_read_entries_under_writable_roots( | ||
| &mut file_system_policy.entries, | ||
| &legacy_non_cwd_writable_roots( | ||
| writable_roots, | ||
| *exclude_tmpdir_env_var, | ||
| *exclude_slash_tmp, | ||
| ), | ||
| ); |
There was a problem hiding this comment.
[P2] Keep cwd-scoped writable roots from becoming read-only carveouts
The cwd-free conversion only prunes legacy readable roots under explicit non-cwd writable roots. For WorkspaceWrite with a restricted readable root under the eventual cwd, for example /repo/docs with cwd=/repo, legacy sandboxing still allowed writes there because cwd was writable. This projection keeps /repo/docs as a more-specific Read entry next to :cwd Write, so once the profile is resolved, can_write_path_with_cwd denies writes under /repo/docs.
Suggested fix: keep the cwd-aware conversion for WorkspaceWrite policies that carry restricted readable roots, and use the cwd-free helper only when the legacy policy can be represented losslessly without cwd. At call sites that do not have a cwd, defer conversion until the cwd is available instead of storing this profile as active state. The shape I had in mind is:
let fs_policy = match sandbox_policy {
SandboxPolicy::WorkspaceWrite {
read_only_access: ReadOnlyAccess::Restricted { readable_roots, .. },
..
} if !readable_roots.is_empty() => {
FileSystemSandboxPolicy::from_legacy_sandbox_policy_for_cwd(sandbox_policy, cwd)
}
_ => FileSystemSandboxPolicy::from_legacy_sandbox_policy(sandbox_policy),
};There was a problem hiding this comment.
I looked into this and while #11387 introduced support for read_only_access: ReadOnlyAccess and exposed it via app-server, I don't believe we are using this. Rather, this was an intermediate attempt to support a richer permission profile, but that was before we came up with the new [permissions] spec and so we should be deleting this rather than keeping this legacy attempt around.
Why
The profile conversion path still required a
cwdeven when it was only translating a legacySandboxPolicyinto aPermissionProfile. That made profile producers invent an ambientcwd, which is exactly the anchoring we are trying to remove from permission-profile data. A legacy workspace-write policy can be represented symbolically instead::cwd = writeplus read-only:project_rootsmetadata subpaths.This PR creates that cwd-free base so the rest of the stack can stop threading cwd through profile construction. Callers that actually need a concrete runtime filesystem policy for a specific cwd still have an explicitly named cwd-bound conversion.
What Changed
PermissionProfile::from_legacy_sandbox_policynow takes only&SandboxPolicy.FileSystemSandboxPolicy::from_legacy_sandbox_policyis now the symbolic, cwd-free projection for profiles.FileSystemSandboxPolicy::from_legacy_sandbox_policy_for_cwdfor runtime/boundary code that must materialize legacy cwd behavior.CurrentWorkingDirectoryandProjectRootsspecial entries instead of materializing cwd into absolute paths.Verification
cargo check -p codex-protocol -p codex-core -p codex-app-server-protocol -p codex-app-server -p codex-exec -p codex-exec-server -p codex-tui -p codex-sandboxing -p codex-linux-sandbox -p codex-analytics --testsjust fix -p codex-protocol -p codex-core -p codex-app-server-protocol -p codex-app-server -p codex-exec -p codex-exec-server -p codex-tui -p codex-sandboxing -p codex-linux-sandbox -p codex-analyticsStack created with Sapling. Best reviewed with ReviewStack.