docs: correct the MCP glob example in the toolApproval policy comment - #14783
docs: correct the MCP glob example in the toolApproval policy comment#14783sakej wants to merge 1 commit into
Conversation
830ed0c to
cc4565a
Compare
danny-avila
left a comment
There was a problem hiding this comment.
Thanks — the underlying report is correct: mcp:server:* cannot match a LibreChat MCP tool key, so a rule written from the documented example selects nothing. But the replacement example has an off-by-one-underscore that reintroduces the same bug.
Constants.mcp_delimiter is '_mcp_', with a single leading underscore:
/** Delimiter for MCP tools */
mcp_delimiter = '_mcp_',So a real key is create_issue_mcp_github, not create_issue__mcp_github. The double underscore in the PR body and in the new comment text only appears when the raw tool name itself ends in _.
Running that through the same globToRegex reasoning:
| glob | regex | matches create_issue_mcp_github |
|---|---|---|
mcp:server:* (current) |
^mcp:server:.*$ |
no |
*__mcp_github (this PR) |
^.*__mcp_github$ |
no |
*_mcp_github |
^.*_mcp_github$ |
yes |
Could you change the example to `*${Constants.mcp_delimiter}<serverName>` — i.e. *_mcp_<serverName>, with create_issue_mcp_github as the sample key? Two smaller notes while you're in there:
- The "The fix" section describes the glob as
`*__mcp_`compiling to/^.*__mcp_$/, which is anchored on the delimiter and matches nothing at all. It also doesn't match the actual diff, which writes*__mcp_<serverName>. Worth aligning the description with the change. - The server segment in a key is the
normalizeServerName()form, not necessarily the rawlibrechat.yamlname, and raw tool names may themselves contain_mcp_(seesplitMCPToolKey's doc comment). A short parenthetical about the normalized form would make the example harder to misapply.
The rest checks out on my read: mapToolApprovalPolicy in packages/api/src/agents/hitl/policy.ts copies allow/deny/ask into ToolPolicyConfig verbatim, so the SDK really does see raw LibreChat tool keys with no colons.
cc4565a to
4226041
Compare
|
Thanks for the careful read, and for catching the delimiter. You're right: The example now uses I also handled the two smaller notes:
|
What
The JSDoc for the per-endpoint tool-approval policy in
packages/data-provider/src/config.tstells you to scope a rule to one MCP server withmcp:server:*. That glob never matches a real MCP tool key, so a rule written from the documented example silently selects nothing.Why it never matches
MCP tool keys are built as
`${rawToolName}${Constants.mcp_delimiter}${serverName}`, andmcp_delimiteris'_mcp_'. A real key looks likecreate_issue_mcp_github. No colons anywhere.mapToolApprovalPolicy(packages/api/src/agents/hitl/policy.ts) copies theallow/deny/askentries verbatim into the SDK'sToolPolicyConfig. No string rewriting happens on the way. In@librechat/agents@3.4.5,createToolPolicyHook'sglobToRegexescapes regex metacharacters, turns*into.*, and anchors the result, somcp:server:*compiles to/^mcp:server:.*$/. Test it against a real key:The SDK's own tests exercise the glob only against colon-form names like
mcp:github:create_issue, which no LibreChat tool key uses.The fix
Point the example at a glob that matches the real key format:
*_mcp_<serverName>(i.e.`*${Constants.mcp_delimiter}<serverName>`). It compiles to/^.*_mcp_<serverName>$/and selects every tool from that server. The example also notes that the server segment is thenormalizeServerName()form and that a raw tool name may itself contain the delimiter (persplitMCPToolKey's doc comment), so the glob isn't misapplied. One comment line changes; no code paths are touched.How I confirmed it
Constants.mcp_delimiterinconfig.tsonmain.mapToolApprovalPolicypasses the policy strings through unchanged.globToRegexin the pinned@librechat/agents@3.4.5, the versionapi/package.jsonresolves inpackage-lock.json.