mirror of
https://github.com/openai/codex.git
synced 2026-10-28 18:58:46 +00:00
Always put Code Mode tool descriptions before namespace descriptions (#57462)
## What changed Make tool descriptions precede namespace descriptions in Code Mode execution and tool search. Mark `code_mode_tool_description_first` as removed and simplify cached definitions to depend only on the input schema budget. ## Testing Update Code Mode integration assertions and catalog snapshots to expect tool descriptions before namespace descriptions. GitOrigin-RevId: 1344d4c264d1d2b18ee0388a064b6813ca81f770
This commit is contained in:
@@ -4,7 +4,6 @@ use crate::tools::context::ToolPayload;
|
||||
use crate::tools::context::boxed_tool_output;
|
||||
use crate::tools::registry::CoreToolRuntime;
|
||||
use crate::tools::registry::ToolExecutor;
|
||||
use codex_features::Feature;
|
||||
use codex_prompts::ResolvedModelMessages;
|
||||
use codex_tools::IndirectNamespacePrefixes;
|
||||
use codex_tools::ToolName;
|
||||
@@ -56,18 +55,12 @@ impl CodeModeExecuteHandler {
|
||||
.config
|
||||
.code_mode
|
||||
.tool_input_schema_max_bytes;
|
||||
let tool_description_first = step_context
|
||||
.turn
|
||||
.config
|
||||
.features
|
||||
.enabled(Feature::CodeModeToolDescriptionFirst);
|
||||
let mut enabled_tools = Vec::with_capacity(self.nested_tool_specs.len());
|
||||
for (spec, cached_runtime) in &self.nested_tool_specs {
|
||||
let definitions = prepare_code_mode_tool_definitions(
|
||||
cached_runtime.as_deref(),
|
||||
|| Cow::Borrowed(spec.as_ref()),
|
||||
code_mode_input_schema_max_bytes,
|
||||
tool_description_first,
|
||||
);
|
||||
match definitions {
|
||||
Cow::Borrowed(definitions) => enabled_tools.extend_from_slice(definitions),
|
||||
|
||||
@@ -1,6 +1,5 @@
|
||||
//! Prepares nested tool definitions for execution and search.
|
||||
//! Cached definitions stay borrowed; rendered schemas are dropped after augmentation.
|
||||
//! Description order follows the session-static feature setting.
|
||||
|
||||
use crate::tools::registry::CoreToolRuntime;
|
||||
use codex_code_mode::ToolDefinition;
|
||||
@@ -11,12 +10,10 @@ pub(crate) fn prepare_code_mode_tool_definitions<'a>(
|
||||
runtime: Option<&'a dyn CoreToolRuntime>,
|
||||
spec: impl FnOnce() -> Cow<'a, ToolSpec>,
|
||||
code_mode_input_schema_max_bytes: Option<usize>,
|
||||
tool_description_first: bool,
|
||||
) -> Cow<'a, [ToolDefinition]> {
|
||||
if let Some(definitions) = runtime.and_then(|runtime| {
|
||||
runtime
|
||||
.cached_code_mode_definitions(code_mode_input_schema_max_bytes, tool_description_first)
|
||||
}) {
|
||||
if let Some(definitions) = runtime
|
||||
.and_then(|runtime| runtime.cached_code_mode_definitions(code_mode_input_schema_max_bytes))
|
||||
{
|
||||
return Cow::Borrowed(definitions);
|
||||
}
|
||||
|
||||
@@ -24,7 +21,6 @@ pub(crate) fn prepare_code_mode_tool_definitions<'a>(
|
||||
let mut definitions = codex_tools::collect_code_mode_tool_definitions(
|
||||
std::iter::once(spec.as_ref()),
|
||||
code_mode_input_schema_max_bytes,
|
||||
tool_description_first,
|
||||
);
|
||||
for definition in &mut definitions {
|
||||
definition.input_schema = None;
|
||||
|
||||
@@ -90,8 +90,7 @@ impl McpHandlerDefinition {
|
||||
|
||||
pub struct McpHandler {
|
||||
definition: Arc<McpHandlerDefinition>,
|
||||
code_mode_tool_definitions:
|
||||
OnceLock<(Option<usize>, bool, Vec<codex_code_mode::ToolDefinition>)>,
|
||||
code_mode_tool_definitions: OnceLock<(Option<usize>, Vec<codex_code_mode::ToolDefinition>)>,
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
@@ -330,29 +329,19 @@ impl CoreToolRuntime for McpHandler {
|
||||
fn cached_code_mode_definitions(
|
||||
&self,
|
||||
code_mode_input_schema_max_bytes: Option<usize>,
|
||||
tool_description_first: bool,
|
||||
) -> Option<&[codex_code_mode::ToolDefinition]> {
|
||||
let (cached_budget, cached_tool_description_first, definitions) =
|
||||
self.code_mode_tool_definitions.get_or_init(|| {
|
||||
let mut definitions = codex_tools::collect_code_mode_tool_definitions(
|
||||
std::iter::once(self.definition.spec.as_ref()),
|
||||
code_mode_input_schema_max_bytes,
|
||||
tool_description_first,
|
||||
);
|
||||
for definition in &mut definitions {
|
||||
definition.input_schema = None;
|
||||
definition.output_schema = None;
|
||||
}
|
||||
(
|
||||
code_mode_input_schema_max_bytes,
|
||||
tool_description_first,
|
||||
definitions,
|
||||
)
|
||||
});
|
||||
// Reuse definitions only when both rendering inputs match.
|
||||
(*cached_budget == code_mode_input_schema_max_bytes
|
||||
&& *cached_tool_description_first == tool_description_first)
|
||||
.then_some(definitions.as_slice())
|
||||
let (cached_budget, definitions) = self.code_mode_tool_definitions.get_or_init(|| {
|
||||
let mut definitions = codex_tools::collect_code_mode_tool_definitions(
|
||||
std::iter::once(self.definition.spec.as_ref()),
|
||||
code_mode_input_schema_max_bytes,
|
||||
);
|
||||
for definition in &mut definitions {
|
||||
definition.input_schema = None;
|
||||
definition.output_schema = None;
|
||||
}
|
||||
(code_mode_input_schema_max_bytes, definitions)
|
||||
});
|
||||
(*cached_budget == code_mode_input_schema_max_bytes).then_some(definitions.as_slice())
|
||||
}
|
||||
|
||||
fn wait_until_ready<'a>(&'a self, session: &'a Arc<Session>) -> Option<BoxFuture<'a, ()>> {
|
||||
@@ -905,36 +894,17 @@ mod tests {
|
||||
));
|
||||
|
||||
let first = handler
|
||||
.cached_code_mode_definitions(
|
||||
/*code_mode_input_schema_max_bytes*/ None,
|
||||
/*tool_description_first*/ false,
|
||||
)
|
||||
.cached_code_mode_definitions(/*code_mode_input_schema_max_bytes*/ None)
|
||||
.expect("MCP definitions should be cached");
|
||||
assert_eq!(first.len(), 1);
|
||||
assert!(first[0].input_schema.is_none());
|
||||
assert!(first[0].output_schema.is_none());
|
||||
|
||||
assert!(
|
||||
handler
|
||||
.cached_code_mode_definitions(
|
||||
/*code_mode_input_schema_max_bytes*/ None,
|
||||
/*tool_description_first*/ true,
|
||||
)
|
||||
.is_none()
|
||||
);
|
||||
|
||||
let second = handler
|
||||
.cached_code_mode_definitions(
|
||||
/*code_mode_input_schema_max_bytes*/ None,
|
||||
/*tool_description_first*/ false,
|
||||
)
|
||||
.cached_code_mode_definitions(/*code_mode_input_schema_max_bytes*/ None)
|
||||
.expect("MCP definitions should be cached");
|
||||
assert!(std::ptr::eq(first, second));
|
||||
assert!(
|
||||
handler
|
||||
.cached_code_mode_definitions(Some(32_000), /*tool_description_first*/ false)
|
||||
.is_none()
|
||||
);
|
||||
assert!(handler.cached_code_mode_definitions(Some(32_000)).is_none());
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -15,7 +15,6 @@ use bm25::Embedder;
|
||||
use bm25::EmbedderBuilder;
|
||||
use bm25::Language;
|
||||
use bm25::Scorer;
|
||||
use codex_features::Feature;
|
||||
use codex_prompts::ResolvedModelMessages;
|
||||
use codex_tools::IndirectNamespacePrefixes;
|
||||
use codex_tools::JsonToolOutput;
|
||||
@@ -260,11 +259,6 @@ impl ToolSearchHandler {
|
||||
.config
|
||||
.code_mode
|
||||
.tool_input_schema_max_bytes,
|
||||
step_context
|
||||
.turn
|
||||
.config
|
||||
.features
|
||||
.enabled(Feature::CodeModeToolDescriptionFirst),
|
||||
&indirect_prefixes,
|
||||
);
|
||||
return Ok(boxed_tool_output(JsonToolOutput::new(serde_json::json!({
|
||||
@@ -292,7 +286,6 @@ impl ToolSearchHandler {
|
||||
limit: usize,
|
||||
router: &ToolRouter,
|
||||
code_mode_input_schema_max_bytes: Option<usize>,
|
||||
tool_description_first: bool,
|
||||
indirect_prefixes: &IndirectNamespacePrefixes<'_>,
|
||||
) -> Vec<CodeModeSearchResult> {
|
||||
let mut tools = Vec::new();
|
||||
@@ -320,7 +313,6 @@ impl ToolSearchHandler {
|
||||
tool.cached_runtime().map(Arc::as_ref),
|
||||
|| Cow::Owned(Arc::unwrap_or_clone(tool.spec())),
|
||||
code_mode_input_schema_max_bytes,
|
||||
tool_description_first,
|
||||
);
|
||||
let Some(definition) = definitions.iter().find(|definition| {
|
||||
definition.tool_name.clone().with_default_namespace() == candidate_name
|
||||
|
||||
@@ -84,11 +84,10 @@ pub(crate) trait CoreToolRuntime: ToolExecutor<ToolInvocation> {
|
||||
|
||||
/// Returns lazily cached, augmented Code Mode definitions owned by this runtime.
|
||||
/// Input and output schemas must be cleared after rendering their declarations.
|
||||
/// Return None when the requested rendering inputs differ from the cached inputs.
|
||||
/// Return None when the requested schema budget differs from the cached budget.
|
||||
fn cached_code_mode_definitions(
|
||||
&self,
|
||||
_code_mode_input_schema_max_bytes: Option<usize>,
|
||||
_tool_description_first: bool,
|
||||
) -> Option<&[codex_code_mode::ToolDefinition]> {
|
||||
None
|
||||
}
|
||||
|
||||
@@ -3619,15 +3619,12 @@ async fn code_mode_complete_calls_preserve_large_arguments() -> Result<()> {
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
#[test_case(false, None, false; "control")]
|
||||
#[test_case(true, None, false; "ranked_search")]
|
||||
#[test_case(true, Some(30_000), false; "ranked_search_expanded_schema")]
|
||||
#[test_case(false, None, true; "tool_description_first")]
|
||||
#[test_case(true, None, true; "ranked_search_tool_description_first")]
|
||||
#[test_case(false, None; "catalog_search")]
|
||||
#[test_case(true, None; "ranked_search")]
|
||||
#[test_case(true, Some(30_000); "ranked_search_expanded_schema")]
|
||||
async fn code_mode_only_searches_and_calls_deferred_app_tools(
|
||||
ranked_search: bool,
|
||||
schema_max_bytes: Option<usize>,
|
||||
tool_description_first: bool,
|
||||
) -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
@@ -3747,13 +3744,6 @@ if (!tool) {
|
||||
.enable(Feature::CodeModeOnly)
|
||||
.expect("test config should allow feature update");
|
||||
config.code_mode.tool_input_schema_max_bytes = schema_max_bytes;
|
||||
config
|
||||
.features
|
||||
.set_enabled(
|
||||
Feature::CodeModeToolDescriptionFirst,
|
||||
tool_description_first,
|
||||
)
|
||||
.expect("test config should allow description order update");
|
||||
if ranked_search {
|
||||
config
|
||||
.features
|
||||
@@ -3841,11 +3831,7 @@ if (!tool) {
|
||||
"hasDeclaration": true,
|
||||
"hasSchemaDescription": schema_max_bytes.is_some(),
|
||||
"hasNamespacePrefix": true,
|
||||
"descriptionPrefix": if tool_description_first {
|
||||
"Calendar search context.\n\nRead timezone option 99.\n\nPlan events and manage your calendar."
|
||||
} else {
|
||||
"Calendar search context.\n\nPlan events and manage your calendar.\n\nRead timezone option 99."
|
||||
},
|
||||
"descriptionPrefix": "Calendar search context.\n\nRead timezone option 99.\n\nPlan events and manage your calendar.",
|
||||
"matchesCatalog": true,
|
||||
"isError": false,
|
||||
"text": "called calendar_timezone_option_99 for at with ",
|
||||
@@ -7918,7 +7904,7 @@ text(JSON.stringify({
|
||||
exec_output,
|
||||
serde_json::json!({
|
||||
"name": "editor__apply_patch",
|
||||
"description": format!("Editing tools.\n\n{second_description}"),
|
||||
"description": format!("Apply a raw editor patch (step 2).\n\nEditing tools.\n\nexec tool declaration:\n```ts\n{declaration}\n```"),
|
||||
"result": {
|
||||
"namespace": "editor",
|
||||
"name": "apply_patch",
|
||||
@@ -8064,8 +8050,8 @@ text(JSON.stringify(tool));
|
||||
serde_json::json!({
|
||||
"name": "mcp__rmcp__echo",
|
||||
"description": concat!(
|
||||
"Use these tools to exercise the rmcp test server.\n\n",
|
||||
"Echo back the provided message and include environment data.\n\n",
|
||||
"Use these tools to exercise the rmcp test server.\n\n",
|
||||
"exec tool declaration:\n",
|
||||
"```ts\n",
|
||||
"declare const tools: { mcp__rmcp__echo(args: { env_var?: string; message: string; }): ",
|
||||
|
||||
+2
-2
@@ -109,9 +109,9 @@ Settings:
|
||||
08:custom_tool_call_output:Script completed
|
||||
Wall time <DURATION> seconds
|
||||
Output:
|
||||
| mcp__codex_apps__calendar_timezone_option_99 | Plan events and manage your calendar.
|
||||
| mcp__codex_apps__calendar_timezone_option_99 | Read timezone option 99.
|
||||
|
||||
Read timezone option 99.
|
||||
Plan events and manage your calendar.
|
||||
|
||||
exec tool declaration:
|
||||
```ts
|
||||
|
||||
+3
-3
@@ -44,7 +44,7 @@ Settings:
|
||||
07:custom_tool_call_output:Script completed
|
||||
Wall time <DURATION> seconds
|
||||
Output:
|
||||
| {"descriptions":["Functions context.\\n\\nImage guidance.\\n\\nView a local image file from the filesystem w...>>; };\\n```"],"echo":"ECHOING: ping","agent":"/root"} [hash=34de0ce5c435f70f]
|
||||
| {"descriptions":["Functions context.\\n\\nImage guidance.\\n\\nView a local image file from the filesystem w...>>; };\\n```"],"echo":"ECHOING: ping","agent":"/root"} [hash=4bd42f2e71ce15d7]
|
||||
|
||||
## Window 2 (after request 2)
|
||||
Settings: relative to window 1
|
||||
@@ -74,7 +74,7 @@ Settings: relative to window 1
|
||||
07:custom_tool_call_output:Script completed
|
||||
Wall time <DURATION> seconds
|
||||
Output:
|
||||
| {"descriptions":["Functions context.\\n\\nImage guidance.\\n\\nView a local image file from the filesystem w...>>; };\\n```"],"echo":"ECHOING: ping","agent":"/root"} [hash=34de0ce5c435f70f]
|
||||
| {"descriptions":["Functions context.\\n\\nImage guidance.\\n\\nView a local image file from the filesystem w...>>; };\\n```"],"echo":"ECHOING: ping","agent":"/root"} [hash=4bd42f2e71ce15d7]
|
||||
08:message/assistant:
|
||||
Done.
|
||||
09:message/developer:
|
||||
@@ -92,4 +92,4 @@ Settings: relative to window 1
|
||||
12:custom_tool_call_output:Script completed
|
||||
Wall time <DURATION> seconds
|
||||
Output:
|
||||
| {"descriptions":["View a local image file from the filesystem when visual inspection is needed. Use this...>>; };\\n```"],"echo":"ECHOING: ping","agent":"/root"} [hash=6ecfde0f75e5f051]
|
||||
| {"descriptions":["View a local image file from the filesystem when visual inspection is needed. Use this...>>; };\\n```"],"echo":"ECHOING: ping","agent":"/root"} [hash=6f5fd573af132df1]
|
||||
|
||||
@@ -152,7 +152,7 @@ pub enum Feature {
|
||||
CodeModePrewarm,
|
||||
/// Terminate active code mode cells when their turn is interrupted.
|
||||
CodeModeInterrupt,
|
||||
/// Put each tool's description before its namespace description in Code Mode.
|
||||
/// Removed compatibility flag for Code Mode tool description ordering.
|
||||
CodeModeToolDescriptionFirst,
|
||||
/// Restrict model-visible tools to code mode entrypoints (`exec`, `wait`).
|
||||
CodeModeOnly,
|
||||
@@ -1280,7 +1280,7 @@ pub const FEATURES: &[FeatureSpec] = &[
|
||||
FeatureSpec {
|
||||
id: Feature::CodeModeToolDescriptionFirst,
|
||||
key: "code_mode_tool_description_first",
|
||||
stage: Stage::UnderDevelopment,
|
||||
stage: Stage::Removed,
|
||||
default_enabled: false,
|
||||
},
|
||||
FeatureSpec {
|
||||
|
||||
@@ -90,7 +90,6 @@ pub fn tool_spec_to_code_mode_tool_definition(spec: &ToolSpec) -> Option<CodeMod
|
||||
pub fn collect_code_mode_tool_definitions<'a>(
|
||||
specs: impl IntoIterator<Item = &'a ToolSpec>,
|
||||
code_mode_input_schema_max_bytes: Option<usize>,
|
||||
tool_description_first: bool,
|
||||
) -> Vec<CodeModeToolDefinition> {
|
||||
let mut tool_definitions = specs
|
||||
.into_iter()
|
||||
@@ -101,11 +100,8 @@ pub fn collect_code_mode_tool_definitions<'a>(
|
||||
let namespace_description = namespace.description.trim();
|
||||
if !namespace_description.is_empty() {
|
||||
for definition in &mut definitions {
|
||||
definition.description = if tool_description_first {
|
||||
format!("{}\n\n{namespace_description}", definition.description)
|
||||
} else {
|
||||
format!("{namespace_description}\n\n{}", definition.description)
|
||||
};
|
||||
definition.description =
|
||||
format!("{}\n\n{namespace_description}", definition.description);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -58,12 +58,10 @@ fn code_mode_materializes_mcp_output_schemas() {
|
||||
super::collect_code_mode_tool_definitions(
|
||||
[&ToolSpec::Function(parsed)],
|
||||
/*code_mode_input_schema_max_bytes*/ None,
|
||||
/*tool_description_first*/ false,
|
||||
),
|
||||
super::collect_code_mode_tool_definitions(
|
||||
[&ToolSpec::Function(eager)],
|
||||
/*code_mode_input_schema_max_bytes*/ None,
|
||||
/*tool_description_first*/ false,
|
||||
),
|
||||
);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user