mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Represent dynamic tools with explicit namespaces internally (#27365)
Follow-up to #27356. ## Stack note This PR changes Codex's internal dynamic-tool shape while leaving `thread/start` unchanged. App-server therefore converts the existing per-tool input into explicit functions and namespaces before passing it to core. [#27371](https://github.com/openai/codex/pull/27371) updates `thread/start` to use the same explicit shape and removes this temporary conversion. ## Why Dynamic tools repeat namespace metadata on every function. Core should keep one explicit namespace with its member tools so descriptions and membership stay consistent across sessions and runtime planning. ## What changed - Represent dynamic tools as top-level functions or explicit namespaces in protocol and session state. - Read old flat rollout metadata and write the canonical hierarchy. - Flatten namespace members only when registering callable tools. - Keep `thread/start.dynamicTools` flat for now and normalize it at the app-server boundary. New builds can read old rollout metadata. Older builds cannot read newly written hierarchical metadata. ## Test plan - `just test -p codex-app-server thread_start_normalizes_legacy_dynamic_tools_into_model_request` - `just test -p codex-protocol session_meta_normalizes_legacy_dynamic_tools` - `just test -p codex-core resume_restores_dynamic_tools_from_rollout_with_sqlite_enabled` - `just test -p codex-core tool_search_returns_deferred_dynamic_tool_and_routes_follow_up_call` - `just test -p codex-core code_mode_can_call_hidden_dynamic_tools` - `just test -p codex-tools`
This commit is contained in:
committed by
GitHub
Unverified
parent
b3f6f70b68
commit
a292faae5a
@@ -377,7 +377,8 @@ use codex_protocol::config_types::Personality;
|
||||
use codex_protocol::config_types::ReasoningSummary;
|
||||
use codex_protocol::config_types::TrustLevel;
|
||||
use codex_protocol::config_types::WindowsSandboxLevel;
|
||||
use codex_protocol::dynamic_tools::DynamicToolSpec as CoreDynamicToolSpec;
|
||||
use codex_protocol::dynamic_tools::DynamicToolFunctionSpec;
|
||||
use codex_protocol::dynamic_tools::group_dynamic_tools_by_namespace;
|
||||
use codex_protocol::error::CodexErr;
|
||||
use codex_protocol::error::Result as CodexResult;
|
||||
#[cfg(test)]
|
||||
|
||||
@@ -1077,22 +1077,29 @@ impl ThreadRequestProcessor {
|
||||
.default_environment_selections(&config.cwd)
|
||||
});
|
||||
let dynamic_tools = dynamic_tools.unwrap_or_default();
|
||||
// Count callable tools before grouping changes the outer list length.
|
||||
let core_dynamic_tool_count = dynamic_tools.len();
|
||||
let core_dynamic_tools = if dynamic_tools.is_empty() {
|
||||
Vec::new()
|
||||
} else {
|
||||
validate_dynamic_tools(&dynamic_tools).map_err(invalid_request)?;
|
||||
dynamic_tools
|
||||
// Normalize the flat app-server input into core's function and namespace types.
|
||||
let tools = dynamic_tools
|
||||
.into_iter()
|
||||
.map(|tool| CoreDynamicToolSpec {
|
||||
namespace: tool.namespace,
|
||||
name: tool.name,
|
||||
description: tool.description,
|
||||
input_schema: tool.input_schema,
|
||||
defer_loading: tool.defer_loading,
|
||||
.map(|tool| {
|
||||
(
|
||||
tool.namespace,
|
||||
DynamicToolFunctionSpec {
|
||||
name: tool.name,
|
||||
description: tool.description,
|
||||
input_schema: tool.input_schema,
|
||||
defer_loading: tool.defer_loading,
|
||||
},
|
||||
)
|
||||
})
|
||||
.collect()
|
||||
.collect();
|
||||
group_dynamic_tools_by_namespace(tools)
|
||||
};
|
||||
let core_dynamic_tool_count = core_dynamic_tools.len();
|
||||
let mut thread_extension_init = ExtensionDataInit::new();
|
||||
if !selected_capability_roots.is_empty() {
|
||||
thread_extension_init.insert(selected_capability_roots);
|
||||
|
||||
@@ -42,9 +42,8 @@ const DEFAULT_READ_TIMEOUT: Duration = Duration::from_secs(60);
|
||||
#[cfg(not(any(target_os = "macos", windows)))]
|
||||
const DEFAULT_READ_TIMEOUT: Duration = Duration::from_secs(10);
|
||||
|
||||
/// Ensures dynamic tool specs are serialized into the model request payload.
|
||||
#[tokio::test]
|
||||
async fn thread_start_injects_dynamic_tools_into_model_requests() -> Result<()> {
|
||||
async fn thread_start_normalizes_legacy_dynamic_tools_into_model_request() -> Result<()> {
|
||||
let responses = vec![create_final_assistant_message_sse_response("Done")?];
|
||||
let server = create_mock_responses_server_sequence_unchecked(responses).await;
|
||||
|
||||
@@ -54,29 +53,45 @@ async fn thread_start_injects_dynamic_tools_into_model_requests() -> Result<()>
|
||||
let mut mcp = TestAppServer::new(codex_home.path()).await?;
|
||||
timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??;
|
||||
|
||||
// Use a minimal JSON schema so we can assert the tool payload round-trips.
|
||||
let input_schema = json!({
|
||||
let visible_schema = json!({
|
||||
"type": "object",
|
||||
"properties": {
|
||||
"city": { "type": "string" }
|
||||
"ticket_id": { "type": "string" }
|
||||
},
|
||||
"required": ["city"],
|
||||
"required": ["ticket_id"],
|
||||
"additionalProperties": false,
|
||||
});
|
||||
let dynamic_tool = DynamicToolSpec {
|
||||
namespace: None,
|
||||
name: "demo_tool".to_string(),
|
||||
description: "Demo dynamic tool".to_string(),
|
||||
input_schema: input_schema.clone(),
|
||||
defer_loading: false,
|
||||
};
|
||||
|
||||
// Thread start injects dynamic tools into the thread's tool registry.
|
||||
let thread_req = mcp
|
||||
.send_thread_start_request(ThreadStartParams {
|
||||
dynamic_tools: Some(vec![dynamic_tool.clone()]),
|
||||
..Default::default()
|
||||
})
|
||||
.send_raw_request(
|
||||
"thread/start",
|
||||
Some(json!({
|
||||
"dynamicTools": [
|
||||
{
|
||||
"name": "lookup_ticket",
|
||||
"description": "Look up a ticket",
|
||||
"inputSchema": visible_schema,
|
||||
},
|
||||
{
|
||||
"namespace": "legacy_app",
|
||||
"name": "lookup_status",
|
||||
"description": "Look up a ticket status",
|
||||
"inputSchema": visible_schema,
|
||||
"exposeToContext": true
|
||||
},
|
||||
{
|
||||
"namespace": "legacy_app",
|
||||
"name": "update_ticket",
|
||||
"description": "Update a ticket",
|
||||
"inputSchema": {
|
||||
"type": "object",
|
||||
"properties": {},
|
||||
"additionalProperties": false
|
||||
},
|
||||
"exposeToContext": false
|
||||
}
|
||||
]
|
||||
})),
|
||||
)
|
||||
.await?;
|
||||
let thread_resp: JSONRPCResponse = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
@@ -85,13 +100,12 @@ async fn thread_start_injects_dynamic_tools_into_model_requests() -> Result<()>
|
||||
.await??;
|
||||
let ThreadStartResponse { thread, .. } = to_response::<ThreadStartResponse>(thread_resp)?;
|
||||
|
||||
// Start a turn so a model request is issued.
|
||||
let turn_req = mcp
|
||||
.send_turn_start_request(TurnStartParams {
|
||||
thread_id: thread.id.clone(),
|
||||
thread_id: thread.id,
|
||||
client_user_message_id: None,
|
||||
input: vec![V2UserInput::Text {
|
||||
text: "Hello".to_string(),
|
||||
text: "Look up the ticket".to_string(),
|
||||
text_elements: Vec::new(),
|
||||
}],
|
||||
..Default::default()
|
||||
@@ -103,26 +117,42 @@ async fn thread_start_injects_dynamic_tools_into_model_requests() -> Result<()>
|
||||
)
|
||||
.await??;
|
||||
let _turn: TurnStartResponse = to_response::<TurnStartResponse>(turn_resp)?;
|
||||
|
||||
timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_notification_message("turn/completed"),
|
||||
)
|
||||
.await??;
|
||||
|
||||
// Inspect the captured model request to assert the tool spec made it through.
|
||||
let bodies = responses_bodies(&server).await?;
|
||||
let body = bodies
|
||||
.first()
|
||||
.context("expected at least one responses request")?;
|
||||
let tool = find_tool(body, &dynamic_tool.name)
|
||||
.context("expected dynamic tool to be injected into request")?;
|
||||
|
||||
let function =
|
||||
find_tool(&bodies[0], "lookup_ticket").context("expected normalized legacy function")?;
|
||||
assert_eq!(
|
||||
tool.get("description"),
|
||||
Some(&Value::String(dynamic_tool.description.clone()))
|
||||
function,
|
||||
&json!({
|
||||
"type": "function",
|
||||
"name": "lookup_ticket",
|
||||
"description": "Look up a ticket",
|
||||
"strict": false,
|
||||
"parameters": visible_schema,
|
||||
})
|
||||
);
|
||||
let namespace =
|
||||
find_tool(&bodies[0], "legacy_app").context("expected normalized legacy namespace")?;
|
||||
assert_eq!(
|
||||
namespace,
|
||||
&json!({
|
||||
"type": "namespace",
|
||||
"name": "legacy_app",
|
||||
"description": "Tools in the legacy_app namespace.",
|
||||
"tools": [{
|
||||
"type": "function",
|
||||
"name": "lookup_status",
|
||||
"description": "Look up a ticket status",
|
||||
"strict": false,
|
||||
"parameters": visible_schema,
|
||||
}],
|
||||
})
|
||||
);
|
||||
assert_eq!(tool.get("parameters"), Some(&input_schema));
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user