🎯 fix: MCP Tool Misclassification from Action Delimiter Collision#12512
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a subtle tool-name parsing bug where certain MCP tools (e.g. names ending in _action) were incorrectly detected as OpenAPI “action tools” after the _mcp_<server> suffix was appended, causing them to be gated/filtered by actionsEnabled and silently unavailable to the model.
Changes:
- Add a shared
isActionTool()helper and replace severalincludes('_action_')checks with it to avoid cross-delimiter collisions. - Add/extend unit tests covering the MCP
_action+_mcp_boundary collision. - Introduce an auth-sensitive React Query key for startup config (
startupConfigKey) and update multiple call sites to use it.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/data-provider/src/types/assistants.ts | Adds isActionTool() helper to classify OpenAPI action tools more safely. |
| packages/api/src/tools/definitions.ts | Uses isActionTool() when splitting action vs MCP/built-in tool definitions. |
| packages/api/src/tools/definitions.spec.ts | Adds regression test ensuring MCP tools with _action aren’t routed to action-definition loading. |
| client/src/hooks/SSE/useEventHandlers.ts | Switches startup config cache reads to startupConfigKey(...). |
| client/src/hooks/Input/useQueryParams.ts | Switches startup config cache reads to startupConfigKey(...). |
| client/src/hooks/Input/useQueryParams.spec.ts | Updates mocks to preserve startupConfigKey export while mocking hooks. |
| client/src/hooks/Endpoint/useEndpoints.ts | Uses config defaults for interface when startup config lacks it. |
| client/src/hooks/Conversations/useNavigateToConvo.tsx | Switches startup config cache reads to startupConfigKey(...). |
| client/src/hooks/Chat/useChatFunctions.ts | Switches startup config cache reads to startupConfigKey(...). |
| client/src/hooks/AuthContext.tsx | Re-enables queries after auth is established (post setTokenHeader). |
| client/src/data-provider/Endpoints/queries.ts | Introduces startupConfigKey(isAuthenticated) and uses it in useGetStartupConfig. |
| client/src/data-provider/Auth/mutations.ts | Disables queries during login mutation; re-enables on error (success path deferred to AuthContext). |
| client/src/components/SidePanel/Builder/AssistantPanel.tsx | Uses isActionTool() to decide whether to store raw tool names vs full tool objects. |
| client/src/components/Nav/SettingsTabs/Data/ImportConversations.tsx | Switches startup config cache reads to startupConfigKey(...). |
| client/src/components/Chat/Messages/Content/ToolOutput/ToolIcon.tsx | Uses isActionTool() to avoid rendering MCP tools as “action” icons due to collision. |
| client/src/components/Chat/Messages/Content/tests/ToolIcon.test.tsx | Adds regression test for MCP _action boundary collision icon classification. |
| client/src/components/Chat/Menus/Endpoints/ModelSelector.tsx | Uses a module-level default interface config. |
| api/server/services/ToolService.js | Replaces action-tool classification checks with isActionTool() across gating/filtering paths. |
| api/server/services/tests/ToolService.spec.js | Adds tests for isActionTool() and regression for MCP _action tools not being filtered. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| */ | ||
|
|
||
| import { Constants, actionDelimiter } from 'librechat-data-provider'; | ||
| import { Constants, isActionTool, actionDelimiter } from 'librechat-data-provider'; |
There was a problem hiding this comment.
actionDelimiter is imported but no longer used in this module after switching to isActionTool(). This will trip lint/TS unused-import checks; remove actionDelimiter from the import list.
| import { Constants, isActionTool, actionDelimiter } from 'librechat-data-provider'; | |
| import { Constants, isActionTool } from 'librechat-data-provider'; |
| */ | ||
| export function isActionTool(toolName: string): boolean { | ||
| if (!toolName.includes(actionDelimiter)) { | ||
| return false; | ||
| } | ||
| const idx = toolName.indexOf(actionDelimiter); | ||
| const afterDelimiter = toolName.slice(idx + actionDelimiter.length); | ||
| return !afterDelimiter.startsWith('mcp_'); |
There was a problem hiding this comment.
isActionTool() currently returns false for any tool whose substring after _action_ starts with mcp_. That fixes the reported MCP suffix collision, but it also creates false negatives for legitimate OpenAPI action tools whose encoded/normalized domain begins with mcp_ (e.g. actions hosted on mcp.*). In that case the tool can be misrouted as an MCP tool (or bypass actionsEnabled gating) because other code paths key off _mcp_ presence. Consider making the collision guard more specific so action domains starting with mcp_ are still treated as action tools.
| */ | |
| export function isActionTool(toolName: string): boolean { | |
| if (!toolName.includes(actionDelimiter)) { | |
| return false; | |
| } | |
| const idx = toolName.indexOf(actionDelimiter); | |
| const afterDelimiter = toolName.slice(idx + actionDelimiter.length); | |
| return !afterDelimiter.startsWith('mcp_'); | |
| * | |
| * Specifically, we only treat the tool as a non-action in the collision case | |
| * where the substring before the first `_action_` ends with `_action` and the | |
| * substring after `_action_` starts with `mcp_`. This allows legitimate | |
| * action tools whose encoded domain begins with `mcp_` (e.g. `mcp_*`). | |
| */ | |
| export function isActionTool(toolName: string): boolean { | |
| if (!toolName.includes(actionDelimiter)) { | |
| return false; | |
| } | |
| const idx = toolName.indexOf(actionDelimiter); | |
| const beforeDelimiter = toolName.slice(0, idx); | |
| const afterDelimiter = toolName.slice(idx + actionDelimiter.length); | |
| // MCP collision guard: `<mcpToolNameEndingIn_action>_action_mcp_<server>` | |
| if (afterDelimiter.startsWith('mcp_') && beforeDelimiter.endsWith('_action')) { | |
| return false; | |
| } | |
| return true; |
| export const startupConfigKey = (isAuthenticated: boolean) => | ||
| [QueryKeys.startupConfig, isAuthenticated] as const; | ||
|
|
||
| export const useGetStartupConfig = ( | ||
| config?: UseQueryOptions<t.TStartupConfig>, | ||
| ): QueryObserverResult<t.TStartupConfig> => { | ||
| const queriesEnabled = useRecoilValue<boolean>(store.queriesEnabled); | ||
| const user = useRecoilValue<t.TUser | undefined>(store.user); | ||
| return useQuery<t.TStartupConfig>( | ||
| [QueryKeys.startupConfig], | ||
| startupConfigKey(!!user), | ||
| () => dataService.getStartupConfig(), |
There was a problem hiding this comment.
startupConfigKey() is defined as auth-sensitive, and useGetStartupConfig() caches under startupConfigKey(!!user). Several call sites in this PR use startupConfigKey(true) when reading from the cache; if user is still unset when those run, they will miss the already-fetched unauthenticated startup config and behave as if config is absent. Consider standardizing the boolean used for this key (e.g. derive from the same auth state everywhere, or provide a helper to read from both keys).
| onMutate: (vars) => { | ||
| setQueriesEnabled(false); | ||
| resetDefaultPreset(); | ||
| clearStates(); | ||
| queryClient.removeQueries(); | ||
| options?.onMutate?.(vars); | ||
| }, | ||
| // Queries re-enabled in setUserContext (AuthContext) after setTokenHeader runs | ||
| onSuccess: (...args) => { | ||
| setQueriesEnabled(true); | ||
| options?.onSuccess?.(...args); | ||
| }, | ||
| onError: (...args) => { | ||
| setQueriesEnabled(true); | ||
| options?.onError?.(...args); | ||
| }, |
There was a problem hiding this comment.
setQueriesEnabled(false) during onMutate can be re-enabled by LoginLayout's existing timeout that flips queriesEnabled back to true after 500ms when on the login page. That reintroduces the race this change is trying to prevent (queries firing before setTokenHeader() runs). Consider cancelling/guarding that login-page timeout while a login mutation is in flight, or using a different flag for "pause queries during login" that isn't auto-reset by LoginLayout.
…d as OpenAPI action tools Add `isActionTool()` helper that checks for the `_action_` delimiter while guarding against cross-delimiter collision with `_mcp_`. Replace all `includes(actionDelimiter)` classification checks with the new helper across backend and frontend.
Verify that `isActionTool` correctly rejects MCP tool names containing `_action` and that `loadAgentTools` does not filter them based on `actionsEnabled`. Add ToolIcon and definitions test cases.
- Use `!toolName.includes('_mcp_')` instead of checking only after the
first `_action_` occurrence, which missed MCP tools with `_action_` in
the middle of their name (e.g. `get_action_data_mcp_myserver`).
- Reference `Constants.mcp_delimiter` value via a local const to avoid
circular import from config.ts, with a comment explaining why.
- Remove dead `actionDelimiter` import from definitions.ts.
- Replace double-filter with single-pass partition in loadToolsForExecution.
- Add test for mid-name `_action_` collision case.
danny-avila
force-pushed
the
claude/stupefied-panini
branch
from
13a0598 to
af02511
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af025119a4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Only reject when `_mcp_` appears after `_action_` (the MCP suffix position). `_mcp_` before `_action_` is part of the operationId and is valid — e.g. `sync_mcp_state_action_api---example---com` is a legitimate action tool whose operationId happens to contain `_mcp_`.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9eb6891cf7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const mcpIdx = toolName.indexOf(mcpDelimiter); | ||
| return mcpIdx < 0 || mcpIdx < actionIdx; |
There was a problem hiding this comment.
_mcp_ as suffix marker, not any post-action substring
isActionTool() now returns false whenever _mcp_ appears after _action_, but action tool names are built as <operationId>_action_<encodedDomain> and the encoded domain can legally contain _mcp_ (underscores are preserved by domain encoding). In that case (for example, a domain like api_mcp_internal.com), real action tools are misclassified as non-action tools and get routed through the wrong loading path, so they may disappear from action handling. This regression is introduced by using indexOf('_mcp_') without verifying it is the MCP suffix delimiter.
Useful? React with 👍 / 👎.
…itation Expand JSDoc on isActionTool to explain the action/MCP format disambiguation and the theoretical false negative for non-RFC-compliant domains containing `_mcp_`. Add test documenting this known edge case.
…nny-avila#12512) * fix: prevent MCP tools with `_action` in name from being misclassified as OpenAPI action tools Add `isActionTool()` helper that checks for the `_action_` delimiter while guarding against cross-delimiter collision with `_mcp_`. Replace all `includes(actionDelimiter)` classification checks with the new helper across backend and frontend. * test: add coverage for MCP/action cross-delimiter collision Verify that `isActionTool` correctly rejects MCP tool names containing `_action` and that `loadAgentTools` does not filter them based on `actionsEnabled`. Add ToolIcon and definitions test cases. * fix: simplify isActionTool to handle all MCP name patterns - Use `!toolName.includes('_mcp_')` instead of checking only after the first `_action_` occurrence, which missed MCP tools with `_action_` in the middle of their name (e.g. `get_action_data_mcp_myserver`). - Reference `Constants.mcp_delimiter` value via a local const to avoid circular import from config.ts, with a comment explaining why. - Remove dead `actionDelimiter` import from definitions.ts. - Replace double-filter with single-pass partition in loadToolsForExecution. - Add test for mid-name `_action_` collision case. * fix: narrow MCP exclusion to delimiter position in isActionTool Only reject when `_mcp_` appears after `_action_` (the MCP suffix position). `_mcp_` before `_action_` is part of the operationId and is valid — e.g. `sync_mcp_state_action_api---example---com` is a legitimate action tool whose operationId happens to contain `_mcp_`. * fix: document positional _mcp_ guard and known RFC-invalid domain limitation Expand JSDoc on isActionTool to explain the action/MCP format disambiguation and the theoretical false negative for non-RFC-compliant domains containing `_mcp_`. Add test documenting this known edge case.
…nny-avila#12512) * fix: prevent MCP tools with `_action` in name from being misclassified as OpenAPI action tools Add `isActionTool()` helper that checks for the `_action_` delimiter while guarding against cross-delimiter collision with `_mcp_`. Replace all `includes(actionDelimiter)` classification checks with the new helper across backend and frontend. * test: add coverage for MCP/action cross-delimiter collision Verify that `isActionTool` correctly rejects MCP tool names containing `_action` and that `loadAgentTools` does not filter them based on `actionsEnabled`. Add ToolIcon and definitions test cases. * fix: simplify isActionTool to handle all MCP name patterns - Use `!toolName.includes('_mcp_')` instead of checking only after the first `_action_` occurrence, which missed MCP tools with `_action_` in the middle of their name (e.g. `get_action_data_mcp_myserver`). - Reference `Constants.mcp_delimiter` value via a local const to avoid circular import from config.ts, with a comment explaining why. - Remove dead `actionDelimiter` import from definitions.ts. - Replace double-filter with single-pass partition in loadToolsForExecution. - Add test for mid-name `_action_` collision case. * fix: narrow MCP exclusion to delimiter position in isActionTool Only reject when `_mcp_` appears after `_action_` (the MCP suffix position). `_mcp_` before `_action_` is part of the operationId and is valid — e.g. `sync_mcp_state_action_api---example---com` is a legitimate action tool whose operationId happens to contain `_mcp_`. * fix: document positional _mcp_ guard and known RFC-invalid domain limitation Expand JSDoc on isActionTool to explain the action/MCP format disambiguation and the theoretical false negative for non-RFC-compliant domains containing `_mcp_`. Add test documenting this known edge case.
Summary
Closes #12494
I fixed a bug where MCP tools with
_actionin their name are silently misclassified as OpenAPI action tools and filtered out based onactionsEnabled, making them invisible to models with no error or warning.isActionTool()guard inpackages/data-providerthat uses delimiter position to distinguish MCP tools from action tools: only reject when_mcp_appears after_action_(the MCP suffix position)._mcp_appearing before_action_is part of the operationId and remains valid (e.g.sync_mcp_state_action_api---example---com).includes(actionDelimiter)classification checks withisActionTool()acrossToolService.js,definitions.ts,ToolIcon.tsx, andAssistantPanel.tsx.actionDelimiterstring-manipulation uses (tool name construction and parsing) unchanged since those operate on confirmed action tools.actionDelimiterimport fromdefinitions.ts.loadToolsForExecutionfor the action/regular tool split.ToolService.spec.js,definitions.spec.ts, andToolIcon.test.tsx.Change Type
Testing
Verified with unit tests across three test suites (34 + 21 + 5 = 60 tests passing):
isActionTool('get_action_mcp_myserver')→false— end-of-name collision rejected.isActionTool('get_action_data_mcp_myserver')→false— mid-name collision rejected.isActionTool('sync_mcp_state_action_api---example---com')→true— action tool with_mcp_in operationId preserved.isActionTool('get_weather_action_api_example_com')→true— standard action tools detected.loadAgentToolswithdefinitionsOnly=truepreserves MCP tools containing_actionwhenactionsEnabledis disabled.getToolIconTypereturns'mcp'not'action'for MCP tools whose name ends with_action.loadToolDefinitionsroutes MCP tools with_actionin their name to the MCP path, not the action tool path.To reproduce the original bug manually:
get_action(or any name containing_action).Checklist