-
Notifications
You must be signed in to change notification settings - Fork 7.1k
fix: implement 'Allow this session' for apply_patch approvals #8451
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
5600af9 to
7f9b921
Compare
dde3e77 to
5e96d2b
Compare
|
@codex review |
jif-oai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to work but it looks a bit like reimplementing what is already partially there. We should be able to re-use the exact same workflow as what exist but with just change the keys we use for approval
| Accept, | ||
| /// Approve and remember the approval for the session. | ||
| AcceptForSession, | ||
| Decline, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really want this? How is it different from Cancel? (in any case, add a docstring please)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unfortunately they behave differently and both are used by VSCE. added docstrings
decline - does not abort the turn
cancel - aborts
| } | ||
| } | ||
|
|
||
| fn map_file_change_approval_decision( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic sounds supper hacky but it was already there so I guess out of scope
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
755a8e5 to
39eb1ad
Compare
39eb1ad to
fc1c0fd
Compare
|
@jif-oai Removed the extra store and refactored things a bit so that Approvable / |
|
@codex review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f1afe46d0b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
jif-oai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok after the processing of all my comments
| } else { | ||
| ReviewDecision::Approved | ||
| } | ||
| if let Some(reason) = retry_reason { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why you extract it from the cache?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm I don't follow - this checks whether there's a retry_reason, which based on codex's explanation happens when the tool orchestrator is trying to retry the command outside of the sandbox (dependent on the right approval policy)
| !matches!(policy, AskForApproval::Never) | ||
| } | ||
|
|
||
| fn exec_approval_requirement( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we have an assess_patch_safety check prior to triggering the runtime, and it returns a SafetyCheck::AutoApprove / AskUser / Reject value and we now convert AskUser to ExecApprovalRequirement::NeedsApproval to trigger start_approval_async, which will in turn check the cache.
This is needed now since I moved approval checking from core/src/apply_patch.rs to ApplyPatchRuntime::start_approval_async to be more in-line with the shell and unified_exec tools
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a docstring
178a094 to
df96740
Compare
Summary
This PR makes “ApprovalDecision::AcceptForSession / don’t ask again this session” actually work for
apply_patchapprovals by caching approvals based on absolute file paths in codex-core, properly wiring it through app-server v2, and exposing the choice in both TUI and TUI2.apply_patchcalls to be at feature-parity with general shell commands, which also have a "Yes, and don't ask again" option.While we're at it, also split the app-server v2 protocol's
ApprovalDecisionenum so execpolicy amendments are only available for command execution approvals.Key changes
Update { move_path: Some(...) }.Approvabletrait andApplyPatchRuntimeto work with multiple keys, because anapply_patchtool call can modify multiple files. For a request to be auto-approved, we will need to check that all file paths have been approved previously.ApprovalDecisionwith two enums:CommandExecutionApprovalDecisionandFileChangeApprovalDecisionTests added/updated
User-visible behavior
Manual testing
Tested both TUI and TUI2 - see screenshots below.
TUI:

TUI2:
