mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Propagate permission approval environment id (#25862)
## Stack 1. #25850 - Key request-permission grants by environment: stores and applies sticky permission grants per environment id. 2. #25858 - Add `environmentId` to `request_permissions`: lets the model target a selected environment and resolves relative permission paths against it. 3. This PR (#25862) - Propagate permission approval environment id: carries the selected environment id through approval events, app-server requests, TUI prompts, and delegate forwarding. 4. #25867 - Add remote request permissions integration coverage: verifies the selected remote environment across request, approval, grant reuse, and exec. This PR is stacked on #25858, and #25867 is stacked on this PR. ## Why PR2 lets the model bind a `request_permissions` call to a selected environment, but the approval event and client-facing request still needed to carry that binding. For CCA, the user-facing prompt and delegated approval path should know which environment the grant applies to instead of relying on cwd alone. ## What Changed - Added optional `environmentId` to `RequestPermissionsEvent`. - Emit the selected environment id from core permission approval events. - Preserve the environment id through delegate forwarding, including cwd-based delegated requests. - Added `environmentId` to app-server permission approval params, generated schema/TypeScript artifacts, and README examples. - Preserve and display the environment id in TUI permission approval prompts. - Updated focused core, app-server protocol, and TUI conversion coverage. ## Testing Not run locally per instruction. Performed read-only `git diff --check`.
This commit is contained in:
@@ -835,6 +835,7 @@ fn sample_permissions_approval_request(request_id: i64) -> ServerRequest {
|
||||
thread_id: "thread-1".to_string(),
|
||||
turn_id: "turn-1".to_string(),
|
||||
item_id: "permissions-1".to_string(),
|
||||
environment_id: None,
|
||||
started_at_ms: 1_000,
|
||||
cwd: test_path_buf("/tmp").abs(),
|
||||
reason: Some("need network".to_string()),
|
||||
|
||||
+8
-1
@@ -285,6 +285,13 @@
|
||||
"cwd": {
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
},
|
||||
"environmentId": {
|
||||
"default": null,
|
||||
"type": [
|
||||
"null",
|
||||
"string"
|
||||
]
|
||||
},
|
||||
"itemId": {
|
||||
"type": "string"
|
||||
},
|
||||
@@ -319,4 +326,4 @@
|
||||
],
|
||||
"title": "PermissionsRequestApprovalParams",
|
||||
"type": "object"
|
||||
}
|
||||
}
|
||||
|
||||
+8
-1
@@ -1590,6 +1590,13 @@
|
||||
"cwd": {
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
},
|
||||
"environmentId": {
|
||||
"default": null,
|
||||
"type": [
|
||||
"null",
|
||||
"string"
|
||||
]
|
||||
},
|
||||
"itemId": {
|
||||
"type": "string"
|
||||
},
|
||||
@@ -1998,4 +2005,4 @@
|
||||
}
|
||||
],
|
||||
"title": "ServerRequest"
|
||||
}
|
||||
}
|
||||
|
||||
+8
-1
@@ -3783,6 +3783,13 @@
|
||||
"cwd": {
|
||||
"$ref": "#/definitions/v2/AbsolutePathBuf"
|
||||
},
|
||||
"environmentId": {
|
||||
"default": null,
|
||||
"type": [
|
||||
"null",
|
||||
"string"
|
||||
]
|
||||
},
|
||||
"itemId": {
|
||||
"type": "string"
|
||||
},
|
||||
@@ -19232,4 +19239,4 @@
|
||||
},
|
||||
"title": "CodexAppServerProtocol",
|
||||
"type": "object"
|
||||
}
|
||||
}
|
||||
|
||||
+1
-1
@@ -4,7 +4,7 @@
|
||||
import type { AbsolutePathBuf } from "../AbsolutePathBuf";
|
||||
import type { RequestPermissionProfile } from "./RequestPermissionProfile";
|
||||
|
||||
export type PermissionsRequestApprovalParams = { threadId: string, turnId: string, itemId: string,
|
||||
export type PermissionsRequestApprovalParams = { threadId: string, turnId: string, itemId: string, environmentId: string | null,
|
||||
/**
|
||||
* Unix timestamp (in milliseconds) when this approval request started.
|
||||
*/
|
||||
|
||||
@@ -663,6 +663,8 @@ pub struct PermissionsRequestApprovalParams {
|
||||
pub thread_id: String,
|
||||
pub turn_id: String,
|
||||
pub item_id: String,
|
||||
#[serde(default)]
|
||||
pub environment_id: Option<String>,
|
||||
/// Unix timestamp (in milliseconds) when this approval request started.
|
||||
#[ts(type = "number")]
|
||||
pub started_at_ms: i64,
|
||||
|
||||
@@ -400,6 +400,7 @@ fn permissions_request_approval_uses_request_permission_profile() {
|
||||
"threadId": "thr_123",
|
||||
"turnId": "turn_123",
|
||||
"itemId": "call_123",
|
||||
"environmentId": "remote",
|
||||
"startedAtMs": 1,
|
||||
"cwd": absolute_path_string("repo"),
|
||||
"reason": "Select a workspace root",
|
||||
@@ -416,6 +417,7 @@ fn permissions_request_approval_uses_request_permission_profile() {
|
||||
.expect("permissions request should deserialize");
|
||||
|
||||
assert_eq!(params.cwd, absolute_path("repo"));
|
||||
assert_eq!(params.environment_id.as_deref(), Some("remote"));
|
||||
assert_eq!(
|
||||
params.permissions,
|
||||
RequestPermissionProfile {
|
||||
|
||||
@@ -1379,7 +1379,7 @@ the client can offer session-scoped and/or persistent approval choices.
|
||||
|
||||
### Permission requests
|
||||
|
||||
The built-in `request_permissions` tool sends an `item/permissions/requestApproval` JSON-RPC request to the client with the requested permission profile. This v2 payload mirrors the command-execution `additionalPermissions` shape: it can request network access and additional filesystem access. The `cwd` field identifies the directory used to resolve project-root permissions and relative deny globs.
|
||||
The built-in `request_permissions` tool sends an `item/permissions/requestApproval` JSON-RPC request to the client with the requested permission profile. This v2 payload mirrors the command-execution `additionalPermissions` shape: it can request network access and additional filesystem access. The `environmentId` and `cwd` fields identify the environment and directory used to resolve project-root permissions and relative deny globs.
|
||||
|
||||
```json
|
||||
{
|
||||
@@ -1389,6 +1389,7 @@ The built-in `request_permissions` tool sends an `item/permissions/requestApprov
|
||||
"threadId": "thr_123",
|
||||
"turnId": "turn_123",
|
||||
"itemId": "call_123",
|
||||
"environmentId": "local",
|
||||
"cwd": "/Users/me/project",
|
||||
"reason": "Select a workspace root",
|
||||
"permissions": {
|
||||
|
||||
@@ -761,6 +761,7 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
thread_id: conversation_id.to_string(),
|
||||
turn_id: request.turn_id.clone(),
|
||||
item_id: request.call_id.clone(),
|
||||
environment_id: request.environment_id.clone(),
|
||||
started_at_ms: request.started_at_ms,
|
||||
cwd: request_cwd.clone(),
|
||||
reason: request.reason,
|
||||
|
||||
@@ -755,7 +755,7 @@ async fn handle_request_permissions(
|
||||
) {
|
||||
let call_id = event.call_id;
|
||||
let args = RequestPermissionsArgs {
|
||||
environment_id: None,
|
||||
environment_id: event.environment_id,
|
||||
reason: event.reason,
|
||||
permissions: event.permissions,
|
||||
};
|
||||
|
||||
@@ -183,9 +183,11 @@ async fn run_codex_thread_interactive_respects_pre_cancelled_spawn() {
|
||||
|
||||
#[tokio::test]
|
||||
async fn handle_request_permissions_uses_tool_call_id_for_round_trip() {
|
||||
let (parent_session, parent_ctx, rx_events) =
|
||||
let (parent_session, mut parent_ctx, rx_events) =
|
||||
crate::session::tests::make_session_and_context_with_rx().await;
|
||||
*parent_session.active_turn.lock().await = Some(crate::state::ActiveTurn::default());
|
||||
let parent_ctx_mut = Arc::get_mut(&mut parent_ctx).expect("single turn context ref");
|
||||
parent_ctx_mut.environments.turn_environments[0].environment_id = "remote".to_string();
|
||||
|
||||
let (tx_sub, rx_sub) = bounded(SUBMISSION_CHANNEL_CAPACITY);
|
||||
let (_tx_events, rx_events_child) = bounded(SUBMISSION_CHANNEL_CAPACITY);
|
||||
@@ -228,6 +230,7 @@ async fn handle_request_permissions_uses_tool_call_id_for_round_trip() {
|
||||
RequestPermissionsEvent {
|
||||
call_id: request_call_id,
|
||||
turn_id: "child-turn-1".to_string(),
|
||||
environment_id: Some("remote".to_string()),
|
||||
started_at_ms: 0,
|
||||
reason: Some("need access".to_string()),
|
||||
permissions: RequestPermissionProfile {
|
||||
@@ -252,6 +255,7 @@ async fn handle_request_permissions_uses_tool_call_id_for_round_trip() {
|
||||
panic!("expected RequestPermissions event");
|
||||
};
|
||||
assert_eq!(request.call_id, call_id.clone());
|
||||
assert_eq!(request.environment_id.as_deref(), Some("remote"));
|
||||
assert_eq!(request.cwd, Some(delegated_cwd));
|
||||
|
||||
parent_session
|
||||
|
||||
@@ -2251,6 +2251,7 @@ impl Session {
|
||||
let event = EventMsg::RequestPermissions(RequestPermissionsEvent {
|
||||
call_id: call_id.clone(),
|
||||
turn_id: turn_context.sub_id.clone(),
|
||||
environment_id: Some(environment.environment_id.clone()),
|
||||
started_at_ms: now_unix_timestamp_ms(),
|
||||
reason: args.reason,
|
||||
permissions: requested_permissions,
|
||||
@@ -2279,14 +2280,22 @@ impl Session {
|
||||
cwd: AbsolutePathBuf,
|
||||
cancellation_token: CancellationToken,
|
||||
) -> Option<RequestPermissionsResponse> {
|
||||
let Some(primary_environment) = turn_context.environments.primary() else {
|
||||
let turn_environment = match args.environment_id.as_deref() {
|
||||
Some(environment_id) => turn_context
|
||||
.environments
|
||||
.turn_environments
|
||||
.iter()
|
||||
.find(|environment| environment.environment_id == environment_id),
|
||||
None => turn_context.environments.primary(),
|
||||
};
|
||||
let Some(turn_environment) = turn_environment else {
|
||||
return Some(RequestPermissionsResponse {
|
||||
permissions: RequestPermissionProfile::default(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
});
|
||||
};
|
||||
let mut environment = primary_environment.selection();
|
||||
let mut environment = turn_environment.selection();
|
||||
environment.cwd = cwd;
|
||||
self.request_permissions_for_environment(
|
||||
turn_context,
|
||||
|
||||
@@ -5399,6 +5399,10 @@ async fn request_permissions_emits_event_when_granular_policy_allows_requests()
|
||||
panic!("expected request_permissions event");
|
||||
};
|
||||
assert_eq!(request.call_id, call_id);
|
||||
assert_eq!(
|
||||
request.environment_id.as_deref(),
|
||||
Some(codex_exec_server::LOCAL_ENVIRONMENT_ID)
|
||||
);
|
||||
#[allow(deprecated)]
|
||||
let turn_cwd = turn_context.cwd.clone();
|
||||
assert_eq!(request.cwd, Some(turn_cwd));
|
||||
@@ -5499,6 +5503,7 @@ async fn request_permissions_tool_resolves_relative_paths_against_selected_envir
|
||||
}),
|
||||
..Default::default()
|
||||
};
|
||||
assert_eq!(request.environment_id.as_deref(), Some("remote"));
|
||||
assert_eq!(request.permissions, expected_permissions);
|
||||
|
||||
session
|
||||
@@ -5616,6 +5621,10 @@ async fn request_permissions_response_materializes_session_cwd_grants_before_rec
|
||||
let EventMsg::RequestPermissions(request) = request_event.msg else {
|
||||
panic!("expected request_permissions event");
|
||||
};
|
||||
assert_eq!(
|
||||
request.environment_id.as_deref(),
|
||||
Some(codex_exec_server::LOCAL_ENVIRONMENT_ID)
|
||||
);
|
||||
let request_cwd = request.cwd.clone().expect("request cwd");
|
||||
|
||||
session
|
||||
|
||||
@@ -79,6 +79,15 @@ pub struct RequestPermissionsEvent {
|
||||
/// Uses `#[serde(default)]` for backwards compatibility.
|
||||
#[serde(default)]
|
||||
pub turn_id: String,
|
||||
#[serde(
|
||||
default,
|
||||
rename = "environmentId",
|
||||
alias = "environment_id",
|
||||
skip_serializing_if = "Option::is_none"
|
||||
)]
|
||||
#[ts(optional)]
|
||||
#[ts(rename = "environmentId")]
|
||||
pub environment_id: Option<String>,
|
||||
#[ts(type = "number")]
|
||||
pub started_at_ms: i64,
|
||||
#[serde(skip_serializing_if = "Option::is_none")]
|
||||
|
||||
@@ -489,6 +489,7 @@ mod tests {
|
||||
thread_id: "thread-1".to_string(),
|
||||
turn_id: "turn-1".to_string(),
|
||||
item_id: "perm-1".to_string(),
|
||||
environment_id: None,
|
||||
started_at_ms: 0,
|
||||
cwd: absolute_path(if cfg!(windows) { r"C:\tmp" } else { "/tmp" }),
|
||||
reason: None,
|
||||
|
||||
@@ -1856,12 +1856,20 @@ impl App {
|
||||
));
|
||||
}
|
||||
ApprovalRequest::Permissions {
|
||||
environment_id,
|
||||
permissions,
|
||||
reason,
|
||||
..
|
||||
} => {
|
||||
let _ = tui.enter_alt_screen();
|
||||
let mut lines = Vec::new();
|
||||
if let Some(environment_id) = environment_id {
|
||||
lines.push(Line::from(vec![
|
||||
"Environment: ".into(),
|
||||
environment_id.bold(),
|
||||
]));
|
||||
lines.push(Line::from(""));
|
||||
}
|
||||
if let Some(reason) = reason {
|
||||
lines.push(Line::from(vec!["Reason: ".into(), reason.italic()]));
|
||||
lines.push(Line::from(""));
|
||||
|
||||
@@ -2539,6 +2539,7 @@ async fn inactive_thread_permissions_approval_preserves_file_system_permissions(
|
||||
thread_id: thread_id.to_string(),
|
||||
turn_id: "turn-approval".to_string(),
|
||||
item_id: "call-approval".to_string(),
|
||||
environment_id: Some("remote".to_string()),
|
||||
started_at_ms: 0,
|
||||
cwd: test_absolute_path("/tmp"),
|
||||
reason: Some("Need access to .git".to_string()),
|
||||
@@ -2557,7 +2558,9 @@ async fn inactive_thread_permissions_approval_preserves_file_system_permissions(
|
||||
};
|
||||
|
||||
let Some(ThreadInteractiveRequest::Approval(ApprovalRequest::Permissions {
|
||||
permissions, ..
|
||||
environment_id,
|
||||
permissions,
|
||||
..
|
||||
})) = app
|
||||
.interactive_request_for_thread_request(thread_id, &request)
|
||||
.await
|
||||
@@ -2565,6 +2568,7 @@ async fn inactive_thread_permissions_approval_preserves_file_system_permissions(
|
||||
panic!("expected permissions approval request");
|
||||
};
|
||||
|
||||
assert_eq!(environment_id.as_deref(), Some("remote"));
|
||||
assert_eq!(
|
||||
permissions,
|
||||
RequestPermissionProfile {
|
||||
|
||||
@@ -310,6 +310,7 @@ impl App {
|
||||
thread_id,
|
||||
thread_label,
|
||||
call_id: params.item_id.clone(),
|
||||
environment_id: params.environment_id.clone(),
|
||||
reason: params.reason.clone(),
|
||||
permissions: params.permissions.clone().into(),
|
||||
}),
|
||||
|
||||
@@ -84,6 +84,7 @@ pub(crate) enum ApprovalRequest {
|
||||
thread_id: ThreadId,
|
||||
thread_label: Option<String>,
|
||||
call_id: String,
|
||||
environment_id: Option<String>,
|
||||
reason: Option<String>,
|
||||
permissions: RequestPermissionProfile,
|
||||
},
|
||||
@@ -713,6 +714,7 @@ fn build_header(request: &ApprovalRequest) -> Box<dyn Renderable> {
|
||||
}
|
||||
ApprovalRequest::Permissions {
|
||||
thread_label,
|
||||
environment_id,
|
||||
reason,
|
||||
permissions,
|
||||
..
|
||||
@@ -725,6 +727,13 @@ fn build_header(request: &ApprovalRequest) -> Box<dyn Renderable> {
|
||||
]));
|
||||
header.push(Line::from(""));
|
||||
}
|
||||
if let Some(environment_id) = environment_id {
|
||||
header.push(Line::from(vec![
|
||||
"Environment: ".into(),
|
||||
environment_id.clone().bold(),
|
||||
]));
|
||||
header.push(Line::from(""));
|
||||
}
|
||||
if let Some(reason) = reason {
|
||||
header.push(Line::from(vec!["Reason: ".into(), reason.clone().italic()]));
|
||||
header.push(Line::from(""));
|
||||
@@ -1227,6 +1236,7 @@ mod tests {
|
||||
thread_id: ThreadId::new(),
|
||||
thread_label: None,
|
||||
call_id: "test".to_string(),
|
||||
environment_id: None,
|
||||
reason: Some("need workspace access".to_string()),
|
||||
permissions: RequestPermissionProfile {
|
||||
network: Some(NetworkPermissions {
|
||||
|
||||
@@ -864,6 +864,7 @@ fn request_permissions_from_params(
|
||||
RequestPermissionsEvent {
|
||||
turn_id: params.turn_id,
|
||||
call_id: params.item_id,
|
||||
environment_id: params.environment_id,
|
||||
started_at_ms: params.started_at_ms,
|
||||
reason: params.reason,
|
||||
permissions: params.permissions.into(),
|
||||
|
||||
@@ -281,6 +281,7 @@ fn app_server_request_permissions_preserves_file_system_permissions() {
|
||||
thread_id: "thread-1".to_string(),
|
||||
turn_id: "turn-1".to_string(),
|
||||
item_id: "item-1".to_string(),
|
||||
environment_id: Some("remote".to_string()),
|
||||
started_at_ms: 0,
|
||||
cwd: cwd.clone(),
|
||||
reason: Some("Select a workspace root".to_string()),
|
||||
@@ -310,6 +311,7 @@ fn app_server_request_permissions_preserves_file_system_permissions() {
|
||||
}
|
||||
);
|
||||
assert_eq!(request.cwd, Some(cwd));
|
||||
assert_eq!(request.environment_id.as_deref(), Some("remote"));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
|
||||
@@ -441,6 +441,7 @@ impl ChatWidget {
|
||||
thread_id: self.thread_id.unwrap_or_default(),
|
||||
thread_label: None,
|
||||
call_id: ev.call_id,
|
||||
environment_id: ev.environment_id,
|
||||
reason: ev.reason,
|
||||
permissions: ev.permissions,
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user