Feat: appServer.requirementList for requirement.toml #8800
Conversation
shijie-oai
force-pushed
the
shijie/app-server-requirement-list
branch
from
4e9dff8 to
336891b
Compare
| ]), | ||
| allowed_sandbox_modes: Some(vec![ | ||
| CoreSandboxModeRequirement::ReadOnly, | ||
| CoreSandboxModeRequirement::ExternalSandbox, |
There was a problem hiding this comment.
A little bit counterintuitive - ExternalSandbox is not supported in config.toml but it is in requirements.toml. Because here we are doing a conversion to sandbox mode applicable to VSCE only for rendering purpose (which does not support ExternalSandbox mode anyway), we will drop this value from the allow list.
| #[ts(export_to = "v2/")] | ||
| pub struct Requirements { | ||
| pub allowed_approval_policies: Option<Vec<AskForApproval>>, | ||
| pub allowed_sandbox_modes: Option<Vec<SandboxMode>>, |
There was a problem hiding this comment.
For now we are not support any additional params for workspace-write because requirements.toml does not support that yet either.
shijie-oai
force-pushed
the
shijie/app-server-requirement-list
branch
from
336891b to
12aa81e
Compare
| async fn handle_requirement_list(&self, request_id: RequestId) { | ||
| match self.config_api.requirement_list().await { | ||
| Ok(response) => self.outgoing.send_response(request_id, response).await, | ||
| Err(error) => self.outgoing.send_error(request_id, error).await, | ||
| } | ||
| } |
There was a problem hiding this comment.
Let me know if folks prefer this in codex_message_processor.
There was a problem hiding this comment.
I also did not use a tokio::spawn because I don't expect this to take long and therefore not blocking.
There was a problem hiding this comment.
fine to put it here alongside the config RPCs, there's a reason for this that I can't quite remember
codex-rs/app-server/README.md
Outdated
| - `config/read` — fetch the effective config on disk after resolving config layering. | ||
| - `config/value/write` — write a single config key/value to the user's config.toml on disk. | ||
| - `config/batchWrite` — apply multiple config edits atomically to the user's config.toml on disk. | ||
| - `requirements/list` — fetch the loaded requirements allow-lists from `requirements.toml` and/or MDM (or `null` if none are configured). |
There was a problem hiding this comment.
This is definitely one valid option. But it would require re-implementing the "is this value selectable" on the front end and corresponding error messages.
An alternate approach could be to implement config/value/canWrite (as a compliment to config/value/write) -- this is how the CLI works at the moment: when you present each value to the user, you first test if that value is acceptable. Then the validation logic is only implemented once. See here:
codex/codex-rs/tui/src/chatwidget.rs
Line 2811 in 124a09e
There was a problem hiding this comment.
This is definitely one valid option. But it would require re-implementing the "is this value selectable" on the front end and corresponding error messages.
I think we are okay with updating the frontend rendering logic.
An alternate approach could be to implement config/value/canWrite (as a compliment to config/value/write) -- this is how the CLI works at the moment: when you present each value to the user, you first test if that value is acceptable. Then the validation logic is only implemented once.
hm this is an interesting approach, instead of getting an allowlist, we ask to see what is allowed on rendering. Also to clarify, we do not hide approval policies in CLI currently if it is not allowed. It does render an error when we attempt to select.
There was a problem hiding this comment.
Chatted offline - I will stick with the current approach for now and we can consider alternative options when there is a need for it.
gt-oai
left a comment
gt-oai
left a comment
There was a problem hiding this comment.
API LGTM assuming we want to keep requirements/list instead of either (1) putting it on ConfigReadResponse or (2) implementing as config/value/canWrite (discussed offline).
You may want an approval from someone who knows more about app-server.
| @@ -446,4 +444,4 @@ | |||
| #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] | |||
| #[serde(rename_all = "camelCase")] | |||
There was a problem hiding this comment.
Should requirements just be a field on this? Rather than a new endpoint?
| #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)] | ||
| #[serde(rename_all = "camelCase")] | ||
| #[ts(export_to = "v2/")] | ||
| pub struct Requirements { |
There was a problem hiding this comment.
nit: rename this ConfigRequirements?
codex-rs/core/src/config/service.rs
Outdated
|
|
||
| let requirements = layers.requirements_toml().clone(); | ||
| if requirements.allowed_approval_policies.is_none() | ||
| && requirements.allowed_sandbox_modes.is_none() |
There was a problem hiding this comment.
might be nice to have an is_empty() helper defined on ConfigRequirementsToml that checks for these fields, otherwise seems easy to miss this code when we add new fields to it
owenlin0
left a comment
owenlin0
left a comment
There was a problem hiding this comment.
small comments but otherwise lgtm
shijie-oai
force-pushed
the
shijie/app-server-requirement-list
branch
from
cc1d4e4 to
f8f33ee
Compare
Summary
We are exposing requirements via
requirement/listmethod from app-server so that we can conditionally disable the agent mode dropdown selection in VSCE and correctly setting the default value.Sample output
etc/codex/requirements.tomlApp server response