feat(mcp): per-call approval card — S4a #41
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -219,6 +219,36 @@ function namespaceToolName(server, tool) { | |
| return full.slice(0, MAX_TOOL_NAME - tag.length - 1) + '_' + tag; | ||
| } | ||
|
|
||
| /** | ||
| * Could `name` have come out of namespaceToolName? The guard for anything that PERSISTS a tool name — | ||
| * today, "Always allow" writing a key into `levelcode.ai.mcp.toolPolicy`. | ||
| * | ||
| * It lives here, beside the function whose output it describes, because the caller was hand-rolling its | ||
| * own regex, and two copies of one naming rule is how they drift apart. | ||
| * | ||
| * Deliberately does NOT require the `__` separator, however much the `server__tool` shape invites it. | ||
| * When a server's name alone reaches the cap, truncation cuts INSIDE that first segment and the | ||
| * hash-tagged result carries no separator at all: | ||
| * | ||
| * namespaceToolName('s'.repeat(70), 'tool') -> 'sss…sss_a1b2c3' // 64 chars, no '__' | ||
| * | ||
| * Requiring it would reject a name this module itself produced, and "Always allow" would then silently | ||
| * do nothing for that server. The alphabet and the length are the real guarantee — they are what makes a | ||
| * name safe to write into settings — so those are what this checks. | ||
| * | ||
| * UNSAFE_KEYS is then rejected EXPLICITLY. The separator requirement used to exclude `__proto__` by | ||
| * accident (it has no non-underscore character before its `__`); dropping that requirement takes the | ||
| * accident with it, since underscores are otherwise legal. Stating it outright means the protection no | ||
| * longer depends on an unrelated rule staying a certain shape. | ||
| */ | ||
| function isNamespacedToolName(name) { | ||
| return typeof name === 'string' | ||
| && name.length > 0 | ||
| && name.length <= MAX_TOOL_NAME | ||
| && UNSAFE_KEYS.indexOf(name) === -1 | ||
| && /^[A-Za-z0-9_-]+$/.test(name); | ||
| } | ||
|
Comment on lines
+244
to
+250
Contributor
Author
There was a problem hiding this comment. Both of these are the same finding, and you are right — fixed in #56.
It now accepts exactly the two shapes
One correction to the reasoning in the second comment: truncation does not always leave a separator. When a server's name alone reaches the cap, the cut lands inside that first segment: So a plain "must contain The Because being too strict fails silently — "Always allow" writes nothing and the user is asked forever — the accept side is swept rather than sampled: 560 (server, tool) length pairs across the truncation boundary must all validate, and the test asserts the sweep actually reached the truncated and separator-less regions so it cannot go vacuous. |
||
|
|
||
| /** | ||
| * Assign a final, unique, provider-legal name to every (server, tool) pair — the last line of defence | ||
| * before names reach the wire. Collisions (with a built-in, or between two servers whose names | ||
|
|
@@ -400,17 +430,60 @@ function classifyMcpTool(name, policy, annotations) { | |
| * @returns {string} | ||
| */ | ||
| function explainMcpRefusal(name, verdict) { | ||
| // Reached only when there is NO interactive approval to fall back on (a headless run, a test harness). | ||
| // With a webview present, S4 shows the per-call card instead of this message. | ||
| const head = 'ERROR: the MCP tool "' + name + '" is not approved to run (' + verdict.reason + '). '; | ||
| const fix = verdict.policyCanAllow | ||
| ? 'No interactive approval is available here, so the only way to permit it is for the USER to add ' | ||
| + '"' + name + '": "allow" to the "levelcode.ai.mcp.toolPolicy" setting. ' | ||
| : 'Such tools always require per-call approval and CANNOT be enabled through the allow-list, so ' | ||
| + 'there is no way to run it in this non-interactive context. '; | ||
| return head + fix + 'Do NOT retry it in this run — continue without it, or tell the user what you needed it for.'; | ||
| } | ||
|
|
||
| // A tool call's arguments can be large, and the approval card must not be blown open by one. See | ||
| // describeMcpCall — the card is capped, the full args still reach the server if approved. | ||
| const MAX_ARG_CHARS = 2000; | ||
|
|
||
| /** Pretty, bounded JSON for the args shown on the approval card. Never throws (circular/huge input). */ | ||
| function previewArgs(args) { | ||
| if (args == null) { return ''; } | ||
| let text; | ||
| try { text = JSON.stringify(args, null, 2); } | ||
| catch { try { text = String(args); } catch { text = '[unserializable arguments]'; } } | ||
| if (text == null) { return ''; } | ||
| return text.length > MAX_ARG_CHARS ? text.slice(0, MAX_ARG_CHARS - 1) + '…' : text; | ||
| } | ||
|
|
||
| /** | ||
| * Shape an MCP call for the approval card (S4) — exactly what the user reads before deciding. | ||
| * | ||
| * Unlike the debug log (G4), the arguments are shown in FULL here, only length-capped. That is not an | ||
| * oversight: the card is ephemeral UI shown to the person who owns the credentials, and seeing the real | ||
| * arguments — the repo it will touch, the id it will delete — IS the decision. Redacting them would make | ||
| * the prompt meaningless. Nothing here is persisted; the card is not the transcript. | ||
| * | ||
| * `canAllowAlways` is false for a destructive tool: a server-marked-destructive tool can never be moved | ||
| * to the allow-list (classifyMcpTool tightens on it), so the card must not offer a button that would do | ||
| * nothing. Derived from the same annotation the classifier reads, so the two cannot disagree. | ||
| * | ||
| * @param {string} name namespaced tool name (server__tool) | ||
| * @param {*} args the arguments the model produced for this call | ||
| * @param {{server?:string, tool?:string, annotations?:object}} [route] | ||
| * @returns {{server:string, tool:string, argsText:string, destructive:boolean, canAllowAlways:boolean}} | ||
| */ | ||
| function describeMcpCall(name, args, route) { | ||
| const r = route || {}; | ||
| const fallback = String(name == null ? '' : name).split(NAME_SEPARATOR); | ||
| const server = typeof r.server === 'string' && r.server ? r.server : (fallback[0] || String(name)); | ||
| const tool = typeof r.tool === 'string' && r.tool ? r.tool : (fallback.slice(1).join(NAME_SEPARATOR) || String(name)); | ||
| const destructive = !!(r.annotations && r.annotations.destructiveHint === true); | ||
| return { server, tool, argsText: previewArgs(args), destructive, canAllowAlways: !destructive }; | ||
| } | ||
|
|
||
| module.exports = { | ||
| loadServerConfig, userScopedSetting, namespaceToolName, isNamespacedToolName, assignToolNames, | ||
| buildAgentTools, safeCopy, | ||
| toolCountsByServer, classifyMcpTool, explainMcpRefusal, describeMcpCall, | ||
| BUILTIN_TOOL_NAMES, MAX_TOOL_NAME, MAX_TOOL_DESC, MAX_ARG_CHARS, MAX_SERVERS, MAX_TOOLS_PER_SERVER, WORKSPACE_CONFIG_PATH | ||
| }; | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct — fixed in #56.
userScopedSettingreturnsinfo.globalValuewhatever its type, so nothing betweensettings.jsonandsafeCopywas checking the shape. A policy that is accidentally a string would havesafeCopyfaithfully copy its character indices and write that back:which destroys the user's policy rather than repairing it. An array would do the same with numeric indices.
mcpAllowAlwaysnow normalizes first — a non-plain-object value is discarded, not migrated, on the grounds that we cannot know what a corrupted policy was meant to say, and silently inventing one is worse than starting clean.