feat: add Vec<ParsedCommand> to ExecApprovalRequestEvent #5222
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 👍.
| /// Optional human-readable reason for the approval (e.g. retry without sandbox). | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub reason: Option<String>, | ||
| pub parsed_cmd: Vec<ParsedCommand>, |
There was a problem hiding this comment.
parsed_cmd field backward compatible
The new parsed_cmd: Vec<ParsedCommand> field is required when deserializing ExecApprovalRequestEvent. Any log replay or client compiled against an older protocol that doesn’t emit this field will now fail to deserialize these events entirely. We already have compatibility shims for other fields (e.g. formatted_output on exec events) in the TUI tests, so this will regress loading existing session logs or interop with older agents. Consider marking the field with #[serde(default)] (or making it Option<Vec<…>>) so missing data defaults to an empty vector rather than causing a hard parse error.
Useful? React with 👍 / 👎.
Note these two types were identical, so it seems clear to standardize on the one in `codex_protocol` and eliminate the `Into` stuff. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/5218). * #5222 * __->__ #5218
| /// Optional human-readable reason for the approval (e.g. retry without sandbox). | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub reason: Option<String>, | ||
| pub parsed_cmd: Vec<ParsedCommand>, |
There was a problem hiding this comment.
Can we make the entire field be ParsedCmd and move the vec inside?
There was a problem hiding this comment.
@pakrym-oai Are you suggesting something like:
struct ParsedCmd {
pub parsed_cmd: Vec<ParsedCommand>,
}but then should ExecCommandBeginEvent change, as well? (And this has to change throughout?)
There was a problem hiding this comment.
Something like:
struct ParsedCmd {
pub components: Vec<ParsedCommandComponent>,
}to give us space for later expansion
There was a problem hiding this comment.
OK, will address this in a follow-up PR.
This adds
parsed_cmd: Vec<ParsedCommand>toExecApprovalRequestEventin the core protocol (protocol/src/protocol.rs), which is also what this field is named onExecCommandBeginEvent. Honestly, I don't love the name (it sounds like a single command, but it is actually a list of them), but I don't want to get distracted by a naming discussion right now.This also adds
parsed_cmdtoExecCommandApprovalParamsincodex-rs/app-server-protocol/src/protocol.rs, so it will be available viacodex app-server, as well.For consistency, I also updated
ExecApprovalElicitRequestParamsincodex-rs/mcp-server/src/exec_approval.rsto include this field under the namecodex_parsed_cmd, as that struct already has a number of specialcodex_*fields. Note this is the code for when Codex is used as an MCP server and therefore has to conform to the official spec for an MCP elicitation type.