mirror of
https://github.com/openai/codex.git
synced 2026-10-27 18:48:37 +00:00
feat(app-server): gate permission tools by surface support
Hide conversational permission tools unless the initializing surface advertises support for the matching server request method. This preserves existing model behavior on unsupported app-server clients while keeping delivery-time denial as a fallback. Add request-permissions coverage for each supportedServerRequests capability combination and update the app-server docs to describe the opt-in contract.
This commit is contained in:
@@ -1077,7 +1077,7 @@ the client can offer session-scoped and/or persistent approval choices.
|
||||
|
||||
### Permission requests
|
||||
|
||||
Clients must opt into each conversational permission request method by listing it in `initialize.params.capabilities.supportedServerRequests`. The opt-in is per connection and per method. Existing clients that do not advertise support keep their current behavior: app-server does not emit an unsupported approval request, no permissions are changed, and the model receives a denial-style result.
|
||||
Clients must opt into each conversational permission request method by listing it in `initialize.params.capabilities.supportedServerRequests`. The opt-in is per connection and per method. App-server uses this capability before starting a thread so unsupported clients do not expose the model-facing tools that trigger these request methods. Existing clients that do not advertise support keep their current behavior: app-server does not ask the model to use an unsupported approval UI, no permissions are changed, and delivery-time denials are only a fallback if a request is still produced.
|
||||
|
||||
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.
|
||||
|
||||
@@ -1121,7 +1121,7 @@ Within the same turn, granted permissions are sticky: later shell-like tool call
|
||||
|
||||
If the session approval policy uses `Granular` with `request_permissions: false`, standalone `request_permissions` tool calls are auto-denied and no `item/permissions/requestApproval` prompt is sent. Inline `with_additional_permissions` command requests remain controlled by `sandbox_approval`, and any previously granted permissions remain sticky for later shell-like calls in the same turn.
|
||||
|
||||
The built-in `request_permission_preset` tool sends an `item/permissionPreset/requestApproval` JSON-RPC request when a model asks the client to open the permission preset picker. The request includes the resolved preset id, label, description, approval policy, approvals reviewer, sandbox policy, and optional reason so clients can show their native picker with the requested option already selected. Clients respond with `{ "decision": "accepted", "preset": "full-access" }` or `{ "decision": "declined", "preset": "full-access" }`. When the client omits `item/permissionPreset/requestApproval` from `supportedServerRequests`, app-server declines the preset request and leaves the active permissions unchanged.
|
||||
The built-in `request_permission_preset` tool sends an `item/permissionPreset/requestApproval` JSON-RPC request when a model asks the client to open the permission preset picker. The request includes the resolved preset id, label, description, approval policy, approvals reviewer, sandbox policy, and optional reason so clients can show their native picker with the requested option already selected. Clients respond with `{ "decision": "accepted", "preset": "full-access" }` or `{ "decision": "declined", "preset": "full-access" }`. When the client omits `item/permissionPreset/requestApproval` from `supportedServerRequests`, app-server hides the preset tool for new threads; if a request is still produced, app-server declines it and leaves the active permissions unchanged.
|
||||
|
||||
### Dynamic tool calls (experimental)
|
||||
|
||||
|
||||
@@ -2331,6 +2331,13 @@ impl CodexMessageProcessor {
|
||||
};
|
||||
}
|
||||
|
||||
apply_surface_capability_feature_gates(
|
||||
&mut config,
|
||||
&listener_task_context.thread_state_manager,
|
||||
&request_id,
|
||||
)
|
||||
.await;
|
||||
|
||||
let instruction_sources = Self::instruction_sources_from_config(&config).await;
|
||||
let dynamic_tools = dynamic_tools.unwrap_or_default();
|
||||
let core_dynamic_tools = if dynamic_tools.is_empty() {
|
||||
@@ -3886,7 +3893,7 @@ impl CodexMessageProcessor {
|
||||
let cloud_requirements = self.current_cloud_requirements();
|
||||
let cli_overrides = self.current_cli_overrides();
|
||||
let runtime_feature_enablement = self.current_runtime_feature_enablement();
|
||||
let config = match derive_config_for_cwd(
|
||||
let mut config = match derive_config_for_cwd(
|
||||
&cli_overrides,
|
||||
request_overrides,
|
||||
typesafe_overrides,
|
||||
@@ -3904,6 +3911,12 @@ impl CodexMessageProcessor {
|
||||
return;
|
||||
}
|
||||
};
|
||||
apply_surface_capability_feature_gates(
|
||||
&mut config,
|
||||
&self.thread_state_manager,
|
||||
&request_id,
|
||||
)
|
||||
.await;
|
||||
|
||||
let fallback_model_provider = config.model_provider_id.clone();
|
||||
let instruction_sources = Self::instruction_sources_from_config(&config).await;
|
||||
@@ -4444,7 +4457,7 @@ impl CodexMessageProcessor {
|
||||
let cloud_requirements = self.current_cloud_requirements();
|
||||
let cli_overrides = self.current_cli_overrides();
|
||||
let runtime_feature_enablement = self.current_runtime_feature_enablement();
|
||||
let config = match derive_config_for_cwd(
|
||||
let mut config = match derive_config_for_cwd(
|
||||
&cli_overrides,
|
||||
request_overrides,
|
||||
typesafe_overrides,
|
||||
@@ -4463,6 +4476,12 @@ impl CodexMessageProcessor {
|
||||
return;
|
||||
}
|
||||
};
|
||||
apply_surface_capability_feature_gates(
|
||||
&mut config,
|
||||
&self.thread_state_manager,
|
||||
&request_id,
|
||||
)
|
||||
.await;
|
||||
|
||||
let fallback_model_provider = config.model_provider_id.clone();
|
||||
let instruction_sources = Self::instruction_sources_from_config(&config).await;
|
||||
@@ -8704,6 +8723,46 @@ async fn derive_config_from_params(
|
||||
Ok(config)
|
||||
}
|
||||
|
||||
async fn apply_surface_capability_feature_gates(
|
||||
config: &mut Config,
|
||||
thread_state_manager: &ThreadStateManager,
|
||||
request_id: &ConnectionRequestId,
|
||||
) {
|
||||
let supported_server_requests = thread_state_manager
|
||||
.supported_server_requests_for_connection(request_id.connection_id)
|
||||
.await;
|
||||
gate_permission_tool_feature(
|
||||
config,
|
||||
Feature::RequestPermissionsTool,
|
||||
supported_server_requests
|
||||
.contains(&SupportedServerRequestMethod::PermissionsRequestApproval),
|
||||
);
|
||||
gate_permission_tool_feature(
|
||||
config,
|
||||
Feature::RequestPermissionPresetTool,
|
||||
supported_server_requests
|
||||
.contains(&SupportedServerRequestMethod::PermissionPresetRequestApproval),
|
||||
);
|
||||
}
|
||||
|
||||
fn gate_permission_tool_feature(
|
||||
config: &mut Config,
|
||||
feature: Feature,
|
||||
surface_supports_tool: bool,
|
||||
) {
|
||||
if surface_supports_tool || !config.features.get().enabled(feature) {
|
||||
return;
|
||||
}
|
||||
|
||||
if let Err(err) = config.features.disable(feature) {
|
||||
warn!(
|
||||
?feature,
|
||||
error = %err,
|
||||
"failed to gate permission tool feature for unsupported app-server surface"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
async fn derive_config_for_cwd(
|
||||
cli_overrides: &[(String, TomlValue)],
|
||||
request_overrides: Option<HashMap<String, serde_json::Value>>,
|
||||
|
||||
@@ -202,6 +202,18 @@ impl ThreadStateManager {
|
||||
.insert(connection_id, supported_server_requests);
|
||||
}
|
||||
|
||||
pub(crate) async fn supported_server_requests_for_connection(
|
||||
&self,
|
||||
connection_id: ConnectionId,
|
||||
) -> HashSet<SupportedServerRequestMethod> {
|
||||
let state = self.state.lock().await;
|
||||
state
|
||||
.supported_server_requests_by_connection
|
||||
.get(&connection_id)
|
||||
.cloned()
|
||||
.unwrap_or_default()
|
||||
}
|
||||
|
||||
pub(crate) async fn subscribed_connection_ids(&self, thread_id: ThreadId) -> Vec<ConnectionId> {
|
||||
let state = self.state.lock().await;
|
||||
let mut connection_ids = state
|
||||
|
||||
@@ -19,11 +19,59 @@ use codex_app_server_protocol::ThreadStartResponse;
|
||||
use codex_app_server_protocol::TurnStartParams;
|
||||
use codex_app_server_protocol::TurnStartResponse;
|
||||
use codex_app_server_protocol::UserInput as V2UserInput;
|
||||
use serde_json::Value;
|
||||
use serde_json::json;
|
||||
use tokio::time::timeout;
|
||||
use wiremock::MockServer;
|
||||
|
||||
const DEFAULT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10);
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 4)]
|
||||
async fn permission_tools_are_hidden_without_supported_server_requests() -> Result<()> {
|
||||
let body = first_responses_request_body_for_supported_server_requests(Vec::new()).await?;
|
||||
assert_permission_tool_exposure(
|
||||
&body, /*expect_permissions*/ false, /*expect_preset*/ false,
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 4)]
|
||||
async fn permission_tools_expose_only_narrow_permission_capability() -> Result<()> {
|
||||
let body = first_responses_request_body_for_supported_server_requests(vec![
|
||||
SupportedServerRequestMethod::PermissionsRequestApproval,
|
||||
])
|
||||
.await?;
|
||||
assert_permission_tool_exposure(
|
||||
&body, /*expect_permissions*/ true, /*expect_preset*/ false,
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 4)]
|
||||
async fn permission_tools_expose_only_preset_capability() -> Result<()> {
|
||||
let body = first_responses_request_body_for_supported_server_requests(vec![
|
||||
SupportedServerRequestMethod::PermissionPresetRequestApproval,
|
||||
])
|
||||
.await?;
|
||||
assert_permission_tool_exposure(
|
||||
&body, /*expect_permissions*/ false, /*expect_preset*/ true,
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 4)]
|
||||
async fn permission_tools_expose_both_supported_capabilities() -> Result<()> {
|
||||
let body = first_responses_request_body_for_supported_server_requests(vec![
|
||||
SupportedServerRequestMethod::PermissionsRequestApproval,
|
||||
SupportedServerRequestMethod::PermissionPresetRequestApproval,
|
||||
])
|
||||
.await?;
|
||||
assert_permission_tool_exposure(
|
||||
&body, /*expect_permissions*/ true, /*expect_preset*/ true,
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 4)]
|
||||
async fn request_permissions_round_trips_when_client_supports_app_request() -> Result<()> {
|
||||
let codex_home = tempfile::TempDir::new()?;
|
||||
@@ -291,6 +339,68 @@ async fn initialize_with_supported_server_requests(
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn first_responses_request_body_for_supported_server_requests(
|
||||
supported_server_requests: Vec<SupportedServerRequestMethod>,
|
||||
) -> Result<Value> {
|
||||
let codex_home = tempfile::TempDir::new()?;
|
||||
let responses = vec![create_final_assistant_message_sse_response("done")?];
|
||||
let server = create_mock_responses_server_sequence(responses).await;
|
||||
create_config_toml(codex_home.path(), &server.uri())?;
|
||||
|
||||
let mut mcp = McpProcess::new(codex_home.path()).await?;
|
||||
if supported_server_requests.is_empty() {
|
||||
timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??;
|
||||
} else {
|
||||
initialize_with_supported_server_requests(&mut mcp, supported_server_requests).await?;
|
||||
}
|
||||
|
||||
start_thread_and_turn(&mut mcp, "Hello").await?;
|
||||
wait_for_turn_completed_without_server_request(&mut mcp).await?;
|
||||
|
||||
first_responses_request_body(&server).await
|
||||
}
|
||||
|
||||
async fn first_responses_request_body(server: &MockServer) -> Result<Value> {
|
||||
let requests = server
|
||||
.received_requests()
|
||||
.await
|
||||
.ok_or_else(|| anyhow::format_err!("failed to fetch received requests"))?;
|
||||
requests
|
||||
.into_iter()
|
||||
.find(|request| request.url.path().ends_with("/responses"))
|
||||
.ok_or_else(|| anyhow::format_err!("expected a /responses request"))?
|
||||
.body_json()
|
||||
.map_err(Into::into)
|
||||
}
|
||||
|
||||
fn assert_permission_tool_exposure(body: &Value, expect_permissions: bool, expect_preset: bool) {
|
||||
let tool_names = body
|
||||
.get("tools")
|
||||
.and_then(Value::as_array)
|
||||
.into_iter()
|
||||
.flatten()
|
||||
.filter_map(|tool| tool.get("name").and_then(Value::as_str))
|
||||
.collect::<Vec<_>>();
|
||||
assert_eq!(
|
||||
tool_names.contains(&"request_permissions"),
|
||||
expect_permissions
|
||||
);
|
||||
assert_eq!(
|
||||
tool_names.contains(&"request_permission_preset"),
|
||||
expect_preset
|
||||
);
|
||||
|
||||
let body_text = body.to_string();
|
||||
assert_eq!(
|
||||
body_text.contains("# request_permissions Tool"),
|
||||
expect_permissions
|
||||
);
|
||||
assert_eq!(
|
||||
body_text.contains("# request_permission_preset Tool"),
|
||||
expect_preset
|
||||
);
|
||||
}
|
||||
|
||||
async fn start_thread_and_turn(mcp: &mut McpProcess, input: &str) -> Result<String> {
|
||||
let thread_start_id = mcp
|
||||
.send_thread_start_request(ThreadStartParams {
|
||||
|
||||
Reference in New Issue
Block a user