clone/codex
1
0
Fork 0
mirror of https://github.com/openai/codex.git synced 2026-06-01 19:02:59 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: implement 'Allow this session' for apply_patch approvals (#8451)

Browse Source 
**Summary**
This PR makes “ApprovalDecision::AcceptForSession / don’t ask again this
session” actually work for `apply_patch` approvals 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.
- This brings `apply_patch` calls to be at feature-parity with general
shell commands, which also have a "Yes, and don't ask again" option.
- This also fixes VSCE's "Allow this session" button to actually work.

While we're at it, also split the app-server v2 protocol's
`ApprovalDecision` enum so execpolicy amendments are only available for
command execution approvals.

**Key changes**
- Core: per-session patch approval allowlist keyed by absolute file
paths
- Handles multi-file patches and renames/moves by recording both source
and destination paths for `Update { move_path: Some(...) }`.
- Extend the `Approvable` trait and `ApplyPatchRuntime` to work with
multiple keys, because an `apply_patch` tool 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.
- App-server v2: honor AcceptForSession for file changes
- File-change approval responses now map AcceptForSession to
ReviewDecision::ApprovedForSession (no longer downgraded to plain
Approved).
- Replace `ApprovalDecision` with two enums:
`CommandExecutionApprovalDecision` and `FileChangeApprovalDecision`
- TUI / TUI2: expose “don’t ask again for these files this session”
- Patch approval overlays now include a third option (“Yes, and don’t
ask again for these files this session (s)”).
    - Snapshot updates for the approval modal.

**Tests added/updated**
- Core:
- Integration test that proves ApprovedForSession on a patch skips the
next patch prompt for the same file
- App-server:
- v2 integration test verifying
FileChangeApprovalDecision::AcceptForSession works properly

**User-visible behavior**
- When the user approves a patch “for session”, future patches touching
only those previously approved file(s) will no longer prompt gain during
that session (both via app-server v2 and TUI/TUI2).

**Manual testing**
Tested both TUI and TUI2 - see screenshots below.

TUI:
<img width="1082" height="355" alt="image"
src="https://github.com/user-attachments/assets/adcf45ad-d428-498d-92fc-1a0a420878d9"
/>


TUI2:
<img width="1089" height="438" alt="image"
src="https://github.com/user-attachments/assets/dd768b1a-2f5f-4bd6-98fd-e52c1d3abd9e"
/>
This commit is contained in:
Owen Lin
2026-01-07 12:11:12 -08:00
committed by GitHub
parent e8421c761c
commit 66450f0445
19 changed files with 576 additions and 149 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
codex-rs/core/src/tools/events.rs
View File

@@ -305,7 +305,12 @@ impl ToolEmitter {
// Normalize common rejection messages for exec tools so tests and
// users see a clear, consistent phrase.
let normalized = if msg == "rejected by user" {
"exec command rejected by user".to_string()
match self {
Self::Shell { .. } | Self::UnifiedExec { .. } => {
"exec command rejected by user".to_string()
}
Self::ApplyPatch { .. } => "patch rejected by user".to_string(),
}
} else {
msg
};

94
codex-rs/core/src/tools/handlers/apply_patch.rs
View File

@@ -26,11 +26,38 @@ use crate::tools::sandboxing::ToolCtx;
use crate::tools::spec::ApplyPatchToolArgs;
use crate::tools::spec::JsonSchema;
use async_trait::async_trait;
use codex_apply_patch::ApplyPatchAction;
use codex_apply_patch::ApplyPatchFileChange;
use codex_utils_absolute_path::AbsolutePathBuf;
pub struct ApplyPatchHandler;
const APPLY_PATCH_LARK_GRAMMAR: &str = include_str!("tool_apply_patch.lark");
fn file_paths_for_action(action: &ApplyPatchAction) -> Vec<AbsolutePathBuf> {
let mut keys = Vec::new();
let cwd = action.cwd.as_path();
for (path, change) in action.changes() {
if let Some(key) = to_abs_path(cwd, path) {
keys.push(key);
}
if let ApplyPatchFileChange::Update { move_path, .. } = change
&& let Some(dest) = move_path
&& let Some(key) = to_abs_path(cwd, dest)
{
keys.push(key);
}
}
keys
}
fn to_abs_path(cwd: &Path, path: &Path) -> Option<AbsolutePathBuf> {
AbsolutePathBuf::resolve_path_against_base(path, cwd).ok()
}
#[async_trait]
impl ToolHandler for ApplyPatchHandler {
fn kind(&self) -> ToolKind {
@@ -81,9 +108,7 @@ impl ToolHandler for ApplyPatchHandler {
let command = vec!["apply_patch".to_string(), patch_input.clone()];
match codex_apply_patch::maybe_parse_apply_patch_verified(&command, &cwd) {
codex_apply_patch::MaybeApplyPatchVerified::Body(changes) => {
match apply_patch::apply_patch(session.as_ref(), turn.as_ref(), &call_id, changes)
.await
{
match apply_patch::apply_patch(turn.as_ref(), changes).await {
InternalApplyPatchInvocation::Output(item) => {
let content = item?;
Ok(ToolOutput::Function {
@@ -93,10 +118,10 @@ impl ToolHandler for ApplyPatchHandler {
})
}
InternalApplyPatchInvocation::DelegateToExec(apply) => {
let emitter = ToolEmitter::apply_patch(
convert_apply_patch_to_protocol(&apply.action),
!apply.user_explicitly_approved_this_action,
);
let changes = convert_apply_patch_to_protocol(&apply.action);
let file_paths = file_paths_for_action(&apply.action);
let emitter =
ToolEmitter::apply_patch(changes.clone(), apply.auto_approved);
let event_ctx = ToolEventCtx::new(
session.as_ref(),
turn.as_ref(),
@@ -106,10 +131,11 @@ impl ToolHandler for ApplyPatchHandler {
emitter.begin(event_ctx).await;
let req = ApplyPatchRequest {
patch: apply.action.patch.clone(),
cwd: apply.action.cwd.clone(),
action: apply.action,
file_paths,
changes,
exec_approval_requirement: apply.exec_approval_requirement,
timeout_ms: None,
user_explicitly_approved: apply.user_explicitly_approved_this_action,
codex_exe: turn.codex_linux_sandbox_exe.clone(),
};
@@ -178,7 +204,7 @@ pub(crate) async fn intercept_apply_patch(
turn,
)
.await;
match apply_patch::apply_patch(session, turn, call_id, changes).await {
match apply_patch::apply_patch(turn, changes).await {
InternalApplyPatchInvocation::Output(item) => {
let content = item?;
Ok(Some(ToolOutput::Function {
@@ -188,19 +214,19 @@ pub(crate) async fn intercept_apply_patch(
}))
}
InternalApplyPatchInvocation::DelegateToExec(apply) => {
let emitter = ToolEmitter::apply_patch(
convert_apply_patch_to_protocol(&apply.action),
!apply.user_explicitly_approved_this_action,
);
let changes = convert_apply_patch_to_protocol(&apply.action);
let approval_keys = file_paths_for_action(&apply.action);
let emitter = ToolEmitter::apply_patch(changes.clone(), apply.auto_approved);
let event_ctx =
ToolEventCtx::new(session, turn, call_id, tracker.as_ref().copied());
emitter.begin(event_ctx).await;
let req = ApplyPatchRequest {
patch: apply.action.patch.clone(),
cwd: apply.action.cwd.clone(),
action: apply.action,
file_paths: approval_keys,
changes,
exec_approval_requirement: apply.exec_approval_requirement,
timeout_ms,
user_explicitly_approved: apply.user_explicitly_approved_this_action,
codex_exe: turn.codex_linux_sandbox_exe.clone(),
};
@@ -342,3 +368,35 @@ It is important to remember:
},
})
}
#[cfg(test)]
mod tests {
use super::*;
use codex_apply_patch::MaybeApplyPatchVerified;
use pretty_assertions::assert_eq;
use tempfile::TempDir;
#[test]
fn approval_keys_include_move_destination() {
let tmp = TempDir::new().expect("tmp");
let cwd = tmp.path();
std::fs::create_dir_all(cwd.join("old")).expect("create old dir");
std::fs::create_dir_all(cwd.join("renamed/dir")).expect("create dest dir");
std::fs::write(cwd.join("old/name.txt"), "old content\n").expect("write old file");
let patch = r#"*** Begin Patch
*** Update File: old/name.txt
*** Move to: renamed/dir/name.txt
@@
-old content
+new content
*** End Patch"#;
let argv = vec!["apply_patch".to_string(), patch.to_string()];
let action = match codex_apply_patch::maybe_parse_apply_patch_verified(&argv, cwd) {
MaybeApplyPatchVerified::Body(action) => action,
other => panic!("expected patch body, got: {other:?}"),
};
let keys = file_paths_for_action(&action);
assert_eq!(keys.len(), 2);
}
}

77
codex-rs/core/src/tools/runtimes/apply_patch.rs
View File

@@ -11,6 +11,7 @@ use crate::sandboxing::SandboxPermissions;
use crate::sandboxing::execute_env;
use crate::tools::sandboxing::Approvable;
use crate::tools::sandboxing::ApprovalCtx;
use crate::tools::sandboxing::ExecApprovalRequirement;
use crate::tools::sandboxing::SandboxAttempt;
use crate::tools::sandboxing::Sandboxable;
use crate::tools::sandboxing::SandboxablePreference;
@@ -18,30 +19,28 @@ use crate::tools::sandboxing::ToolCtx;
use crate::tools::sandboxing::ToolError;
use crate::tools::sandboxing::ToolRuntime;
use crate::tools::sandboxing::with_cached_approval;
use codex_apply_patch::ApplyPatchAction;
use codex_protocol::protocol::AskForApproval;
use codex_protocol::protocol::FileChange;
use codex_protocol::protocol::ReviewDecision;
use codex_utils_absolute_path::AbsolutePathBuf;
use futures::future::BoxFuture;
use std::collections::HashMap;
use std::path::PathBuf;
#[derive(Clone, Debug)]
#[derive(Debug)]
pub struct ApplyPatchRequest {
pub patch: String,
pub cwd: PathBuf,
pub action: ApplyPatchAction,
pub file_paths: Vec<AbsolutePathBuf>,
pub changes: std::collections::HashMap<PathBuf, FileChange>,
pub exec_approval_requirement: ExecApprovalRequirement,
pub timeout_ms: Option<u64>,
pub user_explicitly_approved: bool,
pub codex_exe: Option<PathBuf>,
}
#[derive(Default)]
pub struct ApplyPatchRuntime;
#[derive(serde::Serialize, Clone, Debug, Eq, PartialEq, Hash)]
pub(crate) struct ApprovalKey {
patch: String,
cwd: PathBuf,
}
impl ApplyPatchRuntime {
pub fn new() -> Self {
Self
@@ -58,8 +57,8 @@ impl ApplyPatchRuntime {
let program = exe.to_string_lossy().to_string();
Ok(CommandSpec {
program,
args: vec![CODEX_APPLY_PATCH_ARG1.to_string(), req.patch.clone()],
cwd: req.cwd.clone(),
args: vec![CODEX_APPLY_PATCH_ARG1.to_string(), req.action.patch.clone()],
cwd: req.action.cwd.clone(),
expiration: req.timeout_ms.into(),
// Run apply_patch with a minimal environment for determinism and to avoid leaks.
env: HashMap::new(),
@@ -87,13 +86,10 @@ impl Sandboxable for ApplyPatchRuntime {
}
impl Approvable<ApplyPatchRequest> for ApplyPatchRuntime {
type ApprovalKey = ApprovalKey;
type ApprovalKey = AbsolutePathBuf;
fn approval_key(&self, req: &ApplyPatchRequest) -> Self::ApprovalKey {
ApprovalKey {
patch: req.patch.clone(),
cwd: req.cwd.clone(),
}
fn approval_keys(&self, req: &ApplyPatchRequest) -> Vec<Self::ApprovalKey> {
req.file_paths.clone()
}
fn start_approval_async<'a>(
@@ -101,31 +97,25 @@ impl Approvable<ApplyPatchRequest> for ApplyPatchRuntime {
req: &'a ApplyPatchRequest,
ctx: ApprovalCtx<'a>,
) -> BoxFuture<'a, ReviewDecision> {
let key = self.approval_key(req);
let session = ctx.session;
let turn = ctx.turn;
let call_id = ctx.call_id.to_string();
let cwd = req.cwd.clone();
let retry_reason = ctx.retry_reason.clone();
let user_explicitly_approved = req.user_explicitly_approved;
let approval_keys = self.approval_keys(req);
let changes = req.changes.clone();
Box::pin(async move {
with_cached_approval(&session.services, key, move || async move {
if let Some(reason) = retry_reason {
session
.request_command_approval(
turn,
call_id,
vec!["apply_patch".to_string()],
cwd,
Some(reason),
None,
)
.await
} else if user_explicitly_approved {
ReviewDecision::ApprovedForSession
} else {
ReviewDecision::Approved
}
if let Some(reason) = retry_reason {
let rx_approve = session
.request_patch_approval(turn, call_id, changes.clone(), Some(reason), None)
.await;
return rx_approve.await.unwrap_or_default();
}
with_cached_approval(&session.services, approval_keys, || async move {
let rx_approve = session
.request_patch_approval(turn, call_id, changes, None, None)
.await;
rx_approve.await.unwrap_or_default()
})
.await
})
@@ -134,6 +124,17 @@ impl Approvable<ApplyPatchRequest> for ApplyPatchRuntime {
fn wants_no_sandbox_approval(&self, policy: AskForApproval) -> bool {
!matches!(policy, AskForApproval::Never)
}
// apply_patch approvals are decided upstream by assess_patch_safety.
//
// This override ensures the orchestrator runs the patch approval flow when required instead
// of falling back to the global exec approval policy.
fn exec_approval_requirement(
&self,
req: &ApplyPatchRequest,
) -> Option<ExecApprovalRequirement> {
Some(req.exec_approval_requirement.clone())
}
}
impl ToolRuntime<ApplyPatchRequest, ExecToolCallOutput> for ApplyPatchRuntime {

10
codex-rs/core/src/tools/runtimes/shell.rs
View File

@@ -74,12 +74,12 @@ impl Sandboxable for ShellRuntime {
impl Approvable<ShellRequest> for ShellRuntime {
type ApprovalKey = ApprovalKey;
fn approval_key(&self, req: &ShellRequest) -> Self::ApprovalKey {
ApprovalKey {
fn approval_keys(&self, req: &ShellRequest) -> Vec<Self::ApprovalKey> {
vec![ApprovalKey {
command: req.command.clone(),
cwd: req.cwd.clone(),
sandbox_permissions: req.sandbox_permissions,
}
}]
}
fn start_approval_async<'a>(
@@ -87,7 +87,7 @@ impl Approvable<ShellRequest> for ShellRuntime {
req: &'a ShellRequest,
ctx: ApprovalCtx<'a>,
) -> BoxFuture<'a, ReviewDecision> {
let key = self.approval_key(req);
let keys = self.approval_keys(req);
let command = req.command.clone();
let cwd = req.cwd.clone();
let reason = ctx
@@ -98,7 +98,7 @@ impl Approvable<ShellRequest> for ShellRuntime {
let turn = ctx.turn;
let call_id = ctx.call_id.to_string();
Box::pin(async move {
with_cached_approval(&session.services, key, move || async move {
with_cached_approval(&session.services, keys, move || async move {
session
.request_command_approval(
turn,

10
codex-rs/core/src/tools/runtimes/unified_exec.rs
View File

@@ -92,12 +92,12 @@ impl Sandboxable for UnifiedExecRuntime<'_> {
impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
type ApprovalKey = UnifiedExecApprovalKey;
fn approval_key(&self, req: &UnifiedExecRequest) -> Self::ApprovalKey {
UnifiedExecApprovalKey {
fn approval_keys(&self, req: &UnifiedExecRequest) -> Vec<Self::ApprovalKey> {
vec![UnifiedExecApprovalKey {
command: req.command.clone(),
cwd: req.cwd.clone(),
sandbox_permissions: req.sandbox_permissions,
}
}]
}
fn start_approval_async<'b>(
@@ -105,7 +105,7 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
req: &'b UnifiedExecRequest,
ctx: ApprovalCtx<'b>,
) -> BoxFuture<'b, ReviewDecision> {
let key = self.approval_key(req);
let keys = self.approval_keys(req);
let session = ctx.session;
let turn = ctx.turn;
let call_id = ctx.call_id.to_string();
@@ -116,7 +116,7 @@ impl Approvable<UnifiedExecRequest> for UnifiedExecRuntime<'_> {
.clone()
.or_else(|| req.justification.clone());
Box::pin(async move {
with_cached_approval(&session.services, key, || async move {
with_cached_approval(&session.services, keys, || async move {
session
.request_command_approval(
turn,

39
codex-rs/core/src/tools/sandboxing.rs
View File

@@ -49,28 +49,44 @@ impl ApprovalStore {
}
}
/// Takes a vector of approval keys and returns a ReviewDecision.
/// There will be one key in most cases, but apply_patch can modify multiple files at once.
///
/// - If all keys are already approved for session, we skip prompting.
/// - If the user approves for session, we store the decision for each key individually
/// so future requests touching any subset can also skip prompting.
pub(crate) async fn with_cached_approval<K, F, Fut>(
services: &SessionServices,
key: K,
keys: Vec<K>,
fetch: F,
) -> ReviewDecision
where
K: Serialize + Clone,
K: Serialize,
F: FnOnce() -> Fut,
Fut: Future<Output = ReviewDecision>,
{
{
// To be defensive here, don't bother with checking the cache if keys are empty.
if keys.is_empty() {
return fetch().await;
}
let already_approved = {
let store = services.tool_approvals.lock().await;
if let Some(decision) = store.get(&key) {
return decision;
}
keys.iter()
.all(|key| matches!(store.get(key), Some(ReviewDecision::ApprovedForSession)))
};
if already_approved {
return ReviewDecision::ApprovedForSession;
}
let decision = fetch().await;
if matches!(decision, ReviewDecision::ApprovedForSession) {
let mut store = services.tool_approvals.lock().await;
store.put(key, ReviewDecision::ApprovedForSession);
for key in keys {
store.put(key, ReviewDecision::ApprovedForSession);
}
}
decision
@@ -161,7 +177,14 @@ pub(crate) enum SandboxOverride {
pub(crate) trait Approvable<Req> {
type ApprovalKey: Hash + Eq + Clone + Debug + Serialize;
fn approval_key(&self, req: &Req) -> Self::ApprovalKey;
// In most cases (shell, unified_exec), a request will have a single approval key.
//
// However, apply_patch needs session "approve once, don't ask again" semantics that
// apply to multiple atomic targets (e.g., apply_patch approves per file path). Returning
// a list of keys lets the runtime treat the request as approved-for-session only if
// *all* keys are already approved, while still caching approvals per-key so future
// requests touching a subset can be auto-approved.
fn approval_keys(&self, req: &Req) -> Vec<Self::ApprovalKey>;
/// Some tools may request to skip the sandbox on the first attempt
/// (e.g., when the request explicitly asks for escalated permissions).