Handle "Don't Trust" directory selection in onboarding #4941
Conversation
|
Thanks for the contribution, and apologies for the slow response. We received a large number of PRs, and we're just now catching up on the backlog. It looks like this PR has gone stale. If you'd still like to pursue this change, could you please resolve the conflicts and make sure the tests run? Thanks! |
|
@etraut-openai No worries! I've updated the PR and resolved the conflicts. The implementation now persists the "DontTrust" decision in config.toml as trust_level = "untrusted" (similar to how "Trust" works) and handles it properly in both sandbox_mode and approval_policy logic. I also added tests to cover this behavior. |
|
@vmotta8, looks like the lint / formatting CI check is still failing. Could you fix that? Thanks! |
|
@etraut-openai Sure! Just fixed it, tks! |
codex-rs/core/src/config/mod.rs
Outdated
| pub fn is_untrusted(&self) -> bool { | ||
| match &self.trust_level { | ||
| Some(trust_level) => trust_level == "untrusted", | ||
| None => false, |
There was a problem hiding this comment.
We should think of the default, here you imply that there is a third state (other than trusted and untrusted) that is not defined anywhere
There was a problem hiding this comment.
Good point! I created the enum and removed the helpers as requested. For the is_untrusted() method, I followed the existing pattern from is_trusted() which also treats None as false. This third state was already implicit before my changes. Does it make sense to refactor this now, or should that be a separate change?
| // Verify that untrusted projects get WorkspaceWrite (or ReadOnly on Windows due to downgrade) | ||
| if cfg!(target_os = "windows") { | ||
| assert!( | ||
| matches!(resolution.policy, SandboxPolicy::ReadOnly), |
There was a problem hiding this comment.
There is no sandbox on Windows today.
There was a problem hiding this comment.
Thanks! Just to clarify: the test expects ReadOnly on Windows due to the downgrade logic, but since there's no actual sandbox on Windows, should I remove this assertion or handle it differently?
|
@codex review |
![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".
jif-oai
left a comment
jif-oai
left a comment
There was a problem hiding this comment.
Looks way better. Thank you
The comment of Codex just above is valid so please fix this. After that, lgtm
|
@jif-oai Done! Fixed the onboarding loop for untrusted projects and added a test. |
|
Thanks again for the contribution! |
5 participants
Fixes #4940
Fixes #4892
When selecting "No, ask me to approve edits and commands" during onboarding, the code wasn't applying the correct approval policy, causing Codex to block all write operations instead of requesting approval.
This PR fixes the issue by persisting the "DontTrust" decision in config.toml as
trust_level = "untrusted"and handling it in the sandbox and approval policy logic, so Codex correctly asks for approval before making changes.Before (bug)
After (fixed)