mirror of
https://github.com/openai/codex.git
synced 2026-10-20 17:38:45 +00:00
Gate startup drafts on runtime MCP servers
This commit is contained in:
@@ -234,6 +234,7 @@ fn sample_thread_start_response(
|
||||
model_provider: "openai".to_string(),
|
||||
service_tier: None,
|
||||
cwd: test_path_buf("/tmp").abs(),
|
||||
mcp_server_names: Vec::new(),
|
||||
runtime_workspace_roots: Vec::new(),
|
||||
instruction_sources: Vec::new(),
|
||||
approval_policy: AppServerAskForApproval::OnRequest,
|
||||
|
||||
@@ -309,6 +309,7 @@ fn sample_thread_start_response() -> ClientResponsePayload {
|
||||
model_provider: "openai".to_string(),
|
||||
service_tier: None,
|
||||
cwd: test_path_buf("/tmp").abs(),
|
||||
mcp_server_names: Vec::new(),
|
||||
runtime_workspace_roots: Vec::new(),
|
||||
instruction_sources: Vec::new(),
|
||||
approval_policy: AppServerAskForApproval::OnRequest,
|
||||
|
||||
+8
@@ -19531,6 +19531,14 @@
|
||||
},
|
||||
"type": "array"
|
||||
},
|
||||
"mcpServerNames": {
|
||||
"default": [],
|
||||
"description": "Names of the effective MCP servers in the thread's initial runtime.",
|
||||
"items": {
|
||||
"type": "string"
|
||||
},
|
||||
"type": "array"
|
||||
},
|
||||
"model": {
|
||||
"type": "string"
|
||||
},
|
||||
|
||||
+8
@@ -17310,6 +17310,14 @@
|
||||
},
|
||||
"type": "array"
|
||||
},
|
||||
"mcpServerNames": {
|
||||
"default": [],
|
||||
"description": "Names of the effective MCP servers in the thread's initial runtime.",
|
||||
"items": {
|
||||
"type": "string"
|
||||
},
|
||||
"type": "array"
|
||||
},
|
||||
"model": {
|
||||
"type": "string"
|
||||
},
|
||||
|
||||
@@ -2430,6 +2430,14 @@
|
||||
},
|
||||
"type": "array"
|
||||
},
|
||||
"mcpServerNames": {
|
||||
"default": [],
|
||||
"description": "Names of the effective MCP servers in the thread's initial runtime.",
|
||||
"items": {
|
||||
"type": "string"
|
||||
},
|
||||
"type": "array"
|
||||
},
|
||||
"model": {
|
||||
"type": "string"
|
||||
},
|
||||
|
||||
@@ -10,6 +10,9 @@ import type { SandboxPolicy } from "./SandboxPolicy";
|
||||
import type { Thread } from "./Thread";
|
||||
|
||||
export type ThreadStartResponse = {thread: Thread, model: string, modelProvider: string, serviceTier: string | null, cwd: AbsolutePathBuf, /**
|
||||
* Names of the effective MCP servers in the thread's initial runtime.
|
||||
*/
|
||||
mcpServerNames: Array<string>, /**
|
||||
* Environment-native paths to instruction source files currently loaded for this thread.
|
||||
*/
|
||||
instructionSources: Array<LegacyAppPathString>, approvalPolicy: AskForApproval, /**
|
||||
|
||||
@@ -2608,6 +2608,7 @@ mod tests {
|
||||
model_provider: "openai".to_string(),
|
||||
service_tier: None,
|
||||
cwd,
|
||||
mcp_server_names: vec!["example".to_string()],
|
||||
runtime_workspace_roots: Vec::new(),
|
||||
instruction_sources: vec![
|
||||
codex_utils_path_uri::LegacyAppPathString::from_abs_path(&absolute_path(
|
||||
@@ -2661,6 +2662,7 @@ mod tests {
|
||||
"modelProvider": "openai",
|
||||
"serviceTier": null,
|
||||
"cwd": absolute_path_string("tmp"),
|
||||
"mcpServerNames": ["example"],
|
||||
"runtimeWorkspaceRoots": [],
|
||||
"instructionSources": [absolute_path_string("tmp/AGENTS.md")],
|
||||
"approvalPolicy": "on-request",
|
||||
|
||||
@@ -173,6 +173,9 @@ pub struct ThreadStartResponse {
|
||||
pub model_provider: String,
|
||||
pub service_tier: Option<String>,
|
||||
pub cwd: AbsolutePathBuf,
|
||||
/// Names of the effective MCP servers in the thread's initial runtime.
|
||||
#[serde(default)]
|
||||
pub mcp_server_names: Vec<String>,
|
||||
/// Thread-scoped runtime workspace roots used to materialize
|
||||
/// `:workspace_roots`.
|
||||
#[experimental("thread/start.runtimeWorkspaceRoots")]
|
||||
|
||||
@@ -140,7 +140,7 @@ Example with notification opt-out:
|
||||
- `thread/start` — create a new thread; emits `thread/started` (including the current `thread.status`) and auto-subscribes you to turn/item events for that thread. When the request includes a `cwd` and the resolved sandbox is `workspace-write` or full access, app-server also marks that project as trusted in the user `config.toml`. Pass `sessionStartSource: "clear"` when starting a replacement thread after clearing the current session so `SessionStart` hooks receive `source: "clear"` instead of the default `"startup"`. Experimental `allowProviderModelFallback` lets providers backed by an authoritative static model catalog replace an unavailable requested `model` with the catalog default; dynamic or cached catalogs preserve the requested model. Experimental `runtimeWorkspaceRoots` replaces the thread-scoped runtime workspace roots used to materialize `:workspace_roots`; paths must be absolute. For permissions, prefer experimental `permissions` profile selection by id; the legacy `sandbox` shorthand is still accepted but cannot be combined with `permissions`. Deprecated experimental `multiAgentMode` is ignored; use Ultra reasoning effort for proactive multi-agent behavior. Experimental `environments` selects the sticky execution environments for turns on the thread; omit it to use the server default, pass `[]` to disable environments, or pass explicit environment ids with per-environment `cwd`. Experimental `selectedCapabilityRoots` selects environment-owned plugin or standalone-skill roots using environment-native absolute paths. Skills found below those roots are listed and read through the owning environment. Stdio MCP servers declared by selected plugins are started in that environment, and HTTP MCP connections use that environment's HTTP client.
|
||||
- `thread/resume` — reopen an existing thread by id so subsequent `turn/start` calls append to it. Accepts the same permission override rules as `thread/start`.
|
||||
- `thread/fork` — fork an existing thread into a new thread id by copying the stored history; pass an optional `lastTurnId` to copy history only through that turn, inclusive, and drop later turns from the fork. An in-progress `lastTurnId` is rejected. If `lastTurnId` is null while the source thread is mid-turn, the fork records the same interruption marker as `turn/interrupt` instead of inheriting an unmarked partial turn suffix. The returned `thread.forkedFromId` points at the source thread when known. Accepts `ephemeral: true` for an in-memory temporary fork, emits `thread/started` (including the current `thread.status`), and auto-subscribes you to turn/item events for the new thread. Experimental clients can pass `excludeTurns: true` when they plan to page fork history via `thread/turns/list` instead of receiving the full turn array immediately. Accepts the same permission override rules as `thread/start`.
|
||||
- `thread/start`, `thread/resume`, and `thread/fork` responses include the legacy `sandbox` compatibility projection. `instructionSources` lists loaded instruction files using each source environment's native absolute path syntax, including files loaded from remote environments. Experimental clients can read `runtimeWorkspaceRoots` for the thread-scoped runtime roots and `activePermissionProfile` for the named or implicit built-in profile identity/provenance when known. Their deprecated experimental `multiAgentMode` field, and the corresponding thread setting, always report `explicitRequestOnly`; Ultra reasoning effort is the source of proactive multi-agent behavior.
|
||||
- `thread/start`, `thread/resume`, and `thread/fork` responses include the legacy `sandbox` compatibility projection. `instructionSources` lists loaded instruction files using each source environment's native absolute path syntax, including files loaded from remote environments. The `thread/start` response also includes `mcpServerNames`, the effective MCP server names in the thread's initial runtime. Experimental clients can read `runtimeWorkspaceRoots` for the thread-scoped runtime roots and `activePermissionProfile` for the named or implicit built-in profile identity/provenance when known. Their deprecated experimental `multiAgentMode` field, and the corresponding thread setting, always report `explicitRequestOnly`; Ultra reasoning effort is the source of proactive multi-agent behavior.
|
||||
- `thread/list` — page through stored threads; supports cursor-based pagination and optional `modelProviders`, `sourceKinds`, `archived`, `cwd`, and `searchTerm` filters. Experimental clients can use `parentThreadId` for direct spawned children or `ancestorThreadId` for spawned descendants at any depth; the two filters are mutually exclusive. Review and Guardian threads are not included because they do not participate in that spawn-edge lifecycle. Each returned `thread` includes `status` (`ThreadStatus`), defaulting to `notLoaded` when the thread is not currently loaded. Subagent threads also include `parentThreadId` when the immediate parent is known.
|
||||
- `thread/loaded/list` — list the thread ids currently loaded in memory.
|
||||
- `thread/read` — read a stored thread by id without resuming it; optionally include turns via `includeTurns`. The returned `thread` includes `status` (`ThreadStatus`), defaulting to `notLoaded` when the thread is not currently loaded.
|
||||
|
||||
@@ -1213,6 +1213,14 @@ impl ThreadRequestProcessor {
|
||||
.await?;
|
||||
|
||||
let instruction_sources = thread.legacy_instruction_sources().await;
|
||||
let mut mcp_server_names = thread
|
||||
.current_mcp_runtime()
|
||||
.await
|
||||
.manager()
|
||||
.server_names()
|
||||
.map(str::to_owned)
|
||||
.collect::<Vec<_>>();
|
||||
mcp_server_names.sort();
|
||||
let config_snapshot = thread
|
||||
.config_snapshot()
|
||||
.instrument(tracing::info_span!(
|
||||
@@ -1282,6 +1290,7 @@ impl ThreadRequestProcessor {
|
||||
model_provider: config_snapshot.model_provider_id,
|
||||
service_tier: config_snapshot.service_tier,
|
||||
cwd,
|
||||
mcp_server_names,
|
||||
runtime_workspace_roots: config_snapshot.workspace_roots,
|
||||
instruction_sources,
|
||||
approval_policy: config_snapshot.approval_policy.into(),
|
||||
|
||||
@@ -70,6 +70,20 @@ url = "{mcp_server_url}/mcp"
|
||||
let mut mcp = TestAppServer::new(codex_home.path()).await?;
|
||||
timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??;
|
||||
|
||||
let thread_start_id = mcp
|
||||
.send_thread_start_request(ThreadStartParams::default())
|
||||
.await?;
|
||||
let thread_start_response = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(thread_start_id)),
|
||||
)
|
||||
.await??;
|
||||
let thread_start_response: ThreadStartResponse = to_response(thread_start_response)?;
|
||||
assert_eq!(
|
||||
thread_start_response.mcp_server_names,
|
||||
vec!["some-server".to_string()]
|
||||
);
|
||||
|
||||
let request_id = mcp
|
||||
.send_list_mcp_server_status_request(ListMcpServerStatusParams {
|
||||
cursor: None,
|
||||
|
||||
@@ -370,6 +370,10 @@ impl McpConnectionManager {
|
||||
!self.clients.is_empty()
|
||||
}
|
||||
|
||||
pub fn server_names(&self) -> impl Iterator<Item = &str> {
|
||||
self.clients.keys().map(String::as_str)
|
||||
}
|
||||
|
||||
pub(crate) fn contains_server(&self, server_name: &str) -> bool {
|
||||
self.clients.contains_key(server_name)
|
||||
}
|
||||
|
||||
@@ -782,6 +782,7 @@ fn sample_thread_start_response() -> ThreadStartResponse {
|
||||
model_provider: "openai".to_string(),
|
||||
service_tier: None,
|
||||
cwd: test_path_buf("/tmp").abs(),
|
||||
mcp_server_names: Vec::new(),
|
||||
runtime_workspace_roots: Vec::new(),
|
||||
instruction_sources: Vec::new(),
|
||||
approval_policy: codex_app_server_protocol::AskForApproval::OnRequest,
|
||||
|
||||
@@ -1116,9 +1116,7 @@ See the Codex keymap documentation for supported actions and examples."
|
||||
}
|
||||
|
||||
if allow_startup_text && let Some(startup_text) = tui.take_startup_text()? {
|
||||
let expected_mcp_servers = app.expected_mcp_startup_servers();
|
||||
app.chat_widget
|
||||
.restore_startup_draft(&startup_text, expected_mcp_servers);
|
||||
app.chat_widget.restore_startup_draft(&startup_text);
|
||||
}
|
||||
|
||||
let event_stream_started_at = Instant::now();
|
||||
|
||||
@@ -16,20 +16,14 @@ use codex_app_server_protocol::ServerNotification;
|
||||
use codex_app_server_protocol::ServerRequest;
|
||||
|
||||
impl App {
|
||||
pub(super) fn expected_mcp_startup_servers(&self) -> Vec<String> {
|
||||
if self.app_server_target.uses_remote_workspace() {
|
||||
return Vec::new();
|
||||
}
|
||||
self.config
|
||||
pub(super) fn refresh_mcp_startup_expected_servers_from_config(&mut self) {
|
||||
let enabled_config_mcp_servers: Vec<String> = self
|
||||
.config
|
||||
.mcp_servers
|
||||
.get()
|
||||
.iter()
|
||||
.filter_map(|(name, server)| server.enabled.then_some(name.clone()))
|
||||
.collect()
|
||||
}
|
||||
|
||||
pub(super) fn refresh_mcp_startup_expected_servers(&mut self) {
|
||||
let enabled_config_mcp_servers = self.expected_mcp_startup_servers();
|
||||
.collect();
|
||||
self.chat_widget
|
||||
.set_mcp_startup_expected_servers(enabled_config_mcp_servers);
|
||||
}
|
||||
@@ -45,7 +39,7 @@ impl App {
|
||||
skipped,
|
||||
"app-server event consumer lagged; dropping ignored events"
|
||||
);
|
||||
self.refresh_mcp_startup_expected_servers();
|
||||
self.refresh_mcp_startup_expected_servers_from_config();
|
||||
self.chat_widget.finish_mcp_startup_after_lag();
|
||||
}
|
||||
AppServerEvent::ServerNotification(notification) => {
|
||||
@@ -79,7 +73,7 @@ impl App {
|
||||
}
|
||||
}
|
||||
ServerNotification::McpServerStatusUpdated(_) => {
|
||||
self.refresh_mcp_startup_expected_servers();
|
||||
self.refresh_mcp_startup_expected_servers_from_config();
|
||||
}
|
||||
ServerNotification::AccountRateLimitsUpdated(notification) => {
|
||||
self.chat_widget
|
||||
|
||||
@@ -512,9 +512,14 @@ impl App {
|
||||
self.chat_widget
|
||||
.set_queue_submissions_until_session_configured(/*queue*/ false);
|
||||
match result {
|
||||
Ok(started) => {
|
||||
self.enqueue_primary_thread_session(started.session, started.turns)
|
||||
.await?;
|
||||
Ok(AppServerStartedThread {
|
||||
session,
|
||||
turns,
|
||||
mcp_server_names,
|
||||
}) => {
|
||||
self.chat_widget
|
||||
.set_startup_draft_expected_mcp_servers(mcp_server_names);
|
||||
self.enqueue_primary_thread_session(session, turns).await?;
|
||||
self.chat_widget.maybe_send_next_queued_input();
|
||||
}
|
||||
Err(err) => {
|
||||
|
||||
@@ -3529,6 +3529,7 @@ async fn primary_thread_ignores_child_mcp_startup_notifications() {
|
||||
AppServerStartedThread {
|
||||
session: test_thread_session(child_thread_id, test_path_buf("/tmp/child")),
|
||||
turns: Vec::new(),
|
||||
mcp_server_names: Vec::new(),
|
||||
},
|
||||
&mut child_snapshot,
|
||||
)
|
||||
@@ -5495,6 +5496,7 @@ async fn refreshed_snapshot_session_persists_resumed_turns() {
|
||||
AppServerStartedThread {
|
||||
session: resumed_session.clone(),
|
||||
turns: resumed_turns.clone(),
|
||||
mcp_server_names: Vec::new(),
|
||||
},
|
||||
&mut snapshot,
|
||||
)
|
||||
|
||||
@@ -138,13 +138,13 @@ fn startup_waiting_gate_not_applied_for_resume_or_fork_session_selection() {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn restored_startup_draft_waits_for_initial_mcp_settlement() {
|
||||
async fn restored_startup_draft_waits_for_effective_mcp_settlement() {
|
||||
let (mut app, _app_event_rx, mut op_rx) = make_test_app_with_channels().await;
|
||||
app.pending_startup_thread_start = true;
|
||||
app.chat_widget
|
||||
.set_queue_submissions_until_session_configured(/*queue*/ true);
|
||||
app.chat_widget
|
||||
.restore_startup_draft("captured during startup", ["test".to_string()]);
|
||||
.restore_startup_draft("captured during startup");
|
||||
let enter = KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE);
|
||||
app.chat_widget.handle_key_event(enter);
|
||||
|
||||
@@ -165,6 +165,7 @@ async fn restored_startup_draft_waits_for_initial_mcp_settlement() {
|
||||
Ok(AppServerStartedThread {
|
||||
session: test_thread_session(thread_id, test_path_buf("/tmp/project")),
|
||||
turns: Vec::new(),
|
||||
mcp_server_names: vec!["test".to_string()],
|
||||
}),
|
||||
)
|
||||
.await
|
||||
@@ -245,54 +246,6 @@ async fn startup_without_restored_draft_uses_normal_queued_submission() {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn remote_startup_draft_does_not_wait_for_local_mcp_servers() {
|
||||
let (mut app, _app_event_rx, mut op_rx) = make_test_app_with_channels().await;
|
||||
let local_only_config = toml::from_str::<toml::Value>("command = 'true'")
|
||||
.expect("test MCP config should parse")
|
||||
.try_into()
|
||||
.expect("test MCP config should deserialize");
|
||||
app.config
|
||||
.mcp_servers
|
||||
.set(std::collections::HashMap::from([(
|
||||
"local-only".to_string(),
|
||||
local_only_config,
|
||||
)]))
|
||||
.expect("test MCP servers should accept any configuration");
|
||||
assert_eq!(
|
||||
app.expected_mcp_startup_servers(),
|
||||
vec!["local-only".to_string()]
|
||||
);
|
||||
app.app_server_target = crate::AppServerTarget::Remote {
|
||||
endpoint: codex_app_server_client::RemoteAppServerEndpoint::WebSocket {
|
||||
websocket_url: "wss://remote.example.test".to_string(),
|
||||
auth_token: None,
|
||||
},
|
||||
};
|
||||
let expected_mcp_servers = app.expected_mcp_startup_servers();
|
||||
assert_eq!(expected_mcp_servers, Vec::<String>::new());
|
||||
app.chat_widget
|
||||
.restore_startup_draft("captured during startup", expected_mcp_servers);
|
||||
|
||||
app.chat_widget.handle_thread_session(test_thread_session(
|
||||
ThreadId::new(),
|
||||
test_path_buf("/tmp/remote-project"),
|
||||
));
|
||||
app.chat_widget
|
||||
.handle_key_event(KeyEvent::new(KeyCode::Enter, KeyModifiers::NONE));
|
||||
|
||||
match next_user_turn_op(&mut op_rx) {
|
||||
Op::UserTurn { items, .. } => assert_eq!(
|
||||
items,
|
||||
vec![UserInput::Text {
|
||||
text: "captured during startup".to_string(),
|
||||
text_elements: Vec::new(),
|
||||
}]
|
||||
),
|
||||
other => panic!("expected remote startup draft submission, got {other:?}"),
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn startup_thread_start_failure_returns_error() {
|
||||
let (mut app, _app_event_rx, _op_rx) = make_test_app_with_channels().await;
|
||||
@@ -353,6 +306,7 @@ fn stale_startup_thread_started_removes_local_routing_state() -> Result<()> {
|
||||
Ok(AppServerStartedThread {
|
||||
session: test_thread_session(stale_thread_id, test_path_buf("/tmp/project")),
|
||||
turns: Vec::new(),
|
||||
mcp_server_names: Vec::new(),
|
||||
}),
|
||||
)
|
||||
.await?;
|
||||
|
||||
@@ -1237,7 +1237,7 @@ impl App {
|
||||
started: AppServerStartedThread,
|
||||
snapshot: &mut ThreadEventSnapshot,
|
||||
) {
|
||||
let AppServerStartedThread { session, turns } = started;
|
||||
let AppServerStartedThread { session, turns, .. } = started;
|
||||
if let Some(channel) = self.thread_event_channels.get(&thread_id) {
|
||||
let mut store = channel.store.lock().await;
|
||||
store.set_session(session.clone(), turns.clone());
|
||||
@@ -1316,7 +1316,7 @@ impl App {
|
||||
snapshot: ThreadEventSnapshot,
|
||||
resume_restored_queue: bool,
|
||||
) {
|
||||
self.refresh_mcp_startup_expected_servers();
|
||||
self.refresh_mcp_startup_expected_servers_from_config();
|
||||
let should_buffer_replay = !snapshot.turns.is_empty() || !snapshot.events.is_empty();
|
||||
if should_buffer_replay {
|
||||
self.app_event_tx
|
||||
|
||||
@@ -203,6 +203,7 @@ impl ThreadParamsMode {
|
||||
pub(crate) struct AppServerStartedThread {
|
||||
pub(crate) session: ThreadSessionState,
|
||||
pub(crate) turns: Vec<Turn>,
|
||||
pub(crate) mcp_server_names: Vec<String>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, PartialEq, Eq)]
|
||||
@@ -1531,6 +1532,7 @@ async fn started_thread_from_start_response(
|
||||
Ok(AppServerStartedThread {
|
||||
session,
|
||||
turns: response.thread.turns,
|
||||
mcp_server_names: response.mcp_server_names,
|
||||
})
|
||||
}
|
||||
|
||||
@@ -1546,6 +1548,7 @@ async fn started_thread_from_resume_response(
|
||||
Ok(AppServerStartedThread {
|
||||
session,
|
||||
turns: response.thread.turns,
|
||||
mcp_server_names: Vec::new(),
|
||||
})
|
||||
}
|
||||
|
||||
@@ -1561,6 +1564,7 @@ async fn started_thread_from_fork_response(
|
||||
Ok(AppServerStartedThread {
|
||||
session,
|
||||
turns: response.thread.turns,
|
||||
mcp_server_names: Vec::new(),
|
||||
})
|
||||
}
|
||||
|
||||
|
||||
@@ -7,14 +7,21 @@
|
||||
use super::*;
|
||||
|
||||
impl ChatWidget {
|
||||
pub(crate) fn restore_startup_draft<I>(&mut self, text: &str, expected_mcp_servers: I)
|
||||
pub(crate) fn restore_startup_draft(&mut self, text: &str) {
|
||||
self.insert_str(text);
|
||||
self.startup_draft_pending_mcp_servers = Some(HashSet::new());
|
||||
self.bottom_pane
|
||||
.set_startup_draft_submission_blocked(/*blocked*/ true);
|
||||
}
|
||||
|
||||
pub(crate) fn set_startup_draft_expected_mcp_servers<I>(&mut self, server_names: I)
|
||||
where
|
||||
I: IntoIterator<Item = String>,
|
||||
{
|
||||
self.insert_str(text);
|
||||
self.startup_draft_pending_mcp_servers = Some(expected_mcp_servers.into_iter().collect());
|
||||
self.bottom_pane
|
||||
.set_startup_draft_submission_blocked(/*blocked*/ true);
|
||||
let Some(pending) = &mut self.startup_draft_pending_mcp_servers else {
|
||||
return;
|
||||
};
|
||||
*pending = server_names.into_iter().collect();
|
||||
self.finish_startup_draft_protection_if_ready();
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user