mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
app-server: align dynamic tool identifiers with Responses API (#20724)
## Why Codex currently accepts dynamic tool names and namespaces that the upstream Responses function-tool path does not actually support. In practice, that means app-server can register a dynamic tool successfully and only discover later that the LLM-facing tool contract will reject or mishandle it. This PR tightens the app-server-side dynamic tool contract to match the Responses API before we stack dynamic tool hook support on top of it. ## What changed - validate dynamic tool `name` against the Responses function-tool identifier contract: `^[a-zA-Z0-9_-]+$`, length `1..128` - validate dynamic tool `namespace` the same way, with the Responses namespace length limit `1..64` - reject namespaces that collide with the always-reserved Responses runtime namespaces such as `functions`, `multi_tool_use`, `file_search`, `web`, `browser`, `image_gen`, `computer`, `container`, `terminal`, `python`, `python_user_visible`, `api_tool`, `tool_search`, and `submodel_delegator` - escape invalid identifiers in error messages so control characters do not spill raw into logs or client-visible error text - document the tightened dynamic tool identifier contract in `codex-rs/app-server/README.md` - add both unit coverage for the validator and an app-server integration test that rejects a `thread/start` request with Responses-incompatible dynamic tool identifiers ## Verification - `cargo test -p codex-app-server validate_dynamic_tools_` - `cargo test -p codex-app-server --test all thread_start_rejects_dynamic_tools_not_supported_by_responses`
This commit is contained in:
committed by
GitHub
Unverified
parent
5119680f85
commit
8ef31894dc
@@ -184,6 +184,53 @@ fn has_model_resume_override(
|
||||
}
|
||||
|
||||
fn validate_dynamic_tools(tools: &[ApiDynamicToolSpec]) -> Result<(), String> {
|
||||
const DYNAMIC_TOOL_NAME_MAX_LEN: usize = 128;
|
||||
const DYNAMIC_TOOL_NAMESPACE_MAX_LEN: usize = 64;
|
||||
const DYNAMIC_TOOL_IDENTIFIER_PATTERN: &str = "^[a-zA-Z0-9_-]+$";
|
||||
const RESERVED_RESPONSES_NAMESPACES: &[&str] = &[
|
||||
"api_tool",
|
||||
"browser",
|
||||
"computer",
|
||||
"container",
|
||||
"file_search",
|
||||
"functions",
|
||||
"image_gen",
|
||||
"multi_tool_use",
|
||||
"python",
|
||||
"python_user_visible",
|
||||
"submodel_delegator",
|
||||
"terminal",
|
||||
"tool_search",
|
||||
"web",
|
||||
];
|
||||
|
||||
fn escape_identifier_for_error(value: &str) -> String {
|
||||
value.escape_default().to_string()
|
||||
}
|
||||
|
||||
fn validate_dynamic_tool_identifier(
|
||||
value: &str,
|
||||
label: &str,
|
||||
max_len: usize,
|
||||
) -> Result<(), String> {
|
||||
if !value
|
||||
.bytes()
|
||||
.all(|byte| byte.is_ascii_alphanumeric() || matches!(byte, b'_' | b'-'))
|
||||
{
|
||||
return Err(format!(
|
||||
"{label} must match {DYNAMIC_TOOL_IDENTIFIER_PATTERN} to match Responses API: {}",
|
||||
escape_identifier_for_error(value),
|
||||
));
|
||||
}
|
||||
if value.chars().count() > max_len {
|
||||
return Err(format!(
|
||||
"{label} must be at most {max_len} characters to match Responses API: {}",
|
||||
escape_identifier_for_error(value),
|
||||
));
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
let mut seen = HashSet::new();
|
||||
for tool in tools {
|
||||
let name = tool.name.trim();
|
||||
@@ -193,9 +240,10 @@ fn validate_dynamic_tools(tools: &[ApiDynamicToolSpec]) -> Result<(), String> {
|
||||
if name != tool.name {
|
||||
return Err(format!(
|
||||
"dynamic tool name has leading/trailing whitespace: {}",
|
||||
tool.name
|
||||
escape_identifier_for_error(&tool.name),
|
||||
));
|
||||
}
|
||||
validate_dynamic_tool_identifier(name, "dynamic tool name", DYNAMIC_TOOL_NAME_MAX_LEN)?;
|
||||
if name == "mcp" || name.starts_with("mcp__") {
|
||||
return Err(format!("dynamic tool name is reserved: {name}"));
|
||||
}
|
||||
@@ -209,13 +257,25 @@ fn validate_dynamic_tools(tools: &[ApiDynamicToolSpec]) -> Result<(), String> {
|
||||
if Some(namespace) != tool.namespace.as_deref() {
|
||||
return Err(format!(
|
||||
"dynamic tool namespace has leading/trailing whitespace for {name}: {namespace}",
|
||||
name = escape_identifier_for_error(name),
|
||||
namespace = escape_identifier_for_error(namespace),
|
||||
));
|
||||
}
|
||||
validate_dynamic_tool_identifier(
|
||||
namespace,
|
||||
"dynamic tool namespace",
|
||||
DYNAMIC_TOOL_NAMESPACE_MAX_LEN,
|
||||
)?;
|
||||
if namespace == "mcp" || namespace.starts_with("mcp__") {
|
||||
return Err(format!(
|
||||
"dynamic tool namespace is reserved for {name}: {namespace}"
|
||||
));
|
||||
}
|
||||
if RESERVED_RESPONSES_NAMESPACES.contains(&namespace) {
|
||||
return Err(format!(
|
||||
"dynamic tool namespace collides with a reserved Responses API namespace for {name}: {namespace}",
|
||||
));
|
||||
}
|
||||
}
|
||||
if !seen.insert((namespace, name)) {
|
||||
if let Some(namespace) = namespace {
|
||||
|
||||
Reference in New Issue
Block a user