Use a unified shell tell to not break cache #3814
Conversation
aibrahim-oai
requested review from
bolinfest,
dylan-hurd-oai and
fouad-openai
and removed request for
bolinfest
![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 suggestions.
Reply with @codex fix comments to fix any unresolved comments.
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, or 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 fix this CI failure" or "@codex address that feedback".
codex-rs/core/src/safety.rs
Outdated
| if let Some(decision) = | ||
| reject_forbidden_escalation(approval_policy, with_escalated_permissions, false) | ||
| { | ||
| return decision; | ||
| } | ||
|
|
||
| match (approval_policy, sandbox_policy) { | ||
| (UnlessTrusted, _) => { | ||
| // Even though the user may have opted into DangerFullAccess, |
There was a problem hiding this comment.
[P1] Reject escalated commands even when sandboxing is disabled
In assess_safety_for_untrusted_command the new reject_forbidden_escalation call returns SafetyCheck::Reject for OnFailure or Never whenever with_escalated_permissions is true (lines 157‑160). This happens before the match that handles SandboxPolicy::DangerFullAccess (lines 170‑174). As a result, in sessions where the user has explicitly opted into DangerFullAccess (no sandbox available), any model invocation that includes with_escalated_permissions: true is now rejected with the message “sandbox retry is required…”, even though there is no sandbox to retry and the previous behavior auto‑approved these commands. This means escalated commands can no longer run in DangerFullAccess when the model sets the flag, defeating the purpose of that mode. Consider skipping the rejection when the sandbox policy is DangerFullAccess or when sandboxing is otherwise unavailable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I think we should reject commands with escalated params in DangerFullAccess mode. Curious what others think?
|
@aibrahim-oai can you please update the PR body to explain the "why" behind this change? |
|
@bolinfest done |
Currently, we change the tool description according to the sandbox policy and approval policy. This breaks the cache when the user hits
/approvals. This PR does the following:create_shell_tool_for_sandboxand always uses unified tool viacreate_shell_tool