diff --git a/Termind/Application/Agent/AgentMCPTools.swift b/Termind/Application/Agent/AgentMCPTools.swift index 6b6f3474..cbb2f990 100644 --- a/Termind/Application/Agent/AgentMCPTools.swift +++ b/Termind/Application/Agent/AgentMCPTools.swift @@ -36,11 +36,12 @@ nonisolated enum AgentMCPTools { // make this *stricter*. Nothing a server asserts can grant // parallelism, so the rule that remote annotations are risk hints // rather than proof of safety still holds — and a tool the server - // marks destructive, or wants asked about every time, takes its + // marks destructive, or is configured to ask every time, takes its // whole turn serial. - concurrency: tool.annotations.destructiveHint == true || tool.requiresApproval + concurrency: tool.annotations.destructiveHint == true + || tool.accessPolicy == .alwaysAsk ? .sequential : .parallel, - alwaysAskUser: tool.requiresApproval, + alwaysAskUser: tool.accessPolicy == .alwaysAsk, presentation: .init( symbol: "puzzlepiece.extension", activity: .label(MCPToolNaming.qualifiedName( diff --git a/Termind/Application/FeatureModels/AIConfigurationModel.swift b/Termind/Application/FeatureModels/AIConfigurationModel.swift index e497553c..71e087fc 100644 --- a/Termind/Application/FeatureModels/AIConfigurationModel.swift +++ b/Termind/Application/FeatureModels/AIConfigurationModel.swift @@ -458,11 +458,9 @@ final class AIConfigurationModel { /// What a successful `Connect` writes back. /// - /// A tool the user had already switched off, or marked as not needing - /// approval, keeps those settings across a rediscovery — otherwise every - /// reconnect would silently undo the decisions made about this server. A - /// tool that has genuinely appeared for the first time arrives at the - /// defaults, which means approval required. + /// A tool keeps its access policy across rediscovery — otherwise every + /// reconnect would silently undo the decision made about this server. A + /// genuinely new tool arrives at the default, which means always asking. func setTools(_ discovered: [MCPTool], version: String, for id: MCPServer.ID) { guard let index = mcpServers.firstIndex(where: { $0.id == id }) else { return } let previous = Dictionary(uniqueKeysWithValues: mcpServers[index].tools.map { ($0.id, $0) }) @@ -470,20 +468,17 @@ final class AIConfigurationModel { mcpServers[index].tools = discovered.map { tool in guard let old = previous[tool.id] else { return tool } var merged = tool - merged.isEnabled = old.isEnabled - merged.requiresApproval = old.requiresApproval + merged.accessPolicy = old.accessPolicy return merged } mcpServers[index].negotiatedVersion = version mcpServers[index].lastConnectedAt = Date() } - func setToolEnabled(_ enabled: Bool, tool toolID: String, on serverID: MCPServer.ID) { - mutate(toolID, on: serverID) { $0.isEnabled = enabled } - } - - func setToolRequiresApproval(_ required: Bool, tool toolID: String, on serverID: MCPServer.ID) { - mutate(toolID, on: serverID) { $0.requiresApproval = required } + func setToolAccessPolicy( + _ policy: MCPTool.AccessPolicy, tool toolID: String, on serverID: MCPServer.ID + ) { + mutate(toolID, on: serverID) { $0.accessPolicy = policy } } private func mutate(_ toolID: String, on serverID: MCPServer.ID, _ change: (inout MCPTool) -> Void) { diff --git a/Termind/Domain/AI/MCPServer.swift b/Termind/Domain/AI/MCPServer.swift index 62a91442..77730088 100644 --- a/Termind/Domain/AI/MCPServer.swift +++ b/Termind/Domain/AI/MCPServer.swift @@ -88,7 +88,7 @@ nonisolated struct MCPServer: Identifiable, Hashable, Sendable { } var enabledTools: [MCPTool] { - tools.filter(\.isEnabled) + tools.filter { $0.accessPolicy != .disabled } } var isReachable: Bool { @@ -182,14 +182,19 @@ nonisolated struct MCPHeader: Identifiable, Hashable, Sendable { /// One tool a server offers. /// -/// `requiresApproval` defaults to `true`, and that default is the point rather -/// than caution. Termind's rule is that nothing runs on the user's say-so alone — -/// an AI-proposed command is always a click away from the PTY, never closer. An -/// MCP tool is a command by another name, and it arrives from a list fetched -/// over the network from a server the model can talk to. If the default went -/// the other way, the one rule the whole feature rests on would have a hole in -/// it that nobody opened deliberately. +/// `accessPolicy` defaults to `alwaysAsk`, and that default is the point rather +/// than caution. An MCP tool is a command fetched over the network from a server +/// the model can talk to, so a newly discovered tool must not silently inherit a +/// more permissive conversation setting. nonisolated struct MCPTool: Identifiable, Hashable, Sendable { + nonisolated enum AccessPolicy: String, CaseIterable, Identifiable, Hashable, Sendable { + case disabled + case alwaysAsk + case followPermissions + + nonisolated var id: String { rawValue } + } + /// The tool name as the server reports it. Its identity, since MCP names /// are unique per server. var id: String @@ -199,8 +204,7 @@ nonisolated struct MCPTool: Identifiable, Hashable, Sendable { "type": .string("object"), "properties": .object([:]) ]) var annotations: MCPToolAnnotations = MCPToolAnnotations() - var isEnabled: Bool = true - var requiresApproval: Bool = true + var accessPolicy: AccessPolicy = .alwaysAsk func matches(_ query: String) -> Bool { guard !query.isEmpty else { return true } diff --git a/Termind/Features/Settings/MCPServerEditorSheet.swift b/Termind/Features/Settings/MCPServerEditorSheet.swift index ad97a429..7ba51d93 100644 --- a/Termind/Features/Settings/MCPServerEditorSheet.swift +++ b/Termind/Features/Settings/MCPServerEditorSheet.swift @@ -243,7 +243,7 @@ struct MCPServerEditorPage: View { } footer: { if !server.tools.isEmpty { Text( - "With Ask First on, you approve each call. Turn it off only for trusted, read-only tools." + "Always Ask requires approval for every call. Follow Permissions uses the conversation's permission mode and safety checks." ) .font(.caption) .foregroundStyle(.secondary) @@ -353,7 +353,7 @@ private struct ToolRow: View { } var body: some View { - HStack(alignment: .top, spacing: Metrics.Space.m) { + HStack(alignment: .center, spacing: Metrics.Space.m) { VStack(alignment: .leading, spacing: 1) { Text(qualifiedToolName) .font(.callout.monospaced()) @@ -364,41 +364,41 @@ private struct ToolRow: View { .font(.caption) .foregroundStyle(.secondary) .lineLimit(2) - .fixedSize(horizontal: false, vertical: true) } } + .frame(minWidth: 0, maxWidth: .infinity, alignment: .leading) - Spacer(minLength: Metrics.Space.m) - - Toggle("Offer", isOn: enabled) - .toggleStyle(.switch) - .controlSize(.mini) - .fixedSize() - - Toggle("Ask First", isOn: asksFirst) - .toggleStyle(.switch) - .controlSize(.mini) - .fixedSize() - // Greyed rather than hidden when the tool is off: the setting - // still exists and comes back with it, and a control that - // vanishes reads as one that was reset. - .disabled(!tool.isEnabled) + Picker("Permission", selection: permission) { + ForEach(MCPTool.AccessPolicy.allCases) { option in + Text(option.displayName).tag(option) + } + } + .labelsHidden() + .pickerStyle(.menu) + .fixedSize() + .accessibilityLabel(Text("Permission")) } + .frame(maxWidth: .infinity) .font(.caption) .help(Text(tool.summary.isEmpty ? qualifiedToolName : tool.summary)) } - private var enabled: Binding { + private var permission: Binding { Binding( - get: { tool.isEnabled }, - set: { app.aiConfiguration.setToolEnabled($0, tool: tool.id, on: serverID) } - ) - } - - private var asksFirst: Binding { - Binding( - get: { tool.requiresApproval }, - set: { app.aiConfiguration.setToolRequiresApproval($0, tool: tool.id, on: serverID) } + get: { tool.accessPolicy }, + set: { policy in + app.aiConfiguration.setToolAccessPolicy(policy, tool: tool.id, on: serverID) + } ) } } + +private extension MCPTool.AccessPolicy { + var displayName: LocalizedStringKey { + switch self { + case .disabled: "Disabled" + case .alwaysAsk: "Always Ask" + case .followPermissions: "Follow Permissions" + } + } +} diff --git a/Termind/Infrastructure/MCP/MCPClient.swift b/Termind/Infrastructure/MCP/MCPClient.swift index e3f78c45..1db4aac7 100644 --- a/Termind/Infrastructure/MCP/MCPClient.swift +++ b/Termind/Infrastructure/MCP/MCPClient.swift @@ -17,7 +17,7 @@ nonisolated struct MCPCallResult: Hashable, Sendable { /// Discovery only. This runs the handshake and `tools/list`, and stops there — /// actually *calling* a tool belongs with the chat loop in P3, and it will go /// through the same approval gate an AI-proposed command does. That gate is why -/// `MCPTool.requiresApproval` arrives from here already set. +/// a discovered `MCPTool` arrives with its access policy set to always ask. /// /// Two transports, both HTTP. There is no stdio here because Termind is sandboxed /// and cannot launch a local process — see `MCPServer`. @@ -477,7 +477,7 @@ nonisolated private struct ToolsListResult: Decodable { var inputSchema: AgentJSONValue? var annotations: MCPToolAnnotations? - /// Arrives enabled and requiring approval. See `MCPTool`. + /// Arrives with the always-ask policy. See `MCPTool`. func tool() -> MCPTool { MCPTool( id: name, title: title ?? "", summary: description ?? title ?? "", diff --git a/Termind/Resources/Localizable.xcstrings b/Termind/Resources/Localizable.xcstrings index 71f924df..460c81da 100644 --- a/Termind/Resources/Localizable.xcstrings +++ b/Termind/Resources/Localizable.xcstrings @@ -1371,6 +1371,16 @@ } } }, + "Always Ask" : { + "localizations" : { + "zh-Hans" : { + "stringUnit" : { + "state" : "translated", + "value" : "始终询问" + } + } + } + }, "Always included" : { "localizations" : { "zh-Hans" : { @@ -4633,6 +4643,16 @@ } } }, + "Follow Permissions" : { + "localizations" : { + "zh-Hans" : { + "stringUnit" : { + "state" : "translated", + "value" : "跟随权限设置" + } + } + } + }, "Follow System" : { "localizations" : { "zh-Hans" : { @@ -12786,12 +12806,12 @@ } } }, - "With Ask First on, you approve each call. Turn it off only for trusted, read-only tools." : { + "Always Ask requires approval for every call. Follow Permissions uses the conversation's permission mode and safety checks." : { "localizations" : { "zh-Hans" : { "stringUnit" : { "state" : "translated", - "value" : "开启“先询问”后，每次调用都需你批准。仅对可信的只读工具关闭此选项。" + "value" : "“始终询问”要求每次调用都由你批准；“跟随权限设置”则使用当前会话的权限模式和安全检查。" } } } diff --git a/TermindTests/AgentRuntimeTests.swift b/TermindTests/AgentRuntimeTests.swift index 7d09e476..dfc5da63 100644 --- a/TermindTests/AgentRuntimeTests.swift +++ b/TermindTests/AgentRuntimeTests.swift @@ -775,6 +775,33 @@ struct AgentRuntimeTests { }) } + @Test + func mcpToolAccessPoliciesControlExposureAndApproval() throws { + var server = MCPServer(name: "Gateway") + server.tools = [ + MCPTool(id: "disabled", accessPolicy: .disabled), + MCPTool(id: "always-ask", accessPolicy: .alwaysAsk), + MCPTool(id: "follow-permissions", accessPolicy: .followPermissions), + ] + + let descriptors = AgentMCPTools.make(servers: [(server: server, bearerToken: nil)]) + .map(\.descriptor) + #expect(descriptors.count == 2) + #expect(!descriptors.contains { $0.name == "disabled" }) + + let alwaysAsk = try #require(descriptors.first { $0.name == "always-ask" }) + #expect(alwaysAsk.safety == .requiresAuthorization) + #expect(alwaysAsk.alwaysAskUser) + #expect(alwaysAsk.concurrency == .sequential) + + let followPermissions = try #require( + descriptors.first { $0.name == "follow-permissions" } + ) + #expect(followPermissions.safety == .requiresAuthorization) + #expect(!followPermissions.alwaysAskUser) + #expect(followPermissions.concurrency == .parallel) + } + @Test func secretHandlesAreSingleUseAndBindingScoped() async throws { let broker = SecretBroker() diff --git a/TermindTests/SettingsNavigationTests.swift b/TermindTests/SettingsNavigationTests.swift index f79214c2..34fd242f 100644 --- a/TermindTests/SettingsNavigationTests.swift +++ b/TermindTests/SettingsNavigationTests.swift @@ -83,4 +83,28 @@ struct SettingsNavigationTests { #expect(!store.lockMCPNamespace(for: duplicate.id)) #expect(store.mcpServer(duplicate.id)?.namespaceID == "") } + + @Test + func mcpToolAccessPolicyIsAtomicAndSurvivesRediscovery() throws { + var existingTool = MCPTool(id: "search") + existingTool.accessPolicy = .disabled + var server = MCPServer(name: "Gateway") + server.tools = [existingTool] + let store = AIConfigurationModel(mcpServers: [server], persistsSecrets: false) + + store.setTools( + [MCPTool(id: "search", title: "Updated"), MCPTool(id: "new-tool")], + version: "2025-06-18", for: server.id + ) + + let rediscovered = try #require(store.mcpServer(server.id)) + #expect(rediscovered.tools.first { $0.id == "search" }?.accessPolicy == .disabled) + #expect(rediscovered.tools.first { $0.id == "new-tool" }?.accessPolicy == .alwaysAsk) + + store.setToolAccessPolicy(.followPermissions, tool: "search", on: server.id) + #expect( + store.mcpServer(server.id)?.tools.first { $0.id == "search" }?.accessPolicy + == .followPermissions + ) + } }