Skip to content

Guard OpenAI tool registration when bridging #106

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.

Sign up for GitHub

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

Merged
hackall360 merged 1 commit into from
Sep 15, 2025
Merged

Guard OpenAI tool registration when bridging #106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions codex-rs/core/src/codex.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -473,6 +473,8 @@ impl Session {
model_reasoning_summary,
conversation_id,
);
let provider_supports_tools = provider.supports_tools;
let force_json_bridge = client.force_json_bridge();
let turn_context = TurnContext {
client,
tools_config: ToolsConfig::new(&ToolsConfigParams {
Expand All @@ -485,6 +487,8 @@ impl Session {
use_streamable_shell_tool: config.use_experimental_streamable_shell_tool,
include_view_image_tool: config.include_view_image_tool,
experimental_unified_exec_tool: config.use_experimental_unified_exec_tool,
provider_supports_tools,
force_json_bridge,
}),
user_instructions,
base_instructions,
Expand Down Expand Up @@ -1162,6 +1166,7 @@ async fn submission_loop(
updated_config.model_context_window = Some(model_info.context_window);
}

let provider_supports_tools = provider.supports_tools;
let client = ModelClient::new(
Arc::new(updated_config),
auth_manager,
Expand All @@ -1170,6 +1175,7 @@ async fn submission_loop(
effective_summary,
sess.conversation_id,
);
let force_json_bridge = client.force_json_bridge();

let new_approval_policy = approval_policy.unwrap_or(prev.approval_policy);
let new_sandbox_policy = sandbox_policy
Expand All @@ -1187,6 +1193,8 @@ async fn submission_loop(
use_streamable_shell_tool: config.use_experimental_streamable_shell_tool,
include_view_image_tool: config.include_view_image_tool,
experimental_unified_exec_tool: config.use_experimental_unified_exec_tool,
provider_supports_tools,
force_json_bridge,
});

let new_turn_context = TurnContext {
Expand Down Expand Up @@ -1253,6 +1261,7 @@ async fn submission_loop(

// Build a new client with per‑turn reasoning settings.
// Reuse the same provider and session id; auth defaults to env/API key.
let provider_supports_tools = provider.supports_tools;
let client = ModelClient::new(
Arc::new(per_turn_config),
auth_manager,
Expand All @@ -1261,6 +1270,7 @@ async fn submission_loop(
summary,
sess.conversation_id,
);
let force_json_bridge = client.force_json_bridge();

let fresh_turn_context = TurnContext {
client,
Expand All @@ -1276,6 +1286,8 @@ async fn submission_loop(
include_view_image_tool: config.include_view_image_tool,
experimental_unified_exec_tool: config
.use_experimental_unified_exec_tool,
provider_supports_tools,
force_json_bridge,
}),
user_instructions: turn_context.user_instructions.clone(),
base_instructions: turn_context.base_instructions.clone(),
Expand Down
97 changes: 82 additions & 15 deletions codex-rs/core/src/openai_tools.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ pub(crate) struct ToolsConfig {
pub web_search_request: bool,
pub include_view_image_tool: bool,
pub experimental_unified_exec_tool: bool,
pub supports_native_tools: bool,
}

pub(crate) struct ToolsConfigParams<'a> {
Expand All @@ -83,6 +84,8 @@ pub(crate) struct ToolsConfigParams<'a> {
pub(crate) use_streamable_shell_tool: bool,
pub(crate) include_view_image_tool: bool,
pub(crate) experimental_unified_exec_tool: bool,
pub(crate) provider_supports_tools: bool,
pub(crate) force_json_bridge: bool,
}

impl ToolsConfig {
Expand All @@ -97,39 +100,59 @@ impl ToolsConfig {
use_streamable_shell_tool,
include_view_image_tool,
experimental_unified_exec_tool,
provider_supports_tools,
force_json_bridge,
} = params;
let mut shell_type = if *use_streamable_shell_tool {
let supports_native_tools = *provider_supports_tools && !*force_json_bridge;

let mut shell_type = if supports_native_tools && *use_streamable_shell_tool {
ConfigShellToolType::StreamableShell
} else if model_family.uses_local_shell_tool {
} else if supports_native_tools && model_family.uses_local_shell_tool {
ConfigShellToolType::LocalShell
} else {
ConfigShellToolType::DefaultShell
};
if matches!(approval_policy, AskForApproval::OnRequest) && !use_streamable_shell_tool {
if supports_native_tools
&& matches!(approval_policy, AskForApproval::OnRequest)
&& !use_streamable_shell_tool
{
shell_type = ConfigShellToolType::ShellWithRequest {
sandbox_policy: sandbox_policy.clone(),
}
}

let apply_patch_tool_type = match model_family.apply_patch_tool_type {
Some(ApplyPatchToolType::Freeform) => Some(ApplyPatchToolType::Freeform),
Some(ApplyPatchToolType::Function) => Some(ApplyPatchToolType::Function),
None => {
if *include_apply_patch_tool {
Some(ApplyPatchToolType::Freeform)
} else {
None
let apply_patch_tool_type = if supports_native_tools {
match model_family.apply_patch_tool_type {
Some(ApplyPatchToolType::Freeform) => Some(ApplyPatchToolType::Freeform),
Some(ApplyPatchToolType::Function) => Some(ApplyPatchToolType::Function),
None => {
if *include_apply_patch_tool {
Some(ApplyPatchToolType::Freeform)
} else {
None
}
}
}
} else {
None
};

// When bridging is required we skip exposing tools entirely and let the
// bridge translate assistant JSON into tool traffic instead.
let plan_tool = supports_native_tools && *include_plan_tool;
let web_search_request = supports_native_tools && *include_web_search_request;
let include_view_image_tool = supports_native_tools && *include_view_image_tool;
let experimental_unified_exec_tool =
supports_native_tools && *experimental_unified_exec_tool;

Self {
shell_type,
plan_tool: *include_plan_tool,
plan_tool,
apply_patch_tool_type,
web_search_request: *include_web_search_request,
include_view_image_tool: *include_view_image_tool,
experimental_unified_exec_tool: *experimental_unified_exec_tool,
web_search_request,
include_view_image_tool,
experimental_unified_exec_tool,
supports_native_tools,
}
}
}
Expand Down Expand Up @@ -533,6 +556,13 @@ pub(crate) fn get_openai_tools(
) -> Vec<OpenAiTool> {
let mut tools: Vec<OpenAiTool> = Vec::new();

if !config.supports_native_tools {
// When native tools are unavailable we rely on a JSON bridge to turn
// assistant output into tool calls, so there is nothing to register
// with the provider here.
return tools;
}

if config.experimental_unified_exec_tool {
tools.push(create_unified_exec_tool());
} else {
Expand Down Expand Up @@ -644,6 +674,8 @@ mod tests {
use_streamable_shell_tool: false,
include_view_image_tool: true,
experimental_unified_exec_tool: true,
provider_supports_tools: true,
force_json_bridge: false,
});
let tools = get_openai_tools(&config, Some(HashMap::new()));

Expand All @@ -666,6 +698,8 @@ mod tests {
use_streamable_shell_tool: false,
include_view_image_tool: true,
experimental_unified_exec_tool: true,
provider_supports_tools: true,
force_json_bridge: false,
});
let tools = get_openai_tools(&config, Some(HashMap::new()));

Expand All @@ -688,6 +722,8 @@ mod tests {
use_streamable_shell_tool: false,
include_view_image_tool: true,
experimental_unified_exec_tool: true,
provider_supports_tools: true,
force_json_bridge: false,
});
let tools = get_openai_tools(
&config,
Expand Down Expand Up @@ -781,6 +817,27 @@ mod tests {
);
}

#[test]
fn test_get_openai_tools_skipped_when_bridge_forced() {
let model_family = find_family_for_model("o3").expect("o3 should be a valid model family");
let config = ToolsConfig::new(&ToolsConfigParams {
model_family: &model_family,
approval_policy: AskForApproval::Never,
sandbox_policy: SandboxPolicy::ReadOnly,
include_plan_tool: true,
include_apply_patch_tool: true,
include_web_search_request: true,
use_streamable_shell_tool: false,
include_view_image_tool: true,
experimental_unified_exec_tool: true,
provider_supports_tools: true,
force_json_bridge: true,
});

let tools = get_openai_tools(&config, Some(HashMap::new()));
assert!(tools.is_empty(), "tools should be omitted when bridging");
}

#[test]
fn test_get_openai_tools_mcp_tools_sorted_by_name() {
let model_family = find_family_for_model("o3").expect("o3 should be a valid model family");
Expand All @@ -794,6 +851,8 @@ mod tests {
use_streamable_shell_tool: false,
include_view_image_tool: true,
experimental_unified_exec_tool: true,
provider_supports_tools: true,
force_json_bridge: false,
});

// Intentionally construct a map with keys that would sort alphabetically.
Expand Down Expand Up @@ -872,6 +931,8 @@ mod tests {
use_streamable_shell_tool: false,
include_view_image_tool: true,
experimental_unified_exec_tool: true,
provider_supports_tools: true,
force_json_bridge: false,
});

let tools = get_openai_tools(
Expand Down Expand Up @@ -935,6 +996,8 @@ mod tests {
use_streamable_shell_tool: false,
include_view_image_tool: true,
experimental_unified_exec_tool: true,
provider_supports_tools: true,
force_json_bridge: false,
});

let tools = get_openai_tools(
Expand Down Expand Up @@ -993,6 +1056,8 @@ mod tests {
use_streamable_shell_tool: false,
include_view_image_tool: true,
experimental_unified_exec_tool: true,
provider_supports_tools: true,
force_json_bridge: false,
});

let tools = get_openai_tools(
Expand Down Expand Up @@ -1054,6 +1119,8 @@ mod tests {
use_streamable_shell_tool: false,
include_view_image_tool: true,
experimental_unified_exec_tool: true,
provider_supports_tools: true,
force_json_bridge: false,
});

let tools = get_openai_tools(
Expand Down
Loading