Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d41c5da300
ℹ️ 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 Ok(permissions) = | ||
| codex_windows_sandbox::ResolvedWindowsSandboxPermissions::try_from_permission_profile_for_cwd( | ||
| &permission_profile, | ||
| cwd.as_path(), | ||
| ) | ||
| else { | ||
| return Some((Vec::new(), 0, true)); |
There was a problem hiding this comment.
Returning Some((Vec::new(), 0, true)) on any try_from_permission_profile_for_cwd error causes a false "world-writable scan failed" warning for non-managed profiles (for example Full Access / PermissionProfile::Disabled). In that state there is no Windows sandbox policy to enforce, so this path should be a no-op rather than a failure prompt; otherwise users can get a misleading blocking warning when switching back to Agent mode from Full Access.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8445ece. Unsupported/non-managed profiles now return None from the synchronous warning-details path and the background scan exits without sending a failed-scan warning.
| let permissions = super::resolved_permissions::ResolvedWindowsSandboxPermissions::try_from_permission_profile_for_cwd( | ||
| permission_profile, | ||
| permission_profile_cwd, | ||
| )?; |
There was a problem hiding this comment.
run_windows_sandbox_legacy_preflight now hard-fails on non-managed profiles before checking whether write-capability setup is needed. For unelevated setup flows that start from Full Access (PermissionProfile::Disabled) this turns a previously successful no-op preflight into an error, so windowsSandbox/setupStart can report failure even though no legacy ACL prep is required yet.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 8445ece. run_windows_sandbox_legacy_preflight now no-ops for profiles that cannot resolve to managed Windows sandbox permissions, and the setup refresh helpers match the same behavior. I added Windows-only regression tests for Disabled and External profiles.
bolinfest
force-pushed
the
pr23813
branch
2 times, most recently
from
4ddcfd4 to
6a5a6a6
Compare
bolinfest
force-pushed
the
pr23813
branch
3 times, most recently
from
1dfa0b9 to
0fdea96
Compare
1 participant
Why
The Windows sandbox runner still carried the old
SandboxPolicycompatibility path even though core now computesPermissionProfile. That meant Windows command-runner execution could only see the legacy projection, so profile-only filesystem rules such as deny globs were not part of the runner input.What Changed
SandboxPolicyparser/export and deletedwindows-sandbox-rs/src/policy.rs.PermissionProfileplus the profile cwd.SpawnRequestnow carriespermission_profile/permission_profile_cwdinstead of the legacypolicy_json_or_preset/sandbox_policy_cwdfields.PermissionProfile.PermissionProfileJSON.Verification
cargo test -p codex-windows-sandboxcargo test -p codex-core windows_sandboxcargo test -p codex-app-server request_processors::windows_sandbox_processorjust fix -p codex-windows-sandbox -p codex-core -p codex-app-server -p codex-cli -p codex-tuijust fix -p codex-cli -p codex-tuijust fix -p codex-windows-sandbox -p codex-tuirg "\\bSandboxPolicy\\b" codex-rs/windows-sandbox-rsreturned no matches.Note:
cargo test -p codex-cliwas attempted but did not reach crate tests because local disk filled while compiling dependencies (No space left on device). The targeted clippy pass compiled the affected CLI/TUI surfaces afterward.Stack created with Sapling. Best reviewed with ReviewStack.