feat(core): plumb distinct approval ids for command approvals#12051
feat(core): plumb distinct approval ids for command approvals#12051
Conversation
owenlin0
force-pushed
the
owen/approval_id_plumbing2
branch
from
7809050 to
16cb298
Compare
owenlin0
force-pushed
the
owen/approval_id_plumbing2
branch
from
16cb298 to
9ecd1e9
Compare
|
@codex review |
![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: 9ecd1e99f5
ℹ️ 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".
| let turn = ctx.turn; | ||
| let call_id = ctx.call_id.to_string(); | ||
| Box::pin(async move { | ||
| with_cached_approval(&session.services, "shell", keys, move || async move { |
There was a problem hiding this comment.
Maybe do this here for clarity:
let approval_id = call_id.clone();
Alternatively, it looks like request_command_approval() is close to needing a struct as a param?
| /// The request is keyed by `approval_id` so matching responses are delivered | ||
| /// to the correct in-flight turn. If the task is aborted, this returns the | ||
| /// default `ReviewDecision` (`Denied`). | ||
| #[allow(clippy::too_many_arguments)] |
There was a problem hiding this comment.
Maybe it's time? It's too easy to accidentally switch call_id and approval_id, IMHO. Maybe in the next PR if not this one?
owenlin0
force-pushed
the
owen/approval_id_plumbing2
branch
4 times, most recently
from
c538ea4 to
77e1e90
Compare
| #[ts(optional = nullable)] | ||
| pub reason: Option<String>, | ||
| /// The command to be executed. | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] |
There was a problem hiding this comment.
@owenlin0 some, but not all, of these have is_none
There was a problem hiding this comment.
updated to make it consistent
owenlin0
force-pushed
the
owen/approval_id_plumbing2
branch
6 times, most recently
from
2a312b7 to
bfcf41b
Compare
owenlin0
force-pushed
the
owen/approval_id_plumbing2
branch
from
bfcf41b to
f3e0244
Compare
2 participants
zsh fork PR stack:
With upcoming support for a fork of zsh that allows us to intercept
execveand run execpolicy checks for each subcommand as part of aCommandExecution, it will be possible for there to be multiple approval requests for a shell command like/path/to/zsh -lc 'git status && rg \"TODO\" src && make test'.To support that, this PR introduces a new
approval_idfield across core, protocol, and app-server so that we can associate approvals properly for subcommands.