feat: allow non-workspace filesystem writes with read-only legacy fallback#15929
feat: allow non-workspace filesystem writes with read-only legacy fallback#15929
Conversation
954094f to
3ea5174
Compare
3ea5174 to
cbe1ece
Compare
cbe1ece to
4222b69
Compare
9469051 to
5cce1f7
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cce1f7345
ℹ️ 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".
| SandboxPolicy::ReadOnly { | ||
| access: read_only_access, |
There was a problem hiding this comment.
Expose split writable roots in model-facing permissions text
This fallback makes the legacy policy ReadOnly, but instruction generation derives sandbox_mode/writable_roots only from that legacy policy (protocol/src/models.rs:516-523). Sessions with :tmpdir or other non-workspace writes are therefore described to the model as read-only with no writable roots, so the model may avoid valid writes or over-request escalation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
this seems to need a larger refactor to get it fully right (moving the instruction generation from legacy sandbox policy to new filesystem policy), so won't include it here. can get out a followup PR if needed
Summary
This change stops rejecting permission profiles that write outside the workspace root when the canonical
FileSystemSandboxPolicycan represent them, even if the legacySandboxPolicybridge cannot.:tmpdir,/tmp, or other non-workspace roots to load successfully instead of failing during config loadSandboxPolicy::ReadOnlywhen non-workspace writes cannot be expressed asworkspace-write, while keeping the richer canonical filesystem policy for direct runtime enforcementadditional_writable_rootssuch as memories when the canonical filesystem policy already has writable roots, even if the legacy projection degrades to read-only:tmpdirexpands to the broader temp namespace, while legacyWorkspaceWritebridging keeps the narrower env-backed behavior throughTmpdirEnvVarWorkspaceWritepolicies now pick upTEMPandTMPinstead of only looking for Unix-styleTMPDIR/tmprealpath handlingWhy
The previous behavior was too strict in two ways. First, valid permission profiles were rejected during config load purely because the legacy sandbox projection was lossy, even though the canonical filesystem policy already described the requested writes. Second, temp-path behavior had drifted across platforms: config-facing
:tmpdir, legacy workspace-write bridging, and runtime sandbox defaults were not using the same boundaries.With this change, the canonical filesystem policy remains authoritative. When the legacy bridge cannot represent the writable surface, it degrades to read-only instead of failing, and callers can detect that direct runtime enforcement is required. At the same time, temp handling is clarified across Unix and Windows, and restricted seatbelt defaults stop granting tmp writes unless the policy opts into them.
Testing
verified that this command succeeds:
without this change, the command returns
Error: permissions profile requests filesystem writes outside the workspace root, which is not supported until the runtime enforces FileSystemSandboxPolicy directly.and this command fails: