diff --git a/Frameworks/Resources/Localizable.xcstrings b/Frameworks/Resources/Localizable.xcstrings index f21ae388..3c3062a6 100644 --- a/Frameworks/Resources/Localizable.xcstrings +++ b/Frameworks/Resources/Localizable.xcstrings @@ -5439,20 +5439,20 @@ } } }, - "Connect to remote MCP servers over HTTP. New tools always ask for approval by default." : { + "Connect to remote MCP servers over HTTP. New read-only tools are approved by default; all others ask for approval." : { "comment" : "A description of MCP servers.", "isCommentAutoGenerated" : true, "localizations" : { "zh-Hans" : { "stringUnit" : { "state" : "translated", - "value" : "通过HTTP连接远程MCP服务器。新工具默认始终请求批准。" + "value" : "通过HTTP连接远程MCP服务器。新发现的只读工具默认批准，其他工具默认请求批准。" } }, "zh-Hant" : { "stringUnit" : { "state" : "translated", - "value" : "透過HTTP連線遠端MCP伺服器。新工具預設一律請求核准。" + "value" : "透過HTTP連線遠端MCP伺服器。新發現的唯讀工具預設核准，其他工具預設請求核准。" } } } @@ -7354,18 +7354,18 @@ } } }, - "Deny refuses the call. Ask uses the conversation's manual or automatic approval. Approve runs without approval and is unavailable for tools marked destructive." : { + "Deny refuses the call. Ask uses the conversation's manual or automatic approval. Approve runs without approval." : { "localizations" : { "zh-Hans" : { "stringUnit" : { "state" : "translated", - "value" : "“拒绝”会拒绝调用；“询问”使用会话的手动或自动审批；“批准”无需审批即可运行，且不适用于标记为破坏性的工具。" + "value" : "“拒绝”会拒绝调用；“询问”使用会话的手动或自动审批；“批准”无需审批即可运行。" } }, "zh-Hant" : { "stringUnit" : { "state" : "translated", - "value" : "「拒絕」會拒絕呼叫；「詢問」使用工作階段的手動或自動核准；「核准」無需核准即可執行，且不適用於標示為破壞性的工具。" + "value" : "「拒絕」會拒絕呼叫；「詢問」使用工作階段的手動或自動核准；「核准」無需核准即可執行。" } } } @@ -22217,18 +22217,18 @@ } } }, - "Termind connects directly from this Mac over HTTP. Tools ask for approval before running by default." : { + "Termind connects directly from this Mac over HTTP. New read-only tools are approved by default; all others ask for approval." : { "localizations" : { "zh-Hans" : { "stringUnit" : { "state" : "translated", - "value" : "Termind会通过HTTP从这台Mac直接连接。默认情况下，工具运行前会请求批准。" + "value" : "Termind会通过HTTP从这台Mac直接连接。新发现的只读工具默认批准，其他工具默认请求批准。" } }, "zh-Hant" : { "stringUnit" : { "state" : "translated", - "value" : "Termind會透過HTTP從這台Mac直接連線。預設情況下，工具執行前會請求核准。" + "value" : "Termind會透過HTTP從這台Mac直接連線。新發現的唯讀工具預設核准，其他工具預設請求核准。" } } } diff --git a/Frameworks/TermindApplication/Agent/MCPToolApprovalPolicy.swift b/Frameworks/TermindApplication/Agent/MCPToolApprovalPolicy.swift index 74acc9db..9fbd786f 100644 --- a/Frameworks/TermindApplication/Agent/MCPToolApprovalPolicy.swift +++ b/Frameworks/TermindApplication/Agent/MCPToolApprovalPolicy.swift @@ -1,29 +1,10 @@ import AgentKit nonisolated extension MCPTool { - public var allowsTermindPreapproval: Bool { - annotations.destructiveHint != true - } - - /// Termind never treats a remotely flagged destructive tool as pre-approved. - /// The hint cannot authorize anything, but it may tighten a local choice. - public var termindApprovalPolicy: AgentToolDescriptor.ApprovalPolicy { - !allowsTermindPreapproval && accessPolicy == .approve ? .ask : accessPolicy - } - - public mutating func enforceTermindApprovalPolicy() { - accessPolicy = termindApprovalPolicy - } -} - -nonisolated extension MCPServer { - public func enforcingTermindApprovalPolicies() -> MCPServer { - var server = self - server.tools = tools.map { tool in - var tool = tool - tool.enforceTermindApprovalPolicy() - return tool - } - return server + /// The local policy assigned when Termind discovers a genuinely new MCP tool. + /// Contradictory read-only and destructive hints fail closed to asking, but + /// users may explicitly approve the tool afterward. + public var termindDefaultApprovalPolicy: AgentToolDescriptor.ApprovalPolicy { + annotations.readOnlyHint == true && annotations.destructiveHint != true ? .approve : .ask } } diff --git a/Frameworks/TermindApplication/FeatureModels/AIConfigurationModel.swift b/Frameworks/TermindApplication/FeatureModels/AIConfigurationModel.swift index 57fa76e4..9910a82e 100644 --- a/Frameworks/TermindApplication/FeatureModels/AIConfigurationModel.swift +++ b/Frameworks/TermindApplication/FeatureModels/AIConfigurationModel.swift @@ -86,7 +86,7 @@ public final class AIConfigurationModel { self.webSearchPersistence = webSearchPersistence self.modelProviders = modelProviders self.endpoints = endpoints - self.mcpServers = mcpServers.map { $0.enforcingTermindApprovalPolicies() } + self.mcpServers = mcpServers self.roles = roles self.lastSelectedModel = lastSelectedModel self.lastSelectedPermissionMode = @@ -110,7 +110,7 @@ public final class AIConfigurationModel { self.endpoints = loaded.endpoints self.roles = loaded.roles self.lastSelectedModel = loaded.lastSelectedModel - self.mcpServers = loaded.mcpServers.map { $0.enforcingTermindApprovalPolicies() } + self.mcpServers = loaded.mcpServers } catch { storageError = error.localizedDescription } @@ -278,7 +278,7 @@ public final class AIConfigurationModel { endpoints = snapshot.endpoints roles = snapshot.roles lastSelectedModel = localSelection.flatMap { lookup($0) == nil ? nil : $0 } - mcpServers = snapshot.mcpServers.map { $0.enforcingTermindApprovalPolicies() } + mcpServers = snapshot.mcpServers mcpStatuses = mcpStatuses.filter { id, _ in previousServers[id] == mcpServers.first { $0.id == id } } @@ -557,7 +557,6 @@ public final class AIConfigurationModel { // MARK: - MCP writes public func upsert(_ server: MCPServer) { - let server = server.enforcingTermindApprovalPolicies() mutatePersistedState { if let index = mcpServers.firstIndex(where: { $0.id == server.id }) { mcpServers[index] = server @@ -598,7 +597,8 @@ public final class AIConfigurationModel { /// /// 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. + /// genuinely new tool uses its Termind default: a non-destructive tool with + /// `readOnlyHint == true` is approved, while every other tool asks. public func setTools(_ discovered: [MCPTool], version: String, for id: MCPServer.ID) { mutatePersistedState { guard let index = mcpServers.firstIndex(where: { $0.id == id }) else { return } @@ -608,8 +608,8 @@ public final class AIConfigurationModel { mcpServers[index].tools = discovered.map { tool in var merged = tool - if let old = previous[tool.id] { merged.accessPolicy = old.accessPolicy } - merged.enforceTermindApprovalPolicy() + merged.accessPolicy = + previous[tool.id]?.accessPolicy ?? merged.termindDefaultApprovalPolicy return merged } mcpServers[index].negotiatedVersion = version @@ -622,7 +622,6 @@ public final class AIConfigurationModel { ) { mutate(toolID, on: serverID) { $0.accessPolicy = policy - $0.enforceTermindApprovalPolicy() } } diff --git a/Memory/ARCHITECTURE.md b/Memory/ARCHITECTURE.md index 37dba0de..1258fd63 100644 --- a/Memory/ARCHITECTURE.md +++ b/Memory/ARCHITECTURE.md @@ -223,13 +223,17 @@ Every call has an approval policy before the run's permission mode is consulted: timeout, transport failure, or malformed review fails closed and is recorded. - Full access pre-approves Ask calls but retains host-key, secret, schema, path, fetch, sandbox, and capability boundaries. -- Termind downgrades Approve to Ask for MCP tools carrying a destructive hint. MCP - tools otherwise follow Deny, Ask, or Approve in both acting and planning runs. +- A genuinely new MCP tool starts at Approve only when its remote + `readOnlyHint` is true and its `destructiveHint` is not true; every other new + tool starts at Ask. Rediscovery preserves the user's stored policy, including + an explicit Approve choice for a destructive tool. +- MCP tools follow Deny, Ask, or Approve in both acting and planning runs. Skills are trusted procedures only because users install them through Settings; tools, MCP servers, and fetched pages cannot write or enable them. `allowed-tools` -and MCP annotations never authorize actions. The model receives enabled names and -descriptions at the context boundary, then loads bounded bodies through the +never authorizes actions. MCP annotations may seed the local approval policy for a +new tool, but cannot override a stored user choice. The model receives enabled names +and descriptions at the context boundary, then loads bounded bodies through the read-only `load_skill`; downstream reads remain untrusted. The enabled set is fixed per run. Built-ins seed once, deleted built-ins stay deleted, and reset replaces the library only after confirmation. diff --git a/TFI/Application/FeatureModels/MobileMCPEditorModel.swift b/TFI/Application/FeatureModels/MobileMCPEditorModel.swift index 3c8ebefc..dd31b4dc 100644 --- a/TFI/Application/FeatureModels/MobileMCPEditorModel.swift +++ b/TFI/Application/FeatureModels/MobileMCPEditorModel.swift @@ -77,8 +77,7 @@ final class MobileMCPEditorModel { var seen = Set() draft.tools = outcome.tools.filter { seen.insert($0.id).inserted }.map { var tool = $0 - tool.accessPolicy = policies[tool.id] ?? .ask - tool.enforceTermindApprovalPolicy() + tool.accessPolicy = policies[tool.id] ?? tool.termindDefaultApprovalPolicy return tool } draft.negotiatedVersion = outcome.protocolVersion diff --git a/TFI/Features/Settings/MobileMCPSettings.swift b/TFI/Features/Settings/MobileMCPSettings.swift index de1309d7..e662284f 100644 --- a/TFI/Features/Settings/MobileMCPSettings.swift +++ b/TFI/Features/Settings/MobileMCPSettings.swift @@ -36,7 +36,9 @@ struct MobileMCPSettings: View { } } } footer: { - Text("Connect to remote MCP servers over HTTP. New tools always ask for approval by default.") + Text( + "Connect to remote MCP servers over HTTP. New read-only tools are approved by default; all others ask for approval." + ) } } .navigationTitle("MCP") @@ -265,12 +267,11 @@ struct MobileMCPEditor: View { Text("Ask").tag(AgentToolDescriptor.ApprovalPolicy.ask) Text("Approve") .tag(AgentToolDescriptor.ApprovalPolicy.approve) - .disabled(!tool.allowsTermindPreapproval) } } } Text( - "Deny refuses the call. Ask uses the conversation's manual or automatic approval. Approve runs without approval and is unavailable for tools marked destructive." + "Deny refuses the call. Ask uses the conversation's manual or automatic approval. Approve runs without approval." ) .font(.caption).foregroundStyle(.secondary) } diff --git a/TFITests/MobileAISettingsTests.swift b/TFITests/MobileAISettingsTests.swift index 4ac33835..72b8309a 100644 --- a/TFITests/MobileAISettingsTests.swift +++ b/TFITests/MobileAISettingsTests.swift @@ -195,7 +195,20 @@ struct MobileAISettingsTests { #expect(request.url == "https://draft.invalid/mcp") #expect(token == "draft-token") return MCPDiscovery( - protocolVersion: "test", serverName: "Test", tools: [MCPTool(id: "existing"), MCPTool(id: "new")]) + protocolVersion: "test", serverName: "Test", + tools: [ + MCPTool(id: "existing"), + MCPTool(id: "new"), + MCPTool( + id: "read-only", + annotations: MCPToolAnnotations(readOnlyHint: true) + ), + MCPTool( + id: "conflicting", + annotations: MCPToolAnnotations(readOnlyHint: true, destructiveHint: true) + ), + ] + ) }) editor.draft.url = "https://draft.invalid/mcp" editor.key.action = .replace @@ -203,10 +216,12 @@ struct MobileAISettingsTests { editor.connect() while editor.running { await Task.yield() } #expect(ai.mcpServers.isEmpty) - #expect(editor.draft.tools.map(\.accessPolicy) == [.deny, .ask]) + #expect(editor.draft.tools.map(\.accessPolicy) == [.deny, .ask, .approve, .ask]) + let conflictingIndex = try #require(editor.draft.tools.firstIndex { $0.id == "conflicting" }) + editor.draft.tools[conflictingIndex].accessPolicy = .approve try editor.save() #expect(ai.mcpServer(server.id)?.namespaceID == "gateway") - #expect(ai.mcpServer(server.id)?.tools.map(\.accessPolicy) == [.deny, .ask]) + #expect(ai.mcpServer(server.id)?.tools.map(\.accessPolicy) == [.deny, .ask, .approve, .approve]) } @Test func mcpCancellationAndRemoteDeletionCannotRestoreResults() async { diff --git a/TFM/Features/Settings/MCPServerEditorSheet.swift b/TFM/Features/Settings/MCPServerEditorSheet.swift index eb3531de..4d517806 100644 --- a/TFM/Features/Settings/MCPServerEditorSheet.swift +++ b/TFM/Features/Settings/MCPServerEditorSheet.swift @@ -26,14 +26,10 @@ final class MCPServerEditorSession { /// /// The tool list is the important half. It only exists after a successful /// Connect, because it is what the server said rather than what the user typed — -/// and each row carries two switches, one for whether the tool is offered to the -/// model at all and one for whether calling it needs a click. -/// -/// That second switch defaults to on and is the reason this sheet is worth -/// building now rather than at P3. Termind's whole premise is that nothing runs -/// without the user's say-so; a tool list fetched over the network from a server -/// the model can also talk to is precisely where that premise would spring a -/// leak if approval were opt-in. +/// and each row carries a local policy for whether the tool is denied, follows +/// the conversation's approval mode, or runs without another approval. New +/// non-destructive tools carrying `readOnlyHint == true` begin approved; every +/// other new tool begins by asking, and rediscovery preserves the user's choice. /// /// Edits write straight through and the page returns to the MCP list in place. struct MCPServerEditorPage: View { @@ -245,7 +241,7 @@ struct MCPServerEditorPage: View { } footer: { if !server.tools.isEmpty { Text( - "Deny refuses the call. Ask uses the conversation's manual or automatic approval. Approve runs without approval and is unavailable for tools marked destructive." + "Deny refuses the call. Ask uses the conversation's manual or automatic approval. Approve runs without approval." ) .font(.caption) .foregroundStyle(.secondary) @@ -375,7 +371,6 @@ private struct ToolRow: View { ForEach(AgentToolDescriptor.ApprovalPolicy.allCases) { option in Text(option.displayName) .tag(option) - .disabled(option == .approve && !tool.allowsTermindPreapproval) } } .labelsHidden() diff --git a/TFM/Features/Settings/MCPSettings.swift b/TFM/Features/Settings/MCPSettings.swift index 92e7c087..976ee32a 100644 --- a/TFM/Features/Settings/MCPSettings.swift +++ b/TFM/Features/Settings/MCPSettings.swift @@ -33,7 +33,7 @@ struct MCPSettings: View { } Text( - "Termind connects directly from this Mac over HTTP. Tools ask for approval before running by default." + "Termind connects directly from this Mac over HTTP. New read-only tools are approved by default; all others ask for approval." ) .font(.caption) .foregroundStyle(.secondary) diff --git a/TFMTests/AIProviderPersistenceTests.swift b/TFMTests/AIProviderPersistenceTests.swift index 4f70f3b1..a00936e3 100644 --- a/TFMTests/AIProviderPersistenceTests.swift +++ b/TFMTests/AIProviderPersistenceTests.swift @@ -81,7 +81,13 @@ struct AIProviderPersistenceTests { store.upsert(server) store.setTools( - [MCPTool(id: "existing"), MCPTool(id: "new")], + [ + MCPTool(id: "existing"), MCPTool(id: "new"), + MCPTool( + id: "read-only", + annotations: MCPToolAnnotations(readOnlyHint: true) + ), + ], version: "2025-06-18", for: server.id ) @@ -92,6 +98,7 @@ struct AIProviderPersistenceTests { let tools = try #require(reloaded.mcpServer(server.id)?.tools) #expect(tools.first { $0.id == "existing" }?.accessPolicy == .approve) #expect(tools.first { $0.id == "new" }?.accessPolicy == .ask) + #expect(tools.first { $0.id == "read-only" }?.accessPolicy == .approve) #expect(reloaded.mcpServer(server.id)?.negotiatedVersion == "2025-06-18") #expect(reloaded.mcpServer(server.id)?.lastConnectedAt != nil) } diff --git a/TFMTests/AgentRuntimeTests.swift b/TFMTests/AgentRuntimeTests.swift index e7afc946..5ef4945e 100644 --- a/TFMTests/AgentRuntimeTests.swift +++ b/TFMTests/AgentRuntimeTests.swift @@ -919,7 +919,7 @@ struct AgentRuntimeTests { @Test func mcpToolAccessPoliciesControlApprovalWithoutHidingTools() throws { var server = MCPServer(name: "Gateway") - var destructive = MCPTool(id: "destructive", accessPolicy: .ask) + var destructive = MCPTool(id: "destructive", accessPolicy: .approve) destructive.annotations.destructiveHint = true server.tools = [ MCPTool(id: "denied", accessPolicy: .deny), @@ -934,6 +934,7 @@ struct AgentRuntimeTests { #expect(descriptors.first { $0.name == "denied" }?.approvalPolicy == .deny) #expect(descriptors.first { $0.name == "asked" }?.approvalPolicy == .ask) #expect(descriptors.first { $0.name == "approved" }?.approvalPolicy == .approve) + #expect(descriptors.first { $0.name == "destructive" }?.approvalPolicy == .approve) #expect(descriptors.first { $0.name == "destructive" }?.concurrency == .sequential) } diff --git a/TFMTests/SettingsNavigationTests.swift b/TFMTests/SettingsNavigationTests.swift index af7d849c..9931f957 100644 --- a/TFMTests/SettingsNavigationTests.swift +++ b/TFMTests/SettingsNavigationTests.swift @@ -194,18 +194,48 @@ struct SettingsNavigationTests { func mcpToolAccessPolicyIsAtomicAndSurvivesRediscovery() throws { var existingTool = MCPTool(id: "search") existingTool.accessPolicy = .deny + let existingAsk = MCPTool(id: "existing-ask", accessPolicy: .ask) + let existingApprove = MCPTool(id: "existing-approve", accessPolicy: .approve) var server = MCPServer(name: "Gateway") - server.tools = [existingTool] + server.tools = [existingTool, existingAsk, existingApprove] let store = AIConfigurationModel(mcpServers: [server], persistsSecrets: false) store.setTools( - [MCPTool(id: "search", title: "Updated"), MCPTool(id: "new-tool")], + [ + MCPTool( + id: "search", title: "Updated", + annotations: MCPToolAnnotations(readOnlyHint: true) + ), + MCPTool( + id: "existing-ask", + annotations: MCPToolAnnotations(readOnlyHint: true) + ), + MCPTool(id: "existing-approve"), + MCPTool( + id: "new-read-only", + annotations: MCPToolAnnotations(readOnlyHint: true) + ), + MCPTool( + id: "new-not-read-only", + annotations: MCPToolAnnotations(readOnlyHint: false) + ), + MCPTool(id: "new-without-hint"), + MCPTool( + id: "new-conflicting-hints", + annotations: MCPToolAnnotations(readOnlyHint: true, destructiveHint: true) + ), + ], version: "2025-06-18", for: server.id ) let rediscovered = try #require(store.mcpServer(server.id)) #expect(rediscovered.tools.first { $0.id == "search" }?.accessPolicy == .deny) - #expect(rediscovered.tools.first { $0.id == "new-tool" }?.accessPolicy == .ask) + #expect(rediscovered.tools.first { $0.id == "existing-ask" }?.accessPolicy == .ask) + #expect(rediscovered.tools.first { $0.id == "existing-approve" }?.accessPolicy == .approve) + #expect(rediscovered.tools.first { $0.id == "new-read-only" }?.accessPolicy == .approve) + #expect(rediscovered.tools.first { $0.id == "new-not-read-only" }?.accessPolicy == .ask) + #expect(rediscovered.tools.first { $0.id == "new-without-hint" }?.accessPolicy == .ask) + #expect(rediscovered.tools.first { $0.id == "new-conflicting-hints" }?.accessPolicy == .ask) store.setToolAccessPolicy(.approve, tool: "search", on: server.id) #expect( @@ -215,7 +245,7 @@ struct SettingsNavigationTests { } @Test - func destructiveMCPToolsCannotRemainApprovedAndDeniedToolsStayAvailable() throws { + func destructiveMCPToolsCanBeApprovedAndDeniedToolsStayAvailable() throws { var destructive = MCPTool(id: "destroy", accessPolicy: .approve) destructive.annotations.destructiveHint = true let denied = MCPTool(id: "denied", accessPolicy: .deny) @@ -223,17 +253,16 @@ struct SettingsNavigationTests { let store = AIConfigurationModel(mcpServers: [server], persistsSecrets: false) let normalized = try #require(store.mcpServer(server.id)) - #expect(!normalized.tools[0].allowsTermindPreapproval) - #expect(normalized.tools[0].accessPolicy == .ask) + #expect(normalized.tools[0].accessPolicy == .approve) #expect(store.availableTools.map(\.tool.id).contains("denied")) store.setToolAccessPolicy(.approve, tool: "destroy", on: server.id) - #expect(store.mcpServer(server.id)?.tools[0].accessPolicy == .ask) + #expect(store.mcpServer(server.id)?.tools[0].accessPolicy == .approve) var newlyDestructive = MCPTool(id: "denied", accessPolicy: .ask) newlyDestructive.annotations.destructiveHint = true store.setToolAccessPolicy(.approve, tool: "denied", on: server.id) store.setTools([newlyDestructive], version: "2025-06-18", for: server.id) - #expect(store.mcpServer(server.id)?.tools[0].accessPolicy == .ask) + #expect(store.mcpServer(server.id)?.tools[0].accessPolicy == .approve) } }