-
Notifications
You must be signed in to change notification settings - Fork 15.4k
Expose selecte namespaces as direct model tools #28825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -192,11 +192,41 @@ fn build_tool_specs_and_registry( | |
| }; | ||
| let mut planned_tools = PlannedTools::default(); | ||
| add_tool_sources(&context, &mut planned_tools); | ||
| apply_direct_model_only_namespace_overrides(turn_context, &mut planned_tools); | ||
| append_tool_search_executor(&context, &mut planned_tools); | ||
| prepend_code_mode_executors(&context, &mut planned_tools); | ||
| build_model_visible_specs_and_registry(turn_context, planned_tools) | ||
| } | ||
|
|
||
| fn apply_direct_model_only_namespace_overrides( | ||
| turn_context: &TurnContext, | ||
| planned_tools: &mut PlannedTools, | ||
| ) { | ||
| for runtime in &mut planned_tools.runtimes { | ||
| let configured = runtime | ||
| .tool_name() | ||
| .namespace | ||
| .as_ref() | ||
| .is_some_and(|namespace| { | ||
| turn_context | ||
| .config | ||
| .code_mode | ||
| .direct_only_tool_namespaces | ||
| .contains(namespace) | ||
| }); | ||
| match runtime.exposure() { | ||
| ToolExposure::Direct | ToolExposure::Deferred if configured => { | ||
| *runtime = | ||
| override_tool_exposure(Arc::clone(runtime), ToolExposure::DirectModelOnly); | ||
| } | ||
| ToolExposure::Direct | ||
| | ToolExposure::Deferred | ||
| | ToolExposure::DirectModelOnly | ||
| | ToolExposure::Hidden => {} | ||
| } | ||
| } | ||
|
Comment on lines
+205
to
+227
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i know we're against one-time helpers but i think in this case its warranted; this is logic very specific to this custom config setup and imo it shouldnt pollute
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. This is a good exception to the one-use-helper rule because it is self-contained config policy, while
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| } | ||
|
|
||
| #[instrument(level = "trace", skip_all)] | ||
| fn build_model_visible_specs_and_registry( | ||
| turn_context: &TurnContext, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,85 @@ | ||
| use anyhow::Result; | ||
| use codex_features::Feature; | ||
| use core_test_support::apps_test_server::AppsTestServer; | ||
| use core_test_support::apps_test_server::SEARCH_CALENDAR_CREATE_TOOL; | ||
| use core_test_support::apps_test_server::SEARCH_CALENDAR_NAMESPACE; | ||
| use core_test_support::apps_test_server::search_capable_apps_builder; | ||
| use core_test_support::responses; | ||
| use core_test_support::responses::ev_assistant_message; | ||
| use core_test_support::responses::ev_completed; | ||
| use core_test_support::responses::ev_response_created; | ||
| use core_test_support::responses::namespace_child_tool; | ||
| use core_test_support::responses::sse; | ||
| use core_test_support::skip_if_no_network; | ||
| use serde_json::Value; | ||
|
|
||
| #[tokio::test(flavor = "multi_thread", worker_threads = 2)] | ||
| async fn code_mode_only_exposes_direct_model_only_mcp_namespaces() -> Result<()> { | ||
| skip_if_no_network!(Ok(())); | ||
|
|
||
| let server = responses::start_mock_server().await; | ||
| let apps_server = AppsTestServer::mount_searchable(&server).await?; | ||
| let response = responses::mount_sse_once( | ||
| &server, | ||
| sse(vec![ | ||
| ev_response_created("resp-1"), | ||
| ev_assistant_message("msg-1", "done"), | ||
| ev_completed("resp-1"), | ||
| ]), | ||
| ) | ||
| .await; | ||
|
|
||
| let mut builder = search_capable_apps_builder(apps_server.chatgpt_base_url.clone()) | ||
| .with_config(move |config| { | ||
| config | ||
| .features | ||
| .enable(Feature::CodeModeOnly) | ||
| .expect("test config should allow feature update"); | ||
| config | ||
| .features | ||
| .enable(Feature::ToolSearchAlwaysDeferMcpTools) | ||
| .expect("test config should allow feature update"); | ||
| config.code_mode.direct_only_tool_namespaces = | ||
| vec![SEARCH_CALENDAR_NAMESPACE.to_string()]; | ||
| }); | ||
| let test = builder.build(&server).await?; | ||
| test.submit_turn("inspect directly exposed MCP tools") | ||
| .await?; | ||
| let body = response.single_request().body_json(); | ||
| let tools = body | ||
| .get("tools") | ||
| .and_then(Value::as_array) | ||
| .expect("request should contain tools"); | ||
|
|
||
| assert!( | ||
| namespace_child_tool( | ||
| &body, | ||
| SEARCH_CALENDAR_NAMESPACE, | ||
| SEARCH_CALENDAR_CREATE_TOOL, | ||
| ) | ||
| .is_some(), | ||
| "configured MCP namespace should remain top-level: {body}" | ||
| ); | ||
| assert!( | ||
| !tools.iter().any(|tool| { | ||
| tool.get("name") | ||
| .or_else(|| tool.get("type")) | ||
| .and_then(Value::as_str) | ||
| == Some("tool_search") | ||
| }), | ||
| "configured MCP namespace should not be deferred: {body}" | ||
| ); | ||
| let exec_description = tools.iter().find_map(|tool| { | ||
| (tool.get("name").and_then(Value::as_str) == Some("exec")) | ||
| .then(|| tool.get("description").and_then(Value::as_str)) | ||
| .flatten() | ||
| }); | ||
| assert!( | ||
| exec_description.is_some_and(|description| { | ||
| !description.contains("mcp__codex_apps__calendar_create_event(args:") | ||
| }), | ||
| "direct-model-only MCP namespace should not be available through exec: {body}" | ||
| ); | ||
|
|
||
| Ok(()) | ||
| } |

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The dynamic input currently drives both
ToolExposureandResponsesApiTool.defer_loading. Once the planner promotes a configured deferred runtime toDirectModelOnly, leaving this field set would serialize the now-top-level tool withdefer_loading: trueeven though it has also been removed fromtool_search. Clearing it at construction keeps the stored spec direct-shaped;ToolSearchInfo::from_tool_specadds the marker back only when the tool is actually returned as a deferred search result. A short comment here would make that invariant clearer.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks codex lol
maybe comment like
// ToolExposure is canonical; tool search adds the wire-level marker for deferred results.