mirror of
https://github.com/openai/codex.git
synced 2026-10-19 16:17:49 +00:00
Preserve target-native cwd in permission approval requests (#42314)
## Why Permission requests can originate in an environment whose path convention differs from the Codex host. Requiring the request working directory to be host-native prevents those requests from reaching interactive approval. ## What changed - Carry the request `cwd` as a `LegacyAppPathString` through core events and the app-server protocol so the target environment's native representation is preserved. - Convert the preserved value back to a `PathUri` when materializing permission grants. - Regenerate the JSON and TypeScript protocol schemas for the updated `cwd` type. ## Testing - Exercise the app-server permission request round trip with a target-native working directory across host path conventions. - Verify remote permission requests retain their environment working directory and granted access still unblocks later execution. GitOrigin-RevId: 97dc9707d9048883d20bab1b69c93871039f3188
This commit is contained in:
@@ -1018,7 +1018,7 @@ fn sample_permissions_approval_request(request_id: i64) -> ServerRequest {
|
||||
item_id: "permissions-1".to_string(),
|
||||
environment_id: None,
|
||||
started_at_ms: 1_000,
|
||||
cwd: test_path_buf("/tmp").abs(),
|
||||
cwd: test_path_buf("/tmp").abs().into(),
|
||||
reason: Some("need network".to_string()),
|
||||
permissions: RequestPermissionProfile {
|
||||
network: Some(codex_app_server_protocol::AdditionalNetworkPermissions {
|
||||
|
||||
+1
-5
@@ -1,10 +1,6 @@
|
||||
{
|
||||
"$schema": "http://json-schema.org/draft-07/schema#",
|
||||
"definitions": {
|
||||
"AbsolutePathBuf": {
|
||||
"description": "A path that is guaranteed to be absolute and normalized (though it is not guaranteed to be canonicalized or exist on the filesystem).\n\nIMPORTANT: When deserializing an `AbsolutePathBuf`, a base path must be set using [AbsolutePathBufGuard::new]. If no base path is set, the deserialization will fail unless the path being deserialized is already absolute.",
|
||||
"type": "string"
|
||||
},
|
||||
"AdditionalFileSystemPermissions": {
|
||||
"properties": {
|
||||
"entries": {
|
||||
@@ -294,7 +290,7 @@
|
||||
},
|
||||
"properties": {
|
||||
"cwd": {
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
"$ref": "#/definitions/LegacyAppPathString"
|
||||
},
|
||||
"environmentId": {
|
||||
"default": null,
|
||||
|
||||
+1
-5
@@ -1,10 +1,6 @@
|
||||
{
|
||||
"$schema": "http://json-schema.org/draft-07/schema#",
|
||||
"definitions": {
|
||||
"AbsolutePathBuf": {
|
||||
"description": "A path that is guaranteed to be absolute and normalized (though it is not guaranteed to be canonicalized or exist on the filesystem).\n\nIMPORTANT: When deserializing an `AbsolutePathBuf`, a base path must be set using [AbsolutePathBufGuard::new]. If no base path is set, the deserialization will fail unless the path being deserialized is already absolute.",
|
||||
"type": "string"
|
||||
},
|
||||
"AdditionalFileSystemPermissions": {
|
||||
"properties": {
|
||||
"entries": {
|
||||
@@ -1666,7 +1662,7 @@
|
||||
"PermissionsRequestApprovalParams": {
|
||||
"properties": {
|
||||
"cwd": {
|
||||
"$ref": "#/definitions/AbsolutePathBuf"
|
||||
"$ref": "#/definitions/LegacyAppPathString"
|
||||
},
|
||||
"environmentId": {
|
||||
"default": null,
|
||||
|
||||
+1
-1
@@ -4272,7 +4272,7 @@
|
||||
"$schema": "http://json-schema.org/draft-07/schema#",
|
||||
"properties": {
|
||||
"cwd": {
|
||||
"$ref": "#/definitions/v2/AbsolutePathBuf"
|
||||
"$ref": "#/definitions/v2/LegacyAppPathString"
|
||||
},
|
||||
"environmentId": {
|
||||
"default": null,
|
||||
|
||||
BIN
Binary file not shown.
BIN
Binary file not shown.
+2
-2
@@ -1,11 +1,11 @@
|
||||
// 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.
|
||||
import type { AbsolutePathBuf } from "../AbsolutePathBuf";
|
||||
import type { LegacyAppPathString } from "../LegacyAppPathString";
|
||||
import type { RequestPermissionProfile } from "./RequestPermissionProfile";
|
||||
|
||||
export type PermissionsRequestApprovalParams = { threadId: string, turnId: string, itemId: string, environmentId: string | null,
|
||||
/**
|
||||
* Unix timestamp (in milliseconds) when this approval request started.
|
||||
*/
|
||||
startedAtMs: number, cwd: AbsolutePathBuf, reason: string | null, permissions: RequestPermissionProfile, };
|
||||
startedAtMs: number, cwd: LegacyAppPathString, reason: string | null, permissions: RequestPermissionProfile, };
|
||||
|
||||
@@ -779,7 +779,7 @@ pub struct PermissionsRequestApprovalParams {
|
||||
/// Unix timestamp (in milliseconds) when this approval request started.
|
||||
#[ts(type = "number")]
|
||||
pub started_at_ms: i64,
|
||||
pub cwd: AbsolutePathBuf,
|
||||
pub cwd: LegacyAppPathString,
|
||||
pub reason: Option<String>,
|
||||
pub permissions: RequestPermissionProfile,
|
||||
}
|
||||
|
||||
@@ -743,7 +743,10 @@ fn permissions_request_approval_uses_request_permission_profile() {
|
||||
}))
|
||||
.expect("permissions request should deserialize");
|
||||
|
||||
assert_eq!(params.cwd, absolute_path("repo"));
|
||||
assert_eq!(
|
||||
params.cwd,
|
||||
LegacyAppPathString::from_abs_path(&absolute_path("repo"))
|
||||
);
|
||||
assert_eq!(params.environment_id.as_deref(), Some("remote"));
|
||||
assert_eq!(
|
||||
params.permissions,
|
||||
|
||||
@@ -932,7 +932,9 @@ pub(crate) async fn apply_bespoke_event_handling(
|
||||
.await;
|
||||
let request_cwd = match request.cwd {
|
||||
Some(cwd) => cwd,
|
||||
None => conversation.config_snapshot().await.cwd().clone(),
|
||||
None => {
|
||||
LegacyAppPathString::from_abs_path(conversation.config_snapshot().await.cwd())
|
||||
}
|
||||
};
|
||||
let params = PermissionsRequestApprovalParams {
|
||||
thread_id: conversation_id.to_string(),
|
||||
|
||||
@@ -20,8 +20,8 @@ use codex_protocol::request_permissions::PermissionGrantScope as CorePermissionG
|
||||
use codex_protocol::request_permissions::RequestPermissionProfile;
|
||||
use codex_protocol::request_permissions::RequestPermissionsResponse;
|
||||
use codex_utils_path_uri::LegacyAppPathString;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use core_test_support::responses;
|
||||
use core_test_support::skip_if_wine_exec;
|
||||
use pretty_assertions::assert_eq;
|
||||
use serde_json::json;
|
||||
use tokio::time::timeout;
|
||||
@@ -30,12 +30,6 @@ const DEFAULT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_secs
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 4)]
|
||||
async fn request_permissions_round_trip() -> Result<()> {
|
||||
// TODO(anp): Remove after tool routing accepts a target-native cwd on a different host OS.
|
||||
skip_if_wine_exec!(
|
||||
Ok(()),
|
||||
"request_permissions currently rejects the target-native Windows cwd on the Linux host"
|
||||
);
|
||||
|
||||
let codex_home = tempfile::TempDir::new()?;
|
||||
let project_root_entry = json!({
|
||||
"path": {
|
||||
@@ -112,7 +106,13 @@ async fn request_permissions_round_trip() -> Result<()> {
|
||||
assert_eq!(params.thread_id, thread.id);
|
||||
assert_eq!(params.turn_id, turn.id);
|
||||
assert_eq!(params.item_id, "call1");
|
||||
assert!(params.cwd.as_path().is_absolute());
|
||||
assert_eq!(params.cwd.as_str(), cwd.inferred_native_path_string());
|
||||
let request_cwd: PathUri = params
|
||||
.cwd
|
||||
.clone()
|
||||
.try_into()
|
||||
.expect("request cwd should remain target-native");
|
||||
assert_eq!(request_cwd, cwd);
|
||||
assert_eq!(params.reason, Some("Select a workspace root".to_string()));
|
||||
let requested_file_system = params
|
||||
.permissions
|
||||
|
||||
@@ -2891,19 +2891,6 @@ impl Session {
|
||||
return Some(response);
|
||||
}
|
||||
|
||||
// The interactive approval event remains host-native until its public
|
||||
// app-server/TUI boundary migrates in the next stack stage.
|
||||
let Ok(native_cwd) = cwd.to_abs_path() else {
|
||||
warn!(
|
||||
cwd = %cwd,
|
||||
"request_permissions interactive approval requires a cwd native to the Codex host"
|
||||
);
|
||||
return Some(RequestPermissionsResponse {
|
||||
permissions: RequestPermissionProfile::default(),
|
||||
scope: PermissionGrantScope::Turn,
|
||||
strict_auto_review: false,
|
||||
});
|
||||
};
|
||||
let _elicitation = self.services.elicitations.register();
|
||||
let (tx_response, rx_response) = oneshot::channel();
|
||||
let prev_entry = {
|
||||
@@ -2934,7 +2921,7 @@ impl Session {
|
||||
started_at_ms: now_unix_timestamp_ms(),
|
||||
reason: args.reason,
|
||||
permissions: requested_permissions,
|
||||
cwd: Some(native_cwd),
|
||||
cwd: Some(cwd.clone().into()),
|
||||
});
|
||||
self.send_event(turn_context.as_ref(), event).await;
|
||||
tokio::select! {
|
||||
|
||||
@@ -7353,7 +7353,7 @@ async fn request_permissions_emits_event_when_granular_policy_allows_requests()
|
||||
);
|
||||
#[allow(deprecated)]
|
||||
let turn_cwd = turn_context.cwd.clone();
|
||||
assert_eq!(request.cwd, Some(turn_cwd));
|
||||
assert_eq!(request.cwd, Some(turn_cwd.into()));
|
||||
|
||||
session
|
||||
.notify_request_permissions_response(&request.call_id, expected_response.clone())
|
||||
@@ -7608,7 +7608,8 @@ async fn request_permissions_response_materializes_session_cwd_grants_before_rec
|
||||
request.environment_id.as_deref(),
|
||||
Some(codex_exec_server::LOCAL_ENVIRONMENT_ID)
|
||||
);
|
||||
let request_cwd = request.cwd.clone().expect("request cwd");
|
||||
let request_cwd =
|
||||
PathUri::try_from(request.cwd.clone().expect("request cwd")).expect("request cwd URI");
|
||||
|
||||
session
|
||||
.notify_request_permissions_response(
|
||||
@@ -7622,7 +7623,7 @@ async fn request_permissions_response_materializes_session_cwd_grants_before_rec
|
||||
.await;
|
||||
|
||||
let expected_permissions = RequestPermissionProfile {
|
||||
file_system: Some(FileSystemPermissions::from_read_write_roots(
|
||||
file_system: Some(FileSystemPermissions::from_read_write_path_uris(
|
||||
/*read*/ None,
|
||||
Some(vec![request_cwd]),
|
||||
)),
|
||||
|
||||
@@ -3287,7 +3287,16 @@ async fn remote_request_permissions_grant_unblocks_later_remote_exec() -> Result
|
||||
request.environment_id.as_deref(),
|
||||
Some(REMOTE_ENVIRONMENT_ID)
|
||||
);
|
||||
assert_eq!(request.cwd.as_ref(), Some(&remote_cwd));
|
||||
let request_cwd = request.cwd.expect("request cwd");
|
||||
let expected_cwd = PathUri::from_abs_path(&remote_cwd);
|
||||
assert_eq!(
|
||||
request_cwd.as_str(),
|
||||
expected_cwd.inferred_native_path_string()
|
||||
);
|
||||
let request_cwd: PathUri = request_cwd
|
||||
.try_into()
|
||||
.expect("request cwd should remain target-native");
|
||||
assert_eq!(request_cwd, expected_cwd);
|
||||
assert_eq!(request.permissions, expected_permissions);
|
||||
|
||||
test.codex
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
use crate::models::AdditionalPermissionProfile;
|
||||
use crate::models::FileSystemPermissions;
|
||||
use crate::models::NetworkPermissions;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_utils_path_uri::LegacyAppPathString;
|
||||
use schemars::JsonSchema;
|
||||
use serde::Deserialize;
|
||||
use serde::Serialize;
|
||||
@@ -95,5 +95,5 @@ pub struct RequestPermissionsEvent {
|
||||
pub permissions: RequestPermissionProfile,
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
#[ts(optional)]
|
||||
pub cwd: Option<AbsolutePathBuf>,
|
||||
pub cwd: Option<LegacyAppPathString>,
|
||||
}
|
||||
|
||||
@@ -626,7 +626,7 @@ mod tests {
|
||||
item_id: "perm-1".to_string(),
|
||||
environment_id: None,
|
||||
started_at_ms: 0,
|
||||
cwd,
|
||||
cwd: cwd.into(),
|
||||
reason: None,
|
||||
permissions,
|
||||
},
|
||||
@@ -666,7 +666,7 @@ mod tests {
|
||||
item_id: "perm-1".to_string(),
|
||||
environment_id: None,
|
||||
started_at_ms: 0,
|
||||
cwd: absolute_path(if cfg!(windows) { r"C:\tmp" } else { "/tmp" }),
|
||||
cwd: absolute_path(if cfg!(windows) { r"C:\tmp" } else { "/tmp" }).into(),
|
||||
reason: None,
|
||||
permissions: serde_json::from_value(json!({
|
||||
"network": { "enabled": null }
|
||||
|
||||
@@ -3809,7 +3809,7 @@ async fn inactive_thread_permissions_approval_preserves_file_system_permissions(
|
||||
item_id: "call-approval".to_string(),
|
||||
environment_id: Some("remote".to_string()),
|
||||
started_at_ms: 0,
|
||||
cwd: test_absolute_path("/tmp"),
|
||||
cwd: test_absolute_path("/tmp").into(),
|
||||
reason: Some("Need access to .git".to_string()),
|
||||
permissions: codex_app_server_protocol::RequestPermissionProfile {
|
||||
network: Some(AdditionalNetworkPermissions {
|
||||
|
||||
@@ -335,7 +335,7 @@ fn app_server_request_permissions_preserves_file_system_permissions() {
|
||||
item_id: "item-1".to_string(),
|
||||
environment_id: Some("remote".to_string()),
|
||||
started_at_ms: 0,
|
||||
cwd: cwd.clone(),
|
||||
cwd: cwd.clone().into(),
|
||||
reason: Some("Select a workspace root".to_string()),
|
||||
permissions: codex_app_server_protocol::RequestPermissionProfile {
|
||||
network: Some(AppServerAdditionalNetworkPermissions {
|
||||
@@ -363,7 +363,7 @@ fn app_server_request_permissions_preserves_file_system_permissions() {
|
||||
)),
|
||||
}
|
||||
);
|
||||
assert_eq!(request.cwd, Some(cwd));
|
||||
assert_eq!(request.cwd, Some(cwd.into()));
|
||||
assert_eq!(request.environment_id.as_deref(), Some("remote"));
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user