refactoring with_escalated_permissions to use SandboxPermissions instead #7750
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".
zhao-oai
force-pushed
the
dev/zhao/with-escalated-perms-refactor
branch
5 times, most recently
from
e93a646 to
6066388
Compare
|
@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".
| #[serde(alias = "timeout")] | ||
| pub timeout_ms: Option<u64>, | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub with_escalated_permissions: Option<bool>, | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| #[ts(optional)] | ||
| pub sandbox_permissions: Option<SandboxPermissions>, |
There was a problem hiding this comment.
The new sandbox_permissions field (lines 344-348) replaces the previous with_escalated_permissions flag but the struct no longer accepts the legacy key. Serde ignores unknown fields by default, so any existing client payloads that still send with_escalated_permissions: true will silently deserialize to the default UseDefault permissions and never request escalation/approval, leaving commands sandboxed and likely failing. Consider adding a serde alias or backward-compatibility handling so older requests keep working.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
not an issue here since this is an interface b/w model and codex harness
|
@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".
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| #[ts(optional)] | ||
| pub sandbox_permissions: Option<SandboxPermissions>, |
There was a problem hiding this comment.
Tool call params now only accept the new sandbox_permissions enum, with no serde alias for the previous with_escalated_permissions flag. Any client or cached response still emitting the old boolean will be silently ignored and default to UseDefault, meaning escalation requests that previously ran unsandboxed will now execute in the restricted sandbox and can fail (or be denied) despite the caller asking for elevation. Consider adding an alias/translation so legacy payloads continue to request escalation.
Useful? React with 👍 / 👎.
zhao-oai
force-pushed
the
dev/zhao/with-escalated-perms-refactor
branch
from
0a16ca4 to
5c3aaf9
Compare
helpful in the future if we want more granularity for requesting escalated permissions:
e.g when running in readonly sandbox, model can request to escalate to a sandbox that allows writes