Deduplicate MCP tool names #1571

pakrym-oai · 2025-07-14T22:21:40Z

Store fully qualified names along with tool entries so we don't have to re-parse them.

github-actions · 2025-07-14T22:21:49Z

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

- Reformatted code to improve readability and maintain consistent style - Adjusted line breaks, indentation, and brace placement - No logic changes, only formatting and style improvements

…CP connection manager

pakrym-oai · 2025-07-14T23:17:31Z

I have read the CLA Document and I hereby sign the CLA

…n list_all_tools and parse_tool_name

…rect indexing

…ility

tui: remove unnecessary lifetime parameter from App and BottomPaneView impls

…ger.rs

github-actions · 2025-07-16T15:03:16Z

Summary

This PR de-duplicates MCP tool names by shortening the delimiter to “__”, trimming long names, and appending a random suffix when collisions occur. The connection manager, name-parsing logic, Cargo deps, and a new test suite are updated accordingly.

Review

Nice improvement—unique, spec-compliant tool names with thorough tests.

qualify_tools correctly enforces 64-char limit and retries until the name is unique.
Added rand dependency and unit tests; coverage looks solid.
API surface stays compatible via parse_tool_name, but old try_parse_fully_qualified_tool_name is removed—confirm no external crates relied on it.
Minor: consider seeding rand::thread_rng() explicitly for clarity (rand::rng() is fine but less common).
Docs/comments are clear; cargo-toml clean-up good.

Overall, well-structured change—ship it after the small naming nit if desired.

View workflow run

bolinfest · 2025-07-17T00:44:05Z

I haven't had a chance to review this properly yet, but FYI, you probably have to rebase on 0bc7ee9

bolinfest · 2025-07-17T16:04:00Z

codex-rs/core/src/mcp_connection_manager.rs

-    format!("{server}{MCP_TOOL_NAME_DELIMITER}{tool}")
-}
+fn qualify_tools(tools: Vec<ToolInfo>) -> HashMap<String, ToolInfo> {
+    let mut used_names = HashSet::new();


I think it could be slightly problematic to add a random suffix at the end of the string because if you are trying to do analysis across rollouts, the tool name will not be consistent. What do you think about the following strategy (now that we are keeping track of used names):

If tool.tool_name exceeds MAX_TOOL_NAME_LENGTH, go to Step 4.

If tool.tool_name is not in used_names, just use tool.tool_name as the qualified name.

If {server_name}__{tool_name} is not in used_names and does not exceed MAX_TOOL_NAME_LENGTH, use that as the qualified name.
4a. (Option 1) Take the hash (BLAKE3 or SHA256) of {server_name}__{tool_name} and use the 64-digit hex string as the qualified name.
4b. (Option 2) Take the first 24 characters of tool_name and concatenate it with the SHA1 of {server_name}__{tool_name}, which is a maximum of 64 digits.

Alternatively, we could just always hash, but I expect that makes things harder to debug.

I feel like Option 2 provides a good balance between consistency and readability. What do you think?

Agree with your comment. What I don't fully like in the approach is that tool names are order dependent.

I'd rather always used {server_name}__{tool_name} and if it's too long appended SHA1 of {server_name}__{tool_name}.

How does this sound?

That's a great point about the tool ordering: I like your solution!

# Conflicts: # codex-rs/core/src/mcp_connection_manager.rs

- Switch tool name conflict resolution from random suffixes to SHA1 hashes for determinism. - Names longer than MAX_TOOL_NAME_LENGTH are truncated and suffixed with SHA1 of original. - Duplicated tool names are now logged and skipped. - Updated tests for new logic and dropped randomization.

…nt ordering

bolinfest

LGTM module the codex.js change!

bolinfest · 2025-07-17T18:12:49Z

codex-cli/bin/codex.js

@@ -28,13 +28,14 @@ const __dirname = path.dirname(__filename);
 // For the @native release of the Node module, the `use-native` file is added,
 // indicating we should default to the native binary. For other releases,
 // setting CODEX_RUST=1 will opt-in to the native binary, if included.
-const wantsNative = fs.existsSync(path.join(__dirname, "use-native")) ||
+const wantsNative =


This is an unrelated change: would you mind removing this from the PR?

bolinfest · 2025-07-17T18:17:02Z

codex-rs/core/src/mcp_connection_manager.rs

-    format!("{server}{MCP_TOOL_NAME_DELIMITER}{tool}")
-}
+fn qualify_tools(tools: Vec<ToolInfo>) -> HashMap<String, ToolInfo> {
+    let mut used_names = HashSet::new();


That's a great point about the tool ordering: I like your solution!

bolinfest · 2025-07-17T18:18:48Z

codex-rs/core/src/mcp_connection_manager.rs

+        assert_eq!(keys[0].len(), 64);
+        assert_eq!(
+            keys[0],
+            "my_server__extremely_lena02e507efc5a9de88637e436690364fd4219e4ef"


Seeing this an imagining trying to read it in logs or whatever, I wonder whether we would rather use 15 characters of prefix, _ as a delimiter, and then the hash, so this would be:

my_server__extremely_le_a02e507efc5a9de88637e436690364fd4219e4ef

Though I guess that's not materially more readable? Just a thought: fine either way.

I'd go with existing option and iterate if we feel the pain.

Deduplicate MCP tool names

60ee86e

pakrym-oai added 2 commits July 14, 2025 15:52

style: reformat core/mcp_connection_manager.rs for readability

d7c54b3

- Reformatted code to improve readability and maintain consistent style - Adjusted line breaks, indentation, and brace placement - No logic changes, only formatting and style improvements

core: remove redundant return statements and minor code cleanups in M…

2208aed

…CP connection manager

core/mcp_connection_manager.rs: simplify method chaining formatting i…

92e0ca9

…n list_all_tools and parse_tool_name

github-actions bot added a commit that referenced this pull request Jul 14, 2025

@pakrym-oai has signed the CLA in #1571

1026300

pakrym-oai added 6 commits July 14, 2025 16:21

tests: fix deprecated usage of .next().unwrap() on HashMap keys to di…

ce3555e

…rect indexing

tests: fix qualified_tools key access to avoid keys() indexing instab…

85787e6

…ility

core: use more concise format string in test tool description

f8cb4ab

tui: remove unnecessary lifetime parameter from App and BottomPaneView impls

tui: add explicit lifetime parameters to App and BottomPaneView impls

0758c99

tests: fix assertion to handle unsorted keys in tool name test

d0c53d7

core: allow clippy::unwrap_used in test module of mcp_connection_mana…

37becc9

…ger.rs

pakrym-oai marked this pull request as ready for review July 15, 2025 04:12

bolinfest added the codex-review label Jul 16, 2025

github-actions bot added codex-review-in-progress and removed codex-review labels Jul 16, 2025

github-actions bot added codex-review-completed and removed codex-review-in-progress labels Jul 16, 2025

bolinfest requested changes Jul 17, 2025

View reviewed changes

pakrym-oai added 5 commits July 17, 2025 09:17

Merge branch 'main' into pakrym/deduplicate-mcp-tool-names

34bc9ce

# Conflicts: # codex-rs/core/src/mcp_connection_manager.rs

core: split sha1 import into two lines for Digest and Sha1

dc6065c

core: use inline format syntax for SHA1 string in qualify_tools

2d2572d

tests: sort qualified_tools keys before assertions to ensure consiste…

4dcea8a

…nt ordering

pakrym-oai requested a review from bolinfest July 17, 2025 18:14

bolinfest approved these changes Jul 17, 2025

View reviewed changes

revert

8a10215

pakrym-oai merged commit b95a010 into main Jul 17, 2025
11 checks passed

pakrym-oai deleted the pakrym/deduplicate-mcp-tool-names branch July 17, 2025 18:35

github-actions bot locked and limited conversation to collaborators Jul 17, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Deduplicate MCP tool names #1571

Deduplicate MCP tool names #1571

Uh oh!

pakrym-oai commented Jul 14, 2025 •

edited

Loading

Uh oh!

github-actions bot commented Jul 14, 2025 •

edited

Loading

Uh oh!

pakrym-oai commented Jul 14, 2025

Uh oh!

github-actions bot commented Jul 16, 2025

Uh oh!

bolinfest commented Jul 17, 2025

Uh oh!

bolinfest Jul 17, 2025

Uh oh!

pakrym-oai Jul 17, 2025 •

edited

Loading

Uh oh!

pakrym-oai Jul 17, 2025

Uh oh!

bolinfest Jul 17, 2025

Uh oh!

bolinfest left a comment

Uh oh!

bolinfest Jul 17, 2025

Uh oh!

pakrym-oai Jul 17, 2025

Uh oh!

bolinfest Jul 17, 2025

Uh oh!

bolinfest Jul 17, 2025

Uh oh!

pakrym-oai Jul 17, 2025

Uh oh!

Uh oh!

Uh oh!

Deduplicate MCP tool names #1571

Deduplicate MCP tool names #1571

Uh oh!

Conversation

pakrym-oai commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jul 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pakrym-oai commented Jul 14, 2025

Uh oh!

github-actions bot commented Jul 16, 2025

Summary

Review

Uh oh!

bolinfest commented Jul 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pakrym-oai Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bolinfest left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

pakrym-oai commented Jul 14, 2025 •

edited

Loading

github-actions bot commented Jul 14, 2025 •

edited

Loading

pakrym-oai Jul 17, 2025 •

edited

Loading