diff --git a/docs/plans/2026-07-24-mcp-tool-policy-design.md b/docs/plans/2026-07-24-mcp-tool-policy-design.md new file mode 100644 index 0000000000..0fdb4ba157 --- /dev/null +++ b/docs/plans/2026-07-24-mcp-tool-policy-design.md @@ -0,0 +1,114 @@ +# MCP Tool policy Module design + +## Outcome + +Create one deep private Module for MCP Tool allow and deny policy, then route the existing hosted and runtime adapters through it without changing public behavior. + +The Module target is `src/agent/mcp-tool-policy.ts`. It owns the policy predicate, list filtering, execution guard, live policy mutation behavior, and the exact permission failure construction for MCP Tool policy checks. Existing callers keep their public Interface and continue to compose project scoped Tool behavior, activation gates, access profiles, Studio project switching, and credential binding outside this Module. + +## Current evidence + +- `src/agent/types.ts` exports `AgentMcpToolPolicy` with `allow?: string[]`, `deny?: string[]`, and `approval?: "never"`. +- `src/agent/hosted/child-fork-tool-sources.ts` defines `isMcpToolAllowed`, `filterHostToolsByMcpPolicy`, and `filterToolDefinitionsByMcpPolicy`. It filters Studio host Tools and remote Tool definitions, then wraps host Tool execution to enforce live policy checks. +- `src/agent/hosted/project-remote-tool-source.ts` defines `isHostedMcpToolAllowed` and exports `createHostedMcpToolPolicySource`. It filters `RemoteToolSource.listTools()` and blocks `executeTool()` with `PERMISSION_DENIED.create({ detail: ... })`. +- `src/agent/runtime/mcp-server-tool-sources.ts` defines another `isToolAllowed`, `filterToolDefinitions`, and `createMcpToolPolicySource`. It also embeds the HTTP-server denial detail string in `createMcpServerToolSource`. +- `src/agent/hosted/project-remote-tool-source.ts` separately owns project scoped execution through `createHostedProjectRemoteToolSource(...)`. Its `activatedRemoteToolNames` Set is a live project activation gate, not MCP policy. +- `src/agent/hosted/child-fork-tool-sources.test.ts` locks API, generic, and Studio policy filtering plus live mutation of the Studio policy object after wrapping. +- `src/agent/hosted/project-remote-tool-source.test.ts` locks `activatedRemoteToolNames` as the live execution gate for project remote Tools. +- `src/agent/runtime/mcp-server-tool-sources.test.ts` locks runtime MCP server filtering, bearer auth, first-party project binding, and denied execution behavior. + +## Proposed Module + +`src/agent/mcp-tool-policy.ts` + +```ts +import type { HostToolSet, RemoteToolSource } from "#veryfront/tool"; +import type { AgentMcpToolPolicy } from "./types.ts"; + +export type McpToolPolicyGate = { + allows(toolName: string): boolean; + filterDefinitions(definitions: readonly T[]): T[]; + assertAllowed(toolName: string): void; +}; + +export function createMcpToolPolicyGate( + policy: AgentMcpToolPolicy | undefined, + options?: { deniedDetail?: (toolName: string) => string }, +): McpToolPolicyGate; + +export function wrapRemoteToolSourceWithMcpPolicy( + source: RemoteToolSource, + policy: AgentMcpToolPolicy | undefined, + options?: { deniedDetail?: (toolName: string, sourceId: string) => string }, +): RemoteToolSource; + +export function wrapHostToolSetWithMcpPolicy( + tools: HostToolSet, + policy: AgentMcpToolPolicy | undefined, + options?: { deniedDetail?: (toolName: string) => string }, +): HostToolSet; +``` + +The Module must use the existing `AgentMcpToolPolicy` Interface rather than declaring a competing policy type. `approval?: "never"` remains accepted compatibility data on the policy object. The allow and deny gate does not interpret `approval` because current policy checks do not interpret it. + +The Module must read `policy.allow` and `policy.deny` on every check. It must not snapshot Sets or arrays because existing tests mutate policy objects after wrapping. Deny remains a hard ceiling. Allow remains a positive selection only when present. With no allow and no deny, adapters preserve source identity where current behavior does so. + +## Ownership + +The new Module owns: + +- The allow and deny predicate. +- Filtering ordered lists of Tool definitions by name. +- Wrapping `RemoteToolSource` execution with a permission guard. +- Wrapping `HostToolSet` execution with a permission guard while preserving Tool definition fields. +- Canonical permission errors for policy denial, with caller-supplied detail builders for existing exact messages. + +The Module does not own: + +- Project scoped Tool hydration or `project_reference` replacement. +- `activatedRemoteToolNames` execution gating. +- The `approval` field on `AgentMcpToolPolicy`. +- Veryfront API Tool access profile filtering. +- Remote MCP transport construction, auth headers, bearer token binding, or project credential binding. +- Studio MCP creation, project switch confirmation, or child fork Tool assembly. +- Runtime `tools` boolean resolution and inherited source selection. + +## Interfaces and adapters + +`createMcpToolPolicyGate(policy)` is the small Interface for semantics. It keeps policy Depth by hiding the precedence rules and live mutation behavior. + +`wrapRemoteToolSourceWithMcpPolicy(...)` is the Adapter for remote MCP sources. It returns the original source when policy has neither `allow` nor `deny` to preserve current identity behavior. When policy is non-empty, it filters `listTools()` and blocks `executeTool()`. + +`wrapHostToolSetWithMcpPolicy(...)` is the Adapter for materialized host Tools. It filters the visible Tool set and guards each wrapped `execute` function. It must preserve existing Tool ordering from `Object.entries()` and keep non-executable Tool definitions unchanged except for filtered visibility. + +Existing named helpers stay as compatibility shims: + +- `createHostedMcpToolPolicySource(source, policy)` in `src/agent/hosted/project-remote-tool-source.ts` delegates to `wrapRemoteToolSourceWithMcpPolicy(...)` with detail `Tool "" is not allowed for this MCP server`. +- Runtime private `createMcpToolPolicySource(...)` in `src/agent/runtime/mcp-server-tool-sources.ts` delegates with detail `Tool "" is not allowed for MCP server ""`. +- Runtime HTTP server wrapping in `createMcpServerToolSource(...)` delegates with detail `Tool "" is not allowed for MCP server ""`. +- Child fork policy filtering in `src/agent/hosted/child-fork-tool-sources.ts` delegates host Tool wrapping and definition filtering to the Module. The API access profile filter still runs before policy filtering as it does today. + +## Invariants + +- Deny wins over allow. +- If `allow` exists, only listed Tool names pass unless denied. +- If `allow` is absent, all Tool names pass unless denied. +- Empty policy preserves current behavior. +- Policy reads are live. Mutating the same policy object after wrapping changes future list and execution results. +- `approval` on `AgentMcpToolPolicy` remains valid input and does not change allow or deny behavior. +- Denied execution throws `PERMISSION_DENIED.create({ detail: ... })` and preserves existing detail strings at each caller. +- List filtering preserves the original order of allowed Tool definitions. +- Project activation remains distinct from MCP policy. `activatedRemoteToolNames` continues to decide project remote Tool listing and execution after policy wrapping. +- Public exports and signatures do not change. + +## Risks + +- Wrapping order can change behavior if policy is applied before project scoped catalogs in paths that depend on source identity or context. Keep the hosted project source order as `policySource -> createHostedProjectRemoteToolSource(...)`, matching current behavior. +- The runtime path has two exact denial message forms. Preserve them through Adapter detail options rather than standardizing text. +- Host Tool wrappers must keep live policy mutation. Avoid converting policy arrays to Sets unless done per check or behind a live reader. +- Empty-policy identity preservation may be relied on by tests even if not documented. Preserve it for remote source wrappers and avoid wrapping host Tools when no policy exists. +- Future work may assign meaning to `approval`. This refactor must not preclude that, but it must not implement approval behavior. + +## Rollback + +Rollback is clean: remove `src/agent/mcp-tool-policy.ts`, restore the three local policy helpers in the touched callers, and revert the new focused tests. Because this is a private Module, no public migration is required. diff --git a/docs/plans/2026-07-24-mcp-tool-policy-implementation.md b/docs/plans/2026-07-24-mcp-tool-policy-implementation.md new file mode 100644 index 0000000000..b28e9657db --- /dev/null +++ b/docs/plans/2026-07-24-mcp-tool-policy-implementation.md @@ -0,0 +1,327 @@ +# MCP Tool Policy Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Move MCP Tool allow and deny policy into one private Module while preserving all current hosted and runtime behavior. + +**Architecture:** Add `src/agent/mcp-tool-policy.ts` as the single policy Module. Keep existing hosted and runtime entrypoints as adapters and compatibility shims. Keep project activation, API access profiles, transport setup, auth, and `AgentMcpToolPolicy.approval` outside the allow and deny gate. + +**Tech Stack:** Deno, TypeScript, Veryfront internal imports, `PERMISSION_DENIED`, `AgentMcpToolPolicy`, `RemoteToolSource`, `HostToolSet`, colocated Deno tests. + +## Global Constraints + +- Preserve public API compatibility. +- Use `AgentMcpToolPolicy` from `src/agent/types.ts`; do not create a competing public policy type. +- Accept `approval?: "never"` on the policy object and do not interpret it in the allow and deny gate. +- Keep `activatedRemoteToolNames` as a separate project activation gate. +- Preserve current Tool ordering, allow and deny precedence, empty policy behavior, live policy mutation, exact denial details, and public exports. +- Use `PERMISSION_DENIED.create({ detail: ... })` for denied policy execution. +- Add no dependencies. +- Use `apply_patch` for edits. + +--- + +## File structure + +- Create: `src/agent/mcp-tool-policy.ts` + - Owns allow and deny semantics, definition filtering, remote source wrapping, host Tool wrapping, and denial error construction. +- Create: `src/agent/mcp-tool-policy.test.ts` + - Locks the private Module contract directly before adapter refactors. +- Modify: `src/agent/hosted/child-fork-tool-sources.ts` + - Removes local policy predicate and host Tool policy wrapper. + - Delegates Studio host Tool wrapping and remote definition filtering to `src/agent/mcp-tool-policy.ts`. +- Modify: `src/agent/hosted/project-remote-tool-source.ts` + - Keeps `createHostedMcpToolPolicySource(...)` as a compatibility shim. + - Delegates remote source policy wrapping to `src/agent/mcp-tool-policy.ts`. +- Modify: `src/agent/runtime/mcp-server-tool-sources.ts` + - Removes duplicate runtime predicate and wrapper logic. + - Delegates HTTP, injected, constrained, inherited, and first-party source policy wrapping to `src/agent/mcp-tool-policy.ts`. + +## Baseline + +- [ ] **Step 1: Run focused baseline tests before edits** + +```bash +deno test --no-check --allow-all src/agent/hosted/child-fork-tool-sources.test.ts src/agent/hosted/project-remote-tool-source.test.ts src/agent/runtime/mcp-server-tool-sources.test.ts +``` + +Expected: PASS on the baseline branch. If this fails, stop implementation and record the unrelated baseline failure before editing source. + +## Task 1: Lock the shared Module contract + +**Files:** + +- Create: `src/agent/mcp-tool-policy.test.ts` + +**Interfaces:** + +- Consumes: `AgentMcpToolPolicy` from `src/agent/types.ts`, `RemoteToolSource` and `HostToolSet` from `#veryfront/tool`. +- Produces: Test coverage for `createMcpToolPolicyGate(...)`, `wrapRemoteToolSourceWithMcpPolicy(...)`, and `wrapHostToolSetWithMcpPolicy(...)`. + +- [ ] **Step 1: Write the failing Module tests** + +Create `src/agent/mcp-tool-policy.test.ts` with tests for: + +- `createMcpToolPolicyGate(undefined)` allows all names. +- Deny wins over allow. +- Allow filters definition order without sorting. +- `approval: "never"` does not affect allow and deny behavior. +- Mutating the same policy object after gate creation changes `allows(...)`, `filterDefinitions(...)`, and `assertAllowed(...)`. +- `wrapRemoteToolSourceWithMcpPolicy(...)` returns the same source for empty policy. +- Wrapped remote `listTools()` filters dynamically. +- Wrapped remote `executeTool()` blocks denied names before calling the source. +- `wrapHostToolSetWithMcpPolicy(...)` filters visible Tools and blocks execution if policy later changes after wrapping. +- A detail builder preserves exact caller-provided denial text. + +- [ ] **Step 2: Run the red test** + +```bash +deno test --no-check --allow-all src/agent/mcp-tool-policy.test.ts +``` + +Expected: FAIL because `src/agent/mcp-tool-policy.ts` does not exist. + +## Task 2: Implement the shared Module + +**Files:** + +- Create: `src/agent/mcp-tool-policy.ts` +- Test: `src/agent/mcp-tool-policy.test.ts` + +**Interfaces:** + +- Consumes: `AgentMcpToolPolicy`, `HostToolSet`, `RemoteToolSource`, `PERMISSION_DENIED`. +- Produces: + +```ts +export type McpToolPolicyGate = { + allows(toolName: string): boolean; + filterDefinitions(definitions: readonly T[]): T[]; + assertAllowed(toolName: string): void; +}; + +export function createMcpToolPolicyGate( + policy: AgentMcpToolPolicy | undefined, + options?: { deniedDetail?: (toolName: string) => string }, +): McpToolPolicyGate; + +export function wrapRemoteToolSourceWithMcpPolicy( + source: RemoteToolSource, + policy: AgentMcpToolPolicy | undefined, + options?: { deniedDetail?: (toolName: string, sourceId: string) => string }, +): RemoteToolSource; + +export function wrapHostToolSetWithMcpPolicy( + tools: HostToolSet, + policy: AgentMcpToolPolicy | undefined, + options?: { deniedDetail?: (toolName: string) => string }, +): HostToolSet; +``` + +- [ ] **Step 1: Add minimal implementation** + +Implementation requirements: + +- Import `PERMISSION_DENIED` from `#veryfront/errors`. +- Import `HostToolSet` and `RemoteToolSource` as types from `#veryfront/tool`. +- Import `AgentMcpToolPolicy` as a type from `./types.ts`. +- Treat policy as empty only when both `policy?.allow` and `policy?.deny` are absent. +- Read `policy.allow` and `policy.deny` inside each check. +- Ignore `policy.approval`. +- Use `Object.entries(tools)` to preserve host Tool ordering. +- Preserve each host Tool definition with object spread and replace only `execute` when it exists. +- Call the original host `execute(toolInput, execOptions)` unchanged. +- Call the original remote `source.listTools(context)` and `source.executeTool(toolName, args, context)` unchanged. +- Throw `PERMISSION_DENIED.create({ detail: detailString })` for denied execution. + +- [ ] **Step 2: Run the Module test** + +```bash +deno test --no-check --allow-all src/agent/mcp-tool-policy.test.ts +``` + +Expected: PASS. + +## Task 3: Refactor hosted child fork policy call sites + +**Files:** + +- Modify: `src/agent/hosted/child-fork-tool-sources.ts` +- Test: `src/agent/hosted/child-fork-tool-sources.test.ts` + +**Interfaces:** + +- Consumes: `createMcpToolPolicyGate(...)` and `wrapHostToolSetWithMcpPolicy(...)` from `../mcp-tool-policy.ts`. +- Produces: Existing `prepareDefaultHostedChildForkToolSources(...)` behavior with shared policy semantics. + +- [ ] **Step 1: Replace local host and definition filtering** + +Implementation requirements: + +- Remove `isMcpToolAllowed(...)`. +- Remove `filterHostToolsByMcpPolicy(...)`. +- Remove `filterToolDefinitionsByMcpPolicy(...)`. +- Remove the direct `PERMISSION_DENIED` import if no longer used. +- Keep `AGENT_ERROR` import unchanged. +- Import `createMcpToolPolicyGate` and `wrapHostToolSetWithMcpPolicy` from `../mcp-tool-policy.ts`. +- For Studio host Tools, call: + +```ts +const policyTools = wrapHostToolSetWithMcpPolicy(studioTools.tools, server.toolPolicy, { + deniedDetail: (toolName) => `Tool "${toolName}" is not allowed for this MCP server`, +}); +``` + +- For remote definitions, keep API access profile filtering first, then call: + +```ts +const definitions = createMcpToolPolicyGate(server.toolPolicy).filterDefinitions( + accessFilteredDefinitions, +); +``` + +- Do not change `createHostedMcpToolPolicySource(...)` usage in this task. + +- [ ] **Step 2: Run hosted child fork tests** + +```bash +deno test --no-check --allow-all src/agent/hosted/child-fork-tool-sources.test.ts src/agent/mcp-tool-policy.test.ts +``` + +Expected: PASS. + +## Task 4: Refactor hosted project remote policy shim + +**Files:** + +- Modify: `src/agent/hosted/project-remote-tool-source.ts` +- Test: `src/agent/hosted/project-remote-tool-source.test.ts` + +**Interfaces:** + +- Consumes: `wrapRemoteToolSourceWithMcpPolicy(...)` from `../mcp-tool-policy.ts`. +- Produces: Existing `createHostedMcpToolPolicySource(...)` export as a compatibility shim. + +- [ ] **Step 1: Replace hosted remote policy implementation** + +Implementation requirements: + +- Remove `isHostedMcpToolAllowed(...)`. +- Remove the direct `PERMISSION_DENIED` import only if no other code in the file uses it. +- Keep `createHostedMcpToolPolicySource(source, policy)` exported with the same signature. +- Implement the shim as: + +```ts +return wrapRemoteToolSourceWithMcpPolicy(source, policy, { + deniedDetail: (toolName) => `Tool "${toolName}" is not allowed for this MCP server`, +}); +``` + +- Keep `createHostedProjectRemoteToolSourceFromConfig(...)` wrapping the raw source before project scoped source creation. +- Do not change `activatedRemoteToolNames` logic. + +- [ ] **Step 2: Run hosted project remote tests** + +```bash +deno test --no-check --allow-all src/agent/hosted/project-remote-tool-source.test.ts src/agent/mcp-tool-policy.test.ts +``` + +Expected: PASS. + +## Task 5: Refactor runtime MCP source policy + +**Files:** + +- Modify: `src/agent/runtime/mcp-server-tool-sources.ts` +- Test: `src/agent/runtime/mcp-server-tool-sources.test.ts` + +**Interfaces:** + +- Consumes: `wrapRemoteToolSourceWithMcpPolicy(...)` from `../mcp-tool-policy.ts`. +- Produces: Existing runtime MCP source behavior with shared policy semantics. + +- [ ] **Step 1: Replace runtime duplicate policy code** + +Implementation requirements: + +- Remove `isToolAllowed(...)`. +- Remove `filterToolDefinitions(...)`. +- Replace the private `createMcpToolPolicySource(...)` body with delegation to `wrapRemoteToolSourceWithMcpPolicy(...)`. +- For the private source-id based wrapper, preserve this detail: + +```ts +`Tool "${toolName}" is not allowed for MCP server "${sourceId}"`; +``` + +- For HTTP server wrapping in `createMcpServerToolSource(...)`, preserve this detail: + +```ts +`Tool "${toolName}" is not allowed for MCP server "${server.id}"`; +``` + +- Keep auth resolution, bootstrap project binding, inherited source selection, credential binding, and first-party source selection untouched. + +- [ ] **Step 2: Run runtime MCP source tests** + +```bash +deno test --no-check --allow-all src/agent/runtime/mcp-server-tool-sources.test.ts src/agent/mcp-tool-policy.test.ts +``` + +Expected: PASS. + +## Task 6: Compatibility regression pass + +**Files:** + +- Validate all files touched by Tasks 1 through 5. + +- [ ] **Step 1: Run the focused policy suite** + +```bash +deno test --no-check --allow-all src/agent/mcp-tool-policy.test.ts src/agent/hosted/child-fork-tool-sources.test.ts src/agent/hosted/project-remote-tool-source.test.ts src/agent/runtime/mcp-server-tool-sources.test.ts +``` + +Expected: PASS. + +- [ ] **Step 2: Run adjacent Tool and remote MCP tests** + +```bash +deno test --no-check --allow-all src/tool/project-scoped-remote-tools.test.ts src/tool/remote-mcp.test.ts src/agent/runtime/tool-discovery-execution-gate.test.ts +``` + +Expected: PASS. + +- [ ] **Step 3: Run the broad unit suite** + +```bash +deno test --no-check --allow-all --parallel '--ignore=tests,src/workflow/__tests__,cli/commands/*.integration.test.ts' +``` + +Expected: PASS. If broad tests expose unrelated failures, keep focused passing evidence and record exact unrelated failures for root triage. + +- [ ] **Step 4: Run diff hygiene** + +```bash +git diff --check +``` + +Expected: no output and exit code `0`. + +## Review checklist + +- [ ] No public export map or import map changes were made. +- [ ] No public copy changed. +- [ ] No dependency changes were made. +- [ ] `AgentMcpToolPolicy` remains the policy type and `approval` remains accepted but uninterpreted. +- [ ] Denied execution still blocks before source execution, proven by call counters in tests. +- [ ] Host Tool live policy mutation remains covered by the Studio test and new Module test. +- [ ] Project activation live mutation remains covered in `src/agent/hosted/project-remote-tool-source.test.ts`. +- [ ] Empty-policy identity is preserved for remote source wrappers. + +## Handoff notes + +- Implement in this MCP policy worktree only. +- Keep the main checkout untouched. +- Use `apply_patch` for edits. +- Do not delete caller tests until the new Module tests and caller compatibility tests both pass. +- If cleanup removes duplicate tests, delete only policy-only duplication and keep adapter tests that prove ordering, identity, and exact errors. diff --git a/src/agent/hosted/child-fork-tool-sources.ts b/src/agent/hosted/child-fork-tool-sources.ts index 14a7d9221b..30de2dc8b1 100644 --- a/src/agent/hosted/child-fork-tool-sources.ts +++ b/src/agent/hosted/child-fork-tool-sources.ts @@ -4,10 +4,8 @@ import { type HostToolSet, type RemoteMCPToolSourceConfig, type RemoteToolSource, - type ToolDefinition, - type ToolExecutionContext, } from "#veryfront/tool"; -import { AGENT_ERROR, PERMISSION_DENIED } from "#veryfront/errors"; +import { AGENT_ERROR } from "#veryfront/errors"; import { type AgentServiceMcpServerConfig, createAgentServiceRemoteMcpConfig, @@ -31,9 +29,9 @@ import { buildDefaultHostedChildForkToolSet, type DefaultHostedChildForkToolAssemblySourceResult, } from "./child-requested-tools.ts"; +import { createMcpToolPolicyGate, wrapHostToolSetWithMcpPolicy } from "../mcp-tool-policy.ts"; import { filterVeryfrontApiToolDefinitionsWithAccessProfile } from "./veryfront-api-tool-access.ts"; import { createHostedMcpToolPolicySource } from "./project-remote-tool-source.ts"; -import type { AgentMcpToolPolicy } from "../types.ts"; /** Public API contract for hosted child fork tool sources logger. */ export type HostedChildForkToolSourcesLogger = { @@ -85,56 +83,6 @@ export type PrepareDefaultHostedChildForkSandboxToolSourcesInput = ) => Promise; }; -function isMcpToolAllowed(toolName: string, policy: AgentMcpToolPolicy | undefined): boolean { - if (policy?.deny?.includes(toolName)) { - return false; - } - - return policy?.allow ? policy.allow.includes(toolName) : true; -} - -function filterHostToolsByMcpPolicy( - tools: HostToolSet, - policy: AgentMcpToolPolicy | undefined, -): HostToolSet { - if (!policy?.allow && !policy?.deny) { - return tools; - } - - return Object.fromEntries( - Object.entries(tools) - .filter(([toolName]) => isMcpToolAllowed(toolName, policy)) - .map(([toolName, toolDefinition]) => [ - toolName, - { - ...toolDefinition, - execute: toolDefinition.execute - ? (toolInput: unknown, execOptions?: ToolExecutionContext) => { - if (!isMcpToolAllowed(toolName, policy)) { - throw PERMISSION_DENIED.create({ - detail: `Tool "${toolName}" is not allowed for this MCP server`, - }); - } - - return toolDefinition.execute?.(toolInput, execOptions); - } - : toolDefinition.execute, - }, - ]), - ); -} - -function filterToolDefinitionsByMcpPolicy( - definitions: readonly ToolDefinition[], - policy: AgentMcpToolPolicy | undefined, -): ToolDefinition[] { - if (!policy?.allow && !policy?.deny) { - return [...definitions]; - } - - return definitions.filter((definition) => isMcpToolAllowed(definition.name, policy)); -} - /** Prepare default hosted child fork tool sources. */ export async function prepareDefaultHostedChildForkToolSources( input: PrepareDefaultHostedChildForkToolSourcesInput, @@ -163,7 +111,13 @@ export async function prepareDefaultHostedChildForkToolSources( ? { createRemoteToolSource: input.createRemoteToolSource } : {}), }); - const policyTools = filterHostToolsByMcpPolicy(studioTools.tools, server.toolPolicy); + const policyTools = wrapHostToolSetWithMcpPolicy( + studioTools.tools, + server.toolPolicy, + { + deniedDetail: (toolName) => `Tool "${toolName}" is not allowed for this MCP server`, + }, + ); studioMcpTools = { ...studioMcpTools, ...policyTools, @@ -191,9 +145,8 @@ export async function prepareDefaultHostedChildForkToolSources( projectId: input.getProjectId() ?? null, }) : rawDefinitions; - const definitions = filterToolDefinitionsByMcpPolicy( + const definitions = createMcpToolPolicyGate(server.toolPolicy).filterDefinitions( accessFilteredDefinitions, - server.toolPolicy, ); remoteMcpTools = { ...remoteMcpTools, diff --git a/src/agent/hosted/project-remote-tool-source.ts b/src/agent/hosted/project-remote-tool-source.ts index 919fd83fc2..a28b05c1eb 100644 --- a/src/agent/hosted/project-remote-tool-source.ts +++ b/src/agent/hosted/project-remote-tool-source.ts @@ -15,6 +15,7 @@ import { defaultAgentServiceMcpServers, } from "../service/mcp-server-config.ts"; import type { AgentMcpToolPolicy } from "../types.ts"; +import { wrapRemoteToolSourceWithMcpPolicy } from "../mcp-tool-policy.ts"; import { CONFIG_INVALID, PERMISSION_DENIED } from "#veryfront/errors"; import { toChildRunToolInputRecord } from "../child-run/execution-support.ts"; import type { RuntimeClientProfile } from "../runtime/client-profile.ts"; @@ -344,42 +345,13 @@ function createHostedProjectRemoteToolSourceFromConfig( }); } -function isHostedMcpToolAllowed( - toolName: string, - policy: AgentMcpToolPolicy | undefined, -): boolean { - if (policy?.deny?.includes(toolName)) { - return false; - } - - return policy?.allow ? policy.allow.includes(toolName) : true; -} - export function createHostedMcpToolPolicySource( source: RemoteToolSource, policy: AgentMcpToolPolicy | undefined, ): RemoteToolSource { - if (!policy?.allow && !policy?.deny) { - return source; - } - - return { - id: source.id, - async listTools(context) { - return (await source.listTools(context)).filter((toolDefinition) => - isHostedMcpToolAllowed(toolDefinition.name, policy) - ); - }, - executeTool(toolName, args, context) { - if (!isHostedMcpToolAllowed(toolName, policy)) { - throw PERMISSION_DENIED.create({ - detail: `Tool "${toolName}" is not allowed for this MCP server`, - }); - } - - return source.executeTool(toolName, args, context); - }, - }; + return wrapRemoteToolSourceWithMcpPolicy(source, policy, { + deniedDetail: (toolName) => `Tool "${toolName}" is not allowed for this MCP server`, + }); } /** Create hosted project remote tool sources. */ diff --git a/src/agent/mcp-tool-policy.test.ts b/src/agent/mcp-tool-policy.test.ts new file mode 100644 index 0000000000..b8be458936 --- /dev/null +++ b/src/agent/mcp-tool-policy.test.ts @@ -0,0 +1,284 @@ +import "#veryfront/schemas/_test-setup.ts"; +import { assertEquals, assertStrictEquals } from "#veryfront/testing/assert.ts"; +import { describe, it } from "#veryfront/testing/bdd.ts"; +import { VeryfrontError } from "#veryfront/errors"; +import type { AgentMcpToolPolicy } from "./types.ts"; +import { + createMcpToolPolicyGate, + wrapHostToolSetWithMcpPolicy, + wrapRemoteToolSourceWithMcpPolicy, +} from "./mcp-tool-policy.ts"; +import type { HostToolSet, RemoteToolSource, ToolDefinition } from "#veryfront/tool"; + +const emptyParameters = { type: "object" as const, properties: {} }; + +function remoteTool(name: string): ToolDefinition { + return { + name, + description: `${name} tool`, + parameters: emptyParameters, + }; +} + +function remoteSource(tools: ToolDefinition[], calls: string[] = []): RemoteToolSource { + return { + id: "docs", + listTools: () => Promise.resolve(tools), + executeTool: (toolName, args, context) => { + calls.push(`${toolName}:${String(args.value)}:${String(context?.projectId)}`); + return Promise.resolve({ ok: true, toolName }); + }, + }; +} + +function remoteSourceWithNonEnumerableId( + tools: ToolDefinition[], + calls: string[] = [], +): RemoteToolSource { + const source = { + listTools: () => Promise.resolve(tools), + executeTool: (toolName: string, args: Record, context) => { + calls.push(`${toolName}:${String(args.value)}:${String(context?.projectId)}`); + return Promise.resolve({ ok: true, toolName }); + }, + } as RemoteToolSource; + + Object.defineProperty(source, "id", { + enumerable: false, + get: () => "docs", + }); + + return source; +} + +function hostToolSet(calls: string[] = []): HostToolSet { + return { + search_docs: { + description: "Search docs", + execute: (input: unknown) => { + calls.push(`search_docs:${String((input as Record).value)}`); + return { ok: true, toolName: "search_docs" }; + }, + }, + delete_docs: { + description: "Delete docs", + execute: (input: unknown) => { + calls.push(`delete_docs:${String((input as Record).value)}`); + return { ok: true, toolName: "delete_docs" }; + }, + }, + hidden_without_execute: { + description: "Metadata only", + }, + }; +} + +function assertPermissionDenied(error: unknown, detail: string) { + if (!(error instanceof VeryfrontError)) { + throw new Error("Expected VeryfrontError"); + } + + assertEquals(error.slug, "permission-denied"); + assertEquals(error.message, detail); + assertEquals(error.detail, detail); +} + +function captureThrown(fn: () => unknown): unknown { + try { + fn(); + } catch (error) { + return error; + } + + throw new Error("Expected function to throw"); +} + +describe("agent/mcp-tool-policy", () => { + it("createMcpToolPolicyGate(undefined) allows all names", () => { + const gate = createMcpToolPolicyGate(undefined); + + assertEquals(gate.allows("search_docs"), true); + assertEquals(gate.allows("delete_docs"), true); + assertEquals(gate.filterDefinitions([remoteTool("search_docs"), remoteTool("delete_docs")]), [ + remoteTool("search_docs"), + remoteTool("delete_docs"), + ]); + gate.assertAllowed("delete_docs"); + }); + + it("deny wins over allow", () => { + const gate = createMcpToolPolicyGate({ + allow: ["search_docs", "delete_docs"], + deny: ["delete_docs"], + }); + + assertEquals(gate.allows("search_docs"), true); + assertEquals(gate.allows("delete_docs"), false); + assertEquals(gate.filterDefinitions([remoteTool("search_docs"), remoteTool("delete_docs")]), [ + remoteTool("search_docs"), + ]); + }); + + it("allow filters definition order without sorting", () => { + const gate = createMcpToolPolicyGate({ + allow: ["beta", "alpha"], + }); + + assertEquals( + gate.filterDefinitions([remoteTool("gamma"), remoteTool("alpha"), remoteTool("beta")]).map(( + tool, + ) => tool.name), + ["alpha", "beta"], + ); + }); + + it('approval: "never" does not affect allow and deny behavior', () => { + const gate = createMcpToolPolicyGate({ + allow: ["search_docs"], + deny: ["delete_docs"], + approval: "never", + }); + + assertEquals(gate.allows("search_docs"), true); + assertEquals(gate.allows("delete_docs"), false); + assertEquals(gate.allows("list_docs"), false); + }); + + it("reads policy mutations after gate creation", () => { + const policy: AgentMcpToolPolicy = { allow: ["search_docs"], deny: ["delete_docs"] }; + const gate = createMcpToolPolicyGate(policy, { + deniedDetail: (toolName) => `Denied ${toolName}`, + }); + + assertEquals(gate.allows("search_docs"), true); + assertEquals(gate.allows("write_docs"), false); + + policy.allow?.push("write_docs"); + policy.deny = ["search_docs"]; + + assertEquals(gate.allows("search_docs"), false); + assertEquals(gate.allows("write_docs"), true); + assertEquals( + gate.filterDefinitions([remoteTool("search_docs"), remoteTool("write_docs")]).map((tool) => + tool.name + ), + ["write_docs"], + ); + + const error = captureThrown(() => gate.assertAllowed("search_docs")); + assertPermissionDenied(error, "Denied search_docs"); + }); + + it("keeps gate callbacks usable when detached from the gate object", () => { + const gate = createMcpToolPolicyGate({ allow: ["search_docs"] }, { + deniedDetail: (toolName) => `Detached ${toolName}`, + }); + const { filterDefinitions, assertAllowed } = gate; + + assertEquals( + filterDefinitions([remoteTool("search_docs"), remoteTool("delete_docs")]).map((tool) => + tool.name + ), + ["search_docs"], + ); + assertAllowed("search_docs"); + + const error = captureThrown(() => assertAllowed("delete_docs")); + assertPermissionDenied(error, "Detached delete_docs"); + }); + + it("wrapRemoteToolSourceWithMcpPolicy returns the same source for empty policy", () => { + const source = remoteSource([remoteTool("search_docs")]); + + assertStrictEquals(wrapRemoteToolSourceWithMcpPolicy(source, undefined), source); + assertStrictEquals(wrapRemoteToolSourceWithMcpPolicy(source, {}), source); + assertStrictEquals(wrapRemoteToolSourceWithMcpPolicy(source, { approval: "never" }), source); + }); + + it("wrapped remote listTools filters dynamically", async () => { + const policy: AgentMcpToolPolicy = { allow: ["search_docs"] }; + const source = remoteSource([remoteTool("search_docs"), remoteTool("delete_docs")]); + const wrapped = wrapRemoteToolSourceWithMcpPolicy(source, policy); + + assertStrictEquals(wrapped.id, source.id); + assertEquals((await wrapped.listTools()).map((tool) => tool.name), ["search_docs"]); + + policy.allow = ["delete_docs"]; + + assertEquals((await wrapped.listTools()).map((tool) => tool.name), ["delete_docs"]); + }); + + it("wrapped remote source preserves a non-enumerable id", () => { + const source = remoteSourceWithNonEnumerableId([ + remoteTool("search_docs"), + remoteTool("delete_docs"), + ]); + const wrapped = wrapRemoteToolSourceWithMcpPolicy(source, { deny: ["delete_docs"] }, { + deniedDetail: (toolName, sourceId) => `Tool ${toolName} denied for ${sourceId}`, + }); + + assertEquals(Object.keys(source).includes("id"), false); + assertStrictEquals(wrapped.id, "docs"); + + const error = captureThrown(() => + wrapped.executeTool("delete_docs", { value: "blocked" }, { projectId: "project-1" }) + ); + assertPermissionDenied(error, "Tool delete_docs denied for docs"); + }); + + it("wrapped remote executeTool blocks denied names before calling the source", async () => { + const calls: string[] = []; + const source = remoteSource([remoteTool("search_docs"), remoteTool("delete_docs")], calls); + const wrapped = wrapRemoteToolSourceWithMcpPolicy(source, { deny: ["delete_docs"] }, { + deniedDetail: (toolName, sourceId) => `Tool ${toolName} denied for ${sourceId}`, + }); + const detail = "Tool delete_docs denied for docs"; + + assertStrictEquals(wrapped.id, source.id); + const error = captureThrown(() => + wrapped.executeTool("delete_docs", { value: "blocked" }, { projectId: "project-1" }) + ); + assertPermissionDenied(error, detail); + assertEquals(calls, []); + + assertEquals( + await wrapped.executeTool("search_docs", { value: "allowed" }, { projectId: "project-1" }), + { + ok: true, + toolName: "search_docs", + }, + ); + assertEquals(calls, ["search_docs:allowed:project-1"]); + }); + + it("wrapHostToolSetWithMcpPolicy filters visible Tools and blocks execution after policy mutation", async () => { + const calls: string[] = []; + const policy: AgentMcpToolPolicy = { allow: ["search_docs", "hidden_without_execute"] }; + const wrapped = wrapHostToolSetWithMcpPolicy(hostToolSet(calls), policy, { + deniedDetail: (toolName) => `Host tool ${toolName} denied`, + }); + + assertEquals(Object.keys(wrapped), ["search_docs", "hidden_without_execute"]); + assertEquals(await wrapped.search_docs?.execute?.({ value: "first" }), { + ok: true, + toolName: "search_docs", + }); + + policy.deny = ["search_docs"]; + + const error = captureThrown(() => wrapped.search_docs!.execute!({ value: "second" })); + assertPermissionDenied(error, "Host tool search_docs denied"); + assertEquals(calls, ["search_docs:first"]); + }); + + it("detail builder preserves exact caller-provided denial text", () => { + const detail = 'Tool "delete_docs" is not allowed for MCP server "docs"'; + const gate = createMcpToolPolicyGate({ deny: ["delete_docs"] }, { + deniedDetail: () => detail, + }); + + const error = captureThrown(() => gate.assertAllowed("delete_docs")); + + assertPermissionDenied(error, detail); + }); +}); diff --git a/src/agent/mcp-tool-policy.ts b/src/agent/mcp-tool-policy.ts new file mode 100644 index 0000000000..233eb9c4d9 --- /dev/null +++ b/src/agent/mcp-tool-policy.ts @@ -0,0 +1,100 @@ +import { PERMISSION_DENIED } from "#veryfront/errors"; +import type { HostToolSet, RemoteToolSource, ToolExecutionContext } from "#veryfront/tool"; +import type { AgentMcpToolPolicy } from "./types.ts"; + +export type McpToolPolicyGate = { + allows(toolName: string): boolean; + filterDefinitions(definitions: readonly T[]): T[]; + assertAllowed(toolName: string): void; +}; + +function isPolicyEmpty(policy: AgentMcpToolPolicy | undefined): boolean { + return policy?.allow === undefined && policy?.deny === undefined; +} + +function defaultDeniedDetail(toolName: string): string { + return `Tool "${toolName}" is not allowed for this run`; +} + +export function createMcpToolPolicyGate( + policy: AgentMcpToolPolicy | undefined, + options?: { deniedDetail?: (toolName: string) => string }, +): McpToolPolicyGate { + const deniedDetail = options?.deniedDetail ?? defaultDeniedDetail; + + const allows = (toolName: string): boolean => { + const deny = policy?.deny; + if (deny?.includes(toolName)) return false; + + const allow = policy?.allow; + if (allow !== undefined) return allow.includes(toolName); + + return true; + }; + + const filterDefinitions = (definitions: readonly T[]): T[] => + definitions.filter((definition) => allows(definition.name)); + + const assertAllowed = (toolName: string): void => { + if (allows(toolName)) return; + + throw PERMISSION_DENIED.create({ detail: deniedDetail(toolName) }); + }; + + return { allows, filterDefinitions, assertAllowed }; +} + +export function wrapRemoteToolSourceWithMcpPolicy( + source: RemoteToolSource, + policy: AgentMcpToolPolicy | undefined, + options?: { deniedDetail?: (toolName: string, sourceId: string) => string }, +): RemoteToolSource { + if (isPolicyEmpty(policy)) return source; + + const gate = createMcpToolPolicyGate(policy, { + deniedDetail: (toolName) => + options?.deniedDetail?.(toolName, source.id) ?? + defaultDeniedDetail(toolName), + }); + + return { + ...source, + id: source.id, + listTools: async (context) => gate.filterDefinitions(await source.listTools(context)), + executeTool: (toolName, args, context) => { + gate.assertAllowed(toolName); + return source.executeTool(toolName, args, context); + }, + }; +} + +export function wrapHostToolSetWithMcpPolicy( + tools: HostToolSet, + policy: AgentMcpToolPolicy | undefined, + options?: { deniedDetail?: (toolName: string) => string }, +): HostToolSet { + if (isPolicyEmpty(policy)) return tools; + + const gate = createMcpToolPolicyGate(policy, options); + const wrapped: HostToolSet = {}; + + for (const [toolName, definition] of Object.entries(tools)) { + if (!gate.allows(toolName)) continue; + + if (definition.execute === undefined) { + wrapped[toolName] = { ...definition }; + continue; + } + + const execute = definition.execute; + wrapped[toolName] = { + ...definition, + execute: (toolInput: unknown, execOptions?: ToolExecutionContext) => { + gate.assertAllowed(toolName); + return execute(toolInput, execOptions); + }, + }; + } + + return wrapped; +} diff --git a/src/agent/runtime/mcp-server-tool-sources.ts b/src/agent/runtime/mcp-server-tool-sources.ts index 1a372cd6f5..128587a57c 100644 --- a/src/agent/runtime/mcp-server-tool-sources.ts +++ b/src/agent/runtime/mcp-server-tool-sources.ts @@ -6,7 +6,7 @@ import { type RemoteToolSource, toolRegistry, } from "#veryfront/tool"; -import { CONFIG_INVALID, PERMISSION_DENIED } from "#veryfront/errors"; +import { CONFIG_INVALID } from "#veryfront/errors"; import type { AgentConfig, AgentHttpMcpServerConfig, @@ -14,13 +14,14 @@ import type { AgentMcpServerConfig, AgentVeryfrontMcpServerConfig, } from "../types.ts"; -import type { ToolDefinition, ToolExecutionContext } from "#veryfront/tool"; +import type { ToolExecutionContext } from "#veryfront/tool"; import type { SourceIntegrationPolicyManifest } from "#veryfront/integrations/source-policy.ts"; import { getVeryfrontCloudHostBootstrap, type VeryfrontCloudBootstrap, } from "#veryfront/platform/cloud/resolver.ts"; import { createAgentServiceRemoteMcpConfig } from "../service/mcp-server-config.ts"; +import { wrapRemoteToolSourceWithMcpPolicy } from "../mcp-tool-policy.ts"; import { getActiveRuntimeRemoteToolSources } from "./remote-tool-source-context.ts"; export type RuntimeRemoteToolConfig = { @@ -94,26 +95,6 @@ async function resolveHeaders( return await resolveValue(auth.headers, context); } -function isToolAllowed( - toolName: string, - policy: AgentMcpServerConfig["toolPolicy"], -): boolean { - if (policy?.allow && !policy.allow.includes(toolName)) { - return false; - } - if (policy?.deny?.includes(toolName)) { - return false; - } - return true; -} - -function filterToolDefinitions( - definitions: ToolDefinition[], - policy: AgentMcpServerConfig["toolPolicy"], -): ToolDefinition[] { - return definitions.filter((definition) => isToolAllowed(definition.name, policy)); -} - function isHttpMcpServerConfig(server: AgentMcpServerConfig): server is AgentHttpMcpServerConfig { return "transport" in server; } @@ -126,44 +107,19 @@ function createMcpServerToolSource(server: AgentHttpMcpServerConfig): RemoteTool ...(server.fetch ? { fetch: server.fetch } : {}), }); - return { - id: source.id, - async listTools(context) { - return filterToolDefinitions(await source.listTools(context), server.toolPolicy); - }, - executeTool(toolName, args, context) { - if (!isToolAllowed(toolName, server.toolPolicy)) { - throw PERMISSION_DENIED.create({ - detail: `Tool "${toolName}" is not allowed for MCP server "${server.id}"`, - }); - } - return source.executeTool(toolName, args, context); - }, - }; + return wrapRemoteToolSourceWithMcpPolicy(source, server.toolPolicy, { + deniedDetail: (toolName) => `Tool "${toolName}" is not allowed for MCP server "${server.id}"`, + }); } function createMcpToolPolicySource( source: RemoteToolSource, policy: AgentMcpServerConfig["toolPolicy"], ): RemoteToolSource { - if (!policy?.allow && !policy?.deny) { - return source; - } - - return { - id: source.id, - async listTools(context) { - return filterToolDefinitions(await source.listTools(context), policy); - }, - executeTool(toolName, args, context) { - if (!isToolAllowed(toolName, policy)) { - throw PERMISSION_DENIED.create({ - detail: `Tool "${toolName}" is not allowed for MCP server "${source.id}"`, - }); - } - return source.executeTool(toolName, args, context); - }, - }; + return wrapRemoteToolSourceWithMcpPolicy(source, policy, { + deniedDetail: (toolName, sourceId) => + `Tool "${toolName}" is not allowed for MCP server "${sourceId}"`, + }); } /** Carry an explicit remote-tool ceiling into nested execution. */