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
|
|
||
| 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
from
30f0162 to
02009e5
Compare
gt-oai
force-pushed
the
admin-policies-allowlist-2
branch
from
02009e5 to
74c93ed
Compare
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