feat: Constrain values for approval_policy #7778
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
gt-oai
force-pushed
the
admin-policies-allowlist-2
branch
3 times, most recently
from
b96996e to
697f5c3
Compare
gt-oai
force-pushed
the
admin-policies-allowlist-2
branch
from
697f5c3 to
693addf
Compare
codex-rs/core/src/config/mod.rs
Outdated
| impl<T: Send + Sync> Constrained<T> { | ||
| pub fn new( | ||
| initial_value: T, | ||
| validator: impl Fn(&T) -> std::io::Result<()> + Send + Sync + 'static, |
There was a problem hiding this comment.
Re-use ConstraintValidator in the type def
There was a problem hiding this comment.
I tried to do this but didn't seem able too -- something about runtime sizes vs compile-time sizes
gt-oai
force-pushed
the
admin-policies-allowlist-2
branch
3 times, most recently
from
1e0cd2d to
30f0162
Compare
codex-rs/core/src/config/mod.rs
Outdated
|
|
||
| pub const CONFIG_TOML_FILE: &str = "config.toml"; | ||
|
|
||
| const ALL_APPROVAL_POLICIES: [AskForApproval; 4] = [ |
There was a problem hiding this comment.
I would move as much of these new types as possible to another/new file. This file is already quite large.
There was a problem hiding this comment.
Moved into constraint.rs (new) and types.rs.
codex-rs/core/src/config/mod.rs
Outdated
| }) | ||
| } | ||
|
|
||
| pub fn allow_any(initial_value: T) -> Self { |
There was a problem hiding this comment.
This doesn't help the UI know what the supported values are, though? I guess that is a limitation of ConstraintValidator in general?
There was a problem hiding this comment.
It is a limitation of ConstraintValidator, but we could add this list of possible values to Constrained<T>. I wanted to ensure the abstraction could work when the set of possible values was not enumerable.
codex-rs/core/src/config/mod.rs
Outdated
| self.value | ||
| } | ||
|
|
||
| pub fn can_set(&self, candidate: &T) -> ConstraintResult<()> { |
There was a problem hiding this comment.
I see: so is the idea we can ask whether a candidate is supported, but we can't get the list of supported candidates? I guess it's more dynamic this way...
There was a problem hiding this comment.
Yes, I'm happy to extend this (e.g. ConstrainedSet, which directly contains the allowed_values). But I felt for now what we actually wanted is covered by (1) the can_set method and (2) the validator error message.
There's a risk we end up in a hairy ball of config-driven menus which I feel often isn't great for the user.
gt-oai
force-pushed
the
admin-policies-allowlist-2
branch
3 times, most recently
from
74c93ed to
a5d9bfb
Compare
jif-oai
left a comment
jif-oai
left a comment
There was a problem hiding this comment.
I will defer the @bolinfest to see if this is relevant to covert TUI2 but looks good
|
|
||
| impl From<ConstraintError> for std::io::Error { | ||
| fn from(err: ConstraintError) -> Self { | ||
| std::io::Error::new(std::io::ErrorKind::InvalidInput, err.to_string()) |
There was a problem hiding this comment.
Drop the to_string() to avoid loosing some part of the information
| if let Some(idx) = self.state.selected_idx | ||
| && let Some(actual_idx) = self.filtered_indices.get(idx) | ||
| && let Some(item) = self.items.get(*actual_idx) | ||
| && item.disabled_reason.is_none() |
There was a problem hiding this comment.
Just make sure that skipping disabled, never pushes the selection of screen. Might not be the case but we had the issue before
| (next, sandbox_policy_changed) | ||
| } | ||
| Err(err) => { | ||
| let wrapped = ConstraintError { |
There was a problem hiding this comment.
drop(state) here because we should release the mutex before making an async call (note the .await below) that no longer needs it.
bolinfest
left a comment
bolinfest
left a comment
There was a problem hiding this comment.
Just a few small comments, but otherwise LGTM, nice work!
| &self, | ||
| sub_id: String, | ||
| session_configuration: SessionConfiguration, | ||
| final_output_json_schema: Option<Option<Value>>, |
There was a problem hiding this comment.
double Option is a bit of a bad code smell...can this be Option<Value>?
There was a problem hiding this comment.
The Option<Option<>> is propagated from SessionSettingsUpdate, and I suppose is used to distinguish between "do nothing" and "explicitly set it to null". I'm not sure if that behaviour is necessary but if it is not I'd prefer the leave the cleanup to a follow up PR.
| }; | ||
|
|
||
| let current_context = sess.new_turn_with_sub_id(sub_id, updates).await; | ||
| let Ok(current_context) = sess.new_turn_with_sub_id(sub_id, updates).await else { |
There was a problem hiding this comment.
Should we log an error in the else case?
There was a problem hiding this comment.
new_turn_with_sub_id already emits an error event on failure. I've added a comment to this effect. Can add more logging if that's insufficient.
gt-oai
force-pushed
the
admin-policies-allowlist-2
branch
2 times, most recently
from
5e253a2 to
6a261fd
Compare
gt-oai
force-pushed
the
admin-policies-allowlist-2
branch
from
6a261fd to
b0025f5
Compare
|
FYI after our discussion, I removed the |
Constrain
approval_policythrough newadmin_policyconfig.This PR will:
admin_policysection to config, with a single field (for now)allowed_approval_policies. This list constrains the set of user-settableapproval_policys.Constrained<T>type, which combines a current value and a validator function. The validator function ensures disallowed values are not set.approval_policyonConfigandSessionConfigurationfromAskForApprovaltoConstrained<AskForApproval>. The validator function is set by the values passed intoallowed_approval_policies.GenericDisplayRow: add adisabled_reason: Option<String>. When set, it disables selection of the value and indicates as such in the menu. This also makes it unselectable with arrow keys or numbers. This is used in the/approvalsmenu.Follow ups are:
sandbox_policy.Happy to split this PR up if you prefer, into the logical numbered areas above. Especially if there are parts we want to gavel on separately (e.g. admin_policy).
Disabled full access:
Disabled
--yoloon startup:CODEX-4087