-
Notifications
You must be signed in to change notification settings - Fork 7.1k
Elevated sandbox NUX #8789
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Elevated sandbox NUX #8789
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e74fe4221
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73dd5638db
ℹ️ 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".
|
|
||
| #[cfg(not(target_os = "windows"))] | ||
| pub fn sandbox_setup_is_complete(_codex_home: &Path) -> bool { | ||
| false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this return true? There is no need to do windows sandbox setup on other platforms, so it is compete in a technical/literal sense. Less technically speaking, if a check on this condition leaks into linux/macos code, we probably don't want to follow the path more likely to to invoke run_elevated_setup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I'll update this in a future PR. For now there is no leakage
| cfg!(target_os = "windows") | ||
| && codex_core::windows_sandbox::ELEVATED_SANDBOX_NUX_ENABLED | ||
| && codex_core::get_platform_sandbox().is_some() | ||
| && !codex_core::is_windows_elevated_sandbox_enabled() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like we're piling tech debt on top of the "2 booleans" problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed, will address in a future PR
| use std::time::Duration; | ||
| use std::time::Instant; | ||
|
|
||
| fn windows_degraded_sandbox_active() -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we s/degraded/partial/g ?
| #[cfg(not(target_os = "windows"))] | ||
| { | ||
| Self::approval_preset_actions(preset.approval, preset.sandbox.clone()) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: can we bump the non-windows variant to be first here?
| preset: preset_clone.clone(), | ||
| }); | ||
| })] | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[non-blocking] I know this PR is not the source of the problem, but I feel like we're reaching a tipping point with the complexity in this function - we're iterating over the presets as if we're agnostic, but almost all of the logic in the function is targeted at specific variants. Seems like we'd be better off with a more declarative pattern.
| } | ||
|
|
||
| #[cfg(target_os = "windows")] | ||
| pub(crate) fn open_windows_sandbox_enable_prompt(&mut self, preset: ApprovalPreset) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we move this function out of chatwidget into its own module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do in a future PR
| pub(crate) fn open_windows_sandbox_enable_prompt(&mut self, _preset: ApprovalPreset) {} | ||
|
|
||
| #[cfg(target_os = "windows")] | ||
| pub(crate) fn open_windows_sandbox_fallback_prompt( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto above, separate module for this code would be really helpful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do in a future PR
| self.chat_widget | ||
| .open_windows_sandbox_fallback_prompt(preset, reason); | ||
| } | ||
| AppEvent::BeginWindowsSandboxElevatedSetup { preset } => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we wrap this handler logic into a function similar to open_windows_sandbox_fallback_prompt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do in a future PR
73dd563 to
98aa544
Compare
c8e5101 to
4111eae
Compare
|
@codex review again please! |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ 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". |
Elevated Sandbox NUX: