feat(core) Persist request_permission data across turns (#14009)
## Summary request_permissions flows should support persisting results for the session. Open Question: Still deciding if we need within-turn approvals - this adds complexity but I could see it being useful ## Testing - [x] Updated unit tests --------- Co-authored-by: Codex <noreply@openai.com>
This commit is contained in:
parent
831ee51c86
commit
d241dc598c
21 changed files with 638 additions and 8 deletions
|
|
@ -145,11 +145,26 @@
|
|||
"read_write"
|
||||
],
|
||||
"type": "string"
|
||||
},
|
||||
"PermissionGrantScope": {
|
||||
"enum": [
|
||||
"turn",
|
||||
"session"
|
||||
],
|
||||
"type": "string"
|
||||
}
|
||||
},
|
||||
"properties": {
|
||||
"permissions": {
|
||||
"$ref": "#/definitions/GrantedPermissionProfile"
|
||||
},
|
||||
"scope": {
|
||||
"allOf": [
|
||||
{
|
||||
"$ref": "#/definitions/PermissionGrantScope"
|
||||
}
|
||||
],
|
||||
"default": "turn"
|
||||
}
|
||||
},
|
||||
"required": [
|
||||
|
|
|
|||
|
|
@ -6462,6 +6462,13 @@
|
|||
}
|
||||
]
|
||||
},
|
||||
"PermissionGrantScope": {
|
||||
"enum": [
|
||||
"turn",
|
||||
"session"
|
||||
],
|
||||
"type": "string"
|
||||
},
|
||||
"PermissionProfile": {
|
||||
"properties": {
|
||||
"file_system": {
|
||||
|
|
@ -6533,6 +6540,14 @@
|
|||
"properties": {
|
||||
"permissions": {
|
||||
"$ref": "#/definitions/GrantedPermissionProfile"
|
||||
},
|
||||
"scope": {
|
||||
"allOf": [
|
||||
{
|
||||
"$ref": "#/definitions/PermissionGrantScope"
|
||||
}
|
||||
],
|
||||
"default": "turn"
|
||||
}
|
||||
},
|
||||
"required": [
|
||||
|
|
|
|||
|
|
@ -0,0 +1,5 @@
|
|||
// GENERATED CODE! DO NOT MODIFY BY HAND!
|
||||
|
||||
// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually.
|
||||
|
||||
export type PermissionGrantScope = "turn" | "session";
|
||||
|
|
@ -2,5 +2,6 @@
|
|||
|
||||
// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually.
|
||||
import type { GrantedPermissionProfile } from "./GrantedPermissionProfile";
|
||||
import type { PermissionGrantScope } from "./PermissionGrantScope";
|
||||
|
||||
export type PermissionsRequestApprovalResponse = { permissions: GrantedPermissionProfile, };
|
||||
export type PermissionsRequestApprovalResponse = { permissions: GrantedPermissionProfile, scope: PermissionGrantScope, };
|
||||
|
|
|
|||
|
|
@ -161,6 +161,7 @@ export type { NetworkRequirements } from "./NetworkRequirements";
|
|||
export type { OverriddenMetadata } from "./OverriddenMetadata";
|
||||
export type { PatchApplyStatus } from "./PatchApplyStatus";
|
||||
export type { PatchChangeKind } from "./PatchChangeKind";
|
||||
export type { PermissionGrantScope } from "./PermissionGrantScope";
|
||||
export type { PermissionsRequestApprovalParams } from "./PermissionsRequestApprovalParams";
|
||||
export type { PermissionsRequestApprovalResponse } from "./PermissionsRequestApprovalResponse";
|
||||
export type { PlanDeltaNotification } from "./PlanDeltaNotification";
|
||||
|
|
|
|||
|
|
@ -68,6 +68,7 @@ use codex_protocol::protocol::SkillToolDependency as CoreSkillToolDependency;
|
|||
use codex_protocol::protocol::SubAgentSource as CoreSubAgentSource;
|
||||
use codex_protocol::protocol::TokenUsage as CoreTokenUsage;
|
||||
use codex_protocol::protocol::TokenUsageInfo as CoreTokenUsageInfo;
|
||||
use codex_protocol::request_permissions::PermissionGrantScope as CorePermissionGrantScope;
|
||||
use codex_protocol::user_input::ByteRange as CoreByteRange;
|
||||
use codex_protocol::user_input::TextElement as CoreTextElement;
|
||||
use codex_protocol::user_input::UserInput as CoreUserInput;
|
||||
|
|
@ -83,12 +84,18 @@ use ts_rs::TS;
|
|||
// tends to use either snake_case or kebab-case.
|
||||
macro_rules! v2_enum_from_core {
|
||||
(
|
||||
pub enum $Name:ident from $Src:path { $( $Variant:ident ),+ $(,)? }
|
||||
$(#[$enum_meta:meta])*
|
||||
pub enum $Name:ident from $Src:path {
|
||||
$( $(#[$variant_meta:meta])* $Variant:ident ),+ $(,)?
|
||||
}
|
||||
) => {
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, Copy, PartialEq, Eq, JsonSchema, TS)]
|
||||
$(#[$enum_meta])*
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[ts(export_to = "v2/")]
|
||||
pub enum $Name { $( $Variant ),+ }
|
||||
pub enum $Name {
|
||||
$( $(#[$variant_meta])* $Variant ),+
|
||||
}
|
||||
|
||||
impl $Name {
|
||||
pub fn to_core(self) -> $Src {
|
||||
|
|
@ -4983,11 +4990,22 @@ pub struct PermissionsRequestApprovalParams {
|
|||
pub permissions: AdditionalPermissionProfile,
|
||||
}
|
||||
|
||||
v2_enum_from_core!(
|
||||
#[derive(Default)]
|
||||
pub enum PermissionGrantScope from CorePermissionGrantScope {
|
||||
#[default]
|
||||
Turn,
|
||||
Session
|
||||
}
|
||||
);
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)]
|
||||
#[serde(rename_all = "camelCase")]
|
||||
#[ts(export_to = "v2/")]
|
||||
pub struct PermissionsRequestApprovalResponse {
|
||||
pub permissions: GrantedPermissionProfile,
|
||||
#[serde(default)]
|
||||
pub scope: PermissionGrantScope,
|
||||
}
|
||||
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)]
|
||||
|
|
@ -5449,6 +5467,7 @@ mod tests {
|
|||
}),
|
||||
..Default::default()
|
||||
},
|
||||
scope: PermissionGrantScope::Turn,
|
||||
};
|
||||
|
||||
assert_eq!(
|
||||
|
|
@ -5459,10 +5478,21 @@ mod tests {
|
|||
"accessibility": true,
|
||||
},
|
||||
},
|
||||
"scope": "turn",
|
||||
})
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn permissions_request_approval_response_defaults_scope_to_turn() {
|
||||
let response = serde_json::from_value::<PermissionsRequestApprovalResponse>(json!({
|
||||
"permissions": {},
|
||||
}))
|
||||
.expect("response should deserialize");
|
||||
|
||||
assert_eq!(response.scope, PermissionGrantScope::Turn);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn command_exec_params_default_optional_streaming_flags() {
|
||||
let params = serde_json::from_value::<CommandExecParams>(json!({
|
||||
|
|
|
|||
|
|
@ -901,12 +901,13 @@ The built-in `request_permissions` tool sends an `item/permissions/requestApprov
|
|||
}
|
||||
```
|
||||
|
||||
The client responds with `result.permissions`, which should be the granted subset of the requested permission profile:
|
||||
The client responds with `result.permissions`, which should be the granted subset of the requested permission profile. It may also set `result.scope` to `"session"` to make the grant persist for later turns in the same session; omitted or `"turn"` keeps the existing turn-scoped behavior:
|
||||
|
||||
```json
|
||||
{
|
||||
"id": 61,
|
||||
"result": {
|
||||
"scope": "session",
|
||||
"permissions": {
|
||||
"fileSystem": {
|
||||
"write": [
|
||||
|
|
|
|||
|
|
@ -120,6 +120,7 @@ use codex_protocol::protocol::ReviewDecision;
|
|||
use codex_protocol::protocol::ReviewOutputEvent;
|
||||
use codex_protocol::protocol::TokenCountEvent;
|
||||
use codex_protocol::protocol::TurnDiffEvent;
|
||||
use codex_protocol::request_permissions::PermissionGrantScope as CorePermissionGrantScope;
|
||||
use codex_protocol::request_permissions::RequestPermissionsResponse as CoreRequestPermissionsResponse;
|
||||
use codex_protocol::request_user_input::RequestUserInputAnswer as CoreRequestUserInputAnswer;
|
||||
use codex_protocol::request_user_input::RequestUserInputResponse as CoreRequestUserInputResponse;
|
||||
|
|
@ -718,6 +719,7 @@ pub(crate) async fn apply_bespoke_event_handling(
|
|||
);
|
||||
let empty = CoreRequestPermissionsResponse {
|
||||
permissions: Default::default(),
|
||||
scope: CorePermissionGrantScope::Turn,
|
||||
};
|
||||
if let Err(err) = conversation
|
||||
.submit(Op::RequestPermissionsResponse {
|
||||
|
|
@ -2219,12 +2221,14 @@ fn request_permissions_response_from_client_result(
|
|||
error!("request failed with client error: {err:?}");
|
||||
return Some(CoreRequestPermissionsResponse {
|
||||
permissions: Default::default(),
|
||||
scope: CorePermissionGrantScope::Turn,
|
||||
});
|
||||
}
|
||||
Err(err) => {
|
||||
error!("request failed: {err:?}");
|
||||
return Some(CoreRequestPermissionsResponse {
|
||||
permissions: Default::default(),
|
||||
scope: CorePermissionGrantScope::Turn,
|
||||
});
|
||||
}
|
||||
};
|
||||
|
|
@ -2234,6 +2238,7 @@ fn request_permissions_response_from_client_result(
|
|||
error!("failed to deserialize PermissionsRequestApprovalResponse: {err}");
|
||||
PermissionsRequestApprovalResponse {
|
||||
permissions: V2GrantedPermissionProfile::default(),
|
||||
scope: codex_app_server_protocol::PermissionGrantScope::Turn,
|
||||
}
|
||||
});
|
||||
Some(CoreRequestPermissionsResponse {
|
||||
|
|
@ -2241,6 +2246,7 @@ fn request_permissions_response_from_client_result(
|
|||
requested_permissions,
|
||||
response.permissions.into(),
|
||||
),
|
||||
scope: response.scope.to_core(),
|
||||
})
|
||||
}
|
||||
|
||||
|
|
@ -2766,11 +2772,32 @@ mod tests {
|
|||
response,
|
||||
CoreRequestPermissionsResponse {
|
||||
permissions: expected_permissions,
|
||||
scope: CorePermissionGrantScope::Turn,
|
||||
}
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn request_permissions_response_preserves_session_scope() {
|
||||
let response = request_permissions_response_from_client_result(
|
||||
CorePermissionProfile::default(),
|
||||
Ok(Ok(serde_json::json!({
|
||||
"scope": "session",
|
||||
"permissions": {},
|
||||
}))),
|
||||
)
|
||||
.expect("response should be accepted");
|
||||
|
||||
assert_eq!(
|
||||
response,
|
||||
CoreRequestPermissionsResponse {
|
||||
permissions: CorePermissionProfile::default(),
|
||||
scope: CorePermissionGrantScope::Session,
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn collab_resume_begin_maps_to_item_started_resume_agent() {
|
||||
let event = CollabResumeBeginEvent {
|
||||
|
|
|
|||
|
|
@ -6,6 +6,7 @@ use app_test_support::create_request_permissions_sse_response;
|
|||
use app_test_support::to_response;
|
||||
use codex_app_server_protocol::JSONRPCMessage;
|
||||
use codex_app_server_protocol::JSONRPCResponse;
|
||||
use codex_app_server_protocol::PermissionGrantScope;
|
||||
use codex_app_server_protocol::PermissionsRequestApprovalResponse;
|
||||
use codex_app_server_protocol::RequestId;
|
||||
use codex_app_server_protocol::ServerRequest;
|
||||
|
|
@ -95,6 +96,7 @@ async fn request_permissions_round_trip() -> Result<()> {
|
|||
}),
|
||||
macos: None,
|
||||
},
|
||||
scope: PermissionGrantScope::Turn,
|
||||
})?,
|
||||
)
|
||||
.await?;
|
||||
|
|
|
|||
|
|
@ -103,6 +103,7 @@ use codex_protocol::protocol::TurnAbortReason;
|
|||
use codex_protocol::protocol::TurnContextItem;
|
||||
use codex_protocol::protocol::TurnContextNetworkItem;
|
||||
use codex_protocol::protocol::TurnStartedEvent;
|
||||
use codex_protocol::request_permissions::PermissionGrantScope;
|
||||
use codex_protocol::request_permissions::RequestPermissionsArgs;
|
||||
use codex_protocol::request_permissions::RequestPermissionsEvent;
|
||||
use codex_protocol::request_permissions::RequestPermissionsResponse;
|
||||
|
|
@ -2996,6 +2997,7 @@ impl Session {
|
|||
call_id: &str,
|
||||
response: RequestPermissionsResponse,
|
||||
) {
|
||||
let mut granted_for_session = None;
|
||||
let entry = {
|
||||
let mut active = self.active_turn.lock().await;
|
||||
match active.as_mut() {
|
||||
|
|
@ -3003,13 +3005,24 @@ impl Session {
|
|||
let mut ts = at.turn_state.lock().await;
|
||||
let entry = ts.remove_pending_request_permissions(call_id);
|
||||
if entry.is_some() && !response.permissions.is_empty() {
|
||||
ts.record_granted_permissions(response.permissions.clone());
|
||||
match response.scope {
|
||||
PermissionGrantScope::Turn => {
|
||||
ts.record_granted_permissions(response.permissions.clone());
|
||||
}
|
||||
PermissionGrantScope::Session => {
|
||||
granted_for_session = Some(response.permissions.clone());
|
||||
}
|
||||
}
|
||||
}
|
||||
entry
|
||||
}
|
||||
None => None,
|
||||
}
|
||||
};
|
||||
if let Some(permissions) = granted_for_session {
|
||||
let mut state = self.state.lock().await;
|
||||
state.record_granted_permissions(permissions);
|
||||
}
|
||||
match entry {
|
||||
Some(tx_response) => {
|
||||
tx_response.send(response).ok();
|
||||
|
|
@ -3027,6 +3040,11 @@ impl Session {
|
|||
ts.granted_permissions()
|
||||
}
|
||||
|
||||
pub(crate) async fn granted_session_permissions(&self) -> Option<PermissionProfile> {
|
||||
let state = self.state.lock().await;
|
||||
state.granted_permissions()
|
||||
}
|
||||
|
||||
pub async fn notify_dynamic_tool_response(&self, call_id: &str, response: DynamicToolResponse) {
|
||||
let entry = {
|
||||
let mut active = self.active_turn.lock().await;
|
||||
|
|
|
|||
|
|
@ -13,6 +13,7 @@ use codex_protocol::protocol::RequestUserInputEvent;
|
|||
use codex_protocol::protocol::SessionSource;
|
||||
use codex_protocol::protocol::SubAgentSource;
|
||||
use codex_protocol::protocol::Submission;
|
||||
use codex_protocol::request_permissions::PermissionGrantScope;
|
||||
use codex_protocol::request_permissions::RequestPermissionsArgs;
|
||||
use codex_protocol::request_permissions::RequestPermissionsEvent;
|
||||
use codex_protocol::request_permissions::RequestPermissionsResponse;
|
||||
|
|
@ -505,6 +506,7 @@ where
|
|||
_ = cancel_token.cancelled() => {
|
||||
let empty = RequestPermissionsResponse {
|
||||
permissions: Default::default(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
};
|
||||
parent_session
|
||||
.notify_request_permissions_response(call_id, empty.clone())
|
||||
|
|
@ -513,6 +515,7 @@ where
|
|||
}
|
||||
response = fut => response.unwrap_or_else(|| RequestPermissionsResponse {
|
||||
permissions: Default::default(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
}),
|
||||
}
|
||||
}
|
||||
|
|
@ -698,6 +701,7 @@ mod tests {
|
|||
}),
|
||||
..PermissionProfile::default()
|
||||
},
|
||||
scope: PermissionGrantScope::Turn,
|
||||
};
|
||||
let cancel_token = CancellationToken::new();
|
||||
let request_call_id = call_id.clone();
|
||||
|
|
|
|||
|
|
@ -17,6 +17,7 @@ use crate::tools::format_exec_output_str;
|
|||
use codex_protocol::ThreadId;
|
||||
use codex_protocol::models::FunctionCallOutputBody;
|
||||
use codex_protocol::models::FunctionCallOutputPayload;
|
||||
use codex_protocol::request_permissions::PermissionGrantScope;
|
||||
use tracing::Span;
|
||||
|
||||
use crate::protocol::CompactedItem;
|
||||
|
|
@ -2156,6 +2157,7 @@ async fn notify_request_permissions_response_ignores_unmatched_call_id() {
|
|||
}),
|
||||
..Default::default()
|
||||
},
|
||||
scope: PermissionGrantScope::Turn,
|
||||
},
|
||||
)
|
||||
.await;
|
||||
|
|
|
|||
|
|
@ -1,5 +1,6 @@
|
|||
//! Session-wide mutable state.
|
||||
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::models::ResponseItem;
|
||||
use std::collections::HashMap;
|
||||
use std::collections::HashSet;
|
||||
|
|
@ -32,6 +33,7 @@ pub(crate) struct SessionState {
|
|||
pub(crate) startup_regular_task: Option<JoinHandle<CodexResult<RegularTask>>>,
|
||||
pub(crate) active_mcp_tool_selection: Option<Vec<String>>,
|
||||
pub(crate) active_connector_selection: HashSet<String>,
|
||||
granted_permissions: Option<PermissionProfile>,
|
||||
}
|
||||
|
||||
impl SessionState {
|
||||
|
|
@ -49,6 +51,7 @@ impl SessionState {
|
|||
startup_regular_task: None,
|
||||
active_mcp_tool_selection: None,
|
||||
active_connector_selection: HashSet::new(),
|
||||
granted_permissions: None,
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -218,6 +221,17 @@ impl SessionState {
|
|||
self.active_mcp_tool_selection = None;
|
||||
}
|
||||
|
||||
pub(crate) fn record_granted_permissions(&mut self, permissions: PermissionProfile) {
|
||||
self.granted_permissions = crate::sandboxing::merge_permission_profiles(
|
||||
self.granted_permissions.as_ref(),
|
||||
Some(&permissions),
|
||||
);
|
||||
}
|
||||
|
||||
pub(crate) fn granted_permissions(&self) -> Option<PermissionProfile> {
|
||||
self.granted_permissions.clone()
|
||||
}
|
||||
|
||||
// Adds connector IDs to the active set and returns the merged selection.
|
||||
pub(crate) fn merge_connector_selection<I>(&mut self, connector_ids: I) -> HashSet<String>
|
||||
where
|
||||
|
|
|
|||
|
|
@ -172,7 +172,12 @@ pub(super) async fn apply_granted_turn_permissions(
|
|||
};
|
||||
}
|
||||
|
||||
let granted_permissions = session.granted_turn_permissions().await;
|
||||
let granted_session_permissions = session.granted_session_permissions().await;
|
||||
let granted_turn_permissions = session.granted_turn_permissions().await;
|
||||
let granted_permissions = merge_permission_profiles(
|
||||
granted_session_permissions.as_ref(),
|
||||
granted_turn_permissions.as_ref(),
|
||||
);
|
||||
let effective_permissions = merge_permission_profiles(
|
||||
additional_permissions.as_ref(),
|
||||
granted_permissions.as_ref(),
|
||||
|
|
|
|||
|
|
@ -12,7 +12,7 @@ use crate::tools::registry::ToolHandler;
|
|||
use crate::tools::registry::ToolKind;
|
||||
|
||||
pub(crate) fn request_permissions_tool_description() -> String {
|
||||
"Request additional permissions from the user and wait for the client to grant a subset of the requested permission profile. Granted permissions apply automatically to later shell-like commands in the current turn."
|
||||
"Request additional permissions from the user and wait for the client to grant a subset of the requested permission profile. Granted permissions apply automatically to later shell-like commands in the current turn, or for the rest of the session if the client approves them at session scope."
|
||||
.to_string()
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -104,6 +104,8 @@ mod remote_models;
|
|||
mod request_compression;
|
||||
#[cfg(not(target_os = "windows"))]
|
||||
mod request_permissions;
|
||||
#[cfg(not(target_os = "windows"))]
|
||||
mod request_permissions_tool;
|
||||
mod request_user_input;
|
||||
mod resume;
|
||||
mod resume_warning;
|
||||
|
|
|
|||
|
|
@ -12,6 +12,7 @@ use codex_protocol::protocol::ExecApprovalRequestEvent;
|
|||
use codex_protocol::protocol::Op;
|
||||
use codex_protocol::protocol::ReviewDecision;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
use codex_protocol::request_permissions::PermissionGrantScope;
|
||||
use codex_protocol::request_permissions::RequestPermissionsResponse;
|
||||
use codex_protocol::user_input::UserInput;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
|
|
@ -993,6 +994,7 @@ async fn request_permissions_grants_apply_to_later_exec_command_calls() -> Resul
|
|||
id: "permissions-call".to_string(),
|
||||
response: RequestPermissionsResponse {
|
||||
permissions: normalized_requested_permissions.clone(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
|
|
@ -1106,6 +1108,7 @@ async fn request_permissions_preapprove_explicit_exec_permissions_outside_on_req
|
|||
id: "permissions-call".to_string(),
|
||||
response: RequestPermissionsResponse {
|
||||
permissions: normalized_requested_permissions,
|
||||
scope: PermissionGrantScope::Turn,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
|
|
@ -1218,6 +1221,7 @@ async fn request_permissions_grants_apply_to_later_shell_command_calls() -> Resu
|
|||
id: "permissions-call".to_string(),
|
||||
response: RequestPermissionsResponse {
|
||||
permissions: normalized_requested_permissions.clone(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
|
|
@ -1360,6 +1364,7 @@ async fn partial_request_permissions_grants_do_not_preapprove_new_permissions()
|
|||
id: "permissions-call".to_string(),
|
||||
response: RequestPermissionsResponse {
|
||||
permissions: granted_permissions.clone(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
|
|
@ -1477,6 +1482,7 @@ async fn request_permissions_grants_do_not_carry_across_turns() -> Result<()> {
|
|||
id: "permissions-call".to_string(),
|
||||
response: RequestPermissionsResponse {
|
||||
permissions: normalized_requested_permissions,
|
||||
scope: PermissionGrantScope::Turn,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
|
|
@ -1518,3 +1524,138 @@ async fn request_permissions_grants_do_not_carry_across_turns() -> Result<()> {
|
|||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "current_thread")]
|
||||
#[cfg(target_os = "macos")]
|
||||
async fn request_permissions_session_grants_carry_across_turns() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
skip_if_sandbox!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let approval_policy = AskForApproval::OnRequest;
|
||||
let sandbox_policy = workspace_write_excluding_tmp();
|
||||
let sandbox_policy_for_config = sandbox_policy.clone();
|
||||
|
||||
let mut builder = test_codex().with_config(move |config| {
|
||||
config.permissions.approval_policy = Constrained::allow_any(approval_policy);
|
||||
config.permissions.sandbox_policy = Constrained::allow_any(sandbox_policy_for_config);
|
||||
config
|
||||
.features
|
||||
.enable(Feature::RequestPermissions)
|
||||
.expect("test config should allow feature update");
|
||||
config
|
||||
.features
|
||||
.enable(Feature::RequestPermissionsTool)
|
||||
.expect("test config should allow feature update");
|
||||
});
|
||||
let test = builder.build(&server).await?;
|
||||
|
||||
let outside_dir = tempfile::tempdir()?;
|
||||
let outside_write = outside_dir.path().join("session-sticky-write.txt");
|
||||
let requested_permissions = requested_directory_write_permissions(outside_dir.path());
|
||||
let normalized_requested_permissions =
|
||||
normalized_directory_write_permissions(outside_dir.path())?;
|
||||
let command = format!(
|
||||
"printf {:?} > {:?} && cat {:?}",
|
||||
"session-sticky-ok", outside_write, outside_write
|
||||
);
|
||||
|
||||
let _first_turn = mount_sse_sequence(
|
||||
&server,
|
||||
vec![
|
||||
sse(vec![
|
||||
ev_response_created("resp-session-turn-1"),
|
||||
request_permissions_tool_event(
|
||||
"permissions-call",
|
||||
"Allow writing outside the workspace",
|
||||
&requested_permissions,
|
||||
)?,
|
||||
ev_completed("resp-session-turn-1"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-session-turn-2"),
|
||||
ev_assistant_message("msg-session-turn-1", "done"),
|
||||
ev_completed("resp-session-turn-2"),
|
||||
]),
|
||||
],
|
||||
)
|
||||
.await;
|
||||
|
||||
submit_turn(
|
||||
&test,
|
||||
"request session permissions for later use",
|
||||
approval_policy,
|
||||
sandbox_policy.clone(),
|
||||
)
|
||||
.await?;
|
||||
|
||||
let granted_permissions = expect_request_permissions_event(&test, "permissions-call").await;
|
||||
assert_eq!(
|
||||
granted_permissions,
|
||||
normalized_requested_permissions.clone()
|
||||
);
|
||||
test.codex
|
||||
.submit(Op::RequestPermissionsResponse {
|
||||
id: "permissions-call".to_string(),
|
||||
response: RequestPermissionsResponse {
|
||||
permissions: normalized_requested_permissions,
|
||||
scope: PermissionGrantScope::Session,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
wait_for_completion(&test).await;
|
||||
|
||||
let second_turn = mount_sse_sequence(
|
||||
&server,
|
||||
vec![
|
||||
sse(vec![
|
||||
ev_response_created("resp-session-turn-3"),
|
||||
exec_command_event("exec-call", &command)?,
|
||||
ev_completed("resp-session-turn-3"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-session-turn-4"),
|
||||
ev_assistant_message("msg-session-turn-2", "done"),
|
||||
ev_completed("resp-session-turn-4"),
|
||||
]),
|
||||
],
|
||||
)
|
||||
.await;
|
||||
|
||||
submit_turn(
|
||||
&test,
|
||||
"reuse session permissions in a later turn",
|
||||
approval_policy,
|
||||
sandbox_policy,
|
||||
)
|
||||
.await?;
|
||||
|
||||
let completion_event = wait_for_event(&test.codex, |event| {
|
||||
matches!(
|
||||
event,
|
||||
EventMsg::ExecApprovalRequest(_) | EventMsg::TurnComplete(_)
|
||||
)
|
||||
})
|
||||
.await;
|
||||
if let EventMsg::ExecApprovalRequest(approval) = completion_event {
|
||||
test.codex
|
||||
.submit(Op::ExecApproval {
|
||||
id: approval.effective_approval_id(),
|
||||
turn_id: None,
|
||||
decision: ReviewDecision::Approved,
|
||||
})
|
||||
.await?;
|
||||
wait_for_completion(&test).await;
|
||||
}
|
||||
|
||||
let exec_output = second_turn
|
||||
.function_call_output_text("exec-call")
|
||||
.map(|output| json!({ "output": output }))
|
||||
.unwrap_or_else(|| panic!("expected exec-call output"));
|
||||
let result = parse_result(&exec_output);
|
||||
assert_eq!(result.exit_code, Some(0));
|
||||
assert_eq!(result.stdout.trim(), "session-sticky-ok");
|
||||
assert_eq!(fs::read_to_string(&outside_write)?, "session-sticky-ok");
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
|
|
|||
293
codex-rs/core/tests/suite/request_permissions_tool.rs
Normal file
293
codex-rs/core/tests/suite/request_permissions_tool.rs
Normal file
|
|
@ -0,0 +1,293 @@
|
|||
#![allow(clippy::unwrap_used, clippy::expect_used)]
|
||||
#![cfg(target_os = "macos")]
|
||||
|
||||
use anyhow::Result;
|
||||
use codex_core::config::Constrained;
|
||||
use codex_core::features::Feature;
|
||||
use codex_protocol::models::FileSystemPermissions;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::protocol::AskForApproval;
|
||||
use codex_protocol::protocol::EventMsg;
|
||||
use codex_protocol::protocol::Op;
|
||||
use codex_protocol::protocol::ReviewDecision;
|
||||
use codex_protocol::protocol::SandboxPolicy;
|
||||
use codex_protocol::request_permissions::PermissionGrantScope;
|
||||
use codex_protocol::request_permissions::RequestPermissionsResponse;
|
||||
use codex_protocol::user_input::UserInput;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use core_test_support::responses::ev_assistant_message;
|
||||
use core_test_support::responses::ev_completed;
|
||||
use core_test_support::responses::ev_function_call;
|
||||
use core_test_support::responses::ev_response_created;
|
||||
use core_test_support::responses::mount_sse_sequence;
|
||||
use core_test_support::responses::sse;
|
||||
use core_test_support::responses::start_mock_server;
|
||||
use core_test_support::skip_if_no_network;
|
||||
use core_test_support::skip_if_sandbox;
|
||||
use core_test_support::test_codex::TestCodex;
|
||||
use core_test_support::test_codex::test_codex;
|
||||
use core_test_support::wait_for_event;
|
||||
use pretty_assertions::assert_eq;
|
||||
use regex_lite::Regex;
|
||||
use serde_json::Value;
|
||||
use serde_json::json;
|
||||
use std::fs;
|
||||
use std::path::Path;
|
||||
|
||||
fn absolute_path(path: &Path) -> AbsolutePathBuf {
|
||||
AbsolutePathBuf::try_from(path).expect("absolute path")
|
||||
}
|
||||
|
||||
fn request_permissions_tool_event(
|
||||
call_id: &str,
|
||||
reason: &str,
|
||||
permissions: &PermissionProfile,
|
||||
) -> Result<Value> {
|
||||
let args = json!({
|
||||
"reason": reason,
|
||||
"permissions": permissions,
|
||||
});
|
||||
let args_str = serde_json::to_string(&args)?;
|
||||
Ok(ev_function_call(call_id, "request_permissions", &args_str))
|
||||
}
|
||||
|
||||
fn exec_command_event(call_id: &str, command: &str) -> Result<Value> {
|
||||
let args = json!({
|
||||
"cmd": command,
|
||||
"yield_time_ms": 1_000_u64,
|
||||
});
|
||||
let args_str = serde_json::to_string(&args)?;
|
||||
Ok(ev_function_call(call_id, "exec_command", &args_str))
|
||||
}
|
||||
|
||||
fn workspace_write_excluding_tmp() -> SandboxPolicy {
|
||||
SandboxPolicy::WorkspaceWrite {
|
||||
writable_roots: vec![],
|
||||
read_only_access: Default::default(),
|
||||
network_access: false,
|
||||
exclude_tmpdir_env_var: true,
|
||||
exclude_slash_tmp: true,
|
||||
}
|
||||
}
|
||||
|
||||
fn requested_directory_write_permissions(path: &Path) -> PermissionProfile {
|
||||
PermissionProfile {
|
||||
file_system: Some(FileSystemPermissions {
|
||||
read: Some(vec![]),
|
||||
write: Some(vec![absolute_path(path)]),
|
||||
}),
|
||||
..Default::default()
|
||||
}
|
||||
}
|
||||
|
||||
fn normalized_directory_write_permissions(path: &Path) -> Result<PermissionProfile> {
|
||||
Ok(PermissionProfile {
|
||||
file_system: Some(FileSystemPermissions {
|
||||
read: Some(vec![]),
|
||||
write: Some(vec![AbsolutePathBuf::try_from(path.canonicalize()?)?]),
|
||||
}),
|
||||
..Default::default()
|
||||
})
|
||||
}
|
||||
|
||||
fn parse_result(item: &Value) -> (Option<i64>, String) {
|
||||
let output_str = item
|
||||
.get("output")
|
||||
.and_then(Value::as_str)
|
||||
.expect("shell output payload");
|
||||
match serde_json::from_str::<Value>(output_str) {
|
||||
Ok(parsed) => {
|
||||
let exit_code = parsed["metadata"]["exit_code"].as_i64();
|
||||
let stdout = parsed["output"].as_str().unwrap_or_default().to_string();
|
||||
(exit_code, stdout)
|
||||
}
|
||||
Err(_) => {
|
||||
let structured = Regex::new(r"(?s)^Exit code:\s*(-?\d+).*?Output:\n(.*)$").unwrap();
|
||||
let regex =
|
||||
Regex::new(r"(?s)^.*?Process exited with code (\d+)\n.*?Output:\n(.*)$").unwrap();
|
||||
if let Some(captures) = structured.captures(output_str) {
|
||||
let exit_code = captures.get(1).unwrap().as_str().parse::<i64>().unwrap();
|
||||
let output = captures.get(2).unwrap().as_str();
|
||||
(Some(exit_code), output.to_string())
|
||||
} else if let Some(captures) = regex.captures(output_str) {
|
||||
let exit_code = captures.get(1).unwrap().as_str().parse::<i64>().unwrap();
|
||||
let output = captures.get(2).unwrap().as_str();
|
||||
(Some(exit_code), output.to_string())
|
||||
} else {
|
||||
(None, output_str.to_string())
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
async fn submit_turn(
|
||||
test: &TestCodex,
|
||||
prompt: &str,
|
||||
approval_policy: AskForApproval,
|
||||
sandbox_policy: SandboxPolicy,
|
||||
) -> Result<()> {
|
||||
let session_model = test.session_configured.model.clone();
|
||||
test.codex
|
||||
.submit(Op::UserTurn {
|
||||
items: vec![UserInput::Text {
|
||||
text: prompt.into(),
|
||||
text_elements: Vec::new(),
|
||||
}],
|
||||
final_output_json_schema: None,
|
||||
cwd: test.cwd.path().to_path_buf(),
|
||||
approval_policy,
|
||||
sandbox_policy,
|
||||
model: session_model,
|
||||
effort: None,
|
||||
summary: None,
|
||||
service_tier: None,
|
||||
collaboration_mode: None,
|
||||
personality: None,
|
||||
})
|
||||
.await?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn expect_request_permissions_event(
|
||||
test: &TestCodex,
|
||||
expected_call_id: &str,
|
||||
) -> PermissionProfile {
|
||||
let event = wait_for_event(&test.codex, |event| {
|
||||
matches!(
|
||||
event,
|
||||
EventMsg::RequestPermissions(_) | EventMsg::TurnComplete(_)
|
||||
)
|
||||
})
|
||||
.await;
|
||||
|
||||
match event {
|
||||
EventMsg::RequestPermissions(request) => {
|
||||
assert_eq!(request.call_id, expected_call_id);
|
||||
request.permissions
|
||||
}
|
||||
EventMsg::TurnComplete(_) => panic!("expected request_permissions before completion"),
|
||||
other => panic!("unexpected event: {other:?}"),
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "current_thread")]
|
||||
#[cfg(target_os = "macos")]
|
||||
async fn approved_folder_write_request_permissions_unblocks_later_exec_without_sandbox_args()
|
||||
-> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
skip_if_sandbox!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let approval_policy = AskForApproval::OnRequest;
|
||||
let sandbox_policy = workspace_write_excluding_tmp();
|
||||
let sandbox_policy_for_config = sandbox_policy.clone();
|
||||
|
||||
let mut builder = test_codex().with_config(move |config| {
|
||||
config.permissions.approval_policy = Constrained::allow_any(approval_policy);
|
||||
config.permissions.sandbox_policy = Constrained::allow_any(sandbox_policy_for_config);
|
||||
config
|
||||
.features
|
||||
.enable(Feature::RequestPermissions)
|
||||
.expect("test config should allow feature update");
|
||||
config
|
||||
.features
|
||||
.enable(Feature::RequestPermissionsTool)
|
||||
.expect("test config should allow feature update");
|
||||
});
|
||||
let test = builder.build(&server).await?;
|
||||
|
||||
let requested_dir = tempfile::tempdir()?;
|
||||
let requested_file = requested_dir.path().join("allowed-write.txt");
|
||||
let command = format!(
|
||||
"printf {:?} > {:?} && cat {:?}",
|
||||
"folder-grant-ok", requested_file, requested_file
|
||||
);
|
||||
let requested_permissions = requested_directory_write_permissions(requested_dir.path());
|
||||
let normalized_requested_permissions =
|
||||
normalized_directory_write_permissions(requested_dir.path())?;
|
||||
|
||||
let responses = mount_sse_sequence(
|
||||
&server,
|
||||
vec![
|
||||
sse(vec![
|
||||
ev_response_created("resp-request-permissions-1"),
|
||||
request_permissions_tool_event(
|
||||
"permissions-call",
|
||||
"Allow writing outside the workspace",
|
||||
&requested_permissions,
|
||||
)?,
|
||||
ev_completed("resp-request-permissions-1"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-request-permissions-2"),
|
||||
exec_command_event("exec-call", &command)?,
|
||||
ev_completed("resp-request-permissions-2"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-request-permissions-3"),
|
||||
ev_assistant_message("msg-request-permissions-1", "done"),
|
||||
ev_completed("resp-request-permissions-3"),
|
||||
]),
|
||||
],
|
||||
)
|
||||
.await;
|
||||
|
||||
submit_turn(
|
||||
&test,
|
||||
"write outside the workspace",
|
||||
approval_policy,
|
||||
sandbox_policy,
|
||||
)
|
||||
.await?;
|
||||
|
||||
let granted_permissions = expect_request_permissions_event(&test, "permissions-call").await;
|
||||
assert_eq!(
|
||||
granted_permissions,
|
||||
normalized_requested_permissions.clone()
|
||||
);
|
||||
test.codex
|
||||
.submit(Op::RequestPermissionsResponse {
|
||||
id: "permissions-call".to_string(),
|
||||
response: RequestPermissionsResponse {
|
||||
permissions: normalized_requested_permissions,
|
||||
scope: PermissionGrantScope::Turn,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
|
||||
let completion_event = wait_for_event(&test.codex, |event| {
|
||||
matches!(
|
||||
event,
|
||||
EventMsg::ExecApprovalRequest(_) | EventMsg::TurnComplete(_)
|
||||
)
|
||||
})
|
||||
.await;
|
||||
if let EventMsg::ExecApprovalRequest(approval) = completion_event {
|
||||
test.codex
|
||||
.submit(Op::ExecApproval {
|
||||
id: approval.effective_approval_id(),
|
||||
turn_id: None,
|
||||
decision: ReviewDecision::Approved,
|
||||
})
|
||||
.await?;
|
||||
wait_for_event(&test.codex, |event| {
|
||||
matches!(event, EventMsg::TurnComplete(_))
|
||||
})
|
||||
.await;
|
||||
}
|
||||
|
||||
let exec_output = responses
|
||||
.function_call_output_text("exec-call")
|
||||
.map(|output| json!({ "output": output }))
|
||||
.unwrap_or_else(|| panic!("expected exec-call output"));
|
||||
let (exit_code, stdout) = parse_result(&exec_output);
|
||||
assert!(exit_code.is_none() || exit_code == Some(0));
|
||||
assert!(stdout.contains("folder-grant-ok"));
|
||||
assert!(
|
||||
requested_file.exists(),
|
||||
"touch command should create the file"
|
||||
);
|
||||
assert_eq!(fs::read_to_string(&requested_file)?, "folder-grant-ok");
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
|
@ -4,6 +4,14 @@ use serde::Deserialize;
|
|||
use serde::Serialize;
|
||||
use ts_rs::TS;
|
||||
|
||||
#[derive(Debug, Clone, Copy, Default, Deserialize, Serialize, PartialEq, Eq, JsonSchema, TS)]
|
||||
#[serde(rename_all = "snake_case")]
|
||||
pub enum PermissionGrantScope {
|
||||
#[default]
|
||||
Turn,
|
||||
Session,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq, JsonSchema, TS)]
|
||||
pub struct RequestPermissionsArgs {
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
|
|
@ -14,6 +22,8 @@ pub struct RequestPermissionsArgs {
|
|||
#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq, JsonSchema, TS)]
|
||||
pub struct RequestPermissionsResponse {
|
||||
pub permissions: PermissionProfile,
|
||||
#[serde(default)]
|
||||
pub scope: PermissionGrantScope,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq, JsonSchema, TS)]
|
||||
|
|
|
|||
|
|
@ -28,6 +28,7 @@ use codex_protocol::protocol::NetworkApprovalContext;
|
|||
use codex_protocol::protocol::NetworkPolicyRuleAction;
|
||||
use codex_protocol::protocol::Op;
|
||||
use codex_protocol::protocol::ReviewDecision;
|
||||
use codex_protocol::request_permissions::PermissionGrantScope;
|
||||
use crossterm::event::KeyCode;
|
||||
use crossterm::event::KeyEvent;
|
||||
use crossterm::event::KeyEventKind;
|
||||
|
|
@ -282,9 +283,16 @@ impl ApprovalOverlay {
|
|||
ReviewDecision::ApprovedExecpolicyAmendment { .. }
|
||||
| ReviewDecision::NetworkPolicyAmendment { .. } => Default::default(),
|
||||
};
|
||||
let scope = if matches!(decision, ReviewDecision::ApprovedForSession) {
|
||||
PermissionGrantScope::Session
|
||||
} else {
|
||||
PermissionGrantScope::Turn
|
||||
};
|
||||
if request.thread_label().is_none() {
|
||||
let message = if granted_permissions.is_empty() {
|
||||
"You did not grant additional permissions"
|
||||
} else if matches!(scope, PermissionGrantScope::Session) {
|
||||
"You granted additional permissions for this session"
|
||||
} else {
|
||||
"You granted additional permissions"
|
||||
};
|
||||
|
|
@ -299,6 +307,7 @@ impl ApprovalOverlay {
|
|||
id: call_id.to_string(),
|
||||
response: codex_protocol::request_permissions::RequestPermissionsResponse {
|
||||
permissions: granted_permissions,
|
||||
scope,
|
||||
},
|
||||
},
|
||||
});
|
||||
|
|
@ -831,6 +840,12 @@ fn permissions_options() -> Vec<ApprovalOption> {
|
|||
display_shortcut: None,
|
||||
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('y'))],
|
||||
},
|
||||
ApprovalOption {
|
||||
label: "Yes, grant these permissions for this session".to_string(),
|
||||
decision: ApprovalDecision::Review(ReviewDecision::ApprovedForSession),
|
||||
display_shortcut: None,
|
||||
additional_shortcuts: vec![key_hint::plain(KeyCode::Char('a'))],
|
||||
},
|
||||
ApprovalOption {
|
||||
label: "No, continue without permissions".to_string(),
|
||||
decision: ApprovalDecision::Review(ReviewDecision::Denied),
|
||||
|
|
@ -1244,11 +1259,39 @@ mod tests {
|
|||
labels,
|
||||
vec![
|
||||
"Yes, grant these permissions".to_string(),
|
||||
"Yes, grant these permissions for this session".to_string(),
|
||||
"No, continue without permissions".to_string(),
|
||||
]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn permissions_session_shortcut_submits_session_scope() {
|
||||
let (tx, mut rx) = unbounded_channel::<AppEvent>();
|
||||
let tx = AppEventSender::new(tx);
|
||||
let mut view =
|
||||
ApprovalOverlay::new(make_permissions_request(), tx, Features::with_defaults());
|
||||
|
||||
view.handle_key_event(KeyEvent::new(KeyCode::Char('a'), KeyModifiers::NONE));
|
||||
|
||||
let mut saw_op = false;
|
||||
while let Ok(ev) = rx.try_recv() {
|
||||
if let AppEvent::SubmitThreadOp {
|
||||
op: Op::RequestPermissionsResponse { response, .. },
|
||||
..
|
||||
} = ev
|
||||
{
|
||||
assert_eq!(response.scope, PermissionGrantScope::Session);
|
||||
saw_op = true;
|
||||
break;
|
||||
}
|
||||
}
|
||||
assert!(
|
||||
saw_op,
|
||||
"expected permission approval decision to emit a session-scoped response"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn additional_permissions_prompt_shows_permission_rule_line() {
|
||||
let (tx, _rx) = unbounded_channel::<AppEvent>();
|
||||
|
|
|
|||
|
|
@ -10,6 +10,7 @@ expression: "normalize_snapshot_paths(render_overlay_lines(&view, 120))"
|
|||
Permission rule: network; read `/tmp/readme.txt`; write `/tmp/out.txt`
|
||||
|
||||
› 1. Yes, grant these permissions (y)
|
||||
2. No, continue without permissions (n)
|
||||
2. Yes, grant these permissions for this session (a)
|
||||
3. No, continue without permissions (n)
|
||||
|
||||
Press enter to confirm or esc to cancel
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue