-
Notifications
You must be signed in to change notification settings - Fork 7.2k
Assemble sandbox/approval/network prompts dynamically #8961
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
base: main
Are you sure you want to change the base?
Conversation
|
@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.
Reviewed commit: a40e5a755a
ℹ️ 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 this and make sure tests changes didn't invalidate tests original intentions |
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.
Reviewed commit: 2240249141
ℹ️ 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 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc5c68019b
ℹ️ 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/protocol/src/prompts/permissions/approval_policy/on_request.md
Outdated
Show resolved
Hide resolved
|
@codex review |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
| text: String, | ||
| } | ||
|
|
||
| const APPROVAL_POLICY_NEVER: &str = include_str!("prompts/permissions/approval_policy/never.md"); |
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.
a bit strange to see all of this formatting stuff in the protocol
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.
Yeah, just so that they can be methods on the structs. I can move them with the structs to core.
codex-rs/protocol/src/models.rs
Outdated
| approval_policy: AskForApproval, | ||
| writable_roots: Option<Vec<AbsolutePathBuf>>, | ||
| ) -> Self { | ||
| DeveloperInstructions::sandbox_text(sandbox_mode, network_access) |
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.
A lot of methods. Do we have this many callsites that we need all the overrides?
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.
Not really, we can have one method but just wanted to divide responsibility so we don't have a super long function with complicated logic.
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.
You can return strings from helper method and make them private.
codex-rs/protocol/src/models.rs
Outdated
|
|
||
| pub fn from_policy(sandbox_policy: &SandboxPolicy, approval_policy: AskForApproval) -> Self { | ||
| let (sandbox_mode, network_access, writable_roots) = match sandbox_policy { | ||
| SandboxPolicy::DangerFullAccess => { |
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.
Why are these values encoded here? is there an existing place where we convert SandboxPolicy to SandboxMode/network?
| ResponseItem::Message { | ||
| id: None, | ||
| role: "developer".to_string(), | ||
| content: vec![ContentItem::InputText { |
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.
Do we need a tag to wrap so we can find and ignore this message in future cli versions?
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.
on resume?
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.
On resume or in the future when we decide to parse some of these dev messages
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.
ok will add a tag
| RolloutItem::ResponseItem(items[0].clone()), | ||
| RolloutItem::ResponseItem(items[1].clone()), | ||
| RolloutItem::ResponseItem(items[2].clone()), | ||
| RolloutItem::ResponseItem(items[3].clone()), |
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.
is this expected?
| #[derive(Debug, Clone, Serialize, Deserialize, PartialEq)] | ||
| #[serde(rename = "environment_context", rename_all = "snake_case")] | ||
| pub(crate) struct EnvironmentContext { | ||
| pub cwd: Option<PathBuf>, |
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.
we left so little here I wonder whether it matters.
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.
I want to move it to dev slowly per the plan.
|
@codex do we append a new permission message on resume and fork next to the older messages? If yes, add an integration test. If not, change behavior and add integration test. |
|
Summary
Testing
|
Follow-up: adding a config value to replace the developer permissions message for custom sandboxes.