Skip to content

fix: hashing long namespace flatten name#135

Merged
maralbahari merged 4 commits into
vllm-project:mainfrom
EmbeddedLLM:fix-codex-namespace-flattening-length-bound
Jul 21, 2026
Merged

fix: hashing long namespace flatten name#135
maralbahari merged 4 commits into
vllm-project:mainfrom
EmbeddedLLM:fix-codex-namespace-flattening-length-bound

Conversation

@haoshan98

@haoshan98 haoshan98 commented Jul 20, 2026

Copy link
Copy Markdown
Contributor
  • Bound gateway-generated Codex namespace member names to the upstream 64-character function-name limit while leaving shorter names unchanged.
  • Shorten long names with a readable prefix and deterministic 16-hex fingerprint of the complete name. The request-scoped namespace map continues to rewrite namespaced tool_choice values and restore function calls to their public { namespace, name } representation.
  • Reject ambiguous generated names when they collide with a function-call registry key claimed by a function tool, an mcp built-in tool, a normalized built-in tool, or another namespace member. Validation runs before upstream inference or MCP handler construction.
  • Add boundary, determinism, Unicode, tool_choice, restoration, WebSocket round-trip, and MCP collision regression coverage, and document the bounded model-visible format and collision behavior.

@franciscojavierarceo franciscojavierarceo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the generated shortened name needs to check against MCP registry keys.

typed_top_level_tool_names excludes ResponsesTool::Mcp, but ToolRegistry later inserts MCP entries under p.name.

If a later MCP declaration has a p.name matching the shortened name, it overwrites the client-owned namespace entry and the model’s call is dispatched as gateway-owned MCP work.

Before this change the long name was rejected upstream; shortening makes this misrouting path reachable.

Please reject collisions across all model-visible registry keys and add a regression test with a long namespace member followed by a matching MCP declaration.

@maralbahari maralbahari left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@haoshan98 Thanks for the fix can you fix the merge conflict please. LGTM.

@maralbahari maralbahari left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@haoshan98 will merge after addressing @franciscojavierarceo comment and merge conflict

Signed-off-by: haoshan98 <haoshanw@gmail.com>
Signed-off-by: haoshan98 <haoshanw@gmail.com>
@haoshan98
haoshan98 force-pushed the fix-codex-namespace-flattening-length-bound branch from b8c935f to 902c073 Compare July 21, 2026 06:09
Signed-off-by: haoshan98 <haoshanw@gmail.com>
}

#[derive(Clone, Copy, Debug, Eq, PartialEq)]
enum TopLevelRegistryToolKind {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this Enum sounds the same to the ResponsesTool. the ResponsesTool has tool_type function see if you can reuse that?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i thought we wanted more separation between Messages and Responses not less?

@maralbahari maralbahari Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@franciscojavierarceo yes there will be more separation my request is simply that we do have ToolType enum and it is used in ResponsesToo for mapping between tool and tool_type also used in tool_registry. so the ToolType enum as one single ground truth of what is supported on gateway rather than having to maintain multiple enums and might forget to update them. the ResponseTool is only used for responses api for now. on messages api as off now based on #131 PR merged is maintaining separate contract and a tool build registry that is plugin web search only as off now.

Signed-off-by: haoshan98 <haoshanw@gmail.com>
@haoshan98
haoshan98 requested a review from maralbahari July 21, 2026 09:37

@maralbahari maralbahari left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@maralbahari
maralbahari merged commit 3ddbd86 into vllm-project:main Jul 21, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants