Chore: add cmd related info to exec approval request#9659
Conversation
shijie-oai
requested review from
aibrahim-oai,
dylan-hurd-oai,
jif-oai,
owenlin0 and
pakrym-oai
![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.
Reviewed commit: 6779e07d03
ℹ️ 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".
| /// Best-effort parsed command actions for friendly display. | ||
| #[serde(default, skip_serializing_if = "Vec::is_empty")] | ||
| pub command_actions: Vec<CommandAction>, |
There was a problem hiding this comment.
Because command_actions is skipped when empty (skip_serializing_if = "Vec::is_empty") but it is not marked optional in the TS export, clients using the generated TypeScript types will treat commandActions as required even though the server will omit it whenever parsing yields no actions. That mismatch can lead to undefined at runtime for callers who follow the types and access commandActions unconditionally. Consider either removing the skip-serialize so the field is always present (possibly empty) or adding #[ts(optional)] and handling the missing field on the client side.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
why do we want to skip if it's empty?
There was a problem hiding this comment.
Updated - but essentially we will get undefined instead of an empty array on ts side. Visually it would be the same (i.e. no exec command rendered) but with undefined it is a better indicator that we have nothing to render.
There was a problem hiding this comment.
Yeah the first part makes sense - just double checking that empty array and undefined are always intended to be identical here. given we'd fallback instead, this sounds good to me.
shijie-oai
force-pushed
the
shijie/add-cmd-to-exec-approval-request
branch
from
6779e07 to
67ff933
Compare
dylan-hurd-oai
left a comment
dylan-hurd-oai
left a comment
There was a problem hiding this comment.
Curious about the is_empty decision but not blocking, excited for this!
| /// Best-effort parsed command actions for friendly display. | ||
| #[serde(default, skip_serializing_if = "Vec::is_empty")] | ||
| pub command_actions: Vec<CommandAction>, |
There was a problem hiding this comment.
why do we want to skip if it's empty?
shijie-oai
force-pushed
the
shijie/add-cmd-to-exec-approval-request
branch
from
67ff933 to
b9a19a0
Compare
shijie-oai
force-pushed
the
shijie/add-cmd-to-exec-approval-request
branch
from
b9a19a0 to
b56226f
Compare
2 participants
Summary
We now rely purely on
item/commandExecution/requestApprovalitem to render pending approval in VSCE and app. With v2 approach, it does not include the actual cmd that it is attempting and therefore we can only useproposedExecpolicyAmendmentto render which can be incomplete.Reproduce
prefix_rule(pattern=["echo"], decision="prompt")to your~/.codex/rules.default.rules.Run echo "approval-test" pleasein VSCE or app.Example screenshot
Sample output
Fix
Inlude
commandstring,cwdandcommand_actionsinCommandExecutionRequestApprovalParamsso that consumers can display the correct command instead of relying on exec policy output.