HITL: "Always allow this tool" via set_tool_approval gateway meta-tool - #20
Conversation
Fall back to the existing approval flow when the remote lookup fails or returns malformed data, with regression coverage for approval, denial, cancellation, timeout, and recovery. — sent via Glean Tau
Use downstream annotations matched by server and tool when get_tool_approval fails. Require approval for write, destructive, unknown, or conflicting metadata, and keep successful remote decisions authoritative. — sent via Glean Tau
eshwar-sundar-glean
left a comment
eshwar-sundar-glean
left a comment
There was a problem hiding this comment.
Approving since its a port. Hope you have tested E2E
| * metadata remains cached: inputSchema for argument shaping, and downstream | ||
| * annotations for the read-only fallback when the remote lookup fails. | ||
| */ | ||
| function sanitizeSkillFile(filePath: string, text: string): string { |
There was a problem hiding this comment.
Are we okay doing this? would this require a network call always now?
Read tools are always allow anyway right, can we use that information to optimize network call count from the client?
There was a problem hiding this comment.
Agreed that we should optimize for read tools. Fixed, now we retain read-only annotations and use them to skip approval checks entirely. Approval preferences are still excluded from the cache, so write tools use current remote settings.
|
|
||
| let requiresApproval = true; | ||
| try { | ||
| requiresApproval = await getToolApproval(remoteClient, serverId, toolName); |
There was a problem hiding this comment.
#blocking, please clarify
Majority of tool calls would be read only tools right? and we dont allow setting approval bits for read only tools right? is this network call always required?
There was a problem hiding this comment.
Fixed, thanks for pointing out. Read-only tools now execute directly without an approval lookup or HITL prompt. Write tools still call get_tool_approval, and an explicit “Always Allow” choice is saved through set_tool_approval.
There was a problem hiding this comment.
Thanks. Make sure our idea of read only tools and what the glean server treats as read only tools are the same. If you have checked that and verified it end to end, code LGTM and can merge
eshwar-sundar-glean
left a comment
eshwar-sundar-glean
left a comment
There was a problem hiding this comment.
Would be good to address my comments before merge on performance front
Execute known read-only tools directly. Keep live approval lookups and explicit Always Allow persistence for write tools, with regression coverage for remote revocation. — sent via Glean Tau
| } | ||
|
|
||
| const remoteArgs = buildRemoteArgs(serverId, toolName, resolvedArgs); | ||
| // Read-only tools need no approval lookup, host capability check, or prompt. |
There was a problem hiding this comment.
#nit
I feel " host capability check or prompt" and stuff is not required, just add "Does not need approval lookup because Glean does not allow changing preferences for read tools" the later is the important part
eshwar-sundar-glean
left a comment
eshwar-sundar-glean
left a comment
There was a problem hiding this comment.
Thanks. Make sure our idea of read only tools and what the glean server treats as read only tools are the same. If you have checked that and verified it end to end, code LGTM and can merge
Explain why read-only tools skip approval lookups and document the canonical Glean predicate. Exercise all valid annotation combinations through skill caching and execution without changing runtime behavior. — sent via Glean Tau
2 participants
Summary
Add an Always Allow flow for approval-gated Glean MCP tools.
This PR implements the feature directly in the shared Glean MCP adapter used by this repository. It is a standalone implementation, not a dependency on another plugin repository.
set_tool_approvalwithALWAYS_ALLOWED.The implementation lives in the shared MCP source. It is included in the Claude and Codex local MCP targets. The Cursor target does not bundle this local MCP adapter and continues to use its separately connected MCP integration.
Validation
npm run test:bundle— 340 tests passednpm run typecheck:bundlenpm test— build and validation passed for Claude, Cursor, and Codex— sent via Glean Tau