-
Notifications
You must be signed in to change notification settings - Fork 6.5k
more world-writable warning improvements #6389
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
more world-writable warning improvements #6389
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.
ℹ️ 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.
ℹ️ 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".
| if let Ok(ref paths) = result | ||
| && !paths.is_empty() | ||
| { |
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.
Avoid unstable let-chains in scan check
The new guard in spawn_world_writable_scan uses if let Ok(ref paths) = result && !paths.is_empty(), which relies on the let_chains feature (codex-rs/tui/src/app.rs lines 654‑656). This syntax is still unstable on the Rust channel used by the repo, so the crate will not compile (rustc emits E0658: "let expressions in this position are unstable"). Please split the guard into a normal if let followed by an explicit if !paths.is_empty() (or equivalent) so the code builds on stable Rust.
Useful? React with 👍 / 👎.
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 The workspace pins rust-toolchain.toml:2 to 1.90.0, where let-chains are stable. This compiles just fine, all CI checks pass. Not going to change this
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.
Summary
- Added logging for failed world-writable scans so we record the error and still surface the warning with a
failed_scanflag when the sandbox audit cannot complete. tui/src/app.rsL640-L688 - Clarified that
windows_world_writable_flaggedtreats scan failures as unsafe by usingmap_or(true, …)and documenting why errors still trigger the warning path. tui/src/chatwidget.rsL2142-L2155
Testing
- ✅
cargo test -p codex-tui
etraut-openai
left a comment
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.
Looks good overall. I left one recommendation.
| let title_line = Line::from("Unprotected directories found").bold(); | ||
| let info_line = if failed_scan { | ||
| Line::from(vec![ | ||
| "We couldn't complete the world-writable scan, so protections cannot be verified. " |
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.
Minor nit: It's a bit unusual and inconsistent to use "We" in an error message like this.
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.
ahh yeah, I agree. Lemme land this and update in my next PR (it should be rare for folks to ever see this message)
3 improvements: