Prompt to turn on windows sandbox when auto mode selected. #6618
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.
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ 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-rs/tui/src/chatwidget/tests.rs
Outdated
| if cfg!(target_os = "windows") { | ||
| set_windows_sandbox_enabled(false); | ||
| chat.config.forced_auto_mode_downgraded_on_windows = true; | ||
| } | ||
| chat.open_approvals_popup(); |
There was a problem hiding this comment.
The new test approvals_popup_includes_windows_sandbox_note_for_auto_mode (lines 1462‑1499) toggles the process‑global Windows sandbox flag via set_windows_sandbox_enabled(false/true) to simulate different environments. Because the file has no synchronization (no serial_test::serial, no mutex, etc.), this mutable global state is shared with every other test in the same crate. On Windows these tests run in parallel by default, so this test (and startup_prompts_for_windows_sandbox_when_auto_requested at lines 1545‑1556, which does the same) can interleave, leaving the sandbox flag in the wrong state and making the assertions flaky. Please serialize these tests or otherwise guard the global flag so they don't interfere with each other.
Useful? React with 👍 / 👎.
dylan-hurd-oai
left a comment
dylan-hurd-oai
left a comment
There was a problem hiding this comment.
cc @pakrym for some thoughts here, I feel like we're managing config and the actual behavior separately which breaks down for the app-server
iceweasel-oai
force-pushed
the
dev/iceweasel/windows-sandbox-nux
branch
from
93a4104 to
7b3af7a
Compare
dylan-hurd-oai
left a comment
dylan-hurd-oai
left a comment
There was a problem hiding this comment.
1 more comment about abstractions
dylan-hurd-oai
left a comment
dylan-hurd-oai
left a comment
There was a problem hiding this comment.
1 comment about macros, otherwise LGTM
| use uuid::Uuid; | ||
|
|
||
| #[test] | ||
| #[ignore = "timing out"] |
There was a problem hiding this comment.
is this consistently timing out?
| self.chat_widget.open_windows_sandbox_enable_prompt(preset); | ||
| } | ||
| AppEvent::EnableWindowsSandboxForAuto { preset } => { | ||
| #[cfg(target_os = "windows")] |
There was a problem hiding this comment.
Can we use if (cfg(windows)) here instead?
Uh oh!
There was an error while loading. Please reload this page.