mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
## Why This PR prepares the stack to enable Clippy await-holding lints that were left disabled in #18178. The mechanical lock-scope cleanup is handled separately; this PR is the documentation/configuration layer for the remaining await-across-guard sites. Without explicit annotations, reviewers and future maintainers cannot tell whether an await-holding warning is a real concurrency smell or an intentional serialization boundary. ## What changed - Configures `clippy.toml` so `await_holding_invalid_type` also covers `tokio::sync::{MutexGuard,RwLockReadGuard,RwLockWriteGuard}`. - Adds targeted `#[expect(clippy::await_holding_invalid_type, reason = ...)]` annotations for intentional async guard lifetimes. - Documents the main categories of intentional cases: active-turn state transitions that must remain atomic, session-owned MCP manager accesses, remote-control websocket serialization, JS REPL kernel/process serialization, OAuth persistence, external bearer token refresh serialization, and tests that intentionally serialize shared global or session-owned state. - For external bearer token refresh, documents the existing serialization boundary: holding `cached_token` across the provider command prevents concurrent cache misses from starting duplicate refresh commands, and the current behavior is small enough that an explicit expectation is easier to maintain than adding another synchronization primitive. ## Verification - `cargo clippy -p codex-login --all-targets` - `cargo clippy -p codex-connectors --all-targets` - `cargo clippy -p codex-core --all-targets` - The follow-up PR #18698 enables `await_holding_invalid_type` and `await_holding_lock` as workspace `deny` lints, so any undocumented remaining offender will fail Clippy. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/18423). * #18698 * __->__ #18423
308 lines
9.5 KiB
Rust
308 lines
9.5 KiB
Rust
use std::collections::HashSet;
|
|
use std::sync::Arc;
|
|
|
|
use crate::function_tool::FunctionCallError;
|
|
use crate::session::tests::make_session_and_context;
|
|
use crate::tools::context::ToolPayload;
|
|
use crate::turn_diff_tracker::TurnDiffTracker;
|
|
use codex_protocol::models::ResponseItem;
|
|
use codex_tools::ToolName;
|
|
use tokio_util::sync::CancellationToken;
|
|
|
|
use super::ToolCall;
|
|
use super::ToolCallSource;
|
|
use super::ToolRouter;
|
|
use super::ToolRouterParams;
|
|
|
|
#[tokio::test]
|
|
#[expect(
|
|
clippy::await_holding_invalid_type,
|
|
reason = "test builds a router from session-owned MCP manager state"
|
|
)]
|
|
async fn js_repl_tools_only_blocks_direct_tool_calls() -> anyhow::Result<()> {
|
|
let (session, mut turn) = make_session_and_context().await;
|
|
turn.tools_config.js_repl_tools_only = true;
|
|
|
|
let session = Arc::new(session);
|
|
let turn = Arc::new(turn);
|
|
let mcp_tools = session
|
|
.services
|
|
.mcp_connection_manager
|
|
.read()
|
|
.await
|
|
.list_all_tools()
|
|
.await;
|
|
let deferred_mcp_tools = Some(mcp_tools.clone());
|
|
let router = ToolRouter::from_config(
|
|
&turn.tools_config,
|
|
ToolRouterParams {
|
|
deferred_mcp_tools,
|
|
mcp_tools: Some(mcp_tools),
|
|
unavailable_called_tools: Vec::new(),
|
|
parallel_mcp_server_names: HashSet::new(),
|
|
discoverable_tools: None,
|
|
dynamic_tools: turn.dynamic_tools.as_slice(),
|
|
},
|
|
);
|
|
|
|
let call = ToolCall {
|
|
tool_name: ToolName::plain("shell"),
|
|
call_id: "call-1".to_string(),
|
|
payload: ToolPayload::Function {
|
|
arguments: "{}".to_string(),
|
|
},
|
|
};
|
|
let tracker = Arc::new(tokio::sync::Mutex::new(TurnDiffTracker::new()));
|
|
let err = router
|
|
.dispatch_tool_call_with_code_mode_result(
|
|
session,
|
|
turn,
|
|
CancellationToken::new(),
|
|
tracker,
|
|
call,
|
|
ToolCallSource::Direct,
|
|
)
|
|
.await
|
|
.err()
|
|
.expect("direct tool calls should be blocked");
|
|
let FunctionCallError::RespondToModel(message) = err else {
|
|
panic!("expected RespondToModel, got {err:?}");
|
|
};
|
|
assert!(message.contains("direct tool calls are disabled"));
|
|
|
|
Ok(())
|
|
}
|
|
|
|
#[tokio::test]
|
|
#[expect(
|
|
clippy::await_holding_invalid_type,
|
|
reason = "test builds a router from session-owned MCP manager state"
|
|
)]
|
|
async fn js_repl_tools_only_allows_js_repl_source_calls() -> anyhow::Result<()> {
|
|
let (session, mut turn) = make_session_and_context().await;
|
|
turn.tools_config.js_repl_tools_only = true;
|
|
|
|
let session = Arc::new(session);
|
|
let turn = Arc::new(turn);
|
|
let mcp_tools = session
|
|
.services
|
|
.mcp_connection_manager
|
|
.read()
|
|
.await
|
|
.list_all_tools()
|
|
.await;
|
|
let deferred_mcp_tools = Some(mcp_tools.clone());
|
|
let router = ToolRouter::from_config(
|
|
&turn.tools_config,
|
|
ToolRouterParams {
|
|
deferred_mcp_tools,
|
|
mcp_tools: Some(mcp_tools),
|
|
unavailable_called_tools: Vec::new(),
|
|
parallel_mcp_server_names: HashSet::new(),
|
|
discoverable_tools: None,
|
|
dynamic_tools: turn.dynamic_tools.as_slice(),
|
|
},
|
|
);
|
|
|
|
let call = ToolCall {
|
|
tool_name: ToolName::plain("shell"),
|
|
call_id: "call-2".to_string(),
|
|
payload: ToolPayload::Function {
|
|
arguments: "{}".to_string(),
|
|
},
|
|
};
|
|
let tracker = Arc::new(tokio::sync::Mutex::new(TurnDiffTracker::new()));
|
|
let err = router
|
|
.dispatch_tool_call_with_code_mode_result(
|
|
session,
|
|
turn,
|
|
CancellationToken::new(),
|
|
tracker,
|
|
call,
|
|
ToolCallSource::JsRepl,
|
|
)
|
|
.await
|
|
.err()
|
|
.expect("shell call with empty args should fail");
|
|
let message = err.to_string();
|
|
assert!(
|
|
!message.contains("direct tool calls are disabled"),
|
|
"js_repl source should bypass direct-call policy gate"
|
|
);
|
|
|
|
Ok(())
|
|
}
|
|
|
|
#[tokio::test]
|
|
async fn js_repl_tools_only_blocks_namespaced_js_repl_tool() -> anyhow::Result<()> {
|
|
let (session, mut turn) = make_session_and_context().await;
|
|
turn.tools_config.js_repl_tools_only = true;
|
|
|
|
let session = Arc::new(session);
|
|
let turn = Arc::new(turn);
|
|
let router = ToolRouter::from_config(
|
|
&turn.tools_config,
|
|
ToolRouterParams {
|
|
deferred_mcp_tools: None,
|
|
mcp_tools: None,
|
|
unavailable_called_tools: Vec::new(),
|
|
parallel_mcp_server_names: HashSet::new(),
|
|
discoverable_tools: None,
|
|
dynamic_tools: turn.dynamic_tools.as_slice(),
|
|
},
|
|
);
|
|
|
|
let call = ToolCall {
|
|
tool_name: ToolName::namespaced("mcp__server__", "js_repl"),
|
|
call_id: "call-namespaced-js-repl".to_string(),
|
|
payload: ToolPayload::Mcp {
|
|
server: "server".to_string(),
|
|
tool: "js_repl".to_string(),
|
|
raw_arguments: "{}".to_string(),
|
|
},
|
|
};
|
|
let tracker = Arc::new(tokio::sync::Mutex::new(TurnDiffTracker::new()));
|
|
let err = router
|
|
.dispatch_tool_call_with_code_mode_result(
|
|
session,
|
|
turn,
|
|
CancellationToken::new(),
|
|
tracker,
|
|
call,
|
|
ToolCallSource::Direct,
|
|
)
|
|
.await
|
|
.err()
|
|
.expect("namespaced js_repl calls should be blocked");
|
|
let FunctionCallError::RespondToModel(message) = err else {
|
|
panic!("expected RespondToModel, got {err:?}");
|
|
};
|
|
assert!(message.contains("direct tool calls are disabled"));
|
|
|
|
Ok(())
|
|
}
|
|
|
|
#[tokio::test]
|
|
#[expect(
|
|
clippy::await_holding_invalid_type,
|
|
reason = "test builds a router from session-owned MCP manager state"
|
|
)]
|
|
async fn parallel_support_does_not_match_namespaced_local_tool_names() -> anyhow::Result<()> {
|
|
let (session, turn) = make_session_and_context().await;
|
|
let mcp_tools = session
|
|
.services
|
|
.mcp_connection_manager
|
|
.read()
|
|
.await
|
|
.list_all_tools()
|
|
.await;
|
|
let router = ToolRouter::from_config(
|
|
&turn.tools_config,
|
|
ToolRouterParams {
|
|
deferred_mcp_tools: None,
|
|
mcp_tools: Some(mcp_tools),
|
|
unavailable_called_tools: Vec::new(),
|
|
parallel_mcp_server_names: HashSet::new(),
|
|
discoverable_tools: None,
|
|
dynamic_tools: turn.dynamic_tools.as_slice(),
|
|
},
|
|
);
|
|
|
|
let parallel_tool_name = ["shell", "local_shell", "exec_command", "shell_command"]
|
|
.into_iter()
|
|
.find(|name| {
|
|
router.tool_supports_parallel(&ToolCall {
|
|
tool_name: ToolName::plain(*name),
|
|
call_id: "call-parallel-tool".to_string(),
|
|
payload: ToolPayload::Function {
|
|
arguments: "{}".to_string(),
|
|
},
|
|
})
|
|
})
|
|
.expect("test session should expose a parallel shell-like tool");
|
|
|
|
assert!(!router.tool_supports_parallel(&ToolCall {
|
|
tool_name: ToolName::namespaced("mcp__server__", parallel_tool_name),
|
|
call_id: "call-namespaced-tool".to_string(),
|
|
payload: ToolPayload::Function {
|
|
arguments: "{}".to_string(),
|
|
},
|
|
}));
|
|
|
|
Ok(())
|
|
}
|
|
|
|
#[tokio::test]
|
|
async fn build_tool_call_uses_namespace_for_registry_name() -> anyhow::Result<()> {
|
|
let (session, _) = make_session_and_context().await;
|
|
let session = Arc::new(session);
|
|
let tool_name = "create_event".to_string();
|
|
|
|
let call = ToolRouter::build_tool_call(
|
|
&session,
|
|
ResponseItem::FunctionCall {
|
|
id: None,
|
|
name: tool_name.clone(),
|
|
namespace: Some("mcp__codex_apps__calendar".to_string()),
|
|
arguments: "{}".to_string(),
|
|
call_id: "call-namespace".to_string(),
|
|
},
|
|
)
|
|
.await?
|
|
.expect("function_call should produce a tool call");
|
|
|
|
assert_eq!(
|
|
call.tool_name,
|
|
ToolName::namespaced("mcp__codex_apps__calendar", tool_name)
|
|
);
|
|
assert_eq!(call.call_id, "call-namespace");
|
|
match call.payload {
|
|
ToolPayload::Function { arguments } => {
|
|
assert_eq!(arguments, "{}");
|
|
}
|
|
other => panic!("expected function payload, got {other:?}"),
|
|
}
|
|
|
|
Ok(())
|
|
}
|
|
|
|
#[tokio::test]
|
|
async fn mcp_parallel_support_uses_exact_payload_server() -> anyhow::Result<()> {
|
|
let (_, turn) = make_session_and_context().await;
|
|
let router = ToolRouter::from_config(
|
|
&turn.tools_config,
|
|
ToolRouterParams {
|
|
deferred_mcp_tools: None,
|
|
mcp_tools: None,
|
|
unavailable_called_tools: Vec::new(),
|
|
parallel_mcp_server_names: HashSet::from(["echo".to_string()]),
|
|
discoverable_tools: None,
|
|
dynamic_tools: turn.dynamic_tools.as_slice(),
|
|
},
|
|
);
|
|
|
|
let deferred_call = ToolCall {
|
|
tool_name: ToolName::namespaced("mcp__echo__", "query_with_delay"),
|
|
call_id: "call-deferred".to_string(),
|
|
payload: ToolPayload::Mcp {
|
|
server: "echo".to_string(),
|
|
tool: "query_with_delay".to_string(),
|
|
raw_arguments: "{}".to_string(),
|
|
},
|
|
};
|
|
assert!(router.tool_supports_parallel(&deferred_call));
|
|
|
|
let different_server_call = ToolCall {
|
|
tool_name: ToolName::namespaced("mcp__hello_echo__", "query_with_delay"),
|
|
call_id: "call-other-server".to_string(),
|
|
payload: ToolPayload::Mcp {
|
|
server: "hello_echo".to_string(),
|
|
tool: "query_with_delay".to_string(),
|
|
raw_arguments: "{}".to_string(),
|
|
},
|
|
};
|
|
assert!(!router.tool_supports_parallel(&different_server_call));
|
|
|
|
Ok(())
|
|
}
|