mirror of
https://github.com/openai/codex.git
synced 2026-10-19 16:17:49 +00:00
Respect approval policies in temporary structured threads (#49912)
## Why Forcing `approval_policy` to `never` on temporary structured threads can trigger configuration warnings when managed requirements allow only `on-request`. These hidden threads also need to reject unexpected interaction requests without prompting the user. ## What changed Inherit the app server's approval policy when starting temporary structured threads. Reject their server requests before dynamic tool handling or interactive TUI routing. ## Testing Extend regression tests to verify inherited `on-request` approval without approval-policy warnings, and error responses for recap-thread approval and user-input requests even when the thread is marked active and primary. GitOrigin-RevId: 2d0a071d3687742c83ba933a7ef1357feea53403
This commit is contained in:
@@ -1,4 +1,5 @@
|
||||
//! App-server event stream handling for the TUI app.
|
||||
//! Hidden structured threads reject requests instead of entering interactive routing.
|
||||
|
||||
use super::App;
|
||||
use super::ThreadBufferedEvent;
|
||||
@@ -550,6 +551,23 @@ impl App {
|
||||
app_server_client: &AppServerSession,
|
||||
request: ServerRequest,
|
||||
) {
|
||||
let thread_id = server_request_thread_id(&request);
|
||||
if thread_id
|
||||
.is_some_and(|thread_id| self.temporary_structured_requests.contains_key(&thread_id))
|
||||
{
|
||||
if let Err(err) = self
|
||||
.reject_app_server_request(
|
||||
app_server_client,
|
||||
request.id().clone(),
|
||||
"temporary structured threads cannot request tools or user interaction"
|
||||
.to_string(),
|
||||
)
|
||||
.await
|
||||
{
|
||||
tracing::debug!("{err}");
|
||||
}
|
||||
return;
|
||||
}
|
||||
if let ServerRequest::DynamicToolCall { request_id, params } = &request {
|
||||
if self.dynamic_tool_tasks.contains_key(request_id)
|
||||
|| (params.namespace.as_deref() != Some(crate::dynamic_tools::NAMESPACE)
|
||||
@@ -638,7 +656,6 @@ impl App {
|
||||
return;
|
||||
}
|
||||
|
||||
let thread_id = server_request_thread_id(&request);
|
||||
let background_voice = self.background_voice.as_ref().is_some_and(|owner| {
|
||||
owner.realtime_conversation_is_running()
|
||||
&& owner.thread_id().is_some()
|
||||
|
||||
@@ -463,11 +463,31 @@ async fn temporary_recap_threads_disable_memories_and_remote_mcp_servers() -> Re
|
||||
.map(|profile| profile.id),
|
||||
mcp_server_names: Vec::new(),
|
||||
};
|
||||
crate::temporary_structured_request::start_temporary_thread(
|
||||
let response = crate::temporary_structured_request::start_temporary_thread(
|
||||
&app_server.request_handle(),
|
||||
options,
|
||||
)
|
||||
.await?;
|
||||
let thread_id = ThreadId::from_string(&response.thread.id)?;
|
||||
let (sender, _receiver) = tokio::sync::mpsc::unbounded_channel();
|
||||
app.temporary_structured_requests.insert(thread_id, sender);
|
||||
// Even if a hidden thread reaches visible routing, it must not prompt.
|
||||
app.primary_thread_id = Some(thread_id);
|
||||
app.active_thread_id = Some(thread_id);
|
||||
for request in [
|
||||
exec_approval_request(thread_id, "turn", "item", /*approval_id*/ None),
|
||||
request_user_input_request(thread_id, "turn", "item"),
|
||||
] {
|
||||
app.handle_app_server_event(
|
||||
&app_server,
|
||||
codex_app_server_client::AppServerEvent::ServerRequest(Box::new(request.clone())),
|
||||
)
|
||||
.await;
|
||||
assert!(
|
||||
!app.pending_app_server_requests
|
||||
.contains_server_request(&request)
|
||||
);
|
||||
}
|
||||
|
||||
let starts = recorded_params(&requests, "thread/start");
|
||||
assert_eq!(starts.len(), 1);
|
||||
@@ -484,6 +504,24 @@ async fn temporary_recap_threads_disable_memories_and_remote_mcp_servers() -> Re
|
||||
|
||||
app_server.shutdown().await?;
|
||||
proxy.await??;
|
||||
assert_eq!(
|
||||
requests
|
||||
.lock()
|
||||
.expect("request recorder lock")
|
||||
.iter()
|
||||
.filter(|request| request.method == "server/request/error")
|
||||
.map(|request| (request.id.clone(), request.params.clone()))
|
||||
.collect::<Vec<_>>(),
|
||||
[1, 2]
|
||||
.map(|id| (
|
||||
AppServerRequestId::Integer(id),
|
||||
Some(serde_json::json!({
|
||||
"code": -32000,
|
||||
"message": "temporary structured threads cannot request tools or user interaction"
|
||||
}))
|
||||
))
|
||||
.to_vec()
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
|
||||
@@ -762,7 +762,17 @@ pub(super) async fn start_recording_app_server_with_realtime_speech(
|
||||
trace: None,
|
||||
});
|
||||
}
|
||||
JSONRPCMessage::Error(_) => {}
|
||||
JSONRPCMessage::Error(error) => {
|
||||
request_sink
|
||||
.lock()
|
||||
.expect("request recorder lock")
|
||||
.push(JSONRPCRequest {
|
||||
id: error.id,
|
||||
method: "server/request/error".to_string(),
|
||||
params: Some(serde_json::to_value(error.error)?),
|
||||
trace: None,
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
embedded.shutdown().await?;
|
||||
|
||||
@@ -2,7 +2,6 @@
|
||||
|
||||
use codex_app_server_client::AppServerRequestHandle;
|
||||
use codex_app_server_client::TypedRequestError;
|
||||
use codex_app_server_protocol::AskForApproval;
|
||||
use codex_app_server_protocol::ClientRequest;
|
||||
use codex_app_server_protocol::ConfigReadParams;
|
||||
use codex_app_server_protocol::ConfigReadResponse;
|
||||
@@ -48,6 +47,7 @@ pub(crate) struct TemporaryStructuredThreadOptions {
|
||||
///
|
||||
/// Structured prompts can contain untrusted transcript text, so the effective app-server config is
|
||||
/// read first and every MCP server is explicitly disabled alongside built-in and extension tools.
|
||||
/// Approval policy is inherited from the server; the TUI rejects unexpected interaction requests.
|
||||
pub(crate) async fn start_temporary_thread(
|
||||
request_handle: &AppServerRequestHandle,
|
||||
options: TemporaryStructuredThreadOptions,
|
||||
@@ -143,7 +143,6 @@ pub(crate) async fn start_temporary_thread(
|
||||
model: Some(model),
|
||||
model_provider: Some(model_provider),
|
||||
cwd: Some(cwd),
|
||||
approval_policy: Some(AskForApproval::Never),
|
||||
sandbox: custom_permission_profile
|
||||
.is_none()
|
||||
.then_some(SandboxMode::ReadOnly),
|
||||
|
||||
@@ -5,6 +5,7 @@ use super::start_temporary_thread;
|
||||
use crate::legacy_core::config::ConfigBuilder;
|
||||
use crate::test_support::PathBufExt;
|
||||
use codex_app_server_client::AppServerClient;
|
||||
use codex_app_server_client::AppServerEvent;
|
||||
use codex_app_server_protocol::AskForApproval;
|
||||
use codex_app_server_protocol::ItemCompletedNotification;
|
||||
use codex_app_server_protocol::SandboxPolicy;
|
||||
@@ -61,7 +62,8 @@ async fn managed_workspace_default_respects_read_only_availability() -> color_ey
|
||||
std::fs::write(
|
||||
&requirements_path,
|
||||
format!(
|
||||
"default_permissions = \":workspace\"\n\n\
|
||||
"allowed_approval_policies = [\"on-request\"]\n\
|
||||
default_permissions = \":workspace\"\n\n\
|
||||
[allowed_permission_profiles]\n\
|
||||
\":read-only\" = {read_only_allowed}\n\
|
||||
\":workspace\" = true\n"
|
||||
@@ -103,8 +105,29 @@ async fn managed_workspace_default_respects_read_only_availability() -> color_ey
|
||||
Default::default(),
|
||||
)
|
||||
.await?;
|
||||
let app_server = AppServerClient::InProcess(client);
|
||||
let mut app_server = AppServerClient::InProcess(client);
|
||||
let result = start_temporary_thread(&app_server.request_handle(), options).await;
|
||||
// Thread/start warnings precede thread/started on the connection.
|
||||
tokio::time::timeout(std::time::Duration::from_secs(/*secs*/ 5), async {
|
||||
loop {
|
||||
let AppServerEvent::ServerNotification(notification) =
|
||||
app_server.next_event().await.expect("thread startup event")
|
||||
else {
|
||||
continue;
|
||||
};
|
||||
match *notification {
|
||||
ServerNotification::ConfigWarning(warning) => {
|
||||
assert!(
|
||||
!warning.summary.contains("`approval_policy`"),
|
||||
"{warning:?}"
|
||||
);
|
||||
}
|
||||
ServerNotification::ThreadStarted(_) => break,
|
||||
_ => {}
|
||||
}
|
||||
}
|
||||
})
|
||||
.await?;
|
||||
app_server.shutdown().await?;
|
||||
|
||||
if read_only_allowed {
|
||||
@@ -117,7 +140,7 @@ async fn managed_workspace_default_respects_read_only_availability() -> color_ey
|
||||
),
|
||||
(
|
||||
Some(":read-only".to_string()),
|
||||
AskForApproval::Never,
|
||||
AskForApproval::OnRequest,
|
||||
SandboxPolicy::ReadOnly {
|
||||
network_access: false,
|
||||
},
|
||||
|
||||
Reference in New Issue
Block a user